On Thu, Apr 30, 2020 at 06:18:25PM +0100, Will Deacon wrote:
> On Tue, Apr 28, 2020 at 05:43:30PM +0100, Mark Brown wrote:

> > -work_pending:
> > +SYM_CODE_START_LOCAL(work_pending)
> >  	mov	x0, sp				// 'regs'
> >  	bl	do_notify_resume
> >  #ifdef CONFIG_TRACE_IRQFLAGS
> > @@ -738,10 +738,11 @@ work_pending:
> >  #endif
> >  	ldr	x1, [tsk, #TSK_TI_FLAGS]	// re-check for single-step
> >  	b	finish_ret_to_user
> > +SYM_CODE_END(work_pending)
> >  /*
> >   * "slow" syscall return path.
> >   */
> > -ret_to_user:
> > +SYM_CODE_START_LOCAL(ret_to_user)

> Would this be better off as a SYM_INNER_LABEL inside work_pending? Given
> that ret_to_user and work_pending both branch into each other, separating
> them doesn't feel quite right.

I remember looking at these when doing the conversion and thinking that
nothing looked quite right due to the cross calls :/  The number of
things that branch to ret_to_user made me think it should really be it's
own thing rather than just a label in the middle of another block but
then work_pending is really a subroutine of ret_to_user that uses a
branch rather than a call so how do you annotate that?

Possibly we could move work_pending after the kernel_exit in ret_to_user
and make work_pending the SYM_INNER_LABEL, doing things the opposite way
around to what you suggest?  I think that's more the intent.

> > +SYM_CODE_START_LOCAL(\label)
> >  	b \label
> > -ENDPROC(\label)
> > +SYM_CODE_END(\label)
> >  .endm

> Huh, this is the exact same macro as the one from the hibernate code. Maybe
> we should stick it in asm/asembler.h alongside ventry? Obviously a separate
> patch, though.

I agree.

> > -ENTRY(absolute_data64)
> > +SYM_CODE_START(absolute_data64)
> >  	ldr	x0, 0f
> >  	ret
> >  0:	.quad	sym64_abs
> > -ENDPROC(absolute_data64)
> > +SYM_CODE_END(absolute_data64)

> Hmm, but all the functions in here *are* actually just called from the C
> code in reloc_test_core.c afaict, so they should probably be using
> SYM_FUNC_*.

You're right I think - I remember thinking as I was going through that
since they were explicitly designed to test relocations it might be
important that they emit exactly the instructions that are written but
now I look again the functions are actually called rather than just
linked so we need to emit landing pads for them.

> > -ENTRY(cpu_resume)
> > +SYM_FUNC_START(cpu_resume)
> >  	bl	el2_setup		// if in EL2 drop to EL1 cleanly
> >  	mov	x0, #ARM64_CPU_RUNTIME
> >  	bl	__cpu_setup
> > @@ -107,11 +107,11 @@ ENTRY(cpu_resume)
> >  	bl	__enable_mmu
> >  	ldr	x8, =_cpu_resume
> >  	br	x8
> > -ENDPROC(cpu_resume)
> > +SYM_FUNC_END(cpu_resume)

> SYM_CODE_* here, as this is I think this is the entry point from the resume
> path?

It has a C prototype in asm/suspend.h and swsup_arch_suspend_exit in
hibernate-asm.S runs earlier but now I look again it jumps here by
issuing a ret rather than via a call so it's definitely not a normal C
function.