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

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

[Peter Xu]

All x86 IOMMUs need to fail DEVIOTLB notification registrations if DT is
not supported.  Otherwise any device (e.g. vhost) can register with
DEVIOTLB but it'll never receive the notifications, causing device not
operational.

Fixes: b68ba1ca57 ("memory: Add IOMMU_NOTIFIER_DEVIOTLB_UNMAP IOMMUTLBNotificationType")
Reported-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/i386/amd_iommu.c   | 8 ++++++++
 hw/i386/intel_iommu.c | 7 +++++++
 2 files changed, 15 insertions(+)

diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 725f69095b..42486e790f 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -1476,6 +1476,7 @@ static int amdvi_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu,
                                            Error **errp)
 {
     AMDVIAddressSpace *as = container_of(iommu, AMDVIAddressSpace, iommu);
+    X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(as->iommu_state);
 
     if (new & IOMMU_NOTIFIER_MAP) {
         error_setg(errp,
@@ -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");
+        return -ENOTSUP;
+    }
+
     return 0;
 }
 
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 6524c2ee32..a07d9631b0 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -3057,6 +3057,7 @@ static int vtd_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu,
 {
     VTDAddressSpace *vtd_as = container_of(iommu, VTDAddressSpace, iommu);
     IntelIOMMUState *s = vtd_as->iommu_state;
+    X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s);
 
     /* TODO: add support for VFIO and vhost users */
     if (s->snoop_control) {
@@ -3065,6 +3066,12 @@ static int vtd_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu,
         return -ENOTSUP;
     }
 
+    if ((new & IOMMU_NOTIFIER_DEVIOTLB_UNMAP) && !x86_iommu->dt_supported) {
+        error_setg_errno(errp, ENOTSUP,
+                         "Device-iotlb not declared support for vIOMMU");
+        return -ENOTSUP;
+    }
+
     /* Update per-address-space notifier flags */
     vtd_as->notifier_flags = new;
 
-- 
2.37.3

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

[Jason Wang]

在 2022/10/19 05:54, Peter Xu 写道:
> All x86 IOMMUs need to fail DEVIOTLB notification registrations if DT is
> not supported.  Otherwise any device (e.g. vhost) can register with
> DEVIOTLB but it'll never receive the notifications, causing device not
> operational.
>
> Fixes: b68ba1ca57 ("memory: Add IOMMU_NOTIFIER_DEVIOTLB_UNMAP IOMMUTLBNotificationType")
> Reported-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>


Acked-by: Jason Wang <jasowang@redhat.com>


> ---
>   hw/i386/amd_iommu.c   | 8 ++++++++
>   hw/i386/intel_iommu.c | 7 +++++++
>   2 files changed, 15 insertions(+)
>
> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> index 725f69095b..42486e790f 100644
> --- a/hw/i386/amd_iommu.c
> +++ b/hw/i386/amd_iommu.c
> @@ -1476,6 +1476,7 @@ static int amdvi_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu,
>                                              Error **errp)
>   {
>       AMDVIAddressSpace *as = container_of(iommu, AMDVIAddressSpace, iommu);
> +    X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(as->iommu_state);
>   
>       if (new & IOMMU_NOTIFIER_MAP) {
>           error_setg(errp,
> @@ -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");
> +        return -ENOTSUP;
> +    }
> +
>       return 0;
>   }
>   
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 6524c2ee32..a07d9631b0 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -3057,6 +3057,7 @@ static int vtd_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu,
>   {
>       VTDAddressSpace *vtd_as = container_of(iommu, VTDAddressSpace, iommu);
>       IntelIOMMUState *s = vtd_as->iommu_state;
> +    X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s);
>   
>       /* TODO: add support for VFIO and vhost users */
>       if (s->snoop_control) {
> @@ -3065,6 +3066,12 @@ static int vtd_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu,
>           return -ENOTSUP;
>       }
>   
> +    if ((new & IOMMU_NOTIFIER_DEVIOTLB_UNMAP) && !x86_iommu->dt_supported) {
> +        error_setg_errno(errp, ENOTSUP,
> +                         "Device-iotlb not declared support for vIOMMU");
> +        return -ENOTSUP;
> +    }
> +
>       /* Update per-address-space notifier flags */
>       vtd_as->notifier_flags = new;
>   

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

[Eric Auger]

Hi Peter,

On 10/18/22 23:54, Peter Xu wrote:
> All x86 IOMMUs need to fail DEVIOTLB notification registrations if DT is
> not supported.  Otherwise any device (e.g. vhost) can register with
> DEVIOTLB but it'll never receive the notifications, causing device not
> operational.
>
> Fixes: b68ba1ca57 ("memory: Add IOMMU_NOTIFIER_DEVIOTLB_UNMAP IOMMUTLBNotificationType")
> Reported-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  hw/i386/amd_iommu.c   | 8 ++++++++
>  hw/i386/intel_iommu.c | 7 +++++++
>  2 files changed, 15 insertions(+)
>
> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> index 725f69095b..42486e790f 100644
> --- a/hw/i386/amd_iommu.c
> +++ b/hw/i386/amd_iommu.c
> @@ -1476,6 +1476,7 @@ static int amdvi_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu,
>                                             Error **errp)
>  {
>      AMDVIAddressSpace *as = container_of(iommu, AMDVIAddressSpace, iommu);
> +    X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(as->iommu_state);
>  
>      if (new & IOMMU_NOTIFIER_MAP) {
>          error_setg(errp,
> @@ -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)?

Thanks

Eric
> +        return -ENOTSUP;
> +    }
> +
>      return 0;
>  }
>  
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 6524c2ee32..a07d9631b0 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -3057,6 +3057,7 @@ static int vtd_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu,
>  {
>      VTDAddressSpace *vtd_as = container_of(iommu, VTDAddressSpace, iommu);
>      IntelIOMMUState *s = vtd_as->iommu_state;
> +    X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s);
>  
>      /* TODO: add support for VFIO and vhost users */
>      if (s->snoop_control) {
> @@ -3065,6 +3066,12 @@ static int vtd_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu,
>          return -ENOTSUP;
>      }
>  
> +    if ((new & IOMMU_NOTIFIER_DEVIOTLB_UNMAP) && !x86_iommu->dt_supported) {
> +        error_setg_errno(errp, ENOTSUP,
> +                         "Device-iotlb not declared support for vIOMMU");
> +        return -ENOTSUP;
> +    }
> +
>      /* Update per-address-space notifier flags */
>      vtd_as->notifier_flags = new;
>  

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

[Peter Xu]

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?

Thanks,

-- 
Peter Xu

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

[Eric Auger]

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

On ARM I have always assumed both settings are off and so I am inclined
to think it works ;-)

Thanks

Eric

>
> Thanks,
>

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

[Peter Xu]

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

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

[Jason Wang]

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
>

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

[Peter Xu]

On Thu, Oct 20, 2022 at 11:58:34AM +0800, Jason Wang wrote:
> 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.

Yeah, agreed that we should not register to dev-iotlb notifier if ats is
off in the first place.  Maybe worth another patch.

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

I think the current patch should still be correct, but maybe something else
is missing.  I'll add this one into the todo list but I'd be more than glad
if anyone can look into this too before that..  Or as Eric suggested to
detect such a mismatch before it's fixed, but then if we know they're not
working we should fail hard rather than a warning anymore.

Thanks,

-- 
Peter Xu