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

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

[Adrian Huang]

From: Adrian Huang <ahuang12@lenovo.com>

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].

Fix the issue by checking if boot_cpu_physical_apicid has been
initialized.

[1] https://gist.github.com/AdrianHuang/9e5ce38d410af3ccd0b5ac1703e032bc
[2] Chapter 16, AMD64 Architecture Programmer’s Manual Volume 2:
    System Programming

Fixes: d10a904435fa ("x86/apic: Consolidate boot_cpu_physical_apicid initialization sites")
Signed-off-by: Adrian Huang <ahuang12@lenovo.com>
---
 arch/x86/kernel/apic/apic.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index 4667bc4b00ab..6700d6f266ca 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -1703,6 +1703,9 @@ static __init void apic_read_boot_cpu_id(bool x2apic)
 		boot_cpu_physical_apicid = native_apic_msr_read(APIC_ID);
 		boot_cpu_apic_version = GET_APIC_VERSION(native_apic_msr_read(APIC_LVR));
 	} else {
+		if (boot_cpu_physical_apicid != BAD_APICID)
+			return;
+
 		boot_cpu_physical_apicid = read_apic_id();
 		boot_cpu_apic_version = GET_APIC_VERSION(apic_read(APIC_LVR));
 	}
-- 
2.25.1

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

[Thomas Gleixner]

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);
 }
 
 /*

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

[Adrian Huang12]

> 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);
>  }
> 
>  /*

Confirmed that this patch fixes the issue. Thanks.

Feel free to add: Tested-by: Adrian Huang <ahuang12@lenovo.com>

-- Adrian

[PATCH] x86/apic: Don't access the APIC when disabling X2APIC

[Thomas Gleixner]

With 'iommu=off' on the kernel command line and X2APIC enabled by the BIOS
the code which disables the X2APIC triggers an unchecked MSR access error:

  RDMSR from 0x802 at rIP: 0xffffffff94079992 (native_apic_msr_read+0x12/0x50)                                                                                                                                      

This is happens because default_acpi_madt_oem_check() selects an X2APIC
driver before the X2APIC is disabled.

When the X2APIC is disabled because interrupt remapping cannot be enabled
due to 'iommu=off' on the command line, x2apic_disable() invokes
apic_set_fixmap() which in turn tries to read the APIC ID. This triggers
the MSR warning because X2APIC is disabled, but the APIC driver is still
X2APIC based.

Prevent that by adding an argument to apic_set_fixmap() which makes the
APIC ID read out conditional and set it to false from the X2APIC disable
path. That's correct as the APIC ID has already been read out during early
discovery.
 
Fixes: d10a904435fa ("x86/apic: Consolidate boot_cpu_physical_apicid initialization sites")
Reported-by: Adrian Huang <ahuang12@lenovo.com>                                                                                                                                                                                             
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Adrian Huang <ahuang12@lenovo.com>                                                                                                                                                                                             
Cc: stable@vger.kernel.org
---
 arch/x86/kernel/apic/apic.c |   16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -1771,7 +1771,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)
 {
@@ -1793,7 +1793,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)
@@ -2057,13 +2062,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)
@@ -2073,7 +2079,7 @@ void __init register_lapic_address(unsig
 	mp_lapic_addr = address;
 
 	if (!x2apic_mode)
-		apic_set_fixmap();
+		apic_set_fixmap(true);
 }
 
 /*

[tip: x86/urgent] x86/apic: Don't access the APIC when disabling X2APIC

[tip-bot2 for Thomas Gleixner]

The following commit has been merged into the x86/urgent branch of tip:

Commit-ID:     1e1dd773644e9de88f54386f7147c1068375fc75
Gitweb:        https://git.kernel.org/tip/1e1dd773644e9de88f54386f7147c1068375fc75
Author:        Thomas Gleixner <tglx@linutronix.de>
AuthorDate:    Fri, 26 Apr 2024 00:30:36 +02:00
Committer:     Borislav Petkov (AMD) <bp@alien8.de>
CommitterDate: Mon, 29 Apr 2024 12:08:07 +02:00

x86/apic: Don't access the APIC when disabling X2APIC

With 'iommu=off' on the kernel command line and X2APIC enabled by the BIOS
the code which disables the X2APIC triggers an unchecked MSR access error:

  RDMSR from 0x802 at rIP: 0xffffffff94079992 (native_apic_msr_read+0x12/0x50)

This is happens because default_acpi_madt_oem_check() selects an X2APIC
driver before the X2APIC is disabled.

When the X2APIC is disabled because interrupt remapping cannot be enabled
due to 'iommu=off' on the command line, x2apic_disable() invokes
apic_set_fixmap() which in turn tries to read the APIC ID. This triggers
the MSR warning because X2APIC is disabled, but the APIC driver is still
X2APIC based.

Prevent that by adding an argument to apic_set_fixmap() which makes the
APIC ID read out conditional and set it to false from the X2APIC disable
path. That's correct as the APIC ID has already been read out during early
discovery.

Fixes: d10a904435fa ("x86/apic: Consolidate boot_cpu_physical_apicid initialization sites")
Reported-by: Adrian Huang <ahuang12@lenovo.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
Tested-by: Adrian Huang <ahuang12@lenovo.com>
Cc: stable@vger.kernel.org
Link: https://lore.kernel.org/r/875xw5t6r7.ffs@tglx
---
 arch/x86/kernel/apic/apic.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index c342c4a..b229648 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -1771,7 +1771,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)
 {
@@ -1793,7 +1793,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)
@@ -2057,13 +2062,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)
@@ -2073,7 +2079,7 @@ void __init register_lapic_address(unsigned long address)
 	mp_lapic_addr = address;
 
 	if (!x2apic_mode)
-		apic_set_fixmap();
+		apic_set_fixmap(true);
 }
 
 /*

Re: [tip: x86/urgent] x86/apic: Don't access the APIC when disabling X2APIC

[Ingo Molnar]

* tip-bot2 for Thomas Gleixner <tip-bot2@linutronix.de> wrote:

> -	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);

JFYI, I amended the commit with the fixlet below, to avoid the inevitable 
followup trivial patch:

diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index b229648b7a18..803dcfb0e346 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -1795,7 +1795,7 @@ static __init void x2apic_disable(void)
 	__x2apic_disable();
 	/*
 	 * Don't reread the APIC ID as it was already done from
-	 * check_x2apic() and the apic driver still is a x2APIC variant,
+	 * 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);

Thanks,

	Ingo

[tip: x86/urgent] x86/apic: Don't access the APIC when disabling x2APIC

[tip-bot2 for Thomas Gleixner]

The following commit has been merged into the x86/urgent branch of tip:

Commit-ID:     720a22fd6c1cdadf691281909950c0cbc5cdf17e
Gitweb:        https://git.kernel.org/tip/720a22fd6c1cdadf691281909950c0cbc5cdf17e
Author:        Thomas Gleixner <tglx@linutronix.de>
AuthorDate:    Fri, 26 Apr 2024 00:30:36 +02:00
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Tue, 30 Apr 2024 07:51:34 +02:00

x86/apic: Don't access the APIC when disabling x2APIC

With 'iommu=off' on the kernel command line and x2APIC enabled by the BIOS
the code which disables the x2APIC triggers an unchecked MSR access error:

  RDMSR from 0x802 at rIP: 0xffffffff94079992 (native_apic_msr_read+0x12/0x50)

This is happens because default_acpi_madt_oem_check() selects an x2APIC
driver before the x2APIC is disabled.

When the x2APIC is disabled because interrupt remapping cannot be enabled
due to 'iommu=off' on the command line, x2apic_disable() invokes
apic_set_fixmap() which in turn tries to read the APIC ID. This triggers
the MSR warning because x2APIC is disabled, but the APIC driver is still
x2APIC based.

Prevent that by adding an argument to apic_set_fixmap() which makes the
APIC ID read out conditional and set it to false from the x2APIC disable
path. That's correct as the APIC ID has already been read out during early
discovery.

Fixes: d10a904435fa ("x86/apic: Consolidate boot_cpu_physical_apicid initialization sites")
Reported-by: Adrian Huang <ahuang12@lenovo.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Tested-by: Adrian Huang <ahuang12@lenovo.com>
Cc: stable@vger.kernel.org
Link: https://lore.kernel.org/r/875xw5t6r7.ffs@tglx
---
 arch/x86/kernel/apic/apic.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index c342c4a..803dcfb 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -1771,7 +1771,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)
 {
@@ -1793,7 +1793,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)
@@ -2057,13 +2062,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)
@@ -2073,7 +2079,7 @@ void __init register_lapic_address(unsigned long address)
 	mp_lapic_addr = address;
 
 	if (!x2apic_mode)
-		apic_set_fixmap();
+		apic_set_fixmap(true);
 }
 
 /*