From: Sean Christopherson <seanjc@google.com> Date: 2022-09-21 00:32:54
Non-x86 folks, there's nothing interesting to see here, y'all got pulled
in because removing KVM_REQ_UNHALT requires deleting kvm_clear_request()
from arch code.
Note, this based on:
https://github.com/sean-jc/linux.git tags/kvm-x86-6.1-1
to pre-resolve conflicts with the event/exception cleanups in there.
In Paolo's words...
The following backtrace:
[ 1355.807187] kvm_vcpu_map+0x159/0x190 [kvm]
[ 1355.807628] nested_svm_vmexit+0x4c/0x7f0 [kvm_amd]
[ 1355.808036] ? kvm_vcpu_block+0x54/0xa0 [kvm]
[ 1355.808450] svm_check_nested_events+0x97/0x390 [kvm_amd]
[ 1355.808920] kvm_check_nested_events+0x1c/0x40 [kvm]
[ 1355.809396] kvm_arch_vcpu_runnable+0x4e/0x190 [kvm]
[ 1355.809892] kvm_vcpu_check_block+0x4f/0x100 [kvm]
[ 1355.811259] kvm_vcpu_block+0x6b/0xa0 [kvm]
can occur due to kmap being called in non-sleepable (!TASK_RUNNING) context.
The fix is to extend kvm_x86_ops->nested_ops.hv_timer_pending() to cover
all events not already checked in kvm_arch_vcpu_is_runnable(), and then
get rid of the annoying (and wrong) call to kvm_check_nested_events()
from kvm_vcpu_check_block().
Beware, this is not a complete fix, because kvm_guest_apic_has_interrupt()
might still _read_ memory from non-sleepable context. The fix here is
probably to make kvm_arch_vcpu_is_runnable() return -EAGAIN, and in that
case do a round of kvm_vcpu_check_block() polling in sleepable context.
Nevertheless, it is a good start as it pushes the vmexit into vcpu_block().
The series also does a small cleanup pass on kvm_vcpu_check_block(),
removing KVM_REQ_UNHALT in favor of simply calling kvm_arch_vcpu_runnable()
again. Now that kvm_check_nested_events() is not called anymore by
kvm_arch_vcpu_runnable(), it is much easier to see that KVM will never
consume the event that caused kvm_vcpu_has_events() to return true,
and therefore it is safe to evaluate it again.
The alternative of propagating the return value of
kvm_arch_vcpu_runnable() up to kvm_vcpu_{block,halt}() is inferior
because it does not quite get right the edge cases where the vCPU becomes
runnable right before schedule() or right after kvm_vcpu_check_block().
While these edge cases are unlikely to truly matter in practice, it is
also pointless to get them "wrong".
v4:
- Make event request if INIT/SIPI is pending when GIF=>1 (SVM) and
on nested VM-Enter (VMX).
- Make an event request at VMXOFF iff it's necessary.
- Keep the INIT/SIPI pending vs. blocked checks separate (for the
above nSVM/nVMX fixes).
- Check the result of kvm_check_nested_events() in vcpu_block().
- Rename INIT/SIPI helpers (hopefully we'll eventually rename all of
the related collateral, e.g. "pending_events" is so misleading).
- Drop pending INIT/SIPI snaphsot to avoid creating weird, conflicting
code when kvm_check_nested_events() is called by vcpu_block().
v3:
- https://lore.kernel.org/all/20220822170659.2527086-1-pbonzini@redhat.com
- do not propagate the return value of kvm_arch_vcpu_runnable() up to
kvm_vcpu_{block,halt}()
- move and reformat the comment in vcpu_block()
move KVM_REQ_UNHALT removal last
Paolo Bonzini (5):
KVM: x86: make vendor code check for all nested events
KVM: x86: lapic does not have to process INIT if it is blocked
KVM: x86: never write to memory from kvm_vcpu_check_block()
KVM: mips, x86: do not rely on KVM_REQ_UNHALT
KVM: remove KVM_REQ_UNHALT
Sean Christopherson (7):
KVM: nVMX: Make an event request when pending an MTF nested VM-Exit
KVM: x86: Rename and expose helper to detect if INIT/SIPI are allowed
KVM: x86: Rename kvm_apic_has_events() to make it INIT/SIPI specific
KVM: SVM: Make an event request if INIT or SIPI is pending when GIF is
set
KVM: nVMX: Make an event request if INIT or SIPI is pending on
VM-Enter
KVM: nVMX: Make event request on VMXOFF iff INIT/SIPI is pending
KVM: x86: Don't snapshot pending INIT/SIPI prior to checking nested
events
Documentation/virt/kvm/vcpu-requests.rst | 28 +--------------
arch/arm64/kvm/arm.c | 1 -
arch/mips/kvm/emulate.c | 6 ++--
arch/powerpc/kvm/book3s_pr.c | 1 -
arch/powerpc/kvm/book3s_pr_papr.c | 1 -
arch/powerpc/kvm/booke.c | 1 -
arch/powerpc/kvm/powerpc.c | 1 -
arch/riscv/kvm/vcpu_insn.c | 1 -
arch/s390/kvm/kvm-s390.c | 2 --
arch/x86/include/asm/kvm_host.h | 2 +-
arch/x86/kvm/lapic.c | 38 ++++++--------------
arch/x86/kvm/lapic.h | 9 ++++-
arch/x86/kvm/svm/svm.c | 3 +-
arch/x86/kvm/vmx/nested.c | 33 +++++++++--------
arch/x86/kvm/vmx/vmx.c | 6 ++--
arch/x86/kvm/x86.c | 46 +++++++++++++++---------
arch/x86/kvm/x86.h | 5 ---
arch/x86/kvm/xen.c | 1 -
include/linux/kvm_host.h | 3 +-
virt/kvm/kvm_main.c | 4 +--
20 files changed, 79 insertions(+), 113 deletions(-)
base-commit: 5df50a4a9b60afba4dd2be76d0f0fb8ae8c9beab
--
2.37.3.968.ga6b4b080e4-goog
From: Sean Christopherson <seanjc@google.com> Date: 2022-09-21 00:33:03
From: Paolo Bonzini <pbonzini@redhat.com>
Interrupts, NMIs etc. sent while in guest mode are already handled
properly by the *_interrupt_allowed callbacks, but other events can
cause a vCPU to be runnable that are specific to guest mode.
In the case of VMX there are two, the preemption timer and the
monitor trap. The VMX preemption timer is already special cased via
the hv_timer_pending callback, but the purpose of the callback can be
easily extended to MTF or in fact any other event that can occur only
in guest mode.
Rename the callback and add an MTF check; kvm_arch_vcpu_runnable()
now can return true if an MTF is pending, without relying on
kvm_vcpu_running()'s call to kvm_check_nested_events(). Until that call
is removed, however, the patch introduces no functional change.
Reviewed-by: Maxim Levitsky <redacted>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
arch/x86/include/asm/kvm_host.h | 2 +-
arch/x86/kvm/vmx/nested.c | 8 +++++++-
arch/x86/kvm/x86.c | 8 ++++----
3 files changed, 12 insertions(+), 6 deletions(-)
From: Sean Christopherson <seanjc@google.com> Date: 2022-09-21 00:33:09
Set KVM_REQ_EVENT when MTF becomes pending to ensure that KVM will run
through inject_pending_event() and thus vmx_check_nested_events() prior
to re-entering the guest.
MTF currently works by virtue of KVM's hack that calls
kvm_check_nested_events() from kvm_vcpu_running(), but that hack will
be removed in the near future. Until that call is removed, the patch
introduces no real functional change.
Fixes: 5ef8acbdd687 ("KVM: nVMX: Emulate MTF when performing instruction emulation")
Cc: stable@vger.kernel.org
Reviewed-by: Maxim Levitsky <redacted>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
arch/x86/kvm/vmx/nested.c | 3 +++
arch/x86/kvm/vmx/vmx.c | 6 ++++--
2 files changed, 7 insertions(+), 2 deletions(-)
From: Sean Christopherson <seanjc@google.com> Date: 2022-09-21 00:33:15
Rename and invert kvm_vcpu_latch_init() to kvm_apic_init_sipi_allowed()
so as to match the behavior of {interrupt,nmi,smi}_allowed(), and expose
the helper so that it can be used by kvm_vcpu_has_events() to determine
whether or not an INIT or SIPI is pending _and_ can be taken immediately.
Opportunistically replaced usage of the "latch" terminology with "blocked"
and/or "allowed", again to align with KVM's terminology used for all other
event types.
No functional change intended.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
arch/x86/kvm/lapic.c | 4 ++--
arch/x86/kvm/lapic.h | 7 +++++++
arch/x86/kvm/x86.c | 9 +++++----
arch/x86/kvm/x86.h | 5 -----
4 files changed, 14 insertions(+), 11 deletions(-)
From: Sean Christopherson <seanjc@google.com> Date: 2022-09-21 00:33:23
Rename kvm_apic_has_events() to kvm_apic_has_pending_init_or_sipi() so
that it's more obvious that "events" really just means "INIT or SIPI".
Opportunistically clean up a weirdly worded comment that referenced
kvm_apic_has_events() instead of kvm_apic_accept_events().
No functional change intended.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
arch/x86/kvm/lapic.h | 2 +-
arch/x86/kvm/x86.c | 6 +++---
2 files changed, 4 insertions(+), 4 deletions(-)
From: Sean Christopherson <seanjc@google.com> Date: 2022-09-21 00:33:27
From: Paolo Bonzini <pbonzini@redhat.com>
Do not return true from kvm_vcpu_has_events() if the vCPU isn' going to
immediately process a pending INIT/SIPI. INIT/SIPI shouldn't be treated
as wake events if they are blocked.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
[sean: rebase onto refactored INIT/SIPI helpers, massage changelog]
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
arch/x86/kvm/x86.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
From: Sean Christopherson <seanjc@google.com> Date: 2022-09-21 00:33:40
Set KVM_REQ_EVENT if INIT or SIPI is pending when the guest enables GIF.
INIT in particular is blocked when GIF=0 and needs to be processed when
GIF is toggled to '1'. This bug has been masked by (a) KVM calling
->check_nested_events() in the core run loop and (b) hypervisors toggling
GIF from 0=>1 only when entering guest mode (L1 entering L2).
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
arch/x86/kvm/svm/svm.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
From: Sean Christopherson <seanjc@google.com> Date: 2022-09-21 00:33:47
Evaluate interrupts, i.e. set KVM_REQ_EVENT, if INIT or SIPI is pending
when emulating nested VM-Enter. INIT is blocked while the CPU is in VMX
root mode, but not in VMX non-root, i.e. becomes unblocked on VM-Enter.
This bug has been masked by KVM calling ->check_nested_events() in the
core run loop, but that hack will be fixed in the near future.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
arch/x86/kvm/vmx/nested.c | 18 ++++++------------
1 file changed, 6 insertions(+), 12 deletions(-)
From: Sean Christopherson <seanjc@google.com> Date: 2022-09-21 00:33:51
Don't snapshot pending INIT/SIPI events prior to checking nested events,
architecturally there's nothing wrong with KVM processing (dropping) a
SIPI that is received immediately after synthesizing a VM-Exit. Taking
and consuming the snapshot makes the flow way more subtle than it needs
to be, e.g. nVMX consumes/clears events that trigger VM-Exit (INIT/SIPI),
and so at first glance it appears that KVM is double-dipping on pending
INITs and SIPIs. But that's not the case because INIT is blocked
unconditionally in VMX root mode the CPU cannot be in wait-for_SIPI after
VM-Exit, i.e. the paths that truly consume the snapshot are unreachable
if apic->pending_events is modified by kvm_check_nested_events().
nSVM is a similar story as GIF is cleared by the CPU on VM-Exit; INIT is
blocked regardless of whether or not it was pending prior to VM-Exit.
Drop the snapshot logic so that a future fix doesn't create weirdness
when kvm_vcpu_running()'s call to kvm_check_nested_events() is moved to
vcpu_block(). In that case, kvm_check_nested_events() will be called
immediately before kvm_apic_accept_events(), which raises the obvious
question of why that change doesn't break the snapshot logic.
Note, there is a subtle functional change. Previously, KVM would clear
pending SIPIs if and only SIPI was pending prior to VM-Exit, whereas now
KVM clears pending SIPI unconditionally if INIT+SIPI are blocked. The
latter is architecturally allowed, as SIPI is ignored if the CPU is not
in wait-for-SIPI mode (arguably, KVM should be even more aggressive in
dropping SIPIs). It is software's responsibility to ensure the SIPI is
delivered, i.e. software shouldn't be firing INIT-SIPI at a CPU until
it knows with 100% certaining that the target CPU isn't in VMX root mode.
Furthermore, the existing code is extra weird as SIPIs that arrive after
VM-Exit _are_ dropped if there also happened to be a pending SIPI before
VM-Exit.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
arch/x86/kvm/lapic.c | 36 ++++++++++--------------------------
1 file changed, 10 insertions(+), 26 deletions(-)
@@ -3025,17 +3025,8 @@ int kvm_apic_accept_events(struct kvm_vcpu *vcpu)structkvm_lapic*apic=vcpu->arch.apic;u8sipi_vector;intr;-unsignedlongpe;-if(!lapic_in_kernel(vcpu))-return0;--/*-*Readpendingeventsbeforecallingthecheck_events-*callback.-*/-pe=smp_load_acquire(&apic->pending_events);-if(!pe)+if(!kvm_apic_has_pending_init_or_sipi(vcpu))return0;if(is_guest_mode(vcpu)){
@@ -3043,38 +3034,31 @@ int kvm_apic_accept_events(struct kvm_vcpu *vcpu)if(r<0)returnr==-EBUSY?0:r;/*-*Ifaneventhashappenedandcausedavmexit,-*weknowINITsarelatchedandtherefore-*wewillnotincorrectlydeliveranAPIC-*eventinsteadofavmexit.+*ContinueprocessingINIT/SIPIevenifanestedVM-Exit+*occurred,e.g.pendingSIPIsshouldbedroppedifINIT+SIPI+*areblockedasaresultoftransitioningtoVMXrootmode.*/}/*-*INITsareblockedwhileCPUisinspecificstates-*(SMM,VMXrootmode,SVMwithGIF=0).-*BecauseaCPUcannotbeinthesestatesimmediately-*afterithasprocessedanINITsignal(andthusin-*KVM_MP_STATE_INIT_RECEIVEDstate),justeatSIPIs-*andleavetheINITpending.+*INITsareblockedwhileCPUisinspecificstates(SMM,VMXroot+*mode,SVMwithGIF=0),whileSIPIsaredroppediftheCPUisn'tin+*wait-for-SIPI(WFS).*/if(!kvm_apic_init_sipi_allowed(vcpu)){WARN_ON_ONCE(vcpu->arch.mp_state==KVM_MP_STATE_INIT_RECEIVED);-if(test_bit(KVM_APIC_SIPI,&pe))-clear_bit(KVM_APIC_SIPI,&apic->pending_events);+clear_bit(KVM_APIC_SIPI,&apic->pending_events);return0;}-if(test_bit(KVM_APIC_INIT,&pe)){-clear_bit(KVM_APIC_INIT,&apic->pending_events);+if(test_and_clear_bit(KVM_APIC_INIT,&apic->pending_events)){kvm_vcpu_reset(vcpu,true);if(kvm_vcpu_is_bsp(apic->vcpu))vcpu->arch.mp_state=KVM_MP_STATE_RUNNABLE;elsevcpu->arch.mp_state=KVM_MP_STATE_INIT_RECEIVED;}-if(test_bit(KVM_APIC_SIPI,&pe)){-clear_bit(KVM_APIC_SIPI,&apic->pending_events);+if(test_and_clear_bit(KVM_APIC_SIPI,&apic->pending_events)){if(vcpu->arch.mp_state==KVM_MP_STATE_INIT_RECEIVED){/* evaluate pending_events before reading the vector */smp_rmb();
From: Sean Christopherson <seanjc@google.com> Date: 2022-09-21 00:33:57
Explicitly check for a pending INIT/SIPI event when emulating VMXOFF
instead of blindly making an event request. There's obviously no need
to evaluate events if none are pending.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
arch/x86/kvm/vmx/nested.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
@@ -5193,8 +5193,8 @@ static int handle_vmxoff(struct kvm_vcpu *vcpu)free_nested(vcpu);-/* Process a latched INIT during time CPU was in VMX operation */-kvm_make_request(KVM_REQ_EVENT,vcpu);+if(kvm_apic_has_pending_init_or_sipi(vcpu))+kvm_make_request(KVM_REQ_EVENT,vcpu);returnnested_vmx_succeed(vcpu);}
From: Sean Christopherson <seanjc@google.com> Date: 2022-09-21 00:34:02
From: Paolo Bonzini <pbonzini@redhat.com>
kvm_vcpu_check_block() is called while not in TASK_RUNNING, and therefore
it cannot sleep. Writing to guest memory is therefore forbidden, but it
can happen on AMD processors if kvm_check_nested_events() causes a vmexit.
Fortunately, all events that are caught by kvm_check_nested_events() are
also recognized by kvm_vcpu_has_events() through vendor callbacks such as
kvm_x86_interrupt_allowed() or kvm_x86_ops.nested_ops->has_events(), so
remove the call and postpone the actual processing to vcpu_block().
Opportunistically honor the return of kvm_check_nested_events(). KVM
punted on the check in kvm_vcpu_running() because the only error path is
if vmx_complete_nested_posted_interrupt() fails, in which case KVM exits
to userspace with "internal error" i.e. the VM is likely dead anyways so
it wasn't worth overloading the return of kvm_vcpu_running().
Add the check mostly so that KVM is consistent with itself; the return of
the call via kvm_apic_accept_events()=>kvm_check_nested_events() that
immediately follows _is_ checked.
Reported-by: Maxim Levitsky <redacted>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
[sean: check and handle return of kvm_check_nested_events()]
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
arch/x86/kvm/x86.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
From: Sean Christopherson <seanjc@google.com> Date: 2022-09-21 00:34:04
From: Paolo Bonzini <pbonzini@redhat.com>
KVM_REQ_UNHALT is a weird request that simply reports the value of
kvm_arch_vcpu_runnable() on exit from kvm_vcpu_halt(). Only
MIPS and x86 are looking at it, the others just clear it. Check
the state of the vCPU directly so that the request is handled
as a nop on all architectures.
No functional change intended, except for corner cases where an
event arrive immediately after a signal become pending or after
another similar host-side event.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
arch/mips/kvm/emulate.c | 7 +++----
arch/x86/kvm/x86.c | 9 ++++++++-
2 files changed, 11 insertions(+), 5 deletions(-)
@@ -97,7 +97,7 @@ VCPU requests are simply bit indices of the ``vcpu->requests`` bitmap. This means general bitops, like those documented in [atomic-ops]_ could also be used, e.g. ::- clear_bit(KVM_REQ_UNHALT & KVM_REQUEST_MASK, &vcpu->requests);+ clear_bit(KVM_REQ_UNBLOCK & KVM_REQUEST_MASK, &vcpu->requests); However, VCPU request users should refrain from doing so, as it would break the abstraction. The first 8 bits are reserved for architecture
@@ -126,17 +126,6 @@ KVM_REQ_UNBLOCK or in order to update the interrupt routing and ensure that assigned devices will wake up the vCPU.-KVM_REQ_UNHALT-- This request may be made from the KVM common function kvm_vcpu_block(),- which is used to emulate an instruction that causes a CPU to halt until- one of an architectural specific set of events and/or interrupts is- received (determined by checking kvm_arch_vcpu_runnable()). When that- event or interrupt arrives kvm_vcpu_block() makes the request. This is- in contrast to when kvm_vcpu_block() returns due to any other reason,- such as a pending signal, which does not indicate the VCPU's halt- emulation should stop, and therefore does not make the request.- KVM_REQ_OUTSIDE_GUEST_MODE This "request" ensures the target vCPU has exited guest mode prior to the
@@ -297,21 +286,6 @@ architecture dependent. kvm_vcpu_block() calls kvm_arch_vcpu_runnable() to check if it should awaken. One reason to do so is to provide architectures a function where requests may be checked if necessary.-Clearing Requests--------------------Generally it only makes sense for the receiving VCPU thread to clear a-request. However, in some circumstances, such as when the requesting-thread and the receiving VCPU thread are executed serially, such as when-they are the same thread, or when they are using some form of concurrency-control to temporarily execute synchronously, then it's possible to know-that the request may be cleared immediately, rather than waiting for the-receiving VCPU thread to handle the request in VCPU RUN. The only current-examples of this are kvm_vcpu_block() calls made by VCPUs to block-themselves. A possible side-effect of that call is to make the-KVM_REQ_UNHALT request, which may then be cleared immediately when the-VCPU returns from the call.- References ==========
@@ -499,7 +499,6 @@ static void kvmppc_set_msr_pr(struct kvm_vcpu *vcpu, u64 msr)if(msr&MSR_POW){if(!vcpu->arch.pending_exceptions){kvm_vcpu_halt(vcpu);-kvm_clear_request(KVM_REQ_UNHALT,vcpu);vcpu->stat.generic.halt_wakeup++;/* Unset POW bit after we woke up */
@@ -4343,8 +4343,6 @@ static int kvm_s390_handle_requests(struct kvm_vcpu *vcpu)gotoretry;}-/* nothing to do, just clear the request */-kvm_clear_request(KVM_REQ_UNHALT,vcpu);/* we left the vsie handler, nothing to do, just clear the request */kvm_clear_request(KVM_REQ_VSIE_RESTART,vcpu);
From: Philippe Mathieu-Daudé <hidden> Date: 2022-09-22 13:18:29
On Wed, Sep 21, 2022 at 2:34 AM Sean Christopherson [off-list ref] wrote:
From: Paolo Bonzini <pbonzini@redhat.com>
KVM_REQ_UNHALT is a weird request that simply reports the value of
kvm_arch_vcpu_runnable() on exit from kvm_vcpu_halt(). Only
MIPS and x86 are looking at it, the others just clear it. Check
the state of the vCPU directly so that the request is handled
as a nop on all architectures.
No functional change intended, except for corner cases where an
event arrive immediately after a signal become pending or after
another similar host-side event.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
arch/mips/kvm/emulate.c | 7 +++----
arch/x86/kvm/x86.c | 9 ++++++++-
2 files changed, 11 insertions(+), 5 deletions(-)
From: Marc Zyngier <maz@kernel.org> Date: 2022-09-22 14:52:25
On Wed, 21 Sep 2022 01:32:01 +0100,
Sean Christopherson [off-list ref] wrote:
From: Paolo Bonzini <pbonzini@redhat.com>
KVM_REQ_UNHALT is now unnecessary because it is replaced by the return
value of kvm_vcpu_block/kvm_vcpu_halt. Remove it.
No functional change intended.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
Acked-by: Marc Zyngier <maz@kernel.org>
M.
--
Without deviation from the norm, progress is not possible.
From: Jim Mattson <hidden> Date: 2023-12-07 01:03:22
kvm_vcpu_check_block() is called while not in TASK_RUNNING, and therefore
it cannot sleep. Writing to guest memory is therefore forbidden, but it
can happen on AMD processors if kvm_check_nested_events() causes a vmexit.
Fortunately, all events that are caught by kvm_check_nested_events() are
also recognized by kvm_vcpu_has_events() through vendor callbacks such as
kvm_x86_interrupt_allowed() or kvm_x86_ops.nested_ops->has_events(), so
remove the call and postpone the actual processing to vcpu_block().
Opportunistically honor the return of kvm_check_nested_events(). KVM
punted on the check in kvm_vcpu_running() because the only error path is
if vmx_complete_nested_posted_interrupt() fails, in which case KVM exits
to userspace with "internal error" i.e. the VM is likely dead anyways so
it wasn't worth overloading the return of kvm_vcpu_running().
Add the check mostly so that KVM is consistent with itself; the return of
the call via kvm_apic_accept_events()=>kvm_check_nested_events() that
immediately follows _is_ checked.
Reported-by: Maxim Levitsky <redacted>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
[sean: check and handle return of kvm_check_nested_events()]
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
arch/x86/kvm/x86.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
@@ -10815,6 +10815,17 @@ static inline int vcpu_block(struct kvm_vcpu *vcpu)return1;}+/*+*Evaluatenestedeventsbeforeexitingthehaltedstate.Thisallows+*thehaltstatetoberecordedproperlyintheVMCS12'sactivity+*statefield(AMDdoesnothaveasimilarfieldandaVM-Exitalways+*causesaspuriouswakeupfromHLT).+*/+if(is_guest_mode(vcpu)){+if(kvm_check_nested_events(vcpu)<0)+return0;+}+if(kvm_apic_accept_events(vcpu)<0)return0;switch(vcpu->arch.mp_state){
@@ -10837,9 +10848,6 @@ static inline int vcpu_block(struct kvm_vcpu *vcpu)staticinlineboolkvm_vcpu_running(structkvm_vcpu*vcpu){-if(is_guest_mode(vcpu))-kvm_check_nested_events(vcpu);-return(vcpu->arch.mp_state==KVM_MP_STATE_RUNNABLE&&!vcpu->arch.apf.halted);}
This commit breaks delivery of a (virtualized) posted interrupt from
an L1 vCPU to a halted L2 vCPU.
Looking back at commit e6c67d8cf117 ("KVM: nVMX: Wake blocked vCPU in
guest-mode if pending interrupt in virtual APICv"), Liran wrote:
Note that this also handles the case of nested posted-interrupt by the
fact RVI is updated in vmx_complete_nested_posted_interrupt() which is
called from kvm_vcpu_check_block() -> kvm_arch_vcpu_runnable() ->
kvm_vcpu_running() -> vmx_check_nested_events() ->
vmx_complete_nested_posted_interrupt().
Clearly, that is no longer the case.
From: Sean Christopherson <seanjc@google.com> Date: 2023-12-07 16:21:10
On Wed, Dec 06, 2023, Jim Mattson wrote:
quoted hunk
kvm_vcpu_check_block() is called while not in TASK_RUNNING, and therefore
it cannot sleep. Writing to guest memory is therefore forbidden, but it
can happen on AMD processors if kvm_check_nested_events() causes a vmexit.
Fortunately, all events that are caught by kvm_check_nested_events() are
also recognized by kvm_vcpu_has_events() through vendor callbacks such as
kvm_x86_interrupt_allowed() or kvm_x86_ops.nested_ops->has_events(), so
remove the call and postpone the actual processing to vcpu_block().
Opportunistically honor the return of kvm_check_nested_events(). KVM
punted on the check in kvm_vcpu_running() because the only error path is
if vmx_complete_nested_posted_interrupt() fails, in which case KVM exits
to userspace with "internal error" i.e. the VM is likely dead anyways so
it wasn't worth overloading the return of kvm_vcpu_running().
Add the check mostly so that KVM is consistent with itself; the return of
the call via kvm_apic_accept_events()=>kvm_check_nested_events() that
immediately follows _is_ checked.
Reported-by: Maxim Levitsky <redacted>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
[sean: check and handle return of kvm_check_nested_events()]
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
arch/x86/kvm/x86.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
@@ -10815,6 +10815,17 @@ static inline int vcpu_block(struct kvm_vcpu *vcpu)return1;}+/*+*Evaluatenestedeventsbeforeexitingthehaltedstate.Thisallows+*thehaltstatetoberecordedproperlyintheVMCS12'sactivity+*statefield(AMDdoesnothaveasimilarfieldandaVM-Exitalways+*causesaspuriouswakeupfromHLT).+*/+if(is_guest_mode(vcpu)){+if(kvm_check_nested_events(vcpu)<0)+return0;+}+if(kvm_apic_accept_events(vcpu)<0)return0;switch(vcpu->arch.mp_state){
@@ -10837,9 +10848,6 @@ static inline int vcpu_block(struct kvm_vcpu *vcpu)staticinlineboolkvm_vcpu_running(structkvm_vcpu*vcpu){-if(is_guest_mode(vcpu))-kvm_check_nested_events(vcpu);-return(vcpu->arch.mp_state==KVM_MP_STATE_RUNNABLE&&!vcpu->arch.apf.halted);}
This commit breaks delivery of a (virtualized) posted interrupt from
an L1 vCPU to a halted L2 vCPU.
Looking back at commit e6c67d8cf117 ("KVM: nVMX: Wake blocked vCPU in
guest-mode if pending interrupt in virtual APICv"), Liran wrote:
Note that this also handles the case of nested posted-interrupt by the
fact RVI is updated in vmx_complete_nested_posted_interrupt() which is
called from kvm_vcpu_check_block() -> kvm_arch_vcpu_runnable() ->
kvm_vcpu_running() -> vmx_check_nested_events() ->
vmx_complete_nested_posted_interrupt().
Clearly, that is no longer the case.
Doh. We got the less obvious cases and missed the obvious one.
Ugh, and we also missed a related mess in kvm_guest_apic_has_interrupt(). That
thing should really be folded into vmx_has_nested_events().
Good gravy. And vmx_interrupt_blocked() does the wrong thing because that
specifically checks if L1 interrupts are blocked.
Compile tested only, and definitely needs to be chunked into multiple patches,
but I think something like this mess?
---
arch/x86/include/asm/kvm-x86-ops.h | 1 -
arch/x86/include/asm/kvm_host.h | 1 -
arch/x86/kvm/lapic.c | 20 ++---------------
arch/x86/kvm/lapic.h | 12 ++++++++++
arch/x86/kvm/vmx/nested.c | 36 ++++++++++++++++++++++++++++--
arch/x86/kvm/vmx/vmx.c | 34 ++++++++--------------------
arch/x86/kvm/vmx/vmx.h | 1 +
arch/x86/kvm/x86.c | 10 +--------
8 files changed, 59 insertions(+), 56 deletions(-)
From: Jim Mattson <hidden> Date: 2023-12-10 22:59:23
On Thu, Dec 7, 2023 at 8:21 AM Sean Christopherson [off-list ref] wrote:
Doh. We got the less obvious cases and missed the obvious one.
Ugh, and we also missed a related mess in kvm_guest_apic_has_interrupt(). That
thing should really be folded into vmx_has_nested_events().
Good gravy. And vmx_interrupt_blocked() does the wrong thing because that
specifically checks if L1 interrupts are blocked.
Compile tested only, and definitely needs to be chunked into multiple patches,
but I think something like this mess?
The proposed patch does not fix the problem. In fact, it messes things
up so much that I don't get any test results back.
Google has an internal K-U-T test that demonstrates the problem. I
will post it soon.
From: Sean Christopherson <seanjc@google.com> Date: 2023-12-12 15:29:52
On Sun, Dec 10, 2023, Jim Mattson wrote:
On Thu, Dec 7, 2023 at 8:21 AM Sean Christopherson [off-list ref] wrote:
quoted
Doh. We got the less obvious cases and missed the obvious one.
Ugh, and we also missed a related mess in kvm_guest_apic_has_interrupt(). That
thing should really be folded into vmx_has_nested_events().
Good gravy. And vmx_interrupt_blocked() does the wrong thing because that
specifically checks if L1 interrupts are blocked.
Compile tested only, and definitely needs to be chunked into multiple patches,
but I think something like this mess?
The proposed patch does not fix the problem. In fact, it messes things
up so much that I don't get any test results back.
Drat.
Google has an internal K-U-T test that demonstrates the problem. I
will post it soon.
Received, I'll dig in soonish, though "soonish" might unfortunately might mean
2024.
On Tue, 2023-12-12 at 07:28 -0800, Sean Christopherson wrote:
On Sun, Dec 10, 2023, Jim Mattson wrote:
quoted
On Thu, Dec 7, 2023 at 8:21 AM Sean Christopherson [off-list ref] wrote:
quoted
Doh. We got the less obvious cases and missed the obvious one.
Ugh, and we also missed a related mess in kvm_guest_apic_has_interrupt(). That
thing should really be folded into vmx_has_nested_events().
Good gravy. And vmx_interrupt_blocked() does the wrong thing because that
specifically checks if L1 interrupts are blocked.
Compile tested only, and definitely needs to be chunked into multiple patches,
but I think something like this mess?
The proposed patch does not fix the problem. In fact, it messes things
up so much that I don't get any test results back.
Drat.
quoted
Google has an internal K-U-T test that demonstrates the problem. I
will post it soon.
Received, I'll dig in soonish, though "soonish" might unfortunately might mean
2024.
Hi,
So this is what I think:
KVM does have kvm_guest_apic_has_interrupt() for this exact purpose,
to check if nested APICv has a pending interrupt before halting.
However the problem is bigger - with APICv we have in essence 2 pending interrupt
bitmaps - the PIR and the IRR, and to know if the guest has a pending interrupt
one has in theory to copy PIR to IRR, then see if the max is larger then the current PPR.
Since we don't want to write to guest memory, and the IRR here resides in the guest memory,
I guess we have to do a 'dry-run' version of 'vmx_complete_nested_posted_interrupt' and call
it from kvm_guest_apic_has_interrupt().
What do you think? I can prepare a patch for this.
Can you share a reproducer or write a new one that can be shared?
Best regards,
Maxim Levitsky
From: Jim Mattson <hidden> Date: 2023-12-13 22:32:03
On Wed, Dec 13, 2023 at 2:25 PM Maxim Levitsky [off-list ref] wrote:
On Tue, 2023-12-12 at 07:28 -0800, Sean Christopherson wrote:
quoted
On Sun, Dec 10, 2023, Jim Mattson wrote:
quoted
On Thu, Dec 7, 2023 at 8:21 AM Sean Christopherson [off-list ref] wrote:
quoted
Doh. We got the less obvious cases and missed the obvious one.
Ugh, and we also missed a related mess in kvm_guest_apic_has_interrupt(). That
thing should really be folded into vmx_has_nested_events().
Good gravy. And vmx_interrupt_blocked() does the wrong thing because that
specifically checks if L1 interrupts are blocked.
Compile tested only, and definitely needs to be chunked into multiple patches,
but I think something like this mess?
The proposed patch does not fix the problem. In fact, it messes things
up so much that I don't get any test results back.
Drat.
quoted
Google has an internal K-U-T test that demonstrates the problem. I
will post it soon.
Received, I'll dig in soonish, though "soonish" might unfortunately might mean
2024.
Hi,
So this is what I think:
KVM does have kvm_guest_apic_has_interrupt() for this exact purpose,
to check if nested APICv has a pending interrupt before halting.
However the problem is bigger - with APICv we have in essence 2 pending interrupt
bitmaps - the PIR and the IRR, and to know if the guest has a pending interrupt
one has in theory to copy PIR to IRR, then see if the max is larger then the current PPR.
Since we don't want to write to guest memory, and the IRR here resides in the guest memory,
I guess we have to do a 'dry-run' version of 'vmx_complete_nested_posted_interrupt' and call
it from kvm_guest_apic_has_interrupt().
What do you think? I can prepare a patch for this.
Can you share a reproducer or write a new one that can be shared?
On Wed, 2023-12-13 at 14:31 -0800, Jim Mattson wrote:
On Wed, Dec 13, 2023 at 2:25 PM Maxim Levitsky [off-list ref] wrote:
quoted
On Tue, 2023-12-12 at 07:28 -0800, Sean Christopherson wrote:
quoted
On Sun, Dec 10, 2023, Jim Mattson wrote:
quoted
On Thu, Dec 7, 2023 at 8:21 AM Sean Christopherson [off-list ref] wrote:
quoted
Doh. We got the less obvious cases and missed the obvious one.
Ugh, and we also missed a related mess in kvm_guest_apic_has_interrupt(). That
thing should really be folded into vmx_has_nested_events().
Good gravy. And vmx_interrupt_blocked() does the wrong thing because that
specifically checks if L1 interrupts are blocked.
Compile tested only, and definitely needs to be chunked into multiple patches,
but I think something like this mess?
The proposed patch does not fix the problem. In fact, it messes things
up so much that I don't get any test results back.
Drat.
quoted
Google has an internal K-U-T test that demonstrates the problem. I
will post it soon.
Received, I'll dig in soonish, though "soonish" might unfortunately might mean
2024.
Hi,
So this is what I think:
KVM does have kvm_guest_apic_has_interrupt() for this exact purpose,
to check if nested APICv has a pending interrupt before halting.
However the problem is bigger - with APICv we have in essence 2 pending interrupt
bitmaps - the PIR and the IRR, and to know if the guest has a pending interrupt
one has in theory to copy PIR to IRR, then see if the max is larger then the current PPR.
Since we don't want to write to guest memory, and the IRR here resides in the guest memory,
I guess we have to do a 'dry-run' version of 'vmx_complete_nested_posted_interrupt' and call
it from kvm_guest_apic_has_interrupt().
What do you think? I can prepare a patch for this.
Can you share a reproducer or write a new one that can be shared?
From: Sean Christopherson <seanjc@google.com> Date: 2023-12-13 22:59:22
On Thu, Dec 14, 2023, Maxim Levitsky wrote:
On Tue, 2023-12-12 at 07:28 -0800, Sean Christopherson wrote:
quoted
On Sun, Dec 10, 2023, Jim Mattson wrote:
quoted
On Thu, Dec 7, 2023 at 8:21 AM Sean Christopherson [off-list ref] wrote:
quoted
Doh. We got the less obvious cases and missed the obvious one.
Ugh, and we also missed a related mess in kvm_guest_apic_has_interrupt(). That
thing should really be folded into vmx_has_nested_events().
Good gravy. And vmx_interrupt_blocked() does the wrong thing because that
specifically checks if L1 interrupts are blocked.
Compile tested only, and definitely needs to be chunked into multiple patches,
but I think something like this mess?
The proposed patch does not fix the problem. In fact, it messes things
up so much that I don't get any test results back.
Drat.
quoted
Google has an internal K-U-T test that demonstrates the problem. I
will post it soon.
Received, I'll dig in soonish, though "soonish" might unfortunately might mean
2024.
Hi,
So this is what I think:
KVM does have kvm_guest_apic_has_interrupt() for this exact purpose,
to check if nested APICv has a pending interrupt before halting.
For all intents and purposes, so was nested_ops->has_events(). I don't see
any reason to have two APIs that do the same thing, and the call to
kvm_guest_apic_has_interrupt() is wrong in that it doesn't verify that IRQs are
enabled for _L2_. That's why my preference is to fold the two together.
However the problem is bigger - with APICv we have in essence 2 pending
interrupt bitmaps - the PIR and the IRR, and to know if the guest has a
pending interrupt one has in theory to copy PIR to IRR, then see if the max
is larger then the current PPR.
Yeah, this is what my untested hack-a-patch tried to do.
Since we don't want to write to guest memory,
The changelog is misleading/wrong. Writing guest memory is ok, what isn't safe
is blocking or sleeping, i.e. KVM must not trigger a host page fault due to
accessing a page that's been swapped out. Read vs. write doesn't matter.
So KVM can safely read and write guest memory so long as it already mapped by
kvm_vcpu_map() (or I suppose if we wrapped an access with pagefault_disable(),
but I can't think of a sane reason to do that). E.g. nVMX can access a vCPU's
PID mapping, but synthesizing a nested VM-Exit will cause explosions on nSVM.
and the IRR here resides in the guest memory, I guess we have to do a
'dry-run' version of 'vmx_complete_nested_posted_interrupt' and call it from
kvm_guest_apic_has_interrupt().
nested_ops->has_events() is the much better fit, e.g. the naming won't get weird
and we can gate the whole thing on is_guest_mode(). Though we probably need a
wrapper to handle any commonalities between nVMX and nSVM.
What do you think? I can prepare a patch for this.
As above, this is what I tried to do, sort of. Though it's obviously broken. We
don't need a full dry-run because KVM only needs to detect events that are unique
to L2, e.g. nVMX's preemption timer, MTF, and pending virtual interrupts (hmm,
I suspect nSVM's vNMI is broken too). Things like INIT and SMI don't require
nested virtualization awareness because the event itself is tracked for the vCPU
as a whole.