Re: [PATCH v1 06/14] OvmfPkg/VirtioLib: Add functions to map/unmap VRING

[Laszlo Ersek <lersek@redhat.com>]

First some superficial comments, then some more serious ones.

On 08/07/17 13:58, Brijesh Singh wrote:
> Add functions to map and unmap the ring buffer with BusMasterCommonBuffer.
> 
> 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/Include/Library/VirtioLib.h   | 41 +++++++++++++++
>  OvmfPkg/Library/VirtioLib/VirtioLib.c | 52 ++++++++++++++++++++
>  2 files changed, 93 insertions(+)
> 
> diff --git a/OvmfPkg/Include/Library/VirtioLib.h b/OvmfPkg/Include/Library/VirtioLib.h
> index 610654225de7..877961af0514 100644
> --- a/OvmfPkg/Include/Library/VirtioLib.h
> +++ b/OvmfPkg/Include/Library/VirtioLib.h
> @@ -62,6 +62,47 @@ VirtioRingInit (
>  
>  /**
>  
> +  Map the ring buffer so that it can be accessed equally by both guest
> +  and hypervisor.
> +
> +  @param[in]      VirtIo    The virtio device instance.
> +
> +  @param[in]      Ring      The virtio ring to map.
> +
> +  @param[out]     Mapping   A resulting value to pass to Unmap().

(1) Please say "token" here (to be consistent with my earlier request).

(2) Please replace Unmap() with VirtIo->UnmapSharedBuffer().

> +
> +  @retval         Value returned from VirtIo->MapSharedBuffer()

(3) This should be @return, not @retval. @retval is for concrete
constants. Also I would suggest "Status code" rather than "Value".

(4) Please sync the comment block in the C file.

> +**/
> +EFI_STATUS
> +EFIAPI
> +VirtioRingMap (
> +  IN  VIRTIO_DEVICE_PROTOCOL *VirtIo,
> +  IN  VRING                  *Ring,
> +  OUT VOID                   **Mapping
> +  );
> +
> +/**
> +
> +  Unmap the ring buffer mapped using VirtioRingMap()
> +
> +  @param[in]      VirtIo    The virtio device instance.
> +
> +  @param[in]      Ring      The virtio ring to unmap.
> +
> +  @param[in]      Mapping   A value obtained through Map().
> +
> +  @retval         Value returned from VirtIo->UnmapSharedBuffer()
> +**/
> +EFI_STATUS
> +EFIAPI
> +VirtioRingUnmap (
> +  IN  VIRTIO_DEVICE_PROTOCOL *VirtIo,
> +  IN  VRING                  *Ring,
> +  IN  VOID                   *Mapping
> +  );

(5) Please drop this function. It doesn't save us anything. (The Ring
parameter isn't even used in the implementation.)

> +
> +/**
> +
>    Tear down the internal resources of a configured virtio ring.
>  
>    The caller is responsible to stop the host from using this ring before
> diff --git a/OvmfPkg/Library/VirtioLib/VirtioLib.c b/OvmfPkg/Library/VirtioLib/VirtioLib.c
> index 50e5ec28ea1b..09a3f9b7f2e5 100644
> --- a/OvmfPkg/Library/VirtioLib/VirtioLib.c
> +++ b/OvmfPkg/Library/VirtioLib/VirtioLib.c
> @@ -640,3 +640,55 @@ VirtioUnmapSharedBuffer (
>  {
>    return VirtIo->UnmapSharedBuffer (VirtIo, Mapping);
>  }
> +
> +/**
> +
> +  Map the ring buffer so that it can be accessed equally by both guest
> +  and hypervisor.
> +
> +  @param[in]      VirtIo    The virtio device instance.
> +
> +  @param[in]      Ring      The virtio ring to map.
> +
> +  @param[out]     Mapping   A resulting value to pass to Unmap().
> +
> +  @retval         Value returned from VirtIo->MapSharedBuffer()
> +**/
> +EFI_STATUS
> +EFIAPI
> +VirtioRingMap (
> +  IN  VIRTIO_DEVICE_PROTOCOL *VirtIo,
> +  IN  VRING                  *Ring,
> +  OUT VOID                   **Mapping
> +  )
> +{
> +  UINTN                 NumberOfBytes;
> +
> +  NumberOfBytes = Ring->NumPages * EFI_PAGE_SIZE;

(6) "EFI_PAGES_TO_SIZE (Ring->NumPages)" is more idiomatic.

> +
> +  return VirtioMapSharedBufferCommon (VirtIo, Ring->Base,
> +           NumberOfBytes, Mapping);

(7) So, based on my earlier request, this should become a call to
VirtioMapAllBytesInSharedBuffer(). Please break every argument to a
separate line.

Now, my important comments for this patch. :)

VirtioMapAllBytesInSharedBuffer() will give you a DeviceAddress. Here's
what we should do with it:


(8) Please introduce a new patch that modifies the
VIRTIO_SET_QUEUE_ADDRESS typedef, in
"OvmfPkg/Include/Protocol/VirtioDevice.h". Namely, please add a third
parameter:

  IN UINT64 RingBaseShift

In this initial patch, simply update all call sites to pass in 0.

In parallel (in the same initial patch), update all three implementations:
- VirtioMmioSetQueueAddress()
[OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceFunctions.c]
- VirtioPciSetQueueAddress()
[OvmfPkg/VirtioPciDeviceDxe/VirtioPciFunctions.c]
- Virtio10SetQueueAddress() [OvmfPkg/Virtio10Dxe/Virtio10.c]

as follows:
- accept the new parameter (obviously),
- simply assert that it is zero, at the top of the function.


(9) Modify Virtio10SetQueueAddress() as follows, in another separate patch:

- every time after the local variable "Address" is assigned, please insert:

  Address += RingBaseShift;

This is automatically going to happen in UINT64 arithmetic, which has
well-defined wrap-around semantics. Three such insertions will be necessary.

- remove the ASSERT added in (8)


(10) Then please include the present patch, with the following update
(beyond the changes requested above):

- Add the following output parameter to VirtioRingMap():

  OUT UINT64 *RingBaseShift

- In the implementation, assign it like this, if
VirtioMapAllBytesInSharedBuffer() succeeds:

  *RingBaseShift = DeviceAddress - (UINT64)(UINTN)Ring->Base;

(DeviceAddress has type EFI_PHYSICAL_ADDRESS, which is equivalent to
UINT64, on the spec level.)


(11) Please make sure that no output parameter is modified before we can
guarantee returning EFI_SUCCESS.


(12) In the individual virtio device drivers, whenever you insert the
VirtioRingMap() function call, carry "RingBaseShift" from
VirtioRingMap() to VirtIo->SetQueueAddress() in a new local UINT64 variable.

End result:
- the MMIO and legacy PCI transport implementations will always set
DeviceAddress to HostAddress in their no-op MapSharedBuffer() member
functions,
- therefore VirtioRingMap() will set RingBaseShift to zero,
- the ASSERT()s in the SetQueueAddress() members of those same transport
implementations will be satisfied,
- with the modern-only (1.0) transport, RingBaseShift will also be zero
(because of the SEV IOMMU that we have), and "Address += 0" will have no
effect,
- if we ever add an IOMMU driver that translates addresses, the
translation will happen automatically in the modern-only transport.

Thanks,
Laszlo


> +}
> +
> +/**
> +
> +  Unmap the ring buffer mapped using VirtioRingMap()
> +
> +  @param[in]      VirtIo    The virtio device instance.
> +
> +  @param[in]      Ring      The virtio ring to unmap.
> +
> +  @param[in]      Mapping   A value obtained through Map().
> +
> +  @retval         Value returned from VirtIo->UnmapSharedBuffer()
> +**/
> +EFI_STATUS
> +EFIAPI
> +VirtioRingUnmap (
> +  IN  VIRTIO_DEVICE_PROTOCOL *VirtIo,
> +  IN  VRING                  *Ring,
> +  IN  VOID                   *Mapping
> +  )
> +{
> +  return VirtioUnmapSharedBuffer (VirtIo, Mapping);
> +}
>