Re: [PATCH] KVM: VMX: check apicv is active before using VT-d posted interrupt

From: Yang Zhang <yang.zhang.wz@gmail.com>
To: "Wu, Feng" <feng.wu@intel.com>,
	Paolo Bonzini <pbonzini@redhat.com>,
	"rkrcmar@redhat.com" <rkrcmar@redhat.com>
Cc: "kvm@vger.kernel.org" <kvm@vger.kernel.org>
Subject: Re: [PATCH] KVM: VMX: check apicv is active before using VT-d posted interrupt
Date: Mon, 23 May 2016 09:10:05 +0800	[thread overview]
Message-ID: <f470f583-9fcc-f570-20ac-f765416b0dc8@gmail.com> (raw)
In-Reply-To: <E959C4978C3B6342920538CF579893F0196E1C24@SHSMSX104.ccr.corp.intel.com>

On 2016/5/20 16:22, Wu, Feng wrote:
>
>
>> -----Original Message-----
>> From: Yang Zhang [mailto:yang.zhang.wz@gmail.com]
>> Sent: Friday, May 20, 2016 3:51 PM
>> To: Paolo Bonzini <pbonzini@redhat.com>; rkrcmar@redhat.com; Wu, Feng
>> <feng.wu@intel.com>
>> Cc: kvm@vger.kernel.org
>> Subject: [PATCH] KVM: VMX: check apicv is active before using VT-d posted
>> interrupt
>>
>> VT-d posted interrupt is relying on the CPU side's posted interrupt.
>> Need to check whether VCPU's APICv is active before enabing VT-d posted
>> interrupt.
>
> Yes, the extra check is good. Just curious, Do you see some hardware with
> VT-d PI but no CPU side PI. I remember I was told this cannot happen in
> real world.

Hardware is ok. The problem is user can turn off the APICv manually 
which cause the device fails to work inside guest.

>
> Thanks,
> feng
>
>>
>> Signed-off-by: Yang Zhang <yang.zhang.wz@gmail.com>
>> Signed-off-by: Shengge Ding <shengge.dsg@alibaba-inc.com>
>> ---
>>   arch/x86/kvm/vmx.c | 15 ++++++++++-----
>>   1 file changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index 7ebf27b..8ba87f3 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -2072,7 +2072,8 @@ static void vmx_vcpu_pi_load(struct kvm_vcpu
>> *vcpu, int cpu)
>>   	unsigned int dest;
>>
>>   	if (!kvm_arch_has_assigned_device(vcpu->kvm) ||
>> -		!irq_remapping_cap(IRQ_POSTING_CAP))
>> +		!irq_remapping_cap(IRQ_POSTING_CAP)  ||
>> +		!kvm_vcpu_apicv_active(vcpu))
>>   		return;
>>
>>   	do {
>> @@ -2180,7 +2181,8 @@ static void vmx_vcpu_pi_put(struct kvm_vcpu *vcpu)
>>   	struct pi_desc *pi_desc = vcpu_to_pi_desc(vcpu);
>>
>>   	if (!kvm_arch_has_assigned_device(vcpu->kvm) ||
>> -		!irq_remapping_cap(IRQ_POSTING_CAP))
>> +		!irq_remapping_cap(IRQ_POSTING_CAP)  ||
>> +		!kvm_vcpu_apicv_active(vcpu))
>>   		return;
>>
>>   	/* Set SN when the vCPU is preempted */
>> @@ -10698,7 +10700,8 @@ static int vmx_pre_block(struct kvm_vcpu *vcpu)
>>   	struct pi_desc *pi_desc = vcpu_to_pi_desc(vcpu);
>>
>>   	if (!kvm_arch_has_assigned_device(vcpu->kvm) ||
>> -		!irq_remapping_cap(IRQ_POSTING_CAP))
>> +		!irq_remapping_cap(IRQ_POSTING_CAP)  ||
>> +		!kvm_vcpu_apicv_active(vcpu))
>>   		return 0;
>>
>>   	vcpu->pre_pcpu = vcpu->cpu;
>> @@ -10764,7 +10767,8 @@ static void vmx_post_block(struct kvm_vcpu *vcpu)
>>   	unsigned long flags;
>>
>>   	if (!kvm_arch_has_assigned_device(vcpu->kvm) ||
>> -		!irq_remapping_cap(IRQ_POSTING_CAP))
>> +		!irq_remapping_cap(IRQ_POSTING_CAP)  ||
>> +		!kvm_vcpu_apicv_active(vcpu))
>>   		return;
>>
>>   	do {
>> @@ -10817,7 +10821,8 @@ static int vmx_update_pi_irte(struct kvm *kvm,
>> unsigned int host_irq,
>>   	int idx, ret = -EINVAL;
>>
>>   	if (!kvm_arch_has_assigned_device(kvm) ||
>> -		!irq_remapping_cap(IRQ_POSTING_CAP))
>> +		!irq_remapping_cap(IRQ_POSTING_CAP) ||
>> +		!kvm_vcpu_apicv_active(kvm->vcpus[0]))
>>   		return 0;
>>
>>   	idx = srcu_read_lock(&kvm->irq_srcu);
>> --
>> 1.8.2.1

-- 
best regards
yang