On Fri, May 31, 2013 at 10:48:32AM +0200, Paolo Bonzini wrote: > Il 31/05/2013 06:36, Gleb Natapov ha scritto: > > In my commit message there is two INITs in a row: > > vpu0: vcpu1: > > set INIT > > test_and_clear_bit(KVM_APIC_INIT) > > process INIT > > set INIT > > set SIPI > > test_and_clear_bit(KVM_APIC_SIPI) > > process SIPI > > > > Two INITs before SIPI are essential to trigger the bug > > I see now. Let's draw pending_events as well: > > event sent event processed pending_events > INIT INIT > INIT 0 > INIT INIT > SIPI INIT|SIPI > SIPI INIT > INIT 0 > > Events are reordered, there is indeed a bug if the second INIT comes at > just the right time. With your patch: > > event sent event processed pending_events > INIT INIT > INIT 0 > INIT INIT > SIPI INIT|SIPI > SIPI, failed cmpxchg INIT|SIPI > INIT SIPI > SIPI SIPI > This is incorrect. cmpxchg will fail only if another INIT cames after SIPI. Why would it fail? > The patch introduces a spurious SIPI, that's worse than coalescing. > With my patch: > > event sent event processed pending_events > INIT INIT > INIT 0 > INIT INIT > SIPI INIT|SIPI > (failed cmpxchg) INIT|SIPI > INIT SIPI > SIPI 0 > > My patch looks better to me for this scenario. > > > and coincidentally this is what spec advices to do. > > The spec advises INIT-SIPI-SIPI, not INIT-INIT-SIPI. This is because > the first INIT may have been processed late, and SIPIs are masked if not > in wait-for-SIPI state. You need to send the second just in case. It > is not needed in KVM because INITs effectively unmask the SIPI > immediately, even though the INIT may take place a bit later. > OK, I said this from memory since I cannot check the spec now. In this case we need to fix unit test too. > The INIT-SIPI-SIPI sequence is handled correctly by KVM thanks to the > check on the mp-state. But your patch breaks another corner case: > > event sent event processed pending_events > INIT INIT > INIT 0 > SIPI SIPI > SIPI 0 > SIPI SIPI > ignored SIPI SIPI > > set_mp_state(INIT_RECEIVED) SIPI > SIPI 0 > > With my patch, or no patch at all: > > event sent event processed pending_events > INIT INIT > INIT 0 > SIPI SIPI > SIPI 0 > SIPI SIPI > ignored SIPI 0 > set_mp_state(INIT_RECEIVED) 0 > > Though perhaps the real bug here is in kvm_arch_vcpu_ioctl_setmp_state. Looks like it, also in my patch we can always call cmpxchg to clear SIPI. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
