Re: [PATCH v2 6/8] OvmfPkg/VirtioNetDxe: add Tx packet map/unmap helper functions

[Laszlo Ersek <lersek@redhat.com>]

On 09/11/17 14:16, Brijesh Singh wrote:
> When device is behind IOMMU, driver is require to pass the device address
> of TxBuf in the Tx VRING. The patch adds helper functions and data
> structure to map and unmap the TxBuf system physical address to a device
> address.
>
> Since the TxBuf is returned back to caller from VirtioNetGetStatus() hence
> we use OrderedCollection interface to save the TxBuf system physical to
> device address mapping. After the TxBuf is succesfully transmitted
> VirtioNetUnmapTxBuf() does the reverse lookup in OrderedCollection data
> structure to get the system physical address of TxBuf for a given 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.inf      |   1 +
>  OvmfPkg/VirtioNetDxe/VirtioNet.h        |  32 ++++
>  OvmfPkg/VirtioNetDxe/SnpInitialize.c    |  15 +-
>  OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c | 188 ++++++++++++++++++++
>  4 files changed, 235 insertions(+), 1 deletion(-)
>
> diff --git a/OvmfPkg/VirtioNetDxe/VirtioNet.inf b/OvmfPkg/VirtioNetDxe/VirtioNet.inf
> index a855ad4ac154..9ff6d87e6190 100644
> --- a/OvmfPkg/VirtioNetDxe/VirtioNet.inf
> +++ b/OvmfPkg/VirtioNetDxe/VirtioNet.inf
> @@ -49,6 +49,7 @@
>    DebugLib
>    DevicePathLib
>    MemoryAllocationLib
> +  OrderedCollectionLib
>    UefiBootServicesTableLib
>    UefiDriverEntryPoint
>    UefiLib
> diff --git a/OvmfPkg/VirtioNetDxe/VirtioNet.h b/OvmfPkg/VirtioNetDxe/VirtioNet.h
> index 3f48bcc6b67c..906bec8e88f3 100644
> --- a/OvmfPkg/VirtioNetDxe/VirtioNet.h
> +++ b/OvmfPkg/VirtioNetDxe/VirtioNet.h
> @@ -26,6 +26,7 @@
>  #include <Protocol/DevicePath.h>
>  #include <Protocol/DriverBinding.h>
>  #include <Protocol/SimpleNetwork.h>
> +#include <Library/OrderedCollectionLib.h>
>
>  #define VNET_SIG SIGNATURE_32 ('V', 'N', 'E', 'T')
>
> @@ -100,6 +101,7 @@ typedef struct {
>    VOID                        *TxSharedReqMap;   // VirtioNetInitTx
>    UINT16                      TxLastUsed;        // VirtioNetInitTx
>    EFI_PHYSICAL_ADDRESS        RxBufDeviceBase;   // VirtioNetInitRx
> +  ORDERED_COLLECTION          *TxBufMapInfoCollection; // VirtioNetInitTx

(1) Hmm, I like the name, but I don't like the unaligned comment.

I also don't like the idea of moving all the other comments.

Can you just call this field "TxBufCollection"? Then the comment can be
aligned.


>  } VNET_DEV;
>
>
> @@ -281,6 +283,36 @@ VirtioNetUninitRing (
>    );
>
>  //
> +// utility functions to map caller-supplied Tx buffer system physical address
> +// to a device address and vice versa
> +//

I like this comment!

> +EFI_STATUS
> +EFIAPI
> +VirtioNetMapTxBuf (
> +  IN  VNET_DEV              *Dev,
> +  IN  UINT16                DescIdx,
> +  IN  VOID                  *Buffer,
> +  IN  UINTN                 NumberOfBytes,
> +  OUT EFI_PHYSICAL_ADDRESS  *DeviceAddress
> +  );
> +
> +INTN
> +EFIAPI
> +VirtioNetTxMapInfoCompare (
> +  IN CONST VOID *UserStruct1,
> +  IN CONST VOID *UserStruct2
> +  );
> +
> +EFI_STATUS
> +EFIAPI
> +VirtioNetUnmapTxBuf (
> +  IN  VNET_DEV              *Dev,
> +  IN  UINT16                DescIdx,
> +  OUT VOID                  **Buffer,
> +  IN  EFI_PHYSICAL_ADDRESS  DeviceAddress
> +  );
> +

I will comment on these functions in detail below, I just want to say:

- the location of the declarations in this header file is fine, but

(2) Please try to keep the same order between the new functions in
"VirtioNet.h", and in "SnpSharedHelpers.c".


> +//
>  // event callbacks
>  //
>  VOID
> diff --git a/OvmfPkg/VirtioNetDxe/SnpInitialize.c b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
> index 6cedb406a172..a8ffb9a8a7b1 100644
> --- a/OvmfPkg/VirtioNetDxe/SnpInitialize.c
> +++ b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
> @@ -176,6 +176,15 @@ VirtioNetInitTx (
>      return EFI_OUT_OF_RESOURCES;
>    }
>
> +  Dev->TxBufMapInfoCollection = OrderedCollectionInit (
> +                                  VirtioNetTxMapInfoCompare,
> +                                  VirtioNetTxMapInfoCompare
> +                                  );

(3) VirtioNetTxMapInfoCompare is not right for both of the arguments,
but I'll come to that below.


> +  if (Dev->TxBufMapInfoCollection == NULL) {
> +    Status = EFI_OUT_OF_RESOURCES;
> +    goto FreeTxFreeStack;
> +  }
> +

(4) The code is good, but please update the documentation of "@retval
EFI_OUT_OF_RESOURCES".


>    //
>    // Allocate TxSharedReq header and map with BusMasterCommonBuffer so that it
>    // can be accessed equally by both processor and device.
> @@ -186,7 +195,7 @@ VirtioNetInitTx (
>                            &TxSharedReqBuffer
>                            );
>    if (EFI_ERROR (Status)) {
> -    goto FreeTxFreeStack;
> +    goto UninitMapInfoCollection;
>    }
>
>    ZeroMem (TxSharedReqBuffer, sizeof *Dev->TxSharedReq);
> @@ -267,6 +276,10 @@ FreeTxSharedReqBuffer:
>                   EFI_SIZE_TO_PAGES (sizeof *(Dev->TxSharedReq)),
>                   TxSharedReqBuffer
>                   );
> +
> +UninitMapInfoCollection:
> +  OrderedCollectionUninit (Dev->TxBufMapInfoCollection);
> +
>  FreeTxFreeStack:
>    FreePool (Dev->TxFreeStack);
>
> diff --git a/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c b/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
> index 2ec3dc385a9f..dafb538b4b5a 100644
> --- a/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
> +++ b/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
> @@ -18,6 +18,17 @@
>
>  #include "VirtioNet.h"
>
> +//
> +// The user structure for the ordered collection that will track the mapping
> +// info of the packets queued in TxRing
> +//
> +typedef struct {
> +  UINT16                DescIdx;
> +  VOID                  *Buffer;
> +  EFI_PHYSICAL_ADDRESS  DeviceAddress;
> +  VOID                  *BufMap;
> +} TX_BUF_MAP_INFO;
> +

Great, I like the new name for the structure, and the leading comment
too. Let me comment on the fields:

(5) Please drop the DescIdx field. The reverse mapping should occur
strictly from DeviceAddress to Buffer, as we documented in the previous
patch.

DescIdx is indeed *how* we get at DeviceAddress in the next patch
(namely the hypervisor sends us the DescIdx for the head descriptor, we
calculate the DescIdx for the tail descriptor, and then grab
DeviceAddress from the tail descriptor.) However, for the reverse
mapping itself, DescIdx is irrelevant. The key field for the reverse
lookup is DeviceAddress.


(6) Please add a postfix style comment to the DeviceAddress field:

  EFI_PHYSICAL_ADDRESS  DeviceAddress; // lookup key for reverse mapping


>  /**
>    Release RX and TX resources on the boundary of the
>    EfiSimpleNetworkInitialized state.
> @@ -54,6 +65,20 @@ VirtioNetShutdownTx (
>    IN OUT VNET_DEV *Dev
>    )
>  {
> +  ORDERED_COLLECTION_ENTRY *Entry, *Entry2;
> +  TX_BUF_MAP_INFO          *TxBufMapInfo;
> +
> +  for (Entry = OrderedCollectionMin (Dev->TxBufMapInfoCollection);
> +       Entry != NULL;
> +       Entry = Entry2) {
> +    Entry2 = OrderedCollectionNext (Entry);
> +    TxBufMapInfo = (TX_BUF_MAP_INFO *)Entry2;

This assignment is wrong, for two reasons: first, you should be using
Entry, not Entry2, second, this is not how the OrderedCollectionLib API
works.

The OrderedCollectionLib class operates with four objects; from
primitive to abstract/complex:

- ordering key

- user structure (containing an ordering key)

- iterator (pointing to a collection node which in turn points to a user
  structure)

- collection (consisting of all nodes, and also knowing the comparator
  functions for ordering keys and user structures)

If you look over "MdePkg/Include/Library/OrderedCollectionLib.h", the
functions explain what they take and what they output.

Let me go over some of the functions quickly:

- comparator between two user structures: this is needed whenever you
  insert (link) a brand new user structure into the collection. The
  structure being inserted has to be compared against existing
  structures, using the ordering key field in both.

- comparator between a bare ordering key and a full structure: this is
  needed when searching for an existent structure in the collection. For
  the lookup, you don't want to create a fake user structure, just
  provide the bare key itself. So the comparator has to compare the bare
  key against the complete user structures in the collection (using the
  ordering keys embedded into the latter).

- OrderedCollectionUserStruct(): this is a conversion function from
  "iterator" ("entry") to "user structure". Several functions output
  iterators: (a) lookup, (b) minimum, (c) maximum, (d) next, (e) prev.
  Furthermore, (f) "insert" outputs an iterator for the just inserted
  structure, or to the conflicting structure, if there is one.

- find: takes a standalone key and looks up the user structure with that
  key in the collection. Returns an iterator. If you care about the rest
  of the fields in the found structure, you have to conver the iterator
  to user structure, with OrderedCollectionUserStruct().

- min, max, next, prev: should be obvious if you look at the prototypes

- insert: takes a full user structure and links it into the collection,
  if there is no conflict between keys. Optionally outputs an iterator
  for the just-linked user structure (for example you can use that to
  advance to the just-following element in the collection, for whatever
  reason). If there is a conflict (user structure already present with
  that key), the iterator for the conflicting user struct is output
  (optionally). If you care about the rest of the fields in the
  conflicting element, you have to convert the iterator with
  OrderedCollectionUserStruct().

- delete: takes an iterator (which you may have generated in any way
  described above), unlinks the corresponding user structure from the
  tree, and (optionally) hands the user structure to the caller at once.
  The last part is a convenience parameter, because frequently you have
  to unlink a user structure (which invalidates its iterator) and then
  destroy the user structure too. Normally you'd have to call
  OrderedCollectionUserStruct() on the iterator *first* (to get the user
  structure), and call delete *second*, on the same iterator. With the
  convenience parameter, it's OK to call just delete.


(7) Therefore please simply drop this assignment, and below I'll show
how to lay out the loop body.


> +    Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, TxBufMapInfo->BufMap);
> +    FreePool (TxBufMapInfo);
> +    OrderedCollectionDelete (Dev->TxBufMapInfoCollection, Entry, NULL);

(8) OK, so here's how to do the loop body (after assigning "Entry2",
which is good):

  VOID *UserStruct;

  OrderedCollectionDelete (Dev->TxBufMapInfoCollection, Entry, &UserStruct);
  TxBufMapInfo = UserStruct;
  Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, TxBufMapInfo->BufMap);
  FreePool (TxBufMapInfo);


> +  }
> +  OrderedCollectionUninit (Dev->TxBufMapInfoCollection);
> +
>    Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->TxSharedReqMap);
>    Dev->VirtIo->FreeSharedPages (
>                   Dev->VirtIo,

(9) In this function (VirtioNetShutdownTx()), please keep the same order
of teardown as on the error path in VirtioNetInitTx().

Namely, the ordered collection should be emptied and uninited just
before freeing "Dev->TxFreeStack".


> @@ -83,3 +108,166 @@ VirtioNetUninitRing (
>    Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, RingMap);
>    VirtioRingUninit (Dev->VirtIo, Ring);
>  }
> +
> +
> +/**
> +  Map Caller-supplied TxBuf buffer to the device-mapped address
> +
> +  @param[in]    Dev               The VNET_DEV driver instance which wants to
> +                                  map the Tx packet.
> +  @param[in]    DescIdx           VRING descriptor index which will point to
> +                                  the device address

(10) Please drop the "DescIdx" parameter.


> +  @param[in]    Buffer            The system physical address of TxBuf
> +  @param[in]    NumberOfBytes     Number of bytes to map
> +  @param[out]   DeviceAddress     The resulting device address for the bus
> +                                  master access.
> +
> +  @retval EFI_OUT_OF_RESOURCES    The request could not be completed due to
> +                                  a lack of resources.
> +  @retval EFI_INVALID_PARAMETER   The VRING descriptor index is already mapped.

(11) Please drop the EFI_INVALID_PARAMETER retval (more on it below).


(12) Please add "@return  Status codes from
VirtioMapAllBytesInSharedBuffer()".


(13) Please add "@retval EFI_SUCCESS ..." for completeness.


> +*/
> +EFI_STATUS
> +EFIAPI
> +VirtioNetMapTxBuf (
> +  IN  VNET_DEV              *Dev,
> +  IN  UINT16                DescIdx,
> +  IN  VOID                  *Buffer,
> +  IN  UINTN                 NumberOfBytes,
> +  OUT EFI_PHYSICAL_ADDRESS  *DeviceAddress
> +  )
> +{
> +  EFI_STATUS                Status;
> +  TX_BUF_MAP_INFO           *TxBufMapInfo;
> +  EFI_PHYSICAL_ADDRESS      Address;
> +  VOID                      *Mapping;
> +  ORDERED_COLLECTION_ENTRY  *Entry;
> +
> +  TxBufMapInfo = AllocatePool (sizeof (*TxBufMapInfo));
> +  if (TxBufMapInfo == NULL) {
> +    return EFI_OUT_OF_RESOURCES;
> +  }
> +
> +  Status = VirtioMapAllBytesInSharedBuffer (
> +             Dev->VirtIo,
> +             VirtioOperationBusMasterRead,
> +             Buffer,
> +             NumberOfBytes,
> +             &Address,
> +             &Mapping
> +            );
> +  if (EFI_ERROR (Status)) {
> +    goto FreeTxBufMapInfo;
> +  }
> +
> +  TxBufMapInfo->DescIdx = DescIdx;
> +  TxBufMapInfo->Buffer = Buffer;
> +  TxBufMapInfo->DeviceAddress = Address;
> +  TxBufMapInfo->BufMap = Mapping;
> +
> +  Status = OrderedCollectionInsert (
> +             Dev->TxBufMapInfoCollection,
> +             &Entry,
> +             TxBufMapInfo
> +             );
> +  switch (Status) {
> +  case RETURN_OUT_OF_RESOURCES:
> +    Status = EFI_OUT_OF_RESOURCES;
> +    goto UnmapTxBufBuffer;
> +  case RETURN_ALREADY_STARTED:
> +    Status = EFI_INVALID_PARAMETER;
> +    goto UnmapTxBufBuffer;
> +  default:
> +    ASSERT (Status == RETURN_SUCCESS);
> +    break;
> +  }

(14) Given that, in v3, the ordering key will be
"TX_BUF_MAP_INFO.DeviceAddress", the Status check after
OrderedCollectionInsert() should work like this (i.e., replace the
"switch" with the following):

  if (Status == EFI_OUT_OF_RESOURCES) {
    goto UnmapTxBufBuffer;
  }
  ASSERT_EFI_ERROR (Status);

In other words, ALREADY_STARTED should *never* be returned, because the
key comes from VirtioMapAllBytesInSharedBuffer(), and should be unique.
If there is a conflict, then the breakage is so serious that we cannot
do anything about it.


> +  ASSERT (OrderedCollectionUserStruct (Entry) == TxBufMapInfo);

(15) I suggest to drop the "Entry" local variable, together with the
ASSERT() that depends on it.

Also, pass NULL in &Entry's stead to OrderedCollectionInsert().

(... if you really would like to keep the ASSERT(), it's your call,
ultimately.)


> +
> +  *DeviceAddress = Address;
> +
> +  return EFI_SUCCESS;
> +
> +UnmapTxBufBuffer:

(16) I think this label would be nicer if we renamed it to "UnmapTxBuf".

> +  Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Mapping);
> +
> +FreeTxBufMapInfo:
> +  FreePool (TxBufMapInfo);
> +  return Status;
> +}
> +
> +/**
> +  Unmap (aka reverse mapping) device mapped TxBuf buffer to the system
> +  physical address
> +
> +  @param[in]    Dev               The VNET_DEV driver instance which wants to
> +                                  map the Tx packet.

(17) s/map/reverse- and unmap/


> +  @param[in]    DescIdx           VRING descriptor index which point to
> +                                  the device address

(18) Please drop DescIdx


> +  @param[out]   Buffer            The system physical address of TxBuf
> +  @param[out]   DeviceAddress     The device address for the TxBuf

The [out] label on DeviceAddress is wrong, the variable is marked IN
below.

This error in the patch is not surprising, given that the function
doesn't actually use "DeviceAddress" for anything right now.

This will be fixed in v3, where DescIdx disappears from this
representation, and DeviceAddress will become the reverse lookup key.

(19) So please change [out] to [in] on DeviceAddress.


> +
> +  @retval EFI_INVALID_PARAMETER   The VRING descriptor index is not mapped

(20) Please update the comment: "DeviceAddress is not mapped"


(21) Please add

  @retval EFI_SUCCESS  The TxBuf at DeviceAddress has been unmapped, and Buffer
                       has been set to TxBuf's system physical address.


> +*/
> +EFI_STATUS
> +EFIAPI
> +VirtioNetUnmapTxBuf (
> +  IN  VNET_DEV              *Dev,
> +  IN  UINT16                DescIdx,
> +  OUT VOID                  **Buffer,
> +  IN  EFI_PHYSICAL_ADDRESS  DeviceAddress
> +  )
> +{
> +  TX_BUF_MAP_INFO           StandaloneKey;
> +  ORDERED_COLLECTION_ENTRY  *Entry;
> +  TX_BUF_MAP_INFO           *UserStruct;
> +  VOID                      *Ptr;
> +  EFI_STATUS                Status;
> +
> +  StandaloneKey.DescIdx = DescIdx;
> +  Entry = OrderedCollectionFind (Dev->TxBufMapInfoCollection, &StandaloneKey);

Here you actually construct a full user structure, *not* a stand-alone
key.

The stand-alone key (in version 2) is simply the "DescIdx" input
parameter. It "stands alone" because it is not embedded in a user
structure (i.e., a TX_BUF_MAP_INFO object). Therefore, in version 3,

(22) please drop the "StandaloneKey" local variable, and call
OrderedCollectionFind() like this:

  Entry = OrderedCollectionFind (Dev->TxBufMapInfoCollection, &DeviceAddress);

Namely, in version 3, DeviceAddress is the ordering key, and passing it
like above makes it "stand alone".


> +  if (Entry == NULL) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  OrderedCollectionDelete (Dev->TxBufMapInfoCollection, Entry, &Ptr);

(23) This function call is correct, but the variable names can be
improved:

- please rename "UserStruct" to "TxBufMapInfo",

- please rename "Ptr" to "UserStruct".


> +
> +  UserStruct = Ptr;
> +  ASSERT (UserStruct->DescIdx == DescIdx);

(24) drop the ASSERT()

(... well, if you don't trust OrderedCollectionLib, you can keep the
ASSERT() -- same story as point (15), it's your call; but then update
the key field to DeviceAddress)


> +
> +  *Buffer =  UserStruct->Buffer;

(25) Too much whitespace.


> +  Status = Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, UserStruct->BufMap);
> +  FreePool (UserStruct);
> +
> +  return Status;
> +}

Here we unmap a bus master read operation. An unmap operation tears down
the association unconditionally, the only thing that can fail is
committing the device's data to system memory -- but that doesn't make
sense after a bus master read op. Even if we got an error status from
the unmap, the function still succeeds.

(26) Therefore please ignore the return value of UnmapSharedBuffer() --
do not assign it to Status --, and return the EFI_SUCCESS constant.


(27) In fact you can drop the "Status" variable.


> +
> +/**
> +  Comparator function for two user structures.
> +
> +  @param[in] UserStruct1  Pointer to the first user structure.
> +
> +  @param[in] UserStruct2  Pointer to the second user structure.
> +
> +  @retval <0  If UserStruct1 compares less than UserStruct2.
> +
> +  @retval  0  If UserStruct1 compares equal to UserStruct2.
> +
> +  @retval >0  If UserStruct1 compares greater than UserStruct2.
> +*/

(28) Please update the documentation to:

  Comparator function for two TX_BUF_MAP_INFO objects.
  ...
  ... Pointer to the (first|second) TX_BUF_MAP_INFO object

The parameter names themselves (UserStruct1, UserStruct2) are fine.


> +INTN
> +EFIAPI
> +VirtioNetTxMapInfoCompare (

(29) The function name should be "VirtioNetTxBufMapInfoCompare".


> +  IN CONST VOID *UserStruct1,
> +  IN CONST VOID *UserStruct2
> +  )
> +{
> +  CONST TX_BUF_MAP_INFO *MapInfo1;
> +  CONST TX_BUF_MAP_INFO *MapInfo2;
> +
> +  MapInfo1 = UserStruct1;
> +  MapInfo2 = UserStruct2;
> +
> +  return MapInfo1->DescIdx < MapInfo2->DescIdx ? -1 :
> +         MapInfo1->DescIdx > MapInfo2->DescIdx ?  1 :
> +         0;
> +}
>

(30) Please replace the DescIdx field references with "DeviceAddress"
field references.


(31) Please add the following function (to VirtioNet.h as well), and
pass it to OrderedCollectionInit() as second parameter:

---------------
/**
  Compare a standalone DeviceAddress against a TX_BUF_MAP_INFO object
  containing an embedded DeviceAddress.

  @param[in] StandaloneKey  Pointer to DeviceAddress, which has type
                            EFI_PHYSICAL_ADDRESS.

  @param[in] UserStruct     Pointer to the TX_BUF_MAP_INFO object with the
                            embedded DeviceAddress.

  @retval <0  If StandaloneKey compares less than UserStruct's key.

  @retval  0  If StandaloneKey compares equal to UserStruct's key.

  @retval >0  If StandaloneKey compares greater than UserStruct's key.
**/
INTN
EFIAPI
VirtioNetTxBufDeviceAddressCompare (
  IN CONST VOID *StandaloneKey,
  IN CONST VOID *UserStruct
  )
{
  CONST EFI_PHYSICAL_ADDRESS *DeviceAddress;
  CONST TX_BUF_MAP_INFO      *MapInfo;

  DeviceAddress = StandaloneKey;
  MapInfo = UserStruct;

  return *DeviceAddress < MapInfo->DeviceAddress ? -1 :
         *DeviceAddress > MapInfo->DeviceAddress ?  1 :
         0;
}
---------------


Thank you!
Laszlo