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

[Bean]

On Feb 12, 2008 5:28 AM, Robert Millan <address@hidden> wrote:
> 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 ;-)

oh, yes.

>
> > +_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?

freebsd doesn't need bios at all, openbsd need the memory map, but i
guess it's easy to construct one without using int 15.

>
> > +#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.

ok.

>
> > +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.

bsds using stack to pass parameter, it's like this:

void __cdecl (*func)(parm1, parm2,...);

the first pop remove the return address,  the second gets the address
of func, and then call it. the function never returns, using call just
make sure the first parameter is at esp + 4.

>
> > +#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 '='.

ok.

-- 
Bean