[4/6] ARM: smp: set thread_info->cpu to hardware CPU number for boot thread
diff mbox

Message ID 1312823424-9654-5-git-send-email-will.deacon@arm.com
State New, archived
Headers show

Commit Message

Will Deacon Aug. 8, 2011, 5:10 p.m. UTC
On ARM, Linux assumes that the boot CPU has ID 0. If this ends up being
out of sync with the hardware CPU number, we will configure the GIC
incorrectly and route interrupts to the CPU with hardware ID 0.

This patch implements smp_setup_processor_id for ARM, using the MPIDR to
set the CPU of the boot thread.

Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/kernel/smp.c |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)

Comments

Stephen Boyd Aug. 8, 2011, 5:33 p.m. UTC | #1
On 08/08/2011 10:10 AM, Will Deacon wrote:
>
>  
> +void __init smp_setup_processor_id(void)
> +{
> +	u32 mpidr;
> +
> +	/* Read the MPIDR to find the hardware ID of the current CPU. */
> +	asm("1:		mrc	p15, 0, %0, c0, c0, 5\n"
> +	    "		.pushsection \".alt.smp.init\", \"a\"\n"
> +	    "		.long	1b\n"
> +	    "		mov	%0, #0\n"
> +	    "		.popsection"
> +	    : "=r" (mpidr));

Would it be a good idea to put this into asm/cputype.h? I suppose the
smp alternatives part would need to be written in C.

    mpidr = is_smp() ? read_cpuid_mpidr() : 0;

>
> +
> +	current_thread_info()->cpu = mpidr & 0xff;
> +	printk("Booting Linux on CPU %d\n", current_thread_info()->cpu);
> +}
> +

Should there be a KERN_INFO there?
Will Deacon Aug. 8, 2011, 6:14 p.m. UTC | #2
On Mon, Aug 08, 2011 at 06:33:38PM +0100, Stephen Boyd wrote:
> On 08/08/2011 10:10 AM, Will Deacon wrote:
> >
> >  
> > +void __init smp_setup_processor_id(void)
> > +{
> > +	u32 mpidr;
> > +
> > +	/* Read the MPIDR to find the hardware ID of the current CPU. */
> > +	asm("1:		mrc	p15, 0, %0, c0, c0, 5\n"
> > +	    "		.pushsection \".alt.smp.init\", \"a\"\n"
> > +	    "		.long	1b\n"
> > +	    "		mov	%0, #0\n"
> > +	    "		.popsection"
> > +	    : "=r" (mpidr));
> 
> Would it be a good idea to put this into asm/cputype.h? I suppose the
> smp alternatives part would need to be written in C.
> 
>     mpidr = is_smp() ? read_cpuid_mpidr() : 0;

Yes, that's much better. We used to have asm/smp_mpidr.h which did this, so I
lifted the code from there but doing it in C is easier to read.

> >
> > +
> > +	current_thread_info()->cpu = mpidr & 0xff;
> > +	printk("Booting Linux on CPU %d\n", current_thread_info()->cpu);
> > +}
> > +
> 
> Should there be a KERN_INFO there?

Can be. I just followed suit with secondary_start_kernel, but smp.c doesn't
seem to be consistent anyway so I'll add the specifier.

Thanks for the comments,

Will
Russell King - ARM Linux Aug. 8, 2011, 8:02 p.m. UTC | #3
On Mon, Aug 08, 2011 at 07:14:59PM +0100, Will Deacon wrote:
> On Mon, Aug 08, 2011 at 06:33:38PM +0100, Stephen Boyd wrote:
> > On 08/08/2011 10:10 AM, Will Deacon wrote:
> > >
> > >  
> > > +void __init smp_setup_processor_id(void)
> > > +{
> > > +	u32 mpidr;
> > > +
> > > +	/* Read the MPIDR to find the hardware ID of the current CPU. */
> > > +	asm("1:		mrc	p15, 0, %0, c0, c0, 5\n"
> > > +	    "		.pushsection \".alt.smp.init\", \"a\"\n"
> > > +	    "		.long	1b\n"
> > > +	    "		mov	%0, #0\n"
> > > +	    "		.popsection"
> > > +	    : "=r" (mpidr));
> > 
> > Would it be a good idea to put this into asm/cputype.h? I suppose the
> > smp alternatives part would need to be written in C.
> > 
> >     mpidr = is_smp() ? read_cpuid_mpidr() : 0;
> 
> Yes, that's much better. We used to have asm/smp_mpidr.h which did this, so I
> lifted the code from there but doing it in C is easier to read.
> 
> > >
> > > +
> > > +	current_thread_info()->cpu = mpidr & 0xff;
> > > +	printk("Booting Linux on CPU %d\n", current_thread_info()->cpu);
> > > +}
> > > +
> > 
> > Should there be a KERN_INFO there?
> 
> Can be. I just followed suit with secondary_start_kernel, but smp.c doesn't
> seem to be consistent anyway so I'll add the specifier.

Actually, it's probably much better to keep logical CPU0 as the boot CPU.
The hotplug code makes the assumption that the lowest online CPU number
is the boot CPU.

So unless we want even more pain, I suggest we just ensure logical CPU0
is the boot CPU.

We're already fairly clean on our interactions between CPU numbering and
the hardware CPU numbering - there's only a limited number of places where
the two interact.  Those are the GIC, IPI, secondary CPU bringup, and
hotplug code.  I suggest that we preserve the independence, and introduce
a mapping between logical CPU numbering and hardware CPU numbering.
Will Deacon Aug. 8, 2011, 8:25 p.m. UTC | #4
Hi Russell,

On Mon, Aug 08, 2011 at 09:02:56PM +0100, Russell King - ARM Linux wrote:
> On Mon, Aug 08, 2011 at 07:14:59PM +0100, Will Deacon wrote:
> > Can be. I just followed suit with secondary_start_kernel, but smp.c doesn't
> > seem to be consistent anyway so I'll add the specifier.
> 
> Actually, it's probably much better to keep logical CPU0 as the boot CPU.
> The hotplug code makes the assumption that the lowest online CPU number
> is the boot CPU.

Yes, you're right. I only tested this on a dual-core platform so I didn't
get a chance to play with hotplug (except to see that it failed, like I
expected). That assumption is also in the generic code so I guess it's
easier to leave it be.

> So unless we want even more pain, I suggest we just ensure logical CPU0
> is the boot CPU.

Right. Ensuring the logical CPU number isn't too hard, but we should aim to
support an arbitrary physical CPU number for the boot ID, even if most
platforms can't cater for that at the moment.

> We're already fairly clean on our interactions between CPU numbering and
> the hardware CPU numbering - there's only a limited number of places where
> the two interact.  Those are the GIC, IPI, secondary CPU bringup, and
> hotplug code.  I suggest that we preserve the independence, and introduce
> a mapping between logical CPU numbering and hardware CPU numbering.

This was my initial approach but then I thought I'd try and cheat my messing
with the logical numbering. I'll look at implementing the mapping and then
updating the GIC and friends to indirect their CPU number lookups through
that instead. I'll post it as its own series since it will be more than just
a simple hack now.

Stay tuned...

Cheers,

Will
Sergei Shtylyov Aug. 9, 2011, 11:48 a.m. UTC | #5
Hello.

On 08-08-2011 21:10, Will Deacon wrote:

> On ARM, Linux assumes that the boot CPU has ID 0. If this ends up being
> out of sync with the hardware CPU number, we will configure the GIC
> incorrectly and route interrupts to the CPU with hardware ID 0.

> This patch implements smp_setup_processor_id for ARM, using the MPIDR to
> set the CPU of the boot thread.

> Signed-off-by: Will Deacon<will.deacon@arm.com>
> ---
>   arch/arm/kernel/smp.c |   16 ++++++++++++++++
>   1 files changed, 16 insertions(+), 0 deletions(-)

> diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
> index d88ff02..8e60a4f 100644
> --- a/arch/arm/kernel/smp.c
> +++ b/arch/arm/kernel/smp.c
> @@ -259,6 +259,22 @@ void __ref cpu_die(void)
>   }
>   #endif /* CONFIG_HOTPLUG_CPU */
>
> +void __init smp_setup_processor_id(void)
> +{
> +	u32 mpidr;
> +
> +	/* Read the MPIDR to find the hardware ID of the current CPU. */
> +	asm("1:		mrc	p15, 0, %0, c0, c0, 5\n"
> +	    "		.pushsection \".alt.smp.init\", \"a\"\n"
> +	    "		.long	1b\n"
> +	    "		mov	%0, #0\n"
> +	    "		.popsection"
> +	    : "=r" (mpidr));
> +
> +	current_thread_info()->cpu = mpidr&  0xff;
> +	printk("Booting Linux on CPU %d\n", current_thread_info()->cpu);

    printk() should have the KERN_* logging facility.

WBR, Sergei
Will Deacon Aug. 9, 2011, 12:13 p.m. UTC | #6
On Tue, Aug 09, 2011 at 12:48:05PM +0100, Sergei Shtylyov wrote:
> Hello.
> 
> On 08-08-2011 21:10, Will Deacon wrote:
> 
> > On ARM, Linux assumes that the boot CPU has ID 0. If this ends up being
> > out of sync with the hardware CPU number, we will configure the GIC
> > incorrectly and route interrupts to the CPU with hardware ID 0.
> 
> > This patch implements smp_setup_processor_id for ARM, using the MPIDR to
> > set the CPU of the boot thread.
> 
> > Signed-off-by: Will Deacon<will.deacon@arm.com>
> > ---
> >   arch/arm/kernel/smp.c |   16 ++++++++++++++++
> >   1 files changed, 16 insertions(+), 0 deletions(-)
> 
> > diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
> > index d88ff02..8e60a4f 100644
> > --- a/arch/arm/kernel/smp.c
> > +++ b/arch/arm/kernel/smp.c
> > @@ -259,6 +259,22 @@ void __ref cpu_die(void)
> >   }
> >   #endif /* CONFIG_HOTPLUG_CPU */
> >
> > +void __init smp_setup_processor_id(void)
> > +{
> > +	u32 mpidr;
> > +
> > +	/* Read the MPIDR to find the hardware ID of the current CPU. */
> > +	asm("1:		mrc	p15, 0, %0, c0, c0, 5\n"
> > +	    "		.pushsection \".alt.smp.init\", \"a\"\n"
> > +	    "		.long	1b\n"
> > +	    "		mov	%0, #0\n"
> > +	    "		.popsection"
> > +	    : "=r" (mpidr));
> > +
> > +	current_thread_info()->cpu = mpidr&  0xff;
> > +	printk("Booting Linux on CPU %d\n", current_thread_info()->cpu);
> 
>     printk() should have the KERN_* logging facility.

There's an echo around here :) This patch is pretty much dead anyway after
the discussion with Russell. I'll post some other guys soon.

Will

Patch
diff mbox

diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index d88ff02..8e60a4f 100644
--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c
@@ -259,6 +259,22 @@  void __ref cpu_die(void)
 }
 #endif /* CONFIG_HOTPLUG_CPU */
 
+void __init smp_setup_processor_id(void)
+{
+	u32 mpidr;
+
+	/* Read the MPIDR to find the hardware ID of the current CPU. */
+	asm("1:		mrc	p15, 0, %0, c0, c0, 5\n"
+	    "		.pushsection \".alt.smp.init\", \"a\"\n"
+	    "		.long	1b\n"
+	    "		mov	%0, #0\n"
+	    "		.popsection"
+	    : "=r" (mpidr));
+
+	current_thread_info()->cpu = mpidr & 0xff;
+	printk("Booting Linux on CPU %d\n", current_thread_info()->cpu);
+}
+
 /*
  * Called by both boot and secondaries to move global data into
  * per-processor storage.