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

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

On 27/12/17 14:27, Paolo Bonzini wrote:
> On 27/12/2017 13:01, Liran Alon wrote:
>> I think that current patch series makes more sense than reordering them.
>>
>> This is because the patch you are suggesting will actually, as a
>> side-effect, fix the bug that is fixed in commit "KVM: nVMX: Deliver
>> missed nested-PI notification-vector via self-IPI while interrupts
>> disabled". This is actually the same as the first patch I suggested for
>> fixing the bug in v1 of this series:
>> https://urldefense.proofpoint.com/v2/url?u=http-3A__lkml.kernel.org_r_1510252040-2D5609-2D1-2Dgit-2Dsend-2Demail-2Dliran.alon-40oracle.com&d=DwICaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=Jk6Q8nNzkQ6LJ6g42qARkg6ryIDGQr-yKXPNGZbpTx0&m=Yu_AVwFNqMnoYNj9fVZO9MHUe9-GwD5Ar2DHsbmBus4&s=VxOx9jL1jMWvw4WWvn3RCeRe2GwzTIlaBgZ5nJpch8Q&e=
>>
>> After I submitted v1, Radim mentioned that even though the fix is
>> correct, it is awkward and error-prone. I agreed and therefore we have
>> written another fix using self-IPI.
>
> It's not exactly the same.  My suggested version checks the return value
> of kvm_vcpu_trigger_posted_interrupt and calls kvm_vcpu_kick.
>
>> Therefore, to preserve each commit fixing only one problem, I prefer to
>> keep the orders of commits as they are now.
>
> It's true that it would hide the bug but it makes no sense to say you
> are fixing a bug in patch 9, and then fix that bug in the next patch.

There are 2 separate bugs here. One is fixed in patch 9 and the other in 
patches 10 & 11.

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.
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.

Patch 10 fixes another bug:
Consider the case target vCPU thread is actually blocked because it 
executed HLT and it was not intercepted by L1. When some other L1 vCPU 
post to it a posted-interrupt, it should wake the target vCPU (as it is 
HLT in L2) and process nested posted-interrupt.
Patches 10 & 11 makes sure to fix this issue.

>
> So I think this patch should definitely go first (in fact I'm tempted to
> install it together with patches 1-4...) and the rest should be done as
> a broader cleanup of vmx->nested.pi_pending.  I agree with Radim that as
> of now it's awkward and needs some work.  I was about to reply about
> that so I'll leave it for another email.

Note that this series is not a clean-up of vmx->nested.pi_pending. That 
variable remained untouched. It is used as a signal to know that a 
nested posted-interrupt notification-vector has indeed been sent to 
vCPU. This is required to not process nested posted-interrupts in case 
vmx.nested.pi_desc->control ON bit is set but no nested posted-interrupt 
notification-vector was actually sent by L1.
This is in contrast to how KVM process posted-interrupts where it only 
treats vmx.pi_desc->control ON bit as indicator if it should process 
posted-interrupts.

Instead, this series just fixes a bunch of unrelated nVMX issues which 
all revolve around posted-interrupts.

Regards,
-Liran

>
> Paolo
>