[Qemu-devel] [PATCH v2] virtio-blk: set correct config size for the host driver

[Qemu-devel] [PATCH v2] virtio-blk: set correct config size for the host driver

[Changpeng Liu]

Commit caa1ee43 "vhost-user-blk: add discard/write zeroes features support"
introduced extra fields to existing struct virtio_blk_config, when
migration was executed from older QEMU version to current head, it
will break the migration.  While here, set the correct config size
when initializing the host driver, for now, discard/write zeroes
are not supported by virtio-blk host driver, so set the config
size as before, users can change config size when adding the new
feature bits support.

Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
---
 hw/block/virtio-blk.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 9a87b3b..846b7b9 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -28,6 +28,9 @@
 #include "hw/virtio/virtio-bus.h"
 #include "hw/virtio/virtio-access.h"
 
+#define VIRTIO_BLK_CFG_SIZE (offsetof(struct virtio_blk_config, num_queues) + \
+                             sizeof_field(struct virtio_blk_config, num_queues))
+
 static void virtio_blk_init_request(VirtIOBlock *s, VirtQueue *vq,
                                     VirtIOBlockReq *req)
 {
@@ -761,7 +764,7 @@ static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config)
     blkcfg.alignment_offset = 0;
     blkcfg.wce = blk_enable_write_cache(s->blk);
     virtio_stw_p(vdev, &blkcfg.num_queues, s->conf.num_queues);
-    memcpy(config, &blkcfg, sizeof(struct virtio_blk_config));
+    memcpy(config, &blkcfg, VIRTIO_BLK_CFG_SIZE);
 }
 
 static void virtio_blk_set_config(VirtIODevice *vdev, const uint8_t *config)
@@ -769,7 +772,7 @@ static void virtio_blk_set_config(VirtIODevice *vdev, const uint8_t *config)
     VirtIOBlock *s = VIRTIO_BLK(vdev);
     struct virtio_blk_config blkcfg;
 
-    memcpy(&blkcfg, config, sizeof(blkcfg));
+    memcpy(&blkcfg, config, VIRTIO_BLK_CFG_SIZE);
 
     aio_context_acquire(blk_get_aio_context(s->blk));
     blk_set_enable_write_cache(s->blk, blkcfg.wce != 0);
@@ -952,8 +955,7 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
         return;
     }
 
-    virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK,
-                sizeof(struct virtio_blk_config));
+    virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK, VIRTIO_BLK_CFG_SIZE);
 
     s->blk = conf->conf.blk;
     s->rq = NULL;
-- 
1.9.3

Re: [Qemu-devel] [PATCH v2] virtio-blk: set correct config size for the host driver

[Greg Kurz]

On Tue, 12 Feb 2019 14:01:44 +0800
Changpeng Liu <changpeng.liu@intel.com> wrote:

> Commit caa1ee43 "vhost-user-blk: add discard/write zeroes features support"
> introduced extra fields to existing struct virtio_blk_config, when
> migration was executed from older QEMU version to current head, it

A hint about the breakage is often a good practice, as it helps
people to find the fix if they hit the issue.

> will break the migration.  While here, set the correct config size

"While here" usually means that the corresponding change isn't strictly
needed. I don't believe this is the case here since fixing the config
size is precisely the goal of this patch.

> when initializing the host driver, for now, discard/write zeroes
> are not supported by virtio-blk host driver, so set the config

As mentioned by Michael, there's no such thing as a "host driver".
Just say virtio-blk.

> size as before, users can change config size when adding the new
> feature bits support.
> 

What about ?

Commit caa1ee43 "vhost-user-blk: add discard/write zeroes features
support" added fields to struct virtio_blk_config. This changes
the size of the config space and breaks migration from QEMU 3.1
and older:

qemu-system-ppc64: get_pci_config_device: Bad config data: i=0x10 read: 41 device: 1 cmask: ff wmask: 80 w1cmask:0
qemu-system-ppc64: Failed to load PCIDevice:config
qemu-system-ppc64: Failed to load virtio-blk:virtio
qemu-system-ppc64: error while loading state for instance 0x0 of device 'pci@800000020000000:01.0/virtio-blk'
qemu-system-ppc64: load of migration failed: Invalid argument

Since virtio-blk doesn't support the "discard" and "write zeroes"
features, it shouldn't even expose the associated fields in the
config space actually. Just include all fields up to num_queues to
match QEMU 3.1 and older.

> Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
> ---

LGTM

Reviewed-by: Greg Kurz <groug@kaod.org>
Tested-by: Greg Kurz <groug@kaod.org>

>  hw/block/virtio-blk.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 9a87b3b..846b7b9 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -28,6 +28,9 @@
>  #include "hw/virtio/virtio-bus.h"
>  #include "hw/virtio/virtio-access.h"
>  
> +#define VIRTIO_BLK_CFG_SIZE (offsetof(struct virtio_blk_config, num_queues) + \
> +                             sizeof_field(struct virtio_blk_config, num_queues))
> +
>  static void virtio_blk_init_request(VirtIOBlock *s, VirtQueue *vq,
>                                      VirtIOBlockReq *req)
>  {
> @@ -761,7 +764,7 @@ static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config)
>      blkcfg.alignment_offset = 0;
>      blkcfg.wce = blk_enable_write_cache(s->blk);
>      virtio_stw_p(vdev, &blkcfg.num_queues, s->conf.num_queues);
> -    memcpy(config, &blkcfg, sizeof(struct virtio_blk_config));
> +    memcpy(config, &blkcfg, VIRTIO_BLK_CFG_SIZE);
>  }
>  
>  static void virtio_blk_set_config(VirtIODevice *vdev, const uint8_t *config)
> @@ -769,7 +772,7 @@ static void virtio_blk_set_config(VirtIODevice *vdev, const uint8_t *config)
>      VirtIOBlock *s = VIRTIO_BLK(vdev);
>      struct virtio_blk_config blkcfg;
>  
> -    memcpy(&blkcfg, config, sizeof(blkcfg));
> +    memcpy(&blkcfg, config, VIRTIO_BLK_CFG_SIZE);
>  
>      aio_context_acquire(blk_get_aio_context(s->blk));
>      blk_set_enable_write_cache(s->blk, blkcfg.wce != 0);
> @@ -952,8 +955,7 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
>          return;
>      }
>  
> -    virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK,
> -                sizeof(struct virtio_blk_config));
> +    virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK, VIRTIO_BLK_CFG_SIZE);
>  
>      s->blk = conf->conf.blk;
>      s->rq = NULL;

Re: [Qemu-devel] [PATCH v2] virtio-blk: set correct config size for the host driver

[Michael S. Tsirkin]

On Tue, Feb 12, 2019 at 02:01:44PM +0800, Changpeng Liu wrote:
> Commit caa1ee43 "vhost-user-blk: add discard/write zeroes features support"
> introduced extra fields to existing struct virtio_blk_config, when
> migration was executed from older QEMU version to current head, it
> will break the migration.  While here, set the correct config size
> when initializing the host driver, for now, discard/write zeroes
> are not supported by virtio-blk host driver, so set the config
> size as before, users can change config size when adding the new
> feature bits support.
> 
> Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>


Pls rewrite commit log as suggested in this thread
and repost.

> ---
>  hw/block/virtio-blk.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 9a87b3b..846b7b9 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -28,6 +28,9 @@
>  #include "hw/virtio/virtio-bus.h"
>  #include "hw/virtio/virtio-access.h"
>  
> +#define VIRTIO_BLK_CFG_SIZE (offsetof(struct virtio_blk_config, num_queues) + \
> +                             sizeof_field(struct virtio_blk_config, num_queues))
> +

I would just do offsetof(max_discard_sectors)
with a comment "we don't support discard yet, hide associated config
fields".

>  static void virtio_blk_init_request(VirtIOBlock *s, VirtQueue *vq,
>                                      VirtIOBlockReq *req)
>  {
> @@ -761,7 +764,7 @@ static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config)
>      blkcfg.alignment_offset = 0;
>      blkcfg.wce = blk_enable_write_cache(s->blk);
>      virtio_stw_p(vdev, &blkcfg.num_queues, s->conf.num_queues);
> -    memcpy(config, &blkcfg, sizeof(struct virtio_blk_config));
> +    memcpy(config, &blkcfg, VIRTIO_BLK_CFG_SIZE);


Let's add QEMU_BUILD_BUG_ON(VIRTIO_BLK_CFG_SIZE <= sizeof(struct
virtio_blk_config)) just for documentation purposes.

>  }
>  
>  static void virtio_blk_set_config(VirtIODevice *vdev, const uint8_t *config)
> @@ -769,7 +772,7 @@ static void virtio_blk_set_config(VirtIODevice *vdev, const uint8_t *config)
>      VirtIOBlock *s = VIRTIO_BLK(vdev);
>      struct virtio_blk_config blkcfg;
>  
> -    memcpy(&blkcfg, config, sizeof(blkcfg));
> +    memcpy(&blkcfg, config, VIRTIO_BLK_CFG_SIZE);


Here too, QEMU_BUILD_BUG_ON(VIRTIO_BLK_CFG_SIZE <= sizeof(blkcfg))

>  
>      aio_context_acquire(blk_get_aio_context(s->blk));
>      blk_set_enable_write_cache(s->blk, blkcfg.wce != 0);
> @@ -952,8 +955,7 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
>          return;
>      }
>  
> -    virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK,
> -                sizeof(struct virtio_blk_config));
> +    virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK, VIRTIO_BLK_CFG_SIZE);
>  
>      s->blk = conf->conf.blk;
>      s->rq = NULL;
> -- 
> 1.9.3

Re: [Qemu-devel] [PATCH v2] virtio-blk: set correct config size for the host driver

[Liu, Changpeng]

> -----Original Message-----
> From: Michael S. Tsirkin [mailto:mst@redhat.com]
> Sent: Tuesday, February 12, 2019 9:49 PM
> To: Liu, Changpeng <changpeng.liu@intel.com>
> Cc: qemu-devel@nongnu.org; stefanha@redhat.com; sgarzare@redhat.com;
> dgilbert@redhat.com; ldoktor@redhat.com
> Subject: Re: [PATCH v2] virtio-blk: set correct config size for the host driver
> 
> On Tue, Feb 12, 2019 at 02:01:44PM +0800, Changpeng Liu wrote:
> > Commit caa1ee43 "vhost-user-blk: add discard/write zeroes features support"
> > introduced extra fields to existing struct virtio_blk_config, when
> > migration was executed from older QEMU version to current head, it
> > will break the migration.  While here, set the correct config size
> > when initializing the host driver, for now, discard/write zeroes
> > are not supported by virtio-blk host driver, so set the config
> > size as before, users can change config size when adding the new
> > feature bits support.
> >
> > Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
> 
> 
> Pls rewrite commit log as suggested in this thread
> and repost.
Thanks, repost a v3 patch to address the commit message and your following comments.
> 
> > ---
> >  hw/block/virtio-blk.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> > index 9a87b3b..846b7b9 100644
> > --- a/hw/block/virtio-blk.c
> > +++ b/hw/block/virtio-blk.c
> > @@ -28,6 +28,9 @@
> >  #include "hw/virtio/virtio-bus.h"
> >  #include "hw/virtio/virtio-access.h"
> >
> > +#define VIRTIO_BLK_CFG_SIZE (offsetof(struct virtio_blk_config, num_queues)
> + \
> > +                             sizeof_field(struct virtio_blk_config, num_queues))
> > +
> 
> I would just do offsetof(max_discard_sectors)
> with a comment "we don't support discard yet, hide associated config
> fields".
> 
> >  static void virtio_blk_init_request(VirtIOBlock *s, VirtQueue *vq,
> >                                      VirtIOBlockReq *req)
> >  {
> > @@ -761,7 +764,7 @@ static void virtio_blk_update_config(VirtIODevice *vdev,
> uint8_t *config)
> >      blkcfg.alignment_offset = 0;
> >      blkcfg.wce = blk_enable_write_cache(s->blk);
> >      virtio_stw_p(vdev, &blkcfg.num_queues, s->conf.num_queues);
> > -    memcpy(config, &blkcfg, sizeof(struct virtio_blk_config));
> > +    memcpy(config, &blkcfg, VIRTIO_BLK_CFG_SIZE);
> 
> 
> Let's add QEMU_BUILD_BUG_ON(VIRTIO_BLK_CFG_SIZE <= sizeof(struct
> virtio_blk_config)) just for documentation purposes.
> 
> >  }
> >
> >  static void virtio_blk_set_config(VirtIODevice *vdev, const uint8_t *config)
> > @@ -769,7 +772,7 @@ static void virtio_blk_set_config(VirtIODevice *vdev,
> const uint8_t *config)
> >      VirtIOBlock *s = VIRTIO_BLK(vdev);
> >      struct virtio_blk_config blkcfg;
> >
> > -    memcpy(&blkcfg, config, sizeof(blkcfg));
> > +    memcpy(&blkcfg, config, VIRTIO_BLK_CFG_SIZE);
> 
> 
> Here too, QEMU_BUILD_BUG_ON(VIRTIO_BLK_CFG_SIZE <= sizeof(blkcfg))
> 
> >
> >      aio_context_acquire(blk_get_aio_context(s->blk));
> >      blk_set_enable_write_cache(s->blk, blkcfg.wce != 0);
> > @@ -952,8 +955,7 @@ static void virtio_blk_device_realize(DeviceState *dev,
> Error **errp)
> >          return;
> >      }
> >
> > -    virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK,
> > -                sizeof(struct virtio_blk_config));
> > +    virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK, VIRTIO_BLK_CFG_SIZE);
> >
> >      s->blk = conf->conf.blk;
> >      s->rq = NULL;
> > --
> > 1.9.3