[edk2-devel] [PATCH v3 1/3] OvmfPkg/MemEncryptHypercallLib: add library to support SEV hypercalls.
Ashish Kalra
ashish.kalra at amd.com
Tue Dec 8 14:23:56 UTC 2020
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 at 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 at intel.com>
> > Cc: Laszlo Ersek <lersek at redhat.com>
> > Cc: Ard Biesheuvel <ard.biesheuvel at arm.com>
> >
> > Signed-off-by: Ashish Kalra <ashish.kalra at 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&data=04%7C01%7CAshish.Kalra%40amd.com%7C4123cf72f32946b8e75a08d899e498be%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637428554380675577%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=xUydJsFm7hI04UBbucGpSVD2X%2FIZtwLAMbW%2Bm5RHAuo%3D&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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#68472): https://edk2.groups.io/g/devel/message/68472
Mute This Topic: https://groups.io/mt/78698838/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list