Re: [edk2-devel] [RFC PATCH v3 30/43] OvmfPkg/Sec: Add #VC exception handling for Sec phase

["Laszlo Ersek" <lersek@redhat.com>]

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.justen@intel.com>
>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> Signed-off-by: Tom Lendacky <thomas.lendacky@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
>>>
>>
>