Patchwork virtio-blk: Turn drive serial into a qdev property

login
register
mail settings
Submitter Markus Armbruster
Date June 20, 2011, 9:35 a.m.
Message ID <1308562518-21322-1-git-send-email-armbru@redhat.com>
Download mbox | patch
Permalink /patch/101072/
State New
Headers show

Comments

Markus Armbruster - June 20, 2011, 9:35 a.m.
It needs to be a qdev property, because it belongs to the drive's
guest part.  Precedence: commit a0fef654 and 6ced55a5.

Bonus: info qtree now shows the serial number.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/s390-virtio-bus.c |    4 +++-
 hw/s390-virtio-bus.h |    1 +
 hw/virtio-blk.c      |   29 +++++++++++++++++++----------
 hw/virtio-blk.h      |    2 ++
 hw/virtio-pci.c      |    4 +++-
 hw/virtio-pci.h      |    1 +
 hw/virtio.h          |    3 ++-
 7 files changed, 31 insertions(+), 13 deletions(-)
Markus Armbruster - June 28, 2011, 11:49 a.m.
Markus Armbruster <armbru@redhat.com> writes:

> It needs to be a qdev property, because it belongs to the drive's
> guest part.  Precedence: commit a0fef654 and 6ced55a5.
>
> Bonus: info qtree now shows the serial number.

Ping?
Kevin Wolf - July 1, 2011, 7:53 a.m.
Am 20.06.2011 11:35, schrieb Markus Armbruster:
> It needs to be a qdev property, because it belongs to the drive's
> guest part.  Precedence: commit a0fef654 and 6ced55a5.
> 
> Bonus: info qtree now shows the serial number.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/s390-virtio-bus.c |    4 +++-
>  hw/s390-virtio-bus.h |    1 +
>  hw/virtio-blk.c      |   29 +++++++++++++++++++----------
>  hw/virtio-blk.h      |    2 ++
>  hw/virtio-pci.c      |    4 +++-
>  hw/virtio-pci.h      |    1 +
>  hw/virtio.h          |    3 ++-
>  7 files changed, 31 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/s390-virtio-bus.c b/hw/s390-virtio-bus.c
> index d4a12f7..2bf4821 100644
> --- a/hw/s390-virtio-bus.c
> +++ b/hw/s390-virtio-bus.c
> @@ -128,7 +128,8 @@ static int s390_virtio_blk_init(VirtIOS390Device *dev)
>  {
>      VirtIODevice *vdev;
>  
> -    vdev = virtio_blk_init((DeviceState *)dev, &dev->block);
> +    vdev = virtio_blk_init((DeviceState *)dev, &dev->block,
> +                           &dev->block_serial);
>      if (!vdev) {
>          return -1;
>      }
> @@ -355,6 +356,7 @@ static VirtIOS390DeviceInfo s390_virtio_blk = {
>      .qdev.size = sizeof(VirtIOS390Device),
>      .qdev.props = (Property[]) {
>          DEFINE_BLOCK_PROPERTIES(VirtIOS390Device, block),
> +        DEFINE_PROP_STRING("serial", VirtIOS390Device, block_serial),
>          DEFINE_PROP_END_OF_LIST(),
>      },
>  };
> diff --git a/hw/s390-virtio-bus.h b/hw/s390-virtio-bus.h
> index 0c412d0..f1bece7 100644
> --- a/hw/s390-virtio-bus.h
> +++ b/hw/s390-virtio-bus.h
> @@ -42,6 +42,7 @@ typedef struct VirtIOS390Device {
>      uint8_t feat_len;
>      VirtIODevice *vdev;
>      BlockConf block;
> +    char *block_serial;
>      NICConf nic;
>      uint32_t host_features;
>      virtio_serial_conf serial;
> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
> index 91e0394..6471ac8 100644
> --- a/hw/virtio-blk.c
> +++ b/hw/virtio-blk.c
> @@ -28,8 +28,8 @@ typedef struct VirtIOBlock
>      void *rq;
>      QEMUBH *bh;
>      BlockConf *conf;
> +    char *serial;
>      unsigned short sector_mask;
> -    char sn[BLOCK_SERIAL_STRLEN];
>      DeviceState *qdev;
>  } VirtIOBlock;
>  
> @@ -362,8 +362,13 @@ static void virtio_blk_handle_request(VirtIOBlockReq *req,
>      } else if (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)));
> +        /*
> +         * NB: per existing s/n string convention the string is
> +         * terminated by '\0' only when shorter than buffer.
> +         */
> +        strncpy(req->elem.in_sg[0].iov_base,
> +                s->serial ? s->serial : "",
> +                MIN(req->elem.in_sg[0].iov_len, VIRTIO_BLK_ID_BYTES));

Not sure what you're trying to do with VIRTIO_BLK_ID_BYTES here.

s->string either is dinfo->serial, in which case it happens to be the
same as BLOCK_SERIAL_STRLEN and as such makes some sense. Or it may be a
qdev property, in which case the string just has the length it has. Or
it's the empty string. So I think in two of three cases you're
potentially reading beyond the end of the buffer.

Kevin
Markus Armbruster - July 4, 2011, 11:29 a.m.
Kevin Wolf <kwolf@redhat.com> writes:

> Am 20.06.2011 11:35, schrieb Markus Armbruster:
>> It needs to be a qdev property, because it belongs to the drive's
>> guest part.  Precedence: commit a0fef654 and 6ced55a5.
>> 
>> Bonus: info qtree now shows the serial number.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  hw/s390-virtio-bus.c |    4 +++-
>>  hw/s390-virtio-bus.h |    1 +
>>  hw/virtio-blk.c      |   29 +++++++++++++++++++----------
>>  hw/virtio-blk.h      |    2 ++
>>  hw/virtio-pci.c      |    4 +++-
>>  hw/virtio-pci.h      |    1 +
>>  hw/virtio.h          |    3 ++-
>>  7 files changed, 31 insertions(+), 13 deletions(-)
>> 
>> diff --git a/hw/s390-virtio-bus.c b/hw/s390-virtio-bus.c
>> index d4a12f7..2bf4821 100644
>> --- a/hw/s390-virtio-bus.c
>> +++ b/hw/s390-virtio-bus.c
>> @@ -128,7 +128,8 @@ static int s390_virtio_blk_init(VirtIOS390Device *dev)
>>  {
>>      VirtIODevice *vdev;
>>  
>> -    vdev = virtio_blk_init((DeviceState *)dev, &dev->block);
>> +    vdev = virtio_blk_init((DeviceState *)dev, &dev->block,
>> +                           &dev->block_serial);
>>      if (!vdev) {
>>          return -1;
>>      }
>> @@ -355,6 +356,7 @@ static VirtIOS390DeviceInfo s390_virtio_blk = {
>>      .qdev.size = sizeof(VirtIOS390Device),
>>      .qdev.props = (Property[]) {
>>          DEFINE_BLOCK_PROPERTIES(VirtIOS390Device, block),
>> +        DEFINE_PROP_STRING("serial", VirtIOS390Device, block_serial),
>>          DEFINE_PROP_END_OF_LIST(),
>>      },
>>  };
>> diff --git a/hw/s390-virtio-bus.h b/hw/s390-virtio-bus.h
>> index 0c412d0..f1bece7 100644
>> --- a/hw/s390-virtio-bus.h
>> +++ b/hw/s390-virtio-bus.h
>> @@ -42,6 +42,7 @@ typedef struct VirtIOS390Device {
>>      uint8_t feat_len;
>>      VirtIODevice *vdev;
>>      BlockConf block;
>> +    char *block_serial;
>>      NICConf nic;
>>      uint32_t host_features;
>>      virtio_serial_conf serial;
>> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
>> index 91e0394..6471ac8 100644
>> --- a/hw/virtio-blk.c
>> +++ b/hw/virtio-blk.c
>> @@ -28,8 +28,8 @@ typedef struct VirtIOBlock
>>      void *rq;
>>      QEMUBH *bh;
>>      BlockConf *conf;
>> +    char *serial;
>>      unsigned short sector_mask;
>> -    char sn[BLOCK_SERIAL_STRLEN];
>>      DeviceState *qdev;
>>  } VirtIOBlock;
>>  
>> @@ -362,8 +362,13 @@ static void virtio_blk_handle_request(VirtIOBlockReq *req,
>>      } else if (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)));
>> +        /*
>> +         * NB: per existing s/n string convention the string is
>> +         * terminated by '\0' only when shorter than buffer.
>> +         */
>> +        strncpy(req->elem.in_sg[0].iov_base,
>> +                s->serial ? s->serial : "",
>> +                MIN(req->elem.in_sg[0].iov_len, VIRTIO_BLK_ID_BYTES));
>
> Not sure what you're trying to do with VIRTIO_BLK_ID_BYTES here.
>
> s->string either is dinfo->serial, in which case it happens to be the

You mean s->serial, don't you?

> same as BLOCK_SERIAL_STRLEN and as such makes some sense. Or it may be a
> qdev property, in which case the string just has the length it has. Or
> it's the empty string. So I think in two of three cases you're
> potentially reading beyond the end of the buffer.

I can't see that.

strncpy(dst, src, n) reads up to n characters or the terminating zero,
whatever comes first.

strncpy()'s second argument is zero-terminated.  "" trivially is.
s->serial is a malloc'ed, zero-terminated string.  It's set either by
qdev_prop_string's parse_string(), or two patch hunks down (some snipped
text restored for your convenience).  In both cases, the value is
freshly strdup'ed.  Therefore, strncpy() won't read beyond the buffer.

strncpy(dst, src, n) writes exactly n characters.

Its third argument is the minimum of the buffer size and
VIRTIO_BLK_ID_BYTES.

The old code works the same, only it uses sizeof(s->sn) instead of
VIRTIO_BLK_ID_BYTES, with sn defined as "char sn[BLOCK_SERIAL_STRLEN]".

VIRTIO_BLK_ID_BYTES is really part of the virtio protocol.  Its value
indeed happens to be the same as BLOCK_SERIAL_STRLEN, but I chose not to
rely on that.  Matches IDE and SCSI, only they use literals instead of
defines.

The kernel has it in include/linux/virtio_blk.h.  Our partial copy of
that header is hw/virtio-blk.h.  I added VIRTIO_BLK_ID_BYTES to that
copy (four patch hunks down).

>> @@ -531,7 +536,8 @@ static void virtio_blk_change_cb(void *opaque, int reason)
>>      }
>>  }
>>  
>> -VirtIODevice *virtio_blk_init(DeviceState *dev, BlockConf *conf)
>> +VirtIODevice *virtio_blk_init(DeviceState *dev, BlockConf *conf,
>> +                              char **serial)
>>  {
>>      VirtIOBlock *s;
>>      int cylinders, heads, secs;
>> @@ -547,6 +553,14 @@ VirtIODevice *virtio_blk_init(DeviceState *dev, BlockConf *conf)
>>          return NULL;
>>      }
>>  
>> +    if (!*serial) {
>> +        /* try to fall back to value set with legacy -drive serial=... */
>> +        dinfo = drive_get_by_blockdev(conf->bs);
>> +        if (*dinfo->serial) {
>> +            *serial = strdup(dinfo->serial);
>> +        }
>> +    }
>> +
>>      s = (VirtIOBlock *)virtio_common_init("virtio-blk", VIRTIO_ID_BLOCK,
>>                                            sizeof(struct virtio_blk_config),
>>                                            sizeof(VirtIOBlock));
>> 
>> @@ -556,16 +570,11 @@ VirtIODevice *virtio_blk_init(DeviceState *dev, BlockConf *conf)
>>      s->vdev.reset = virtio_blk_reset;
>>      s->bs = conf->bs;
>>      s->conf = conf;
>> +    s->serial = *serial;
>>      s->rq = NULL;
>>      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 the string is terminated
>> -     * by '\0' only when less than sizeof (s->sn)
>> -     */
>> -    dinfo = drive_get_by_blockdev(s->bs);
>> -    strncpy(s->sn, dinfo->serial, 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 fff46da..5645d2b 100644
>> --- a/hw/virtio-blk.h
>> +++ b/hw/virtio-blk.h
>> @@ -34,6 +34,8 @@
>>  #define VIRTIO_BLK_F_WCACHE     9       /* write cache enabled */
>>  #define VIRTIO_BLK_F_TOPOLOGY   10      /* Topology information is available */
>>  
>> +#define VIRTIO_BLK_ID_BYTES     20      /* ID string length */
>> +
>>  struct virtio_blk_config
>>  {
>>      uint64_t capacity;
Kevin Wolf - July 4, 2011, 12:09 p.m.
Am 04.07.2011 13:29, schrieb Markus Armbruster:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
>> Am 20.06.2011 11:35, schrieb Markus Armbruster:
>>> It needs to be a qdev property, because it belongs to the drive's
>>> guest part.  Precedence: commit a0fef654 and 6ced55a5.
>>>
>>> Bonus: info qtree now shows the serial number.
>>>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>>  hw/s390-virtio-bus.c |    4 +++-
>>>  hw/s390-virtio-bus.h |    1 +
>>>  hw/virtio-blk.c      |   29 +++++++++++++++++++----------
>>>  hw/virtio-blk.h      |    2 ++
>>>  hw/virtio-pci.c      |    4 +++-
>>>  hw/virtio-pci.h      |    1 +
>>>  hw/virtio.h          |    3 ++-
>>>  7 files changed, 31 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/hw/s390-virtio-bus.c b/hw/s390-virtio-bus.c
>>> index d4a12f7..2bf4821 100644
>>> --- a/hw/s390-virtio-bus.c
>>> +++ b/hw/s390-virtio-bus.c
>>> @@ -128,7 +128,8 @@ static int s390_virtio_blk_init(VirtIOS390Device *dev)
>>>  {
>>>      VirtIODevice *vdev;
>>>  
>>> -    vdev = virtio_blk_init((DeviceState *)dev, &dev->block);
>>> +    vdev = virtio_blk_init((DeviceState *)dev, &dev->block,
>>> +                           &dev->block_serial);
>>>      if (!vdev) {
>>>          return -1;
>>>      }
>>> @@ -355,6 +356,7 @@ static VirtIOS390DeviceInfo s390_virtio_blk = {
>>>      .qdev.size = sizeof(VirtIOS390Device),
>>>      .qdev.props = (Property[]) {
>>>          DEFINE_BLOCK_PROPERTIES(VirtIOS390Device, block),
>>> +        DEFINE_PROP_STRING("serial", VirtIOS390Device, block_serial),
>>>          DEFINE_PROP_END_OF_LIST(),
>>>      },
>>>  };
>>> diff --git a/hw/s390-virtio-bus.h b/hw/s390-virtio-bus.h
>>> index 0c412d0..f1bece7 100644
>>> --- a/hw/s390-virtio-bus.h
>>> +++ b/hw/s390-virtio-bus.h
>>> @@ -42,6 +42,7 @@ typedef struct VirtIOS390Device {
>>>      uint8_t feat_len;
>>>      VirtIODevice *vdev;
>>>      BlockConf block;
>>> +    char *block_serial;
>>>      NICConf nic;
>>>      uint32_t host_features;
>>>      virtio_serial_conf serial;
>>> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
>>> index 91e0394..6471ac8 100644
>>> --- a/hw/virtio-blk.c
>>> +++ b/hw/virtio-blk.c
>>> @@ -28,8 +28,8 @@ typedef struct VirtIOBlock
>>>      void *rq;
>>>      QEMUBH *bh;
>>>      BlockConf *conf;
>>> +    char *serial;
>>>      unsigned short sector_mask;
>>> -    char sn[BLOCK_SERIAL_STRLEN];
>>>      DeviceState *qdev;
>>>  } VirtIOBlock;
>>>  
>>> @@ -362,8 +362,13 @@ static void virtio_blk_handle_request(VirtIOBlockReq *req,
>>>      } else if (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)));
>>> +        /*
>>> +         * NB: per existing s/n string convention the string is
>>> +         * terminated by '\0' only when shorter than buffer.
>>> +         */
>>> +        strncpy(req->elem.in_sg[0].iov_base,
>>> +                s->serial ? s->serial : "",
>>> +                MIN(req->elem.in_sg[0].iov_len, VIRTIO_BLK_ID_BYTES));
>>
>> Not sure what you're trying to do with VIRTIO_BLK_ID_BYTES here.
>>
>> s->string either is dinfo->serial, in which case it happens to be the
> 
> You mean s->serial, don't you?
> 
>> same as BLOCK_SERIAL_STRLEN and as such makes some sense. Or it may be a
>> qdev property, in which case the string just has the length it has. Or
>> it's the empty string. So I think in two of three cases you're
>> potentially reading beyond the end of the buffer.
> 
> I can't see that.

You're right, sorry for the noise.

What confused me is that I didn't expect some limit in the protocol (and
if any, then certainly not 20), so I started making wild guesses what
this might be used for, and reading strncpy as memcpy because it made
more sense with the guesses...

I should just stop reviewing patches early in the morning. :-)

Applied the patch to the block branch.

Kevin

Patch

diff --git a/hw/s390-virtio-bus.c b/hw/s390-virtio-bus.c
index d4a12f7..2bf4821 100644
--- a/hw/s390-virtio-bus.c
+++ b/hw/s390-virtio-bus.c
@@ -128,7 +128,8 @@  static int s390_virtio_blk_init(VirtIOS390Device *dev)
 {
     VirtIODevice *vdev;
 
-    vdev = virtio_blk_init((DeviceState *)dev, &dev->block);
+    vdev = virtio_blk_init((DeviceState *)dev, &dev->block,
+                           &dev->block_serial);
     if (!vdev) {
         return -1;
     }
@@ -355,6 +356,7 @@  static VirtIOS390DeviceInfo s390_virtio_blk = {
     .qdev.size = sizeof(VirtIOS390Device),
     .qdev.props = (Property[]) {
         DEFINE_BLOCK_PROPERTIES(VirtIOS390Device, block),
+        DEFINE_PROP_STRING("serial", VirtIOS390Device, block_serial),
         DEFINE_PROP_END_OF_LIST(),
     },
 };
diff --git a/hw/s390-virtio-bus.h b/hw/s390-virtio-bus.h
index 0c412d0..f1bece7 100644
--- a/hw/s390-virtio-bus.h
+++ b/hw/s390-virtio-bus.h
@@ -42,6 +42,7 @@  typedef struct VirtIOS390Device {
     uint8_t feat_len;
     VirtIODevice *vdev;
     BlockConf block;
+    char *block_serial;
     NICConf nic;
     uint32_t host_features;
     virtio_serial_conf serial;
diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index 91e0394..6471ac8 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -28,8 +28,8 @@  typedef struct VirtIOBlock
     void *rq;
     QEMUBH *bh;
     BlockConf *conf;
+    char *serial;
     unsigned short sector_mask;
-    char sn[BLOCK_SERIAL_STRLEN];
     DeviceState *qdev;
 } VirtIOBlock;
 
@@ -362,8 +362,13 @@  static void virtio_blk_handle_request(VirtIOBlockReq *req,
     } else if (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)));
+        /*
+         * NB: per existing s/n string convention the string is
+         * terminated by '\0' only when shorter than buffer.
+         */
+        strncpy(req->elem.in_sg[0].iov_base,
+                s->serial ? s->serial : "",
+                MIN(req->elem.in_sg[0].iov_len, VIRTIO_BLK_ID_BYTES));
         virtio_blk_req_complete(req, VIRTIO_BLK_S_OK);
     } else if (type & VIRTIO_BLK_T_OUT) {
         qemu_iovec_init_external(&req->qiov, &req->elem.out_sg[1],
@@ -531,7 +536,8 @@  static void virtio_blk_change_cb(void *opaque, int reason)
     }
 }
 
-VirtIODevice *virtio_blk_init(DeviceState *dev, BlockConf *conf)
+VirtIODevice *virtio_blk_init(DeviceState *dev, BlockConf *conf,
+                              char **serial)
 {
     VirtIOBlock *s;
     int cylinders, heads, secs;
@@ -547,6 +553,14 @@  VirtIODevice *virtio_blk_init(DeviceState *dev, BlockConf *conf)
         return NULL;
     }
 
+    if (!*serial) {
+        /* try to fall back to value set with legacy -drive serial=... */
+        dinfo = drive_get_by_blockdev(conf->bs);
+        if (*dinfo->serial) {
+            *serial = strdup(dinfo->serial);
+        }
+    }
+
     s = (VirtIOBlock *)virtio_common_init("virtio-blk", VIRTIO_ID_BLOCK,
                                           sizeof(struct virtio_blk_config),
                                           sizeof(VirtIOBlock));
@@ -556,16 +570,11 @@  VirtIODevice *virtio_blk_init(DeviceState *dev, BlockConf *conf)
     s->vdev.reset = virtio_blk_reset;
     s->bs = conf->bs;
     s->conf = conf;
+    s->serial = *serial;
     s->rq = NULL;
     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 the string is terminated
-     * by '\0' only when less than sizeof (s->sn)
-     */
-    dinfo = drive_get_by_blockdev(s->bs);
-    strncpy(s->sn, dinfo->serial, 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 fff46da..5645d2b 100644
--- a/hw/virtio-blk.h
+++ b/hw/virtio-blk.h
@@ -34,6 +34,8 @@ 
 #define VIRTIO_BLK_F_WCACHE     9       /* write cache enabled */
 #define VIRTIO_BLK_F_TOPOLOGY   10      /* Topology information is available */
 
+#define VIRTIO_BLK_ID_BYTES     20      /* ID string length */
+
 struct virtio_blk_config
 {
     uint64_t capacity;
diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index c018351..a8c236e 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -710,7 +710,8 @@  static int virtio_blk_init_pci(PCIDevice *pci_dev)
         proxy->class_code != PCI_CLASS_STORAGE_OTHER)
         proxy->class_code = PCI_CLASS_STORAGE_SCSI;
 
-    vdev = virtio_blk_init(&pci_dev->qdev, &proxy->block);
+    vdev = virtio_blk_init(&pci_dev->qdev, &proxy->block,
+                           &proxy->block_serial);
     if (!vdev) {
         return -1;
     }
@@ -825,6 +826,7 @@  static PCIDeviceInfo virtio_info[] = {
         .qdev.props = (Property[]) {
             DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, 0),
             DEFINE_BLOCK_PROPERTIES(VirtIOPCIProxy, block),
+            DEFINE_PROP_STRING("serial", VirtIOPCIProxy, block_serial),
             DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags,
                             VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true),
             DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2),
diff --git a/hw/virtio-pci.h b/hw/virtio-pci.h
index a4b5fd3..bc29864 100644
--- a/hw/virtio-pci.h
+++ b/hw/virtio-pci.h
@@ -26,6 +26,7 @@  typedef struct {
     uint32_t class_code;
     uint32_t nvectors;
     BlockConf block;
+    char *block_serial;
     NICConf nic;
     uint32_t host_features;
 #ifdef CONFIG_LINUX
diff --git a/hw/virtio.h b/hw/virtio.h
index bc72289..583600d 100644
--- a/hw/virtio.h
+++ b/hw/virtio.h
@@ -192,7 +192,8 @@  void virtio_bind_device(VirtIODevice *vdev, const VirtIOBindings *binding,
                         void *opaque);
 
 /* Base devices.  */
-VirtIODevice *virtio_blk_init(DeviceState *dev, BlockConf *conf);
+VirtIODevice *virtio_blk_init(DeviceState *dev, BlockConf *conf,
+                              char **serial);
 struct virtio_net_conf;
 VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf,
                               struct virtio_net_conf *net);