From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [207.211.31.120]) by mx.groups.io with SMTP id smtpd.web11.7346.1588714777761732226 for ; Tue, 05 May 2020 14:39:38 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=YC8/V52Z; spf=pass (domain: redhat.com, ip: 207.211.31.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1588714776; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=XN62rCISgItcimXd7yNYcwdmvZl3LX+zov6I1eLdYbo=; b=YC8/V52Z/HhChy7qPhjNjKnLCOlt+QOqs0Yp8Mm0i/eSWwjq5uebM8KmdwMLQ26rIymap2 h2nUyMjW33vhkbfuPuC8WkLjO06m8q6UPKTWTkIT2dtbA+VLDAIQ+hQ1gf1fzF3obCMQKT X8UmT3/fJC4k9S5RO4TcI/rzaDWDz9M= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-231-0YlDb8P4MiKGBD2d5bmgNg-1; Tue, 05 May 2020 17:39:26 -0400 X-MC-Unique: 0YlDb8P4MiKGBD2d5bmgNg-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id C7E68872FE1; Tue, 5 May 2020 21:39:23 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-114-134.ams2.redhat.com [10.36.114.134]) by smtp.corp.redhat.com (Postfix) with ESMTP id 1184660BEC; Tue, 5 May 2020 21:39:19 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 1/4] UefiCpuPkg/CpuExceptionHandler: Make XCODE5 changes toolchain specific To: devel@edk2.groups.io, thomas.lendacky@amd.com 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> From: "Laszlo Ersek" Message-ID: <862a2551-7f07-a4e8-52d6-dccb1db01fcb@redhat.com> Date: Tue, 5 May 2020 23:39:19 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <7517ff11143e7a5d81dd2c9f450dce3ffa195b24.1588364261.git.thomas.lendacky@amd.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Hi Tom, On 05/01/20 22:17, Lendacky, Thomas wrote: > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2340 > > 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. (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. > FILE_GUID = CA4BBC99-DFC6-4234-B553-8B6586B7B113 (3) Please generate a new FILE_GUID with "uuidgen". > 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. > > [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