Re: IA64 port

[Tristan Gingold]

On Mon, Jan 28, 2008 at 05:55:04PM +0100, Robert Millan wrote:
> 
> Hi Tristan!
[...]
> > This port deviate from other grub ports in modules:  I currently use a trick
> > to provide basic module support: they are prelinked during installation.
> > This makes the initial port easier (and possible other ports too).
> 
> Have you checked if this trick works on other ports?  Maybe it'd be a good 
> idea
> to merge this first.

I don't really understand what do you mean by 'works on other ports'.  It is
designed to be an optionnal feature used only by ia64.  Nothing IA64 specific
and other ports may use it.  If we go this way, it would be good to slightly
improve it.

> > +STRIP_FLAGS=--strip-unneeded -K grub_mod_init -K grub_mod_fini -R .note -R 
> > .comment 
> 
> Why this?  I recall strip was already run with those parameters.

This is the default value, overriden by ia64.  Maybe each port should define
it but I'd prefer not to touch other port (as I can't test all of them).

> >  RMKFILES = $(addprefix conf/,common.rmk i386-pc.rmk powerpc-ieee1275.rmk \
> > -   sparc64-ieee1275.rmk i386-efi.rmk)
> > +   sparc64-ieee1275.rmk i386-efi.rmk ia64-efi.rmk)
> 
> Oops, the two last ports I added are missing here.  I wonder if there's any 
> way
> to automate this part.  conf/*.rmk ?

Ok for *.rmk.

> > +static grub_uint32_t read16 (grub_uint8_t *p)
> > +{
> > +  return p[0] | (p[1] << 8);
> > +}
> > +
> > +static grub_uint32_t read32 (grub_uint8_t *p)
> > +{
> > +  return p[0] | (p[1] << 8) | (p[2] << 16) | (p[3] << 24);
> > +}
> > +
> > +static grub_uint64_t read64 (grub_uint8_t *p)
> > +{
> > +  grub_uint32_t l, h;
> > +  l = read32(p);
> > +  h = read32(p + 4);
> > +  return l | (((grub_uint64_t)h) << 32);
> > +}
> 
> You could use the endian conversion macros from grub/types.h

Ok.

> > +# For grub-emu.
> > +grub_emu_SOURCES = commands/boot.c commands/cat.c commands/cmp.c   \
> > +   commands/configfile.c commands/help.c                           \
> > +   commands/terminal.c commands/ls.c commands/test.c               \
> > +   commands/search.c commands/blocklist.c                          \
> > +   disk/loopback.c                                                 \
> > +   fs/affs.c fs/ext2.c fs/fat.c fs/fshelp.c fs/hfs.c fs/iso9660.c  \
> > +   fs/jfs.c fs/minix.c fs/sfs.c fs/ufs.c fs/xfs.c fs/hfsplus.c     \
> 
> Please could you resync the filesystem chunk to have the (recently changed)
> same layout as the other ports?  This will ease maintainance and prevent
> future mistakes.

Ok.

> > +# For memmap.mod.
> > +memmap_mod_SOURCES = commands/efi/memmap.c
> > +memmap_mod_CFLAGS = $(COMMON_CFLAGS)
> > +memmap_mod_LDFLAGS = $(COMMON_LDFLAGS)
> > +
> > +# For systab.mod.
> > +systab_mod_SOURCES = commands/efi/systab.c commands/efi/acpi.c
> > +systab_mod_CFLAGS = $(COMMON_CFLAGS)
> > +systab_mod_LDFLAGS = $(COMMON_LDFLAGS)
> 
> Does this work on i386-efi ?

Nothing IA64 specific and useless there are bugs it should work as is on
i386-efi.  That's mostly debug commands, not that useful.


> > diff -ruNp -x '*~' -x CVS grub2.orig/fs/fat.c grub2/fs/fat.c
> > --- grub2.orig/fs/fat.c     2007-08-02 20:40:36.000000000 +0200
> > +++ grub2/fs/fat.c  2008-01-28 16:29:57.000000000 +0100
> > @@ -568,7 +568,7 @@ grub_fat_find_dir (grub_disk_t disk, str
> >               continue;
> >             }
> >  
> > -         if (grub_strcmp (dirname, filename) == 0)
> > +         if (grub_strcasecmp (dirname, filename) == 0)
> >             {
> >               if (call_hook)
> >                 hook (filename, dir.attr & GRUB_FAT_ATTR_DIRECTORY);
> > @@ -601,7 +601,7 @@ grub_fat_find_dir (grub_disk_t disk, str
> >       if (hook (filename, dir.attr & GRUB_FAT_ATTR_DIRECTORY))
> >         break;
> >     }
> > -      else if (grub_strcmp (dirname, filename) == 0)
> > +      else if (grub_strcasecmp (dirname, filename) == 0)
> >     {
> >       if (call_hook)
> >         hook (filename, dir.attr & GRUB_FAT_ATTR_DIRECTORY);
> 
> Why is this needed?  I'm not sure if it's good to exploit this "unreliability"
> feature that fat provides us ;-)

On EFI, the prefix is extracted from an EFI path, whose case may not match
the FAT entries.  Without case insensitive comparaison, grub may not find
the prefix and thus refuses to go to normal mode.  Quiet boring and not easy
to understand for a beginner.

> > + *  GRUB  --  GRand Unified Bootloader
> > + *  Copyright (C) 2002,2003,2007  Free Software Foundation, Inc.
> 
> Please remember to update copyright years (in new or modified files).

Ok.

> > +__ia64_trampoline:
> > +   // Read address of the real descriptor
> 
> I think the consensus is to use /**/ comments in GRUB.

Ok.

> > diff -ruNp -x '*~' -x CVS grub2.orig/kern/rescue.c grub2/kern/rescue.c
> > --- grub2.orig/kern/rescue.c        2007-09-03 22:28:23.000000000 +0200
> > +++ grub2/kern/rescue.c     2008-01-28 16:29:58.000000000 +0100
> > @@ -659,6 +659,8 @@ grub_enter_rescue_mode (void)
> >  
> >        /* Get a command line.  */
> >        grub_rescue_get_command_line ("grub rescue> ");
> > +      if (line[0] == 0)
> > +   continue;
> 
> Great!  Finally somebody found that annoying bug ;-)

I can make a separate patch for this one, if you prefer.

> > +struct ia64_boot_param {
> 
> Please add a newline before opening brackets.

Ok.

> > +GRUB_MOD_INIT(linux_normal)
> > +{
> > +  (void) mod; /* To stop warning.  */
> > +  grub_register_command
> > +    ("linux", grub_normal_linux_command,
> > +     GRUB_COMMAND_FLAG_BOTH | GRUB_COMMAND_FLAG_NO_ARG_PARSE,
> > +     "linux FILE [ARGS...]",
> > +     "Load a linux kernel.", 0);
> > +  
> > +  grub_register_command
> > +    ("initrd", grub_normal_initrd_command,
> > +     GRUB_COMMAND_FLAG_BOTH | GRUB_COMMAND_FLAG_NO_ARG_PARSE,
> > +     "initrd FILE",
> > +     "Load an initrd.", 0);
> > +
> > +  grub_register_command
> > +    ("module", grub_normal_cmd_module,
> > +     GRUB_COMMAND_FLAG_BOTH | GRUB_COMMAND_FLAG_NO_ARG_PARSE,
> > +     "module FILE [ARGS...]",
> > +     "Load a Multiboot module.", 0);
> 
> Multiboot module loader in linux_normal.mod ?

Well, well well.  Long question :-)

Ia64 doesn't really fit in multiboot: it's a full 64 bits processor, there
may be no room for an header in the 8KB (or you have to waste a lot of memory
to keep alignment), no room for EFI pointers in the header and no Ia64 OS
uses it.  Well this were my conclusion when I read MB specs.

On the other hand I really need to have modules for Xen.

 
> Btw, this command is not unregistered.

Ok.

> > diff -ruNp -x '*~' -x CVS grub2.orig/util/ia64/efi/elf2pe.c 
> > grub2/util/ia64/efi/elf2pe.c
> > --- grub2.orig/util/ia64/efi/elf2pe.c       1970-01-01 01:00:00.000000000 
> > +0100
> > +++ grub2/util/ia64/efi/elf2pe.c    2008-01-28 16:29:58.000000000 +0100
[...]
> 
> This utility seems to be usable on i386 too?  In that case, better to put it
> outside ia64/ dir?

It should work on i386 too, but not tested.
I have written this utility for my port of EFI and adapted the style for
grub.  It may be nice to share this code with i386 but not really required
now IMHO.
 
> > diff -ruNp -x '*~' -x CVS grub2.orig/util/ia64/efi/grub-install.in 
> > grub2/util/ia64/efi/grub-install.in
> > --- grub2.orig/util/ia64/efi/grub-install.in        1970-01-01 
> > 01:00:00.000000000 +0100
> > +++ grub2/util/ia64/efi/grub-install.in     2008-01-28 16:29:58.000000000 
> > +0100
> 
> Any ia64-isms here, or just improvements (the module hack you described?)
> that could be shared with i386/efi/grub-install.in ?

Yes, the module hack makes ia64/grub-install.in very different from the
i386 one.

Tristan.