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

["Yao, Jiewen" <jiewen.yao@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 11290 bytes --]

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 <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; Yao, Jiewen <jiewen.yao@intel.com>; Laszlo Ersek <lersek@redhat.com>; Zhang, Qi1 <qi1.zhang@intel.com>
Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@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.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<mailto:lersek@redhat.com>>
> Sent: Saturday, August 29, 2020 6:59 AM
> To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Zhang, Qi1 <qi1.zhang@intel.com<mailto:qi1.zhang@intel.com>>
> Cc: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>;
> Yao, Jiewen <jiewen.yao@intel.com<mailto: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<mailto:qi1.zhang@intel.com>>
> >> Cc: Jian J Wang <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>
> >> Cc: Hao A Wu <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>
> >> Cc: Jiewen Yao <jiewen.yao@intel.com<mailto: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




[-- Attachment #2: Type: text/html, Size: 18707 bytes --]