Re: [libvirt PATCH] qemu_migration: Delete vDPA check

On Tue, Jul 19, 2022 at 6:43 PM Laine Stump <laine(a)laine.org&gt; 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(a)laine.org&gt; 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(a)redhat.com&gt; 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/4003b7047058a17465083178d6c0... > 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.

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