Re: multiboot on EFI

[Robert Millan]

On Mon, Mar 23, 2009 at 01:29:55PM +0100, phcoder wrote:
> Hello. Here is an initial version of patch for booting multiboot kernels  
> on i386-efi. No Changelog yet because it's not for inclusion yet.

Very nice!

Would it be hard to split the patch and make it more granular?  I see it
implements base mmap / lsmmap support on efi, then ports the *BSD loaders
and the Multiboot loader too, and the uppermem facility.

>  pkglib_MODULES = kernel.mod normal.mod _chain.mod chain.mod appleldr.mod \
>       _linux.mod linux.mod halt.mod reboot.mod pci.mod lspci.mod \
> -     datetime.mod date.mod datehook.mod
> +     datetime.mod date.mod datehook.mod _multiboot.mod multiboot.mod
> +
> +# For _multiboot.mod.
> +_multiboot_mod_SOURCES = loader/i386/multiboot.c \
> +                      loader/i386/multiboot_helper.S \
> +                         loader/multiboot_loader.c
> +_multiboot_mod_CFLAGS = $(COMMON_CFLAGS)
> +_multiboot_mod_LDFLAGS = $(COMMON_LDFLAGS)
> +_multiboot_mod_ASFLAGS = $(COMMON_ASFLAGS)

If everybody's fine with it, I'd like to suggest adding stuff to pkglib_MODULES
in the same place as its corresponding variables.  I've done this already a few
times, and I think it makes the build system a bit more maintainable.  What do
you all think about this?

> diff --git a/include/grub/i386/pc/memory.h b/include/grub/i386/pc/memory.h
> index 08e92a9..e69ff77 100644
> --- a/include/grub/i386/pc/memory.h
> +++ b/include/grub/i386/pc/memory.h
> @@ -92,6 +92,8 @@ struct grub_machine_mmap_entry
>    grub_uint64_t len;
>  #define GRUB_MACHINE_MEMORY_AVAILABLE        1
>  #define GRUB_MACHINE_MEMORY_RESERVED 2
> +#define GRUB_MACHINE_MEMORY_ACPI     3
> +#define GRUB_MACHINE_MEMORY_NVS      4
>    grub_uint32_t type;
>  } __attribute__((packed));

Do we need specific knowledge of these two on i386-pc ?

>  /* The minimum and maximum heap size for GRUB itself.  */
>  #define MIN_HEAP_SIZE        0x100000
> -#define MAX_HEAP_SIZE        (16 * 0x100000)
> +#define MAX_HEAP_SIZE        (1600 * 0x100000)

Is 1600 MB what we want, or to remove the limit?

> +  /* Bubble-sort the memory map */
> +  while (done)
> +    {
> +      done = 0;
> +      for (i = 0; i < count - 1; i++)
> +     if (regions[i].start > regions[i + 1].start)
> +       {
> +         done = 1;
> +         t = regions[i];
> +         regions[i] = regions[i + 1];
> +         regions[i + 1] = t;
> +       }
> +    }

Do we need the memory map to be sorted?  AFAIK loadees can cope with unsorted
maps fine;  is there an exception?

> +#ifdef GRUB_MACHINE_PCBIOS
> +  grub_stop_floppy ();
> +#endif

grub_stop_floppy() doesn't do any BIOS-specific stuff.  Wouldn't __i386__
be more appropiate?

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."