Re: [PATCH v8 09/13] arm/smmu-v3: add suspend/resume handlers

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Luca,

Thank you for the review.

On Mon, Apr 27, 2026 at 5:03 PM Luca Fancellu <Luca.Fancellu@xxxxxxx> wrote:
>
> Hi Mykola,
>
> >
> > diff --git a/xen/drivers/passthrough/arm/smmu-v3.c 
> > b/xen/drivers/passthrough/arm/smmu-v3.c
> > index bf153227db..7607ffc9ca 100644
> > --- a/xen/drivers/passthrough/arm/smmu-v3.c
> > +++ b/xen/drivers/passthrough/arm/smmu-v3.c
> > @@ -1814,8 +1814,7 @@ static int arm_smmu_write_reg_sync(struct 
> > arm_smmu_device *smmu, u32 val,
> > }
> >
> > /* GBPA is "special" */
> > -static int __init arm_smmu_update_gbpa(struct arm_smmu_device *smmu,
> > -                                       u32 set, u32 clr)
> > +static int arm_smmu_update_gbpa(struct arm_smmu_device *smmu, u32 set, u32 
> > clr)
> > {
> > int ret;
> > u32 reg, __iomem *gbpa = smmu->base + ARM_SMMU_GBPA;
> > @@ -1995,10 +1994,29 @@ err_free_evtq_irq:
> > return ret;
> > }
> >
> > +static int arm_smmu_enable_irqs(struct arm_smmu_device *smmu)
> > +{
> > + int ret;
> > + u32 irqen_flags = IRQ_CTRL_EVTQ_IRQEN | IRQ_CTRL_GERROR_IRQEN;
> > +
> > + if ( smmu->features & ARM_SMMU_FEAT_PRI )
> > + irqen_flags |= IRQ_CTRL_PRIQ_IRQEN;
> > +
> > + /* Enable interrupt generation on the SMMU */
> > + ret = arm_smmu_write_reg_sync(smmu, irqen_flags,
> > +      ARM_SMMU_IRQ_CTRL, ARM_SMMU_IRQ_CTRLACK);
> > + if ( ret )
> > + {
> > + dev_warn(smmu->dev, "failed to enable irqs\n");
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static int __init arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
> > {
> > int ret, irq;
> > - u32 irqen_flags = IRQ_CTRL_EVTQ_IRQEN | IRQ_CTRL_GERROR_IRQEN;
> >
> > /* Disable IRQs first */
> > ret = arm_smmu_write_reg_sync(smmu, 0, ARM_SMMU_IRQ_CTRL,
> > @@ -2028,22 +2046,7 @@ static int __init arm_smmu_setup_irqs(struct 
> > arm_smmu_device *smmu)
> > }
> > }
> >
> > - if (smmu->features & ARM_SMMU_FEAT_PRI)
> > - irqen_flags |= IRQ_CTRL_PRIQ_IRQEN;
> > -
> > - /* Enable interrupt generation on the SMMU */
> > - ret = arm_smmu_write_reg_sync(smmu, irqen_flags,
> > -      ARM_SMMU_IRQ_CTRL, ARM_SMMU_IRQ_CTRLACK);
> > - if (ret) {
> > - dev_warn(smmu->dev, "failed to enable irqs\n");
> > - goto err_free_irqs;
> > - }
> > -
> > return 0;
> > -
> > -err_free_irqs:
> > - arm_smmu_free_irqs(smmu);
> > - return ret;
> > }
> >
> > static int arm_smmu_device_disable(struct arm_smmu_device *smmu)
> > @@ -2057,7 +2060,7 @@ static int arm_smmu_device_disable(struct 
> > arm_smmu_device *smmu)
> > return ret;
> > }
> >
> > -static int __init arm_smmu_device_reset(struct arm_smmu_device *smmu)
> > +static int arm_smmu_device_reset(struct arm_smmu_device *smmu)
> > {
> > int ret;
> > u32 reg, enables;
> > @@ -2163,17 +2166,9 @@ static int __init arm_smmu_device_reset(struct 
> > arm_smmu_device *smmu)
> > }
> > }
> >
> > - ret = arm_smmu_setup_irqs(smmu);
> > - if (ret) {
> > - dev_err(smmu->dev, "failed to setup irqs\n");
>
> We are moving this one to the probe and ..
>
> > + ret = arm_smmu_enable_irqs(smmu);
> > + if ( ret )
>
> changing with this one, but arm_smmu_setup_irqs() also calls 
> arm_smmu_setup_unique_irqs() which
> calls arm_smmu_setup_msis(), are we sure that on resume we will get the same 
> state?

This follows the split introduced in the Linux arm-smmu-v3 runtime/system sleep
series:

https://lore.kernel.org/linux-iommu/20260414194702.1229094-1-praan@xxxxxxxxxx/

The intent is to keep IRQ handler registration as one-time probe state, while
reset/resume only restores the SMMU hardware state and re-enables interrupt
generation.

You are right that the MSI case needs extra care. In the Linux series this is
handled by arm_smmu_resume_msis(), which restores the SMMU-side MSI
configuration. I did not port that part in this patch because Xen SMMUv3 MSI
support is currently documented as unsupported and is not part of the
supported/tested path, so this patch only covers the wired IRQ path used by Xen
today.

If Xen SMMUv3 MSI support becomes usable in the future, the resume path will
need an equivalent MSI restore step before IRQ_CTRL is re-enabled.

I will add a code comment and update the commit message to make this scope
explicit. I also noticed that I accidentally dropped the reference to Pranjal's
Linux series while reworking the patch; I will restore the Link/attribution in
the next version.

>
> > return ret;
> > - }
> > -
> > - /* Initialize tasklets for threaded IRQs*/
> > - tasklet_init(&smmu->evtq_irq_tasklet, arm_smmu_evtq_tasklet, smmu);
> > - tasklet_init(&smmu->priq_irq_tasklet, arm_smmu_priq_tasklet, smmu);
> > - tasklet_init(&smmu->combined_irq_tasklet, arm_smmu_combined_irq_tasklet,
> > - smmu);
> >
> > /* Enable the SMMU interface, or ensure bypass */
> > if (disable_bypass) {
> > @@ -2181,20 +2176,16 @@ static int __init arm_smmu_device_reset(struct 
> > arm_smmu_device *smmu)
> > } else {
> > ret = arm_smmu_update_gbpa(smmu, 0, GBPA_ABORT);
> > if (ret)
> > - goto err_free_irqs;
> > + return ret;
> > }
> > ret = arm_smmu_write_reg_sync(smmu, enables, ARM_SMMU_CR0,
> >     ARM_SMMU_CR0ACK);
> > if (ret) {
> > dev_err(smmu->dev, "failed to enable SMMU interface\n");
> > - goto err_free_irqs;
> > + return ret;
> > }
> >
> > return 0;
> > -
> > -err_free_irqs:
> > - arm_smmu_free_irqs(smmu);
> > - return ret;
> > }
> >
> > static int arm_smmu_device_hw_probe(struct arm_smmu_device *smmu)
> > @@ -2558,10 +2549,23 @@ static int __init arm_smmu_device_probe(struct 
> > platform_device *pdev)
> > if (ret)
> > goto out_free;
> >
> > + ret = arm_smmu_setup_irqs(smmu);
> > + if ( ret )
> > + {
> > + dev_err(smmu->dev, "failed to setup irqs\n");
> > + goto out_free;
> > + }
> > +
> > + /* Initialize tasklets for threaded IRQs*/
> > + tasklet_init(&smmu->evtq_irq_tasklet, arm_smmu_evtq_tasklet, smmu);
> > + tasklet_init(&smmu->priq_irq_tasklet, arm_smmu_priq_tasklet, smmu);
> > + tasklet_init(&smmu->combined_irq_tasklet, arm_smmu_combined_irq_tasklet,
> > + smmu);
> > +
> > /* Reset the device */
> > ret = arm_smmu_device_reset(smmu);
> > if (ret)
> > - goto out_free;
> > + goto out_free_irqs;
> >
> > /*
> > * Keep a list of all probed devices. This will be used to query
> > @@ -2575,6 +2579,8 @@ static int __init arm_smmu_device_probe(struct 
> > platform_device *pdev)
> >
> > return 0;
> >
> > +out_free_irqs:
> > + arm_smmu_free_irqs(smmu);
> >
> > out_free:
> > arm_smmu_free_structures(smmu);
> > @@ -2855,6 +2861,96 @@ static void 
> > arm_smmu_iommu_xen_domain_teardown(struct domain *d)
> > xfree(xen_domain);
> > }
> >
> > +#ifdef CONFIG_SYSTEM_SUSPEND
> > +
> > +static void arm_smmu_reset_for_suspend_rollback(struct arm_smmu_device 
> > *smmu)
> > +{
> > + int ret = arm_smmu_device_reset(smmu);
> > +
> > + if ( ret )
> > + dev_err(smmu->dev, "Failed to reset during suspend rollback: %d\n",
> > + ret);
> > +}
> > +
> > +static int arm_smmu_suspend(void)
> > +{
> > + struct arm_smmu_device *smmu;
> > + int ret = 0;
> > +
> > + list_for_each_entry(smmu, &arm_smmu_devices, devices)
> > + {
> > + bool wfe = !!(smmu->features & ARM_SMMU_FEAT_SEV);
> > +
> > + /* Abort all transactions before disable to avoid spurious bypass */
> > + ret = arm_smmu_update_gbpa(smmu, GBPA_ABORT, 0);
> > + if ( ret )
> > + goto fail;
> > +
> > + /* Disable the SMMU via CR0.EN and all queues except CMDQ */
> > + ret = arm_smmu_write_reg_sync(smmu, CR0_CMDQEN, ARM_SMMU_CR0,
> > + ARM_SMMU_CR0ACK);
> > + if ( ret )
> > + {
> > + dev_err(smmu->dev, "Timed-out while disabling smmu\n");
> > + goto fail;
> > + }
> > +
> > + /*
> > + * At this point the SMMU is completely disabled and won't access
> > + * any translation/config structures, even speculative accesses
> > + * aren't performed as per the IHI0070 spec (section 6.3.9.6).
> > + */
> > +
> > + /* Wait for the CMDQs to be drained to flush any pending commands */
> > + ret = queue_poll_cons(&smmu->cmdq.q, true, wfe);
> > + if ( ret )
> > + {
> > + dev_err(smmu->dev, "Draining queues timed-out\n");
> > + goto fail;
> > + }
>
> polling the queue doesn’t give you the assurance that all prior commands are 
> complete,
> I would use arm_smmu_cmdq_issue_sync for that instead of the above.
>
> ret = arm_smmu_cmdq_issue_sync(smmu);
> if ( ret )
>    goto fail;

Yes, I agree.

Polling CONS only shows that the SMMU has consumed the CMDQ entries; it does
not provide the completion semantics we want here. I will replace the direct
queue_poll_cons() in the suspend path with arm_smmu_cmdq_issue_sync(), while
CMDQ is still enabled, and update the comment/commit message accordingly.

Best regards,
Mykola

>
> > +
> > + /* Disable everything */
> > + ret = arm_smmu_device_disable(smmu);
> > + if ( ret )
> > + goto fail;
> > +
> > + dev_dbg(smmu->dev, "Suspended smmu\n");
> > + }
> > +
> > + return 0;
> > +
> > + fail:
> > + /* Reset the device that failed as well as any already-suspended ones. */
> > + arm_smmu_reset_for_suspend_rollback(smmu);
> > +
> > + list_for_each_entry_continue_reverse(smmu, &arm_smmu_devices, devices)
> > + arm_smmu_reset_for_suspend_rollback(smmu);
> > +
> > + return ret;
> > +}
> > +
>
> Cheers,
> Luca
>
>