* [PATCH v4 0/4] SEV Live Migration support for OVMF.
@ 2021-06-21 13:56 Ashish Kalra
2021-06-21 13:56 ` [PATCH v4 1/4] OvmfPkg/MemEncryptHypercallLib: add library to support SEV hypercalls Ashish Kalra
` (4 more replies)
0 siblings, 5 replies; 25+ messages in thread
From: Ashish Kalra @ 2021-06-21 13:56 UTC (permalink / raw)
To: devel
Cc: brijesh.singh, Thomas.Lendacky, jejb, erdemaktas, jiewen.yao,
min.m.xu, lersek, jordan.l.justen, ard.biesheuvel
From: Ashish Kalra <ashish.kalra@amd.com>
By default all the SEV guest memory regions are considered encrypted,
if a guest changes the encryption attribute of the page (e.g mark a
page as decrypted) then notify hypervisor. Hypervisor will need to
track the unencrypted pages. The information will be used during
guest live migration, guest page migration and guest debugging.
The patch-set adds a new SEV and SEV-ES hypercall abstraction
library to support SEV Page encryption/decryption status hypercalls
for SEV and SEV-ES guests.
BaseMemEncryptSevLib invokes hypercalls via this new hypercall library.
The patch-set detects if it is running under KVM hypervisor and then
checks for SEV live migration feature support via KVM_FEATURE_CPUID,
if detected setup a new UEFI enviroment variable to indicate OVMF
support for SEV live migration.
A branch containing these patches is available here:
https://github.com/ashkalra/edk2/tree/sev_live_migration_v4
Changes since v3:
- Fix all DSC files under OvmfPkg except X64 to add support for
BaseMemEncryptLib and add NULL instance of BaseMemEncryptLib
for 32 bit platforms.
- Add the MemEncryptHypercallLib-related files to Maintainers.txt,
in section "OvmfPkg: Confidential Computing".
- Add support for the new KVM_HC_MAP_GPA_RANGE hypercall interface.
- Add patch for SEV live migration support.
Changes since v2:
- GHCB_BASE setup during reset-vector as decrypted is marked explicitly
in the hypervisor page encryption bitmap after setting the
PcdSevEsIsEnabled PCD.
Changes since v1:
- Mark GHCB_BASE setup during reset-vector as decrypted explicitly in
the hypervisor page encryption bitmap.
- Resending the series with correct shallow threading.
Ashish Kalra (3):
OvmfPkg/MemEncryptHypercallLib: add library to support SEV hypercalls.
OvmfPkg/PlatformPei: Mark SEC GHCB page as unencrypted via hypercall
OvmfPkg/PlatformDxe: Add support for SEV live migration.
Brijesh Singh (1):
OvmfPkg/BaseMemEncryptLib: Support to issue unencrypted hypercall
Maintainers.txt | 2 +
OvmfPkg/Include/Guid/MemEncryptLib.h | 20 ++++
.../Include/Library/MemEncryptHypercallLib.h | 43 +++++++
.../DxeMemEncryptSevLib.inf | 1 +
.../PeiMemEncryptSevLib.inf | 1 +
.../X64/PeiDxeVirtualMemory.c | 22 ++++
.../Ia32/MemEncryptHypercallLib.c | 37 ++++++
.../MemEncryptHypercallLib.inf | 42 +++++++
.../X64/AsmHelperStub.nasm | 28 +++++
.../X64/MemEncryptHypercallLib.c | 105 +++++++++++++++++
OvmfPkg/OvmfPkg.dec | 1 +
OvmfPkg/OvmfPkgIa32.dsc | 1 +
OvmfPkg/OvmfPkgIa32X64.dsc | 1 +
OvmfPkg/OvmfPkgX64.dsc | 1 +
OvmfPkg/OvmfXen.dsc | 1 +
OvmfPkg/PlatformDxe/AmdSev.c | 108 ++++++++++++++++++
OvmfPkg/PlatformDxe/Platform.c | 5 +
OvmfPkg/PlatformDxe/Platform.inf | 2 +
OvmfPkg/PlatformDxe/PlatformConfig.h | 5 +
OvmfPkg/PlatformPei/AmdSev.c | 10 ++
20 files changed, 436 insertions(+)
create mode 100644 OvmfPkg/Include/Guid/MemEncryptLib.h
create mode 100644 OvmfPkg/Include/Library/MemEncryptHypercallLib.h
create mode 100644 OvmfPkg/Library/MemEncryptHypercallLib/Ia32/MemEncryptHypercallLib.c
create mode 100644 OvmfPkg/Library/MemEncryptHypercallLib/MemEncryptHypercallLib.inf
create mode 100644 OvmfPkg/Library/MemEncryptHypercallLib/X64/AsmHelperStub.nasm
create mode 100644 OvmfPkg/Library/MemEncryptHypercallLib/X64/MemEncryptHypercallLib.c
create mode 100644 OvmfPkg/PlatformDxe/AmdSev.c
--
2.17.1
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v4 1/4] OvmfPkg/MemEncryptHypercallLib: add library to support SEV hypercalls.
2021-06-21 13:56 [PATCH v4 0/4] SEV Live Migration support for OVMF Ashish Kalra
@ 2021-06-21 13:56 ` Ashish Kalra
2021-06-22 19:47 ` Brijesh Singh
2021-06-22 22:47 ` Lendacky, Thomas
2021-06-21 13:57 ` [PATCH v4 2/4] OvmfPkg/BaseMemEncryptLib: Support to issue unencrypted hypercall Ashish Kalra
` (3 subsequent siblings)
4 siblings, 2 replies; 25+ messages in thread
From: Ashish Kalra @ 2021-06-21 13:56 UTC (permalink / raw)
To: devel
Cc: brijesh.singh, Thomas.Lendacky, jejb, erdemaktas, jiewen.yao,
min.m.xu, lersek, jordan.l.justen, ard.biesheuvel
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>
---
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)
+#define KVM_MAP_GPA_RANGE_PAGE_SZ_1G (1 << 1)
+#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
+
+**/
+
+VOID
+EFIAPI
+SetMemoryEncDecHypercall3 (
+ IN PHYSICAL_ADDRESS PhysicalAddress,
+ IN UINTN Pages,
+ 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
+ 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);
+}
+
+/**
+ 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
+ );
+ }
+}
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
--
2.17.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v4 2/4] OvmfPkg/BaseMemEncryptLib: Support to issue unencrypted hypercall
2021-06-21 13:56 [PATCH v4 0/4] SEV Live Migration support for OVMF Ashish Kalra
2021-06-21 13:56 ` [PATCH v4 1/4] OvmfPkg/MemEncryptHypercallLib: add library to support SEV hypercalls Ashish Kalra
@ 2021-06-21 13:57 ` Ashish Kalra
2021-06-22 22:50 ` Lendacky, Thomas
2021-06-21 13:57 ` [PATCH v4 3/4] OvmfPkg/PlatformPei: Mark SEC GHCB page as unencrypted via hypercall Ashish Kalra
` (2 subsequent siblings)
4 siblings, 1 reply; 25+ messages in thread
From: Ashish Kalra @ 2021-06-21 13:57 UTC (permalink / raw)
To: devel
Cc: brijesh.singh, Thomas.Lendacky, jejb, erdemaktas, jiewen.yao,
min.m.xu, lersek, jordan.l.justen, ard.biesheuvel
From: Brijesh Singh <brijesh.singh@amd.com>
By default all the SEV guest memory regions are considered encrypted,
if a guest changes the encryption attribute of the page (e.g mark a
page as decrypted) then notify hypervisor. Hypervisor will need to
track the unencrypted pages. The information will be used during
guest live migration, guest page migration and guest debugging.
Invoke hypercall via the new hypercall library.
This hypercall is used to notify hypervisor when a page is marked as
'decrypted' (i.e C-bit removed).
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
---
OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf | 1 +
OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf | 1 +
OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c | 22 ++++++++++++++++++++
3 files changed, 24 insertions(+)
diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf b/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf
index f2e162d680..aefcd7c0f7 100644
--- a/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf
+++ b/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf
@@ -49,6 +49,7 @@
DebugLib
MemoryAllocationLib
PcdLib
+ MemEncryptHypercallLib
[FeaturePcd]
gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire
diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf
index 03a78c32df..7503f56a0b 100644
--- a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf
+++ b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf
@@ -49,6 +49,7 @@
DebugLib
MemoryAllocationLib
PcdLib
+ MemEncryptHypercallLib
[FeaturePcd]
gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire
diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
index c696745f9d..12b3a9fcfb 100644
--- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
+++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
@@ -15,6 +15,7 @@
#include <Library/MemEncryptSevLib.h>
#include <Register/Amd/Cpuid.h>
#include <Register/Cpuid.h>
+#include <Library/MemEncryptHypercallLib.h>
#include "VirtualMemory.h"
@@ -585,6 +586,9 @@ SetMemoryEncDec (
UINT64 AddressEncMask;
BOOLEAN IsWpEnabled;
RETURN_STATUS Status;
+ UINTN Size;
+ BOOLEAN CBitChanged;
+ PHYSICAL_ADDRESS OrigPhysicalAddress;
//
// Set PageMapLevel4Entry to suppress incorrect compiler/analyzer warnings.
@@ -636,6 +640,10 @@ SetMemoryEncDec (
Status = EFI_SUCCESS;
+ Size = Length;
+ CBitChanged = FALSE;
+ OrigPhysicalAddress = PhysicalAddress;
+
while (Length != 0)
{
//
@@ -695,6 +703,7 @@ SetMemoryEncDec (
));
PhysicalAddress += BIT30;
Length -= BIT30;
+ CBitChanged = TRUE;
} else {
//
// We must split the page
@@ -749,6 +758,7 @@ SetMemoryEncDec (
SetOrClearCBit (&PageDirectory2MEntry->Uint64, Mode);
PhysicalAddress += BIT21;
Length -= BIT21;
+ CBitChanged = TRUE;
} else {
//
// We must split up this page into 4K pages
@@ -791,6 +801,7 @@ SetMemoryEncDec (
SetOrClearCBit (&PageTableEntry->Uint64, Mode);
PhysicalAddress += EFI_PAGE_SIZE;
Length -= EFI_PAGE_SIZE;
+ CBitChanged = TRUE;
}
}
}
@@ -808,6 +819,17 @@ SetMemoryEncDec (
//
CpuFlushTlb();
+ //
+ // Notify Hypervisor on C-bit status
+ //
+ if (CBitChanged) {
+ SetMemoryEncDecHypercall3 (
+ OrigPhysicalAddress,
+ EFI_SIZE_TO_PAGES(Size),
+ KVM_MAP_GPA_RANGE_ENC_STAT(!Mode)
+ );
+ }
+
Done:
//
// Restore page table write protection, if any.
--
2.17.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v4 3/4] OvmfPkg/PlatformPei: Mark SEC GHCB page as unencrypted via hypercall
2021-06-21 13:56 [PATCH v4 0/4] SEV Live Migration support for OVMF Ashish Kalra
2021-06-21 13:56 ` [PATCH v4 1/4] OvmfPkg/MemEncryptHypercallLib: add library to support SEV hypercalls Ashish Kalra
2021-06-21 13:57 ` [PATCH v4 2/4] OvmfPkg/BaseMemEncryptLib: Support to issue unencrypted hypercall Ashish Kalra
@ 2021-06-21 13:57 ` Ashish Kalra
2021-06-22 20:35 ` Brijesh Singh
2021-06-21 13:57 ` [PATCH v4 4/4] OvmfPkg/PlatformDxe: Add support for SEV live migration Ashish Kalra
2021-06-22 17:20 ` [PATCH v4 0/4] SEV Live Migration support for OVMF Laszlo Ersek
4 siblings, 1 reply; 25+ messages in thread
From: Ashish Kalra @ 2021-06-21 13:57 UTC (permalink / raw)
To: devel
Cc: brijesh.singh, Thomas.Lendacky, jejb, erdemaktas, jiewen.yao,
min.m.xu, lersek, jordan.l.justen, ard.biesheuvel
From: Ashish Kalra <ashish.kalra@amd.com>
Mark the SEC GHCB page (that is mapped as unencrypted in
ResetVector code) in the hypervisor page status tracking.
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/PlatformPei/AmdSev.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
index a8bf610022..3f642ecb06 100644
--- a/OvmfPkg/PlatformPei/AmdSev.c
+++ b/OvmfPkg/PlatformPei/AmdSev.c
@@ -15,6 +15,7 @@
#include <Library/HobLib.h>
#include <Library/MemEncryptSevLib.h>
#include <Library/MemoryAllocationLib.h>
+#include <Library/MemEncryptHypercallLib.h>
#include <Library/PcdLib.h>
#include <PiPei.h>
#include <Register/Amd/Msr.h>
@@ -52,6 +53,15 @@ AmdSevEsInitialize (
PcdStatus = PcdSetBoolS (PcdSevEsIsEnabled, TRUE);
ASSERT_RETURN_ERROR (PcdStatus);
+ //
+ // GHCB_BASE setup during reset-vector needs to be marked as
+ // decrypted in the hypervisor page encryption bitmap.
+ //
+ SetMemoryEncDecHypercall3 (FixedPcdGet32 (PcdOvmfSecGhcbBase),
+ EFI_SIZE_TO_PAGES(FixedPcdGet32 (PcdOvmfSecGhcbSize)),
+ KVM_MAP_GPA_RANGE_DECRYPTED
+ );
+
//
// Allocate GHCB and per-CPU variable pages.
// Since the pages must survive across the UEFI to OS transition
--
2.17.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v4 4/4] OvmfPkg/PlatformDxe: Add support for SEV live migration.
2021-06-21 13:56 [PATCH v4 0/4] SEV Live Migration support for OVMF Ashish Kalra
` (2 preceding siblings ...)
2021-06-21 13:57 ` [PATCH v4 3/4] OvmfPkg/PlatformPei: Mark SEC GHCB page as unencrypted via hypercall Ashish Kalra
@ 2021-06-21 13:57 ` Ashish Kalra
2021-06-22 23:06 ` Lendacky, Thomas
2021-06-22 17:20 ` [PATCH v4 0/4] SEV Live Migration support for OVMF Laszlo Ersek
4 siblings, 1 reply; 25+ messages in thread
From: Ashish Kalra @ 2021-06-21 13:57 UTC (permalink / raw)
To: devel
Cc: brijesh.singh, Thomas.Lendacky, jejb, erdemaktas, jiewen.yao,
min.m.xu, lersek, jordan.l.justen, ard.biesheuvel
From: Ashish Kalra <ashish.kalra@amd.com>
Detect for KVM hypervisor and check for SEV live migration
feature support via KVM_FEATURE_CPUID, if detected setup a new
UEFI enviroment variable to indicate OVMF support for SEV
live migration.
Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
---
OvmfPkg/Include/Guid/MemEncryptLib.h | 20 ++++
OvmfPkg/OvmfPkg.dec | 1 +
OvmfPkg/PlatformDxe/AmdSev.c | 108 ++++++++++++++++++++
OvmfPkg/PlatformDxe/Platform.c | 5 +
OvmfPkg/PlatformDxe/Platform.inf | 2 +
OvmfPkg/PlatformDxe/PlatformConfig.h | 5 +
6 files changed, 141 insertions(+)
diff --git a/OvmfPkg/Include/Guid/MemEncryptLib.h b/OvmfPkg/Include/Guid/MemEncryptLib.h
new file mode 100644
index 0000000000..4c046ba439
--- /dev/null
+++ b/OvmfPkg/Include/Guid/MemEncryptLib.h
@@ -0,0 +1,20 @@
+/** @file
+
+ AMD Memory Encryption GUID, define a new GUID for defining
+ new UEFI enviroment variables assocaiated with SEV Memory Encryption.
+
+ Copyright (c) 2020, AMD Inc. All rights reserved.<BR>
+
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#ifndef __MEMENCRYPT_LIB_H__
+#define __MEMENCRYPT_LIB_H__
+
+#define MEMENCRYPT_GUID \
+{0x0cf29b71, 0x9e51, 0x433a, {0xa3, 0xb7, 0x81, 0xf3, 0xab, 0x16, 0xb8, 0x75}}
+
+extern EFI_GUID gMemEncryptGuid;
+
+#endif
diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
index 6ae733f6e3..e452dc8494 100644
--- a/OvmfPkg/OvmfPkg.dec
+++ b/OvmfPkg/OvmfPkg.dec
@@ -122,6 +122,7 @@
gQemuKernelLoaderFsMediaGuid = {0x1428f772, 0xb64a, 0x441e, {0xb8, 0xc3, 0x9e, 0xbd, 0xd7, 0xf8, 0x93, 0xc7}}
gGrubFileGuid = {0xb5ae312c, 0xbc8a, 0x43b1, {0x9c, 0x62, 0xeb, 0xb8, 0x26, 0xdd, 0x5d, 0x07}}
gConfidentialComputingSecretGuid = {0xadf956ad, 0xe98c, 0x484c, {0xae, 0x11, 0xb5, 0x1c, 0x7d, 0x33, 0x64, 0x47}}
+ gMemEncryptGuid = {0x0cf29b71, 0x9e51, 0x433a, {0xa3, 0xb7, 0x81, 0xf3, 0xab, 0x16, 0xb8, 0x75}}
[Ppis]
# PPI whose presence in the PPI database signals that the TPM base address
diff --git a/OvmfPkg/PlatformDxe/AmdSev.c b/OvmfPkg/PlatformDxe/AmdSev.c
new file mode 100644
index 0000000000..3dbf17a8cd
--- /dev/null
+++ b/OvmfPkg/PlatformDxe/AmdSev.c
@@ -0,0 +1,108 @@
+/**@file
+ Detect KVM hypervisor support for SEV live migration and if
+ detected, setup a new UEFI enviroment variable indicating
+ OVMF support for SEV live migration.
+
+ Copyright (c) 2020, Advanced Micro Devices. All rights reserved.<BR>
+
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+//
+// The package level header files this module uses
+//
+
+#include <Library/BaseLib.h>
+#include <Library/BaseMemoryLib.h>
+#include <Library/DebugLib.h>
+#include <Library/UefiBootServicesTableLib.h>
+#include <Library/UefiRuntimeServicesTableLib.h>
+#include <Guid/MemEncryptLib.h>
+
+#define KVM_FEATURE_MIGRATION_CONTROL 17
+
+/**
+ Figures out if we are running inside KVM HVM and
+ KVM HVM supports SEV Live Migration feature.
+
+ @retval TRUE KVM was detected and Live Migration supported
+ @retval FALSE KVM was not detected or Live Migration not supported
+
+**/
+BOOLEAN
+KvmDetectSevLiveMigrationFeature(
+ VOID
+ )
+{
+ UINT8 Signature[13];
+ UINT32 mKvmLeaf = 0;
+ UINT32 RegEax, RegEbx, RegEcx, RegEdx;
+
+ Signature[12] = '\0';
+ for (mKvmLeaf = 0x40000000; mKvmLeaf < 0x40010000; mKvmLeaf += 0x100) {
+ AsmCpuid (mKvmLeaf,
+ NULL,
+ (UINT32 *) &Signature[0],
+ (UINT32 *) &Signature[4],
+ (UINT32 *) &Signature[8]);
+
+ if (!AsciiStrCmp ((CHAR8 *) Signature, "KVMKVMKVM\0\0\0")) {
+ DEBUG ((
+ DEBUG_ERROR,
+ "%a: KVM Detected, signature = %s\n",
+ __FUNCTION__,
+ Signature
+ ));
+
+ RegEax = 0x40000001;
+ RegEcx = 0;
+ AsmCpuid (0x40000001, &RegEax, &RegEbx, &RegEcx, &RegEdx);
+ if (RegEax & (1 << KVM_FEATURE_MIGRATION_CONTROL)) {
+ DEBUG ((
+ DEBUG_ERROR,
+ "%a: Live Migration feature supported\n",
+ __FUNCTION__
+ ));
+ return TRUE;
+ }
+ }
+ }
+
+ return FALSE;
+}
+
+/**
+
+ Function checks if SEV Live Migration support is available, if present then it sets
+ a UEFI enviroment variable to be queried later using Runtime services.
+
+ **/
+VOID
+AmdSevSetConfig(
+ VOID
+ )
+{
+ EFI_STATUS Status;
+ BOOLEAN SevLiveMigrationEnabled;
+
+ SevLiveMigrationEnabled = KvmDetectSevLiveMigrationFeature();
+
+ if (SevLiveMigrationEnabled) {
+ Status = gRT->SetVariable (
+ L"SevLiveMigrationEnabled",
+ &gMemEncryptGuid,
+ EFI_VARIABLE_NON_VOLATILE |
+ EFI_VARIABLE_BOOTSERVICE_ACCESS |
+ EFI_VARIABLE_RUNTIME_ACCESS,
+ sizeof (BOOLEAN),
+ &SevLiveMigrationEnabled
+ );
+
+ DEBUG ((
+ DEBUG_ERROR,
+ "%a: Setting SevLiveMigrationEnabled variable, status = %lx\n",
+ __FUNCTION__,
+ Status
+ ));
+ }
+}
diff --git a/OvmfPkg/PlatformDxe/Platform.c b/OvmfPkg/PlatformDxe/Platform.c
index f2e51960ce..f61302d98b 100644
--- a/OvmfPkg/PlatformDxe/Platform.c
+++ b/OvmfPkg/PlatformDxe/Platform.c
@@ -763,6 +763,11 @@ PlatformInit (
{
EFI_STATUS Status;
+ //
+ // Set Amd Sev configuation
+ //
+ AmdSevSetConfig();
+
ExecutePlatformConfig ();
mConfigAccess.ExtractConfig = &ExtractConfig;
diff --git a/OvmfPkg/PlatformDxe/Platform.inf b/OvmfPkg/PlatformDxe/Platform.inf
index 14727c1220..2896f0a1d1 100644
--- a/OvmfPkg/PlatformDxe/Platform.inf
+++ b/OvmfPkg/PlatformDxe/Platform.inf
@@ -24,6 +24,7 @@
PlatformConfig.c
PlatformConfig.h
PlatformForms.vfr
+ AmdSev.c
[Packages]
MdePkg/MdePkg.dec
@@ -56,6 +57,7 @@
[Guids]
gEfiIfrTianoGuid
gOvmfPlatformConfigGuid
+ gMemEncryptGuid
[Depex]
gEfiHiiConfigRoutingProtocolGuid AND
diff --git a/OvmfPkg/PlatformDxe/PlatformConfig.h b/OvmfPkg/PlatformDxe/PlatformConfig.h
index 716514da21..4f662aafa4 100644
--- a/OvmfPkg/PlatformDxe/PlatformConfig.h
+++ b/OvmfPkg/PlatformDxe/PlatformConfig.h
@@ -44,6 +44,11 @@ PlatformConfigLoad (
OUT UINT64 *OptionalElements
);
+VOID
+AmdSevSetConfig(
+ VOID
+ );
+
//
// Feature flags for OptionalElements.
//
--
2.17.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH v4 0/4] SEV Live Migration support for OVMF.
2021-06-21 13:56 [PATCH v4 0/4] SEV Live Migration support for OVMF Ashish Kalra
` (3 preceding siblings ...)
2021-06-21 13:57 ` [PATCH v4 4/4] OvmfPkg/PlatformDxe: Add support for SEV live migration Ashish Kalra
@ 2021-06-22 17:20 ` Laszlo Ersek
2021-06-22 17:45 ` Brijesh Singh
2021-06-22 17:46 ` Ashish Kalra
4 siblings, 2 replies; 25+ messages in thread
From: Laszlo Ersek @ 2021-06-22 17:20 UTC (permalink / raw)
To: Ashish Kalra, devel
Cc: brijesh.singh, Thomas.Lendacky, jejb, erdemaktas, jiewen.yao,
min.m.xu, jordan.l.justen, ard.biesheuvel, Dr. David Alan Gilbert,
Paolo Bonzini
Hi Ashish,
(+Dave, +Paolo)
On 06/21/21 15:56, Ashish Kalra wrote:
> From: Ashish Kalra <ashish.kalra@amd.com>
>
> By default all the SEV guest memory regions are considered encrypted,
> if a guest changes the encryption attribute of the page (e.g mark a
> page as decrypted) then notify hypervisor. Hypervisor will need to
> track the unencrypted pages. The information will be used during
> guest live migration, guest page migration and guest debugging.
>
> The patch-set adds a new SEV and SEV-ES hypercall abstraction
> library to support SEV Page encryption/decryption status hypercalls
> for SEV and SEV-ES guests.
>
> BaseMemEncryptSevLib invokes hypercalls via this new hypercall library.
>
> The patch-set detects if it is running under KVM hypervisor and then
> checks for SEV live migration feature support via KVM_FEATURE_CPUID,
> if detected setup a new UEFI enviroment variable to indicate OVMF
> support for SEV live migration.
>
> A branch containing these patches is available here:
> https://github.com/ashkalra/edk2/tree/sev_live_migration_v4
>
> Changes since v3:
> - Fix all DSC files under OvmfPkg except X64 to add support for
> BaseMemEncryptLib and add NULL instance of BaseMemEncryptLib
> for 32 bit platforms.
> - Add the MemEncryptHypercallLib-related files to Maintainers.txt,
> in section "OvmfPkg: Confidential Computing".
> - Add support for the new KVM_HC_MAP_GPA_RANGE hypercall interface.
> - Add patch for SEV live migration support.
I have absolutely zero context in my mind about this work.
By v1 / v2 / v3, are you referring to the following patch series (from December 2020):
- [PATCH v1 0/2] SEV Page Encryption Bitmap support for OVMF.
https://listman.redhat.com/archives/edk2-devel-archive/2020-December/msg00081.html
- [PATCH v2 0/3] SEV Page Encryption Bitmap support for OVMF.
https://listman.redhat.com/archives/edk2-devel-archive/2020-December/msg00198.html
- [PATCH v3 0/3] SEV Page Encryption Bitmap support for OVMF.
https://listman.redhat.com/archives/edk2-devel-archive/2020-December/msg00202.html
We certainly need a new TianoCore BZ for tracking this feature; I only found the above patch set versions because I have full text search for my complete email traffic on my laptop. Sending v4 after half a year hiatus is like sending it in the next century. :)
Anyway, where I'm particularly lost is that I (very vaguely) recall conflicting approaches from AMD and IBM on migration. Has an agreement been reached there?
I certainly apologize for missing the context here; had someone asked me if I had seen any version of this patch set before, I would have *sworn* that I hadn't.
I'm basically incapable of tracking this volume of development around confidential computing; sorry.
Laszlo
>
> Changes since v2:
> - GHCB_BASE setup during reset-vector as decrypted is marked explicitly
> in the hypervisor page encryption bitmap after setting the
> PcdSevEsIsEnabled PCD.
>
> Changes since v1:
> - Mark GHCB_BASE setup during reset-vector as decrypted explicitly in
> the hypervisor page encryption bitmap.
> - Resending the series with correct shallow threading.
>
> Ashish Kalra (3):
> OvmfPkg/MemEncryptHypercallLib: add library to support SEV hypercalls.
> OvmfPkg/PlatformPei: Mark SEC GHCB page as unencrypted via hypercall
> OvmfPkg/PlatformDxe: Add support for SEV live migration.
>
> Brijesh Singh (1):
> OvmfPkg/BaseMemEncryptLib: Support to issue unencrypted hypercall
>
> Maintainers.txt | 2 +
> OvmfPkg/Include/Guid/MemEncryptLib.h | 20 ++++
> .../Include/Library/MemEncryptHypercallLib.h | 43 +++++++
> .../DxeMemEncryptSevLib.inf | 1 +
> .../PeiMemEncryptSevLib.inf | 1 +
> .../X64/PeiDxeVirtualMemory.c | 22 ++++
> .../Ia32/MemEncryptHypercallLib.c | 37 ++++++
> .../MemEncryptHypercallLib.inf | 42 +++++++
> .../X64/AsmHelperStub.nasm | 28 +++++
> .../X64/MemEncryptHypercallLib.c | 105 +++++++++++++++++
> OvmfPkg/OvmfPkg.dec | 1 +
> OvmfPkg/OvmfPkgIa32.dsc | 1 +
> OvmfPkg/OvmfPkgIa32X64.dsc | 1 +
> OvmfPkg/OvmfPkgX64.dsc | 1 +
> OvmfPkg/OvmfXen.dsc | 1 +
> OvmfPkg/PlatformDxe/AmdSev.c | 108 ++++++++++++++++++
> OvmfPkg/PlatformDxe/Platform.c | 5 +
> OvmfPkg/PlatformDxe/Platform.inf | 2 +
> OvmfPkg/PlatformDxe/PlatformConfig.h | 5 +
> OvmfPkg/PlatformPei/AmdSev.c | 10 ++
> 20 files changed, 436 insertions(+)
> create mode 100644 OvmfPkg/Include/Guid/MemEncryptLib.h
> create mode 100644 OvmfPkg/Include/Library/MemEncryptHypercallLib.h
> create mode 100644 OvmfPkg/Library/MemEncryptHypercallLib/Ia32/MemEncryptHypercallLib.c
> create mode 100644 OvmfPkg/Library/MemEncryptHypercallLib/MemEncryptHypercallLib.inf
> create mode 100644 OvmfPkg/Library/MemEncryptHypercallLib/X64/AsmHelperStub.nasm
> create mode 100644 OvmfPkg/Library/MemEncryptHypercallLib/X64/MemEncryptHypercallLib.c
> create mode 100644 OvmfPkg/PlatformDxe/AmdSev.c
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v4 0/4] SEV Live Migration support for OVMF.
2021-06-22 17:20 ` [PATCH v4 0/4] SEV Live Migration support for OVMF Laszlo Ersek
@ 2021-06-22 17:45 ` Brijesh Singh
2021-06-22 17:46 ` Ashish Kalra
1 sibling, 0 replies; 25+ messages in thread
From: Brijesh Singh @ 2021-06-22 17:45 UTC (permalink / raw)
To: Laszlo Ersek, Ashish Kalra, devel
Cc: brijesh.singh, Thomas.Lendacky, jejb, erdemaktas, jiewen.yao,
min.m.xu, jordan.l.justen, ard.biesheuvel, Dr. David Alan Gilbert,
Paolo Bonzini
Hi Ashish,
I have queue'd to review this series for later part of the week.
Just curious, did you run CI on this series ? A quick glance hints that this
series may fail to build on some platforms and additionally have formatting
error.
P.S: If you don't know how to use EDK2 CI then buzz me off-list.
thanks
On 6/22/2021 12:20 PM, Laszlo Ersek wrote:
> Hi Ashish,
>
> (+Dave, +Paolo)
>
> On 06/21/21 15:56, Ashish Kalra wrote:
>> From: Ashish Kalra <ashish.kalra@amd.com>
>>
>> By default all the SEV guest memory regions are considered encrypted,
>> if a guest changes the encryption attribute of the page (e.g mark a
>> page as decrypted) then notify hypervisor. Hypervisor will need to
>> track the unencrypted pages. The information will be used during
>> guest live migration, guest page migration and guest debugging.
>>
>> The patch-set adds a new SEV and SEV-ES hypercall abstraction
>> library to support SEV Page encryption/decryption status hypercalls
>> for SEV and SEV-ES guests.
>>
>> BaseMemEncryptSevLib invokes hypercalls via this new hypercall library.
>>
>> The patch-set detects if it is running under KVM hypervisor and then
>> checks for SEV live migration feature support via KVM_FEATURE_CPUID,
>> if detected setup a new UEFI enviroment variable to indicate OVMF
>> support for SEV live migration.
>>
>> A branch containing these patches is available here:
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fashkalra%2Fedk2%2Ftree%2Fsev_live_migration_v4&data=04%7C01%7Cbrijesh.singh%40amd.com%7Cb6f0cd9ca0cb4203327908d935a21cb3%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637599792656890122%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=zwiAg6jzSPYtUA8UARYE6K39Q3VCJkhm9Ey00aGYC10%3D&reserved=0
>>
>> Changes since v3:
>> - Fix all DSC files under OvmfPkg except X64 to add support for
>> BaseMemEncryptLib and add NULL instance of BaseMemEncryptLib
>> for 32 bit platforms.
>> - Add the MemEncryptHypercallLib-related files to Maintainers.txt,
>> in section "OvmfPkg: Confidential Computing".
>> - Add support for the new KVM_HC_MAP_GPA_RANGE hypercall interface.
>> - Add patch for SEV live migration support.
>
> I have absolutely zero context in my mind about this work.
>
> By v1 / v2 / v3, are you referring to the following patch series (from December 2020):
>
> - [PATCH v1 0/2] SEV Page Encryption Bitmap support for OVMF.
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flistman.redhat.com%2Farchives%2Fedk2-devel-archive%2F2020-December%2Fmsg00081.html&data=04%7C01%7Cbrijesh.singh%40amd.com%7Cb6f0cd9ca0cb4203327908d935a21cb3%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637599792656890122%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=QkZUdYyeWREfXyx2%2B32chbp7dMzEVfBb78dEsecduFw%3D&reserved=0
>
> - [PATCH v2 0/3] SEV Page Encryption Bitmap support for OVMF.
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flistman.redhat.com%2Farchives%2Fedk2-devel-archive%2F2020-December%2Fmsg00198.html&data=04%7C01%7Cbrijesh.singh%40amd.com%7Cb6f0cd9ca0cb4203327908d935a21cb3%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637599792656900118%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=TH%2BbYo%2B2CZyOunhIpegEjqQkdXlBuZsiyWz1k%2BGXtQc%3D&reserved=0
>
> - [PATCH v3 0/3] SEV Page Encryption Bitmap support for OVMF.
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flistman.redhat.com%2Farchives%2Fedk2-devel-archive%2F2020-December%2Fmsg00202.html&data=04%7C01%7Cbrijesh.singh%40amd.com%7Cb6f0cd9ca0cb4203327908d935a21cb3%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637599792656900118%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=css90wZ%2BFgbYm%2FQjvCLIFZwwozZz3dfzaVPDpsQsCsk%3D&reserved=0
>
> We certainly need a new TianoCore BZ for tracking this feature; I only found the above patch set versions because I have full text search for my complete email traffic on my laptop. Sending v4 after half a year hiatus is like sending it in the next century. :)
>
> Anyway, where I'm particularly lost is that I (very vaguely) recall conflicting approaches from AMD and IBM on migration. Has an agreement been reached there?
>
> I certainly apologize for missing the context here; had someone asked me if I had seen any version of this patch set before, I would have *sworn* that I hadn't.
>
> I'm basically incapable of tracking this volume of development around confidential computing; sorry.
>
> Laszlo
>
>>
>> Changes since v2:
>> - GHCB_BASE setup during reset-vector as decrypted is marked explicitly
>> in the hypervisor page encryption bitmap after setting the
>> PcdSevEsIsEnabled PCD.
>>
>> Changes since v1:
>> - Mark GHCB_BASE setup during reset-vector as decrypted explicitly in
>> the hypervisor page encryption bitmap.
>> - Resending the series with correct shallow threading.
>>
>> Ashish Kalra (3):
>> OvmfPkg/MemEncryptHypercallLib: add library to support SEV hypercalls.
>> OvmfPkg/PlatformPei: Mark SEC GHCB page as unencrypted via hypercall
>> OvmfPkg/PlatformDxe: Add support for SEV live migration.
>>
>> Brijesh Singh (1):
>> OvmfPkg/BaseMemEncryptLib: Support to issue unencrypted hypercall
>>
>> Maintainers.txt | 2 +
>> OvmfPkg/Include/Guid/MemEncryptLib.h | 20 ++++
>> .../Include/Library/MemEncryptHypercallLib.h | 43 +++++++
>> .../DxeMemEncryptSevLib.inf | 1 +
>> .../PeiMemEncryptSevLib.inf | 1 +
>> .../X64/PeiDxeVirtualMemory.c | 22 ++++
>> .../Ia32/MemEncryptHypercallLib.c | 37 ++++++
>> .../MemEncryptHypercallLib.inf | 42 +++++++
>> .../X64/AsmHelperStub.nasm | 28 +++++
>> .../X64/MemEncryptHypercallLib.c | 105 +++++++++++++++++
>> OvmfPkg/OvmfPkg.dec | 1 +
>> OvmfPkg/OvmfPkgIa32.dsc | 1 +
>> OvmfPkg/OvmfPkgIa32X64.dsc | 1 +
>> OvmfPkg/OvmfPkgX64.dsc | 1 +
>> OvmfPkg/OvmfXen.dsc | 1 +
>> OvmfPkg/PlatformDxe/AmdSev.c | 108 ++++++++++++++++++
>> OvmfPkg/PlatformDxe/Platform.c | 5 +
>> OvmfPkg/PlatformDxe/Platform.inf | 2 +
>> OvmfPkg/PlatformDxe/PlatformConfig.h | 5 +
>> OvmfPkg/PlatformPei/AmdSev.c | 10 ++
>> 20 files changed, 436 insertions(+)
>> create mode 100644 OvmfPkg/Include/Guid/MemEncryptLib.h
>> create mode 100644 OvmfPkg/Include/Library/MemEncryptHypercallLib.h
>> create mode 100644 OvmfPkg/Library/MemEncryptHypercallLib/Ia32/MemEncryptHypercallLib.c
>> create mode 100644 OvmfPkg/Library/MemEncryptHypercallLib/MemEncryptHypercallLib.inf
>> create mode 100644 OvmfPkg/Library/MemEncryptHypercallLib/X64/AsmHelperStub.nasm
>> create mode 100644 OvmfPkg/Library/MemEncryptHypercallLib/X64/MemEncryptHypercallLib.c
>> create mode 100644 OvmfPkg/PlatformDxe/AmdSev.c
>>
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v4 0/4] SEV Live Migration support for OVMF.
2021-06-22 17:20 ` [PATCH v4 0/4] SEV Live Migration support for OVMF Laszlo Ersek
2021-06-22 17:45 ` Brijesh Singh
@ 2021-06-22 17:46 ` Ashish Kalra
2021-06-23 13:18 ` [edk2-devel] " Dov Murik
2021-06-23 16:42 ` Laszlo Ersek
1 sibling, 2 replies; 25+ messages in thread
From: Ashish Kalra @ 2021-06-22 17:46 UTC (permalink / raw)
To: Laszlo Ersek
Cc: devel, brijesh.singh, Thomas.Lendacky, jejb, erdemaktas,
jiewen.yao, min.m.xu, jordan.l.justen, ard.biesheuvel,
Dr. David Alan Gilbert, Paolo Bonzini
Hello Laszlo,
Please see my replies below :
On Tue, Jun 22, 2021 at 07:20:53PM +0200, Laszlo Ersek wrote:
> Hi Ashish,
>
> (+Dave, +Paolo)
>
> On 06/21/21 15:56, Ashish Kalra wrote:
> > From: Ashish Kalra <ashish.kalra@amd.com>
> >
> > By default all the SEV guest memory regions are considered encrypted,
> > if a guest changes the encryption attribute of the page (e.g mark a
> > page as decrypted) then notify hypervisor. Hypervisor will need to
> > track the unencrypted pages. The information will be used during
> > guest live migration, guest page migration and guest debugging.
> >
> > The patch-set adds a new SEV and SEV-ES hypercall abstraction
> > library to support SEV Page encryption/decryption status hypercalls
> > for SEV and SEV-ES guests.
> >
> > BaseMemEncryptSevLib invokes hypercalls via this new hypercall library.
> >
> > The patch-set detects if it is running under KVM hypervisor and then
> > checks for SEV live migration feature support via KVM_FEATURE_CPUID,
> > if detected setup a new UEFI enviroment variable to indicate OVMF
> > support for SEV live migration.
> >
> > A branch containing these patches is available here:
> > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fashkalra%2Fedk2%2Ftree%2Fsev_live_migration_v4&data=04%7C01%7Cashish.kalra%40amd.com%7Ca402470f4cba428feaac08d935a21c79%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637599792647899814%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=41Jj%2BGNUyEL43UhZgI19iwGcOJcP%2FWg94D8fTopaZxQ%3D&reserved=0
> >
> > Changes since v3:
> > - Fix all DSC files under OvmfPkg except X64 to add support for
> > BaseMemEncryptLib and add NULL instance of BaseMemEncryptLib
> > for 32 bit platforms.
> > - Add the MemEncryptHypercallLib-related files to Maintainers.txt,
> > in section "OvmfPkg: Confidential Computing".
> > - Add support for the new KVM_HC_MAP_GPA_RANGE hypercall interface.
> > - Add patch for SEV live migration support.
>
> I have absolutely zero context in my mind about this work.
>
> By v1 / v2 / v3, are you referring to the following patch series (from December 2020):
>
> - [PATCH v1 0/2] SEV Page Encryption Bitmap support for OVMF.
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flistman.redhat.com%2Farchives%2Fedk2-devel-archive%2F2020-December%2Fmsg00081.html&data=04%7C01%7Cashish.kalra%40amd.com%7Ca402470f4cba428feaac08d935a21c79%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637599792647899814%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=cVUvWe6VgjR78OAk5LXgBQiKon4Gp%2F62a2hc%2FKwoLw4%3D&reserved=0
>
> - [PATCH v2 0/3] SEV Page Encryption Bitmap support for OVMF.
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flistman.redhat.com%2Farchives%2Fedk2-devel-archive%2F2020-December%2Fmsg00198.html&data=04%7C01%7Cashish.kalra%40amd.com%7Ca402470f4cba428feaac08d935a21c79%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637599792647899814%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=IXLGA2ttyxdVoC63HeCkPVNuUMH3u5Vd3U1fc6c8LQc%3D&reserved=0
>
> - [PATCH v3 0/3] SEV Page Encryption Bitmap support for OVMF.
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flistman.redhat.com%2Farchives%2Fedk2-devel-archive%2F2020-December%2Fmsg00202.html&data=04%7C01%7Cashish.kalra%40amd.com%7Ca402470f4cba428feaac08d935a21c79%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637599792647899814%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=cOJBqGyM3a7xMbfhbBAh3IEm7IBJFOGu2ReQMSFJ%2BDw%3D&reserved=0
>
Yes, actually the guest kernel API for SEV live migration was not
decided at the time of the last patch-set submission, hence, i am now
submitting this patch-set as the guest kernel API has been discussed and
closed and the corresponding KVM/kernel patches have been queued now.
And therefore, this is simply not the SEV Page Encryption Bitmap support
anymore, but the SEV live migration support which includes the guest page
encryption status tracking.
> We certainly need a new TianoCore BZ for tracking this feature; I only found the above patch set versions because I have full text search for my complete email traffic on my laptop. Sending v4 after half a year hiatus is like sending it in the next century. :)
>
Ok.
As i mentioned above it took such a long time to re-submit the
patch-set because of the guest kernel API discussions taking some
time and getting closed.
> Anyway, where I'm particularly lost is that I (very vaguely) recall conflicting approaches from AMD and IBM on migration. Has an agreement been reached there?
>
Yes, these are the slow SEV live migration patches. The fast migration
support is being developed by IBM and that is a separate effort.
As this slow live migration support has now been included in KVM, we
will need the corresponding OVMF and QEMU support now.
> I certainly apologize for missing the context here; had someone asked me if I had seen any version of this patch set before, I would have *sworn* that I hadn't.
>
> I'm basically incapable of tracking this volume of development around confidential computing; sorry.
>
>
Please find below your reply on v3 of this patch-set :
Please include such a patch in v4 -- if Tom and Brijesh agree, I'd like to put the new lib explicitly under their reviewership.
Also, I plan to review this series (v4, at this point) only for formalities. I'd like to receive an R-b from Tom or Brijesh [*], and another from Dov or a colleague at IBM, for this series; those together should suffice for merging the library.
So, if you are fine with this approach, then probably Tom or Brijesh can
take these patches under their reviewership and provide their R-b for
this patch-set to be accepted and merged.
I believe that they are anyway the maintainers for confidential computing related stuff.
Thanks,
Ashish
> >
> > Changes since v2:
> > - GHCB_BASE setup during reset-vector as decrypted is marked explicitly
> > in the hypervisor page encryption bitmap after setting the
> > PcdSevEsIsEnabled PCD.
> >
> > Changes since v1:
> > - Mark GHCB_BASE setup during reset-vector as decrypted explicitly in
> > the hypervisor page encryption bitmap.
> > - Resending the series with correct shallow threading.
> >
> > Ashish Kalra (3):
> > OvmfPkg/MemEncryptHypercallLib: add library to support SEV hypercalls.
> > OvmfPkg/PlatformPei: Mark SEC GHCB page as unencrypted via hypercall
> > OvmfPkg/PlatformDxe: Add support for SEV live migration.
> >
> > Brijesh Singh (1):
> > OvmfPkg/BaseMemEncryptLib: Support to issue unencrypted hypercall
> >
> > Maintainers.txt | 2 +
> > OvmfPkg/Include/Guid/MemEncryptLib.h | 20 ++++
> > .../Include/Library/MemEncryptHypercallLib.h | 43 +++++++
> > .../DxeMemEncryptSevLib.inf | 1 +
> > .../PeiMemEncryptSevLib.inf | 1 +
> > .../X64/PeiDxeVirtualMemory.c | 22 ++++
> > .../Ia32/MemEncryptHypercallLib.c | 37 ++++++
> > .../MemEncryptHypercallLib.inf | 42 +++++++
> > .../X64/AsmHelperStub.nasm | 28 +++++
> > .../X64/MemEncryptHypercallLib.c | 105 +++++++++++++++++
> > OvmfPkg/OvmfPkg.dec | 1 +
> > OvmfPkg/OvmfPkgIa32.dsc | 1 +
> > OvmfPkg/OvmfPkgIa32X64.dsc | 1 +
> > OvmfPkg/OvmfPkgX64.dsc | 1 +
> > OvmfPkg/OvmfXen.dsc | 1 +
> > OvmfPkg/PlatformDxe/AmdSev.c | 108 ++++++++++++++++++
> > OvmfPkg/PlatformDxe/Platform.c | 5 +
> > OvmfPkg/PlatformDxe/Platform.inf | 2 +
> > OvmfPkg/PlatformDxe/PlatformConfig.h | 5 +
> > OvmfPkg/PlatformPei/AmdSev.c | 10 ++
> > 20 files changed, 436 insertions(+)
> > create mode 100644 OvmfPkg/Include/Guid/MemEncryptLib.h
> > create mode 100644 OvmfPkg/Include/Library/MemEncryptHypercallLib.h
> > create mode 100644 OvmfPkg/Library/MemEncryptHypercallLib/Ia32/MemEncryptHypercallLib.c
> > create mode 100644 OvmfPkg/Library/MemEncryptHypercallLib/MemEncryptHypercallLib.inf
> > create mode 100644 OvmfPkg/Library/MemEncryptHypercallLib/X64/AsmHelperStub.nasm
> > create mode 100644 OvmfPkg/Library/MemEncryptHypercallLib/X64/MemEncryptHypercallLib.c
> > create mode 100644 OvmfPkg/PlatformDxe/AmdSev.c
> >
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v4 1/4] OvmfPkg/MemEncryptHypercallLib: add library to support SEV hypercalls.
2021-06-21 13:56 ` [PATCH v4 1/4] OvmfPkg/MemEncryptHypercallLib: add library to support SEV hypercalls Ashish Kalra
@ 2021-06-22 19:47 ` Brijesh Singh
2021-06-22 19:58 ` Brijesh Singh
2021-06-22 22:47 ` Lendacky, Thomas
1 sibling, 1 reply; 25+ messages in thread
From: Brijesh Singh @ 2021-06-22 19:47 UTC (permalink / raw)
To: Ashish Kalra, devel
Cc: brijesh.singh, Thomas.Lendacky, jejb, erdemaktas, jiewen.yao,
min.m.xu, lersek, jordan.l.justen, ard.biesheuvel
On 6/21/2021 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.
>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
>
Remove this newline.
> 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>
^^^^
2021
> +
> + 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)
> +#define KVM_MAP_GPA_RANGE_PAGE_SZ_1G (1 << 1)
> +#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).
> +
Looking at the function signature it seems this routine is used for both set
and clear. Please update the comment accordingly.
> + @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>
^^^^
2021
> +
> + 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
> +
> +**/
> +
> +VOID
> +EFIAPI
> +SetMemoryEncDecHypercall3 (
> + IN PHYSICAL_ADDRESS PhysicalAddress,
> + IN UINTN Pages,
> + 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>
^^^^
2021
> +#
> +# 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
> + 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>
^^^^
2021
> +
> + 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
> + );
> +
The function signature does not match with documented signature. Fix the
SetMemoryEncDecHypercall3AsmStub() documented in AsmHelperStub.nasm to use
UINTN.
> +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);
> +}
> +
This looks similar to VmgSetOffsetValid().
> +/**
> + This hyercall is used to notify hypervisor when a page is marked as
> + 'decrypted' (i.e C-bit removed).
> +Please update the comment.
> + @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));
You need to issue an SEV-ES guest termination vmexit followed by a deadloop to ensure
that boot does not proceed.
You probably also need to check for the RAX register for the return code.
> + }
> + VmgDone (Ghcb, InterruptState);
> + } else {
> + SetMemoryEncDecHypercall3AsmStub (
> + KVM_HC_MAP_GPA_RANGE,
> + PhysicalAddress,
> + Pages,
> + Mode
> + );
How do you know whether the hyperviosr supports the Live migration ? In other
words, is it safe to call the HC without knowing if HV supports the feature ?
Also, what will happen if we pass a bogus GPA. Does the HC return an error ?
Same as SEV-ES block, you probably need to check the RAX register for the
return code. On failure, cause an assert() and terminate the boot.
> + }
> +}
> 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
>
Update the AmdSev.dsc to include this library.
-Brijesh
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v4 1/4] OvmfPkg/MemEncryptHypercallLib: add library to support SEV hypercalls.
2021-06-22 19:47 ` Brijesh Singh
@ 2021-06-22 19:58 ` Brijesh Singh
0 siblings, 0 replies; 25+ messages in thread
From: Brijesh Singh @ 2021-06-22 19:58 UTC (permalink / raw)
To: Ashish Kalra, devel
Cc: brijesh.singh, Thomas.Lendacky, jejb, erdemaktas, jiewen.yao,
min.m.xu, lersek, jordan.l.justen, ard.biesheuvel
On 6/22/2021 2:47 PM, Brijesh Singh wrote:
>
>
> On 6/21/2021 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.
>>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
>>
> Remove this newline.
>
>> 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>
> ^^^^
> 2021
>
>> +
>> + 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)
>> +#define KVM_MAP_GPA_RANGE_PAGE_SZ_1G (1 << 1)
>> +#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).
>> +
> Looking at the function signature it seems this routine is used for both set
> and clear. Please update the comment accordingly.
>
>
>> + @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>
> ^^^^
> 2021
>
>> +
>> + 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
>> +
>> +**/
>> +
>> +VOID
>> +EFIAPI
>> +SetMemoryEncDecHypercall3 (
>> + IN PHYSICAL_ADDRESS PhysicalAddress,
>> + IN UINTN Pages,
>> + 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>
> ^^^^
> 2021
>> +#
>> +# 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
>> + 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>
> ^^^^
> 2021
>> +
>> + 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
>> + );
>> +
> The function signature does not match with documented signature. Fix the
> SetMemoryEncDecHypercall3AsmStub() documented in AsmHelperStub.nasm to use
> UINTN.
>
>
>> +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);
>> +}
>> +
>
> This looks similar to VmgSetOffsetValid().
>
>> +/**
>> + This hyercall is used to notify hypervisor when a page is marked as
>> + 'decrypted' (i.e C-bit removed).
>> +Please update the comment.
>
>> + @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));
>
> You need to issue an SEV-ES guest termination vmexit followed by a deadloop to ensure
> that boot does not proceed.
>
> You probably also need to check for the RAX register for the return code.
>
>
>> + }
>> + VmgDone (Ghcb, InterruptState);
>> + } else {
>> + SetMemoryEncDecHypercall3AsmStub (
>> + KVM_HC_MAP_GPA_RANGE,
>> + PhysicalAddress,
>> + Pages,
>> + Mode
>> + );
>
> How do you know whether the hyperviosr supports the Live migration ? In other
> words, is it safe to call the HC without knowing if HV supports the feature ?
>
I see that in patch#4 you are adding a support to query the HV features flag
to determine whether HV supports the Live migration. You probably need to
pull that function into this library and and check the feature flag before
invoking HC.
This library can also export the function for others to use. You can cache
the value on the first call (similar to how BaseMemEncryptSevLib does for
the MemEncryptSevIsEnabled).
> Also, what will happen if we pass a bogus GPA. Does the HC return an error ?
> Same as SEV-ES block, you probably need to check the RAX register for the
> return code. On failure, cause an assert() and terminate the boot.
>
>
>> + }
>> +}
>> 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
>>
> Update the AmdSev.dsc to include this library.
>
>
> -Brijesh
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v4 3/4] OvmfPkg/PlatformPei: Mark SEC GHCB page as unencrypted via hypercall
2021-06-21 13:57 ` [PATCH v4 3/4] OvmfPkg/PlatformPei: Mark SEC GHCB page as unencrypted via hypercall Ashish Kalra
@ 2021-06-22 20:35 ` Brijesh Singh
0 siblings, 0 replies; 25+ messages in thread
From: Brijesh Singh @ 2021-06-22 20:35 UTC (permalink / raw)
To: Ashish Kalra, devel
Cc: brijesh.singh, Thomas.Lendacky, jejb, erdemaktas, jiewen.yao,
min.m.xu, lersek, jordan.l.justen, ard.biesheuvel
On 6/21/2021 8:57 AM, Ashish Kalra wrote:
> From: Ashish Kalra <ashish.kalra@amd.com>
>
> Mark the SEC GHCB page (that is mapped as unencrypted in
> ResetVector code) in the hypervisor page status tracking.
>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
>
Remove this new line.
> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
> ---
> OvmfPkg/PlatformPei/AmdSev.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
> index a8bf610022..3f642ecb06 100644
> --- a/OvmfPkg/PlatformPei/AmdSev.c
> +++ b/OvmfPkg/PlatformPei/AmdSev.c
> @@ -15,6 +15,7 @@
> #include <Library/HobLib.h>
> #include <Library/MemEncryptSevLib.h>
> #include <Library/MemoryAllocationLib.h>
> +#include <Library/MemEncryptHypercallLib.h>
> #include <Library/PcdLib.h>
> #include <PiPei.h>
> #include <Register/Amd/Msr.h>
> @@ -52,6 +53,15 @@ AmdSevEsInitialize (
> PcdStatus = PcdSetBoolS (PcdSevEsIsEnabled, TRUE);
> ASSERT_RETURN_ERROR (PcdStatus);
>
> + //
> + // GHCB_BASE setup during reset-vector needs to be marked as
> + // decrypted in the hypervisor page encryption bitmap.
> + //
> + SetMemoryEncDecHypercall3 (FixedPcdGet32 (PcdOvmfSecGhcbBase),
> + EFI_SIZE_TO_PAGES(FixedPcdGet32 (PcdOvmfSecGhcbSize)),
> + KVM_MAP_GPA_RANGE_DECRYPTED
> + );
> +
Typically we should invoke the HC as soon as the page state is changed in the PTE.
Why we are notifying it too late? Is this because you are trying to avoid asm code
or there is no MSR protocol for VMMCALL NAE ?
I am okay with not notifying in ASM code, but at least we should notify the change
during the ES protocol negotiation and before the GHCB is setup. In other words,
do it inside the SevEsProtocolCheck() [Sec/SecMain.c].
-Brijesh
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v4 1/4] OvmfPkg/MemEncryptHypercallLib: add library to support SEV hypercalls.
2021-06-21 13:56 ` [PATCH v4 1/4] OvmfPkg/MemEncryptHypercallLib: add library to support SEV hypercalls Ashish Kalra
2021-06-22 19:47 ` Brijesh Singh
@ 2021-06-22 22:47 ` Lendacky, Thomas
2021-06-22 23:20 ` Ashish Kalra
2021-06-23 1:47 ` Ashish Kalra
1 sibling, 2 replies; 25+ messages in thread
From: Lendacky, Thomas @ 2021-06-22 22:47 UTC (permalink / raw)
To: Ashish Kalra, devel
Cc: brijesh.singh, jejb, erdemaktas, jiewen.yao, min.m.xu, lersek,
jordan.l.justen, ard.biesheuvel
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
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v4 2/4] OvmfPkg/BaseMemEncryptLib: Support to issue unencrypted hypercall
2021-06-21 13:57 ` [PATCH v4 2/4] OvmfPkg/BaseMemEncryptLib: Support to issue unencrypted hypercall Ashish Kalra
@ 2021-06-22 22:50 ` Lendacky, Thomas
0 siblings, 0 replies; 25+ messages in thread
From: Lendacky, Thomas @ 2021-06-22 22:50 UTC (permalink / raw)
To: Ashish Kalra, devel
Cc: brijesh.singh, jejb, erdemaktas, jiewen.yao, min.m.xu, lersek,
jordan.l.justen, ard.biesheuvel
On 6/21/21 8:57 AM, Ashish Kalra wrote:
> From: Brijesh Singh <brijesh.singh@amd.com>
>
> By default all the SEV guest memory regions are considered encrypted,
> if a guest changes the encryption attribute of the page (e.g mark a
> page as decrypted) then notify hypervisor. Hypervisor will need to
> track the unencrypted pages. The information will be used during
> guest live migration, guest page migration and guest debugging.
>
> Invoke hypercall via the new hypercall library.
>
> This hypercall is used to notify hypervisor when a page is marked as
> 'decrypted' (i.e C-bit removed).
>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
>
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
> ---
> OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf | 1 +
> OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf | 1 +
> OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c | 22 ++++++++++++++++++++
> 3 files changed, 24 insertions(+)
>
> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf b/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf
> index f2e162d680..aefcd7c0f7 100644
> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf
> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf
> @@ -49,6 +49,7 @@
> DebugLib
> MemoryAllocationLib
> PcdLib
> + MemEncryptHypercallLib
>
> [FeaturePcd]
> gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire
> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf
> index 03a78c32df..7503f56a0b 100644
> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf
> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf
> @@ -49,6 +49,7 @@
> DebugLib
> MemoryAllocationLib
> PcdLib
> + MemEncryptHypercallLib
>
> [FeaturePcd]
> gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire
> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
> index c696745f9d..12b3a9fcfb 100644
> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
> @@ -15,6 +15,7 @@
> #include <Library/MemEncryptSevLib.h>
> #include <Register/Amd/Cpuid.h>
> #include <Register/Cpuid.h>
> +#include <Library/MemEncryptHypercallLib.h>
>
> #include "VirtualMemory.h"
>
> @@ -585,6 +586,9 @@ SetMemoryEncDec (
> UINT64 AddressEncMask;
> BOOLEAN IsWpEnabled;
> RETURN_STATUS Status;
> + UINTN Size;
> + BOOLEAN CBitChanged;
> + PHYSICAL_ADDRESS OrigPhysicalAddress;
>
> //
> // Set PageMapLevel4Entry to suppress incorrect compiler/analyzer warnings.
> @@ -636,6 +640,10 @@ SetMemoryEncDec (
>
> Status = EFI_SUCCESS;
>
> + Size = Length;
> + CBitChanged = FALSE;
> + OrigPhysicalAddress = PhysicalAddress;
> +
> while (Length != 0)
> {
> //
> @@ -695,6 +703,7 @@ SetMemoryEncDec (
> ));
> PhysicalAddress += BIT30;
> Length -= BIT30;
> + CBitChanged = TRUE;
> } else {
> //
> // We must split the page
> @@ -749,6 +758,7 @@ SetMemoryEncDec (
> SetOrClearCBit (&PageDirectory2MEntry->Uint64, Mode);
> PhysicalAddress += BIT21;
> Length -= BIT21;
> + CBitChanged = TRUE;
> } else {
> //
> // We must split up this page into 4K pages
> @@ -791,6 +801,7 @@ SetMemoryEncDec (
> SetOrClearCBit (&PageTableEntry->Uint64, Mode);
> PhysicalAddress += EFI_PAGE_SIZE;
> Length -= EFI_PAGE_SIZE;
> + CBitChanged = TRUE;
> }
> }
> }
> @@ -808,6 +819,17 @@ SetMemoryEncDec (
> //
> CpuFlushTlb();
>
> + //
> + // Notify Hypervisor on C-bit status
> + //
> + if (CBitChanged) {
> + SetMemoryEncDecHypercall3 (
> + OrigPhysicalAddress,
> + EFI_SIZE_TO_PAGES(Size),
> + KVM_MAP_GPA_RANGE_ENC_STAT(!Mode)
> + );
> + }
Maybe add a comment here that the "Mode" doesn't care whether the change
was for 4k vs 2mb vs 1gb. It is just tracking the total number of pages
changed (should you just remove those #defines for the page sizes then?)
which can be done by calculating everything at the 4k level.
Thanks,
Tom
> +
> Done:
> //
> // Restore page table write protection, if any.
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v4 4/4] OvmfPkg/PlatformDxe: Add support for SEV live migration.
2021-06-21 13:57 ` [PATCH v4 4/4] OvmfPkg/PlatformDxe: Add support for SEV live migration Ashish Kalra
@ 2021-06-22 23:06 ` Lendacky, Thomas
2021-06-24 16:29 ` Ashish Kalra
0 siblings, 1 reply; 25+ messages in thread
From: Lendacky, Thomas @ 2021-06-22 23:06 UTC (permalink / raw)
To: Ashish Kalra, devel
Cc: brijesh.singh, jejb, erdemaktas, jiewen.yao, min.m.xu, lersek,
jordan.l.justen, ard.biesheuvel
On 6/21/21 8:57 AM, Ashish Kalra wrote:
> From: Ashish Kalra <ashish.kalra@amd.com>
>
> Detect for KVM hypervisor and check for SEV live migration
> feature support via KVM_FEATURE_CPUID, if detected setup a new
> UEFI enviroment variable to indicate OVMF support for SEV
> live migration.
>
> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
> ---
> OvmfPkg/Include/Guid/MemEncryptLib.h | 20 ++++
> OvmfPkg/OvmfPkg.dec | 1 +
> OvmfPkg/PlatformDxe/AmdSev.c | 108 ++++++++++++++++++++
> OvmfPkg/PlatformDxe/Platform.c | 5 +
> OvmfPkg/PlatformDxe/Platform.inf | 2 +
> OvmfPkg/PlatformDxe/PlatformConfig.h | 5 +
> 6 files changed, 141 insertions(+)
>
> diff --git a/OvmfPkg/Include/Guid/MemEncryptLib.h b/OvmfPkg/Include/Guid/MemEncryptLib.h
> new file mode 100644
> index 0000000000..4c046ba439
> --- /dev/null
> +++ b/OvmfPkg/Include/Guid/MemEncryptLib.h
> @@ -0,0 +1,20 @@
> +/** @file
> +
> + AMD Memory Encryption GUID, define a new GUID for defining
> + new UEFI enviroment variables assocaiated with SEV Memory Encryption.
> +
> + Copyright (c) 2020, AMD Inc. All rights reserved.<BR>
> +
> + SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#ifndef __MEMENCRYPT_LIB_H__
> +#define __MEMENCRYPT_LIB_H__
> +
> +#define MEMENCRYPT_GUID \
> +{0x0cf29b71, 0x9e51, 0x433a, {0xa3, 0xb7, 0x81, 0xf3, 0xab, 0x16, 0xb8, 0x75}}
> +
> +extern EFI_GUID gMemEncryptGuid;
> +
> +#endif
> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
> index 6ae733f6e3..e452dc8494 100644
> --- a/OvmfPkg/OvmfPkg.dec
> +++ b/OvmfPkg/OvmfPkg.dec
> @@ -122,6 +122,7 @@
> gQemuKernelLoaderFsMediaGuid = {0x1428f772, 0xb64a, 0x441e, {0xb8, 0xc3, 0x9e, 0xbd, 0xd7, 0xf8, 0x93, 0xc7}}
> gGrubFileGuid = {0xb5ae312c, 0xbc8a, 0x43b1, {0x9c, 0x62, 0xeb, 0xb8, 0x26, 0xdd, 0x5d, 0x07}}
> gConfidentialComputingSecretGuid = {0xadf956ad, 0xe98c, 0x484c, {0xae, 0x11, 0xb5, 0x1c, 0x7d, 0x33, 0x64, 0x47}}
> + gMemEncryptGuid = {0x0cf29b71, 0x9e51, 0x433a, {0xa3, 0xb7, 0x81, 0xf3, 0xab, 0x16, 0xb8, 0x75}}
>
> [Ppis]
> # PPI whose presence in the PPI database signals that the TPM base address
> diff --git a/OvmfPkg/PlatformDxe/AmdSev.c b/OvmfPkg/PlatformDxe/AmdSev.c
> new file mode 100644
> index 0000000000..3dbf17a8cd
> --- /dev/null
> +++ b/OvmfPkg/PlatformDxe/AmdSev.c
Can this be done in OvmfPkg/AmdSevDxe/AmdSevDxe.c or is that too early?
> @@ -0,0 +1,108 @@
> +/**@file
> + Detect KVM hypervisor support for SEV live migration and if
> + detected, setup a new UEFI enviroment variable indicating
> + OVMF support for SEV live migration.
> +
> + Copyright (c) 2020, Advanced Micro Devices. All rights reserved.<BR>
> +
> + SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +//
> +// The package level header files this module uses
> +//
> +
> +#include <Library/BaseLib.h>
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +#include <Library/UefiRuntimeServicesTableLib.h>
> +#include <Guid/MemEncryptLib.h>
> +
> +#define KVM_FEATURE_MIGRATION_CONTROL 17
BIT17
> +
> +/**
> + Figures out if we are running inside KVM HVM and
> + KVM HVM supports SEV Live Migration feature.
> +
> + @retval TRUE KVM was detected and Live Migration supported
> + @retval FALSE KVM was not detected or Live Migration not supported
> +
> +**/
> +BOOLEAN
> +KvmDetectSevLiveMigrationFeature(
> + VOID
> + )
> +{
> + UINT8 Signature[13];
> + UINT32 mKvmLeaf = 0;
> + UINT32 RegEax, RegEbx, RegEcx, RegEdx;
> +
> + Signature[12] = '\0';
> + for (mKvmLeaf = 0x40000000; mKvmLeaf < 0x40010000; mKvmLeaf += 0x100) {
What's the reason for the loop? I would think that just checking
0x40000000 would be enough, so a comment seems to be warranted.
> + AsmCpuid (mKvmLeaf,
> + NULL,
> + (UINT32 *) &Signature[0],
> + (UINT32 *) &Signature[4],
> + (UINT32 *) &Signature[8]);
> +
> + if (!AsciiStrCmp ((CHAR8 *) Signature, "KVMKVMKVM\0\0\0")) {
> + DEBUG ((
> + DEBUG_ERROR,
DEBUG_INFO, it doesn't seem like an error.
> + "%a: KVM Detected, signature = %s\n",
> + __FUNCTION__,
> + Signature
> + ));
> +> + RegEax = 0x40000001;
Should this be mKvmLeaf + 1? It is confusing that you may check 0x40000100
and then not do 0x40000101.
> + RegEcx = 0;
> + AsmCpuid (0x40000001, &RegEax, &RegEbx, &RegEcx, &RegEdx);
> + if (RegEax & (1 << KVM_FEATURE_MIGRATION_CONTROL)) {
This needs to be (assuming BIT17 is used above):
if ((RegEax & KVM_FEATURE_MIGRATION_CONTROL) != 0) {
You must use an actual comparison if the variable is not a boolean.
> + DEBUG ((
> + DEBUG_ERROR,
> + "%a: Live Migration feature supported\n",
DEBUG_INFO again.
> + __FUNCTION__
> + ));
> + return TRUE;
> + }
> + }
> + }
> +
> + return FALSE;
> +}
> +
> +/**
> +
> + Function checks if SEV Live Migration support is available, if present then it sets
> + a UEFI enviroment variable to be queried later using Runtime services.
> +
> + **/
> +VOID
> +AmdSevSetConfig(
Kind of a generic name for what is being done.
> + VOID
> + )
> +{
> + EFI_STATUS Status;
> + BOOLEAN SevLiveMigrationEnabled;
> +
> + SevLiveMigrationEnabled = KvmDetectSevLiveMigrationFeature();
> +
> + if (SevLiveMigrationEnabled) {
> + Status = gRT->SetVariable (
> + L"SevLiveMigrationEnabled",
> + &gMemEncryptGuid,
> + EFI_VARIABLE_NON_VOLATILE |
> + EFI_VARIABLE_BOOTSERVICE_ACCESS |
> + EFI_VARIABLE_RUNTIME_ACCESS,
> + sizeof (BOOLEAN),
> + &SevLiveMigrationEnabled
> + );
> +
> + DEBUG ((
> + DEBUG_ERROR,
> + "%a: Setting SevLiveMigrationEnabled variable, status = %lx\n",
DEBUG_INFO.
Thanks,
Tom
> + __FUNCTION__,
> + Status
> + ));
> + }
> +}
> diff --git a/OvmfPkg/PlatformDxe/Platform.c b/OvmfPkg/PlatformDxe/Platform.c
> index f2e51960ce..f61302d98b 100644
> --- a/OvmfPkg/PlatformDxe/Platform.c
> +++ b/OvmfPkg/PlatformDxe/Platform.c
> @@ -763,6 +763,11 @@ PlatformInit (
> {
> EFI_STATUS Status;
>
> + //
> + // Set Amd Sev configuation
> + //
> + AmdSevSetConfig();
> +
> ExecutePlatformConfig ();
>
> mConfigAccess.ExtractConfig = &ExtractConfig;
> diff --git a/OvmfPkg/PlatformDxe/Platform.inf b/OvmfPkg/PlatformDxe/Platform.inf
> index 14727c1220..2896f0a1d1 100644
> --- a/OvmfPkg/PlatformDxe/Platform.inf
> +++ b/OvmfPkg/PlatformDxe/Platform.inf
> @@ -24,6 +24,7 @@
> PlatformConfig.c
> PlatformConfig.h
> PlatformForms.vfr
> + AmdSev.c
>
> [Packages]
> MdePkg/MdePkg.dec
> @@ -56,6 +57,7 @@
> [Guids]
> gEfiIfrTianoGuid
> gOvmfPlatformConfigGuid
> + gMemEncryptGuid
>
> [Depex]
> gEfiHiiConfigRoutingProtocolGuid AND
> diff --git a/OvmfPkg/PlatformDxe/PlatformConfig.h b/OvmfPkg/PlatformDxe/PlatformConfig.h
> index 716514da21..4f662aafa4 100644
> --- a/OvmfPkg/PlatformDxe/PlatformConfig.h
> +++ b/OvmfPkg/PlatformDxe/PlatformConfig.h
> @@ -44,6 +44,11 @@ PlatformConfigLoad (
> OUT UINT64 *OptionalElements
> );
>
> +VOID
> +AmdSevSetConfig(
> + VOID
> + );
> +
> //
> // Feature flags for OptionalElements.
> //
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v4 1/4] OvmfPkg/MemEncryptHypercallLib: add library to support SEV hypercalls.
2021-06-22 22:47 ` Lendacky, Thomas
@ 2021-06-22 23:20 ` Ashish Kalra
2021-06-22 23:38 ` Brijesh Singh
2021-06-23 1:47 ` Ashish Kalra
1 sibling, 1 reply; 25+ messages in thread
From: Ashish Kalra @ 2021-06-22 23:20 UTC (permalink / raw)
To: Tom Lendacky
Cc: devel, brijesh.singh, jejb, erdemaktas, jiewen.yao, min.m.xu,
lersek, jordan.l.justen, ard.biesheuvel
Hello Tom,
On Tue, Jun 22, 2021 at 05:47:48PM -0500, Tom Lendacky wrote:
...
> > +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?
>
IIRC, we have already discussed this internally.
Letting the VC handler do it was making it too complicated to add hooks
inside the VmgExitLib, and corresponding updation of MdePkg and UefiCpuPkg
(as described in the email thread below), and at that time
Brijesh had suggested the use of this alternative VmgExit() approach.
Email thread copied below :
...
Well, I does not mean that you should literally use VMMCALL instruction instead you use its corresponding VMGEXIT number.
Something like this:
Status = VmgExit (Ghcb, SVM_EXIT_VMMCALL, 0, 0);
This way, a #VC will not be kicked in and there is no need to hook anything inside the VmgExitLib. Maybe Tom can correct me if that is not acceptable.
-Brijesh
>> I am not able to follow your OVMF patches, could you provide a very high level overview of what exactly you are trying to achieve? Its possible that I am missing something fundamental but why do we care of Hypercall inside the bare metal pkg (e.g MdePkg, UefiCpuPkg)? Why we are needing a Hob etc ? IMO, a Hypercall implementation should be straight forward like what I did the SEV live migration. In case of ES, all you need to use the Ghcb instance to save the register values (rax, rbx, rcx etc) then invoke vmmcall instruction.
>
I need to do this hypercall validation and setup as part of VC# exception's VMMCALL handling,
i.e, in the VmgExitLib code. I need to use a HOB to store/cache hypercalls invoked during SEC
and PEI phase and flush them later at DXE IPL phase. As VmgExitLib code is invoked in SEC and
PEI phases and references MdePkg and UefiCpuPkg, i need to add HypercallLib references in UefiCpuPkg.
Also, i need to verify if Hypercall library interfaces are being invoked during SEC and/or PEI phase,
currently i do it by checking PcdOvmfSecGhcbBase and for accessing that i had to add reference to
it in MdePkg/MdePkg.dec and UefiCpuPkg/UefiCpuPkg.dec, if i can check that the Hypercall library
interfaces are being invoked in SEC/PEI phase using some other mechanism then i can drop this
reference to PcdOvmfSecGhcbBase.
...
Thanks,
Ashish
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v4 1/4] OvmfPkg/MemEncryptHypercallLib: add library to support SEV hypercalls.
2021-06-22 23:20 ` Ashish Kalra
@ 2021-06-22 23:38 ` Brijesh Singh
0 siblings, 0 replies; 25+ messages in thread
From: Brijesh Singh @ 2021-06-22 23:38 UTC (permalink / raw)
To: Ashish Kalra, Tom Lendacky
Cc: brijesh.singh, devel, jejb, erdemaktas, jiewen.yao, min.m.xu,
lersek, jordan.l.justen, ard.biesheuvel
On 6/22/2021 6:20 PM, Ashish Kalra wrote:
> Hello Tom,
>
> On Tue, Jun 22, 2021 at 05:47:48PM -0500, Tom Lendacky wrote:
> ...
>>> +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?
>>
>
> IIRC, we have already discussed this internally.
>
> Letting the VC handler do it was making it too complicated to add hooks
> inside the VmgExitLib, and corresponding updation of MdePkg and UefiCpuPkg
> (as described in the email thread below), and at that time
> Brijesh had suggested the use of this alternative VmgExit() approach.
>
A lot has changed in the OVMF code since you last submitted the patch.
IIRC, in ES wip patches Tom was implementing the VC handling library
in the EDK2 core. But in the final version, all the VC handling is
done in the OVMF. The EDK2 core provides a Null library that get
override from the OVMF. In other words, you no longer need to
touch the MdePkg and UefiCpuPkg etc for this change.
I agree with Tom that it would be nice if we can add smarts
in the VC handler. My previous comment was mainly focuses around
how you can avoid touching the EDK2 core.
> Email thread copied below :
>
> ...
> Well, I does not mean that you should literally use VMMCALL instruction instead you use its corresponding VMGEXIT number.
> Something like this:
>
> Status = VmgExit (Ghcb, SVM_EXIT_VMMCALL, 0, 0);
>
> This way, a #VC will not be kicked in and there is no need to hook anything inside the VmgExitLib. Maybe Tom can correct me if that is not acceptable.
>
> -Brijesh
>
>>> I am not able to follow your OVMF patches, could you provide a very high level overview of what exactly you are trying to achieve? Its possible that I am missing something fundamental but why do we care of Hypercall inside the bare metal pkg (e.g MdePkg, UefiCpuPkg)? Why we are needing a Hob etc ? IMO, a Hypercall implementation should be straight forward like what I did the SEV live migration. In case of ES, all you need to use the Ghcb instance to save the register values (rax, rbx, rcx etc) then invoke vmmcall instruction.
>>
> I need to do this hypercall validation and setup as part of VC# exception's VMMCALL handling,
> i.e, in the VmgExitLib code. I need to use a HOB to store/cache hypercalls invoked during SEC
> and PEI phase and flush them later at DXE IPL phase. As VmgExitLib code is invoked in SEC and
> PEI phases and references MdePkg and UefiCpuPkg, i need to add HypercallLib references in UefiCpuPkg.
>
> Also, i need to verify if Hypercall library interfaces are being invoked during SEC and/or PEI phase,
> currently i do it by checking PcdOvmfSecGhcbBase and for accessing that i had to add reference to
> it in MdePkg/MdePkg.dec and UefiCpuPkg/UefiCpuPkg.dec, if i can check that the Hypercall library
> interfaces are being invoked in SEC/PEI phase using some other mechanism then i can drop this
> reference to PcdOvmfSecGhcbBase.
> ...
>
>
> Thanks,
> Ashish
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v4 1/4] OvmfPkg/MemEncryptHypercallLib: add library to support SEV hypercalls.
2021-06-22 22:47 ` Lendacky, Thomas
2021-06-22 23:20 ` Ashish Kalra
@ 2021-06-23 1:47 ` Ashish Kalra
2021-06-23 15:02 ` Ashish Kalra
1 sibling, 1 reply; 25+ messages in thread
From: Ashish Kalra @ 2021-06-23 1:47 UTC (permalink / raw)
To: Tom Lendacky
Cc: devel, brijesh.singh, jejb, erdemaktas, jiewen.yao, min.m.xu,
lersek, jordan.l.justen, ard.biesheuvel
Hello Tom,
On Tue, Jun 22, 2021 at 05:47:48PM -0500, Tom Lendacky wrote:
> 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.
>
Actually i like this approach, instead of adding a new library.
Again, IIRC we had discussed this internally and then decided to
create a new hypercall library similar to Xen, with reference to
the email copied below :
...
Xen has support under Ovmf for a Xen hypercall library
(OvmfPkg/Library/XenHypercallLib). Take a look at that and maybe create
something similar for KVM.
...
Thanks,
Ashish
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [edk2-devel] [PATCH v4 0/4] SEV Live Migration support for OVMF.
2021-06-22 17:46 ` Ashish Kalra
@ 2021-06-23 13:18 ` Dov Murik
2021-06-23 16:42 ` Laszlo Ersek
1 sibling, 0 replies; 25+ messages in thread
From: Dov Murik @ 2021-06-23 13:18 UTC (permalink / raw)
To: devel, ashish.kalra, Laszlo Ersek
Cc: brijesh.singh, Thomas.Lendacky, jejb, erdemaktas, jiewen.yao,
min.m.xu, jordan.l.justen, ard.biesheuvel, Dr. David Alan Gilbert,
Paolo Bonzini, tobin@ibm.com
+cc Tobin
On 22/06/2021 20:46, Ashish Kalra via groups.io wrote:
> Hello Laszlo,
>
> Please see my replies below :
>
> On Tue, Jun 22, 2021 at 07:20:53PM +0200, Laszlo Ersek wrote:
-snip-
>
>> Anyway, where I'm particularly lost is that I (very vaguely) recall conflicting approaches from AMD and IBM on migration. Has an agreement been reached there?
>>
>
> Yes, these are the slow SEV live migration patches. The fast migration
> support is being developed by IBM and that is a separate effort.
>
> As this slow live migration support has now been included in KVM, we
> will need the corresponding OVMF and QEMU support now.
>
I'll also add that the fast (guest-assisted) SEV migration we (IBM) are
working on uses some of the features of the "slow" migration. For
example, for sure, patches 1-3 in this series will be needed to keep the
record of which of the guest pages are encrypted and which are shared
(also in fast migration).
-Dov
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v4 1/4] OvmfPkg/MemEncryptHypercallLib: add library to support SEV hypercalls.
2021-06-23 1:47 ` Ashish Kalra
@ 2021-06-23 15:02 ` Ashish Kalra
0 siblings, 0 replies; 25+ messages in thread
From: Ashish Kalra @ 2021-06-23 15:02 UTC (permalink / raw)
To: Tom Lendacky
Cc: devel, brijesh.singh, jejb, erdemaktas, jiewen.yao, min.m.xu,
lersek, jordan.l.justen, ard.biesheuvel, dgilbert, pbonzini,
tobin, dovmurik
Hello Tom, Brijesh,
Thanks for your comments and feedback on this patch-set.
With reference to those, i will remove the new library and
add the hypercall function inside BaseMemEncryptSevLib library.
Also i will let the hypercall handling for SEV-ES be done
as part of #VC exception handling in VmgExitLib library.
Thanks,
Ashish
On Wed, Jun 23, 2021 at 01:47:47AM +0000, Ashish Kalra wrote:
> Hello Tom,
>
> On Tue, Jun 22, 2021 at 05:47:48PM -0500, Tom Lendacky wrote:
> > 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.
> >
>
> Actually i like this approach, instead of adding a new library.
>
> Again, IIRC we had discussed this internally and then decided to
> create a new hypercall library similar to Xen, with reference to
> the email copied below :
>
> ...
> Xen has support under Ovmf for a Xen hypercall library
> (OvmfPkg/Library/XenHypercallLib). Take a look at that and maybe create
> something similar for KVM.
> ...
>
> Thanks,
> Ashish
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v4 0/4] SEV Live Migration support for OVMF.
2021-06-22 17:46 ` Ashish Kalra
2021-06-23 13:18 ` [edk2-devel] " Dov Murik
@ 2021-06-23 16:42 ` Laszlo Ersek
2021-06-23 16:49 ` Laszlo Ersek
1 sibling, 1 reply; 25+ messages in thread
From: Laszlo Ersek @ 2021-06-23 16:42 UTC (permalink / raw)
To: Ashish Kalra
Cc: devel, brijesh.singh, Thomas.Lendacky, jejb, erdemaktas,
jiewen.yao, min.m.xu, jordan.l.justen, ard.biesheuvel,
Dr. David Alan Gilbert, Paolo Bonzini
On 06/22/21 19:46, Ashish Kalra wrote:
> Hello Laszlo,
>
> Please see my replies below :
>
> On Tue, Jun 22, 2021 at 07:20:53PM +0200, Laszlo Ersek wrote:
>> Hi Ashish,
>>
>> (+Dave, +Paolo)
>>
>> On 06/21/21 15:56, Ashish Kalra wrote:
>>> From: Ashish Kalra <ashish.kalra@amd.com>
>>>
>>> By default all the SEV guest memory regions are considered encrypted,
>>> if a guest changes the encryption attribute of the page (e.g mark a
>>> page as decrypted) then notify hypervisor. Hypervisor will need to
>>> track the unencrypted pages. The information will be used during
>>> guest live migration, guest page migration and guest debugging.
>>>
>>> The patch-set adds a new SEV and SEV-ES hypercall abstraction
>>> library to support SEV Page encryption/decryption status hypercalls
>>> for SEV and SEV-ES guests.
>>>
>>> BaseMemEncryptSevLib invokes hypercalls via this new hypercall library.
>>>
>>> The patch-set detects if it is running under KVM hypervisor and then
>>> checks for SEV live migration feature support via KVM_FEATURE_CPUID,
>>> if detected setup a new UEFI enviroment variable to indicate OVMF
>>> support for SEV live migration.
>>>
>>> A branch containing these patches is available here:
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fashkalra%2Fedk2%2Ftree%2Fsev_live_migration_v4&data=04%7C01%7Cashish.kalra%40amd.com%7Ca402470f4cba428feaac08d935a21c79%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637599792647899814%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=41Jj%2BGNUyEL43UhZgI19iwGcOJcP%2FWg94D8fTopaZxQ%3D&reserved=0
>>>
>>> Changes since v3:
>>> - Fix all DSC files under OvmfPkg except X64 to add support for
>>> BaseMemEncryptLib and add NULL instance of BaseMemEncryptLib
>>> for 32 bit platforms.
>>> - Add the MemEncryptHypercallLib-related files to Maintainers.txt,
>>> in section "OvmfPkg: Confidential Computing".
>>> - Add support for the new KVM_HC_MAP_GPA_RANGE hypercall interface.
>>> - Add patch for SEV live migration support.
>>
>> I have absolutely zero context in my mind about this work.
>>
>> By v1 / v2 / v3, are you referring to the following patch series (from December 2020):
>>
>> - [PATCH v1 0/2] SEV Page Encryption Bitmap support for OVMF.
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flistman.redhat.com%2Farchives%2Fedk2-devel-archive%2F2020-December%2Fmsg00081.html&data=04%7C01%7Cashish.kalra%40amd.com%7Ca402470f4cba428feaac08d935a21c79%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637599792647899814%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=cVUvWe6VgjR78OAk5LXgBQiKon4Gp%2F62a2hc%2FKwoLw4%3D&reserved=0
>>
>> - [PATCH v2 0/3] SEV Page Encryption Bitmap support for OVMF.
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flistman.redhat.com%2Farchives%2Fedk2-devel-archive%2F2020-December%2Fmsg00198.html&data=04%7C01%7Cashish.kalra%40amd.com%7Ca402470f4cba428feaac08d935a21c79%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637599792647899814%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=IXLGA2ttyxdVoC63HeCkPVNuUMH3u5Vd3U1fc6c8LQc%3D&reserved=0
>>
>> - [PATCH v3 0/3] SEV Page Encryption Bitmap support for OVMF.
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flistman.redhat.com%2Farchives%2Fedk2-devel-archive%2F2020-December%2Fmsg00202.html&data=04%7C01%7Cashish.kalra%40amd.com%7Ca402470f4cba428feaac08d935a21c79%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637599792647899814%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=cOJBqGyM3a7xMbfhbBAh3IEm7IBJFOGu2ReQMSFJ%2BDw%3D&reserved=0
>>
>
> Yes, actually the guest kernel API for SEV live migration was not
> decided at the time of the last patch-set submission, hence, i am now
> submitting this patch-set as the guest kernel API has been discussed and
> closed and the corresponding KVM/kernel patches have been queued now.
>
> And therefore, this is simply not the SEV Page Encryption Bitmap support
> anymore, but the SEV live migration support which includes the guest page
> encryption status tracking.
>
>> We certainly need a new TianoCore BZ for tracking this feature; I only found the above patch set versions because I have full text search for my complete email traffic on my laptop. Sending v4 after half a year hiatus is like sending it in the next century. :)
>>
>
> Ok.
>
> As i mentioned above it took such a long time to re-submit the
> patch-set because of the guest kernel API discussions taking some
> time and getting closed.
>
>> Anyway, where I'm particularly lost is that I (very vaguely) recall conflicting approaches from AMD and IBM on migration. Has an agreement been reached there?
>>
>
> Yes, these are the slow SEV live migration patches. The fast migration
> support is being developed by IBM and that is a separate effort.
>
> As this slow live migration support has now been included in KVM, we
> will need the corresponding OVMF and QEMU support now.
>
>> I certainly apologize for missing the context here; had someone asked me if I had seen any version of this patch set before, I would have *sworn* that I hadn't.
>>
>> I'm basically incapable of tracking this volume of development around confidential computing; sorry.
>>
>>
>
> Please find below your reply on v3 of this patch-set :
>
> Please include such a patch in v4 -- if Tom and Brijesh agree, I'd like to put the new lib explicitly under their reviewership.
>
> Also, I plan to review this series (v4, at this point) only for formalities. I'd like to receive an R-b from Tom or Brijesh [*], and another from Dov or a colleague at IBM, for this series; those together should suffice for merging the library.
>
> So, if you are fine with this approach, then probably Tom or Brijesh can
> take these patches under their reviewership and provide their R-b for
> this patch-set to be accepted and merged.
Indeed. This helps. Thanks.
I'll keep this patch set in my review queue then, for said "formalities
review".
Thanks.
Laszlo
>
> I believe that they are anyway the maintainers for confidential computing related stuff.
>
> Thanks,
> Ashish
>
>>>
>>> Changes since v2:
>>> - GHCB_BASE setup during reset-vector as decrypted is marked explicitly
>>> in the hypervisor page encryption bitmap after setting the
>>> PcdSevEsIsEnabled PCD.
>>>
>>> Changes since v1:
>>> - Mark GHCB_BASE setup during reset-vector as decrypted explicitly in
>>> the hypervisor page encryption bitmap.
>>> - Resending the series with correct shallow threading.
>>>
>>> Ashish Kalra (3):
>>> OvmfPkg/MemEncryptHypercallLib: add library to support SEV hypercalls.
>>> OvmfPkg/PlatformPei: Mark SEC GHCB page as unencrypted via hypercall
>>> OvmfPkg/PlatformDxe: Add support for SEV live migration.
>>>
>>> Brijesh Singh (1):
>>> OvmfPkg/BaseMemEncryptLib: Support to issue unencrypted hypercall
>>>
>>> Maintainers.txt | 2 +
>>> OvmfPkg/Include/Guid/MemEncryptLib.h | 20 ++++
>>> .../Include/Library/MemEncryptHypercallLib.h | 43 +++++++
>>> .../DxeMemEncryptSevLib.inf | 1 +
>>> .../PeiMemEncryptSevLib.inf | 1 +
>>> .../X64/PeiDxeVirtualMemory.c | 22 ++++
>>> .../Ia32/MemEncryptHypercallLib.c | 37 ++++++
>>> .../MemEncryptHypercallLib.inf | 42 +++++++
>>> .../X64/AsmHelperStub.nasm | 28 +++++
>>> .../X64/MemEncryptHypercallLib.c | 105 +++++++++++++++++
>>> OvmfPkg/OvmfPkg.dec | 1 +
>>> OvmfPkg/OvmfPkgIa32.dsc | 1 +
>>> OvmfPkg/OvmfPkgIa32X64.dsc | 1 +
>>> OvmfPkg/OvmfPkgX64.dsc | 1 +
>>> OvmfPkg/OvmfXen.dsc | 1 +
>>> OvmfPkg/PlatformDxe/AmdSev.c | 108 ++++++++++++++++++
>>> OvmfPkg/PlatformDxe/Platform.c | 5 +
>>> OvmfPkg/PlatformDxe/Platform.inf | 2 +
>>> OvmfPkg/PlatformDxe/PlatformConfig.h | 5 +
>>> OvmfPkg/PlatformPei/AmdSev.c | 10 ++
>>> 20 files changed, 436 insertions(+)
>>> create mode 100644 OvmfPkg/Include/Guid/MemEncryptLib.h
>>> create mode 100644 OvmfPkg/Include/Library/MemEncryptHypercallLib.h
>>> create mode 100644 OvmfPkg/Library/MemEncryptHypercallLib/Ia32/MemEncryptHypercallLib.c
>>> create mode 100644 OvmfPkg/Library/MemEncryptHypercallLib/MemEncryptHypercallLib.inf
>>> create mode 100644 OvmfPkg/Library/MemEncryptHypercallLib/X64/AsmHelperStub.nasm
>>> create mode 100644 OvmfPkg/Library/MemEncryptHypercallLib/X64/MemEncryptHypercallLib.c
>>> create mode 100644 OvmfPkg/PlatformDxe/AmdSev.c
>>>
>>
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v4 0/4] SEV Live Migration support for OVMF.
2021-06-23 16:42 ` Laszlo Ersek
@ 2021-06-23 16:49 ` Laszlo Ersek
2021-06-23 17:03 ` Ashish Kalra
2021-06-30 9:11 ` Ashish Kalra
0 siblings, 2 replies; 25+ messages in thread
From: Laszlo Ersek @ 2021-06-23 16:49 UTC (permalink / raw)
To: Ashish Kalra
Cc: devel, brijesh.singh, Thomas.Lendacky, jejb, erdemaktas,
jiewen.yao, min.m.xu, jordan.l.justen, ard.biesheuvel,
Dr. David Alan Gilbert, Paolo Bonzini
On 06/23/21 18:42, Laszlo Ersek wrote:
> On 06/22/21 19:46, Ashish Kalra wrote:
>> Please find below your reply on v3 of this patch-set :
>>
>> Please include such a patch in v4 -- if Tom and Brijesh agree, I'd like to put the new lib explicitly under their reviewership.
>>
>> Also, I plan to review this series (v4, at this point) only for formalities. I'd like to receive an R-b from Tom or Brijesh [*], and another from Dov or a colleague at IBM, for this series; those together should suffice for merging the library.
>>
>> So, if you are fine with this approach, then probably Tom or Brijesh can
>> take these patches under their reviewership and provide their R-b for
>> this patch-set to be accepted and merged.
>
> Indeed. This helps. Thanks.
>
> I'll keep this patch set in my review queue then, for said "formalities
> review".
Please do file a TianoCore Feature Request BZ for this, and reference
the bug URL in the commit messages. One important purpose of such a BZ
is to succinctly link together all versions of a patch set -- that way
poor maintainers know where to find previous versions, even if the blurb
subject line changes over time. I also like to capture "permanent
workflow notes" like the above in BZs (basically a high-level summary of
who does what).
For now it seems that a v5 will be necessary. Please keep me on CC, and
when you have the BZ filed, we should link all past and future versions
into it.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v4 0/4] SEV Live Migration support for OVMF.
2021-06-23 16:49 ` Laszlo Ersek
@ 2021-06-23 17:03 ` Ashish Kalra
2021-06-30 9:11 ` Ashish Kalra
1 sibling, 0 replies; 25+ messages in thread
From: Ashish Kalra @ 2021-06-23 17:03 UTC (permalink / raw)
To: Laszlo Ersek
Cc: devel, brijesh.singh, Thomas.Lendacky, jejb, erdemaktas,
jiewen.yao, min.m.xu, jordan.l.justen, ard.biesheuvel,
Dr. David Alan Gilbert, Paolo Bonzini
Hello Laszlo,
Yes i will file a TianoCore Feature Request BZ for this and i am working
on a v5 for this patch-set.
Thanks,
Ashish
On Wed, Jun 23, 2021 at 06:49:06PM +0200, Laszlo Ersek wrote:
> On 06/23/21 18:42, Laszlo Ersek wrote:
> > On 06/22/21 19:46, Ashish Kalra wrote:
>
> >> Please find below your reply on v3 of this patch-set :
> >>
> >> Please include such a patch in v4 -- if Tom and Brijesh agree, I'd like to put the new lib explicitly under their reviewership.
> >>
> >> Also, I plan to review this series (v4, at this point) only for formalities. I'd like to receive an R-b from Tom or Brijesh [*], and another from Dov or a colleague at IBM, for this series; those together should suffice for merging the library.
> >>
> >> So, if you are fine with this approach, then probably Tom or Brijesh can
> >> take these patches under their reviewership and provide their R-b for
> >> this patch-set to be accepted and merged.
> >
> > Indeed. This helps. Thanks.
> >
> > I'll keep this patch set in my review queue then, for said "formalities
> > review".
>
> Please do file a TianoCore Feature Request BZ for this, and reference
> the bug URL in the commit messages. One important purpose of such a BZ
> is to succinctly link together all versions of a patch set -- that way
> poor maintainers know where to find previous versions, even if the blurb
> subject line changes over time. I also like to capture "permanent
> workflow notes" like the above in BZs (basically a high-level summary of
> who does what).
>
> For now it seems that a v5 will be necessary. Please keep me on CC, and
> when you have the BZ filed, we should link all past and future versions
> into it.
>
> Thanks
> Laszlo
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v4 4/4] OvmfPkg/PlatformDxe: Add support for SEV live migration.
2021-06-22 23:06 ` Lendacky, Thomas
@ 2021-06-24 16:29 ` Ashish Kalra
0 siblings, 0 replies; 25+ messages in thread
From: Ashish Kalra @ 2021-06-24 16:29 UTC (permalink / raw)
To: Tom Lendacky
Cc: devel, brijesh.singh, jejb, erdemaktas, jiewen.yao, min.m.xu,
lersek, jordan.l.justen, ard.biesheuvel, pbonzini, dgilbert,
dovmurik, tobin
Hello Tom,
On Tue, Jun 22, 2021 at 06:06:24PM -0500, Tom Lendacky wrote:
> > +
> > +/**
> > + Figures out if we are running inside KVM HVM and
> > + KVM HVM supports SEV Live Migration feature.
> > +
> > + @retval TRUE KVM was detected and Live Migration supported
> > + @retval FALSE KVM was not detected or Live Migration not supported
> > +
> > +**/
> > +BOOLEAN
> > +KvmDetectSevLiveMigrationFeature(
> > + VOID
> > + )
> > +{
> > + UINT8 Signature[13];
> > + UINT32 mKvmLeaf = 0;
> > + UINT32 RegEax, RegEbx, RegEcx, RegEdx;
> > +
> > + Signature[12] = '\0';
> > + for (mKvmLeaf = 0x40000000; mKvmLeaf < 0x40010000; mKvmLeaf += 0x100) {
>
> What's the reason for the loop? I would think that just checking
> 0x40000000 would be enough, so a comment seems to be warranted.
>
0x40000000 leaf is the hypervisor CPUID information leaf, so probably
just checking 0x40000000 should be enough.
But i see that other hypervisor detection functions like XenDetect()
do a loop test on the hypervisor existence function until the
signature match, is there a specific reason for that ?
Is this for some kind of support for another/multiple hypervisors ?
> > + AsmCpuid (mKvmLeaf,
> > + NULL,
> > + (UINT32 *) &Signature[0],
> > + (UINT32 *) &Signature[4],
> > + (UINT32 *) &Signature[8]);
> > +
> > + if (!AsciiStrCmp ((CHAR8 *) Signature, "KVMKVMKVM\0\0\0")) {
> > + DEBUG ((
> > + DEBUG_ERROR,
>
> DEBUG_INFO, it doesn't seem like an error.
>
Ok.
> > + "%a: KVM Detected, signature = %s\n",
> > + __FUNCTION__,
> > + Signature
> > + ));
> > +> + RegEax = 0x40000001;
>
> Should this be mKvmLeaf + 1? It is confusing that you may check 0x40000100
> and then not do 0x40000101.
>
Yes, it should be mKvmLeaf + 1, assuming the loop above is being used.
> > + RegEcx = 0;
> > + AsmCpuid (0x40000001, &RegEax, &RegEbx, &RegEcx, &RegEdx);
> > + if (RegEax & (1 << KVM_FEATURE_MIGRATION_CONTROL)) {
>
Thanks,
Ashish
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v4 0/4] SEV Live Migration support for OVMF.
2021-06-23 16:49 ` Laszlo Ersek
2021-06-23 17:03 ` Ashish Kalra
@ 2021-06-30 9:11 ` Ashish Kalra
2021-06-30 16:25 ` [edk2-devel] " Laszlo Ersek
1 sibling, 1 reply; 25+ messages in thread
From: Ashish Kalra @ 2021-06-30 9:11 UTC (permalink / raw)
To: Laszlo Ersek
Cc: devel, brijesh.singh, Thomas.Lendacky, jejb, erdemaktas,
jiewen.yao, min.m.xu, jordan.l.justen, ard.biesheuvel,
Dr. David Alan Gilbert, Paolo Bonzini
Hello Laszlo,
On Wed, Jun 23, 2021 at 06:49:06PM +0200, Laszlo Ersek wrote:
> On 06/23/21 18:42, Laszlo Ersek wrote:
> > On 06/22/21 19:46, Ashish Kalra wrote:
>
> >> Please find below your reply on v3 of this patch-set :
> >>
> >> Please include such a patch in v4 -- if Tom and Brijesh agree, I'd like to put the new lib explicitly under their reviewership.
> >>
> >> Also, I plan to review this series (v4, at this point) only for formalities. I'd like to receive an R-b from Tom or Brijesh [*], and another from Dov or a colleague at IBM, for this series; those together should suffice for merging the library.
> >>
> >> So, if you are fine with this approach, then probably Tom or Brijesh can
> >> take these patches under their reviewership and provide their R-b for
> >> this patch-set to be accepted and merged.
> >
> > Indeed. This helps. Thanks.
> >
> > I'll keep this patch set in my review queue then, for said "formalities
> > review".
>
> Please do file a TianoCore Feature Request BZ for this, and reference
> the bug URL in the commit messages. One important purpose of such a BZ
> is to succinctly link together all versions of a patch set -- that way
> poor maintainers know where to find previous versions, even if the blurb
> subject line changes over time. I also like to capture "permanent
> workflow notes" like the above in BZs (basically a high-level summary of
> who does what).
>
I have filed a new TianoCore Feature request BZ for this.
https://bugzilla.tianocore.org/show_bug.cgi?id=3467
I will refer this bug in future commit messages for this patch-set.
Please let me know if you want me to add additional contents and
comments to this bug.
Thanks,
Ashish
> For now it seems that a v5 will be necessary. Please keep me on CC, and
> when you have the BZ filed, we should link all past and future versions
> into it.
>
> Thanks
> Laszlo
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [edk2-devel] [PATCH v4 0/4] SEV Live Migration support for OVMF.
2021-06-30 9:11 ` Ashish Kalra
@ 2021-06-30 16:25 ` Laszlo Ersek
0 siblings, 0 replies; 25+ messages in thread
From: Laszlo Ersek @ 2021-06-30 16:25 UTC (permalink / raw)
To: devel, ashish.kalra
Cc: brijesh.singh, Thomas.Lendacky, jejb, erdemaktas, jiewen.yao,
min.m.xu, jordan.l.justen, ard.biesheuvel, Dr. David Alan Gilbert,
Paolo Bonzini
On 06/30/21 11:11, Ashish Kalra via groups.io wrote:
> Hello Laszlo,
>
> On Wed, Jun 23, 2021 at 06:49:06PM +0200, Laszlo Ersek wrote:
>> On 06/23/21 18:42, Laszlo Ersek wrote:
>>> On 06/22/21 19:46, Ashish Kalra wrote:
>>
>>>> Please find below your reply on v3 of this patch-set :
>>>>
>>>> Please include such a patch in v4 -- if Tom and Brijesh agree, I'd like to put the new lib explicitly under their reviewership.
>>>>
>>>> Also, I plan to review this series (v4, at this point) only for formalities. I'd like to receive an R-b from Tom or Brijesh [*], and another from Dov or a colleague at IBM, for this series; those together should suffice for merging the library.
>>>>
>>>> So, if you are fine with this approach, then probably Tom or Brijesh can
>>>> take these patches under their reviewership and provide their R-b for
>>>> this patch-set to be accepted and merged.
>>>
>>> Indeed. This helps. Thanks.
>>>
>>> I'll keep this patch set in my review queue then, for said "formalities
>>> review".
>>
>> Please do file a TianoCore Feature Request BZ for this, and reference
>> the bug URL in the commit messages. One important purpose of such a BZ
>> is to succinctly link together all versions of a patch set -- that way
>> poor maintainers know where to find previous versions, even if the blurb
>> subject line changes over time. I also like to capture "permanent
>> workflow notes" like the above in BZs (basically a high-level summary of
>> who does what).
>>
>
> I have filed a new TianoCore Feature request BZ for this.
> https://bugzilla.tianocore.org/show_bug.cgi?id=3467
>
> I will refer this bug in future commit messages for this patch-set.
>
> Please let me know if you want me to add additional contents and
> comments to this bug.
Thanks for filing the BZ, I've captured my previous statements / wishes
in some new BZ comments now.
Laszlo
>
> Thanks,
> Ashish
>
>> For now it seems that a v5 will be necessary. Please keep me on CC, and
>> when you have the BZ filed, we should link all past and future versions
>> into it.
>>
>> Thanks
>> Laszlo
>>
>
>
>
>
>
^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2021-06-30 16:25 UTC | newest]
Thread overview: 25+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-06-21 13:56 [PATCH v4 0/4] SEV Live Migration support for OVMF Ashish Kalra
2021-06-21 13:56 ` [PATCH v4 1/4] OvmfPkg/MemEncryptHypercallLib: add library to support SEV hypercalls Ashish Kalra
2021-06-22 19:47 ` Brijesh Singh
2021-06-22 19:58 ` Brijesh Singh
2021-06-22 22:47 ` Lendacky, Thomas
2021-06-22 23:20 ` Ashish Kalra
2021-06-22 23:38 ` Brijesh Singh
2021-06-23 1:47 ` Ashish Kalra
2021-06-23 15:02 ` Ashish Kalra
2021-06-21 13:57 ` [PATCH v4 2/4] OvmfPkg/BaseMemEncryptLib: Support to issue unencrypted hypercall Ashish Kalra
2021-06-22 22:50 ` Lendacky, Thomas
2021-06-21 13:57 ` [PATCH v4 3/4] OvmfPkg/PlatformPei: Mark SEC GHCB page as unencrypted via hypercall Ashish Kalra
2021-06-22 20:35 ` Brijesh Singh
2021-06-21 13:57 ` [PATCH v4 4/4] OvmfPkg/PlatformDxe: Add support for SEV live migration Ashish Kalra
2021-06-22 23:06 ` Lendacky, Thomas
2021-06-24 16:29 ` Ashish Kalra
2021-06-22 17:20 ` [PATCH v4 0/4] SEV Live Migration support for OVMF Laszlo Ersek
2021-06-22 17:45 ` Brijesh Singh
2021-06-22 17:46 ` Ashish Kalra
2021-06-23 13:18 ` [edk2-devel] " Dov Murik
2021-06-23 16:42 ` Laszlo Ersek
2021-06-23 16:49 ` Laszlo Ersek
2021-06-23 17:03 ` Ashish Kalra
2021-06-30 9:11 ` Ashish Kalra
2021-06-30 16:25 ` [edk2-devel] " Laszlo Ersek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox