Re: Please help me understand VIRTIO_F_IOMMU_PLATFORM

[Mark Cave-Ayland]

On 31/07/2021 16:41, Jason Thorpe wrote:

(added Michael on CC)

> Hey folks —
>
> I’d like to be able to use VirtIO with qemu-system-alpha but, at least on a NetBSD 
> x86_64 host, it does not currently work.  This is because virtio_bus_device_plugged() 
> in hw/virtio/virtio-bus.c ends up picking address_space_memory as the DMA address 
> space for the VirtIODevice.  This does not work for alpha because the CPU and PCI 
> have different views of system memory.  All that’s needed to fix it is for 
> virtio_bus_device_plugged() to call klass->get_dma_as(qbus->parent), but the 
> code only does that if:
>
>         bool has_iommu = virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM);
>
> So, obviously, VIRTIO_F_IOMMU_PLATFORM is not getting set for an emulated alpha 
> system, despite the alpha platform having one[*].  But it’s not clear to me 
> that it’s appropriate for alpha to use VIRTIO_F_IOMMU_PLATFORM, at least from 
> my reading of how it’s used.
>
> In any case, the following extremely simple change allows me to use VirtIO 
> devices in qemu-system-alpha with a NetBSD/alpha guest (and I’m told this also 
> fixes using VirtIO devices in qemu-system-sparc64 for a NetBSD/sparc64 guest):
>
> diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
> index 859978d248..c083e8d737 100644
> --- a/hw/virtio/virtio-bus.c
> +++ b/hw/virtio/virtio-bus.c
> @@ -85,6 +85,8 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error 
> **errp)
>       if (klass->get_dma_as != NULL && has_iommu) {
>           virtio_add_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM);
>           vdev->dma_as = klass->get_dma_as(qbus->parent);
> +    } else if (klass->get_dma_as != NULL) {
> +        vdev->dma_as = klass->get_dma_as(qbus->parent);
>       } else {
>           vdev->dma_as = &address_space_memory;
>       }
>
> So, VirtIO experts, please weigh in on the correctness of this change… if it 
> is, I’ll post the patch formally.
>
> [*] The way the alpha platform works is that the IOMMU is used if the PCI 
> device performs a memory access to a DMA window where SGMAPs are enabled.  If 
> SGMAPs are not enabled in the DMA window the PCI device is accessing, the 
> translation is performed directly by subtracting the address from the window’s 
> Window Base and appending the result to the window’s Translated Base.  A 
> typical alpha PCI platform has a 1GB DMA window at 1GB from the PCI’s 
> perspective, which maps to 0-1GB in the system address map, and an alpha system 
> with 1GB or less of RAM would thus not need to use the IOMMU, but the 
> translation take place regardless.
>
> -- thorpej

Hi Jason,

Thanks for looking at this! I've had previous discussions with Martin trying to 
figure out why virtio-blk-pci doesn't work with Netbsd/sparc64 so glad that you've 
been able to find the underlying cause.

My read on VIRTIO_F_IOMMU_PLATFORM is that this is declaring host IOMMU support so 
the implementation on the guest should not be relevant here.

Testing Linux/sparc64 boot from a virtio-blk-pci device on current git master shows I 
can boot from a virtio-blk-pci device without any problem, even though the existing 
code fails the has_iommu check and vdev->dma_as gets set to address_space_memory 
which is why I haven't spotted this before.

Stepping through the code shows that klass->get_dma_as() is pointing to 
virtio_pci_get_dma_as() which in turn returns pci_get_address_space(dev) which looks 
correct to me. I think that the logic to set vdev->dma_as is incorrect here since 
qemu-system-sparc64 has an emulated IOMMU with its own address space independent of 
whether the host has an IOMMU.

I modified your patch slightly as below and confirmed that I can still boot 
Linux/sparc64 here:

diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
index 859978d248..ee178b8223 100644
--- a/hw/virtio/virtio-bus.c
+++ b/hw/virtio/virtio-bus.c
@@ -82,9 +82,11 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error 
**errp)
         return;
     }

-    if (klass->get_dma_as != NULL && has_iommu) {
-        virtio_add_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM);
+    if (klass->get_dma_as != NULL) {
         vdev->dma_as = klass->get_dma_as(qbus->parent);
+        if (has_iommu) {
+            virtio_add_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM);
+        }
     } else {
         vdev->dma_as = &address_space_memory;
     }

Michael: can you comment further on whether the analysis and patch above are 
correct?


ATB,

Mark.