| Message ID | 2257cac7474705fc35d74fcdff0566fb7dc0770e.1267467030.git.jan.kiszka@siemens.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Mar 01, 2010 at 07:10:30PM +0100, Jan Kiszka wrote: > This grand cleanup drops all reset and vmsave/load related > synchronization points in favor of four(!) generic hooks: > > - cpu_synchronize_all_states in qemu_savevm_state_complete > (initial sync from kernel before vmsave) > - cpu_synchronize_all_post_init in qemu_loadvm_state > (writeback after vmload) > - cpu_synchronize_all_post_init in main after machine init > - cpu_synchronize_all_post_reset in qemu_system_reset > (writeback after system reset) > > These writeback points + the existing one of VCPU exec after > cpu_synchronize_state map on three levels of writeback: > > - KVM_PUT_RUNTIME_STATE (during runtime, other VCPUs continue to run) > - KVM_PUT_RESET_STATE (on synchronous system reset, all VCPUs stopped) > - KVM_PUT_FULL_STATE (on init or vmload, all VCPUs stopped as well) > > This level is passed to the arch-specific VCPU state writing function > that will decide which concrete substates need to be written. That way, > no writer of load, save or reset functions that interact with in-kernel > KVM states will ever have to worry about synchronization again. That > also means that a lot of reasons for races, segfaults and deadlocks are > eliminated. > > cpu_synchronize_state remains untouched, just as Anthony suggested. We > continue to need it before reading or writing of VCPU states that are > also tracked by in-kernel KVM subsystems. > > Consequently, this patch removes many cpu_synchronize_state calls that > are now redundant, just like remaining explicit register syncs. > > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> Jan, This patch breaks system reset of WinXP.32 install (more easily reproducible without iothread enabled). Screenshot attached.
Marcelo Tosatti wrote: > On Mon, Mar 01, 2010 at 07:10:30PM +0100, Jan Kiszka wrote: >> This grand cleanup drops all reset and vmsave/load related >> synchronization points in favor of four(!) generic hooks: >> >> - cpu_synchronize_all_states in qemu_savevm_state_complete >> (initial sync from kernel before vmsave) >> - cpu_synchronize_all_post_init in qemu_loadvm_state >> (writeback after vmload) >> - cpu_synchronize_all_post_init in main after machine init >> - cpu_synchronize_all_post_reset in qemu_system_reset >> (writeback after system reset) >> >> These writeback points + the existing one of VCPU exec after >> cpu_synchronize_state map on three levels of writeback: >> >> - KVM_PUT_RUNTIME_STATE (during runtime, other VCPUs continue to run) >> - KVM_PUT_RESET_STATE (on synchronous system reset, all VCPUs stopped) >> - KVM_PUT_FULL_STATE (on init or vmload, all VCPUs stopped as well) >> >> This level is passed to the arch-specific VCPU state writing function >> that will decide which concrete substates need to be written. That way, >> no writer of load, save or reset functions that interact with in-kernel >> KVM states will ever have to worry about synchronization again. That >> also means that a lot of reasons for races, segfaults and deadlocks are >> eliminated. >> >> cpu_synchronize_state remains untouched, just as Anthony suggested. We >> continue to need it before reading or writing of VCPU states that are >> also tracked by in-kernel KVM subsystems. >> >> Consequently, this patch removes many cpu_synchronize_state calls that >> are now redundant, just like remaining explicit register syncs. >> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> > > Jan, > > This patch breaks system reset of WinXP.32 install (more easily > reproducible without iothread enabled). > > Screenshot attached. > Strange - no issues with qemu-kvm? Any special command line switch? /me goes scrounging for some installation XP32 CD in the meantime... Jan
On Tue, Mar 02, 2010 at 09:00:04AM +0100, Jan Kiszka wrote: > Marcelo Tosatti wrote: > > On Mon, Mar 01, 2010 at 07:10:30PM +0100, Jan Kiszka wrote: > >> This grand cleanup drops all reset and vmsave/load related > >> synchronization points in favor of four(!) generic hooks: > >> > >> - cpu_synchronize_all_states in qemu_savevm_state_complete > >> (initial sync from kernel before vmsave) > >> - cpu_synchronize_all_post_init in qemu_loadvm_state > >> (writeback after vmload) > >> - cpu_synchronize_all_post_init in main after machine init > >> - cpu_synchronize_all_post_reset in qemu_system_reset > >> (writeback after system reset) > >> > >> These writeback points + the existing one of VCPU exec after > >> cpu_synchronize_state map on three levels of writeback: > >> > >> - KVM_PUT_RUNTIME_STATE (during runtime, other VCPUs continue to run) > >> - KVM_PUT_RESET_STATE (on synchronous system reset, all VCPUs stopped) > >> - KVM_PUT_FULL_STATE (on init or vmload, all VCPUs stopped as well) > >> > >> This level is passed to the arch-specific VCPU state writing function > >> that will decide which concrete substates need to be written. That way, > >> no writer of load, save or reset functions that interact with in-kernel > >> KVM states will ever have to worry about synchronization again. That > >> also means that a lot of reasons for races, segfaults and deadlocks are > >> eliminated. > >> > >> cpu_synchronize_state remains untouched, just as Anthony suggested. We > >> continue to need it before reading or writing of VCPU states that are > >> also tracked by in-kernel KVM subsystems. > >> > >> Consequently, this patch removes many cpu_synchronize_state calls that > >> are now redundant, just like remaining explicit register syncs. > >> > >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> > > > > Jan, > > > > This patch breaks system reset of WinXP.32 install (more easily > > reproducible without iothread enabled). > > > > Screenshot attached. > > > > Strange - no issues with qemu-kvm? Any special command line switch? /me > goes scrounging for some installation XP32 CD in the meantime... No issues with qemu-kvm. Could not spot anything obvious. > > Jan >
Marcelo Tosatti wrote: > On Tue, Mar 02, 2010 at 09:00:04AM +0100, Jan Kiszka wrote: >> Marcelo Tosatti wrote: >>> On Mon, Mar 01, 2010 at 07:10:30PM +0100, Jan Kiszka wrote: >>>> This grand cleanup drops all reset and vmsave/load related >>>> synchronization points in favor of four(!) generic hooks: >>>> >>>> - cpu_synchronize_all_states in qemu_savevm_state_complete >>>> (initial sync from kernel before vmsave) >>>> - cpu_synchronize_all_post_init in qemu_loadvm_state >>>> (writeback after vmload) >>>> - cpu_synchronize_all_post_init in main after machine init >>>> - cpu_synchronize_all_post_reset in qemu_system_reset >>>> (writeback after system reset) >>>> >>>> These writeback points + the existing one of VCPU exec after >>>> cpu_synchronize_state map on three levels of writeback: >>>> >>>> - KVM_PUT_RUNTIME_STATE (during runtime, other VCPUs continue to run) >>>> - KVM_PUT_RESET_STATE (on synchronous system reset, all VCPUs stopped) >>>> - KVM_PUT_FULL_STATE (on init or vmload, all VCPUs stopped as well) >>>> >>>> This level is passed to the arch-specific VCPU state writing function >>>> that will decide which concrete substates need to be written. That way, >>>> no writer of load, save or reset functions that interact with in-kernel >>>> KVM states will ever have to worry about synchronization again. That >>>> also means that a lot of reasons for races, segfaults and deadlocks are >>>> eliminated. >>>> >>>> cpu_synchronize_state remains untouched, just as Anthony suggested. We >>>> continue to need it before reading or writing of VCPU states that are >>>> also tracked by in-kernel KVM subsystems. >>>> >>>> Consequently, this patch removes many cpu_synchronize_state calls that >>>> are now redundant, just like remaining explicit register syncs. >>>> >>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> >>> Jan, >>> >>> This patch breaks system reset of WinXP.32 install (more easily >>> reproducible without iothread enabled). >>> >>> Screenshot attached. >>> >> Strange - no issues with qemu-kvm? Any special command line switch? /me >> goes scrounging for some installation XP32 CD in the meantime... > > No issues with qemu-kvm. Could not spot anything obvious. > And, of course, my WinXP installation did not trigger any reset issue, even in non-iothreaded mode. :( Jan
On 03/02/2010 02:14 AM, Marcelo Tosatti wrote: > On Mon, Mar 01, 2010 at 07:10:30PM +0100, Jan Kiszka wrote: > >> This grand cleanup drops all reset and vmsave/load related >> synchronization points in favor of four(!) generic hooks: >> >> - cpu_synchronize_all_states in qemu_savevm_state_complete >> (initial sync from kernel before vmsave) >> - cpu_synchronize_all_post_init in qemu_loadvm_state >> (writeback after vmload) >> - cpu_synchronize_all_post_init in main after machine init >> - cpu_synchronize_all_post_reset in qemu_system_reset >> (writeback after system reset) >> >> These writeback points + the existing one of VCPU exec after >> cpu_synchronize_state map on three levels of writeback: >> >> - KVM_PUT_RUNTIME_STATE (during runtime, other VCPUs continue to run) >> - KVM_PUT_RESET_STATE (on synchronous system reset, all VCPUs stopped) >> - KVM_PUT_FULL_STATE (on init or vmload, all VCPUs stopped as well) >> >> This level is passed to the arch-specific VCPU state writing function >> that will decide which concrete substates need to be written. That way, >> no writer of load, save or reset functions that interact with in-kernel >> KVM states will ever have to worry about synchronization again. That >> also means that a lot of reasons for races, segfaults and deadlocks are >> eliminated. >> >> cpu_synchronize_state remains untouched, just as Anthony suggested. We >> continue to need it before reading or writing of VCPU states that are >> also tracked by in-kernel KVM subsystems. >> >> Consequently, this patch removes many cpu_synchronize_state calls that >> are now redundant, just like remaining explicit register syncs. >> >> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com> >> > Jan, > > This patch breaks system reset of WinXP.32 install (more easily > reproducible without iothread enabled). > > What's the conclusion here? The patch is innocent of the regression?
On Thu, Mar 11, 2010 at 10:32:50AM +0200, Avi Kivity wrote: > On 03/02/2010 02:14 AM, Marcelo Tosatti wrote: > >On Mon, Mar 01, 2010 at 07:10:30PM +0100, Jan Kiszka wrote: > >>This grand cleanup drops all reset and vmsave/load related > >>synchronization points in favor of four(!) generic hooks: > >> > >>- cpu_synchronize_all_states in qemu_savevm_state_complete > >> (initial sync from kernel before vmsave) > >>- cpu_synchronize_all_post_init in qemu_loadvm_state > >> (writeback after vmload) > >>- cpu_synchronize_all_post_init in main after machine init > >>- cpu_synchronize_all_post_reset in qemu_system_reset > >> (writeback after system reset) > >> > >>These writeback points + the existing one of VCPU exec after > >>cpu_synchronize_state map on three levels of writeback: > >> > >>- KVM_PUT_RUNTIME_STATE (during runtime, other VCPUs continue to run) > >>- KVM_PUT_RESET_STATE (on synchronous system reset, all VCPUs stopped) > >>- KVM_PUT_FULL_STATE (on init or vmload, all VCPUs stopped as well) > >> > >>This level is passed to the arch-specific VCPU state writing function > >>that will decide which concrete substates need to be written. That way, > >>no writer of load, save or reset functions that interact with in-kernel > >>KVM states will ever have to worry about synchronization again. That > >>also means that a lot of reasons for races, segfaults and deadlocks are > >>eliminated. > >> > >>cpu_synchronize_state remains untouched, just as Anthony suggested. We > >>continue to need it before reading or writing of VCPU states that are > >>also tracked by in-kernel KVM subsystems. > >> > >>Consequently, this patch removes many cpu_synchronize_state calls that > >>are now redundant, just like remaining explicit register syncs. > >> > >>Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com> > >Jan, > > > >This patch breaks system reset of WinXP.32 install (more easily > >reproducible without iothread enabled). > > > > What's the conclusion here? The patch is innocent of the regression? Yes, it is. The problem was caused by a recent seabios change, now fixed.
diff --git a/exec.c b/exec.c index 8616ff9..50a2e46 100644 --- a/exec.c +++ b/exec.c @@ -518,21 +518,6 @@ void cpu_exec_init_all(unsigned long tb_size) #if defined(CPU_SAVE_VERSION) && !defined(CONFIG_USER_ONLY) -static void cpu_common_pre_save(void *opaque) -{ - CPUState *env = opaque; - - cpu_synchronize_state(env); -} - -static int cpu_common_pre_load(void *opaque) -{ - CPUState *env = opaque; - - cpu_synchronize_state(env); - return 0; -} - static int cpu_common_post_load(void *opaque, int version_id) { CPUState *env = opaque; @@ -550,8 +535,6 @@ static const VMStateDescription vmstate_cpu_common = { .version_id = 1, .minimum_version_id = 1, .minimum_version_id_old = 1, - .pre_save = cpu_common_pre_save, - .pre_load = cpu_common_pre_load, .post_load = cpu_common_post_load, .fields = (VMStateField []) { VMSTATE_UINT32(halted, CPUState), diff --git a/hw/apic.c b/hw/apic.c index 87e7dc0..3c90f4c 100644 --- a/hw/apic.c +++ b/hw/apic.c @@ -938,8 +938,6 @@ static void apic_reset(void *opaque) APICState *s = opaque; int bsp; - cpu_synchronize_state(s->cpu_env); - bsp = cpu_is_bsp(s->cpu_env); s->apicbase = 0xfee00000 | (bsp ? MSR_IA32_APICBASE_BSP : 0) | MSR_IA32_APICBASE_ENABLE; diff --git a/hw/ppc_newworld.c b/hw/ppc_newworld.c index bc86c85..d4f9013 100644 --- a/hw/ppc_newworld.c +++ b/hw/ppc_newworld.c @@ -167,9 +167,6 @@ static void ppc_core99_init (ram_addr_t ram_size, envs[i] = env; } - /* Make sure all register sets take effect */ - cpu_synchronize_state(env); - /* allocate RAM */ ram_offset = qemu_ram_alloc(ram_size); cpu_register_physical_memory(0, ram_size, ram_offset); diff --git a/hw/ppc_oldworld.c b/hw/ppc_oldworld.c index 04a7835..93c95ba 100644 --- a/hw/ppc_oldworld.c +++ b/hw/ppc_oldworld.c @@ -165,9 +165,6 @@ static void ppc_heathrow_init (ram_addr_t ram_size, envs[i] = env; } - /* Make sure all register sets take effect */ - cpu_synchronize_state(env); - /* allocate RAM */ if (ram_size > (2047 << 20)) { fprintf(stderr, diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c index 3582728..ad3386f 100644 --- a/hw/s390-virtio.c +++ b/hw/s390-virtio.c @@ -185,7 +185,6 @@ static void s390_init(ram_addr_t ram_size, exit(1); } - cpu_synchronize_state(env); env->psw.addr = KERN_IMAGE_START; env->psw.mask = 0x0000000180000000ULL; } diff --git a/kvm-all.c b/kvm-all.c index 2f7e33a..534ead0 100644 --- a/kvm-all.c +++ b/kvm-all.c @@ -156,10 +156,6 @@ static void kvm_reset_vcpu(void *opaque) CPUState *env = opaque; kvm_arch_reset_vcpu(env); - if (kvm_arch_put_registers(env)) { - fprintf(stderr, "Fatal: kvm vcpu reset failed\n"); - abort(); - } } int kvm_irqchip_in_kernel(void) @@ -214,7 +210,6 @@ int kvm_init_vcpu(CPUState *env) if (ret == 0) { qemu_register_reset(kvm_reset_vcpu, env); kvm_arch_reset_vcpu(env); - ret = kvm_arch_put_registers(env); } err: return ret; @@ -753,6 +748,18 @@ void kvm_cpu_synchronize_state(CPUState *env) } } +void kvm_cpu_synchronize_post_reset(CPUState *env) +{ + kvm_arch_put_registers(env, KVM_PUT_RESET_STATE); + env->kvm_vcpu_dirty = 0; +} + +void kvm_cpu_synchronize_post_init(CPUState *env) +{ + kvm_arch_put_registers(env, KVM_PUT_FULL_STATE); + env->kvm_vcpu_dirty = 0; +} + int kvm_cpu_exec(CPUState *env) { struct kvm_run *run = env->kvm_run; @@ -770,7 +777,7 @@ int kvm_cpu_exec(CPUState *env) #endif if (env->kvm_vcpu_dirty) { - kvm_arch_put_registers(env); + kvm_arch_put_registers(env, KVM_PUT_RUNTIME_STATE); env->kvm_vcpu_dirty = 0; } diff --git a/kvm.h b/kvm.h index a602e45..b2937b9 100644 --- a/kvm.h +++ b/kvm.h @@ -82,7 +82,14 @@ int kvm_arch_pre_run(CPUState *env, struct kvm_run *run); int kvm_arch_get_registers(CPUState *env); -int kvm_arch_put_registers(CPUState *env); +/* state subset only touched by the VCPU itself during runtime */ +#define KVM_PUT_RUNTIME_STATE 1 +/* state subset modified during VCPU reset */ +#define KVM_PUT_RESET_STATE 2 +/* full state set, modified during initialization or on vmload */ +#define KVM_PUT_FULL_STATE 3 + +int kvm_arch_put_registers(CPUState *env, int level); int kvm_arch_init(KVMState *s, int smp_cpus); @@ -126,6 +133,8 @@ int kvm_check_extension(KVMState *s, unsigned int extension); uint32_t kvm_arch_get_supported_cpuid(CPUState *env, uint32_t function, int reg); void kvm_cpu_synchronize_state(CPUState *env); +void kvm_cpu_synchronize_post_reset(CPUState *env); +void kvm_cpu_synchronize_post_init(CPUState *env); /* generic hooks - to be moved/refactored once there are more users */ @@ -136,4 +145,18 @@ static inline void cpu_synchronize_state(CPUState *env) } } +static inline void cpu_synchronize_post_reset(CPUState *env) +{ + if (kvm_enabled()) { + kvm_cpu_synchronize_post_reset(env); + } +} + +static inline void cpu_synchronize_post_init(CPUState *env) +{ + if (kvm_enabled()) { + kvm_cpu_synchronize_post_init(env); + } +} + #endif diff --git a/savevm.c b/savevm.c index 4b58663..a6e774b 100644 --- a/savevm.c +++ b/savevm.c @@ -1345,6 +1345,8 @@ int qemu_savevm_state_complete(Monitor *mon, QEMUFile *f) { SaveStateEntry *se; + cpu_synchronize_all_states(); + QTAILQ_FOREACH(se, &savevm_handlers, entry) { if (se->save_live_state == NULL) continue; @@ -1545,6 +1547,8 @@ int qemu_loadvm_state(QEMUFile *f) } } + cpu_synchronize_all_post_init(); + ret = 0; out: diff --git a/sysemu.h b/sysemu.h index 8ba618e..d77344c 100644 --- a/sysemu.h +++ b/sysemu.h @@ -58,6 +58,10 @@ int load_vmstate(Monitor *mon, const char *name); void do_delvm(Monitor *mon, const QDict *qdict); void do_info_snapshots(Monitor *mon); +void cpu_synchronize_all_states(void); +void cpu_synchronize_all_post_reset(void); +void cpu_synchronize_all_post_init(void); + void qemu_announce_self(void); void main_loop_wait(int timeout); diff --git a/target-i386/kvm.c b/target-i386/kvm.c index 2365ca3..a4767b2 100644 --- a/target-i386/kvm.c +++ b/target-i386/kvm.c @@ -882,7 +882,7 @@ static int kvm_guest_debug_workarounds(CPUState *env) return ret; } -int kvm_arch_put_registers(CPUState *env) +int kvm_arch_put_registers(CPUState *env, int level) { int ret; diff --git a/target-i386/machine.c b/target-i386/machine.c index 8770491..b547e2a 100644 --- a/target-i386/machine.c +++ b/target-i386/machine.c @@ -321,8 +321,6 @@ static void cpu_pre_save(void *opaque) CPUState *env = opaque; int i; - cpu_synchronize_state(env); - /* FPU */ env->fpus_vmstate = (env->fpus & ~0x3800) | (env->fpstt & 0x7) << 11; env->fptag_vmstate = 0; @@ -337,14 +335,6 @@ static void cpu_pre_save(void *opaque) #endif } -static int cpu_pre_load(void *opaque) -{ - CPUState *env = opaque; - - cpu_synchronize_state(env); - return 0; -} - static int cpu_post_load(void *opaque, int version_id) { CPUState *env = opaque; @@ -373,7 +363,6 @@ static const VMStateDescription vmstate_cpu = { .minimum_version_id = 3, .minimum_version_id_old = 3, .pre_save = cpu_pre_save, - .pre_load = cpu_pre_load, .post_load = cpu_post_load, .fields = (VMStateField []) { VMSTATE_UINTTL_ARRAY(regs, CPUState, CPU_NB_REGS), diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c index 8ad0037..aa3d432 100644 --- a/target-ppc/kvm.c +++ b/target-ppc/kvm.c @@ -73,7 +73,7 @@ void kvm_arch_reset_vcpu(CPUState *env) { } -int kvm_arch_put_registers(CPUState *env) +int kvm_arch_put_registers(CPUState *env, int level) { struct kvm_regs regs; int ret; diff --git a/target-ppc/machine.c b/target-ppc/machine.c index 4897c8a..67de951 100644 --- a/target-ppc/machine.c +++ b/target-ppc/machine.c @@ -7,8 +7,6 @@ void cpu_save(QEMUFile *f, void *opaque) CPUState *env = (CPUState *)opaque; unsigned int i, j; - cpu_synchronize_state(env); - for (i = 0; i < 32; i++) qemu_put_betls(f, &env->gpr[i]); #if !defined(TARGET_PPC64) @@ -96,8 +94,6 @@ int cpu_load(QEMUFile *f, void *opaque, int version_id) CPUState *env = (CPUState *)opaque; unsigned int i, j; - cpu_synchronize_state(env); - for (i = 0; i < 32; i++) qemu_get_betls(f, &env->gpr[i]); #if !defined(TARGET_PPC64) diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c index 0199a65..72e77b0 100644 --- a/target-s390x/kvm.c +++ b/target-s390x/kvm.c @@ -91,7 +91,7 @@ void kvm_arch_reset_vcpu(CPUState *env) /* FIXME: add code to reset vcpu. */ } -int kvm_arch_put_registers(CPUState *env) +int kvm_arch_put_registers(CPUState *env, int level) { struct kvm_regs regs; int ret; @@ -296,7 +296,6 @@ static int handle_hypercall(CPUState *env, struct kvm_run *run) cpu_synchronize_state(env); r = s390_virtio_hypercall(env); - kvm_arch_put_registers(env); return r; } diff --git a/vl.c b/vl.c index 66e477a..67143a7 100644 --- a/vl.c +++ b/vl.c @@ -3002,6 +3002,33 @@ static void nographic_update(void *opaque) qemu_mod_timer(nographic_timer, interval + qemu_get_clock(rt_clock)); } +void cpu_synchronize_all_states(void) +{ + CPUState *cpu; + + for (cpu = first_cpu; cpu; cpu = cpu->next_cpu) { + cpu_synchronize_state(cpu); + } +} + +void cpu_synchronize_all_post_reset(void) +{ + CPUState *cpu; + + for (cpu = first_cpu; cpu; cpu = cpu->next_cpu) { + cpu_synchronize_post_reset(cpu); + } +} + +void cpu_synchronize_all_post_init(void) +{ + CPUState *cpu; + + for (cpu = first_cpu; cpu; cpu = cpu->next_cpu) { + cpu_synchronize_post_init(cpu); + } +} + struct vm_change_state_entry { VMChangeStateHandler *cb; void *opaque; @@ -3143,6 +3170,7 @@ void qemu_system_reset(void) QTAILQ_FOREACH_SAFE(re, &reset_handlers, entry, nre) { re->func(re->opaque); } + cpu_synchronize_all_post_reset(); } void qemu_system_reset_request(void) @@ -5927,6 +5955,7 @@ int main(int argc, char **argv, char **envp) machine->init(ram_size, boot_devices, kernel_filename, kernel_cmdline, initrd_filename, cpu_model); + cpu_synchronize_all_post_init(); #ifndef _WIN32 /* must be after terminal init, SDL library changes signal handlers */
This grand cleanup drops all reset and vmsave/load related synchronization points in favor of four(!) generic hooks: - cpu_synchronize_all_states in qemu_savevm_state_complete (initial sync from kernel before vmsave) - cpu_synchronize_all_post_init in qemu_loadvm_state (writeback after vmload) - cpu_synchronize_all_post_init in main after machine init - cpu_synchronize_all_post_reset in qemu_system_reset (writeback after system reset) These writeback points + the existing one of VCPU exec after cpu_synchronize_state map on three levels of writeback: - KVM_PUT_RUNTIME_STATE (during runtime, other VCPUs continue to run) - KVM_PUT_RESET_STATE (on synchronous system reset, all VCPUs stopped) - KVM_PUT_FULL_STATE (on init or vmload, all VCPUs stopped as well) This level is passed to the arch-specific VCPU state writing function that will decide which concrete substates need to be written. That way, no writer of load, save or reset functions that interact with in-kernel KVM states will ever have to worry about synchronization again. That also means that a lot of reasons for races, segfaults and deadlocks are eliminated. cpu_synchronize_state remains untouched, just as Anthony suggested. We continue to need it before reading or writing of VCPU states that are also tracked by in-kernel KVM subsystems. Consequently, this patch removes many cpu_synchronize_state calls that are now redundant, just like remaining explicit register syncs. Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> --- exec.c | 17 ----------------- hw/apic.c | 2 -- hw/ppc_newworld.c | 3 --- hw/ppc_oldworld.c | 3 --- hw/s390-virtio.c | 1 - kvm-all.c | 19 +++++++++++++------ kvm.h | 25 ++++++++++++++++++++++++- savevm.c | 4 ++++ sysemu.h | 4 ++++ target-i386/kvm.c | 2 +- target-i386/machine.c | 11 ----------- target-ppc/kvm.c | 2 +- target-ppc/machine.c | 4 ---- target-s390x/kvm.c | 3 +-- vl.c | 29 +++++++++++++++++++++++++++++ 15 files changed, 77 insertions(+), 52 deletions(-)