Re: [PATCH v5 10/12] xen/arm: Save/restore context on suspend/resume

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Volodymyr,

On Sat, Aug 23, 2025 at 8:34 PM Volodymyr Babchuk
<Volodymyr_Babchuk@xxxxxxxx> wrote:
>
>
> Hi Mykola,
>
> Mykola Kvach <xakep.amatop@xxxxxxxxx> writes:
>
> > From: Mirela Simonovic <mirela.simonovic@xxxxxxxxxx>
> >
> > The context of CPU general purpose and system control registers
> > has to be saved on suspend and restored on resume. This is
> > implemented in hyp_suspend and before the return from hyp_resume
> > function. The hyp_suspend is invoked just before the PSCI system
> > suspend call is issued to the ATF. The hyp_suspend has to return a
> > non-zero value so that the calling 'if' statement evaluates to true,
> > causing the system suspend to be invoked. Upon the resume, context
> > saved on suspend will be restored, including the link register.
> > Therefore, after restoring the context the control flow will
> > return to the address pointed by the saved link register, which
> > is the place from which the hyp_suspend was called. To ensure
> > that the calling 'if' statement doesn't again evaluate to true
> > and initiate system suspend, hyp_resume has to return a zero value
> > after restoring the context.
> >
> > Note that the order of saving register context into cpu_context
> > structure has to match the order of restoring.
> >
> > Support for ARM32 is not implemented. Instead, compilation fails with a
> > build-time error if suspend is enabled for ARM32.
> >
> > Signed-off-by: Mirela Simonovic <mirela.simonovic@xxxxxxxxxx>
> > Signed-off-by: Saeed Nowshadi <saeed.nowshadi@xxxxxxxxxx>
> > Signed-off-by: Mykyta Poturai <mykyta_poturai@xxxxxxxx>
> > Signed-off-by: Mykola Kvach <mykola_kvach@xxxxxxxx>
> > ---
> > Changes in v4:
> > - produce build-time error for ARM32 when CONFIG_SYSTEM_SUSPEND is enabled
> > - use register_t instead of uint64_t in cpu_context structure
> > ---
> >  xen/arch/arm/arm64/head.S          | 91 +++++++++++++++++++++++++++++-
> >  xen/arch/arm/include/asm/suspend.h | 20 +++++++
> >  xen/arch/arm/suspend.c             | 23 +++++++-
> >  3 files changed, 130 insertions(+), 4 deletions(-)
> >
> > diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> > index 596e960152..ad8b48de3a 100644
> > --- a/xen/arch/arm/arm64/head.S
> > +++ b/xen/arch/arm/arm64/head.S
> > @@ -562,6 +562,52 @@ END(efi_xen_start)
> >  #endif /* CONFIG_ARM_EFI */
> >
> >  #ifdef CONFIG_SYSTEM_SUSPEND
> > +/*
> > + * int hyp_suspend(struct cpu_context *ptr)
> > + *
> > + * x0 - pointer to the storage where callee's context will be saved
> > + *
> > + * CPU context saved here will be restored on resume in hyp_resume 
> > function.
> > + * hyp_suspend shall return a non-zero value. Upon restoring context
> > + * hyp_resume shall return value zero instead. From C code that invokes
> > + * hyp_suspend, the return value is interpreted to determine whether the 
> > context
> > + * is saved (hyp_suspend) or restored (hyp_resume).
> > + */
> > +FUNC(hyp_suspend)
>
> I don't think that hyp_suspend is the correct name, as this function in
> fact suspend_nothing. Maybe "prepare_resume_ctx" will be better?

Not a problem to rename it.

>
> > +        /* Store callee-saved registers */
> > +        stp     x19, x20, [x0], #16
>
> If you have struct cpu_context defined, then you probably should use
> define provided by <asm-offsets.h> to access struct fields. Otherwise,
> it will be really easy to get desync between struct definition and this
> asm code.
>
> > +        stp     x21, x22, [x0], #16
> > +        stp     x23, x24, [x0], #16
> > +        stp     x25, x26, [x0], #16
> > +        stp     x27, x28, [x0], #16
> > +        stp     x29, lr, [x0], #16
> > +
> > +        /* Store stack-pointer */
> > +        mov     x2, sp
> > +        str     x2, [x0], #8
> > +
> > +        /* Store system control registers */
> > +        mrs     x2, VBAR_EL2
> > +        str     x2, [x0], #8
> > +        mrs     x2, VTCR_EL2
> > +        str     x2, [x0], #8
> > +        mrs     x2, VTTBR_EL2
> > +        str     x2, [x0], #8
> > +        mrs     x2, TPIDR_EL2
> > +        str     x2, [x0], #8
> > +        mrs     x2, MDCR_EL2
> > +        str     x2, [x0], #8
> > +        mrs     x2, HSTR_EL2
> > +        str     x2, [x0], #8
> > +        mrs     x2, CPTR_EL2
> > +        str     x2, [x0], #8
> > +        mrs     x2, HCR_EL2
> > +        str     x2, [x0], #8
> > +
> > +        /* hyp_suspend must return a non-zero value */
> > +        mov     x0, #1
> > +        ret
> > +END(hyp_suspend)
> >
> >  FUNC(hyp_resume)
> >          /* Initialize the UART if earlyprintk has been enabled. */
> > @@ -580,7 +626,50 @@ FUNC(hyp_resume)
> >          b     enable_secondary_cpu_mm
> >
> >  mmu_resumed:
> > -        b .
> > +        /*
> > +         * Now we can access the cpu_context, so restore the context here
> > +         * TODO: can we reuse __context_switch and saved_context struct 
> > here ?
> > +         */
>
> This is a great idea and I like it very much, but sadly saved_context
> struct has no fields for system _EL2 registers.
>
> > +        ldr     x0, =cpu_context
> > +
> > +        /* Restore callee-saved registers */
> > +        ldp     x19, x20, [x0], #16
> > +        ldp     x21, x22, [x0], #16
> > +        ldp     x23, x24, [x0], #16
> > +        ldp     x25, x26, [x0], #16
> > +        ldp     x27, x28, [x0], #16
> > +        ldp     x29, lr, [x0], #16
> > +
> > +        /* Restore stack pointer */
> > +        ldr     x2, [x0], #8
> > +        mov     sp, x2
> > +
> > +        /* Restore system control registers */
> > +        ldr     x2, [x0], #8
> > +        msr     VBAR_EL2, x2
> > +        ldr     x2, [x0], #8
> > +        msr     VTCR_EL2, x2
> > +        ldr     x2, [x0], #8
> > +        msr     VTTBR_EL2, x2
> > +        ldr     x2, [x0], #8
> > +        msr     TPIDR_EL2, x2
> > +        ldr     x2, [x0], #8
> > +        msr     MDCR_EL2, x2
> > +        ldr     x2, [x0], #8
> > +        msr     HSTR_EL2, x2
> > +        ldr     x2, [x0], #8
> > +        msr     CPTR_EL2, x2
> > +        ldr     x2, [x0], #8
> > +        msr     HCR_EL2, x2
> > +        isb
> > +
> > +        /* Since context is restored return from this function will appear 
> > as
> > +         * return from hyp_suspend. To distinguish a return from 
> > hyp_suspend
> > +         * which is called upon finalizing the suspend, as opposed to 
> > return
> > +         * from this function which executes on resume, we need to return 
> > zero
> > +         * value here. */
> > +        mov x0, #0
> > +        ret
> >  END(hyp_resume)
> >
> >  #endif /* CONFIG_SYSTEM_SUSPEND */
> > diff --git a/xen/arch/arm/include/asm/suspend.h 
> > b/xen/arch/arm/include/asm/suspend.h
> > index 55041a5d06..ae71ccb87b 100644
> > --- a/xen/arch/arm/include/asm/suspend.h
> > +++ b/xen/arch/arm/include/asm/suspend.h
> > @@ -5,9 +5,29 @@
> >
> >  #ifdef CONFIG_SYSTEM_SUSPEND
> >
> > +#ifdef CONFIG_ARM_64
> > +struct cpu_context {
> > +    register_t callee_regs[12];
> > +    register_t sp;
> > +    register_t vbar_el2;
> > +    register_t vtcr_el2;
> > +    register_t vttbr_el2;
> > +    register_t tpidr_el2;
> > +    register_t mdcr_el2;
> > +    register_t hstr_el2;
> > +    register_t cptr_el2;
> > +    register_t hcr_el2;
> > +} __aligned(16);
> > +#else
> > +#error "Define cpu_context structure for arm32"
> > +#endif
> > +
> > +extern struct cpu_context cpu_context;
> > +
> >  int host_system_suspend(void);
> >
> >  void hyp_resume(void);
> > +int hyp_suspend(struct cpu_context *ptr);
> >
> >  #endif /* CONFIG_SYSTEM_SUSPEND */
> >
> > diff --git a/xen/arch/arm/suspend.c b/xen/arch/arm/suspend.c
> > index 08b6acaede..b5398e5ca6 100644
> > --- a/xen/arch/arm/suspend.c
> > +++ b/xen/arch/arm/suspend.c
> > @@ -1,6 +1,7 @@
> >  /* SPDX-License-Identifier: GPL-2.0-only */
> >
> >  #include <asm/psci.h>
> > +#include <asm/suspend.h>
> >  #include <xen/console.h>
> >  #include <xen/cpu.h>
> >  #include <xen/llc-coloring.h>
> > @@ -17,6 +18,8 @@
> >   *  - Investigate feasibility and need for implementing system suspend on 
> > ARM32
> >   */
> >
> > +struct cpu_context cpu_context;
> > +
> >  /* Xen suspend. Note: data is not used (suspend is the suspend to RAM) */
> >  static long system_suspend(void *data)
> >  {
> > @@ -73,9 +76,23 @@ static long system_suspend(void *data)
> >       */
> >      update_boot_mapping(true);
> >
> > -    status = call_psci_system_suspend();
> > -    if ( status )
> > -        dprintk(XENLOG_WARNING, "PSCI system suspend failed, err=%d\n", 
> > status);
> > +    if ( hyp_suspend(&cpu_context) )
> > +    {
> > +        status = call_psci_system_suspend();
> > +        /*
> > +         * If suspend is finalized properly by above system suspend PSCI 
> > call,
> > +         * the code below in this 'if' branch will never execute. Execution
> > +         * will continue from hyp_resume which is the hypervisor's resume 
> > point.
> > +         * In hyp_resume CPU context will be restored and since 
> > link-register is
> > +         * restored as well, it will appear to return from hyp_suspend. The
> > +         * difference in returning from hyp_suspend on system suspend 
> > versus
> > +         * resume is in function's return value: on suspend, the return 
> > value is
> > +         * a non-zero value, on resume it is zero. That is why the control 
> > flow
> > +         * will not re-enter this 'if' branch on resume.
> > +         */
>
> Looks like this comment is misplaced. It should be before "if (
> hyp_suspend() )", right?

Actually, the comment is correctly placed here and does not need to be moved.

>
> > +        if ( status )
> > +            dprintk(XENLOG_WARNING, "PSCI system suspend failed, 
> > err=%d\n", status);
> > +    }
> >
> >      system_state = SYS_STATE_resume;
> >      update_boot_mapping(false);
>
> --
> WBR, Volodymyr

Best regards,
Mykola