Re: [PATCH v6 2/6] OvmfPkg/BaseMemEncryptLib: Hypercall API for page encryption state change

["Lendacky, Thomas" <thomas.lendacky@amd.com>]

On 8/2/21 7:31 AM, Ashish Kalra wrote:
> From: Ashish Kalra <ashish.kalra@amd.com>
> 
> Add API to issue hypercall on page encryption state change.
> 
> 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: 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                         | 43 +++++++++++++
>  OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf       |  1 +
>  OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c       | 27 +++++++++
>  OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf       |  1 +
>  OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLibInternal.c | 20 ++++++
>  OvmfPkg/Library/BaseMemEncryptSevLib/X64/AsmHelperStub.nasm        | 33 ++++++++++
>  OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c        | 64 ++++++++++++++++++++
>  7 files changed, 189 insertions(+)
> 
> diff --git a/OvmfPkg/Include/Library/MemEncryptSevLib.h b/OvmfPkg/Include/Library/MemEncryptSevLib.h
> index 59f694fb8a..56cc7bb958 100644
> --- a/OvmfPkg/Include/Library/MemEncryptSevLib.h
> +++ b/OvmfPkg/Include/Library/MemEncryptSevLib.h
> @@ -249,4 +249,47 @@ KvmDetectSevLiveMigrationFeature(
>    VOID
>    );
>  
> +/**
> + 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.
> + @param[in]   Pages                 Number of pages in memory region.
> + @param[in]   IsEncrypted           Encrypted or Decrypted.
> +
> + @retval RETURN_SUCCESS             Hypercall returned success.
> + @retval RETURN_UNSUPPORTED         Hypercall not supported.
> + @retval RETURN_NO_MAPPING          Hypercall returned error.
> +**/
> +RETURN_STATUS
> +EFIAPI
> +SetMemoryEncDecHypercall3 (
> +  IN  UINTN     PhysicalAddress,
> +  IN  UINTN     Pages,
> +  IN  BOOLEAN   IsEncrypted
> +  );
> +
> +#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)

s/STAT/STATE/ ?

> +#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)
> +
> +/**
> +  Interface exposed by the ASM implementation of the core hypercall

Need to put the function parameters in the comment here.

> +
> +  @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/Ia32/MemEncryptSevLib.c b/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c
> index be260e0d10..516d639489 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.
> + @param[in]   Pages                 Number of Pages in the memory region.
> + @param[in]   IsEncrypted           Encrypted or Decrypted.
> +
> + @retval RETURN_SUCCESS             Hypercall returned success.
> + @retval RETURN_UNSUPPORTED         Hypercall not supported.
> + @retval RETURN_NO_MAPPING          Hypercall returned error.
> +**/
> +RETURN_STATUS
> +EFIAPI
> +SetMemoryEncDecHypercall3 (
> +  IN  UINTN     PhysicalAddress,
> +  IN  UINTN     Pages,
> +  IN  BOOLEAN   IsEncrypted
> +  )
> +{
> +  //
> +  // Memory encryption bit is not accessible in 32-bit mode
> +  //
> +  return RETURN_UNSUPPORTED;
> +}
> 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/SecMemEncryptSevLibInternal.c b/OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLibInternal.c
> index d9f7befcd2..ebb1c39319 100644
> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLibInternal.c
> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLibInternal.c
> @@ -118,6 +118,26 @@ MemEncryptSevLiveMigrationIsEnabled (
>    return 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 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..0ec35dd9b6
> --- /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 Arg3 to register expected by KVM
> +  vmmcall         ; Call VMMCALL
> +  pop rbx
> +  ret
> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c
> index a57e8fd37f..fa679c9fc9 100644
> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c
> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c
> @@ -143,3 +143,67 @@ 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.
> + @param[in]   Pages                 Number of Pages in the memory region.
> + @param[in]   IsEncrypted           Encrypted or Decrypted.
> +
> + @retval RETURN_SUCCESS             Hypercall returned success.
> + @retval RETURN_UNSUPPORTED         Hypercall not supported.
> + @retval RETURN_NO_MAPPING          Hypercall returned error.
> +**/
> +RETURN_STATUS
> +EFIAPI
> +SetMemoryEncDecHypercall3 (
> +  IN  UINTN     PhysicalAddress,
> +  IN  UINTN     Pages,
> +  IN  BOOLEAN   IsEncrypted
> +  )
> +{
> +  RETURN_STATUS Ret;
> +  UINTN Error;
> +  UINTN EncryptMask;
> +
> +  Ret = RETURN_UNSUPPORTED;
> +
> +  if (MemEncryptSevLiveMigrationIsEnabled ()) {
> +    Ret = RETURN_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, 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 and adding an assert
> +    // here prevents booting. Hence, rounding it down when calling
> +    // SetMemoryEncDecHypercall3AsmStub below.
> +    //
> +
> +    EncryptMask = IsEncrypted ? KVM_MAP_GPA_RANGE_ENCRYPTED :
> +        KVM_MAP_GPA_RANGE_DECRYPTED;

Just a nit, but EncryptMask is a bit confusing because is sounds like the
encryption mask used by SEV, but it's really the page encryption state as
defined by the hypercall, maybe call it EncryptionState or EncryptState?

> +
> +    Error = SetMemoryEncDecHypercall3AsmStub (
> +              KVM_HC_MAP_GPA_RANGE,
> +              PhysicalAddress & ~EFI_PAGE_MASK,
> +              Pages,
> +              KVM_MAP_GPA_RANGE_PAGE_SZ_4K | EncryptMask
> +              );
> +
> +    if (Error != 0) {
> +      DEBUG ((DEBUG_ERROR,
> +              "SetMemoryEncDecHypercall3 failed, Phys = %Lx, Pages = %Ld, Err = %Ld\n",

I don't believe the "L" is needed for "Phys" and "Pages" since those are
UINTN variables.

> +              PhysicalAddress,
> +              Pages,
> +              (INT64)Error));

Indentation needs to be two spaces past the "DEBUG" function call.

Thanks,
Tom

> +
> +      Ret = RETURN_NO_MAPPING;
> +    }
> +  }
> +
> +  return Ret;
> +}
>