Re: [edk2-devel] [PATCH] MdeModulePkg/Library: change TpmMeasurementLibNull to BASE library.

["Laszlo Ersek" <lersek@redhat.com>]

On 08/29/20 02:25, Bret Barkelew wrote:
> 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.

Minimalism.

EFI_* is appropriate (or even required) when the library deals with
concepts from the PI and/or the UEFI spec. Otherwise, EFI_* brings in
too much baggage (conceptual baggage at the least).

RETURN_* is good for stuff that's per se independent of PI/UEFI, and it
still can be mapped easily to EFI_*.

This is just my understanding, I admit it's not crystal clear.

Thanks
Laszlo


> 
>  
> 
> - Bret
> 
>  
> 
> *From: *Yao, Jiewen via groups.io <mailto:jiewen.yao=intel.com@groups.io>
> *Sent: *Friday, August 28, 2020 5:20 PM
> *To: *Laszlo Ersek <mailto:lersek@redhat.com>; devel@edk2.groups.io
> <mailto:devel@edk2.groups.io>; Zhang, Qi1 <mailto:qi1.zhang@intel.com>
> *Cc: *Wang, Jian J <mailto:jian.j.wang@intel.com>; Wu, Hao A
> <mailto:hao.a.wu@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 <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:
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2940&amp;data=02%7C01%7Cbret.barkelew%40microsoft.com%7C7ae38c56bf854c2ea4c408d84bb147c7%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637342572075610295&amp;sdata=DEVDpeDBr5mTYuA0NdqgmGBUAdbQF1qDK2TuujmeSiQ%3D&amp;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.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. <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
> 
> 
> 
> 
>  
>