Re: [PATCH v2 13/23] OvmfPkg/Virtio: take RingBaseShift in VirtioSetQueueAddress()

[Laszlo Ersek <lersek@redhat.com>]

(1) The "VirtioSetQueueAddress" identifier does not exist in the edk2
tree as a function or protocol member name, so I suggest at least
replacing "VirtioSetQueueAddress" with "SetQueueAddress".

On 08/14/17 13:36, Brijesh Singh wrote:
> 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.

(2) I suggest the following wording in order to keep our references
within the virtio device protocol:

    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.

> - MMIO and legacy virtio device does not use IOMMU to translate the
>   addresses hence RingBaseShift will always be set to zero.

(3) s/device does not use/transports do not support/

>
> - modern virtio device use IOMMU to translate the address, in next patch

(4) s/device use/transport supports/

>   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                         | 5 ++++-
>  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                                 | 3 ++-
>  OvmfPkg/VirtioNetDxe/SnpInitialize.c                            | 2 +-
>  OvmfPkg/VirtioPciDeviceDxe/VirtioPciFunctions.c                 | 5 ++++-
>  OvmfPkg/VirtioRngDxe/VirtioRng.c                                | 2 +-
>  OvmfPkg/VirtioScsiDxe/VirtioScsi.c                              | 2 +-
>  11 files changed, 26 insertions(+), 11 deletions(-)
>
> diff --git a/OvmfPkg/Include/Protocol/VirtioDevice.h b/OvmfPkg/Include/Protocol/VirtioDevice.h
> index 14f980d7bf0a..25fd73b847a5 100644
> --- a/OvmfPkg/Include/Protocol/VirtioDevice.h
> +++ b/OvmfPkg/Include/Protocol/VirtioDevice.h
> @@ -156,6 +156,8 @@ EFI_STATUS
>    @param[in] Ring             The initialized VRING object to take the
>                                addresses from.
>
> +  @param[in] RingBaseShift    The offset for the Ring Base address.
> +

OK, so this comment block is critical.

(5) Please update the documentation of the "Ring" parameter like this:

  @param[in] Ring             The initialized VRING object to take the
                              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().

(This documentation basically provides the specification for our
VirtioRingMap() function.)

(6) Then we can simply define RingBaseShift like this:

  @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
>                                provided address offset and write size.
> @@ -164,7 +166,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 b69f6d7b7a85..b5cc091fe820 100644
> --- a/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDevice.h
> +++ b/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDevice.h
> @@ -113,7 +113,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 f3f69f324c6c..3d14b1035e91 100644
> --- a/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceFunctions.c
> +++ b/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceFunctions.c
> @@ -183,11 +183,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 413ffa06cf35..d102e1fd8551 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..df8ded99edff 100644
> --- a/OvmfPkg/VirtioGpuDxe/Commands.c
> +++ b/OvmfPkg/VirtioGpuDxe/Commands.c
> @@ -132,7 +132,8 @@ VirtioGpuInit (
>    if (EFI_ERROR (Status)) {
>      goto Failed;
>    }
> -  Status = VgpuDev->VirtIo->SetQueueAddress (VgpuDev->VirtIo, &VgpuDev->Ring);
> +  Status = VgpuDev->VirtIo->SetQueueAddress (VgpuDev->VirtIo,
> +                              &VgpuDev->Ring, 0);

(7) Please break all arguments to separate lines (and the closing paren
to another separate line).

The rest looks fine, thanks!
Laszlo

>    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 4597095deb78..86f752e1651f 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;
>    }
>