Skip to contentSkip to conversation

another one step toward APIC merging

RFC PATCH In review Cyrill Gorcunov 3 participants 20 messages Aug 14, 2008 – Aug 15, 2008 x86

lore
Share & tools
lore

Cyrill Gorcunov

author
#0 lore raw

cover another one step toward APIC merging

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 <[email protected]>
Date: Tue Aug 12 11:22:08 2008 +0200

Merge branch 'core/urgent'

- Cyrill -

author
#1 lore raw

PATCH 1/8 x86: apic - use SET_APIC_DEST_FIELD instead of hardcoded shift

Signed-off-by Cyrill Gorcunov <[email protected]>

---

arch/x86/kernel/apic_64.c +1 -1
Index: linux-2.6.git/arch/x86/kernel/apic_64.c
===================================================================
@@ -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);
 }
author
#2 lore raw

PATCH 2/8 x86: apic - unify disable_apic_timer

In reply to Cyrill Gorcunov

Get rid of local_apic_timer_disabled and use disable_apic_timer instead.

Signed-off-by Cyrill Gorcunov <[email protected]>

---

arch/x86/kernel/apic_32.c +12 -5
Index: linux-2.6.git/arch/x86/kernel/apic_32.c
===================================================================
@@ -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)
 {
arch/x86/kernel/apic_64.c +11 -5
Index: linux-2.6.git/arch/x86/kernel/apic_64.c
===================================================================
@@ -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)
 {
author
#3 lore raw

PATCH 3/8 x86: apic - unify __setup_APIC_LVTT

In reply to Cyrill Gorcunov

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 <[email protected]>

---

arch/x86/kernel/apic_64.c +8 -2
index 1e0b5d9..5d07689 100644
@@ -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
author
#4 lore raw

PATCH 4/8 x86: apic - do not clear APIC twice in lapic_shutdown

In reply to Cyrill Gorcunov

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

Signed-off-by Cyrill Gorcunov <[email protected]>

---

arch/x86/kernel/apic_32.c +2 -1
Index: linux-2.6.git/arch/x86/kernel/apic_32.c
===================================================================
@@ -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);
 }
author
#5 lore raw

PATCH 5/8 x86: apic - get rid of local_apic_timer_verify_ok

In reply to Cyrill Gorcunov

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 <[email protected]>

---

arch/x86/kernel/apic_32.c +4 -6
Index: linux-2.6.git/arch/x86/kernel/apic_32.c
===================================================================
@@ -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;
author
#6 lore raw

PATCH 6/8 x86: apic - unify verify_local_APIC

In reply to Cyrill Gorcunov
Signed-off-by Cyrill Gorcunov <[email protected]>

---

arch/x86/kernel/apic_32.c +6 -0
Index: linux-2.6.git/arch/x86/kernel/apic_32.c
===================================================================
@@ -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.
author
#7 lore raw

PATCH 7/8 x86: apic - unify sync_Arb_IDs

In reply to Cyrill Gorcunov
Signed-off-by Cyrill Gorcunov <[email protected]>

---

arch/x86/kernel/apic_32.c +1 -2
index 1d5f830..ec0f18f 100644
@@ -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);
 }
 
 /*
arch/x86/kernel/apic_64.c +6 -4
index 5d07689..f8d2918 100644
@@ -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);
 }
 
 /*
Signature · 3 lines
1.6.0.rc1.34.g0fe8c

--
#8 lore raw

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

On Thu, 1 Jan 1970, Cyrill Gorcunov wrote:

Hmm, are you living in the past?

Quoted 8 lines
Signed-off-by Cyrill Gorcunov <[email protected]>

---

arch/x86/kernel/apic_32.c +0 -0
index 1d5f830..ec0f18f 100644

arch/x86/kernel/apic_32.c void __init sync_Arb_IDs(void) · line 925

Quoted 9 lines
arch/x86/kernel/apic_32.c +1 -2
@@ -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

author
#9 lore raw

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

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

Quoted 29 lines · Maciej W. Rozycki

On Thu, 1 Jan 1970, Cyrill Gorcunov wrote:

Hmm, are you living in the past?

Quoted 17 lines
Signed-off-by Cyrill Gorcunov <[email protected]>

---

arch/x86/kernel/apic_32.c +1 -2
index 1d5f830..ec0f18f 100644
@@ -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);
 }
Quoted 1 line

/*

[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

author
#10 lore raw

PATCH 8/8 x86: apic - unify init_bsp_APIC

In reply to Cyrill Gorcunov

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 <[email protected]>

---

arch/x86/kernel/apic_32.c +3 -2
Index: linux-2.6.git/arch/x86/kernel/apic_32.c
===================================================================
@@ -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);
 }
arch/x86/kernel/apic_64.c +9 -3
Index: linux-2.6.git/arch/x86/kernel/apic_64.c
===================================================================
@@ -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);
 }
#11 lore raw

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

arch/x86/kernel/apic_32.c void __init init_bsp_APIC(void) · line 969

Quoted 10 lines · Cyrill Gorcunov
arch/x86/kernel/apic_32.c +2 -1
@@ -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

author
#12 lore raw

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

Quoted 15 lines · Maciej W. Rozycki

arch/x86/kernel/apic_32.c void __init init_bsp_APIC(void) · line 969

Quoted 10 lines · Cyrill Gorcunov
arch/x86/kernel/apic_32.c +2 -1
@@ -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.

Quoted 2 lines

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

#13 lore raw

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

Quoted 17 lines · Cyrill Gorcunov
Quoted 15 lines · Maciej W. Rozycki

arch/x86/kernel/apic_32.c void __init init_bsp_APIC(void) · line 969

Quoted 10 lines · Cyrill Gorcunov
arch/x86/kernel/apic_32.c +2 -1
@@ -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.

Quoted 6 lines
Quoted 2 lines

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

#14 lore raw

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

Quoted 13 lines · Ingo Molnar
Quoted 13 lines
Quoted 13 lines

arch/x86/kernel/apic_32.c void __init init_bsp_APIC(void) · line 969

Quoted 10 lines
arch/x86/kernel/apic_32.c +2 -1
@@ -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.

Quoted 8 lines
Quoted 6 lines
Quoted 2 lines

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().

Quoted 4 lines

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

#15 lore raw

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

Quoted 8 lines · Maciej W. Rozycki
Quoted 4 lines

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

author
#16 lore raw

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

[Ingo Molnar - Fri, Aug 15, 2008 at 03:48:01PM +0200]

| 
| * Maciej W. Rozycki <[email protected]> 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 -

#17 lore raw

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

Quoted 8 lines · Cyrill Gorcunov

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

#18 lore raw

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

In reply to Cyrill Gorcunov
Quoted 3 lines · Cyrill Gorcunov

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!

Quoted 3 lines

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

author
#19 lore raw

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

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