1. Patchew
  2. EDK2
  3. [edk2] [PATCH v2 0/8] OvmfPkg/VirtioNetDxe: map host address to device address
  4. View patch

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

Brijesh Singh posted 8 patches 7 years, 3 months ago
There is a newer version of this series
[edk2] [PATCH v2 7/8] OvmfPkg/VirtioNetDxe: map caller-supplied Tx packet to device-address
Posted by Brijesh Singh 7 years, 3 months ago 
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 | 30 +++++++++++++----
 OvmfPkg/VirtioNetDxe/SnpTransmit.c  | 34 ++++++++++++++++----
 2 files changed, 50 insertions(+), 14 deletions(-)

diff --git a/OvmfPkg/VirtioNetDxe/SnpGetStatus.c b/OvmfPkg/VirtioNetDxe/SnpGetStatus.c
index 694940ea1d97..9bd62fbe8ec0 100644
--- a/OvmfPkg/VirtioNetDxe/SnpGetStatus.c
+++ b/OvmfPkg/VirtioNetDxe/SnpGetStatus.c
@@ -61,11 +61,12 @@ 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;
 
   if (This == NULL) {
     return EFI_INVALID_PARAMETER;
@@ -141,9 +142,24 @@ VirtioNetGetStatus (
       ASSERT (DescIdx < (UINT32) (2 * Dev->TxMaxPending - 1));
 
       //
-      // report buffer address to caller that has been enqueued by caller
+      // get the device address to caller that has been enqueued by caller
       //
-      *TxBuf = (VOID *)(UINTN) Dev->TxRing.Desc[DescIdx + 1].Addr;
+      DeviceAddress = Dev->TxRing.Desc[DescIdx + 1].Addr;
+
+      //
+      // Unmap the device address and perform the reverse mapping to find the
+      // caller buffer address.
+      //
+      Status = VirtioNetUnmapTxBuf (
+                 Dev,
+                 DescIdx + 1,
+                 TxBuf,
+                 DeviceAddress
+                 );
+      if (EFI_ERROR (Status)) {
+        Status = EFI_DEVICE_ERROR;
+        goto Exit;
+      }
 
       //
       // now this descriptor can be used again to enqueue a transmit buffer
diff --git a/OvmfPkg/VirtioNetDxe/SnpTransmit.c b/OvmfPkg/VirtioNetDxe/SnpTransmit.c
index 7ca40d5d0650..0be3243b4606 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,29 @@ VirtioNetTransmit (
   }
 
   //
-  // virtio-0.9.5, 2.4.1 Supplying Buffers to The Device
+  // Get the free VRING descriptor
   //
   DescIdx = Dev->TxFreeStack[Dev->TxCurPending++];
-  Dev->TxRing.Desc[DescIdx + 1].Addr  = (UINTN) Buffer;
+
+  //
+  // Map the transmit buffer system physical address to device address.
+  //
+  Status = VirtioNetMapTxBuf (
+             Dev,
+             DescIdx + 1,
+             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
+  //
+  Dev->TxRing.Desc[DescIdx + 1].Addr  = DeviceAddress;
   Dev->TxRing.Desc[DescIdx + 1].Len   = (UINT32) BufferSize;
 
   //
-- 
2.9.4

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2 7/8] OvmfPkg/VirtioNetDxe: map caller-supplied Tx packet to device-address
Posted by Laszlo Ersek 7 years, 3 months ago 
On 09/11/17 14:16, 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 | 30 +++++++++++++----
>  OvmfPkg/VirtioNetDxe/SnpTransmit.c  | 34 ++++++++++++++++----
>  2 files changed, 50 insertions(+), 14 deletions(-)
> 
> diff --git a/OvmfPkg/VirtioNetDxe/SnpGetStatus.c b/OvmfPkg/VirtioNetDxe/SnpGetStatus.c
> index 694940ea1d97..9bd62fbe8ec0 100644
> --- a/OvmfPkg/VirtioNetDxe/SnpGetStatus.c
> +++ b/OvmfPkg/VirtioNetDxe/SnpGetStatus.c
> @@ -61,11 +61,12 @@ 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;
>  
>    if (This == NULL) {
>      return EFI_INVALID_PARAMETER;
> @@ -141,9 +142,24 @@ VirtioNetGetStatus (
>        ASSERT (DescIdx < (UINT32) (2 * Dev->TxMaxPending - 1));
>  
>        //
> -      // report buffer address to caller that has been enqueued by caller
> +      // get the device address to caller that has been enqueued by caller

(1) I suggest, "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;
> +
> +      //
> +      // Unmap the device address and perform the reverse mapping to find the
> +      // caller buffer address.
> +      //
> +      Status = VirtioNetUnmapTxBuf (
> +                 Dev,
> +                 DescIdx + 1,

(2) As discussed, this argument should go away.

> +                 TxBuf,
> +                 DeviceAddress
> +                 );
> +      if (EFI_ERROR (Status)) {
> +        Status = EFI_DEVICE_ERROR;
> +        goto Exit;
> +      }

Now, normally I would request the following: "Because you are
introducing a new error path here, it is now possible that
*InterruptStatus is modified earlier, but we exit with and error. Please
introduce a local variable for InterruptStatus, and only assign
*InterruptStatus when everything is fine."

However, VirtioNetUnmapTxBuf() should never fail. It can only return an
error if "DeviceAddress is not mapped", and that means our internal
state has been corrupted somehow.

(3) Therefore, please add such a comment, plus an "ASSERT (FALSE)" right
above the "Status = EFI_DEVICE_ERROR" assignment.


>  
>        //
>        // now this descriptor can be used again to enqueue a transmit buffer

(4) Please move the VirtioNetUnmapTxBuf() call *under* the TxFreeStack
manipulation. So that the order of operations is ultimately:

(4a) fetch DeviceAddress

(4b) put DescIdx back on TxFreeStack

(4c) call VirtioNetUnmapTxBuf() as final operation (followed by the
error check + ASSERT, as discussed above)


> diff --git a/OvmfPkg/VirtioNetDxe/SnpTransmit.c b/OvmfPkg/VirtioNetDxe/SnpTransmit.c
> index 7ca40d5d0650..0be3243b4606 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,29 @@ VirtioNetTransmit (
>    }
>  
>    //
> -  // virtio-0.9.5, 2.4.1 Supplying Buffers to The Device
> +  // Get the free VRING descriptor
>    //
>    DescIdx = Dev->TxFreeStack[Dev->TxCurPending++];
> -  Dev->TxRing.Desc[DescIdx + 1].Addr  = (UINTN) Buffer;
> +
> +  //
> +  // Map the transmit buffer system physical address to device address.
> +  //
> +  Status = VirtioNetMapTxBuf (
> +             Dev,
> +             DescIdx + 1,

(5) this argument is going away


> +             Buffer,
> +             BufferSize,
> +             &DeviceAddress
> +             );
> +  if (EFI_ERROR (Status)) {
> +    Status = EFI_DEVICE_ERROR;
> +    goto Exit;
> +  }

VirtioNetMapTxBuf() can genuinely fail for a number of reasons, and if
we exit here like this, we will have leaked a descriptor from "TxFreeStack".

(6) Therefore, please don't touch the comment

  // virtio-0.9.5, 2.4.1 Supplying Buffers to The Device

Instead, move the VirtioNetMapTxBuf() call right above that comment. The
error handling for VirtioNetMapTxBuf() becomes correct then.

Thanks!
Laszlo

> +
> +  //
> +  // virtio-0.9.5, 2.4.1 Supplying Buffers to The Device
> +  //
> +  Dev->TxRing.Desc[DescIdx + 1].Addr  = DeviceAddress;
>    Dev->TxRing.Desc[DescIdx + 1].Len   = (UINT32) BufferSize;
>  
>    //
> 

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

Patchew 2.1-devel-8c64711

© 2016 - 2024 Red Hat, Inc.