Re: [PATCH 1/3] OvmfPkg: Realize EfiMemoryAcceptProtocol in AmdSevDxe

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

On 9/22/22 15:50, Dionna Glaze wrote:
> From: Sophia Wolf <phiawolf@google.com>
> 
> When a guest OS does not support unaccepted memory, the unaccepted
> memory must be accepted before returning a memory map to the caller.
> 
> EfiMemoryAcceptProtocol is defined in MdePkg and is implementated /
> Installed in AmdSevDxe for AMD SEV-SNP memory acceptance.
> 
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> 
> Signed-off-by: Sophia Wolf <phiawolf@google.com>
> ---
>   OvmfPkg/AmdSevDxe/AmdSevDxe.c                 | 27 ++++++++++++++
>   OvmfPkg/AmdSevDxe/AmdSevDxe.inf               |  3 ++
>   OvmfPkg/Include/Library/MemEncryptSevLib.h    | 14 ++++++++
>   .../Ia32/MemEncryptSevLib.c                   | 17 +++++++++
>   .../X64/DxeSnpSystemRamValidate.c             | 35 +++++++++++++++++++
>   .../X64/PeiSnpSystemRamValidate.c             | 17 +++++++++
>   .../X64/SecSnpSystemRamValidate.c             | 18 ++++++++++
>   7 files changed, 131 insertions(+)
> 
> diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.c b/OvmfPkg/AmdSevDxe/AmdSevDxe.c
> index 662d3c4ccb..74b82a5814 100644
> --- a/OvmfPkg/AmdSevDxe/AmdSevDxe.c
> +++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.c
> @@ -20,6 +20,7 @@
>   #include <Library/UefiBootServicesTableLib.h>
>   #include <Guid/ConfidentialComputingSevSnpBlob.h>
>   #include <Library/PcdLib.h>
> +#include <Protocol/MemoryAccept.h>
>   
>   STATIC CONFIDENTIAL_COMPUTING_SNP_BLOB_LOCATION  mSnpBootDxeTable = {
>     SIGNATURE_32 ('A',                                    'M', 'D', 'E'),
> @@ -31,6 +32,25 @@ STATIC CONFIDENTIAL_COMPUTING_SNP_BLOB_LOCATION  mSnpBootDxeTable = {
>     FixedPcdGet32 (PcdOvmfCpuidSize),
>   };
>   
> +EFI_HANDLE mAmdSevDxeHandle = NULL;

Add STATIC this variable and the function below.

> +
> +EFI_STATUS
> +EFIAPI
> +AmdSevMemoryAccept (
> +  IN EFI_MEMORY_ACCEPT_PROTOCOL *This,
> +  IN EFI_PHYSICAL_ADDRESS StartAddress,
> +  IN UINTN Size
> +)
> +{
> +  MemEncryptSnpAcceptPages (StartAddress, Size / SIZE_4KB);

Can't this instead just call MemEncryptSevSnpPreValidateSystemRam()? All 
phases have this function, so it would just be changing the version that 
is currently in
OvmfPkg/Library/BaseMemEncryptSevLib/X64/DxeSnpSystemRamValidate.c

Also, this needs to follow the coding standards and should be:

   MemEncryptSevSnpPreValidateSystemRam (
     StartAddress,
     EFI_SIZE_TO_PAGES (Size)
     );

> +
> +  return EFI_SUCCESS;
> +}
> +
> +EFI_MEMORY_ACCEPT_PROTOCOL      mMemoryAcceptProtocol = {

STATIC

> +  AmdSevMemoryAccept
> +};
> +
>   EFI_STATUS
>   EFIAPI
>   AmdSevDxeEntryPoint (
> @@ -147,6 +167,13 @@ AmdSevDxeEntryPoint (
>       }
>     }
>   
> +  Status = gBS->InstallProtocolInterface (&mAmdSevDxeHandle,
> +                  &gEfiMemoryAcceptProtocolGuid, EFI_NATIVE_INTERFACE,
> +                  &mMemoryAcceptProtocol);

   Status = gBS->InstallProtocolInterface (
                   &mAmdSevDxeHandle,
                   &gEfiMemoryAcceptProtocolGuid,
                   EFI_NATIVE_INTERFACE,
                   &mMemoryAcceptProtocol
                   );

(You'll need to this in all places in order to pass CI)

> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "Install EfiMemoryAcceptProtocol failed.\n"));
> +  }
> +
>     //
>     // If its SEV-SNP active guest then install the CONFIDENTIAL_COMPUTING_SEV_SNP_BLOB.
>     // It contains the location for both the Secrets and CPUID page.
> diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.inf b/OvmfPkg/AmdSevDxe/AmdSevDxe.inf
> index 9acf860cf2..5ddddabc32 100644
> --- a/OvmfPkg/AmdSevDxe/AmdSevDxe.inf
> +++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.inf
> @@ -47,6 +47,9 @@
>     gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpSecretsBase
>     gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpSecretsSize
>   
> +[Protocols]
> +  gEfiMemoryAcceptProtocolGuid
> +
>   [Guids]
>     gConfidentialComputingSevSnpBlobGuid
>   
> diff --git a/OvmfPkg/Include/Library/MemEncryptSevLib.h b/OvmfPkg/Include/Library/MemEncryptSevLib.h
> index 4fa9c0d700..05ec10471d 100644
> --- a/OvmfPkg/Include/Library/MemEncryptSevLib.h
> +++ b/OvmfPkg/Include/Library/MemEncryptSevLib.h
> @@ -228,4 +228,18 @@ MemEncryptSevSnpPreValidateSystemRam (
>     IN UINTN             NumPages
>     );
>   
> +/**
> +  Accept pages system RAM when SEV-SNP is enabled in the guest VM.
> +
> +  @param[in]  BaseAddress             Base address
> +  @param[in]  NumPages                Number of pages starting from the base address
> +
> +**/
> +VOID
> +EFIAPI
> +MemEncryptSnpAcceptPages (
> +  IN PHYSICAL_ADDRESS           BaseAddress,
> +  IN UINTN                      NumPages
> +  );
> +

This becomes unnecessary if you use MemEncryptSevSnpPreValidateSystemRam()
instead, since this will only be called during DXE, right?

Thanks,
Tom

>   #endif // _MEM_ENCRYPT_SEV_LIB_H_
> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c b/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c
> index f92299fc77..f0747d792e 100644
> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c
> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c
> @@ -153,3 +153,20 @@ MemEncryptSevSnpPreValidateSystemRam (
>   {
>     ASSERT (FALSE);
>   }
> +
> +/**
> +  Accept pages system RAM when SEV-SNP is enabled in the guest VM.
> +
> +  @param[in]  BaseAddress             Base address
> +  @param[in]  NumPages                Number of pages starting from the base address
> +
> +**/
> +VOID
> +EFIAPI
> +MemEncryptSnpAcceptPages (
> +  IN PHYSICAL_ADDRESS           BaseAddress,
> +  IN UINTN                      NumPages
> +  )
> +{
> +  ASSERT (FALSE);
> +}
> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/DxeSnpSystemRamValidate.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/DxeSnpSystemRamValidate.c
> index d3a95e4913..7693e0ca66 100644
> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/DxeSnpSystemRamValidate.c
> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/DxeSnpSystemRamValidate.c
> @@ -14,6 +14,7 @@
>   #include <Library/MemEncryptSevLib.h>
>   
>   #include "SnpPageStateChange.h"
> +#include "VirtualMemory.h"
>   
>   /**
>     Pre-validate the system RAM when SEV-SNP is enabled in the guest VM.
> @@ -38,3 +39,37 @@ MemEncryptSevSnpPreValidateSystemRam (
>     //
>     ASSERT (FALSE);
>   }
> +
> +/**
> +  Accept pages system RAM when SEV-SNP is enabled in the guest VM.
> +
> +  @param[in]  BaseAddress             Base address
> +  @param[in]  NumPages                Number of pages starting from the base address
> +
> +**/
> +VOID
> +EFIAPI
> +MemEncryptSnpAcceptPages (
> +  IN PHYSICAL_ADDRESS           BaseAddress,
> +  IN UINTN                      NumPages
> +  )
> +{
> +  EFI_STATUS Status;
> +
> +  if (!MemEncryptSevSnpIsEnabled ()) {
> +    return;
> +  }
> +  if (BaseAddress >= SIZE_4GB) {
> +    Status = InternalMemEncryptSevCreateIdentityMap1G (
> +               0,
> +               BaseAddress,
> +               EFI_PAGES_TO_SIZE (NumPages)
> +               );
> +    if (EFI_ERROR (Status)) {
> +      ASSERT (FALSE);
> +      CpuDeadLoop ();
> +    }
> +  }
> +
> +  InternalSetPageState (BaseAddress, NumPages, SevSnpPagePrivate, TRUE);
> +}
> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiSnpSystemRamValidate.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiSnpSystemRamValidate.c
> index 4970165444..1c52bfe691 100644
> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiSnpSystemRamValidate.c
> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiSnpSystemRamValidate.c
> @@ -126,3 +126,20 @@ MemEncryptSevSnpPreValidateSystemRam (
>       BaseAddress = EndAddress;
>     }
>   }
> +
> +/**
> +  Accept pages system RAM when SEV-SNP is enabled in the guest VM.
> +
> +  @param[in]  BaseAddress             Base address
> +  @param[in]  NumPages                Number of pages starting from the base address
> +
> +**/
> +VOID
> +EFIAPI
> +MemEncryptSnpAcceptPages (
> +  IN PHYSICAL_ADDRESS           BaseAddress,
> +  IN UINTN                      NumPages
> +  )
> +{
> +  ASSERT (FALSE);
> +}
> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecSnpSystemRamValidate.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecSnpSystemRamValidate.c
> index 7797febb8a..edfebf6ef4 100644
> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecSnpSystemRamValidate.c
> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecSnpSystemRamValidate.c
> @@ -10,6 +10,7 @@
>   
>   #include <Uefi/UefiBaseType.h>
>   #include <Library/BaseLib.h>
> +#include <Library/DebugLib.h>
>   #include <Library/MemEncryptSevLib.h>
>   
>   #include "SnpPageStateChange.h"
> @@ -80,3 +81,20 @@ MemEncryptSevSnpPreValidateSystemRam (
>   
>     InternalSetPageState (BaseAddress, NumPages, SevSnpPagePrivate, TRUE);
>   }
> +
> +/**
> +  Accept pages system RAM when SEV-SNP is enabled in the guest VM.
> +
> +  @param[in]  BaseAddress             Base address
> +  @param[in]  NumPages                Number of pages starting from the base address
> +
> +**/
> +VOID
> +EFIAPI
> +MemEncryptSnpAcceptPages (
> +  IN PHYSICAL_ADDRESS           BaseAddress,
> +  IN UINTN                      NumPages
> +  )
> +{
> +  ASSERT(FALSE);
> +}