Re: [PATCH v9 10/14] vfio/migration: Implement VFIO migration protocol v2

[Avihai Horon]

On 08/02/2023 1:42, Alex Williamson wrote:
> External email: Use caution opening links or attachments
>
>
> On Mon, 6 Feb 2023 14:31:33 +0200
> Avihai Horon <avihaih@nvidia.com> wrote:
> > @@ -523,6 +745,41 @@ static int vfio_save_iterate(QEMUFile *f, void *opaque)
> >       return 0;
> >   }
> >
> > +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 only */
> > +    recover_state = VFIO_DEVICE_STATE_STOP;
> Why do we need to put this in a local variable?

No need. I will remove the local variable.

> > +    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;
> > +    }
> > +
> > +    recover_state = VFIO_DEVICE_STATE_ERROR;
> IIRC, the ERROR state is not reachable as a user directed state.  I
> suppose passing it as the recovery state guarantees a device reset when
> it fails, but if that's the intention it should be documented with a
> comment to explain so (and vfio_migration_set_state() should not bother
> trying to use it as a recovery state).

Right, that's the intention.

I will add a comment and adjust vfio_migration_set_state().

> > +    ret = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_STOP,
> > +                                   recover_state);
> > +    trace_vfio_save_complete_precopy(vbasedev->name, ret);
> > +
> > +    return ret;
> > +}
> > +
> >   static int vfio_v1_save_complete_precopy(QEMUFile *f, void *opaque)
> >   {
> >       VFIODevice *vbasedev = opaque;
> ...
> > @@ -769,12 +1087,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);
> Same here.

Will change.

Thanks.