Re: [PATCH] x86/apic: Switch to x2apic driver early if x2apic is enabled

[Thomas Gleixner <tglx@kernel.org>]

On Fri, Sep 11 2026 at 17:17, Grzegorz Jaszczyk wrote:
> On Fri, Sep 11, 2026 at 2:51 PM Thomas Gleixner <tglx@kernel.org> wrote:
>>
>> On Thu, Sep 10 2026 at 09:04, Grzegorz Jaszczyk wrote:
>> > To prevent this, check if x2apic is enabled in hardware during early APIC
>> > call setup in apic_setup_apic_calls() and switch the default APIC driver
>> > from apic_physflat to apic_x2apic_phys immediately. This ensures that any
>> > early EOI writes use safe MSR-based accesses. The driver can still be
>> > upgraded later during the normal APIC probe phase.
>>
>> That's really the wrong place. Initializing those calls has absolutely
>> nothing to do with the driver probing.
>>
>> There is quite some historical ballast in this whole APIC driver
>> probing, which I never came around to clean up.
>>
>> I'll have a look how this can be done sanely without replicating half of
>> the existing code just to paper over the real issue.
>>
> Ok, understood. Thank you. Let me know when you have something to look
> at or test.

It actually needs a larger overhaul because when the x2apic_phys driver
is installed early on then its probe function will prevent installing
the cluster driver.

So your claim that any other driver can still be selected is only true
when ACPI invokes the MADT check function, but not via the probe
mechanism.

I'm not really happy about the untested below, but something like that
should solve your problem and still keep all options open. That patch
needs to be split up in more digestable pieces.

FYI, I'm on vacation and only sporadically at the keyboard. So if you
get it working feel free to polish it up and submit the whole pile with
a "Suggested-by: tglx..." tag. It definitely needs some eyeballs and
testing time in tip.

Thanks,

        tglx
---
--- a/arch/x86/include/asm/apic.h
+++ b/arch/x86/include/asm/apic.h
@@ -133,7 +133,8 @@ extern void sync_Arb_IDs(void);
 extern void init_bsp_APIC(void);
 extern void apic_intr_mode_select(void);
 extern void apic_intr_mode_init(void);
-extern void init_apic_mappings(void);
+void apic_early_init(void);
+void apic_finalize_early_init(void);
 void register_lapic_address(unsigned long address);
 extern void setup_boot_APIC_clock(void);
 extern void setup_secondary_APIC_clock(void);
@@ -254,7 +255,6 @@ static inline u32 native_apic_msr_read(u
 #define x2apic_mode		(0)
 #define	x2apic_supported()	(0)
 #endif /* !CONFIG_X86_X2APIC */
-extern void __init check_x2apic(void);
 
 struct irq_data;
 
@@ -288,7 +288,8 @@ struct apic {
 	u32	disable_esr		: 1,
 		dest_mode_logical	: 1,
 		x2apic_set_max_apicid	: 1,
-		nmi_to_offline_cpu	: 1;
+		nmi_to_offline_cpu	: 1,
+		probed			: 1;
 
 	u32	(*calc_dest_apicid)(unsigned int cpu);
 
@@ -366,9 +367,6 @@ extern int lapic_can_unplug_cpu(void);
 #ifdef CONFIG_X86_LOCAL_APIC
 extern struct apic_override __x86_apic_override;
 
-void __init apic_setup_apic_calls(void);
-void __init apic_install_driver(struct apic *driver);
-
 #define apic_update_callback(_callback, _fn) {					\
 		__x86_apic_override._callback = _fn;				\
 		apic->_callback = _fn;						\
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -1869,7 +1869,7 @@ static __init void try_to_enable_x2apic(
 	x2apic_enable();
 }
 
-void __init check_x2apic(void)
+static __init struct apic *check_x2apic(void)
 {
 	if (x2apic_enabled()) {
 		pr_info("x2apic: enabled by BIOS, switching to x2apic ops\n");
@@ -1879,15 +1879,19 @@ void __init check_x2apic(void)
 		else
 			x2apic_state = X2APIC_ON;
 		apic_read_boot_cpu_id(true);
+		/* Force the initial driver to X2APIC */
+		return &apic_x2apic_phys;
 	} else if (!boot_cpu_has(X86_FEATURE_X2APIC)) {
 		x2apic_state = X2APIC_DISABLED;
 	}
+	/* Keep the default driver */
+	return NULL;
 }
 #else /* CONFIG_X86_X2APIC */
-void __init check_x2apic(void)
+static __init struct apic *check_x2apic(void)
 {
 	if (!apic_is_x2apic_enabled())
-		return;
+		return NULL;
 	/*
 	 * Checkme: Can we simply turn off x2APIC here instead of disabling the APIC?
 	 */
@@ -1896,6 +1900,7 @@ void __init check_x2apic(void)
 
 	apic_is_disabled = true;
 	setup_clear_cpu_cap(X86_FEATURE_APIC);
+	return &apic_noop;
 }
 
 static inline void try_to_enable_x2apic(int remap_mode) { }
@@ -2064,10 +2069,22 @@ static bool __init detect_init_APIC(void
 }
 #endif
 
-/**
- * init_apic_mappings - initialize APIC mappings
- */
-void __init init_apic_mappings(void)
+void __init apic_early_init(void)
+{
+	struct apic *driver;
+
+	if (acpi_mps_check()) {
+		apic_is_disabled = true;
+		setup_clear_cpu_cap(X86_FEATURE_APIC);
+		driver = &apic_noop;
+	} else {
+		driver = check_x2apic();
+	}
+
+	apic_install_default_driver(driver);
+}
+
+void __init apic_finalize_early_init(void)
 {
 	if (apic_validate_deadline_timer())
 		pr_info("TSC deadline timer available\n");
--- a/arch/x86/kernel/apic/init.c
+++ b/arch/x86/kernel/apic/init.c
@@ -80,19 +80,8 @@ static __init void update_static_calls(v
 	update_call(wakeup_secondary_cpu_64);
 }
 
-void __init apic_setup_apic_calls(void)
+static void __init __apic_install_driver(struct apic *driver)
 {
-	/* Ensure that the default APIC has native_eoi populated */
-	apic->native_eoi = apic->eoi;
-	update_static_calls();
-	pr_info("Static calls initialized\n");
-}
-
-void __init apic_install_driver(struct apic *driver)
-{
-	if (apic == driver)
-		return;
-
 	apic = driver;
 
 	if (IS_ENABLED(CONFIG_X86_X2APIC) && apic->x2apic_set_max_apicid)
@@ -105,6 +94,24 @@ void __init apic_install_driver(struct a
 	/* Apply any already installed callback overrides */
 	restore_override_callbacks();
 	update_static_calls();
+}
 
-	pr_info("Switched APIC routing to: %s\n", driver->name);
+void __init apic_install_default_driver(struct apic *driver)
+{
+	if (driver && driver != apic) {
+		__apic_install_driver(driver);
+	} else {
+		/* Ensure that the default APIC has the native_eoi() callback populated */
+		apic->native_eoi = apic->eoi;
+		update_static_calls();
+	}
+	pr_info("Set default APIC routing to: %s\n", apic->name);
+}
+
+void __init apic_install_driver(struct apic *driver)
+{
+	if (apic != driver)
+		__apic_install_driver(driver);
+	apic->probed = true;
+	pr_info("Switched APIC routing to: %s\n", apic->name);
 }
--- a/arch/x86/kernel/apic/local.h
+++ b/arch/x86/kernel/apic/local.h
@@ -13,12 +13,16 @@
 #include <asm/irq_vectors.h>
 #include <asm/apic.h>
 
+void __init apic_install_default_driver(struct apic *driver);
+void __init apic_install_driver(struct apic *driver);
+
 /* X2APIC */
 u32 x2apic_get_apic_id(u32 id);
 
 void x2apic_send_IPI_all(int vector);
 void x2apic_send_IPI_allbutself(int vector);
 void x2apic_send_IPI_self(int vector);
+extern struct apic apic_x2apic_phys;
 extern u32 x2apic_max_apicid;
 
 /* IPI */
--- a/arch/x86/kernel/apic/x2apic_phys.c
+++ b/arch/x86/kernel/apic/x2apic_phys.c
@@ -7,7 +7,6 @@
 
 int x2apic_phys;
 
-static struct apic apic_x2apic_phys;
 u32 x2apic_max_apicid __ro_after_init = UINT_MAX;
 
 void __init x2apic_set_max_apicid(u32 apicid)
@@ -113,7 +112,7 @@ static int x2apic_phys_probe(void)
 	if (x2apic_phys || x2apic_fadt_phys())
 		return 1;
 
-	return apic == &apic_x2apic_phys;
+	return apic == &apic_x2apic_phys && apic->probed;
 }
 
 u32 x2apic_get_apic_id(u32 id)
@@ -121,7 +120,7 @@ u32 x2apic_get_apic_id(u32 id)
 	return id;
 }
 
-static struct apic apic_x2apic_phys __ro_after_init = {
+struct apic apic_x2apic_phys __ro_after_init = {
 
 	.name				= "physical x2apic",
 	.probe				= x2apic_phys_probe,
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -993,14 +993,7 @@ void __init setup_arch(char **cmdline_p)
 
 	x86_report_nx();
 
-	apic_setup_apic_calls();
-
-	if (acpi_mps_check()) {
-#ifdef CONFIG_X86_LOCAL_APIC
-		apic_is_disabled = true;
-#endif
-		setup_clear_cpu_cap(X86_FEATURE_APIC);
-	}
+	apic_early_init();
 
 	e820__finish_early_params();
 
@@ -1062,7 +1055,6 @@ void __init setup_arch(char **cmdline_p)
 	/* max_low_pfn get updated here */
 	find_low_pfn_range();
 #else
-	check_x2apic();
 
 	/* How many end-of-memory variables you have, grandma! */
 	/* need this before calling reserve_initrd */
@@ -1242,7 +1234,7 @@ void __init setup_arch(char **cmdline_p)
 	x86_init.mpparse.parse_smp_cfg();
 
 	/* Last opportunity to detect and map the local APIC */
-	init_apic_mappings();
+	apic_finalize_early_init();
 
 	topology_init_possible_cpus();