On 08/30/20 03:20, gaoliming wrote: > 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.
So I guess I wasn't much "off" then, either. Thanks! Laszlo > > > > 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 – 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.tianocore.org%2Fshow_bug.cgi%3Fid%3D2940&data=02%7C01%7Cbret.barkelew%40microsoft.com%7C7ae38c56bf854c2ea4c408d84bb147c7%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 (#64826): https://edk2.groups.io/g/devel/message/64826 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] -=-=-=-=-=-=-=-=-=-=-=-
