diff mbox series

[1/3] intel-iommu: fail MAP notifier without caching mode

Message ID 20221129081037.12099-2-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 caching mode, MAP notifier won't work correctly since guest
won't send IOTLB update event when it establishes new mappings in the
I/O page tables. Let's fail the IOMMU notifiers early instead of
misbehaving silently.

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

Comments

Peter Xu Nov. 29, 2022, 3:35 p.m. UTC | #1
On Tue, Nov 29, 2022 at 04:10:35PM +0800, Jason Wang wrote:
> Without caching mode, MAP notifier won't work correctly since guest
> won't send IOTLB update event when it establishes new mappings in the
> I/O page tables. Let's fail the IOMMU notifiers early instead of
> misbehaving silently.
> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  hw/i386/intel_iommu.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index a08ee85edf..9143376677 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -3186,6 +3186,13 @@ static int vtd_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu,
>                           "Snoop Control with vhost or VFIO is not supported");
>          return -ENOTSUP;
>      }
> +    if (!s->caching_mode && (new & IOMMU_NOTIFIER_MAP)) {
> +        error_setg_errno(errp, ENOTSUP,
> +                         "device %02x.%02x.%x requires caching mode",
> +                         pci_bus_num(vtd_as->bus), PCI_SLOT(vtd_as->devfn),
> +                         PCI_FUNC(vtd_as->devfn));
> +        return -ENOTSUP;
> +    }

We used to have that but got reverted because it's too late to fail, so we
moved it over even though not as clean..

https://lore.kernel.org/all/20190916080718.3299-5-peterx@redhat.com/

Thanks,
Jason Wang Nov. 30, 2022, 6:23 a.m. UTC | #2
On Tue, Nov 29, 2022 at 11:35 PM Peter Xu <peterx@redhat.com> wrote:
>
> On Tue, Nov 29, 2022 at 04:10:35PM +0800, Jason Wang wrote:
> > Without caching mode, MAP notifier won't work correctly since guest
> > won't send IOTLB update event when it establishes new mappings in the
> > I/O page tables. Let's fail the IOMMU notifiers early instead of
> > misbehaving silently.
> >
> > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > ---
> >  hw/i386/intel_iommu.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > index a08ee85edf..9143376677 100644
> > --- a/hw/i386/intel_iommu.c
> > +++ b/hw/i386/intel_iommu.c
> > @@ -3186,6 +3186,13 @@ static int vtd_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu,
> >                           "Snoop Control with vhost or VFIO is not supported");
> >          return -ENOTSUP;
> >      }
> > +    if (!s->caching_mode && (new & IOMMU_NOTIFIER_MAP)) {
> > +        error_setg_errno(errp, ENOTSUP,
> > +                         "device %02x.%02x.%x requires caching mode",
> > +                         pci_bus_num(vtd_as->bus), PCI_SLOT(vtd_as->devfn),
> > +                         PCI_FUNC(vtd_as->devfn));
> > +        return -ENOTSUP;
> > +    }
>
> We used to have that but got reverted because it's too late to fail, so we
> moved it over even though not as clean..
>
> https://lore.kernel.org/all/20190916080718.3299-5-peterx@redhat.com/

One of the difference is that the patch doesn't do exit() here. I
think it's better to fail instead of misbehving silently, this is what
other vIOMMU did:

E.g in smmu we had:

    if (new & IOMMU_NOTIFIER_MAP) {
        error_setg(errp,
                   "device %02x.%02x.%x requires iommu MAP notifier which is "
                   "not currently supported", pci_bus_num(sdev->bus),
                   PCI_SLOT(sdev->devfn), PCI_FUNC(sdev->devfn));
        return -EINVAL;
    }

So did for amd iommu.

Thanks

>
> Thanks,
>
> --
> Peter Xu
>
Peter Xu Nov. 30, 2022, 3:06 p.m. UTC | #3
On Wed, Nov 30, 2022 at 02:23:06PM +0800, Jason Wang wrote:
> On Tue, Nov 29, 2022 at 11:35 PM Peter Xu <peterx@redhat.com> wrote:
> >
> > On Tue, Nov 29, 2022 at 04:10:35PM +0800, Jason Wang wrote:
> > > Without caching mode, MAP notifier won't work correctly since guest
> > > won't send IOTLB update event when it establishes new mappings in the
> > > I/O page tables. Let's fail the IOMMU notifiers early instead of
> > > misbehaving silently.
> > >
> > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > ---
> > >  hw/i386/intel_iommu.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > >
> > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > > index a08ee85edf..9143376677 100644
> > > --- a/hw/i386/intel_iommu.c
> > > +++ b/hw/i386/intel_iommu.c
> > > @@ -3186,6 +3186,13 @@ static int vtd_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu,
> > >                           "Snoop Control with vhost or VFIO is not supported");
> > >          return -ENOTSUP;
> > >      }
> > > +    if (!s->caching_mode && (new & IOMMU_NOTIFIER_MAP)) {
> > > +        error_setg_errno(errp, ENOTSUP,
> > > +                         "device %02x.%02x.%x requires caching mode",
> > > +                         pci_bus_num(vtd_as->bus), PCI_SLOT(vtd_as->devfn),
> > > +                         PCI_FUNC(vtd_as->devfn));
> > > +        return -ENOTSUP;
> > > +    }
> >
> > We used to have that but got reverted because it's too late to fail, so we
> > moved it over even though not as clean..
> >
> > https://lore.kernel.org/all/20190916080718.3299-5-peterx@redhat.com/
> 
> One of the difference is that the patch doesn't do exit() here. I
> think it's better to fail instead of misbehving silently, this is what
> other vIOMMU did:
> 
> E.g in smmu we had:
> 
>     if (new & IOMMU_NOTIFIER_MAP) {
>         error_setg(errp,
>                    "device %02x.%02x.%x requires iommu MAP notifier which is "
>                    "not currently supported", pci_bus_num(sdev->bus),
>                    PCI_SLOT(sdev->devfn), PCI_FUNC(sdev->devfn));
>         return -EINVAL;
>     }
> 
> So did for amd iommu.

Yeah that's fine.  Would you mind add the explanation (and also above link,
which might be helpful to pick up the history..) to the commit message?
With that:

Reviewed-by: Peter Xu <peterx@redhat.com>

Thanks,
Jason Wang Dec. 1, 2022, 8:46 a.m. UTC | #4
On Wed, Nov 30, 2022 at 11:06 PM Peter Xu <peterx@redhat.com> wrote:
>
> On Wed, Nov 30, 2022 at 02:23:06PM +0800, Jason Wang wrote:
> > On Tue, Nov 29, 2022 at 11:35 PM Peter Xu <peterx@redhat.com> wrote:
> > >
> > > On Tue, Nov 29, 2022 at 04:10:35PM +0800, Jason Wang wrote:
> > > > Without caching mode, MAP notifier won't work correctly since guest
> > > > won't send IOTLB update event when it establishes new mappings in the
> > > > I/O page tables. Let's fail the IOMMU notifiers early instead of
> > > > misbehaving silently.
> > > >
> > > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > > ---
> > > >  hw/i386/intel_iommu.c | 7 +++++++
> > > >  1 file changed, 7 insertions(+)
> > > >
> > > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > > > index a08ee85edf..9143376677 100644
> > > > --- a/hw/i386/intel_iommu.c
> > > > +++ b/hw/i386/intel_iommu.c
> > > > @@ -3186,6 +3186,13 @@ static int vtd_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu,
> > > >                           "Snoop Control with vhost or VFIO is not supported");
> > > >          return -ENOTSUP;
> > > >      }
> > > > +    if (!s->caching_mode && (new & IOMMU_NOTIFIER_MAP)) {
> > > > +        error_setg_errno(errp, ENOTSUP,
> > > > +                         "device %02x.%02x.%x requires caching mode",
> > > > +                         pci_bus_num(vtd_as->bus), PCI_SLOT(vtd_as->devfn),
> > > > +                         PCI_FUNC(vtd_as->devfn));
> > > > +        return -ENOTSUP;
> > > > +    }
> > >
> > > We used to have that but got reverted because it's too late to fail, so we
> > > moved it over even though not as clean..
> > >
> > > https://lore.kernel.org/all/20190916080718.3299-5-peterx@redhat.com/
> >
> > One of the difference is that the patch doesn't do exit() here. I
> > think it's better to fail instead of misbehving silently, this is what
> > other vIOMMU did:
> >
> > E.g in smmu we had:
> >
> >     if (new & IOMMU_NOTIFIER_MAP) {
> >         error_setg(errp,
> >                    "device %02x.%02x.%x requires iommu MAP notifier which is "
> >                    "not currently supported", pci_bus_num(sdev->bus),
> >                    PCI_SLOT(sdev->devfn), PCI_FUNC(sdev->devfn));
> >         return -EINVAL;
> >     }
> >
> > So did for amd iommu.
>
> Yeah that's fine.  Would you mind add the explanation (and also above link,
> which might be helpful to pick up the history..) to the commit message?

Will do.

> With that:
>
> Reviewed-by: Peter Xu <peterx@redhat.com>
>
> Thanks,

Thanks

>
> --
> Peter Xu
>
Eric Auger Dec. 6, 2022, 1:23 p.m. UTC | #5
Hi Jason,

On 11/29/22 09:10, Jason Wang wrote:
> Without caching mode, MAP notifier won't work correctly since guest
> won't send IOTLB update event when it establishes new mappings in the
> I/O page tables. Let's fail the IOMMU notifiers early instead of
> misbehaving silently.
>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric
> ---
>  hw/i386/intel_iommu.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index a08ee85edf..9143376677 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -3186,6 +3186,13 @@ static int vtd_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu,
>                           "Snoop Control with vhost or VFIO is not supported");
>          return -ENOTSUP;
>      }
> +    if (!s->caching_mode && (new & IOMMU_NOTIFIER_MAP)) {
> +        error_setg_errno(errp, ENOTSUP,
> +                         "device %02x.%02x.%x requires caching 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;
diff mbox series

Patch

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index a08ee85edf..9143376677 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -3186,6 +3186,13 @@  static int vtd_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu,
                          "Snoop Control with vhost or VFIO is not supported");
         return -ENOTSUP;
     }
+    if (!s->caching_mode && (new & IOMMU_NOTIFIER_MAP)) {
+        error_setg_errno(errp, ENOTSUP,
+                         "device %02x.%02x.%x requires caching 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;