Re: [edk2-devel] [RFC PATCH v3 07/43] UefiCpuPkg: Implement library support for VMGEXIT

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

On 11/20/19 21:06, Lendacky, Thomas wrote:
> 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>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
>  UefiCpuPkg/UefiCpuPkg.dec                    |   3 +
>  UefiCpuPkg/UefiCpuPkg.dsc                    |   5 +
>  UefiCpuPkg/Library/VmgExitLib/VmgExitLib.inf |  33 +++++
>  UefiCpuPkg/Include/Library/VmgExitLib.h      |  96 ++++++++++++++
>  UefiCpuPkg/Library/VmgExitLib/VmgExitLib.c   | 132 +++++++++++++++++++
>  UefiCpuPkg/Library/VmgExitLib/VmgExitLib.uni |  15 +++
>  6 files changed, 284 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 12f4413ea5b0..90feb9166dc8 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..5ab7e423e8ab 100644
> --- a/UefiCpuPkg/UefiCpuPkg.dsc
> +++ b/UefiCpuPkg/UefiCpuPkg.dsc
> @@ -63,6 +63,7 @@ [LibraryClasses.common.SEC]
>    HobLib|MdePkg/Library/PeiHobLib/PeiHobLib.inf
>    PeiServicesTablePointerLib|MdePkg/Library/PeiServicesTablePointerLibIdt/PeiServicesTablePointerLibIdt.inf
>    MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/PeiMemoryAllocationLib.inf
> +  VmgExitLib|UefiCpuPkg/Library/VmgExitLib/VmgExitLib.inf
>  
>  [LibraryClasses.common.PEIM]
>    MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/PeiMemoryAllocationLib.inf
> @@ -74,6 +75,7 @@ [LibraryClasses.common.PEIM]
>  [LibraryClasses.IA32.PEIM, LibraryClasses.X64.PEIM]
>    PeiServicesTablePointerLib|MdePkg/Library/PeiServicesTablePointerLibIdt/PeiServicesTablePointerLibIdt.inf
>    CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
> +  VmgExitLib|UefiCpuPkg/Library/VmgExitLib/VmgExitLib.inf
>  
>  [LibraryClasses.common.DXE_DRIVER]
>    MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
> @@ -81,12 +83,14 @@ [LibraryClasses.common.DXE_DRIVER]
>    CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf
>    MpInitLib|UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
>    RegisterCpuFeaturesLib|UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.inf
> +  VmgExitLib|UefiCpuPkg/Library/VmgExitLib/VmgExitLib.inf
>  
>  [LibraryClasses.common.DXE_SMM_DRIVER]
>    SmmServicesTableLib|MdePkg/Library/SmmServicesTableLib/SmmServicesTableLib.inf
>    MemoryAllocationLib|MdePkg/Library/SmmMemoryAllocationLib/SmmMemoryAllocationLib.inf
>    HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
>    CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/SmmCpuExceptionHandlerLib.inf
> +  VmgExitLib|UefiCpuPkg/Library/VmgExitLib/VmgExitLib.inf
>  
>  [LibraryClasses.common.UEFI_APPLICATION]
>    UefiApplicationEntryPoint|MdePkg/Library/UefiApplicationEntryPoint/UefiApplicationEntryPoint.inf
> @@ -136,6 +140,7 @@ [Components.IA32, Components.X64]
>    UefiCpuPkg/Library/SmmCpuPlatformHookLibNull/SmmCpuPlatformHookLibNull.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..b5639fbfa1a5
> --- /dev/null
> +++ b/UefiCpuPkg/Include/Library/VmgExitLib.h
> @@ -0,0 +1,96 @@
> +/** @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]  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 issued.
> +
> +**/
> +UINTN
> +EFIAPI
> +VmgExit (
> +  GHCB                *Ghcb,
> +  UINT64              ExitCode,
> +  UINT64              ExitInfo1,
> +  UINT64              ExitInfo2
> +  );
> +
> +/**
> +  Perform pre-VMGEXIT initialization/preparation.
> +
> +  Performs the necessary steps in preparation for invoking VMGEXIT.
> +
> +  @param[in]  GHCB       A pointer to the GHCB
> +
> +**/
> +VOID
> +EFIAPI
> +VmgInit (
> +  GHCB                *Ghcb
> +  );
> +
> +/**
> +  Perform post-VMGEXIT cleanup.
> +
> +  Performs the necessary steps to cleanup after invoking VMGEXIT.
> +
> +  @param[in]  GHCB       A pointer to the GHCB
> +
> +**/
> +VOID
> +EFIAPI
> +VmgDone (
> +  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]  UINT8      A pointer to the destination buffer
> +  @param[in]  UINTN      The immediate value to write
> +  @param[in]  UINTN      Number of bytes to write
> +
> +**/
> +VOID
> +EFIAPI
> +VmgMmioWrite (
> +  UINT8               *Dest,
> +  UINT8               *Src,
> +  UINTN                Bytes
> +  );
> +
> +#endif
> diff --git a/UefiCpuPkg/Library/VmgExitLib/VmgExitLib.c b/UefiCpuPkg/Library/VmgExitLib/VmgExitLib.c
> new file mode 100644
> index 000000000000..23965b7ff022
> --- /dev/null
> +++ b/UefiCpuPkg/Library/VmgExitLib/VmgExitLib.c
> @@ -0,0 +1,132 @@
> +/** @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>
> +
> +STATIC
> +UINTN
> +VmgExitErrorCheck (
> +  GHCB                *Ghcb
> +  )
> +{
> +  GHCB_EXIT_INFO   ExitInfo;
> +  UINTN            Reason, Action;
> +
> +  if (!Ghcb->SaveArea.SwExitInfo1) {
> +    return 0;
> +  }
> +
> +  ExitInfo.Uint64 = Ghcb->SaveArea.SwExitInfo1;
> +  Action = ExitInfo.Elements.Lower32Bits;
> +  if (Action == 1) {
> +    Reason = ExitInfo.Elements.Upper32Bits;
> +
> +    switch (Reason) {
> +    case UD_EXCEPTION:
> +    case GP_EXCEPTION:
> +      return Reason;
> +    }
> +  }
> +
> +  ASSERT (0);
> +  return GP_EXCEPTION;
> +}
> +
> +UINTN
> +EFIAPI
> +VmgExit (
> +  GHCB                *Ghcb,
> +  UINT64              ExitCode,
> +  UINT64              ExitInfo1,
> +  UINT64              ExitInfo2
> +  )
> +{
> +  Ghcb->SaveArea.SwExitCode = ExitCode;
> +  Ghcb->SaveArea.SwExitInfo1 = ExitInfo1;
> +  Ghcb->SaveArea.SwExitInfo2 = ExitInfo2;
> +  AsmVmgExit ();

This patch looks good to me (and, in general, I'd like to defer to Ray
and Eric on the UefiCpuPkg patches); just one comment:

AsmVmgExit() orchestrates guest-host communication in guest RAM, and in
that sense, it is somewhat similar to virtio. In virtio (per spec), we
use MemoryFence() calls carefully.

Now, if you check the MemoryFence() implementation in
"MdePkg/Library/BaseLib/X64/GccInline.c", it is only

  __asm__ __volatile__ ("":::"memory");

which is already part of AsmVmgExit(), from the previous patch:

  [edk2-devel] [RFC PATCH v3 06/43]
  MdePkg/BaseLib: Add support for the VMGEXIT instruction

  __asm__ __volatile__ ("rep; vmmcall":::"memory");

So, I think it's not necessary *in practice* to add a MemoryFence()
ahead of the AsmVmgExit(). But, I think it is needed afterwards.

The comment in "MdePkg/Library/BaseLib/X64/GccInline.c" says, "it is
more about the compiler that it is actually processor synchronization".
With that in mind, I'd like to suggest one of two alternatives:

- modify the AsmVmgExit() documentation (function level comment in the
lib class header) so that it spell out that all barriers (before &
after) are contained within,

- or please modify the call site so that it is both preceded and
succeeded by MemoryFence().

With either option implemented:

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

Thanks
Laszlo



> +
> +  return VmgExitErrorCheck (Ghcb);
> +}
> +
> +VOID
> +EFIAPI
> +VmgInit (
> +  GHCB                *Ghcb
> +  )
> +{
> +  SetMem (&Ghcb->SaveArea, sizeof (Ghcb->SaveArea), 0);
> +}
> +
> +VOID
> +EFIAPI
> +VmgDone (
> +  GHCB                *Ghcb
> +  )
> +{
> +}
> +
> +UINTN
> +EFIAPI
> +VmgMmio (
> +  UINT8               *MmioAddress,
> +  UINT8               *Buffer,
> +  UINTN               Bytes,
> +  BOOLEAN             Write
> +  )
> +{
> +  UINT64                    MmioOp;
> +  UINT64                    ExitInfo1, ExitInfo2;
> +  UINTN                     Status;
> +  GHCB                      *Ghcb;
> +  MSR_SEV_ES_GHCB_REGISTER  Msr;
> +
> +  Msr.GhcbPhysicalAddress = AsmReadMsr64 (MSR_SEV_ES_GHCB);
> +  Ghcb = Msr.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);
> +  }
> +
> +  return 0;
> +}
> +
> +VOID
> +EFIAPI
> +VmgMmioWrite (
> +  UINT8               *Dest,
> +  UINT8               *Src,
> +  UINTN                Bytes
> +  )
> +{
> +  VmgMmio (Dest, Src, Bytes, TRUE);
> +}
> +
> 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."
> +
>