Re: [PATCH] Fix chainloding + Chainloading into logical partitions

Re: [PATCH] Fix chainloding + Chainloading into logical partitions

[Jean-Pierre Flori]

[-- Attachment #1.1: Type: text/plain, Size: 868 bytes --]

As already mentioned in this mailing list (
http://lists.gnu.org/archive/html/grub-devel/2008-07/msg00192.html)n<http://lists.gnu.org/archive/html/grub-devel/2008-07/msg00192.html%29n>I
can't chainload Syslinux from Grub2 on a disk with a pc partition
table
(Syslinux is installed on a primary partition).
I just get "Boot error" (from Syslinux i guess).
However it does work from Grub Legacy.
I also couldn't chainload Syslinux from Grub2 if the disk used a GPT
partition table.
The patch provided in the previous thread and adapted to current Debian
unstable Grub2 package (1.96+20090721-3, patch attached) fixes the problem.
Have any decision been made to include that patch in a future release (or a
"better" one just providing enough information for Syslinux to boot as it
was mentioned that this one copies more than necessary) ?

Thanks.

-- 
Jean-Pierre Flori

[-- Attachment #1.2: Type: text/html, Size: 991 bytes --]

[-- Attachment #2: chainloader.diff --]
[-- Type: text/x-patch, Size: 2163 bytes --]

diff -ru grub2-1.96+20090721/include/grub/pc_partition.h grub2+patch/include/grub/pc_partition.h
--- grub2-1.96+20090721/include/grub/pc_partition.h	2009-05-09 13:04:08.000000000 +0200
+++ grub2+patch/include/grub/pc_partition.h	2009-07-22 10:49:34.557536456 +0200
@@ -184,6 +184,9 @@
 
   /* The offset of the extended partition.  */
   unsigned long ext_offset;
+
+  /* Partition entry. */
+  struct grub_pc_partition_entry pc_part_entry;
 };
 
 static inline int
diff -ru grub2-1.96+20090721/loader/i386/pc/chainloader.c grub2+patch/loader/i386/pc/chainloader.c
--- grub2-1.96+20090721/loader/i386/pc/chainloader.c	2009-06-11 18:13:39.000000000 +0200
+++ grub2+patch/loader/i386/pc/chainloader.c	2009-07-22 10:51:36.170516258 +0200
@@ -32,6 +32,7 @@
 #include <grub/dl.h>
 #include <grub/command.h>
 #include <grub/machine/biosnum.h>
+#include <grub/pc_partition.h>
 
 static grub_dl_t my_mod;
 static int boot_drive;
@@ -94,10 +95,11 @@
   dev = grub_device_open (0);
   if (dev && dev->disk && dev->disk->partition)
     {
-      grub_disk_read (dev->disk, dev->disk->partition->offset, 446, 64,
-		      (void *) GRUB_MEMORY_MACHINE_PART_TABLE_ADDR);
-      part_addr = (void *) (GRUB_MEMORY_MACHINE_PART_TABLE_ADDR
-			    + (dev->disk->partition->index << 4));
+      struct grub_pc_partition *pcdata = dev->disk->partition->data;
+      grub_memcpy((void *) GRUB_MEMORY_MACHINE_PART_TABLE_ADDR,
+			  (void *) &pcdata->pc_part_entry,
+			  sizeof(struct grub_pc_partition_entry));
+      part_addr = (void *) (GRUB_MEMORY_MACHINE_PART_TABLE_ADDR);
     }
 
   if (dev)
diff -ru grub2-1.96+20090721/partmap/pc.c grub2+patch/partmap/pc.c
--- grub2-1.96+20090721/partmap/pc.c	2009-06-10 23:04:23.000000000 +0200
+++ grub2+patch/partmap/pc.c	2009-07-22 10:53:34.679495828 +0200
@@ -131,6 +131,8 @@
 	  pcdata.bsd_part = -1;
 	  pcdata.dos_type = e->type;
 	  pcdata.bsd_type = -1;
+	  grub_memcpy(&pcdata.pc_part_entry, e, sizeof(struct grub_pc_partition_entry));
+	  pcdata.pc_part_entry.start = grub_cpu_to_le32(p.offset + grub_le_to_cpu32 (e->start));
 
 	  grub_dprintf ("partition",
 			"partition %d: flag 0x%x, type 0x%x, start 0x%llx, len 0x%llx\n",

Re: [PATCH] Fix chainloding + Chainloading into logical partitions

[Vladimir 'phcoder' Serbinenko]

On Thu, Jul 23, 2009 at 12:11 AM, Jean-Pierre Flori<jpflori@gmail.com> wrote:
> As already mentioned in this mailing list
> (http://lists.gnu.org/archive/html/grub-devel/2008-07/msg00192.html)n I
> can't chainload Syslinux from Grub2 on a disk with a pc partition table
> (Syslinux is installed on a primary partition).
> I just get "Boot error" (from Syslinux i guess).
> However it does work from Grub Legacy.
> I also couldn't chainload Syslinux from Grub2 if the disk used a GPT
> partition table.
> The patch provided in the previous thread and adapted to current Debian
> unstable Grub2 package (1.96+20090721-3, patch attached) fixes the problem.
> Have any decision been made to include that patch in a future release (or a
> "better" one just providing enough information for Syslinux to boot as it
> was mentioned that this one copies more than necessary) ?
>
There is strictly no need to do this restructuration. The real bug is
different fix would be setting dev->disk->partition to 0 before
calling grub_disk_read and restoring it afterwards. This part of code
is changed anyway with my nested partition patch and I was hoping it
could be applied quickly. Could you test nestpart branch of my
repository?
> Thanks.
>
> --
> Jean-Pierre Flori
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

Personal git repository: http://repo.or.cz/w/grub2/phcoder.git

Re: [PATCH] Fix chainloding + Chainloading into logical partitions

[Pavel Roskin]

On Thu, 2009-07-23 at 00:18 +0200, Vladimir 'phcoder' Serbinenko wrote:

> There is strictly no need to do this restructuration. The real bug is
> different fix would be setting dev->disk->partition to 0 before
> calling grub_disk_read and restoring it afterwards. This part of code
> is changed anyway with my nested partition patch and I was hoping it
> could be applied quickly. Could you test nestpart branch of my
> repository?

I'm not comfortable to approve a 60k long patch that reorganizes the
code and fixed numerous bugs in the same time.  I'm sure bugs can be
fixed separately.

-- 
Regards,
Pavel Roskin

Re: [PATCH] Fix chainloding + Chainloading into logical partitions

[Vladimir 'phcoder' Serbinenko]

On Thu, Jul 23, 2009 at 2:04 AM, Pavel Roskin<proski@gnu.org> wrote:
> On Thu, 2009-07-23 at 00:18 +0200, Vladimir 'phcoder' Serbinenko wrote:
>
>> There is strictly no need to do this restructuration. The real bug is
>> different fix would be setting dev->disk->partition to 0 before
>> calling grub_disk_read and restoring it afterwards. This part of code
>> is changed anyway with my nested partition patch and I was hoping it
>> could be applied quickly. Could you test nestpart branch of my
>> repository?
>
> I'm not comfortable to approve a 60k long patch that reorganizes the
> code and fixed numerous bugs in the same time.  I'm sure bugs can be
> fixed separately.
>
It fixed bugs because it replaced some parts of code with new
constructions. So basically wht you ask is to do some work and then
abandon, it additionally spending time rediffing patches. Similar
thing happens when you ask to split patches but then all split go in
without any discussion. Sorry, but this is just useless work
> --
> Regards,
> Pavel Roskin
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

Personal git repository: http://repo.or.cz/w/grub2/phcoder.git

RE: [PATCH] Fix chainloding + Chainloading into logical partitions

[Gregg C Levine]

Hello!
But I am. It is better that we fix these numerous bugs and probably does
reorganize the code at the same time; then allowing this to persist and
probably explode into a mess that causes undo upset to a user who's got no
idea as to how to fix it if he can.

Just like I am not comfortable with people persisting in posting their
messages to this list using a so called digital signature file, and causing
a nuisance amongst us who run primitive scanning tools who mistake it for a
binary thing behind the viruses. (Those same messages also arrive in ugly
UTF-7 or UTF-8 as well.) 
--
Gregg C Levine hansolofalcon@worldnet.att.net
"The Force will be with you always." Obi-Wan Kenobi
  


> -----Original Message-----
> From: grub-devel-bounces+hansolofalcon=worldnet.att.net@gnu.org
[mailto:grub-devel-
> bounces+hansolofalcon=worldnet.att.net@gnu.org] On Behalf Of Pavel Roskin
> Sent: Wednesday, July 22, 2009 8:05 PM
> To: The development of GRUB 2
> Subject: Re: [PATCH] Fix chainloding + Chainloading into logical
partitions
> 
> On Thu, 2009-07-23 at 00:18 +0200, Vladimir 'phcoder' Serbinenko wrote:
> 
> > There is strictly no need to do this restructuration. The real bug is
> > different fix would be setting dev->disk->partition to 0 before
> > calling grub_disk_read and restoring it afterwards. This part of code
> > is changed anyway with my nested partition patch and I was hoping it
> > could be applied quickly. Could you test nestpart branch of my
> > repository?
> 
> I'm not comfortable to approve a 60k long patch that reorganizes the
> code and fixed numerous bugs in the same time.  I'm sure bugs can be
> fixed separately.
> 
> --
> Regards,
> Pavel Roskin

Re: [PATCH] Fix chainloding + Chainloading into logical partitions

[Jean-Pierre Flori]

>On Thu, Jul 23, 2009 at 2:04 AM, Pavel Roskin<address@hidden> wrote:
>> On Thu, 2009-07-23 at 00:18 +0200, Vladimir 'phcoder' Serbinenko wrote:
>>
>>> There is strictly no need to do this restructuration. The real bug is
>>> different fix would be setting dev->disk->partition to 0 before
>>> calling grub_disk_read and restoring it afterwards. This part of code
>>> is changed anyway with my nested partition patch and I was hoping it
>>> could be applied quickly. Could you test nestpart branch of my
>>> repository?
With your version exerything seems to work fine.
Thanks a lot.
>>
>> I'm not comfortable to approve a 60k long patch that reorganizes the
>> code and fixed numerous bugs in the same time.  I'm sure bugs can be
>> fixed separately.
>>
>It fixed bugs because it replaced some parts of code with new
>constructions. So basically wht you ask is to do some work and then
>abandon, it additionally spending time rediffing patches. Similar
>thing happens when you ask to split patches but then all split go in
>without any discussion. Sorry, but this is just useless work
>> --
>> Regards,
>> Pavel Roskin
>>
>>
>> _______________________________________________
>> Grub-devel mailing list
>> address@hidden
>> http://lists.gnu.org/mailman/listinfo/grub-devel
>>
>
>
>
>--
>Regards
>Vladimir 'phcoder' Serbinenko
>
>Personal git repository: http://repo.or.cz/w/grub2/phcoder.git

-- 
Jean-Pierre Flori

[PATCH] Fix chainloding + Chainloading into logical partitions

[Lucas Gadani]

[-- Attachment #1: Type: text/plain, Size: 975 bytes --]

Hello,

This is my first patch for GRUB, and I haven't found any coding
styles/formatting, nor developer documentation, so, if there's
something wrong, please, forgive me and point me to the right
direction.

Current implementation of the chainloading code does not work, cause
it reads the partition table from the first sector of the root device,
and the root device does not contain partition information. I've
changed the way partition table are loaded, keeping a copy of the
partition entry in grub_partition_t structs data field, and just
loading this information in the correct address when chainloading.
I've also changed the logical partitions start address to point to the
actual disk sector, instead of pointing to the sector relative to the
beginning of the extended partition, so we can now boot into logical
partitions (for example, syslinux doesn't boot if the partition table
doesn't map to the correct disk sector).

Any comments are appreciated.

-- 
Lucas

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: chainloader.patch --]
[-- Type: text/x-diff; name=chainloader.patch, Size: 1891 bytes --]

diff -ru grub2.org/include/grub/pc_partition.h grub2/include/grub/pc_partition.h
--- grub2.org/include/grub/pc_partition.h	2007-07-21 20:32:22.000000000 -0300
+++ grub2/include/grub/pc_partition.h	2008-06-29 22:13:38.000000000 -0300
@@ -182,6 +182,9 @@
 
   /* The offset of the extended partition.  */
   unsigned long ext_offset;
+
+  /* Partition entry. */
+  struct grub_pc_partition_entry e;
 };
 
 static inline int
diff -ru grub2.org/loader/i386/pc/chainloader.c grub2/loader/i386/pc/chainloader.c
--- grub2.org/loader/i386/pc/chainloader.c	2007-09-03 17:10:19.000000000 -0300
+++ grub2/loader/i386/pc/chainloader.c	2008-06-29 22:50:50.000000000 -0300
@@ -31,6 +31,7 @@
 #include <grub/machine/memory.h>
 #include <grub/rescue.h>
 #include <grub/dl.h>
+#include <grub/pc_partition.h>
 
 static grub_dl_t my_mod;
 static int boot_drive;
@@ -103,10 +104,11 @@
 
 	  if (p)
 	    {
-	      grub_disk_read (disk, p->offset, 446, 64,
-			      (char *) GRUB_MEMORY_MACHINE_PART_TABLE_ADDR);
-	      part_addr = (void *) (GRUB_MEMORY_MACHINE_PART_TABLE_ADDR
-				    + (p->index << 4));
+	      struct grub_pc_partition *pcdata = p->data;
+	      grub_memcpy((void *) GRUB_MEMORY_MACHINE_PART_TABLE_ADDR,
+			  (void *) &pcdata->e,
+			  sizeof(struct grub_pc_partition_entry));
+	      part_addr = (void *) (GRUB_MEMORY_MACHINE_PART_TABLE_ADDR);
 	    }
 	}
 
diff -ru grub2.org/partmap/pc.c grub2/partmap/pc.c
--- grub2.org/partmap/pc.c	2007-07-21 20:32:30.000000000 -0300
+++ grub2/partmap/pc.c	2008-06-29 22:55:50.000000000 -0300
@@ -135,6 +135,8 @@
 	  pcdata.bsd_part = -1;
 	  pcdata.dos_type = e->type;
 	  pcdata.bsd_type = -1;
+	  grub_memcpy(&pcdata.e, e, sizeof(struct grub_pc_partition_entry));
+	  pcdata.e.start = grub_cpu_to_le32(p.offset + grub_le_to_cpu32 (e->start));
 
 	  grub_dprintf ("partition",
 			"partition %d: flag 0x%x, type 0x%x, start 0x%llx, len 0x%llx\n",

Re: [PATCH] Fix chainloding + Chainloading into logical partitions

[Fulvio Scapin]

[-- Attachment #1: Type: text/plain, Size: 1702 bytes --]

2008/6/30 Lucas Gadani <lgadani@gmail.com>:

> Hello,
>
> This is my first patch for GRUB, and I haven't found any coding
> styles/formatting, nor developer documentation, so, if there's
> something wrong, please, forgive me and point me to the right
> direction.
>
> Current implementation of the chainloading code does not work, cause
> it reads the partition table from the first sector of the root device,
> and the root device does not contain partition information. I've
> changed the way partition table are loaded, keeping a copy of the
> partition entry in grub_partition_t structs data field, and just
> loading this information in the correct address when chainloading.
> I've also changed the logical partitions start address to point to the
> actual disk sector, instead of pointing to the sector relative to the
> beginning of the extended partition, so we can now boot into logical
> partitions (for example, syslinux doesn't boot if the partition table
> doesn't map to the correct disk sector).
>
> Any comments are appreciated.
>
> --
> Lucas
>
Thank you so much for this. I've been having problem for weeks trying to
boot some syslinux-bootable partitions on a usb stick. I knew of the problem
but lacked the necessary knowledge to contribute myself. I've just begun to
try the patched grub, but until now performs as expected.
Something that might be related: I've created a stick with a GPT label, and
I get partitions to boot using syslinux, or rather, they all boot until the
fourth. The fifth and the following ones don't, even when I clone the
contents of another partition which does boot.
I suspect a similar problem to the one you've addressed.
Thanks again for the effort.

[-- Attachment #2: Type: text/html, Size: 2082 bytes --]

Re: [PATCH] Fix chainloding + Chainloading into logical partitions

[Robert Millan]

On Mon, Jun 30, 2008 at 02:28:24AM +0000, Lucas Gadani wrote:
> Hello,
> 
> This is my first patch for GRUB, and I haven't found any coding
> styles/formatting, nor developer documentation, so, if there's
> something wrong, please, forgive me and point me to the right
> direction.
> 
> Current implementation of the chainloading code does not work, cause
> it reads the partition table from the first sector of the root device,
> and the root device does not contain partition information. I've
> changed the way partition table are loaded, keeping a copy of the
> partition entry in grub_partition_t structs data field, and just
> loading this information in the correct address when chainloading.
> I've also changed the logical partitions start address to point to the
> actual disk sector, instead of pointing to the sector relative to the
> beginning of the extended partition, so we can now boot into logical
> partitions (for example, syslinux doesn't boot if the partition table
> doesn't map to the correct disk sector).
> 
> Any comments are appreciated.

Looks like it increases pc.mod code to store information that is only useful
to the chainloader.  Is there a reason for not having the chainloader
retrieve it?  (note that for disk access we have a cache)

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What good is a phone call… if you are unable to speak?
(as seen on /.)

Re: [PATCH] Fix chainloding + Chainloading into logical partitions

[Lucas Gadani]

On Sun, Jul 6, 2008 at 18:41, Robert Millan <rmh@aybabtu.com> wrote:
>
> Looks like it increases pc.mod code to store information that is only useful
> to the chainloader.  Is there a reason for not having the chainloader
> retrieve it?  (note that for disk access we have a cache)

We would need to duplicate the code that iterates through the logical
partitions in the chainloader or we can change
pc_partition_map_iterate hook function to also receive
grub_pc_partition (we can't use the pc_partition_map_iterate cause the
hook function only receives a grub_disk_t and grub_partition_t, but
not the grub_pc_partition that's necessary to reconstruct the
partition info).

-- 
Lucas

Re: [PATCH] Fix chainloding + Chainloading into logical partitions

[Lucas Gadani]

On Sun, Jul 6, 2008 at 5:47 PM, Lucas Gadani <lgadani@gmail.com> wrote:
> On Sun, Jul 6, 2008 at 18:41, Robert Millan <rmh@aybabtu.com> wrote:
>>
>> Looks like it increases pc.mod code to store information that is only useful
>> to the chainloader.  Is there a reason for not having the chainloader
>> retrieve it?  (note that for disk access we have a cache)
>
> We would need to duplicate the code that iterates through the logical
> partitions in the chainloader or we can change
> pc_partition_map_iterate hook function to also receive
> grub_pc_partition (we can't use the pc_partition_map_iterate cause the
> hook function only receives a grub_disk_t and grub_partition_t, but
> not the grub_pc_partition that's necessary to reconstruct the
> partition info).

What would be the best patch to get accepted into grub? I'm willing to
code the patch, but first I'd like to know the best way to get
comitted to grub.

-- 
Lucas

Re: [PATCH] Fix chainloding + Chainloading into logical partitions

[Bean]

On Thu, Jul 10, 2008 at 10:08 PM, Lucas Gadani <lgadani@gmail.com> wrote:
> On Sun, Jul 6, 2008 at 5:47 PM, Lucas Gadani <lgadani@gmail.com> wrote:
>> On Sun, Jul 6, 2008 at 18:41, Robert Millan <rmh@aybabtu.com> wrote:
>>>
>>> Looks like it increases pc.mod code to store information that is only useful
>>> to the chainloader.  Is there a reason for not having the chainloader
>>> retrieve it?  (note that for disk access we have a cache)
>>
>> We would need to duplicate the code that iterates through the logical
>> partitions in the chainloader or we can change
>> pc_partition_map_iterate hook function to also receive
>> grub_pc_partition (we can't use the pc_partition_map_iterate cause the
>> hook function only receives a grub_disk_t and grub_partition_t, but
>> not the grub_pc_partition that's necessary to reconstruct the
>> partition info).
>
> What would be the best patch to get accepted into grub? I'm willing to
> code the patch, but first I'd like to know the best way to get
> comitted to grub.

Hi,

I don't know how many fields of partition table is actually used by
syslinux. But from your code, I get the sense that only the start
sector is used. If that's the case, there is no need to save the
partition entry, we can just set the value in chainloader.

-- 
Bean