Re: [PATCH v2 4/6] qapi: introduce device-sync-config

[Vladimir Sementsov-Ogievskiy]

On 07.03.24 12:54, 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>
> > ---
> >   hw/block/vhost-user-blk.c | 27 ++++++++++++++++++++-------
> >   hw/virtio/virtio-pci.c    |  9 +++++++++
> >   include/hw/qdev-core.h    |  3 +++
> >   qapi/qdev.json            | 17 +++++++++++++++++
> >   system/qdev-monitor.c     | 23 +++++++++++++++++++++++
> >   5 files changed, 72 insertions(+), 7 deletions(-)
> >
> > diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> > index 9e6bbc6950..2f301f380c 100644
> > --- a/hw/block/vhost-user-blk.c
> > +++ b/hw/block/vhost-user-blk.c
> > @@ -88,27 +88,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;
> >   }
> >   
> > @@ -576,6 +588,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 1a7039fb0c..1197341a3c 100644
> > --- a/hw/virtio/virtio-pci.c
> > +++ b/hw/virtio/virtio-pci.c
> > @@ -2318,6 +2318,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);
> > @@ -2334,6 +2342,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 9228e96c87..87135bdcdf 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
> > @@ -546,6 +548,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 32ffaee644..6ece164172 100644
> > --- a/qapi/qdev.json
> > +++ b/qapi/qdev.json
> > @@ -162,3 +162,20 @@
> >   ##
> >   { 'event': 'DEVICE_UNPLUG_GUEST_ERROR',
> >     'data': { '*device': 'str', 'path': 'str' } }
> > +
> > +##
> > +# @device-sync-config:
> > +#
> > +# Synchronize config from backend to the guest.
> > +#
> > +# @id: the device's ID or QOM path
> > +#
> > +# Features:
> > +#
> > +# @unstable: The command is experimental.
> > +#
> > +# Since: 9.0
> > +##
>
> Even for an experimental command a bit more guidance on intended would
> be good.

Will add

> The commit message describes this as a "command to sync config from
> vhost-user backend to the device".  Here, there's no mention of
> vhost-user.
>
> Could the command make sense for devices other than vhost-user?

Yes it should work for any kind of backend, which can't notify Qemu about 
changed configuration (like disk size).

> > +{ 'command': 'device-sync-config',
> > +  'features': [ 'unstable' ],
> > +  'data': {'id': 'str'} }
> > diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
> > index cf7481e416..e3107a12d7 100644
> > --- a/system/qdev-monitor.c
> > +++ b/system/qdev-monitor.c
> > @@ -968,6 +968,29 @@ void qmp_device_del(const char *id, Error **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_device_sync_config(const char *id, Error **errp)
> > +{
> > +    DeviceState *dev = find_device_state(id, true, errp);
> > +    if (!dev) {
> > +        return;
> > +    }
> > +
> > +    qdev_sync_config(dev, errp);
> > +}
> > +
> >   void hmp_device_add(Monitor *mon, const QDict *qdict)
> >   {
> >       Error *err = NULL;

--
Best regards,
Vladimir