Re: [RFC v4 13/13] OvmfPkg/QemuFwCfgLib: Add SEV support

[Laszlo Ersek <lersek@redhat.com>]

comments below:

On 05/11/17 00:09, Brijesh Singh wrote:
> When SEV is enabled, use a bounce buffer to perform the DMA operation.
> 
> 
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c | 54 +++++++++++++++++++-
>  1 file changed, 52 insertions(+), 2 deletions(-)
> 
> diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
> index 73a19772bee1..86d8bf880e71 100644
> --- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
> +++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
> @@ -72,6 +72,8 @@ InternalQemuFwCfgDmaBytes (
>    volatile FW_CFG_DMA_ACCESS *Access;
>    UINT32                     AccessHigh, AccessLow;
>    UINT32                     Status;
> +  UINT32                     NumPages;
> +  VOID                       *DmaBuffer, *BounceBuffer;
>  
>    ASSERT (Control == FW_CFG_DMA_CTL_WRITE || Control == FW_CFG_DMA_CTL_READ ||
>      Control == FW_CFG_DMA_CTL_SKIP);
> @@ -80,11 +82,44 @@ InternalQemuFwCfgDmaBytes (
>      return;
>    }
>  
> -  Access = &LocalAccess;
> +  //
> +  // When SEV is enabled then allocate DMA bounce buffer
> +  //
> +  if (InternalQemuFwCfgSevIsEnabled ()) {
> +    UINT32  TotalSize;

(1) Please make TotalSize a UINTN.

> +
> +    TotalSize = sizeof (*Access);
> +    //
> +    // Control operation does not need buffer

(2) The comment should say "skip operation".

> +    //
> +    if (Control != FW_CFG_DMA_CTL_SKIP) {
> +      TotalSize += Size;
> +    }
> +
> +    //
> +    // Allocate SEV DMA bounce buffer
> +    //
> +    NumPages = EFI_SIZE_TO_PAGES (TotalSize);

(3) Please write

    NumPages = (UINT32)EFI_SIZE_TO_PAGES (TotalSize)

otherwise Visual Studio will likely yell at us.

> +    InternalQemuFwCfgSevDmaAllocateBuffer (NumPages, &BounceBuffer);
> +
> +    Access = BounceBuffer;
> +    DmaBuffer = BounceBuffer + sizeof (*Access);

(4) Please cast BounceBuffer to (UINT8*) before the addition; we
shouldn't do arithmetic on (VOID*).

> +
> +    //
> +    //  Copy data from Host buffer into DMA buffer
> +    //
> +    if (Buffer && Control == FW_CFG_DMA_CTL_WRITE) {

(5) The Control check suffices.

If FW_CFG_DMA_CTL_WRITE is passed in, then Buffer can only be NULL if
Size is also 0, and a zero size is handled transparently by CopyMem().

> +      CopyMem (DmaBuffer, Buffer, Size);

(Side remark: it's funny how this innocent-looking CopyMem() actually
implements decryption :))

> +    }
> +  } else {
> +    Access = &LocalAccess;
> +    DmaBuffer = Buffer;
> +    BounceBuffer = NULL;
> +  }
>  
>    Access->Control = SwapBytes32 (Control);
>    Access->Length  = SwapBytes32 (Size);
> -  Access->Address = SwapBytes64 ((UINTN)Buffer);
> +  Access->Address = SwapBytes64 ((UINTN)DmaBuffer);
>  
>    //
>    // Delimit the transfer from (a) modifications to Access, (b) in case of a
> @@ -117,6 +152,21 @@ InternalQemuFwCfgDmaBytes (
>    // After a read, the caller will want to use Buffer.
>    //
>    MemoryFence ();
> +
> +  //
> +  // If Bounce buffer was allocated then copy the data into host buffer and
> +  // free the bounce buffer
> +  //
> +  if (BounceBuffer) {

(6) The edk2 coding style wants us to write this as

  if (BounceBuffer != NULL) {

> +    //
> +    //  Copy data from DMA buffer into host buffer
> +    //
> +    if (Buffer && Control == FW_CFG_DMA_CTL_READ) {

(7) Again, checking only (Control == FW_CFG_DMA_CTL_READ) suffices.

> +      CopyMem (Buffer, DmaBuffer, Size);

(Side note: funny how this innocent-looking CopyMem() implements
encryption :))

> +    }
> +
> +    InternalQemuFwCfgSevDmaFreeBuffer (BounceBuffer, NumPages);
> +  }
>  }
>  
>  
> 

(8) In several comments above, you wrote "host buffer". Shouldn't those
say "guest buffer"?

I agree it is somewhat confusing, because in DMA parlance, "host buffer"
is likely the right term. Unfortunately, in virtualization, the "device"
that performs the DMA is actually the virtualization host, so "host
buffer" ends up meaning the exact opposite of what we want.

Can you replace the expression "host buffer" with "encrypted guest
buffer" everywhere?

Accordingly, can you replace the word "copy" with "encrypt" vs.
"decrypt" everywhere, as appropriate?

For example, we should end up with something like:

  //
  // Copy data from Host buffer into DMA buffer
  //

-->

  //
  // Decrypt data from encrypted guest buffer into DMA buffer
  //


Otherwise, the logic of the patch looks good to me.

Thanks!
Laszlo