[libvirt PATCH] qemu_migration: Delete vDPA check

[Laine Stump]

On 7/19/22 2:38 PM, Eugenio Perez Martin wrote:
> On Tue, Jul 19, 2022 at 6:43 PM Laine Stump <laine at laine.org> wrote:
>>
>> 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
>>
> 
> Thanks!
> 
>>>
>>> 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).
>>
> 
> I'm actually returned all the array in the form of `char ***`
> 
>>> 3) a patch that adds a call to that function to
>>> qemuMigrationSrcIsAllowed().
>>>
> 
> What I cannot find an example of is how to get the qemuMonitor from
> the virQEMUDriver that qemuMigrationSrcIsAllowed has as the parameter.

Hopefully I'm not leading you down a false path, but it looks like the 
call chain of:

    qemuDomainChangeMemoryRequestedSize()
      qemuMonitorChangeMemoryRequestedSize()
        qemuMonitorJSONChangeMemoryRequestedSize()

contains all the bits you need, including the toplevel function that 
calls qemuDomainObj(Enter|Exit)Monitor() and grabs the qemuMonitor 
object from the domainObj's privateData before calling down to a wrapper 
function in qemu_monitor.c (that I think was originally there because 
there could be either an old-style shell-like command or a QMP command 
to do the same thing), and then from there down to the QMP/JSON function 
in qemu_monitor_json.c that actually calls the monitor.


> 
>>> 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).
>>>
> 
> Yes, I agree.
> 
> Thanks!
> 
>>> I had thought there was a bugzilla somewhere that was tracking this for
>>> libvirt, but I can't find it.
>>>
>>
>