Re: [PATCH v0 1/2] vhost-user-blk: add a new vhost-user-virtio-blk type
On Mon, Oct 04, 2021 at 06:07:30PM +0300, Denis Plotnikov wrote: > Adding the new vhost-user-blk type instead of modifying the existing one > is convenent. It ease to differ the new virtio-blk-compatible vhost-user-blk > device from the existing non-compatible one using qemu machinery without any > other modifiactions. That gives all the variety of qemu device related > constraints out of box. Convenient for the developer maybe, but isn't it confusing for the user? I don't really understand this paragraph. E.g. what are the constraints here? > > Signed-off-by: Denis Plotnikov > --- > hw/block/vhost-user-blk.c | 63 ++ > include/hw/virtio/vhost-user-blk.h | 2 + > 2 files changed, 65 insertions(+) > > diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c > index ba13cb87e520..877fe54e891f 100644 > --- a/hw/block/vhost-user-blk.c > +++ b/hw/block/vhost-user-blk.c > @@ -30,6 +30,7 @@ > #include "hw/virtio/virtio-access.h" > #include "sysemu/sysemu.h" > #include "sysemu/runstate.h" > +#include "migration/qemu-file-types.h" > > #define REALIZE_CONNECTION_RETRIES 3 > > @@ -612,9 +613,71 @@ static const TypeInfo vhost_user_blk_info = { > .class_init = vhost_user_blk_class_init, > }; > > +/* > + * this is the same as vmstate_virtio_blk > + * we use it to allow virtio-blk <-> vhost-user-virtio-blk migration > + */ > +static const VMStateDescription vmstate_vhost_user_virtio_blk = { > +.name = "virtio-blk", > +.minimum_version_id = 2, > +.version_id = 2, > +.fields = (VMStateField[]) { > +VMSTATE_VIRTIO_DEVICE, > +VMSTATE_END_OF_LIST() > +}, > +}; > + > +static void vhost_user_virtio_blk_save(VirtIODevice *vdev, QEMUFile *f) > +{ > +/* > + * put a zero byte in the stream to be compatible with virtio-blk > + */ > +qemu_put_sbyte(f, 0); > +} > + > +static int vhost_user_virtio_blk_load(VirtIODevice *vdev, QEMUFile *f, > + int version_id) > +{ > +if (qemu_get_sbyte(f)) { > +/* > + * on virtio-blk -> vhost-user-virtio-blk migration we don't expect > + * to get any infilght requests in the migration stream because > + * we can't load them yet. > + * TODO: consider putting those inflight requests to inflight region > + */ > +error_report("%s: can't load in-flight requests", > + TYPE_VHOST_USER_VIRTIO_BLK); > +return -EINVAL; > +} > + > +return 0; > +} > + > +static void vhost_user_virtio_blk_class_init(ObjectClass *klass, void *data) > +{ > +DeviceClass *dc = DEVICE_CLASS(klass); > +VirtioDeviceClass *vdc = VIRTIO_DEVICE_CLASS(klass); > + > +/* override vmstate of vhost_user_blk */ > +dc->vmsd = _vhost_user_virtio_blk; So can't we do something like this in vhost_user_blk_class_init: if qemu >= 6.2 dc->vmsd = _vhost_user_virtio_blk; else dc->vmsd = _vhost_user_blk ? > + > +/* adding callbacks to be compatible with virtio-blk migration stream */ > +vdc->save = vhost_user_virtio_blk_save; > +vdc->load = vhost_user_virtio_blk_load; > +} > + > +static const TypeInfo vhost_user_virtio_blk_info = { > +.name = TYPE_VHOST_USER_VIRTIO_BLK, > +.parent = TYPE_VHOST_USER_BLK, > +.instance_size = sizeof(VHostUserBlk), > +/* instance_init is the same as in parent type */ > +.class_init = vhost_user_virtio_blk_class_init, > +}; > + > static void virtio_register_types(void) > { > type_register_static(_user_blk_info); > +type_register_static(_user_virtio_blk_info); > } > > type_init(virtio_register_types) > diff --git a/include/hw/virtio/vhost-user-blk.h > b/include/hw/virtio/vhost-user-blk.h > index 7c91f15040eb..d81f18d22596 100644 > --- a/include/hw/virtio/vhost-user-blk.h > +++ b/include/hw/virtio/vhost-user-blk.h > @@ -23,6 +23,8 @@ > #include "qom/object.h" > > #define TYPE_VHOST_USER_BLK "vhost-user-blk" > +#define TYPE_VHOST_USER_VIRTIO_BLK "vhost-user-virtio-blk" > + > OBJECT_DECLARE_SIMPLE_TYPE(VHostUserBlk, VHOST_USER_BLK) > > #define VHOST_USER_BLK_AUTO_NUM_QUEUES UINT16_MAX > -- > 2.25.1
[PATCH v0 1/2] vhost-user-blk: add a new vhost-user-virtio-blk type
The main reason of adding a new type is to make cross-device live migration between "virtio-blk" and "vhost-user-blk" devices possible in both directions. It might be useful for the cases when a slow block layer should be replaced with a more performant one on running VM without stopping, i.e. with very low downtime comparable with the one on migration. It's possible to achive that for two reasons: 1.The VMStates of "virtio-blk" and "vhost-user-blk" are almost the same. They consist of the identical VMSTATE_VIRTIO_DEVICE and differs from each other in the values of migration service fields only. 2.The device driver used in the guest is the same: virtio-blk The new type uses virtio-blk VMState instead of vhost-user-blk specific VMstate, also it implements migration save/load callbacks to be compatible with migration stream produced by "virtio-blk" device. Adding the new vhost-user-blk type instead of modifying the existing one is convenent. It ease to differ the new virtio-blk-compatible vhost-user-blk device from the existing non-compatible one using qemu machinery without any other modifiactions. That gives all the variety of qemu device related constraints out of box. Signed-off-by: Denis Plotnikov --- hw/block/vhost-user-blk.c | 63 ++ include/hw/virtio/vhost-user-blk.h | 2 + 2 files changed, 65 insertions(+) diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c index ba13cb87e520..877fe54e891f 100644 --- a/hw/block/vhost-user-blk.c +++ b/hw/block/vhost-user-blk.c @@ -30,6 +30,7 @@ #include "hw/virtio/virtio-access.h" #include "sysemu/sysemu.h" #include "sysemu/runstate.h" +#include "migration/qemu-file-types.h" #define REALIZE_CONNECTION_RETRIES 3 @@ -612,9 +613,71 @@ static const TypeInfo vhost_user_blk_info = { .class_init = vhost_user_blk_class_init, }; +/* + * this is the same as vmstate_virtio_blk + * we use it to allow virtio-blk <-> vhost-user-virtio-blk migration + */ +static const VMStateDescription vmstate_vhost_user_virtio_blk = { +.name = "virtio-blk", +.minimum_version_id = 2, +.version_id = 2, +.fields = (VMStateField[]) { +VMSTATE_VIRTIO_DEVICE, +VMSTATE_END_OF_LIST() +}, +}; + +static void vhost_user_virtio_blk_save(VirtIODevice *vdev, QEMUFile *f) +{ +/* + * put a zero byte in the stream to be compatible with virtio-blk + */ +qemu_put_sbyte(f, 0); +} + +static int vhost_user_virtio_blk_load(VirtIODevice *vdev, QEMUFile *f, + int version_id) +{ +if (qemu_get_sbyte(f)) { +/* + * on virtio-blk -> vhost-user-virtio-blk migration we don't expect + * to get any infilght requests in the migration stream because + * we can't load them yet. + * TODO: consider putting those inflight requests to inflight region + */ +error_report("%s: can't load in-flight requests", + TYPE_VHOST_USER_VIRTIO_BLK); +return -EINVAL; +} + +return 0; +} + +static void vhost_user_virtio_blk_class_init(ObjectClass *klass, void *data) +{ +DeviceClass *dc = DEVICE_CLASS(klass); +VirtioDeviceClass *vdc = VIRTIO_DEVICE_CLASS(klass); + +/* override vmstate of vhost_user_blk */ +dc->vmsd = _vhost_user_virtio_blk; + +/* adding callbacks to be compatible with virtio-blk migration stream */ +vdc->save = vhost_user_virtio_blk_save; +vdc->load = vhost_user_virtio_blk_load; +} + +static const TypeInfo vhost_user_virtio_blk_info = { +.name = TYPE_VHOST_USER_VIRTIO_BLK, +.parent = TYPE_VHOST_USER_BLK, +.instance_size = sizeof(VHostUserBlk), +/* instance_init is the same as in parent type */ +.class_init = vhost_user_virtio_blk_class_init, +}; + static void virtio_register_types(void) { type_register_static(_user_blk_info); +type_register_static(_user_virtio_blk_info); } type_init(virtio_register_types) diff --git a/include/hw/virtio/vhost-user-blk.h b/include/hw/virtio/vhost-user-blk.h index 7c91f15040eb..d81f18d22596 100644 --- a/include/hw/virtio/vhost-user-blk.h +++ b/include/hw/virtio/vhost-user-blk.h @@ -23,6 +23,8 @@ #include "qom/object.h" #define TYPE_VHOST_USER_BLK "vhost-user-blk" +#define TYPE_VHOST_USER_VIRTIO_BLK "vhost-user-virtio-blk" + OBJECT_DECLARE_SIMPLE_TYPE(VHostUserBlk, VHOST_USER_BLK) #define VHOST_USER_BLK_AUTO_NUM_QUEUES UINT16_MAX -- 2.25.1