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

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Luca,

Thank you for the feedback.

On Mon, Jun 1, 2026 at 8:12 PM Luca Fancellu <Luca.Fancellu@xxxxxxx> wrote:
>
> Hi Mykola,
>
> > On 21 May 2026, at 18:45, Mykola Kvach <xakep.amatop@xxxxxxxxx> wrote:
> >
> > From: Mykola Kvach <mykola_kvach@xxxxxxxx>
> >
> > 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>
> > ---
> > 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 | 186 +++++++++++++++++++++-----
> > 1 file changed, 150 insertions(+), 36 deletions(-)
> >
> > diff --git a/xen/drivers/passthrough/arm/smmu-v3.c 
> > b/xen/drivers/passthrough/arm/smmu-v3.c
> > index bf153227db..be8028c036 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)
>
> Now this one and arm_smmu_device_reset loose __init also for 
> !CONFIG_SYSTEM_SUSPEND,
> but I’m not sure if in the codebase we are dealing with these kind of cases 
> already or if it’s still ok
> to let it be without __init anyway.

Good point. I will avoid keeping these helpers in runtime text for
!CONFIG_SYSTEM_SUSPEND.

I think a small local annotation in the SMMUv3 driver would work well here,
something like:

#ifdef CONFIG_SYSTEM_SUSPEND
#define __init_or_smmu_suspend
#else
#define __init_or_smmu_suspend __init
#endif

and then use it for arm_smmu_update_gbpa() and arm_smmu_device_reset().

This keeps the suspend-enabled case unchanged while preserving the existing
init-only placement otherwise.

What do you think?

Best regards,
Mykola