Jan Kiszka <jan.kiszka@xxxxxx> 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@xxxxxxxxxx>
>>>>>> ---
>>>>>>
>>>>>> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
