Re: [PATCH V7 05/12] migration: propagate suspended runstate

[Steven Sistare <steven.sistare@oracle.com>]

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.sistare@oracle.com>
> 
> Reviewed-by: Peter Xu <peterx@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,
>