<html xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40"> <head> <meta http-equiv="Content-Type" content="text/html; charset=Windows-1252"> <meta name="Generator" content="Microsoft Word 15 (filtered medium)"> <style></style> </head> <body lang="EN-US" link="blue" vlink="#954F72" style="word-wrap:break-word"> <div class="WordSection1"> Question (since it’s been brought up): when *wouldn’t* you use EFI_*? They’re clearly superior in every way. I mean, they’ve got EFI right in the name. <o:p> </o:p> - Bret <o:p></o:p> <o:p> </o:p> <div style="mso-element:para-border-div;border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in"> From: <a href="mailto:jiewen.yao=intel.com@groups.io">Yao, Jiewen via groups.io</a> Sent: Friday, August 28, 2020 5:20 PM To: <a href="mailto:lersek@redhat.com">Laszlo Ersek</a>; <a href="mailto:devel@edk2.groups.io"> devel@edk2.groups.io</a>; <a href="mailto:qi1.zhang@intel.com">Zhang, Qi1</a> Cc: <a href="mailto:jian.j.wang@intel.com">Wang, Jian J</a>; <a href="mailto:hao.a.wu@intel.com"> Wu, Hao A</a> Subject: [EXTERNAL] Re: [edk2-devel] [PATCH] MdeModulePkg/Library: change TpmMeasurementLibNull to BASE library. </div> <o:p> </o:p> Laszlo Good feedback. > The reason is that this change actually requires us to change the lib > class header too. Consider: the whole motivation for the patch is that a > client module that is more primitive than either a PEIM or a DXE_DRIVER > wants to consume the lib instance. That requires that the lib class > header be first consumable by the client module. And for that, the lib > class header must not declare the interface with EFI_xxx in the first > place, but with RETURN_xxx. [Jiewen] But I don’t think it is absolutely necessary to change EFI_xxx to RETURN_xxx in library class, just because a library instance could be PEI and DXE. EFI_xxx is legal for both PEI and DXE. That means, another way to fix the issue is to *add* PEIM and SEC to the LIBRARY_CLASS, instead of *remove* them. Thank you Yao Jiewen > -----Original Message----- > From: Laszlo Ersek <lersek@redhat.com> > Sent: Saturday, August 29, 2020 6:59 AM > To: devel@edk2.groups.io; Zhang, Qi1 <qi1.zhang@intel.com> > Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; > Yao, Jiewen <jiewen.yao@intel.com> > Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/Library: change > TpmMeasurementLibNull to BASE library. > > On 08/28/20 19:17, Laszlo Ersek wrote: > > On 08/28/20 08:15, Qi Zhang wrote: > >> REF: <a href="https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2940&data=02%7C01%7Cbret.barkelew%40microsoft.com%7C7ae38c56bf854c2ea4c408d84bb147c7%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637342572075610295&sdata=DEVDpeDBr5mTYuA0NdqgmGBUAdbQF1qDK2TuujmeSiQ%3D&reserved=0"> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2940&data=02%7C01%7Cbret.barkelew%40microsoft.com%7C7ae38c56bf854c2ea4c408d84bb147c7%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637342572075610295&sdata=DEVDpeDBr5mTYuA0NdqgmGBUAdbQF1qDK2TuujmeSiQ%3D&reserved=0</a> > >> > >> TpmMeasurementLib includes DxeTpmMeasurementLib and > PeiTpmMeasurementLib. > >> So need to change TpmMeasurementLibNull to BASE library to avoid build > >> error in some platform. > >> > >> Signed-off-by: Qi Zhang <qi1.zhang@intel.com> > >> Cc: Jian J Wang <jian.j.wang@intel.com> > >> Cc: Hao A Wu <hao.a.wu@intel.com> > >> Cc: Jiewen Yao <jiewen.yao@intel.com> > >> --- > >> .../Library/TpmMeasurementLibNull/TpmMeasurementLibNull.c | 4 +++- > >> .../Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf | 6 +++--- > >> 2 files changed, 6 insertions(+), 4 deletions(-) > >> > >> diff --git > a/MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.c > b/MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.c > >> index b9c5b68de8..ee3be62fc6 100644 > >> --- > a/MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.c > >> +++ > b/MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.c > >> @@ -1,11 +1,13 @@ > >> /** @file > >> This library is used by other modules to measure data to TPM. > >> > >> -Copyright (c) 2015, Intel Corporation. All rights reserved. > >> +Copyright (c) 2015-2020, Intel Corporation. All rights reserved. > >> SPDX-License-Identifier: BSD-2-Clause-Patent > >> > >> **/ > >> > >> +#include <Uefi/UefiBaseType.h> > >> + > >> /** > >> Tpm measure and log data, and extend the measurement result into a > specific PCR. > >> > >> diff --git > a/MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.in > f > b/MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.in > f > >> index 61abcfa2ec..1db2c0d6a7 100644 > >> --- > a/MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.in > f > >> +++ > b/MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.in > f > >> @@ -1,7 +1,7 @@ > >> ## @file > >> # Provides NULL TPM measurement function. > >> # > >> -# Copyright (c) 2015 - 2018, Intel Corporation. All rights reserved. > >> +# Copyright (c) 2015 - 2020, Intel Corporation. All rights reserved. > >> # SPDX-License-Identifier: BSD-2-Clause-Patent > >> # > >> ## > >> @@ -10,9 +10,9 @@ > >> INF_VERSION = 0x00010005 > >> BASE_NAME = TpmMeasurementLibNull > >> FILE_GUID = 6DFD6E9F-9278-48D8-8F45-B6CFF2C2B69C > >> - MODULE_TYPE = UEFI_DRIVER > >> + MODULE_TYPE = BASE > >> VERSION_STRING = 1.0 > >> - LIBRARY_CLASS = TpmMeasurementLib|DXE_DRIVER > DXE_RUNTIME_DRIVER DXE_SMM_DRIVER UEFI_APPLICATION UEFI_DRIVER > >> + LIBRARY_CLASS = TpmMeasurementLib > >> MODULE_UNI_FILE = TpmMeasurementLibNull.uni > >> > >> # > >> > > > > (1) I agree this is a bugfix, and should be included in the stable tag. > > > > > > (2) The commit message makes zero sense to me, on the other hand. I > > don't understand how DxeTpmMeasurementLib and PeiTpmMeasurementLib > are > > relevant at all. I also don't understand how TpmMeasurementLib > > "includes" DxeTpmMeasurementLib and PeiTpmMeasurementLib. > > > > I guess the intent is to say that *some* of the known TpmMeasurementLib > > instances are PeiTpmMeasurementLib and DxeTpmMeasurementLib. I guess > > that would be a valid statement, but it's still irrelevant here. > > > > The issue here is that *all* Null instances (regardless of library > > class) should have MODULE_TYPE=BASE, so that they can be consumed by the > > broadest selection of client modules. This specific Null instance breaks > > that principle, and that's what the patch fixes. > > > > The fact that this particular Null instance happens to implement the > > TpmMeasurementLib class is irrelevant in this regard. > > > > Please update the commit message accordingly. (There is time for a > > repost, this patch certainly qualifies for both review and merging > > during the hard feature freeze.) Again, the bug we're fixing is that > > this is a Null instance that currently does not have MODULE_TYPE=BASE. > > > > (Removing the client type restrictions from the LIBRARY_CLASS line is > > correct, of course.) > > > > > > (3) The C file needs more changes. Because we're flipping the module > > type to BASE, we should replace the EFI_STATUS type and the EFI_xxx > > return values with RETURN_STATUS and RETURN_xxx, respectively. > > > > > > (4) Consequently, for RETURN_STATUS and RETURN_xxx, we should #include > > <Base.h>, rather than <Uefi/UefiBaseType.h>. > > I've been thinking more about this. > > Assume that we replace EFI_STATUS (and the constants) with RETURN_STATUS > (and RETURN_xxx) in this Null library instance. > > Then we'll have an interesting situation where this library instance > will no longer match the lib class header -- > "MdeModulePkg/Include/Library/TpmMeasurementLib.h" will continue > declaring this function as returning EFI_STATUS. > > So what's the reason for that conflict? > > The reason is that this change actually requires us to change the lib > class header too. Consider: the whole motivation for the patch is that a > client module that is more primitive than either a PEIM or a DXE_DRIVER > wants to consume the lib instance. That requires that the lib class > header be first consumable by the client module. And for that, the lib > class header must not declare the interface with EFI_xxx in the first > place, but with RETURN_xxx. > > In turn, other implementations (instances) of the same lib class should > be updated to use RETURN_xxx. Luckily this lib class is small -- it's > just one function declaration. > > Importantly, call sites of TpmMeasureAndLogData() in PEIMs and > DXE_DRIVERs etc need not be touched, as assigning a RETURN_STATUS to an > EFI_STATUS variable (or checking with EFI_ERROR / ASSERT_EFI_ERROR) is > fine, not just technically, but conceptually too. > > Interestingly though, the BASE module in OpenBoardPkg for whose sake the > whole thing is being done, should use RETURN_STATUS only, not EFI_STATUS > -- being a BASE module, its own self should not use EFI_xxx, only > RETURN_xxx. > > > OK; I'll get off my soap box now. I don't want to blow up this patch to > modify a lib class header in MdeModulePkg during the hard feature > freeze. So just do whatever the MdeModulePkg maintainers / reviewers are > OK with, for now. > > But, for the next development cycle, I suggest that the return type and > return values of TpmMeasureAndLogData() be cleaned up (= be made > RETURN_xxx) in the lib class header, and in all of the instances. Again, > existent call sites in edk2 should need no changes. (The call site in > OpenBoardPkg like does, though.) > > > (5) Final point -- if we know that this is for OpenBoardPkg's sake, then > please don't say "some platform" in the commit message. Name > OpenBoardPkg, please. > > Thanks > Laszlo <o:p></o:p> <o:p> </o:p> </div> </body> </html> <div width="1" style="color:white;clear:both">_._,_._,_</div> <hr> Groups.io Links: You receive all messages sent to this group. <a target="_blank" href="https://edk2.groups.io/g/devel/message/64789">View/Reply Online (#64789)</a> | | <a target="_blank" href="https://groups.io/mt/76468437/1813853">Mute This Topic</a> | <a href="https://edk2.groups.io/g/devel/post">New Topic</a> <a href="https://edk2.groups.io/g/devel/editsub/1813853">Your Subscription</a> | <a href="mailto:devel+owner@edk2.groups.io">Contact Group Owner</a> | <a href="https://edk2.groups.io/g/devel/unsub">Unsubscribe</a> [edk2-devel-archive@redhat.com] <div width="1" style="color:white;clear:both">_._,_._,_</div>