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

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

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?


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

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
>