[PATCH] iommu/arm-smmu-v3: Allocate cmdq_batch on the heap

[PATCH] iommu/arm-smmu-v3: Allocate cmdq_batch on the heap

[Cheng-Yang Chou]

The arm_smmu_cmdq_batch structure is large and was being allocated on
the stack in four call sites, causing stack frame sizes to exceed the
1024-byte limit:

- arm_smmu_atc_inv_domain: 1120 bytes
- arm_smmu_atc_inv_master: 1088 bytes
- arm_smmu_sync_cd: 1088 bytes
- __arm_smmu_tlb_inv_range: 1072 bytes

Move these allocations to the heap using kmalloc_obj() and kfree() to
eliminate the -Wframe-larger-than=1024 warnings and prevent potential
stack overflows.

Signed-off-by: Cheng-Yang Chou <yphbchou0911@gmail.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 66 +++++++++++++++------
 1 file changed, 48 insertions(+), 18 deletions(-)

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 4d00d796f078..734546dc6a78 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -1281,7 +1281,7 @@ static void arm_smmu_sync_cd(struct arm_smmu_master *master,
 			     int ssid, bool leaf)
 {
 	size_t i;
-	struct arm_smmu_cmdq_batch cmds;
+	struct arm_smmu_cmdq_batch *cmds;
 	struct arm_smmu_device *smmu = master->smmu;
 	struct arm_smmu_cmdq_ent cmd = {
 		.opcode	= CMDQ_OP_CFGI_CD,
@@ -1291,13 +1291,23 @@ static void arm_smmu_sync_cd(struct arm_smmu_master *master,
 		},
 	};
 
-	arm_smmu_cmdq_batch_init(smmu, &cmds, &cmd);
+	cmds = kmalloc_obj(*cmds);
+	if (!cmds) {
+		struct arm_smmu_cmdq_ent cmd_all = { .opcode = CMDQ_OP_CFGI_ALL };
+
+		WARN_ONCE(1, "arm-smmu-v3: failed to allocate cmdq_batch, falling back to full CD invalidation\n");
+		arm_smmu_cmdq_issue_cmd_with_sync(smmu, &cmd_all);
+		return;
+	}
+
+	arm_smmu_cmdq_batch_init(smmu, cmds, &cmd);
 	for (i = 0; i < master->num_streams; i++) {
 		cmd.cfgi.sid = master->streams[i].id;
-		arm_smmu_cmdq_batch_add(smmu, &cmds, &cmd);
+		arm_smmu_cmdq_batch_add(smmu, cmds, &cmd);
 	}
 
-	arm_smmu_cmdq_batch_submit(smmu, &cmds);
+	arm_smmu_cmdq_batch_submit(smmu, cmds);
+	kfree(cmds);
 }
 
 static void arm_smmu_write_cd_l1_desc(struct arm_smmu_cdtab_l1 *dst,
@@ -2225,31 +2235,37 @@ arm_smmu_atc_inv_to_cmd(int ssid, unsigned long iova, size_t size,
 static int arm_smmu_atc_inv_master(struct arm_smmu_master *master,
 				   ioasid_t ssid)
 {
-	int i;
+	int i, ret;
 	struct arm_smmu_cmdq_ent cmd;
-	struct arm_smmu_cmdq_batch cmds;
+	struct arm_smmu_cmdq_batch *cmds;
 
 	arm_smmu_atc_inv_to_cmd(ssid, 0, 0, &cmd);
 
-	arm_smmu_cmdq_batch_init(master->smmu, &cmds, &cmd);
+	cmds = kmalloc_obj(*cmds);
+	if (!cmds)
+		return -ENOMEM;
+
+	arm_smmu_cmdq_batch_init(master->smmu, cmds, &cmd);
 	for (i = 0; i < master->num_streams; i++) {
 		cmd.atc.sid = master->streams[i].id;
-		arm_smmu_cmdq_batch_add(master->smmu, &cmds, &cmd);
+		arm_smmu_cmdq_batch_add(master->smmu, cmds, &cmd);
 	}
 
-	return arm_smmu_cmdq_batch_submit(master->smmu, &cmds);
+	ret = arm_smmu_cmdq_batch_submit(master->smmu, cmds);
+	kfree(cmds);
+	return ret;
 }
 
 int arm_smmu_atc_inv_domain(struct arm_smmu_domain *smmu_domain,
 			    unsigned long iova, size_t size)
 {
 	struct arm_smmu_master_domain *master_domain;
-	int i;
+	int i, ret;
 	unsigned long flags;
 	struct arm_smmu_cmdq_ent cmd = {
 		.opcode = CMDQ_OP_ATC_INV,
 	};
-	struct arm_smmu_cmdq_batch cmds;
+	struct arm_smmu_cmdq_batch *cmds;
 
 	if (!(smmu_domain->smmu->features & ARM_SMMU_FEAT_ATS))
 		return 0;
@@ -2271,7 +2287,11 @@ int arm_smmu_atc_inv_domain(struct arm_smmu_domain *smmu_domain,
 	if (!atomic_read(&smmu_domain->nr_ats_masters))
 		return 0;
 
-	arm_smmu_cmdq_batch_init(smmu_domain->smmu, &cmds, &cmd);
+	cmds = kmalloc_obj(*cmds);
+	if (!cmds)
+		return -ENOMEM;
+
+	arm_smmu_cmdq_batch_init(smmu_domain->smmu, cmds, &cmd);
 
 	spin_lock_irqsave(&smmu_domain->devices_lock, flags);
 	list_for_each_entry(master_domain, &smmu_domain->devices,
@@ -2294,12 +2314,14 @@ int arm_smmu_atc_inv_domain(struct arm_smmu_domain *smmu_domain,
 
 		for (i = 0; i < master->num_streams; i++) {
 			cmd.atc.sid = master->streams[i].id;
-			arm_smmu_cmdq_batch_add(smmu_domain->smmu, &cmds, &cmd);
+			arm_smmu_cmdq_batch_add(smmu_domain->smmu, cmds, &cmd);
 		}
 	}
 	spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
 
-	return arm_smmu_cmdq_batch_submit(smmu_domain->smmu, &cmds);
+	ret = arm_smmu_cmdq_batch_submit(smmu_domain->smmu, cmds);
+	kfree(cmds);
+	return ret;
 }
 
 /* IO_PGTABLE API */
@@ -2334,7 +2356,7 @@ static void __arm_smmu_tlb_inv_range(struct arm_smmu_cmdq_ent *cmd,
 	struct arm_smmu_device *smmu = smmu_domain->smmu;
 	unsigned long end = iova + size, num_pages = 0, tg = 0;
 	size_t inv_range = granule;
-	struct arm_smmu_cmdq_batch cmds;
+	struct arm_smmu_cmdq_batch *cmds;
 
 	if (!size)
 		return;
@@ -2362,7 +2384,14 @@ static void __arm_smmu_tlb_inv_range(struct arm_smmu_cmdq_ent *cmd,
 			num_pages++;
 	}
 
-	arm_smmu_cmdq_batch_init(smmu, &cmds, cmd);
+	cmds = kmalloc_obj(*cmds);
+	if (!cmds) {
+		WARN_ONCE(1, "arm-smmu-v3: failed to allocate cmdq_batch, falling back to full TLB invalidation\n");
+		arm_smmu_tlb_inv_context(smmu_domain);
+		return;
+	}
+
+	arm_smmu_cmdq_batch_init(smmu, cmds, cmd);
 
 	while (iova < end) {
 		if (smmu->features & ARM_SMMU_FEAT_RANGE_INV) {
@@ -2391,10 +2420,11 @@ static void __arm_smmu_tlb_inv_range(struct arm_smmu_cmdq_ent *cmd,
 		}
 
 		cmd->tlbi.addr = iova;
-		arm_smmu_cmdq_batch_add(smmu, &cmds, cmd);
+		arm_smmu_cmdq_batch_add(smmu, cmds, cmd);
 		iova += inv_range;
 	}
-	arm_smmu_cmdq_batch_submit(smmu, &cmds);
+	arm_smmu_cmdq_batch_submit(smmu, cmds);
+	kfree(cmds);
 }
 
 static void arm_smmu_tlb_inv_range_domain(unsigned long iova, size_t size,
-- 
2.48.1

Re: [PATCH] iommu/arm-smmu-v3: Allocate cmdq_batch on the heap

[Pranjal Shrivastava]

On Wed, Mar 11, 2026 at 05:44:44PM +0800, Cheng-Yang Chou wrote:
> The arm_smmu_cmdq_batch structure is large and was being allocated on
> the stack in four call sites, causing stack frame sizes to exceed the
> 1024-byte limit:
> 
> - arm_smmu_atc_inv_domain: 1120 bytes
> - arm_smmu_atc_inv_master: 1088 bytes
> - arm_smmu_sync_cd: 1088 bytes
> - __arm_smmu_tlb_inv_range: 1072 bytes
> 
> Move these allocations to the heap using kmalloc_obj() and kfree() to
> eliminate the -Wframe-larger-than=1024 warnings and prevent potential
> stack overflows.
> 

Thanks for the patch. I agree that we should address these warnings, but
moving these allocations to the heap via kmalloc_obj() in the fast path
is problematic. Introducing heap allocation adds unnecessary latency and
potential for allocation failure in hot paths.

So, yes, we are using a lot of stack but we're using it to do good
things.. 

IMO, if we really want to address these, instead of kmalloc, we could
potentially consider some pre-allocated per-CPU buffers (that's a lot of
additional book-keeping though) to keep the data off the stack or
something similar following a simple rule: The fast path must be 
deterministic- no SLAB allocations and no introducing new failure points

The last thing we'd want is a graphic driver's shrinker calling
dma-unmaps when the system is already under heavy memory pressure and 
calling kmalloc leading to a circular dependency or allocation failure
exactly when the system needs to perform the unmap the most.

Thanks,
Praan

> Signed-off-by: Cheng-Yang Chou <yphbchou0911@gmail.com>
> ---
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 66 +++++++++++++++------
>  1 file changed, 48 insertions(+), 18 deletions(-)
> 
> 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 4d00d796f078..734546dc6a78 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -1281,7 +1281,7 @@ static void arm_smmu_sync_cd(struct arm_smmu_master *master,
>  			     int ssid, bool leaf)
>  {
>  	size_t i;
> -	struct arm_smmu_cmdq_batch cmds;
> +	struct arm_smmu_cmdq_batch *cmds;
>  	struct arm_smmu_device *smmu = master->smmu;
>  	struct arm_smmu_cmdq_ent cmd = {
>  		.opcode	= CMDQ_OP_CFGI_CD,
> @@ -1291,13 +1291,23 @@ static void arm_smmu_sync_cd(struct arm_smmu_master *master,
>  		},
>  	};
>  
> -	arm_smmu_cmdq_batch_init(smmu, &cmds, &cmd);
> +	cmds = kmalloc_obj(*cmds);
> +	if (!cmds) {
> +		struct arm_smmu_cmdq_ent cmd_all = { .opcode = CMDQ_OP_CFGI_ALL };
> +
> +		WARN_ONCE(1, "arm-smmu-v3: failed to allocate cmdq_batch, falling back to full CD invalidation\n");
> +		arm_smmu_cmdq_issue_cmd_with_sync(smmu, &cmd_all);
> +		return;
> +	}
> +
> +	arm_smmu_cmdq_batch_init(smmu, cmds, &cmd);
>  	for (i = 0; i < master->num_streams; i++) {
>  		cmd.cfgi.sid = master->streams[i].id;
> -		arm_smmu_cmdq_batch_add(smmu, &cmds, &cmd);
> +		arm_smmu_cmdq_batch_add(smmu, cmds, &cmd);
>  	}
>  
> -	arm_smmu_cmdq_batch_submit(smmu, &cmds);
> +	arm_smmu_cmdq_batch_submit(smmu, cmds);
> +	kfree(cmds);
>  }
>  
>  static void arm_smmu_write_cd_l1_desc(struct arm_smmu_cdtab_l1 *dst,
> @@ -2225,31 +2235,37 @@ arm_smmu_atc_inv_to_cmd(int ssid, unsigned long iova, size_t size,
>  static int arm_smmu_atc_inv_master(struct arm_smmu_master *master,
>  				   ioasid_t ssid)
>  {
> -	int i;
> +	int i, ret;
>  	struct arm_smmu_cmdq_ent cmd;
> -	struct arm_smmu_cmdq_batch cmds;
> +	struct arm_smmu_cmdq_batch *cmds;
>  
>  	arm_smmu_atc_inv_to_cmd(ssid, 0, 0, &cmd);
>  
> -	arm_smmu_cmdq_batch_init(master->smmu, &cmds, &cmd);
> +	cmds = kmalloc_obj(*cmds);
> +	if (!cmds)
> +		return -ENOMEM;
> +
> +	arm_smmu_cmdq_batch_init(master->smmu, cmds, &cmd);
>  	for (i = 0; i < master->num_streams; i++) {
>  		cmd.atc.sid = master->streams[i].id;
> -		arm_smmu_cmdq_batch_add(master->smmu, &cmds, &cmd);
> +		arm_smmu_cmdq_batch_add(master->smmu, cmds, &cmd);
>  	}
>  
> -	return arm_smmu_cmdq_batch_submit(master->smmu, &cmds);
> +	ret = arm_smmu_cmdq_batch_submit(master->smmu, cmds);
> +	kfree(cmds);
> +	return ret;
>  }
>  
>  int arm_smmu_atc_inv_domain(struct arm_smmu_domain *smmu_domain,
>  			    unsigned long iova, size_t size)
>  {
>  	struct arm_smmu_master_domain *master_domain;
> -	int i;
> +	int i, ret;
>  	unsigned long flags;
>  	struct arm_smmu_cmdq_ent cmd = {
>  		.opcode = CMDQ_OP_ATC_INV,
>  	};
> -	struct arm_smmu_cmdq_batch cmds;
> +	struct arm_smmu_cmdq_batch *cmds;
>  
>  	if (!(smmu_domain->smmu->features & ARM_SMMU_FEAT_ATS))
>  		return 0;
> @@ -2271,7 +2287,11 @@ int arm_smmu_atc_inv_domain(struct arm_smmu_domain *smmu_domain,
>  	if (!atomic_read(&smmu_domain->nr_ats_masters))
>  		return 0;
>  
> -	arm_smmu_cmdq_batch_init(smmu_domain->smmu, &cmds, &cmd);
> +	cmds = kmalloc_obj(*cmds);
> +	if (!cmds)
> +		return -ENOMEM;
> +
> +	arm_smmu_cmdq_batch_init(smmu_domain->smmu, cmds, &cmd);
>  
>  	spin_lock_irqsave(&smmu_domain->devices_lock, flags);
>  	list_for_each_entry(master_domain, &smmu_domain->devices,
> @@ -2294,12 +2314,14 @@ int arm_smmu_atc_inv_domain(struct arm_smmu_domain *smmu_domain,
>  
>  		for (i = 0; i < master->num_streams; i++) {
>  			cmd.atc.sid = master->streams[i].id;
> -			arm_smmu_cmdq_batch_add(smmu_domain->smmu, &cmds, &cmd);
> +			arm_smmu_cmdq_batch_add(smmu_domain->smmu, cmds, &cmd);
>  		}
>  	}
>  	spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
>  
> -	return arm_smmu_cmdq_batch_submit(smmu_domain->smmu, &cmds);
> +	ret = arm_smmu_cmdq_batch_submit(smmu_domain->smmu, cmds);
> +	kfree(cmds);
> +	return ret;
>  }
>  
>  /* IO_PGTABLE API */
> @@ -2334,7 +2356,7 @@ static void __arm_smmu_tlb_inv_range(struct arm_smmu_cmdq_ent *cmd,
>  	struct arm_smmu_device *smmu = smmu_domain->smmu;
>  	unsigned long end = iova + size, num_pages = 0, tg = 0;
>  	size_t inv_range = granule;
> -	struct arm_smmu_cmdq_batch cmds;
> +	struct arm_smmu_cmdq_batch *cmds;
>  
>  	if (!size)
>  		return;
> @@ -2362,7 +2384,14 @@ static void __arm_smmu_tlb_inv_range(struct arm_smmu_cmdq_ent *cmd,
>  			num_pages++;
>  	}
>  
> -	arm_smmu_cmdq_batch_init(smmu, &cmds, cmd);
> +	cmds = kmalloc_obj(*cmds);
> +	if (!cmds) {
> +		WARN_ONCE(1, "arm-smmu-v3: failed to allocate cmdq_batch, falling back to full TLB invalidation\n");
> +		arm_smmu_tlb_inv_context(smmu_domain);
> +		return;
> +	}
> +
> +	arm_smmu_cmdq_batch_init(smmu, cmds, cmd);
>  
>  	while (iova < end) {
>  		if (smmu->features & ARM_SMMU_FEAT_RANGE_INV) {
> @@ -2391,10 +2420,11 @@ static void __arm_smmu_tlb_inv_range(struct arm_smmu_cmdq_ent *cmd,
>  		}
>  
>  		cmd->tlbi.addr = iova;
> -		arm_smmu_cmdq_batch_add(smmu, &cmds, cmd);
> +		arm_smmu_cmdq_batch_add(smmu, cmds, cmd);
>  		iova += inv_range;
>  	}
> -	arm_smmu_cmdq_batch_submit(smmu, &cmds);
> +	arm_smmu_cmdq_batch_submit(smmu, cmds);
> +	kfree(cmds);
>  }
>  
>  static void arm_smmu_tlb_inv_range_domain(unsigned long iova, size_t size,
> -- 
> 2.48.1
> 
> 

Re: [PATCH] iommu/arm-smmu-v3: Allocate cmdq_batch on the heap

[Cheng-Yang Chou]

On Wed, Mar 11, 2026 at 02:22:50PM +0000, Pranjal Shrivastava wrote:
> On Wed, Mar 11, 2026 at 05:44:44PM +0800, Cheng-Yang Chou wrote:
> > The arm_smmu_cmdq_batch structure is large and was being allocated on
> > the stack in four call sites, causing stack frame sizes to exceed the
> > 1024-byte limit:
> > 
> > - arm_smmu_atc_inv_domain: 1120 bytes
> > - arm_smmu_atc_inv_master: 1088 bytes
> > - arm_smmu_sync_cd: 1088 bytes
> > - __arm_smmu_tlb_inv_range: 1072 bytes
> > 
> > Move these allocations to the heap using kmalloc_obj() and kfree() to
> > eliminate the -Wframe-larger-than=1024 warnings and prevent potential
> > stack overflows.
> > 
> 
> Thanks for the patch. I agree that we should address these warnings, but
> moving these allocations to the heap via kmalloc_obj() in the fast path
> is problematic. Introducing heap allocation adds unnecessary latency and
> potential for allocation failure in hot paths.
> 
> So, yes, we are using a lot of stack but we're using it to do good
> things.. 
> 
> IMO, if we really want to address these, instead of kmalloc, we could
> potentially consider some pre-allocated per-CPU buffers (that's a lot of
> additional book-keeping though) to keep the data off the stack or
> something similar following a simple rule: The fast path must be 
> deterministic- no SLAB allocations and no introducing new failure points
> 
> The last thing we'd want is a graphic driver's shrinker calling
> dma-unmaps when the system is already under heavy memory pressure and 
> calling kmalloc leading to a circular dependency or allocation failure
> exactly when the system needs to perform the unmap the most.
> 
> Thanks,
> Praan

Hi Praan,

Thanks for the feedback.
I agree that kmalloc() is unsuitable for the SMMU fast path due to
potential deadlocks and the need for determinism.

To resolve the stack warnings, I'm considering using per-CPU buffers in v2. 
Does this direction sound reasonable, or would you prefer to keep it as-is
to avoid the added complexity?

-- 
Thanks,
Cheng-Yang

Re: [PATCH] iommu/arm-smmu-v3: Allocate cmdq_batch on the heap

[Nicolin Chen]

On Fri, Mar 13, 2026 at 02:24:09AM +0800, Cheng-Yang Chou wrote:
> On Wed, Mar 11, 2026 at 02:22:50PM +0000, Pranjal Shrivastava wrote:
> > IMO, if we really want to address these, instead of kmalloc, we could
> > potentially consider some pre-allocated per-CPU buffers (that's a lot of
> > additional book-keeping though) to keep the data off the stack or
> > something similar following a simple rule: The fast path must be 
> > deterministic- no SLAB allocations and no introducing new failure points

> To resolve the stack warnings, I'm considering using per-CPU buffers in v2. 
> Does this direction sound reasonable, or would you prefer to keep it as-is
> to avoid the added complexity?

I don't think per-CPU buffers would work here either..

arm_smmu_atc_inv_master() is used in a preemptible context, while
arm_smmu_atc_inv_domain() can be called from an irq context.

Think of a !SMP case for simplification: we only have one per-CPU
buffer, which is not enough if an IRQ preempts the task context.

Maybe having a smaller backup array on the stack that can be used
when the heap allocation fails? Still, I don't see how to address
it elegantly without losing some of the performance optimization.

Nicolin

Re: [PATCH] iommu/arm-smmu-v3: Allocate cmdq_batch on the heap

[Pranjal Shrivastava]

On Thu, Mar 12, 2026 at 03:50:19PM -0700, Nicolin Chen wrote:
> On Fri, Mar 13, 2026 at 02:24:09AM +0800, Cheng-Yang Chou wrote:
> > On Wed, Mar 11, 2026 at 02:22:50PM +0000, Pranjal Shrivastava wrote:
> > > IMO, if we really want to address these, instead of kmalloc, we could
> > > potentially consider some pre-allocated per-CPU buffers (that's a lot of
> > > additional book-keeping though) to keep the data off the stack or
> > > something similar following a simple rule: The fast path must be 
> > > deterministic- no SLAB allocations and no introducing new failure points
> 
> > To resolve the stack warnings, I'm considering using per-CPU buffers in v2. 
> > Does this direction sound reasonable, or would you prefer to keep it as-is
> > to avoid the added complexity?
> 
> I don't think per-CPU buffers would work here either..
> 
> arm_smmu_atc_inv_master() is used in a preemptible context, while
> arm_smmu_atc_inv_domain() can be called from an irq context.
> 
> Think of a !SMP case for simplification: we only have one per-CPU
> buffer, which is not enough if an IRQ preempts the task context.

+1

> 
> Maybe having a smaller backup array on the stack that can be used
> when the heap allocation fails? Still, I don't see how to address
> it elegantly without losing some of the performance optimization.
> 

A backup array is no good either IMO, stack sizes are fixed at compile
time, the compiler will still count those bytes against the 1024-byte
limit regardless of whether the heap allocation succeeds or fails. If
the limit changes tomorrow, we'll have to adjust the "backup array size"
Furthermore, for deep call chains 'smaller' array can still be the straw
that breaks the boundary. 

As for a pre-allocated global buffer, the synchronization and bookkeeping
required to safely handle re-entrancy between task and IRQ contexts would
essentially require writing a custom allocator inside the driver.

Falling back to code paths based on transient heap availability also 
introduces non-deterministic behavior in a critical path which must 
remain reliable when the system is under pressure.

I'm still open to suggestions in case we're able to come up with a
solution that keeps the unmap paths equally performant and reliable..

Thanks,
Praan

Re: [PATCH] iommu/arm-smmu-v3: Allocate cmdq_batch on the heap

[Robin Murphy]

On 2026-03-11 2:22 pm, Pranjal Shrivastava wrote:
> On Wed, Mar 11, 2026 at 05:44:44PM +0800, Cheng-Yang Chou wrote:
>> The arm_smmu_cmdq_batch structure is large and was being allocated on
>> the stack in four call sites, causing stack frame sizes to exceed the
>> 1024-byte limit:
>>
>> - arm_smmu_atc_inv_domain: 1120 bytes
>> - arm_smmu_atc_inv_master: 1088 bytes
>> - arm_smmu_sync_cd: 1088 bytes
>> - __arm_smmu_tlb_inv_range: 1072 bytes
>>
>> Move these allocations to the heap using kmalloc_obj() and kfree() to
>> eliminate the -Wframe-larger-than=1024 warnings and prevent potential
>> stack overflows.

Pro tip: you can also eliminate the warning by setting CONFIG_FRAME_WARN 
to a larger number, or to 0. The default is 2048, so you're already only 
getting a warning because you've gone out of your way to ask for a 
warning. The smaller the number you choose, the more warnings you'll 
get, but does that alone justify "fixing" them?

It's certainly plausible that we could get to issuing invalidation 
commands at the bottom of a relatively long callchain through 
subsystem/client driver/DMA API code, but have you observed a stack 
overflow in practice? It's not like these functions are re-entrant, or 
calling out to unknown external code, so while ~1KB is admittedly 
reasonably big, we can at least reason that there's never going to be 
much more *beyond* that (basically just whatever 
arm_smmu_cmdq_issue_cmdlist() uses).

> Thanks for the patch. I agree that we should address these warnings, but
> moving these allocations to the heap via kmalloc_obj() in the fast path
> is problematic. Introducing heap allocation adds unnecessary latency and
> potential for allocation failure in hot paths.
> 
> So, yes, we are using a lot of stack but we're using it to do good
> things..
> 
> IMO, if we really want to address these, instead of kmalloc, we could
> potentially consider some pre-allocated per-CPU buffers (that's a lot of
> additional book-keeping though) to keep the data off the stack or
> something similar following a simple rule: The fast path must be
> deterministic- no SLAB allocations and no introducing new failure points
> 
> The last thing we'd want is a graphic driver's shrinker calling
> dma-unmaps when the system is already under heavy memory pressure and
> calling kmalloc leading to a circular dependency or allocation failure
> exactly when the system needs to perform the unmap the most.

ISTR it's worse than that, and in fact we must not even attempt to 
allocate in a reclaim path at all, or it risks deadlock. So since the 
SMMU driver cannot realistically know the context of *why* it's being 
asked to unmap/invalidate something, I'm not sure it can ever be assumed 
to be safe.

Thanks,
Robin.

> 
> Thanks,
> Praan
> 
>> Signed-off-by: Cheng-Yang Chou <yphbchou0911@gmail.com>
>> ---
>>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 66 +++++++++++++++------
>>   1 file changed, 48 insertions(+), 18 deletions(-)
>>
>> 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 4d00d796f078..734546dc6a78 100644
>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> @@ -1281,7 +1281,7 @@ static void arm_smmu_sync_cd(struct arm_smmu_master *master,
>>   			     int ssid, bool leaf)
>>   {
>>   	size_t i;
>> -	struct arm_smmu_cmdq_batch cmds;
>> +	struct arm_smmu_cmdq_batch *cmds;
>>   	struct arm_smmu_device *smmu = master->smmu;
>>   	struct arm_smmu_cmdq_ent cmd = {
>>   		.opcode	= CMDQ_OP_CFGI_CD,
>> @@ -1291,13 +1291,23 @@ static void arm_smmu_sync_cd(struct arm_smmu_master *master,
>>   		},
>>   	};
>>   
>> -	arm_smmu_cmdq_batch_init(smmu, &cmds, &cmd);
>> +	cmds = kmalloc_obj(*cmds);
>> +	if (!cmds) {
>> +		struct arm_smmu_cmdq_ent cmd_all = { .opcode = CMDQ_OP_CFGI_ALL };
>> +
>> +		WARN_ONCE(1, "arm-smmu-v3: failed to allocate cmdq_batch, falling back to full CD invalidation\n");
>> +		arm_smmu_cmdq_issue_cmd_with_sync(smmu, &cmd_all);
>> +		return;
>> +	}
>> +
>> +	arm_smmu_cmdq_batch_init(smmu, cmds, &cmd);
>>   	for (i = 0; i < master->num_streams; i++) {
>>   		cmd.cfgi.sid = master->streams[i].id;
>> -		arm_smmu_cmdq_batch_add(smmu, &cmds, &cmd);
>> +		arm_smmu_cmdq_batch_add(smmu, cmds, &cmd);
>>   	}
>>   
>> -	arm_smmu_cmdq_batch_submit(smmu, &cmds);
>> +	arm_smmu_cmdq_batch_submit(smmu, cmds);
>> +	kfree(cmds);
>>   }
>>   
>>   static void arm_smmu_write_cd_l1_desc(struct arm_smmu_cdtab_l1 *dst,
>> @@ -2225,31 +2235,37 @@ arm_smmu_atc_inv_to_cmd(int ssid, unsigned long iova, size_t size,
>>   static int arm_smmu_atc_inv_master(struct arm_smmu_master *master,
>>   				   ioasid_t ssid)
>>   {
>> -	int i;
>> +	int i, ret;
>>   	struct arm_smmu_cmdq_ent cmd;
>> -	struct arm_smmu_cmdq_batch cmds;
>> +	struct arm_smmu_cmdq_batch *cmds;
>>   
>>   	arm_smmu_atc_inv_to_cmd(ssid, 0, 0, &cmd);
>>   
>> -	arm_smmu_cmdq_batch_init(master->smmu, &cmds, &cmd);
>> +	cmds = kmalloc_obj(*cmds);
>> +	if (!cmds)
>> +		return -ENOMEM;
>> +
>> +	arm_smmu_cmdq_batch_init(master->smmu, cmds, &cmd);
>>   	for (i = 0; i < master->num_streams; i++) {
>>   		cmd.atc.sid = master->streams[i].id;
>> -		arm_smmu_cmdq_batch_add(master->smmu, &cmds, &cmd);
>> +		arm_smmu_cmdq_batch_add(master->smmu, cmds, &cmd);
>>   	}
>>   
>> -	return arm_smmu_cmdq_batch_submit(master->smmu, &cmds);
>> +	ret = arm_smmu_cmdq_batch_submit(master->smmu, cmds);
>> +	kfree(cmds);
>> +	return ret;
>>   }
>>   
>>   int arm_smmu_atc_inv_domain(struct arm_smmu_domain *smmu_domain,
>>   			    unsigned long iova, size_t size)
>>   {
>>   	struct arm_smmu_master_domain *master_domain;
>> -	int i;
>> +	int i, ret;
>>   	unsigned long flags;
>>   	struct arm_smmu_cmdq_ent cmd = {
>>   		.opcode = CMDQ_OP_ATC_INV,
>>   	};
>> -	struct arm_smmu_cmdq_batch cmds;
>> +	struct arm_smmu_cmdq_batch *cmds;
>>   
>>   	if (!(smmu_domain->smmu->features & ARM_SMMU_FEAT_ATS))
>>   		return 0;
>> @@ -2271,7 +2287,11 @@ int arm_smmu_atc_inv_domain(struct arm_smmu_domain *smmu_domain,
>>   	if (!atomic_read(&smmu_domain->nr_ats_masters))
>>   		return 0;
>>   
>> -	arm_smmu_cmdq_batch_init(smmu_domain->smmu, &cmds, &cmd);
>> +	cmds = kmalloc_obj(*cmds);
>> +	if (!cmds)
>> +		return -ENOMEM;
>> +
>> +	arm_smmu_cmdq_batch_init(smmu_domain->smmu, cmds, &cmd);
>>   
>>   	spin_lock_irqsave(&smmu_domain->devices_lock, flags);
>>   	list_for_each_entry(master_domain, &smmu_domain->devices,
>> @@ -2294,12 +2314,14 @@ int arm_smmu_atc_inv_domain(struct arm_smmu_domain *smmu_domain,
>>   
>>   		for (i = 0; i < master->num_streams; i++) {
>>   			cmd.atc.sid = master->streams[i].id;
>> -			arm_smmu_cmdq_batch_add(smmu_domain->smmu, &cmds, &cmd);
>> +			arm_smmu_cmdq_batch_add(smmu_domain->smmu, cmds, &cmd);
>>   		}
>>   	}
>>   	spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
>>   
>> -	return arm_smmu_cmdq_batch_submit(smmu_domain->smmu, &cmds);
>> +	ret = arm_smmu_cmdq_batch_submit(smmu_domain->smmu, cmds);
>> +	kfree(cmds);
>> +	return ret;
>>   }
>>   
>>   /* IO_PGTABLE API */
>> @@ -2334,7 +2356,7 @@ static void __arm_smmu_tlb_inv_range(struct arm_smmu_cmdq_ent *cmd,
>>   	struct arm_smmu_device *smmu = smmu_domain->smmu;
>>   	unsigned long end = iova + size, num_pages = 0, tg = 0;
>>   	size_t inv_range = granule;
>> -	struct arm_smmu_cmdq_batch cmds;
>> +	struct arm_smmu_cmdq_batch *cmds;
>>   
>>   	if (!size)
>>   		return;
>> @@ -2362,7 +2384,14 @@ static void __arm_smmu_tlb_inv_range(struct arm_smmu_cmdq_ent *cmd,
>>   			num_pages++;
>>   	}
>>   
>> -	arm_smmu_cmdq_batch_init(smmu, &cmds, cmd);
>> +	cmds = kmalloc_obj(*cmds);
>> +	if (!cmds) {
>> +		WARN_ONCE(1, "arm-smmu-v3: failed to allocate cmdq_batch, falling back to full TLB invalidation\n");
>> +		arm_smmu_tlb_inv_context(smmu_domain);
>> +		return;
>> +	}
>> +
>> +	arm_smmu_cmdq_batch_init(smmu, cmds, cmd);
>>   
>>   	while (iova < end) {
>>   		if (smmu->features & ARM_SMMU_FEAT_RANGE_INV) {
>> @@ -2391,10 +2420,11 @@ static void __arm_smmu_tlb_inv_range(struct arm_smmu_cmdq_ent *cmd,
>>   		}
>>   
>>   		cmd->tlbi.addr = iova;
>> -		arm_smmu_cmdq_batch_add(smmu, &cmds, cmd);
>> +		arm_smmu_cmdq_batch_add(smmu, cmds, cmd);
>>   		iova += inv_range;
>>   	}
>> -	arm_smmu_cmdq_batch_submit(smmu, &cmds);
>> +	arm_smmu_cmdq_batch_submit(smmu, cmds);
>> +	kfree(cmds);
>>   }
>>   
>>   static void arm_smmu_tlb_inv_range_domain(unsigned long iova, size_t size,
>> -- 
>> 2.48.1
>>
>>