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

["Dov Murik" <dovmurik@linux.vnet.ibm.com>]

On 04/12/2020 2:03, 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.
> 
> 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>
> ---
>   OvmfPkg/Include/Library/MemEncryptHypercallLib.h                  |  37 +++++++
>   OvmfPkg/Library/MemEncryptHypercallLib/MemEncryptHypercallLib.c   | 105 ++++++++++++++++++++
>   OvmfPkg/Library/MemEncryptHypercallLib/MemEncryptHypercallLib.inf |  39 ++++++++
>   OvmfPkg/Library/MemEncryptHypercallLib/X64/AsmHelperStub.nasm     |  39 ++++++++
>   OvmfPkg/OvmfPkgX64.dsc                                            |   1 +
>   5 files changed, 221 insertions(+)
> 
> diff --git a/OvmfPkg/Include/Library/MemEncryptHypercallLib.h b/OvmfPkg/Include/Library/MemEncryptHypercallLib.h
> new file mode 100644
> index 0000000000..cd46a7f2b3
> --- /dev/null
> +++ b/OvmfPkg/Include/Library/MemEncryptHypercallLib.h
> @@ -0,0 +1,37 @@
> +/** @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 SEV_PAGE_ENC_HYPERCALL  12
> +
> +/**
> + 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
> +  );

(1) I'm not sure why the "3" is needed at the end of the function name.

(2) The second argument is called 'Length' here and 'Pages' in the .c 
file. Which name is correct? If the semantics of the hypercall deals 
only with whole pages, maybe it'll be better to modify the address to 
GFN and length to NumberOfPages.  This way you don't have to do deal 
with non-page-aligned addresses or lengths.  Of course this may reflect 
on changes needed in the hypercall implementation itself in KVM.

(3) Mode: is this actually a boolean, or the enum which can be 
SetCBit(=0) or ClearCBit(=1)? Maybe a clearer argument would be "BOOL 
Encrypted" or "BOOL NewCBitValue"?  Of course this has to match 
semantics of this argument in KVM.


> +
> +#endif
> diff --git a/OvmfPkg/Library/MemEncryptHypercallLib/MemEncryptHypercallLib.c b/OvmfPkg/Library/MemEncryptHypercallLib/MemEncryptHypercallLib.c
> new file mode 100644
> index 0000000000..f1136b7d36
> --- /dev/null
> +++ b/OvmfPkg/Library/MemEncryptHypercallLib/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
> +  );
> +
> +/**
> + This function returns the current CPU privilege level, implemented
> + in ASM helper stub.
> +
> +**/
> +
> +UINT8
> +EFIAPI
> +GetCurrentCpuPrivilegeLevel (
> +  VOID
> +  );
> +
> +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);
> +}
> +
> +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 = SEV_PAGE_ENC_HYPERCALL;
> +    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 = GetCurrentCpuPrivilegeLevel();
> +    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 (
> +      SEV_PAGE_ENC_HYPERCALL,
> +      PhysicalAddress,
> +      Pages,
> +      Mode);
> +  }
> +}
> diff --git a/OvmfPkg/Library/MemEncryptHypercallLib/MemEncryptHypercallLib.inf b/OvmfPkg/Library/MemEncryptHypercallLib/MemEncryptHypercallLib.inf
> new file mode 100644
> index 0000000000..1936fe5b37
> --- /dev/null
> +++ b/OvmfPkg/Library/MemEncryptHypercallLib/MemEncryptHypercallLib.inf
> @@ -0,0 +1,39 @@
> +## @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|SEC PEI_CORE PEIM DXE_DRIVER DXE_RUNTIME_DRIVER DXE_SMM_DRIVER UEFI_DRIVER
> +
> +#
> +# 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]
> +  MemEncryptHypercallLib.c
> +  X64/AsmHelperStub.nasm
> +
> +[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..5d8a7aa85a
> --- /dev/null
> +++ b/OvmfPkg/Library/MemEncryptHypercallLib/X64/AsmHelperStub.nasm
> @@ -0,0 +1,39 @@
> +DEFAULT REL
> +SECTION .text
> +
> +; VOID
> +; EFIAPI
> +; SetMemoryEncDecHypercall3AsmStub (
> +;   IN UINT HypercallNum,
> +;   IN INTN Arg1,
> +;   IN INTN Arg2,
> +;   IN INTN Arg3
> +;   );

The name of the function hints that this has something to do with memory 
enc/dec, but actually this is a generic vmmcall-based hypercall.  In 
your corresponding linux patch 
<https://lore.kernel.org/kvm/b6bc54ed6c8ae4444f3acf1ed4386010783ad386.1606782580.git.ashish.kalra@amd.com/> 
you called it kvm_sev_hypercall3, so I assume something like that would 
be good here too.  Also, to support future hypercalls, allow it to 
return an INTN (value of rax), even though that is not used for the 
current hypercall in question.


> +global ASM_PFX(SetMemoryEncDecHypercall3AsmStub)
> +ASM_PFX(SetMemoryEncDecHypercall3AsmStub):
> +  ; UEFI calling conventions require RBX to
> +  ; be nonvolatile/callee-saved.
> +  push rbx
> +  ; Copy HypercallNumber 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
> +
> +; UINT8
> +; EFIAPI
> +; GetCurrentCpuPrivilegeLevel (
> +;   VOID
> +;   );
> +global ASM_PFX(GetCurrentCpuPrivilegeLevel)
> +ASM_PFX(GetCurrentCpuPrivilegeLevel):
> +  mov  ax, cs
> +  and  al, 0x3
> +  ret

I think GetCurrentCpuPrivilegeLevel can be implemented in C as:

   return AsmReadCS() & 0x3;




> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index e59ae05b73..97c31c7586 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -174,6 +174,7 @@
>     VirtioLib|OvmfPkg/Library/VirtioLib/VirtioLib.inf
>     LoadLinuxLib|OvmfPkg/Library/LoadLinuxLib/LoadLinuxLib.inf
>     MemEncryptSevLib|OvmfPkg/Library/BaseMemEncryptSevLib/BaseMemEncryptSevLib.inf
> +  MemEncryptHypercallLib|OvmfPkg/Library/MemEncryptHypercallLib/MemEncryptHypercallLib.inf
>   !if $(SMM_REQUIRE) == FALSE
>     LockBoxLib|OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf
>   !endif
> 


-Dov