[Qemu-devel] [PATCH 2/2] Add serial number support for virtio_blk, V3

[Qemu-devel] [PATCH 2/2] Add serial number support for virtio_blk, V3

[john cooper]

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


-- 
john.cooper@redhat.com


[-- Attachment #2: virtio_blk-serial-3.patch --]
[-- Type: text/x-patch, Size: 2574 bytes --]

 drivers/block/virtio_blk.c |   32 +++++++++++++++++++++++++++++---
 include/linux/virtio_blk.h |    6 ++++++
 2 files changed, 35 insertions(+), 3 deletions(-)
=================================================================
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -146,12 +146,37 @@ static void do_virtblk_request(struct re
 		vblk->vq->vq_ops->kick(vblk->vq);
 }
 
+/* return serial# in IDE identify data (all other fields are currently zero)
+ */
+static int virtblk_identity(struct block_device *bdev, void *buf)
+{
+	struct virtio_blk *vblk = bdev->bd_disk->private_data;
+	u16 *id;
+	int rv;
+
+	if (!(id = kzalloc(ATA_ID_WORDS, GFP_KERNEL)))
+		rv = -ENOMEM;
+	else if ((rv = virtio_config_buf(vblk->vdev, VIRTIO_BLK_F_SN,
+		offsetof(struct virtio_blk_config, serial), &id[ATA_ID_SERNO],
+		ATA_ID_SERNO_LEN)))
+			;
+	else if (copy_to_user(buf, id, ATA_ID_WORDS))
+		rv = -EFAULT;
+	else
+		rv = 0;
+	if (id)
+        	kfree(id);
+        return (rv);
+}
+
 static int virtblk_ioctl(struct block_device *bdev, fmode_t mode,
 			 unsigned cmd, unsigned long data)
 {
-	return scsi_cmd_ioctl(bdev->bd_disk->queue,
-			      bdev->bd_disk, mode, cmd,
-			      (void __user *)data);
+	if (cmd == HDIO_GET_IDENTITY)
+		return (virtblk_identity(bdev, (void __user *)data));
+	else
+		return scsi_cmd_ioctl(bdev->bd_disk->queue, bdev->bd_disk,
+			mode, cmd, (void __user *)data);
 }
 
 /* We provide getgeo only to please some old bootloader/partitioning tools */
@@ -356,6 +381,7 @@ static struct virtio_device_id id_table[
 static unsigned int features[] = {
 	VIRTIO_BLK_F_BARRIER, VIRTIO_BLK_F_SEG_MAX, VIRTIO_BLK_F_SIZE_MAX,
 	VIRTIO_BLK_F_GEOMETRY, VIRTIO_BLK_F_RO, VIRTIO_BLK_F_BLK_SIZE,
+	VIRTIO_BLK_F_SN
 };
 
 static struct virtio_driver virtio_blk = {
=================================================================
--- a/include/linux/virtio_blk.h
+++ b/include/linux/virtio_blk.h
@@ -15,7 +15,12 @@
 #define VIRTIO_BLK_F_GEOMETRY	4	/* Legacy geometry available  */
 #define VIRTIO_BLK_F_RO		5	/* Disk is read-only */
 #define VIRTIO_BLK_F_BLK_SIZE	6	/* Block size of disk is available*/
+#define VIRTIO_BLK_F_SN		8	/* serial number supported */
 
+#define BLOCK_SERIAL_STRLEN	20
+
+/* mapped into pci i/o region 0
+ */
 struct virtio_blk_config
 {
 	/* The capacity (in 512-byte sectors). */
@@ -32,6 +37,7 @@ struct virtio_blk_config
 	} geometry;
 	/* block size of device (if VIRTIO_BLK_F_BLK_SIZE) */
 	__u32 blk_size;
+	__u8 serial[BLOCK_SERIAL_STRLEN];
 } __attribute__((packed));
 
 /* These two define direction. */

Re: [Qemu-devel] [PATCH 2/2] Add serial number support for virtio_blk, V3

[Christoph Hellwig]

You should probably include rusty as he's collecting the patches
for the virtio guest drivers.

Also can you send the patch inline next time? That makes quoting it for
review a lot easier.


 drivers/block/virtio_blk.c |   32 +++++++++++++++++++++++++++++---
 include/linux/virtio_blk.h |    6 ++++++
 2 files changed, 35 insertions(+), 3 deletions(-)
=================================================================
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -146,12 +146,37 @@ static void do_virtblk_request(struct re
 		vblk->vq->vq_ops->kick(vblk->vq);
 }
 
+/* return serial# in IDE identify data (all other fields are currently zero)
+ */
+static int virtblk_identity(struct block_device *bdev, void *buf)
+{
+	struct virtio_blk *vblk = bdev->bd_disk->private_data;
+	u16 *id;
+	int rv;
+
+	if (!(id = kzalloc(ATA_ID_WORDS, GFP_KERNEL)))
+		rv = -ENOMEM;
+	else if ((rv = virtio_config_buf(vblk->vdev, VIRTIO_BLK_F_SN,
+		offsetof(struct virtio_blk_config, serial), &id[ATA_ID_SERNO],
+		ATA_ID_SERNO_LEN)))
+			;
+	else if (copy_to_user(buf, id, ATA_ID_WORDS))
+		rv = -EFAULT;
+	else
+		rv = 0;
+	if (id)
+        	kfree(id);
+        return (rv);
+}
+
 static int virtblk_ioctl(struct block_device *bdev, fmode_t mode,
 			 unsigned cmd, unsigned long data)
 {
-	return scsi_cmd_ioctl(bdev->bd_disk->queue,
-			      bdev->bd_disk, mode, cmd,
-			      (void __user *)data);
+	if (cmd == HDIO_GET_IDENTITY)
+		return (virtblk_identity(bdev, (void __user *)data));
+	else
+		return scsi_cmd_ioctl(bdev->bd_disk->queue, bdev->bd_disk,
+			mode, cmd, (void __user *)data);
 }
 

This looks functionally correct, but pretty far from normal kernel coding
style.  I'd envision something like this instead:

/*
 * IDE-compatible identify ioctl.
 *
 * Currenlyt only returns the serial number and leaves all other fields
 * zero.
 */
static int virtblk_identity(struct gendisk *disk, void *argp)
{
	struct virtio_blk *vblk = disk->private_data;
	u16 *id;
	int error = -ENOMEM;

	id = kzalloc(ATA_ID_WORDS, GFP_KERNEL)
	if (!id)
		goto out;

	error = virtio_config_buf(vblk->vdev, VIRTIO_BLK_F_SN,
				  offsetof(struct virtio_blk_config, serial),
				  &id[ATA_ID_SERNO], ATA_ID_SERNO_LEN);
	if (error)
		goto out_kfree;

	if (copy_to_user(argp, id, ATA_ID_WORDS))
		error = -EFAULT;

out_kfree:
	kfree(id);
out:
	return error;
}


static int virtblk_ioctl(struct block_device *bdev, fmode_t mode,
			 unsigned cmd, unsigned long arg)
{
	struct gendisk *disk = bdev->bd_disk;
	void __user *argp = (void __user *arg);

	switch (cmd) {
	case HDIO_GET_IDENTITY:
		return virtblk_identity(disk, argp);
	default:
		return scsi_cmd_ioctl(disk->queue, disk, mode, cmd, argp);
	}
}
 

Re: [Qemu-devel] [PATCH 2/2] Add serial number support for virtio_blk, V3

[Christoph Hellwig]

On Wed, May 27, 2009 at 09:49:19AM +0200, Christoph Hellwig wrote:
> /*
>  * IDE-compatible identify ioctl.
>  *
>  * Currenlyt only returns the serial number and leaves all other fields
>  * zero.
>  */

Btw, thinking about it the rest of the information in the ioctl should
probably be filled up with faked data, similar to how we do it for
the ide emulation inside qemu.

Re: [Qemu-devel] [PATCH 2/2] Add serial number support for virtio_blk, V3

[john cooper]

Christoph Hellwig wrote:
> On Wed, May 27, 2009 at 09:49:19AM +0200, Christoph Hellwig wrote:
>> /*
>>  * IDE-compatible identify ioctl.
>>  *
>>  * Currenlyt only returns the serial number and leaves all other fields
>>  * zero.
>>  */
> 
> Btw, thinking about it the rest of the information in the ioctl should
> probably be filled up with faked data, similar to how we do it for
> the ide emulation inside qemu.

Agreed.  I've done so to the extent it makes sense
in the case of the more generic fields.  A fair
amount of the identify information being generated
by hw/ide.c appears obsoleted.

-john

-- 
john.cooper@redhat.com

Re: [Qemu-devel] [PATCH 2/2] Add serial number support for virtio_blk, V3

[Rusty Russell]

On Wed, 27 May 2009 05:19:19 pm Christoph Hellwig wrote:
> You should probably include rusty as he's collecting the patches
> for the virtio guest drivers.

Yes.  It *does* help to cc the maintainer if you want your patches applied :)

And I particularly love untested code like this!

> +	if (!(id = kzalloc(ATA_ID_WORDS, GFP_KERNEL)))
> +		rv = -ENOMEM;

Doesn't ATA_ID_WORDS seem like a strange name for a number of bytes?

What's this *for* BTW?
Rusty.

Re: [Qemu-devel] [PATCH 2/2] Add serial number support for virtio_blk, V3

[john cooper]

>> +	if (!(id = kzalloc(ATA_ID_WORDS, GFP_KERNEL)))
>> +		rv = -ENOMEM;
> 
> Doesn't ATA_ID_WORDS seem like a strange name for a number of bytes?

Yes I caught that bug in the rework as well.

> What's this *for* BTW?

Sorry -- I assumed you were on either list.
Please see patch to follow.

-john

-- 
john.cooper@redhat.com

Re: [Qemu-devel] [PATCH 2/2] Add serial number support for virtio_blk, V3

[john cooper]

Christoph Hellwig wrote:

> This looks functionally correct, but pretty far from normal kernel coding
> style.

I tend to avoid 'goto's.

Christoph Hellwig wrote:
>> /*
>>  * IDE-compatible identify ioctl.
>>  *
>>  * Currenlyt only returns the serial number and leaves all other fields
>>  * zero.
>>  */
> 
> Btw, thinking about it the rest of the information in the ioctl should
> probably be filled up with faked data, similar to how we do it for
> the ide emulation inside qemu.

Doing so crossed my mind but thought it may be better to
start here and provide data on an as-needed basis.  But
as you point out there is precedent (and likely reason)
for hw/ide.c:ide_identify() doing as such.  I don't have
a strong bias either way.  Comments from others?

-john

-- 
john.cooper@redhat.com