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

[Gleb Natapov <gleb@redhat.com>]

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.