On 2/13/26 17:35, Peter Xu wrote:
On Fri, Feb 13, 2026 at 10:05:06AM +0100, Cédric Le Goater wrote:
On 2/11/26 20:06, Peter Xu wrote:
On Wed, Feb 11, 2026 at 06:15:32PM +0100, Cédric Le Goater wrote:
The file migration/cpr.c uses vmstate_cpr_vfio_devices which is
declared in hw/vfio/vfio-cpr.h, not in hw/vfio/vfio-device.h.
Replace the include with the correct header file to avoid pulling in
unnecessary VFIO device declarations.
Signed-off-by: Cédric Le Goater <[email protected]>
---
migration/cpr.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/migration/cpr.c b/migration/cpr.c
index
adee2a919a364244867ef190fdf29bd1fe7e20e2..a0b37007f55d6dac32bd6220ffe65d32c4e6b52e
100644
--- a/migration/cpr.c
+++ b/migration/cpr.c
@@ -9,7 +9,7 @@
#include "qemu/error-report.h"
#include "qapi/error.h"
#include "qemu/error-report.h"
-#include "hw/vfio/vfio-device.h"
+#include "hw/vfio/vfio-cpr.h"
#include "migration/cpr.h"
#include "migration/misc.h"
#include "migration/options.h"
--
2.53.0
This should be better as a standalone change, thanks.
Reviewed-by: Peter Xu <[email protected]>
Some thoughts beyond this patch alone.
When looking at this, it's a layer violation. Logically migration/cpr.c
shouldn't include a vfio header, otherwise it means when new devices added
to support cpr then migration needs to include headers to every single of
these devices..
Logically it should be the other way round where migration/cpr.c should
provide a helper allowing devices to opt-in for cpr-states to register
instead.
with vmstate_un/register() ?
Likely not, because the vmstate_register() API only put things onto the
global default savevm_state.handlers. Those are "normal" VMSDs that will
be migrated within the normal migration framework / channel, not the CPR
special channel at earlier stage.
CPR channel currently saves only the special VMSD, which is currently
vmstate_cpr_state. That should also be why the vfio early sub-section is
attached.
Logically it can be another similar vmstate_register_cpr(), then that'll
put the VFIO speical VMSD to be one of the subsections registered to the
CPR vmstate_cpr_state global VMSD. Here subsections can be a good API
between CPR core and CPR submodules (like VFIO) because when we send a
subsection the dest QEMU will be able to lookup the subsections based on
the name of subsection.
Then I believe it means when there're >1 consumers we don't need to worry
on which got registered first, so the order of subsections to be registered
shouldn't be part of ABI, however the name of the subsection VMSD will (see
vmstate_get_subsection() on how we do the lookup when loadvm, that applies
too when loading CPR state).
I don't know any existing use case of such dynamically registering
subsections, but I think it sounds doable and maybe one simple solution for
CPR's use to make registrations dynamic.
Now it's also awkward that vmstate_cpr_state always have a subsection
called vmstate_cpr_vfio_devices.. it means we will migrate this subsection
even if there's no vfio device.. that's also another point that I think it
should be done the other way round..
I do not know whether we must persist CPR ABI now.. if we need, we can't
change this anymore without a machine type compat entry (as we will expect
vmstate_cpr_vfio_devices to come from an older binary that does a cpr
migration). But that'll be sad..
vmstate_cpr_state was introduced in 10.0
vmstate_cpr_vfio_devices in 10.1
commit a6f2f9c42f3a ("migration: vfio cpr state hook") already
broke migration compatibility. May be this is something we can
still fix without a machine compat.
I have no idea how cpr is used today, in which product ? Can the
maintainers tell us more ?
Yep.
I didn't pay much attention when reading the series when being reviewed,
but IMHO we could have marked cpr features unstable from the start until
it's relatively solid and in serious production deployments.
It's quite invasive currently and I wish it could simply be compiled out.
It would make it easier to catch design issues like this one and save us
time when investigating problems.
C.
