Please support installation on Xen virtual block devices

Please support installation on Xen virtual block devices

[Ian Campbell]

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

I filed this patch against the Debian grub2 package and the maintainer
asked me to bring it to grub-devel@gnu.org. Took me an age but here I
am ;-) [resending now I'm subscribed].

Back then I said:
> The grub package does not currently support installation in a Xen
> guest domain because it does not understand the Xen virtual block
> devices (/dev/xvd[a-z]).
> 
> This support is mainly useful because it allows the Debian installer to
> work in a Xen guest without Xen specific workarounds.
> 
> It might also be useful for someone who wanted to switch between
> a guest between para-virtual and fully-virtualised (HVM) operation.

Patch is attached.

Ian.
-- 
Ian Campbell
Current Noise: Strapping Young Lad - Antiproduct

The Public is merely a multiplied "me."
		-- Mark Twain

[-- Attachment #1.2: 04_support-xvd.diff --]
[-- Type: text/x-patch, Size: 1692 bytes --]

Index: grub2-1.95+20071101/util/biosdisk.c
===================================================================
--- grub2-1.95+20071101.orig/util/biosdisk.c	2007-12-16 16:41:43.000000000 +0000
+++ grub2-1.95+20071101/util/biosdisk.c	2007-12-16 16:42:29.000000000 +0000
@@ -640,6 +640,13 @@
 	  return path;
 	}
       
+      if ((strncmp ("xvd", p, 3) == 0) && p[3] >= 'a' && p[3] <= 'z')
+        {
+	  /* /dev/xvd[a-z][0-9]* */
+	  p[4] = '\0';
+	  return path;
+	}
+
       /* If this is an IDE disk or a SCSI disk.  */
       if ((strncmp ("hd", p, 2) == 0
 	   || strncmp ("sd", p, 2) == 0)
Index: grub2-1.95+20071101/util/grub-mkdevicemap.c
===================================================================
--- grub2-1.95+20071101.orig/util/grub-mkdevicemap.c	2007-12-16 16:41:43.000000000 +0000
+++ grub2-1.95+20071101/util/grub-mkdevicemap.c	2007-12-16 16:49:22.000000000 +0000
@@ -273,6 +273,13 @@
 {
   sprintf (name, "/dev/i2o/hd%c", unit);
 }
+
+static void
+get_xvd_disk_name (char *name, int unit)
+{
+  sprintf (name, "/dev/xvd%c", unit + 'a');
+}
+
 #endif
 
 /* Check if DEVICE can be read. If an error occurs, return zero,
@@ -515,6 +522,24 @@
   }
 #endif /* __linux__ */
 
+#ifdef __linux__
+  /* Xen Virtual Disks. */
+  for (i = 0; i < 16; i++)
+    {
+      char name[16];
+
+      get_xvd_disk_name (name, i);
+      if (check_device (name))
+        {
+            char *p;
+            p = grub_util_get_disk_name (num_hd, name);
+            fprintf (fp, "(%s)\t%s\n", p, name);
+            free (p);
+            num_hd++;
+        }
+    }
+#endif
+
  finish:
   if (fp != stdout)
     fclose (fp);

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: Please support installation on Xen virtual block devices

[Ian Campbell]

Any comments on this patch?

Cheers,
Ian.

On Thu, 2008-04-24 at 20:28 +0100, Ian Campbell wrote:
> I filed this patch against the Debian grub2 package and the maintainer
> asked me to bring it to grub-devel@gnu.org. Took me an age but here I
> am ;-) [resending now I'm subscribed].
> 
> Back then I said:
> > The grub package does not currently support installation in a Xen
> > guest domain because it does not understand the Xen virtual block
> > devices (/dev/xvd[a-z]).
> > 
> > This support is mainly useful because it allows the Debian installer to
> > work in a Xen guest without Xen specific workarounds.
> > 
> > It might also be useful for someone who wanted to switch between
> > a guest between para-virtual and fully-virtualised (HVM) operation.
> 
> Patch is attached.
> 
> Ian.
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
-- 
Ian Campbell
Current Noise: Celtic Frost - Dawn of Megiddo

QOTD:
	"This is a one line proof... if we start sufficiently far to the
	left."

Re: Please support installation on Xen virtual block devices

[Pavel Roskin]

On Tue, 2008-05-06 at 16:29 +0100, Ian Campbell wrote:
> Any comments on this patch?

It's basically OK, bug please provide the GNU style ChangeLog entry.
The second part of the patch doesn't need preprocessor conditionals
because it's already compiled for Linux only.  Please use tabs for
indentation rather than 8 spaces.  The innermost block is indented two
spaces too far.

In the long term, I would prefer that we don't need device.map at all.
This is discussed in a separate thread.

-- 
Regards,
Pavel Roskin

Re: Please support installation on Xen virtual block devices

[Robert Millan]

On Tue, May 06, 2008 at 05:33:23PM -0400, Pavel Roskin wrote:
> On Tue, 2008-05-06 at 16:29 +0100, Ian Campbell wrote:
> > Any comments on this patch?
> 
> It's basically OK, bug please provide the GNU style ChangeLog entry.
> The second part of the patch doesn't need preprocessor conditionals
> because it's already compiled for Linux only.  Please use tabs for
> indentation rather than 8 spaces.  The innermost block is indented two
> spaces too far.

Ian, it looks like you copied this style from existing code.  Is that so?

In that case, I don't think the indentation issue concerns your patch (if we
have to fix indentation, we could just do it in the whole file).

-- 
Robert Millan

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

Re: Please support installation on Xen virtual block devices

[Pavel Roskin]

On Wed, 2008-05-07 at 14:43 +0200, Robert Millan wrote:
> On Tue, May 06, 2008 at 05:33:23PM -0400, Pavel Roskin wrote:
> > On Tue, 2008-05-06 at 16:29 +0100, Ian Campbell wrote:
> > > Any comments on this patch?
> > 
> > It's basically OK, bug please provide the GNU style ChangeLog entry.
> > The second part of the patch doesn't need preprocessor conditionals
> > because it's already compiled for Linux only.  Please use tabs for
> > indentation rather than 8 spaces.  The innermost block is indented two
> > spaces too far.
> 
> Ian, it looks like you copied this style from existing code.  Is that so?
> 
> In that case, I don't think the indentation issue concerns your patch (if we
> have to fix indentation, we could just do it in the whole file).

Apparently, it was copied from the code that was inside an extra "for"
loop.  Anyway, it's a very minor issue, and I would just apply the patch
right now, but I prefer not to write changelog entries for others.

-- 
Regards,
Pavel Roskin

Re: Please support installation on Xen virtual block devices

[Ian Campbell]

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

On Wed, 2008-05-07 at 13:33 -0400, Pavel Roskin wrote:
> Apparently, it was copied from the code that was inside an extra "for"
> loop.  Anyway, it's a very minor issue, and I would just apply the patch
> right now, but I prefer not to write changelog entries for others.

Sorry about that. An updated version is attached with changelog and
(hopefully) correct indentation.

I wasn't sure about the pre-processor conditionals, there seems to be
lots of existing ifdef __linux__ in there and the device path is pretty
Linux specific. I merged it into the one above but if you want them gone
I will redo.

Ian.

-- 
Ian Campbell

omnibiblious, adj.:
	Indifferent to type of drink.  Ex: "Oh, you can get me anything.
	I'm omnibiblious."

[-- Attachment #1.2: support-xvd-v2.diff --]
[-- Type: text/x-patch, Size: 2361 bytes --]

diff -purN -X /home/ijc/development/dontdiff.txt grub2-1.96+20080430.orig/ChangeLog grub2-1.96+20080430/ChangeLog
--- grub2-1.96+20080430.orig/ChangeLog	2008-04-30 12:06:26.000000000 +0100
+++ grub2-1.96+20080430/ChangeLog	2008-05-07 19:41:10.000000000 +0100
@@ -1,3 +1,9 @@
+2008-05-07  Ian Campbell  <ijc@hellion.org.uk>
+
+	* util/biosdisk.c (get_os_disk): Recognise xvd type disks.
+	* util/grub-mkdevicemap.c (get_xvd_disk_name): New function.
+	(make_device_map): Output entries for xvd type disks.
+	
 2008-04-30  Robert Millan  <rmh@aybabtu.com>
 
 	Based on patch from Fabian Greffrath <greffrath@leat.rub.de>
diff -purN -X /home/ijc/development/dontdiff.txt grub2-1.96+20080430.orig/util/biosdisk.c grub2-1.96+20080430/util/biosdisk.c
--- grub2-1.96+20080430.orig/util/biosdisk.c	2008-04-07 16:00:27.000000000 +0100
+++ grub2-1.96+20080430/util/biosdisk.c	2008-05-07 19:34:42.000000000 +0100
@@ -645,6 +645,13 @@ get_os_disk (const char *os_dev)
 	  return path;
 	}
       
+      if ((strncmp ("xvd", p, 3) == 0) && p[3] >= 'a' && p[3] <= 'z')
+        {
+	  /* /dev/xvd[a-z][0-9]* */
+	  p[4] = '\0';
+	  return path;
+	}
+
       /* If this is an IDE disk or a SCSI disk.  */
       if ((strncmp ("hd", p, 2) == 0
 	   || strncmp ("sd", p, 2) == 0)
diff -purN -X /home/ijc/development/dontdiff.txt grub2-1.96+20080430.orig/util/grub-mkdevicemap.c grub2-1.96+20080430/util/grub-mkdevicemap.c
--- grub2-1.96+20080430.orig/util/grub-mkdevicemap.c	2008-04-06 18:51:55.000000000 +0100
+++ grub2-1.96+20080430/util/grub-mkdevicemap.c	2008-05-07 19:49:26.000000000 +0100
@@ -273,6 +273,12 @@ get_i2o_disk_name (char *name, char unit
 {
   sprintf (name, "/dev/i2o/hd%c", unit);
 }
+
+static void
+get_xvd_disk_name (char *name, int unit)
+{
+  sprintf (name, "/dev/xvd%c", unit + 'a');
+}
 #endif
 
 /* Check if DEVICE can be read. If an error occurs, return zero,
@@ -513,6 +519,22 @@ make_device_map (const char *device_map,
 	  }
       }
   }
+
+  /* Xen Virtual Disks. */
+  for (i = 0; i < 16; i++)
+    {
+      char name[16];
+
+      get_xvd_disk_name (name, i);
+      if (check_device (name))
+	{
+	  char *p;
+	  p = grub_util_get_disk_name (num_hd, name);
+	  fprintf (fp, "(%s)\t%s\n", p, name);
+	  free (p);
+	  num_hd++;
+	}
+    }
 #endif /* __linux__ */
 
  finish:

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: Please support installation on Xen virtual block devices

[Pavel Roskin]

On Wed, 2008-05-07 at 19:51 +0100, Ian Campbell wrote:
> On Wed, 2008-05-07 at 13:33 -0400, Pavel Roskin wrote:
> > Apparently, it was copied from the code that was inside an extra "for"
> > loop.  Anyway, it's a very minor issue, and I would just apply the patch
> > right now, but I prefer not to write changelog entries for others.
> 
> Sorry about that. An updated version is attached with changelog and
> (hopefully) correct indentation.
> 
> I wasn't sure about the pre-processor conditionals, there seems to be
> lots of existing ifdef __linux__ in there and the device path is pretty
> Linux specific. I merged it into the one above but if you want them gone
> I will redo.

Applied.

It looks like you missed changes to linux_find_partition().  I'll fix it
myself (if indeed necessary).

-- 
Regards,
Pavel Roskin

Re: Please support installation on Xen virtual block devices

[Ian Campbell]

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

On Wed, 2008-05-07 at 16:49 -0400, Pavel Roskin wrote:
> On Wed, 2008-05-07 at 19:51 +0100, Ian Campbell wrote:
> > On Wed, 2008-05-07 at 13:33 -0400, Pavel Roskin wrote:
> > > Apparently, it was copied from the code that was inside an extra "for"
> > > loop.  Anyway, it's a very minor issue, and I would just apply the patch
> > > right now, but I prefer not to write changelog entries for others.
> > 
> > Sorry about that. An updated version is attached with changelog and
> > (hopefully) correct indentation.
> > 
> > I wasn't sure about the pre-processor conditionals, there seems to be
> > lots of existing ifdef __linux__ in there and the device path is pretty
> > Linux specific. I merged it into the one above but if you want them gone
> > I will redo.
> 
> Applied.

Awesome thanks!

> It looks like you missed changes to linux_find_partition().  I'll fix it
> myself (if indeed necessary).

It doesn't seem to be needed for my simplistic test of installing the
grub-pc .deb and running "install-grub /dev/xvda":
        # dpkg -l grub-pc
        Desired=Unknown/Install/Remove/Purge/Hold
        | Status=Not/Inst/Cfg-files/Unpacked/Failed-cfg/Half-inst/trig-aWait/Trig-pend
        |/ Err?=(none)/Hold/Reinst-required/X=both-problems (Status,Err: uppercase=bad)
        ||/ Name           Version        Description
        +++-==============-==============-============================================
        ii  grub-pc        1.96+20080430- GRand Unified Bootloader, version 2 (PC/BIOS
        # grub-install /dev/xvda
        Installation finished. No error reported.
        This is the contents of the device map /boot/grub/device.map.
        Check if this is correct or not. If any of the lines is incorrect,
        fix it and re-run the script `grub-install'.
        
        (hd0)	/dev/xvda
        # grub-install /dev/xvda1
        Installation finished. No error reported.
        This is the contents of the device map /boot/grub/device.map.
        Check if this is correct or not. If any of the lines is incorrect,
        fix it and re-run the script `grub-install'.
        
        (hd0)	/dev/xvda

However to my not-so-familiar-with-grub eyes it does look like such a
change would make sense.

Cheers,
Ian.
-- 
Ian Campbell

I love you more than anything in this world.  I don't expect that will last.
		-- Elvis Costello

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: Please support installation on Xen virtual block devices

[Robert Millan]

On Wed, May 07, 2008 at 10:07:41PM +0100, Ian Campbell wrote:
> It doesn't seem to be needed for my simplistic test of installing the
> grub-pc .deb and running "install-grub /dev/xvda":
>         # dpkg -l grub-pc
>         Desired=Unknown/Install/Remove/Purge/Hold
>         | Status=Not/Inst/Cfg-files/Unpacked/Failed-cfg/Half-inst/trig-aWait/Trig-pend
>         |/ Err?=(none)/Hold/Reinst-required/X=both-problems (Status,Err: uppercase=bad)
>         ||/ Name           Version        Description
>         +++-==============-==============-============================================
>         ii  grub-pc        1.96+20080430- GRand Unified Bootloader, version 2 (PC/BIOS
>         # grub-install /dev/xvda
>         Installation finished. No error reported.
>         This is the contents of the device map /boot/grub/device.map.
>         Check if this is correct or not. If any of the lines is incorrect,
>         fix it and re-run the script `grub-install'.
>         
>         (hd0)	/dev/xvda
>         # grub-install /dev/xvda1
>         Installation finished. No error reported.
>         This is the contents of the device map /boot/grub/device.map.
>         Check if this is correct or not. If any of the lines is incorrect,
>         fix it and re-run the script `grub-install'.
>         
>         (hd0)	/dev/xvda
> 
> However to my not-so-familiar-with-grub eyes it does look like such a
> change would make sense.

If you want to test the full functionality, try using grub-probe to convert
/dev/xvda1 to a grub drive (this checks GRUB knows how to count partitions
in Xen devices).

-- 
Robert Millan

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

Re: Please support installation on Xen virtual block devices

[Ian Campbell]

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

On Fri, 2008-05-09 at 14:53 +0200, Robert Millan wrote:
> If you want to test the full functionality, try using grub-probe to convert
> /dev/xvda1 to a grub drive (this checks GRUB knows how to count partitions
> in Xen devices).

I get these results:
        d-i:~# grub-probe -t drive /
        (hd0,1)
        d-i:~# grub-probe -t drive -d /dev/xvda
        (hd0)
        d-i:~# grub-probe -t drive -d /dev/xvda1
        (hd0,1)
        
Seems correct to me, right?

Ian.

-- 
Ian Campbell

It is not best to swap horses while crossing the river.
		-- Abraham Lincoln

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: Please support installation on Xen virtual block devices

[Pavel Roskin]

On Sat, 2008-05-10 at 13:03 +0100, Ian Campbell wrote:
> On Fri, 2008-05-09 at 14:53 +0200, Robert Millan wrote:
> > If you want to test the full functionality, try using grub-probe to convert
> > /dev/xvda1 to a grub drive (this checks GRUB knows how to count partitions
> > in Xen devices).
> 
> I get these results:
>         d-i:~# grub-probe -t drive /
>         (hd0,1)
>         d-i:~# grub-probe -t drive -d /dev/xvda
>         (hd0)
>         d-i:~# grub-probe -t drive -d /dev/xvda1
>         (hd0,1)
>         
> Seems correct to me, right?

Actually, linux_find_partition() does something different - it converts
an offset on the disk to a partition in Linux notation.

I understand that linux_find_partition() is only used for cache
synchronization.  It may fail in more subtle ways.  And it looks like
support for other devices, such as /dev/i2o, is also missing.

Perhaps linux_find_partition() should default to adding the bare number
(format="%d") and only specifically handle the cases where something
different is needed.

-- 
Regards,
Pavel Roskin

Re: Please support installation on Xen virtual block devices

[Robert Millan]

On Sat, May 10, 2008 at 01:03:08PM +0100, Ian Campbell wrote:
> On Fri, 2008-05-09 at 14:53 +0200, Robert Millan wrote:
> > If you want to test the full functionality, try using grub-probe to convert
> > /dev/xvda1 to a grub drive (this checks GRUB knows how to count partitions
> > in Xen devices).
> 
> I get these results:
>         d-i:~# grub-probe -t drive /
>         (hd0,1)
>         d-i:~# grub-probe -t drive -d /dev/xvda
>         (hd0)
>         d-i:~# grub-probe -t drive -d /dev/xvda1
>         (hd0,1)
>         
> Seems correct to me, right?

Yep

-- 
Robert Millan

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