| Message ID | 1549951304-26100-1-git-send-email-changpeng.liu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] virtio-blk: set correct config size for the host driver | expand |
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;
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
> -----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
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;
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(-)