Re: [edk2-devel] [RFC PATCH v2 38/44] UefiCpuPkg: Allow AP booting under SEV-ES
> On Oct 14, 2019, at 6:11 AM, Laszlo Ersek wrote: > > On 10/12/19 08:42, Andrew Fish wrote: >> 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 lead to some unfortunate >> coding outcomes. > > First I was confused by this; I didn't understand why the "bitness" of > SEC/PEI mattered here. > > But, of course, you are right: if SEC/PEI are 32-bit, then the > problematic relocations are never generated by the compiler in the first > place. > Laszlo, The other "human" problem is people assume DXE always runs from memory so they think that patching is OK. I've even see people use IA32 to mean PEI on X64 platforms. So the implementation (32-bit PEI) leaks into the architecture. >> 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 issue with >> 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 toolchain? > > In SEC, all sections of the binary are located in flash, aren't they? Yes. > Why would it help if the relocations were placed in .data? Sorry if I was unclear. The Xcode linker (ld64) enforces a macOS x86_64 ABI that makes it so even the image loader can't write the text section. Thus you can't link an image that has relocations in the text section, and there is no flag to ld64 to turn this behavior off. So the workaround is to generate code like the compiler and don't have any relocations in the text section. Moving the relocation from the text to the data section is needed to make the Xcode linker (ld64) link the code. if there are relocations in the text section then the link will fail on Xcode builds. When you are writing assembly code there is no restriction about placing code in the data section. Generally we try to fix issues by converting the code to use PC relative addressing, but a quick and dirty fix is to change the code over to the data section. The cleaner solution is to have a global in the data section that contains the relocatable address, and that is like the simple example of the C code global I sent out. > I'm reminded > of global variables in SEC, and those are not writable in SEC either. > (At least on QEMU.) I'm uncertain if this is somehow connected to > Cache-as-RAM, but if it is: QEMU does not support Cache-as-RAM. > For SEC the relocations would have been applied at build time. The SEC will have been linked at around zero, and when it was placed in the FV it would have been relocated to its XIP location. For code running from memory the EFI PE/COFF loader would apply the relocations as part of loading the image. But non of the above works if your build fails due to a linker error :(. Thanks, Andrew Fish > Thanks > Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48935): https://edk2.groups.io/g/devel/message/48935 Mute This Topic: https://groups.io/mt/34203585/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [RFC PATCH v2 38/44] UefiCpuPkg: Allow AP booting under SEV-ES
On 10/12/19 08:42, Andrew Fish wrote: > 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 lead to some unfortunate > coding outcomes. First I was confused by this; I didn't understand why the "bitness" of SEC/PEI mattered here. But, of course, you are right: if SEC/PEI are 32-bit, then the problematic relocations are never generated by the compiler in the first place. > 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 issue with > 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 toolchain? In SEC, all sections of the binary are located in flash, aren't they? Why would it help if the relocations were placed in .data? I'm reminded of global variables in SEC, and those are not writable in SEC either. (At least on QEMU.) I'm uncertain if this is somehow connected to Cache-as-RAM, but if it is: QEMU does not support Cache-as-RAM. Thanks Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48901): https://edk2.groups.io/g/devel/message/48901 Mute This Topic: https://groups.io/mt/34203585/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [RFC PATCH v2 38/44] UefiCpuPkg: Allow AP booting under SEV-ES
Liming,
Here is a simple example of a global with 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 relocation).
3) The text section can access data section via PC relative addressing.
4) While the code looks 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 data section is 4K from the start of the text section.
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 = (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
.p2align3
_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 wrote:
>
> 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 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>
> [mailto: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 <mailto:devel@edk2.groups.io>; Laszlo Ersek
> mailto:ler...@redhat.com>>
> Cc: Lendacky, Thomas <mailto:thomas.lenda...@amd.com>>; Justen, Jordan L
> mailto:jordan.l.jus...@intel.com>>; Ard
> Biesheuvel mailto:ard.biesheu...@linaro.org>>;
> Kinney, Michael D <mailto:michael.d.kin...@intel.com>>; Gao, Liming <mailto:liming@intel.com>>; Dong, Eric <mailto:eric.d...@intel.com>>; Ni, Ray <mailto:ray...@intel.com>>; Singh, Brijesh <mailto:brijesh.si...@amd.com>>; Philippe Mathieu-Daudé <mailto:phi...@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 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 putting
> 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 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,
>
> Andrew Fish
>
>
> On Oct 11, 2019, at 1:56 AM, Laszlo Ersek <mailto:ler...@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
> behaves. B
Re: [edk2-devel] [RFC PATCH v2 38/44] UefiCpuPkg: Allow AP booting under SEV-ES
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 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 Andrew
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, Michael D ; Gao, Liming
; Dong, Eric ; Ni, Ray
; Singh, Brijesh ; Philippe
Mathieu-Daudé
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 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 putting 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 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,
Andrew Fish
On Oct 11, 2019, at 1:56 AM, Laszlo Ersek
mailto:ler...@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
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 <https://bugzilla.tianocore.org/show_bug.cgi?id=849>,
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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#488
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 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 putting 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 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,
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.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
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#48861): https://edk2.groups.io/g/devel/message/48861
Mute This Topic: https://groups.io/mt/34203585/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [RFC PATCH v2 38/44] UefiCpuPkg: Allow AP booting under SEV-ES
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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#48807): https://edk2.groups.io/g/devel/message/48807
Mute This Topic: https://groups.io/mt/34203585/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [RFC PATCH v2 38/44] UefiCpuPkg: Allow AP booting under SEV-ES
> On Oct 10, 2019, at 4:17 PM, 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?
>
> 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.
>
> 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.
>
Did you fill a bugzilla for 2)?
Thanks,
Andrew Fish
> After those two changes, the above method works well.
>
> Thanks,
> Tom
>
>>
>> Thanks!
>> Tom
>>
>>>
>>> Thanks
>>> Laszlo
>>>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#48766): https://edk2.groups.io/g/devel/message/48766
Mute This Topic: https://groups.io/mt/34203585/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [RFC PATCH v2 38/44] UefiCpuPkg: Allow AP booting under SEV-ES
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?
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.
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.
Thanks,
Tom
>
> Thanks!
> Tom
>
>>
>> Thanks
>> Laszlo
>>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#48762): https://edk2.groups.io/g/devel/message/48762
Mute This Topic: https://groups.io/mt/34203585/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [RFC PATCH v2 38/44] UefiCpuPkg: Allow AP booting under SEV-ES
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. Thanks! Tom > > Thanks > Laszlo > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48436): https://edk2.groups.io/g/devel/message/48436 Mute This Topic: https://groups.io/mt/34203585/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [RFC PATCH v2 38/44] UefiCpuPkg: Allow AP booting under SEV-ES
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. Thanks Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48428): https://edk2.groups.io/g/devel/message/48428 Mute This Topic: https://groups.io/mt/34203585/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [RFC PATCH v2 38/44] UefiCpuPkg: Allow AP booting under SEV-ES
On 10/02/19 20:07, Lendacky, Thomas wrote:
> On 10/2/19 10:26 AM, Laszlo Ersek wrote:
>> On 10/02/19 17:15, Laszlo Ersek wrote:
>>> Adding Phil.
>>>
>>> I'm looking at this patch only because one thing caught my attention in
>>> the previous one, "OvmfPkg: Add support for SEV-ES AP reset vector
>>> re-directing":
>>>
>>> On 09/19/19 21:53, Lendacky, Thomas wrote:
From: Tom Lendacky
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2198
Typically, an AP is booted using the INIT-SIPI-SIPI sequence. This
sequence is intercepted by the hypervisor, which sets the AP's registers
to the values requested by the sequence. At that point, the hypervisor can
start the AP, which will then begin execution at the appropriate location.
Under SEV-ES, AP booting presents some challenges since the hypervisor is
not allowed to alter the AP's register state. In this situation, we have
to distinguish between the AP's first boot and AP's subsequent boots.
First boot:
Once the AP's register state has been defined (which is before the guest
is first booted) it cannot be altered. Should the hypervisor attempt to
alter the register state, the change would be detected by the hardware
and the VMRUN instruction would fail. Given this, the first boot for the
AP is required to begin execution with this initial register state, which
is typically the reset vector. This prevents the BSP from directing the
AP startup location through the INIT-SIPI-SIPI sequence.
To work around this, provide a four-byte field at offset 0xffd0 that
can contain an IP / CS register combination, that if non-zero, causes
the AP to perform a far jump to that location instead of a near jump to
EarlyBspInitReal16. Before booting the AP for the first time, the BSP
should set the IP / CS value for the AP based on the value that would be
derived from the INIT-SIPI-SIPI sequence.
>>>
>>> I don't understand how this can work: the guest-phys address 0xffd0
>>> is backed by read-only pflash in most OVMF deployments.
>>>
>>> In addition:
>>>
>>> [...]
>>>
@@ -1002,6 +1204,7 @@ WakeUpAP (
CpuMpData->InitFlag != ApInitDone) {
ResetVectorRequired = TRUE;
AllocateResetVector (CpuMpData);
+AllocateSevEsAPMemory (CpuMpData);
FillExchangeInfoData (CpuMpData);
SaveLocalApicTimerSetting (CpuMpData);
}
@@ -1038,6 +1241,15 @@ WakeUpAP (
}
}
if (ResetVectorRequired) {
+ //
+ // For SEV-ES, set the jump address for initial AP boot
+ //
+ if (CpuMpData->SevEsActive) {
+SEV_ES_AP_JMP_FAR *JmpFar = (SEV_ES_AP_JMP_FAR *)0xFFD0;
+
+JmpFar->ApStart.Rip = 0;
+JmpFar->ApStart.Segment = (UINT16) (ExchangeInfo->BufferStart >>
4);
+ }
>>>
>>> Even if the address is backed by a single "unified" pflash, mapped r/w
>>> -- which we can call a "non-standard OVMF deployment" nowadays --, a
>>> normal store doesn't appear sufficient to me. The first write to pflash
>>> will flip it to "programming mode", and the values stored are supposed
>>> to be pflash commands (not just the raw data we intend to put in place).
>>>
>>> See for example the QemuFlashWrite() function in
>>> "OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c". (But, again, that
>>> is used with the pflash chip that hosts the variable store, and is
>>> therefore mapped r/w.)
>>>
>>>
>>> Taking a step back... I don't think APs execute any code from pflash,
>>> when MpInitLib boots them.
>>>
>>> In OVMF, PcdCpuApLoopMode is ApInHltLoop (value 1), therefore
>>> "CpuMpData->WakeUpByInitSipiSipi" should be TRUE, and
>>> "ResetVectorRequired" too should be TRUE, at first AP boot.
>>> Consequently, the reset vector seems to be allocated with
>>> AllocateResetVector().
>>>
>>> AllocateResetVector() has separate implementations for PEI and DXE, but
>>> in both cases, it returns RAM. So I don't see where the AP accesses (or
>>> executes) pflash.
>>
>> ... I believe I understand that this is precisely what cannot work under
>> SEV-ES -- because we cannot launch an AP at an address that's
>> dynamically chosen by the firmware (and passed to the hypervisor), like
>> with INIT-SIPI-SIPI.
>
> Correct.
>
>>
>> And so firmware and hypervisor have to agree upon a *constant* AP reset
>> vector address, in advance.
>>
>> We have two options:
>>
>> - pick the reset vector address *constant* such that it falls into RAM, or
>>
>> - let the AP reset vector reside in pflash, but then the code in pflash
>> has to look for a parameter block at a fixed address in RAM.
>>
>> So in the end, both options require the same -- we need a RAM address
>> constant that is determined at firmware build time. Either for the reset
>> vector itself, or for the reset vector's parameter block.
>
> As long
Re: [edk2-devel] [RFC PATCH v2 38/44] UefiCpuPkg: Allow AP booting under SEV-ES
On 10/02/19 19:58, Lendacky, Thomas wrote:
> On 10/2/19 10:15 AM, Laszlo Ersek via Groups.Io wrote:
>> Adding Phil.
>>
>> I'm looking at this patch only because one thing caught my attention in
>> the previous one, "OvmfPkg: Add support for SEV-ES AP reset vector
>> re-directing":
>>
>> On 09/19/19 21:53, Lendacky, Thomas wrote:
>>> From: Tom Lendacky
>>>
>>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2198
>>>
>>> Typically, an AP is booted using the INIT-SIPI-SIPI sequence. This
>>> sequence is intercepted by the hypervisor, which sets the AP's registers
>>> to the values requested by the sequence. At that point, the hypervisor can
>>> start the AP, which will then begin execution at the appropriate location.
>>>
>>> Under SEV-ES, AP booting presents some challenges since the hypervisor is
>>> not allowed to alter the AP's register state. In this situation, we have
>>> to distinguish between the AP's first boot and AP's subsequent boots.
>>>
>>> First boot:
>>> Once the AP's register state has been defined (which is before the guest
>>> is first booted) it cannot be altered. Should the hypervisor attempt to
>>> alter the register state, the change would be detected by the hardware
>>> and the VMRUN instruction would fail. Given this, the first boot for the
>>> AP is required to begin execution with this initial register state, which
>>> is typically the reset vector. This prevents the BSP from directing the
>>> AP startup location through the INIT-SIPI-SIPI sequence.
>>>
>>> To work around this, provide a four-byte field at offset 0xffd0 that
>>> can contain an IP / CS register combination, that if non-zero, causes
>>> the AP to perform a far jump to that location instead of a near jump to
>>> EarlyBspInitReal16. Before booting the AP for the first time, the BSP
>>> should set the IP / CS value for the AP based on the value that would be
>>> derived from the INIT-SIPI-SIPI sequence.
>>
>> I don't understand how this can work: the guest-phys address 0xffd0
>> is backed by read-only pflash in most OVMF deployments.
>>
>> In addition:
>>
>> [...]
>>
>>> @@ -1002,6 +1204,7 @@ WakeUpAP (
>>>CpuMpData->InitFlag != ApInitDone) {
>>> ResetVectorRequired = TRUE;
>>> AllocateResetVector (CpuMpData);
>>> +AllocateSevEsAPMemory (CpuMpData);
>>> FillExchangeInfoData (CpuMpData);
>>> SaveLocalApicTimerSetting (CpuMpData);
>>>}
>>> @@ -1038,6 +1241,15 @@ WakeUpAP (
>>>}
>>> }
>>> if (ResetVectorRequired) {
>>> + //
>>> + // For SEV-ES, set the jump address for initial AP boot
>>> + //
>>> + if (CpuMpData->SevEsActive) {
>>> +SEV_ES_AP_JMP_FAR *JmpFar = (SEV_ES_AP_JMP_FAR *)0xFFD0;
>>> +
>>> +JmpFar->ApStart.Rip = 0;
>>> +JmpFar->ApStart.Segment = (UINT16) (ExchangeInfo->BufferStart >>
>>> 4);
>>> + }
>>
>> Even if the address is backed by a single "unified" pflash, mapped r/w
>> -- which we can call a "non-standard OVMF deployment" nowadays --, a
>> normal store doesn't appear sufficient to me. The first write to pflash
>> will flip it to "programming mode", and the values stored are supposed
>> to be pflash commands (not just the raw data we intend to put in place).
>
> There is a corresponding patch in Qemu that does not set the
> KVM_MEM_READONLY flag for the ROM when starting an SEV-ES guest, thus
> allowing the MP library to update the location with desired address.
Isn't that a price too high to pay? It seems to allow the memory mapped
contents of the pflash chip to diverge from the host-side file, even if
the QEMU command line specifies that the pflash chip is to be mapped r/o.
With regard to SMM, I think this could even be considered a security
problem -- if the pflash region is not marked as readonly, then the
guest OS could write to it as well (with direct hardware access, like
the code seen above). The write would not trap to QEMU, and QEMU
couldn't prevent the write.
The OS could use this to inject code into the pflash region (e.g., SEC).
Although the host-side pflash file would not be modified, if the OS
performed a warm reboot (or S3 cycle) afterwards, the code it injected
into the flash region would be executed with firmware privileges.
AMD Publication #56421 states that SMM is currently out of scope for
SEV-ES, so I don't think there's a problem right now; I'm just generally
concerned that here we're enabling something I've always considered a
big no-no for SMM builds.
I still have to read your next response though (to my suggestion to use
a fixed RAM address, I believe).
Thanks!
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#48426): https://edk2.groups.io/g/devel/message/48426
Mute This Topic: https://groups.io/mt/34203585/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [RFC PATCH v2 38/44] UefiCpuPkg: Allow AP booting under SEV-ES
On 10/2/19 10:26 AM, Laszlo Ersek wrote:
> On 10/02/19 17:15, Laszlo Ersek wrote:
>> Adding Phil.
>>
>> I'm looking at this patch only because one thing caught my attention in
>> the previous one, "OvmfPkg: Add support for SEV-ES AP reset vector
>> re-directing":
>>
>> On 09/19/19 21:53, Lendacky, Thomas wrote:
>>> From: Tom Lendacky
>>>
>>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2198
>>>
>>> Typically, an AP is booted using the INIT-SIPI-SIPI sequence. This
>>> sequence is intercepted by the hypervisor, which sets the AP's registers
>>> to the values requested by the sequence. At that point, the hypervisor can
>>> start the AP, which will then begin execution at the appropriate location.
>>>
>>> Under SEV-ES, AP booting presents some challenges since the hypervisor is
>>> not allowed to alter the AP's register state. In this situation, we have
>>> to distinguish between the AP's first boot and AP's subsequent boots.
>>>
>>> First boot:
>>> Once the AP's register state has been defined (which is before the guest
>>> is first booted) it cannot be altered. Should the hypervisor attempt to
>>> alter the register state, the change would be detected by the hardware
>>> and the VMRUN instruction would fail. Given this, the first boot for the
>>> AP is required to begin execution with this initial register state, which
>>> is typically the reset vector. This prevents the BSP from directing the
>>> AP startup location through the INIT-SIPI-SIPI sequence.
>>>
>>> To work around this, provide a four-byte field at offset 0xffd0 that
>>> can contain an IP / CS register combination, that if non-zero, causes
>>> the AP to perform a far jump to that location instead of a near jump to
>>> EarlyBspInitReal16. Before booting the AP for the first time, the BSP
>>> should set the IP / CS value for the AP based on the value that would be
>>> derived from the INIT-SIPI-SIPI sequence.
>>
>> I don't understand how this can work: the guest-phys address 0xffd0
>> is backed by read-only pflash in most OVMF deployments.
>>
>> In addition:
>>
>> [...]
>>
>>> @@ -1002,6 +1204,7 @@ WakeUpAP (
>>>CpuMpData->InitFlag != ApInitDone) {
>>> ResetVectorRequired = TRUE;
>>> AllocateResetVector (CpuMpData);
>>> +AllocateSevEsAPMemory (CpuMpData);
>>> FillExchangeInfoData (CpuMpData);
>>> SaveLocalApicTimerSetting (CpuMpData);
>>>}
>>> @@ -1038,6 +1241,15 @@ WakeUpAP (
>>>}
>>> }
>>> if (ResetVectorRequired) {
>>> + //
>>> + // For SEV-ES, set the jump address for initial AP boot
>>> + //
>>> + if (CpuMpData->SevEsActive) {
>>> +SEV_ES_AP_JMP_FAR *JmpFar = (SEV_ES_AP_JMP_FAR *)0xFFD0;
>>> +
>>> +JmpFar->ApStart.Rip = 0;
>>> +JmpFar->ApStart.Segment = (UINT16) (ExchangeInfo->BufferStart >>
>>> 4);
>>> + }
>>
>> Even if the address is backed by a single "unified" pflash, mapped r/w
>> -- which we can call a "non-standard OVMF deployment" nowadays --, a
>> normal store doesn't appear sufficient to me. The first write to pflash
>> will flip it to "programming mode", and the values stored are supposed
>> to be pflash commands (not just the raw data we intend to put in place).
>>
>> See for example the QemuFlashWrite() function in
>> "OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c". (But, again, that
>> is used with the pflash chip that hosts the variable store, and is
>> therefore mapped r/w.)
>>
>>
>> Taking a step back... I don't think APs execute any code from pflash,
>> when MpInitLib boots them.
>>
>> In OVMF, PcdCpuApLoopMode is ApInHltLoop (value 1), therefore
>> "CpuMpData->WakeUpByInitSipiSipi" should be TRUE, and
>> "ResetVectorRequired" too should be TRUE, at first AP boot.
>> Consequently, the reset vector seems to be allocated with
>> AllocateResetVector().
>>
>> AllocateResetVector() has separate implementations for PEI and DXE, but
>> in both cases, it returns RAM. So I don't see where the AP accesses (or
>> executes) pflash.
>
> ... I believe I understand that this is precisely what cannot work under
> SEV-ES -- because we cannot launch an AP at an address that's
> dynamically chosen by the firmware (and passed to the hypervisor), like
> with INIT-SIPI-SIPI.
Correct.
>
> And so firmware and hypervisor have to agree upon a *constant* AP reset
> vector address, in advance.
>
> We have two options:
>
> - pick the reset vector address *constant* such that it falls into RAM, or
>
> - let the AP reset vector reside in pflash, but then the code in pflash
> has to look for a parameter block at a fixed address in RAM.
>
> So in the end, both options require the same -- we need a RAM address
> constant that is determined at firmware build time. Either for the reset
> vector itself, or for the reset vector's parameter block.
As long as the hypervisor has a way to determine a build time address,
that would be the preferred method. I couldn't come up with a way (or at
least don't know of a
Re: [edk2-devel] [RFC PATCH v2 38/44] UefiCpuPkg: Allow AP booting under SEV-ES
On 10/02/19 17:15, Laszlo Ersek wrote:
> Adding Phil.
>
> I'm looking at this patch only because one thing caught my attention in
> the previous one, "OvmfPkg: Add support for SEV-ES AP reset vector
> re-directing":
>
> On 09/19/19 21:53, Lendacky, Thomas wrote:
>> From: Tom Lendacky
>>
>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2198
>>
>> Typically, an AP is booted using the INIT-SIPI-SIPI sequence. This
>> sequence is intercepted by the hypervisor, which sets the AP's registers
>> to the values requested by the sequence. At that point, the hypervisor can
>> start the AP, which will then begin execution at the appropriate location.
>>
>> Under SEV-ES, AP booting presents some challenges since the hypervisor is
>> not allowed to alter the AP's register state. In this situation, we have
>> to distinguish between the AP's first boot and AP's subsequent boots.
>>
>> First boot:
>> Once the AP's register state has been defined (which is before the guest
>> is first booted) it cannot be altered. Should the hypervisor attempt to
>> alter the register state, the change would be detected by the hardware
>> and the VMRUN instruction would fail. Given this, the first boot for the
>> AP is required to begin execution with this initial register state, which
>> is typically the reset vector. This prevents the BSP from directing the
>> AP startup location through the INIT-SIPI-SIPI sequence.
>>
>> To work around this, provide a four-byte field at offset 0xffd0 that
>> can contain an IP / CS register combination, that if non-zero, causes
>> the AP to perform a far jump to that location instead of a near jump to
>> EarlyBspInitReal16. Before booting the AP for the first time, the BSP
>> should set the IP / CS value for the AP based on the value that would be
>> derived from the INIT-SIPI-SIPI sequence.
>
> I don't understand how this can work: the guest-phys address 0xffd0
> is backed by read-only pflash in most OVMF deployments.
>
> In addition:
>
> [...]
>
>> @@ -1002,6 +1204,7 @@ WakeUpAP (
>>CpuMpData->InitFlag != ApInitDone) {
>> ResetVectorRequired = TRUE;
>> AllocateResetVector (CpuMpData);
>> +AllocateSevEsAPMemory (CpuMpData);
>> FillExchangeInfoData (CpuMpData);
>> SaveLocalApicTimerSetting (CpuMpData);
>>}
>> @@ -1038,6 +1241,15 @@ WakeUpAP (
>>}
>> }
>> if (ResetVectorRequired) {
>> + //
>> + // For SEV-ES, set the jump address for initial AP boot
>> + //
>> + if (CpuMpData->SevEsActive) {
>> +SEV_ES_AP_JMP_FAR *JmpFar = (SEV_ES_AP_JMP_FAR *)0xFFD0;
>> +
>> +JmpFar->ApStart.Rip = 0;
>> +JmpFar->ApStart.Segment = (UINT16) (ExchangeInfo->BufferStart >> 4);
>> + }
>
> Even if the address is backed by a single "unified" pflash, mapped r/w
> -- which we can call a "non-standard OVMF deployment" nowadays --, a
> normal store doesn't appear sufficient to me. The first write to pflash
> will flip it to "programming mode", and the values stored are supposed
> to be pflash commands (not just the raw data we intend to put in place).
>
> See for example the QemuFlashWrite() function in
> "OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c". (But, again, that
> is used with the pflash chip that hosts the variable store, and is
> therefore mapped r/w.)
>
>
> Taking a step back... I don't think APs execute any code from pflash,
> when MpInitLib boots them.
>
> In OVMF, PcdCpuApLoopMode is ApInHltLoop (value 1), therefore
> "CpuMpData->WakeUpByInitSipiSipi" should be TRUE, and
> "ResetVectorRequired" too should be TRUE, at first AP boot.
> Consequently, the reset vector seems to be allocated with
> AllocateResetVector().
>
> AllocateResetVector() has separate implementations for PEI and DXE, but
> in both cases, it returns RAM. So I don't see where the AP accesses (or
> executes) pflash.
... I believe I understand that this is precisely what cannot work under
SEV-ES -- because we cannot launch an AP at an address that's
dynamically chosen by the firmware (and passed to the hypervisor), like
with INIT-SIPI-SIPI.
And so firmware and hypervisor have to agree upon a *constant* AP reset
vector address, in advance.
We have two options:
- pick the reset vector address *constant* such that it falls into RAM, or
- let the AP reset vector reside in pflash, but then the code in pflash
has to look for a parameter block at a fixed address in RAM.
So in the end, both options require the same -- we need a RAM address
constant that is determined at firmware build time. Either for the reset
vector itself, or for the reset vector's parameter block.
Thanks
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#48377): https://edk2.groups.io/g/devel/message/48377
Mute This Topic: https://groups.io/mt/34203585/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub
Re: [edk2-devel] [RFC PATCH v2 38/44] UefiCpuPkg: Allow AP booting under SEV-ES
Adding Phil.
I'm looking at this patch only because one thing caught my attention in
the previous one, "OvmfPkg: Add support for SEV-ES AP reset vector
re-directing":
On 09/19/19 21:53, Lendacky, Thomas wrote:
> From: Tom Lendacky
>
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2198
>
> Typically, an AP is booted using the INIT-SIPI-SIPI sequence. This
> sequence is intercepted by the hypervisor, which sets the AP's registers
> to the values requested by the sequence. At that point, the hypervisor can
> start the AP, which will then begin execution at the appropriate location.
>
> Under SEV-ES, AP booting presents some challenges since the hypervisor is
> not allowed to alter the AP's register state. In this situation, we have
> to distinguish between the AP's first boot and AP's subsequent boots.
>
> First boot:
> Once the AP's register state has been defined (which is before the guest
> is first booted) it cannot be altered. Should the hypervisor attempt to
> alter the register state, the change would be detected by the hardware
> and the VMRUN instruction would fail. Given this, the first boot for the
> AP is required to begin execution with this initial register state, which
> is typically the reset vector. This prevents the BSP from directing the
> AP startup location through the INIT-SIPI-SIPI sequence.
>
> To work around this, provide a four-byte field at offset 0xffd0 that
> can contain an IP / CS register combination, that if non-zero, causes
> the AP to perform a far jump to that location instead of a near jump to
> EarlyBspInitReal16. Before booting the AP for the first time, the BSP
> should set the IP / CS value for the AP based on the value that would be
> derived from the INIT-SIPI-SIPI sequence.
I don't understand how this can work: the guest-phys address 0xffd0
is backed by read-only pflash in most OVMF deployments.
In addition:
[...]
> @@ -1002,6 +1204,7 @@ WakeUpAP (
>CpuMpData->InitFlag != ApInitDone) {
> ResetVectorRequired = TRUE;
> AllocateResetVector (CpuMpData);
> +AllocateSevEsAPMemory (CpuMpData);
> FillExchangeInfoData (CpuMpData);
> SaveLocalApicTimerSetting (CpuMpData);
>}
> @@ -1038,6 +1241,15 @@ WakeUpAP (
>}
> }
> if (ResetVectorRequired) {
> + //
> + // For SEV-ES, set the jump address for initial AP boot
> + //
> + if (CpuMpData->SevEsActive) {
> +SEV_ES_AP_JMP_FAR *JmpFar = (SEV_ES_AP_JMP_FAR *)0xFFD0;
> +
> +JmpFar->ApStart.Rip = 0;
> +JmpFar->ApStart.Segment = (UINT16) (ExchangeInfo->BufferStart >> 4);
> + }
Even if the address is backed by a single "unified" pflash, mapped r/w
-- which we can call a "non-standard OVMF deployment" nowadays --, a
normal store doesn't appear sufficient to me. The first write to pflash
will flip it to "programming mode", and the values stored are supposed
to be pflash commands (not just the raw data we intend to put in place).
See for example the QemuFlashWrite() function in
"OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c". (But, again, that
is used with the pflash chip that hosts the variable store, and is
therefore mapped r/w.)
Taking a step back... I don't think APs execute any code from pflash,
when MpInitLib boots them.
In OVMF, PcdCpuApLoopMode is ApInHltLoop (value 1), therefore
"CpuMpData->WakeUpByInitSipiSipi" should be TRUE, and
"ResetVectorRequired" too should be TRUE, at first AP boot.
Consequently, the reset vector seems to be allocated with
AllocateResetVector().
AllocateResetVector() has separate implementations for PEI and DXE, but
in both cases, it returns RAM. So I don't see where the AP accesses (or
executes) pflash.
Thanks,
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#48376): https://edk2.groups.io/g/devel/message/48376
Mute This Topic: https://groups.io/mt/34203585/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [RFC PATCH v2 38/44] UefiCpuPkg: Allow AP booting under SEV-ES
From: Tom Lendacky BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2198 Typically, an AP is booted using the INIT-SIPI-SIPI sequence. This sequence is intercepted by the hypervisor, which sets the AP's registers to the values requested by the sequence. At that point, the hypervisor can start the AP, which will then begin execution at the appropriate location. Under SEV-ES, AP booting presents some challenges since the hypervisor is not allowed to alter the AP's register state. In this situation, we have to distinguish between the AP's first boot and AP's subsequent boots. First boot: Once the AP's register state has been defined (which is before the guest is first booted) it cannot be altered. Should the hypervisor attempt to alter the register state, the change would be detected by the hardware and the VMRUN instruction would fail. Given this, the first boot for the AP is required to begin execution with this initial register state, which is typically the reset vector. This prevents the BSP from directing the AP startup location through the INIT-SIPI-SIPI sequence. To work around this, provide a four-byte field at offset 0xffd0 that can contain an IP / CS register combination, that if non-zero, causes the AP to perform a far jump to that location instead of a near jump to EarlyBspInitReal16. Before booting the AP for the first time, the BSP should set the IP / CS value for the AP based on the value that would be derived from the INIT-SIPI-SIPI sequence. Subsequent boots: Again, the hypervisor cannot alter the AP register state, so a method is required to take the AP out of halt state and redirect it to the desired IP location. If it is determined that the AP is running in an SEV-ES guest, then instead of calling CpuSleep(), a VMGEXIT is issued with the AP Reset Hold exit code (0x8004). The hypervisor will put the AP in a halt state, waiting for an INIT-SIPI-SIPI sequence. Once the sequence is recognized, the hypervisor will resume the AP. At this point the AP must transition from the current 64-bit long mode down to 16-bit real mode and begin executing at the derived location from the INIT-SIPI-SIPI sequence. Another change is around the area of obtaining the (x2)APIC ID during AP startup. During AP startup, the AP can't take a #VC exception before the AP has established a stack. However, the AP stack is set by using the (x2)APIC ID, which is obtained through CPUID instructions. A CPUID instruction will cause a #VC, so a different method must be used. The GHCB protocol supports a method to obtain CPUID information from the hypervisor through the GHCB MSR. This method does not require a stack, so it is used to obtain the necessary CPUID information to determine the (x2)APIC ID. The OVMF SEV support is updated to set the SEV-ES active PCD entry (PcdSevEsActive) when the guest is an SEV-ES guest. Also, the OVMF support is updated to create its own reset vector routine in order to supply the far jump field required for an AP first boot. The new 16-bit protected mode GDT entry is used in order to transition from 64-bit long mode down to 16-bit real mode. A new assembler routine is created that takes the AP from 64-bit long mode to 16-bit real mode. This is located under 1MB in memory and transitions from 64-bit long mode to 32-bit compatibility mode to 16-bit protected mode and finally 16-bit real mode. Cc: Eric Dong Cc: Ray Ni Cc: Laszlo Ersek Signed-off-by: Tom Lendacky --- UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf | 2 + UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf | 2 + UefiCpuPkg/Library/MpInitLib/MpLib.h | 58 + UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 70 - UefiCpuPkg/Library/MpInitLib/MpLib.c | 235 - UefiCpuPkg/Library/MpInitLib/PeiMpLib.c | 19 ++ UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c | 2 +- UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc | 2 +- .../Library/MpInitLib/Ia32/MpFuncs.nasm | 15 ++ UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc| 4 +- UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 239 ++ 11 files changed, 634 insertions(+), 14 deletions(-) diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf index 557507e9a466..ad5f33451aa3 100644 --- a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf @@ -51,6 +51,7 @@ [LibraryClasses] UefiBootServicesTableLib DebugAgentLib SynchronizationLib + VmgExitLib [Protocols] gEfiTimerArchProtocolGuid ## SOMETIMES_CONSUMES @@ -69,4 +70,5 @@ [Pcd] gUefiCpuPkgTokenSpaceGuid.PcdCpuApTargetCstate ## SOMETIMES_CONSUMES gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard ## CONSUMES gEfiMdeModulePkgTokenSpaceGuid.PcdSevEsActive## CONSUMES + gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbBase
