Re: [PATCH RFC] KVM: Fix race in apic->pending_events processing

[Paolo Bonzini <pbonzini@redhat.com>]

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

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.

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.
Setting the mp_state to anything bug SIPI_RECEIVED should clear the SIPI
event.

Paolo

>>    event sent           event processed            pending_events
>>      INIT                                                 INIT
>>      SIPI                                               INIT|SIPI
>>                               INIT                        SIPI
>>                               SIPI                         0
>>      INIT                                                 INIT
>>      SIPI                                               INIT|SIPI
>>                               INIT                        SIPI
>>                               SIPI                         0