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

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

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.

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

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

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

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

With these addressed:

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

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