[Qemu-devel] [PATCH] target-i386:kvm_get/put_vcpu_events don't handle sipi_vector

[Qemu-devel] [PATCH] target-i386:kvm_get/put_vcpu_events don't handle sipi_vector

[Peng Hao]

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) {
@@ -2506,8 +2505,6 @@ static int kvm_get_vcpu_events(X86CPU *cpu)
         }
     }
 
-    env->sipi_vector = events.sipi_vector;
-
     return 0;
 }
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] target-i386:kvm_get/put_vcpu_events don't handle sipi_vector

[Paolo Bonzini]

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.

> @@ -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

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.

Paolo