From owner-freebsd-arm@FreeBSD.ORG Fri Nov 23 07:58:05 2012 Return-Path: Delivered-To: freebsd-arm@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 9F15920D; Fri, 23 Nov 2012 07:58:05 +0000 (UTC) (envelope-from giovanni.trematerra@gmail.com) Received: from mail-qc0-f182.google.com (mail-qc0-f182.google.com [209.85.216.182]) by mx1.freebsd.org (Postfix) with ESMTP id 373B48FC16; Fri, 23 Nov 2012 07:58:04 +0000 (UTC) Received: by mail-qc0-f182.google.com with SMTP id k19so7897375qcs.13 for ; Thu, 22 Nov 2012 23:58:04 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:sender:in-reply-to:references:date :x-google-sender-auth:message-id:subject:from:to:cc:content-type :content-transfer-encoding; bh=smumi4TZC+LzhRl4rJd9IhzD2j1dGqrkvxy4OEKAsTQ=; b=TjwWd8qL3TNBxIJtPwSVeWTAhH4rsgh71nzzxkrEf5oLKyQDLdSzCAGshsWPJVAWf9 I7VY3ZTUNc1PvAO4BoqL+hNledv3+B3kpEJmUIcWLxI5v2W/JFHt6nj+4oE/Q+u4SoPk KYOlqtFnHpSVQDub2Gh8d+qGxZWUZ3ONuoUckkGPqpYYhYFO32qlFMwmmpARjC4Fkgdo DUmmeffhKzb7i0Ul+tFYy6edJ4D/nt9w/rpQHh185CIE4tnr1nWgRpI4BB4VDoZuUw/8 SlSYuTKR4FHwlb1D2ocDMe1RvMIw+NXrD8TQqY/9CfZRAoE3RWZABF6H771VytaNc2zb gaRg== MIME-Version: 1.0 Received: by 10.224.116.12 with SMTP id k12mr3543329qaq.47.1353657484228; Thu, 22 Nov 2012 23:58:04 -0800 (PST) Sender: giovanni.trematerra@gmail.com Received: by 10.229.117.1 with HTTP; Thu, 22 Nov 2012 23:58:04 -0800 (PST) In-Reply-To: <94A3C1F1-0138-4D11-85BC-9E988AEF2E9F@bsdimp.com> References: <50AA4E87.3000505@semihalf.com> <50ACE2B4.8010904@semihalf.com> <50AE3C0F.20809@semihalf.com> <94A3C1F1-0138-4D11-85BC-9E988AEF2E9F@bsdimp.com> Date: Fri, 23 Nov 2012 08:58:04 +0100 X-Google-Sender-Auth: 4kHFyo74AR4bAsxobtuzOAa4IH0 Message-ID: Subject: Re: ARM/SMP, Some patches for review. From: Giovanni Trematerra To: Warner Losh Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Cc: freebsd-arm@freebsd.org, cognet@freebsd.org X-BeenThere: freebsd-arm@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: Porting FreeBSD to the StrongARM Processor List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 23 Nov 2012 07:58:05 -0000 On Thu, Nov 22, 2012 at 5:46 PM, Warner Losh 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 wrote: >>>> On 20.11.2012 00:08, Giovanni Trematerra wrote: >>>>> >>>>> On Mon, Nov 19, 2012 at 4:21 PM, =C5=81ukasz P=C5=82achno 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