Re: [edk2] [RFC PATCH v2 09/10] OvmfPkg/QemuFwCfgLib: Add Secure Encrypted Virtualization (SEV) support

[Brijesh Singh]

On Mon, Mar 27, 2017 at 5:19 AM, Laszlo Ersek  wrote:

> On 03/21/17 22:13, Brijesh Singh wrote:
> > The patch adds SEV support in QemuFwCfgLib. When SEV is enabled:
> >
> >  * Pei phase support IO-style operations. This is mainly because we need
> to
> >use a bounce buffer inorder to support DMA operation. Allocate a
> memory
> >for bounce buffer can get painful in Pei phase hence if we detect
> FWCfg DMA
> >support then silently fallback to IO.
> >
> > * Dxe phase supports both IO and DMA style operations.
> > ---
> >  OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c|   73 +
> >  OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxeLib.inf   |2
> >  OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c|  112
> 
> >  .../Library/QemuFwCfgLib/QemuFwCfgLibInternal.h|   38 +++
> >  OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPei.c|   93
> +
> >  OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPeiLib.inf   |2
> >  OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c|   82
> +++
> >  7 files changed, 402 insertions(+)
>
> Can you please split this patch into 5 patches?
> - new interfaces in QemuFwCfgLibInternal.h
> - implementation of new APIs for SEC instance
> - implementation of new APIs for PEI instance
> - implementation of new APIs for DXE instance
> - call new APIs from common code
>
> If that is possible. Otherwise, it's quite hard to review & compare the
> instances for the different firmware phases.
>
>
Sure I will divide into multiple patches.



> Anyway, some things I've noticed:
>
> - InternalQemuFwCfgSevIsEnabled() should use global variables in the PEI
> instance as well. Possibly the appropriate interface from
> BaseMemcryptSevLib could be called from both the PEI and DXE instances
> here. (I think the DXE instance already does that.)
>
>
Id tou are okay with it then I will perfer to use BaseMemEncryptSevLib's
provided function in both PEI and DXE phases.



> - regarding InternalQemuFwCfgDmaBytes(): would it be possible to hook
> the SEV-related tricks into the existent function, rather than
> copy+modify the entire function as InternalQemuFwCfgSevDmaBytes()? It
> seems to me that a conditional "prologue" and "epilogue" in
> InternalQemuFwCfgDmaBytes() could do the trick.
>
>
I wanted to reuse the existing function but I was not sure how do you
wanted me to handle static allocation of "Access" variable inside
the InternalQemuFwCfgDmaBytes()

 VOID
 InternalQemuFwCfgDmaBytes (
   IN UINT32   Size,
   IN OUT VOID *Buffer OPTIONAL,
   IN UINT32   Control
   )
 {
   volatile FW_CFG_DMA_ACCESS Access;
   UINT32 AccessHigh, AccessLow;
   UINT32 Status;


The "Access" variable is statically allocated inside this function, in case
of SEV the Dma "Access" control variable need to be dynamically allocated
because hypervisor read/writes data into Access->status.

I could convert Access into FW_CFG_DMA_ACCESS pointer for non SEV case and
then integerate the SEV specific changes inside the same function.

I was just not sure which approach is prefrered hence decided to create a
new function for now and at least start the discussion.




> ... From skimming this patch, I think those are the only
> functionality-related comments I have at this point, beyond the remarks
> I made elsewhere in this series (like: line length, DEBUG_* macros, and
> so on). Please recheck all patches for those comments.
>
>
I will revist all the patches and fix those DEBUG_* macros.



> >
> > diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c
> b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c
> > index ac05f4c..be8e945 100644
> > --- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c
> > +++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c
> > @@ -4,6 +4,7 @@
> >
> >Copyright (C) 2013, Red Hat, Inc.
> >Copyright (c) 2011 - 2013, Intel Corporation. All rights reserved.
> > +  Copyright (c) 2017, Advanced Micro Devices. All rights reserved.
> >
> >This program and the accompanying materials are licensed and made
> available
> >under the terms and conditions of the BSD License which accompanies
> this
> > @@ -14,14 +15,34 @@
> >WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> >  **/
> >
> > +#include "Uefi.h"
> > +
> > +#include 
> >  #include 
> >  #include 
> > +#include 
> > +#include 
> > +#include 
> >
> >  #include "QemuFwCfgLibInternal.h"
> >
> >  STATIC BOOLEAN mQemuFwCfgSupported = FALSE;
> >  STATIC BOOLEAN mQemuFwCfgDmaSupported;
> > +STATIC BOOLEAN mQemuFwCfgSevIsEnabled = FALSE;
> > +
> > +/**
> > + Returns a boolean indicating whether the SEV is enabled
> >
> > + @retvalTRUESEV is enabled
> > + @retvalFALSE   SEV is not enabled
> > +**/
> > +BOOLEAN
> > +InternalQemuFwCfgSevIsEnabled (
> > +  VOID
> > +  )
> > +{
> > +  return mQemuFwCfgSevIsEnabled;
> > +}
> >
> >  /**
> >Returns a boolean indicating if the firmware 

Re: [edk2] [RFC PATCH v2 09/10] OvmfPkg/QemuFwCfgLib: Add Secure Encrypted Virtualization (SEV) support

[Laszlo Ersek]

On 03/21/17 22:13, Brijesh Singh wrote:
> The patch adds SEV support in QemuFwCfgLib. When SEV is enabled:
> 
>  * Pei phase support IO-style operations. This is mainly because we need to
>use a bounce buffer inorder to support DMA operation. Allocate a memory
>for bounce buffer can get painful in Pei phase hence if we detect FWCfg DMA
>support then silently fallback to IO.
> 
> * Dxe phase supports both IO and DMA style operations.
> ---
>  OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c|   73 +
>  OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxeLib.inf   |2 
>  OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c|  112 
> 
>  .../Library/QemuFwCfgLib/QemuFwCfgLibInternal.h|   38 +++
>  OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPei.c|   93 +
>  OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPeiLib.inf   |2 
>  OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c|   82 +++
>  7 files changed, 402 insertions(+)

Can you please split this patch into 5 patches?
- new interfaces in QemuFwCfgLibInternal.h
- implementation of new APIs for SEC instance
- implementation of new APIs for PEI instance
- implementation of new APIs for DXE instance
- call new APIs from common code

If that is possible. Otherwise, it's quite hard to review & compare the
instances for the different firmware phases.

Anyway, some things I've noticed:

- InternalQemuFwCfgSevIsEnabled() should use global variables in the PEI
instance as well. Possibly the appropriate interface from
BaseMemcryptSevLib could be called from both the PEI and DXE instances
here. (I think the DXE instance already does that.)

- regarding InternalQemuFwCfgDmaBytes(): would it be possible to hook
the SEV-related tricks into the existent function, rather than
copy+modify the entire function as InternalQemuFwCfgSevDmaBytes()? It
seems to me that a conditional "prologue" and "epilogue" in
InternalQemuFwCfgDmaBytes() could do the trick.

... From skimming this patch, I think those are the only
functionality-related comments I have at this point, beyond the remarks
I made elsewhere in this series (like: line length, DEBUG_* macros, and
so on). Please recheck all patches for those comments.

Thanks!
Laszlo

> 
> diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c 
> b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c
> index ac05f4c..be8e945 100644
> --- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c
> +++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c
> @@ -4,6 +4,7 @@
>  
>Copyright (C) 2013, Red Hat, Inc.
>Copyright (c) 2011 - 2013, Intel Corporation. All rights reserved.
> +  Copyright (c) 2017, Advanced Micro Devices. All rights reserved.
>  
>This program and the accompanying materials are licensed and made available
>under the terms and conditions of the BSD License which accompanies this
> @@ -14,14 +15,34 @@
>WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>  **/
>  
> +#include "Uefi.h"
> +
> +#include 
>  #include 
>  #include 
> +#include 
> +#include 
> +#include 
>  
>  #include "QemuFwCfgLibInternal.h"
>  
>  STATIC BOOLEAN mQemuFwCfgSupported = FALSE;
>  STATIC BOOLEAN mQemuFwCfgDmaSupported;
> +STATIC BOOLEAN mQemuFwCfgSevIsEnabled = FALSE;
> +
> +/**
> + Returns a boolean indicating whether the SEV is enabled
>  
> + @retvalTRUESEV is enabled
> + @retvalFALSE   SEV is not enabled
> +**/
> +BOOLEAN
> +InternalQemuFwCfgSevIsEnabled (
> +  VOID
> +  )
> +{
> +  return mQemuFwCfgSevIsEnabled;
> +}
>  
>  /**
>Returns a boolean indicating if the firmware configuration interface
> @@ -79,6 +100,9 @@ QemuFwCfgInitialize (
>  mQemuFwCfgDmaSupported = TRUE;
>  DEBUG ((DEBUG_INFO, "QemuFwCfg interface (DMA) is supported.\n"));
>}
> +
> +  mQemuFwCfgSevIsEnabled = MemEncryptSevIsEnabled ();
> +
>return RETURN_SUCCESS;
>  }
>  
> @@ -114,3 +138,52 @@ InternalQemuFwCfgDmaIsAvailable (
>  {
>return mQemuFwCfgDmaSupported;
>  }
> +
> +/**
> + Allocate a bounce buffer for SEV DMA.
> +

Please document that this function either succeeds or doesn't return.

> +  @param[in] NumPage  Number of pages.
> +  @param[out]Buffer   Allocated DMA Buffer pointer
> +
> +**/
> +VOID
> +InternalQemuFwCfgSevDmaAllocateBuffer (
> +  IN UINT32   NumPages,
> +  OUTVOID **Buffer
> +  )
> +{
> +  EFI_STATUS  Status;
> +
> +  //
> +  // Allocate DMA bounce buffer
> +  //
> +  Status = BmDmaAllocateBuffer (TRUE, EfiBootServicesData, NumPages, Buffer);
> +  if (EFI_ERROR(Status)) {
> +DEBUG ((EFI_D_ERROR, "SEV: Failed to allocate bounce buffer %d pages\n", 
> NumPages));
> +ASSERT_EFI_ERROR (Status);
> +CpuDeadLoop ();
> +  }
> +
> +  DEBUG ((EFI_D_VERBOSE, "QemuFwCfgSevDma allocate buffer 0x%Lx Pages %d\n", 
> (UINTN)Buffer, NumPages));
> +}
> +
> +/**
> + Free the DMA buffer allocated using InternalQemuFwCfgSevDmaAllocateBuffer
> +
> +  @param[in] NumPage  Number of pages.
> +  @param[in] Buffer   DMA