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

[Bertrand Marquis <Bertrand.Marquis@arm.com>]

Hi Mykola,

> On 27 Aug 2026, at 16:31, Mykola Kvach <mykola_kvach@epam.com> 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@google.com/
> Based-on-patch-by: Pranjal Shrivastava <praan@google.com>
> Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
> Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
> ---
> 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?


> +}
> +
> +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?

Cheers
Bertrand

> +
> + /* 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;
> +}
> +
> +static void arm_smmu_resume(void)
> +{
> + int ret;
> + struct arm_smmu_device *smmu;
> +
> + list_for_each_entry(smmu, &arm_smmu_devices, devices)
> + {
> + dev_dbg(smmu->dev, "Resuming device\n");
> +
> + /*
> + * The reset will re-initialize all the base addresses, queues,
> + * prod and cons maintained within struct arm_smmu_device as well as
> + * re-enable the interrupts.
> + */
> + ret = arm_smmu_device_reset(smmu);
> + if ( ret )
> + panic("SMMUv3: %s: Failed to reset during resume: %d\n",
> +      dev_name(smmu->dev), ret);
> + }
> +}
> +#endif
> +
> static const struct iommu_ops arm_smmu_iommu_ops = {
> .page_sizes = PAGE_SIZE_4K,
> .init = arm_smmu_iommu_xen_domain_init,
> @@ -2867,6 +2985,10 @@ static const struct iommu_ops arm_smmu_iommu_ops = {
> .unmap_page = arm_iommu_unmap_page,
> .dt_xlate = arm_smmu_dt_xlate,
> .add_device = arm_smmu_add_device,
> +#ifdef CONFIG_SYSTEM_SUSPEND
> + .suspend = arm_smmu_suspend,
> + .resume = arm_smmu_resume,
> +#endif
> };
> 
> static __init int arm_smmu_dt_init(struct dt_device_node *dev,
> -- 
> 2.43.0
>