Re: [PATCH] device-assignment: Register as un-migratable
Jan Kiszka 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
>> ---
>>
>> 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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] device-assignment: Register as un-migratable
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
> ---
>
> 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
signature.asc
Description: OpenPGP digital signature
Re: [PATCH] device-assignment: Register as un-migratable
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
> >>> ---
> >>>
> >>> 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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] device-assignment: Register as un-migratable
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
>>> ---
>>>
>>> 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
signature.asc
Description: OpenPGP digital signature
Re: [PATCH] device-assignment: Register as un-migratable
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
> > ---
> >
> > 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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] device-assignment: Register as un-migratable
Am 15.11.2010 20:41, Alex Williamson wrote:
> Use register_device_unmigratable() to declare ourselves as
> non-migratable.
>
> Signed-off-by: Alex Williamson
> ---
>
> 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);
>
signature.asc
Description: OpenPGP digital signature
