While polling for n spaces in the cmdq, the current code instead checks
if the queue is full. If the queue is almost full but not enough space
(<n), then the CMDQ timeout warning is never triggered even if the
polling has exceeded timeout limit.
The existing arm_smmu_cmdq_poll_until_not_full() doesn't fit efficiently
nor ideally to the only caller arm_smmu_cmdq_issue_cmdlist():
- It uses a new timer at every single call, which fails to limit to the
preset ARM_SMMU_POLL_TIMEOUT_US per issue.
- It has a redundant internal queue_full(), which doesn't detect whether
there is a enough space for number of n commands.
This patch polls for the availability of exact space instead of full and
emit timeout warning accordingly.
Fixes: 587e6c10a7ce ("iommu/arm-smmu-v3: Reduce contention during command-queue insertion")
Co-developed-by: Yu Zhang <zhangyu1@linux.microsoft.com>
Signed-off-by: Yu Zhang <zhangyu1@linux.microsoft.com>
Signed-off-by: Jacob Pan <jacob.pan@linux.microsoft.com>
---
v4:
- Deleted non-ETIMEOUT error handling for queue_poll (Nicolin)
v3:
- Use a helper for cmdq poll instead of open coding (Nicolin)
- Add more explanation in the commit message (Nicolin)
v2: - Reduced debug print info (Nicolin)
- Use a separate irq flags for exclusive lock
- Handle queue_poll err code other than ETIMEOUT
---
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 41 ++++++++-------------
1 file changed, 16 insertions(+), 25 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 2a8b46b948f0..9824bd808725 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -138,12 +138,6 @@ static bool queue_has_space(struct arm_smmu_ll_queue *q, u32 n)
return space >= n;
}
-static bool queue_full(struct arm_smmu_ll_queue *q)
-{
- return Q_IDX(q, q->prod) == Q_IDX(q, q->cons) &&
- Q_WRP(q, q->prod) != Q_WRP(q, q->cons);
-}
-
static bool queue_empty(struct arm_smmu_ll_queue *q)
{
return Q_IDX(q, q->prod) == Q_IDX(q, q->cons) &&
@@ -633,14 +627,13 @@ static void arm_smmu_cmdq_poll_valid_map(struct arm_smmu_cmdq *cmdq,
__arm_smmu_cmdq_poll_set_valid_map(cmdq, sprod, eprod, false);
}
-/* Wait for the command queue to become non-full */
-static int arm_smmu_cmdq_poll_until_not_full(struct arm_smmu_device *smmu,
- struct arm_smmu_cmdq *cmdq,
- struct arm_smmu_ll_queue *llq)
+
+static inline void arm_smmu_cmdq_poll(struct arm_smmu_device *smmu,
+ struct arm_smmu_cmdq *cmdq,
+ struct arm_smmu_ll_queue *llq,
+ struct arm_smmu_queue_poll *qp)
{
unsigned long flags;
- struct arm_smmu_queue_poll qp;
- int ret = 0;
/*
* Try to update our copy of cons by grabbing exclusive cmdq access. If
@@ -650,19 +643,16 @@ static int arm_smmu_cmdq_poll_until_not_full(struct arm_smmu_device *smmu,
WRITE_ONCE(cmdq->q.llq.cons, readl_relaxed(cmdq->q.cons_reg));
arm_smmu_cmdq_exclusive_unlock_irqrestore(cmdq, flags);
llq->val = READ_ONCE(cmdq->q.llq.val);
- return 0;
+ return;
}
- queue_poll_init(smmu, &qp);
- do {
- llq->val = READ_ONCE(cmdq->q.llq.val);
- if (!queue_full(llq))
- break;
-
- ret = queue_poll(&qp);
- } while (!ret);
-
- return ret;
+ if (queue_poll(qp) == -ETIMEDOUT) {
+ dev_err_ratelimited(smmu->dev, "CMDQ timed out, cons: %08x, prod: 0x%08x\n",
+ llq->cons, llq->prod);
+ /* Restart the timer */
+ queue_poll_init(smmu, qp);
+ }
+ llq->val = READ_ONCE(cmdq->q.llq.val);
}
/*
@@ -804,12 +794,13 @@ int arm_smmu_cmdq_issue_cmdlist(struct arm_smmu_device *smmu,
local_irq_save(flags);
llq.val = READ_ONCE(cmdq->q.llq.val);
do {
+ struct arm_smmu_queue_poll qp;
u64 old;
+ queue_poll_init(smmu, &qp);
while (!queue_has_space(&llq, n + sync)) {
local_irq_restore(flags);
- if (arm_smmu_cmdq_poll_until_not_full(smmu, cmdq, &llq))
- dev_err_ratelimited(smmu->dev, "CMDQ timeout\n");
+ arm_smmu_cmdq_poll(smmu, cmdq, &llq, &qp);
local_irq_save(flags);
}
--
2.43.0On Fri, Nov 14, 2025 at 09:17:17AM -0800, Jacob Pan wrote:
> While polling for n spaces in the cmdq, the current code instead checks
> if the queue is full. If the queue is almost full but not enough space
> (<n), then the CMDQ timeout warning is never triggered even if the
> polling has exceeded timeout limit.
>
> The existing arm_smmu_cmdq_poll_until_not_full() doesn't fit efficiently
> nor ideally to the only caller arm_smmu_cmdq_issue_cmdlist():
> - It uses a new timer at every single call, which fails to limit to the
> preset ARM_SMMU_POLL_TIMEOUT_US per issue.
> - It has a redundant internal queue_full(), which doesn't detect whether
> there is a enough space for number of n commands.
>
> This patch polls for the availability of exact space instead of full and
> emit timeout warning accordingly.
>
> Fixes: 587e6c10a7ce ("iommu/arm-smmu-v3: Reduce contention during command-queue insertion")
> Co-developed-by: Yu Zhang <zhangyu1@linux.microsoft.com>
> Signed-off-by: Yu Zhang <zhangyu1@linux.microsoft.com>
> Signed-off-by: Jacob Pan <jacob.pan@linux.microsoft.com>
I'm assuming you're seeing problems with an emulated command queue? Any
chance you could make that bigger?
> @@ -804,12 +794,13 @@ int arm_smmu_cmdq_issue_cmdlist(struct arm_smmu_device *smmu,
> local_irq_save(flags);
> llq.val = READ_ONCE(cmdq->q.llq.val);
> do {
> + struct arm_smmu_queue_poll qp;
> u64 old;
>
> + queue_poll_init(smmu, &qp);
> while (!queue_has_space(&llq, n + sync)) {
> local_irq_restore(flags);
> - if (arm_smmu_cmdq_poll_until_not_full(smmu, cmdq, &llq))
> - dev_err_ratelimited(smmu->dev, "CMDQ timeout\n");
> + arm_smmu_cmdq_poll(smmu, cmdq, &llq, &qp);
Isn't this broken for wfe-based polling? The SMMU only generates the
wake-up event when the queue becomes non-full.
Will
Hi Will, On Tue, 25 Nov 2025 17:19:16 +0000 Will Deacon <will@kernel.org> wrote: > I'm assuming you're seeing problems with an emulated command queue? > Any chance you could make that bigger? Yes, it was initially observed on HyperV emulated CMDQ with small queue size, which was chosen for latency reasons. But as I explained in the other thread, the queue space polling timeout detection problem is not directly related to queue size. It is a code logic bug IMHO. Sorry I forgot to directly answer these.
Hi Will,
On Tue, 25 Nov 2025 17:19:16 +0000
Will Deacon <will@kernel.org> wrote:
> On Fri, Nov 14, 2025 at 09:17:17AM -0800, Jacob Pan wrote:
> > While polling for n spaces in the cmdq, the current code instead
> > checks if the queue is full. If the queue is almost full but not
> > enough space (<n), then the CMDQ timeout warning is never triggered
> > even if the polling has exceeded timeout limit.
> >
> > The existing arm_smmu_cmdq_poll_until_not_full() doesn't fit
> > efficiently nor ideally to the only caller
> > arm_smmu_cmdq_issue_cmdlist():
> > - It uses a new timer at every single call, which fails to limit
> > to the preset ARM_SMMU_POLL_TIMEOUT_US per issue.
> > - It has a redundant internal queue_full(), which doesn't detect
> > whether there is a enough space for number of n commands.
> >
> > This patch polls for the availability of exact space instead of
> > full and emit timeout warning accordingly.
> >
> > Fixes: 587e6c10a7ce ("iommu/arm-smmu-v3: Reduce contention during
> > command-queue insertion") Co-developed-by: Yu Zhang
> > <zhangyu1@linux.microsoft.com> Signed-off-by: Yu Zhang
> > <zhangyu1@linux.microsoft.com> Signed-off-by: Jacob Pan
> > <jacob.pan@linux.microsoft.com>
>
> I'm assuming you're seeing problems with an emulated command queue?
> Any chance you could make that bigger?
>
This is not related to queue size, but rather a logic issue when
anytime queue is nearly full.
> > @@ -804,12 +794,13 @@ int arm_smmu_cmdq_issue_cmdlist(struct
> > arm_smmu_device *smmu, local_irq_save(flags);
> > llq.val = READ_ONCE(cmdq->q.llq.val);
> > do {
> > + struct arm_smmu_queue_poll qp;
> > u64 old;
> >
> > + queue_poll_init(smmu, &qp);
> > while (!queue_has_space(&llq, n + sync)) {
> > local_irq_restore(flags);
> > - if
> > (arm_smmu_cmdq_poll_until_not_full(smmu, cmdq, &llq))
> > - dev_err_ratelimited(smmu->dev,
> > "CMDQ timeout\n");
> > + arm_smmu_cmdq_poll(smmu, cmdq, &llq, &qp);
> >
>
> Isn't this broken for wfe-based polling? The SMMU only generates the
> wake-up event when the queue becomes non-full.
I don't see this is a problem since any interrupts such as scheduler
tick can be a break evnt for WFE, no?
I have also tested this with WFE on BM with no issues. HyperV VM does
not support WFE.
On 2025-11-30 11:06 pm, Jacob Pan wrote:
> Hi Will,
>
> On Tue, 25 Nov 2025 17:19:16 +0000
> Will Deacon <will@kernel.org> wrote:
>
>> On Fri, Nov 14, 2025 at 09:17:17AM -0800, Jacob Pan wrote:
>>> While polling for n spaces in the cmdq, the current code instead
>>> checks if the queue is full. If the queue is almost full but not
>>> enough space (<n), then the CMDQ timeout warning is never triggered
>>> even if the polling has exceeded timeout limit.
>>>
>>> The existing arm_smmu_cmdq_poll_until_not_full() doesn't fit
>>> efficiently nor ideally to the only caller
>>> arm_smmu_cmdq_issue_cmdlist():
>>> - It uses a new timer at every single call, which fails to limit
>>> to the preset ARM_SMMU_POLL_TIMEOUT_US per issue.
>>> - It has a redundant internal queue_full(), which doesn't detect
>>> whether there is a enough space for number of n commands.
>>>
>>> This patch polls for the availability of exact space instead of
>>> full and emit timeout warning accordingly.
>>>
>>> Fixes: 587e6c10a7ce ("iommu/arm-smmu-v3: Reduce contention during
>>> command-queue insertion") Co-developed-by: Yu Zhang
>>> <zhangyu1@linux.microsoft.com> Signed-off-by: Yu Zhang
>>> <zhangyu1@linux.microsoft.com> Signed-off-by: Jacob Pan
>>> <jacob.pan@linux.microsoft.com>
>>
>> I'm assuming you're seeing problems with an emulated command queue?
>> Any chance you could make that bigger?
>>
> This is not related to queue size, but rather a logic issue when
> anytime queue is nearly full.
It is absolutely related to queue size, because there are only two
reasons why this warning should ever be seen:
1: The SMMU is in some unexpected error state and has stopped consuming
commands altogether.
2: The queue is far too small to do its job of buffering commands for
the number of present CPUs.
>>> @@ -804,12 +794,13 @@ int arm_smmu_cmdq_issue_cmdlist(struct
>>> arm_smmu_device *smmu, local_irq_save(flags);
>>> llq.val = READ_ONCE(cmdq->q.llq.val);
>>> do {
>>> + struct arm_smmu_queue_poll qp;
>>> u64 old;
>>>
>>> + queue_poll_init(smmu, &qp);
>>> while (!queue_has_space(&llq, n + sync)) {
>>> local_irq_restore(flags);
>>> - if
>>> (arm_smmu_cmdq_poll_until_not_full(smmu, cmdq, &llq))
>>> - dev_err_ratelimited(smmu->dev,
>>> "CMDQ timeout\n");
>>> + arm_smmu_cmdq_poll(smmu, cmdq, &llq, &qp);
>>>
>>
>> Isn't this broken for wfe-based polling? The SMMU only generates the
>> wake-up event when the queue becomes non-full.
> I don't see this is a problem since any interrupts such as scheduler
> tick can be a break evnt for WFE, no?
No, we cannot assume that any other WFE wakeup event is *guaranteed*.
It's certainly possible to get stuck in WFE on a NOHZ_FULL kernel with
the arch timer event stream disabled - I recall proving that back when I
hoped I might not have to bother upstreaming a workaround for MMU-600
erratum #1076982.
Yes, a large part of the reason we enable the event stream by default is
to help mitigate errata and software bugs which lead to missed events,
but that still doesn't mean we should consciously abuse it. I guess
something like the diff below (completely untested) would be a bit
closer to correct (basically, allow WFE when the queue is completely
full, but suppress it if we're then still waiting for more space in a
non-full queue).
Thanks,
Robin.
----->8-----
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 a0c6d87f85a1..206c6c6860dd 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -123,7 +123,7 @@ static void parse_driver_options(struct arm_smmu_device *smmu)
}
/* Low-level queue manipulation functions */
-static bool queue_has_space(struct arm_smmu_ll_queue *q, u32 n)
+static int queue_space(struct arm_smmu_ll_queue *q)
{
u32 space, prod, cons;
@@ -135,7 +135,7 @@ static bool queue_has_space(struct arm_smmu_ll_queue *q, u32 n)
else
space = cons - prod;
- return space >= n;
+ return space;
}
static bool queue_empty(struct arm_smmu_ll_queue *q)
@@ -796,9 +796,11 @@ int arm_smmu_cmdq_issue_cmdlist(struct arm_smmu_device *smmu,
do {
struct arm_smmu_queue_poll qp;
u64 old;
+ int space;
queue_poll_init(smmu, &qp);
- while (!queue_has_space(&llq, n + sync)) {
+ while (space = queue_space(&llq), space < n + sync) {
+ qp.wfe &= !space;
local_irq_restore(flags);
arm_smmu_cmdq_poll(smmu, cmdq, &llq, &qp);
local_irq_save(flags);
Hi Robin,
On Mon, 1 Dec 2025 19:57:31 +0000
Robin Murphy <robin.murphy@arm.com> wrote:
> On 2025-11-30 11:06 pm, Jacob Pan wrote:
> > Hi Will,
> >
> > On Tue, 25 Nov 2025 17:19:16 +0000
> > Will Deacon <will@kernel.org> wrote:
> >
> >> On Fri, Nov 14, 2025 at 09:17:17AM -0800, Jacob Pan wrote:
> >>> While polling for n spaces in the cmdq, the current code instead
> >>> checks if the queue is full. If the queue is almost full but not
> >>> enough space (<n), then the CMDQ timeout warning is never
> >>> triggered even if the polling has exceeded timeout limit.
> >>>
> >>> The existing arm_smmu_cmdq_poll_until_not_full() doesn't fit
> >>> efficiently nor ideally to the only caller
> >>> arm_smmu_cmdq_issue_cmdlist():
> >>> - It uses a new timer at every single call, which fails to limit
> >>> to the preset ARM_SMMU_POLL_TIMEOUT_US per issue.
> >>> - It has a redundant internal queue_full(), which doesn't detect
> >>> whether there is a enough space for number of n commands.
> >>>
> >>> This patch polls for the availability of exact space instead of
> >>> full and emit timeout warning accordingly.
> >>>
> >>> Fixes: 587e6c10a7ce ("iommu/arm-smmu-v3: Reduce contention during
> >>> command-queue insertion") Co-developed-by: Yu Zhang
> >>> <zhangyu1@linux.microsoft.com> Signed-off-by: Yu Zhang
> >>> <zhangyu1@linux.microsoft.com> Signed-off-by: Jacob Pan
> >>> <jacob.pan@linux.microsoft.com>
> >>
> >> I'm assuming you're seeing problems with an emulated command queue?
> >> Any chance you could make that bigger?
> >>
> > This is not related to queue size, but rather a logic issue when
> > anytime queue is nearly full.
>
> It is absolutely related to queue size, because there are only two
> reasons why this warning should ever be seen:
>
> 1: The SMMU is in some unexpected error state and has stopped
> consuming commands altogether.
> 2: The queue is far too small to do its job of buffering commands for
> the number of present CPUs.
I agree that smaller queue size or slow emulation makes this problem
more visible, in this sense they are related.
> >>> @@ -804,12 +794,13 @@ int arm_smmu_cmdq_issue_cmdlist(struct
> >>> arm_smmu_device *smmu, local_irq_save(flags);
> >>> llq.val = READ_ONCE(cmdq->q.llq.val);
> >>> do {
> >>> + struct arm_smmu_queue_poll qp;
> >>> u64 old;
> >>>
> >>> + queue_poll_init(smmu, &qp);
> >>> while (!queue_has_space(&llq, n + sync)) {
> >>> local_irq_restore(flags);
> >>> - if
> >>> (arm_smmu_cmdq_poll_until_not_full(smmu, cmdq, &llq))
> >>> - dev_err_ratelimited(smmu->dev,
> >>> "CMDQ timeout\n");
> >>> + arm_smmu_cmdq_poll(smmu, cmdq, &llq,
> >>> &qp);
> >>
> >> Isn't this broken for wfe-based polling? The SMMU only generates
> >> the wake-up event when the queue becomes non-full.
> > I don't see this is a problem since any interrupts such as scheduler
> > tick can be a break evnt for WFE, no?
>
> No, we cannot assume that any other WFE wakeup event is *guaranteed*.
> It's certainly possible to get stuck in WFE on a NOHZ_FULL kernel with
> the arch timer event stream disabled - I recall proving that back
> when I hoped I might not have to bother upstreaming a workaround for
> MMU-600 erratum #1076982.
>
Make sense, thanks for sharing this history.
> Yes, a large part of the reason we enable the event stream by default
> is to help mitigate errata and software bugs which lead to missed
> events, but that still doesn't mean we should consciously abuse it. I
> guess something like the diff below (completely untested) would be a
> bit closer to correct (basically, allow WFE when the queue is
> completely full, but suppress it if we're then still waiting for more
> space in a non-full queue).
>
The diff below looks good to me, I can give it a try.
But at the same time, since queue full is supposed to be an exceptional
scenario, I wonder if we can just disable WFE all together in this case.
Power saving may not matter here?
> Thanks,
> Robin.
>
> ----->8-----
> 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
> a0c6d87f85a1..206c6c6860dd 100644 ---
> a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++
> b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -123,7 +123,7 @@
> static void parse_driver_options(struct arm_smmu_device *smmu) }
>
> /* Low-level queue manipulation functions */
> -static bool queue_has_space(struct arm_smmu_ll_queue *q, u32 n)
> +static int queue_space(struct arm_smmu_ll_queue *q)
> {
> u32 space, prod, cons;
>
> @@ -135,7 +135,7 @@ static bool queue_has_space(struct
> arm_smmu_ll_queue *q, u32 n) else
> space = cons - prod;
>
> - return space >= n;
> + return space;
> }
>
> static bool queue_empty(struct arm_smmu_ll_queue *q)
> @@ -796,9 +796,11 @@ int arm_smmu_cmdq_issue_cmdlist(struct
> arm_smmu_device *smmu, do {
> struct arm_smmu_queue_poll qp;
> u64 old;
> + int space;
>
> queue_poll_init(smmu, &qp);
> - while (!queue_has_space(&llq, n + sync)) {
> + while (space = queue_space(&llq), space < n + sync) {
> + qp.wfe &= !space;
> local_irq_restore(flags);
> arm_smmu_cmdq_poll(smmu, cmdq, &llq, &qp);
> local_irq_save(flags);
On Fri, Nov 14, 2025 at 09:17:17AM -0800, Jacob Pan wrote:
> While polling for n spaces in the cmdq, the current code instead checks
> if the queue is full. If the queue is almost full but not enough space
> (<n), then the CMDQ timeout warning is never triggered even if the
> polling has exceeded timeout limit.
>
> The existing arm_smmu_cmdq_poll_until_not_full() doesn't fit efficiently
> nor ideally to the only caller arm_smmu_cmdq_issue_cmdlist():
> - It uses a new timer at every single call, which fails to limit to the
> preset ARM_SMMU_POLL_TIMEOUT_US per issue.
> - It has a redundant internal queue_full(), which doesn't detect whether
> there is a enough space for number of n commands.
>
> This patch polls for the availability of exact space instead of full and
> emit timeout warning accordingly.
>
> Fixes: 587e6c10a7ce ("iommu/arm-smmu-v3: Reduce contention during command-queue insertion")
> Co-developed-by: Yu Zhang <zhangyu1@linux.microsoft.com>
> Signed-off-by: Yu Zhang <zhangyu1@linux.microsoft.com>
> Signed-off-by: Jacob Pan <jacob.pan@linux.microsoft.com>
Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
© 2016 - 2026 Red Hat, Inc.