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

[Laszlo Ersek <lersek@redhat.com>]

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;
>  
>    //
>