Re: [PATCH v8 11/13] xen/arm: Save/restore context on suspend/resume

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Volodymyr,

Thanks for taking a look at this.

On Fri, May 8, 2026 at 1:17 AM Volodymyr Babchuk
<Volodymyr_Babchuk@xxxxxxxx> wrote:
>
> Hi Mikola,
>
> Mykola Kvach <xakep.amatop@xxxxxxxxx> writes:
>
> > From: Mirela Simonovic <mirela.simonovic@xxxxxxxxxx>
> >
> > The context of CPU general purpose and system control registers must be
> > saved on suspend and restored on resume. This is implemented in
> > prepare_resume_ctx and before the return from the hyp_resume function.
> > The prepare_resume_ctx must be invoked just before the PSCI system suspend
> > call is issued to the ATF. The prepare_resume_ctx must return a non-zero
> > value so that the calling 'if' statement evaluates to true, causing the
> > system suspend to be invoked. Upon resume, the 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 to by the saved link register, which is the place from which
> > prepare_resume_ctx was called. To ensure that the calling 'if' statement
> > does not again evaluate to true and initiate system suspend, hyp_resume
> > must return a zero value after restoring the context.
> >
> > Note that the order of saving register context into cpu_context structure
> > must 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 v8:
> > - fix alignments in code
> >
> > Changes in v7:
> > - no changes
> > ---
> >  xen/arch/arm/Makefile              |  1 +
> >  xen/arch/arm/arm64/head.S          | 90 +++++++++++++++++++++++++++++-
> >  xen/arch/arm/include/asm/suspend.h | 26 +++++++++
> >  xen/arch/arm/suspend.c             | 14 +++++
> >  4 files changed, 130 insertions(+), 1 deletion(-)
> >  create mode 100644 xen/arch/arm/suspend.c
> >
> > diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> > index 69200b2728..c36158271a 100644
> > --- a/xen/arch/arm/Makefile
> > +++ b/xen/arch/arm/Makefile
> > @@ -51,6 +51,7 @@ obj-y += setup.o
> >  obj-y += shutdown.o
> >  obj-y += smp.o
> >  obj-y += smpboot.o
> > +obj-$(CONFIG_SYSTEM_SUSPEND) += suspend.o
> >  obj-$(CONFIG_SYSCTL) += sysctl.o
> >  obj-y += time.o
> >  obj-y += traps.o
> > diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> > index 596e960152..2cb02ee314 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 prepare_resume_ctx(struct cpu_context *ptr)
>
> "cpu_context" is very generic name, especially taking into account that
> you are introducing a global variable with the same name. How about
> "resume_cpu_context"?

Ack.

>
> > + *
> > + * 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.
> > + * prepare_resume_ctx shall return a non-zero value. Upon restoring context
> > + * hyp_resume shall return value zero instead. From C code that invokes
> > + * prepare_resume_ctx, the return value is interpreted to determine whether
> > + * the context is saved (prepare_resume_ctx) or restored (hyp_resume).
> > + */
> > +FUNC(prepare_resume_ctx)
> > +        /* Store callee-saved registers */
>
> How are planning to synchronise this code with actual cpu_context?
>
> I am pretty sure it is better to use offsets generated by asm-offset.c

Ack.

>
> > +        stp   x19, x20, [x0], #16
> > +        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
> > +
> > +        /* prepare_resume_ctx must return a non-zero value */
> > +        mov   x0, #1
> > +        ret
> > +END(prepare_resume_ctx)
> >
> >  FUNC(hyp_resume)
> >          /* Initialize the UART if earlyprintk has been enabled. */
> > @@ -580,7 +626,49 @@ FUNC(hyp_resume)
> >          b     enable_secondary_cpu_mm
> >
> >  mmu_resumed:
> > -        b .
> > +        /* Now we can access the cpu_context, so restore the context here 
> > */
> > +        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 prepare_resume_ctx. To distinguish a return from
> > +         * prepare_resume_ctx 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 31a98a1f1b..c127fa3d78 100644
> > --- a/xen/arch/arm/include/asm/suspend.h
> > +++ b/xen/arch/arm/include/asm/suspend.h
> > @@ -3,6 +3,8 @@
> >  #ifndef ARM_SUSPEND_H
> >  #define ARM_SUSPEND_H
> >
> > +#include <xen/types.h>
> > +
> >  struct domain;
> >  struct vcpu;
> >  struct vcpu_guest_context;
> > @@ -14,6 +16,30 @@ struct resume_info {
> >
> >  void arch_domain_resume(struct domain *d);
> >
> > +#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 prepare_resume_ctx(struct cpu_context *ptr);
> > +void hyp_resume(void);
> > +#endif /* CONFIG_SYSTEM_SUSPEND */
> > +
> >  #endif /* ARM_SUSPEND_H */
> >
> >  /*
> > diff --git a/xen/arch/arm/suspend.c b/xen/arch/arm/suspend.c
> > new file mode 100644
> > index 0000000000..e38566b0b7
> > --- /dev/null
> > +++ b/xen/arch/arm/suspend.c
> > @@ -0,0 +1,14 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +
> > +#include <asm/suspend.h>
> > +
> > +struct cpu_context cpu_context = {};
>
> Don't need to zero-initialize a global variable.

Ack.

Best regards,
Mykola

>
> > +
> > +/*
> > + * Local variables:
> > + * mode: C
> > + * c-file-style: "BSD"
> > + * c-basic-offset: 4
> > + * indent-tabs-mode: nil
> > + * End:
> > + */
>
> --
> WBR, Volodymyr