| Message ID | 20221129081037.12099-3-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: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>
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 >
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;
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
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
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,
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 --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;
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(+)