[PATCH] KVM: x86: Allow APICv APIC ID inhibit to be cleared on legacy kernels

[PATCH] KVM: x86: Allow APICv APIC ID inhibit to be cleared on legacy kernels

[Greg Edwards]

Legacy kernels prior to commit 4399c03c6780 ("x86/apic: Remove
verify_local_APIC()") write the xAPIC ID of the boot CPU twice to verify
a functioning local APIC.  This results in APIC acceleration inhibited
on these kernels for reason APICV_INHIBIT_REASON_APIC_ID_MODIFIED.

Allow the APICV_INHIBIT_REASON_APIC_ID_MODIFIED inhibit reason to be
cleared if/when the xAPIC ID is set back to the expected vcpu_id value.
This occurs on the second xAPIC ID write in verify_local_APIC().

Fixes: 3743c2f02517 ("KVM: x86: inhibit APICv/AVIC on changes to APIC ID or APIC base")
Signed-off-by: Greg Edwards <gedwards@ddn.com>
---
 arch/x86/kvm/lapic.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index d7639d126e6c..4064d0af094d 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -2075,8 +2075,13 @@ static void kvm_lapic_xapic_id_updated(struct kvm_lapic *apic)
 	if (KVM_BUG_ON(apic_x2apic_mode(apic), kvm))
 		return;
 
-	if (kvm_xapic_id(apic) == apic->vcpu->vcpu_id)
+	if (kvm_xapic_id(apic) == apic->vcpu->vcpu_id) {
+		/* Legacy kernels prior to 4399c03c6780 write APIC ID twice. */
+		if (!kvm_apicv_activated(kvm))
+			kvm_clear_apicv_inhibit(kvm,
+					APICV_INHIBIT_REASON_APIC_ID_MODIFIED);
 		return;
+	}
 
 	kvm_set_apicv_inhibit(apic->vcpu->kvm, APICV_INHIBIT_REASON_APIC_ID_MODIFIED);
 }
-- 
2.38.1

Re: [PATCH] KVM: x86: Allow APICv APIC ID inhibit to be cleared on legacy kernels

[Sean Christopherson]

On Mon, Nov 14, 2022, Greg Edwards wrote:
> Legacy kernels prior to commit 4399c03c6780 ("x86/apic: Remove
> verify_local_APIC()") write the xAPIC ID of the boot CPU twice to verify
> a functioning local APIC.  This results in APIC acceleration inhibited
> on these kernels for reason APICV_INHIBIT_REASON_APIC_ID_MODIFIED.
> 
> Allow the APICV_INHIBIT_REASON_APIC_ID_MODIFIED inhibit reason to be
> cleared if/when the xAPIC ID is set back to the expected vcpu_id value.
> This occurs on the second xAPIC ID write in verify_local_APIC().
> 
> Fixes: 3743c2f02517 ("KVM: x86: inhibit APICv/AVIC on changes to APIC ID or APIC base")
> Signed-off-by: Greg Edwards <gedwards@ddn.com>
> ---
>  arch/x86/kvm/lapic.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index d7639d126e6c..4064d0af094d 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -2075,8 +2075,13 @@ static void kvm_lapic_xapic_id_updated(struct kvm_lapic *apic)
>  	if (KVM_BUG_ON(apic_x2apic_mode(apic), kvm))
>  		return;
>  
> -	if (kvm_xapic_id(apic) == apic->vcpu->vcpu_id)
> +	if (kvm_xapic_id(apic) == apic->vcpu->vcpu_id) {
> +		/* Legacy kernels prior to 4399c03c6780 write APIC ID twice. */
> +		if (!kvm_apicv_activated(kvm))
> +			kvm_clear_apicv_inhibit(kvm,
> +					APICV_INHIBIT_REASON_APIC_ID_MODIFIED);

This sadly doesn't work because the inhibit is per-VM, i.e. will do the wrong
thing if there are still vCPU's with different APIC IDs.

Does skipping the check if the APIC is disabled help[*]?  At a glance, I can't
tell if the APIC is enabled/disabled at that point in time.  It's not a true fix,
but it's a lot easier to backport if it remedies the issue.

For a proper fix, this entire path should be moved to kvm_recalculate_apic_map()
so that can can safely toggle the inhibit, e.g. the recalc helper already deals
with multiple vCPUs changing their APIC state in parallel.  I don't think the fix
will be too difficult to craft such that it's backport friendly, but it would need
to be slotted into the series containing the aforementioned fix.

[*] https://lore.kernel.org/all/20221001005915.2041642-6-seanjc@google.com

---
 arch/x86/kvm/lapic.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 5de1c7aa1ce9..67260f7ce43a 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -2072,6 +2072,9 @@ static void kvm_lapic_xapic_id_updated(struct kvm_lapic *apic)
 {
 	struct kvm *kvm = apic->vcpu->kvm;
 
+	if (!kvm_apic_hw_enabled(apic))
+		return;
+
 	if (KVM_BUG_ON(apic_x2apic_mode(apic), kvm))
 		return;
 

Re: [PATCH] KVM: x86: Allow APICv APIC ID inhibit to be cleared on legacy kernels

[Greg Edwards]

On Mon, Nov 14, 2022 at 09:30:18PM +0000, Sean Christopherson wrote:
> On Mon, Nov 14, 2022, Greg Edwards wrote:
>> Legacy kernels prior to commit 4399c03c6780 ("x86/apic: Remove
>> verify_local_APIC()") write the xAPIC ID of the boot CPU twice to verify
>> a functioning local APIC.  This results in APIC acceleration inhibited
>> on these kernels for reason APICV_INHIBIT_REASON_APIC_ID_MODIFIED.
>>
>> Allow the APICV_INHIBIT_REASON_APIC_ID_MODIFIED inhibit reason to be
>> cleared if/when the xAPIC ID is set back to the expected vcpu_id value.
>> This occurs on the second xAPIC ID write in verify_local_APIC().
>>
>> Fixes: 3743c2f02517 ("KVM: x86: inhibit APICv/AVIC on changes to APIC ID or APIC base")
>> Signed-off-by: Greg Edwards <gedwards@ddn.com>
>> ---
>>  arch/x86/kvm/lapic.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>> index d7639d126e6c..4064d0af094d 100644
>> --- a/arch/x86/kvm/lapic.c
>> +++ b/arch/x86/kvm/lapic.c
>> @@ -2075,8 +2075,13 @@ static void kvm_lapic_xapic_id_updated(struct kvm_lapic *apic)
>>  	if (KVM_BUG_ON(apic_x2apic_mode(apic), kvm))
>>  		return;
>>
>> -	if (kvm_xapic_id(apic) == apic->vcpu->vcpu_id)
>> +	if (kvm_xapic_id(apic) == apic->vcpu->vcpu_id) {
>> +		/* Legacy kernels prior to 4399c03c6780 write APIC ID twice. */
>> +		if (!kvm_apicv_activated(kvm))
>> +			kvm_clear_apicv_inhibit(kvm,
>> +					APICV_INHIBIT_REASON_APIC_ID_MODIFIED);
>
> This sadly doesn't work because the inhibit is per-VM, i.e. will do the wrong
> thing if there are still vCPU's with different APIC IDs.

That is true.  Thanks for pointing that out.

> Does skipping the check if the APIC is disabled help[*]?  At a glance, I can't
> tell if the APIC is enabled/disabled at that point in time.  It's not a true fix,
> but it's a lot easier to backport if it remedies the issue.

It does not.  I did find your patch series when I started investigating
this, but the behavior was the same on that series.

> For a proper fix, this entire path should be moved to kvm_recalculate_apic_map()
> so that can can safely toggle the inhibit, e.g. the recalc helper already deals
> with multiple vCPUs changing their APIC state in parallel.  I don't think the fix
> will be too difficult to craft such that it's backport friendly, but it would need
> to be slotted into the series containing the aforementioned fix.

Thank you for the suggestion.  I will take a look, and start from your
series.

[PATCH v2] KVM: x86: Allow APICv APIC ID inhibit to be cleared

[Greg Edwards]

Legacy kernels prior to commit 4399c03c6780 ("x86/apic: Remove
verify_local_APIC()") write the APIC ID of the boot CPU twice to verify
a functioning local APIC.  This results in APIC acceleration inhibited
on these kernels for reason APICV_INHIBIT_REASON_APIC_ID_MODIFIED.

Allow the APICV_INHIBIT_REASON_APIC_ID_MODIFIED inhibit reason to be
cleared if/when all APICs in xAPIC mode set their APIC ID back to the
expected vcpu_id value.

Fold the functionality previously in kvm_lapic_xapic_id_updated() into
kvm_recalculate_apic_map(), as this allows us examine all APICs in one
pass.

Fixes: 3743c2f02517 ("KVM: x86: inhibit APICv/AVIC on changes to APIC ID or APIC base")
Signed-off-by: Greg Edwards <gedwards@ddn.com>
---
Changes from v1:
  * Fold kvm_lapic_xapic_id_updated() into kvm_recalculate_apic_map() and
    verify no APICs in xAPIC mode have a modified APIC ID before clearing
    APICV_INHIBIT_REASON_APIC_ID_MODIFIED.  [Sean]
  * Rebase on top of Sean's APIC fixes+cleanups series.  [Sean]
    https://lore.kernel.org/all/20221001005915.2041642-1-seanjc@google.com/

 arch/x86/kvm/lapic.c | 45 +++++++++++++++++++-------------------------
 1 file changed, 19 insertions(+), 26 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 9b3af49d2524..362472da6e7f 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -236,6 +236,7 @@ void kvm_recalculate_apic_map(struct kvm *kvm)
 	struct kvm_vcpu *vcpu;
 	unsigned long i;
 	u32 max_id = 255; /* enough space for any xAPIC ID */
+	bool xapic_id_modified = false;
 
 	/* Read kvm->arch.apic_map_dirty before kvm->arch.apic_map.  */
 	if (atomic_read_acquire(&kvm->arch.apic_map_dirty) == CLEAN)
@@ -285,6 +286,19 @@ void kvm_recalculate_apic_map(struct kvm *kvm)
 		xapic_id = kvm_xapic_id(apic);
 		x2apic_id = kvm_x2apic_id(apic);
 
+		if (!apic_x2apic_mode(apic)) {
+			/*
+			 * Deliberately truncate the vCPU ID when detecting a
+			 * modified APIC ID to avoid false positives if the
+			 * vCPU ID, i.e. x2APIC ID, is a 32-bit value.  If the
+			 * wrap/truncation results in unwanted aliasing, APICv
+			 * will be inhibited as part of updating KVM's
+			 * optimized APIC maps.
+			 */
+			if (xapic_id != (u8)x2apic_id)
+				xapic_id_modified = true;
+		}
+
 		/*
 		 * Apply KVM's hotplug hack if userspace has enable 32-bit APIC
 		 * IDs.  Allow sending events to vCPUs by their x2APIC ID even
@@ -396,6 +410,11 @@ void kvm_recalculate_apic_map(struct kvm *kvm)
 	else
 		kvm_clear_apicv_inhibit(kvm, APICV_INHIBIT_REASON_LOGICAL_ID_ALIASED);
 
+	if (xapic_id_modified)
+		kvm_set_apicv_inhibit(kvm, APICV_INHIBIT_REASON_APIC_ID_MODIFIED);
+	else
+		kvm_clear_apicv_inhibit(kvm, APICV_INHIBIT_REASON_APIC_ID_MODIFIED);
+
 	old = rcu_dereference_protected(kvm->arch.apic_map,
 			lockdep_is_held(&kvm->arch.apic_map_lock));
 	rcu_assign_pointer(kvm->arch.apic_map, new);
@@ -2155,28 +2174,6 @@ static void apic_manage_nmi_watchdog(struct kvm_lapic *apic, u32 lvt0_val)
 	}
 }
 
-static void kvm_lapic_xapic_id_updated(struct kvm_lapic *apic)
-{
-	struct kvm *kvm = apic->vcpu->kvm;
-
-	if (!kvm_apic_hw_enabled(apic))
-		return;
-
-	if (KVM_BUG_ON(apic_x2apic_mode(apic), kvm))
-		return;
-
-	/*
-	 * Deliberately truncate the vCPU ID when detecting a modified APIC ID
-	 * to avoid false positives if the vCPU ID, i.e. x2APIC ID, is a 32-bit
-	 * value.  If the wrap/truncation results in unwatned aliasing, APICv
-	 * will be inhibited as part of updating KVM's optimized APIC maps.
-	 */
-	if (kvm_xapic_id(apic) == (u8)apic->vcpu->vcpu_id)
-		return;
-
-	kvm_set_apicv_inhibit(apic->vcpu->kvm, APICV_INHIBIT_REASON_APIC_ID_MODIFIED);
-}
-
 static int get_lvt_index(u32 reg)
 {
 	if (reg == APIC_LVTCMCI)
@@ -2197,7 +2194,6 @@ static int kvm_lapic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val)
 	case APIC_ID:		/* Local APIC ID */
 		if (!apic_x2apic_mode(apic)) {
 			kvm_apic_set_xapic_id(apic, val >> 24);
-			kvm_lapic_xapic_id_updated(apic);
 		} else {
 			ret = 1;
 		}
@@ -2919,9 +2915,6 @@ int kvm_apic_set_state(struct kvm_vcpu *vcpu, struct kvm_lapic_state *s)
 	}
 	memcpy(vcpu->arch.apic->regs, s->regs, sizeof(*s));
 
-	if (!apic_x2apic_mode(vcpu->arch.apic))
-		kvm_lapic_xapic_id_updated(vcpu->arch.apic);
-
 	atomic_set_release(&apic->vcpu->kvm->arch.apic_map_dirty, DIRTY);
 	kvm_recalculate_apic_map(vcpu->kvm);
 	kvm_apic_set_version(vcpu);
-- 
2.38.1

Re: [PATCH v2] KVM: x86: Allow APICv APIC ID inhibit to be cleared

[Sean Christopherson]

On Wed, Nov 16, 2022, Greg Edwards wrote:
> Legacy kernels prior to commit 4399c03c6780 ("x86/apic: Remove
> verify_local_APIC()") write the APIC ID of the boot CPU twice to verify
> a functioning local APIC.  This results in APIC acceleration inhibited
> on these kernels for reason APICV_INHIBIT_REASON_APIC_ID_MODIFIED.
> 
> Allow the APICV_INHIBIT_REASON_APIC_ID_MODIFIED inhibit reason to be
> cleared if/when all APICs in xAPIC mode set their APIC ID back to the
> expected vcpu_id value.
> 
> Fold the functionality previously in kvm_lapic_xapic_id_updated() into
> kvm_recalculate_apic_map(), as this allows us examine all APICs in one
> pass.
> 
> Fixes: 3743c2f02517 ("KVM: x86: inhibit APICv/AVIC on changes to APIC ID or APIC base")
> Signed-off-by: Greg Edwards <gedwards@ddn.com>
> ---
> Changes from v1:
>   * Fold kvm_lapic_xapic_id_updated() into kvm_recalculate_apic_map() and
>     verify no APICs in xAPIC mode have a modified APIC ID before clearing
>     APICV_INHIBIT_REASON_APIC_ID_MODIFIED.  [Sean]
>   * Rebase on top of Sean's APIC fixes+cleanups series.  [Sean]
>     https://lore.kernel.org/all/20221001005915.2041642-1-seanjc@google.com/
> 
>  arch/x86/kvm/lapic.c | 45 +++++++++++++++++++-------------------------
>  1 file changed, 19 insertions(+), 26 deletions(-)
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 9b3af49d2524..362472da6e7f 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -236,6 +236,7 @@ void kvm_recalculate_apic_map(struct kvm *kvm)
>  	struct kvm_vcpu *vcpu;
>  	unsigned long i;
>  	u32 max_id = 255; /* enough space for any xAPIC ID */
> +	bool xapic_id_modified = false;

Maybe "xapic_id_mismatch"?  E.g. if KVM ends up back because the xAPIC ID was
modified back to be vcpu_id, then this is somewhat misleading from super pedantic
point of view.  "modified" was ok when the inhibit was a one-way street.

>  	/* Read kvm->arch.apic_map_dirty before kvm->arch.apic_map.  */
>  	if (atomic_read_acquire(&kvm->arch.apic_map_dirty) == CLEAN)
> @@ -285,6 +286,19 @@ void kvm_recalculate_apic_map(struct kvm *kvm)
>  		xapic_id = kvm_xapic_id(apic);
>  		x2apic_id = kvm_x2apic_id(apic);
>  
> +		if (!apic_x2apic_mode(apic)) {
> +			/*
> +			 * Deliberately truncate the vCPU ID when detecting a
> +			 * modified APIC ID to avoid false positives if the
> +			 * vCPU ID, i.e. x2APIC ID, is a 32-bit value.  If the
> +			 * wrap/truncation results in unwanted aliasing, APICv
> +			 * will be inhibited as part of updating KVM's
> +			 * optimized APIC maps.

Heh, the last sentence is stale since this _is_ the flow updates the optimized
maps.

> +			 */
> +			if (xapic_id != (u8)x2apic_id)

It's more than a bit silly, but I would rather this use vcpu->vcpu_id instead of
x2apic_id.  KVM's requirement is that the xAPIC ID must match the vCPU ID.  The
reason I called out x2APIC in the comment was to hint at why KVM even supports
32-bit vCPU IDs.  The fact that KVM also makes the x2APIC ID immutable is orthogonal,
which makes the above check somewhat confusing (even though they're identical under
the hood).

And we should fold the two if-statements together, then the block comment can be
placed outside of the if and have more less indentation to deal with.

How about:
		/*
		 * Deliberately truncate the vCPU ID when detecting a mismatched
		 * APIC ID to avoid false positives if the vCPU ID, i.e. x2APIC
		 * ID, is a 32-bit value.  Any unwanted aliasing due to
		 * truncation results will be detected below.
		 */
		if (!apic_x2apic_mode(apic) && xapic_id != vcpu->vcpu_id)
			xapic_id_mismatch = true;

[PATCH v3] KVM: x86: Allow APICv APIC ID inhibit to be cleared

[Greg Edwards]

Legacy kernels prior to commit 4399c03c6780 ("x86/apic: Remove
verify_local_APIC()") write the APIC ID of the boot CPU twice to verify
a functioning local APIC.  This results in APIC acceleration inhibited
on these kernels for reason APICV_INHIBIT_REASON_APIC_ID_MODIFIED.

Allow the APICV_INHIBIT_REASON_APIC_ID_MODIFIED inhibit reason to be
cleared if/when all APICs in xAPIC mode set their APIC ID back to the
expected vcpu_id value.

Fold the functionality previously in kvm_lapic_xapic_id_updated() into
kvm_recalculate_apic_map(), as this allows examining all APICs in one
pass.

Fixes: 3743c2f02517 ("KVM: x86: inhibit APICv/AVIC on changes to APIC ID or APIC base")
Signed-off-by: Greg Edwards <gedwards@ddn.com>
---
Changes from v2:
  * Comment and variable name tweaks.  [Sean]

Changes from v1:
  * Fold kvm_lapic_xapic_id_updated() into kvm_recalculate_apic_map() and
    verify no APICs in xAPIC mode have a modified APIC ID before clearing
    APICV_INHIBIT_REASON_APIC_ID_MODIFIED.  [Sean]
  * Rebase on top of Sean's APIC fixes+cleanups series.  [Sean]
    https://lore.kernel.org/all/20221001005915.2041642-1-seanjc@google.com/

 arch/x86/kvm/lapic.c | 41 +++++++++++++++--------------------------
 1 file changed, 15 insertions(+), 26 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 9b3af49d2524..5224d73cd84a 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -236,6 +236,7 @@ void kvm_recalculate_apic_map(struct kvm *kvm)
 	struct kvm_vcpu *vcpu;
 	unsigned long i;
 	u32 max_id = 255; /* enough space for any xAPIC ID */
+	bool xapic_id_mismatch = false;
 
 	/* Read kvm->arch.apic_map_dirty before kvm->arch.apic_map.  */
 	if (atomic_read_acquire(&kvm->arch.apic_map_dirty) == CLEAN)
@@ -285,6 +286,15 @@ void kvm_recalculate_apic_map(struct kvm *kvm)
 		xapic_id = kvm_xapic_id(apic);
 		x2apic_id = kvm_x2apic_id(apic);
 
+		/*
+		 * Deliberately truncate the vCPU ID when detecting a mismatched
+		 * APIC ID to avoid false positives if the vCPU ID, i.e. x2APIC
+		 * ID, is a 32-bit value.  Any unwanted aliasing due to
+		 * truncation results will be detected below.
+		 */
+		if (!apic_x2apic_mode(apic) && xapic_id != (u8)vcpu->vcpu_id)
+			xapic_id_mismatch = true;
+
 		/*
 		 * Apply KVM's hotplug hack if userspace has enable 32-bit APIC
 		 * IDs.  Allow sending events to vCPUs by their x2APIC ID even
@@ -396,6 +406,11 @@ void kvm_recalculate_apic_map(struct kvm *kvm)
 	else
 		kvm_clear_apicv_inhibit(kvm, APICV_INHIBIT_REASON_LOGICAL_ID_ALIASED);
 
+	if (xapic_id_mismatch)
+		kvm_set_apicv_inhibit(kvm, APICV_INHIBIT_REASON_APIC_ID_MODIFIED);
+	else
+		kvm_clear_apicv_inhibit(kvm, APICV_INHIBIT_REASON_APIC_ID_MODIFIED);
+
 	old = rcu_dereference_protected(kvm->arch.apic_map,
 			lockdep_is_held(&kvm->arch.apic_map_lock));
 	rcu_assign_pointer(kvm->arch.apic_map, new);
@@ -2155,28 +2170,6 @@ static void apic_manage_nmi_watchdog(struct kvm_lapic *apic, u32 lvt0_val)
 	}
 }
 
-static void kvm_lapic_xapic_id_updated(struct kvm_lapic *apic)
-{
-	struct kvm *kvm = apic->vcpu->kvm;
-
-	if (!kvm_apic_hw_enabled(apic))
-		return;
-
-	if (KVM_BUG_ON(apic_x2apic_mode(apic), kvm))
-		return;
-
-	/*
-	 * Deliberately truncate the vCPU ID when detecting a modified APIC ID
-	 * to avoid false positives if the vCPU ID, i.e. x2APIC ID, is a 32-bit
-	 * value.  If the wrap/truncation results in unwatned aliasing, APICv
-	 * will be inhibited as part of updating KVM's optimized APIC maps.
-	 */
-	if (kvm_xapic_id(apic) == (u8)apic->vcpu->vcpu_id)
-		return;
-
-	kvm_set_apicv_inhibit(apic->vcpu->kvm, APICV_INHIBIT_REASON_APIC_ID_MODIFIED);
-}
-
 static int get_lvt_index(u32 reg)
 {
 	if (reg == APIC_LVTCMCI)
@@ -2197,7 +2190,6 @@ static int kvm_lapic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val)
 	case APIC_ID:		/* Local APIC ID */
 		if (!apic_x2apic_mode(apic)) {
 			kvm_apic_set_xapic_id(apic, val >> 24);
-			kvm_lapic_xapic_id_updated(apic);
 		} else {
 			ret = 1;
 		}
@@ -2919,9 +2911,6 @@ int kvm_apic_set_state(struct kvm_vcpu *vcpu, struct kvm_lapic_state *s)
 	}
 	memcpy(vcpu->arch.apic->regs, s->regs, sizeof(*s));
 
-	if (!apic_x2apic_mode(vcpu->arch.apic))
-		kvm_lapic_xapic_id_updated(vcpu->arch.apic);
-
 	atomic_set_release(&apic->vcpu->kvm->arch.apic_map_dirty, DIRTY);
 	kvm_recalculate_apic_map(vcpu->kvm);
 	kvm_apic_set_version(vcpu);
-- 
2.38.1

Re: [PATCH v3] KVM: x86: Allow APICv APIC ID inhibit to be cleared

[Sean Christopherson]

On Thu, Nov 17, 2022, Greg Edwards wrote:
> Legacy kernels prior to commit 4399c03c6780 ("x86/apic: Remove
> verify_local_APIC()") write the APIC ID of the boot CPU twice to verify
> a functioning local APIC.  This results in APIC acceleration inhibited
> on these kernels for reason APICV_INHIBIT_REASON_APIC_ID_MODIFIED.
> 
> Allow the APICV_INHIBIT_REASON_APIC_ID_MODIFIED inhibit reason to be
> cleared if/when all APICs in xAPIC mode set their APIC ID back to the
> expected vcpu_id value.
> 
> Fold the functionality previously in kvm_lapic_xapic_id_updated() into
> kvm_recalculate_apic_map(), as this allows examining all APICs in one
> pass.
> 
> Fixes: 3743c2f02517 ("KVM: x86: inhibit APICv/AVIC on changes to APIC ID or APIC base")
> Signed-off-by: Greg Edwards <gedwards@ddn.com>
> ---

Reviewed-by: Sean Christopherson <seanjc@google.com>

Re: [PATCH v3] KVM: x86: Allow APICv APIC ID inhibit to be cleared

[Maxim Levitsky]

On Thu, 2022-11-17 at 11:33 -0700, Greg Edwards wrote:
> Legacy kernels prior to commit 4399c03c6780 ("x86/apic: Remove
> verify_local_APIC()") write the APIC ID of the boot CPU twice to verify
> a functioning local APIC.  This results in APIC acceleration inhibited
> on these kernels for reason APICV_INHIBIT_REASON_APIC_ID_MODIFIED.
> 
> Allow the APICV_INHIBIT_REASON_APIC_ID_MODIFIED inhibit reason to be
> cleared if/when all APICs in xAPIC mode set their APIC ID back to the
> expected vcpu_id value.
> 
> Fold the functionality previously in kvm_lapic_xapic_id_updated() into
> kvm_recalculate_apic_map(), as this allows examining all APICs in one
> pass.
> 
> Fixes: 3743c2f02517 ("KVM: x86: inhibit APICv/AVIC on changes to APIC ID or APIC base")
> Signed-off-by: Greg Edwards <gedwards@ddn.com>
> ---
> Changes from v2:
>   * Comment and variable name tweaks.  [Sean]
> 
> Changes from v1:
>   * Fold kvm_lapic_xapic_id_updated() into kvm_recalculate_apic_map() and
>     verify no APICs in xAPIC mode have a modified APIC ID before clearing
>     APICV_INHIBIT_REASON_APIC_ID_MODIFIED.  [Sean]
>   * Rebase on top of Sean's APIC fixes+cleanups series.  [Sean]
>     https://lore.kernel.org/all/20221001005915.2041642-1-seanjc@google.com/
> 
>  arch/x86/kvm/lapic.c | 41 +++++++++++++++--------------------------
>  1 file changed, 15 insertions(+), 26 deletions(-)
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 9b3af49d2524..5224d73cd84a 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -236,6 +236,7 @@ void kvm_recalculate_apic_map(struct kvm *kvm)
>         struct kvm_vcpu *vcpu;
>         unsigned long i;
>         u32 max_id = 255; /* enough space for any xAPIC ID */
> +       bool xapic_id_mismatch = false;
>  
>         /* Read kvm->arch.apic_map_dirty before kvm->arch.apic_map.  */
>         if (atomic_read_acquire(&kvm->arch.apic_map_dirty) == CLEAN)
> @@ -285,6 +286,15 @@ void kvm_recalculate_apic_map(struct kvm *kvm)
>                 xapic_id = kvm_xapic_id(apic);
>                 x2apic_id = kvm_x2apic_id(apic);
>  
> +               /*
> +                * Deliberately truncate the vCPU ID when detecting a mismatched
> +                * APIC ID to avoid false positives if the vCPU ID, i.e. x2APIC
> +                * ID, is a 32-bit value.  Any unwanted aliasing due to
> +                * truncation results will be detected below.
> +                */
> +               if (!apic_x2apic_mode(apic) && xapic_id != (u8)vcpu->vcpu_id)
> +                       xapic_id_mismatch = true;
> +
>                 /*
>                  * Apply KVM's hotplug hack if userspace has enable 32-bit APIC
>                  * IDs.  Allow sending events to vCPUs by their x2APIC ID even
> @@ -396,6 +406,11 @@ void kvm_recalculate_apic_map(struct kvm *kvm)
>         else
>                 kvm_clear_apicv_inhibit(kvm, APICV_INHIBIT_REASON_LOGICAL_ID_ALIASED);
>  
> +       if (xapic_id_mismatch)
> +               kvm_set_apicv_inhibit(kvm, APICV_INHIBIT_REASON_APIC_ID_MODIFIED);
> +       else
> +               kvm_clear_apicv_inhibit(kvm, APICV_INHIBIT_REASON_APIC_ID_MODIFIED);
> +
>         old = rcu_dereference_protected(kvm->arch.apic_map,
>                         lockdep_is_held(&kvm->arch.apic_map_lock));
>         rcu_assign_pointer(kvm->arch.apic_map, new);
> @@ -2155,28 +2170,6 @@ static void apic_manage_nmi_watchdog(struct kvm_lapic *apic, u32 lvt0_val)
>         }
>  }
>  
> -static void kvm_lapic_xapic_id_updated(struct kvm_lapic *apic)
> -{
> -       struct kvm *kvm = apic->vcpu->kvm;
> -
> -       if (!kvm_apic_hw_enabled(apic))
> -               return;
> -
> -       if (KVM_BUG_ON(apic_x2apic_mode(apic), kvm))
> -               return;
> -
> -       /*
> -        * Deliberately truncate the vCPU ID when detecting a modified APIC ID
> -        * to avoid false positives if the vCPU ID, i.e. x2APIC ID, is a 32-bit
> -        * value.  If the wrap/truncation results in unwatned aliasing, APICv
> -        * will be inhibited as part of updating KVM's optimized APIC maps.
> -        */
> -       if (kvm_xapic_id(apic) == (u8)apic->vcpu->vcpu_id)
> -               return;
> -
> -       kvm_set_apicv_inhibit(apic->vcpu->kvm, APICV_INHIBIT_REASON_APIC_ID_MODIFIED);
> -}
> -
>  static int get_lvt_index(u32 reg)
>  {
>         if (reg == APIC_LVTCMCI)
> @@ -2197,7 +2190,6 @@ static int kvm_lapic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val)
>         case APIC_ID:           /* Local APIC ID */
>                 if (!apic_x2apic_mode(apic)) {
>                         kvm_apic_set_xapic_id(apic, val >> 24);
> -                       kvm_lapic_xapic_id_updated(apic);
>                 } else {
>                         ret = 1;
>                 }
> @@ -2919,9 +2911,6 @@ int kvm_apic_set_state(struct kvm_vcpu *vcpu, struct kvm_lapic_state *s)
>         }
>         memcpy(vcpu->arch.apic->regs, s->regs, sizeof(*s));
>  
> -       if (!apic_x2apic_mode(vcpu->arch.apic))
> -               kvm_lapic_xapic_id_updated(vcpu->arch.apic);
> -
>         atomic_set_release(&apic->vcpu->kvm->arch.apic_map_dirty, DIRTY);
>         kvm_recalculate_apic_map(vcpu->kvm);
>         kvm_apic_set_version(vcpu);


Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>

Best regards,
	Maxim Levitsky