* [PATCH v5 0/4] SEV Live Migration support for OVMF.
@ 2021-07-08 14:07 Ashish Kalra
2021-07-08 14:07 ` [PATCH v5 1/4] OvmfPkg/BaseMemEncryptLib: Support to issue unencrypted hypercall Ashish Kalra
` (3 more replies)
0 siblings, 4 replies; 17+ messages in thread
From: Ashish Kalra @ 2021-07-08 14:07 UTC (permalink / raw)
To: devel
Cc: dovmurik, brijesh.singh, tobin, Thomas.Lendacky, jejb, lersek,
jordan.l.justen, ard.biesheuvel, erdemaktas, jiewen.yao, min.m.xu
From: Ashish Kalra <ashish.kalra@amd.com>
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3467
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 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-1/tree/sev_live_migration_v5_7
Changes since v4:
- Remove MemEncryptHypercallLib Library and add support to issue
hypercall in the BaseMemEncryptSevLib library itself.
- For SEV-ES, make the VC handler hypercall aware by comparing
the hypercall number and add the additional register values
in the GHCB.
- Fix comments in the hypercall API interface.
- The encryption bit is set/clear on the smallest page size, hence
use the 4k page size in MAP_GPA_RANGE hypercall.
- Make the hypercall expect the guest physical address to be
page-aligned.
- Add KVM live migration feature flag check in BaseMemEncryptSevLib
library similar to how BaseMemEncryptSevLib does for the
MemEncryptSevIsEnabled() and check it before invoking HC. Also
export the MemEncryptSevLiveMigrationIsEnabled() function as
part of the library.
- Add error handling on hypercall return, on failure, return error
code to caller which potentially will cause an assert() and
terminate the boot.
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 (4):
OvmfPkg/BaseMemEncryptLib: Support to issue unencrypted hypercall
OvmfPkg/VmgExitLib: Add support for hypercalls with SEV-ES.
OvmfPkg/PlatformPei: Mark SEC GHCB page as unencrypted via hypercall
OvmfPkg/AmdSevDxe: Add support for SEV live migration.
OvmfPkg/AmdSevDxe/AmdSevDxe.c | 59 ++++++++++++++++
OvmfPkg/AmdSevDxe/AmdSevDxe.inf | 4 ++
OvmfPkg/Include/Guid/MemEncryptLib.h | 20 ++++++
OvmfPkg/Include/Library/MemEncryptSevLib.h | 69 +++++++++++++++++++
.../DxeMemEncryptSevLib.inf | 1 +
.../DxeMemEncryptSevLibInternal.c | 39 +++++++++++
.../Ia32/MemEncryptSevLib.c | 27 ++++++++
.../PeiDxeMemEncryptSevLibInternal.c | 51 ++++++++++++++
.../PeiMemEncryptSevLib.inf | 1 +
.../PeiMemEncryptSevLibInternal.c | 39 +++++++++++
.../SecMemEncryptSevLibInternal.c | 38 ++++++++++
.../X64/AsmHelperStub.nasm | 33 +++++++++
.../X64/MemEncryptSevLib.c | 54 +++++++++++++++
.../X64/PeiDxeVirtualMemory.c | 22 +++++-
OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c | 9 +++
OvmfPkg/OvmfPkg.dec | 1 +
OvmfPkg/PlatformPei/AmdSev.c | 9 +++
17 files changed, 475 insertions(+), 1 deletion(-)
create mode 100644 OvmfPkg/Include/Guid/MemEncryptLib.h
create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/X64/AsmHelperStub.nasm
--
2.17.1
^ permalink raw reply [flat|nested] 17+ messages in thread* [PATCH v5 1/4] OvmfPkg/BaseMemEncryptLib: Support to issue unencrypted hypercall 2021-07-08 14:07 [PATCH v5 0/4] SEV Live Migration support for OVMF Ashish Kalra @ 2021-07-08 14:07 ` Ashish Kalra 2021-07-15 20:58 ` Dov Murik 2021-07-16 14:11 ` Lendacky, Thomas 2021-07-08 14:08 ` [PATCH v5 2/4] OvmfPkg/VmgExitLib: Add support for hypercalls with SEV-ES Ashish Kalra ` (2 subsequent siblings) 3 siblings, 2 replies; 17+ messages in thread From: Ashish Kalra @ 2021-07-08 14:07 UTC (permalink / raw) To: devel Cc: dovmurik, brijesh.singh, tobin, Thomas.Lendacky, jejb, lersek, jordan.l.justen, ard.biesheuvel, erdemaktas, jiewen.yao, min.m.xu 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. This hypercall is used to notify hypervisor when the page's encryption state changes. 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/Include/Library/MemEncryptSevLib.h | 69 ++++++++++++++++++++ OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf | 1 + OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLibInternal.c | 39 +++++++++++ OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c | 27 ++++++++ OvmfPkg/Library/BaseMemEncryptSevLib/PeiDxeMemEncryptSevLibInternal.c | 51 +++++++++++++++ OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf | 1 + OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c | 39 +++++++++++ OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLibInternal.c | 38 +++++++++++ OvmfPkg/Library/BaseMemEncryptSevLib/X64/AsmHelperStub.nasm | 33 ++++++++++ OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c | 54 +++++++++++++++ OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c | 22 ++++++- 11 files changed, 373 insertions(+), 1 deletion(-) diff --git a/OvmfPkg/Include/Library/MemEncryptSevLib.h b/OvmfPkg/Include/Library/MemEncryptSevLib.h index 76d06c206c..c2b2a99a08 100644 --- a/OvmfPkg/Include/Library/MemEncryptSevLib.h +++ b/OvmfPkg/Include/Library/MemEncryptSevLib.h @@ -90,6 +90,18 @@ MemEncryptSevIsEnabled ( VOID ); +/** + Returns a boolean to indicate whether SEV live migration is enabled. + + @retval TRUE SEV live migration is enabled + @retval FALSE SEV live migration is not enabled +**/ +BOOLEAN +EFIAPI +MemEncryptSevLiveMigrationIsEnabled ( + VOID + ); + /** This function clears memory encryption bit for the memory region specified by BaseAddress and NumPages from the current page table context. @@ -222,4 +234,61 @@ MemEncryptSevClearMmioPageEncMask ( IN UINTN NumPages ); +/** + This hypercall is used to notify hypervisor when the page's encryption + state changes. + + @param[in] PhysicalAddress The physical address that is the start address + of a memory region. The PhysicalAddress is + expected to be PAGE_SIZE aligned. + @param[in] Pages Number of pages in memory region. + @param[in] Status Encrypted(1) or Decrypted(0). + +@retval RETURN_SUCCESS Hypercall returned success. +**/ +RETURN_STATUS +EFIAPI +SetMemoryEncDecHypercall3 ( + IN UINTN PhysicalAddress, + IN UINTN Pages, + IN UINTN Status + ); + +#define KVM_HC_MAP_GPA_RANGE 12 +#define KVM_MAP_GPA_RANGE_PAGE_SZ_4K 0 +#define KVM_MAP_GPA_RANGE_PAGE_SZ_2M BIT0 +#define KVM_MAP_GPA_RANGE_PAGE_SZ_1G BIT1 +#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) + +#define KVM_FEATURE_MIGRATION_CONTROL BIT17 + +/** + Figures out if we are running inside KVM HVM and + KVM HVM supports SEV Live Migration feature. + + @retval TRUE SEV live migration is supported. + @retval FALSE SEV live migration is not supported. +**/ +BOOLEAN +EFIAPI +KvmDetectSevLiveMigrationFeature( + VOID + ); + +/** + Interface exposed by the ASM implementation of the core hypercall + + @retval Hypercall returned status. +**/ +UINTN +EFIAPI +SetMemoryEncDecHypercall3AsmStub ( + IN UINTN HypercallNum, + IN UINTN PhysicalAddress, + IN UINTN Pages, + IN UINTN Attributes + ); + #endif // _MEM_ENCRYPT_SEV_LIB_H_ diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf b/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf index f2e162d680..0c28afadee 100644 --- a/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf @@ -38,6 +38,7 @@ X64/PeiDxeVirtualMemory.c X64/VirtualMemory.c X64/VirtualMemory.h + X64/AsmHelperStub.nasm [Sources.IA32] Ia32/MemEncryptSevLib.c diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLibInternal.c b/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLibInternal.c index 2816f859a0..ead754cd7b 100644 --- a/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLibInternal.c +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLibInternal.c @@ -20,6 +20,8 @@ STATIC BOOLEAN mSevStatus = FALSE; STATIC BOOLEAN mSevEsStatus = FALSE; STATIC BOOLEAN mSevStatusChecked = FALSE; +STATIC BOOLEAN mSevLiveMigrationStatus = FALSE; +STATIC BOOLEAN mSevLiveMigrationStatusChecked = FALSE; STATIC UINT64 mSevEncryptionMask = 0; STATIC BOOLEAN mSevEncryptionMaskSaved = FALSE; @@ -87,6 +89,24 @@ InternalMemEncryptSevStatus ( mSevStatusChecked = TRUE; } +/** + Figures out if we are running inside KVM HVM and + KVM HVM supports SEV Live Migration feature. +**/ +STATIC +VOID +EFIAPI +InternalDetectSevLiveMigrationFeature( + VOID + ) +{ + if (KvmDetectSevLiveMigrationFeature()) { + mSevLiveMigrationStatus = TRUE; + } + + mSevLiveMigrationStatusChecked = TRUE; +} + /** Returns a boolean to indicate whether SEV-ES is enabled. @@ -125,6 +145,25 @@ MemEncryptSevIsEnabled ( return mSevStatus; } +/** + Returns a boolean to indicate whether SEV live migration is enabled. + + @retval TRUE SEV live migration is enabled + @retval FALSE SEV live migration is not enabled +**/ +BOOLEAN +EFIAPI +MemEncryptSevLiveMigrationIsEnabled ( + VOID + ) +{ + if (!mSevLiveMigrationStatusChecked) { + InternalDetectSevLiveMigrationFeature (); + } + + return mSevLiveMigrationStatus; +} + /** Returns the SEV encryption mask. diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c b/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c index be260e0d10..62392309fe 100644 --- a/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c @@ -136,3 +136,30 @@ MemEncryptSevClearMmioPageEncMask ( // return RETURN_UNSUPPORTED; } + +/** + This hyercall is used to notify hypervisor when the page's encryption + state changes. + + @param[in] PhysicalAddress The physical address that is the start address + of a memory region. The physical address is + expected to be PAGE_SIZE aligned. + @param[in] Pages Number of Pages in the memory region. + @param[in] Status Encrypted(1) or Decrypted(0). + +@retval RETURN_SUCCESS Hypercall returned success. +**/ +RETURN_STATUS +EFIAPI +SetMemoryEncDecHypercall3 ( + IN UINTN PhysicalAddress, + IN UINTN Pages, + IN UINTN Status + ) +{ + // + // Memory encryption bit is not accessible in 32-bit mode + // + return RETURN_UNSUPPORTED; +} + diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiDxeMemEncryptSevLibInternal.c b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiDxeMemEncryptSevLibInternal.c index b4a9f464e2..0c9f7e17ba 100644 --- a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiDxeMemEncryptSevLibInternal.c +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiDxeMemEncryptSevLibInternal.c @@ -61,3 +61,54 @@ MemEncryptSevLocateInitialSmramSaveStateMapPages ( return RETURN_SUCCESS; } + +/** + Figures out if we are running inside KVM HVM and + KVM HVM supports SEV Live Migration feature. + + @retval TRUE SEV live migration is supported. + @retval FALSE SEV live migration is not supported. +**/ +BOOLEAN +EFIAPI +KvmDetectSevLiveMigrationFeature( + VOID + ) +{ + CHAR8 Signature[13]; + UINT32 mKvmLeaf; + 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") == 0) { + DEBUG (( + DEBUG_INFO, + "%a: KVM Detected, signature = %s\n", + __FUNCTION__, + Signature + )); + + RegEax = mKvmLeaf + 1; + RegEcx = 0; + AsmCpuid (mKvmLeaf + 1, &RegEax, &RegEbx, &RegEcx, &RegEdx); + if ((RegEax & KVM_FEATURE_MIGRATION_CONTROL) != 0) { + DEBUG (( + DEBUG_INFO, + "%a: Live Migration feature supported\n", + __FUNCTION__ + )); + + return TRUE; + } + } + } + + return FALSE; +} diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf index 03a78c32df..3233ca7979 100644 --- a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf @@ -38,6 +38,7 @@ X64/PeiDxeVirtualMemory.c X64/VirtualMemory.c X64/VirtualMemory.h + X64/AsmHelperStub.nasm [Sources.IA32] Ia32/MemEncryptSevLib.c diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c index e2fd109d12..9db6c2ef71 100644 --- a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c @@ -20,6 +20,8 @@ STATIC BOOLEAN mSevStatus = FALSE; STATIC BOOLEAN mSevEsStatus = FALSE; STATIC BOOLEAN mSevStatusChecked = FALSE; +STATIC BOOLEAN mSevLiveMigrationStatus = FALSE; +STATIC BOOLEAN mSevLiveMigrationStatusChecked = FALSE; STATIC UINT64 mSevEncryptionMask = 0; STATIC BOOLEAN mSevEncryptionMaskSaved = FALSE; @@ -87,6 +89,24 @@ InternalMemEncryptSevStatus ( mSevStatusChecked = TRUE; } +/** + Figures out if we are running inside KVM HVM and + KVM HVM supports SEV Live Migration feature. +**/ +STATIC +VOID +EFIAPI +InternalDetectSevLiveMigrationFeature( + VOID + ) +{ + if (KvmDetectSevLiveMigrationFeature()) { + mSevLiveMigrationStatus = TRUE; + } + + mSevLiveMigrationStatusChecked = TRUE; +} + /** Returns a boolean to indicate whether SEV-ES is enabled. @@ -125,6 +145,25 @@ MemEncryptSevIsEnabled ( return mSevStatus; } +/** + Returns a boolean to indicate whether SEV live migration is enabled. + + @retval TRUE SEV live migration is enabled + @retval FALSE SEV live migration is not enabled +**/ +BOOLEAN +EFIAPI +MemEncryptSevLiveMigrationIsEnabled ( + VOID + ) +{ + if (!mSevLiveMigrationStatusChecked) { + InternalDetectSevLiveMigrationFeature (); + } + + return mSevLiveMigrationStatus; +} + /** Returns the SEV encryption mask. diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLibInternal.c b/OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLibInternal.c index 56d8f3f318..b926c7b786 100644 --- a/OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLibInternal.c +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLibInternal.c @@ -100,6 +100,44 @@ MemEncryptSevIsEnabled ( return Msr.Bits.SevBit ? TRUE : FALSE; } +/** + Interface exposed by the ASM implementation of the core hypercall + + @retval Hypercall returned status. +**/ +UINTN +EFIAPI +SetMemoryEncDecHypercall3AsmStub ( + IN UINTN HypercallNum, + IN UINTN PhysicalAddress, + IN UINTN Pages, + IN UINTN Attributes + ) +{ + // + // Not used in SEC phase. + // + return RETURN_UNSUPPORTED; +} + +/** + Returns a boolean to indicate whether SEV live migration is enabled. + + @retval TRUE SEV live migration is enabled + @retval FALSE SEV live migration is not enabled +**/ +BOOLEAN +EFIAPI +MemEncryptSevLiveMigrationIsEnabled ( + VOID + ) +{ + // + // Not used in SEC phase. + // + return FALSE; +} + /** Returns the SEV encryption mask. diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/AsmHelperStub.nasm b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/AsmHelperStub.nasm new file mode 100644 index 0000000000..c7c11f77f1 --- /dev/null +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/AsmHelperStub.nasm @@ -0,0 +1,33 @@ +/** @file + + ASM helper stub to invoke hypercall + + Copyright (c) 2021, AMD Incorporated. All rights reserved.<BR> + + SPDX-License-Identifier: BSD-2-Clause-Patent + +**/ + +DEFAULT REL +SECTION .text + +; UINTN +; EFIAPI +; SetMemoryEncDecHypercall3AsmStub ( +; IN UINTN HypercallNum, +; IN UINTN Arg1, +; IN UINTN Arg2, +; IN UINTN Arg3 +; ); +global ASM_PFX(SetMemoryEncDecHypercall3AsmStub) +ASM_PFX(SetMemoryEncDecHypercall3AsmStub): + ; UEFI calling conventions require RBX to + ; be nonvolatile/callee-saved. + push rbx + mov rax, rcx ; Copy HypercallNumber to rax + mov rbx, rdx ; Copy Arg1 to the register expected by KVM + mov rcx, r8 ; Copy Arg2 to register expected by KVM + mov rdx, r9 ; Copy Arg2 to register expected by KVM + vmmcall ; Call VMMCALL + pop rbx + ret diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c index a57e8fd37f..57447e69dc 100644 --- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c @@ -143,3 +143,57 @@ MemEncryptSevClearMmioPageEncMask ( ); } + +/** + This hyercall is used to notify hypervisor when the page's encryption + state changes. + + @param[in] PhysicalAddress The physical address that is the start address + of a memory region. The physical address is + expected to be PAGE_SIZE aligned. + @param[in] Pages Number of Pages in the memory region. + @param[in] Status Encrypted(1) or Decrypted(0). + +@retval RETURN_SUCCESS Hypercall returned success. +**/ +RETURN_STATUS +EFIAPI +SetMemoryEncDecHypercall3 ( + IN UINTN PhysicalAddress, + IN UINTN Pages, + IN UINTN Status + ) +{ + RETURN_STATUS Ret; + INTN Error; + + Ret = RETURN_UNSUPPORTED; + + if (MemEncryptSevLiveMigrationIsEnabled ()) { + Ret = EFI_SUCCESS; + // + // The encryption bit is set/clear on the smallest page size, hence + // use the 4k page size in MAP_GPA_RANGE hypercall below. + // Also, the hypercall expects the guest physical address to be + // page-aligned. + // + Error = SetMemoryEncDecHypercall3AsmStub ( + KVM_HC_MAP_GPA_RANGE, + (PhysicalAddress & (~(EFI_PAGE_SIZE-1))), + Pages, + KVM_MAP_GPA_RANGE_PAGE_SZ_4K | KVM_MAP_GPA_RANGE_ENC_STAT(Status) + ); + + if (Error != 0) { + DEBUG ((DEBUG_ERROR, + "SetMemoryEncDecHypercall3 failed, Phys = %Lx, Pages = %Ld, Err = %Ld\n", + PhysicalAddress, + Pages, + (INT64)Error)); + + Ret = RETURN_NO_MAPPING; + } + } + + return Ret; +} diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c index c696745f9d..0b1588a4c1 100644 --- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c @@ -536,7 +536,6 @@ EnableReadOnlyPageWriteProtect ( AsmWriteCr0 (AsmReadCr0() | BIT16); } - /** This function either sets or clears memory encryption bit for the memory region specified by PhysicalAddress and Length from the current page table @@ -585,6 +584,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 +638,10 @@ SetMemoryEncDec ( Status = EFI_SUCCESS; + Size = Length; + CBitChanged = FALSE; + OrigPhysicalAddress = PhysicalAddress; + while (Length != 0) { // @@ -695,6 +701,7 @@ SetMemoryEncDec ( )); PhysicalAddress += BIT30; Length -= BIT30; + CBitChanged = TRUE; } else { // // We must split the page @@ -749,6 +756,7 @@ SetMemoryEncDec ( SetOrClearCBit (&PageDirectory2MEntry->Uint64, Mode); PhysicalAddress += BIT21; Length -= BIT21; + CBitChanged = TRUE; } else { // // We must split up this page into 4K pages @@ -791,6 +799,7 @@ SetMemoryEncDec ( SetOrClearCBit (&PageTableEntry->Uint64, Mode); PhysicalAddress += EFI_PAGE_SIZE; Length -= EFI_PAGE_SIZE; + CBitChanged = TRUE; } } } @@ -808,6 +817,17 @@ SetMemoryEncDec ( // CpuFlushTlb(); + // + // Notify Hypervisor on C-bit status + // + if (CBitChanged) { + Status = SetMemoryEncDecHypercall3 ( + OrigPhysicalAddress, + EFI_SIZE_TO_PAGES(Size), + !Mode + ); + } + Done: // // Restore page table write protection, if any. -- 2.17.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v5 1/4] OvmfPkg/BaseMemEncryptLib: Support to issue unencrypted hypercall 2021-07-08 14:07 ` [PATCH v5 1/4] OvmfPkg/BaseMemEncryptLib: Support to issue unencrypted hypercall Ashish Kalra @ 2021-07-15 20:58 ` Dov Murik 2021-07-16 12:29 ` Ashish Kalra 2021-07-16 14:11 ` Lendacky, Thomas 1 sibling, 1 reply; 17+ messages in thread From: Dov Murik @ 2021-07-15 20:58 UTC (permalink / raw) To: Ashish Kalra, devel Cc: dovmurik, brijesh.singh, tobin, Thomas.Lendacky, jejb, lersek, jordan.l.justen, ard.biesheuvel, erdemaktas, jiewen.yao, min.m.xu Hi Ashish, On 08/07/2021 17:07, 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. > > This hypercall is used to notify hypervisor when the page's > encryption state changes. > > 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/Include/Library/MemEncryptSevLib.h | 69 ++++++++++++++++++++ > OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf | 1 + > OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLibInternal.c | 39 +++++++++++ > OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c | 27 ++++++++ > OvmfPkg/Library/BaseMemEncryptSevLib/PeiDxeMemEncryptSevLibInternal.c | 51 +++++++++++++++ > OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf | 1 + > OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c | 39 +++++++++++ > OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLibInternal.c | 38 +++++++++++ > OvmfPkg/Library/BaseMemEncryptSevLib/X64/AsmHelperStub.nasm | 33 ++++++++++ > OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c | 54 +++++++++++++++ > OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c | 22 ++++++- > 11 files changed, 373 insertions(+), 1 deletion(-) > > diff --git a/OvmfPkg/Include/Library/MemEncryptSevLib.h b/OvmfPkg/Include/Library/MemEncryptSevLib.h > index 76d06c206c..c2b2a99a08 100644 > --- a/OvmfPkg/Include/Library/MemEncryptSevLib.h > +++ b/OvmfPkg/Include/Library/MemEncryptSevLib.h > @@ -90,6 +90,18 @@ MemEncryptSevIsEnabled ( > VOID > ); > > +/** > + Returns a boolean to indicate whether SEV live migration is enabled. > + > + @retval TRUE SEV live migration is enabled > + @retval FALSE SEV live migration is not enabled > +**/ > +BOOLEAN > +EFIAPI > +MemEncryptSevLiveMigrationIsEnabled ( > + VOID > + ); > + > /** > This function clears memory encryption bit for the memory region specified by > BaseAddress and NumPages from the current page table context. > @@ -222,4 +234,61 @@ MemEncryptSevClearMmioPageEncMask ( > IN UINTN NumPages > ); > > +/** > + This hypercall is used to notify hypervisor when the page's encryption > + state changes. > + > + @param[in] PhysicalAddress The physical address that is the start address > + of a memory region. The PhysicalAddress is > + expected to be PAGE_SIZE aligned. > + @param[in] Pages Number of pages in memory region. > + @param[in] Status Encrypted(1) or Decrypted(0). > + > +@retval RETURN_SUCCESS Hypercall returned success. > +**/ > +RETURN_STATUS > +EFIAPI > +SetMemoryEncDecHypercall3 ( > + IN UINTN PhysicalAddress, > + IN UINTN Pages, > + IN UINTN Status > + ); > + > +#define KVM_HC_MAP_GPA_RANGE 12 > +#define KVM_MAP_GPA_RANGE_PAGE_SZ_4K 0 > +#define KVM_MAP_GPA_RANGE_PAGE_SZ_2M BIT0 > +#define KVM_MAP_GPA_RANGE_PAGE_SZ_1G BIT1 > +#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) > + > +#define KVM_FEATURE_MIGRATION_CONTROL BIT17 > + > +/** > + Figures out if we are running inside KVM HVM and > + KVM HVM supports SEV Live Migration feature. > + > + @retval TRUE SEV live migration is supported. > + @retval FALSE SEV live migration is not supported. > +**/ > +BOOLEAN > +EFIAPI > +KvmDetectSevLiveMigrationFeature( > + VOID > + ); > + > +/** > + Interface exposed by the ASM implementation of the core hypercall > + > + @retval Hypercall returned status. > +**/ > +UINTN > +EFIAPI > +SetMemoryEncDecHypercall3AsmStub ( > + IN UINTN HypercallNum, > + IN UINTN PhysicalAddress, > + IN UINTN Pages, > + IN UINTN Attributes > + ); > + > #endif // _MEM_ENCRYPT_SEV_LIB_H_ > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf b/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf > index f2e162d680..0c28afadee 100644 > --- a/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf > @@ -38,6 +38,7 @@ > X64/PeiDxeVirtualMemory.c > X64/VirtualMemory.c > X64/VirtualMemory.h > + X64/AsmHelperStub.nasm > > [Sources.IA32] > Ia32/MemEncryptSevLib.c > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLibInternal.c b/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLibInternal.c > index 2816f859a0..ead754cd7b 100644 > --- a/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLibInternal.c > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLibInternal.c > @@ -20,6 +20,8 @@ > STATIC BOOLEAN mSevStatus = FALSE; > STATIC BOOLEAN mSevEsStatus = FALSE; > STATIC BOOLEAN mSevStatusChecked = FALSE; > +STATIC BOOLEAN mSevLiveMigrationStatus = FALSE; > +STATIC BOOLEAN mSevLiveMigrationStatusChecked = FALSE; > > STATIC UINT64 mSevEncryptionMask = 0; > STATIC BOOLEAN mSevEncryptionMaskSaved = FALSE; > @@ -87,6 +89,24 @@ InternalMemEncryptSevStatus ( > mSevStatusChecked = TRUE; > } > > +/** > + Figures out if we are running inside KVM HVM and > + KVM HVM supports SEV Live Migration feature. > +**/ > +STATIC > +VOID > +EFIAPI > +InternalDetectSevLiveMigrationFeature( > + VOID > + ) > +{ > + if (KvmDetectSevLiveMigrationFeature()) { > + mSevLiveMigrationStatus = TRUE; > + } > + > + mSevLiveMigrationStatusChecked = TRUE; > +} > + > /** > Returns a boolean to indicate whether SEV-ES is enabled. > > @@ -125,6 +145,25 @@ MemEncryptSevIsEnabled ( > return mSevStatus; > } > > +/** > + Returns a boolean to indicate whether SEV live migration is enabled. > + > + @retval TRUE SEV live migration is enabled > + @retval FALSE SEV live migration is not enabled > +**/ > +BOOLEAN > +EFIAPI > +MemEncryptSevLiveMigrationIsEnabled ( > + VOID > + ) > +{ > + if (!mSevLiveMigrationStatusChecked) { > + InternalDetectSevLiveMigrationFeature (); > + } > + > + return mSevLiveMigrationStatus; > +} > + > /** > Returns the SEV encryption mask. > > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c b/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c > index be260e0d10..62392309fe 100644 > --- a/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c > @@ -136,3 +136,30 @@ MemEncryptSevClearMmioPageEncMask ( > // > return RETURN_UNSUPPORTED; > } > + > +/** > + This hyercall is used to notify hypervisor when the page's encryption > + state changes. > + > + @param[in] PhysicalAddress The physical address that is the start address > + of a memory region. The physical address is > + expected to be PAGE_SIZE aligned. > + @param[in] Pages Number of Pages in the memory region. > + @param[in] Status Encrypted(1) or Decrypted(0). > + > +@retval RETURN_SUCCESS Hypercall returned success. > +**/ > +RETURN_STATUS > +EFIAPI > +SetMemoryEncDecHypercall3 ( > + IN UINTN PhysicalAddress, > + IN UINTN Pages, > + IN UINTN Status > + ) > +{ > + // > + // Memory encryption bit is not accessible in 32-bit mode > + // > + return RETURN_UNSUPPORTED; > +} > + > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiDxeMemEncryptSevLibInternal.c b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiDxeMemEncryptSevLibInternal.c > index b4a9f464e2..0c9f7e17ba 100644 > --- a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiDxeMemEncryptSevLibInternal.c > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiDxeMemEncryptSevLibInternal.c > @@ -61,3 +61,54 @@ MemEncryptSevLocateInitialSmramSaveStateMapPages ( > > return RETURN_SUCCESS; > } > + > +/** > + Figures out if we are running inside KVM HVM and > + KVM HVM supports SEV Live Migration feature. > + > + @retval TRUE SEV live migration is supported. > + @retval FALSE SEV live migration is not supported. > +**/ > +BOOLEAN > +EFIAPI > +KvmDetectSevLiveMigrationFeature( > + VOID > + ) > +{ > + CHAR8 Signature[13]; > + UINT32 mKvmLeaf; > + 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") == 0) { I assume this will also match if Signature is "KVMKVMKVM\0YZ". I don't know if that matters. > + DEBUG (( > + DEBUG_INFO, > + "%a: KVM Detected, signature = %s\n", > + __FUNCTION__, > + Signature > + )); > + > + RegEax = mKvmLeaf + 1; > + RegEcx = 0; > + AsmCpuid (mKvmLeaf + 1, &RegEax, &RegEbx, &RegEcx, &RegEdx); > + if ((RegEax & KVM_FEATURE_MIGRATION_CONTROL) != 0) { > + DEBUG (( > + DEBUG_INFO, > + "%a: Live Migration feature supported\n", I'd write: "%a: SEV Live Migration feature supported\n" > + __FUNCTION__ > + )); > + > + return TRUE; > + } > + } > + } > + > + return FALSE; > +} > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf > index 03a78c32df..3233ca7979 100644 > --- a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf > @@ -38,6 +38,7 @@ > X64/PeiDxeVirtualMemory.c > X64/VirtualMemory.c > X64/VirtualMemory.h > + X64/AsmHelperStub.nasm > > [Sources.IA32] > Ia32/MemEncryptSevLib.c > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c > index e2fd109d12..9db6c2ef71 100644 > --- a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c > @@ -20,6 +20,8 @@ > STATIC BOOLEAN mSevStatus = FALSE; > STATIC BOOLEAN mSevEsStatus = FALSE; > STATIC BOOLEAN mSevStatusChecked = FALSE; > +STATIC BOOLEAN mSevLiveMigrationStatus = FALSE; > +STATIC BOOLEAN mSevLiveMigrationStatusChecked = FALSE; > > STATIC UINT64 mSevEncryptionMask = 0; > STATIC BOOLEAN mSevEncryptionMaskSaved = FALSE; > @@ -87,6 +89,24 @@ InternalMemEncryptSevStatus ( > mSevStatusChecked = TRUE; > } > > +/** > + Figures out if we are running inside KVM HVM and > + KVM HVM supports SEV Live Migration feature. > +**/ > +STATIC > +VOID > +EFIAPI > +InternalDetectSevLiveMigrationFeature( > + VOID > + ) > +{ > + if (KvmDetectSevLiveMigrationFeature()) { > + mSevLiveMigrationStatus = TRUE; > + } > + > + mSevLiveMigrationStatusChecked = TRUE; > +} > + > /** > Returns a boolean to indicate whether SEV-ES is enabled. > > @@ -125,6 +145,25 @@ MemEncryptSevIsEnabled ( > return mSevStatus; > } > > +/** > + Returns a boolean to indicate whether SEV live migration is enabled. > + > + @retval TRUE SEV live migration is enabled > + @retval FALSE SEV live migration is not enabled > +**/ > +BOOLEAN > +EFIAPI > +MemEncryptSevLiveMigrationIsEnabled ( > + VOID > + ) > +{ > + if (!mSevLiveMigrationStatusChecked) { > + InternalDetectSevLiveMigrationFeature (); > + } > + > + return mSevLiveMigrationStatus; > +} > + > /** > Returns the SEV encryption mask. > > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLibInternal.c b/OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLibInternal.c > index 56d8f3f318..b926c7b786 100644 > --- a/OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLibInternal.c > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLibInternal.c > @@ -100,6 +100,44 @@ MemEncryptSevIsEnabled ( > return Msr.Bits.SevBit ? TRUE : FALSE; > } > > +/** > + Interface exposed by the ASM implementation of the core hypercall > + > + @retval Hypercall returned status. > +**/ > +UINTN > +EFIAPI > +SetMemoryEncDecHypercall3AsmStub ( > + IN UINTN HypercallNum, > + IN UINTN PhysicalAddress, > + IN UINTN Pages, > + IN UINTN Attributes > + ) > +{ > + // > + // Not used in SEC phase. > + // > + return RETURN_UNSUPPORTED; > +} > + > +/** > + Returns a boolean to indicate whether SEV live migration is enabled. > + > + @retval TRUE SEV live migration is enabled > + @retval FALSE SEV live migration is not enabled > +**/ > +BOOLEAN > +EFIAPI > +MemEncryptSevLiveMigrationIsEnabled ( > + VOID > + ) > +{ > + // > + // Not used in SEC phase. > + // > + return FALSE; > +} > + > /** > Returns the SEV encryption mask. > > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/AsmHelperStub.nasm b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/AsmHelperStub.nasm > new file mode 100644 > index 0000000000..c7c11f77f1 > --- /dev/null > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/AsmHelperStub.nasm > @@ -0,0 +1,33 @@ > +/** @file > + > + ASM helper stub to invoke hypercall > + > + Copyright (c) 2021, AMD Incorporated. All rights reserved.<BR> > + > + SPDX-License-Identifier: BSD-2-Clause-Patent > + > +**/ > + > +DEFAULT REL > +SECTION .text > + > +; UINTN > +; EFIAPI > +; SetMemoryEncDecHypercall3AsmStub ( > +; IN UINTN HypercallNum, > +; IN UINTN Arg1, > +; IN UINTN Arg2, > +; IN UINTN Arg3 > +; ); > +global ASM_PFX(SetMemoryEncDecHypercall3AsmStub) > +ASM_PFX(SetMemoryEncDecHypercall3AsmStub): > + ; UEFI calling conventions require RBX to > + ; be nonvolatile/callee-saved. > + push rbx > + mov rax, rcx ; Copy HypercallNumber to rax > + mov rbx, rdx ; Copy Arg1 to the register expected by KVM > + mov rcx, r8 ; Copy Arg2 to register expected by KVM > + mov rdx, r9 ; Copy Arg2 to register expected by KVM Comment: s/Arg2/Arg3/ > + vmmcall ; Call VMMCALL > + pop rbx > + ret > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c > index a57e8fd37f..57447e69dc 100644 > --- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c > @@ -143,3 +143,57 @@ MemEncryptSevClearMmioPageEncMask ( > ); > > } > + > +/** > + This hyercall is used to notify hypervisor when the page's encryption > + state changes. > + > + @param[in] PhysicalAddress The physical address that is the start address > + of a memory region. The physical address is > + expected to be PAGE_SIZE aligned. > + @param[in] Pages Number of Pages in the memory region. > + @param[in] Status Encrypted(1) or Decrypted(0). > + > +@retval RETURN_SUCCESS Hypercall returned success. or RETURN_UNSUPPORTED or RETURN_NO_MAPPING. > +**/ > +RETURN_STATUS > +EFIAPI > +SetMemoryEncDecHypercall3 ( > + IN UINTN PhysicalAddress, > + IN UINTN Pages, > + IN UINTN Status Consider: IN BOOL IsEncrypted or: IN MAP_RANGE_MODE EncMode (it's not a Status in the EFI_STATUS sense that appears all around edk2). > + ) > +{ > + RETURN_STATUS Ret; > + INTN Error; > + Add assert for the expected alignment of PhysicalAddress, and then you don't need to round it down when calling SetMemoryEncDecHypercall3AsmStub. > + Ret = RETURN_UNSUPPORTED; > + > + if (MemEncryptSevLiveMigrationIsEnabled ()) { > + Ret = EFI_SUCCESS; > + // > + // The encryption bit is set/clear on the smallest page size, hence > + // use the 4k page size in MAP_GPA_RANGE hypercall below. > + // Also, the hypercall expects the guest physical address to be > + // page-aligned. > + // > + Error = SetMemoryEncDecHypercall3AsmStub ( > + KVM_HC_MAP_GPA_RANGE, > + (PhysicalAddress & (~(EFI_PAGE_SIZE-1))), Simpler: PhysicalAddress & ~EFI_PAGE_MASK > + Pages, > + KVM_MAP_GPA_RANGE_PAGE_SZ_4K | KVM_MAP_GPA_RANGE_ENC_STAT(Status) > + ); > + > + if (Error != 0) { > + DEBUG ((DEBUG_ERROR, > + "SetMemoryEncDecHypercall3 failed, Phys = %Lx, Pages = %Ld, Err = %Ld\n", > + PhysicalAddress, > + Pages, > + (INT64)Error)); > + > + Ret = RETURN_NO_MAPPING; > + } > + } > + > + return Ret; > +} > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c > index c696745f9d..0b1588a4c1 100644 > --- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c > @@ -536,7 +536,6 @@ EnableReadOnlyPageWriteProtect ( > AsmWriteCr0 (AsmReadCr0() | BIT16); > } > > - > /** > This function either sets or clears memory encryption bit for the memory > region specified by PhysicalAddress and Length from the current page table > @@ -585,6 +584,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 +638,10 @@ SetMemoryEncDec ( > > Status = EFI_SUCCESS; > > + Size = Length; > + CBitChanged = FALSE; > + OrigPhysicalAddress = PhysicalAddress; > + > while (Length != 0) > { > // > @@ -695,6 +701,7 @@ SetMemoryEncDec ( > )); > PhysicalAddress += BIT30; > Length -= BIT30; > + CBitChanged = TRUE; > } else { > // > // We must split the page > @@ -749,6 +756,7 @@ SetMemoryEncDec ( > SetOrClearCBit (&PageDirectory2MEntry->Uint64, Mode); > PhysicalAddress += BIT21; > Length -= BIT21; > + CBitChanged = TRUE; > } else { > // > // We must split up this page into 4K pages > @@ -791,6 +799,7 @@ SetMemoryEncDec ( > SetOrClearCBit (&PageTableEntry->Uint64, Mode); > PhysicalAddress += EFI_PAGE_SIZE; > Length -= EFI_PAGE_SIZE; > + CBitChanged = TRUE; > } > } > } > @@ -808,6 +817,17 @@ SetMemoryEncDec ( > // > CpuFlushTlb(); > > + // > + // Notify Hypervisor on C-bit status > + // > + if (CBitChanged) { > + Status = SetMemoryEncDecHypercall3 ( > + OrigPhysicalAddress, > + EFI_SIZE_TO_PAGES(Size), > + !Mode Here you pass !Mode (which is 0 or 1) as the third argument to SetMemoryEncDecHypercall3 . But on patch 3/4 you pass KVM_MAP_GPA_RANGE_DECRYPTED (which is 0<<4); but that hints that you expect either 0<<4 or 1<<4 as this third argument. If this is the case, then here it should be: (Mode == SetCBit) ? KVM_MAP_GPA_RANGE_ENCRYPTED : KVM_MAP_GPA_RANGE_DECRYPTED If it's the other way around, then patch 3/4 needs to pass a simple 0 as the third argument. -Dov > + ); > + } > + > Done: > // > // Restore page table write protection, if any. > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v5 1/4] OvmfPkg/BaseMemEncryptLib: Support to issue unencrypted hypercall 2021-07-15 20:58 ` Dov Murik @ 2021-07-16 12:29 ` Ashish Kalra 2021-07-19 8:04 ` Dov Murik 0 siblings, 1 reply; 17+ messages in thread From: Ashish Kalra @ 2021-07-16 12:29 UTC (permalink / raw) To: Dov Murik Cc: devel, dovmurik, brijesh.singh, tobin, Thomas.Lendacky, jejb, lersek, jordan.l.justen, ard.biesheuvel, erdemaktas, jiewen.yao, min.m.xu Hello Dov, On Thu, Jul 15, 2021 at 11:58:17PM +0300, Dov Murik wrote: > Hi Ashish, > > On 08/07/2021 17:07, 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. > > > > This hypercall is used to notify hypervisor when the page's > > encryption state changes. > > > > 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/Include/Library/MemEncryptSevLib.h | 69 ++++++++++++++++++++ > > OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf | 1 + > > OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLibInternal.c | 39 +++++++++++ > > OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c | 27 ++++++++ > > OvmfPkg/Library/BaseMemEncryptSevLib/PeiDxeMemEncryptSevLibInternal.c | 51 +++++++++++++++ > > OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf | 1 + > > OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c | 39 +++++++++++ > > OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLibInternal.c | 38 +++++++++++ > > OvmfPkg/Library/BaseMemEncryptSevLib/X64/AsmHelperStub.nasm | 33 ++++++++++ > > OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c | 54 +++++++++++++++ > > OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c | 22 ++++++- > > 11 files changed, 373 insertions(+), 1 deletion(-) > > > > diff --git a/OvmfPkg/Include/Library/MemEncryptSevLib.h b/OvmfPkg/Include/Library/MemEncryptSevLib.h > > index 76d06c206c..c2b2a99a08 100644 > > --- a/OvmfPkg/Include/Library/MemEncryptSevLib.h > > +++ b/OvmfPkg/Include/Library/MemEncryptSevLib.h > > @@ -90,6 +90,18 @@ MemEncryptSevIsEnabled ( > > VOID > > ); > > > > +/** > > + Returns a boolean to indicate whether SEV live migration is enabled. > > + > > + @retval TRUE SEV live migration is enabled > > + @retval FALSE SEV live migration is not enabled > > +**/ > > +BOOLEAN > > +EFIAPI > > +MemEncryptSevLiveMigrationIsEnabled ( > > + VOID > > + ); > > + > > /** > > This function clears memory encryption bit for the memory region specified by > > BaseAddress and NumPages from the current page table context. > > @@ -222,4 +234,61 @@ MemEncryptSevClearMmioPageEncMask ( > > IN UINTN NumPages > > ); > > > > +/** > > + This hypercall is used to notify hypervisor when the page's encryption > > + state changes. > > + > > + @param[in] PhysicalAddress The physical address that is the start address > > + of a memory region. The PhysicalAddress is > > + expected to be PAGE_SIZE aligned. > > + @param[in] Pages Number of pages in memory region. > > + @param[in] Status Encrypted(1) or Decrypted(0). > > + > > +@retval RETURN_SUCCESS Hypercall returned success. > > +**/ > > +RETURN_STATUS > > +EFIAPI > > +SetMemoryEncDecHypercall3 ( > > + IN UINTN PhysicalAddress, > > + IN UINTN Pages, > > + IN UINTN Status > > + ); > > + > > +#define KVM_HC_MAP_GPA_RANGE 12 > > +#define KVM_MAP_GPA_RANGE_PAGE_SZ_4K 0 > > +#define KVM_MAP_GPA_RANGE_PAGE_SZ_2M BIT0 > > +#define KVM_MAP_GPA_RANGE_PAGE_SZ_1G BIT1 > > +#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) > > + > > +#define KVM_FEATURE_MIGRATION_CONTROL BIT17 > > + > > +/** > > + Figures out if we are running inside KVM HVM and > > + KVM HVM supports SEV Live Migration feature. > > + > > + @retval TRUE SEV live migration is supported. > > + @retval FALSE SEV live migration is not supported. > > +**/ > > +BOOLEAN > > +EFIAPI > > +KvmDetectSevLiveMigrationFeature( > > + VOID > > + ); > > + > > +/** > > + Interface exposed by the ASM implementation of the core hypercall > > + > > + @retval Hypercall returned status. > > +**/ > > +UINTN > > +EFIAPI > > +SetMemoryEncDecHypercall3AsmStub ( > > + IN UINTN HypercallNum, > > + IN UINTN PhysicalAddress, > > + IN UINTN Pages, > > + IN UINTN Attributes > > + ); > > + > > #endif // _MEM_ENCRYPT_SEV_LIB_H_ > > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf b/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf > > index f2e162d680..0c28afadee 100644 > > --- a/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf > > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf > > @@ -38,6 +38,7 @@ > > X64/PeiDxeVirtualMemory.c > > X64/VirtualMemory.c > > X64/VirtualMemory.h > > + X64/AsmHelperStub.nasm > > > > [Sources.IA32] > > Ia32/MemEncryptSevLib.c > > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLibInternal.c b/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLibInternal.c > > index 2816f859a0..ead754cd7b 100644 > > --- a/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLibInternal.c > > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLibInternal.c > > @@ -20,6 +20,8 @@ > > STATIC BOOLEAN mSevStatus = FALSE; > > STATIC BOOLEAN mSevEsStatus = FALSE; > > STATIC BOOLEAN mSevStatusChecked = FALSE; > > +STATIC BOOLEAN mSevLiveMigrationStatus = FALSE; > > +STATIC BOOLEAN mSevLiveMigrationStatusChecked = FALSE; > > > > STATIC UINT64 mSevEncryptionMask = 0; > > STATIC BOOLEAN mSevEncryptionMaskSaved = FALSE; > > @@ -87,6 +89,24 @@ InternalMemEncryptSevStatus ( > > mSevStatusChecked = TRUE; > > } > > > > +/** > > + Figures out if we are running inside KVM HVM and > > + KVM HVM supports SEV Live Migration feature. > > +**/ > > +STATIC > > +VOID > > +EFIAPI > > +InternalDetectSevLiveMigrationFeature( > > + VOID > > + ) > > +{ > > + if (KvmDetectSevLiveMigrationFeature()) { > > + mSevLiveMigrationStatus = TRUE; > > + } > > + > > + mSevLiveMigrationStatusChecked = TRUE; > > +} > > + > > /** > > Returns a boolean to indicate whether SEV-ES is enabled. > > > > @@ -125,6 +145,25 @@ MemEncryptSevIsEnabled ( > > return mSevStatus; > > } > > > > +/** > > + Returns a boolean to indicate whether SEV live migration is enabled. > > + > > + @retval TRUE SEV live migration is enabled > > + @retval FALSE SEV live migration is not enabled > > +**/ > > +BOOLEAN > > +EFIAPI > > +MemEncryptSevLiveMigrationIsEnabled ( > > + VOID > > + ) > > +{ > > + if (!mSevLiveMigrationStatusChecked) { > > + InternalDetectSevLiveMigrationFeature (); > > + } > > + > > + return mSevLiveMigrationStatus; > > +} > > + > > /** > > Returns the SEV encryption mask. > > > > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c b/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c > > index be260e0d10..62392309fe 100644 > > --- a/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c > > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c > > @@ -136,3 +136,30 @@ MemEncryptSevClearMmioPageEncMask ( > > // > > return RETURN_UNSUPPORTED; > > } > > + > > +/** > > + This hyercall is used to notify hypervisor when the page's encryption > > + state changes. > > + > > + @param[in] PhysicalAddress The physical address that is the start address > > + of a memory region. The physical address is > > + expected to be PAGE_SIZE aligned. > > + @param[in] Pages Number of Pages in the memory region. > > + @param[in] Status Encrypted(1) or Decrypted(0). > > + > > +@retval RETURN_SUCCESS Hypercall returned success. > > +**/ > > +RETURN_STATUS > > +EFIAPI > > +SetMemoryEncDecHypercall3 ( > > + IN UINTN PhysicalAddress, > > + IN UINTN Pages, > > + IN UINTN Status > > + ) > > +{ > > + // > > + // Memory encryption bit is not accessible in 32-bit mode > > + // > > + return RETURN_UNSUPPORTED; > > +} > > + > > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiDxeMemEncryptSevLibInternal.c b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiDxeMemEncryptSevLibInternal.c > > index b4a9f464e2..0c9f7e17ba 100644 > > --- a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiDxeMemEncryptSevLibInternal.c > > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiDxeMemEncryptSevLibInternal.c > > @@ -61,3 +61,54 @@ MemEncryptSevLocateInitialSmramSaveStateMapPages ( > > > > return RETURN_SUCCESS; > > } > > + > > +/** > > + Figures out if we are running inside KVM HVM and > > + KVM HVM supports SEV Live Migration feature. > > + > > + @retval TRUE SEV live migration is supported. > > + @retval FALSE SEV live migration is not supported. > > +**/ > > +BOOLEAN > > +EFIAPI > > +KvmDetectSevLiveMigrationFeature( > > + VOID > > + ) > > +{ > > + CHAR8 Signature[13]; > > + UINT32 mKvmLeaf; > > + 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") == 0) { > > I assume this will also match if Signature is "KVMKVMKVM\0YZ". I don't > know if that matters. > I don't understand what do you mean by "KVMKVMKVM\0YZ", this is comparing for "KVMKVMKVM\0\0\0" ? > > + DEBUG (( > > + DEBUG_INFO, > > + "%a: KVM Detected, signature = %s\n", > > + __FUNCTION__, > > + Signature > > + )); > > + > > + RegEax = mKvmLeaf + 1; > > + RegEcx = 0; > > + AsmCpuid (mKvmLeaf + 1, &RegEax, &RegEbx, &RegEcx, &RegEdx); > > + if ((RegEax & KVM_FEATURE_MIGRATION_CONTROL) != 0) { > > + DEBUG (( > > + DEBUG_INFO, > > + "%a: Live Migration feature supported\n", > > I'd write: "%a: SEV Live Migration feature supported\n" > Ok. > > > + __FUNCTION__ > > + )); > > + > > + return TRUE; > > + } > > + } > > + } > > + > > + return FALSE; > > +} > > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf > > index 03a78c32df..3233ca7979 100644 > > --- a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf > > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf > > @@ -38,6 +38,7 @@ > > X64/PeiDxeVirtualMemory.c > > X64/VirtualMemory.c > > X64/VirtualMemory.h > > + X64/AsmHelperStub.nasm > > > > [Sources.IA32] > > Ia32/MemEncryptSevLib.c > > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c > > index e2fd109d12..9db6c2ef71 100644 > > --- a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c > > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c > > @@ -20,6 +20,8 @@ > > STATIC BOOLEAN mSevStatus = FALSE; > > STATIC BOOLEAN mSevEsStatus = FALSE; > > STATIC BOOLEAN mSevStatusChecked = FALSE; > > +STATIC BOOLEAN mSevLiveMigrationStatus = FALSE; > > +STATIC BOOLEAN mSevLiveMigrationStatusChecked = FALSE; > > > > STATIC UINT64 mSevEncryptionMask = 0; > > STATIC BOOLEAN mSevEncryptionMaskSaved = FALSE; > > @@ -87,6 +89,24 @@ InternalMemEncryptSevStatus ( > > mSevStatusChecked = TRUE; > > } > > > > +/** > > + Figures out if we are running inside KVM HVM and > > + KVM HVM supports SEV Live Migration feature. > > +**/ > > +STATIC > > +VOID > > +EFIAPI > > +InternalDetectSevLiveMigrationFeature( > > + VOID > > + ) > > +{ > > + if (KvmDetectSevLiveMigrationFeature()) { > > + mSevLiveMigrationStatus = TRUE; > > + } > > + > > + mSevLiveMigrationStatusChecked = TRUE; > > +} > > + > > /** > > Returns a boolean to indicate whether SEV-ES is enabled. > > > > @@ -125,6 +145,25 @@ MemEncryptSevIsEnabled ( > > return mSevStatus; > > } > > > > +/** > > + Returns a boolean to indicate whether SEV live migration is enabled. > > + > > + @retval TRUE SEV live migration is enabled > > + @retval FALSE SEV live migration is not enabled > > +**/ > > +BOOLEAN > > +EFIAPI > > +MemEncryptSevLiveMigrationIsEnabled ( > > + VOID > > + ) > > +{ > > + if (!mSevLiveMigrationStatusChecked) { > > + InternalDetectSevLiveMigrationFeature (); > > + } > > + > > + return mSevLiveMigrationStatus; > > +} > > + > > /** > > Returns the SEV encryption mask. > > > > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLibInternal.c b/OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLibInternal.c > > index 56d8f3f318..b926c7b786 100644 > > --- a/OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLibInternal.c > > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLibInternal.c > > @@ -100,6 +100,44 @@ MemEncryptSevIsEnabled ( > > return Msr.Bits.SevBit ? TRUE : FALSE; > > } > > > > +/** > > + Interface exposed by the ASM implementation of the core hypercall > > + > > + @retval Hypercall returned status. > > +**/ > > +UINTN > > +EFIAPI > > +SetMemoryEncDecHypercall3AsmStub ( > > + IN UINTN HypercallNum, > > + IN UINTN PhysicalAddress, > > + IN UINTN Pages, > > + IN UINTN Attributes > > + ) > > +{ > > + // > > + // Not used in SEC phase. > > + // > > + return RETURN_UNSUPPORTED; > > +} > > + > > +/** > > + Returns a boolean to indicate whether SEV live migration is enabled. > > + > > + @retval TRUE SEV live migration is enabled > > + @retval FALSE SEV live migration is not enabled > > +**/ > > +BOOLEAN > > +EFIAPI > > +MemEncryptSevLiveMigrationIsEnabled ( > > + VOID > > + ) > > +{ > > + // > > + // Not used in SEC phase. > > + // > > + return FALSE; > > +} > > + > > /** > > Returns the SEV encryption mask. > > > > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/AsmHelperStub.nasm b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/AsmHelperStub.nasm > > new file mode 100644 > > index 0000000000..c7c11f77f1 > > --- /dev/null > > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/AsmHelperStub.nasm > > @@ -0,0 +1,33 @@ > > +/** @file > > + > > + ASM helper stub to invoke hypercall > > + > > + Copyright (c) 2021, AMD Incorporated. All rights reserved.<BR> > > + > > + SPDX-License-Identifier: BSD-2-Clause-Patent > > + > > +**/ > > + > > +DEFAULT REL > > +SECTION .text > > + > > +; UINTN > > +; EFIAPI > > +; SetMemoryEncDecHypercall3AsmStub ( > > +; IN UINTN HypercallNum, > > +; IN UINTN Arg1, > > +; IN UINTN Arg2, > > +; IN UINTN Arg3 > > +; ); > > +global ASM_PFX(SetMemoryEncDecHypercall3AsmStub) > > +ASM_PFX(SetMemoryEncDecHypercall3AsmStub): > > + ; UEFI calling conventions require RBX to > > + ; be nonvolatile/callee-saved. > > + push rbx > > + mov rax, rcx ; Copy HypercallNumber to rax > > + mov rbx, rdx ; Copy Arg1 to the register expected by KVM > > + mov rcx, r8 ; Copy Arg2 to register expected by KVM > > + mov rdx, r9 ; Copy Arg2 to register expected by KVM > > Comment: s/Arg2/Arg3/ > Yes. > > + vmmcall ; Call VMMCALL > > + pop rbx > > + ret > > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c > > index a57e8fd37f..57447e69dc 100644 > > --- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c > > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c > > @@ -143,3 +143,57 @@ MemEncryptSevClearMmioPageEncMask ( > > ); > > > > } > > + > > +/** > > + This hyercall is used to notify hypervisor when the page's encryption > > + state changes. > > + > > + @param[in] PhysicalAddress The physical address that is the start address > > + of a memory region. The physical address is > > + expected to be PAGE_SIZE aligned. > > + @param[in] Pages Number of Pages in the memory region. > > + @param[in] Status Encrypted(1) or Decrypted(0). > > + > > +@retval RETURN_SUCCESS Hypercall returned success. > > or RETURN_UNSUPPORTED or RETURN_NO_MAPPING. > Ok. > > +**/ > > +RETURN_STATUS > > +EFIAPI > > +SetMemoryEncDecHypercall3 ( > > + IN UINTN PhysicalAddress, > > + IN UINTN Pages, > > + IN UINTN Status > > Consider: > > IN BOOL IsEncrypted > > or: > > IN MAP_RANGE_MODE EncMode > > (it's not a Status in the EFI_STATUS sense that appears all around edk2). > Ok, i think i will prefer something like a MAP_RANGE_MODE. > > > + ) > > +{ > > + RETURN_STATUS Ret; > > + INTN Error; > > + > > Add assert for the expected alignment of PhysicalAddress, and then > you don't need to round it down when calling SetMemoryEncDecHypercall3AsmStub. > Cannot really use an assert here, as when the GCD map is being walked and the c-bit being cleared from MMIO and NonExistent memory spaces, the physical address range being passed may not be page-aligned, so adding an assert here prevents booting. Hence, rounding it down when calling SetMemoryEncDecHypercall3AsmStub below. > > > + Ret = RETURN_UNSUPPORTED; > > + > > + if (MemEncryptSevLiveMigrationIsEnabled ()) { > > + Ret = EFI_SUCCESS; > > + // > > + // The encryption bit is set/clear on the smallest page size, hence > > + // use the 4k page size in MAP_GPA_RANGE hypercall below. > > + // Also, the hypercall expects the guest physical address to be > > + // page-aligned. > > + // > > + Error = SetMemoryEncDecHypercall3AsmStub ( > > + KVM_HC_MAP_GPA_RANGE, > > + (PhysicalAddress & (~(EFI_PAGE_SIZE-1))), > > Simpler: > > PhysicalAddress & ~EFI_PAGE_MASK > > Ok. > > + Pages, > > + KVM_MAP_GPA_RANGE_PAGE_SZ_4K | KVM_MAP_GPA_RANGE_ENC_STAT(Status) > > + ); > > + > > + if (Error != 0) { > > + DEBUG ((DEBUG_ERROR, > > + "SetMemoryEncDecHypercall3 failed, Phys = %Lx, Pages = %Ld, Err = %Ld\n", > > + PhysicalAddress, > > + Pages, > > + (INT64)Error)); > > + > > + Ret = RETURN_NO_MAPPING; > > + } > > + } > > + > > + return Ret; > > +} > > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c > > index c696745f9d..0b1588a4c1 100644 > > --- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c > > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c > > @@ -536,7 +536,6 @@ EnableReadOnlyPageWriteProtect ( > > AsmWriteCr0 (AsmReadCr0() | BIT16); > > } > > > > - > > /** > > This function either sets or clears memory encryption bit for the memory > > region specified by PhysicalAddress and Length from the current page table > > @@ -585,6 +584,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 +638,10 @@ SetMemoryEncDec ( > > > > Status = EFI_SUCCESS; > > > > + Size = Length; > > + CBitChanged = FALSE; > > + OrigPhysicalAddress = PhysicalAddress; > > + > > while (Length != 0) > > { > > // > > @@ -695,6 +701,7 @@ SetMemoryEncDec ( > > )); > > PhysicalAddress += BIT30; > > Length -= BIT30; > > + CBitChanged = TRUE; > > } else { > > // > > // We must split the page > > @@ -749,6 +756,7 @@ SetMemoryEncDec ( > > SetOrClearCBit (&PageDirectory2MEntry->Uint64, Mode); > > PhysicalAddress += BIT21; > > Length -= BIT21; > > + CBitChanged = TRUE; > > } else { > > // > > // We must split up this page into 4K pages > > @@ -791,6 +799,7 @@ SetMemoryEncDec ( > > SetOrClearCBit (&PageTableEntry->Uint64, Mode); > > PhysicalAddress += EFI_PAGE_SIZE; > > Length -= EFI_PAGE_SIZE; > > + CBitChanged = TRUE; > > } > > } > > } > > @@ -808,6 +817,17 @@ SetMemoryEncDec ( > > // > > CpuFlushTlb(); > > > > + // > > + // Notify Hypervisor on C-bit status > > + // > > + if (CBitChanged) { > > + Status = SetMemoryEncDecHypercall3 ( > > + OrigPhysicalAddress, > > + EFI_SIZE_TO_PAGES(Size), > > + !Mode > > Here you pass !Mode (which is 0 or 1) as the third argument to > SetMemoryEncDecHypercall3 . > > But on patch 3/4 you pass KVM_MAP_GPA_RANGE_DECRYPTED (which is 0<<4); > but that hints that you expect either 0<<4 or 1<<4 as this third argument. > If this is the case, then here it should be: > > (Mode == SetCBit) ? KVM_MAP_GPA_RANGE_ENCRYPTED : KVM_MAP_GPA_RANGE_DECRYPTED > > If it's the other way around, then patch 3/4 needs to pass a simple 0 as > the third argument. > Yes, i need to pass a 0 (decrypted) as the third argument in that patch. Thanks, Ashish > > > > > + ); > > + } > > + > > Done: > > // > > // Restore page table write protection, if any. > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v5 1/4] OvmfPkg/BaseMemEncryptLib: Support to issue unencrypted hypercall 2021-07-16 12:29 ` Ashish Kalra @ 2021-07-19 8:04 ` Dov Murik 2021-07-19 15:30 ` Ashish Kalra 0 siblings, 1 reply; 17+ messages in thread From: Dov Murik @ 2021-07-19 8:04 UTC (permalink / raw) To: Ashish Kalra Cc: devel, dovmurik, brijesh.singh, tobin, Thomas.Lendacky, jejb, lersek, jordan.l.justen, ard.biesheuvel, erdemaktas, jiewen.yao, min.m.xu On 16/07/2021 15:29, Ashish Kalra wrote: > Hello Dov, > > On Thu, Jul 15, 2021 at 11:58:17PM +0300, Dov Murik wrote: >> Hi Ashish, >> >> On 08/07/2021 17:07, 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. >>> >>> This hypercall is used to notify hypervisor when the page's >>> encryption state changes. >>> >>> 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/Include/Library/MemEncryptSevLib.h | 69 ++++++++++++++++++++ >>> OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf | 1 + >>> OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLibInternal.c | 39 +++++++++++ >>> OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c | 27 ++++++++ >>> OvmfPkg/Library/BaseMemEncryptSevLib/PeiDxeMemEncryptSevLibInternal.c | 51 +++++++++++++++ >>> OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf | 1 + >>> OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c | 39 +++++++++++ >>> OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLibInternal.c | 38 +++++++++++ >>> OvmfPkg/Library/BaseMemEncryptSevLib/X64/AsmHelperStub.nasm | 33 ++++++++++ >>> OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c | 54 +++++++++++++++ >>> OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c | 22 ++++++- >>> 11 files changed, 373 insertions(+), 1 deletion(-) >>> >>> diff --git a/OvmfPkg/Include/Library/MemEncryptSevLib.h b/OvmfPkg/Include/Library/MemEncryptSevLib.h >>> index 76d06c206c..c2b2a99a08 100644 >>> --- a/OvmfPkg/Include/Library/MemEncryptSevLib.h >>> +++ b/OvmfPkg/Include/Library/MemEncryptSevLib.h >>> @@ -90,6 +90,18 @@ MemEncryptSevIsEnabled ( >>> VOID >>> ); >>> >>> +/** >>> + Returns a boolean to indicate whether SEV live migration is enabled. >>> + >>> + @retval TRUE SEV live migration is enabled >>> + @retval FALSE SEV live migration is not enabled >>> +**/ >>> +BOOLEAN >>> +EFIAPI >>> +MemEncryptSevLiveMigrationIsEnabled ( >>> + VOID >>> + ); >>> + >>> /** >>> This function clears memory encryption bit for the memory region specified by >>> BaseAddress and NumPages from the current page table context. >>> @@ -222,4 +234,61 @@ MemEncryptSevClearMmioPageEncMask ( >>> IN UINTN NumPages >>> ); >>> >>> +/** >>> + This hypercall is used to notify hypervisor when the page's encryption >>> + state changes. >>> + >>> + @param[in] PhysicalAddress The physical address that is the start address >>> + of a memory region. The PhysicalAddress is >>> + expected to be PAGE_SIZE aligned. >>> + @param[in] Pages Number of pages in memory region. >>> + @param[in] Status Encrypted(1) or Decrypted(0). >>> + >>> +@retval RETURN_SUCCESS Hypercall returned success. >>> +**/ >>> +RETURN_STATUS >>> +EFIAPI >>> +SetMemoryEncDecHypercall3 ( >>> + IN UINTN PhysicalAddress, >>> + IN UINTN Pages, >>> + IN UINTN Status >>> + ); >>> + >>> +#define KVM_HC_MAP_GPA_RANGE 12 >>> +#define KVM_MAP_GPA_RANGE_PAGE_SZ_4K 0 >>> +#define KVM_MAP_GPA_RANGE_PAGE_SZ_2M BIT0 >>> +#define KVM_MAP_GPA_RANGE_PAGE_SZ_1G BIT1 >>> +#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) >>> + >>> +#define KVM_FEATURE_MIGRATION_CONTROL BIT17 >>> + >>> +/** >>> + Figures out if we are running inside KVM HVM and >>> + KVM HVM supports SEV Live Migration feature. >>> + >>> + @retval TRUE SEV live migration is supported. >>> + @retval FALSE SEV live migration is not supported. >>> +**/ >>> +BOOLEAN >>> +EFIAPI >>> +KvmDetectSevLiveMigrationFeature( >>> + VOID >>> + ); >>> + >>> +/** >>> + Interface exposed by the ASM implementation of the core hypercall >>> + >>> + @retval Hypercall returned status. >>> +**/ >>> +UINTN >>> +EFIAPI >>> +SetMemoryEncDecHypercall3AsmStub ( >>> + IN UINTN HypercallNum, >>> + IN UINTN PhysicalAddress, >>> + IN UINTN Pages, >>> + IN UINTN Attributes >>> + ); >>> + >>> #endif // _MEM_ENCRYPT_SEV_LIB_H_ >>> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf b/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf >>> index f2e162d680..0c28afadee 100644 >>> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf >>> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf >>> @@ -38,6 +38,7 @@ >>> X64/PeiDxeVirtualMemory.c >>> X64/VirtualMemory.c >>> X64/VirtualMemory.h >>> + X64/AsmHelperStub.nasm >>> >>> [Sources.IA32] >>> Ia32/MemEncryptSevLib.c >>> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLibInternal.c b/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLibInternal.c >>> index 2816f859a0..ead754cd7b 100644 >>> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLibInternal.c >>> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLibInternal.c >>> @@ -20,6 +20,8 @@ >>> STATIC BOOLEAN mSevStatus = FALSE; >>> STATIC BOOLEAN mSevEsStatus = FALSE; >>> STATIC BOOLEAN mSevStatusChecked = FALSE; >>> +STATIC BOOLEAN mSevLiveMigrationStatus = FALSE; >>> +STATIC BOOLEAN mSevLiveMigrationStatusChecked = FALSE; >>> >>> STATIC UINT64 mSevEncryptionMask = 0; >>> STATIC BOOLEAN mSevEncryptionMaskSaved = FALSE; >>> @@ -87,6 +89,24 @@ InternalMemEncryptSevStatus ( >>> mSevStatusChecked = TRUE; >>> } >>> >>> +/** >>> + Figures out if we are running inside KVM HVM and >>> + KVM HVM supports SEV Live Migration feature. >>> +**/ >>> +STATIC >>> +VOID >>> +EFIAPI >>> +InternalDetectSevLiveMigrationFeature( >>> + VOID >>> + ) >>> +{ >>> + if (KvmDetectSevLiveMigrationFeature()) { >>> + mSevLiveMigrationStatus = TRUE; >>> + } >>> + >>> + mSevLiveMigrationStatusChecked = TRUE; >>> +} >>> + >>> /** >>> Returns a boolean to indicate whether SEV-ES is enabled. >>> >>> @@ -125,6 +145,25 @@ MemEncryptSevIsEnabled ( >>> return mSevStatus; >>> } >>> >>> +/** >>> + Returns a boolean to indicate whether SEV live migration is enabled. >>> + >>> + @retval TRUE SEV live migration is enabled >>> + @retval FALSE SEV live migration is not enabled >>> +**/ >>> +BOOLEAN >>> +EFIAPI >>> +MemEncryptSevLiveMigrationIsEnabled ( >>> + VOID >>> + ) >>> +{ >>> + if (!mSevLiveMigrationStatusChecked) { >>> + InternalDetectSevLiveMigrationFeature (); >>> + } >>> + >>> + return mSevLiveMigrationStatus; >>> +} >>> + >>> /** >>> Returns the SEV encryption mask. >>> >>> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c b/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c >>> index be260e0d10..62392309fe 100644 >>> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c >>> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c >>> @@ -136,3 +136,30 @@ MemEncryptSevClearMmioPageEncMask ( >>> // >>> return RETURN_UNSUPPORTED; >>> } >>> + >>> +/** >>> + This hyercall is used to notify hypervisor when the page's encryption >>> + state changes. >>> + >>> + @param[in] PhysicalAddress The physical address that is the start address >>> + of a memory region. The physical address is >>> + expected to be PAGE_SIZE aligned. >>> + @param[in] Pages Number of Pages in the memory region. >>> + @param[in] Status Encrypted(1) or Decrypted(0). >>> + >>> +@retval RETURN_SUCCESS Hypercall returned success. >>> +**/ >>> +RETURN_STATUS >>> +EFIAPI >>> +SetMemoryEncDecHypercall3 ( >>> + IN UINTN PhysicalAddress, >>> + IN UINTN Pages, >>> + IN UINTN Status >>> + ) >>> +{ >>> + // >>> + // Memory encryption bit is not accessible in 32-bit mode >>> + // >>> + return RETURN_UNSUPPORTED; >>> +} >>> + >>> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiDxeMemEncryptSevLibInternal.c b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiDxeMemEncryptSevLibInternal.c >>> index b4a9f464e2..0c9f7e17ba 100644 >>> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiDxeMemEncryptSevLibInternal.c >>> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiDxeMemEncryptSevLibInternal.c >>> @@ -61,3 +61,54 @@ MemEncryptSevLocateInitialSmramSaveStateMapPages ( >>> >>> return RETURN_SUCCESS; >>> } >>> + >>> +/** >>> + Figures out if we are running inside KVM HVM and >>> + KVM HVM supports SEV Live Migration feature. >>> + >>> + @retval TRUE SEV live migration is supported. >>> + @retval FALSE SEV live migration is not supported. >>> +**/ >>> +BOOLEAN >>> +EFIAPI >>> +KvmDetectSevLiveMigrationFeature( >>> + VOID >>> + ) >>> +{ >>> + CHAR8 Signature[13]; >>> + UINT32 mKvmLeaf; >>> + UINT32 RegEax, RegEbx, RegEcx, RegEdx; >>> + >>> + Signature[12] = '\0'; >>> + for (mKvmLeaf = 0x40000000; mKvmLeaf < 0x40010000; mKvmLeaf += 0x100) { >>> + AsmCpuid (mKvmLeaf, Put the first argument on its own line. >>> + NULL, >>> + (UINT32 *) &Signature[0], >>> + (UINT32 *) &Signature[4], >>> + (UINT32 *) &Signature[8]); >>> + >>> + if (AsciiStrCmp ((CHAR8 *) Signature, "KVMKVMKVM\0\0\0") == 0) { >> >> I assume this will also match if Signature is "KVMKVMKVM\0YZ". I don't >> know if that matters. >> > > I don't understand what do you mean by "KVMKVMKVM\0YZ", this is > comparing for "KVMKVMKVM\0\0\0" ? > AsciiStrCmp will stop at the first '\0' char. So adding those three '\0' at the end is pointless (the compiler will add one '\0' at the end of a literal string). Instead, you can use: if (CompareMem (Signature, "KVMKVMKVM\0\0\0", 12) == 0) and then you are sure to compare all 12 signature bytes. I'm not sure this matters at all, maybe a simple: if (AsciiStrCmp (Signature, "KVMKVMKVM") == 0) is good enough. I see similar code to detect Xen in OvmfPkg/XenPlatformPei/Xen.c . >>> + DEBUG (( >>> + DEBUG_INFO, >>> + "%a: KVM Detected, signature = %s\n", s/%s/%a/ (edk2 format strings are confusing.) >>> + __FUNCTION__, >>> + Signature >>> + )); >>> + >>> + RegEax = mKvmLeaf + 1; >>> + RegEcx = 0; >>> + AsmCpuid (mKvmLeaf + 1, &RegEax, &RegEbx, &RegEcx, &RegEdx); >>> + if ((RegEax & KVM_FEATURE_MIGRATION_CONTROL) != 0) { >>> + DEBUG (( >>> + DEBUG_INFO, >>> + "%a: Live Migration feature supported\n", >> >> I'd write: "%a: SEV Live Migration feature supported\n" >> > Ok. >> >>> + __FUNCTION__ >>> + )); >>> + >>> + return TRUE; >>> + } >>> + } >>> + } >>> + >>> + return FALSE; >>> +} >>> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf >>> index 03a78c32df..3233ca7979 100644 >>> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf >>> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf >>> @@ -38,6 +38,7 @@ >>> X64/PeiDxeVirtualMemory.c >>> X64/VirtualMemory.c >>> X64/VirtualMemory.h >>> + X64/AsmHelperStub.nasm >>> >>> [Sources.IA32] >>> Ia32/MemEncryptSevLib.c >>> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c >>> index e2fd109d12..9db6c2ef71 100644 >>> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c >>> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c >>> @@ -20,6 +20,8 @@ >>> STATIC BOOLEAN mSevStatus = FALSE; >>> STATIC BOOLEAN mSevEsStatus = FALSE; >>> STATIC BOOLEAN mSevStatusChecked = FALSE; >>> +STATIC BOOLEAN mSevLiveMigrationStatus = FALSE; >>> +STATIC BOOLEAN mSevLiveMigrationStatusChecked = FALSE; >>> >>> STATIC UINT64 mSevEncryptionMask = 0; >>> STATIC BOOLEAN mSevEncryptionMaskSaved = FALSE; >>> @@ -87,6 +89,24 @@ InternalMemEncryptSevStatus ( >>> mSevStatusChecked = TRUE; >>> } >>> >>> +/** >>> + Figures out if we are running inside KVM HVM and >>> + KVM HVM supports SEV Live Migration feature. >>> +**/ >>> +STATIC >>> +VOID >>> +EFIAPI >>> +InternalDetectSevLiveMigrationFeature( >>> + VOID >>> + ) >>> +{ >>> + if (KvmDetectSevLiveMigrationFeature()) { >>> + mSevLiveMigrationStatus = TRUE; >>> + } >>> + >>> + mSevLiveMigrationStatusChecked = TRUE; >>> +} >>> + >>> /** >>> Returns a boolean to indicate whether SEV-ES is enabled. >>> >>> @@ -125,6 +145,25 @@ MemEncryptSevIsEnabled ( >>> return mSevStatus; >>> } >>> >>> +/** >>> + Returns a boolean to indicate whether SEV live migration is enabled. >>> + >>> + @retval TRUE SEV live migration is enabled >>> + @retval FALSE SEV live migration is not enabled >>> +**/ >>> +BOOLEAN >>> +EFIAPI >>> +MemEncryptSevLiveMigrationIsEnabled ( >>> + VOID >>> + ) >>> +{ >>> + if (!mSevLiveMigrationStatusChecked) { >>> + InternalDetectSevLiveMigrationFeature (); >>> + } >>> + >>> + return mSevLiveMigrationStatus; >>> +} >>> + >>> /** >>> Returns the SEV encryption mask. >>> >>> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLibInternal.c b/OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLibInternal.c >>> index 56d8f3f318..b926c7b786 100644 >>> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLibInternal.c >>> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLibInternal.c >>> @@ -100,6 +100,44 @@ MemEncryptSevIsEnabled ( >>> return Msr.Bits.SevBit ? TRUE : FALSE; >>> } >>> >>> +/** >>> + Interface exposed by the ASM implementation of the core hypercall >>> + >>> + @retval Hypercall returned status. >>> +**/ >>> +UINTN >>> +EFIAPI >>> +SetMemoryEncDecHypercall3AsmStub ( >>> + IN UINTN HypercallNum, >>> + IN UINTN PhysicalAddress, >>> + IN UINTN Pages, >>> + IN UINTN Attributes >>> + ) >>> +{ >>> + // >>> + // Not used in SEC phase. >>> + // >>> + return RETURN_UNSUPPORTED; >>> +} >>> + >>> +/** >>> + Returns a boolean to indicate whether SEV live migration is enabled. >>> + >>> + @retval TRUE SEV live migration is enabled >>> + @retval FALSE SEV live migration is not enabled >>> +**/ >>> +BOOLEAN >>> +EFIAPI >>> +MemEncryptSevLiveMigrationIsEnabled ( >>> + VOID >>> + ) >>> +{ >>> + // >>> + // Not used in SEC phase. >>> + // >>> + return FALSE; >>> +} >>> + >>> /** >>> Returns the SEV encryption mask. >>> >>> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/AsmHelperStub.nasm b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/AsmHelperStub.nasm >>> new file mode 100644 >>> index 0000000000..c7c11f77f1 >>> --- /dev/null >>> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/AsmHelperStub.nasm >>> @@ -0,0 +1,33 @@ >>> +/** @file >>> + >>> + ASM helper stub to invoke hypercall >>> + >>> + Copyright (c) 2021, AMD Incorporated. All rights reserved.<BR> >>> + >>> + SPDX-License-Identifier: BSD-2-Clause-Patent >>> + >>> +**/ >>> + >>> +DEFAULT REL >>> +SECTION .text >>> + >>> +; UINTN >>> +; EFIAPI >>> +; SetMemoryEncDecHypercall3AsmStub ( >>> +; IN UINTN HypercallNum, >>> +; IN UINTN Arg1, >>> +; IN UINTN Arg2, >>> +; IN UINTN Arg3 >>> +; ); >>> +global ASM_PFX(SetMemoryEncDecHypercall3AsmStub) >>> +ASM_PFX(SetMemoryEncDecHypercall3AsmStub): >>> + ; UEFI calling conventions require RBX to >>> + ; be nonvolatile/callee-saved. >>> + push rbx >>> + mov rax, rcx ; Copy HypercallNumber to rax >>> + mov rbx, rdx ; Copy Arg1 to the register expected by KVM >>> + mov rcx, r8 ; Copy Arg2 to register expected by KVM >>> + mov rdx, r9 ; Copy Arg2 to register expected by KVM >> >> Comment: s/Arg2/Arg3/ >> > Yes. >>> + vmmcall ; Call VMMCALL >>> + pop rbx >>> + ret >>> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c >>> index a57e8fd37f..57447e69dc 100644 >>> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c >>> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c >>> @@ -143,3 +143,57 @@ MemEncryptSevClearMmioPageEncMask ( >>> ); >>> >>> } >>> + >>> +/** >>> + This hyercall is used to notify hypervisor when the page's encryption >>> + state changes. >>> + >>> + @param[in] PhysicalAddress The physical address that is the start address >>> + of a memory region. The physical address is >>> + expected to be PAGE_SIZE aligned. >>> + @param[in] Pages Number of Pages in the memory region. >>> + @param[in] Status Encrypted(1) or Decrypted(0). >>> + >>> +@retval RETURN_SUCCESS Hypercall returned success. >> >> or RETURN_UNSUPPORTED or RETURN_NO_MAPPING. >> > > Ok. > >>> +**/ >>> +RETURN_STATUS >>> +EFIAPI >>> +SetMemoryEncDecHypercall3 ( >>> + IN UINTN PhysicalAddress, >>> + IN UINTN Pages, >>> + IN UINTN Status >> >> Consider: >> >> IN BOOL IsEncrypted >> >> or: >> >> IN MAP_RANGE_MODE EncMode >> >> (it's not a Status in the EFI_STATUS sense that appears all around edk2). >> > Ok, i think i will prefer something like a MAP_RANGE_MODE. >> >>> + ) >>> +{ >>> + RETURN_STATUS Ret; >>> + INTN Error; >>> + >> >> Add assert for the expected alignment of PhysicalAddress, and then >> you don't need to round it down when calling SetMemoryEncDecHypercall3AsmStub. >> > > Cannot really use an assert here, as when the GCD map is being walked > and the c-bit being cleared from MMIO and NonExistent memory spaces, the > physical address range being passed may not be page-aligned, so adding > an assert here prevents booting. Hence, rounding it down when calling > SetMemoryEncDecHypercall3AsmStub below. > OK. So fix the comment above the function (which says: "The physical address is expected to be PAGE_SIZE aligned.") >> >>> + Ret = RETURN_UNSUPPORTED; >>> + >>> + if (MemEncryptSevLiveMigrationIsEnabled ()) { >>> + Ret = EFI_SUCCESS; >>> + // >>> + // The encryption bit is set/clear on the smallest page size, hence >>> + // use the 4k page size in MAP_GPA_RANGE hypercall below. >>> + // Also, the hypercall expects the guest physical address to be >>> + // page-aligned. >>> + // >>> + Error = SetMemoryEncDecHypercall3AsmStub ( >>> + KVM_HC_MAP_GPA_RANGE, >>> + (PhysicalAddress & (~(EFI_PAGE_SIZE-1))), >> >> Simpler: >> >> PhysicalAddress & ~EFI_PAGE_MASK >> >> > > Ok. > >>> + Pages, >>> + KVM_MAP_GPA_RANGE_PAGE_SZ_4K | KVM_MAP_GPA_RANGE_ENC_STAT(Status) >>> + ); >>> + >>> + if (Error != 0) { >>> + DEBUG ((DEBUG_ERROR, >>> + "SetMemoryEncDecHypercall3 failed, Phys = %Lx, Pages = %Ld, Err = %Ld\n", >>> + PhysicalAddress, >>> + Pages, >>> + (INT64)Error)); >>> + >>> + Ret = RETURN_NO_MAPPING; >>> + } >>> + } >>> + >>> + return Ret; >>> +} >>> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c >>> index c696745f9d..0b1588a4c1 100644 >>> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c >>> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c >>> @@ -536,7 +536,6 @@ EnableReadOnlyPageWriteProtect ( >>> AsmWriteCr0 (AsmReadCr0() | BIT16); >>> } >>> >>> - >>> /** >>> This function either sets or clears memory encryption bit for the memory >>> region specified by PhysicalAddress and Length from the current page table >>> @@ -585,6 +584,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 +638,10 @@ SetMemoryEncDec ( >>> >>> Status = EFI_SUCCESS; >>> >>> + Size = Length; >>> + CBitChanged = FALSE; >>> + OrigPhysicalAddress = PhysicalAddress; >>> + >>> while (Length != 0) >>> { >>> // >>> @@ -695,6 +701,7 @@ SetMemoryEncDec ( >>> )); >>> PhysicalAddress += BIT30; >>> Length -= BIT30; >>> + CBitChanged = TRUE; >>> } else { >>> // >>> // We must split the page >>> @@ -749,6 +756,7 @@ SetMemoryEncDec ( >>> SetOrClearCBit (&PageDirectory2MEntry->Uint64, Mode); >>> PhysicalAddress += BIT21; >>> Length -= BIT21; >>> + CBitChanged = TRUE; >>> } else { >>> // >>> // We must split up this page into 4K pages >>> @@ -791,6 +799,7 @@ SetMemoryEncDec ( >>> SetOrClearCBit (&PageTableEntry->Uint64, Mode); >>> PhysicalAddress += EFI_PAGE_SIZE; >>> Length -= EFI_PAGE_SIZE; >>> + CBitChanged = TRUE; >>> } >>> } >>> } >>> @@ -808,6 +817,17 @@ SetMemoryEncDec ( >>> // >>> CpuFlushTlb(); >>> >>> + // >>> + // Notify Hypervisor on C-bit status >>> + // >>> + if (CBitChanged) { >>> + Status = SetMemoryEncDecHypercall3 ( >>> + OrigPhysicalAddress, >>> + EFI_SIZE_TO_PAGES(Size), >>> + !Mode >> >> Here you pass !Mode (which is 0 or 1) as the third argument to >> SetMemoryEncDecHypercall3 . >> >> But on patch 3/4 you pass KVM_MAP_GPA_RANGE_DECRYPTED (which is 0<<4); >> but that hints that you expect either 0<<4 or 1<<4 as this third argument. >> If this is the case, then here it should be: >> >> (Mode == SetCBit) ? KVM_MAP_GPA_RANGE_ENCRYPTED : KVM_MAP_GPA_RANGE_DECRYPTED >> >> If it's the other way around, then patch 3/4 needs to pass a simple 0 as >> the third argument. >> > > Yes, i need to pass a 0 (decrypted) as the third argument in that patch. > Even clearer than a literal 0 -- pass a ClearCBit as the third argument (assuming you're changing the argument type to MAP_RANGE_MODE). -Dov > Thanks, > Ashish >> >> >> >>> + ); >>> + } >>> + >>> Done: >>> // >>> // Restore page table write protection, if any. >>> ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v5 1/4] OvmfPkg/BaseMemEncryptLib: Support to issue unencrypted hypercall 2021-07-19 8:04 ` Dov Murik @ 2021-07-19 15:30 ` Ashish Kalra 0 siblings, 0 replies; 17+ messages in thread From: Ashish Kalra @ 2021-07-19 15:30 UTC (permalink / raw) To: Dov Murik Cc: devel, dovmurik, brijesh.singh, tobin, Thomas.Lendacky, jejb, lersek, jordan.l.justen, ard.biesheuvel, erdemaktas, jiewen.yao, min.m.xu Hello Dov, On Mon, Jul 19, 2021 at 11:04:17AM +0300, Dov Murik wrote: > > > On 16/07/2021 15:29, Ashish Kalra wrote: > > Hello Dov, > > > > On Thu, Jul 15, 2021 at 11:58:17PM +0300, Dov Murik wrote: > >> Hi Ashish, > >> > >> On 08/07/2021 17:07, 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. > >>> > >>> This hypercall is used to notify hypervisor when the page's > >>> encryption state changes. > >>> > >>> 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/Include/Library/MemEncryptSevLib.h | 69 ++++++++++++++++++++ > >>> OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf | 1 + > >>> OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLibInternal.c | 39 +++++++++++ > >>> OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c | 27 ++++++++ > >>> OvmfPkg/Library/BaseMemEncryptSevLib/PeiDxeMemEncryptSevLibInternal.c | 51 +++++++++++++++ > >>> OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf | 1 + > >>> OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c | 39 +++++++++++ > >>> OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLibInternal.c | 38 +++++++++++ > >>> OvmfPkg/Library/BaseMemEncryptSevLib/X64/AsmHelperStub.nasm | 33 ++++++++++ > >>> OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c | 54 +++++++++++++++ > >>> OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c | 22 ++++++- > >>> 11 files changed, 373 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/OvmfPkg/Include/Library/MemEncryptSevLib.h b/OvmfPkg/Include/Library/MemEncryptSevLib.h > >>> index 76d06c206c..c2b2a99a08 100644 > >>> --- a/OvmfPkg/Include/Library/MemEncryptSevLib.h > >>> +++ b/OvmfPkg/Include/Library/MemEncryptSevLib.h > >>> @@ -90,6 +90,18 @@ MemEncryptSevIsEnabled ( > >>> VOID > >>> ); > >>> > >>> +/** > >>> + Returns a boolean to indicate whether SEV live migration is enabled. > >>> + > >>> + @retval TRUE SEV live migration is enabled > >>> + @retval FALSE SEV live migration is not enabled > >>> +**/ > >>> +BOOLEAN > >>> +EFIAPI > >>> +MemEncryptSevLiveMigrationIsEnabled ( > >>> + VOID > >>> + ); > >>> + > >>> /** > >>> This function clears memory encryption bit for the memory region specified by > >>> BaseAddress and NumPages from the current page table context. > >>> @@ -222,4 +234,61 @@ MemEncryptSevClearMmioPageEncMask ( > >>> IN UINTN NumPages > >>> ); > >>> > >>> +/** > >>> + This hypercall is used to notify hypervisor when the page's encryption > >>> + state changes. > >>> + > >>> + @param[in] PhysicalAddress The physical address that is the start address > >>> + of a memory region. The PhysicalAddress is > >>> + expected to be PAGE_SIZE aligned. > >>> + @param[in] Pages Number of pages in memory region. > >>> + @param[in] Status Encrypted(1) or Decrypted(0). > >>> + > >>> +@retval RETURN_SUCCESS Hypercall returned success. > >>> +**/ > >>> +RETURN_STATUS > >>> +EFIAPI > >>> +SetMemoryEncDecHypercall3 ( > >>> + IN UINTN PhysicalAddress, > >>> + IN UINTN Pages, > >>> + IN UINTN Status > >>> + ); > >>> + > >>> +#define KVM_HC_MAP_GPA_RANGE 12 > >>> +#define KVM_MAP_GPA_RANGE_PAGE_SZ_4K 0 > >>> +#define KVM_MAP_GPA_RANGE_PAGE_SZ_2M BIT0 > >>> +#define KVM_MAP_GPA_RANGE_PAGE_SZ_1G BIT1 > >>> +#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) > >>> + > >>> +#define KVM_FEATURE_MIGRATION_CONTROL BIT17 > >>> + > >>> +/** > >>> + Figures out if we are running inside KVM HVM and > >>> + KVM HVM supports SEV Live Migration feature. > >>> + > >>> + @retval TRUE SEV live migration is supported. > >>> + @retval FALSE SEV live migration is not supported. > >>> +**/ > >>> +BOOLEAN > >>> +EFIAPI > >>> +KvmDetectSevLiveMigrationFeature( > >>> + VOID > >>> + ); > >>> + > >>> +/** > >>> + Interface exposed by the ASM implementation of the core hypercall > >>> + > >>> + @retval Hypercall returned status. > >>> +**/ > >>> +UINTN > >>> +EFIAPI > >>> +SetMemoryEncDecHypercall3AsmStub ( > >>> + IN UINTN HypercallNum, > >>> + IN UINTN PhysicalAddress, > >>> + IN UINTN Pages, > >>> + IN UINTN Attributes > >>> + ); > >>> + > >>> #endif // _MEM_ENCRYPT_SEV_LIB_H_ > >>> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf b/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf > >>> index f2e162d680..0c28afadee 100644 > >>> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf > >>> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf > >>> @@ -38,6 +38,7 @@ > >>> X64/PeiDxeVirtualMemory.c > >>> X64/VirtualMemory.c > >>> X64/VirtualMemory.h > >>> + X64/AsmHelperStub.nasm > >>> > >>> [Sources.IA32] > >>> Ia32/MemEncryptSevLib.c > >>> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLibInternal.c b/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLibInternal.c > >>> index 2816f859a0..ead754cd7b 100644 > >>> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLibInternal.c > >>> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLibInternal.c > >>> @@ -20,6 +20,8 @@ > >>> STATIC BOOLEAN mSevStatus = FALSE; > >>> STATIC BOOLEAN mSevEsStatus = FALSE; > >>> STATIC BOOLEAN mSevStatusChecked = FALSE; > >>> +STATIC BOOLEAN mSevLiveMigrationStatus = FALSE; > >>> +STATIC BOOLEAN mSevLiveMigrationStatusChecked = FALSE; > >>> > >>> STATIC UINT64 mSevEncryptionMask = 0; > >>> STATIC BOOLEAN mSevEncryptionMaskSaved = FALSE; > >>> @@ -87,6 +89,24 @@ InternalMemEncryptSevStatus ( > >>> mSevStatusChecked = TRUE; > >>> } > >>> > >>> +/** > >>> + Figures out if we are running inside KVM HVM and > >>> + KVM HVM supports SEV Live Migration feature. > >>> +**/ > >>> +STATIC > >>> +VOID > >>> +EFIAPI > >>> +InternalDetectSevLiveMigrationFeature( > >>> + VOID > >>> + ) > >>> +{ > >>> + if (KvmDetectSevLiveMigrationFeature()) { > >>> + mSevLiveMigrationStatus = TRUE; > >>> + } > >>> + > >>> + mSevLiveMigrationStatusChecked = TRUE; > >>> +} > >>> + > >>> /** > >>> Returns a boolean to indicate whether SEV-ES is enabled. > >>> > >>> @@ -125,6 +145,25 @@ MemEncryptSevIsEnabled ( > >>> return mSevStatus; > >>> } > >>> > >>> +/** > >>> + Returns a boolean to indicate whether SEV live migration is enabled. > >>> + > >>> + @retval TRUE SEV live migration is enabled > >>> + @retval FALSE SEV live migration is not enabled > >>> +**/ > >>> +BOOLEAN > >>> +EFIAPI > >>> +MemEncryptSevLiveMigrationIsEnabled ( > >>> + VOID > >>> + ) > >>> +{ > >>> + if (!mSevLiveMigrationStatusChecked) { > >>> + InternalDetectSevLiveMigrationFeature (); > >>> + } > >>> + > >>> + return mSevLiveMigrationStatus; > >>> +} > >>> + > >>> /** > >>> Returns the SEV encryption mask. > >>> > >>> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c b/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c > >>> index be260e0d10..62392309fe 100644 > >>> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c > >>> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c > >>> @@ -136,3 +136,30 @@ MemEncryptSevClearMmioPageEncMask ( > >>> // > >>> return RETURN_UNSUPPORTED; > >>> } > >>> + > >>> +/** > >>> + This hyercall is used to notify hypervisor when the page's encryption > >>> + state changes. > >>> + > >>> + @param[in] PhysicalAddress The physical address that is the start address > >>> + of a memory region. The physical address is > >>> + expected to be PAGE_SIZE aligned. > >>> + @param[in] Pages Number of Pages in the memory region. > >>> + @param[in] Status Encrypted(1) or Decrypted(0). > >>> + > >>> +@retval RETURN_SUCCESS Hypercall returned success. > >>> +**/ > >>> +RETURN_STATUS > >>> +EFIAPI > >>> +SetMemoryEncDecHypercall3 ( > >>> + IN UINTN PhysicalAddress, > >>> + IN UINTN Pages, > >>> + IN UINTN Status > >>> + ) > >>> +{ > >>> + // > >>> + // Memory encryption bit is not accessible in 32-bit mode > >>> + // > >>> + return RETURN_UNSUPPORTED; > >>> +} > >>> + > >>> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiDxeMemEncryptSevLibInternal.c b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiDxeMemEncryptSevLibInternal.c > >>> index b4a9f464e2..0c9f7e17ba 100644 > >>> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiDxeMemEncryptSevLibInternal.c > >>> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiDxeMemEncryptSevLibInternal.c > >>> @@ -61,3 +61,54 @@ MemEncryptSevLocateInitialSmramSaveStateMapPages ( > >>> > >>> return RETURN_SUCCESS; > >>> } > >>> + > >>> +/** > >>> + Figures out if we are running inside KVM HVM and > >>> + KVM HVM supports SEV Live Migration feature. > >>> + > >>> + @retval TRUE SEV live migration is supported. > >>> + @retval FALSE SEV live migration is not supported. > >>> +**/ > >>> +BOOLEAN > >>> +EFIAPI > >>> +KvmDetectSevLiveMigrationFeature( > >>> + VOID > >>> + ) > >>> +{ > >>> + CHAR8 Signature[13]; > >>> + UINT32 mKvmLeaf; > >>> + UINT32 RegEax, RegEbx, RegEcx, RegEdx; > >>> + > >>> + Signature[12] = '\0'; > >>> + for (mKvmLeaf = 0x40000000; mKvmLeaf < 0x40010000; mKvmLeaf += 0x100) { > >>> + AsmCpuid (mKvmLeaf, > > Put the first argument on its own line. > Ok. > > >>> + NULL, > >>> + (UINT32 *) &Signature[0], > >>> + (UINT32 *) &Signature[4], > >>> + (UINT32 *) &Signature[8]); > >>> + > >>> + if (AsciiStrCmp ((CHAR8 *) Signature, "KVMKVMKVM\0\0\0") == 0) { > >> > >> I assume this will also match if Signature is "KVMKVMKVM\0YZ". I don't > >> know if that matters. > >> > > > > I don't understand what do you mean by "KVMKVMKVM\0YZ", this is > > comparing for "KVMKVMKVM\0\0\0" ? > > > > AsciiStrCmp will stop at the first '\0' char. So adding those three > '\0' at the end is pointless (the compiler will add one '\0' at the end > of a literal string). > > > Instead, you can use: > > if (CompareMem (Signature, "KVMKVMKVM\0\0\0", 12) == 0) > > and then you are sure to compare all 12 signature bytes. > > > I'm not sure this matters at all, maybe a simple: > > if (AsciiStrCmp (Signature, "KVMKVMKVM") == 0) > > is good enough. I see similar code to detect Xen in > OvmfPkg/XenPlatformPei/Xen.c . > > Yes, as with the Xen detection code, AsciiStrCmp(Signature, "KVMKVMKVM") should be good. > > > >>> + DEBUG (( > >>> + DEBUG_INFO, > >>> + "%a: KVM Detected, signature = %s\n", > > s/%s/%a/ > > (edk2 format strings are confusing.) > Ok. > >>> + __FUNCTION__, > >>> + Signature > >>> + )); > >>> + > >>> + RegEax = mKvmLeaf + 1; > >>> + RegEcx = 0; > >>> + AsmCpuid (mKvmLeaf + 1, &RegEax, &RegEbx, &RegEcx, &RegEdx); > >>> + if ((RegEax & KVM_FEATURE_MIGRATION_CONTROL) != 0) { > >>> + DEBUG (( > >>> + DEBUG_INFO, > >>> + "%a: Live Migration feature supported\n", > >> > >> I'd write: "%a: SEV Live Migration feature supported\n" > >> > > Ok. > >> > >>> + __FUNCTION__ > >>> + )); > >>> + > >>> + return TRUE; > >>> + } > >>> + } > >>> + } > >>> + > >>> + return FALSE; > >>> +} > >>> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf > >>> index 03a78c32df..3233ca7979 100644 > >>> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf > >>> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf > >>> @@ -38,6 +38,7 @@ > >>> X64/PeiDxeVirtualMemory.c > >>> X64/VirtualMemory.c > >>> X64/VirtualMemory.h > >>> + X64/AsmHelperStub.nasm > >>> > >>> [Sources.IA32] > >>> Ia32/MemEncryptSevLib.c > >>> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c > >>> index e2fd109d12..9db6c2ef71 100644 > >>> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c > >>> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c > >>> @@ -20,6 +20,8 @@ > >>> STATIC BOOLEAN mSevStatus = FALSE; > >>> STATIC BOOLEAN mSevEsStatus = FALSE; > >>> STATIC BOOLEAN mSevStatusChecked = FALSE; > >>> +STATIC BOOLEAN mSevLiveMigrationStatus = FALSE; > >>> +STATIC BOOLEAN mSevLiveMigrationStatusChecked = FALSE; > >>> > >>> STATIC UINT64 mSevEncryptionMask = 0; > >>> STATIC BOOLEAN mSevEncryptionMaskSaved = FALSE; > >>> @@ -87,6 +89,24 @@ InternalMemEncryptSevStatus ( > >>> mSevStatusChecked = TRUE; > >>> } > >>> > >>> +/** > >>> + Figures out if we are running inside KVM HVM and > >>> + KVM HVM supports SEV Live Migration feature. > >>> +**/ > >>> +STATIC > >>> +VOID > >>> +EFIAPI > >>> +InternalDetectSevLiveMigrationFeature( > >>> + VOID > >>> + ) > >>> +{ > >>> + if (KvmDetectSevLiveMigrationFeature()) { > >>> + mSevLiveMigrationStatus = TRUE; > >>> + } > >>> + > >>> + mSevLiveMigrationStatusChecked = TRUE; > >>> +} > >>> + > >>> /** > >>> Returns a boolean to indicate whether SEV-ES is enabled. > >>> > >>> @@ -125,6 +145,25 @@ MemEncryptSevIsEnabled ( > >>> return mSevStatus; > >>> } > >>> > >>> +/** > >>> + Returns a boolean to indicate whether SEV live migration is enabled. > >>> + > >>> + @retval TRUE SEV live migration is enabled > >>> + @retval FALSE SEV live migration is not enabled > >>> +**/ > >>> +BOOLEAN > >>> +EFIAPI > >>> +MemEncryptSevLiveMigrationIsEnabled ( > >>> + VOID > >>> + ) > >>> +{ > >>> + if (!mSevLiveMigrationStatusChecked) { > >>> + InternalDetectSevLiveMigrationFeature (); > >>> + } > >>> + > >>> + return mSevLiveMigrationStatus; > >>> +} > >>> + > >>> /** > >>> Returns the SEV encryption mask. > >>> > >>> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLibInternal.c b/OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLibInternal.c > >>> index 56d8f3f318..b926c7b786 100644 > >>> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLibInternal.c > >>> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLibInternal.c > >>> @@ -100,6 +100,44 @@ MemEncryptSevIsEnabled ( > >>> return Msr.Bits.SevBit ? TRUE : FALSE; > >>> } > >>> > >>> +/** > >>> + Interface exposed by the ASM implementation of the core hypercall > >>> + > >>> + @retval Hypercall returned status. > >>> +**/ > >>> +UINTN > >>> +EFIAPI > >>> +SetMemoryEncDecHypercall3AsmStub ( > >>> + IN UINTN HypercallNum, > >>> + IN UINTN PhysicalAddress, > >>> + IN UINTN Pages, > >>> + IN UINTN Attributes > >>> + ) > >>> +{ > >>> + // > >>> + // Not used in SEC phase. > >>> + // > >>> + return RETURN_UNSUPPORTED; > >>> +} > >>> + > >>> +/** > >>> + Returns a boolean to indicate whether SEV live migration is enabled. > >>> + > >>> + @retval TRUE SEV live migration is enabled > >>> + @retval FALSE SEV live migration is not enabled > >>> +**/ > >>> +BOOLEAN > >>> +EFIAPI > >>> +MemEncryptSevLiveMigrationIsEnabled ( > >>> + VOID > >>> + ) > >>> +{ > >>> + // > >>> + // Not used in SEC phase. > >>> + // > >>> + return FALSE; > >>> +} > >>> + > >>> /** > >>> Returns the SEV encryption mask. > >>> > >>> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/AsmHelperStub.nasm b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/AsmHelperStub.nasm > >>> new file mode 100644 > >>> index 0000000000..c7c11f77f1 > >>> --- /dev/null > >>> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/AsmHelperStub.nasm > >>> @@ -0,0 +1,33 @@ > >>> +/** @file > >>> + > >>> + ASM helper stub to invoke hypercall > >>> + > >>> + Copyright (c) 2021, AMD Incorporated. All rights reserved.<BR> > >>> + > >>> + SPDX-License-Identifier: BSD-2-Clause-Patent > >>> + > >>> +**/ > >>> + > >>> +DEFAULT REL > >>> +SECTION .text > >>> + > >>> +; UINTN > >>> +; EFIAPI > >>> +; SetMemoryEncDecHypercall3AsmStub ( > >>> +; IN UINTN HypercallNum, > >>> +; IN UINTN Arg1, > >>> +; IN UINTN Arg2, > >>> +; IN UINTN Arg3 > >>> +; ); > >>> +global ASM_PFX(SetMemoryEncDecHypercall3AsmStub) > >>> +ASM_PFX(SetMemoryEncDecHypercall3AsmStub): > >>> + ; UEFI calling conventions require RBX to > >>> + ; be nonvolatile/callee-saved. > >>> + push rbx > >>> + mov rax, rcx ; Copy HypercallNumber to rax > >>> + mov rbx, rdx ; Copy Arg1 to the register expected by KVM > >>> + mov rcx, r8 ; Copy Arg2 to register expected by KVM > >>> + mov rdx, r9 ; Copy Arg2 to register expected by KVM > >> > >> Comment: s/Arg2/Arg3/ > >> > > Yes. > >>> + vmmcall ; Call VMMCALL > >>> + pop rbx > >>> + ret > >>> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c > >>> index a57e8fd37f..57447e69dc 100644 > >>> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c > >>> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c > >>> @@ -143,3 +143,57 @@ MemEncryptSevClearMmioPageEncMask ( > >>> ); > >>> > >>> } > >>> + > >>> +/** > >>> + This hyercall is used to notify hypervisor when the page's encryption > >>> + state changes. > >>> + > >>> + @param[in] PhysicalAddress The physical address that is the start address > >>> + of a memory region. The physical address is > >>> + expected to be PAGE_SIZE aligned. > >>> + @param[in] Pages Number of Pages in the memory region. > >>> + @param[in] Status Encrypted(1) or Decrypted(0). > >>> + > >>> +@retval RETURN_SUCCESS Hypercall returned success. > >> > >> or RETURN_UNSUPPORTED or RETURN_NO_MAPPING. > >> > > > > Ok. > > > >>> +**/ > >>> +RETURN_STATUS > >>> +EFIAPI > >>> +SetMemoryEncDecHypercall3 ( > >>> + IN UINTN PhysicalAddress, > >>> + IN UINTN Pages, > >>> + IN UINTN Status > >> > >> Consider: > >> > >> IN BOOL IsEncrypted > >> > >> or: > >> > >> IN MAP_RANGE_MODE EncMode > >> > >> (it's not a Status in the EFI_STATUS sense that appears all around edk2). > >> > > Ok, i think i will prefer something like a MAP_RANGE_MODE. > >> > >>> + ) > >>> +{ > >>> + RETURN_STATUS Ret; > >>> + INTN Error; > >>> + > >> > >> Add assert for the expected alignment of PhysicalAddress, and then > >> you don't need to round it down when calling SetMemoryEncDecHypercall3AsmStub. > >> > > > > Cannot really use an assert here, as when the GCD map is being walked > > and the c-bit being cleared from MMIO and NonExistent memory spaces, the > > physical address range being passed may not be page-aligned, so adding > > an assert here prevents booting. Hence, rounding it down when calling > > SetMemoryEncDecHypercall3AsmStub below. > > > > OK. So fix the comment above the function (which says: "The physical > address is expected to be PAGE_SIZE aligned.") > Ok. > > >> > >>> + Ret = RETURN_UNSUPPORTED; > >>> + > >>> + if (MemEncryptSevLiveMigrationIsEnabled ()) { > >>> + Ret = EFI_SUCCESS; > >>> + // > >>> + // The encryption bit is set/clear on the smallest page size, hence > >>> + // use the 4k page size in MAP_GPA_RANGE hypercall below. > >>> + // Also, the hypercall expects the guest physical address to be > >>> + // page-aligned. > >>> + // > >>> + Error = SetMemoryEncDecHypercall3AsmStub ( > >>> + KVM_HC_MAP_GPA_RANGE, > >>> + (PhysicalAddress & (~(EFI_PAGE_SIZE-1))), > >> > >> Simpler: > >> > >> PhysicalAddress & ~EFI_PAGE_MASK > >> > >> > > > > Ok. > > > >>> + Pages, > >>> + KVM_MAP_GPA_RANGE_PAGE_SZ_4K | KVM_MAP_GPA_RANGE_ENC_STAT(Status) > >>> + ); > >>> + > >>> + if (Error != 0) { > >>> + DEBUG ((DEBUG_ERROR, > >>> + "SetMemoryEncDecHypercall3 failed, Phys = %Lx, Pages = %Ld, Err = %Ld\n", > >>> + PhysicalAddress, > >>> + Pages, > >>> + (INT64)Error)); > >>> + > >>> + Ret = RETURN_NO_MAPPING; > >>> + } > >>> + } > >>> + > >>> + return Ret; > >>> +} > >>> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c > >>> index c696745f9d..0b1588a4c1 100644 > >>> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c > >>> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c > >>> @@ -536,7 +536,6 @@ EnableReadOnlyPageWriteProtect ( > >>> AsmWriteCr0 (AsmReadCr0() | BIT16); > >>> } > >>> > >>> - > >>> /** > >>> This function either sets or clears memory encryption bit for the memory > >>> region specified by PhysicalAddress and Length from the current page table > >>> @@ -585,6 +584,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 +638,10 @@ SetMemoryEncDec ( > >>> > >>> Status = EFI_SUCCESS; > >>> > >>> + Size = Length; > >>> + CBitChanged = FALSE; > >>> + OrigPhysicalAddress = PhysicalAddress; > >>> + > >>> while (Length != 0) > >>> { > >>> // > >>> @@ -695,6 +701,7 @@ SetMemoryEncDec ( > >>> )); > >>> PhysicalAddress += BIT30; > >>> Length -= BIT30; > >>> + CBitChanged = TRUE; > >>> } else { > >>> // > >>> // We must split the page > >>> @@ -749,6 +756,7 @@ SetMemoryEncDec ( > >>> SetOrClearCBit (&PageDirectory2MEntry->Uint64, Mode); > >>> PhysicalAddress += BIT21; > >>> Length -= BIT21; > >>> + CBitChanged = TRUE; > >>> } else { > >>> // > >>> // We must split up this page into 4K pages > >>> @@ -791,6 +799,7 @@ SetMemoryEncDec ( > >>> SetOrClearCBit (&PageTableEntry->Uint64, Mode); > >>> PhysicalAddress += EFI_PAGE_SIZE; > >>> Length -= EFI_PAGE_SIZE; > >>> + CBitChanged = TRUE; > >>> } > >>> } > >>> } > >>> @@ -808,6 +817,17 @@ SetMemoryEncDec ( > >>> // > >>> CpuFlushTlb(); > >>> > >>> + // > >>> + // Notify Hypervisor on C-bit status > >>> + // > >>> + if (CBitChanged) { > >>> + Status = SetMemoryEncDecHypercall3 ( > >>> + OrigPhysicalAddress, > >>> + EFI_SIZE_TO_PAGES(Size), > >>> + !Mode > >> > >> Here you pass !Mode (which is 0 or 1) as the third argument to > >> SetMemoryEncDecHypercall3 . > >> > >> But on patch 3/4 you pass KVM_MAP_GPA_RANGE_DECRYPTED (which is 0<<4); > >> but that hints that you expect either 0<<4 or 1<<4 as this third argument. > >> If this is the case, then here it should be: > >> > >> (Mode == SetCBit) ? KVM_MAP_GPA_RANGE_ENCRYPTED : KVM_MAP_GPA_RANGE_DECRYPTED > >> > >> If it's the other way around, then patch 3/4 needs to pass a simple 0 as > >> the third argument. > >> > > > > Yes, i need to pass a 0 (decrypted) as the third argument in that patch. > > > > Even clearer than a literal 0 -- pass a ClearCBit as the third argument > (assuming you're changing the argument type to MAP_RANGE_MODE). > Ok. Thanks, Ashish > >> > >> > >>> + ); > >>> + } > >>> + > >>> Done: > >>> // > >>> // Restore page table write protection, if any. > >>> ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v5 1/4] OvmfPkg/BaseMemEncryptLib: Support to issue unencrypted hypercall 2021-07-08 14:07 ` [PATCH v5 1/4] OvmfPkg/BaseMemEncryptLib: Support to issue unencrypted hypercall Ashish Kalra 2021-07-15 20:58 ` Dov Murik @ 2021-07-16 14:11 ` Lendacky, Thomas 2021-07-19 20:24 ` Ashish Kalra 1 sibling, 1 reply; 17+ messages in thread From: Lendacky, Thomas @ 2021-07-16 14:11 UTC (permalink / raw) To: Ashish Kalra, devel Cc: dovmurik, brijesh.singh, tobin, jejb, lersek, jordan.l.justen, ard.biesheuvel, erdemaktas, jiewen.yao, min.m.xu On 7/8/21 9:07 AM, Ashish Kalra wrote: > From: Ashish Kalra <ashish.kalra@amd.com> > The patch subject is a bit confusing. Something more like "Add API to issue hypercall on page encryption state change" or similar, since this is issued for changes to shared and private, not just shared. > 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. > > This hypercall is used to notify hypervisor when the page's > encryption state changes. This is a large patch. It looks like this should be split into a few patches. - one patch for the MemEncryptSevLiveMigrationIsEnabled() API - one patch for the SetMemoryEncDecHypercall3() API - one patch to make use of the SetMemoryEncDecHypercall3() API. > > 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/Include/Library/MemEncryptSevLib.h | 69 ++++++++++++++++++++ > OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf | 1 + > OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLibInternal.c | 39 +++++++++++ > OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c | 27 ++++++++ > OvmfPkg/Library/BaseMemEncryptSevLib/PeiDxeMemEncryptSevLibInternal.c | 51 +++++++++++++++ > OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf | 1 + > OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c | 39 +++++++++++ > OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLibInternal.c | 38 +++++++++++ > OvmfPkg/Library/BaseMemEncryptSevLib/X64/AsmHelperStub.nasm | 33 ++++++++++ > OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c | 54 +++++++++++++++ > OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c | 22 ++++++- > 11 files changed, 373 insertions(+), 1 deletion(-) > > diff --git a/OvmfPkg/Include/Library/MemEncryptSevLib.h b/OvmfPkg/Include/Library/MemEncryptSevLib.h > index 76d06c206c..c2b2a99a08 100644 > --- a/OvmfPkg/Include/Library/MemEncryptSevLib.h > +++ b/OvmfPkg/Include/Library/MemEncryptSevLib.h > @@ -90,6 +90,18 @@ MemEncryptSevIsEnabled ( > VOID > ); > > +/** > + Returns a boolean to indicate whether SEV live migration is enabled. > + > + @retval TRUE SEV live migration is enabled > + @retval FALSE SEV live migration is not enabled > +**/ > +BOOLEAN > +EFIAPI > +MemEncryptSevLiveMigrationIsEnabled ( > + VOID > + ); > + > /** > This function clears memory encryption bit for the memory region specified by > BaseAddress and NumPages from the current page table context. > @@ -222,4 +234,61 @@ MemEncryptSevClearMmioPageEncMask ( > IN UINTN NumPages > ); > > +/** > + This hypercall is used to notify hypervisor when the page's encryption > + state changes. > + > + @param[in] PhysicalAddress The physical address that is the start address > + of a memory region. The PhysicalAddress is > + expected to be PAGE_SIZE aligned. > + @param[in] Pages Number of pages in memory region. > + @param[in] Status Encrypted(1) or Decrypted(0). > + > +@retval RETURN_SUCCESS Hypercall returned success. It looks like RETURN_UNSUPPORTED is also possible. > +**/ > +RETURN_STATUS > +EFIAPI > +SetMemoryEncDecHypercall3 ( > + IN UINTN PhysicalAddress, > + IN UINTN Pages, > + IN UINTN Status > + ); > + > +#define KVM_HC_MAP_GPA_RANGE 12 > +#define KVM_MAP_GPA_RANGE_PAGE_SZ_4K 0 > +#define KVM_MAP_GPA_RANGE_PAGE_SZ_2M BIT0 > +#define KVM_MAP_GPA_RANGE_PAGE_SZ_1G BIT1 > +#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) You define these but don't use them (and you should). > + > +#define KVM_FEATURE_MIGRATION_CONTROL BIT17 > + > +/** > + Figures out if we are running inside KVM HVM and > + KVM HVM supports SEV Live Migration feature. > + > + @retval TRUE SEV live migration is supported. > + @retval FALSE SEV live migration is not supported. > +**/ > +BOOLEAN > +EFIAPI > +KvmDetectSevLiveMigrationFeature( > + VOID > + ); > + > +/** > + Interface exposed by the ASM implementation of the core hypercall > + > + @retval Hypercall returned status. > +**/ > +UINTN > +EFIAPI > +SetMemoryEncDecHypercall3AsmStub ( > + IN UINTN HypercallNum, > + IN UINTN PhysicalAddress, > + IN UINTN Pages, > + IN UINTN Attributes > + ); > + > #endif // _MEM_ENCRYPT_SEV_LIB_H_ > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf b/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf > index f2e162d680..0c28afadee 100644 > --- a/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf > @@ -38,6 +38,7 @@ > X64/PeiDxeVirtualMemory.c > X64/VirtualMemory.c > X64/VirtualMemory.h > + X64/AsmHelperStub.nasm > > [Sources.IA32] > Ia32/MemEncryptSevLib.c > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLibInternal.c b/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLibInternal.c > index 2816f859a0..ead754cd7b 100644 > --- a/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLibInternal.c > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLibInternal.c > @@ -20,6 +20,8 @@ > STATIC BOOLEAN mSevStatus = FALSE; > STATIC BOOLEAN mSevEsStatus = FALSE; > STATIC BOOLEAN mSevStatusChecked = FALSE; > +STATIC BOOLEAN mSevLiveMigrationStatus = FALSE; > +STATIC BOOLEAN mSevLiveMigrationStatusChecked = FALSE; > > STATIC UINT64 mSevEncryptionMask = 0; > STATIC BOOLEAN mSevEncryptionMaskSaved = FALSE; > @@ -87,6 +89,24 @@ InternalMemEncryptSevStatus ( > mSevStatusChecked = TRUE; > } > > +/** > + Figures out if we are running inside KVM HVM and > + KVM HVM supports SEV Live Migration feature. > +**/ > +STATIC > +VOID > +EFIAPI > +InternalDetectSevLiveMigrationFeature( > + VOID > + ) > +{ > + if (KvmDetectSevLiveMigrationFeature()) { > + mSevLiveMigrationStatus = TRUE; > + } > + > + mSevLiveMigrationStatusChecked = TRUE; > +} > + > /** > Returns a boolean to indicate whether SEV-ES is enabled. > > @@ -125,6 +145,25 @@ MemEncryptSevIsEnabled ( > return mSevStatus; > } > > +/** > + Returns a boolean to indicate whether SEV live migration is enabled. > + > + @retval TRUE SEV live migration is enabled > + @retval FALSE SEV live migration is not enabled > +**/ > +BOOLEAN > +EFIAPI > +MemEncryptSevLiveMigrationIsEnabled ( > + VOID > + ) > +{ > + if (!mSevLiveMigrationStatusChecked) { > + InternalDetectSevLiveMigrationFeature (); > + } > + > + return mSevLiveMigrationStatus; > +} > + > /** > Returns the SEV encryption mask. > > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c b/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c > index be260e0d10..62392309fe 100644 > --- a/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c > @@ -136,3 +136,30 @@ MemEncryptSevClearMmioPageEncMask ( > // > return RETURN_UNSUPPORTED; > } > + > +/** > + This hyercall is used to notify hypervisor when the page's encryption > + state changes. > + > + @param[in] PhysicalAddress The physical address that is the start address > + of a memory region. The physical address is > + expected to be PAGE_SIZE aligned. > + @param[in] Pages Number of Pages in the memory region. > + @param[in] Status Encrypted(1) or Decrypted(0). > + > +@retval RETURN_SUCCESS Hypercall returned success. > +**/ > +RETURN_STATUS > +EFIAPI > +SetMemoryEncDecHypercall3 ( > + IN UINTN PhysicalAddress, > + IN UINTN Pages, > + IN UINTN Status > + ) > +{ > + // > + // Memory encryption bit is not accessible in 32-bit mode > + // > + return RETURN_UNSUPPORTED; > +} > + > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiDxeMemEncryptSevLibInternal.c b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiDxeMemEncryptSevLibInternal.c > index b4a9f464e2..0c9f7e17ba 100644 > --- a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiDxeMemEncryptSevLibInternal.c > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiDxeMemEncryptSevLibInternal.c > @@ -61,3 +61,54 @@ MemEncryptSevLocateInitialSmramSaveStateMapPages ( > > return RETURN_SUCCESS; > } > + > +/** > + Figures out if we are running inside KVM HVM and > + KVM HVM supports SEV Live Migration feature. > + > + @retval TRUE SEV live migration is supported. > + @retval FALSE SEV live migration is not supported. > +**/ > +BOOLEAN > +EFIAPI > +KvmDetectSevLiveMigrationFeature( > + VOID > + ) > +{ > + CHAR8 Signature[13]; > + UINT32 mKvmLeaf; > + 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") == 0) { > + DEBUG (( > + DEBUG_INFO, > + "%a: KVM Detected, signature = %s\n", > + __FUNCTION__, > + Signature > + )); > + > + RegEax = mKvmLeaf + 1; > + RegEcx = 0; > + AsmCpuid (mKvmLeaf + 1, &RegEax, &RegEbx, &RegEcx, &RegEdx); > + if ((RegEax & KVM_FEATURE_MIGRATION_CONTROL) != 0) { > + DEBUG (( > + DEBUG_INFO, > + "%a: Live Migration feature supported\n", > + __FUNCTION__ > + )); > + > + return TRUE; > + } > + } > + } > + > + return FALSE; > +} > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf > index 03a78c32df..3233ca7979 100644 > --- a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf > @@ -38,6 +38,7 @@ > X64/PeiDxeVirtualMemory.c > X64/VirtualMemory.c > X64/VirtualMemory.h > + X64/AsmHelperStub.nasm > > [Sources.IA32] > Ia32/MemEncryptSevLib.c > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c > index e2fd109d12..9db6c2ef71 100644 > --- a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c > @@ -20,6 +20,8 @@ > STATIC BOOLEAN mSevStatus = FALSE; > STATIC BOOLEAN mSevEsStatus = FALSE; > STATIC BOOLEAN mSevStatusChecked = FALSE; > +STATIC BOOLEAN mSevLiveMigrationStatus = FALSE; > +STATIC BOOLEAN mSevLiveMigrationStatusChecked = FALSE; > > STATIC UINT64 mSevEncryptionMask = 0; > STATIC BOOLEAN mSevEncryptionMaskSaved = FALSE; > @@ -87,6 +89,24 @@ InternalMemEncryptSevStatus ( > mSevStatusChecked = TRUE; > } > > +/** > + Figures out if we are running inside KVM HVM and > + KVM HVM supports SEV Live Migration feature. > +**/ > +STATIC > +VOID > +EFIAPI > +InternalDetectSevLiveMigrationFeature( > + VOID > + ) > +{ > + if (KvmDetectSevLiveMigrationFeature()) { > + mSevLiveMigrationStatus = TRUE; > + } > + > + mSevLiveMigrationStatusChecked = TRUE; > +} > + > /** > Returns a boolean to indicate whether SEV-ES is enabled. > > @@ -125,6 +145,25 @@ MemEncryptSevIsEnabled ( > return mSevStatus; > } > > +/** > + Returns a boolean to indicate whether SEV live migration is enabled. > + > + @retval TRUE SEV live migration is enabled > + @retval FALSE SEV live migration is not enabled > +**/ > +BOOLEAN > +EFIAPI > +MemEncryptSevLiveMigrationIsEnabled ( > + VOID > + ) > +{ > + if (!mSevLiveMigrationStatusChecked) { > + InternalDetectSevLiveMigrationFeature (); > + } > + > + return mSevLiveMigrationStatus; > +} > + > /** > Returns the SEV encryption mask. > > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLibInternal.c b/OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLibInternal.c > index 56d8f3f318..b926c7b786 100644 > --- a/OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLibInternal.c > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLibInternal.c > @@ -100,6 +100,44 @@ MemEncryptSevIsEnabled ( > return Msr.Bits.SevBit ? TRUE : FALSE; > } > > +/** > + Interface exposed by the ASM implementation of the core hypercall > + > + @retval Hypercall returned status. > +**/ > +UINTN > +EFIAPI > +SetMemoryEncDecHypercall3AsmStub ( > + IN UINTN HypercallNum, > + IN UINTN PhysicalAddress, > + IN UINTN Pages, > + IN UINTN Attributes > + ) > +{ > + // > + // Not used in SEC phase. > + // > + return RETURN_UNSUPPORTED; > +} > + > +/** > + Returns a boolean to indicate whether SEV live migration is enabled. > + > + @retval TRUE SEV live migration is enabled > + @retval FALSE SEV live migration is not enabled > +**/ > +BOOLEAN > +EFIAPI > +MemEncryptSevLiveMigrationIsEnabled ( > + VOID > + ) > +{ > + // > + // Not used in SEC phase. > + // > + return FALSE; > +} > + > /** > Returns the SEV encryption mask. > > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/AsmHelperStub.nasm b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/AsmHelperStub.nasm > new file mode 100644 > index 0000000000..c7c11f77f1 > --- /dev/null > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/AsmHelperStub.nasm > @@ -0,0 +1,33 @@ > +/** @file > + > + ASM helper stub to invoke hypercall > + > + Copyright (c) 2021, AMD Incorporated. All rights reserved.<BR> > + > + SPDX-License-Identifier: BSD-2-Clause-Patent > + > +**/ > + > +DEFAULT REL > +SECTION .text > + > +; UINTN > +; EFIAPI > +; SetMemoryEncDecHypercall3AsmStub ( > +; IN UINTN HypercallNum, > +; IN UINTN Arg1, > +; IN UINTN Arg2, > +; IN UINTN Arg3 > +; ); > +global ASM_PFX(SetMemoryEncDecHypercall3AsmStub) > +ASM_PFX(SetMemoryEncDecHypercall3AsmStub): > + ; UEFI calling conventions require RBX to > + ; be nonvolatile/callee-saved. > + push rbx > + mov rax, rcx ; Copy HypercallNumber to rax > + mov rbx, rdx ; Copy Arg1 to the register expected by KVM > + mov rcx, r8 ; Copy Arg2 to register expected by KVM > + mov rdx, r9 ; Copy Arg2 to register expected by KVM > + vmmcall ; Call VMMCALL > + pop rbx > + ret > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c > index a57e8fd37f..57447e69dc 100644 > --- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c > @@ -143,3 +143,57 @@ MemEncryptSevClearMmioPageEncMask ( > ); > > } > + > +/** > + This hyercall is used to notify hypervisor when the page's encryption > + state changes. > + > + @param[in] PhysicalAddress The physical address that is the start address > + of a memory region. The physical address is > + expected to be PAGE_SIZE aligned. > + @param[in] Pages Number of Pages in the memory region. > + @param[in] Status Encrypted(1) or Decrypted(0). > + > +@retval RETURN_SUCCESS Hypercall returned success. I see RETURN_NO_MAPPING also, so you'll need to update the retvals everywhere. > +**/ > +RETURN_STATUS > +EFIAPI > +SetMemoryEncDecHypercall3 ( > + IN UINTN PhysicalAddress, > + IN UINTN Pages, > + IN UINTN Status > + ) > +{ > + RETURN_STATUS Ret; > + INTN Error; Should be UINTN. > + > + Ret = RETURN_UNSUPPORTED; > + > + if (MemEncryptSevLiveMigrationIsEnabled ()) { > + Ret = EFI_SUCCESS; RETURN_SUCCESS since Ret is type RETURN_STATUS. > + // > + // The encryption bit is set/clear on the smallest page size, hence > + // use the 4k page size in MAP_GPA_RANGE hypercall below. > + // Also, the hypercall expects the guest physical address to be > + // page-aligned. > + // > + Error = SetMemoryEncDecHypercall3AsmStub ( > + KVM_HC_MAP_GPA_RANGE, > + (PhysicalAddress & (~(EFI_PAGE_SIZE-1))), > + Pages, > + KVM_MAP_GPA_RANGE_PAGE_SZ_4K | KVM_MAP_GPA_RANGE_ENC_STAT(Status) Status is UINTN, but is passed from an enum variable. If for any reason that enum should change in the future, this may break. So you should fixup your call to explicitly pass 0 or 1 and then you can safely use that value here. Maybe add an "ASSERT (Status == 0 || Status == 1)" to catch bad input values. > + ); > + > + if (Error != 0) { > + DEBUG ((DEBUG_ERROR, > + "SetMemoryEncDecHypercall3 failed, Phys = %Lx, Pages = %Ld, Err = %Ld\n", > + PhysicalAddress, > + Pages, > + (INT64)Error)); > + > + Ret = RETURN_NO_MAPPING; > + } > + } > + > + return Ret; > +} > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c > index c696745f9d..0b1588a4c1 100644 > --- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c > @@ -536,7 +536,6 @@ EnableReadOnlyPageWriteProtect ( > AsmWriteCr0 (AsmReadCr0() | BIT16); > } > > - > /** > This function either sets or clears memory encryption bit for the memory > region specified by PhysicalAddress and Length from the current page table > @@ -585,6 +584,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 +638,10 @@ SetMemoryEncDec ( > > Status = EFI_SUCCESS; > > + Size = Length; > + CBitChanged = FALSE; > + OrigPhysicalAddress = PhysicalAddress; > + > while (Length != 0) > { > // > @@ -695,6 +701,7 @@ SetMemoryEncDec ( > )); > PhysicalAddress += BIT30; > Length -= BIT30; > + CBitChanged = TRUE; > } else { > // > // We must split the page > @@ -749,6 +756,7 @@ SetMemoryEncDec ( > SetOrClearCBit (&PageDirectory2MEntry->Uint64, Mode); > PhysicalAddress += BIT21; > Length -= BIT21; > + CBitChanged = TRUE; > } else { > // > // We must split up this page into 4K pages > @@ -791,6 +799,7 @@ SetMemoryEncDec ( > SetOrClearCBit (&PageTableEntry->Uint64, Mode); > PhysicalAddress += EFI_PAGE_SIZE; > Length -= EFI_PAGE_SIZE; > + CBitChanged = TRUE; > } > } > } > @@ -808,6 +817,17 @@ SetMemoryEncDec ( > // > CpuFlushTlb(); > > + // > + // Notify Hypervisor on C-bit status > + // > + if (CBitChanged) { > + Status = SetMemoryEncDecHypercall3 ( > + OrigPhysicalAddress, > + EFI_SIZE_TO_PAGES(Size), > + !Mode "Mode" is a MAP_RANGE_MODE enum that is local to this file. So you need to either move this to a common header file so you can use it with SetMemoryEncDecHypercall3() or set a 0 or 1 based on Mode and pass that. Thanks, Tom > + ); > + } > + > Done: > // > // Restore page table write protection, if any. > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v5 1/4] OvmfPkg/BaseMemEncryptLib: Support to issue unencrypted hypercall 2021-07-16 14:11 ` Lendacky, Thomas @ 2021-07-19 20:24 ` Ashish Kalra 0 siblings, 0 replies; 17+ messages in thread From: Ashish Kalra @ 2021-07-19 20:24 UTC (permalink / raw) To: Tom Lendacky Cc: devel, dovmurik, brijesh.singh, tobin, jejb, lersek, jordan.l.justen, ard.biesheuvel, erdemaktas, jiewen.yao, min.m.xu Hello Tom, On Fri, Jul 16, 2021 at 09:11:23AM -0500, Tom Lendacky wrote: > On 7/8/21 9:07 AM, Ashish Kalra wrote: > > From: Ashish Kalra <ashish.kalra@amd.com> > > > > The patch subject is a bit confusing. Something more like "Add API to > issue hypercall on page encryption state change" or similar, since this is > issued for changes to shared and private, not just shared. > > > 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. > > > > This hypercall is used to notify hypervisor when the page's > > encryption state changes. > > This is a large patch. It looks like this should be split into a few patches. > - one patch for the MemEncryptSevLiveMigrationIsEnabled() API > - one patch for the SetMemoryEncDecHypercall3() API > - one patch to make use of the SetMemoryEncDecHypercall3() API. > Ok. > > > > 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/Include/Library/MemEncryptSevLib.h | 69 ++++++++++++++++++++ > > OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf | 1 + > > OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLibInternal.c | 39 +++++++++++ > > OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c | 27 ++++++++ > > OvmfPkg/Library/BaseMemEncryptSevLib/PeiDxeMemEncryptSevLibInternal.c | 51 +++++++++++++++ > > OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf | 1 + > > OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c | 39 +++++++++++ > > OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLibInternal.c | 38 +++++++++++ > > OvmfPkg/Library/BaseMemEncryptSevLib/X64/AsmHelperStub.nasm | 33 ++++++++++ > > OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c | 54 +++++++++++++++ > > OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c | 22 ++++++- > > 11 files changed, 373 insertions(+), 1 deletion(-) > > > > diff --git a/OvmfPkg/Include/Library/MemEncryptSevLib.h b/OvmfPkg/Include/Library/MemEncryptSevLib.h > > index 76d06c206c..c2b2a99a08 100644 > > --- a/OvmfPkg/Include/Library/MemEncryptSevLib.h > > +++ b/OvmfPkg/Include/Library/MemEncryptSevLib.h > > @@ -90,6 +90,18 @@ MemEncryptSevIsEnabled ( > > VOID > > ); > > > > +/** > > + Returns a boolean to indicate whether SEV live migration is enabled. > > + > > + @retval TRUE SEV live migration is enabled > > + @retval FALSE SEV live migration is not enabled > > +**/ > > +BOOLEAN > > +EFIAPI > > +MemEncryptSevLiveMigrationIsEnabled ( > > + VOID > > + ); > > + > > /** > > This function clears memory encryption bit for the memory region specified by > > BaseAddress and NumPages from the current page table context. > > @@ -222,4 +234,61 @@ MemEncryptSevClearMmioPageEncMask ( > > IN UINTN NumPages > > ); > > > > +/** > > + This hypercall is used to notify hypervisor when the page's encryption > > + state changes. > > + > > + @param[in] PhysicalAddress The physical address that is the start address > > + of a memory region. The PhysicalAddress is > > + expected to be PAGE_SIZE aligned. > > + @param[in] Pages Number of pages in memory region. > > + @param[in] Status Encrypted(1) or Decrypted(0). > > + > > +@retval RETURN_SUCCESS Hypercall returned success. > > It looks like RETURN_UNSUPPORTED is also possible. > > > +**/ > > +RETURN_STATUS > > +EFIAPI > > +SetMemoryEncDecHypercall3 ( > > + IN UINTN PhysicalAddress, > > + IN UINTN Pages, > > + IN UINTN Status > > + ); > > + > > +#define KVM_HC_MAP_GPA_RANGE 12 > > +#define KVM_MAP_GPA_RANGE_PAGE_SZ_4K 0 > > +#define KVM_MAP_GPA_RANGE_PAGE_SZ_2M BIT0 > > +#define KVM_MAP_GPA_RANGE_PAGE_SZ_1G BIT1 > > +#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) > > You define these but don't use them (and you should). > Used later in another patch. > > + > > +#define KVM_FEATURE_MIGRATION_CONTROL BIT17 > > + > > +/** > > + Figures out if we are running inside KVM HVM and > > + KVM HVM supports SEV Live Migration feature. > > + > > + @retval TRUE SEV live migration is supported. > > + @retval FALSE SEV live migration is not supported. > > +**/ > > +BOOLEAN > > +EFIAPI > > +KvmDetectSevLiveMigrationFeature( > > + VOID > > + ); > > + > > +/** > > + Interface exposed by the ASM implementation of the core hypercall > > + > > + @retval Hypercall returned status. > > +**/ > > +UINTN > > +EFIAPI > > +SetMemoryEncDecHypercall3AsmStub ( > > + IN UINTN HypercallNum, > > + IN UINTN PhysicalAddress, > > + IN UINTN Pages, > > + IN UINTN Attributes > > + ); > > + > > #endif // _MEM_ENCRYPT_SEV_LIB_H_ > > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf b/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf > > index f2e162d680..0c28afadee 100644 > > --- a/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf > > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf > > @@ -38,6 +38,7 @@ > > X64/PeiDxeVirtualMemory.c > > X64/VirtualMemory.c > > X64/VirtualMemory.h > > + X64/AsmHelperStub.nasm > > > > [Sources.IA32] > > Ia32/MemEncryptSevLib.c > > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLibInternal.c b/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLibInternal.c > > index 2816f859a0..ead754cd7b 100644 > > --- a/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLibInternal.c > > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLibInternal.c > > @@ -20,6 +20,8 @@ > > STATIC BOOLEAN mSevStatus = FALSE; > > STATIC BOOLEAN mSevEsStatus = FALSE; > > STATIC BOOLEAN mSevStatusChecked = FALSE; > > +STATIC BOOLEAN mSevLiveMigrationStatus = FALSE; > > +STATIC BOOLEAN mSevLiveMigrationStatusChecked = FALSE; > > > > STATIC UINT64 mSevEncryptionMask = 0; > > STATIC BOOLEAN mSevEncryptionMaskSaved = FALSE; > > @@ -87,6 +89,24 @@ InternalMemEncryptSevStatus ( > > mSevStatusChecked = TRUE; > > } > > > > +/** > > + Figures out if we are running inside KVM HVM and > > + KVM HVM supports SEV Live Migration feature. > > +**/ > > +STATIC > > +VOID > > +EFIAPI > > +InternalDetectSevLiveMigrationFeature( > > + VOID > > + ) > > +{ > > + if (KvmDetectSevLiveMigrationFeature()) { > > + mSevLiveMigrationStatus = TRUE; > > + } > > + > > + mSevLiveMigrationStatusChecked = TRUE; > > +} > > + > > /** > > Returns a boolean to indicate whether SEV-ES is enabled. > > > > @@ -125,6 +145,25 @@ MemEncryptSevIsEnabled ( > > return mSevStatus; > > } > > > > +/** > > + Returns a boolean to indicate whether SEV live migration is enabled. > > + > > + @retval TRUE SEV live migration is enabled > > + @retval FALSE SEV live migration is not enabled > > +**/ > > +BOOLEAN > > +EFIAPI > > +MemEncryptSevLiveMigrationIsEnabled ( > > + VOID > > + ) > > +{ > > + if (!mSevLiveMigrationStatusChecked) { > > + InternalDetectSevLiveMigrationFeature (); > > + } > > + > > + return mSevLiveMigrationStatus; > > +} > > + > > /** > > Returns the SEV encryption mask. > > > > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c b/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c > > index be260e0d10..62392309fe 100644 > > --- a/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c > > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c > > @@ -136,3 +136,30 @@ MemEncryptSevClearMmioPageEncMask ( > > // > > return RETURN_UNSUPPORTED; > > } > > + > > +/** > > + This hyercall is used to notify hypervisor when the page's encryption > > + state changes. > > + > > + @param[in] PhysicalAddress The physical address that is the start address > > + of a memory region. The physical address is > > + expected to be PAGE_SIZE aligned. > > + @param[in] Pages Number of Pages in the memory region. > > + @param[in] Status Encrypted(1) or Decrypted(0). > > + > > +@retval RETURN_SUCCESS Hypercall returned success. > > +**/ > > +RETURN_STATUS > > +EFIAPI > > +SetMemoryEncDecHypercall3 ( > > + IN UINTN PhysicalAddress, > > + IN UINTN Pages, > > + IN UINTN Status > > + ) > > +{ > > + // > > + // Memory encryption bit is not accessible in 32-bit mode > > + // > > + return RETURN_UNSUPPORTED; > > +} > > + > > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiDxeMemEncryptSevLibInternal.c b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiDxeMemEncryptSevLibInternal.c > > index b4a9f464e2..0c9f7e17ba 100644 > > --- a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiDxeMemEncryptSevLibInternal.c > > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiDxeMemEncryptSevLibInternal.c > > @@ -61,3 +61,54 @@ MemEncryptSevLocateInitialSmramSaveStateMapPages ( > > > > return RETURN_SUCCESS; > > } > > + > > +/** > > + Figures out if we are running inside KVM HVM and > > + KVM HVM supports SEV Live Migration feature. > > + > > + @retval TRUE SEV live migration is supported. > > + @retval FALSE SEV live migration is not supported. > > +**/ > > +BOOLEAN > > +EFIAPI > > +KvmDetectSevLiveMigrationFeature( > > + VOID > > + ) > > +{ > > + CHAR8 Signature[13]; > > + UINT32 mKvmLeaf; > > + 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") == 0) { > > + DEBUG (( > > + DEBUG_INFO, > > + "%a: KVM Detected, signature = %s\n", > > + __FUNCTION__, > > + Signature > > + )); > > + > > + RegEax = mKvmLeaf + 1; > > + RegEcx = 0; > > + AsmCpuid (mKvmLeaf + 1, &RegEax, &RegEbx, &RegEcx, &RegEdx); > > + if ((RegEax & KVM_FEATURE_MIGRATION_CONTROL) != 0) { > > + DEBUG (( > > + DEBUG_INFO, > > + "%a: Live Migration feature supported\n", > > + __FUNCTION__ > > + )); > > + > > + return TRUE; > > + } > > + } > > + } > > + > > + return FALSE; > > +} > > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf > > index 03a78c32df..3233ca7979 100644 > > --- a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf > > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf > > @@ -38,6 +38,7 @@ > > X64/PeiDxeVirtualMemory.c > > X64/VirtualMemory.c > > X64/VirtualMemory.h > > + X64/AsmHelperStub.nasm > > > > [Sources.IA32] > > Ia32/MemEncryptSevLib.c > > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c > > index e2fd109d12..9db6c2ef71 100644 > > --- a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c > > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c > > @@ -20,6 +20,8 @@ > > STATIC BOOLEAN mSevStatus = FALSE; > > STATIC BOOLEAN mSevEsStatus = FALSE; > > STATIC BOOLEAN mSevStatusChecked = FALSE; > > +STATIC BOOLEAN mSevLiveMigrationStatus = FALSE; > > +STATIC BOOLEAN mSevLiveMigrationStatusChecked = FALSE; > > > > STATIC UINT64 mSevEncryptionMask = 0; > > STATIC BOOLEAN mSevEncryptionMaskSaved = FALSE; > > @@ -87,6 +89,24 @@ InternalMemEncryptSevStatus ( > > mSevStatusChecked = TRUE; > > } > > > > +/** > > + Figures out if we are running inside KVM HVM and > > + KVM HVM supports SEV Live Migration feature. > > +**/ > > +STATIC > > +VOID > > +EFIAPI > > +InternalDetectSevLiveMigrationFeature( > > + VOID > > + ) > > +{ > > + if (KvmDetectSevLiveMigrationFeature()) { > > + mSevLiveMigrationStatus = TRUE; > > + } > > + > > + mSevLiveMigrationStatusChecked = TRUE; > > +} > > + > > /** > > Returns a boolean to indicate whether SEV-ES is enabled. > > > > @@ -125,6 +145,25 @@ MemEncryptSevIsEnabled ( > > return mSevStatus; > > } > > > > +/** > > + Returns a boolean to indicate whether SEV live migration is enabled. > > + > > + @retval TRUE SEV live migration is enabled > > + @retval FALSE SEV live migration is not enabled > > +**/ > > +BOOLEAN > > +EFIAPI > > +MemEncryptSevLiveMigrationIsEnabled ( > > + VOID > > + ) > > +{ > > + if (!mSevLiveMigrationStatusChecked) { > > + InternalDetectSevLiveMigrationFeature (); > > + } > > + > > + return mSevLiveMigrationStatus; > > +} > > + > > /** > > Returns the SEV encryption mask. > > > > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLibInternal.c b/OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLibInternal.c > > index 56d8f3f318..b926c7b786 100644 > > --- a/OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLibInternal.c > > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLibInternal.c > > @@ -100,6 +100,44 @@ MemEncryptSevIsEnabled ( > > return Msr.Bits.SevBit ? TRUE : FALSE; > > } > > > > +/** > > + Interface exposed by the ASM implementation of the core hypercall > > + > > + @retval Hypercall returned status. > > +**/ > > +UINTN > > +EFIAPI > > +SetMemoryEncDecHypercall3AsmStub ( > > + IN UINTN HypercallNum, > > + IN UINTN PhysicalAddress, > > + IN UINTN Pages, > > + IN UINTN Attributes > > + ) > > +{ > > + // > > + // Not used in SEC phase. > > + // > > + return RETURN_UNSUPPORTED; > > +} > > + > > +/** > > + Returns a boolean to indicate whether SEV live migration is enabled. > > + > > + @retval TRUE SEV live migration is enabled > > + @retval FALSE SEV live migration is not enabled > > +**/ > > +BOOLEAN > > +EFIAPI > > +MemEncryptSevLiveMigrationIsEnabled ( > > + VOID > > + ) > > +{ > > + // > > + // Not used in SEC phase. > > + // > > + return FALSE; > > +} > > + > > /** > > Returns the SEV encryption mask. > > > > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/AsmHelperStub.nasm b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/AsmHelperStub.nasm > > new file mode 100644 > > index 0000000000..c7c11f77f1 > > --- /dev/null > > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/AsmHelperStub.nasm > > @@ -0,0 +1,33 @@ > > +/** @file > > + > > + ASM helper stub to invoke hypercall > > + > > + Copyright (c) 2021, AMD Incorporated. All rights reserved.<BR> > > + > > + SPDX-License-Identifier: BSD-2-Clause-Patent > > + > > +**/ > > + > > +DEFAULT REL > > +SECTION .text > > + > > +; UINTN > > +; EFIAPI > > +; SetMemoryEncDecHypercall3AsmStub ( > > +; IN UINTN HypercallNum, > > +; IN UINTN Arg1, > > +; IN UINTN Arg2, > > +; IN UINTN Arg3 > > +; ); > > +global ASM_PFX(SetMemoryEncDecHypercall3AsmStub) > > +ASM_PFX(SetMemoryEncDecHypercall3AsmStub): > > + ; UEFI calling conventions require RBX to > > + ; be nonvolatile/callee-saved. > > + push rbx > > + mov rax, rcx ; Copy HypercallNumber to rax > > + mov rbx, rdx ; Copy Arg1 to the register expected by KVM > > + mov rcx, r8 ; Copy Arg2 to register expected by KVM > > + mov rdx, r9 ; Copy Arg2 to register expected by KVM > > + vmmcall ; Call VMMCALL > > + pop rbx > > + ret > > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c > > index a57e8fd37f..57447e69dc 100644 > > --- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c > > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c > > @@ -143,3 +143,57 @@ MemEncryptSevClearMmioPageEncMask ( > > ); > > > > } > > + > > +/** > > + This hyercall is used to notify hypervisor when the page's encryption > > + state changes. > > + > > + @param[in] PhysicalAddress The physical address that is the start address > > + of a memory region. The physical address is > > + expected to be PAGE_SIZE aligned. > > + @param[in] Pages Number of Pages in the memory region. > > + @param[in] Status Encrypted(1) or Decrypted(0). > > + > > +@retval RETURN_SUCCESS Hypercall returned success. > > I see RETURN_NO_MAPPING also, so you'll need to update the retvals everywhere. > Yes. > > +**/ > > +RETURN_STATUS > > +EFIAPI > > +SetMemoryEncDecHypercall3 ( > > + IN UINTN PhysicalAddress, > > + IN UINTN Pages, > > + IN UINTN Status > > + ) > > +{ > > + RETURN_STATUS Ret; > > + INTN Error; > > Should be UINTN. > > > + > > + Ret = RETURN_UNSUPPORTED; > > + > > + if (MemEncryptSevLiveMigrationIsEnabled ()) { > > + Ret = EFI_SUCCESS; > > RETURN_SUCCESS since Ret is type RETURN_STATUS. Ok. > > + // > > + // The encryption bit is set/clear on the smallest page size, hence > > + // use the 4k page size in MAP_GPA_RANGE hypercall below. > > + // Also, the hypercall expects the guest physical address to be > > + // page-aligned. > > + // > > + Error = SetMemoryEncDecHypercall3AsmStub ( > > + KVM_HC_MAP_GPA_RANGE, > > + (PhysicalAddress & (~(EFI_PAGE_SIZE-1))), > > + Pages, > > + KVM_MAP_GPA_RANGE_PAGE_SZ_4K | KVM_MAP_GPA_RANGE_ENC_STAT(Status) > > Status is UINTN, but is passed from an enum variable. If for any reason > that enum should change in the future, this may break. So you should fixup > your call to explicitly pass 0 or 1 and then you can safely use that value > here. > > Maybe add an "ASSERT (Status == 0 || Status == 1)" to catch bad input values. > Ok. > > + ); > > + > > + if (Error != 0) { > > + DEBUG ((DEBUG_ERROR, > > + "SetMemoryEncDecHypercall3 failed, Phys = %Lx, Pages = %Ld, Err = %Ld\n", > > + PhysicalAddress, > > + Pages, > > + (INT64)Error)); > > + > > + Ret = RETURN_NO_MAPPING; > > + } > > + } > > + > > + return Ret; > > +} > > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c > > index c696745f9d..0b1588a4c1 100644 > > --- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c > > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c > > @@ -536,7 +536,6 @@ EnableReadOnlyPageWriteProtect ( > > AsmWriteCr0 (AsmReadCr0() | BIT16); > > } > > > > - > > /** > > This function either sets or clears memory encryption bit for the memory > > region specified by PhysicalAddress and Length from the current page table > > @@ -585,6 +584,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 +638,10 @@ SetMemoryEncDec ( > > > > Status = EFI_SUCCESS; > > > > + Size = Length; > > + CBitChanged = FALSE; > > + OrigPhysicalAddress = PhysicalAddress; > > + > > while (Length != 0) > > { > > // > > @@ -695,6 +701,7 @@ SetMemoryEncDec ( > > )); > > PhysicalAddress += BIT30; > > Length -= BIT30; > > + CBitChanged = TRUE; > > } else { > > // > > // We must split the page > > @@ -749,6 +756,7 @@ SetMemoryEncDec ( > > SetOrClearCBit (&PageDirectory2MEntry->Uint64, Mode); > > PhysicalAddress += BIT21; > > Length -= BIT21; > > + CBitChanged = TRUE; > > } else { > > // > > // We must split up this page into 4K pages > > @@ -791,6 +799,7 @@ SetMemoryEncDec ( > > SetOrClearCBit (&PageTableEntry->Uint64, Mode); > > PhysicalAddress += EFI_PAGE_SIZE; > > Length -= EFI_PAGE_SIZE; > > + CBitChanged = TRUE; > > } > > } > > } > > @@ -808,6 +817,17 @@ SetMemoryEncDec ( > > // > > CpuFlushTlb(); > > > > + // > > + // Notify Hypervisor on C-bit status > > + // > > + if (CBitChanged) { > > + Status = SetMemoryEncDecHypercall3 ( > > + OrigPhysicalAddress, > > + EFI_SIZE_TO_PAGES(Size), > > + !Mode > > "Mode" is a MAP_RANGE_MODE enum that is local to this file. So you need to > either move this to a common header file so you can use it with > SetMemoryEncDecHypercall3() or set a 0 or 1 based on Mode and pass that. > > Ok. Thanks, Ashish > > + ); > > + } > > + > > Done: > > // > > // Restore page table write protection, if any. > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v5 2/4] OvmfPkg/VmgExitLib: Add support for hypercalls with SEV-ES. 2021-07-08 14:07 [PATCH v5 0/4] SEV Live Migration support for OVMF Ashish Kalra 2021-07-08 14:07 ` [PATCH v5 1/4] OvmfPkg/BaseMemEncryptLib: Support to issue unencrypted hypercall Ashish Kalra @ 2021-07-08 14:08 ` Ashish Kalra 2021-07-16 14:16 ` Lendacky, Thomas 2021-07-08 14:08 ` [PATCH v5 3/4] OvmfPkg/PlatformPei: Mark SEC GHCB page as unencrypted via hypercall Ashish Kalra 2021-07-08 14:09 ` [PATCH v5 4/4] OvmfPkg/AmdSevDxe: Add support for SEV live migration Ashish Kalra 3 siblings, 1 reply; 17+ messages in thread From: Ashish Kalra @ 2021-07-08 14:08 UTC (permalink / raw) To: devel Cc: dovmurik, brijesh.singh, tobin, Thomas.Lendacky, jejb, lersek, jordan.l.justen, ard.biesheuvel, erdemaktas, jiewen.yao, min.m.xu From: Ashish Kalra <ashish.kalra@amd.com> Make the VC handler hypercall aware by adding support to compare the hypercall number and add the additional register values used by hypercall in the GHCB. Also mark the SEC GHCB page (that is mapped as unencrypted in ResetVector code) in the hypervisor guest 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/Library/VmgExitLib/VmgExitVcHandler.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c index 41b0c8cc53..7f69bfab5f 100644 --- a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c +++ b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c @@ -1171,6 +1171,15 @@ VmmCallExit ( Ghcb->SaveArea.Cpl = (UINT8) (Regs->Cs & 0x3); VmgSetOffsetValid (Ghcb, GhcbCpl); + if (Regs->Rax == KVM_HC_MAP_GPA_RANGE) { + Ghcb->SaveArea.Rbx = Regs->Rbx; + VmgSetOffsetValid (Ghcb, GhcbRbx); + Ghcb->SaveArea.Rcx = Regs->Rcx; + VmgSetOffsetValid (Ghcb, GhcbRcx); + Ghcb->SaveArea.Rdx = Regs->Rdx; + VmgSetOffsetValid (Ghcb, GhcbRdx); + } + Status = VmgExit (Ghcb, SVM_EXIT_VMMCALL, 0, 0); if (Status != 0) { return Status; -- 2.17.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v5 2/4] OvmfPkg/VmgExitLib: Add support for hypercalls with SEV-ES. 2021-07-08 14:08 ` [PATCH v5 2/4] OvmfPkg/VmgExitLib: Add support for hypercalls with SEV-ES Ashish Kalra @ 2021-07-16 14:16 ` Lendacky, Thomas 2021-07-19 20:25 ` Ashish Kalra 0 siblings, 1 reply; 17+ messages in thread From: Lendacky, Thomas @ 2021-07-16 14:16 UTC (permalink / raw) To: Ashish Kalra, devel Cc: dovmurik, brijesh.singh, tobin, jejb, lersek, jordan.l.justen, ard.biesheuvel, erdemaktas, jiewen.yao, min.m.xu On 7/8/21 9:08 AM, Ashish Kalra wrote: > From: Ashish Kalra <ashish.kalra@amd.com> > The subject isn't correct since the #VC handler already supports hypercalls. It should say something like "Make the #VC handler aware of the encryption state change hypercall" or "Update the #VC handler to support the encryption state change hypercall" or something like that. > Make the VC handler hypercall aware by adding support > to compare the hypercall number and add the additional > register values used by hypercall in the GHCB. > > Also mark the SEC GHCB page (that is mapped as > unencrypted in ResetVector code) in the hypervisor > guest page status tracking. This part of the commit message shoudn't be here any more. > > 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/Library/VmgExitLib/VmgExitVcHandler.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c > index 41b0c8cc53..7f69bfab5f 100644 > --- a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c > +++ b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c > @@ -1171,6 +1171,15 @@ VmmCallExit ( > Ghcb->SaveArea.Cpl = (UINT8) (Regs->Cs & 0x3); > VmgSetOffsetValid (Ghcb, GhcbCpl); > Add a comment that this hypercall requires these extra registers so you are explicitly adding them. Thanks, Tom > + if (Regs->Rax == KVM_HC_MAP_GPA_RANGE) { > + Ghcb->SaveArea.Rbx = Regs->Rbx; > + VmgSetOffsetValid (Ghcb, GhcbRbx); > + Ghcb->SaveArea.Rcx = Regs->Rcx; > + VmgSetOffsetValid (Ghcb, GhcbRcx); > + Ghcb->SaveArea.Rdx = Regs->Rdx; > + VmgSetOffsetValid (Ghcb, GhcbRdx); > + } > + > Status = VmgExit (Ghcb, SVM_EXIT_VMMCALL, 0, 0); > if (Status != 0) { > return Status; > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v5 2/4] OvmfPkg/VmgExitLib: Add support for hypercalls with SEV-ES. 2021-07-16 14:16 ` Lendacky, Thomas @ 2021-07-19 20:25 ` Ashish Kalra 0 siblings, 0 replies; 17+ messages in thread From: Ashish Kalra @ 2021-07-19 20:25 UTC (permalink / raw) To: Tom Lendacky Cc: devel, dovmurik, brijesh.singh, tobin, jejb, lersek, jordan.l.justen, ard.biesheuvel, erdemaktas, jiewen.yao, min.m.xu Hello Tom, On Fri, Jul 16, 2021 at 09:16:00AM -0500, Tom Lendacky wrote: > On 7/8/21 9:08 AM, Ashish Kalra wrote: > > From: Ashish Kalra <ashish.kalra@amd.com> > > > > The subject isn't correct since the #VC handler already supports > hypercalls. It should say something like "Make the #VC handler aware of > the encryption state change hypercall" or "Update the #VC handler to > support the encryption state change hypercall" or something like that. > Ok. > > Make the VC handler hypercall aware by adding support > > to compare the hypercall number and add the additional > > register values used by hypercall in the GHCB. > > > > Also mark the SEC GHCB page (that is mapped as > > unencrypted in ResetVector code) in the hypervisor > > guest page status tracking. > > This part of the commit message shoudn't be here any more. > Yes. > > > > 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/Library/VmgExitLib/VmgExitVcHandler.c | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c > > index 41b0c8cc53..7f69bfab5f 100644 > > --- a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c > > +++ b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c > > @@ -1171,6 +1171,15 @@ VmmCallExit ( > > Ghcb->SaveArea.Cpl = (UINT8) (Regs->Cs & 0x3); > > VmgSetOffsetValid (Ghcb, GhcbCpl); > > > > Add a comment that this hypercall requires these extra registers so you > are explicitly adding them. > Ok. Thanks, Ashish > > + if (Regs->Rax == KVM_HC_MAP_GPA_RANGE) { > > + Ghcb->SaveArea.Rbx = Regs->Rbx; > > + VmgSetOffsetValid (Ghcb, GhcbRbx); > > + Ghcb->SaveArea.Rcx = Regs->Rcx; > > + VmgSetOffsetValid (Ghcb, GhcbRcx); > > + Ghcb->SaveArea.Rdx = Regs->Rdx; > > + VmgSetOffsetValid (Ghcb, GhcbRdx); > > + } > > + > > Status = VmgExit (Ghcb, SVM_EXIT_VMMCALL, 0, 0); > > if (Status != 0) { > > return Status; > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v5 3/4] OvmfPkg/PlatformPei: Mark SEC GHCB page as unencrypted via hypercall 2021-07-08 14:07 [PATCH v5 0/4] SEV Live Migration support for OVMF Ashish Kalra 2021-07-08 14:07 ` [PATCH v5 1/4] OvmfPkg/BaseMemEncryptLib: Support to issue unencrypted hypercall Ashish Kalra 2021-07-08 14:08 ` [PATCH v5 2/4] OvmfPkg/VmgExitLib: Add support for hypercalls with SEV-ES Ashish Kalra @ 2021-07-08 14:08 ` Ashish Kalra 2021-07-16 14:22 ` Lendacky, Thomas 2021-07-08 14:09 ` [PATCH v5 4/4] OvmfPkg/AmdSevDxe: Add support for SEV live migration Ashish Kalra 3 siblings, 1 reply; 17+ messages in thread From: Ashish Kalra @ 2021-07-08 14:08 UTC (permalink / raw) To: devel Cc: dovmurik, brijesh.singh, tobin, Thomas.Lendacky, jejb, lersek, jordan.l.justen, ard.biesheuvel, erdemaktas, jiewen.yao, min.m.xu 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 | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c index a8bf610022..1ec0de48fe 100644 --- a/OvmfPkg/PlatformPei/AmdSev.c +++ b/OvmfPkg/PlatformPei/AmdSev.c @@ -52,6 +52,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] 17+ messages in thread
* Re: [PATCH v5 3/4] OvmfPkg/PlatformPei: Mark SEC GHCB page as unencrypted via hypercall 2021-07-08 14:08 ` [PATCH v5 3/4] OvmfPkg/PlatformPei: Mark SEC GHCB page as unencrypted via hypercall Ashish Kalra @ 2021-07-16 14:22 ` Lendacky, Thomas 2021-07-19 20:27 ` Ashish Kalra 0 siblings, 1 reply; 17+ messages in thread From: Lendacky, Thomas @ 2021-07-16 14:22 UTC (permalink / raw) To: Ashish Kalra, devel Cc: dovmurik, brijesh.singh, tobin, jejb, lersek, jordan.l.justen, ard.biesheuvel, erdemaktas, jiewen.yao, min.m.xu On 7/8/21 9:08 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> > Signed-off-by: Ashish Kalra <ashish.kalra@amd.com> > --- > OvmfPkg/PlatformPei/AmdSev.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c > index a8bf610022..1ec0de48fe 100644 > --- a/OvmfPkg/PlatformPei/AmdSev.c > +++ b/OvmfPkg/PlatformPei/AmdSev.c > @@ -52,6 +52,15 @@ AmdSevEsInitialize ( > PcdStatus = PcdSetBoolS (PcdSevEsIsEnabled, TRUE); > ASSERT_RETURN_ERROR (PcdStatus); > > + // > + // GHCB_BASE setup during reset-vector needs to be marked as s/GHCB_BASE/The SEC Ghcb/ > + // decrypted in the hypervisor page encryption bitmap. Is the "hypervisor page encryption bitmap" valid anymore? This gets passed up to userspace now, right? You should go through all the patches to be sure you aren't talking about a bitmap anymore and just state that you're updating the encryption state with the hypervisor. > + // > + SetMemoryEncDecHypercall3 (FixedPcdGet32 (PcdOvmfSecGhcbBase), The first argument needs to be moved down to a line of its own and indented like the following arguments. > + EFI_SIZE_TO_PAGES(FixedPcdGet32 (PcdOvmfSecGhcbSize)), > + KVM_MAP_GPA_RANGE_DECRYPTED Ah, now I see this #define used, but you should be passing a 0 or 1, right? This happens to evaluate to 0, but it's the wrong way to call this function. Thanks, Tom > + ); > + > // > // Allocate GHCB and per-CPU variable pages. > // Since the pages must survive across the UEFI to OS transition > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v5 3/4] OvmfPkg/PlatformPei: Mark SEC GHCB page as unencrypted via hypercall 2021-07-16 14:22 ` Lendacky, Thomas @ 2021-07-19 20:27 ` Ashish Kalra 0 siblings, 0 replies; 17+ messages in thread From: Ashish Kalra @ 2021-07-19 20:27 UTC (permalink / raw) To: Tom Lendacky Cc: devel, dovmurik, brijesh.singh, tobin, jejb, lersek, jordan.l.justen, ard.biesheuvel, erdemaktas, jiewen.yao, min.m.xu Hello Tom, On Fri, Jul 16, 2021 at 09:22:20AM -0500, Tom Lendacky wrote: > On 7/8/21 9:08 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> > > Signed-off-by: Ashish Kalra <ashish.kalra@amd.com> > > --- > > OvmfPkg/PlatformPei/AmdSev.c | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c > > index a8bf610022..1ec0de48fe 100644 > > --- a/OvmfPkg/PlatformPei/AmdSev.c > > +++ b/OvmfPkg/PlatformPei/AmdSev.c > > @@ -52,6 +52,15 @@ AmdSevEsInitialize ( > > PcdStatus = PcdSetBoolS (PcdSevEsIsEnabled, TRUE); > > ASSERT_RETURN_ERROR (PcdStatus); > > > > + // > > + // GHCB_BASE setup during reset-vector needs to be marked as > > s/GHCB_BASE/The SEC Ghcb/ > > > + // decrypted in the hypervisor page encryption bitmap. > > Is the "hypervisor page encryption bitmap" valid anymore? This gets passed > up to userspace now, right? > > You should go through all the patches to be sure you aren't talking about > a bitmap anymore and just state that you're updating the encryption state > with the hypervisor. > Ok, i will fix that. > > + // > > + SetMemoryEncDecHypercall3 (FixedPcdGet32 (PcdOvmfSecGhcbBase), > > The first argument needs to be moved down to a line of its own and > indented like the following arguments. > > > + EFI_SIZE_TO_PAGES(FixedPcdGet32 (PcdOvmfSecGhcbSize)), > > + KVM_MAP_GPA_RANGE_DECRYPTED > > Ah, now I see this #define used, but you should be passing a 0 or 1, > right? This happens to evaluate to 0, but it's the wrong way to call this > function. > Ok. Thanks, Ashish > > + ); > > + > > // > > // Allocate GHCB and per-CPU variable pages. > > // Since the pages must survive across the UEFI to OS transition > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v5 4/4] OvmfPkg/AmdSevDxe: Add support for SEV live migration. 2021-07-08 14:07 [PATCH v5 0/4] SEV Live Migration support for OVMF Ashish Kalra ` (2 preceding siblings ...) 2021-07-08 14:08 ` [PATCH v5 3/4] OvmfPkg/PlatformPei: Mark SEC GHCB page as unencrypted via hypercall Ashish Kalra @ 2021-07-08 14:09 ` Ashish Kalra 2021-07-19 7:31 ` Dov Murik 3 siblings, 1 reply; 17+ messages in thread From: Ashish Kalra @ 2021-07-08 14:09 UTC (permalink / raw) To: devel Cc: dovmurik, brijesh.singh, tobin, Thomas.Lendacky, jejb, lersek, jordan.l.justen, ard.biesheuvel, erdemaktas, jiewen.yao, min.m.xu From: Ashish Kalra <ashish.kalra@amd.com> Check for SEV live migration feature support, if detected setup a new UEFI enviroment variable to indicate OVMF support for SEV live migration. The new runtime UEFI environment variable is set via the notification function registered for the EFI_END_OF_DXE_EVENT_GROUP_GUID event in AmdSevDxe driver. Signed-off-by: Ashish Kalra <ashish.kalra@amd.com> --- OvmfPkg/AmdSevDxe/AmdSevDxe.c | 59 ++++++++++++++++++++ OvmfPkg/AmdSevDxe/AmdSevDxe.inf | 4 ++ OvmfPkg/Include/Guid/MemEncryptLib.h | 20 +++++++ OvmfPkg/OvmfPkg.dec | 1 + 4 files changed, 84 insertions(+) diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.c b/OvmfPkg/AmdSevDxe/AmdSevDxe.c index c66c4e9b92..45adf3249c 100644 --- a/OvmfPkg/AmdSevDxe/AmdSevDxe.c +++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.c @@ -15,10 +15,49 @@ #include <Library/BaseMemoryLib.h> #include <Library/DebugLib.h> #include <Library/DxeServicesTableLib.h> +#include <Library/UefiRuntimeServicesTableLib.h> +#include <Library/UefiBootServicesTableLib.h> #include <Library/MemEncryptSevLib.h> #include <Library/MemoryAllocationLib.h> +#include <Guid/MemEncryptLib.h> +#include <Guid/EventGroup.h> #include <Library/PcdLib.h> +STATIC +VOID +EFIAPI +AmdSevDxeOnEndOfDxe ( + IN EFI_EVENT Event, + IN VOID *EventToSignal + ) +{ + EFI_STATUS Status; + BOOLEAN SevLiveMigrationEnabled; + + SevLiveMigrationEnabled = MemEncryptSevLiveMigrationIsEnabled(); + + 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_INFO, + "%a: Setting SevLiveMigrationEnabled variable, status = %lx\n", + __FUNCTION__, + Status + )); + } + + DEBUG ((DEBUG_VERBOSE, "%a\n", __FUNCTION__)); +} + EFI_STATUS EFIAPI AmdSevDxeEntryPoint ( @@ -30,6 +69,7 @@ AmdSevDxeEntryPoint ( EFI_GCD_MEMORY_SPACE_DESCRIPTOR *AllDescMap; UINTN NumEntries; UINTN Index; + EFI_EVENT Event; // // Do nothing when SEV is not enabled @@ -130,5 +170,24 @@ AmdSevDxeEntryPoint ( } } + // + // Register EFI_END_OF_DXE_EVENT_GROUP_GUID event. + // The notification function sets the runtime variable indicating OVMF + // support for SEV live migration. + // + Status = gBS->CreateEventEx ( + EVT_NOTIFY_SIGNAL, + TPL_CALLBACK, + AmdSevDxeOnEndOfDxe, + NULL, + &gEfiEndOfDxeEventGroupGuid, + &Event + ); + + if (EFI_ERROR (Status)) { + DEBUG ((DEBUG_INFO, "%a: CreateEventEx(): %r\n", + __FUNCTION__, Status)); + } + return EFI_SUCCESS; } diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.inf b/OvmfPkg/AmdSevDxe/AmdSevDxe.inf index 0676fcc5b6..f4e40ff412 100644 --- a/OvmfPkg/AmdSevDxe/AmdSevDxe.inf +++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.inf @@ -45,3 +45,7 @@ [Pcd] gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId + +[Guids] + gMemEncryptGuid + gEfiEndOfDxeEventGroupGuid ## CONSUMES ## Event 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 -- 2.17.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v5 4/4] OvmfPkg/AmdSevDxe: Add support for SEV live migration. 2021-07-08 14:09 ` [PATCH v5 4/4] OvmfPkg/AmdSevDxe: Add support for SEV live migration Ashish Kalra @ 2021-07-19 7:31 ` Dov Murik 2021-07-19 15:22 ` Ashish Kalra 0 siblings, 1 reply; 17+ messages in thread From: Dov Murik @ 2021-07-19 7:31 UTC (permalink / raw) To: Ashish Kalra, devel Cc: dovmurik, brijesh.singh, tobin, Thomas.Lendacky, jejb, lersek, jordan.l.justen, ard.biesheuvel, erdemaktas, jiewen.yao, min.m.xu Ashish, On 08/07/2021 17:09, Ashish Kalra wrote: > From: Ashish Kalra <ashish.kalra@amd.com> > > Check for SEV live migration feature support, if detected > setup a new UEFI enviroment variable to indicate OVMF > support for SEV live migration. > > The new runtime UEFI environment variable is set via the > notification function registered for the > EFI_END_OF_DXE_EVENT_GROUP_GUID event in AmdSevDxe driver. > Why is this indirect notification needed? Why not simply call gRT->SetVariable in AmdSevDxeEntryPoint (instead of calling CreateEventEx)? If this is needed, please add a clarification (in the commit message and before the CreateEventEx call). > Signed-off-by: Ashish Kalra <ashish.kalra@amd.com> > --- > OvmfPkg/AmdSevDxe/AmdSevDxe.c | 59 ++++++++++++++++++++ > OvmfPkg/AmdSevDxe/AmdSevDxe.inf | 4 ++ > OvmfPkg/Include/Guid/MemEncryptLib.h | 20 +++++++ > OvmfPkg/OvmfPkg.dec | 1 + > 4 files changed, 84 insertions(+) > > diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.c b/OvmfPkg/AmdSevDxe/AmdSevDxe.c > index c66c4e9b92..45adf3249c 100644 > --- a/OvmfPkg/AmdSevDxe/AmdSevDxe.c > +++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.c > @@ -15,10 +15,49 @@ > #include <Library/BaseMemoryLib.h> > #include <Library/DebugLib.h> > #include <Library/DxeServicesTableLib.h> > +#include <Library/UefiRuntimeServicesTableLib.h> > +#include <Library/UefiBootServicesTableLib.h> > #include <Library/MemEncryptSevLib.h> > #include <Library/MemoryAllocationLib.h> > +#include <Guid/MemEncryptLib.h> > +#include <Guid/EventGroup.h> > #include <Library/PcdLib.h> > > +STATIC > +VOID > +EFIAPI > +AmdSevDxeOnEndOfDxe ( > + IN EFI_EVENT Event, > + IN VOID *EventToSignal > + ) > +{ > + EFI_STATUS Status; > + BOOLEAN SevLiveMigrationEnabled; > + > + SevLiveMigrationEnabled = MemEncryptSevLiveMigrationIsEnabled(); > + > + if (SevLiveMigrationEnabled) { > + Status = gRT->SetVariable ( > + L"SevLiveMigrationEnabled", > + &gMemEncryptGuid, > + EFI_VARIABLE_NON_VOLATILE | > + EFI_VARIABLE_BOOTSERVICE_ACCESS | > + EFI_VARIABLE_RUNTIME_ACCESS, > + sizeof (BOOLEAN), Should be: sizeof SevLiveMigrationEnabled, > + &SevLiveMigrationEnabled > + ); > + > + DEBUG (( > + DEBUG_INFO, > + "%a: Setting SevLiveMigrationEnabled variable, status = %lx\n", > + __FUNCTION__, > + Status > + )); > + } > + > + DEBUG ((DEBUG_VERBOSE, "%a\n", __FUNCTION__)); Remove debug print. > +} > + > EFI_STATUS > EFIAPI > AmdSevDxeEntryPoint ( > @@ -30,6 +69,7 @@ AmdSevDxeEntryPoint ( > EFI_GCD_MEMORY_SPACE_DESCRIPTOR *AllDescMap; > UINTN NumEntries; > UINTN Index; > + EFI_EVENT Event; > > // > // Do nothing when SEV is not enabled > @@ -130,5 +170,24 @@ AmdSevDxeEntryPoint ( > } > } > > + // > + // Register EFI_END_OF_DXE_EVENT_GROUP_GUID event. > + // The notification function sets the runtime variable indicating OVMF > + // support for SEV live migration. > + // > + Status = gBS->CreateEventEx ( > + EVT_NOTIFY_SIGNAL, > + TPL_CALLBACK, > + AmdSevDxeOnEndOfDxe, > + NULL, > + &gEfiEndOfDxeEventGroupGuid, > + &Event > + ); > + > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_INFO, "%a: CreateEventEx(): %r\n", > + __FUNCTION__, Status)); > + } > + > return EFI_SUCCESS; > } > diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.inf b/OvmfPkg/AmdSevDxe/AmdSevDxe.inf > index 0676fcc5b6..f4e40ff412 100644 > --- a/OvmfPkg/AmdSevDxe/AmdSevDxe.inf > +++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.inf > @@ -45,3 +45,7 @@ > > [Pcd] > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId > + > +[Guids] > + gMemEncryptGuid > + gEfiEndOfDxeEventGroupGuid ## CONSUMES ## Event > 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 Should the filename, GUID #define name, and global var name include "AMD" or "SEV" in them? (and similarly in the corresponding Linux patch) Or: maybe the new "SevLiveMigrationEnabled" variable can be set in the confidential computing GUID? (not sure what are the guidelines for creating or reusing GUIDs). > @@ -0,0 +1,20 @@ > +/** @file > + > + AMD Memory Encryption GUID, define a new GUID for defining > + new UEFI enviroment variables assocaiated with SEV Memory Encryption. typos: environment, associated > + > + 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 > -Dov ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v5 4/4] OvmfPkg/AmdSevDxe: Add support for SEV live migration. 2021-07-19 7:31 ` Dov Murik @ 2021-07-19 15:22 ` Ashish Kalra 0 siblings, 0 replies; 17+ messages in thread From: Ashish Kalra @ 2021-07-19 15:22 UTC (permalink / raw) To: Dov Murik Cc: devel, dovmurik, brijesh.singh, tobin, Thomas.Lendacky, jejb, lersek, jordan.l.justen, ard.biesheuvel, erdemaktas, jiewen.yao, min.m.xu Hello Dov, On Mon, Jul 19, 2021 at 10:31:10AM +0300, Dov Murik wrote: > Ashish, > > > On 08/07/2021 17:09, Ashish Kalra wrote: > > From: Ashish Kalra <ashish.kalra@amd.com> > > > > Check for SEV live migration feature support, if detected > > setup a new UEFI enviroment variable to indicate OVMF > > support for SEV live migration. > > > > The new runtime UEFI environment variable is set via the > > notification function registered for the > > EFI_END_OF_DXE_EVENT_GROUP_GUID event in AmdSevDxe driver. > > > > > Why is this indirect notification needed? Why not simply call > gRT->SetVariable in AmdSevDxeEntryPoint (instead of calling CreateEventEx)? > AmdSevDxe module is an apriori driver so it gets loaded between PEI and DXE phases and the SetVariable call will fail at the driver's entry point as the Variable DXE module is still not loaded yet. So i need to wait for an event notification which is signaled after the Variable DXE module is loaded and i am using the EndOfDxe event notification to make this call. > If this is needed, please add a clarification (in the commit message and > before the CreateEventEx call). > Yes, i will add the above explantion. > > > Signed-off-by: Ashish Kalra <ashish.kalra@amd.com> > > --- > > OvmfPkg/AmdSevDxe/AmdSevDxe.c | 59 ++++++++++++++++++++ > > OvmfPkg/AmdSevDxe/AmdSevDxe.inf | 4 ++ > > OvmfPkg/Include/Guid/MemEncryptLib.h | 20 +++++++ > > OvmfPkg/OvmfPkg.dec | 1 + > > 4 files changed, 84 insertions(+) > > > > diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.c b/OvmfPkg/AmdSevDxe/AmdSevDxe.c > > index c66c4e9b92..45adf3249c 100644 > > --- a/OvmfPkg/AmdSevDxe/AmdSevDxe.c > > +++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.c > > @@ -15,10 +15,49 @@ > > #include <Library/BaseMemoryLib.h> > > #include <Library/DebugLib.h> > > #include <Library/DxeServicesTableLib.h> > > +#include <Library/UefiRuntimeServicesTableLib.h> > > +#include <Library/UefiBootServicesTableLib.h> > > #include <Library/MemEncryptSevLib.h> > > #include <Library/MemoryAllocationLib.h> > > +#include <Guid/MemEncryptLib.h> > > +#include <Guid/EventGroup.h> > > #include <Library/PcdLib.h> > > > > +STATIC > > +VOID > > +EFIAPI > > +AmdSevDxeOnEndOfDxe ( > > + IN EFI_EVENT Event, > > + IN VOID *EventToSignal > > + ) > > +{ > > + EFI_STATUS Status; > > + BOOLEAN SevLiveMigrationEnabled; > > + > > + SevLiveMigrationEnabled = MemEncryptSevLiveMigrationIsEnabled(); > > + > > + if (SevLiveMigrationEnabled) { > > + Status = gRT->SetVariable ( > > + L"SevLiveMigrationEnabled", > > + &gMemEncryptGuid, > > + EFI_VARIABLE_NON_VOLATILE | > > + EFI_VARIABLE_BOOTSERVICE_ACCESS | > > + EFI_VARIABLE_RUNTIME_ACCESS, > > + sizeof (BOOLEAN), > > Should be: > > sizeof SevLiveMigrationEnabled, > > > > > + &SevLiveMigrationEnabled > > + ); > > + > > + DEBUG (( > > + DEBUG_INFO, > > + "%a: Setting SevLiveMigrationEnabled variable, status = %lx\n", > > + __FUNCTION__, > > + Status > > + )); > > + } > > + > > + DEBUG ((DEBUG_VERBOSE, "%a\n", __FUNCTION__)); > > Remove debug print. > > Ok. > > +} > > + > > EFI_STATUS > > EFIAPI > > AmdSevDxeEntryPoint ( > > @@ -30,6 +69,7 @@ AmdSevDxeEntryPoint ( > > EFI_GCD_MEMORY_SPACE_DESCRIPTOR *AllDescMap; > > UINTN NumEntries; > > UINTN Index; > > + EFI_EVENT Event; > > > > // > > // Do nothing when SEV is not enabled > > @@ -130,5 +170,24 @@ AmdSevDxeEntryPoint ( > > } > > } > > > > + // > > + // Register EFI_END_OF_DXE_EVENT_GROUP_GUID event. > > + // The notification function sets the runtime variable indicating OVMF > > + // support for SEV live migration. > > + // > > + Status = gBS->CreateEventEx ( > > + EVT_NOTIFY_SIGNAL, > > + TPL_CALLBACK, > > + AmdSevDxeOnEndOfDxe, > > + NULL, > > + &gEfiEndOfDxeEventGroupGuid, > > + &Event > > + ); > > + > > + if (EFI_ERROR (Status)) { > > + DEBUG ((DEBUG_INFO, "%a: CreateEventEx(): %r\n", > > + __FUNCTION__, Status)); > > + } > > + > > return EFI_SUCCESS; > > } > > diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.inf b/OvmfPkg/AmdSevDxe/AmdSevDxe.inf > > index 0676fcc5b6..f4e40ff412 100644 > > --- a/OvmfPkg/AmdSevDxe/AmdSevDxe.inf > > +++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.inf > > @@ -45,3 +45,7 @@ > > > > [Pcd] > > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId > > + > > +[Guids] > > + gMemEncryptGuid > > + gEfiEndOfDxeEventGroupGuid ## CONSUMES ## Event > > 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 > > > Should the filename, GUID #define name, and global var name include > "AMD" or "SEV" in them? (and similarly in the corresponding Linux patch) > > Or: maybe the new "SevLiveMigrationEnabled" variable can be set in the > confidential computing GUID? (not sure what are the guidelines for > creating or reusing GUIDs). > > Ok, i will use the same one as used for the corresponding Linux patch. > > > @@ -0,0 +1,20 @@ > > +/** @file > > + > > + AMD Memory Encryption GUID, define a new GUID for defining > > + new UEFI enviroment variables assocaiated with SEV Memory Encryption. > > typos: environment, associated > > Thanks, Ashish ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2021-07-19 20:27 UTC | newest] Thread overview: 17+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-07-08 14:07 [PATCH v5 0/4] SEV Live Migration support for OVMF Ashish Kalra 2021-07-08 14:07 ` [PATCH v5 1/4] OvmfPkg/BaseMemEncryptLib: Support to issue unencrypted hypercall Ashish Kalra 2021-07-15 20:58 ` Dov Murik 2021-07-16 12:29 ` Ashish Kalra 2021-07-19 8:04 ` Dov Murik 2021-07-19 15:30 ` Ashish Kalra 2021-07-16 14:11 ` Lendacky, Thomas 2021-07-19 20:24 ` Ashish Kalra 2021-07-08 14:08 ` [PATCH v5 2/4] OvmfPkg/VmgExitLib: Add support for hypercalls with SEV-ES Ashish Kalra 2021-07-16 14:16 ` Lendacky, Thomas 2021-07-19 20:25 ` Ashish Kalra 2021-07-08 14:08 ` [PATCH v5 3/4] OvmfPkg/PlatformPei: Mark SEC GHCB page as unencrypted via hypercall Ashish Kalra 2021-07-16 14:22 ` Lendacky, Thomas 2021-07-19 20:27 ` Ashish Kalra 2021-07-08 14:09 ` [PATCH v5 4/4] OvmfPkg/AmdSevDxe: Add support for SEV live migration Ashish Kalra 2021-07-19 7:31 ` Dov Murik 2021-07-19 15:22 ` Ashish Kalra
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox