From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from nwk-aaemail-lapp03.apple.com (nwk-aaemail-lapp03.apple.com [17.151.62.68]) by mx.groups.io with SMTP id smtpd.web10.2455.1570862646869258861 for ; Fri, 11 Oct 2019 23:44:07 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@apple.com header.s=20180706 header.b=P58SYErL; spf=pass (domain: apple.com, ip: 17.151.62.68, mailfrom: afish@apple.com) Received: from pps.filterd (nwk-aaemail-lapp03.apple.com [127.0.0.1]) by nwk-aaemail-lapp03.apple.com (8.16.0.27/8.16.0.27) with SMTP id x9C6gWlD006134; Fri, 11 Oct 2019 23:44:03 -0700 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=apple.com; h=sender : from : message-id : content-type : mime-version : subject : date : in-reply-to : cc : to : references; s=20180706; bh=GaTs3HP48htHqNp/2lAYrlvXOtj9fpGcQO67UaUPkg8=; b=P58SYErLn4GZDNRlSAHxDT5+dbNqyUwtpdeMSWCQgXIYE30xEe8Dvf4mBZljnM3qKbnn LQxmM2/MxL76I+6RRvP3Rdk7igpMnUjlVo4UCZOj0S1QYJ+ig9S4b82B/fwrBQusizd1 N91/yK5F4+o3UF4ozRNvL/M7lbbPEAmB8DnnXToQuVlfXy4Us17jx6Z3QVkuHMqlm2vL Jv+ybaYm4oklpM97btArsjd7mnwcogj/kyiZnqVAUWwI7aRbDHnmYeWBf5CLmw5rgtTq vNyauFG0+ar2Psp4/ZYn5WDCMSZ2FipTOIRdzcwku6YiGxgOOubEhJTRxatQqm6gUJDD DQ== Received: from ma1-mtap-s01.corp.apple.com (ma1-mtap-s01.corp.apple.com [17.40.76.5]) by nwk-aaemail-lapp03.apple.com with ESMTP id 2vfbdnvap2-12 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=NO); Fri, 11 Oct 2019 23:44:03 -0700 Received: from nwk-mmpp-sz09.apple.com (nwk-mmpp-sz09.apple.com [17.128.115.80]) by ma1-mtap-s01.corp.apple.com (Oracle Communications Messaging Server 8.0.2.4.20190507 64bit (built May 7 2019)) with ESMTPS id <0PZ900LQ91DD0M20@ma1-mtap-s01.corp.apple.com>; Fri, 11 Oct 2019 23:44:02 -0700 (PDT) Received: from process_milters-daemon.nwk-mmpp-sz09.apple.com by nwk-mmpp-sz09.apple.com (Oracle Communications Messaging Server 8.0.2.4.20190507 64bit (built May 7 2019)) id <0PZ900H001AJR800@nwk-mmpp-sz09.apple.com>; Fri, 11 Oct 2019 23:44:01 -0700 (PDT) X-Va-A: X-Va-T-CD: 08777febe38bb384cc57fda39d0586b7 X-Va-E-CD: 17a6de702c0d9dcaf89f320d435a4bc6 X-Va-R-CD: 4287d1648b5c1ab4e136057a2bee17a6 X-Va-CD: 0 X-Va-ID: 0798865a-47c1-4f80-b4a9-11fd48772deb X-V-A: X-V-T-CD: 08777febe38bb384cc57fda39d0586b7 X-V-E-CD: 17a6de702c0d9dcaf89f320d435a4bc6 X-V-R-CD: 4287d1648b5c1ab4e136057a2bee17a6 X-V-CD: 0 X-V-ID: 0e4e6d08-a1ba-473e-b3d3-5cf862478a93 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2019-10-12_03:,, signatures=0 Received: from [17.235.27.235] (unknown [17.235.27.235]) by nwk-mmpp-sz09.apple.com (Oracle Communications Messaging Server 8.0.2.4.20190507 64bit (built May 7 2019)) with ESMTPSA id <0PZ900DHP1BK2620@nwk-mmpp-sz09.apple.com>; Fri, 11 Oct 2019 23:42:58 -0700 (PDT) Sender: afish@apple.com From: "Andrew Fish" Message-id: <635C113A-D0A2-4A11-AF9A-8A65BBE1C9BC@apple.com> MIME-version: 1.0 (Mac OS X Mail 13.0 \(3594.4.17\)) Subject: Re: [edk2-devel] [RFC PATCH v2 38/44] UefiCpuPkg: Allow AP booting under SEV-ES Date: Fri, 11 Oct 2019 23:42:55 -0700 In-reply-to: Cc: "Lendacky, Thomas" , Jordan Justen , Ard Biesheuvel , Mike Kinney , Liming Gao , Eric Dong , Ray Ni , "Singh, Brijesh" , =?utf-8?Q?Philippe_Mathieu-Daud=C3=A9?= To: devel@edk2.groups.io, Laszlo Ersek 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> X-Mailer: Apple Mail (2.3594.4.17) X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2019-10-12_03:,, signatures=0 Content-type: multipart/alternative; boundary="Apple-Mail=_1377D3B7-1093-43ED-8362-A5FF889107B7" --Apple-Mail=_1377D3B7-1093-43ED-8362-A5FF889107B7 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=us-ascii Laszlo, For 2) this is very unfortunate. I think the root cause is for those of u= s who work on x86 hardware day to day we get programed that SEC/PEI is IA32= and DXE is X64, and this can lead to some unfortunate coding outcomes.=20 I'm guessing this code probably got ported from the DXE CPU driver or some= other place that had no XIP assumptions. One option vs. patching is puttin= g the relocations in the .data section. The only issue with that could be t= he need to align sections on page boundaries and that may take up too much = space in XIP code. Perhaps we could only require the .data section relocati= ons for XCODE, and map them to .text for the other toolchain?=20 Thanks, Andrew Fish > On Oct 11, 2019, at 1:56 AM, Laszlo Ersek wrote: >=20 > On 10/11/19 01:17, Lendacky, Thomas wrote: >> On 10/3/19 10:12 AM, Tom Lendacky wrote: >>>=20 >>>=20 >>> On 10/3/19 5:32 AM, Laszlo Ersek wrote: >>>> On 10/03/19 12:12, Laszlo Ersek wrote: >>>>=20 >>>>> UINT32 ApEntryPoint; >>>>> EFI_GUID SevEsFooterGuid; >>>>> UINT16 Size; >>>>=20 >>>> It's probably better to reverse the order of "Size" and >>>> "SevEsFooterGuid", like this: >>>>=20 >>>> UINT32 ApEntryPoint; >>>> UINT16 Size; >>>> EFI_GUID SevEsFooterGuid; >>>>=20 >>>> because then even the "Size" field can be changed (or resized), as a >>>> function of the footer GUID. >>>=20 >>> Cool, I'll look into doing this and see how it works out. >>=20 >> 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: >>=20 >> 1. QemuFlashDetected() will attempt to detect how the flash memory devi= ce >> 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. >>=20 >> The solution here was to check for SEV-ES being enabled and just retu= rn >> false from QemuFlashDetected(). Any downfalls to doing that? >=20 > Short-circuiting QemuFlashDetected() on SEV-ES seems appropriate. >=20 > 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. >=20 > 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. >=20 >>=20 >> 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. >=20 > That's... stunning. >=20 > Commit 2db0ccc2d7fe changes the file >=20 > UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm >=20 > such that it does in-place binary patching. >=20 > This source file is referenced from: >=20 > UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.= inf >=20 > as well. Note "SecPei". >=20 > That makes the commit buggy, to my eyes, regardless of SEV-ES. Because: >=20 > 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. >=20 > Upon re-reading >, > this commit worked around an XCODE toolchain bug. >=20 > 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.) >=20 > 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. >=20 > (I'll comment on this issue in a different thread too; I'll CC you on it= .) >=20 >> 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. >>=20 >> After those two changes, the above method works well. >=20 > I'm happy to hear! >=20 > Thanks, > Laszlo >=20 >=20 --Apple-Mail=_1377D3B7-1093-43ED-8362-A5FF889107B7 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=us-ascii Laszlo,

For 2) this  is very unfortunate= . I think the root cause is for those of us who work on x86 hardware day to= day we get programed that SEC/PEI is IA32 and DXE is X64, and this can lea= d to some unfortunate coding outcomes. 

I'm guessing this code probably got ported from the DXE C= PU driver or some other place that had no XIP assumptions. One option vs. p= atching is putting the relocations in the .data section. The only issue wit= h that could be the need to align sections on page boundaries and that may = take up too much space in XIP code. Perhaps we could only require the .data= section relocations for XCODE, and map them to .text for the other toolcha= in? 

Thanks,

Andrew Fish

On Oct 11, 2019, at 1:56 AM, Laszlo Ersek = <lersek@redhat.com&g= t; wrote:

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  &n= bsp;ApEntryPoint;
 EFI_GUID SevEsFooterGuid;
 UINT16   Size;

It's probably better to reverse the order of "Size" and
"Sev= EsFooterGuid", like this:

 UINT32  &= nbsp;ApEntryPoint;
 UINT16   Size;
 EFI_GUID SevEsFooterGuid;

because the= n even the "Size" field can be changed (or resized), as a
fun= ction 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, bu= t has a couple of
caveats. Removing the Qemu change to make t= he flash mapped read-only in
the nested page tables, caused t= he following:

1. QemuFlashDetected() will atte= mpt 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-E= S,
  emulation of the instruction can't be performe= d (can't read guest
  memory and not provided the f= aulting instruction bytes), so the vCPU is
  just r= estarted. 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 s= eems appropriate.

However, I don't understand why you retur= n FALSE in that case. You
should return TRUE. If QemuFlashDetected() returns FALSE, then the UEFI<= /span>
variable store will not= be backed by the real pflash chip, it will be
emulated with an \NvVars file on the EFI system par= tition. That
emulation = should really not be used nowadays.

So IMO the right appro= ach here is:
- declare = that SEV-ES only targets the "two pflash chips" setup
- return TRUE from QemuFlashDetected() when = SEV-ES is on.


2. Commit 2db0ccc= 2d7fe ("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 dev= ice.

That's... stunning.

Commit 2db0ccc2d7fe changes the file=

 UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Except= ionHandlerAsm.nasm

such that it does in-place binary pat= ching.

This source file is referenced from:

 UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandler= Lib.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<= /span>
time when the exception= handler is located in flash. That's incorrect on
physical hardware too.

Upon re-r= eading <https://bugzilla.tia= nocore.org/show_bug.cgi?id=3D849>,
this commit worked around an XCODE toolchain bug.=

Unfortunately, the workaround is not suitable for the SEC = phase. (Also
not suitab= le 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 ple= ase also make the new bug
block BZ#2198.

(I'll comment on this issue in a differen= t thread too; I'll CC you on it.)

 &nb= sp;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


--Apple-Mail=_1377D3B7-1093-43ED-8362-A5FF889107B7--