Re: [PATCH v16 QEMU 08/16] vfio: Register SaveVMHandlers for VFIO device

[Kirti Wankhede]

On 4/1/2020 11:06 PM, Dr. David Alan Gilbert wrote:
> * Kirti Wankhede (address@hidden) wrote:
> > Define flags to be used as delimeter in migration file stream.
> > Added .save_setup and .save_cleanup functions. Mapped & unmapped migration
> > region from these functions at source during saving or pre-copy phase.
> > Set VFIO device state depending on VM's state. During live migration, VM is
> > running when .save_setup is called, _SAVING | _RUNNING state is set for VFIO
> > device. During save-restore, VM is paused, _SAVING state is set for VFIO device.
> >
> > Signed-off-by: Kirti Wankhede <address@hidden>
> > Reviewed-by: Neo Jia <address@hidden>
> > ---
> >   hw/vfio/migration.c  | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >   hw/vfio/trace-events |  2 ++
> >   2 files changed, 78 insertions(+)
> >
> > diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> > index 22ded9d28cf3..033f76526e49 100644
> > --- a/hw/vfio/migration.c
> > +++ b/hw/vfio/migration.c
> > @@ -8,6 +8,7 @@
> >    */
> >   
> >   #include "qemu/osdep.h"
> > +#include "qemu/main-loop.h"
> >   #include <linux/vfio.h>
> >   
> >   #include "sysemu/runstate.h"
> > @@ -24,6 +25,17 @@
> >   #include "pci.h"
> >   #include "trace.h"
> >   
> > +/*
> > + * Flags used as delimiter:
> > + * 0xffffffff => MSB 32-bit all 1s
> > + * 0xef10     => emulated (virtual) function IO
> > + * 0x0000     => 16-bits reserved for flags
> > + */
> > +#define VFIO_MIG_FLAG_END_OF_STATE      (0xffffffffef100001ULL)
> > +#define VFIO_MIG_FLAG_DEV_CONFIG_STATE  (0xffffffffef100002ULL)
> > +#define VFIO_MIG_FLAG_DEV_SETUP_STATE   (0xffffffffef100003ULL)
> > +#define VFIO_MIG_FLAG_DEV_DATA_STATE    (0xffffffffef100004ULL)
> > +
> >   static void vfio_migration_region_exit(VFIODevice *vbasedev)
> >   {
> >       VFIOMigration *migration = vbasedev->migration;
> > @@ -126,6 +138,69 @@ static int vfio_migration_set_state(VFIODevice *vbasedev, 
> > uint32_t mask,
> >       return 0;
> >   }
> >   
> > +/* ---------------------------------------------------------------------- */
> > +
> > +static int vfio_save_setup(QEMUFile *f, void *opaque)
> > +{
> > +    VFIODevice *vbasedev = opaque;
> > +    VFIOMigration *migration = vbasedev->migration;
> > +    int ret;
> > +
> > +    qemu_put_be64(f, VFIO_MIG_FLAG_DEV_SETUP_STATE);
> > +
> > +    if (migration->region.mmaps) {
> > +        qemu_mutex_lock_iothread();
> > +        ret = vfio_region_mmap(&migration->region);
> > +        qemu_mutex_unlock_iothread();
> > +        if (ret) {
> > +            error_report("%s: Failed to mmap VFIO migration region %d: %s",
> > +                         vbasedev->name, migration->region.index,
> > +                         strerror(-ret));
> > +            return ret;
> > +        }
> > +    }
> > +
> > +    ret = vfio_migration_set_state(vbasedev, ~0, VFIO_DEVICE_STATE_SAVING);
> > +    if (ret) {
> > +        error_report("%s: Failed to set state SAVING", vbasedev->name);
> > +        return ret;
> > +    }
> > +
> > +    /*
> > +     * Save migration region size. This is used to verify migration region size
> > +     * is greater than or equal to migration region size at destination
> > +     */
> > +    qemu_put_be64(f, migration->region.size);
> > +
> > +    qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE);
>
> OK, good, so now we can change that to something else if you want to
> migrate something extra in the future.
>
> > +    ret = qemu_file_get_error(f);
> > +    if (ret) {
> > +        return ret;
> > +    }
> > +
> > +    trace_vfio_save_setup(vbasedev->name);
>
> I'd put that trace at the start of the function.
>
> > +    return 0;
> > +}
> > +
> > +static void vfio_save_cleanup(void *opaque)
> > +{
> > +    VFIODevice *vbasedev = opaque;
> > +    VFIOMigration *migration = vbasedev->migration;
> > +
> > +    if (migration->region.mmaps) {
> > +        vfio_region_unmap(&migration->region);
> > +    }
> > +    trace_vfio_save_cleanup(vbasedev->name);
> > +}
> > +
> > +static SaveVMHandlers savevm_vfio_handlers = {
> > +    .save_setup = vfio_save_setup,
> > +    .save_cleanup = vfio_save_cleanup,
> > +};
> > +
> > +/* ---------------------------------------------------------------------- */
> > +
> >   static void vfio_vmstate_change(void *opaque, int running, RunState state)
> >   {
> >       VFIODevice *vbasedev = opaque;
> > @@ -191,6 +266,7 @@ static int vfio_migration_init(VFIODevice *vbasedev,
> >           return ret;
> >       }
> >   
> > +    register_savevm_live("vfio", -1, 1, &savevm_vfio_handlers, vbasedev);
>
> That doesn't look right to me;  firstly the -1 should now be
> VMSTATE_INSTANCE_ID_ANY - after the recent change in commit 1df2c9a
>
> Have you tried this with two vfio devices?

Yes. And it works with multiple vfio devices.

Thanks,
Kirti

> This is quite rare - it's an iterative device that can have
> multiple instances;  if you look at 'ram' for example, all the RAM
> instances are handled inside the save_setup/save for the one instance of
> 'ram'.  I think here you're trying to register an individual vfio
> device, so if you had multiple devices you'd see this called twice.
>
> So either you need to make vfio_save_* do all of the devices in a loop -
> which feels like a bad idea;  or replace "vfio" in that call by a unique
> device name;  as long as your device has a bus path then you should be
> able to use the same trick vmstate_register_with_alias_id does, and use
> I think,  vmstate_if_get_id(VMSTAETE_IF(vbasedev)).
>
> but it might take some experimentation since this is an odd use.
>
> >       vbasedev->vm_state = qemu_add_vm_change_state_handler(vfio_vmstate_change,
> >                                                             vbasedev);
> >   
> > diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events
> > index 69503228f20e..4bb43f18f315 100644
> > --- a/hw/vfio/trace-events
> > +++ b/hw/vfio/trace-events
> > @@ -149,3 +149,5 @@ vfio_migration_probe(char *name, uint32_t index) " (%s) Region 
> > %d"
> >   vfio_migration_set_state(char *name, uint32_t state) " (%s) state %d"
> >   vfio_vmstate_change(char *name, int running, const char *reason, uint32_t dev_state) 
> > " (%s) running %d reason %s device state %d"
> >   vfio_migration_state_notifier(char *name, int state) " (%s) state %d"
> > +vfio_save_setup(char *name) " (%s)"
> > +vfio_save_cleanup(char *name) " (%s)"
> > --
> > 2.7.0
> --
> Dr. David Alan Gilbert / address@hidden / Manchester, UK