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

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

On 3/3/20 7:29 AM, Laszlo Ersek wrote:
> Hi Tom,
> 
> On 03/03/20 00:07, 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%7C3fd18aafb8934669131408d7bf76e70a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637188389716613512&sdata=uGe95zCVx0egVqiWYDqpAeS3k5aO5ZU6eHiyx8fpKxo%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>
>> 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.

Sorry about that, missed that when I made this change.

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

Correct.

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

Will fix.

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

Will change this.

> 
> With the style warts (1) and (2) fixed:
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks,
Tom

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