Jiewen: Your understanding is correct. By design, BASE type is for all usages. Non UEFI/PI implementation can also consume them. But, Edk2 main usage is for UEFI/PI implementation. So, the developer may not be aware this type. I agree more detail rules are required to guide the developer on how to develop BASE type module.
For this patch, I think your solution is OK with more supported module types instead of change library type. Thanks Liming 发件人: bounce+27952+64790+4905953+8761...@groups.io <bounce+27952+64790+4905953+8761...@groups.io> 代表 Yao, Jiewen 发送时间: 2020年8月29日 8:47 收件人: devel@edk2.groups.io; bret.barke...@microsoft.com; Laszlo Ersek <ler...@redhat.com>; Zhang, Qi1 <qi1.zh...@intel.com> 抄送: Wang, Jian J <jian.j.w...@intel.com>; Wu, Hao A <hao.a...@intel.com> 主题: Re: [edk2-devel] [PATCH] MdeModulePkg/Library: change TpmMeasurementLibNull to BASE library. My understanding is that if it is something NOT related to EFI, then we use RETURN_XXX. The best example is the BaseLib �C StrCpyS, PciLib If we know it is related to EFI, then we use EFI_XXX. E.g. the DxeServiceLib, UefiLib However, there are some gray area. For example: -- UnitTestLib uses EFI_xxx. -- CapsuleLib uses EFI_xxx. -- CpuExceptionLibHandlerLib uses EFI_xxx. -- IpmiLib uses EFI_xxx. -- MemoryProfileLib uses EFI_xxx. -- OemHookStatusCodeLib uses EFI_xxx. -- SecurityManagementLib uses EFI_xxx. -- HashLib uses EFI_xxx -- RpmcLib uses EFI_xxx -- TcgEventLogRecordLib uses EFI_xxx. -- Tpm12CommandLib uses EFI_xxx. -- Tpm12DeviceLib uses EFI_xxx. -- Tpm2CommandLib uses EFI_xxx. -- Tpm2DeviceLib uses EFI_xxx. -- VariableKeyLib uses EFI_xxx. I am not sure if those are correct or not. I feel the reason is that the working instance should be PEI or DXE. Things are getting complicated, when we add more Dummy/Null instance. It brings confusing. Mike can clarify more on that. Bret, I think you raised a good question. Probably, we should define the rule at first. Then do the cleanup for all instances based upon the rule (not only TpmMeasurementLib) Thank you Yao Jiewen From: devel@edk2.groups.io <mailto:devel@edk2.groups.io> <devel@edk2.groups.io <mailto:devel@edk2.groups.io> > On Behalf Of Bret Barkelew via groups.io Sent: Saturday, August 29, 2020 8:25 AM To: devel@edk2.groups.io <mailto:devel@edk2.groups.io> ; Yao, Jiewen <jiewen....@intel.com <mailto:jiewen....@intel.com> >; Laszlo Ersek <ler...@redhat.com <mailto:ler...@redhat.com> >; Zhang, Qi1 <qi1.zh...@intel.com <mailto:qi1.zh...@intel.com> > Cc: Wang, Jian J <jian.j.w...@intel.com <mailto:jian.j.w...@intel.com> >; Wu, Hao A <hao.a...@intel.com <mailto:hao.a...@intel.com> > Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/Library: change TpmMeasurementLibNull to BASE library. 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. - Bret From: Yao, Jiewen via groups.io <mailto:jiewen.yao=intel....@groups.io> Sent: Friday, August 28, 2020 5:20 PM To: Laszlo Ersek <mailto:ler...@redhat.com> ; devel@edk2.groups.io <mailto:devel@edk2.groups.io> ; Zhang, Qi1 <mailto:qi1.zh...@intel.com> Cc: Wang, Jian J <mailto:jian.j.w...@intel.com> ; Wu, Hao A <mailto:hao.a...@intel.com> Subject: [EXTERNAL] Re: [edk2-devel] [PATCH] MdeModulePkg/Library: change TpmMeasurementLibNull to BASE library. 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 <ler...@redhat.com <mailto:ler...@redhat.com> > > Sent: Saturday, August 29, 2020 6:59 AM > To: devel@edk2.groups.io <mailto:devel@edk2.groups.io> ; Zhang, Qi1 <qi1.zh...@intel.com <mailto:qi1.zh...@intel.com> > > Cc: Wang, Jian J <jian.j.w...@intel.com <mailto:jian.j.w...@intel.com> >; Wu, Hao A <hao.a...@intel.com <mailto:hao.a...@intel.com> >; > Yao, Jiewen <jiewen....@intel.com <mailto:jiewen....@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: https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.t ianocore.org%2Fshow_bug.cgi%3Fid%3D2940 <https://nam06.safelinks.protection. outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D 2940&data=02%7C01%7Cbret.barkelew%40microsoft.com%7C7ae38c56bf854c2ea4c4 08d84bb147c7%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637342572075610295 &sdata=DEVDpeDBr5mTYuA0NdqgmGBUAdbQF1qDK2TuujmeSiQ%3D&reserved=0> &data=02%7C01%7Cbret.barkelew%40microsoft.com%7C7ae38c56bf854c2ea4c408d8 4bb147c7%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637342572075610295& ;sdata=DEVDpeDBr5mTYuA0NdqgmGBUAdbQF1qDK2TuujmeSiQ%3D&reserved=0 > >> > >> 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.zh...@intel.com <mailto:qi1.zh...@intel.com> > > >> Cc: Jian J Wang <jian.j.w...@intel.com <mailto:jian.j.w...@intel.com> > > >> Cc: Hao A Wu <hao.a...@intel.com <mailto:hao.a...@intel.com> > > >> Cc: Jiewen Yao <jiewen....@intel.com <mailto:jiewen....@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. <BR> > >> +Copyright (c) 2015-2020, Intel Corporation. All rights reserved. <BR> > >> 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.<BR> > >> +# Copyright (c) 2015 - 2020, Intel Corporation. All rights reserved.<BR> > >> # 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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#64801): https://edk2.groups.io/g/devel/message/64801 Mute This Topic: https://groups.io/mt/76507161/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
