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

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

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?

> +        /* 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?

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

-- 
WBR, Volodymyr