Re: [edk2-devel] [PATCH v5 34/42] OvmfPkg/Sec: Add #VC exception handling for Sec phase

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

Hi Tom,

On 03/03/20 00:07, 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>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>

this patch has changed significantly since the last version (v4); I
think my R-b given for v4 should not have been picked up in v5.

AFAICS the new stuff is SevEsProtocolFailure() and SevEsProtocolCheck(),
which -- per v5 blurb -- have moved from UefiCpuPkg to OvmfPkg:


> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
>  OvmfPkg/Sec/SecMain.inf |   4 ++
>  OvmfPkg/Sec/SecMain.c   | 151 ++++++++++++++++++++++++++++++++++++----
>  2 files changed, 142 insertions(+), 13 deletions(-)
> 
> diff --git a/OvmfPkg/Sec/SecMain.inf b/OvmfPkg/Sec/SecMain.inf
> index 63ba4cb555fb..7f78dcee2772 100644
> --- a/OvmfPkg/Sec/SecMain.inf
> +++ b/OvmfPkg/Sec/SecMain.inf
> @@ -50,15 +50,19 @@ [LibraryClasses]
>    PeCoffExtraActionLib
>    ExtractGuidedSectionLib
>    LocalApicLib
> +  CpuExceptionHandlerLib
>  
>  [Ppis]
>    gEfiTemporaryRamSupportPpiGuid                # PPI ALWAYS_PRODUCED
>  
>  [Pcd]
> +  gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvBase
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvSize
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDxeMemFvBase
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDxeMemFvSize
> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase
> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesBase
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize
> diff --git a/OvmfPkg/Sec/SecMain.c b/OvmfPkg/Sec/SecMain.c
> index bae9764577f0..577596a949f9 100644
> --- a/OvmfPkg/Sec/SecMain.c
> +++ b/OvmfPkg/Sec/SecMain.c
> @@ -24,6 +24,9 @@
>  #include <Library/PeCoffExtraActionLib.h>
>  #include <Library/ExtractGuidedSectionLib.h>
>  #include <Library/LocalApicLib.h>
> +#include <Library/CpuExceptionHandlerLib.h>
> +#include <Register/Amd/Ghcb.h>
> +#include <Register/Amd/Msr.h>
>  
>  #include <Ppi/TemporaryRamSupport.h>
>  
> @@ -34,6 +37,10 @@ typedef struct _SEC_IDT_TABLE {
>    IA32_IDT_GATE_DESCRIPTOR  IdtTable[SEC_IDT_ENTRY_COUNT];
>  } SEC_IDT_TABLE;
>  
> +typedef struct _SEC_SEV_ES_WORK_AREA {
> +  UINT8  SevEsEnabled;
> +} SEC_SEV_ES_WORK_AREA;
> +
>  VOID
>  EFIAPI
>  SecStartupPhase2 (
> @@ -712,6 +719,90 @@ FindAndReportEntryPoints (
>    return;
>  }
>  
> +STATIC
> +VOID
> +SevEsProtocolFailure (
> +  IN UINT8  ReasonCode
> +  )
> +{
> +  MSR_SEV_ES_GHCB_REGISTER  Msr;
> +
> +  //
> +  // Use the GHCB MSR Protocol to request termination by the hypervisor
> +  //
> +  Msr.GhcbPhysicalAddress = 0;
> +  Msr.GhcbTerminate.Function = GHCB_INFO_TERMINATE_REQUEST;
> +  Msr.GhcbTerminate.ReasonCodeSet = GHCB_TERMINATE_GHCB;
> +  Msr.GhcbTerminate.ReasonCode = ReasonCode;
> +  AsmWriteMsr64 (MSR_SEV_ES_GHCB, Msr.GhcbPhysicalAddress);
> +
> +  AsmVmgExit ();
> +
> +  ASSERT (0);

(1) The argument should be FALSE, not 0.

> +  CpuDeadLoop ();
> +}
> +
> +STATIC
> +VOID
> +SevEsProtocolCheck (
> +  VOID
> +  )
> +{
> +  MSR_SEV_ES_GHCB_REGISTER  Msr;
> +  GHCB                      *Ghcb;
> +
> +  //
> +  // Use the GHCB MSR Protocol to obtain the GHCB SEV-ES Information for
> +  // protocol checking
> +  //
> +  Msr.GhcbPhysicalAddress = 0;
> +  Msr.GhcbInfo.Function = GHCB_INFO_SEV_INFO_GET;
> +  AsmWriteMsr64 (MSR_SEV_ES_GHCB, Msr.GhcbPhysicalAddress);
> +
> +  AsmVmgExit ();
> +
> +  Msr.GhcbPhysicalAddress = AsmReadMsr64 (MSR_SEV_ES_GHCB);
> +
> +  if (Msr.GhcbInfo.Function != GHCB_INFO_SEV_INFO) {
> +    SevEsProtocolFailure (GHCB_TERMINATE_GHCB_GENERAL);
> +  }
> +
> +  if (Msr.GhcbProtocol.SevEsProtocolMin > Msr.GhcbProtocol.SevEsProtocolMax) {
> +    SevEsProtocolFailure (GHCB_TERMINATE_GHCB_PROTOCOL);
> +  }
> +
> +  if ((Msr.GhcbProtocol.SevEsProtocolMin > GHCB_VERSION_MAX) ||
> +      (Msr.GhcbProtocol.SevEsProtocolMax < GHCB_VERSION_MIN)) {
> +    SevEsProtocolFailure (GHCB_TERMINATE_GHCB_PROTOCOL);
> +  }
> +
> +  //
> +  // SEV-ES protocol checking succeeded, set the initial GHCB address
> +  //
> +  Msr.GhcbPhysicalAddress = FixedPcdGet32 (PcdOvmfSecGhcbBase);
> +  AsmWriteMsr64(MSR_SEV_ES_GHCB, Msr.GhcbPhysicalAddress);
> +
> +  Ghcb = Msr.Ghcb;
> +  SetMem (Ghcb, sizeof (*Ghcb), 0);
> +
> +  /* Set the version to the maximum that can be supported */

(2) The comment style is not consistent with the rest.

With the style warts (1) and (2) fixed:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo

> +  Ghcb->ProtocolVersion = MIN (Msr.GhcbProtocol.SevEsProtocolMax, GHCB_VERSION_MAX);
> +  Ghcb->GhcbUsage = GHCB_STANDARD_USAGE;
> +}
> +
> +STATIC
> +BOOLEAN
> +SevEsIsEnabled (
> +  VOID
> +  )
> +{
> +  SEC_SEV_ES_WORK_AREA  *SevEsWorkArea;
> +
> +  SevEsWorkArea = (SEC_SEV_ES_WORK_AREA *) FixedPcdGet32 (PcdSevEsWorkAreaBase);
> +
> +  return ((SevEsWorkArea != NULL) && (SevEsWorkArea->SevEsEnabled != 0));
> +}
> +
>  VOID
>  EFIAPI
>  SecCoreStartupWithStack (
> @@ -737,8 +828,55 @@ SecCoreStartupWithStack (
>      Table[Index] = 0;
>    }
>  
> +  //
> +  // Initialize IDT - Since this is before library constructors are called,
> +  // we use a loop rather than CopyMem.
> +  //
> +  IdtTableInStack.PeiService = NULL;
> +  for (Index = 0; Index < SEC_IDT_ENTRY_COUNT; Index ++) {
> +    UINT8  *Src, *Dst;
> +    UINTN  Byte;
> +
> +    Src = (UINT8 *) &mIdtEntryTemplate;
> +    Dst = (UINT8 *) &IdtTableInStack.IdtTable[Index];
> +    for (Byte = 0; Byte < sizeof (mIdtEntryTemplate); Byte++) {
> +      Dst[Byte] = Src[Byte];
> +    }
> +  }
> +
> +  IdtDescriptor.Base  = (UINTN)&IdtTableInStack.IdtTable;
> +  IdtDescriptor.Limit = (UINT16)(sizeof (IdtTableInStack.IdtTable) - 1);
> +
> +  if (SevEsIsEnabled ()) {
> +    SevEsProtocolCheck ();
> +
> +    //
> +    // For SEV-ES guests, the exception handler is needed before calling
> +    // ProcessLibraryConstructorList() because some of the library constructors
> +    // perform some functions that result in #VC exceptions being generated.
> +    //
> +    // Due to this code executing before library constructors, *all* library
> +    // API calls are theoretically interface contract violations. However,
> +    // because this is SEC (executing in flash), those constructors cannot
> +    // write variables with static storage duration anyway. Furthermore, only
> +    // a small, restricted set of APIs, such as AsmWriteIdtr() and
> +    // InitializeCpuExceptionHandlers(), are called, where we require that the
> +    // underlying library not require constructors to have been invoked and
> +    // that the library instance not trigger any #VC exceptions.
> +    //
> +    AsmWriteIdtr (&IdtDescriptor);
> +    InitializeCpuExceptionHandlers (NULL);
> +  }
> +
>    ProcessLibraryConstructorList (NULL, NULL);
>  
> +  if (!SevEsIsEnabled ()) {
> +    //
> +    // For non SEV-ES guests, just load the IDTR.
> +    //
> +    AsmWriteIdtr (&IdtDescriptor);
> +  }
> +
>    DEBUG ((EFI_D_INFO,
>      "SecCoreStartupWithStack(0x%x, 0x%x)\n",
>      (UINT32)(UINTN)BootFv,
> @@ -751,19 +889,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
>