On 28/11/17 06:55, Jim Mattson wrote:
On Mon, Nov 27, 2017 at 2:42 PM, Liran Alon <LIRAN.ALON@xxxxxxxxxx> wrote:
On 27/11/17 22:48, Jim Mattson wrote:
I am concerned about the possible conflation of L1 and L2 events.
Vmx_check_nested_events() reads as though vcpu->arch.exception.pending
is always associated with an L1 event, yet inject_pending_event()
reads as though vcpu->arch.exception.injected is associated with the
current level. That seems odd, to split this structure that way.
I think you misunderstand code (rightfully as it is confusing...).
vmx_check_nested_events() treats exception.pending as related to
current-level (L1 or L2). Specifically, because vmx_check_nested_events() is
always called when is_guest_mode(vcpu)==true, then it treats it as a
"pending L2 exception". This is also why it checks if L1 intercept the
"pending L2 exception" and if yes, perform an exit from L2 to L1.
I understand the structures in the following way:
1. "injected" status is always associated with the current-level.
(Same for interrupt.pending because it is actually interrupt.injected, see
comment below marked with (*))
2. exception.pending is associated with current-level.
3. nmi_pending is associated with L1.
4. In general, all the exception/nmi/interrupt structures are "supposed" to
follow the same convention:
(a) A "pending" status means that the event should be injected to guest but
it's side-effects have not occurred yet (not yet injected to guest).
(b) An "injected" status means that the event was already "processed" and
therefore injected to guest along with it's side-effects.
(*) Currently, interrupt.pending is a confusing wrong name. It actually
represents "interrupt.injected". This is why I made the a patch which
renames this variable:
("[PATCH v2 2/8] KVM: x86: Rename interrupt.pending to interrupt.injected")
https://urldefense.proofpoint.com/v2/url?u=https-3A__marc.info_-3Fl-3Dkvm-26m-3D151127826204179-26w-3D2&d=DwIBaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=Jk6Q8nNzkQ6LJ6g42qARkg6ryIDGQr-yKXPNGZbpTx0&m=jEXMkXjEtpUV8ujOR5HKAievAdmBLs-ernvMak55rWI&s=7LxsKUsoJs_ZxeyBEm9t8TtPML-Zhcim11Bdx6TKA60&e=
(BTW, the reason why interrupt doesn't have an "interrupt.pending" flag is
because it's "pending" status can be considered to be held in LAPIC IRR.
This is why for example vmx_check_nested_events() call
kvm_cpu_has_interrupt() to determine if there is a "pending" L1 interrupt
that may cause exit from L2 to L1).
Worse, __vmx_complete_interrupts() calls kvm_queue_interrupt() to set
vcpu->arch.interrupt.pending if IDT vectoring for an L2 virtual
interrupt is interrupted by another event. But
vmx_check_nested_events() calls kvm_cpu_has_interrupt() to see if an
L1 "physical" interrupt should cause a VM-exit from L2 to L1, and
kvm_cpu_has_interrupt() looks at vcpu->arch.interrupt.pending when
!lapic_in_kernel(). Clearly, there is some disagreement here over what
vcpu->arch.interrupt.pending means.
Again, there is some confusion here.
__vmx_complete_interrupts() only re-queues an "injected" event. (Either due
to exit during event-delivery and therefore valid IDT-vectoring-info or
because of injection-cancellation in vcpu_enter_guest()).
Therefore, this function should only re-insert events in "injected" state.
As it really does:
1. NMI is re-injected with nmi_injected status.
2. exception is injected using kvm_requeue_exception() which will mark
exception.injected=true.
3. interrupt is injected using kvm_queue_interrupt() which indeed mark
interrupt.pending. But as stated above, this is just a wrong name and it is
actually "interrupt.injected".
kvm_queue_interrupt() begins as follows:
vcpu->arch.interrupt.pending = true;
kvm_cpu_has_interrupt() begins as follows:
if (!lapic_in_kernel(v))
return v->arch.interrupt.pending;
In the referenced [patch 2/8], you change interrupt.pending to
interrupt.injected, but the same field is still referenced by these
two functions.
It's clear that __vmx_complete_interrupts sets this field to indicate
an interrupted L2 virtual interrupt, and I agree with your assertion
above that kvm_cpu_has_interrupt() *should* determine if there is a
"pending" L1 physical interrupt that may cause exit from L2 to L1, but
it seems to me that the injected L2 event and the pending L1 event
have been conflated in this field, at least when lapic_in_kernel() is
false.
First of all, I hope we agree that in case lapic_in_kernel()==true then
interrupt.pending actually represents interrupt.injected.
In regards to lapic_in_kernel()==false:
"interrupt.pending" can currently only be set by user-mode in the
following ways:
1. KVM_INTERRUPT ioctl.
2. KVM_SET_VCPU_EVENTS ioctl.
3. KVM_SET_SREGS ioctl.
Looking at how QEMU use these ioctls, one can see that they use it
either to re-set an "interrupt.injected" state it has received from KVM
(via KVM_GET_VCPU_EVENTS interrupt.injected or via KVM_GET_SREGS
interrupt_bitmap) or by dispatching a new interrupt from QEMU's emulated
LAPIC which reset bit in IRR and set bit in ISR before sending ioctl to KVM.
So it is true that "interrupt.pending" in KVM always represents
"interrupt.injected".
One could argue however that kvm_cpu_has_interrupt() &
kvm_cpu_has_injectable_intr() specifically is misusing
interrupt.injected in order to return if there is a pending-interrupt in
case lapic_in_kernel()==false. I agree with this. However, all other
places in KVM code, as far as I can tell, don't misuse this flag and
treat it as "interrupt.injected".
Because the issue with these functions always existed and it is
unrelated to the fixes introduced by this series (and specially this
nVMX fix patch), I suggest modifying my patch ("[PATCH v2 2/8] KVM: x86:
Rename interrupt.pending to interrupt.injected") to also add a FIXME
comment on top of these places.
What do you think?
-Liran
I think there may be some more fundamental problems lurking here.
Yep. You are indeed right.
We have found several more issues revolving treatments of pending events in
regard to nested-virtualization.
I am about to post another patch series which handles some such cases which
relates to nested-posted-interrupts handling. Stay tuned. Your review will
be extremely valuable :)
Regards,
-Liran
On Tue, Nov 21, 2017 at 7:30 AM, Liran Alon <liran.alon@xxxxxxxxxx> wrote:
In case L2 VMExit to L0 during event-delivery, VMCS02 is filled with
IDT-vectoring-info which vmx_complete_interrupts() makes sure to
reinject before next resume of L2.
While handling the VMExit in L0, an IPI could be sent by another L1 vCPU
to the L1 vCPU which currently runs L2 and exited to L0.
When L0 will reach vcpu_enter_guest() and call inject_pending_event(),
it will note that a previous event was re-injected to L2 (by
IDT-vectoring-info) and therefore won't check if there are pending L1
events which require exit from L2 to L1. Thus, L0 enters L2 without
immediate VMExit even though there are pending L1 events!
This commit fixes the issue by making sure to check for L1 pending
events even if a previous event was reinjected to L2 and bailing out
from inject_pending_event() before evaluating a new pending event in
case an event was already reinjected.
The bug was observed by the following setup:
* L0 is a 64CPU machine which runs KVM.
* L1 is a 16CPU machine which runs KVM.
* L0 & L1 runs with APICv disabled.
(Also reproduced with APICv enabled but easier to analyze below info
with APICv disabled)
* L1 runs a 16CPU L2 Windows Server 2012 R2 guest.
During L2 boot, L1 hangs completely and analyzing the hang reveals that
one L1 vCPU is holding KVM's mmu_lock and is waiting forever on an IPI
that he has sent for another L1 vCPU. And all other L1 vCPUs are
currently attempting to grab mmu_lock. Therefore, all L1 vCPUs are stuck
forever (as L1 runs with kernel-preemption disabled).
Observing /sys/kernel/debug/tracing/trace_pipe reveals the following
series of events:
(1) qemu-system-x86-19066 [030] kvm_nested_vmexit: rip:
0xfffff802c5dca82f reason: EPT_VIOLATION ext_inf1: 0x0000000000000182
ext_inf2: 0x00000000800000d2 ext_int: 0x00000000 ext_int_err: 0x00000000
(2) qemu-system-x86-19054 [028] kvm_apic_accept_irq: apicid f
vec 252 (Fixed|edge)
(3) qemu-system-x86-19066 [030] kvm_inj_virq: irq 210
(4) qemu-system-x86-19066 [030] kvm_entry: vcpu 15
(5) qemu-system-x86-19066 [030] kvm_exit: reason EPT_VIOLATION
rip 0xffffe00069202690 info 83 0
(6) qemu-system-x86-19066 [030] kvm_nested_vmexit: rip:
0xffffe00069202690 reason: EPT_VIOLATION ext_inf1: 0x0000000000000083
ext_inf2: 0x0000000000000000 ext_int: 0x00000000 ext_int_err: 0x00000000
(7) qemu-system-x86-19066 [030] kvm_nested_vmexit_inject: reason:
EPT_VIOLATION ext_inf1: 0x0000000000000083 ext_inf2: 0x0000000000000000
ext_int: 0x00000000 ext_int_err: 0x00000000
(8) qemu-system-x86-19066 [030] kvm_entry: vcpu 15
Which can be analyzed as follows:
(1) L2 VMExit to L0 on EPT_VIOLATION during delivery of vector 0xd2.
Therefore, vmx_complete_interrupts() will set KVM_REQ_EVENT and reinject
a pending-interrupt of 0xd2.
(2) L1 sends an IPI of vector 0xfc (CALL_FUNCTION_VECTOR) to destination
vCPU 15. This will set relevant bit in LAPIC's IRR and set KVM_REQ_EVENT.
(3) L0 reach vcpu_enter_guest() which calls inject_pending_event() which
notes that interrupt 0xd2 was reinjected and therefore calls
vmx_inject_irq() and returns. Without checking for pending L1 events!
Note that at this point, KVM_REQ_EVENT was cleared by vcpu_enter_guest()
before calling inject_pending_event().
(4) L0 resumes L2 without immediate-exit even though there is a pending
L1 event (The IPI pending in LAPIC's IRR).
We have already reached the buggy scenario but events could be
furthered analyzed:
(5+6) L2 VMExit to L0 on EPT_VIOLATION. This time not during
event-delivery.
(7) L0 decides to forward the VMExit to L1 for further handling.
(8) L0 resumes into L1. Note that because KVM_REQ_EVENT is cleared, the
LAPIC's IRR is not examined and therefore the IPI is still not delivered
into L1!
Signed-off-by: Liran Alon <liran.alon@xxxxxxxxxx>
Reviewed-by: Nikita Leshenko <nikita.leshchenko@xxxxxxxxxx>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
---
arch/x86/kvm/x86.c | 33 ++++++++++++++++++++-------------
1 file changed, 20 insertions(+), 13 deletions(-)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index a4b5a013064b..63359ab0e798 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6398,28 +6398,27 @@ static int inject_pending_event(struct kvm_vcpu
*vcpu, bool req_int_win)
int r;
/* try to reinject previous events if any */
- if (vcpu->arch.exception.injected) {
- kvm_x86_ops->queue_exception(vcpu);
- return 0;
- }
+ if (vcpu->arch.exception.injected)
+ kvm_x86_ops->queue_exception(vcpu);
/*
* NMI/interrupt must not be injected if an exception is
* pending, because the latter may have been queued by
* handling exit due to NMI/interrupt delivery.
*/
- if (!vcpu->arch.exception.pending) {
- if (vcpu->arch.nmi_injected) {
+ else if (!vcpu->arch.exception.pending) {
+ if (vcpu->arch.nmi_injected)
kvm_x86_ops->set_nmi(vcpu);
- return 0;
- }
-
- if (vcpu->arch.interrupt.injected) {
+ else if (vcpu->arch.interrupt.injected)
kvm_x86_ops->set_irq(vcpu);
- return 0;
- }
}
+ /*
+ * Call check_nested_events() even if we reinjected a previous
event
+ * in order for caller to determine if it should require
immediate-exit
+ * from L2 to L1 due to pending L1 events which require exit
+ * from L2 to L1.
+ */
if (is_guest_mode(vcpu) && kvm_x86_ops->check_nested_events) {
r = kvm_x86_ops->check_nested_events(vcpu, req_int_win);
if (r != 0)
@@ -6438,6 +6437,7 @@ static int inject_pending_event(struct kvm_vcpu
*vcpu, bool req_int_win)
vcpu->arch.exception.has_error_code,
vcpu->arch.exception.error_code);
+ WARN_ON_ONCE(vcpu->arch.exception.injected);
vcpu->arch.exception.pending = false;
vcpu->arch.exception.injected = true;
@@ -6452,7 +6452,14 @@ static int inject_pending_event(struct kvm_vcpu
*vcpu, bool req_int_win)
}
kvm_x86_ops->queue_exception(vcpu);
- } else if (vcpu->arch.smi_pending && !is_smm(vcpu) &&
kvm_x86_ops->smi_allowed(vcpu)) {
+ }
+
+ /* Don't consider new event if we re-injected an event */
+ if (kvm_event_needs_reinjection(vcpu))
+ return 0;
+
+ if (vcpu->arch.smi_pending && !is_smm(vcpu) &&
+ kvm_x86_ops->smi_allowed(vcpu)) {
vcpu->arch.smi_pending = false;
enter_smm(vcpu);
} else if (vcpu->arch.nmi_pending &&
kvm_x86_ops->nmi_allowed(vcpu)) {
--
1.9.1
