On 11/21/19 21:46, Tom Lendacky wrote:
> On 11/21/19 6:06 AM, Laszlo Ersek wrote:
>> On 11/20/19 21:06, Lendacky, Thomas wrote:
>>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2198
>>>
>>> An SEV-ES guest will generate a #VC exception when it encounters a
>>> non-automatic exit (NAE) event. It is expected that the #VC exception
>>> handler will communicate with the hypervisor using the GHCB to handle
>>> the NAE event.
>>>
>>> NAE events can occur during the Sec phase, so initialize exception
>>> handling early in the OVMF Sec support.
>>>
>>> Cc: Jordan Justen <jordan.l.jus...@intel.com>
>>> Cc: Laszlo Ersek <ler...@redhat.com>
>>> Cc: Ard Biesheuvel <ard.biesheu...@linaro.org>
>>> Signed-off-by: Tom Lendacky <thomas.lenda...@amd.com>
>>> ---
>>> OvmfPkg/Sec/SecMain.inf | 1 +
>>> OvmfPkg/Sec/SecMain.c | 29 ++++++++++++++++-------------
>>> 2 files changed, 17 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/OvmfPkg/Sec/SecMain.inf b/OvmfPkg/Sec/SecMain.inf
>>> index 63ba4cb555fb..7f53845f5436 100644
>>> --- a/OvmfPkg/Sec/SecMain.inf
>>> +++ b/OvmfPkg/Sec/SecMain.inf
>>> @@ -50,6 +50,7 @@ [LibraryClasses]
>>> PeCoffExtraActionLib
>>> ExtractGuidedSectionLib
>>> LocalApicLib
>>> + CpuExceptionHandlerLib
>>>
>>> [Ppis]
>>> gEfiTemporaryRamSupportPpiGuid # PPI ALWAYS_PRODUCED
>>> diff --git a/OvmfPkg/Sec/SecMain.c b/OvmfPkg/Sec/SecMain.c
>>> index bae9764577f0..db319030ee58 100644
>>> --- a/OvmfPkg/Sec/SecMain.c
>>> +++ b/OvmfPkg/Sec/SecMain.c
>>> @@ -24,6 +24,7 @@
>>> #include <Library/PeCoffExtraActionLib.h>
>>> #include <Library/ExtractGuidedSectionLib.h>
>>> #include <Library/LocalApicLib.h>
>>> +#include <Library/CpuExceptionHandlerLib.h>
>>>
>>> #include <Ppi/TemporaryRamSupport.h>
>>>
>>> @@ -737,6 +738,21 @@ SecCoreStartupWithStack (
>>> Table[Index] = 0;
>>> }
>>>
>>> + //
>>> + // Initialize IDT
>>> + //
>>> + IdtTableInStack.PeiService = NULL;
>>> + for (Index = 0; Index < SEC_IDT_ENTRY_COUNT; Index ++) {
>>> + CopyMem (&IdtTableInStack.IdtTable[Index], &mIdtEntryTemplate, sizeof
>>> (mIdtEntryTemplate));
>>> + }
>>> +
>>> + IdtDescriptor.Base = (UINTN)&IdtTableInStack.IdtTable;
>>> + IdtDescriptor.Limit = (UINT16)(sizeof (IdtTableInStack.IdtTable) - 1);
>>> +
>>> + AsmWriteIdtr (&IdtDescriptor);
>>> +
>>> + InitializeCpuExceptionHandlers (NULL);
>>> +
>>> ProcessLibraryConstructorList (NULL, NULL);
>>>
>>> DEBUG ((EFI_D_INFO,
>>
>> (1) The problem here is that we call multiple library APIs before
>> calling ProcessLibraryConstructorList() -- namely CopyMem(),
>> AsmWriteIdtr(), and InitializeCpuExceptionHandlers().
>>
>> (See also the "SetMem" reference in the leading context, in the
>> source file -- it is not quoted in this patch.)
>>
>> Thus, would it be possible to move all the "+" lines, quoted above,
>> just below the ProcessLibraryConstructorList() call?
>
> Unfortunately, I can't. The invocation of
> ProcessLibraryConstructorList() results in #VC exceptions and so the
> exception handler needs to be in place before invoking
> ProcessLibraryConstructorList(). It looks like there are some
> SerialPort I/O writes to initialize the serial port and some PCI I/O
> reads and writes from AcpiTimerLibConstructor() in
> OvmfPkg/Library/AcpiTimerLib/BaseRomAcpiTimerLib.c.
I have to accept what you're saying, but this makes the code quite
brittle. It's a tenet that we don't call library APIs before the library
constructor had a chance to initialize whatever memory or hardware the
library APIs rely on.
So, in this case,
(a) please add a comment above this block that we're making an exception
with CopyMem(), AsmWriteIdtr() and InitializeCpuExceptionHandlers(),
(b) I'd still like to see this pre-constructor logic restricted to
SEV-ES. (More on that below.)
So something like:
if (SevEs) {
//
// We have to initialize the IDT and set up exception handlers here,
// i.e. before calling library constructors, because those library
// constructors may access hardware such that #VC exceptions are
// triggered.
//
// Due to this code executing before library constructors, *all*
// library API calls are theoretically interface contract
// violations. However, because we are in SEC (executing in flash),
// those constructors cannot write variables with static storage
// duration anyway. Furthermore, we call a small, restricted set of
// APIs, such as CopyMem(), AsmWriteIdtr(),
// InitializeCpuExceptionHandlers(), where we require that the
// underlying library instance not trigger any #VC exceptions.
//
InitIdt ();
InitExceptionHandlers ();
}
ProcessLibraryConstructorList ();
if (!SevEs) {
InitIdt ();
}
This makes me feel safer because:
- we're explicit about the principles we (have to) break,
- even our limited assumptions are restricted to SEV-ES.
>
>>
>>
>> (2) If possible I'd like to restrict the
>> InitializeCpuExceptionHandlers() call to SevEsIsEnabled().
>>
>> (Unless you're implying that InitializeCpuExceptionHandlers() is
>> useful even without SEV-ES -- but then the commit message should be
>> reworded accordingly.)
>
> It does give you earlier exception handling and displays the exception
> information should an exception occur during SEC.
>
> But, it might require some tricks to somehow communicate from the
> ResetVector code to the SecMain code that SEV-ES is enabled. This is
> because you need to do a CPUID instruction to determine if SEV-ES is
> enabled, which will generate a #VC, which requires an exception
> handler...
So even *checking* whether SEV-ES is enabled requires a #VC handler to
be set up. Thanks for the reminder. How about this idea: in
[edk2-devel] [RFC PATCH v3 37/43]
OvmfPkg: Reserve a page in memory for the SEV-ES AP reset vector
you are carving out a page at PcdSevEsResetRipBase -- only in
"OvmfPkgX64.fdf". And, a very small (leading) stretch of that page is
used as the SEV_ES_AP_JMP_FAR structure.
Now, could we implement the following?
(1) Append a UINT8 ("BOOLEAN") field to the SEV_ES_AP_JMP_FAR structure
(and possibly rename the structure),
(2) in the OvmfPkgX64 reset vector, where you determine SEV-ES anyway,
explicitly set this field to zero if SEV-ES is disabled, and set it to
one, if SEV-ES is enabled,
(3) In OvmfPkg/Sec, introduce a new (local) header file declaring the
function SevEsIsEnabled(),
(4) Provide two C-language implementations (under the Ia32 and X64
directories): in the 32-bit version, return constant FALSE; in the
64-bit version, return the value of the new field. Something like:
return ((SEV_ES_AP_JMP_FAR *)FixedPcdGet32
(PcdSevEsResetRipBase))->SevEsEnabled;
FixedPcdGet32() is explicitly safe to use without library constructors
having run.
Does this look viable? (It might require you to reshuffle patch 37 vs.
patch 30.)
Thanks!
Laszlo
>
> Thanks,
> Tom
>
>>
>> If you agree to that restriction, then I further suggest reordering this
>> patch against the next one:
>>
>> [edk2-devel] [RFC PATCH v3 31/43]
>> OvmfPkg/Sec: Enable cache early to speed up booting
>>
>> Namely, if you put that patch first, then in the present patch you can
>> extend the already existing SevEsIsEnabled()-dependent scope, with a
>> call to InitializeCpuExceptionHandlers().
>>
>> The end result would be something like:
>>
>> ProcessLibraryConstructorList();
>>
>> //
>> // Initialize IDT
>> // ...
>> //
>>
>> if (SevEsIsEnabled()) {
>> //
>> // non-automatic exit events (NAE) can occur during SEC ...
>> //
>> InitializeCpuExceptionHandlers (NULL);
>>
>> //
>> // Under SEV-ES, the hypervisor can't modify CR0 ...
>> //
>> AsmEnableCache ();
>> }
>>
>> What's your opinion?
>>
>> Thanks!
>> Laszlo
>>
>>> @@ -751,19 +767,6 @@ SecCoreStartupWithStack (
>>> //
>>> InitializeFloatingPointUnits ();
>>>
>>> - //
>>> - // Initialize IDT
>>> - //
>>> - IdtTableInStack.PeiService = NULL;
>>> - for (Index = 0; Index < SEC_IDT_ENTRY_COUNT; Index ++) {
>>> - CopyMem (&IdtTableInStack.IdtTable[Index], &mIdtEntryTemplate, sizeof
>>> (mIdtEntryTemplate));
>>> - }
>>> -
>>> - IdtDescriptor.Base = (UINTN)&IdtTableInStack.IdtTable;
>>> - IdtDescriptor.Limit = (UINT16)(sizeof (IdtTableInStack.IdtTable) - 1);
>>> -
>>> - AsmWriteIdtr (&IdtDescriptor);
>>> -
>>> #if defined (MDE_CPU_X64)
>>> //
>>> // ASSERT that the Page Tables were set by the reset vector code to
>>>
>>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#51189): https://edk2.groups.io/g/devel/message/51189
Mute This Topic: https://groups.io/mt/60973137/21656
Mute #vc: https://groups.io/mk?hashtag=vc&subid=3846945
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
