[Qemu-devel] 答复: Re: [PATCH] target-i386:kvm_get/put_vcpu_events don't handlesipi_vector

[Qemu-devel] 答复: Re: [PATCH] target-i386:kvm_get/put_vcpu_events don't handlesipi_vector

[peng.hao2]

发件人： <pbonzini@redhat.com>
收件人：彭浩10096742 <rth@twiddle.net> <ehabkost@redhat.com> <kvm@vger.kernel.org> <mtosatti@redhat.com>
抄送人： <qemu-devel@nongnu.org> <jan.kiszka@siemens.com>
日 期 ：2017年07月13日 01:01
主 题 ：Re: [PATCH] target-i386:kvm_get/put_vcpu_events don't handlesipi_vector





On 12/07/2017 11:12, Peng Hao wrote:
> qemu call kvm_get_vcpu_events, and kernel return sipi_vector always 0,
> never valid when reporting to user space. But qemu call kvm_put_vcpu_events
> will make sipi_vector in kernel be 0. This will accidently modify sipi_vector
> when sipi_vector in kernel is not 0.
> 
> Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
> Reviewed-by: Wang Yechao <wang.yechao255@zte.com.cn>
> ---
>  target/i386/kvm.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
> index f84a49d..bbbd696 100644
> --- a/target/i386/kvm.c
> +++ b/target/i386/kvm.c
> @@ -2417,7 +2417,6 @@ static int kvm_put_vcpu_events(X86CPU *cpu, int level)
>      events.nmi.masked = !!(env->hflags2 & HF2_NMI_MASK)
>      events.nmi.pad = 0
>  
> -    events.sipi_vector = env->sipi_vector
>      events.flags = 0
>  
>      if (has_msr_smbase) {

This should clear KVM_VCPUEVENT_VALID_SIPI_VECTOR in the "if" statement
below.  Otherwise, you're passing a random value to the kernel.
---yes,you're right.I lost this )

> @@ -2506,8 +2505,6 @@ static int kvm_get_vcpu_events(X86CPU *cpu)
>          }
>      }
>  
> -    env->sipi_vector = events.sipi_vector
> -
>      return 0
>  }
>  
> 

I think what you're seeing is a race like this:

    VCPU 0                    VCPU 1
    [qemu] kvm_get_mp_state
      [kvm] kvm_apic_accept_events
                        __apic_accept_irq
                          set KVM_APIC_SIPI
    [qemu] kvm_get_vcpu_events
----I don't think it will happen.
If this is what happens happen, the fix is to call the functions in this
order:

- first kvm_get_vcpu_events

- then kvm_get_mp_state

- then all the others.

This way you have:

    VCPU 0                    VCPU 1
    [qemu] kvm_get_vcpu_events
                        __apic_accept_irq
                          set KVM_APIC_SIPI
    [qemu] kvm_get_mp_state
      [kvm] kvm_apic_accept_events
         [kvm] kvm_vcpu_deliver_sipi_vector
            // this modifies CS and RIP!!!
    [qemu] kvm_getput_regs
      // reads RIP
    ...
    [qemu] kvm_getput_sregs
      // reads CS
    ...

and the sipi_vector is never lost.

----I suspect that sipi_vector is lost when hotplug cpu in some time.

VCPU0                                     VCPU1 (hotplug)

[kvm]apic_send_ipi

[kvm]set vcpu1.sipi_vector      

[kvm] wakeup vcpu1 thread   [qemu] kvm_put_vcpu_events

                                              [kvm] set vcpu1.sipi_vector=0

                                              [kvm] kvm_apic_accept_events

                                                       [kvm]kvm_vcpu_deliver_sipi_vector(sipi_vector=0)

Re: [Qemu-devel] 答复: Re: [PATCH] target-i386:kvm_get/put_vcpu_events don't handlesipi_vector

[Paolo Bonzini]

On 13/07/2017 04:53, peng.hao2@zte.com.cn wrote:
> > I think what you're seeing is a race like this:
> > 
> >     VCPU 0                           VCPU 1
> >     [qemu] kvm_get_mp_state
> >       [kvm] kvm_apic_accept_events
> >                                     __apic_accept_irq
> >                                     set KVM_APIC_SIPI
> >     [qemu] kvm_get_vcpu_events
>
> I suspect that sipi_vector is lost when hotplug cpu in some time.
> 
> VCPU0                               VCPU1 (hotplug)
> [kvm] apic_send_ipi
>   [kvm] __apic_accept_irq
>     [kvm] set vcpu1.sipi_vector      
>     [kvm] set KVM_APIC_SIPI
> [kvm] wakeup vcpu1 thread           [qemu] kvm_put_vcpu_events
>                                        [kvm] set vcpu1.sipi_vector=0
>                                        [kvm] kvm_apic_accept_events
>                                        [kvm] kvm_vcpu_deliver_sipi_vector(sipi_vector=0)

So I suggest the following changes in QEMU:

- reorder kvm_get_vcpu_events, then kvm_get_mp_state, then the others.
This is just to be safe and ensure that a KVM_APIC_SIPI event is not lost.

- don't set KVM_VCPUEVENT_VALID_SIPI_VECTOR unless the mp_state is
KVM_MP_STATE_SIPI_RECEIVED (which will only happen for old kernels).

- call kvm_put_mp_state after kvm_put_vcpu_events, so that KVM_APIC_SIPI
is only set after the sipi_vector is in place.

Thanks,

Paolo