Re: [PATCH 3/5] OvmfPkg/VirtioNetDxe: dynamically alloc transmit header

[Laszlo Ersek <lersek@redhat.com>]

On 09/01/17 13:24, Brijesh Singh wrote:
> A network packets are transmitted by placing them into a transmit queue,
> each packet is preceded by a VIRTIO_1_0_NET_REQ header. VirtioNetInitTx(),
> builds the header once and fills the vring descriptors with the system
> physical address of the VIRTIO_1_0_NET_REQ header.

(1) Please *replace* this paragraph (emphasis added to avoid
misunderstandings) with the following:

----
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 could map it with  BusMasterRead but since the header pointer is
> used after it was added into vring hence I choose to dynamically allocate
> and map it with BusMasterCommonBuffer to avoid the code complexity.

(2) This is a very good choice!, but I'd like the commit message to be
clearer. How about this, as a replacement for the last paragraph:

-------
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
second.
-------

(And then, we're not even mentioning that the header has to be
unmappable at ExitBootServices() without freeing memory. But see the
parallel discussion about that anyway...)


> 
> 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/Events.c           |  6 ++
>  OvmfPkg/VirtioNetDxe/SnpInitialize.c    | 65 +++++++++++++++++---
>  OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c |  7 +++
>  4 files changed, 72 insertions(+), 9 deletions(-)
> 
> diff --git a/OvmfPkg/VirtioNetDxe/VirtioNet.h b/OvmfPkg/VirtioNetDxe/VirtioNet.h
> index 7df51bd044f5..3efe95a735d8 100644
> --- a/OvmfPkg/VirtioNetDxe/VirtioNet.h
> +++ b/OvmfPkg/VirtioNetDxe/VirtioNet.h
> @@ -94,7 +94,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;   // VirtioMapSharedBuffer

(3) Consistently with this code, and with my other comments for this
patch-set (and inconsistently with the other virtio drivers, sorry about
that), please make the comment say "VirtioNetInitTx" for
"TxSharedReqMap" as well.


>    UINT16                      TxLastUsed;        // VirtioNetInitTx
>  } VNET_DEV;
>  
> diff --git a/OvmfPkg/VirtioNetDxe/Events.c b/OvmfPkg/VirtioNetDxe/Events.c
> index 0586a82cdf09..9366aa00a1b3 100644
> --- a/OvmfPkg/VirtioNetDxe/Events.c
> +++ b/OvmfPkg/VirtioNetDxe/Events.c
> @@ -101,4 +101,10 @@ VirtioNetExitBoot (
>    // device is reset.
>    //
>    Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RxBufMap);
> +
> +  //
> +  // Unmap shared Tx request mapping so that hypervisor will not be able to get
> +  // readable data after device is reset.
> +  //
> +  Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->TxSharedReqMap);
>  }

(4) This too belongs in the "EfiSimpleNetworkInitialized" scope.

(Assuming we're going to stick with the unmap-at-ExitBootServices idea
at all... See the other discussion. For now, I recommend going ahead
with this approach.)


> diff --git a/OvmfPkg/VirtioNetDxe/SnpInitialize.c b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
> index 6052d40fc6cb..551820e30f36 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>
>  
> @@ -151,8 +152,12 @@ VirtioNetInitTx (
>    IN OUT VNET_DEV *Dev
>    )
>  {
> -  UINTN TxSharedReqSize;
> -  UINTN PktIdx;
> +  UINTN                 TxSharedReqSize;
> +  UINTN                 PktIdx;
> +  EFI_STATUS            Status;
> +  EFI_PHYSICAL_ADDRESS  DeviceAddress;
> +  UINT64                BufBaseShift;

(5) Please drop "BufBaseShift".


> +  VOID                  *TxSharedReqBuffer;
>  
>    Dev->TxMaxPending = (UINT16) MIN (Dev->TxRing.QueueSize / 2,
>                                   VNET_MAX_PENDING);
> @@ -164,24 +169,60 @@ 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)) {
> +    return EFI_OUT_OF_RESOURCES;

(6) This is wrong, we would leak "Dev->TxFreeStack" by returning here.
Please jump to an error handling label like "FreeTxFreeStack".


(7) The Status value should be propagated here, not masked as
EFI_OUT_OF_RESOURCES.

The error handling label will handle this anyway, but I'm spelling it
out so that you please update the leading comment block on the function
(= error conditions).


> +  }
> +
> +  Status = VirtioMapAllBytesInSharedBuffer (
> +             Dev->VirtIo,
> +             VirtioOperationBusMasterCommonBuffer,
> +             TxSharedReqBuffer,
> +             sizeof *(Dev->TxSharedReq),
> +             &DeviceAddress,
> +             &Dev->TxSharedReqMap
> +             );
> +  if (EFI_ERROR (Status)) {
> +    Status = EFI_OUT_OF_RESOURCES;

(8) Please don't mask the returned Status.


> +    goto FreeBuffer;

(9) For clarity, I suggest "FreeTxSharedReqBuffer" for the label.


> +  }
> +
> +  Dev->TxSharedReq = TxSharedReqBuffer;
> +
> +  BufBaseShift = (UINT64) (UINTN)Dev->TxSharedReq - (UINT64) DeviceAddress;
> +
> +  ZeroMem ((VOID *) Dev->TxSharedReq, sizeof *(Dev->TxSharedReq));

(10) This ZeroMem() is in the wrong spot; it should be done *between*
AllocateSharedPages() and VirtioMapAllBytesInSharedBuffer().


(11) No need for the explicit (VOID*) cast.


(12) I have a question about

  *(Dev->TxSharedReq)

Did you write the expression like this because you genuinely find the
parens easier to read, or because you did a search-and-replace, and
wanted to ensure that the operator bindings stay fine?

If you genuinely like it better this way, I can accept that -- I dislike
it :) --, but if you just did a search-and-replace, then please replace
all occurrences with

  *Dev->TxSharedReq

(this affects other files modified in the patch as well).


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

(13) More of the above, but in this case, I don't feel so lenient; the
following expression:

  (Dev->TxSharedReq)->V0_9_5

simply stumps me :) Please replace it with:

  Dev->TxSharedReq->V0_9_5


>  
>    for (PktIdx = 0; PktIdx < Dev->TxMaxPending; ++PktIdx) {
>      UINT16 DescIdx;
> +    UINT64 Address;

(14) Please drop "Address".


>  
>      DescIdx = (UINT16) (2 * PktIdx);
>      Dev->TxFreeStack[PktIdx] = DescIdx;
>  
> +    Address = (UINTN) Dev->TxSharedReq;
> +    Address += BufBaseShift;
> +
>      //
>      // 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  = Address;

(15) Simply assign "DeviceAddress".

(BTW, your BufBaseShift calculation should be negated anyway, because
you want to go from host address to device address. You start with the
host address, so your addend, i.e. BusBaseShift, has to *subtract* the
host address, and *add* the device address.

But it's moot anyway, just assign DeviceAddress to Addr.)


>      Dev->TxRing.Desc[DescIdx].Len   = (UINT32) TxSharedReqSize;
>      Dev->TxRing.Desc[DescIdx].Flags = VRING_DESC_F_NEXT;
>      Dev->TxRing.Desc[DescIdx].Next  = (UINT16) (DescIdx + 1);
> @@ -196,13 +237,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
> @@ -217,6 +258,14 @@ VirtioNetInitTx (
>    *Dev->TxRing.Avail.Flags = (UINT16) VRING_AVAIL_F_NO_INTERRUPT;
>  
>    return EFI_SUCCESS;
> +
> +FreeBuffer:
> +  Dev->VirtIo->FreeSharedPages (
> +                 Dev->VirtIo,
> +                 EFI_SIZE_TO_PAGES (sizeof *(Dev->TxSharedReq)),
> +                 (VOID *) Dev->TxSharedReq
> +                 );

(16) The last argument is wrong. If VirtioMapAllBytesInSharedBuffer()
fails and you jump here, you won't have assigned "Dev->TxSharedReq" the
value of "TxSharedReqBuffer" just yet.

So please replace the last argument with "TxSharedReqBuffer".


(17) The explicit (VOID*) cast is unnecessary.


> +  return Status;
>  }
>  
>  
> diff --git a/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c b/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
> index 4c9d9ece0790..aeb9e6605f0d 100644
> --- a/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
> +++ b/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
> @@ -54,5 +54,12 @@ 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
> +                 );
> +

(18) The (VOID*) cast is unneeded.


>    FreePool (Dev->TxFreeStack);
>  }
> 

Thanks!
Laszlo