Date: Sun, 19 Nov 2006 03:25:49 +0100 From: Marius Strobl <marius@alchemy.franken.de> To: Kip Macy <kip.macy@gmail.com> Cc: freebsd-sparc64@freebsd.org Subject: Re: request for review Message-ID: <20061119022549.GP76234@alchemy.franken.de> In-Reply-To: <b1fa29170611181201v10d06201xa958a0b6e3cae88d@mail.gmail.com> References: <b1fa29170611181201v10d06201xa958a0b6e3cae88d@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, Nov 18, 2006 at 12:01:42PM -0800, Kip Macy wrote: > I'm trying to remove files from sun4v that are largely redundant with > sparc64. Below I've made small changes to some sparc64 files for sun4v. > Please review the following: > http://www.fsmware.com/sun4v/file_remove.diff Some thoughts: - If FreeBSD/sun4v is to use source files in sys/sparc64/sparc64 it should also use the corresponding headers in sys/sparc64/include and not duplicate them in sys/sun4v/include, i.e. for pairs like f.e. ofw_machdep.{c,h}. The approach FreeBSD/pc98 takes, i.e. using sys/pc98/foo.h headers that just #include <i386/foo.h> seems ok for this (looks like include/Makefile already automatically installs sys/sparc64/include/*.h to /usr/include/sparc64 on FreeBSD/sun4v). Another way could be to introduce <machine_arch/foo.h> type includes and use that in the shared .c files. The #include <sparc64/foo.h> approach at least seems to be also a good idea for headers like ieee.h that just don't differ between FreeBSD/sparc64 and FreeBSD/sun4v. For sys/sun4v/include headers that merely exist for compiling the shared boot loader using the sys/sparc64/include version would be also nice regardless of the approach taken. - sys/conf/files.sun4v should be sorted like the rest of sys/conf/files* - In sys/sparc64/sparc64/dump_machdep.c the XXX comment you add seems to refer to SUN4V only; if that's the case you should make that clear as such. Maybe: #ifdef SUN4U hdr.dh_tsb_pa = tsb_kernel_phys; <...> #else /* XXX SUN4V */ #endif - The changes to sys/sparc64/sparc64/genassym.c seem unrelated. Generally you should pay attention to not adding gratuitous white space. - I'd suggest to not share sys/sparc64/sparc64/tick.c as sun4v should not need to use the workaround (the wrtickcmpr() macro) for the bug in some USII CPUs and therefore can save some instructions here and because eventually sys/sparc64/sparc64/tick.c should grow support for the stick counters of USII{e,i} and USIII CPUs (different beast each), making sys/sparc64/sparc64/tick.c already complicated without the #ifdef for sun4v. Regarding redundant and unused source files of FreeBSD/sun4v in general I've noticed that sys/boot/sparc64/loader/hcall.S is neither connected to the build nor included by another source file and probably currently violates the CDDL. There also seem to be several headers like cache.h, iommureg.h, iommuvar.h and ofw_upa.h in sys/sun4v/include that are sun4u-specific but which aren't used for compling the shared loader and therefore should not actually exist (some of which are superfluously included in sys/sun4v/sun4v though). Btw., I'd like to commit: http://people.freebsd.org/~marius/loader.diff which removes some code duplication by consistently using the global instance and package handles libofw sets up (maybe not the cleanest approach but actually already there, at least in the libofw openfirm.h, and not that bad if you look at other areas of the loader...) and by using OF_call_method() directly instead of its sparc64- and sun4v- specific special purpose variants (which therefore shouldn't be part of libofw). IIRC this reduces the loader binary by 8k. I don't expect this to break anything but could you please give it a try on sun4v hardware? Btw., for the same MI reason it would be nice if you could move the implementation of OF_set_mmfsa_traptable() from sys/dev/ofw/openfirm.c to somewhere in sys/sun4v/sun4v; maybe SUNW,set-trap-table also works on sun4u but very unlikely on powerpc. Similar sun4u-specific SUNW,foo stuff also lives in sys/sparc64/sparc64 and not in the MI intended sys/dev/ofw... I'd like to also commit: http://people.freebsd.org/~marius/asm_direct_macro.diff which converts the rest of the low hanging fruits regarding including headers for their macros in .S directly instead of going through genassym.c/assym.s. Do you object this? Marius
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20061119022549.GP76234>