Date: Fri, 23 Nov 2012 08:58:04 +0100 From: Giovanni Trematerra <gianni@freebsd.org> To: Warner Losh <imp@bsdimp.com> Cc: freebsd-arm@freebsd.org, cognet@freebsd.org Subject: Re: ARM/SMP, Some patches for review. Message-ID: <CACfq092M2xP-tVBH9acdb9qg4geZvmHWohm2Me4iFpryhqixKw@mail.gmail.com> In-Reply-To: <94A3C1F1-0138-4D11-85BC-9E988AEF2E9F@bsdimp.com> References: <50AA4E87.3000505@semihalf.com> <CACfq093LakTKEf8E3L5S7QnYcgdnBVpnFSRJo%2BMx=mfgbXpg0g@mail.gmail.com> <50ACE2B4.8010904@semihalf.com> <CACfq091X6BK7S5jenr46=iwpsyGEz1FdnRLMhXhPdZ4jd06g0w@mail.gmail.com> <50AE3C0F.20809@semihalf.com> <94A3C1F1-0138-4D11-85BC-9E988AEF2E9F@bsdimp.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, Nov 22, 2012 at 5:46 PM, Warner Losh <imp@bsdimp.com> wrote:
>
> On Nov 22, 2012, at 7:51 AM, =C5=81ukasz P=C5=82achno wrote:
>
>> On 21.11.2012 17:00, Giovanni Trematerra wrote:
>>> On Wed, Nov 21, 2012 at 3:18 PM, =C5=81ukasz P=C5=82achno <luk@semihalf=
.com> wrote:
>>>> On 20.11.2012 00:08, Giovanni Trematerra wrote:
>>>>>
>>>>> On Mon, Nov 19, 2012 at 4:21 PM, =C5=81ukasz P=C5=82achno <luk@semiha=
lf.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 pj4=
b)
>>>>>> 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 duri=
ng
>>>>>> 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, s=
o 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 =3D $S/$M/$M/cpufunc_asm_arm7td=
mi.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 en=
d
>>>>> 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 ar=
mv6
>>>> targets)
>>>> - Fixed issues with build for pre-armv6 targets (tested with make tin=
derbox
>>>> TARGETS=3Darm
>>>>
>>>
>>> 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
>>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>>> --- sys/arm/arm/cpufunc.c (revision 243182)
>>> +++ sys/arm/arm/cpufunc.c (working copy)
>>> @@ -1079,18 +1079,18 @@ struct cpu_functions cortexa_cpufuncs =3D {
>>> /* 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 =3D {
>>
>> 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 want=
ed 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) !=3D 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) !=3D 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
>>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>>> --- sys/conf/Makefile.arm (revision 243182)
>>> +++ sys/conf/Makefile.arm (working copy)
>>> @@ -51,6 +51,7 @@ SYSTEM_LD_TAIL +=3D;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 =3D $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!=3D grep 'CPU_CORTEXA\|CPU_MV_PJ4B' opt_global.h || true ; ec=
ho
>>> +
>>> +.if ${NO_TRAMP} =3D=3D ""
>>> KERNEL_EXTRA=3Dtrampoline
>>> KERNEL_EXTRA_INSTALL=3Dkernel.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+=3D MACHINE=3D${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!=3D grep DDB opt_ddb.h || true ; echo
DTR_ENABLED!=3D grep KDTRACE_FRAME opt_kdtrace.h || true ; echo
HWPMC_ENABLED!=3D 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?
Thank you
--
Gianni
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CACfq092M2xP-tVBH9acdb9qg4geZvmHWohm2Me4iFpryhqixKw>