Vladimir Sementsov-Ogievskiy <vsement...@yandex-team.ru> writes:
> On 24.04.24 14:48, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsement...@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 <vsement...@yandex-team.ru>
[...]
>>> diff --git a/include/sysemu/runstate.h b/include/sysemu/runstate.h
>>> index 0117d243c4..296af52322 100644
>>> --- a/include/sysemu/runstate.h
>>> +++ b/include/sysemu/runstate.h
>>> @@ -5,6 +5,7 @@
>>> #include "qemu/notify.h"
>>>
>>> bool runstate_check(RunState state);
>>> +const char *current_run_state_str(void);
>>> void runstate_set(RunState new_state);
>>> RunState runstate_get(void);
>>> bool runstate_is_running(void);
>>> diff --git a/qapi/qdev.json b/qapi/qdev.json
>>> index facaa0bc6a..e8be79c3d5 100644
>>> --- a/qapi/qdev.json
>>> +++ b/qapi/qdev.json
>>> @@ -161,3 +161,24 @@
>>> ##
>>> { 'event': 'DEVICE_UNPLUG_GUEST_ERROR',
>>> 'data': { '*device': 'str', 'path': 'str' } }
>>> +
>>> +##
>>> +# @device-sync-config:
>>> +#
>>> +# Synchronize config from backend to the guest. The command notifies
>>> +# re-read the device config from the backend and notifies the guest
>>> +# to re-read the config. The command may be used to notify the guest
>>> +# about block device capcity change. Currently only vhost-user-blk
>>> +# device supports this.
>>
>> I'm not sure I understand this. To work towards an understanding, I
>> rephrase it, and you point out the errors.
>>
>> 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.
>
> Correct, thanks
Perhaps
Synchronize guest-visible device configuration with the backend's
configuration, and notify guest software that device configuration
changed.
This may be useful to notify the guest of a block device capacity
change. Currenrly, only vhost-user-blk devices support this.
Next question: what happens when the device *doesn't* support this?
>> I wonder how configuration can get out of sync. Can you explain?
>>
>
> The example (and the original feature, which triggered developing this) is
> vhost disk resize. If vhost-server (backend) doesn't support
> VHOST_USER_SLAVE_CONFIG_CHANGE_MSG, neither QEMU nor guest will know that
> disk capacity changed.
Sounds like we wouldn't need this command if we could make the
vhost-server support VHOST_USER_SLAVE_CONFIG_CHANGE_MSG. Is making it
support it impractical? Or are there other uses for this command?
>>> +#
>>> +# @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 7e075d91c1..cb35ea0b86 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"
>>> @@ -969,6 +970,52 @@ 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`config and
>>> + * migrating it, so let's just not allow it.
>>
>> Can you briefly explain the race?
>
> If at the moment of qmp command, corresponding config already migrated to the
> target, we'll change only the config on source, but on the target we'll still
> have outdated config.
For RAM, dirty tracking ensures the change gets sent. But this is
device memory. Correct?
>>> + *
>>> + * Moreover, let's not rely on setting up interrupts in paused
>>> + * state, which may be a part of migration process.
>>
>> What dependence exactly are you avoiding? Config synchronization
>> depending on guest interrupt delivery?
>
> Right, guest is notified by pci_set_irq.
If we allowed it in paused state, the delivery of the interrupt would be
delayed until the guest resumes running. Correct?
>>> + */
>>> +
>>> + if (migration_is_running()) {
>>> + error_setg(errp, "Config synchronization is not allowed "
>>> + "during migration.");
>>
>> qapi/error.h:
>>
>> * The resulting message should be a single phrase, with no newline or
>> * trailing punctuation.
>>
>> Drop the period, please.
>
> Will do
>
>>
>>> + return;
>>> + }
>>> +
>>> + if (!runstate_is_running()) {
>>> + error_setg(errp, "Config synchronization allowed only in '%s'
>>> state, "
>>> + "current state is '%s'",
>>> RunState_str(RUN_STATE_RUNNING),
>>> + current_run_state_str());
>>> + 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;
[...]
