* [PATCH 0/3] Fix UNMAP notifier for intel-iommu @ 2022-11-29 8:10 Jason Wang 2022-11-29 8:10 ` [PATCH 1/3] intel-iommu: fail MAP notifier without caching mode Jason Wang ` (5 more replies) 0 siblings, 6 replies; 35+ messages in thread From: Jason Wang @ 2022-11-29 8:10 UTC (permalink / raw) To: mst, peterx; +Cc: qemu-devel, eric.auger, viktor, Jason Wang Hi All: According to ATS, device should work if ATS is disabled. This is not correctly implemented in the current intel-iommu since it doesn't handle the UNMAP notifier correctly. This breaks the vhost-net + vIOMMU without dt. The root casue is that the when there's a device IOTLB miss (note that it's not specific to PCI so it can work without ATS), Qemu doesn't build the IOVA tree, so when guest start an IOTLB invalidation, Qemu won't trigger the UNMAP notifier. Fixing by build IOVA tree during IOMMU translsation. Thanks Jason Wang (3): intel-iommu: fail MAP notifier without caching mode intel-iommu: fail DEVIOTLB_UNMAP without dt mode intel-iommu: build iova tree during IOMMU translation hw/i386/intel_iommu.c | 58 ++++++++++++++++++++++++------------------- 1 file changed, 33 insertions(+), 25 deletions(-) -- 2.25.1 ^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 1/3] intel-iommu: fail MAP notifier without caching mode 2022-11-29 8:10 [PATCH 0/3] Fix UNMAP notifier for intel-iommu Jason Wang @ 2022-11-29 8:10 ` Jason Wang 2022-11-29 15:35 ` Peter Xu 2022-12-06 13:23 ` Eric Auger 2022-11-29 8:10 ` [PATCH 2/3] intel-iommu: fail DEVIOTLB_UNMAP without dt mode Jason Wang ` (4 subsequent siblings) 5 siblings, 2 replies; 35+ messages in thread From: Jason Wang @ 2022-11-29 8:10 UTC (permalink / raw) To: mst, peterx; +Cc: qemu-devel, eric.auger, viktor, Jason Wang 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; + } /* Update per-address-space notifier flags */ vtd_as->notifier_flags = new; -- 2.25.1 ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH 1/3] intel-iommu: fail MAP notifier without caching mode 2022-11-29 8:10 ` [PATCH 1/3] intel-iommu: fail MAP notifier without caching mode Jason Wang @ 2022-11-29 15:35 ` Peter Xu 2022-11-30 6:23 ` Jason Wang 2022-12-06 13:23 ` Eric Auger 1 sibling, 1 reply; 35+ messages in thread From: Peter Xu @ 2022-11-29 15:35 UTC (permalink / raw) To: Jason Wang; +Cc: mst, qemu-devel, eric.auger, viktor 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, -- Peter Xu ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 1/3] intel-iommu: fail MAP notifier without caching mode 2022-11-29 15:35 ` Peter Xu @ 2022-11-30 6:23 ` Jason Wang 2022-11-30 15:06 ` Peter Xu 0 siblings, 1 reply; 35+ messages in thread From: Jason Wang @ 2022-11-30 6:23 UTC (permalink / raw) To: Peter Xu; +Cc: mst, qemu-devel, eric.auger, viktor 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 > ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 1/3] intel-iommu: fail MAP notifier without caching mode 2022-11-30 6:23 ` Jason Wang @ 2022-11-30 15:06 ` Peter Xu 2022-12-01 8:46 ` Jason Wang 0 siblings, 1 reply; 35+ messages in thread From: Peter Xu @ 2022-11-30 15:06 UTC (permalink / raw) To: Jason Wang; +Cc: mst, qemu-devel, eric.auger, viktor 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, -- Peter Xu ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 1/3] intel-iommu: fail MAP notifier without caching mode 2022-11-30 15:06 ` Peter Xu @ 2022-12-01 8:46 ` Jason Wang 0 siblings, 0 replies; 35+ messages in thread From: Jason Wang @ 2022-12-01 8:46 UTC (permalink / raw) To: Peter Xu; +Cc: mst, qemu-devel, eric.auger, viktor 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 > ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 1/3] intel-iommu: fail MAP notifier without caching mode 2022-11-29 8:10 ` [PATCH 1/3] intel-iommu: fail MAP notifier without caching mode Jason Wang 2022-11-29 15:35 ` Peter Xu @ 2022-12-06 13:23 ` Eric Auger 1 sibling, 0 replies; 35+ messages in thread From: Eric Auger @ 2022-12-06 13:23 UTC (permalink / raw) To: Jason Wang, mst, peterx; +Cc: qemu-devel, viktor 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; ^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 2/3] intel-iommu: fail DEVIOTLB_UNMAP without dt mode 2022-11-29 8:10 [PATCH 0/3] Fix UNMAP notifier for intel-iommu Jason Wang 2022-11-29 8:10 ` [PATCH 1/3] intel-iommu: fail MAP notifier without caching mode Jason Wang @ 2022-11-29 8:10 ` Jason Wang 2022-11-29 15:38 ` Peter Xu ` (3 more replies) 2022-11-29 8:10 ` [PATCH 3/3] intel-iommu: build iova tree during IOMMU translation Jason Wang ` (3 subsequent siblings) 5 siblings, 4 replies; 35+ messages in thread From: Jason Wang @ 2022-11-29 8:10 UTC (permalink / raw) To: mst, peterx; +Cc: qemu-devel, eric.auger, viktor, Jason Wang 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; -- 2.25.1 ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH 2/3] intel-iommu: fail DEVIOTLB_UNMAP without dt mode 2022-11-29 8:10 ` [PATCH 2/3] intel-iommu: fail DEVIOTLB_UNMAP without dt mode Jason Wang @ 2022-11-29 15:38 ` Peter Xu 2022-12-01 16:03 ` Peter Xu ` (2 subsequent siblings) 3 siblings, 0 replies; 35+ messages in thread From: Peter Xu @ 2022-11-29 15:38 UTC (permalink / raw) To: Jason Wang; +Cc: mst, qemu-devel, eric.auger, viktor 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 ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 2/3] intel-iommu: fail DEVIOTLB_UNMAP without dt mode 2022-11-29 8:10 ` [PATCH 2/3] intel-iommu: fail DEVIOTLB_UNMAP without dt mode Jason Wang 2022-11-29 15:38 ` Peter Xu @ 2022-12-01 16:03 ` Peter Xu 2023-02-23 3:19 ` Jason Wang 2022-12-06 13:33 ` Eric Auger 2023-02-03 9:08 ` Laurent Vivier 3 siblings, 1 reply; 35+ messages in thread From: Peter Xu @ 2022-12-01 16:03 UTC (permalink / raw) To: Jason Wang; +Cc: mst, qemu-devel, eric.auger, viktor 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 > -- Peter Xu ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 2/3] intel-iommu: fail DEVIOTLB_UNMAP without dt mode 2022-12-01 16:03 ` Peter Xu @ 2023-02-23 3:19 ` Jason Wang 0 siblings, 0 replies; 35+ messages in thread From: Jason Wang @ 2023-02-23 3:19 UTC (permalink / raw) To: Peter Xu; +Cc: mst, qemu-devel, eric.auger, viktor 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 > ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 2/3] intel-iommu: fail DEVIOTLB_UNMAP without dt mode 2022-11-29 8:10 ` [PATCH 2/3] intel-iommu: fail DEVIOTLB_UNMAP without dt mode Jason Wang 2022-11-29 15:38 ` Peter Xu 2022-12-01 16:03 ` Peter Xu @ 2022-12-06 13:33 ` Eric Auger 2023-02-03 9:08 ` Laurent Vivier 3 siblings, 0 replies; 35+ messages in thread From: Eric Auger @ 2022-12-06 13:33 UTC (permalink / raw) To: Jason Wang, mst, peterx; +Cc: qemu-devel, viktor 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; ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 2/3] intel-iommu: fail DEVIOTLB_UNMAP without dt mode 2022-11-29 8:10 ` [PATCH 2/3] intel-iommu: fail DEVIOTLB_UNMAP without dt mode Jason Wang ` (2 preceding siblings ...) 2022-12-06 13:33 ` Eric Auger @ 2023-02-03 9:08 ` Laurent Vivier 2023-02-07 16:17 ` Laurent Vivier 3 siblings, 1 reply; 35+ messages in thread From: Laurent Vivier @ 2023-02-03 9:08 UTC (permalink / raw) To: Jason Wang, mst, peterx; +Cc: qemu-devel, eric.auger, viktor 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 ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 2/3] intel-iommu: fail DEVIOTLB_UNMAP without dt mode 2023-02-03 9:08 ` Laurent Vivier @ 2023-02-07 16:17 ` Laurent Vivier 2023-02-07 16:35 ` Peter Xu 0 siblings, 1 reply; 35+ messages in thread From: Laurent Vivier @ 2023-02-07 16:17 UTC (permalink / raw) To: Jason Wang, mst, peterx; +Cc: qemu-devel, eric.auger, viktor 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 ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 2/3] intel-iommu: fail DEVIOTLB_UNMAP without dt mode 2023-02-07 16:17 ` Laurent Vivier @ 2023-02-07 16:35 ` Peter Xu 0 siblings, 0 replies; 35+ messages in thread From: Peter Xu @ 2023-02-07 16:35 UTC (permalink / raw) To: Laurent Vivier; +Cc: Jason Wang, mst, qemu-devel, eric.auger, viktor 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, -- Peter Xu ^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 3/3] intel-iommu: build iova tree during IOMMU translation 2022-11-29 8:10 [PATCH 0/3] Fix UNMAP notifier for intel-iommu Jason Wang 2022-11-29 8:10 ` [PATCH 1/3] intel-iommu: fail MAP notifier without caching mode Jason Wang 2022-11-29 8:10 ` [PATCH 2/3] intel-iommu: fail DEVIOTLB_UNMAP without dt mode Jason Wang @ 2022-11-29 8:10 ` Jason Wang 2022-11-29 15:57 ` Peter Xu 2022-11-30 16:37 ` [PATCH 0/3] Fix UNMAP notifier for intel-iommu Michael S. Tsirkin ` (2 subsequent siblings) 5 siblings, 1 reply; 35+ messages in thread From: Jason Wang @ 2022-11-29 8:10 UTC (permalink / raw) To: mst, peterx; +Cc: qemu-devel, eric.auger, viktor, Jason Wang The IOVA tree is only built during page walk this breaks the device that tries to use UNMAP notifier only. One example is vhost-net, it tries to use UNMAP notifier when vIOMMU doesn't support DEVIOTLB_UNMAP notifier (e.g when dt mode is not enabled). The interesting part is that it doesn't use MAP since it can query the IOMMU translation by itself upon a IOTLB miss. This doesn't work since Qemu doesn't build IOVA tree in IOMMU translation which means the UNMAP notifier won't be triggered during the page walk since Qemu think it is never mapped. This could be noticed when vIOMMU is used with vhost_net but dt is disabled. Fixing this by build the iova tree during IOMMU translation, this makes sure the UNMAP notifier event could be identified during page walk. And we need to walk page table not only for UNMAP notifier but for MAP notifier during PSI. Signed-off-by: Jason Wang <jasowang@redhat.com> --- hw/i386/intel_iommu.c | 43 ++++++++++++++++++------------------------- 1 file changed, 18 insertions(+), 25 deletions(-) diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index d025ef2873..edeb62f4b2 100644 --- a/hw/i386/intel_iommu.c +++ b/hw/i386/intel_iommu.c @@ -1834,6 +1834,8 @@ static bool vtd_do_iommu_translate(VTDAddressSpace *vtd_as, PCIBus *bus, uint8_t access_flags; bool rid2pasid = (pasid == PCI_NO_PASID) && s->root_scalable; VTDIOTLBEntry *iotlb_entry; + const DMAMap *mapped; + DMAMap target; /* * We have standalone memory region for interrupt addresses, we @@ -1954,6 +1956,21 @@ out: entry->translated_addr = vtd_get_slpte_addr(slpte, s->aw_bits) & page_mask; entry->addr_mask = ~page_mask; entry->perm = access_flags; + + target.iova = entry->iova; + target.size = entry->addr_mask; + target.translated_addr = entry->translated_addr; + target.perm = entry->perm; + + mapped = iova_tree_find(vtd_as->iova_tree, &target); + if (!mapped) { + /* To make UNMAP notifier work, we need build iova tree here + * in order to have the UNMAP iommu notifier to be triggered + * during the page walk. + */ + iova_tree_insert(vtd_as->iova_tree, &target); + } + return true; error: @@ -2161,31 +2178,7 @@ static void vtd_iotlb_page_invalidate_notify(IntelIOMMUState *s, ret = vtd_dev_to_context_entry(s, pci_bus_num(vtd_as->bus), vtd_as->devfn, &ce); if (!ret && domain_id == vtd_get_domain_id(s, &ce, vtd_as->pasid)) { - if (vtd_as_has_map_notifier(vtd_as)) { - /* - * As long as we have MAP notifications registered in - * any of our IOMMU notifiers, we need to sync the - * shadow page table. - */ - vtd_sync_shadow_page_table_range(vtd_as, &ce, addr, size); - } else { - /* - * For UNMAP-only notifiers, we don't need to walk the - * page tables. We just deliver the PSI down to - * invalidate caches. - */ - IOMMUTLBEvent event = { - .type = IOMMU_NOTIFIER_UNMAP, - .entry = { - .target_as = &address_space_memory, - .iova = addr, - .translated_addr = 0, - .addr_mask = size - 1, - .perm = IOMMU_NONE, - }, - }; - memory_region_notify_iommu(&vtd_as->iommu, 0, event); - } + vtd_sync_shadow_page_table_range(vtd_as, &ce, addr, size); } } } -- 2.25.1 ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH 3/3] intel-iommu: build iova tree during IOMMU translation 2022-11-29 8:10 ` [PATCH 3/3] intel-iommu: build iova tree during IOMMU translation Jason Wang @ 2022-11-29 15:57 ` Peter Xu 2022-11-30 6:33 ` Jason Wang 0 siblings, 1 reply; 35+ messages in thread From: Peter Xu @ 2022-11-29 15:57 UTC (permalink / raw) To: Jason Wang; +Cc: mst, qemu-devel, eric.auger, viktor On Tue, Nov 29, 2022 at 04:10:37PM +0800, Jason Wang wrote: > The IOVA tree is only built during page walk this breaks the device > that tries to use UNMAP notifier only. One example is vhost-net, it > tries to use UNMAP notifier when vIOMMU doesn't support DEVIOTLB_UNMAP > notifier (e.g when dt mode is not enabled). The interesting part is > that it doesn't use MAP since it can query the IOMMU translation by > itself upon a IOTLB miss. > > This doesn't work since Qemu doesn't build IOVA tree in IOMMU > translation which means the UNMAP notifier won't be triggered during > the page walk since Qemu think it is never mapped. This could be > noticed when vIOMMU is used with vhost_net but dt is disabled. > > Fixing this by build the iova tree during IOMMU translation, this > makes sure the UNMAP notifier event could be identified during page > walk. And we need to walk page table not only for UNMAP notifier but > for MAP notifier during PSI. > > Signed-off-by: Jason Wang <jasowang@redhat.com> > --- > hw/i386/intel_iommu.c | 43 ++++++++++++++++++------------------------- > 1 file changed, 18 insertions(+), 25 deletions(-) > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > index d025ef2873..edeb62f4b2 100644 > --- a/hw/i386/intel_iommu.c > +++ b/hw/i386/intel_iommu.c > @@ -1834,6 +1834,8 @@ static bool vtd_do_iommu_translate(VTDAddressSpace *vtd_as, PCIBus *bus, > uint8_t access_flags; > bool rid2pasid = (pasid == PCI_NO_PASID) && s->root_scalable; > VTDIOTLBEntry *iotlb_entry; > + const DMAMap *mapped; > + DMAMap target; > > /* > * We have standalone memory region for interrupt addresses, we > @@ -1954,6 +1956,21 @@ out: > entry->translated_addr = vtd_get_slpte_addr(slpte, s->aw_bits) & page_mask; > entry->addr_mask = ~page_mask; > entry->perm = access_flags; > + > + target.iova = entry->iova; > + target.size = entry->addr_mask; > + target.translated_addr = entry->translated_addr; > + target.perm = entry->perm; > + > + mapped = iova_tree_find(vtd_as->iova_tree, &target); > + if (!mapped) { > + /* To make UNMAP notifier work, we need build iova tree here > + * in order to have the UNMAP iommu notifier to be triggered > + * during the page walk. > + */ > + iova_tree_insert(vtd_as->iova_tree, &target); > + } > + > return true; > > error: > @@ -2161,31 +2178,7 @@ static void vtd_iotlb_page_invalidate_notify(IntelIOMMUState *s, > ret = vtd_dev_to_context_entry(s, pci_bus_num(vtd_as->bus), > vtd_as->devfn, &ce); > if (!ret && domain_id == vtd_get_domain_id(s, &ce, vtd_as->pasid)) { > - if (vtd_as_has_map_notifier(vtd_as)) { > - /* > - * As long as we have MAP notifications registered in > - * any of our IOMMU notifiers, we need to sync the > - * shadow page table. > - */ > - vtd_sync_shadow_page_table_range(vtd_as, &ce, addr, size); > - } else { > - /* > - * For UNMAP-only notifiers, we don't need to walk the > - * page tables. We just deliver the PSI down to > - * invalidate caches. > - */ > - IOMMUTLBEvent event = { > - .type = IOMMU_NOTIFIER_UNMAP, > - .entry = { > - .target_as = &address_space_memory, > - .iova = addr, > - .translated_addr = 0, > - .addr_mask = size - 1, > - .perm = IOMMU_NONE, > - }, > - }; > - memory_region_notify_iommu(&vtd_as->iommu, 0, event); Isn't this path the one that will be responsible for pass-through the UNMAP events from guest to vhost when there's no MAP notifier requested? At least that's what I expected when introducing the iova tree, because for unmap-only device hierachy I thought we didn't need the tree at all. Jason, do you know where I miss? Thanks, > - } > + vtd_sync_shadow_page_table_range(vtd_as, &ce, addr, size); > } > } > } > -- > 2.25.1 > -- Peter Xu ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 3/3] intel-iommu: build iova tree during IOMMU translation 2022-11-29 15:57 ` Peter Xu @ 2022-11-30 6:33 ` Jason Wang 2022-11-30 15:17 ` Peter Xu 0 siblings, 1 reply; 35+ messages in thread From: Jason Wang @ 2022-11-30 6:33 UTC (permalink / raw) To: Peter Xu; +Cc: mst, qemu-devel, eric.auger, viktor On Tue, Nov 29, 2022 at 11:57 PM Peter Xu <peterx@redhat.com> wrote: > > On Tue, Nov 29, 2022 at 04:10:37PM +0800, Jason Wang wrote: > > The IOVA tree is only built during page walk this breaks the device > > that tries to use UNMAP notifier only. One example is vhost-net, it > > tries to use UNMAP notifier when vIOMMU doesn't support DEVIOTLB_UNMAP > > notifier (e.g when dt mode is not enabled). The interesting part is > > that it doesn't use MAP since it can query the IOMMU translation by > > itself upon a IOTLB miss. > > > > This doesn't work since Qemu doesn't build IOVA tree in IOMMU > > translation which means the UNMAP notifier won't be triggered during > > the page walk since Qemu think it is never mapped. This could be > > noticed when vIOMMU is used with vhost_net but dt is disabled. > > > > Fixing this by build the iova tree during IOMMU translation, this > > makes sure the UNMAP notifier event could be identified during page > > walk. And we need to walk page table not only for UNMAP notifier but > > for MAP notifier during PSI. > > > > Signed-off-by: Jason Wang <jasowang@redhat.com> > > --- > > hw/i386/intel_iommu.c | 43 ++++++++++++++++++------------------------- > > 1 file changed, 18 insertions(+), 25 deletions(-) > > > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > > index d025ef2873..edeb62f4b2 100644 > > --- a/hw/i386/intel_iommu.c > > +++ b/hw/i386/intel_iommu.c > > @@ -1834,6 +1834,8 @@ static bool vtd_do_iommu_translate(VTDAddressSpace *vtd_as, PCIBus *bus, > > uint8_t access_flags; > > bool rid2pasid = (pasid == PCI_NO_PASID) && s->root_scalable; > > VTDIOTLBEntry *iotlb_entry; > > + const DMAMap *mapped; > > + DMAMap target; > > > > /* > > * We have standalone memory region for interrupt addresses, we > > @@ -1954,6 +1956,21 @@ out: > > entry->translated_addr = vtd_get_slpte_addr(slpte, s->aw_bits) & page_mask; > > entry->addr_mask = ~page_mask; > > entry->perm = access_flags; > > + > > + target.iova = entry->iova; > > + target.size = entry->addr_mask; > > + target.translated_addr = entry->translated_addr; > > + target.perm = entry->perm; > > + > > + mapped = iova_tree_find(vtd_as->iova_tree, &target); > > + if (!mapped) { > > + /* To make UNMAP notifier work, we need build iova tree here > > + * in order to have the UNMAP iommu notifier to be triggered > > + * during the page walk. > > + */ > > + iova_tree_insert(vtd_as->iova_tree, &target); > > + } > > + > > return true; > > > > error: > > @@ -2161,31 +2178,7 @@ static void vtd_iotlb_page_invalidate_notify(IntelIOMMUState *s, > > ret = vtd_dev_to_context_entry(s, pci_bus_num(vtd_as->bus), > > vtd_as->devfn, &ce); > > if (!ret && domain_id == vtd_get_domain_id(s, &ce, vtd_as->pasid)) { > > - if (vtd_as_has_map_notifier(vtd_as)) { > > - /* > > - * As long as we have MAP notifications registered in > > - * any of our IOMMU notifiers, we need to sync the > > - * shadow page table. > > - */ > > - vtd_sync_shadow_page_table_range(vtd_as, &ce, addr, size); > > - } else { > > - /* > > - * For UNMAP-only notifiers, we don't need to walk the > > - * page tables. We just deliver the PSI down to > > - * invalidate caches. > > - */ > > - IOMMUTLBEvent event = { > > - .type = IOMMU_NOTIFIER_UNMAP, > > - .entry = { > > - .target_as = &address_space_memory, > > - .iova = addr, > > - .translated_addr = 0, > > - .addr_mask = size - 1, > > - .perm = IOMMU_NONE, > > - }, > > - }; > > - memory_region_notify_iommu(&vtd_as->iommu, 0, event); > > Isn't this path the one that will be responsible for pass-through the UNMAP > events from guest to vhost when there's no MAP notifier requested? Yes, but it doesn't do the iova tree removing. More below. > > At least that's what I expected when introducing the iova tree, because for > unmap-only device hierachy I thought we didn't need the tree at all. Then the problem is the UNMAP notifier won't be trigger at all during DSI page walk in vtd_page_walk_one() because there's no DMAMap stored in the iova tree.: if (!mapped) { /* Skip since we didn't map this range at all */ trace_vtd_page_walk_one_skip_unmap(entry->iova, entry->addr_mask); return 0; } So I choose to build the iova tree in translate then we won't go within the above condition. Thanks > > Jason, do you know where I miss? > > Thanks, > > > - } > > + vtd_sync_shadow_page_table_range(vtd_as, &ce, addr, size); > > } > > } > > } > > -- > > 2.25.1 > > > > -- > Peter Xu > ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 3/3] intel-iommu: build iova tree during IOMMU translation 2022-11-30 6:33 ` Jason Wang @ 2022-11-30 15:17 ` Peter Xu 2022-12-01 8:35 ` Jason Wang 0 siblings, 1 reply; 35+ messages in thread From: Peter Xu @ 2022-11-30 15:17 UTC (permalink / raw) To: Jason Wang; +Cc: mst, qemu-devel, eric.auger, viktor On Wed, Nov 30, 2022 at 02:33:51PM +0800, Jason Wang wrote: > On Tue, Nov 29, 2022 at 11:57 PM Peter Xu <peterx@redhat.com> wrote: > > > > On Tue, Nov 29, 2022 at 04:10:37PM +0800, Jason Wang wrote: > > > The IOVA tree is only built during page walk this breaks the device > > > that tries to use UNMAP notifier only. One example is vhost-net, it > > > tries to use UNMAP notifier when vIOMMU doesn't support DEVIOTLB_UNMAP > > > notifier (e.g when dt mode is not enabled). The interesting part is > > > that it doesn't use MAP since it can query the IOMMU translation by > > > itself upon a IOTLB miss. > > > > > > This doesn't work since Qemu doesn't build IOVA tree in IOMMU > > > translation which means the UNMAP notifier won't be triggered during > > > the page walk since Qemu think it is never mapped. This could be > > > noticed when vIOMMU is used with vhost_net but dt is disabled. > > > > > > Fixing this by build the iova tree during IOMMU translation, this > > > makes sure the UNMAP notifier event could be identified during page > > > walk. And we need to walk page table not only for UNMAP notifier but > > > for MAP notifier during PSI. > > > > > > Signed-off-by: Jason Wang <jasowang@redhat.com> > > > --- > > > hw/i386/intel_iommu.c | 43 ++++++++++++++++++------------------------- > > > 1 file changed, 18 insertions(+), 25 deletions(-) > > > > > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > > > index d025ef2873..edeb62f4b2 100644 > > > --- a/hw/i386/intel_iommu.c > > > +++ b/hw/i386/intel_iommu.c > > > @@ -1834,6 +1834,8 @@ static bool vtd_do_iommu_translate(VTDAddressSpace *vtd_as, PCIBus *bus, > > > uint8_t access_flags; > > > bool rid2pasid = (pasid == PCI_NO_PASID) && s->root_scalable; > > > VTDIOTLBEntry *iotlb_entry; > > > + const DMAMap *mapped; > > > + DMAMap target; > > > > > > /* > > > * We have standalone memory region for interrupt addresses, we > > > @@ -1954,6 +1956,21 @@ out: > > > entry->translated_addr = vtd_get_slpte_addr(slpte, s->aw_bits) & page_mask; > > > entry->addr_mask = ~page_mask; > > > entry->perm = access_flags; > > > + > > > + target.iova = entry->iova; > > > + target.size = entry->addr_mask; > > > + target.translated_addr = entry->translated_addr; > > > + target.perm = entry->perm; > > > + > > > + mapped = iova_tree_find(vtd_as->iova_tree, &target); > > > + if (!mapped) { > > > + /* To make UNMAP notifier work, we need build iova tree here > > > + * in order to have the UNMAP iommu notifier to be triggered > > > + * during the page walk. > > > + */ > > > + iova_tree_insert(vtd_as->iova_tree, &target); > > > + } > > > + > > > return true; > > > > > > error: > > > @@ -2161,31 +2178,7 @@ static void vtd_iotlb_page_invalidate_notify(IntelIOMMUState *s, > > > ret = vtd_dev_to_context_entry(s, pci_bus_num(vtd_as->bus), > > > vtd_as->devfn, &ce); > > > if (!ret && domain_id == vtd_get_domain_id(s, &ce, vtd_as->pasid)) { > > > - if (vtd_as_has_map_notifier(vtd_as)) { > > > - /* > > > - * As long as we have MAP notifications registered in > > > - * any of our IOMMU notifiers, we need to sync the > > > - * shadow page table. > > > - */ > > > - vtd_sync_shadow_page_table_range(vtd_as, &ce, addr, size); > > > - } else { > > > - /* > > > - * For UNMAP-only notifiers, we don't need to walk the > > > - * page tables. We just deliver the PSI down to > > > - * invalidate caches. > > > - */ > > > - IOMMUTLBEvent event = { > > > - .type = IOMMU_NOTIFIER_UNMAP, > > > - .entry = { > > > - .target_as = &address_space_memory, > > > - .iova = addr, > > > - .translated_addr = 0, > > > - .addr_mask = size - 1, > > > - .perm = IOMMU_NONE, > > > - }, > > > - }; > > > - memory_region_notify_iommu(&vtd_as->iommu, 0, event); > > > > Isn't this path the one that will be responsible for pass-through the UNMAP > > events from guest to vhost when there's no MAP notifier requested? > > Yes, but it doesn't do the iova tree removing. More below. > > > > > At least that's what I expected when introducing the iova tree, because for > > unmap-only device hierachy I thought we didn't need the tree at all. > > Then the problem is the UNMAP notifier won't be trigger at all during > DSI page walk in vtd_page_walk_one() because there's no DMAMap stored > in the iova tree.: > > if (!mapped) { > /* Skip since we didn't map this range at all */ > trace_vtd_page_walk_one_skip_unmap(entry->iova, entry->addr_mask); > return 0; > } > > So I choose to build the iova tree in translate then we won't go > within the above condition. That's also why it's weird because IIUC we should never walk a page table at all if there's no MAP notifier regiestered. When I'm looking at the walk callers I found that indeed there's one path missing where can cause it to actually walk the pgtables without !MAP, then I also noticed commit f7701e2c7983b6, and I'm wondering what we really want is something like this: diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index a08ee85edf..c46f3db992 100644 --- a/hw/i386/intel_iommu.c +++ b/hw/i386/intel_iommu.c @@ -1536,7 +1536,7 @@ static int vtd_sync_shadow_page_table(VTDAddressSpace *vtd_as) VTDContextEntry ce; IOMMUNotifier *n; - if (!(vtd_as->iommu.iommu_notify_flags & IOMMU_NOTIFIER_IOTLB_EVENTS)) { + if (!vtd_as_has_map_notifier(vtd_as)) { return 0; } So I'm not sure whether this patch is the problem resolver; so far I feel like it's patch 2 who does the real fix. Then we can have the above oneliner so we stop any walks when there's no map notifiers. Thanks, -- Peter Xu ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH 3/3] intel-iommu: build iova tree during IOMMU translation 2022-11-30 15:17 ` Peter Xu @ 2022-12-01 8:35 ` Jason Wang 2022-12-01 14:58 ` Peter Xu 0 siblings, 1 reply; 35+ messages in thread From: Jason Wang @ 2022-12-01 8:35 UTC (permalink / raw) To: Peter Xu; +Cc: mst, qemu-devel, eric.auger, viktor On Wed, Nov 30, 2022 at 11:17 PM Peter Xu <peterx@redhat.com> wrote: > > On Wed, Nov 30, 2022 at 02:33:51PM +0800, Jason Wang wrote: > > On Tue, Nov 29, 2022 at 11:57 PM Peter Xu <peterx@redhat.com> wrote: > > > > > > On Tue, Nov 29, 2022 at 04:10:37PM +0800, Jason Wang wrote: > > > > The IOVA tree is only built during page walk this breaks the device > > > > that tries to use UNMAP notifier only. One example is vhost-net, it > > > > tries to use UNMAP notifier when vIOMMU doesn't support DEVIOTLB_UNMAP > > > > notifier (e.g when dt mode is not enabled). The interesting part is > > > > that it doesn't use MAP since it can query the IOMMU translation by > > > > itself upon a IOTLB miss. > > > > > > > > This doesn't work since Qemu doesn't build IOVA tree in IOMMU > > > > translation which means the UNMAP notifier won't be triggered during > > > > the page walk since Qemu think it is never mapped. This could be > > > > noticed when vIOMMU is used with vhost_net but dt is disabled. > > > > > > > > Fixing this by build the iova tree during IOMMU translation, this > > > > makes sure the UNMAP notifier event could be identified during page > > > > walk. And we need to walk page table not only for UNMAP notifier but > > > > for MAP notifier during PSI. > > > > > > > > Signed-off-by: Jason Wang <jasowang@redhat.com> > > > > --- > > > > hw/i386/intel_iommu.c | 43 ++++++++++++++++++------------------------- > > > > 1 file changed, 18 insertions(+), 25 deletions(-) > > > > > > > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > > > > index d025ef2873..edeb62f4b2 100644 > > > > --- a/hw/i386/intel_iommu.c > > > > +++ b/hw/i386/intel_iommu.c > > > > @@ -1834,6 +1834,8 @@ static bool vtd_do_iommu_translate(VTDAddressSpace *vtd_as, PCIBus *bus, > > > > uint8_t access_flags; > > > > bool rid2pasid = (pasid == PCI_NO_PASID) && s->root_scalable; > > > > VTDIOTLBEntry *iotlb_entry; > > > > + const DMAMap *mapped; > > > > + DMAMap target; > > > > > > > > /* > > > > * We have standalone memory region for interrupt addresses, we > > > > @@ -1954,6 +1956,21 @@ out: > > > > entry->translated_addr = vtd_get_slpte_addr(slpte, s->aw_bits) & page_mask; > > > > entry->addr_mask = ~page_mask; > > > > entry->perm = access_flags; > > > > + > > > > + target.iova = entry->iova; > > > > + target.size = entry->addr_mask; > > > > + target.translated_addr = entry->translated_addr; > > > > + target.perm = entry->perm; > > > > + > > > > + mapped = iova_tree_find(vtd_as->iova_tree, &target); > > > > + if (!mapped) { > > > > + /* To make UNMAP notifier work, we need build iova tree here > > > > + * in order to have the UNMAP iommu notifier to be triggered > > > > + * during the page walk. > > > > + */ > > > > + iova_tree_insert(vtd_as->iova_tree, &target); > > > > + } > > > > + > > > > return true; > > > > > > > > error: > > > > @@ -2161,31 +2178,7 @@ static void vtd_iotlb_page_invalidate_notify(IntelIOMMUState *s, > > > > ret = vtd_dev_to_context_entry(s, pci_bus_num(vtd_as->bus), > > > > vtd_as->devfn, &ce); > > > > if (!ret && domain_id == vtd_get_domain_id(s, &ce, vtd_as->pasid)) { > > > > - if (vtd_as_has_map_notifier(vtd_as)) { > > > > - /* > > > > - * As long as we have MAP notifications registered in > > > > - * any of our IOMMU notifiers, we need to sync the > > > > - * shadow page table. > > > > - */ > > > > - vtd_sync_shadow_page_table_range(vtd_as, &ce, addr, size); > > > > - } else { > > > > - /* > > > > - * For UNMAP-only notifiers, we don't need to walk the > > > > - * page tables. We just deliver the PSI down to > > > > - * invalidate caches. > > > > - */ > > > > - IOMMUTLBEvent event = { > > > > - .type = IOMMU_NOTIFIER_UNMAP, > > > > - .entry = { > > > > - .target_as = &address_space_memory, > > > > - .iova = addr, > > > > - .translated_addr = 0, > > > > - .addr_mask = size - 1, > > > > - .perm = IOMMU_NONE, > > > > - }, > > > > - }; > > > > - memory_region_notify_iommu(&vtd_as->iommu, 0, event); > > > > > > Isn't this path the one that will be responsible for pass-through the UNMAP > > > events from guest to vhost when there's no MAP notifier requested? > > > > Yes, but it doesn't do the iova tree removing. More below. > > > > > > > > At least that's what I expected when introducing the iova tree, because for > > > unmap-only device hierachy I thought we didn't need the tree at all. > > > > Then the problem is the UNMAP notifier won't be trigger at all during > > DSI page walk in vtd_page_walk_one() because there's no DMAMap stored > > in the iova tree.: > > > > if (!mapped) { > > /* Skip since we didn't map this range at all */ > > trace_vtd_page_walk_one_skip_unmap(entry->iova, entry->addr_mask); > > return 0; > > } > > > > So I choose to build the iova tree in translate then we won't go > > within the above condition. > > That's also why it's weird because IIUC we should never walk a page table > at all if there's no MAP notifier regiestered. If this is true, we probably need to document this somewhere. > > When I'm looking at the walk callers I found that indeed there's one path > missing where can cause it to actually walk the pgtables without !MAP, then > I also noticed commit f7701e2c7983b6, and I'm wondering what we really want > is something like this: > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > index a08ee85edf..c46f3db992 100644 > --- a/hw/i386/intel_iommu.c > +++ b/hw/i386/intel_iommu.c > @@ -1536,7 +1536,7 @@ static int vtd_sync_shadow_page_table(VTDAddressSpace *vtd_as) > VTDContextEntry ce; > IOMMUNotifier *n; > > - if (!(vtd_as->iommu.iommu_notify_flags & IOMMU_NOTIFIER_IOTLB_EVENTS)) { > + if (!vtd_as_has_map_notifier(vtd_as)) { > return 0; > } > > So I'm not sure whether this patch is the problem resolver; so far I feel > like it's patch 2 who does the real fix. Then we can have the above > oneliner so we stop any walks when there's no map notifiers. > > Thanks, I may miss something but as state above, the problem is a missing UNMAP notification during DSI when there's only UNMAP notifier. To solve it we might have two ways: 1) build the iova tree during iommu translation then we can correctly trigger UNMAP during page walk caused by DSI 2) don't do the iova tree walk for !MAP notifier, need new logic to trigger UNMAP notifier in PSI/DSI This patch choose to go 1) (which seems easier at least for -stable). Do you mean you prefer to go with 2)? Thanks > > -- > Peter Xu > ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 3/3] intel-iommu: build iova tree during IOMMU translation 2022-12-01 8:35 ` Jason Wang @ 2022-12-01 14:58 ` Peter Xu 2022-12-05 4:12 ` Jason Wang 0 siblings, 1 reply; 35+ messages in thread From: Peter Xu @ 2022-12-01 14:58 UTC (permalink / raw) To: Jason Wang; +Cc: mst, qemu-devel, eric.auger, viktor On Thu, Dec 01, 2022 at 04:35:48PM +0800, Jason Wang wrote: > On Wed, Nov 30, 2022 at 11:17 PM Peter Xu <peterx@redhat.com> wrote: > > > > On Wed, Nov 30, 2022 at 02:33:51PM +0800, Jason Wang wrote: > > > On Tue, Nov 29, 2022 at 11:57 PM Peter Xu <peterx@redhat.com> wrote: > > > > > > > > On Tue, Nov 29, 2022 at 04:10:37PM +0800, Jason Wang wrote: > > > > > The IOVA tree is only built during page walk this breaks the device > > > > > that tries to use UNMAP notifier only. One example is vhost-net, it > > > > > tries to use UNMAP notifier when vIOMMU doesn't support DEVIOTLB_UNMAP > > > > > notifier (e.g when dt mode is not enabled). The interesting part is > > > > > that it doesn't use MAP since it can query the IOMMU translation by > > > > > itself upon a IOTLB miss. > > > > > > > > > > This doesn't work since Qemu doesn't build IOVA tree in IOMMU > > > > > translation which means the UNMAP notifier won't be triggered during > > > > > the page walk since Qemu think it is never mapped. This could be > > > > > noticed when vIOMMU is used with vhost_net but dt is disabled. > > > > > > > > > > Fixing this by build the iova tree during IOMMU translation, this > > > > > makes sure the UNMAP notifier event could be identified during page > > > > > walk. And we need to walk page table not only for UNMAP notifier but > > > > > for MAP notifier during PSI. > > > > > > > > > > Signed-off-by: Jason Wang <jasowang@redhat.com> > > > > > --- > > > > > hw/i386/intel_iommu.c | 43 ++++++++++++++++++------------------------- > > > > > 1 file changed, 18 insertions(+), 25 deletions(-) > > > > > > > > > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > > > > > index d025ef2873..edeb62f4b2 100644 > > > > > --- a/hw/i386/intel_iommu.c > > > > > +++ b/hw/i386/intel_iommu.c > > > > > @@ -1834,6 +1834,8 @@ static bool vtd_do_iommu_translate(VTDAddressSpace *vtd_as, PCIBus *bus, > > > > > uint8_t access_flags; > > > > > bool rid2pasid = (pasid == PCI_NO_PASID) && s->root_scalable; > > > > > VTDIOTLBEntry *iotlb_entry; > > > > > + const DMAMap *mapped; > > > > > + DMAMap target; > > > > > > > > > > /* > > > > > * We have standalone memory region for interrupt addresses, we > > > > > @@ -1954,6 +1956,21 @@ out: > > > > > entry->translated_addr = vtd_get_slpte_addr(slpte, s->aw_bits) & page_mask; > > > > > entry->addr_mask = ~page_mask; > > > > > entry->perm = access_flags; > > > > > + > > > > > + target.iova = entry->iova; > > > > > + target.size = entry->addr_mask; > > > > > + target.translated_addr = entry->translated_addr; > > > > > + target.perm = entry->perm; > > > > > + > > > > > + mapped = iova_tree_find(vtd_as->iova_tree, &target); > > > > > + if (!mapped) { > > > > > + /* To make UNMAP notifier work, we need build iova tree here > > > > > + * in order to have the UNMAP iommu notifier to be triggered > > > > > + * during the page walk. > > > > > + */ > > > > > + iova_tree_insert(vtd_as->iova_tree, &target); > > > > > + } > > > > > + > > > > > return true; > > > > > > > > > > error: > > > > > @@ -2161,31 +2178,7 @@ static void vtd_iotlb_page_invalidate_notify(IntelIOMMUState *s, > > > > > ret = vtd_dev_to_context_entry(s, pci_bus_num(vtd_as->bus), > > > > > vtd_as->devfn, &ce); > > > > > if (!ret && domain_id == vtd_get_domain_id(s, &ce, vtd_as->pasid)) { > > > > > - if (vtd_as_has_map_notifier(vtd_as)) { > > > > > - /* > > > > > - * As long as we have MAP notifications registered in > > > > > - * any of our IOMMU notifiers, we need to sync the > > > > > - * shadow page table. > > > > > - */ > > > > > - vtd_sync_shadow_page_table_range(vtd_as, &ce, addr, size); > > > > > - } else { > > > > > - /* > > > > > - * For UNMAP-only notifiers, we don't need to walk the > > > > > - * page tables. We just deliver the PSI down to > > > > > - * invalidate caches. > > > > > - */ > > > > > - IOMMUTLBEvent event = { > > > > > - .type = IOMMU_NOTIFIER_UNMAP, > > > > > - .entry = { > > > > > - .target_as = &address_space_memory, > > > > > - .iova = addr, > > > > > - .translated_addr = 0, > > > > > - .addr_mask = size - 1, > > > > > - .perm = IOMMU_NONE, > > > > > - }, > > > > > - }; > > > > > - memory_region_notify_iommu(&vtd_as->iommu, 0, event); [1] > > > > > > > > Isn't this path the one that will be responsible for pass-through the UNMAP > > > > events from guest to vhost when there's no MAP notifier requested? > > > > > > Yes, but it doesn't do the iova tree removing. More below. > > > > > > > > > > > At least that's what I expected when introducing the iova tree, because for > > > > unmap-only device hierachy I thought we didn't need the tree at all. > > > > > > Then the problem is the UNMAP notifier won't be trigger at all during > > > DSI page walk in vtd_page_walk_one() because there's no DMAMap stored > > > in the iova tree.: > > > > > > if (!mapped) { > > > /* Skip since we didn't map this range at all */ > > > trace_vtd_page_walk_one_skip_unmap(entry->iova, entry->addr_mask); > > > return 0; > > > } > > > > > > So I choose to build the iova tree in translate then we won't go > > > within the above condition. > > > > That's also why it's weird because IIUC we should never walk a page table > > at all if there's no MAP notifier regiestered. > > If this is true, we probably need to document this somewhere. Agree. I'll post a patch. > > > > > When I'm looking at the walk callers I found that indeed there's one path > > missing where can cause it to actually walk the pgtables without !MAP, then > > I also noticed commit f7701e2c7983b6, and I'm wondering what we really want > > is something like this: > > > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > > index a08ee85edf..c46f3db992 100644 > > --- a/hw/i386/intel_iommu.c > > +++ b/hw/i386/intel_iommu.c > > @@ -1536,7 +1536,7 @@ static int vtd_sync_shadow_page_table(VTDAddressSpace *vtd_as) > > VTDContextEntry ce; > > IOMMUNotifier *n; > > > > - if (!(vtd_as->iommu.iommu_notify_flags & IOMMU_NOTIFIER_IOTLB_EVENTS)) { > > + if (!vtd_as_has_map_notifier(vtd_as)) { > > return 0; > > } > > > > So I'm not sure whether this patch is the problem resolver; so far I feel > > like it's patch 2 who does the real fix. Then we can have the above > > oneliner so we stop any walks when there's no map notifiers. > > > > Thanks, > > I may miss something but as state above, the problem is a missing > UNMAP notification during DSI when there's only UNMAP notifier. I got confused too on why we didn't notify UNMAP for DSI already, that's so weird because I thought it should be there or it should be broken for a long time.. as we discussed multiple times around this one: /* * Fallback to domain selective flush if no PSI support or * the size is too big. */ if (!cap_pgsel_inv(iommu->cap) || mask > cap_max_amask_val(iommu->cap)) iommu->flush.flush_iotlb(iommu, did, 0, 0, DMA_TLB_DSI_FLUSH); else iommu->flush.flush_iotlb(iommu, did, addr | ih, mask, DMA_TLB_PSI_FLUSH); I guess we were just always lucky?.. > > To solve it we might have two ways: > > 1) build the iova tree during iommu translation then we can correctly > trigger UNMAP during page walk caused by DSI > 2) don't do the iova tree walk for !MAP notifier, need new logic to > trigger UNMAP notifier in PSI/DSI > > This patch choose to go 1) (which seems easier at least for -stable). > Do you mean you prefer to go with 2)? Yes. IOVA tree is unnecessary overhead IMHO because UNMAP (both IOTLB or DEVIOTLB unmap) shouldn't need that complexity at all. Using the iova tree can be accurate on which page got unmapped when the kernel driver used DSI for a large PSI as shown above, however IMHO it needs more justification that the pgtable walk is worth the effort. Not to mention if a device in QEMU that wants to use the iova tree for some reason, one can just register with MAP and ignore all MAP events, while by default we keep UNMAP simple. So we could do: (1) Rename vtd_sync_shadow_page_table() to vtd_sync_domain() (2) instead of optimizing dev-iotlb only there: if (!(vtd_as->iommu.iommu_notify_flags & IOMMU_NOTIFIER_IOTLB_EVENTS)) { return 0; } we should firstly check if UNMAP or DEVUNMAP registered, we directly send a notification to the whole domain. We need to choose the event that the register happens with but not both. This also reminded me that whether we should sanity check on iommu notifiers on some invalid cases. E.g. it seems to me when registered with DEVIOTLB_UNMAP it should not register with either MAP or UNMAP anymore or it doesn't make sense. One step further, I'm wondering whether the DEV_IOTLB event should exist at all. Maybe we want to have DEV_IOTLB typed iommu notifier only, but then when we got dev-iotlb PSI/DSI we notify with type=UNMAP just like normal UNMAP events, then in above (2) we can send UNMAP constantly as long as !MAP. But even if so that'll just be another optional change on top. Thanks, -- Peter Xu ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 3/3] intel-iommu: build iova tree during IOMMU translation 2022-12-01 14:58 ` Peter Xu @ 2022-12-05 4:12 ` Jason Wang 2022-12-05 23:18 ` Peter Xu 0 siblings, 1 reply; 35+ messages in thread From: Jason Wang @ 2022-12-05 4:12 UTC (permalink / raw) To: Peter Xu; +Cc: mst, qemu-devel, eric.auger, viktor ` On Thu, Dec 1, 2022 at 10:59 PM Peter Xu <peterx@redhat.com> wrote: > > On Thu, Dec 01, 2022 at 04:35:48PM +0800, Jason Wang wrote: > > On Wed, Nov 30, 2022 at 11:17 PM Peter Xu <peterx@redhat.com> wrote: > > > > > > On Wed, Nov 30, 2022 at 02:33:51PM +0800, Jason Wang wrote: > > > > On Tue, Nov 29, 2022 at 11:57 PM Peter Xu <peterx@redhat.com> wrote: > > > > > > > > > > On Tue, Nov 29, 2022 at 04:10:37PM +0800, Jason Wang wrote: > > > > > > The IOVA tree is only built during page walk this breaks the device > > > > > > that tries to use UNMAP notifier only. One example is vhost-net, it > > > > > > tries to use UNMAP notifier when vIOMMU doesn't support DEVIOTLB_UNMAP > > > > > > notifier (e.g when dt mode is not enabled). The interesting part is > > > > > > that it doesn't use MAP since it can query the IOMMU translation by > > > > > > itself upon a IOTLB miss. > > > > > > > > > > > > This doesn't work since Qemu doesn't build IOVA tree in IOMMU > > > > > > translation which means the UNMAP notifier won't be triggered during > > > > > > the page walk since Qemu think it is never mapped. This could be > > > > > > noticed when vIOMMU is used with vhost_net but dt is disabled. > > > > > > > > > > > > Fixing this by build the iova tree during IOMMU translation, this > > > > > > makes sure the UNMAP notifier event could be identified during page > > > > > > walk. And we need to walk page table not only for UNMAP notifier but > > > > > > for MAP notifier during PSI. > > > > > > > > > > > > Signed-off-by: Jason Wang <jasowang@redhat.com> > > > > > > --- > > > > > > hw/i386/intel_iommu.c | 43 ++++++++++++++++++------------------------- > > > > > > 1 file changed, 18 insertions(+), 25 deletions(-) > > > > > > > > > > > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > > > > > > index d025ef2873..edeb62f4b2 100644 > > > > > > --- a/hw/i386/intel_iommu.c > > > > > > +++ b/hw/i386/intel_iommu.c > > > > > > @@ -1834,6 +1834,8 @@ static bool vtd_do_iommu_translate(VTDAddressSpace *vtd_as, PCIBus *bus, > > > > > > uint8_t access_flags; > > > > > > bool rid2pasid = (pasid == PCI_NO_PASID) && s->root_scalable; > > > > > > VTDIOTLBEntry *iotlb_entry; > > > > > > + const DMAMap *mapped; > > > > > > + DMAMap target; > > > > > > > > > > > > /* > > > > > > * We have standalone memory region for interrupt addresses, we > > > > > > @@ -1954,6 +1956,21 @@ out: > > > > > > entry->translated_addr = vtd_get_slpte_addr(slpte, s->aw_bits) & page_mask; > > > > > > entry->addr_mask = ~page_mask; > > > > > > entry->perm = access_flags; > > > > > > + > > > > > > + target.iova = entry->iova; > > > > > > + target.size = entry->addr_mask; > > > > > > + target.translated_addr = entry->translated_addr; > > > > > > + target.perm = entry->perm; > > > > > > + > > > > > > + mapped = iova_tree_find(vtd_as->iova_tree, &target); > > > > > > + if (!mapped) { > > > > > > + /* To make UNMAP notifier work, we need build iova tree here > > > > > > + * in order to have the UNMAP iommu notifier to be triggered > > > > > > + * during the page walk. > > > > > > + */ > > > > > > + iova_tree_insert(vtd_as->iova_tree, &target); > > > > > > + } > > > > > > + > > > > > > return true; > > > > > > > > > > > > error: > > > > > > @@ -2161,31 +2178,7 @@ static void vtd_iotlb_page_invalidate_notify(IntelIOMMUState *s, > > > > > > ret = vtd_dev_to_context_entry(s, pci_bus_num(vtd_as->bus), > > > > > > vtd_as->devfn, &ce); > > > > > > if (!ret && domain_id == vtd_get_domain_id(s, &ce, vtd_as->pasid)) { > > > > > > - if (vtd_as_has_map_notifier(vtd_as)) { > > > > > > - /* > > > > > > - * As long as we have MAP notifications registered in > > > > > > - * any of our IOMMU notifiers, we need to sync the > > > > > > - * shadow page table. > > > > > > - */ > > > > > > - vtd_sync_shadow_page_table_range(vtd_as, &ce, addr, size); > > > > > > - } else { > > > > > > - /* > > > > > > - * For UNMAP-only notifiers, we don't need to walk the > > > > > > - * page tables. We just deliver the PSI down to > > > > > > - * invalidate caches. > > > > > > - */ > > > > > > - IOMMUTLBEvent event = { > > > > > > - .type = IOMMU_NOTIFIER_UNMAP, > > > > > > - .entry = { > > > > > > - .target_as = &address_space_memory, > > > > > > - .iova = addr, > > > > > > - .translated_addr = 0, > > > > > > - .addr_mask = size - 1, > > > > > > - .perm = IOMMU_NONE, > > > > > > - }, > > > > > > - }; > > > > > > - memory_region_notify_iommu(&vtd_as->iommu, 0, event); > > [1] > > > > > > > > > > > Isn't this path the one that will be responsible for pass-through the UNMAP > > > > > events from guest to vhost when there's no MAP notifier requested? > > > > > > > > Yes, but it doesn't do the iova tree removing. More below. > > > > > > > > > > > > > > At least that's what I expected when introducing the iova tree, because for > > > > > unmap-only device hierachy I thought we didn't need the tree at all. > > > > > > > > Then the problem is the UNMAP notifier won't be trigger at all during > > > > DSI page walk in vtd_page_walk_one() because there's no DMAMap stored > > > > in the iova tree.: > > > > > > > > if (!mapped) { > > > > /* Skip since we didn't map this range at all */ > > > > trace_vtd_page_walk_one_skip_unmap(entry->iova, entry->addr_mask); > > > > return 0; > > > > } > > > > > > > > So I choose to build the iova tree in translate then we won't go > > > > within the above condition. > > > > > > That's also why it's weird because IIUC we should never walk a page table > > > at all if there's no MAP notifier regiestered. > > > > If this is true, we probably need to document this somewhere. > > Agree. I'll post a patch. > > > > > > > > > When I'm looking at the walk callers I found that indeed there's one path > > > missing where can cause it to actually walk the pgtables without !MAP, then > > > I also noticed commit f7701e2c7983b6, and I'm wondering what we really want > > > is something like this: > > > > > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > > > index a08ee85edf..c46f3db992 100644 > > > --- a/hw/i386/intel_iommu.c > > > +++ b/hw/i386/intel_iommu.c > > > @@ -1536,7 +1536,7 @@ static int vtd_sync_shadow_page_table(VTDAddressSpace *vtd_as) > > > VTDContextEntry ce; > > > IOMMUNotifier *n; > > > > > > - if (!(vtd_as->iommu.iommu_notify_flags & IOMMU_NOTIFIER_IOTLB_EVENTS)) { > > > + if (!vtd_as_has_map_notifier(vtd_as)) { > > > return 0; > > > } > > > > > > So I'm not sure whether this patch is the problem resolver; so far I feel > > > like it's patch 2 who does the real fix. Then we can have the above > > > oneliner so we stop any walks when there's no map notifiers. > > > > > > Thanks, > > > > I may miss something but as state above, the problem is a missing > > UNMAP notification during DSI when there's only UNMAP notifier. > > I got confused too on why we didn't notify UNMAP for DSI already, that's so > weird because I thought it should be there or it should be broken for a > long time.. as we discussed multiple times around this one: > > /* > * Fallback to domain selective flush if no PSI support or > * the size is too big. > */ > if (!cap_pgsel_inv(iommu->cap) || > mask > cap_max_amask_val(iommu->cap)) > iommu->flush.flush_iotlb(iommu, did, 0, 0, > DMA_TLB_DSI_FLUSH); > else > iommu->flush.flush_iotlb(iommu, did, addr | ih, mask, > DMA_TLB_PSI_FLUSH); > > I guess we were just always lucky?.. Probably, or the reason is that we a notifier with UNMAP only is not commonly used before until patch 2. > > > > > To solve it we might have two ways: > > > > 1) build the iova tree during iommu translation then we can correctly > > trigger UNMAP during page walk caused by DSI > > 2) don't do the iova tree walk for !MAP notifier, need new logic to > > trigger UNMAP notifier in PSI/DSI > > > > This patch choose to go 1) (which seems easier at least for -stable). > > Do you mean you prefer to go with 2)? > > Yes. > > IOVA tree is unnecessary overhead IMHO because UNMAP (both IOTLB or > DEVIOTLB unmap) shouldn't need that complexity at all. Using the iova tree > can be accurate on which page got unmapped when the kernel driver used DSI > for a large PSI as shown above, however IMHO it needs more justification > that the pgtable walk is worth the effort. Not to mention if a device in > QEMU that wants to use the iova tree for some reason, one can just register > with MAP and ignore all MAP events, while by default we keep UNMAP simple. > > So we could do: > > (1) Rename vtd_sync_shadow_page_table() to vtd_sync_domain() > (2) instead of optimizing dev-iotlb only there: > > if (!(vtd_as->iommu.iommu_notify_flags & IOMMU_NOTIFIER_IOTLB_EVENTS)) { > return 0; > } > > we should firstly check if UNMAP or DEVUNMAP registered, we directly > send a notification to the whole domain. We need to choose the event > that the register happens with but not both. > > This also reminded me that whether we should sanity check on iommu > notifiers on some invalid cases. E.g. it seems to me when registered with > DEVIOTLB_UNMAP it should not register with either MAP or UNMAP anymore or > it doesn't make sense. It seems it doesn't' harm to allow both UNMAP and DEVIOTLB_UNMAP work. > > One step further, I'm wondering whether the DEV_IOTLB event should exist at > all. Maybe we want to have DEV_IOTLB typed iommu notifier only, This seems cleaner ( I remember we had some discussion before ). > but then > when we got dev-iotlb PSI/DSI we notify with type=UNMAP just like normal > UNMAP events, then in above (2) we can send UNMAP constantly as long as > !MAP. But even if so that'll just be another optional change on top. I'm fine to go without iova-tree. Would you mind to post patches for fix? I can test and include it in this series then. Thanks > > Thanks, > > -- > Peter Xu > ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 3/3] intel-iommu: build iova tree during IOMMU translation 2022-12-05 4:12 ` Jason Wang @ 2022-12-05 23:18 ` Peter Xu 2022-12-06 3:18 ` Jason Wang 0 siblings, 1 reply; 35+ messages in thread From: Peter Xu @ 2022-12-05 23:18 UTC (permalink / raw) To: Jason Wang; +Cc: mst, qemu-devel, eric.auger, viktor [-- Attachment #1: Type: text/plain, Size: 562 bytes --] Jason, On Mon, Dec 05, 2022 at 12:12:04PM +0800, Jason Wang wrote: > I'm fine to go without iova-tree. Would you mind to post patches for > fix? I can test and include it in this series then. One sample patch attached, only compile tested. I can also work on this but I'll be slow in making progress, so I'll add it into my todo. If you can help to fix this issue it'll be more than great. No worry on the ownership or authorship of the patch if you agree on the change and moving forward with this when modifying - just take it over! Thanks! -- Peter Xu [-- Attachment #2: 0001-memory-sanity-check-flatview-deref-on-mr-transaction.patch --] [-- Type: text/plain, Size: 1572 bytes --] From 57e5cab805c94d56f801a7e21098389a77584e34 Mon Sep 17 00:00:00 2001 From: Peter Xu <peterx@redhat.com> Date: Mon, 5 Dec 2022 11:14:02 -0500 Subject: [PATCH] memory: sanity check flatview deref on mr transactions Content-type: text/plain Signed-off-by: Peter Xu <peterx@redhat.com> --- include/exec/memory.h | 9 +++++++++ softmmu/memory.c | 2 +- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/include/exec/memory.h b/include/exec/memory.h index 91f8a2395a..e136ab9558 100644 --- a/include/exec/memory.h +++ b/include/exec/memory.h @@ -1069,8 +1069,17 @@ struct FlatView { MemoryRegion *root; }; +extern unsigned memory_region_transaction_depth; + static inline FlatView *address_space_to_flatview(AddressSpace *as) { + /* + * Before using any flatview, sanity check we're not during a memory + * region transaction or the map can be invalid. Note that this can + * also be called during commit phase of memory transaction, but that + * should also only happen when the depth decreases to 0 first. + */ + assert(memory_region_transaction_depth == 0); return qatomic_rcu_read(&as->current_map); } diff --git a/softmmu/memory.c b/softmmu/memory.c index bc0be3f62c..7cfcf5dffe 100644 --- a/softmmu/memory.c +++ b/softmmu/memory.c @@ -37,7 +37,7 @@ //#define DEBUG_UNASSIGNED -static unsigned memory_region_transaction_depth; +unsigned memory_region_transaction_depth; static bool memory_region_update_pending; static bool ioeventfd_update_pending; unsigned int global_dirty_tracking; -- 2.37.3 ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH 3/3] intel-iommu: build iova tree during IOMMU translation 2022-12-05 23:18 ` Peter Xu @ 2022-12-06 3:18 ` Jason Wang 2022-12-06 13:58 ` Peter Xu 0 siblings, 1 reply; 35+ messages in thread From: Jason Wang @ 2022-12-06 3:18 UTC (permalink / raw) To: Peter Xu; +Cc: mst, qemu-devel, eric.auger, viktor On Tue, Dec 6, 2022 at 7:19 AM Peter Xu <peterx@redhat.com> wrote: > > Jason, > > On Mon, Dec 05, 2022 at 12:12:04PM +0800, Jason Wang wrote: > > I'm fine to go without iova-tree. Would you mind to post patches for > > fix? I can test and include it in this series then. > > One sample patch attached, only compile tested. I don't see any direct connection between the attached patch and the intel-iommu? > > I can also work on this but I'll be slow in making progress, so I'll add it > into my todo. If you can help to fix this issue it'll be more than great. Ok, let me try but it might take some time :) > No worry on the ownership or authorship of the patch if you agree on the > change and moving forward with this when modifying - just take it over! Ok. Thanks > > Thanks! > > -- > Peter Xu ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 3/3] intel-iommu: build iova tree during IOMMU translation 2022-12-06 3:18 ` Jason Wang @ 2022-12-06 13:58 ` Peter Xu 2022-12-23 8:02 ` Jason Wang 0 siblings, 1 reply; 35+ messages in thread From: Peter Xu @ 2022-12-06 13:58 UTC (permalink / raw) To: Jason Wang; +Cc: mst, qemu-devel, eric.auger, viktor [-- Attachment #1: Type: text/plain, Size: 878 bytes --] On Tue, Dec 06, 2022 at 11:18:03AM +0800, Jason Wang wrote: > On Tue, Dec 6, 2022 at 7:19 AM Peter Xu <peterx@redhat.com> wrote: > > > > Jason, > > > > On Mon, Dec 05, 2022 at 12:12:04PM +0800, Jason Wang wrote: > > > I'm fine to go without iova-tree. Would you mind to post patches for > > > fix? I can test and include it in this series then. > > > > One sample patch attached, only compile tested. > > I don't see any direct connection between the attached patch and the > intel-iommu? Sorry! Wrong tree dumped... Trying again. > > > > > I can also work on this but I'll be slow in making progress, so I'll add it > > into my todo. If you can help to fix this issue it'll be more than great. > > Ok, let me try but it might take some time :) Sure. :) I'll also add it into my todo (and I think the other similar one has been there for a while.. :( ). -- Peter Xu [-- Attachment #2: 0001-intel-iommu-Send-unmap-notifications-for-domain-or-g.patch --] [-- Type: text/plain, Size: 3728 bytes --] From 37c743761d20c16891856c5bef2e7b3fb89893b6 Mon Sep 17 00:00:00 2001 From: Peter Xu <peterx@redhat.com> Date: Mon, 5 Dec 2022 18:11:36 -0500 Subject: [PATCH] intel-iommu: Send unmap notifications for domain or global inv desc Content-type: text/plain Signed-off-by: Peter Xu <peterx@redhat.com> --- hw/i386/intel_iommu.c | 41 +++++++++++++++++++++++++---------------- 1 file changed, 25 insertions(+), 16 deletions(-) diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index a08ee85edf..2c6ca68df0 100644 --- a/hw/i386/intel_iommu.c +++ b/hw/i386/intel_iommu.c @@ -206,6 +206,23 @@ static inline gboolean vtd_as_has_map_notifier(VTDAddressSpace *as) return as->notifier_flags & IOMMU_NOTIFIER_MAP; } +static void vtd_as_notify_unmap(VTDAddressSpace *as, hwaddr iova, + hwaddr addr_mask) +{ + IOMMUTLBEvent event = { + .type = IOMMU_NOTIFIER_UNMAP, + .entry = { + .target_as = &address_space_memory, + .iova = iova, + .translated_addr = 0, + .addr_mask = addr_mask, + .perm = IOMMU_NONE, + }, + }; + + memory_region_notify_iommu(&as->iommu, 0, event); +} + /* GHashTable functions */ static gboolean vtd_iotlb_equal(gconstpointer v1, gconstpointer v2) { @@ -1530,13 +1547,15 @@ static int vtd_sync_shadow_page_table_range(VTDAddressSpace *vtd_as, return vtd_page_walk(s, ce, addr, addr + size, &info, vtd_as->pasid); } -static int vtd_sync_shadow_page_table(VTDAddressSpace *vtd_as) +static int vtd_address_space_sync(VTDAddressSpace *vtd_as) { int ret; VTDContextEntry ce; IOMMUNotifier *n; - if (!(vtd_as->iommu.iommu_notify_flags & IOMMU_NOTIFIER_IOTLB_EVENTS)) { + /* If no MAP notifier registered, we simply invalidate all the cache */ + if (!vtd_as_has_map_notifier(vtd_as)) { + vtd_as_notify_unmap(vtd_as, 0, HWADDR_MAX); return 0; } @@ -2000,7 +2019,7 @@ static void vtd_iommu_replay_all(IntelIOMMUState *s) VTDAddressSpace *vtd_as; QLIST_FOREACH(vtd_as, &s->vtd_as_with_notifiers, next) { - vtd_sync_shadow_page_table(vtd_as); + vtd_address_space_sync(vtd_as); } } @@ -2082,7 +2101,7 @@ static void vtd_context_device_invalidate(IntelIOMMUState *s, * framework will skip MAP notifications if that * happened. */ - vtd_sync_shadow_page_table(vtd_as); + vtd_address_space_sync(vtd_as); } } } @@ -2140,7 +2159,7 @@ static void vtd_iotlb_domain_invalidate(IntelIOMMUState *s, uint16_t domain_id) if (!vtd_dev_to_context_entry(s, pci_bus_num(vtd_as->bus), vtd_as->devfn, &ce) && domain_id == vtd_get_domain_id(s, &ce, vtd_as->pasid)) { - vtd_sync_shadow_page_table(vtd_as); + vtd_address_space_sync(vtd_as); } } } @@ -2174,17 +2193,7 @@ static void vtd_iotlb_page_invalidate_notify(IntelIOMMUState *s, * page tables. We just deliver the PSI down to * invalidate caches. */ - IOMMUTLBEvent event = { - .type = IOMMU_NOTIFIER_UNMAP, - .entry = { - .target_as = &address_space_memory, - .iova = addr, - .translated_addr = 0, - .addr_mask = size - 1, - .perm = IOMMU_NONE, - }, - }; - memory_region_notify_iommu(&vtd_as->iommu, 0, event); + vtd_as_notify_unmap(vtd_as, addr, size - 1); } } } -- 2.37.3 ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH 3/3] intel-iommu: build iova tree during IOMMU translation 2022-12-06 13:58 ` Peter Xu @ 2022-12-23 8:02 ` Jason Wang 2022-12-23 16:22 ` Peter Xu 0 siblings, 1 reply; 35+ messages in thread From: Jason Wang @ 2022-12-23 8:02 UTC (permalink / raw) To: Peter Xu; +Cc: mst, qemu-devel, eric.auger, viktor On Tue, Dec 6, 2022 at 9:58 PM Peter Xu <peterx@redhat.com> wrote: > > On Tue, Dec 06, 2022 at 11:18:03AM +0800, Jason Wang wrote: > > On Tue, Dec 6, 2022 at 7:19 AM Peter Xu <peterx@redhat.com> wrote: > > > > > > Jason, > > > > > > On Mon, Dec 05, 2022 at 12:12:04PM +0800, Jason Wang wrote: > > > > I'm fine to go without iova-tree. Would you mind to post patches for > > > > fix? I can test and include it in this series then. > > > > > > One sample patch attached, only compile tested. > > > > I don't see any direct connection between the attached patch and the > > intel-iommu? > > Sorry! Wrong tree dumped... Trying again. The HWADDR breaks memory_region_notify_iommu_one(): qemu-system-x86_64: ../softmmu/memory.c:1991: memory_region_notify_iommu_one: Assertion `entry->iova >= notifier->start && entry_end <= notifier->end' failed. I wonder if we need either: 1) remove the assert or 2) introduce a new memory_region_notify_unmap_all() to unmap from notifier->start to notifier->end. Thanks > > > > > > > > > I can also work on this but I'll be slow in making progress, so I'll add it > > > into my todo. If you can help to fix this issue it'll be more than great. > > > > Ok, let me try but it might take some time :) > > Sure. :) > > I'll also add it into my todo (and I think the other similar one has been > there for a while.. :( ). > > -- > Peter Xu ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 3/3] intel-iommu: build iova tree during IOMMU translation 2022-12-23 8:02 ` Jason Wang @ 2022-12-23 16:22 ` Peter Xu 0 siblings, 0 replies; 35+ messages in thread From: Peter Xu @ 2022-12-23 16:22 UTC (permalink / raw) To: Jason Wang; +Cc: mst, qemu-devel, eric.auger, viktor On Fri, Dec 23, 2022 at 04:02:29PM +0800, Jason Wang wrote: > On Tue, Dec 6, 2022 at 9:58 PM Peter Xu <peterx@redhat.com> wrote: > > > > On Tue, Dec 06, 2022 at 11:18:03AM +0800, Jason Wang wrote: > > > On Tue, Dec 6, 2022 at 7:19 AM Peter Xu <peterx@redhat.com> wrote: > > > > > > > > Jason, > > > > > > > > On Mon, Dec 05, 2022 at 12:12:04PM +0800, Jason Wang wrote: > > > > > I'm fine to go without iova-tree. Would you mind to post patches for > > > > > fix? I can test and include it in this series then. > > > > > > > > One sample patch attached, only compile tested. > > > > > > I don't see any direct connection between the attached patch and the > > > intel-iommu? > > > > Sorry! Wrong tree dumped... Trying again. > > The HWADDR breaks memory_region_notify_iommu_one(): > > qemu-system-x86_64: ../softmmu/memory.c:1991: > memory_region_notify_iommu_one: Assertion `entry->iova >= > notifier->start && entry_end <= notifier->end' failed. > > I wonder if we need either: > > 1) remove the assert I vote for this one. Not only removing the assertion, we should probably crop the range too just like dev-iotlb unmaps? Thanks, > > or > > 2) introduce a new memory_region_notify_unmap_all() to unmap from > notifier->start to notifier->end. -- Peter Xu ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/3] Fix UNMAP notifier for intel-iommu 2022-11-29 8:10 [PATCH 0/3] Fix UNMAP notifier for intel-iommu Jason Wang ` (2 preceding siblings ...) 2022-11-29 8:10 ` [PATCH 3/3] intel-iommu: build iova tree during IOMMU translation Jason Wang @ 2022-11-30 16:37 ` Michael S. Tsirkin 2022-12-01 8:29 ` Jason Wang 2022-12-20 13:53 ` Michael S. Tsirkin 2023-01-15 23:30 ` Viktor Prutyanov 5 siblings, 1 reply; 35+ messages in thread From: Michael S. Tsirkin @ 2022-11-30 16:37 UTC (permalink / raw) To: Jason Wang; +Cc: peterx, qemu-devel, eric.auger, viktor On Tue, Nov 29, 2022 at 04:10:34PM +0800, Jason Wang wrote: > Hi All: > > According to ATS, device should work if ATS is disabled. This is not > correctly implemented in the current intel-iommu since it doesn't > handle the UNMAP notifier correctly. This breaks the vhost-net + > vIOMMU without dt. > > The root casue is that the when there's a device IOTLB miss (note that > it's not specific to PCI so it can work without ATS), Qemu doesn't > build the IOVA tree, so when guest start an IOTLB invalidation, Qemu > won't trigger the UNMAP notifier. > > Fixing by build IOVA tree during IOMMU translsation. > > Thanks Any changes of Fixes tags? this is 8.0 yes? > Jason Wang (3): > intel-iommu: fail MAP notifier without caching mode > intel-iommu: fail DEVIOTLB_UNMAP without dt mode > intel-iommu: build iova tree during IOMMU translation > > hw/i386/intel_iommu.c | 58 ++++++++++++++++++++++++------------------- > 1 file changed, 33 insertions(+), 25 deletions(-) > > -- > 2.25.1 ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/3] Fix UNMAP notifier for intel-iommu 2022-11-30 16:37 ` [PATCH 0/3] Fix UNMAP notifier for intel-iommu Michael S. Tsirkin @ 2022-12-01 8:29 ` Jason Wang 0 siblings, 0 replies; 35+ messages in thread From: Jason Wang @ 2022-12-01 8:29 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: peterx, qemu-devel, eric.auger, viktor On Thu, Dec 1, 2022 at 12:38 AM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Tue, Nov 29, 2022 at 04:10:34PM +0800, Jason Wang wrote: > > Hi All: > > > > According to ATS, device should work if ATS is disabled. This is not > > correctly implemented in the current intel-iommu since it doesn't > > handle the UNMAP notifier correctly. This breaks the vhost-net + > > vIOMMU without dt. > > > > The root casue is that the when there's a device IOTLB miss (note that > > it's not specific to PCI so it can work without ATS), Qemu doesn't > > build the IOVA tree, so when guest start an IOTLB invalidation, Qemu > > won't trigger the UNMAP notifier. > > > > Fixing by build IOVA tree during IOMMU translsation. > > > > Thanks > > Any changes of Fixes tags? this is 8.0 yes? Yes, it's for 8.0. Thanks > > > Jason Wang (3): > > intel-iommu: fail MAP notifier without caching mode > > intel-iommu: fail DEVIOTLB_UNMAP without dt mode > > intel-iommu: build iova tree during IOMMU translation > > > > hw/i386/intel_iommu.c | 58 ++++++++++++++++++++++++------------------- > > 1 file changed, 33 insertions(+), 25 deletions(-) > > > > -- > > 2.25.1 > ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/3] Fix UNMAP notifier for intel-iommu 2022-11-29 8:10 [PATCH 0/3] Fix UNMAP notifier for intel-iommu Jason Wang ` (3 preceding siblings ...) 2022-11-30 16:37 ` [PATCH 0/3] Fix UNMAP notifier for intel-iommu Michael S. Tsirkin @ 2022-12-20 13:53 ` Michael S. Tsirkin 2022-12-21 3:17 ` Jason Wang 2023-01-15 23:30 ` Viktor Prutyanov 5 siblings, 1 reply; 35+ messages in thread From: Michael S. Tsirkin @ 2022-12-20 13:53 UTC (permalink / raw) To: Jason Wang; +Cc: peterx, qemu-devel, eric.auger, viktor On Tue, Nov 29, 2022 at 04:10:34PM +0800, Jason Wang wrote: > Hi All: > > According to ATS, device should work if ATS is disabled. This is not > correctly implemented in the current intel-iommu since it doesn't > handle the UNMAP notifier correctly. This breaks the vhost-net + > vIOMMU without dt. > > The root casue is that the when there's a device IOTLB miss (note that > it's not specific to PCI so it can work without ATS), Qemu doesn't > build the IOVA tree, so when guest start an IOTLB invalidation, Qemu > won't trigger the UNMAP notifier. > > Fixing by build IOVA tree during IOMMU translsation. > > Thanks IIUC you were going to post v2? At least commit log fixes. > Jason Wang (3): > intel-iommu: fail MAP notifier without caching mode > intel-iommu: fail DEVIOTLB_UNMAP without dt mode > intel-iommu: build iova tree during IOMMU translation > > hw/i386/intel_iommu.c | 58 ++++++++++++++++++++++++------------------- > 1 file changed, 33 insertions(+), 25 deletions(-) > > -- > 2.25.1 ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/3] Fix UNMAP notifier for intel-iommu 2022-12-20 13:53 ` Michael S. Tsirkin @ 2022-12-21 3:17 ` Jason Wang 0 siblings, 0 replies; 35+ messages in thread From: Jason Wang @ 2022-12-21 3:17 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: peterx, qemu-devel, eric.auger, viktor On Tue, Dec 20, 2022 at 9:53 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Tue, Nov 29, 2022 at 04:10:34PM +0800, Jason Wang wrote: > > Hi All: > > > > According to ATS, device should work if ATS is disabled. This is not > > correctly implemented in the current intel-iommu since it doesn't > > handle the UNMAP notifier correctly. This breaks the vhost-net + > > vIOMMU without dt. > > > > The root casue is that the when there's a device IOTLB miss (note that > > it's not specific to PCI so it can work without ATS), Qemu doesn't > > build the IOVA tree, so when guest start an IOTLB invalidation, Qemu > > won't trigger the UNMAP notifier. > > > > Fixing by build IOVA tree during IOMMU translsation. > > > > Thanks > > > IIUC you were going to post v2? At least commit log fixes. Yes. Thanks > > > Jason Wang (3): > > intel-iommu: fail MAP notifier without caching mode > > intel-iommu: fail DEVIOTLB_UNMAP without dt mode > > intel-iommu: build iova tree during IOMMU translation > > > > hw/i386/intel_iommu.c | 58 ++++++++++++++++++++++++------------------- > > 1 file changed, 33 insertions(+), 25 deletions(-) > > > > -- > > 2.25.1 > ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/3] Fix UNMAP notifier for intel-iommu 2022-11-29 8:10 [PATCH 0/3] Fix UNMAP notifier for intel-iommu Jason Wang ` (4 preceding siblings ...) 2022-12-20 13:53 ` Michael S. Tsirkin @ 2023-01-15 23:30 ` Viktor Prutyanov 2023-01-16 7:06 ` Jason Wang 5 siblings, 1 reply; 35+ messages in thread From: Viktor Prutyanov @ 2023-01-15 23:30 UTC (permalink / raw) To: Jason Wang; +Cc: mst, peterx, qemu-devel, eric.auger, Yan Vugenfirer On Tue, Nov 29, 2022 at 11:10 AM Jason Wang <jasowang@redhat.com> wrote: > > Hi All: > > According to ATS, device should work if ATS is disabled. This is not > correctly implemented in the current intel-iommu since it doesn't > handle the UNMAP notifier correctly. This breaks the vhost-net + > vIOMMU without dt. > > The root casue is that the when there's a device IOTLB miss (note that > it's not specific to PCI so it can work without ATS), Qemu doesn't > build the IOVA tree, so when guest start an IOTLB invalidation, Qemu > won't trigger the UNMAP notifier. > > Fixing by build IOVA tree during IOMMU translsation. > > Thanks > > Jason Wang (3): > intel-iommu: fail MAP notifier without caching mode > intel-iommu: fail DEVIOTLB_UNMAP without dt mode > intel-iommu: build iova tree during IOMMU translation > > hw/i386/intel_iommu.c | 58 ++++++++++++++++++++++++------------------- > 1 file changed, 33 insertions(+), 25 deletions(-) > > -- > 2.25.1 > Hi Jason, I've tried the series with Windows Server 2022 guest with vhost and intel-iommu (device-iotlb=off) and now networking on this system has become working. So, as we discussed, I'm waiting for the series to be accepted in some form to continue my work about supporting guests who refuse Device-TLB on systems with device-iotlb=on. Tested-by: Viktor Prutyanov <viktor@daynix.com> Best regards, Viktor Prutyanov ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/3] Fix UNMAP notifier for intel-iommu 2023-01-15 23:30 ` Viktor Prutyanov @ 2023-01-16 7:06 ` Jason Wang 2023-01-27 13:17 ` Michael S. Tsirkin 0 siblings, 1 reply; 35+ messages in thread From: Jason Wang @ 2023-01-16 7:06 UTC (permalink / raw) To: Viktor Prutyanov; +Cc: mst, peterx, qemu-devel, eric.auger, Yan Vugenfirer On Mon, Jan 16, 2023 at 7:30 AM Viktor Prutyanov <viktor@daynix.com> wrote: > > On Tue, Nov 29, 2022 at 11:10 AM Jason Wang <jasowang@redhat.com> wrote: > > > > Hi All: > > > > According to ATS, device should work if ATS is disabled. This is not > > correctly implemented in the current intel-iommu since it doesn't > > handle the UNMAP notifier correctly. This breaks the vhost-net + > > vIOMMU without dt. > > > > The root casue is that the when there's a device IOTLB miss (note that > > it's not specific to PCI so it can work without ATS), Qemu doesn't > > build the IOVA tree, so when guest start an IOTLB invalidation, Qemu > > won't trigger the UNMAP notifier. > > > > Fixing by build IOVA tree during IOMMU translsation. > > > > Thanks > > > > Jason Wang (3): > > intel-iommu: fail MAP notifier without caching mode > > intel-iommu: fail DEVIOTLB_UNMAP without dt mode > > intel-iommu: build iova tree during IOMMU translation > > > > hw/i386/intel_iommu.c | 58 ++++++++++++++++++++++++------------------- > > 1 file changed, 33 insertions(+), 25 deletions(-) > > > > -- > > 2.25.1 > > > > Hi Jason, > > I've tried the series with Windows Server 2022 guest with vhost and > intel-iommu (device-iotlb=off) and now networking on this system has > become working. > So, as we discussed, I'm waiting for the series to be accepted in some > form to continue my work about supporting guests who refuse Device-TLB > on systems with device-iotlb=on. > > Tested-by: Viktor Prutyanov <viktor@daynix.com> Great, Peter has some comments on this series, so I will probably send a new version (probably after the chinese new year). Thanks > > Best regards, > Viktor Prutyanov > ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/3] Fix UNMAP notifier for intel-iommu 2023-01-16 7:06 ` Jason Wang @ 2023-01-27 13:17 ` Michael S. Tsirkin 2023-01-29 5:43 ` Jason Wang 0 siblings, 1 reply; 35+ messages in thread From: Michael S. Tsirkin @ 2023-01-27 13:17 UTC (permalink / raw) To: Jason Wang Cc: Viktor Prutyanov, peterx, qemu-devel, eric.auger, Yan Vugenfirer On Mon, Jan 16, 2023 at 03:06:44PM +0800, Jason Wang wrote: > On Mon, Jan 16, 2023 at 7:30 AM Viktor Prutyanov <viktor@daynix.com> wrote: > > > > On Tue, Nov 29, 2022 at 11:10 AM Jason Wang <jasowang@redhat.com> wrote: > > > > > > Hi All: > > > > > > According to ATS, device should work if ATS is disabled. This is not > > > correctly implemented in the current intel-iommu since it doesn't > > > handle the UNMAP notifier correctly. This breaks the vhost-net + > > > vIOMMU without dt. > > > > > > The root casue is that the when there's a device IOTLB miss (note that > > > it's not specific to PCI so it can work without ATS), Qemu doesn't > > > build the IOVA tree, so when guest start an IOTLB invalidation, Qemu > > > won't trigger the UNMAP notifier. > > > > > > Fixing by build IOVA tree during IOMMU translsation. > > > > > > Thanks > > > > > > Jason Wang (3): > > > intel-iommu: fail MAP notifier without caching mode > > > intel-iommu: fail DEVIOTLB_UNMAP without dt mode > > > intel-iommu: build iova tree during IOMMU translation > > > > > > hw/i386/intel_iommu.c | 58 ++++++++++++++++++++++++------------------- > > > 1 file changed, 33 insertions(+), 25 deletions(-) > > > > > > -- > > > 2.25.1 > > > > > > > Hi Jason, > > > > I've tried the series with Windows Server 2022 guest with vhost and > > intel-iommu (device-iotlb=off) and now networking on this system has > > become working. > > So, as we discussed, I'm waiting for the series to be accepted in some > > form to continue my work about supporting guests who refuse Device-TLB > > on systems with device-iotlb=on. > > > > Tested-by: Viktor Prutyanov <viktor@daynix.com> > > Great, Peter has some comments on this series, so I will probably send > a new version (probably after the chinese new year). > > Thanks Were you going to post a new version? > > > > Best regards, > > Viktor Prutyanov > > ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/3] Fix UNMAP notifier for intel-iommu 2023-01-27 13:17 ` Michael S. Tsirkin @ 2023-01-29 5:43 ` Jason Wang 0 siblings, 0 replies; 35+ messages in thread From: Jason Wang @ 2023-01-29 5:43 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Viktor Prutyanov, peterx, qemu-devel, eric.auger, Yan Vugenfirer On Fri, Jan 27, 2023 at 9:17 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Mon, Jan 16, 2023 at 03:06:44PM +0800, Jason Wang wrote: > > On Mon, Jan 16, 2023 at 7:30 AM Viktor Prutyanov <viktor@daynix.com> wrote: > > > > > > On Tue, Nov 29, 2022 at 11:10 AM Jason Wang <jasowang@redhat.com> wrote: > > > > > > > > Hi All: > > > > > > > > According to ATS, device should work if ATS is disabled. This is not > > > > correctly implemented in the current intel-iommu since it doesn't > > > > handle the UNMAP notifier correctly. This breaks the vhost-net + > > > > vIOMMU without dt. > > > > > > > > The root casue is that the when there's a device IOTLB miss (note that > > > > it's not specific to PCI so it can work without ATS), Qemu doesn't > > > > build the IOVA tree, so when guest start an IOTLB invalidation, Qemu > > > > won't trigger the UNMAP notifier. > > > > > > > > Fixing by build IOVA tree during IOMMU translsation. > > > > > > > > Thanks > > > > > > > > Jason Wang (3): > > > > intel-iommu: fail MAP notifier without caching mode > > > > intel-iommu: fail DEVIOTLB_UNMAP without dt mode > > > > intel-iommu: build iova tree during IOMMU translation > > > > > > > > hw/i386/intel_iommu.c | 58 ++++++++++++++++++++++++------------------- > > > > 1 file changed, 33 insertions(+), 25 deletions(-) > > > > > > > > -- > > > > 2.25.1 > > > > > > > > > > Hi Jason, > > > > > > I've tried the series with Windows Server 2022 guest with vhost and > > > intel-iommu (device-iotlb=off) and now networking on this system has > > > become working. > > > So, as we discussed, I'm waiting for the series to be accepted in some > > > form to continue my work about supporting guests who refuse Device-TLB > > > on systems with device-iotlb=on. > > > > > > Tested-by: Viktor Prutyanov <viktor@daynix.com> > > > > Great, Peter has some comments on this series, so I will probably send > > a new version (probably after the chinese new year). > > > > Thanks > > Were you going to post a new version? Yes. Thanks > > > > > > > Best regards, > > > Viktor Prutyanov > > > > ^ permalink raw reply [flat|nested] 35+ messages in thread
end of thread, other threads:[~2023-02-23 3:20 UTC | newest] Thread overview: 35+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-11-29 8:10 [PATCH 0/3] Fix UNMAP notifier for intel-iommu Jason Wang 2022-11-29 8:10 ` [PATCH 1/3] intel-iommu: fail MAP notifier without caching mode Jason Wang 2022-11-29 15:35 ` Peter Xu 2022-11-30 6:23 ` Jason Wang 2022-11-30 15:06 ` Peter Xu 2022-12-01 8:46 ` Jason Wang 2022-12-06 13:23 ` Eric Auger 2022-11-29 8:10 ` [PATCH 2/3] intel-iommu: fail DEVIOTLB_UNMAP without dt mode Jason Wang 2022-11-29 15:38 ` Peter Xu 2022-12-01 16:03 ` Peter Xu 2023-02-23 3:19 ` Jason Wang 2022-12-06 13:33 ` Eric Auger 2023-02-03 9:08 ` Laurent Vivier 2023-02-07 16:17 ` Laurent Vivier 2023-02-07 16:35 ` Peter Xu 2022-11-29 8:10 ` [PATCH 3/3] intel-iommu: build iova tree during IOMMU translation Jason Wang 2022-11-29 15:57 ` Peter Xu 2022-11-30 6:33 ` Jason Wang 2022-11-30 15:17 ` Peter Xu 2022-12-01 8:35 ` Jason Wang 2022-12-01 14:58 ` Peter Xu 2022-12-05 4:12 ` Jason Wang 2022-12-05 23:18 ` Peter Xu 2022-12-06 3:18 ` Jason Wang 2022-12-06 13:58 ` Peter Xu 2022-12-23 8:02 ` Jason Wang 2022-12-23 16:22 ` Peter Xu 2022-11-30 16:37 ` [PATCH 0/3] Fix UNMAP notifier for intel-iommu Michael S. Tsirkin 2022-12-01 8:29 ` Jason Wang 2022-12-20 13:53 ` Michael S. Tsirkin 2022-12-21 3:17 ` Jason Wang 2023-01-15 23:30 ` Viktor Prutyanov 2023-01-16 7:06 ` Jason Wang 2023-01-27 13:17 ` Michael S. Tsirkin 2023-01-29 5:43 ` Jason Wang
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.