From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail.byosoft.com.cn (mail.byosoft.com.cn [58.240.74.243]) by mx.groups.io with SMTP id smtpd.web11.23978.1598750424262960873 for ; Sat, 29 Aug 2020 18:20:25 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=none, err=permanent DNS error (domain: byosoft.com.cn, ip: 58.240.74.243, mailfrom: gaoliming@byosoft.com.cn) Received: from DESKTOPS6D0PVI ([116.233.155.155]) (envelope-sender ) by 192.168.6.13 with ESMTP for ; Sun, 30 Aug 2020 09:20:17 +0800 X-WM-Sender: gaoliming@byosoft.com.cn X-WM-AuthFlag: YES X-WM-AuthUser: gaoliming@byosoft.com.cn From: "gaoliming" To: , , , "'Laszlo Ersek'" , "'Zhang, Qi1'" Cc: "'Wang, Jian J'" , "'Wu, Hao A'" References: <20200828061506.8068-1-qi1.zhang@intel.com> <2c97a51c-32f0-0a8b-25c3-7327d93f3891@redhat.com>, In-Reply-To: Subject: =?UTF-8?B?5Zue5aSNOiBbZWRrMi1kZXZlbF0gW1BBVENIXSBNZGVNb2R1bGVQa2cvTGlicmFyeTogY2hhbmdlIFRwbU1lYXN1cmVtZW50TGliTnVsbCB0byBCQVNFIGxpYnJhcnku?= Date: Sun, 30 Aug 2020 09:20:21 +0800 Message-ID: <001101d67e6b$babae7e0$3030b7a0$@byosoft.com.cn> MIME-Version: 1.0 X-Mailer: Microsoft Outlook 16.0 Thread-Index: AQLpLW0w849lJkYbtGP1qW9t+hfL0wI0Ft8mAlZAtzoBlw3tLwKotbe2AiiV9gum0uP5oA== Content-Type: multipart/alternative; boundary="----=_NextPart_000_0012_01D67EAE.C8DFFCA0" Content-Language: zh-cn ------=_NextPart_000_0012_01D67EAE.C8DFFCA0 Content-Type: text/plain; charset="gb2312" Content-Transfer-Encoding: quoted-printable Jiewen: Your understanding is correct. By design, BASE type is for all usages. No= n 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.=20 =20 For this patch, I think your solution is OK with more supported module types instead of change library type. =20 Thanks Liming =B7=A2=BC=FE=C8=CB: bounce+27952+64790+4905953+8761045@groups.io =B4=FA=B1=ED Yao, Jiewen =B7=A2=CB=CD=CA=B1=BC=E4: 2020=C4=EA8=D4=C229=C8=D5 8:47 =CA=D5=BC=FE=C8=CB: devel@edk2.groups.io; bret.barkelew@microsoft.com; Las= zlo Ersek ; Zhang, Qi1 =B3=AD=CB=CD: Wang, Jian J ; Wu, Hao A =D6=F7=CC=E2: Re: [edk2-devel] [PATCH] MdeModulePkg/Library: change TpmMeasurementLibNull to BASE library. =20 My understanding is that if it is something NOT related to EFI, then we us= e RETURN_XXX. The best example is the BaseLib =A8C StrCpyS, PciLib If we know it is related to EFI, then we use EFI_XXX. E.g. the DxeServiceLib, UefiLib =20 However, there are some gray area. For example: -- UnitTestLib uses EFI_xxx. -- CapsuleLib uses EFI_xxx.=20 -- CpuExceptionLibHandlerLib uses EFI_xxx. -- IpmiLib uses EFI_xxx. -- MemoryProfileLib uses EFI_xxx. -- OemHookStatusCodeLib uses EFI_xxx.=20 -- SecurityManagementLib uses EFI_xxx. -- HashLib uses EFI_xxx -- RpmcLib uses EFI_xxx -- TcgEventLogRecordLib uses EFI_xxx. -- Tpm12CommandLib uses EFI_xxx.=20 -- 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. =20 Mike can clarify more on that. =20 Bret, I think you raised a good question. Probably, we should define the rule at first.=20 Then do the cleanup for all instances based upon the rule (not only TpmMeasurementLib) =20 Thank you Yao Jiewen =20 From: 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 >; Laszlo Ersek >; Zhang, Qi1 > Cc: Wang, Jian J >; Wu, Hao A > Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/Library: change TpmMeasurementLibNull to BASE library. =20 Question (since it=A1=AFs been brought up): when *wouldn=A1=AFt* you use E= FI_*? They=A1=AFre clearly superior in every way. I mean, they=A1=AFve got EFI r= ight in the name. =20 - Bret=20 =20 From: Yao, Jiewen via groups.io = =20 Sent: Friday, August 28, 2020 5:20 PM To: Laszlo Ersek ; devel@edk2.groups.io ; Zhang, Qi1 =20 Cc: Wang, Jian J ; Wu, Hao A =20 Subject: [EXTERNAL] Re: [edk2-devel] [PATCH] MdeModulePkg/Library: change TpmMeasurementLibNull to BASE library. =20 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=A1=AFt think it is absolutely necessary to change EFI_x= xx 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 > > Sent: Saturday, August 29, 2020 6:59 AM > To: devel@edk2.groups.io ; Zhang, Qi1 > > Cc: Wang, Jian J >= ; Wu, Hao A >; > Yao, Jiewen > > Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/Library: change > TpmMeasurementLibNull to BASE library. >=20 > 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=3Dhttps%3A%2F%2Fbugzil= la.t ianocore.org%2Fshow_bug.cgi%3Fid%3D2940 &data=3D02%7C01%7Cbret.barkelew%40microsoft.com%7C7ae38c56bf854c2ea4c4= 08d8 4bb147c7%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637342572075610295&a= mp ;sdata=3DDEVDpeDBr5mTYuA0NdqgmGBUAdbQF1qDK2TuujmeSiQ%3D&reserved=3D0 > >> > >> TpmMeasurementLib includes DxeTpmMeasurementLib and > PeiTpmMeasurementLib. > >> So need to change TpmMeasurementLibNull to BASE library to avoid buil= d > >> error in some platform. > >> > >> Signed-off-by: Qi Zhang > > >> Cc: Jian J Wang = > > >> Cc: Hao A Wu > > >> Cc: Jiewen Yao > > >> --- > >> .../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.
> >> +Copyright (c) 2015-2020, Intel Corporation. All rights reserved. > >> SPDX-License-Identifier: BSD-2-Clause-Patent > >> > >> **/ > >> > >> +#include > >> + > >> /** > >> 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.
> >> +# Copyright (c) 2015 - 2020, Intel Corporation. All rights reserved.
> >> # SPDX-License-Identifier: BSD-2-Clause-Patent > >> # > >> ## > >> @@ -10,9 +10,9 @@ > >> INF_VERSION =3D 0x00010005 > >> BASE_NAME =3D TpmMeasurementLibNull > >> FILE_GUID =3D 6DFD6E9F-9278-48D8-8F45-B6CFF2C2B69C > >> - MODULE_TYPE =3D UEFI_DRIVER > >> + MODULE_TYPE =3D BASE > >> VERSION_STRING =3D 1.0 > >> - LIBRARY_CLASS =3D TpmMeasurementLib|DXE_DRIVER > DXE_RUNTIME_DRIVER DXE_SMM_DRIVER UEFI_APPLICATION UEFI_DRIVER > >> + LIBRARY_CLASS =3D TpmMeasurementLib > >> MODULE_UNI_FILE =3D 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 TpmMeasurementLi= b > > 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=3DBASE, so that they can be consumed by= the > > broadest selection of client modules. This specific Null instance brea= ks > > 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=3DBAS= E. > > > > (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 > > , rather than . >=20 > I've been thinking more about this. >=20 > Assume that we replace EFI_STATUS (and the constants) with RETURN_STATUS > (and RETURN_xxx) in this Null library instance. >=20 > 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. >=20 > So what's the reason for that conflict? >=20 > 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. >=20 > 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. >=20 > 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. >=20 > 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. >=20 >=20 > 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. >=20 > But, for the next development cycle, I suggest that the return type and > return values of TpmMeasureAndLogData() be cleaned up (=3D 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.) >=20 >=20 > (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. >=20 > Thanks > Laszlo =20 ------=_NextPart_000_0012_01D67EAE.C8DFFCA0 Content-Type: text/html; charset="gb2312" Content-Transfer-Encoding: quoted-printable

Jiewen:<= /o:p>

 Your understanding is correct. By d= esign, BASE type is for all usages. Non UEFI/PI implementation can also con= sume them. But, Edk2 main usage is for UEFI/PI implementation. So, the deve= loper may not be aware this type. I agree more detail rules are required to= guide the developer on how to develop BASE type module. =

 

 Fo= r this patch, I think your solution is OK with more supported module types = instead of change library type.

<= span lang=3DEN-US style=3D'font-size:10.5pt;font-family:=B5=C8=CF=DF'>=  

Thanks

Liming

= =B7=A2=BC=FE=C8=CB: bounce+27952+64790+4905953+8761045@groups.io <bounce+27952+64790+= 4905953+8761045@groups.io> =B4=FA=B1=ED Yao, Jiewen
=B7=A2=CB=CD=CA=B1=BC=E4: 2020=C4=EA8=D4=C229=C8=D5 8:47
=CA=D5= =BC=FE=C8=CB: devel@edk2.= groups.io; bret.barkelew@microsoft.com; Laszlo Ersek <lersek@redhat.com&= gt;; Zhang, Qi1 <qi1.zhang@intel.com>
=B3=AD=CB=CD: Wang, Jian J <jian.j.wang@i= ntel.com>; Wu, Hao A <hao.a.wu@intel.com>
=D6=F7=CC= =E2: Re: [edk2-devel] [PA= TCH] MdeModulePkg/Library: change TpmMeasurementLibNull to BASE library.

 

My u= nderstanding is that if it is something NOT related to EFI, then we use RET= URN_XXX. The best example is the BaseLib =A8C 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. Fo= r example:

-- = UnitTestLib uses EFI_xxx.

-- CapsuleLib uses EFI_xxx.

-- CpuExceptionLibHandlerLib uses EFI_xxx.<= /o:p>

-- IpmiLib uses EFI= _xxx.

-- Memor= yProfileLib uses EFI_xxx.

-- OemHookStatusCodeLib uses EFI_xxx.

-- SecurityManagementLib uses EFI_xxx.=

-- HashLib us= es EFI_xxx

-- = RpmcLib uses EFI_xxx

-- TcgEventLogRecordLib uses EFI_xxx.

-- Tpm12CommandLib uses EFI_xxx.

-- Tpm12DeviceLib use= s EFI_xxx.

-- = Tpm2CommandLib uses EFI_xxx.

-- Tpm2DeviceLib uses EFI_xxx.

-- VariableKeyLib uses EFI_xxx.=

I am not sure if those a= re correct or not. I feel the reason is that the working instance should be= PEI or DXE.

T= hings are getting complicated, when we add more Dummy/Null instance. It bri= ngs confusing.

 

Mike c= an clarify more on that.

 

Bret, I think you raised a good question.

Probably, we should define the rule at fir= st.

Then do t= he cleanup for all instances based upon the rule (not only TpmMeasurementLi= b)

 =

Thank you

Yao Jiewen<= /span>

 <= /p>

From: devel@edk2.gro= ups.io <devel@edk2.groups.io= > On Behalf Of Bret Barkelew via groups.io
Sent: Sa= turday, 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=A1=AFs bee= n brought up): when *wouldn=A1=AFt* you use EFI_*? They=A1=AFre clea= rly superior in every way. I mean, they=A1=AFve got EFI right in the name.<= o:p>

 

- Bret

 

From: Yao, Jiew= en via groups.io
Sent: Friday, August 28, 2020 5:20 PM
= To: Laszlo Ersek; devel@edk2.groups.io; Zhang, Qi1
Cc: Wang, Jian J; Wu, Ha= o A
Subject: [EXTERNAL] Re: [edk2-devel] [PATCH] MdeModulePkg= /Library: change TpmMeasurementLibNull to BASE library.

 

Las= zlo
Good feedback.

> The reason is that this change actually r= equires us to change the lib
> class header too. Consider: the whole = motivation for the patch is that a
> client module that is more primi= tive than either a PEIM or a DXE_DRIVER
> wants to consume the lib in= stance. 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 RET= URN_xxx.

[Jiewen] But I don=A1=AFt think it is absolutely necessary = to change EFI_xxx to RETURN_xxx in library class, just because a library in= stance could be PEI and DXE.

EFI_xxx is legal for both PEI and DXE.<= br>
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 Jiewe= n


> -----Original Message-----
> From: Laszlo Ersek <= ;lersek@redhat.com>
> Sen= t: 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.y= ao@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=3Dhttps%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2940= &amp;data=3D02%7C01%7Cbret.barkelew%40microsoft.com%7C7ae38c56bf854c2ea= 4c408d84bb147c7%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C63734257207561= 0295&amp;sdata=3DDEVDpeDBr5mTYuA0NdqgmGBUAdbQF1qDK2TuujmeSiQ%3D&amp= ;reserved=3D0
> >>
> >> TpmMeasurementLib inclu= des DxeTpmMeasurementLib and
> PeiTpmMeasurementLib.
> >>= So need to change TpmMeasurementLibNull to BASE library to avoid build
= > >>  error in some platform.
> >>
> >&g= t; Signed-off-by: Qi Zhang <qi1.z= hang@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>
> >> ---
> >>&nb= sp; .../Library/TpmMeasurementLibNull/TpmMeasurementLibNull.c   |= 4 +++-
> >>  .../Library/TpmMeasurementLibNull/TpmMeasure= mentLibNull.inf | 6 +++---
> >>  2 files changed, 6 insert= ions(+), 4 deletions(-)
> >>
> >> diff --git
>= ; a/MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.c
&= gt; b/MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.c> >> index b9c5b68de8..ee3be62fc6 100644
> >> ---
= > a/MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.c> >> +++
> b/MdeModulePkg/Library/TpmMeasurementLibNull/Tp= mMeasurementLibNull.c
> >> @@ -1,11 +1,13 @@
> >>&n= bsp; /** @file
> >>    This library is used by o= ther modules to measure data to TPM.
> >>
> >> -Cop= yright (c) 2015, Intel Corporation. All rights reserved. <BR>
>= >> +Copyright (c) 2015-2020, Intel Corporation. All rights reserved.= <BR>
> >>  SPDX-License-Identifier: BSD-2-Clause-Pa= tent
> >>
> >>  **/
> >>
> &= gt;> +#include <Uefi/UefiBaseType.h>
> >> +
> &g= t;>  /**
> >>    Tpm measure and log dat= a, and extend the measurement result into a
> specific PCR.
> &= gt;>
> >> diff --git
> a/MdeModulePkg/Library/TpmMeasu= rementLibNull/TpmMeasurementLibNull.in
> f
> b/MdeModulePkg/Lib= rary/TpmMeasurementLibNull/TpmMeasurementLibNull.in
> f
> >&= gt; index 61abcfa2ec..1db2c0d6a7 100644
> >> ---
> a/MdeM= odulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.in
> f> >> +++
> b/MdeModulePkg/Library/TpmMeasurementLibNull/Tp= mMeasurementLibNull.in
> f
> >> @@ -1,7 +1,7 @@
> &= gt;>  ## @file
> >>  #  Provides NULL TPM mea= surement function.
> >>  #
> >> -# Copyright (= c) 2015 - 2018, Intel Corporation. All rights reserved.<BR>
> &= gt;> +# Copyright (c) 2015 - 2020, Intel Corporation. All rights reserve= d.<BR>
> >>  # SPDX-License-Identifier: BSD-2-Clause= -Patent
> >>  #
> >>  ##
> >>= @@ -10,9 +10,9 @@
> >>    INF_VERSION &nbs= p;            &= nbsp;     =3D 0x00010005
> >>  &n= bsp; BASE_NAME          &= nbsp;           =3D TpmMe= asurementLibNull
> >>    FILE_GUID  &n= bsp;            = ;       =3D 6DFD6E9F-9278-48D8-8F45-B6CFF2C2B= 69C
> >> -  MODULE_TYPE      = ;            &n= bsp; =3D UEFI_DRIVER
> >> +  MODULE_TYPE   =             &nb= sp;    =3D BASE
> >>    VERSION_S= TRING           &nbs= p;     =3D 1.0
> >> -  LIBRARY_CLASS&n= bsp;            = ;     =3D TpmMeasurementLib|DXE_DRIVER
> DXE_RUNT= IME_DRIVER DXE_SMM_DRIVER UEFI_APPLICATION UEFI_DRIVER
> >> +&n= bsp; LIBRARY_CLASS         &nb= sp;        =3D TpmMeasurementLib
>= >>    MODULE_UNI_FILE     &n= bsp;          =3D TpmMeasureme= ntLibNull.uni
> >>
> >>  #
> >>> >
> > (1) I agree this is a bugfix, and should be include= d in the stable tag.
> >
> >
> > (2) The commit = message makes zero sense to me, on the other hand. I
> > don't und= erstand how DxeTpmMeasurementLib and PeiTpmMeasurementLib
> are
&g= t; > relevant at all. I also don't understand how TpmMeasurementLib
&= gt; > "includes" DxeTpmMeasurementLib and PeiTpmMeasurementLib= .
> >
> > I guess the intent is to say that *some* of the= known TpmMeasurementLib
> > instances are PeiTpmMeasurementLib an= d DxeTpmMeasurementLib. I guess
> > that would be a valid statemen= t, but it's still irrelevant here.
> >
> > The issue here= is that *all* Null instances (regardless of library
> > class) sh= ould have MODULE_TYPE=3DBASE, so that they can be consumed by the
> &= gt; broadest selection of client modules. This specific Null instance break= s
> > that principle, and that's what the patch fixes.
> >= ;
> > The fact that this particular Null instance happens to imple= ment the
> > TpmMeasurementLib class is irrelevant in this regard.=
> >
> > Please update the commit message accordingly. (T= here 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 cu= rrently does not have MODULE_TYPE=3DBASE.
> >
> > (Removi= ng 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
> > r= eturn values with RETURN_STATUS and RETURN_xxx, respectively.
> ><= br>> >
> > (4) Consequently, for RETURN_STATUS and RETURN_xx= x, we should #include
> > <Base.h>, rather than <Uefi/Uef= iBaseType.h>.
>
> I've been thinking more about this.
&g= t;
> Assume that we replace EFI_STATUS (and the constants) with RETU= RN_STATUS
> (and RETURN_xxx) in this Null library instance.
> <= br>> Then we'll have an interesting situation where this library instanc= e
> will no longer match the lib class header --
> "MdeMod= ulePkg/Include/Library/TpmMeasurementLib.h" will continue
> decl= aring 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, bu= t with RETURN_xxx.
>
> In turn, other implementations (instanc= es) of the same lib class should
> be updated to use RETURN_xxx. Luck= ily this lib class is small -- it's
> just one function declaration.<= br>>
> Importantly, call sites of TpmMeasureAndLogData() in PEIMs= and
> DXE_DRIVERs etc need not be touched, as assigning a RETURN_STA= TUS 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 who= se sake the
> whole thing is being done, should use RETURN_STATUS onl= y, not EFI_STATUS
> -- being a BASE module, its own self should not u= se 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 sug= gest that the return type and
> return values of TpmMeasureAndLogData= () be cleaned up (=3D be made
> RETURN_xxx) in the lib class header, = and in all of the instances. Again,
> existent call sites in edk2 sho= uld need no changes. (The call site in
> OpenBoardPkg like does, thou= gh.)
>
>
> (5) Final point -- if we know that this is f= or OpenBoardPkg's sake, then
> please don't say "some platform&q= uot; in the commit message. Name
> OpenBoardPkg, please.
>
= > Thanks
> Laszlo

 

------=_NextPart_000_0012_01D67EAE.C8DFFCA0--