[PATCH] KVM: x86: optimize local IRQ disable time before vmentry

[PATCH] KVM: x86: optimize local IRQ disable time before vmentry

[Wanpeng Li]

From: Wanpeng Li <wanpeng.li@hotmail.com>

Commit b95234c84 (kvm: x86: do not use KVM_REQ_EVENT for APICv interrupt 
injection) disables interrupts before setting vcpu->mode to fix an race:

| The IPI for posted interrupts may be issued between setting vcpu->mode = 
| IN_GUEST_MODE and disabling interrupts. If that happens, 
| kvm_trigger_posted_interrupt returns true, but smp_kvm_posted_intr_ipi 
| doesn't do anything about it.  The guest is entered with PIR.ON, but the 
| posted interrupt IPI has not been sent and the interrupt is only delivered 
| to the guest on the next vmentry (if any).

However the race has already been fixed by the side-effects of moving the 
RVI update after IN_GUEST_MODE in the commit:

- The IPI for posted interrupts is issued after setting vcpu->mode = 
  IN_GUEST_MODE and disabling interrupts, the posted interrupt IPI is 
  delayed until the guest enters non-root mode; it is then trapped by 
  the processor causing the interrupt to be injected.
- The IPI for posted interrupts is issued between setting vcpu->mode = 
  IN_GUEST_MODE and disabling interrupts, the movement of the RVI update 
  after IN_GUEST_MODE in the commit will catch the new PIR, and set RVI.

This patch tries to reduce the local IRQ disable time by restoring the 
place of disable local IRQ in order to improve host kernel responsibility 
to some degree.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Radim Krčmář <rkrcmar@redhat.com>
Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
---
 arch/x86/kvm/x86.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 1faf620..9d97041 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6857,12 +6857,6 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 	kvm_x86_ops->prepare_guest_switch(vcpu);
 	kvm_load_guest_fpu(vcpu);
 
-	/*
-	 * Disable IRQs before setting IN_GUEST_MODE.  Posted interrupt
-	 * IPI are then delayed after guest entry, which ensures that they
-	 * result in virtual interrupt delivery.
-	 */
-	local_irq_disable();
 	vcpu->mode = IN_GUEST_MODE;
 
 	srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
@@ -6881,6 +6875,8 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 	 */
 	smp_mb__after_srcu_read_unlock();
 
+	local_irq_disable();
+
 	/*
 	 * This handles the case where a posted interrupt was
 	 * notified with kvm_vcpu_kick.
-- 
2.7.4

Re: [PATCH] KVM: x86: optimize local IRQ disable time before vmentry

[Paolo Bonzini]

On 09/03/2017 13:16, Wanpeng Li wrote:
> From: Wanpeng Li <wanpeng.li@hotmail.com>
> 
> Commit b95234c84 (kvm: x86: do not use KVM_REQ_EVENT for APICv interrupt 
> injection) disables interrupts before setting vcpu->mode to fix an race:
> 
> | The IPI for posted interrupts may be issued between setting vcpu->mode = 
> | IN_GUEST_MODE and disabling interrupts. If that happens, 
> | kvm_trigger_posted_interrupt returns true, but smp_kvm_posted_intr_ipi 
> | doesn't do anything about it.  The guest is entered with PIR.ON, but the 
> | posted interrupt IPI has not been sent and the interrupt is only delivered 
> | to the guest on the next vmentry (if any).
> 
> However the race has already been fixed by the side-effects of moving the 
> RVI update after IN_GUEST_MODE in the commit:
> 
> - The IPI for posted interrupts is issued after setting vcpu->mode = 
>   IN_GUEST_MODE and disabling interrupts, the posted interrupt IPI is 
>   delayed until the guest enters non-root mode; it is then trapped by 
>   the processor causing the interrupt to be injected.
> - The IPI for posted interrupts is issued between setting vcpu->mode = 
>   IN_GUEST_MODE and disabling interrupts, the movement of the RVI update 
>   after IN_GUEST_MODE in the commit will catch the new PIR, and set RVI.
> 
> This patch tries to reduce the local IRQ disable time by restoring the 
> place of disable local IRQ in order to improve host kernel responsibility 
> to some degree.

This should not make any difference; we're talking of a few dozen clock
cycles, and it's quite possible that the extra chance of getting a PI
interrupt in the host negates it.

Paolo

> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Radim Krčmář <rkrcmar@redhat.com>
> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
> ---
>  arch/x86/kvm/x86.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 1faf620..9d97041 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6857,12 +6857,6 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>  	kvm_x86_ops->prepare_guest_switch(vcpu);
>  	kvm_load_guest_fpu(vcpu);
>  
> -	/*
> -	 * Disable IRQs before setting IN_GUEST_MODE.  Posted interrupt
> -	 * IPI are then delayed after guest entry, which ensures that they
> -	 * result in virtual interrupt delivery.
> -	 */
> -	local_irq_disable();
>  	vcpu->mode = IN_GUEST_MODE;
>  
>  	srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
> @@ -6881,6 +6875,8 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>  	 */
>  	smp_mb__after_srcu_read_unlock();
>  
> +	local_irq_disable();
> +
>  	/*
>  	 * This handles the case where a posted interrupt was
>  	 * notified with kvm_vcpu_kick.
> 

Re: [PATCH] KVM: x86: optimize local IRQ disable time before vmentry

[David Hildenbrand]

Am 09.03.2017 um 13:33 schrieb Paolo Bonzini:
> 
> 
> On 09/03/2017 13:16, Wanpeng Li wrote:
>> From: Wanpeng Li <wanpeng.li@hotmail.com>
>>
>> Commit b95234c84 (kvm: x86: do not use KVM_REQ_EVENT for APICv interrupt 
>> injection) disables interrupts before setting vcpu->mode to fix an race:
>>
>> | The IPI for posted interrupts may be issued between setting vcpu->mode = 
>> | IN_GUEST_MODE and disabling interrupts. If that happens, 
>> | kvm_trigger_posted_interrupt returns true, but smp_kvm_posted_intr_ipi 
>> | doesn't do anything about it.  The guest is entered with PIR.ON, but the 
>> | posted interrupt IPI has not been sent and the interrupt is only delivered 
>> | to the guest on the next vmentry (if any).
>>
>> However the race has already been fixed by the side-effects of moving the 
>> RVI update after IN_GUEST_MODE in the commit:
>>
>> - The IPI for posted interrupts is issued after setting vcpu->mode = 
>>   IN_GUEST_MODE and disabling interrupts, the posted interrupt IPI is 
>>   delayed until the guest enters non-root mode; it is then trapped by 
>>   the processor causing the interrupt to be injected.
>> - The IPI for posted interrupts is issued between setting vcpu->mode = 
>>   IN_GUEST_MODE and disabling interrupts, the movement of the RVI update 
>>   after IN_GUEST_MODE in the commit will catch the new PIR, and set RVI.
>>
>> This patch tries to reduce the local IRQ disable time by restoring the 
>> place of disable local IRQ in order to improve host kernel responsibility 
>> to some degree.
> 
> This should not make any difference; we're talking of a few dozen clock
> cycles, and it's quite possible that the extra chance of getting a PI
> interrupt in the host negates it.
> 
> Paolo
> 

Also, I think after all the discussion regarding handling vcpu->mode +
preemption when it comes to a vcpu_kick(), keeping it that way is way
cleaner.

-- 
Thanks,

David