drivers/accel/amdxdna/aie2_pci.c | 13 +- drivers/dma/idxd/init.c | 43 +-- drivers/iommu/amd/iommu.c | 34 -- .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 86 +---- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 130 ++++---- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 34 +- drivers/iommu/intel/iommu.c | 301 ++++++------------ drivers/iommu/intel/iommu.h | 50 ++- drivers/iommu/intel/nested.c | 16 +- drivers/iommu/intel/pasid.c | 41 +-- drivers/iommu/intel/prq.c | 2 +- drivers/iommu/intel/svm.c | 52 ++- drivers/iommu/iommu-sva.c | 3 - drivers/iommu/iommu.c | 32 -- drivers/iommu/iommufd/device.c | 1 - drivers/iommu/iommufd/fault.c | 111 ++----- drivers/iommu/iommufd/iommufd_private.h | 3 - drivers/iommu/iommufd/selftest.c | 52 ++- drivers/misc/uacce/uacce.c | 40 --- include/linux/iommu.h | 35 -- 20 files changed, 380 insertions(+), 699 deletions(-)
The new method for driver fault reporting support relies on the domain to specify a iopf_handler. The driver should detect this and setup the HW when fault capable domains are attached. Move SMMUv3 to use this method and have VT-D validate support during attach so that all three fault capable drivers have a no-op FEAT_SVA and _IOPF. Then remove them. This was initiated by Jason. I'm following up to remove FEAT_IOPF and further clean up. The whole series is also available at github: https://github.com/LuBaolu/intel-iommu/commits/iommu_no_feat-v1 Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> Jason Gunthorpe (3): iommu/arm-smmu-v3: Put iopf enablement in the domain attach path iommu/vt-d: Check if SVA is supported when attaching the SVA domain iommu: Remove IOMMU_DEV_FEAT_SVA Lu Baolu (9): iommu/vt-d: Move scalable mode ATS enablement to probe path iommu/vt-d: Move PRI enablement in probe path iommu/vt-d: Cleanup intel_context_flush_present() iommu/vt-d: Put iopf enablement in domain attach path iommufd/selftest: Put iopf enablement in domain attach path dmaengine: idxd: Remove unnecessary IOMMU_DEV_FEAT_IOPF uacce: Remove unnecessary IOMMU_DEV_FEAT_IOPF iommufd: Remove unnecessary IOMMU_DEV_FEAT_IOPF iommu: Remove iommu_dev_enable/disable_feature() drivers/accel/amdxdna/aie2_pci.c | 13 +- drivers/dma/idxd/init.c | 43 +-- drivers/iommu/amd/iommu.c | 34 -- .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 86 +---- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 130 ++++---- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 34 +- drivers/iommu/intel/iommu.c | 301 ++++++------------ drivers/iommu/intel/iommu.h | 50 ++- drivers/iommu/intel/nested.c | 16 +- drivers/iommu/intel/pasid.c | 41 +-- drivers/iommu/intel/prq.c | 2 +- drivers/iommu/intel/svm.c | 52 ++- drivers/iommu/iommu-sva.c | 3 - drivers/iommu/iommu.c | 32 -- drivers/iommu/iommufd/device.c | 1 - drivers/iommu/iommufd/fault.c | 111 ++----- drivers/iommu/iommufd/iommufd_private.h | 3 - drivers/iommu/iommufd/selftest.c | 52 ++- drivers/misc/uacce/uacce.c | 40 --- include/linux/iommu.h | 35 -- 20 files changed, 380 insertions(+), 699 deletions(-) -- 2.43.0
Hi, Baolu On Fri, 14 Feb 2025 at 14:11, Lu Baolu <baolu.lu@linux.intel.com> wrote: > > The new method for driver fault reporting support relies on the domain > to specify a iopf_handler. The driver should detect this and setup the > HW when fault capable domains are attached. > > Move SMMUv3 to use this method and have VT-D validate support during > attach so that all three fault capable drivers have a no-op FEAT_SVA and > _IOPF. Then remove them. > > This was initiated by Jason. I'm following up to remove FEAT_IOPF and > further clean up. > > The whole series is also available at github: > https://github.com/LuBaolu/intel-iommu/commits/iommu_no_feat-v1 I quickly test this branch 1. host test is OK 2. qemu boot one device, test ok, though reports this when guest bootup. vfio-pci xxx: resetting vfio-pci xxx: reset done 3. qemu boot multi device, test fails, host kernel reports [Hardware Error] qemu can boot no problem Test fails. Will do more check without these patches. Thanks
On Fri, Feb 14, 2025 at 04:43:12PM +0800, Zhangfei Gao wrote: > 3. qemu boot multi device, test fails, host kernel reports > [Hardware Error] More details? Can you bisect? Thanks, Jason
Hi, Jason On Fri, 14 Feb 2025 at 20:56, Jason Gunthorpe <jgg@ziepe.ca> wrote: > > On Fri, Feb 14, 2025 at 04:43:12PM +0800, Zhangfei Gao wrote: > > 3. qemu boot multi device, test fails, host kernel reports > > [Hardware Error] > > More details? Can you bisect? It does not relate to multi devices, one device also happens when user page fault triggers. iopf_queue_remove_device is called. rcu_assign_pointer(param->fault_param, NULL); call trace [ 304.961312] Call trace: [ 304.961314] show_stack+0x20/0x38 (C) [ 304.961319] dump_stack_lvl+0xc0/0xd0 [ 304.961324] dump_stack+0x18/0x28 [ 304.961327] iopf_queue_remove_device+0xb0/0x1f0 [ 304.961331] arm_smmu_remove_master_domain+0x204/0x250 [ 304.961336] arm_smmu_attach_commit+0x64/0x100 [ 304.961338] arm_smmu_attach_dev_nested+0x104/0x1a8 [ 304.961340] __iommu_attach_device+0x2c/0x110 [ 304.961343] __iommu_device_set_domain.isra.0+0x78/0xe0 [ 304.961345] __iommu_group_set_domain_internal+0x78/0x160 [ 304.961347] iommu_replace_group_handle+0x9c/0x150 [ 304.961350] iommufd_fault_domain_replace_dev+0x88/0x120 [ 304.961353] iommufd_device_do_replace+0x190/0x3c0 [ 304.961355] iommufd_device_change_pt+0x270/0x688 [ 304.961357] iommufd_device_replace+0x20/0x38 [ 304.961359] vfio_iommufd_physical_attach_ioas+0x30/0x78 [ 304.961363] vfio_df_ioctl_attach_pt+0xa8/0x188 [ 304.961366] vfio_device_fops_unl_ioctl+0x310/0x990 When page fault triggers: [ 1016.383578] ------------[ cut here ]----------- [ 1016.388184] WARNING: CPU: 35 PID: 717 at drivers/iommu/io-pgfault.c:231 iommu_report_device_fault+0x2c8/0x470 [ 1016.398057] Modules linked in: [ 1016.401102] CPU: 35 UID: 0 PID: 717 Comm: irq/31-arm-smmu Not tainted 6.14.0-rc2-g4384d0f9bd24-dirty #19 [ 1016.410538] Hardware name: Huawei TaiShan 200 (Model 2280)/BC82AMDD, BIOS 2280-V2 CS V5.B133.01 03/25/2021 [ 1016.420147] pstate: a0400009 (NzCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) [ 1016.427077] pc : iommu_report_device_fault+0x2c8/0x470 [ 1016.432194] lr : iommu_report_device_fault+0x2c8/0x470 [ 1016.437309] sp : ffff80008c7fbb20 [ 1016.440609] x29: ffff80008c7fbb20 x28: ffff4f25c95aa6ac x27: ffffb5d5fd356818 [ 1016.447714] x26: ffffb5d600a0b8a0 x25: 0000000000000000 x24: ffff6f454371d0a0 [ 1016.454819] x23: ffffb5d5ff9544e0 x22: ffff4f4545796040 x21: ffff6f25466d80c8 [ 1016.461923] x20: ffff80008c7fbc88 x19: ffff6f25631a2780 x18: 000000000000ffff [ 1016.469028] x17: 0000000000000001 x16: ffffffffffffffff x15: 0000000000000001 [ 1016.476132] x14: 0000000000000001 x13: 0000000000000001 x12: ffff0001fffedf00 [ 1016.483236] x11: 0000000000000000 x10: 0000000000000c80 x9 : ffffb5d5fde985f0 [ 1016.490340] x8 : ffff4f4545796d20 x7 : 0000aaaada7af000 x6 : 0000000000000001 [ 1016.497444] x5 : ffffb5d600a0b000 x4 : ffffb5d600a0be70 x3 : 0000000000000000 [ 1016.504548] x2 : ffff4f4545796040 x1 : 0000000000000000 x0 : 0000000000000000 [ 1016.511652] Call trace: [ 1016.514088] iommu_report_device_fault+0x2c8/0x470 (P) [ 1016.519205] arm_smmu_handle_event+0x100/0x170 [ 1016.523633] arm_smmu_evtq_thread+0x1e4/0x4a0 [ 1016.527973] irq_thread_fn+0x34/0xb8 [ 1016.531534] irq_thread+0x160/0x310 [ 1016.535008] kthread+0x124/0x220 [ 1016.538225] ret_from_fork+0x10/0x20 [ 1016.541786] ---[ end trace 0000000000000000 ]--- [ 1016.546403] arm-smmu-v3 arm-smmu-v3.3.auto: event 0x10 received: [ 1016.552474] arm-smmu-v3 arm-smmu-v3.3.auto: 0x0000750100001810 [ 1016.558453] arm-smmu-v3 arm-smmu-v3.3.auto: 0x0000120080000176 [ 1016.564430] arm-smmu-v3 arm-smmu-v3.3.auto: 0x0000aaaada7af000 [ 1016.570406] arm-smmu-v3 arm-smmu-v3.3.auto: 0x0000aaaada7af000 [ 1016.576380] arm-smmu-v3 arm-smmu-v3.3.auto: event: F_TRANSLATION client: 0000:75:00.1 sid: 0x7501 ssid: 0x1 iova: 0xaaaada7af000 ipa: 0xaaaada7af000 [ 1016.589700] arm-smmu-v3 arm-smmu-v3.3.auto: unpriv data write s1 "Input address caused fault" stall stag: 0x176 Thanks
On 2/15/25 16:11, Zhangfei Gao wrote: > It does not relate to multi devices, one device also happens when user > page fault triggers. > > iopf_queue_remove_device is called. > rcu_assign_pointer(param->fault_param, NULL); > > call trace > [ 304.961312] Call trace: > [ 304.961314] show_stack+0x20/0x38 (C) > [ 304.961319] dump_stack_lvl+0xc0/0xd0 > [ 304.961324] dump_stack+0x18/0x28 > [ 304.961327] iopf_queue_remove_device+0xb0/0x1f0 > [ 304.961331] arm_smmu_remove_master_domain+0x204/0x250 > [ 304.961336] arm_smmu_attach_commit+0x64/0x100 > [ 304.961338] arm_smmu_attach_dev_nested+0x104/0x1a8 > [ 304.961340] __iommu_attach_device+0x2c/0x110 > [ 304.961343] __iommu_device_set_domain.isra.0+0x78/0xe0 > [ 304.961345] __iommu_group_set_domain_internal+0x78/0x160 > [ 304.961347] iommu_replace_group_handle+0x9c/0x150 > [ 304.961350] iommufd_fault_domain_replace_dev+0x88/0x120 > [ 304.961353] iommufd_device_do_replace+0x190/0x3c0 > [ 304.961355] iommufd_device_change_pt+0x270/0x688 > [ 304.961357] iommufd_device_replace+0x20/0x38 > [ 304.961359] vfio_iommufd_physical_attach_ioas+0x30/0x78 > [ 304.961363] vfio_df_ioctl_attach_pt+0xa8/0x188 > [ 304.961366] vfio_device_fops_unl_ioctl+0x310/0x990 > > > When page fault triggers: > > [ 1016.383578] ------------[ cut here ]----------- > [ 1016.388184] WARNING: CPU: 35 PID: 717 at > drivers/iommu/io-pgfault.c:231 iommu_report_device_fault+0x2c8/0x470 It's likely that iopf_queue_add_device() was not called for this device. Thanks, baolu
On Sat, 15 Feb 2025 at 18:09, Baolu Lu <baolu.lu@linux.intel.com> wrote:
>
> On 2/15/25 16:11, Zhangfei Gao wrote:
> > It does not relate to multi devices, one device also happens when user
> > page fault triggers.
> >
> > iopf_queue_remove_device is called.
> > rcu_assign_pointer(param->fault_param, NULL);
> >
> > call trace
> > [ 304.961312] Call trace:
> > [ 304.961314] show_stack+0x20/0x38 (C)
> > [ 304.961319] dump_stack_lvl+0xc0/0xd0
> > [ 304.961324] dump_stack+0x18/0x28
> > [ 304.961327] iopf_queue_remove_device+0xb0/0x1f0
> > [ 304.961331] arm_smmu_remove_master_domain+0x204/0x250
> > [ 304.961336] arm_smmu_attach_commit+0x64/0x100
> > [ 304.961338] arm_smmu_attach_dev_nested+0x104/0x1a8
> > [ 304.961340] __iommu_attach_device+0x2c/0x110
> > [ 304.961343] __iommu_device_set_domain.isra.0+0x78/0xe0
> > [ 304.961345] __iommu_group_set_domain_internal+0x78/0x160
> > [ 304.961347] iommu_replace_group_handle+0x9c/0x150
> > [ 304.961350] iommufd_fault_domain_replace_dev+0x88/0x120
> > [ 304.961353] iommufd_device_do_replace+0x190/0x3c0
> > [ 304.961355] iommufd_device_change_pt+0x270/0x688
> > [ 304.961357] iommufd_device_replace+0x20/0x38
> > [ 304.961359] vfio_iommufd_physical_attach_ioas+0x30/0x78
> > [ 304.961363] vfio_df_ioctl_attach_pt+0xa8/0x188
> > [ 304.961366] vfio_device_fops_unl_ioctl+0x310/0x990
> >
> >
> > When page fault triggers:
> >
> > [ 1016.383578] ------------[ cut here ]-----------
> > [ 1016.388184] WARNING: CPU: 35 PID: 717 at
> > drivers/iommu/io-pgfault.c:231 iommu_report_device_fault+0x2c8/0x470
>
> It's likely that iopf_queue_add_device() was not called for this device.
iopf_queue_add_device is called, but quickly iopf_queue_remove_device
is called during guest bootup.
Then fault_param is set to NULL.
arm_smmu_attach_commit
arm_smmu_remove_master_domain
// newly added in the first patch
if (master_domain) {
if (master_domain->using_iopf)
arm_smmu_disable_iopf(master); ->
iopf_queue_remove_device
kfree(master_domain);
}
As a comparison, without this patchset, only iopf_queue_add_device is
called, not call iopf_queue_remove_device
Thanks
On 2/15/25 19:35, Zhangfei Gao wrote:
> On Sat, 15 Feb 2025 at 18:09, Baolu Lu<baolu.lu@linux.intel.com> wrote:
>> On 2/15/25 16:11, Zhangfei Gao wrote:
>>> It does not relate to multi devices, one device also happens when user
>>> page fault triggers.
>>>
>>> iopf_queue_remove_device is called.
>>> rcu_assign_pointer(param->fault_param, NULL);
>>>
>>> call trace
>>> [ 304.961312] Call trace:
>>> [ 304.961314] show_stack+0x20/0x38 (C)
>>> [ 304.961319] dump_stack_lvl+0xc0/0xd0
>>> [ 304.961324] dump_stack+0x18/0x28
>>> [ 304.961327] iopf_queue_remove_device+0xb0/0x1f0
>>> [ 304.961331] arm_smmu_remove_master_domain+0x204/0x250
>>> [ 304.961336] arm_smmu_attach_commit+0x64/0x100
>>> [ 304.961338] arm_smmu_attach_dev_nested+0x104/0x1a8
>>> [ 304.961340] __iommu_attach_device+0x2c/0x110
>>> [ 304.961343] __iommu_device_set_domain.isra.0+0x78/0xe0
>>> [ 304.961345] __iommu_group_set_domain_internal+0x78/0x160
>>> [ 304.961347] iommu_replace_group_handle+0x9c/0x150
>>> [ 304.961350] iommufd_fault_domain_replace_dev+0x88/0x120
>>> [ 304.961353] iommufd_device_do_replace+0x190/0x3c0
>>> [ 304.961355] iommufd_device_change_pt+0x270/0x688
>>> [ 304.961357] iommufd_device_replace+0x20/0x38
>>> [ 304.961359] vfio_iommufd_physical_attach_ioas+0x30/0x78
>>> [ 304.961363] vfio_df_ioctl_attach_pt+0xa8/0x188
>>> [ 304.961366] vfio_device_fops_unl_ioctl+0x310/0x990
>>>
>>>
>>> When page fault triggers:
>>>
>>> [ 1016.383578] ------------[ cut here ]-----------
>>> [ 1016.388184] WARNING: CPU: 35 PID: 717 at
>>> drivers/iommu/io-pgfault.c:231 iommu_report_device_fault+0x2c8/0x470
>> It's likely that iopf_queue_add_device() was not called for this device.
> iopf_queue_add_device is called, but quickly iopf_queue_remove_device
> is called during guest bootup.
> Then fault_param is set to NULL.
>
> arm_smmu_attach_commit
> arm_smmu_remove_master_domain
> // newly added in the first patch
> if (master_domain) {
> if (master_domain->using_iopf)
It seems the above check is incorrect. We only need to disable iopf when
an iopf-capable domain is about to be removed. Will the following
additional change make any difference?
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 28e67a9e3861..9b9ef738d070 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2822,7 +2822,7 @@ static void arm_smmu_remove_master_domain(struct
arm_smmu_master *master,
spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
if (master_domain) {
- if (master_domain->using_iopf)
+ if (domain->iopf_handler)
arm_smmu_disable_iopf(master);
kfree(master_domain);
}
> arm_smmu_disable_iopf(master); ->
> iopf_queue_remove_device
> kfree(master_domain);
> }
>
> As a comparison, without this patchset, only iopf_queue_add_device is
> called, not call iopf_queue_remove_device
Thanks,
baolu
On Tue, Feb 18, 2025 at 10:57:30AM +0800, Baolu Lu wrote:
> > > > [ 304.961340] __iommu_attach_device+0x2c/0x110
> > > > [ 304.961343] __iommu_device_set_domain.isra.0+0x78/0xe0
> > > > [ 304.961345] __iommu_group_set_domain_internal+0x78/0x160
> > > > [ 304.961347] iommu_replace_group_handle+0x9c/0x150
> > > > [ 304.961350] iommufd_fault_domain_replace_dev+0x88/0x120
> > > > [ 304.961353] iommufd_device_do_replace+0x190/0x3c0
> > > > [ 304.961355] iommufd_device_change_pt+0x270/0x688
> > > > [ 304.961357] iommufd_device_replace+0x20/0x38
> > > > [ 304.961359] vfio_iommufd_physical_attach_ioas+0x30/0x78
> > > > [ 304.961363] vfio_df_ioctl_attach_pt+0xa8/0x188
> > > > [ 304.961366] vfio_device_fops_unl_ioctl+0x310/0x990
> > > >
> > > >
> > > > When page fault triggers:
> > > >
> > > > [ 1016.383578] ------------[ cut here ]-----------
> > > > [ 1016.388184] WARNING: CPU: 35 PID: 717 at
> > > > drivers/iommu/io-pgfault.c:231 iommu_report_device_fault+0x2c8/0x470
> > > It's likely that iopf_queue_add_device() was not called for this device.
> > iopf_queue_add_device is called, but quickly iopf_queue_remove_device
> > is called during guest bootup.
> > Then fault_param is set to NULL.
> >
> > arm_smmu_attach_commit
> > arm_smmu_remove_master_domain
> > // newly added in the first patch
> > if (master_domain) {
> > if (master_domain->using_iopf)
>
> It seems the above check is incorrect. We only need to disable iopf when
> an iopf-capable domain is about to be removed. Will the following
> additional change make any difference?
The check looks right, it should only disable if it was enabled? The
refcounting is what keep track of the 'about to be removed' and it
should be that using_iopf and domain->iopf_handler are mostly the
same.
Hmm, I think the issue is related to nested
to_smmu_domain_devices() returns the S2 parent for the nesting domain
always
Which means the smmu_domain->devices list (on the s2) will end up with
two entries for the same SID during the replace operation at VM boot,
one with faulting and one without.
I think that arm_smmu_remove_master_domain() will end up removing the
wrong master_domain because arm_smmu_find_master_domain() can't tell
the two apart.
When I wrote this there was no nested and the list devices list was
unique to each domain, so everything inside was the same.
Like below?
Jason
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index b14f1d0ee7076b..dc8708b414468e 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2710,10 +2710,9 @@ static void arm_smmu_disable_pasid(struct arm_smmu_master *master)
pci_disable_pasid(pdev);
}
-static struct arm_smmu_master_domain *
-arm_smmu_find_master_domain(struct arm_smmu_domain *smmu_domain,
- struct arm_smmu_master *master,
- ioasid_t ssid, bool nested_ats_flush)
+static struct arm_smmu_master_domain *arm_smmu_find_master_domain(
+ struct arm_smmu_domain *smmu_domain, struct iommu_domain *domain,
+ struct arm_smmu_master *master, ioasid_t ssid, bool nested_ats_flush)
{
struct arm_smmu_master_domain *master_domain;
@@ -2722,6 +2721,7 @@ arm_smmu_find_master_domain(struct arm_smmu_domain *smmu_domain,
list_for_each_entry(master_domain, &smmu_domain->devices,
devices_elm) {
if (master_domain->master == master &&
+ master_domain->domain == domain &&
master_domain->ssid == ssid &&
master_domain->nested_ats_flush == nested_ats_flush)
return master_domain;
@@ -2812,8 +2812,8 @@ static void arm_smmu_remove_master_domain(struct arm_smmu_master *master,
nested_ats_flush = to_smmu_nested_domain(domain)->enable_ats;
spin_lock_irqsave(&smmu_domain->devices_lock, flags);
- master_domain = arm_smmu_find_master_domain(smmu_domain, master, ssid,
- nested_ats_flush);
+ master_domain = arm_smmu_find_master_domain(smmu_domain, domain, master,
+ ssid, nested_ats_flush);
if (master_domain) {
list_del(&master_domain->devices_elm);
if (master->ats_enabled)
@@ -2889,6 +2889,7 @@ int arm_smmu_attach_prepare(struct arm_smmu_attach_state *state,
master_domain = kzalloc(sizeof(*master_domain), GFP_KERNEL);
if (!master_domain)
return -ENOMEM;
+ master_domain->domain = new_domain;
master_domain->master = master;
master_domain->ssid = state->ssid;
if (new_domain->type == IOMMU_DOMAIN_NESTED)
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
index 5653d7417db7d9..fe6b88affa4a60 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -907,6 +907,11 @@ void arm_smmu_make_sva_cd(struct arm_smmu_cd *target,
struct arm_smmu_master_domain {
struct list_head devices_elm;
struct arm_smmu_master *master;
+ /*
+ * For nested domains the master_domain is threaded onto the S2 parent,
+ * this points to the IOMMU_DOMAIN_NESTED to disambiguate the masters.
+ */
+ struct iommu_domain *domain;
ioasid_t ssid;
bool nested_ats_flush : 1;
bool using_iopf : 1;
Hi, Jason
On Tue, 18 Feb 2025 at 21:57, Jason Gunthorpe <jgg@ziepe.ca> wrote:
>
> On Tue, Feb 18, 2025 at 10:57:30AM +0800, Baolu Lu wrote:
> > > > > [ 304.961340] __iommu_attach_device+0x2c/0x110
> > > > > [ 304.961343] __iommu_device_set_domain.isra.0+0x78/0xe0
> > > > > [ 304.961345] __iommu_group_set_domain_internal+0x78/0x160
> > > > > [ 304.961347] iommu_replace_group_handle+0x9c/0x150
> > > > > [ 304.961350] iommufd_fault_domain_replace_dev+0x88/0x120
> > > > > [ 304.961353] iommufd_device_do_replace+0x190/0x3c0
> > > > > [ 304.961355] iommufd_device_change_pt+0x270/0x688
> > > > > [ 304.961357] iommufd_device_replace+0x20/0x38
> > > > > [ 304.961359] vfio_iommufd_physical_attach_ioas+0x30/0x78
> > > > > [ 304.961363] vfio_df_ioctl_attach_pt+0xa8/0x188
> > > > > [ 304.961366] vfio_device_fops_unl_ioctl+0x310/0x990
> > > > >
> > > > >
> > > > > When page fault triggers:
> > > > >
> > > > > [ 1016.383578] ------------[ cut here ]-----------
> > > > > [ 1016.388184] WARNING: CPU: 35 PID: 717 at
> > > > > drivers/iommu/io-pgfault.c:231 iommu_report_device_fault+0x2c8/0x470
> > > > It's likely that iopf_queue_add_device() was not called for this device.
> > > iopf_queue_add_device is called, but quickly iopf_queue_remove_device
> > > is called during guest bootup.
> > > Then fault_param is set to NULL.
> > >
> > > arm_smmu_attach_commit
> > > arm_smmu_remove_master_domain
> > > // newly added in the first patch
> > > if (master_domain) {
> > > if (master_domain->using_iopf)
> >
> > It seems the above check is incorrect. We only need to disable iopf when
> > an iopf-capable domain is about to be removed. Will the following
> > additional change make any difference?
>
> The check looks right, it should only disable if it was enabled? The
> refcounting is what keep track of the 'about to be removed' and it
> should be that using_iopf and domain->iopf_handler are mostly the
> same.
>
> Hmm, I think the issue is related to nested
>
> to_smmu_domain_devices() returns the S2 parent for the nesting domain
> always
>
> Which means the smmu_domain->devices list (on the s2) will end up with
> two entries for the same SID during the replace operation at VM boot,
> one with faulting and one without.
>
> I think that arm_smmu_remove_master_domain() will end up removing the
> wrong master_domain because arm_smmu_find_master_domain() can't tell
> the two apart.
>
> When I wrote this there was no nested and the list devices list was
> unique to each domain, so everything inside was the same.
>
> Like below?
>
> Jason
>
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index b14f1d0ee7076b..dc8708b414468e 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -2710,10 +2710,9 @@ static void arm_smmu_disable_pasid(struct arm_smmu_master *master)
> pci_disable_pasid(pdev);
> }
>
> -static struct arm_smmu_master_domain *
> -arm_smmu_find_master_domain(struct arm_smmu_domain *smmu_domain,
> - struct arm_smmu_master *master,
> - ioasid_t ssid, bool nested_ats_flush)
> +static struct arm_smmu_master_domain *arm_smmu_find_master_domain(
> + struct arm_smmu_domain *smmu_domain, struct iommu_domain *domain,
> + struct arm_smmu_master *master, ioasid_t ssid, bool nested_ats_flush)
> {
> struct arm_smmu_master_domain *master_domain;
>
> @@ -2722,6 +2721,7 @@ arm_smmu_find_master_domain(struct arm_smmu_domain *smmu_domain,
> list_for_each_entry(master_domain, &smmu_domain->devices,
> devices_elm) {
> if (master_domain->master == master &&
> + master_domain->domain == domain &&
> master_domain->ssid == ssid &&
> master_domain->nested_ats_flush == nested_ats_flush)
> return master_domain;
> @@ -2812,8 +2812,8 @@ static void arm_smmu_remove_master_domain(struct arm_smmu_master *master,
> nested_ats_flush = to_smmu_nested_domain(domain)->enable_ats;
>
> spin_lock_irqsave(&smmu_domain->devices_lock, flags);
> - master_domain = arm_smmu_find_master_domain(smmu_domain, master, ssid,
> - nested_ats_flush);
> + master_domain = arm_smmu_find_master_domain(smmu_domain, domain, master,
> + ssid, nested_ats_flush);
> if (master_domain) {
> list_del(&master_domain->devices_elm);
> if (master->ats_enabled)
> @@ -2889,6 +2889,7 @@ int arm_smmu_attach_prepare(struct arm_smmu_attach_state *state,
> master_domain = kzalloc(sizeof(*master_domain), GFP_KERNEL);
> if (!master_domain)
> return -ENOMEM;
> + master_domain->domain = new_domain;
> master_domain->master = master;
> master_domain->ssid = state->ssid;
> if (new_domain->type == IOMMU_DOMAIN_NESTED)
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> index 5653d7417db7d9..fe6b88affa4a60 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> @@ -907,6 +907,11 @@ void arm_smmu_make_sva_cd(struct arm_smmu_cd *target,
> struct arm_smmu_master_domain {
> struct list_head devices_elm;
> struct arm_smmu_master *master;
> + /*
> + * For nested domains the master_domain is threaded onto the S2 parent,
> + * this points to the IOMMU_DOMAIN_NESTED to disambiguate the masters.
> + */
> + struct iommu_domain *domain;
> ioasid_t ssid;
> bool nested_ats_flush : 1;
> bool using_iopf : 1;
I have tested it, and it solved the issue.
Thanks
On Tue, Feb 18, 2025 at 11:25:59PM +0800, Zhangfei Gao wrote: > I have tested it, and it solved the issue. Great, thanks, Baolu can you updated the patch? Thanks Jason
On 2025/2/19 0:53, Jason Gunthorpe wrote: > On Tue, Feb 18, 2025 at 11:25:59PM +0800, Zhangfei Gao wrote: > >> I have tested it, and it solved the issue. > Great, thanks, Baolu can you updated the patch? Yes, sure. Will be included in the next version. Thanks, baolu
On Tue, 18 Feb 2025 at 11:00, Baolu Lu <baolu.lu@linux.intel.com> wrote:
>
> On 2/15/25 19:35, Zhangfei Gao wrote:
> > On Sat, 15 Feb 2025 at 18:09, Baolu Lu<baolu.lu@linux.intel.com> wrote:
> >> On 2/15/25 16:11, Zhangfei Gao wrote:
> >>> It does not relate to multi devices, one device also happens when user
> >>> page fault triggers.
> >>>
> >>> iopf_queue_remove_device is called.
> >>> rcu_assign_pointer(param->fault_param, NULL);
> >>>
> >>> call trace
> >>> [ 304.961312] Call trace:
> >>> [ 304.961314] show_stack+0x20/0x38 (C)
> >>> [ 304.961319] dump_stack_lvl+0xc0/0xd0
> >>> [ 304.961324] dump_stack+0x18/0x28
> >>> [ 304.961327] iopf_queue_remove_device+0xb0/0x1f0
> >>> [ 304.961331] arm_smmu_remove_master_domain+0x204/0x250
> >>> [ 304.961336] arm_smmu_attach_commit+0x64/0x100
> >>> [ 304.961338] arm_smmu_attach_dev_nested+0x104/0x1a8
> >>> [ 304.961340] __iommu_attach_device+0x2c/0x110
> >>> [ 304.961343] __iommu_device_set_domain.isra.0+0x78/0xe0
> >>> [ 304.961345] __iommu_group_set_domain_internal+0x78/0x160
> >>> [ 304.961347] iommu_replace_group_handle+0x9c/0x150
> >>> [ 304.961350] iommufd_fault_domain_replace_dev+0x88/0x120
> >>> [ 304.961353] iommufd_device_do_replace+0x190/0x3c0
> >>> [ 304.961355] iommufd_device_change_pt+0x270/0x688
> >>> [ 304.961357] iommufd_device_replace+0x20/0x38
> >>> [ 304.961359] vfio_iommufd_physical_attach_ioas+0x30/0x78
> >>> [ 304.961363] vfio_df_ioctl_attach_pt+0xa8/0x188
> >>> [ 304.961366] vfio_device_fops_unl_ioctl+0x310/0x990
> >>>
> >>>
> >>> When page fault triggers:
> >>>
> >>> [ 1016.383578] ------------[ cut here ]-----------
> >>> [ 1016.388184] WARNING: CPU: 35 PID: 717 at
> >>> drivers/iommu/io-pgfault.c:231 iommu_report_device_fault+0x2c8/0x470
> >> It's likely that iopf_queue_add_device() was not called for this device.
> > iopf_queue_add_device is called, but quickly iopf_queue_remove_device
> > is called during guest bootup.
> > Then fault_param is set to NULL.
> >
> > arm_smmu_attach_commit
> > arm_smmu_remove_master_domain
> > // newly added in the first patch
> > if (master_domain) {
> > if (master_domain->using_iopf)
>
> It seems the above check is incorrect. We only need to disable iopf when
> an iopf-capable domain is about to be removed. Will the following
> additional change make any difference?
>
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index 28e67a9e3861..9b9ef738d070 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -2822,7 +2822,7 @@ static void arm_smmu_remove_master_domain(struct
> arm_smmu_master *master,
> spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
>
> if (master_domain) {
> - if (master_domain->using_iopf)
> + if (domain->iopf_handler)
> arm_smmu_disable_iopf(master);
> kfree(master_domain);
> }
Yes, good idea, using this can solve the issue.
Thanks
On Fri, 14 Feb 2025 at 16:43, Zhangfei Gao <zhangfei.gao@linaro.org> wrote: > > Hi, Baolu > > On Fri, 14 Feb 2025 at 14:11, Lu Baolu <baolu.lu@linux.intel.com> wrote: > > > > The new method for driver fault reporting support relies on the domain > > to specify a iopf_handler. The driver should detect this and setup the > > HW when fault capable domains are attached. > > > > Move SMMUv3 to use this method and have VT-D validate support during > > attach so that all three fault capable drivers have a no-op FEAT_SVA and > > _IOPF. Then remove them. > > > > This was initiated by Jason. I'm following up to remove FEAT_IOPF and > > further clean up. > > > > The whole series is also available at github: > > https://github.com/LuBaolu/intel-iommu/commits/iommu_no_feat-v1 > > I quickly test this branch > > 1. host test is OK > > 2. qemu boot one device, test ok, > though reports this when guest bootup. > vfio-pci xxx: resetting > vfio-pci xxx: reset done > > 3. qemu boot multi device, test fails, host kernel reports [Hardware Error] > qemu can boot no problem > Test fails. > > Will do more checks without these patches. Test on 6.14-rc2 without this patch set 1. qemu boot multi device, test OK 2. log "vfio-pci xxx: resetting/reset done" also exists. Thanks
© 2016 - 2025 Red Hat, Inc.