----- On Feb 6, 2018, at 8:55 AM, Will Deacon will.dea...@arm.com wrote:
> On Tue, Feb 06, 2018 at 12:52:34PM +0000, Mathieu Desnoyers wrote:
>> ----- On Feb 5, 2018, at 7:40 PM, Stephen Rothwell s...@canb.auug.org.au
>> wrote:
>>
>> > 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
