Re: [PATCH v3 07/23] OvmfPkg/Virtio: take RingBaseShift in SetQueueAddress()

[Laszlo Ersek <lersek@redhat.com>]

On 08/23/17 14:22, Brijesh Singh wrote:
> For the case when an IOMMU is used for translating system physical
> addresses to DMA bus master addresses, the transport-independent
> virtio device drivers will be required to map their VRING areas to
> bus addresses with VIRTIO_DEVICE_PROTOCOL.MapSharedBuffer() calls.
> 
> VirtioRingMap() maps the ring buffer system physical to a bus address.
> When an IOMMU is used for translating the address then bus address can
> start at a different offset from the system physical address.

(1) The paragraph that you now have as first paragraph above was my
suggestion, so thank you for picking it up. However, the second
paragraph should have been deleted; I suggested the now-first paragraph
as a replacement for the now-second one.

I wrote, "to keep our references within the virtio device protocol".
VirtioRingMap() is a VirtioLib function, which is a utility layer on top
of the virtio device protocol. So, as I said, VirtioLib patches may
refer to both VirtioLib and the protocol, but protocol patches should
preferably only refer to the protocol, and not VirtioLib.

  VirtioLib --+
   |  ^       |
   |  |       |
   |  +-------+
   |
   v
  VirtioDeviceProtocol --+
                ^        |
                |        |
                +--------+

This is also consistent with the reordering of the patches that I asked
for (and that you implemented well in v3, thank you for it).

So, apologies if I wasn't clear enough of this -- it's not a big deal at
all, I can remove the second paragraph when I push this.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks!
Laszlo

> 
> - MMIO and legacy virtio transport do not support IOMMU to translate the
>   addresses hence RingBaseShift will always be set to zero.
> 
> - modern virtio transport supports IOMMU to translate the address, in
>   next patch we will update the Virtio10Dxe to use RingBaseShift offset.
> 
> Suggested-by: Laszlo Ersek <lersek@redhat.com>
> 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/Protocol/VirtioDevice.h                         | 19 +++++++++++++++++--
>  OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDevice.h          |  3 ++-
>  OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.h                    |  3 ++-
>  OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceFunctions.c |  5 ++++-
>  OvmfPkg/Virtio10Dxe/Virtio10.c                                  |  5 ++++-
>  OvmfPkg/VirtioBlkDxe/VirtioBlk.c                                |  2 +-
>  OvmfPkg/VirtioGpuDxe/Commands.c                                 |  6 +++++-
>  OvmfPkg/VirtioNetDxe/SnpInitialize.c                            |  2 +-
>  OvmfPkg/VirtioPciDeviceDxe/VirtioPciFunctions.c                 |  5 ++++-
>  OvmfPkg/VirtioRngDxe/VirtioRng.c                                |  2 +-
>  OvmfPkg/VirtioScsiDxe/VirtioScsi.c                              |  2 +-
>  11 files changed, 42 insertions(+), 12 deletions(-)
> 
> diff --git a/OvmfPkg/Include/Protocol/VirtioDevice.h b/OvmfPkg/Include/Protocol/VirtioDevice.h
> index 9a01932958a2..2e3a6d6edf04 100644
> --- a/OvmfPkg/Include/Protocol/VirtioDevice.h
> +++ b/OvmfPkg/Include/Protocol/VirtioDevice.h
> @@ -156,7 +156,21 @@ EFI_STATUS
>    @param[in] This             This instance of VIRTIO_DEVICE_PROTOCOL
>  
>    @param[in] Ring             The initialized VRING object to take the
> -                              addresses from.
> +                              addresses from. The caller is responsible for
> +                              ensuring that on input, all Ring->NumPages pages,
> +                              starting at Ring->Base, have been successfully
> +                              mapped with a single call to
> +                              This->MapSharedBuffer() for CommonBuffer bus
> +                              master operation..
> +
> +  @param[in] RingBaseShift    Adding this value using UINT64 arithmetic to the
> +                              addresses found in Ring translates them from
> +                              system memory to bus addresses. The caller shall
> +                              calculate RingBaseShift as
> +                              (DeviceAddress - (UINT64)(UINTN)HostAddress),
> +                              where DeviceAddress and HostAddress (i.e.,
> +                              Ring->Base) were output and input parameters of
> +                              This->MapSharedBuffer(), respectively.
>  
>    @retval EFI_SUCCESS         The data was written successfully.
>    @retval EFI_UNSUPPORTED     The underlying IO device doesn't support the
> @@ -166,7 +180,8 @@ typedef
>  EFI_STATUS
>  (EFIAPI *VIRTIO_SET_QUEUE_ADDRESS) (
>    IN VIRTIO_DEVICE_PROTOCOL  *This,
> -  IN VRING                   *Ring
> +  IN VRING                   *Ring,
> +  IN UINT64                  RingBaseShift
>    );
>  
>  /**
> diff --git a/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDevice.h b/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDevice.h
> index e5881d537f09..e6279159f8ba 100644
> --- a/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDevice.h
> +++ b/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDevice.h
> @@ -115,7 +115,8 @@ VirtioMmioSetQueueSel (
>  EFI_STATUS
>  VirtioMmioSetQueueAddress (
>    IN VIRTIO_DEVICE_PROTOCOL  *This,
> -  IN VRING                   *Ring
> +  IN VRING                   *Ring,
> +  IN UINT64                  RingBaseShift
>    );
>  
>  EFI_STATUS
> diff --git a/OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.h b/OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.h
> index 41df5a98e560..1f0dc45d501e 100644
> --- a/OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.h
> +++ b/OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.h
> @@ -126,7 +126,8 @@ EFI_STATUS
>  EFIAPI
>  VirtioPciSetQueueAddress (
>    IN VIRTIO_DEVICE_PROTOCOL  *This,
> -  IN VRING                   *Ring
> +  IN VRING                   *Ring,
> +  IN UINT64                  RingBaseShift
>    );
>  
>  EFI_STATUS
> diff --git a/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceFunctions.c b/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceFunctions.c
> index 644ec65e1788..67458e56231b 100644
> --- a/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceFunctions.c
> +++ b/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceFunctions.c
> @@ -181,11 +181,14 @@ VirtioMmioSetQueueSel (
>  EFI_STATUS
>  VirtioMmioSetQueueAddress (
>    IN VIRTIO_DEVICE_PROTOCOL  *This,
> -  IN VRING                   *Ring
> +  IN VRING                   *Ring,
> +  IN UINT64                  RingBaseShift
>    )
>  {
>    VIRTIO_MMIO_DEVICE *Device;
>  
> +  ASSERT (RingBaseShift == 0);
> +
>    Device = VIRTIO_MMIO_DEVICE_FROM_VIRTIO_DEVICE (This);
>  
>    VIRTIO_CFG_WRITE (Device, VIRTIO_MMIO_OFFSET_QUEUE_PFN,
> diff --git a/OvmfPkg/Virtio10Dxe/Virtio10.c b/OvmfPkg/Virtio10Dxe/Virtio10.c
> index 89ccac8c1c04..ef9a00710668 100644
> --- a/OvmfPkg/Virtio10Dxe/Virtio10.c
> +++ b/OvmfPkg/Virtio10Dxe/Virtio10.c
> @@ -489,7 +489,8 @@ EFI_STATUS
>  EFIAPI
>  Virtio10SetQueueAddress (
>    IN VIRTIO_DEVICE_PROTOCOL  *This,
> -  IN VRING                   *Ring
> +  IN VRING                   *Ring,
> +  IN UINT64                  RingBaseShift
>    )
>  {
>    VIRTIO_1_0_DEV *Dev;
> @@ -497,6 +498,8 @@ Virtio10SetQueueAddress (
>    UINT64         Address;
>    UINT16         Enable;
>  
> +  ASSERT (RingBaseShift == 0);
> +
>    Dev = VIRTIO_1_0_FROM_VIRTIO_DEVICE (This);
>  
>    Address = (UINTN)Ring->Desc;
> diff --git a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
> index 61b9cab4ff02..bff15fe3add1 100644
> --- a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
> +++ b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
> @@ -745,7 +745,7 @@ VirtioBlkInit (
>    //
>    // step 4c -- Report GPFN (guest-physical frame number) of queue.
>    //
> -  Status = Dev->VirtIo->SetQueueAddress (Dev->VirtIo, &Dev->Ring);
> +  Status = Dev->VirtIo->SetQueueAddress (Dev->VirtIo, &Dev->Ring, 0);
>    if (EFI_ERROR (Status)) {
>      goto ReleaseQueue;
>    }
> diff --git a/OvmfPkg/VirtioGpuDxe/Commands.c b/OvmfPkg/VirtioGpuDxe/Commands.c
> index c2e4d72feb67..5cb003161207 100644
> --- a/OvmfPkg/VirtioGpuDxe/Commands.c
> +++ b/OvmfPkg/VirtioGpuDxe/Commands.c
> @@ -132,7 +132,11 @@ VirtioGpuInit (
>    if (EFI_ERROR (Status)) {
>      goto Failed;
>    }
> -  Status = VgpuDev->VirtIo->SetQueueAddress (VgpuDev->VirtIo, &VgpuDev->Ring);
> +  Status = VgpuDev->VirtIo->SetQueueAddress (
> +                              VgpuDev->VirtIo,
> +                              &VgpuDev->Ring,
> +                              0
> +                              );
>    if (EFI_ERROR (Status)) {
>      goto ReleaseQueue;
>    }
> diff --git a/OvmfPkg/VirtioNetDxe/SnpInitialize.c b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
> index 6d9b81a9f939..0ecfe044a977 100644
> --- a/OvmfPkg/VirtioNetDxe/SnpInitialize.c
> +++ b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
> @@ -96,7 +96,7 @@ VirtioNetInitRing (
>    //
>    // step 4c -- report GPFN (guest-physical frame number) of queue
>    //
> -  Status = Dev->VirtIo->SetQueueAddress (Dev->VirtIo, Ring);
> +  Status = Dev->VirtIo->SetQueueAddress (Dev->VirtIo, Ring, 0);
>    if (EFI_ERROR (Status)) {
>      goto ReleaseQueue;
>    }
> diff --git a/OvmfPkg/VirtioPciDeviceDxe/VirtioPciFunctions.c b/OvmfPkg/VirtioPciDeviceDxe/VirtioPciFunctions.c
> index bd912cca9b29..b52060d13d97 100644
> --- a/OvmfPkg/VirtioPciDeviceDxe/VirtioPciFunctions.c
> +++ b/OvmfPkg/VirtioPciDeviceDxe/VirtioPciFunctions.c
> @@ -183,11 +183,14 @@ EFI_STATUS
>  EFIAPI
>  VirtioPciSetQueueAddress (
>    IN VIRTIO_DEVICE_PROTOCOL  *This,
> -  IN VRING                   *Ring
> +  IN VRING                   *Ring,
> +  IN UINT64                  RingBaseShift
>    )
>  {
>    VIRTIO_PCI_DEVICE *Dev;
>  
> +  ASSERT (RingBaseShift == 0);
> +
>    Dev = VIRTIO_PCI_DEVICE_FROM_VIRTIO_DEVICE (This);
>  
>    return VirtioPciIoWrite (Dev, VIRTIO_PCI_OFFSET_QUEUE_ADDRESS, sizeof (UINT32),
> diff --git a/OvmfPkg/VirtioRngDxe/VirtioRng.c b/OvmfPkg/VirtioRngDxe/VirtioRng.c
> index e20602ac7225..0abca488e6cd 100644
> --- a/OvmfPkg/VirtioRngDxe/VirtioRng.c
> +++ b/OvmfPkg/VirtioRngDxe/VirtioRng.c
> @@ -298,7 +298,7 @@ VirtioRngInit (
>    //
>    // step 4c -- Report GPFN (guest-physical frame number) of queue.
>    //
> -  Status = Dev->VirtIo->SetQueueAddress (Dev->VirtIo, &Dev->Ring);
> +  Status = Dev->VirtIo->SetQueueAddress (Dev->VirtIo, &Dev->Ring, 0);
>    if (EFI_ERROR (Status)) {
>      goto ReleaseQueue;
>    }
> diff --git a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> index c2f6f412ff40..a983b3df7b9c 100644
> --- a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> +++ b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> @@ -855,7 +855,7 @@ VirtioScsiInit (
>    //
>    // step 4c -- Report GPFN (guest-physical frame number) of queue.
>    //
> -  Status = Dev->VirtIo->SetQueueAddress (Dev->VirtIo, &Dev->Ring);
> +  Status = Dev->VirtIo->SetQueueAddress (Dev->VirtIo, &Dev->Ring, 0);
>    if (EFI_ERROR (Status)) {
>      goto ReleaseQueue;
>    }
>