[PATCH] x86/apic: check FADT settings after enable x2apic

[PATCH] x86/apic: check FADT settings after enable x2apic

[Stoney Wang]

OS will enable x2apic mode even BIOS default in xapic mode.

FADT settings check (commit ea0dcf903e7d76aa5d483d876215fedcfdfe140f)
should be applied after detect default mode and change the mode (enable_IR_x2apic called)

Signed-off-by: Stoney Wang <song-bo.wang@hp.com>
---
 arch/x86/kernel/apic/x2apic_phys.c |   25 ++++++++++++++++---------
 1 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kernel/apic/x2apic_phys.c b/arch/x86/kernel/apic/x2apic_phys.c
index e03a1e1..76ea60d 100644
--- a/arch/x86/kernel/apic/x2apic_phys.c
+++ b/arch/x86/kernel/apic/x2apic_phys.c
@@ -20,18 +20,22 @@ static int set_x2apic_phys_mode(char *arg)
 }
 early_param("x2apic_phys", set_x2apic_phys_mode);
 
-static int x2apic_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
+static int x2apic_fadt_phys(void)
 {
-	if (x2apic_phys)
-		return x2apic_enabled();
-	else if ((acpi_gbl_FADT.header.revision >= FADT2_REVISION_ID) &&
-		(acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL) &&
-		x2apic_enabled()) {
+	if ((acpi_gbl_FADT.header.revision >= FADT2_REVISION_ID) &&
+		(acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL)) {
 		printk(KERN_DEBUG "System requires x2apic physical mode\n");
 		return 1;
 	}
-	else
-		return 0;
+	return 0;
+}
+
+static int x2apic_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
+{
+	if (x2apic_enabled())
+		return x2apic_phys || x2apic_fadt_phys();
+
+	return 0;
 }
 
 static void
@@ -85,7 +89,10 @@ static int x2apic_phys_probe(void)
 	if (x2apic_mode && x2apic_phys)
 		return 1;
 
-	return apic == &apic_x2apic_phys;
+	if (apic == &apic_x2apic_phys)
+		return 1;
+
+	return x2apic_enabled() && x2apic_fadt_phys();
 }
 
 static struct apic apic_x2apic_phys = {
-- 
1.7.1

RE: [PATCH] x86/apic: check FADT settings after enable x2apic

[Wang, Song-Bo (Stoney)]

Hi Yinghai, hpa and others,

Would you please review the patch on detecting x2apic FADT settings?

We meet a BIOS system which works on x2apic physical mode by setting the bit ACPI_FADT_APIC_PHYSICAL in FADT table.
And for those systems with all cpuid < 255, the spec requires BIOS's default mode in xapic.
The kernel detects the default mode and do some initializations and will call enable_IR_x2apic and change the mode to x2apic successfully.

So it is necessary to check ACPI_FADT_APIC_PHYSICAL bit after the kernel change the mode from xapic to x2apic.

(*drv)->acpi_madt_oem_check is called on detect default BIOS mode,
(*drv)->probe is called after enable_IR_x2apic,

The previous FADT check (commit ea0dcf90) should be applied to x2apic_phys_probe too.

Thanks,
Stoney

-----Original Message-----
From: Wang, Song-Bo (Stoney) 
Sent: Tuesday, January 15, 2013 9:51 AM
To: suresh.b.siddha@intel.com
Cc: Zhang, Lin-Bao (Linux Kernel R&D); Pearson, Greg; linux-kernel@vger.kernel.org; Wang, Song-Bo (Stoney)
Subject: [PATCH] x86/apic: check FADT settings after enable x2apic

OS will enable x2apic mode even BIOS default in xapic mode.

FADT settings check (commit ea0dcf903e7d76aa5d483d876215fedcfdfe140f)
should be applied after detect default mode and change the mode (enable_IR_x2apic called)

Signed-off-by: Stoney Wang <song-bo.wang@hp.com>
---
 arch/x86/kernel/apic/x2apic_phys.c |   25 ++++++++++++++++---------
 1 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kernel/apic/x2apic_phys.c b/arch/x86/kernel/apic/x2apic_phys.c
index e03a1e1..76ea60d 100644
--- a/arch/x86/kernel/apic/x2apic_phys.c
+++ b/arch/x86/kernel/apic/x2apic_phys.c
@@ -20,18 +20,22 @@ static int set_x2apic_phys_mode(char *arg)  }  early_param("x2apic_phys", set_x2apic_phys_mode);
 
-static int x2apic_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
+static int x2apic_fadt_phys(void)
 {
-	if (x2apic_phys)
-		return x2apic_enabled();
-	else if ((acpi_gbl_FADT.header.revision >= FADT2_REVISION_ID) &&
-		(acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL) &&
-		x2apic_enabled()) {
+	if ((acpi_gbl_FADT.header.revision >= FADT2_REVISION_ID) &&
+		(acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL)) {
 		printk(KERN_DEBUG "System requires x2apic physical mode\n");
 		return 1;
 	}
-	else
-		return 0;
+	return 0;
+}
+
+static int x2apic_acpi_madt_oem_check(char *oem_id, char *oem_table_id) 
+{
+	if (x2apic_enabled())
+		return x2apic_phys || x2apic_fadt_phys();
+
+	return 0;
 }
 
 static void
@@ -85,7 +89,10 @@ static int x2apic_phys_probe(void)
 	if (x2apic_mode && x2apic_phys)
 		return 1;
 
-	return apic == &apic_x2apic_phys;
+	if (apic == &apic_x2apic_phys)
+		return 1;
+
+	return x2apic_enabled() && x2apic_fadt_phys();
 }
 
 static struct apic apic_x2apic_phys = {
--
1.7.1

Re: [PATCH] x86/apic: check FADT settings after enable x2apic

[Yinghai Lu]

[-- Attachment #1: Type: text/plain, Size: 731 bytes --]

On Sun, Jan 27, 2013 at 9:05 PM, Wang, Song-Bo (Stoney)
<song-bo.wang@hp.com> wrote:
> Hi Yinghai, hpa and others,
>
> Would you please review the patch on detecting x2apic FADT settings?
>
> We meet a BIOS system which works on x2apic physical mode by setting the bit ACPI_FADT_APIC_PHYSICAL in FADT table.
> And for those systems with all cpuid < 255, the spec requires BIOS's default mode in xapic.
> The kernel detects the default mode and do some initializations and will call enable_IR_x2apic and change the mode to x2apic successfully.

Hi, Peter and Ingo,

I checked the patch, and looks right.

I updated the changelog and simplify the code a little bit.

Please check if you can put it into tip:x86/apic

Thanks

Yinghai

[-- Attachment #2: wang_hp_x2apic.patch --]
[-- Type: application/octet-stream, Size: 2646 bytes --]

From:	Stoney Wang <song-bo.wang@hp.com>
Subject: [PATCH] x86, apic: Check fadt x2apic phys in x2apic_phys_probe()

HP has systems that work with x2apic phys mode and BIOS set
ACPI_FADT_APIC_PHYSICAL in FADT table, and all cpuid < 255, the spec
requires BIOS only put system on xapic mode.
Kernel will set to x2apic logical mode instead of x2apic phys mode.

Current code handle x2apic as:
1. BIOS set x2apic mode:
When user specify x2apic_phys, or FADT indicates PHYSICAL.
During madt oem check, apic driver will set correctly to x2apic phys driver.

2. BIOS does NOT set x2apic mode:
When user specify x2apic_phys, or FADT indicates PHYSICAL.
During madt oem check, apic driver will get set with xapic logical or
xapic phys driver at first.
Later enable_IR_x2apic() will enable x2apic_mode.
After that, x2apic_phys_probe() get called.
That will install right x2apic physical driver when user specify x2apic_phys,
but will install wrong apic driver (x2apic logical) when FADT indicates
PHYSICAL, because that probe does not check FADT PHYSICAL.

Fix that by adding check x2apic_fadt_phys in x2apic_phys_probe().

[ changelog, and simplify code - Yinghai Lu ]
Signed-off-by: Stoney Wang <song-bo.wang@hp.com>
Signed-off-by: Yinghai Lu <yinghai@kernel.org>

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

Index: linux-2.6/arch/x86/kernel/apic/x2apic_phys.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/apic/x2apic_phys.c
+++ linux-2.6/arch/x86/kernel/apic/x2apic_phys.c
@@ -20,18 +20,19 @@ static int set_x2apic_phys_mode(char *ar
 }
 early_param("x2apic_phys", set_x2apic_phys_mode);
 
-static int x2apic_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
+static int x2apic_fadt_phys(void)
 {
-	if (x2apic_phys)
-		return x2apic_enabled();
-	else if ((acpi_gbl_FADT.header.revision >= FADT2_REVISION_ID) &&
-		(acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL) &&
-		x2apic_enabled()) {
+	if ((acpi_gbl_FADT.header.revision >= FADT2_REVISION_ID) &&
+		(acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL)) {
 		printk(KERN_DEBUG "System requires x2apic physical mode\n");
 		return 1;
 	}
-	else
-		return 0;
+	return 0;
+}
+
+static int x2apic_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
+{
+	return x2apic_enabled() && (x2apic_phys || x2apic_fadt_phys());
 }
 
 static void
@@ -82,7 +83,7 @@ static void init_x2apic_ldr(void)
 
 static int x2apic_phys_probe(void)
 {
-	if (x2apic_mode && x2apic_phys)
+	if (x2apic_mode && (x2apic_phys || x2apic_fadt_phys()))
 		return 1;
 
 	return apic == &apic_x2apic_phys;

Re: [PATCH] x86/apic: check FADT settings after enable x2apic

[Ingo Molnar]

* Yinghai Lu <yinghai@kernel.org> wrote:

> On Sun, Jan 27, 2013 at 9:05 PM, Wang, Song-Bo (Stoney)
> <song-bo.wang@hp.com> wrote:
> > Hi Yinghai, hpa and others,
> >
> > Would you please review the patch on detecting x2apic FADT settings?
> >
> > We meet a BIOS system which works on x2apic physical mode by 
> > setting the bit ACPI_FADT_APIC_PHYSICAL in FADT table. And 
> > for those systems with all cpuid < 255, the spec requires 
> > BIOS's default mode in xapic. The kernel detects the default 
> > mode and do some initializations and will call 
> > enable_IR_x2apic and change the mode to x2apic successfully.
> 
> Hi, Peter and Ingo,
> 
> I checked the patch, and looks right.
> 
> I updated the changelog and simplify the code a little bit.
> 
> Please check if you can put it into tip:x86/apic

The code looks good to me, but the changelog is still lacking:

> HP has systems that work with x2apic phys mode and BIOS set 
> ACPI_FADT_APIC_PHYSICAL in FADT table, and all cpuid < 255, 
> the spec requires BIOS only put system on xapic mode. Kernel 

Which spec?

> will set to x2apic logical mode instead of x2apic phys mode.

Which has exactly what bad effect on users of these systems?

You left out the most important information from the changelog: 
why do users care, what good does the patch do?

Thanks,

	Ingo

Re: [PATCH] x86/apic: check FADT settings after enable x2apic

[Yinghai Lu]

On Mon, Jan 28, 2013 at 2:11 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
>> HP has systems that work with x2apic phys mode and BIOS set
>> ACPI_FADT_APIC_PHYSICAL in FADT table, and all cpuid < 255,
>> the spec requires BIOS only put system on xapic mode. Kernel
>
> Which spec?
>
>> will set to x2apic logical mode instead of x2apic phys mode.
>
> Which has exactly what bad effect on users of these systems?
>
> You left out the most important information from the changelog:
> why do users care, what good does the patch do?

please check you are happy with this:

---
From:   Stoney Wang <song-bo.wang@hp.com>
Subject: [PATCH] x86, apic: Check fadt x2apic phys in x2apic_phys_probe()

HP has systems that only work with x2apic phys mode and BIOS set
ACPI_FADT_APIC_PHYSICAL in FADT table. But all apicid < 255,
according to x2apic-spec, chapter 2.9, BIOS need to pass the control
to the OS with xapic mode.
Kernel will set apic driver wrong to x2apic cluster instead of x2apic phys.

The user will have to append nox2apic in boot command line to stay xapic mode,
or append x2apic_phys to switch to x2apic phys mode.

That is kernel bug about handling fadt phys bit.

Current code handle x2apic as:
1. When BIOS set x2apic mode:
When user specify x2apic_phys, or FADT indicates PHYSICAL.
During madt oem check, apic driver is set correctly to x2apic phys driver.

2. When BIOS does NOT set x2apic mode:
When user specify x2apic_phys, or FADT indicates PHYSICAL.
During madt oem check, apic driver is set with xapic logical or
xapic phys driver at first.
Later enable_IR_x2apic() will enable x2apic_mode.
After that, x2apic_phys_probe() will install right x2apic phys driver
when user specify x2apic_phys,
but will skip and let x2apic_cluster_probe to take over to install
wrong apic driver (x2apic cluster) when FADT indicates
PHYSICAL, because x2apic_phys_probe does not check FADT PHYSICAL.

Fix that by adding check x2apic_fadt_phys in x2apic_phys_probe().

[ changelog, and simplify code - Yinghai Lu ]
Signed-off-by: Stoney Wang <song-bo.wang@hp.com>
Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---

Re: [PATCH] x86/apic: check FADT settings after enable x2apic

[Ingo Molnar]

* Yinghai Lu <yinghai@kernel.org> wrote:

> On Mon, Jan 28, 2013 at 2:11 AM, Ingo Molnar <mingo@kernel.org> wrote:
> >
> >> HP has systems that work with x2apic phys mode and BIOS set
> >> ACPI_FADT_APIC_PHYSICAL in FADT table, and all cpuid < 255,
> >> the spec requires BIOS only put system on xapic mode. Kernel
> >
> > Which spec?
> >
> >> will set to x2apic logical mode instead of x2apic phys mode.
> >
> > Which has exactly what bad effect on users of these systems?
> >
> > You left out the most important information from the changelog:
> > why do users care, what good does the patch do?
> 
> please check you are happy with this:
> 
> ---
> From:   Stoney Wang <song-bo.wang@hp.com>
> Subject: [PATCH] x86, apic: Check fadt x2apic phys in x2apic_phys_probe()
> 
> HP has systems that only work with x2apic phys mode and BIOS set
> ACPI_FADT_APIC_PHYSICAL in FADT table. But all apicid < 255,
> according to x2apic-spec, chapter 2.9, BIOS need to pass the control
> to the OS with xapic mode.
> Kernel will set apic driver wrong to x2apic cluster instead of x2apic phys.
> 
> The user will have to append nox2apic in boot command line to stay xapic mode,
> or append x2apic_phys to switch to x2apic phys mode.

This still does not explain what happens if none of this user 
action is taken. I.e. what exact _user visible problem_ does the 
patch fix?

Is this really so unimportant to you? Almost everyone will start 
a changelog with explaining what badness happens. Not you - you 
explain everything from how the fix works to how to work around 
the bug - except describing the most important thing: theuser 
visible problem itself ... Weird.

Thanks,

	Ingo

RE: [PATCH] x86/apic: check FADT settings after enable x2apic

[Wang, Song-Bo (Stoney)]

> From: Ingo Molnar [mailto:mingo.kernel.org@gmail.com] On Behalf Of Ingo
> Molnar
> Sent: Tuesday, January 29, 2013 3:48 PM
> To: Yinghai Lu
> Cc: Wang, Song-Bo (Stoney); H. Peter Anvin; Ingo Molnar; Thomas
> Gleixner; Zhang, Lin-Bao (Linux Kernel R&D); Pearson, Greg; linux-
> kernel@vger.kernel.org; suresh.b.siddha@intel.com
> Subject: Re: [PATCH] x86/apic: check FADT settings after enable x2apic
> 
> 
> * Yinghai Lu <yinghai@kernel.org> wrote:
> 
> > On Mon, Jan 28, 2013 at 2:11 AM, Ingo Molnar <mingo@kernel.org> wrote:
> > >
> > >> HP has systems that work with x2apic phys mode and BIOS set
> > >> ACPI_FADT_APIC_PHYSICAL in FADT table, and all cpuid < 255, the
> > >> spec requires BIOS only put system on xapic mode. Kernel
> > >
> > > Which spec?
> > >
> > >> will set to x2apic logical mode instead of x2apic phys mode.
> > >
> > > Which has exactly what bad effect on users of these systems?
> > >
> > > You left out the most important information from the changelog:
> > > why do users care, what good does the patch do?
> >
> > please check you are happy with this:
> >
> > ---
> > From:   Stoney Wang <song-bo.wang@hp.com>
> > Subject: [PATCH] x86, apic: Check fadt x2apic phys in
> > x2apic_phys_probe()
> >
> > HP has systems that only work with x2apic phys mode and BIOS set
> > ACPI_FADT_APIC_PHYSICAL in FADT table. But all apicid < 255,
> according
> > to x2apic-spec, chapter 2.9, BIOS need to pass the control to the OS
> > with xapic mode.
> > Kernel will set apic driver wrong to x2apic cluster instead of x2apic
> phys.
> >
> > The user will have to append nox2apic in boot command line to stay
> > xapic mode, or append x2apic_phys to switch to x2apic phys mode.
> 
> This still does not explain what happens if none of this user action is
> taken. I.e. what exact _user visible problem_ does the patch fix?
> 
> Is this really so unimportant to you? Almost everyone will start a
> changelog with explaining what badness happens. Not you - you explain
> everything from how the fix works to how to work around the bug -
> except describing the most important thing: theuser visible problem
> itself ... Weird.
> 
> Thanks,
> 
> 	Ingo

Hi Ingo,

The HW requires x2apic physical mode, and if we do not apply this patch, the OS will follow into x2apic cluster mode by default.
Due to this mode mismatch, with specific HW configurations, there will be intermittent lost interrupts, which could result in a hang or data loss.

Logically, there is a mismatch because the kernel missed detection on HW configurations, and we should fix it, right?
Do we need more detailed information?

Thanks,

    Stoney

Re: [PATCH] x86/apic: check FADT settings after enable x2apic

[Ingo Molnar]

* Wang, Song-Bo (Stoney) <song-bo.wang@hp.com> wrote:

> [...] Due to this mode mismatch, with specific HW 
> configurations, there will be intermittent lost interrupts, 
> which could result in a hang or data loss.

That's the key piece of information that was missing - which 
should be put into the changelog into a very prominent place.

> Logically, there is a mismatch because the kernel missed 
> detection on HW configurations, and we should fix it, right? 
> Do we need more detailed information?

No, the above sentence is more than enough and the fix looks 
nice. Note that this sentence is more useful to users than the 
rest of the changelog combined!

Thanks,

	Ingo

Re: [PATCH] x86/apic: check FADT settings after enable x2apic

[Yinghai Lu]

On Tue, Jan 29, 2013 at 12:49 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Wang, Song-Bo (Stoney) <song-bo.wang@hp.com> wrote:
>
>> [...] Due to this mode mismatch, with specific HW
>> configurations, there will be intermittent lost interrupts,
>> which could result in a hang or data loss.

Wang,

Maybe off topic, You should never ship those kind of system before you
fix that in BIOS, as user will have problem with current distribution.

You need to ask your BIOS guys (or OEM's BIOS guys) to check with Intel
to set one bit in IIO configure space to enable x2apic cluster.

Thanks

Yinghai

Re: [PATCH] x86/apic: check FADT settings after enable x2apic

[Yinghai Lu]

[-- Attachment #1: Type: text/plain, Size: 795 bytes --]

On Tue, Jan 29, 2013 at 12:49 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Wang, Song-Bo (Stoney) <song-bo.wang@hp.com> wrote:
>
>> [...] Due to this mode mismatch, with specific HW
>> configurations, there will be intermittent lost interrupts,
>> which could result in a hang or data loss.
>
> That's the key piece of information that was missing - which
> should be put into the changelog into a very prominent place.
>
>> Logically, there is a mismatch because the kernel missed
>> detection on HW configurations, and we should fix it, right?
>> Do we need more detailed information?
>
> No, the above sentence is more than enough and the fix looks
> nice. Note that this sentence is more useful to users than the
> rest of the changelog combined!

Please check attached.

Thanks

Yinghai

[-- Attachment #2: wang_hp_x2apic.patch --]
[-- Type: application/octet-stream, Size: 2707 bytes --]

From:	Stoney Wang <song-bo.wang@hp.com>
Subject: [PATCH] x86, apic: Check fadt x2apic phys in x2apic_phys_probe()

When some HP sytems boot without x2apic_phys, there will be intermittent
lost interrupts and could result in a hang or data loss.

Those systems only work with x2apic phys mode so BIOS set
ACPI_FADT_APIC_PHYSICAL in FADT table.
Also because all apicids are less then 255, BIOS need to pass the control
to the OS with xapic mode, according to x2apic-spec, chapter 2.9.

Current code handle x2apic when BIOS pass with xapic mode:

When user specify x2apic_phys, or FADT indicates PHYSICAL.
During madt oem check, apic driver is set with xapic logical or
xapic phys driver at first.
Later enable_IR_x2apic() will enable x2apic_mode.
After that, x2apic_phys_probe() will install right x2apic phys driver
if user specify x2apic_phys,
but will skip and let x2apic_cluster_probe to take over to install
wrong apic driver (x2apic cluster) when FADT indicates
PHYSICAL, because x2apic_phys_probe does not check FADT PHYSICAL.

Add checking x2apic_fadt_phys in x2apic_phys_probe() to fix the problem.

-v3: update the change according to Ingo.

[ changelog, and simplify code - Yinghai Lu ]
Signed-off-by: Stoney Wang <song-bo.wang@hp.com>
Signed-off-by: Yinghai Lu <yinghai@kernel.org>

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

Index: linux-2.6/arch/x86/kernel/apic/x2apic_phys.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/apic/x2apic_phys.c
+++ linux-2.6/arch/x86/kernel/apic/x2apic_phys.c
@@ -20,18 +20,19 @@ static int set_x2apic_phys_mode(char *ar
 }
 early_param("x2apic_phys", set_x2apic_phys_mode);
 
-static int x2apic_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
+static bool x2apic_fadt_phys(void)
 {
-	if (x2apic_phys)
-		return x2apic_enabled();
-	else if ((acpi_gbl_FADT.header.revision >= FADT2_REVISION_ID) &&
-		(acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL) &&
-		x2apic_enabled()) {
+	if ((acpi_gbl_FADT.header.revision >= FADT2_REVISION_ID) &&
+		(acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL)) {
 		printk(KERN_DEBUG "System requires x2apic physical mode\n");
-		return 1;
+		return true;
 	}
-	else
-		return 0;
+	return false;
+}
+
+static int x2apic_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
+{
+	return x2apic_enabled() && (x2apic_phys || x2apic_fadt_phys());
 }
 
 static void
@@ -82,7 +83,7 @@ static void init_x2apic_ldr(void)
 
 static int x2apic_phys_probe(void)
 {
-	if (x2apic_mode && x2apic_phys)
+	if (x2apic_mode && (x2apic_phys || x2apic_fadt_phys()))
 		return 1;
 
 	return apic == &apic_x2apic_phys;

Re: [PATCH] x86/apic: check FADT settings after enable x2apic

[Ingo Molnar]

* Yinghai Lu <yinghai@kernel.org> wrote:

> Please check attached.

Almost good.

This:

  > When some HP sytems boot without x2apic_phys, there will be

Should mention the approximate models of the systems affected - 
is it just a specific line of systems, or a wider range of 
systems affected?

This will inform users and will help maintainers like me to 
prioritize the merging and backporting of patches.

Thanks,

	Ingo

Re: [PATCH] x86/apic: check FADT settings after enable x2apic

[Yinghai Lu]

On Thu, Jan 31, 2013 at 3:32 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> This:
>
>   > When some HP sytems boot without x2apic_phys, there will be
>
> Should mention the approximate models of the systems affected -
> is it just a specific line of systems, or a wider range of
> systems affected?
>
> This will inform users and will help maintainers like me to
> prioritize the merging and backporting of patches.

Yes.

Wang,

Can you give the model No. if it is already shipped?

Thanks

Yinghai

RE: [PATCH] x86/apic: check FADT settings after enable x2apic

[Wang, Song-Bo (Stoney)]

 * Ingo Molnar <mingo.kernel.org@gmail.com> wrote:
> 
> * Yinghai Lu <yinghai@kernel.org> wrote:
> 
> > Please check attached.
> 
> Almost good.
> 
> This:
> 
>   > When some HP sytems boot without x2apic_phys, there will be
> 
> Should mention the approximate models of the systems affected - is it
> just a specific line of systems, or a wider range of systems affected?
> 
> This will inform users and will help maintainers like me to prioritize
> the merging and backporting of patches.
> 
> Thanks,
> 
> 	Ingo

Hi Ingo,

Due to some HW limitation, HP ProLiant DL980 G7 Server has the BIT ACPI_FADT_APIC_PHYSICAL set in BIOS.

This model of systems already shipped. It is great if some backporting could be done.

Thanks,

  Stoney

Re: [PATCH] x86/apic: check FADT settings after enable x2apic

[Ingo Molnar]

* Wang, Song-Bo (Stoney) <song-bo.wang@hp.com> wrote:

>  * Ingo Molnar <mingo.kernel.org@gmail.com> wrote:
> > 
> > * Yinghai Lu <yinghai@kernel.org> wrote:
> > 
> > > Please check attached.
> > 
> > Almost good.
> > 
> > This:
> > 
> >   > When some HP sytems boot without x2apic_phys, there will be
> > 
> > Should mention the approximate models of the systems affected - is it
> > just a specific line of systems, or a wider range of systems affected?
> > 
> > This will inform users and will help maintainers like me to prioritize
> > the merging and backporting of patches.
> > 
> > Thanks,
> > 
> > 	Ingo
> 
> Hi Ingo,
> 
> Due to some HW limitation, HP ProLiant DL980 G7 Server has the 
> BIT ACPI_FADT_APIC_PHYSICAL set in BIOS.
> 
> This model of systems already shipped. It is great if some 
> backporting could be done.

Ok, please re-send the last version of the patch with the model 
information included in the changelog and also include a Cc: 
<stable@kernel.org> tag before your signoff line.

Thanks,

	Ingo

[PATCH] x86, apic: Check fadt x2apic phys in x2apic_phys_probe()

[Yinghai Lu]

From: Stoney Wang <song-bo.wang@hp.com>

When HP ProLiant DL980 G7 Server boot without x2apic_phys, there will be
intermittent lost interrupts and could result in a hang or data loss.

Those systems only work with x2apic phys mode so BIOS set
ACPI_FADT_APIC_PHYSICAL in FADT table.
Also because all apicids are less then 255, BIOS need to pass the control
to the OS with xapic mode, according to x2apic-spec, chapter 2.9.

Current code handle x2apic when BIOS pass with xapic mode:

When user specify x2apic_phys, or FADT indicates PHYSICAL.
During madt oem check, apic driver is set with xapic logical or
xapic phys driver at first.
Later enable_IR_x2apic() will enable x2apic_mode.
After that, x2apic_phys_probe() will install right x2apic phys driver
if user specify x2apic_phys,
but will skip and let x2apic_cluster_probe to take over to install
wrong apic driver (x2apic cluster) when FADT indicates
PHYSICAL, because x2apic_phys_probe does not check FADT PHYSICAL.

Add checking x2apic_fadt_phys in x2apic_phys_probe() to fix the problem.

-v3: update the change according to Ingo.

[ changelog, and simplify code - Yinghai Lu ]
Signed-off-by: Stoney Wang <song-bo.wang@hp.com>
Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Cc: stable@kernel.org

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

Index: linux-2.6/arch/x86/kernel/apic/x2apic_phys.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/apic/x2apic_phys.c
+++ linux-2.6/arch/x86/kernel/apic/x2apic_phys.c
@@ -20,18 +20,19 @@ static int set_x2apic_phys_mode(char *ar
 }
 early_param("x2apic_phys", set_x2apic_phys_mode);
 
-static int x2apic_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
+static bool x2apic_fadt_phys(void)
 {
-	if (x2apic_phys)
-		return x2apic_enabled();
-	else if ((acpi_gbl_FADT.header.revision >= FADT2_REVISION_ID) &&
-		(acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL) &&
-		x2apic_enabled()) {
+	if ((acpi_gbl_FADT.header.revision >= FADT2_REVISION_ID) &&
+		(acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL)) {
 		printk(KERN_DEBUG "System requires x2apic physical mode\n");
-		return 1;
+		return true;
 	}
-	else
-		return 0;
+	return false;
+}
+
+static int x2apic_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
+{
+	return x2apic_enabled() && (x2apic_phys || x2apic_fadt_phys());
 }
 
 static void
@@ -82,7 +83,7 @@ static void init_x2apic_ldr(void)
 
 static int x2apic_phys_probe(void)
 {
-	if (x2apic_mode && x2apic_phys)
+	if (x2apic_mode && (x2apic_phys || x2apic_fadt_phys()))
 		return 1;
 
 	return apic == &apic_x2apic_phys;

Re: [PATCH] x86, apic: Check fadt x2apic phys in x2apic_phys_probe()

[Ingo Molnar]

* Yinghai Lu <yinghai@kernel.org> wrote:

> From: Stoney Wang <song-bo.wang@hp.com>
> 
> When HP ProLiant DL980 G7 Server boot without x2apic_phys, there will be
> intermittent lost interrupts and could result in a hang or data loss.

What does 'boot without x2apic_phys' mean?

Does it mean x2apic_phys=0 boot command line? Or, because 
x2apic_phys is off by default, does it simply mean that if it's 
booted with a default kernel, without any workaround specified 
on the boot command line?

Thanks,

	Ingo

Re: [PATCH] x86, apic: Check fadt x2apic phys in x2apic_phys_probe()

[Yinghai Lu]

On Tue, Feb 5, 2013 at 4:24 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Yinghai Lu <yinghai@kernel.org> wrote:
>
>> From: Stoney Wang <song-bo.wang@hp.com>
>>
>> When HP ProLiant DL980 G7 Server boot without x2apic_phys, there will be
>> intermittent lost interrupts and could result in a hang or data loss.
>
> What does 'boot without x2apic_phys' mean?
>
> Does it mean x2apic_phys=0 boot command line? Or, because
> x2apic_phys is off by default, does it simply mean that if it's
> booted with a default kernel, without any workaround specified
> on the boot command line?

means that user does not append "x2apic_phys" in boot command line.

current we have:

        x2apic_phys     [X86-64,APIC] Use x2apic physical mode instead of
                        default x2apic cluster mode on platforms
                        supporting x2apic.

int x2apic_phys;

static int set_x2apic_phys_mode(char *arg)
{
        x2apic_phys = 1;
        return 0;
}
early_param("x2apic_phys", set_x2apic_phys_mode);

if the user specify "x2apic_phys=0", kernel still enable x2apic_phys.

Now because system could enable x2apic phys mode via fadt, do we
need to make "x2apic_phys=0" works as disabling x2apic_phys and
overriding FADT ?

Thanks

Yinghai

Re: [PATCH] x86, apic: Check fadt x2apic phys in x2apic_phys_probe()

[Ingo Molnar]

* Yinghai Lu <yinghai@kernel.org> wrote:

> On Tue, Feb 5, 2013 at 4:24 AM, Ingo Molnar <mingo@kernel.org> wrote:
> >
> > * Yinghai Lu <yinghai@kernel.org> wrote:
> >
> >> From: Stoney Wang <song-bo.wang@hp.com>
> >>
> >> When HP ProLiant DL980 G7 Server boot without x2apic_phys, there will be
> >> intermittent lost interrupts and could result in a hang or data loss.
> >
> > What does 'boot without x2apic_phys' mean?
> >
> > Does it mean x2apic_phys=0 boot command line? Or, because
> > x2apic_phys is off by default, does it simply mean that if it's
> > booted with a default kernel, without any workaround specified
> > on the boot command line?
> 
> means that user does not append "x2apic_phys" in boot command line.

The user does not append a whole lot of other 
behavior-modification command line options either! There's no 
apic=0 line either. Nor smp=0.

Adding this essentially irrelevant piece of information to the 
*FIRST*, most important sentence of the changelog is thus not 
just confusing but utterly misleading. Communications 101.

Instead it should say something like:

   When a HP ProLiant DL980 G7 Server boots a regular kernel,
   there will be intermittent lost interrupts which could
   result in a hang or (in extreme cases) data loss.

   The reason is that this system only supports x2apic physical 
   mode, while the kernel boots with a logical-cluster default 
   setting.

   This bug can be worked around by specifying the x2apic_phys=1
   boot option, but we want to handle this sytem without
   requiring manual workarounds.

Right? Writing a clean changelog is like writing clean code - 
you have to learn it if you want to contribute to the kernel 
smoothly. Think of it as an engineering task: the other required 
half of modifying kernel code.

Thanks,

	Ingo

Re: [PATCH] x86, apic: Check fadt x2apic phys in x2apic_phys_probe()

[Yinghai Lu]

On Wed, Feb 6, 2013 at 5:16 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Yinghai Lu <yinghai@kernel.org> wrote:
>
>> On Tue, Feb 5, 2013 at 4:24 AM, Ingo Molnar <mingo@kernel.org> wrote:
>> >
>> > * Yinghai Lu <yinghai@kernel.org> wrote:
>> >
>> >> From: Stoney Wang <song-bo.wang@hp.com>
>> >>
>> >> When HP ProLiant DL980 G7 Server boot without x2apic_phys, there will be
>> >> intermittent lost interrupts and could result in a hang or data loss.
>> >
>> > What does 'boot without x2apic_phys' mean?
>> >
>> > Does it mean x2apic_phys=0 boot command line? Or, because
>> > x2apic_phys is off by default, does it simply mean that if it's
>> > booted with a default kernel, without any workaround specified
>> > on the boot command line?
>>
>> means that user does not append "x2apic_phys" in boot command line.
>
> The user does not append a whole lot of other
> behavior-modification command line options either! There's no
> apic=0 line either. Nor smp=0.
>
> Adding this essentially irrelevant piece of information to the
> *FIRST*, most important sentence of the changelog is thus not
> just confusing but utterly misleading. Communications 101.

Those system should not be out of the door with buggy bios.
Assume that HP would have release notes that require user to append
"x2apic_phys"

>
> Instead it should say something like:
>
>    When a HP ProLiant DL980 G7 Server boots a regular kernel,
>    there will be intermittent lost interrupts which could
>    result in a hang or (in extreme cases) data loss.
>
>    The reason is that this system only supports x2apic physical
>    mode, while the kernel boots with a logical-cluster default
>    setting.
>
>    This bug can be worked around by specifying the x2apic_phys=1
>    boot option, but we want to handle this sytem without
>    requiring manual workarounds.
>
> Right? Writing a clean changelog is like writing clean code -
> you have to learn it if you want to contribute to the kernel
> smoothly. Think of it as an engineering task: the other required
> half of modifying kernel code.

Yes. that is clean. just want to  update the workaround words to
"x2apic_phys" or "nox2apic"

   This bug can be worked around by specifying "x2apic_phys" or
   "nox2apic" boot option, but we want to handle this system without
   requiring manual workarounds.

Will continue to work on writing clean changelog.

Thanks

Yinghai

[PATCH v4] x86, apic: Check fadt x2apic phys in x2apic_phys_probe()

[Yinghai Lu]

From: Stoney Wang <song-bo.wang@hp.com>

When a HP ProLiant DL980 G7 Server boots a regular kernel,
there will be intermittent lost interrupts which could
result in a hang or (in extreme cases) data loss.

The reason is that this system only supports x2apic physical
mode, while the kernel boots with a logical-cluster default
setting.

This bug can be worked around by specifying "x2apic_phys" or
"nox2apic" boot option, but we want to handle this system without
requiring manual workarounds.

BIOS set ACPI_FADT_APIC_PHYSICAL in FADT table.
As all apicids are less than 255, BIOS need to pass the control
to the OS with xapic mode, according to x2apic-spec, chapter 2.9.

Current code handle x2apic when BIOS pass with xapic mode:
When user specify x2apic_phys, or FADT indicates PHYSICAL.
1. During madt oem check, apic driver is set with xapic logical or
   xapic phys driver at first.
2. enable_IR_x2apic() will enable x2apic_mode.
3. if user specify x2apic_phys, x2apic_phys_probe() will install
   right x2apic phys driver and use x2apic phys mode.
   otherwise will skip and let x2apic_cluster_probe to take over
   to install x2apic cluster driver (wrong one) even FADT indicates
   PHYSICAL, because x2apic_phys_probe does not check FADT PHYSICAL.

Add checking x2apic_fadt_phys in x2apic_phys_probe() to fix the problem.

-v3: update the change according to Ingo.
-v4: merge changelog from Ingo.

[ changelog, and simplify code - Yinghai Lu ]
Signed-off-by: Stoney Wang <song-bo.wang@hp.com>
Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Cc: stable@kernel.org

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

Index: linux-2.6/arch/x86/kernel/apic/x2apic_phys.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/apic/x2apic_phys.c
+++ linux-2.6/arch/x86/kernel/apic/x2apic_phys.c
@@ -20,18 +20,19 @@ static int set_x2apic_phys_mode(char *ar
 }
 early_param("x2apic_phys", set_x2apic_phys_mode);
 
-static int x2apic_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
+static bool x2apic_fadt_phys(void)
 {
-	if (x2apic_phys)
-		return x2apic_enabled();
-	else if ((acpi_gbl_FADT.header.revision >= FADT2_REVISION_ID) &&
-		(acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL) &&
-		x2apic_enabled()) {
+	if ((acpi_gbl_FADT.header.revision >= FADT2_REVISION_ID) &&
+		(acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL)) {
 		printk(KERN_DEBUG "System requires x2apic physical mode\n");
-		return 1;
+		return true;
 	}
-	else
-		return 0;
+	return false;
+}
+
+static int x2apic_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
+{
+	return x2apic_enabled() && (x2apic_phys || x2apic_fadt_phys());
 }
 
 static void
@@ -82,7 +83,7 @@ static void init_x2apic_ldr(void)
 
 static int x2apic_phys_probe(void)
 {
-	if (x2apic_mode && x2apic_phys)
+	if (x2apic_mode && (x2apic_phys || x2apic_fadt_phys()))
 		return 1;
 
 	return apic == &apic_x2apic_phys;

[tip:x86/urgent] x86/apic: Work around boot failure on HP ProLiant DL980 G7 Server systems

[tip-bot for Stoney Wang]

Commit-ID:  cb214ede7657db458fd0b2a25ea0b28dbf900ebc
Gitweb:     http://git.kernel.org/tip/cb214ede7657db458fd0b2a25ea0b28dbf900ebc
Author:     Stoney Wang <song-bo.wang@hp.com>
AuthorDate: Thu, 7 Feb 2013 10:53:02 -0800
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Mon, 11 Feb 2013 11:13:00 +0100

x86/apic: Work around boot failure on HP ProLiant DL980 G7 Server systems

When a HP ProLiant DL980 G7 Server boots a regular kernel,
there will be intermittent lost interrupts which could
result in a hang or (in extreme cases) data loss.

The reason is that this system only supports x2apic physical
mode, while the kernel boots with a logical-cluster default
setting.

This bug can be worked around by specifying the "x2apic_phys" or
"nox2apic" boot option, but we want to handle this system
without requiring manual workarounds.

The BIOS sets ACPI_FADT_APIC_PHYSICAL in FADT table.
As all apicids are smaller than 255, BIOS need to pass the
control to the OS with xapic mode, according to x2apic-spec,
chapter 2.9.

Current code handle x2apic when BIOS pass with xapic mode
enabled:

When user specifies x2apic_phys, or FADT indicates PHYSICAL:

1. During madt oem check, apic driver is set with xapic logical
   or xapic phys driver at first.

2. enable_IR_x2apic() will enable x2apic_mode.

3. if user specifies x2apic_phys on the boot line, x2apic_phys_probe()
   will install the correct x2apic phys driver and use x2apic phys mode.
   Otherwise it will skip the driver will let x2apic_cluster_probe to
   take over to install x2apic cluster driver (wrong one) even though FADT
   indicates PHYSICAL, because x2apic_phys_probe does not check
   FADT PHYSICAL.

Add checking x2apic_fadt_phys in x2apic_phys_probe() to fix the
problem.

Signed-off-by: Stoney Wang <song-bo.wang@hp.com>
[ updated the changelog and simplified the code ]
Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Cc: stable@kernel.org
Link: http://lkml.kernel.org/r/1360263182-16226-1-git-send-email-yinghai@kernel.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/apic/x2apic_phys.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/arch/x86/kernel/apic/x2apic_phys.c b/arch/x86/kernel/apic/x2apic_phys.c
index e03a1e1..562a76d 100644
--- a/arch/x86/kernel/apic/x2apic_phys.c
+++ b/arch/x86/kernel/apic/x2apic_phys.c
@@ -20,18 +20,19 @@ static int set_x2apic_phys_mode(char *arg)
 }
 early_param("x2apic_phys", set_x2apic_phys_mode);
 
-static int x2apic_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
+static bool x2apic_fadt_phys(void)
 {
-	if (x2apic_phys)
-		return x2apic_enabled();
-	else if ((acpi_gbl_FADT.header.revision >= FADT2_REVISION_ID) &&
-		(acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL) &&
-		x2apic_enabled()) {
+	if ((acpi_gbl_FADT.header.revision >= FADT2_REVISION_ID) &&
+		(acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL)) {
 		printk(KERN_DEBUG "System requires x2apic physical mode\n");
-		return 1;
+		return true;
 	}
-	else
-		return 0;
+	return false;
+}
+
+static int x2apic_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
+{
+	return x2apic_enabled() && (x2apic_phys || x2apic_fadt_phys());
 }
 
 static void
@@ -82,7 +83,7 @@ static void init_x2apic_ldr(void)
 
 static int x2apic_phys_probe(void)
 {
-	if (x2apic_mode && x2apic_phys)
+	if (x2apic_mode && (x2apic_phys || x2apic_fadt_phys()))
 		return 1;
 
 	return apic == &apic_x2apic_phys;

Re: [PATCH v4] x86, apic: Check fadt x2apic phys in x2apic_phys_probe()

[Lin-Bao Zhang]

Hi Yinghai ,
this patch has been committed into some git repository ? if yes ,
which repository ?
I just " git log arch/x86/kernel/apic/x2apic_phys.c " on
"linux-stable" , but I could not find it.
i guess maybe you will put it into another repository ,thanks very much!

Re: [PATCH v4] x86, apic: Check fadt x2apic phys in x2apic_phys_probe()

[Yinghai Lu]

On Mon, Feb 18, 2013 at 12:52 AM, Lin-Bao Zhang <2004.zhang@gmail.com> wrote:
> Hi Yinghai ,
> this patch has been committed into some git repository ? if yes ,
> which repository ?
> I just " git log arch/x86/kernel/apic/x2apic_phys.c " on
> "linux-stable" , but I could not find it.
> i guess maybe you will put it into another repository ,thanks very much!

It is already in linus's tree.

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commitdiff;h=cb214ede7657db458fd0b2a25ea0b28dbf900ebc

Stable tree should have that soon.

Thanks

Yinghai