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

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

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"?

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

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

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

-- 
WBR, Volodymyr