Re: [PATCH v9 08/13] iommu/ipmmu-vmsa: Implement suspend/resume callbacks

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Luca,

Thank you for the review.

On Thu, May 14, 2026 at 6:58 PM Luca Fancellu <Luca.Fancellu@xxxxxxx> wrote:
>
> Hi Mykola,
>
> >
> > diff --git a/xen/drivers/passthrough/arm/ipmmu-vmsa.c 
> > b/xen/drivers/passthrough/arm/ipmmu-vmsa.c
> > index fa9ab9cb13..e1b47a5824 100644
> > --- a/xen/drivers/passthrough/arm/ipmmu-vmsa.c
> > +++ b/xen/drivers/passthrough/arm/ipmmu-vmsa.c
> > @@ -71,6 +71,8 @@
> > })
> > #endif
> >
> > +#define dev_dbg(dev, fmt, ...)    \
> > +    dev_print(dev, XENLOG_DEBUG, fmt, ## __VA_ARGS__)
> > #define dev_info(dev, fmt, ...)    \
> >     dev_print(dev, XENLOG_INFO, fmt, ## __VA_ARGS__)
> > #define dev_warn(dev, fmt, ...)    \
> > @@ -130,6 +132,24 @@ struct ipmmu_features {
> >     unsigned int imuctr_ttsel_mask;
> > };
> >
> > +#ifdef CONFIG_SYSTEM_SUSPEND
> > +
> > +struct ipmmu_reg_ctx {
> > +    unsigned int imttlbr0;
> > +    unsigned int imttubr0;
> > +    unsigned int imttbcr;
> > +    unsigned int imctr;
> > +};
> > +
> > +struct ipmmu_vmsa_backup {
> > +    struct device *dev;
> > +    unsigned int *utlbs_val;
> > +    unsigned int *asids_val;
> > +    struct list_head list;
> > +};
> > +
> > +#endif
> > +
> > /* Root/Cache IPMMU device's information */
> > struct ipmmu_vmsa_device {
> >     struct device *dev;
> > @@ -142,6 +162,9 @@ struct ipmmu_vmsa_device {
> >     struct ipmmu_vmsa_domain *domains[IPMMU_CTX_MAX];
> >     unsigned int utlb_refcount[IPMMU_UTLB_MAX];
> >     const struct ipmmu_features *features;
> > +#ifdef CONFIG_SYSTEM_SUSPEND
> > +    struct ipmmu_reg_ctx *reg_backup[IPMMU_CTX_MAX];
> > +#endif
> > };
> >
> > /*
> > @@ -547,6 +570,245 @@ static void ipmmu_domain_free_context(struct 
> > ipmmu_vmsa_device *mmu,
> >     spin_unlock_irqrestore(&mmu->lock, flags);
> > }
> >
> > +#ifdef CONFIG_SYSTEM_SUSPEND
> > +
> > +static DEFINE_SPINLOCK(ipmmu_devices_backup_lock);
> > +static LIST_HEAD(ipmmu_devices_backup);
> > +
> > +static struct ipmmu_reg_ctx root_pgtable[IPMMU_CTX_MAX];
> > +
> > +static uint32_t ipmmu_imuasid_read(struct ipmmu_vmsa_device *mmu,
> > +                                   unsigned int utlb)
> > +{
> > +    return ipmmu_read(mmu, ipmmu_utlb_reg(mmu, IMUASID(utlb)));
> > +}
> > +
> > +static void ipmmu_utlbs_backup(struct ipmmu_vmsa_device *mmu)
> > +{
> > +    struct ipmmu_vmsa_backup *backup_data;
> > +
> > +    dev_dbg(mmu->dev, "Handle micro-TLBs backup\n");
> > +
> > +    spin_lock(&ipmmu_devices_backup_lock);
> > +
> > +    list_for_each_entry( backup_data, &ipmmu_devices_backup, list )
> > +    {
> > +        struct iommu_fwspec *fwspec = 
> > dev_iommu_fwspec_get(backup_data->dev);
> > +        unsigned int i;
> > +
> > +        if ( to_ipmmu(backup_data->dev) != mmu )
> > +            continue;
> > +
> > +        for ( i = 0; i < fwspec->num_ids; i++ )
> > +        {
> > +            unsigned int utlb = fwspec->ids[i];
> > +
> > +            backup_data->asids_val[i] = ipmmu_imuasid_read(mmu, utlb);
> > +            backup_data->utlbs_val[i] = ipmmu_imuctr_read(mmu, utlb);
> > +        }
> > +    }
> > +
> > +    spin_unlock(&ipmmu_devices_backup_lock);
> > +}
> > +
> > +static void ipmmu_utlbs_restore(struct ipmmu_vmsa_device *mmu)
> > +{
> > +    struct ipmmu_vmsa_backup *backup_data;
> > +
> > +    dev_dbg(mmu->dev, "Handle micro-TLBs restore\n");
> > +
> > +    spin_lock(&ipmmu_devices_backup_lock);
> > +
> > +    list_for_each_entry( backup_data, &ipmmu_devices_backup, list )
> > +    {
> > +        struct iommu_fwspec *fwspec = 
> > dev_iommu_fwspec_get(backup_data->dev);
> > +        unsigned int i;
> > +
> > +        if ( to_ipmmu(backup_data->dev) != mmu )
> > +            continue;
> > +
> > +        for ( i = 0; i < fwspec->num_ids; i++ )
> > +        {
> > +            unsigned int utlb = fwspec->ids[i];
> > +
> > +            ipmmu_imuasid_write(mmu, utlb, backup_data->asids_val[i]);
> > +            ipmmu_imuctr_write(mmu, utlb, backup_data->utlbs_val[i]);
> > +        }
> > +    }
> > +
> > +    spin_unlock(&ipmmu_devices_backup_lock);
> > +}
> > +
> > +static void ipmmu_domain_backup_context(struct ipmmu_vmsa_domain *domain)
> > +{
> > +    struct ipmmu_vmsa_device *mmu = domain->mmu->root;
> > +    struct ipmmu_reg_ctx *regs = mmu->reg_backup[domain->context_id];
> > +
> > +    dev_dbg(mmu->dev, "Handle domain context %u backup\n", 
> > domain->context_id);
> > +
> > +    regs->imttlbr0 = ipmmu_ctx_read_root(domain, IMTTLBR0);
> > +    regs->imttubr0 = ipmmu_ctx_read_root(domain, IMTTUBR0);
> > +    regs->imttbcr  = ipmmu_ctx_read_root(domain, IMTTBCR);
> > +    regs->imctr    = ipmmu_ctx_read_root(domain, IMCTR);
> > +}
> > +
> > +static void ipmmu_domain_restore_context(struct ipmmu_vmsa_domain *domain)
> > +{
> > +    struct ipmmu_vmsa_device *mmu = domain->mmu->root;
> > +    struct ipmmu_reg_ctx *regs  = mmu->reg_backup[domain->context_id];
>
> NIT: There is a double space before the `=`

Ack.

>
> > +
> > +    dev_dbg(mmu->dev, "Handle domain context %u restore\n", 
> > domain->context_id);
> > +
> > +    ipmmu_ctx_write_root(domain, IMTTLBR0, regs->imttlbr0);
> > +    ipmmu_ctx_write_root(domain, IMTTUBR0, regs->imttubr0);
> > +    ipmmu_ctx_write_root(domain, IMTTBCR,  regs->imttbcr);
> > +    ipmmu_ctx_write_all(domain,  IMCTR,    regs->imctr | IMCTR_FLUSH);
>
> I see in ipmmu_tlb_invalidate() we do:
> dsb(sy);
> ipmmu_tlb_sync(domain);
>
> Is it safe to omit them here?
>
> > +}
> > +
> > +/*
> > + * Xen: Unlike Linux implementation, Xen uses a single driver instance
> > + * for handling all IPMMUs. There is no framework for ipmmu_suspend/resume
> > + * callbacks to be invoked for each IPMMU device. So, we need to iterate
> > + * through all registered IPMMUs performing required actions.
> > + *
> > + * Also take care of restoring special settings, such as translation
> > + * table format, etc.
> > + */
> > +static int __must_check ipmmu_suspend(void)
> > +{
> > +    struct ipmmu_vmsa_device *mmu;
> > +
> > +    if ( !iommu_enabled )
> > +        return 0;
> > +
> > +    printk(XENLOG_DEBUG "ipmmu: Suspending...\n");
> > +
> > +    spin_lock(&ipmmu_devices_lock);
> > +
> > +    list_for_each_entry( mmu, &ipmmu_devices, list )
> > +    {
> > +        if ( ipmmu_is_root(mmu) )
> > +        {
> > +            unsigned int i;
> > +
> > +            for ( i = 0; i < mmu->num_ctx; i++ )
> > +            {
> > +                if ( !mmu->domains[i] )
> > +                    continue;
> > +                ipmmu_domain_backup_context(mmu->domains[i]);
> > +            }
> > +        }
> > +        else
> > +            ipmmu_utlbs_backup(mmu);
> > +    }
> > +
> > +    spin_unlock(&ipmmu_devices_lock);
> > +
> > +    return 0;
> > +}
> > +
> > +static void ipmmu_resume(void)
> > +{
> > +    struct ipmmu_vmsa_device *mmu;
> > +
> > +    if ( !iommu_enabled )
> > +        return;
> > +
> > +    printk(XENLOG_DEBUG "ipmmu: Resuming...\n");
> > +
> > +    spin_lock(&ipmmu_devices_lock);
> > +
> > +    list_for_each_entry( mmu, &ipmmu_devices, list )
>
> This loop has an ordering problem because we can run ipmmu_utlbs_restore() 
> before
> the root ipmmu is restored (ipmmu_probe() uses `list_add()`).
> Maybe going twice on the list, restoring first the root and in the second 
> round the rest
> should work.

Ack, good point.

I will split the resume path into two passes: restore the Root IPMMU context
state first, then restore the micro-TLB state on Cache IPMMUs afterwards.

Best regards,
Mykola