On 16.07.2026 15:30, Peter Xu wrote:
On Thu, Jul 16, 2026 at 02:18:34PM +0200, Maciej S. Szmigiero wrote:
On 15.07.2026 17:30, Peter Xu wrote:
On Tue, Jul 14, 2026 at 10:23:05PM +0200, Maciej S. Szmigiero wrote:
The VFIO devices in {PRE_COPY,RUNNING}_P2P states can {still,already} accept
DMA so in the master branch by the time prepare_cb() handlers exit and
cb() ones start running the VFIO devices are {still,already} capable of
accepting incoming DMA.
On the other hand, if you meant the state transition into STOP_COPY on the
migration source currently initiated from cb() handler then there can be an
ordering issue in principle.
It looks like the current master branch mostly avoids this by registering
vhost-net vm_state_change handler from VirtIODevice and not from its parent
PCI device, so with the usual PCI topology vhost-net cb() handler will run
before the VFIO PCI device one (and so still hit DMA-capable VFIO device)
due to the vhost-net VirtIODevice being one level deeper in the qdev tree.
It could theoretically be problem with more complex PCI topology though.
That's an interesting observation, but neither do I think it's intentional,
nor do I think it guarantees the depth ordering. Consider when a VFIO
device is put under a few layers of PCIe switches, or PCI bridges. In that
case the VFIO device notifier can have larger depth value v.s. the virtio
device when the virtio device is attached to the root complex.
Yeah, that's why I wrote that it could be a problem with "more complex PCI
topology".
Ah yes.
Nevertheless, that's probably uncommon enough setup to not generate visible
complaints about things being broken.
It is also an issue then even before P2P_DMA state introduced, because
before that all cb()s can be run at random orders.
Technically, the order is based on the device qdev tree depth and within
the same qdev tree depth level it looks like the handler calling order will
be dependent on the order the registering devices were instantiated.
This means that even before these P2P states were introduced
VM configurations that instantiated VFIO/virtio devices in the certain
"right" order or topology weren't affected.
So maybe adding these P2P states mostly fixed this issue too?
I still think they're two different issues, and I still think with/without
P2P the issue is the same, now I tend to agree this issue exists,
especially if virtio-blk have similar behavior like virtio-net.
For one example of virtio-blk DMA to VFIO MMIO regions:
https://lore.kernel.org/r/[email protected]
Said that, I don't know it's 100% triggerable in this case with notifiers,
but sounds relevant.
It does look like it could trigger this issue indeed since it's DMA done
to VFIO device MMIO BAR.
However, how easy would be to reproduce this is another question.
For virtio-net and others, I have less idea.
In all cases, I still think there are two issues to fix, and I think we
don't need to fix this problem in one shot. I still don't think they're
directly relevant, at least on the goals.
Sure, let's just not make the possible race situation worse.
So with what I suggested previously relying on prepare_cb() I believe we
can still do the concurrency issue you're looking for, making sure when
reaching VFIO's cb() it's still at least accepting DMAs (P2P is fine).
But that's more or less the design of this (original) patch set -
make the state change to RUNNING_P2P on the target finish already in the
prepare_cb() time, make the state change to STOP_COPY on the source begin
only in the cb() time?
I am not against the idea in general. I'm against patch 1 where it hijacks
"depth" into something even more complicated.
The "depth" itself makes sense to be recorded as a value, but I think it's
already a bit confusing to call it "priority" in VMChangeStateEntry,
because it isn't: when something has higher "priority", it should always be
executed before lower priority items. Here, the current code invert the
order for !RUN case, so it's in reality "depth", not "priority".
Now patch 1 added yet another extra "adj" concept on top of qdev hierachy's
"depth", which in fact the caller needs "priority" and callers need to
explicitly handle that by checking RUNNING and invert things.
That's the part I'm definitely against. Such code is IMHO not maintainable.
So my point is if we can implement your idea with existing prepare_cb() we
should just use it. Even if we want to enhance the priority support for
cb()s, we shouldn't use patch 1. I never thought further on that because I
still assumed relying on prepare_cb() works.
If term "priority" is misleading due to reverse order of calling these handlers
on start vs stop we can change this term to "order".
Then the new adjustment becomes "order adjustment".
On the other hand, if the need for "additional indirection" inside VFIO code
to make the start vs stop handler ordering constant and the priority/order
adjustment mechanism from patch 1 are the most controversial parts of this
approach then there's another way to have prepare_cb()/cb() called at the
beginning and end of vm_change_state priority level:
Simply add another data structure besides vm_change_state_head with these
"pre" and "post" handlers (probably a hash table) and then call these
handlers before and after entering each priority value inside the existing
QTAILQ_FOREACH_SAFE() and QTAILQ_FOREACH_REVERSE_SAFE() in vm_state_notify().
This way the VFIO code could register these "pre" and "post" handlers
directly for its prepare_cb() and cb() state transitions.
For the other issue you reported, even if existed, I am not yet sure VFIO
is the only special case that got affected. I think it's possible some
other emulated devices suffer the same even if it has no direct hardware
attached. Say, there can be device backends got stopped in cb() when VM
stopped, further DMA to it may cause assertions, then it's the same issue
that needs solving, where essentially we may require all such devices to
provide similar P2P states like VFIO to make sure they initiate DMAs and
flush them in a prepare_cb(), then if all devices' prepare_cb() will make
sure no DMA to be initiated anymore, cb()s will have no ordering constraint
on DMAs.
Although the description above is about the migration source the situation
on the migration target is essentially a mirror image of it so I will refer
to both cases below.
Specifically:
* on the source no device should reach non DMA accepting state (like STOP_COPY
for VFIO) before all devices reach non DMA generating state
(PRE_COPY_P2P-equivalent).
* on the target no device should reach DMA generating state (RUNNING-equivalent)
before all devices reach DMA accepting state (RUNNING_P2P-like).
That's why even in the case where non-VFIO devices introduce the equivalent of
VFIO P2P states and implement a prepare_cb() callback we still need a (VM-wide)
sync point between these callbacks for the benefit of async state
changes/threads
since no cb()-like async state change should start until all prepare_cb()-like
changes finish (including these for other devices).
IIUC, prepare_cb() + cb() is already that "sync point". After all
prepare_cb() done, the VM should achieve "DMA quiesced" across the system?
What's missing is we should move all problematic device cb()s "let's flush
pending DMAs" logic into a prepare_cb(), leaving the real stop logic in
cb()s. That's the 2nd problem which I think we can skip until a reproducer
pops up at least.
Judging by the fact that you mention "flushing the pending DMA" I think you
refer to situation on the source.
On the source there's the vhost-net issue, which one can indeed be broken
even in the current git master by atypical PCI topology.
But there's similar but mirror problem on the target (described in my second
bullet point in the quote above) - the one with virtio-blk and scsi-bus.
Even today, this issue should not depend on VM PCI topology.
In the target case the "sync point" is about all devices being "DMA ready"
(but not yet launching DMA - I wouldn't call it "DMA quiescent" since
there's nothing to quiesce yet at this point).
So this would be a true regression with VFIO parallel state change threads.
VFIO is only special here when it wants to concurrent process of state
changes. If you create threads in prepare_cb(), then in cb() you have
chance to synchronize everything correctly satisfying the "sync point".
That's not a requirement for most of the rest devices.
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).
Thanks,
Thanks,
Maciej
