Re: [PATCH v5 4/7] iommu/arm-smmu-v3: Pre-allocate a per-master invalidation array
From: Nicolin Chen <hidden>
Date: 2025-11-25 07:43:24
Also in:
linux-iommu, lkml
On Mon, Nov 24, 2025 at 03:31:17PM -0800, Nicolin Chen wrote:
On Mon, Nov 24, 2025 at 07:08:45PM -0400, Jason Gunthorpe wrote:quoted
On Mon, Nov 24, 2025 at 02:43:58PM -0800, Nicolin Chen wrote:quoted
On Mon, Nov 24, 2025 at 09:42:55PM +0000, Will Deacon wrote:quoted
On Sat, Nov 08, 2025 at 12:08:05AM -0800, Nicolin Chen wrote:quoted
+ /* Put the ids into order for sorted to_merge/to_unref arrays */ + sort_nonatomic(fwspec->ids, fwspec->num_ids, + sizeof(fwspec->ids[0]), arm_smmu_ids_cmp, NULL); + /* ATS case adds num_ids of entries, on top of the base case */ + master->build_invs = arm_smmu_invs_alloc(2 + fwspec->num_ids);Although I can't point at a specific issue here, I'm nervous about mutating the 'fwspec->ids' array from within the driver, The array isn't allocated or populated directly by the driver and so I don't think we really have any business sorting it. Could we hack iommu_fwspec_add_ids() to keep the array ordered instead?Yea, I think it makes sense to do it in the core, once we have the data structure provided by the core as well.I would be more worried about sorting it everywhere for every driver. I feel confident SMMUv3 doesn't use it, but something really old and embedded focused like tegra or omap, IDK. So I wouldn't propose to change iommu_fwspec_add_ids().Perhaps a different helper iommu_fwspec_add_ids_sorted()? Drivers can choose to use the sorted version, when they want to implement the invalidation array. And SMMU can be the first only caller.quoted
If you want to be conservative then the thing to do is sort the master->streams that arm_smmu_insert_master() copies the fwspec into. It just has to be sorted prior to feeding it into the rbtree. Then consistently use master->streams as the sorted list.How about kmemdup() an local id array to bridge betwen fwspec->ids and rbtree?
Does this look okay?
@@ -3713,6 +3713,7 @@ static int arm_smmu_insert_master(struct arm_smmu_device *smmu, { int i; int ret = 0; + u32 *ids, *ids_sorted = NULL; struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(master->dev); bool ats_supported = dev_is_pci(master->dev) && pci_ats_supported(to_pci_dev(master->dev));
@@ -3723,26 +3724,38 @@ static int arm_smmu_insert_master(struct arm_smmu_device *smmu, return -ENOMEM; master->num_streams = fwspec->num_ids; + ids = fwspec->ids; if (!ats_supported) { /* Base case has 1 ASID entry or maximum 2 VMID entries */ master->build_invs = arm_smmu_invs_alloc(2); } else { /* Put the ids into order for sorted to_merge/to_unref arrays */ - sort_nonatomic(fwspec->ids, fwspec->num_ids, - sizeof(fwspec->ids[0]), arm_smmu_ids_cmp, NULL); + if (fwspec->num_ids > 1) { + ids = kmemdup_array(fwspec->ids, fwspec->num_ids, + sizeof(*ids), GFP_KERNEL); + if (!ids) { + kfree(master->streams); + return -ENOMEM; + } + + sort_nonatomic(ids, fwspec->num_ids, sizeof(*ids), + arm_smmu_ids_cmp, NULL); + ids_sorted = ids; + } /* ATS case adds num_ids of entries, on top of the base case */ master->build_invs = arm_smmu_invs_alloc(2 + fwspec->num_ids); } - if (IS_ERR(master->build_invs)) { + if (!master->build_invs) { kfree(master->streams); - return PTR_ERR(master->build_invs); + ret = -ENOMEM; + goto out; } mutex_lock(&smmu->streams_mutex); for (i = 0; i < fwspec->num_ids; i++) { struct arm_smmu_stream *new_stream = &master->streams[i]; struct rb_node *existing; - u32 sid = fwspec->ids[i]; + u32 sid = ids[i]; new_stream->id = sid; new_stream->master = master;
@@ -3779,6 +3792,8 @@ static int arm_smmu_insert_master(struct arm_smmu_device *smmu, } mutex_unlock(&smmu->streams_mutex); +out: + kfree(ids_sorted); return ret; }
Thanks Nicolin