From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from nwk-aaemail-lapp01.apple.com (nwk-aaemail-lapp01.apple.com [17.151.62.66]) by mx.groups.io with SMTP id smtpd.web10.904.1570906222175491954 for ; Sat, 12 Oct 2019 11:50:22 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@apple.com header.s=20180706 header.b=e5l22LSX; spf=pass (domain: apple.com, ip: 17.151.62.66, mailfrom: afish@apple.com) Received: from pps.filterd (nwk-aaemail-lapp01.apple.com [127.0.0.1]) by nwk-aaemail-lapp01.apple.com (8.16.0.27/8.16.0.27) with SMTP id x9CIkTXf062864; Sat, 12 Oct 2019 11:50:18 -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=NCDJjJVqn6Fu5OwyP5MAW4nxAXoSAK9kNyQ2u7JEJcw=; b=e5l22LSX3Rzk944eCsRTIhLhQgPG1gs4NzcjtBSCb1lKxP/wYMT6hEpK1tWMayd2HKoo phhJTF+wnl76LQF5U5Xy85/5A4ANnV/C9pdjTX3QtoIN+Y48KMuUTPTta9O5cnbz4s2x 9wjpIMVZEO2P+0UbO2OSWcIZmu/XogfKFvCXs57gPAy06uZovzi11qCYoKC4QJMAf8xL OLU1RErLu/w+OCM1ux8ma2vx9AXn90TiOF+12ZB7qmGupi2apxjA0u6AUPhgmlMx1ROS cWX/CBzGeaMrZjkPBFvNPSXd2X87kmJ3p3lA5JhvPrRW8vOzoReBLNHiQVEr6iz5TAuE 2Q== Received: from ma1-mtap-s02.corp.apple.com (ma1-mtap-s02.corp.apple.com [17.40.76.6]) by nwk-aaemail-lapp01.apple.com with ESMTP id 2vkds23d4q-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=NO); Sat, 12 Oct 2019 11:50:18 -0700 Received: from nwk-mmpp-sz12.apple.com (nwk-mmpp-sz12.apple.com [17.128.115.204]) by ma1-mtap-s02.corp.apple.com (Oracle Communications Messaging Server 8.0.2.4.20190507 64bit (built May 7 2019)) with ESMTPS id <0PZ900EDEYZTD110@ma1-mtap-s02.corp.apple.com>; Sat, 12 Oct 2019 11:50:17 -0700 (PDT) Received: from process_milters-daemon.nwk-mmpp-sz12.apple.com by nwk-mmpp-sz12.apple.com (Oracle Communications Messaging Server 8.0.2.4.20190507 64bit (built May 7 2019)) id <0PZ900K00YX7AH00@nwk-mmpp-sz12.apple.com>; Sat, 12 Oct 2019 11:50:17 -0700 (PDT) X-Va-A: X-Va-T-CD: 7daa14ab80d2839c17f099a8fda5373c X-Va-E-CD: 17a6de702c0d9dcaf89f320d435a4bc6 X-Va-R-CD: 4287d1648b5c1ab4e136057a2bee17a6 X-Va-CD: 0 X-Va-ID: fba26cb5-8ffc-4ef0-8a08-8933d18e2197 X-V-A: X-V-T-CD: 7daa14ab80d2839c17f099a8fda5373c X-V-E-CD: 17a6de702c0d9dcaf89f320d435a4bc6 X-V-R-CD: 4287d1648b5c1ab4e136057a2bee17a6 X-V-CD: 0 X-V-ID: 506c200b-cbea-44c5-9bc8-8eade37a65b6 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2019-10-12_10:,, signatures=0 Received: from [17.235.46.147] (unknown [17.235.46.147]) by nwk-mmpp-sz12.apple.com (Oracle Communications Messaging Server 8.0.2.4.20190507 64bit (built May 7 2019)) with ESMTPSA id <0PZ9001DVYZQYP60@nwk-mmpp-sz12.apple.com>; Sat, 12 Oct 2019 11:50:16 -0700 (PDT) Sender: afish@apple.com From: "Andrew Fish" Message-id: <3391DF85-25CC-4B8B-AEBF-0CB833B7713F@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: Sat, 12 Oct 2019 11:50:13 -0700 In-reply-to: <4A89E2EF3DFEDB4C8BFDE51014F606A14E516356@SHSMSX104.ccr.corp.intel.com> Cc: Laszlo Ersek , "Lendacky, Thomas" , Jordan Justen , Ard Biesheuvel , Mike Kinney , "Dong, Eric" , "Ni, Ray" , "Singh, Brijesh" , =?utf-8?Q?Philippe_Mathieu-Daud=C3=A9?= To: devel@edk2.groups.io, liming.gao@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> <4A89E2EF3DFEDB4C8BFDE51014F606A14E516356@SHSMSX104.ccr.corp.intel.com> X-Mailer: Apple Mail (2.3594.4.17) X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2019-10-12_10:,, signatures=0 Content-type: multipart/alternative; boundary="Apple-Mail=_9497D104-9652-4E78-B78C-CCE0DC19352E" --Apple-Mail=_9497D104-9652-4E78-B78C-CCE0DC19352E Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 Liming, Here is a simple example of a global with absolute function address in it.= = =20 1) The text section uses %rip relative addressing. So the 1st goto techniq= ue is to convert absolute addressing to PC relative addressing if possible.= = =20 2) The data section can contain absolute addresses. The data section is re= ad/write. As you can see .quad can have a pointer to an absolute address (t= hat would require a relocation). 3) The text section can access data section via PC relative addressing.=20 4) While the code looks like it is located together the data section is go= ing to follow the text section and get aligned to section alignment. So in = my simple example the data section is 4K from the start of the text section= .=20 5) If all else fails the assembler will let you put code in the data secti= on, and that code can have relocations, but see 4).=20 ~/work/Compiler>cat relocation.c int main(); void *gRelocation =3D (void *)main; int main () { return (int)(unsigned long long)gRelocation; } ~/work/Compiler>clang -S -Os relocation.c ~/work/Compiler>cat relocation.S .section __TEXT,__text,regular,pure_instructions .globl _main ## -- Begin function main _main: ## @main pushq %rbp movq %rsp, %rbp movl _gRelocation(%rip), %eax popq %rbp retq ## -- End function .section __DATA,__data .globl _gRelocation ## @gRelocation .p2align 3 _gRelocation: .quad _main .subsections_via_symbols If you have questions about a specific chunk of code to convert let me kno= w.=20 Thanks, Andrew Fish > On Oct 12, 2019, at 12:46 AM, Liming Gao wrote: >=20 > Andrew: > Can you give more detail on how to update nasm source code to put the = 64bit absolute address from .text section to .data section? I will verify i= t. Now, the patching way doesn=E2=80=99t support X64 SEC/PEI. This is a gab= in XCODE tool chain.=20 > > Thanks > Liming > From: devel@edk2.groups.io [mailto:devel@e= dk2.groups.io ] On Behalf Of Andrew Fish via G= roups.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, Michael D >; Gao, Liming >; Dong, Eric >; Ni, Ray >; Singh, Brijesh= >; Philippe Mathieu-D= aud=C3=A9 > > Subject: Re: [edk2-devel] [RFC PATCH v2 38/44] UefiCpuPkg: Allow AP boot= ing 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 programed that SEC/PEI is IA= 32 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 so= me other place that had no XIP assumptions. One option vs. patching is putt= ing the relocations in the .data section. The only issue with that could be= the need to align sections on page boundaries and that may take up too muc= h space in XIP code. Perhaps we could only require the .data section reloca= tions for XCODE, and map them to .text for the other toolchain?=20 > > Thanks, > > Andrew Fish >=20 >=20 > On Oct 11, 2019, at 1:56 AM, Laszlo Ersek > wrote: > > On 10/11/19 01:17, Lendacky, Thomas wrote: >=20 > On 10/3/19 10:12 AM, Tom Lendacky wrote: >=20 >=20 >=20 > On 10/3/19 5:32 AM, Laszlo Ersek wrote: >=20 > On 10/03/19 12:12, Laszlo Ersek wrote: >=20 >=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 devic= e > 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 i= s > just restarted. This results in an infinite #NPF occurring. >=20 > The solution here was to check for SEV-ES being enabled and just retur= n > 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 >=20 > 2. Commit 2db0ccc2d7fe ("UefiCpuPkg: Update CpuExceptionHandlerLib pass > XCODE5 tool chain") causes a similar situation to #1. It attempts to d= o > 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 >=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 > >=20 --Apple-Mail=_9497D104-9652-4E78-B78C-CCE0DC19352E Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8 Liming,
Here is a simple example of a global wit= h absolute function address in it. 

1) The text section uses %rip relative addressing. = So the 1st goto technique is to convert absolute addressing to PC relative = addressing if possible. 
2) The data section can = contain absolute addresses. The data section is read/write. As you can see = .quad can have a pointer to an absolute address (that would require a reloc= ation).
3) The text section can access data section vi= a PC relative addressing. 
4) While the code look= s like it is located together the data section is going to follow the text = section and get aligned to section alignment. So in my simple example the d= ata section is 4K from the start of the text section. 
<= div class=3D"">5) If all else fails the assembler will let you put code in = the data section, and that code can have relocations, but see 4). 

~/work/Compiler>cat relocation.c
int main();
void *gRelocation =3D (void *)main;

int main ()
{
  return (int)(unsigned long long)gRelocation;
}
~/work/Compiler>clang -S -Os relocation.c
~/work/Compiler>cat relocation.S
.section __TEXT,__text,regu= lar,pure_instructions
.globl _main                =   ## -- Begin function main
_main:            &n= bsp;                     = ## @main
pushq = %rbp
= movq %rsp, %rbp
movl _gRelocation(%rip)= , %eax
popq %rbp
<= div style=3D"margin: 0px; font-stretch: normal; font-size: 11px; line-heigh= t: normal; font-family: Menlo; color: rgb(0, 0, 0);" class=3D""> retq
    &nb= sp;                     &= nbsp;             ## -- End function

.section __DATA,__data
.globl _gRelocation  &= nbsp;         ## @gRelocation
.p2align 3
_gRelocation:
.quad _main


.subsections_via_symbols

If you have questions about = a specific chunk of code to convert let me know. 

Thanks,

Andrew Fish

=
On Oct 12, 2019, at 12= :46 AM, Liming Gao <l= iming.gao@intel.com> wrote:

Andrew:
  Can you give mor= e detail on how to update nasm source code to put the 64bit absolute addres= s from .text section to .data section? I will verify it. Now, the patching = way doesn=E2=80=99t 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 Andrew Fish via Groups.Io
Sent: Saturday, October 12, 2019 2:43 PM
To: devel@edk2.groups.io; Laszlo Ersek <lerse= k@redhat.com>
Cc: Lendacky, Thomas <Thomas.Lendacky@amd.com>; Justen, Jordan L <jordan.l.justen@intel.com>; Ard Biesheuvel &l= t;ard.biesheuvel@linaro.org>; Kinn= ey, Michael D <michael.d.kinney@intel= .com>; Gao, Liming <liming.gao@inte= l.com>; Dong, Eric <eric.dong@intel= .com>; Ni, Ray <ray.ni@intel.com>= ; Singh, Brijesh <brijesh.singh@amd.com>; Philippe Mathieu-Daud=C3=A9 <philmd@red= hat.com>
Subject: Re: [edk2-devel] [RFC PATCH v2 38/44] UefiC= puPkg: Allow AP booting under SEV-ES
 
Laszlo,
 
For 2) this  is very unfortunate. I think the roo= t cause is for those of us who work on x86 hardware day to day we get progr= amed that SEC/PEI is IA32 and DXE is X64, and this can lead to some unfortu= nate 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 relocat= ions in the .data section. The only issue with that could be the need to al= ign 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 toolchain? 
 
Thanks,
 <= /div>
Andre= w Fish


On Oct 11, 201= 9, at 1:56 AM, Laszlo Ersek <lersek@redhat.c= om> wrote:
 
On 10/11/19 01:17, Lendacky, Thoma= s 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, La= szlo Ersek wrote:


 UINT3= 2   ApEntryPoint;
 EFI_GUID SevEsFooterGuid; UINT16   Size;

It's probably better to reverse the order of "Size" and
"S= evEsFooterGuid", 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 c= ouple of
caveats. Removing the Qemu change to make the flash = mapped read-only in
the nested page tables, caused the follow= ing:

1. QemuFlashDetected() will attempt to de= tect how the flash memory device
  behaves. Because= it is marked as read-only by the hypervisor, writing
 &= nbsp;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 i= nstruction bytes), so the vCPU is
  just restarted.= This results in an infinite #NPF occurring.

&= nbsp; The solution here was to check for SEV-ES being enabled and just= return
  false from QemuFlashDetected(). Any downf= alls to doing that?

Short-circuiting Qemu= FlashDetected() on SEV-ES seems appropriate.

H= owever, I don't understand why you return FALSE in that case. You
should return TRUE. If QemuFlashDetected() returns FALSE, then the U= EFI
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 ("UefiCpuPk= g: 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.

Comm= it 2db0ccc2d7fe changes the file

 UefiCpu= Pkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm

such that it does in-place binary patching.

This source file is referenced from:

 UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHa= ndlerLib.inf

as well. Note "SecPei".

That makes the commit buggy, to my eyes, regardless o= f SEV-ES. Because:

The binary patching appears= to occur in the SEC phase as well, i.e. at a
time when the e= xception handler is located in flash. That's incorrect on
phy= sical 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#84= 9 in the See Also field, and please also make the new bug
blo= ck BZ#2198.

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


&nbs= p; Reverting that commit fixes the issue. I don't think that will be a= n
  acceptable solution, though, so need to think a= bout what to do here.

After those two changes,= the above method works well.

I'm happy t= o hear!

Thanks,
Laszlo


 


<= /div> --Apple-Mail=_9497D104-9652-4E78-B78C-CCE0DC19352E--