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

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Oleksandr,

Thank you for the review.

On Mon, May 11, 2026 at 7:00 PM Oleksandr Tyshchenko
<olekstysh@xxxxxxxxx> wrote:
>
>
>
> On 4/2/26 13:45, Mykola Kvach wrote:
>
> Hello Mykola
>
> I did not spot any obvious issues with this patch. As far as I can tell,
> the save/restore register set appears to be complete and correct for the
> current codebase.
>
> Just one observation: there is an API asymmetry between
> prepare_resume_ctx() and hyp_resume() (save uses pointer, restore
> hardcodes global) ...
>
> > 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)
> > + *
> > + * x0 - pointer to the storage where callee's context will be saved
>
>     ... the C signature takes a pointer (struct cpu_context *ptr) and
> the save path uses it, ...
>
> > + *
> > + * 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 */
> > +        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
>
>     ... but the restore path hardcodes =cpu_context, ignoring whatever
> pointer was originally passed. If a caller were to pass anything other
> than &cpu_context, the resume would load from the wrong location. Since
> the sole call site does pass &cpu_context (called from system_suspend()
> in the last patch), this works correctly today — but the API is somewhat
> misleading.
>
> I might be missing something, but why not make prepare_resume_ctx() take
> no arguments and use =cpu_context directly inside the assembly? That way
> the save and restore paths would both use the same global, and the API
> would not be misleading.

Yes, good point. Since the resume path restores from the global context object,
the argument to prepare_resume_ctx() is misleading.

I will remove the argument and make both the save and restore paths use the
same global resume_cpu_context object.

Best regards,
Mykola

>
> > +
> > +        /* 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 */
>
>
> [snip]
>
>