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

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

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>
> > ---
> >  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
>
> 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().


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