From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by mx.groups.io with SMTP id smtpd.web12.7490.1570784173810347205 for ; Fri, 11 Oct 2019 01:56:13 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 2947F3086228; Fri, 11 Oct 2019 08:56:13 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-177.rdu2.redhat.com [10.10.120.177]) by smtp.corp.redhat.com (Postfix) with ESMTP id 333C860920; Fri, 11 Oct 2019 08:56:07 +0000 (UTC) Subject: Re: [edk2-devel] [RFC PATCH v2 38/44] UefiCpuPkg: Allow AP booting under SEV-ES To: "Lendacky, Thomas" , "devel@edk2.groups.io" Cc: Jordan Justen , Ard Biesheuvel , Michael D Kinney , Liming Gao , Eric Dong , Ray Ni , "Singh, Brijesh" , =?UTF-8?Q?Philippe_Mathieu-Daud=c3=a9?= References: <81e310d1f2929f839cd166d1c7de6694220743b6.1568922729.git.thomas.lendacky@amd.com> <284e15f0-25ee-bb69-dcd1-09e146346c69@redhat.com> <8a8f839a-9e50-29da-06f7-50e3fc3b93c1@redhat.com> <851cc695-8902-3b07-4867-a101e0f9ee4f@amd.com> <8eb55d97-0ba3-c217-a160-c24730b9f036@amd.com> From: "Laszlo Ersek" Message-ID: Date: Fri, 11 Oct 2019 10:56:06 +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: <8eb55d97-0ba3-c217-a160-c24730b9f036@amd.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.42]); Fri, 11 Oct 2019 08:56:13 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 10/11/19 01:17, Lendacky, Thomas wrote: > On 10/3/19 10:12 AM, Tom Lendacky wrote: >> >> >> On 10/3/19 5:32 AM, Laszlo Ersek wrote: >>> On 10/03/19 12:12, Laszlo Ersek wrote: >>> >>>> UINT32 ApEntryPoint; >>>> EFI_GUID SevEsFooterGuid; >>>> UINT16 Size; >>> >>> It's probably better to reverse the order of "Size" and >>> "SevEsFooterGuid", like this: >>> >>> UINT32 ApEntryPoint; >>> UINT16 Size; >>> EFI_GUID SevEsFooterGuid; >>> >>> because then even the "Size" field can be changed (or resized), as a >>> function of the footer GUID. >> >> Cool, I'll look into doing this and see how it works out. > > Just an update on this idea. This has worked out well, but has a couple of > caveats. Removing the Qemu change to make the flash mapped read-only in > the nested page tables, caused the following: > > 1. QemuFlashDetected() will attempt to detect how the flash memory device > behaves. Because it is marked as read-only by the hypervisor, writing > to the area results in a #NPF for the write-fault. With SEV-ES, > emulation of the instruction can't be performed (can't read guest > memory and not provided the faulting instruction bytes), so the vCPU is > just restarted. This results in an infinite #NPF occurring. > > The solution here was to check for SEV-ES being enabled and just return > false from QemuFlashDetected(). Any downfalls to doing that? Short-circuiting QemuFlashDetected() on SEV-ES seems appropriate. However, I don't understand why you return FALSE in that case. You should return TRUE. If QemuFlashDetected() returns FALSE, then the UEFI variable store will not be backed by the real pflash chip, it will be emulated with an \NvVars file on the EFI system partition. That emulation should really not be used nowadays. So IMO the right approach here is: - declare that SEV-ES only targets the "two pflash chips" setup - return TRUE from QemuFlashDetected() when SEV-ES is on. > > 2. Commit 2db0ccc2d7fe ("UefiCpuPkg: Update CpuExceptionHandlerLib pass > XCODE5 tool chain") causes a similar situation to #1. It attempts to do > some address fixups and write to the flash device. That's... stunning. Commit 2db0ccc2d7fe changes the file UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm such that it does in-place binary patching. This source file is referenced from: UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.inf as well. Note "SecPei". That makes the commit buggy, to my eyes, regardless of SEV-ES. Because: The binary patching appears to occur in the SEC phase as well, i.e. at a time when the exception handler is located in flash. That's incorrect on physical hardware too. Upon re-reading , this commit worked around an XCODE toolchain bug. Unfortunately, the workaround is not suitable for the SEC phase. (Also not suitable for the PEI phase, for such PEIMs that still execute from flash.) Please open a new bug for UefiCpuPkg in the TianoCore Bugzilla, reference BZ#849 in the See Also field, and please also make the new bug block BZ#2198. (I'll comment on this issue in a different thread too; I'll CC you on it.) > Reverting that commit fixes the issue. I don't think that will be an > acceptable solution, though, so need to think about what to do here. > > After those two changes, the above method works well. I'm happy to hear! Thanks, Laszlo