When a master is attached from an old domain to a new domain, it needs to
build an invalidation array to delete and add the array entries from/onto
the invalidation arrays of those two domains, passed via the to_merge and
to_unref arguments into arm_smmu_invs_merge/unref() respectively.
Since the master->num_streams might differ across masters, a memory would
have to be allocated when building an to_merge/to_unref array which might
fail with -ENOMEM.
On the other hand, an attachment to arm_smmu_blocked_domain must not fail
so it's the best to avoid any memory allocation in that path.
Pre-allocate a fixed size invalidation array for every master. This array
will be used as a scratch to fill dynamically when building a to_merge or
to_unref invs array. Sort fwspec->ids in an ascending order to fit to the
arm_smmu_invs_merge() function.
Co-developed-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 8 ++++++
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 27 +++++++++++++++++++++
2 files changed, 35 insertions(+)
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 757158b9ea655..7b81a82c0dfe4 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -922,6 +922,14 @@ struct arm_smmu_master {
struct arm_smmu_device *smmu;
struct device *dev;
struct arm_smmu_stream *streams;
+ /*
+ * Scratch memory for a to_merge or to_unref array to build a per-domain
+ * invalidation array. It'll be pre-allocated with enough enries for all
+ * possible build scenarios. It can be used by only one caller at a time
+ * until the arm_smmu_invs_merge/unref() finishes. Must be locked by the
+ * iommu_group mutex.
+ */
+ struct arm_smmu_invs *build_invs;
struct arm_smmu_vmaster *vmaster; /* use smmu->streams_mutex */
/* Locked by the iommu core using the group mutex */
struct arm_smmu_ctx_desc_cfg cd_table;
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 8266d0839a927..26b8492a13f20 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -3693,12 +3693,22 @@ static int arm_smmu_init_sid_strtab(struct arm_smmu_device *smmu, u32 sid)
return 0;
}
+static int arm_smmu_ids_cmp(const void *_l, const void *_r)
+{
+ const typeof_member(struct iommu_fwspec, ids[0]) *l = _l;
+ const typeof_member(struct iommu_fwspec, ids[0]) *r = _r;
+
+ return cmp_int(*l, *r);
+}
+
static int arm_smmu_insert_master(struct arm_smmu_device *smmu,
struct arm_smmu_master *master)
{
int i;
int ret = 0;
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));
master->streams = kcalloc(fwspec->num_ids, sizeof(*master->streams),
GFP_KERNEL);
@@ -3706,6 +3716,21 @@ static int arm_smmu_insert_master(struct arm_smmu_device *smmu,
return -ENOMEM;
master->num_streams = fwspec->num_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);
+ /* 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)) {
+ kfree(master->streams);
+ return PTR_ERR(master->build_invs);
+ }
+
mutex_lock(&smmu->streams_mutex);
for (i = 0; i < fwspec->num_ids; i++) {
struct arm_smmu_stream *new_stream = &master->streams[i];
@@ -3743,6 +3768,7 @@ static int arm_smmu_insert_master(struct arm_smmu_device *smmu,
for (i--; i >= 0; i--)
rb_erase(&master->streams[i].node, &smmu->streams);
kfree(master->streams);
+ kfree(master->build_invs);
}
mutex_unlock(&smmu->streams_mutex);
@@ -3764,6 +3790,7 @@ static void arm_smmu_remove_master(struct arm_smmu_master *master)
mutex_unlock(&smmu->streams_mutex);
kfree(master->streams);
+ kfree(master->build_invs);
}
static struct iommu_device *arm_smmu_probe_device(struct device *dev)
--
2.43.0On Sat, Nov 08, 2025 at 12:08:05AM -0800, Nicolin Chen wrote:
> When a master is attached from an old domain to a new domain, it needs to
> build an invalidation array to delete and add the array entries from/onto
> the invalidation arrays of those two domains, passed via the to_merge and
> to_unref arguments into arm_smmu_invs_merge/unref() respectively.
>
> Since the master->num_streams might differ across masters, a memory would
> have to be allocated when building an to_merge/to_unref array which might
> fail with -ENOMEM.
>
> On the other hand, an attachment to arm_smmu_blocked_domain must not fail
> so it's the best to avoid any memory allocation in that path.
>
> Pre-allocate a fixed size invalidation array for every master. This array
> will be used as a scratch to fill dynamically when building a to_merge or
> to_unref invs array. Sort fwspec->ids in an ascending order to fit to the
> arm_smmu_invs_merge() function.
>
> Co-developed-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> ---
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 8 ++++++
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 27 +++++++++++++++++++++
> 2 files changed, 35 insertions(+)
>
> 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 757158b9ea655..7b81a82c0dfe4 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> @@ -922,6 +922,14 @@ struct arm_smmu_master {
> struct arm_smmu_device *smmu;
> struct device *dev;
> struct arm_smmu_stream *streams;
> + /*
> + * Scratch memory for a to_merge or to_unref array to build a per-domain
> + * invalidation array. It'll be pre-allocated with enough enries for all
> + * possible build scenarios. It can be used by only one caller at a time
> + * until the arm_smmu_invs_merge/unref() finishes. Must be locked by the
> + * iommu_group mutex.
> + */
> + struct arm_smmu_invs *build_invs;
> struct arm_smmu_vmaster *vmaster; /* use smmu->streams_mutex */
> /* Locked by the iommu core using the group mutex */
> struct arm_smmu_ctx_desc_cfg cd_table;
> 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 8266d0839a927..26b8492a13f20 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -3693,12 +3693,22 @@ static int arm_smmu_init_sid_strtab(struct arm_smmu_device *smmu, u32 sid)
> return 0;
> }
>
> +static int arm_smmu_ids_cmp(const void *_l, const void *_r)
> +{
> + const typeof_member(struct iommu_fwspec, ids[0]) *l = _l;
> + const typeof_member(struct iommu_fwspec, ids[0]) *r = _r;
> +
> + return cmp_int(*l, *r);
> +}
> +
> static int arm_smmu_insert_master(struct arm_smmu_device *smmu,
> struct arm_smmu_master *master)
> {
> int i;
> int ret = 0;
> 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));
>
> master->streams = kcalloc(fwspec->num_ids, sizeof(*master->streams),
> GFP_KERNEL);
> @@ -3706,6 +3716,21 @@ static int arm_smmu_insert_master(struct arm_smmu_device *smmu,
> return -ENOMEM;
> master->num_streams = fwspec->num_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);
> + /* 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?
Will
On Mon, Nov 24, 2025 at 09:42:55PM +0000, Will Deacon wrote: > On Sat, Nov 08, 2025 at 12:08:05AM -0800, Nicolin Chen wrote: > > + /* 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. Thanks Nicolin
On Mon, Nov 24, 2025 at 02:43:58PM -0800, Nicolin Chen wrote: > On Mon, Nov 24, 2025 at 09:42:55PM +0000, Will Deacon wrote: > > On Sat, Nov 08, 2025 at 12:08:05AM -0800, Nicolin Chen wrote: > > > + /* 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(). 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. Jason
On Mon, Nov 24, 2025 at 07:08:45PM -0400, Jason Gunthorpe wrote: > On Mon, Nov 24, 2025 at 02:43:58PM -0800, Nicolin Chen wrote: > > On Mon, Nov 24, 2025 at 09:42:55PM +0000, Will Deacon wrote: > > > On Sat, Nov 08, 2025 at 12:08:05AM -0800, Nicolin Chen wrote: > > > > + /* 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. > 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? Thanks Nicolin
On Mon, Nov 24, 2025 at 03:31:15PM -0800, Nicolin Chen wrote:
> > 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?
Not needed, just like this:
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -3454,15 +3454,21 @@ static int arm_smmu_insert_master(struct arm_smmu_device *smmu,
if (!master->streams)
return -ENOMEM;
master->num_streams = fwspec->num_ids;
-
- 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];
- new_stream->id = sid;
+ new_stream->id = fwspec->ids[i];
new_stream->master = master;
+ }
+
+ sort_nonatomic(master->streams, master->num_streams,
+ sizeof(master->streams[0]), arm_smmu_ids_cmp, NULL);
+
+ mutex_lock(&smmu->streams_mutex);
+ for (i = 0; i < master->num_streams; i++) {
+ struct arm_smmu_stream *new_stream = &master->streams[i];
+ struct rb_node *existing;
+ u32 sid = new_stream->id;
ret = arm_smmu_init_sid_strtab(smmu, sid);
if (ret)
cmp would work on a struct arm_smmu_stream.
Jason
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:
> > On Mon, Nov 24, 2025 at 02:43:58PM -0800, Nicolin Chen wrote:
> > > On Mon, Nov 24, 2025 at 09:42:55PM +0000, Will Deacon wrote:
> > > > On Sat, Nov 08, 2025 at 12:08:05AM -0800, Nicolin Chen wrote:
> > > > > + /* 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.
>
> > 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
© 2016 - 2025 Red Hat, Inc.