Re: [Qemu-devel] [BUG/RFC] INIT IPI lost when VM starts

[Gonglei (Arei)]

Hi Paolo,

What's your opinion about this patch? We found it just before finishing patches 
for the past two days.


Thanks,
-Gonglei


> -----Original Message-----
> From: address@hidden [mailto:address@hidden On
> Behalf Of Herongguang (Stephen)
> Sent: Thursday, April 06, 2017 9:47 AM
> To: Paolo Bonzini; address@hidden; address@hidden;
> address@hidden; address@hidden; address@hidden;
> wangxin (U); Huangweidong (C)
> Subject: Re: [BUG/RFC] INIT IPI lost when VM starts
> 
> 
> 
> On 2017/4/6 0:16, Paolo Bonzini wrote:
> >
> > On 20/03/2017 15:21, Herongguang (Stephen) wrote:
> >> We encountered a problem that when a domain starts, seabios failed to
> >> online a vCPU.
> >>
> >> After investigation, we found that the reason is in kvm-kmod,
> >> KVM_APIC_INIT bit in
> >> vcpu->arch.apic->pending_events was overwritten by qemu, and thus an
> >> INIT IPI sent
> >> to AP was lost. Qemu does this since libvirtd sends a ‘query-cpus’ qmp
> >> command to qemu
> >> on VM start.
> >>
> >> In qemu, qmp_query_cpus-> cpu_synchronize_state->
> >> kvm_cpu_synchronize_state->
> >> do_kvm_cpu_synchronize_state, qemu gets registers/vcpu_events from
> >> kvm-kmod and
> >> sets cpu->kvm_vcpu_dirty to true, and vcpu thread in qemu will call
> >> kvm_arch_put_registers if cpu->kvm_vcpu_dirty is true, thus
> >> pending_events is
> >> overwritten by qemu.
> >>
> >> I think there is no need for qemu to set cpu->kvm_vcpu_dirty to true
> >> after ‘query-cpus’,
> >> and  kvm-kmod should not clear KVM_APIC_INIT unconditionally. And I am
> >> not sure whether
> >> it is OK for qemu to set cpu->kvm_vcpu_dirty in
> >> do_kvm_cpu_synchronize_state in each caller.
> >>
> >> What’s your opinion?
> > Hi Rongguang,
> >
> > sorry for the late response.
> >
> > Where exactly is KVM_APIC_INIT dropped?  kvm_get_mp_state does clear
> the
> > bit, but the result of the INIT is stored in mp_state.
> 
> It's dropped in KVM_SET_VCPU_EVENTS, see below.
> 
> >
> > kvm_get_vcpu_events is called after kvm_get_mp_state; it retrieves
> > KVM_APIC_INIT in events.smi.latched_init and kvm_set_vcpu_events passes
> > it back.  Maybe it should ignore events.smi.latched_init if not in SMM,
> > but I would like to understand the exact sequence of events.
> 
> time0:
> vcpu1:
> qmp_query_cpus-> cpu_synchronize_state-> kvm_cpu_synchronize_state->
>  > do_kvm_cpu_synchronize_state(and set vcpu1's cpu->kvm_vcpu_dirty to
> true)-> kvm_arch_get_registers(KVM_APIC_INIT bit in
> vcpu->arch.apic->pending_events was not set)
> 
> time1:
> vcpu0:
> send INIT-SIPI to all AP->(in vcpu 0's
> context)__apic_accept_irq(KVM_APIC_INIT bit in vcpu1's
> arch.apic->pending_events is set)
> 
> time2:
> vcpu1:
> kvm_cpu_exec->(if cpu->kvm_vcpu_dirty is
> true)kvm_arch_put_registers->kvm_put_vcpu_events(overwritten
> KVM_APIC_INIT bit in vcpu->arch.apic->pending_events!)
> 
> So it's a race between vcpu1 get/put registers with kvm/other vcpus changing
> vcpu1's status/structure fields in the mean time, I am in worry of if there 
> are
> other fields may be overwritten,
> sipi_vector is one.
> 
> also see:
> https://www.mail-archive.com/address@hidden/msg438675.html
> 
> > Thanks,
> >
> > paolo
> >
> > .
> >
>