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

On Fri, 15 Jul 2011, Wolfgang BETZ wrote:

Hallo Frank,

On 07/14/2011 12:24 PM, Frank Hofmann wrote:

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


[ ... ]


+/*
+ * 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();
+}



While I'm not qualified to comment on the technical correctness of the
patch, I've got a few questions about the way this change is done:

Specifically:

1. about the above, (unsigned int)calc_context_id(), the cast and/or
   the data type isn't required, the asm doesn't care about types.


Well, this is not in because of the asm statement but just in order to make the compiler happier regarding the initialization of ctxid, i.e. to avoid getting a potential warning about incompatible types in an assignment. In any case it's a no-op.



Generally:

2. the patch changes the signature of cpu_switch_mm() but not all the
   callers (doesn't touch the kexec / reset / suspend paths), why ?


Could you pls. specify more precisely (i.e. kernel source file, line number, etc.) which calls to cpu_switch_mm() you are exactly referring to?

Ah, ok, I see now ...


We were a bit ahead of the world; Will Deacon and myself have recently 
(June) submitted patches for discussion that require / use MMU-off 
codepaths.

These:

http://www.spinics.net/lists/arm-kernel/msg127556.html
http://www.spinics.net/lists/arm-kernel/msg128605.html


To get that, the method is to put 1:1 mappings into the pagedir in use, so 
that cpu_reset() on ARMv6/7 can switch the MMU off just as it does on all 
other architectures.


The pagedir best suitable for this is the initial one, swapper_pg_dir, and 
hence the abovementioned changes call

 	cpu_switch_mm(swapper_pg_dir, &init_mm)

What should Will and myself do there ?


Again, I think the interface change is unnecessarily complicated - three 
versions of a wrapper, some of them empty, arg#2 to cpu_switch_mm 
scope-reduced from a full mmu context reference to an integer.

If using the task struct input is unavoidable, then why not at least make 
it resemble the generic switch_mm signature ? Like:

--- arch/arm/include/asm/proc-fns.h	2011-06-15 14:28:06.261757007 +0100
+++ x	2011-07-15 16:36:28.820353998 +0100

@@ -97,7 +97,7 @@

  #ifdef CONFIG_MMU

-#define cpu_switch_mm(pgd,mm) cpu_do_switch_mm(virt_to_phys(pgd),mm)
+#define cpu_switch_mm(pgd,mm,tsk) cpu_do_switch_mm(virt_to_phys(pgd) calc_context_id(mm, tsk))

  #define cpu_get_pgd()	\
  	({						\

That's just an idea and sneaky because it forces calc_context_id() to be a 
macro by drawing the comma in so the argument marshaling can be completely 
discarded at compile time on cpu types where it's not used/useful.

The reset code could then:

 	cpu_switch_mm(swapper_pg_dir, &init_mm, &init_task);



Is there a special need for doing the change _the way you proposed_ ?


FrankH.