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

[Markus Armbruster <armbru@redhat.com>]

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!

[...]