[PATCH 2/3] arm: introduce IRQ stacks

[PATCH 2/3] arm: introduce IRQ stacks

[Maninder Singh]

This patch adds code for switching to IRQ stack.
IRQ stack and Kernel SVC stack have below design.

IRQ STACK:-
                    ------------ IRQ stack top
                    |          |
                    ------------
                    .          .
                    .          .
                    .          .
                    ------------
                    |    sp    | <- irq_stack_base + 0x8
                    ------------
                    |    fp    | <- irq_stack_base + 0x4
                    ------------
                    |tinfo_ptr | /* pointer to thread info */
irq_stack_ptr -->   ------------ IRQ stack base

Kernel SVC stack:-
                    ------------  Kernel stack top
                    |          |
                    ------------
                    .          .
                    .          .
                    .          .
                    ------------
                    |          |
                    |          |
                    ------------
                    |tinfo_ptr |  /* pointer to thread info */
                    ------------ Kernel stack base

Co-developed-by: Vaneet Narang <v.narang@samsung.com>
Signed-off-by: Vaneet Narang <v.narang@samsung.com>
Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
---
 arch/arm/include/asm/assembler.h |  8 ++++++++
 arch/arm/include/asm/irq.h       |  6 ++++++
 arch/arm/kernel/entry-armv.S     | 41 +++++++++++++++++++++++++++++++++++++++-
 arch/arm/kernel/irq.c            | 24 +++++++++++++++++++++++
 4 files changed, 78 insertions(+), 1 deletion(-)

diff --git a/arch/arm/include/asm/assembler.h b/arch/arm/include/asm/assembler.h
index 8512bdc..82ee6ee 100644
--- a/arch/arm/include/asm/assembler.h
+++ b/arch/arm/include/asm/assembler.h
@@ -212,6 +212,14 @@
 #endif
 	.endm
 
+	.macro	this_cpu_ptr, sym, reg, tmp
+	ldr	\reg, =\sym
+#if defined(CONFIG_SMP) && !defined(CONFIG_CPU_V6)
+	mrc	p15, 0, \tmp, cr13, c0, 4
+	add	\reg, \reg, \tmp
+#endif
+	.endm
+
 /*
  * Increment/decrement the preempt count.
  */
diff --git a/arch/arm/include/asm/irq.h b/arch/arm/include/asm/irq.h
index 46d4114..f3299ab 100644
--- a/arch/arm/include/asm/irq.h
+++ b/arch/arm/include/asm/irq.h
@@ -22,10 +22,16 @@
 #define NO_IRQ	((unsigned int)(-1))
 #endif
 
+#define IRQ_STACK_SIZE	THREAD_SIZE
+
 #ifndef __ASSEMBLY__
 struct irqaction;
 struct pt_regs;
 
+#ifdef CONFIG_IRQ_STACK
+DECLARE_PER_CPU(unsigned long *, irq_stack_ptr);
+#endif
+
 extern void asm_do_IRQ(unsigned int, struct pt_regs *);
 void handle_IRQ(unsigned int, struct pt_regs *);
 void init_IRQ(void);
diff --git a/arch/arm/kernel/entry-armv.S b/arch/arm/kernel/entry-armv.S
index 55a47df..13a5889 100644
--- a/arch/arm/kernel/entry-armv.S
+++ b/arch/arm/kernel/entry-armv.S
@@ -32,6 +32,43 @@
 #include "entry-header.S"
 #include <asm/entry-macro-multi.S>
 #include <asm/probes.h>
+#ifdef CONFIG_IRQ_STACK
+#include <asm/irq.h>
+#endif
+
+	.macro  irq_stack_entry
+#ifdef CONFIG_IRQ_STACK
+	mov	r6, sp	/* preserve sp */
+
+	this_cpu_ptr irq_stack_ptr, r7, r8
+	ldr	r7, [r7]
+	mov	r8, r7
+
+	/*
+	 * Compare sp with base of IRQ stack.
+	 * if the top ~(#THREAD_SIZE_ORDER + PAGE_SHIFT) bits match,
+	 * we are on a irq stack.
+	 */
+	eor	r8, r8, sp
+	lsrs	r8, #THREAD_SIZE_ORDER + PAGE_SHIFT
+	beq	9998f
+
+	/*
+	 * store thread info pointer on IRQ stack and
+	 * switch to the irq stack.
+	 */
+	get_thread_info r8
+	stm	r7, {r8, fp, sp}
+	add	sp, r7, #IRQ_STACK_SIZE
+9998:
+#endif
+        .endm
+
+	.macro	irq_stack_exit
+#ifdef CONFIG_IRQ_STACK
+	mov	sp, r6	/* restore sp */
+#endif
+       .endm
 
 /*
  * Interrupt handling.
@@ -41,11 +78,13 @@
 	ldr	r1, =handle_arch_irq
 	mov	r0, sp
 	badr	lr, 9997f
+	irq_stack_entry
 	ldr	pc, [r1]
+9997:
+	irq_stack_exit
 #else
 	arch_irq_handler_default
 #endif
-9997:
 	.endm
 
 	.macro	pabt_helper
diff --git a/arch/arm/kernel/irq.c b/arch/arm/kernel/irq.c
index 698b6f6..79872e5 100644
--- a/arch/arm/kernel/irq.c
+++ b/arch/arm/kernel/irq.c
@@ -43,6 +43,15 @@
 
 unsigned long irq_err_count;
 
+#ifdef CONFIG_IRQ_STACK
+DEFINE_PER_CPU(unsigned long *, irq_stack_ptr);
+
+/*
+ * irq_stack must be IRQ_STACK_SIZE(THREAD_SIZE) aligned,
+ */
+DEFINE_PER_CPU(unsigned long [IRQ_STACK_SIZE/sizeof(long)], irq_stack) __aligned(IRQ_STACK_SIZE);
+#endif
+
 int arch_show_interrupts(struct seq_file *p, int prec)
 {
 #ifdef CONFIG_FIQ
@@ -55,6 +64,20 @@ int arch_show_interrupts(struct seq_file *p, int prec)
 	return 0;
 }
 
+#ifdef CONFIG_IRQ_STACK
+static void init_irq_stacks(void)
+{
+	int cpu;
+
+	for_each_possible_cpu(cpu)
+		per_cpu(irq_stack_ptr, cpu) = per_cpu(irq_stack, cpu);
+}
+#else
+static inline void init_irq_stacks(void)
+{
+}
+#endif
+
 /*
  * handle_IRQ handles all hardware IRQ's.  Decoded IRQs should
  * not come via this function.  Instead, they should provide their
@@ -79,6 +102,7 @@ void __init init_IRQ(void)
 {
 	int ret;
 
+	init_irq_stacks();
 	if (IS_ENABLED(CONFIG_OF) && !machine_desc->init_irq)
 		irqchip_init();
 	else
-- 
1.9.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/3] arm: introduce IRQ stacks

[Maninder Singh]

This patch adds code for switching to IRQ stack.
IRQ stack and Kernel SVC stack have below design.

IRQ STACK:-
                    ------------ IRQ stack top
                    |          |
                    ------------
                    .          .
                    .          .
                    .          .
                    ------------
                    |    sp    | <- irq_stack_base + 0x8
                    ------------
                    |    fp    | <- irq_stack_base + 0x4
                    ------------
                    |tinfo_ptr | /* pointer to thread info */
irq_stack_ptr -->   ------------ IRQ stack base

Kernel SVC stack:-
                    ------------  Kernel stack top
                    |          |
                    ------------
                    .          .
                    .          .
                    .          .
                    ------------
                    |          |
                    |          |
                    ------------
                    |tinfo_ptr |  /* pointer to thread info */
                    ------------ Kernel stack base

Co-developed-by: Vaneet Narang <v.narang@samsung.com>
Signed-off-by: Vaneet Narang <v.narang@samsung.com>
Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
---
 arch/arm/include/asm/assembler.h |  8 ++++++++
 arch/arm/include/asm/irq.h       |  6 ++++++
 arch/arm/kernel/entry-armv.S     | 41 +++++++++++++++++++++++++++++++++++++++-
 arch/arm/kernel/irq.c            | 24 +++++++++++++++++++++++
 4 files changed, 78 insertions(+), 1 deletion(-)

diff --git a/arch/arm/include/asm/assembler.h b/arch/arm/include/asm/assembler.h
index 8512bdc..82ee6ee 100644
--- a/arch/arm/include/asm/assembler.h
+++ b/arch/arm/include/asm/assembler.h
@@ -212,6 +212,14 @@
 #endif
 	.endm
 
+	.macro	this_cpu_ptr, sym, reg, tmp
+	ldr	\reg, =\sym
+#if defined(CONFIG_SMP) && !defined(CONFIG_CPU_V6)
+	mrc	p15, 0, \tmp, cr13, c0, 4
+	add	\reg, \reg, \tmp
+#endif
+	.endm
+
 /*
  * Increment/decrement the preempt count.
  */
diff --git a/arch/arm/include/asm/irq.h b/arch/arm/include/asm/irq.h
index 46d4114..f3299ab 100644
--- a/arch/arm/include/asm/irq.h
+++ b/arch/arm/include/asm/irq.h
@@ -22,10 +22,16 @@
 #define NO_IRQ	((unsigned int)(-1))
 #endif
 
+#define IRQ_STACK_SIZE	THREAD_SIZE
+
 #ifndef __ASSEMBLY__
 struct irqaction;
 struct pt_regs;
 
+#ifdef CONFIG_IRQ_STACK
+DECLARE_PER_CPU(unsigned long *, irq_stack_ptr);
+#endif
+
 extern void asm_do_IRQ(unsigned int, struct pt_regs *);
 void handle_IRQ(unsigned int, struct pt_regs *);
 void init_IRQ(void);
diff --git a/arch/arm/kernel/entry-armv.S b/arch/arm/kernel/entry-armv.S
index 55a47df..13a5889 100644
--- a/arch/arm/kernel/entry-armv.S
+++ b/arch/arm/kernel/entry-armv.S
@@ -32,6 +32,43 @@
 #include "entry-header.S"
 #include <asm/entry-macro-multi.S>
 #include <asm/probes.h>
+#ifdef CONFIG_IRQ_STACK
+#include <asm/irq.h>
+#endif
+
+	.macro  irq_stack_entry
+#ifdef CONFIG_IRQ_STACK
+	mov	r6, sp	/* preserve sp */
+
+	this_cpu_ptr irq_stack_ptr, r7, r8
+	ldr	r7, [r7]
+	mov	r8, r7
+
+	/*
+	 * Compare sp with base of IRQ stack.
+	 * if the top ~(#THREAD_SIZE_ORDER + PAGE_SHIFT) bits match,
+	 * we are on a irq stack.
+	 */
+	eor	r8, r8, sp
+	lsrs	r8, #THREAD_SIZE_ORDER + PAGE_SHIFT
+	beq	9998f
+
+	/*
+	 * store thread info pointer on IRQ stack and
+	 * switch to the irq stack.
+	 */
+	get_thread_info r8
+	stm	r7, {r8, fp, sp}
+	add	sp, r7, #IRQ_STACK_SIZE
+9998:
+#endif
+        .endm
+
+	.macro	irq_stack_exit
+#ifdef CONFIG_IRQ_STACK
+	mov	sp, r6	/* restore sp */
+#endif
+       .endm
 
 /*
  * Interrupt handling.
@@ -41,11 +78,13 @@
 	ldr	r1, =handle_arch_irq
 	mov	r0, sp
 	badr	lr, 9997f
+	irq_stack_entry
 	ldr	pc, [r1]
+9997:
+	irq_stack_exit
 #else
 	arch_irq_handler_default
 #endif
-9997:
 	.endm
 
 	.macro	pabt_helper
diff --git a/arch/arm/kernel/irq.c b/arch/arm/kernel/irq.c
index 698b6f6..79872e5 100644
--- a/arch/arm/kernel/irq.c
+++ b/arch/arm/kernel/irq.c
@@ -43,6 +43,15 @@
 
 unsigned long irq_err_count;
 
+#ifdef CONFIG_IRQ_STACK
+DEFINE_PER_CPU(unsigned long *, irq_stack_ptr);
+
+/*
+ * irq_stack must be IRQ_STACK_SIZE(THREAD_SIZE) aligned,
+ */
+DEFINE_PER_CPU(unsigned long [IRQ_STACK_SIZE/sizeof(long)], irq_stack) __aligned(IRQ_STACK_SIZE);
+#endif
+
 int arch_show_interrupts(struct seq_file *p, int prec)
 {
 #ifdef CONFIG_FIQ
@@ -55,6 +64,20 @@ int arch_show_interrupts(struct seq_file *p, int prec)
 	return 0;
 }
 
+#ifdef CONFIG_IRQ_STACK
+static void init_irq_stacks(void)
+{
+	int cpu;
+
+	for_each_possible_cpu(cpu)
+		per_cpu(irq_stack_ptr, cpu) = per_cpu(irq_stack, cpu);
+}
+#else
+static inline void init_irq_stacks(void)
+{
+}
+#endif
+
 /*
  * handle_IRQ handles all hardware IRQ's.  Decoded IRQs should
  * not come via this function.  Instead, they should provide their
@@ -79,6 +102,7 @@ void __init init_IRQ(void)
 {
 	int ret;
 
+	init_irq_stacks();
 	if (IS_ENABLED(CONFIG_OF) && !machine_desc->init_irq)
 		irqchip_init();
 	else
-- 
1.9.1

Re: [PATCH 2/3] arm: introduce IRQ stacks

[kernel test robot]

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

CC: kbuild-all(a)lists.01.org
In-Reply-To: <1602141333-17822-3-git-send-email-maninder1.s@samsung.com>
References: <1602141333-17822-3-git-send-email-maninder1.s@samsung.com>
TO: Maninder Singh <maninder1.s@samsung.com>
TO: linux(a)armlinux.org.uk
TO: ndesaulniers(a)google.com
TO: caij2003(a)gmail.com
TO: tglx(a)linutronix.de
TO: bigeasy(a)linutronix.de
TO: maz(a)kernel.org
TO: valentin.schneider(a)arm.com
TO: vincent.whitchurch(a)axis.com
TO: nhuck(a)google.com
TO: akpm(a)linux-foundation.org

Hi Maninder,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on arm/for-next]
[also build test WARNING on linus/master v5.9-rc8 next-20201008]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Maninder-Singh/IRQ-stack-support-for-ARM/20201008-151928
base:   git://git.armlinux.org.uk/~rmk/linux-arm.git for-next
:::::: branch date: 6 hours ago
:::::: commit date: 6 hours ago
compiler: arm-linux-gnueabi-gcc (GCC) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


vim +53 arch/arm/kernel/irq.c

b503b88302e9143 Maninder Singh 2020-10-08  49  
b503b88302e9143 Maninder Singh 2020-10-08  50  /*
b503b88302e9143 Maninder Singh 2020-10-08  51   * irq_stack must be IRQ_STACK_SIZE(THREAD_SIZE) aligned,
b503b88302e9143 Maninder Singh 2020-10-08  52   */
b503b88302e9143 Maninder Singh 2020-10-08 @53  DEFINE_PER_CPU(unsigned long [IRQ_STACK_SIZE/sizeof(long)], irq_stack) __aligned(IRQ_STACK_SIZE);
b503b88302e9143 Maninder Singh 2020-10-08  54  #endif
b503b88302e9143 Maninder Singh 2020-10-08  55  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

Re: [PATCH 2/3] arm: introduce IRQ stacks

[Arnd Bergmann]

(also resending this reply from @kernel.org)

On Fri, Oct 16, 2020 at 12:19 PM Arnd Bergmann <arnd@arndb.de> wrote:
> On Thu, Oct 8, 2020 at 9:20 AM Maninder Singh <maninder1.s@samsung.com> wrote:
> >
> > This patch adds code for switching to IRQ stack.
> > IRQ stack and Kernel SVC stack have below design.
> >
> > IRQ STACK:-
> >                     ------------ IRQ stack top
> >                     |          |
> >                     ------------
> >                     .          .
> >                     .          .
> >                     .          .
> >                     ------------
> >                     |    sp    | <- irq_stack_base + 0x8
> >                     ------------
> >                     |    fp    | <- irq_stack_base + 0x4
> >                     ------------
> >                     |tinfo_ptr | /* pointer to thread info */
> > irq_stack_ptr -->   ------------ IRQ stack base
> >
> > Kernel SVC stack:-
> >                     ------------  Kernel stack top
> >                     |          |
> >                     ------------
> >                     .          .
> >                     .          .
> >                     .          .
> >                     ------------
> >                     |          |
> >                     |          |
> >                     ------------
> >                     |tinfo_ptr |  /* pointer to thread info */
> >                     ------------ Kernel stack base
>
> The extra indirection doesn't look great, and I don't see any of the
> other architectures need that. Since we can access percpu data
> without going through thread_info, maybe doing the same as
> x86 would work here:
>
> - define 'current' as 'this_cpu_read_stable(current_task);'
> - convert to CONFIG_THREAD_INFO_IN_TASK
>
>        Arnd

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] arm: introduce IRQ stacks

[Arnd Bergmann]

(also resending this reply from @kernel.org)

On Fri, Oct 16, 2020 at 12:19 PM Arnd Bergmann <arnd@arndb.de> wrote:
> On Thu, Oct 8, 2020 at 9:20 AM Maninder Singh <maninder1.s@samsung.com> wrote:
> >
> > This patch adds code for switching to IRQ stack.
> > IRQ stack and Kernel SVC stack have below design.
> >
> > IRQ STACK:-
> >                     ------------ IRQ stack top
> >                     |          |
> >                     ------------
> >                     .          .
> >                     .          .
> >                     .          .
> >                     ------------
> >                     |    sp    | <- irq_stack_base + 0x8
> >                     ------------
> >                     |    fp    | <- irq_stack_base + 0x4
> >                     ------------
> >                     |tinfo_ptr | /* pointer to thread info */
> > irq_stack_ptr -->   ------------ IRQ stack base
> >
> > Kernel SVC stack:-
> >                     ------------  Kernel stack top
> >                     |          |
> >                     ------------
> >                     .          .
> >                     .          .
> >                     .          .
> >                     ------------
> >                     |          |
> >                     |          |
> >                     ------------
> >                     |tinfo_ptr |  /* pointer to thread info */
> >                     ------------ Kernel stack base
>
> The extra indirection doesn't look great, and I don't see any of the
> other architectures need that. Since we can access percpu data
> without going through thread_info, maybe doing the same as
> x86 would work here:
>
> - define 'current' as 'this_cpu_read_stable(current_task);'
> - convert to CONFIG_THREAD_INFO_IN_TASK
>
>        Arnd

Re: [PATCH 2/3] arm: introduce IRQ stacks

[Russell King - ARM Linux admin]

On Wed, Oct 21, 2020 at 02:42:48PM +0200, Arnd Bergmann wrote:
> (also resending this reply from @kernel.org)
> 
> On Fri, Oct 16, 2020 at 12:19 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > On Thu, Oct 8, 2020 at 9:20 AM Maninder Singh <maninder1.s@samsung.com> wrote:
> > >
> > > This patch adds code for switching to IRQ stack.
> > > IRQ stack and Kernel SVC stack have below design.
> > >
> > > IRQ STACK:-
> > >                     ------------ IRQ stack top
> > >                     |          |
> > >                     ------------
> > >                     .          .
> > >                     .          .
> > >                     .          .
> > >                     ------------
> > >                     |    sp    | <- irq_stack_base + 0x8
> > >                     ------------
> > >                     |    fp    | <- irq_stack_base + 0x4
> > >                     ------------
> > >                     |tinfo_ptr | /* pointer to thread info */
> > > irq_stack_ptr -->   ------------ IRQ stack base
> > >
> > > Kernel SVC stack:-
> > >                     ------------  Kernel stack top
> > >                     |          |
> > >                     ------------
> > >                     .          .
> > >                     .          .
> > >                     .          .
> > >                     ------------
> > >                     |          |
> > >                     |          |
> > >                     ------------
> > >                     |tinfo_ptr |  /* pointer to thread info */
> > >                     ------------ Kernel stack base
> >
> > The extra indirection doesn't look great, and I don't see any of the
> > other architectures need that. Since we can access percpu data
> > without going through thread_info, maybe doing the same as
> > x86 would work here:
> >
> > - define 'current' as 'this_cpu_read_stable(current_task);'
> > - convert to CONFIG_THREAD_INFO_IN_TASK

That means we need to also code that up in assembly - remember, we
need to access thread_info from assembly code.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] arm: introduce IRQ stacks

[Russell King - ARM Linux admin]

On Wed, Oct 21, 2020 at 02:42:48PM +0200, Arnd Bergmann wrote:
> (also resending this reply from @kernel.org)
> 
> On Fri, Oct 16, 2020 at 12:19 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > On Thu, Oct 8, 2020 at 9:20 AM Maninder Singh <maninder1.s@samsung.com> wrote:
> > >
> > > This patch adds code for switching to IRQ stack.
> > > IRQ stack and Kernel SVC stack have below design.
> > >
> > > IRQ STACK:-
> > >                     ------------ IRQ stack top
> > >                     |          |
> > >                     ------------
> > >                     .          .
> > >                     .          .
> > >                     .          .
> > >                     ------------
> > >                     |    sp    | <- irq_stack_base + 0x8
> > >                     ------------
> > >                     |    fp    | <- irq_stack_base + 0x4
> > >                     ------------
> > >                     |tinfo_ptr | /* pointer to thread info */
> > > irq_stack_ptr -->   ------------ IRQ stack base
> > >
> > > Kernel SVC stack:-
> > >                     ------------  Kernel stack top
> > >                     |          |
> > >                     ------------
> > >                     .          .
> > >                     .          .
> > >                     .          .
> > >                     ------------
> > >                     |          |
> > >                     |          |
> > >                     ------------
> > >                     |tinfo_ptr |  /* pointer to thread info */
> > >                     ------------ Kernel stack base
> >
> > The extra indirection doesn't look great, and I don't see any of the
> > other architectures need that. Since we can access percpu data
> > without going through thread_info, maybe doing the same as
> > x86 would work here:
> >
> > - define 'current' as 'this_cpu_read_stable(current_task);'
> > - convert to CONFIG_THREAD_INFO_IN_TASK

That means we need to also code that up in assembly - remember, we
need to access thread_info from assembly code.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 2/3] arm: introduce IRQ stacks

[Arnd Bergmann]

On Wed, Oct 21, 2020 at 2:45 PM Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
> On Wed, Oct 21, 2020 at 02:42:48PM +0200, Arnd Bergmann wrote:
> > >
> > > - define 'current' as 'this_cpu_read_stable(current_task);'
> > > - convert to CONFIG_THREAD_INFO_IN_TASK
>
> That means we need to also code that up in assembly - remember, we
> need to access thread_info from assembly code.

Are there any obvious places that need patching aside from
.macro get_thread_info? I did expect the above conversion to
be somewhat more complicated than Maninder's original patch,
but that part seems easy enough.

       Arnd

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] arm: introduce IRQ stacks

[Arnd Bergmann]

On Wed, Oct 21, 2020 at 2:45 PM Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
> On Wed, Oct 21, 2020 at 02:42:48PM +0200, Arnd Bergmann wrote:
> > >
> > > - define 'current' as 'this_cpu_read_stable(current_task);'
> > > - convert to CONFIG_THREAD_INFO_IN_TASK
>
> That means we need to also code that up in assembly - remember, we
> need to access thread_info from assembly code.

Are there any obvious places that need patching aside from
.macro get_thread_info? I did expect the above conversion to
be somewhat more complicated than Maninder's original patch,
but that part seems easy enough.

       Arnd

Re: [PATCH 2/3] arm: introduce IRQ stacks

[Russell King - ARM Linux admin]

On Wed, Oct 21, 2020 at 01:45:42PM +0100, Russell King - ARM Linux admin wrote:
> On Wed, Oct 21, 2020 at 02:42:48PM +0200, Arnd Bergmann wrote:
> > (also resending this reply from @kernel.org)
> > 
> > On Fri, Oct 16, 2020 at 12:19 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > > On Thu, Oct 8, 2020 at 9:20 AM Maninder Singh <maninder1.s@samsung.com> wrote:
> > > >
> > > > This patch adds code for switching to IRQ stack.
> > > > IRQ stack and Kernel SVC stack have below design.
> > > >
> > > > IRQ STACK:-
> > > >                     ------------ IRQ stack top
> > > >                     |          |
> > > >                     ------------
> > > >                     .          .
> > > >                     .          .
> > > >                     .          .
> > > >                     ------------
> > > >                     |    sp    | <- irq_stack_base + 0x8
> > > >                     ------------
> > > >                     |    fp    | <- irq_stack_base + 0x4
> > > >                     ------------
> > > >                     |tinfo_ptr | /* pointer to thread info */
> > > > irq_stack_ptr -->   ------------ IRQ stack base
> > > >
> > > > Kernel SVC stack:-
> > > >                     ------------  Kernel stack top
> > > >                     |          |
> > > >                     ------------
> > > >                     .          .
> > > >                     .          .
> > > >                     .          .
> > > >                     ------------
> > > >                     |          |
> > > >                     |          |
> > > >                     ------------
> > > >                     |tinfo_ptr |  /* pointer to thread info */
> > > >                     ------------ Kernel stack base
> > >
> > > The extra indirection doesn't look great, and I don't see any of the
> > > other architectures need that. Since we can access percpu data
> > > without going through thread_info, maybe doing the same as
> > > x86 would work here:
> > >
> > > - define 'current' as 'this_cpu_read_stable(current_task);'
> > > - convert to CONFIG_THREAD_INFO_IN_TASK
> 
> That means we need to also code that up in assembly - remember, we
> need to access thread_info from assembly code.

Note also that there is a circular dependency involved. If you make
thread_info accessible via per-cpu, then:

#ifndef __my_cpu_offset
#define __my_cpu_offset per_cpu_offset(raw_smp_processor_id())
#endif
#ifdef CONFIG_DEBUG_PREEMPT
#define my_cpu_offset per_cpu_offset(smp_processor_id())
#else
#define my_cpu_offset __my_cpu_offset
#endif

smp_processor_id() ultimately ends up as raw_smp_processor_id() which
is:

#define raw_smp_processor_id() (current_thread_info()->cpu)

and if current_thread_info() itself involves reading from per-cpu data,
we end up recursing... infinitely.

This is why I said in the other thread:

"We don't do it because we don't have a separate register to be able
to store the thread_info pointer, and copying that lump between the
SVC and IRQ stack will add massively to IRQ latency, especially for
older machines."

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] arm: introduce IRQ stacks

[Russell King - ARM Linux admin]

On Wed, Oct 21, 2020 at 01:45:42PM +0100, Russell King - ARM Linux admin wrote:
> On Wed, Oct 21, 2020 at 02:42:48PM +0200, Arnd Bergmann wrote:
> > (also resending this reply from @kernel.org)
> > 
> > On Fri, Oct 16, 2020 at 12:19 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > > On Thu, Oct 8, 2020 at 9:20 AM Maninder Singh <maninder1.s@samsung.com> wrote:
> > > >
> > > > This patch adds code for switching to IRQ stack.
> > > > IRQ stack and Kernel SVC stack have below design.
> > > >
> > > > IRQ STACK:-
> > > >                     ------------ IRQ stack top
> > > >                     |          |
> > > >                     ------------
> > > >                     .          .
> > > >                     .          .
> > > >                     .          .
> > > >                     ------------
> > > >                     |    sp    | <- irq_stack_base + 0x8
> > > >                     ------------
> > > >                     |    fp    | <- irq_stack_base + 0x4
> > > >                     ------------
> > > >                     |tinfo_ptr | /* pointer to thread info */
> > > > irq_stack_ptr -->   ------------ IRQ stack base
> > > >
> > > > Kernel SVC stack:-
> > > >                     ------------  Kernel stack top
> > > >                     |          |
> > > >                     ------------
> > > >                     .          .
> > > >                     .          .
> > > >                     .          .
> > > >                     ------------
> > > >                     |          |
> > > >                     |          |
> > > >                     ------------
> > > >                     |tinfo_ptr |  /* pointer to thread info */
> > > >                     ------------ Kernel stack base
> > >
> > > The extra indirection doesn't look great, and I don't see any of the
> > > other architectures need that. Since we can access percpu data
> > > without going through thread_info, maybe doing the same as
> > > x86 would work here:
> > >
> > > - define 'current' as 'this_cpu_read_stable(current_task);'
> > > - convert to CONFIG_THREAD_INFO_IN_TASK
> 
> That means we need to also code that up in assembly - remember, we
> need to access thread_info from assembly code.

Note also that there is a circular dependency involved. If you make
thread_info accessible via per-cpu, then:

#ifndef __my_cpu_offset
#define __my_cpu_offset per_cpu_offset(raw_smp_processor_id())
#endif
#ifdef CONFIG_DEBUG_PREEMPT
#define my_cpu_offset per_cpu_offset(smp_processor_id())
#else
#define my_cpu_offset __my_cpu_offset
#endif

smp_processor_id() ultimately ends up as raw_smp_processor_id() which
is:

#define raw_smp_processor_id() (current_thread_info()->cpu)

and if current_thread_info() itself involves reading from per-cpu data,
we end up recursing... infinitely.

This is why I said in the other thread:

"We don't do it because we don't have a separate register to be able
to store the thread_info pointer, and copying that lump between the
SVC and IRQ stack will add massively to IRQ latency, especially for
older machines."

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

RE: [PATCH 2/3] arm: introduce IRQ stacks

[Vaneet Narang]

Hi Russel & Arnd,

> > > - define 'current' as 'this_cpu_read_stable(current_task);'
> > > - convert to CONFIG_THREAD_INFO_IN_TASK
> 
unlike ARM64 ARM doesn't suport dedicated register like "sp_el0", so 
arm still stores thread info on stack and fetches using current task using
stack pointer (SP) so CONFIG_THREAD_INFO_IN_TASK is not supported on ARM. 
To implement solution inline with current ARM design, we have used indirection.

> That means we need to also code that up in assembly - remember, we
> need to access thread_info from assembly code.

> Note also that there is a circular dependency involved. If you make
> thread_info accessible via per-cpu, then:

> "We don't do it because we don't have a separate register to be able
> to store the thread_info pointer, and copying that lump between the
> SVC and IRQ stack will add massively to IRQ latency, especially for
> older machines."

We tried to minimize latency in switching between SVC stack and IRQ stack
by just copying extra thread info pointer to IRQ stack. Apart from this, 
Most of the code of SVC to IRQ switching is similar to ARM64 implementation.

Also We tried to minimize latency of get_current() by introducting self pointer in
SVC stack to avoid if check to determine, is current is called from SVC context
or IRQ context. This way will always do one extra indirection in get_current, 
irrespective get_current is called from SVC or IRQ context.

Yes we agree still there will be minor additional latency on accessing current
task and SVC to IRQ switching  which might be significant for older machines,
So this is the reason why we kept this sol. under kconfig. This solution
provides extra stack to kernel developers for further development,instead of increasing
stack size to 16KB or spending time on debugging stack overflow issues.

PS: We have debugged and resolved stack overflow issue but we still implemented this sol. to avoid
debugging such issues in future also it gives us flexibility for kernel enhancement on ARM.

Thanks & Regards,
Vaneet Narang

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH 2/3] arm: introduce IRQ stacks

[Vaneet Narang]

Hi Russel & Arnd,

> > > - define 'current' as 'this_cpu_read_stable(current_task);'
> > > - convert to CONFIG_THREAD_INFO_IN_TASK
> 
unlike ARM64 ARM doesn't suport dedicated register like "sp_el0", so 
arm still stores thread info on stack and fetches using current task using
stack pointer (SP) so CONFIG_THREAD_INFO_IN_TASK is not supported on ARM. 
To implement solution inline with current ARM design, we have used indirection.

> That means we need to also code that up in assembly - remember, we
> need to access thread_info from assembly code.

> Note also that there is a circular dependency involved. If you make
> thread_info accessible via per-cpu, then:

> "We don't do it because we don't have a separate register to be able
> to store the thread_info pointer, and copying that lump between the
> SVC and IRQ stack will add massively to IRQ latency, especially for
> older machines."

We tried to minimize latency in switching between SVC stack and IRQ stack
by just copying extra thread info pointer to IRQ stack. Apart from this, 
Most of the code of SVC to IRQ switching is similar to ARM64 implementation.

Also We tried to minimize latency of get_current() by introducting self pointer in
SVC stack to avoid if check to determine, is current is called from SVC context
or IRQ context. This way will always do one extra indirection in get_current, 
irrespective get_current is called from SVC or IRQ context.

Yes we agree still there will be minor additional latency on accessing current
task and SVC to IRQ switching  which might be significant for older machines,
So this is the reason why we kept this sol. under kconfig. This solution
provides extra stack to kernel developers for further development,instead of increasing
stack size to 16KB or spending time on debugging stack overflow issues.

PS: We have debugged and resolved stack overflow issue but we still implemented this sol. to avoid
debugging such issues in future also it gives us flexibility for kernel enhancement on ARM.

Thanks & Regards,
Vaneet Narang

Re: [PATCH 2/3] arm: introduce IRQ stacks

[Arnd Bergmann]

On Wed, Oct 21, 2020 at 2:57 PM Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
> On Wed, Oct 21, 2020 at 01:45:42PM +0100, Russell King - ARM Linux admin wrote:
> > > > - define 'current' as 'this_cpu_read_stable(current_task);'
> > > > - convert to CONFIG_THREAD_INFO_IN_TASK
> >
> > That means we need to also code that up in assembly - remember, we
> > need to access thread_info from assembly code.
>
> Note also that there is a circular dependency involved. If you make
> thread_info accessible via per-cpu, then:
>
> #ifndef __my_cpu_offset
> #define __my_cpu_offset per_cpu_offset(raw_smp_processor_id())
> #endif
> #ifdef CONFIG_DEBUG_PREEMPT
> #define my_cpu_offset per_cpu_offset(smp_processor_id())
> #else
> #define my_cpu_offset __my_cpu_offset
> #endif

Right, I had missed the fallback path using asm-generic/percpu.h
that is used with CONFIG_SMP && CONFIG_CPU_V6
Almost everything either uses fixed percpu data (on UP builds)
or TPIDRPRW when building a v7-only or v6k/v7 kernel without
v6 support.

> smp_processor_id() ultimately ends up as raw_smp_processor_id() which
> is:
>
> #define raw_smp_processor_id() (current_thread_info()->cpu)
>
> and if current_thread_info() itself involves reading from per-cpu data,
> we end up recursing... infinitely.
>
> This is why I said in the other thread:
>
> "We don't do it because we don't have a separate register to be able
> to store the thread_info pointer, and copying that lump between the
> SVC and IRQ stack will add massively to IRQ latency, especially for
> older machines."

As discussed on IRC, I think it can still be done in one of these
ways, though admittedly none of them are perfect:

a) add runtime patching for __my_cpu_offset() when
  CONFIG_SMP_ON_UP is set. This adds complexity but avoids the
  fallback for for SMP&&CPU_V6. It possibly also speeds up
  running on single-cpu systems if the TPIDRPRW access adds
  any measurable runtime overhead compared to patching it out.

b) If irq stacks are left as a compile-time option, that could be
  made conditional on "!(SMP&&CPU_V6)". Presumably very
  few people still run kernels built that way any more. The only
  supported platforms are i.MX3, OMAP2 and Realview-eb, all of
  which are fairly uncommon these days and would usually
  run v6-only non-SMP kernels.

c) If we decide that we no longer care about that configuration
  at all, we could decide to just make SMP depend on !CPU_V6,
  and possibly kill off the entire SMP_ON_UP patching logic.
  I suspect we still want to keep SMP_ON_UP for performance
  reasons, but I don't know how significant they are to start with.

       Arnd

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] arm: introduce IRQ stacks

[Arnd Bergmann]

On Wed, Oct 21, 2020 at 2:57 PM Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
> On Wed, Oct 21, 2020 at 01:45:42PM +0100, Russell King - ARM Linux admin wrote:
> > > > - define 'current' as 'this_cpu_read_stable(current_task);'
> > > > - convert to CONFIG_THREAD_INFO_IN_TASK
> >
> > That means we need to also code that up in assembly - remember, we
> > need to access thread_info from assembly code.
>
> Note also that there is a circular dependency involved. If you make
> thread_info accessible via per-cpu, then:
>
> #ifndef __my_cpu_offset
> #define __my_cpu_offset per_cpu_offset(raw_smp_processor_id())
> #endif
> #ifdef CONFIG_DEBUG_PREEMPT
> #define my_cpu_offset per_cpu_offset(smp_processor_id())
> #else
> #define my_cpu_offset __my_cpu_offset
> #endif

Right, I had missed the fallback path using asm-generic/percpu.h
that is used with CONFIG_SMP && CONFIG_CPU_V6
Almost everything either uses fixed percpu data (on UP builds)
or TPIDRPRW when building a v7-only or v6k/v7 kernel without
v6 support.

> smp_processor_id() ultimately ends up as raw_smp_processor_id() which
> is:
>
> #define raw_smp_processor_id() (current_thread_info()->cpu)
>
> and if current_thread_info() itself involves reading from per-cpu data,
> we end up recursing... infinitely.
>
> This is why I said in the other thread:
>
> "We don't do it because we don't have a separate register to be able
> to store the thread_info pointer, and copying that lump between the
> SVC and IRQ stack will add massively to IRQ latency, especially for
> older machines."

As discussed on IRC, I think it can still be done in one of these
ways, though admittedly none of them are perfect:

a) add runtime patching for __my_cpu_offset() when
  CONFIG_SMP_ON_UP is set. This adds complexity but avoids the
  fallback for for SMP&&CPU_V6. It possibly also speeds up
  running on single-cpu systems if the TPIDRPRW access adds
  any measurable runtime overhead compared to patching it out.

b) If irq stacks are left as a compile-time option, that could be
  made conditional on "!(SMP&&CPU_V6)". Presumably very
  few people still run kernels built that way any more. The only
  supported platforms are i.MX3, OMAP2 and Realview-eb, all of
  which are fairly uncommon these days and would usually
  run v6-only non-SMP kernels.

c) If we decide that we no longer care about that configuration
  at all, we could decide to just make SMP depend on !CPU_V6,
  and possibly kill off the entire SMP_ON_UP patching logic.
  I suspect we still want to keep SMP_ON_UP for performance
  reasons, but I don't know how significant they are to start with.

       Arnd

Re: [PATCH 2/3] arm: introduce IRQ stacks

[Tony Lindgren]

* Arnd Bergmann <arnd@kernel.org> [201021 16:07]:
> On Wed, Oct 21, 2020 at 2:57 PM Russell King - ARM Linux admin
> <linux@armlinux.org.uk> wrote:
> > On Wed, Oct 21, 2020 at 01:45:42PM +0100, Russell King - ARM Linux admin wrote:
> > > > > - define 'current' as 'this_cpu_read_stable(current_task);'
> > > > > - convert to CONFIG_THREAD_INFO_IN_TASK
> > >
> > > That means we need to also code that up in assembly - remember, we
> > > need to access thread_info from assembly code.
> >
> > Note also that there is a circular dependency involved. If you make
> > thread_info accessible via per-cpu, then:
> >
> > #ifndef __my_cpu_offset
> > #define __my_cpu_offset per_cpu_offset(raw_smp_processor_id())
> > #endif
> > #ifdef CONFIG_DEBUG_PREEMPT
> > #define my_cpu_offset per_cpu_offset(smp_processor_id())
> > #else
> > #define my_cpu_offset __my_cpu_offset
> > #endif
> 
> Right, I had missed the fallback path using asm-generic/percpu.h
> that is used with CONFIG_SMP && CONFIG_CPU_V6
> Almost everything either uses fixed percpu data (on UP builds)
> or TPIDRPRW when building a v7-only or v6k/v7 kernel without
> v6 support.
> 
> > smp_processor_id() ultimately ends up as raw_smp_processor_id() which
> > is:
> >
> > #define raw_smp_processor_id() (current_thread_info()->cpu)
> >
> > and if current_thread_info() itself involves reading from per-cpu data,
> > we end up recursing... infinitely.
> >
> > This is why I said in the other thread:
> >
> > "We don't do it because we don't have a separate register to be able
> > to store the thread_info pointer, and copying that lump between the
> > SVC and IRQ stack will add massively to IRQ latency, especially for
> > older machines."
> 
> As discussed on IRC, I think it can still be done in one of these
> ways, though admittedly none of them are perfect:
> 
> a) add runtime patching for __my_cpu_offset() when
>   CONFIG_SMP_ON_UP is set. This adds complexity but avoids the
>   fallback for for SMP&&CPU_V6. It possibly also speeds up
>   running on single-cpu systems if the TPIDRPRW access adds
>   any measurable runtime overhead compared to patching it out.

Out of these options a) sounds best to me.

> b) If irq stacks are left as a compile-time option, that could be
>   made conditional on "!(SMP&&CPU_V6)". Presumably very
>   few people still run kernels built that way any more. The only
>   supported platforms are i.MX3, OMAP2 and Realview-eb, all of
>   which are fairly uncommon these days and would usually
>   run v6-only non-SMP kernels.

This has been working just fine for years though. In general,
removing the conditional compile ifdefferey has made things quite
a bit easier for us, so let's continue on that.

> c) If we decide that we no longer care about that configuration
>   at all, we could decide to just make SMP depend on !CPU_V6,
>   and possibly kill off the entire SMP_ON_UP patching logic.
>   I suspect we still want to keep SMP_ON_UP for performance
>   reasons, but I don't know how significant they are to start with.

And this too has been working just fine for years :)

Regards,

Tony

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] arm: introduce IRQ stacks

[Tony Lindgren]

* Arnd Bergmann <arnd@kernel.org> [201021 16:07]:
> On Wed, Oct 21, 2020 at 2:57 PM Russell King - ARM Linux admin
> <linux@armlinux.org.uk> wrote:
> > On Wed, Oct 21, 2020 at 01:45:42PM +0100, Russell King - ARM Linux admin wrote:
> > > > > - define 'current' as 'this_cpu_read_stable(current_task);'
> > > > > - convert to CONFIG_THREAD_INFO_IN_TASK
> > >
> > > That means we need to also code that up in assembly - remember, we
> > > need to access thread_info from assembly code.
> >
> > Note also that there is a circular dependency involved. If you make
> > thread_info accessible via per-cpu, then:
> >
> > #ifndef __my_cpu_offset
> > #define __my_cpu_offset per_cpu_offset(raw_smp_processor_id())
> > #endif
> > #ifdef CONFIG_DEBUG_PREEMPT
> > #define my_cpu_offset per_cpu_offset(smp_processor_id())
> > #else
> > #define my_cpu_offset __my_cpu_offset
> > #endif
> 
> Right, I had missed the fallback path using asm-generic/percpu.h
> that is used with CONFIG_SMP && CONFIG_CPU_V6
> Almost everything either uses fixed percpu data (on UP builds)
> or TPIDRPRW when building a v7-only or v6k/v7 kernel without
> v6 support.
> 
> > smp_processor_id() ultimately ends up as raw_smp_processor_id() which
> > is:
> >
> > #define raw_smp_processor_id() (current_thread_info()->cpu)
> >
> > and if current_thread_info() itself involves reading from per-cpu data,
> > we end up recursing... infinitely.
> >
> > This is why I said in the other thread:
> >
> > "We don't do it because we don't have a separate register to be able
> > to store the thread_info pointer, and copying that lump between the
> > SVC and IRQ stack will add massively to IRQ latency, especially for
> > older machines."
> 
> As discussed on IRC, I think it can still be done in one of these
> ways, though admittedly none of them are perfect:
> 
> a) add runtime patching for __my_cpu_offset() when
>   CONFIG_SMP_ON_UP is set. This adds complexity but avoids the
>   fallback for for SMP&&CPU_V6. It possibly also speeds up
>   running on single-cpu systems if the TPIDRPRW access adds
>   any measurable runtime overhead compared to patching it out.

Out of these options a) sounds best to me.

> b) If irq stacks are left as a compile-time option, that could be
>   made conditional on "!(SMP&&CPU_V6)". Presumably very
>   few people still run kernels built that way any more. The only
>   supported platforms are i.MX3, OMAP2 and Realview-eb, all of
>   which are fairly uncommon these days and would usually
>   run v6-only non-SMP kernels.

This has been working just fine for years though. In general,
removing the conditional compile ifdefferey has made things quite
a bit easier for us, so let's continue on that.

> c) If we decide that we no longer care about that configuration
>   at all, we could decide to just make SMP depend on !CPU_V6,
>   and possibly kill off the entire SMP_ON_UP patching logic.
>   I suspect we still want to keep SMP_ON_UP for performance
>   reasons, but I don't know how significant they are to start with.

And this too has been working just fine for years :)

Regards,

Tony

Re: [PATCH 2/3] arm: introduce IRQ stacks

[Arnd Bergmann]

On Mon, Nov 9, 2020 at 3:45 PM Tony Lindgren <tony@atomide.com> wrote:
> >
> > As discussed on IRC, I think it can still be done in one of these
> > ways, though admittedly none of them are perfect:
> >
> > a) add runtime patching for __my_cpu_offset() when
> >   CONFIG_SMP_ON_UP is set. This adds complexity but avoids the
> >   fallback for for SMP&&CPU_V6. It possibly also speeds up
> >   running on single-cpu systems if the TPIDRPRW access adds
> >   any measurable runtime overhead compared to patching it out.
>
> Out of these options a) sounds best to me.

Ok. Maninder, would you like to give implementing this a try?

> > b) If irq stacks are left as a compile-time option, that could be
> >   made conditional on "!(SMP&&CPU_V6)". Presumably very
> >   few people still run kernels built that way any more. The only
> >   supported platforms are i.MX3, OMAP2 and Realview-eb, all of
> >   which are fairly uncommon these days and would usually
> >   run v6-only non-SMP kernels.
>
> This has been working just fine for years though. In general,
> removing the conditional compile ifdefferey has made things quite
> a bit easier for us, so let's continue on that.
>
> > c) If we decide that we no longer care about that configuration
> >   at all, we could decide to just make SMP depend on !CPU_V6,
> >   and possibly kill off the entire SMP_ON_UP patching logic.
> >   I suspect we still want to keep SMP_ON_UP for performance
> >   reasons, but I don't know how significant they are to start with.
>
> And this too has been working just fine for years :)

I know it works, my point was that I'm not sure anyone cares
any more ;-)

I suppose the existence of omap2plus_defconfig and
imx_v6_v7_defconfig means it does at least get tested
regularly.

       Arnd

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] arm: introduce IRQ stacks

[Arnd Bergmann]

On Mon, Nov 9, 2020 at 3:45 PM Tony Lindgren <tony@atomide.com> wrote:
> >
> > As discussed on IRC, I think it can still be done in one of these
> > ways, though admittedly none of them are perfect:
> >
> > a) add runtime patching for __my_cpu_offset() when
> >   CONFIG_SMP_ON_UP is set. This adds complexity but avoids the
> >   fallback for for SMP&&CPU_V6. It possibly also speeds up
> >   running on single-cpu systems if the TPIDRPRW access adds
> >   any measurable runtime overhead compared to patching it out.
>
> Out of these options a) sounds best to me.

Ok. Maninder, would you like to give implementing this a try?

> > b) If irq stacks are left as a compile-time option, that could be
> >   made conditional on "!(SMP&&CPU_V6)". Presumably very
> >   few people still run kernels built that way any more. The only
> >   supported platforms are i.MX3, OMAP2 and Realview-eb, all of
> >   which are fairly uncommon these days and would usually
> >   run v6-only non-SMP kernels.
>
> This has been working just fine for years though. In general,
> removing the conditional compile ifdefferey has made things quite
> a bit easier for us, so let's continue on that.
>
> > c) If we decide that we no longer care about that configuration
> >   at all, we could decide to just make SMP depend on !CPU_V6,
> >   and possibly kill off the entire SMP_ON_UP patching logic.
> >   I suspect we still want to keep SMP_ON_UP for performance
> >   reasons, but I don't know how significant they are to start with.
>
> And this too has been working just fine for years :)

I know it works, my point was that I'm not sure anyone cares
any more ;-)

I suppose the existence of omap2plus_defconfig and
imx_v6_v7_defconfig means it does at least get tested
regularly.

       Arnd

Re: [PATCH 2/3] arm: introduce IRQ stacks

[Tony Lindgren]

* Arnd Bergmann <arnd@kernel.org> [201109 19:10]:
> On Mon, Nov 9, 2020 at 3:45 PM Tony Lindgren <tony@atomide.com> wrote:
> > >
> > > As discussed on IRC, I think it can still be done in one of these
> > > ways, though admittedly none of them are perfect:
> > >
> > > a) add runtime patching for __my_cpu_offset() when
> > >   CONFIG_SMP_ON_UP is set. This adds complexity but avoids the
> > >   fallback for for SMP&&CPU_V6. It possibly also speeds up
> > >   running on single-cpu systems if the TPIDRPRW access adds
> > >   any measurable runtime overhead compared to patching it out.
> >
> > Out of these options a) sounds best to me.
> 
> Ok. Maninder, would you like to give implementing this a try?
> 
> > > b) If irq stacks are left as a compile-time option, that could be
> > >   made conditional on "!(SMP&&CPU_V6)". Presumably very
> > >   few people still run kernels built that way any more. The only
> > >   supported platforms are i.MX3, OMAP2 and Realview-eb, all of
> > >   which are fairly uncommon these days and would usually
> > >   run v6-only non-SMP kernels.
> >
> > This has been working just fine for years though. In general,
> > removing the conditional compile ifdefferey has made things quite
> > a bit easier for us, so let's continue on that.
> >
> > > c) If we decide that we no longer care about that configuration
> > >   at all, we could decide to just make SMP depend on !CPU_V6,
> > >   and possibly kill off the entire SMP_ON_UP patching logic.
> > >   I suspect we still want to keep SMP_ON_UP for performance
> > >   reasons, but I don't know how significant they are to start with.
> >
> > And this too has been working just fine for years :)
> 
> I know it works, my point was that I'm not sure anyone cares
> any more ;-)

Well for example whatever Linux running ARMv6 LTE modems out there might
need to be supported for quite some time. Not sure how many of them are
able to update kernels though. Certainly network security related issues
would be a good reason to update the kernels.

> I suppose the existence of omap2plus_defconfig and
> imx_v6_v7_defconfig means it does at least get tested
> regularly.

Yes. I probably would just stop any random ARMv6 related testing if
it it needs a custom .config.

Regards,

Tony

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] arm: introduce IRQ stacks

[Tony Lindgren]

* Arnd Bergmann <arnd@kernel.org> [201109 19:10]:
> On Mon, Nov 9, 2020 at 3:45 PM Tony Lindgren <tony@atomide.com> wrote:
> > >
> > > As discussed on IRC, I think it can still be done in one of these
> > > ways, though admittedly none of them are perfect:
> > >
> > > a) add runtime patching for __my_cpu_offset() when
> > >   CONFIG_SMP_ON_UP is set. This adds complexity but avoids the
> > >   fallback for for SMP&&CPU_V6. It possibly also speeds up
> > >   running on single-cpu systems if the TPIDRPRW access adds
> > >   any measurable runtime overhead compared to patching it out.
> >
> > Out of these options a) sounds best to me.
> 
> Ok. Maninder, would you like to give implementing this a try?
> 
> > > b) If irq stacks are left as a compile-time option, that could be
> > >   made conditional on "!(SMP&&CPU_V6)". Presumably very
> > >   few people still run kernels built that way any more. The only
> > >   supported platforms are i.MX3, OMAP2 and Realview-eb, all of
> > >   which are fairly uncommon these days and would usually
> > >   run v6-only non-SMP kernels.
> >
> > This has been working just fine for years though. In general,
> > removing the conditional compile ifdefferey has made things quite
> > a bit easier for us, so let's continue on that.
> >
> > > c) If we decide that we no longer care about that configuration
> > >   at all, we could decide to just make SMP depend on !CPU_V6,
> > >   and possibly kill off the entire SMP_ON_UP patching logic.
> > >   I suspect we still want to keep SMP_ON_UP for performance
> > >   reasons, but I don't know how significant they are to start with.
> >
> > And this too has been working just fine for years :)
> 
> I know it works, my point was that I'm not sure anyone cares
> any more ;-)

Well for example whatever Linux running ARMv6 LTE modems out there might
need to be supported for quite some time. Not sure how many of them are
able to update kernels though. Certainly network security related issues
would be a good reason to update the kernels.

> I suppose the existence of omap2plus_defconfig and
> imx_v6_v7_defconfig means it does at least get tested
> regularly.

Yes. I probably would just stop any random ARMv6 related testing if
it it needs a custom .config.

Regards,

Tony

Re: [PATCH 2/3] arm: introduce IRQ stacks

[Arnd Bergmann]

On Tue, Nov 10, 2020 at 10:19 AM Tony Lindgren <tony@atomide.com> wrote:
> * Arnd Bergmann <arnd@kernel.org> [201109 19:10]:
> > On Mon, Nov 9, 2020 at 3:45 PM Tony Lindgren <tony@atomide.com> wrote:
> >
> > I know it works, my point was that I'm not sure anyone cares
> > any more ;-)
>
> Well for example whatever Linux running ARMv6 LTE modems out there might
> need to be supported for quite some time. Not sure how many of them are
> able to update kernels though. Certainly network security related issues
> would be a good reason to update the kernels.

While I agree they should update their kernels, I suspect none of those
modems do. I am however certain that none of them are running an
SMP-enabled multiplatform kernel on an ARM1136r0!

Are these actually ARMv6? Most ARM11 cores you'd come across
in practice are ARMv6K (ARM1136r1, ARM1167, ARM11MPCore),
in particular every SoC that has any mainline support except for
the ARM1136r0 based OMAP2 and i.MX3.

        Arnd

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] arm: introduce IRQ stacks

[Arnd Bergmann]

On Tue, Nov 10, 2020 at 10:19 AM Tony Lindgren <tony@atomide.com> wrote:
> * Arnd Bergmann <arnd@kernel.org> [201109 19:10]:
> > On Mon, Nov 9, 2020 at 3:45 PM Tony Lindgren <tony@atomide.com> wrote:
> >
> > I know it works, my point was that I'm not sure anyone cares
> > any more ;-)
>
> Well for example whatever Linux running ARMv6 LTE modems out there might
> need to be supported for quite some time. Not sure how many of them are
> able to update kernels though. Certainly network security related issues
> would be a good reason to update the kernels.

While I agree they should update their kernels, I suspect none of those
modems do. I am however certain that none of them are running an
SMP-enabled multiplatform kernel on an ARM1136r0!

Are these actually ARMv6? Most ARM11 cores you'd come across
in practice are ARMv6K (ARM1136r1, ARM1167, ARM11MPCore),
in particular every SoC that has any mainline support except for
the ARM1136r0 based OMAP2 and i.MX3.

        Arnd

Re: [PATCH 2/3] arm: introduce IRQ stacks

[Tony Lindgren]

* Arnd Bergmann <arnd@kernel.org> [201110 10:04]:
> On Tue, Nov 10, 2020 at 10:19 AM Tony Lindgren <tony@atomide.com> wrote:
> > * Arnd Bergmann <arnd@kernel.org> [201109 19:10]:
> > > On Mon, Nov 9, 2020 at 3:45 PM Tony Lindgren <tony@atomide.com> wrote:
> > >
> > > I know it works, my point was that I'm not sure anyone cares
> > > any more ;-)
> >
> > Well for example whatever Linux running ARMv6 LTE modems out there might
> > need to be supported for quite some time. Not sure how many of them are
> > able to update kernels though. Certainly network security related issues
> > would be a good reason to update the kernels.
> 
> While I agree they should update their kernels, I suspect none of those
> modems do. I am however certain that none of them are running an
> SMP-enabled multiplatform kernel on an ARM1136r0!

Nope, AFAIK all the SMP parts are ARMv6K :)

> Are these actually ARMv6? Most ARM11 cores you'd come across
> in practice are ARMv6K (ARM1136r1, ARM1167, ARM11MPCore),
> in particular every SoC that has any mainline support except for
> the ARM1136r0 based OMAP2 and i.MX3.

I've been only using smp_on_up for the ARMv6 ARM1136r0 variants
for omap2, no SMP on those.

Regards,

Tony

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] arm: introduce IRQ stacks

[Tony Lindgren]

* Arnd Bergmann <arnd@kernel.org> [201110 10:04]:
> On Tue, Nov 10, 2020 at 10:19 AM Tony Lindgren <tony@atomide.com> wrote:
> > * Arnd Bergmann <arnd@kernel.org> [201109 19:10]:
> > > On Mon, Nov 9, 2020 at 3:45 PM Tony Lindgren <tony@atomide.com> wrote:
> > >
> > > I know it works, my point was that I'm not sure anyone cares
> > > any more ;-)
> >
> > Well for example whatever Linux running ARMv6 LTE modems out there might
> > need to be supported for quite some time. Not sure how many of them are
> > able to update kernels though. Certainly network security related issues
> > would be a good reason to update the kernels.
> 
> While I agree they should update their kernels, I suspect none of those
> modems do. I am however certain that none of them are running an
> SMP-enabled multiplatform kernel on an ARM1136r0!

Nope, AFAIK all the SMP parts are ARMv6K :)

> Are these actually ARMv6? Most ARM11 cores you'd come across
> in practice are ARMv6K (ARM1136r1, ARM1167, ARM11MPCore),
> in particular every SoC that has any mainline support except for
> the ARM1136r0 based OMAP2 and i.MX3.

I've been only using smp_on_up for the ARMv6 ARM1136r0 variants
for omap2, no SMP on those.

Regards,

Tony

Re: [PATCH 2/3] arm: introduce IRQ stacks

[Arnd Bergmann]

On Tue, Nov 10, 2020 at 1:06 PM Tony Lindgren <tony@atomide.com> wrote:

> > Are these actually ARMv6? Most ARM11 cores you'd come across
> > in practice are ARMv6K (ARM1136r1, ARM1167, ARM11MPCore),
> > in particular every SoC that has any mainline support except for
> > the ARM1136r0 based OMAP2 and i.MX3.
>
> I've been only using smp_on_up for the ARMv6 ARM1136r0 variants
> for omap2, no SMP on those.

Obviously all SMP hardware is ARMv6K, the only question I raised
in point "c)" is what we would lose by making ARMv6 (ARM1136r0)
support and SMP mutually exclusive in a kernel configuration, and
I suppose the answer remains "testing".

      Arnd

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] arm: introduce IRQ stacks

[Arnd Bergmann]

On Tue, Nov 10, 2020 at 1:06 PM Tony Lindgren <tony@atomide.com> wrote:

> > Are these actually ARMv6? Most ARM11 cores you'd come across
> > in practice are ARMv6K (ARM1136r1, ARM1167, ARM11MPCore),
> > in particular every SoC that has any mainline support except for
> > the ARM1136r0 based OMAP2 and i.MX3.
>
> I've been only using smp_on_up for the ARMv6 ARM1136r0 variants
> for omap2, no SMP on those.

Obviously all SMP hardware is ARMv6K, the only question I raised
in point "c)" is what we would lose by making ARMv6 (ARM1136r0)
support and SMP mutually exclusive in a kernel configuration, and
I suppose the answer remains "testing".

      Arnd

Re: [PATCH 2/3] arm: introduce IRQ stacks

[Tony Lindgren]

* Arnd Bergmann <arnd@kernel.org> [201110 13:35]:
> On Tue, Nov 10, 2020 at 1:06 PM Tony Lindgren <tony@atomide.com> wrote:
> 
> > > Are these actually ARMv6? Most ARM11 cores you'd come across
> > > in practice are ARMv6K (ARM1136r1, ARM1167, ARM11MPCore),
> > > in particular every SoC that has any mainline support except for
> > > the ARM1136r0 based OMAP2 and i.MX3.
> >
> > I've been only using smp_on_up for the ARMv6 ARM1136r0 variants
> > for omap2, no SMP on those.
> 
> Obviously all SMP hardware is ARMv6K, the only question I raised
> in point "c)" is what we would lose by making ARMv6 (ARM1136r0)
> support and SMP mutually exclusive in a kernel configuration, and
> I suppose the answer remains "testing".

Agreed that is probably the biggest reason to keep it at this point.

Regards,

Tony

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] arm: introduce IRQ stacks

[Tony Lindgren]

* Arnd Bergmann <arnd@kernel.org> [201110 13:35]:
> On Tue, Nov 10, 2020 at 1:06 PM Tony Lindgren <tony@atomide.com> wrote:
> 
> > > Are these actually ARMv6? Most ARM11 cores you'd come across
> > > in practice are ARMv6K (ARM1136r1, ARM1167, ARM11MPCore),
> > > in particular every SoC that has any mainline support except for
> > > the ARM1136r0 based OMAP2 and i.MX3.
> >
> > I've been only using smp_on_up for the ARMv6 ARM1136r0 variants
> > for omap2, no SMP on those.
> 
> Obviously all SMP hardware is ARMv6K, the only question I raised
> in point "c)" is what we would lose by making ARMv6 (ARM1136r0)
> support and SMP mutually exclusive in a kernel configuration, and
> I suppose the answer remains "testing".

Agreed that is probably the biggest reason to keep it at this point.

Regards,

Tony