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

[Laszlo Ersek <lersek@redhat.com>]

On 09/11/17 14:16, 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    | 77 +++++++++++++++-----
>  OvmfPkg/VirtioNetDxe/SnpReceive.c       |  5 +-
>  OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c |  7 +-
>  OvmfPkg/VirtioNetDxe/TechNotes.txt      |  2 +-
>  5 files changed, 74 insertions(+), 21 deletions(-)
> 
> diff --git a/OvmfPkg/VirtioNetDxe/VirtioNet.h b/OvmfPkg/VirtioNetDxe/VirtioNet.h
> index 6762fc9d1d6e..7d5f33b01dc8 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,8 @@ typedef struct {
>                                                   // VirtioNetInitRing
>    UINT8                       *RxBuf;            // VirtioNetInitRx
>    UINT16                      RxLastUsed;        // VirtioNetInitRx
> +  UINTN                       RxBufNrPages;      // VirtioNetInitRx
> +  VOID                        *RxBufMap;         // VirtioNetInitRx
>  
>    VRING                       TxRing;            // VirtioNetInitRing
>    VOID                        *TxRingMap;        // VirtioRingMap and
> @@ -95,6 +98,7 @@ typedef struct {
>    UINT16                      *TxFreeStack;      // VirtioNetInitTx
>    VIRTIO_1_0_NET_REQ          TxSharedReq;       // VirtioNetInitTx
>    UINT16                      TxLastUsed;        // VirtioNetInitTx
> +  EFI_PHYSICAL_ADDRESS        RxBufDeviceBase;   // VirtioNetInitRx

(1) For clarity, please move the new "RxBufDeviceBase" field between
"RxBufNrPages" and "RxBufMap".


>  } VNET_DEV;
>  
>  
> diff --git a/OvmfPkg/VirtioNetDxe/SnpInitialize.c b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
> index 8eabdbff6f5e..54c808c501bf 100644
> --- a/OvmfPkg/VirtioNetDxe/SnpInitialize.c
> +++ b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
> @@ -242,8 +242,10 @@ 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().
> +  @retval EFI_OUT_OF_RESOURCES  Failed to allocate or map RX destination area.
> +  @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.
>  */

The update to "@return ..." is fine.

(2) But, please remove the "@retval EFI_OUT_OF_RESOURCES" line entirely;
we no longer return this status code explicitly.


> @@ -255,13 +257,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 +290,37 @@ 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;
>    }
>  

(3) In this spot, please add:

  ZeroMem (RxBuffer, NumBytes);

The rest looks good.

Thank you,
Laszlo

> +  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 +340,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 +351,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 +381,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 0891e8210489..f39426fb13e4 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.
>