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.
Here, it would be a (at least theoretical) regression.
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
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.
* 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.
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.
Thanks,
Thanks,
Maciej
