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

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

[Alex Williamson]

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);
+
     return 0;
 
 assigned_out:
@@ -1503,6 +1517,7 @@ static int assigned_exitfn(struct PCIDevice *pci_dev)
 {
     AssignedDevice *dev = DO_UPCAST(AssignedDevice, dev, pci_dev);
 
+    unregister_savevm(&dev->dev.qdev, "pci-assign", dev);
     QLIST_REMOVE(dev, next);
     deassign_device(dev);
     free_assigned_device(dev);

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

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1953 bytes --]

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.

Jan

>      return 0;
>  
>  assigned_out:
> @@ -1503,6 +1517,7 @@ static int assigned_exitfn(struct PCIDevice *pci_dev)
>  {
>      AssignedDevice *dev = DO_UPCAST(AssignedDevice, dev, pci_dev);
>  
> +    unregister_savevm(&dev->dev.qdev, "pci-assign", dev);
>      QLIST_REMOVE(dev, next);
>      deassign_device(dev);
>      free_assigned_device(dev);
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

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

[Alex Williamson]

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,

Alex

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

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2586 bytes --]

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:

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?

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

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

[Alex Williamson]

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

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

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 3764 bytes --]

Am 15.11.2010 23:25, Alex Williamson wrote:
> 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.

Ah, of course. Still, vmstate is the way to go IMO.

> 
>>  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

Don't we? We are enforcing qdev but not vmstate? Sounds silly. I think
new devices should all be expressible this way, no?

> 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,

IMHO, new designs should not use register_savevm anymore.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

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

[Markus Armbruster]

Jan Kiszka <jan.kiszka@web.de> writes:

> Am 15.11.2010 23:25, Alex Williamson wrote:
>> 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.
>
> Ah, of course. Still, vmstate is the way to go IMO.

Seconded.

If X can reasonably be done as a declaration (data), then it probably
shouldn't be done as operation (code).

>>>  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
>
> Don't we? We are enforcing qdev but not vmstate? Sounds silly. I think
> new devices should all be expressible this way, no?

We certainly should be enforcing that!

>> 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,
>
> IMHO, new designs should not use register_savevm anymore.
>
> Jan