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

["Lendacky, Thomas" <thomas.lendacky@amd.com>]

On 11/22/19 6:52 AM, Laszlo Ersek wrote:
> 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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2198&data=02%7C01%7Cthomas.lendacky%40amd.com%7C86943210aff44681d5b908d76f4acf49%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637100239406633853&sdata=8APpMBall%2F2urZh6V3kpuWpsBjOh8GVqhWGNBjL%2B30U%3D&reserved=0
>>>>
>>>> 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().

We can reduce this exposure a bit and replace the CopyMem() call with
something similar to the loop above it.  I could also use assembler
code directly in here to load the IDTR.

That would leave just InitializeCpuExceptionHandlers(). Is there
something that can added so as to warn when a library has a
CONSTRUCTOR added to/part of the definition?

>>>
>>> (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(),

Can do.

> 
> (b) I'd still like to see this pre-constructor logic restricted to
> SEV-ES. (More on that below.)

Yup, propagating the SEV-ES status from the ResetVector seems to be the
way to go.

> 
> 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.)

I think this does. Since this is SEC and the reset vector page isn't
needed until PEI and later we could even just use the first byte (make a
union with an SEC usage field) and make this even simpler. Then we don't
have to worry about positioning it. Let me work on that and see where I
get. Anything after the #VC is established would use the current method
of determine SEV-ES status.

Thanks!
Tom

> 
> 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
>>>>
>>>
>>
>