From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from NAM10-MW2-obe.outbound.protection.outlook.com (NAM10-MW2-obe.outbound.protection.outlook.com [40.107.94.133]) by mx.groups.io with SMTP id smtpd.web10.2606.1598660719999052785 for ; Fri, 28 Aug 2020 17:25:20 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@microsoft.com header.s=selector2 header.b=g7WmXcqZ; spf=pass (domain: microsoft.com, ip: 40.107.94.133, mailfrom: bret.barkelew@microsoft.com) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=Se0OSzwICEFP2zhaNBr3iDGgClSlzxYMlCV8FeyxqdZ1glLRmzpxioIqPIYc1NRywOeiYbH3FF4F5jlBLeGLZfOYJeEEyf07Nbs/DWlo/VLihSO8MGZvKbQd0FezbEAmuTqKVBCakl4d07gol7QC6tAWwi+xHk4LejRLoirSHywtzYqWGFau9Isq9zJopg35JI5vBVQYY2k8jW1CfHmJiXY9mTnfRQqWUlzsx4vmK+PdQ2IaaapLDNnWeHD2JP676kOkst7Y5/e4+zQqrDpCU5a5NTvXr36Hi7/z2u8Q3f5/uuqG/xXD01oAU6Lfl4ArdznUnyTsF1+yZWdK5OB2Iw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=c2nE78LKtiX4TECft/ehTtx/R1BH5FnR6fCV1RgauUc=; b=CCq7+NfzDV0yrOMIl1X/TdHj3TQY0f3X9gcBohpGcBQrcvocxVGGhLc8b39z/gGfVW9RYPQXBRatFU8ewh2/FA0lwB07R2IocqeFBC3f9YWRZlWpcPsGNQjpZbPmvidvYTgPtiGfU/dBbA/OEoO9C6cHeGBumRDgMy2n/2UWTjX5jhenM/S3eZVvQha4AmDsy5duzixohGUIonGHzld18x6iVNbqD7K/RwmKkpRyRp2YNhjVo2ZyLf/ae74aNGE2nOM2AFUUcrigsUMv8dVQtfmh2ANSkoLxFUZ8zs6IdFVlcAfEoH2PZAiJd7/dPuqKCh/Bv4T9FPRfMoJSwrDj3A== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=microsoft.com; dmarc=pass action=none header.from=microsoft.com; dkim=pass header.d=microsoft.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=selector2; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=c2nE78LKtiX4TECft/ehTtx/R1BH5FnR6fCV1RgauUc=; b=g7WmXcqZ3yZs8ZCiLSuY10M/06pGT9MjQl41ABSsif3XuCegAq7BU0ley+bjeDSrXNBj/8Sr3jDOXZwXicNd/vN4DQQ3GctyCYL42bQIe/fC7Z+TOJBTCaPm8jhlqPRS1nMi7mQZA/mLHRjmcrxzbpgRYdTavAPK9+xqQK4fCqc= Received: from CY4PR21MB0743.namprd21.prod.outlook.com (2603:10b6:903:b2::9) by CY4PR2101MB0802.namprd21.prod.outlook.com (2603:10b6:910:8f::18) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3348.4; Sat, 29 Aug 2020 00:25:18 +0000 Received: from CY4PR21MB0743.namprd21.prod.outlook.com ([fe80::2ca0:7d3e:e918:c47a]) by CY4PR21MB0743.namprd21.prod.outlook.com ([fe80::2ca0:7d3e:e918:c47a%12]) with mapi id 15.20.3326.016; Sat, 29 Aug 2020 00:25:18 +0000 From: "Bret Barkelew" To: "devel@edk2.groups.io" , "Yao, Jiewen" , Laszlo Ersek , "Zhang, Qi1" CC: "Wang, Jian J" , "Wu, Hao A" Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/Library: change TpmMeasurementLibNull to BASE library. Thread-Topic: [edk2-devel] [PATCH] MdeModulePkg/Library: change TpmMeasurementLibNull to BASE library. Thread-Index: AQHWfZomUVolN8ZMfEi4qepmjoj8ralOOleJ Date: Sat, 29 Aug 2020 00:25:17 +0000 Message-ID: References: <20200828061506.8068-1-qi1.zhang@intel.com> <2c97a51c-32f0-0a8b-25c3-7327d93f3891@redhat.com>, In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: msip_labels: MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_Enabled=True;MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_SiteId=72f988bf-86f1-41af-91ab-2d7cd011db47;MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_SetDate=2020-08-29T00:24:35.1145357Z;MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_ContentBits=0;MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_Method=Privileged authentication-results: edk2.groups.io; dkim=none (message not signed) header.d=none;edk2.groups.io; dmarc=none action=none header.from=microsoft.com; x-originating-ip: [174.21.132.206] x-ms-publictraffictype: Email x-ms-office365-filtering-ht: Tenant x-ms-office365-filtering-correlation-id: 5c4222b7-0bdc-45e8-34c3-08d84bb201f1 x-ms-traffictypediagnostic: CY4PR2101MB0802: x-ld-processed: 72f988bf-86f1-41af-91ab-2d7cd011db47,ExtAddr x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:10000; x-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: DMJO9M4jmvrbdBNj7wR9/ebCBcFG8Y96KPt9614O7WNZxIoebDcbkpirngyCcQ1EN2kK+6Pb0/A01PKw59HCVuKGSOQjnXM59y3w/lap8PVNzej6qislwsAoDR6+I5mZV0Di0V2gYUDBoXVYMtNXkoNVUk5LVahYHE2TfKZLQXsXYJSeJxsKPLQ+hNWnoi/LltAHpPaLYq6+lmPED8fByrE1RMStCaQOLYkZ3O3tuxF1Kw//Mut4TO7kqZF6bK+Z8+1PjoHSZ9ygj5H/xP+n3GjH27OdBkTkrMwgWLzn1kw/lF1RbE3w+720twW52hDf0JBxspGoinMPVS73+hfZ0GNxK00j/1ied11WixeOsAN6uWbxmih25VSwjkwXd8bEuNMfL8HXjWKc1N9mW+3RzzWktHuY6HlqiN6FN08QbGoNzVR+/Cg2N9IWWS5mO3EL4Zt3uc9d2pd3sZ8zN8WTag== x-forefront-antispam-report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:CY4PR21MB0743.namprd21.prod.outlook.com;PTR:;CAT:NONE;SFS:(4636009)(136003)(366004)(346002)(376002)(39860400002)(396003)(33656002)(83380400001)(52536014)(86362001)(2906002)(4326008)(8990500004)(5660300002)(71200400001)(966005)(478600001)(53546011)(6506007)(82960400001)(54906003)(186003)(26005)(9686003)(7696005)(10290500003)(8676002)(55016002)(82950400001)(66556008)(64756008)(66476007)(66446008)(166002)(76116006)(110136005)(66946007)(316002)(8936002)(21314003);DIR:OUT;SFP:1102; x-ms-exchange-antispam-messagedata: P8FyVEsLNPu90tUu6D3v2R29ZN5UdKgR4oStBk0bu3/LhgVeQGqMrl4RbHLUDsRhZsqpK0IU+IUEw+dgD1JVSlqNlfabGXb1+Wr0Jvk8fxp/ILIGsxNcoWlAAU4MhincvMjq2jBD6AtDABS5SQLEhCQAPsH0RnxpDPbGQD7qHywbZtOB1e/c1N+1Bp8bKvRFj07xCOANS9YBfwUTxLrI9pI+1/KZAkE/Y0i3Loy0EcFmtHRFHfPU8nKS1l4kPj5x/DbCLFIBqWT/zc4v5bGXHiRzsDlB8Ob4F+c9HmXbxzRl8fDtCOa+vM8aFNbInN7zTFcekFIBthoufbNLgCdnQlSo5fdaREAbRxroX5BJYLyqKosniI32MjcQRNjUcVVfTkzeMSdvp34emWPvcHZl1/PaV2oE2IKpznRaXZXAfKzyPh6YjNejelqahaEnt5b4AKTNJbbBd8eEdIHWjzu5NTzL/q0Is7qCWWF3OCAzohh33zPNsNjfNiXo0JJ0UkZ/fxqkTtqqKPRR70VdNWnCDcXq6CqDMxJdHcqIsit5C69DLkdn7V6crh6/cefOaTAHpzWHnTpbyanv1tvc7/R0covPYFVKOT6bG9khWgB1E/tjVI5WtoCNB2s4TreTgycfrFN11mK0dzQgw+yUk+My2Q== x-ms-exchange-transport-forked: True MIME-Version: 1.0 X-OriginatorOrg: microsoft.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: CY4PR21MB0743.namprd21.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: 5c4222b7-0bdc-45e8-34c3-08d84bb201f1 X-MS-Exchange-CrossTenant-originalarrivaltime: 29 Aug 2020 00:25:18.0812 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 72f988bf-86f1-41af-91ab-2d7cd011db47 X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: SyL4widEkyIt50rE8AdOATdlPbEUUKBnkSxgP0t5FTU53a4N4xHFKpe91GGb1x7HgxjHQSJvsro3AM69uNEUXg== X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY4PR2101MB0802 Content-Language: en-US Content-Type: multipart/alternative; boundary="_000_CY4PR21MB0743C103E8F4CB7B2D551A6BEF530CY4PR21MB0743namp_" --_000_CY4PR21MB0743C103E8F4CB7B2D551A6BEF530CY4PR21MB0743namp_ Content-Type: text/plain; charset="Windows-1252" Content-Transfer-Encoding: quoted-printable Question (since it=92s been brought up): when *wouldn=92t* you use EFI_*? T= hey=92re clearly superior in every way. I mean, they=92ve got EFI right in = the name. - Bret From: Yao, Jiewen via groups.io Sent: Friday, August 28, 2020 5:20 PM To: Laszlo Ersek; devel@edk2.groups.io; Zhang, Qi1 Cc: Wang, Jian J; Wu, Hao A 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=92t think it is absolutely necessary to change EFI_xxx = to RETURN_xxx in library class, just because a library instance could be PE= I 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 L= IBRARY_CLASS, instead of *remove* them. Thank you Yao Jiewen > -----Original Message----- > From: Laszlo Ersek > Sent: Saturday, August 29, 2020 6:59 AM > To: devel@edk2.groups.io; Zhang, Qi1 > Cc: Wang, Jian J ; Wu, Hao A = ; > Yao, Jiewen > Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/Library: change > TpmMeasurementLibNull to BASE library. > > On 08/28/20 19:17, Laszlo Ersek wrote: > > On 08/28/20 08:15, Qi Zhang wrote: > >> REF: https://nam06.safelinks.protection.outlook.com/?url=3Dhttps%3A%2= F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2940&data=3D02%7C01%7C= bret.barkelew%40microsoft.com%7C7ae38c56bf854c2ea4c408d84bb147c7%7C72f988bf= 86f141af91ab2d7cd011db47%7C1%7C0%7C637342572075610295&sdata=3DDEVDpeDBr= 5mTYuA0NdqgmGBUAdbQF1qDK2TuujmeSiQ%3D&reserved=3D0 > >> > >> TpmMeasurementLib includes DxeTpmMeasurementLib and > PeiTpmMeasurementLib. > >> So need to change TpmMeasurementLibNull to BASE library to avoid buil= d > >> error in some platform. > >> > >> Signed-off-by: Qi Zhang > >> Cc: Jian J Wang > >> Cc: Hao A Wu > >> Cc: Jiewen Yao > >> --- > >> .../Library/TpmMeasurementLibNull/TpmMeasurementLibNull.c | 4 +++- > >> .../Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf | 6 +++-= -- > >> 2 files changed, 6 insertions(+), 4 deletions(-) > >> > >> diff --git > a/MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.c > b/MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.c > >> index b9c5b68de8..ee3be62fc6 100644 > >> --- > a/MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.c > >> +++ > b/MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.c > >> @@ -1,11 +1,13 @@ > >> /** @file > >> This library is used by other modules to measure data to TPM. > >> > >> -Copyright (c) 2015, Intel Corporation. All rights reserved.
> >> +Copyright (c) 2015-2020, Intel Corporation. All rights reserved. > >> SPDX-License-Identifier: BSD-2-Clause-Patent > >> > >> **/ > >> > >> +#include > >> + > >> /** > >> Tpm measure and log data, and extend the measurement result into a > specific PCR. > >> > >> diff --git > a/MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.in > f > b/MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.in > f > >> index 61abcfa2ec..1db2c0d6a7 100644 > >> --- > a/MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.in > f > >> +++ > b/MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.in > f > >> @@ -1,7 +1,7 @@ > >> ## @file > >> # Provides NULL TPM measurement function. > >> # > >> -# Copyright (c) 2015 - 2018, Intel Corporation. All rights reserved.=
> >> +# Copyright (c) 2015 - 2020, Intel Corporation. All rights reserved.=
> >> # SPDX-License-Identifier: BSD-2-Clause-Patent > >> # > >> ## > >> @@ -10,9 +10,9 @@ > >> INF_VERSION =3D 0x00010005 > >> BASE_NAME =3D TpmMeasurementLibNull > >> FILE_GUID =3D 6DFD6E9F-9278-48D8-8F45-B6CFF2C= 2B69C > >> - MODULE_TYPE =3D UEFI_DRIVER > >> + MODULE_TYPE =3D BASE > >> VERSION_STRING =3D 1.0 > >> - LIBRARY_CLASS =3D TpmMeasurementLib|DXE_DRIVER > DXE_RUNTIME_DRIVER DXE_SMM_DRIVER UEFI_APPLICATION UEFI_DRIVER > >> + LIBRARY_CLASS =3D TpmMeasurementLib > >> MODULE_UNI_FILE =3D TpmMeasurementLibNull.uni > >> > >> # > >> > > > > (1) I agree this is a bugfix, and should be included in the stable tag= . > > > > > > (2) The commit message makes zero sense to me, on the other hand. I > > don't understand how DxeTpmMeasurementLib and PeiTpmMeasurementLib > are > > relevant at all. I also don't understand how TpmMeasurementLib > > "includes" DxeTpmMeasurementLib and PeiTpmMeasurementLib. > > > > I guess the intent is to say that *some* of the known TpmMeasurementLi= b > > instances are PeiTpmMeasurementLib and DxeTpmMeasurementLib. I guess > > that would be a valid statement, but it's still irrelevant here. > > > > The issue here is that *all* Null instances (regardless of library > > class) should have MODULE_TYPE=3DBASE, so that they can be consumed by= the > > broadest selection of client modules. This specific Null instance brea= ks > > that principle, and that's what the patch fixes. > > > > The fact that this particular Null instance happens to implement the > > TpmMeasurementLib class is irrelevant in this regard. > > > > Please update the commit message accordingly. (There is time for a > > repost, this patch certainly qualifies for both review and merging > > during the hard feature freeze.) Again, the bug we're fixing is that > > this is a Null instance that currently does not have MODULE_TYPE=3DBAS= E. > > > > (Removing the client type restrictions from the LIBRARY_CLASS line is > > correct, of course.) > > > > > > (3) The C file needs more changes. Because we're flipping the module > > type to BASE, we should replace the EFI_STATUS type and the EFI_xxx > > return values with RETURN_STATUS and RETURN_xxx, respectively. > > > > > > (4) Consequently, for RETURN_STATUS and RETURN_xxx, we should #include > > , rather than . > > 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 (=3D be made > RETURN_xxx) in the lib class header, and in all of the instances. Again, > existent call sites in edk2 should need no changes. (The call site in > OpenBoardPkg like does, though.) > > > (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 --_000_CY4PR21MB0743C103E8F4CB7B2D551A6BEF530CY4PR21MB0743namp_ Content-Type: text/html; charset="Windows-1252" Content-Transfer-Encoding: quoted-printable

Question (since it=92s been brought up): when *w= ouldn=92t* you use EFI_*? They=92re clearly superior in every way. I me= an, they=92ve got EFI right in the name.

 

- Bret

 

From: Yao, Jiewen via groups.io
Sent: Friday, August 28, 2020 5:20 PM
To: Laszlo Ersek; devel@edk2.groups.io; Zhang, Qi= 1
Cc: Wang, Jian J; Wu, Hao A
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 tha= t a
> client module that is more primitive than either a PEIM or a DXE_DRIV= ER
> wants to consume the lib instance. That requires that the lib class > header be first consumable by the client module. And for that, the li= b
> class header must not declare the interface with EFI_xxx in the first=
> place, but with RETURN_xxx.

[Jiewen] But I don=92t think it is absolutely necessary to change EFI_xxx = to RETURN_xxx in library class, just because a library instance could be PE= I 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 L= IBRARY_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.w= u@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=3Dhttps%3A%2F%2Fbugzil= la.tianocore.org%2Fshow_bug.cgi%3Fid%3D2940&amp;data=3D02%7C01%7Cbret.b= arkelew%40microsoft.com%7C7ae38c56bf854c2ea4c408d84bb147c7%7C72f988bf86f141= af91ab2d7cd011db47%7C1%7C0%7C637342572075610295&amp;sdata=3DDEVDpeDBr5m= TYuA0NdqgmGBUAdbQF1qDK2TuujmeSiQ%3D&amp;reserved=3D0
> >>
> >> TpmMeasurementLib includes DxeTpmMeasurementLib and
> PeiTpmMeasurementLib.
> >> So need to change TpmMeasurementLibNull to BASE library to a= void 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/TpmMeasurementLibNul= l.c   | 4 +++-
> >>  .../Library/TpmMeasurementLibNull/TpmMeasurementLibNul= l.inf | 6 +++---
> >>  2 files changed, 6 insertions(+), 4 deletions(-)
> >>
> >> diff --git
> a/MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.c<= br> > b/MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.c<= br> > >> index b9c5b68de8..ee3be62fc6 100644
> >> ---
> a/MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.c<= br> > >> +++
> b/MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.c<= br> > >> @@ -1,11 +1,13 @@
> >>  /** @file
> >>    This library is used by other modules to m= easure data to TPM.
> >>
> >> -Copyright (c) 2015, Intel Corporation. All rights reserved.= <BR>
> >> +Copyright (c) 2015-2020, Intel Corporation. All rights rese= rved. <BR>
> >>  SPDX-License-Identifier: BSD-2-Clause-Patent
> >>
> >>  **/
> >>
> >> +#include <Uefi/UefiBaseType.h>
> >> +
> >>  /**
> >>    Tpm measure and log data, and extend the m= easurement 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     &= nbsp;           &nbs= p;  =3D 0x00010005
> >>    BASE_NAME     &nb= sp;            =     =3D TpmMeasurementLibNull
> >>    FILE_GUID     &nb= sp;            =     =3D 6DFD6E9F-9278-48D8-8F45-B6CFF2C2B69C
> >> -  MODULE_TYPE       = ;             = =3D UEFI_DRIVER
> >> +  MODULE_TYPE       = ;             = =3D BASE
> >>    VERSION_STRING    &nbs= p;            =3D 1.= 0
> >> -  LIBRARY_CLASS      &nb= sp;           =3D TpmMeas= urementLib|DXE_DRIVER
> DXE_RUNTIME_DRIVER DXE_SMM_DRIVER UEFI_APPLICATION UEFI_DRIVER
> >> +  LIBRARY_CLASS      &nb= sp;           =3D TpmMeas= urementLib
> >>    MODULE_UNI_FILE    &nb= sp;           =3D TpmMeas= urementLibNull.uni
> >>
> >>  #
> >>
> >
> > (1) I agree this is a bugfix, and should be included in the stab= le tag.
> >
> >
> > (2) The commit message makes zero sense to me, on the other hand= . I
> > don't understand how DxeTpmMeasurementLib and PeiTpmMeasurementL= ib
> are
> > relevant at all. I also don't understand how TpmMeasurementLib > > "includes" DxeTpmMeasurementLib and PeiTpmMeasurementL= ib.
> >
> > I guess the intent is to say that *some* of the known TpmMeasure= mentLib
> > instances are PeiTpmMeasurementLib and DxeTpmMeasurementLib. I g= uess
> > that would be a valid statement, but it's still irrelevant here.=
> >
> > The issue here is that *all* Null instances (regardless of libra= ry
> > class) should have MODULE_TYPE=3DBASE, so that they can be consu= med by the
> > broadest selection of client modules. This specific Null instanc= e 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 mergi= ng
> > 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= = =3DBASE.
> >
> > (Removing the client type restrictions from the LIBRARY_CLASS li= ne is
> > correct, of course.)
> >
> >
> > (3) The C file needs more changes. Because we're flipping the mo= dule
> > 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 #i= nclude
> > <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_STA= TUS
> (and RETURN_xxx) in this Null library instance.
>
> Then we'll have an interesting situation where this library instance<= br> > will no longer match the lib class header --
> "MdeModulePkg/Include/Library/TpmMeasurementLib.h" will con= tinue
> 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 tha= t a
> client module that is more primitive than either a PEIM or a DXE_DRIV= ER
> wants to consume the lib instance. That requires that the lib class > header be first consumable by the client module. And for that, the li= b
> 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 shou= ld
> 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) i= s
> 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_STA= TUS
> -- 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 a= nd
> return values of TpmMeasureAndLogData() be cleaned up (=3D be made > RETURN_xxx) in the lib class header, and in all of the instances. Aga= in,
> 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, t= hen
> please don't say "some platform" in the commit message. Nam= e
> OpenBoardPkg, please.
>
> Thanks
> Laszlo


 

--_000_CY4PR21MB0743C103E8F4CB7B2D551A6BEF530CY4PR21MB0743namp_--