Re: [PATCH 1/5] OvmfPkg/VirtioNetDxe: map VRING using VirtioRingMap()

[Laszlo Ersek <lersek@redhat.com>]

On 09/01/17 13:24, 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 to device address.
>
> 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     |  2 +
>  OvmfPkg/VirtioNetDxe/Events.c        |  7 ++++
>  OvmfPkg/VirtioNetDxe/SnpInitialize.c | 40 ++++++++++++++++----
>  OvmfPkg/VirtioNetDxe/SnpShutdown.c   |  2 +
>  4 files changed, 43 insertions(+), 8 deletions(-)
>
> diff --git a/OvmfPkg/VirtioNetDxe/VirtioNet.h b/OvmfPkg/VirtioNetDxe/VirtioNet.h
> index 710859bc6115..d80d441b50a4 100644
> --- a/OvmfPkg/VirtioNetDxe/VirtioNet.h
> +++ b/OvmfPkg/VirtioNetDxe/VirtioNet.h
> @@ -82,10 +82,12 @@ typedef struct {
>    EFI_HANDLE                  MacHandle;         // VirtioNetDriverBindingStart
>
>    VRING                       RxRing;            // VirtioNetInitRing
> +  VOID                        *RxRingMap;        // VirtioRingMap
>    UINT8                       *RxBuf;            // VirtioNetInitRx
>    UINT16                      RxLastUsed;        // VirtioNetInitRx
>
>    VRING                       TxRing;            // VirtioNetInitRing
> +  VOID                        *TxRingMap;        // VirtioRingMap
>    UINT16                      TxMaxPending;      // VirtioNetInitTx
>    UINT16                      TxCurPending;      // VirtioNetInitTx
>    UINT16                      *TxFreeStack;      // VirtioNetInitTx

(1) Hmmm, here I could be inconsistent with my earlier requests for the
other drivers, but, in order to remain consistent with the comments
here, I think the new comments should say "VirtioNetInitRing" too.

Namely, the existent RxRing and TxRing fields are set up on the
following call path:

  VirtioNetInitialize() -- this is the exported Initialize() protocol
                           member
    VirtioNetInitRing()
      VirtioRingInit() -- set up here
      VirtioRingMap()  -- this is added now

So, because we name VirtioNetInitRing() -- and not VirtioRingInit() --
for "RxRing" and "TxRing", the comments for "RxRingMap" and "TxRingMap"
should also say VirtioNetInitRing.


> diff --git a/OvmfPkg/VirtioNetDxe/Events.c b/OvmfPkg/VirtioNetDxe/Events.c
> index 5be1af6ffbee..6950c4d56df1 100644
> --- a/OvmfPkg/VirtioNetDxe/Events.c
> +++ b/OvmfPkg/VirtioNetDxe/Events.c
> @@ -88,4 +88,11 @@ VirtioNetExitBoot (
>    if (Dev->Snm.State == EfiSimpleNetworkInitialized) {
>      Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
>    }
> +
> +  //
> +  // Unmap Tx and Rx rings so that hypervisor will not be able get readable data
> +  // after device is reset.
> +  //
> +  Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->TxRingMap);
> +  Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RxRingMap);
>  }

(2) This is not correct; the mappings will exist if, and only if,
VirtioNetInitialize() completed successfully.

That is equivalent to (Dev->Snm.State == EfiSimpleNetworkInitialized).

Therefore please move both of these calls into the bracket just above,
visible in the context, after the virtio reset.


> diff --git a/OvmfPkg/VirtioNetDxe/SnpInitialize.c b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
> index 0ecfe044a977..803a38bd4239 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 token return from the VirtioRingMap().
>
>    @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,13 @@ 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;
>
>    //
>    // step 4b -- allocate selected queue
> @@ -79,30 +83,38 @@ VirtioNetInitRing (
>      return Status;
>    }
>

(3) Please add a comment here:

  //
  // If anything fails from here on, we must release the ring resources.
  //

> +  Status = VirtioRingMap (Dev->VirtIo, Ring, &RingBaseShift, Mapping);

(4) Please use a local variable here; we shouldn't modify *Mapping until
the function succeeds fully.

> +  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.
>    //

(5) In the above comment, please replace "release" with "unmap".

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

(6) So we should set *Mapping from the local variable here.

>    return EFI_SUCCESS;
>
> +UnmapQueue:
> +  Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Mapping);
> +

(7) The last argument should be (*Mapping), not (Mapping).

But, given my points (4) and (6), the local variable should be used
here.

>  ReleaseQueue:
>    VirtioRingUninit (Dev->VirtIo, Ring);
>
> @@ -456,12 +468,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,9 +532,11 @@ AbortDevice:
>    Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
>
>  ReleaseTxRing:
> +  Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->TxRingMap);
>    VirtioRingUninit (Dev->VirtIo, &Dev->TxRing);
>
>  ReleaseRxRing:
> +  Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RxRingMap);
>    VirtioRingUninit (Dev->VirtIo, &Dev->RxRing);
>
>  DeviceFailed:
> diff --git a/OvmfPkg/VirtioNetDxe/SnpShutdown.c b/OvmfPkg/VirtioNetDxe/SnpShutdown.c
> index 5e84191fbbdd..36f3253e77ad 100644
> --- a/OvmfPkg/VirtioNetDxe/SnpShutdown.c
> +++ b/OvmfPkg/VirtioNetDxe/SnpShutdown.c
> @@ -67,7 +67,9 @@ VirtioNetShutdown (
>    Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
>    VirtioNetShutdownRx (Dev);
>    VirtioNetShutdownTx (Dev);
> +  Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->TxRingMap);
>    VirtioRingUninit (Dev->VirtIo, &Dev->TxRing);
> +  Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RxRingMap);
>    VirtioRingUninit (Dev->VirtIo, &Dev->RxRing);
>
>    Dev->Snm.State = EfiSimpleNetworkStarted;
>

(8) The logic seems good (all locations are covered correctly), but I
feel we're adding too many standalone UnmapSharedBuffer() calls.

Therefore, please prepend a patch to the series that does the following:

(8a) affected files: "VirtioNet.h", "SnpSharedHelpers.c"

(8b) leading comments for the new function are not needed in *either*
"VirtioNet.h" *or* "SnpSharedHelpers.c". The global comments currently
seen in "SnpSharedHelpers.c" suffice.

(8c) the new function should be added right under VirtioNetShutdownTx().

(8d) This is the new function:

VOID
EFIAPI
VirtioNetUninitRing (
  IN OUT VNET_DEV *Dev,
  IN OUT VRING    *Ring
  )
{
  VirtioRingUninit (Dev->VirtIo, Ring);
}

(EFIAPI wouldn't be strictly necessary, but I'd like to remain
consistent with the rest of the code.)

(8e) The VirtioRingUninit() calls in VirtioNetInitialize() and in
VirtioNetShutdown() -- four (4) in total -- should be replaced with
calls to this new function.

(8f) In this extra patch, please modify the file
"OvmfPkg/VirtioNetDxe/TechNotes.txt" to the following state:

>                    +-------------------------+
>                    | EfiSimpleNetworkStarted |
>                    +-------------------------+
>                                |  ^
>   [SnpInitialize.c]            |  | [SnpShutdown.c]
>   VirtioNetInitialize          |  | VirtioNetShutdown
>     VirtioNetInitRing {Rx, Tx} |  |   VirtioNetShutdownRx [SnpSharedHelpers.c]
>       VirtioRingInit           |  |   VirtioNetShutdownTx [SnpSharedHelpers.c]
>     VirtioNetInitTx            |  |   VirtioNetUninitRing [SnpSharedHelpers.c]
>     VirtioNetInitRx            |  |                       {Tx, Rx}
>                                |  |     VirtioRingUninit
>                                v  |
>                   +-----------------------------+
>                   | EfiSimpleNetworkInitialized |
>                   +-----------------------------+


(9) Then, in v2 of this patch,

(9a) modify the VirtioNetUninitRing() function like this:

VOID
EFIAPI
VirtioNetUninitRing (
  IN OUT VNET_DEV *Dev,
  IN OUT VRING    *Ring,
  IN     VOID     *RingMap
  )
{
  Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, RingMap);
  VirtioRingUninit (Dev->VirtIo, Ring);
}

(9b) The documentation of the "Mapping" parameter, for the
VirtioNetInitialize() function, should say, "a resulting token to pass
to VirtioNetUninitRing()".

(9c) modify the call sites to pass in Dev->TxRingMap / Dev->RxRingMap as
appropriate.

(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 |
>                   +-----------------------------+

(10) Please modify the subject to say "VRINGs" (plural).

Please also modify the commit message similarly: "map the VRING system
physical address[es] to device address[es]".


Would it be OK with you to submit only these two patches next?

I wouldn't like to look at the rest of the patches just now. I expect
quite a few tweaks -- especially because I would *really* like to keep
"TechNotes.txt" up to date! --, and I'd like to keep my focus, and
advance in small steps. I must re-read TechNotes.txt myself, in parallel
to the progress that we make with this series.

Once we consider these patches complete, we can test them and commit
them in isolation. Further versions of the series won't have to repeat
these patches.

I'll strive to be very responsive, so that you don't have to wait long
after every small step.

Thank you,
Laszlo