Re: [PATCH v2 1/1] OvmfPkg : QemuFwCfgLib: Use BusMasterCommandBuffer to map FW_CFG_DMA_ACCESS

[Brijesh Singh <brijesh.singh@amd.com>]

Hi Laszlo,

Thanks for the detail review, I will soon send v3 with all your feedback
addressed. I must admit that I have constant struggle with formating issues
in EDKII contributions. While browsing the code, several packages have
code and comment exceeding 79 char. But looking at your previous feedbacks
somehow I was under assumption that comments should be <= 79 chars but code
can exceed 79 char limit. I think my understanding was wrong. I will update
my vi setting to warn me when I exceed 79 char limit.

Sorry about all those formating errors, and thanks for being so patience.

-Brijesh

On 08/03/2017 03:51 PM, Laszlo Ersek wrote:
> On 08/03/17 18:10, Brijesh Singh wrote:
>> Commit 09719a01b11b (OvmfPkg/QemuFwCfgLib: Implement SEV internal function for Dxe phase)
>> uses IOMMU protocol to allocate and free FW_CFG_DMA_ACCESS buffer
>> when SEV is active. During initial commits we made assumption that
>> IOMMU.AllocateBuffer() will provide PlainTextAddress (i.e C-bit
>> cleared). This assumption was wrong, the AllocateBuffer() protocol
>> member is not expected to produce a buffer that is immediatly usable,
>> and client is required to call Map() uncondtionally with BusMasterCommonBuffer[64]
>> to get a mapping which is accessable by both host and device.
>>
>> The patch refactors code a bit and add the support to Map()
>> FW_CFG_DMA_ACCESS buffer using BusMasterCommonBuffer operation
>> after allocation and Unamp() before free.
>>
>> The complete discussion about this and recommendation from Laszlo
>> can be found here [1]
>>
>> [1] https://lists.01.org/pipermail/edk2-devel/2017-July/012652.html
>>
>> Suggested-by: Laszlo Ersek <lersek@redhat.com>
>> 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/QemuFwCfgLibInternal.h |  42 ++--
>>   OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c         | 247 ++++++++++++++++----
>>   OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c         | 130 -----------
>>   OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPei.c         | 101 +++++---
>>   OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c         |  56 ++---
>>   5 files changed, 292 insertions(+), 284 deletions(-)
> 
> (1) please remove the space characters in front of the colons (":") in
> the subject.
> 
> (2) Still for the subject, it is "CommonBuffer", not "CommandBuffer".
> 
> So the subject should be
> 
> OvmfPkg: QemuFwCfgLib: Use BusMasterCommonBuffer to map FW_CFG_DMA_ACCESS
> 
> (3) Please rewrap the commit message to 74 characters.
> 
>>
>> diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h
>> index 8cfa7913ffae..327f1d37e17d 100644
>> --- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h
>> +++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h
>> @@ -45,39 +45,25 @@ InternalQemuFwCfgDmaIsAvailable (
>>     );
>>   
>>   /**
>> - Returns a boolean indicating whether SEV support is enabled
>> +  Transfer an array of bytes, or skip a number of bytes, using the DMA
>> +  interface.
>>   
>> - @retval    TRUE    SEV is enabled
>> - @retval    FALSE   SEV is disabled
>> -**/
>> -BOOLEAN
>> -InternalQemuFwCfgSevIsEnabled (
>> -  VOID
>> -  );
>> +  @param[in]     Size     Size in bytes to transfer or skip.
>>   
>> -/**
>> - Allocate a bounce buffer for SEV DMA.
>> -
>> -  @param[out]    Buffer   Allocated DMA Buffer pointer
>> -  @param[in]     NumPage  Number of pages.
>> +  @param[in,out] Buffer   Buffer to read data into or write data from. Ignored,
>> +                          and may be NULL, if Size is zero, or Control is
>> +                          FW_CFG_DMA_CTL_SKIP.
>>   
>> +  @param[in]     Control  One of the following:
>> +                          FW_CFG_DMA_CTL_WRITE - write to fw_cfg from Buffer.
>> +                          FW_CFG_DMA_CTL_READ  - read from fw_cfg into Buffer.
>> +                          FW_CFG_DMA_CTL_SKIP  - skip bytes in fw_cfg.
>>   **/
>>   VOID
>> -InternalQemuFwCfgSevDmaAllocateBuffer (
>> -  OUT    VOID     **Buffer,
>> -  IN     UINT32   NumPages
>> +InternalQemuFwCfgDmaBytes (
>> +  IN     UINT32   Size,
>> +  IN OUT VOID     *Buffer OPTIONAL,
>> +  IN     UINT32   Control
>>     );
>>   
>> -/**
>> - Free the DMA buffer allocated using InternalQemuFwCfgSevDmaAllocateBuffer
>> -
>> -  @param[in]     NumPage  Number of pages.
>> -  @param[in]     Buffer   DMA Buffer pointer
>> -
>> -**/
>> -VOID
>> -InternalQemuFwCfgSevDmaFreeBuffer (
>> -  IN     VOID     *Buffer,
>> -  IN     UINT32   NumPages
>> -  );
>>   #endif
>> diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c
> 
> (4) You forgot to remove the InternalQemuFwCfgSevIsEnabled() function
> from this file.
> 
>> index f8eb03bc3a9a..e03647bae3bd 100644
>> --- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c
>> +++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c
>> @@ -20,6 +20,7 @@
>>   #include <Protocol/IoMmu.h>
>>   
>>   #include <Library/BaseLib.h>
>> +#include <Library/IoLib.h>
>>   #include <Library/DebugLib.h>
>>   #include <Library/QemuFwCfgLib.h>
>>   #include <Library/UefiBootServicesTableLib.h>
>> @@ -157,74 +158,228 @@ InternalQemuFwCfgDmaIsAvailable (
>>   }
>>   
>>   /**
>> - Allocate a bounce buffer for SEV DMA.
>> -
>> -  @param[in]     NumPage  Number of pages.
>> -  @param[out]    Buffer   Allocated DMA Buffer pointer
>> +  Function is used for allocating a bi-directional FW_CFG_DMA_ACCESS used
>> +  between Host and device to exchange the information. The buffer must be free'd
>> +  using FreeFwCfgDmaAccessBuffer ().
>>   
>>   **/
>> -VOID
>> -InternalQemuFwCfgSevDmaAllocateBuffer (
>> -  OUT    VOID     **Buffer,
>> -  IN     UINT32   NumPages
>> +STATIC
>> +EFI_STATUS
>> +AllocFwCfgDmaAccessBuffer (
>> +  OUT   VOID     **Access,
>> +  OUT   VOID     **Mapping
>>     )
>>   {
>> -  EFI_STATUS    Status;
>> +  UINTN                 Size;
>> +  UINTN                 NumPages;
>> +  EFI_STATUS            Status;
>> +  VOID                  *HostAddress;
>> +  EFI_PHYSICAL_ADDRESS  DmaAddress;
>>   
>> -  ASSERT (mIoMmuProtocol != NULL);
>> +  Size = sizeof (FW_CFG_DMA_ACCESS);
>> +  NumPages = EFI_SIZE_TO_PAGES (Size);
>>   
>> +  //
>> +  // As per UEFI spec,  in order to map a host address with BusMasterCommomBuffer64,
> 
> (5) This line is too long. Also, you have one too many space chars in
> front of the comma.
> 
>> +  // the buffer must be allocated using the IOMMU AllocateBuffer()
>> +  //
>>     Status = mIoMmuProtocol->AllocateBuffer (
>> -                            mIoMmuProtocol,
>> -                            0,
>> -                            EfiBootServicesData,
>> -                            NumPages,
>> -                            Buffer,
>> -                            EDKII_IOMMU_ATTRIBUTE_MEMORY_CACHED
>> +                             mIoMmuProtocol,
>> +                             AllocateAnyPages,
>> +                             EfiBootServicesData,
>> +                             NumPages,
>> +                             &HostAddress,
>> +                             EDKII_IOMMU_ATTRIBUTE_MEMORY_CACHED
> 
> (6) Please OR this attribute with
> EDKII_IOMMU_ATTRIBUTE_DUAL_ADDRESS_CYCLE, so that the buffer can be
> allocated from 64-bit memory.
> 
>>                             );
> 
> (7) The closing paren should be aligned with the arguments.
> 
> (8) Please handle any errors returned by AllocateBuffer(), by
> propagating the error.
> 
>> +
>> +  //
>> +  // Map the host buffer with BusMasterCommonBuffer64
>> +  //
>> +  Status = mIoMmuProtocol->Map (
>> +                             mIoMmuProtocol,
>> +                             EdkiiIoMmuOperationBusMasterCommonBuffer64,
>> +                             HostAddress,
>> +                             &Size,
>> +                             &DmaAddress,
>> +                             Mapping
>> +                             );
> 
> (9) Please add a check here that, on success, Size is still equal to
> sizeof(FW_CFG_DMA_ACCESS). (Theoretically it could be smaller, even on
> success.)
> 
> If it is smaller, then both the map and allocate operations should be
> rolled back, and we should return EFI_OUT_OF_RESOURCES. For this, error
> handling labels at the end of the function are likely the best.
> 
> (10) We should only set the output parameters Mapping and Access only if
> the entire function succeeds.
> 
>>     if (EFI_ERROR (Status)) {
>> -    DEBUG ((DEBUG_ERROR,
>> -      "%a:%a failed to allocate %u pages\n", gEfiCallerBaseName, __FUNCTION__,
>> -      NumPages));
>> -    ASSERT (FALSE);
>> -    CpuDeadLoop ();
>> +    mIoMmuProtocol->FreeBuffer (mIoMmuProtocol, NumPages, HostAddress);
>>     }
>>   
>> -  DEBUG ((DEBUG_VERBOSE,
>> -    "%a:%a buffer 0x%Lx Pages %u\n", gEfiCallerBaseName, __FUNCTION__,
>> -    (UINT64)(UINTN)Buffer, NumPages));
>> +  *Access = HostAddress;
>> +  return Status;
>> +}
>> +
>> +/**
>> +  Function is to used for freeing the Access buffer allocated using
>> +  AllocFwCfgDmaAccessBuffer()
>> +
>> +**/
>> +STATIC
>> +VOID
>> +FreeFwCfgDmaAccessBuffer (
>> +  IN  VOID    *Access,
>> +  IN  VOID    *Mapping
>> +  )
>> +{
>> +  UINTN   NumPages;
>> +
>> +  NumPages = EFI_SIZE_TO_PAGES (sizeof (FW_CFG_DMA_ACCESS));
>> +
>> +  mIoMmuProtocol->Unmap (mIoMmuProtocol, Mapping);
>> +
>> +  mIoMmuProtocol->FreeBuffer (mIoMmuProtocol, NumPages, Access);
>> +}
>> +
>> +/**
>> +  Function is used for mapping host address to device address. The buffer must
>> +  be unmapped with UnmapDmaDataBuffer ().
>> +
>> +**/
>> +STATIC
>> +EFI_STATUS
>> +MapFwCfgDmaDataBuffer (
>> +  IN  BOOLEAN               IsWrite,
>> +  IN  VOID                  *HostAddress,
>> +  IN  UINT32                Size,
>> +  OUT EFI_PHYSICAL_ADDRESS  *DeviceAddress,
>> +  OUT VOID                  **Mapping
>> +  )
>> +{
>> +  EFI_STATUS            Status;
>> +  UINTN                 NumberOfBytes;
>> +
>> +  NumberOfBytes = Size;
>> +  Status = mIoMmuProtocol->Map (
>> +                             mIoMmuProtocol,
>> +                             IsWrite ? EdkiiIoMmuOperationBusMasterRead64 : EdkiiIoMmuOperationBusMasterWrite64,
> 
> (11) Please make sure that the file is not wider than 80 characters --
> the above can be broken up like
> 
>   (IsWrite ?
>    EdkiiIoMmuOperationBusMasterRead64 :
>    EdkiiIoMmuOperationBusMasterWrite64)
> 
> And if that's still too wide, then a separate variable should be used.
> 
> 
>> +                             HostAddress,
>> +                             &NumberOfBytes,
>> +                             DeviceAddress,
>> +                             Mapping
>> +                             );
> 
> (12) Same comment as (9): on success, we should check that NumberOfBytes
> has the original value (i.e., Size). Otherwise, roll back the Map(), and
> return EFI_OUT_OF_RESOURCES.
> 
> (13) Same comment as (10): we should only set the output parameters
> (DeviceAddress and Mapping) if the entire function succeeds.
> 
> (14) Here's an alternative to the explicit / cascading error handling
> recommended above: given that all of these functions will be called from
> InternalQemuFwCfgSevDmaFreeBuffer(), which cannot propagate errors
> anyway, you might as well hang immediately upon error. For this, please
> preserve the following triplet, used earlier in
> InternalQemuFwCfgSevDmaAllocateBuffer():
> - DEBUG_ERROR message with gEfiCallerBaseName,
> - ASSERT (FALSE),
> - CpuDeadLoop().
> 
> Correspondingly, none of the helper functions would have to return
> EFI_STATUS.
> 
>> +  return Status;
>> +}
>> +
>> +EFI_STATUS
>> +UnmapFwCfgDmaDataBuffer (
>> +  IN  VOID  *Mapping
>> +  )
>> +{
>> +  return mIoMmuProtocol->Unmap (mIoMmuProtocol, Mapping);
>>   }
> 
> (15) I think it's fine if we ignore (don't propagate) the error here --
> please change the return type to VOID (similarly to
> FreeFwCfgDmaAccessBuffer()).
> 
> (16) Please make this function STATIC.
> 
>>   
>>   /**
>> - Free the DMA buffer allocated using InternalQemuFwCfgSevDmaAllocateBuffer
>> +  Transfer an array of bytes, or skip a number of bytes, using the DMA
>> +  interface.
>>   
>> -  @param[in]     NumPage  Number of pages.
>> -  @param[in]     Buffer   DMA Buffer pointer
>> +  @param[in]     Size     Size in bytes to transfer or skip.
>>   
>> +  @param[in,out] Buffer   Buffer to read data into or write data from. Ignored,
>> +                          and may be NULL, if Size is zero, or Control is
>> +                          FW_CFG_DMA_CTL_SKIP.
>> +
>> +  @param[in]     Control  One of the following:
>> +                          FW_CFG_DMA_CTL_WRITE - write to fw_cfg from Buffer.
>> +                          FW_CFG_DMA_CTL_READ  - read from fw_cfg into Buffer.
>> +                          FW_CFG_DMA_CTL_SKIP  - skip bytes in fw_cfg.
>>   **/
>>   VOID
>> -InternalQemuFwCfgSevDmaFreeBuffer (
>> -  IN     VOID     *Buffer,
>> -  IN     UINT32   NumPages
>> +InternalQemuFwCfgDmaBytes (
>> +  IN     UINT32   Size,
>> +  IN OUT VOID     *Buffer OPTIONAL,
>> +  IN     UINT32   Control
>>     )
>>   {
>> -  EFI_STATUS    Status;
>> +  volatile FW_CFG_DMA_ACCESS LocalAccess;
>> +  volatile FW_CFG_DMA_ACCESS *Access;
>> +  UINT32                     AccessHigh, AccessLow;
>> +  UINT32                     Status;
>> +  VOID                       *AccessMapping, *DataMapping;
>>   
>> -  ASSERT (mIoMmuProtocol != NULL);
>> +  ASSERT (Control == FW_CFG_DMA_CTL_WRITE || Control == FW_CFG_DMA_CTL_READ ||
>> +    Control == FW_CFG_DMA_CTL_SKIP);
>>   
>> -  Status = mIoMmuProtocol->FreeBuffer (
>> -                            mIoMmuProtocol,
>> -                            NumPages,
>> -                            Buffer
>> -                          );
>> -  if (EFI_ERROR (Status)) {
>> -    DEBUG ((DEBUG_ERROR,
>> -      "%a:%a failed to free buffer 0x%Lx pages %u\n", gEfiCallerBaseName,
>> -      __FUNCTION__, (UINT64)(UINTN)Buffer, NumPages));
>> -    ASSERT (FALSE);
>> -    CpuDeadLoop ();
>> +  if (Size == 0) {
>> +    return;
>>     }
>>   
>> -  DEBUG ((DEBUG_VERBOSE,
>> -    "%a:%a buffer 0x%Lx Pages %u\n", gEfiCallerBaseName,__FUNCTION__,
>> -    (UINT64)(UINTN)Buffer, NumPages));
>> +  //
>> +  // When SEV is enabled, map Buffer to DMA address before issuing the DMA request
> 
> (17) This line is too long.
> 
>> +  //
>> +  if (MemEncryptSevIsEnabled ()) {
>> +    VOID                  *AccessBuffer;
>> +    EFI_PHYSICAL_ADDRESS  DataBuffer;
>> +
>> +    //
>> +    // Allocate DMA Access buffer
>> +    //
>> +    Status = AllocFwCfgDmaAccessBuffer (&AccessBuffer, &AccessMapping);
>> +    ASSERT_EFI_ERROR (Status);
> 
> (18) You can drop the assert according to cmment (14).
> 
>> +
>> +    Access = AccessBuffer;
>> +
>> +    //
>> +    // Map actual data buffer
>> +    //
>> +    if (Buffer) {
> 
> (19) Please don't rely on Buffer being NULL for SKIP operations; that
> may or may not be true. Instead, please check for
> 
>    (Control != FW_CFG_DMA_CTL_SKIP)
> 
>> +      Status = MapFwCfgDmaDataBuffer (Control == FW_CFG_DMA_CTL_WRITE ? 1 : 0,
>> +                Buffer, Size, &DataBuffer, &DataMapping);
> 
> (20) Please break every argument to a separate line.
> 
> (21) The following expression:
> 
>    Control == FW_CFG_DMA_CTL_WRITE ? 1 : 0
> 
> should be simplified as:
> 
>    Control == FW_CFG_DMA_CTL_WRITE
> 
>> +      ASSERT_EFI_ERROR (Status);
> 
> (22) You can drop this, according to (14).
> 
>> +
>> +      Buffer = (VOID *) (UINTN) DataBuffer;
> 
> (23) Ugh, please don't overwrite a function parameter. Instead, please
> introduce an additional variable.
> 
> For example, you could rename "DataBuffer" to "DataBufferAddress", and
> introduce "DataBuffer" as a (VOID*). In the non-SEV case, you could set
> DataBuffer to Buffer, and in the SEV case, you could set DataBuffer to
> (VOID*)(UINTN)DataBufferAddress.
> 
> Then use DataBuffer for setting Access->Address.
> 
>> +    }
>> +  } else {
>> +    Access = &LocalAccess;
>> +    AccessMapping = NULL;
>> +    DataMapping = NULL;
>> +  }
>> +
>> +  Access->Control = SwapBytes32 (Control);
>> +  Access->Length  = SwapBytes32 (Size);
>> +  Access->Address = SwapBytes64 ((UINTN)Buffer);
>> +
>> +  //
>> +  // Delimit the transfer from (a) modifications to Access, (b) in case of a
>> +  // write, from writes to Buffer by the caller.
>> +  //
>> +  MemoryFence ();
>> +
>> +  //
>> +  // Start the transfer.
>> +  //
>> +  AccessHigh = (UINT32)RShiftU64 ((UINTN)Access, 32);
>> +  AccessLow  = (UINT32)(UINTN)Access;
>> +  IoWrite32 (FW_CFG_IO_DMA_ADDRESS,     SwapBytes32 (AccessHigh));
>> +  IoWrite32 (FW_CFG_IO_DMA_ADDRESS + 4, SwapBytes32 (AccessLow));
>> +
>> +  //
>> +  // Don't look at Access.Control before starting the transfer.
>> +  //
>> +  MemoryFence ();
>> +
>> +  //
>> +  // Wait for the transfer to complete.
>> +  //
>> +  do {
>> +    Status = SwapBytes32 (Access->Control);
>> +    ASSERT ((Status & FW_CFG_DMA_CTL_ERROR) == 0);
>> +  } while (Status != 0);
>> +
>> +  //
>> +  // After a read, the caller will want to use Buffer.
>> +  //
>> +  MemoryFence ();
>> +
>> +  //
>> +  // If Access buffer was dynamically allocated then free it.
>> +  //
>> +  if (AccessMapping) {
>> +    FreeFwCfgDmaAccessBuffer ((VOID *)Access, AccessMapping);
>> +  }
>> +
>> +  if (DataMapping) {
>> +    UnmapFwCfgDmaDataBuffer (DataMapping);
>> +  }
> 
> (24) The DataMapping check is not valid. You have a code path where
> DataMapping is not set: Buffer==NULL (i.e., SKIP operation) with SEV
> enabled. I guess the simplest way to fix this is to eliminate the
> non-SEV branch near the top, and make all those assignments unconditional:
> 
>    Access = &LocalAccess;
>    AccessMapping = NULL;
>    DataMapping = NULL;
>    DataBuffer = Buffer;
>    //
>    // When SEV is enabled...
>    //
>    if (MemEncryptSevIsEnabled ()) {
>      ...
> 
>>   }
>> diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
>> index d3bf75498d60..7f42f38d1c05 100644
>> --- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
>> +++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
>> @@ -45,136 +45,6 @@ QemuFwCfgSelectItem (
>>     IoWrite16 (FW_CFG_IO_SELECTOR, (UINT16)(UINTN) QemuFwCfgItem);
>>   }
>>   
>> -
>> -/**
>> -  Transfer an array of bytes, or skip a number of bytes, using the DMA
>> -  interface.
>> -
>> -  @param[in]     Size     Size in bytes to transfer or skip.
>> -
>> -  @param[in,out] Buffer   Buffer to read data into or write data from. Ignored,
>> -                          and may be NULL, if Size is zero, or Control is
>> -                          FW_CFG_DMA_CTL_SKIP.
>> -
>> -  @param[in]     Control  One of the following:
>> -                          FW_CFG_DMA_CTL_WRITE - write to fw_cfg from Buffer.
>> -                          FW_CFG_DMA_CTL_READ  - read from fw_cfg into Buffer.
>> -                          FW_CFG_DMA_CTL_SKIP  - skip bytes in fw_cfg.
>> -**/
>> -VOID
>> -InternalQemuFwCfgDmaBytes (
>> -  IN     UINT32   Size,
>> -  IN OUT VOID     *Buffer OPTIONAL,
>> -  IN     UINT32   Control
>> -  )
>> -{
>> -  volatile FW_CFG_DMA_ACCESS LocalAccess;
>> -  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);
>> -
>> -  if (Size == 0) {
>> -    return;
>> -  }
>> -
>> -  //
>> -  // set NumPages to suppress incorrect compiler/analyzer warnings
>> -  //
>> -  NumPages = 0;
>> -
>> -  //
>> -  // When SEV is enabled then allocate DMA bounce buffer
>> -  //
>> -  if (InternalQemuFwCfgSevIsEnabled ()) {
>> -    UINTN  TotalSize;
>> -
>> -    TotalSize = sizeof (*Access);
>> -    //
>> -    // Skip operation does not need buffer
>> -    //
>> -    if (Control != FW_CFG_DMA_CTL_SKIP) {
>> -      TotalSize += Size;
>> -    }
>> -
>> -    //
>> -    // Allocate SEV DMA buffer
>> -    //
>> -    NumPages = (UINT32)EFI_SIZE_TO_PAGES (TotalSize);
>> -    InternalQemuFwCfgSevDmaAllocateBuffer (&BounceBuffer, NumPages);
>> -
>> -    Access = BounceBuffer;
>> -    DmaBuffer = (UINT8*)BounceBuffer + sizeof (*Access);
>> -
>> -    //
>> -    //  Decrypt data from encrypted guest buffer into DMA buffer
>> -    //
>> -    if (Control == FW_CFG_DMA_CTL_WRITE) {
>> -      CopyMem (DmaBuffer, Buffer, Size);
>> -    }
>> -  } else {
>> -    Access = &LocalAccess;
>> -    DmaBuffer = Buffer;
>> -    BounceBuffer = NULL;
>> -  }
>> -
>> -  Access->Control = SwapBytes32 (Control);
>> -  Access->Length  = SwapBytes32 (Size);
>> -  Access->Address = SwapBytes64 ((UINTN)DmaBuffer);
>> -
>> -  //
>> -  // Delimit the transfer from (a) modifications to Access, (b) in case of a
>> -  // write, from writes to Buffer by the caller.
>> -  //
>> -  MemoryFence ();
>> -
>> -  //
>> -  // Start the transfer.
>> -  //
>> -  AccessHigh = (UINT32)RShiftU64 ((UINTN)Access, 32);
>> -  AccessLow  = (UINT32)(UINTN)Access;
>> -  IoWrite32 (FW_CFG_IO_DMA_ADDRESS,     SwapBytes32 (AccessHigh));
>> -  IoWrite32 (FW_CFG_IO_DMA_ADDRESS + 4, SwapBytes32 (AccessLow));
>> -
>> -  //
>> -  // Don't look at Access.Control before starting the transfer.
>> -  //
>> -  MemoryFence ();
>> -
>> -  //
>> -  // Wait for the transfer to complete.
>> -  //
>> -  do {
>> -    Status = SwapBytes32 (Access->Control);
>> -    ASSERT ((Status & FW_CFG_DMA_CTL_ERROR) == 0);
>> -  } while (Status != 0);
>> -
>> -  //
>> -  // After a read, the caller will want to use Buffer.
>> -  //
>> -  MemoryFence ();
>> -
>> -  //
>> -  // If Bounce buffer was allocated then copy the data into guest buffer and
>> -  // free the bounce buffer
>> -  //
>> -  if (BounceBuffer != NULL) {
>> -    //
>> -    //  Encrypt the data from DMA buffer into guest buffer
>> -    //
>> -    if (Control == FW_CFG_DMA_CTL_READ) {
>> -      CopyMem (Buffer, DmaBuffer, Size);
>> -    }
>> -
>> -    InternalQemuFwCfgSevDmaFreeBuffer (BounceBuffer, NumPages);
>> -  }
>> -}
>> -
>> -
>>   /**
>>     Reads firmware configuration bytes into a buffer
>>   
>> diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPei.c b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPei.c
>> index 40f89c3b53e2..bc649b40aec3 100644
>> --- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPei.c
>> +++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPei.c
>> @@ -16,6 +16,7 @@
>>   **/
>>   
>>   #include <Library/BaseLib.h>
>> +#include <Library/IoLib.h>
>>   #include <Library/DebugLib.h>
>>   #include <Library/QemuFwCfgLib.h>
>>   #include <Library/MemEncryptSevLib.h>
>> @@ -85,7 +86,7 @@ QemuFwCfgInitialize (
>>       // (which need to allocate dynamic memory and allocating a PAGE size'd
>>       // buffer can be challenge in PEI phase)
>>       //
>> -    if (InternalQemuFwCfgSevIsEnabled ()) {
>> +    if (MemEncryptSevIsEnabled ()) {
>>         DEBUG ((DEBUG_INFO, "SEV: QemuFwCfg fallback to IO Port interface.\n"));
>>       } else {
>>         mQemuFwCfgDmaSupported = TRUE;
>> @@ -129,56 +130,80 @@ InternalQemuFwCfgDmaIsAvailable (
>>   }
>>   
>>   /**
>> +  Transfer an array of bytes, or skip a number of bytes, using the DMA
>> +  interface.
>>   
>> - Returns a boolean indicating whether SEV is enabled
>> +  @param[in]     Size     Size in bytes to transfer or skip.
>>   
>> - @retval    TRUE    SEV is enabled
>> - @retval    FALSE   SEV is disabled
>> +  @param[in,out] Buffer   Buffer to read data into or write data from. Ignored,
>> +                          and may be NULL, if Size is zero, or Control is
>> +                          FW_CFG_DMA_CTL_SKIP.
>> +
>> +  @param[in]     Control  One of the following:
>> +                          FW_CFG_DMA_CTL_WRITE - write to fw_cfg from Buffer.
>> +                          FW_CFG_DMA_CTL_READ  - read from fw_cfg into Buffer.
>> +                          FW_CFG_DMA_CTL_SKIP  - skip bytes in fw_cfg.
>>   **/
>> -BOOLEAN
>> -InternalQemuFwCfgSevIsEnabled (
>> -  VOID
>> +VOID
>> +InternalQemuFwCfgDmaBytes (
>> +  IN     UINT32   Size,
>> +  IN OUT VOID     *Buffer OPTIONAL,
>> +  IN     UINT32   Control
>>     )
>>   {
>> -  return MemEncryptSevIsEnabled ();
>> -}
>> +  volatile FW_CFG_DMA_ACCESS Access;
>> +  UINT32                     AccessHigh, AccessLow;
>> +  UINT32                     Status;
>>   
>> -/**
>> - Allocate a bounce buffer for SEV DMA.
>> +  ASSERT (Control == FW_CFG_DMA_CTL_WRITE || Control == FW_CFG_DMA_CTL_READ ||
>> +    Control == FW_CFG_DMA_CTL_SKIP);
>>   
>> -  @param[in]     NumPage  Number of pages.
>> -  @param[out]    Buffer   Allocated DMA Buffer pointer
>> +  if (Size == 0) {
>> +    return;
>> +  }
>> +
>> +  if (MemEncryptSevIsEnabled ()) {
>> +    //
>> +    // SEV does not support DMA operations in PEI stage, we should
>> +    // not have reached here.
>> +    //
>> +    ASSERT (FALSE);
>> +  }
> 
> (25) This is good; you are basically restoring / copying the
> pre-7cfe445d7f4b state of the InternalQemuFwCfgDmaBytes() function, plus
> adding this assert. But, it can be written better like this:
> 
>    //
>    // ... keep the same comment ...
>    //
>    ASSERT (!MemEncryptSevIsEnabled ());
> 
>> +
>> +  Access.Control = SwapBytes32 (Control);
>> +  Access.Length  = SwapBytes32 (Size);
>> +  Access.Address = SwapBytes64 ((UINTN)Buffer);
>>   
>> -**/
>> -VOID
>> -InternalQemuFwCfgSevDmaAllocateBuffer (
>> -  OUT    VOID     **Buffer,
>> -  IN     UINT32   NumPages
>> -  )
>> -{
>>     //
>> -  // We should never reach here
>> +  // Delimit the transfer from (a) modifications to Access, (b) in case of a
>> +  // write, from writes to Buffer by the caller.
>>     //
>> -  ASSERT (FALSE);
>> -  CpuDeadLoop ();
>> -}
>> +  MemoryFence ();
>> +
>> +  //
>> +  // Start the transfer.
>> +  //
>> +  AccessHigh = (UINT32)RShiftU64 ((UINTN)&Access, 32);
>> +  AccessLow  = (UINT32)(UINTN)&Access;
>> +  IoWrite32 (FW_CFG_IO_DMA_ADDRESS,     SwapBytes32 (AccessHigh));
>> +  IoWrite32 (FW_CFG_IO_DMA_ADDRESS + 4, SwapBytes32 (AccessLow));
>>   
>> -/**
>> - Free the DMA buffer allocated using InternalQemuFwCfgSevDmaAllocateBuffer
>> +  //
>> +  // Don't look at Access.Control before starting the transfer.
>> +  //
>> +  MemoryFence ();
>>   
>> -  @param[in]     NumPage  Number of pages.
>> -  @param[in]     Buffer   DMA Buffer pointer
>> +  //
>> +  // Wait for the transfer to complete.
>> +  //
>> +  do {
>> +    Status = SwapBytes32 (Access.Control);
>> +    ASSERT ((Status & FW_CFG_DMA_CTL_ERROR) == 0);
>> +  } while (Status != 0);
>>   
>> -**/
>> -VOID
>> -InternalQemuFwCfgSevDmaFreeBuffer (
>> -  IN     VOID     *Buffer,
>> -  IN     UINT32   NumPages
>> -  )
>> -{
>>     //
>> -  // We should never reach here
>> +  // After a read, the caller will want to use Buffer.
>>     //
>> -  ASSERT (FALSE);
>> -  CpuDeadLoop ();
>> +  MemoryFence ();
>>   }
>> +
>> diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c
>> index 071b8d9b91d4..62ddb69d3b4d 100644
>> --- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c
>> +++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c
>> @@ -97,53 +97,25 @@ InternalQemuFwCfgDmaIsAvailable (
>>   }
>>   
>>   /**
>> +  Transfer an array of bytes, or skip a number of bytes, using the DMA
>> +  interface.
>>   
>> - Returns a boolean indicating whether SEV is enabled
>> +  @param[in]     Size     Size in bytes to transfer or skip.
>>   
>> - @retval    TRUE    SEV is enabled
>> - @retval    FALSE   SEV is disabled
>> -**/
>> -BOOLEAN
>> -InternalQemuFwCfgSevIsEnabled (
>> -  VOID
>> -  )
>> -{
>> -  //
>> -  // DMA is not supported in SEC phase hence SEV support is irrelevant
>> -  //
>> -  return FALSE;
>> -}
>> -
>> -/**
>> - Allocate a bounce buffer for SEV DMA.
>> -
>> -  @param[in]     NumPage  Number of pages.
>> -  @param[out]    Buffer   Allocated DMA Buffer pointer
>> -
>> -**/
>> -VOID
>> -InternalQemuFwCfgSevDmaAllocateBuffer (
>> -  OUT    VOID     **Buffer,
>> -  IN     UINT32   NumPages
>> -  )
>> -{
>> -  //
>> -  // We should never reach here
>> -  //
>> -  ASSERT (FALSE);
>> -}
>> -
>> -/**
>> - Free the DMA buffer allocated using InternalQemuFwCfgSevDmaAllocateBuffer
>> -
>> -  @param[in]     NumPage  Number of pages.
>> -  @param[in]     Buffer   DMA Buffer pointer
>> +  @param[in,out] Buffer   Buffer to read data into or write data from. Ignored,
>> +                          and may be NULL, if Size is zero, or Control is
>> +                          FW_CFG_DMA_CTL_SKIP.
>>   
>> +  @param[in]     Control  One of the following:
>> +                          FW_CFG_DMA_CTL_WRITE - write to fw_cfg from Buffer.
>> +                          FW_CFG_DMA_CTL_READ  - read from fw_cfg into Buffer.
>> +                          FW_CFG_DMA_CTL_SKIP  - skip bytes in fw_cfg.
>>   **/
>>   VOID
>> -InternalQemuFwCfgSevDmaFreeBuffer (
>> -  IN     VOID     *Buffer,
>> -  IN     UINT32   NumPages
>> +InternalQemuFwCfgDmaBytes (
>> +  IN     UINT32   Size,
>> +  IN OUT VOID     *Buffer OPTIONAL,
>> +  IN     UINT32   Control
>>     )
>>   {
>>     //
>>
> 
> (26) Can you add a CpuDeadLoop() here as well (i.e., in the SEC
> instance), after the ASSERT()? I know the previous version of the SEC
> instance doesn't use CpuDeadLoop(), but I mentioned it here:
> 
> http://mid.mail-archive.com/2b9361cc-03f9-a403-98aa-c1cf5bfd17c0@redhat.com
> 
> Thanks!
> Laszlo
>