[PATCH 0/3] x86/irq: Refactor special vector definition and cleanup

[PATCH 0/3] x86/irq: Refactor special vector definition and cleanup

[Jianyu Zhan]

Currently special(system) irq definition layout is a bit random, due to quite
a long period of code shuffle and refactorization, making native_init_IRQ()
quit hard to follow.

Besides, there are also some leftovers on the vector layout comment.
For example, INVALIDATE_TLB_VECTOR_START vectors have gone by using generic IPI 
mechanism(see commit 52aec3308db8). VSYSCALL_EMU_VECTOR is also gone because
vsyscalls are emulated by instruction fault traps(see commit 3ae36655b97a).

This patch set aims at refactoring the speical vector defnition and do some cleanup.

*** Test done ***
This patch set has been rebased on tip/master and have done build test and run it
for hours, doing daily jobs, and found no problem.

Jianyu Zhan (3):
  x86/asm/irq: Rearrange definitoin of specical irq vectors and cleanup.
  x86/irq: refactor native_init_IRQ
  x86/irq: update first_system_vector only when X86_LOCAL_PIC is on

 arch/x86/include/asm/desc.h        |  2 ++
 arch/x86/include/asm/irq_vectors.h | 72 +++++++++++++++++++++++++++++---------
 arch/x86/kernel/irqinit.c          | 71 +++++++++++++++++++++----------------
 3 files changed, 97 insertions(+), 48 deletions(-)

-- 
2.4.3

[PATCH 1/3] x86/asm/irq: Rearrange definitoin of specical irq vectors and cleanup.

[Jianyu Zhan]

Currently special(system) irq definition layout is a bit random, due to quite
a long period of code shuffle and refactorization, making native_init_IRQ()
quit hard to follow.

Besides, there are also some leftovers on the vector layout comment.
For example, INVALIDATE_TLB_VECTOR_START vectors have gone by using generic IPI
mechanism(see commit 52aec3308db8). VSYSCALL_EMU_VECTOR is also gone because
vsyscalls are emulated by instruction fault traps(see commit 3ae36655b97a).

So this patch aims at refactoring the special(system) irq vector layout and cleanup.

New system irq vectors layout are now look like:

0xff, 0xfe:
	Two highest vectors, assigned to spurious vector and error vector.
	Also add sanity check for error vector definition:

	SPURIOUS_APIC_VECTOR           0xff
	ERROR_APIC_VECTOR              0xfe

0xfd - 0xf9:
	CONFIG_SMP dependent vectors. On morden machines these are achieved
	via local APIC, but not neccessary:

   	RESCHEDULE_VECTOR     		0xfd
	CALL_FUNCTION_VECTOR		0xfc
	CALL_FUNCTION_SINGLE_VECTOR	0xfb
	REBOOT_VECTOR			0xfa
	X86_PLATFORM_IPI_VECTOR		0xf9

0xf8 - 0xf0:
	Local APIC dependent vectors. Some are only depending on Local ACPI,
	but some are depending on more.

	IRQ_WORK is not neccessarily depending on SMP, but currently it depends
	on X86_LOCAL_APIC. Werid, just leav it as-is:

	IRQ_WORK_VECTOR			0xf8

	Below are all depending on X86_LOCAL_APIC, some depend on more(MCE, Virt, etc):

	THERMAL_APIC_VECTOR             0xf7
	THRESHOLD_APIC_VECTOR           0xf6
	UV_BAU_MESSAGE                  0xf5
	DEFERRED_ERROR_VECTOR           0xf4
	HYPERVISOR_CALLBACK_VECTOR      0xf3

	POSTED_INTR_VECTOR              0xf2
	POSTED_INTR_WAKEUP_VECTOR       0xf1
	/* 0xf0 is currently not used */

0xef:
	Local APIC timer vector		0xef

Once this layout is applied, next patch will arrange native_init_IRQ() per this layout.

Signed-off-by: Jianyu Zhan <nasa4836@gmail.com>
---
 arch/x86/include/asm/irq_vectors.h | 72 +++++++++++++++++++++++++++++---------
 1 file changed, 55 insertions(+), 17 deletions(-)

diff --git a/arch/x86/include/asm/irq_vectors.h b/arch/x86/include/asm/irq_vectors.h
index 6ca9fd6..b785a19 100644
--- a/arch/x86/include/asm/irq_vectors.h
+++ b/arch/x86/include/asm/irq_vectors.h
@@ -17,8 +17,9 @@
  *  Vectors   0 ...  31 : system traps and exceptions - hardcoded events
  *  Vectors  32 ... 127 : device interrupts
  *  Vector  128         : legacy int80 syscall interface
- *  Vectors 129 ... INVALIDATE_TLB_VECTOR_START-1 except 204 : device interrupts
- *  Vectors INVALIDATE_TLB_VECTOR_START ... 255 : special interrupts
+ *  Vectors 129 ... 238 : device interrupts
+ *  Vectors 239(0xef)   : special(system) interrupt LOCAL_TIMER_VECTOR
+ *  Vectors 240 ... 255 : special(system) interrupts, see definition below for details.
  *
  * 64-bit x86 has per CPU IDT tables, 32-bit has one shared IDT table.
  *
@@ -55,40 +56,71 @@
 #define ISA_IRQ_VECTOR(irq)		(((FIRST_EXTERNAL_VECTOR + 16) & ~15) + irq)
 
 /*
- * Special IRQ vectors used by the SMP architecture, 0xf0-0xff
+ * Special IRQ vectors: 0xef - 0xff, for system vectors.
  *
  *  some of the following vectors are 'rare', they are merged
  *  into a single vector (CALL_FUNCTION_VECTOR) to save vector space.
  *  TLB, reschedule and local APIC vectors are performance-critical.
+ *
+ *  Layout:
+ *  0xff, 0xfe:
+ *	Two highest vectors, granted for spurious vector and error vector.
+ *  0xfd - 0xf9:
+ *	CONFIG_SMP dependent vectors. On morden machines these are achieved
+ *	via local APIC, but not neccessary.
+ *  0xf8 - 0xf0:
+ *      Local APIC dependent vectors. Some are only depending on Local ACPI,
+ *      but some are depending on more.
+ *  0xef:
+ *      Local APIC timer vector.
  */
 
-#define SPURIOUS_APIC_VECTOR		0xff
 /*
- * Sanity check
+ * Grant highest 2 vectors for two special vectors:
+ * Spurious Vector and Error Vector.
  */
-#if ((SPURIOUS_APIC_VECTOR & 0x0F) != 0x0F)
-# error SPURIOUS_APIC_VECTOR definition error
+#define SPURIOUS_APIC_VECTOR		0xff
+#define ERROR_APIC_VECTOR		0xfe
+
+#if SPURIOUS_APIC_VECTOR != 0xff
+# error SPURIOUS_APIC_VECTOR definition error, should grant it: 0xff
 #endif
 
-#define ERROR_APIC_VECTOR		0xfe
+#if ERROR_APIC_VECTOR  != 0xfe
+# error ERROR_APIC_VECTOR definition error, should grant it: 0xfe
+#endif
+
+
+/*
+ * SMP dependent vectors
+ */
+/* CPU-to-CPU reschedule-helper IPI, driven by wakeup.*/
 #define RESCHEDULE_VECTOR		0xfd
+
+/* IPI for generic function call */
 #define CALL_FUNCTION_VECTOR		0xfc
+
+/* IPI for generic single function call */
 #define CALL_FUNCTION_SINGLE_VECTOR	0xfb
-#define THERMAL_APIC_VECTOR		0xfa
-#define THRESHOLD_APIC_VECTOR		0xf9
-#define REBOOT_VECTOR			0xf8
+
+/* IPI used for rebooting/stopping */
+#define REBOOT_VECTOR			0xfa
+
+/* IPI for X86 platform specific use */
+#define X86_PLATFORM_IPI_VECTOR		0xf9
 
 /*
- * Generic system vector for platform specific use
+ * Local APCI dependent only vectors, these may or may not depend on SMP.
  */
-#define X86_PLATFORM_IPI_VECTOR		0xf7
+/* IRQ work vector: a mechanism that allows running code in IRQ context */
+#define IRQ_WORK_VECTOR			0xf8
 
-#define POSTED_INTR_WAKEUP_VECTOR	0xf1
 /*
- * IRQ work vector:
+ * Local APCI dependent vectors, but also depend on other configurations
+ * (MCE, virtualization, etc)
  */
-#define IRQ_WORK_VECTOR			0xf6
-
+#define THERMAL_APIC_VECTOR		0xf7
+#define THRESHOLD_APIC_VECTOR		0xf6
 #define UV_BAU_MESSAGE			0xf5
 #define DEFERRED_ERROR_VECTOR		0xf4
 
@@ -99,6 +131,9 @@
 #ifdef CONFIG_HAVE_KVM
 #define POSTED_INTR_VECTOR		0xf2
 #endif
+#define POSTED_INTR_WAKEUP_VECTOR	0xf1
+
+/* Vector 0xf0 is not used yet, reserved */
 
 /*
  * Local APIC timer IRQ vector is on a different priority level,
@@ -107,6 +142,9 @@
  */
 #define LOCAL_TIMER_VECTOR		0xef
 
+/* --- end of special vectors definitions ---  */
+
+
 #define NR_VECTORS			 256
 
 #ifdef CONFIG_X86_LOCAL_APIC
-- 
2.4.3

[PATCH 2/3] x86/irq: refactor native_init_IRQ

[Jianyu Zhan]

After prepatory patch(x86/asm/irq: Rearrange definitoin of specical irq vectors and cleanup)
is applied, now refactor native_init_IRQ() per the special irq vectors layout.

Signed-off-by: Jianyu Zhan <nasa4836@gmail.com>
---
 arch/x86/kernel/irqinit.c | 68 ++++++++++++++++++++++++++++-------------------
 1 file changed, 40 insertions(+), 28 deletions(-)

diff --git a/arch/x86/kernel/irqinit.c b/arch/x86/kernel/irqinit.c
index 1423ab1..0e9fa7c 100644
--- a/arch/x86/kernel/irqinit.c
+++ b/arch/x86/kernel/irqinit.c
@@ -121,47 +121,55 @@ static void __init smp_intr_init(void)
 
 	/* IPI used for rebooting/stopping */
 	alloc_intr_gate(REBOOT_VECTOR, reboot_interrupt);
+
+	/* IPI for X86 platform specific use */
+	alloc_intr_gate(X86_PLATFORM_IPI_VECTOR, x86_platform_ipi);
 #endif /* CONFIG_SMP */
 }
 
 static void __init apic_intr_init(void)
 {
-	smp_intr_init();
-
-#ifdef CONFIG_X86_THERMAL_VECTOR
-	alloc_intr_gate(THERMAL_APIC_VECTOR, thermal_interrupt);
-#endif
-#ifdef CONFIG_X86_MCE_THRESHOLD
-	alloc_intr_gate(THRESHOLD_APIC_VECTOR, threshold_interrupt);
-#endif
+#ifdef CONFIG_X86_LOCAL_APIC
 
-#ifdef CONFIG_X86_MCE_AMD
-	alloc_intr_gate(DEFERRED_ERROR_VECTOR, deferred_error_interrupt);
-#endif
+	/* IPI vectors for APIC spurious and error interrupts */
+	alloc_intr_gate(SPURIOUS_APIC_VECTOR, spurious_interrupt);
+	alloc_intr_gate(ERROR_APIC_VECTOR, error_interrupt);
 
-#ifdef CONFIG_X86_LOCAL_APIC
 	/* self generated IPI for local APIC timer */
 	alloc_intr_gate(LOCAL_TIMER_VECTOR, apic_timer_interrupt);
 
-	/* IPI for X86 platform specific use */
-	alloc_intr_gate(X86_PLATFORM_IPI_VECTOR, x86_platform_ipi);
-#ifdef CONFIG_HAVE_KVM
-	/* IPI for KVM to deliver posted interrupt */
-	alloc_intr_gate(POSTED_INTR_VECTOR, kvm_posted_intr_ipi);
-	/* IPI for KVM to deliver interrupt to wake up tasks */
-	alloc_intr_gate(POSTED_INTR_WAKEUP_VECTOR, kvm_posted_intr_wakeup_ipi);
-#endif
+# ifdef CONFIG_X86_THERMAL_VECTOR
+	alloc_intr_gate(THERMAL_APIC_VECTOR, thermal_interrupt);
+# endif
 
-	/* IPI vectors for APIC spurious and error interrupts */
-	alloc_intr_gate(SPURIOUS_APIC_VECTOR, spurious_interrupt);
-	alloc_intr_gate(ERROR_APIC_VECTOR, error_interrupt);
+# ifdef CONFIG_X86_MCE_THRESHOLD
+         alloc_intr_gate(THRESHOLD_APIC_VECTOR, threshold_interrupt);
+# endif
 
-	/* IRQ work interrupts: */
-# ifdef CONFIG_IRQ_WORK
-	alloc_intr_gate(IRQ_WORK_VECTOR, irq_work_interrupt);
+# ifdef CONFIG_X86_MCE_AMD
+         alloc_intr_gate(DEFERRED_ERROR_VECTOR, deferred_error_interrupt);
 # endif
 
+# ifdef CONFIG_HAVE_KVM
+         /* IPI for KVM to deliver posted interrupt */
+         alloc_intr_gate(POSTED_INTR_VECTOR, kvm_posted_intr_ipi);
+         /* IPI for KVM to deliver interrupt to wake up tasks */
+         alloc_intr_gate(POSTED_INTR_WAKEUP_VECTOR, kvm_posted_intr_wakeup_ipi);
+# endif
+
+#endif
+}
+
+/* See asm/irq_vectors.h for sepcial vector definiton */
+static void __init system_intr_init(void)
+{
+	smp_intr_init();
+
+#ifdef CONFIG_IRQ_WORK
+	alloc_intr_gate(IRQ_WORK_VECTOR, irq_work_interrupt);
 #endif
+
+	apic_intr_init();
 }
 
 void __init native_init_IRQ(void)
@@ -171,10 +179,11 @@ void __init native_init_IRQ(void)
 	/* Execute any quirks before the call gates are initialised: */
 	x86_init.irqs.pre_vector_init();
 
-	apic_intr_init();
+	/* First, init system vectors, will update 'first_system_vector' */
+	system_intr_init();
 
 	/*
-	 * Cover the whole vector space, no vector can escape
+	 * Second, cover the whole vector space, no vector can escape
 	 * us. (some of these will be overridden and become
 	 * 'special' SMP interrupts)
 	 */
@@ -187,11 +196,14 @@ void __init native_init_IRQ(void)
 		set_intr_gate(i, irq_entries_start +
 				8 * (i - FIRST_EXTERNAL_VECTOR));
 	}
+
+	/* Third, mark all spare vector as spurious. */
 #ifdef CONFIG_X86_LOCAL_APIC
 	for_each_clear_bit_from(i, used_vectors, NR_VECTORS)
 		set_intr_gate(i, spurious_interrupt);
 #endif
 
+	/* Fourth, fixup for legacy PIC */
 	if (!acpi_ioapic && !of_ioapic && nr_legacy_irqs())
 		setup_irq(2, &irq2);
 
-- 
2.4.3

Re: [PATCH 2/3] x86/irq: refactor native_init_IRQ

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1042 bytes --]

Hi Jianyu,

[auto build test ERROR on tip/x86/core]
[also build test ERROR on v4.5-rc7 next-20160311]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Jianyu-Zhan/x86-irq-Refactor-special-vector-definition-and-cleanup/20160312-231331
config: i386-tinyconfig (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   arch/x86/built-in.o: In function `native_init_IRQ':
>> (.init.text+0xb8d): undefined reference to `first_system_vector'
   arch/x86/built-in.o: In function `native_init_IRQ':
   (.init.text+0xb99): undefined reference to `first_system_vector'
   arch/x86/built-in.o: In function `native_init_IRQ':
>> (.init.text+0xba6): undefined reference to `irq_work_interrupt'

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 6205 bytes --]

Re: [PATCH 2/3] x86/irq: refactor native_init_IRQ

[Jianyu Zhan]

HI,

This patch is based on tip/master.

I have built it locally on my box and did not encounter such problem.

Please recheck.

Regards,
Jianyu Zhan


On Sat, Mar 12, 2016 at 11:30 PM, kbuild test robot <lkp@intel.com> wrote:
> Hi Jianyu,
>
> [auto build test ERROR on tip/x86/core]
> [also build test ERROR on v4.5-rc7 next-20160311]
> [if your patch is applied to the wrong git tree, please drop us a note to help improving the system]
>
> url:    https://github.com/0day-ci/linux/commits/Jianyu-Zhan/x86-irq-Refactor-special-vector-definition-and-cleanup/20160312-231331
> config: i386-tinyconfig (attached as .config)
> reproduce:
>         # save the attached .config to linux build tree
>         make ARCH=i386
>
> All errors (new ones prefixed by >>):
>
>    arch/x86/built-in.o: In function `native_init_IRQ':
>>> (.init.text+0xb8d): undefined reference to `first_system_vector'
>    arch/x86/built-in.o: In function `native_init_IRQ':
>    (.init.text+0xb99): undefined reference to `first_system_vector'
>    arch/x86/built-in.o: In function `native_init_IRQ':
>>> (.init.text+0xba6): undefined reference to `irq_work_interrupt'
>
> ---
> 0-DAY kernel test infrastructure                Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

Re: [PATCH 2/3] x86/irq: refactor native_init_IRQ

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1184 bytes --]

Hi Jianyu,

[auto build test ERROR on tip/x86/core]
[also build test ERROR on v4.5-rc7 next-20160311]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Jianyu-Zhan/x86-irq-Refactor-special-vector-definition-and-cleanup/20160312-231331
config: i386-randconfig-n0-201610 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   arch/x86/built-in.o: In function `native_init_IRQ':
   (.init.text+0x12cf): undefined reference to `first_system_vector'
   arch/x86/built-in.o: In function `native_init_IRQ':
   (.init.text+0x12db): undefined reference to `first_system_vector'
   arch/x86/built-in.o: In function `native_init_IRQ':
   (.init.text+0x12eb): undefined reference to `irq_work_interrupt'
   arch/x86/built-in.o: In function `native_init_IRQ':
>> (.init.text+0x1327): undefined reference to `trace_irq_work_interrupt'

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 23884 bytes --]

[PATCH 3/3] x86/irq: update first_system_vector only when X86_LOCAL_PIC is on

[Jianyu Zhan]

During native_init_IRQ(), we will update first_system_vector conditionally
when we init system vector. But on !CONFIG_X86_LOCAL_PIC, we prefer it
to NR_IRQS, so don't bother set it on this case.

Signed-off-by: Jianyu Zhan <nasa4836@gmail.com>
---
 arch/x86/include/asm/desc.h | 2 ++
 arch/x86/kernel/irqinit.c   | 3 ---
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/x86/include/asm/desc.h b/arch/x86/include/asm/desc.h
index 4e10d73..4fc2deb 100644
--- a/arch/x86/include/asm/desc.h
+++ b/arch/x86/include/asm/desc.h
@@ -383,8 +383,10 @@ static inline void alloc_system_vector(int vector)
 {
 	if (!test_bit(vector, used_vectors)) {
 		set_bit(vector, used_vectors);
+#ifdef CONFIG_X86_LOCAL_APIC
 		if (first_system_vector > vector)
 			first_system_vector = vector;
+#endif
 	} else {
 		BUG();
 	}
diff --git a/arch/x86/kernel/irqinit.c b/arch/x86/kernel/irqinit.c
index 0e9fa7c..e999b38 100644
--- a/arch/x86/kernel/irqinit.c
+++ b/arch/x86/kernel/irqinit.c
@@ -188,9 +188,6 @@ void __init native_init_IRQ(void)
 	 * 'special' SMP interrupts)
 	 */
 	i = FIRST_EXTERNAL_VECTOR;
-#ifndef CONFIG_X86_LOCAL_APIC
-#define first_system_vector NR_VECTORS
-#endif
 	for_each_clear_bit_from(i, used_vectors, first_system_vector) {
 		/* IA32_SYSCALL_VECTOR could be used in trap_init already. */
 		set_intr_gate(i, irq_entries_start +
-- 
2.4.3

Re: [PATCH 3/3] x86/irq: update first_system_vector only when X86_LOCAL_PIC is on

[Thomas Gleixner]

On Sat, 12 Mar 2016, Jianyu Zhan wrote:

> During native_init_IRQ(), we will update first_system_vector conditionally
> when we init system vector. But on !CONFIG_X86_LOCAL_PIC, we prefer it
> to NR_IRQS, so don't bother set it on this case.
> 
> Signed-off-by: Jianyu Zhan <nasa4836@gmail.com>
> ---
>  arch/x86/include/asm/desc.h | 2 ++
>  arch/x86/kernel/irqinit.c   | 3 ---
>  2 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/include/asm/desc.h b/arch/x86/include/asm/desc.h
> index 4e10d73..4fc2deb 100644
> --- a/arch/x86/include/asm/desc.h
> +++ b/arch/x86/include/asm/desc.h
> @@ -383,8 +383,10 @@ static inline void alloc_system_vector(int vector)
>  {
>  	if (!test_bit(vector, used_vectors)) {
>  		set_bit(vector, used_vectors);
> +#ifdef CONFIG_X86_LOCAL_APIC
>  		if (first_system_vector > vector)
>  			first_system_vector = vector;
> +#endif

This is pointless, because it's only called when local apic is enabled as all
call sites of alloc_intr_gate() depend on CONFIG_X86_LOCAL_APIC ....

>  	} else {
>  		BUG();
>  	}
> diff --git a/arch/x86/kernel/irqinit.c b/arch/x86/kernel/irqinit.c
> index 0e9fa7c..e999b38 100644
> --- a/arch/x86/kernel/irqinit.c
> +++ b/arch/x86/kernel/irqinit.c
> @@ -188,9 +188,6 @@ void __init native_init_IRQ(void)
>  	 * 'special' SMP interrupts)
>  	 */
>  	i = FIRST_EXTERNAL_VECTOR;
> -#ifndef CONFIG_X86_LOCAL_APIC
> -#define first_system_vector NR_VECTORS
> -#endif
>  	for_each_clear_bit_from(i, used_vectors, first_system_vector) {

And how exactly is this here supposed to compile when CONFIG_X86_LOCAL_APIC=n?

>  		/* IA32_SYSCALL_VECTOR could be used in trap_init already. */
>  		set_intr_gate(i, irq_entries_start +
> -- 
> 2.4.3
> 
> 

Re: [PATCH 3/3] x86/irq: update first_system_vector only when X86_LOCAL_PIC is on

[Jianyu Zhan]

On Sun, Mar 13, 2016 at 4:08 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> This is pointless, because it's only called when local apic is enabled as all
> call sites of alloc_intr_gate() depend on CONFIG_X86_LOCAL_APIC ....

Not exactly,  currently at least  smp_intr_init()  DOES NOT depend on
CONFIG_X86_LOCAL_APIC:

static void __init smp_intr_init(void)
{
#ifdef CONFIG_SMP
       /*
        * The reschedule interrupt is a CPU-to-CPU reschedule-helper
        * IPI, driven by wakeup.
        */
       alloc_intr_gate(RESCHEDULE_VECTOR, reschedule_interrupt);

       /* IPI for generic function call */
       alloc_intr_gate(CALL_FUNCTION_VECTOR, call_function_interrupt);

...
}

So alloc_intr_gate will be called, and first_system_vector will be updated !

I know this is weird, because modern SMP machines implies Local APIC.
But currently we have CONFIG_SMP detangle from CONFIG_X86_LOCAL_APIC,
which I think is fine.

Another place which is weird is CONFIG_IRQ_WORK.  Technically,  it
does not depend
on SMP,   nor even necessary Local APIC.  Actually, it is just a base
configuration selected
by others.  But currently we have the

#ifdef CONFIG_IRQ_WORK
       alloc_intr_gate(IRQ_WORK_VECTOR, irq_work_interrupt);
#endif

block surrounded by CONFIG_X86_LOCAL_APIC.

In new scheme, I just move it out,  see [2/3] patch.



>
>>       } else {
>>               BUG();
>>       }
>> diff --git a/arch/x86/kernel/irqinit.c b/arch/x86/kernel/irqinit.c
>> index 0e9fa7c..e999b38 100644
>> --- a/arch/x86/kernel/irqinit.c
>> +++ b/arch/x86/kernel/irqinit.c
>> @@ -188,9 +188,6 @@ void __init native_init_IRQ(void)
>>        * 'special' SMP interrupts)
>>        */
>>       i = FIRST_EXTERNAL_VECTOR;
>> -#ifndef CONFIG_X86_LOCAL_APIC
>> -#define first_system_vector NR_VECTORS
>> -#endif
>>       for_each_clear_bit_from(i, used_vectors, first_system_vector) {
>
> And how exactly is this here supposed to compile when CONFIG_X86_LOCAL_APIC=n?

Dunno.  I guess this code on !CONFIG_X86_LOCAL_APIC case hasn't been
tested yet ?

 first_system_vector is a global variable, and is initially assigned
to FIRST_SYSTEM_VECTOR:

int first_system_vector = FIRST_SYSTEM_VECTOR;

#ifdef CONFIG_X86_LOCAL_APIC
#define FIRST_SYSTEM_VECTOR             LOCAL_TIMER_VECTOR
#else
#define FIRST_SYSTEM_VECTOR             NR_VECTORS
#endif

For CONFIG_X86_LOCAL_APIC case,  the define makes sense.
But for ! CONFIG_X86_LOCAL_APIC case,  why we confine it to NR_VECTORS
is a mystery
to me.  Have digged into git history, but found no proof.

So to maintain consistency, this patch just retain what it is,  but we
do not bother update it for
!CONFIG_X86_LOCAL_APIC case.

Regards,
Jianyu Zhan

Re: [PATCH 3/3] x86/irq: update first_system_vector only when X86_LOCAL_PIC is on

[Thomas Gleixner]

On Sun, 13 Mar 2016, Jianyu Zhan wrote:

> On Sun, Mar 13, 2016 at 4:08 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > This is pointless, because it's only called when local apic is enabled as all
> > call sites of alloc_intr_gate() depend on CONFIG_X86_LOCAL_APIC ....
> 
> Not exactly,  currently at least  smp_intr_init()  DOES NOT depend on
> CONFIG_X86_LOCAL_APIC:
> 
> static void __init smp_intr_init(void)
> {
> #ifdef CONFIG_SMP

It does, because CONFIG_SMP enables CONFIG_X86_LOCAL_APIC

> I know this is weird, because modern SMP machines implies Local APIC.
> But currently we have CONFIG_SMP detangle from CONFIG_X86_LOCAL_APIC,
> which I think is fine.

Do you actually understand how all that works together?
 
> Another place which is weird is CONFIG_IRQ_WORK.  Technically,  it
> does not depend
> on SMP,   nor even necessary Local APIC.  Actually, it is just a base
> configuration selected
> by others.  But currently we have the

Have you tried to enable it independent from CONFIG_X86_LOCAL_APIC?
 
> >>       i = FIRST_EXTERNAL_VECTOR;
> >> -#ifndef CONFIG_X86_LOCAL_APIC
> >> -#define first_system_vector NR_VECTORS
> >> -#endif
> >>       for_each_clear_bit_from(i, used_vectors, first_system_vector) {
> >
> > And how exactly is this here supposed to compile when CONFIG_X86_LOCAL_APIC=n?
> 
> Dunno.  I guess this code on !CONFIG_X86_LOCAL_APIC case hasn't been
> tested yet ?

It's your job to at least compile test your patches not the job of others.
 
> For CONFIG_X86_LOCAL_APIC case,  the define makes sense.
> But for ! CONFIG_X86_LOCAL_APIC case,  why we confine it to NR_VECTORS
> is a mystery
> to me.  Have digged into git history, but found no proof.

And because it's a mystery you can just change it as you think it's fine and
thereby break the build?
 
> So to maintain consistency, this patch just retain what it is,  but we
> do not bother update it for
> !CONFIG_X86_LOCAL_APIC case.

To maintain consistency we leave it as is, because that actually compiles AND
works.

Thanks,

	tglx

Re: [PATCH 3/3] x86/irq: update first_system_vector only when X86_LOCAL_PIC is on

[Jianyu Zhan]

On Sun, Mar 13, 2016 at 2:35 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
>> On Sun, Mar 13, 2016 at 4:08 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
>> > This is pointless, because it's only called when local apic is enabled as all
>> > call sites of alloc_intr_gate() depend on CONFIG_X86_LOCAL_APIC ....
>>
>> Not exactly,  currently at least  smp_intr_init()  DOES NOT depend on
>> CONFIG_X86_LOCAL_APIC:
>>
>> static void __init smp_intr_init(void)
>> {
>> #ifdef CONFIG_SMP
>
> It does, because CONFIG_SMP enables CONFIG_X86_LOCAL_APIC
>

It is.  Once CONFIG_SMP is on, CONFIG_X86_LOCAL_APIC will be turned on.

So the situation should be described as:   first_system_vector will be updated
on system vector init,  on SMP case(which indirectly implies
CONFIG_X86_LOCAL_APIC)
and on explicit CONFIG_X86_LOCAL_APIC(X86_UP_APIC).

So initial code:

#ifndef CONFIG_X86_LOCAL_APIC
#define first_system_vector NR_VECTORS
#endif

is actually not needed,  becaused it won't ever change on !
CONFIG_X86_LOCAL_APIC
case.

I will clarify this in v2.

>> I know this is weird, because modern SMP machines implies Local APIC.
>> But currently we have CONFIG_SMP detangle from CONFIG_X86_LOCAL_APIC,
>> which I think is fine.
>
> Do you actually understand how all that works together?
>

So the dependency should be reversed, and it should be like this:

Currently we have CONFIG_X86_LOCAL_APIC detangle from  CONFIG_SMP
(we can enable CONFIG_X86_LOCAL_APIC on 32-bit uniprocessor).


>> Another place which is weird is CONFIG_IRQ_WORK.  Technically,  it
>> does not depend
>> on SMP,   nor even necessary Local APIC.  Actually, it is just a base
>> configuration selected
>> by others.  But currently we have the
>
> Have you tried to enable it independent from CONFIG_X86_LOCAL_APIC?
>

I did.  I can turn off SMP and  did not use UP_APIC.  But IRQ_WORK is still
able be turn on(by selected by other subsystem).

My purpose of posting this patch set is trying to make the system vector layout
reveal this fact.

we have SMP system vector defined first( these may rely on or not rely
on CONFIG_X86_LOCAL_APIC),

then comes the CONFIG_X86_LOCAL_APIC dependent vector definition.

then comes the rest vector definition that not only depends on
CONFIG_X86_LOCAL_APIC, but
also others(like MCE, virtualization).

Among these stands out IRQ_WORK,   which neither depends on SMP nor
CONFIG_X86_LOCAL_APIC.

So the new init funciton is look like(from [2/3]):

static void __init system_intr_init(void)
{
       smp_intr_init();

#ifdef CONFIG_IRQ_WORK
       alloc_intr_gate(IRQ_WORK_VECTOR, irq_work_interrupt);
#endif

       apic_intr_init();
}


>> >>       i = FIRST_EXTERNAL_VECTOR;
>> >> -#ifndef CONFIG_X86_LOCAL_APIC
>> >> -#define first_system_vector NR_VECTORS
>> >> -#endif
>> >>       for_each_clear_bit_from(i, used_vectors, first_system_vector) {
>> >
>> > And how exactly is this here supposed to compile when CONFIG_X86_LOCAL_APIC=n?
>>
>> Dunno.  I guess this code on !CONFIG_X86_LOCAL_APIC case hasn't been
>> tested yet ?
>
> It's your job to at least compile test your patches not the job of others.
>

Will do in next round.






Regards,
Jianyu Zhan

Re: [PATCH 3/3] x86/irq: update first_system_vector only when X86_LOCAL_PIC is on

[Jianyu Zhan]

On Sun, Mar 13, 2016 at 3:25 PM, Jianyu Zhan <nasa4836@gmail.com> wrote:
> My purpose of posting this patch set is trying to make the system vector layout
> reveal this fact.
>
> we have SMP system vector defined first( these may rely on or not rely
> on CONFIG_X86_LOCAL_APIC),
>
> then comes the CONFIG_X86_LOCAL_APIC dependent vector definition.
>
> then comes the rest vector definition that not only depends on
> CONFIG_X86_LOCAL_APIC, but
> also others(like MCE, virtualization).
>
> Among these stands out IRQ_WORK,   which neither depends on SMP nor
> CONFIG_X86_LOCAL_APIC.
>

My wording is incorrect,  should be like this:

The new system vector layout:

First, two special vector:  spurious and error.

Second, we have SMP system vector defined first( these implicitly
reply on CONFIG_X86_LOCAL_APIC),

Third, comes the CONFIG_X86_LOCAL_APIC dependent vector definition,
but not necessary
SMP dependent.( Among these stands out IRQ_WORK,   which neither
depends on SMP nor
CONFIG_X86_LOCAL_APIC.)

Finally,  comes the rest vector definition that not only depends on
CONFIG_X86_LOCAL_APIC, but
also others(like MCE, virtualization).





Regards,
Jianyu Zhan

Re: [PATCH 3/3] x86/irq: update first_system_vector only when X86_LOCAL_PIC is on

[Thomas Gleixner]

On Sun, 13 Mar 2016, Jianyu Zhan wrote:
> On Sun, Mar 13, 2016 at 2:35 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> >> On Sun, Mar 13, 2016 at 4:08 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> >> > This is pointless, because it's only called when local apic is enabled as all
> >> > call sites of alloc_intr_gate() depend on CONFIG_X86_LOCAL_APIC ....
> >>
> >> Not exactly,  currently at least  smp_intr_init()  DOES NOT depend on
> >> CONFIG_X86_LOCAL_APIC:
> >>
> >> static void __init smp_intr_init(void)
> >> {
> >> #ifdef CONFIG_SMP
> >
> > It does, because CONFIG_SMP enables CONFIG_X86_LOCAL_APIC
> >
> 
> It is.  Once CONFIG_SMP is on, CONFIG_X86_LOCAL_APIC will be turned on.
> 
> So the situation should be described as:   first_system_vector will be updated
> on system vector init,  on SMP case(which indirectly implies
> CONFIG_X86_LOCAL_APIC)
> and on explicit CONFIG_X86_LOCAL_APIC(X86_UP_APIC).
> 
> So initial code:
> 
> #ifndef CONFIG_X86_LOCAL_APIC
> #define first_system_vector NR_VECTORS
> #endif
> 
> is actually not needed,  becaused it won't ever change on !
> CONFIG_X86_LOCAL_APIC
> case.

It will never ever be updated in that case, simply because nothing uses it.
 
> > Do you actually understand how all that works together?
> >
> 
> So the dependency should be reversed, and it should be like this:
> 
> Currently we have CONFIG_X86_LOCAL_APIC detangle from  CONFIG_SMP
> (we can enable CONFIG_X86_LOCAL_APIC on 32-bit uniprocessor).

And what's the benefit of this?
 
> Among these stands out IRQ_WORK,   which neither depends on SMP nor
> CONFIG_X86_LOCAL_APIC.
> 
> So the new init funciton is look like(from [2/3]):
> 
> static void __init system_intr_init(void)
> {
>        smp_intr_init();
> 
> #ifdef CONFIG_IRQ_WORK
>        alloc_intr_gate(IRQ_WORK_VECTOR, irq_work_interrupt);
> #endif

And that is completely wrong. IRQ_WORK can work independent of LOCAL_APIC, but
if LOCAL_APIC is disabled it does not use the interrupt, simply because there
is no way to trigger it. That setup is inside #ifdef CONFIG_X86_LOCAL_APIC for
exactly that reason.

Just because IRQ_WORK has no config dependency on LOCAL APIC that does not
mean it uses the interrupt gate unconditionally.

The code is correct as is and there is no reason to shuffle it in circles for
no value.

Thanks,

	tglx

Re: [PATCH 3/3] x86/irq: update first_system_vector only when X86_LOCAL_PIC is on

[Jianyu Zhan]

On Sun, Mar 13, 2016 at 3:55 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> if LOCAL_APIC is disabled it does not use the interrupt, simply because there
> is no way to trigger it. That setup is inside #ifdef CONFIG_X86_LOCAL_APIC for
> exactly that reason.
>
> Just because IRQ_WORK has no config dependency on LOCAL APIC that does not
> mean it uses the interrupt gate unconditionally.
>

Thanks for clarification.

I think IRQ_WORK works as generic hardirq context callbacks, it should reply on
self IPI,  which is a functionality provided by LOCAL_APIC, while
legacy PIC doesn't
provide this(correct?).

If so,  it really makes sense to enable IRQ_WORK only when X86_LOCAL_APIC,
and I think we should make CONFIG_IRQ_WORK depend on this.


> The code is correct as is and there is no reason to shuffle it in circles for
> no value.

Will the below layout make sense?


*  Layout:
*  0xff, 0xfe:
*      Two highest vectors, granted for spurious vector and error vector.
*  0xfd - 0xf9:
*      CONFIG_SMP dependent vectors. On morden machines these are achieved
*      via local APIC, so these imply CONFIG_X86_LOCAL_APIC.
*
*  0xf8 - 0xf0:
*      CONFIG_X86_LOCAL_APIC dependent vectors, but these do not necessarily
*      depend on CONFIG_SMP, so are seperated from above.
*      Some are only depending on CONFIG_X86_LOCAL_APIC, but some are depending
*      on more(MCE, Virtualization, etc).
*
*     Note:  CONFIG_IRQ_WORK replies on CONFIG_X86_LOCAL_APIC(for self
IPI),  though it could
*                 be turned on  ! CONFIG_X86_LOCAL_APIC.
*  0xef:
*      Local APIC timer vector.



Regards,
Jianyu Zhan

Re: [PATCH 3/3] x86/irq: update first_system_vector only when X86_LOCAL_PIC is on

[Thomas Gleixner]

On Sun, 13 Mar 2016, Jianyu Zhan wrote:
> On Sun, Mar 13, 2016 at 3:55 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > if LOCAL_APIC is disabled it does not use the interrupt, simply because there
> > is no way to trigger it. That setup is inside #ifdef CONFIG_X86_LOCAL_APIC for
> > exactly that reason.
> >
> > Just because IRQ_WORK has no config dependency on LOCAL APIC that does not
> > mean it uses the interrupt gate unconditionally.
> >
> 
> Thanks for clarification.
> 
> I think IRQ_WORK works as generic hardirq context callbacks, it should reply on
> self IPI,  which is a functionality provided by LOCAL_APIC, while
> legacy PIC doesn't
> provide this(correct?).

As I said before IRQ_WORK can work w/o APIC. And therefor IRQ_WORK does NOT
depend on APIC.
 
End of story. Nothing to change here at all.

Thanks,

	tglx

Re: [PATCH 3/3] x86/irq: update first_system_vector only when X86_LOCAL_PIC is on

[Jianyu Zhan]

On Sun, Mar 13, 2016 at 5:11 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Sun, 13 Mar 2016, Jianyu Zhan wrote:
>> On Sun, Mar 13, 2016 at 3:55 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
>> > if LOCAL_APIC is disabled it does not use the interrupt, simply because there
>> > is no way to trigger it. That setup is inside #ifdef CONFIG_X86_LOCAL_APIC for
>> > exactly that reason.
>> >
>> > Just because IRQ_WORK has no config dependency on LOCAL APIC that does not
>> > mean it uses the interrupt gate unconditionally.
>> >
>>
>> Thanks for clarification.
>>
>> I think IRQ_WORK works as generic hardirq context callbacks, it should reply on
>> self IPI,  which is a functionality provided by LOCAL_APIC, while
>> legacy PIC doesn't
>> provide this(correct?).
>
> As I said before IRQ_WORK can work w/o APIC. And therefor IRQ_WORK does NOT
> depend on APIC.
>
> End of story. Nothing to change here at all.

If so, then it is weird, because in current code, IRQ_WORK vector init
is surrounded by
CONFIG_X86_LOCAL_APIC.  And actually my patch did move it out.

After all,  thanks for all the clarifications :-).

Regards,
Jianyu Zhan

Re: [PATCH 3/3] x86/irq: update first_system_vector only when X86_LOCAL_PIC is on

[Thomas Gleixner]

On Sun, 13 Mar 2016, Jianyu Zhan wrote:
> On Sun, Mar 13, 2016 at 5:11 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > On Sun, 13 Mar 2016, Jianyu Zhan wrote:
> >> On Sun, Mar 13, 2016 at 3:55 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> >> > if LOCAL_APIC is disabled it does not use the interrupt, simply because there
> >> > is no way to trigger it. That setup is inside #ifdef CONFIG_X86_LOCAL_APIC for
> >> > exactly that reason.
> >> >
> >> > Just because IRQ_WORK has no config dependency on LOCAL APIC that does not
> >> > mean it uses the interrupt gate unconditionally.
> >> >
> >>
> >> Thanks for clarification.
> >>
> >> I think IRQ_WORK works as generic hardirq context callbacks, it should reply on
> >> self IPI,  which is a functionality provided by LOCAL_APIC, while
> >> legacy PIC doesn't
> >> provide this(correct?).
> >
> > As I said before IRQ_WORK can work w/o APIC. And therefor IRQ_WORK does NOT
> > depend on APIC.
> >
> > End of story. Nothing to change here at all.
> 
> If so, then it is weird, because in current code, IRQ_WORK vector init
> is surrounded by
> CONFIG_X86_LOCAL_APIC.  And actually my patch did move it out.

Sigh. Do you actaully read what I write?

      IRQ_WORK can work w/o APIC

Emphasis on CAN. If the APIC is available it's used, if not then there is no
point in setting up the gate for nothing.

So why would your patch do any good?

Thanks,

	tglx

Re: [PATCH 3/3] x86/irq: update first_system_vector only when X86_LOCAL_PIC is on

[Jianyu Zhan]

On Sun, Mar 13, 2016 at 5:33 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
>       IRQ_WORK can work w/o APIC
>
> Emphasis on CAN. If the APIC is available it's used, if not then there is no
> point in setting up the gate for nothing.
>
> So why would your patch do any good?

I understood it is no point setting up if  APIC is not available,  but
just got confused by
your wording 'can',  now all clear.

As for the patch set.   My initial purpose is just wanting to make the
layout clear and
clean up stale comments and dead code:

#ifndef CONFIG_X86_LOCAL_APIC
#define first_system_vector NR_VECTORS
#endif

as we've talked about this before,  it won't ever be change on
!CONFIG_X86_LOCAL_APIC,
so no point define it here(it is initialized to NR_VECTORS).

Since all points are clear now, if  the above purpose still make sense.
I will  respin this patch set.

Thank you for your patient explanation .:-)


Regards,
Jianyu Zhan

Re: [PATCH 3/3] x86/irq: update first_system_vector only when X86_LOCAL_PIC is on

[Thomas Gleixner]

On Sun, 13 Mar 2016, Jianyu Zhan wrote:
> On Sun, Mar 13, 2016 at 5:33 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> >       IRQ_WORK can work w/o APIC
> >
> > Emphasis on CAN. If the APIC is available it's used, if not then there is no
> > point in setting up the gate for nothing.
> >
> > So why would your patch do any good?
> 
> I understood it is no point setting up if  APIC is not available,  but
> just got confused by
> your wording 'can',  now all clear.
> 
> As for the patch set.   My initial purpose is just wanting to make the
> layout clear and
> clean up stale comments and dead code:
> 
> #ifndef CONFIG_X86_LOCAL_APIC
> #define first_system_vector NR_VECTORS
> #endif
> 
> as we've talked about this before,  it won't ever be change on
> !CONFIG_X86_LOCAL_APIC,
> so no point define it here(it is initialized to NR_VECTORS).

Hell no. If CONFIG_X86_LOCAL_APIC=n then first_system_vector is not a variable
and not available and not initialized at all.

> Since all points are clear now, 

Obviously not.