| Message ID | 20221129081037.12099-2-jasowang@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix UNMAP notifier for intel-iommu | expand |
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,
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 >
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,
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 >
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 --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;
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(+)