Re: [PATCH v3 10/11] KVM: nVMX: Wake halted L2 on nested posted-interrupt

[Liran Alon <LIRAN.ALON@ORACLE.COM>]

On 27/12/17 15:05, Paolo Bonzini wrote:
> On 27/12/2017 13:52, Liran Alon wrote:
>>
>> The race-condition described in commit message of patch 9 is fixed
>> in patch 9. It is fixed because we get rid of the dependency on
>> KVM_REQ_EVENT.
>
> Or is it fixed because, while getting rid of the dependency on
> KVM_REQ_EVENT, you had to move kvm_vcpu_trigger_posted_interrupt to the
> correct place:
>
>
> -		/* the PIR and ON have been set by L1. */
> -		kvm_vcpu_trigger_posted_interrupt(vcpu, true);
>   		/*
>   		 * 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);
> +		/* the PIR and ON have been set by L1. */
> +		kvm_vcpu_trigger_posted_interrupt(vcpu, true);
>   		return 0;
>
> ?
>
> Getting rid of KVM_REQ_EVENT is not a requirement to fix the
> vcpu->requests race.  There is canonical way to avoid that, which is to
> use kvm_vcpu_kick.
>
> To me the main value of patch 9 is introducing the nested PI handler.
> The fact that patch 9 fixes that race condition is almost a side effect
> (and in fact it's not entirely fixed until patch 10, in my opinion).
>

That's a good point. Haven't thought about it like that.
I now tend to agree with you.

I will re-think about how to change patches 5-11 such that we will:

1. Get rid of pi_pending and instead use virtual LAPIC IRR and process 
the vmcs12->posted_intr_nv specially in case vCPU is in non-root-mode 
(and posted-interrupts is active). Similar to what a real CPU does.

2. Re-order patches to be similar to the following:
(a) Simple bug-fix for the race-condition issue: Just changing order 
like I did in v1 and adding a kvm_vcpu_kick() like in patch 10 of this 
series.
(b) Get rid of pi_pending and instead use virtual LAPIC IRR bit and 
process it specially in case vCPU in non-root-mode & posted-interrupts 
is active.
(c) Get rid of software simulation of nested posted-interrupts 
processing and instead use self-IPI trick to make CPU process it for us.

What do you think?

Regards,
-Liran

> Paolo
>
>> If the target vCPU thread passes the check for pending
>> kvm requests, it means it is already running with interrupts disabled
>> and therefore the physical IPI of POSTED_INTR_NESTED_VECTOR will be
>> received in guest which will process nested posted-interrupts
>> correctly. If guest will exit because of another external-interrupt
>> before the physical IPI will be received, on next VMEntry, code will
>> note there are pending nested posted-interrupts and re-trigger
>> physical IPI of POSTED_INTR_NESTED_VECTOR. Therefore, nested
>> posted-interrupts will eventually be processed in guest. This is in
>> contrast to what happens before patch 9 where L2 guest will continue
>> running until next time KVM_REQ_EVENT will be consumed. Therefore,
>> bug is indeed fixed in patch 9.
>