[PATCH 0/1 V2] Add Thread Support for the Context ID Register of ARM v6 & v7 Architectures

[PATCH 0/1 V2] Add Thread Support for the Context ID Register of ARM v6 & v7 Architectures

[Wolfgang BETZ]

From: Wolfgang Betz <wolfgang.betz@st.com>

The Context ID Register, CONTEXTIDR, identifies the current:
- Process Identifier (PROCID) &
- Address Space Identifier (ASID).

The value of the whole of this register is called the Context ID and is 
used by:
- the ARM debug logic, for Linked and Unlinked Context ID matching
  (e.g. for breakpoint debug and watchpoint debug events).
- the trace logic, to identify the current process.

The CONTEXTIDR is a 32-bit read/write register whose format is:
- PROCID, bits [31:8]
  Process Identifier. This field should be programmed with a unique 
  value that identifies the current process and is used by the trace logic and 
  the debug logic to identify the process that is running currently.
- ASID, bits [7:0]
  Address Space Identifier. This field must be programmed with the 
  value of the current ASID and is used by many memory management functions.

This change-set aims at:
- implementing thread tracing support based on the armv6 & v7 CONTEXTIDR 
  register while leaving the Linux kernel ASID functionality (semantically)
  unchanged.
- focusing on compatibility with tracing tools such as Lauterbach's 
  TRACE32 tool.
- the avoidance of platform specific calls in generic code.
- simplicity, readability, and good performances.
- being general: i.e. the change-set applies to all armv7/v6 platforms &
  is in general compilable for all (other) platforms.

The patch has been jointly developed by 
Lauterbach GmbH (http://www.lauterbach.com) and 
STMicroelectronics Srl (http://www.st.com/). 
Main contributors are:
- Khaled Jmal <khaled.jmal@lauterbach.com>
- Rudi Dienstbeck <Rudolf.Dienstbeck@Lauterbach.com>
- Wolfgang Betz <wolfgang.betz@st.com>


Wolfgang Betz (1):
  Add Thread Support for the Context ID Register of ARM v6 & v7
    Architectures

 arch/arm/Kconfig.debug             |   13 +++++++
 arch/arm/include/asm/mmu_context.h |   63 +++++++++++++++++++++++++++++++----
 arch/arm/kernel/smp.c              |    2 +-
 arch/arm/mm/context.c              |   42 +++++++++++++++++++++++-
 arch/arm/mm/proc-v6.S              |    1 -
 arch/arm/mm/proc-v7.S              |    1 -
 6 files changed, 109 insertions(+), 13 deletions(-)

-- 
1.7.4.4

[PATCH 1/1 V2] Add Thread Support for the Context ID Register of ARM v6 & v7 Architectures

[Wolfgang BETZ]

From: Wolfgang Betz <wolfgang.betz@st.com>

The aim of this patch is to enable thread support in the context ID register
(CONTEXTIDR) as it comes with ARM architectures v6 & v7.

 On ARMv6 & v7, we have the following structure in the context ID:

   31                         7          0
   +-------------------------+-----------+
   |      process ID         |   ASID    |
   +-------------------------+-----------+
   |              context ID             |
   +-------------------------------------+

- The ASID is used to tag entries in the CPU caches and TLBs.
- The process ID must be programmed with a unique value that identifies the
  current process. It is used by the trace logic and the debug logic to
  identify the process that is running currently.

Currently the Linux kernel does correctly support the ASID field of the register,
but does not make use of the process ID in a way that would allow trace logic to
efficiently identify context switches.
In order to achieve this, this patch modifies 6 files of the kernel as described
hereafter:
- First a new configuration variable THREAD_CONTEXTID has been introduced in
  file "arch/arm/Kconfig.debug", which basically enables the patch (if not
  enabled, the kernel behaves as if it would not have been modified at all).
  This configuration variable depends obviously on the presence of a context
  ID register and automatically selects TRACING as the patch is partially based
  on tracepoints. Furthermore it enables both DEBUG_KERNEL and DEBUG_INFO as it
  is supposed that a backend tool which will analyze the generated trace will
  require access to debugging information like e.g. the debug symbols of the
  kernel and modules.
- The major part of the modifications of this patch are concentrated in file
  "arch/arm/include/asm/mmu_context.h", where a new function, i.e.
  "calc_context_id", has been introduced. The objective of this function is to
  calculate the contents of the context ID register (CONTEXTIDR), as described
  above, which should be moved into this register on the next context switch.
  Furthermore, there is a new convenience function, i.e. "set_context_id",
  which allows to set the CONTEXTIDR based on the outcome of a call to
  "calc_context_id". Finally, function "switch_mm" has been modified similarly
  by replacing the second argument in the call to "cpu_switch_mm" with the
  outcome of a call to "calc_context_id". The very same modification was
  necessary also in function "secondary_start_kernel" of file
  "arch/arm/kernel/smp.c".
- File "arch/arm/mm/context.c" has been modified for one thing to make use of the
  new convenience function "set_context_id", for another thing to register a new
  "sched_switch" tracepoint function to trace thread switches not already covered
  by "switch_mm".
- Finally, functions "cpu_v6_switch_mm" and "cpu_v7_switch_mm", in files
  "arch/arm/mm/proc-v6.S" and "arch/arm/mm/proc-v7.S" respectively, have been
  modified so that these expect now the content to be moved into CONTEXTIDR to be
  directly passed as second argument (i.e. within "r1").

Signed-off-by: Wolfgang Betz <wolfgang.betz@st.com>
---
 arch/arm/Kconfig.debug             |   13 +++++++
 arch/arm/include/asm/mmu_context.h |   63 +++++++++++++++++++++++++++++++----
 arch/arm/kernel/smp.c              |    2 +-
 arch/arm/mm/context.c              |   42 +++++++++++++++++++++++-
 arch/arm/mm/proc-v6.S              |    1 -
 arch/arm/mm/proc-v7.S              |    1 -
 6 files changed, 109 insertions(+), 13 deletions(-)

diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug
index 81cbe40..0b20c04 100644
--- a/arch/arm/Kconfig.debug
+++ b/arch/arm/Kconfig.debug
@@ -129,4 +129,17 @@ config DEBUG_S3C_UART
 	  The uncompressor code port configuration is now handled
 	  by CONFIG_S3C_LOWLEVEL_UART_PORT.
 
+config THREAD_CONTEXTID
+	bool "Enable thread support for the Context ID Register"
+	depends on CPU_HAS_ASID
+	default n
+	select DEBUG_KERNEL
+	select DEBUG_INFO
+	select TRACING
+	help
+	  Say Y here if you want to enable thread support for the trace logic
+	  of tools such as Lauterbach's TRACE32 tool.
+	  This thread tracing support is based on the CONTEXTIDR register of
+	  architectures like the ARM v6 or v7.
+
 endmenu
diff --git a/arch/arm/include/asm/mmu_context.h b/arch/arm/include/asm/mmu_context.h
index 71605d9..299cff4 100644
--- a/arch/arm/include/asm/mmu_context.h
+++ b/arch/arm/include/asm/mmu_context.h
@@ -24,7 +24,7 @@ void __check_kvm_seq(struct mm_struct *mm);
 #ifdef CONFIG_CPU_HAS_ASID
 
 /*
- * On ARMv6, we have the following structure in the Context ID:
+ * On ARMv6 & v7, we have the following structure in the Context ID:
  *
  * 31                         7          0
  * +-------------------------+-----------+
@@ -34,8 +34,9 @@ void __check_kvm_seq(struct mm_struct *mm);
  * +-------------------------------------+
  *
  * The ASID is used to tag entries in the CPU caches and TLBs.
- * The context ID is used by debuggers and trace logic, and
- * should be unique within all running processes.
+ * The process ID must be programmed with a unique value that identifies the
+ * current process. It is used by the trace logic and the debug logic
+ * to identify the process that is running currently.
  */
 #define ASID_BITS		8
 #define ASID_MASK		((~0) << ASID_BITS)
@@ -68,7 +69,53 @@ static inline void check_context(struct mm_struct *mm)
 
 #define init_new_context(tsk,mm)	(__init_new_context(tsk,mm),0)
 
-#else
+#ifdef CONFIG_THREAD_CONTEXTID
+/*
+ * Calculate context ID for task and mm
+ */
+static inline
+struct mm_struct *calc_context_id(struct task_struct *tsk,
+				  struct mm_struct *mm)
+{
+	unsigned int ret;
+
+	if (unlikely(tsk == NULL))
+		ret = (current->pid << ASID_BITS);
+	else
+		ret = (tsk->pid << ASID_BITS);
+
+
+	if (unlikely(!ret))
+		ret = (0xFFFFFFFF << ASID_BITS);
+
+	return (struct mm_struct *)((mm->context.id & ~ASID_MASK) | ret);
+}
+#else /* !CONFIG_THREAD_CONTEXTID */
+/*
+ * Calculate context ID for task and mm
+ */
+static inline
+struct mm_struct *calc_context_id(struct task_struct *tsk,
+				  struct mm_struct *mm)
+{
+	return (struct mm_struct *)(mm->context.id);
+}
+#endif /* !CONFIG_THREAD_CONTEXTID */
+
+/*
+ * Set context ID for task and mm
+ */
+static inline
+void set_context_id(struct task_struct *tsk, struct mm_struct *mm)
+{
+	unsigned int ctxid = (unsigned int)calc_context_id(tsk, mm);
+
+	/* set the new ContextID */
+	asm("mcr	p15, 0, %0, c13, c0, 1\n" : : "r" (ctxid));
+	isb();
+}
+
+#else /* !CONFIG_CPU_HAS_ASID */
 
 static inline void check_context(struct mm_struct *mm)
 {
@@ -78,9 +125,9 @@ static inline void check_context(struct mm_struct *mm)
 #endif
 }
 
-#define init_new_context(tsk,mm)	0
-
-#endif
+#define init_new_context(tsk, mm)	0
+#define calc_context_id(tsk, mm)	(mm)
+#endif /* !CONFIG_CPU_HAS_ASID */
 
 #define destroy_context(mm)		do { } while(0)
 
@@ -123,7 +170,7 @@ switch_mm(struct mm_struct *prev, struct mm_struct *next,
 		*crt_mm = next;
 #endif
 		check_context(next);
-		cpu_switch_mm(next->pgd, next);
+		cpu_switch_mm(next->pgd, calc_context_id(tsk, next));
 		if (cache_is_vivt())
 			cpumask_clear_cpu(cpu, mm_cpumask(prev));
 	}
diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index e7f92a4..a50725e 100644
--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c
@@ -288,7 +288,7 @@ asmlinkage void __cpuinit secondary_start_kernel(void)
 	atomic_inc(&mm->mm_count);
 	current->active_mm = mm;
 	cpumask_set_cpu(cpu, mm_cpumask(mm));
-	cpu_switch_mm(mm->pgd, mm);
+	cpu_switch_mm(mm->pgd, calc_context_id(current, mm));
 	enter_lazy_tlb(mm, current);
 	local_flush_tlb_all();
 
diff --git a/arch/arm/mm/context.c b/arch/arm/mm/context.c
index b0ee9ba..8f63718 100644
--- a/arch/arm/mm/context.c
+++ b/arch/arm/mm/context.c
@@ -16,6 +16,10 @@
 #include <asm/mmu_context.h>
 #include <asm/tlbflush.h>
 
+#ifdef CONFIG_THREAD_CONTEXTID
+#include <trace/events/sched.h>
+#endif
+
 static DEFINE_SPINLOCK(cpu_asid_lock);
 unsigned int cpu_last_asid = ASID_FIRST_VERSION;
 #ifdef CONFIG_SMP
@@ -99,8 +103,7 @@ static void reset_context(void *info)
 	set_mm_context(mm, asid);
 
 	/* set the new ASID */
-	asm("mcr	p15, 0, %0, c13, c0, 1\n" : : "r" (mm->context.id));
-	isb();
+	set_context_id(current, mm);
 }
 
 #else
@@ -155,3 +158,38 @@ void __new_context(struct mm_struct *mm)
 	set_mm_context(mm, asid);
 	spin_unlock(&cpu_asid_lock);
 }
+
+#ifdef CONFIG_THREAD_CONTEXTID
+/*
+ * Add support for threads in CONTEXTIDR by registering a
+ * 'sched_switch' tracepoint event function
+ */
+static void thrctx_sched_switch(void *ignore, struct task_struct *prev,
+				struct task_struct *next)
+{
+	struct mm_struct *mm, *oldmm;
+
+	mm = next->mm;
+	oldmm = prev->active_mm;
+
+	if (!mm) {
+		set_context_id(next, oldmm);
+	} else {
+		if (oldmm == mm)
+			set_context_id(next, mm);
+	}
+}
+
+static int __init init_thread_contextid(void)
+{
+	int ret;
+
+	ret = register_trace_sched_switch(thrctx_sched_switch, NULL);
+	if (ret)
+		pr_info("ftrace_graph: Couldn't activate tracepoint"
+			" probe to kernel_sched_switch\n");
+
+	return ret;
+}
+device_initcall(init_thread_contextid);
+#endif /* CONFIG_THREAD_CONTEXTID */
diff --git a/arch/arm/mm/proc-v6.S b/arch/arm/mm/proc-v6.S
index 1d2b845..57f3574 100644
--- a/arch/arm/mm/proc-v6.S
+++ b/arch/arm/mm/proc-v6.S
@@ -93,7 +93,6 @@ ENTRY(cpu_v6_dcache_clean_area)
 ENTRY(cpu_v6_switch_mm)
 #ifdef CONFIG_MMU
 	mov	r2, #0
-	ldr	r1, [r1, #MM_CONTEXT_ID]	@ get mm->context.id
 	ALT_SMP(orr	r0, r0, #TTB_FLAGS_SMP)
 	ALT_UP(orr	r0, r0, #TTB_FLAGS_UP)
 	mcr	p15, 0, r2, c7, c5, 6		@ flush BTAC/BTB
diff --git a/arch/arm/mm/proc-v7.S b/arch/arm/mm/proc-v7.S
index 089c0b5..f725698 100644
--- a/arch/arm/mm/proc-v7.S
+++ b/arch/arm/mm/proc-v7.S
@@ -102,7 +102,6 @@ ENDPROC(cpu_v7_dcache_clean_area)
 ENTRY(cpu_v7_switch_mm)
 #ifdef CONFIG_MMU
 	mov	r2, #0
-	ldr	r1, [r1, #MM_CONTEXT_ID]	@ get mm->context.id
 	ALT_SMP(orr	r0, r0, #TTB_FLAGS_SMP)
 	ALT_UP(orr	r0, r0, #TTB_FLAGS_UP)
 #ifdef CONFIG_ARM_ERRATA_430973
-- 
1.7.4.4

[PATCH 1/1 V2] Add Thread Support for the Context ID Register of ARM v6 & v7 Architectures

[Will Deacon]

Hi Wolfgang,

On Thu, Jul 14, 2011 at 10:33:10AM +0100, Wolfgang BETZ wrote:
> From: Wolfgang Betz <wolfgang.betz@st.com>
> 
> The aim of this patch is to enable thread support in the context ID register
> (CONTEXTIDR) as it comes with ARM architectures v6 & v7.
> 
>  On ARMv6 & v7, we have the following structure in the context ID:
> 
>    31                         7          0
>    +-------------------------+-----------+
>    |      process ID         |   ASID    |
>    +-------------------------+-----------+
>    |              context ID             |
>    +-------------------------------------+
> 
> - The ASID is used to tag entries in the CPU caches and TLBs.
> - The process ID must be programmed with a unique value that identifies the
>   current process. It is used by the trace logic and the debug logic to
>   identify the process that is running currently.

We also use upper bits (>= 8) to identify the ASID generation, so you can't
just start putting data in there as this will break the rollover logic.

As Russell said previously, I really don't see how this can work with
current CPUs. Maybe you're better off using one of the unused thread ID
registers if you want to do this sort of thing.

Will

[PATCH 1/1 V2] Add Thread Support for the Context ID Register of ARM v6 & v7 Architectures

[Will Deacon]

On Thu, Jul 14, 2011 at 12:07:00PM +0100, Wolfgang BETZ wrote:
> Ciao Will,

Hello,

> maybe there is a basic misunderstanding: do we speak about the actual CONTEXTID
> register or the data structures in the Linux kernel which represents this
> register?

Sorry, I didn't see that you'd changed the context ID writes so that they
write the masked value. I also looked to see if there's a handy thread ID
register you can use instead but there isn't (TPIDRRO is used for the TLS).

Looking again at the structure of your code (FrankH has already pointed out
a lot of the implementation issues), perhaps this would be better off as a
(configurable) thread notifier which reads the context ID, sets the upper
bits (taking care to preserve the ASID) and writes it back.

This is far less invasive, is easy to enable/disable and is also easy to
implement for future cores without requiring changes to older CPUs.

Would that work?

Will

[PATCH 1/1 V2] Add Thread Support for the Context ID Register of ARM v6 & v7 Architectures

[Will Deacon]

On Mon, Jul 18, 2011 at 12:52:00PM +0100, Wolfgang BETZ wrote:
> Hello Will,
> 
> On 07/14/2011 01:36 PM, Will Deacon wrote:
> 
>     Looking again at the structure of your code (FrankH has already pointed out
>     a lot of the implementation issues), perhaps this would be better off as a
>     (configurable) thread notifier which reads the context ID, sets the upper
>     bits (taking care to preserve the ASID) and writes it back.

Wolfgang,

> Yes, there are already trace events in the kernel that can be used to trace a
> thread switch. The patch already uses it for writing the CONTEXTIDR.

No, don't use trace events. Use a thread notifier and when you get
THREAD_NOTIFY_SWITCH you can read the context ID, write the PID to the top
bits and write it back. This will occur after switch_mm so the ASID will
already be in place. You will need to disable interrupts during this so that
you don't get an ASID rollover during the read-modify-write. You will also
need to change switch_mm so that it preserves the upper bits of the context
ID.

> However, this is not enough. The kernel itself writes the CONTEXTIDR (together
> with the wrapper id) *after* the scheduler called the event trace.
> Consequently, in the ETM trace there would be two writes to the CONTEXTIDR
> register, and it would be difficult (or even impossible) to find out, if it was
> the additional write for a thread switch or the original "Linux" write for a
> process switch. That's why a patch to the original writing of the CONTEXTIDR is
> still necessary.
> What do you think about this?

I don't understand the problem here. Seeing two context ID writes in your
event stream is up to your tools to handle. How do you handle ASID rollover
broadcasts where the context ID can be updated at any point? That surely is
far more complicated than the case of a process switch, where we switch the
mm and then write the PID.

I still maintain that you will struggle to get this code past Russell (based
on his previous comments) so keeping the changes to a minimum is in your
best interest if you want to convince him to merge it.

Will

[PATCH 1/1 V2] Add Thread Support for the Context ID Register of ARM v6 & v7 Architectures

[Will Deacon]

On Mon, Jul 18, 2011 at 03:34:20PM +0100, Wolfgang BETZ wrote:
> Hi Will,
> 
> On 07/18/2011 02:57 PM, Will Deacon wrote:
> 
>     No, don't use trace events. Use a thread notifier and when you get
>     THREAD_NOTIFY_SWITCH you can read the context ID, write the PID to the top
>     bits and write it back. This will occur after switch_mm so the ASID will
>     already be in place. You will need to disable interrupts during this so that
>     you don't get an ASID rollover during the read-modify-write. You will also
>     need to change switch_mm so that it preserves the upper bits of the context
>     ID.
> 
> OK, I think I understood now your idea and do believe that this would be
> feasible absolutely from the kernel's point of view.

Good stuff.

> 
>         However, this is not enough. The kernel itself writes the CONTEXTIDR (together
>         with the wrapper id) *after* the scheduler called the event trace.
>         Consequently, in the ETM trace there would be two writes to the CONTEXTIDR
>         register, and it would be difficult (or even impossible) to find out, if it was
>         the additional write for a thread switch or the original "Linux" write for a
>         process switch. That's why a patch to the original writing of the CONTEXTIDR is
>         still necessary.
>         What do you think about this?
> 
>     I don't understand the problem here. Seeing two context ID writes in your
>     event stream is up to your tools to handle. How do you handle ASID rollover
>     broadcasts where the context ID can be updated at any point? That surely is
>     far more complicated than the case of a process switch, where we switch the
>     mm and then write the PID.
> 
> Well, the tools are not interested in the ASID at all, they are interested in
> getting a correct PID representing a real thread. Therefore ASID rollover isn't
> a problem for the tools, while I think that the major problem with your
> solution is, how a tool can distinguish between a "fake" PID which is used for
> ASID rollover handling and a real PID as written by the notifier. Honestly, I
> would not know how to do this :-(   ... any idea would be welcome.

We don't use fake PIDs for anything. The only thing that can go wrong is
during task switch where we switch the mm and then switch the PID in your
thread notifier. Reading the context ID between these two points will give
you the PID of the previous task. I don't think this is a big problem since
we're in the middle of context switch anyway, so it's hard to define when
the thread switch has completed. You just have to be aware that the
current mm_struct / ASID may not correspond to the PID. One nice property is
that you can be sure the PID is correct once you've returned to userspace.

Will

[PATCH 1/1 V2] Add Thread Support for the Context ID Register of ARM v6 & v7 Architectures

[Will Deacon]

On Tue, Jul 19, 2011 at 06:21:41AM +0100, Wolfgang BETZ wrote:
>     We don't use fake PIDs for anything. The only thing that can go wrong is
>     during task switch where we switch the mm and then switch the PID in your
>     thread notifier. Reading the context ID between these two points will give
>     you the PID of the previous task.
> 
> Well, don't we set the CONTEXTIDR also during mm switch (in cpu_switch_mm())?
> And in this occasion we will copy the value coming from mm->context.id, i.e.
> with a "PID" value which does not correspond to a thread identifier but to what
> is used for ASID rollover handling!?! I am afraid that this will/can confuse
> external tools.

Yes, but that's easy to fix. We just make switch_mm do read-modify-write of
the upper bits. It also doesn't require us to change the type signature of
that function (see patch below)

> Furthermore, I do not like the idea that we are sending out over the trace
> logic two times the CONTEXTIDR content for each context switch. Seems not to be
> a very clean solution to me :-\

Fair enough, but I think it's a lot better than changing the ASID handling
code.

>     I don't think this is a big problem since
>     we're in the middle of context switch anyway, so it's hard to define when
>     the thread switch has completed. You just have to be aware that the
>     current mm_struct / ASID may not correspond to the PID. One nice property is
>     that you can be sure the PID is correct once you've returned to userspace.
> 
> OK, once you return to user-space everything will be good, but the tools we are
> talking about here do not care (only) about user- or kernel-space, these want
> to trace both (simultaneously)!

I was just giving an example as to an easy way of knowing when the PID is in
sync with the ASID by looking at traces.

Will


So here's something I hacked up. I've compiled it but that's all. I'm also
not sure that the notifier belongs in mm/context.c and you may or may not want
an isb after updating the register.


diff --git a/arch/arm/mm/context.c b/arch/arm/mm/context.c
index b0ee9ba..59ab68a 100644
--- a/arch/arm/mm/context.c
+++ b/arch/arm/mm/context.c
@@ -14,6 +14,7 @@
 #include <linux/percpu.h>
 
 #include <asm/mmu_context.h>
+#include <asm/thread_notify.h>
 #include <asm/tlbflush.h>
 
 static DEFINE_SPINLOCK(cpu_asid_lock);
@@ -155,3 +156,43 @@ void __new_context(struct mm_struct *mm)
 	set_mm_context(mm, asid);
 	spin_unlock(&cpu_asid_lock);
 }
+
+#ifdef CONFIG_PID_IN_CONTEXTIDR
+static int contextidr_notifier(struct notifier_block *unused, unsigned long cmd,
+			       void *t)
+{
+	unsigned long flags;
+	u32 contextid;
+	struct thread_info *thread = t;
+
+	if (cmd != THREAD_NOTIFY_SWITCH)
+		return NOTIFY_DONE;
+
+	local_irq_save(flags);
+	asm volatile("mrc	p15, 0, %0, c15, c0, 1\n"
+		     "bic	%0, %0, #0xffffff00\n"
+		     "orr	%0, %0, %1, lsl #8\n"
+		     "mcr	p15, 0, %0, c15, c0, 1"
+		     : "=&r" (contextid)
+		     : "r" (task_tgid_vnr(thread->task)));
+	local_irq_restore(flags);
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block contextidr_notifier_block = {
+	.notifier_call = contextidr_notifier,
+};
+
+static int __init contextidr_init(void)
+{
+	unsigned int cpu_arch = cpu_architecture();
+
+	if (cpu_arch < CPU_ARCH_ARMv6)
+		return 0;
+
+	thread_register_notifier(&contextidr_notifier_block);
+	return 0;
+}
+arch_initcall(contextidr_init);
+#endif
diff --git a/arch/arm/mm/proc-v7.S b/arch/arm/mm/proc-v7.S
index 089c0b5..3605c00 100644
--- a/arch/arm/mm/proc-v7.S
+++ b/arch/arm/mm/proc-v7.S
@@ -111,6 +111,12 @@ ENTRY(cpu_v7_switch_mm)
 #ifdef CONFIG_ARM_ERRATA_754322
 	dsb
 #endif
+#ifdef CONFIG_PID_IN_CONTEXTIDR
+	mrc	p15, 0, r2, c15, c0, 1
+	bic	r2, r2, #0xff
+	bfc	r1, #8, #24
+	orr	r1, r1, r2
+#endif
 	mcr	p15, 0, r2, c13, c0, 1		@ set reserved context ID
 	isb
 1:	mcr	p15, 0, r0, c2, c0, 0		@ set TTB 0

[PATCH 1/1 V2] Add Thread Support for the Context ID Register of ARM v6 & v7 Architectures

[Russell King - ARM Linux]

On Mon, Jul 18, 2011 at 01:57:19PM +0100, Will Deacon wrote:
> I still maintain that you will struggle to get this code past Russell (based
> on his previous comments) so keeping the changes to a minimum is in your
> best interest if you want to convince him to merge it.

One of the reasons that there'll be a struggle is the abuse that's in the
patch.

If Wolfgang wants to pass something into a function which isn't already
being passed, then the prototype needs to be changed - and all
implementations and users need to be fixed up for that change.  Fudging
it with casts to an existing arguments type so something else can be
passed is just not on.

How does Wolfgang know that he's fixed up everywhere which calls
cpu_switch_mm() to ensure that it now passes the context ID value in r1
rather than the struct mm_struct pointer?  Or more to the point, how do
we know that there isn't a new call to cpu_switch_mm() which hasn't been
fixed up.  There is no way for the compiler to tell us because the
information is hidden from the compiler by those casts.

C is a typed language for a reason.  Don't destroy it with casts.

So, the _minimum_ that needs to change in this patch is for those casts
to go, and cpu_switch_mm() needs to be fixed to take the context ID
value, rather a context ID value casted to a mm_struct.  If that results
in the mm_struct argument not being used by any implementation, that
argument can then be removed.

[PATCH 1/1 V2] Add Thread Support for the Context ID Register of ARM v6 & v7 Architectures

[Russell King - ARM Linux]

On Mon, Jul 25, 2011 at 12:12:29PM +0200, Wolfgang BETZ wrote:
> Hi Russell,
> 
> On 07/19/2011 12:23 PM, Russell King - ARM Linux wrote:
> 
> 
> One of the reasons that there'll be a struggle is the abuse that's in the
> patch.
> 
> If Wolfgang wants to pass something into a function which isn't already
> being passed, then the prototype needs to be changed - and all
> implementations and users need to be fixed up for that change.  Fudging
> it with casts to an existing arguments type so something else can be
> passed is just not on.
> 
> How does Wolfgang know that he's fixed up everywhere which calls
> cpu_switch_mm() to ensure that it now passes the context ID value in r1
> rather than the struct mm_struct pointer?  Or more to the point, how do
> we know that there isn't a new call to cpu_switch_mm() which hasn't been
> fixed up.  There is no way for the compiler to tell us because the
> information is hidden from the compiler by those casts.
> 
> Please note, that only for ARM v6 and v7 the context ID value will be passed in r1 at the place of the struct mm_struct pointer. For other platforms/architecture nothing has changed!
> So the compiler will (continue to) throw out a warning, in case you pass something else which is not a pointer to mm_struct.
> 
> 
> 
> 
> C is a typed language for a reason.  Don't destroy it with casts.
> 
> So, the _minimum_ that needs to change in this patch is for those casts
> to go, and cpu_switch_mm() needs to be fixed to take the context ID
> value, rather a context ID value casted to a mm_struct.
> 
> Well, this is exactly what the patch is doing right now.
> I have intentionally avoided to centralize this cast into cpu_switch_mm() in order to be sure to not miss any call to it, accidentally. As said above, the compiler will warn about something like this.
> Maybe you could take a closer look at v3 of the patch, which I will send out within today.

You've completely missed my point.  What's the really scary thing here
is that you can't see that you're doing something very very wrong.

Think about this case:

	static inline struct mm_struct *some_function(struct mm_struct *mm)
	{
		return (struct mm_struct *)mm->context.id;
	}

	cpu_switch_mm(pgd, some_function(mm));

and somewhere else there's another call to:

	cpu_switch_mm(pgd, mm);

You've destroyed the ability of the compiler to spot that error because
of your insistance to use idiotic casts and avoid doing the job properly.
Had you changed the second argument to 'unsigned long' or 'u32' or
something like that - or even added that - the compiler would be able to
spot the calls which hadn't been fixed up.

So, here's what I want you to do:

1. Change the cpu_switch_mm() prototype.
2. Get rid of the madness of sometimes passing a real mm_struct and
   sometimes passing the context ID value depending on configuration.

Here's some examples:

	cpu_switch_mm(pgd, mm);

where mm is a real mm_struct => good.

	cpu_switch_mm(pgd, some_function(mm));

	#ifdef CONFIG_I_WANTING_TO_BE_RANTED_AT
	static inline struct mm_struct *some_function(struct mm_struct *mm)
	{
		return (struct mm_struct *)mm->context.id;
	}
	#else
	static inline struct mm_struct *some_function(struct mm_struct *mm)
	{
		return mm;
	}
	#endif

unacceptable, potentially buggy, violates the principles of C's type
checking etc.

	cpu_switch_mm(pgd, mm, context_id(mm));

better.

	cpu_switch_mm(pgd, context_id(mm));

even better if 'mm' becomes unused by _all_ implementations of
cpu_switch_mm().

[PATCH 1/1 V2] Add Thread Support for the Context ID Register of ARM v6 & v7 Architectures

[Will Deacon]

On Mon, Jul 25, 2011 at 11:33:23AM +0100, Russell King - ARM Linux wrote:
> On Mon, Jul 25, 2011 at 12:12:29PM +0200, Wolfgang BETZ wrote:
> > C is a typed language for a reason.  Don't destroy it with casts.
> > 
> > So, the _minimum_ that needs to change in this patch is for those casts
> > to go, and cpu_switch_mm() needs to be fixed to take the context ID
> > value, rather a context ID value casted to a mm_struct.
> > 
> > Well, this is exactly what the patch is doing right now.
> > I have intentionally avoided to centralize this cast into cpu_switch_mm() in order to be sure to not miss any call to it, accidentally. As said above, the compiler will warn about something like this.
> > Maybe you could take a closer look at v3 of the patch, which I will send out within today.
> 
> You've completely missed my point.  What's the really scary thing here
> is that you can't see that you're doing something very very wrong.

I'd still like to know why we can't avoid piggybacking on the switch_mm code
by doing similar to the hack I posted here:

http://lists.infradead.org/pipermail/linux-arm-kernel/2011-July/058436.html

Given that future cores (>= A15) put the ASID in a different register and
there's apparently a need for a thread switch notifier anyway, tying the
contextidr up so tightly with switch_mm doesn't feel right to me.

So, ugly casts aside, I'm not convinced by the approach taken here.

Will