[PATCH] add virtio disk geometry feature

[PATCH] add virtio disk geometry feature

[Ryan Harper]

From: Ryan Harper <ryanh@us.ibm.com>

Rather than faking up some geometry, allow the backend to push the disk
geometry via virtio pci config option.  Keep the old geo code around for
compatibility.

Signed-off-by: Ryan Harper <ryanh@us.ibm.com>
Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 0cfbe8c..1d2142a 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -157,10 +157,28 @@ static int virtblk_ioctl(struct inode *inode, struct file *filp,
 /* We provide getgeo only to please some old bootloader/partitioning tools */
 static int virtblk_getgeo(struct block_device *bd, struct hd_geometry *geo)
 {
-	/* some standard values, similar to sd */
-	geo->heads = 1 << 6;
-	geo->sectors = 1 << 5;
-	geo->cylinders = get_capacity(bd->bd_disk) >> 11;
+	struct virtio_blk *vblk = bd->bd_disk->private_data;
+	int err = 0;
+
+	/* see if the host passed in geometry config */
+	err = virtio_config_val(vblk->vdev, VIRTIO_BLK_F_GEOMETRY,
+				offsetof(struct virtio_blk_config, cylinders),
+				&geo->cylinders);
+	
+	/* if host sets geo flag, all 3 values must be present */
+	if (!err) {
+		__virtio_config_val(vblk->vdev,
+				offsetof(struct virtio_blk_config, heads),
+				&geo->heads);
+		__virtio_config_val(vblk->vdev,
+				offsetof(struct virtio_blk_config, sectors),
+				&geo->sectors);
+	} else {
+		/* some standard values, similar to sd */
+		geo->heads = 1 << 6;
+		geo->sectors = 1 << 5;
+		geo->cylinders = get_capacity(bd->bd_disk) >> 11;
+	}
 	return 0;
 }
 
diff --git a/include/linux/virtio_blk.h b/include/linux/virtio_blk.h
index bca0b10..142c496 100644
--- a/include/linux/virtio_blk.h
+++ b/include/linux/virtio_blk.h
@@ -9,6 +9,7 @@
 #define VIRTIO_BLK_F_BARRIER	0	/* Does host support barriers? */
 #define VIRTIO_BLK_F_SIZE_MAX	1	/* Indicates maximum segment size */
 #define VIRTIO_BLK_F_SEG_MAX	2	/* Indicates maximum # of segments */
+#define VIRTIO_BLK_F_GEOMETRY	4	/* Legacy geometry available  */
 
 struct virtio_blk_config
 {
@@ -18,6 +19,12 @@ struct virtio_blk_config
 	__le32 size_max;
 	/* The maximum number of segments (if VIRTIO_BLK_F_SEG_MAX) */
 	__le32 seg_max;
+	/* cylinders of the device (if VIRTIO_BLK_F_GEOMETRY) */
+	__le16 cylinders;
+	/* heads of the device (if VIRTIO_BLK_F_GEOMETRY) */
+	__u8 heads;
+	/* sectors of the device (if VIRTIO_BLK_F_GEOMETRY) */
+	__u8 sectors;
 } __attribute__((packed));
 
 /* These two define direction. */

Re: [PATCH] add virtio disk geometry feature

[Rusty Russell]

On Thursday 17 April 2008 04:56:37 Ryan Harper wrote:
> From: Ryan Harper <ryanh@us.ibm.com>
>
> Rather than faking up some geometry, allow the backend to push the disk
> geometry via virtio pci config option.  Keep the old geo code around for
> compatibility.

Hi Ryan,

   Looks good! Some brief review below.  Mainly just "how I would have done
things" stuff.  BTW, does this help in real life?  I assume something in
userspace wants it?

> +	int err = 0;
> +
> +	/* see if the host passed in geometry config */
> +	err = virtio_config_val(vblk->vdev, VIRTIO_BLK_F_GEOMETRY,
> +				offsetof(struct virtio_blk_config, cylinders),
> +				&geo->cylinders);

Unnecessary err initialization.  Sometimes gcc catches bugs when you defer 
initializations of err to as late as possible (ie. paths where err isn't
set properly), so I tend to do it.

> +	/* if host sets geo flag, all 3 values must be present */
> +	if (!err) {
> +		__virtio_config_val(vblk->vdev,
> +				offsetof(struct virtio_blk_config, heads),
> +				&geo->heads);
> +		__virtio_config_val(vblk->vdev,
> +				offsetof(struct virtio_blk_config, sectors),
> +				&geo->sectors);

Kind of ugly; we can represent this in the data structure explicitly tho...

> @@ -18,6 +19,12 @@ struct virtio_blk_config
>  	__le32 size_max;
>  	/* The maximum number of segments (if VIRTIO_BLK_F_SEG_MAX) */
>  	__le32 seg_max;
> +	/* cylinders of the device (if VIRTIO_BLK_F_GEOMETRY) */
> +	__le16 cylinders;
> +	/* heads of the device (if VIRTIO_BLK_F_GEOMETRY) */
> +	__u8 heads;
> +	/* sectors of the device (if VIRTIO_BLK_F_GEOMETRY) */
> +	__u8 sectors;
>  } __attribute__((packed));

... using a struct-within-a-struct.

Here's the result:
Subject: add virtio disk geometry feature
Date: Wed, 16 Apr 2008 13:56:37 -0500
From: Ryan Harper <ryanh@us.ibm.com>

Rather than faking up some geometry, allow the backend to push the disk
geometry via virtio pci config option.  Keep the old geo code around for
compatibility.

Signed-off-by: Ryan Harper <ryanh@us.ibm.com>
Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> (modified to single struct)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -157,10 +157,25 @@ static int virtblk_ioctl(struct inode *i
 /* We provide getgeo only to please some old bootloader/partitioning tools */
 static int virtblk_getgeo(struct block_device *bd, struct hd_geometry *geo)
 {
-	/* some standard values, similar to sd */
-	geo->heads = 1 << 6;
-	geo->sectors = 1 << 5;
-	geo->cylinders = get_capacity(bd->bd_disk) >> 11;
+	struct virtio_blk *vblk = bd->bd_disk->private_data;
+	struct virtio_blk_geometry vgeo;
+	int err;
+
+	/* see if the host passed in geometry config */
+	err = virtio_config_val(vblk->vdev, VIRTIO_BLK_F_GEOMETRY,
+				offsetof(struct virtio_blk_config, geometry),
+				&vgeo);
+
+	if (!err) {
+		geo->heads = vgeo.heads;
+		geo->sectors = vgeo.sectors;
+		geo->cylinders = vgeo.cylinders;
+	} else {
+		/* some standard values, similar to sd */
+		geo->heads = 1 << 6;
+		geo->sectors = 1 << 5;
+		geo->cylinders = get_capacity(bd->bd_disk) >> 11;
+	}
 	return 0;
 }
 
diff --git a/include/linux/virtio_blk.h b/include/linux/virtio_blk.h
--- a/include/linux/virtio_blk.h
+++ b/include/linux/virtio_blk.h
@@ -9,6 +9,7 @@
 #define VIRTIO_BLK_F_BARRIER	0	/* Does host support barriers? */
 #define VIRTIO_BLK_F_SIZE_MAX	1	/* Indicates maximum segment size */
 #define VIRTIO_BLK_F_SEG_MAX	2	/* Indicates maximum # of segments */
+#define VIRTIO_BLK_F_GEOMETRY	4	/* Legacy geometry available  */
 
 struct virtio_blk_config
 {
@@ -18,6 +19,12 @@ struct virtio_blk_config
 	__le32 size_max;
 	/* The maximum number of segments (if VIRTIO_BLK_F_SEG_MAX) */
 	__le32 seg_max;
+	/* geometry the device (if VIRTIO_BLK_F_GEOMETRY) */
+	struct virtio_blk_geometry {
+		__le16 cylinders;
+		__u8 heads;
+		__u8 sectors;
+	} geometry;
 } __attribute__((packed));
 
 /* These two define direction. */

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: [PATCH] add virtio disk geometry feature

[Anthony Liguori]

Rusty Russell wrote:
> On Thursday 17 April 2008 04:56:37 Ryan Harper wrote:
>   
>> From: Ryan Harper <ryanh@us.ibm.com>
>>
>> Rather than faking up some geometry, allow the backend to push the disk
>> geometry via virtio pci config option.  Keep the old geo code around for
>> compatibility.
>>     
>
> Hi Ryan,
>
>    Looks good! Some brief review below.  Mainly just "how I would have done
> things" stuff.  BTW, does this help in real life?  I assume something in
> userspace wants it?
>   

Boot loaders (like grub) query the geometry from the kernel to figure 
out how to setup the stage1/stage2.  We've seen strange issues with grub 
thinking it has crazy geometries when installed on a virtio disk (as 
opposed to booting from virtio with an existing disk).

Ryan: have you tested a hardy install with your patches?  Does it help 
when installing to virtio?  I could pretty reliably reproduce the 
strangeness with a 20GB disk image FWIW.

> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -157,10 +157,25 @@ static int virtblk_ioctl(struct inode *i
>  /* We provide getgeo only to please some old bootloader/partitioning tools */
>  static int virtblk_getgeo(struct block_device *bd, struct hd_geometry *geo)
>  {
> -	/* some standard values, similar to sd */
> -	geo->heads = 1 << 6;
> -	geo->sectors = 1 << 5;
> -	geo->cylinders = get_capacity(bd->bd_disk) >> 11;
> +	struct virtio_blk *vblk = bd->bd_disk->private_data;
> +	struct virtio_blk_geometry vgeo;
> +	int err;
> +
> +	/* see if the host passed in geometry config */
> +	err = virtio_config_val(vblk->vdev, VIRTIO_BLK_F_GEOMETRY,
> +				offsetof(struct virtio_blk_config, geometry),
> +				&vgeo);
> +
> +	if (!err) {
> +		geo->heads = vgeo.heads;
> +		geo->sectors = vgeo.sectors;
> +		geo->cylinders = vgeo.cylinders;
> +	} else {
> +		/* some standard values, similar to sd */
> +		geo->heads = 1 << 6;
> +		geo->sectors = 1 << 5;
> +		geo->cylinders = get_capacity(bd->bd_disk) >> 11;
> +	}
>  	return 0;
>  }
>   

You're probably breaking PPC since the values in the config space are in 
little endian format.  virtio_config_val does automagic endianness 
conversion if the size is 2, 4, or 8.  In this case, the structure size 
is 4 so the endianness conversion will do the wrong thing.

Magic endianness conversion based on read size is looking pretty evil to 
me... Perhaps we need explicit *_val[8,16,32,64]?

Regards,

Anthony Liguori


-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: [PATCH] add virtio disk geometry feature

[Hollis Blanchard]

On Wednesday 16 April 2008 16:32:30 Anthony Liguori wrote:
> > diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> > --- a/drivers/block/virtio_blk.c
> > +++ b/drivers/block/virtio_blk.c
> > @@ -157,10 +157,25 @@ static int virtblk_ioctl(struct inode *i
> >  /* We provide getgeo only to please some old bootloader/partitioning 
tools */
> >  static int virtblk_getgeo(struct block_device *bd, struct hd_geometry 
*geo)
> >  {
> > -     /* some standard values, similar to sd */
> > -     geo->heads = 1 << 6;
> > -     geo->sectors = 1 << 5;
> > -     geo->cylinders = get_capacity(bd->bd_disk) >> 11;
> > +     struct virtio_blk *vblk = bd->bd_disk->private_data;
> > +     struct virtio_blk_geometry vgeo;
> > +     int err;
> > +
> > +     /* see if the host passed in geometry config */
> > +     err = virtio_config_val(vblk->vdev, VIRTIO_BLK_F_GEOMETRY,
> > +                             offsetof(struct virtio_blk_config, 
geometry),
> > +                             &vgeo);
> > +
> > +     if (!err) {
> > +             geo->heads = vgeo.heads;
> > +             geo->sectors = vgeo.sectors;
> > +             geo->cylinders = vgeo.cylinders;
> > +     } else {
> > +             /* some standard values, similar to sd */
> > +             geo->heads = 1 << 6;
> > +             geo->sectors = 1 << 5;
> > +             geo->cylinders = get_capacity(bd->bd_disk) >> 11;
> > +     }
> >       return 0;
> >  }
> >   
> 
> You're probably breaking PPC since the values in the config space are in 
> little endian format.  virtio_config_val does automagic endianness 
> conversion if the size is 2, 4, or 8.  In this case, the structure size 
> is 4 so the endianness conversion will do the wrong thing.

Good catch; byte-swapping an entire structure is a terrible terrible idea.

> Magic endianness conversion based on read size is looking pretty evil to 
> me... Perhaps we need explicit *_val[8,16,32,64]?

Implicit byteswapping based on access size is the standard way of implementing 
accessors.

In this case, reading each structure member individually will do the right 
implicit swapping, rather than trying to load the whole thing as a single 
access.

-- 
Hollis Blanchard
IBM Linux Technology Center
-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone
_______________________________________________
kvm-devel mailing list
kvm-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Re: [PATCH] add virtio disk geometry feature

[Ryan Harper]

* Anthony Liguori <aliguori@us.ibm.com> [2008-04-16 16:33]:
> Rusty Russell wrote:
> >On Thursday 17 April 2008 04:56:37 Ryan Harper wrote:
> >  
> >>From: Ryan Harper <ryanh@us.ibm.com>
> >>
> >>Rather than faking up some geometry, allow the backend to push the disk
> >>geometry via virtio pci config option.  Keep the old geo code around for
> >>compatibility.
> >>    
> >
> >Hi Ryan,
> >
> >   Looks good! Some brief review below.  Mainly just "how I would have done
> >things" stuff.  BTW, does this help in real life?  I assume something in
> >userspace wants it?
> >  
> 
> Boot loaders (like grub) query the geometry from the kernel to figure 
> out how to setup the stage1/stage2.  We've seen strange issues with grub 
> thinking it has crazy geometries when installed on a virtio disk (as 
> opposed to booting from virtio with an existing disk).
> 
> Ryan: have you tested a hardy install with your patches?  Does it help 
> when installing to virtio?  I could pretty reliably reproduce the 
> strangeness with a 20GB disk image FWIW.

I had tested out hardy with a 10G disk and saw nothing out of the
ordinary.  Disk size was the expected value and grub had no issues with
it.

-- 
Ryan Harper
Software Engineer; Linux Technology Center
IBM Corp., Austin, Tx
(512) 838-9253   T/L: 678-9253
ryanh@us.ibm.com