From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by mx.groups.io with SMTP id smtpd.web11.7714.1624861137366823781 for ; Sun, 27 Jun 2021 23:18:57 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@intel.onmicrosoft.com header.s=selector2-intel-onmicrosoft-com header.b=jqEpTMtm; spf=pass (domain: intel.com, ip: 192.55.52.115, mailfrom: hao.a.wu@intel.com) X-IronPort-AV: E=McAfee;i="6200,9189,10028"; a="207711814" X-IronPort-AV: E=Sophos;i="5.83,305,1616482800"; d="scan'208,217";a="207711814" Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga103.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 27 Jun 2021 23:18:54 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.83,305,1616482800"; d="scan'208,217";a="425019455" Received: from orsmsx603.amr.corp.intel.com ([10.22.229.16]) by orsmga002.jf.intel.com with ESMTP; 27 Jun 2021 23:18:53 -0700 Received: from orsmsx605.amr.corp.intel.com (10.22.229.18) by ORSMSX603.amr.corp.intel.com (10.22.229.16) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2242.4; Sun, 27 Jun 2021 23:18:53 -0700 Received: from ORSEDG602.ED.cps.intel.com (10.7.248.7) by orsmsx605.amr.corp.intel.com (10.22.229.18) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2242.4 via Frontend Transport; Sun, 27 Jun 2021 23:18:53 -0700 Received: from NAM11-DM6-obe.outbound.protection.outlook.com (104.47.57.176) by edgegateway.intel.com (134.134.137.103) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.2242.4; Sun, 27 Jun 2021 23:18:52 -0700 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=AKWMgDl7e7AXCvZfmesTeIPWRRGOS79KWoSafZoThIkTbXH7Yhg/YTgd3+btTIvlY3UZDIYrKtzMFBi6zANqg/B/KpODg2QtdrIPV/2yknE97t6frHsortPUnOK12DH5pWv7u7Bu/l8umfr2j6bfLIvpLrIQ8GCZ+K3AiVl0Jpg+kPjpI0lNgAYsLbVe3EZNJ5K2PNqS7hkZxK0EXkPVInK3ZfwN9bNKMYQOsUSf9xwERTTJU2Yk6+eDI7dPMHS4syIi7yP14QyhA1pz5UoGw07RAbwCpgzKfAe6uM4sNJgcH9GdVT0AtsfM8djUqutDpa6GFG3IQj2+nsvcUT3lgg== 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=tfXcWSFsPZhgnU1mMfp9oAum8m4ciNu14MbUChXp3BQ=; b=DCq2LsbVRGLsnBAMUvninx4eiKF9HnIiXhHQFFVzNvuk/CloCeg4LGIZKNTzLtreKjCY8+hl/ghvHtFcsN43KVrrML053xHFqp8mVXw0deZEA4/Xx4w+D8nZSYnvFHpYxNo0PK05JPmfv51axXWEQmeE63+GwRRp3ukQi5kPpfZqCtZUvMj0ROjDGo9GTNABI1aWbSwvPk5Mz9qDYzP59+e8rSwWIF1gTmSBRPN8RFlDLir1zRXD788Uaygas6Azu9KQPbBDr86xfDbJVeNgP5R7tuGEQiHtBzpWA2mla9Q6uVmSKyVRi2ZxjwaYKTu2OM5fd31V/AHJ4p2Vo03pkg== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=intel.com; dmarc=pass action=none header.from=intel.com; dkim=pass header.d=intel.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=intel.onmicrosoft.com; s=selector2-intel-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=tfXcWSFsPZhgnU1mMfp9oAum8m4ciNu14MbUChXp3BQ=; b=jqEpTMtmydGxbWtjnSyQad5cXdtJOlEs9WFyPITAIvhi+6rJvUMpvNYqKz+b/DWaeIb733sRw5ArkkO3KrVcq/PX5rxAS5dShFxUYzF3/nulEqzKzGLnvx7SqHlS6xH0xoK+titBbMlsekw9QyhcUDiuNtd2xRNIw3PtSs8aejQ= Received: from BN8PR11MB3666.namprd11.prod.outlook.com (2603:10b6:408:8c::19) by BN6PR1101MB2356.namprd11.prod.outlook.com (2603:10b6:404:9c::8) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4264.18; Mon, 28 Jun 2021 06:18:47 +0000 Received: from BN8PR11MB3666.namprd11.prod.outlook.com ([fe80::cd58:25ce:5017:619d]) by BN8PR11MB3666.namprd11.prod.outlook.com ([fe80::cd58:25ce:5017:619d%5]) with mapi id 15.20.4264.026; Mon, 28 Jun 2021 06:18:47 +0000 From: "Wu, Hao A" To: "devel@edk2.groups.io" , "bret.barkelew@microsoft.com" , "kuqin12@gmail.com" CC: "Kinney, Michael D" , Liming Gao , "Liu, Zhiguang" , "Andrew Fish" , Laszlo Ersek , "Lindholm, Leif" , Michael Kubacki Subject: Re: [EXTERNAL] Re: [edk2-devel] [PATCH v1 1/5] EDK2 Code First: PI Specification: EFI_MM_COMMUNICATE_HEADER Update Thread-Topic: [EXTERNAL] Re: [edk2-devel] [PATCH v1 1/5] EDK2 Code First: PI Specification: EFI_MM_COMMUNICATE_HEADER Update Thread-Index: AQHXXZpnzCD566nGBUCicqQmjnUV3asObx+ggAcmBICAAEdRIIAMjwIAgAAq0oCABnMNIA== Date: Mon, 28 Jun 2021 06:18:47 +0000 Message-ID: References: <20210610014259.1151-1-kuqin12@gmail.com> <20210610014259.1151-2-kuqin12@gmail.com> ,<064c216a-bb12-e2b2-6ae8-17f836a8e0a7@gmail.com> In-Reply-To: Accept-Language: en-US, zh-CN 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=2021-06-24T03:25:17.4038706Z; MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_ContentBits=0; MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_Method=Privileged dlp-product: dlpe-windows dlp-reaction: no-action dlp-version: 11.5.1.3 authentication-results: edk2.groups.io; dkim=none (message not signed) header.d=none;edk2.groups.io; dmarc=none action=none header.from=intel.com; x-originating-ip: [192.198.143.16] x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: f040bf3e-c5d0-4088-c657-08d939fc96e3 x-ms-traffictypediagnostic: BN6PR1101MB2356: x-ms-exchange-transport-forked: True x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:7219; x-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: ZeOaVOlNp79ItxTOvFi+YYfK7/KzTeQTmvH6WaJFgvILDmhOmXJ509+oLkWoPh9YQ/M16NRsPfl4IW2R19Mgkk2pbqa77JYGn3S4heIJw5EO9p52E+8mJsnKLqwdsQIpT/w8v0TKjWbttjm74+izsRmCtXDRydUBj97KnJykWcQW6o+SGyBK0gv4TLWh8wDpDGFmyAJu8zhc24yRxwOC7+AxeOSHMJSpxWFpg4GwGs4Uvusj+ZiyCHuCNs+EINh98Ei4CaQ7kaLSNZPvqFNsRkNxSgd/utfs1lUXUjy8Hpmlx/6/wHUGEbV7Yk6Qf1VwWADQ+zuspwl/gHGK5/NG+wG3WfkfFjolQCmdpxmvIvZhq2fvQEfTR7bwa5aESpm47T8q62rImQc7a03XPi+AEZfrGyh45KVKjgesq+mOYtlJXDmcvPN/hpl0IMbSDHPkB9Xt1AMsncOssZp+S+jJS5O0fGkR1CF0aeh5ekp8eAxepvLwxIyuT17Pcjpwy+ezrtbH0W1hdtmUIwc1elimut6zNG07oJQ8S51pTzS9xczlJy3xJ3CTVNkgrZhHn6IDMhhXvR/4YbifdbwE/ls7BenkLvP9SeyCx9VDJu9OctripVlUwgI2+svi1cnTvK6iDlL4RAApZ+E0HLsbj5xRN3dju0HrVnT5Mpwh5mudTWs3rEYOVsZjY+Doa8Vmo0u332J3b1r7c7tVW3OW/WlntgVwgsIx5C9V2lwr5+O7nzbFMy6i2wejm6BM42aBvHCf x-forefront-antispam-report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:BN8PR11MB3666.namprd11.prod.outlook.com;PTR:;CAT:NONE;SFS:(346002)(376002)(396003)(366004)(39860400002)(136003)(54906003)(478600001)(4326008)(6506007)(186003)(316002)(7696005)(53546011)(83380400001)(86362001)(966005)(26005)(110136005)(45080400002)(76236003)(71200400001)(2906002)(38100700002)(15650500001)(8936002)(8676002)(76116006)(66476007)(66946007)(52536014)(66556008)(64756008)(5660300002)(9686003)(19627235002)(66446008)(33656002)(30864003)(122000001)(166002)(55016002);DIR:OUT;SFP:1102; x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?77+QCDVUduvW/pnNeL00X/HjuYjwj2DPLGIryGN+xxQe/iSmCsHTsu2QIv8p?= =?us-ascii?Q?SKBdAXN2Sg7TB1MObeoaIpFjU7ehU5rSpbAACE/rI3lPHzXowddsUeLGmAOQ?= =?us-ascii?Q?gT1Xot0ZM4fXR3CyW9507z0v+QCV9KIlCQCDciAIeSS0Slx1adQObpP5yjxb?= =?us-ascii?Q?LKrROjqUCM5hU7BbiSHxOqpaFkwek27FI5/7190mliFQjXKYNopLMz793HAH?= =?us-ascii?Q?0CgTkIACN19aIIPQtJlEomqZ/jt3vcNe0EnxhCtjjosrWMvqzD/9gy0JrR+L?= =?us-ascii?Q?sjdwTQ0YZd8Xq8wkG6rZApi7P4h8fyyt6y0lOyrBOAs3eUKDbDPYz313bQS3?= =?us-ascii?Q?gUdTRhGWdYDNZuNpRgNmFQNr2t7jyj1Hcz+WzycdruxtRKObiH6AbHaLkdOX?= =?us-ascii?Q?L3NYY4XM9Sl3uO0OAH6FvSHMX0VgPi4uisdFPgekf5ovtIbCS/QDTTZczqfK?= =?us-ascii?Q?l2j3s3AdOCnSowgNOQFX6Jl0uHroNuWUzYgneHPGrGKEj2mgNFOLSyZBnz5Z?= =?us-ascii?Q?imIRlSF6Yks6Sc4GmlxlQT/rjvFOVt0lTTLDvk+UZ3OiGPsYAOHzuDO8s/it?= =?us-ascii?Q?6r9lbrMNaE9F9bu4toqbp8rbsx29+7BnXXshLKZP4WrraEP6e1+JN1Avwa3s?= =?us-ascii?Q?bXoju5b6pl5clwB62RXSbSKnJnib9+qC88HmBJwEQ8bpcjAwNQ4TWPnd9akm?= =?us-ascii?Q?XXA7whXLt2rRmtB9nwmlo9/BYoNGhUxNt+vz+FC3qo5FnM2xfU4IzHKQbOQ6?= =?us-ascii?Q?8pdLUYveCM07Dt6vdUts/cbAzAoPHQcp9nf+UWBper8f+/4AzV+MDAla9qEP?= =?us-ascii?Q?6+sMmnU+B3owXvDY+uplqbwFfJBF2q74oiyF0lxyI9DtnoJuO9XC1RpxDFY0?= =?us-ascii?Q?wPnTSARNkDnYtfJJqwYf5wiyAl5mfee3vSzIOU4fV+wiJ1Dul0gq0pR7ayJa?= =?us-ascii?Q?FWEgl4bmRfIyJTxiXJI9B4hv9aBDTpZRFJMKikY1soBN8ulHdIZYoTJbp8sq?= =?us-ascii?Q?xA9ObOV/gSpaH82oE+OCK3OF/CZA7GGiDjYnbCYHDNV9e9SyCh/Z1z439bpO?= =?us-ascii?Q?tSCoKGnrTmLRi07vkcI0er1QIyLJrncHeUX/wvapALxgP9VGp+IxdPShra2P?= =?us-ascii?Q?aGuOiimMMYqpzrQmN0mEpqI9/2wEl9ibe055MSg+0Ki1pAroMkSs56b5LYU4?= =?us-ascii?Q?XKETfT3/yqyOi9kzkBB0+FXlsA1MF6mwxF55iEBq3caYw3FTgwLP0SP9CZRa?= =?us-ascii?Q?OjIX34lqcDNphy4BxETpfhOTWZMtlTeV1vzYNaS2vwCz+C/RFmfEKREjRCH3?= =?us-ascii?Q?yS5uAC0z7goBmoGB8a2hDFa9?= MIME-Version: 1.0 X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: BN8PR11MB3666.namprd11.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: f040bf3e-c5d0-4088-c657-08d939fc96e3 X-MS-Exchange-CrossTenant-originalarrivaltime: 28 Jun 2021 06:18:47.3858 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: OhlBljPOluJ62NMBNXXHE+QfTVdWh/kI/HXY4OajYVw1s9gKEBQ2b7jXsBL5IZu2amRe9JTraKmgjlNAWaeEiA== X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN6PR1101MB2356 Return-Path: hao.a.wu@intel.com X-OriginatorOrg: intel.com Content-Language: en-US Content-Type: multipart/alternative; boundary="_000_BN8PR11MB3666920921B1CE209BC99BF1CA039BN8PR11MB3666namp_" --_000_BN8PR11MB3666920921B1CE209BC99BF1CA039BN8PR11MB3666namp_ Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hello Kun and Bret, sorry for the delayed response. I think I agree with the approach mentioned by Bret below: "deprecate, bre= ak builds, fix code, move forward". If we just modify the structure, it is likely that platform drivers that c= onsumes EFI_(S)MM_COMMUNICATE_HEADER may not be aware of the change and pot= entially consuming data at wrong offset. A build failure can be an indication to platform owners that certain drive= rs should be reviewed upon a checklist mentioned in this patch. Best Regards, Hao Wu From: devel@edk2.groups.io On Behalf Of Bret Barkel= ew via groups.io Sent: Thursday, June 24, 2021 11:27 AM To: devel@edk2.groups.io; kuqin12@gmail.com; Wu, Hao A Cc: Kinney, Michael D ; Liming Gao ; Liu, Zhiguang ; Andrew Fish ; Laszlo Ersek ; Lindholm, Leif ; Michael Kubacki Subject: Re: [EXTERNAL] Re: [edk2-devel] [PATCH v1 1/5] EDK2 Code First: P= I Specification: EFI_MM_COMMUNICATE_HEADER Update Yeah, the only other thought I had for moving forwards quickly (i.e. hacki= ly) is to define a new GUID that just means "I use an updated header config= uration" since the EFI_GUID field comes first and is a well-understood size= . I would prefer the "deprecate, break builds, fix code, move forward" appro= ach. - Bret From: Kun Qin via groups.io Sent: Wednesday, June 23, 2021 5:53 PM To: Wu, Hao A; devel@edk2.groups.io Cc: Kinney, Michael D; Liming Gao; Liu, Zhiguang; = Andrew Fish; Laszlo Ersek= ; Lindholm, Leif; Bret Barkelew; Michael Kubacki Subject: [EXTERNAL] Re: [edk2-devel] [PATCH v1 1/5] EDK2 Code First: PI Sp= ecification: EFI_MM_COMMUNICATE_HEADER Update Hi Hao, We have been circulating different ideas internally about how to enforce a warning around this change. There is an idea of deprecating the old structure and replacing it with a newly defined EFI_MM_COMMUNICATE_HEADER_V2. That way all consumers will be enforced to update their implementation and (hopefully) inspect the compatibility accordingly. In addition, we could add other fields such as signature and/or header version number for further extensibility. If there are code instances that uses "sizeof(EFI_GUID) + sizeof(UINTN)" in lieu of OFFSET_OF, this implementation is essentially decoupled from the structure definition. So compiler might not be able to help. In that case, runtime protections such as pool guard or stack guard might be usefu= l. Please let me know if you think header structure V2 is an idea worth to tr= y. Thanks, Kun On 06/15/2021 18:15, Wu, Hao A wrote: > Thanks Kun, > > I am a little concerned on whether there will be other missing cases tha= t are not covered by this patch series. > > I am also wondering is there any detection can be made to warn the cases= that code modification should be made after this structure update. > Otherwise, it will be the burden for the platform owners to review the p= latform codes following your guide mentioned in this patch. > > Hoping others can provide some inputs on this. > > Best Regards, > Hao Wu > >> -----Original Message----- >> From: Kun Qin > >> Sent: Wednesday, June 16, 2021 4:51 AM >> To: Wu, Hao A >; devel@ed= k2.groups.io >> Cc: Kinney, Michael D >; Liming Gao >> >; Liu, Zhigu= ang >; >> Andrew Fish >; Laszlo Ersek >; Leif >> Lindholm > >> Subject: Re: [edk2-devel] [PATCH v1 1/5] EDK2 Code First: PI Specificat= ion: >> EFI_MM_COMMUNICATE_HEADER Update >> >> Hi Hao, >> >> Sorry that I missed comments for this patch earlier. You are correct. I= only >> inspected SmmLockBoxPeiLib. The PEI instance handled mode switch with >> ```OFFSET_OF ``` function. But DXE instance still have a few use cases = that will >> be impacted. >> >> I will update this read me file and add a code change patch for this li= brary in >> v2. Thanks for pointing this out. >> >> Regards, >> Kun >> >> On 06/11/2021 00:46, Wu, Hao A wrote: >>>> -----Original Message----- >>>> From: devel@edk2.groups.io > On Behalf Of Kun >>>> Qin >>>> Sent: Thursday, June 10, 2021 9:43 AM >>>> To: devel@edk2.groups.io >>>> Cc: Kinney, Michael D >; Liming Gao >>>> >; Liu, Zhi= guang >; >>>> Andrew Fish >; Laszlo Ersek <= lersek@redhat.com>; Leif >>>> Lindholm > >>>> Subject: [edk2-devel] [PATCH v1 1/5] EDK2 Code First: PI Specificatio= n: >>>> EFI_MM_COMMUNICATE_HEADER Update >>>> >>>> REF: https://nam06.safelinks.protection.outlook.com/?url=3Dhttps%3A%2= F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3430&data=3D04%7C01%7C= Bret.Barkelew%40microsoft.com%7Ca480819ff5e84e12239f08d936aa7bc5%7C72f988bf= 86f141af91ab2d7cd011db47%7C1%7C0%7C637600928127681913%7CUnknown%7CTWFpbGZsb= 3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C100= 0&sdata=3D483mG24s%2Bp0xSF90Wf%2Fmh2TN1atrIQa5mOrLyEPCTuE%3D&reserv= ed=3D0 >>>> >>>> This change includes specification update markdown file that >>>> describes the proposed PI Specification v1.7 Errata A in detail and >>>> potential impact to the existing codebase. >>>> >>>> Cc: Michael D Kinney > >>>> Cc: Liming Gao > >>>> Cc: Zhiguang Liu > >>>> Cc: Andrew Fish > >>>> Cc: Laszlo Ersek > >>>> Cc: Leif Lindholm > >>>> >>>> Signed-off-by: Kun Qin > >>>> --- >>>> BZ3430-SpecChange.md | 88 ++++++++++++++++++++ >>>> 1 file changed, 88 insertions(+) >>>> >>>> diff --git a/BZ3430-SpecChange.md b/BZ3430-SpecChange.md new file >>>> mode >>>> 100644 index 000000000000..33a1ffda447b >>>> --- /dev/null >>>> +++ b/BZ3430-SpecChange.md >>>> @@ -0,0 +1,88 @@ >>>> +# Title: Change MessageLength Field of >> EFI_MM_COMMUNICATE_HEADER >>>> to >>>> +UINT64 >>>> + >>>> +## Status: Draft >>>> + >>>> +## Document: UEFI Platform Initialization Specification Version 1.7 >>>> +Errata A >>>> + >>>> +## License >>>> + >>>> +SPDX-License-Identifier: CC-BY-4.0 >>>> + >>>> +## Submitter: [TianoCore Community](https://nam06.safelinks.protecti= on.outlook.com/?url=3Dhttps%3A%2F%2Fwww.tianocore.org%2F&data=3D04%7C01= %7CBret.Barkelew%40microsoft.com%7Ca480819ff5e84e12239f08d936aa7bc5%7C72f98= 8bf86f141af91ab2d7cd011db47%7C1%7C0%7C637600928127681913%7CUnknown%7CTWFpbG= Zsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C= 1000&sdata=3DeRu4We7iSzuwrsS9MqIO2iqLxXHZ6CpUkInVpty554c%3D&reserve= d=3D0) >>>> + >>>> +## Summary of the change >>>> + >>>> +Change the `MessageLength` Field of >> `EFI_MM_COMMUNICATE_HEADER` >>>> from UINTN to UINT64 to remove architecture dependency: >>>> + >>>> +```c >>>> +typedef struct { >>>> + EFI_GUID HeaderGuid; >>>> + UINT64 MessageLength; >>>> + UINT8 Data[ANYSIZE_ARRAY]; >>>> +} EFI_MM_COMMUNICATE_HEADER; >>>> +``` >>>> + >>>> +## Benefits of the change >>>> + >>>> +In PI Spec v1.7 Errata A, Vol.4, Sec 5.7 MM Communication Protocol, >>>> +the >>>> MessageLength field of `EFI_MM_COMMUNICATE_HEADER` (also >> defined as >>>> `EFI_SMM_COMMUNICATE_HEADER`) is defined as type UINTN. >>>> + >>>> +But this structure, as a generic definition, could be used for both >>>> +PEI and >>>> DXE MM communication. Thus for a system that supports PEI MM launch, >>>> but operates PEI in 32bit mode and MM foundation in 64bit, the >>>> current `EFI_MM_COMMUNICATE_HEADER` definition will cause >> structure >>>> parse error due to UINTN used. >>>> + >>>> +## Impact of the change >>>> + >>>> +This change will impact the known structure consumers including: >>>> + >>>> +```bash >>>> +MdeModulePkg/Core/PiSmmCore/PiSmmIpl >>>> +MdeModulePkg/Application/SmiHandlerProfileInfo >>>> +MdeModulePkg/Application/MemoryProfileInfo >>>> +``` >>>> + >>>> +For consumers that are not using >>>> `OFFSET_OF(EFI_MM_COMMUNICATE_HEADER, Data)`, but performing >> explicit >>>> addition such as the existing >>>> >> MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo. >>>> c, one will need to change code implementation to match new structure >>>> definition. Otherwise, the code compiled on IA32 architecture will >>>> experience structure field dereference error. >>>> + >>>> +User who currently uses UINTN local variables as place holder of >>>> MessageLength will need to use caution to make cast from UINTN to >>>> UINT64 and vice versa. It is recommended to use `SafeUint64ToUintn` >>>> for such operations when the value is indeterministic. >>>> + >>>> +Note: MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxPeiLib is >> also >>>> consuming this structure, but it handled this size discrepancy >>>> internally. If this >>> >>> >>> Hello Kun, >>> >>> Sorry for a question. >>> I am not sure why the current codes in file SmmLockBoxDxeLib.c will wo= rk >> properly after the UINTN -> UINT64 change. >>> >>> For example: >>> LockBoxGetSmmCommBuffer(): >>> MinimalSizeNeeded =3D sizeof (EFI_GUID) + >>> sizeof (UINTN) + >>> MAX (sizeof (EFI_SMM_LOCK_BOX_PARAMETER_SAVE), >>> MAX (sizeof >> (EFI_SMM_LOCK_BOX_PARAMETER_SET_ATTRIBUTES), >>> MAX (sizeof >> (EFI_SMM_LOCK_BOX_PARAMETER_UPDATE), >>> MAX (sizeof >> (EFI_SMM_LOCK_BOX_PARAMETER_RESTORE), >>> sizeof >>> (EFI_SMM_LOCK_BOX_PARAMETER_RESTORE_ALL_IN_PLACE))))); >>> >>> SaveLockBox(): >>> UINT8 TempCommBuffer[sizeof(EFI_GUID) + = sizeof(UINTN) >> + sizeof(EFI_SMM_LOCK_BOX_PARAMETER_SAVE)]; >>> >>> Is the series missed changes for SmmLockBoxDxeLib or I got something >> wrong? >>> >>> Best Regards, >>> Hao Wu >>> >>> >>>> potential spec change is not applied, all applicable PEI MM >>>> communicate callers will need to use the same routine as that of >>>> SmmLockBoxPeiLib to invoke a properly populated >>>> EFI_MM_COMMUNICATE_HEADER to be used in X64 MM foundation. >>>> + >>>> +## Detailed description of the change [normative updates] >>>> + >>>> +### Specification Changes >>>> + >>>> +1. In PI Specification v1.7 Errata A: Vol. 4 Page-91, the definition >>>> +of >>>> `EFI_MM_COMMUNICATE_HEADER` should be changed from current: >>>> + >>>> +```c >>>> +typedef struct { >>>> + EFI_GUID HeaderGuid; >>>> + UINTN MessageLength; >>>> + UINT8 Data[ANYSIZE_ARRAY]; >>>> +} EFI_MM_COMMUNICATE_HEADER; >>>> +``` >>>> + >>>> +to: >>>> + >>>> +```c >>>> +typedef struct { >>>> + EFI_GUID HeaderGuid; >>>> + UINT64 MessageLength; >>>> + UINT8 Data[ANYSIZE_ARRAY]; >>>> +} EFI_MM_COMMUNICATE_HEADER; >>>> +``` >>>> + >>>> +### Code Changes >>>> + >>>> +1. Remove the explicit calculation of the offset of `Data` in >>>> `EFI_MM_COMMUNICATE_HEADER`. Thus applicable calculations of >>>> `sizeof(EFI_GUID) + sizeof(UINTN)` should be replaced with >>>> `OFFSET_OF(EFI_MM_COMMUNICATE_HEADER, Data)` or similar >> alternatives. >>>> These calculations are identified in: >>>> + >>>> +```bash >>>> >> +MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo. >>>> c >>>> +MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c >>>> +``` >>>> + >>>> +1. Resolve potentially mismatched type between `UINTN` and `UINT64`. >>>> This would occur when `MessageLength` or its derivitive are used for >>>> local calculation with existing `UINTN` typed variables. Code change >>>> regarding this perspective is per case evaluation: if the variables >>>> involved are all deterministic values, and there is no overflow or >>>> underflow risk, a cast operation (from `UINTN` to `UINT64`) can be >>>> safely used. Otherwise, the calculation will be performed in `UINT64` >>>> bitwidth and then convert to `UINTN` using `SafeUint64*` and >>>> `SafeUint64ToUintn`, respectively. These operations are identified in= : >>>> + >>>> +```bash >>>> +MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c >>>> >> +MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo. >>>> c >>>> +MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c >>>> +``` >>>> + >>>> +1. After all above changes applied and specification updated, >>>> `MdePkg/Include/Protocol/MmCommunication.h` will need to be >> updated >>>> to match new definition that includes the field type update. >>>> -- >>>> 2.31.1.windows.1 >>>> >>>> >>>> >>>> >>>> >>> --_000_BN8PR11MB3666920921B1CE209BC99BF1CA039BN8PR11MB3666namp_ Content-Type: text/html; charset="us-ascii" Content-Transfer-Encoding: quoted-printable

Hello Kun and Bret, sorry for the delayed response.=

 

I think I agree with the approach mentioned by Bret= below: “deprecate, break builds, fix code, move forward”.=

If we just modify the structure, it is likely that = platform drivers that consumes EFI_(S)MM_COMMUNICATE_HEADER may not be awar= e of the change and potentially consuming data at wrong offset.<= /p>

A build failure can be an indication to platform ow= ners that certain drivers should be reviewed upon a checklist mentioned in = this patch.

 

Best Regards,

Hao Wu

 

From: devel@edk2.groups.io <devel@edk2.gr= oups.io> On Behalf Of Bret Barkelew via groups.io
Sent: Thursday, June 24, 2021 11:27 AM
To: devel@edk2.groups.io; kuqin12@gmail.com; Wu, Hao A <hao.a.wu= @intel.com>
Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Liming Ga= o <gaoliming@byosoft.com.cn>; Liu, Zhiguang <zhiguang.liu@intel.co= m>; Andrew Fish <afish@apple.com>; Laszlo Ersek <lersek@redhat.= com>; Lindholm, Leif <leif@nuviainc.com>; Michael Kubacki <Mich= ael.Kubacki@microsoft.com>
Subject: Re: [EXTERNAL] Re: [edk2-devel] [PATCH v1 1/5] EDK2 Code F= irst: PI Specification: EFI_MM_COMMUNICATE_HEADER Update

 

Yeah, the only other thought I had for moving forwa= rds quickly (i.e. hackily) is to define a new GUID that just means “I= use an updated header configuration” since the EFI_GUID field comes = first and is a well-understood size.

 

I would prefer the “deprecate, break builds, = fix code, move forward” approach.

 

- Bret

 

From: Kun Qin via groups.io
Sent: Wednesday, June 23, 2021 5:53 PM
To: Wu, Hao A; devel@edk2.groups.io
Cc: Kinney, Michael D= ; Liming Gao; Liu, Zhiguang; Andrew Fish; Laszlo Ersek; Lindholm, Leif; Bret Ba= rkelew; Michael Kubacki
Subject: [EXTERNAL] Re: [edk2-devel] [PATCH v1 1/5] EDK2 Code First= : PI Specification: EFI_MM_COMMUNICATE_HEADER Update

 

Hi Hao,

We have been circulating different ideas internally about how to enforce <= br> a warning around this change.

There is an idea of deprecating the old structure and replacing it with a newly defined EFI_MM_COMMUNICATE_HEADER_V2. That way all consumers
will be enforced to update their implementation and (hopefully) inspect the compatibility accordingly. In addition, we could add other fields
such as signature and/or header version number for further extensibility.<= br>
If there are code instances that uses "sizeof(EFI_GUID) + sizeof(UINT= N)"
in lieu of OFFSET_OF, this implementation is essentially decoupled from the structure definition. So compiler might not be able to help. In that <= br> case, runtime protections such as pool guard or stack guard might be usefu= l.

Please let me know if you think header structure V2 is an idea worth to tr= y.

Thanks,
Kun

On 06/15/2021 18:15, Wu, Hao A wrote:
> Thanks Kun,
>
> I am a little concerned on whether there will be other missing cases = that are not covered by this patch series.
>
> I am also wondering is there any detection can be made to warn the ca= ses that code modification should be made after this structure update.
> Otherwise, it will be the burden for the platform owners to review th= e platform codes following your guide mentioned in this patch.
>
> Hoping others can provide some inputs on this.
>
> Best Regards,
> Hao Wu
>
>> -----Original Message-----
>> From: Kun Qin <kuqin12@gm= ail.com>
>> Sent: Wednesday, June 16, 2021 4:51 AM
>> To: Wu, Hao A <hao.a.wu@= intel.com>; devel@edk2.groups.io
>> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Liming Gao
>> <gaoliming@byosoft= .com.cn>; Liu, Zhiguang <zhiguang.liu@intel.com>;
>> Andrew Fish <afish@apple.co= m>; Laszlo Ersek <lersek@red= hat.com>; Leif
>> Lindholm <leif@nuviainc.c= om>
>> Subject: Re: [edk2-devel] [PATCH v1 1/5] EDK2 Code First: PI Spec= ification:
>> EFI_MM_COMMUNICATE_HEADER Update
>>
>> Hi Hao,
>>
>> Sorry that I missed comments for this patch earlier. You are corr= ect. I only
>> inspected SmmLockBoxPeiLib. The PEI instance handled mode switch = with
>> ```OFFSET_OF ``` function. But DXE instance still have a few use = cases that will
>> be impacted.
>>
>> I will update this read me file and add a code change patch for t= his library in
>> v2. Thanks for pointing this out.
>>
>> Regards,
>> Kun
>>
>> On 06/11/2021 00:46, Wu, Hao A wrote:
>>>> -----Original Message-----
>>>> From: devel@edk2.= groups.io <devel@edk2.groups= .io> On Behalf Of Kun
>>>> Qin
>>>> Sent: Thursday, June 10, 2021 9:43 AM
>>>> To: devel@edk2.gr= oups.io
>>>> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Liming Gao
>>>> <gaoliming= @byosoft.com.cn>; Liu, Zhiguang <zhiguang.liu@intel.com>;
>>>> Andrew Fish <afish@= apple.com>; Laszlo Ersek <le= rsek@redhat.com>; Leif
>>>> Lindholm <leif@nu= viainc.com>
>>>> Subject: [edk2-devel] [PATCH v1 1/5] EDK2 Code First: PI = Specification:
>>>> EFI_MM_COMMUNICATE_HEADER Update
>>>>
>>>> REF: https://nam06.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F%2Fbugzil= la.tianocore.org%2Fshow_bug.cgi%3Fid%3D3430&amp;data=3D04%7C01%7CBret.B= arkelew%40microsoft.com%7Ca480819ff5e84e12239f08d936aa7bc5%7C72f988bf86f141= af91ab2d7cd011db47%7C1%7C0%7C637600928127681913%7CUnknown%7CTWFpbGZsb3d8eyJ= WIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&= amp;sdata=3D483mG24s%2Bp0xSF90Wf%2Fmh2TN1atrIQa5mOrLyEPCTuE%3D&amp;rese= rved=3D0
>>>>
>>>> This change includes specification update markdown file t= hat
>>>> describes the proposed PI Specification v1.7 Errata A in = detail and
>>>> potential impact to the existing codebase.
>>>>
>>>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>>>> Cc: Liming Gao <gaoliming@byosoft.com.cn>
>>>> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
>>>> Cc: Andrew Fish <af= ish@apple.com>
>>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>>> Cc: Leif Lindholm <leif@nuviainc.com>
>>>>
>>>> Signed-off-by: Kun Qin <kuqin12@gmail.com>
>>>> ---
>>>>    BZ3430-SpecChange.md | 88 +++++++++++++= +++++++
>>>>    1 file changed, 88 insertions(+)
>>>>
>>>> diff --git a/BZ3430-SpecChange.md b/BZ3430-SpecChange.md = new file
>>>> mode
>>>> 100644 index 000000000000..33a1ffda447b
>>>> --- /dev/null
>>>> +++ b/BZ3430-SpecChange.md
>>>> @@ -0,0 +1,88 @@
>>>> +# Title: Change MessageLength Field of
>> EFI_MM_COMMUNICATE_HEADER
>>>> to
>>>> +UINT64
>>>> +
>>>> +## Status: Draft
>>>> +
>>>> +## Document: UEFI Platform Initialization Specification = Version 1.7
>>>> +Errata A
>>>> +
>>>> +## License
>>>> +
>>>> +SPDX-License-Identifier: CC-BY-4.0
>>>> +
>>>> +## Submitter: [TianoCore Community](https://nam06.safelinks.protectio= n.outlook.com/?url=3Dhttps%3A%2F%2Fwww.tianocore.org%2F&amp;data=3D04%7= C01%7CBret.Barkelew%40microsoft.com%7Ca480819ff5e84e12239f08d936aa7bc5%7C72= f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637600928127681913%7CUnknown%7CTWF= pbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D= %7C1000&amp;sdata=3DeRu4We7iSzuwrsS9MqIO2iqLxXHZ6CpUkInVpty554c%3D&= amp;reserved=3D0)
>>>> +
>>>> +## Summary of the change
>>>> +
>>>> +Change the `MessageLength` Field of
>> `EFI_MM_COMMUNICATE_HEADER`
>>>> from UINTN to UINT64 to remove architecture dependency: >>>> +
>>>> +```c
>>>> +typedef struct {
>>>> +  EFI_GUID  HeaderGuid;
>>>> +  UINT64    MessageLength;
>>>> +  UINT8     Data[ANYSIZE_ARRAY]= ;
>>>> +} EFI_MM_COMMUNICATE_HEADER;
>>>> +```
>>>> +
>>>> +## Benefits of the change
>>>> +
>>>> +In PI Spec v1.7 Errata A, Vol.4, Sec 5.7 MM Communicatio= n Protocol,
>>>> +the
>>>> MessageLength field of `EFI_MM_COMMUNICATE_HEADER` (also<= br> >> defined as
>>>> `EFI_SMM_COMMUNICATE_HEADER`) is defined as type UINTN. >>>> +
>>>> +But this structure, as a generic definition, could be us= ed for both
>>>> +PEI and
>>>> DXE MM communication. Thus for a system that supports PEI= MM launch,
>>>> but operates PEI in 32bit mode and MM foundation in 64bit= , the
>>>> current `EFI_MM_COMMUNICATE_HEADER` definition will cause=
>> structure
>>>> parse error due to UINTN used.
>>>> +
>>>> +## Impact of the change
>>>> +
>>>> +This change will impact the known structure consumers in= cluding:
>>>> +
>>>> +```bash
>>>> +MdeModulePkg/Core/PiSmmCore/PiSmmIpl
>>>> +MdeModulePkg/Application/SmiHandlerProfileInfo
>>>> +MdeModulePkg/Application/MemoryProfileInfo
>>>> +```
>>>> +
>>>> +For consumers that are not using
>>>> `OFFSET_OF(EFI_MM_COMMUNICATE_HEADER, Data)`, but perform= ing
>> explicit
>>>> addition such as the existing
>>>>
>> MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileI= nfo.
>>>> c, one will need to change code implementation to match n= ew structure
>>>> definition. Otherwise, the code compiled on IA32 architec= ture will
>>>> experience structure field dereference error.
>>>> +
>>>> +User who currently uses UINTN local variables as place h= older of
>>>> MessageLength will need to use caution to make cast from = UINTN to
>>>> UINT64 and vice versa. It is recommended to use `SafeUint= 64ToUintn`
>>>> for such operations when the value is indeterministic. >>>> +
>>>> +Note: MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxPeiLi= b is
>> also
>>>> consuming this structure, but it handled this size discre= pancy
>>>> internally. If this
>>>
>>>
>>> Hello Kun,
>>>
>>> Sorry for a question.
>>> I am not sure why the current codes in file SmmLockBoxDxeLib.= c will work
>> properly after the UINTN -> UINT64 change.
>>>
>>> For example:
>>> LockBoxGetSmmCommBuffer():
>>>     MinimalSizeNeeded =3D sizeof (EFI_GUI= D) +
>>>          &n= bsp;            = ;  sizeof (UINTN) +
>>>          &n= bsp;            = ;  MAX (sizeof (EFI_SMM_LOCK_BOX_PARAMETER_SAVE),
>>>          &n= bsp;            = ;       MAX (sizeof
>> (EFI_SMM_LOCK_BOX_PARAMETER_SET_ATTRIBUTES),
>>>          &n= bsp;            = ;            MAX (si= zeof
>> (EFI_SMM_LOCK_BOX_PARAMETER_UPDATE),
>>>          &n= bsp;            = ;            &n= bsp;    MAX (sizeof
>> (EFI_SMM_LOCK_BOX_PARAMETER_RESTORE),
>>>          &n= bsp;            = ;            &n= bsp;         sizeof
>>> (EFI_SMM_LOCK_BOX_PARAMETER_RESTORE_ALL_IN_PLACE)))));
>>>
>>> SaveLockBox():
>>>     UINT8     &n= bsp;            = ;         TempCommBuffer[sizeof(EFI= _GUID) + sizeof(UINTN)
>> + sizeof(EFI_SMM_LOCK_BOX_PARAMETER_SAVE)];
>>>
>>> Is the series missed changes for SmmLockBoxDxeLib or I got so= mething
>> wrong?
>>>
>>> Best Regards,
>>> Hao Wu
>>>
>>>
>>>> potential spec change is not applied, all applicable PEI = MM
>>>> communicate callers will need to use the same routine as = that of
>>>> SmmLockBoxPeiLib to invoke a properly populated
>>>> EFI_MM_COMMUNICATE_HEADER to be used in X64 MM foundation= .
>>>> +
>>>> +## Detailed description of the change [normative updates= ]
>>>> +
>>>> +### Specification Changes
>>>> +
>>>> +1. In PI Specification v1.7 Errata A: Vol. 4 Page-91, th= e definition
>>>> +of
>>>> `EFI_MM_COMMUNICATE_HEADER` should be changed from curren= t:
>>>> +
>>>> +```c
>>>> +typedef struct {
>>>> +  EFI_GUID  HeaderGuid;
>>>> +  UINTN     MessageLength;
>>>> +  UINT8     Data[ANYSIZE_ARRAY]= ;
>>>> +} EFI_MM_COMMUNICATE_HEADER;
>>>> +```
>>>> +
>>>> +to:
>>>> +
>>>> +```c
>>>> +typedef struct {
>>>> +  EFI_GUID  HeaderGuid;
>>>> +  UINT64    MessageLength;
>>>> +  UINT8     Data[ANYSIZE_ARRAY]= ;
>>>> +} EFI_MM_COMMUNICATE_HEADER;
>>>> +```
>>>> +
>>>> +### Code Changes
>>>> +
>>>> +1. Remove the explicit calculation of the offset of `Dat= a` in
>>>> `EFI_MM_COMMUNICATE_HEADER`. Thus applicable calculations= of
>>>> `sizeof(EFI_GUID) + sizeof(UINTN)` should be replaced wit= h
>>>> `OFFSET_OF(EFI_MM_COMMUNICATE_HEADER, Data)` or similar >> alternatives.
>>>> These calculations are identified in:
>>>> +
>>>> +```bash
>>>>
>> +MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfile= Info.
>>>> c
>>>> +MdeModulePkg/Application/MemoryProfileInfo/MemoryProfile= Info.c
>>>> +```
>>>> +
>>>> +1. Resolve potentially mismatched type between `UINTN` a= nd `UINT64`.
>>>> This would occur when `MessageLength` or its derivitive a= re used for
>>>> local calculation with existing `UINTN` typed variables. = Code change
>>>> regarding this perspective is per case evaluation: if the= variables
>>>> involved are all deterministic values, and there is no ov= erflow or
>>>> underflow risk, a cast operation (from `UINTN` to `UINT64= `) can be
>>>> safely used. Otherwise, the calculation will be performed= in `UINT64`
>>>> bitwidth and then convert to `UINTN` using `SafeUint64*` = and
>>>> `SafeUint64ToUintn`, respectively. These operations are i= dentified in:
>>>> +
>>>> +```bash
>>>> +MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
>>>>
>> +MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfile= Info.
>>>> c
>>>> +MdeModulePkg/Application/MemoryProfileInfo/MemoryProfile= Info.c
>>>> +```
>>>> +
>>>> +1. After all above changes applied and specification upd= ated,
>>>> `MdePkg/Include/Protocol/MmCommunication.h` will need to = be
>> updated
>>>> to match new definition that includes the field type upda= te.
>>>> --
>>>> 2.31.1.windows.1
>>>>
>>>>
>>>>
>>>>
>>>>
>>>



 

--_000_BN8PR11MB3666920921B1CE209BC99BF1CA039BN8PR11MB3666namp_--