request for review

[Marius Strobl]

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