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

["Ashish Kalra" <ashish.kalra@amd.com>]

Hello Dov,

On Mon, Jul 19, 2021 at 10:31:10AM +0300, Dov Murik wrote:
> 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)?
> 

AmdSevDxe module is an apriori driver so it gets loaded between PEI and
DXE phases and the SetVariable call will fail at the driver's entry
point as the Variable DXE module is still not loaded yet.

So i need to wait for an event notification which is signaled after the
Variable DXE module is loaded and i am using the EndOfDxe event
notification to make this call.

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

Yes, i will add the above explantion. 

> 
> > 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.
> 
> 
Ok.
> > +}
> > +
> >  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).
> 
> 

Ok, i will use the same one as used for the corresponding Linux patch.

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

Thanks,
Ashish