2008-10-22 14:00:28

by Cyrill Gorcunov

[permalink] [raw]
Subject: [PATCH -tip] x86: do_boot_cpu - check if we have ESR register

We should touch ESR register if only we have it.
The patch fixes the problem mentoined here

http://lkml.org/lkml/2008/10/17/147

Signed-off-by: Cyrill Gorcunov <[email protected]>
CC: Glauber Costa <[email protected]>
---

arch/x86/kernel/smpboot.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

Index: linux-2.6.git/arch/x86/kernel/smpboot.c
===================================================================
--- linux-2.6.git.orig/arch/x86/kernel/smpboot.c 2008-10-21 20:35:27.000000000 +0400
+++ linux-2.6.git/arch/x86/kernel/smpboot.c 2008-10-22 17:45:28.000000000 +0400
@@ -894,8 +894,10 @@ do_rest:
/*
* Be paranoid about clearing APIC errors.
*/
- apic_write(APIC_ESR, 0);
- apic_read(APIC_ESR);
+ if (APIC_INTEGRATED(apic_version[phys_apicid])) {
+ apic_write(APIC_ESR, 0);
+ apic_read(APIC_ESR);
+ }
}

/*


2008-10-22 14:15:52

by Ingo Molnar

[permalink] [raw]
Subject: Re: [PATCH -tip] x86: do_boot_cpu - check if we have ESR register


* Cyrill Gorcunov <[email protected]> wrote:

> We should touch ESR register if only we have it.
> The patch fixes the problem mentoined here
>
> http://lkml.org/lkml/2008/10/17/147
>
> Signed-off-by: Cyrill Gorcunov <[email protected]>
> CC: Glauber Costa <[email protected]>

> - apic_write(APIC_ESR, 0);
> - apic_read(APIC_ESR);
> + if (APIC_INTEGRATED(apic_version[phys_apicid])) {
> + apic_write(APIC_ESR, 0);
> + apic_read(APIC_ESR);
> + }

i'm wondering - is the server there really that old, that it has no
integrated lapic? I.e. it's an i486 SMP box or so? Or perhaps some
other, weird SMP box?

Ingo

2008-10-22 14:17:28

by Glauber Costa

[permalink] [raw]
Subject: Re: [PATCH -tip] x86: do_boot_cpu - check if we have ESR register

On Wed, Oct 22, 2008 at 12:13 PM, Ingo Molnar <[email protected]> wrote:
>
> * Cyrill Gorcunov <[email protected]> wrote:
>
>> We should touch ESR register if only we have it.
>> The patch fixes the problem mentoined here
>>
>> http://lkml.org/lkml/2008/10/17/147
>>
>> Signed-off-by: Cyrill Gorcunov <[email protected]>
>> CC: Glauber Costa <[email protected]>
>
>> - apic_write(APIC_ESR, 0);
>> - apic_read(APIC_ESR);
>> + if (APIC_INTEGRATED(apic_version[phys_apicid])) {
>> + apic_write(APIC_ESR, 0);
>> + apic_read(APIC_ESR);
>> + }
>
> i'm wondering - is the server there really that old, that it has no
> integrated lapic? I.e. it's an i486 SMP box or so? Or perhaps some
> other, weird SMP box?

It's a Xeon, so probably just weird.

>
> Ingo
>



--
Glauber Costa.
"Free as in Freedom"
http://glommer.net

"The less confident you are, the more serious you have to act."

2008-10-22 14:17:41

by Cyrill Gorcunov

[permalink] [raw]
Subject: Re: [PATCH -tip] x86: do_boot_cpu - check if we have ESR register

[Ingo Molnar - Wed, Oct 22, 2008 at 04:13:54PM +0200]
|
| * Cyrill Gorcunov <[email protected]> wrote:
|
| > We should touch ESR register if only we have it.
| > The patch fixes the problem mentoined here
| >
| > http://lkml.org/lkml/2008/10/17/147
| >
| > Signed-off-by: Cyrill Gorcunov <[email protected]>
| > CC: Glauber Costa <[email protected]>
|
| > - apic_write(APIC_ESR, 0);
| > - apic_read(APIC_ESR);
| > + if (APIC_INTEGRATED(apic_version[phys_apicid])) {
| > + apic_write(APIC_ESR, 0);
| > + apic_read(APIC_ESR);
| > + }
|
| i'm wondering - is the server there really that old, that it has no
| integrated lapic? I.e. it's an i486 SMP box or so? Or perhaps some
| other, weird SMP box?
|
| Ingo
|

I was quite wondering too -- it's Xeon server.

>From http://lkml.org/lkml/2008/10/20/34
>> Hardware: Compaq P4 Xeon server, Broadcom CMIC-WS / CIOB-X2 board.
>> Tell me if you need more detailed information.

- Cyrill -

2008-10-22 14:23:29

by Ingo Molnar

[permalink] [raw]
Subject: Re: [PATCH -tip] x86: do_boot_cpu - check if we have ESR register


* Cyrill Gorcunov <[email protected]> wrote:

> | > - apic_write(APIC_ESR, 0);
> | > - apic_read(APIC_ESR);
> | > + if (APIC_INTEGRATED(apic_version[phys_apicid])) {
> | > + apic_write(APIC_ESR, 0);
> | > + apic_read(APIC_ESR);
> | > + }
> |
> | i'm wondering - is the server there really that old, that it has no
> | integrated lapic? I.e. it's an i486 SMP box or so? Or perhaps some
> | other, weird SMP box?
> |
> | Ingo
> |
>
> I was quite wondering too -- it's Xeon server.
>
> >From http://lkml.org/lkml/2008/10/20/34
> >> Hardware: Compaq P4 Xeon server, Broadcom CMIC-WS / CIOB-X2 board.
> >> Tell me if you need more detailed information.

ah, Compaq - how many CPUs does that box support? If it's 8 or more then
perhaps they turned off the real local APIC, fudged some chipset glue to
emulate APIC functionality and thus were able to use 8 or more of these
chips? The built-in lapic would only go up to 4 CPUs. (or maybe even
just up to dual, depending on the model)

This reminds us that all the is-integrated-lapic checks still matter
today.

Ingo

2008-10-22 14:52:54

by Cyrill Gorcunov

[permalink] [raw]
Subject: Re: [PATCH -tip] x86: do_boot_cpu - check if we have ESR register

[Ingo Molnar - Wed, Oct 22, 2008 at 04:23:12PM +0200]
|
| * Cyrill Gorcunov <[email protected]> wrote:
|
| > | > - apic_write(APIC_ESR, 0);
| > | > - apic_read(APIC_ESR);
| > | > + if (APIC_INTEGRATED(apic_version[phys_apicid])) {
| > | > + apic_write(APIC_ESR, 0);
| > | > + apic_read(APIC_ESR);
| > | > + }
| > |
| > | i'm wondering - is the server there really that old, that it has no
| > | integrated lapic? I.e. it's an i486 SMP box or so? Or perhaps some
| > | other, weird SMP box?
| > |
| > | Ingo
| > |
| >
| > I was quite wondering too -- it's Xeon server.
| >
| > >From http://lkml.org/lkml/2008/10/20/34
| > >> Hardware: Compaq P4 Xeon server, Broadcom CMIC-WS / CIOB-X2 board.
| > >> Tell me if you need more detailed information.
|
| ah, Compaq - how many CPUs does that box support? If it's 8 or more then
| perhaps they turned off the real local APIC, fudged some chipset glue to
| emulate APIC functionality and thus were able to use 8 or more of these
| chips? The built-in lapic would only go up to 4 CPUs. (or maybe even
| just up to dual, depending on the model)

I have no glue how many cpus it does supposrt :) Maybe Max know.

|
| This reminds us that all the is-integrated-lapic checks still matter
| today.
|
| Ingo
|
- Cyrill -

2008-10-22 14:59:44

by Ingo Molnar

[permalink] [raw]
Subject: Re: [PATCH -tip] x86: do_boot_cpu - check if we have ESR register


> > | > - apic_write(APIC_ESR, 0);
> > | > - apic_read(APIC_ESR);
> > | > + if (APIC_INTEGRATED(apic_version[phys_apicid])) {
> > | > + apic_write(APIC_ESR, 0);
> > | > + apic_read(APIC_ESR);
> > | > + }

this wouldnt build with certain configs:

arch/x86/kernel/smpboot.c: In function 'do_boot_cpu':
arch/x86/kernel/smpboot.c:897: error: 'phys_apicid' undeclared (first use in this function)
arch/x86/kernel/smpboot.c:897: error: (Each undeclared identifier is reported only once
arch/x86/kernel/smpboot.c:897: error: for each function it appears in.)
include/asm/io_32.h: In function 'memcpy_fromio':

because phys_apicid is not available in the !WAKE_SECONDARY_VIA_INIT
case. Changed it to boot_cpu_physical_apicid - that should be good
enough.

Ingo

2008-10-22 15:03:13

by Cyrill Gorcunov

[permalink] [raw]
Subject: Re: [PATCH -tip] x86: do_boot_cpu - check if we have ESR register

[Ingo Molnar - Wed, Oct 22, 2008 at 04:59:09PM +0200]
|
| > > | > - apic_write(APIC_ESR, 0);
| > > | > - apic_read(APIC_ESR);
| > > | > + if (APIC_INTEGRATED(apic_version[phys_apicid])) {
| > > | > + apic_write(APIC_ESR, 0);
| > > | > + apic_read(APIC_ESR);
| > > | > + }
|
| this wouldnt build with certain configs:
|
| arch/x86/kernel/smpboot.c: In function 'do_boot_cpu':
| arch/x86/kernel/smpboot.c:897: error: 'phys_apicid' undeclared (first use in this function)
| arch/x86/kernel/smpboot.c:897: error: (Each undeclared identifier is reported only once
| arch/x86/kernel/smpboot.c:897: error: for each function it appears in.)
| include/asm/io_32.h: In function 'memcpy_fromio':
|
| because phys_apicid is not available in the !WAKE_SECONDARY_VIA_INIT
| case. Changed it to boot_cpu_physical_apicid - that should be good
| enough.
|
| Ingo
|

Did you fixed it? (oh... actually I've been compilling the patch
but had this CONFIG turned on and didn't catch this issue)

- Cyrill -

2008-10-22 15:04:39

by Ingo Molnar

[permalink] [raw]
Subject: Re: [PATCH -tip] x86: do_boot_cpu - check if we have ESR register


* Cyrill Gorcunov <[email protected]> wrote:

> [Ingo Molnar - Wed, Oct 22, 2008 at 04:59:09PM +0200]
> |
> | > > | > - apic_write(APIC_ESR, 0);
> | > > | > - apic_read(APIC_ESR);
> | > > | > + if (APIC_INTEGRATED(apic_version[phys_apicid])) {
> | > > | > + apic_write(APIC_ESR, 0);
> | > > | > + apic_read(APIC_ESR);
> | > > | > + }
> |
> | this wouldnt build with certain configs:
> |
> | arch/x86/kernel/smpboot.c: In function 'do_boot_cpu':
> | arch/x86/kernel/smpboot.c:897: error: 'phys_apicid' undeclared (first use in this function)
> | arch/x86/kernel/smpboot.c:897: error: (Each undeclared identifier is reported only once
> | arch/x86/kernel/smpboot.c:897: error: for each function it appears in.)
> | include/asm/io_32.h: In function 'memcpy_fromio':
> |
> | because phys_apicid is not available in the !WAKE_SECONDARY_VIA_INIT
> | case. Changed it to boot_cpu_physical_apicid - that should be good
> | enough.
> |
> | Ingo
> |
>
> Did you fixed it? (oh... actually I've been compilling the patch
> but had this CONFIG turned on and didn't catch this issue)

yeah, fixed it up, dont worry about it.

Ingo

2008-10-22 15:06:48

by Cyrill Gorcunov

[permalink] [raw]
Subject: Re: [PATCH -tip] x86: do_boot_cpu - check if we have ESR register

[Ingo Molnar - Wed, Oct 22, 2008 at 05:04:16PM +0200]
|
| * Cyrill Gorcunov <[email protected]> wrote:
|
| > [Ingo Molnar - Wed, Oct 22, 2008 at 04:59:09PM +0200]
| > |
| > | > > | > - apic_write(APIC_ESR, 0);
| > | > > | > - apic_read(APIC_ESR);
| > | > > | > + if (APIC_INTEGRATED(apic_version[phys_apicid])) {
| > | > > | > + apic_write(APIC_ESR, 0);
| > | > > | > + apic_read(APIC_ESR);
| > | > > | > + }
| > |
| > | this wouldnt build with certain configs:
| > |
| > | arch/x86/kernel/smpboot.c: In function 'do_boot_cpu':
| > | arch/x86/kernel/smpboot.c:897: error: 'phys_apicid' undeclared (first use in this function)
| > | arch/x86/kernel/smpboot.c:897: error: (Each undeclared identifier is reported only once
| > | arch/x86/kernel/smpboot.c:897: error: for each function it appears in.)
| > | include/asm/io_32.h: In function 'memcpy_fromio':
| > |
| > | because phys_apicid is not available in the !WAKE_SECONDARY_VIA_INIT
| > | case. Changed it to boot_cpu_physical_apicid - that should be good
| > | enough.
| > |
| > | Ingo
| > |
| >
| > Did you fixed it? (oh... actually I've been compilling the patch
| > but had this CONFIG turned on and didn't catch this issue)
|
| yeah, fixed it up, dont worry about it.
|
| Ingo
|

Thanks Ingo!

- Cyrill -

2008-10-22 15:20:12

by Max Kellermann

[permalink] [raw]
Subject: Re: [PATCH -tip] x86: do_boot_cpu - check if we have ESR register

On 2008/10/22 16:23, Ingo Molnar <[email protected]> wrote:
> ah, Compaq - how many CPUs does that box support? If it's 8 or more then
> perhaps they turned off the real local APIC, fudged some chipset glue to
> emulate APIC functionality and thus were able to use 8 or more of these
> chips? The built-in lapic would only go up to 4 CPUs. (or maybe even
> just up to dual, depending on the model)

No, it's a rather small ProLiant server. 2 physical CPUs, maybe it
supports up to 4, but no more (each with HT). I have attached its
dmesg.

Max


Attachments:
(No filename) (546.00 B)
dmesg (17.08 kB)
Download all attachments

2008-10-22 15:29:53

by Maciej W. Rozycki

[permalink] [raw]
Subject: Re: [PATCH -tip] x86: do_boot_cpu - check if we have ESR register

On Wed, 22 Oct 2008, Cyrill Gorcunov wrote:

> We should touch ESR register if only we have it.
> The patch fixes the problem mentoined here
>
> http://lkml.org/lkml/2008/10/17/147
>
> Signed-off-by: Cyrill Gorcunov <[email protected]>
> CC: Glauber Costa <[email protected]>
> ---

Acked-by: Maciej W. Rozycki <[email protected]>

Note this is one of the few places you can actually write to the ESR
unconditionally -- there is another similar piece of code elsewhere in
this file which could get adjusted to look the same as this one after your
change. Thanks for this fix.

Maciej

2008-10-22 15:41:19

by Maciej W. Rozycki

[permalink] [raw]
Subject: Re: [PATCH -tip] x86: do_boot_cpu - check if we have ESR register

On Wed, 22 Oct 2008, Ingo Molnar wrote:

> i'm wondering - is the server there really that old, that it has no
> integrated lapic? I.e. it's an i486 SMP box or so? Or perhaps some
> other, weird SMP box?

I've been wondering about that too, though the fix is correct either way.

Note that all the discrete APIC boxes we have ever supported were
Pentium-based, either the original P5 (i.e. 60/66MHz) chips which had no
APIC on chip at all or P54C ones but with the integrated APIC disabled
(because the systems were 4-way SMP or suchlike and there was no suitable
chipset component to provide a compatible I/O APIC).

All the documentation for i486 SMP boxes I was able to track down
indicated that even if APIC chips were used with them, the whole setup did
not support the MP table as defined by the MultiProcessor Specification
and therefore could not be supported by Linux as it is. And I have never
come across a specimen to look into it any further.

Maciej

2008-10-22 15:42:58

by Maciej W. Rozycki

[permalink] [raw]
Subject: Re: [PATCH -tip] x86: do_boot_cpu - check if we have ESR register

On Wed, 22 Oct 2008, Glauber Costa wrote:

> It's a Xeon, so probably just weird.

The change would not affect it in any way then, because the condition is
true.

Maciej

2008-10-22 15:52:51

by Maciej W. Rozycki

[permalink] [raw]
Subject: Re: [PATCH -tip] x86: do_boot_cpu - check if we have ESR register

On Wed, 22 Oct 2008, Ingo Molnar wrote:

> ah, Compaq - how many CPUs does that box support? If it's 8 or more then
> perhaps they turned off the real local APIC, fudged some chipset glue to
> emulate APIC functionality and thus were able to use 8 or more of these
> chips? The built-in lapic would only go up to 4 CPUs. (or maybe even
> just up to dual, depending on the model)

Hmm, interesting. The integrated APIC should support up to 14 CPUs
(assuming one ID is needed for the I/O APIC). One cannot preclude silicon
errata affecting the less common hardware configurations though. It would
be interesting to see a full bootstrap log with all the APIC debug
information enabled. That would probably clarify matters a bit.

Note that Compaq would have to come with their own solution about the
discrete APIC as the 82489DX went out of production many years ago -- ten
at the very least. OTOH, they could have stockpiled them back then. ;)
This is one of the few manufacturers I would actually suspect doing so as
they always seemed keen on going SMP. Good to know we can support these
boxes at all these days.

> This reminds us that all the is-integrated-lapic checks still matter
> today.

Quite surprising indeed.

Maciej