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

[edk2] [PATCH v3 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 v3 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 | 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

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v3 7/8] OvmfPkg/VirtioNetDxe: map caller-supplied Tx packet to device-address
Posted by Laszlo Ersek 7 years, 3 months ago 
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;
>  
>    //
> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v3 7/8] OvmfPkg/VirtioNetDxe: map caller-supplied Tx packet to device-address
Posted by Brijesh Singh 7 years, 3 months ago 

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
_______________________________________________
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.