Re: [PATCH] device-assignment: Register as un-migratable

[Alex Williamson <alex.williamson@redhat.com>]

On Mon, 2010-11-15 at 23:04 +0100, Jan Kiszka wrote:
> Am 15.11.2010 21:25, Alex Williamson wrote:
> > On Mon, 2010-11-15 at 21:05 +0100, Jan Kiszka wrote:
> >> Am 15.11.2010 20:41, Alex Williamson wrote:
> >>> Use register_device_unmigratable() to declare ourselves as
> >>> non-migratable.
> >>>
> >>> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> >>> ---
> >>>
> >>>  hw/device-assignment.c |   15 +++++++++++++++
> >>>  1 files changed, 15 insertions(+), 0 deletions(-)
> >>>
> >>> diff --git a/hw/device-assignment.c b/hw/device-assignment.c
> >>> index bde231d..cd93941 100644
> >>> --- a/hw/device-assignment.c
> >>> +++ b/hw/device-assignment.c
> >>> @@ -1434,6 +1434,13 @@ static void assigned_dev_unregister_msix_mmio(AssignedDevice *dev)
> >>>      dev->msix_table_page = NULL;
> >>>  }
> >>>  
> >>> +/* This should never get called, but we're required to create a save_state
> >>> + * handler or else the no_migrate flag will never be checked. */
> >>> +static void assigned_save(QEMUFile* f, void *opaque)
> >>> +{
> >>> +    abort();
> >>> +}
> >>> +
> >>>  static int assigned_initfn(struct PCIDevice *pci_dev)
> >>>  {
> >>>      AssignedDevice *dev = DO_UPCAST(AssignedDevice, dev, pci_dev);
> >>> @@ -1490,6 +1497,13 @@ static int assigned_initfn(struct PCIDevice *pci_dev)
> >>>  
> >>>      assigned_dev_load_option_rom(dev);
> >>>      QLIST_INSERT_HEAD(&devs, dev, next);
> >>> +
> >>> +    /* Assigned devices are not migratable, register a save
> >>> +     * state entry so that we can mark it unmigratable. */
> >>> +    register_savevm(&dev->dev.qdev, "pci-assign", 0, 0,
> >>> +                    assigned_save, NULL, dev);
> >>> +    register_device_unmigratable(&dev->dev.qdev, "pci-assign", dev);
> >>> +
> >>
> >> Isn't this expressible via some VMStateDescription? If not, that should
> >> be changed first.
> > 
> > Nope, save state handlers aren't allowed to fail.  I tried to fix it:
> > 
> > http://lists.nongnu.org/archive/html/qemu-devel/2010-11/msg00417.html
> > 
> > (you can find more discussion in other branches of that subject)  I've
> > succumbed to not getting that series in, so now I'm just trying to use
> > the code as it exists.  Thanks,
> 
> Hmm, didn't get why you need that series for the purpose of no_migration
> declaration.
>  My point is:

I was hoping you were going down the path I started, that we don't need
to special case non-migratable devices if we just allow save to return
an error.

>  My point is:
> 
> struct VMStateDescription {
>     const char *name;
>     int version_id;
>     int minimum_version_id;
>     int minimum_version_id_old;
>     int no_migrate; /* or 'flags' */
>     ...
> 
> so that you can specify an empty vmstate with that flag set, and you do
> not need to register/unregister things via to-be-deprecated service
> calls. Or am I missing some subtle detail?

We don't seem to be enforcing that new drivers should use vmsd vs the
old style handling and there are some drivers that are currently too
complicated for vmsd to handle, which means no_migrate has to be
registered on the save state entry, not the vmsd.  So I could create a
dummy vmsd, then call register_device_unmigratable, to achieve roughly
the same effect.  Six of one, half dozen of the other... I can switch to
a vmsd dummy save if it's preferred.  Thanks,

Alex