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

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Oleksandr,

On Tue, Aug 26, 2025 at 6:01 PM Oleksandr Tyshchenko
<olekstysh@xxxxxxxxx> wrote:
>
>
>
> 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).

Got it, I’ll drop the Tested-by tag in the next version.

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

I’m fine with that proposal, adding stub callbacks sounds like a clean
solution.

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

Best regards,
Mykola