Re: [PATCH] x86/apic: Fix APIC MSR access error when x2apic is disabled

[Thomas Gleixner <tglx@linutronix.de>]

On Tue, Jan 30 2024 at 22:56, Adrian Huang wrote:
> When appending the 'iommu=off' kernel parameter, the kernel complains
> about the following error message [1]:
>
> unchecked MSR access error: RDMSR from 0x802 at rIP: 0xffffffff94079992 (native_apic_msr_read+0x12/0x50)
>
> The root cause is that:
>   1. x2apic_mode is configured as '1' in check_x2apic().
>   2. apic_x2apic_cluster (assigned to global variable 'apic') is
>      selected in default_acpi_madt_oem_check().
>   3. x2apic_disable() is invoked in try_to_enable_x2apic().
>      Call path:
>        enable_IR_x2apic
>         |- try_to_enable_x2apic
>           |- x2apic_disable
>             |- __x2apic_disable
>             |- apic_set_fixmap
>               |- apic_read_boot_cpu_id(false)
>   4. read_apic_id() in apic_read_boot_cpu_id() invokes
>      native_apic_msr_read(), which leads to the error message.
>      Call path:
>        apic_read_boot_cpu_id
>          |- read_apic_id
>            |- apic_read
>              |- apic->read() ['apic' points to apic_x2apic_cluster]
> 	       |- native_apic_msr_read
>
> Since x2apic mode has been disabled by writing MSR_IA32_APICBASE in
> __x2apic_disable, the upcoming MSR accesses will trigger the MSR
> access error. Note that APIC and x2APIC registers are accessed via
> MMIO in xapic mode and those regiters are access via the MSR-based
> interface in x2apic mode [2].

Nice detective work.

Aside of that apic_read_boot_cpu_id() invokes cpu_set_boot_apic() which
accounts for the boot APIC another time.

> Fix the issue by checking if boot_cpu_physical_apicid has been
> initialized.

That works by some definition of works, but why invoking
apic_read_boot_cpu_id() in the first place?

x2apic_disable() knows for sure that X2APIC was enabled early due to
x2apic_state. So it can tell apic_set_fixmap() to _not_ invoke
apic_read_boot_cpu_id(), no?

Something like the untested below.

> [1] https://gist.github.com/AdrianHuang/9e5ce38d410af3ccd0b5ac1703e032bc

That's not really helpful as it's a full dmesg and does not provide any
more information than the above decoded stacktrace.

> [2] Chapter 16, AMD64 Architecture Programmer’s Manual Volume 2:
>     System Programming

Neither this as people who fiddle with X86 APIC internals should know
where to find that information, no?

Thanks,

        tglx
---
diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index 4667bc4b00ab..7fc6d5c2c38c 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -1808,7 +1808,7 @@ void x2apic_setup(void)
 	__x2apic_enable();
 }
 
-static __init void apic_set_fixmap(void);
+static __init void apic_set_fixmap(bool read_apic);
 
 static __init void x2apic_disable(void)
 {
@@ -1830,7 +1830,12 @@ static __init void x2apic_disable(void)
 	}
 
 	__x2apic_disable();
-	apic_set_fixmap();
+	/*
+	 * Don't reread the APIC ID as it was already done from
+	 * check_x2apic() and the apic driver still is a x2APIC variant,
+	 * which fails to do the read after x2APIC was disabled.
+	 */
+	apic_set_fixmap(false);
 }
 
 static __init void x2apic_enable(void)
@@ -2095,13 +2100,14 @@ void __init init_apic_mappings(void)
 	}
 }
 
-static __init void apic_set_fixmap(void)
+static __init void apic_set_fixmap(bool read_apic)
 {
 	set_fixmap_nocache(FIX_APIC_BASE, mp_lapic_addr);
 	apic_mmio_base = APIC_BASE;
 	apic_printk(APIC_VERBOSE, "mapped APIC to %16lx (%16lx)\n",
 		    apic_mmio_base, mp_lapic_addr);
-	apic_read_boot_cpu_id(false);
+	if (read_apic)
+		apic_read_boot_cpu_id(false);
 }
 
 void __init register_lapic_address(unsigned long address)
@@ -2111,7 +2117,7 @@ void __init register_lapic_address(unsigned long address)
 	mp_lapic_addr = address;
 
 	if (!x2apic_mode)
-		apic_set_fixmap();
+		apic_set_fixmap(true);
 }
 
 /*