public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: Laszlo Ersek <lersek@redhat.com>
To: Brijesh Singh <brijesh.singh@amd.com>, edk2-devel@lists.01.org
Cc: Jordan Justen <jordan.l.justen@intel.com>,
	Ard Biesheuvel <ard.biesheuvel@linaro.org>,
	Tom Lendacky <thomas.lendacky@amd.com>
Subject: Re: [PATCH v2 6/8] OvmfPkg/VirtioNetDxe: add Tx packet map/unmap helper functions
Date: Wed, 13 Sep 2017 21:24:09 +0200	[thread overview]
Message-ID: <2adc266a-949d-c546-9bed-d78fea19d969@redhat.com> (raw)
In-Reply-To: <6c148d15-a98e-59c0-9b9e-227f3456233a@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


  reply	other threads:[~2017-09-13 19:21 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-09-11 12:16 [PATCH v2 0/8] OvmfPkg/VirtioNetDxe: map host address to device address Brijesh Singh
2017-09-11 12:16 ` [PATCH v2 1/8] OvmfPkg/VirtioNetDxe: add helper VirtioNetUninitRing() Brijesh Singh
2017-09-13  7:26   ` Laszlo Ersek
2017-09-11 12:16 ` [PATCH v2 2/8] OvmfPkg/VirtioNetDxe: map VRINGs using VirtioRingMap() Brijesh Singh
2017-09-13  8:07   ` Laszlo Ersek
2017-09-11 12:16 ` [PATCH v2 3/8] OvmfPkg/VirtioNetDxe: alloc RxBuf using AllocateSharedPages() Brijesh Singh
2017-09-13  9:14   ` Laszlo Ersek
2017-09-11 12:16 ` [PATCH v2 4/8] OvmfPkg/VirtioNetDxe: dynamically alloc transmit header Brijesh Singh
2017-09-13 14:31   ` Laszlo Ersek
2017-09-11 12:16 ` [PATCH v2 5/8] OvmfPkg/VirtioNetDxe: update TechNotes Brijesh Singh
2017-09-13 14:40   ` Laszlo Ersek
2017-09-11 12:16 ` [PATCH v2 6/8] OvmfPkg/VirtioNetDxe: add Tx packet map/unmap helper functions Brijesh Singh
2017-09-13 18:07   ` Laszlo Ersek
2017-09-13 19:24     ` Laszlo Ersek [this message]
2017-09-11 12:16 ` [PATCH v2 7/8] OvmfPkg/VirtioNetDxe: map caller-supplied Tx packet to device-address Brijesh Singh
2017-09-13 20:15   ` Laszlo Ersek
2017-09-11 12:16 ` [PATCH v2 8/8] OvmfPkg/VirtioNetDxe: negotiate VIRTIO_F_IOMMU_PLATFORM Brijesh Singh

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=2adc266a-949d-c546-9bed-d78fea19d969@redhat.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox