Re: grub-mkimage and module loading

Re: grub-mkimage and module loading

[Hollis Blanchard]

This is a more complete patch for PPC grub-mkimage that I posted a while
back. Important points:
- move grub_load_modules() into i386/pc as grub_arch_load_modules()
- implement PPC's grub_arch_load_modules() using an in-memory structure
  telling us how many modules to expect
- the structure contains a version field to avoid grub-mkimage/grub
  binary layout mismatches
- util/powerpc/ieee1275/grub-mkimage.c is a fair amount of code, but
  it's used almost exactly like the i386 version
- added a couple helper functions to the util/misc.c code

I think there is still some PC cleanup needed, e.g. grub_end_addr and
grub_add_unused_region() should also be moved into i386 code, but my
patch is already fairly large, and I'd prefer a PC person were involved
in this additional cleanup anyways.

This patch has been tested and is needed to load modules on PPC. Please
comment soon so we can get PPC modules working in the main tree...

-Hollis

Index: boot/powerpc/ieee1275/crt0.S
===================================================================
RCS file: /cvsroot/grub/grub2/boot/powerpc/ieee1275/crt0.S,v
retrieving revision 1.4
diff -u -p -r1.4 crt0.S
--- boot/powerpc/ieee1275/crt0.S	28 Dec 2004 22:43:37 -0000	1.4
+++ boot/powerpc/ieee1275/crt0.S	2 Jan 2005 19:01:50 -0000
@@ -18,21 +18,6 @@
  *  Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
  */
 
-	.section ".note"
-	.align	2
-.note_section_header:
-	.long	8
-	.long	24
-	.long	0x1275
-	.string	"PowerPC"
-.note_descriptor:
-	.long   0x0             /* real-mode */
-	.long   0xffffffff      /* real-base */
-	.long   0xffffffff      /* real-size */
-	.long   0xffffffff      /* virt-base */
-	.long   0xffffffff      /* virt-size */
-	.long   0x00030000      /* load-base */
-
 .extern __bss_start
 .extern _end
 
Index: conf/powerpc-ieee1275.rmk
===================================================================
RCS file: /cvsroot/grub/grub2/conf/powerpc-ieee1275.rmk,v
retrieving revision 1.18
diff -u -p -r1.18 powerpc-ieee1275.rmk
--- conf/powerpc-ieee1275.rmk	28 Dec 2004 22:43:37 -0000	1.18
+++ conf/powerpc-ieee1275.rmk	2 Jan 2005 19:01:53 -0000
@@ -24,9 +24,13 @@ kernel_syms.lst: $(addprefix include/gru
 pkgdata_PROGRAMS = grubof
 
 # Utilities.
-bin_UTILITIES = grub-emu
+bin_UTILITIES = grub-emu grub-mkimage
 noinst_UTILITIES = genmoddep
 
+# For grub-mkimage.
+grub_mkimage_SOURCES = util/powerpc/ieee1275/grub-mkimage.c util/misc.c \
+        util/resolve.c 
+
 # For grub-emu
 grub_emu_SOURCES = kern/main.c kern/device.c				\
 	kern/disk.c kern/dl.c kern/file.c kern/fs.c kern/err.c		\
Index: include/grub/kernel.h
===================================================================
RCS file: /cvsroot/grub/grub2/include/grub/kernel.h,v
retrieving revision 1.4
diff -u -p -r1.4 kernel.h
--- include/grub/kernel.h	4 Apr 2004 13:46:00 -0000	1.4
+++ include/grub/kernel.h	2 Jan 2005 19:01:53 -0000
@@ -31,8 +31,14 @@ struct grub_module_header
   grub_size_t size;
 };
 
-/* The start address of the kernel.  */
-extern grub_addr_t grub_start_addr;
+#define MODINFO_VERSION 1
+struct grub_module_info {
+  grub_uint32_t version;
+  grub_uint32_t num_modules;
+  struct grub_module_header module_data[0];
+};
+
+extern void grub_arch_load_modules (void);
 
 /* The end address of the kernel.  */
 extern grub_addr_t grub_end_addr;
@@ -49,9 +55,6 @@ void grub_main (void);
 /* The machine-specific initialization. This must initialize memory.  */
 void grub_machine_init (void);
 
-/* Return the end address of the core image.  */
-grub_addr_t grub_get_end_addr (void);
-
 /* Register all the exported symbols. This is automatically generated.  */
 void grub_register_exported_symbols (void);
 
Index: include/grub/util/misc.h
===================================================================
RCS file: /cvsroot/grub/grub2/include/grub/util/misc.h,v
retrieving revision 1.5
diff -u -p -r1.5 misc.h
--- include/grub/util/misc.h	4 Apr 2004 13:46:01 -0000	1.5
+++ include/grub/util/misc.h	2 Jan 2005 19:01:53 -0000
@@ -34,9 +34,13 @@ void *xrealloc (void *ptr, size_t size);
 char *xstrdup (const char *str);
 
 char *grub_util_get_path (const char *dir, const char *file);
+size_t grub_util_get_fp_size (FILE *fp);
 size_t grub_util_get_image_size (const char *path);
+void grub_util_read_at (void *img, size_t len, off_t offset, FILE *fp);
 char *grub_util_read_image (const char *path);
 void grub_util_load_image (const char *path, char *buf);
 void grub_util_write_image (const char *img, size_t size, FILE *out);
+void grub_util_write_image_at (const void *img, size_t size, off_t offset,
+			       FILE *out);
 
 #endif /* ! GRUB_UTIL_MISC_HEADER */
Index: kern/main.c
===================================================================
RCS file: /cvsroot/grub/grub2/kern/main.c,v
retrieving revision 1.8
diff -u -p -r1.8 main.c
--- kern/main.c	4 Apr 2004 13:46:01 -0000	1.8
+++ kern/main.c	2 Jan 2005 19:01:53 -0000
@@ -29,29 +29,6 @@
 #include <grub/device.h>
 #include <grub/env.h>
 
-/* Return the end of the core image.  */
-grub_addr_t
-grub_get_end_addr (void)
-{
-  return grub_total_module_size + grub_end_addr;
-}
-
-/* Load all modules in core.  */
-static void
-grub_load_modules (void)
-{
-  struct grub_module_header *header;
-
-  for (header = (struct grub_module_header *) grub_end_addr;
-       header < (struct grub_module_header *) grub_get_end_addr ();
-       header = (struct grub_module_header *) ((char *) header + header->size))
-    {
-      if (! grub_dl_load_core ((char *) header + header->offset,
-			       (header->size - header->offset)))
-	grub_fatal ("%s", grub_errmsg);
-    }
-}
-
 /* Add the region where modules reside into dynamic memory.  */
 static void
 grub_add_unused_region (void)
@@ -110,7 +87,7 @@ grub_main (void)
 
   /* Load pre-loaded modules and free the space.  */
   grub_register_exported_symbols ();
-  grub_load_modules ();
+  grub_arch_load_modules ();
   grub_add_unused_region ();
 
   /* Load the normal mode module.  */
Index: kern/i386/pc/init.c
===================================================================
RCS file: /cvsroot/grub/grub2/kern/i386/pc/init.c,v
retrieving revision 1.9
diff -u -p -r1.9 init.c
--- kern/i386/pc/init.c	27 Dec 2004 13:46:20 -0000	1.9
+++ kern/i386/pc/init.c	2 Jan 2005 19:01:53 -0000
@@ -233,3 +233,26 @@ grub_machine_init (void)
   /* Initialize the prefix.  */
   grub_env_set ("prefix", make_install_device ());
 }
+
+/* Return the end of the core image.  */
+static grub_addr_t
+grub_get_end_addr (void)
+{
+  return grub_total_module_size + grub_end_addr;
+}
+
+/* Load all modules in core.  */
+void
+grub_arch_load_modules (void)
+{
+  struct grub_module_header *header;
+
+  for (header = (struct grub_module_header *) grub_end_addr;
+       header < (struct grub_module_header *) grub_get_end_addr ();
+       header = (struct grub_module_header *) ((char *) header + header->size))
+    {
+      if (! grub_dl_load_core ((char *) header + header->offset,
+			       (header->size - header->offset)))
+	grub_fatal ("%s", grub_errmsg);
+    }
+}
Index: kern/powerpc/ieee1275/init.c
===================================================================
RCS file: /cvsroot/grub/grub2/kern/powerpc/ieee1275/init.c,v
retrieving revision 1.11
diff -u -p -r1.11 init.c
--- kern/powerpc/ieee1275/init.c	27 Dec 2004 13:46:20 -0000	1.11
+++ kern/powerpc/ieee1275/init.c	2 Jan 2005 19:01:53 -0000
@@ -31,6 +31,7 @@
 #include <grub/misc.h>
 #include <grub/machine/init.h>
 #include <grub/machine/time.h>
+#include <grub/machine/kernel.h>
 
 /* XXX: Modules are not yet supported.  */
 grub_addr_t grub_end_addr = -1;
@@ -84,3 +85,25 @@ grub_get_rtc (void)
 {
   return 0;
 }
+
+/* Load all modules in core.  */
+void
+grub_arch_load_modules (void)
+{
+  struct grub_module_info *base = (struct grub_module_info *) MODULE_BASE;
+  struct grub_module_header *header = base->module_data;
+  unsigned int i;
+
+  if (base->version != MODINFO_VERSION)
+    grub_fatal ("module version mismatch: wanted %d; found %d\n",
+		MODINFO_VERSION, base->version);
+
+  for (i = 0; i < base->num_modules; i++)
+  {
+    if (! grub_dl_load_core ((char *) header + header->offset,
+	  (header->size - header->offset)))
+      grub_fatal ("%s", grub_errmsg);
+    header = (struct grub_module_header *) ((char *) header + header->size);
+  }
+}
+
Index: util/misc.c
===================================================================
RCS file: /cvsroot/grub/grub2/util/misc.c,v
retrieving revision 1.9
diff -u -p -r1.9 misc.c
--- util/misc.c	27 Dec 2004 13:46:20 -0000	1.9
+++ util/misc.c	2 Jan 2005 19:01:53 -0000
@@ -25,6 +25,7 @@
 #include <sys/stat.h>
 #include <sys/times.h>
 #include <malloc.h>
+#include <unistd.h>
 
 #include <grub/util/misc.h>
 #include <grub/mm.h>
@@ -107,6 +108,20 @@ grub_util_get_path (const char *dir, con
 }
 
 size_t
+grub_util_get_fp_size (FILE *fp)
+{
+  struct stat st;
+  
+  if (fflush (fp) == EOF)
+    grub_util_error ("fflush failed");
+
+  if (fstat (fileno (fp), &st) == -1)
+    grub_util_error ("fstat failed");
+  
+  return st.st_size;
+}
+
+size_t
 grub_util_get_image_size (const char *path)
 {
   struct stat st;
@@ -119,6 +134,16 @@ grub_util_get_image_size (const char *pa
   return st.st_size;
 }
 
+void
+grub_util_read_at (void *img, size_t size, off_t offset, FILE *fp)
+{
+  if (fseek (fp, offset, SEEK_SET) == -1)
+    grub_util_error ("fseek failed");
+
+  if (fread (img, 1, size, fp) != size)
+    grub_util_error ("read failed");
+}
+
 char *
 grub_util_read_image (const char *path)
 {
@@ -134,9 +159,8 @@ grub_util_read_image (const char *path)
   fp = fopen (path, "rb");
   if (! fp)
     grub_util_error ("cannot open %s", path);
-  
-  if (fread (img, 1, size, fp) != size)
-    grub_util_error ("cannot read %s", path);
+
+  grub_util_read_at (img, size, 0, fp);
 
   fclose (fp);
   
@@ -164,13 +188,21 @@ grub_util_load_image (const char *path, 
 }
 
 void
-grub_util_write_image (const char *img, size_t size, FILE *out)
+grub_util_write_image_at (const void *img, size_t size, off_t offset, FILE *out)
 {
-  grub_util_info ("writing 0x%x bytes", size);
+  grub_util_info ("writing 0x%x bytes at offset 0x%x", size, offset);
+  if (fseek (out, offset, SEEK_SET) == -1)
+    grub_util_error ("seek failed");
   if (fwrite (img, 1, size, out) != size)
     grub_util_error ("write failed");
 }
 
+void
+grub_util_write_image (const char *img, size_t size, FILE *out)
+{
+  grub_util_write_image_at (img, size, 0, out);
+}
+
 void *
 grub_malloc (unsigned size)
 {
Index: util/powerpc/ieee1275/grub-mkimage.c
===================================================================
RCS file: util/powerpc/ieee1275/grub-mkimage.c
diff -N util/powerpc/ieee1275/grub-mkimage.c
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ util/powerpc/ieee1275/grub-mkimage.c	2 Jan 2005 19:05:06 -0000
@@ -0,0 +1,296 @@
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2004  Free Software Foundation, Inc.
+ *
+ *  This program 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 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program 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 this program; if not, write to the Free Software
+ *  Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+/* TODO: endianness */
+
+#include <stdio.h>
+#include <stdint.h>
+#include <unistd.h>
+#include <fcntl.h>
+#include <getopt.h>
+#include <stdlib.h>
+#include <grub/elf.h>
+#include <grub/util/misc.h>
+#include <grub/util/resolve.h>
+#include <grub/kernel.h>
+#include <grub/machine/kernel.h>
+
+#define ALIGN_UP(addr, align) ((long)((char *)addr + align - 1) & ~(align - 1))
+
+static char *kernel_path = "grubof";
+static char *note_path = "note";
+
+
+void load_note (Elf32_Phdr *phdr, const char *dir, FILE *out)
+{
+  char *note_img;
+  char *path;
+  int note_size;
+
+  grub_util_info ("adding CHRP NOTE segment");
+
+  path = grub_util_get_path (dir, note_path);
+  note_size = grub_util_get_image_size (path);
+  note_img = xmalloc (note_size);
+  grub_util_load_image (path, note_img);
+  free (path);
+
+  /* Write the note data to the new segment.  */
+  grub_util_write_image_at (note_img, note_size, phdr->p_offset, out);
+
+  /* Fill in the rest of the segment header.  */
+  phdr->p_type = PT_NOTE;
+  phdr->p_flags = PF_R;
+  phdr->p_align = sizeof (long);
+  phdr->p_vaddr = 0;
+  phdr->p_paddr = 0;
+  phdr->p_filesz = note_size;
+  phdr->p_memsz = 0;
+}
+
+void load_modules (Elf32_Phdr *phdr, const char *dir, char *mods[], FILE *out)
+{
+  char *module_img;
+  struct grub_util_path_list *path_list, *p;
+  struct grub_module_info *modinfo;
+  size_t offset;
+  size_t total_module_size;
+  int num_modules = 0;
+
+  path_list = grub_util_resolve_dependencies (dir, "moddep.lst", mods);
+
+  offset = sizeof (struct grub_module_info);
+  total_module_size = sizeof (struct grub_module_info);
+  for (p = path_list; p; p = p->next)
+    {
+      total_module_size += (grub_util_get_image_size (p->name)
+	  + sizeof (struct grub_module_header));
+      num_modules++;
+    }
+
+  grub_util_info ("the total module size is 0x%x", total_module_size);
+
+  module_img = xmalloc (total_module_size);
+  modinfo = (struct grub_module_info *) module_img;
+  modinfo->version = MODINFO_VERSION;
+  modinfo->num_modules = num_modules;
+
+  /* Load all the modules, with headers, into module_img.  */
+  for (p = path_list; p; p = p->next)
+    {
+      struct grub_module_header *header;
+      size_t mod_size;
+
+      grub_util_info ("adding module %s", p->name);
+
+      mod_size = grub_util_get_image_size (p->name);
+
+      header = (struct grub_module_header *) (module_img + offset);
+      header->offset = grub_cpu_to_be32 (sizeof (*header));
+      header->size = grub_cpu_to_be32 (mod_size + sizeof (*header));
+
+      grub_util_load_image (p->name, module_img + offset + sizeof (*header));
+
+      offset += sizeof (*header) + mod_size;
+    }
+
+  /* Write the module data to the new segment.  */
+  grub_util_write_image_at (module_img, total_module_size, phdr->p_offset, out);
+
+  /* Fill in the rest of the segment header.  */
+  phdr->p_type = PT_LOAD;
+  phdr->p_flags = PF_R | PF_W | PF_X;
+  phdr->p_align = sizeof (long);
+  phdr->p_vaddr = MODULE_BASE;
+  phdr->p_paddr = MODULE_BASE;
+  phdr->p_filesz = total_module_size;
+  phdr->p_memsz = total_module_size;
+}
+
+void add_segments (char *dir, FILE *out, int chrp, char *mods[])
+{
+  Elf32_Ehdr ehdr;
+  Elf32_Phdr *phdrs = NULL;
+  Elf32_Phdr *phdr;
+  FILE *in;
+  off_t phdroff;
+  int i;
+
+  /* Read ELF header.  */
+  in = fopen (kernel_path, "rb");
+  if (! in)
+    grub_util_error ("cannot open %s", kernel_path);
+  grub_util_read_at (&ehdr, sizeof (ehdr), 0, in);
+
+  phdrs = xmalloc (ehdr.e_phentsize * (ehdr.e_phnum + 2));
+
+  /* Copy all existing segments.  */
+  for (i = 0; i < ehdr.e_phnum; i++)
+    {
+      char *segment_img;
+
+      phdr = phdrs + i;
+
+      /* Read segment header.  */
+      grub_util_read_at (phdr, sizeof (Elf32_Phdr), ehdr.e_phoff
+			 + (i * ehdr.e_phentsize), in);
+      grub_util_info ("copying segment %d, type %d", i, phdr->p_type);
+
+      /* Read segment data and write it to new file.  */
+      segment_img = xmalloc (phdr->p_filesz);
+      grub_util_read_at (segment_img, phdr->p_filesz, phdr->p_offset, in);
+      grub_util_write_image_at (segment_img, phdr->p_filesz, phdr->p_offset, out);
+
+      free (segment_img);
+    }
+
+  if (mods[0] != NULL)
+    {
+      /* Construct new segment header for modules.  */
+      phdr = phdrs + ehdr.e_phnum;
+      ehdr.e_phnum++;
+
+      /* Fill in p_offset so the callees know where to write.  */
+      phdr->p_offset = ALIGN_UP (grub_util_get_fp_size (out), sizeof (long));
+
+      load_modules (phdr, dir, mods, out);
+    }
+
+  if (chrp)
+    {
+      /* Construct new segment header for the CHRP note.  */
+      phdr = phdrs + ehdr.e_phnum;
+      ehdr.e_phnum++;
+
+      /* Fill in p_offset so the callees know where to write.  */
+      phdr->p_offset = ALIGN_UP (grub_util_get_fp_size (out), sizeof (long));
+
+      load_note (phdr, dir, out);
+    }
+
+  /* Don't bother preserving the section headers.  */
+  ehdr.e_shoff = 0;
+  ehdr.e_shnum = 0;
+  ehdr.e_shstrndx = 0;
+
+  /* Append entire segment table to the file.  */
+  phdroff = ALIGN_UP (grub_util_get_fp_size (out), sizeof (long));
+  grub_util_write_image_at (phdrs, ehdr.e_phentsize * ehdr.e_phnum, phdroff,
+			    out);
+
+  /* Write ELF header.  */
+  ehdr.e_phoff = phdroff;
+  grub_util_write_image_at (&ehdr, sizeof (ehdr), 0, out);
+
+  free (phdrs);
+}
+
+static struct option options[] =
+  {
+    {"directory", required_argument, 0, 'd'},
+    {"output", required_argument, 0, 'o'},
+    {"help", no_argument, 0, 'h'},
+    {"note", no_argument, 0, 'n'},
+    {"version", no_argument, 0, 'V'},
+    {"verbose", no_argument, 0, 'v'},
+    { 0, 0, 0, 0 },
+  };
+
+static void
+usage (int status)
+{
+  if (status)
+    fprintf (stderr, "Try ``grub-mkimage --help'' for more information.\n");
+  else
+    printf ("\
+Usage: grub-mkimage -o FILE [OPTION]... [MODULES]\n\
+\n\
+Make a bootable image of GRUB.\n\
+\n\
+-d, --directory=DIR     use images and modules under DIR [default=%s]\n\
+-o, --output=FILE       output a generated image to FILE\n\
+-h, --help              display this message and exit\n\
+-n, --note              add NOTE segment for CHRP Open Firmware\n\
+-V, --version           print version information and exit\n\
+-v, --verbose           print verbose messages\n\
+\n\
+Report bugs to <%s>.\n\
+", GRUB_DATADIR, PACKAGE_BUGREPORT);
+
+  exit (status);
+}
+
+int main (int argc, char *argv[])
+{
+  FILE *fp;
+  char *output = NULL;
+  char *dir = NULL;
+  int chrp = 0;
+
+  progname = "grub-mkimage";
+
+  while (1)
+    {
+      int c = getopt_long (argc, argv, "d:o:hVvn", options, 0);
+      if (c == -1)
+	break;
+
+      switch (c)
+	{
+	  case 'd':
+	    if (dir)
+	      free (dir);
+	    dir = xstrdup (optarg);
+	    break;
+	  case 'h':
+	    usage (0);
+	    break;
+	  case 'n':
+	    chrp = 1;
+	    break;
+	  case 'o':
+	    if (output)
+	      free (output);
+	    output = xstrdup (optarg);
+	    break;
+	  case 'V':
+	    printf ("grub-mkimage (%s) %s\n", PACKAGE_NAME, PACKAGE_VERSION);
+	    return 0;
+	  case 'v':
+	    verbosity++;
+	    break;
+	  default:
+	    usage (1);
+	    break;
+	}
+  }
+
+  if (!output)
+    usage (1);
+
+  fp = fopen (output, "wb");
+  if (! fp)
+    grub_util_error ("cannot open %s", output);
+
+  add_segments (dir ? : GRUB_DATADIR, fp, chrp, argv + optind);
+
+  fclose (fp);
+
+  return 0;
+}
Index: include/grub/powerpc/ieee1275/kernel.h
===================================================================
RCS file: include/grub/powerpc/ieee1275/kernel.h
diff -N include/grub/powerpc/ieee1275/kernel.h
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ include/grub/powerpc/ieee1275/kernel.h	2 Jan 2005 19:06:04 -0000
@@ -0,0 +1,26 @@
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2002  Free Software Foundation, Inc.
+ *
+ *  This program 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 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program 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 this program; if not, write to the Free Software
+ *  Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#ifndef GRUB_KERNEL_MACHINE_HEADER
+#define GRUB_KERNEL_MACHINE_HEADER	1
+
+/* Where grub-mkimage places the core modules in memory.  */
+#define MODULE_BASE 0x00300000
+
+#endif

Re: grub-mkimage and module loading

[Marco Gerards]

Hollis Blanchard <hollis@penguinppc.org> writes:

> This is a more complete patch for PPC grub-mkimage that I posted a while
> back. Important points:
> - move grub_load_modules() into i386/pc as grub_arch_load_modules()
> - implement PPC's grub_arch_load_modules() using an in-memory structure
>   telling us how many modules to expect

I would prefer a grub_load_modules and a function to return the
address and size of the modules in memory.  I explained this in my
other email.

Anyway, I'd prefer this to work the same on every machine and this
code to be shared.

> - the structure contains a version field to avoid grub-mkimage/grub
>   binary layout mismatches

I don't think versioning is required.

> I think there is still some PC cleanup needed, e.g. grub_end_addr and
> grub_add_unused_region() should also be moved into i386 code, but my
> patch is already fairly large, and I'd prefer a PC person were involved
> in this additional cleanup anyways.

Ok.  Can you at least make sure it compiles on the PC and try not to
break it?

> This patch has been tested and is needed to load modules on PPC. Please
> comment soon so we can get PPC modules working in the main tree...

I assume you will make a changelog entry when the patch is more or
less ok?  It's easier to review a patch using a changelog entry.

> Index: include/grub/kernel.h
> ===================================================================
> RCS file: /cvsroot/grub/grub2/include/grub/kernel.h,v
> retrieving revision 1.4
> diff -u -p -r1.4 kernel.h
> --- include/grub/kernel.h	4 Apr 2004 13:46:00 -0000	1.4
> +++ include/grub/kernel.h	2 Jan 2005 19:01:53 -0000
> @@ -31,8 +31,14 @@ struct grub_module_header
>    grub_size_t size;
>  };
>  
> -/* The start address of the kernel.  */
> -extern grub_addr_t grub_start_addr;

...

> -/* Return the end address of the core image.  */
> -grub_addr_t grub_get_end_addr (void);

Doesn't this break the PC port?

> +/* TODO: endianness */

Better move this to the TODO list.

> +void load_note (Elf32_Phdr *phdr, const char *dir, FILE *out)

GCS nitpick: put the return type on a separate line:

void
load_note (Elf32_Phdr *phdr, const char *dir, FILE *out)

Same for all other functions, btw.

> +void load_modules (Elf32_Phdr *phdr, const char *dir, char *mods[], FILE *out)
> +{
> +  char *module_img;
> +  struct grub_util_path_list *path_list, *p;

Please use separate lines for every declaration:

struct grub_util_path_list *path_list;
struct grub_util_path_list *p;

Thanks,
Marco

Re: grub-mkimage and module loading

[Hollis Blanchard]

On Jan 2, 2005, at 3:05 PM, Marco Gerards wrote:

> Hollis Blanchard <hollis@penguinppc.org> writes:
>
>> This is a more complete patch for PPC grub-mkimage that I posted a 
>> while
>> back. Important points:
>> - move grub_load_modules() into i386/pc as grub_arch_load_modules()
>> - implement PPC's grub_arch_load_modules() using an in-memory 
>> structure
>>   telling us how many modules to expect
>
> I would prefer a grub_load_modules and a function to return the
> address and size of the modules in memory.  I explained this in my
> other email.

I suggested the same thing. But I'd really rather have someone who 
understands the PC side to make such changes. I don't have the 
equipment or the interest to rework the PC code. This patch has almost 
no impact on the PC side; sharing the code will be more invasive.

> Anyway, I'd prefer this to work the same on every machine and this
> code to be shared.

I understand. I will see what I can do.

>> - the structure contains a version field to avoid grub-mkimage/grub
>>   binary layout mismatches
>
> I don't think versioning is required.

Fine.

>> I think there is still some PC cleanup needed, e.g. grub_end_addr and
>> grub_add_unused_region() should also be moved into i386 code, but my
>> patch is already fairly large, and I'd prefer a PC person were 
>> involved
>> in this additional cleanup anyways.
>
> Ok.  Can you at least make sure it compiles on the PC and try not to
> break it?

I have tried not to break it. Even compile-testing is going to be 
pretty inconvenient I think, but I'll see...
>
>> This patch has been tested and is needed to load modules on PPC. 
>> Please
>> comment soon so we can get PPC modules working in the main tree...
>
> I assume you will make a changelog entry when the patch is more or
> less ok?  It's easier to review a patch using a changelog entry.

Yes, but there was no sense in me making a changelog description when 
the patch was just going to be ripped apart anyways. :)

>> Index: include/grub/kernel.h
>> ===================================================================
>> RCS file: /cvsroot/grub/grub2/include/grub/kernel.h,v
>> retrieving revision 1.4
>> diff -u -p -r1.4 kernel.h
>> --- include/grub/kernel.h	4 Apr 2004 13:46:00 -0000	1.4
>> +++ include/grub/kernel.h	2 Jan 2005 19:01:53 -0000
>> @@ -31,8 +31,14 @@ struct grub_module_header
>>    grub_size_t size;
>>  };
>>
>> -/* The start address of the kernel.  */
>> -extern grub_addr_t grub_start_addr;
>
> ...
>
>> -/* Return the end address of the core image.  */
>> -grub_addr_t grub_get_end_addr (void);
>
> Doesn't this break the PC port?

grub_get_end_addr() has only a single caller, and was moved with that 
caller.

grub_start_addr is never used anywhere.

>> +/* TODO: endianness */
>
> Better move this to the TODO list.

Ok.

>> +void load_note (Elf32_Phdr *phdr, const char *dir, FILE *out)
>
> GCS nitpick: put the return type on a separate line:
>
> void
> load_note (Elf32_Phdr *phdr, const char *dir, FILE *out)
>
> Same for all other functions, btw.

Ok.

>> +void load_modules (Elf32_Phdr *phdr, const char *dir, char *mods[], 
>> FILE *out)
>> +{
>> +  char *module_img;
>> +  struct grub_util_path_list *path_list, *p;
>
> Please use separate lines for every declaration:

Ok.

-Hollis

Re: grub-mkimage and module loading

[Marco Gerards]

Hollis Blanchard <hollis@penguinppc.org> writes:

> I suggested the same thing. But I'd really rather have someone who
> understands the PC side to make such changes. I don't have the
> equipment or the interest to rework the PC code. This patch has almost
> no impact on the PC side; sharing the code will be more invasive.

Ok. I can do that, no problem.  In that case, please don't move the
function out of kern/main.c but just write the PPC parts.  In that
case breaking the PC port is not a problem because I will immediately
start fixing.  I can apply the patches short after each other so there
will not be any problem.

>> Anyway, I'd prefer this to work the same on every machine and this
>> code to be shared.
>
> I understand. I will see what I can do.

Leave that to me as well if that is more comfortable for you.

>> I assume you will make a changelog entry when the patch is more or
>> less ok?  It's easier to review a patch using a changelog entry.
>
> Yes, but there was no sense in me making a changelog description when
> the patch was just going to be ripped apart anyways. :)

LOL!

Thanks,
Marco

Re: Pentium II boot failure

[Hollis Blanchard]

On Jan 2, 2005, at 4:45 PM, Marco Gerards wrote:

> Hollis Blanchard <hollis@penguinppc.org> writes:
>
>> I suggested the same thing. But I'd really rather have someone who
>> understands the PC side to make such changes. I don't have the
>> equipment or the interest to rework the PC code. This patch has almost
>> no impact on the PC side; sharing the code will be more invasive.
>
> Ok. I can do that, no problem.  In that case, please don't move the
> function out of kern/main.c but just write the PPC parts.  In that
> case breaking the PC port is not a problem because I will immediately
> start fixing.  I can apply the patches short after each other so there
> will not be any problem.

I pulled a Pentium II out of the closet for testing, but unfortunately 
even stock CVS failed to boot to the GRUB prompt. I did a build, then
	./grub-mkimage -d . -o image *.mod
	dd if=image of=/dev/fd0
	reboot
That should work, right? This system just hangs with nothing on the 
screen except for the last BIOS messages, and the floppy light goes 
off. The same thing happens when I use kernel.img without adding 
modules.

At least I could compile-test... I hope to send the updated patch out 
in a few minutes.

-Hollis

Re: Pentium II boot failure

[Marco Gerards]

Hollis Blanchard <hollis@penguinppc.org> writes:

> I pulled a Pentium II out of the closet for testing, but unfortunately
> even stock CVS failed to boot to the GRUB prompt. I did a build, then
> 	./grub-mkimage -d . -o image *.mod
> 	dd if=image of=/dev/fd0
> 	reboot
> That should work, right? This system just hangs with nothing on the
> screen except for the last BIOS messages, and the floppy light goes
> off. The same thing happens when I use kernel.img without adding
> modules.

This sounds like a problem Vincent has.  Anyway, it does not matter
that much.  At least it compiles, I can test if it also works.

Thanks,
Marco

Re: Pentium II boot failure

[Vincent Pelletier]

Marco Gerards a écrit :
> Hollis Blanchard <hollis@penguinppc.org> writes:
> 
> 
>>I pulled a Pentium II out of the closet for testing, but unfortunately
>>even stock CVS failed to boot to the GRUB prompt. I did a build, then
>>	./grub-mkimage -d . -o image *.mod
>>	dd if=image of=/dev/fd0
>>	reboot
>>That should work, right? This system just hangs with nothing on the
>>screen except for the last BIOS messages, and the floppy light goes
>>off. The same thing happens when I use kernel.img without adding
>>modules.
> 
> 
> This sounds like a problem Vincent has.  Anyway, it does not matter
> that much.  At least it compiles, I can test if it also works.


Actually, the problem I had was that grub2 gives that error during stage 1 :
GRUB Read Error
and is because an asm instruction was removed in stage 1 which was a
workaround for buggy BIOSes like mine.

Does your computer works with grub legacy ? there are not much changes
between legacy's stage 1 and grub 2's one.

Vincent Pelletier

Re: Pentium II boot failure

[Hollis Blanchard]

On Jan 3, 2005, at 7:17 AM, Vincent Pelletier wrote:

> Marco Gerards a écrit :
>> Hollis Blanchard <hollis@penguinppc.org> writes:
>>> I pulled a Pentium II out of the closet for testing, but 
>>> unfortunately
>>> even stock CVS failed to boot to the GRUB prompt. I did a build, then
>>> 	./grub-mkimage -d . -o image *.mod
>>> 	dd if=image of=/dev/fd0
>>> 	reboot
>>> That should work, right? This system just hangs with nothing on the
>>> screen except for the last BIOS messages, and the floppy light goes
>>> off. The same thing happens when I use kernel.img without adding
>>> modules.
>> This sounds like a problem Vincent has.  Anyway, it does not matter
>> that much.  At least it compiles, I can test if it also works.
>
> Actually, the problem I had was that grub2 gives that error during 
> stage 1 :
> GRUB Read Error
> and is because an asm instruction was removed in stage 1 which was a
> workaround for buggy BIOSes like mine.
>
> Does your computer works with grub legacy ? there are not much changes
> between legacy's stage 1 and grub 2's one.

GRUB Legacy, i.e. ftp://alpha.gnu.org/gnu/grub/grub-0.95-i386-pc.ext2fs 
, does boot. It could be a problem with my compiler I guess... it's a 
stock Debian compiler though.

-Hollis