[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Big Endian fix patch
From: |
Lennart Sorensen |
Subject: |
Re: Big Endian fix patch |
Date: |
Wed, 28 Jul 2010 11:00:37 -0400 |
User-agent: |
Mutt/1.5.18 (2008-05-17) |
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
- Big Endian fix patch (was: Re: Couple more fixes for Linux raid metadata 1.x support), Doug Nazar, 2010/07/26
- Re: Big Endian fix patch (was: Re: Couple more fixes for Linux raid metadata 1.x support), Lennart Sorensen, 2010/07/27
- Re: Big Endian fix patch, Doug Nazar, 2010/07/28
- Re: Big Endian fix patch,
Lennart Sorensen <=
- Re: Big Endian fix patch, Lennart Sorensen, 2010/07/28
- Re: Big Endian fix patch, Doug Nazar, 2010/07/28
- Re: Big Endian fix patch, Lennart Sorensen, 2010/07/28
- Re: Big Endian fix patch, Doug Nazar, 2010/07/28
- Re: Big Endian fix patch, Lennart Sorensen, 2010/07/28
- Re: Big Endian fix patch, Lennart Sorensen, 2010/07/28
- Re: Big Endian fix patch, Doug Nazar, 2010/07/28
- Re: Big Endian fix patch, Lennart Sorensen, 2010/07/28
- Re: Big Endian fix patch, Doug Nazar, 2010/07/28
- Re: Big Endian fix patch, Lennart Sorensen, 2010/07/28