Re: [PATCH] vhost: Warn if DEVIOTLB_UNMAP is not supported and ats is set

[Peter Xu]

On Tue, Oct 18, 2022 at 05:08:19PM +0200, Eric Auger wrote:
> Hi Peter,
> 
> On 10/18/22 16:25, Peter Xu wrote:
> > Hi, Eric,
> >
> > On Tue, Oct 18, 2022 at 02:28:52PM +0200, Eric Auger wrote:
> >> Since b68ba1ca5767 ("memory: Add IOMMU_NOTIFIER_DEVIOTLB_UNMAP
> >> IOMMUTLBNotificationType"), vhost attempts to register DEVIOTLB_UNMAP
> >> notifier. This latter is supported by the intel-iommu which supports
> >> device-iotlb if the corresponding option is set. Then 958ec334bca3
> >> ("vhost: Unbreak SMMU and virtio-iommu on dev-iotlb support") allowed
> >> silent fallback to the legacy UNMAP notifier if the viommu does not
> >> support device iotlb.
> >>
> >> Initially vhost/viommu integration was introduced with intel iommu
> >> assuming ats=on was set on virtio-pci device and device-iotlb was set
> >> on the intel iommu. vhost acts as an ATS capable device since it
> >> implements an IOTLB on kernel side. However translated transactions
> >> that hit the device IOTLB do not transit through the vIOMMU. So this
> >> requires a limited ATS support on viommu side.
> >>
> >> However, in theory, if ats=on is set on a pci device, the
> >> viommu should support ATS for that device to work.
> > Pure question: what will happen if one ATS supported PCI device got plugged
> > into a system whose physical IOMMU does not support ATS?  Will ATS just be
> > ignored and the device keep working simply without ATS?
> Yes that's my understanding: in that case the ATS capable device would
> work with ats disabled (baremetal case). In the iommu driver you can
> have a look at the pci_enable_ats() call which is guarded by
> info->ats_supported for instance on intel iommu.
> 
> Following that reasoning vhost modality should not be enabled without
> ATS support on vIOMMU side. But it is.
> 
> In that sense I may rename the ats_enabled helpers with ats_capable?

Sounds good to me.

> If I understand correctly setting ats=on exposes the ATS capability (
> 615c4ed205  virtio-pci: address space translation service (ATS) support)
> which is then enabled by the guest driver.

I think it won't, as long as vIOMMU doesn't have DT support declared?

> 
> > [1]
> >
> > [...]
> >
> >> @@ -760,8 +771,16 @@ static void vhost_iommu_region_add(MemoryListener 
> >> *listener,
> >>      iommu->iommu_offset = section->offset_within_address_space -
> >>                            section->offset_within_region;
> >>      iommu->hdev = dev;
> >> -    ret = memory_region_register_iommu_notifier(section->mr, &iommu->n, 
> >> NULL);
> >> +    ret = memory_region_register_iommu_notifier(section->mr, &iommu->n, 
> >> &err);
> >>      if (ret) {
> >> +        if (vhost_dev_ats_enabled(dev)) {
> >> +            error_reportf_err(err,
> >> +                              "vhost cannot register DEVIOTLB_UNMAP "
> >> +                              "although ATS is enabled, "
> >> +                              "fall back to legacy UNMAP notifier: ");
> > We want to use the warning message to either remind the user to (1) add the
> > dev-iotlb=on parameter for vIOMMU, or (2) drop the ats=on on device.  Am I
> > right?
> My focus is to warn the end user there is no support for device-iotlb
> support in virtio-iommu or vsmmuv3 but vhost does not really require
> it.Indeed current users of virtio-iommu/vsmmuv3 seem confused now wrt
> vhost integration and the lack of device-iotlb option on those viommus.
> 
> On intel I understand we would like to enforce that ats and dev-iotlb
> are both set or unset. But this is not really addressed in that series.
> Indeed vtd_iommu_notify_flag_changed does not reject any registration of
> IOMMU_NOTIFIER_DEVIOTLB_UNMAP notifier in case it does not support
> device-iotlb. I think it should.

Yes I agree, thanks for finding it.  Just posted a patch:

20221018215407.363986-1-peterx@redhat.com">https://lore.kernel.org/r/20221018215407.363986-1-peterx@redhat.com

> The trouble is vhost_iommu_region_add
> is not meant to nicely fail.
> >
> > As we've discussed - I remember Jason used to test with/without dev-iotlb
> > on vhost on Intel and dev-iotlb is faster on vt-d guest driver than without
> It would be nice to have a clarification about this. Indeed
> 
> [PATCH v3 0/5] memory: Skip assertion in 
> memory_region_unregister_iommu_notifier
> https://lore.kernel.org/all/20201116165506.31315-1-eperezma@redhat.com/
> mostly focussed on removing an assertion although one patch mentionned perf 
> improvements. What does make the perf better (less device iotlb flushes than 
> general iotlb flushes?)

I'll leave that to Jason.  Thanks.

> 
> > it.  So that can make sense to me for (1).  I don't know whether it helps
> > for (2) because fundamentally it's the same question as [1] above, and
> > whether that's a legal configuration.
> >
> > Thanks,
> >
> Adding jean in the loop too
> 
> Thanks
> 
> Eric
> 

-- 
Peter Xu