From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mx.groups.io with SMTP id smtpd.web12.1098.1598655554862305769 for ; Fri, 28 Aug 2020 15:59:15 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=BzJXdF+c; spf=pass (domain: redhat.com, ip: 216.205.24.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1598655553; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=0wjmq0cJScHlWIMYrXsATK0ODY4HIpYh/Ib2gEcAeX0=; b=BzJXdF+c3SZa8jrxrFA6VJYKHM1Ks9m0cgbf5nVfDkhKMGDDsrTs5pa01V1zXHQ5m5bvo7 2mHRKjgMaK3IjK0XqJ0UTdtb4hkjUblg0u10Ua+0RUvbHlkCRXm0/TNAm7sp4sSeYeFOIE zhFNsYkVQZOQngnPaUFTAfoI1In9ojc= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-99-ucr2swRKPq-a8QS5QzCxjw-1; Fri, 28 Aug 2020 18:59:03 -0400 X-MC-Unique: ucr2swRKPq-a8QS5QzCxjw-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 92AB7807338; Fri, 28 Aug 2020 22:59:02 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-11.ams2.redhat.com [10.36.112.11]) by smtp.corp.redhat.com (Postfix) with ESMTP id 6A13710027A6; Fri, 28 Aug 2020 22:59:00 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/Library: change TpmMeasurementLibNull to BASE library. From: "Laszlo Ersek" To: devel@edk2.groups.io, qi1.zhang@intel.com Cc: Jian J Wang , Hao A Wu , Jiewen Yao References: <20200828061506.8068-1-qi1.zhang@intel.com> Message-ID: <2c97a51c-32f0-0a8b-25c3-7327d93f3891@redhat.com> Date: Sat, 29 Aug 2020 00:58:57 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0.002 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Content-Language: en-US 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 >> Cc: Jian J Wang >> Cc: Hao A Wu >> Cc: Jiewen Yao >> --- >> .../Library/TpmMeasurementLibNull/TpmMeasurementLibNull.c | 4 +++- >> .../Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf | 6 +++--- >> 2 files changed, 6 insertions(+), 4 deletions(-) >> >> diff --git a/MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.c b/MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.c >> index b9c5b68de8..ee3be62fc6 100644 >> --- a/MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.c >> +++ b/MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.c >> @@ -1,11 +1,13 @@ >> /** @file >> This library is used by other modules to measure data to TPM. >> >> -Copyright (c) 2015, Intel Corporation. All rights reserved.
>> +Copyright (c) 2015-2020, Intel Corporation. All rights reserved.
>> SPDX-License-Identifier: BSD-2-Clause-Patent >> >> **/ >> >> +#include >> + >> /** >> Tpm measure and log data, and extend the measurement result into a specific PCR. >> >> diff --git a/MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.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.
>> +# Copyright (c) 2015 - 2020, Intel Corporation. All rights reserved.
>> # 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 > , rather than . 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