Re: [PATCH] x86-iommu: Fail flag registration of DEVIOTLB if DT not supported

[Peter Xu <peterx@redhat.com>]

On Wed, Oct 19, 2022 at 04:12:22PM +0200, Eric Auger wrote:
> Hi Peter,
> 
> On 10/19/22 16:01, Peter Xu wrote:
> > Hi, Eric,
> >
> > On Wed, Oct 19, 2022 at 01:24:15PM +0200, Eric Auger wrote:
> >>> @@ -1484,6 +1485,13 @@ static int amdvi_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu,
> >>>                     PCI_FUNC(as->devfn));
> >>>          return -EINVAL;
> >>>      }
> >>> +
> >>> +    if ((new & IOMMU_NOTIFIER_DEVIOTLB_UNMAP) && !x86_iommu->dt_supported) {
> >>> +        error_setg_errno(errp, ENOTSUP,
> >>> +                         "Device-iotlb not declared support for vIOMMU");
> >> with current vhost code, vhost will then silently fallbac to UNMAP
> >> notifier registration and this will succeed. It would be nice to clarify
> >> whether the vIOMMU works with vhost in this downgraded mode (at least
> >> ats=off and device-ioltb=off)?
> > I'm slightly confused, why do we need to clarify that?
> >
> > As we have discussed, if a device with ATS capability got attached into a
> > vIOMMU context that does not support ATS, then it should just work like
> > without ATS without any warning.  Isn't this the case here?
> 
> Yes that's the theory and what should happen at baremetal level. However
> I am not sure this is still true with the intel-iommu emulation/vhost
> integration.
> Remember we always assumed vhost was supported on intel with both ats=on
> and device-iotlb=on if I am correct.
> 
> vhost/viommu integration requires unmap notifications to be properly
> sent from viommu to vhost, would it be though DEVIOTLB_UNMAP or UNMAP
> notifiers.
> Does the intel-iommu/vhost works if both ats=off and device-iotlb=off or
> ats=on and device-iotlb=off. This I am not sure. I gave it a try and I
> got some errors but maybe that's something else...

Indeed it's not working.  Obviously my test on this patch is not complete,
as I was testing with pings and even after patch applied the ping can get
lost after a few successful ones.  I should have tried harder.

Yes only device-iotlb=on & ats=on work in my case, all the rest three
combinations won't work reliably or just don't work, but I do expect they
should all work, so I definitely missed something here.  Jason, are you
aware of the problem?

Thanks,

-- 
Peter Xu