[PATCH] x86/apic: Fix the issues in x2apic_disable()

[PATCH] x86/apic: Fix the issues in x2apic_disable()

[Yuntao Wang]

There are two issues in x2apic_disable().

The first issue is that the 'pr_warn' in 'if (x2apic_hw_locked())' will
never be executed, because when x2apic_hw_locked() evaluates to true,
x2apic_state should be X2APIC_ON_LOCKED. However, the current logic in
x2apic_disable() is that if x2apic_state is not X2APIC_ON, it returns
early, so the subsequent logic will not be executed.

Therefore, 'if (state != X2APIC_ON)' should be changed to
'if (state < X2APIC_ON)', so that when x2apic_state is X2APIC_ON_LOCKED,
the corresponding warning log can be printed.

The second issue is that the current logic of x2apic_disable() first sets
x2apic_mode and x2apic_state to 0 and X2APIC_DISABLED, respectively, and
then tries to disable x2APIC. However, when the APIC is locked in x2APIC
mode, that is, x2apic_state is X2APIC_ON_LOCKED, x2APIC cannot be disabled,
which causes the final values of the x2apic_mode/x2apic_state variables to
be inconsistent with the actual state of x2APIC.

Let's fix these issues.

Signed-off-by: Yuntao Wang <yuntao.wang@linux.dev>
---
 arch/x86/kernel/apic/apic.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index 66fd4b2a37a3..fce8d0214069 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -1775,13 +1775,10 @@ static __init void apic_set_fixmap(bool read_apic);
 
 static __init void x2apic_disable(void)
 {
-	u32 x2apic_id, state = x2apic_state;
+	u32 x2apic_id;
 
-	x2apic_mode = 0;
-	x2apic_state = X2APIC_DISABLED;
-
-	if (state != X2APIC_ON)
-		return;
+	if (x2apic_state < X2APIC_ON)
+		goto out;
 
 	x2apic_id = read_apic_id();
 	if (x2apic_id >= 255)
@@ -1799,6 +1796,10 @@ static __init void x2apic_disable(void)
 	 * which fails to do the read after x2APIC was disabled.
 	 */
 	apic_set_fixmap(false);
+
+out:
+	x2apic_mode = 0;
+	x2apic_state = X2APIC_DISABLED;
 }
 
 static __init void x2apic_enable(void)
-- 
2.46.0

Re: [PATCH] x86/apic: Fix the issues in x2apic_disable()

[Thomas Gleixner]

On Mon, Aug 12 2024 at 18:08, Yuntao Wang wrote:
>  static __init void x2apic_disable(void)
>  {
> -	u32 x2apic_id, state = x2apic_state;
> +	u32 x2apic_id;
>  
> -	x2apic_mode = 0;
> -	x2apic_state = X2APIC_DISABLED;
> -
> -	if (state != X2APIC_ON)
> -		return;
> +	if (x2apic_state < X2APIC_ON)
> +		goto out;

There is no point in overwriting the state in case it is < ON, no?
  
Thanks,

        tglx

Re: [PATCH] x86/apic: Fix the issues in x2apic_disable()

[Yuntao Wang]

On Mon, 12 Aug 2024 16:48:05 +0200, Thomas Gleixner <tglx@linutronix.de> wrote:

> On Mon, Aug 12 2024 at 18:08, Yuntao Wang wrote:
> >  static __init void x2apic_disable(void)
> >  {
> > -	u32 x2apic_id, state = x2apic_state;
> > +	u32 x2apic_id;
> >  
> > -	x2apic_mode = 0;
> > -	x2apic_state = X2APIC_DISABLED;
> > -
> > -	if (state != X2APIC_ON)
> > -		return;
> > +	if (x2apic_state < X2APIC_ON)
> > +		goto out;
> 
> There is no point in overwriting the state in case it is < ON, no?

Are you saying that we should replace 'goto out' with a return statement?

However, when x2apic_disable() is called, it's possible that x2apic_state
is X2APIC_OFF. In that case, we should set x2apic_state to X2APIC_DISABLED.

So, I think overwriting the state is necessary.

Thanks,
Yuntao

> Thanks,
> 
>         tglx

Re: [PATCH] x86/apic: Fix the issues in x2apic_disable()

[Thomas Gleixner]

On Mon, Aug 12 2024 at 23:53, Yuntao Wang wrote:
> On Mon, 12 Aug 2024 16:48:05 +0200, Thomas Gleixner <tglx@linutronix.de> wrote:
>> There is no point in overwriting the state in case it is < ON, no?
>
> Are you saying that we should replace 'goto out' with a return statement?
>
> However, when x2apic_disable() is called, it's possible that x2apic_state
> is X2APIC_OFF. In that case, we should set x2apic_state to X2APIC_DISABLED.

What for? It can't be enabled later on and for the rest of the system it
does not matter whether the state is OFF or DISABLED. Either case says:
X2APIC is not enabled.

Thanks,

        tglx

[PATCH v2] x86/apic: Fix the issues in x2apic_disable()

[Yuntao Wang]

There are two issues in x2apic_disable().

The first issue is that the 'pr_warn' in 'if (x2apic_hw_locked())' will
never be executed, because when x2apic_hw_locked() evaluates to true,
x2apic_state should be X2APIC_ON_LOCKED. However, the current logic in
x2apic_disable() is that if x2apic_state is not X2APIC_ON, it returns
early, so the subsequent logic will not be executed.

Therefore, 'if (state != X2APIC_ON)' should be changed to
'if (state < X2APIC_ON)', so that when x2apic_state is X2APIC_ON_LOCKED,
the corresponding warning log can be printed.

The second issue is that the current logic of x2apic_disable() first sets
x2apic_mode and x2apic_state to 0 and X2APIC_DISABLED, respectively, and
then tries to disable x2APIC. However, when the APIC is locked in x2APIC
mode, that is, x2apic_state is X2APIC_ON_LOCKED, x2APIC cannot be disabled,
which causes the final values of the x2apic_mode/x2apic_state variables to
be inconsistent with the actual state of x2APIC.

Let's fix these issues.

Signed-off-by: Yuntao Wang <yuntao.wang@linux.dev>
---
v1->v2: Modify according to Thomas Gleixner's suggestion.

 arch/x86/kernel/apic/apic.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index 66fd4b2a37a3..50acd094b055 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -1775,12 +1775,9 @@ static __init void apic_set_fixmap(bool read_apic);
 
 static __init void x2apic_disable(void)
 {
-	u32 x2apic_id, state = x2apic_state;
+	u32 x2apic_id;
 
-	x2apic_mode = 0;
-	x2apic_state = X2APIC_DISABLED;
-
-	if (state != X2APIC_ON)
+	if (x2apic_state < X2APIC_ON)
 		return;
 
 	x2apic_id = read_apic_id();
@@ -1799,6 +1796,9 @@ static __init void x2apic_disable(void)
 	 * which fails to do the read after x2APIC was disabled.
 	 */
 	apic_set_fixmap(false);
+
+	x2apic_mode = 0;
+	x2apic_state = X2APIC_DISABLED;
 }
 
 static __init void x2apic_enable(void)
-- 
2.46.0

[tip: x86/urgent] x86/apic: Make x2apic_disable() work correctly

[tip-bot2 for Yuntao Wang]

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

Commit-ID:     0ecc5be200c84e67114f3640064ba2bae3ba2f5a
Gitweb:        https://git.kernel.org/tip/0ecc5be200c84e67114f3640064ba2bae3ba2f5a
Author:        Yuntao Wang <yuntao.wang@linux.dev>
AuthorDate:    Tue, 13 Aug 2024 09:48:27 +08:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Tue, 13 Aug 2024 15:15:19 +02:00

x86/apic: Make x2apic_disable() work correctly

x2apic_disable() clears x2apic_state and x2apic_mode unconditionally, even
when the state is X2APIC_ON_LOCKED, which prevents the kernel to disable
it thereby creating inconsistent state.

Due to the early state check for X2APIC_ON, the code path which warns about
a locked X2APIC cannot be reached.

Test for state < X2APIC_ON instead and move the clearing of the state and
mode variables to the place which actually disables X2APIC.

[ tglx: Massaged change log. Added Fixes tag. Moved clearing so it's at the
  	right place for back ports ]

Fixes: a57e456a7b28 ("x86/apic: Fix fallout from x2apic cleanup")
Signed-off-by: Yuntao Wang <yuntao.wang@linux.dev>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: stable@vger.kernel.org
Link: https://lore.kernel.org/all/20240813014827.895381-1-yuntao.wang@linux.dev

---
 arch/x86/kernel/apic/apic.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index 66fd4b2..3736386 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -1775,12 +1775,9 @@ static __init void apic_set_fixmap(bool read_apic);
 
 static __init void x2apic_disable(void)
 {
-	u32 x2apic_id, state = x2apic_state;
+	u32 x2apic_id;
 
-	x2apic_mode = 0;
-	x2apic_state = X2APIC_DISABLED;
-
-	if (state != X2APIC_ON)
+	if (x2apic_state < X2APIC_ON)
 		return;
 
 	x2apic_id = read_apic_id();
@@ -1793,6 +1790,10 @@ static __init void x2apic_disable(void)
 	}
 
 	__x2apic_disable();
+
+	x2apic_mode = 0;
+	x2apic_state = X2APIC_DISABLED;
+
 	/*
 	 * Don't reread the APIC ID as it was already done from
 	 * check_x2apic() and the APIC driver still is a x2APIC variant,