diff mbox series

[v4,3/3] qapi: introduce device-sync-config

Message ID 20240429101623.1992943-4-vsementsov@yandex-team.ru
State New
Headers show
Series [v4,1/3] qdev-monitor: add option to report GenericError from find_device_state | expand

Commit Message

Vladimir Sementsov-Ogievskiy April 29, 2024, 10:16 a.m. UTC
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).

Command result is racy if allow it during migration. Let's allow the
sync only in RUNNING state.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 hw/block/vhost-user-blk.c |  1 +
 hw/virtio/virtio-pci.c    |  9 ++++++++
 include/hw/qdev-core.h    |  3 +++
 qapi/qdev.json            | 23 +++++++++++++++++++
 system/qdev-monitor.c     | 48 +++++++++++++++++++++++++++++++++++++++
 5 files changed, 84 insertions(+)

Comments

Markus Armbruster April 30, 2024, 8:19 a.m. UTC | #1
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).
>
> Command result is racy if allow it during migration. Let's allow the
> sync only in RUNNING state.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  hw/block/vhost-user-blk.c |  1 +
>  hw/virtio/virtio-pci.c    |  9 ++++++++
>  include/hw/qdev-core.h    |  3 +++
>  qapi/qdev.json            | 23 +++++++++++++++++++
>  system/qdev-monitor.c     | 48 +++++++++++++++++++++++++++++++++++++++
>  5 files changed, 84 insertions(+)
>
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index 091d2c6acf..2f301f380c 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -588,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 b1d02f4b3d..0d91e8b5dc 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -2351,6 +2351,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);
> @@ -2367,6 +2375,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 facaa0bc6a..fc5e125a45 100644
> --- a/qapi/qdev.json
> +++ b/qapi/qdev.json
> @@ -161,3 +161,26 @@
>  ##
>  { 'event': 'DEVICE_UNPLUG_GUEST_ERROR',
>    'data': { '*device': 'str', 'path': 'str' } }
> +
> +##
> +# @device-sync-config:
> +#
> +# Synchronize device configuration from host to guest part.  First,
> +# copy the configuration from the host part (backend) to the guest
> +# part (frontend).  Then notify guest software that device
> +# configuration changed.

Blank line here, please.

> +# The command may be used to notify the guest about block device
> +# capcity change.  Currently only vhost-user-blk device supports
> +# this.
> +#
> +# @id: the device's ID or QOM path
> +#
> +# Features:
> +#
> +# @unstable: The command is experimental.
> +#
> +# Since: 9.1
> +##
> +{ 'command': 'device-sync-config',
> +  'features': [ 'unstable' ],
> +  'data': {'id': 'str'} }
> diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
> index 264978aa40..47bfc0506e 100644
> --- a/system/qdev-monitor.c
> +++ b/system/qdev-monitor.c
> @@ -23,6 +23,7 @@
>  #include "monitor/monitor.h"
>  #include "monitor/qdev.h"
>  #include "sysemu/arch_init.h"
> +#include "sysemu/runstate.h"
>  #include "qapi/error.h"
>  #include "qapi/qapi-commands-qdev.h"
>  #include "qapi/qmp/dispatch.h"
> @@ -971,6 +972,53 @@ 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;
> +
> +    /*
> +     * During migration there is a race between syncing`configuration and
> +     * migrating it (if migrate first, that target would get outdated version),
> +     * so let's just not allow it.

Wrap comment lines around column 70 for legibility, please.

> +     *
> +     * Moreover, let's not rely on setting up interrupts in paused
> +     * state, which may be a part of migration process.

We discussed this in review of v3.  You wanted to check whether the
problem is real.  Is it?

> +     */
> +
> +    if (migration_is_running()) {
> +        error_setg(errp, "Config synchronization is not allowed "
> +                   "during migration");
> +        return;
> +    }
> +
> +    if (!runstate_is_running()) {
> +        error_setg(errp, "Config synchronization allowed only in '%s' state, "

Suggest a line break after errp,

> +                   "current state is '%s'", RunState_str(RUN_STATE_RUNNING),
> +                   RunState_str(runstate_get()));
> +        return;
> +    }
> +
> +    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;
Vladimir Sementsov-Ogievskiy April 30, 2024, 8:31 a.m. UTC | #2
On 30.04.24 11:19, 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).
>>
>> Command result is racy if allow it during migration. Let's allow the
>> sync only in RUNNING state.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> ---
>>   hw/block/vhost-user-blk.c |  1 +
>>   hw/virtio/virtio-pci.c    |  9 ++++++++
>>   include/hw/qdev-core.h    |  3 +++
>>   qapi/qdev.json            | 23 +++++++++++++++++++
>>   system/qdev-monitor.c     | 48 +++++++++++++++++++++++++++++++++++++++
>>   5 files changed, 84 insertions(+)
>>
>> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
>> index 091d2c6acf..2f301f380c 100644
>> --- a/hw/block/vhost-user-blk.c
>> +++ b/hw/block/vhost-user-blk.c
>> @@ -588,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 b1d02f4b3d..0d91e8b5dc 100644
>> --- a/hw/virtio/virtio-pci.c
>> +++ b/hw/virtio/virtio-pci.c
>> @@ -2351,6 +2351,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);
>> @@ -2367,6 +2375,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 facaa0bc6a..fc5e125a45 100644
>> --- a/qapi/qdev.json
>> +++ b/qapi/qdev.json
>> @@ -161,3 +161,26 @@
>>   ##
>>   { 'event': 'DEVICE_UNPLUG_GUEST_ERROR',
>>     'data': { '*device': 'str', 'path': 'str' } }
>> +
>> +##
>> +# @device-sync-config:
>> +#
>> +# Synchronize device configuration from host to guest part.  First,
>> +# copy the configuration from the host part (backend) to the guest
>> +# part (frontend).  Then notify guest software that device
>> +# configuration changed.
> 
> Blank line here, please.
> 
>> +# The command may be used to notify the guest about block device
>> +# capcity change.  Currently only vhost-user-blk device supports
>> +# this.
>> +#
>> +# @id: the device's ID or QOM path
>> +#
>> +# Features:
>> +#
>> +# @unstable: The command is experimental.
>> +#
>> +# Since: 9.1
>> +##
>> +{ 'command': 'device-sync-config',
>> +  'features': [ 'unstable' ],
>> +  'data': {'id': 'str'} }
>> diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
>> index 264978aa40..47bfc0506e 100644
>> --- a/system/qdev-monitor.c
>> +++ b/system/qdev-monitor.c
>> @@ -23,6 +23,7 @@
>>   #include "monitor/monitor.h"
>>   #include "monitor/qdev.h"
>>   #include "sysemu/arch_init.h"
>> +#include "sysemu/runstate.h"
>>   #include "qapi/error.h"
>>   #include "qapi/qapi-commands-qdev.h"
>>   #include "qapi/qmp/dispatch.h"
>> @@ -971,6 +972,53 @@ 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;
>> +
>> +    /*
>> +     * During migration there is a race between syncing`configuration and
>> +     * migrating it (if migrate first, that target would get outdated version),
>> +     * so let's just not allow it.
> 
> Wrap comment lines around column 70 for legibility, please.
> 
>> +     *
>> +     * Moreover, let's not rely on setting up interrupts in paused
>> +     * state, which may be a part of migration process.
> 
> We discussed this in review of v3.  You wanted to check whether the
> problem is real.  Is it?

We discussed it later than I've sent v4 :) No, I didn't check yet.

> 
>> +     */
>> +
>> +    if (migration_is_running()) {
>> +        error_setg(errp, "Config synchronization is not allowed "
>> +                   "during migration");
>> +        return;
>> +    }
>> +
>> +    if (!runstate_is_running()) {
>> +        error_setg(errp, "Config synchronization allowed only in '%s' state, "
> 
> Suggest a line break after errp,
> 
>> +                   "current state is '%s'", RunState_str(RUN_STATE_RUNNING),
>> +                   RunState_str(runstate_get()));
>> +        return;
>> +    }
>> +
>> +    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;
>
Vladimir Sementsov-Ogievskiy May 1, 2024, 8:50 a.m. UTC | #3
On 30.04.24 11:31, Vladimir Sementsov-Ogievskiy wrote:
> On 30.04.24 11:19, 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).
>>>
>>> Command result is racy if allow it during migration. Let's allow the
>>> sync only in RUNNING state.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>>> ---
>>>   hw/block/vhost-user-blk.c |  1 +
>>>   hw/virtio/virtio-pci.c    |  9 ++++++++
>>>   include/hw/qdev-core.h    |  3 +++
>>>   qapi/qdev.json            | 23 +++++++++++++++++++
>>>   system/qdev-monitor.c     | 48 +++++++++++++++++++++++++++++++++++++++
>>>   5 files changed, 84 insertions(+)
>>>
>>> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
>>> index 091d2c6acf..2f301f380c 100644
>>> --- a/hw/block/vhost-user-blk.c
>>> +++ b/hw/block/vhost-user-blk.c
>>> @@ -588,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 b1d02f4b3d..0d91e8b5dc 100644
>>> --- a/hw/virtio/virtio-pci.c
>>> +++ b/hw/virtio/virtio-pci.c
>>> @@ -2351,6 +2351,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);
>>> @@ -2367,6 +2375,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 facaa0bc6a..fc5e125a45 100644
>>> --- a/qapi/qdev.json
>>> +++ b/qapi/qdev.json
>>> @@ -161,3 +161,26 @@
>>>   ##
>>>   { 'event': 'DEVICE_UNPLUG_GUEST_ERROR',
>>>     'data': { '*device': 'str', 'path': 'str' } }
>>> +
>>> +##
>>> +# @device-sync-config:
>>> +#
>>> +# Synchronize device configuration from host to guest part.  First,
>>> +# copy the configuration from the host part (backend) to the guest
>>> +# part (frontend).  Then notify guest software that device
>>> +# configuration changed.
>>
>> Blank line here, please.
>>
>>> +# The command may be used to notify the guest about block device
>>> +# capcity change.  Currently only vhost-user-blk device supports
>>> +# this.
>>> +#
>>> +# @id: the device's ID or QOM path
>>> +#
>>> +# Features:
>>> +#
>>> +# @unstable: The command is experimental.
>>> +#
>>> +# Since: 9.1
>>> +##
>>> +{ 'command': 'device-sync-config',
>>> +  'features': [ 'unstable' ],
>>> +  'data': {'id': 'str'} }
>>> diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
>>> index 264978aa40..47bfc0506e 100644
>>> --- a/system/qdev-monitor.c
>>> +++ b/system/qdev-monitor.c
>>> @@ -23,6 +23,7 @@
>>>   #include "monitor/monitor.h"
>>>   #include "monitor/qdev.h"
>>>   #include "sysemu/arch_init.h"
>>> +#include "sysemu/runstate.h"
>>>   #include "qapi/error.h"
>>>   #include "qapi/qapi-commands-qdev.h"
>>>   #include "qapi/qmp/dispatch.h"
>>> @@ -971,6 +972,53 @@ 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;
>>> +
>>> +    /*
>>> +     * During migration there is a race between syncing`configuration and
>>> +     * migrating it (if migrate first, that target would get outdated version),
>>> +     * so let's just not allow it.
>>
>> Wrap comment lines around column 70 for legibility, please.
>>
>>> +     *
>>> +     * Moreover, let's not rely on setting up interrupts in paused
>>> +     * state, which may be a part of migration process.
>>
>> We discussed this in review of v3.  You wanted to check whether the
>> problem is real.  Is it?
> 
> We discussed it later than I've sent v4 :) No, I didn't check yet.

Checked. Seems this works (in scheme pause -> migrate -> resume, interrupts are migrated and triggered on target after resume), so my doubt was wrong.

> 
>>
>>> +     */
>>> +
>>> +    if (migration_is_running()) {
>>> +        error_setg(errp, "Config synchronization is not allowed "
>>> +                   "during migration");
>>> +        return;
>>> +    }
>>> +
>>> +    if (!runstate_is_running()) {
>>> +        error_setg(errp, "Config synchronization allowed only in '%s' state, "
>>
>> Suggest a line break after errp,
>>
>>> +                   "current state is '%s'", RunState_str(RUN_STATE_RUNNING),
>>> +                   RunState_str(runstate_get()));
>>> +        return;
>>> +    }
>>> +
>>> +    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;
>>
>
Markus Armbruster May 2, 2024, 7:25 a.m. UTC | #4
Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:

> On 30.04.24 11:31, Vladimir Sementsov-Ogievskiy wrote:
>> On 30.04.24 11:19, 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).
>>>>
>>>> Command result is racy if allow it during migration. Let's allow the
>>>> sync only in RUNNING state.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

[...]

>>>> diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
>>>> index 264978aa40..47bfc0506e 100644
>>>> --- a/system/qdev-monitor.c
>>>> +++ b/system/qdev-monitor.c
>>>> @@ -23,6 +23,7 @@
>>>>   #include "monitor/monitor.h"
>>>>   #include "monitor/qdev.h"
>>>>   #include "sysemu/arch_init.h"
>>>> +#include "sysemu/runstate.h"
>>>>   #include "qapi/error.h"
>>>>   #include "qapi/qapi-commands-qdev.h"
>>>>   #include "qapi/qmp/dispatch.h"
>>>> @@ -971,6 +972,53 @@ 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;
>>>> +
>>>> +    /*
>>>> +     * During migration there is a race between syncing`configuration and
>>>> +     * migrating it (if migrate first, that target would get outdated version),
>>>> +     * so let's just not allow it.
>>>
>>> Wrap comment lines around column 70 for legibility, please.
>>>
>>>> +     *
>>>> +     * Moreover, let's not rely on setting up interrupts in paused
>>>> +     * state, which may be a part of migration process.
>>>
>>> We discussed this in review of v3.  You wanted to check whether the
>>> problem is real.  Is it?
>>
>> We discussed it later than I've sent v4 :) No, I didn't check yet.
>
> Checked. Seems this works (in scheme pause -> migrate -> resume, interrupts are migrated and triggered on target after resume), so my doubt was wrong.

Thanks!

[...]
diff mbox series

Patch

diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index 091d2c6acf..2f301f380c 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -588,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 b1d02f4b3d..0d91e8b5dc 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -2351,6 +2351,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);
@@ -2367,6 +2375,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 facaa0bc6a..fc5e125a45 100644
--- a/qapi/qdev.json
+++ b/qapi/qdev.json
@@ -161,3 +161,26 @@ 
 ##
 { 'event': 'DEVICE_UNPLUG_GUEST_ERROR',
   'data': { '*device': 'str', 'path': 'str' } }
+
+##
+# @device-sync-config:
+#
+# Synchronize device configuration from host to guest part.  First,
+# copy the configuration from the host part (backend) to the guest
+# part (frontend).  Then notify guest software that device
+# configuration changed.
+# The command may be used to notify the guest about block device
+# capcity change.  Currently only vhost-user-blk device supports
+# this.
+#
+# @id: the device's ID or QOM path
+#
+# Features:
+#
+# @unstable: The command is experimental.
+#
+# Since: 9.1
+##
+{ 'command': 'device-sync-config',
+  'features': [ 'unstable' ],
+  'data': {'id': 'str'} }
diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
index 264978aa40..47bfc0506e 100644
--- a/system/qdev-monitor.c
+++ b/system/qdev-monitor.c
@@ -23,6 +23,7 @@ 
 #include "monitor/monitor.h"
 #include "monitor/qdev.h"
 #include "sysemu/arch_init.h"
+#include "sysemu/runstate.h"
 #include "qapi/error.h"
 #include "qapi/qapi-commands-qdev.h"
 #include "qapi/qmp/dispatch.h"
@@ -971,6 +972,53 @@  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;
+
+    /*
+     * During migration there is a race between syncing`configuration and
+     * migrating it (if migrate first, that target would get outdated version),
+     * so let's just not allow it.
+     *
+     * Moreover, let's not rely on setting up interrupts in paused
+     * state, which may be a part of migration process.
+     */
+
+    if (migration_is_running()) {
+        error_setg(errp, "Config synchronization is not allowed "
+                   "during migration");
+        return;
+    }
+
+    if (!runstate_is_running()) {
+        error_setg(errp, "Config synchronization allowed only in '%s' state, "
+                   "current state is '%s'", RunState_str(RUN_STATE_RUNNING),
+                   RunState_str(runstate_get()));
+        return;
+    }
+
+    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;