Re: [PATCH 4/5] OvmfPkg/VirtioNetDxe: map virtio-net transmit request buffer

[Laszlo Ersek <lersek@redhat.com>]

On 09/05/17 14:41, Laszlo Ersek wrote:
> On 09/01/17 13:24, Brijesh Singh wrote:
>> When device is behind the IOMMU, driver is require to pass the device
>> address of transmit buffer for the bus master operations.
>>
>> The patch uses VirtioMapAllBytesInSharedBuffer() to map transmit buffer
>> system physical address to the device address.
>>
>> Since the transmit buffers are returned back to caller in
>> VirtioNetGetStatus() hence we use OrderCollection library interface to
>> save the host to device address mapping. After the buffer is succesfully
>> transmited we do reverse lookup in OrderCollection data structure to get
>> the host address for the transmitted 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        |  19 +++
>>  OvmfPkg/VirtioNetDxe/SnpGetStatus.c     |  30 +++-
>>  OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c | 157 ++++++++++++++++++++
>>  OvmfPkg/VirtioNetDxe/SnpTransmit.c      |  37 ++++-
>>  5 files changed, 232 insertions(+), 12 deletions(-)

> (7) We'll have to document the new model in "TechNotes.txt".

I've now re-read the final section of the text file, section

  Virtio internals -- Tx

I propose the following updates.

(Here I'm providing a diff, not a desired "end status", like I did for
the diagram earlier. A diff is harder to interpret for a diagram, but
easy for plain text.)

Please verify that the proposed documentation updates match the logic
that you've implemented:

> diff --git a/OvmfPkg/VirtioNetDxe/TechNotes.txt b/OvmfPkg/VirtioNetDxe/TechNotes.txt
> index 9c1dfe6a773e..7a7b3071abba 100644
> --- a/OvmfPkg/VirtioNetDxe/TechNotes.txt
> +++ b/OvmfPkg/VirtioNetDxe/TechNotes.txt
> @@ -310,10 +310,14 @@ in the following:
>    that is shared by all of the head descriptors. This virtio-net request header
>    is never modified by the host.
>
> -- Each tail descriptor is re-pointed to the caller-supplied packet buffer
> -  whenever VirtioNetTransmit places the corresponding head descriptor on the
> -  Available Ring. The caller is responsible to hang on to the unmodified buffer
> -  until it is reported transmitted by VirtioNetGetStatus.
> +- Each tail descriptor is re-pointed to the device-mapped address of the
> +  caller-supplied packet buffer whenever VirtioNetTransmit places the
> +  corresponding head descriptor on the Available Ring. A reverse mapping, from
> +  the device-mapped address to the caller-supplied packet address, is saved in
> +  an associative data structure that belongs to the driver instance.
> +
> +- Per spec, the caller is responsible to hang on to the unmodified packet
> +  buffer until it is reported transmitted by VirtioNetGetStatus.
>
>  Steps of packet transmission:
>
> @@ -336,9 +340,11 @@ Steps of packet transmission:
>  - Client code calls VirtioNetGetStatus. In case the Used Ring is empty, the
>    function reports no Tx completion. Otherwise, a head descriptor's index is
>    consumed from the Used Ring and recycled to the private stack. The client
> -  code's original packet buffer address is fetched from the tail descriptor
> -  (where it has been stored at VirtioNetTransmit time) and returned to the
> -  caller.
> +  code's original packet buffer address is calculated by fetching the
> +  device-mapped address from the tail descriptor (where it has been stored at
> +  VirtioNetTransmit time), and by looking up the device-mapped address in the
> +  associative data structure. The reverse-mapped packet buffer address is
> +  returned to the caller.
>
>  - The Len field of the Used Ring Element is not checked. The host is assumed to
>    have transmitted the entire packet -- VirtioNetTransmit had forced it below

Thanks!
Laszlo