[PATCH v3 0/8] OvmfPkg/VirtioNetDxe: map host address to device address

[PATCH v3 0/8] OvmfPkg/VirtioNetDxe: map host address to device address

[Brijesh Singh]

Thank you Laszlo for detail review feedbacks!

Repo: https://github.com/codomania/edk2
Branch: virtionet-3

Changes since v2:
 * changes to address v2 feedback

Brijesh Singh (8):
  OvmfPkg/VirtioNetDxe: add helper VirtioNetUninitRing()
  OvmfPkg/VirtioNetDxe: map VRINGs using VirtioRingMap()
  OvmfPkg/VirtioNetDxe: alloc RxBuf using AllocateSharedPages()
  OvmfPkg/VirtioNetDxe: dynamically alloc transmit header
  OvmfPkg/VirtioNetDxe: update TechNotes
  OvmfPkg/VirtioNetDxe: add Tx packet map/unmap helper functions
  OvmfPkg/VirtioNetDxe: map caller-supplied Tx packet to device-address
  OvmfPkg/VirtioNetDxe: negotiate VIRTIO_F_IOMMU_PLATFORM

 OvmfPkg/VirtioNetDxe/VirtioNet.inf      |   1 +
 OvmfPkg/VirtioNetDxe/VirtioNet.h        |  57 ++++-
 OvmfPkg/VirtioNetDxe/SnpGetStatus.c     |  43 +++-
 OvmfPkg/VirtioNetDxe/SnpInitialize.c    | 213 +++++++++++++---
 OvmfPkg/VirtioNetDxe/SnpReceive.c       |   5 +-
 OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c | 258 +++++++++++++++++++-
 OvmfPkg/VirtioNetDxe/SnpShutdown.c      |   4 +-
 OvmfPkg/VirtioNetDxe/SnpTransmit.c      |  27 +-
 OvmfPkg/VirtioNetDxe/TechNotes.txt      |  28 ++-
 9 files changed, 566 insertions(+), 70 deletions(-)

-- 
2.9.5

[PATCH v3 1/8] OvmfPkg/VirtioNetDxe: add helper VirtioNetUninitRing()

[Brijesh Singh]

Consolidate the virtio VRING resource cleanup into VirtioNetUninitRing().

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/VirtioNetDxe/VirtioNet.h        |  7 +++++++
 OvmfPkg/VirtioNetDxe/SnpInitialize.c    |  4 ++--
 OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c | 16 ++++++++++++++++
 OvmfPkg/VirtioNetDxe/SnpShutdown.c      |  4 ++--
 OvmfPkg/VirtioNetDxe/TechNotes.txt      |  5 +++--
 5 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/OvmfPkg/VirtioNetDxe/VirtioNet.h b/OvmfPkg/VirtioNetDxe/VirtioNet.h
index 710859bc6115..87a0f06e01a4 100644
--- a/OvmfPkg/VirtioNetDxe/VirtioNet.h
+++ b/OvmfPkg/VirtioNetDxe/VirtioNet.h
@@ -263,6 +263,13 @@ VirtioNetShutdownTx (
   IN OUT VNET_DEV *Dev
   );
 
+VOID
+EFIAPI
+VirtioNetUninitRing (
+  IN OUT VNET_DEV *Dev,
+  IN OUT VRING    *Ring
+  );
+
 //
 // event callbacks
 //
diff --git a/OvmfPkg/VirtioNetDxe/SnpInitialize.c b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
index 0ecfe044a977..637c978709fd 100644
--- a/OvmfPkg/VirtioNetDxe/SnpInitialize.c
+++ b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
@@ -510,10 +510,10 @@ AbortDevice:
   Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
 
 ReleaseTxRing:
-  VirtioRingUninit (Dev->VirtIo, &Dev->TxRing);
+  VirtioNetUninitRing (Dev, &Dev->TxRing);
 
 ReleaseRxRing:
-  VirtioRingUninit (Dev->VirtIo, &Dev->RxRing);
+  VirtioNetUninitRing (Dev, &Dev->RxRing);
 
 DeviceFailed:
   //
diff --git a/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c b/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
index 9fedb72fdbd4..5b75eabc7a6b 100644
--- a/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
+++ b/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
@@ -51,3 +51,19 @@ VirtioNetShutdownTx (
 {
   FreePool (Dev->TxFreeStack);
 }
+
+/**
+  Release TX and RX VRING resources.
+
+  @param[in,out] Dev   The VNET_DEV driver instance which was using the ring.
+  @param[in,out] Ring  The virtio ring to clean up.
+*/
+VOID
+EFIAPI
+VirtioNetUninitRing (
+  IN OUT VNET_DEV *Dev,
+  IN OUT VRING    *Ring
+  )
+{
+  VirtioRingUninit (Dev->VirtIo, Ring);
+}
diff --git a/OvmfPkg/VirtioNetDxe/SnpShutdown.c b/OvmfPkg/VirtioNetDxe/SnpShutdown.c
index 5e84191fbbdd..432e0691d457 100644
--- a/OvmfPkg/VirtioNetDxe/SnpShutdown.c
+++ b/OvmfPkg/VirtioNetDxe/SnpShutdown.c
@@ -67,8 +67,8 @@ VirtioNetShutdown (
   Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
   VirtioNetShutdownRx (Dev);
   VirtioNetShutdownTx (Dev);
-  VirtioRingUninit (Dev->VirtIo, &Dev->TxRing);
-  VirtioRingUninit (Dev->VirtIo, &Dev->RxRing);
+  VirtioNetUninitRing (Dev, &Dev->TxRing);
+  VirtioNetUninitRing (Dev, &Dev->RxRing);
 
   Dev->Snm.State = EfiSimpleNetworkStarted;
   Status = EFI_SUCCESS;
diff --git a/OvmfPkg/VirtioNetDxe/TechNotes.txt b/OvmfPkg/VirtioNetDxe/TechNotes.txt
index 9c1dfe6a773e..86b91f561495 100644
--- a/OvmfPkg/VirtioNetDxe/TechNotes.txt
+++ b/OvmfPkg/VirtioNetDxe/TechNotes.txt
@@ -70,8 +70,9 @@ faithfully indented) that implement the transition.
   VirtioNetInitialize          |  | VirtioNetShutdown
     VirtioNetInitRing {Rx, Tx} |  |   VirtioNetShutdownRx [SnpSharedHelpers.c]
       VirtioRingInit           |  |   VirtioNetShutdownTx [SnpSharedHelpers.c]
-    VirtioNetInitTx            |  |   VirtioRingUninit {Tx, Rx}
-    VirtioNetInitRx            |  |
+    VirtioNetInitTx            |  |   VirtioNetUninitRing [SnpSharedHelpers.c]
+    VirtioNetInitRx            |  |                       {Tx, Rx}
+                               |  |     VirtioRingUninit
                                v  |
                   +-----------------------------+
                   | EfiSimpleNetworkInitialized |
-- 
2.9.5

[PATCH v3 2/8] OvmfPkg/VirtioNetDxe: map VRINGs using VirtioRingMap()

[Brijesh Singh]

When device is behind the IOMMU then driver need to pass the device
address when programing the bus master. The patch uses VirtioRingMap() to
map the VRING system physical address[es] to device address[es].

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 OvmfPkg/VirtioNetDxe/VirtioNet.h        |  7 ++-
 OvmfPkg/VirtioNetDxe/SnpInitialize.c    | 50 +++++++++++++++-----
 OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c | 10 ++--
 OvmfPkg/VirtioNetDxe/SnpShutdown.c      |  4 +-
 OvmfPkg/VirtioNetDxe/TechNotes.txt      |  5 +-
 5 files changed, 57 insertions(+), 19 deletions(-)

diff --git a/OvmfPkg/VirtioNetDxe/VirtioNet.h b/OvmfPkg/VirtioNetDxe/VirtioNet.h
index 87a0f06e01a4..6762fc9d1d6e 100644
--- a/OvmfPkg/VirtioNetDxe/VirtioNet.h
+++ b/OvmfPkg/VirtioNetDxe/VirtioNet.h
@@ -82,10 +82,14 @@ typedef struct {
   EFI_HANDLE                  MacHandle;         // VirtioNetDriverBindingStart
 
   VRING                       RxRing;            // VirtioNetInitRing
+  VOID                        *RxRingMap;        // VirtioRingMap and
+                                                 // VirtioNetInitRing
   UINT8                       *RxBuf;            // VirtioNetInitRx
   UINT16                      RxLastUsed;        // VirtioNetInitRx
 
   VRING                       TxRing;            // VirtioNetInitRing
+  VOID                        *TxRingMap;        // VirtioRingMap and
+                                                 // VirtioNetInitRing
   UINT16                      TxMaxPending;      // VirtioNetInitTx
   UINT16                      TxCurPending;      // VirtioNetInitTx
   UINT16                      *TxFreeStack;      // VirtioNetInitTx
@@ -267,7 +271,8 @@ VOID
 EFIAPI
 VirtioNetUninitRing (
   IN OUT VNET_DEV *Dev,
-  IN OUT VRING    *Ring
+  IN OUT VRING    *Ring,
+  IN     VOID     *RingMap
   );
 
 //
diff --git a/OvmfPkg/VirtioNetDxe/SnpInitialize.c b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
index 637c978709fd..8eabdbff6f5e 100644
--- a/OvmfPkg/VirtioNetDxe/SnpInitialize.c
+++ b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
@@ -35,11 +35,13 @@
                            the network device.
   @param[out]    Ring      The virtio-ring inside the VNET_DEV structure,
                            corresponding to Selector.
+  @param[out]    Mapping   A resulting token to pass to VirtioNetUninitRing()
 
   @retval EFI_UNSUPPORTED  The queue size reported by the virtio-net device is
                            too small.
   @return                  Status codes from VIRTIO_CFG_WRITE(),
-                           VIRTIO_CFG_READ() and VirtioRingInit().
+                           VIRTIO_CFG_READ(), VirtioRingInit() and
+                           VirtioRingMap().
   @retval EFI_SUCCESS      Ring initialized.
 */
 
@@ -49,11 +51,14 @@ EFIAPI
 VirtioNetInitRing (
   IN OUT VNET_DEV *Dev,
   IN     UINT16   Selector,
-  OUT    VRING    *Ring
+  OUT    VRING    *Ring,
+  OUT    VOID     **Mapping
   )
 {
   EFI_STATUS Status;
   UINT16     QueueSize;
+  UINT64     RingBaseShift;
+  VOID       *MapInfo;
 
   //
   // step 4b -- allocate selected queue
@@ -80,29 +85,42 @@ VirtioNetInitRing (
   }
 
   //
+  // If anything fails from here on, we must release the ring resources.
+  //
+  Status = VirtioRingMap (Dev->VirtIo, Ring, &RingBaseShift, &MapInfo);
+  if (EFI_ERROR (Status)) {
+    goto ReleaseQueue;
+  }
+
+  //
   // Additional steps for MMIO: align the queue appropriately, and set the
-  // size. If anything fails from here on, we must release the ring resources.
+  // size. If anything fails from here on, we must unmap the ring resources.
   //
   Status = Dev->VirtIo->SetQueueNum (Dev->VirtIo, QueueSize);
   if (EFI_ERROR (Status)) {
-    goto ReleaseQueue;
+    goto UnmapQueue;
   }
 
   Status = Dev->VirtIo->SetQueueAlign (Dev->VirtIo, EFI_PAGE_SIZE);
   if (EFI_ERROR (Status)) {
-    goto ReleaseQueue;
+    goto UnmapQueue;
   }
 
   //
   // step 4c -- report GPFN (guest-physical frame number) of queue
   //
-  Status = Dev->VirtIo->SetQueueAddress (Dev->VirtIo, Ring, 0);
+  Status = Dev->VirtIo->SetQueueAddress (Dev->VirtIo, Ring, RingBaseShift);
   if (EFI_ERROR (Status)) {
-    goto ReleaseQueue;
+    goto UnmapQueue;
   }
 
+  *Mapping = MapInfo;
+
   return EFI_SUCCESS;
 
+UnmapQueue:
+  Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, MapInfo);
+
 ReleaseQueue:
   VirtioRingUninit (Dev->VirtIo, Ring);
 
@@ -456,12 +474,22 @@ VirtioNetInitialize (
   //
   // step 4b, 4c -- allocate and report virtqueues
   //
-  Status = VirtioNetInitRing (Dev, VIRTIO_NET_Q_RX, &Dev->RxRing);
+  Status = VirtioNetInitRing (
+             Dev,
+             VIRTIO_NET_Q_RX,
+             &Dev->RxRing,
+             &Dev->RxRingMap
+             );
   if (EFI_ERROR (Status)) {
     goto DeviceFailed;
   }
 
-  Status = VirtioNetInitRing (Dev, VIRTIO_NET_Q_TX, &Dev->TxRing);
+  Status = VirtioNetInitRing (
+             Dev,
+             VIRTIO_NET_Q_TX,
+             &Dev->TxRing,
+             &Dev->TxRingMap
+             );
   if (EFI_ERROR (Status)) {
     goto ReleaseRxRing;
   }
@@ -510,10 +538,10 @@ AbortDevice:
   Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
 
 ReleaseTxRing:
-  VirtioNetUninitRing (Dev, &Dev->TxRing);
+  VirtioNetUninitRing (Dev, &Dev->TxRing, Dev->TxRingMap);
 
 ReleaseRxRing:
-  VirtioNetUninitRing (Dev, &Dev->RxRing);
+  VirtioNetUninitRing (Dev, &Dev->RxRing, Dev->RxRingMap);
 
 DeviceFailed:
   //
diff --git a/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c b/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
index 5b75eabc7a6b..57c7395848bd 100644
--- a/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
+++ b/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
@@ -55,15 +55,19 @@ VirtioNetShutdownTx (
 /**
   Release TX and RX VRING resources.
 
-  @param[in,out] Dev   The VNET_DEV driver instance which was using the ring.
-  @param[in,out] Ring  The virtio ring to clean up.
+  @param[in,out] Dev       The VNET_DEV driver instance which was using
+                           the ring.
+  @param[in,out] Ring      The virtio ring to clean up.
+  @param[in]     RingMap   A token return from the VirtioRingMap()
 */
 VOID
 EFIAPI
 VirtioNetUninitRing (
   IN OUT VNET_DEV *Dev,
-  IN OUT VRING    *Ring
+  IN OUT VRING    *Ring,
+  IN     VOID     *RingMap
   )
 {
+  Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, RingMap);
   VirtioRingUninit (Dev->VirtIo, Ring);
 }
diff --git a/OvmfPkg/VirtioNetDxe/SnpShutdown.c b/OvmfPkg/VirtioNetDxe/SnpShutdown.c
index 432e0691d457..d8c11f20de61 100644
--- a/OvmfPkg/VirtioNetDxe/SnpShutdown.c
+++ b/OvmfPkg/VirtioNetDxe/SnpShutdown.c
@@ -67,8 +67,8 @@ VirtioNetShutdown (
   Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
   VirtioNetShutdownRx (Dev);
   VirtioNetShutdownTx (Dev);
-  VirtioNetUninitRing (Dev, &Dev->TxRing);
-  VirtioNetUninitRing (Dev, &Dev->RxRing);
+  VirtioNetUninitRing (Dev, &Dev->TxRing, Dev->TxRingMap);
+  VirtioNetUninitRing (Dev, &Dev->RxRing, Dev->RxRingMap);
 
   Dev->Snm.State = EfiSimpleNetworkStarted;
   Status = EFI_SUCCESS;
diff --git a/OvmfPkg/VirtioNetDxe/TechNotes.txt b/OvmfPkg/VirtioNetDxe/TechNotes.txt
index 86b91f561495..37250b14a98c 100644
--- a/OvmfPkg/VirtioNetDxe/TechNotes.txt
+++ b/OvmfPkg/VirtioNetDxe/TechNotes.txt
@@ -70,8 +70,9 @@ faithfully indented) that implement the transition.
   VirtioNetInitialize          |  | VirtioNetShutdown
     VirtioNetInitRing {Rx, Tx} |  |   VirtioNetShutdownRx [SnpSharedHelpers.c]
       VirtioRingInit           |  |   VirtioNetShutdownTx [SnpSharedHelpers.c]
-    VirtioNetInitTx            |  |   VirtioNetUninitRing [SnpSharedHelpers.c]
-    VirtioNetInitRx            |  |                       {Tx, Rx}
+      VirtioRingMap            |  |   VirtioNetUninitRing [SnpSharedHelpers.c]
+    VirtioNetInitTx            |  |                       {Tx, Rx}
+    VirtioNetInitRx            |  |     VirtIo->UnmapSharedBuffer
                                |  |     VirtioRingUninit
                                v  |
                   +-----------------------------+
-- 
2.9.5

[PATCH v3 3/8] OvmfPkg/VirtioNetDxe: alloc RxBuf using AllocateSharedPages()

[Brijesh Singh]

When device is behind the IOMMU, VirtioNetDxe is required to use the
device address in bus master operations. RxBuf is allocated using
AllocatePool() which returns the system physical address.

The patch uses VIRTIO_DEVICE_PROTOCOL.AllocateSharedPages() to allocate
the RxBuf and map with VirtioMapAllBytesInSharedBuffer() so that we can
obtain the device address for RxBuf.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 OvmfPkg/VirtioNetDxe/VirtioNet.h        |  4 +
 OvmfPkg/VirtioNetDxe/SnpInitialize.c    | 78 +++++++++++++++-----
 OvmfPkg/VirtioNetDxe/SnpReceive.c       |  5 +-
 OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c |  7 +-
 OvmfPkg/VirtioNetDxe/TechNotes.txt      |  2 +-
 5 files changed, 75 insertions(+), 21 deletions(-)

diff --git a/OvmfPkg/VirtioNetDxe/VirtioNet.h b/OvmfPkg/VirtioNetDxe/VirtioNet.h
index 6762fc9d1d6e..995593f4b236 100644
--- a/OvmfPkg/VirtioNetDxe/VirtioNet.h
+++ b/OvmfPkg/VirtioNetDxe/VirtioNet.h
@@ -4,6 +4,7 @@
   Protocol instances for virtio-net devices.
 
   Copyright (C) 2013, Red Hat, Inc.
+  Copyright (c) 2017, AMD Inc, All rights reserved.<BR>
 
   This program and the accompanying materials are licensed and made available
   under the terms and conditions of the BSD License which accompanies this
@@ -86,6 +87,9 @@ typedef struct {
                                                  // VirtioNetInitRing
   UINT8                       *RxBuf;            // VirtioNetInitRx
   UINT16                      RxLastUsed;        // VirtioNetInitRx
+  UINTN                       RxBufNrPages;      // VirtioNetInitRx
+  EFI_PHYSICAL_ADDRESS        RxBufDeviceBase;   // VirtioNetInitRx
+  VOID                        *RxBufMap;         // VirtioNetInitRx
 
   VRING                       TxRing;            // VirtioNetInitRing
   VOID                        *TxRingMap;        // VirtioRingMap and
diff --git a/OvmfPkg/VirtioNetDxe/SnpInitialize.c b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
index 8eabdbff6f5e..b739875d4c01 100644
--- a/OvmfPkg/VirtioNetDxe/SnpInitialize.c
+++ b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
@@ -242,8 +242,9 @@ VirtioNetInitTx (
   @param[in,out] Dev       The VNET_DEV driver instance about to enter the
                            EfiSimpleNetworkInitialized state.
 
-  @retval EFI_OUT_OF_RESOURCES  Failed to allocate RX destination area.
-  @return                       Status codes from VIRTIO_CFG_WRITE().
+  @return                       Status codes from VIRTIO_CFG_WRITE() or
+                                VIRTIO_DEVICE_PROTOCOL.AllocateSharedPages or
+                                VirtioMapAllBytesInSharedBuffer().
   @retval EFI_SUCCESS           RX setup successful. The device is live and may
                                 already be writing to the receive area.
 */
@@ -255,13 +256,15 @@ VirtioNetInitRx (
   IN OUT VNET_DEV *Dev
   )
 {
-  EFI_STATUS Status;
-  UINTN      VirtioNetReqSize;
-  UINTN      RxBufSize;
-  UINT16     RxAlwaysPending;
-  UINTN      PktIdx;
-  UINT16     DescIdx;
-  UINT8      *RxPtr;
+  EFI_STATUS            Status;
+  UINTN                 VirtioNetReqSize;
+  UINTN                 RxBufSize;
+  UINT16                RxAlwaysPending;
+  UINTN                 PktIdx;
+  UINT16                DescIdx;
+  UINTN                 NumBytes;
+  EFI_PHYSICAL_ADDRESS  RxBufDeviceAddress;
+  VOID                  *RxBuffer;
 
   //
   // In VirtIo 1.0, the NumBuffers field is mandatory. In 0.9.5, it depends on
@@ -286,11 +289,39 @@ VirtioNetInitRx (
   //
   RxAlwaysPending = (UINT16) MIN (Dev->RxRing.QueueSize / 2, VNET_MAX_PENDING);
 
-  Dev->RxBuf = AllocatePool (RxAlwaysPending * RxBufSize);
-  if (Dev->RxBuf == NULL) {
-    return EFI_OUT_OF_RESOURCES;
+  //
+  // The RxBuf is shared between guest and hypervisor, use
+  // AllocateSharedPages() to allocate this memory region and map it with
+  // BusMasterCommonBuffer so that it can be accessed by both guest and
+  // hypervisor.
+  //
+  NumBytes = RxAlwaysPending * RxBufSize;
+  Dev->RxBufNrPages = EFI_SIZE_TO_PAGES (NumBytes);
+  Status = Dev->VirtIo->AllocateSharedPages (
+                          Dev->VirtIo,
+                          Dev->RxBufNrPages,
+                          &RxBuffer
+                          );
+  if (EFI_ERROR (Status)) {
+    return Status;
   }
 
+  ZeroMem (RxBuffer, NumBytes);
+
+  Status = VirtioMapAllBytesInSharedBuffer (
+             Dev->VirtIo,
+             VirtioOperationBusMasterCommonBuffer,
+             RxBuffer,
+             NumBytes,
+             &Dev->RxBufDeviceBase,
+             &Dev->RxBufMap
+             );
+  if (EFI_ERROR (Status)) {
+    goto FreeSharedBuffer;
+  }
+
+  Dev->RxBuf = RxBuffer;
+
   //
   // virtio-0.9.5, 2.4.2 Receiving Used Buffers From the Device
   //
@@ -310,7 +341,7 @@ VirtioNetInitRx (
   // link each chain into (from) the available ring as well
   //
   DescIdx = 0;
-  RxPtr = Dev->RxBuf;
+  RxBufDeviceAddress = Dev->RxBufDeviceBase;
   for (PktIdx = 0; PktIdx < RxAlwaysPending; ++PktIdx) {
     //
     // virtio-0.9.5, 2.4.1.2 Updating the Available Ring
@@ -321,16 +352,16 @@ VirtioNetInitRx (
     //
     // virtio-0.9.5, 2.4.1.1 Placing Buffers into the Descriptor Table
     //
-    Dev->RxRing.Desc[DescIdx].Addr  = (UINTN) RxPtr;
+    Dev->RxRing.Desc[DescIdx].Addr  = RxBufDeviceAddress;
     Dev->RxRing.Desc[DescIdx].Len   = (UINT32) VirtioNetReqSize;
     Dev->RxRing.Desc[DescIdx].Flags = VRING_DESC_F_WRITE | VRING_DESC_F_NEXT;
     Dev->RxRing.Desc[DescIdx].Next  = (UINT16) (DescIdx + 1);
-    RxPtr += Dev->RxRing.Desc[DescIdx++].Len;
+    RxBufDeviceAddress += Dev->RxRing.Desc[DescIdx++].Len;
 
-    Dev->RxRing.Desc[DescIdx].Addr  = (UINTN) RxPtr;
+    Dev->RxRing.Desc[DescIdx].Addr  = RxBufDeviceAddress;
     Dev->RxRing.Desc[DescIdx].Len   = (UINT32) (RxBufSize - VirtioNetReqSize);
     Dev->RxRing.Desc[DescIdx].Flags = VRING_DESC_F_WRITE;
-    RxPtr += Dev->RxRing.Desc[DescIdx++].Len;
+    RxBufDeviceAddress += Dev->RxRing.Desc[DescIdx++].Len;
   }
 
   //
@@ -351,10 +382,21 @@ VirtioNetInitRx (
   Status = Dev->VirtIo->SetQueueNotify (Dev->VirtIo, VIRTIO_NET_Q_RX);
   if (EFI_ERROR (Status)) {
     Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
-    FreePool (Dev->RxBuf);
+    goto UnmapSharedBuffer;
   }
 
   return Status;
+
+UnmapSharedBuffer:
+  Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RxBufMap);
+
+FreeSharedBuffer:
+  Dev->VirtIo->FreeSharedPages (
+                 Dev->VirtIo,
+                 Dev->RxBufNrPages,
+                 RxBuffer
+                 );
+  return Status;
 }
 
 
diff --git a/OvmfPkg/VirtioNetDxe/SnpReceive.c b/OvmfPkg/VirtioNetDxe/SnpReceive.c
index 99abd7ebe454..c42489636ea0 100644
--- a/OvmfPkg/VirtioNetDxe/SnpReceive.c
+++ b/OvmfPkg/VirtioNetDxe/SnpReceive.c
@@ -82,6 +82,7 @@ VirtioNetReceive (
   UINT8      *RxPtr;
   UINT16     AvailIdx;
   EFI_STATUS NotifyStatus;
+  UINTN      RxBufOffset;
 
   if (This == NULL || BufferSize == NULL || Buffer == NULL) {
     return EFI_INVALID_PARAMETER;
@@ -143,7 +144,9 @@ VirtioNetReceive (
     *HeaderSize = Dev->Snm.MediaHeaderSize;
   }
 
-  RxPtr = (UINT8 *)(UINTN) Dev->RxRing.Desc[DescIdx + 1].Addr;
+  RxBufOffset = (UINTN)(Dev->RxRing.Desc[DescIdx + 1].Addr -
+                        Dev->RxBufDeviceBase);
+  RxPtr = Dev->RxBuf + RxBufOffset;
   CopyMem (Buffer, RxPtr, RxLen);
 
   if (DestAddr != NULL) {
diff --git a/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c b/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
index 57c7395848bd..ee4f9ed36ecd 100644
--- a/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
+++ b/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
@@ -39,7 +39,12 @@ VirtioNetShutdownRx (
   IN OUT VNET_DEV *Dev
   )
 {
-  FreePool (Dev->RxBuf);
+  Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RxBufMap);
+  Dev->VirtIo->FreeSharedPages (
+                 Dev->VirtIo,
+                 Dev->RxBufNrPages,
+                 Dev->RxBuf
+                 );
 }
 
 
diff --git a/OvmfPkg/VirtioNetDxe/TechNotes.txt b/OvmfPkg/VirtioNetDxe/TechNotes.txt
index 37250b14a98c..fedbaee07ec7 100644
--- a/OvmfPkg/VirtioNetDxe/TechNotes.txt
+++ b/OvmfPkg/VirtioNetDxe/TechNotes.txt
@@ -247,7 +247,7 @@ In VirtioNetInitRx, the guest allocates the fixed size Receive Destination
 Area, which accommodates all packets delivered asynchronously by the host. To
 each packet, a slice of this area is dedicated; each slice is further
 subdivided into virtio-net request header and network packet data. The
-(guest-physical) addresses of these sub-slices are denoted with A2, A3, A4 and
+(device-physical) addresses of these sub-slices are denoted with A2, A3, A4 and
 so on. Importantly, an even-subscript "A" always belongs to a virtio-net
 request header, while an odd-subscript "A" always belongs to a packet
 sub-slice.
-- 
2.9.5

[PATCH v3 4/8] OvmfPkg/VirtioNetDxe: dynamically alloc transmit header

[Brijesh Singh]

Each network packet is submitted for transmission by pushing the head
descriptor of a two-part descriptor chain to the Available Ring of the
TX queue. VirtioNetInitTx() sets up the the descriptor chains for all
queueable packets in advance, and points all the head descriptors to the
same shared, never modified, VIRTIO_1_0_NET_REQ header object (or its
initial VIRTIO_NET_REQ sub-object, dependent on virtio version).
VirtioNetInitTx() currently uses the header object's system physical
address for populating the head descriptors.

When device is behind the IOMMU, VirtioNet driver is required to provide
the device address of VIRTIO_1_0_NET_REQ header. In this patch we
dynamically allocate the header using AllocateSharedPages() and map with
BusMasterCommonBuffer so that header can be accessed by both processor
and the device.

We map the header object for CommonBuffer operation because, in order to
stick with the current code order, we populate the head descriptors with
the header's device address first, and fill in the header itself second.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 OvmfPkg/VirtioNetDxe/VirtioNet.h        |  3 +-
 OvmfPkg/VirtioNetDxe/SnpInitialize.c    | 64 +++++++++++++++++---
 OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c |  7 +++
 3 files changed, 65 insertions(+), 9 deletions(-)

diff --git a/OvmfPkg/VirtioNetDxe/VirtioNet.h b/OvmfPkg/VirtioNetDxe/VirtioNet.h
index 995593f4b236..027f75993e8e 100644
--- a/OvmfPkg/VirtioNetDxe/VirtioNet.h
+++ b/OvmfPkg/VirtioNetDxe/VirtioNet.h
@@ -97,7 +97,8 @@ typedef struct {
   UINT16                      TxMaxPending;      // VirtioNetInitTx
   UINT16                      TxCurPending;      // VirtioNetInitTx
   UINT16                      *TxFreeStack;      // VirtioNetInitTx
-  VIRTIO_1_0_NET_REQ          TxSharedReq;       // VirtioNetInitTx
+  VIRTIO_1_0_NET_REQ          *TxSharedReq;      // VirtioNetInitTx
+  VOID                        *TxSharedReqMap;   // VirtioNetInitTx
   UINT16                      TxLastUsed;        // VirtioNetInitTx
 } VNET_DEV;
 
diff --git a/OvmfPkg/VirtioNetDxe/SnpInitialize.c b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
index b739875d4c01..9621f936d2cb 100644
--- a/OvmfPkg/VirtioNetDxe/SnpInitialize.c
+++ b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
@@ -18,6 +18,7 @@
 **/
 
 #include <Library/BaseLib.h>
+#include <Library/BaseMemoryLib.h>
 #include <Library/MemoryAllocationLib.h>
 #include <Library/UefiBootServicesTableLib.h>
 
@@ -147,6 +148,9 @@ ReleaseQueue:
 
   @retval EFI_OUT_OF_RESOURCES  Failed to allocate the stack to track the heads
                                 of free descriptor chains.
+  @return                       Status codes from VIRTIO_DEVICE_PROTOCOL.
+                                AllocateSharedPages() or
+                                VirtioMapAllBytesInSharedBuffer()
   @retval EFI_SUCCESS           TX setup successful.
 */
 
@@ -157,8 +161,11 @@ VirtioNetInitTx (
   IN OUT VNET_DEV *Dev
   )
 {
-  UINTN TxSharedReqSize;
-  UINTN PktIdx;
+  UINTN                 TxSharedReqSize;
+  UINTN                 PktIdx;
+  EFI_STATUS            Status;
+  EFI_PHYSICAL_ADDRESS  DeviceAddress;
+  VOID                  *TxSharedReqBuffer;
 
   Dev->TxMaxPending = (UINT16) MIN (Dev->TxRing.QueueSize / 2,
                                  VNET_MAX_PENDING);
@@ -170,12 +177,42 @@ VirtioNetInitTx (
   }
 
   //
+  // Allocate TxSharedReq header and map with BusMasterCommonBuffer so that it
+  // can be accessed equally by both processor and device.
+  //
+  Status = Dev->VirtIo->AllocateSharedPages (
+                          Dev->VirtIo,
+                          EFI_SIZE_TO_PAGES (sizeof *Dev->TxSharedReq),
+                          &TxSharedReqBuffer
+                          );
+  if (EFI_ERROR (Status)) {
+    goto FreeTxFreeStack;
+  }
+
+  ZeroMem (TxSharedReqBuffer, sizeof *Dev->TxSharedReq);
+
+  Status = VirtioMapAllBytesInSharedBuffer (
+             Dev->VirtIo,
+             VirtioOperationBusMasterCommonBuffer,
+             TxSharedReqBuffer,
+             sizeof *(Dev->TxSharedReq),
+             &DeviceAddress,
+             &Dev->TxSharedReqMap
+             );
+  if (EFI_ERROR (Status)) {
+    goto FreeTxSharedReqBuffer;
+  }
+
+  Dev->TxSharedReq = TxSharedReqBuffer;
+
+
+  //
   // In VirtIo 1.0, the NumBuffers field is mandatory. In 0.9.5, it depends on
   // VIRTIO_NET_F_MRG_RXBUF, which we never negotiate.
   //
   TxSharedReqSize = (Dev->VirtIo->Revision < VIRTIO_SPEC_REVISION (1, 0, 0)) ?
-                    sizeof Dev->TxSharedReq.V0_9_5 :
-                    sizeof Dev->TxSharedReq;
+                    sizeof (Dev->TxSharedReq->V0_9_5) :
+                    sizeof *Dev->TxSharedReq;
 
   for (PktIdx = 0; PktIdx < Dev->TxMaxPending; ++PktIdx) {
     UINT16 DescIdx;
@@ -187,7 +224,7 @@ VirtioNetInitTx (
     // For each possibly pending packet, lay out the descriptor for the common
     // (unmodified by the host) virtio-net request header.
     //
-    Dev->TxRing.Desc[DescIdx].Addr  = (UINTN) &Dev->TxSharedReq;
+    Dev->TxRing.Desc[DescIdx].Addr  = DeviceAddress;
     Dev->TxRing.Desc[DescIdx].Len   = (UINT32) TxSharedReqSize;
     Dev->TxRing.Desc[DescIdx].Flags = VRING_DESC_F_NEXT;
     Dev->TxRing.Desc[DescIdx].Next  = (UINT16) (DescIdx + 1);
@@ -202,13 +239,13 @@ VirtioNetInitTx (
   //
   // virtio-0.9.5, Appendix C, Packet Transmission
   //
-  Dev->TxSharedReq.V0_9_5.Flags   = 0;
-  Dev->TxSharedReq.V0_9_5.GsoType = VIRTIO_NET_HDR_GSO_NONE;
+  Dev->TxSharedReq->V0_9_5.Flags   = 0;
+  Dev->TxSharedReq->V0_9_5.GsoType = VIRTIO_NET_HDR_GSO_NONE;
 
   //
   // For VirtIo 1.0 only -- the field exists, but it is unused
   //
-  Dev->TxSharedReq.NumBuffers = 0;
+  Dev->TxSharedReq->NumBuffers = 0;
 
   //
   // virtio-0.9.5, 2.4.2 Receiving Used Buffers From the Device
@@ -223,6 +260,17 @@ VirtioNetInitTx (
   *Dev->TxRing.Avail.Flags = (UINT16) VRING_AVAIL_F_NO_INTERRUPT;
 
   return EFI_SUCCESS;
+
+FreeTxSharedReqBuffer:
+  Dev->VirtIo->FreeSharedPages (
+                 Dev->VirtIo,
+                 EFI_SIZE_TO_PAGES (sizeof *(Dev->TxSharedReq)),
+                 TxSharedReqBuffer
+                 );
+FreeTxFreeStack:
+  FreePool (Dev->TxFreeStack);
+
+  return Status;
 }
 
 
diff --git a/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c b/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
index ee4f9ed36ecd..2fce8142d554 100644
--- a/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
+++ b/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
@@ -54,6 +54,13 @@ VirtioNetShutdownTx (
   IN OUT VNET_DEV *Dev
   )
 {
+  Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->TxSharedReqMap);
+  Dev->VirtIo->FreeSharedPages (
+                 Dev->VirtIo,
+                 EFI_SIZE_TO_PAGES (sizeof *(Dev->TxSharedReq)),
+                 Dev->TxSharedReq
+                 );
+
   FreePool (Dev->TxFreeStack);
 }
 
-- 
2.9.5

[PATCH v3 5/8] OvmfPkg/VirtioNetDxe: update TechNotes

[Brijesh Singh]

In next patches we will update Virtio transmit to use the device-mapped
address of the caller-supplied packet. The patch documents the new model.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/VirtioNetDxe/TechNotes.txt | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/OvmfPkg/VirtioNetDxe/TechNotes.txt b/OvmfPkg/VirtioNetDxe/TechNotes.txt
index fedbaee07ec7..40a22f66dc22 100644
--- a/OvmfPkg/VirtioNetDxe/TechNotes.txt
+++ b/OvmfPkg/VirtioNetDxe/TechNotes.txt
@@ -312,10 +312,14 @@ in the following:
   that is shared by all of the head descriptors. This virtio-net request header
   is never modified by the host.
 
-- Each tail descriptor is re-pointed to the caller-supplied packet buffer
-  whenever VirtioNetTransmit places the corresponding head descriptor on the
-  Available Ring. The caller is responsible to hang on to the unmodified buffer
-  until it is reported transmitted by VirtioNetGetStatus.
+- Each tail descriptor is re-pointed to the device-mapped address of the
+  caller-supplied packet buffer whenever VirtioNetTransmit places the
+  corresponding head descriptor on the Available Ring. A reverse mapping, from
+  the device-mapped address to the caller-supplied packet address, is saved in
+  an associative data structure that belongs to the driver instance.
+
+- Per spec, the caller is responsible to hang on to the unmodified packet
+  buffer until it is reported transmitted by VirtioNetGetStatus.
 
 Steps of packet transmission:
 
@@ -338,9 +342,11 @@ Steps of packet transmission:
 - Client code calls VirtioNetGetStatus. In case the Used Ring is empty, the
   function reports no Tx completion. Otherwise, a head descriptor's index is
   consumed from the Used Ring and recycled to the private stack. The client
-  code's original packet buffer address is fetched from the tail descriptor
-  (where it has been stored at VirtioNetTransmit time) and returned to the
-  caller.
+  code's original packet buffer address is calculated by fetching the
+  device-mapped address from the tail descriptor (where it has been stored at
+  VirtioNetTransmit time), and by looking up the device-mapped address in the
+  associative data structure. The reverse-mapped packet buffer address is
+  returned to the caller.
 
 - The Len field of the Used Ring Element is not checked. The host is assumed to
   have transmitted the entire packet -- VirtioNetTransmit had forced it below
-- 
2.9.5

[PATCH v3 6/8] OvmfPkg/VirtioNetDxe: add Tx packet map/unmap helper functions

[Brijesh Singh]

When device is behind IOMMU, driver is require to pass the device address
of TxBuf in the Tx VRING. The patch adds helper functions and data
structure to map and unmap the TxBuf system physical address to a device
address.

Since the TxBuf is returned back to caller from VirtioNetGetStatus() hence
we use OrderedCollection interface to save the TxBuf system physical to
device address mapping. After the TxBuf is succesfully transmitted
VirtioNetUnmapTxBuf() does the reverse lookup in OrderedCollection data
structure to get the system physical address of TxBuf for a given device
address.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 OvmfPkg/VirtioNetDxe/VirtioNet.inf      |   1 +
 OvmfPkg/VirtioNetDxe/VirtioNet.h        |  38 ++++
 OvmfPkg/VirtioNetDxe/SnpInitialize.c    |  18 +-
 OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c | 224 ++++++++++++++++++++
 4 files changed, 279 insertions(+), 2 deletions(-)

diff --git a/OvmfPkg/VirtioNetDxe/VirtioNet.inf b/OvmfPkg/VirtioNetDxe/VirtioNet.inf
index a855ad4ac154..9ff6d87e6190 100644
--- a/OvmfPkg/VirtioNetDxe/VirtioNet.inf
+++ b/OvmfPkg/VirtioNetDxe/VirtioNet.inf
@@ -49,6 +49,7 @@
   DebugLib
   DevicePathLib
   MemoryAllocationLib
+  OrderedCollectionLib
   UefiBootServicesTableLib
   UefiDriverEntryPoint
   UefiLib
diff --git a/OvmfPkg/VirtioNetDxe/VirtioNet.h b/OvmfPkg/VirtioNetDxe/VirtioNet.h
index 027f75993e8e..3fc88cfb790e 100644
--- a/OvmfPkg/VirtioNetDxe/VirtioNet.h
+++ b/OvmfPkg/VirtioNetDxe/VirtioNet.h
@@ -26,6 +26,7 @@
 #include <Protocol/DevicePath.h>
 #include <Protocol/DriverBinding.h>
 #include <Protocol/SimpleNetwork.h>
+#include <Library/OrderedCollectionLib.h>
 
 #define VNET_SIG SIGNATURE_32 ('V', 'N', 'E', 'T')
 
@@ -100,6 +101,7 @@ typedef struct {
   VIRTIO_1_0_NET_REQ          *TxSharedReq;      // VirtioNetInitTx
   VOID                        *TxSharedReqMap;   // VirtioNetInitTx
   UINT16                      TxLastUsed;        // VirtioNetInitTx
+  ORDERED_COLLECTION          *TxBufCollection;  // VirtioNetInitTx
 } VNET_DEV;
 
 
@@ -281,6 +283,42 @@ VirtioNetUninitRing (
   );
 
 //
+// utility functions to map caller-supplied Tx buffer system physical address
+// to a device address and vice versa
+//
+EFI_STATUS
+EFIAPI
+VirtioNetMapTxBuf (
+  IN  VNET_DEV              *Dev,
+  IN  VOID                  *Buffer,
+  IN  UINTN                 NumberOfBytes,
+  OUT EFI_PHYSICAL_ADDRESS  *DeviceAddress
+  );
+
+EFI_STATUS
+EFIAPI
+VirtioNetUnmapTxBuf (
+  IN  VNET_DEV              *Dev,
+  OUT VOID                  **Buffer,
+  IN  EFI_PHYSICAL_ADDRESS  DeviceAddress
+  );
+
+INTN
+EFIAPI
+VirtioNetTxBufMapInfoCompare (
+  IN CONST VOID *UserStruct1,
+  IN CONST VOID *UserStruct2
+  );
+
+INTN
+EFIAPI
+VirtioNetTxBufDeviceAddressCompare (
+  IN CONST VOID *StandaloneKey,
+  IN CONST VOID *UserStruct
+  );
+
+
+//
 // event callbacks
 //
 VOID
diff --git a/OvmfPkg/VirtioNetDxe/SnpInitialize.c b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
index 9621f936d2cb..ffb3deefe00c 100644
--- a/OvmfPkg/VirtioNetDxe/SnpInitialize.c
+++ b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
@@ -147,7 +147,8 @@ ReleaseQueue:
                            EfiSimpleNetworkInitialized state.
 
   @retval EFI_OUT_OF_RESOURCES  Failed to allocate the stack to track the heads
-                                of free descriptor chains.
+                                of free descriptor chains or failed to init
+                                TxBufCollection.
   @return                       Status codes from VIRTIO_DEVICE_PROTOCOL.
                                 AllocateSharedPages() or
                                 VirtioMapAllBytesInSharedBuffer()
@@ -176,6 +177,15 @@ VirtioNetInitTx (
     return EFI_OUT_OF_RESOURCES;
   }
 
+  Dev->TxBufCollection = OrderedCollectionInit (
+                           VirtioNetTxBufMapInfoCompare,
+                           VirtioNetTxBufDeviceAddressCompare
+                           );
+  if (Dev->TxBufCollection == NULL) {
+    Status = EFI_OUT_OF_RESOURCES;
+    goto FreeTxFreeStack;
+  }
+
   //
   // Allocate TxSharedReq header and map with BusMasterCommonBuffer so that it
   // can be accessed equally by both processor and device.
@@ -186,7 +196,7 @@ VirtioNetInitTx (
                           &TxSharedReqBuffer
                           );
   if (EFI_ERROR (Status)) {
-    goto FreeTxFreeStack;
+    goto UninitTxBufCollection;
   }
 
   ZeroMem (TxSharedReqBuffer, sizeof *Dev->TxSharedReq);
@@ -267,6 +277,10 @@ FreeTxSharedReqBuffer:
                  EFI_SIZE_TO_PAGES (sizeof *(Dev->TxSharedReq)),
                  TxSharedReqBuffer
                  );
+
+UninitTxBufCollection:
+  OrderedCollectionUninit (Dev->TxBufCollection);
+
 FreeTxFreeStack:
   FreePool (Dev->TxFreeStack);
 
diff --git a/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c b/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
index 2fce8142d554..18dbf1812541 100644
--- a/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
+++ b/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
@@ -18,6 +18,16 @@
 
 #include "VirtioNet.h"
 
+//
+// The user structure for the ordered collection that will track the mapping
+// info of the packets queued in TxRing
+//
+typedef struct {
+  VOID                  *Buffer;
+  EFI_PHYSICAL_ADDRESS  DeviceAddress;  // lookup key for reverse mapping
+  VOID                  *BufMap;
+} TX_BUF_MAP_INFO;
+
 /**
   Release RX and TX resources on the boundary of the
   EfiSimpleNetworkInitialized state.
@@ -54,6 +64,10 @@ VirtioNetShutdownTx (
   IN OUT VNET_DEV *Dev
   )
 {
+  ORDERED_COLLECTION_ENTRY *Entry, *Entry2;
+  TX_BUF_MAP_INFO          *TxBufMapInfo;
+  VOID                     *UserStruct;
+
   Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->TxSharedReqMap);
   Dev->VirtIo->FreeSharedPages (
                  Dev->VirtIo,
@@ -61,6 +75,17 @@ VirtioNetShutdownTx (
                  Dev->TxSharedReq
                  );
 
+  for (Entry = OrderedCollectionMin (Dev->TxBufCollection);
+       Entry != NULL;
+       Entry = Entry2) {
+    Entry2 = OrderedCollectionNext (Entry);
+    OrderedCollectionDelete (Dev->TxBufCollection, Entry, &UserStruct);
+    TxBufMapInfo = UserStruct;
+    Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, TxBufMapInfo->BufMap);
+    FreePool (TxBufMapInfo);
+  }
+  OrderedCollectionUninit (Dev->TxBufCollection);
+
   FreePool (Dev->TxFreeStack);
 }
 
@@ -83,3 +108,202 @@ VirtioNetUninitRing (
   Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, RingMap);
   VirtioRingUninit (Dev->VirtIo, Ring);
 }
+
+
+/**
+  Map Caller-supplied TxBuf buffer to the device-mapped address
+
+  @param[in]    Dev               The VNET_DEV driver instance which wants to
+                                  map the Tx packet.
+  @param[in]    Buffer            The system physical address of TxBuf
+  @param[in]    NumberOfBytes     Number of bytes to map
+  @param[out]   DeviceAddress     The resulting device address for the bus
+                                  master access.
+
+  @retval EFI_OUT_OF_RESOURCES    The request could not be completed due to
+                                  a lack of resources.
+  @return                         Status codes from
+                                  VirtioMapAllBytesInSharedBuffer()
+  @retval EFI_SUCCESS             Caller-supplied buffer is succesfully mapped.
+*/
+EFI_STATUS
+EFIAPI
+VirtioNetMapTxBuf (
+  IN  VNET_DEV              *Dev,
+  IN  VOID                  *Buffer,
+  IN  UINTN                 NumberOfBytes,
+  OUT EFI_PHYSICAL_ADDRESS  *DeviceAddress
+  )
+{
+  EFI_STATUS                Status;
+  TX_BUF_MAP_INFO           *TxBufMapInfo;
+  EFI_PHYSICAL_ADDRESS      Address;
+  VOID                      *Mapping;
+
+  TxBufMapInfo = AllocatePool (sizeof (*TxBufMapInfo));
+  if (TxBufMapInfo == NULL) {
+    return EFI_OUT_OF_RESOURCES;
+  }
+
+  Status = VirtioMapAllBytesInSharedBuffer (
+             Dev->VirtIo,
+             VirtioOperationBusMasterRead,
+             Buffer,
+             NumberOfBytes,
+             &Address,
+             &Mapping
+            );
+  if (EFI_ERROR (Status)) {
+    goto FreeTxBufMapInfo;
+  }
+
+  TxBufMapInfo->Buffer = Buffer;
+  TxBufMapInfo->DeviceAddress = Address;
+  TxBufMapInfo->BufMap = Mapping;
+
+  Status = OrderedCollectionInsert (
+             Dev->TxBufCollection,
+             NULL,
+             TxBufMapInfo
+             );
+  switch (Status) {
+  case EFI_OUT_OF_RESOURCES:
+    goto UnmapTxBuf;
+  case EFI_ALREADY_STARTED:
+    //
+    // This should never happen: it implies
+    //
+    // - an identity-mapping VIRTIO_DEVICE_PROTOCOL.MapSharedBuffer()
+    //   implementation -- which is fine,
+    //
+    // - and an SNP client that queues multiple instances of the exact same
+    //   buffer address with SNP.Transmit() -- which is undefined behavior,
+    //   based on the TxBuf language in UEFI-2.7,
+    //   EFI_SIMPLE_NETWORK.GetStatus().
+    //
+    ASSERT (FALSE);
+    Status = EFI_INVALID_PARAMETER;
+    goto UnmapTxBuf;
+  default:
+    ASSERT_EFI_ERROR (Status);
+    break;
+  }
+
+  *DeviceAddress = Address;
+  return EFI_SUCCESS;
+
+UnmapTxBuf:
+  Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Mapping);
+
+FreeTxBufMapInfo:
+  FreePool (TxBufMapInfo);
+  return Status;
+}
+
+/**
+  Unmap (aka reverse mapping) device mapped TxBuf buffer to the system
+  physical address
+
+  @param[in]    Dev               The VNET_DEV driver instance which wants to
+                                  reverse- and unmap the Tx packet.
+  @param[out]   Buffer            The system physical address of TxBuf
+  @param[in]    DeviceAddress     The device address for the TxBuf
+
+  @retval EFI_INVALID_PARAMETER   The DeviceAddress is not mapped
+  @retval EFI_SUCCESS             The TxBuf at DeviceAddress has been unmapped,
+                                  and Buffer has been set to TxBuf's system
+                                  physical address.
+
+*/
+EFI_STATUS
+EFIAPI
+VirtioNetUnmapTxBuf (
+  IN  VNET_DEV              *Dev,
+  OUT VOID                  **Buffer,
+  IN  EFI_PHYSICAL_ADDRESS  DeviceAddress
+  )
+{
+  ORDERED_COLLECTION_ENTRY  *Entry;
+  TX_BUF_MAP_INFO           *TxBufMapInfo;
+  VOID                      *UserStruct;
+
+  Entry = OrderedCollectionFind (Dev->TxBufCollection, &DeviceAddress);
+  if (Entry == NULL) {
+    return EFI_INVALID_PARAMETER;
+  }
+
+  OrderedCollectionDelete (Dev->TxBufCollection, Entry, &UserStruct);
+
+  TxBufMapInfo = UserStruct;
+
+  *Buffer = TxBufMapInfo->Buffer;
+  Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, TxBufMapInfo->BufMap);
+  FreePool (TxBufMapInfo);
+
+  return EFI_SUCCESS;
+}
+
+/**
+  Comparator function for two TX_BUF_MAP_INFO objects.
+
+  @param[in] UserStruct1  Pointer to the first TX_BUF_MAP_INFO object.
+
+  @param[in] UserStruct2  Pointer to the second TX_BUF_MAP_INFO object.
+
+  @retval <0  If UserStruct1 compares less than UserStruct2.
+
+  @retval  0  If UserStruct1 compares equal to UserStruct2.
+
+  @retval >0  If UserStruct1 compares greater than UserStruct2.
+*/
+INTN
+EFIAPI
+VirtioNetTxBufMapInfoCompare (
+  IN CONST VOID *UserStruct1,
+  IN CONST VOID *UserStruct2
+  )
+{
+  CONST TX_BUF_MAP_INFO *MapInfo1;
+  CONST TX_BUF_MAP_INFO *MapInfo2;
+
+  MapInfo1 = UserStruct1;
+  MapInfo2 = UserStruct2;
+
+  return MapInfo1->DeviceAddress < MapInfo2->DeviceAddress ? -1 :
+         MapInfo1->DeviceAddress > MapInfo2->DeviceAddress ?  1 :
+         0;
+}
+
+/**
+  Compare a standalone DeviceAddress against a TX_BUF_MAP_INFO object
+  containing an embedded DeviceAddress.
+
+  @param[in] StandaloneKey  Pointer to DeviceAddress, which has type
+                            EFI_PHYSICAL_ADDRESS.
+
+  @param[in] UserStruct     Pointer to the TX_BUF_MAP_INFO object with the
+                            embedded DeviceAddress.
+
+  @retval <0  If StandaloneKey compares less than UserStruct's key.
+
+  @retval  0  If StandaloneKey compares equal to UserStruct's key.
+
+  @retval >0  If StandaloneKey compares greater than UserStruct's key.
+**/
+INTN
+EFIAPI
+VirtioNetTxBufDeviceAddressCompare (
+  IN CONST VOID *StandaloneKey,
+  IN CONST VOID *UserStruct
+  )
+{
+  CONST EFI_PHYSICAL_ADDRESS *DeviceAddress;
+  CONST TX_BUF_MAP_INFO      *MapInfo;
+
+  DeviceAddress = StandaloneKey;
+  MapInfo = UserStruct;
+
+  return *DeviceAddress < MapInfo->DeviceAddress ? -1 :
+         *DeviceAddress > MapInfo->DeviceAddress ?  1 :
+         0;
+}
-- 
2.9.5

[PATCH v3 7/8] OvmfPkg/VirtioNetDxe: map caller-supplied Tx packet to device-address

[Brijesh Singh]

When device is behind the IOMMU, driver is require to pass the device
address of caller-supplied transmit buffer for the bus master operations.

The patch uses VirtioNetMapTxBuf() to map caller-supplied Tx packet to a
device-address and enqueue the device address in VRING for transfer and
perform the reverse mapping when transfer is completed so that we can
return the caller-supplied buffer.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 OvmfPkg/VirtioNetDxe/SnpGetStatus.c | 41 +++++++++++++++-----
 OvmfPkg/VirtioNetDxe/SnpTransmit.c  | 27 ++++++++++---
 2 files changed, 53 insertions(+), 15 deletions(-)

diff --git a/OvmfPkg/VirtioNetDxe/SnpGetStatus.c b/OvmfPkg/VirtioNetDxe/SnpGetStatus.c
index 694940ea1d97..f817b98801fa 100644
--- a/OvmfPkg/VirtioNetDxe/SnpGetStatus.c
+++ b/OvmfPkg/VirtioNetDxe/SnpGetStatus.c
@@ -61,11 +61,15 @@ VirtioNetGetStatus (
   OUT VOID                       **TxBuf OPTIONAL
   )
 {
-  VNET_DEV   *Dev;
-  EFI_TPL    OldTpl;
-  EFI_STATUS Status;
-  UINT16     RxCurUsed;
-  UINT16     TxCurUsed;
+  VNET_DEV             *Dev;
+  EFI_TPL              OldTpl;
+  EFI_STATUS           Status;
+  UINT16               RxCurUsed;
+  UINT16               TxCurUsed;
+  EFI_PHYSICAL_ADDRESS DeviceAddress;
+  UINT32               LocalInterruptStatus;
+
+  LocalInterruptStatus = *InterruptStatus;
 
   if (This == NULL) {
     return EFI_INVALID_PARAMETER;
@@ -113,11 +117,11 @@ VirtioNetGetStatus (
     //
     *InterruptStatus = 0;
     if (Dev->RxLastUsed != RxCurUsed) {
-      *InterruptStatus |= EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT;
+      LocalInterruptStatus |= EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT;
     }
     if (Dev->TxLastUsed != TxCurUsed) {
       ASSERT (Dev->TxCurPending > 0);
-      *InterruptStatus |= EFI_SIMPLE_NETWORK_TRANSMIT_INTERRUPT;
+      LocalInterruptStatus |= EFI_SIMPLE_NETWORK_TRANSMIT_INTERRUPT;
     }
   }
 
@@ -141,17 +145,36 @@ VirtioNetGetStatus (
       ASSERT (DescIdx < (UINT32) (2 * Dev->TxMaxPending - 1));
 
       //
-      // report buffer address to caller that has been enqueued by caller
+      // get the device address that has been enqueued for the caller's
+      // transmit buffer
       //
-      *TxBuf = (VOID *)(UINTN) Dev->TxRing.Desc[DescIdx + 1].Addr;
+      DeviceAddress = Dev->TxRing.Desc[DescIdx + 1].Addr;
 
       //
       // now this descriptor can be used again to enqueue a transmit buffer
       //
       Dev->TxFreeStack[--Dev->TxCurPending] = (UINT16) DescIdx;
+
+      //
+      // Unmap the device address and perform the reverse mapping to find the
+      // caller buffer address.
+      //
+      Status = VirtioNetUnmapTxBuf (
+                 Dev,
+                 TxBuf,
+                 DeviceAddress
+                 );
+      if (EFI_ERROR (Status)) {
+        //
+        // VirtioNetUnmapTxBuf should never fail, if we have reached here
+        // that means our internal state has been corrupted
+        //
+        ASSERT (FALSE);
+      }
     }
   }
 
+  *InterruptStatus = LocalInterruptStatus;
   Status = EFI_SUCCESS;
 
 Exit:
diff --git a/OvmfPkg/VirtioNetDxe/SnpTransmit.c b/OvmfPkg/VirtioNetDxe/SnpTransmit.c
index 7ca40d5d0650..b39226e138b9 100644
--- a/OvmfPkg/VirtioNetDxe/SnpTransmit.c
+++ b/OvmfPkg/VirtioNetDxe/SnpTransmit.c
@@ -73,11 +73,12 @@ VirtioNetTransmit (
   IN UINT16                      *Protocol OPTIONAL
   )
 {
-  VNET_DEV   *Dev;
-  EFI_TPL    OldTpl;
-  EFI_STATUS Status;
-  UINT16     DescIdx;
-  UINT16     AvailIdx;
+  VNET_DEV              *Dev;
+  EFI_TPL               OldTpl;
+  EFI_STATUS            Status;
+  UINT16                DescIdx;
+  UINT16                AvailIdx;
+  EFI_PHYSICAL_ADDRESS  DeviceAddress;
 
   if (This == NULL || BufferSize == 0 || Buffer == NULL) {
     return EFI_INVALID_PARAMETER;
@@ -144,10 +145,24 @@ VirtioNetTransmit (
   }
 
   //
+  // Map the transmit buffer system physical address to device address.
+  //
+  Status = VirtioNetMapTxBuf (
+             Dev,
+             Buffer,
+             BufferSize,
+             &DeviceAddress
+             );
+  if (EFI_ERROR (Status)) {
+    Status = EFI_DEVICE_ERROR;
+    goto Exit;
+  }
+
+  //
   // virtio-0.9.5, 2.4.1 Supplying Buffers to The Device
   //
   DescIdx = Dev->TxFreeStack[Dev->TxCurPending++];
-  Dev->TxRing.Desc[DescIdx + 1].Addr  = (UINTN) Buffer;
+  Dev->TxRing.Desc[DescIdx + 1].Addr  = DeviceAddress;
   Dev->TxRing.Desc[DescIdx + 1].Len   = (UINT32) BufferSize;
 
   //
-- 
2.9.5

[PATCH v3 8/8] OvmfPkg/VirtioNetDxe: negotiate VIRTIO_F_IOMMU_PLATFORM

[Brijesh Singh]

VirtioNetDxe driver has been updated to use IOMMU-like member functions
from VIRTIO_DEVICE_PROTOCOL to translate the system physical address to
device address. We do not need to do anything special when
VIRTIO_F_IOMMU_PLATFORM bit is present hence treat it in parallel with
VIRTIO_F_VERSION_1.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/VirtioNetDxe/SnpInitialize.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/OvmfPkg/VirtioNetDxe/SnpInitialize.c b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
index ffb3deefe00c..9dff04ce6071 100644
--- a/OvmfPkg/VirtioNetDxe/SnpInitialize.c
+++ b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
@@ -562,7 +562,8 @@ VirtioNetInitialize (
   ASSERT (Dev->Snm.MediaPresentSupported ==
     !!(Features & VIRTIO_NET_F_STATUS));
 
-  Features &= VIRTIO_NET_F_MAC | VIRTIO_NET_F_STATUS | VIRTIO_F_VERSION_1;
+  Features &= VIRTIO_NET_F_MAC | VIRTIO_NET_F_STATUS | VIRTIO_F_VERSION_1 |
+              VIRTIO_F_IOMMU_PLATFORM;
 
   //
   // In virtio-1.0, feature negotiation is expected to complete before queue
@@ -602,7 +603,7 @@ VirtioNetInitialize (
   // step 5 -- keep only the features we want
   //
   if (Dev->VirtIo->Revision < VIRTIO_SPEC_REVISION (1, 0, 0)) {
-    Features &= ~(UINT64)VIRTIO_F_VERSION_1;
+    Features &= ~(UINT64)(VIRTIO_F_VERSION_1 | VIRTIO_F_IOMMU_PLATFORM);
     Status = Dev->VirtIo->SetGuestFeatures (Dev->VirtIo, Features);
     if (EFI_ERROR (Status)) {
       goto ReleaseTxRing;
-- 
2.9.5

Re: [PATCH v3 2/8] OvmfPkg/VirtioNetDxe: map VRINGs using VirtioRingMap()

[Laszlo Ersek]

On 09/14/17 13:08, Brijesh Singh wrote:
> When device is behind the IOMMU then driver need to pass the device
> address when programing the bus master. The patch uses VirtioRingMap() to
> map the VRING system physical address[es] to device address[es].
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  OvmfPkg/VirtioNetDxe/VirtioNet.h        |  7 ++-
>  OvmfPkg/VirtioNetDxe/SnpInitialize.c    | 50 +++++++++++++++-----
>  OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c | 10 ++--
>  OvmfPkg/VirtioNetDxe/SnpShutdown.c      |  4 +-
>  OvmfPkg/VirtioNetDxe/TechNotes.txt      |  5 +-
>  5 files changed, 57 insertions(+), 19 deletions(-)

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

Re: [PATCH v3 3/8] OvmfPkg/VirtioNetDxe: alloc RxBuf using AllocateSharedPages()

[Laszlo Ersek]

On 09/14/17 13:08, Brijesh Singh wrote:
> When device is behind the IOMMU, VirtioNetDxe is required to use the
> device address in bus master operations. RxBuf is allocated using
> AllocatePool() which returns the system physical address.
> 
> The patch uses VIRTIO_DEVICE_PROTOCOL.AllocateSharedPages() to allocate
> the RxBuf and map with VirtioMapAllBytesInSharedBuffer() so that we can
> obtain the device address for RxBuf.
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  OvmfPkg/VirtioNetDxe/VirtioNet.h        |  4 +
>  OvmfPkg/VirtioNetDxe/SnpInitialize.c    | 78 +++++++++++++++-----
>  OvmfPkg/VirtioNetDxe/SnpReceive.c       |  5 +-
>  OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c |  7 +-
>  OvmfPkg/VirtioNetDxe/TechNotes.txt      |  2 +-
>  5 files changed, 75 insertions(+), 21 deletions(-)


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

Re: [PATCH v3 4/8] OvmfPkg/VirtioNetDxe: dynamically alloc transmit header

[Laszlo Ersek]

On 09/14/17 13:08, Brijesh Singh wrote:
> Each network packet is submitted for transmission by pushing the head
> descriptor of a two-part descriptor chain to the Available Ring of the
> TX queue. VirtioNetInitTx() sets up the the descriptor chains for all
> queueable packets in advance, and points all the head descriptors to the
> same shared, never modified, VIRTIO_1_0_NET_REQ header object (or its
> initial VIRTIO_NET_REQ sub-object, dependent on virtio version).
> VirtioNetInitTx() currently uses the header object's system physical
> address for populating the head descriptors.
> 
> When device is behind the IOMMU, VirtioNet driver is required to provide
> the device address of VIRTIO_1_0_NET_REQ header. In this patch we
> dynamically allocate the header using AllocateSharedPages() and map with
> BusMasterCommonBuffer so that header can be accessed by both processor
> and the device.
> 
> We map the header object for CommonBuffer operation because, in order to
> stick with the current code order, we populate the head descriptors with
> the header's device address first, and fill in the header itself second.
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  OvmfPkg/VirtioNetDxe/VirtioNet.h        |  3 +-
>  OvmfPkg/VirtioNetDxe/SnpInitialize.c    | 64 +++++++++++++++++---
>  OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c |  7 +++
>  3 files changed, 65 insertions(+), 9 deletions(-)

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

Re: [PATCH v3 6/8] OvmfPkg/VirtioNetDxe: add Tx packet map/unmap helper functions

[Laszlo Ersek]

On 09/14/17 13:08, Brijesh Singh wrote:
> When device is behind IOMMU, driver is require to pass the device address
> of TxBuf in the Tx VRING. The patch adds helper functions and data
> structure to map and unmap the TxBuf system physical address to a device
> address.
> 
> Since the TxBuf is returned back to caller from VirtioNetGetStatus() hence
> we use OrderedCollection interface to save the TxBuf system physical to
> device address mapping. After the TxBuf is succesfully transmitted
> VirtioNetUnmapTxBuf() does the reverse lookup in OrderedCollection data
> structure to get the system physical address of TxBuf for a given device
> address.
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  OvmfPkg/VirtioNetDxe/VirtioNet.inf      |   1 +
>  OvmfPkg/VirtioNetDxe/VirtioNet.h        |  38 ++++
>  OvmfPkg/VirtioNetDxe/SnpInitialize.c    |  18 +-
>  OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c | 224 ++++++++++++++++++++
>  4 files changed, 279 insertions(+), 2 deletions(-)

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

Re: [PATCH v3 7/8] OvmfPkg/VirtioNetDxe: map caller-supplied Tx packet to device-address

[Laszlo Ersek]

On 09/14/17 13:08, Brijesh Singh wrote:
> When device is behind the IOMMU, driver is require to pass the device
> address of caller-supplied transmit buffer for the bus master operations.
> 
> The patch uses VirtioNetMapTxBuf() to map caller-supplied Tx packet to a
> device-address and enqueue the device address in VRING for transfer and
> perform the reverse mapping when transfer is completed so that we can
> return the caller-supplied buffer.
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  OvmfPkg/VirtioNetDxe/SnpGetStatus.c | 41 +++++++++++++++-----
>  OvmfPkg/VirtioNetDxe/SnpTransmit.c  | 27 ++++++++++---
>  2 files changed, 53 insertions(+), 15 deletions(-)
> 
> diff --git a/OvmfPkg/VirtioNetDxe/SnpGetStatus.c b/OvmfPkg/VirtioNetDxe/SnpGetStatus.c
> index 694940ea1d97..f817b98801fa 100644
> --- a/OvmfPkg/VirtioNetDxe/SnpGetStatus.c
> +++ b/OvmfPkg/VirtioNetDxe/SnpGetStatus.c
> @@ -61,11 +61,15 @@ VirtioNetGetStatus (
>    OUT VOID                       **TxBuf OPTIONAL
>    )
>  {
> -  VNET_DEV   *Dev;
> -  EFI_TPL    OldTpl;
> -  EFI_STATUS Status;
> -  UINT16     RxCurUsed;
> -  UINT16     TxCurUsed;
> +  VNET_DEV             *Dev;
> +  EFI_TPL              OldTpl;
> +  EFI_STATUS           Status;
> +  UINT16               RxCurUsed;
> +  UINT16               TxCurUsed;
> +  EFI_PHYSICAL_ADDRESS DeviceAddress;
> +  UINT32               LocalInterruptStatus;
> +
> +  LocalInterruptStatus = *InterruptStatus;
>  
>    if (This == NULL) {
>      return EFI_INVALID_PARAMETER;
> @@ -113,11 +117,11 @@ VirtioNetGetStatus (
>      //
>      *InterruptStatus = 0;
>      if (Dev->RxLastUsed != RxCurUsed) {
> -      *InterruptStatus |= EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT;
> +      LocalInterruptStatus |= EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT;
>      }
>      if (Dev->TxLastUsed != TxCurUsed) {
>        ASSERT (Dev->TxCurPending > 0);
> -      *InterruptStatus |= EFI_SIMPLE_NETWORK_TRANSMIT_INTERRUPT;
> +      LocalInterruptStatus |= EFI_SIMPLE_NETWORK_TRANSMIT_INTERRUPT;
>      }
>    }
>  

I must have been unclear in my v2 review, sorry about that. My point was
that, given the changes that I requested in points v2/3 and v2/4, the
handling of "InterruptStatus" was fine as it was.

(1) Therefore please undo the v3 "InterruptStatus" changes -- the
"LocalInterruptStatus" variable is unnecessary.


> @@ -141,17 +145,36 @@ VirtioNetGetStatus (
>        ASSERT (DescIdx < (UINT32) (2 * Dev->TxMaxPending - 1));
>  
>        //
> -      // report buffer address to caller that has been enqueued by caller
> +      // get the device address that has been enqueued for the caller's
> +      // transmit buffer
>        //
> -      *TxBuf = (VOID *)(UINTN) Dev->TxRing.Desc[DescIdx + 1].Addr;
> +      DeviceAddress = Dev->TxRing.Desc[DescIdx + 1].Addr;
>  
>        //
>        // now this descriptor can be used again to enqueue a transmit buffer
>        //
>        Dev->TxFreeStack[--Dev->TxCurPending] = (UINT16) DescIdx;
> +
> +      //
> +      // Unmap the device address and perform the reverse mapping to find the
> +      // caller buffer address.
> +      //
> +      Status = VirtioNetUnmapTxBuf (
> +                 Dev,
> +                 TxBuf,
> +                 DeviceAddress
> +                 );
> +      if (EFI_ERROR (Status)) {
> +        //
> +        // VirtioNetUnmapTxBuf should never fail, if we have reached here
> +        // that means our internal state has been corrupted
> +        //
> +        ASSERT (FALSE);

The rest of VirtioNetGetStatus() looks good, but here's the other
misunderstanding:

(2) Please *keep* the ASSERT() that you are adding above, and below it,
*add back* what you had in v2:

        Status = EFI_DEVICE_ERROR;
        goto Exit;

The rest looks good.

Can you submit v4 soon please? :)

Thanks,
Laszlo

> +      }
>      }
>    }
>  
> +  *InterruptStatus = LocalInterruptStatus;
>    Status = EFI_SUCCESS;
>  
>  Exit:
> diff --git a/OvmfPkg/VirtioNetDxe/SnpTransmit.c b/OvmfPkg/VirtioNetDxe/SnpTransmit.c
> index 7ca40d5d0650..b39226e138b9 100644
> --- a/OvmfPkg/VirtioNetDxe/SnpTransmit.c
> +++ b/OvmfPkg/VirtioNetDxe/SnpTransmit.c
> @@ -73,11 +73,12 @@ VirtioNetTransmit (
>    IN UINT16                      *Protocol OPTIONAL
>    )
>  {
> -  VNET_DEV   *Dev;
> -  EFI_TPL    OldTpl;
> -  EFI_STATUS Status;
> -  UINT16     DescIdx;
> -  UINT16     AvailIdx;
> +  VNET_DEV              *Dev;
> +  EFI_TPL               OldTpl;
> +  EFI_STATUS            Status;
> +  UINT16                DescIdx;
> +  UINT16                AvailIdx;
> +  EFI_PHYSICAL_ADDRESS  DeviceAddress;
>  
>    if (This == NULL || BufferSize == 0 || Buffer == NULL) {
>      return EFI_INVALID_PARAMETER;
> @@ -144,10 +145,24 @@ VirtioNetTransmit (
>    }
>  
>    //
> +  // Map the transmit buffer system physical address to device address.
> +  //
> +  Status = VirtioNetMapTxBuf (
> +             Dev,
> +             Buffer,
> +             BufferSize,
> +             &DeviceAddress
> +             );
> +  if (EFI_ERROR (Status)) {
> +    Status = EFI_DEVICE_ERROR;
> +    goto Exit;
> +  }
> +
> +  //
>    // virtio-0.9.5, 2.4.1 Supplying Buffers to The Device
>    //
>    DescIdx = Dev->TxFreeStack[Dev->TxCurPending++];
> -  Dev->TxRing.Desc[DescIdx + 1].Addr  = (UINTN) Buffer;
> +  Dev->TxRing.Desc[DescIdx + 1].Addr  = DeviceAddress;
>    Dev->TxRing.Desc[DescIdx + 1].Len   = (UINT32) BufferSize;
>  
>    //
> 

Re: [PATCH v3 7/8] OvmfPkg/VirtioNetDxe: map caller-supplied Tx packet to device-address

[Brijesh Singh]

On 09/14/2017 03:58 PM, Laszlo Ersek wrote:

...> 
> The rest of VirtioNetGetStatus() looks good, but here's the other
> misunderstanding:
> 
> (2) Please *keep* the ASSERT() that you are adding above, and below it,
> *add back* what you had in v2:
> 
>          Status = EFI_DEVICE_ERROR;
>          goto Exit;
> 
> The rest looks good.
> 
> Can you submit v4 soon please? :)
> 

Yes, I should be submitting very soon :)

-Brijesh