Re: [PATCH] a.out support for multiboot and freebsd

[Robert Millan <rmh@aybabtu.com>]

On Tue, Feb 12, 2008 at 04:46:31AM +0800, Bean wrote:
> +# For _freebsd.mod
> +_bsd_mod_SOURCES = loader/i386/pc/bsd.c

You forgot to rename the commend ;-)

> +_bsd_mod_CFLAGS = $(COMMON_CFLAGS)
> +_bsd_mod_LDFLAGS = $(COMMON_LDFLAGS)
> +
> +# For freebsd.mod
> +bsd_mod_SOURCES = loader/i386/pc/bsd_normal.c
> +bsd_mod_CFLAGS = $(COMMON_CFLAGS)
> +bsd_mod_LDFLAGS = $(COMMON_LDFLAGS)

How much pc-dependant are those?  Can we hope they will work in e.g.
i386-coreboot in the future, when *BSD kernels remove their BIOS
dependency?

> +#define FREEBSD_RB_ASKNAME	0x1	/* ask for file name to reboot from */
> +#define FREEBSD_RB_SINGLE       0x2	/* reboot to single user only */
> +#define FREEBSD_RB_NOSYNC       0x4	/* dont sync before reboot */
> +#define FREEBSD_RB_HALT         0x8	/* don't reboot, just halt */
> +#define FREEBSD_RB_INITNAME     0x10	/* name given for /etc/init (unused) */
> +#define FREEBSD_RB_DFLTROOT     0x20	/* use compiled-in rootdev */
> +#define FREEBSD_RB_KDB          0x40	/* give control to kernel debugger */
> +#define FREEBSD_RB_RDONLY       0x80	/* mount root fs read-only */
> +#define FREEBSD_RB_DUMP         0x100	/* dump kernel memory before reboot */
> +#define FREEBSD_RB_MINIROOT     0x200	/* mini-root present in memory at boot time */
> +#define FREEBSD_RB_CONFIG       0x400	/* invoke user configuration routing */
> +#define FREEBSD_RB_VERBOSE      0x800	/* print all potentially useful info */
> +#define FREEBSD_RB_SERIAL       0x1000	/* user serial port as console */
> +#define FREEBSD_RB_CDROM        0x2000	/* use cdrom as root */
> +#define FREEBSD_RB_GDB		0x8000	/* use GDB remote debugger instead of DDB */
> +#define FREEBSD_RB_MUTE		0x10000	/* Come up with the console muted */
> +#define FREEBSD_RB_PAUSE	0x100000
> +#define FREEBSD_RB_QUIET	0x200000
> +#define FREEBSD_RB_NOINTR	0x10000000
> +#define FREENSD_RB_MULTIPLE	0x20000000 /* Use multiple consoles */
> +#define FREEBSD_RB_DUAL		FREENSD_RB_MULTIPLE
> +#define FREEBSD_RB_BOOTINFO     0x80000000 /* have `struct bootinfo *' arg */

Usually we have "(1 << 0)", "(1 << 1)", etc for bitmask lists.

> +FUNCTION(grub_unix_real_boot)
> +        call    EXT_C(grub_dl_unload_all)
> +        call    EXT_C(grub_stop_floppy)
> +
> +        cli
> +
> +        popl    %eax
> +        popl	%eax
> +        call	*%eax

Why a call?  Do we need the return address in the stack here?  A comment would
be nice in that case.

> +#define ALIGN_DWORD(a)	((a + 3) & (~3))
> +#define ALIGN_PAGE(a)	((a + 4095) & (~4095))

We already have ALIGN_UP:

  ./include/grub/misc.h:#define ALIGN_UP(addr, align) ((long)((char *)addr + align - 1) & ~(align - 1))

it'd be better to reuse that, I think.

> +static const grub_uint32_t freebsd_flags[] = {
> +  FREEBSD_RB_DUAL, FREEBSD_RB_SERIAL, FREEBSD_RB_ASKNAME,
> +  FREEBSD_RB_CDROM, FREEBSD_RB_CONFIG, FREEBSD_RB_KDB,
> +  FREEBSD_RB_GDB, FREEBSD_RB_MUTE, FREEBSD_RB_NOINTR,
> +  FREEBSD_RB_PAUSE, FREEBSD_RB_QUIET, FREEBSD_RB_DFLTROOT,
> +  FREEBSD_RB_SINGLE, FREEBSD_RB_VERBOSE
> +};

We usually put a newline after the '='.

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call… if you are unable to speak?
(as seen on /.)