On 02.09.25 01:10, Mykola Kvach wrote:
Hello Mykola
> From: Oleksandr Tyshchenko <oleksandr_tyshche...@epam.com>
>
> Store and restore active context and micro-TLB registers.
>
> Tested on R-Car H3 Starter Kit.
>
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshche...@epam.com>
> Signed-off-by: Mykola Kvach <mykola_kv...@epam.com>
> ---
> Changes in V6:
> - refactor code related to hw_register struct, from now it's called
> ipmmu_reg_ctx
The updated version looks good, thanks. However, I have one
concern/request ...
> ---
> xen/drivers/passthrough/arm/ipmmu-vmsa.c | 257 +++++++++++++++++++++++
> 1 file changed, 257 insertions(+)
>
> diff --git a/xen/drivers/passthrough/arm/ipmmu-vmsa.c
> b/xen/drivers/passthrough/arm/ipmmu-vmsa.c
> index ea9fa9ddf3..0973559861 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,222 @@ 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];
> +
> + 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);
> +}
> +
> +/*
> + * 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 )
> + {
> + uint32_t reg;
> +
> + /* Do not use security group function */
> + reg = IMSCTLR + mmu->features->control_offset_base;
> + ipmmu_write(mmu, reg, ipmmu_read(mmu, reg) & ~IMSCTLR_USE_SECGRP);
> +
> + if ( ipmmu_is_root(mmu) )
> + {
> + unsigned int i;
> +
> + /* Use stage 2 translation table format */
> + reg = IMSAUXCTLR + mmu->features->control_offset_base;
> + ipmmu_write(mmu, reg, ipmmu_read(mmu, reg) | IMSAUXCTLR_S2PTE);
> +
> + for ( i = 0; i < mmu->num_ctx; i++ )
> + {
> + if ( !mmu->domains[i] )
> + continue;
> + ipmmu_domain_restore_context(mmu->domains[i]);
> + }
> + }
> + else
> + ipmmu_utlbs_restore(mmu);
> + }
> +
> + spin_unlock(&ipmmu_devices_lock);
> +}
> +
> +static int ipmmu_alloc_ctx_suspend(struct device *dev)
> +{
> + struct ipmmu_vmsa_backup *backup_data;
> + unsigned int *utlbs_val, *asids_val;
> + struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> +
> + utlbs_val = xzalloc_array(unsigned int, fwspec->num_ids);
> + if ( !utlbs_val )
> + return -ENOMEM;
> +
> + asids_val = xzalloc_array(unsigned int, fwspec->num_ids);
> + if ( !asids_val )
> + {
> + xfree(utlbs_val);
> + return -ENOMEM;
> + }
> +
> + backup_data = xzalloc(struct ipmmu_vmsa_backup);
> + if ( !backup_data )
> + {
> + xfree(utlbs_val);
> + xfree(asids_val);
> + return -ENOMEM;
> + }
> +
> + backup_data->dev = dev;
> + backup_data->utlbs_val = utlbs_val;
> + backup_data->asids_val = asids_val;
> +
> + spin_lock(&ipmmu_devices_backup_lock);
> + list_add(&backup_data->list, &ipmmu_devices_backup);
> + spin_unlock(&ipmmu_devices_backup_lock);
> +
> + return 0;
> +}
> +
> +#endif /* CONFIG_SYSTEM_SUSPEND */
> +
> static int ipmmu_domain_init_context(struct ipmmu_vmsa_domain *domain)
> {
> uint64_t ttbr;
> @@ -559,6 +798,9 @@ static int ipmmu_domain_init_context(struct
> ipmmu_vmsa_domain *domain)
> return ret;
>
> domain->context_id = ret;
> +#ifdef CONFIG_SYSTEM_SUSPEND
> + domain->mmu->root->reg_backup[ret] = &root_pgtable[ret];
> +#endif
>
> /*
> * TTBR0
> @@ -615,6 +857,9 @@ static void ipmmu_domain_destroy_context(struct
> ipmmu_vmsa_domain *domain)
> ipmmu_ctx_write_root(domain, IMCTR, IMCTR_FLUSH);
> ipmmu_tlb_sync(domain);
>
> +#ifdef CONFIG_SYSTEM_SUSPEND
> + domain->mmu->root->reg_backup[domain->context_id] = NULL;
> +#endif
> ipmmu_domain_free_context(domain->mmu->root, domain->context_id);
> }
>
> @@ -1427,6 +1672,14 @@ static int ipmmu_add_device(u8 devfn, struct device
> *dev)
> }
> #endif
>
> +#ifdef CONFIG_SYSTEM_SUSPEND
> + if ( ipmmu_alloc_ctx_suspend(dev) )
> + {
> + dev_err(dev, "Failed to allocate context for suspend\n");
> + return -ENOMEM;
> + }
> +#endif
... The initial version was based on the driver code without PCI
support, but it is now present. There is PCI-specific code above in this
function (not visible in the context) that performs some initialization,
allocation and device assignment. What I mean is that in case of the
suspend context allocation error here, we will need to undo these
actions (i.e. deassign device). I would move this context allocation
(whose probability to fail is much lower than what is done for PCI dev)
above the PCI-specific stuff, and perform the context freeing on the
error path.
> +
> dev_info(dev, "Added master device (IPMMU %s micro-TLBs %u)\n",
> dev_name(fwspec->iommu_dev), fwspec->num_ids);
>
> @@ -1492,6 +1745,10 @@ static const struct iommu_ops ipmmu_iommu_ops =
> .unmap_page = arm_iommu_unmap_page,
> .dt_xlate = ipmmu_dt_xlate,
> .add_device = ipmmu_add_device,
> +#ifdef CONFIG_SYSTEM_SUSPEND
> + .suspend = ipmmu_suspend,
> + .resume = ipmmu_resume,
> +#endif
> };
>
> static __init int ipmmu_init(struct dt_device_node *node, const void *data)
