| Message ID | 20231006202045.1161543-3-vsementsov@yandex-team.ru |
|---|---|
| State | New |
| Headers | show |
| Series | vhost-user-blk: live resize additional APIs | expand |
Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes: > Add command to sync config from vhost-user backend to the device. It > may be helpful when VHOST_USER_SLAVE_CONFIG_CHANGE_MSG failed or not > triggered interrupt to the guest or just not available (not supported > by vhost-user server). > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> [...] > diff --git a/qapi/qdev.json b/qapi/qdev.json > index fa80694735..2468f8bddf 100644 > --- a/qapi/qdev.json > +++ b/qapi/qdev.json > @@ -315,3 +315,17 @@ > # Since: 8.2 > ## > { 'event': 'X_DEVICE_ON', 'data': 'DeviceAndPath' } > + > +## > +# @x-device-sync-config: > +# > +# Sync config from backend to the guest. "Sync" is not a word; "synchronize" is :) > +# > +# @id: the device's ID or QOM path > +# > +# Returns: Nothing on success > +# If @id is not a valid device, DeviceNotFound Why not GenericError? > +# > +# Since: 8.2 > +## > +{ 'command': 'x-device-sync-config', 'data': {'id': 'str'} } The commit message above and the error message below talk about command device-sync-config, but you actually name it x-device-sync-config. I figure you use x- to signify "unstable". Please use feature flag 'unstable' for that. See docs/devel/qapi-code-gen.rst section "Features", in particular "Special features", and also the note on x- in section "Naming rules and reserved names". We tend to eschew abbreviations in QAPI schema names. device-synchronize-config is quite a mouthful, though. What do you think? > diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c > index 19c31446d8..b6da24389f 100644 > --- a/softmmu/qdev-monitor.c > +++ b/softmmu/qdev-monitor.c > @@ -987,6 +987,29 @@ HotplugInfo *qmp_x_query_hotplug(const char *id, Error **errp) > return hotplug_handler_get_state(hotplug_ctrl, dev, errp); > } > > +int qdev_sync_config(DeviceState *dev, Error **errp) > +{ > + DeviceClass *dc = DEVICE_GET_CLASS(dev); > + > + if (!dc->sync_config) { > + error_setg(errp, "device-sync-config is not supported for '%s'", > + object_get_typename(OBJECT(dev))); > + return -ENOTSUP; > + } > + > + return dc->sync_config(dev, errp); > +} > + > +void qmp_x_device_sync_config(const char *id, Error **errp) > +{ > + DeviceState *dev = find_device_state(id, errp); Not your patch's fault, but here goes anyway: when @id refers to a non-device, find_device_state() fails with "is not a hotpluggable device". "hotpluggable" is misleading. > + if (!dev) { > + return; > + } > + > + qdev_sync_config(dev, errp); > +} > + > void hmp_device_add(Monitor *mon, const QDict *qdict) > { > Error *err = NULL;
On 17.10.23 17:57, Markus Armbruster wrote: > Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes: > >> Add command to sync config from vhost-user backend to the device. It >> may be helpful when VHOST_USER_SLAVE_CONFIG_CHANGE_MSG failed or not >> triggered interrupt to the guest or just not available (not supported >> by vhost-user server). >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> > > [...] > >> diff --git a/qapi/qdev.json b/qapi/qdev.json >> index fa80694735..2468f8bddf 100644 >> --- a/qapi/qdev.json >> +++ b/qapi/qdev.json >> @@ -315,3 +315,17 @@ >> # Since: 8.2 >> ## >> { 'event': 'X_DEVICE_ON', 'data': 'DeviceAndPath' } >> + >> +## >> +# @x-device-sync-config: >> +# >> +# Sync config from backend to the guest. > > "Sync" is not a word; "synchronize" is :) Seems, I learn English from code :) > >> +# >> +# @id: the device's ID or QOM path >> +# >> +# Returns: Nothing on success >> +# If @id is not a valid device, DeviceNotFound > > Why not GenericError? I just designed the command looking at device_del. device_del reports DeviceNotFound in this case. GenericError is OK for me, if you think it's better even in this case. I remember now that everything except GenericError is not recommended. > >> +# >> +# Since: 8.2 >> +## >> +{ 'command': 'x-device-sync-config', 'data': {'id': 'str'} } > > The commit message above and the error message below talk about command > device-sync-config, but you actually name it x-device-sync-config. > > I figure you use x- to signify "unstable". Please use feature flag > 'unstable' for that. See docs/devel/qapi-code-gen.rst section > "Features", in particular "Special features", and also the note on x- in > section "Naming rules and reserved names". > > We tend to eschew abbreviations in QAPI schema names. > device-synchronize-config is quite a mouthful, though. What do you > think? OK for me. Hmm, could I ask here, is "config" a word?)) device-synchronize-configuration would become a precedent, I'm afraid) > >> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c >> index 19c31446d8..b6da24389f 100644 >> --- a/softmmu/qdev-monitor.c >> +++ b/softmmu/qdev-monitor.c >> @@ -987,6 +987,29 @@ HotplugInfo *qmp_x_query_hotplug(const char *id, Error **errp) >> return hotplug_handler_get_state(hotplug_ctrl, dev, errp); >> } >> >> +int qdev_sync_config(DeviceState *dev, Error **errp) >> +{ >> + DeviceClass *dc = DEVICE_GET_CLASS(dev); >> + >> + if (!dc->sync_config) { >> + error_setg(errp, "device-sync-config is not supported for '%s'", >> + object_get_typename(OBJECT(dev))); >> + return -ENOTSUP; >> + } >> + >> + return dc->sync_config(dev, errp); >> +} >> + >> +void qmp_x_device_sync_config(const char *id, Error **errp) >> +{ >> + DeviceState *dev = find_device_state(id, errp); > > Not your patch's fault, but here goes anyway: when @id refers to a > non-device, find_device_state() fails with "is not a hotpluggable > device". "hotpluggable" is misleading. Hmm. Thanks, OK, I'll rework it somehow in v2. > >> + if (!dev) { >> + return; >> + } >> + >> + qdev_sync_config(dev, errp); >> +} >> + >> void hmp_device_add(Monitor *mon, const QDict *qdict) >> { >> Error *err = NULL; >
Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes: > On 17.10.23 17:57, Markus Armbruster wrote: >> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes: >> >>> Add command to sync config from vhost-user backend to the device. It >>> may be helpful when VHOST_USER_SLAVE_CONFIG_CHANGE_MSG failed or not >>> triggered interrupt to the guest or just not available (not supported >>> by vhost-user server). >>> >>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> >> [...] >> >>> diff --git a/qapi/qdev.json b/qapi/qdev.json >>> index fa80694735..2468f8bddf 100644 >>> --- a/qapi/qdev.json >>> +++ b/qapi/qdev.json >>> @@ -315,3 +315,17 @@ >>> # Since: 8.2 >>> ## >>> { 'event': 'X_DEVICE_ON', 'data': 'DeviceAndPath' } >>> + >>> +## >>> +# @x-device-sync-config: >>> +# >>> +# Sync config from backend to the guest. >> >> "Sync" is not a word; "synchronize" is :) > > Seems, I learn English from code :) It's working, so no worries ;) >>> +# >>> +# @id: the device's ID or QOM path >>> +# >>> +# Returns: Nothing on success >>> +# If @id is not a valid device, DeviceNotFound >> >> Why not GenericError? > > I just designed the command looking at device_del. device_del reports DeviceNotFound in this case. GenericError is OK for me, if you think it's better even in this case. I remember now that everything except GenericError is not recommended. I figure you picked up DeviceNotFound by reusing find_device_state(). Same happened when commit 9680caee0fa added blk_by_qdev_id(). At least some of its users don't document the error code. I'm not sure the unwanted use of DeviceNotFound is worth fixing after all this time. But I certainly don't want it documented. For your patch, not reusing find_device_state() would let you avoid DeviceNotFound at the price of a few more lines of code. >>> +# >>> +# Since: 8.2 >>> +## >>> +{ 'command': 'x-device-sync-config', 'data': {'id': 'str'} } >> >> The commit message above and the error message below talk about command >> device-sync-config, but you actually name it x-device-sync-config. >> >> I figure you use x- to signify "unstable". Please use feature flag >> 'unstable' for that. See docs/devel/qapi-code-gen.rst section >> "Features", in particular "Special features", and also the note on x- in >> section "Naming rules and reserved names". >> >> We tend to eschew abbreviations in QAPI schema names. >> device-synchronize-config is quite a mouthful, though. What do you >> think? > > OK for me. > > Hmm, could I ask here, is "config" a word?)) device-synchronize-configuration would become a precedent, I'm afraid) In the words of Captain Barbossa, it's "more what you'd call 'guidelines' than actual rules." I didn't come up with the "avoid abbreviations" stylistic guideline. I inherited it. I do like consistent style. I don't like excessively long names. Sometimes these likes conflict, and we need to pick. Checking... alright, there precedence both for 'config' and for 'sync' in the QAPI schema. You pick what you like best. >>> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c >>> index 19c31446d8..b6da24389f 100644 >>> --- a/softmmu/qdev-monitor.c >>> +++ b/softmmu/qdev-monitor.c >>> @@ -987,6 +987,29 @@ HotplugInfo *qmp_x_query_hotplug(const char *id, Error **errp) >>> return hotplug_handler_get_state(hotplug_ctrl, dev, errp); >>> } >>> +int qdev_sync_config(DeviceState *dev, Error **errp) >>> +{ >>> + DeviceClass *dc = DEVICE_GET_CLASS(dev); >>> + >>> + if (!dc->sync_config) { >>> + error_setg(errp, "device-sync-config is not supported for '%s'", >>> + object_get_typename(OBJECT(dev))); >>> + return -ENOTSUP; >>> + } >>> + >>> + return dc->sync_config(dev, errp); >>> +} >>> + >>> +void qmp_x_device_sync_config(const char *id, Error **errp) >>> +{ >>> + DeviceState *dev = find_device_state(id, errp); >> >> Not your patch's fault, but here goes anyway: when @id refers to a >> non-device, find_device_state() fails with "is not a hotpluggable >> device". "hotpluggable" is misleading. > > Hmm. Thanks, OK, I'll rework it somehow in v2. I think "hotpluggable" is misleading for all the existing uses of find_device_state(). Suggest a preliminary patch deleting the word. >>> + if (!dev) { >>> + return; >>> + } >>> + >>> + qdev_sync_config(dev, errp); >>> +} >>> + >>> void hmp_device_add(Monitor *mon, const QDict *qdict) >>> { >>> Error *err = NULL; >>
diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c index 1ee05b46ee..05ce7b5684 100644 --- a/hw/block/vhost-user-blk.c +++ b/hw/block/vhost-user-blk.c @@ -90,27 +90,39 @@ static void vhost_user_blk_set_config(VirtIODevice *vdev, const uint8_t *config) s->blkcfg.wce = blkcfg->wce; } +static int vhost_user_blk_sync_config(DeviceState *dev, Error **errp) +{ + int ret; + VirtIODevice *vdev = VIRTIO_DEVICE(dev); + VHostUserBlk *s = VHOST_USER_BLK(vdev); + + ret = vhost_dev_get_config(&s->dev, (uint8_t *)&s->blkcfg, + vdev->config_len, errp); + if (ret < 0) { + return ret; + } + + memcpy(vdev->config, &s->blkcfg, vdev->config_len); + virtio_notify_config(vdev); + + return 0; +} + static int vhost_user_blk_handle_config_change(struct vhost_dev *dev) { int ret; - VirtIODevice *vdev = dev->vdev; - VHostUserBlk *s = VHOST_USER_BLK(dev->vdev); Error *local_err = NULL; if (!dev->started) { return 0; } - ret = vhost_dev_get_config(dev, (uint8_t *)&s->blkcfg, - vdev->config_len, &local_err); + ret = vhost_user_blk_sync_config(DEVICE(dev->vdev), &local_err); if (ret < 0) { error_report_err(local_err); return ret; } - memcpy(dev->vdev->config, &s->blkcfg, vdev->config_len); - virtio_notify_config(dev->vdev); - return 0; } @@ -580,6 +592,7 @@ static void vhost_user_blk_class_init(ObjectClass *klass, void *data) device_class_set_props(dc, vhost_user_blk_properties); dc->vmsd = &vmstate_vhost_user_blk; + dc->sync_config = vhost_user_blk_sync_config; set_bit(DEVICE_CATEGORY_STORAGE, dc->categories); vdc->realize = vhost_user_blk_device_realize; vdc->unrealize = vhost_user_blk_device_unrealize; diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c index edbc0daa18..dd4620462b 100644 --- a/hw/virtio/virtio-pci.c +++ b/hw/virtio/virtio-pci.c @@ -2315,6 +2315,14 @@ static void virtio_pci_dc_realize(DeviceState *qdev, Error **errp) vpciklass->parent_dc_realize(qdev, errp); } +static int virtio_pci_sync_config(DeviceState *dev, Error **errp) +{ + VirtIOPCIProxy *proxy = VIRTIO_PCI(dev); + VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus); + + return qdev_sync_config(DEVICE(vdev), errp); +} + static void virtio_pci_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); @@ -2331,6 +2339,7 @@ static void virtio_pci_class_init(ObjectClass *klass, void *data) device_class_set_parent_realize(dc, virtio_pci_dc_realize, &vpciklass->parent_dc_realize); rc->phases.hold = virtio_pci_bus_reset_hold; + dc->sync_config = virtio_pci_sync_config; } static const TypeInfo virtio_pci_info = { diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h index a27ef2eb24..6fa5bac86e 100644 --- a/include/hw/qdev-core.h +++ b/include/hw/qdev-core.h @@ -95,6 +95,7 @@ typedef void (*DeviceUnrealize)(DeviceState *dev); typedef void (*DeviceReset)(DeviceState *dev); typedef void (*BusRealize)(BusState *bus, Error **errp); typedef void (*BusUnrealize)(BusState *bus); +typedef int (*DeviceSyncConfig)(DeviceState *dev, Error **errp); /** * struct DeviceClass - The base class for all devices. @@ -162,6 +163,7 @@ struct DeviceClass { DeviceReset reset; DeviceRealize realize; DeviceUnrealize unrealize; + DeviceSyncConfig sync_config; /** * @vmsd: device state serialisation description for @@ -555,6 +557,7 @@ bool qdev_hotplug_allowed(DeviceState *dev, Error **errp); */ HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev); void qdev_unplug(DeviceState *dev, Error **errp); +int qdev_sync_config(DeviceState *dev, Error **errp); void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev, Error **errp); void qdev_machine_creation_done(void); diff --git a/qapi/qdev.json b/qapi/qdev.json index fa80694735..2468f8bddf 100644 --- a/qapi/qdev.json +++ b/qapi/qdev.json @@ -315,3 +315,17 @@ # Since: 8.2 ## { 'event': 'X_DEVICE_ON', 'data': 'DeviceAndPath' } + +## +# @x-device-sync-config: +# +# Sync config from backend to the guest. +# +# @id: the device's ID or QOM path +# +# Returns: Nothing on success +# If @id is not a valid device, DeviceNotFound +# +# Since: 8.2 +## +{ 'command': 'x-device-sync-config', 'data': {'id': 'str'} } diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c index 19c31446d8..b6da24389f 100644 --- a/softmmu/qdev-monitor.c +++ b/softmmu/qdev-monitor.c @@ -987,6 +987,29 @@ HotplugInfo *qmp_x_query_hotplug(const char *id, Error **errp) return hotplug_handler_get_state(hotplug_ctrl, dev, errp); } +int qdev_sync_config(DeviceState *dev, Error **errp) +{ + DeviceClass *dc = DEVICE_GET_CLASS(dev); + + if (!dc->sync_config) { + error_setg(errp, "device-sync-config is not supported for '%s'", + object_get_typename(OBJECT(dev))); + return -ENOTSUP; + } + + return dc->sync_config(dev, errp); +} + +void qmp_x_device_sync_config(const char *id, Error **errp) +{ + DeviceState *dev = find_device_state(id, errp); + if (!dev) { + return; + } + + qdev_sync_config(dev, errp); +} + void hmp_device_add(Monitor *mon, const QDict *qdict) { Error *err = NULL;
Add command to sync config from vhost-user backend to the device. It may be helpful when VHOST_USER_SLAVE_CONFIG_CHANGE_MSG failed or not triggered interrupt to the guest or just not available (not supported by vhost-user server). Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> --- hw/block/vhost-user-blk.c | 27 ++++++++++++++++++++------- hw/virtio/virtio-pci.c | 9 +++++++++ include/hw/qdev-core.h | 3 +++ qapi/qdev.json | 14 ++++++++++++++ softmmu/qdev-monitor.c | 23 +++++++++++++++++++++++ 5 files changed, 69 insertions(+), 7 deletions(-)