On 08/29/20 02:19, Yao, Jiewen wrote: > 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.
I agree completely. I guess we'd only really have to "demote" the TpmMeasurementLib class to BASE / RETURN_xxx if we had another BASE library instance (for whatever class) from which we wanted to use TpmMeasurementLib. Thanks! Laszlo >> -----Original Message----- >> From: Laszlo Ersek <ler...@redhat.com> >> Sent: Saturday, August 29, 2020 6:59 AM >> To: devel@edk2.groups.io; Zhang, Qi1 <qi1.zh...@intel.com> >> Cc: Wang, Jian J <jian.j.w...@intel.com>; Wu, Hao A <hao.a...@intel.com>; >> Yao, Jiewen <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://bugzilla.tianocore.org/show_bug.cgi?id=2940 >>>> >>>> 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> >>>> Cc: Jian J Wang <jian.j.w...@intel.com> >>>> Cc: Hao A Wu <hao.a...@intel.com> >>>> Cc: Jiewen Yao <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 (#64823): https://edk2.groups.io/g/devel/message/64823 Mute This Topic: https://groups.io/mt/76468437/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
