Re: [PATCH RFC] vfio: Move the saving of the config space to the right place in VFIO migration

[Alex Williamson]

On Thu, 19 Nov 2020 14:13:24 +0530
Kirti Wankhede <kwankhede@nvidia.com> wrote:

> On 11/14/2020 2:47 PM, Shenming Lu wrote:
> > When running VFIO migration, I found that the restoring of VFIO PCI device’s
> > config space is before VGIC on ARM64 target. But generally, interrupt 
> > controllers
> > need to be restored before PCI devices.   
> 
> Is there any other way by which VGIC can be restored before PCI device?
> 
> > Besides, if a VFIO PCI device is
> > configured to have directly-injected MSIs (VLPIs), the restoring of its 
> > config
> > space will trigger the configuring of these VLPIs (in kernel), where it 
> > would
> > return an error as I saw due to the dependency on kvm’s vgic.
> >   
> 
> Can this be fixed in kernel to re-initialize the kernel state?
> 
> > To avoid this, we can move the saving of the config space from the iterable
> > process to the non-iterable process, so that it will be called after VGIC
> > according to their priorities.
> >   
> 
> With this change, at resume side, pre-copy phase data would reach 
> destination without restored config space. VFIO device on destination 
> might need it's config space setup and validated before it can accept 
> further VFIO device specific migration state.
> 
> This also changes bit-stream, so it would break migration with original 
> migration patch-set.

Config space can continue to change while in pre-copy, if we're only
sending config space at the initiation of pre-copy, how are any changes
that might occur before the VM is stopped conveyed to the target?  For
example the guest might reboot and a device returned to INTx mode from
MSI during pre-copy.  Thanks,

Alex


> > Signed-off-by: Shenming Lu <lushenming@huawei.com>
> > ---
> >   hw/vfio/migration.c | 22 ++++++----------------
> >   1 file changed, 6 insertions(+), 16 deletions(-)
> > 
> > diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> > index 3ce285ea39..028da35a25 100644
> > --- a/hw/vfio/migration.c
> > +++ b/hw/vfio/migration.c
> > @@ -351,7 +351,7 @@ static int vfio_update_pending(VFIODevice *vbasedev)
> >       return 0;
> >   }
> >   
> > -static int vfio_save_device_config_state(QEMUFile *f, void *opaque)
> > +static void vfio_save_device_config_state(QEMUFile *f, void *opaque)
> >   {
> >       VFIODevice *vbasedev = opaque;
> >   
> > @@ -365,13 +365,14 @@ static int vfio_save_device_config_state(QEMUFile *f, 
> > void *opaque)
> >   
> >       trace_vfio_save_device_config_state(vbasedev->name);
> >   
> > -    return qemu_file_get_error(f);
> > +    if (qemu_file_get_error(f))
> > +        error_report("%s: Failed to save device config space",
> > +                     vbasedev->name);
> >   }
> >   
> >   static int vfio_load_device_config_state(QEMUFile *f, void *opaque)
> >   {
> >       VFIODevice *vbasedev = opaque;
> > -    uint64_t data;
> >   
> >       if (vbasedev->ops && vbasedev->ops->vfio_load_config) {
> >           int ret;
> > @@ -384,15 +385,8 @@ static int vfio_load_device_config_state(QEMUFile *f, 
> > void *opaque)
> >           }
> >       }
> >   
> > -    data = qemu_get_be64(f);
> > -    if (data != VFIO_MIG_FLAG_END_OF_STATE) {
> > -        error_report("%s: Failed loading device config space, "
> > -                     "end flag incorrect 0x%"PRIx64, vbasedev->name, data);
> > -        return -EINVAL;
> > -    }
> > -
> >       trace_vfio_load_device_config_state(vbasedev->name);
> > -    return qemu_file_get_error(f);
> > +    return 0;
> >   }
> >   
> >   static int vfio_set_dirty_page_tracking(VFIODevice *vbasedev, bool start)
> > @@ -575,11 +569,6 @@ static int vfio_save_complete_precopy(QEMUFile *f, 
> > void *opaque)
> >           return ret;
> >       }
> >   
> > -    ret = vfio_save_device_config_state(f, opaque);
> > -    if (ret) {
> > -        return ret;
> > -    }
> > -
> >       ret = vfio_update_pending(vbasedev);
> >       if (ret) {
> >           return ret;
> > @@ -720,6 +709,7 @@ static SaveVMHandlers savevm_vfio_handlers = {
> >       .save_live_pending = vfio_save_pending,
> >       .save_live_iterate = vfio_save_iterate,
> >       .save_live_complete_precopy = vfio_save_complete_precopy,
> > +    .save_state = vfio_save_device_config_state,
> >       .load_setup = vfio_load_setup,
> >       .load_cleanup = vfio_load_cleanup,
> >       .load_state = vfio_load_state,
> >   
>