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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Mykola,

> On 28 Sep 2026, at 10:01, Mykola Kvach <xakep.amatop@xxxxxxxxx> wrote:
> 
> Hi all,
> 
> I have one more thing to check here regarding the R-Car Gen4 PCIe
> BDF-to-OSID mappings across system suspend/resume.
> 
> The mappings are programmed in the PCIe APP CNVID/CNVIDMSK registers,
> while CNVOSIDCTRL provides the default OSID. If this state is lost
> during SYSTEM_SUSPEND, PCI DMA could potentially use the default OSID
> after resume instead of the OSID assigned to the device.
> 
> I also found a recent Linux rcar-gen4 PCIe PM patch which notes that
> S4 and V4H use an always-on PCIe power domain, while on V4M the PCIe
> controller loses state during suspend. However, I have not found an
> explicit guarantee that these particular APP registers are retained
> on S4.
> 
> The Linux BSP is not a direct reference for this case, as its current
> R-Car S4 IPMMU setup does not use the per-BDF OSID mappings programmed
> through these registers.
> 
> This series has not previously been tested on R-Car S4 Spider. I have
> tested the suspend/resume path on several other platforms (R-Car H3ULCB,
> R-Car Gen5 and Orange Pi 5), but not this particular PCIe state.
> 
> I will also try to check this on a real R-Car S4 board by comparing
> the PCIe APP register state before and after SYSTEM_SUSPEND.
> 
> So please consider this patch as needing some further investigation
> from my side for now.

Thanks for the information.

I will skip this patch for now (and anyway i will need some help on it as IPMMU 
and
in general Renesas drivers are not really something i know).

Cheers
Bertrand

> 
> Thanks,
> Mykola
> 
> On Thu, Aug 27, 2026 at 6:45 PM Mykola Kvach <mykola_kvach@xxxxxxxx> wrote:
>> 
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
>> 
>> Store and restore active context and micro-TLB registers.
>> 
>> On resume, restore Root IPMMU context state before restoring Cache IPMMU
>> micro-TLB state. Cache IPMMUs select Root contexts through their micro-TLB
>> configuration, so restoring Cache micro-TLBs before the Root context
>> registers are restored can expose stale or uninitialized context state.
>> 
>> Tested on R-Car H3 Starter Kit.
>> 
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
>> Signed-off-by: Mykola Kvach <mykola_kvach@xxxxxxxx>
>> Reviewed-by: Luca Fancellu <luca.fancellu@xxxxxxx>
>> ---
>> Changes in V10:
>> - Iterate over registered IPMMUs in reverse order during resume so Root IPMMU
>>  context state is restored before Cache IPMMU micro-TLB state.
>> 
>> Changes in V9:
>> - set dt_device_set_protected() only after ipmmu_alloc_ctx_suspend()
>>  succeeds, so DT devices do not remain protected on allocation failure.
>> 
>> Changes in V7:
>> - moved suspend context allocation before pci stuff
>> ---
>> xen/drivers/passthrough/arm/ipmmu-vmsa.c | 323 +++++++++++++++++++++--
>> 1 file changed, 308 insertions(+), 15 deletions(-)
>> 
>> diff --git a/xen/drivers/passthrough/arm/ipmmu-vmsa.c 
>> b/xen/drivers/passthrough/arm/ipmmu-vmsa.c
>> index fa9ab9cb13..2e54fa63d6 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,249 @@ 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);
>> +
>> +    /*
>> +     * IPMMUs are registered with list_add(), with Root IPMMU probed first.
>> +     * Walk backwards to restore Root contexts before Cache micro-TLBs.
>> +     */
>> +    list_for_each_entry_reverse( 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;
>> +}
>> +
>> +#ifdef CONFIG_HAS_PCI
>> +static void ipmmu_free_ctx_suspend(struct device *dev)
>> +{
>> +    struct ipmmu_vmsa_backup *backup_data, *tmp;
>> +
>> +    spin_lock(&ipmmu_devices_backup_lock);
>> +
>> +    list_for_each_entry_safe( backup_data, tmp, &ipmmu_devices_backup, list 
>> )
>> +    {
>> +        if ( backup_data->dev == dev )
>> +        {
>> +            list_del(&backup_data->list);
>> +            xfree(backup_data->utlbs_val);
>> +            xfree(backup_data->asids_val);
>> +            xfree(backup_data);
>> +            break;
>> +        }
>> +    }
>> +
>> +    spin_unlock(&ipmmu_devices_backup_lock);
>> +}
>> +#endif /* CONFIG_HAS_PCI */
>> +
>> +#endif /* CONFIG_SYSTEM_SUSPEND */
>> +
>> static int ipmmu_domain_init_context(struct ipmmu_vmsa_domain *domain)
>> {
>>     uint64_t ttbr;
>> @@ -559,6 +825,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 +884,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);
>> }
>> 
>> @@ -1338,10 +1610,11 @@ static int ipmmu_add_device(u8 devfn, struct device 
>> *dev)
>>     struct iommu_fwspec *fwspec;
>> 
>> #ifdef CONFIG_HAS_PCI
>> +    int ret;
>> +
>>     if ( dev_is_pci(dev) )
>>     {
>>         struct pci_dev *pdev = dev_to_pci(dev);
>> -        int ret;
>> 
>>         if ( devfn != pdev->devfn )
>>             return 0;
>> @@ -1358,17 +1631,24 @@ static int ipmmu_add_device(u8 devfn, struct device 
>> *dev)
>>     if ( !to_ipmmu(dev) )
>>         return -ENODEV;
>> 
>> -    if ( !dev_is_pci(dev) )
>> +    if ( !dev_is_pci(dev) && dt_device_is_protected(dev_to_dt(dev)) )
>>     {
>> -        if ( dt_device_is_protected(dev_to_dt(dev)) )
>> -        {
>> -            dev_err(dev, "Already added to IPMMU\n");
>> -            return -EEXIST;
>> -        }
>> +        dev_err(dev, "Already added to IPMMU\n");
>> +        return -EEXIST;
>> +    }
>> 
>> -        /* Let Xen know that the master device is protected by an IOMMU. */
>> -        dt_device_set_protected(dev_to_dt(dev));
>> +#ifdef CONFIG_SYSTEM_SUSPEND
>> +    if ( ipmmu_alloc_ctx_suspend(dev) )
>> +    {
>> +        dev_err(dev, "Failed to allocate context for suspend\n");
>> +        return -ENOMEM;
>>     }
>> +#endif
>> +
>> +    /* Let Xen know that the master device is protected by an IOMMU. */
>> +    if ( !dev_is_pci(dev) )
>> +        dt_device_set_protected(dev_to_dt(dev));
>> +
>> #ifdef CONFIG_HAS_PCI
>>     if ( dev_is_pci(dev) )
>>     {
>> @@ -1377,26 +1657,28 @@ static int ipmmu_add_device(u8 devfn, struct device 
>> *dev)
>>         struct pci_host_bridge *bridge;
>>         struct iommu_fwspec *fwspec_bridge;
>>         unsigned int utlb_osid0 = 0;
>> -        int ret;
>> 
>>         bridge = pci_find_host_bridge(pdev->seg, pdev->bus);
>>         if ( !bridge )
>>         {
>>             dev_err(dev, "Failed to find host bridge\n");
>> -            return -ENODEV;
>> +            ret = -ENODEV;
>> +            goto free_suspend_ctx;
>>         }
>> 
>>         fwspec_bridge = dev_iommu_fwspec_get(dt_to_dev(bridge->dt_node));
>>         if ( fwspec_bridge->num_ids < 1 )
>>         {
>>             dev_err(dev, "Failed to find host bridge uTLB\n");
>> -            return -ENXIO;
>> +            ret = -ENXIO;
>> +            goto free_suspend_ctx;
>>         }
>> 
>>         if ( fwspec->num_ids < 1 )
>>         {
>>             dev_err(dev, "Failed to find uTLB");
>> -            return -ENXIO;
>> +            ret = -ENXIO;
>> +            goto free_suspend_ctx;
>>         }
>> 
>>         rcar4_pcie_osid_regs_init(bridge);
>> @@ -1405,7 +1687,7 @@ static int ipmmu_add_device(u8 devfn, struct device 
>> *dev)
>>         if ( ret < 0 )
>>         {
>>             dev_err(dev, "No unused OSID regs\n");
>> -            return ret;
>> +            goto free_suspend_ctx;
>>         }
>>         reg_id = ret;
>> 
>> @@ -1420,7 +1702,7 @@ static int ipmmu_add_device(u8 devfn, struct device 
>> *dev)
>>         {
>>             rcar4_pcie_osid_bdf_clear(bridge, reg_id);
>>             rcar4_pcie_osid_reg_free(bridge, reg_id);
>> -            return ret;
>> +            goto free_suspend_ctx;
>>         }
>>     }
>> #endif
>> @@ -1429,6 +1711,13 @@ static int ipmmu_add_device(u8 devfn, struct device 
>> *dev)
>>              dev_name(fwspec->iommu_dev), fwspec->num_ids);
>> 
>>     return 0;
>> +#ifdef CONFIG_HAS_PCI
>> + free_suspend_ctx:
>> +#ifdef CONFIG_SYSTEM_SUSPEND
>> +    ipmmu_free_ctx_suspend(dev);
>> +#endif
>> +    return ret;
>> +#endif
>> }
>> 
>> static int ipmmu_iommu_domain_init(struct domain *d)
>> @@ -1490,6 +1779,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)
>> --
>> 2.43.0
>> 
>>