diff mbox series

virtio-blk: set correct config size for the host driver

Message ID 1549945516-25643-1-git-send-email-changpeng.liu@intel.com
State New
Headers show
Series virtio-blk: set correct config size for the host driver | expand

Commit Message

Liu, Changpeng Feb. 12, 2019, 4:25 a.m. UTC 
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          | 15 +++++++++++----
 include/hw/virtio/virtio-blk.h |  1 +
 2 files changed, 12 insertions(+), 4 deletions(-)

Comments

Michael S. Tsirkin Feb. 12, 2019, 4:31 a.m. UTC | #1 
On Tue, Feb 12, 2019 at 12:25:16PM +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>
> ---
>  hw/block/virtio-blk.c          | 15 +++++++++++----
>  include/hw/virtio/virtio-blk.h |  1 +
>  2 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 9a87b3b..fac5d33 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -761,7 +761,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, s->config_size);
>  }
>  
>  static void virtio_blk_set_config(VirtIODevice *vdev, const uint8_t *config)
> @@ -769,7 +769,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, s->config_size);
>  
>      aio_context_acquire(blk_get_aio_context(s->blk));
>      blk_set_enable_write_cache(s->blk, blkcfg.wce != 0);
> @@ -847,6 +847,13 @@ static void virtio_blk_set_status(VirtIODevice *vdev, uint8_t status)
>      }
>  }
>  
> +static void virtio_blk_set_config_size(VirtIOBlock *s)
> +{
> +    /* VIRTIO_BLK_F_MQ is supported by host driver */

It can be disabled though.

It just so happens that only the addition of max_discard_seg
crosses the next power of 2 boundary.


> +    s->config_size = offsetof(struct virtio_blk_config, num_queues) +
> +                     sizeof_field(struct virtio_blk_config, num_queues);
> +}
> +
>  static void virtio_blk_save_device(VirtIODevice *vdev, QEMUFile *f)
>  {
>      VirtIOBlock *s = VIRTIO_BLK(vdev);
> @@ -952,8 +959,8 @@ 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_blk_set_config_size(s);
> +    virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK, s->config_size);
>  
>      s->blk = conf->conf.blk;
>      s->rq = NULL;
> diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
> index 5117431..9181a93 100644
> --- a/include/hw/virtio/virtio-blk.h
> +++ b/include/hw/virtio/virtio-blk.h
> @@ -51,6 +51,7 @@ typedef struct VirtIOBlock {
>      void *rq;
>      QEMUBH *bh;
>      VirtIOBlkConf conf;
> +    size_t config_size;
>      unsigned short sector_mask;
>      bool original_wce;
>      VMChangeStateEntry *change;

Well assuming you are looking for a minimal change,
go further and drop config_size completely,
replace with a macro.


> -- 
> 1.9.3
Liu, Changpeng Feb. 12, 2019, 5:24 a.m. UTC | #2 
> -----Original Message-----
> From: Michael S. Tsirkin [mailto:mst@redhat.com]
> Sent: Tuesday, February 12, 2019 12:31 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] virtio-blk: set correct config size for the host driver
> 
> On Tue, Feb 12, 2019 at 12:25:16PM +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>
> > ---
> >  hw/block/virtio-blk.c          | 15 +++++++++++----
> >  include/hw/virtio/virtio-blk.h |  1 +
> >  2 files changed, 12 insertions(+), 4 deletions(-)
> >
> > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> > index 9a87b3b..fac5d33 100644
> > --- a/hw/block/virtio-blk.c
> > +++ b/hw/block/virtio-blk.c
> > @@ -761,7 +761,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, s->config_size);
> >  }
> >
> >  static void virtio_blk_set_config(VirtIODevice *vdev, const uint8_t *config)
> > @@ -769,7 +769,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, s->config_size);
> >
> >      aio_context_acquire(blk_get_aio_context(s->blk));
> >      blk_set_enable_write_cache(s->blk, blkcfg.wce != 0);
> > @@ -847,6 +847,13 @@ static void virtio_blk_set_status(VirtIODevice *vdev,
> uint8_t status)
> >      }
> >  }
> >
> > +static void virtio_blk_set_config_size(VirtIOBlock *s)
> > +{
> > +    /* VIRTIO_BLK_F_MQ is supported by host driver */
> 
> It can be disabled though.
It means the host driver can support this feature, users can use it or not.
So the config_size is same with previous old version.
> 
> It just so happens that only the addition of max_discard_seg
> crosses the next power of 2 boundary.
> 
> 
> > +    s->config_size = offsetof(struct virtio_blk_config, num_queues) +
> > +                     sizeof_field(struct virtio_blk_config, num_queues);
> > +}
> > +
> >  static void virtio_blk_save_device(VirtIODevice *vdev, QEMUFile *f)
> >  {
> >      VirtIOBlock *s = VIRTIO_BLK(vdev);
> > @@ -952,8 +959,8 @@ 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_blk_set_config_size(s);
> > +    virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK, s->config_size);
> >
> >      s->blk = conf->conf.blk;
> >      s->rq = NULL;
> > diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
> > index 5117431..9181a93 100644
> > --- a/include/hw/virtio/virtio-blk.h
> > +++ b/include/hw/virtio/virtio-blk.h
> > @@ -51,6 +51,7 @@ typedef struct VirtIOBlock {
> >      void *rq;
> >      QEMUBH *bh;
> >      VirtIOBlkConf conf;
> > +    size_t config_size;
> >      unsigned short sector_mask;
> >      bool original_wce;
> >      VMChangeStateEntry *change;
> 
> Well assuming you are looking for a minimal change,
> go further and drop config_size completely,
> replace with a macro.
OK.
> 
> 
> > --
> > 1.9.3
Michael S. Tsirkin Feb. 12, 2019, 5:41 a.m. UTC | #3 
On Tue, Feb 12, 2019 at 05:24:25AM +0000, Liu, Changpeng wrote:
> 
> 
> > -----Original Message-----
> > From: Michael S. Tsirkin [mailto:mst@redhat.com]
> > Sent: Tuesday, February 12, 2019 12:31 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] virtio-blk: set correct config size for the host driver
> > 
> > On Tue, Feb 12, 2019 at 12:25:16PM +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>
> > > ---
> > >  hw/block/virtio-blk.c          | 15 +++++++++++----
> > >  include/hw/virtio/virtio-blk.h |  1 +
> > >  2 files changed, 12 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> > > index 9a87b3b..fac5d33 100644
> > > --- a/hw/block/virtio-blk.c
> > > +++ b/hw/block/virtio-blk.c
> > > @@ -761,7 +761,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, s->config_size);
> > >  }
> > >
> > >  static void virtio_blk_set_config(VirtIODevice *vdev, const uint8_t *config)
> > > @@ -769,7 +769,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, s->config_size);
> > >
> > >      aio_context_acquire(blk_get_aio_context(s->blk));
> > >      blk_set_enable_write_cache(s->blk, blkcfg.wce != 0);
> > > @@ -847,6 +847,13 @@ static void virtio_blk_set_status(VirtIODevice *vdev,
> > uint8_t status)
> > >      }
> > >  }
> > >
> > > +static void virtio_blk_set_config_size(VirtIOBlock *s)
> > > +{
> > > +    /* VIRTIO_BLK_F_MQ is supported by host driver */
> > 
> > It can be disabled though.
> It means the host driver can support this feature, users can use it or not.
> So the config_size is same with previous old version.

But MQ didn't exist since creation it was only added in 2016.  For
virtio pci it all just happens to be within same power of 2: between 32
and 64 bytes. It's probably wrong for e.g. s390, and maybe
the difference is harmless.

Besides "host driver" makes no sense.

So something like "include all fields up to num_queues to match
QEMU 4.0 and older".

> > 
> > It just so happens that only the addition of max_discard_seg
> > crosses the next power of 2 boundary.
> > 
> > 
> > > +    s->config_size = offsetof(struct virtio_blk_config, num_queues) +
> > > +                     sizeof_field(struct virtio_blk_config, num_queues);
> > > +}
> > > +
> > >  static void virtio_blk_save_device(VirtIODevice *vdev, QEMUFile *f)
> > >  {
> > >      VirtIOBlock *s = VIRTIO_BLK(vdev);
> > > @@ -952,8 +959,8 @@ 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_blk_set_config_size(s);
> > > +    virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK, s->config_size);
> > >
> > >      s->blk = conf->conf.blk;
> > >      s->rq = NULL;
> > > diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
> > > index 5117431..9181a93 100644
> > > --- a/include/hw/virtio/virtio-blk.h
> > > +++ b/include/hw/virtio/virtio-blk.h
> > > @@ -51,6 +51,7 @@ typedef struct VirtIOBlock {
> > >      void *rq;
> > >      QEMUBH *bh;
> > >      VirtIOBlkConf conf;
> > > +    size_t config_size;
> > >      unsigned short sector_mask;
> > >      bool original_wce;
> > >      VMChangeStateEntry *change;
> > 
> > Well assuming you are looking for a minimal change,
> > go further and drop config_size completely,
> > replace with a macro.
> OK.
> > 
> > 
> > > --
> > > 1.9.3
no-reply@patchew.org Feb. 13, 2019, 3:34 p.m. UTC | #4 
Patchew URL: https://patchew.org/QEMU/1549945516-25643-1-git-send-email-changpeng.liu@intel.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 1549945516-25643-1-git-send-email-changpeng.liu@intel.com
Subject: [Qemu-devel] [PATCH] virtio-blk: set correct config size for the host driver

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
error: Ref refs/heads/master is at 0b5e750bea635b167eb03d86c3d9a09bbd43bc06 but expected e47f81b617684c4546af286d307b69014a83538a
From https://github.com/patchew-project/qemu
 ! e47f81b..0b5e750  master     -> master  (unable to update local ref)
 * [new tag]         patchew/1549945516-25643-1-git-send-email-changpeng.liu@intel.com -> patchew/1549945516-25643-1-git-send-email-changpeng.liu@intel.com
 - [tag update]      patchew/20190207102445.71998-1-vsementsov@virtuozzo.com -> patchew/20190207102445.71998-1-vsementsov@virtuozzo.com
 - [tag update]      patchew/20190207160617.1142-1-marcandre.lureau@redhat.com -> patchew/20190207160617.1142-1-marcandre.lureau@redhat.com
 - [tag update]      patchew/20190210104537.1488-1-yuval.shaia@oracle.com -> patchew/20190210104537.1488-1-yuval.shaia@oracle.com
 * [new tag]         patchew/20190212025241.5330-1-stefanha@redhat.com -> patchew/20190212025241.5330-1-stefanha@redhat.com
 * [new tag]         patchew/20190212105201.13795-1-peter.maydell@linaro.org -> patchew/20190212105201.13795-1-peter.maydell@linaro.org
 * [new tag]         patchew/20190212193855.13223-1-ccarrara@redhat.com -> patchew/20190212193855.13223-1-ccarrara@redhat.com
error: Could not fetch 3c8cf5a9c21ff8782164d1def7f44bd888713384
Traceback (most recent call last):
  File "patchew-tester/src/patchew-cli", line 521, in test_one
    git_clone_repo(clone, r["repo"], r["head"], logf, True)
  File "patchew-tester/src/patchew-cli", line 48, in git_clone_repo
    stdout=logf, stderr=logf)
  File "/usr/lib64/python3.4/subprocess.py", line 558, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['git', 'remote', 'add', '-f', '--mirror=fetch', '3c8cf5a9c21ff8782164d1def7f44bd888713384', 'https://github.com/patchew-project/qemu']' returned non-zero exit status 1



The full log is available at
http://patchew.org/logs/1549945516-25643-1-git-send-email-changpeng.liu@intel.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
diff mbox series

Patch

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 9a87b3b..fac5d33 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -761,7 +761,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, s->config_size);
 }
 
 static void virtio_blk_set_config(VirtIODevice *vdev, const uint8_t *config)
@@ -769,7 +769,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, s->config_size);
 
     aio_context_acquire(blk_get_aio_context(s->blk));
     blk_set_enable_write_cache(s->blk, blkcfg.wce != 0);
@@ -847,6 +847,13 @@  static void virtio_blk_set_status(VirtIODevice *vdev, uint8_t status)
     }
 }
 
+static void virtio_blk_set_config_size(VirtIOBlock *s)
+{
+    /* VIRTIO_BLK_F_MQ is supported by host driver */
+    s->config_size = offsetof(struct virtio_blk_config, num_queues) +
+                     sizeof_field(struct virtio_blk_config, num_queues);
+}
+
 static void virtio_blk_save_device(VirtIODevice *vdev, QEMUFile *f)
 {
     VirtIOBlock *s = VIRTIO_BLK(vdev);
@@ -952,8 +959,8 @@  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_blk_set_config_size(s);
+    virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK, s->config_size);
 
     s->blk = conf->conf.blk;
     s->rq = NULL;
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index 5117431..9181a93 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -51,6 +51,7 @@  typedef struct VirtIOBlock {
     void *rq;
     QEMUBH *bh;
     VirtIOBlkConf conf;
+    size_t config_size;
     unsigned short sector_mask;
     bool original_wce;
     VMChangeStateEntry *change;