On 12/11/2023 1:46 AM, Peter Xu wrote:
> On Wed, Dec 06, 2023 at 09:23:30AM -0800, Steve Sistare wrote:
>> If the outgoing machine was previously suspended, propagate that to the
>> incoming side via global_state, so a subsequent vm_start restores the
>> suspended state. To maintain backward and forward compatibility, reclaim
>> some space from the runstate member.
>>
>> Signed-off-by: Steve Sistare <steven.sist...@oracle.com>
>
> Reviewed-by: Peter Xu <pet...@redhat.com>
>
> One nitpick below.
>
>> ---
>> migration/global_state.c | 35 +++++++++++++++++++++++++++++++++--
>> 1 file changed, 33 insertions(+), 2 deletions(-)
>>
>> diff --git a/migration/global_state.c b/migration/global_state.c
>> index 4e2a9d8..d4f61a1 100644
>> --- a/migration/global_state.c
>> +++ b/migration/global_state.c
>> @@ -22,7 +22,16 @@
>>
>> typedef struct {
>> uint32_t size;
>> - uint8_t runstate[100];
>> +
>> + /*
>> + * runstate was 100 bytes, zero padded, but we trimmed it to add a
>> + * few fields and maintain backwards compatibility.
>> + */
>> + uint8_t runstate[32];
>> + uint8_t has_vm_was_suspended;
>> + uint8_t vm_was_suspended;
>> + uint8_t unused[66];
>> +
>> RunState state;
>> bool received;
>> } GlobalState;
>> @@ -35,6 +44,10 @@ static void global_state_do_store(RunState state)
>> assert(strlen(state_str) < sizeof(global_state.runstate));
>> strpadcpy((char *)global_state.runstate, sizeof(global_state.runstate),
>> state_str, '\0');
>> + global_state.has_vm_was_suspended = true;
>> + global_state.vm_was_suspended = vm_get_suspended();
>> +
>> + memset(global_state.unused, 0, sizeof(global_state.unused));
>> }
>>
>> void global_state_store(void)
>> @@ -68,6 +81,12 @@ static bool global_state_needed(void *opaque)
>> return true;
>> }
>>
>> + /* If the suspended state must be remembered, it is needed */
>> +
>> + if (vm_get_suspended()) {
>> + return true;
>> + }
>
> Can we drop this section?
>
> I felt unsafe when QEMU can overwrite the option even if user explicitly
> specified store-global-state=off but we still send this.. Ideally I think
> it's better if it's as simple as:
>
> static bool global_state_needed(void *opaque)
> {
> return migrate_get_current()->store_global_state;
> }
I agree, I also did not see the point of dropping global_state for some states.
I will simplify it to this.
- Steve
> I don't think we can remove the old trick due to compatibility reasons, but
> maybe nice to not add new ones to make this section more unpredictable in
> the migration stream?
>
> IMHO it shouldn't matter in reality for the current use case even dropping
> it, as I don't expect any non-Xen QEMU VMs migrates without having the
> option turned on (store-global-state=on) after QEMU 2.4.
>
> Thanks,
>
