Re: [patch] PPC build fixes

[Marco Gerards]

Hollis Blanchard <address@hidden> writes:

> This patch fixes PowerPC build breakage from the recent grub-emu changes. Of
> note:
>
> - grub_reboot and grub_halt in util/i386/pc/misc.c are not
>   architecture-specific, so have been moved to util/grub-emu.c.
>
> - commands/ieee1275/halt.c and reboot.c are no longer
>   architecture-specific either. This is because we now want those
>   commands to be loaded in grub-emu, and of course grub-emu does not
>   emulate Open Firmware. Accordingly, these files are moved up to the
>   commands directory. Since i386 needs a special argument ("no-apm"), it
>   still uses its version in commands/i386/pc.
>
> - since grub_machine_fini isn't called yet, I haven't yet figured out
>   how to release the memory indicated in the comments.
>
> OK?
>
> -Hollis
>
> 2005-02-19  Hollis Blanchard  <address@hidden>
>
>       * commands/ieee1275/halt.c (grub_cmd_halt): Call grub_halt.
>       Move file...
>       * commands/halt.c: ... to here.
>       * commands/ieee1275/reboot.c (grub_cmd_reboot): Call grub_reboot.
>       Move file...
>       * commands/reboot.c: ... to here.

If you do this, you could delete the PC specific reboot and halt
commands.  Personally I like separate reboot and halt commands.  If
you do it like this it will become hard/ugly to make platform specific
changes, I think.

>       * disk/powerpc/ieee1275/ofdisk.c (grub_ofdisk_fini): New
>       function.
>       * include/grub/powerpc/ieee1275/ieee1275.h (grub_reboot): Add
>       prototype.

Better put this in `include/grub/powerpc/ieee1275/init.h', just like
it is done for the PC.  Perhaps it would be even better to have a new
header file for such things.

>       (grub_halt): Likewise.
>       * kern/powerpc/ieee1275/init.c (heap_start): Make global.
>       (heap_len): Likewise.

`New variable.' would be more appropriate, I think.  Can you put a
prefix before the function name?

>       (grub_machine_fini): New function.

It's really nice that we have this now. :)

>       * kern/powerpc/ieee1275/openfw.c (grub_reboot): New function.
>       (grub_halt): Likewise.
>       * term/powerpc/ieee1275/ofconsole.c (grub_ofconsole_fini): New
>       function.

You have added grub_console_fini, not grub_ofconsole_fini.

>  void
> +grub_machine_fini (void)
> +{
> +  grub_ofdisk_fini ();
> +  grub_console_fini ();
> +
> +  grub_ieee1275_release (heap_start, heap_len);
> +  /* XXX Release memory claimed in 'linux' and 'initrd' commands.  */
> +  /* XXX Release memory claimed for Old World firmware.  */
> +}

Why would you release the memory for linux and initrd?  IIRC there is
a callback function.

I think it should work like this:

In case someone quits GRUB:

- longjump (?)
- Release the memory for the loader (use grub_loader_unset).
- Call grub_console_fini.
- Call grub_ofdisk_fini.
- Release the heap.

In case someone starts a loader the same should be done, except
calling grub_loader_unset.  After that the OS should be loaded.  I
wonder what should happen when the OS can not be loaded.  Perhaps GRUB
just has to fail or so.

Thanks,
Marco