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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

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 `=`

> +
> +    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.

Cheers,
Luca