Re: [PATCH v2 2/8] OvmfPkg/VirtioNetDxe: map VRINGs using VirtioRingMap()

[Laszlo Ersek <lersek@redhat.com>]

On 09/11/17 14:16, Brijesh Singh wrote:
> When device is behind the IOMMU then driver need to pass the device
> address when programing the bus master. The patch uses VirtioRingMap() to
> map the VRING system physical address[es] to device address[es].
>
> 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        |  7 ++-
>  OvmfPkg/VirtioNetDxe/SnpInitialize.c    | 50 +++++++++++++++-----
>  OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c | 10 ++--
>  OvmfPkg/VirtioNetDxe/SnpShutdown.c      |  4 +-
>  OvmfPkg/VirtioNetDxe/TechNotes.txt      |  1 +
>  5 files changed, 55 insertions(+), 17 deletions(-)
>
> diff --git a/OvmfPkg/VirtioNetDxe/VirtioNet.h b/OvmfPkg/VirtioNetDxe/VirtioNet.h
> index 87a0f06e01a4..6762fc9d1d6e 100644
> --- a/OvmfPkg/VirtioNetDxe/VirtioNet.h
> +++ b/OvmfPkg/VirtioNetDxe/VirtioNet.h
> @@ -82,10 +82,14 @@ typedef struct {
>    EFI_HANDLE                  MacHandle;         // VirtioNetDriverBindingStart
>
>    VRING                       RxRing;            // VirtioNetInitRing
> +  VOID                        *RxRingMap;        // VirtioRingMap and
> +                                                 // VirtioNetInitRing
>    UINT8                       *RxBuf;            // VirtioNetInitRx
>    UINT16                      RxLastUsed;        // VirtioNetInitRx
>
>    VRING                       TxRing;            // VirtioNetInitRing
> +  VOID                        *TxRingMap;        // VirtioRingMap and
> +                                                 // VirtioNetInitRing
>    UINT16                      TxMaxPending;      // VirtioNetInitTx
>    UINT16                      TxCurPending;      // VirtioNetInitTx
>    UINT16                      *TxFreeStack;      // VirtioNetInitTx
> @@ -267,7 +271,8 @@ VOID
>  EFIAPI
>  VirtioNetUninitRing (
>    IN OUT VNET_DEV *Dev,
> -  IN OUT VRING    *Ring
> +  IN OUT VRING    *Ring,
> +  IN     VOID     *RingMap
>    );
>
>  //
> diff --git a/OvmfPkg/VirtioNetDxe/SnpInitialize.c b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
> index 637c978709fd..8eabdbff6f5e 100644
> --- a/OvmfPkg/VirtioNetDxe/SnpInitialize.c
> +++ b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
> @@ -35,11 +35,13 @@
>                             the network device.
>    @param[out]    Ring      The virtio-ring inside the VNET_DEV structure,
>                             corresponding to Selector.
> +  @param[out]    Mapping   A resulting token to pass to VirtioNetUninitRing()
>
>    @retval EFI_UNSUPPORTED  The queue size reported by the virtio-net device is
>                             too small.
>    @return                  Status codes from VIRTIO_CFG_WRITE(),
> -                           VIRTIO_CFG_READ() and VirtioRingInit().
> +                           VIRTIO_CFG_READ(), VirtioRingInit() and
> +                           VirtioRingMap().
>    @retval EFI_SUCCESS      Ring initialized.
>  */
>
> @@ -49,11 +51,14 @@ EFIAPI
>  VirtioNetInitRing (
>    IN OUT VNET_DEV *Dev,
>    IN     UINT16   Selector,
> -  OUT    VRING    *Ring
> +  OUT    VRING    *Ring,
> +  OUT    VOID     **Mapping
>    )
>  {
>    EFI_STATUS Status;
>    UINT16     QueueSize;
> +  UINT64     RingBaseShift;
> +  VOID       *MapInfo;
>
>    //
>    // step 4b -- allocate selected queue
> @@ -80,29 +85,42 @@ VirtioNetInitRing (
>    }
>
>    //
> +  // If anything fails from here on, we must release the ring resources.
> +  //
> +  Status = VirtioRingMap (Dev->VirtIo, Ring, &RingBaseShift, &MapInfo);
> +  if (EFI_ERROR (Status)) {
> +    goto ReleaseQueue;
> +  }
> +
> +  //
>    // Additional steps for MMIO: align the queue appropriately, and set the
> -  // size. If anything fails from here on, we must release the ring resources.
> +  // size. If anything fails from here on, we must unmap the ring resources.
>    //
>    Status = Dev->VirtIo->SetQueueNum (Dev->VirtIo, QueueSize);
>    if (EFI_ERROR (Status)) {
> -    goto ReleaseQueue;
> +    goto UnmapQueue;
>    }
>
>    Status = Dev->VirtIo->SetQueueAlign (Dev->VirtIo, EFI_PAGE_SIZE);
>    if (EFI_ERROR (Status)) {
> -    goto ReleaseQueue;
> +    goto UnmapQueue;
>    }
>
>    //
>    // step 4c -- report GPFN (guest-physical frame number) of queue
>    //
> -  Status = Dev->VirtIo->SetQueueAddress (Dev->VirtIo, Ring, 0);
> +  Status = Dev->VirtIo->SetQueueAddress (Dev->VirtIo, Ring, RingBaseShift);
>    if (EFI_ERROR (Status)) {
> -    goto ReleaseQueue;
> +    goto UnmapQueue;
>    }
>
> +  *Mapping = MapInfo;
> +
>    return EFI_SUCCESS;
>
> +UnmapQueue:
> +  Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, MapInfo);
> +
>  ReleaseQueue:
>    VirtioRingUninit (Dev->VirtIo, Ring);
>
> @@ -456,12 +474,22 @@ VirtioNetInitialize (
>    //
>    // step 4b, 4c -- allocate and report virtqueues
>    //
> -  Status = VirtioNetInitRing (Dev, VIRTIO_NET_Q_RX, &Dev->RxRing);
> +  Status = VirtioNetInitRing (
> +             Dev,
> +             VIRTIO_NET_Q_RX,
> +             &Dev->RxRing,
> +             &Dev->RxRingMap
> +             );
>    if (EFI_ERROR (Status)) {
>      goto DeviceFailed;
>    }
>
> -  Status = VirtioNetInitRing (Dev, VIRTIO_NET_Q_TX, &Dev->TxRing);
> +  Status = VirtioNetInitRing (
> +             Dev,
> +             VIRTIO_NET_Q_TX,
> +             &Dev->TxRing,
> +             &Dev->TxRingMap
> +             );
>    if (EFI_ERROR (Status)) {
>      goto ReleaseRxRing;
>    }
> @@ -510,10 +538,10 @@ AbortDevice:
>    Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
>
>  ReleaseTxRing:
> -  VirtioNetUninitRing (Dev, &Dev->TxRing);
> +  VirtioNetUninitRing (Dev, &Dev->TxRing, Dev->TxRingMap);
>
>  ReleaseRxRing:
> -  VirtioNetUninitRing (Dev, &Dev->RxRing);
> +  VirtioNetUninitRing (Dev, &Dev->RxRing, Dev->RxRingMap);
>
>  DeviceFailed:
>    //
> diff --git a/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c b/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
> index 5b75eabc7a6b..57c7395848bd 100644
> --- a/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
> +++ b/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
> @@ -55,15 +55,19 @@ VirtioNetShutdownTx (
>  /**
>    Release TX and RX VRING resources.
>
> -  @param[in,out] Dev   The VNET_DEV driver instance which was using the ring.
> -  @param[in,out] Ring  The virtio ring to clean up.
> +  @param[in,out] Dev       The VNET_DEV driver instance which was using
> +                           the ring.
> +  @param[in,out] Ring      The virtio ring to clean up.
> +  @param[in]     RingMap   A token return from the VirtioRingMap()
>  */
>  VOID
>  EFIAPI
>  VirtioNetUninitRing (
>    IN OUT VNET_DEV *Dev,
> -  IN OUT VRING    *Ring
> +  IN OUT VRING    *Ring,
> +  IN     VOID     *RingMap
>    )
>  {
> +  Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, RingMap);
>    VirtioRingUninit (Dev->VirtIo, Ring);
>  }
> diff --git a/OvmfPkg/VirtioNetDxe/SnpShutdown.c b/OvmfPkg/VirtioNetDxe/SnpShutdown.c
> index 432e0691d457..d8c11f20de61 100644
> --- a/OvmfPkg/VirtioNetDxe/SnpShutdown.c
> +++ b/OvmfPkg/VirtioNetDxe/SnpShutdown.c
> @@ -67,8 +67,8 @@ VirtioNetShutdown (
>    Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
>    VirtioNetShutdownRx (Dev);
>    VirtioNetShutdownTx (Dev);
> -  VirtioNetUninitRing (Dev, &Dev->TxRing);
> -  VirtioNetUninitRing (Dev, &Dev->RxRing);
> +  VirtioNetUninitRing (Dev, &Dev->TxRing, Dev->TxRingMap);
> +  VirtioNetUninitRing (Dev, &Dev->RxRing, Dev->RxRingMap);
>
>    Dev->Snm.State = EfiSimpleNetworkStarted;
>    Status = EFI_SUCCESS;
> diff --git a/OvmfPkg/VirtioNetDxe/TechNotes.txt b/OvmfPkg/VirtioNetDxe/TechNotes.txt
> index 86b91f561495..0891e8210489 100644
> --- a/OvmfPkg/VirtioNetDxe/TechNotes.txt
> +++ b/OvmfPkg/VirtioNetDxe/TechNotes.txt
> @@ -72,6 +72,7 @@ faithfully indented) that implement the transition.
>        VirtioRingInit           |  |   VirtioNetShutdownTx [SnpSharedHelpers.c]
>      VirtioNetInitTx            |  |   VirtioNetUninitRing [SnpSharedHelpers.c]
>      VirtioNetInitRx            |  |                       {Tx, Rx}
> +                               |  |     VirtIo->UnmapSharedBuffer
>                                 |  |     VirtioRingUninit
>                                 v  |
>                    +-----------------------------+
>

(1) The documentation udpate is not correct; please check my previous
review:

http://mid.mail-archive.com/04fefb16-23d5-f6df-0657-9d4c5e96ac57@redhat.com

In particular you forgot to add the VirtioRingMap function call on the
left side, under VirtioNetInitRing:

On 09/05/17 13:47, Laszlo Ersek wrote:
> (9d) modify the documentation again, to the following state:
>
>>                    +-------------------------+
>>                    | EfiSimpleNetworkStarted |
>>                    +-------------------------+
>>                                |  ^
>>   [SnpInitialize.c]            |  | [SnpShutdown.c]
>>   VirtioNetInitialize          |  | VirtioNetShutdown
>>     VirtioNetInitRing {Rx, Tx} |  |   VirtioNetShutdownRx [SnpSharedHelpers.c]
>>       VirtioRingInit           |  |   VirtioNetShutdownTx [SnpSharedHelpers.c]
>>       VirtioRingMap            |  |   VirtioNetUninitRing [SnpSharedHelpers.c]
>>     VirtioNetInitTx            |  |                       {Tx, Rx}
>>     VirtioNetInitRx            |  |     VirtIo->UnmapSharedBuffer
>>                                |  |     VirtioRingUninit
>>                                v  |
>>                   +-----------------------------+
>>                   | EfiSimpleNetworkInitialized |
>>                   +-----------------------------+

I think simply pasting the above into "TechNotes.txt" would be easiest.

The rest looks good!

Thanks
Laszlo