[PATCH v3 0/4] Allow AVIC's IPI virtualization to be optional

[PATCH v3 0/4] Allow AVIC's IPI virtualization to be optional

[Maxim Levitsky]

Hi!

This patch allows AVIC's ICR emulation to be optional and thus allows
to workaround AVIC's errata #1235 by disabling this portion of the feature.

This is v3 of my patch series 'AVIC bugfixes and workarounds' including
review feedback.

Best regards,
    Maxim Levitsky

Maxim Levitsky (4):
  KVM: Add per vCPU flag specifying that a vCPU is loaded
  x86: KVM: AVIC: stop using 'is_running' bit in avic_vcpu_put()
  x86: KVM: don't read physical ID table entry in avic_pi_update_irte()
  x86: KVM: SVM: allow optionally to disable AVIC's IPI virtualization

 arch/x86/kvm/svm/avic.c  | 72 ++++++++++++++++++++++++++--------------
 include/linux/kvm_host.h |  1 +
 virt/kvm/kvm_main.c      | 10 ++++++
 3 files changed, 59 insertions(+), 24 deletions(-)

-- 
2.26.3

[PATCH v3 1/4] KVM: Add per vCPU flag specifying that a vCPU is loaded

[Maxim Levitsky]

Add vcpu->loaded boolean flag specifying that a vCPU is loaded.

Such flag can be useful in a vendor code (e.g AVIC) to make
decisions based on it.

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 include/linux/kvm_host.h |  1 +
 virt/kvm/kvm_main.c      | 10 ++++++++++
 2 files changed, 11 insertions(+)

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index fb6c6109fdcad69..331432d86e44d51 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -379,6 +379,7 @@ struct kvm_vcpu {
 #endif
 	bool preempted;
 	bool ready;
+	bool loaded;
 	struct kvm_vcpu_arch arch;
 	struct kvm_vcpu_stat stat;
 	char stats_id[KVM_STATS_NAME_SIZE];
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 486800a7024b373..615f2a02b7cb97f 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -214,6 +214,10 @@ void vcpu_load(struct kvm_vcpu *vcpu)
 	__this_cpu_write(kvm_running_vcpu, vcpu);
 	preempt_notifier_register(&vcpu->preempt_notifier);
 	kvm_arch_vcpu_load(vcpu, cpu);
+
+	/* Ensure that vcpu->cpu is visible before vcpu->loaded is set to true */
+	smp_wmb();
+	WRITE_ONCE(vcpu->loaded, true);
 	put_cpu();
 }
 EXPORT_SYMBOL_GPL(vcpu_load);
@@ -221,6 +225,12 @@ EXPORT_SYMBOL_GPL(vcpu_load);
 void vcpu_put(struct kvm_vcpu *vcpu)
 {
 	preempt_disable();
+	WRITE_ONCE(vcpu->loaded, false);
+	/*
+	 * Ensure that vcpu->loaded is set and visible,
+	 * before KVM actually unloads the vCPU.
+	 */
+	smp_wmb();
 	kvm_arch_vcpu_put(vcpu);
 	preempt_notifier_unregister(&vcpu->preempt_notifier);
 	__this_cpu_write(kvm_running_vcpu, NULL);
-- 
2.26.3

[PATCH v3 2/4] x86: KVM: AVIC: stop using 'is_running' bit in avic_vcpu_put()

[Maxim Levitsky]

An optimization was added to avic_vcpu_load() in commit 782f64558de7
("KVM: SVM: Skip AVIC and IRTE updates when loading blocking vCPU")
to avoid re-enabling AVIC if the vCPU is about to block.

Such situation arises when a vCPU thread is preempted in between the call
to kvm_arch_vcpu_blocking() and before the matching call to
kvm_arch_vcpu_unblocking() which in case of AVIC disables/enables the AVIC
on this vCPU.

The same optimization was done in avic_vcpu_put() however the code was
based on physical id table's 'is_running' bit, building upon assumption
that if avic_vcpu_load() didn't set it, then kvm doesn't need to disable
avic (since it wasn't really enabled).

However, once AVIC's IPI virtualization is made optional, this bit
might be always false regardless if a vCPU is running or not.

To fix this, instead of checking this bit, check the same
'kvm_vcpu_is_blocking()' condition.

Also, as a bonus, re-enable the warning for already set 'is_running' bit,
if it was found set, during avic_vcpu_put() execution and the vCPU was not
blocking a condition which indicates a bug.

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 arch/x86/kvm/svm/avic.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index 2092db892d7d052..4c75ca15999fcd4 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -1075,16 +1075,10 @@ void avic_vcpu_put(struct kvm_vcpu *vcpu)
 	lockdep_assert_preemption_disabled();
 
 	/*
-	 * Note, reading the Physical ID entry outside of ir_list_lock is safe
-	 * as only the pCPU that has loaded (or is loading) the vCPU is allowed
-	 * to modify the entry, and preemption is disabled.  I.e. the vCPU
-	 * can't be scheduled out and thus avic_vcpu_{put,load}() can't run
-	 * recursively.
+	 * If the vcpu is blocking, there is no need to do anything.
+	 * See the comment in avic_vcpu_load().
 	 */
-	entry = READ_ONCE(*(svm->avic_physical_id_cache));
-
-	/* Nothing to do if IsRunning == '0' due to vCPU blocking. */
-	if (!(entry & AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK))
+	if (kvm_vcpu_is_blocking(vcpu))
 		return;
 
 	/*
@@ -1099,6 +1093,9 @@ void avic_vcpu_put(struct kvm_vcpu *vcpu)
 
 	avic_update_iommu_vcpu_affinity(vcpu, -1, 0);
 
+	entry = READ_ONCE(*(svm->avic_physical_id_cache));
+	WARN_ON_ONCE(!(entry & AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK));
+
 	entry &= ~AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK;
 	WRITE_ONCE(*(svm->avic_physical_id_cache), entry);
 
-- 
2.26.3

[PATCH v3 3/4] x86: KVM: don't read physical ID table entry in avic_pi_update_irte()

[Maxim Levitsky]

Change AVIC's code to use vcpu->loaded and vcpu->cpu instead of reading
back the cpu and 'is_running' bit from the avic's physical id entry.

Once AVIC's IPI virtualization is made optional, the 'is_running'
bit might always be false regardless if a vCPU is running or not.

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 arch/x86/kvm/svm/avic.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index 4c75ca15999fcd4..bdab28005ad3405 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -791,7 +791,6 @@ static int svm_ir_list_add(struct vcpu_svm *svm, struct amd_iommu_pi_data *pi)
 	int ret = 0;
 	unsigned long flags;
 	struct amd_svm_iommu_ir *ir;
-	u64 entry;
 
 	/**
 	 * In some cases, the existing irte is updated and re-set,
@@ -832,10 +831,11 @@ static int svm_ir_list_add(struct vcpu_svm *svm, struct amd_iommu_pi_data *pi)
 	 * will update the pCPU info when the vCPU awkened and/or scheduled in.
 	 * See also avic_vcpu_load().
 	 */
-	entry = READ_ONCE(*(svm->avic_physical_id_cache));
-	if (entry & AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK)
-		amd_iommu_update_ga(entry & AVIC_PHYSICAL_ID_ENTRY_HOST_PHYSICAL_ID_MASK,
-				    true, pi->ir_data);
+	if (READ_ONCE(svm->vcpu.loaded)) {
+		/* Ensure that the vcpu->loaded is read before the vcpu->cpu */
+		smp_rmb();
+		amd_iommu_update_ga(READ_ONCE(svm->vcpu.cpu), true, pi->ir_data);
+	}
 
 	list_add(&ir->node, &svm->ir_list);
 	spin_unlock_irqrestore(&svm->ir_list_lock, flags);
-- 
2.26.3

[PATCH v3 4/4] x86: KVM: SVM: allow optionally to disable AVIC's IPI virtualization

[Maxim Levitsky]

On Zen2 (and likely on Zen1 as well), AVIC doesn't reliably detect a change
in the 'is_running' bit during ICR write emulation and might skip a
VM exit, if that bit was recently cleared.

The absence of the VM exit, leads to the KVM not waking up / triggering
nested vm exit on the target(s) of the IPI, which can, in some cases,
lead to unbounded delays in the guest execution.

As I recently discovered, a reasonable workaround exists: make the KVM
never set the is_running bit, which in essence disables the
IPI virtualization portion of AVIC making it equal to APICv without IPI
virtualization.

This workaround ensures that (*) all ICR writes always cause a VM exit
and therefore correctly emulated, in expense of never enjoying VM exit-less
ICR write emulation.

To let the user control the workaround, a new kvm_amd module parameter was
added: 'enable_ipiv', using the same name as IPI virtualization of VMX.

However unlike VMX, this parameter is tri-state: 0, 1, -1.
-1 is the default value which instructs KVM to choose the default based
on the CPU model.

(*) More correctly all ICR writes except when the 'Self' shorthand is used:

In this case AVIC skips reading physid table and just sets bits in IRR
of local APIC. Thankfully in this case, the errata is not possible,
therefore an extra workaround is not needed.

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 arch/x86/kvm/svm/avic.c | 51 +++++++++++++++++++++++++++++++----------
 1 file changed, 39 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index bdab28005ad3405..b3ec693083cc883 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -62,6 +62,9 @@ static_assert(__AVIC_GATAG(AVIC_VM_ID_MASK, AVIC_VCPU_ID_MASK) == -1u);
 static bool force_avic;
 module_param_unsafe(force_avic, bool, 0444);
 
+static int enable_ipiv = -1;
+module_param(enable_ipiv, int, 0444);
+
 /* Note:
  * This hash table is used to map VM_ID to a struct kvm_svm,
  * when handling AMD IOMMU GALOG notification to schedule in
@@ -1024,7 +1027,6 @@ avic_update_iommu_vcpu_affinity(struct kvm_vcpu *vcpu, int cpu, bool r)
 
 void avic_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
 {
-	u64 entry;
 	int h_physical_id = kvm_cpu_get_apicid(cpu);
 	struct vcpu_svm *svm = to_svm(vcpu);
 	unsigned long flags;
@@ -1053,14 +1055,22 @@ void avic_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
 	 */
 	spin_lock_irqsave(&svm->ir_list_lock, flags);
 
-	entry = READ_ONCE(*(svm->avic_physical_id_cache));
-	WARN_ON_ONCE(entry & AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK);
+	/*
+	 * Do not update the actual physical id table entry, if the IPI
+	 * virtualization portion of AVIC is not enabled.
+	 * In this case all ICR writes except Self IPIs will be intercepted.
+	 */
+
+	if (enable_ipiv) {
+		u64 entry = READ_ONCE(*svm->avic_physical_id_cache);
 
-	entry &= ~AVIC_PHYSICAL_ID_ENTRY_HOST_PHYSICAL_ID_MASK;
-	entry |= (h_physical_id & AVIC_PHYSICAL_ID_ENTRY_HOST_PHYSICAL_ID_MASK);
-	entry |= AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK;
+		WARN_ON_ONCE(entry & AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK);
+		entry &= ~AVIC_PHYSICAL_ID_ENTRY_HOST_PHYSICAL_ID_MASK;
+		entry |= (h_physical_id & AVIC_PHYSICAL_ID_ENTRY_HOST_PHYSICAL_ID_MASK);
+		entry |= AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK;
+		WRITE_ONCE(*(svm->avic_physical_id_cache), entry);
+	}
 
-	WRITE_ONCE(*(svm->avic_physical_id_cache), entry);
 	avic_update_iommu_vcpu_affinity(vcpu, h_physical_id, true);
 
 	spin_unlock_irqrestore(&svm->ir_list_lock, flags);
@@ -1068,7 +1078,6 @@ void avic_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
 
 void avic_vcpu_put(struct kvm_vcpu *vcpu)
 {
-	u64 entry;
 	struct vcpu_svm *svm = to_svm(vcpu);
 	unsigned long flags;
 
@@ -1093,11 +1102,17 @@ void avic_vcpu_put(struct kvm_vcpu *vcpu)
 
 	avic_update_iommu_vcpu_affinity(vcpu, -1, 0);
 
-	entry = READ_ONCE(*(svm->avic_physical_id_cache));
-	WARN_ON_ONCE(!(entry & AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK));
+	/*
+	 * Do not update the actual physical id table entry if the IPI
+	 * virtualization is disabled. See explanation in avic_vcpu_load().
+	 */
+	if (enable_ipiv) {
+		u64 entry = READ_ONCE(*svm->avic_physical_id_cache);
 
-	entry &= ~AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK;
-	WRITE_ONCE(*(svm->avic_physical_id_cache), entry);
+		WARN_ON_ONCE(!(entry & AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK));
+		entry &= ~AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK;
+		WRITE_ONCE(*(svm->avic_physical_id_cache), entry);
+	}
 
 	spin_unlock_irqrestore(&svm->ir_list_lock, flags);
 
@@ -1211,5 +1226,17 @@ bool avic_hardware_setup(void)
 
 	amd_iommu_register_ga_log_notifier(&avic_ga_log_notifier);
 
+	if (enable_ipiv == -1) {
+		enable_ipiv = 1;
+		/* Assume that Zen1 and Zen2 have errata #1235 */
+		if (boot_cpu_data.x86 == 0x17) {
+			pr_info("AVIC's IPI virtualization disabled due to errata #1235\n");
+			enable_ipiv = 0;
+		}
+	}
+
+	if (enable_ipiv)
+		pr_info("AVIC's IPI virtualization enabled\n");
+
 	return true;
 }
-- 
2.26.3

Re: [PATCH v3 0/4] Allow AVIC's IPI virtualization to be optional

[Sean Christopherson]

On Mon, Oct 02, 2023, Maxim Levitsky wrote:
> Hi!
> 
> This patch allows AVIC's ICR emulation to be optional and thus allows
> to workaround AVIC's errata #1235 by disabling this portion of the feature.
> 
> This is v3 of my patch series 'AVIC bugfixes and workarounds' including
> review feedback.

Please respond to my idea[*] instead of sending more patches.  I'm not opposed to
a different approach, but we need to have an actual discussion around the pros and
cons, and hopefully come to an agreement.  This cover letter doesn't even acknowledge
that there is an alternative proposal, let alone justify why the vcpu->loaded
approach was taken.

[*] https://lore.kernel.org/all/ZRYxPNeq1rnp-M0f@google.com

Re: [PATCH v3 0/4] Allow AVIC's IPI virtualization to be optional

[Maxim Levitsky]

У пн, 2023-10-02 у 12:21 -0700, Sean Christopherson пише:
> On Mon, Oct 02, 2023, Maxim Levitsky wrote:
> > Hi!
> > 
> > This patch allows AVIC's ICR emulation to be optional and thus allows
> > to workaround AVIC's errata #1235 by disabling this portion of the feature.
> > 
> > This is v3 of my patch series 'AVIC bugfixes and workarounds' including
> > review feedback.
> 
> Please respond to my idea[*] instead of sending more patches. 

Hi,

For the v2 of the patch I was already on the fence if to do it this way or to refactor
the code, and back when I posted it, I decided still to avoid the refactoring.

However, your idea of rewriting this patch, while it does change less lines of code,
is even less obvious and consequently required you to write even longer comment to 
justify it which is not a good sign.

In particular I don't want someone to find out later, and in the hard way that sometimes
real physid table is accessed, and sometimes a fake copy of it is.

So I decided to fix the root cause by not reading the physid table back,
which made the code cleaner, and even with the workaround the code 
IMHO is still simpler than it was before.

About the added 'vcpu->loaded' variable, I added it also because it is something that is 
long overdue to be added, I remember that in IPIv code there was also a need for this, 
and probalby more places in KVM can be refactored to take advantage of it,
instead of various hacks.

I did adopt your idea of using 'enable_ipiv', although I am still not 100% sure that this
is more readable than 'avic_zen2_workaround'.

Best regards,
	Maxim Levitsky

>  I'm not opposed to
> a different approach, but we need to have an actual discussion around the pros and
> cons, and hopefully come to an agreement.  This cover letter doesn't even acknowledge
> that there is an alternative proposal, let alone justify why the vcpu->loaded
> approach was taken.
> 
> [*] https://lore.kernel.org/all/ZRYxPNeq1rnp-M0f@google.com
> 

Re: [PATCH v3 0/4] Allow AVIC's IPI virtualization to be optional

[Maxim Levitsky]

On Wed, 2023-10-04 at 16:14 +0300, Maxim Levitsky wrote:
> У пн, 2023-10-02 у 12:21 -0700, Sean Christopherson пише:
> > On Mon, Oct 02, 2023, Maxim Levitsky wrote:
> > > Hi!
> > > 
> > > This patch allows AVIC's ICR emulation to be optional and thus allows
> > > to workaround AVIC's errata #1235 by disabling this portion of the feature.
> > > 
> > > This is v3 of my patch series 'AVIC bugfixes and workarounds' including
> > > review feedback.
> > 
> > Please respond to my idea[*] instead of sending more patches. 
> 
> Hi,
> 
> For the v2 of the patch I was already on the fence if to do it this way or to refactor
> the code, and back when I posted it, I decided still to avoid the refactoring.
> 
> However, your idea of rewriting this patch, while it does change less lines of code,
> is even less obvious and consequently required you to write even longer comment to 
> justify it which is not a good sign.
> 
> In particular I don't want someone to find out later, and in the hard way that sometimes
> real physid table is accessed, and sometimes a fake copy of it is.
> 
> So I decided to fix the root cause by not reading the physid table back,
> which made the code cleaner, and even with the workaround the code 
> IMHO is still simpler than it was before.
> 
> About the added 'vcpu->loaded' variable, I added it also because it is something that is 
> long overdue to be added, I remember that in IPIv code there was also a need for this, 
> and probalby more places in KVM can be refactored to take advantage of it,
> instead of various hacks.
> 
> I did adopt your idea of using 'enable_ipiv', although I am still not 100% sure that this
> is more readable than 'avic_zen2_workaround'.

Hi!

Sean, can you take another look at this patch series?

Thanks in advance,
	Maxim Levitsky

> 
> Best regards,
> 	Maxim Levitsky
> 
> >  I'm not opposed to
> > a different approach, but we need to have an actual discussion around the pros and
> > cons, and hopefully come to an agreement.  This cover letter doesn't even acknowledge
> > that there is an alternative proposal, let alone justify why the vcpu->loaded
> > approach was taken.
> > 
> > [*] https://lore.kernel.org/all/ZRYxPNeq1rnp-M0f@google.com
> > 
> 
> 

Re: [PATCH v3 0/4] Allow AVIC's IPI virtualization to be optional

[Sean Christopherson]

On Tue, Sep 10, 2024, Maxim Levitsky wrote:
> On Wed, 2023-10-04 at 16:14 +0300, Maxim Levitsky wrote:
> > У пн, 2023-10-02 у 12:21 -0700, Sean Christopherson пише:
> > > On Mon, Oct 02, 2023, Maxim Levitsky wrote:
> > > > Hi!
> > > > 
> > > > This patch allows AVIC's ICR emulation to be optional and thus allows
> > > > to workaround AVIC's errata #1235 by disabling this portion of the feature.
> > > > 
> > > > This is v3 of my patch series 'AVIC bugfixes and workarounds' including
> > > > review feedback.
> > > 
> > > Please respond to my idea[*] instead of sending more patches. 
> > 
> > Hi,
> > 
> > For the v2 of the patch I was already on the fence if to do it this way or to refactor
> > the code, and back when I posted it, I decided still to avoid the refactoring.
> > 
> > However, your idea of rewriting this patch, while it does change less lines of code,
> > is even less obvious and consequently required you to write even longer comment to 
> > justify it which is not a good sign.
> > 
> > In particular I don't want someone to find out later, and in the hard way that sometimes
> > real physid table is accessed, and sometimes a fake copy of it is.
> > 
> > So I decided to fix the root cause by not reading the physid table back,
> > which made the code cleaner, and even with the workaround the code 
> > IMHO is still simpler than it was before.
> > 
> > About the added 'vcpu->loaded' variable, I added it also because it is something that is 
> > long overdue to be added, I remember that in IPIv code there was also a need for this, 
> > and probalby more places in KVM can be refactored to take advantage of it,
> > instead of various hacks.
> > 
> > I did adopt your idea of using 'enable_ipiv', although I am still not 100% sure that this
> > is more readable than 'avic_zen2_workaround'.
> 
> Hi!
> 
> Sean, can you take another look at this patch series?

Ya, it might take a week or two, but it's on my todo list.

Re: [PATCH v3 0/4] Allow AVIC's IPI virtualization to be optional

[Maxim Levitsky]

On Mon, 2024-09-23 at 02:29 -0700, Sean Christopherson wrote:
> On Tue, Sep 10, 2024, Maxim Levitsky wrote:
> > On Wed, 2023-10-04 at 16:14 +0300, Maxim Levitsky wrote:
> > > У пн, 2023-10-02 у 12:21 -0700, Sean Christopherson пише:
> > > > On Mon, Oct 02, 2023, Maxim Levitsky wrote:
> > > > > Hi!
> > > > > 
> > > > > This patch allows AVIC's ICR emulation to be optional and thus allows
> > > > > to workaround AVIC's errata #1235 by disabling this portion of the feature.
> > > > > 
> > > > > This is v3 of my patch series 'AVIC bugfixes and workarounds' including
> > > > > review feedback.
> > > > 
> > > > Please respond to my idea[*] instead of sending more patches. 
> > > 
> > > Hi,
> > > 
> > > For the v2 of the patch I was already on the fence if to do it this way or to refactor
> > > the code, and back when I posted it, I decided still to avoid the refactoring.
> > > 
> > > However, your idea of rewriting this patch, while it does change less lines of code,
> > > is even less obvious and consequently required you to write even longer comment to 
> > > justify it which is not a good sign.
> > > 
> > > In particular I don't want someone to find out later, and in the hard way that sometimes
> > > real physid table is accessed, and sometimes a fake copy of it is.
> > > 
> > > So I decided to fix the root cause by not reading the physid table back,
> > > which made the code cleaner, and even with the workaround the code 
> > > IMHO is still simpler than it was before.
> > > 
> > > About the added 'vcpu->loaded' variable, I added it also because it is something that is 
> > > long overdue to be added, I remember that in IPIv code there was also a need for this, 
> > > and probalby more places in KVM can be refactored to take advantage of it,
> > > instead of various hacks.
> > > 
> > > I did adopt your idea of using 'enable_ipiv', although I am still not 100% sure that this
> > > is more readable than 'avic_zen2_workaround'.
> > 
> > Hi!
> > 
> > Sean, can you take another look at this patch series?
> 
> Ya, it might take a week or two, but it's on my todo list.
> 
Thanks in advance!
Best regards,
       Maxim Levitsky

Re: [PATCH v3 0/4] Allow AVIC's IPI virtualization to be optional

[Sean Christopherson]

Finally got back to this...

On Wed, Oct 04, 2023, Maxim Levitsky wrote:
> У пн, 2023-10-02 у 12:21 -0700, Sean Christopherson пише:
> > On Mon, Oct 02, 2023, Maxim Levitsky wrote:
> > > Hi!
> > > 
> > > This patch allows AVIC's ICR emulation to be optional and thus allows
> > > to workaround AVIC's errata #1235 by disabling this portion of the feature.
> > > 
> > > This is v3 of my patch series 'AVIC bugfixes and workarounds' including
> > > review feedback.
> > 
> > Please respond to my idea[*] instead of sending more patches. 
> 
> Hi,
> 
> For the v2 of the patch I was already on the fence if to do it this way or to
> refactor the code, and back when I posted it, I decided still to avoid the
> refactoring.
> 
> However, your idea of rewriting this patch, while it does change less lines
> of code, is even less obvious and consequently required you to write even
> longer comment to justify it which is not a good sign.

Agreed.  And FWIW, if we keep the local "entry" variables, I actually like your
version much better.

> In particular I don't want someone to find out later, and in the hard way
> that sometimes real physid table is accessed, and sometimes a fake copy of it
> is.

Note, this quirk is present in your v2 as well.  I bring that up only because I
prefer your v2, and rebased it (with massaging) on top of the next version of the
max vCPUs.  This is what I have currently:

-       WRITE_ONCE(kvm_svm->avic_physical_id_table[vcpu->vcpu_id], entry);
+       svm->avic_physical_id_entry = entry;
+
+       /*
+        * If IPI virtualization is disable, don't update the actual Physical
+        * ID table, so that the CPU never sees IsRunning=1.
+        */
+       if (enable_ipiv)
+               WRITE_ONCE(kvm_svm->avic_physical_id_table[vcpu->vcpu_id], entry);
+

I have no objection to writing the "real" table, but with IsRunning=0.  That's
easy enough to do, e.g. tweak the above to:

	/*
	 * If IPI virtualization is disabled, clear IsRunning when updating the
	 * actual Physical ID table, so that the CPU never sees IsRunning=1.
	 */
	if (!enable_ipiv)
		entry &= ~AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK;

	WRITE_ONCE(kvm_svm->avic_physical_id_table[vcpu->vcpu_id], entry);

> So I decided to fix the root cause by not reading the physid table back,
> which made the code cleaner, and even with the workaround the code IMHO is
> still simpler than it was before.
> 
> About the added 'vcpu->loaded' variable, I added it also because it is
> something that is long overdue to be added, I remember that in IPIv code
> there was also a need for this, and probalby more places in KVM can be
> refactored to take advantage of it, instead of various hacks.

I don't view using the information from the Physical ID table as a hack.  It very
explicitly uses the ir_list_lock to ensure that the pCPU that's programmed into
the IRTE is the pCPU on which the vCPU is loaded, and provides rather strict
ordering between task migration and device assignment.  It's not a super hot path,
so I don't think lockless programming is justified.

I also think we should keep IsRunning=1 when the vCPU is unloaded.  That approach
won't run afoul of your concern with signaling the wrong pCPU, because KVM can
still keep the ID up-to-date, e.g. if the task is migrated when a pCPU is being
offlined.

The motiviation for keeping IsRunning=1 is to avoid unnecessary VM-Exits and GA
log IRQs.  E.g. if a vCPU exits to userspace, there's zero reason to force IPI
senders to exit, because KVM can't/won't notify userspace, and the pending virtual
interrupt will be processed on the next VMRUN.

> I did adopt your idea of using 'enable_ipiv', although I am still not 100%
> sure that this is more readable than 'avic_zen2_workaround'.

The problem with "avic_zen2_workaround" is that it becomes nonsensical if a user
wants to disable IPI virtualization on a CPU that isn't affected by the erratum.
And I also think KVM should disallow enabling IPI virtualization if the CPU is
affected by the erratum; even with HLT-exiting disabled on every VM, kvm_vcpu_block()
is still reachable, so I don't think it's at all reasonable to expect a user to
be able to know when it's safe to ignore the erratum.

Re: [PATCH v3 0/4] Allow AVIC's IPI virtualization to be optional

[Sean Christopherson]

On Mon, Oct 21, 2024, Sean Christopherson wrote:
> On Wed, Oct 04, 2023, Maxim Levitsky wrote:
> > About the added 'vcpu->loaded' variable, I added it also because it is
> > something that is long overdue to be added, I remember that in IPIv code
> > there was also a need for this, and probalby more places in KVM can be
> > refactored to take advantage of it, instead of various hacks.
> 
> I don't view using the information from the Physical ID table as a hack.  It very
> explicitly uses the ir_list_lock to ensure that the pCPU that's programmed into
> the IRTE is the pCPU on which the vCPU is loaded, and provides rather strict
> ordering between task migration and device assignment.  It's not a super hot path,
> so I don't think lockless programming is justified.
> 
> I also think we should keep IsRunning=1 when the vCPU is unloaded.  That approach
> won't run afoul of your concern with signaling the wrong pCPU, because KVM can
> still keep the ID up-to-date, e.g. if the task is migrated when a pCPU is being
> offlined.
> 
> The motiviation for keeping IsRunning=1 is to avoid unnecessary VM-Exits and GA
> log IRQs.  E.g. if a vCPU exits to userspace, there's zero reason to force IPI
> senders to exit, because KVM can't/won't notify userspace, and the pending virtual
> interrupt will be processed on the next VMRUN.

My only hesitation to keeping IsRunning=1 is that there could, in theory, be a
noisy neighbor problem.  E.g. if there is meaningful overhead when the CPU responds
to the doorbell.  Hrm, and if another vCPU is scheduled in on the same pCPU, that
vCPU could end up processing a virtual interrupt in response to a doorbell intended
for a different vCPU.

The counter-argument to both concerns is that APICv Posted Interrupts have had a
_worse_ version of that behavior for years, and no one has complained.  KVM sets
PID.SN only when a vCPU is _preempted_, and so devices (and now virtual IPIs) will
send notification IRQs to pCPUs that aren't actively running the vCPU, or are
running a different vCPU.

The counter-counter-argument is that (a) IPI virtualization is a recent addition,
and device posted interrupts are unlikely to be used in a CPU oversubscribed setup,
and (b) Posted Interrupts are effectively rate-limited to a single "spurious"
notification per vCPU, as notification IRQs are sent if and only if PID.ON=0.

That said, while I'm somewhat less confident that keeping IsRunning=1 is desirable
for all use cases than I was yesterday, I still think we should avoid tightly
coupling it to whether or not the vCPU is loaded, because there are undoubtedly
setups where it _is_ desirable, e.g. if vCPUs are pinned 1:1 to pCPUs.

Re: [PATCH v3 0/4] Allow AVIC's IPI virtualization to be optional

[Maxim Levitsky]

On Tue, 2024-10-22 at 12:00 -0700, Sean Christopherson wrote:
> On Mon, Oct 21, 2024, Sean Christopherson wrote:
> > On Wed, Oct 04, 2023, Maxim Levitsky wrote:
> > > About the added 'vcpu->loaded' variable, I added it also because it is
> > > something that is long overdue to be added, I remember that in IPIv code
> > > there was also a need for this, and probalby more places in KVM can be
> > > refactored to take advantage of it, instead of various hacks.
> > 
> > I don't view using the information from the Physical ID table as a hack.  It very
> > explicitly uses the ir_list_lock to ensure that the pCPU that's programmed into
> > the IRTE is the pCPU on which the vCPU is loaded, and provides rather strict
> > ordering between task migration and device assignment.  It's not a super hot path,
> > so I don't think lockless programming is justified.

If you strongly prefer this I won't argue. KVM does read back its SPTE entries,
which is also something I can't say that I like that much.

> > 
> > I also think we should keep IsRunning=1 when the vCPU is unloaded.  That approach
> > won't run afoul of your concern with signaling the wrong pCPU, because KVM can
> > still keep the ID up-to-date, e.g. if the task is migrated when a pCPU is being
> > offlined.
> > 
> > The motiviation for keeping IsRunning=1 is to avoid unnecessary VM-Exits and GA
> > log IRQs.  E.g. if a vCPU exits to userspace, there's zero reason to force IPI
> > senders to exit, because KVM can't/won't notify userspace, and the pending virtual
> > interrupt will be processed on the next VMRUN.
> 
> My only hesitation to keeping IsRunning=1 is that there could, in theory, be a
> noisy neighbor problem.  E.g. if there is meaningful overhead when the CPU responds
> to the doorbell. 

I once measured this by bombarding a regular CPU, which is not running any guests,
with AVIC doorbells. It was like 60% reduction of its performance if I remember correctly.

So physical id table entries of a VM can't point to a CPU which doesn't run the VM's vCPU thread, because
only in this case this doesn't pose a DOS risk.

Same with IOMMU (malicious guest can in theory make an assigned device generate an interrupt
storm, and then this storm can get redirected to a doorbell of a CPU which doesn't belong to a VM).


Best regards,
	Maxim Levitsky



>  Hrm, and if another vCPU is scheduled in on the same pCPU, that
> vCPU could end up processing a virtual interrupt in response to a doorbell intended
> for a different vCPU.
> 
> The counter-argument to both concerns is that APICv Posted Interrupts have had a
> _worse_ version of that behavior for years, and no one has complained.  KVM sets
> PID.SN only when a vCPU is _preempted_, and so devices (and now virtual IPIs) will
> send notification IRQs to pCPUs that aren't actively running the vCPU, or are
> running a different vCPU.
> 
> The counter-counter-argument is that (a) IPI virtualization is a recent addition,
> and device posted interrupts are unlikely to be used in a CPU oversubscribed setup,
> and (b) Posted Interrupts are effectively rate-limited to a single "spurious"
> notification per vCPU, as notification IRQs are sent if and only if PID.ON=0.
> 
> That said, while I'm somewhat less confident that keeping IsRunning=1 is desirable
> for all use cases than I was yesterday, I still think we should avoid tightly
> coupling it to whether or not the vCPU is loaded, because there are undoubtedly
> setups where it _is_ desirable, e.g. if vCPUs are pinned 1:1 to pCPUs.
> 

Re: [PATCH v3 0/4] Allow AVIC's IPI virtualization to be optional

[Sean Christopherson]

On Thu, Nov 21, 2024, Maxim Levitsky wrote:
> On Tue, 2024-10-22 at 12:00 -0700, Sean Christopherson wrote:
> > On Mon, Oct 21, 2024, Sean Christopherson wrote:
> > > On Wed, Oct 04, 2023, Maxim Levitsky wrote:
> > > > About the added 'vcpu->loaded' variable, I added it also because it is
> > > > something that is long overdue to be added, I remember that in IPIv code
> > > > there was also a need for this, and probalby more places in KVM can be
> > > > refactored to take advantage of it, instead of various hacks.
> > > 
> > > I don't view using the information from the Physical ID table as a hack.  It very
> > > explicitly uses the ir_list_lock to ensure that the pCPU that's programmed into
> > > the IRTE is the pCPU on which the vCPU is loaded, and provides rather strict
> > > ordering between task migration and device assignment.  It's not a super hot path,
> > > so I don't think lockless programming is justified.
> 
> If you strongly prefer this I won't argue. KVM does read back its SPTE entries,
> which is also something I can't say that I like that much.

Heh, ignoring the conundrum with SPTEs being writable by hardware for A/D assists,
not reading SPTEs would add an almost absurd amount of complexity due to the need
to manage mappings in a separate data structure.  E.g. see TDX's S-EPT implementation
for how messy things get.

> > > I also think we should keep IsRunning=1 when the vCPU is unloaded.  That approach
> > > won't run afoul of your concern with signaling the wrong pCPU, because KVM can
> > > still keep the ID up-to-date, e.g. if the task is migrated when a pCPU is being
> > > offlined.
> > > 
> > > The motiviation for keeping IsRunning=1 is to avoid unnecessary VM-Exits and GA
> > > log IRQs.  E.g. if a vCPU exits to userspace, there's zero reason to force IPI
> > > senders to exit, because KVM can't/won't notify userspace, and the pending virtual
> > > interrupt will be processed on the next VMRUN.
> > 
> > My only hesitation to keeping IsRunning=1 is that there could, in theory, be a
> > noisy neighbor problem.  E.g. if there is meaningful overhead when the CPU responds
> > to the doorbell. 
> 
> I once measured this by bombarding a regular CPU, which is not running any
> guests, with AVIC doorbells. It was like 60% reduction of its performance if
> I remember correctly.

Ah, right, I keep forgetting the Intel's posted interrupts limits the spam to a
single IRQ thanks to the PID.ON behavior, which is why it's ok-ish to keep posted
interrupts active when a vCPU is put.

> So physical id table entries of a VM can't point to a CPU which doesn't run
> the VM's vCPU thread, because only in this case this doesn't pose a DOS risk.
> 
> Same with IOMMU (malicious guest can in theory make an assigned device
> generate an interrupt storm, and then this storm can get redirected to a
> doorbell of a CPU which doesn't belong to a VM).