Re: [PATCH v7 08/43] UefiCpuPkg: Implement library support for VMGEXIT

["Dong, Eric" <eric.dong@intel.com>]

Hi Tom,

> -----Original Message-----
> From: Tom Lendacky <thomas.lendacky@amd.com>
> Sent: Thursday, April 23, 2020 1:41 AM
> To: devel@edk2.groups.io
> Cc: Justen, Jordan L <jordan.l.justen@intel.com>; Laszlo Ersek
> <lersek@redhat.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>; Kinney,
> Michael D <michael.d.kinney@intel.com>; Gao, Liming
> <liming.gao@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ray
> <ray.ni@intel.com>; Brijesh Singh <brijesh.singh@amd.com>
> Subject: [PATCH v7 08/43] UefiCpuPkg: Implement library support for
> VMGEXIT
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2198
> 
> To support issuing a VMGEXIT instruction, create a library that can be used to
> perform GHCB and VMGEXIT related operations and to issue the actual
> VMGEXIT instruction when using the GHCB.
> 
> Additionally, two VMGEXIT / MMIO related functions are created to support
> flash emulation. Flash emulation currently is done by marking the flash area
> as read-only and taking a nested page fault to perform the emulation of the
> instruction. However, emulation cannot be performed because there is no
> instruction decode assist support when SEV-ES is enabled. Provide routines
> to initiate an MMIO request to perform actual writes to flash.
> 
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Acked-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
>  UefiCpuPkg/UefiCpuPkg.dec                    |   3 +
>  UefiCpuPkg/UefiCpuPkg.dsc                    |   2 +
>  UefiCpuPkg/Library/VmgExitLib/VmgExitLib.inf |  33 +++
>  UefiCpuPkg/Include/Library/VmgExitLib.h      | 117 ++++++++
>  UefiCpuPkg/Library/VmgExitLib/VmgExitLib.c   | 293
> +++++++++++++++++++
>  UefiCpuPkg/Library/VmgExitLib/VmgExitLib.uni |  15 +
>  6 files changed, 463 insertions(+)
>  create mode 100644 UefiCpuPkg/Library/VmgExitLib/VmgExitLib.inf
>  create mode 100644 UefiCpuPkg/Include/Library/VmgExitLib.h
>  create mode 100644 UefiCpuPkg/Library/VmgExitLib/VmgExitLib.c
>  create mode 100644 UefiCpuPkg/Library/VmgExitLib/VmgExitLib.uni
> 
> diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
> index df5d02bae6b4..cb92f34b6f55 100644
> --- a/UefiCpuPkg/UefiCpuPkg.dec
> +++ b/UefiCpuPkg/UefiCpuPkg.dec
> @@ -53,6 +53,9 @@ [LibraryClasses.IA32, LibraryClasses.X64]
>    ##
>    MpInitLib|Include/Library/MpInitLib.h
> 
> +  ##  @libraryclass  Provides function to support VMGEXIT processing.
> +  VmgExitLib|Include/Library/VmgExitLib.h
> +
>  [Guids]
>    gUefiCpuPkgTokenSpaceGuid      = { 0xac05bf33, 0x995a, 0x4ed4, { 0xaa,
> 0xb8, 0xef, 0x7a, 0xe8, 0xf, 0x5c, 0xb0 }}
>    gMsegSmramGuid                 = { 0x5802bce4, 0xeeee, 0x4e33, { 0xa1, 0x30,
> 0xeb, 0xad, 0x27, 0xf0, 0xe4, 0x39 }}
> diff --git a/UefiCpuPkg/UefiCpuPkg.dsc b/UefiCpuPkg/UefiCpuPkg.dsc index
> d28cb5cccb52..997840452218 100644
> --- a/UefiCpuPkg/UefiCpuPkg.dsc
> +++ b/UefiCpuPkg/UefiCpuPkg.dsc
> @@ -56,6 +56,7 @@ [LibraryClasses]
> 
> PeCoffGetEntryPointLib|MdePkg/Library/BasePeCoffGetEntryPointLib/Base
> PeCoffGetEntryPointLib.inf
> 
> PeCoffExtraActionLib|MdePkg/Library/BasePeCoffExtraActionLibNull/BaseP
> eCoffExtraActionLibNull.inf
> 
> TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/Tp
> mMeasurementLibNull.inf
> +  VmgExitLib|UefiCpuPkg/Library/VmgExitLib/VmgExitLib.inf
> 
>  [LibraryClasses.common.SEC]
> 
> PlatformSecLib|UefiCpuPkg/Library/PlatformSecLibNull/PlatformSecLibNull.i
> nf
> @@ -136,6 +137,7 @@ [Components.IA32, Components.X64]
> 
> UefiCpuPkg/Library/SmmCpuPlatformHookLibNull/SmmCpuPlatformHookLib
> Null.inf
>    UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
>    UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf
> +  UefiCpuPkg/Library/VmgExitLib/VmgExitLib.inf
>    UefiCpuPkg/PiSmmCommunication/PiSmmCommunicationPei.inf
>    UefiCpuPkg/PiSmmCommunication/PiSmmCommunicationSmm.inf
>    UefiCpuPkg/SecCore/SecCore.inf
> diff --git a/UefiCpuPkg/Library/VmgExitLib/VmgExitLib.inf
> b/UefiCpuPkg/Library/VmgExitLib/VmgExitLib.inf
> new file mode 100644
> index 000000000000..6acfa779e75a
> --- /dev/null
> +++ b/UefiCpuPkg/Library/VmgExitLib/VmgExitLib.inf
> @@ -0,0 +1,33 @@
> +## @file
> +#  VMGEXIT Support Library.
> +#
> +#  Copyright (c) 2019, 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
> +  FILE_GUID                      = 3cd7368f-ef9b-4a9b-9571-2ed93813677e
> +  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           = IA32 X64
> +#
> +
> +[Sources]
> +  VmgExitLib.c
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +  UefiCpuPkg/UefiCpuPkg.dec
> +
> +[LibraryClasses]
> +  BaseLib
> +
> diff --git a/UefiCpuPkg/Include/Library/VmgExitLib.h
> b/UefiCpuPkg/Include/Library/VmgExitLib.h
> new file mode 100644
> index 000000000000..3bf05bebd326
> --- /dev/null
> +++ b/UefiCpuPkg/Include/Library/VmgExitLib.h
> @@ -0,0 +1,117 @@
> +/** @file
> +  Public header file for the VMGEXIT Support library class.
> +
> +  This library class defines some routines used when invoking the
> + VMGEXIT  instruction in support of SEV-ES.
> +
> +  Copyright (c) 2019, Advanced Micro Devices, Inc. All rights
> + reserved.<BR>
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#ifndef __VMG_EXIT_LIB_H__
> +#define __VMG_EXIT_LIB_H__
> +
> +#include <Register/Amd/Ghcb.h>
> +
> +
> +/**
> +  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
> +                              number to be propagated.
> +
> +**/
> +UINT64
> +EFIAPI
> +VmgExit (
> +  IN OUT GHCB                *Ghcb,
> +  IN     UINT64              ExitCode,
> +  IN     UINT64              ExitInfo1,
> +  IN     UINT64              ExitInfo2
> +  );
> +
> +/**
> +  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
> +  );
> +
> +/**
> +  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
> +  );
> +
> +#define VMGMMIO_READ   False
> +#define VMGMMIO_WRITE  True
> +
> +/**
> +  Perform MMIO write of a buffer to a non-MMIO marked range.
> +
> +  Performs an MMIO write without taking a #VC. This is useful  for
> + Flash devices, which are marked read-only.
> +
> +  @param[in, out]  Dest       A pointer to the destination buffer
> +  @param[in]       Src        A pointer to the source data to be written
> +  @param[in]       Bytes      Number of bytes to write
> +
> +**/
> +VOID
> +EFIAPI
> +VmgMmioWrite (
> +  IN OUT UINT8               *Dest,
> +  IN     UINT8               *Src,
> +  IN     UINTN                Bytes
> +  );
> +
> +/**
> +  Issue the GHCB set AP Jump Table VMGEXIT.
> +
> +  Performs a VMGEXIT using the GHCB AP Jump Table exit code to save the
> + AP Jump Table address with the hypervisor for retrieval at a later time.
> +
> +  @param[in]  Address  Physical address of the AP Jump Table
> +
> +  @retval  0           VMGEXIT succeeded.
> +  @retval  Others      VMGEXIT processing did not succeed. Exception
> +                       number to be propagated.
> +
> +**/
> +UINT64
> +EFIAPI
> +VmgExitSetAPJumpTable (
> +  IN EFI_PHYSICAL_ADDRESS  Address
> +  );

I think above two APIs should not been added to this library, they are not the basic actions for VmgExit. 
Remove these two APIs will make the library more stable.

Also, I check all the code in this patch series, only one caller for each API, so I think we can directly
move these codes to the caller. If later more and more callers need to use these two APIs, we can
create another service library to convenient the callers.

I ignore all the coding style related issues in this patch because I assume you have passed ECC
checks in your new patches.

Thanks,
Eric
> +
> +#endif
> diff --git a/UefiCpuPkg/Library/VmgExitLib/VmgExitLib.c
> b/UefiCpuPkg/Library/VmgExitLib/VmgExitLib.c
> new file mode 100644
> index 000000000000..6137b1a0eb64
> --- /dev/null
> +++ b/UefiCpuPkg/Library/VmgExitLib/VmgExitLib.c
> @@ -0,0 +1,293 @@
> +/** @file
> +  VMGEXIT Support Library.
> +
> +  Copyright (c) 2019, 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 <Register/Amd/Ghcb.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
> +    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
> +        Status = Event.Uint64;
> +      }
> +    }
> +  }
> +
> +  if (Status == 0) {
> +    GHCB_EVENT_INJECTION  Event;
> +
> +    Event.Uint64 = 0;
> +    Event.Elements.Vector = GP_EXCEPTION;
> +    Event.Elements.Type   = GHCB_EVENT_INJECTION_TYPE_EXCEPTION;
> +    Event.Elements.Valid  = 1;
> +
> +    Status = Event.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
> +                              number 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
> +  )
> +{
> +}
> +
> +/**
> +  Perform VMGEXIT MMIO read or write.
> +
> +  Performs the requested MMIO read or write using the VMGEXIT
> instruction.
> +
> +  For an MMIO read, the data that has been read during the VMGEXIT is
> + placed in  the SharedBuffer area of the GHCB. This is then copied to
> + the actual  destination buffer within the guest.
> +
> +  For an MMIO write, the data to be written is copied into the
> + SharedBuffer area  of the GHCB by the guest. This is then copied to
> + the actual destination buffer  by the hypervisor during the VMGEXIT.
> +
> +  @param[in, out]  MmioAddress  A pointer to the MMIO buffer to be
> read/written
> +  @param[in, out]  Buffer       A pointer to the buffer to hold the data thas
> +                                has been read or hold the data to be written
> +  @param[in]       Bytes        Number of bytes to read or write
> +  @param[in]       Write        If set, the request is for an MMIO write, else
> +                                it is an MMIO read.
> +
> +  @retval  0                    VMGEXIT succeeded.
> +  @retval  Others               VMGEXIT processing did not succeed. Exception
> +                                number to be propagated.
> +
> +**/
> +STATIC
> +UINT64
> +EFIAPI
> +VmgMmio (
> +  IN OUT UINT8               *MmioAddress,
> +  IN OUT UINT8               *Buffer,
> +  IN     UINTN               Bytes,
> +  IN     BOOLEAN             Write
> +  )
> +{
> +  UINT64                    MmioOp, ExitInfo1, ExitInfo2, Status;
> +  GHCB                      *Ghcb;
> +  MSR_SEV_ES_GHCB_REGISTER  Msr;
> +
> +  Msr.GhcbPhysicalAddress = AsmReadMsr64 (MSR_SEV_ES_GHCB);  Ghcb
> =
> + Msr.Ghcb;
> +
> +  //
> +  // This function is about to set fields in the GHCB. Do not execute
> + // anything that will cause a #VC before issuing the VmgExit(). Any
> + #VC  // will result in all GHCB settings being overwritten (this
> + means, e.g.,  // do not add DEBUG() statements).
> +  //
> +  VmgInit (Ghcb);
> +
> +  if (Write) {
> +    MmioOp = SvmExitMmioWrite;
> +  } else {
> +    MmioOp = SvmExitMmioRead;
> +  }
> +
> +  ExitInfo1 = (UINT64) (UINTN) MmioAddress;
> +  ExitInfo2 = Bytes;
> +
> +  if (Write) {
> +    CopyMem (Ghcb->SharedBuffer, Buffer, Bytes);  }
> +
> +  Ghcb->SaveArea.SwScratch = (UINT64) (UINTN) Ghcb->SharedBuffer;
> + Status = VmgExit (Ghcb, MmioOp, ExitInfo1, ExitInfo2);  if (Status !=
> + 0) {
> +    return Status;
> +  }
> +
> +  if (!Write) {
> +    CopyMem (Buffer, Ghcb->SharedBuffer, Bytes);  }
> +
> +  VmgDone (Ghcb);
> +
> +  return 0;
> +}
> +
> +/**
> +  Perform MMIO write of a buffer to a non-MMIO marked range.
> +
> +  Performs an MMIO write without taking a #VC. This is useful  for
> + Flash devices, which are marked read-only.
> +
> +  @param[in, out]  Dest       A pointer to the destination buffer
> +  @param[in]       Src        A pointer to the source data to be written
> +  @param[in]       Bytes      Number of bytes to write
> +
> +**/
> +VOID
> +EFIAPI
> +VmgMmioWrite (
> +  IN OUT UINT8               *Dest,
> +  IN     UINT8               *Src,
> +  IN     UINTN                Bytes
> +  )
> +{
> +  VmgMmio (Dest, Src, Bytes, TRUE);
> +}
> +
> +/**
> +  Issue the GHCB set AP Jump Table VMGEXIT.
> +
> +  Performs a VMGEXIT using the GHCB AP Jump Table exit code to save the
> + AP Jump Table address with the hypervisor for retrieval at a later time.
> +
> +  @param[in]  Address  Physical address of the AP Jump Table
> +
> +  @retval  0           VMGEXIT succeeded.
> +  @retval  Others      VMGEXIT processing did not succeed. Exception
> +                       number to be propagated.
> +
> +**/
> +UINT64
> +EFIAPI
> +VmgExitSetAPJumpTable (
> +  IN EFI_PHYSICAL_ADDRESS  Address
> +  )
> +{
> +  UINT64                    ExitInfo1, ExitInfo2, Status;
> +  GHCB                      *Ghcb;
> +  MSR_SEV_ES_GHCB_REGISTER  Msr;
> +
> +  Msr.GhcbPhysicalAddress = AsmReadMsr64 (MSR_SEV_ES_GHCB);  Ghcb
> =
> + Msr.Ghcb;
> +
> +  VmgInit (Ghcb);
> +
> +  ExitInfo1 = 0;
> +  ExitInfo2 = (UINT64) (UINTN) Address;
> +
> +  Status = VmgExit (Ghcb, SvmExitApJumpTable, ExitInfo1, ExitInfo2);
> +
> +  VmgDone (Ghcb);
> +
> +  return Status;
> +}
> +
> diff --git a/UefiCpuPkg/Library/VmgExitLib/VmgExitLib.uni
> b/UefiCpuPkg/Library/VmgExitLib/VmgExitLib.uni
> new file mode 100644
> index 000000000000..e8656aae4726
> --- /dev/null
> +++ b/UefiCpuPkg/Library/VmgExitLib/VmgExitLib.uni
> @@ -0,0 +1,15 @@
> +// /** @file
> +// VMGEXIT support library instance.
> +//
> +// VMGEXIT support library instance.
> +//
> +// Copyright (c) 2019, Advanced Micro Devices, Inc. All rights
> +reserved.<BR> // SPDX-License-Identifier: BSD-2-Clause-Patent // // **/
> +
> +
> +#string STR_MODULE_ABSTRACT             #language en-US "VMGEXIT
> Support Library."
> +
> +#string STR_MODULE_DESCRIPTION          #language en-US "VMGEXIT
> Support Library."
> +
> --
> 2.17.1