Re: Big Endian fix patch

[Lennart Sorensen]

On Wed, Jul 28, 2010 at 04:51:24AM -0400, Doug Nazar wrote:
> Ok, finally made some progress. Ran into several issues, some of them  
> obviously QEMU/OpenBios that I'm not sure if GRUB should work around.  
> With your patch the raid mostly worked, small problem with too many  
> devices because OpenBios creates several aliases for some devices. Also  
> I think you missed the endianess when setting the level.

OK, no idea how I got by without that.  It is currently working for me.
Weird.

I only use raid1 of course, so that is all I tested with.

> This patch includes the following:
>
> - Fix the ofdisk_hash system. We weren't making a copy of the devpath so  
> never found the cached item again.

Could this have anything to do with why I can't see disks without
devaliases assigned?

> - Extend the ofdisk_hash to cache the disk size
> - Scan for a disk size using seek (probably want to set a different  
> start size). Required for metadata 1.0 arrays
> - Optimize checking of raid level
> - If we find a duplicate disk (claims to be same index in the array),  
> skip it or else level 0 arrays wont be found
> - QEMU/OpenBios doesn't seem to like if the prev & name parameters of  
> ieee1275_next_property are the same pointer which caused no devices to  
> be found

QEMU/openbios has many bugs unfortunately.  If I could make any sense
of the code I would try to fix some of them, but I simply can't follow
that code.

> The issues that I came across which are in QEMU/OpenBios:
>
> - The rows are misreported. screen-#rows is set to 75 when in fact there  
> are only 60 rows. Worked around using -prom-env parameter
> - Aliases don't take into account the index (i.e. address@hidden). I ended up 
> with
>     disk /pci/pci-ata/ata-1/disk
>     hd /pci/pci-ata/ata-1/disk
>     ide0 /pci/pci-ata/ata-1/disk
>     ide1 /pci/mac-io/ata-3/disk
>     ide2 /pci/mac-io/ata-3/disk
>
> when ide1 should be address@hidden and ide2 should be address@hidden
> - boot command hangs when passed wrong disk or used from boot-command.  
> Worked around by using load & go
>
> Things do work, and fixing QEMU/OpenBios is a bit further down the  
> rabbit hole than I want to go. ;-)
>
> Thanks,
> Doug
>

> === modified file 'disk/ieee1275/ofdisk.c'
> --- disk/ieee1275/ofdisk.c    2010-05-31 19:01:01 +0000
> +++ disk/ieee1275/ofdisk.c    2010-07-28 07:51:12 +0000
> @@ -26,6 +26,7 @@
>  struct ofdisk_hash_ent
>  {
>    char *devpath;
> +  grub_disk_addr_t size;
>    struct ofdisk_hash_ent *next;
>  };
>  
> @@ -63,7 +64,8 @@
>  
>    if (p)
>      {
> -      p->devpath = devpath;
> +      p->devpath = grub_strdup(devpath);
> +      p->size = 0;
>        p->next = *head;
>        *head = p;
>      }
> @@ -201,11 +203,72 @@
>    grub_dprintf ("disk", "Opened `%s' as handle %p.\n", op->devpath,
>               (void *) (unsigned long) dev_ihandle);
>  
> -  /* XXX: There is no property to read the number of blocks.  There
> -     should be a property `#blocks', but it is not there.  Perhaps it
> -     is possible to use seek for this.  */
> -  disk->total_sectors = GRUB_DISK_SIZE_UNKNOWN;
> -
> +  if (op->size != 0)
> +  {
> +    disk->total_sectors = op->size;
> +  }
> +  else
> +  {
> +    grub_disk_addr_t curr = 1LLU * 1024 * 1024 * 1024;
> +    grub_disk_addr_t size = curr;
> +    grub_ssize_t status;
> +    int seek_top = 1;
> +      
> +    disk->total_sectors = GRUB_DISK_SIZE_UNKNOWN;
> +      
> +    while (1)
> +    {
> +      grub_int8_t success = 0;
> +      char buf[32];
> +
> +      grub_ieee1275_seek (dev_ihandle, curr, &status);
> +      if (status >= 0)
> +      {
> +     grub_ieee1275_read (dev_ihandle, buf, 1, &actual);
> +     if (actual == 1)
> +       success = 1;
> +      }
> +     
> +      if (seek_top)
> +      {
> +     if (success)
> +        {
> +       /* grow */
> +       size = curr;
> +       curr = curr * 2;
> +        }
> +     else
> +     {
> +       seek_top = 0;
> +     }
> +      }
> +      
> +      if (!seek_top)
> +      {
> +     size /= 2;
> +     if (size < 512)
> +       size = 512;
> +
> +     if (success)
> +     {
> +       curr += size;
> +     }
> +     else
> +     {
> +       if (size == 512)
> +         break;
> +
> +       curr -= size;
> +     }
> +      }
> +      
> +    }
> +
> +    if (size > 1024)
> +      disk->total_sectors = curr / 512;
> +    op->size = disk->total_sectors;
> +  }
> +     
>    disk->id = (unsigned long) op;
>  
>    /* XXX: Read this, somehow.  */
> 
> === modified file 'disk/mdraid_linux.c'
> --- disk/mdraid_linux.c       2010-07-28 08:15:40 +0000
> +++ disk/mdraid_linux.c       2010-07-28 08:16:59 +0000
> @@ -322,6 +322,7 @@
>                      grub_disk_addr_t *start_sector)
>  {
>    grub_uint64_t sb_size;
> +  grub_int32_t level;
>    struct grub_raid_super_1x *real_sb;
>  
>    if (grub_le_to_cpu32(sb->major_version) != 1)
> @@ -341,14 +342,15 @@
>    if (grub_le_to_cpu32(sb->feature_map) & MD_FEATURE_RESHAPE_ACTIVE)
>      return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET, "reshape active");
>  
> +  level = grub_le_to_cpu32(sb->level);
>    /* Multipath.  */
> -  if ((int) grub_le_to_cpu32(sb->level) == -4)
> -    sb->level = 1;
> +  if (level == -4)
> +    level = 1;

Oh I see.  I missed the endian conversion on setting the level.  Given I
wasn't using multipath (or whatever -4 is) I would not have hit that.

The one time conversion to a local variable and reuse of that does look
a lot better.

> -  if (grub_le_to_cpu32(sb->level) != 0 && grub_le_to_cpu32(sb->level) != 1 
> && grub_le_to_cpu32(sb->level) != 4 &&
> -      grub_le_to_cpu32(sb->level) != 5 && grub_le_to_cpu32(sb->level) != 6 
> && grub_le_to_cpu32(sb->level) != 10)
> +  if (level != 0 && level != 1 && level != 4 &&
> +      level != 5 && level != 6 && level != 10)
>      return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
> -                    "Unsupported RAID level: %d", 
> grub_le_to_cpu32(sb->level));
> +                    "Unsupported RAID level: %d", level);
>  
>    /* 1.x superblocks don't have a fixed size on disk.  So we have to
>       read it again now that we now the max device count.  */
> @@ -390,7 +392,7 @@
>      }
>  
>    array->number = 0;
> -  array->level = grub_le_to_cpu32 (real_sb->level);
> +  array->level = level;
>    array->layout = grub_le_to_cpu32 (real_sb->layout);
>    array->total_devs = grub_le_to_cpu32 (real_sb->raid_disks);
>    array->disk_size = grub_le_to_cpu64 (real_sb->size);
> 
> === modified file 'disk/raid.c'
> --- disk/raid.c       2010-07-24 09:59:10 +0000
> +++ disk/raid.c       2010-07-28 07:38:32 +0000
> @@ -511,10 +511,13 @@
>                       array->total_devs);
>  
>          if (array->device[new_array->index] != NULL)
> +     {
>            /* We found multiple devices with the same number. Again,
>               this shouldn't happen.  */
>            grub_dprintf ("raid", "Found two disks with the number %d?!?\n",
>                       new_array->number);
> +          return grub_error (GRUB_ERR_OUT_OF_RANGE, "duplicate disk number");
> +        }
>  
>          if (new_array->disk_size < array->disk_size)
>            array->disk_size = new_array->disk_size;
> 
> === modified file 'kern/ieee1275/openfw.c'
> --- kern/ieee1275/openfw.c    2010-07-02 12:47:14 +0000
> +++ kern/ieee1275/openfw.c    2010-07-28 08:09:40 +0000
> @@ -122,7 +122,7 @@
>  grub_devalias_iterate (int (*hook) (struct grub_ieee1275_devalias *alias))
>  {
>    grub_ieee1275_phandle_t aliases;
> -  char *aliasname, *devtype;
> +  char *aliasname, *devtype, *prev;
>    grub_ssize_t actual;
>    struct grub_ieee1275_devalias alias;
>    int ret = 0;
> @@ -133,21 +133,30 @@
>    aliasname = grub_malloc (IEEE1275_MAX_PROP_LEN);
>    if (!aliasname)
>      return 0;
> +  prev = grub_malloc (IEEE1275_MAX_PROP_LEN);
> +  if (!prev)
> +  {
> +    grub_free (aliasname);
> +    return 0;
> +  }
>    devtype = grub_malloc (IEEE1275_MAX_PROP_LEN);
>    if (!devtype)
> -    {
> -      grub_free (aliasname);
> -      return 0;
> -    }
> -
> +  {
> +    grub_free (prev);
> +    grub_free (aliasname);
> +    return 0;
> +  }
> +    
>    /* Find the first property.  */
> +  prev[0] = '\0';
>    aliasname[0] = '\0';
>  
> -  while (grub_ieee1275_next_property (aliases, aliasname, aliasname) > 0)
> +  while (grub_ieee1275_next_property (aliases, prev, aliasname) > 0)
>      {
>        grub_ieee1275_phandle_t dev;
>        grub_ssize_t pathlen;
>        char *devpath;
> +      char *temp = prev;
>  
>        grub_dprintf ("devalias", "devalias name = %s\n", aliasname);
>  
> @@ -155,7 +164,7 @@
>  
>        /* The property `name' is a special case we should skip.  */
>        if (!grub_strcmp (aliasname, "name"))
> -     continue;
> +     goto skip;
>  
>        /* Sun's OpenBoot often doesn't zero terminate the device alias
>        strings, so we will add a NULL byte at the end explicitly.  */
> @@ -165,6 +174,7 @@
>        if (! devpath)
>       {
>         grub_free (devtype);
> +       grub_free (prev);
>         grub_free (aliasname);
>         return 0;
>       }
> @@ -199,9 +209,14 @@
>        grub_free (devpath);
>        if (ret)
>       break;
> +skip:
> +      prev = aliasname;
> +      aliasname = temp;
> +      aliasname[0] = '\0';
>      }
>  
>    grub_free (devtype);
> +  grub_free (prev);
>    grub_free (aliasname);
>    return ret;
>  }

I will give this patch a try on top of mine then and see how it behaves.

-- 
Len Sorensen