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