ARM/SMP, Some patches for review.
Warner Losh
imp at bsdimp.com
Fri Nov 23 16:31:50 UTC 2012
On Nov 23, 2012, at 12:58 AM, Giovanni Trematerra wrote:
> On Thu, Nov 22, 2012 at 5:46 PM, Warner Losh <imp at bsdimp.com> wrote:
>>
>> 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.
>
>
> Well, I just mimicked what is done in sys/conf/kern.pre.mk
>
> # Are various things configured?
> DDB_ENABLED!= grep DDB opt_ddb.h || true ; echo
> DTR_ENABLED!= grep KDTRACE_FRAME opt_kdtrace.h || true ; echo
> HWPMC_ENABLED!= grep HWPMC opt_hwpmc_hooks.h || true ; echo
>
> I'm completely open to hear about the best way to accomplish that.
> Please could you be more specific on how I can test a kernel configuration knob
> from a Makefile?
Those really shouldn't be there either. At least they are reliable. The grepping for one of many options for the CPU is much more fragile.
makeoptions in the kernel config files is the proper way to do that sort of thing you want to disable the TRAMP code.
Warner
More information about the freebsd-arm
mailing list