diff mbox series

[2/3] intel-iommu: fail DEVIOTLB_UNMAP without dt mode

Message ID 20221129081037.12099-3-jasowang@redhat.com (mailing list archive)
State New, archived
Headers show
Series Fix UNMAP notifier for intel-iommu | expand

Commit Message

Jason Wang Nov. 29, 2022, 8:10 a.m. UTC
Without dt mode, device IOTLB notifier won't work since guest won't
send device IOTLB invalidation descriptor in this case. Let's fail
early instead of misbehaving silently.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/i386/intel_iommu.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Peter Xu Nov. 29, 2022, 3:38 p.m. UTC | #1
On Tue, Nov 29, 2022 at 04:10:36PM +0800, Jason Wang wrote:
> Without dt mode, device IOTLB notifier won't work since guest won't
> send device IOTLB invalidation descriptor in this case. Let's fail
> early instead of misbehaving silently.
> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  hw/i386/intel_iommu.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 9143376677..d025ef2873 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -3179,6 +3179,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) {
> @@ -3193,6 +3194,13 @@ static int vtd_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu,
>                           PCI_FUNC(vtd_as->devfn));
>          return -ENOTSUP;
>      }
> +    if (!x86_iommu->dt_supported && (new & IOMMU_NOTIFIER_DEVIOTLB_UNMAP)) {
> +        error_setg_errno(errp, ENOTSUP,
> +                         "device %02x.%02x.%x requires device IOTLB mode",
> +                         pci_bus_num(vtd_as->bus), PCI_SLOT(vtd_as->devfn),
> +                         PCI_FUNC(vtd_as->devfn));
> +        return -ENOTSUP;
> +    }

Hmm I thought we have had this already, so we don't?... :-(

Reviewed-by: Peter Xu <peterx@redhat.com>
Peter Xu Dec. 1, 2022, 4:03 p.m. UTC | #2
On Tue, Nov 29, 2022 at 04:10:36PM +0800, Jason Wang wrote:
> Without dt mode, device IOTLB notifier won't work since guest won't
> send device IOTLB invalidation descriptor in this case. Let's fail
> early instead of misbehaving silently.
> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  hw/i386/intel_iommu.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 9143376677..d025ef2873 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -3179,6 +3179,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) {
> @@ -3193,6 +3194,13 @@ static int vtd_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu,
>                           PCI_FUNC(vtd_as->devfn));
>          return -ENOTSUP;
>      }
> +    if (!x86_iommu->dt_supported && (new & IOMMU_NOTIFIER_DEVIOTLB_UNMAP)) {
> +        error_setg_errno(errp, ENOTSUP,
> +                         "device %02x.%02x.%x requires device IOTLB mode",
> +                         pci_bus_num(vtd_as->bus), PCI_SLOT(vtd_as->devfn),
> +                         PCI_FUNC(vtd_as->devfn));
> +        return -ENOTSUP;
> +    }

While my r-b holds.. let's also do this for amd-iommu in the same patch?
dt never supported there, so we can fail as long as DEVIOTLB registered.

>  
>      /* Update per-address-space notifier flags */
>      vtd_as->notifier_flags = new;
> -- 
> 2.25.1
>
Eric Auger Dec. 6, 2022, 1:33 p.m. UTC | #3
Hi jason,

On 11/29/22 09:10, Jason Wang wrote:
> Without dt mode, device IOTLB notifier won't work since guest won't
> send device IOTLB invalidation descriptor in this case. Let's fail
> early instead of misbehaving silently.
>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  hw/i386/intel_iommu.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 9143376677..d025ef2873 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -3179,6 +3179,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) {
> @@ -3193,6 +3194,13 @@ static int vtd_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu,
>                           PCI_FUNC(vtd_as->devfn));
>          return -ENOTSUP;
>      }
> +    if (!x86_iommu->dt_supported && (new & IOMMU_NOTIFIER_DEVIOTLB_UNMAP)) {
> +        error_setg_errno(errp, ENOTSUP,
> +                         "device %02x.%02x.%x requires device IOTLB mode",
maybe precise INTEL IOMMU device-IOTLB mode. otherwise this may be
confused with device ATS capability?

While thinking about those error handlings (including the SMMU ones)
nothing should really prevent you from registering a notifier that is
not signalled. Maybe we should add in the documentation that any attempt
to register an IOMMU notifier to an IOMMU MR that is not able to signal
it will return an error.

Besides
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric
> +                         pci_bus_num(vtd_as->bus), PCI_SLOT(vtd_as->devfn),
> +                         PCI_FUNC(vtd_as->devfn));
> +        return -ENOTSUP;
> +    }
>  
>      /* Update per-address-space notifier flags */
>      vtd_as->notifier_flags = new;
Laurent Vivier Feb. 3, 2023, 9:08 a.m. UTC | #4
On 11/29/22 09:10, Jason Wang wrote:
> Without dt mode, device IOTLB notifier won't work since guest won't
> send device IOTLB invalidation descriptor in this case. Let's fail
> early instead of misbehaving silently.
> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>   hw/i386/intel_iommu.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 9143376677..d025ef2873 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -3179,6 +3179,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) {
> @@ -3193,6 +3194,13 @@ static int vtd_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu,
>                            PCI_FUNC(vtd_as->devfn));
>           return -ENOTSUP;
>       }
> +    if (!x86_iommu->dt_supported && (new & IOMMU_NOTIFIER_DEVIOTLB_UNMAP)) {
> +        error_setg_errno(errp, ENOTSUP,
> +                         "device %02x.%02x.%x requires device IOTLB mode",
> +                         pci_bus_num(vtd_as->bus), PCI_SLOT(vtd_as->devfn),
> +                         PCI_FUNC(vtd_as->devfn));
> +        return -ENOTSUP;
> +    }
>   
>       /* Update per-address-space notifier flags */
>       vtd_as->notifier_flags = new;

Reviewed-by: Laurent Vivier <lvivier@redhat.com>
Tested-by: Laurent Vivier <lvivier@redhat.com>
Buglink: https://bugzilla.redhat.com/2156876
Laurent Vivier Feb. 7, 2023, 4:17 p.m. UTC | #5
On 2/3/23 10:08, Laurent Vivier wrote:
> On 11/29/22 09:10, Jason Wang wrote:
>> Without dt mode, device IOTLB notifier won't work since guest won't
>> send device IOTLB invalidation descriptor in this case. Let's fail
>> early instead of misbehaving silently.
>>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>>   hw/i386/intel_iommu.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>> index 9143376677..d025ef2873 100644
>> --- a/hw/i386/intel_iommu.c
>> +++ b/hw/i386/intel_iommu.c
>> @@ -3179,6 +3179,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) {
>> @@ -3193,6 +3194,13 @@ static int vtd_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu,
>>                            PCI_FUNC(vtd_as->devfn));
>>           return -ENOTSUP;
>>       }
>> +    if (!x86_iommu->dt_supported && (new & IOMMU_NOTIFIER_DEVIOTLB_UNMAP)) {
>> +        error_setg_errno(errp, ENOTSUP,
>> +                         "device %02x.%02x.%x requires device IOTLB mode",
>> +                         pci_bus_num(vtd_as->bus), PCI_SLOT(vtd_as->devfn),
>> +                         PCI_FUNC(vtd_as->devfn));
>> +        return -ENOTSUP;
>> +    }
>>       /* Update per-address-space notifier flags */
>>       vtd_as->notifier_flags = new;
> 
> Reviewed-by: Laurent Vivier <lvivier@redhat.com>
> Tested-by: Laurent Vivier <lvivier@redhat.com>
> Buglink: https://bugzilla.redhat.com/2156876

Is this possible to have this patch merged?
It fixes a real problem and it is really trivial.

Thanks,
Laurent
Peter Xu Feb. 7, 2023, 4:35 p.m. UTC | #6
On Tue, Feb 07, 2023 at 05:17:42PM +0100, Laurent Vivier wrote:
> On 2/3/23 10:08, Laurent Vivier wrote:
> > On 11/29/22 09:10, Jason Wang wrote:
> > > Without dt mode, device IOTLB notifier won't work since guest won't
> > > send device IOTLB invalidation descriptor in this case. Let's fail
> > > early instead of misbehaving silently.
> > > 
> > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > ---
> > >   hw/i386/intel_iommu.c | 8 ++++++++
> > >   1 file changed, 8 insertions(+)
> > > 
> > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > > index 9143376677..d025ef2873 100644
> > > --- a/hw/i386/intel_iommu.c
> > > +++ b/hw/i386/intel_iommu.c
> > > @@ -3179,6 +3179,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) {
> > > @@ -3193,6 +3194,13 @@ static int vtd_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu,
> > >                            PCI_FUNC(vtd_as->devfn));
> > >           return -ENOTSUP;
> > >       }
> > > +    if (!x86_iommu->dt_supported && (new & IOMMU_NOTIFIER_DEVIOTLB_UNMAP)) {
> > > +        error_setg_errno(errp, ENOTSUP,
> > > +                         "device %02x.%02x.%x requires device IOTLB mode",
> > > +                         pci_bus_num(vtd_as->bus), PCI_SLOT(vtd_as->devfn),
> > > +                         PCI_FUNC(vtd_as->devfn));
> > > +        return -ENOTSUP;
> > > +    }
> > >       /* Update per-address-space notifier flags */
> > >       vtd_as->notifier_flags = new;
> > 
> > Reviewed-by: Laurent Vivier <lvivier@redhat.com>
> > Tested-by: Laurent Vivier <lvivier@redhat.com>
> > Buglink: https://bugzilla.redhat.com/2156876
> 
> Is this possible to have this patch merged?
> It fixes a real problem and it is really trivial.

AFAIU Jason will post a new version soon for this whole set.  But I also
agree if Michael has an earlier pull we can add this in even earlier.

Thanks,
Jason Wang Feb. 23, 2023, 3:19 a.m. UTC | #7
On Fri, Dec 2, 2022 at 12:03 AM Peter Xu <peterx@redhat.com> wrote:
>
> On Tue, Nov 29, 2022 at 04:10:36PM +0800, Jason Wang wrote:
> > Without dt mode, device IOTLB notifier won't work since guest won't
> > send device IOTLB invalidation descriptor in this case. Let's fail
> > early instead of misbehaving silently.
> >
> > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > ---
> >  hw/i386/intel_iommu.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > index 9143376677..d025ef2873 100644
> > --- a/hw/i386/intel_iommu.c
> > +++ b/hw/i386/intel_iommu.c
> > @@ -3179,6 +3179,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) {
> > @@ -3193,6 +3194,13 @@ static int vtd_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu,
> >                           PCI_FUNC(vtd_as->devfn));
> >          return -ENOTSUP;
> >      }
> > +    if (!x86_iommu->dt_supported && (new & IOMMU_NOTIFIER_DEVIOTLB_UNMAP)) {
> > +        error_setg_errno(errp, ENOTSUP,
> > +                         "device %02x.%02x.%x requires device IOTLB mode",
> > +                         pci_bus_num(vtd_as->bus), PCI_SLOT(vtd_as->devfn),
> > +                         PCI_FUNC(vtd_as->devfn));
> > +        return -ENOTSUP;
> > +    }
>
> While my r-b holds.. let's also do this for amd-iommu in the same patch?
> dt never supported there, so we can fail as long as DEVIOTLB registered.

Looks like there's one implementation:

Per spec:

""
The INVALIDATE_IOTLB_PAGES command is only present in IOMMU
implementations that support remote IOTLB caching of translations (see
Capability Offset 00h[IotlbSup]). This command instructs the specified
device to invalidate the given range of addresses in its IOTLB. The
size of the invalidate command is determined by the S bit and the
address.
""

And it has one implementation (though buggy) iommu_inval_iotlb() which
doesn't trigger any DEVIOTLB_UNMAP notifier.

We can leave this for the future.

(Last time I tried amd-iommu it didn't even boot).

Thanks

>
> >
> >      /* Update per-address-space notifier flags */
> >      vtd_as->notifier_flags = new;
> > --
> > 2.25.1
> >
>
> --
> Peter Xu
>
diff mbox series

Patch

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 9143376677..d025ef2873 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -3179,6 +3179,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) {
@@ -3193,6 +3194,13 @@  static int vtd_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu,
                          PCI_FUNC(vtd_as->devfn));
         return -ENOTSUP;
     }
+    if (!x86_iommu->dt_supported && (new & IOMMU_NOTIFIER_DEVIOTLB_UNMAP)) {
+        error_setg_errno(errp, ENOTSUP,
+                         "device %02x.%02x.%x requires device IOTLB mode",
+                         pci_bus_num(vtd_as->bus), PCI_SLOT(vtd_as->devfn),
+                         PCI_FUNC(vtd_as->devfn));
+        return -ENOTSUP;
+    }
 
     /* Update per-address-space notifier flags */
     vtd_as->notifier_flags = new;