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

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

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
>