回复: [edk2-devel] [PATCH] MdeModulePkg/Library: change TpmMeasurementLibNull to BASE library.
gaoliming
gaoliming at byosoft.com.cn
Sun Aug 30 01:20:21 UTC 2020
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 at groups.io
<bounce+27952+64790+4905953+8761045 at groups.io> ´ú±í Yao, Jiewen
·¢ËÍʱ¼ä: 2020Äê8ÔÂ29ÈÕ 8:47
ÊÕ¼þÈË: devel at edk2.groups.io; bret.barkelew at microsoft.com; Laszlo Ersek
<lersek at redhat.com>; Zhang, Qi1 <qi1.zhang at intel.com>
³ËÍ: Wang, Jian J <jian.j.wang at intel.com>; Wu, Hao A <hao.a.wu at 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 at edk2.groups.io <mailto:devel at edk2.groups.io>
<devel at edk2.groups.io <mailto:devel at edk2.groups.io> > On Behalf Of Bret
Barkelew via groups.io
Sent: Saturday, August 29, 2020 8:25 AM
To: devel at edk2.groups.io <mailto:devel at edk2.groups.io> ; Yao, Jiewen
<jiewen.yao at intel.com <mailto:jiewen.yao at intel.com> >; Laszlo Ersek
<lersek at redhat.com <mailto:lersek at redhat.com> >; Zhang, Qi1
<qi1.zhang at intel.com <mailto:qi1.zhang at intel.com> >
Cc: Wang, Jian J <jian.j.wang at intel.com <mailto:jian.j.wang at intel.com> >;
Wu, Hao A <hao.a.wu at intel.com <mailto:hao.a.wu at 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 at groups.io>
Sent: Friday, August 28, 2020 5:20 PM
To: Laszlo Ersek <mailto:lersek at redhat.com> ; devel at edk2.groups.io
<mailto:devel at edk2.groups.io> ; Zhang, Qi1 <mailto:qi1.zhang at intel.com>
Cc: Wang, Jian J <mailto:jian.j.wang at intel.com> ; Wu, Hao A
<mailto:hao.a.wu at 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 at redhat.com <mailto:lersek at redhat.com> >
> Sent: Saturday, August 29, 2020 6:59 AM
> To: devel at edk2.groups.io <mailto:devel at edk2.groups.io> ; Zhang, Qi1
<qi1.zhang at intel.com <mailto:qi1.zhang at intel.com> >
> Cc: Wang, Jian J <jian.j.wang at intel.com <mailto:jian.j.wang at intel.com> >;
Wu, Hao A <hao.a.wu at intel.com <mailto:hao.a.wu at intel.com> >;
> Yao, Jiewen <jiewen.yao at intel.com <mailto:jiewen.yao at 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&data=02%7C01%7Cbret.barkelew%40microsoft.com%7C7ae38c56bf854c2ea4c4
08d84bb147c7%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637342572075610295
&sdata=DEVDpeDBr5mTYuA0NdqgmGBUAdbQF1qDK2TuujmeSiQ%3D&reserved=0>
&data=02%7C01%7Cbret.barkelew%40microsoft.com%7C7ae38c56bf854c2ea4c408d8
4bb147c7%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.zhang at intel.com
<mailto:qi1.zhang at intel.com> >
> >> Cc: Jian J Wang <jian.j.wang at intel.com <mailto:jian.j.wang at intel.com> >
> >> Cc: Hao A Wu <hao.a.wu at intel.com <mailto:hao.a.wu at intel.com> >
> >> Cc: Jiewen Yao <jiewen.yao at intel.com <mailto:jiewen.yao at 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 (#64801): https://edk2.groups.io/g/devel/message/64801
Mute This Topic: https://groups.io/mt/76507161/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/edk2-devel-archive/attachments/20200830/ecb88bc9/attachment.htm>
More information about the edk2-devel-archive
mailing list