Re: [PATCH v5 12/12] xen/arm: Suspend/resume IOMMU on Xen suspend/resume

[Oleksandr Tyshchenko <olekstysh@xxxxxxxxx>]

On 26.08.25 16:42, Mykola Kvach wrote:

Hello Mykola, Volodymyr


> Hi Volodymyr,
>
> On Sat, Aug 23, 2025 at 8:55 PM Volodymyr Babchuk
> <Volodymyr_Babchuk@xxxxxxxx> wrote:
> > Hi,
> >
> > Mykola Kvach <xakep.amatop@xxxxxxxxx> writes:
> >
> > > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> > >
> > > This is done using generic iommu_suspend/resume functions that cause
> > > IOMMU driver specific suspend/resume handlers to be called for enabled
> > > IOMMU (if one has suspend/resume driver handlers implemented).
> > >
> > > These handlers work only when IPMMU-VMSA is used; otherwise,
> > > we return an error during system suspend requests.
> > >
> > > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> > > Signed-off-by: Mykola Kvach <mykola_kvach@xxxxxxxx>
> > > Tested-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
I think, the Tested-by here should be dropped. A lot of time has passed 
since Oleksandr provided the tag, and the code has changed a bit (I am 
afraid, the tag is now stale).

> > > ---
> > >   xen/arch/arm/suspend.c | 21 +++++++++++++++++++++
> > >   1 file changed, 21 insertions(+)
> > >
> > > diff --git a/xen/arch/arm/suspend.c b/xen/arch/arm/suspend.c
> > > index b5398e5ca6..cb86426ebd 100644
> > > --- a/xen/arch/arm/suspend.c
> > > +++ b/xen/arch/arm/suspend.c
> > > @@ -4,6 +4,7 @@
> > >   #include <asm/suspend.h>
> > >   #include <xen/console.h>
> > >   #include <xen/cpu.h>
> > > +#include <xen/iommu.h>
> > >   #include <xen/llc-coloring.h>
> > >   #include <xen/sched.h>
> > >
> > > @@ -49,6 +50,13 @@ static long system_suspend(void *data)
> > >
> > >       time_suspend();
> > >
> > > +    status = iommu_suspend();
> > > +    if ( status )
> > > +    {
> > > +        system_state = SYS_STATE_resume;
> > > +        goto resume_time;
> > > +    }
> > > +
> > >       local_irq_save(flags);
> > >       status = gic_suspend();
> > >       if ( status )
> > > @@ -105,6 +113,10 @@ static long system_suspend(void *data)
> > >
> > >    resume_irqs:
> > >       local_irq_restore(flags);
> > > +
> > > +    iommu_resume();
> > > +
> > > + resume_time:
> > >       time_resume();
> > >
> > >    resume_nonboot_cpus:
> > > @@ -140,6 +152,15 @@ int host_system_suspend(void)
> > >           return -ENOSYS;
> > >       }
> > >
> > > +    /* TODO: drop check once suspend/resume support for SMMU is implemented */
> > > +#ifndef CONFIG_IPMMU_VMSA
The original patch did not seem to have this check...

> > This check is meaningless, as you can have CONFIG_IPMMU_VMSA enabled
> > along with CONFIG_ARM_SMMU on a system with SMMU. I think it is enough
> > that iommu_suspend() will fail during system_suspend(), isn't it?
> Not exactly. In the case of SMMU, we don’t have valid iommu_suspend/iommu_resume
> handlers. So it’s possible to have CONFIG_ARM_SMMU enabled and iommu_enabled
> set, but without the appropriate suspend handlers. This could lead to
> a crash during
> system_suspend().
 ... I also have doubts whether this check is needed (at least in its 
current shape). Xen has 2 SMMU drivers at the moment. Lets imagine that 
S2R for SMMUv3 is implemented, so we will need to extend #ifndef above 
to cover CONFIG_ARM_SMMU_V3 as well, right (otherwise an attempt to 
suspend SMMUv2-powered system will lead to crash)? Or there is a plan to 
implement S2R for both SMMU implementations?
***

If we care for possible crash because iommu_suspend is NULL for 
SMMUv2/SMMUv3, maybe we should consider adding stub callbacks to the 
both SMMU drivers, just returning -ENOSYS?
Let's see what other people's opinions are.

> > > +    if ( iommu_enabled )
> > > +    {
> > > +        dprintk(XENLOG_ERR, "IOMMU is enabled, suspend not supported yet\n");
> > > +        return -ENOSYS;
> > > +    }
> > > +#endif
> > > +
> > >       /*
> > >        * system_suspend should be called when Dom0 finalizes the suspend
> > >        * procedure from its boot core (VCPU#0). However, Dom0's VCPU#0 could
> > --
> > WBR, Volodymyr
> Best regards,
> Mykola