| 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: <63886376-07d3-45f2-90b3-89e1b63501f3@intel.com>
Date: Thu, 27 Feb 2025 11:44:54 +1300
From: "Huang, Kai" <kai.huang@...el.com>
To: weizijie <zijie.wei@...ux.alibaba.com>, Sean Christopherson
<seanjc@...gle.com>, Paolo Bonzini <pbonzini@...hat.com>, Thomas Gleixner
<tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>, Borislav Petkov
<bp@...en8.de>, Dave Hansen <dave.hansen@...ux.intel.com>, <x86@...nel.org>,
"H . Peter Anvin" <hpa@...or.com>, <kvm@...r.kernel.org>,
<linux-kernel@...r.kernel.org>
CC: xuyun <xuyun_xy.xy@...ux.alibaba.com>
Subject: Re: [PATCH Resend] KVM: x86: ioapic: Optimize EOI handling to reduce
unnecessary VM exits
On 25/02/2025 7:42 pm, weizijie wrote:
> Address performance issues caused by a vector being reused by a
> non-IOAPIC source.
>
> Commit 0fc5a36dd6b3
> ("KVM: x86: ioapic: Fix level-triggered EOI and IOAPIC reconfigure race")
> addressed the issues related to EOI and IOAPIC reconfiguration races.
> However, it has introduced some performance concerns:
>
> Configuring IOAPIC interrupts while an interrupt request (IRQ) is
> already in service can unintentionally trigger a VM exit for other
> interrupts that normally do not require one, due to the settings of
> `ioapic_handled_vectors`. If the IOAPIC is not reconfigured during
> runtime, this issue persists, continuing to adversely affect
> performance.
Could you elaborate why the guest would configure the IOAPIC entry to
use the same vector of an IRQ which is already in service? Is it some
kinda temporary configuration (which means the guest will either the
reconfigure the vector of the conflicting IRQ or the IOAPIC entry soon)?
I.e., why would this issue persist?
If such "persist" is due to guest bug or bad behaviour I am not sure we
need to tackle that in KVM.
>
> Simple Fix Proposal:
> A straightforward solution is to record highest in-service IRQ that
> is pending at the time of the last scan. Then, upon the next guest
> exit, do a full KVM_REQ_SCAN_IOAPIC. This ensures that a re-scan of
> the ioapic occurs only when the recorded vector is EOI'd, and
> subsequently, the extra bit in the eoi_exit_bitmap are cleared,
> avoiding unnecessary VM exits.
>
> Co-developed-by: xuyun <xuyun_xy.xy@...ux.alibaba.com>
> Signed-off-by: xuyun <xuyun_xy.xy@...ux.alibaba.com>
> Signed-off-by: weizijie <zijie.wei@...ux.alibaba.com>
> ---
> arch/x86/include/asm/kvm_host.h | 1 +
> arch/x86/kvm/ioapic.c | 10 ++++++++--
> arch/x86/kvm/irq_comm.c | 9 +++++++--
> arch/x86/kvm/vmx/vmx.c | 9 +++++++++
> 4 files changed, 25 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 0b7af5902ff7..8c50e7b4a96f 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1062,6 +1062,7 @@ struct kvm_vcpu_arch {
> #if IS_ENABLED(CONFIG_HYPERV)
> hpa_t hv_root_tdp;
> #endif
> + u8 last_pending_vector;
> };
>
> struct kvm_lpage_info {
> diff --git a/arch/x86/kvm/ioapic.c b/arch/x86/kvm/ioapic.c
> index 995eb5054360..40252a800897 100644
> --- a/arch/x86/kvm/ioapic.c
> +++ b/arch/x86/kvm/ioapic.c
> @@ -297,10 +297,16 @@ void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, ulong *ioapic_handled_vectors)
> u16 dm = kvm_lapic_irq_dest_mode(!!e->fields.dest_mode);
>
> if (kvm_apic_match_dest(vcpu, NULL, APIC_DEST_NOSHORT,
> - e->fields.dest_id, dm) ||
> - kvm_apic_pending_eoi(vcpu, e->fields.vector))
> + e->fields.dest_id, dm))
> __set_bit(e->fields.vector,
> ioapic_handled_vectors);
> + else if (kvm_apic_pending_eoi(vcpu, e->fields.vector)) {
> + __set_bit(e->fields.vector,
> + ioapic_handled_vectors);
> + vcpu->arch.last_pending_vector = e->fields.vector >
> + vcpu->arch.last_pending_vector ? e->fields.vector :
> + vcpu->arch.last_pending_vector;
> + }
> }
> }
> spin_unlock(&ioapic->lock);
> diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
> index 8136695f7b96..1d23c52576e1 100644
> --- a/arch/x86/kvm/irq_comm.c
> +++ b/arch/x86/kvm/irq_comm.c
> @@ -426,9 +426,14 @@ void kvm_scan_ioapic_routes(struct kvm_vcpu *vcpu,
>
> if (irq.trig_mode &&
> (kvm_apic_match_dest(vcpu, NULL, APIC_DEST_NOSHORT,
> - irq.dest_id, irq.dest_mode) ||
> - kvm_apic_pending_eoi(vcpu, irq.vector)))
> + irq.dest_id, irq.dest_mode)))
> __set_bit(irq.vector, ioapic_handled_vectors);
> + else if (kvm_apic_pending_eoi(vcpu, irq.vector)) {
> + __set_bit(irq.vector, ioapic_handled_vectors);
> + vcpu->arch.last_pending_vector = irq.vector >
> + vcpu->arch.last_pending_vector ? irq.vector :
> + vcpu->arch.last_pending_vector;
> + }
> }
> }
> srcu_read_unlock(&kvm->irq_srcu, idx);
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 6c56d5235f0f..047cdd5964e5 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -5712,6 +5712,15 @@ static int handle_apic_eoi_induced(struct kvm_vcpu *vcpu)
>
> /* EOI-induced VM exit is trap-like and thus no need to adjust IP */
> kvm_apic_set_eoi_accelerated(vcpu, vector);
> +
> + /* When there are instances where ioapic_handled_vectors is
> + * set due to pending interrupts, clean up the record and do
> + * a full KVM_REQ_SCAN_IOAPIC.
> + */
Comment style:
/*
* When ...
*/
> + if (vcpu->arch.last_pending_vector == vector) {
> + vcpu->arch.last_pending_vector = 0;
> + kvm_make_request(KVM_REQ_SCAN_IOAPIC, vcpu);
> + }
As Sean commented before, this should be in a common code probably in
kvm_ioapic_send_eoi().
https://lore.kernel.org/all/Z2IDkWPz2rhDLD0P@google.com/
Powered by blists - more mailing lists