Re: Please help me understand VIRTIO_F_IOMMU_PLATFORM

[Jason Thorpe]

> On Aug 18, 2021, at 12:58 AM, Mark Cave-Ayland 
> <mark.cave-ayland@ilande.co.uk> wrote:
> 
> On 31/07/2021 16:41, Jason Thorpe wrote:
> 
> (added Michael on CC)
> 
> 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.

That’s basically the conclusion I came to, as well, but I still wasn’t quite 
sure how the host IOMMU was relevant so I wasn’t quite willing to stick my neck 
out :-)

> 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.

Right, this more-or-less the same situation as qemu-system-alpha.  I’m curious 
why the Linux/sparc64 guest is able to use VirtIO without the patch, though.  I 
guess that VirtIO client implementation is skipping the normal platform DMA 
mapping step and is just poking physical addresses in directly?  The NetBSD 
VirtIO client code behaves like all other NetBSD PCI drivers and does NOT skip 
the platform DMA mapping step.

> 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.

-- thorpej