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

[Jason Wang]

在 2022/10/19 05:56, 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?


Yes, it's exposed but not enabled.


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


If you mean f7701e2c7983b680790af47117577b285b6a1aed ("intel_iommu: Skip 
page walking on device iotlb invalidations"), it should help in the case 
of domian or global invalidation. But it really depends on whether or 
not guest is using those.

Regrading to the perf number, it might be tricky since:

1) the invalidation was batched or delayed which should be rare or at 
least very less frequent

2) meaning with TCP might be impact by a lot of other factors (you can 
see UDP_STREAM doesn't give ovbious improvement).

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