Re: [Qemu-devel] [PATCH 2/2] Add serial number support for virtio_blk, V3
+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.coo...@redhat.com -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH 2/2] Add serial number support for virtio_blk, V3
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.coo...@redhat.com -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH 2/2] Add serial number support for virtio_blk, V3
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);
}
}
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH 2/2] Add serial number support for virtio_blk, V3
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. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH 2/2] Add serial number support for virtio_blk, V3
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. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] Add serial number support for virtio_blk, V3
--
john.coo...@redhat.com
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. */
