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.biesheu...@linaro.org>
> Cc: Jordan Justen <jordan.l.jus...@intel.com>
> Cc: Tom Lendacky <thomas.lenda...@amd.com>
> Cc: Laszlo Ersek <ler...@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Brijesh Singh <brijesh.si...@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);
> +}
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
