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

[Jason Wang <jasowang@redhat.com>]

On Wed, Oct 19, 2022 at 10:54 PM Peter Xu <peterx@redhat.com> wrote:
>
> 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?

Haven't tried but I guess there would be some issue other than the
suggested configuration "ats=on, device-iotlb=on"

So we have:

1) ats=on, device-iotlb=on, this is the configuration that libvirt is
using and it should work
2) ats=off, device-iotlb=on, in this case, the DEVICEIOTLB_UNMAP
notifier will succeed but there won't be a device iotlb invalidation
sent from guest, so we will meet errors since there's no way to flush
device IOTLB. According to the PCIe spec, the device should still work
(using untranslated transactions). In this case we probably need a way
to detect if device page fault (ats) is enabled and fallback to UNMAP
if it doesn't.
3) ats=on, device-iotlb=off, in this case, without your patch, it
won't work since the DEVICEIOTLB_UNMAP will succeed but guest won't
enable ATS so there will be no IOTLB invalidation. With your patch, we
fallback to UNMAP and I think it should then work
4) ats=off, device-iotlb=off, similar to 3), it won't work without
your patch, but with your patch we fallback to UNMAP so it should
work.

Thanks

>
> Thanks,
>
> --
> Peter Xu
>