Re: [patch] set prefix on PPC

Re: [patch] set prefix on PPC

[Hollis Blanchard]

Updated patch which uses devaliases.

-Hollis

2005-04-17  Hollis Blanchard  <hollis@penguinppc.org>

	* include/grub/powerpc/ieee1275/ieee1275.h
	(grub_ieee1275_encode_devname): New prototype.
	(grub_ieee1275_get_filename): Likewise.
	* kern/powerpc/ieee1275/init.c (grub_translate_ieee175_path): New
	function.
	(grub_set_prefix): Likewise.
	(grub_machine_init): Call grub_set_prefix.
	* kern/powerpc/ieee1275/openfw.c: Fix typos.
	(grub_parse_type): New enum.
	(grub_ieee1275_get_devargs): New function.
	(grub_ieee1275_get_devname): Likewise.
	(grub_ieee1275_parse_args): Likewise.
	(grub_ieee1275_get_filename): Likewise.
	(grub_ieee1275_encode_devname): Likewise.

Index: include/grub/powerpc/ieee1275/ieee1275.h
===================================================================
RCS file: /cvsroot/grub/grub2/include/grub/powerpc/ieee1275/ieee1275.h,v
retrieving revision 1.15
diff -u -p -r1.15 ieee1275.h
--- include/grub/powerpc/ieee1275/ieee1275.h	26 Mar 2005 17:34:50 -0000	1.15
+++ include/grub/powerpc/ieee1275/ieee1275.h	17 Apr 2005 18:47:25 -0000
@@ -134,5 +134,7 @@ void EXPORT_FUNC(abort) (void);
 void EXPORT_FUNC (grub_reboot) (void);
 void EXPORT_FUNC (grub_halt) (void);
 
+char *EXPORT_FUNC(grub_ieee1275_encode_devname) (const char *path);
+char *EXPORT_FUNC(grub_ieee1275_get_filename) (const char *path);
 
 #endif /* ! GRUB_IEEE1275_MACHINE_HEADER */
Index: kern/powerpc/ieee1275/init.c
===================================================================
RCS file: /cvsroot/grub/grub2/kern/powerpc/ieee1275/init.c,v
retrieving revision 1.15
diff -u -p -r1.15 init.c
--- kern/powerpc/ieee1275/init.c	30 Mar 2005 18:59:00 -0000	1.15
+++ kern/powerpc/ieee1275/init.c	17 Apr 2005 18:47:25 -0000
@@ -46,6 +46,69 @@ abort (void)
   for (;;);
 }
 
+/* Translate an OF filesystem path (separated by backslashes), into a GRUB
+   path (separated by forward slashes).  */
+static void
+grub_translate_ieee1275_path (char *filepath)
+{
+  char *backslash;
+
+  backslash = grub_strchr (filepath, '\\');
+  while (backslash != 0)
+    {
+      *backslash = '/';
+      backslash = grub_strchr (filepath, '\\');
+    }
+}
+
+static void
+grub_set_prefix (void)
+{
+  char bootpath[64]; /* XXX check length */
+  char *filename;
+  char *prefix;
+  grub_ieee1275_phandle_t chosen;
+
+  grub_ieee1275_finddevice ("/chosen", &chosen);
+  if (grub_ieee1275_get_property (chosen, "bootpath", &bootpath,
+				  sizeof (bootpath), 0))
+    { 
+      /* Should never happen.  */
+      grub_printf ("/chosen/bootpath property missing!\n");
+      grub_env_set ("prefix", "");
+      return;
+    }
+
+  /* Transform an OF device path to a GRUB path.  */
+
+  prefix = grub_ieee1275_encode_devname (bootpath);
+
+  filename = grub_ieee1275_get_filename (bootpath);
+  if (filename)
+    {
+      char *newprefix;
+      char *lastslash = grub_strrchr (filename, '\\');
+
+      /* Truncate at last directory.  */
+      if (lastslash)
+        {
+	  *lastslash = '\0';
+	  grub_translate_ieee1275_path (filename);
+
+	  newprefix = grub_malloc (grub_strlen (prefix)
+				   + grub_strlen (filename));
+	  grub_sprintf (newprefix, "%s%s", prefix, filename);
+	  grub_free (prefix);
+	  prefix = newprefix;
+	}
+    }
+
+  grub_env_set ("prefix", prefix);
+
+  grub_free (filename);
+  grub_free (prefix);
+}
+
 void
 grub_machine_init (void)
 {
@@ -65,7 +128,7 @@ grub_machine_init (void)
     }
   grub_mm_init_region ((void *) grub_heap_start, grub_heap_len);
 
-  grub_env_set ("prefix", "");
+  grub_set_prefix ();
 
   grub_ofdisk_init ();
 }
Index: kern/powerpc/ieee1275/openfw.c
===================================================================
RCS file: /cvsroot/grub/grub2/kern/powerpc/ieee1275/openfw.c,v
retrieving revision 1.8
diff -u -p -r1.8 openfw.c
--- kern/powerpc/ieee1275/openfw.c	26 Mar 2005 17:34:50 -0000	1.8
+++ kern/powerpc/ieee1275/openfw.c	17 Apr 2005 18:47:26 -0000
@@ -23,6 +23,12 @@
 #include <grub/mm.h>
 #include <grub/machine/ieee1275.h>
 
+enum grub_ieee1275_parse_type
+{
+  GRUB_PARSE_FILENAME,
+  GRUB_PARSE_PARTITION,
+};
+
 /* Walk children of 'devpath', calling hook for each.  */
 grub_err_t
 grub_children_iterate (char *devpath,
@@ -64,7 +70,7 @@ grub_children_iterate (char *devpath,
       if (actual == -1)
 	continue;
 
-      grub_sprintf(fullname, "%s/%s", devpath, childname);
+      grub_sprintf (fullname, "%s/%s", devpath, childname);
 
       alias.type = childtype;
       alias.path = childpath;
@@ -76,7 +82,7 @@ grub_children_iterate (char *devpath,
   return 0;
 }
 
-/* Iterate through all device aliasses.  Thisfunction can be used to
+/* Iterate through all device aliases.  This function can be used to
    find a device of a specific type.  */
 grub_err_t
 grub_devalias_iterate (int (*hook) (struct grub_ieee1275_devalias *alias))
@@ -200,6 +206,155 @@ grub_claimmap (grub_addr_t addr, grub_si
   return 0;
 }
 
+/* Get the device arguments of the Open Firmware node name `path'.  */
+static char *
+grub_ieee1275_get_devargs (const char *path)
+{
+  char *colon = grub_strchr (path, ':');
+
+  if (! colon)
+    return 0;
+
+  return grub_strdup (colon + 1);
+}
+
+/* Get the device path of the Open Firmware node name `path'.  */
+static char *
+grub_ieee1275_get_devname (const char *path)
+{
+  char *colon = grub_strchr (path, ':');
+  char *newpath = 0;
+  int pathlen = grub_strlen (path);
+  auto int match_alias (struct grub_ieee1275_devalias *alias);
+
+  int match_alias (struct grub_ieee1275_devalias *curalias)
+    {
+      if (! grub_strncmp (curalias->path, path, pathlen))
+        {
+	  newpath = grub_strndup (curalias->name, grub_strlen (curalias->name));
+	  return 1;
+	}
+
+      return 0;
+    }
+
+  if (colon)
+    pathlen = (int)(colon - path);
+
+  /* Try to find an alias for this device.  */
+  grub_devalias_iterate (match_alias);
+
+  if (! newpath)
+    newpath = grub_strdup (path);
+
+  return newpath;
+}
+
+static char *
+grub_ieee1275_parse_args (const char *path, enum grub_ieee1275_parse_type ptype)
+{
+  char type[64]; /* XXX check size.  */
+  char *device = grub_ieee1275_get_devname (path);
+  char *args = grub_ieee1275_get_devargs (path);
+  char *ret = 0;
+  grub_ieee1275_phandle_t dev;
+
+  if (!args)
+    /* Shouldn't happen.  */
+    return 0;
+
+  /* We need to know what type of device it is in order to parse the full
+     file path properly.  */
+  if (grub_ieee1275_finddevice (device, &dev))
+    {
+      grub_error (GRUB_ERR_UNKNOWN_DEVICE, "Device %s not found\n", device);
+      goto fail;
+    }
+  if (grub_ieee1275_get_property (dev, "device_type", &type, sizeof type, 0))
+    {
+      grub_error (GRUB_ERR_UNKNOWN_DEVICE,
+		  "Device %s lacks a device_type property\n", device);
+      goto fail;
+    }
+
+  if (!grub_strcmp ("block", type))
+    {
+      /* The syntax of the device arguments is defined in the CHRP and PReP
+         IEEE1275 bindings: "[partition][,[filename]]".  */
+      char *comma = grub_strchr (args, ',');
+
+      if (ptype == GRUB_PARSE_FILENAME)
+	{
+	  if (comma)
+	    {
+	      char *filepath = comma + 1;
+
+	      ret = grub_malloc (grub_strlen (filepath) + 1);
+	      /* Make sure filepath has leading backslash.  */
+	      if (filepath[0] != '\\')
+		grub_sprintf (ret, "\\%s", filepath);
+	      else
+		grub_strcpy (ret, filepath);
+	    }
+	}
+      else if (ptype == GRUB_PARSE_PARTITION)
+        {
+	  if (!comma)
+	    ret = grub_strdup (args);
+	  else
+	    ret = grub_strndup (args, (grub_size_t)(comma - args));
+	}
+    }
+  else
+    {
+      /* XXX Handle net devices by configuring & registering a grub_net_dev
+	 here, then return its name?
+	 Example path: "net:<server ip>,<file name>,<client ip>,<gateway
+	 ip>,<bootp retries>,<tftp retries>".  */
+      grub_printf ("Unsupported type %s for device %s\n", type, device);
+    }
+
+fail:
+  grub_free (device);
+  grub_free (args);
+  return ret;
+}
+
+char *
+grub_ieee1275_get_filename (const char *path)
+{
+  return grub_ieee1275_parse_args (path, GRUB_PARSE_FILENAME);
+}
+
+/* Convert a device name from IEEE1275 syntax to GRUB syntax.  */
+char *
+grub_ieee1275_encode_devname (const char *path)
+{
+  char *device = grub_ieee1275_get_devname (path);
+  char *partition = grub_ieee1275_parse_args (path, GRUB_PARSE_PARTITION);
+  char *encoding;
+
+  if (partition)
+    {
+      unsigned int partno = grub_strtoul (partition, 0, 0);
+      partno--; /* GRUB partition numbering is 0-based.  */
+
+      /* Assume partno will require less than five bytes to encode.  */
+      encoding = grub_malloc (grub_strlen (device) + 3 + 5);
+      grub_sprintf (encoding, "(%s,%d)", device, partno);
+    }
+  else
+    {
+      encoding = grub_malloc (grub_strlen (device) + 2);
+      grub_sprintf (encoding, "(%s)", device);
+    }
+
+  grub_free (partition);
+  grub_free (device);
+
+  return encoding;
+}
+
 void
 grub_reboot (void)
 {

Re: [patch] set prefix on PPC

[Marco Gerards]

Hollis Blanchard <hollis@penguinppc.org> writes:

> Updated patch which uses devaliases.

On my pegasos the devalias is looked up perfectly for both ethernet
and the harddisk. :)

> +  if (partition)
> +    {
> +      unsigned int partno = grub_strtoul (partition, 0, 0);
> +      partno--; /* GRUB partition numbering is 0-based.  */

On my pegasos the partition numbering is 0-based as well.  So when I
run `boot hd:4 grub2', I get `prefix=(hd,3)'.  This should be
`prefix=(hd,4)'.  So my pegasos has proven me right already about what
I said about firmware. :-/

Thanks,
Marco

Re: [patch] set prefix on PPC

[Hollis Blanchard]

On Apr 17, 2005, at 2:38 PM, Marco Gerards wrote:

> Hollis Blanchard <hollis@penguinppc.org> writes:
>
>> Updated patch which uses devaliases.
>
> On my pegasos the devalias is looked up perfectly for both ethernet
> and the harddisk. :)
>
>> +  if (partition)
>> +    {
>> +      unsigned int partno = grub_strtoul (partition, 0, 0);
>> +      partno--; /* GRUB partition numbering is 0-based.  */
>
> On my pegasos the partition numbering is 0-based as well.

Are you sure? This behavior is not CHRP-compliant. The spec is quite 
clear in that regard:
	11.1.2. Open Method Algorithm
	...
	If the partition component is present, it selects the desired 
partition,  where
	partition 0 refers to the entire disk, partition 1 refers to the first 
partition,
	partition 2 to the second, and so forth.

Instead, perhaps our MS-DOS partition map enumeration differs from the 
firmware's. In that case, firmware would not be "0-based", but simply 
different.

This deserves further investigation. I will see what my briQ does, but 
not tonight. I think that this failure on Pegasos does not warrant 
delaying the prefix patch further, since the patch does not break any 
existing functionality and adds important functionality on Macs. Can 
Pegasos be fixed later?

> So when I run `boot hd:4 grub2', I get `prefix=(hd,3)'.  This should be
> `prefix=(hd,4)'.  So my pegasos has proven me right already about what
> I said about firmware. :-/

If this is the case we will need another flag for grub_ieee1275_flags.

-Hollis

Re: [patch] set prefix on PPC - briQ results

[Hollis Blanchard]

I've tested the patch on briQ. As far as I can tell, CodeGen's 
"SmartFirmware" is crap, frequently suffering mysterious failures that 
require unplugging and waiting (unplugging and replugging rapidly isn't 
enough). It also suffers frequent ATA failures, where the disk or 
controller simply stop responding.

1. (major) Firmware does not properly set /chosen/bootpath to include 
the partition number.
2. (major) Partition numbering *seems* to be 0-based. I had a lot of 
problems booting, but I'm pretty sure it is.
3. (minor) /chosen/bootpath is this:
	/pci@FF500000/isa@6/ide@i1F0/disk@0,0
but the "hd" devalias is this:
	/pci@FF500000/isa@6/ide@i1f0/disk@0,0
Note the differing case of "f". This prevents the devalias matching 
from succeeding. I've fixed this by using strncasecmp instead of 
strncmp.

There are various other problems, such as ignoring arguments to the 
"boot" command and instead always using boot-device and boot-file. Also 
I get unexplained errors like these:
	ok boot hda:0,/grubof.chrp
	error: error while trying to load or boot
but this works:
	ok " hda:0" " /grubof.chrp" (load) go

Problem 1 could be worked around by taking the yaboot approach: iterate 
over all disk partitions looking for a file named "/grub/grub.cfg" 
(i.e. /boot/grub/grub.cfg). This fallback can be used if we do not find 
a grub.cfg in prefix.

Problem 2 could be worked around by blacklisting firmwares based on the 
/openprom properties. Here is the briQ's output:
	ok dev /openprom
	ok .properties
	relative-addressing
	model                 "BRIQ,1.0.2.60"
	SmartFirmware-version "1.1"
	CodeGen-copyright     "SmartFirmware(tm) Copyright 1996-2000 by 
CodeGen, Inc.  All Rights Reserved."
	name                  "openprom"

Does the Pegasos have a SmartFirmware-version property?

-Hollis

Re: [patch] set prefix on PPC - briQ results

[Marco Gerards]

Hollis Blanchard <hollis@penguinppc.org> writes:

> I've tested the patch on briQ. As far as I can tell, CodeGen's
> "SmartFirmware" is crap, frequently suffering mysterious failures that
> require unplugging and waiting (unplugging and replugging rapidly
> isn't enough). It also suffers frequent ATA failures, where the disk
> or controller simply stop responding.
>
> 1. (major) Firmware does not properly set /chosen/bootpath to include
> the partition number.

What happens?  Is it something I could try to reproduce on the PegasosII?

> 2. (major) Partition numbering *seems* to be 0-based. I had a lot of
> problems booting, but I'm pretty sure it is.

Same on the pegasos... :/

> 3. (minor) /chosen/bootpath is this:
> 	/pci@FF500000/isa@6/ide@i1F0/disk@0,0
> but the "hd" devalias is this:
> 	/pci@FF500000/isa@6/ide@i1f0/disk@0,0
> Note the differing case of "f". This prevents the devalias matching
> from succeeding. I've fixed this by using strncasecmp instead of
> strncmp.

That seems the right way to solve this problem.

> Problem 1 could be worked around by taking the yaboot approach:
> iterate over all disk partitions looking for a file named
> "/grub/grub.cfg" (i.e. /boot/grub/grub.cfg). This fallback can be used
> if we do not find a grub.cfg in prefix.

Right...

> Problem 2 could be worked around by blacklisting firmwares based on
> the /openprom properties. Here is the briQ's output:

With blacklist you mean setting some additional flags that describe
which bugs the firmware has?

> 	ok dev /openprom
> 	ok .properties
> 	relative-addressing
> 	model                 "BRIQ,1.0.2.60"
> 	SmartFirmware-version "1.1"
> 	CodeGen-copyright     "SmartFirmware(tm) Copyright 1996-2000
> by CodeGen, Inc.  All Rights Reserved."
> 	name                  "openprom"
>
> Does the Pegasos have a SmartFirmware-version property?

It does.  It has both a CodeGen and bplan copyright property.  The
SmartFirmware-version is 1.2 here.  The model is "Pegasos2,1.2".  It
seems to me that this SmartFirmware-version is not interesting because
it looks like this version is not changed for every build.

The most interesting property is "built-on", which we can use to
detect if a specific version has the bug or not.

Thanks,
Marco

Re: [patch] set prefix on PPC - briQ results

[Hollis Blanchard]

On Apr 19, 2005, at 12:36 PM, Marco Gerards wrote:

> Hollis Blanchard <hollis@penguinppc.org> writes:
>
>> I've tested the patch on briQ. As far as I can tell, CodeGen's
>> "SmartFirmware" is crap, frequently suffering mysterious failures that
>> require unplugging and waiting (unplugging and replugging rapidly
>> isn't enough). It also suffers frequent ATA failures, where the disk
>> or controller simply stop responding.
>>
>> 1. (major) Firmware does not properly set /chosen/bootpath to include
>> the partition number.
>
> What happens?  Is it something I could try to reproduce on the 
> PegasosII?

Boot GRUB, run suspend, "dev /chosen .properties". You'll notice the 
device arguments are missing from bootpath.

>> Problem 1 could be worked around by taking the yaboot approach:
>> iterate over all disk partitions looking for a file named
>> "/grub/grub.cfg" (i.e. /boot/grub/grub.cfg). This fallback can be used
>> if we do not find a grub.cfg in prefix.
>
> Right...
>
>> Problem 2 could be worked around by blacklisting firmwares based on
>> the /openprom properties. Here is the briQ's output:
>
> With blacklist you mean setting some additional flags that describe
> which bugs the firmware has?

Yes.

>> 	ok dev /openprom
>> 	ok .properties
>> 	relative-addressing
>> 	model                 "BRIQ,1.0.2.60"
>> 	SmartFirmware-version "1.1"
>> 	CodeGen-copyright     "SmartFirmware(tm) Copyright 1996-2000
>> by CodeGen, Inc.  All Rights Reserved."
>> 	name                  "openprom"
>>
>> Does the Pegasos have a SmartFirmware-version property?
>
> It does.  It has both a CodeGen and bplan copyright property.  The
> SmartFirmware-version is 1.2 here.  The model is "Pegasos2,1.2".  It
> seems to me that this SmartFirmware-version is not interesting because
> it looks like this version is not changed for every build.
>
> The most interesting property is "built-on", which we can use to
> detect if a specific version has the bug or not.

Right now I'm inclined to set a "0_BASED_PARTITION" flag if the 
SmartFirmware-version property exists. Later on if bplan fixes it then 
they will need to provide some other property we can examine.

After I check in my patch I will work on these workarounds.

-Hollis

Re: [patch] set prefix on PPC - briQ results

[Marco Gerards]

Hollis Blanchard <hollis@penguinppc.org> writes:

>>> Does the Pegasos have a SmartFirmware-version property?
>>
>> It does.  It has both a CodeGen and bplan copyright property.  The
>> SmartFirmware-version is 1.2 here.  The model is "Pegasos2,1.2".  It
>> seems to me that this SmartFirmware-version is not interesting because
>> it looks like this version is not changed for every build.
>>
>> The most interesting property is "built-on", which we can use to
>> detect if a specific version has the bug or not.
>
> Right now I'm inclined to set a "0_BASED_PARTITION" flag if the
> SmartFirmware-version property exists. Later on if bplan fixes it then
> they will need to provide some other property we can examine.

Agreed.

> After I check in my patch I will work on these workarounds.

Fine for me.

Thanks,
Marco

Re: [patch] set prefix on PPC

[Hollis Blanchard]

Marco, I believe I addressed all your comments in my mail on 23 Feb
2005. I have implemented your suggestions, with the exception of
checking the device node property lengths, as this requires modifying
many callers elsewhere in the code.

I will commit this in a couple days if I don't hear any complaints.

-Hollis

2005-02-13  Hollis Blanchard  <hollis@penguinppc.org>

	* include/grub/powerpc/ieee1275/ieee1275.h
	(grub_ieee1275_get_dev_encoding): New prototype.
	(grub_ieee1275_get_filename): Likewise.
	* kern/powerpc/ieee1275/init.c (grub_translate_ieee175_path): New
	function.
	(grub_set_prefix): Likewise.
	(grub_machine_init): Call grub_set_prefix.
	* kern/powerpc/ieee1275/openfw.c (grub_parse_type): New
	enum.
	(grub_ieee1275_get_devargs): New function.
	(grub_ieee1275_get_devname): Likewise.
	(grub_ieee1275_parse_args): Likewise.
	(grub_ieee1275_get_filename): Likewise.
	(grub_ieee1275_get_dev_encoding): Likewise.

Index: include/grub/powerpc/ieee1275/ieee1275.h
===================================================================
RCS file: /cvsroot/grub/grub2/include/grub/powerpc/ieee1275/ieee1275.h,v
retrieving revision 1.15
diff -u -p -r1.15 ieee1275.h
--- include/grub/powerpc/ieee1275/ieee1275.h	26 Mar 2005 17:34:50 -0000	1.15
+++ include/grub/powerpc/ieee1275/ieee1275.h	14 Apr 2005 02:57:01 -0000
@@ -134,5 +134,7 @@ void EXPORT_FUNC(abort) (void);
 void EXPORT_FUNC (grub_reboot) (void);
 void EXPORT_FUNC (grub_halt) (void);
 
+char *EXPORT_FUNC(grub_ieee1275_get_dev_encoding) (const char *path);
+char *EXPORT_FUNC(grub_ieee1275_get_filename) (const char *path);
 
 #endif /* ! GRUB_IEEE1275_MACHINE_HEADER */
Index: kern/powerpc/ieee1275/init.c
===================================================================
RCS file: /cvsroot/grub/grub2/kern/powerpc/ieee1275/init.c,v
retrieving revision 1.14
diff -u -p -r1.14 init.c
--- kern/powerpc/ieee1275/init.c	26 Mar 2005 17:34:50 -0000	1.14
+++ kern/powerpc/ieee1275/init.c	14 Apr 2005 02:57:01 -0000
@@ -46,6 +46,68 @@ abort (void)
   for (;;);
 }
 
+/* Translate an OF filesystem path (separated by backslashes), into a GRUB
+   path (separated by forward slashes).  */
+static void
+grub_translate_ieee1275_path(char *filepath)
+{
+  char *backslash;
+
+  backslash = grub_strchr (filepath, '\\');
+  while (backslash != 0)
+    {
+      *backslash = '/';
+      backslash = grub_strchr (filepath, '\\');
+    }
+}
+
+static void
+grub_set_prefix (void)
+{
+  char bootpath[64]; /* XXX check length */
+  char *filename;
+  char *prefix;
+  grub_ieee1275_phandle_t chosen;
+
+  grub_ieee1275_finddevice ("/chosen", &chosen);
+  if (grub_ieee1275_get_property (chosen, "bootpath", &bootpath,
+				  sizeof (bootpath), 0))
+    { 
+      /* Should never happen.  */
+      grub_printf ("/chosen/bootpath property missing!\n");
+      grub_env_set ("prefix", "");
+    }
+
+  /* Transform an OF device path to a GRUB path.  */
+
+  prefix = grub_ieee1275_get_dev_encoding (bootpath);
+
+  filename = grub_ieee1275_get_filename (bootpath);
+  if (filename)
+    {
+      char *newprefix;
+      char *lastslash = grub_strrchr (filename, '\\');
+
+      /* Truncate at last directory.  */
+      if (lastslash)
+        {
+	  *lastslash = '\0';
+	  grub_translate_ieee1275_path (filename);
+
+	  newprefix = grub_malloc (grub_strlen (prefix)
+				   + grub_strlen (filename));
+	  grub_sprintf (newprefix, "%s%s", prefix, filename);
+	  grub_free (prefix);
+	  prefix = newprefix;
+	}
+    }
+
+  grub_env_set ("prefix", prefix);
+
+  grub_free (filename);
+  grub_free (prefix);
+}
+
 void
 grub_machine_init (void)
 {
@@ -65,7 +127,7 @@ grub_machine_init (void)
     }
   grub_mm_init_region ((void *) grub_heap_start, grub_heap_len);
 
-  grub_env_set ("prefix", "");
+  grub_set_prefix ();
 
   grub_ofdisk_init ();
 }
Index: kern/powerpc/ieee1275/openfw.c
===================================================================
RCS file: /cvsroot/grub/grub2/kern/powerpc/ieee1275/openfw.c,v
retrieving revision 1.8
diff -u -p -r1.8 openfw.c
--- kern/powerpc/ieee1275/openfw.c	26 Mar 2005 17:34:50 -0000	1.8
+++ kern/powerpc/ieee1275/openfw.c	14 Apr 2005 02:57:01 -0000
@@ -23,6 +23,12 @@
 #include <grub/mm.h>
 #include <grub/machine/ieee1275.h>
 
+enum grub_ieee1275_parse_type
+{
+  GRUB_PARSE_FILENAME,
+  GRUB_PARSE_PARTITION,
+};
+
 /* Walk children of 'devpath', calling hook for each.  */
 grub_err_t
 grub_children_iterate (char *devpath,
@@ -64,7 +70,7 @@ grub_children_iterate (char *devpath,
       if (actual == -1)
 	continue;
 
-      grub_sprintf(fullname, "%s/%s", devpath, childname);
+      grub_sprintf (fullname, "%s/%s", devpath, childname);
 
       alias.type = childtype;
       alias.path = childpath;
@@ -200,6 +206,138 @@ grub_claimmap (grub_addr_t addr, grub_si
   return 0;
 }
 
+/* Get the device arguments of the Open Firmware device specifier `path'.  */
+static char *
+grub_ieee1275_get_devargs (const char *path)
+{
+  char *colon = grub_strchr (path, ':');
+  char *end;
+
+  if (! colon)
+    return 0;
+
+  end = colon + grub_strlen (colon);
+
+  return grub_strndup (colon + 1, end - colon);
+}
+
+/* Get the device path of the Open Firmware device specifier `path'.  */
+static char *
+grub_ieee1275_get_devname (const char *path)
+{
+  char *colon = grub_strchr (path, ':');
+
+  if (! colon)
+    return grub_strdup (path);
+
+  return grub_strndup (path, (grub_size_t)(colon - path));
+}
+
+static char *
+grub_ieee1275_parse_args (const char *path, enum grub_ieee1275_parse_type type)
+{
+  char type[64]; /* XXX check size.  */
+  char *device = grub_ieee1275_get_devname (path);
+  char *args = grub_ieee1275_get_devargs (path);
+  char *ret = 0;
+  grub_ieee1275_phandle_t dev;
+
+  if (!args)
+    /* Shouldn't happen.  */
+    return 0;
+
+  /* We need to know what type of device it is in order to parse the full
+     file path properly.  */
+  if (grub_ieee1275_finddevice (device, &dev))
+    {
+      grub_error (GRUB_ERR_UNKNOWN_DEVICE, "Device %s not found\n", device);
+      goto fail;
+    }
+  if (grub_ieee1275_get_property (dev, "device_type", &type, sizeof type, 0))
+    {
+      grub_error (GRUB_ERR_UNKNOWN_DEVICE,
+		  "Device %s lacks a device_type property\n", device);
+      goto fail;
+    }
+
+  if (!grub_strcmp ("block", type))
+    {
+      /* The syntax of the device arguments is defined in the CHRP and PReP
+         IEEE1275 bindings: "[partition][,[filename]]".  */
+      char *comma = grub_strchr (args, ',');
+
+      if (field == GRUB_PARSE_FILENAME)
+	{
+	  if (comma)
+	    {
+	      char *filepath = comma + 1;
+
+	      ret = grub_malloc (grub_strlen (filepath) + 1);
+	      /* Make sure filepath has leading backslash.  */
+	      if (filepath[0] != '\\')
+		grub_sprintf (ret, "\\%s", filepath);
+	      else
+		grub_strcpy (ret, filepath);
+	    }
+	}
+      else if (field == GRUB_PARSE_PARTITION)
+        {
+	  if (!comma)
+	    ret = grub_strdup (args);
+	  else
+	    ret = grub_strndup (args, (grub_size_t)(comma - args));
+	}
+    }
+  else
+    {
+      /* XXX Handle net devices by configuring & registering a grub_net_dev
+	 here, then return its name?
+	 Example path: "net:<server ip>,<file name>,<client ip>,<gateway
+	 ip>,<bootp retries>,<tftp retries>".  */
+      grub_printf ("Unsupported type %s for device %s\n", type, device);
+    }
+
+fail:
+  grub_free (device);
+  grub_free (args);
+  return ret;
+}
+
+char *
+grub_ieee1275_get_filename (const char *path)
+{
+  return grub_ieee1275_parse_args (path, GRUB_PARSE_FILENAME);
+}
+
+char *
+grub_ieee1275_get_dev_encoding (const char *path)
+{
+  char *device = grub_ieee1275_get_devname (path);
+  char *partition = grub_ieee1275_parse_args (path, GRUB_PARSE_PARTITION);
+  char *encoding;
+
+  if (partition)
+    {
+      unsigned int partno = grub_strtoul (partition, 0, 0);
+      partno--; /* GRUB partition numbering is 0-based.  */
+
+      /* Assume partno will require less than five bytes to encode (not
+         including punctuation).  */
+      encoding = grub_malloc (grub_strlen (device) + 3 + 5);
+      grub_sprintf (encoding, "(%s,%d)", device, partno);
+    }
+  else
+    {
+      encoding = grub_malloc (grub_strlen (device) + 2);
+      grub_sprintf (encoding, "(%s)", device);
+    }
+
+  grub_free (partition);
+  grub_free (device);
+
+  return encoding;
+}
+
 void
 grub_reboot (void)
 {
Index: loader/powerpc/ieee1275/linux.c
===================================================================
RCS file: /cvsroot/grub/grub2/loader/powerpc/ieee1275/linux.c,v
retrieving revision 1.6
diff -u -p -r1.6 linux.c
--- loader/powerpc/ieee1275/linux.c	14 Feb 2005 18:41:33 -0000	1.6
+++ loader/powerpc/ieee1275/linux.c	14 Apr 2005 02:57:01 -0000
@@ -55,7 +55,7 @@ grub_linux_boot (void)
   /* Set the command line arguments.  */
   grub_ieee1275_set_property (chosen, "bootargs", linux_args,
 			      grub_strlen (linux_args) + 1, &actual);
-  
+
   /* Boot the kernel.  */
   linuxmain = (kernel_entry_t) linux_addr;
   linuxmain ((void *) initrd_addr, initrd_size, grub_ieee1275_entry_fn, 0, 0);

Re: [patch] set prefix on PPC

[Marco Gerards]

Hollis Blanchard <hollis@penguinppc.org> writes:

> Marco, I believe I addressed all your comments in my mail on 23 Feb
> 2005. I have implemented your suggestions, with the exception of
> checking the device node property lengths, as this requires modifying
> many callers elsewhere in the code.

Ok.  I will test the patch on both of my machines to see if it works
for me.

> I will commit this in a couple days if I don't hear any complaints.

Ok.

> Index: loader/powerpc/ieee1275/linux.c
> ===================================================================
> RCS file: /cvsroot/grub/grub2/loader/powerpc/ieee1275/linux.c,v
> retrieving revision 1.6
> diff -u -p -r1.6 linux.c
> --- loader/powerpc/ieee1275/linux.c	14 Feb 2005 18:41:33 -0000	1.6
> +++ loader/powerpc/ieee1275/linux.c	14 Apr 2005 02:57:01 -0000
> @@ -55,7 +55,7 @@ grub_linux_boot (void)
>    /* Set the command line arguments.  */
>    grub_ieee1275_set_property (chosen, "bootargs", linux_args,
>  			      grub_strlen (linux_args) + 1, &actual);
> -  
> +
>    /* Boot the kernel.  */
>    linuxmain = (kernel_entry_t) linux_addr;
>    linuxmain ((void *) initrd_addr, initrd_size, grub_ieee1275_entry_fn, 0, 0);

Can you fix this? :)

--
Marco

Re: [patch] set prefix on PPC

[Marco Gerards]

Hollis Blanchard <hollis@penguinppc.org> writes:

> Marco, I believe I addressed all your comments in my mail on 23 Feb
> 2005. I have implemented your suggestions, with the exception of
> checking the device node property lengths, as this requires modifying
> many callers elsewhere in the code.
>
> I will commit this in a couple days if I don't hear any complaints.

There is a problem with the patch, so please don't.  When booting on
the pegasosII I see the following problem:

grub> set
prefix=(/pci@80000000/ide@C,1/disk@0,0,3)

That is when I am booting using `boot hd:4 grub)'.

The prefix contains an OF device name which will not work in GRUB,
mainly because of the comma's.  Would it be possible to lookup the OF
path in /aliases and set the prefix to the alias?  That will be
something that will work and is more user friendly I think.

Another thing we could do, but which is a bit more ugly, is creating
an alias.  I am not sure if it is possible to create an OF alias from
GRUB, but I assume it will be.

Shouldn't the prefix end with a `/' like you mentioned in one of your
other emails.  So in this case:

prefix=(/pci@80000000/ide@C,1/disk@0,0,3)/


Thanks,
Marco

Re: [patch] set prefix on PPC

[Hollis Blanchard]

On Apr 15, 2005, at 3:24 PM, Marco Gerards wrote:
> When booting on the pegasosII I see the following problem:
>
> grub> set
> prefix=(/pci@80000000/ide@C,1/disk@0,0,3)
>
> That is when I am booting using `boot hd:4 grub)'.
>
> The prefix contains an OF device name which will not work in GRUB,
> mainly because of the comma's.

What, a disk name with a comma in it?! Gee, I wish somebody had 
realized this would be a problem.</sarcasm>

> Would it be possible to lookup the OF
> path in /aliases and set the prefix to the alias?  That will be
> something that will work and is more user friendly I think.

I agree, using the alias is much more user-friendly, and that's the 
only reason I'm comfortable doing it. I will send the revised patch in 
a moment.

Interestingly, as Nico mentioned before, there are often multiple 
aliases to the same device. In my case, when I boot from "hd", prefix 
is set to "ultra0", because that comes first in the /aliases property 
list. Everything works, but it could be a source of user confusion.

> Another thing we could do, but which is a bit more ugly, is creating
> an alias.  I am not sure if it is possible to create an OF alias from
> GRUB, but I assume it will be.

It is quite rude to unexpectedly clobber a user's devalias. With that 
in mind, what is the exact algorithm you're proposing?

> Shouldn't the prefix end with a `/' like you mentioned in one of your
> other emails.  So in this case:
>
> prefix=(/pci@80000000/ide@C,1/disk@0,0,3)/

The slash was removed when the path was truncated to the directory.

-Hollis

Re: [patch] set prefix on PPC

[Marco Gerards]

Hollis Blanchard <hollis@penguinppc.org> writes:

> Interestingly, as Nico mentioned before, there are often multiple
> aliases to the same device. In my case, when I boot from "hd", prefix
> is set to "ultra0", because that comes first in the /aliases property
> list. Everything works, but it could be a source of user confusion.

Right.  This sucks, I agree.

>> Another thing we could do, but which is a bit more ugly, is creating
>> an alias.  I am not sure if it is possible to create an OF alias from
>> GRUB, but I assume it will be.
>
> It is quite rude to unexpectedly clobber a user's devalias. With that
> in mind, what is the exact algorithm you're proposing?

Is this a problem if you do not store it?

--
Marco

[patch] set prefix on PPC

[Hollis Blanchard]

Any comments on this patch?

This code sets the GRUB "prefix" environment variable from the OF
/chosen/bootpath property. It must therefore translate between the two
syntaxes.

I believe most of this code could be moved to a kern/ieee1275/device.c
file, but that can wait until there is another supported OF platform.

I have verified that this works (a config file is loaded) from disk, and
netbooting is not broken. The subdirectory stuff works as well.

-Hollis

2005-02-13  Hollis Blanchard  <hollis@penguinppc.org>

	* include/grub/powerpc/ieee1275/ieee1275.h
	(grub_ieee1275_get_dev_encoding): New prototype.
	(grub_ieee1275_get_filename): Likewise.
	* kern/powerpc/ieee1275/init.c (translate_path): New function.
	(grub_set_prefix): Likewise.
	(grub_machine_init): Call grub_set_prefix.
	* kern/powerpc/ieee1275/openfw.c (GRUB_PARSE_FILENAME): New
	constant.
	(GRUB_PARSE_PARTITION): Likewise.
	(grub_ieee1275_get_devargs): New function.
	(grub_ieee1275_get_devname): Likewise.
	(grub_ieee1275_parse_args): Likewise.
	(grub_ieee1275_get_filename): Likewise.
	(grub_ieee1275_get_dev_encoding): Likewise.

Index: include/grub/powerpc/ieee1275/ieee1275.h
===================================================================
RCS file: /cvsroot/grub/grub2/include/grub/powerpc/ieee1275/ieee1275.h,v
retrieving revision 1.14
diff -u -p -r1.14 ieee1275.h
--- include/grub/powerpc/ieee1275/ieee1275.h	31 Jan 2005 21:28:34 -0000	1.14
+++ include/grub/powerpc/ieee1275/ieee1275.h	13 Feb 2005 17:16:17 -0000
@@ -132,5 +132,8 @@ int EXPORT_FUNC(grub_claimmap) (grub_add
 
 void EXPORT_FUNC(abort) (void);
 
+char *EXPORT_FUNC(grub_ieee1275_get_dev_encoding) (const char *path);
+char *EXPORT_FUNC(grub_ieee1275_get_filename) (const char *path);
+
 
 #endif /* ! GRUB_IEEE1275_MACHINE_HEADER */
Index: kern/powerpc/ieee1275/init.c
===================================================================
RCS file: /cvsroot/grub/grub2/kern/powerpc/ieee1275/init.c,v
retrieving revision 1.13
diff -u -p -r1.13 init.c
--- kern/powerpc/ieee1275/init.c	4 Jan 2005 14:01:45 -0000	1.13
+++ kern/powerpc/ieee1275/init.c	13 Feb 2005 17:16:17 -0000
@@ -42,6 +42,68 @@ abort (void)
   for (;;);
 }
 
+/* Translate an OF filesystem path (separated by backslashes), into a GRUB
+   path (separated by forward slashes).  */
+static void
+translate_path (char *filepath)
+{
+  char *backslash;
+
+  backslash = grub_strchr (filepath, '\\');
+  while (backslash != 0)
+    {
+      *backslash = '/';
+      backslash = grub_strchr (filepath, '\\');
+    }
+}
+
+static void
+grub_set_prefix (void)
+{
+  char bootpath[64]; /* XXX check length */
+  char *filename;
+  char *prefix;
+  grub_ieee1275_phandle_t chosen;
+
+  grub_ieee1275_finddevice ("/chosen", &chosen);
+  if (grub_ieee1275_get_property (chosen, "bootpath", &bootpath,
+				  sizeof (bootpath), 0))
+    { 
+      /* Should never happen.  */
+      grub_printf ("/chosen/bootpath property missing!\n");
+      grub_env_set ("prefix", "");
+    }
+
+  /* Transform an OF device path to a GRUB path.  */
+
+  prefix = grub_ieee1275_get_dev_encoding (bootpath);
+
+  filename = grub_ieee1275_get_filename (bootpath);
+  if (filename)
+    {
+      char *newprefix;
+      char *lastslash = grub_strrchr (filename, '\\');
+
+      /* Truncate at last directory.  */
+      if (lastslash)
+        {
+	  *lastslash = '\0';
+	  translate_path (filename);
+
+	  newprefix = grub_malloc (grub_strlen (prefix)
+				   + grub_strlen (filename));
+	  grub_sprintf (newprefix, "%s%s", prefix, filename);
+	  grub_free (prefix);
+	  prefix = newprefix;
+	}
+    }
+
+  grub_env_set ("prefix", prefix);
+
+  grub_free (filename);
+  grub_free (prefix);
+}
+
 void
 grub_machine_init (void)
 {
@@ -65,7 +127,7 @@ grub_machine_init (void)
     }
   grub_mm_init_region ((void *) heap_start, heap_len);
 
-  grub_env_set ("prefix", "");
+  grub_set_prefix ();
 
   grub_ofdisk_init ();
 }
Index: kern/powerpc/ieee1275/openfw.c
===================================================================
RCS file: /cvsroot/grub/grub2/kern/powerpc/ieee1275/openfw.c,v
retrieving revision 1.7
diff -u -p -r1.7 openfw.c
--- kern/powerpc/ieee1275/openfw.c	3 Jan 2005 17:44:25 -0000	1.7
+++ kern/powerpc/ieee1275/openfw.c	13 Feb 2005 17:16:17 -0000
@@ -23,6 +23,11 @@
 #include <grub/mm.h>
 #include <grub/machine/ieee1275.h>
 
+enum {
+  GRUB_PARSE_FILENAME,
+  GRUB_PARSE_PARTITION,
+};
+
 /* Walk children of 'devpath', calling hook for each.  */
 grub_err_t
 grub_children_iterate (char *devpath,
@@ -64,7 +69,7 @@ grub_children_iterate (char *devpath,
       if (actual == -1)
 	continue;
 
-      grub_sprintf(fullname, "%s/%s", devpath, childname);
+      grub_sprintf (fullname, "%s/%s", devpath, childname);
 
       alias.type = childtype;
       alias.path = childpath;
@@ -199,3 +204,133 @@ grub_claimmap (grub_addr_t addr, grub_si
 
   return 0;
 }
+
+/* Get the device arguments of the Open Firmware device specifier `path'.  */
+static char *
+grub_ieee1275_get_devargs (const char *path)
+{
+  char *colon = grub_strchr (path, ':');
+  char *end;
+
+  if (! colon)
+    return 0;
+
+  end = colon + grub_strlen (colon);
+
+  return grub_strndup (colon + 1, end - colon);
+}
+
+/* Get the device path of the Open Firmware device specifier `path'.  */
+static char *
+grub_ieee1275_get_devname (const char *path)
+{
+  char *colon = grub_strchr (path, ':');
+
+  if (! colon)
+    return grub_strdup (path);
+
+  return grub_strndup (path, (grub_size_t)(colon - path));
+}
+
+static char *
+grub_ieee1275_parse_args (const char *path, int field)
+{
+  char type[64]; /* XXX check size.  */
+  char *device = grub_ieee1275_get_devname (path);
+  char *args = grub_ieee1275_get_devargs (path);
+  char *ret = 0;
+  grub_ieee1275_phandle_t dev;
+
+  if (!args)
+    /* Shouldn't happen.  */
+    return 0;
+
+  /* We need to know what type of device it is in order to parse the full
+     file path properly.  */
+  if (grub_ieee1275_finddevice (device, &dev))
+    {
+      grub_error (GRUB_ERR_UNKNOWN_DEVICE, "Device %s not found\n", device);
+      goto out;
+    }
+  if (grub_ieee1275_get_property (dev, "device_type", &type, sizeof type, 0))
+    {
+      grub_error (GRUB_ERR_UNKNOWN_DEVICE,
+		  "Device %s lacks a device_type property\n", device);
+      goto out;
+    }
+
+  if (!grub_strcmp ("block", type))
+    {
+      /* Example: "disk:<partition number>,<file name>".  */
+      char *comma = grub_strchr (args, ',');
+
+      if (field == GRUB_PARSE_FILENAME)
+	{
+	  if (comma)
+	    {
+	      char *filepath = comma + 1;
+
+	      ret = grub_malloc (grub_strlen (filepath) + 1);
+	      /* Make sure filepath has leading backslash.  */
+	      if (filepath[0] != '\\')
+		grub_sprintf (ret, "\\%s", filepath);
+	      else
+		grub_strcpy (ret, filepath);
+	    }
+	}
+      else if (field == GRUB_PARSE_PARTITION)
+        {
+	  if (!comma)
+	    ret = grub_strdup (args);
+	  else
+	    ret = grub_strndup (args, (grub_size_t)(comma - args));
+	}
+    }
+  else
+    {
+      /* XXX Handle net devices by configuring & registering a grub_net_dev
+	 here, then return its name?
+	 Example path: "net:<server ip>,<file name>,<client ip>,<gateway
+	 ip>,<bootp retries>,<tftp retries>".  */
+      grub_printf ("Unsupported type %s for device %s\n", type, device);
+    }
+
+out:
+  grub_free (device);
+  grub_free (args);
+  return ret;
+}
+
+char *
+grub_ieee1275_get_filename (const char *path)
+{
+  return grub_ieee1275_parse_args (path, GRUB_PARSE_FILENAME);
+}
+
+char *
+grub_ieee1275_get_dev_encoding (const char *path)
+{
+  char *device = grub_ieee1275_get_devname (path);
+  char *partition = grub_ieee1275_parse_args (path, GRUB_PARSE_PARTITION);
+  char *encoding;
+
+  if (partition)
+    {
+      unsigned int partno = grub_strtoul (partition, 0, 0);
+      partno--; /* GRUB partition numbering is 0-based.  */
+
+      /* XXX Assume partno will only require two bytes? */
+      encoding = grub_malloc (grub_strlen (device) + 5);
+      grub_sprintf (encoding, "(%s,%d)", device, partno);
+    }
+  else
+    {
+      encoding = grub_malloc (grub_strlen (device) + 2);
+      grub_sprintf (encoding, "(%s)", device);
+    }
+
+  grub_free (partition);
+  grub_free (device);
+
+  return encoding;
+}

Re: [patch] set prefix on PPC

[Marco Gerards]

Hollis Blanchard <hollis@penguinppc.org> writes:

Hi,

> This code sets the GRUB "prefix" environment variable from the OF
> /chosen/bootpath property. It must therefore translate between the two
> syntaxes.

Personally I don't like setting prefix to the path grubof was loaded
from.  Mainly because of the way how things work on the macintosh.  It
has a special HFS[+] boot partition (at least my installation, but I
think this is the common practice).

I don't like installing all modules and the configuration file on this
partition.  There were some problems with writing to this partition
IIRC and it is not comfortable to use.  It should not be touched by a
user, I just can't remember why.  I will look that up.

What I would prefer is a way to encode the prefix into the grubof
binary.  The only disadvantage is that it might be hard to detect this
install time.

What I would prefer is this:

1)  Set prefix to whatever was set in the binary.
2)  If nothing was set, use `bootpath'.
3)  Read the arguments and if a prefix was passed to GRUB, use it to
    override the prefix.

For #3 I wrote a patch, but I forgot to send it to grub-devel... It's
included with this email.  As far as I am concerned #1 is what is
really important for us.

Perhaps I might be confused by what I read last year and it is not
really an issue.  I just want to get things right.

Thanks,
Marco


2005-02-13  Marco Gerards  <metgerards@student.han.nl>

	* kern/powerpc/ieee1275/init.c (grub_machine_init): Initialize the
	prefix env variable with the bootargs when it has a valid value.

	* loader/powerpc/ieee1275/linux.c (grub_rescue_cmd_linux): Don't
	initialize `init_addr' yet, set it to 0 instead.



Index: kern/powerpc/ieee1275/init.c
===================================================================
RCS file: /cvsroot/grub/grub2/kern/powerpc/ieee1275/init.c,v
retrieving revision 1.13
diff -u -p -r1.13 init.c
--- kern/powerpc/ieee1275/init.c	4 Jan 2005 14:01:45 -0000	1.13
+++ kern/powerpc/ieee1275/init.c	11 Feb 2005 22:41:15 -0000
@@ -48,6 +48,8 @@ grub_machine_init (void)
   extern char _start;
   grub_addr_t heap_start;
   grub_addr_t heap_len;
+  grub_ieee1275_phandle_t chosen;
+  grub_size_t proplen;
 
   grub_console_init ();
 
@@ -67,6 +69,20 @@ grub_machine_init (void)
 
   grub_env_set ("prefix", "");
 
+  /* Try to override the prefix using the argument passed to GRUB.  */
+  grub_ieee1275_finddevice ("/chosen", &chosen);
+  grub_ieee1275_get_property_length (chosen, "bootargs", &proplen);
+  {
+    char args[proplen];
+    grub_size_t actual;
+    
+    grub_ieee1275_get_property (chosen, "bootargs", args, proplen, &actual);
+    
+    /* If the argument is a valid path, use it.  */
+    if (actual > 0 && args[0] == '(' && grub_strchr (args, ')'))
+      grub_env_set ("prefix", args);
+  }
+  
   grub_ofdisk_init ();
 }
 
Index: loader/powerpc/ieee1275/linux.c
===================================================================
RCS file: /cvsroot/grub/grub2/loader/powerpc/ieee1275/linux.c,v
retrieving revision 1.5
diff -u -p -r1.5 linux.c
--- loader/powerpc/ieee1275/linux.c	31 Jan 2005 21:44:35 -0000	1.5
+++ loader/powerpc/ieee1275/linux.c	11 Feb 2005 22:41:15 -0000
@@ -258,7 +258,7 @@ grub_rescue_cmd_linux (int argc, char *a
   else
     {
       grub_loader_set (grub_linux_boot, grub_linux_unload);
-      initrd_addr = 0xc0000000;
+      initrd_addr = 0;
       loaded = 1;
     }
   

Re: [patch] set prefix on PPC

[Hollis Blanchard]

On Feb 13, 2005, at 12:35 PM, Marco Gerards wrote:

> Hollis Blanchard <hollis@penguinppc.org> writes:
>
>> This code sets the GRUB "prefix" environment variable from the OF
>> /chosen/bootpath property. It must therefore translate between the two
>> syntaxes.
>
> Personally I don't like setting prefix to the path grubof was loaded
> from.  Mainly because of the way how things work on the macintosh.  It
> has a special HFS[+] boot partition (at least my installation, but I
> think this is the common practice).
>
> I don't like installing all modules and the configuration file on this
> partition.  There were some problems with writing to this partition
> IIRC and it is not comfortable to use.  It should not be touched by a
> user, I just can't remember why.  I will look that up.

Please let me know if you find any actual data. I have heard the ybin  
author rant about this for years (see  
http://lists.penguinppc.org/yaboot-users/2002/yaboot-users-200210/ 
msg00008.html as an example), but he doesn't like to substantiate his  
claims.

I believe there were two problems with keeping /boot on an HFS  
partition:
1) The Linux HFS driver wasn't overly reliable. That's unfortunate if  
you're writing a kernel there, or editing grub.cfg. For that reason,  
/boot is typically kept as a Linux-native filesystem, and to install a  
kernel ybin invokes hfsutils to do the copying to the "bootstrap"  
partition. (Note the similarity in names "boot" and "bootstrap" causes  
confusion.)
2) HFS has a concept called "blessed"; basically the firmware can  
automatically find and boot blessed executables without needing to  
specify a file name or even partition. Some versions of Mac OS  
allegedly will unbless a non-Mac OS blessed file. For that reason, ybin  
prefers the HFS "bootstrap" partition be of a type that Mac OS will not  
mount (and so will not alter).

However, please do not mistake "this is how ybin does it" for "this is  
the only way to do it". I have never seen these problems (though I  
personally ran an HFS /boot partition that Mac OS mounted), and I have  
never seen data as to what exact software versions or scenarios cause a  
problem. The user in the URL above claims no problems either, and I  
haven't heard of other users with these problems.

> What I would prefer is a way to encode the prefix into the grubof
> binary.  The only disadvantage is that it might be hard to detect this
> install time.

If possible, placing grub.cfg next to grubof will be the easiest  
solution. That way, no matter how or where you boot GRUB (even from  
rescue CD or whatever), you will always know what grub.cfg it will  
load.

Example: you're making a distribution installer and need to boot from  
CD. What prefix do you embed in GRUB? There is no guarantee that every  
machine out there has a "cd" devalias, nor would you want to require  
users to create that in OF before booting. Simply looking adjacent to  
the grubof binary is a much better solution.

> What I would prefer is this:
>
> 1)  Set prefix to whatever was set in the binary.

This would involve more post-compilation ELF manipulation. To  
accomplish that, if we do decide to go that route, I think it would be  
worth converting the existing module segment into a more  
general-purpose segment, containing all the added arguments.

Remember, right now we load modules at a fixed address. At runtime,  
GRUB finds at that address:
1. the size of the modules in memory
2. the module binaries

We can change this to be more general-purpose, e.g. a chain or  
"parameter" structures that could contain the module binaries, the size  
of modules, the prefix, etc.

> 2)  If nothing was set, use `bootpath'.

So it's not such a bad idea after all huh? :)

> 3)  Read the arguments and if a prefix was passed to GRUB, use it to
>     override the prefix.
>
> For #3 I wrote a patch, but I forgot to send it to grub-devel... It's
> included with this email.  As far as I am concerned #1 is what is
> really important for us.
>
> 2005-02-13  Marco Gerards  <metgerards@student.han.nl>
>
> 	* kern/powerpc/ieee1275/init.c (grub_machine_init): Initialize the
> 	prefix env variable with the bootargs when it has a valid value.

I don't like that this assumes bootargs only contains the prefix. For  
example, if we ever get our fancy debugging support, it would be very  
nice to boot grub with "debug=all" as an argument.

> 	* loader/powerpc/ieee1275/linux.c (grub_rescue_cmd_linux): Don't
> 	initialize `init_addr' yet, set it to 0 instead.

Confused... how is this related to bootargs and prefix?

-Hollis

Re: [patch] set prefix on PPC

[Marco Gerards]

Hollis Blanchard <hollis@penguinppc.org> writes:

> On Feb 13, 2005, at 12:35 PM, Marco Gerards wrote:
>
>> Hollis Blanchard <hollis@penguinppc.org> writes:
>>
>>> This code sets the GRUB "prefix" environment variable from the OF
>>> /chosen/bootpath property. It must therefore translate between the two
>>> syntaxes.
>>
>> Personally I don't like setting prefix to the path grubof was loaded
>> from.  Mainly because of the way how things work on the macintosh.  It
>> has a special HFS[+] boot partition (at least my installation, but I
>> think this is the common practice).
>>
>> I don't like installing all modules and the configuration file on this
>> partition.  There were some problems with writing to this partition
>> IIRC and it is not comfortable to use.  It should not be touched by a
>> user, I just can't remember why.  I will look that up.
>
> Please let me know if you find any actual data. I have heard the ybin
> author rant about this for years (see
> http://lists.penguinppc.org/yaboot-users/2002/yaboot-users-200210/
> msg00008.html as an example), but he doesn't like to substantiate his
> claims.

this is interesting.  We could put a warning in the manual (when
writing it), but I most certainly don't want to force people to use
some kind of partition.

> I believe there were two problems with keeping /boot on an HFS
> partition:
> 1) The Linux HFS driver wasn't overly reliable. That's unfortunate if
> you're writing a kernel there, or editing grub.cfg. For that reason,
> /boot is typically kept as a Linux-native filesystem, and to install a
> kernel ybin invokes hfsutils to do the copying to the "bootstrap"
> partition. (Note the similarity in names "boot" and "bootstrap" causes
> confusion.)

Yeah, ok.  This is something we should not worry about at the moment.
If the HFS filesystem is broken, which I doubt, we could either fix it
or install GRUB there using a similar script.

> 2) HFS has a concept called "blessed"; basically the firmware can
> automatically find and boot blessed executables without needing to
> specify a file name or even partition. Some versions of Mac OS
> allegedly will unbless a non-Mac OS blessed file. For that reason,
> ybin  prefers the HFS "bootstrap" partition be of a type that Mac OS
> will not  mount (and so will not alter).

How is the file blessed and why does that make it a problem to keep
/boot on a HFS partition?

> However, please do not mistake "this is how ybin does it" for "this is
> the only way to do it". I have never seen these problems (though I
> personally ran an HFS /boot partition that Mac OS mounted), and I have
> never seen data as to what exact software versions or scenarios cause
> a  problem. The user in the URL above claims no problems either, and I
> haven't heard of other users with these problems.

No, of course not.  But I usually take warnings seriously.  And yaboot
is full of those. :)

>> What I would prefer is a way to encode the prefix into the grubof
>> binary.  The only disadvantage is that it might be hard to detect this
>> install time.
>
> If possible, placing grub.cfg next to grubof will be the easiest
> solution. That way, no matter how or where you boot GRUB (even from
> rescue CD or whatever), you will always know what grub.cfg it will
> load.

Right.

>> What I would prefer is this:
>>
>> 1)  Set prefix to whatever was set in the binary.
>
> This would involve more post-compilation ELF manipulation. To
> accomplish that, if we do decide to go that route, I think it would be
> worth converting the existing module segment into a more
> general-purpose segment, containing all the added arguments.

Ok.  If we use your patch, this is not that important, I guess.  If it
works right that is.

> Remember, right now we load modules at a fixed address. At runtime,
> GRUB finds at that address:
> 1. the size of the modules in memory
> 2. the module binaries
>
> We can change this to be more general-purpose, e.g. a chain or
> "parameter" structures that could contain the module binaries, the
> size  of modules, the prefix, etc.

This might be nice someday.

>> 2)  If nothing was set, use `bootpath'.
>
> So it's not such a bad idea after all huh? :)

:)

>> 3)  Read the arguments and if a prefix was passed to GRUB, use it to
>>     override the prefix.
>>
>> For #3 I wrote a patch, but I forgot to send it to grub-devel... It's
>> included with this email.  As far as I am concerned #1 is what is
>> really important for us.
>>
>> 2005-02-13  Marco Gerards  <metgerards@student.han.nl>
>>
>> 	* kern/powerpc/ieee1275/init.c (grub_machine_init): Initialize the
>> 	prefix env variable with the bootargs when it has a valid value.
>
> I don't like that this assumes bootargs only contains the prefix. For
> example, if we ever get our fancy debugging support, it would be very
> nice to boot grub with "debug=all" as an argument.

I don't want to add a fancy parser yet.  At the moment we just use a
single argument.  If more will be used, this code has to be changed.

>> 	* loader/powerpc/ieee1275/linux.c (grub_rescue_cmd_linux): Don't
>> 	initialize `init_addr' yet, set it to 0 instead.
>
> Confused... how is this related to bootargs and prefix?

It is required to make GNU/Linux boot here, just like the bootargs
stuff. ;)

It is separated with a blank line to make clear they do not serve the
same purpose.

Thanks,
Marco

Re: [patch] set prefix on PPC

[Hollis Blanchard]

On Feb 14, 2005, at 1:01 PM, Marco Gerards wrote:

> Hollis Blanchard <hollis@penguinppc.org> writes:
>
>> 2) HFS has a concept called "blessed"; basically the firmware can
>> automatically find and boot blessed executables without needing to
>> specify a file name or even partition. Some versions of Mac OS
>> allegedly will unbless a non-Mac OS blessed file. For that reason,
>> ybin  prefers the HFS "bootstrap" partition be of a type that Mac OS
>> will not  mount (and so will not alter).
>
> How is the file blessed and why does that make it a problem to keep
> /boot on a HFS partition?

The file is blessed with hfsutils, spefically hattrib(1). I do not know 
if the in-kernel HFS(+) drivers allow for such changes (via ioctls for 
example). That will be worth investigating.

Actually it seems I was mistaken, and "blessed" refers to the directory 
rather than the file itself. I believe then the proper file to be 
booted is found from its 4-byte HFS file type, "tbxi" being the magic 
"Mac OS kernel" type.

>>> What I would prefer is this:
>>>
>>> 1)  Set prefix to whatever was set in the binary.
>>
>> This would involve more post-compilation ELF manipulation. To
>> accomplish that, if we do decide to go that route, I think it would be
>> worth converting the existing module segment into a more
>> general-purpose segment, containing all the added arguments.
>
> Ok.  If we use your patch, this is not that important, I guess.  If it
> works right that is.

Ok. I assume you want to test my patch and review it further, so I will 
wait for more comments before committing it.

>>> 3)  Read the arguments and if a prefix was passed to GRUB, use it to
>>>     override the prefix.
>>>
>>> For #3 I wrote a patch, but I forgot to send it to grub-devel... It's
>>> included with this email.  As far as I am concerned #1 is what is
>>> really important for us.
>>>
>>> 2005-02-13  Marco Gerards  <metgerards@student.han.nl>
>>>
>>> 	* kern/powerpc/ieee1275/init.c (grub_machine_init): Initialize the
>>> 	prefix env variable with the bootargs when it has a valid value.
>>
>> I don't like that this assumes bootargs only contains the prefix. For
>> example, if we ever get our fancy debugging support, it would be very
>> nice to boot grub with "debug=all" as an argument.
>
> I don't want to add a fancy parser yet.  At the moment we just use a
> single argument.  If more will be used, this code has to be changed.

And in that case, the format of "bootargs" will have to change too. 
It's never too early to think about backwards compatibility, especially 
if people are thinking of packaging and distributing a grub2.deb... :)

>>> 	* loader/powerpc/ieee1275/linux.c (grub_rescue_cmd_linux): Don't
>>> 	initialize `init_addr' yet, set it to 0 instead.
>>
>> Confused... how is this related to bootargs and prefix?
>
> It is required to make GNU/Linux boot here, just like the bootargs
> stuff. ;)
>
> It is separated with a blank line to make clear they do not serve the
> same purpose.

I thought each ChangeLog entry represented a distinct "changeset", with 
changesets being logically different changes to a group of files. The 
ChangeLog was explained to me as emulating a changeset in version 
control systems that lack the concept (such as CVS).

Since I believe the ChangeLog is an anachronism, I don't really care 
what it looks like or what format it's in. But since these changes are 
logically distinct, I believe they should at least be committed with 
separate "cvs commit" commands.

-Hollis

Re: [patch] set prefix on PPC

[Marco Gerards]

Hollis Blanchard <hollis@penguinppc.org> writes:

>> How is the file blessed and why does that make it a problem to keep
>> /boot on a HFS partition?
>
> The file is blessed with hfsutils, spefically hattrib(1). I do not
> know if the in-kernel HFS(+) drivers allow for such changes (via
> ioctls for example). That will be worth investigating.
>
> Actually it seems I was mistaken, and "blessed" refers to the
> directory rather than the file itself. I believe then the proper file
> to be booted is found from its 4-byte HFS file type, "tbxi" being the
> magic "Mac OS kernel" type.

Ok.  But the file does not need to be blessed to boot from it.  It's
just used so the user can use:

boot hd,0

instead of:

boot hd,0:grubof

To me the second sounds good enough.  Or does that cause other problems?

> Ok. I assume you want to test my patch and review it further, so I
> will wait for more comments before committing it.

Sure, I will test and review it on both of my PPC systems and review
the patch.  I hope you understand that it can take a while. :/

>> I don't want to add a fancy parser yet.  At the moment we just use a
>> single argument.  If more will be used, this code has to be changed.
>
> And in that case, the format of "bootargs" will have to change
> too. It's never too early to think about backwards compatibility,
> especially if people are thinking of packaging and distributing a
> grub2.deb... :)

Huh?  Why would backward compatibility be broken?

>>> Confused... how is this related to bootargs and prefix?
>>
>> It is required to make GNU/Linux boot here, just like the bootargs
>> stuff. ;)
>>
>> It is separated with a blank line to make clear they do not serve the
>> same purpose.
>
> I thought each ChangeLog entry represented a distinct "changeset",
> with changesets being logically different changes to a group of
> files. The ChangeLog was explained to me as emulating a changeset in
> version control systems that lack the concept (such as CVS).

From the GCS:

   "Separate unrelated change log entries with blank lines.  When two
entries represent parts of the same change, so that they work together,
then don't put blank lines between them.  Then you can omit the file
name and the asterisk when successive entries are in the same file."

Did I misinterpret this?

> Since I believe the ChangeLog is an anachronism, I don't really care
> what it looks like or what format it's in. But since these changes are
> logically distinct, I believe they should at least be committed with
> separate "cvs commit" commands.

One commit it one ChangeLog entry, it's the same.

Thanks,
Marco

Re: [patch] set prefix on PPC

[Hollis Blanchard]

On Feb 15, 2005, at 3:31 PM, Marco Gerards wrote:
> Ok.  But the file does not need to be blessed to boot from it.  It's
> just used so the user can use:
>
> boot hd,0
>
> instead of:
>
> boot hd,0:grubof
>
> To me the second sounds good enough.  Or does that cause other 
> problems?

Remember that resetting the PRAM (a rare but not unusual action on 
Macs) will revert to the stock firmware configuration, and if OS X is 
the only blessed OS, the user will find themselves unable to boot into 
Linux without a decent understanding of Open Firmware. Blessing GRUB 
would eliminate this problem*.

* If OS X is on a lower partition number, the user will still boot into 
OS X after a PRAM reset. However, if the firmware recognizes GRUB as a 
valid kernel, there is a graphical "select boot device" interface on 
newer machines that can be triggered by holding down the Option key at 
startup.

>> Ok. I assume you want to test my patch and review it further, so I
>> will wait for more comments before committing it.
>
> Sure, I will test and review it on both of my PPC systems and review
> the patch.  I hope you understand that it can take a while. :/

I hope I can talk you into taking a look soon, as this functionality is 
essential if we want to actually *use* GRUB2.

It should be pretty easy to build and see what happens... :) (See my 
patch from earlier today since current CVS doesn't build.)

>>> I don't want to add a fancy parser yet.  At the moment we just use a
>>> single argument.  If more will be used, this code has to be changed.
>>
>> And in that case, the format of "bootargs" will have to change
>> too. It's never too early to think about backwards compatibility,
>> especially if people are thinking of packaging and distributing a
>> grub2.deb... :)
>
> Huh?  Why would backward compatibility be broken?

If a user installs GRUB now with this bootargs patch, then later if we 
wish to add a debug option with "fancy parser" they will no longer be 
able to boot, as the syntax will change.

Now: "boot grubof hd0,6"
Later: "boot grubof prefix=hd0,6 debug=all"

Users trying to boot with the old syntax will fail.

-Hollis

Re: [patch] set prefix on PPC

[Marco Gerards]

Hollis Blanchard <hollis@penguinppc.org> writes:

> On Feb 15, 2005, at 3:31 PM, Marco Gerards wrote:
>> Ok.  But the file does not need to be blessed to boot from it.  It's
>> just used so the user can use:
>>
>> boot hd,0
>>
>> instead of:
>>
>> boot hd,0:grubof
>>
>> To me the second sounds good enough.  Or does that cause other
>> problems?
>
> Remember that resetting the PRAM (a rare but not unusual action on
> Macs) will revert to the stock firmware configuration, and if OS X is
> the only blessed OS, the user will find themselves unable to boot into
> Linux without a decent understanding of Open Firmware. Blessing GRUB
> would eliminate this problem*.

This is a good explanation of the problem.  When we have
documentation we should have a text like this in it. :)

> * If OS X is on a lower partition number, the user will still boot
> into OS X after a PRAM reset. However, if the firmware recognizes GRUB
> as a valid kernel, there is a graphical "select boot device" interface
> on newer machines that can be triggered by holding down the Option key
> at startup.

Ok.

>>> Ok. I assume you want to test my patch and review it further, so I
>>> will wait for more comments before committing it.
>>
>> Sure, I will test and review it on both of my PPC systems and review
>> the patch.  I hope you understand that it can take a while. :/
>
> I hope I can talk you into taking a look soon, as this functionality
> is essential if we want to actually *use* GRUB2.

Ok.

> It should be pretty easy to build and see what happens... :) (See my
> patch from earlier today since current CVS doesn't build.)

That's something I could do. :)

>>>> I don't want to add a fancy parser yet.  At the moment we just use a
>>>> single argument.  If more will be used, this code has to be changed.
>>>
>>> And in that case, the format of "bootargs" will have to change
>>> too. It's never too early to think about backwards compatibility,
>>> especially if people are thinking of packaging and distributing a
>>> grub2.deb... :)
>>
>> Huh?  Why would backward compatibility be broken?
>
> If a user installs GRUB now with this bootargs patch, then later if we
> wish to add a debug option with "fancy parser" they will no longer be
> able to boot, as the syntax will change.
>
> Now: "boot grubof hd0,6"
> Later: "boot grubof prefix=hd0,6 debug=all"

I do not like the prefix= syntax.  What I want is:

Now: "boot grubof (hd0,6)"
Later: "boot grubof (hd0,6) --debug=all"

> Users trying to boot with the old syntax will fail.

In my case it will still work. :)

Thanks,
Marco

Re: [patch] set prefix on PPC

[Hollis Blanchard]

On Feb 20, 2005, at 11:50 AM, Marco Gerards wrote:
>>
>> If a user installs GRUB now with this bootargs patch, then later if we
>> wish to add a debug option with "fancy parser" they will no longer be
>> able to boot, as the syntax will change.
>>
>> Now: "boot grubof hd0,6"
>> Later: "boot grubof prefix=hd0,6 debug=all"
>
> I do not like the prefix= syntax.  What I want is:
>
> Now: "boot grubof (hd0,6)"
> Later: "boot grubof (hd0,6) --debug=all"
>
>> Users trying to boot with the old syntax will fail.
>
> In my case it will still work. :)

In your case we are stuck with an inflexible syntax forever. There is a 
reason that commandline tools use named switches, and do not require 
them in any particular order.

-Hollis

Re: [patch] set prefix on PPC

[Marco Gerards]

Hollis Blanchard <hollis@penguinppc.org> writes:

>>> Users trying to boot with the old syntax will fail.
>>
>> In my case it will still work. :)
>
> In your case we are stuck with an inflexible syntax forever. There is
> a reason that commandline tools use named switches, and do not require
> them in any particular order.

Why?  If you add add a --debug it will still work:

boot grubof --debug=all (hd0,6)

If it is really important, I could change this, but I think a full
featured argument parser would be a bit overkill. :)

--
Marco

Re: [patch] set prefix on PPC

[Marco Gerards]

Hollis Blanchard <hollis@penguinppc.org> writes:

> This code sets the GRUB "prefix" environment variable from the OF
> /chosen/bootpath property. It must therefore translate between the two
> syntaxes.

Sven just told me bootpath does not work on the pegasos II, although
it will be fixed.  So the argument will still be required, I guess.  I
will send in a patch for that later (one that will work with this
patch applied).

> I believe most of this code could be moved to a kern/ieee1275/device.c
> file, but that can wait until there is another supported OF platform.

Right.  A lot of code can be moved and shared.

> I have verified that this works (a config file is loaded) from disk, and
> netbooting is not broken. The subdirectory stuff works as well.

Ok.  I still have to test it... :/

> 2005-02-13  Hollis Blanchard  <hollis@penguinppc.org>

[...]

> 	(GRUB_PARSE_PARTITION): Likewise.

Just make this a new enum.  So name the enum and say "New enum
`foo'.".  See my comment below.


> Index: kern/powerpc/ieee1275/init.c

[...]

> +/* Translate an OF filesystem path (separated by backslashes), into a GRUB
> +   path (separated by forward slashes).  */
> +static void
> +translate_path (char *filepath)

Can you add a prefix?

> +static void
> +grub_set_prefix (void)
> +{
> +  char bootpath[64]; /* XXX check length */

You could get the property length and use that.  It would be clearer.
See my patch for passing the prefix as an argument for an example of
this.  I think this was done like this before, but that should be
changed as well. :)

> Index: kern/powerpc/ieee1275/openfw.c

[...]

> +enum {

Please put the { on a new line.

> +  GRUB_PARSE_FILENAME,
> +  GRUB_PARSE_PARTITION,
> +};

I think there can be some problems with unnamed enums.  I am not sure
though...

> +
>  /* Walk children of 'devpath', calling hook for each.  */
>  grub_err_t
>  grub_children_iterate (char *devpath,
> @@ -64,7 +69,7 @@ grub_children_iterate (char *devpath,
>        if (actual == -1)
>  	continue;
>  
> -      grub_sprintf(fullname, "%s/%s", devpath, childname);
> +      grub_sprintf (fullname, "%s/%s", devpath, childname);
>  
>        alias.type = childtype;
>        alias.path = childpath;
> @@ -199,3 +204,133 @@ grub_claimmap (grub_addr_t addr, grub_si
>  
>    return 0;
>  }
> +
> +/* Get the device arguments of the Open Firmware device specifier `path'.  */
> +static char *

Device arguments are like the partition number or so?

> +static char *
> +grub_ieee1275_parse_args (const char *path, int field)
> +{
> +  char type[64]; /* XXX check size.  */

Same stuff as before. :)

> +  char *device = grub_ieee1275_get_devname (path);
> +  char *args = grub_ieee1275_get_devargs (path);
> +  char *ret = 0;
> +  grub_ieee1275_phandle_t dev;
> +
> +  if (!args)
> +    /* Shouldn't happen.  */
> +    return 0;

If I understood your code correctly it can't happen at all, in that
case the test would be useless.

> +
> +  /* We need to know what type of device it is in order to parse the full
> +     file path properly.  */
> +  if (grub_ieee1275_finddevice (device, &dev))
> +    {
> +      grub_error (GRUB_ERR_UNKNOWN_DEVICE, "Device %s not found\n", device);
> +      goto out;
> +    }
> +  if (grub_ieee1275_get_property (dev, "device_type", &type, sizeof type, 0))
> +    {
> +      grub_error (GRUB_ERR_UNKNOWN_DEVICE,
> +		  "Device %s lacks a device_type property\n", device);
> +      goto out;
> +    }
> +
> +  if (!grub_strcmp ("block", type))
> +    {
> +      /* Example: "disk:<partition number>,<file name>".  */

Is it always like this without exceptions?  Will this always be an
alias or will this be copied from "boot"?

> +	      ret = grub_malloc (grub_strlen (filepath) + 1);
> +	      /* Make sure filepath has leading backslash.  */
> +	      if (filepath[0] != '\\')
> +		grub_sprintf (ret, "\\%s", filepath);
> +	      else
> +		grub_strcpy (ret, filepath);

Why is this required?

> +out:

Please use "fail:", just to be consistent.

> +char *
> +grub_ieee1275_get_dev_encoding (const char *path)
> +{
> +  char *device = grub_ieee1275_get_devname (path);
> +  char *partition = grub_ieee1275_parse_args (path, GRUB_PARSE_PARTITION);
> +  char *encoding;
> +
> +  if (partition)
> +    {
> +      unsigned int partno = grub_strtoul (partition, 0, 0);
> +      partno--; /* GRUB partition numbering is 0-based.  */

Right.  But how can you be sure both match?

> +
> +      /* XXX Assume partno will only require two bytes? */
> +      encoding = grub_malloc (grub_strlen (device) + 5);

Can't you just calculate this?  Otherwise you can better allocate a
few bytes too much.

Thanks,
Marco

Re: [patch] set prefix on PPC

[Hollis Blanchard]

On Feb 21, 2005, at 1:01 PM, Marco Gerards wrote:
>
>> +static void
>> +grub_set_prefix (void)
>> +{
>> +  char bootpath[64]; /* XXX check length */
>
> You could get the property length and use that.  It would be clearer.

All these stack-based get_property calls are an accident waiting to 
happen, but you've advocated them until now. :-P Instead, we need a 
wrapper function that returns a malloced buffer of the proper size. 
That's what you're suggesting open-coding here anyways.

However I would rather not do that right now. The few developers 
(myself included) have other things going on, and there are still 
significant functionality gaps. Right now I don't want to spend time 
developing patches that produce lots of code churn and no user-visible 
improvement. Others are more than welcome to, of course. :)

>> +
>> +/* Get the device arguments of the Open Firmware device specifier 
>> `path'.  */
>> +static char *
>
> Device arguments are like the partition number or so?

Yes. OF "node names" contain two parts, the device path and the device 
arguments. The device arguments are everything following a colon, and 
the syntax of the arguments is device-specific (but by convention they 
are separated by commas).

In the case of a node name like "disk:6,grubof", "6,grubof" are the 
device arguments.

I used the term "device specifier" incorrectly the comments; I will 
correct that. (IEEE1275 defines its terms at the beginning, which is 
quite helpful...)
>
>> +  char *device = grub_ieee1275_get_devname (path);
>> +  char *args = grub_ieee1275_get_devargs (path);
>> +  char *ret = 0;
>> +  grub_ieee1275_phandle_t dev;
>> +
>> +  if (!args)
>> +    /* Shouldn't happen.  */
>> +    return 0;
>
> If I understood your code correctly it can't happen at all, in that
> case the test would be useless.

That is not the case:

+grub_ieee1275_get_devargs (const char *path)
+{
+  char *colon = grub_strchr (path, ':');
+  char *end;
+
+  if (! colon)
+    return 0;
...
+}

So if no colon is present, grub_ieee1275_get_devargs returns NULL (ahem 
"0" because we're silly), and then 'args' is NULL, and then we fail.

As a separate issue, I *believe* that bootpath should always be a 
fully-expanded node name, and so always contain a colon, but that 
depends on well-behaved firmware. Accordingly, I don't think a sanity 
check here is unreasonable.

>> +  /* We need to know what type of device it is in order to parse the 
>> full
>> +     file path properly.  */
>> +  if (grub_ieee1275_finddevice (device, &dev))
>> +    {
>> +      grub_error (GRUB_ERR_UNKNOWN_DEVICE, "Device %s not found\n", 
>> device);
>> +      goto out;
>> +    }
>> +  if (grub_ieee1275_get_property (dev, "device_type", &type, sizeof 
>> type, 0))
>> +    {
>> +      grub_error (GRUB_ERR_UNKNOWN_DEVICE,
>> +		  "Device %s lacks a device_type property\n", device);
>> +      goto out;
>> +    }
>> +
>> +  if (!grub_strcmp ("block", type))
>> +    {
>> +      /* Example: "disk:<partition number>,<file name>".  */
>
> Is it always like this without exceptions?  Will this always be an
> alias or will this be copied from "boot"?

Please see section 3.5, "Standard system nodes". From the /chosen 
table, we can see that bootpath contains a "device path". By definition 
that does not include the device arguments; it seems to have been the 
intent of the spec that the device arguments belong in bootargs.

However, as we can see experimentally, at least Apple firmware includes 
the device arguments in bootpath. My briQ's firmware does not, at least 
for netbooting:
	ok boot net:,grubof.chrp
	grub> suspend
	ok dev /chosen
	ok .properties
	...
	bootargs              "briqboot"
	bootpath              "/pci@FF500000/ethernet@7"
	...
... where "briqboot" is the contents of the the boot-file environment 
variable. This is likely the behavior Sven said was broken.

>> +	      ret = grub_malloc (grub_strlen (filepath) + 1);
>> +	      /* Make sure filepath has leading backslash.  */
>> +	      if (filepath[0] != '\\')
>> +		grub_sprintf (ret, "\\%s", filepath);
>> +	      else
>> +		grub_strcpy (ret, filepath);
>
> Why is this required?

In OF, it is possible to boot from either "disk:foo" or "disk:\foo" 
(both are the same file).

However, our filesystem code fails when accessing a path like 
"(hd0,0)//foo", and it also fails on "(hd0,0)foo". The above code 
handles either case by ensuring that the resulting path always has a 
leading backslash (later translated into a forward slash before being 
passed to our filesystem code).

>> +char *
>> +grub_ieee1275_get_dev_encoding (const char *path)
>> +{
>> +  char *device = grub_ieee1275_get_devname (path);
>> +  char *partition = grub_ieee1275_parse_args (path, 
>> GRUB_PARSE_PARTITION);
>> +  char *encoding;
>> +
>> +  if (partition)
>> +    {
>> +      unsigned int partno = grub_strtoul (partition, 0, 0);
>> +      partno--; /* GRUB partition numbering is 0-based.  */
>
> Right.  But how can you be sure both match?

Eh? OF partition numbers are 1-based. To convert to GRUB's 0-based 
numbering, we subtract one. How could that not "match"?

>> +
>> +      /* XXX Assume partno will only require two bytes? */
>> +      encoding = grub_malloc (grub_strlen (device) + 5);
>
> Can't you just calculate this?  Otherwise you can better allocate a
> few bytes too much.

How exactly would you like to calculate what sprintf will do before it 
does it? We have no snprintf (with "sophisticated" return values), and 
I don't think it's worth the effort either.

I will change the 5 to an 8 if it makes you happy, though I would be 
very surprised to see a disk with partitions in even the (base 10) 
triple digits.

-Hollis

Re: [patch] set prefix on PPC

[Marco Gerards]

Hollis Blanchard <hollis@penguinppc.org> writes:

Hi Hollis,

Sorry for the late reply.  For some reason I did not see this email.

>> You could get the property length and use that.  It would be clearer.
>
> All these stack-based get_property calls are an accident waiting to
> happen, but you've advocated them until now. :-P Instead, we need a
> wrapper function that returns a malloced buffer of the proper
> size. That's what you're suggesting open-coding here anyways.

Using malloc is way better.

> However I would rather not do that right now. The few developers
> (myself included) have other things going on, and there are still
> significant functionality gaps. Right now I don't want to spend time
> developing patches that produce lots of code churn and no user-visible
> improvement. Others are more than welcome to, of course. :)

Sure, I don't have a big problem with this in this case.  But please
don't do that all the time because that will just make a crap out of
GRUB, which is something I don't want.

>> Device arguments are like the partition number or so?
>
> Yes. OF "node names" contain two parts, the device path and the device
> arguments. The device arguments are everything following a colon, and
> the syntax of the arguments is device-specific (but by convention they
> are separated by commas).
>
> In the case of a node name like "disk:6,grubof", "6,grubof" are the
> device arguments.

Ok.

> I used the term "device specifier" incorrectly the comments; I will
> correct that. (IEEE1275 defines its terms at the beginning, which is
> quite helpful...)

Great. :)

> As a separate issue, I *believe* that bootpath should always be a
> fully-expanded node name, and so always contain a colon, but that
> depends on well-behaved firmware. Accordingly, I don't think a sanity
> check here is unreasonable.

When dealing with some open firmware implementations, it is better to
be paranoid. ;)

> However, as we can see experimentally, at least Apple firmware
> includes the device arguments in bootpath. My briQ's firmware does
> not, at least for netbooting:
> 	ok boot net:,grubof.chrp
> 	grub> suspend
> 	ok dev /chosen
> 	ok .properties
> 	...
> 	bootargs              "briqboot"
> 	bootpath              "/pci@FF500000/ethernet@7"

So we don't get an alias... hmm.

Doesn't this cause any problems?

> ... where "briqboot" is the contents of the the boot-file environment
> variable. This is likely the behavior Sven said was broken.

Ok..

>>> +char *
>>> +grub_ieee1275_get_dev_encoding (const char *path)
>>> +{
>>> +  char *device = grub_ieee1275_get_devname (path);
>>> +  char *partition = grub_ieee1275_parse_args (path,
>>> GRUB_PARSE_PARTITION);
>>> +  char *encoding;
>>> +
>>> +  if (partition)
>>> +    {
>>> +      unsigned int partno = grub_strtoul (partition, 0, 0);
>>> +      partno--; /* GRUB partition numbering is 0-based.  */
>>
>> Right.  But how can you be sure both match?
>
> Eh? OF partition numbers are 1-based. To convert to GRUB's 0-based
> numbering, we subtract one. How could that not "match"?

Because not in all cases GRUB and the firmware will count partitions
the same way.  A good example is the PC partition map.  In linux
primary partitions are numbers from 1 to 4, extended partitions are
numbered from 5 (IIRC).  One other way to count these partitions is
just by starting counting from 1.

This is just an example.  There are a lot of partition table layouts
and many ways to interpret partition numbers.  I can imagine GRUB
does not always work the same as a specific firmware implementation
all the time.

>>> +
>>> +      /* XXX Assume partno will only require two bytes? */
>>> +      encoding = grub_malloc (grub_strlen (device) + 5);
>>
>> Can't you just calculate this?  Otherwise you can better allocate a
>> few bytes too much.
>
> How exactly would you like to calculate what sprintf will do before it
> does it? We have no snprintf (with "sophisticated" return values), and
> I don't think it's worth the effort either.
>
> I will change the 5 to an 8 if it makes you happy, though I would be
> very surprised to see a disk with partitions in even the (base 10)
> triple digits.

Please do.

--
Marco