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

[Laszlo Ersek <lersek@redhat.com>]

On 09/11/17 14:16, 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(-)
> 
> diff --git a/OvmfPkg/VirtioNetDxe/VirtioNet.h b/OvmfPkg/VirtioNetDxe/VirtioNet.h
> index 7d5f33b01dc8..3f48bcc6b67c 100644
> --- a/OvmfPkg/VirtioNetDxe/VirtioNet.h
> +++ b/OvmfPkg/VirtioNetDxe/VirtioNet.h
> @@ -96,7 +96,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
>    EFI_PHYSICAL_ADDRESS        RxBufDeviceBase;   // VirtioNetInitRx
>  } VNET_DEV;
> diff --git a/OvmfPkg/VirtioNetDxe/SnpInitialize.c b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
> index 54c808c501bf..6cedb406a172 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..2ec3dc385a9f 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)),
> +                 (VOID *) Dev->TxSharedReq
> +                 );
> +
>    FreePool (Dev->TxFreeStack);
>  }
>  
> 

(1) Regarding the last argument of FreeSharedPages(), from my previous
review at

http://mid.mail-archive.com/ea935749-2a4f-bedd-44ee-05b59524ea07@redhat.com

you missed:

On 09/06/17 11:11, Laszlo Ersek wrote:
> (18) The (VOID*) cast is unneeded.

The rest is good.

Thanks!
Laszlo