----- On Feb 6, 2018, at 8:55 AM, Will Deacon will.deacon@arm.com wrote:

On Tue, Feb 06, 2018 at 12:52:34PM +0000, Mathieu Desnoyers wrote:

quoted

----- On Feb 5, 2018, at 7:40 PM, Stephen Rothwell sfr@canb.auug.org.au wrote:

quoted

Hi all,

Today's linux-next merge of the tip tree got a conflict in:

 arch/arm64/kernel/entry.S

between commit:

 4bf3286d29f3 ("arm64: entry: Hook up entry trampoline to exception vectors")

from Linus' tree and commit:

 f1e3a12b6543 ("membarrier/arm64: Provide core serializing command")

from the tip tree.

I fixed it up (probably not completely - see below) and can carry the
fix as necessary. This is now fixed as far as linux-next is concerned,
but any non trivial conflicts should be mentioned to your upstream
maintainer when your tree is submitted for merging.  You may also want
to consider cooperating with the maintainer of the conflicting tree to
minimise any particularly complex conflicts.

The change introduced by 4bf3286d29 "arm64: entry: Hook up entry trampoline
to exception vectors" adds a trampoline as mechanism for return:

-       eret                                    // return to kernel
+
+#ifndef CONFIG_UNMAP_KERNEL_AT_EL0
+       eret
+#else
+       .if     \el == 0
+       bne     4f
+       msr     far_el1, x30
+       tramp_alias     x30, tramp_exit_native
+       br      x30
+4:
+       tramp_alias     x30, tramp_exit_compat
+       br      x30
+       .else
+       eret
+       .endif
+#endif

Which means that with CONFIG_UNMAP_KERNEL_AT_EL0, for return to EL0,
the eret is in the trampoline:

        .macro tramp_exit, regsize = 64
        adr     x30, tramp_vectors
        msr     vbar_el1, x30
        tramp_unmap_kernel      x30
        .if     \regsize == 64
        mrs     x30, far_el1
        .endif
        eret
        .endm

ENTRY(tramp_exit_native)
        tramp_exit
END(tramp_exit_native)

ENTRY(tramp_exit_compat)
        tramp_exit      32
END(tramp_exit_compat)


One approach I would consider for this is to duplicate this
comment and add it just above the "eret" instruction within the
macro:

	/*
	 * ARCH_HAS_MEMBARRIER_SYNC_CORE rely on eret context synchronization
	 * when returning from IPI handler, and when returning to user-space.
	 */

Or perhaps Will has something else in mind ?

To be honest with you, I'd just drop the comment entirely. entry.S is
terrifying these days and nobody should have to go in there to understand
why we select ARCH_HAS_MEMBARRIER_SYNC_CORE. If you really feel a justification
is needed, I'd be happy with a line in the Kconfig file.

My concern is that someone wanting to optimize away a few cycles by changing
eret to something else in the future will not be looking at Kconfig: that
person will be staring at entry.S.

One alternative presented by PeterZ on irc is to do like ppc: define a
macro for eret, and stick all appropriate comments near the macro. This
way, it won't hurt when reading the code, but at least it keeps the
comments near the instruction being discussed.

Thanks,

Mathieu


-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com