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

[Robert Millan]

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 /.)