|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v8 11/13] xen/arm: Save/restore context on suspend/resume
On 4/2/26 13:45, Mykola Kvach wrote: Hello MykolaI 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) ... 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. + + /* 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]
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |