Re: [PATCH v3] KVM: LAPIC: Recalculate apic map in batch

[Igor Mammedov]

On Fri, 19 Jun 2020 16:10:43 +0200
Paolo Bonzini <pbonzini@xxxxxxxxxx> wrote:

> On 19/06/20 14:36, Igor Mammedov wrote:
> > qemu-kvm -m 2G -smp 4,maxcpus=8  -monitor stdio
> > (qemu) device_add qemu64-x86_64-cpu,socket-id=4,core-id=0,thread-id=0
> > 
> > in guest fails with:
> > 
> >  smpboot: do_boot_cpu failed(-1) to wakeup CPU#4
> > 
> > which makes me suspect that  INIT/SIPI wasn't delivered
> > 
> > Is it a know issue?
> >   
> 
> No, it isn't.  I'll revert.
> 
> Paolo
> 

Following fixes immediate issue:

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 34a7e0533dad..6dc177da19da 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -2567,6 +2567,7 @@ int kvm_apic_set_state(struct kvm_vcpu *vcpu, struct kvm_lapic_state *s)
        }
        memcpy(vcpu->arch.apic->regs, s->regs, sizeof(*s));
 
+       apic->vcpu->kvm->arch.apic_map_dirty = true;
        kvm_recalculate_apic_map(vcpu->kvm);
        kvm_apic_set_version(vcpu);

Problem is that during kvm_arch_vcpu_create() new vcpu is not visible to
kvm_recalculate_apic_map(), so whoever many times map update was called
during it, it didn't affect apic map.

What broke hotplug is that kvm_vcpu_ioctl_set_lapic -> kvm_apic_set_state,
which is called after new vcpu is visible, used to make an unconditional update
which pulled in the new vcpu, but with this patch the map update is gone
since state hasn't actuaaly changed, so we lost the one call of
kvm_recalculate_apic_map() which did actually matter.

It happens to work for vcpus present at boot just by luck
(BSP updates SPIV after all vcpus has been created which triggers kvm_recalculate_apic_map())

I'm not sending formal patch yet, since I have doubts wrt subj.

following sequence looks like a race that can cause lost map update events:

         cpu1                            cpu2
                             
                                apic_map_dirty = true     
  ------------------------------------------------------------   
                                kvm_recalculate_apic_map:
                                     pass check
                                         mutex_lock(&kvm->arch.apic_map_lock);
                                         if (!kvm->arch.apic_map_dirty)
                                     and in process of updating map
  -------------------------------------------------------------
    other calls to
       apic_map_dirty = true         might be too late for affected cpu
  -------------------------------------------------------------
                                     apic_map_dirty = false
  -------------------------------------------------------------
    kvm_recalculate_apic_map:
    bail out on
      if (!kvm->arch.apic_map_dirty)

it's safer to revert this patch for now like you have suggested earlier.

If you prefer to keep it, I'll post above fixup as a patch.