[8/8] x86: apic - unify init_bsp_APIC
diff mbox series

Message ID 20080814184652.475361864@gmail.com
State New, archived
Headers show
Series
  • another one step toward APIC merging
Related show

Commit Message

Cyrill Gorcunov Jan. 1, 1970, midnight UTC
Remove redundant apic_read(APIC_LVR) and add check for
reserved bit on P4/Xeon in 64bit mode. For now it's
not really needed and made in a sake of unification.
#ifdef would be added probably.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---

Comments

Maciej W. Rozycki Aug. 14, 2008, 7:44 p.m. UTC | #1
On Thu, 1 Jan 1970, Cyrill Gorcunov wrote:

> @@ -962,7 +962,8 @@ void __init init_bsp_APIC(void)
>  	 */
>  	apic_write(APIC_LVT0, APIC_DM_EXTINT);
>  	value = APIC_DM_NMI;
> -	if (!lapic_is_integrated())		/* 82489DX */
> +	/* discrete on 82489DX */
> +	if (!lapic_is_integrated())
>  		value |= APIC_LVT_LEVEL_TRIGGER;
>  	apic_write(APIC_LVT1, value);
>  }

 Please elaborate.

  Maciej
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Cyrill Gorcunov Aug. 15, 2008, 6:41 a.m. UTC | #2
On Thu, Aug 14, 2008 at 11:44 PM, Maciej W. Rozycki
<macro@linux-mips.org> wrote:
> On Thu, 1 Jan 1970, Cyrill Gorcunov wrote:
>
>> @@ -962,7 +962,8 @@ void __init init_bsp_APIC(void)
>>        */
>>       apic_write(APIC_LVT0, APIC_DM_EXTINT);
>>       value = APIC_DM_NMI;
>> -     if (!lapic_is_integrated())             /* 82489DX */
>> +     /* discrete on 82489DX */
>> +     if (!lapic_is_integrated())
>>               value |= APIC_LVT_LEVEL_TRIGGER;
>>       apic_write(APIC_LVT1, value);
>>  }
>
>  Please elaborate.
>
>  Maciej
>

Hi Maciej,

don't really understand what do you mean. Do you mean it should
be a separate patch for this code snippet? Actually since we
always have lapic integrated on 64bit cpu gcc will eliminate
this check.

Cyrill
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
tip-bot for Ingo Molnar Aug. 15, 2008, 11:51 a.m. UTC | #3
* Cyrill Gorcunov <gorcunov@gmail.com> wrote:

> On Thu, Aug 14, 2008 at 11:44 PM, Maciej W. Rozycki
> <macro@linux-mips.org> wrote:
> > On Thu, 1 Jan 1970, Cyrill Gorcunov wrote:
> >
> >> @@ -962,7 +962,8 @@ void __init init_bsp_APIC(void)
> >>        */
> >>       apic_write(APIC_LVT0, APIC_DM_EXTINT);
> >>       value = APIC_DM_NMI;
> >> -     if (!lapic_is_integrated())             /* 82489DX */
> >> +     /* discrete on 82489DX */
> >> +     if (!lapic_is_integrated())
> >>               value |= APIC_LVT_LEVEL_TRIGGER;
> >>       apic_write(APIC_LVT1, value);
> >>  }
> >
> >  Please elaborate.
> >
> >  Maciej
> >
> 
> Hi Maciej,
> 
> don't really understand what do you mean. [...]

i suspect the question might have been: 'why this change'.

If that was the question, the answer would be: to unify apic_32.c and 
apic_64.c we first use tiny little changes to bring the two files in 
sync. Presumably, each such change is a NOP or at least very safe - and 
clearly bisectable in the worst-case.

In this case, something that only makes sense on 32-bit has been added 
over to the 64-bit side. The resulting apic.c file will have to have 
legacy code as well - but hopefully not too much.

Cyrill, i've applied your series to tip/x86/apic. (i have fixed the 
timestamps) Please address Maciej's feedback as well, in subsequent 
patches.

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Maciej W. Rozycki Aug. 15, 2008, 12:15 p.m. UTC | #4
On Fri, 15 Aug 2008, Ingo Molnar wrote:

> > >> @@ -962,7 +962,8 @@ void __init init_bsp_APIC(void)
> > >>        */
> > >>       apic_write(APIC_LVT0, APIC_DM_EXTINT);
> > >>       value = APIC_DM_NMI;
> > >> -     if (!lapic_is_integrated())             /* 82489DX */
> > >> +     /* discrete on 82489DX */
> > >> +     if (!lapic_is_integrated())
> > >>               value |= APIC_LVT_LEVEL_TRIGGER;
> > >>       apic_write(APIC_LVT1, value);
> > >>  }
> > >
> > >  Please elaborate.
> > >
> > >  Maciej
> > >
> > 
> > Hi Maciej,
> > 
> > don't really understand what do you mean. [...]
> 
> i suspect the question might have been: 'why this change'.

 Indeed -- the change just moves a comment around from the obvious place 
(where it means: "the condition true applies to the 82489DX") to elsewhere 
while rephrasing it in a way that makes me wonder: "What the hell is that 
meant to mean?"  Perhaps it is clearer to the others, but for me it is 
just obfuscation.

 I think the comment as it is is clear enough and is also clearly visible
when browsing through the source, when you want to spot all the odd bits
related to the discrete APIC.  This is no longer true after the change.  
And any more complete explanation belongs to the definition of
lapic_is_integrated().

> If that was the question, the answer would be: to unify apic_32.c and 
> apic_64.c we first use tiny little changes to bring the two files in 
> sync. Presumably, each such change is a NOP or at least very safe - and 
> clearly bisectable in the worst-case.

 If the obfuscation comes from apic_64.c, then the clean-up should be done
in th other direction IMO.  Not everything that comes from the 64-bit
variation is better than its 32-bit counterpart.

 Please note I do not question the semantic changes contained within this
patch, so the issue of bisectability or code bloat (I am assuming
lapic_is_integrated() expands to 0 on 64-bit; if not, this is clearly
asking for improvement) is irrelevant.

  Maciej
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
tip-bot for Ingo Molnar Aug. 15, 2008, 1:48 p.m. UTC | #5
* Maciej W. Rozycki <macro@linux-mips.org> wrote:

> > If that was the question, the answer would be: to unify apic_32.c 
> > and apic_64.c we first use tiny little changes to bring the two 
> > files in sync. Presumably, each such change is a NOP or at least 
> > very safe - and clearly bisectable in the worst-case.
> 
>  If the obfuscation comes from apic_64.c, then the clean-up should be 
> done in th other direction IMO.  Not everything that comes from the 
> 64-bit variation is better than its 32-bit counterpart.

yeah - in fact for most unifications we did the 32-bit counterpart was 
the cleaner one. (which is no big surprise, 80%-90% of the x86 Linux 
use, even today, is on the 32-bit side - so most developers watch that 
one.)

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Cyrill Gorcunov Aug. 15, 2008, 2:45 p.m. UTC | #6
[Ingo Molnar - Fri, Aug 15, 2008 at 03:48:01PM +0200]
| 
| * Maciej W. Rozycki <macro@linux-mips.org> wrote:
| 
| > > If that was the question, the answer would be: to unify apic_32.c 
| > > and apic_64.c we first use tiny little changes to bring the two 
| > > files in sync. Presumably, each such change is a NOP or at least 
| > > very safe - and clearly bisectable in the worst-case.
| > 
| >  If the obfuscation comes from apic_64.c, then the clean-up should be 
| > done in th other direction IMO.  Not everything that comes from the 
| > 64-bit variation is better than its 32-bit counterpart.
| 
| yeah - in fact for most unifications we did the 32-bit counterpart was 
| the cleaner one. (which is no big surprise, 80%-90% of the x86 Linux 
| use, even today, is on the 32-bit side - so most developers watch that 
| one.)
| 
| 	Ingo
| 

Thanks you both for review and comments!

Since lapic_is_integrated is just eliminated on 64bit I thought
it's better to have smooth code flow as much as possible. By 'smooth'
I mean to not use #ifdef until there is no chance to escape from.

I think I'll send next series if only merging procedure will be
completed.

And Maciej - you're so right - if someone will take a look on the
apic_64.c code now he will be quite wondered what lapic_is_integrated
is doing in 64bit code.

Ingo, I don't know but maybe you should drop the patches I sent since
they could (espec the snippet Maciej pointed to) confuse code reader
(sine merging process is not completed yet). Sorry for incovenience
and thanks again for review!

		- Cyrill -
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
tip-bot for Ingo Molnar Aug. 15, 2008, 2:47 p.m. UTC | #7
* Cyrill Gorcunov <gorcunov@gmail.com> wrote:

> And Maciej - you're so right - if someone will take a look on the 
> apic_64.c code now he will be quite wondered what lapic_is_integrated 
> is doing in 64bit code.
> 
> Ingo, I don't know but maybe you should drop the patches I sent since 
> they could (espec the snippet Maciej pointed to) confuse code reader 
> (sine merging process is not completed yet). Sorry for incovenience 
> and thanks again for review!

no problem with having such temporary state at all - and we want to test 
things out as well.

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Maciej W. Rozycki Aug. 15, 2008, 3:31 p.m. UTC | #8
On Fri, 15 Aug 2008, Cyrill Gorcunov wrote:

> Since lapic_is_integrated is just eliminated on 64bit I thought
> it's better to have smooth code flow as much as possible. By 'smooth'
> I mean to not use #ifdef until there is no chance to escape from.

 Of course!

> And Maciej - you're so right - if someone will take a look on the
> apic_64.c code now he will be quite wondered what lapic_is_integrated
> is doing in 64bit code.

 I just questioned the purpose of reformatting the comment.  I see no
problem with lapic_is_integrated() appearing in 64-bit code -- if someone
cannot figure out why it would appear there, they better practised some
thinking at home before coming back.  We have plenty of places with such
constructs that expand to literals for some configurations all over our
tree anyway.

  Maciej
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Cyrill Gorcunov Aug. 15, 2008, 4:35 p.m. UTC | #9
[Maciej W. Rozycki - Fri, Aug 15, 2008 at 04:31:57PM +0100]
| On Fri, 15 Aug 2008, Cyrill Gorcunov wrote:
| 
| > Since lapic_is_integrated is just eliminated on 64bit I thought
| > it's better to have smooth code flow as much as possible. By 'smooth'
| > I mean to not use #ifdef until there is no chance to escape from.
| 
|  Of course!
| 
| > And Maciej - you're so right - if someone will take a look on the
| > apic_64.c code now he will be quite wondered what lapic_is_integrated
| > is doing in 64bit code.
| 
|  I just questioned the purpose of reformatting the comment.  I see no
| problem with lapic_is_integrated() appearing in 64-bit code -- if someone
| cannot figure out why it would appear there, they better practised some
| thinking at home before coming back.  We have plenty of places with such
| constructs that expand to literals for some configurations all over our
| tree anyway.
| 
|   Maciej
| 

Ah... it was about comment,  now it's clear what you meant (for me).

		- Cyrill -
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Patch
diff mbox series

Index: linux-2.6.git/arch/x86/kernel/apic_32.c
===================================================================
--- linux-2.6.git.orig/arch/x86/kernel/apic_32.c	2008-08-14 22:23:04.000000000 +0400
+++ linux-2.6.git/arch/x86/kernel/apic_32.c	2008-08-14 22:24:49.000000000 +0400
@@ -927,7 +927,7 @@  void __init sync_Arb_IDs(void)
  */
 void __init init_bsp_APIC(void)
 {
-	unsigned long value;
+	unsigned int value;
 
 	/*
 	 * Don't do the setup now if we have a SMP BIOS as the
@@ -962,7 +962,8 @@  void __init init_bsp_APIC(void)
 	 */
 	apic_write(APIC_LVT0, APIC_DM_EXTINT);
 	value = APIC_DM_NMI;
-	if (!lapic_is_integrated())		/* 82489DX */
+	/* discrete on 82489DX */
+	if (!lapic_is_integrated())
 		value |= APIC_LVT_LEVEL_TRIGGER;
 	apic_write(APIC_LVT1, value);
 }
Index: linux-2.6.git/arch/x86/kernel/apic_64.c
===================================================================
--- linux-2.6.git.orig/arch/x86/kernel/apic_64.c	2008-08-14 22:23:04.000000000 +0400
+++ linux-2.6.git/arch/x86/kernel/apic_64.c	2008-08-14 22:24:49.000000000 +0400
@@ -776,8 +776,6 @@  void __init init_bsp_APIC(void)
 	if (smp_found_config || !cpu_has_apic)
 		return;
 
-	value = apic_read(APIC_LVR);
-
 	/*
 	 * Do not trust the local APIC being empty at bootup.
 	 */
@@ -789,7 +787,12 @@  void __init init_bsp_APIC(void)
 	value = apic_read(APIC_SPIV);
 	value &= ~APIC_VECTOR_MASK;
 	value |= APIC_SPIV_APIC_ENABLED;
-	value |= APIC_SPIV_FOCUS_DISABLED;
+	/* This bit is reserved on P4/Xeon and should be cleared */
+	if ((boot_cpu_data.x86_vendor == X86_VENDOR_INTEL) &&
+	    (boot_cpu_data.x86 == 15))
+		value &= ~APIC_SPIV_FOCUS_DISABLED;
+	else
+		value |= APIC_SPIV_FOCUS_DISABLED;
 	value |= SPURIOUS_APIC_VECTOR;
 	apic_write(APIC_SPIV, value);
 
@@ -798,6 +801,9 @@  void __init init_bsp_APIC(void)
 	 */
 	apic_write(APIC_LVT0, APIC_DM_EXTINT);
 	value = APIC_DM_NMI;
+	/* discrete on 82489DX */
+	if (!lapic_is_integrated())
+		value |= APIC_LVT_LEVEL_TRIGGER;
 	apic_write(APIC_LVT1, value);
 }