Re: [kvm-unit-tests PATCH 1/8] x86: Always use legacy xAPIC to get APIC ID during TSS setup

From: David Matlack <dmatlack@google.com>
To: Sean Christopherson <seanjc@google.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>, kvm list <kvm@vger.kernel.org>
Subject: Re: [kvm-unit-tests PATCH 1/8] x86: Always use legacy xAPIC to get APIC ID during TSS setup
Date: Fri, 21 Jan 2022 17:18:47 -0800	[thread overview]
Message-ID: <CALzav=dFUwKcq0cEJ38OxcSH7WjH2PCmJh_2msFUQjTNzXexNg@mail.gmail.com> (raw)
In-Reply-To: <20220121231852.1439917-2-seanjc@google.com>

On Fri, Jan 21, 2022 at 3:23 PM Sean Christopherson <seanjc@google.com> wrote:
>
> Force use of xAPIC to retrieve the APIC ID during TSS setup to fix an
> issue where an AP can switch apic_ops to point at x2apic_ops before
> setup_tss() completes, leading to a #GP and triple fault due to trying
> to read an x2APIC MSR without x2APIC being enabled.
>
> A future patch will make apic_ops a per-cpu pointer, but that's not of
> any help for 32-bit, which uses the APIC ID to determine the GS selector,
> i.e. 32-bit KUT has a chicken-and-egg problem.  All setup_tss() callers
> ensure the local APIC is in xAPIC mode, so just force use of xAPIC in
> this case.
>
> Fixes: 7e33895 ("x86: Move 32-bit GDT and TSS to desc.c")
> Fixes: dbd3800 ("x86: Move 64-bit GDT and TSS to desc.c")

FYI there was recently a report [1] to the KVM mailing list that Fixes
tags should use at least 12 characters for hashes. So I guess this
should be:

Fixes: 7e33895d3232 ("x86: Move 32-bit GDT and TSS to desc.c")
Fixes: dbd380049042 ("x86: Move 64-bit GDT and TSS to desc.c")

[1] https://lore.kernel.org/kvm/20220121081432.5b671602@canb.auug.org.au/

> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  lib/x86/apic.c  | 5 +++++
>  lib/x86/apic.h  | 2 ++
>  lib/x86/setup.c | 4 ++--
>  3 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/lib/x86/apic.c b/lib/x86/apic.c
> index da8f3013..b404d580 100644
> --- a/lib/x86/apic.c
> +++ b/lib/x86/apic.c
> @@ -48,6 +48,11 @@ static uint32_t xapic_id(void)
>      return xapic_read(APIC_ID) >> 24;
>  }
>
> +uint32_t pre_boot_apic_id(void)
> +{
> +       return xapic_id();
> +}
> +
>  static const struct apic_ops xapic_ops = {
>      .reg_read = xapic_read,
>      .reg_write = xapic_write,
> diff --git a/lib/x86/apic.h b/lib/x86/apic.h
> index c4821716..7844324b 100644
> --- a/lib/x86/apic.h
> +++ b/lib/x86/apic.h
> @@ -53,6 +53,8 @@ bool apic_read_bit(unsigned reg, int n);
>  void apic_write(unsigned reg, uint32_t val);
>  void apic_icr_write(uint32_t val, uint32_t dest);
>  uint32_t apic_id(void);
> +uint32_t pre_boot_apic_id(void);
> +
>
>  int enable_x2apic(void);
>  void disable_apic(void);
> diff --git a/lib/x86/setup.c b/lib/x86/setup.c
> index bbd34682..64217add 100644
> --- a/lib/x86/setup.c
> +++ b/lib/x86/setup.c
> @@ -109,7 +109,7 @@ unsigned long setup_tss(u8 *stacktop)
>         u32 id;
>         tss64_t *tss_entry;
>
> -       id = apic_id();
> +       id = pre_boot_apic_id();
>
>         /* Runtime address of current TSS */
>         tss_entry = &tss[id];
> @@ -129,7 +129,7 @@ unsigned long setup_tss(u8 *stacktop)
>         u32 id;
>         tss32_t *tss_entry;
>
> -       id = apic_id();
> +       id = pre_boot_apic_id();
>
>         /* Runtime address of current TSS */
>         tss_entry = &tss[id];
> --
> 2.35.0.rc0.227.g00780c9af4-goog
>