ARM/SMP, Some patches for review.

Łukasz Płachno luk at semihalf.com
Thu Nov 22 14:52:14 UTC 2012


On 21.11.2012 17:00, Giovanni Trematerra wrote:
> 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            */
>   };
>

I agree, but with this change I included also:

diff --git a/sys/arm/arm/cpufunc.c b/sys/arm/arm/cpufunc.c
index dd43c27..1d6f93f 100644
--- a/sys/arm/arm/cpufunc.c
+++ b/sys/arm/arm/cpufunc.c
@@ -1049,14 +1049,14 @@ struct cpu_functions cortexa_cpufuncs = {
  	
  	armv7_tlb_flushID,              /* tlb_flushID          */
  	armv7_tlb_flushID_SE,           /* tlb_flushID_SE       */
-	arm11_tlb_flushI,               /* tlb_flushI           */
-	arm11_tlb_flushI_SE,            /* tlb_flushI_SE        */
-	arm11_tlb_flushD,               /* tlb_flushD           */
-	arm11_tlb_flushD_SE,            /* tlb_flushD_SE        */
+	armv7_tlb_flushID,              /* tlb_flushI           */
+	armv7_tlb_flushID_SE,           /* tlb_flushI_SE        */
+	armv7_tlb_flushID,              /* tlb_flushD           */
+	armv7_tlb_flushID_SE,           /* tlb_flushD_SE        */

Changes merged into patch 1_SMP_fixes.diff

>
> 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.

None of changes in this patch are related to style. In this patch I 
wanted to improve code readability, not remove style conflicts.

>
> 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

Changed to ARM_ARCH_6_7A

>
> 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}
>

Change merged into commit 3.

> 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.
>

#defines reordered

> I'll try to test the pachset ASAP.
>

New set of patches attached.

Regards,
Łukasz Płachno

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 1_SMP_fixes.diff
Type: text/x-patch
Size: 5908 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/freebsd-arm/attachments/20121122/e54a0db9/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 2_ARM_cleanup.diff
Type: text/x-patch
Size: 5046 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/freebsd-arm/attachments/20121122/e54a0db9/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 3_trampoline.diff
Type: text/x-patch
Size: 3141 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/freebsd-arm/attachments/20121122/e54a0db9/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 4_tex_remap.diff
Type: text/x-patch
Size: 12197 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/freebsd-arm/attachments/20121122/e54a0db9/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 5_memory_barriers.diff
Type: text/x-patch
Size: 1283 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/freebsd-arm/attachments/20121122/e54a0db9/attachment-0004.bin>


More information about the freebsd-arm mailing list