| Message ID | 8474c6ca63bbbf85ac7721732a7bbdb033f7aa50.1664378882.git.maciej.szmigiero@oracle.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] hyperv: fix SynIC SINT assertion failure on guest reset | expand |
On 9/28/22 18:17, Maciej S. Szmigiero wrote: > 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 x86_cpu_after_reset() function co-located with > the existing x86_cpu_reset() in target/i386/cpu.c. > > Fixes: 64ddecc88bcf ("hyperv: SControl is optional to enable SynIc") # exposed the bug > Signed-off-by: Maciej S. Szmigiero<maciej.szmigiero@oracle.com> Thanks, looks good. hw/i386/microvm.c has to be adjusted too, what do you think of this: diff --git a/hw/i386/microvm.c b/hw/i386/microvm.c index dc929727dc..64eb6374ad 100644 --- a/hw/i386/microvm.c +++ b/hw/i386/microvm.c @@ -485,9 +485,7 @@ static void microvm_machine_reset(MachineState *machine) CPU_FOREACH(cs) { cpu = X86_CPU(cs); - if (cpu->apic_state) { - device_legacy_reset(cpu->apic_state); - } + x86_cpu_after_reset(cpu); } } diff --git a/hw/i386/pc.c b/hw/i386/pc.c index 655439fe62..15a854b149 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -1863,10 +1863,6 @@ static void pc_machine_reset(MachineState *machine) cpu = X86_CPU(cs); x86_cpu_after_reset(cpu); - - if (cpu->apic_state) { - device_legacy_reset(cpu->apic_state); - } } } diff --git a/target/i386/cpu.c b/target/i386/cpu.c index 27ee8c1ced..349bd5d048 100644 --- a/target/i386/cpu.c +++ b/target/i386/cpu.c @@ -6042,6 +6042,10 @@ void x86_cpu_after_reset(X86CPU *cpu) if (kvm_enabled()) { kvm_arch_after_reset_vcpu(cpu); } + + if (cpu->apic_state) { + device_legacy_reset(cpu->apic_state); + } #endif }
On 29.09.2022 19:13, Paolo Bonzini wrote: > On 9/28/22 18:17, Maciej S. Szmigiero wrote: >> 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 x86_cpu_after_reset() function co-located with >> the existing x86_cpu_reset() in target/i386/cpu.c. >> >> Fixes: 64ddecc88bcf ("hyperv: SControl is optional to enable SynIc") # exposed the bug >> Signed-off-by: Maciej S. Szmigiero<maciej.szmigiero@oracle.com> > > Thanks, looks good. > > hw/i386/microvm.c has to be adjusted too, You're right, I was misled by the fact that VMBus is only available on pc or q35, but obviously kvm_arch_after_reset_vcpu() has (or will have) other side effects, too. > what do you think of this: > diff --git a/hw/i386/microvm.c b/hw/i386/microvm.c > index dc929727dc..64eb6374ad 100644 > --- a/hw/i386/microvm.c > +++ b/hw/i386/microvm.c > @@ -485,9 +485,7 @@ static void microvm_machine_reset(MachineState *machine) > CPU_FOREACH(cs) { > cpu = X86_CPU(cs); > > - if (cpu->apic_state) { > - device_legacy_reset(cpu->apic_state); > - } > + x86_cpu_after_reset(cpu); > } > } > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index 655439fe62..15a854b149 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -1863,10 +1863,6 @@ static void pc_machine_reset(MachineState *machine) > cpu = X86_CPU(cs); > > x86_cpu_after_reset(cpu); > - > - if (cpu->apic_state) { > - device_legacy_reset(cpu->apic_state); > - } > } > } > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > index 27ee8c1ced..349bd5d048 100644 > --- a/target/i386/cpu.c > +++ b/target/i386/cpu.c > @@ -6042,6 +6042,10 @@ void x86_cpu_after_reset(X86CPU *cpu) > if (kvm_enabled()) { > kvm_arch_after_reset_vcpu(cpu); > } > + > + if (cpu->apic_state) { > + device_legacy_reset(cpu->apic_state); > + } > #endif > } > Definitely makes sense, will prepare a v3 tomorrow. Thanks, Maciej
diff --git a/hw/i386/pc.c b/hw/i386/pc.c index 566accf7e6..736a4f34b0 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -92,6 +92,7 @@ #include "hw/virtio/virtio-mem-pci.h" #include "hw/mem/memory-device.h" #include "sysemu/replay.h" +#include "target/i386/cpu.h" #include "qapi/qmp/qerror.h" #include "e820_memory_layout.h" #include "fw_cfg.h" @@ -1859,6 +1860,8 @@ static void pc_machine_reset(MachineState *machine) CPU_FOREACH(cs) { cpu = X86_CPU(cs); + x86_cpu_after_reset(cpu); + if (cpu->apic_state) { device_legacy_reset(cpu->apic_state); } diff --git a/target/i386/cpu.c b/target/i386/cpu.c index 1db1278a59..4ea605c3b0 100644 --- a/target/i386/cpu.c +++ b/target/i386/cpu.c @@ -6034,6 +6034,15 @@ static void x86_cpu_reset(DeviceState *dev) #endif } +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; diff --git a/target/i386/cpu.h b/target/i386/cpu.h index 82004b65b9..c67d98e1a9 100644 --- a/target/i386/cpu.h +++ b/target/i386/cpu.h @@ -2079,6 +2079,8 @@ typedef struct PropValue { } PropValue; void x86_cpu_apply_props(X86CPU *cpu, PropValue *props); +void x86_cpu_after_reset(X86CPU *cpu); + uint32_t cpu_x86_virtual_addr_width(CPUX86State *env); /* cpu.c other functions (cpuid) */ 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);