Re: [PATCH v2] hyperv: fix SynIC SINT assertion failure on guest reset

[Maciej S. Szmigiero]

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