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

["Laszlo Ersek" <lersek@redhat.com>]

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