On 9/28/26 8:39 PM, Akihiko Odaki wrote:
> On 2026/09/28 23:54, Fabiano Rosas wrote:
>> Andrey Drobyshev <[email protected]> writes:
>>
>>> The savevm/loadvm operation doesn't make any sense in any of the CPR
>>> modes. Saving VM state implies saving its RAM entirely, while the whole
>>> point of CPR is avoiding that: in cpr-transfer and cpr-exec modes RAM is
>>> preserved in place and never put into the migration stream (see
>>> migrate_ram_is_ignored() in migration/ram.c). As a result, a snapshot
>>> taken with such a mode set has no RAM in it, and loading a snapshot with
>>> such a mode set fails in the RAM loader.
>>>
>>> Currently nothing stops these operations from firing with a CPR mode
>>> still set, e.g. after an earlier CPR attempt. Reject them early in
>>> migrate_can_snapshot(), which guards 'savevm'/'loadvm' HMP commands as
>>> well as 'snapshot-save'/'snapshot-load' QMP commands.
>>>
>>> Signed-off-by: Andrey Drobyshev <[email protected]>
>>> ---
>>> migration/options.c | 7 +++++++
>>> 1 file changed, 7 insertions(+)
>>>
>>> diff --git a/migration/options.c b/migration/options.c
>>> index 05b026f4bf2..f648be1302b 100644
>>> --- a/migration/options.c
>>> +++ b/migration/options.c
>>> @@ -555,8 +555,15 @@ static bool migrate_incoming_started(void)
>>> bool migrate_can_snapshot(Error **errp)
>>> {
>>> MigrationState *s = migrate_get_current();
>>> + MigMode mode = migrate_mode();
>>> int i;
>>>
>>> + if (mode != MIG_MODE_NORMAL) {
>>> + error_setg(errp, "Snapshots are not compatible with migration mode
>>> %s",
>>> + MigMode_str(mode));
>>> + return false;
>>> + }
>>> +
>>> for (i = 0; i < check_caps_savevm.size; i++) {
>>> int incomp_cap = check_caps_savevm.caps[i];
>>
>> +CC Akihiko Odaki
>>
>> Reviewed-by: Fabiano Rosas <[email protected]>
>
> Thanks for CCing me. This is also relevant to my recent work on Xen
> migration. The snapshot RAM-saving path does not support Xen's memory
> mappings, so migrate_can_snapshot() should reject Xen as well.
>
> If you send another revision, could you include that check? This is a
> pre-existing issue, though, so it need not hold up this patch.
>
> Reviewed-by: Akihiko Odaki <[email protected]>
>
> Regards,
> Akihiko Odaki
Hello Akihiko,
Sure, I can add the check. It's of course slightly beyond the scope of
this series, but since the patch is trivial there shouldn't be any
problems. IIUC adding this should be enough:
if (xen_enabled()) {
error_setg(errp, "Snapshots are not compatible with Xen");
return false;
}
I'll make it a separate patch on the next respin.
Thanks,
Andrey
