Re: [edk2-devel] [PATCH v8 12/46] OvmfPkg/VmgExitLib: Implement library support for VmgExitLib in OVMF

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

On 5/21/20 11:52 AM, Laszlo Ersek wrote:
> On 05/19/20 23:50, Lendacky, Thomas wrote:
>> The base VmgExitLib library provides a default limited interface. As it
>> does not provide full support, create an OVMF version of this library to
>> begin the process of providing full support of SEV-ES within OVMF.
>>
>> SEV-ES support is only provided for X64 builds, so only OvmfPkgX64.dsc is
>> updated to make use of the OvmfPkg version of the library.
>>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>> ---
>>   OvmfPkg/OvmfPkgX64.dsc                        |   2 +-
>>   OvmfPkg/Library/VmgExitLib/VmgExitLib.inf     |  36 ++++
>>   OvmfPkg/Library/VmgExitLib/VmgExitLib.c       | 155 ++++++++++++++++++
>>   .../Library/VmgExitLib/X64/VmgExitVcHandler.c |  81 +++++++++
>>   OvmfPkg/Library/VmgExitLib/VmgExitLib.uni     |  15 ++
> 
> (1) Please drop the UNI file. UNI files are needed (to my understanding)
> with UPT (UEFI Packaging Tool), but OvmfPkg content is not distributed
> like that. So UNI files would only be a distraction under OvmfPkg.

Will do.

> 
>>   5 files changed, 288 insertions(+), 1 deletion(-)
>>   create mode 100644 OvmfPkg/Library/VmgExitLib/VmgExitLib.inf
>>   create mode 100644 OvmfPkg/Library/VmgExitLib/VmgExitLib.c
>>   create mode 100644 OvmfPkg/Library/VmgExitLib/X64/VmgExitVcHandler.c
>>   create mode 100644 OvmfPkg/Library/VmgExitLib/VmgExitLib.uni
>>
>> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
>> index 0b9189ab1e38..b5f3859420d0 100644
>> --- a/OvmfPkg/OvmfPkgX64.dsc
>> +++ b/OvmfPkg/OvmfPkgX64.dsc
>> @@ -232,7 +232,7 @@ [LibraryClasses]
>>   
>>   [LibraryClasses.common]
>>     BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
>> -  VmgExitLib|UefiCpuPkg/Library/VmgExitLibNull/VmgExitLibNull.inf
>> +  VmgExitLib|OvmfPkg/Library/VmgExitLib/VmgExitLib.inf
>>   
>>   [LibraryClasses.common.SEC]
>>     TimerLib|OvmfPkg/Library/AcpiTimerLib/BaseRomAcpiTimerLib.inf
>> diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitLib.inf b/OvmfPkg/Library/VmgExitLib/VmgExitLib.inf
>> new file mode 100644
>> index 000000000000..0e6bc8432314
>> --- /dev/null
>> +++ b/OvmfPkg/Library/VmgExitLib/VmgExitLib.inf
>> @@ -0,0 +1,36 @@
>> +## @file
>> +#  VMGEXIT Support Library.
>> +#
>> +#  Copyright (C) 2020, Advanced Micro Devices, Inc. All rights reserved.<BR>
>> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
>> +#
>> +##
>> +
>> +[Defines]
>> +  INF_VERSION                    = 0x00010005
>> +  BASE_NAME                      = VmgExitLib
>> +  MODULE_UNI_FILE                = VmgExitLib.uni
> 
> (2) Please drop MODULE_UNI_FILE too, according to (1).

Will do.

> 
>> +  FILE_GUID                      = 0e923c25-13cd-430b-8714-ffe85652a97b
>> +  MODULE_TYPE                    = BASE
>> +  VERSION_STRING                 = 1.0
>> +  LIBRARY_CLASS                  = VmgExitLib
>> +
>> +#
>> +# The following information is for reference only and not required by the build tools.
>> +#
>> +#  VALID_ARCHITECTURES           = X64
>> +#
>> +
>> +[Sources.X64]
>> +  X64/VmgExitVcHandler.c
>> +
>> +[Sources.common]
>> +  VmgExitLib.c
> 
> (3) I think this split for [Sources] does not make sense under OvmfPkg.
> I'd only use one [Sources] Section, and also move VmgExitVcHandler.c out
> of the X64 subdir. The X64 subdir per se doesn't look useful either.

Will do.

> 
>> +
>> +[Packages]
>> +  MdePkg/MdePkg.dec
>> +  UefiCpuPkg/UefiCpuPkg.dec
>> +
>> +[LibraryClasses]
>> +  BaseLib
>> +
>> diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitLib.c b/OvmfPkg/Library/VmgExitLib/VmgExitLib.c
>> new file mode 100644
>> index 000000000000..7b7ebea85256
>> --- /dev/null
>> +++ b/OvmfPkg/Library/VmgExitLib/VmgExitLib.c
>> @@ -0,0 +1,155 @@
>> +/** @file
>> +  VMGEXIT Support Library.
>> +
>> +  Copyright (C) 2020, Advanced Micro Devices, Inc. All rights reserved.<BR>
>> +  SPDX-License-Identifier: BSD-2-Clause-Patent
>> +
>> +**/
>> +
>> +#include <Base.h>
>> +#include <Uefi.h>
>> +#include <Library/BaseMemoryLib.h>
>> +#include <Library/VmgExitLib.h>
>> +#include <Register/Amd/Msr.h>
>> +
>> +/**
>> +  Check for VMGEXIT error
>> +
>> +  Check if the hypervisor has returned an error after completion of the VMGEXIT
>> +  by examining the SwExitInfo1 field of the GHCB.
>> +
>> +  @param[in]  Ghcb       A pointer to the GHCB
>> +
>> +  @retval  0             VMGEXIT succeeded.
>> +  @retval  Others        VMGEXIT processing did not succeed. Exception number to
>> +                         be propagated.
>> +
>> +**/
>> +STATIC
>> +UINT64
>> +VmgExitErrorCheck (
>> +  IN GHCB                *Ghcb
>> +  )
>> +{
>> +  GHCB_EVENT_INJECTION  Event;
>> +  GHCB_EXIT_INFO        ExitInfo;
>> +  UINT64                Status;
>> +
>> +  ExitInfo.Uint64 = Ghcb->SaveArea.SwExitInfo1;
>> +  ASSERT ((ExitInfo.Elements.Lower32Bits == 0) ||
>> +          (ExitInfo.Elements.Lower32Bits == 1));
>> +
>> +  Status = 0;
>> +  if (ExitInfo.Elements.Lower32Bits == 0) {
>> +    return Status;
>> +  }
>> +
>> +  if (ExitInfo.Elements.Lower32Bits == 1) {
>> +    ASSERT (Ghcb->SaveArea.SwExitInfo2 != 0);
>> +
>> +    // Check that the return event is valid
> 
> (4) Please prepend and append empty "//" lines.

Will do.

> 
>> +    Event.Uint64 = Ghcb->SaveArea.SwExitInfo2;
>> +    if (Event.Elements.Valid &&
>> +        Event.Elements.Type == GHCB_EVENT_INJECTION_TYPE_EXCEPTION) {
>> +      switch (Event.Elements.Vector) {
>> +      case GP_EXCEPTION:
>> +      case UD_EXCEPTION:
>> +        // Use returned event as return code
> 
> (5) Same as (4).

Will do.

> 
> With these addressed:
> 
> Acked-by: Laszlo Ersek <lersek@redhat.com>

Thanks!
Tom

> 
> Thanks,
> Laszlo
> 
>> +        Status = Event.Uint64;
>> +      }
>> +    }
>> +  }
>> +
>> +  if (Status == 0) {
>> +    GHCB_EVENT_INJECTION  GpEvent;
>> +
>> +    GpEvent.Uint64 = 0;
>> +    GpEvent.Elements.Vector = GP_EXCEPTION;
>> +    GpEvent.Elements.Type   = GHCB_EVENT_INJECTION_TYPE_EXCEPTION;
>> +    GpEvent.Elements.Valid  = 1;
>> +
>> +    Status = GpEvent.Uint64;
>> +  }
>> +
>> +  return Status;
>> +}
>> +
>> +/**
>> +  Perform VMGEXIT.
>> +
>> +  Sets the necessary fields of the GHCB, invokes the VMGEXIT instruction and
>> +  then handles the return actions.
>> +
>> +  @param[in, out]  Ghcb       A pointer to the GHCB
>> +  @param[in]       ExitCode   VMGEXIT code to be assigned to the SwExitCode
>> +                              field of the GHCB.
>> +  @param[in]       ExitInfo1  VMGEXIT information to be assigned to the
>> +                              SwExitInfo1 field of the GHCB.
>> +  @param[in]       ExitInfo2  VMGEXIT information to be assigned to the
>> +                              SwExitInfo2 field of the GHCB.
>> +
>> +  @retval  0                  VMGEXIT succeeded.
>> +  @retval  Others             VMGEXIT processing did not succeed. Exception
>> +                              event to be propagated.
>> +
>> +**/
>> +UINT64
>> +EFIAPI
>> +VmgExit (
>> +  IN OUT GHCB                *Ghcb,
>> +  IN     UINT64              ExitCode,
>> +  IN     UINT64              ExitInfo1,
>> +  IN     UINT64              ExitInfo2
>> +  )
>> +{
>> +  Ghcb->SaveArea.SwExitCode = ExitCode;
>> +  Ghcb->SaveArea.SwExitInfo1 = ExitInfo1;
>> +  Ghcb->SaveArea.SwExitInfo2 = ExitInfo2;
>> +
>> +  //
>> +  // Guest memory is used for the guest-hypervisor communication, so fence
>> +  // the invocation of the VMGEXIT instruction to ensure GHCB accesses are
>> +  // synchronized properly.
>> +  //
>> +  MemoryFence ();
>> +  AsmVmgExit ();
>> +  MemoryFence ();
>> +
>> +  return VmgExitErrorCheck (Ghcb);
>> +}
>> +
>> +/**
>> +  Perform pre-VMGEXIT initialization/preparation.
>> +
>> +  Performs the necessary steps in preparation for invoking VMGEXIT. Must be
>> +  called before setting any fields within the GHCB.
>> +
>> +  @param[in, out]  Ghcb       A pointer to the GHCB
>> +
>> +**/
>> +VOID
>> +EFIAPI
>> +VmgInit (
>> +  IN OUT GHCB                *Ghcb
>> +  )
>> +{
>> +  SetMem (&Ghcb->SaveArea, sizeof (Ghcb->SaveArea), 0);
>> +}
>> +
>> +/**
>> +  Perform post-VMGEXIT cleanup.
>> +
>> +  Performs the necessary steps to cleanup after invoking VMGEXIT. Must be
>> +  called after obtaining needed fields within the GHCB.
>> +
>> +  @param[in, out]  Ghcb       A pointer to the GHCB
>> +
>> +**/
>> +VOID
>> +EFIAPI
>> +VmgDone (
>> +  IN OUT GHCB                *Ghcb
>> +  )
>> +{
>> +}
>> +
>> diff --git a/OvmfPkg/Library/VmgExitLib/X64/VmgExitVcHandler.c b/OvmfPkg/Library/VmgExitLib/X64/VmgExitVcHandler.c
>> new file mode 100644
>> index 000000000000..036f030d6b34
>> --- /dev/null
>> +++ b/OvmfPkg/Library/VmgExitLib/X64/VmgExitVcHandler.c
>> @@ -0,0 +1,81 @@
>> +/** @file
>> +  X64 #VC Exception Handler functon.
>> +
>> +  Copyright (C) 2020, Advanced Micro Devices, Inc. All rights reserved.<BR>
>> +  SPDX-License-Identifier: BSD-2-Clause-Patent
>> +
>> +**/
>> +
>> +#include <Base.h>
>> +#include <Uefi.h>
>> +#include <Library/BaseMemoryLib.h>
>> +#include <Library/VmgExitLib.h>
>> +#include <Register/Amd/Msr.h>
>> +
>> +/**
>> +  Handle a #VC exception.
>> +
>> +  Performs the necessary processing to handle a #VC exception.
>> +
>> +  @param[in, out]  ExceptionType  Pointer to an EFI_EXCEPTION_TYPE to be set
>> +                                  as value to use on error.
>> +  @param[in, out]  SystemContext  Pointer to EFI_SYSTEM_CONTEXT
>> +
>> +  @retval  EFI_SUCCESS            Exception handled
>> +  @retval  EFI_UNSUPPORTED        #VC not supported, (new) exception value to
>> +                                  propagate provided
>> +  @retval  EFI_PROTOCOL_ERROR     #VC handling failed, (new) exception value to
>> +                                  propagate provided
>> +
>> +**/
>> +EFI_STATUS
>> +EFIAPI
>> +VmgExitHandleVc (
>> +  IN OUT EFI_EXCEPTION_TYPE  *ExceptionType,
>> +  IN OUT EFI_SYSTEM_CONTEXT  SystemContext
>> +  )
>> +{
>> +  MSR_SEV_ES_GHCB_REGISTER  Msr;
>> +  EFI_SYSTEM_CONTEXT_X64    *Regs;
>> +  GHCB                      *Ghcb;
>> +  UINT64                    ExitCode, Status;
>> +  EFI_STATUS                VcRet;
>> +
>> +  VcRet = EFI_SUCCESS;
>> +
>> +  Msr.GhcbPhysicalAddress = AsmReadMsr64 (MSR_SEV_ES_GHCB);
>> +  ASSERT (Msr.GhcbInfo.Function == 0);
>> +  ASSERT (Msr.Ghcb != 0);
>> +
>> +  Regs = SystemContext.SystemContextX64;
>> +  Ghcb = Msr.Ghcb;
>> +
>> +  VmgInit (Ghcb);
>> +
>> +  ExitCode = Regs->ExceptionData;
>> +  switch (ExitCode) {
>> +  default:
>> +    Status = VmgExit (Ghcb, SVM_EXIT_UNSUPPORTED, ExitCode, 0);
>> +    if (Status == 0) {
>> +      Regs->ExceptionData = 0;
>> +      *ExceptionType = GP_EXCEPTION;
>> +    } else {
>> +      GHCB_EVENT_INJECTION  Event;
>> +
>> +      Event.Uint64 = Status;
>> +      if (Event.Elements.ErrorCodeValid) {
>> +        Regs->ExceptionData = Event.Elements.ErrorCode;
>> +      } else {
>> +        Regs->ExceptionData = 0;
>> +      }
>> +
>> +      *ExceptionType = Event.Elements.Vector;
>> +    }
>> +
>> +    VcRet = EFI_PROTOCOL_ERROR;
>> +  }
>> +
>> +  VmgDone (Ghcb);
>> +
>> +  return VcRet;
>> +}
>> diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitLib.uni b/OvmfPkg/Library/VmgExitLib/VmgExitLib.uni
>> new file mode 100644
>> index 000000000000..a919b484c319
>> --- /dev/null
>> +++ b/OvmfPkg/Library/VmgExitLib/VmgExitLib.uni
>> @@ -0,0 +1,15 @@
>> +// /** @file
>> +// VMGEXIT support library instance.
>> +//
>> +// VMGEXIT support library instance.
>> +//
>> +// Copyright (C) 2020, Advanced Micro Devices, Inc. All rights reserved.<BR>
>> +// SPDX-License-Identifier: BSD-2-Clause-Patent
>> +//
>> +// **/
>> +
>> +
>> +#string STR_MODULE_ABSTRACT             #language en-US "OVMF VMGEXIT Support Library."
>> +
>> +#string STR_MODULE_DESCRIPTION          #language en-US "OVMF VMGEXIT Support Library."
>> +
>>
>