Re: [RFC PATCH v3 4/8] iommu/arm-smmu-v3: Cache and restore MSI config

[Mostafa Saleh <smostafa@google.com>]

On Mon, Jun 16, 2025 at 08:31:45PM +0000, Pranjal Shrivastava wrote:
> The SMMU's MSI configuration registers (*_IRQ_CFGn) containing target
> address, data and memory attributes lose their state when the SMMU is
> powered down. We'll need to cache and restore their contents to ensure
> that MSIs work after the system resumes.
> 
> To address this, cache the original `msi_msg` within the `msi_desc`
> when the configuration is first written by `arm_smmu_write_msi_msg`.
> This primarily includes the target address and data since the memory
> attributes are fixed.
> 
> Introduce a new helper `arm_smmu_resume_msis` which will later be called
> during the driver's resume callback. The helper would retrieve the
> cached MSI message for each relevant interrupt (evtq, gerr, priq) via
> get_cached_msi_msg & re-config the registers via arm_smmu_write_msi_msg
> 
> Signed-off-by: Pranjal Shrivastava <praan@google.com>
> ---
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 45 +++++++++++++++++++++
>  1 file changed, 45 insertions(+)
> 
> 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 9ce00c959034..4699d3294d3e 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -3962,6 +3962,9 @@ static void arm_smmu_write_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
>  	struct arm_smmu_device *smmu = dev_get_drvdata(dev);
>  	phys_addr_t *cfg = arm_smmu_msi_cfg[desc->msi_index];
>  
> +	/* Cache the msi_msg for resume */
> +	desc->msg = *msg;

All callers to arm_smmu_write_msi_msg() get the msg from
get_cached_msi_msg() which already returns msi_desc::msg of this irq,
so that seems redundant.


> +
>  	doorbell = (((u64)msg->address_hi) << 32) | msg->address_lo;
>  	doorbell &= MSI_CFG0_ADDR_MASK;
>  
> @@ -3970,6 +3973,48 @@ static void arm_smmu_write_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
>  	writel_relaxed(ARM_SMMU_MEMATTR_DEVICE_nGnRE, smmu->base + cfg[2]);
>  }
>  
> +static void arm_smmu_resume_msis(struct arm_smmu_device *smmu)
> +{
> +	unsigned int irq;
> +	struct msi_desc *desc;
> +	struct msi_msg msg;
> +
> +	if (!(smmu->features & ARM_SMMU_FEAT_MSI))
> +		return;
> +
> +	if (!smmu->dev->msi.domain)
> +		return;
Is there a reason that is needed?

I’d expect irq_get_msi_desc() to return NULL if MSI is not used.
Otherwise, I think dev_get_msi_domain() would be more suitable to check?

> +
> +	irq = smmu->evtq.q.irq;
> +	desc = irq ? irq_get_msi_desc(irq) : NULL;
> +	if (desc) {
> +		get_cached_msi_msg(irq, &msg);
> +		arm_smmu_write_msi_msg(desc, &msg);
> +	} else {
> +		dev_err(smmu->dev, "Failed to resume evtq msi");
> +	}
> +
> +	irq = smmu->gerr_irq;
> +	desc = irq ? irq_get_msi_desc(irq) : NULL;
> +	if (desc) {
> +		get_cached_msi_msg(irq, &msg);
> +		arm_smmu_write_msi_msg(desc, &msg);
> +	} else {
> +		dev_err(smmu->dev, "Failed to resume gerror msi");
> +	}
> +
> +	if (smmu->features & ARM_SMMU_FEAT_PRI) {
> +		irq = smmu->priq.q.irq;
> +		desc = irq ? irq_get_msi_desc(irq) : NULL;
> +		if (desc) {
> +			get_cached_msi_msg(smmu->priq.q.irq, &msg);
> +			arm_smmu_write_msi_msg(desc, &msg);
> +		} else {
> +			dev_err(smmu->dev, "Failed to resume priq msi");
> +		}
> +	}
> +}
> +

This pattern seems to repeat, maybe it’s better in a macro

Something as:
#define smmu_resume_msi(smmu, _irq)  {	\
	unsigned int irq;	\
	struct msi_desc *desc; \
	struct msi_msg msg; \
	irq = smmu->_irq; \
	desc = irq ? irq_get_msi_desc(irq) : NULL; \
	if (desc) { \
		get_cached_msi_msg(irq, &msg); \
		arm_smmu_write_msi_msg(desc, &msg); \
	} else {	\
		dev_err(smmu->dev, "Failed to resume %s\n", #_irq);	\
	} \
}

static void arm_smmu_resume_msis(struct arm_smmu_device *smmu)
{
	if (!(smmu->features & ARM_SMMU_FEAT_MSI))
		return;

	if (!smmu->dev->msi.domain)
		return;

	smmu_resume_msi(smmu, evtq.q.irq);
	smmu_resume_msi(smmu, gerr_irq);
	if (smmu->features & ARM_SMMU_FEAT_PRI)
		smmu_resume_msi(smmu, priq.q.irq);
}


Thanks,
Mostafa

>  static void arm_smmu_setup_msis(struct arm_smmu_device *smmu)
>  {
>  	int ret, nvec = ARM_SMMU_MAX_MSIS;
> -- 
> 2.50.0.rc2.692.g299adb8693-goog
>