[v2] KVM: x86: Allow APICv APIC ID inhibit to be cleared
Commit Message
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(-)
Comments
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;
@@ -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);