Re: [PATCH v4 1/4] OvmfPkg/MemEncryptHypercallLib: add library to support SEV hypercalls.

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

On 6/21/21 8:56 AM, Ashish Kalra wrote:
> From: Ashish Kalra <ashish.kalra@amd.com>
> 
> Add SEV and SEV-ES hypercall abstraction library to support SEV Page
> encryption/deceryption status hypercalls for SEV and SEV-ES guests.

Does this have to be a new library? It's just a single function and so I
would think it could live in the BaseMemEncryptSevLib library where the
change to the c-bit is being done anyway.

> 
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> 
> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
> ---
>  Maintainers.txt                                                      |   2 +
>  OvmfPkg/Include/Library/MemEncryptHypercallLib.h                     |  43 ++++++++
>  OvmfPkg/Library/MemEncryptHypercallLib/Ia32/MemEncryptHypercallLib.c |  37 +++++++
>  OvmfPkg/Library/MemEncryptHypercallLib/MemEncryptHypercallLib.inf    |  42 ++++++++
>  OvmfPkg/Library/MemEncryptHypercallLib/X64/AsmHelperStub.nasm        |  28 ++++++
>  OvmfPkg/Library/MemEncryptHypercallLib/X64/MemEncryptHypercallLib.c  | 105 ++++++++++++++++++++
>  OvmfPkg/OvmfPkgIa32.dsc                                              |   1 +
>  OvmfPkg/OvmfPkgIa32X64.dsc                                           |   1 +
>  OvmfPkg/OvmfPkgX64.dsc                                               |   1 +
>  OvmfPkg/OvmfXen.dsc                                                  |   1 +
>  10 files changed, 261 insertions(+)
> 
> diff --git a/Maintainers.txt b/Maintainers.txt
> index ea54e0b7e9..8ecc8464ba 100644
> --- a/Maintainers.txt
> +++ b/Maintainers.txt
> @@ -449,8 +449,10 @@ F: OvmfPkg/AmdSev/
>  F: OvmfPkg/AmdSevDxe/
>  F: OvmfPkg/Include/Guid/ConfidentialComputingSecret.h
>  F: OvmfPkg/Include/Library/MemEncryptSevLib.h
> +F: OvmfPkg/Include/Library/MemEncryptHypercallLib.h
>  F: OvmfPkg/IoMmuDxe/AmdSevIoMmu.*
>  F: OvmfPkg/Library/BaseMemEncryptSevLib/
> +F: OvmfPkg/Library/MemEncryptHypercallLib/
>  F: OvmfPkg/Library/PlatformBootManagerLibGrub/
>  F: OvmfPkg/Library/VmgExitLib/
>  F: OvmfPkg/PlatformPei/AmdSev.c
> diff --git a/OvmfPkg/Include/Library/MemEncryptHypercallLib.h b/OvmfPkg/Include/Library/MemEncryptHypercallLib.h
> new file mode 100644
> index 0000000000..b241a189b6
> --- /dev/null
> +++ b/OvmfPkg/Include/Library/MemEncryptHypercallLib.h
> @@ -0,0 +1,43 @@
> +/** @file
> +
> +  Define Secure Encrypted Virtualization (SEV) hypercall library.
> +
> +  Copyright (c) 2020, AMD Incorporated. All rights reserved.<BR>
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#ifndef _MEM_ENCRYPT_HYPERCALL_LIB_H_
> +#define _MEM_ENCRYPT_HYPERCALL_LIB_H_
> +
> +#include <Base.h>
> +
> +#define KVM_HC_MAP_GPA_RANGE   12
> +#define KVM_MAP_GPA_RANGE_PAGE_SZ_4K    0

> +#define KVM_MAP_GPA_RANGE_PAGE_SZ_2M    (1 << 0)

Use the BIT0 define, e.g.:
#define KVM_MAP_GPA_RANGE_PAGE_SZ_2M  BIT0

> +#define KVM_MAP_GPA_RANGE_PAGE_SZ_1G    (1 << 1)

BIT1

Also, where are these used? Are they supposed to be part of the "Mode"
parameter, in which case the comment for Mode below is incorrect.

> +#define KVM_MAP_GPA_RANGE_ENC_STAT(n)   ((n) << 4)
> +#define KVM_MAP_GPA_RANGE_ENCRYPTED     KVM_MAP_GPA_RANGE_ENC_STAT(1)
> +#define KVM_MAP_GPA_RANGE_DECRYPTED     KVM_MAP_GPA_RANGE_ENC_STAT(0)
> +
> +/**
> + This hyercall is used to notify hypervisor when a page is marked as
> + 'decrypted' (i.e C-bit removed).
> +
> + @param[in]   PhysicalAddress       The physical address that is the start address
> +                                    of a memory region.
> + @param[in]   Length                The length of memory region
> + @param[in]   Mode                  SetCBit or ClearCBit
> +
> +**/
> +
> +VOID
> +EFIAPI
> +SetMemoryEncDecHypercall3 (
> +  IN  UINTN     PhysicalAddress,
> +  IN  UINTN     Length,
> +  IN  UINTN     Mode
> +  );
> +
> +#endif
> diff --git a/OvmfPkg/Library/MemEncryptHypercallLib/Ia32/MemEncryptHypercallLib.c b/OvmfPkg/Library/MemEncryptHypercallLib/Ia32/MemEncryptHypercallLib.c
> new file mode 100644
> index 0000000000..2e73d47ee6
> --- /dev/null
> +++ b/OvmfPkg/Library/MemEncryptHypercallLib/Ia32/MemEncryptHypercallLib.c
> @@ -0,0 +1,37 @@
> +/** @file
> +
> +  Secure Encrypted Virtualization (SEV) hypercall helper library
> +
> +  Copyright (c) 2020, AMD Incorporated. All rights reserved.<BR>
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include <Uefi.h>
> +#include <Uefi/UefiBaseType.h>
> +#include <Library/BaseLib.h>
> +
> +/**
> + This hyercall is used to notify hypervisor when a page is marked as
> + 'decrypted' (i.e C-bit removed).
> +
> + @param[in]   PhysicalAddress       The physical address that is the start address
> +                                    of a memory region.
> + @param[in]   Length                The length of memory region
> + @param[in]   Mode                  SetCBit or ClearCBit

This isn't actually SetCBit or ClearCBit based on how I see it used below.

> +
> +**/
> +
> +VOID
> +EFIAPI
> +SetMemoryEncDecHypercall3 (
> +  IN PHYSICAL_ADDRESS PhysicalAddress,
> +  IN UINTN            Pages,

This doesn't match the comment above.

> +  IN UINTN            Mode
> +  )
> +{
> +  //
> +  // Memory encryption bit is not accessible in 32-bit mode
> +  //
> +}
> diff --git a/OvmfPkg/Library/MemEncryptHypercallLib/MemEncryptHypercallLib.inf b/OvmfPkg/Library/MemEncryptHypercallLib/MemEncryptHypercallLib.inf
> new file mode 100644
> index 0000000000..a77d58a7e6
> --- /dev/null
> +++ b/OvmfPkg/Library/MemEncryptHypercallLib/MemEncryptHypercallLib.inf
> @@ -0,0 +1,42 @@
> +## @file
> +#  Library provides the hypervisor helper functions for SEV guest
> +#
> +# Copyright (c) 2020 Advanced Micro Devices. All rights reserved.<BR>
> +#
> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +#
> +##
> +
> +[Defines]
> +  INF_VERSION                    = 1.25
> +  BASE_NAME                      = MemEncryptHypercallLib
> +  FILE_GUID                      = 86f2501e-f128-45f3-91c4-3cff31656ca8
> +  MODULE_TYPE                    = BASE
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = MemEncryptHypercallLib
> +
> +#
> +# The following information is for reference only and not required by the build
> +# tools.
> +#
> +# VALID_ARCHITECTURES           = IA32 X64
> +#
> +
> +[Packages]
> +  MdeModulePkg/MdeModulePkg.dec
> +  MdePkg/MdePkg.dec
> +  UefiCpuPkg/UefiCpuPkg.dec
> +  OvmfPkg/OvmfPkg.dec
> +
> +[Sources.X64]
> +  X64/MemEncryptHypercallLib.c
> +  X64/AsmHelperStub.nasm
> +
> +[Sources.IA32]
> +  Ia32/MemEncryptHypercallLib.c
> +
> +[LibraryClasses]
> +  BaseLib
> +  DebugLib
> +  VmgExitLib
> diff --git a/OvmfPkg/Library/MemEncryptHypercallLib/X64/AsmHelperStub.nasm b/OvmfPkg/Library/MemEncryptHypercallLib/X64/AsmHelperStub.nasm
> new file mode 100644
> index 0000000000..f29b96f9b0
> --- /dev/null
> +++ b/OvmfPkg/Library/MemEncryptHypercallLib/X64/AsmHelperStub.nasm
> @@ -0,0 +1,28 @@
> +DEFAULT REL
> +SECTION .text
> +
> +; VOID
> +; EFIAPI
> +; SetMemoryEncDecHypercall3AsmStub (
> +;   IN UINT HypercallNum,
> +;   IN INTN Arg1,
> +;   IN INTN Arg2,
> +;   IN INTN Arg3
> +;   );
> +global ASM_PFX(SetMemoryEncDecHypercall3AsmStub)
> +ASM_PFX(SetMemoryEncDecHypercall3AsmStub):
> +  ; UEFI calling conventions require RBX to
> +  ; be nonvolatile/callee-saved.
> +  push rbx
> +  ; Copy HypercallNumber to rax

Placing these comments on the same line will make this more readable, e.g.:

  mov rax, rcx      ; Copy HypercallNum to rax

> +  mov rax, rcx
> +  ; Copy Arg1 to the register expected by KVM
> +  mov rbx, rdx
> +  ; Copy Arg2 to register expected by KVM
> +  mov rcx, r8
> +  ; Copy Arg2 to register expected by KVM
> +  mov rdx, r9
> +  ; Call VMMCALL
> +  vmmcall
> +  pop rbx
> +  ret
> diff --git a/OvmfPkg/Library/MemEncryptHypercallLib/X64/MemEncryptHypercallLib.c b/OvmfPkg/Library/MemEncryptHypercallLib/X64/MemEncryptHypercallLib.c
> new file mode 100644
> index 0000000000..1c09ea012b
> --- /dev/null
> +++ b/OvmfPkg/Library/MemEncryptHypercallLib/X64/MemEncryptHypercallLib.c
> @@ -0,0 +1,105 @@
> +/** @file
> +
> +  Secure Encrypted Virtualization (SEV) hypercall helper library
> +
> +  Copyright (c) 2020, AMD Incorporated. All rights reserved.<BR>
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include <Uefi.h>
> +#include <Uefi/UefiBaseType.h>
> +#include <Library/BaseLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/VmgExitLib.h>
> +#include <Register/Amd/Ghcb.h>
> +#include <Register/Amd/Msr.h>
> +#include <Library/MemEncryptSevLib.h>
> +#include <Library/MemEncryptHypercallLib.h>
> +
> +//
> +// Interface exposed by the ASM implementation of the core hypercall
> +//
> +//
> +
> +VOID
> +EFIAPI
> +SetMemoryEncDecHypercall3AsmStub (
> +  IN  UINTN  HypercallNum,
> +  IN  UINTN  PhysicalAddress,
> +  IN  UINTN  Length,
> +  IN  UINTN  Mode
> +  );
> +
> +STATIC
> +VOID
> +GhcbSetRegValid (
> +  IN OUT GHCB       *Ghcb,
> +  IN GHCB_REGISTER  Reg
> +  )
> +{
> +  UINT32  RegIndex;
> +  UINT32  RegBit;
> +
> +  RegIndex = Reg / 8;
> +  RegBit   = Reg & 0x07;
> +
> +  Ghcb->SaveArea.ValidBitmap[RegIndex] |= (1 << RegBit);
> +}

As Brijesh noted, you should be using the function defined in VmgExitLib.h.

> +
> +/**
> + This hyercall is used to notify hypervisor when a page is marked as
> + 'decrypted' (i.e C-bit removed).
> +
> + @param[in]   PhysicalAddress       The physical address that is the start address
> +                                    of a memory region.
> + @param[in]   Length                The length of memory region
> + @param[in]   Mode                  SetCBit or ClearCBit
> +
> +**/
> +
> +VOID
> +EFIAPI
> +SetMemoryEncDecHypercall3 (
> +  IN PHYSICAL_ADDRESS PhysicalAddress,
> +  IN UINTN            Pages,
> +  IN UINTN            Mode
> +  )
> +{
> +  if (MemEncryptSevEsIsEnabled ()) {
> +    MSR_SEV_ES_GHCB_REGISTER  Msr;
> +    GHCB                      *Ghcb;
> +    BOOLEAN                   InterruptState;
> +    UINT64                    Status;
> +
> +    Msr.GhcbPhysicalAddress = AsmReadMsr64 (MSR_SEV_ES_GHCB);
> +    Ghcb = Msr.Ghcb;
> +
> +    VmgInit (Ghcb, &InterruptState);
> +
> +    Ghcb->SaveArea.Rax = KVM_HC_MAP_GPA_RANGE;
> +    GhcbSetRegValid (Ghcb, GhcbRax);
> +    Ghcb->SaveArea.Rbx = PhysicalAddress;
> +    GhcbSetRegValid (Ghcb, GhcbRbx);
> +    Ghcb->SaveArea.Rcx = Pages;
> +    GhcbSetRegValid (Ghcb, GhcbRcx);
> +    Ghcb->SaveArea.Rdx = Mode;
> +    GhcbSetRegValid (Ghcb, GhcbRdx);
> +    Ghcb->SaveArea.Cpl = AsmReadCs() & 0x3;
> +    GhcbSetRegValid (Ghcb, GhcbCpl);
> +
> +    Status = VmgExit (Ghcb, SVM_EXIT_VMMCALL, 0, 0);
> +    if (Status) {
> +      DEBUG ((DEBUG_ERROR, "SVM_EXIT_VMMCALL failed %lx\n", Status));
> +    }
> +    VmgDone (Ghcb, InterruptState);
> +  } else {
> +    SetMemoryEncDecHypercall3AsmStub (
> +      KVM_HC_MAP_GPA_RANGE,
> +      PhysicalAddress,
> +      Pages,
> +      Mode
> +      );
> +  }
> +}

You could just issue the VMMCALL and, for SEV-ES, let the VC handler take
care of this. You would just have to add some smarts to the VC handler to
compare the hypercall number and add the additional register values. You
could probably get rid of a level of function calls that way. Thoughts?

Thanks,
Tom

> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index f53efeae79..36f1d82ce7 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -176,6 +176,7 @@
>    VirtioLib|OvmfPkg/Library/VirtioLib/VirtioLib.inf
>    LoadLinuxLib|OvmfPkg/Library/LoadLinuxLib/LoadLinuxLib.inf
>    MemEncryptSevLib|OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf
> +  MemEncryptHypercallLib|OvmfPkg/Library/MemEncryptHypercallLib/MemEncryptHypercallLib.inf
>  !if $(SMM_REQUIRE) == FALSE
>    LockBoxLib|OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf
>  !endif
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index b3662e17f2..2a743688b4 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -180,6 +180,7 @@
>    VirtioLib|OvmfPkg/Library/VirtioLib/VirtioLib.inf
>    LoadLinuxLib|OvmfPkg/Library/LoadLinuxLib/LoadLinuxLib.inf
>    MemEncryptSevLib|OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf
> +  MemEncryptHypercallLib|OvmfPkg/Library/MemEncryptHypercallLib/MemEncryptHypercallLib.inf
>  !if $(SMM_REQUIRE) == FALSE
>    LockBoxLib|OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf
>  !endif
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index 0a237a9058..eb9da51a15 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -180,6 +180,7 @@
>    VirtioLib|OvmfPkg/Library/VirtioLib/VirtioLib.inf
>    LoadLinuxLib|OvmfPkg/Library/LoadLinuxLib/LoadLinuxLib.inf
>    MemEncryptSevLib|OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf
> +  MemEncryptHypercallLib|OvmfPkg/Library/MemEncryptHypercallLib/MemEncryptHypercallLib.inf
>  !if $(SMM_REQUIRE) == FALSE
>    LockBoxLib|OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf
>  !endif
> diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc
> index 3c1ca6bfd4..de0c052832 100644
> --- a/OvmfPkg/OvmfXen.dsc
> +++ b/OvmfPkg/OvmfXen.dsc
> @@ -167,6 +167,7 @@
>    QemuFwCfgLib|OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxeLib.inf
>    QemuLoadImageLib|OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf
>    MemEncryptSevLib|OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf
> +  MemEncryptHypercallLib|OvmfPkg/Library/MemEncryptHypercallLib/MemEncryptHypercallLib.inf
>    LockBoxLib|OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf
>    CustomizedDisplayLib|MdeModulePkg/Library/CustomizedDisplayLib/CustomizedDisplayLib.inf
>    FrameBufferBltLib|MdeModulePkg/Library/FrameBufferBltLib/FrameBufferBltLib.inf
>