* [Qemu-devel] [PATCH v4] virtio-blk: set correct config size for the host driver
@ 2019-02-13 1:48 Changpeng Liu
2019-02-13 2:05 ` Michael S. Tsirkin
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Changpeng Liu @ 2019-02-13 1:48 UTC (permalink / raw)
To: qemu-devel; +Cc: mst, stefanha, sgarzare, dgilbert, ldoktor, changpeng.liu
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>
---
hw/block/virtio-blk.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 9a87b3b..6fce9c7 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -28,6 +28,10 @@
#include "hw/virtio/virtio-bus.h"
#include "hw/virtio/virtio-access.h"
+/* We don't support discard yet, hide associated config fields. */
+#define VIRTIO_BLK_CFG_SIZE offsetof(struct virtio_blk_config, \
+ max_discard_sectors)
+
static void virtio_blk_init_request(VirtIOBlock *s, VirtQueue *vq,
VirtIOBlockReq *req)
{
@@ -761,7 +765,8 @@ 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);
+ QEMU_BUILD_BUG_ON(VIRTIO_BLK_CFG_SIZE > sizeof(blkcfg));
}
static void virtio_blk_set_config(VirtIODevice *vdev, const uint8_t *config)
@@ -769,7 +774,8 @@ 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);
+ 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 +958,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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v4] virtio-blk: set correct config size for the host driver
2019-02-13 1:48 [Qemu-devel] [PATCH v4] virtio-blk: set correct config size for the host driver Changpeng Liu
@ 2019-02-13 2:05 ` Michael S. Tsirkin
2019-02-13 8:00 ` Stefan Hajnoczi
2019-02-13 8:01 ` Stefan Hajnoczi
2019-02-13 8:07 ` Greg Kurz
2 siblings, 1 reply; 10+ messages in thread
From: Michael S. Tsirkin @ 2019-02-13 2:05 UTC (permalink / raw)
To: Changpeng Liu; +Cc: qemu-devel, stefanha, sgarzare, dgilbert, ldoktor
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.liu@intel.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Stefan, are you merging this?
> ---
> hw/block/virtio-blk.c | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 9a87b3b..6fce9c7 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -28,6 +28,10 @@
> #include "hw/virtio/virtio-bus.h"
> #include "hw/virtio/virtio-access.h"
>
> +/* We don't support discard yet, hide associated config fields. */
> +#define VIRTIO_BLK_CFG_SIZE offsetof(struct virtio_blk_config, \
> + max_discard_sectors)
> +
> static void virtio_blk_init_request(VirtIOBlock *s, VirtQueue *vq,
> VirtIOBlockReq *req)
> {
> @@ -761,7 +765,8 @@ 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);
> + QEMU_BUILD_BUG_ON(VIRTIO_BLK_CFG_SIZE > sizeof(blkcfg));
> }
>
> static void virtio_blk_set_config(VirtIODevice *vdev, const uint8_t *config)
> @@ -769,7 +774,8 @@ 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);
> + 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 +958,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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v4] virtio-blk: set correct config size for the host driver
2019-02-13 2:05 ` Michael S. Tsirkin
@ 2019-02-13 8:00 ` Stefan Hajnoczi
0 siblings, 0 replies; 10+ messages in thread
From: Stefan Hajnoczi @ 2019-02-13 8:00 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Changpeng Liu, ldoktor, dgilbert, qemu-devel, stefanha, sgarzare
[-- Attachment #1: Type: text/plain, Size: 1212 bytes --]
On Tue, Feb 12, 2019 at 09:05:11PM -0500, Michael S. Tsirkin 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.liu@intel.com>
>
>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>
> Stefan, are you merging this?
Yes.
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v4] virtio-blk: set correct config size for the host driver
2019-02-13 1:48 [Qemu-devel] [PATCH v4] virtio-blk: set correct config size for the host driver Changpeng Liu
2019-02-13 2:05 ` Michael S. Tsirkin
@ 2019-02-13 8:01 ` Stefan Hajnoczi
2019-02-13 8:32 ` Stefano Garzarella
2019-02-13 8:07 ` Greg Kurz
2 siblings, 1 reply; 10+ messages in thread
From: Stefan Hajnoczi @ 2019-02-13 8:01 UTC (permalink / raw)
To: Changpeng Liu; +Cc: qemu-devel, ldoktor, mst, dgilbert, stefanha, sgarzare
[-- Attachment #1: Type: text/plain, Size: 1418 bytes --]
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.liu@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.
Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v4] virtio-blk: set correct config size for the host driver
2019-02-13 1:48 [Qemu-devel] [PATCH v4] virtio-blk: set correct config size for the host driver Changpeng Liu
2019-02-13 2:05 ` Michael S. Tsirkin
2019-02-13 8:01 ` Stefan Hajnoczi
@ 2019-02-13 8:07 ` Greg Kurz
2 siblings, 0 replies; 10+ messages in thread
From: Greg Kurz @ 2019-02-13 8:07 UTC (permalink / raw)
To: Changpeng Liu; +Cc: qemu-devel, ldoktor, mst, dgilbert, stefanha, sgarzare
On Wed, 13 Feb 2019 09:48:57 +0800
Changpeng Liu <changpeng.liu@intel.com> 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.liu@intel.com>
> ---
Again ;-)
Reviewed-by: Greg Kurz <groug@kaod.org>
Tested-by: Greg Kurz <groug@kaod.org>
> hw/block/virtio-blk.c | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 9a87b3b..6fce9c7 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -28,6 +28,10 @@
> #include "hw/virtio/virtio-bus.h"
> #include "hw/virtio/virtio-access.h"
>
> +/* We don't support discard yet, hide associated config fields. */
> +#define VIRTIO_BLK_CFG_SIZE offsetof(struct virtio_blk_config, \
> + max_discard_sectors)
> +
> static void virtio_blk_init_request(VirtIOBlock *s, VirtQueue *vq,
> VirtIOBlockReq *req)
> {
> @@ -761,7 +765,8 @@ 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);
> + QEMU_BUILD_BUG_ON(VIRTIO_BLK_CFG_SIZE > sizeof(blkcfg));
> }
>
> static void virtio_blk_set_config(VirtIODevice *vdev, const uint8_t *config)
> @@ -769,7 +774,8 @@ 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);
> + 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 +958,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;
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v4] virtio-blk: set correct config size for the host driver
2019-02-13 8:01 ` Stefan Hajnoczi
@ 2019-02-13 8:32 ` Stefano Garzarella
2019-02-13 12:17 ` Stefano Garzarella
0 siblings, 1 reply; 10+ messages in thread
From: Stefano Garzarella @ 2019-02-13 8:32 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Changpeng Liu, qemu-devel, ldoktor, mst, dgilbert, stefanha, Greg Kurz
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.liu@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?
Thanks,
Stefano
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v4] virtio-blk: set correct config size for the host driver
2019-02-13 8:32 ` Stefano Garzarella
@ 2019-02-13 12:17 ` Stefano Garzarella
2019-02-13 17:06 ` Michael S. Tsirkin
0 siblings, 1 reply; 10+ messages in thread
From: Stefano Garzarella @ 2019-02-13 12:17 UTC (permalink / raw)
To: Stefan Hajnoczi, Changpeng Liu, mst
Cc: qemu-devel, ldoktor, dgilbert, stefanha, Greg Kurz
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.liu@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))
+
+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)},
+ {}
+};
+
+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;
+}
+
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;
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v4] virtio-blk: set correct config size for the host driver
2019-02-13 12:17 ` Stefano Garzarella
@ 2019-02-13 17:06 ` Michael S. Tsirkin
2019-02-13 17:45 ` Stefano Garzarella
0 siblings, 1 reply; 10+ messages in thread
From: Michael S. Tsirkin @ 2019-02-13 17:06 UTC (permalink / raw)
To: Stefano Garzarella
Cc: Stefan Hajnoczi, Changpeng Liu, qemu-devel, ldoktor, dgilbert,
stefanha, Greg Kurz
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.liu@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;
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v4] virtio-blk: set correct config size for the host driver
2019-02-13 17:06 ` Michael S. Tsirkin
@ 2019-02-13 17:45 ` Stefano Garzarella
2019-02-14 3:37 ` Stefan Hajnoczi
0 siblings, 1 reply; 10+ messages in thread
From: Stefano Garzarella @ 2019-02-13 17:45 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Stefan Hajnoczi, Changpeng Liu, qemu devel list, Lukas Doktor,
Dr. David Alan Gilbert, Stefan Hajnoczi, Greg Kurz
On Wed, Feb 13, 2019 at 6:07 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> On Wed, Feb 13, 2019 at 01:17:03PM +0100, Stefano Garzarella wrote:
> >
> > 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.
Make sense, maybe also the VirtIOFeature defined below can go in virtio.h.
>
> > +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.
Yes, they are wrong :) s/NET/BLK
I'm not sure if using the feature_sizes array the migration works well.
I mean if we have QEMU 3.1 with a single queue and we want to migrate to
QEMU 4.0 with a single queue, the config_size could be different,
because VIRTIO_NET_F_MQ is not set in the host_features.
Maybe we should initialize a config_size to the VIRTIO_BLK_CFG_SIZE macro
defined by Changpeng and then use the feature_sizes arrays only for the
new features (i.e. discard, write_zeroes)
What do you think?
>
> > +};
> > +
> > +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.
Do you want to reuse it also in virtio-net?
I should add some parameters (feature_sizes pointer and len), but I
think can work.
Thanks,
Stefano
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v4] virtio-blk: set correct config size for the host driver
2019-02-13 17:45 ` Stefano Garzarella
@ 2019-02-14 3:37 ` Stefan Hajnoczi
0 siblings, 0 replies; 10+ messages in thread
From: Stefan Hajnoczi @ 2019-02-14 3:37 UTC (permalink / raw)
To: Stefano Garzarella
Cc: Michael S. Tsirkin, Stefan Hajnoczi, Changpeng Liu,
qemu devel list, Lukas Doktor, Dr. David Alan Gilbert, Greg Kurz
[-- Attachment #1: Type: text/plain, Size: 756 bytes --]
On Wed, Feb 13, 2019 at 06:45:20PM +0100, Stefano Garzarella wrote:
> I'm not sure if using the feature_sizes array the migration works well.
> I mean if we have QEMU 3.1 with a single queue and we want to migrate to
> QEMU 4.0 with a single queue, the config_size could be different,
> because VIRTIO_NET_F_MQ is not set in the host_features.
>
> Maybe we should initialize a config_size to the VIRTIO_BLK_CFG_SIZE macro
> defined by Changpeng and then use the feature_sizes arrays only for the
> new features (i.e. discard, write_zeroes)
>
> What do you think?
I agree. The config size must not change for existing machine types.
Your code makes sense for future features but old features must use the
static config size.
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-02-14 3:37 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-13 1:48 [Qemu-devel] [PATCH v4] virtio-blk: set correct config size for the host driver Changpeng Liu
2019-02-13 2:05 ` Michael S. Tsirkin
2019-02-13 8:00 ` Stefan Hajnoczi
2019-02-13 8:01 ` Stefan Hajnoczi
2019-02-13 8:32 ` Stefano Garzarella
2019-02-13 12:17 ` Stefano Garzarella
2019-02-13 17:06 ` Michael S. Tsirkin
2019-02-13 17:45 ` Stefano Garzarella
2019-02-14 3:37 ` Stefan Hajnoczi
2019-02-13 8:07 ` Greg Kurz
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.