Re: [PATCH v2 1/5] KVM: nVMX: Remove pi_pending as signal to process nested posted-interrupts

[Liran Alon <liran.alon@oracle.com>]

On 12/05/17 10:16, Liran Alon wrote: 
> When L1 wants to send a posted-interrupt to another L1 CPU which runs
> L2, it does the following operations:
> 1. Sets the relevant bit in vmx->nested.pi_desc PIR.
> 2. Set ON bit in vmx->nested.pi_desc->control field.
> 3. Sends an IPI to dest L1 CPU by writing the the posted-interrupt
>     notification-vector into the LAPIC ICR.
> 
> Step (3) will exit to L0 on APIC_WRITE which will eventually reach
> vmx_deliver_nested_posted_interrupt(). If dest L0 CPU is in guest,
> then a physical IPI of notification-vector will be sent which will
> trigger evaluation of posted-interrupts and clear
> vmx->nested.pi_desc->control ON bit.
> Otherwise (or if dest L0 CPU exited from guest just before sending the
> physical IPI), the nested-posted-interrupts will be evaluated on next
> vmentry by vmx_complete_nested_posted_interrupt().
> 
> In order for vmx_complete_nested_posted_interrupt() to know if it
> should do any work, the flag vmx->nested.pi_pending was used which
> was set by vmx_deliver_nested_posted_interrupt().
> 
> However, this seems unnecessary because if posted-interrupts was not
> processed yet, vmx->nested.pi_desc->control ON bit should still be
> set. Therefore, it should suffice to use it in order to know if work
> should be done.
> 
> Signed-off-by: Liran Alon <liran.alon@oracle.com>
> Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
> Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>   arch/x86/kvm/vmx.c | 6 +-----
>   1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 714a0673ec3c..f5074ec5701b 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -453,7 +453,6 @@ struct nested_vmx {
>   	struct page *virtual_apic_page;
>   	struct page *pi_desc_page;
>   	struct pi_desc *pi_desc;
> -	bool pi_pending;
>   	u16 posted_intr_nv;
>   
>   	unsigned long *msr_bitmap;
> @@ -5050,10 +5049,9 @@ static void vmx_complete_nested_posted_interrupt(struct kvm_vcpu *vcpu)
>   	void *vapic_page;
>   	u16 status;
>   
> -	if (!vmx->nested.pi_desc || !vmx->nested.pi_pending)
> +	if (!vmx->nested.pi_desc)
>   		return;
>   
> -	vmx->nested.pi_pending = false;
>   	if (!pi_test_and_clear_on(vmx->nested.pi_desc))
>   		return;
>   
> @@ -5126,7 +5124,6 @@ static int vmx_deliver_nested_posted_interrupt(struct kvm_vcpu *vcpu,
>   		 * If a posted intr is not recognized by hardware,
>   		 * we will accomplish it in the next vmentry.
>   		 */
> -		vmx->nested.pi_pending = true;
>   		kvm_make_request(KVM_REQ_EVENT, vcpu);
>   		return 0;
>   	}
> @@ -10488,7 +10485,6 @@ static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
>   	/* Posted interrupts setting is only taken from vmcs12.  */
>   	if (nested_cpu_has_posted_intr(vmcs12)) {
>   		vmx->nested.posted_intr_nv = vmcs12->posted_intr_nv;
> -		vmx->nested.pi_pending = false;
>   		vmcs_write16(POSTED_INTR_NV, POSTED_INTR_NESTED_VECTOR);
>   	} else {
>   		exec_control &= ~PIN_BASED_POSTED_INTR;
> 

It turns out this patch is incorrect and should be removed from series.
I will remove it from next version of this patch series. Just didn't want you to queue it by mistake. :)

I figured that pi_pending is used as a flag to indicate if L1 has sent a posted-interrupt notification-vector such that KVM won't process the nested-posted-interrupts on vmentry in case L1 has already set the ON bit in vmx->nested.pi_desc->control field but not sent the notification-vector yet. That will break vCPU semantics.

(This is in contrast to how vmx_sync_pir_to_irr() does ignore whether a posted-interrupt notification-vector was sent or not and just consume posted-interrupts on each vmentry if the vmx->pi_desc.control ON bit is set. That is fine because in this case L0 KVM defines how it wishes to handle posted-interrupts). 

Regards,
-Liran