[edk2-devel] [PATCH v5 1/4] OvmfPkg/BaseMemEncryptLib: Support to issue unencrypted hypercall
Dov Murik
dovmurik at linux.ibm.com
Mon Jul 19 08:04:17 UTC 2021
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 at 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 at intel.com>
>>> Cc: Laszlo Ersek <lersek at redhat.com>
>>> Cc: Ard Biesheuvel <ard.biesheuvel at arm.com>
>>> Signed-off-by: Brijesh Singh <brijesh.singh at amd.com>
>>> Signed-off-by: Ashish Kalra <ashish.kalra at 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).
>>> +
>>> + at 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).
>>> +
>>> + at 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).
>>> +
>>> + at 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.
>>>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#77877): https://edk2.groups.io/g/devel/message/77877
Mute This Topic: https://groups.io/mt/84068331/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list