From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from NAM11-BN8-obe.outbound.protection.outlook.com (NAM11-BN8-obe.outbound.protection.outlook.com [40.107.236.84]) by mx.groups.io with SMTP id smtpd.web11.7835.1588716563399294083 for ; Tue, 05 May 2020 15:09:23 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@amdcloud.onmicrosoft.com header.s=selector2-amdcloud-onmicrosoft-com header.b=hITHBUN1; spf=none, err=SPF record not found (domain: amd.com, ip: 40.107.236.84, mailfrom: thomas.lendacky@amd.com) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=YcerNSda4QRFEMEkfgJrjVpZCVLYRtlgK9NTZ25AMiqeTIW/MdV1ELGAI8US3PLoASbTXOPmT93+bUXBxo+LFsL7Lw+HGYzCEjGMT+eLsxLMfZysi4Gmsa1yDYHyrV0BARqEvVKtYFCzlH6tHlBAy+pY+BQ4d/4NE5TbDAWZnrFCTfF5ULdgaVPjjX6FJEPIS7t+kxmacEQ0ZiDXla4ilfdCvBSzGWitFDzu+N5RHsImH6AabjySJFH90EFgVk5oG3ZOJackGw0y1oVagiL2LzPGlRLgT1vv8pXIQoLLyJZgnPWnH0kCWB0fN9dwpHAITL9rRwj+oQXADlS7X2df+Q== 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=MTaCRVs29xE44/xMOALyhJv/DwooBULDtDAsWGY8Lpo=; b=Vi5JL8PrV2zxRQZ4oryYWSmJb/cK94yLxbYzpTTW/tKb7AA8jPMQXoLApVPQgBsmKs0QJQ0GOnYP0q6bhyvB8Mr7tcXUmMTY6l+GPn7MIa+XNzxJIp/NNzUDHM6EgbUH535NwKQSGU509hmVeEu/xnT3FYMezqUSVQfZrTCzQzX/7NWd4hjbz4OJM+xDBXCGX3+a7WsTG2LE1tsFO2VkxCMMhWC0rL1N/1BONN2ZJ4oFc8PsKcI3Rjo3emc9dFPGrchm+2GDEwoxZQ72xdAy4lOl/B2tZJyZdi0ndiZcip2HY/W3aKtvXITi6sCrIvZxhh8/9EKZXAa5JNwOtbdK+w== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=amd.com; dmarc=pass action=none header.from=amd.com; dkim=pass header.d=amd.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=amdcloud.onmicrosoft.com; s=selector2-amdcloud-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=MTaCRVs29xE44/xMOALyhJv/DwooBULDtDAsWGY8Lpo=; b=hITHBUN10XgLvWvEhtEtuNaIZFWLGdmU3e4kWgJvI7dCug8y07ykO9pP5+ZFZOpWwpNZYYkK30bcWn59jDmYbK58ulODsLlVEPRMyLRpS9hg1wiyfVM/ye2pWs69OeZVmpzGH0t409v86yqUd2ezta+yU1QD9zXlwrFEj/6dHf4= Authentication-Results: apple.com; dkim=none (message not signed) header.d=none;apple.com; dmarc=none action=none header.from=amd.com; Received: from DM5PR12MB1355.namprd12.prod.outlook.com (2603:10b6:3:6e::7) by DM5PR12MB1324.namprd12.prod.outlook.com (2603:10b6:3:76::11) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2958.20; Tue, 5 May 2020 22:09:20 +0000 Received: from DM5PR12MB1355.namprd12.prod.outlook.com ([fe80::4ce1:9947:9681:c8b1]) by DM5PR12MB1355.namprd12.prod.outlook.com ([fe80::4ce1:9947:9681:c8b1%10]) with mapi id 15.20.2958.030; Tue, 5 May 2020 22:09:20 +0000 Subject: Re: [edk2-devel] [PATCH 1/4] UefiCpuPkg/CpuExceptionHandler: Make XCODE5 changes toolchain specific To: Laszlo Ersek , 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 References: <7517ff11143e7a5d81dd2c9f450dce3ffa195b24.1588364261.git.thomas.lendacky@amd.com> <862a2551-7f07-a4e8-52d6-dccb1db01fcb@redhat.com> From: "Lendacky, Thomas" Message-ID: Date: Tue, 5 May 2020 17:09:17 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 In-Reply-To: <862a2551-7f07-a4e8-52d6-dccb1db01fcb@redhat.com> X-ClientProxiedBy: SN4PR0501CA0142.namprd05.prod.outlook.com (2603:10b6:803:2c::20) To DM5PR12MB1355.namprd12.prod.outlook.com (2603:10b6:3:6e::7) Return-Path: thomas.lendacky@amd.com MIME-Version: 1.0 X-MS-Exchange-MessageSentRepresentingType: 1 Received: from office-linux.texastahm.com (67.79.209.213) by SN4PR0501CA0142.namprd05.prod.outlook.com (2603:10b6:803:2c::20) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2979.11 via Frontend Transport; Tue, 5 May 2020 22:09:18 +0000 X-Originating-IP: [67.79.209.213] X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-HT: Tenant X-MS-Office365-Filtering-Correlation-Id: 1fe1dfde-cb7a-4886-8858-08d7f140f582 X-MS-TrafficTypeDiagnostic: DM5PR12MB1324:|DM5PR12MB1324: X-MS-Exchange-Transport-Forked: True X-Microsoft-Antispam-PRVS: X-MS-Oob-TLC-OOBClassifiers: OLM:7219; X-Forefront-PRVS: 0394259C80 X-MS-Exchange-SenderADCheck: 1 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: N2vc7ZfhUxItLdSWgKFxPzVVgakHmX5L/4NS9w1/SOmd/UTu3yD6jnnOvBG2Nos0y66y1DklNk1VKQE7ewzlsm7tUksHs6scLQ0gCEzysDOcoX6+08uoU2XRxXRZSAZy0Fnk57n2Ov9kej+MJZdqf0CYqIirYsTccWjS7IkptVZwQj0qch4rav5cNFFpUW4pT7W9hiQS1MJp/pMIU9doIo4DbVGyMhw7TRc3dzW66psdbv5TU2nbukam9JbC3D0PRw/iViHoXVyp9ol36ipBFiHye6Cpxhkd4K5s8uPbPCIT83xcBQG8XgduWTo3NBbXO4anu4eUl7I3pcEkr8GWsnC3RBzIyEI6bdWbYNWs4zTtAp5zWiGjqHp9S5upc9fl4KiOIyLEne33bnaV/zm2aZolDJByffQ9I9JWMYi8MTyzWyWLQBfPE1lFYigl4nc4jeN/y6+DEl+uneD9wjRvpQCh5W0iQwCMKx5X9ZP9eCFyvVw4FMhGl387bRI1AkX73dPIH0SP8tLVk9KXbI06o+8QGKs5F93NT2beey9w6oamogz7rLCpKSzY3DWgHIzHe6WxF582vZBYBLnQCYaUV7YrMF9Oic4i6MedDBnDAjfKQAywnYTNd1gofauIcIYuecp6oZroLnNuFvwj1iexbQ== X-Forefront-Antispam-Report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:DM5PR12MB1355.namprd12.prod.outlook.com;PTR:;CAT:NONE;SFTY:;SFS:(4636009)(396003)(39860400002)(366004)(136003)(346002)(376002)(33430700001)(4326008)(36756003)(66556008)(31696002)(66476007)(66946007)(6486002)(54906003)(86362001)(6512007)(8676002)(5660300002)(8936002)(316002)(19627235002)(16526019)(26005)(52116002)(956004)(2616005)(186003)(53546011)(6506007)(7416002)(478600001)(31686004)(2906002)(966005)(33440700001)(45080400002)(43740500002);DIR:OUT;SFP:1101; X-MS-Exchange-AntiSpam-MessageData: u0UqP5GWg3ao1YSrSHsx2HQa22U8lZ1DkDAUAI4/NvEBLd0vTBTuGEd0KGbkcw2RjdARFq+ds3qpHvOCxozhIHaWJNnUHHgE5uqDCzps7Cwyp4VWR0VvKv5dk6LS6qRM7QO33sja3FJZfCCK7y1Ud4WFstAmWQ5Vw6p5PcpHY9xwPfjTx8YwUFuWo0NyKpLm5K8NoY5js3Vx5FCHf9RTQx3lbi1qT1dvRAegpFDHFb3ko81/d4AClGJhE8q5X/UsL6emPb4DwPoR1I+iQb8S6Xj5eaNOLhfq4gIyOUbf/MsZLf4IF5a0ObM/DEl3R1/L7XmOuPOJr/aVnN1oL6nqrVaREsSq7ZjM+R6nEeMWYLjgpNDnDCcDUuIBa80aJ5QSjVUO/mczUNtfZkOf0OOK/9Jtit5n/gUPi5y0qoQ9pZBIIsna1/OA3o0MqSEmzeHnRLtGWyAy/OQV0HxPRcRgmiFGhby/Rzy1c+ngq0FVQmLbAFVyLg2QP6K0dWXMmbn+7O+I0W/S7gJj8Y/NR5w0fBbmC+kORl3/FrAGi+KVSXnAtLwYNdIuY6XyRPUELsMKEW+XBYEmYYIPTUGyc+NA5WYCWiIj+zinPyBci9Q1wBMEvVPf3OiU3EI9W3BFc3Xjv3N3HzMfGmpX6cemtd4NrdE3z+Yqih6gPXkMzb9oXrR+iHSXJwPd7YfNFv4DLPjal0gNDqPy3w7YF4IwytisqkqG2v1JVZmZm/22A5C9GkRdeUXSpGSB8NfDi/rXZZS1+8w/LSneEGnfACi0phv1XEmIZCr58rIpnfDp8vB6ufw= X-OriginatorOrg: amd.com X-MS-Exchange-CrossTenant-Network-Message-Id: 1fe1dfde-cb7a-4886-8858-08d7f140f582 X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 May 2020 22:09:19.9116 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d X-MS-Exchange-CrossTenant-MailboxType: HOSTED X-MS-Exchange-CrossTenant-UserPrincipalName: sLYOPYMlSoAHqOhKgY2WpnG7qwcv7VlXdP7oritmx/0HlGUwbShWZV+yShxoI14D4hWU2sfYcfteouZZ1dhbIg== X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM5PR12MB1324 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit On 5/5/20 4:39 PM, Laszlo Ersek wrote: > Hi Tom, > > On 05/01/20 22:17, Lendacky, Thomas wrote: >> BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2340&data=02%7C01%7Cthomas.lendacky%40amd.com%7C4d398c73a4bc4674d36608d7f13cce1d%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637243115777198329&sdata=YA9qmWhZDrwGCchGXKDmOQPFqXdDztokQSZeZIq6u18%3D&reserved=0 >> >> Commit 2db0ccc2d7fe ("UefiCpuPkg: Update CpuExceptionHandlerLib pass >> XCODE5 tool chain") introduced binary patching into the exception handling >> support. CPU exception handling is allowed during SEC and this results in >> binary patching of flash, which should not be done. >> >> Separate the changes from commit 2db0ccc2d7fe into an XCODE5 toolchain >> specific file, Xcode5ExceptionHandlerAsm.nasm, and create new INF files >> for an XCODE5 version of CpuExceptionHandlerLib. Update the UefiCpuPkg.dsc >> file to use the new files when the XCODE5 toolchain is used. >> >> Cc: Eric Dong >> Cc: Ray Ni >> Cc: Laszlo Ersek >> Cc: Liming Gao >> Signed-off-by: Tom Lendacky >> --- >> UefiCpuPkg/UefiCpuPkg.dsc | 23 + >> .../Xcode5DxeCpuExceptionHandlerLib.inf | 64 +++ >> .../Xcode5PeiCpuExceptionHandlerLib.inf | 63 +++ >> .../Xcode5SecPeiCpuExceptionHandlerLib.inf | 55 +++ >> .../Xcode5SmmCpuExceptionHandlerLib.inf | 59 +++ > > I don't think that paralleling all the existent INF files is necessary > for XCODE5. > > The binary patching is a problem when the UEFI module containing the > self-patching CpuExceptionHandlerLib instance executes in-place from > flash. That applies to: (a) SEC modules, (b) PEI modules that do *not* > have a DEPEX on "gEfiPeiMemoryDiscoveredPpiGuid". (PEIMs that do have a > DEPEX on that PPI GUID are only dispatched after the permanent PEI RAM > has been discovered / published, so they run out of normal RAM.) > > Reviewing the existent INF files, we have: > > - DxeCpuExceptionHandlerLib.inf: for DXE_CORE, DXE_DRIVER, > UEFI_APPLICATION modules. Self-patching is fine. > > - SmmCpuExceptionHandlerLib.inf: for DXE_SMM_DRIVER modules. > Self-patching is fine. > > - SecPeiCpuExceptionHandlerLib.inf: SEC is listed explicitly, so here we > certainly need an alternative. > > - PeiCpuExceptionHandlerLib.inf: unfortunately, the differences of this > library instance with "SecPeiCpuExceptionHandlerLib.inf" is not obvious; > only SEC's absence is easily visible. > > If we look at the commit that introduced this lib instance > (a81abf161666, "UefiCpuPkg/ExceptionLib: Import > PeiCpuExceptionHandlerLib module", 2016-06-01), we find: > >> This module could be linked by CpuMpPei driver to handle reserved vector list >> and provide spin lock for BSP/APs to prevent dump message corrupted. > > So the library was added explicitly for CpuMpPei's sake -- which looks > promising, because CpuMpPei certainly depends on > "gEfiPeiMemoryDiscoveredPpiGuid", as it needs a bunch of RAM for > offering the multi-processing PPI. That suggests the self-patching is OK > in "PeiCpuExceptionHandlerLib.inf" too. > > The CpuMpPei DEPEX in question was replaced with a PPI notify callback > in commit 0a0d5296e448 ("UefiCpuPkg/CpuMpPei: support stack guard > feature", 2018-09-10). This would be a problem if the self-patching in > the PeiCpuExceptionHandlerLib instance occurred in the library > constructor, because the CpuMpPei can now actually be dispatched before > permanent PEI RAM is available -- and the constructor would run > immediately. > > Luckily, the lib instance has no CONSTRUCTOR at all, and CpuMpPei calls > InitializeCpuExceptionHandlers() explicitly in InitializeCpuMpWorker(), > which is the PPI notify in question. (And per > , the > self-patching occurs in InitializeCpuExceptionHandlers().) > > Therefore, having two variants of PeiCpuExceptionHandlerLib.inf is also > unnecessary. > > (1) We only need two variants for "SecPeiCpuExceptionHandlerLib.inf", in > my opinion. Ok, I'll rework it to have variants for just SecPeiCpuExceptionHandlerLib. > > (Note: if we check OVMF, we see that PeiCpuExceptionHandlerLib.inf is > used universally for PEIMs. That's because OVMF is special -- its PEI > phase runs entirely out of RAM. See also commit f0e6a56a9a2f, "OvmfPkg: > include UefiCpuPkg/CpuMpPei", 2016-07-15.) > > --*-- > > With this patch applied: > > $ diff -u \ > SecPeiCpuExceptionHandlerLib.inf \ > Xcode5SecPeiCpuExceptionHandlerLib.inf > >> --- SecPeiCpuExceptionHandlerLib.inf 2020-05-05 18:36:12.813156743 +0200 >> +++ Xcode5SecPeiCpuExceptionHandlerLib.inf 2020-05-05 23:25:24.578572971 +0200 >> @@ -8,7 +8,7 @@ >> >> [Defines] >> INF_VERSION = 0x00010005 >> - BASE_NAME = SecPeiCpuExceptionHandlerLib >> + BASE_NAME = Xcode5SecPeiCpuExceptionHandlerLib > > OK > >> MODULE_UNI_FILE = SecPeiCpuExceptionHandlerLib.uni > > (2) We'll need a separate UNI file here -- also we should customize the > file-top comment in the INF file -- that explains the difference between > the XCODE5 and non-XCODE5 variants, briefly. Ok, will do. > >> FILE_GUID = CA4BBC99-DFC6-4234-B553-8B6586B7B113 > > (3) Please generate a new FILE_GUID with "uuidgen". Ok, thanks, that answers my question that I asked in another email. > >> MODULE_TYPE = PEIM >> @@ -26,16 +26,20 @@ >> Ia32/ExceptionTssEntryAsm.nasm >> Ia32/ArchExceptionHandler.c >> Ia32/ArchInterruptDefs.h >> + Ia32/ArchAMDSevVcHandler.c > > (4) Even though the blurb says that this series is based on edk2 commit > e54310451f1a, some SEV-ES specific parts remain in this patch, and > should be eliminated. The first example is above. Ugh. I thought I had that all cleaned up before sending. My bad, I'll fix that in the next version. Thanks, Tom > >> >> [Sources.X64] >> - X64/ExceptionHandlerAsm.nasm >> + X64/Xcode5ExceptionHandlerAsm.nasm >> X64/ArchExceptionHandler.c >> X64/ArchInterruptDefs.h >> + X64/ArchAMDSevVcHandler.c > > (5) Another SEV-ES change. > >> >> [Sources.common] >> CpuExceptionCommon.h >> CpuExceptionCommon.c >> SecPeiCpuException.c >> + AMDSevVcHandler.c >> + AMDSevVcCommon.h > > (6) ditto > >> >> [Packages] >> MdePkg/MdePkg.dec >> @@ -48,3 +52,4 @@ >> PrintLib >> LocalApicLib >> PeCoffGetEntryPointLib >> + VmgExitLib > > (7) ditto > > Furthermore: > > $ diff -u \ > ExceptionHandlerAsm.nasm \ > Xcode5ExceptionHandlerAsm.nasm > >> --- ExceptionHandlerAsm.nasm 2020-05-05 23:26:30.941784203 +0200 >> +++ Xcode5ExceptionHandlerAsm.nasm 2020-05-05 23:25:24.578572971 +0200 >> @@ -18,6 +18,8 @@ >> ; CommonExceptionHandler() >> ; >> >> +%define VC_EXCEPTION 29 >> + >> extern ASM_PFX(mErrorCodeFlag) ; Error code flags for exceptions >> extern ASM_PFX(mDoFarReturnFlag) ; Do far return flag >> extern ASM_PFX(CommonExceptionHandler) >> @@ -225,6 +227,9 @@ >> push rax >> >> ;; UINT64 Dr0, Dr1, Dr2, Dr3, Dr6, Dr7; >> + cmp qword [rbp + 8], VC_EXCEPTION >> + je VcDebugRegs ; For SEV-ES (#VC) Debug registers ignored >> + >> mov rax, dr7 >> push rax >> mov rax, dr6 >> @@ -237,7 +242,19 @@ >> push rax >> mov rax, dr0 >> push rax >> + jmp DrFinish >> + >> +VcDebugRegs: >> +;; UINT64 Dr0, Dr1, Dr2, Dr3, Dr6, Dr7 are skipped for #VC to avoid exception recursion >> + xor rax, rax >> + push rax >> + push rax >> + push rax >> + push rax >> + push rax >> + push rax >> >> +DrFinish: >> ;; FX_SAVE_STATE_X64 FxSaveState; >> sub rsp, 512 >> mov rdi, rsp > > (8) All of these should be removed -- they should be part of your SEV-ES > series, on top of this set. > > Thanks, > Laszlo >