KVM: VMX: Fix APICv activation bugs
Sean Christopherson
maintainerv3 cover KVM: VMX: Fix APICv activation bugs
Fix two bugs related to updating APICv state, add a regression test, and then rip out the "defer updates until nested VM-Exit" that contributed to bug #2, and eliminated a number ideas for fixing bug #1 (ignoring that my ideas weren't all that great).
The only thing that gives me pause is the TLB flushing logic in vmx_set_virtual_apic_mode(), mainly because I don't love open coding things like that. But for me, it's a much lesser evil than the mounting pile of booleans related to tracking deferred updates, and the mental gymnastics needed to understanding the interactions and ordering.
The fixes are tagged for stable@, and I'll probably land the selftest in 6.19 as well. Everything else is most definitely 6.20+ material.
v3:
- Add a selftest.
- Rip out the deferred updates stuff.
- Collect Chao's review.
- Add Dongli's fix for bug #2. [Chao]
v2:
- https://lore.kernel.org/all/[email protected]
- Add support for guest mode (suggested by Chao Gao).
- Add comments in the code (suggested by Chao Gao).
- Remove WARN_ON_ONCE from vmx_hwapic_isr_update().
- Edit commit message "AMD SVM APICv" to "AMD SVM AVIC" (suggested by Alejandro Jimenez).
Dongli Zhang (2):
KVM: VMX: Update SVI during runtime APICv activation
KVM: nVMX: Immediately refresh APICv controls as needed on nested
VM-Exit
Sean Christopherson (8):
KVM: selftests: Add a test to verify APICv updates (while L2 is
active)
KVM: nVMX: Switch to vmcs01 to update PML controls on-demand if L2 is
active
KVM: nVMX: Switch to vmcs01 to update TPR threshold on-demand if L2 is
active
KVM: nVMX: Switch to vmcs01 to update SVI on-demand if L2 is active
KVM: nVMX: Switch to vmcs01 to refresh APICv controls on-demand if L2
is active
KVM: nVMX: Switch to vmcs01 to update APIC page on-demand if L2 is
active
KVM: nVMX: Switch to vmcs01 to set virtual APICv mode on-demand if L2
is active
KVM: x86: Update APICv ISR (a.k.a. SVI) as part of
kvm_apic_update_apicv()
arch/x86/kvm/lapic.c | 21 +- arch/x86/kvm/lapic.h | 1 - arch/x86/kvm/vmx/nested.c | 30 +-- arch/x86/kvm/vmx/vmx.c | 104 +++++----- arch/x86/kvm/vmx/vmx.h | 9 - arch/x86/kvm/x86.c | 5 + tools/testing/selftests/kvm/Makefile.kvm | 1 + .../testing/selftests/kvm/include/x86/apic.h | 4 + .../kvm/x86/vmx_apicv_updates_test.c | 181 ++++++++++++++++++ 9 files changed, 257 insertions(+), 99 deletions(-) create mode 100644 tools/testing/selftests/kvm/x86/vmx_apicv_updates_test.c
base-commit: 5d3e2d9ba9ed68576c70c127e4f7446d896f2af2
Signature · 1 line
2.52.0.223.gf5cc29aaa4-goog
Sean Christopherson
maintainerPATCH v3 1/10 KVM: VMX: Update SVI during runtime APICv activation
From: Dongli Zhang <[email protected]>
The APICv (apic->apicv_active) can be activated or deactivated at runtime, for instance, because of APICv inhibit reasons. Intel VMX employs different mechanisms to virtualize LAPIC based on whether APICv is active.
When APICv is activated at runtime, GUEST_INTR_STATUS is used to configure and report the current pending IRR and ISR states. Unless a specific vector is explicitly included in EOI_EXIT_BITMAP, its EOI will not be trapped to KVM. Intel VMX automatically clears the corresponding ISR bit based on the GUEST_INTR_STATUS.SVI field.
When APICv is deactivated at runtime, the VM_ENTRY_INTR_INFO_FIELD is used to specify the next interrupt vector to invoke upon VM-entry. The VMX IDT_VECTORING_INFO_FIELD is used to report un-invoked vectors on VM-exit. EOIs are always trapped to KVM, so the software can manually clear pending ISR bits.
There are scenarios where, with APICv activated at runtime, a guest-issued EOI may not be able to clear the pending ISR bit.
Taking vector 236 as an example, here is one scenario.
1. Suppose APICv is inactive. Vector 236 is pending in the IRR.
2. To handle KVM_REQ_EVENT, KVM moves vector 236 from the IRR to the ISR, and configures the VM_ENTRY_INTR_INFO_FIELD via vmx_inject_irq().
3. After VM-entry, vector 236 is invoked through the guest IDT. At this point, the data in VM_ENTRY_INTR_INFO_FIELD is no longer valid. The guest interrupt handler for vector 236 is invoked.
4. Suppose a VM exit occurs very early in the guest interrupt handler, before the EOI is issued.
5. Nothing is reported through the IDT_VECTORING_INFO_FIELD because vector 236 has already been invoked in the guest.
6. Now, suppose APICv is activated. Before the next VM-entry, KVM calls kvm_vcpu_update_apicv() to activate APICv.
7. Unfortunately, GUEST_INTR_STATUS.SVI is not configured, although vector 236 is still pending in the ISR.
8. After VM-entry, the guest finally issues the EOI for vector 236. However, because SVI is not configured, vector 236 is not cleared.
9. ISR is stalled forever on vector 236.
Here is another scenario.
1. Suppose APICv is inactive. Vector 236 is pending in the IRR.
2. To handle KVM_REQ_EVENT, KVM moves vector 236 from the IRR to the ISR, and configures the VM_ENTRY_INTR_INFO_FIELD via vmx_inject_irq().
3. VM-exit occurs immediately after the next VM-entry. The vector 236 is not invoked through the guest IDT. Instead, it is saved to the IDT_VECTORING_INFO_FIELD during the VM-exit.
4. KVM calls kvm_queue_interrupt() to re-queue the un-invoked vector 236 into vcpu->arch.interrupt. A KVM_REQ_EVENT is requested.
5. Now, suppose APICv is activated. Before the next VM-entry, KVM calls kvm_vcpu_update_apicv() to activate APICv.
6. Although APICv is now active, KVM still uses the legacy VM_ENTRY_INTR_INFO_FIELD to re-inject vector 236. GUEST_INTR_STATUS.SVI is not configured.
7. After the next VM-entry, vector 236 is invoked through the guest IDT. Finally, an EOI occurs. However, due to the lack of GUEST_INTR_STATUS.SVI configuration, vector 236 is not cleared from the ISR.
8. ISR is stalled forever on vector 236.
Using QEMU as an example, vector 236 is stuck in ISR forever.
(qemu) info lapic 1
dumping local APIC state for CPU 1
LVT0 0x00010700 active-hi edge masked ExtINT (vec 0) LVT1 0x00010400 active-hi edge masked NMI
LVTPC 0x00000400 active-hi edge NMI
LVTERR 0x000000fe active-hi edge Fixed (vec 254)
LVTTHMR 0x00010000 active-hi edge masked Fixed (vec 0)
LVTT 0x000400ec active-hi edge tsc-deadline Fixed (vec 236)
Timer DCR=0x0 (divide by 2) initial_count = 0 current_count = 0
SPIV 0x000001ff APIC enabled, focus=off, spurious vec 255
ICR 0x000000fd physical edge de-assert no-shorthand
ICR2 0x00000000 cpu 0 (X2APIC ID)
ESR 0x00000000
ISR 236
IRR 37(level) 236
The issue isn't applicable to AMD SVM as KVM simply writes vmcb01 directly irrespective of whether L1 (vmcs01) or L2 (vmcb02) is active (unlike VMX, there is no need/cost to switch between VMCBs). In addition, APICV_INHIBIT_REASON_IRQWIN ensures AMD SVM AVIC is not activated until the last interrupt is EOI'd.
Fix the bug by configuring Intel VMX GUEST_INTR_STATUS.SVI if APICv is activated at runtime.
--- arch/x86/kvm/vmx/vmx.c | 9 --------- arch/x86/kvm/x86.c | 7 +++++++ 2 files changed, 7 insertions(+), 9 deletions(-)
arch/x86/kvm/vmx/vmx.c +0 -9
@@ -6937,15 +6937,6 @@ void vmx_hwapic_isr_update(struct kvm_vcpu *vcpu, int max_isr)* VM-Exit, otherwise L1 with run with a stale SVI. */ if (is_guest_mode (vcpu)) { -/* -* KVM is supposed to forward intercepted L2 EOIs to L1 if VID -* is enabled in vmcs12; as above, the EOIs affect L2's vAPIC. -* Note, userspace can stuff state while L2 is active; assert -* that VID is disabled if and only if the vCPU is in KVM_RUN -* to avoid false positives if userspace is setting APIC state. -*/ -WARN_ON_ONCE (vcpu->wants_to_run && -nested_cpu_has_vid (get_vmcs12 (vcpu)));to_vmx (vcpu)->nested.update_vmcs01_hwapic_isr = true;return ; }
arch/x86/kvm/x86.c +7 -0
@@ -10886,9 +10886,16 @@ void __kvm_vcpu_update_apicv(struct kvm_vcpu *vcpu)* pending. At the same time, KVM_REQ_EVENT may not be set as APICv was * still active when the interrupt got accepted. Make sure * kvm_check_and_inject_events() is called to check for that. +* +* Update SVI when APICv gets enabled, otherwise SVI won't reflect the +* highest bit in vISR and the next accelerated EOI in the guest won't +* be virtualized correctly (the CPU uses SVI to determine which vISR +* vector to clear). */ if (!apic->apicv_active)kvm_make_request (KVM_REQ_EVENT , vcpu); +else +kvm_apic_update_hwapic_isr (vcpu); out:preempt_enable ();
Signature · 1 line
2.52.0.223.gf5cc29aaa4-goog
Sean Christopherson
maintainerPATCH v3 2/10 KVM: nVMX: Immediately refresh APICv controls as needed on nested VM-Exit
In reply to Sean ChristophersonFrom: Dongli Zhang <[email protected]>
If an APICv status updated was pended while L2 was active, immediately refresh vmcs01's controls instead of pending KVM_REQ_APICV_UPDATE as kvm_vcpu_update_apicv() only calls into vendor code if a change is necessary.
E.g. if APICv is inhibited, and then activated while L2 is running:
kvm_vcpu_update_apicv()
|
-> __kvm_vcpu_update_apicv()
|
-> apic->apicv_active = true
|
-> vmx_refresh_apicv_exec_ctrl()
|
-> vmx->nested.update_vmcs01_apicv_status = true
|
-> returnThen L2 exits to L1:
__nested_vmx_vmexit()
|
-> kvm_make_request(KVM_REQ_APICV_UPDATE)
vcpu_enter_guest(): KVM_REQ_APICV_UPDATE
-> kvm_vcpu_update_apicv()
|
-> __kvm_vcpu_update_apicv()
|
-> return // because if (apic->apicv_active == activate)--- arch/x86/kvm/vmx/nested.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
arch/x86/kvm/vmx/nested.c +2 -1
@@ -19,6 +19,7 @@#include "trace.h" #include "vmx.h" #include "smm.h" +#include "x86_ops.h" static bool __read_mostly enable_shadow_vmcs =1 ;module_param_named (enable_shadow_vmcs, enable_shadow_vmcs, bool,S_IRUGO );
@@ -5165,7 +5166,7 @@ void __nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 vm_exit_reason,if (vmx->nested.update_vmcs01_apicv_status) { vmx->nested.update_vmcs01_apicv_status = false; -kvm_make_request (KVM_REQ_APICV_UPDATE , vcpu); +vmx_refresh_apicv_exec_ctrl (vcpu); }if (vmx->nested.update_vmcs01_hwapic_isr) {
Signature · 1 line
2.52.0.223.gf5cc29aaa4-goog
Sean Christopherson
maintainerPATCH v3 3/10 KVM: selftests: Add a test to verify APICv updates (while L2 is active)
In reply to Sean ChristophersonAdd a test to verify KVM correctly handles a variety of edge cases related to APICv updates, and in particular updates that are triggered while L2 is actively running.
--- tools/testing/selftests/kvm/Makefile.kvm | 1 + .../testing/selftests/kvm/include/x86/apic.h | 4 + .../kvm/x86/vmx_apicv_updates_test.c | 181 ++++++++++++++++++ 3 files changed, 186 insertions(+) create mode 100644 tools/testing/selftests/kvm/x86/vmx_apicv_updates_test.c
tools/testing/selftests/kvm/Makefile.kvm +1 -0
@@ -115,6 +115,7 @@ TEST_GEN_PROGS_x86 += x86/ucna_injection_test TEST_GEN_PROGS_x86 += x86/userspace_io_test TEST_GEN_PROGS_x86 += x86/userspace_msr_exit_test TEST_GEN_PROGS_x86 += x86/vmx_apic_access_test +TEST_GEN_PROGS_x86 += x86/vmx_apicv_updates_test TEST_GEN_PROGS_x86 += x86/vmx_dirty_log_test TEST_GEN_PROGS_x86 += x86/vmx_exception_with_invalid_guest_state TEST_GEN_PROGS_x86 += x86/vmx_msrs_test
tools/testing/selftests/kvm/include/x86/apic.h +4 -0
@@ -32,6 +32,7 @@#define APIC_SPIV 0xF0#define APIC_SPIV_FOCUS_DISABLED (1 << 9)#define APIC_SPIV_APIC_ENABLED (1 << 8) +#define APIC_ISR 0x100#define APIC_IRR 0x200#define APIC_ICR 0x300#define APIC_LVTCMCI 0x2f0
@@ -68,6 +69,9 @@#define APIC_TMCCT 0x390#define APIC_TDCR 0x3E0 +#define APIC_VECTOR_TO_BIT_NUMBER (v) ((unsigned int)(v) % 32) +#define APIC_VECTOR_TO_REG_OFFSET (v) ((unsigned int)(v) / 32 * 0x10) +void apic_disable (void );void xapic_enable (void );void x2apic_enable (void );
tools/testing/selftests/kvm/x86/vmx_apicv_updates_test.c new file +181 -0
181 lines not on this page · raw message
Signature · 1 line
2.52.0.223.gf5cc29aaa4-goog
Re: [PATCH v3 03/10] KVM: selftests: Add a test to verify APICv updates (while L2 is active)
Quoted 5 lines · Sean Christopherson
Add a test to verify KVM correctly handles a variety of edge cases related to APICv updates, and in particular updates that are triggered while L2 is actively running.
Signed-off-by Sean Christopherson <[email protected]>
A few nits below:
Quoted 12 lines
tools/testing/selftests/kvm/x86/vmx_apicv_updates_test.c new file +9
-0@@ -0,0 +1,181 @@ +// SPDX-License-Identifier: GPL-2.0-only +#include "test_util.h" +#include "kvm_util.h" +#include "processor.h" +#include "vmx.h" + +#define GOOD_IPI_VECTOR 0xe0 +#define BAD_IPI_VECTOR 0xf0 +
tools/testing/selftests/kvm/x86/vmx_apicv_updates_test.c line 19
Quoted 10 lines
tools/testing/selftests/kvm/x86/vmx_apicv_updates_test.c new file +10
-0+static volatile int good_ipis_received; + +static void good_ipi_handler (struct ex_regs *regs) +{ + good_ipis_received++; +} + +static void bad_ipi_handler (struct ex_regs *regs) +{ +TEST_FAIL ("Received \"bad\" IPI; ICR MMIO write should have been ignored" );
is it ok to use TEST_FAIL() in guest code?
Quoted 17 lines
tools/testing/selftests/kvm/x86/vmx_apicv_updates_test.c new file +17
-0+} + +static void l2_vmcall (void ) +{ +/* +* Exit to L1. Assume all registers may be clobbered as selftests's +* VM-Enter code doesn't preserve L2 GPRs. +*/ + asmvolatile ("push %%rbp\n\t" +"push %%r15\n\t" +"push %%r14\n\t" +"push %%r13\n\t" +"push %%r12\n\t" +"push %%rbx\n\t" +"push %%rdx\n\t" +"push %%rdi\n\t" + "vmcall\n\t"
tools/testing/selftests/kvm/x86/vmx_apicv_updates_test.c line 46
Quoted 10 lines
tools/testing/selftests/kvm/x86/vmx_apicv_updates_test.c new file +10
-0+"pop %%rdi\n\t" +"pop %%rdx\n\t" +"pop %%rbx\n\t" +"pop %%r12\n\t" +"pop %%r13\n\t" +"pop %%r14\n\t" +"pop %%r15\n\t" +"pop %%rbp\n\t" + ::: "rax", "rcx", "rdx", "rsi", "rdx", "r8", "r9", "r10", "r11", "memory"); +}
There's already a vmcall() helper in vmx.h. Why add a new one?
Quoted 5 lines
tools/testing/selftests/kvm/x86/vmx_apicv_updates_test.c new file +5
-0+int main (int argc,char *argv[]) +{ +vm_vaddr_t vmx_pages_gva; +struct vmx_pages *vmx; +struct kvm_vcpu *vcpu;
tools/testing/selftests/kvm/x86/vmx_apicv_updates_test.c line 151
Quoted 10 lines
tools/testing/selftests/kvm/x86/vmx_apicv_updates_test.c new file +10
-0+struct kvm_vm *vm; +struct ucall uc; + +TEST_REQUIRE (kvm_cpu_has (X86_FEATURE_VMX )); + + vm =vm_create_with_one_vcpu (&vcpu, l1_guest_code); + + vmx =vcpu_alloc_vmx (vm, &vmx_pages_gva); +prepare_virtualize_apic_accesses (vmx, vm); +vcpu_args_set (vcpu,2 , vmx_pages_gva);
s/2/1
only one argument here.
Sean Christopherson
maintainerRe: [PATCH v3 03/10] KVM: selftests: Add a test to verify APICv updates (while L2 is active)
Quoted 6 lines · Chao Gao
tools/testing/selftests/kvm/x86/vmx_apicv_updates_test.c line 19
Quoted 3 lines · Sean Christopherson
tools/testing/selftests/kvm/x86/vmx_apicv_updates_test.c new file +3
-0+static void bad_ipi_handler (struct ex_regs *regs) +{ +TEST_FAIL ("Received \"bad\" IPI; ICR MMIO write should have been ignored" );is it ok to use
TEST_FAIL()in guest code?
Doh. Good point. It'll definitely generate a failure, but not a very helpful one.
Quoted 19 lines
Quoted 19 lines
tools/testing/selftests/kvm/x86/vmx_apicv_updates_test.c new file +19
-0+static void l2_vmcall (void ) +{ +/* +* Exit to L1. Assume all registers may be clobbered as selftests's +* VM-Enter code doesn't preserve L2 GPRs. +*/ + asmvolatile ("push %%rbp\n\t" +"push %%r15\n\t" +"push %%r14\n\t" +"push %%r13\n\t" +"push %%r12\n\t" +"push %%rbx\n\t" +"push %%rdx\n\t" +"push %%rdi\n\t" +"vmcall\n\t" +"pop %%rdi\n\t" +"pop %%rdx\n\t" +"pop %%rbx\n\t" + "pop %%r12\n\t"
Quoted 8 lines
tools/testing/selftests/kvm/x86/vmx_apicv_updates_test.c line 46
Quoted 6 lines
tools/testing/selftests/kvm/x86/vmx_apicv_updates_test.c new file +6
-0+"pop %%r13\n\t" +"pop %%r14\n\t" +"pop %%r15\n\t" +"pop %%rbp\n\t" + ::: "rax", "rcx", "rdx", "rsi", "rdx", "r8", "r9", "r10", "r11", "memory"); +}There's already a
vmcall()helper invmx.h. Why add a new one?
Oh, nice, I somehow missed that.
Quoted 18 lines
Quoted 5 lines
tools/testing/selftests/kvm/x86/vmx_apicv_updates_test.c new file +5
-0+int main (int argc,char *argv[]) +{ +vm_vaddr_t vmx_pages_gva; +struct vmx_pages *vmx; +struct kvm_vcpu *vcpu;tools/testing/selftests/kvm/x86/vmx_apicv_updates_test.c line 151
Quoted 10 lines
tools/testing/selftests/kvm/x86/vmx_apicv_updates_test.c new file +10
-0+struct kvm_vm *vm; +struct ucall uc; + +TEST_REQUIRE (kvm_cpu_has (X86_FEATURE_VMX )); + + vm =vm_create_with_one_vcpu (&vcpu, l1_guest_code); + + vmx =vcpu_alloc_vmx (vm, &vmx_pages_gva); +prepare_virtualize_apic_accesses (vmx, vm); +vcpu_args_set (vcpu,2 , vmx_pages_gva);s/2/1
Quoted 1 line
only one argument here.
Gah. Thank you!
Sean Christopherson
maintainerPATCH v3 4/10 KVM: nVMX: Switch to vmcs01 to update PML controls on-demand if L2 is active
In reply to Sean ChristophersonIf KVM toggles "CPU dirty logging", a.k.a. Page-Modification Logging (PML), while L2 is active, temporarily load vmcs01 and immediately update the relevant controls instead of deferring the update until the next nested VM-Exit. For PML, deferring the update is relatively straightforward, but for several APICv related updates, deferring updates creates ordering and state consistency problems, e.g. KVM at-large thinks APICv is enabled, but vmcs01 is still running with stale (and effectively unknown) state.
Convert PML first precisely because it's the simplest case to handle: if something is broken with the vmcs01 <=> vmcs02 dance, then hopefully bugs will bisect here.
--- arch/x86/kvm/vmx/nested.c | 5 ----- arch/x86/kvm/vmx/vmx.c | 40 +++++++++++++++++++++++++++++++++++---- arch/x86/kvm/vmx/vmx.h | 1 - 3 files changed, 36 insertions(+), 10 deletions(-)
arch/x86/kvm/vmx/nested.c +0 -5
@@ -5152,11 +5152,6 @@ void __nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 vm_exit_reason,vmx_set_virtual_apic_mode (vcpu); } -if (vmx->nested.update_vmcs01_cpu_dirty_logging) { - vmx->nested.update_vmcs01_cpu_dirty_logging = false; -vmx_update_cpu_dirty_logging (vcpu); - } -nested_put_vmcs12_pages (vcpu);if (vmx->nested.reload_vmcs01_apic_access_page) {
arch/x86/kvm/vmx/vmx.c +36 -4
@@ -1594,6 +1594,41 @@ void vmx_vcpu_put(struct kvm_vcpu *vcpu)vmx_prepare_switch_to_host (to_vmx (vcpu)); } +static void vmx_switch_loaded_vmcs (struct kvm_vcpu *vcpu, +struct loaded_vmcs *vmcs) +{ +struct vcpu_vmx *vmx =to_vmx (vcpu); +int cpu; + + cpu =get_cpu (); + vmx->loaded_vmcs = vmcs; +vmx_vcpu_load_vmcs (vcpu, cpu); +put_cpu (); +} + +static void vmx_load_vmcs01 (struct kvm_vcpu *vcpu) +{ +struct vcpu_vmx *vmx =to_vmx (vcpu); + +if (!is_guest_mode (vcpu)) { +WARN_ON_ONCE (vmx->loaded_vmcs != &vmx->vmcs01); +return ; + } + +WARN_ON_ONCE (vmx->loaded_vmcs != &vmx->nested.vmcs02); +vmx_switch_loaded_vmcs (vcpu, &vmx->vmcs01); +} + +static void vmx_put_vmcs01 (struct kvm_vcpu *vcpu) +{ +if (!is_guest_mode (vcpu)) +return ; + +vmx_switch_loaded_vmcs (vcpu, &to_vmx (vcpu)->nested.vmcs02); +} +DEFINE_GUARD(vmx_vmcs01, struct kvm_vcpu *, + vmx_load_vmcs01(_T), vmx_put_vmcs01(_T)) +bool vmx_emulation_required (struct kvm_vcpu *vcpu) {return emulate_invalid_guest_state && !vmx_guest_state_valid (vcpu);
@@ -8267,10 +8302,7 @@ void vmx_update_cpu_dirty_logging(struct kvm_vcpu *vcpu)if (WARN_ON_ONCE (!enable_pml))return ; -if (is_guest_mode (vcpu)) { - vmx->nested.update_vmcs01_cpu_dirty_logging = true; -return ; - } +guard (vmx_vmcs01)(vcpu);/* * Note, nr_memslots_dirty_logging can be changed concurrent with this
arch/x86/kvm/vmx/vmx.h +0 -1
@@ -133,7 +133,6 @@ struct nested_vmx {bool change_vmcs01_virtual_apic_mode;bool reload_vmcs01_apic_access_page; -bool update_vmcs01_cpu_dirty_logging;bool update_vmcs01_apicv_status;bool update_vmcs01_hwapic_isr;
Signature · 1 line
2.52.0.223.gf5cc29aaa4-goog
Re: [PATCH v3 04/10] KVM: nVMX: Switch to vmcs01 to update PML controls on-demand if L2 is active
Quoted 8 lines · Sean Christopherson
If KVM toggles "CPU dirty logging", a.k.a. Page-Modification Logging (PML), while L2 is active, temporarily load vmcs01 and immediately update the relevant controls instead of deferring the update until the next nested VM-Exit. For PML, deferring the update is relatively straightforward, but for several APICv related updates, deferring updates creates ordering and state consistency problems, e.g. KVM at-large thinks APICv is enabled, but vmcs01 is still running with stale (and effectively unknown) state.
Quoted 5 lines
Convert PML first precisely because it's the simplest case to handle: if something is broken with the vmcs01 <=> vmcs02 dance, then hopefully bugs will bisect here.
Signed-off-by Sean Christopherson <[email protected]>
Sean Christopherson
maintainerPATCH v3 5/10 KVM: nVMX: Switch to vmcs01 to update TPR threshold on-demand if L2 is active
In reply to Sean ChristophersonIf KVM updates L1's TPR Threshold while L2 is active, temporarily load vmcs01 and immediately update TPR_THRESHOLD instead of deferring the update until the next nested VM-Exit. Deferring the TPR Threshold update is relatively straightforward, but for several APICv related updates, deferring updates creates ordering and state consistency problems, e.g. KVM at-large thinks APICv is enabled, but vmcs01 is still running with stale (and effectively unknown) state.
--- arch/x86/kvm/vmx/nested.c | 4 ---- arch/x86/kvm/vmx/vmx.c | 7 +++---- arch/x86/kvm/vmx/vmx.h | 3 --- 3 files changed, 3 insertions(+), 11 deletions(-)
arch/x86/kvm/vmx/nested.c +0 -4
@@ -2402,7 +2402,6 @@ static void prepare_vmcs02_early(struct vcpu_vmx *vmx, struct loaded_vmcs *vmcs0 exec_control &= ~CPU_BASED_TPR_SHADOW ; exec_control |= vmcs12->cpu_based_vm_exec_control; - vmx->nested.l1_tpr_threshold =-1 ;if (exec_control &CPU_BASED_TPR_SHADOW )vmcs_write32 (TPR_THRESHOLD , vmcs12->tpr_threshold);#ifdef CONFIG_X86_64
@@ -5144,9 +5143,6 @@ void __nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 vm_exit_reason,if (kvm_caps.has_tsc_control)vmcs_write64 (TSC_MULTIPLIER , vcpu->arch.tsc_scaling_ratio); -if (vmx->nested.l1_tpr_threshold !=-1 ) -vmcs_write32 (TPR_THRESHOLD , vmx->nested.l1_tpr_threshold); -if (vmx->nested.change_vmcs01_virtual_apic_mode) { vmx->nested.change_vmcs01_virtual_apic_mode = false;vmx_set_virtual_apic_mode (vcpu);
arch/x86/kvm/vmx/vmx.c +3 -4
@@ -6827,11 +6827,10 @@ void vmx_update_cr8_intercept(struct kvm_vcpu *vcpu, int tpr, int irr) nested_cpu_has(vmcs12, CPU_BASED_TPR_SHADOW)) return; +guard (vmx_vmcs01)(vcpu); + tpr_threshold = (irr ==-1 || tpr < irr) ?0 : irr; -if (is_guest_mode (vcpu)) -to_vmx (vcpu)->nested.l1_tpr_threshold = tpr_threshold; -else -vmcs_write32 (TPR_THRESHOLD , tpr_threshold); +vmcs_write32 (TPR_THRESHOLD , tpr_threshold); }void vmx_set_virtual_apic_mode (struct kvm_vcpu *vcpu)
arch/x86/kvm/vmx/vmx.h +0 -3
@@ -184,9 +184,6 @@ struct nested_vmx {u64 pre_vmenter_ssp;u64 pre_vmenter_ssp_tbl; -/* to migrate it to L1 if L2 writes to L1's CR8 directly */ -int l1_tpr_threshold; -u16 vpid02;u16 last_vpid;
Signature · 1 line
2.52.0.223.gf5cc29aaa4-goog
Re: [PATCH v3 05/10] KVM: nVMX: Switch to vmcs01 to update TPR threshold on-demand if L2 is active
Quoted 9 lines · Sean Christopherson
If KVM updates L1's TPR Threshold while L2 is active, temporarily load vmcs01 and immediately update
TPR_THRESHOLDinstead of deferring the update until the next nested VM-Exit. Deferring the TPR Threshold update is relatively straightforward, but for several APICv related updates, deferring updates creates ordering and state consistency problems, e.g. KVM at-large thinks APICv is enabled, but vmcs01 is still running with stale (and effectively unknown) state.Signed-off-by Sean Christopherson <[email protected]>
Sean Christopherson
maintainerPATCH v3 6/10 KVM: nVMX: Switch to vmcs01 to update SVI on-demand if L2 is active
In reply to Sean ChristophersonIf APICv is activated while L2 is running and triggers an SVI update, temporarily load vmcs01 and immediately update SVI instead of deferring the update until the next nested VM-Exit. This will eventually allow killing off kvm_apic_update_hwapic_isr(), and all of nVMX's deferred APICv updates.
--- arch/x86/kvm/vmx/nested.c | 5 ----- arch/x86/kvm/vmx/vmx.c | 19 +++++++------------ arch/x86/kvm/vmx/vmx.h | 1 - 3 files changed, 7 insertions(+), 18 deletions(-)
arch/x86/kvm/vmx/nested.c +0 -5
@@ -5160,11 +5160,6 @@ void __nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 vm_exit_reason,vmx_refresh_apicv_exec_ctrl (vcpu); } -if (vmx->nested.update_vmcs01_hwapic_isr) { - vmx->nested.update_vmcs01_hwapic_isr = false; -kvm_apic_update_hwapic_isr (vcpu); - } -if ((vm_exit_reason !=-1 ) && (enable_shadow_vmcs ||nested_vmx_is_evmptr12_valid (vmx))) vmx->nested.need_vmcs12_to_shadow_sync = true;
arch/x86/kvm/vmx/vmx.c +7 -12
@@ -6963,21 +6963,16 @@ void vmx_hwapic_isr_update(struct kvm_vcpu *vcpu, int max_isr)u16 status;u8 old; -/* -* If L2 is active, defer the SVI update until vmcs01 is loaded, as SVI -* is only relevant for if and only if Virtual Interrupt Delivery is -* enabled in vmcs12, and if VID is enabled then L2 EOIs affect L2's -* vAPIC, not L1's vAPIC. KVM must update vmcs01 on the next nested -* VM-Exit, otherwise L1 with run with a stale SVI. -*/ -if (is_guest_mode (vcpu)) { -to_vmx (vcpu)->nested.update_vmcs01_hwapic_isr = true; -return ; - } -if (max_isr ==-1 ) max_isr =0 ; +/* +* Always update SVI in vmcs01, as SVI is only relevant for L2 if and +* only if Virtual Interrupt Delivery is enabled in vmcs12, and if VID +* is enabled then L2 EOIs affect L2's vAPIC, not L1's vAPIC. +*/ +guard (vmx_vmcs01)(vcpu); + status =vmcs_read16 (GUEST_INTR_STATUS ); old = status >>8 ;if (max_isr != old) {
arch/x86/kvm/vmx/vmx.h +0 -1
@@ -134,7 +134,6 @@ struct nested_vmx {bool change_vmcs01_virtual_apic_mode;bool reload_vmcs01_apic_access_page;bool update_vmcs01_apicv_status; -bool update_vmcs01_hwapic_isr;/* * Enlightened VMCS has been enabled. It does not mean that L1 has to
Signature · 1 line
2.52.0.223.gf5cc29aaa4-goog
Re: [PATCH v3 06/10] KVM: nVMX: Switch to vmcs01 to update SVI on-demand if L2 is active
Quoted 7 lines · Sean Christopherson
If APICv is activated while L2 is running and triggers an SVI update, temporarily load vmcs01 and immediately update SVI instead of deferring the update until the next nested VM-Exit. This will eventually allow killing off
kvm_apic_update_hwapic_isr(), and all of nVMX's deferred APICv updates.Signed-off-by Sean Christopherson <[email protected]>
A side topic below:
<snip>
Quoted 15 lines
(quoted diff) +0
-11@@ -6963,21 +6963,16 @@ void vmx_hwapic_isr_update(struct kvm_vcpu *vcpu, int max_isr) u16 status; u8 old; - /* - * If L2 is active, defer the SVI update until vmcs01 is loaded, as SVI - * is only relevant for if and only if Virtual Interrupt Delivery is - * enabled in vmcs12, and if VID is enabled then L2 EOIs affect L2's - * vAPIC, not L1's vAPIC. KVM must update vmcs01 on the next nested - * VM-Exit, otherwise L1 with run with a stale SVI. - */ - if (is_guest_mode(vcpu)) { - to_vmx(vcpu)->nested.update_vmcs01_hwapic_isr = true; - return; - }
Review void vmx_hwapic_isr_update(struct kvm_vcpu *vcpu, int max_isr) · line 6974
Quoted 10 lines
(quoted diff) +6
-1- if (max_isr == -1) max_isr = 0; + /* + * Always update SVI in vmcs01, as SVI is only relevant for L2 if and + * only if Virtual Interrupt Delivery is enabled in vmcs12, and if VID + * is enabled then L2 EOIs affect L2's vAPIC, not L1's vAPIC. + */ + guard(vmx_vmcs01)(vcpu);
KVM calls this function when virtualizing EOI for L2, and in a previous discussion, you mentioned that the overhead of switching to VMCS01 is "non-trivial and unnecessary" (see [1]).
My testing shows that guard(vmx_vmcs01) takes about 140-250 cycles. I think this overhead is acceptable for nested scenarios, since it only affects EOI-induced VM-exits in specific/suboptimal configurations.
But I'm wondering whether KVM should update SVI on every VM-entry instead of doing it on-demand (i.e., when vISR gets changed). We've encountered two SVI-related bugs [1][2] that were difficult to debug. Preventing these issues entirely seems worthwhile, and the overhead of always updating SVI during VM-entry should be minimal since KVM already updates RVI (RVI and SVI are in the the same VMCS field) in vmx_sync_irr_to_pir() when APICv is enabled.
[1]: https://lore.kernel.org/kvm/[email protected]/
[2]: https://lore.kernel.org/kvm/[email protected]/
e.g.,
--- arch/x86/include/asm/kvm-x86-ops.h | 1 - arch/x86/include/asm/kvm_host.h | 1 - arch/x86/kvm/lapic.c | 24 ++++++++-------- arch/x86/kvm/lapic.h | 1 + arch/x86/kvm/vmx/main.c | 9 ------ arch/x86/kvm/vmx/vmx.c | 45 +++++++----------------------- arch/x86/kvm/vmx/x86_ops.h | 1 - 7 files changed, 22 insertions(+), 60 deletions(-)
arch/x86/include/asm/kvm-x86-ops.h +0 -1
@@ -84,7 +84,6 @@ KVM_X86_OP(enable_nmi_window) KVM_X86_OP(enable_irq_window) KVM_X86_OP_OPTIONAL(update_cr8_intercept) KVM_X86_OP(refresh_apicv_exec_ctrl) -KVM_X86_OP_OPTIONAL(hwapic_isr_update) KVM_X86_OP_OPTIONAL(load_eoi_exitmap) KVM_X86_OP_OPTIONAL(set_virtual_apic_mode) KVM_X86_OP_OPTIONAL(set_apic_access_page_addr)
arch/x86/include/asm/kvm_host.h +0 -0
@@ -1824,7 +1824,6 @@ struct kvm_x86_ops {
const unsigned long required_apicv_inhibits; bool allow_apicv_in_x2apic_without_x2apic_virtualization; void (*refresh_apicv_exec_ctrl)(struct kvm_vcpu *vcpu); - void (*hwapic_isr_update)(struct kvm_vcpu *vcpu, int isr); void (*load_eoi_exitmap)(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap); void (*set_virtual_apic_mode)(struct kvm_vcpu *vcpu); void (*set_apic_access_page_addr)(struct kvm_vcpu *vcpu);
arch/x86/kvm/lapic.c +0 -0
@@ -715,9 +715,7 @@ static inline void apic_set_isr(int vec, struct kvm_lapic *apic)
* because the processor can modify ISR under the hood. Instead
* just set SVI.
*/
- if (unlikely(apic->apicv_active))
- kvm_x86_call(hwapic_isr_update)(apic->vcpu, vec);
- else {
+ if (likely(!apic->apicv_active)) {
++apic->isr_count;
BUG_ON(apic->isr_count > MAX_APIC_VECTOR);
/*@@ -761,9 +759,7 @@ static inline void apic_clear_isr(int vec, struct kvm_lapic *apic)
* on the other hand isr_count and highest_isr_cache are unused
* and must be left alone.
*/
- if (unlikely(apic->apicv_active))
- kvm_x86_call(hwapic_isr_update)(apic->vcpu, apic_find_highest_isr(apic));
- else {
+ if (likely(!apic->apicv_active)) {
--apic->isr_count;
BUG_ON(apic->isr_count < 0);
apic->highest_isr_cache = -1;@@ -781,6 +777,12 @@ int kvm_lapic_find_highest_irr(struct kvm_vcpu *vcpu)
}
EXPORT_SYMBOL_FOR_KVM_INTERNAL(kvm_lapic_find_highest_irr);
+int kvm_lapic_find_highest_isr(struct kvm_vcpu *vcpu)
+{
+ return apic_find_highest_isr(vcpu->arch.apic);
+} +EXPORT_SYMBOL_FOR_KVM_INTERNAL(kvm_lapic_find_highest_isr); +
static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
int vector, int level, int trig_mode, struct dest_map *dest_map); @@ -2774,12 +2776,10 @@ void kvm_apic_update_apicv(struct kvm_vcpu *vcpu) */ apic->irr_pending = true;
- if (apic->apicv_active) {
+ if (apic->apicv_active)
apic->isr_count = 1;
- kvm_x86_call(hwapic_isr_update)(vcpu, apic_find_highest_isr(apic));
- } else {
+ else
apic->isr_count = count_vectors(apic->regs + APIC_ISR);- }
apic->highest_isr_cache = -1;
}
@@ -2907,10 +2907,8 @@ void kvm_lapic_reset(struct kvm_vcpu *vcpu, bool init_event)
vcpu->arch.pv_eoi.msr_val = 0;
apic_update_ppr(apic);
- if (apic->apicv_active) {
+ if (apic->apicv_active)
kvm_x86_call(apicv_post_state_restore)(vcpu);- kvm_x86_call(hwapic_isr_update)(vcpu, -1);
- }
vcpu->arch.apic_arb_prio = 0; vcpu->arch.apic_attention = 0;
arch/x86/kvm/lapic.h +1 -0
@@ -127,6 +127,7 @@ int kvm_apic_set_base(struct kvm_vcpu *vcpu, u64 value, bool host_initiated);int kvm_apic_get_state (struct kvm_vcpu *vcpu,struct kvm_lapic_state *s);int kvm_apic_set_state (struct kvm_vcpu *vcpu,struct kvm_lapic_state *s);int kvm_lapic_find_highest_irr (struct kvm_vcpu *vcpu); +int kvm_lapic_find_highest_isr (struct kvm_vcpu *vcpu);u64 kvm_get_lapic_tscdeadline_msr (struct kvm_vcpu *vcpu);void kvm_set_lapic_tscdeadline_msr (struct kvm_vcpu *vcpu,u64 data);
arch/x86/kvm/vmx/main.c +0 -0
@@ -282,14 +282,6 @@ static void vt_set_virtual_apic_mode(struct kvm_vcpu *vcpu)
return vmx_set_virtual_apic_mode(vcpu);
}
-static void vt_hwapic_isr_update(struct kvm_vcpu *vcpu, int max_isr) -{
- if (is_td_vcpu(vcpu))
- return;
-
- return vmx_hwapic_isr_update(vcpu, max_isr);
-}
-
static int vt_sync_pir_to_irr(struct kvm_vcpu *vcpu)
{
if (is_td_vcpu(vcpu))@@ -953,7 +945,6 @@ struct kvm_x86_ops vt_x86_ops __initdata = {
.load_eoi_exitmap = vt_op(load_eoi_exitmap), .apicv_pre_state_restore = pi_apicv_pre_state_restore, .required_apicv_inhibits = VMX_REQUIRED_APICV_INHIBITS,
- .hwapic_isr_update = vt_op(hwapic_isr_update),
.sync_pir_to_irr = vt_op(sync_pir_to_irr), .deliver_interrupt = vt_op(deliver_interrupt), .dy_apicv_has_pending_interrupt = pi_has_pending_interrupt,
arch/x86/kvm/vmx/vmx.c +0 -0
@@ -6957,45 +6957,20 @@ void vmx_set_apic_access_page_addr(struct kvm_vcpu *vcpu)
read_unlock(&vcpu->kvm->mmu_lock);
}
-void vmx_hwapic_isr_update(struct kvm_vcpu *vcpu, int max_isr) +static void vmx_set_rvi_svi(int rvi, int svi)
{
u16 status;- u8 old;
+ u16 new;
- if (max_isr == -1)
- max_isr = 0;
-
- /*
- * Always update SVI in vmcs01, as SVI is only relevant for L2 if and
- * only if Virtual Interrupt Delivery is enabled in vmcs12, and if VID
- * is enabled then L2 EOIs affect L2's vAPIC, not L1's vAPIC.
- */
- guard(vmx_vmcs01)(vcpu);
+ if (rvi == -1)
+ rvi = 0;
+ if (svi == -1)
+ svi = 0;
status = vmcs_read16(GUEST_INTR_STATUS);
- old = status >> 8;
- if (max_isr != old) {
- status &= 0xff;
- status |= max_isr << 8;
- vmcs_write16(GUEST_INTR_STATUS, status);
- }
-}
-
-static void vmx_set_rvi(int vector)
-{
- u16 status;
- u8 old;
-
- if (vector == -1)
- vector = 0;
-
- status = vmcs_read16(GUEST_INTR_STATUS);
- old = (u8)status & 0xff;
- if ((u8)vector != old) {
- status &= ~0xff;
- status |= (u8)vector;
- vmcs_write16(GUEST_INTR_STATUS, status);
- }
+ new = (rvi & 0xff) | ((u8)svi << 8);
+ if (new != status)
+ vmcs_write16(GUEST_INTR_STATUS, new);
}
int vmx_sync_pir_to_irr(struct kvm_vcpu *vcpu)
@@ -7037,7 +7012,7 @@ int vmx_sync_pir_to_irr(struct kvm_vcpu *vcpu)
* a VM-Exit and the subsequent entry will call sync_pir_to_irr. */ if (!is_guest_mode(vcpu) && kvm_vcpu_apicv_active(vcpu)) - vmx_set_rvi(max_irr); + vmx_set_rvi_svi(max_irr, kvm_lapic_find_highest_isr(vcpu)); else if (got_posted_interrupt) kvm_make_request(KVM_REQ_EVENT, vcpu);
arch/x86/kvm/vmx/x86_ops.h +0 -1
@@ -46,7 +46,6 @@ int vmx_check_intercept(struct kvm_vcpu *vcpu,bool vmx_apic_init_signal_blocked (struct kvm_vcpu *vcpu);void vmx_migrate_timers (struct kvm_vcpu *vcpu);void vmx_set_virtual_apic_mode (struct kvm_vcpu *vcpu); -void vmx_hwapic_isr_update (struct kvm_vcpu *vcpu,int max_isr);int vmx_sync_pir_to_irr (struct kvm_vcpu *vcpu); void vmx_deliver_interrupt(struct kvm_lapic *apic, int delivery_mode,
int trig_mode, int vector);
Signature · 1 line
2.47.3
Sean Christopherson
maintainerRe: [PATCH v3 06/10] KVM: nVMX: Switch to vmcs01 to update SVI on-demand if L2 is active
Quoted 35 lines · Chao Gao
Quoted 15 lines · Sean Christopherson
(quoted diff) +0
-11@@ -6963,21 +6963,16 @@ void vmx_hwapic_isr_update(struct kvm_vcpu *vcpu, int max_isr) u16 status; u8 old; - /* - * If L2 is active, defer the SVI update until vmcs01 is loaded, as SVI - * is only relevant for if and only if Virtual Interrupt Delivery is - * enabled in vmcs12, and if VID is enabled then L2 EOIs affect L2's - * vAPIC, not L1's vAPIC. KVM must update vmcs01 on the next nested - * VM-Exit, otherwise L1 with run with a stale SVI. - */ - if (is_guest_mode(vcpu)) { - to_vmx(vcpu)->nested.update_vmcs01_hwapic_isr = true; - return; - }Review void vmx_hwapic_isr_update(struct kvm_vcpu *vcpu, int max_isr) · line 6974
Quoted 10 lines
(quoted diff) +6
-1- if (max_isr == -1) max_isr = 0; + /* + * Always update SVI in vmcs01, as SVI is only relevant for L2 if and + * only if Virtual Interrupt Delivery is enabled in vmcs12, and if VID + * is enabled then L2 EOIs affect L2's vAPIC, not L1's vAPIC. + */ + guard(vmx_vmcs01)(vcpu);KVM calls this function when virtualizing EOI for L2, and in a previous discussion, you mentioned that the overhead of switching to VMCS01 is "non-trivial and unnecessary" (see [1]).
My testing shows that
guard(vmx_vmcs01)takes about 140-250 cycles. I think this overhead is acceptable for nested scenarios, since it only affects EOI-induced VM-exits in specific/suboptimal configurations.
Quoted 6 lines
But I'm wondering whether KVM should update SVI on every VM-entry instead of doing it on-demand (i.e., when vISR gets changed). We've encountered two SVI-related bugs [1][2] that were difficult to debug. Preventing these issues entirely seems worthwhile, and the overhead of always updating SVI during VM-entry should be minimal since KVM already updates RVI (RVI and SVI are in the the same VMCS field) in
vmx_sync_irr_to_pir()when APICv is enabled.
Hmm. At first glance, I _really_ like this idea, but I'm leaning fairly strongly towards keeping .hwapic_isr_update().
While small (~28 cycles on EMR), the runtime cost isn't zero, and it affects the fastpath. And number of useful updates is comically small. E.g. without a nested VM, AFAICT they basically never happen post-boot. Even when running nested VMs, the number of useful update when running L1 hovers around ~0.001%.
More importantly, KVM will carry most of the complexity related to vISR updates regardless of how KVM handles SVI because of the ISR caching for non-APICv systems. So while I acknowledge that we've had some nasty bugs and 100% agree that squashing them entirely is _very_ enticing, I think those bugs were due to what were effectively two systemic flaws in KVM: (1) not aligning SVI with KVM's ISR caching code, and (2) the whole "defer updates to nested VM-Exit" mess.
At the end of this series, both (1) and (2) are "solved". Huh. And now that I look at (1) again, the last patch is wrong (benignly wrong, but still wrong). The changelog says this:
First, it adds a call during kvm_lapic_reset(), but that's a glorified nop as the ISR has already been zeroed.
but that's simply not true. There's already a call in kvm_lapic_reset(). So that patch can be amended with:
arch/x86/kvm/lapic.c +1 -3
@@ -2907,10 +2907,8 @@ void kvm_lapic_reset(struct kvm_vcpu *vcpu, bool init_event) vcpu->arch.pv_eoi.msr_val =0 ;apic_update_ppr (apic); -if (apic->apicv_active) { +if (apic->apicv_active)kvm_x86_call (apicv_post_state_restore)(vcpu); -kvm_x86_call (hwapic_isr_update)(vcpu,-1 ); - } vcpu->arch.apic_arb_prio =0 ; vcpu->arch.apic_attention =0 ;
At which point updates to highest_isr_cache and .hwapic_isr_update() are fully symmetrical (ignoring that KVM simply invalidates highest_isr_cache instead of scanning the vISR on EOI and APICv changes).
So yeah, the more I look at all of this, the more I'm in favor of keeping .hwapic_isr_update(), e.g. if only to let it serve as a canary for finding issues related to highest_isr_cache and/or isr_count.
arch/x86/kvm/vmx/vmx.c void vmx_set_apic_access_page_addr(struct kvm_vcpu *vcpu) · line 6960
Quoted 10 lines
arch/x86/kvm/vmx/vmx.c +1
-1@@ -6957,45 +6957,20 @@ void vmx_set_apic_access_page_addr(struct kvm_vcpu *vcpu)read_unlock (&vcpu->kvm->mmu_lock); } -void vmx_hwapic_isr_update (struct kvm_vcpu *vcpu,int max_isr) +static void vmx_set_rvi_svi (int rvi,int svi)
If this ever goes anywhere, my vote would be to call this vmx_sync_guest_intr_status(), and pass in only @rvi, e.g.
static void vmx_sync_guest_intr_status(struct kvm_vcpu *vcpu, int rvi)
{
int svi = kvm_lapic_find_highest_isr(vcpu);
u16 status, new;
...}
arch/x86/kvm/vmx/vmx.c void vmx_set_apic_access_page_addr(struct kvm_vcpu *vcpu) · line 6962
Quoted 2 lines
arch/x86/kvm/vmx/vmx.c +1
-0status =vmcs_read16 (GUEST_INTR_STATUS ); + new = (rvi &0xff ) | ((u8 )svi <<8 );
I think this is technically undefined behavior? Due to a shift larger than type (casting to an 8-bit value and then shifting by 8). svi[31:8] should always be '0', but to be paranoid we could do:
new = (rvi & 0xff) | ((svi & 0xff) << 8);
Quoted 3 lines
arch/x86/kvm/vmx/vmx.c +2
-0+if (new != status) +vmcs_write16 (GUEST_INTR_STATUS , new); }
Re: [PATCH v3 06/10] KVM: nVMX: Switch to vmcs01 to update SVI on-demand if L2 is active
Quoted 15 lines
Quoted 6 lines
But I'm wondering whether KVM should update SVI on every VM-entry instead of doing it on-demand (i.e., when vISR gets changed). We've encountered two SVI-related bugs [1][2] that were difficult to debug. Preventing these issues entirely seems worthwhile, and the overhead of always updating SVI during VM-entry should be minimal since KVM already updates RVI (RVI and SVI are in the the same VMCS field) in
vmx_sync_irr_to_pir()when APICv is enabled.Hmm. At first glance, I _really_ like this idea, but I'm leaning fairly strongly towards keeping .hwapic_isr_update().
While small (~28 cycles on EMR), the runtime cost isn't zero, and it affects the fastpath. And number of useful updates is comically small. E.g. without a nested VM, AFAICT they basically never happen post-boot. Even when running nested VMs, the number of useful update when running L1 hovers around ~0.001%.
Quoted 8 lines
More importantly, KVM will carry most of the complexity related to vISR updates regardless of how KVM handles SVI because of the ISR caching for non-APICv systems. So while I acknowledge that we've had some nasty bugs and 100% agree that squashing them entirely is _very_ enticing, I think those bugs were due to what were effectively two systemic flaws in KVM: (1) not aligning SVI with KVM's ISR caching code, and (2) the whole "defer updates to nested VM-Exit" mess.
At the end of this series, both (1) and (2) are "solved".
Fair enough. Thanks for this explanation.
Quoted 9 lines
Huh. And now that I
look at (1) again, the last patch is wrong (benignly wrong, but still wrong). The changelog says this:First, it adds a call during
kvm_lapic_reset(), but that's a glorified nop as the ISR has already been zeroed.but that's simply not true. There's already a call in
kvm_lapic_reset(). So that patch can be amended with:
Yeah, the fix looks good to me.
Quoted 18 lines
arch/x86/kvm/lapic.c +1
-3@@ -2907,10 +2907,8 @@ void kvm_lapic_reset(struct kvm_vcpu *vcpu, bool init_event) vcpu->arch.pv_eoi.msr_val =0 ;apic_update_ppr (apic); -if (apic->apicv_active) { +if (apic->apicv_active)kvm_x86_call (apicv_post_state_restore)(vcpu); -kvm_x86_call (hwapic_isr_update)(vcpu,-1 ); - } vcpu->arch.apic_arb_prio =0 ; vcpu->arch.apic_attention =0 ;
Sean Christopherson
maintainerPATCH v3 7/10 KVM: nVMX: Switch to vmcs01 to refresh APICv controls on-demand if L2 is active
In reply to Sean ChristophersonIf APICv is (un)inhibited while L2 is running, temporarily load vmcs01 and immediately refresh the APICv controls in vmcs01 instead of deferring the update until the next nested VM-Exit. This all but eliminates potential ordering issues due to vmcs01 not being synchronized with kvm_lapic.apicv_active, e.g. where KVM _thinks_ it refreshed APICv, but vmcs01 still contains stale state.
--- arch/x86/kvm/vmx/nested.c | 5 ----- arch/x86/kvm/vmx/vmx.c | 5 +---- arch/x86/kvm/vmx/vmx.h | 1 - 3 files changed, 1 insertion(+), 10 deletions(-)
arch/x86/kvm/vmx/nested.c +0 -5
@@ -5155,11 +5155,6 @@ void __nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 vm_exit_reason,kvm_make_request (KVM_REQ_APIC_PAGE_RELOAD , vcpu); } -if (vmx->nested.update_vmcs01_apicv_status) { - vmx->nested.update_vmcs01_apicv_status = false; -vmx_refresh_apicv_exec_ctrl (vcpu); - } -if ((vm_exit_reason !=-1 ) && (enable_shadow_vmcs ||nested_vmx_is_evmptr12_valid (vmx))) vmx->nested.need_vmcs12_to_shadow_sync = true;
arch/x86/kvm/vmx/vmx.c +1 -4
@@ -4578,10 +4578,7 @@ void vmx_refresh_apicv_exec_ctrl(struct kvm_vcpu *vcpu) {struct vcpu_vmx *vmx =to_vmx (vcpu); -if (is_guest_mode (vcpu)) { - vmx->nested.update_vmcs01_apicv_status = true; -return ; - } +guard (vmx_vmcs01)(vcpu);pin_controls_set (vmx,vmx_pin_based_exec_ctrl (vmx));
arch/x86/kvm/vmx/vmx.h +0 -1
@@ -133,7 +133,6 @@ struct nested_vmx {bool change_vmcs01_virtual_apic_mode;bool reload_vmcs01_apic_access_page; -bool update_vmcs01_apicv_status;/* * Enlightened VMCS has been enabled. It does not mean that L1 has to
Signature · 1 line
2.52.0.223.gf5cc29aaa4-goog
Re: [PATCH v3 07/10] KVM: nVMX: Switch to vmcs01 to refresh APICv controls on-demand if L2 is active
Quoted 8 lines · Sean Christopherson
If APICv is (un)inhibited while L2 is running, temporarily load vmcs01 and immediately refresh the APICv controls in vmcs01 instead of deferring the update until the next nested VM-Exit. This all but eliminates potential ordering issues due to vmcs01 not being synchronized with kvm_lapic.apicv_active, e.g. where KVM _thinks_ it refreshed APICv, but vmcs01 still contains stale state.
Signed-off-by Sean Christopherson <[email protected]>
Sean Christopherson
maintainerPATCH v3 8/10 KVM: nVMX: Switch to vmcs01 to update APIC page on-demand if L2 is active
In reply to Sean ChristophersonIf the KVM-owned APIC-access page is migrated while L2 is running, temporarily load vmcs01 and immediately update APIC_ACCESS_ADDR instead of deferring the update until the next nested VM-Exit. Once changing the virtual APIC mode is converted to always do on-demand updates, all of the "defer until vmcs01 is active" logic will be gone.
--- arch/x86/kvm/vmx/nested.c | 5 ----- arch/x86/kvm/vmx/vmx.c | 7 ++----- arch/x86/kvm/vmx/vmx.h | 1 - 3 files changed, 2 insertions(+), 11 deletions(-)
arch/x86/kvm/vmx/nested.c +0 -5
@@ -5150,11 +5150,6 @@ void __nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 vm_exit_reason,nested_put_vmcs12_pages (vcpu); -if (vmx->nested.reload_vmcs01_apic_access_page) { - vmx->nested.reload_vmcs01_apic_access_page = false; -kvm_make_request (KVM_REQ_APIC_PAGE_RELOAD , vcpu); - } -if ((vm_exit_reason !=-1 ) && (enable_shadow_vmcs ||nested_vmx_is_evmptr12_valid (vmx))) vmx->nested.need_vmcs12_to_shadow_sync = true;
arch/x86/kvm/vmx/vmx.c +2 -5
@@ -6895,11 +6895,8 @@ void vmx_set_apic_access_page_addr(struct kvm_vcpu *vcpu)kvm_pfn_t pfn;bool writable; -/* Defer reload until vmcs01 is the current VMCS. */ -if (is_guest_mode (vcpu)) { -to_vmx (vcpu)->nested.reload_vmcs01_apic_access_page = true; -return ; - } +/* Note, the VIRTUALIZE_APIC_ACCESSES check needs to query vmcs01. */ +guard (vmx_vmcs01)(vcpu);if (!(secondary_exec_controls_get (to_vmx (vcpu)) &SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES ))
arch/x86/kvm/vmx/vmx.h +0 -1
@@ -132,7 +132,6 @@ struct nested_vmx {bool vmcs02_initialized;bool change_vmcs01_virtual_apic_mode; -bool reload_vmcs01_apic_access_page;/* * Enlightened VMCS has been enabled. It does not mean that L1 has to
Signature · 1 line
2.52.0.223.gf5cc29aaa4-goog
Re: [PATCH v3 08/10] KVM: nVMX: Switch to vmcs01 to update APIC page on-demand if L2 is active
Quoted 7 lines · Sean Christopherson
If the KVM-owned APIC-access page is migrated while L2 is running, temporarily load vmcs01 and immediately update
APIC_ACCESS_ADDRinstead of deferring the update until the next nested VM-Exit. Once changing the virtual APIC mode is converted to always do on-demand updates, all of the "defer until vmcs01 is active" logic will be gone.Signed-off-by Sean Christopherson <[email protected]>
Sean Christopherson
maintainerPATCH v3 9/10 KVM: nVMX: Switch to vmcs01 to set virtual APICv mode on-demand if L2 is active
In reply to Sean ChristophersonIf L1's virtual APIC mode changes while L2 is active, e.g. because L1 doesn't intercept writes to the APIC_BASE MSR and L2 changes the mode, temporarily load vmcs01 and do all of the necessary actions instead of deferring the update until the next nested VM-Exit.
This will help in fixing yet more issues related to updates while L2 is active, e.g. KVM neglects to update vmcs02 MSR intercepts if vmcs01's MSR intercepts are modified while L2 is active. Not updating x2APIC MSRs is benign because vmcs01's settings are not factored into vmcs02's bitmap, but deferring the x2APIC MSR updates would create a weird, inconsistent state.
--- arch/x86/kvm/vmx/nested.c | 5 ----- arch/x86/kvm/vmx/vmx.c | 17 +++++++++++------ arch/x86/kvm/vmx/vmx.h | 2 -- 3 files changed, 11 insertions(+), 13 deletions(-)
arch/x86/kvm/vmx/nested.c +0 -5
@@ -5143,11 +5143,6 @@ void __nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 vm_exit_reason,if (kvm_caps.has_tsc_control)vmcs_write64 (TSC_MULTIPLIER , vcpu->arch.tsc_scaling_ratio); -if (vmx->nested.change_vmcs01_virtual_apic_mode) { - vmx->nested.change_vmcs01_virtual_apic_mode = false; -vmx_set_virtual_apic_mode (vcpu); - } -nested_put_vmcs12_pages (vcpu); if ((vm_exit_reason != -1) &&
arch/x86/kvm/vmx/vmx.c +11 -6
@@ -6842,11 +6842,7 @@ void vmx_set_virtual_apic_mode(struct kvm_vcpu *vcpu) !cpu_has_vmx_virtualize_x2apic_mode())return ; -/* Postpone execution until vmcs01 is the current VMCS. */ -if (is_guest_mode (vcpu)) { - vmx->nested.change_vmcs01_virtual_apic_mode = true; -return ; - } +guard (vmx_vmcs01)(vcpu); sec_exec_control =secondary_exec_controls_get (vmx); sec_exec_control &= ~(SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES |
@@ -6869,8 +6865,17 @@ void vmx_set_virtual_apic_mode(struct kvm_vcpu *vcpu)* only do so if its physical address has changed, but * the guest may have inserted a non-APIC mapping into * the TLB while the APIC access page was disabled. +* +* If L2 is active, immediately flush L1's TLB instead +* of requesting a flush of the current TLB, because +* the current TLB context is L2's. */ -kvm_make_request (KVM_REQ_TLB_FLUSH_CURRENT , vcpu); +if (!is_guest_mode (vcpu)) +kvm_make_request (KVM_REQ_TLB_FLUSH_CURRENT , vcpu); +else if (!enable_ept) +vpid_sync_context (to_vmx (vcpu)->vpid); +else if (VALID_PAGE (vcpu->arch.root_mmu.root.hpa)) +vmx_flush_tlb_ept_root (vcpu->arch.root_mmu.root.hpa); }break ;case LAPIC_MODE_X2APIC :
arch/x86/kvm/vmx/vmx.h +0 -2
@@ -131,8 +131,6 @@ struct nested_vmx {*/ bool vmcs02_initialized; -bool change_vmcs01_virtual_apic_mode; -/* * Enlightened VMCS has been enabled. It does not mean that L1 has to * use it. However, VMX features available to L1 will be limited based
Signature · 1 line
2.52.0.223.gf5cc29aaa4-goog
Re: [PATCH v3 09/10] KVM: nVMX: Switch to vmcs01 to set virtual APICv mode on-demand if L2 is active
Quoted 5 lines · Sean Christopherson
If L1's virtual APIC mode changes while L2 is active, e.g. because L1 doesn't intercept writes to the
APIC_BASEMSR and L2 changes the mode, temporarily load vmcs01 and do all of the necessary actions instead of deferring the update until the next nested VM-Exit.
Quoted 7 lines
This will help in fixing yet more issues related to updates while L2 is active, e.g. KVM neglects to update vmcs02 MSR intercepts if vmcs01's MSR intercepts are modified while L2 is active. Not updating x2APIC MSRs is benign because vmcs01's settings are not factored into vmcs02's bitmap, but deferring the x2APIC MSR updates would create a weird, inconsistent state.
Signed-off-by Sean Christopherson <[email protected]>
Quoted 4 lines
arch/x86/kvm/vmx/vmx.c +0
-0@@ -6869,8 +6865,17 @@ void vmx_set_virtual_apic_mode(struct kvm_vcpu *vcpu)* only do so if its physical address has changed, but * the guest may have inserted a non-APIC mapping into * the TLB while the APIC access page was disabled.
arch/x86/kvm/vmx/vmx.c void vmx_set_virtual_apic_mode(struct kvm_vcpu *vcpu) · line 6876
Quoted 10 lines
arch/x86/kvm/vmx/vmx.c +8
-1+* +* If L2 is active, immediately flush L1's TLB instead +* of requesting a flush of the current TLB, because +* the current TLB context is L2's. */ -kvm_make_request (KVM_REQ_TLB_FLUSH_CURRENT , vcpu); +if (!is_guest_mode (vcpu)) +kvm_make_request (KVM_REQ_TLB_FLUSH_CURRENT , vcpu); +else if (!enable_ept) +vpid_sync_context (to_vmx (vcpu)->vpid);
Nit: there's already a local "vmx" variable available. you can use it directly.
Sean Christopherson
maintainerPATCH v3 10/10 KVM: x86: Update APICv ISR (a.k.a. SVI) as part of kvm_apic_update_apicv()
In reply to Sean ChristophersonFold the calls to .hwapic_isr_update() in kvm_apic_set_state() and __kvm_vcpu_update_apicv() into kvm_apic_update_apicv(), as updating SVI is directly related to updating KVM's own cache of ISR information, e.g. SVI is more or less the APICv equivalent of highest_isr_cache.
Note, calling .hwapic_isr_update() during kvm_apic_update_apicv() has two benign side effects. First, it adds a call during kvm_lapic_reset(), but that's a glorified nop as the ISR has already been zeroed. Second, it changes the order between .hwapic_isr_update() and .apicv_post_state_restore() in kvm_apic_set_state(), but the former is VMX-only and the latter is SVM-only, i.e. is also a glorified nop.
--- arch/x86/kvm/lapic.c | 21 +++++---------------- arch/x86/kvm/lapic.h | 1 - arch/x86/kvm/x86.c | 2 -- 3 files changed, 5 insertions(+), 19 deletions(-)
arch/x86/kvm/lapic.c +5 -16
@@ -770,17 +770,6 @@ static inline void apic_clear_isr(int vec, struct kvm_lapic *apic) } } -void kvm_apic_update_hwapic_isr (struct kvm_vcpu *vcpu) -{ -struct kvm_lapic *apic = vcpu->arch.apic; - -if (WARN_ON_ONCE (!lapic_in_kernel (vcpu)) || !apic->apicv_active) -return ; - -kvm_x86_call (hwapic_isr_update)(vcpu,apic_find_highest_isr (apic)); -} -EXPORT_SYMBOL_FOR_KVM_INTERNAL (kvm_apic_update_hwapic_isr); -int kvm_lapic_find_highest_irr (struct kvm_vcpu *vcpu) {/* This may race with setting of irr in __apic_accept_irq() and
@@ -2785,10 +2774,12 @@ void kvm_apic_update_apicv(struct kvm_vcpu *vcpu)*/ apic->irr_pending = true; -if (apic->apicv_active) +if (apic->apicv_active) { apic->isr_count =1 ; -else +kvm_x86_call (hwapic_isr_update)(vcpu,apic_find_highest_isr (apic)); + }else { apic->isr_count =count_vectors (apic->regs +APIC_ISR ); + } apic->highest_isr_cache =-1 ; }
@@ -3232,10 +3223,8 @@ int kvm_apic_set_state(struct kvm_vcpu *vcpu, struct kvm_lapic_state *s)__start_apic_timer (apic,APIC_TMCCT );kvm_lapic_set_reg (apic,APIC_TMCCT ,0 );kvm_apic_update_apicv (vcpu); -if (apic->apicv_active) { +if (apic->apicv_active)kvm_x86_call (apicv_post_state_restore)(vcpu); -kvm_x86_call (hwapic_isr_update)(vcpu,apic_find_highest_isr (apic)); - }kvm_make_request (KVM_REQ_EVENT , vcpu);#ifdef CONFIG_KVM_IOAPIC
arch/x86/kvm/lapic.h +0 -1
@@ -126,7 +126,6 @@ void kvm_apic_send_ipi(struct kvm_lapic *apic, u32 icr_low, u32 icr_high);int kvm_apic_set_base (struct kvm_vcpu *vcpu,u64 value,bool host_initiated);int kvm_apic_get_state (struct kvm_vcpu *vcpu,struct kvm_lapic_state *s);int kvm_apic_set_state (struct kvm_vcpu *vcpu,struct kvm_lapic_state *s); -void kvm_apic_update_hwapic_isr (struct kvm_vcpu *vcpu);int kvm_lapic_find_highest_irr (struct kvm_vcpu *vcpu);u64 kvm_get_lapic_tscdeadline_msr (struct kvm_vcpu *vcpu);
arch/x86/kvm/x86.c +0 -2
@@ -10894,8 +10894,6 @@ void __kvm_vcpu_update_apicv(struct kvm_vcpu *vcpu)*/ if (!apic->apicv_active)kvm_make_request (KVM_REQ_EVENT , vcpu); -else -kvm_apic_update_hwapic_isr (vcpu); out:preempt_enable ();
Signature · 1 line
2.52.0.223.gf5cc29aaa4-goog
Re: [PATCH v3 10/10] KVM: x86: Update APICv ISR (a.k.a. SVI) as part of kvm_apic_update_apicv()
Quoted 5 lines · Sean Christopherson
Fold the calls to .hwapic_isr_update() in
kvm_apic_set_state()and__kvm_vcpu_update_apicv()intokvm_apic_update_apicv(), as updating SVI is directly related to updating KVM's own cache of ISR information, e.g. SVI is more or less the APICv equivalent ofhighest_isr_cache.
Quoted 8 lines
Note, calling .hwapic_isr_update() during
kvm_apic_update_apicv()has two benign side effects. First, it adds a call duringkvm_lapic_reset(), but that's a glorified nop as the ISR has already been zeroed. Second, it changes the order between .hwapic_isr_update() and .apicv_post_state_restore() inkvm_apic_set_state(), but the former is VMX-only and the latter is SVM-only, i.e. is also a glorified nop.Signed-off-by Sean Christopherson <[email protected]>
Sean Christopherson
maintainerQuoted 5 lines · Sean Christopherson
Fix two bugs related to updating APICv state, add a regression test, and then rip out the "defer updates until nested VM-Exit" that contributed to bug #2, and eliminated a number ideas for fixing bug #1 (ignoring that my ideas weren't all that great).
Quoted 7 lines
The only thing that gives me pause is the TLB flushing logic in
vmx_set_virtual_apic_mode(), mainly because I don't love open coding things like that. But for me, it's a much lesser evil than the mounting pile of booleans related to tracking deferred updates, and the mental gymnastics needed to understanding the interactions and ordering.[...]
Applied 1 and 2 to kvm-x86 fixes.
[01/10] KVM: VMX: Update SVI during runtime APICv activation
https://github.com/kvm-x86/linux/commit/b2849bec936b [02/10] KVM: nVMX: Immediately refresh APICv controls as needed on nested VM-Exit https://github.com/kvm-x86/linux/commit/297631388309