[v5] Add initial Xen Suspend-to-RAM support on ARM64

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

Posted by Mykola Kvach 2 months, 2 weeks ago

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

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@epam.com>
Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
Tested-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
---
 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
+    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
-- 
2.48.1

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

Posted by Volodymyr Babchuk 2 months, 1 week ago

Hi,

Mykola Kvach <xakep.amatop@gmail.com> writes:

> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>
> 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@epam.com>
> Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
> Tested-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> ---
>  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?


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

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

Posted by Mykola Kvach 2 months ago

Hi Volodymyr,

On Sat, Aug 23, 2025 at 8:55 PM Volodymyr Babchuk
<Volodymyr_Babchuk@epam.com> wrote:
>
> Hi,
>
> Mykola Kvach <xakep.amatop@gmail.com> writes:
>
> > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> >
> > 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@epam.com>
> > Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
> > Tested-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> > ---
> >  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

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

Posted by Oleksandr Tyshchenko 2 months ago


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@epam.com> wrote:
>>
>> Hi,
>>
>> Mykola Kvach <xakep.amatop@gmail.com> writes:
>>
>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>
>>> 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@epam.com>
>>> Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
>>> Tested-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

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
>

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

Posted by Mykola Kvach 2 months ago

Hi Oleksandr,

On Tue, Aug 26, 2025 at 6:01 PM Oleksandr Tyshchenko
<olekstysh@gmail.com> 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@epam.com> wrote:
> >>
> >> Hi,
> >>
> >> Mykola Kvach <xakep.amatop@gmail.com> writes:
> >>
> >>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> >>>
> >>> 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@epam.com>
> >>> Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
> >>> Tested-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>
> 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