public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Dov Murik" <dovmurik@linux.ibm.com>
To: Ashish Kalra <ashish.kalra@amd.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 11:04:17 +0300	[thread overview]
Message-ID: <11cc351b-c1c6-f33c-5000-335125b098c8@linux.ibm.com> (raw)
In-Reply-To: <20210716122925.GA17576@ashkalra_ubuntu_server>



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

  reply	other threads:[~2021-07-19  8:09 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 [this message]
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

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=11cc351b-c1c6-f33c-5000-335125b098c8@linux.ibm.com \
    --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