[PATCH] MdeModulePkg/Library: change TpmMeasurementLibNull to BASE library.

[PATCH] MdeModulePkg/Library: change TpmMeasurementLibNull to BASE library.

[Qi Zhang]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2940

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.inf b/MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf
index 61abcfa2ec..1db2c0d6a7 100644
--- a/MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf
+++ b/MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf
@@ -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
 
 #
-- 
2.26.2.windows.1

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

[Yao, Jiewen]

Reviewed-by: Jiewen Yao <Jiewen.yao@intel.com>

> -----Original Message-----
> From: Zhang, Qi1 <qi1.zhang@intel.com>
> Sent: Friday, August 28, 2020 2:15 PM
> To: devel@edk2.groups.io
> Cc: Zhang, Qi1 <qi1.zhang@intel.com>; Wang, Jian J <jian.j.wang@intel.com>;
> Wu, Hao A <hao.a.wu@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
> Subject: [PATCH] MdeModulePkg/Library: change TpmMeasurementLibNull to
> BASE library.
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2940
> 
> 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
> 
> 
> 
>  #
> 
> --
> 2.26.2.windows.1

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

[gaoliming]

Qi:
  This is a bug fix. Do you request to catch it into this stable tag 202008?

Thanks
Liming
> -----邮件原件-----
> 发件人: bounce+27952+64729+4905953+8761045@groups.io
> <bounce+27952+64729+4905953+8761045@groups.io> 代表 Yao, Jiewen
> 发送时间: 2020年8月28日 14:17
> 收件人: Zhang, Qi1 <qi1.zhang@intel.com>; devel@edk2.groups.io
> 抄送: 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.
> 
> Reviewed-by: Jiewen Yao <Jiewen.yao@intel.com>
> 
> > -----Original Message-----
> > From: Zhang, Qi1 <qi1.zhang@intel.com>
> > Sent: Friday, August 28, 2020 2:15 PM
> > To: devel@edk2.groups.io
> > Cc: Zhang, Qi1 <qi1.zhang@intel.com>; Wang, Jian J
> <jian.j.wang@intel.com>;
> > Wu, Hao A <hao.a.wu@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
> > Subject: [PATCH] MdeModulePkg/Library: change TpmMeasurementLibNull
> to
> > BASE library.
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2940
> >
> > 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/TpmMeasurementLibNu
> ll.c
> >
> b/MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNu
> ll.c
> > index b9c5b68de8..ee3be62fc6 100644
> > ---
> >
> a/MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNu
> ll.c
> > +++
> >
> b/MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNu
> ll.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/TpmMeasurementLibNu
> ll.in
> > f
> >
> b/MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNu
> ll.in
> > f
> > index 61abcfa2ec..1db2c0d6a7 100644
> > ---
> >
> a/MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNu
> ll.in
> > f
> > +++
> >
> b/MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNu
> ll.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
> >
> >
> >
> >  #
> >
> > --
> > 2.26.2.windows.1
> 
> 
> 

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

[Qi Zhang]

Yes. This fix is for build error of intel OpenBoardPkg in  edk2-platform .

> -----Original Message-----
> From: gaoliming <gaoliming@byosoft.com.cn>
> Sent: Friday, August 28, 2020 2:31 PM
> To: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.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: 回复: [edk2-devel] [PATCH] MdeModulePkg/Library: change
> TpmMeasurementLibNull to BASE library.
> 
> Qi:
>   This is a bug fix. Do you request to catch it into this stable tag 202008?
> 
> Thanks
> Liming
> > -----邮件原件-----
> > 发件人: bounce+27952+64729+4905953+8761045@groups.io
> > <bounce+27952+64729+4905953+8761045@groups.io> 代表 Yao, Jiewen
> > 发送时间: 2020年8月28日 14:17
> > 收件人: Zhang, Qi1 <qi1.zhang@intel.com>; devel@edk2.groups.io
> > 抄送: 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.
> >
> > Reviewed-by: Jiewen Yao <Jiewen.yao@intel.com>
> >
> > > -----Original Message-----
> > > From: Zhang, Qi1 <qi1.zhang@intel.com>
> > > Sent: Friday, August 28, 2020 2:15 PM
> > > To: devel@edk2.groups.io
> > > Cc: Zhang, Qi1 <qi1.zhang@intel.com>; Wang, Jian J
> > <jian.j.wang@intel.com>;
> > > Wu, Hao A <hao.a.wu@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
> > > Subject: [PATCH] MdeModulePkg/Library: change TpmMeasurementLibNull
> > to
> > > BASE library.
> > >
> > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2940
> > >
> > > 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/TpmMeasurementLibNu
> > ll.c
> > >
> > b/MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNu
> > ll.c
> > > index b9c5b68de8..ee3be62fc6 100644
> > > ---
> > >
> > a/MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNu
> > ll.c
> > > +++
> > >
> > b/MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNu
> > ll.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/TpmMeasurementLibNu
> > ll.in
> > > f
> > >
> > b/MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNu
> > ll.in
> > > f
> > > index 61abcfa2ec..1db2c0d6a7 100644
> > > ---
> > >
> > a/MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNu
> > ll.in
> > > f
> > > +++
> > >
> > b/MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNu
> > ll.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
> > >
> > >
> > >
> > >  #
> > >
> > > --
> > > 2.26.2.windows.1
> >
> >
> > 
> 
> 

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

[Qi Zhang]

Hi, Jian & Hao

Could you please review this change as well? Thanks!

Qi Zhang

> -----Original Message-----
> From: Yao, Jiewen <jiewen.yao@intel.com>
> Sent: Friday, August 28, 2020 2:17 PM
> To: Zhang, Qi1 <qi1.zhang@intel.com>; devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>
> Subject: RE: [PATCH] MdeModulePkg/Library: change TpmMeasurementLibNull
> to BASE library.
> 
> Reviewed-by: Jiewen Yao <Jiewen.yao@intel.com>
> 
> > -----Original Message-----
> > From: Zhang, Qi1 <qi1.zhang@intel.com>
> > Sent: Friday, August 28, 2020 2:15 PM
> > To: devel@edk2.groups.io
> > Cc: Zhang, Qi1 <qi1.zhang@intel.com>; Wang, Jian J
> > <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Yao, Jiewen
> > <jiewen.yao@intel.com>
> > Subject: [PATCH] MdeModulePkg/Library: change TpmMeasurementLibNull
> to
> > BASE library.
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2940
> >
> > 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.i
> n
> > f
> >
> b/MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.i
> n
> > f
> > index 61abcfa2ec..1db2c0d6a7 100644
> > ---
> >
> a/MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.i
> n
> > f
> > +++
> >
> b/MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.i
> n
> > 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
> >
> >
> >
> >  #
> >
> > --
> > 2.26.2.windows.1

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

[Laszlo Ersek]

On 08/28/20 08:15, Qi Zhang wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2940
> 
> 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.inf b/MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf
> index 61abcfa2ec..1db2c0d6a7 100644
> --- a/MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf
> +++ b/MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf
> @@ -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>.

Thanks
Laszlo

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

[Laszlo Ersek]

On 08/28/20 19:17, Laszlo Ersek wrote:
> On 08/28/20 08:15, Qi Zhang wrote:
>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2940
>>
>> 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.inf b/MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf
>> index 61abcfa2ec..1db2c0d6a7 100644
>> --- a/MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf
>> +++ b/MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf
>> @@ -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

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

[Yao, Jiewen]

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://bugzilla.tianocore.org/show_bug.cgi?id=2940
> >>
> >> 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

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

[Bret Barkelew]

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

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>
> 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





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

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

[Yao, Jiewen]

[-- 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 --]

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

[gaoliming]

[-- 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 --]

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

[Laszlo Ersek]

On 08/30/20 03:20, gaoliming wrote:
> 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.

So I guess I wasn't much "off" then, either.

Thanks!
Laszlo

> 
>  
> 
>  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 – 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.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
> 
>  
> 
> 
> 

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

[Laszlo Ersek]

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
> 
> 
> 
> 
>  
> 

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

[Laszlo Ersek]

On 08/29/20 02:19, Yao, Jiewen wrote:
> 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.

I agree completely.

I guess we'd only really have to "demote" the TpmMeasurementLib class to
BASE / RETURN_xxx if we had another BASE library instance (for whatever
class) from which we wanted to use TpmMeasurementLib.

Thanks!
Laszlo

>> -----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://bugzilla.tianocore.org/show_bug.cgi?id=2940
>>>>
>>>> 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
> 

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

[Wang, Jian J]

Reviewed-by: Jian J Wang <jian.j.wang@intel.com>

Regards,
Jian

> -----Original Message-----
> From: Zhang, Qi1 <qi1.zhang@intel.com>
> Sent: Friday, August 28, 2020 2:15 PM
> To: devel@edk2.groups.io
> Cc: Zhang, Qi1 <qi1.zhang@intel.com>; Wang, Jian J <jian.j.wang@intel.com>;
> Wu, Hao A <hao.a.wu@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
> Subject: [PATCH] MdeModulePkg/Library: change TpmMeasurementLibNull to
> BASE library.
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2940
> 
> 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
> 
> 
> 
>  #
> 
> --
> 2.26.2.windows.1