Date: Thu, 3 Nov 2016 21:16:38 +0100
From: Radim =?utf-8?B?S3LEjW3DocWZ?= <rkrcmar@redhat.com>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
        yang.zhang.wz@gmail.com, feng.wu@intel.com, mst@redhat.com
Subject: Re: [PATCH 2/5] KVM: x86: do not scan IRR twice on APICv vmentry
Message-ID: <20161103201638.GH7771@potion>
References: <1476469291-5039-1-git-send-email-pbonzini@redhat.com>
 <1476469291-5039-3-git-send-email-pbonzini@redhat.com>
 <20161026195900.GC4212@potion>
 <00273a1a-ef5d-f814-3e02-24b4e855d229@redhat.com>
 <20161103150356.GE7771@potion>
 <af052add-55c9-d314-3d21-ac32e2cfbb82@redhat.com>
 <20161103180753.GF7771@potion>
 <18e681d1-640f-8c56-dc05-acc7ca34cd45@redhat.com>
 <20161103182930.GG7771@potion>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20161103182930.GG7771@potion>
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2057
Lines: 59

[Oh, I got distracted and sent without finishing ...]

2016-11-03 19:29+0100, Radim Krčmář:
> 2016-11-03 19:18+0100, Paolo Bonzini:
>> On 03/11/2016 19:07, Radim Krčmář wrote:
>>> I think a bug is likely for hypervisors that don't enable
>>> PIN_BASED_EXT_INTR_MASK.  The bug would trigger when
>>> kvm_cpu_has_interrupt() in vmx_check_nested_events() in
>>> kvm_arch_vcpu_runnable() queues the interrupt ...
>>> but I didn't see how this would have caused a problem. :)
>>
>> Ironically, _not_ enabling PIN_BASED_EXT_INTR_MASK and not using HALT
>> activity state is the only case that passes of the four that vmx.flat tests.
> 
> Heh, the behavior is nice
> 
> PASS: direct interrupt + hlt
> FAIL: intercepted interrupt + hlt
> FAIL: direct interrupt + activity state hlt
> FAIL: intercepted interrupt + activity state hlt

but the 3rd one is racy, so I sometimes also get

  PASS: direct interrupt + hlt
  FAIL: intercepted interrupt + hlt
  PASS: direct interrupt + activity state hlt
  FAIL: intercepted interrupt + activity state hlt

1st and 3rd have disabled extint and 2nd and 4th enabled ...
but that would mean that we a bug in a path that gets called in both
cases, so calling vmx_hwapic_irr_update() isn't a problem ...
and suddenly the bug becomes obvious:

> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> +static void vmx_sync_pir_to_irr(struct kvm_vcpu *vcpu)
> +{
> +	struct vcpu_vmx *vmx = to_vmx(vcpu);
> +	int max_irr;
> +
> +	if (!pi_test_on(&vmx->pi_desc))

We don't call vmx_hwapic_irr_update() when returning early.

> +		return;
> +
> +	pi_clear_on(&vmx->pi_desc);
> +	max_irr = kvm_apic_update_irr(vcpu, vmx->pi_desc.pir);
> +	vmx_hwapic_irr_update(vcpu, max_irr);
> +}
> +
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> @@ -6611,8 +6611,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>  		 * virtual interrupt delivery.
>  		 */
>  		if (vcpu->arch.apicv_active)
> -			kvm_x86_ops->hwapic_irr_update(vcpu,
> -				kvm_lapic_find_highest_irr(vcpu));
> +			kvm_x86_ops->sync_pir_to_irr(vcpu);
>  	}