Re: [PATCH v11 04/11] iommu/arm-smmu-v3: Add ARM_SMMU_FEAT_EVTQ for the event queue

[Mostafa Saleh <smostafa@google.com>]

On Mon, Oct 05, 2026 at 12:30:47PM -0700, Nicolin Chen wrote:
> The driver programs and enables the event queue unconditionally, while the
> PRI queue has an ARM_SMMU_FEAT_PRI gating each of its touch points. Yet a
> kdump kernel wants to leave both of the queues alone, which would take an
> is_kdump_kernel() test at every one of those places.
> 
> Add an ARM_SMMU_FEAT_EVTQ that the probe always sets, as the event queue is
> architecturally mandatory, and gate the queue's allocation, its interrupt,
> its MSI vector, and its CR0 and IRQ_CTRL enables on it. A subsequent change
> will turn the queue off in a single place.
> 
> No functional change intended.
> 
> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> Suggested-by: Robin Murphy <robin.murphy@arm.com>
> Acked-by: Kiryl Shutsemau (Meta) <kas@kernel.org>
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> Tested-by: Breno Leitao <leitao@kernel.org>
> Tested-by: Cristian Prundeanu <cpru@amazon.com>
> Assisted-by: LLM
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>

Reviewed-by: Mostafa Saleh <smostafa@google.com>

There is a nit below though.

Thanks,
Mostafa

> ---
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  1 +
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 89 ++++++++++++++-------
>  2 files changed, 61 insertions(+), 29 deletions(-)
> 
> 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 8f2c0b1cc4a8d..559bfa8cbc5e0 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> @@ -972,6 +972,7 @@ struct arm_smmu_device {
>  #define ARM_SMMU_FEAT_BBML2		(1 << 24)
>  #define ARM_SMMU_FEAT_HAFT		(1 << 25)
>  #define ARM_SMMU_FEAT_DS		(1 << 26)
> +#define ARM_SMMU_FEAT_EVTQ		(1 << 27)
>  	u32				features;
>  
>  #define ARM_SMMU_OPT_SKIP_PREFETCH	(1 << 0)
> 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 6ff61565f7583..756825e004774 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -2381,7 +2381,8 @@ static irqreturn_t arm_smmu_combined_irq_thread(int irq, void *dev)
>  {
>  	struct arm_smmu_device *smmu = dev;
>  
> -	arm_smmu_evtq_thread(irq, dev);
> +	if (smmu->features & ARM_SMMU_FEAT_EVTQ)
> +		arm_smmu_evtq_thread(irq, dev);
>  	if (smmu->features & ARM_SMMU_FEAT_PRI)
>  		arm_smmu_priq_thread(irq, dev);
>  
> @@ -2390,7 +2391,12 @@ static irqreturn_t arm_smmu_combined_irq_thread(int irq, void *dev)
>  
>  static irqreturn_t arm_smmu_combined_irq_handler(int irq, void *dev)
>  {
> -	arm_smmu_gerror_handler(irq, dev);
> +	struct arm_smmu_device *smmu = dev;
> +	irqreturn_t ret = arm_smmu_gerror_handler(irq, dev);
> +
> +	/* Without either queue, the thread would have nothing to drain */
> +	if (!(smmu->features & (ARM_SMMU_FEAT_EVTQ | ARM_SMMU_FEAT_PRI)))
> +		return ret;
>  	return IRQ_WAKE_THREAD;
>  }
>  
> @@ -4763,11 +4769,14 @@ static int arm_smmu_init_queues(struct arm_smmu_device *smmu)
>  		return ret;
>  
>  	/* evtq */
> -	ret = arm_smmu_init_one_queue(smmu, &smmu->evtq.q, smmu->page1,
> -				      ARM_SMMU_EVTQ_PROD, ARM_SMMU_EVTQ_CONS,
> -				      EVTQ_ENT_DWORDS, "evtq");
> -	if (ret)
> -		return ret;
> +	if (smmu->features & ARM_SMMU_FEAT_EVTQ) {
> +		ret = arm_smmu_init_one_queue(smmu, &smmu->evtq.q, smmu->page1,
> +					      ARM_SMMU_EVTQ_PROD,
> +					      ARM_SMMU_EVTQ_CONS,
> +					      EVTQ_ENT_DWORDS, "evtq");
> +		if (ret)
> +			return ret;
> +	}
>  
>  	if ((smmu->features & ARM_SMMU_FEAT_SVA) &&
>  	    (smmu->features & ARM_SMMU_FEAT_STALLS)) {
> @@ -4950,7 +4959,15 @@ static void arm_smmu_setup_msis(struct arm_smmu_device *smmu)
>  
>  	/* Clear the MSI address regs */
>  	writeq_relaxed(0, smmu->base + ARM_SMMU_GERROR_IRQ_CFG0);
> -	writeq_relaxed(0, smmu->base + ARM_SMMU_EVTQ_IRQ_CFG0);
> +
> +	/* PRIQ's vector needs EVTQ's to be kept */
> +	WARN_ON_ONCE(!(smmu->features & ARM_SMMU_FEAT_EVTQ) &&
> +		     (smmu->features & ARM_SMMU_FEAT_PRI));
> +

I can't see how we can end up in this case, I’d suggest dropping it
if I am not missing something.


> +	if (smmu->features & ARM_SMMU_FEAT_EVTQ)
> +		writeq_relaxed(0, smmu->base + ARM_SMMU_EVTQ_IRQ_CFG0);
> +	else
> +		nvec--;
>  
>  	if (smmu->features & ARM_SMMU_FEAT_PRI)
>  		writeq_relaxed(0, smmu->base + ARM_SMMU_PRIQ_IRQ_CFG0);
> @@ -4987,16 +5004,20 @@ static void arm_smmu_setup_unique_irqs(struct arm_smmu_device *smmu)
>  	arm_smmu_setup_msis(smmu);
>  
>  	/* Request interrupt lines */
> -	irq = smmu->evtq.q.irq;
> -	if (irq) {
> -		ret = devm_request_threaded_irq(smmu->dev, irq, NULL,
> -						arm_smmu_evtq_thread,
> -						IRQF_ONESHOT,
> -						"arm-smmu-v3-evtq", smmu);
> -		if (ret < 0)
> -			dev_warn(smmu->dev, "failed to enable evtq irq\n");
> -	} else {
> -		dev_warn(smmu->dev, "no evtq irq - events will not be reported!\n");
> +	if (smmu->features & ARM_SMMU_FEAT_EVTQ) {
> +		irq = smmu->evtq.q.irq;
> +		if (irq) {
> +			ret = devm_request_threaded_irq(smmu->dev, irq, NULL,
> +							arm_smmu_evtq_thread,
> +							IRQF_ONESHOT,
> +							"arm-smmu-v3-evtq", smmu);
> +			if (ret < 0)
> +				dev_warn(smmu->dev,
> +					 "failed to enable evtq irq\n");
> +		} else {
> +			dev_warn(smmu->dev,
> +				 "no evtq irq - events will not be reported!\n");
> +		}
>  	}
>  
>  	irq = smmu->gerr_irq;
> @@ -5029,7 +5050,7 @@ static void arm_smmu_setup_unique_irqs(struct arm_smmu_device *smmu)
>  static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
>  {
>  	int ret, irq;
> -	u32 irqen_flags = IRQ_CTRL_EVTQ_IRQEN | IRQ_CTRL_GERROR_IRQEN;
> +	u32 irqen_flags = IRQ_CTRL_GERROR_IRQEN;
>  
>  	/* Disable IRQs first */
>  	ret = arm_smmu_write_reg_sync(smmu, 0, ARM_SMMU_IRQ_CTRL,
> @@ -5055,6 +5076,8 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
>  	} else
>  		arm_smmu_setup_unique_irqs(smmu);
>  
> +	if (smmu->features & ARM_SMMU_FEAT_EVTQ)
> +		irqen_flags |= IRQ_CTRL_EVTQ_IRQEN;
>  	if (smmu->features & ARM_SMMU_FEAT_PRI)
>  		irqen_flags |= IRQ_CTRL_PRIQ_IRQEN;
>  
> @@ -5173,16 +5196,21 @@ static int arm_smmu_device_reset(struct arm_smmu_device *smmu)
>  		smmu, arm_smmu_make_cmd_op(CMDQ_OP_TLBI_NSNH_ALL));
>  
>  	/* Event queue */
> -	writeq_relaxed(smmu->evtq.q.q_base, smmu->base + ARM_SMMU_EVTQ_BASE);
> -	writel_relaxed(smmu->evtq.q.llq.prod, smmu->page1 + ARM_SMMU_EVTQ_PROD);
> -	writel_relaxed(smmu->evtq.q.llq.cons, smmu->page1 + ARM_SMMU_EVTQ_CONS);
> -
> -	enables |= CR0_EVTQEN;
> -	ret = arm_smmu_write_reg_sync(smmu, enables, ARM_SMMU_CR0,
> -				      ARM_SMMU_CR0ACK);
> -	if (ret) {
> -		dev_err(smmu->dev, "failed to enable event queue\n");
> -		return ret;
> +	if (smmu->features & ARM_SMMU_FEAT_EVTQ) {
> +		writeq_relaxed(smmu->evtq.q.q_base,
> +			       smmu->base + ARM_SMMU_EVTQ_BASE);
> +		writel_relaxed(smmu->evtq.q.llq.prod,
> +			       smmu->page1 + ARM_SMMU_EVTQ_PROD);
> +		writel_relaxed(smmu->evtq.q.llq.cons,
> +			       smmu->page1 + ARM_SMMU_EVTQ_CONS);
> +
> +		enables |= CR0_EVTQEN;
> +		ret = arm_smmu_write_reg_sync(smmu, enables, ARM_SMMU_CR0,
> +					      ARM_SMMU_CR0ACK);
> +		if (ret) {
> +			dev_err(smmu->dev, "failed to enable event queue\n");
> +			return ret;
> +		}
>  	}
>  
>  	/* PRI queue */
> @@ -5330,6 +5358,9 @@ static int arm_smmu_device_hw_probe(struct arm_smmu_device *smmu)
>  	u32 reg;
>  	bool coherent = smmu->features & ARM_SMMU_FEAT_COHERENCY;
>  
> +	/* The event queue is architecturally mandatory, unlike the PRI queue */
> +	smmu->features |= ARM_SMMU_FEAT_EVTQ;
> +
>  	/* IDR0 */
>  	reg = readl_relaxed(smmu->base + ARM_SMMU_IDR0);
>  
> -- 
> 2.43.0
>