Luiz - not clear to me where exactly this falls between perhaps Eric, David, or Peter - so I'll let you decide

Hi John, I can take that one. If needed I will ask for help ...

(In reply to yalzhang from comment #0)
> Description of problem:
> Qemu should return error info since virtio iommu do not support device-iotlb
> 
> Version-Release number of selected component (if applicable):
> Host:
> libvirt-8.4.0-3.el9.x86_64
> qemu-kvm-7.0.50-6.el9.wrb220615.x86_64
> Guest:
> kernel-5.14.0-104.mr955_220602_1540.el9.x86_64
> 
> How reproducible:
> 100%
> 
> Steps to Reproduce:
> 1. Start vm with virtio iommu and virtio device with iommu enabled as below:
> # virsh start rhel
> Domain 'rhel' started
> # virsh dumpxml rhel
> ......
>  <interface type='network'>
>       <mac address='52:54:00:7b:3d:d9'/>
>       <source network='default'/>
>       <model type='virtio'/>
>       <driver iommu='on' ats='on'/>
>       <address type='pci' domain='0x0000' bus='0x03' slot='0x00'
> function='0x0'/>
>     </interface>
> <iommu model='virtio'>
>       <address type='pci' domain='0x0000' bus='0x00' slot='0x04'
> function='0x0'/>
>     </iommu>
> 
> Qemu cmd:
> -device '{"driver":"virtio-iommu","bus":"pcie.0","addr":"0x4"}'
> ......
> -device
> '{"driver":"virtio-net-pci","netdev":"hostnet0","id":"net0","mac":"52:54:00:
> a6:e0:f3","bus":"pci.7","addr":"0x0"}' \
> -netdev tap,fd=26,vhost=on,vhostfd=27,id=hostnet1 \
> -device
> '{"driver":"virtio-net-pci","iommu_platform":true,"ats":true,"netdev":
> "hostnet1","id":"net1","mac":"52:54:00:7b:3d:d9","bus":"pci.3","addr":
> "0x0"}' \
> 
> 2. device-iotlb is not supported by the virtio-iommu. Logically the qemu
> virtio-iommu device should return an error when ats enabled virtio device
> tries to attach an IOMMU_NOTIFIER_DEVIOTLB_UNMAP notifier (virtio-iommu.c /
> virtio_iommu_notify_flag_changed : error_setg(errp, "Virtio-iommu does not
> support dev-iotlb yet");  refer to bug 2089765 comment 33
> 
> Actual results:
> "ats=on" on virtio device pairs with "device-iotlb=on" on iommu device.
> device-iotlb is not supported by the virtio-iommu, but the guest with ats=on
> starts successfully without any error. 
> 
> Expected results:
> There should be some error prompt or waring in the qemu log when device ats
> enabled without "device-iotlb=on"
> 
> Additional info:
> Same for intel iommu when we do not set the "device-iotlb=on"

Looks the consistency check is done at libvirt level since at qemu level I am able to start a guest with ats=on virtio-net-pci and without dev-iotlb along with the intel-iommu device. Andrea, please can you confirm?

(In reply to Eric Auger from comment #3)
> (In reply to yalzhang from comment #0)
> > Description of problem:
> > Qemu should return error info since virtio iommu do not support device-iotlb
> > 
> > Version-Release number of selected component (if applicable):
> > Host:
> > libvirt-8.4.0-3.el9.x86_64
> > qemu-kvm-7.0.50-6.el9.wrb220615.x86_64
> > Guest:
> > kernel-5.14.0-104.mr955_220602_1540.el9.x86_64
> > 
> > How reproducible:
> > 100%
> > 
> > Steps to Reproduce:
> > 1. Start vm with virtio iommu and virtio device with iommu enabled as below:
> > # virsh start rhel
> > Domain 'rhel' started
> > # virsh dumpxml rhel
> > ......
> >  <interface type='network'>
> >       <mac address='52:54:00:7b:3d:d9'/>
> >       <source network='default'/>
> >       <model type='virtio'/>
> >       <driver iommu='on' ats='on'/>
> >       <address type='pci' domain='0x0000' bus='0x03' slot='0x00'
> > function='0x0'/>
> >     </interface>
> > <iommu model='virtio'>
> >       <address type='pci' domain='0x0000' bus='0x00' slot='0x04'
> > function='0x0'/>
> >     </iommu>
> > 
> > Qemu cmd:
> > -device '{"driver":"virtio-iommu","bus":"pcie.0","addr":"0x4"}'
> > ......
> > -device
> > '{"driver":"virtio-net-pci","netdev":"hostnet0","id":"net0","mac":"52:54:00:
> > a6:e0:f3","bus":"pci.7","addr":"0x0"}' \
> > -netdev tap,fd=26,vhost=on,vhostfd=27,id=hostnet1 \
> > -device
> > '{"driver":"virtio-net-pci","iommu_platform":true,"ats":true,"netdev":
> > "hostnet1","id":"net1","mac":"52:54:00:7b:3d:d9","bus":"pci.3","addr":
> > "0x0"}' \
> > 
> > 2. device-iotlb is not supported by the virtio-iommu. Logically the qemu
> > virtio-iommu device should return an error when ats enabled virtio device
> > tries to attach an IOMMU_NOTIFIER_DEVIOTLB_UNMAP notifier (virtio-iommu.c /
> > virtio_iommu_notify_flag_changed : error_setg(errp, "Virtio-iommu does not
> > support dev-iotlb yet");  refer to bug 2089765 comment 33

My input here was incomplete.

indeed the virtio-iommu device properly returns an error on DEVIOTLB_UNMAP notifier registration. vhost is currently the only user attempting to register this notifier type. But in vhost.c, vhost_iommu_region_add() first attempts to register the DEV-IOTLB notifier but if this fails (in virtio-iommu case for instance) we silently fall back to the legacy UNMAP notifier. This scheme was introduced in [3] 958ec334bca3 ("vhost: Unbreak SMMU and virtio-iommu on dev-iotlb support") to avoid regressing the vhost use case with smmuv3 and virtio-iommu.

both virtio-pci device ats option and intel iommu device-iotlb options were introduced in [1] for vhost/viommu integration. The rationale most likely is vhost behaves like an ATS enabled end point, ie. it implements a device IOTLB cache on kernel side. However translated transactions directly go to the kernel and are not visible to the emulated iommu. So to me this is a vhost specificity which diverges from real emulated PCI device which would send translated transactions through the viommu. 

The "ats" as virtio device parameter adds ATS capability to the PCI device and initialize ATS_CAP/CTRL. The "device-iotlb" as intel-iommu parameter enables ATS in the intel-iommu device (and also reports that in ACPI table, ATSR), allow the decoding of Device-TLB Invalidate descriptors (dev_tlb_inv_dsc).

At that point it was said both settings should be paired: I guess if the guest intel-iommu driver detects the EP has ats enabled, dev-iotlb flush commands are sent and the associated descriptor must be properly decoded in the vIOMMU. This support was introduced in [1]. Upon those dev-iotlb flush, the intel-iommu device was originally sending UNMAP notifiers. And later on, those were replaced by DEVIOTLB_UNMAP [2]. I think vhost should also work if both ats=on and device-iotlb are unset. In that case, the driver will send unmap notifications which also do the job. However my guess is unpaired settings are likely to disfunction but I would appreciate confirmations.

With virtio-iommu we do support vhost integration without relying on virtio-pci device ats=on (at least there is no known issue). the virtio-iommu driver cannot send any "device-iotlb" invalidations anyway. This does not mean we would support a fully compliant ats enabled emulated end point: for instance, if the translated transactions were not going to the kernel as it is the case for vhost, the vIOMMU would see translated transactions it should be able to handle properly. This is definitively not the case for both the virtio-iommu and the vSMMUv3. 

(1) [PATCH V4 06/10] virtio-pci: address space translation service
(ATS) support
(https://lore.kernel.org/qemu-devel/1483092559-24488-1-git-send-email-jasowang@redhat.com/)

(2) [PATCH v3 0/5] memory: Skip assertion in memory_region_unregister_iommu_notifier
https://lore.kernel.org/all/20201116165506.31315-1-eperezma@redhat.com/

(3) vhost: Unbreak SMMU and virtio-iommu on dev-iotlb support
https://lore.kernel.org/qemu-devel/20210204191228.187550-1-peterx@redhat.com/

Andrea,

Please could you confirm you never set ats=on on any virtio-pci device protected by a virtio-iommu or smmuv3?

I just tried patching the xml for virtio-iommu and I don't see ats=on, - which is the expectation - but I prefer to double check ....

Thanks

Eric

I sent "[PATCH] vhost: Warn if DEVIOTLB_UNMAP is not supported and ats is set" upstream to start discussion

To progress on this BZ I would be tempted to close it as NOT A BUG.

When ATS is set on the virtio-net-pci device it means the device supports ATS but does not necessarily mean ATS is enabled. And in practice if the iommu does not support ATS (which is the case for the virtio-iommu), ATS won't be enabled between the end point and the viommu.

So to me there is no real issue behind being able to set ats=on along with virtio-iommu, as long as vhost works. If the ats capable virtio-net device wouldn't work propertly with the virtio-iommu *this* would be a real bug but if I understand correctly this is not the case. 

However there are discussions upstream wrt ATS=on & intel-iommu because as far as I understand ATS=on & intel-iommu without device iotlb does not work and to me, this is a problem:
https://lore.kernel.org/all/CACGkMEtL9Rf6YkHBJpNm2LXfvX51s2KDCgvBNH6cJWfp7PdfVA@mail.gmail.com/

(In reply to Eric Auger from comment #7)
> To progress on this BZ I would be tempted to close it as NOT A BUG.
> 
> When ATS is set on the virtio-net-pci device it means the device supports
> ATS but does not necessarily mean ATS is enabled. And in practice if the
> iommu does not support ATS (which is the case for the virtio-iommu), ATS
> won't be enabled between the end point and the viommu.
> 
> So to me there is no real issue behind being able to set ats=on along with
> virtio-iommu, as long as vhost works. If the ats capable virtio-net device
> wouldn't work propertly with the virtio-iommu *this* would be a real bug but
> if I understand correctly this is not the case. 
> 
> However there are discussions upstream wrt ATS=on & intel-iommu because as
> far as I understand ATS=on & intel-iommu without device iotlb does not work
> and to me, this is a problem:
> https://lore.kernel.org/all/
> CACGkMEtL9Rf6YkHBJpNm2LXfvX51s2KDCgvBNH6cJWfp7PdfVA.com/

Yes, I plan to fix this.

Thanks

Closing this BZ as NOTABUG per Comment #7

Apologies for the embarrassingly late reply :(


(In reply to Eric Auger from comment #3)
> (In reply to yalzhang from comment #0)
> > Expected results:
> > There should be some error prompt or waring in the qemu log when device ats
> > enabled without "device-iotlb=on"
> >
> > Additional info:
> > Same for intel iommu when we do not set the "device-iotlb=on"
>
> Looks the consistency check is done at libvirt level since at qemu level I
> am able to start a guest with ats=on virtio-net-pci and without dev-iotlb
> along with the intel-iommu device. Andrea, please can you confirm?

I cannot find any check for this in the context of intel-iommu
either.

I believe the reporter might have wanted to suggest that such a check
should be introduced for intel-iommu along with virtio-iommu, and not
that virtio-iommu should adopt the check that already existed for
intel-iommu.

But I might have misinterpreted things :)


(In reply to Eric Auger from comment #5)
> Andrea,
> 
> Please could you confirm you never set ats=on on any virtio-pci device
> protected by a virtio-iommu or smmuv3?
> 
> I just tried patching the xml for virtio-iommu and I don't see ats=on, -
> which is the expectation - but I prefer to double check ....

AFAICT there is no check against the IOMMU model when deciding
whether to add ats=on to a device's command line.

In other words,

  <interface type='...'>
    <model type='virtio'/>
    <driver ats='on'/>
  </interface>

will *always* result in ats=on[1] being generated.

Does this answer your question?


[1] Well, "ats":true to be more accurate - libvirt generates JSON
    command lines these days.