Re: [PATCH v5 4/4] OvmfPkg/AmdSevDxe: Add support for SEV live migration.

["Dov Murik" <dovmurik@linux.ibm.com>]

Ashish,


On 08/07/2021 17:09, Ashish Kalra wrote:
> From: Ashish Kalra <ashish.kalra@amd.com>
> 
> Check for SEV live migration feature support, if detected
> setup a new UEFI enviroment variable to indicate OVMF
> support for SEV live migration.
> 
> The new runtime UEFI environment variable is set via the
> notification function registered for the
> EFI_END_OF_DXE_EVENT_GROUP_GUID event in AmdSevDxe driver.
> 


Why is this indirect notification needed?  Why not simply call
gRT->SetVariable in AmdSevDxeEntryPoint (instead of calling CreateEventEx)?

If this is needed, please add a clarification (in the commit message and
before the CreateEventEx call).


> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
> ---
>  OvmfPkg/AmdSevDxe/AmdSevDxe.c        | 59 ++++++++++++++++++++
>  OvmfPkg/AmdSevDxe/AmdSevDxe.inf      |  4 ++
>  OvmfPkg/Include/Guid/MemEncryptLib.h | 20 +++++++
>  OvmfPkg/OvmfPkg.dec                  |  1 +
>  4 files changed, 84 insertions(+)
> 
> diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.c b/OvmfPkg/AmdSevDxe/AmdSevDxe.c
> index c66c4e9b92..45adf3249c 100644
> --- a/OvmfPkg/AmdSevDxe/AmdSevDxe.c
> +++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.c
> @@ -15,10 +15,49 @@
>  #include <Library/BaseMemoryLib.h>
>  #include <Library/DebugLib.h>
>  #include <Library/DxeServicesTableLib.h>
> +#include <Library/UefiRuntimeServicesTableLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
>  #include <Library/MemEncryptSevLib.h>
>  #include <Library/MemoryAllocationLib.h>
> +#include <Guid/MemEncryptLib.h>
> +#include <Guid/EventGroup.h>
>  #include <Library/PcdLib.h>
> 
> +STATIC
> +VOID
> +EFIAPI
> +AmdSevDxeOnEndOfDxe (
> +  IN EFI_EVENT Event,
> +  IN VOID      *EventToSignal
> +  )
> +{
> +  EFI_STATUS Status;
> +  BOOLEAN SevLiveMigrationEnabled;
> +
> +  SevLiveMigrationEnabled = MemEncryptSevLiveMigrationIsEnabled();
> +
> +  if (SevLiveMigrationEnabled) {
> +    Status = gRT->SetVariable (
> +               L"SevLiveMigrationEnabled",
> +               &gMemEncryptGuid,
> +               EFI_VARIABLE_NON_VOLATILE |
> +               EFI_VARIABLE_BOOTSERVICE_ACCESS |
> +               EFI_VARIABLE_RUNTIME_ACCESS,
> +               sizeof (BOOLEAN),

Should be:

                  sizeof SevLiveMigrationEnabled,



> +               &SevLiveMigrationEnabled
> +               );
> +
> +    DEBUG ((
> +      DEBUG_INFO,
> +      "%a: Setting SevLiveMigrationEnabled variable, status = %lx\n",
> +      __FUNCTION__,
> +      Status
> +      ));
> +  }
> +
> +  DEBUG ((DEBUG_VERBOSE, "%a\n", __FUNCTION__));

Remove debug print.


> +}
> +
>  EFI_STATUS
>  EFIAPI
>  AmdSevDxeEntryPoint (
> @@ -30,6 +69,7 @@ AmdSevDxeEntryPoint (
>    EFI_GCD_MEMORY_SPACE_DESCRIPTOR  *AllDescMap;
>    UINTN                            NumEntries;
>    UINTN                            Index;
> +  EFI_EVENT                        Event;
> 
>    //
>    // Do nothing when SEV is not enabled
> @@ -130,5 +170,24 @@ AmdSevDxeEntryPoint (
>      }
>    }
> 
> +  //
> +  // Register EFI_END_OF_DXE_EVENT_GROUP_GUID event.
> +  // The notification function sets the runtime variable indicating OVMF
> +  // support for SEV live migration.
> +  //
> +  Status = gBS->CreateEventEx (
> +                  EVT_NOTIFY_SIGNAL,
> +                  TPL_CALLBACK,
> +                  AmdSevDxeOnEndOfDxe,
> +                  NULL,
> +                  &gEfiEndOfDxeEventGroupGuid,
> +                  &Event
> +                  );
> +
> +  if (EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_INFO, "%a: CreateEventEx(): %r\n",
> +        __FUNCTION__, Status));
> +  }
> +
>    return EFI_SUCCESS;
>  }
> diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.inf b/OvmfPkg/AmdSevDxe/AmdSevDxe.inf
> index 0676fcc5b6..f4e40ff412 100644
> --- a/OvmfPkg/AmdSevDxe/AmdSevDxe.inf
> +++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.inf
> @@ -45,3 +45,7 @@
> 
>  [Pcd]
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId
> +
> +[Guids]
> +  gMemEncryptGuid
> +  gEfiEndOfDxeEventGroupGuid         ## CONSUMES   ## Event
> diff --git a/OvmfPkg/Include/Guid/MemEncryptLib.h b/OvmfPkg/Include/Guid/MemEncryptLib.h
> new file mode 100644
> index 0000000000..4c046ba439
> --- /dev/null
> +++ b/OvmfPkg/Include/Guid/MemEncryptLib.h


Should the filename, GUID #define name, and global var name include
"AMD" or "SEV" in them? (and similarly in the corresponding Linux patch)

Or: maybe the new "SevLiveMigrationEnabled" variable can be set in the
confidential computing GUID? (not sure what are the guidelines for
creating or reusing GUIDs).



> @@ -0,0 +1,20 @@
> +/** @file
> +
> +  AMD Memory Encryption GUID, define a new GUID for defining
> +  new UEFI enviroment variables assocaiated with SEV Memory Encryption.

typos: environment, associated


> +
> +  Copyright (c) 2020, AMD Inc. All rights reserved.<BR>
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#ifndef __MEMENCRYPT_LIB_H__
> +#define __MEMENCRYPT_LIB_H__
> +
> +#define MEMENCRYPT_GUID \
> +{0x0cf29b71, 0x9e51, 0x433a, {0xa3, 0xb7, 0x81, 0xf3, 0xab, 0x16, 0xb8, 0x75}}
> +
> +extern EFI_GUID gMemEncryptGuid;
> +
> +#endif
> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
> index 6ae733f6e3..e452dc8494 100644
> --- a/OvmfPkg/OvmfPkg.dec
> +++ b/OvmfPkg/OvmfPkg.dec
> @@ -122,6 +122,7 @@
>    gQemuKernelLoaderFsMediaGuid          = {0x1428f772, 0xb64a, 0x441e, {0xb8, 0xc3, 0x9e, 0xbd, 0xd7, 0xf8, 0x93, 0xc7}}
>    gGrubFileGuid                         = {0xb5ae312c, 0xbc8a, 0x43b1, {0x9c, 0x62, 0xeb, 0xb8, 0x26, 0xdd, 0x5d, 0x07}}
>    gConfidentialComputingSecretGuid      = {0xadf956ad, 0xe98c, 0x484c, {0xae, 0x11, 0xb5, 0x1c, 0x7d, 0x33, 0x64, 0x47}}
> +  gMemEncryptGuid                       = {0x0cf29b71, 0x9e51, 0x433a, {0xa3, 0xb7, 0x81, 0xf3, 0xab, 0x16, 0xb8, 0x75}}
> 
>  [Ppis]
>    # PPI whose presence in the PPI database signals that the TPM base address
> 


-Dov