| Message ID | 2ca557c8eb947112103168a9da3033ac5dc6ab99.1664291365.git.maciej.szmigiero@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RESEND] hyperv: fix SynIC SINT assertion failure on guest reset | expand |
Why does this need to be a virtual function, if it is the same for all CPUs (it differs between system and user-mode emulation, but it is never called by user-mode emulation so that does not matter)? Paolo Il mar 27 set 2022, 17:12 Maciej S. Szmigiero <mail@maciej.szmigiero.name> ha scritto: > From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com> > > Resetting a guest that has Hyper-V VMBus support enabled triggers a QEMU > assertion failure: > hw/hyperv/hyperv.c:131: synic_reset: Assertion > `QLIST_EMPTY(&synic->sint_routes)' failed. > > This happens both on normal guest reboot or when using "system_reset" HMP > command. > > The failing assertion was introduced by commit 64ddecc88bcf ("hyperv: > SControl is optional to enable SynIc") > to catch dangling SINT routes on SynIC reset. > > The root cause of this problem is that the SynIC itself is reset before > devices using SINT routes have chance to clean up these routes. > > Since there seems to be no existing mechanism to force reset callbacks (or > methods) to be executed in specific order let's use a similar method that > is already used to reset another interrupt controller (APIC) after devices > have been reset - by invoking the SynIC reset from the machine reset > handler via a new "after_reset" X86 CPU method. > > Fixes: 64ddecc88bcf ("hyperv: SControl is optional to enable SynIc") # > exposed the bug > Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com> > --- > hw/i386/pc.c | 6 ++++++ > target/i386/cpu-qom.h | 2 ++ > target/i386/cpu.c | 10 ++++++++++ > target/i386/kvm/hyperv.c | 4 ++++ > target/i386/kvm/kvm.c | 24 +++++++++++++++++------- > target/i386/kvm/kvm_i386.h | 1 + > 6 files changed, 40 insertions(+), 7 deletions(-) > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index 566accf7e6..e44f11efb3 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -1850,6 +1850,7 @@ static void pc_machine_reset(MachineState *machine) > { > CPUState *cs; > X86CPU *cpu; > + const X86CPUClass *cpuc; > > qemu_devices_reset(); > > @@ -1858,6 +1859,11 @@ static void pc_machine_reset(MachineState *machine) > */ > CPU_FOREACH(cs) { > cpu = X86_CPU(cs); > + cpuc = X86_CPU_GET_CLASS(cpu); > + > + if (cpuc->after_reset) { > + cpuc->after_reset(cpu); > + } > > if (cpu->apic_state) { > device_legacy_reset(cpu->apic_state); > diff --git a/target/i386/cpu-qom.h b/target/i386/cpu-qom.h > index c557a522e1..339d23006a 100644 > --- a/target/i386/cpu-qom.h > +++ b/target/i386/cpu-qom.h > @@ -43,6 +43,7 @@ typedef struct X86CPUModel X86CPUModel; > * @static_model: See CpuDefinitionInfo::static > * @parent_realize: The parent class' realize handler. > * @parent_reset: The parent class' reset handler. > + * @after_reset: Reset handler to be called only after all other devices > have been reset. > * > * An x86 CPU model or family. > */ > @@ -68,6 +69,7 @@ struct X86CPUClass { > DeviceRealize parent_realize; > DeviceUnrealize parent_unrealize; > DeviceReset parent_reset; > + void (*after_reset)(X86CPU *cpu); > }; > > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > index 1db1278a59..c908b944bd 100644 > --- a/target/i386/cpu.c > +++ b/target/i386/cpu.c > @@ -6034,6 +6034,15 @@ static void x86_cpu_reset(DeviceState *dev) > #endif > } > > +static void x86_cpu_after_reset(X86CPU *cpu) > +{ > +#ifndef CONFIG_USER_ONLY > + if (kvm_enabled()) { > + kvm_arch_after_reset_vcpu(cpu); > + } > +#endif > +} > + > static void mce_init(X86CPU *cpu) > { > CPUX86State *cenv = &cpu->env; > @@ -7099,6 +7108,7 @@ static void x86_cpu_common_class_init(ObjectClass > *oc, void *data) > device_class_set_props(dc, x86_cpu_properties); > > device_class_set_parent_reset(dc, x86_cpu_reset, &xcc->parent_reset); > + xcc->after_reset = x86_cpu_after_reset; > cc->reset_dump_flags = CPU_DUMP_FPU | CPU_DUMP_CCOP; > > cc->class_by_name = x86_cpu_class_by_name; > diff --git a/target/i386/kvm/hyperv.c b/target/i386/kvm/hyperv.c > index 9026ef3a81..e3ac978648 100644 > --- a/target/i386/kvm/hyperv.c > +++ b/target/i386/kvm/hyperv.c > @@ -23,6 +23,10 @@ int hyperv_x86_synic_add(X86CPU *cpu) > return 0; > } > > +/* > + * All devices possibly using SynIC have to be reset before calling this > to let > + * them remove their SINT routes first. > + */ > void hyperv_x86_synic_reset(X86CPU *cpu) > { > hyperv_synic_reset(CPU(cpu)); > diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c > index a1fd1f5379..774484c588 100644 > --- a/target/i386/kvm/kvm.c > +++ b/target/i386/kvm/kvm.c > @@ -2203,20 +2203,30 @@ void kvm_arch_reset_vcpu(X86CPU *cpu) > env->mp_state = KVM_MP_STATE_RUNNABLE; > } > > + /* enabled by default */ > + env->poll_control_msr = 1; > + > + kvm_init_nested_state(env); > + > + sev_es_set_reset_vector(CPU(cpu)); > +} > + > +void kvm_arch_after_reset_vcpu(X86CPU *cpu) > +{ > + CPUX86State *env = &cpu->env; > + int i; > + > + /* > + * Reset SynIC after all other devices have been reset to let them > remove > + * their SINT routes first. > + */ > if (hyperv_feat_enabled(cpu, HYPERV_FEAT_SYNIC)) { > - int i; > for (i = 0; i < ARRAY_SIZE(env->msr_hv_synic_sint); i++) { > env->msr_hv_synic_sint[i] = HV_SINT_MASKED; > } > > hyperv_x86_synic_reset(cpu); > } > - /* enabled by default */ > - env->poll_control_msr = 1; > - > - kvm_init_nested_state(env); > - > - sev_es_set_reset_vector(CPU(cpu)); > } > > void kvm_arch_do_init_vcpu(X86CPU *cpu) > diff --git a/target/i386/kvm/kvm_i386.h b/target/i386/kvm/kvm_i386.h > index 4124912c20..096a5dd781 100644 > --- a/target/i386/kvm/kvm_i386.h > +++ b/target/i386/kvm/kvm_i386.h > @@ -38,6 +38,7 @@ bool kvm_has_adjust_clock_stable(void); > bool kvm_has_exception_payload(void); > void kvm_synchronize_all_tsc(void); > void kvm_arch_reset_vcpu(X86CPU *cs); > +void kvm_arch_after_reset_vcpu(X86CPU *cpu); > void kvm_arch_do_init_vcpu(X86CPU *cs); > > void kvm_put_apicbase(X86CPU *cpu, uint64_t value); > >
On 28.09.2022 01:17, Paolo Bonzini wrote: > Why does this need to be a virtual function, if it is the same for all CPUs (it differs between system and user-mode emulation, but it is never called by user-mode emulation so that does not matter)? Will change the patch to directly call x86_cpu_after_reset() from pc_machine_reset() then. > > Paolo Thanks, Maciej > Il mar 27 set 2022, 17:12 Maciej S. Szmigiero <mail@maciej.szmigiero.name <mailto:mail@maciej.szmigiero.name>> ha scritto: > > From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com <mailto:maciej.szmigiero@oracle.com>> > > Resetting a guest that has Hyper-V VMBus support enabled triggers a QEMU > assertion failure: > hw/hyperv/hyperv.c:131: synic_reset: Assertion `QLIST_EMPTY(&synic->sint_routes)' failed. > > This happens both on normal guest reboot or when using "system_reset" HMP > command. > > The failing assertion was introduced by commit 64ddecc88bcf ("hyperv: SControl is optional to enable SynIc") > to catch dangling SINT routes on SynIC reset. > > The root cause of this problem is that the SynIC itself is reset before > devices using SINT routes have chance to clean up these routes. > > Since there seems to be no existing mechanism to force reset callbacks (or > methods) to be executed in specific order let's use a similar method that > is already used to reset another interrupt controller (APIC) after devices > have been reset - by invoking the SynIC reset from the machine reset > handler via a new "after_reset" X86 CPU method. > > Fixes: 64ddecc88bcf ("hyperv: SControl is optional to enable SynIc") # exposed the bug > Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com <mailto:maciej.szmigiero@oracle.com>> > --- > hw/i386/pc.c | 6 ++++++ > target/i386/cpu-qom.h | 2 ++ > target/i386/cpu.c | 10 ++++++++++ > target/i386/kvm/hyperv.c | 4 ++++ > target/i386/kvm/kvm.c | 24 +++++++++++++++++------- > target/i386/kvm/kvm_i386.h | 1 + > 6 files changed, 40 insertions(+), 7 deletions(-) > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index 566accf7e6..e44f11efb3 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -1850,6 +1850,7 @@ static void pc_machine_reset(MachineState *machine) > { > CPUState *cs; > X86CPU *cpu; > + const X86CPUClass *cpuc; > > qemu_devices_reset(); > > @@ -1858,6 +1859,11 @@ static void pc_machine_reset(MachineState *machine) > */ > CPU_FOREACH(cs) { > cpu = X86_CPU(cs); > + cpuc = X86_CPU_GET_CLASS(cpu); > + > + if (cpuc->after_reset) { > + cpuc->after_reset(cpu); > + } > > if (cpu->apic_state) { > device_legacy_reset(cpu->apic_state); > diff --git a/target/i386/cpu-qom.h b/target/i386/cpu-qom.h > index c557a522e1..339d23006a 100644 > --- a/target/i386/cpu-qom.h > +++ b/target/i386/cpu-qom.h > @@ -43,6 +43,7 @@ typedef struct X86CPUModel X86CPUModel; > * @static_model: See CpuDefinitionInfo::static > * @parent_realize: The parent class' realize handler. > * @parent_reset: The parent class' reset handler. > + * @after_reset: Reset handler to be called only after all other devices have been reset. > * > * An x86 CPU model or family. > */ > @@ -68,6 +69,7 @@ struct X86CPUClass { > DeviceRealize parent_realize; > DeviceUnrealize parent_unrealize; > DeviceReset parent_reset; > + void (*after_reset)(X86CPU *cpu); > }; > > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > index 1db1278a59..c908b944bd 100644 > --- a/target/i386/cpu.c > +++ b/target/i386/cpu.c > @@ -6034,6 +6034,15 @@ static void x86_cpu_reset(DeviceState *dev) > #endif > } > > +static void x86_cpu_after_reset(X86CPU *cpu) > +{ > +#ifndef CONFIG_USER_ONLY > + if (kvm_enabled()) { > + kvm_arch_after_reset_vcpu(cpu); > + } > +#endif > +} > + > static void mce_init(X86CPU *cpu) > { > CPUX86State *cenv = &cpu->env; > @@ -7099,6 +7108,7 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data) > device_class_set_props(dc, x86_cpu_properties); > > device_class_set_parent_reset(dc, x86_cpu_reset, &xcc->parent_reset); > + xcc->after_reset = x86_cpu_after_reset; > cc->reset_dump_flags = CPU_DUMP_FPU | CPU_DUMP_CCOP; > > cc->class_by_name = x86_cpu_class_by_name; > diff --git a/target/i386/kvm/hyperv.c b/target/i386/kvm/hyperv.c > index 9026ef3a81..e3ac978648 100644 > --- a/target/i386/kvm/hyperv.c > +++ b/target/i386/kvm/hyperv.c > @@ -23,6 +23,10 @@ int hyperv_x86_synic_add(X86CPU *cpu) > return 0; > } > > +/* > + * All devices possibly using SynIC have to be reset before calling this to let > + * them remove their SINT routes first. > + */ > void hyperv_x86_synic_reset(X86CPU *cpu) > { > hyperv_synic_reset(CPU(cpu)); > diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c > index a1fd1f5379..774484c588 100644 > --- a/target/i386/kvm/kvm.c > +++ b/target/i386/kvm/kvm.c > @@ -2203,20 +2203,30 @@ void kvm_arch_reset_vcpu(X86CPU *cpu) > env->mp_state = KVM_MP_STATE_RUNNABLE; > } > > + /* enabled by default */ > + env->poll_control_msr = 1; > + > + kvm_init_nested_state(env); > + > + sev_es_set_reset_vector(CPU(cpu)); > +} > + > +void kvm_arch_after_reset_vcpu(X86CPU *cpu) > +{ > + CPUX86State *env = &cpu->env; > + int i; > + > + /* > + * Reset SynIC after all other devices have been reset to let them remove > + * their SINT routes first. > + */ > if (hyperv_feat_enabled(cpu, HYPERV_FEAT_SYNIC)) { > - int i; > for (i = 0; i < ARRAY_SIZE(env->msr_hv_synic_sint); i++) { > env->msr_hv_synic_sint[i] = HV_SINT_MASKED; > } > > hyperv_x86_synic_reset(cpu); > } > - /* enabled by default */ > - env->poll_control_msr = 1; > - > - kvm_init_nested_state(env); > - > - sev_es_set_reset_vector(CPU(cpu)); > } > > void kvm_arch_do_init_vcpu(X86CPU *cpu) > diff --git a/target/i386/kvm/kvm_i386.h b/target/i386/kvm/kvm_i386.h > index 4124912c20..096a5dd781 100644 > --- a/target/i386/kvm/kvm_i386.h > +++ b/target/i386/kvm/kvm_i386.h > @@ -38,6 +38,7 @@ bool kvm_has_adjust_clock_stable(void); > bool kvm_has_exception_payload(void); > void kvm_synchronize_all_tsc(void); > void kvm_arch_reset_vcpu(X86CPU *cs); > +void kvm_arch_after_reset_vcpu(X86CPU *cpu); > void kvm_arch_do_init_vcpu(X86CPU *cs); > > void kvm_put_apicbase(X86CPU *cpu, uint64_t value); >
diff --git a/hw/i386/pc.c b/hw/i386/pc.c index 566accf7e6..e44f11efb3 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -1850,6 +1850,7 @@ static void pc_machine_reset(MachineState *machine) { CPUState *cs; X86CPU *cpu; + const X86CPUClass *cpuc; qemu_devices_reset(); @@ -1858,6 +1859,11 @@ static void pc_machine_reset(MachineState *machine) */ CPU_FOREACH(cs) { cpu = X86_CPU(cs); + cpuc = X86_CPU_GET_CLASS(cpu); + + if (cpuc->after_reset) { + cpuc->after_reset(cpu); + } if (cpu->apic_state) { device_legacy_reset(cpu->apic_state); diff --git a/target/i386/cpu-qom.h b/target/i386/cpu-qom.h index c557a522e1..339d23006a 100644 --- a/target/i386/cpu-qom.h +++ b/target/i386/cpu-qom.h @@ -43,6 +43,7 @@ typedef struct X86CPUModel X86CPUModel; * @static_model: See CpuDefinitionInfo::static * @parent_realize: The parent class' realize handler. * @parent_reset: The parent class' reset handler. + * @after_reset: Reset handler to be called only after all other devices have been reset. * * An x86 CPU model or family. */ @@ -68,6 +69,7 @@ struct X86CPUClass { DeviceRealize parent_realize; DeviceUnrealize parent_unrealize; DeviceReset parent_reset; + void (*after_reset)(X86CPU *cpu); }; diff --git a/target/i386/cpu.c b/target/i386/cpu.c index 1db1278a59..c908b944bd 100644 --- a/target/i386/cpu.c +++ b/target/i386/cpu.c @@ -6034,6 +6034,15 @@ static void x86_cpu_reset(DeviceState *dev) #endif } +static void x86_cpu_after_reset(X86CPU *cpu) +{ +#ifndef CONFIG_USER_ONLY + if (kvm_enabled()) { + kvm_arch_after_reset_vcpu(cpu); + } +#endif +} + static void mce_init(X86CPU *cpu) { CPUX86State *cenv = &cpu->env; @@ -7099,6 +7108,7 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data) device_class_set_props(dc, x86_cpu_properties); device_class_set_parent_reset(dc, x86_cpu_reset, &xcc->parent_reset); + xcc->after_reset = x86_cpu_after_reset; cc->reset_dump_flags = CPU_DUMP_FPU | CPU_DUMP_CCOP; cc->class_by_name = x86_cpu_class_by_name; diff --git a/target/i386/kvm/hyperv.c b/target/i386/kvm/hyperv.c index 9026ef3a81..e3ac978648 100644 --- a/target/i386/kvm/hyperv.c +++ b/target/i386/kvm/hyperv.c @@ -23,6 +23,10 @@ int hyperv_x86_synic_add(X86CPU *cpu) return 0; } +/* + * All devices possibly using SynIC have to be reset before calling this to let + * them remove their SINT routes first. + */ void hyperv_x86_synic_reset(X86CPU *cpu) { hyperv_synic_reset(CPU(cpu)); diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c index a1fd1f5379..774484c588 100644 --- a/target/i386/kvm/kvm.c +++ b/target/i386/kvm/kvm.c @@ -2203,20 +2203,30 @@ void kvm_arch_reset_vcpu(X86CPU *cpu) env->mp_state = KVM_MP_STATE_RUNNABLE; } + /* enabled by default */ + env->poll_control_msr = 1; + + kvm_init_nested_state(env); + + sev_es_set_reset_vector(CPU(cpu)); +} + +void kvm_arch_after_reset_vcpu(X86CPU *cpu) +{ + CPUX86State *env = &cpu->env; + int i; + + /* + * Reset SynIC after all other devices have been reset to let them remove + * their SINT routes first. + */ if (hyperv_feat_enabled(cpu, HYPERV_FEAT_SYNIC)) { - int i; for (i = 0; i < ARRAY_SIZE(env->msr_hv_synic_sint); i++) { env->msr_hv_synic_sint[i] = HV_SINT_MASKED; } hyperv_x86_synic_reset(cpu); } - /* enabled by default */ - env->poll_control_msr = 1; - - kvm_init_nested_state(env); - - sev_es_set_reset_vector(CPU(cpu)); } void kvm_arch_do_init_vcpu(X86CPU *cpu) diff --git a/target/i386/kvm/kvm_i386.h b/target/i386/kvm/kvm_i386.h index 4124912c20..096a5dd781 100644 --- a/target/i386/kvm/kvm_i386.h +++ b/target/i386/kvm/kvm_i386.h @@ -38,6 +38,7 @@ bool kvm_has_adjust_clock_stable(void); bool kvm_has_exception_payload(void); void kvm_synchronize_all_tsc(void); void kvm_arch_reset_vcpu(X86CPU *cs); +void kvm_arch_after_reset_vcpu(X86CPU *cpu); void kvm_arch_do_init_vcpu(X86CPU *cs); void kvm_put_apicbase(X86CPU *cpu, uint64_t value);