From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from NAM11-CO1-obe.outbound.protection.outlook.com (NAM11-CO1-obe.outbound.protection.outlook.com [40.107.220.129]) by mx.groups.io with SMTP id smtpd.web12.741.1588788472886742576 for ; Wed, 06 May 2020 11:07:53 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@microsoft.com header.s=selector2 header.b=JbjwsPH6; spf=pass (domain: microsoft.com, ip: 40.107.220.129, mailfrom: bret.barkelew@microsoft.com) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=dxU1uszq7Ed4lawKsljI1pqhM3Hgh//TLpKS10pqO37W0xI84mclYq4YJ0jIbizIjI8SuFUvudovVVcCrKGlXsOvsqmVt2+VA1ntbnNeJcSsTwOpa5Kp9Jmqh3FMAOFLZXF+KTR7EG7iGo0ObH3vi6EIg/SVAtBXGdCtKRsV49d9ZTUv7vkr1aVspKLA0jVRmVJ4OZ40UgDgYCStpclAnAD1M/YXwWokcqP3OM3MhY1b2lLXV16f+ddZw32kQY3wE83zzma8vKQFK/l/E2hfCzAJCKYaeA1Y7mY9gkXX+/E196WcIfOKMa39eS6ZF0Ua0lyH0sPnXV7Ir8u52WO22A== 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=6+4SpJKfu97K1wkQjyIKouBiY0ivs3U6BL0bxJgh0Kk=; b=D1rgVTcfGkJJIUjIGXrIAtBoEP8rg57H2SpwarWt1lLJP6/nCDZbqscgHCxgUOFDnDdAeaL4DSlHAd4z+mSm4+V3FY5i54IrIMpTDYUBlBYWtzsCvmFzabNxkDmkQQLav98SdbVg57FxfBUColJ2pZ0nKuaN2T5x9+wKuRMkq/oTiZXELxLXJhTTr+SafNwKJVRjHPIw/dE7auJaTkJMyKFhky6xK0nh2GP22Yfk7AY69cq0rIIrmWBek81rMOvJ4RH++AYVanUVKFxqWPj1Nkjg0td0x1spUDE0iExgGVrXe/LLz6IaDKsxoijRV/VLek+pItsZX+azcPDqYs50NQ== 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=6+4SpJKfu97K1wkQjyIKouBiY0ivs3U6BL0bxJgh0Kk=; b=JbjwsPH6EB8g9EZEt+KPORUXNYO9epGm4CZpK5GI8qVBs7NE7eQptgT4aTtceDBI0USzRP1W0FwwGS3dfmyCaiQaeUQJtM8CBtC/osaieGh8NErskg1GfPedDg0Gm6F5oJyqt5/OobbER1sXP3+EpssM9wN9g6Np/WNIag1qnRY= Received: from CY4PR21MB0743.namprd21.prod.outlook.com (2603:10b6:903:b2::9) by CY4PR21MB0501.namprd21.prod.outlook.com (2603:10b6:903:dc::11) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3000.0; Wed, 6 May 2020 18:07:51 +0000 Received: from CY4PR21MB0743.namprd21.prod.outlook.com ([fe80::90d:10d9:c5bc:5318]) by CY4PR21MB0743.namprd21.prod.outlook.com ([fe80::90d:10d9:c5bc:5318%11]) with mapi id 15.20.2979.017; Wed, 6 May 2020 18:07:51 +0000 From: "Bret Barkelew" To: "devel@edk2.groups.io" , "lersek@redhat.com" , Tom Lendacky CC: Jordan Justen , Ard Biesheuvel , Liming Gao , Eric Dong , Ray Ni , Brijesh Singh , Anthony Perard , Benjamin You , Guo Dong , Julien Grall , Maurice Ma , Andrew Fish Subject: Re: [EXTERNAL] Re: [edk2-devel] [PATCH 4/4] UefiCpuPkg/CpuExceptionHandler: Revert binary patching in standard CpuExceptionHandlerLib Thread-Topic: [EXTERNAL] Re: [edk2-devel] [PATCH 4/4] UefiCpuPkg/CpuExceptionHandler: Revert binary patching in standard CpuExceptionHandlerLib Thread-Index: AQHWIyq4asUh0ZfLyUe6V71r6KueBqibILaAgAAhI4CAABfWhg== Date: Wed, 6 May 2020 18:07:50 +0000 Message-ID: References: <26e51d24-fe19-4fa1-6bac-6936041af39d@amd.com>,<39964d2d-e9fa-31bb-78b4-f65ba54a9f36@redhat.com> In-Reply-To: <39964d2d-e9fa-31bb-78b4-f65ba54a9f36@redhat.com> 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-05-06T17:58:55.3423744Z;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.83.205] x-ms-publictraffictype: Email x-ms-office365-filtering-ht: Tenant x-ms-office365-filtering-correlation-id: aa4d113f-a679-4c4d-8419-08d7f1e8641e x-ms-traffictypediagnostic: CY4PR21MB0501: x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:10000; x-forefront-prvs: 03950F25EC x-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: c8lzkzcMqWZm/F0dSPwo90POQ1585PW6jhciqEwkDnXYKfH0DZL8v/DqVYncgDkI84zyGDTYiFHsddEX+XPWXBpd6SqpUpCTZPvQ5emoGVgtu4My78GiKsFXLY1Q6qwhHK/Zkq7bV1IcSbfjv1ocKcXcp+nVy3snFtSatUJnmoIvy9173lTF9x0AJGjqV2qFf2V2k0QKp84VV9SYJLP55rfQOiO7xKaKO2oQdYgzz48WAg4rzcw5koBxo8eKMUBbPbUCmzHeavYIhXNmO9SunrafFXFdDvXYLu6rtL/tfgHLu8b8XSEZ4LfqwOa1w8qXmjn9QDCd0DAYyYsllMzMq2IcrJJ8zP9tfLtEitUDuU3tNv5Mrjx96Ubgj5tR/Nc0jJ/6VGvxd4HmyWco6U5LNrr/+AJ/wNTYMqw7Mhfu8eJile7NsuBB07urQW1zBz4PG1ezxlcm4XKmx2726/k8dMonwmESBX8Sw2bImciQWG0EGcQa5EECmDxeurFMaF1/tZbClPosgiCQo7fKs2KJO0Pgj6I2ADnQsnr9rjxCiWFra0k9JOQ6mD+l2MOLyqCgW2Gp9G32Gr/A3Gw14eNFI/F20vKEiECnNQ579bl2kpA= 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;SFTY:;SFS:(4636009)(39860400002)(396003)(136003)(366004)(346002)(376002)(33430700001)(166002)(186003)(7696005)(26005)(6506007)(53546011)(52536014)(9686003)(55016002)(33440700001)(71200400001)(110136005)(76116006)(82960400001)(82950400001)(4326008)(66556008)(54906003)(91956017)(66946007)(64756008)(316002)(66476007)(66446008)(8676002)(8990500004)(16799955002)(5660300002)(2906002)(966005)(19627235002)(10290500003)(33656002)(30864003)(7416002)(86362001)(478600001)(8936002);DIR:OUT;SFP:1102; x-ms-exchange-antispam-messagedata: FEVqy9xDLgGhOHQsUGg6I6feaAq/ND+HW0P/Z9mEOA+CF6lrYZdQRdTDR+eGAexW7z4dW64meMHCT18253Jb9HgRMHqg3eOOwRaE4ZGPqVe0nQU/Fh+08iKTqMQ2g+4zRm1uZvUdHiR7A9saBCaZ/ncCgj7y5gfKIx+KWj5+bMKTlOtcetbzHrRTj+WCz+06oqD5SEJ7iAm+bGiqjwIwjMUiwQPS3jx2GnU9d/Ts2RP0cbJ1t2J58eTqH/wCHh0ugEx3Xz04zWPeaUnNp9RbTeXbJngnJGwrWYVsgKPETk6PEb3BufqhN91bOJh1vi/ds3tPkfR+Wgy64BBUUV5T7l172hQp+3YSVtGV/pjjKIspJ2cJ2b3vIiisuZ7jXtaDAl6X7nvDFq6Kn7WWsytwLD4Ujs/J9jP9TUTSPL4gTTf7JBwMPBEHb+CqiqAifgNHvrLsL2uzc6RjgO1RZogzJ0AUpP3azaqTEu+9H6OXMrjNK9+gPq4/mSbzI5fHMnUW1RBuikfhwlca9dVQ12qds04GWK9uhqVQst6u26WODJt6P0NcSSALyYaoiL6k1Ual4Kjq5UHHbWIr2n0jdBENLgONPPHXFQ1UPlBZi97++KteKFjqtTyRmbsfZlp1DWwLsbsNwJThBJggglAj6+nICF4GbCvNG4wSFRc9d9fEC7WSjev37QcMprJ18Y7DQEzsrUsOqnOcUXv2zfxsGXtczIa+MquviPw8V0qCymrlH6PozZawpckZlkG0WEOh7NveXP6HyRTJvXPfld3DjwR4d1Yr2OV8I8/mbyBX7h8VYfs= x-ms-exchange-transport-forked: True MIME-Version: 1.0 X-OriginatorOrg: microsoft.com X-MS-Exchange-CrossTenant-Network-Message-Id: aa4d113f-a679-4c4d-8419-08d7f1e8641e X-MS-Exchange-CrossTenant-originalarrivaltime: 06 May 2020 18:07:50.8938 (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: 3X6v7uOBNNYKwjywZskE46kIh7RgGAAG30QpHiM+axm1dsoGf/i7cu0oXRybZGlRDTmlXj9MRliChb/hKKc4rQ== X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY4PR21MB0501 Content-Language: en-US Content-Type: multipart/alternative; boundary="_000_CY4PR21MB0743420B913B1BC1747EC18BEFA40CY4PR21MB0743namp_" --_000_CY4PR21MB0743420B913B1BC1747EC18BEFA40CY4PR21MB0743namp_ Content-Type: text/plain; charset="Windows-1252" Content-Transfer-Encoding: quoted-printable > Should that > section not use the !if check and just list both .inf files > (SecPeiCpuExceptionHandlerLib.inf and > Xcode5SecPeiCpuExceptionHandlerLib.inf)? Hmmm, this is a very good point; after all, the updated (=3Dreverted) "SecPeiCpuExceptionHandlerLib.inf" instance will not build with XCODE5. Therefore we should list both lib instance INF files under [Components], but make "SecPeiCpuExceptionHandlerLib.inf" conditional on non-XCODE5. Xcode5SecPeiCpuExceptionHandlerLib.inf could be added to the ignore list i= n the CI yaml file. PR gating CI currently only uses GCC and VS2017/19 and = shouldn=92t have a problem with the reverted lib. This makes Xcode5SecPeiCp= uExceptionHandlerLib the exception which can be documented in the ignore li= st (why it=92s being ignored). Thoughts? - Bret From: Laszlo Ersek via groups.io Sent: Wednesday, May 6, 2020 9:33 AM To: Tom Lendacky; devel@edk2.groups.io Cc: Jordan Justen; Ard Biesheuvel; Liming Gao; Eric = Dong; Ray Ni; Brijesh = Singh; Anthony Perard; Benjamin You; Guo Dong; Julien Grall; Maurice Ma; Andrew Fish Subject: [EXTERNAL] Re: [edk2-devel] [PATCH 4/4] UefiCpuPkg/CpuExceptionHa= ndler: Revert binary patching in standard CpuExceptionHandlerLib On 05/06/20 16:35, Tom Lendacky wrote: > On 5/5/20 5:15 PM, Laszlo Ersek via groups.io wrote: >> On 05/01/20 22:17, Lendacky, Thomas wrote: >>> BZ: >>> https://nam06.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F%2Fbu= gzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2340&data=3D02%7C01%7CBret.B= arkelew%40microsoft.com%7Ca9365fbefc53477c9e3308d7f1db4560%7C72f988bf86f141= af91ab2d7cd011db47%7C1%7C0%7C637243796372782584&sdata=3DR%2B5IgncDzx7vi= v3yIwX3MloX1QlzuUJ4bZlKnc%2B0xoI%3D&reserved=3D0 >>> >>> >>> Now that an XCODE5 specific CpuExceptionHandlerLib library is in place= , >>> revert the changes made to the ExceptionHandlerAsm.nasm in commit >>> 2db0ccc2d7fe ("UefiCpuPkg: Update CpuExceptionHandlerLib pass XCODE5 >>> tool >>> chain") so that binary patching of flash code is not performed. >>> >>> Cc: Eric Dong >>> Cc: Ray Ni >>> Cc: Laszlo Ersek >>> Cc: Liming Gao >>> Signed-off-by: Tom Lendacky >>> --- >>> .../X64/ExceptionHandlerAsm.nasm | 25 +++++------------= -- >>> 1 file changed, 6 insertions(+), 19 deletions(-) >>> >>> diff --git >>> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.na= sm >>> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.na= sm >>> index 19198f273137..3814f9de3703 100644 >>> --- >>> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.na= sm >>> +++ >>> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.na= sm >>> @@ -34,7 +34,7 @@ AsmIdtVectorBegin: >>> db 0x6a ; push #VectorNum >>> db ($ - AsmIdtVectorBegin) / ((AsmIdtVectorEnd - >>> AsmIdtVectorBegin) / 32) ; VectorNum >>> push rax >>> - mov rax, strict qword 0 ; mov rax, >>> ASM_PFX(CommonInterruptEntry) >>> + mov rax, ASM_PFX(CommonInterruptEntry) >>> jmp rax >>> %endrep >>> AsmIdtVectorEnd: >>> @@ -44,8 +44,7 @@ HookAfterStubHeaderBegin: >>> @VectorNum: >>> db 0 ; 0 will be fixed >>> push rax >>> - mov rax, strict qword 0 ; mov rax, >>> HookAfterStubHeaderEnd >>> -JmpAbsoluteAddress: >>> + mov rax, HookAfterStubHeaderEnd >>> jmp rax >>> HookAfterStubHeaderEnd: >>> mov rax, rsp >>> @@ -257,7 +256,8 @@ HasErrorCode: >>> ; and make sure RSP is 16-byte aligned >>> ; >>> sub rsp, 4 * 8 + 8 >>> - call ASM_PFX(CommonExceptionHandler) >>> + mov rax, ASM_PFX(CommonExceptionHandler) >>> + call rax >>> add rsp, 4 * 8 + 8 >>> >>> cli >>> @@ -365,24 +365,11 @@ DoIret: >>> ; comments here for definition of address map >>> global ASM_PFX(AsmGetTemplateAddressMap) >>> ASM_PFX(AsmGetTemplateAddressMap): >>> - lea rax, [AsmIdtVectorBegin] >>> + mov rax, AsmIdtVectorBegin >>> mov qword [rcx], rax >>> mov qword [rcx + 0x8], (AsmIdtVectorEnd - >>> AsmIdtVectorBegin) / 32 >>> - lea rax, [HookAfterStubHeaderBegin] >>> + mov rax, HookAfterStubHeaderBegin >>> mov qword [rcx + 0x10], rax >>> - >>> -; Fix up CommonInterruptEntry address >>> - lea rax, [ASM_PFX(CommonInterruptEntry)] >>> - lea rcx, [AsmIdtVectorBegin] >>> -%rep 32 >>> - mov qword [rcx + (JmpAbsoluteAddress - 8 - >>> HookAfterStubHeaderBegin)], rax >>> - add rcx, (AsmIdtVectorEnd - AsmIdtVectorBegin) / 32 >>> -%endrep >>> -; Fix up HookAfterStubHeaderEnd >>> - lea rax, [HookAfterStubHeaderEnd] >>> - lea rcx, [JmpAbsoluteAddress] >>> - mov qword [rcx - 8], rax >>> - >>> ret >>> >>> >>> ;---------------------------------------------------------------------= ---------------- >>> >>> >> >> With this patch applied, the differences with the "original" remain: >> >> $ git diff 2db0ccc2d7fe^..HEAD -- \ >> >> UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm >> >>> diff --git >>> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.na= sm >>> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.na= sm >>> index ba8993d84b0b..3814f9de3703 100644 >>> --- >>> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.na= sm >>> +++ >>> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.na= sm >>> @@ -1,12 +1,6 @@ >>> >>> ;---------------------------------------------------------------------= --------- >>> ; >>> -; Copyright (c) 2012 - 2014, Intel Corporation. All rights >>> reserved.
>>> -; This program and the accompanying materials >>> -; are licensed and made available under the terms and conditions of >>> the BSD License >>> -; which accompanies this distribution. The full text of the license >>> may be found at >>> -; >>> https://nam06.safelinks.protection.outlook.com/?url=3Dhttp%3A%2F%2Fope= nsource.org%2Flicenses%2Fbsd-license.php&data=3D02%7C01%7CBret.Barkelew= %40microsoft.com%7Ca9365fbefc53477c9e3308d7f1db4560%7C72f988bf86f141af91ab2= d7cd011db47%7C1%7C0%7C637243796372792580&sdata=3DCZovXRJ6HURgo6sz%2FDi5= 5SUy9IsrJ2pFzcHX%2Bp%2Fv8qA%3D&reserved=3D0. >>> >>> -; >>> -; THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASI= S, >>> -; WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS >>> OR IMPLIED. >>> +; Copyright (c) 2012 - 2018, Intel Corporation. All rights >>> reserved.
>>> +; SPDX-License-Identifier: BSD-2-Clause-Patent >>> ; >>> ; Module Name: >>> ; >> >> This is expected. >> >>> @@ -189,17 +183,19 @@ HasErrorCode: >>> push rax >>> push rax >>> sidt [rsp] >>> - xchg rax, [rsp + 2] >>> - xchg rax, [rsp] >>> - xchg rax, [rsp + 8] >>> + mov bx, word [rsp] >>> + mov rax, qword [rsp + 2] >>> + mov qword [rsp], rax >>> + mov word [rsp + 8], bx >>> >>> xor rax, rax >>> push rax >>> push rax >>> sgdt [rsp] >>> - xchg rax, [rsp + 2] >>> - xchg rax, [rsp] >>> - xchg rax, [rsp + 8] >>> + mov bx, word [rsp] >>> + mov rax, qword [rsp + 2] >>> + mov qword [rsp], rax >>> + mov word [rsp + 8], bx >>> >>> ;; UINT64 Ldtr, Tr; >>> xor rax, rax >>> >> >> Also expected, from commit f4c898f2b2db >> ("UefiCpuPkg/CpuExceptionHandlerLib: Fix split lock", 2019-09-20). >> >> Therefore, for this patch: >> >> Reviewed-by: Laszlo Ersek >> >> *However*, this revert must be restricted to the original >> "SecPeiCpuExceptionHandlerLib.inf" instance, i.e. where binary patching >> is not acceptable. (Otherwise, in combination with my request (1) under >> patch#1, we'd needlessly break the PEI / DXE / SMM lib instances under >> XCODE5.) >> >> (1) Therefore, please insert a new patch between patches #1 and #2, suc= h >> that the new patch flip >> >> - PeiCpuExceptionHandlerLib.inf >> - DxeCpuExceptionHandlerLib.inf >> - SmmCpuExceptionHandlerLib.inf >> >> to using "Xcode5ExceptionHandlerAsm.nasm". >> >> (If you wish, you can squash these modifications into the updated >> patch#1, rather than inserting them as a separate patch between #1 and >> #2.) >> >> >> In summary, I suggest the following end-state: >> >> - we should have a self-patching NASM file, and one without >> self-patching, >> >> - the self-patching variant should be called >> "Xcode5ExceptionHandlerAsm.nasm" (because the *only* reason for the >> self-patching is xcode5), >> >> - we should have 5 INF files in total, >> >> - "PeiCpuExceptionHandlerLib.inf", "DxeCpuExceptionHandlerLib.inf", >> "SmmCpuExceptionHandlerLib.inf" should use >> "Xcode5ExceptionHandlerAsm.nasm" (the self-patching is harmless), >> >> - "SecPeiCpuExceptionHandlerLib.inf" should use >> "ExceptionHandlerAsm.nasm" (self-patching is invalid, so don't do it), >> >> - "Xcode5SecPeiCpuExceptionHandlerLib.inf" should use >> "Xcode5ExceptionHandlerAsm.nasm" (the self-patching is invalid, but we >> can't avoid it when building with XCODE5), >> >> - platforms should resolve the CpuExceptionHandlerLib class to >> "Xcode5SecPeiCpuExceptionHandlerLib.inf" only for the XCODE5 toolchain >> *and* for the SEC phase. > > Ok, I have v2 ready to go, but when I ran it through the integration > tests using a pull request I received some errors (see > https://nam06.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F%2Fdev.= azure.com%2Ftianocore%2Fedk2-ci%2F_build%2Fresults%3FbuildId%3D6516%26view%= 3Dresults&data=3D02%7C01%7CBret.Barkelew%40microsoft.com%7Ca9365fbefc53= 477c9e3308d7f1db4560%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637243796= 372792580&sdata=3DN2aYxzfr%2BNw7hkhBTEg0C%2Fm4Hv00xXBZhSz3%2FFgiW1s%3D&= amp;reserved=3D0). > The error is the same in all cases and the error message is: > > CRITICAL - > UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandle= rLib.inf > not in UefiCpuPkg/UefiCpuPkg.dsc > > Any idea about the reason for this message? The reason is that the CI code found a library instance (INF) in UefiCpuPkg that cannot be built *stand-alone* (i.e. without being consumed by a different UEFI module / INF file). In core packages, library instances should be buildable stand-alone with the "-m" build flag, and for that, the lib instances need to be listed in the [Components] section. > Is it due to the > [Components] section of the UefiCpuPkg/UefiCpuPkg.dsc file? Yes. > Should that > section not use the !if check and just list both .inf files > (SecPeiCpuExceptionHandlerLib.inf and > Xcode5SecPeiCpuExceptionHandlerLib.inf)? Hmmm, this is a very good point; after all, the updated (=3Dreverted) "SecPeiCpuExceptionHandlerLib.inf" instance will not build with XCODE5. Therefore we should list both lib instance INF files under [Components], but make "SecPeiCpuExceptionHandlerLib.inf" conditional on non-XCODE5. I wonder how the CI logic will cope with this! Thanks, Laszlo --_000_CY4PR21MB0743420B913B1BC1747EC18BEFA40CY4PR21MB0743namp_ Content-Type: text/html; charset="Windows-1252" Content-Transfer-Encoding: quoted-printable

<Quote>

> Should that
> section not use the !if check and just list both .inf files
> (SecPeiCpuExceptionHandlerLib.inf and
> Xcode5SecPeiCpuExceptionHandlerLib.inf)?

Hmmm, this is a very good point; after all, the updated (=3Dreverted)
"SecPeiCpuExceptionHandlerLib.inf" instance will not build with = XCODE5.
Therefore we should list both lib instance INF files under [Components], but make "SecPeiCpuExceptionHandlerLib.inf" conditional on non-X= CODE5.

</Quote>

 

Xcode5SecPeiCpuExceptionHandlerLib.inf could be add= ed to the ignore list in the CI yaml file. PR gating CI currently only uses= GCC and VS2017/19 and shouldn=92t have a problem with the reverted lib. Th= is makes Xcode5SecPeiCpuExceptionHandlerLib the exception which can be documented in the ignore list (why it=92s bein= g ignored).

 

Thoughts?

 

- Bret

 

From: Laszlo Ersek via groups.io=
Sent: Wednesday, May 6, 2020 9:33 AM
To: Tom Lendacky; devel@edk2.groups.io
Cc: Jordan Justen;= Ard Biesheuvel; Liming Gao= ; Eric Dong; Ray Ni; Brijesh Singh; Anthony Pe= rard; Benjamin You; Guo Dong; Julien Grall; Maurice Ma; Andrew Fish
Subject: [EXTERNAL] Re: [edk2-devel] [PATCH 4/4] UefiCpuPkg/CpuExce= ptionHandler: Revert binary patching in standard CpuExceptionHandlerLib

 

On 05/06/20 16:35, T= om Lendacky wrote:
> On 5/5/20 5:15 PM, Laszlo Ersek via groups.io wrote:
>> On 05/01/20 22:17, Lendacky, Thomas wrote:
>>> BZ:
>>> https://nam06.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F%2Fbugzil= la.tianocore.org%2Fshow_bug.cgi%3Fid%3D2340&amp;data=3D02%7C01%7CBret.B= arkelew%40microsoft.com%7Ca9365fbefc53477c9e3308d7f1db4560%7C72f988bf86f141= af91ab2d7cd011db47%7C1%7C0%7C637243796372782584&amp;sdata=3DR%2B5IgncDz= x7viv3yIwX3MloX1QlzuUJ4bZlKnc%2B0xoI%3D&amp;reserved=3D0
>>>
>>>
>>> Now that an XCODE5 specific CpuExceptionHandlerLib library is= in place,
>>> revert the changes made to the ExceptionHandlerAsm.nasm in co= mmit
>>> 2db0ccc2d7fe ("UefiCpuPkg: Update CpuExceptionHandlerLib= pass XCODE5
>>> tool
>>> chain") so that binary patching of flash code is not per= formed.
>>>
>>> Cc: Eric Dong <eric.dong@intel.com>
>>> Cc: Ray Ni <ray.ni@intel.com>
>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>> Cc: Liming Gao <liming.gao@intel.com>
>>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com> >>> ---
>>>   .../X64/ExceptionHandlerAsm.nasm   &nbs= p;          | 25 ++= 3;++--------------
>>>   1 file changed, 6 insertions(+), 19 deletions(-) >>>
>>> diff --git
>>> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHand= lerAsm.nasm
>>> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHand= lerAsm.nasm
>>> index 19198f273137..3814f9de3703 100644
>>> ---
>>> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHand= lerAsm.nasm
>>> +++
>>> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHand= lerAsm.nasm
>>> @@ -34,7 +34,7 @@ AsmIdtVectorBegin:
>>>       db    &nbs= p; 0x6a        ; push  #VectorNum >>>       db    &nbs= p; ($ - AsmIdtVectorBegin) / ((AsmIdtVectorEnd -
>>> AsmIdtVectorBegin) / 32) ; VectorNum
>>>       push    rax
>>> -    mov     rax, strict q= word 0 ;    mov     rax,
>>> ASM_PFX(CommonInterruptEntry)
>>> +    mov     rax, ASM_= PFX(CommonInterruptEntry)
>>>       jmp     ra= x
>>>   %endrep
>>>   AsmIdtVectorEnd:
>>> @@ -44,8 +44,7 @@ HookAfterStubHeaderBegin:
>>>   @VectorNum:
>>>       db    &nbs= p; 0          ; 0 will be fixe= d
>>>       push    rax
>>> -    mov     rax, strict q= word 0 ;     mov     rax,
>>> HookAfterStubHeaderEnd
>>> -JmpAbsoluteAddress:
>>> +    mov     rax, Hook= AfterStubHeaderEnd
>>>       jmp     ra= x
>>>   HookAfterStubHeaderEnd:
>>>       mov     ra= x, rsp
>>> @@ -257,7 +256,8 @@ HasErrorCode:
>>>       ; and make sure RSP is 16-byte= aligned
>>>       ;
>>>       sub     rs= p, 4 * 8 + 8
>>> -    call    ASM_PFX(CommonExce= ptionHandler)
>>> +    mov     rax, ASM_= PFX(CommonExceptionHandler)
>>> +    call    rax
>>>       add     rs= p, 4 * 8 + 8
>>>
>>>       cli
>>> @@ -365,24 +365,11 @@ DoIret:
>>>   ; comments here for definition of address map
>>>   global ASM_PFX(AsmGetTemplateAddressMap)
>>>   ASM_PFX(AsmGetTemplateAddressMap):
>>> -    lea     rax, [AsmIdtV= ectorBegin]
>>> +    mov     rax, AsmI= dtVectorBegin
>>>       mov     qw= ord [rcx], rax
>>>       mov     qw= ord [rcx + 0x8],  (AsmIdtVectorEnd -
>>> AsmIdtVectorBegin) / 32
>>> -    lea     rax, [HookAft= erStubHeaderBegin]
>>> +    mov     rax, Hook= AfterStubHeaderBegin
>>>       mov     qw= ord [rcx + 0x10], rax
>>> -
>>> -; Fix up CommonInterruptEntry address
>>> -    lea    rax, [ASM_PFX(Commo= nInterruptEntry)]
>>> -    lea    rcx, [AsmIdtVectorB= egin]
>>> -%rep  32
>>> -    mov    qword [rcx + (J= mpAbsoluteAddress - 8 -
>>> HookAfterStubHeaderBegin)], rax
>>> -    add    rcx, (AsmIdtVectorE= nd - AsmIdtVectorBegin) / 32
>>> -%endrep
>>> -; Fix up HookAfterStubHeaderEnd
>>> -    lea    rax, [HookAfterStub= HeaderEnd]
>>> -    lea    rcx, [JmpAbsoluteAd= dress]
>>> -    mov    qword [rcx - 8], ra= x
>>> -
>>>       ret
>>>
>>>  
>>> ;------------------------------------------------------------= -------------------------
>>>
>>>
>>
>> With this patch applied, the differences with the "original&= quot; remain:
>>
>> $ git diff 2db0ccc2d7fe^..HEAD -- \
>>       
>> UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm= .nasm
>>
>>> diff --git
>>> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHand= lerAsm.nasm
>>> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHand= lerAsm.nasm
>>> index ba8993d84b0b..3814f9de3703 100644
>>> ---
>>> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHand= lerAsm.nasm
>>> +++
>>> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHand= lerAsm.nasm
>>> @@ -1,12 +1,6 @@
>>>  
>>> ;------------------------------------------------------------= ------------------
>>> ;
>>> -; Copyright (c) 2012 - 2014, Intel Corporation. All rights >>> reserved.<BR>
>>> -; This program and the accompanying materials
>>> -; are licensed and made available under the terms and condit= ions of
>>> the BSD License
>>> -; which accompanies this distribution.  The full text o= f the license
>>> may be found at
>>> -;
>>> https://nam06.safelinks.protection.outlook.com/?url=3Dhttp%3A%2F%2Fopensou= rce.org%2Flicenses%2Fbsd-license.php&amp;data=3D02%7C01%7CBret.Barkelew= %40microsoft.com%7Ca9365fbefc53477c9e3308d7f1db4560%7C72f988bf86f141af91ab2= d7cd011db47%7C1%7C0%7C637243796372792580&amp;sdata=3DCZovXRJ6HURgo6sz%2= FDi55SUy9IsrJ2pFzcHX%2Bp%2Fv8qA%3D&amp;reserved=3D0.
>>>
>>> -;
>>> -; THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN &qu= ot;AS IS" BASIS,
>>> -; WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER = EXPRESS
>>> OR IMPLIED.
>>> +; Copyright (c) 2012 - 2018, Intel Corporation. All righ= ts
>>> reserved.<BR>
>>> +; SPDX-License-Identifier: BSD-2-Clause-Patent
>>>   ;
>>>   ; Module Name:
>>>   ;
>>
>> This is expected.
>>
>>> @@ -189,17 +183,19 @@ HasErrorCode:
>>>       push    rax
>>>       push    rax
>>>       sidt    [rsp] >>> -    xchg    rax, [rsp + 2]=
>>> -    xchg    rax, [rsp]
>>> -    xchg    rax, [rsp + 8]=
>>> +    mov     bx, word = [rsp]
>>> +    mov     rax, qwor= d [rsp + 2]
>>> +    mov     qword [rs= p], rax
>>> +    mov     word [rsp= + 8], bx
>>>
>>>       xor     ra= x, rax
>>>       push    rax
>>>       push    rax
>>>       sgdt    [rsp] >>> -    xchg    rax, [rsp + 2]=
>>> -    xchg    rax, [rsp]
>>> -    xchg    rax, [rsp + 8]=
>>> +    mov     bx, word = [rsp]
>>> +    mov     rax, qwor= d [rsp + 2]
>>> +    mov     qword [rs= p], rax
>>> +    mov     word [rsp= + 8], bx
>>>
>>>   ;; UINT64  Ldtr, Tr;
>>>       xor     ra= x, rax
>>>
>>
>> Also expected, from commit f4c898f2b2db
>> ("UefiCpuPkg/CpuExceptionHandlerLib: Fix split lock", 2= 019-09-20).
>>
>> Therefore, for this patch:
>>
>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>
>> *However*, this revert must be restricted to the original
>> "SecPeiCpuExceptionHandlerLib.inf" instance, i.e. where= binary patching
>> is not acceptable. (Otherwise, in combination with my request (1)= under
>> patch#1, we'd needlessly break the PEI / DXE / SMM lib instances = under
>> XCODE5.)
>>
>> (1) Therefore, please insert a new patch between patches #1 and #= 2, such
>> that the new patch flip
>>
>> - PeiCpuExceptionHandlerLib.inf
>> - DxeCpuExceptionHandlerLib.inf
>> - SmmCpuExceptionHandlerLib.inf
>>
>> to using "Xcode5ExceptionHandlerAsm.nasm".
>>
>> (If you wish, you can squash these modifications into the updated=
>> patch#1, rather than inserting them as a separate patch between #= 1 and
>> #2.)
>>
>>
>> In summary, I suggest the following end-state:
>>
>> - we should have a self-patching NASM file, and one without
>> self-patching,
>>
>> - the self-patching variant should be called
>> "Xcode5ExceptionHandlerAsm.nasm" (because the *only* re= ason for the
>> self-patching is xcode5),
>>
>> - we should have 5 INF files in total,
>>
>> - "PeiCpuExceptionHandlerLib.inf", "DxeCpuExceptio= nHandlerLib.inf",
>> "SmmCpuExceptionHandlerLib.inf" should use
>> "Xcode5ExceptionHandlerAsm.nasm" (the self-patching is = harmless),
>>
>> - "SecPeiCpuExceptionHandlerLib.inf" should use
>> "ExceptionHandlerAsm.nasm" (self-patching is invalid, s= o don't do it),
>>
>> - "Xcode5SecPeiCpuExceptionHandlerLib.inf" should use >> "Xcode5ExceptionHandlerAsm.nasm" (the self-patching is = invalid, but we
>> can't avoid it when building with XCODE5),
>>
>> - platforms should resolve the CpuExceptionHandlerLib class to >> "Xcode5SecPeiCpuExceptionHandlerLib.inf" only for the X= CODE5 toolchain
>> *and* for the SEC phase.
>
> Ok, I have v2 ready to go, but when I ran it through the integration<= br> > tests using a pull request I received some errors (see
> https://nam06.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F%2Fdev.az= ure.com%2Ftianocore%2Fedk2-ci%2F_build%2Fresults%3FbuildId%3D6516%26view%3D= results&amp;data=3D02%7C01%7CBret.Barkelew%40microsoft.com%7Ca9365fbefc= 53477c9e3308d7f1db4560%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C6372437= 96372792580&amp;sdata=3DN2aYxzfr%2BNw7hkhBTEg0C%2Fm4Hv00xXBZhSz3%2FFgiW= 1s%3D&amp;reserved=3D0).
> The error is the same in all cases and the error message is:
>
> CRITICAL -
> UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHan= dlerLib.inf
> not in UefiCpuPkg/UefiCpuPkg.dsc
>
> Any idea about the reason for this message?

The reason is that the CI code found a library instance (INF) in
UefiCpuPkg that cannot be built *stand-alone* (i.e. without being
consumed by a different UEFI module / INF file).

In core packages, library instances should be buildable stand-alone with the "-m" build flag, and for that, the lib instances need to be = listed
in the [Components] section.

> Is it due to the
> [Components] section of the UefiCpuPkg/UefiCpuPkg.dsc file?

Yes.

> Should that
> section not use the !if check and just list both .inf files
> (SecPeiCpuExceptionHandlerLib.inf and
> Xcode5SecPeiCpuExceptionHandlerLib.inf)?

Hmmm, this is a very good point; after all, the updated (=3Dreverted)
"SecPeiCpuExceptionHandlerLib.inf" instance will not build with = XCODE5.
Therefore we should list both lib instance INF files under [Components], but make "SecPeiCpuExceptionHandlerLib.inf" conditional on non-X= CODE5.

I wonder how the CI logic will cope with this!

Thanks,
Laszlo


 

--_000_CY4PR21MB0743420B913B1BC1747EC18BEFA40CY4PR21MB0743namp_--