On Wed, Feb 13, 2019 at 01:17:03PM +0100, Stefano Garzarella wrote:
> On Wed, Feb 13, 2019 at 09:32:27AM +0100, Stefano Garzarella wrote:
> > On Wed, Feb 13, 2019 at 04:01:43PM +0800, Stefan Hajnoczi wrote:
> > > On Wed, Feb 13, 2019 at 09:48:57AM +0800, Changpeng Liu wrote:
> > > > 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....@intel.com>
> > > > ---
> > > > hw/block/virtio-blk.c | 13 +++++++++----
> > > > 1 file changed, 9 insertions(+), 4 deletions(-)
> > >
> > > Stefano: Please rebase your DISCARD/WRITE_ZEROES series onto this and
> > > check that the config space is correctly sized. Machine types before
> > > 4.0 shouldn't have these fields so that the config space size remains
> > > unchanged.
> >
> > Sure!
> >
> > Since I should set a correct config size checking if new features are
> > enabled or not at runtime, should be better to add a variable and an array
> > of sizes like in virtio-net?
>
> In my series "[PATCH v4 0/6] virtio-blk: add DISCARD and WRITE_ZEROES"
> I'm adding the host_features field in VirtIOBlock. Then, I could add
> something like the following patch (proof of concept) inspired by the
> virtio-net approach, that would be simplest to maintain when we will add
> new features.
>
> What do you think?
>
> Thanks,
> Stefano
>
>
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 843bb2bec8..84dcc1406c 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -28,6 +28,51 @@
> #include "hw/virtio/virtio-bus.h"
> #include "hw/virtio/virtio-access.h"
>
> +/*
> + * Calculate the number of bytes up to and including the given 'field' of
> + * 'container'.
> + */
> +#define endof(container, field) \
> + (offsetof(container, field) + sizeof_field(container, field))
> +
e.g. virtio.h. just add virtio prefix.
> +typedef struct VirtIOFeature {
> + uint64_t flags;
> + size_t end;
> +} VirtIOFeature;
> +
> +static VirtIOFeature feature_sizes[] = {
> + {.flags = 1ULL << VIRTIO_NET_F_SIZE_MAX,
> + .end = endof(struct virtio_blk_config, size_max)},
> + {.flags = 1ULL << VIRTIO_NET_F_SEG_MAX,
> + .end = endof(struct virtio_blk_config, seg_max)},
> + {.flags = 1ULL << VIRTIO_NET_F_GEOMETRY,
> + .end = endof(struct virtio_blk_config, geometry)},
> + {.flags = 1ULL << VIRTIO_NET_F_BLK_SIZE,
> + .end = endof(struct virtio_blk_config, blk_size)},
> + {.flags = 1ULL << VIRTIO_NET_F_TOPOLOGY,
> + .end = endof(struct virtio_blk_config, opt_io_size)},
> + {.flags = 1ULL << VIRTIO_NET_F_CONFIG_WCE,
> + .end = endof(struct virtio_blk_config, wce)},
> + {.flags = 1ULL << VIRTIO_NET_F_MQ,
> + .end = endof(struct virtio_blk_config, num_queues)},
> + {}
All names above with net look wrong to me.
> +};
> +
> +static void virtio_blk_set_config_size(VirtIOBlock *s, uint64_t
> host_features)
> +{
> + int i, config_size;
> +
> + config_size = endof(struct virtio_blk_config, capacity);
> +
> + for (i = 0; feature_sizes[i].flags != 0; i++) {
> + if (host_features & feature_sizes[i].flags) {
> + config_size = MAX(feature_sizes[i].end, config_size);
> + }
> + }
> +
> + s->config_size = config_size;
> +}
> +
Put this in virtio.c maybe? size can be returned.
> static void virtio_blk_init_request(VirtIOBlock *s, VirtQueue *vq,
> VirtIOBlockReq *req)
> {
> @@ -757,7 +802,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)
> @@ -765,7 +810,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);
> @@ -948,8 +993,9 @@ 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, s->host_features);
> +
> + virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK, s->config_size);
>
> s->blk = conf->conf.blk;
> s->rq = NULL;
