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

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Bertrand,

Thank you for the review.

On Mon, Sep 28, 2026 at 7:18 PM Bertrand Marquis
<Bertrand.Marquis@xxxxxxx> wrote:
>
> Hi Mykola,
>
> > On 27 Aug 2026, at 16:31, Mykola Kvach <mykola_kvach@xxxxxxxx> wrote:
> >
> > Add system suspend/resume callbacks for the Arm SMMUv3 driver.
> >
> > During suspend, configure GBPA to abort incoming transactions, disable the
> > translation interface while keeping CMDQ enabled, issue CMD_SYNC to ensure
> > all previously issued commands have completed, then disable the SMMU IRQs
> > and SMMU.
> >
> > Resume uses arm_smmu_device_reset() to reprogram the SMMU and re-enable
> > translation and interrupt generation.
> >
> > The IRQ setup split follows the approach from Pranjal Shrivastava's Linux
> > arm-smmu-v3 runtime/system sleep series: IRQ handlers are requested once
> > during probe, while reset/resume only restores SMMU hardware state and
> > re-enables IRQ_CTRL.
> >
> > Only the pieces relevant to Xen's currently supported SMMUv3 path are
> > ported here. Xen documents SMMUv3 MSI and PCI ATS as unsupported and not
> > compiled/tested, so this patch does not restore SMMU MSI IRQ_CFGn registers
> > nor reinitialize ATS/PRI endpoints. If those paths become usable,
> > suspend/resume will need corresponding MSI restore and ATS/PRI
> > quiesce/reinit steps.
> >
> > Link: https://lore.kernel.org/r/20260414194702.1229094-1-praan@xxxxxxxxxx/
> > Based-on-patch-by: Pranjal Shrivastava <praan@xxxxxxxxxx>
> > Signed-off-by: Mykola Kvach <mykola_kvach@xxxxxxxx>
> > Reviewed-by: Luca Fancellu <luca.fancellu@xxxxxxx>
> > ---
> > Changes in V11:
> > - Keep arm_smmu_update_gbpa() and arm_smmu_device_reset() in init text when
> >  CONFIG_SYSTEM_SUSPEND is disabled.
> >
> > Changes in V10:
> > - Disable SMMU interrupt generation during suspend before disabling the
> >  SMMU interface, matching the resume/reset path which re-enables IRQ_CTRL.
> >
> > Changes in V9:
> > - Use CMD_SYNC in suspend instead of polling CMDQ_CONS, so the suspend
> >  path waits for command completion rather than only command consumption.
> > - Document that arm_smmu_setup_irqs() is probe-only and that future Xen
> >  SMMUv3 MSI support will need to restore SMMU IRQ_CFGn registers on
> >  resume.
> > - Restore the reference to Pranjal's Linux runtime/system sleep series and
> >  clarify that MSI/ATS/PRI resume handling is outside the supported Xen
> >  path.
> > - Prefix the subject with xen/arm for consistency with the rest of the
> >  Arm suspend/resume series.
> >
> > Changes in V8:
> > - Honor ARM_SMMU_FEAT_SEV when draining the CMDQ during suspend, matching
> >  the existing runtime CMD_SYNC path.
> > - Fold the suspend rollback reset path into a helper and rename the error
> >  reporting to describe suspend rollback rather than resume.
> > - Treat SMMU reset failure during resume as fatal instead of logging and
> >  continuing with a potentially unusable IOMMU.
> > - cosmetic changes
> > ---
> > xen/drivers/passthrough/arm/smmu-v3.c | 194 +++++++++++++++++++++-----
> > 1 file changed, 158 insertions(+), 36 deletions(-)
> >
> > diff --git a/xen/drivers/passthrough/arm/smmu-v3.c 
> > b/xen/drivers/passthrough/arm/smmu-v3.c
> > index bf153227db..7f1d00fb81 100644
> > --- a/xen/drivers/passthrough/arm/smmu-v3.c
> > +++ b/xen/drivers/passthrough/arm/smmu-v3.c
> > @@ -94,6 +94,12 @@
> >
> > #include "smmu-v3.h"
> >
> > +#ifdef CONFIG_SYSTEM_SUSPEND
> > +#define __init_or_smmu_suspend
> > +#else
> > +#define __init_or_smmu_suspend __init
> > +#endif
> > +
> > #define ARM_SMMU_VTCR_SH_IS 3
> > #define ARM_SMMU_VTCR_RGN_WBWA 1
> > #define ARM_SMMU_VTCR_TG0_4K 0
> > @@ -1814,8 +1820,8 @@ 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 __init_or_smmu_suspend
> > +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 +2001,35 @@ 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;
> > +}
> > +
> > +/*
> > + * Probe-time only: request host IRQs and, when available, program the 
> > SMMU's
> > + * MSI doorbells. Resume does not restore the SMMU *_IRQ_CFGn MSI 
> > registers,
> > + * so any host suspend support must treat the active MSI IRQ path as
> > + * unsupported until that restore path exists.
> > + */
> > 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 +2059,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 +2073,8 @@ 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 __init_or_smmu_suspend
> > +arm_smmu_device_reset(struct arm_smmu_device *smmu)
> > {
> > int ret;
> > u32 reg, enables;
> > @@ -2163,17 +2180,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");
> > + ret = arm_smmu_enable_irqs(smmu);
> > + if ( ret )
> > 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 +2190,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 +2563,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 +2593,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 +2875,104 @@ 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);
>
> If reset fails here, we only print an error.
>
> Could the SMMU be left disabled with GBPA.ABORT cleared, allowing guest
> DMA to bypass translation when the domains resume?

In Xen, disable_bypass is always true, so reset does not clear
GBPA.ABORT. After a successful ABORT update, the bit stays set
during rollback. If reset then fails with the SMMU disabled,
transactions will still be aborted.

Still, logging a rollback error is not enough. After suspend
fails, the host can let domains run again, so rollback must
restore the SMMU.

In the next version, if the first GBPA update times out, I will
leave the current SMMU enabled and restore only the previously
suspended ones.

The GBPA update may still complete later. This is safe here:
GBPA.ABORT controls transactions only when CR0.SMMUEN is clear
(IHI0070G.b, section 6.3.14). We have not written CR0 at this
point, so translation and queues remain enabled even if the
GBPA update completes after the timeout.

If a later suspend step fails, I will try to restore both the
current SMMU and the previously suspended ones. If any rollback
reset fails, I will call panic(), as we already do when reset
fails during resume.

I will also check the GBPA and both CMD_SYNC return values in
reset, so these errors are passed back to the caller.

A suspend failure will still be recoverable if rollback succeeds.

>
>
> > +}
> > +
> > +static int arm_smmu_suspend(void)
> > +{
> > + struct arm_smmu_device *smmu;
> > + int ret = 0;
> > +
> > + list_for_each_entry(smmu, &arm_smmu_devices, devices)
> > + {
> > + /* Abort all transactions before disable to avoid spurious bypass */
> > + ret = arm_smmu_update_gbpa(smmu, GBPA_ABORT, 0);
> > + if ( ret )
> > + goto fail;
> > +
> > + ret = arm_smmu_write_reg_sync(smmu, 0, ARM_SMMU_IRQ_CTRL,
> > + ARM_SMMU_IRQ_CTRLACK);
> > + if ( ret )
> > + {
> > + dev_err(smmu->dev, "Timed-out while disabling SMMU irqs\n");
> > + 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 translation interface is disabled and the
> > + * SMMU won't access translation/config structures, even
> > + * speculatively, as per the IHI0070 spec (section 6.3.9.6).
> > + * CMDQ is still enabled so that a CMD_SYNC can complete any
> > + * previously issued commands.
> > + */
> > +
> > + /* Ensure all previously issued commands have completed. */
> > + ret = arm_smmu_cmdq_issue_sync(smmu);
> > + if ( ret )
> > + {
> > + dev_err(smmu->dev, "Timed-out waiting for pending commands\n");
> > + goto fail;
> > + }
>
> Could we lose EVTQ events here because we do not check the queue after
> stopping it?

Yes, the old code could lose unread EVTQ entries. The hardware
producer index may be ahead of our cached value. Reset would
then restore the old index and hide those entries.

I will save the producer index after the queue is disabled and
CR0ACK confirms that the change is complete. The queue memory
and consumer index will be kept across suspend.

I will also cover rollback after a timeout while stopping the
queue. I will wait again for the stop acknowledgement, then save
the producer index before reset overwrites it. If this wait also
times out, I will call panic().

After reset, I will schedule the EVTQ tasklet if unread entries
remain. Re-enabling interrupts does not report old events again
(IHI0070G.b, section 6.3.16).

This preserves entries already recorded in the queue. Section
6.3.9.4 allows uncommitted events from terminated faulting
transactions to be discarded when the queue is disabled.

Best regards,
Mykola