Rusty Russell wrote:
> On Thursday 17 April 2008 04:56:37 Ryan Harper wrote:
>
>> From: Ryan Harper <ryanh@xxxxxxxxxx>
>>
>> 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
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/virtualization
