[PATCH 3/6] arm64: kernel: Rework finisher callback out of __cpu_suspend_enter().

[lorenzo.pieralisi@arm.com (Lorenzo Pieralisi)]

Hi James,

On Mon, Oct 12, 2015 at 02:17:35PM +0100, James Morse wrote:
> Hibernate could make use of the cpu_suspend() code to save/restore cpu
> state, however it needs to be able to return '0' from the 'finisher'.
> 
> Rework cpu_suspend() so that the finisher is called from C code,
> independently from the save/restore of cpu state. Space to save the context
> in is allocated in the caller's stack frame, and passed into
> __cpu_suspend_enter().
> 
> Hibernate's use of this API will look like a copy of the cpu_suspend()
> function.
> 
> Signed-off-by: James Morse <james.morse@arm.com>
> ---
>  arch/arm64/include/asm/suspend.h |  8 ++++
>  arch/arm64/kernel/asm-offsets.c  |  2 +
>  arch/arm64/kernel/sleep.S        | 86 +++++++++++++---------------------------
>  arch/arm64/kernel/suspend.c      | 81 ++++++++++++++++++++++---------------
>  4 files changed, 86 insertions(+), 91 deletions(-)

Two minor requests below to update some comments, otherwise:

Acked-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>

> diff --git a/arch/arm64/include/asm/suspend.h b/arch/arm64/include/asm/suspend.h
> index 59a5b0f1e81c..a9de0d3f543f 100644
> --- a/arch/arm64/include/asm/suspend.h
> +++ b/arch/arm64/include/asm/suspend.h
> @@ -2,6 +2,7 @@
>  #define __ASM_SUSPEND_H
>  
>  #define NR_CTX_REGS 11
> +#define NR_CALLEE_SAVED_REGS 12
>  
>  /*
>   * struct cpu_suspend_ctx must be 16-byte aligned since it is allocated on
> @@ -21,6 +22,13 @@ struct sleep_save_sp {
>  	phys_addr_t save_ptr_stash_phys;
>  };
>  
> +struct sleep_stack_data {
> +	struct cpu_suspend_ctx	system_regs;
> +	unsigned long		callee_saved_regs[NR_CALLEE_SAVED_REGS];

Please add a comment referring to the __cpu_suspend_enter expected
registers layout and how this struct and __cpu_suspend_enter are related.

> +};
> +
>  extern int cpu_suspend(unsigned long arg, int (*fn)(unsigned long));
>  extern void cpu_resume(void);
> +int __cpu_suspend_enter(struct sleep_stack_data *state);
> +void __cpu_suspend_exit(struct mm_struct *mm);
>  #endif
> diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
> index 8d89cf8dae55..5daa4e692932 100644
> --- a/arch/arm64/kernel/asm-offsets.c
> +++ b/arch/arm64/kernel/asm-offsets.c
> @@ -160,6 +160,8 @@ int main(void)
>    DEFINE(SLEEP_SAVE_SP_SZ,	sizeof(struct sleep_save_sp));
>    DEFINE(SLEEP_SAVE_SP_PHYS,	offsetof(struct sleep_save_sp, save_ptr_stash_phys));
>    DEFINE(SLEEP_SAVE_SP_VIRT,	offsetof(struct sleep_save_sp, save_ptr_stash));
> +  DEFINE(SLEEP_STACK_DATA_SYSTEM_REGS,	offsetof(struct sleep_stack_data, system_regs));
> +  DEFINE(SLEEP_STACK_DATA_CALLEE_REGS,	offsetof(struct sleep_stack_data, callee_saved_regs));
>  #endif
>    return 0;
>  }
> diff --git a/arch/arm64/kernel/sleep.S b/arch/arm64/kernel/sleep.S
> index f586f7c875e2..6182388e32a5 100644
> --- a/arch/arm64/kernel/sleep.S
> +++ b/arch/arm64/kernel/sleep.S
> @@ -50,36 +50,24 @@
>  	.endm
>  /*
>   * Save CPU state for a suspend and execute the suspend finisher.

This function does not call the finisher anymore, please update the
comment above.

> - * On success it will return 0 through cpu_resume - ie through a CPU
> - * soft/hard reboot from the reset vector.
> - * On failure it returns the suspend finisher return value or force
> - * -EOPNOTSUPP if the finisher erroneously returns 0 (the suspend finisher
> - * is not allowed to return, if it does this must be considered failure).
> - * It saves callee registers, and allocates space on the kernel stack
> - * to save the CPU specific registers + some other data for resume.
> + * This function returns a non-zero value. Resuming through cpu_resume()
> + * will cause 0 to appear to be returned by this function.

Nit: please replace the description with an updated one to explain
what __cpu_suspend_enter is meant to achieve, in particular the
reasoning behind the return value (and code path) logic.

Thanks,
Lorenzo

>   *
> - *  x0 = suspend finisher argument
> - *  x1 = suspend finisher function pointer
> + *  x0 = struct sleep_stack_data area
>   */
>  ENTRY(__cpu_suspend_enter)
> -	stp	x29, lr, [sp, #-96]!
> -	stp	x19, x20, [sp,#16]
> -	stp	x21, x22, [sp,#32]
> -	stp	x23, x24, [sp,#48]
> -	stp	x25, x26, [sp,#64]
> -	stp	x27, x28, [sp,#80]
> -	/*
> -	 * Stash suspend finisher and its argument in x20 and x19
> -	 */
> -	mov	x19, x0
> -	mov	x20, x1
> +	stp	x29, lr, [x0, #SLEEP_STACK_DATA_CALLEE_REGS]
> +	stp	x19, x20, [x0,#SLEEP_STACK_DATA_CALLEE_REGS+16]
> +	stp	x21, x22, [x0,#SLEEP_STACK_DATA_CALLEE_REGS+32]
> +	stp	x23, x24, [x0,#SLEEP_STACK_DATA_CALLEE_REGS+48]
> +	stp	x25, x26, [x0,#SLEEP_STACK_DATA_CALLEE_REGS+64]
> +	stp	x27, x28, [x0,#SLEEP_STACK_DATA_CALLEE_REGS+80]
> +
> +	/* save the sp in cpu_suspend_ctx */
>  	mov	x2, sp
> -	sub	sp, sp, #CPU_SUSPEND_SZ	// allocate cpu_suspend_ctx
> -	mov	x0, sp
> -	/*
> -	 * x0 now points to struct cpu_suspend_ctx allocated on the stack
> -	 */
> -	str	x2, [x0, #CPU_CTX_SP]
> +	str	x2, [x0, #SLEEP_STACK_DATA_SYSTEM_REGS + CPU_CTX_SP]
> +
> +	/* find the mpidr_hash */
>  	ldr	x1, =sleep_save_sp
>  	ldr	x1, [x1, #SLEEP_SAVE_SP_VIRT]
>  	mrs	x7, mpidr_el1
> @@ -93,34 +81,11 @@ ENTRY(__cpu_suspend_enter)
>  	ldp	w5, w6, [x9, #(MPIDR_HASH_SHIFTS + 8)]
>  	compute_mpidr_hash x8, x3, x4, x5, x6, x7, x10
>  	add	x1, x1, x8, lsl #3
> +
> +	push	x29, lr
>  	bl	__cpu_suspend_save
> -	/*
> -	 * Grab suspend finisher in x20 and its argument in x19
> -	 */
> -	mov	x0, x19
> -	mov	x1, x20
> -	/*
> -	 * We are ready for power down, fire off the suspend finisher
> -	 * in x1, with argument in x0
> -	 */
> -	blr	x1
> -        /*
> -	 * Never gets here, unless suspend finisher fails.
> -	 * Successful cpu_suspend should return from cpu_resume, returning
> -	 * through this code path is considered an error
> -	 * If the return value is set to 0 force x0 = -EOPNOTSUPP
> -	 * to make sure a proper error condition is propagated
> -	 */
> -	cmp	x0, #0
> -	mov	x3, #-EOPNOTSUPP
> -	csel	x0, x3, x0, eq
> -	add	sp, sp, #CPU_SUSPEND_SZ	// rewind stack pointer
> -	ldp	x19, x20, [sp, #16]
> -	ldp	x21, x22, [sp, #32]
> -	ldp	x23, x24, [sp, #48]
> -	ldp	x25, x26, [sp, #64]
> -	ldp	x27, x28, [sp, #80]
> -	ldp	x29, lr, [sp], #96
> +	pop	x29, lr
> +	mov	x0, #1
>  	ret
>  ENDPROC(__cpu_suspend_enter)
>  	.ltorg
> @@ -146,12 +111,6 @@ ENDPROC(cpu_resume_mmu)
>  	.popsection
>  cpu_resume_after_mmu:
>  	mov	x0, #0			// return zero on success
> -	ldp	x19, x20, [sp, #16]
> -	ldp	x21, x22, [sp, #32]
> -	ldp	x23, x24, [sp, #48]
> -	ldp	x25, x26, [sp, #64]
> -	ldp	x27, x28, [sp, #80]
> -	ldp	x29, lr, [sp], #96
>  	ret
>  ENDPROC(cpu_resume_after_mmu)
>  
> @@ -168,6 +127,8 @@ ENTRY(cpu_resume)
>          /* x7 contains hash index, let's use it to grab context pointer */
>  	ldr_l	x0, sleep_save_sp + SLEEP_SAVE_SP_PHYS
>  	ldr	x0, [x0, x7, lsl #3]
> +	add	x29, x0, #SLEEP_STACK_DATA_CALLEE_REGS
> +	add	x0, x0, #SLEEP_STACK_DATA_SYSTEM_REGS
>  	/* load sp from context */
>  	ldr	x2, [x0, #CPU_CTX_SP]
>  	/* load physical address of identity map page table in x1 */
> @@ -178,5 +139,12 @@ ENTRY(cpu_resume)
>  	 * pointer and x1 to contain physical address of 1:1 page tables
>  	 */
>  	bl	cpu_do_resume		// PC relative jump, MMU off
> +	/* Can't access these by physical address once the MMU is on */
> +	ldp	x19, x20, [x29, #16]
> +	ldp	x21, x22, [x29, #32]
> +	ldp	x23, x24, [x29, #48]
> +	ldp	x25, x26, [x29, #64]
> +	ldp	x27, x28, [x29, #80]
> +	ldp	x29, lr, [x29]
>  	b	cpu_resume_mmu		// Resume MMU, never returns
>  ENDPROC(cpu_resume)
> diff --git a/arch/arm64/kernel/suspend.c b/arch/arm64/kernel/suspend.c
> index 8297d502217e..2c1a1fd0b4bb 100644
> --- a/arch/arm64/kernel/suspend.c
> +++ b/arch/arm64/kernel/suspend.c
> @@ -9,22 +9,22 @@
>  #include <asm/suspend.h>
>  #include <asm/tlbflush.h>
>  
> -extern int __cpu_suspend_enter(unsigned long arg, int (*fn)(unsigned long));
> +
>  /*
>   * This is called by __cpu_suspend_enter() to save the state, and do whatever
>   * flushing is required to ensure that when the CPU goes to sleep we have
>   * the necessary data available when the caches are not searched.
>   *
> - * ptr: CPU context virtual address
> + * ptr: sleep_stack_data containing cpu state virtual address.
>   * save_ptr: address of the location where the context physical address
>   *           must be saved
>   */
> -void notrace __cpu_suspend_save(struct cpu_suspend_ctx *ptr,
> +void notrace __cpu_suspend_save(struct sleep_stack_data *ptr,
>  				phys_addr_t *save_ptr)
>  {
>  	*save_ptr = virt_to_phys(ptr);
>  
> -	cpu_do_suspend(ptr);
> +	cpu_do_suspend(&ptr->system_regs);
>  	/*
>  	 * Only flush the context that must be retrieved with the MMU
>  	 * off. VA primitives ensure the flush is applied to all
> @@ -50,6 +50,37 @@ void __init cpu_suspend_set_dbg_restorer(void (*hw_bp_restore)(void *))
>  	hw_breakpoint_restore = hw_bp_restore;
>  }
>  
> +void notrace __cpu_suspend_exit(struct mm_struct *mm)
> +{
> +	/*
> +	 * We are resuming from reset with TTBR0_EL1 set to the
> +	 * idmap to enable the MMU; restore the active_mm mappings in
> +	 * TTBR0_EL1 unless the active_mm == &init_mm, in which case
> +	 * the thread entered cpu_suspend with TTBR0_EL1 set to
> +	 * reserved TTBR0 page tables and should be restored as such.
> +	 */
> +	if (mm == &init_mm)
> +		cpu_set_reserved_ttbr0();
> +	else
> +		cpu_switch_mm(mm->pgd, mm);
> +
> +	flush_tlb_all();
> +
> +	/*
> +	 * Restore per-cpu offset before any kernel
> +	 * subsystem relying on it has a chance to run.
> +	 */
> +	set_my_cpu_offset(per_cpu_offset(smp_processor_id()));
> +
> +	/*
> +	 * Restore HW breakpoint registers to sane values
> +	 * before debug exceptions are possibly reenabled
> +	 * through local_dbg_restore.
> +	 */
> +	if (hw_breakpoint_restore)
> +		hw_breakpoint_restore(NULL);
> +}
> +
>  /*
>   * cpu_suspend
>   *
> @@ -60,8 +91,9 @@ void __init cpu_suspend_set_dbg_restorer(void (*hw_bp_restore)(void *))
>  int cpu_suspend(unsigned long arg, int (*fn)(unsigned long))
>  {
>  	struct mm_struct *mm = current->active_mm;
> -	int ret;
> +	int ret = 0;
>  	unsigned long flags;
> +	struct sleep_stack_data state;
>  
>  	/*
>  	 * From this point debug exceptions are disabled to prevent
> @@ -76,36 +108,21 @@ int cpu_suspend(unsigned long arg, int (*fn)(unsigned long))
>  	 * page tables, so that the thread address space is properly
>  	 * set-up on function return.
>  	 */
> -	ret = __cpu_suspend_enter(arg, fn);
> -	if (ret == 0) {
> -		/*
> -		 * We are resuming from reset with TTBR0_EL1 set to the
> -		 * idmap to enable the MMU; restore the active_mm mappings in
> -		 * TTBR0_EL1 unless the active_mm == &init_mm, in which case
> -		 * the thread entered cpu_suspend with TTBR0_EL1 set to
> -		 * reserved TTBR0 page tables and should be restored as such.
> -		 */
> -		if (mm == &init_mm)
> -			cpu_set_reserved_ttbr0();
> -		else
> -			cpu_switch_mm(mm->pgd, mm);
> -
> -		flush_tlb_all();
> -
> -		/*
> -		 * Restore per-cpu offset before any kernel
> -		 * subsystem relying on it has a chance to run.
> -		 */
> -		set_my_cpu_offset(per_cpu_offset(smp_processor_id()));
> +	if (__cpu_suspend_enter(&state)) {
> +		/* Call the suspend finisher */
> +		ret = fn(arg);
>  
>  		/*
> -		 * Restore HW breakpoint registers to sane values
> -		 * before debug exceptions are possibly reenabled
> -		 * through local_dbg_restore.
> +		 * Never gets here, unless suspend finisher fails.
> +		 * Successful cpu_suspend should return from cpu_resume,
> +		 * returning through this code path is considered an error
> +		 * If the return value is set to 0 force ret = -EOPNOTSUPP
> +		 * to make sure a proper error condition is propagated
>  		 */
> -		if (hw_breakpoint_restore)
> -			hw_breakpoint_restore(NULL);
> -	}
> +		if (!ret)
> +			ret = -EOPNOTSUPP;
> +	} else
> +		__cpu_suspend_exit(mm);
>  
>  	/*
>  	 * Restore pstate flags. OS lock and mdscr have been already
> -- 
> 2.1.4
>