The big.LITTLE In-Kernel Switcher (IKS), part 1

Currently we hash the MPIDR of the CPU being suspended to determine which entry in the sleep_save_sp array to use. In some situations, such as when we want to resume on another physical CPU, the MPIDR of another CPU should be used instead.

So let's use the value of cpu_logical_map(smp_processor_id()) in place of the MPIDR in the suspend path. This will result in the same index being used as with the previous code unless the caller has modified cpu_logical_map() beforehand.

The register allocation in __cpu_suspend is reworked in order to better accommodate the additional argument.

Signed-off-by Nicolas Pitre <[email protected]>

---
 arch/arm/kernel/sleep.S   | 25 +++++++++++--------------
 arch/arm/kernel/suspend.c |  8 +++++---
 2 files changed, 16 insertions(+), 17 deletions(-)

arch/arm/kernel/sleep.S +11 -14

index db1536b8b3..836d10e698 100644
@@ -55,6 +55,7 @@
  * specific registers and some other data for resume.
  *  r0 = suspend function arg0
  *  r1 = suspend function
+ *  r2 = MPIDR value used to index into sleep_save_sp
  */
 ENTRY(__cpu_suspend)
 	stmfd	sp!, {r4 - r11, lr}

@@ -67,23 +68,19 @@ ENTRY(__cpu_suspend)
 	mov	r5, sp			@ current virtual SP
 	add	r4, r4, #12		@ Space for pgd, virt sp, phys resume fn
 	sub	sp, sp, r4		@ allocate CPU state on stack
-	stmfd	sp!, {r0, r1}		@ save suspend func arg and pointer
-	add	r0, sp, #8		@ save pointer to save block
-	mov	r1, r4			@ size of save block
-	mov	r2, r5			@ virtual SP
 	ldr	r3, =sleep_save_sp
+	stmfd	sp!, {r0, r1}		@ save suspend func arg and pointer
 	ldr	r3, [r3, #SLEEP_SAVE_SP_VIRT]
-	ALT_SMP(mrc p15, 0, r9, c0, c0, 5)
-        ALT_UP_B(1f)
-	ldr	r8, =mpidr_hash
-	/*
-	 * This ldmia relies on the memory layout of the mpidr_hash
-	 * struct mpidr_hash.
-	 */
-	ldmia	r8, {r4-r7}	@ r4 = mpidr mask (r5,r6,r7) = l[0,1,2] shifts
-	compute_mpidr_hash	lr, r5, r6, r7, r9, r4
-	add	r3, r3, lr, lsl #2
+	ALT_SMP(ldr r0, =mpidr_hash)
+       ALT_UP_B(1f)
+	/* This ldmia relies on the memory layout of the mpidr_hash struct */
+	ldmia	r0, {r1, r6-r8}	@ r1 = mpidr mask (r6,r7,r8) = l[0,1,2] shifts
+	compute_mpidr_hash	r0, r6, r7, r8, r2, r1
+	add	r3, r3, r0, lsl #2
 1:
+	mov	r2, r5			@ virtual SP
+	mov	r1, r4			@ size of save block
+	add	r0, sp, #8		@ pointer to save block
 	bl	__cpu_suspend_save
 	adr	lr, BSYM(cpu_suspend_abort)
 	ldmfd	sp!, {r0, pc}		@ call suspend fn

arch/arm/kernel/suspend.c +5 -3

index 41cf3cbf75..2835d35234 100644
@@ -10,7 +10,7 @@
 #include <asm/suspend.h>
 #include <asm/tlbflush.h>
 
-extern int __cpu_suspend(unsigned long, int (*)(unsigned long));
+extern int __cpu_suspend(unsigned long, int (*)(unsigned long), u32 cpuid);
 extern void cpu_resume_mmu(void);
 
 #ifdef CONFIG_MMU

@@ -21,6 +21,7 @@ extern void cpu_resume_mmu(void);
 int cpu_suspend(unsigned long arg, int (*fn)(unsigned long))
 {
 	struct mm_struct *mm = current->active_mm;
+	u32 __mpidr = cpu_logical_map(smp_processor_id());
 	int ret;
 
 	if (!idmap_pgd)

@@ -32,7 +33,7 @@ int cpu_suspend(unsigned long arg, int (*fn)(unsigned long))
 	 * resume (indicated by a zero return code), we need to switch
 	 * back to the correct page tables.
 	 */
-	ret = __cpu_suspend(arg, fn);
+	ret = __cpu_suspend(arg, fn, __mpidr);
 	if (ret == 0) {
 		cpu_switch_mm(mm->pgd, mm);
 		local_flush_bp_all();

@@ -44,7 +45,8 @@ int cpu_suspend(unsigned long arg, int (*fn)(unsigned long))
 #else
 int cpu_suspend(unsigned long arg, int (*fn)(unsigned long))
 {
-	return __cpu_suspend(arg, fn);
+	u32 __mpidr = cpu_logical_map(smp_processor_id());
+	return __cpu_suspend(arg, fn, __mpidr);
 }
 #define	idmap_pgd	NULL
 #endif

Signature · 1 line

1.8.1.2

Quoted 9 lines · Nicolas Pitre

Currently we hash the MPIDR of the CPU being suspended to determine which entry in the sleep_save_sp array to use. In some situations, such as when we want to resume on another physical CPU, the MPIDR of another CPU should be used instead.

So let's use the value of cpu_logical_map(smp_processor_id()) in place of the MPIDR in the suspend path. This will result in the same index being used as with the previous code unless the caller has modified cpu_logical_map() beforehand.

This only works because we happen to swap MPIDRs in cpu_logical_map before we get here, so that cpu_logical_map(smp_processor_id()) described the post-resume situation for this logical CPU, not the current situation.

We have to swizzle cpu_logical_map() somewhere, and the suspend path is just as good as the resume path for that.

But this patch commits us to requiring that on the suspend path specifically -- I think that ought to be mentioned somewhere. A comment in the preamble for __cpu_suspend would be enough, I think.

Looks like the patch should work though, and it does remove the need to read the MPIDR, which is slightly nicer for the non-switcher case.

Quoted 11 lines · Dave Martin

Quoted 9 lines · Nicolas Pitre

Currently we hash the MPIDR of the CPU being suspended to determine which entry in the sleep_save_sp array to use. In some situations, such as when we want to resume on another physical CPU, the MPIDR of another CPU should be used instead.

So let's use the value of cpu_logical_map(smp_processor_id()) in place of the MPIDR in the suspend path. This will result in the same index being used as with the previous code unless the caller has modified cpu_logical_map() beforehand.

Quoted 4 lines

This only works because we happen to swap MPIDRs in cpu_logical_map before we get here, so that cpu_logical_map(smp_processor_id()) described the post-resume situation for this logical CPU, not the current situation.

Yes, that's more or less what I said above. Maybe that should be clarified further?

Quoted 2 lines

We have to swizzle cpu_logical_map() somewhere, and the suspend path is just as good as the resume path for that.

Not quite. On the resume path, we have to get to the context save array while having no mmu active and no stack. There is no way we could take over another CPU's context in those conditions without making it extremely painful on ourselves. It is therefore way easier to swizzle cpu_logical_map() on the suspend path and keep the suspend/resume logic unmodified.

Quoted 3 lines

But this patch commits us to requiring that on the suspend path specifically -- I think that ought to be mentioned somewhere. A comment in the preamble for __cpu_suspend would be enough, I think.

What comment would you suggest? I want to make sure the possible confusion you see is properly addressed.

Quoted 2 lines

Looks like the patch should work though, and it does remove the need to read the MPIDR, which is slightly nicer for the non-switcher case.

This patch works fine here, and I'd expect rather catastrophic results if it didn't. ;-)

Quoted 6 lines · Nicolas Pitre

Quoted 3 lines · Dave Martin

But this patch commits us to requiring that on the suspend path specifically -- I think that ought to be mentioned somewhere. A comment in the preamble for __cpu_suspend would be enough, I think.

Quoted 2 lines

What comment would you suggest? I want to make sure the possible confusion you see is properly addressed.

I think we just need to state that the value of cpu_logical_map(smp_processor_id()) must be the MPIDR of the physical CPU the suspending logical CPU will resume on. Consequently, if doing a migration, cpu_logical_map() must be updated appropriately somewhere between cpu_pm_enter() and cpu_suspend().

Quoted 7 lines · Dave Martin

Quoted 6 lines · Nicolas Pitre

Quoted 3 lines · Dave Martin

But this patch commits us to requiring that on the suspend path specifically -- I think that ought to be mentioned somewhere. A comment in the preamble for __cpu_suspend would be enough, I think.

Quoted 8 lines

Quoted 2 lines

What comment would you suggest? I want to make sure the possible confusion you see is properly addressed.

I think we just need to state that the value of cpu_logical_map(smp_processor_id()) must be the MPIDR of the physical CPU the suspending logical CPU will resume on. Consequently, if doing a migration, cpu_logical_map() must be updated appropriately somewhere between cpu_pm_enter() and cpu_suspend().

Excellent. I've amended the patch with this:

arch/arm/kernel/suspend.c +5 -0

index 2835d35234..caf938db62 100644
@@ -17,6 +17,11 @@ extern void cpu_resume_mmu(void);
 /*
  * Hide the first two arguments to __cpu_suspend - these are an implementation
  * detail which platform code shouldn't have to know about.
+ *
+ * On SMP systems, the value of cpu_logical_map(smp_processor_id()) must be
+ * the MPIDR of the physical CPU the suspending logical CPU will resume on.
+ * Consequently, if doing a physical CPU migration, cpu_logical_map() must be
+ * updated appropriately somewhere between cpu_pm_enter() and cpu_suspend().
  */
 int cpu_suspend(unsigned long arg, int (*fn)(unsigned long))
 {

I've put it against cpu_suspend() rather than __cpu_suspend(() as this is what users should care about.

ACK?

Quoted 37 lines · Nicolas Pitre

Quoted 7 lines · Dave Martin

Quoted 6 lines · Nicolas Pitre

Quoted 3 lines · Dave Martin

But this patch commits us to requiring that on the suspend path specifically -- I think that ought to be mentioned somewhere. A comment in the preamble for __cpu_suspend would be enough, I think.

Quoted 8 lines

Quoted 2 lines

What comment would you suggest? I want to make sure the possible confusion you see is properly addressed.

I think we just need to state that the value of cpu_logical_map(smp_processor_id()) must be the MPIDR of the physical CPU the suspending logical CPU will resume on. Consequently, if doing a migration, cpu_logical_map() must be updated appropriately somewhere between cpu_pm_enter() and cpu_suspend().

Excellent. I've amended the patch with this:

arch/arm/kernel/suspend.c +5 -0

index 2835d35234..caf938db62 100644
@@ -17,6 +17,11 @@ extern void cpu_resume_mmu(void);
 /*
  * Hide the first two arguments to __cpu_suspend - these are an implementation
  * detail which platform code shouldn't have to know about.
+ *
+ * On SMP systems, the value of cpu_logical_map(smp_processor_id()) must be
+ * the MPIDR of the physical CPU the suspending logical CPU will resume on.
+ * Consequently, if doing a physical CPU migration, cpu_logical_map() must be
+ * updated appropriately somewhere between cpu_pm_enter() and cpu_suspend().
  */
 int cpu_suspend(unsigned long arg, int (*fn)(unsigned long))
 {

Quoted 4 lines

I've put it against cpu_suspend() rather than __cpu_suspend(() as this is what users should care about.

ACK?

We need this patch to allow IKS to store a cpu context at a specific index, let's be honest. It is a moot point and I am not very happy about changing this code for a very specific usage, but the way code is implemented makes the change acceptable. I really do not think we should write guidelines on how cpu_suspend users have to change cpu_logical_map though, that's not needed apart from IKS and that should be limited to IKS code only.

Again, that's just my opinion, but cpu_suspend API must be kept as it is and we should not encourage people to use it in creative ways.

Quoted 51 lines · Lorenzo Pieralisi

Quoted 37 lines · Nicolas Pitre

Quoted 7 lines · Dave Martin

Quoted 6 lines · Nicolas Pitre

Quoted 3 lines · Dave Martin

But this patch commits us to requiring that on the suspend path specifically -- I think that ought to be mentioned somewhere. A comment in the preamble for __cpu_suspend would be enough, I think.

Quoted 8 lines

Quoted 2 lines

What comment would you suggest? I want to make sure the possible confusion you see is properly addressed.

I think we just need to state that the value of cpu_logical_map(smp_processor_id()) must be the MPIDR of the physical CPU the suspending logical CPU will resume on. Consequently, if doing a migration, cpu_logical_map() must be updated appropriately somewhere between cpu_pm_enter() and cpu_suspend().

Excellent. I've amended the patch with this:

arch/arm/kernel/suspend.c +5 -0

index 2835d35234..caf938db62 100644
@@ -17,6 +17,11 @@ extern void cpu_resume_mmu(void);
 /*
  * Hide the first two arguments to __cpu_suspend - these are an implementation
  * detail which platform code shouldn't have to know about.
+ *
+ * On SMP systems, the value of cpu_logical_map(smp_processor_id()) must be
+ * the MPIDR of the physical CPU the suspending logical CPU will resume on.
+ * Consequently, if doing a physical CPU migration, cpu_logical_map() must be
+ * updated appropriately somewhere between cpu_pm_enter() and cpu_suspend().
  */
 int cpu_suspend(unsigned long arg, int (*fn)(unsigned long))
 {

Quoted 4 lines

I've put it against cpu_suspend() rather than __cpu_suspend(() as this is what users should care about.

ACK?

We need this patch to allow IKS to store a cpu context at a specific index, let's be honest. It is a moot point and I am not very happy about changing this code for a very specific usage, but the way code is implemented makes the change acceptable. I really do not think we should write guidelines on how cpu_suspend users have to change cpu_logical_map though, that's not needed apart from IKS and that should be limited to IKS code only.

Quoted 2 lines

Again, that's just my opinion, but cpu_suspend API must be kept as it is and we should not encourage people to use it in creative ways.

I tend to agree, but I'm now stuck between two conflicting requests.

Are you saying you are willing to give me your ACK if I revert the suggested change?

Nicolas

Quoted 56 lines · Nicolas Pitre

Quoted 53 lines · Lorenzo Pieralisi

Quoted 37 lines · Nicolas Pitre

Quoted 7 lines · Dave Martin

Quoted 6 lines · Nicolas Pitre

Quoted 3 lines · Dave Martin

But this patch commits us to requiring that on the suspend path specifically -- I think that ought to be mentioned somewhere. A comment in the preamble for __cpu_suspend would be enough, I think.

Quoted 8 lines

Quoted 2 lines

What comment would you suggest? I want to make sure the possible confusion you see is properly addressed.

I think we just need to state that the value of cpu_logical_map(smp_processor_id()) must be the MPIDR of the physical CPU the suspending logical CPU will resume on. Consequently, if doing a migration, cpu_logical_map() must be updated appropriately somewhere between cpu_pm_enter() and cpu_suspend().

Excellent. I've amended the patch with this:

arch/arm/kernel/suspend.c +5 -0

index 2835d35234..caf938db62 100644
@@ -17,6 +17,11 @@ extern void cpu_resume_mmu(void);
 /*
  * Hide the first two arguments to __cpu_suspend - these are an implementation
  * detail which platform code shouldn't have to know about.
+ *
+ * On SMP systems, the value of cpu_logical_map(smp_processor_id()) must be
+ * the MPIDR of the physical CPU the suspending logical CPU will resume on.
+ * Consequently, if doing a physical CPU migration, cpu_logical_map() must be
+ * updated appropriately somewhere between cpu_pm_enter() and cpu_suspend().
  */
 int cpu_suspend(unsigned long arg, int (*fn)(unsigned long))
 {

Quoted 4 lines

I've put it against cpu_suspend() rather than __cpu_suspend(() as this is what users should care about.

ACK?

We need this patch to allow IKS to store a cpu context at a specific index, let's be honest. It is a moot point and I am not very happy about changing this code for a very specific usage, but the way code is implemented makes the change acceptable. I really do not think we should write guidelines on how cpu_suspend users have to change cpu_logical_map though, that's not needed apart from IKS and that should be limited to IKS code only.

Again, that's just my opinion, but cpu_suspend API must be kept as it is and we should not encourage people to use it in creative ways.

Quoted 1 line

I tend to agree, but I'm now stuck between two conflicting requests.

Would it make sense to keep the same API to cpu_suspend(), but make it a wrapper for another function which has the MPIDR argument? Then people calling cpu_suspend() continue as normal. Only IKS needs to know about the underlying MPIDR handling when calling this.

Quoted 57 lines · Dave Martin

Quoted 56 lines · Nicolas Pitre

Quoted 53 lines · Lorenzo Pieralisi

Quoted 37 lines · Nicolas Pitre

Quoted 7 lines · Dave Martin

Quoted 6 lines · Nicolas Pitre

Quoted 3 lines · Dave Martin

But this patch commits us to requiring that on the suspend path specifically -- I think that ought to be mentioned somewhere. A comment in the preamble for __cpu_suspend would be enough, I think.

Quoted 8 lines

Quoted 2 lines

What comment would you suggest? I want to make sure the possible confusion you see is properly addressed.

I think we just need to state that the value of cpu_logical_map(smp_processor_id()) must be the MPIDR of the physical CPU the suspending logical CPU will resume on. Consequently, if doing a migration, cpu_logical_map() must be updated appropriately somewhere between cpu_pm_enter() and cpu_suspend().

Excellent. I've amended the patch with this:

arch/arm/kernel/suspend.c +5 -0

index 2835d35234..caf938db62 100644
@@ -17,6 +17,11 @@ extern void cpu_resume_mmu(void);
 /*
  * Hide the first two arguments to __cpu_suspend - these are an implementation
  * detail which platform code shouldn't have to know about.
+ *
+ * On SMP systems, the value of cpu_logical_map(smp_processor_id()) must be
+ * the MPIDR of the physical CPU the suspending logical CPU will resume on.
+ * Consequently, if doing a physical CPU migration, cpu_logical_map() must be
+ * updated appropriately somewhere between cpu_pm_enter() and cpu_suspend().
  */
 int cpu_suspend(unsigned long arg, int (*fn)(unsigned long))
 {

Quoted 4 lines

I've put it against cpu_suspend() rather than __cpu_suspend(() as this is what users should care about.

ACK?

We need this patch to allow IKS to store a cpu context at a specific index, let's be honest. It is a moot point and I am not very happy about changing this code for a very specific usage, but the way code is implemented makes the change acceptable. I really do not think we should write guidelines on how cpu_suspend users have to change cpu_logical_map though, that's not needed apart from IKS and that should be limited to IKS code only.

Again, that's just my opinion, but cpu_suspend API must be kept as it is and we should not encourage people to use it in creative ways.

Quoted 6 lines

Quoted 1 line

I tend to agree, but I'm now stuck between two conflicting requests.

Would it make sense to keep the same API to cpu_suspend(), but make it a wrapper for another function which has the MPIDR argument? Then people calling cpu_suspend() continue as normal. Only IKS needs to know about the underlying MPIDR handling when calling this.

The fact is that the switcher _does_ need to swizzle the cpu_logical_map anyway before suspending. Hence really there is no point creating extra wrappers for this.

So Lorenzo's suggestion to simply not advertise this too much for people to not get too creative is probably the best compromize IMHO.

Quoted 57 lines · Dave Martin

Quoted 56 lines · Nicolas Pitre

Quoted 53 lines · Lorenzo Pieralisi

Quoted 37 lines · Nicolas Pitre

Quoted 7 lines · Dave Martin

Quoted 6 lines · Nicolas Pitre

Quoted 3 lines · Dave Martin

But this patch commits us to requiring that on the suspend path specifically -- I think that ought to be mentioned somewhere. A comment in the preamble for __cpu_suspend would be enough, I think.

Quoted 8 lines

Quoted 2 lines

What comment would you suggest? I want to make sure the possible confusion you see is properly addressed.

I think we just need to state that the value of cpu_logical_map(smp_processor_id()) must be the MPIDR of the physical CPU the suspending logical CPU will resume on. Consequently, if doing a migration, cpu_logical_map() must be updated appropriately somewhere between cpu_pm_enter() and cpu_suspend().

Excellent. I've amended the patch with this:

arch/arm/kernel/suspend.c +5 -0

index 2835d35234..caf938db62 100644
@@ -17,6 +17,11 @@ extern void cpu_resume_mmu(void);
 /*
  * Hide the first two arguments to __cpu_suspend - these are an implementation
  * detail which platform code shouldn't have to know about.
+ *
+ * On SMP systems, the value of cpu_logical_map(smp_processor_id()) must be
+ * the MPIDR of the physical CPU the suspending logical CPU will resume on.
+ * Consequently, if doing a physical CPU migration, cpu_logical_map() must be
+ * updated appropriately somewhere between cpu_pm_enter() and cpu_suspend().
  */
 int cpu_suspend(unsigned long arg, int (*fn)(unsigned long))
 {

Quoted 4 lines

I've put it against cpu_suspend() rather than __cpu_suspend(() as this is what users should care about.

ACK?

We need this patch to allow IKS to store a cpu context at a specific index, let's be honest. It is a moot point and I am not very happy about changing this code for a very specific usage, but the way code is implemented makes the change acceptable. I really do not think we should write guidelines on how cpu_suspend users have to change cpu_logical_map though, that's not needed apart from IKS and that should be limited to IKS code only.

Again, that's just my opinion, but cpu_suspend API must be kept as it is and we should not encourage people to use it in creative ways.

Quoted 6 lines

Quoted 1 line

I tend to agree, but I'm now stuck between two conflicting requests.

Would it make sense to keep the same API to cpu_suspend(), but make it a wrapper for another function which has the MPIDR argument? Then people calling cpu_suspend() continue as normal. Only IKS needs to know about the underlying MPIDR handling when calling this.

The fact is that the switcher _does_ need to swizzle the cpu_logical_map anyway before suspending. Hence really there is no point creating extra wrappers for this.

So Lorenzo's suggestion to simply not advertise this too much for people to not get too creative is probably the best compromize IMHO.

Quoted 66 lines · Nicolas Pitre

Quoted 57 lines · Dave Martin

Quoted 56 lines · Nicolas Pitre

Quoted 53 lines · Lorenzo Pieralisi

Quoted 37 lines · Nicolas Pitre

Quoted 7 lines · Dave Martin

Quoted 6 lines · Nicolas Pitre

Quoted 3 lines · Dave Martin

But this patch commits us to requiring that on the suspend path specifically -- I think that ought to be mentioned somewhere. A comment in the preamble for __cpu_suspend would be enough, I think.

Quoted 8 lines

Quoted 2 lines

What comment would you suggest? I want to make sure the possible confusion you see is properly addressed.

I think we just need to state that the value of cpu_logical_map(smp_processor_id()) must be the MPIDR of the physical CPU the suspending logical CPU will resume on. Consequently, if doing a migration, cpu_logical_map() must be updated appropriately somewhere between cpu_pm_enter() and cpu_suspend().

Excellent. I've amended the patch with this:

arch/arm/kernel/suspend.c +5 -0

index 2835d35234..caf938db62 100644
@@ -17,6 +17,11 @@ extern void cpu_resume_mmu(void);
 /*
  * Hide the first two arguments to __cpu_suspend - these are an implementation
  * detail which platform code shouldn't have to know about.
+ *
+ * On SMP systems, the value of cpu_logical_map(smp_processor_id()) must be
+ * the MPIDR of the physical CPU the suspending logical CPU will resume on.
+ * Consequently, if doing a physical CPU migration, cpu_logical_map() must be
+ * updated appropriately somewhere between cpu_pm_enter() and cpu_suspend().
  */
 int cpu_suspend(unsigned long arg, int (*fn)(unsigned long))
 {

Quoted 4 lines

I've put it against cpu_suspend() rather than __cpu_suspend(() as this is what users should care about.

ACK?

We need this patch to allow IKS to store a cpu context at a specific index, let's be honest. It is a moot point and I am not very happy about changing this code for a very specific usage, but the way code is implemented makes the change acceptable. I really do not think we should write guidelines on how cpu_suspend users have to change cpu_logical_map though, that's not needed apart from IKS and that should be limited to IKS code only.

Again, that's just my opinion, but cpu_suspend API must be kept as it is and we should not encourage people to use it in creative ways.

Quoted 6 lines

Quoted 1 line

I tend to agree, but I'm now stuck between two conflicting requests.

Would it make sense to keep the same API to cpu_suspend(), but make it a wrapper for another function which has the MPIDR argument? Then people calling cpu_suspend() continue as normal. Only IKS needs to know about the underlying MPIDR handling when calling this.

Quoted 6 lines

The fact is that the switcher _does_ need to swizzle the cpu_logical_map anyway before suspending. Hence really there is no point creating extra wrappers for this.

So Lorenzo's suggestion to simply not advertise this too much for people to not get too creative is probably the best compromize IMHO.

We could add a function cpu_migrate, almost identical to cpu_suspend but accepting an MPIDR parameter as Dave said. cpu_suspend would read its own MPIDR using read_cpuid_mpidr() and pass it to __cpu_suspend, keeping the current behaviour unchanged (well, MPIDR is now read in cpu_suspend instead of __cpu_suspend).
cpu_migrate will pass the mpidr to __cpu_suspend, to say "migrate this CPU to the CPU identified by this MPIDR"; IKS can swizzle cpu_logical_map internally.

Looks like lot of churn though, probably the intent is clearer and closer to PSCI. Just an idea, probably useless.

I will apply and test the patch as it is first thing next week.

Currently we hash the MPIDR of the CPU being suspended to determine which entry in the sleep_save_sp array to use. In some situations, such as when we want to resume on another physical CPU, the MPIDR of another CPU should be used instead.

So let's use the value of cpu_logical_map(smp_processor_id()) in place of the MPIDR in the suspend path. This will result in the same index being used as with the previous code unless the caller has modified cpu_logical_map() beforehand.

The register allocation in __cpu_suspend is reworked in order to better accommodate the additional argument.

Signed-off-by Nicolas Pitre <[email protected]>

---
 arch/arm/kernel/sleep.S   | 25 +++++++++++--------------
 arch/arm/kernel/suspend.c |  8 +++++---
 2 files changed, 16 insertions(+), 17 deletions(-)

arch/arm/kernel/sleep.S +0 -0

index db1536b8b3..836d10e698 100644

arch/arm/kernel/sleep.S +1 -6

  */
 ENTRY(__cpu_suspend)
 	stmfd	sp!, {r4 - r11, lr}

@@ -67,23 +68,19 @@ ENTRY(__cpu_suspend)
 	mov	r5, sp			@ current virtual SP
 	add	r4, r4, #12		@ Space for pgd, virt sp, phys resume fn
 	sub	sp, sp, r4		@ allocate CPU state on stack
-	stmfd	sp!, {r0, r1}		@ save suspend func arg and pointer
-	add	r0, sp, #8		@ save pointer to save block
-	mov	r1, r4			@ size of save block
-	mov	r2, r5			@ virtual SP
 	ldr	r3, =sleep_save_sp
+	stmfd	sp!, {r0, r1}		@ save suspend func arg and pointer
 	ldr	r3, [r3, #SLEEP_SAVE_SP_VIRT]
-	ALT_SMP(mrc p15, 0, r9, c0, c0, 5)
-        ALT_UP_B(1f)

arch/arm/kernel/sleep.S +0 -0

 	bl	__cpu_suspend_save
 	adr	lr, BSYM(cpu_suspend_abort)
 	ldmfd	sp!, {r0, pc}		@ call suspend fn

arch/arm/kernel/suspend.c +3 -2

index 41cf3cbf75..2835d35234 100644
@@ -10,7 +10,7 @@
 #include <asm/suspend.h>
 #include <asm/tlbflush.h>
 
-extern int __cpu_suspend(unsigned long, int (*)(unsigned long));
+extern int __cpu_suspend(unsigned long, int (*)(unsigned long), u32 cpuid);
 extern void cpu_resume_mmu(void);
 
 #ifdef CONFIG_MMU

@@ -21,6 +21,7 @@ extern void cpu_resume_mmu(void);
 int cpu_suspend(unsigned long arg, int (*fn)(unsigned long))
 {
 	struct mm_struct *mm = current->active_mm;
+	u32 __mpidr = cpu_logical_map(smp_processor_id());
 	int ret;
 
 	if (!idmap_pgd)

@@ -32,7 +33,7 @@ int cpu_suspend(unsigned long arg, int (*fn)(unsigned long))
 	 * resume (indicated by a zero return code), we need to switch
 	 * back to the correct page tables.
 	 */
-	ret = __cpu_suspend(arg, fn);
+	ret = __cpu_suspend(arg, fn, __mpidr);
 	if (ret == 0) {
 		cpu_switch_mm(mm->pgd, mm);
 		local_flush_bp_all();

Quoted 5 lines · Nicolas Pitre

Currently we hash the MPIDR of the CPU being suspended to determine which entry in the sleep_save_sp array to use. In some situations, such as when we want to resume on another physical CPU, the MPIDR of another CPU should be used instead.

Quoted 4 lines

So let's use the value of cpu_logical_map(smp_processor_id()) in place of the MPIDR in the suspend path. This will result in the same index being used as with the previous code unless the caller has modified cpu_logical_map() beforehand.

I will rely on your wisdom to rewrite the commit log in a way that does not hint at dangerous creativity, if you catch my drift :D

Quoted 9 lines

The register allocation in __cpu_suspend is reworked in order to better accommodate the additional argument.

Signed-off-by Nicolas Pitre <[email protected]>

---
 arch/arm/kernel/sleep.S   | 25 +++++++++++--------------
 arch/arm/kernel/suspend.c |  8 +++++---
 2 files changed, 16 insertions(+), 17 deletions(-)

arch/arm/kernel/sleep.S line 58

Quoted 9 lines

arch/arm/kernel/sleep.S +1 -0

index db1536b8b3..836d10e698 100644
@@ -55,6 +55,7 @@
  * specific registers and some other data for resume.
  *  r0 = suspend function arg0
  *  r1 = suspend function
+ *  r2 = MPIDR value used to index into sleep_save_sp

CPU's MPIDR or something like that, let's not hint at possible creative usage.

Quoted 20 lines

arch/arm/kernel/sleep.S +1 -10

  */
 ENTRY(__cpu_suspend)
 	stmfd	sp!, {r4 - r11, lr}

@@ -67,23 +68,19 @@ ENTRY(__cpu_suspend)
 	mov	r5, sp			@ current virtual SP
 	add	r4, r4, #12		@ Space for pgd, virt sp, phys resume fn
 	sub	sp, sp, r4		@ allocate CPU state on stack
-	stmfd	sp!, {r0, r1}		@ save suspend func arg and pointer
-	add	r0, sp, #8		@ save pointer to save block
-	mov	r1, r4			@ size of save block
-	mov	r2, r5			@ virtual SP
 	ldr	r3, =sleep_save_sp
+	stmfd	sp!, {r0, r1}		@ save suspend func arg and pointer
 	ldr	r3, [r3, #SLEEP_SAVE_SP_VIRT]
-	ALT_SMP(mrc p15, 0, r9, c0, c0, 5)
-        ALT_UP_B(1f)
-	ldr	r8, =mpidr_hash
-	/*
-	 * This ldmia relies on the memory layout of the mpidr_hash
-	 * struct mpidr_hash.

arch/arm/kernel/sleep.S ENTRY(__cpu_suspend) · line 75

Quoted 6 lines

arch/arm/kernel/sleep.S +2 -4

-	 */
-	ldmia	r8, {r4-r7}	@ r4 = mpidr mask (r5,r6,r7) = l[0,1,2] shifts
-	compute_mpidr_hash	lr, r5, r6, r7, r9, r4
-	add	r3, r3, lr, lsl #2
+	ALT_SMP(ldr r0, =mpidr_hash)
+       ALT_UP_B(1f)

Should be a tab, do not know if that's an email server issue or not.

arch/arm/kernel/sleep.S ENTRY(__cpu_suspend) · line 83

Quoted 8 lines

arch/arm/kernel/sleep.S +7 -0

+	/* This ldmia relies on the memory layout of the mpidr_hash struct */
+	ldmia	r0, {r1, r6-r8}	@ r1 = mpidr mask (r6,r7,r8) = l[0,1,2] shifts
+	compute_mpidr_hash	r0, r6, r7, r8, r2, r1
+	add	r3, r3, r0, lsl #2
 1:
+	mov	r2, r5			@ virtual SP
+	mov	r1, r4			@ size of save block
+	add	r0, sp, #8		@ pointer to save block

It is already a bit complex to follow, code is ok, but line above was better placed closer to sp last change, not after hashing the MPIDR.

Quoted 34 lines

arch/arm/kernel/sleep.S +0 -0

 	bl	__cpu_suspend_save
 	adr	lr, BSYM(cpu_suspend_abort)
 	ldmfd	sp!, {r0, pc}		@ call suspend fn

arch/arm/kernel/suspend.c +3 -2

index 41cf3cbf75..2835d35234 100644
@@ -10,7 +10,7 @@
 #include <asm/suspend.h>
 #include <asm/tlbflush.h>
 
-extern int __cpu_suspend(unsigned long, int (*)(unsigned long));
+extern int __cpu_suspend(unsigned long, int (*)(unsigned long), u32 cpuid);
 extern void cpu_resume_mmu(void);
 
 #ifdef CONFIG_MMU

@@ -21,6 +21,7 @@ extern void cpu_resume_mmu(void);
 int cpu_suspend(unsigned long arg, int (*fn)(unsigned long))
 {
 	struct mm_struct *mm = current->active_mm;
+	u32 __mpidr = cpu_logical_map(smp_processor_id());
 	int ret;
 
 	if (!idmap_pgd)

@@ -32,7 +33,7 @@ int cpu_suspend(unsigned long arg, int (*fn)(unsigned long))
 	 * resume (indicated by a zero return code), we need to switch
 	 * back to the correct page tables.
 	 */
-	ret = __cpu_suspend(arg, fn);
+	ret = __cpu_suspend(arg, fn, __mpidr);
 	if (ret == 0) {
 		cpu_switch_mm(mm->pgd, mm);
 		local_flush_bp_all();

arch/arm/kernel/suspend.c int cpu_suspend(unsigned long arg, int (*fn)(unsigned long)) · line 52

Quoted 9 lines

arch/arm/kernel/suspend.c +2 -1

@@ -44,7 +45,8 @@ int cpu_suspend(unsigned long arg, int (*fn)(unsigned long))
 #else
 int cpu_suspend(unsigned long arg, int (*fn)(unsigned long))
 {
-	return __cpu_suspend(arg, fn);
+	u32 __mpidr = cpu_logical_map(smp_processor_id());
+	return __cpu_suspend(arg, fn, __mpidr);
 }
 #define	idmap_pgd	NULL
 #endif

Apart from these nits, let's hope it is the last cpu_suspend bit we need to change, please land it on -next asap, of course if Russell is ok with that.

I tested it on TC2.

FWIW:

Acked-by Lorenzo Pieralisi <[email protected]>

Quoted 6 lines · Lorenzo Pieralisi

Quoted 5 lines · Nicolas Pitre

Currently we hash the MPIDR of the CPU being suspended to determine which entry in the sleep_save_sp array to use. In some situations, such as when we want to resume on another physical CPU, the MPIDR of another CPU should be used instead.

Quoted 7 lines

Quoted 4 lines

So let's use the value of cpu_logical_map(smp_processor_id()) in place of the MPIDR in the suspend path. This will result in the same index being used as with the previous code unless the caller has modified cpu_logical_map() beforehand.

I will rely on your wisdom to rewrite the commit log in a way that does not hint at dangerous creativity, if you catch my drift :D

I've augmented it with some of the earlier comments from Dave Martin. I think it is fine to be clear in the commit log about what we intend this change to be used for.

Quoted 9 lines

Quoted 9 lines

The register allocation in __cpu_suspend is reworked in order to better accommodate the additional argument.

Signed-off-by Nicolas Pitre <[email protected]>

---
 arch/arm/kernel/sleep.S   | 25 +++++++++++--------------
 arch/arm/kernel/suspend.c |  8 +++++---
 2 files changed, 16 insertions(+), 17 deletions(-)

Quoted 11 lines

arch/arm/kernel/sleep.S line 58

Quoted 9 lines

arch/arm/kernel/sleep.S +1 -0

index db1536b8b3..836d10e698 100644
@@ -55,6 +55,7 @@
  * specific registers and some other data for resume.
  *  r0 = suspend function arg0
  *  r1 = suspend function
+ *  r2 = MPIDR value used to index into sleep_save_sp

CPU's MPIDR or something like that, let's not hint at possible creative usage.

What about "MPIDR of the CPU to be resumed" which should normally just be the current CPU's?

After all, creativity is not always a bad thing given it is reviewed properly.

Quoted 21 lines

Quoted 21 lines

arch/arm/kernel/sleep.S +1 -10

  */
 ENTRY(__cpu_suspend)
 	stmfd	sp!, {r4 - r11, lr}

@@ -67,23 +68,19 @@ ENTRY(__cpu_suspend)
 	mov	r5, sp			@ current virtual SP
 	add	r4, r4, #12		@ Space for pgd, virt sp, phys resume fn
 	sub	sp, sp, r4		@ allocate CPU state on stack
-	stmfd	sp!, {r0, r1}		@ save suspend func arg and pointer
-	add	r0, sp, #8		@ save pointer to save block
-	mov	r1, r4			@ size of save block
-	mov	r2, r5			@ virtual SP
 	ldr	r3, =sleep_save_sp
+	stmfd	sp!, {r0, r1}		@ save suspend func arg and pointer
 	ldr	r3, [r3, #SLEEP_SAVE_SP_VIRT]
-	ALT_SMP(mrc p15, 0, r9, c0, c0, 5)
-        ALT_UP_B(1f)
-	ldr	r8, =mpidr_hash
-	/*
-	 * This ldmia relies on the memory layout of the mpidr_hash
-	 * struct mpidr_hash.

Quoted 8 lines

arch/arm/kernel/sleep.S ENTRY(__cpu_suspend) · line 75

Quoted 6 lines

arch/arm/kernel/sleep.S +2 -4

-	 */
-	ldmia	r8, {r4-r7}	@ r4 = mpidr mask (r5,r6,r7) = l[0,1,2] shifts
-	compute_mpidr_hash	lr, r5, r6, r7, r9, r4
-	add	r3, r3, lr, lsl #2
+	ALT_SMP(ldr r0, =mpidr_hash)
+       ALT_UP_B(1f)

Should be a tab, do not know if that's an email server issue or not.

Amistake on my part.

Quoted 11 lines

arch/arm/kernel/sleep.S ENTRY(__cpu_suspend) · line 83

Quoted 8 lines

arch/arm/kernel/sleep.S +7 -0

+	/* This ldmia relies on the memory layout of the mpidr_hash struct */
+	ldmia	r0, {r1, r6-r8}	@ r1 = mpidr mask (r6,r7,r8) = l[0,1,2] shifts
+	compute_mpidr_hash	r0, r6, r7, r8, r2, r1
+	add	r3, r3, r0, lsl #2
 1:
+	mov	r2, r5			@ virtual SP
+	mov	r1, r4			@ size of save block
+	add	r0, sp, #8		@ pointer to save block

It is already a bit complex to follow, code is ok, but line above was better placed closer to sp last change, not after hashing the MPIDR.

Well, I tend to disagree. I think it is much clearer to set function arguments closer to the call site so that r0 doesn't have to be live with the stack location throughout this code.

Quoted 48 lines

Quoted 35 lines

arch/arm/kernel/sleep.S +0 -0

 	bl	__cpu_suspend_save
 	adr	lr, BSYM(cpu_suspend_abort)
 	ldmfd	sp!, {r0, pc}		@ call suspend fn

arch/arm/kernel/suspend.c +3 -2

index 41cf3cbf75..2835d35234 100644
@@ -10,7 +10,7 @@
 #include <asm/suspend.h>
 #include <asm/tlbflush.h>
 
-extern int __cpu_suspend(unsigned long, int (*)(unsigned long));
+extern int __cpu_suspend(unsigned long, int (*)(unsigned long), u32 cpuid);
 extern void cpu_resume_mmu(void);
 
 #ifdef CONFIG_MMU

@@ -21,6 +21,7 @@ extern void cpu_resume_mmu(void);
 int cpu_suspend(unsigned long arg, int (*fn)(unsigned long))
 {
 	struct mm_struct *mm = current->active_mm;
+	u32 __mpidr = cpu_logical_map(smp_processor_id());
 	int ret;
 
 	if (!idmap_pgd)

@@ -32,7 +33,7 @@ int cpu_suspend(unsigned long arg, int (*fn)(unsigned long))
 	 * resume (indicated by a zero return code), we need to switch
 	 * back to the correct page tables.
 	 */
-	ret = __cpu_suspend(arg, fn);
+	ret = __cpu_suspend(arg, fn, __mpidr);
 	if (ret == 0) {
 		cpu_switch_mm(mm->pgd, mm);
 		local_flush_bp_all();

arch/arm/kernel/suspend.c int cpu_suspend(unsigned long arg, int (*fn)(unsigned long)) · line 52

Quoted 9 lines

arch/arm/kernel/suspend.c +2 -1

@@ -44,7 +45,8 @@ int cpu_suspend(unsigned long arg, int (*fn)(unsigned long))
 #else
 int cpu_suspend(unsigned long arg, int (*fn)(unsigned long))
 {
-	return __cpu_suspend(arg, fn);
+	u32 __mpidr = cpu_logical_map(smp_processor_id());
+	return __cpu_suspend(arg, fn, __mpidr);
 }
 #define	idmap_pgd	NULL
 #endif

Apart from these nits, let's hope it is the last cpu_suspend bit we need to change, please land it on -next asap, of course if Russell is ok with that.

Quoted 5 lines

I tested it on TC2.

FWIW:

Acked-by Lorenzo Pieralisi <[email protected]>

Thanks.

Nicolas

This is required by the big.LITTLE switcher code.

The gic_migrate_target() changes the CPU interface mapping for the current CPU to redirect SGIs to the specified interface, and it also updates the target CPU for each interrupts to that CPU interface if they were targeting the current interface. Finally, pending SGIs for the current CPU are forwarded to the new interface.

Because Linux does not use it, the SGI source information for the forwarded SGIs is not preserved. Neither is the source information for the SGIs sent by the current CPU to other CPUs adjusted to match the new CPU interface mapping. The required registers are banked so only the target CPU could do it.

Signed-off-by Nicolas Pitre <[email protected]>

---
  drivers/irqchip/irq-gic.c       | 81 +++++++++++++++++++++++++++++++++++++++--
  include/linux/irqchip/arm-gic.h |  4 ++
  2 files changed, 82 insertions(+), 3 deletions(-)

drivers/irqchip/irq-gic.c +4 -3

index ee7c503120..6bd5a8c1aa 100644
@@ -253,10 +253,9 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
  	if (cpu >= NR_GIC_CPU_IF || cpu >= nr_cpu_ids)
  		return -EINVAL;
 
+	raw_spin_lock(&irq_controller_lock);
  	mask = 0xff << shift;
  	bit = gic_cpu_map[cpu] << shift;
-
-	raw_spin_lock(&irq_controller_lock);
  	val = readl_relaxed(reg) & ~mask;
  	writel_relaxed(val | bit, reg);
  	raw_spin_unlock(&irq_controller_lock);

@@ -646,7 +645,9 @@ static void __init gic_pm_init(struct gic_chip_data *gic)
  void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
  {
  	int cpu;
-	unsigned long map = 0;
+	unsigned long flags, map = 0;
+
+	raw_spin_lock_irqsave(&irq_controller_lock, flags);
 
  	/* Convert our logical CPU mask into a physical one. */
  	for_each_cpu(cpu, mask)

@@ -660,6 +661,80 @@ void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
 

drivers/irqchip/irq-gic.c +15 -0

+ *
+ * @new_cpu_id: the CPU target ID to migrate IRQs to
+ *
+ * Migrate all peripheral interrupts with a target matching the current CPU
+ * to the interface corresponding to @new_cpu_id.  The CPU interface mapping
+ * is also updated.  Targets to other CPU interfaces are unchanged.
+ * This must be called with IRQs locally disabled.
+ */
+void gic_migrate_target(unsigned int new_cpu_id)
+{
+	unsigned int old_cpu_id, gic_irqs, gic_nr = 0;
+	void __iomem *dist_base;
+	int i, ror_val, cpu = smp_processor_id();
+	u32 val, old_mask, active_mask;
+

drivers/irqchip/irq-gic.c +24 -0

+		val = readl_relaxed(dist_base + GIC_DIST_TARGET + i * 4);
+		active_mask = val & old_mask;
+		if (active_mask) {
+			val &= ~active_mask;
+			val |= ror32(active_mask, ror_val);
+			writel_relaxed(val, dist_base + GIC_DIST_TARGET + i*4);
+		}
+	}
+
+	raw_spin_unlock(&irq_controller_lock);
+
+	/*
+	 * Now let's migrate and clear any potential SGIs that might be
+	 * pending for us (old_cpu_id).  Since GIC_DIST_SGI_PENDING_SET
+	 * is a banked register, we can only forward the SGI using
+	 * GIC_DIST_SOFTINT.  The original SGI source is lost but Linux
+	 * doesn't use that information anyway.
+	 *
+	 * For the same reason we do not adjust SGI source information
+	 * for previously sent SGIs by us to other CPUs either.
+	 */
+	for (i = 0; i < 16; i += 4) {
+		int j;
+		val = readl_relaxed(dist_base + GIC_DIST_SGI_PENDING_SET + i);

drivers/irqchip/irq-gic.c +0 -0

  }
  #endif
 

include/linux/irqchip/arm-gic.h +4 -0

index 3e203eb23c..40bfcac959 100644
@@ -31,6 +31,8 @@
  #define GIC_DIST_TARGET			0x800
  #define GIC_DIST_CONFIG			0xc00
  #define GIC_DIST_SOFTINT		0xf00
+#define GIC_DIST_SGI_PENDING_CLEAR	0xf10
+#define GIC_DIST_SGI_PENDING_SET	0xf20
 
  #define GICH_HCR			0x0
  #define GICH_VTR			0x4

@@ -73,6 +75,8 @@ static inline void gic_init(unsigned int nr, int start,
  	gic_init_bases(nr, start, dist, cpu, 0, NULL);
  }
 
+void gic_migrate_target(unsigned int new_cpu_id);
+
  #endif /* __ASSEMBLY */
 
  #endif

Hi Nico,

I've got a couple of minor questions/comments about this one...

Quoted 53 lines · Nicolas Pitre

This is required by the big.LITTLE switcher code.

The gic_migrate_target() changes the CPU interface mapping for the current CPU to redirect SGIs to the specified interface, and it also updates the target CPU for each interrupts to that CPU interface if they were targeting the current interface. Finally, pending SGIs for the current CPU are forwarded to the new interface.

Because Linux does not use it, the SGI source information for the forwarded SGIs is not preserved. Neither is the source information for the SGIs sent by the current CPU to other CPUs adjusted to match the new CPU interface mapping. The required registers are banked so only the target CPU could do it.

Signed-off-by Nicolas Pitre <[email protected]>

---
  drivers/irqchip/irq-gic.c       | 81
+++++++++++++++++++++++++++++++++++++++--
  include/linux/irqchip/arm-gic.h |  4 ++
  2 files changed, 82 insertions(+), 3 deletions(-)

drivers/irqchip/irq-gic.c +1 -2

index ee7c503120..6bd5a8c1aa 100644
@@ -253,10 +253,9 @@ static int gic_set_affinity(struct irq_data *d, const
 struct cpumask *mask_val,
  	if (cpu >= NR_GIC_CPU_IF || cpu >= nr_cpu_ids)
  		return -EINVAL;
 
+	raw_spin_lock(&irq_controller_lock);
  	mask = 0xff << shift;
  	bit = gic_cpu_map[cpu] << shift;
-
-	raw_spin_lock(&irq_controller_lock);
  	val = readl_relaxed(reg) & ~mask;
  	writel_relaxed(val | bit, reg);

  	raw_spin_unlock(&irq_controller_lock);

drivers/irqchip/irq-gic.c +3 -1

@@ -646,7 +645,9 @@ static void __init gic_pm_init(struct gic_chip_data
 *gic)
  void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
  {
  	int cpu;
-	unsigned long map = 0;
+	unsigned long flags, map = 0;
+
+	raw_spin_lock_irqsave(&irq_controller_lock, flags);
 
  	/* Convert our logical CPU mask into a physical one. */

for_each_cpu(cpu, mask)

drivers/irqchip/irq-gic.c +0 -0

@@ -660,6 +661,80 @@ void gic_raise_softirq(const struct cpumask *mask,
 unsigned int irq)
 

drivers/irqchip/irq-gic.c void gic_raise_softirq(const struct cpumask *mask, · line 673

Quoted 11 lines

drivers/irqchip/irq-gic.c +8 -0

  	/* this always happens on GIC0 */
  	writel_relaxed(map << 16 | irq, gic_data_dist_base(&gic_data[0]) +
 GIC_DIST_SOFTINT);
+
+	raw_spin_unlock_irqrestore(&irq_controller_lock, flags);
+}
+#endif
+
+#ifdef CONFIG_BL_SWITCHER
+/*
+ * gic_migrate_target - migrate IRQs to another PU interface

(nit) Should that be _C_PU?

Quoted 16 lines

drivers/irqchip/irq-gic.c +5 -0

+ *
+ * @new_cpu_id: the CPU target ID to migrate IRQs to
+ *
+ * Migrate all peripheral interrupts with a target matching the current CPU
+ * to the interface corresponding to @new_cpu_id.  The CPU interface

mapping
+ * is also updated. Targets to other CPU interfaces are unchanged.
+ * This must be called with IRQs locally disabled.
+ */

drivers/irqchip/irq-gic.c +7 -0

+void gic_migrate_target(unsigned int new_cpu_id)
+{
+	unsigned int old_cpu_id, gic_irqs, gic_nr = 0;
+	void __iomem *dist_base;
+	int i, ror_val, cpu = smp_processor_id();
+	u32 val, old_mask, active_mask;
+

drivers/irqchip/irq-gic.c

Quoted 10 lines

drivers/irqchip/irq-gic.c +10 -0

+	if (gic_nr >= MAX_GIC_NR)
+		BUG();
+
+	dist_base = gic_data_dist_base(&gic_data[gic_nr]);
+	if (!dist_base)
+		return;
+	gic_irqs = gic_data[gic_nr].gic_irqs;
+
+	old_cpu_id = __ffs(gic_cpu_map[cpu]);
+	old_mask = 0x01010101 << old_cpu_id;

I don't think this is very clear, though section 4.3.12 of the GIC spec helps a lot! A little pointer or some more self-evident name for the mask might be nice...

old_mask = GIC_ITARGETSR_MASK << old_cpu_id

or similar? This at least points one to the right bit of documentation?

drivers/irqchip/irq-gic.c

Quoted 7 lines

drivers/irqchip/irq-gic.c +7 -0

+	ror_val = (old_cpu_id - new_cpu_id) & 31;
+
+	raw_spin_lock(&irq_controller_lock);
+
+	gic_cpu_map[cpu] = 1 << new_cpu_id;
+
+	for (i = 8; i < DIV_ROUND_UP(gic_irqs, 4); i++) {

Does this '8' here warrant a #define? GIC_RO_INTR_REGS?

Perhaps everyone looking at the code will be familiar enough with the GIC to see immediately why the first 8 entries are being skipped...?

I'm curious as to why we don't need to do anything for PPIs here - could there be any pending PPIs that don't get handled (I guess retargetting doesn't make sense for these?)

This is the core code implementing big.LITTLE switcher functionality. Rational for this code is available here:

http://lwn.net/Articles/481055/

The main entry point for a switch request is:

void bL_switch_request(unsigned int cpu, unsigned int new_cluster_id)

If the calling CPU is not the wanted one, this wrapper takes care of sending the request to the appropriate CPU with schedule_work_on().

At the moment the core switch operation is handled by bL_switch_to() which must be called on the CPU for which a switch is requested.

What this code does:

* Return early if the current cluster is the wanted one.

* Close the gate in the kernel entry vector for both the inbound and outbound CPUs.

* Wake up the inbound CPU so it can perform its reset sequence in parallel up to the kernel entry vector gate.

* Migrate all interrupts in the GIC targeting the outbound CPU interface to the inbound CPU interface, including SGIs. This is performed by gic_migrate_target() in drivers/irqchip/irq-gic.c.

* Call cpu_pm_enter() which takes care of flushing the VFP state to RAM and save the CPU interface config from the GIC to RAM.

* Modify the cpu_logical_map to refer to the inbound physical CPU.

* Call cpu_suspend() which saves the CPU state (general purpose registers, page table address) onto the stack and store the resulting stack pointer in an array indexed by the updated cpu_logical_map, then call the provided shutdown function. This happens in arch/arm/kernel/sleep.S.

At this point, the provided shutdown function executed by the outbound CPU ungates the inbound CPU. Therefore the inbound CPU:

* Picks up the saved stack pointer in the array indexed by its MPIDR in arch/arm/kernel/sleep.S.

* The MMU and caches are re-enabled using the saved state on the provided stack, just like if this was a resume operation from a suspended state.

* Then cpu_suspend() returns, although this is on the inbound CPU rather than the outbound CPU which called it initially.

* The function cpu_pm_exit() is called which effect is to restore the CPU interface state in the GIC using the state previously saved by the outbound CPU.

* Exit of bL_switch_to() to resume normal kernel execution on the new CPU.

However, the outbound CPU is potentially still running in parallel while the inbound CPU is resuming normal kernel execution, hence we need per CPU stack isolation to execute bL_do_switch(). After the outbound CPU has ungated the inbound CPU, it calls mcpm_cpu_power_down() to:

* Clean its L1 cache.

* If it is the last CPU still alive in its cluster (last man standing), it also cleans its L2 cache and disables cache snooping from the other cluster.

* Power down the CPU (or whole cluster).

Code called from bL_do_switch() might end up referencing 'current' for some reasons. However, 'current' is derived from the stack pointer. With any arbitrary stack, the returned value for 'current' and any dereferenced values through it are just random garbage which may lead to segmentation faults.

The active page table during the execution of bL_do_switch() is also a problem. There is no guarantee that the inbound CPU won't destroy the corresponding task which would free the attached page table while the outbound CPU is still running and relying on it.

To solve both issues, we borrow some of the task space belonging to the init/idle task which, by its nature, is lightly used and therefore is unlikely to clash with our usage. The init task is also never going away.

Right now the logical CPU number is assumed to be equivalent to the physical CPU number within each cluster. The kernel should also be booted with only one cluster active. These limitations will be lifted eventually.

Signed-off-by Nicolas Pitre <[email protected]>

---
  arch/arm/Kconfig                   |  17 +++
  arch/arm/common/Makefile           |   1 +
  arch/arm/common/bL_switcher.c      | 247 +++++++++++++++++++++++++++++++++++++
  arch/arm/include/asm/bL_switcher.h |  17 +++
  4 files changed, 282 insertions(+)

create mode 100644 arch/arm/common/bL_switcher.c create mode 100644 arch/arm/include/asm/bL_switcher.h

arch/arm/Kconfig +0 -0

index ba412e02ec..2c9e5bf734 100644

arch/arm/Kconfig +11 -0

+
+config BL_SWITCHER
+       bool "big.LITTLE switcher support"
+       depends on BIG_LITTLE && MCPM && HOTPLUG_CPU
+       select CPU_PM
+       select ARM_CPU_SUSPEND
+       help
+         The big.LITTLE "switcher" provides the core functionality to
+         transparently handle transition between a cluster of A15's
+         and a cluster of A7's in a big.LITTLE system.
+
  choice
         prompt "Memory split"
         default VMSPLIT_3G

arch/arm/common/Makefile +1 -0

index 8c60f473e9..2586240d5a 100644
@@ -17,3 +17,4 @@ obj-$(CONFIG_MCPM)            += mcpm_head.o mcpm_entry.o mcpm_platsmp.o vlock.o
  AFLAGS_mcpm_head.o             := -march=armv7-a
  AFLAGS_vlock.o                 := -march=armv7-a
  obj-$(CONFIG_TI_PRIV_EDMA)     += edma.o
+obj-$(CONFIG_BL_SWITCHER)      += bL_switcher.o

arch/arm/common/bL_switcher.c new file +29 -0

new file mode 100644
index 0000000000..e63881b430
@@ -0,0 +1,247 @@
+/*
+ * arch/arm/common/bL_switcher.c -- big.LITTLE cluster switcher core driver
+ *
+ * Created by: Nicolas Pitre, March 2012
+ * Copyright:  (C) 2012-2013  Linaro Limited
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/sched.h>
+#include <linux/interrupt.h>
+#include <linux/cpu_pm.h>
+#include <linux/workqueue.h>
+#include <linux/mm.h>
+#include <linux/string.h>
+#include <linux/irqchip/arm-gic.h>
+
+#include <asm/smp_plat.h>
+#include <asm/cacheflush.h>
+#include <asm/suspend.h>
+#include <asm/mcpm.h>
+#include <asm/bL_switcher.h>
+
+

arch/arm/common/bL_switcher.c new file +8 -0

+       return id & MPIDR_HWID_BITMASK;
+}
+
+/*
+ * bL switcher core code.
+ */
+
+static void bL_do_switch(void *_unused)

arch/arm/common/bL_switcher.c new file +38 -0

+
+       mpidr = read_mpidr();
+       cpuid = MPIDR_AFFINITY_LEVEL(mpidr, 0);
+       clusterid = MPIDR_AFFINITY_LEVEL(mpidr, 1);
+       ob_cluster = clusterid;
+       ib_cluster = clusterid ^ 1;
+
+       /*
+        * Our state has been saved at this point.  Let's release our
+        * inbound CPU.
+        */
+       mcpm_set_entry_vector(cpuid, ib_cluster, cpu_resume);
+       sev();
+
+       /*
+        * From this point, we must assume that our counterpart CPU might
+        * have taken over in its parallel world already, as if execution
+        * just returned from cpu_suspend().  It is therefore important to
+        * be very careful not to make any change the other guy is not
+        * expecting.  This is why we need stack isolation.
+        *
+        * Fancy under cover tasks could be performed here.  For now
+        * we have none.
+        */
+
+       /* Let's put ourself down. */
+       mcpm_cpu_power_down();
+
+       /* should never get here */
+       BUG();
+}
+
+/*
+ * Stack isolation.  To ensure 'current' remains valid, we just borrow
+ * a slice of the init/idle task which should be fairly lightly used.
+ * The borrowed area starts just above the thread_info structure located
+ * at the very bottom of the stack, aligned to a cache line.
+ */

arch/arm/common/bL_switcher.c new file +40 -0

+       stack += cpu_index * STACK_SIZE + STACK_SIZE;
+       call_with_stack(bL_do_switch, (void *)_arg, stack);
+       BUG();
+}
+
+/*
+ * Generic switcher interface
+ */
+
+/*
+ * bL_switch_to - Switch to a specific cluster for the current CPU
+ * @new_cluster_id: the ID of the cluster to switch to.
+ *
+ * This function must be called on the CPU to be switched.
+ * Returns 0 on success, else a negative status code.
+ */
+static int bL_switch_to(unsigned int new_cluster_id)
+{
+       unsigned int mpidr, cpuid, clusterid, ob_cluster, ib_cluster, this_cpu;
+       int ret;
+
+       mpidr = read_mpidr();
+       cpuid = MPIDR_AFFINITY_LEVEL(mpidr, 0);
+       clusterid = MPIDR_AFFINITY_LEVEL(mpidr, 1);
+       ob_cluster = clusterid;
+       ib_cluster = clusterid ^ 1;
+
+       if (new_cluster_id == clusterid)
+               return 0;
+
+       pr_debug("before switch: CPU %d in cluster %d\n", cpuid, clusterid);
+
+       /* Close the gate for our entry vectors */
+       mcpm_set_entry_vector(cpuid, ob_cluster, NULL);
+       mcpm_set_entry_vector(cpuid, ib_cluster, NULL);
+
+       /*
+        * Let's wake up the inbound CPU now in case it requires some delay
+        * to come online, but leave it gated in our entry vector code.
+        */

arch/arm/common/bL_switcher.c new file +24 -0

+       local_irq_disable();
+       local_fiq_disable();
+
+       this_cpu = smp_processor_id();
+
+       /* redirect GIC's SGIs to our counterpart */
+       gic_migrate_target(cpuid + ib_cluster*4);
+
+       /*
+        * Raise a SGI on the inbound CPU to make sure it doesn't stall
+        * in a possible WFI, such as in mcpm_power_down().
+        */
+       arch_send_wakeup_ipi_mask(cpumask_of(this_cpu));
+
+       ret = cpu_pm_enter();
+
+       /* we can not tolerate errors at this point */
+       if (ret)
+               panic("%s: cpu_pm_enter() returned %d\n", __func__, ret);
+
+       /*
+        * Flip the cluster in the CPU logical map for this CPU.
+        * This must be flushed to RAM as the resume code
+        * needs to access it while the caches are still disabled.

arch/arm/common/bL_switcher.c new file +41 -0

+       BUG_ON(clusterid != ib_cluster);
+
+       mcpm_cpu_powered_up();
+
+       ret = cpu_pm_exit();
+
+       local_fiq_enable();
+       local_irq_enable();
+
+       if (ret)
+               pr_err("%s exiting with error %d\n", __func__, ret);
+       return ret;
+}
+
+struct switch_args {
+       unsigned int cluster;
+       struct work_struct work;
+};
+
+static void __bL_switch_to(struct work_struct *work)
+{
+       struct switch_args *args = container_of(work, struct switch_args, work);
+       bL_switch_to(args->cluster);
+}
+
+/*
+ * bL_switch_request - Switch to a specific cluster for the given CPU
+ *
+ * @cpu: the CPU to switch
+ * @new_cluster_id: the ID of the cluster to switch to.
+ *
+ * This function causes a cluster switch on the given CPU.  If the given
+ * CPU is the same as the calling CPU then the switch happens right away.
+ * Otherwise the request is put on a work queue to be scheduled on the
+ * remote CPU.
+ */
+void bL_switch_request(unsigned int cpu, unsigned int new_cluster_id)
+{
+       unsigned int this_cpu = get_cpu();
+       struct switch_args args;
+

arch/arm/common/bL_switcher.c new file +3 -0

+       flush_work(&args.work);
+}
+EXPORT_SYMBOL_GPL(bL_switch_request);

arch/arm/include/asm/bL_switcher.h new file +17 -1

new file mode 100644
index 0000000000..72efe3f349
@@ -0,0 +1,17 @@
+/*
+ * arch/arm/include/asm/bL_switcher.h
+ *
+ * Created by:  Nicolas Pitre, April 2012
+ * Copyright:   (C) 2012-2013  Linaro Limited
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef ASM_BL_SWITCHER_H
+#define ASM_BL_SWITCHER_H
+
+void bL_switch_request(unsigned int cpu, unsigned int new_cluster_id);
+
+#endif
--

1.8.1.2

Hi Nico,

I've got a few more questions here...

Quoted 84 lines · Nicolas Pitre

This is the core code implementing big.LITTLE switcher functionality. Rational for this code is available here:

http://lwn.net/Articles/481055/

The main entry point for a switch request is:

void bL_switch_request(unsigned int cpu, unsigned int new_cluster_id)

If the calling CPU is not the wanted one, this wrapper takes care of sending the request to the appropriate CPU with schedule_work_on().

At the moment the core switch operation is handled by bL_switch_to() which must be called on the CPU for which a switch is requested.

What this code does:

* Return early if the current cluster is the wanted one.

* Close the gate in the kernel entry vector for both the inbound and outbound CPUs.

* Wake up the inbound CPU so it can perform its reset sequence in parallel up to the kernel entry vector gate.

* Migrate all interrupts in the GIC targeting the outbound CPU interface to the inbound CPU interface, including SGIs. This is performed by gic_migrate_target() in drivers/irqchip/irq-gic.c.

* Call cpu_pm_enter() which takes care of flushing the VFP state to RAM and save the CPU interface config from the GIC to RAM.

* Modify the cpu_logical_map to refer to the inbound physical CPU.

* Call cpu_suspend() which saves the CPU state (general purpose registers, page table address) onto the stack and store the resulting stack pointer in an array indexed by the updated cpu_logical_map, then call the provided shutdown function. This happens in arch/arm/kernel/sleep.S.

At this point, the provided shutdown function executed by the outbound CPU ungates the inbound CPU. Therefore the inbound CPU:

* Picks up the saved stack pointer in the array indexed by its MPIDR in arch/arm/kernel/sleep.S.

* The MMU and caches are re-enabled using the saved state on the provided stack, just like if this was a resume operation from a suspended state.

* Then cpu_suspend() returns, although this is on the inbound CPU rather than the outbound CPU which called it initially.

* The function cpu_pm_exit() is called which effect is to restore the CPU interface state in the GIC using the state previously saved by the outbound CPU.

* Exit of bL_switch_to() to resume normal kernel execution on the new CPU.

However, the outbound CPU is potentially still running in parallel while the inbound CPU is resuming normal kernel execution, hence we need per CPU stack isolation to execute bL_do_switch(). After the outbound CPU has ungated the inbound CPU, it calls mcpm_cpu_power_down() to:

* Clean its L1 cache.

* If it is the last CPU still alive in its cluster (last man standing), it also cleans its L2 cache and disables cache snooping from the other cluster.

* Power down the CPU (or whole cluster).

Code called from bL_do_switch() might end up referencing 'current' for some reasons. However, 'current' is derived from the stack pointer. With any arbitrary stack, the returned value for 'current' and any dereferenced values through it are just random garbage which may lead to segmentation faults.

The active page table during the execution of bL_do_switch() is also a problem. There is no guarantee that the inbound CPU won't destroy the corresponding task which would free the attached page table while the outbound CPU is still running and relying on it.

Quoted 4 lines

To solve both issues, we borrow some of the task space belonging to the init/idle task which, by its nature, is lightly used and therefore is unlikely to clash with our usage. The init task is also never going away.

I had written a comment here about this looking hairy, and asking whether you'd considered using dedicated threads for this.

As I hit the later patches in the series I see that's exactly what you've done. Is it really worth keeping the historical solution in here?

Could build/bisectability be maintained by moving the Kconfig options later in the series, but without requiring the weird init/idle hack to be kept around?

Quoted 5 lines

Right now the logical CPU number is assumed to be equivalent to the physical CPU number within each cluster. The kernel should also be booted with only one cluster active. These limitations will be lifted eventually.

I just read this aloud in the office and Dave Martin mentioned that the MPIDR hashing schemes make this obselete now?

Quoted 16 lines

Signed-off-by Nicolas Pitre <[email protected]>

---
  arch/arm/Kconfig                   |  17 +++
  arch/arm/common/Makefile           |   1 +
  arch/arm/common/bL_switcher.c      | 247
+++++++++++++++++++++++++++++++++++++
  arch/arm/include/asm/bL_switcher.h |  17 +++
  4 files changed, 282 insertions(+)

create mode 100644 arch/arm/common/bL_switcher.c create mode 100644 arch/arm/include/asm/bL_switcher.h

arch/arm/Kconfig +0 -0

index ba412e02ec..2c9e5bf734 100644

arch/arm/Kconfig config MCPM · line 1546

Quoted 9 lines

arch/arm/Kconfig +6 -0

@@ -1538,6 +1538,23 @@ config MCPM
           for (multi-)cluster based systems, such as big.LITTLE based
           systems.
 
+config BIG_LITTLE
+       bool "big.LITTLE support (Experimental)"
+       depends on CPU_V7 && SMP
+       select MCPM
+       help
+         This option enables support for the big.LITTLE architecture.

I'm not sure we should describe this as an architecture?

"This option enables support for big.LITTLE processing"?

What are the plans for/implications of having CONFIG_BIG_LITTLE and !CONFIG_BL_SWITCHER?

Quoted 64 lines

arch/arm/Kconfig +10 -0

+config BL_SWITCHER
+       bool "big.LITTLE switcher support"
+       depends on BIG_LITTLE && MCPM && HOTPLUG_CPU
+       select CPU_PM
+       select ARM_CPU_SUSPEND
+       help
+         The big.LITTLE "switcher" provides the core functionality to
+         transparently handle transition between a cluster of A15's
+         and a cluster of A7's in a big.LITTLE system.
+
  choice
         prompt "Memory split"
         default VMSPLIT_3G

arch/arm/common/Makefile +0 -0

index 8c60f473e9..2586240d5a 100644
@@ -17,3 +17,4 @@ obj-$(CONFIG_MCPM)            += mcpm_head.o mcpm_entry.o

mcpm_platsmp.o vlock.o

  AFLAGS_mcpm_head.o             := -march=armv7-a
  AFLAGS_vlock.o                 := -march=armv7-a
  obj-$(CONFIG_TI_PRIV_EDMA)     += edma.o

arch/arm/common/Makefile +1 -0

+obj-$(CONFIG_BL_SWITCHER)      += bL_switcher.o

arch/arm/common/bL_switcher.c new file +35 -0

new file mode 100644
index 0000000000..e63881b430
@@ -0,0 +1,247 @@
+/*
+ * arch/arm/common/bL_switcher.c -- big.LITTLE cluster switcher core driver
+ *
+ * Created by: Nicolas Pitre, March 2012
+ * Copyright:  (C) 2012-2013  Linaro Limited
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/sched.h>
+#include <linux/interrupt.h>
+#include <linux/cpu_pm.h>
+#include <linux/workqueue.h>
+#include <linux/mm.h>
+#include <linux/string.h>
+#include <linux/irqchip/arm-gic.h>
+
+#include <asm/smp_plat.h>
+#include <asm/cacheflush.h>
+#include <asm/suspend.h>
+#include <asm/mcpm.h>
+#include <asm/bL_switcher.h>
+
+
+/*
+ * Use our own MPIDR accessors as the generic ones in asm/cputype.h have
+ * __attribute_const__ and we don't want the compiler to assume any
+ * constness here as the value _does_ change along some code paths.
+ */
+

arch/arm/common/bL_switcher.c line 39

Quoted 4 lines

arch/arm/common/bL_switcher.c new file +4 -0

+static int read_mpidr(void)
+{
+       unsigned int id;
+       asm volatile ("mrc\tp15, 0, %0, c0, c0, 5" : "=r" (id));

Is the \t in here an artefact of our mail servers? I notice that in other places, for example arch/arm/include/asm/cp15.h, we don't have that formatting.

Quoted 13 lines

arch/arm/common/bL_switcher.c new file +13 -0

+       return id & MPIDR_HWID_BITMASK;
+}
+
+/*
+ * bL switcher core code.
+ */
+
+static void bL_do_switch(void *_unused)
+{
+       unsigned mpidr, cpuid, clusterid, ob_cluster, ib_cluster;
+
+       /*
+        * We now have a piece of stack borrowed from the init task's.

arch/arm/common/bL_switcher.c line 57

Quoted 5 lines

arch/arm/common/bL_switcher.c new file +5 -0

+        * Let's also switch to init_mm right away to match it.
+        */
+       cpu_switch_mm(init_mm.pgd, &init_mm);
+
+       pr_debug("%s\n", __func__);

This looks like fairly uninformative debug - is it left here intentionally? If so, isn't there a better message to print than just the function name?

Quoted 43 lines

arch/arm/common/bL_switcher.c new file +43 -0

+       mpidr = read_mpidr();
+       cpuid = MPIDR_AFFINITY_LEVEL(mpidr, 0);
+       clusterid = MPIDR_AFFINITY_LEVEL(mpidr, 1);
+       ob_cluster = clusterid;
+       ib_cluster = clusterid ^ 1;
+
+       /*
+        * Our state has been saved at this point.  Let's release our
+        * inbound CPU.
+        */
+       mcpm_set_entry_vector(cpuid, ib_cluster, cpu_resume);
+       sev();
+
+       /*
+        * From this point, we must assume that our counterpart CPU might
+        * have taken over in its parallel world already, as if execution
+        * just returned from cpu_suspend().  It is therefore important to
+        * be very careful not to make any change the other guy is not
+        * expecting.  This is why we need stack isolation.
+        *
+        * Fancy under cover tasks could be performed here.  For now
+        * we have none.
+        */
+
+       /* Let's put ourself down. */
+       mcpm_cpu_power_down();
+
+       /* should never get here */
+       BUG();
+}
+
+/*
+ * Stack isolation.  To ensure 'current' remains valid, we just borrow
+ * a slice of the init/idle task which should be fairly lightly used.
+ * The borrowed area starts just above the thread_info structure located
+ * at the very bottom of the stack, aligned to a cache line.
+ */
+#define STACK_SIZE 256
+extern void call_with_stack(void (*fn)(void *), void *arg, void *sp);
+static int bL_switchpoint(unsigned long _arg)
+{
+       unsigned int mpidr = read_mpidr();
+       unsigned int cpuid = MPIDR_AFFINITY_LEVEL(mpidr, 0);

arch/arm/common/bL_switcher.c line 105

Quoted 4 lines

arch/arm/common/bL_switcher.c new file +4 -0

+       unsigned int clusterid = MPIDR_AFFINITY_LEVEL(mpidr, 1);
+       unsigned int cpu_index = cpuid + clusterid * MAX_CPUS_PER_CLUSTER;
+       void *stack = &init_thread_info + 1;
+       stack = PTR_ALIGN(stack, L1_CACHE_BYTES);

Are there any implications if there were big.LITTLE cores with different cache line sizes? The LWN article that you reference mentions different cache topologies...

Quoted 45 lines

arch/arm/common/bL_switcher.c new file +19 -0

+       stack += cpu_index * STACK_SIZE + STACK_SIZE;
+       call_with_stack(bL_do_switch, (void *)_arg, stack);
+       BUG();
+}
+
+/*
+ * Generic switcher interface
+ */
+
+/*
+ * bL_switch_to - Switch to a specific cluster for the current CPU
+ * @new_cluster_id: the ID of the cluster to switch to.
+ *
+ * This function must be called on the CPU to be switched.
+ * Returns 0 on success, else a negative status code.
+ */
+static int bL_switch_to(unsigned int new_cluster_id)
+{
+       unsigned int mpidr, cpuid, clusterid, ob_cluster, ib_cluster,

this_cpu;

arch/arm/common/bL_switcher.c new file +24 -0

+       int ret;
+
+       mpidr = read_mpidr();
+       cpuid = MPIDR_AFFINITY_LEVEL(mpidr, 0);
+       clusterid = MPIDR_AFFINITY_LEVEL(mpidr, 1);
+       ob_cluster = clusterid;
+       ib_cluster = clusterid ^ 1;
+
+       if (new_cluster_id == clusterid)
+               return 0;
+
+       pr_debug("before switch: CPU %d in cluster %d\n", cpuid, clusterid);
+
+       /* Close the gate for our entry vectors */
+       mcpm_set_entry_vector(cpuid, ob_cluster, NULL);
+       mcpm_set_entry_vector(cpuid, ib_cluster, NULL);
+
+       /*
+        * Let's wake up the inbound CPU now in case it requires some delay
+        * to come online, but leave it gated in our entry vector code.
+        */
+       ret = mcpm_cpu_power_up(cpuid, ib_cluster);
+       if (ret) {
+               pr_err("%s: mcpm_cpu_power_up() returned %d\n", __func__,

ret);

arch/arm/common/bL_switcher.c line 155

Quoted 7 lines

arch/arm/common/bL_switcher.c new file +7 -0

+               return ret;
+       }
+
+       /*
+        * From this point we are entering the switch critical zone
+        * and can't sleep/schedule anymore.
+        */

Will this function ever be called from anywhere other than bL_switch_request? As far as I could see, bl_switch_request uses two different methods of ensuring that we will still be on the same CPU throughout this function (if called directly the get_cpu and put_cpu dis/enbale pre-emption and via the work queue we're guaranteed the same logical cpu). The different approaches somewhat obfuscate the need for something to have been done even before this point to stop us getting migrated to another CPU, I think...

Quoted 24 lines

arch/arm/common/bL_switcher.c new file +24 -0

+       local_irq_disable();
+       local_fiq_disable();
+
+       this_cpu = smp_processor_id();
+
+       /* redirect GIC's SGIs to our counterpart */
+       gic_migrate_target(cpuid + ib_cluster*4);
+
+       /*
+        * Raise a SGI on the inbound CPU to make sure it doesn't stall
+        * in a possible WFI, such as in mcpm_power_down().
+        */
+       arch_send_wakeup_ipi_mask(cpumask_of(this_cpu));
+
+       ret = cpu_pm_enter();
+
+       /* we can not tolerate errors at this point */
+       if (ret)
+               panic("%s: cpu_pm_enter() returned %d\n", __func__, ret);
+
+       /*
+        * Flip the cluster in the CPU logical map for this CPU.
+        * This must be flushed to RAM as the resume code
+        * needs to access it while the caches are still disabled.

arch/arm/common/bL_switcher.c line 189

Quoted 10 lines

arch/arm/common/bL_switcher.c new file +10 -0

+        */
+       cpu_logical_map(this_cpu) ^= (1 << 8);
+       sync_cache_w(&cpu_logical_map(this_cpu));
+
+       /* Let's do the actual CPU switch. */
+       ret = cpu_suspend(0, bL_switchpoint);
+       if (ret > 0)
+               panic("%s: cpu_suspend() returned %d\n", __func__, ret);
+
+       /* We are executing on the inbound CPU at this point */

Very cool :)

arch/arm/common/bL_switcher.c line 193

Quoted 4 lines

arch/arm/common/bL_switcher.c new file +4 -0

+       mpidr = read_mpidr();
+       cpuid = MPIDR_AFFINITY_LEVEL(mpidr, 0);
+       clusterid = MPIDR_AFFINITY_LEVEL(mpidr, 1);
+       pr_debug("after switch: CPU %d in cluster %d\n", cpuid, clusterid);

Do we want a 'switcher' prefix on these? "bL_switcher: after switch..." etc?

Quoted 42 lines

arch/arm/common/bL_switcher.c new file +22 -0

+       BUG_ON(clusterid != ib_cluster);
+
+       mcpm_cpu_powered_up();
+
+       ret = cpu_pm_exit();
+
+       local_fiq_enable();
+       local_irq_enable();
+
+       if (ret)
+               pr_err("%s exiting with error %d\n", __func__, ret);
+       return ret;
+}
+
+struct switch_args {
+       unsigned int cluster;
+       struct work_struct work;
+};
+
+static void __bL_switch_to(struct work_struct *work)
+{
+       struct switch_args *args = container_of(work, struct switch_args,

work);

arch/arm/common/bL_switcher.c new file +19 -0

+       bL_switch_to(args->cluster);
+}
+
+/*
+ * bL_switch_request - Switch to a specific cluster for the given CPU
+ *
+ * @cpu: the CPU to switch
+ * @new_cluster_id: the ID of the cluster to switch to.
+ *
+ * This function causes a cluster switch on the given CPU.  If the given
+ * CPU is the same as the calling CPU then the switch happens right away.
+ * Otherwise the request is put on a work queue to be scheduled on the
+ * remote CPU.
+ */
+void bL_switch_request(unsigned int cpu, unsigned int new_cluster_id)
+{
+       unsigned int this_cpu = get_cpu();
+       struct switch_args args;
+

arch/arm/common/bL_switcher.c line 244

Quoted 10 lines

arch/arm/common/bL_switcher.c new file +10 -0

+       if (cpu == this_cpu) {
+               bL_switch_to(new_cluster_id);
+               put_cpu();
+               return;
+       }
+       put_cpu();
+
+       args.cluster = new_cluster_id;
+       INIT_WORK_ONSTACK(&args.work, __bL_switch_to);
+       schedule_work_on(cpu, &args.work);

The comment at the top of schedule_work_on() says:
[...]
* We queue the work to a specific CPU, the caller must ensure it
* can't go away.
[...]

I see we don't disable cpu hotplug requests until later in the series - does that matter? Until that patch, it seems that we could hotplug off the core that we'd scheduled the work onto?

Hope some of those questions are helpful,

This is the core code implementing big.LITTLE switcher functionality. Rational for this code is available here:

http://lwn.net/Articles/481055/

The main entry point for a switch request is:

void bL_switch_request(unsigned int cpu, unsigned int new_cluster_id)

If the calling CPU is not the wanted one, this wrapper takes care of sending the request to the appropriate CPU with schedule_work_on().

At the moment the core switch operation is handled by bL_switch_to() which must be called on the CPU for which a switch is requested.

What this code does:

* Return early if the current cluster is the wanted one.

* Close the gate in the kernel entry vector for both the inbound and outbound CPUs.

* Wake up the inbound CPU so it can perform its reset sequence in parallel up to the kernel entry vector gate.

* Migrate all interrupts in the GIC targeting the outbound CPU interface to the inbound CPU interface, including SGIs. This is performed by gic_migrate_target() in drivers/irqchip/irq-gic.c.

* Call cpu_pm_enter() which takes care of flushing the VFP state to RAM and save the CPU interface config from the GIC to RAM.

* Modify the cpu_logical_map to refer to the inbound physical CPU.

* Call cpu_suspend() which saves the CPU state (general purpose registers, page table address) onto the stack and store the resulting stack pointer in an array indexed by the updated cpu_logical_map, then call the provided shutdown function. This happens in arch/arm/kernel/sleep.S.

At this point, the provided shutdown function executed by the outbound CPU ungates the inbound CPU. Therefore the inbound CPU:

* Picks up the saved stack pointer in the array indexed by its MPIDR in arch/arm/kernel/sleep.S.

* The MMU and caches are re-enabled using the saved state on the provided stack, just like if this was a resume operation from a suspended state.

* Then cpu_suspend() returns, although this is on the inbound CPU rather than the outbound CPU which called it initially.

* The function cpu_pm_exit() is called which effect is to restore the CPU interface state in the GIC using the state previously saved by the outbound CPU.

* Exit of bL_switch_to() to resume normal kernel execution on the new CPU.

However, the outbound CPU is potentially still running in parallel while the inbound CPU is resuming normal kernel execution, hence we need per CPU stack isolation to execute bL_do_switch(). After the outbound CPU has ungated the inbound CPU, it calls mcpm_cpu_power_down() to:

* Clean its L1 cache.

* If it is the last CPU still alive in its cluster (last man standing), it also cleans its L2 cache and disables cache snooping from the other cluster.

* Power down the CPU (or whole cluster).

Code called from bL_do_switch() might end up referencing 'current' for some reasons. However, 'current' is derived from the stack pointer. With any arbitrary stack, the returned value for 'current' and any dereferenced values through it are just random garbage which may lead to segmentation faults.

The active page table during the execution of bL_do_switch() is also a problem. There is no guarantee that the inbound CPU won't destroy the corresponding task which would free the attached page table while the outbound CPU is still running and relying on it.

To solve both issues, we borrow some of the task space belonging to the init/idle task which, by its nature, is lightly used and therefore is unlikely to clash with our usage. The init task is also never going away.

Right now the logical CPU number is assumed to be equivalent to the physical CPU number within each cluster. The kernel should also be booted with only one cluster active. These limitations will be lifted eventually.

Signed-off-by Nicolas Pitre <[email protected]>

---
 arch/arm/Kconfig                   |  17 +++
 arch/arm/common/Makefile           |   1 +
 arch/arm/common/bL_switcher.c      | 247 +++++++++++++++++++++++++++++++++++++
 arch/arm/include/asm/bL_switcher.h |  17 +++
 4 files changed, 282 insertions(+)
 create mode 100644 arch/arm/common/bL_switcher.c
 create mode 100644 arch/arm/include/asm/bL_switcher.h

arch/arm/Kconfig +0 -0

index ba412e02ec..2c9e5bf734 100644

arch/arm/Kconfig +10 -0

+config BL_SWITCHER
+       bool "big.LITTLE switcher support"
+       depends on BIG_LITTLE && MCPM && HOTPLUG_CPU
+       select CPU_PM
+       select ARM_CPU_SUSPEND
+       help
+         The big.LITTLE "switcher" provides the core functionality to
+         transparently handle transition between a cluster of A15's
+         and a cluster of A7's in a big.LITTLE system.
+
 choice
        prompt "Memory split"
        default VMSPLIT_3G

arch/arm/common/Makefile +1 -0

index 8c60f473e9..2586240d5a 100644
@@ -17,3 +17,4 @@ obj-$(CONFIG_MCPM)            += mcpm_head.o mcpm_entry.o mcpm_platsmp.o vlock.o
 AFLAGS_mcpm_head.o             := -march=armv7-a
 AFLAGS_vlock.o                 := -march=armv7-a
 obj-$(CONFIG_TI_PRIV_EDMA)     += edma.o
+obj-$(CONFIG_BL_SWITCHER)      += bL_switcher.o

arch/arm/common/bL_switcher.c new file +45 -0

new file mode 100644
index 0000000000..e63881b430
@@ -0,0 +1,247 @@
+/*
+ * arch/arm/common/bL_switcher.c -- big.LITTLE cluster switcher core driver
+ *
+ * Created by: Nicolas Pitre, March 2012
+ * Copyright:  (C) 2012-2013  Linaro Limited
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/sched.h>
+#include <linux/interrupt.h>
+#include <linux/cpu_pm.h>
+#include <linux/workqueue.h>
+#include <linux/mm.h>
+#include <linux/string.h>
+#include <linux/irqchip/arm-gic.h>
+
+#include <asm/smp_plat.h>
+#include <asm/cacheflush.h>
+#include <asm/suspend.h>
+#include <asm/mcpm.h>
+#include <asm/bL_switcher.h>
+
+
+/*
+ * Use our own MPIDR accessors as the generic ones in asm/cputype.h have
+ * __attribute_const__ and we don't want the compiler to assume any
+ * constness here as the value _does_ change along some code paths.
+ */
+
+static int read_mpidr(void)
+{
+       unsigned int id;
+       asm volatile ("mrc\tp15, 0, %0, c0, c0, 5" : "=r" (id));
+       return id & MPIDR_HWID_BITMASK;
+}
+
+/*
+ * bL switcher core code.
+ */

arch/arm/common/bL_switcher.c new file +41 -0

+
+       pr_debug("%s\n", __func__);
+
+       mpidr = read_mpidr();
+       cpuid = MPIDR_AFFINITY_LEVEL(mpidr, 0);
+       clusterid = MPIDR_AFFINITY_LEVEL(mpidr, 1);
+       ob_cluster = clusterid;
+       ib_cluster = clusterid ^ 1;
+
+       /*
+        * Our state has been saved at this point.  Let's release our
+        * inbound CPU.
+        */
+       mcpm_set_entry_vector(cpuid, ib_cluster, cpu_resume);
+       sev();
+
+       /*
+        * From this point, we must assume that our counterpart CPU might
+        * have taken over in its parallel world already, as if execution
+        * just returned from cpu_suspend().  It is therefore important to
+        * be very careful not to make any change the other guy is not
+        * expecting.  This is why we need stack isolation.
+        *
+        * Fancy under cover tasks could be performed here.  For now
+        * we have none.
+        */
+
+       /* Let's put ourself down. */
+       mcpm_cpu_power_down();
+
+       /* should never get here */
+       BUG();
+}
+
+/*
+ * Stack isolation.  To ensure 'current' remains valid, we just borrow
+ * a slice of the init/idle task which should be fairly lightly used.
+ * The borrowed area starts just above the thread_info structure located
+ * at the very bottom of the stack, aligned to a cache line.
+ */
+#define STACK_SIZE 256

arch/arm/common/bL_switcher.c new file +66 -0

+       call_with_stack(bL_do_switch, (void *)_arg, stack);
+       BUG();
+}
+
+/*
+ * Generic switcher interface
+ */
+
+/*
+ * bL_switch_to - Switch to a specific cluster for the current CPU
+ * @new_cluster_id: the ID of the cluster to switch to.
+ *
+ * This function must be called on the CPU to be switched.
+ * Returns 0 on success, else a negative status code.
+ */
+static int bL_switch_to(unsigned int new_cluster_id)
+{
+       unsigned int mpidr, cpuid, clusterid, ob_cluster, ib_cluster, this_cpu;
+       int ret;
+
+       mpidr = read_mpidr();
+       cpuid = MPIDR_AFFINITY_LEVEL(mpidr, 0);
+       clusterid = MPIDR_AFFINITY_LEVEL(mpidr, 1);
+       ob_cluster = clusterid;
+       ib_cluster = clusterid ^ 1;
+
+       if (new_cluster_id == clusterid)
+               return 0;
+
+       pr_debug("before switch: CPU %d in cluster %d\n", cpuid, clusterid);
+
+       /* Close the gate for our entry vectors */
+       mcpm_set_entry_vector(cpuid, ob_cluster, NULL);
+       mcpm_set_entry_vector(cpuid, ib_cluster, NULL);
+
+       /*
+        * Let's wake up the inbound CPU now in case it requires some delay
+        * to come online, but leave it gated in our entry vector code.
+        */
+       ret = mcpm_cpu_power_up(cpuid, ib_cluster);
+       if (ret) {
+               pr_err("%s: mcpm_cpu_power_up() returned %d\n", __func__, ret);
+               return ret;
+       }
+
+       /*
+        * From this point we are entering the switch critical zone
+        * and can't sleep/schedule anymore.
+        */
+       local_irq_disable();
+       local_fiq_disable();
+
+       this_cpu = smp_processor_id();
+
+       /* redirect GIC's SGIs to our counterpart */
+       gic_migrate_target(cpuid + ib_cluster*4);
+
+       /*
+        * Raise a SGI on the inbound CPU to make sure it doesn't stall
+        * in a possible WFI, such as in mcpm_power_down().
+        */
+       arch_send_wakeup_ipi_mask(cpumask_of(this_cpu));
+
+       ret = cpu_pm_enter();
+
+       /* we can not tolerate errors at this point */

Quoted 29 lines · Nicolas Pitre

This is the core code implementing big.LITTLE switcher functionality. Rational for this code is available here:

http://lwn.net/Articles/481055/

The main entry point for a switch request is:

void bL_switch_request(unsigned int cpu, unsigned int new_cluster_id)

If the calling CPU is not the wanted one, this wrapper takes care of sending the request to the appropriate CPU with schedule_work_on().

At the moment the core switch operation is handled by bL_switch_to() which must be called on the CPU for which a switch is requested.

What this code does:

* Return early if the current cluster is the wanted one.

* Close the gate in the kernel entry vector for both the inbound and outbound CPUs.

* Wake up the inbound CPU so it can perform its reset sequence in parallel up to the kernel entry vector gate.

* Migrate all interrupts in the GIC targeting the outbound CPU interface to the inbound CPU interface, including SGIs. This is performed by gic_migrate_target() in drivers/irqchip/irq-gic.c.

Quoted 2 lines

* Call cpu_pm_enter() which takes care of flushing the VFP state to RAM and save the CPU interface config from the GIC to RAM.

If I read the code properly, you always resume on the same logical cpu id you suspended on (different HW CPU of course), correct ? This is to validate my assumptions on CPU PM notifiers and other code.

Quoted 48 lines

* Modify the cpu_logical_map to refer to the inbound physical CPU.

* Call cpu_suspend() which saves the CPU state (general purpose registers, page table address) onto the stack and store the resulting stack pointer in an array indexed by the updated cpu_logical_map, then call the provided shutdown function. This happens in arch/arm/kernel/sleep.S.

At this point, the provided shutdown function executed by the outbound CPU ungates the inbound CPU. Therefore the inbound CPU:

* Picks up the saved stack pointer in the array indexed by its MPIDR in arch/arm/kernel/sleep.S.

* The MMU and caches are re-enabled using the saved state on the provided stack, just like if this was a resume operation from a suspended state.

* Then cpu_suspend() returns, although this is on the inbound CPU rather than the outbound CPU which called it initially.

* The function cpu_pm_exit() is called which effect is to restore the CPU interface state in the GIC using the state previously saved by the outbound CPU.

* Exit of bL_switch_to() to resume normal kernel execution on the new CPU.

However, the outbound CPU is potentially still running in parallel while the inbound CPU is resuming normal kernel execution, hence we need per CPU stack isolation to execute bL_do_switch(). After the outbound CPU has ungated the inbound CPU, it calls mcpm_cpu_power_down() to:

* Clean its L1 cache.

* If it is the last CPU still alive in its cluster (last man standing), it also cleans its L2 cache and disables cache snooping from the other cluster.

* Power down the CPU (or whole cluster).

Code called from bL_do_switch() might end up referencing 'current' for some reasons. However, 'current' is derived from the stack pointer. With any arbitrary stack, the returned value for 'current' and any dereferenced values through it are just random garbage which may lead to segmentation faults.

Quoted 4 lines

The active page table during the execution of bL_do_switch() is also a problem. There is no guarantee that the inbound CPU won't destroy the corresponding task which would free the attached page table while the outbound CPU is still running and relying on it.

Yes this is really a sticking point, some comments below.

Quoted 4 lines

To solve both issues, we borrow some of the task space belonging to the init/idle task which, by its nature, is lightly used and therefore is unlikely to clash with our usage. The init task is also never going away.

Unlikely does not mean it will always work and I think it is best avoided. Better stick to kernel threads, or a temporary stack but I understand for a temporary stack to work you have to force constraints on the mcpm backend you do not necessarily want to. More below.

Quoted 20 lines

Right now the logical CPU number is assumed to be equivalent to the physical CPU number within each cluster. The kernel should also be booted with only one cluster active. These limitations will be lifted eventually.

Signed-off-by Nicolas Pitre <[email protected]>

---
 arch/arm/Kconfig                   |  17 +++
 arch/arm/common/Makefile           |   1 +
 arch/arm/common/bL_switcher.c      | 247 +++++++++++++++++++++++++++++++++++++
 arch/arm/include/asm/bL_switcher.h |  17 +++
 4 files changed, 282 insertions(+)
 create mode 100644 arch/arm/common/bL_switcher.c
 create mode 100644 arch/arm/include/asm/bL_switcher.h

arch/arm/Kconfig +0 -0

index ba412e02ec..2c9e5bf734 100644

arch/arm/Kconfig config MCPM · line 1546

Quoted 9 lines

arch/arm/Kconfig +6 -0

@@ -1538,6 +1538,23 @@ config MCPM
          for (multi-)cluster based systems, such as big.LITTLE based
          systems.
 
+config BIG_LITTLE
+       bool "big.LITTLE support (Experimental)"
+       depends on CPU_V7 && SMP
+       select MCPM
+       help
+         This option enables support for the big.LITTLE architecture.

Probably this belongs in a separate patch (CPU idle driver probably needs it too). Not a big deal though (and honestly I do not know how you can justify a single patch for that - we need to add this config option, somehow).

Quoted 80 lines

arch/arm/Kconfig +10 -0

+config BL_SWITCHER
+       bool "big.LITTLE switcher support"
+       depends on BIG_LITTLE && MCPM && HOTPLUG_CPU
+       select CPU_PM
+       select ARM_CPU_SUSPEND
+       help
+         The big.LITTLE "switcher" provides the core functionality to
+         transparently handle transition between a cluster of A15's
+         and a cluster of A7's in a big.LITTLE system.
+
 choice
        prompt "Memory split"
        default VMSPLIT_3G

arch/arm/common/Makefile +1 -0

index 8c60f473e9..2586240d5a 100644
@@ -17,3 +17,4 @@ obj-$(CONFIG_MCPM)            += mcpm_head.o mcpm_entry.o mcpm_platsmp.o vlock.o
 AFLAGS_mcpm_head.o             := -march=armv7-a
 AFLAGS_vlock.o                 := -march=armv7-a
 obj-$(CONFIG_TI_PRIV_EDMA)     += edma.o
+obj-$(CONFIG_BL_SWITCHER)      += bL_switcher.o

arch/arm/common/bL_switcher.c new file +52 -0

new file mode 100644
index 0000000000..e63881b430
@@ -0,0 +1,247 @@
+/*
+ * arch/arm/common/bL_switcher.c -- big.LITTLE cluster switcher core driver
+ *
+ * Created by: Nicolas Pitre, March 2012
+ * Copyright:  (C) 2012-2013  Linaro Limited
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/sched.h>
+#include <linux/interrupt.h>
+#include <linux/cpu_pm.h>
+#include <linux/workqueue.h>
+#include <linux/mm.h>
+#include <linux/string.h>
+#include <linux/irqchip/arm-gic.h>
+
+#include <asm/smp_plat.h>
+#include <asm/cacheflush.h>
+#include <asm/suspend.h>
+#include <asm/mcpm.h>
+#include <asm/bL_switcher.h>
+
+
+/*
+ * Use our own MPIDR accessors as the generic ones in asm/cputype.h have
+ * __attribute_const__ and we don't want the compiler to assume any
+ * constness here as the value _does_ change along some code paths.
+ */
+
+static int read_mpidr(void)
+{
+       unsigned int id;
+       asm volatile ("mrc\tp15, 0, %0, c0, c0, 5" : "=r" (id));
+       return id & MPIDR_HWID_BITMASK;
+}
+
+/*
+ * bL switcher core code.
+ */
+
+static void bL_do_switch(void *_unused)
+{
+       unsigned mpidr, cpuid, clusterid, ob_cluster, ib_cluster;
+
+       /*
+        * We now have a piece of stack borrowed from the init task's.

arch/arm/common/bL_switcher.c line 55

Quoted 3 lines

arch/arm/common/bL_switcher.c new file +3 -0

+        * Let's also switch to init_mm right away to match it.
+        */
+       cpu_switch_mm(init_mm.pgd, &init_mm);

Is this cpu_switch_mm necessary ? You are executing it to make sure you get to wfi with page tables that can't be deallocated by the running inbound if I got it right. It is not needed to use the init stack, but you know that.

Quoted 41 lines

arch/arm/common/bL_switcher.c new file +41 -0

+
+       pr_debug("%s\n", __func__);
+
+       mpidr = read_mpidr();
+       cpuid = MPIDR_AFFINITY_LEVEL(mpidr, 0);
+       clusterid = MPIDR_AFFINITY_LEVEL(mpidr, 1);
+       ob_cluster = clusterid;
+       ib_cluster = clusterid ^ 1;
+
+       /*
+        * Our state has been saved at this point.  Let's release our
+        * inbound CPU.
+        */
+       mcpm_set_entry_vector(cpuid, ib_cluster, cpu_resume);
+       sev();
+
+       /*
+        * From this point, we must assume that our counterpart CPU might
+        * have taken over in its parallel world already, as if execution
+        * just returned from cpu_suspend().  It is therefore important to
+        * be very careful not to make any change the other guy is not
+        * expecting.  This is why we need stack isolation.
+        *
+        * Fancy under cover tasks could be performed here.  For now
+        * we have none.
+        */
+
+       /* Let's put ourself down. */
+       mcpm_cpu_power_down();
+
+       /* should never get here */
+       BUG();
+}
+
+/*
+ * Stack isolation.  To ensure 'current' remains valid, we just borrow
+ * a slice of the init/idle task which should be fairly lightly used.
+ * The borrowed area starts just above the thread_info structure located
+ * at the very bottom of the stack, aligned to a cache line.
+ */
+#define STACK_SIZE 256

arch/arm/common/bL_switcher.c line 106

Quoted 10 lines

arch/arm/common/bL_switcher.c new file +10 -0

+extern void call_with_stack(void (*fn)(void *), void *arg, void *sp);
+static int bL_switchpoint(unsigned long _arg)
+{
+       unsigned int mpidr = read_mpidr();
+       unsigned int cpuid = MPIDR_AFFINITY_LEVEL(mpidr, 0);
+       unsigned int clusterid = MPIDR_AFFINITY_LEVEL(mpidr, 1);
+       unsigned int cpu_index = cpuid + clusterid * MAX_CPUS_PER_CLUSTER;
+       void *stack = &init_thread_info + 1;
+       stack = PTR_ALIGN(stack, L1_CACHE_BYTES);
+       stack += cpu_index * STACK_SIZE + STACK_SIZE;

I do not like this much , for two reasons. Firstly because it might not work, secondly because whatever current usage we do from that point onwards is attached to the init thread, I am not sure that's 100% safe either.

Moving to kernel threads suffers from the same issue, even though I should grant there it is easier to monitor stack usage and grab safely some starting at the bottom. At least the current pointer behaviour is kept coherent. Overall the risk of using current on both outbound and inbound CPUs at once with no coordination is a bit scary, probably it is best to prevent current pointer usage altogether (but this forces constraints on MCPM). I will give it more thought.

Quoted 70 lines

arch/arm/common/bL_switcher.c new file +70 -0

+       call_with_stack(bL_do_switch, (void *)_arg, stack);
+       BUG();
+}
+
+/*
+ * Generic switcher interface
+ */
+
+/*
+ * bL_switch_to - Switch to a specific cluster for the current CPU
+ * @new_cluster_id: the ID of the cluster to switch to.
+ *
+ * This function must be called on the CPU to be switched.
+ * Returns 0 on success, else a negative status code.
+ */
+static int bL_switch_to(unsigned int new_cluster_id)
+{
+       unsigned int mpidr, cpuid, clusterid, ob_cluster, ib_cluster, this_cpu;
+       int ret;
+
+       mpidr = read_mpidr();
+       cpuid = MPIDR_AFFINITY_LEVEL(mpidr, 0);
+       clusterid = MPIDR_AFFINITY_LEVEL(mpidr, 1);
+       ob_cluster = clusterid;
+       ib_cluster = clusterid ^ 1;
+
+       if (new_cluster_id == clusterid)
+               return 0;
+
+       pr_debug("before switch: CPU %d in cluster %d\n", cpuid, clusterid);
+
+       /* Close the gate for our entry vectors */
+       mcpm_set_entry_vector(cpuid, ob_cluster, NULL);
+       mcpm_set_entry_vector(cpuid, ib_cluster, NULL);
+
+       /*
+        * Let's wake up the inbound CPU now in case it requires some delay
+        * to come online, but leave it gated in our entry vector code.
+        */
+       ret = mcpm_cpu_power_up(cpuid, ib_cluster);
+       if (ret) {
+               pr_err("%s: mcpm_cpu_power_up() returned %d\n", __func__, ret);
+               return ret;
+       }
+
+       /*
+        * From this point we are entering the switch critical zone
+        * and can't sleep/schedule anymore.
+        */
+       local_irq_disable();
+       local_fiq_disable();
+
+       this_cpu = smp_processor_id();
+
+       /* redirect GIC's SGIs to our counterpart */
+       gic_migrate_target(cpuid + ib_cluster*4);
+
+       /*
+        * Raise a SGI on the inbound CPU to make sure it doesn't stall
+        * in a possible WFI, such as in mcpm_power_down().
+        */
+       arch_send_wakeup_ipi_mask(cpumask_of(this_cpu));
+
+       ret = cpu_pm_enter();
+
+       /* we can not tolerate errors at this point */
+       if (ret)
+               panic("%s: cpu_pm_enter() returned %d\n", __func__, ret);
+
+       /*

arch/arm/common/bL_switcher.c line 182

Quoted 6 lines

arch/arm/common/bL_switcher.c new file +6 -0

+        * Flip the cluster in the CPU logical map for this CPU.
+        * This must be flushed to RAM as the resume code
+        * needs to access it while the caches are still disabled.
+        */
+       cpu_logical_map(this_cpu) ^= (1 << 8);
+       sync_cache_w(&cpu_logical_map(this_cpu));

sync_w is not needed anymore, cpu_resume does not use cpu_logical_map.

To solve both issues, we borrow some of the task space belonging to the init/idle task which, by its nature, is lightly used and therefore is unlikely to clash with our usage. The init task is also never going away.

Quoted 4 lines · Nicolas Pitre

To solve both issues, we borrow some of the task space belonging to the init/idle task which, by its nature, is lightly used and therefore is unlikely to clash with our usage. The init task is also never going away.

That is... quite a horrid hack - and it is a hack. You claim that the idle task is not likely to clash, but with 8 CPUs each of 256 bytes of stack, that's 2k of stack. With the thread data structure at over 512 bytes, that's 2.5k of 8k.

Don't forget that the amount of kernel stack used depends on many things - not just what the thread does (in the case of the idle thread, that includes cpuidle stuff) but also the function call depth of any interrupt in the system, as well as the path used to get into the idle thread.

If you want to do this, it would be much better to use the bottom of the stack space for the idle thread associated with the CPU. The idle threads are all forked at boot for any possible CPUs before the secondaries are brought online, and so are persistent. That would reduce the additional required space to 256 bytes on the stack, not 2k on a single stack.

The workqueues are problematic as they may be contended.
They can't be scheduled with top priority either. Also the optimization in bL_switch_request() to skip the workqueue entirely when the target CPU and the calling CPU were the same didn't allow for bL_switch_request() to be called from atomic context, as might be the case for some cpufreq drivers.

Let's move to dedicated kthreads instead.

Signed-off-by Nicolas Pitre <[email protected]>

---
 arch/arm/common/bL_switcher.c      | 101 ++++++++++++++++++++++++++++---------
 arch/arm/include/asm/bL_switcher.h |   2 +-
 2 files changed, 79 insertions(+), 24 deletions(-)

arch/arm/common/bL_switcher.c +59 -23

index d6f7e507d9..c2355cafc9 100644
@@ -15,8 +15,10 @@
 #include <linux/sched.h>
 #include <linux/interrupt.h>
 #include <linux/cpu_pm.h>
+#include <linux/cpu.h>
 #include <linux/cpumask.h>
-#include <linux/workqueue.h>
+#include <linux/kthread.h>
+#include <linux/wait.h>
 #include <linux/clockchips.h>
 #include <linux/hrtimer.h>
 #include <linux/tick.h>

@@ -225,15 +227,48 @@ static int bL_switch_to(unsigned int new_cluster_id)
 	return ret;
 }
 
-struct switch_args {
-	unsigned int cluster;
-	struct work_struct work;
+struct bL_thread {
+	struct task_struct *task;
+	wait_queue_head_t wq;
+	int wanted_cluster;
 };
 
-static void __bL_switch_to(struct work_struct *work)
+static struct bL_thread bL_threads[MAX_CPUS_PER_CLUSTER];
+
+static int bL_switcher_thread(void *arg)
+{
+	struct bL_thread *t = arg;
+	struct sched_param param = { .sched_priority = 1 };
+	int cluster;
+
+	sched_setscheduler_nocheck(current, SCHED_FIFO, &param);
+
+	do {
+		if (signal_pending(current))
+			flush_signals(current);
+		wait_event_interruptible(t->wq,
+				t->wanted_cluster != -1 ||
+				kthread_should_stop());
+		cluster = xchg(&t->wanted_cluster, -1);
+		if (cluster != -1)
+			bL_switch_to(cluster);
+	} while (!kthread_should_stop());
+
+	return 0;
+}
+
+static struct task_struct * __init bL_switcher_thread_create(int cpu, void *arg)
 {
-	struct switch_args *args = container_of(work, struct switch_args, work);
-	bL_switch_to(args->cluster);
+	struct task_struct *task;
+
+	task = kthread_create_on_node(bL_switcher_thread, arg,
+				      cpu_to_node(cpu), "kswitcher_%d", cpu);
+	if (!IS_ERR(task)) {
+		kthread_bind(task, cpu);
+		wake_up_process(task);
+	} else
+		pr_err("%s failed for CPU %d\n", __func__, cpu);
+	return task;
 }
 
 /*

@@ -242,26 +277,46 @@ static void __bL_switch_to(struct work_struct *work)
  * @cpu: the CPU to switch
  * @new_cluster_id: the ID of the cluster to switch to.
  *
- * This function causes a cluster switch on the given CPU.  If the given
- * CPU is the same as the calling CPU then the switch happens right away.
- * Otherwise the request is put on a work queue to be scheduled on the
- * remote CPU.
+ * This function causes a cluster switch on the given CPU by waking up
+ * the appropriate switcher thread.  This function may or may not return
+ * before the switch has occurred.
  */
-void bL_switch_request(unsigned int cpu, unsigned int new_cluster_id)
+int bL_switch_request(unsigned int cpu, unsigned int new_cluster_id)
 {
-	unsigned int this_cpu = get_cpu();
-	struct switch_args args;
+	struct bL_thread *t;
 
-	if (cpu == this_cpu) {
-		bL_switch_to(new_cluster_id);
-		put_cpu();
-		return;
+	if (cpu >= MAX_CPUS_PER_CLUSTER) {
+		pr_err("%s: cpu %d out of bounds\n", __func__, cpu);
+		return -EINVAL;
 	}
-	put_cpu();
 
-	args.cluster = new_cluster_id;
-	INIT_WORK_ONSTACK(&args.work, __bL_switch_to);
-	schedule_work_on(cpu, &args.work);
-	flush_work(&args.work);
+	t = &bL_threads[cpu];
+	if (IS_ERR(t->task))
+		return PTR_ERR(t->task);
+	if (!t->task)
+		return -ESRCH;
+
+	t->wanted_cluster = new_cluster_id;
+	wake_up(&t->wq);
+	return 0;
 }
 EXPORT_SYMBOL_GPL(bL_switch_request);

arch/arm/common/bL_switcher.c +9 -0

+		t->wanted_cluster = -1;
+		t->task = bL_switcher_thread_create(cpu, t);
+	}
+
+	pr_info("big.LITTLE switcher initialized\n");
+	return 0;
+}
+
+late_initcall(bL_switcher_init);

arch/arm/include/asm/bL_switcher.h +0 -0

index 72efe3f349..e0c0bba70b 100644

Quoted 136 lines · Nicolas Pitre

The workqueues are problematic as they may be contended.
They can't be scheduled with top priority either. Also the optimization in bL_switch_request() to skip the workqueue entirely when the target CPU and the calling CPU were the same didn't allow for bL_switch_request() to be called from atomic context, as might be the case for some cpufreq drivers.

Let's move to dedicated kthreads instead.

Signed-off-by Nicolas Pitre <[email protected]>

---
 arch/arm/common/bL_switcher.c      | 101 ++++++++++++++++++++++++++++---------
 arch/arm/include/asm/bL_switcher.h |   2 +-
 2 files changed, 79 insertions(+), 24 deletions(-)

arch/arm/common/bL_switcher.c +65 -23

index d6f7e507d9..c2355cafc9 100644
@@ -15,8 +15,10 @@
 #include <linux/sched.h>
 #include <linux/interrupt.h>
 #include <linux/cpu_pm.h>
+#include <linux/cpu.h>
 #include <linux/cpumask.h>
-#include <linux/workqueue.h>
+#include <linux/kthread.h>
+#include <linux/wait.h>
 #include <linux/clockchips.h>
 #include <linux/hrtimer.h>
 #include <linux/tick.h>

@@ -225,15 +227,48 @@ static int bL_switch_to(unsigned int new_cluster_id)
 	return ret;
 }
 
-struct switch_args {
-	unsigned int cluster;
-	struct work_struct work;
+struct bL_thread {
+	struct task_struct *task;
+	wait_queue_head_t wq;
+	int wanted_cluster;
 };
 
-static void __bL_switch_to(struct work_struct *work)
+static struct bL_thread bL_threads[MAX_CPUS_PER_CLUSTER];
+
+static int bL_switcher_thread(void *arg)
+{
+	struct bL_thread *t = arg;
+	struct sched_param param = { .sched_priority = 1 };
+	int cluster;
+
+	sched_setscheduler_nocheck(current, SCHED_FIFO, &param);
+
+	do {
+		if (signal_pending(current))
+			flush_signals(current);
+		wait_event_interruptible(t->wq,
+				t->wanted_cluster != -1 ||
+				kthread_should_stop());
+		cluster = xchg(&t->wanted_cluster, -1);
+		if (cluster != -1)
+			bL_switch_to(cluster);
+	} while (!kthread_should_stop());
+
+	return 0;
+}
+
+static struct task_struct * __init bL_switcher_thread_create(int cpu, void *arg)
 {
-	struct switch_args *args = container_of(work, struct switch_args, work);
-	bL_switch_to(args->cluster);
+	struct task_struct *task;
+
+	task = kthread_create_on_node(bL_switcher_thread, arg,
+				      cpu_to_node(cpu), "kswitcher_%d", cpu);
+	if (!IS_ERR(task)) {
+		kthread_bind(task, cpu);
+		wake_up_process(task);
+	} else
+		pr_err("%s failed for CPU %d\n", __func__, cpu);
+	return task;
 }
 
 /*

@@ -242,26 +277,46 @@ static void __bL_switch_to(struct work_struct *work)
  * @cpu: the CPU to switch
  * @new_cluster_id: the ID of the cluster to switch to.
  *
- * This function causes a cluster switch on the given CPU.  If the given
- * CPU is the same as the calling CPU then the switch happens right away.
- * Otherwise the request is put on a work queue to be scheduled on the
- * remote CPU.
+ * This function causes a cluster switch on the given CPU by waking up
+ * the appropriate switcher thread.  This function may or may not return
+ * before the switch has occurred.
  */
-void bL_switch_request(unsigned int cpu, unsigned int new_cluster_id)
+int bL_switch_request(unsigned int cpu, unsigned int new_cluster_id)
 {
-	unsigned int this_cpu = get_cpu();
-	struct switch_args args;
+	struct bL_thread *t;
 
-	if (cpu == this_cpu) {
-		bL_switch_to(new_cluster_id);
-		put_cpu();
-		return;
+	if (cpu >= MAX_CPUS_PER_CLUSTER) {
+		pr_err("%s: cpu %d out of bounds\n", __func__, cpu);
+		return -EINVAL;
 	}
-	put_cpu();
 
-	args.cluster = new_cluster_id;
-	INIT_WORK_ONSTACK(&args.work, __bL_switch_to);
-	schedule_work_on(cpu, &args.work);
-	flush_work(&args.work);
+	t = &bL_threads[cpu];
+	if (IS_ERR(t->task))
+		return PTR_ERR(t->task);
+	if (!t->task)
+		return -ESRCH;
+
+	t->wanted_cluster = new_cluster_id;
+	wake_up(&t->wq);
+	return 0;
 }
 EXPORT_SYMBOL_GPL(bL_switch_request);
+
+static int __init bL_switcher_init(void)
+{
+	int cpu;
+
+	pr_info("big.LITTLE switcher initializing\n");