[PATCH v3 0/1] Use BusMasterCommonBuffer to map FW_CFG_DMA_ACCESS

[PATCH v3 0/1] Use BusMasterCommonBuffer to map FW_CFG_DMA_ACCESS

[Brijesh Singh]

The patch applies on top of Laszlo's IoMmuDxe cleanup series [1].

Commit is also available at https://github.com/codomania/edk2/tree/qemufwcfg-sev

[1] https://lists.01.org/pipermail/edk2-devel/2017-August/012808.html

Changes since v2:
* Changes to address v2 feedbacks.

Changes since v1:
* Drop Patch 1 through 3, they are covered by Laszlo's cleanup series
* Rebase to the latest

Brijesh Singh (1):
  OvmfPkg/QemuFwCfgLib: Use BusMasterCommonBuffer to map
    FW_CFG_DMA_ACCESS

 OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h |  42 +--
 OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c         | 330 ++++++++++++++++----
 OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c         | 130 --------
 OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPei.c         |  99 +++---
 OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c         |  58 +---
 5 files changed, 367 insertions(+), 292 deletions(-)

-- 
2.7.4

[PATCH v3 1/1] OvmfPkg/QemuFwCfgLib: Use BusMasterCommonBuffer to map FW_CFG_DMA_ACCESS

[Brijesh Singh]

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         | 330 ++++++++++++++++----
 OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c         | 130 --------
 OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPei.c         |  99 +++---
 OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c         |  58 +---
 5 files changed, 367 insertions(+), 292 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..576941749cf2 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>
@@ -31,20 +32,6 @@ STATIC BOOLEAN mQemuFwCfgSupported = FALSE;
 STATIC BOOLEAN mQemuFwCfgDmaSupported;
 
 STATIC EDKII_IOMMU_PROTOCOL        *mIoMmuProtocol;
-/**
-
- Returns a boolean indicating whether SEV is enabled
-
- @retval    TRUE    SEV is enabled
- @retval    FALSE   SEV is disabled
-**/
-BOOLEAN
-InternalQemuFwCfgSevIsEnabled (
-  VOID
-  )
-{
-  return MemEncryptSevIsEnabled ();
-}
 
 /**
   Returns a boolean indicating if the firmware configuration interface
@@ -110,7 +97,8 @@ QemuFwCfgInitialize (
     // IoMmuDxe driver must have installed the IOMMU protocol. If we are not
     // able to locate the protocol then something must have gone wrong.
     //
-    Status = gBS->LocateProtocol (&gEdkiiIoMmuProtocolGuid, NULL, (VOID **)&mIoMmuProtocol);
+    Status = gBS->LocateProtocol (&gEdkiiIoMmuProtocolGuid, NULL,
+                    (VOID **)&mIoMmuProtocol);
     if (EFI_ERROR (Status)) {
       DEBUG ((DEBUG_ERROR,
         "QemuFwCfgSevDma %a:%a Failed to locate IOMMU protocol.\n",
@@ -157,74 +145,308 @@ 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 ().
 
 **/
+STATIC
 VOID
-InternalQemuFwCfgSevDmaAllocateBuffer (
-  OUT    VOID     **Buffer,
-  IN     UINT32   NumPages
+AllocFwCfgDmaAccessBuffer (
+  OUT   VOID     **Access,
+  OUT   VOID     **MapInfo
   )
 {
-  EFI_STATUS    Status;
+  UINTN                 Size;
+  UINTN                 NumPages;
+  EFI_STATUS            Status;
+  VOID                  *HostAddress;
+  EFI_PHYSICAL_ADDRESS  DmaAddress;
+  VOID                  *Mapping;
 
-  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_DUAL_ADDRESS_CYCLE
+                             );
   if (EFI_ERROR (Status)) {
     DEBUG ((DEBUG_ERROR,
-      "%a:%a failed to allocate %u pages\n", gEfiCallerBaseName, __FUNCTION__,
-      NumPages));
+      "%a:%a failed to allocate FW_CFG_DMA_ACCESS\n", gEfiCallerBaseName,
+      __FUNCTION__));
     ASSERT (FALSE);
     CpuDeadLoop ();
   }
 
-  DEBUG ((DEBUG_VERBOSE,
-    "%a:%a buffer 0x%Lx Pages %u\n", gEfiCallerBaseName, __FUNCTION__,
-    (UINT64)(UINTN)Buffer, NumPages));
+  //
+  // Map the host buffer with BusMasterCommonBuffer64
+  //
+  Status = mIoMmuProtocol->Map (
+                             mIoMmuProtocol,
+                             EdkiiIoMmuOperationBusMasterCommonBuffer64,
+                             HostAddress,
+                             &Size,
+                             &DmaAddress,
+                             &Mapping
+                             );
+  if (EFI_ERROR (Status)) {
+    mIoMmuProtocol->FreeBuffer (mIoMmuProtocol, NumPages, HostAddress);
+    DEBUG ((DEBUG_ERROR,
+      "%a:%a failed to Map() FW_CFG_DMA_ACCESS\n", gEfiCallerBaseName,
+      __FUNCTION__));
+    ASSERT (FALSE);
+    CpuDeadLoop ();
+  }
+
+  if (Size < sizeof (FW_CFG_DMA_ACCESS)) {
+    mIoMmuProtocol->Unmap (mIoMmuProtocol, Mapping);
+    mIoMmuProtocol->FreeBuffer (mIoMmuProtocol, NumPages, HostAddress);
+    DEBUG ((DEBUG_ERROR,
+      "%a:%a failed to Map() - requested 0x%Lx got 0x%Lx\n", gEfiCallerBaseName,
+      __FUNCTION__, (UINT64)sizeof (FW_CFG_DMA_ACCESS), (UINT64)Size));
+    ASSERT (FALSE);
+    CpuDeadLoop ();
+  }
+
+  *Access = HostAddress;
+  *MapInfo = Mapping;
 }
 
 /**
- Free the DMA buffer allocated using InternalQemuFwCfgSevDmaAllocateBuffer
+  Function is to used for freeing the Access buffer allocated using
+  AllocFwCfgDmaAccessBuffer()
+
+**/
+STATIC
+VOID
+FreeFwCfgDmaAccessBuffer (
+  IN  VOID    *Access,
+  IN  VOID    *Mapping
+  )
+{
+  UINTN       NumPages;
+  EFI_STATUS  Status;
+
+  NumPages = EFI_SIZE_TO_PAGES (sizeof (FW_CFG_DMA_ACCESS));
+
+  Status = mIoMmuProtocol->Unmap (mIoMmuProtocol, Mapping);
+  if (EFI_ERROR (Status)) {
+    DEBUG ((DEBUG_ERROR,
+      "%a:%a failed to UnMap() Mapping 0x%Lx\n", gEfiCallerBaseName,
+      __FUNCTION__, (UINT64)Mapping));
+    ASSERT (FALSE);
+    CpuDeadLoop ();
+  }
 
-  @param[in]     NumPage  Number of pages.
-  @param[in]     Buffer   DMA Buffer pointer
+  Status = mIoMmuProtocol->FreeBuffer (mIoMmuProtocol, NumPages, Access);
+  if (EFI_ERROR (Status)) {
+    DEBUG ((DEBUG_ERROR,
+      "%a:%a failed to Free() 0x%Lx\n", gEfiCallerBaseName, __FUNCTION__,
+      (UINT64) Access));
+    ASSERT (FALSE);
+    CpuDeadLoop ();
+  }
+}
+
+/**
+  Function is used for mapping host address to device address. The buffer must
+  be unmapped with UnmapDmaDataBuffer ().
 
 **/
+STATIC
 VOID
-InternalQemuFwCfgSevDmaFreeBuffer (
-  IN     VOID     *Buffer,
-  IN     UINT32   NumPages
+MapFwCfgDmaDataBuffer (
+  IN  BOOLEAN               IsWrite,
+  IN  VOID                  *HostAddress,
+  IN  UINT32                Size,
+  OUT EFI_PHYSICAL_ADDRESS  *DeviceAddress,
+  OUT VOID                  **MapInfo
   )
 {
-  EFI_STATUS    Status;
+  EFI_STATUS              Status;
+  UINTN                   NumberOfBytes;
+  VOID                    *Mapping;
+  EFI_PHYSICAL_ADDRESS    PhysicalAddress;
+
+  NumberOfBytes = Size;
+  Status = mIoMmuProtocol->Map (
+                             mIoMmuProtocol,
+                             (IsWrite ?
+                              EdkiiIoMmuOperationBusMasterRead64 :
+                              EdkiiIoMmuOperationBusMasterWrite64),
+                             HostAddress,
+                             &NumberOfBytes,
+                             &PhysicalAddress,
+                             &Mapping
+                             );
+  if (EFI_ERROR (Status)) {
+    DEBUG ((DEBUG_ERROR,
+      "%a:%a failed to Map() Address 0x%Lx Size 0x%Lx\n", gEfiCallerBaseName,
+      __FUNCTION__, (UINT64) HostAddress, (UINT64)Size));
+    ASSERT (FALSE);
+    CpuDeadLoop ();
+  }
+
+  if (NumberOfBytes < Size) {
+    mIoMmuProtocol->Unmap (mIoMmuProtocol, Mapping);
+    DEBUG ((DEBUG_ERROR,
+      "%a:%a failed to Map() - requested 0x%x got 0x%Lx\n", gEfiCallerBaseName,
+      __FUNCTION__, Size, (UINT64)NumberOfBytes));
+    ASSERT (FALSE);
+    CpuDeadLoop ();
+  }
+
+  *DeviceAddress = PhysicalAddress;
+  *MapInfo = Mapping;
+}
 
-  ASSERT (mIoMmuProtocol != NULL);
+STATIC
+VOID
+UnmapFwCfgDmaDataBuffer (
+  IN  VOID  *Mapping
+  )
+{
+  EFI_STATUS  Status;
 
-  Status = mIoMmuProtocol->FreeBuffer (
-                            mIoMmuProtocol,
-                            NumPages,
-                            Buffer
-                          );
+  Status = mIoMmuProtocol->Unmap (mIoMmuProtocol, Mapping);
   if (EFI_ERROR (Status)) {
     DEBUG ((DEBUG_ERROR,
-      "%a:%a failed to free buffer 0x%Lx pages %u\n", gEfiCallerBaseName,
-      __FUNCTION__, (UINT64)(UINTN)Buffer, NumPages));
+      "%a:%a failed to UnMap() Mapping 0x%Lx\n", gEfiCallerBaseName,
+      __FUNCTION__, (UINT64)Mapping));
     ASSERT (FALSE);
     CpuDeadLoop ();
   }
+}
+
+/**
+  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;
+  VOID                       *AccessMapping, *DataMapping;
+  VOID                       *DataBuffer;
+
+  ASSERT (Control == FW_CFG_DMA_CTL_WRITE || Control == FW_CFG_DMA_CTL_READ ||
+    Control == FW_CFG_DMA_CTL_SKIP);
+
+  if (Size == 0) {
+    return;
+  }
+
+  Access = &LocalAccess;
+  AccessMapping = NULL;
+  DataMapping = NULL;
+  DataBuffer = Buffer;
+
+  //
+  // When SEV is enabled, map Buffer to DMA address before issuing the DMA
+  // request
+  //
+  if (MemEncryptSevIsEnabled ()) {
+    VOID                  *AccessBuffer;
+    EFI_PHYSICAL_ADDRESS  DataBufferAddress;
+
+    //
+    // Allocate DMA Access buffer
+    //
+    AllocFwCfgDmaAccessBuffer (&AccessBuffer, &AccessMapping);
+
+    Access = AccessBuffer;
+
+    //
+    // Map actual data buffer
+    //
+    if (Control != FW_CFG_DMA_CTL_SKIP) {
+      MapFwCfgDmaDataBuffer (
+                             Control == FW_CFG_DMA_CTL_WRITE,
+                             Buffer,
+                             Size,
+                             &DataBufferAddress,
+                             &DataMapping
+                             );
+
+      DataBuffer = (VOID *) (UINTN) DataBufferAddress;
+    }
+  }
 
-  DEBUG ((DEBUG_VERBOSE,
-    "%a:%a buffer 0x%Lx Pages %u\n", gEfiCallerBaseName,__FUNCTION__,
-    (UINT64)(UINTN)Buffer, NumPages));
+  Access->Control = SwapBytes32 (Control);
+  Access->Length  = SwapBytes32 (Size);
+  Access->Address = SwapBytes64 ((UINTN)DataBuffer);
+
+  //
+  // 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 DataBuffer was mapped then unmap it.
+  //
+  if (DataMapping) {
+    UnmapFwCfgDmaDataBuffer (DataMapping);
+  }
 }
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..471ab0335e64 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,78 @@ 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;
+  }
 
-**/
-VOID
-InternalQemuFwCfgSevDmaAllocateBuffer (
-  OUT    VOID     **Buffer,
-  IN     UINT32   NumPages
-  )
-{
   //
-  // We should never reach here
+  // SEV does not support DMA operations in PEI stage, we should
+  // not have reached here.
   //
-  ASSERT (FALSE);
-  CpuDeadLoop ();
-}
+  ASSERT (!MemEncryptSevIsEnabled ());
 
-/**
- Free the DMA buffer allocated using InternalQemuFwCfgSevDmaAllocateBuffer
+  Access.Control = SwapBytes32 (Control);
+  Access.Length  = SwapBytes32 (Size);
+  Access.Address = SwapBytes64 ((UINTN)Buffer);
 
-  @param[in]     NumPage  Number of pages.
-  @param[in]     Buffer   DMA Buffer pointer
+  //
+  // Delimit the transfer from (a) modifications to Access, (b) in case of a
+  // write, from writes to Buffer by the caller.
+  //
+  MemoryFence ();
 
-**/
-VOID
-InternalQemuFwCfgSevDmaFreeBuffer (
-  IN     VOID     *Buffer,
-  IN     UINT32   NumPages
-  )
-{
   //
-  // We should never reach here
+  // 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.
   //
-  ASSERT (FALSE);
-  CpuDeadLoop ();
+  MemoryFence ();
 }
+
diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c
index 071b8d9b91d4..1eadba99e1b3 100644
--- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c
+++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c
@@ -17,6 +17,7 @@
   WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
 **/
 
+#include <Library/BaseLib.h>
 #include <Library/DebugLib.h>
 #include <Library/QemuFwCfgLib.h>
 
@@ -97,57 +98,30 @@ 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
   )
 {
   //
   // We should never reach here
   //
   ASSERT (FALSE);
+  CpuDeadLoop ();
 }
-- 
2.7.4

Issue of step by step debugging of OVMF SEC code in QEMU

[wang xiaofeng]

Hello,
       I am tring to add my own SEC code base on OVMF and run on QEMU. Since the code cannot run I need to step to step trace the assembly code .
      The hang point is very early before I can use either UDK or debug serial output. I tried to use gdb to connect to QEMU.I start gdb in another terminal, and issue the following commands:
  (gdb) set architecture i386:x86-64:intel
  (gdb) target remote localhost:1234
   It really stops at the bios first instruction at 0XFFFFFFF0. But gdb shows eip= 0xFFF0 and CS=0xF000(why it not be 0xfff0). After I trigger the command "display /i $pc"
   It shows the assembly code in 0xFFF0 instead of    0XFFFFFFF0, so the information is incorrect.
   Anyone knows how to corrently debug the SEC code ? Other debug tool is also ok.
   Thanks in advance!


      

Re: Issue of step by step debugging of OVMF SEC code in QEMU

[wang xiaofeng]

HI Andrew,
     How can I adjust the debugger to correct mode? Or I have to enable the debug after swtich to protected mode?








At 2017-08-04 10:57:16, "Andrew Fish" <afish@apple.com> wrote:
>The reset vector is 16-bit real mode, so you have the debugger in the wrong mode. The code should transition to 32 bit protected early in the flow.
>
>Sent from my iPhone
>
>> On Aug 3, 2017, at 7:47 PM, wang xiaofeng <winggundum82@163.com> wrote:
>> 
>> Hello,
>>       I am tring to add my own SEC code base on OVMF and run on QEMU. Since the code cannot run I need to step to step trace the assembly code .
>>      The hang point is very early before I can use either UDK or debug serial output. I tried to use gdb to connect to QEMU.I start gdb in another terminal, and issue the following commands:
>>  (gdb) set architecture i386:x86-64:intel
>>  (gdb) target remote localhost:1234
>>   It really stops at the bios first instruction at 0XFFFFFFF0. But gdb shows eip= 0xFFF0 and CS=0xF000(why it not be 0xfff0). After I trigger the command "display /i $pc"
>>   It shows the assembly code in 0xFFF0 instead of    0XFFFFFFF0, so the information is incorrect.
>>   Anyone knows how to corrently debug the SEC code ? Other debug tool is also ok.
>>   Thanks in advance!
>> 
>> 
>> 
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
>_______________________________________________
>edk2-devel mailing list
>edk2-devel@lists.01.org
>https://lists.01.org/mailman/listinfo/edk2-devel
\x16&#65533;&

Re: Issue of step by step debugging of OVMF SEC code in QEMU

[wang xiaofeng]

Hi  Andrew,
    THe problem is solved, after the SEC code switch to protected mode, gdb can work well now








At 2017-08-04 11:26:16, "wang xiaofeng" <winggundum82@163.com> wrote:

HI Andrew,
     How can I adjust the debugger to correct mode? Or I have to enable the debug after swtich to protected mode?








At 2017-08-04 10:57:16, "Andrew Fish" <afish@apple.com> wrote:
>The reset vector is 16-bit real mode, so you have the debugger in the wrong mode. The code should transition to 32 bit protected early in the flow.
>
>Sent from my iPhone
>
>> On Aug 3, 2017, at 7:47 PM, wang xiaofeng <winggundum82@163.com> wrote:
>> 
>> Hello,
>>       I am tring to add my own SEC code base on OVMF and run on QEMU. Since the code cannot run I need to step to step trace the assembly code .
>>      The hang point is very early before I can use either UDK or debug serial output. I tried to use gdb to connect to QEMU.I start gdb in another terminal, and issue the following commands:
>>  (gdb) set architecture i386:x86-64:intel
>>  (gdb) target remote localhost:1234
>>   It really stops at the bios first instruction at 0XFFFFFFF0. But gdb shows eip= 0xFFF0 and CS=0xF000(why it not be 0xfff0). After I trigger the command "display /i $pc"
>>   It shows the assembly code in 0xFFF0 instead of    0XFFFFFFF0, so the information is incorrect.
>>   Anyone knows how to corrently debug the SEC code ? Other debug tool is also ok.
>>   Thanks in advance!
>> 
>> 
>> 
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
>_______________________________________________
>edk2-devel mailing list
>edk2-devel@lists.01.org
>https://lists.01.org/mailman/listinfo/edk2-devel





 

Re: Issue of step by step debugging of OVMF SEC code in QEMU

[Andrew Fish]

It is generally hard to debug across x86 processor mode transitions, and to debug 16-bit real mode code with modern tools. There are a few places in the x86 that still require 16-bit real mode for handoffs (like the reset vector) so you tend to hit this issue more in debugging firmware.

Thanks,

Andrew Fish


> On Aug 3, 2017, at 8:41 PM, wang xiaofeng <winggundum82@163.com> wrote:
> 
> Hi  Andrew,
>     THe problem is solved, after the SEC code switch to protected mode, gdb can work well now
> 
> 
> 
> 
> 
> 
> At 2017-08-04 11:26:16, "wang xiaofeng" <winggundum82@163.com> wrote:
> HI Andrew,
>      How can I adjust the debugger to correct mode? Or I have to enable the debug after swtich to protected mode?
> 
> 
> 
> 
> 
> 
> At 2017-08-04 10:57:16, "Andrew Fish" <afish@apple.com <mailto:afish@apple.com>> wrote:
> >The reset vector is 16-bit real mode, so you have the debugger in the wrong mode. The code should transition to 32 bit protected early in the flow.
> >
> >Sent from my iPhone
> >
> >> On Aug 3, 2017, at 7:47 PM, wang xiaofeng <winggundum82@163.com <mailto:winggundum82@163.com>> wrote:
> >> 
> >> Hello,
> >>       I am tring to add my own SEC code base on OVMF and run on QEMU. Since the code cannot run I need to step to step trace the assembly code .
> >>      The hang point is very early before I can use either UDK or debug serial output. I tried to use gdb to connect to QEMU.I start gdb in another terminal, and issue the following commands:
> >>  (gdb) set architecture i386:x86-64:intel
> >>  (gdb) target remote localhost:1234
> >>   It really stops at the bios first instruction at 0XFFFFFFF0. But gdb shows eip= 0xFFF0 and CS=0xF000(why it not be 0xfff0). After I trigger the command "display /i $pc"
> >>   It shows the assembly code in 0xFFF0 instead of    0XFFFFFFF0, so the information is incorrect.
> >>   Anyone knows how to corrently debug the SEC code ? Other debug tool is also ok.
> >>   Thanks in advance!
> >> 
> >> 
> >> 
> >> _______________________________________________
> >> edk2-devel mailing list
> >> edk2-devel@lists.01.org <mailto:edk2-devel@lists.01.org>
> >> https://lists.01.org/mailman/listinfo/edk2-devel
> >_______________________________________________
> >edk2-devel mailing list
> >edk2-devel@lists.01.org <mailto:edk2-devel@lists.01.org>
> >https://lists.01.org/mailman/listinfo/edk2-devel
> 
> 
>  
> 
> 
> 【网易自营|30天无忧退货】不到同款1折价！Tory Burch制造商美式休闲人字拖限时仅29.9元>> <http://you.163.com/item/detail?id=1185012&from=web_gg_mail_jiaobiao_9>    

Re: [PATCH v3 1/1] OvmfPkg/QemuFwCfgLib: Use BusMasterCommonBuffer to map FW_CFG_DMA_ACCESS

[Laszlo Ersek]

On 08/04/17 01:11, 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

For the future: please use version 1.1 of the agreement:

https://github.com/tianocore/edk2/commit/b6538c118ae8

> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h |  42 +--
>  OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c         | 330 ++++++++++++++++----
>  OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c         | 130 --------
>  OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPei.c         |  99 +++---
>  OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c         |  58 +---
>  5 files changed, 367 insertions(+), 292 deletions(-)
>

[lersek@redhat.com: convert pointers to UINTN before converting to UINT64]
[lersek@redhat.com: fix argument indentation in multi-line function call]
[lersek@redhat.com: explicitly compare pointers to NULL]

Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Regression-tested-by: Laszlo Ersek <lersek@redhat.com>

Pushed as commit f6c909ae5d65.

(See the fixups below, formatted with function context for easier
understanding.)

Thanks
Laszlo

> diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c
> index 576941749cf2..8b98208591e6 100644
> --- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c
> +++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c
> @@ -232,34 +232,34 @@ VOID
>  FreeFwCfgDmaAccessBuffer (
>    IN  VOID    *Access,
>    IN  VOID    *Mapping
>    )
>  {
>    UINTN       NumPages;
>    EFI_STATUS  Status;
>
>    NumPages = EFI_SIZE_TO_PAGES (sizeof (FW_CFG_DMA_ACCESS));
>
>    Status = mIoMmuProtocol->Unmap (mIoMmuProtocol, Mapping);
>    if (EFI_ERROR (Status)) {
>      DEBUG ((DEBUG_ERROR,
>        "%a:%a failed to UnMap() Mapping 0x%Lx\n", gEfiCallerBaseName,
> -      __FUNCTION__, (UINT64)Mapping));
> +      __FUNCTION__, (UINT64)(UINTN)Mapping));
>      ASSERT (FALSE);
>      CpuDeadLoop ();
>    }
>
>    Status = mIoMmuProtocol->FreeBuffer (mIoMmuProtocol, NumPages, Access);
>    if (EFI_ERROR (Status)) {
>      DEBUG ((DEBUG_ERROR,
>        "%a:%a failed to Free() 0x%Lx\n", gEfiCallerBaseName, __FUNCTION__,
> -      (UINT64) Access));
> +      (UINT64)(UINTN)Access));
>      ASSERT (FALSE);
>      CpuDeadLoop ();
>    }
>  }
>
>  /**
>    Function is used for mapping host address to device address. The buffer must
>    be unmapped with UnmapDmaDataBuffer ().
>
>  **/
> @@ -268,44 +268,44 @@ VOID
>  MapFwCfgDmaDataBuffer (
>    IN  BOOLEAN               IsWrite,
>    IN  VOID                  *HostAddress,
>    IN  UINT32                Size,
>    OUT EFI_PHYSICAL_ADDRESS  *DeviceAddress,
>    OUT VOID                  **MapInfo
>    )
>  {
>    EFI_STATUS              Status;
>    UINTN                   NumberOfBytes;
>    VOID                    *Mapping;
>    EFI_PHYSICAL_ADDRESS    PhysicalAddress;
>
>    NumberOfBytes = Size;
>    Status = mIoMmuProtocol->Map (
>                               mIoMmuProtocol,
>                               (IsWrite ?
>                                EdkiiIoMmuOperationBusMasterRead64 :
>                                EdkiiIoMmuOperationBusMasterWrite64),
>                               HostAddress,
>                               &NumberOfBytes,
>                               &PhysicalAddress,
>                               &Mapping
>                               );
>    if (EFI_ERROR (Status)) {
>      DEBUG ((DEBUG_ERROR,
>        "%a:%a failed to Map() Address 0x%Lx Size 0x%Lx\n", gEfiCallerBaseName,
> -      __FUNCTION__, (UINT64) HostAddress, (UINT64)Size));
> +      __FUNCTION__, (UINT64)(UINTN)HostAddress, (UINT64)Size));
>      ASSERT (FALSE);
>      CpuDeadLoop ();
>    }
>
>    if (NumberOfBytes < Size) {
>      mIoMmuProtocol->Unmap (mIoMmuProtocol, Mapping);
>      DEBUG ((DEBUG_ERROR,
>        "%a:%a failed to Map() - requested 0x%x got 0x%Lx\n", gEfiCallerBaseName,
>        __FUNCTION__, Size, (UINT64)NumberOfBytes));
>      ASSERT (FALSE);
>      CpuDeadLoop ();
>    }
>
>    *DeviceAddress = PhysicalAddress;
>    *MapInfo = Mapping;
>  }
> @@ -315,31 +315,31 @@ VOID
>  UnmapFwCfgDmaDataBuffer (
>    IN  VOID  *Mapping
>    )
>  {
>    EFI_STATUS  Status;
>
>    Status = mIoMmuProtocol->Unmap (mIoMmuProtocol, Mapping);
>    if (EFI_ERROR (Status)) {
>      DEBUG ((DEBUG_ERROR,
>        "%a:%a failed to UnMap() Mapping 0x%Lx\n", gEfiCallerBaseName,
> -      __FUNCTION__, (UINT64)Mapping));
> +      __FUNCTION__, (UINT64)(UINTN)Mapping));
>      ASSERT (FALSE);
>      CpuDeadLoop ();
>    }
>  }
>
>  /**
>    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.
>  **/
> @@ -347,106 +347,106 @@ 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;
>    VOID                       *AccessMapping, *DataMapping;
>    VOID                       *DataBuffer;
>
>    ASSERT (Control == FW_CFG_DMA_CTL_WRITE || Control == FW_CFG_DMA_CTL_READ ||
>      Control == FW_CFG_DMA_CTL_SKIP);
>
>    if (Size == 0) {
>      return;
>    }
>
>    Access = &LocalAccess;
>    AccessMapping = NULL;
>    DataMapping = NULL;
>    DataBuffer = Buffer;
>
>    //
>    // When SEV is enabled, map Buffer to DMA address before issuing the DMA
>    // request
>    //
>    if (MemEncryptSevIsEnabled ()) {
>      VOID                  *AccessBuffer;
>      EFI_PHYSICAL_ADDRESS  DataBufferAddress;
>
>      //
>      // Allocate DMA Access buffer
>      //
>      AllocFwCfgDmaAccessBuffer (&AccessBuffer, &AccessMapping);
>
>      Access = AccessBuffer;
>
>      //
>      // Map actual data buffer
>      //
>      if (Control != FW_CFG_DMA_CTL_SKIP) {
>        MapFwCfgDmaDataBuffer (
> -                             Control == FW_CFG_DMA_CTL_WRITE,
> -                             Buffer,
> -                             Size,
> -                             &DataBufferAddress,
> -                             &DataMapping
> -                             );
> +        Control == FW_CFG_DMA_CTL_WRITE,
> +        Buffer,
> +        Size,
> +        &DataBufferAddress,
> +        &DataMapping
> +        );
>
>        DataBuffer = (VOID *) (UINTN) DataBufferAddress;
>      }
>    }
>
>    Access->Control = SwapBytes32 (Control);
>    Access->Length  = SwapBytes32 (Size);
>    Access->Address = SwapBytes64 ((UINTN)DataBuffer);
>
>    //
>    // 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) {
> +  if (AccessMapping != NULL) {
>      FreeFwCfgDmaAccessBuffer ((VOID *)Access, AccessMapping);
>    }
>
>    //
>    // If DataBuffer was mapped then unmap it.
>    //
> -  if (DataMapping) {
> +  if (DataMapping != NULL) {
>      UnmapFwCfgDmaDataBuffer (DataMapping);
>    }
>  }

Re: [PATCH v3 0/1] Use BusMasterCommonBuffer to map FW_CFG_DMA_ACCESS

[Laszlo Ersek]

Hi Brijesh,

On 08/04/17 01:11, Brijesh Singh wrote:
> The patch applies on top of Laszlo's IoMmuDxe cleanup series [1].
> 
> Commit is also available at https://github.com/codomania/edk2/tree/qemufwcfg-sev

when pushing a new version of a series, it is best to include the
version number in the branch name, so that the branches of the old
versions are not disturbed.

(No need to do anything about it right now, just saying for the future.)

Thanks,
Laszlo

> 
> [1] https://lists.01.org/pipermail/edk2-devel/2017-August/012808.html
> 
> Changes since v2:
> * Changes to address v2 feedbacks.
> 
> Changes since v1:
> * Drop Patch 1 through 3, they are covered by Laszlo's cleanup series
> * Rebase to the latest
> 
> Brijesh Singh (1):
>   OvmfPkg/QemuFwCfgLib: Use BusMasterCommonBuffer to map
>     FW_CFG_DMA_ACCESS
> 
>  OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h |  42 +--
>  OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c         | 330 ++++++++++++++++----
>  OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c         | 130 --------
>  OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPei.c         |  99 +++---
>  OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c         |  58 +---
>  5 files changed, 367 insertions(+), 292 deletions(-)
>