[Qemu-devel] [PATCH 0/4] Add virtio disk identification support

[Qemu-devel] [PATCH 0/4] Add virtio disk identification support

[john cooper]

This series adds the minimal support to qemu and virtio_blk
to support passing of a virtio_blk serial id string from qemu
through the guest driver and to the guest userland.

This is derived in part from a patch set posted by Rusty some
time ago, but has been minimized to remove support for prior
versions which attempted to provide the same functionality via
pci config/io space.  This version rather uses a virtio request
as proposed in Rusty's example.

Also removed is the packaging of the serial/id string within
the glorious bag of bits returned by the ATA_IDENTIFY command.
Here we transfer only the 20 bytes of serial/id string from
qemu to the guest userland.  In the proposed interface, this
is made available by an ioctl() into the virtio_blk driver
however other interfaces (eg: /sys) have also been proposed.
A code snippet is attached below as an example of ioctl usage.

The resulting code is quite minimal and I believe it addresses
all concerns raised in prior versions.

-john



#include <stdio.h>
#include <strings.h>
#include <sys/types.h>
#include <fcntl.h>
#include <linux/hdreg.h>

#define IOCTL_CMD	'VBID'

main()
{
	int fd, rv;
	char buf[512];

	bzero(buf, sizeof (buf));
	if ((fd = open("/dev/vda", O_RDONLY)) < 0)
		perror("open");
	else if (ioctl(fd, IOCTL_CMD, buf) < 0)
		perror("ioctl");
	else
		printf("[%s]\n", buf);
}

-- 
john.cooper@redhat.com

Re: [Qemu-devel] [PATCH 0/4] Add virtio disk identification support

[Ryan Harper]

* john cooper <john.cooper@redhat.com> [2010-03-25 00:45]:
> This series adds the minimal support to qemu and virtio_blk
> to support passing of a virtio_blk serial id string from qemu
> through the guest driver and to the guest userland.
> 
> This is derived in part from a patch set posted by Rusty some
> time ago, but has been minimized to remove support for prior
> versions which attempted to provide the same functionality via
> pci config/io space.  This version rather uses a virtio request
> as proposed in Rusty's example.
> 
> Also removed is the packaging of the serial/id string within
> the glorious bag of bits returned by the ATA_IDENTIFY command.
> Here we transfer only the 20 bytes of serial/id string from
> qemu to the guest userland.  In the proposed interface, this
> is made available by an ioctl() into the virtio_blk driver
> however other interfaces (eg: /sys) have also been proposed.
> A code snippet is attached below as an example of ioctl usage.
> 
> The resulting code is quite minimal and I believe it addresses
> all concerns raised in prior versions.
> 
> -john
> 
> 
> 
> #include <stdio.h>
> #include <strings.h>
> #include <sys/types.h>
> #include <fcntl.h>
> #include <linux/hdreg.h>
> 
> #define IOCTL_CMD	'VBID'
> 
> main()
> {
> 	int fd, rv;
> 	char buf[512];
> 
> 	bzero(buf, sizeof (buf));
> 	if ((fd = open("/dev/vda", O_RDONLY)) < 0)
> 		perror("open");
> 	else if (ioctl(fd, IOCTL_CMD, buf) < 0)
> 		perror("ioctl");
> 	else
> 		printf("[%s]\n", buf);
> }

Would we want to patch up blkid command to use this so distro stacks can
extract the serial and build the typical /dev/disk/by-id/ links ?


-- 
Ryan Harper
Software Engineer; Linux Technology Center
IBM Corp., Austin, Tx
ryanh@us.ibm.com

Re: [Qemu-devel] [PATCH 0/4] Add virtio disk identification support

[Ryan Harper]

* john cooper <john.cooper@redhat.com> [2010-03-25 00:45]:
> This series adds the minimal support to qemu and virtio_blk
> to support passing of a virtio_blk serial id string from qemu
> through the guest driver and to the guest userland.
> 
> This is derived in part from a patch set posted by Rusty some
> time ago, but has been minimized to remove support for prior
> versions which attempted to provide the same functionality via
> pci config/io space.  This version rather uses a virtio request
> as proposed in Rusty's example.
> 
> Also removed is the packaging of the serial/id string within
> the glorious bag of bits returned by the ATA_IDENTIFY command.
> Here we transfer only the 20 bytes of serial/id string from
> qemu to the guest userland.  In the proposed interface, this
> is made available by an ioctl() into the virtio_blk driver
> however other interfaces (eg: /sys) have also been proposed.
> A code snippet is attached below as an example of ioctl usage.
> 
> The resulting code is quite minimal and I believe it addresses
> all concerns raised in prior versions.
> 
> -john

I've applied the qemu and kernel side of this patch series and tested
this out using the sample code below.  I've also reworked this example
into a virtioblk_id tool to work with udev to generate /dev/disk/by-id
links; I'll be submitting a patch set to linux-hotplug with these
changes, (and update for path_id) and some udev rules to
persistent-storage script to autogenerate by-id and by-path symlinks for
virtio-blk devices.

I've also got a patch to apply ontop of the qemu patches to generate a
default serial number if one isn't specified (like we do for ide
devices).

Anthony, is this series in your queue yet?

Acked-by: Ryan Harper <ryanh@us.ibm.com>

> 
> 
> 
> #include <stdio.h>
> #include <strings.h>
> #include <sys/types.h>
> #include <fcntl.h>
> #include <linux/hdreg.h>
> 
> #define IOCTL_CMD	'VBID'
> 
> main()
> {
> 	int fd, rv;
> 	char buf[512];
> 
> 	bzero(buf, sizeof (buf));
> 	if ((fd = open("/dev/vda", O_RDONLY)) < 0)
> 		perror("open");
> 	else if (ioctl(fd, IOCTL_CMD, buf) < 0)
> 		perror("ioctl");
> 	else
> 		printf("[%s]\n", buf);
> }
> 
> -- 
> john.cooper@redhat.com
> 

-- 
Ryan Harper
Software Engineer; Linux Technology Center
IBM Corp., Austin, Tx
ryanh@us.ibm.com

Re: [Qemu-devel] [PATCH 0/4] Add virtio disk identification support

[john cooper]

Ryan Harper wrote:
> I've applied the qemu and kernel side of this patch series and tested
> this out using the sample code below.  I've also reworked this example
> into a virtioblk_id tool to work with udev to generate /dev/disk/by-id
> links; I'll be submitting a patch set to linux-hotplug with these
> changes, (and update for path_id) and some udev rules to
> persistent-storage script to autogenerate by-id and by-path symlinks for
> virtio-blk devices.

Sorry I didn't respond to you earlier.  Actually the
guest visible interface below was only intended as an
example while waiting for (what I misinterpreted as
Marc's intention to create) a /sys interface.

I'm all for putting this issue to rest, but if we're
going to live with an ioctl interface retrieving the
id string, let's make it a little more friendly from
the user's perspective.  I have a slightly modified
version which basically implements the same interface
but expects the sizeof the entire array to be preset
in the first element.  Otherwise the user has no
way of informing the driver of the destination's size,
nor a way for the driver to indicate when the data
won't fit in the area specified by the user.

I should still have the patch on a test machine which
ATM is unaccessible, but will have at first thing tomorrow.
Let's hold off until then so we can address this nit
and avoid yet another hiccup in nailing down this
interface.

Thanks,

-john

> I've also got a patch to apply ontop of the qemu patches to generate a
> default serial number if one isn't specified (like we do for ide
> devices).
> 
> Anthony, is this series in your queue yet?
> 
> Acked-by: Ryan Harper <ryanh@us.ibm.com>
> 
>>
>>
>> #include <stdio.h>
>> #include <strings.h>
>> #include <sys/types.h>
>> #include <fcntl.h>
>> #include <linux/hdreg.h>
>>
>> #define IOCTL_CMD	'VBID'
>>
>> main()
>> {
>> 	int fd, rv;
>> 	char buf[512];
>>
>> 	bzero(buf, sizeof (buf));
>> 	if ((fd = open("/dev/vda", O_RDONLY)) < 0)
>> 		perror("open");
>> 	else if (ioctl(fd, IOCTL_CMD, buf) < 0)
>> 		perror("ioctl");
>> 	else
>> 		printf("[%s]\n", buf);
>> }
>>
>> -- 
>> john.cooper@redhat.com
>>
> 


-- 
john.cooper@redhat.com

Re: [Qemu-devel] [PATCH 0/4] Add virtio disk identification support

[john cooper]

john cooper wrote:
> I'm all for putting this issue to rest, but if we're
> going to live with an ioctl interface retrieving the
> id string, let's make it a little more friendly from
> the user's perspective.

The qemu side of the patch is ok as-is.  The guest-user
interface issue is contained in the driver.  While I
see the example ioctl patch has been incorporated into
the virtio_blk driver, there can be no data retrieved
through this interface as virtblk_get_id() will fail
without the qemu counterpart.  So we can clean up the
details without concern of existing usage.

The only difference (as above) is allowing the caller
to pass a buffer size to the driver and the driver
informing the caller of the total number of bytes
available:

#include <stdio.h>
#include <strings.h>
#include <fcntl.h>

#define IOCTL_CMD       'VBID'
#define BUFSZ           10

main()
{
        int fd, rv;
        char buf[255];

        bzero(buf, sizeof (buf));
        buf[0] = BUFSZ;
        if ((fd = open("/dev/vda", O_RDONLY)) < 0)
                perror("open");
        else if ((rv = ioctl(fd, IOCTL_CMD, buf)) < 0)
                perror("ioctl");
        else
                printf("[%s] %d of %d bytes\n", buf,
                       BUFSZ < rv ? BUFSZ : rv, rv);
}


Signed-off-by: john cooper <john.cooper@redhat.com>
---

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 83fa09a..6237732 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -225,15 +225,29 @@ static int virtblk_ioctl(struct block_device *bdev, fmode_t mode,
 	struct gendisk *disk = bdev->bd_disk;
 	struct virtio_blk *vblk = disk->private_data;
 
+	/* user passes the address of a char[] for return of the id string
+	 * and has set char[0] to the array size.  copy id string to this
+	 * char[] and return the number of non-nul characters in the internal
+	 * id string.  The caller can then determine if all were received.
+	 */
 	if (cmd == 0x56424944) { /* 'VBID' */
 		void __user *usr_data = (void __user *)data;
 		char id_str[VIRTIO_BLK_ID_BYTES];
-		int err;
-
-		err = virtblk_get_id(disk, id_str);
-		if (!err && copy_to_user(usr_data, id_str, VIRTIO_BLK_ID_BYTES))
-			err = -EFAULT;
-		return err;
+		unsigned char idlen;
+		int rv;
+
+		if (copy_from_user(&idlen, usr_data, sizeof (idlen)))
+			return -EFAULT;
+		if (VIRTIO_BLK_ID_BYTES < idlen)
+			idlen = VIRTIO_BLK_ID_BYTES;
+		if ((rv = virtblk_get_id(disk, id_str)))
+			return rv;
+		if (copy_to_user(usr_data, id_str, idlen))
+			return -EFAULT;
+		for (rv = 0; rv < VIRTIO_BLK_ID_BYTES; ++rv)
+			if (!id_str[rv])
+				break;
+		return rv;
 	}
 	/*
 	 * Only allow the generic SCSI ioctls if the host can support it.

-- 
john.cooper@redhat.com

[Qemu-devel] [PATCH 0/4] Add virtio disk identification support

[john cooper]

This patch adds the final missing bits for support of
passing a serial/id string to a virtio-blk guest driver.

The guest-side component already exists in the virtio
driver, and has recently been reworked by Ryan to export
a /sys interface for retrival of the id from guest userland.

Signed-off-by: john cooper<john.cooper@redhat.com>
---

diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index 0bf929a..bd6b896 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -26,6 +26,7 @@ typedef struct VirtIOBlock
     QEMUBH *bh;
     BlockConf *conf;
     unsigned short sector_mask;
+    char sn[BLOCK_SERIAL_STRLEN];
 } VirtIOBlock;
 
 static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev)
@@ -324,6 +325,12 @@ static void virtio_blk_handle_request(VirtIOBlockReq *req,
         virtio_blk_handle_flush(req, mrb);
     } else if (req->out->type & VIRTIO_BLK_T_SCSI_CMD) {
         virtio_blk_handle_scsi(req);
+    } else if (req->out->type & VIRTIO_BLK_T_GET_ID) {
+        VirtIOBlock *s = req->dev;
+
+        memcpy(req->elem.in_sg[0].iov_base, s->sn,
+               MIN(req->elem.in_sg[0].iov_len, sizeof(s->sn)));
+        virtio_blk_req_complete(req, VIRTIO_BLK_S_OK);
     } else if (req->out->type & VIRTIO_BLK_T_OUT) {
         qemu_iovec_init_external(&req->qiov, &req->elem.out_sg[1],
                                  req->elem.out_num - 1);
@@ -495,6 +502,12 @@ VirtIODevice *virtio_blk_init(DeviceState *dev, BlockConf *conf)
     s->sector_mask = (s->conf->logical_block_size / BDRV_SECTOR_SIZE) - 1;
     bdrv_guess_geometry(s->bs, &cylinders, &heads, &secs);
 
+    /* NB: per existing s/n string convention we really intend to hard-limit
+     * the copy length to sizeof (s->sn) even in the case we're left without
+     * a trailing '\0'
+     */
+    strncpy(s->sn, drive_get_serial(s->bs), sizeof (s->sn));
+
     s->vq = virtio_add_queue(&s->vdev, 128, virtio_blk_handle_output);
 
     qemu_add_vm_change_state_handler(virtio_blk_dma_restart_cb, s);
diff --git a/hw/virtio-blk.h b/hw/virtio-blk.h
index 7a7ece3..fff46da 100644
--- a/hw/virtio-blk.h
+++ b/hw/virtio-blk.h
@@ -59,6 +59,9 @@ struct virtio_blk_config
 /* Flush the volatile write cache */
 #define VIRTIO_BLK_T_FLUSH      4
 
+/* return the device ID string */
+#define VIRTIO_BLK_T_GET_ID     8
+
 /* Barrier before this op. */
 #define VIRTIO_BLK_T_BARRIER    0x80000000
 
-- 
john.cooper@redhat.com

Re: [Qemu-devel] [PATCH 0/4] Add virtio disk identification support

[Markus Armbruster]

Subject is confusing: suggests a series of four parts.

john cooper <john.cooper@redhat.com> writes:

> This patch adds the final missing bits for support of
> passing a serial/id string to a virtio-blk guest driver.
>
> The guest-side component already exists in the virtio
> driver, and has recently been reworked by Ryan to export
> a /sys interface for retrival of the id from guest userland.
>
> Signed-off-by: john cooper<john.cooper@redhat.com>

Looks like this conflicts with my "PATCH v3 00/13] More block-related
fixes and cleanups".

Perhaps the easiest way to get this in would be to rebase to Kevin's
block branch, which already has my series, and cc the respin to him.
See ide_dev_initfn() and scsi_initfn() there.

Kevin's tree: git://repo.or.cz/qemu/kevin.git

Re: [Qemu-devel] [PATCH 0/4] Add virtio disk identification support

[john cooper]

Markus Armbruster wrote:
> Subject is confusing: suggests a series of four parts.

Sorry.  My bad for recycling old mail.

> Looks like this conflicts with my "PATCH v3 00/13] More block-related
> fixes and cleanups".
> 
> Perhaps the easiest way to get this in would be to rebase to Kevin's
> block branch, which already has my series, and cc the respin to him.
> See ide_dev_initfn() and scsi_initfn() there.
> 
> Kevin's tree: git://repo.or.cz/qemu/kevin.git

Will do.

-john

-- 
john.cooper@third-harmonic.com