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

["gaoliming" <gaoliming@byosoft.com.cn>]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="gb2312", Size: 13005 bytes --]

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+8761045@groups.io
<bounce+27952+64790+4905953+8761045@groups.io> ´ú±í Yao, Jiewen
·¢ËÍʱ¼ä: 2020Äê8ÔÂ29ÈÕ 8:47
ÊÕ¼þÈË: devel@edk2.groups.io; bret.barkelew@microsoft.com; Laszlo Ersek
<lersek@redhat.com>; Zhang, Qi1 <qi1.zhang@intel.com>
³­ËÍ: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@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.yao@intel.com <mailto:jiewen.yao@intel.com> >; Laszlo Ersek
<lersek@redhat.com <mailto:lersek@redhat.com> >; 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> >
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.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&amp;data=02%7C01%7Cbret.barkelew%40microsoft.com%7C7ae38c56bf854c2ea4c4
08d84bb147c7%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637342572075610295
&amp;sdata=DEVDpeDBr5mTYuA0NdqgmGBUAdbQF1qDK2TuujmeSiQ%3D&amp;reserved=0>
&amp;data=02%7C01%7Cbret.barkelew%40microsoft.com%7C7ae38c56bf854c2ea4c408d8
4bb147c7%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: 21809 bytes --]