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

[Steven Sistare]

On 12/8/2023 11:37 AM, Fabiano Rosas wrote:
> Steve Sistare <steven.sistare@oracle.com> writes:
> 
>> 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>
>> ---
>>  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;
>> +    }
>> +
>>      /* If state is running or paused, it is not needed */
>>  
>>      if (strcmp(runstate, "running") == 0 ||
>> @@ -85,6 +104,7 @@ static int global_state_post_load(void *opaque, int 
>> version_id)
>>      Error *local_err = NULL;
>>      int r;
>>      char *runstate = (char *)s->runstate;
>> +    bool vm_was_suspended = s->has_vm_was_suspended && s->vm_was_suspended;
> 
> Why do you need has_vm_was_suspended? If they're always read like this,
> then one flag should do, no?

has_vm_was_suspended=0 means a legacy qemu.  Currently the dest has no reason 
to care,
and we treat that as a vm_was_suspended=0 case, but I want to keep that field in
case we ever need to know.  But you are correct that this line can simply be:
    bool vm_was_suspended = s->vm_was_suspended;

- Steve
 
>>      s->received = true;
>>      trace_migrate_global_state_post_load(runstate);
>> @@ -93,7 +113,7 @@ static int global_state_post_load(void *opaque, int 
>> version_id)
>>                  sizeof(s->runstate)) == sizeof(s->runstate)) {
>>          /*
>>           * This condition should never happen during migration, because
>> -         * all runstate names are shorter than 100 bytes (the size of
>> +         * all runstate names are shorter than 32 bytes (the size of
>>           * s->runstate). However, a malicious stream could overflow
>>           * the qapi_enum_parse() call, so we force the last character
>>           * to a NUL byte.
>> @@ -110,6 +130,14 @@ static int global_state_post_load(void *opaque, int 
>> version_id)
>>      }
>>      s->state = r;
>>  
>> +    /*
>> +     * global_state is saved on the outgoing side before forcing a stopped
>> +     * state, so it may have saved state=suspended and vm_was_suspended=0.
>> +     * Now we are in a paused state, and when we later call vm_start, it 
>> must
>> +     * restore the suspended state, so we must set vm_was_suspended=1 here.
>> +     */
>> +    vm_set_suspended(vm_was_suspended || r == RUN_STATE_SUSPENDED);
>> +
>>      return 0;
>>  }
>>  
>> @@ -134,6 +162,9 @@ static const VMStateDescription vmstate_globalstate = {
>>      .fields = (VMStateField[]) {
>>          VMSTATE_UINT32(size, GlobalState),
>>          VMSTATE_BUFFER(runstate, GlobalState),
>> +        VMSTATE_UINT8(has_vm_was_suspended, GlobalState),
>> +        VMSTATE_UINT8(vm_was_suspended, GlobalState),
>> +        VMSTATE_BUFFER(unused, GlobalState),
>>          VMSTATE_END_OF_LIST()
>>      },
>>  };