[libvirt PATCH] qemu_migration: Delete vDPA check

[Laine Stump]

On 7/19/22 12:01 PM, Laine Stump wrote:
> On 7/19/22 11:09 AM, Eugenio Perez Martin wrote:
>> On Tue, Jul 19, 2022 at 4:02 PM Laine Stump <laine at laine.org> wrote:
>>>
>>> On 7/18/22 11:15 AM, Jiri Denemark wrote:
>>>> On Mon, Jul 18, 2022 at 10:40:56 +0200, Eugenio Perez Martin wrote:
>>>>> On Mon, Jul 18, 2022 at 10:25 AM Jiri Denemark 
>>>>> <jdenemar at redhat.com> wrote:
>>>>>> Which in ideal case would mean only a QMP command (such as
>>>>>> hotplugging a non-migratable device) is the only way to add migration
>>>>>> blocker. If this is true, than we're safe as libvirt does not 
>>>>>> allow such
>>>>>> commands between qemuMigrationSrcIsAllowed and migration start.
>>>>>>
>>>>>
>>>>> Ok, that rules out a few bad use cases. I can do a fast lookup to
>>>>> check if blockers can be added without the knowledge of libvirt.
>>>>>
>>>>>> That said, is there a reason for not implementing the correct 
>>>>>> solution
>>>>>> right away as a separate patch?
>>>>>>
>>>>>
>>>>> I was not sure if libvirt already had another way to check, for
>>>>> example, if the vhost device didn't have VHOST_F_LOG_ALL feature.
>>>>
>>>> I'm not aware of such check, but even if it exists, checking for
>>>> migration blockers looks like the right way of doing things anyway.
>>>
>>> Actually that's been on my todo list for a long time - for any qemu that
>>> supports the QMP command that checks for migratability, we should be
>>> calling this command rather than checking against our own internal list
>>>    (which is really just an "informed guess") of what can't be migrated.
>>> This way we'll always get the right answer (or at least what QEMU
>>> believes to be the right answer :-)). Fixing it this way will also mean
>>> that migration of VFIO devices will just "magically" start working once
>>> a migration-supporting driver is written for the device, and the correct
>>> vfio driver is bound to the device (this latter item is also on my 
>>> list).
>>>
>>> So if you're up for making the patch to call the QMP command, I'd be
>>> happy to review it!
>>>
>>
>> Thanks! Actually I'd need some guidance first, I'm not very used to
>> libvirt code.
>>
>> As I understand I should create a function in qemu_agent.h/c, a getter
>> similar to qemuAgentGetFSInfo. How can I get a qemuAgent from
>> qemuMigrationSrcIsAllowed? I only have a virQEMUDriver there.
> 
> qemu_agent.c is only for functions that require calling to the QEMU 
> guest agent, which is a process running inside the guest. You just need 
> to run a simple QMP command. There are some good examples of this in 
> qemu_monitor_json.c
> 
>>
>> For now it should be enough to delete vdpa hardcoded negation, and
>> then other parts of libvirt can delete other hardcoded checks, isn't
>> it?
> 
> There's just a single function that checks for migratability 
> (qemuMigrationSrcIsAllowed()). In theory *everything* in that function 
> should be deprecated by just calling qemu to ask. In practice there may 
> be / probably are things that qemu doesn't count as "can't migrate" that 
> really should be counted that way. Certainly the VDPA and hostdev checks 
> should be removable immediately though (although of course this should 
> still be checked before pushing!)
> 
> 
> What I would do is this:
> 
> 1) a patch that adds code to the qemu_capabilities to set a flag if the 
> desired field in the "query-migrate" QMP command would be filled in by 
> this qemu binary.

Just to permanently document live discussions from IRC:

jjongsma pointed us to a patch he wrote a year ago (but never pushed 
upstream) that implements (1):

https://gitlab.com/jjongsma/libvirt/-/commit/4003b7047058a17465083178d6c0a0f62c900c74

> 
> 2) a patch that adds a function to qemu_monitor_json.c to call that 
> command and return migratable/not.

Thinking about this more, I guess a function that returns the full text 
of "blocked-reason" would be more useful (that way it could be easily 
logged).

> 3) a patch that adds a call to that function to 
> qemuMigrationSrcIsAllowed().
> 
> 4) additional patches that remove specific hardcoded checks *only if the 
> new field in query-migrate is available (as indicated by the new 
> capabilities flag) and returned a definite yes/no (otherwise the checks 
> still need to be done, to account for older qemu binaries that don't 
> have the qmp command).
> 
> I had thought there was a bugzilla somewhere that was tracking this for 
> libvirt, but I can't find it.
>