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

[Laszlo Ersek <lersek@redhat.com>]

On 09/13/17 20:07, Laszlo Ersek wrote:
> On 09/11/17 14:16, Brijesh Singh wrote:

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

I'd like to elaborate on my above comment.

Let's consider what happens when client code calls SNP.Transmit()
several times, in quick succession, using the *exact same* Buffer
argument -- for queueing the same packet several times, for whatever
reason --, *AND* we are using a virtio protocol implementation that
identity-maps the packets.

That means ALREADY_STARTED *will* be returned, because DeviceAddress
will not be unique.

The question is: is this a valid use of SNP.Transmit(), so that we must
accommodate it?

In order to answer this, let's look at the SNP.GetStatus() interface.
SNP.GetStatus() reports transmit completion by returning the original
TxBuf address. From the UEFI-2.7 spec, "EFI_SIMPLE_NETWORK.GetStatus()":

    If TxBuf is not NULL, a recycled transmit buffer address will be
    retrieved. If a recycled transmit buffer address is returned in
    TxBuf, then the buffer has been successfully transmitted, and the
    status for that buffer is cleared.

It is clear that the transmit buffer address shall uniquely identify the
transmit buffer; and that given a transmit buffer address, there is
exactly *one status* for that transmit buffer / transmit buffer address.

Therefore the use pattern I described above is invalid.

However, to be on the safe side, even in RELEASE builds, I suggest that
we keep your original error handling code, with the following
modification (note that I'm replacing RETURN_ with EFI_, because we're
already investigating an EFI_STATUS variable):

--------
  switch (Status) {
  case EFI_OUT_OF_RESOURCES:
    goto UnmapTxBufBuffer;
  case EFI_ALREADY_STARTED:
    //
    // This should never happen: it implies
    //
    // - an identity-mapping VIRTIO_DEVICE_PROTOCOL.MapSharedBuffer()
    //   implementation -- which is fine,
    //
    // - and an SNP client that queues multiple instances of the exact same
    //   buffer address with SNP.Transmit() -- which is undefined behavior,
    //   based on the TxBuf language in UEFI-2.7,
    //   EFI_SIMPLE_NETWORK.GetStatus().
    //
    ASSERT (FALSE);
    Status = EFI_INVALID_PARAMETER;
    goto UnmapTxBufBuffer;
  default:
    ASSERT_EFI_ERROR (Status);
    break;
  }
--------

Thanks!
Laszlo