All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH] Add serial number support for virtio_blk
@ 2009-06-22 18:26 john cooper
  2009-06-22 18:46 ` [Qemu-devel] " Anthony Liguori
  2009-06-22 19:05 ` Anthony Liguori
  0 siblings, 2 replies; 10+ messages in thread
From: john cooper @ 2009-06-22 18:26 UTC (permalink / raw)
  To: qemu-devel; +Cc: john.cooper

[brought forward to current qemu-kvm.git]

This patch implements the missing qemu logic to
interpret a '-drive .. serial=XYZ ..' flag for
a virtio_blk device.

The serial number string is contained in a
skeletal IDENTIFY DEVICE data structure and
this structure is made available to the guest
virtio_blk driver via pci i/o region 0.

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

diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index dad4ef0..8e675a8 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -25,6 +25,7 @@ typedef struct VirtIOBlock
     BlockDriverState *bs;
     VirtQueue *vq;
     void *rq;
+    char serial_str[BLOCK_SERIAL_STRLEN + 1];
 } VirtIOBlock;
 
 static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev)
@@ -32,6 +33,47 @@ static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev)
     return (VirtIOBlock *)vdev;
 }
 
+/* store identify data in little endian format
+ */
+static inline void put_le16(uint16_t *p, unsigned int v)
+{
+    *p = cpu_to_le16(v);
+}
+
+/* copy to *dst from *src, nul pad dst tail as needed to len bytes
+ */
+static inline void padstr(char *dst, const char *src, int len)
+{
+    while (len--)
+        *dst++ = *src ? *src++ : '\0';
+}
+
+/* setup simulated identify data as appropriate for virtio block device
+ *
+ * ref: AT Attachment 8 - ATA/ATAPI Command Set (ATA8-ACS)
+ */
+static inline void virtio_identify_template(struct virtio_blk_config *bc)
+{
+    uint16_t *p = &bc->identify[0];
+    uint64_t lba_sectors = bc->capacity;
+
+    memset(p, 0, sizeof(bc->identify));
+    put_le16(p + 0, 0x0);                            /* ATA device */
+    padstr((char *)(p + 23), QEMU_VERSION, 8);       /* firmware revision */
+    padstr((char *)(p + 27), "QEMU VIRT_BLK", 40);   /* model# */
+    put_le16(p + 47, 0x80ff);                        /* max xfer 255 sectors */
+    put_le16(p + 49, 0x0b00);                        /* support IORDY/LBA/DMA */
+    put_le16(p + 59, 0x1ff);                         /* cur xfer 255 sectors */
+    put_le16(p + 80, 0x1f0);                         /* support ATA8/7/6/5/4 */
+    put_le16(p + 81, 0x16);
+    put_le16(p + 82, 0x400);
+    put_le16(p + 83, 0x400);
+    put_le16(p + 100, lba_sectors);
+    put_le16(p + 101, lba_sectors >> 16);
+    put_le16(p + 102, lba_sectors >> 32);
+    put_le16(p + 103, lba_sectors >> 48);
+}
+
 typedef struct VirtIOBlockReq
 {
     VirtIOBlock *dev;
@@ -285,6 +327,8 @@ static void virtio_blk_reset(VirtIODevice *vdev)
     qemu_aio_flush();
 }
 
+/* coalesce internal state, copy to pci i/o region 0
+ */
 static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config)
 {
     VirtIOBlock *s = to_virtio_blk(vdev);
@@ -299,11 +343,15 @@ static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config)
     stw_raw(&blkcfg.cylinders, cylinders);
     blkcfg.heads = heads;
     blkcfg.sectors = secs;
+    virtio_identify_template(&blkcfg);
+    memcpy(&blkcfg.identify[VIRTIO_BLK_ID_SN], s->serial_str,
+        VIRTIO_BLK_ID_SN_BYTES);
     memcpy(config, &blkcfg, sizeof(blkcfg));
 }
 
 static uint32_t virtio_blk_get_features(VirtIODevice *vdev)
 {
+    VirtIOBlock *s = to_virtio_blk(vdev);
     uint32_t features = 0;
 
     features |= (1 << VIRTIO_BLK_F_SEG_MAX);
@@ -311,6 +359,8 @@ static uint32_t virtio_blk_get_features(VirtIODevice *vdev)
 #ifdef __linux__
     features |= (1 << VIRTIO_BLK_F_SCSI);
 #endif
+    if (strcmp(s->serial_str, "0"))
+        features |= 1 << VIRTIO_BLK_F_IDENTIFY;
 
     return features;
 }
@@ -353,6 +403,7 @@ void *virtio_blk_init(PCIBus *bus, BlockDriverState *bs)
     VirtIOBlock *s;
     int cylinders, heads, secs;
     static int virtio_blk_id;
+    char *ps;
 
     s = (VirtIOBlock *)virtio_init_pci(bus, "virtio-blk",
                                        PCI_VENDOR_ID_REDHAT_QUMRANET,
@@ -369,6 +420,10 @@ void *virtio_blk_init(PCIBus *bus, BlockDriverState *bs)
     s->vdev.reset = virtio_blk_reset;
     s->bs = bs;
     s->rq = NULL;
+    if (strlen(ps = (char *)drive_get_serial(bs)))
+        strncpy(s->serial_str, ps, sizeof(s->serial_str));
+    else
+        snprintf(s->serial_str, sizeof(s->serial_str), "0");
     bs->private = &s->vdev.pci_dev;
     bdrv_guess_geometry(s->bs, &cylinders, &heads, &secs);
     bdrv_set_geometry_hint(s->bs, cylinders, heads, secs);
diff --git a/hw/virtio-blk.h b/hw/virtio-blk.h
index 5ef6c36..7555492 100644
--- a/hw/virtio-blk.h
+++ b/hw/virtio-blk.h
@@ -31,6 +31,11 @@
 #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_SCSI       7       /* Supports scsi command passthru */
+#define VIRTIO_BLK_F_IDENTIFY   8       /* ATA IDENTIFY supported */
+
+#define VIRTIO_BLK_ID_LEN       256     /* length of identify u16 array */
+#define VIRTIO_BLK_ID_SN        10      /* start of char * serial# */
+#define VIRTIO_BLK_ID_SN_BYTES  20      /* length in bytes of serial# */
 
 struct virtio_blk_config
 {
@@ -40,6 +45,8 @@ struct virtio_blk_config
     uint16_t cylinders;
     uint8_t heads;
     uint8_t sectors;
+    uint32_t _blk_size;    /* structure pad, currently unused */
+    uint16_t identify[VIRTIO_BLK_ID_LEN];
 } __attribute__((packed));
 
 /* These two define direction. */
diff --git a/sysemu.h b/sysemu.h
index 1f45fd6..185b4e3 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -141,6 +141,8 @@ typedef enum {
     BLOCK_ERR_STOP_ANY
 } BlockInterfaceErrorAction;
 
+#define BLOCK_SERIAL_STRLEN 20
+
 typedef struct DriveInfo {
     BlockDriverState *bdrv;
     BlockInterfaceType type;
@@ -149,7 +151,7 @@ typedef struct DriveInfo {
     int used;
     int drive_opt_idx;
     BlockInterfaceErrorAction onerror;
-    char serial[21];
+    char serial[BLOCK_SERIAL_STRLEN + 1];
 } DriveInfo;
 
 #define MAX_IDE_DEVS	2


-- 
john.cooper@redhat.com

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [Qemu-devel] Re: [PATCH] Add serial number support for virtio_blk
  2009-06-22 18:26 [Qemu-devel] [PATCH] Add serial number support for virtio_blk john cooper
@ 2009-06-22 18:46 ` Anthony Liguori
  2009-06-23 12:42   ` john cooper
  2009-06-22 19:05 ` Anthony Liguori
  1 sibling, 1 reply; 10+ messages in thread
From: Anthony Liguori @ 2009-06-22 18:46 UTC (permalink / raw)
  To: john cooper; +Cc: qemu-devel

john cooper wrote:
> @@ -353,6 +403,7 @@ void *virtio_blk_init(PCIBus *bus, BlockDriverState *bs)
>      VirtIOBlock *s;
>      int cylinders, heads, secs;
>      static int virtio_blk_id;
> +    char *ps;
>   
const char *ps;
>  
>      s = (VirtIOBlock *)virtio_init_pci(bus, "virtio-blk",
>                                         PCI_VENDOR_ID_REDHAT_QUMRANET,
> @@ -369,6 +420,10 @@ void *virtio_blk_init(PCIBus *bus, BlockDriverState *bs)
>      s->vdev.reset = virtio_blk_reset;
>      s->bs = bs;
>      s->rq = NULL;
> +    if (strlen(ps = (char *)drive_get_serial(bs)))
> +        strncpy(s->serial_str, ps, sizeof(s->serial_str));
> +    else
> +        snprintf(s->serial_str, sizeof(s->serial_str), "0");
>   

ps = drive_get_serial(bs);
snprintf(s->serial_str, sizeof(s->serial_str), "%s", *ps ? ps : "0");

strncpy() doesn't do what you think it does.  It doesn't always null 
terminate.

Doesn't serial_str need to be saved in the savevm format?

Regards,

Anthony Liguori

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [Qemu-devel] Re: [PATCH] Add serial number support for virtio_blk
  2009-06-22 18:26 [Qemu-devel] [PATCH] Add serial number support for virtio_blk john cooper
  2009-06-22 18:46 ` [Qemu-devel] " Anthony Liguori
@ 2009-06-22 19:05 ` Anthony Liguori
  2009-06-22 19:30   ` Gleb Natapov
  2009-06-23 12:44   ` john cooper
  1 sibling, 2 replies; 10+ messages in thread
From: Anthony Liguori @ 2009-06-22 19:05 UTC (permalink / raw)
  To: john cooper; +Cc: qemu-devel

john cooper wrote:
> [brought forward to current qemu-kvm.git]
>
> This patch implements the missing qemu logic to
> interpret a '-drive .. serial=XYZ ..' flag for
> a virtio_blk device.
>
> The serial number string is contained in a
> skeletal IDENTIFY DEVICE data structure and
> this structure is made available to the guest
> virtio_blk driver via pci i/o region 0.
>
> Signed-off-by: john cooper <john.cooper@redhat.com>
>   
I'm trying to write a test case for this functionality.  The first thing 
I observed is that serial isn't included in info block.  Could you add 
it?  This will let us figure out what the serial number ought to be so 
that we can then verify it in the guest.

So far, I don't see a wonderful way to verify a serial number in the 
guest.  Looks like the information can be gotten indirectly via hdparm 
-i or by looking at /dev/disk/by-id.  The later doesn't have an easy to 
predict name though.

Any thoughts?

I also noticed that if=scsi,serial=XXX doesn't seem to do anything useful.

Regards,

Anthony Liguori

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] Re: [PATCH] Add serial number support for virtio_blk
  2009-06-22 19:05 ` Anthony Liguori
@ 2009-06-22 19:30   ` Gleb Natapov
  2009-06-22 20:28     ` Anthony Liguori
  2009-06-23 12:44   ` john cooper
  1 sibling, 1 reply; 10+ messages in thread
From: Gleb Natapov @ 2009-06-22 19:30 UTC (permalink / raw)
  To: Anthony Liguori; +Cc: john cooper, qemu-devel

On Mon, Jun 22, 2009 at 02:05:12PM -0500, Anthony Liguori wrote:
> I also noticed that if=scsi,serial=XXX doesn't seem to do anything useful.
>
What makes you think so?

--
			Gleb.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] Re: [PATCH] Add serial number support for virtio_blk
  2009-06-22 19:30   ` Gleb Natapov
@ 2009-06-22 20:28     ` Anthony Liguori
  2009-06-22 20:38       ` Gleb Natapov
  0 siblings, 1 reply; 10+ messages in thread
From: Anthony Liguori @ 2009-06-22 20:28 UTC (permalink / raw)
  To: Gleb Natapov; +Cc: john cooper, qemu-devel

Gleb Natapov wrote:
> On Mon, Jun 22, 2009 at 02:05:12PM -0500, Anthony Liguori wrote:
>   
>> I also noticed that if=scsi,serial=XXX doesn't seem to do anything useful.
>>
>>     
> What makes you think so?
>   

It doesn't seem to change /dev/disk/by-id/PATH

Regards,

Anthony Liguori

> --
> 			Gleb.
>   

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] Re: [PATCH] Add serial number support for virtio_blk
  2009-06-22 20:28     ` Anthony Liguori
@ 2009-06-22 20:38       ` Gleb Natapov
  2009-06-22 20:42         ` Daniel P. Berrange
  0 siblings, 1 reply; 10+ messages in thread
From: Gleb Natapov @ 2009-06-22 20:38 UTC (permalink / raw)
  To: Anthony Liguori; +Cc: john cooper, qemu-devel

On Mon, Jun 22, 2009 at 03:28:40PM -0500, Anthony Liguori wrote:
> Gleb Natapov wrote:
>> On Mon, Jun 22, 2009 at 02:05:12PM -0500, Anthony Liguori wrote:
>>   
>>> I also noticed that if=scsi,serial=XXX doesn't seem to do anything useful.
>>>
>>>     
>> What makes you think so?
>>   
>
> It doesn't seem to change /dev/disk/by-id/PATH
>
I thought you mean that there is no way to get it from guest hence the
question. I don't know what Linux uses for /dev/disk/by-id/PATH, but
Windows retrieve disk serial using the scsi command that QEMU
implements.

--
			Gleb.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] Re: [PATCH] Add serial number support for virtio_blk
  2009-06-22 20:38       ` Gleb Natapov
@ 2009-06-22 20:42         ` Daniel P. Berrange
  2009-06-23  8:22           ` Gleb Natapov
  0 siblings, 1 reply; 10+ messages in thread
From: Daniel P. Berrange @ 2009-06-22 20:42 UTC (permalink / raw)
  To: Gleb Natapov; +Cc: john cooper, qemu-devel

On Mon, Jun 22, 2009 at 11:38:51PM +0300, Gleb Natapov wrote:
> On Mon, Jun 22, 2009 at 03:28:40PM -0500, Anthony Liguori wrote:
> > Gleb Natapov wrote:
> >> On Mon, Jun 22, 2009 at 02:05:12PM -0500, Anthony Liguori wrote:
> >>   
> >>> I also noticed that if=scsi,serial=XXX doesn't seem to do anything useful.
> >>>
> >>>     
> >> What makes you think so?
> >>   
> >
> > It doesn't seem to change /dev/disk/by-id/PATH
> >
> I thought you mean that there is no way to get it from guest hence the
> question. I don't know what Linux uses for /dev/disk/by-id/PATH, but
> Windows retrieve disk serial using the scsi command that QEMU
> implements.

It is done by udev rules, which call out to a 'scsi_id' command. On a 
Fedora 11 host /lib/udev/rules.d/60-persistent-storage.rules

Regards,
Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] Re: [PATCH] Add serial number support for virtio_blk
  2009-06-22 20:42         ` Daniel P. Berrange
@ 2009-06-23  8:22           ` Gleb Natapov
  0 siblings, 0 replies; 10+ messages in thread
From: Gleb Natapov @ 2009-06-23  8:22 UTC (permalink / raw)
  To: Daniel P. Berrange; +Cc: john cooper, qemu-devel

On Mon, Jun 22, 2009 at 09:42:16PM +0100, Daniel P. Berrange wrote:
> On Mon, Jun 22, 2009 at 11:38:51PM +0300, Gleb Natapov wrote:
> > On Mon, Jun 22, 2009 at 03:28:40PM -0500, Anthony Liguori wrote:
> > > Gleb Natapov wrote:
> > >> On Mon, Jun 22, 2009 at 02:05:12PM -0500, Anthony Liguori wrote:
> > >>   
> > >>> I also noticed that if=scsi,serial=XXX doesn't seem to do anything useful.
> > >>>
> > >>>     
> > >> What makes you think so?
> > >>   
> > >
> > > It doesn't seem to change /dev/disk/by-id/PATH
> > >
> > I thought you mean that there is no way to get it from guest hence the
> > question. I don't know what Linux uses for /dev/disk/by-id/PATH, but
> > Windows retrieve disk serial using the scsi command that QEMU
> > implements.
> 
> It is done by udev rules, which call out to a 'scsi_id' command. On a 
> Fedora 11 host /lib/udev/rules.d/60-persistent-storage.rules
> 
According to man page scsi_id uses VPD page 0x80 or 0x83. QEMU implement
both, but puts serial into page 0x80. scsi_id -p 0x80 -s /dev/sda should
print something similar to what was provided to serial option.

--
			Gleb.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [Qemu-devel] Re: [PATCH] Add serial number support for virtio_blk
  2009-06-22 18:46 ` [Qemu-devel] " Anthony Liguori
@ 2009-06-23 12:42   ` john cooper
  0 siblings, 0 replies; 10+ messages in thread
From: john cooper @ 2009-06-23 12:42 UTC (permalink / raw)
  To: Anthony Liguori; +Cc: john.cooper, qemu-devel

Anthony Liguori wrote:
> john cooper wrote:
>>
>>      s = (VirtIOBlock *)virtio_init_pci(bus, "virtio-blk",
>>                                         PCI_VENDOR_ID_REDHAT_QUMRANET,
>> @@ -369,6 +420,10 @@ void *virtio_blk_init(PCIBus *bus,
>> BlockDriverState *bs)
>>      s->vdev.reset = virtio_blk_reset;
>>      s->bs = bs;
>>      s->rq = NULL;
>> +    if (strlen(ps = (char *)drive_get_serial(bs)))
>> +        strncpy(s->serial_str, ps, sizeof(s->serial_str));
>> +    else
>> +        snprintf(s->serial_str, sizeof(s->serial_str), "0");
>>   
> 
> ps = drive_get_serial(bs);
> snprintf(s->serial_str, sizeof(s->serial_str), "%s", *ps ? ps : "0");
> 
> strncpy() doesn't do what you think it does.  It doesn't always null
> terminate.

In general yes, but here it is contrived to copy a
terminating nul.  The string is maintained as a 21
byte [BLOCK_SERIAL_STRLEN + 1] char[] and the
incoming cmdline serial string is hard null terminated
by get_opt_value().  Above, strncpy() into a sizeof(21)
byte s->serial_str, will copy the trailing nul.
However when this data is exported by the guest driver
in a char[20] sized structure the trailing nul may be
omitted.

The same logic may be found in IDE and SCSI counterparts.
I agree it's not the most obvious approach, and this
clause should ideally be factored out as common to
all cases once we have the patch under discussion
resolved.

> Doesn't serial_str need to be saved in the savevm format?

Possibly, but currently it isn't being captured in
IDE nor SCSI AFAICT.  I'll take a closer look.

-john

-- 
john.cooper@redhat.com

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [Qemu-devel] Re: [PATCH] Add serial number support for virtio_blk
  2009-06-22 19:05 ` Anthony Liguori
  2009-06-22 19:30   ` Gleb Natapov
@ 2009-06-23 12:44   ` john cooper
  1 sibling, 0 replies; 10+ messages in thread
From: john cooper @ 2009-06-23 12:44 UTC (permalink / raw)
  To: Anthony Liguori; +Cc: john.cooper, qemu-devel

Anthony Liguori wrote:
> 
> I'm trying to write a test case for this functionality.  The first thing
> I observed is that serial isn't included in info block.  Could you add
> it?  This will let us figure out what the serial number ought to be so
> that we can then verify it in the guest.

Yes, it seems to be missing for all block devices.

> So far, I don't see a wonderful way to verify a serial number in the
> guest.  Looks like the information can be gotten indirectly via hdparm
> -i or by looking at /dev/disk/by-id.  The later doesn't have an easy to
> predict name though.
> 
> Any thoughts?

Initially I added a new ioctl in the guest driver
to mine out the serial number.  Admittedly it was
a wart as both IDE and SCSI already had methods
to accomplish the same.  Emulating an IDE IDENTIFY
command was the simplest alternative and allowed
existing utilities such as 'hdparm -i /dev/vda' to
retrieve the information, which is essentially
what I had used to verify the functionality.

-john

-- 
john.cooper@redhat.com

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2009-06-23 12:48 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-06-22 18:26 [Qemu-devel] [PATCH] Add serial number support for virtio_blk john cooper
2009-06-22 18:46 ` [Qemu-devel] " Anthony Liguori
2009-06-23 12:42   ` john cooper
2009-06-22 19:05 ` Anthony Liguori
2009-06-22 19:30   ` Gleb Natapov
2009-06-22 20:28     ` Anthony Liguori
2009-06-22 20:38       ` Gleb Natapov
2009-06-22 20:42         ` Daniel P. Berrange
2009-06-23  8:22           ` Gleb Natapov
2009-06-23 12:44   ` john cooper

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.