one more apic merging preliminary series

one more apic merging preliminary series

[Cyrill Gorcunov]

Please review - any comments are welcome!

For now it's like code bloating - but it's just preliminary
series to make apic_*.c code more or less similar.
And it's still a bit far from being ready to be merged down.

		- Cyrill -

[PATCH 1/6] x86: apic - unify clear_local_APIC

[Cyrill Gorcunov]

- Remove redundant masking of APIC_LVTTHMR register in apic_32.c

- Add masking of APIC_LVTTHMR register to apic_64.c. We use a bit
  complicated #ifdef here: CONFIG_X86_MCE_P4THERMAL is 32bit specific
  and X86_MCE_INTEL is 64bit specific so the appropriate config variable
  will be set by Kconfig.

- the APIC_ESR register clearing in apic_64.c now uses not straightforward
  way but this is allowed tradeoff.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 arch/x86/kernel/apic_32.c |    6 +-----
 arch/x86/kernel/apic_64.c |   17 +++++++++++++++--
 2 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kernel/apic_32.c b/arch/x86/kernel/apic_32.c
index 16d9721..3131603 100644
--- a/arch/x86/kernel/apic_32.c
+++ b/arch/x86/kernel/apic_32.c
@@ -754,7 +754,7 @@ void clear_local_APIC(void)
 	}
 
 	/* lets not touch this if we didn't frob it */
-#ifdef CONFIG_X86_MCE_P4THERMAL
+#if defined(CONFIG_X86_MCE_P4THERMAL) || defined(X86_MCE_INTEL)
 	if (maxlvt >= 5) {
 		v = apic_read(APIC_LVTTHMR);
 		apic_write(APIC_LVTTHMR, v | APIC_LVT_MASKED);
@@ -771,10 +771,6 @@ void clear_local_APIC(void)
 	if (maxlvt >= 4)
 		apic_write(APIC_LVTPC, APIC_LVT_MASKED);
 
-#ifdef CONFIG_X86_MCE_P4THERMAL
-	if (maxlvt >= 5)
-		apic_write(APIC_LVTTHMR, APIC_LVT_MASKED);
-#endif
 	/* Integrated APIC (!82489DX) ? */
 	if (lapic_is_integrated()) {
 		if (maxlvt > 3)
diff --git a/arch/x86/kernel/apic_64.c b/arch/x86/kernel/apic_64.c
index 99d18b8..d834b75 100644
--- a/arch/x86/kernel/apic_64.c
+++ b/arch/x86/kernel/apic_64.c
@@ -630,6 +630,13 @@ void clear_local_APIC(void)
 		apic_write(APIC_LVTPC, v | APIC_LVT_MASKED);
 	}
 
+	/* lets not touch this if we didn't frob it */
+#if defined(CONFIG_X86_MCE_P4THERMAL) || defined(X86_MCE_INTEL)
+	if (maxlvt >= 5) {
+		v = apic_read(APIC_LVTTHMR);
+		apic_write(APIC_LVTTHMR, v | APIC_LVT_MASKED);
+	}
+#endif
 	/*
 	 * Clean APIC state for other OSs:
 	 */
@@ -640,8 +647,14 @@ void clear_local_APIC(void)
 		apic_write(APIC_LVTERR, APIC_LVT_MASKED);
 	if (maxlvt >= 4)
 		apic_write(APIC_LVTPC, APIC_LVT_MASKED);
-	apic_write(APIC_ESR, 0);
-	apic_read(APIC_ESR);
+
+	/* Integrated APIC (!82489DX) ? */
+	if (lapic_is_integrated()) {
+		if (maxlvt > 3)
+			/* Clear ESR due to Pentium errata 3AP and 11AP */
+			apic_write(APIC_ESR, 0);
+		apic_read(APIC_ESR);
+	}
 }
 
 /**
-- 
1.6.0.rc1.34.g0fe8c

[PATCH 2/6] x86: apic - unify lapic_resume

[Cyrill Gorcunov]

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 arch/x86/kernel/apic_32.c |   29 ++++++++++++++++++-----------
 arch/x86/kernel/apic_64.c |   19 +++++++++++++++----
 2 files changed, 33 insertions(+), 15 deletions(-)

diff --git a/arch/x86/kernel/apic_32.c b/arch/x86/kernel/apic_32.c
index 3131603..3d40213 100644
--- a/arch/x86/kernel/apic_32.c
+++ b/arch/x86/kernel/apic_32.c
@@ -1606,16 +1606,21 @@ static int lapic_resume(struct sys_device *dev)
 
 	local_irq_save(flags);
 
-	/*
-	 * Make sure the APICBASE points to the right address
-	 *
-	 * FIXME! This will be wrong if we ever support suspend on
-	 * SMP! We'll need to do this as part of the CPU restore!
-	 */
-	rdmsr(MSR_IA32_APICBASE, l, h);
-	l &= ~MSR_IA32_APICBASE_BASE;
-	l |= MSR_IA32_APICBASE_ENABLE | mp_lapic_addr;
-	wrmsr(MSR_IA32_APICBASE, l, h);
+#ifdef CONFIG_X86_64
+	if (x2apic)
+		enable_x2apic();
+	else
+#endif
+		/*
+		 * Make sure the APICBASE points to the right address
+		 *
+		 * FIXME! This will be wrong if we ever support suspend on
+		 * SMP! We'll need to do this as part of the CPU restore!
+		 */
+		rdmsr(MSR_IA32_APICBASE, l, h);
+		l &= ~MSR_IA32_APICBASE_BASE;
+		l |= MSR_IA32_APICBASE_ENABLE | mp_lapic_addr;
+		wrmsr(MSR_IA32_APICBASE, l, h);
 
 	apic_write(APIC_LVTERR, ERROR_APIC_VECTOR | APIC_LVT_MASKED);
 	apic_write(APIC_ID, apic_pm_state.apic_id);
@@ -1625,7 +1630,7 @@ static int lapic_resume(struct sys_device *dev)
 	apic_write(APIC_SPIV, apic_pm_state.apic_spiv);
 	apic_write(APIC_LVT0, apic_pm_state.apic_lvt0);
 	apic_write(APIC_LVT1, apic_pm_state.apic_lvt1);
-#ifdef CONFIG_X86_MCE_P4THERMAL
+#if defined(CONFIG_X86_MCE_P4THERMAL) || defined(CONFIG_X86_MCE_INTEL)
 	if (maxlvt >= 5)
 		apic_write(APIC_LVTTHMR, apic_pm_state.apic_thmr);
 #endif
@@ -1639,7 +1644,9 @@ static int lapic_resume(struct sys_device *dev)
 	apic_write(APIC_LVTERR, apic_pm_state.apic_lvterr);
 	apic_write(APIC_ESR, 0);
 	apic_read(APIC_ESR);
+
 	local_irq_restore(flags);
+
 	return 0;
 }
 
diff --git a/arch/x86/kernel/apic_64.c b/arch/x86/kernel/apic_64.c
index d834b75..e542a2d 100644
--- a/arch/x86/kernel/apic_64.c
+++ b/arch/x86/kernel/apic_64.c
@@ -1412,13 +1412,22 @@ static int lapic_resume(struct sys_device *dev)
 	maxlvt = lapic_get_maxlvt();
 
 	local_irq_save(flags);
-	if (!x2apic) {
+
+#ifdef CONFIG_X86_64
+	if (x2apic)
+		enable_x2apic();
+	else
+#endif
+		/*
+		 * Make sure the APICBASE points to the right address
+		 *
+		 * FIXME! This will be wrong if we ever support suspend on
+		 * SMP! We'll need to do this as part of the CPU restore!
+		 */
 		rdmsr(MSR_IA32_APICBASE, l, h);
 		l &= ~MSR_IA32_APICBASE_BASE;
 		l |= MSR_IA32_APICBASE_ENABLE | mp_lapic_addr;
 		wrmsr(MSR_IA32_APICBASE, l, h);
-	} else
-		enable_x2apic();
 
 	apic_write(APIC_LVTERR, ERROR_APIC_VECTOR | APIC_LVT_MASKED);
 	apic_write(APIC_ID, apic_pm_state.apic_id);
@@ -1428,7 +1437,7 @@ static int lapic_resume(struct sys_device *dev)
 	apic_write(APIC_SPIV, apic_pm_state.apic_spiv);
 	apic_write(APIC_LVT0, apic_pm_state.apic_lvt0);
 	apic_write(APIC_LVT1, apic_pm_state.apic_lvt1);
-#ifdef CONFIG_X86_MCE_INTEL
+#if defined(CONFIG_X86_MCE_P4THERMAL) || defined(CONFIG_X86_MCE_INTEL)
 	if (maxlvt >= 5)
 		apic_write(APIC_LVTTHMR, apic_pm_state.apic_thmr);
 #endif
@@ -1442,7 +1451,9 @@ static int lapic_resume(struct sys_device *dev)
 	apic_write(APIC_LVTERR, apic_pm_state.apic_lvterr);
 	apic_write(APIC_ESR, 0);
 	apic_read(APIC_ESR);
+
 	local_irq_restore(flags);
+
 	return 0;
 }
 
-- 
1.6.0.rc1.34.g0fe8c

[PATCH 3/6] x86: apic - unify lapic_suspend

[Cyrill Gorcunov]

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 arch/x86/kernel/apic_32.c |    2 +-
 arch/x86/kernel/apic_64.c |    3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/apic_32.c b/arch/x86/kernel/apic_32.c
index 3d40213..6cb8aaa 100644
--- a/arch/x86/kernel/apic_32.c
+++ b/arch/x86/kernel/apic_32.c
@@ -1582,7 +1582,7 @@ static int lapic_suspend(struct sys_device *dev, pm_message_t state)
 	apic_pm_state.apic_lvterr = apic_read(APIC_LVTERR);
 	apic_pm_state.apic_tmict = apic_read(APIC_TMICT);
 	apic_pm_state.apic_tdcr = apic_read(APIC_TDCR);
-#ifdef CONFIG_X86_MCE_P4THERMAL
+#if defined(CONFIG_X86_MCE_P4THERMAL) || defined(CONFIG_X86_MCE_INTEL)
 	if (maxlvt >= 5)
 		apic_pm_state.apic_thmr = apic_read(APIC_LVTTHMR);
 #endif
diff --git a/arch/x86/kernel/apic_64.c b/arch/x86/kernel/apic_64.c
index e542a2d..13dea93 100644
--- a/arch/x86/kernel/apic_64.c
+++ b/arch/x86/kernel/apic_64.c
@@ -1390,10 +1390,11 @@ static int lapic_suspend(struct sys_device *dev, pm_message_t state)
 	apic_pm_state.apic_lvterr = apic_read(APIC_LVTERR);
 	apic_pm_state.apic_tmict = apic_read(APIC_TMICT);
 	apic_pm_state.apic_tdcr = apic_read(APIC_TDCR);
-#ifdef CONFIG_X86_MCE_INTEL
+#if defined(CONFIG_X86_MCE_P4THERMAL) || defined(CONFIG_X86_MCE_INTEL)
 	if (maxlvt >= 5)
 		apic_pm_state.apic_thmr = apic_read(APIC_LVTTHMR);
 #endif
+
 	local_irq_save(flags);
 	disable_local_APIC();
 	local_irq_restore(flags);
-- 
1.6.0.rc1.34.g0fe8c

[PATCH 4/6] x86: apic - rearrange functions and comments

[Cyrill Gorcunov]

Rearrange functions and comments to find differences
easier.

Also use apic_printk in setup_boot_APIC_clock for
64bit mode.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 arch/x86/kernel/apic_32.c |   69 +++++++++++++++++++++++++-------------------
 arch/x86/kernel/apic_64.c |   48 ++++++++++++++++++++++++------
 2 files changed, 77 insertions(+), 40 deletions(-)

diff --git a/arch/x86/kernel/apic_32.c b/arch/x86/kernel/apic_32.c
index 6cb8aaa..9e8702e 100644
--- a/arch/x86/kernel/apic_32.c
+++ b/arch/x86/kernel/apic_32.c
@@ -251,6 +251,9 @@ int lapic_get_maxlvt(void)
  * this function twice on the boot CPU, once with a bogus timeout
  * value, second time for real. The other (noncalibrating) CPUs
  * call this function only once, with the real, calibrated value.
+ *
+ * We do reads before writes even if unnecessary, to get around the
+ * P5 APIC double write bug.
  */
 static void __setup_APIC_LVTT(unsigned int clocks, int oneshot, int irqen)
 {
@@ -280,6 +283,36 @@ static void __setup_APIC_LVTT(unsigned int clocks, int oneshot, int irqen)
 }
 
 /*
+ * Setup extended LVT, AMD specific (K8, family 10h)
+ *
+ * Vector mappings are hard coded. On K8 only offset 0 (APIC500) and
+ * MCE interrupts are supported. Thus MCE offset must be set to 0.
+ */
+
+#define APIC_EILVT_LVTOFF_MCE 0
+#define APIC_EILVT_LVTOFF_IBS 1
+
+static void setup_APIC_eilvt(u8 lvt_off, u8 vector, u8 msg_type, u8 mask)
+{
+	unsigned long reg = (lvt_off << 4) + APIC_EILVT0;
+	unsigned int  v   = (mask << 16) | (msg_type << 8) | vector;
+
+	apic_write(reg, v);
+}
+
+u8 setup_APIC_eilvt_mce(u8 vector, u8 msg_type, u8 mask)
+{
+	setup_APIC_eilvt(APIC_EILVT_LVTOFF_MCE, vector, msg_type, mask);
+	return APIC_EILVT_LVTOFF_MCE;
+}
+
+u8 setup_APIC_eilvt_ibs(u8 vector, u8 msg_type, u8 mask)
+{
+	setup_APIC_eilvt(APIC_EILVT_LVTOFF_IBS, vector, msg_type, mask);
+	return APIC_EILVT_LVTOFF_IBS;
+}
+
+/*
  * Program the next event, relative to now
  */
 static int lapic_next_event(unsigned long delta,
@@ -298,7 +331,7 @@ static void lapic_timer_setup(enum clock_event_mode mode,
 	unsigned long flags;
 	unsigned int v;
 
-	/* Lapic used for broadcast ? */
+	/* Lapic used as dummy for broadcast ? */
 	if (evt->features & CLOCK_EVT_FEAT_DUMMY)
 		return;
 
@@ -681,35 +714,6 @@ int setup_profiling_timer(unsigned int multiplier)
 }
 
 /*
- * Setup extended LVT, AMD specific (K8, family 10h)
- *
- * Vector mappings are hard coded. On K8 only offset 0 (APIC500) and
- * MCE interrupts are supported. Thus MCE offset must be set to 0.
- */
-
-#define APIC_EILVT_LVTOFF_MCE 0
-#define APIC_EILVT_LVTOFF_IBS 1
-
-static void setup_APIC_eilvt(u8 lvt_off, u8 vector, u8 msg_type, u8 mask)
-{
-	unsigned long reg = (lvt_off << 4) + APIC_EILVT0;
-	unsigned int  v   = (mask << 16) | (msg_type << 8) | vector;
-	apic_write(reg, v);
-}
-
-u8 setup_APIC_eilvt_mce(u8 vector, u8 msg_type, u8 mask)
-{
-	setup_APIC_eilvt(APIC_EILVT_LVTOFF_MCE, vector, msg_type, mask);
-	return APIC_EILVT_LVTOFF_MCE;
-}
-
-u8 setup_APIC_eilvt_ibs(u8 vector, u8 msg_type, u8 mask)
-{
-	setup_APIC_eilvt(APIC_EILVT_LVTOFF_IBS, vector, msg_type, mask);
-	return APIC_EILVT_LVTOFF_IBS;
-}
-
-/*
  * Local APIC start and shutdown
  */
 
@@ -1542,6 +1546,11 @@ void __cpuinit generic_processor_info(int apicid, int version)
 #ifdef CONFIG_PM
 
 static struct {
+	/*
+	 * 'active' is true if the local APIC was enabled by us and
+	 * not the BIOS; this signifies that we are also responsible
+	 * for disabling it before entering apm/acpi suspend
+	 */
 	int active;
 	/* r/w apic fields */
 	unsigned int apic_id;
diff --git a/arch/x86/kernel/apic_64.c b/arch/x86/kernel/apic_64.c
index 13dea93..46523c0 100644
--- a/arch/x86/kernel/apic_64.c
+++ b/arch/x86/kernel/apic_64.c
@@ -81,6 +81,9 @@ static void lapic_timer_setup(enum clock_event_mode mode,
 static void lapic_timer_broadcast(cpumask_t mask);
 static void apic_pm_activate(void);
 
+/*
+ * The local apic timer can be used for any function which is CPU local.
+ */
 static struct clock_event_device lapic_clockevent = {
 	.name		= "lapic",
 	.features	= CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT
@@ -127,6 +130,11 @@ static int modern_apic(void)
 	return lapic_get_version() >= 0x14;
 }
 
+/*
+ * Paravirt kernels also might be using these below ops. So we still
+ * use generic apic_read()/apic_write(), which might be pointing to different
+ * ops in PARAVIRT case.
+ */
 void xapic_wait_icr_idle(void)
 {
 	while (apic_read(APIC_ICR) & APIC_ICR_BUSY)
@@ -175,7 +183,6 @@ static struct apic_ops xapic_ops = {
 };
 
 struct apic_ops __read_mostly *apic_ops = &xapic_ops;
-
 EXPORT_SYMBOL_GPL(apic_ops);
 
 static void x2apic_wait_icr_idle(void)
@@ -244,6 +251,10 @@ int lapic_get_maxlvt(void)
 	return APIC_INTEGRATED(GET_APIC_VERSION(v)) ? GET_APIC_MAXLVT(v) : 2;
 }
 
+/*
+ * Local APIC timer
+ */
+
 /* Clock divisor is set to 1 */
 #define APIC_DIVISOR 1
 
@@ -257,7 +268,6 @@ int lapic_get_maxlvt(void)
  * We do reads before writes even if unnecessary, to get around the
  * P5 APIC double write bug.
  */
-
 static void __setup_APIC_LVTT(unsigned int clocks, int oneshot, int irqen)
 {
 	unsigned int lvtt_value, tmp_value;
@@ -474,10 +484,10 @@ static int __init calibrate_APIC_clock(void)
 void __init setup_boot_APIC_clock(void)
 {
 	/*
-	 * The local apic timer can be disabled via the kernel commandline.
-	 * Register the lapic timer as a dummy clock event source on SMP
-	 * systems, so the broadcast mechanism is used. On UP systems simply
-	 * ignore it.
+	 * The local apic timer can be disabled via the kernel
+	 * commandline or from the CPU detection code. Register the lapic
+	 * timer as a dummy clock event source on SMP systems, so the
+	 * broadcast mechanism is used. On UP systems simply ignore it.
 	 */
 	if (disable_apic_timer) {
 		printk(KERN_INFO "Disabling APIC timer\n");
@@ -489,7 +499,9 @@ void __init setup_boot_APIC_clock(void)
 		return;
 	}
 
-	printk(KERN_INFO "Using local APIC timer interrupts.\n");
+	apic_printk(APIC_VERBOSE, "Using local APIC timer interrupts.\n"
+		    "calibrating APIC timer ...\n");
+
 	if (calibrate_APIC_clock()) {
 		/* No broadcast on UP ! */
 		if (num_possible_cpus() > 1)
@@ -508,6 +520,7 @@ void __init setup_boot_APIC_clock(void)
 		printk(KERN_WARNING "APIC timer registered as dummy,"
 			" due to nmi_watchdog=%d!\n", nmi_watchdog);
 
+	/* Setup the lapic or request the broadcast */
 	setup_APIC_timer();
 }
 
@@ -577,6 +590,7 @@ void smp_apic_timer_interrupt(struct pt_regs *regs)
 	irq_enter();
 	local_apic_timer_interrupt();
 	irq_exit();
+
 	set_irq_regs(old_regs);
 }
 
@@ -1248,6 +1262,13 @@ void __init connect_bsp_APIC(void)
 	enable_apic_mode();
 }
 
+/**
+ * disconnect_bsp_APIC - detach the APIC from the interrupt system
+ * @virt_wire_setup:	indicates, whether virtual wire mode is selected
+ *
+ * Virtual wire mode is necessary to deliver legacy interrupts even when the
+ * APIC is disabled.
+ */
 void disconnect_bsp_APIC(int virt_wire_setup)
 {
 	/* Go back to Virtual Wire compatibility mode */
@@ -1347,9 +1368,11 @@ int hard_smp_processor_id(void)
 #ifdef CONFIG_PM
 
 static struct {
-	/* 'active' is true if the local APIC was enabled by us and
-	   not the BIOS; this signifies that we are also responsible
-	   for disabling it before entering apm/acpi suspend */
+	/*
+	 * 'active' is true if the local APIC was enabled by us and
+	 * not the BIOS; this signifies that we are also responsible
+	 * for disabling it before entering apm/acpi suspend
+	 */
 	int active;
 	/* r/w apic fields */
 	unsigned int apic_id;
@@ -1458,6 +1481,11 @@ static int lapic_resume(struct sys_device *dev)
 	return 0;
 }
 
+/*
+ * This device has no shutdown method - fully functioning local APICs
+ * are needed on every CPU up until machine_halt/restart/poweroff.
+ */
+
 static struct sysdev_class lapic_sysclass = {
 	.name		= "lapic",
 	.resume		= lapic_resume,
-- 
1.6.0.rc1.34.g0fe8c

[PATCH 5/6] x86: apic - unify lapic_is_integrated

[Cyrill Gorcunov]

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 arch/x86/kernel/apic_32.c |    4 ++++
 arch/x86/kernel/apic_64.c |    6 +++++-
 2 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/apic_32.c b/arch/x86/kernel/apic_32.c
index 9e8702e..41134d4 100644
--- a/arch/x86/kernel/apic_32.c
+++ b/arch/x86/kernel/apic_32.c
@@ -128,7 +128,11 @@ static inline int lapic_get_version(void)
  */
 static inline int lapic_is_integrated(void)
 {
+#ifdef CONFIG_X86_64
+	return 1;
+#else
 	return APIC_INTEGRATED(lapic_get_version());
+#endif
 }
 
 /*
diff --git a/arch/x86/kernel/apic_64.c b/arch/x86/kernel/apic_64.c
index 46523c0..18c0b8c 100644
--- a/arch/x86/kernel/apic_64.c
+++ b/arch/x86/kernel/apic_64.c
@@ -111,11 +111,15 @@ static inline int lapic_get_version(void)
 }
 
 /*
- * Check, if the APIC is integrated or a seperate chip
+ * Check, if the APIC is integrated or a separate chip
  */
 static inline int lapic_is_integrated(void)
 {
+#ifdef CONFIG_X86_64
 	return 1;
+#else
+	return APIC_INTEGRATED(lapic_get_version());
+#endif
 }
 
 /*
-- 
1.6.0.rc1.34.g0fe8c

[PATCH 6/6] x86: apic - unify xapic_icr_read

[Cyrill Gorcunov]

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 arch/x86/kernel/apic_64.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/apic_64.c b/arch/x86/kernel/apic_64.c
index 18c0b8c..5e07381 100644
--- a/arch/x86/kernel/apic_64.c
+++ b/arch/x86/kernel/apic_64.c
@@ -174,7 +174,7 @@ u64 xapic_icr_read(void)
 	icr2 = apic_read(APIC_ICR2);
 	icr1 = apic_read(APIC_ICR);
 
-	return (icr1 | ((u64)icr2 << 32));
+	return icr1 | ((u64)icr2 << 32);
 }
 
 static struct apic_ops xapic_ops = {
-- 
1.6.0.rc1.34.g0fe8c

Re: [PATCH 2/6] x86: apic - unify lapic_resume

[Maciej W. Rozycki]

On Sat, 16 Aug 2008, Cyrill Gorcunov wrote:

> +#ifdef CONFIG_X86_64
> +	if (x2apic)
> +		enable_x2apic();
> +	else
> +#endif

 Hmm, x86-64 hardware can run a 32-bit kernel, so it might be a good idea
to take the opportunity of the merge and extend x2APIC support to the
32-bit configuration too.  It should be mostly a mechanical change.  Just
a suggestion though -- feel free to ignore if you'd rather not dive into
it. ;)

  Maciej

Re: [PATCH 2/6] x86: apic - unify lapic_resume

[Arjan van de Ven]

On Sat, 16 Aug 2008 20:52:07 +0100 (BST)
"Maciej W. Rozycki" <macro@linux-mips.org> wrote:

> On Sat, 16 Aug 2008, Cyrill Gorcunov wrote:
> 
> > +#ifdef CONFIG_X86_64
> > +	if (x2apic)
> > +		enable_x2apic();
> > +	else
> > +#endif
> 
>  Hmm, x86-64 hardware can run a 32-bit kernel, so it might be a good
> idea to take the opportunity of the merge and extend x2APIC support
> to the 32-bit configuration too.  It should be mostly a mechanical
> change.  Just a suggestion though -- feel free to ignore if you'd
> rather not dive into it. ;)

it's not like you can/want to go over 128 cpus on a 32 bit kernel
though..
-- 
If you want to reach me at my work email, use arjan@linux.intel.com
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [PATCH 2/6] x86: apic - unify lapic_resume

[Cyrill Gorcunov]

[Maciej W. Rozycki - Sat, Aug 16, 2008 at 08:52:07PM +0100]
| On Sat, 16 Aug 2008, Cyrill Gorcunov wrote:
| 
| > +#ifdef CONFIG_X86_64
| > +	if (x2apic)
| > +		enable_x2apic();
| > +	else
| > +#endif
| 
|  Hmm, x86-64 hardware can run a 32-bit kernel, so it might be a good idea
| to take the opportunity of the merge and extend x2APIC support to the
| 32-bit configuration too.  It should be mostly a mechanical change.  Just
| a suggestion though -- feel free to ignore if you'd rather not dive into
| it. ;)
| 
|   Maciej
| 

yes, I thought about this - will take a look, thanks!

		- Cyrill -

Re: [PATCH 2/6] x86: apic - unify lapic_resume

[Cyrill Gorcunov]

[Arjan van de Ven - Sat, Aug 16, 2008 at 01:00:08PM -0700]
| On Sat, 16 Aug 2008 20:52:07 +0100 (BST)
| "Maciej W. Rozycki" <macro@linux-mips.org> wrote:
| 
| > On Sat, 16 Aug 2008, Cyrill Gorcunov wrote:
| > 
| > > +#ifdef CONFIG_X86_64
| > > +	if (x2apic)
| > > +		enable_x2apic();
| > > +	else
| > > +#endif
| > 
| >  Hmm, x86-64 hardware can run a 32-bit kernel, so it might be a good
| > idea to take the opportunity of the merge and extend x2APIC support
| > to the 32-bit configuration too.  It should be mostly a mechanical
| > change.  Just a suggestion though -- feel free to ignore if you'd
| > rather not dive into it. ;)
| 
| it's not like you can/want to go over 128 cpus on a 32 bit kernel
| though..
| -- 
| If you want to reach me at my work email, use arjan@linux.intel.com
| For development, discussion and tips for power savings, 
| visit http://www.lesswatts.org
| 

Arjan,

it seems this limit is not APIC related since iirc 82489DX allows
to address 2^14 bits, x2APIC - more then 128 entities too. So I suspect
it somehow cpu bitmap related. Am I wrong (I didn't _check_ the code)?

		- Cyrill -

Re: [PATCH 2/6] x86: apic - unify lapic_resume

[Maciej W. Rozycki]

On Sat, 16 Aug 2008, Arjan van de Ven wrote:

> it's not like you can/want to go over 128 cpus on a 32 bit kernel
> though..

 That's a quantitative rather than a qualitative limitation though.  I can
see no reason why limiting the number of CPUs would preclude the use of
the x2APIC mode altogether -- the extra CPUs will simply be ignored and
never waken up.  And with smaller systems and a 32-bit kernel one may
still want to get the small performance gain from using the MSRs rather
than going through the TLB for example.

 Similarly there is some RAM size limitation with the 32-bit configuration
too, but that does not prevent us from using however much we can in the
32-bit mode either. ;)

  Maciej

Re: one more apic merging preliminary series

[Ingo Molnar]

* Cyrill Gorcunov <gorcunov@gmail.com> wrote:

> Please review - any comments are welcome!
> 
> For now it's like code bloating - but it's just preliminary series to 
> make apic_*.c code more or less similar. And it's still a bit far from 
> being ready to be merged down.

applied to tip/x86/apic - thanks Cyrill.

Maciej's point about cleaning up the x2apic impact is very much true - 
i've Cc:-ed Suresh and Venki. Even if we wont truly use x2apic in 32-bit 
kernels, it's a piece of glue hardware that does not depend on which 
mode the CPU is in, so support for it should be bitsize agnostic. It 
will also obviously be good for test coverage, once x2apic capable hw 
will be more widespread.

	Ingo

Re: one more apic merging preliminary series

[Cyrill Gorcunov]

[Ingo Molnar - Sun, Aug 17, 2008 at 02:45:40PM +0200]
| 
| * Cyrill Gorcunov <gorcunov@gmail.com> wrote:
| 
| > Please review - any comments are welcome!
| > 
| > For now it's like code bloating - but it's just preliminary series to 
| > make apic_*.c code more or less similar. And it's still a bit far from 
| > being ready to be merged down.
| 
| applied to tip/x86/apic - thanks Cyrill.
| 
| Maciej's point about cleaning up the x2apic impact is very much true - 
| i've Cc:-ed Suresh and Venki. Even if we wont truly use x2apic in 32-bit 
| kernels, it's a piece of glue hardware that does not depend on which 
| mode the CPU is in, so support for it should be bitsize agnostic. It 
| will also obviously be good for test coverage, once x2apic capable hw 
| will be more widespread.
| 
| 	Ingo
| 

Thanks Ingo. I found a bit obscure point in APIC 32bit code -
disable_esr variable to be clear. Code reading didn't answer
me the question "for what is needed". 82489DX doesn't have ESR
register indeed I presumed that the variable is needed to set
'absence-flag' of a such register on some platform but it seems
the only code snippet where we use this flag is 32bit lapic_setup_esr.
Moreover others 32bit writers don't check for this feature but
just writting to ESR register...

		- Cyrill -

Re: [PATCH 2/6] x86: apic - unify lapic_resume

[Maciej W. Rozycki]

On Sun, 17 Aug 2008, Cyrill Gorcunov wrote:

> it seems this limit is not APIC related since iirc 82489DX allows
> to address 2^14 bits, x2APIC - more then 128 entities too. So I suspect
> it somehow cpu bitmap related. Am I wrong (I didn't _check_ the code)?

 For the record: the 82489DX supports an 8-bit physical unit ID so 256
distinct values are supported.  Of these I gather 255 is taken for the
broadcast ID in the physical destination mode, but that is nowhere
explicitly confirmed in the 82489DX docs (it can be implied from protocol
description though).  While the ID is indeed 8-bit in the 82489DX, for
systems where future compatibility was a concern Intel recommended the use
of IDs from 0 through to 14 only in anticipation of the future integrated
APIC with the physical mode addressing capability limited to 4 bits only
(and 15 taken for the broadcast ID).

 Full 32 bits are available with the 82489DX for the logical destination 
mode and only the flat (no cluster) mode is supported.

  Maciej

Re: [PATCH 2/6] x86: apic - unify lapic_resume

[Cyrill Gorcunov]

[Maciej W. Rozycki - Mon, Aug 18, 2008 at 03:19:05PM +0100]
| On Sun, 17 Aug 2008, Cyrill Gorcunov wrote:
| 
| > it seems this limit is not APIC related since iirc 82489DX allows
| > to address 2^14 bits, x2APIC - more then 128 entities too. So I suspect
| > it somehow cpu bitmap related. Am I wrong (I didn't _check_ the code)?
| 
|  For the record: the 82489DX supports an 8-bit physical unit ID so 256
| distinct values are supported.  Of these I gather 255 is taken for the
| broadcast ID in the physical destination mode, but that is nowhere
| explicitly confirmed in the 82489DX docs (it can be implied from protocol
| description though).  While the ID is indeed 8-bit in the 82489DX, for
| systems where future compatibility was a concern Intel recommended the use
| of IDs from 0 through to 14 only in anticipation of the future integrated
| APIC with the physical mode addressing capability limited to 4 bits only
| (and 15 taken for the broadcast ID).
| 
|  Full 32 bits are available with the 82489DX for the logical destination 
| mode and only the flat (no cluster) mode is supported.
| 
|   Maciej
| 

Thanks, Maciej! The doc says to use 0-14 so I misinterpreted it as
bits :(. I've to be more carefull next time. Thanks!

		- Cyrill -

Re: one more apic merging preliminary series

[Suresh Siddha]

On Sun, Aug 17, 2008 at 05:45:40AM -0700, Ingo Molnar wrote:
> 
> * Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> 
> > Please review - any comments are welcome!
> >
> > For now it's like code bloating - but it's just preliminary series to
> > make apic_*.c code more or less similar. And it's still a bit far from
> > being ready to be merged down.
> 
> applied to tip/x86/apic - thanks Cyrill.
> 
> Maciej's point about cleaning up the x2apic impact is very much true -
> i've Cc:-ed Suresh and Venki. Even if we wont truly use x2apic in 32-bit
> kernels, it's a piece of glue hardware that does not depend on which
> mode the CPU is in, so support for it should be bitsize agnostic. It
> will also obviously be good for test coverage, once x2apic capable hw
> will be more widespread.

Interrupt-remapping(part of chipset) must be enabled before enabling/using
x2apic mode.

So, there is little bit more work (adding the intr-remapping pieces to 32bit
kernel etc) to be done, before enabling x2apic for 32-bit kernel aswell.

I am planning to do some work (introduce some kind of interrupt-remapping ops
and cleanup some of the code in the existing io_apic_64.c), that will
eventually make it easy to add interrupt-remapping and x2apic to 32bit aswell.

thanks,
suresh

Re: one more apic merging preliminary series

[Ingo Molnar]

* Suresh Siddha <suresh.b.siddha@intel.com> wrote:

> On Sun, Aug 17, 2008 at 05:45:40AM -0700, Ingo Molnar wrote:
> > 
> > * Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> > 
> > > Please review - any comments are welcome!
> > >
> > > For now it's like code bloating - but it's just preliminary series to
> > > make apic_*.c code more or less similar. And it's still a bit far from
> > > being ready to be merged down.
> > 
> > applied to tip/x86/apic - thanks Cyrill.
> > 
> > Maciej's point about cleaning up the x2apic impact is very much true -
> > i've Cc:-ed Suresh and Venki. Even if we wont truly use x2apic in 32-bit
> > kernels, it's a piece of glue hardware that does not depend on which
> > mode the CPU is in, so support for it should be bitsize agnostic. It
> > will also obviously be good for test coverage, once x2apic capable hw
> > will be more widespread.
> 
> Interrupt-remapping(part of chipset) must be enabled before 
> enabling/using x2apic mode.
> 
> So, there is little bit more work (adding the intr-remapping pieces to 
> 32bit kernel etc) to be done, before enabling x2apic for 32-bit kernel 
> aswell.
> 
> I am planning to do some work (introduce some kind of 
> interrupt-remapping ops and cleanup some of the code in the existing 
> io_apic_64.c), that will eventually make it easy to add 
> interrupt-remapping and x2apic to 32bit aswell.

Yinghai unified io_apic.c in latest tip/master, and that means that the 
CONFIG_INTR_REMAP bit are now all in 32-bit code as well. It's not yet 
enabled in the Kconfig:

 config INTR_REMAP
        bool "Support for Interrupt Remapping (EXPERIMENTAL)"
        depends on X86_64 && X86_IO_APIC && PCI_MSI && ACPI && EXPERIMENTAL

but it should be a lot less work now i suspect.

	Ingo