linux-kernel - Re: [PATCH 3/3] KVM: x86: Decouple APICv activation state from apicv_inhibit

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <60cef3e4-8e94-4cf1-92ae-34089e78a82d@redhat.com>
Date: Mon, 3 Feb 2025 23:22:20 +0100
From: Paolo Bonzini <pbonzini@...hat.com>
To: Sean Christopherson <seanjc@...gle.com>,
 "Naveen N Rao (AMD)" <naveen@...nel.org>
Cc: kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
 Suravee Suthikulpanit <suravee.suthikulpanit@....com>,
 Vasant Hegde <vasant.hegde@....com>, Maxim Levitsky <mlevitsk@...hat.com>,
 Vitaly Kuznetsov <vkuznets@...hat.com>
Subject: Re: [PATCH 3/3] KVM: x86: Decouple APICv activation state from
 apicv_inhibit_reasons

On 2/3/25 19:45, Sean Christopherson wrote:
> Unless there's a very, very good reason to support a use case that generates
> ExtInts during boot, but _only_ during boot, and otherwise doesn't have any APICv
> ihibits, I'm leaning towards making SVM's IRQ window inhibit sticky, i.e. never
> clear it.

BIOS tends to use PIT, so that may be too much.  With respect to Naveen's report
of contention on apicv_update_lock, I would go with the sticky-bit idea but apply
it to APICV_INHIBIT_REASON_PIT_REINJ.

Plus, to avoid crazy ExtINT configurations, something like this:

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 3ec6197b1386..3e358d55b676 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1295,6 +1295,11 @@ enum kvm_apicv_inhibit {
  	 */
  	APICV_INHIBIT_REASON_LOGICAL_ID_ALIASED,
  
+	/*
+	 * AVIC is disabled because more than one vCPU has extint unmasked
+	 */
+	APICV_INHIBIT_REASON_EXTINT,
+
  	/*********************************************************/
  	/* INHIBITs that are relevant only to the Intel's APICv. */
  	/*********************************************************/
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 71544b0f6301..33a5f4ef42bd 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -377,6 +377,7 @@ void kvm_recalculate_apic_map(struct kvm *kvm)
  	struct kvm_apic_map *new, *old = NULL;
  	struct kvm_vcpu *vcpu;
  	unsigned long i;
+	int extint_cnt = 0;
  	u32 max_id = 255; /* enough space for any xAPIC ID */
  	bool xapic_id_mismatch;
  	int r;
@@ -432,6 +433,8 @@ void kvm_recalculate_apic_map(struct kvm *kvm)
  		if (!kvm_apic_present(vcpu))
  			continue;
  
+		extint_cnt += kvm_apic_accept_pic_intr(vcpu);
+
  		r = kvm_recalculate_phys_map(new, vcpu, &xapic_id_mismatch);
  		if (r) {
  			kvfree(new);
@@ -457,6 +460,11 @@ void kvm_recalculate_apic_map(struct kvm *kvm)
  	else
  		kvm_clear_apicv_inhibit(kvm, APICV_INHIBIT_REASON_PHYSICAL_ID_ALIASED);
  
+	if (extint_cnt > 1)
+		kvm_set_apicv_inhibit(kvm, APICV_INHIBIT_REASON_EXTINT);
+	else
+		kvm_clear_apicv_inhibit(kvm, APICV_INHIBIT_REASON_EXTINT);
+
  	if (!new || new->logical_mode == KVM_APIC_MODE_MAP_DISABLED)
  		kvm_set_apicv_inhibit(kvm, APICV_INHIBIT_REASON_LOGICAL_ID_ALIASED);
  	else
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index 57ff79bc02a4..ba2fc7dd8ca2 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -676,6 +676,7 @@ extern struct kvm_x86_nested_ops svm_nested_ops;
  	BIT(APICV_INHIBIT_REASON_HYPERV) |		\
  	BIT(APICV_INHIBIT_REASON_NESTED) |		\
  	BIT(APICV_INHIBIT_REASON_IRQWIN) |		\
+	BIT(APICV_INHIBIT_REASON_EXTINT) |		\
  	BIT(APICV_INHIBIT_REASON_PIT_REINJ) |		\
  	BIT(APICV_INHIBIT_REASON_BLOCKIRQ) |		\
  	BIT(APICV_INHIBIT_REASON_SEV)      |		\


I don't love adding another inhibit reason but, together, these two should
remove the contention on apicv_update_lock.  Another idea could be to move
IRQWIN to per-vCPU reason but Maxim tells me that it's not so easy.

Paolo