On Tue, Sep 29, 2026 at 03:21:46PM +0200, Juraj Marcin wrote: > From: Juraj Marcin <[email protected]> > > Previously, the post init CPU synchronization with the accelerator, like > KVM, was performed in the bottom half of the POSTCOPY_RUN command > handler. However, this causes several problems. > > First issue is that if CPU synchronization fails, the destination QEMU > crashes. However, it is too late to recover the source side as the > response to special PING has been already sent, and both sides are > already in the POSTCOPY_ACTIVE state. > > By moving synchronization before responding, if the machine crashes, the > response is never sent and the source side can resume from the > POSTCOPY_DEVICE state. > > Second issue is caused when migration is paused due to a network > failure or user command right after transitioning to the POSTCOPY_ACTIVE > state and the CPU synchronization causes a page fault. This page fault > blocks the CPU and the main QEMU threads and cannot be resolved until > postcopy migration is recovered. However, as libvirt also tries to > execute 'cont' QMP command at this time (destination side transitions > from POSTCOPY_DEVICE to POSTCOPY_ACTIVE), it will also halt waiting for > the response from the blocked main QEMU thread, unaware of the fact that > the migration is paused and needs to be recovered. Thus, it will wait > indefinitely and never report postcopy migration error. > > This patch adds an option to VMSD that identifies devices that require > CPU post init synchronization and runs synchronization for those device > after they are loaded. > > This moved CPU synchronization right to the device load and if this CPU > synchronization fails, or the network fails during it, the source side > can safely transition from POSTCOPY_DEVICE to a FAILED state. > > Signed-off-by: Juraj Marcin <[email protected]> > --- > This patch an alternative approach to my previous RFC patch [1] that is > in my opinion less hacky and also pretty minimal. > > [1]: https://lore.kernel.org/r/[email protected]/
Thanks for the repost, Juraj. Looks like this "integrate post_init into post_load" idea isn't trivial indeed. So after I read it, concerns I have: - The current approach of marking VMSD with cpu_sync, definitely not ideal when VMSD is a generic structures; not good if VMSD code needs to reference CPUState at all, - The difficulty justifying this won't break, one example is what happens if some other VMSDs may update the CPUState object, used to be reflected in post_init() in master branch where it was done after all post_load()s (and maybe something else). It's the part where I questioned myself too in previous reply of your rfc series, paragraph "Said that, I do see some complexity over such a change..". So now I start to wonder if we should stick with the PONG-later idea: simpler, easier to justify, and expecting to fix the issue too. Denis already sent a patch for another issue, assuming it fixes this issue too: https://lore.kernel.org/r/[email protected] Let me know your thoughts. > --- > include/migration/vmstate.h | 6 ++++++ > migration/savevm.c | 5 ----- > migration/vmstate.c | 6 ++++++ > target/alpha/machine.c | 1 + > target/arm/machine.c | 1 + > target/avr/machine.c | 1 + > target/hexagon/machine.c | 1 + > target/hppa/machine.c | 1 + > target/i386/machine.c | 1 + > target/loongarch/machine.c | 1 + > target/m68k/cpu.c | 1 + > target/microblaze/machine.c | 1 + > target/mips/system/machine.c | 1 + > target/or1k/machine.c | 1 + > target/ppc/machine.c | 1 + > target/riscv/machine.c | 1 + > target/s390x/machine.c | 1 + > target/sh4/cpu.c | 1 + > target/sparc/machine.c | 1 + > target/xtensa/cpu.c | 1 + > 20 files changed, 29 insertions(+), 5 deletions(-) > > diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h > index 2901877308..60ddeacdd5 100644 > --- a/include/migration/vmstate.h > +++ b/include/migration/vmstate.h > @@ -298,6 +298,12 @@ struct VMStateDescription { > */ > > bool early_setup; > + /* > + * This VMSD describes a CPU which requires synchronization with an > + * accelerator after it's loaded. Can be set to true only if the device > + * described by this VMSD is of type CPU. > + */ > + bool cpu_sync; > int version_id; > int minimum_version_id; > MigrationPriority priority; > diff --git a/migration/savevm.c b/migration/savevm.c > index 4b590ea672..d7153912b1 100644 > --- a/migration/savevm.c > +++ b/migration/savevm.c > @@ -2235,8 +2235,6 @@ static void loadvm_postcopy_handle_run_bh(void *opaque) > /* TODO we should move all of this lot into postcopy_ram.c or a shared > code > * in migration.c > */ > - cpu_synchronize_all_post_init(); > - > trace_vmstate_downtime_checkpoint("dst-postcopy-bh-cpu-synced"); > > qemu_announce_self(&mis->announce_timer, migrate_announce_params()); > @@ -3217,8 +3215,6 @@ int qemu_loadvm_state(QEMUFile *f, Error **errp) > } > } > > - cpu_synchronize_all_post_init(); > - > return ret; > } > > @@ -3233,7 +3229,6 @@ int qemu_load_device_state(QEMUFile *f, Error **errp) > return ret; > } > > - cpu_synchronize_all_post_init(); > return 0; > } > > diff --git a/migration/vmstate.c b/migration/vmstate.c > index 24004565bf..80a23f8943 100644 > --- a/migration/vmstate.c > +++ b/migration/vmstate.c > @@ -19,6 +19,7 @@ > #include "qemu-file.h" > #include "qemu/bitops.h" > #include "qemu/error-report.h" > +#include "system/hw_accel.h" > #include "trace.h" > > static bool vmstate_subsection_save(QEMUFile *f, const VMStateDescription > *vmsd, > @@ -449,6 +450,11 @@ bool vmstate_load_vmsd(QEMUFile *f, const > VMStateDescription *vmsd, > return false; > } > > + if (vmsd->cpu_sync) { > + CPUState *cpu = CPU(opaque); > + cpu_synchronize_post_init(cpu); > + } > + > trace_vmstate_load_state_success(vmsd->name); > return true; > } > diff --git a/target/alpha/machine.c b/target/alpha/machine.c > index dcba5d441b..2b72159f06 100644 > --- a/target/alpha/machine.c > +++ b/target/alpha/machine.c > @@ -82,6 +82,7 @@ static const VMStateField vmstate_cpu_fields[] = { > > const VMStateDescription vmstate_alpha_cpu = { > .name = "cpu", > + .cpu_sync = true, > .version_id = 1, > .minimum_version_id = 1, > .fields = vmstate_cpu_fields, > diff --git a/target/arm/machine.c b/target/arm/machine.c > index 53a2734d25..cdc9003866 100644 > --- a/target/arm/machine.c > +++ b/target/arm/machine.c > @@ -1247,6 +1247,7 @@ static int cpu_post_load(void *opaque, int version_id) > > const VMStateDescription vmstate_arm_cpu = { > .name = "cpu", > + .cpu_sync = true, > .version_id = 22, > .minimum_version_id = 22, > .pre_save = cpu_pre_save, > diff --git a/target/avr/machine.c b/target/avr/machine.c > index 8064a7214a..9232c98e72 100644 > --- a/target/avr/machine.c > +++ b/target/avr/machine.c > @@ -99,6 +99,7 @@ static const VMStateInfo vms_eind = { > > const VMStateDescription vms_avr_cpu = { > .name = "cpu", > + .cpu_sync = true, > .version_id = 1, > .minimum_version_id = 1, > .fields = (const VMStateField[]) { > diff --git a/target/hexagon/machine.c b/target/hexagon/machine.c > index 179b6f53a2..23548639a7 100644 > --- a/target/hexagon/machine.c > +++ b/target/hexagon/machine.c > @@ -21,6 +21,7 @@ static int hexagon_cpu_post_load(void *opaque, int > version_id) > > const VMStateDescription vmstate_hexagon_cpu = { > .name = "cpu", > + .cpu_sync = true, > .version_id = 2, > .minimum_version_id = 2, > .post_load = hexagon_cpu_post_load, > diff --git a/target/hppa/machine.c b/target/hppa/machine.c > index 6a143cf8df..bfc286caaa 100644 > --- a/target/hppa/machine.c > +++ b/target/hppa/machine.c > @@ -224,6 +224,7 @@ static const VMStateField vmstate_cpu_fields[] = { > > const VMStateDescription vmstate_hppa_cpu = { > .name = "cpu", > + .cpu_sync = true, > .version_id = 1, > .minimum_version_id = 1, > .fields = vmstate_cpu_fields, > diff --git a/target/i386/machine.c b/target/i386/machine.c > index 8d69d7e25e..ced3c9a2ac 100644 > --- a/target/i386/machine.c > +++ b/target/i386/machine.c > @@ -1806,6 +1806,7 @@ static const VMStateDescription vmstate_apx = { > > const VMStateDescription vmstate_x86_cpu = { > .name = "cpu", > + .cpu_sync = true, > .version_id = 12, > .minimum_version_id = 11, > .pre_save = cpu_pre_save, > diff --git a/target/loongarch/machine.c b/target/loongarch/machine.c > index 931a5ca5ba..e6deddce58 100644 > --- a/target/loongarch/machine.c > +++ b/target/loongarch/machine.c > @@ -210,6 +210,7 @@ static const VMStateDescription vmstate_tlb = { > /* LoongArch CPU state */ > const VMStateDescription vmstate_loongarch_cpu = { > .name = "cpu", > + .cpu_sync = true, > .version_id = 4, > .minimum_version_id = 4, > .fields = (const VMStateField[]) { > diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c > index 9b52ad5fc2..28bc7bdd71 100644 > --- a/target/m68k/cpu.c > +++ b/target/m68k/cpu.c > @@ -638,6 +638,7 @@ const VMStateDescription vmstate_68040_spregs = { > > static const VMStateDescription vmstate_m68k_cpu = { > .name = "cpu", > + .cpu_sync = true, > .version_id = 1, > .minimum_version_id = 1, > .fields = (const VMStateField[]) { > diff --git a/target/microblaze/machine.c b/target/microblaze/machine.c > index f930ff8609..63c8816582 100644 > --- a/target/microblaze/machine.c > +++ b/target/microblaze/machine.c > @@ -101,6 +101,7 @@ static const VMStateField vmstate_cpu_fields[] = { > > const VMStateDescription vmstate_mb_cpu = { > .name = "cpu", > + .cpu_sync = true, > .version_id = 0, > .minimum_version_id = 0, > .fields = vmstate_cpu_fields, > diff --git a/target/mips/system/machine.c b/target/mips/system/machine.c > index bd1e4002cf..536d98f3c4 100644 > --- a/target/mips/system/machine.c > +++ b/target/mips/system/machine.c > @@ -374,6 +374,7 @@ static const VMStateDescription > mips_vmstate_octeon_crypto = { > > const VMStateDescription vmstate_mips_cpu = { > .name = "cpu", > + .cpu_sync = true, > .version_id = 21, > .minimum_version_id = 21, > .post_load = cpu_post_load, > diff --git a/target/or1k/machine.c b/target/or1k/machine.c > index 2d5ca16c51..abf11a37a5 100644 > --- a/target/or1k/machine.c > +++ b/target/or1k/machine.c > @@ -133,6 +133,7 @@ static int cpu_post_load(void *opaque, int version_id) > > const VMStateDescription vmstate_openrisc_cpu = { > .name = "cpu", > + .cpu_sync = true, > .version_id = 1, > .minimum_version_id = 1, > .post_load = cpu_post_load, > diff --git a/target/ppc/machine.c b/target/ppc/machine.c > index 569536bd02..33b46d6f3c 100644 > --- a/target/ppc/machine.c > +++ b/target/ppc/machine.c > @@ -733,6 +733,7 @@ static const VMStateDescription vmstate_bhrb = { > > const VMStateDescription vmstate_ppc_cpu = { > .name = "cpu", > + .cpu_sync = true, > .version_id = 5, > .minimum_version_id = 5, > .pre_save = cpu_pre_save, > diff --git a/target/riscv/machine.c b/target/riscv/machine.c > index bf203bffce..f96f5b4dfb 100644 > --- a/target/riscv/machine.c > +++ b/target/riscv/machine.c > @@ -497,6 +497,7 @@ static const VMStateDescription vmstate_mseccfg = { > > const VMStateDescription vmstate_riscv_cpu = { > .name = "cpu", > + .cpu_sync = true, > .version_id = 12, > .minimum_version_id = 12, > #ifdef CONFIG_KVM > diff --git a/target/s390x/machine.c b/target/s390x/machine.c > index b44fba43f1..411d563e25 100644 > --- a/target/s390x/machine.c > +++ b/target/s390x/machine.c > @@ -256,6 +256,7 @@ static const VMStateDescription vmstate_diag318 = { > > const VMStateDescription vmstate_s390_cpu = { > .name = "cpu", > + .cpu_sync = true, > .post_load = cpu_post_load, > .pre_save = cpu_pre_save, > .version_id = 4, > diff --git a/target/sh4/cpu.c b/target/sh4/cpu.c > index 3bbdee301d..c9c724551b 100644 > --- a/target/sh4/cpu.c > +++ b/target/sh4/cpu.c > @@ -285,6 +285,7 @@ static void superh_cpu_initfn(Object *obj) > #ifndef CONFIG_USER_ONLY > static const VMStateDescription vmstate_sh_cpu = { > .name = "cpu", > + .cpu_sync = true, > .unmigratable = 1, > }; > > diff --git a/target/sparc/machine.c b/target/sparc/machine.c > index 0d5d79b5e7..a45071b09a 100644 > --- a/target/sparc/machine.c > +++ b/target/sparc/machine.c > @@ -227,6 +227,7 @@ static int cpu_pre_save(void *opaque) > > const VMStateDescription vmstate_sparc_cpu = { > .name = "cpu", > + .cpu_sync = true, > .version_id = SPARC_VMSTATE_VER, > .minimum_version_id = SPARC_VMSTATE_VER, > .pre_save = cpu_pre_save, > diff --git a/target/xtensa/cpu.c b/target/xtensa/cpu.c > index 7c25b9ab70..c431450e58 100644 > --- a/target/xtensa/cpu.c > +++ b/target/xtensa/cpu.c > @@ -298,6 +298,7 @@ XtensaCPU *xtensa_cpu_create_with_clock(const char > *cpu_type, Clock *cpu_refclk) > #ifndef CONFIG_USER_ONLY > static const VMStateDescription vmstate_xtensa_cpu = { > .name = "cpu", > + .cpu_sync = true, > .unmigratable = 1, > }; > > -- > 2.55.0 > -- Peter Xu
