[patch 0/8] [RFC -tip] another one step toward APIC merging

[patch 0/8] [RFC -tip] another one step toward APIC merging

[Cyrill Gorcunov]

Hi,

here are the series of patches to help APIC code merging.
I've more patches in my queue (~20 for now). But I still playing
with them so they too much ugly to be even tested.

The patch with subject
	x86: apic - do not clear APIC twice in lapic_shutdown
actually is not merge-helper but rather I found a bit strange
of double clearing of APIC in 32bit code.

As always I would really appreciate any comments on this series.

The patches are over -tip/master

commit 8f87590f7c4a2164ab6330711c9a33ab59f97e25
Merge: fb71068... c2fc119...
Author: Ingo Molnar <mingo@elte.hu>
Date:   Tue Aug 12 11:22:08 2008 +0200

    Merge branch 'core/urgent'

		- Cyrill -
-- 

[patch 1/8] x86: apic - use SET_APIC_DEST_FIELD instead of hardcoded shift

[Cyrill Gorcunov]

[-- Attachment #1: x86-apic-use-SET_APIC_DEST_FIELD-instead-of-hardc.patch --]
[-- Type: text/plain, Size: 560 bytes --]

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

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 21:59:24.000000000 +0400
+++ linux-2.6.git/arch/x86/kernel/apic_64.c	2008-08-14 22:01:24.000000000 +0400
@@ -150,7 +150,7 @@ u32 safe_xapic_wait_icr_idle(void)
 
 void xapic_icr_write(u32 low, u32 id)
 {
-	apic_write(APIC_ICR2, id << 24);
+	apic_write(APIC_ICR2, SET_APIC_DEST_FIELD(id));
 	apic_write(APIC_ICR, low);
 }
 

-- 

[patch 2/8] x86: apic - unify disable_apic_timer

[Cyrill Gorcunov]

[-- Attachment #1: x86-apic-unify-disable_apic_timer.patch --]
[-- Type: text/plain, Size: 2945 bytes --]

Get rid of local_apic_timer_disabled and use disable_apic_timer instead.

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

Index: linux-2.6.git/arch/x86/kernel/apic_32.c
===================================================================
--- linux-2.6.git.orig/arch/x86/kernel/apic_32.c	2008-08-12 22:45:09.000000000 +0400
+++ linux-2.6.git/arch/x86/kernel/apic_32.c	2008-08-12 22:46:16.000000000 +0400
@@ -63,7 +63,7 @@ int disable_apic;
 /* Local APIC timer verification ok */
 static int local_apic_timer_verify_ok;
 /* Disable local APIC timer from the kernel commandline or via dmi quirk */
-static int local_apic_timer_disabled;
+static int disable_apic_timer __cpuinitdata;
 /* Local APIC timer works in C2 */
 int local_apic_timer_c2_ok;
 EXPORT_SYMBOL_GPL(local_apic_timer_c2_ok);
@@ -574,7 +574,7 @@ void __init setup_boot_APIC_clock(void)
 	 * timer as a dummy clock event source on SMP systems, so the
 	 * broadcast mechanism is used. On UP systems simply ignore it.
 	 */
-	if (local_apic_timer_disabled) {
+	if (disable_apic_timer) {
 		/* No broadcast on UP ! */
 		if (num_possible_cpus() > 1) {
 			lapic_clockevent.mult = 1;
@@ -1703,12 +1703,19 @@ static int __init parse_nolapic(char *ar
 }
 early_param("nolapic", parse_nolapic);
 
-static int __init parse_disable_lapic_timer(char *arg)
+static int __init parse_disable_apic_timer(char *arg)
 {
-	local_apic_timer_disabled = 1;
+	disable_apic_timer = 1;
 	return 0;
 }
-early_param("nolapic_timer", parse_disable_lapic_timer);
+early_param("noapictimer", parse_disable_apic_timer);
+
+static int __init parse_nolapic_timer(char *arg)
+{
+	disable_apic_timer = 1;
+	return 0;
+}
+early_param("nolapic_timer", parse_nolapic_timer);
 
 static int __init parse_lapic_timer_c2_ok(char *arg)
 {
Index: linux-2.6.git/arch/x86/kernel/apic_64.c
===================================================================
--- linux-2.6.git.orig/arch/x86/kernel/apic_64.c	2008-08-12 22:45:37.000000000 +0400
+++ linux-2.6.git/arch/x86/kernel/apic_64.c	2008-08-12 22:46:16.000000000 +0400
@@ -45,6 +45,7 @@
 #include <mach_ipi.h>
 #include <mach_apic.h>
 
+/* Disable local APIC timer from the kernel commandline or via dmi quirk */
 static int disable_apic_timer __cpuinitdata;
 static int apic_calibrate_pmtmr __initdata;
 int disable_apic;
@@ -1587,14 +1588,19 @@ static int __init parse_lapic_timer_c2_o
 }
 early_param("lapic_timer_c2_ok", parse_lapic_timer_c2_ok);
 
-static __init int setup_noapictimer(char *str)
+static int __init parse_disable_apic_timer(char *arg)
 {
-	if (str[0] != ' ' && str[0] != 0)
-		return 0;
 	disable_apic_timer = 1;
-	return 1;
+	return 0;
+}
+early_param("noapictimer", parse_disable_apic_timer);
+
+static int __init parse_nolapic_timer(char *arg)
+{
+	disable_apic_timer = 1;
+	return 0;
 }
-__setup("noapictimer", setup_noapictimer);
+early_param("nolapic_timer", parse_nolapic_timer);
 
 static __init int setup_apicpmtimer(char *s)
 {

-- 

[patch 3/8] x86: apic - unify __setup_APIC_LVTT

[Cyrill Gorcunov]

[-- Attachment #1: x86-apic-unify-__setup_APIC_LVTT.patch --]
[-- Type: text/plain, Size: 1752 bytes --]

To be able to unify this function we RE-introduce
APIC_DIVISOR for 64bit mode. This snipped was eliminated
in some time ago in a sake of clenup but now we need it
again since it allow up to get rid of #ifdef(s).

And lapic_is_integrated call is added in apic_64.c but
since we always have APIC integrated on 64bit cpu compiler
will ignore this call.

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

diff --git a/arch/x86/kernel/apic_64.c b/arch/x86/kernel/apic_64.c
index 1e0b5d9..5d07689 100644
--- a/arch/x86/kernel/apic_64.c
+++ b/arch/x86/kernel/apic_64.c
@@ -244,6 +244,9 @@ int lapic_get_maxlvt(void)
 	return APIC_INTEGRATED(GET_APIC_VERSION(v)) ? GET_APIC_MAXLVT(v) : 2;
 }
 
+/* Clock divisor is set to 1 */
+#define APIC_DIVISOR 1
+
 /*
  * This function sets up the local APIC timer, with a timeout of
  * 'clocks' APIC bus clock. During calibration we actually call
@@ -262,6 +265,9 @@ static void __setup_APIC_LVTT(unsigned int clocks, int oneshot, int irqen)
 	lvtt_value = LOCAL_TIMER_VECTOR;
 	if (!oneshot)
 		lvtt_value |= APIC_LVT_TIMER_PERIODIC;
+	if (!lapic_is_integrated())
+		lvtt_value |= SET_APIC_TIMER_BASE(APIC_TIMER_BASE_DIV);
+
 	if (!irqen)
 		lvtt_value |= APIC_LVT_MASKED;
 
@@ -276,7 +282,7 @@ static void __setup_APIC_LVTT(unsigned int clocks, int oneshot, int irqen)
 				| APIC_TDR_DIV_16);
 
 	if (!oneshot)
-		apic_write(APIC_TMICT, clocks);
+		apic_write(APIC_TMICT, clocks / APIC_DIVISOR);
 }
 
 /*
@@ -450,7 +456,7 @@ static int __init calibrate_APIC_clock(void)
 	lapic_clockevent.min_delta_ns =
 		clockevent_delta2ns(0xF, &lapic_clockevent);
 
-	calibration_result = result / HZ;
+	calibration_result = (result * APIC_DIVISOR) / HZ;
 
 	/*
 	 * Do a sanity check on the APIC calibration result

-- 

[patch 4/8] x86: apic - do not clear APIC twice in lapic_shutdown

[Cyrill Gorcunov]

[-- Attachment #1: x86-apic-lapic_shutdown-don-t-clear-APIC-twice.patch --]
[-- Type: text/plain, Size: 653 bytes --]

There is no need to clear APIC twice since
disable_local_APIC will clear it anyway.

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

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:03:28.000000000 +0400
+++ linux-2.6.git/arch/x86/kernel/apic_32.c	2008-08-14 22:16:01.000000000 +0400
@@ -834,10 +834,11 @@ void lapic_shutdown(void)
 		return;
 
 	local_irq_save(flags);
-	clear_local_APIC();
 
 	if (enabled_via_apicbase)
 		disable_local_APIC();
+	else
+		clear_local_APIC();
 
 	local_irq_restore(flags);
 }

-- 

[patch 5/8] x86: apic - get rid of local_apic_timer_verify_ok

[Cyrill Gorcunov]

[-- Attachment #1: x86-apic-get-rid-of-local_apic_timer_verify_ok.patch --]
[-- Type: text/plain, Size: 1775 bytes --]

We are able to use clock_event_device as it's done in
64bit apic code so lets get rid of local_apic_timer_verify_ok
variable.

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

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:16:01.000000000 +0400
+++ linux-2.6.git/arch/x86/kernel/apic_32.c	2008-08-14 22:18:00.000000000 +0400
@@ -60,8 +60,6 @@ unsigned long mp_lapic_addr;
 static int force_enable_local_apic;
 int disable_apic;
 
-/* Local APIC timer verification ok */
-static int local_apic_timer_verify_ok;
 /* Disable local APIC timer from the kernel commandline or via dmi quirk */
 static int disable_apic_timer __cpuinitdata;
 /* Local APIC timer works in C2 */
@@ -301,7 +299,7 @@ static void lapic_timer_setup(enum clock
 	unsigned int v;
 
 	/* Lapic used for broadcast ? */
-	if (!local_apic_timer_verify_ok)
+	if (evt->features & CLOCK_EVT_FEAT_DUMMY)
 		return;
 
 	local_irq_save(flags);
@@ -514,7 +512,7 @@ static int __init calibrate_APIC_clock(v
 		return -1;
 	}
 
-	local_apic_timer_verify_ok = 1;
+	levt->features &= ~CLOCK_EVT_FEAT_DUMMY;
 
 	/* We trust the pm timer based calibration */
 	if (!pm_referenced) {
@@ -548,11 +546,11 @@ static int __init calibrate_APIC_clock(v
 		if (deltaj >= LAPIC_CAL_LOOPS-2 && deltaj <= LAPIC_CAL_LOOPS+2)
 			apic_printk(APIC_VERBOSE, "... jiffies result ok\n");
 		else
-			local_apic_timer_verify_ok = 0;
+			levt->features |= CLOCK_EVT_FEAT_DUMMY;
 	} else
 		local_irq_enable();
 
-	if (!local_apic_timer_verify_ok) {
+	if (levt->features & CLOCK_EVT_FEAT_DUMMY) {
 		printk(KERN_WARNING
 		       "APIC timer disabled due to verification failure.\n");
 			return -1;

-- 

[patch 6/8] x86: apic - unify verify_local_APIC

[Cyrill Gorcunov]

[-- Attachment #1: 0010-x86-apic-unify-verify_local_APIC.patch --]
[-- Type: text/plain, Size: 756 bytes --]

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

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:18:00.000000000 +0400
+++ linux-2.6.git/arch/x86/kernel/apic_32.c	2008-08-14 22:21:49.000000000 +0400
@@ -882,6 +882,12 @@ int __init verify_local_APIC(void)
 	 */
 	reg0 = apic_read(APIC_ID);
 	apic_printk(APIC_DEBUG, "Getting ID: %x\n", reg0);
+	apic_write(APIC_ID, reg0 ^ APIC_ID_MASK);
+	reg1 = apic_read(APIC_ID);
+	apic_printk(APIC_DEBUG, "Getting ID: %x\n", reg1);
+	apic_write(APIC_ID, reg0);
+	if (reg1 != (reg0 ^ APIC_ID_MASK))
+		return 0;
 
 	/*
 	 * The next two are just to see if we have sane values.

-- 

[patch 7/8] x86: apic - unify sync_Arb_IDs

[Cyrill Gorcunov]

[-- Attachment #1: 0011-x86-apic-unify-sync_Arb_IDs.patch --]
[-- Type: text/plain, Size: 1387 bytes --]

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

diff --git a/arch/x86/kernel/apic_32.c b/arch/x86/kernel/apic_32.c
index 1d5f830..ec0f18f 100644
--- a/arch/x86/kernel/apic_32.c
+++ b/arch/x86/kernel/apic_32.c
@@ -919,8 +919,7 @@ void __init sync_Arb_IDs(void)
 	apic_wait_icr_idle();
 
 	apic_printk(APIC_DEBUG, "Synchronizing Arb IDs.\n");
-	apic_write(APIC_ICR,
-		   APIC_DEST_ALLINC | APIC_INT_LEVELTRIG | APIC_DM_INIT);
+	apic_write(APIC_ICR, APIC_DEST_ALLINC | APIC_INT_LEVELTRIG | APIC_DM_INIT);
 }
 
 /*
diff --git a/arch/x86/kernel/apic_64.c b/arch/x86/kernel/apic_64.c
index 5d07689..f8d2918 100644
--- a/arch/x86/kernel/apic_64.c
+++ b/arch/x86/kernel/apic_64.c
@@ -746,8 +746,11 @@ int __init verify_local_APIC(void)
  */
 void __init sync_Arb_IDs(void)
 {
-	/* Unsupported on P4 - see Intel Dev. Manual Vol. 3, Ch. 8.6.1 */
-	if (modern_apic())
+	/*
+	 * Unsupported on P4 - see Intel Dev. Manual Vol. 3, Ch. 8.6.1 And not
+	 * needed on AMD.
+	 */
+	if (modern_apic() || boot_cpu_data.x86_vendor == X86_VENDOR_AMD)
 		return;
 
 	/*
@@ -756,8 +759,7 @@ void __init sync_Arb_IDs(void)
 	apic_wait_icr_idle();
 
 	apic_printk(APIC_DEBUG, "Synchronizing Arb IDs.\n");
-	apic_write(APIC_ICR, APIC_DEST_ALLINC | APIC_INT_LEVELTRIG
-				| APIC_DM_INIT);
+	apic_write(APIC_ICR, APIC_DEST_ALLINC | APIC_INT_LEVELTRIG | APIC_DM_INIT);
 }
 
 /*
-- 
1.6.0.rc1.34.g0fe8c

-- 

Re: [patch 7/8] x86: apic - unify sync_Arb_IDs

[Maciej W. Rozycki]

On Thu, 1 Jan 1970, Cyrill Gorcunov wrote:

 Hmm, are you living in the past?

> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
> 
> diff --git a/arch/x86/kernel/apic_32.c b/arch/x86/kernel/apic_32.c
> index 1d5f830..ec0f18f 100644
> --- a/arch/x86/kernel/apic_32.c
> +++ b/arch/x86/kernel/apic_32.c
> @@ -919,8 +919,7 @@ void __init sync_Arb_IDs(void)
>  	apic_wait_icr_idle();
>  
>  	apic_printk(APIC_DEBUG, "Synchronizing Arb IDs.\n");
> -	apic_write(APIC_ICR,
> -		   APIC_DEST_ALLINC | APIC_INT_LEVELTRIG | APIC_DM_INIT);
> +	apic_write(APIC_ICR, APIC_DEST_ALLINC | APIC_INT_LEVELTRIG | APIC_DM_INIT);
>  }
>  
>  /*
[etc...]

 Well, I think this one is backwards...  There are exceptions, but the
general rule is to with within the width of the terminal.

  Maciej

Re: [patch 7/8] x86: apic - unify sync_Arb_IDs

[Cyrill Gorcunov]

That happend sendmail/quilt sent mail by past :(. I'll fix and resend
tomorrow. thanks.

On 8/14/08, Maciej W. Rozycki <macro@linux-mips.org> wrote:
> On Thu, 1 Jan 1970, Cyrill Gorcunov wrote:
>
>  Hmm, are you living in the past?
>
>> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
>> ---
>>
>> diff --git a/arch/x86/kernel/apic_32.c b/arch/x86/kernel/apic_32.c
>> index 1d5f830..ec0f18f 100644
>> --- a/arch/x86/kernel/apic_32.c
>> +++ b/arch/x86/kernel/apic_32.c
>> @@ -919,8 +919,7 @@ void __init sync_Arb_IDs(void)
>>  	apic_wait_icr_idle();
>>
>>  	apic_printk(APIC_DEBUG, "Synchronizing Arb IDs.\n");
>> -	apic_write(APIC_ICR,
>> -		   APIC_DEST_ALLINC | APIC_INT_LEVELTRIG | APIC_DM_INIT);
>> +	apic_write(APIC_ICR, APIC_DEST_ALLINC | APIC_INT_LEVELTRIG |
>> APIC_DM_INIT);
>>  }
>>
>>  /*
> [etc...]
>
>  Well, I think this one is backwards...  There are exceptions, but the
> general rule is to with within the width of the terminal.
>
>   Maciej
>

[patch 8/8] x86: apic - unify init_bsp_APIC

[Cyrill Gorcunov]

[-- Attachment #1: 0012-x86-apic-unify-init_bsp_APIC.patch --]
[-- Type: text/plain, Size: 2260 bytes --]

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

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

-- 

Re: [patch 8/8] x86: apic - unify init_bsp_APIC

[Maciej W. Rozycki]

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

Re: [patch 8/8] x86: apic - unify init_bsp_APIC

[Cyrill Gorcunov]

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

Re: [patch 8/8] x86: apic - unify init_bsp_APIC

[Ingo Molnar]

* 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

Re: [patch 8/8] x86: apic - unify init_bsp_APIC

[Maciej W. Rozycki]

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

Re: [patch 8/8] x86: apic - unify init_bsp_APIC

[Ingo Molnar]

* 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

Re: [patch 8/8] x86: apic - unify init_bsp_APIC

[Cyrill Gorcunov]

[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 -

Re: [patch 8/8] x86: apic - unify init_bsp_APIC

[Ingo Molnar]

* 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

Re: [patch 8/8] x86: apic - unify init_bsp_APIC

[Maciej W. Rozycki]

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

Re: [patch 8/8] x86: apic - unify init_bsp_APIC

[Cyrill Gorcunov]

[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 -