Re: [PATCH v2 07/11] vfio/migration: Implement VFIO migration protocol v2

[Joao Martins]

On 6/14/22 17:34, Avihai Horon wrote:
> 
> On 6/14/2022 2:08 PM, Joao Martins wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> On 5/30/22 18:07, Avihai Horon wrote:
>>> +static int vfio_save_complete_precopy(QEMUFile *f, void *opaque)
>>> +{
>>> +    VFIODevice *vbasedev = opaque;
>>> +    enum vfio_device_mig_state recover_state;
>>> +    int ret;
>>> +
>>> +    /* We reach here with device state STOP or STOP_COPY only */
>>> +    recover_state = VFIO_DEVICE_STATE_STOP;
>>> +    ret = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_STOP_COPY,
>>> +                                   recover_state);
>>> +    if (ret) {
>>> +        return ret;
>>> +    }
>>> +
>>> +    do {
>>> +        ret = vfio_save_block(f, vbasedev->migration);
>>> +        if (ret < 0) {
>>> +            return ret;
>>> +        }
>>> +    } while (!ret);
>>> +
>>> +    qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE);
>>> +    ret = qemu_file_get_error(f);
>>> +    if (ret) {
>>> +        return ret;
>>> +    }
>>> +
>>> +    ret = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_STOP,
>>> +                                   recover_state);
>> Is it expected that you are setting VFIO_DEVICE_STATE_STOP while
>> @recover_state is the same value (VFIO_DEVICE_STATE_STOP) ?
> 
> 
> Yes.
> Transitioning to any other state from STOP_COPY will first go through 
> STOP state (this is done internally by kernel).
> So there is no better option for the recover state but STOP.
> 
I was think about ERROR state given that you can transition there
from any state, but wasn't quite sure if it's appropriate to make that arc
while in stop copy migration phase.

>>> +    if (ret) {
>>> +        return ret;
>>> +    }
>>> +
>>> +    trace_vfio_save_complete_precopy(vbasedev->name);
>>> +
>>> +    return 0;

just a cosmetic nit: you could probably rewrite these last couple of lines as:

        if (!ret) {
            trace_vfio_save_complete_precopy(vbasedev->name);
        }

        return ret;

Let's you avoid the double return path.

>>> +}
>>> +
>>>   static int vfio_v1_save_complete_precopy(QEMUFile *f, void *opaque)
>>>   {
>>>       VFIODevice *vbasedev = opaque;
>>> @@ -593,6 +775,14 @@ static void vfio_save_state(QEMUFile *f, void *opaque)
>>>       }
>>>   }
>>>
>>> +static int vfio_load_setup(QEMUFile *f, void *opaque)
>>> +{
>>> +    VFIODevice *vbasedev = opaque;
>>> +
>>> +    return vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_RESUMING,
>>> +                                   vbasedev->migration->device_state);
>>> +}
>>> +
>>>   static int vfio_v1_load_setup(QEMUFile *f, void *opaque)
>>>   {
>>>       VFIODevice *vbasedev = opaque;
>>> @@ -620,6 +810,15 @@ static int vfio_v1_load_setup(QEMUFile *f, void 
>>> *opaque)
>>>       return ret;
>>>   }
>>>
>>> +static int vfio_load_cleanup(void *opaque)
>>> +{
>>> +    VFIODevice *vbasedev = opaque;
>>> +
>>> +    vfio_migration_cleanup(vbasedev);
>>> +    trace_vfio_load_cleanup(vbasedev->name);
>>> +    return 0;
>>> +}
>>> +
>>>   static int vfio_v1_load_cleanup(void *opaque)
>>>   {
>>>       VFIODevice *vbasedev = opaque;
>>> @@ -662,7 +861,11 @@ static int vfio_load_state(QEMUFile *f, void *opaque, 
>>> int version_id)
>>>               uint64_t data_size = qemu_get_be64(f);
>>>
>>>               if (data_size) {
>>> -                ret = vfio_v1_load_buffer(f, vbasedev, data_size);
>>> +                if (vbasedev->migration->v2) {
>>> +                    ret = vfio_load_buffer(f, vbasedev, data_size);
>>> +                } else {
>>> +                    ret = vfio_v1_load_buffer(f, vbasedev, data_size);
>>> +                }
>>>                   if (ret < 0) {
>>>                       return ret;
>>>                   }
>>> @@ -683,6 +886,16 @@ static int vfio_load_state(QEMUFile *f, void *opaque, 
>>> int version_id)
>>>       return ret;
>>>   }
>>>
>>> +static SaveVMHandlers savevm_vfio_handlers = {
>>> +    .save_setup = vfio_save_setup,
>>> +    .save_cleanup = vfio_save_cleanup,
>>> +    .save_live_complete_precopy = vfio_save_complete_precopy,
>>> +    .save_state = vfio_save_state,
>>> +    .load_setup = vfio_load_setup,
>>> +    .load_cleanup = vfio_load_cleanup,
>>> +    .load_state = vfio_load_state,
>>> +};
>>> +
>>>   static SaveVMHandlers savevm_vfio_v1_handlers = {
>>>       .save_setup = vfio_v1_save_setup,
>>>       .save_cleanup = vfio_v1_save_cleanup,
>>> @@ -697,6 +910,34 @@ static SaveVMHandlers savevm_vfio_v1_handlers = {
>>>
>>>   /* ---------------------------------------------------------------------- 
>>> */
>>>
>>> +static void vfio_vmstate_change(void *opaque, bool running, RunState state)
>>> +{
>>> +    VFIODevice *vbasedev = opaque;
>>> +    enum vfio_device_mig_state new_state;
>>> +    int ret;
>>> +
>>> +    if (running) {
>>> +        new_state = VFIO_DEVICE_STATE_RUNNING;
>>> +    } else {
>>> +        new_state = VFIO_DEVICE_STATE_STOP;
>>> +    }
>>> +
>>> +    ret = vfio_migration_set_state(vbasedev, new_state,
>>> +                                   VFIO_DEVICE_STATE_ERROR);
>>> +    if (ret) {
>>> +        /*
>>> +         * Migration should be aborted in this case, but vm_state_notify()
>>> +         * currently does not support reporting failures.
>>> +         */
>>> +        if (migrate_get_current()->to_dst_file) {
>>> +            qemu_file_set_error(migrate_get_current()->to_dst_file, ret);
>>> +        }
>>> +    }
>>> +
>>> +    trace_vfio_vmstate_change(vbasedev->name, running, RunState_str(state),
>>> +                              new_state);
>>> +}
>>> +
>>>   static void vfio_v1_vmstate_change(void *opaque, bool running, RunState 
>>> state)
>>>   {
>>>       VFIODevice *vbasedev = opaque;
>>> @@ -770,12 +1011,17 @@ static void vfio_migration_state_notifier(Notifier 
>>> *notifier, void *data)
>>>       case MIGRATION_STATUS_CANCELLED:
>>>       case MIGRATION_STATUS_FAILED:
>>>           bytes_transferred = 0;
>>> -        ret = vfio_migration_v1_set_state(vbasedev,
>>> -                                          ~(VFIO_DEVICE_STATE_V1_SAVING |
>>> -                                            VFIO_DEVICE_STATE_V1_RESUMING),
>>> -                                          VFIO_DEVICE_STATE_V1_RUNNING);
>>> -        if (ret) {
>>> -            error_report("%s: Failed to set state RUNNING", 
>>> vbasedev->name);
>>> +        if (migration->v2) {
>>> +            vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_RUNNING,
>>> +                                     VFIO_DEVICE_STATE_ERROR);
>> Perhaps you are discarding the error?
>>
>> Shouldn't it be:
>>
>>          err =  vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_RUNNING,
>>                                          VFIO_DEVICE_STATE_ERROR);
>>
>>> +        } else {
>>> +            ret = vfio_migration_v1_set_state(vbasedev,
>>> +                                              
>>> ~(VFIO_DEVICE_STATE_V1_SAVING |
>>> +                                                
>>> VFIO_DEVICE_STATE_V1_RESUMING),
>>> +                                              
>>> VFIO_DEVICE_STATE_V1_RUNNING);
>>> +            if (ret) {
>>> +                error_report("%s: Failed to set state RUNNING", 
>>> vbasedev->name);
>>> +            }
>> Perhaps this error_report and condition is in the wrong scope?
>>
>> Shouldn't it be more like this:
>>
>> if (migration->v2) {
>>          ret = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_RUNNING,
>>                                   VFIO_DEVICE_STATE_ERROR);
>> } else {
>>          ret = vfio_migration_v1_set_state(vbasedev,
>>                                            ~(VFIO_DEVICE_STATE_V1_SAVING |
>>                                              VFIO_DEVICE_STATE_V1_RESUMING),
>>                                            VFIO_DEVICE_STATE_V1_RUNNING);
>> }
>>
>>
>> if (ret) {
>>      error_report("%s: Failed to set state RUNNING", vbasedev->name);
>> }
> 
> 
> It was intentionally discarded.
> The return value is used by v1 code to determine whether to print an 
> error message or not.
> In v2 code the error message print is done inside 
> vfio_migration_set_state(), so there is no
> need for the return value here.
> 
Oh yes, I forgot that other print.