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@xxxxxxx>
> ---
> 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@xxxxxxxxxx/
>
> 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@xxxxxxxxxx>
Best regards,
Maxim Levitsky
