| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aUz2J/cK2PN/n0of@intel.com>
Date: Thu, 25 Dec 2025 16:30:31 +0800
From: Chao Gao <chao.gao@...el.com>
To: Sean Christopherson <seanjc@...gle.com>
CC: Paolo Bonzini <pbonzini@...hat.com>, <kvm@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, Dongli Zhang <dongli.zhang@...cle.com>
Subject: Re: [PATCH v3 06/10] KVM: nVMX: Switch to vmcs01 to update SVI
on-demand if L2 is active
On Fri, Dec 05, 2025 at 03:19:09PM -0800, Sean Christopherson wrote:
>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 <seanjc@...gle.com>
Reviewed-by: Chao Gao <chao.gao@...el.com>
A side topic below:
<snip>
>@@ -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);
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/ZxAL6thxEH67CpW7@google.com/
[2]: https://lore.kernel.org/kvm/20251205231913.441872-2-seanjc@google.com/
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(-)
diff --git a/arch/x86/include/asm/kvm-x86-ops.h b/arch/x86/include/asm/kvm-x86-ops.h
index de709fb5bd76..69aa64aa9910 100644
--- a/arch/x86/include/asm/kvm-x86-ops.h
+++ b/arch/x86/include/asm/kvm-x86-ops.h
@@ -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)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 5a3bfa293e8b..de57a90dac2f 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -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);
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 7be4d759884c..b0c87fa68f2a 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -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;
diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
index aa0a9b55dbb7..6242c8c7a682 100644
--- a/arch/x86/kvm/lapic.h
+++ b/arch/x86/kvm/lapic.h
@@ -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);
diff --git a/arch/x86/kvm/vmx/main.c b/arch/x86/kvm/vmx/main.c
index a46ccd670785..73dd74efcf9a 100644
--- a/arch/x86/kvm/vmx/main.c
+++ b/arch/x86/kvm/vmx/main.c
@@ -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,
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index ef8d29c677b9..e7883bf7665f 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -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);
diff --git a/arch/x86/kvm/vmx/x86_ops.h b/arch/x86/kvm/vmx/x86_ops.h
index d09abeac2b56..ab7349e67809 100644
--- a/arch/x86/kvm/vmx/x86_ops.h
+++ b/arch/x86/kvm/vmx/x86_ops.h
@@ -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);
--
2.47.3
Powered by blists - more mailing lists