Re: [PATCH v2 2/2] KVM: x86: Add lowest-priority support for vt-d posted-interrupts

["rkrcmar@redhat.com" <rkrcmar@redhat.com>]

2015-12-22 14:42+0800, Yang Zhang:
> On 2015/12/22 12:36, Wu, Feng wrote:
>>>From: Yang Zhang [mailto:yang.zhang.wz@gmail.com]
>>>On 2015/12/21 9:55, Wu, Feng wrote:
>>>>>From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-
>>>>>On 2015/12/16 9:37, Feng Wu wrote:
>>>>>>diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>>>>>>@@ -10702,8 +10702,16 @@ static int vmx_update_pi_irte(struct kvm
>>>*kvm,
>>>>>unsigned int host_irq,
>>>>>>    		 */
>>>>>>
>>>>>>    		kvm_set_msi_irq(e, &irq);
>>>>>>-		if (!kvm_intr_is_single_vcpu(kvm, &irq, &vcpu))
>>>>>>-			continue;
>>>>>>+
>>>>>>+		if (!kvm_intr_is_single_vcpu(kvm, &irq, &vcpu)) {
>>>>>>+			if (!kvm_vector_hashing_enabled() ||
>>>>>>+					irq.delivery_mode !=
>>>>>APIC_DM_LOWEST)
>>>>>>+				continue;
>>>>>>+
>>>>>>+			vcpu = kvm_intr_vector_hashing_dest(kvm, &irq);
>>>>>>+			if (!vcpu)
>>>>>>+				continue;
>>>>>>+		}
>>>>>
>>>>>I am a little confused with the 'continue'. If the destination is not
>>>>>single vcpu, shouldn't we rollback to use non-PI mode?
>>>>
>>>>Here is the logic:
>>>>- If it is single destination, we will use PI no matter it is fixed or lowest-priority.
>>>>- If it is not single destination:
>>>>	a) It is fixed, we will use non-PI
>>>>	b) It is lowest-priority and vector-hashing is enabled, we will use PI
>>>>	c) otherwise, use non-PI
>>>
>>>If it is single destination previously, then change to no-single mode.
>>>Can current code cover this case?
>>
>>In my test, before setting irq affinity (change single vcpu to non-single vcpu
>>in this case), the guest will mask the interrupt first, so before getting here, IRTE
>>has been changed back to remapped mode already(when guest masks the MSIx,
>>we will change back to remapped mode), hence nothing needed here.
>>
>>Digging into the linux code (guest) a bit more, I found that if interrupt remapping
>>is not enabled in the guest (IR is not supported for guest anyway), it will always
>>mask the MSI/MSIx before setting the irq affinity. So the code should work
>>well currently.
> 
> We should not rely on guest's behavior. From code level, it need be fixed.
> 
>>However, for robustness, I think explicitly changing IRTE back to remapped
>>mode for the 'continue' case should be a good idea.
> 
> This is what i am looking for.

I agree, that would be a nice addition.

IIRC, the masking is optional -- if the guest can handle interrupts that
are generated while the device is half-configured, it doesn't need to
disable MSIs.