[edk2] [PATCH v1 4/4] OvmfPkg : QemuFwCfgLib: Map DMA buffer with CommonBuffer when SEV is enable

Brijesh Singh posted 4 patches 7 years, 4 months ago
[edk2] [PATCH v1 4/4] OvmfPkg : QemuFwCfgLib: Map DMA buffer with CommonBuffer when SEV is enable
Posted by Brijesh Singh 7 years, 4 months ago
Commit 09719a01b11b (OvmfPkg/QemuFwCfgLib: Implement SEV internal function for Dxe phase)
uses IOMMU protocol to allocate and free DMA buffer used by
QemuFwCfgDxe when SEV is enabled. During the initial commit we
made assumption that IOMMU.AllocateBuffer() will provide a buffer
with C-bit cleared. But inorder to comply with EFI PCI protocol,
recent changes in IoMmuDxe driver does not provide a buffer with
C-bit cleared. If the DMA buffer need to be bi-directional (i.e
processor and the DMA bus master can access the buffer equally)
then buffer must be Map() with BusMasterCommonBuffer operation.

The patch refactors the code and add the support to Map() the
FW_CFG_DMA_ACCESS buffer using BusMasterCommonBuffer operation.

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>
Contributed-under: TianoCore Contribution Agreement 1.0
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
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         | 131 -----------
 OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPei.c         | 101 +++++---
 OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c         |  56 ++---
 5 files changed, 292 insertions(+), 285 deletions(-)

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
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,
+  // 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
                           );
+
+  //
+  // Map the host buffer with BusMasterCommonBuffer64
+  //
+  Status = mIoMmuProtocol->Map (
+                             mIoMmuProtocol,
+                             EdkiiIoMmuOperationBusMasterCommonBuffer64,
+                             HostAddress,
+                             &Size,
+                             &DmaAddress,
+                             Mapping
+                             );
   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,
+                             HostAddress,
+                             &NumberOfBytes,
+                             DeviceAddress,
+                             Mapping
+                             );
+  return Status;
+}
+
+EFI_STATUS
+UnmapFwCfgDmaDataBuffer (
+  IN  VOID  *Mapping
+  )
+{
+  return mIoMmuProtocol->Unmap (mIoMmuProtocol, Mapping);
 }
 
 /**
- 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
+  //
+  if (MemEncryptSevIsEnabled ()) {
+    VOID                  *AccessBuffer;
+    EFI_PHYSICAL_ADDRESS  DataBuffer;
+
+    //
+    // Allocate DMA Access buffer
+    //
+    Status = AllocFwCfgDmaAccessBuffer (&AccessBuffer, &AccessMapping);
+    ASSERT_EFI_ERROR (Status);
+
+    Access = AccessBuffer;
+
+    //
+    // Map actual data buffer
+    //
+    if (Buffer) {
+      Status = MapFwCfgDmaDataBuffer (Control == FW_CFG_DMA_CTL_WRITE ? 1 : 0,
+                Buffer, Size, &DataBuffer, &DataMapping);
+      ASSERT_EFI_ERROR (Status);
+
+      Buffer = (VOID *) (UINTN) DataBuffer;
+    }
+  } 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);
+  }
 }
diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
index d4e09c434e6a..7f42f38d1c05 100644
--- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
+++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
@@ -45,137 +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 warnigns
-  // 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);
+  }
+
+  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
   )
 {
   //
-- 
2.7.4

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v1 4/4] OvmfPkg : QemuFwCfgLib: Map DMA buffer with CommonBuffer when SEV is enable
Posted by Laszlo Ersek 7 years, 4 months ago
On 07/31/17 21:31, Brijesh Singh wrote:

> diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
> index d4e09c434e6a..7f42f38d1c05 100644
> --- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
> +++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c

> -  // set NumPages to suppress incorrect compiler/analyzer warnigns
> -  // set NumPages to suppress incorrect compiler/analyzer warnings
> -  //
> -  NumPages = 0;

In order to review this patch more easily, I tried to apply it locally.
It failed, due to the above lines.

I think you may have an earlier mismerge in your local tree. Namely,
before committing e508e069a809 ("OvmfPkg/QemuFwCfgLib: Suppress GCC49
IA32 build failure", 2017-07-11), I fixed up the comment:

    [lersek@redhat.com: s/warnigns/warnings/ in the code comment]

and when you pulled master later, you may have ended up with a merge
rather than a fast-forward, in your local tree.

Please fix this and I'll review this patch in the next iteration.
(Applying the patch locally helps a bit with that, because removed and
added code are unfortunately intermixed in this patch, and so the
colorizing done by "git diff --color" helps.)

Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel