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.ka...@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.jus...@intel.com>
> >>> Cc: Laszlo Ersek <ler...@redhat.com>
> >>> Cc: Ard Biesheuvel <ard.biesheu...@arm.com>
> >>> Signed-off-by: Brijesh Singh <brijesh.si...@amd.com>
> >>> Signed-off-by: Ashish Kalra <ashish.ka...@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.
> >>>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#77903): https://edk2.groups.io/g/devel/message/77903
Mute This Topic: https://groups.io/mt/84068331/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
