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

[Pranjal Shrivastava <praan@google.com>]

On Tue, Jul 08, 2025 at 03:34:32PM +0000, Mostafa Saleh wrote:
> 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.
> 

Can you point me to the caller? I quick grep shows me that only vfio,
dw-dma and some spi drivers call get_cached_msi_msg().. I can't also
find a place where the assignment desc->msg = msg happens except for in
PCIe bus.

> 
> > +
> >  	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.

Do you mean that this check is redundant as moving ahead we anyway seem
to check irq_get_msi_desc for each irq?

> Otherwise, I think dev_get_msi_domain() would be more suitable to check?

Ack.

> 
> > +
> > +	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);	\
> 	} \
> }
> 

Yup, that looks good. Thanks!

> 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
> >