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

["Ashish Kalra" <ashish.kalra@amd.com>]

Hello Dov,

On Sun, Dec 06, 2020 at 02:43:45PM +0200, Dov Murik wrote:
> 
> 
> 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.
> 

As you mentioned below, this is analogous to our kernel interface, i.e.,
kvm_sev_hypercall3() and the XenHypercallLib also uses functional
interfaces such as XenHypercall2(). Though i think this functional
interface can be at the lowest level, at a higher level something like
SetMemoryEncDecHypercall() should be better.

> (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.

I will fix this.
> 
> 
> > +
> > +#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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fkvm%2Fb6bc54ed6c8ae4444f3acf1ed4386010783ad386.1606782580.git.ashish.kalra%40amd.com%2F&amp;data=04%7C01%7CAshish.Kalra%40amd.com%7C4123cf72f32946b8e75a08d899e498be%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637428554380675577%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=xUydJsFm7hI04UBbucGpSVD2X%2FIZtwLAMbW%2Bm5RHAuo%3D&amp;reserved=0>
> 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.
> 
> 
Yes, i will add a return value to the hypercall.

> > +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;
> 

Ok. 
> 
> 
> > 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
> > 
> 
> 
Thanks,
Ashish