On Mon, Jul 27, 2026 at 06:40:09PM +0200, Maciej S. Szmigiero wrote: > On 23.07.2026 21:48, Peter Xu wrote: > > On Thu, Jul 23, 2026 at 12:11:30PM +0200, Maciej S. Szmigiero wrote: > > > On 21.07.2026 20:21, Peter Xu wrote: > > > > On Tue, Jul 21, 2026 at 05:07:53PM +0200, Maciej S. Szmigiero wrote: > > > > > On 20.07.2026 16:55, Peter Xu wrote: > > > > > > On Fri, Jul 17, 2026 at 04:31:32PM +0200, Maciej S. Szmigiero wrote: > > > > > > > Inside the VFIO code such sync point for VFIO devices > > > > > > > could/should indeed > > > > > > > be created, but then it's not available for these other > > > > > > > (non-VFIO) devices > > > > > > > for the purpose of avoiding the regression from the previous > > > > > > > paragraph. > > > > > > > > > > > > > > However, replacing the order priority/adjustment mechanism from > > > > > > > patch 1 > > > > > > > with the "pre" and "post" handlers I described above would avoid > > > > > > > having > > > > > > > that regression (and help fix the vhost-net case too). > > > > > > > > > > > > I'm not sure I fully get the pre/post handlers idea, I think it > > > > > > sounds > > > > > > working, but in all cases I want to decouple it with patch 1: I > > > > > > don't think > > > > > > it requires patch 1, am I right? > > > > > > > > > > It wouldn't require the *current* patch 1 but it would need to be > > > > > implemented > > > > > in some preparatory patch so it would effectively be a new patch 1 > > > > > (or a set > > > > > of patches replacing patch 1). > > > > > > > > > > Assuming that we would go this route instead of the one you described > > > > > below. > > > > > > > > > > > Now if we want to avoid this "theoretical regression" and solve > > > > > > both things > > > > > > together.. I think we may need to refactor the notifiers mechanism. > > > > > > > > > > > > Firstly, I hope we're on the same page that essentially > > > > > > prepare_cb() is the > > > > > > priority mechanism here, we only have HIGH and NORMAL priority, > > > > > > where cb() > > > > > > is the NORMAL priority. > > > > > > > > > > Yeah, I think the current situation could be described this way. > > > > > > > > > > > We also need to persist depth concept per-notifier, I think we > > > > > > should start > > > > > > by renaming VMChangeStateEntry.priority to depth, add a comment > > > > > > explaining > > > > > > it (on different order of invokations on VM start/shutdown). > > > > > > > > > > > > But then, I don't think we need anything as complex as pre/post > > > > > > hooks with > > > > > > hashes. I think you're right then we need SYNC point which can be > > > > > > essentially a priority notifier that is in the middle of HIGH and > > > > > > NORMAL. > > > > > > > > > > > > Hence, I want to see if below should be the easiest: > > > > > > > > > > > > - Rename VMChangeStateEntry.priority to depth > > > > > > > > > > > > - Normalize prepare_cb() into VM_CHANGE_NOTIFY_PRI_HIGH, > > > > > > making cb() to > > > > > > be NORMAL, OTOH. With this, prepare_cb() needs to be > > > > > > registered > > > > > > separately with > > > > > > qemu_add_vm_change_state_handler_prio_full(). The > > > > > > function now should drop prepare_cb() but instead take a real > > > > > > "priority" value of VM_CHANGE_NOTIFY_PRI_*. > > > > > > > > > > And where the qdev tree depth will go in a call to > > > > > qemu_add_vm_change_state_handler_prio_full() - as an additional > > > > > "depth" > > > > > parameter? > > > > > > > > Yes. The rename change I mentioned above should have done so. > > > > > > > > > > > > > > > > > > > > > - Introduce VM_CHANGE_NOTIFY_PRI_SYNC, in the middle of > > > > > > PREPARE / NORMAL. > > > > > > > > > > > > - VFIO can now register its 3rd notifier against SYNC. > > > > > > > > > > > > All rest devices will need to shift part of its logic into PRI_HIGH > > > > > > to > > > > > > either quiesce DMA or enable the backend to accept DMA (when on > > > > > > dest QEMU). > > > > > > None of them will need SYNC only if they'll also switch to an async > > > > > > thread > > > > > > model. > > > > > > > > > > In principle, this *should* work but with some caveats: > > > > > * vhost-net would need the DMA-stopping part of its cb() handler > > > > > separated > > > > > and moved into prepare_cb(), > > > > > > > > I thought this is required with whatever priority impl we will have, > > > > even > > > > if I don't think we need to fix it right now unless you wanted to.. > > > > > > Theoretically this depends on VM's PCI topology but indeed one can argue > > > it's a "pre-existing condition". > > > > > > > > > > > > > * There's some restore code in "PowerPC sPAPR XIVE interrupt > > > > > controller" > > > > > running from cb() at prio 0, so originally always completing before > > > > > VFIO device > > > > > was switched into RUNNING state. > > > > > I'm not 100% sure if it's okay to make VFIO device reach this state > > > > > before > > > > > that interrupt controller restore code finishes, > > > > > > > > Is anyone using VFIO migration on PPC that you're aware of? > > > > > > No, but at the other hand it's hard to be sure of that. > > > > > > Also, see the next paragraphs about one or two additional possibly > > > affected interrupt controllers. > > > > > > > If not, maybe > > > > we don't need to worry too much for now. Meanwhile, this looks like an > > > > existing issue as well. > > > > > > It's an existing issue *on the target* but for a slightly different > > > reason: > > > reading VFIO uAPI it seems that a device is allowed to generate interrupts > > > already in RUNNING_P2P state. > > > And this state is/was reached already after prepare_cb() handlers, so > > > before > > > that PPC interrupt controller handler running from cb() had chance to > > > restore > > > it. > > > > > > However, it does not seem to be an existing issue *on the source* since > > > there INTC cb() callback are currently always guaranteed to run after the > > > VFIO device has already reached STOP_COPY and so was prevented from > > > generating further interrupts. > > > > If we rely on intc have very small (or, 0 by default) depth, then yes. > > These interrupt controllers don't use the vm state change handler > registration functions that account for depth - > qdev_add_vm_change_state_handler*(). > > Instead, they use qemu_add_vm_change_state_handler(), which explicitly > uses/forces priority of 0. > > I presume that's intentional to guarantee correct ordering - the commit > that added ability to specify other priority values (60dbc5a1c517 - "add > qemu_add_vm_change_state_handler_prio()") even said that this new priority > value is "necessary when handlers depend on each other".
It was for ordering scsi bus and virtio device only at that point: https://lore.kernel.org/qemu-devel/[email protected]/ > > > > > > > Here, it would be a (at least theoretical) regression. > > > > I don't yet get what is referred as "regression" here. If what you said is > > correct (which it seems true, that IRQ can be generated during PRECOPY_P2P > > and RUNNING_P2P), then dest QEMU should be broken already with VFIO being > > able to prepare_cb() converting devices to RUNNING_P2P. > > > > That's not a regression by either your current patch or what I proposed > > previously. This is then a bug existed since prepare_cb(). > > By "regression" above I meant the situation that was described in the above > paragraph, that is *on the source*. > > I think there's confusion here because the situation with respect to > prepare_cb() that you are describing above happens *on the target*: > as I wrote above on the target one can argue it's indeed a "pre-existing > condition" - so *not* a regression. > Not so much on the source where the current code *guarantees* the correct > ordering. > > Also, the registered handler code is obviously doing very different thing > on start vs stop so having no ordering requirements/issues on start does > *not* imply there aren't any on stop. > A trivial example: the ARM GICv3 controllers handlers are even NOP on > start/target but they still do certain sync on stop/source. > > > Meanwhile, it was also an accident if it wasn't a bug even before it, > > after all intr chips are still using qemu_* helpers to register, > > where depth isn't accounted. > > But "depth" in the current code is a qdev thing, the core system/runstate.c > only know "priority" and the priority of these INTC *stop* handlers > guarantees they are run after all VFIO devices are in STOP_COPY state. > > In other words, not using "depth" for registration does *not* mean unordered > with respect to qdev handlers - actually quite the opposite. IIUC if irq chips want to order against any device that uses irqs, it should at least have all of them registered via qdev API to provide such depth info. My feeling is the irq chip order isn't intentional, but I didn't check further. > > > In reality maybe we should have done that; in case of an irq controller we > > should make sure the depth should be very small, at least smaller than the > > rest / normal devices to make sure its order of operation, otherwise even > > emulated devices seem to be prone to irq lost during vm start/stop or > > migration. > > The ordering is already correct on *on the source*, but as you wrote above, > due to the existence of prepare_cb() handlers it might not be *on the target*. > > > > > > > What's more, there seem to be one more interrupt controller in two > > > variants/implementations that also do similar things on the source from > > > their > > > cb() handlers: ARM GICv3 basic flavor and ITS flavor. > > > > > > > For the fix whenever wanted, IIUC the intr controller should register > > > > both > > > > HIGH and NORMAL priority notifiers, then when VM stop, disable the > > > > controller only in NORMAL notifier, and when VM starts, enable the > > > > controller only in HIGH notifier (we could think about better names for > > > > the > > > > three steps, maybe it shouldn't be called as "priority" as well even if > > > > the > > > > executions will be strictly ordered). > > > > > > As I wrote above, it looks like a device is already allowed to generate > > > interrupts once it reaches RUNNING_P2P state. > > > Correspondingly, PRE_COPY_P2P only quiesces *peer-to-peer* DMA, there's no > > > mention in the specs that it prevents MSI DMA traffic, let alone INTx > > > interrupt generation. > > > > > > In other words, to implement this properly an interrupt controller would > > > need > > > to have its start/restore handler called at some VERY_HIGH priority > > > (called before > > > > IIUC, my solution proposed should be fine as long as the INTR chip will > > split its vm state handler into two, "running" handler inserted into HIGH > > prority, > > This needs this INTC handler to be run before VFIO launches its device state > changing threads so INTC handler should be registered with what will in the > new code be called "depth" = 0 (or what is called "priority" in the old code) > at HIGH priority, while VFIO threads will be launched at their qdev tree > depth. > So far so good. > > > but "!running" handler inserted into NORMAL priority, as I > > discussed above. > > This needs this INTC handler to be run after VFIO collects its device state > changing threads so again with "depth" = 0. > > This would be fine - as long as VFIO thread collection *on stop/source* > happens at some "depth" > 0 - see the caveat down below. > > > This also depends on that we will notify each queue (one > > for each priority) with the depth in mind, so we will make sure to disable > > / quiesce the irq chip last (NORMAL, depth=0) when stop, and enable it > > first (HIGH, depth=0) when start. > > Yeah. > > > > > NOTE: we have HIGH/SYNC/NORMAL, so NORMAL is the lowest priority, if we > > call it like that. We can rename them.. > > > > > VFIO device state change threads are launched) and have its stop/suspend > > > handler > > > called at a VERY_LOW priority (called after VFIO threads are collected) - > > > this would be the equivalent of running these INTC cb() at prio 0 during > > > VM stop now. > > > > > > Assuming, of course that the interrupt controller actually needs to do > > > some work > > > on VM start to be able to accept interrupts and that it needs to do some > > > saving work on VM stop which can't be done until no new interrupts could > > > came. > > > > Yes, I also didn't read into all irq chip code yet. > > > > > > > > > > > > > > > > > > > * The postponed dirty memory logging stop handler also runs from cb() > > > > > at > > > > > prio 0, so currently always finishes before VFIO device was switched > > > > > into > > > > > RUNNING state. > > > > > This handler also stops DMA logging for VFIO device or its container, > > > > > but here I presume it should be safe to do so while the VFIO device > > > > > has already > > > > > started dirtying memory even though this couldn't happen in the > > > > > existing code? > > > > > > > > It's only stopping the tracking, results of the tracking shouldn't > > > > matter > > > > anymore on whether some DMA was trapped. It looks safe no matter where > > > > we > > > > put it. > > > > > > > > > > > > > > * There's some sync/setup of x86 vAPIC state (for 32-bit guests?) > > > > > also running > > > > > from cb() at prio 0. > > > > > Not sure if this needs to finish before VFIO device gets switched into > > > > > RUNNING state. > > > > > > > > Doesn't look relevant. IIUC, only anything that is DMA capable can be > > > > relevant. > > > > > > It's relevant since a VFIO device could generate interrupts as soon as it > > > is in RUNNING_P2P state. > > > > > > However, this one here is start-side so it would be an existing issue > > > rather > > > than a regression. > > > > > > > > > > > > > These 3 possible issues above (besides vhost-net move/refactor) could > > > > > be > > > > > theoretically worked around by moving these callbacks from cb() to a > > > > > new > > > > > priority *before* VM_CHANGE_NOTIFY_PRI_SYNC (maybe called > > > > > VM_CHANGE_NOTIFY_PRI_SYNC_BEFORE or similar) as I presume they won't > > > > > need the > > > > > VFIO device to reach DMA-accepting state but at the same time they > > > > > shouldn't > > > > > delay the launch of VFIO device state changing threads from > > > > > VM_CHANGE_NOTIFY_PRI_HIGH. > > > > > > > > IIUC we don't need a 4th step here, correct me otherwise, > > > > > > Depends whether we want to fix the existing issues or not. > > > If not, then we don't need the extra priority level on the target. > > > > > > However, I think we still need a possible extra level (VERY_LOW or > > > similar) on > > > the source to prevent possible regression with respect to the current > > > code. > > > > I'll re-comment after you read above. In short, we have 3 priorities > > (HIGH/SYNC/NORMAL, but again, prone to be renamed) but that's not the only > > way we order things: for each priority we will still notify with "depth" > > order, it's just that we will revert the depth order depending on running > > or not. irq chip issue is better solved with "depth" setting right all > > over the places, because it has a semantic of "depth" not real "priority" > > (e.g. turns off last, turns on first). > > As I wrote above, if we do INTC at "depth" = 0 then we'll want to collect > VFIO threads *on stop/source* at "depth" > 0. > > At the same time, we want to have these threads running in parallel with > other cb() handlers at NORMAL priority for as long as possible. > > Here, it seems that just using "depth" of 1 for VFIO threads collection > *on stop/source* seems to work since every other existing cb() handler > seems to sit at a minimum "depth" of 2-3. Why VFIO needs to adjust "depth"? Can't it always be the depth of qdev tree? On source side: - Phase 1 HIGH should turn all devices (VFIO or emulated) to be into P2P state. Things like buses or irq chip not included; they need to be ordered hierachically / properly. Other that that I don't see why VFIO cares about other devices if it's only about "do not initiate any more DMA at this stage". - Phase 2 SYNC vfio should be the only user sync with HIGH's threads making sure P2P conversions complete for vfios, then kick off the 2nd phase for all vfio devices. - Phase 3 NORMAL should turn all devices STOPPED. Again, I don't see why we need specific ordering here amoung all the normal devices. > > However, on the *target/start* the call order vs "depth" is inverted, > so ordering VFIO threads collection after every other existing cb() > handler needs them registered at INT_MAX or other very high depth. > > > > > > > > But that's a minor detail as I presume adding additional level would be > > > essentially > > > only a small data addition in some array and/or enum. > > > > > > > while the 3rd > > > > SYNC was only needed for VFIO's threaded approach. > > > > > > I assume that we're designing the "plumbing" that allows doing > > > async/threaded > > > VFIO state change safely here. > > > > > > > > > > > > > Also, the final VFIO thread joining/collection does *not* need to be > > > > > ordered > > > > > with exiting cb() handlers, so it should really run at > > > > > VM_CHANGE_NOTIFY_PRI_LOW > > > > > (below PRI_NORMAL) for best parallelism. > > > > > > > > Yes. VFIO device only need its SYNC / MEDIUM priority notifier be > > > > ordered > > > > with NORMAL notifiers, to make sure all VFIO devices switched to P2P > > > > state > > > > before the NORMAL notifiers. > > > > > > > > > > > > > > The disadvantages of this overall design are that it would need > > > > > coordination > > > > > between different sub-maintainers, increase the "blast radius" of the > > > > > patch > > > > > set to different parts of QEMU code, and so obviously would be more > > > > > regression-risky than the aforementioned "pre" and "post" handlers > > > > > design. > > > > > > > > > > Especially that probably few people would be able to test, for > > > > > example, > > > > > the PPC XIVE case with VFIO. > > > > > > > > We shouldn't justify a solution by "how many subsystems it touches". > > > > > > > > I was suggesting to solve one problem at a time, but then prepare_cb() > > > > already does it. You pointed out there might be real regression on VM > > > > start path, I agreed. > > > > > > > > I thought you wanted to fix all things, which I'm ok with and agree it > > > > is > > > > better, then it's unavoidable to touch all modules. But yes, we need to > > > > reproduce the problems and verify the fix. > > > > > > > > The middle ground is we define the notifiers to suite for the solution > > > > we > > > > think will fix everything, but you only need to fix VFIO and leave the > > > > rest > > > > for later. We can discuss the problems in the cover letter or commit > > > > logs > > > > for future works on top. After all, in this case of VFIO to do the > > > > right > > > > things it does need three steps / notifiers. > > > > > > I think if there are existing issues then it's probably okay just to > > > document > > > these for future reference. > > > > > > However, I would be vary of creating any clear regression, especially on > > > a hardware that I can't test. > > > > Understood, I have no good idea on testing. IIUC what needs to be tested > > for irq chips do not need VFIO involvement, maybe some regression test > > should be enough on either migration or even stop/cont. May need some help > > from arch developers indeed if we want some better coverage. > > > > If you can think of anything that doesn't need to mess up the depth as > > patch 1 does (so if you want to reuse depth, make it a real "depth" would > > be fine, not a mixture of depth+prio), then maybe we can check that. I just > > can't think of anything else yet. > > Overall, I think that the design that we discussed above is workable, > barring some unforeseen troubles with its implementation or runtime testing > issues. > > I also feel that at this point it will be much better to prepare a v2 > and then tweak it as/if necessary. > Agree? I always prefer having some code and do some real tests to support discussions, rather than pure discussions. Please send it whenever you feel comfortable. E.g. I treat the irq issues still theoretical until we can provide a reproducer. Thanks, -- Peter Xu
