Date: Fri, 23 Nov 2012 09:40:42 -0700 From: Warner Losh <imp@bsdimp.com> To: Giovanni Trematerra <gianni@freebsd.org> Cc: freebsd-arm@freebsd.org, cognet@freebsd.org Subject: Re: ARM/SMP, Some patches for review. Message-ID: <B76BA216-DDA4-47B8-B85E-7C2DA8952777@bsdimp.com> In-Reply-To: <CACfq092M2xP-tVBH9acdb9qg4geZvmHWohm2Me4iFpryhqixKw@mail.gmail.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> <CACfq092M2xP-tVBH9acdb9qg4geZvmHWohm2Me4iFpryhqixKw@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Nov 23, 2012, at 12:58 AM, Giovanni Trematerra wrote:
> On Thu, Nov 22, 2012 at 5:46 PM, Warner Losh <imp@bsdimp.com> wrote:
>>=20
>> On Nov 22, 2012, at 7:51 AM, =C5=81ukasz P=C5=82achno wrote:
>>=20
>>> 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:
>>>>>>=20
>>>>>> On Mon, Nov 19, 2012 at 4:21 PM, =C5=81ukasz P=C5=82achno =
<luk@semihalf.com> wrote:
>>>>>>>=20
>>>>>>> Hi,
>>>>>>>=20
>>>>>>> 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.
>>>>>>>=20
>>>>>>> More details inside patch files.
>>>>>>>=20
>>>>>>> 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.
>>>>>>>=20
>>>>>>> I do not think adding hack like this:
>>>>>>> #ifndef LOCORE
>>>>>>> #define LOCORE
>>>>>>> #endif
>>>>>>>=20
>>>>>>> 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
>>>>>>=20
>>>>>>=20
>>>>>> In commit-2.txt
>>>>>> you should include style changes in =
sys/arm/arm/cpufunc_asm_armv7.S
>>>>>> into a different patch.
>>>>>=20
>>>>>=20
>>>>> fixed
>>>>>=20
>>>>>=20
>>>>>>=20
>>>>>> @@ -63,7 +64,6 @@ FILES_CPU_FUNC =3D =
$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
>>>>>>=20
>>>>>> 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.
>>>>>>=20
>>>>>=20
>>>>> Fixed, updated set of patches is attached.
>>>>>=20
>>>>> - 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=3Darm
>>>>>=20
>>>>=20
>>>> 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.
>>>>=20
>>>> 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 */
>>>>=20
>>>> 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 */
>>>>=20
>>>> - arm11_sleep, /* sleep */
>>>> + armv7_cpu_sleep, /* sleep */
>>>>=20
>>>> /* Soft functions */
>>>>=20
>>>> cpufunc_null_fixup, /* dataabt_fixup */
>>>> cpufunc_null_fixup, /* prefetchabt_fixup */
>>>>=20
>>>> - arm11_context_switch, /* context_switch */
>>>> + armv7_context_switch, /* context_switch */
>>>>=20
>>>> cortexa_setup /* cpu setup */
>>>> };
>>>>=20
>>>=20
>>> I agree, but with this change I included also:
>>>=20
>>> 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 {
>>>=20
>>> 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 */
>>>=20
>>> Changes merged into patch 1_SMP_fixes.diff
>>>=20
>>>>=20
>>>> 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.
>>>>=20
>>>> I'm not sure changes in sys/arm/arm/locore.S are style ones.
>>>=20
>>> None of changes in this patch are related to style. In this patch I =
wanted to improve code readability, not remove style conflicts.
>>>=20
>>>>=20
>>>> I think that things like this aren't so readable.
>>>> #if (ARM_ARCH_6 + ARM_ARCH_7A) !=3D 0
>>>>=20
>>>> 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?
>>>>=20
>>>> I mean in sys/arm/arm/cpuconf.h we could make something like this
>>>>=20
>>>> #if (ARM_ARCH_6 + ARM_ARCH_7A) !=3D 0
>>>> #define ARM_ARCH_6_7A
>>>> #endif
>>>=20
>>> Changed to ARM_ARCH_6_7A
>>>=20
>>>>=20
>>>> 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
>>>>=20
>>>> 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
>>>>=20
>>>> .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
>>>>=20
>>>> +NO_TRAMP!=3D grep 'CPU_CORTEXA\|CPU_MV_PJ4B' opt_global.h || true =
; echo
>>>> +
>>>> +.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
>>>>=20
>>>> MKMODULESENV+=3D MACHINE=3D${MACHINE}
>>>>=20
>>>=20
>>> Change merged into commit 3.
>>=20
>> 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.
>=20
>=20
> Well, I just mimicked what is done in sys/conf/kern.pre.mk
>=20
> # 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
>=20
> 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?
Furthermore these items are used exclusively to change the compiler args =
only (except the weird DDB_ENABLED for clean files in the arm makefile =
that I see absolutely no reason to be there). There should be a better =
way of doing things, and I'll investigate it. However, I sometimes =
think we need to revamp all the building substantially, and not just add =
hacks to config.
There have often been times that I wanted to disable building these for =
my own reasons not related to what CPU I'm on.
Warner=
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?B76BA216-DDA4-47B8-B85E-7C2DA8952777>