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
