[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v4 3/3] qapi: introduce device-sync-config
From: |
Markus Armbruster |
Subject: |
Re: [PATCH v4 3/3] qapi: introduce device-sync-config |
Date: |
Thu, 02 May 2024 09:25:26 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) |
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!
[...]