public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Ashish Kalra" <ashish.kalra@amd.com>
To: Dov Murik <dovmurik@linux.ibm.com>
Cc: devel@edk2.groups.io, dovmurik@linux.vnet.ibm.com,
	brijesh.singh@amd.com, tobin@ibm.com, Thomas.Lendacky@amd.com,
	jejb@linux.ibm.com, lersek@redhat.com, jordan.l.justen@intel.com,
	ard.biesheuvel@arm.com, erdemaktas@google.com,
	jiewen.yao@intel.com, min.m.xu@intel.com
Subject: Re: [PATCH v5 1/4] OvmfPkg/BaseMemEncryptLib: Support to issue unencrypted hypercall
Date: Mon, 19 Jul 2021 15:30:09 +0000	[thread overview]
Message-ID: <20210719153009.GB24210@ashkalra_ubuntu_server> (raw)
In-Reply-To: <11cc351b-c1c6-f33c-5000-335125b098c8@linux.ibm.com>

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.
> >>>

  reply	other threads:[~2021-07-19 15:30 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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 [this message]
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

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210719153009.GB24210@ashkalra_ubuntu_server \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox