[PATCH] ARM: Allow SMP_ON_UP to work with Thumb-2 kernels.

[Dave Martin]

On Mon, Nov 22, 2010 at 8:08 PM, Nicolas Pitre <nicolas.pitre at linaro.org> wrote:
> On Mon, 22 Nov 2010, Dave Martin wrote:
>
>>   * __fixup_smp_on_up has been modified with support for the THUMB2_KERNEL
>>       case.  For THUMB2_KERNEL only, fixups are split into halfwords in
>>       case of misalignment, since we can't rely on unaligned accesses
>>       working before turning the MMU on.
>>
>>       No attempt is made to optimise the aligned case, since the number
>>       of fixups is typically small, and it seems best to keep the code
>>       as simple as possible.
>
> ACK.
>
>>   * A "mode" parameter has been added to smp_dmb:
>>
>>       smp_dmp arm     @ assumes 4-byte instructions (for ARM code, e.g. kuser)
>>       smp_dmp         @ uses W() to ensure 4-byte instructions for ALT_SMP()
>>
>>       This avoids assembly failures due to use of W() inside smp_dmb,
>>       when assembling pure-ARM code in the vectors page.
>>
>>       There might be a better way to achieve this.
>
> Is the W() sufficient to indicate an ARM encoding?  To me it is meant to
> specify a wide encoding when using Thumb2 encoding.  And if this is for

Your understanding is correct--- for most code we don't want to force
an ARM encoding; it's just necessary to have a 32-bit encoding so that
the instructions inserted by the fixup code will fit in, and also
completely fill the gap.

I did have a previous implementation which inserted a 16-bit nop where
necessary, but that was arguably messier.  The principle is the same.

> the kuser code segments (which should always be ARM mode by the way)
> then this shouldn't make any difference.

It would be nice to be able to use W() in ARM state (in which case it
should simply have no effect).  However, the .w suffix (generated by
W()) is invalid syntax in traditional ARM assembler.

Unfortunately, W() is defined once per configuration, depending on
CONFIG_THUMB2_KERNEL ... and there's no way to query the current
target instruction set (.arm/.thumb) from inside the definition of W()
(or at all, for that matter).  So using this macro in .arm code causes
assembly failures.  However, we don't really need this in ARM code,
since ARM instructions are always 32 bits (this is why SMP_ON_UP
always works for ARM).

The statelessness of the C preprocessor makes it tricky to implement
this kind of thing in a generic way--- it might be better to use gas
macros for some features such as this, since it's very much more
powerful.  However, I'm sure there are good arguments against that
too, I didn't want to introduce disruption for the sake of it.

Notwithstanding this, I'm very much open to suggestions about a better
solution for this if anyone can think of any ideas.

>
> And did you mean "smp_dmb" instead of "smp_dmp" above?

Ummm, yes.  Fortunately, the diff is spelled correctly ... I'll tweak
the message when I repost.

>
>>   * Kconfig: make SMP_ON_UP depend on (!THUMB2_KERNEL || !BIG_ENDIAN)
>>       i.e., THUMB2_KERNEL is now supported, but only if !BIG_ENDIAN
>>       (The fixup code for Thumb-2 currently assumes little-endian order.)
> [...]
>> --- a/arch/arm/kernel/head.S
>> +++ b/arch/arm/kernel/head.S
>> @@ -407,10 +407,14 @@ __fixup_smp_on_up:
>>       add     r6, r6, r3
>>       add     r7, r7, r3
>>  2:   cmp     r6, r7
>> +     movhs   pc, lr
>>       ldmia   r6!, {r0, r4}
>> -     strlo   r4, [r0, r3]
>> -     blo     2b
>> -     mov     pc, lr
>> + ARM(        str     r4, [r0, r3]    )
>> + THUMB(      add     r0, r0, r3      )
>> + THUMB(      strh    r4, [r0], #2    )       @ For Thumb-2, store as two halfwords
>> + THUMB(      mov     r4, r4, lsr #16 )       @ to be robust against misaligned r3.
>> + THUMB(      strh    r4, [r0]        )       @ NOTE: currently assumes little-endian.
>> +     b       2b
>
> All you minimally need to support big endian here is to insert:
>
> #ifndef __ARMEB__
> THUMB(  mov     r4, r4, ror #16 )
> #endif
>
> just before the first store.

Cool, I'll add that.  (I assume you mean #ifdef, not #ifndef ?)

Since we're running strongly-ordered at this point, this will have
negligible impact on execution time.

Cheers
---Dave