ARM/SMP, Some patches for review.
Warner Losh
imp at bsdimp.com
Thu Nov 22 16:48:28 UTC 2012
On Nov 22, 2012, at 7:51 AM, Łukasz Płachno wrote:
> 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.
I really don't like this part of the change. The if is ok, but the grep isn't. It should be in the kernel config file as an option. I can be in std.XXX files easily.
>> 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.
I'll take a look at these in more detail...
Warner
> Regards,
> Łukasz Płachno
>
> <1_SMP_fixes.diff><2_ARM_cleanup.diff><3_trampoline.diff><4_tex_remap.diff><5_memory_barriers.diff>_______________________________________________
> freebsd-arm at freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-arm
> To unsubscribe, send any mail to "freebsd-arm-unsubscribe at freebsd.org"
More information about the freebsd-arm
mailing list