From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by mx.groups.io with SMTP id smtpd.web10.2753.1570866414415061917 for ; Sat, 12 Oct 2019 00:46:54 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.136, mailfrom: liming.gao@intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by fmsmga106.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 12 Oct 2019 00:46:54 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.67,286,1566889200"; d="scan'208,217";a="194549648" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by fmsmga007.fm.intel.com with ESMTP; 12 Oct 2019 00:46:54 -0700 Received: from FMSMSX110.amr.corp.intel.com (10.18.116.10) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.439.0; Sat, 12 Oct 2019 00:46:53 -0700 Received: from shsmsx101.ccr.corp.intel.com (10.239.4.153) by fmsmsx110.amr.corp.intel.com (10.18.116.10) with Microsoft SMTP Server (TLS) id 14.3.439.0; Sat, 12 Oct 2019 00:46:53 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.166]) by SHSMSX101.ccr.corp.intel.com ([169.254.1.96]) with mapi id 14.03.0439.000; Sat, 12 Oct 2019 15:46:51 +0800 From: "Liming Gao" To: "devel@edk2.groups.io" , "afish@apple.com" , Laszlo Ersek , "Gao, Liming" CC: "Lendacky, Thomas" , "Justen, Jordan L" , Ard Biesheuvel , "Kinney, Michael D" , "Dong, Eric" , "Ni, Ray" , "Singh, Brijesh" , =?iso-8859-1?Q?Philippe_Mathieu-Daud=E9?= Subject: Re: [edk2-devel] [RFC PATCH v2 38/44] UefiCpuPkg: Allow AP booting under SEV-ES Thread-Topic: [edk2-devel] [RFC PATCH v2 38/44] UefiCpuPkg: Allow AP booting under SEV-ES Thread-Index: AQHVbyPzSgCtb4tTPkmIaISrkjhcKKdHBCMAgAAC6ACAAC0RAIABDbmAgAAFl4CAAE4EgIALh/IAgAChqACAAW0fgIAAlXnQ Date: Sat, 12 Oct 2019 07:46:50 +0000 Message-ID: <4A89E2EF3DFEDB4C8BFDE51014F606A14E516356@SHSMSX104.ccr.corp.intel.com> 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> <635C113A-D0A2-4A11-AF9A-8A65BBE1C9BC@apple.com> In-Reply-To: <635C113A-D0A2-4A11-AF9A-8A65BBE1C9BC@apple.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: liming.gao@intel.com Content-Language: en-US Content-Type: multipart/alternative; boundary="_000_4A89E2EF3DFEDB4C8BFDE51014F606A14E516356SHSMSX104ccrcor_" --_000_4A89E2EF3DFEDB4C8BFDE51014F606A14E516356SHSMSX104ccrcor_ Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Andrew: Can you give more detail on how to update nasm source code to put the 64= bit absolute address from .text section to .data section? I will verify it.= Now, the patching way doesn't support X64 SEC/PEI. This is a gab in XCODE = tool chain. Thanks Liming From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Andr= ew Fish via Groups.Io Sent: Saturday, October 12, 2019 2:43 PM To: devel@edk2.groups.io; Laszlo Ersek Cc: Lendacky, Thomas ; Justen, Jordan L ; Ard Biesheuvel ; Kinney, Mic= hael D ; Gao, Liming ; Do= ng, Eric ; Ni, Ray ; Singh, Brijesh = ; Philippe Mathieu-Daud=E9 Subject: Re: [edk2-devel] [RFC PATCH v2 38/44] UefiCpuPkg: Allow AP bootin= g under SEV-ES 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. 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? Thanks, Andrew Fish On Oct 11, 2019, at 1:56 AM, Laszlo Ersek > 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 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.in= f 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 --_000_4A89E2EF3DFEDB4C8BFDE51014F606A14E516356SHSMSX104ccrcor_ Content-Type: text/html; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable

Andrew:

  Can you give more detail on h= ow to update nasm source code to put the 64bit absolute address from .text = section to .data section? I will verify it. Now, the patching way doesn’t support X64 SEC/PEI. This is a gab in XCODE to= ol chain.

 

Thanks

Liming

From: devel@edk2.groups.io [mailto:= devel@edk2.groups.io] On Behalf Of Andrew Fish via Groups.Io
Sent: Saturday, October 12, 2019 2:43 PM
To: devel@edk2.groups.io; Laszlo Ersek <lersek@redhat.com> Cc: Lendacky, Thomas <Thomas.Lendacky@amd.com>; Justen, Jorda= n L <jordan.l.justen@intel.com>; Ard Biesheuvel <ard.biesheuvel@li= naro.org>; Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Li= ming <liming.gao@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Singh, Brijesh <brijesh.singh@amd.co= m>; Philippe Mathieu-Daud=E9 <philmd@redhat.com>
Subject: Re: [edk2-devel] [RFC PATCH v2 38/44] UefiCpuPkg: Allow AP= booting under SEV-ES

 

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 pr= ogramed that SEC/PEI is IA32 and DXE is X64, and this can lead to some unfo= rtunate coding outcomes. 

 

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 putting the relocations in the .data section. The only iss= ue with that could be the need to align sections on page boundaries and that may take up too much space in XIP co= de. Perhaps we could only require the .data section relocations for XCODE, = and map them to .text for the other toolchain? 

 

Thanks,

 

Andrew Fish



On Oct 11, 2019, at 1:56 AM, Laszlo Ersek <lersek@redhat.com> 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   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<= br>   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 gu= est
  memory and not provided the faulting instruction bytes), so th= e vCPU is
  just restarted. This results in an infinite #NPF occurring.
  The solution here was to check for SEV-ES being enabled and ju= st 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<= br> - return TRUE from QemuFlashDetected() when SEV-ES is on.



2. Commit 2db0ccc2d7fe ("UefiCpuPkg: Update CpuExceptionHandlerLib pa= ss
  XCODE5 tool chain") causes a similar situation to #1. It = attempts to do
  some address fixups and write to the flash device.<= /span>


That's... stunning.

Commit 2db0ccc2d7fe changes the file

 UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.na= sm

such that it does in-place binary patching.

This source file is referenced from:

 UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerL= ib.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 <https://bugzilla.tianocore.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 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


 

--_000_4A89E2EF3DFEDB4C8BFDE51014F606A14E516356SHSMSX104ccrcor_--