Re: IA64 port

[Robert Millan <rmh@aybabtu.com>]

Hi Tristan!

On Mon, Jan 28, 2008 at 05:09:05PM +0100, Tristan Gingold wrote:
> Hi,
> 
> here is the patch to add support for ia64.  This is mostly new files
> (as well as new commands used to debug), and a few fixes in kern/efi/mm.c and
> fs/fat.c.
> 
> Ia64 uses EFI so this port leverage on the already existing EFI support.
> 
> 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.

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

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

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

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

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

> 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 ;-)

> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2002,2003,2007  Free Software Foundation, Inc.

Please remember to update copyright years (in new or modified files).

> +__ia64_trampoline:
> +	// Read address of the real descriptor

I think the consensus is to use /**/ comments in GRUB.

> 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 ;-)

> +struct ia64_boot_param {

Please add a newline before opening brackets.

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

Btw, this command is not unregistered.

> 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
> @@ -0,0 +1,812 @@
> +/* elf2pe.c - convert elf binary to PE/Coff.  */
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2008  Free Software Foundation, Inc.
> + *
> + *  GRUB is free software: you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation, either version 3 of the License, or
> + *  (at your option) any later version.
> + *
> + *  GRUB is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +#include <stdint.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <time.h>
> +#include <elf.h>
> +
> +#if defined(i386)
> +#define USE_ELF32
> +#define USE_PE32
> +#define ELF_MACHINE EM_386
> +#define EFI_MACHINE PE32_MACHINE_I386
> +#elif defined(__ia64__)
> +#define USE_ELF64
> +#define USE_PE32PLUS
> +#define ELF_MACHINE EM_IA_64
> +#define EFI_MACHINE PE32_MACHINE_IA64
> +#else
> +#error "unknown architecture"
> +#endif

This utility seems to be usable on i386 too?  In that case, better to put it
outside ia64/ dir?

> --- grub2.orig/util/ia64/efi/pe32.h	1970-01-01 01:00:00.000000000 +0100
> +++ grub2/util/ia64/efi/pe32.h	2008-01-28 16:29:58.000000000 +0100

(same here, I assume)

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

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