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

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

On 11/21/19 5:15 AM, Laszlo Ersek via Groups.Io wrote:
> 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().

I'll update VmgExit() in VmgExitLib.c to add the MemoryFence() calls
around the invocation of AsmVmgExit().

Thanks,
Tom

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