ARM/SMP, Some patches for review.

[Giovanni Trematerra]

On Wed, Nov 21, 2012 at 3:18 PM, Łukasz Płachno <luk at semihalf.com> wrote:
> On 20.11.2012 00:08, Giovanni Trematerra wrote:
>>
>> On Mon, Nov 19, 2012 at 4:21 PM, Łukasz Płachno <luk at semihalf.com> wrote:
>>>
>>> Hi,
>>>
>>> I would like to propose few changes for ARM specific code.
>>> Three attached patches for freebsd-current allows building SMP-safe world
>>> for ARM targets and turns on TEX remap for ARMv6 and ARMv7 targets.
>>>
>>> More details inside patch files.
>>>
>>> Change introduced by "commit-2" removes armv7 targets (armv7 and pj4b)
>>> from
>>> kernel.tramp.
>>> AFAIK this feature is not working properly for armv7 targets and is
>>> causing
>>> problem during compilation:
>>>   - LOCORE is defined during kernel compilation but not defined during
>>> kernel.tramp compilation, so #include pmap.h causes build errors.
>>>
>>> I do not think adding hack like this:
>>> #ifndef LOCORE
>>> #define LOCORE
>>> #endif
>>>
>>> to allow building something that is already broken is a good idea, so I
>>> removed cpufunc_asm_pj4b.S and cpufunc_asm_armv7.S from Makefile.arm
>>
>>
>> In commit-2.txt
>> you should include style changes in sys/arm/arm/cpufunc_asm_armv7.S
>> into a different patch.
>
>
> fixed
>
>
>>
>> @@ -63,7 +64,6 @@ FILES_CPU_FUNC =      $S/$M/$M/cpufunc_asm_arm7tdmi.S \
>>          $S/$M/$M/cpufunc_asm_xscale.S $S/$M/$M/cpufunc_asm.S \
>>          $S/$M/$M/cpufunc_asm_xscale_c3.S $S/$M/$M/cpufunc_asm_armv5_ec.S
>> \
>>          $S/$M/$M/cpufunc_asm_fa526.S $S/$M/$M/cpufunc_asm_sheeva.S \
>> -       $S/$M/$M/cpufunc_asm_pj4b.S $S/$M/$M/cpufunc_asm_armv7.S
>>
>> You left a trailing back slash but beside that you should clean up
>> sys/arm/arm/elf_trampoline.c
>> and not make kernel.tramp to build at all for armv7 cpus or you'll end
>> up with a linker error
>> during generation of the kernel.tramp.
>>
>
> Fixed, updated set of patches is attached.
>
>  - TEX remap is supported only for armv6 (changed to avoid breaking armv6
> targets)
>  - Fixed issues with build for pre-armv6 targets (tested with make tinderbox
> TARGETS=arm
>

1_SMP_fixes.diff
You'll endup to get a panic for PandaBoard systems.
The arm11 functions don't handle the SMP case.
So I propose to merge the changes below or commit them first.

Index: sys/arm/arm/cpufunc.c
===================================================================
--- sys/arm/arm/cpufunc.c       (revision 243182)
+++ sys/arm/arm/cpufunc.c       (working copy)
@@ -1079,18 +1079,18 @@ struct cpu_functions cortexa_cpufuncs = {
        /* Other functions */

        cpufunc_nullop,                 /* flush_prefetchbuf    */
-       arm11_drain_writebuf,           /* drain_writebuf       */
+       armv7_drain_writebuf,           /* drain_writebuf       */
        cpufunc_nullop,                 /* flush_brnchtgt_C     */
        (void *)cpufunc_nullop,         /* flush_brnchtgt_E     */

-       arm11_sleep,                    /* sleep                */
+       armv7_cpu_sleep,                /* sleep                */

        /* Soft functions */

        cpufunc_null_fixup,             /* dataabt_fixup        */
        cpufunc_null_fixup,             /* prefetchabt_fixup    */

-       arm11_context_switch,           /* context_switch       */
+       armv7_context_switch,           /* context_switch       */

        cortexa_setup                     /* cpu setup            */
 };


2_ARM_cleanup.diff
Changes to sys/arm/arm/machdep.c don't seem style changes and
they should live in a separate patch with a different motivation.

I'm not sure changes in sys/arm/arm/locore.S are style ones.

I think that things like this aren't so readable.
#if (ARM_ARCH_6 + ARM_ARCH_7A) != 0

Instead of things like that wouldn't be better to define different
macros when the sum is zero or non zero and stick with the
#if defined/!defined thing?

I mean in sys/arm/arm/cpuconf.h we could make something like this

#if (ARM_ARCH_6 + ARM_ARCH_7A) != 0
#define ARM_ARCH_6_7A
#endif

3_kernel_trampoline.diff
I think we should not make kernel_trampoline at all for the unsupported CPUs.
I propose this change to Makefile.arm

Index: sys/conf/Makefile.arm
===================================================================
--- sys/conf/Makefile.arm       (revision 243182)
+++ sys/conf/Makefile.arm       (working copy)
@@ -51,6 +51,7 @@ SYSTEM_LD_TAIL +=;sed s/" + SIZEOF_HEADERS"// ldsc
                ${SYSTEM_LD_}; \
                ${OBJCOPY} -S -O binary ${FULLKERNEL}.noheader \
                ${KERNEL_KO}.bin; \
+               ${NM} ${FULLKERNEL}.noheader | sort > ${FULLKERNEL}.map; \
                rm ${FULLKERNEL}.noheader

 .if defined(MFS_IMAGE)
@@ -62,9 +63,11 @@ FILES_CPU_FUNC =     $S/$M/$M/cpufunc_asm_arm7tdmi.S \
        $S/$M/$M/cpufunc_asm_sa1.S $S/$M/$M/cpufunc_asm_arm10.S \
        $S/$M/$M/cpufunc_asm_xscale.S $S/$M/$M/cpufunc_asm.S \
        $S/$M/$M/cpufunc_asm_xscale_c3.S $S/$M/$M/cpufunc_asm_armv5_ec.S \
        $S/$M/$M/cpufunc_asm_fa526.S $S/$M/$M/cpufunc_asm_sheeva.S \
-       $S/$M/$M/cpufunc_asm_pj4b.S $S/$M/$M/cpufunc_asm_armv7.S
+       $S/$M/$M/cpufunc_asm_armv6.S

+NO_TRAMP!= grep 'CPU_CORTEXA\|CPU_MV_PJ4B' opt_global.h || true ; echo
+
+.if ${NO_TRAMP} == ""
 KERNEL_EXTRA=trampoline
 KERNEL_EXTRA_INSTALL=kernel.gz.tramp
 trampoline: ${KERNEL_KO}.tramp
@@ -110,6 +113,7 @@ ${KERNEL_KO}.tramp: ${KERNEL_KO} $S/$M/$M/inckern.
        ${KERNEL_KO}.gz.tramp.bin
        rm ${KERNEL_KO}.tmp.gz ${KERNEL_KO}.tramp.noheader opt_kernname.h \
        inflate-tramp.o tmphack.S
+.endif

 MKMODULESENV+= MACHINE=${MACHINE}

4_tex-remap.diff
Some style(9) consideration.
#include(s) should be grouped together in alphabetical order.
So you should fix the pmap.h includes that you made.

I'll try to test the pachset ASAP.

--
Gianni