[PATCH] KVM: LAPIC: make the kvm unblock information globally seen before calling kvm_vcpu_kick

[PATCH] KVM: LAPIC: make the kvm unblock information globally seen before calling kvm_vcpu_kick

[Zhuangyanying]

From: Xiangyou Xie <xiexiangyou@huawei.com>

When the guest is using the pv-spinlock, and there is a race of vcpu1 decides to HALT and vcpu2 releasing the lock,  the vcpu2 might fail to wake up vcpu1.
In the case of PV spinlock, the vcpu2 executes the following to wake up vcpu:
case APIC_DM_REMRD:
	result = 1;
	vcpu->arch.pv.pv_unhalted = 1;
	kvm_make_request(KVM_REQ_EVENT, vcpu);
	kvm_vcpu_kick(vcpu);
	break;
Due to lack of smp_mb before kvm_vcpu_kick, the changes of pv_unhalted and vcpu->requests may be delayed to be after the operation of kvm_vcpu_kick.
On the other part, if the vcpu1 is just to execute the prepare_to_wait, vcpu2's kvm_vcpu_kick will have no effect, but vcpu1 fails to pass the kvm_vcpu_check_block() and as a result get blocked.
for (;;) {
          //------->when vcpu2 do the kvm_vcpu_kick, vcpu1 has not reached here
	prepare_to_wait(&vcpu->wq, &wait, TASK_INTERRUPTIBLE);

	if (kvm_vcpu_check_block(vcpu) < 0)
		break;

	waited = true;
	schedule();
}

Adding an smp_mb() before vcpu2 doing the kvm_vcpu_kick works.

Signed-off-by: Xiangyou Xie<xiexiangyou@huawei.com>
Signed-off-by: Liuxiaojian <liuxiaojian6@huawei.com>
---
 arch/x86/kvm/lapic.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 69c5612..d37c0fd 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -987,6 +987,7 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
 		result = 1;
 		vcpu->arch.pv.pv_unhalted = 1;
 		kvm_make_request(KVM_REQ_EVENT, vcpu);
+		smp_mb();
 		kvm_vcpu_kick(vcpu);
 		break;
 
-- 
1.8.3.1

Re: [PATCH] KVM: LAPIC: make the kvm unblock information globally seen before calling kvm_vcpu_kick

[Paolo Bonzini]

On 25/10/2017 09:26, Zhuangyanying wrote:
> From: Xiangyou Xie <xiexiangyou@huawei.com>
> 
> When the guest is using the pv-spinlock, and there is a race of vcpu1 decides to HALT and vcpu2 releasing the lock,  the vcpu2 might fail to wake up vcpu1.
> In the case of PV spinlock, the vcpu2 executes the following to wake up vcpu:
> case APIC_DM_REMRD:
> 	result = 1;
> 	vcpu->arch.pv.pv_unhalted = 1;
> 	kvm_make_request(KVM_REQ_EVENT, vcpu);
> 	kvm_vcpu_kick(vcpu);
> 	break;
> Due to lack of smp_mb before kvm_vcpu_kick, the changes of pv_unhalted
> and vcpu->requests may be delayed to be after the operation of kvm_vcpu_kick.

Since Linux 4.7 (commit 2e4682ba2ed7, "KVM: add missing memory barrier
in kvm_{make,check}_request", 2016-04-20), kvm_make_request has a smp_wmb().

However, even before that commit "set_bit" implicitly has a barrier on
x86, so there is no effect from the patch below.

Paolo

> On the other part, if the vcpu1 is just to execute the prepare_to_wait, vcpu2's kvm_vcpu_kick will have no effect, but vcpu1 fails to pass the kvm_vcpu_check_block() and as a result get blocked.
> for (;;) {
>           //------->when vcpu2 do the kvm_vcpu_kick, vcpu1 has not reached here
> 	prepare_to_wait(&vcpu->wq, &wait, TASK_INTERRUPTIBLE);
> 
> 	if (kvm_vcpu_check_block(vcpu) < 0)
> 		break;
> 
> 	waited = true;
> 	schedule();
> }
> 
> Adding an smp_mb() before vcpu2 doing the kvm_vcpu_kick works.
> 
> Signed-off-by: Xiangyou Xie<xiexiangyou@huawei.com>
> Signed-off-by: Liuxiaojian <liuxiaojian6@huawei.com>
> ---
>  arch/x86/kvm/lapic.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 69c5612..d37c0fd 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -987,6 +987,7 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
>  		result = 1;
>  		vcpu->arch.pv.pv_unhalted = 1;
>  		kvm_make_request(KVM_REQ_EVENT, vcpu);
> +		smp_mb();
>  		kvm_vcpu_kick(vcpu);
>  		break;
>  
>