mach-spear SMP questions

mach-spear SMP questions

[Alex Elder]

I'm doing some code cleanup and have a question about
something related to SMP in mach-spear.  Can anyone
help me with these questions?

My question has to do with how secondary cores start for
SMP.  Most machines need to send a wakeup event from the
boot core to each secondary core as it is told to start.
But mach-spear does not do this.  I'd like to know if this
is an oversight, or confirm that this is simply not necessary
for this machine.

Also, while most machines touch a memory location and then
do a targeted cache involving only that location, mach-spear
does a full flush.  Is the full flush really required for
some reason?

Thanks.

					-Alex

mach-spear SMP questions

[Russell King - ARM Linux]

On Thu, Mar 27, 2014 at 08:33:38AM -0500, Alex Elder wrote:
> I'm doing some code cleanup and have a question about
> something related to SMP in mach-spear.  Can anyone
> help me with these questions?

I think you're working in a similar area to that which I have been - but
with different ends in mind.  Have a look for my L2 cache series on LAKML.

> My question has to do with how secondary cores start for
> SMP.  Most machines need to send a wakeup event from the
> boot core to each secondary core as it is told to start.
> But mach-spear does not do this.  I'd like to know if this
> is an oversight, or confirm that this is simply not necessary
> for this machine.

It could be that the "BootMonitor" waits for spear13xx_smp_prepare_cpus()
to write the address of the function to jump to into SYS_LOCATION - I can't
see any other way this code would work (there's nothing else there to tell
the secondary CPUs where to start executing from or when to start executing
from that point.)

> Also, while most machines touch a memory location and then
> do a targeted cache involving only that location, mach-spear
> does a full flush.  Is the full flush really required for
> some reason?

It's not required.  As part of the L2 cache cleanup, I have this
change:

Author: Russell King <rmk+kernel@arm.linux.org.uk>
Date:   Sat Mar 15 16:47:47 2014 +0000

    ARM: l2c: avoid calling outer_flush_all() unnecessarily (Spear)

    Spear calls outer_flush_all() from it's SMP bringup function.  This
    is potentially dangerous as the L2C set/way operations which implement
    this don't take kindly to concurrent operations.  Besides, there's
    better solutions to this, as implemented on other platforms.

    Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>

@@ -20,6 +20,18 @@
+/*
+ * Write pen_release in a way that is guaranteed to be visible to all
+ * observers, irrespective of whether they're taking part in coherency
+ * or not.  This is necessary for the hotplug code to work reliably.
+ */
+static void write_pen_release(int val)
+{
+       pen_release = val;
+       smp_wmb();
+       sync_cache_w(&pen_release);
+}
+
...
@@ -30,8 +42,7 @@ static void spear13xx_secondary_init(unsigned int cpu)
-       pen_release = -1;
-       smp_wmb();
+       write_pen_release(-1);
...
@@ -58,9 +69,7 @@ static int spear13xx_boot_secondary(unsigned int cpu, struct t
-       pen_release = cpu;
-       flush_cache_all();
-       outer_flush_all();
+       write_pen_release(cpu);

for Spear, which makes it behave like every other platform we have using
this pen release method.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

mach-spear SMP questions

[Viresh Kumar]

Fixing Shiraz's id and Cc'ing Vipin as well..

On Thu, Mar 27, 2014 at 7:03 PM, Alex Elder <elder@linaro.org> wrote:
> I'm doing some code cleanup and have a question about
> something related to SMP in mach-spear.  Can anyone
> help me with these questions?
>
> My question has to do with how secondary cores start for
> SMP.  Most machines need to send a wakeup event from the
> boot core to each secondary core as it is told to start.
> But mach-spear does not do this.  I'd like to know if this
> is an oversight, or confirm that this is simply not necessary
> for this machine.
>
> Also, while most machines touch a memory location and then
> do a targeted cache involving only that location, mach-spear
> does a full flush.  Is the full flush really required for
> some reason?
>
> Thanks.
>
>                                         -Alex
>

mach-spear SMP questions

[Alex Elder]

On 03/27/2014 09:39 AM, Russell King - ARM Linux wrote:
> On Thu, Mar 27, 2014 at 08:33:38AM -0500, Alex Elder wrote:
>> I'm doing some code cleanup and have a question about
>> something related to SMP in mach-spear.  Can anyone
>> help me with these questions?
> 
> I think you're working in a similar area to that which I have been - but
> with different ends in mind.  Have a look for my L2 cache series on LAKML.

By "different ends in mind," do you mean conflicting
goals, or simply for different reasons?

>> My question has to do with how secondary cores start for
>> SMP.  Most machines need to send a wakeup event from the
>> boot core to each secondary core as it is told to start.
>> But mach-spear does not do this.  I'd like to know if this
>> is an oversight, or confirm that this is simply not necessary
>> for this machine.
> 
> It could be that the "BootMonitor" waits for spear13xx_smp_prepare_cpus()
> to write the address of the function to jump to into SYS_LOCATION - I can't
> see any other way this code would work (there's nothing else there to tell
> the secondary CPUs where to start executing from or when to start executing
> from that point.)

That could be.

My next question is whether sending a wakeup event would
be *harmful*.  If it's not, this machine can use the
common pen-release code I'm consolidating things to use.

>> Also, while most machines touch a memory location and then
>> do a targeted cache involving only that location, mach-spear
>> does a full flush.  Is the full flush really required for
>> some reason?
> 
> It's not required.  As part of the L2 cache cleanup, I have this
> change:

That's perfect.  Just like all other implementations
(so I can factor it out).  Now I just need to know
whether I can use the shared the smp_boot_secondary
method.

Thanks.

					-Alex

> Author: Russell King <rmk+kernel@arm.linux.org.uk>
> Date:   Sat Mar 15 16:47:47 2014 +0000
> 
>     ARM: l2c: avoid calling outer_flush_all() unnecessarily (Spear)
> 
>     Spear calls outer_flush_all() from it's SMP bringup function.  This
>     is potentially dangerous as the L2C set/way operations which implement
>     this don't take kindly to concurrent operations.  Besides, there's
>     better solutions to this, as implemented on other platforms.
> 
>     Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> 
> @@ -20,6 +20,18 @@
> +/*
> + * Write pen_release in a way that is guaranteed to be visible to all
> + * observers, irrespective of whether they're taking part in coherency
> + * or not.  This is necessary for the hotplug code to work reliably.
> + */
> +static void write_pen_release(int val)
> +{
> +       pen_release = val;
> +       smp_wmb();
> +       sync_cache_w(&pen_release);
> +}
> +
> ...
> @@ -30,8 +42,7 @@ static void spear13xx_secondary_init(unsigned int cpu)
> -       pen_release = -1;
> -       smp_wmb();
> +       write_pen_release(-1);
> ...
> @@ -58,9 +69,7 @@ static int spear13xx_boot_secondary(unsigned int cpu, struct t
> -       pen_release = cpu;
> -       flush_cache_all();
> -       outer_flush_all();
> +       write_pen_release(cpu);
> 
> for Spear, which makes it behave like every other platform we have using
> this pen release method.
> 

mach-spear SMP questions

[Russell King - ARM Linux]

On Thu, Mar 27, 2014 at 11:56:44AM -0500, Alex Elder wrote:
> On 03/27/2014 09:39 AM, Russell King - ARM Linux wrote:
> > On Thu, Mar 27, 2014 at 08:33:38AM -0500, Alex Elder wrote:
> >> I'm doing some code cleanup and have a question about
> >> something related to SMP in mach-spear.  Can anyone
> >> help me with these questions?
> > 
> > I think you're working in a similar area to that which I have been - but
> > with different ends in mind.  Have a look for my L2 cache series on LAKML.
> 
> By "different ends in mind," do you mean conflicting
> goals, or simply for different reasons?

Hmm, "L2 cache series" doesn't suggest anything?  The L2 cache series is...
a series cleaning up the L2 cache code, which includes removing incorrect
uses of outer_flush_all().

Merely removing that call would likely break Spear, so it had to be fixed
properly.

> >> My question has to do with how secondary cores start for
> >> SMP.  Most machines need to send a wakeup event from the
> >> boot core to each secondary core as it is told to start.
> >> But mach-spear does not do this.  I'd like to know if this
> >> is an oversight, or confirm that this is simply not necessary
> >> for this machine.
> > 
> > It could be that the "BootMonitor" waits for spear13xx_smp_prepare_cpus()
> > to write the address of the function to jump to into SYS_LOCATION - I can't
> > see any other way this code would work (there's nothing else there to tell
> > the secondary CPUs where to start executing from or when to start executing
> > from that point.)
> 
> That could be.
> 
> My next question is whether sending a wakeup event would
> be *harmful*.  If it's not, this machine can use the
> common pen-release code I'm consolidating things to use.

I've seen this attempt many times before... you're not the first.
Somehow, saying "no" to something never sticks.  It just postpones it
until someone else thinks it's a good idea and tries the same thing
again. :(

> >> Also, while most machines touch a memory location and then
> >> do a targeted cache involving only that location, mach-spear
> >> does a full flush.  Is the full flush really required for
> >> some reason?
> > 
> > It's not required.  As part of the L2 cache cleanup, I have this
> > change:
> 
> That's perfect.  Just like all other implementations
> (so I can factor it out).  Now I just need to know
> whether I can use the shared the smp_boot_secondary
> method.

People keep trying to do this, and I keep saying no to it because (as I
keep stating each time this comes up) the majority of implementations
haven't /thought/ about their hotplug implementations at all, and have
just gone down the path of "let's copy-n-paste the first solution we
find" which has historically been that used on the ARM Realview
platform.

The problem with that is the ARM Realview is a really dumb piece of
hardware: it has no control over the secondary CPUs.  When it powers
up, all CPUs come out of reset simultaneously, and enter the boot
monitor.

The boot monitor then holds the secondary CPUs itself, allowing the
boot CPU to run the rest of the boot monitor code, eventually falling
through to the boot loader and ultimately to Linux.

Linux then sets the address of the CPU holding pen code, and pokes the
boot monitor to release the secondary CPUs to that holding place - it
results in all three secondary CPUs ending up there simultaneously.
The holding pen's purpose is to hold the CPUs there while Linux
continues the boot process to the point of requesting each CPU
individually to come up.

The secondary function of the pen release code is to deal with hotplug
on this platform: once a secondary CPU is released into the kernel code,
there is no way to take it back out of the kernel.  So, the CPU hotplug
code has to "fake" taking a CPU offline.  It does this by taking the
CPU out of coherency, and putting it into a low power state, waiting
for an event to wake it back up.  That wakeup event is based upon the
pen thing again selecting the appropriate CPU to "plug" back in.

This approach also has the problem that it is /not/ kexec friendly -
since we can't get rid of the secondary CPUs, a kexec'd kernel ends up
simply overwriting the code that the secondary CPUs happened to be
executing at the time, probably crashing them.  In any case, the
secondary CPUs will not come back up after a kexec since they'll probably
have crashed.

In the case of a system where the secondary CPUs can be held in reset
independently of the boot CPU, that hardware should be used when bringing
up or taking down a secondary CPU.  If you have that facility, then there
is absolutely no reason to use the "pen" stuff.

I consider any platform which copies the Realview SMP code to be broken
by definition.  I do not wish this code to be turned into some kind of
generic helper, because by doing so, we're endorsing it as a proper way
to do something.  It is *not* a proper way to implement this.  It's the
implementation used by ARM Ltd's platforms for the sole reason that this
hardware has no form of controls on the secondary CPUs what so ever.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

mach-spear SMP questions

[Alex Elder]

On 03/27/2014 12:19 PM, Russell King - ARM Linux wrote:
> On Thu, Mar 27, 2014 at 11:56:44AM -0500, Alex Elder wrote:
>> On 03/27/2014 09:39 AM, Russell King - ARM Linux wrote:
>>> On Thu, Mar 27, 2014 at 08:33:38AM -0500, Alex Elder wrote:
>>>> I'm doing some code cleanup and have a question about
>>>> something related to SMP in mach-spear.  Can anyone
>>>> help me with these questions?
>>>
>>> I think you're working in a similar area to that which I have been - but
>>> with different ends in mind.  Have a look for my L2 cache series on LAKML.
>>
>> By "different ends in mind," do you mean conflicting
>> goals, or simply for different reasons?
> 
> Hmm, "L2 cache series" doesn't suggest anything?  The L2 cache series is...
> a series cleaning up the L2 cache code, which includes removing incorrect
> uses of outer_flush_all().

I merely wanted to be clarify your meaning.

> Merely removing that call would likely break Spear, so it had to be fixed
> properly.
> 
>>>> My question has to do with how secondary cores start for
>>>> SMP.  Most machines need to send a wakeup event from the
>>>> boot core to each secondary core as it is told to start.
>>>> But mach-spear does not do this.  I'd like to know if this
>>>> is an oversight, or confirm that this is simply not necessary
>>>> for this machine.
>>>
>>> It could be that the "BootMonitor" waits for spear13xx_smp_prepare_cpus()
>>> to write the address of the function to jump to into SYS_LOCATION - I can't
>>> see any other way this code would work (there's nothing else there to tell
>>> the secondary CPUs where to start executing from or when to start executing
>>> from that point.)
>>
>> That could be.
>>
>> My next question is whether sending a wakeup event would
>> be *harmful*.  If it's not, this machine can use the
>> common pen-release code I'm consolidating things to use.
> 
> I've seen this attempt many times before... you're not the first.
> Somehow, saying "no" to something never sticks.  It just postpones it
> until someone else thinks it's a good idea and tries the same thing
> again. :(

Maybe this time will be better?  :)

>>>> Also, while most machines touch a memory location and then
>>>> do a targeted cache involving only that location, mach-spear
>>>> does a full flush.  Is the full flush really required for
>>>> some reason?
>>>
>>> It's not required.  As part of the L2 cache cleanup, I have this
>>> change:
>>
>> That's perfect.  Just like all other implementations
>> (so I can factor it out).  Now I just need to know
>> whether I can use the shared the smp_boot_secondary
>> method.
> 
> People keep trying to do this, and I keep saying no to it because (as I
> keep stating each time this comes up) the majority of implementations
> haven't /thought/ about their hotplug implementations at all, and have
> just gone down the path of "let's copy-n-paste the first solution we
> find" which has historically been that used on the ARM Realview
> platform.

It looks that way to me.  But how do you stop people from
spreading the copy/paste disease if there's no obvious
alternative--or if it appears that it's what "everybody
does?"

What I really am trying to do is get to the point where we
can specify an "enable-method" in the device tree.  To do
that I looked at how these various platforms were starting
their secondary cores and they're almost all the same "pen"
method.  And obvious first step is to factor out the common
code.

> The problem with that is the ARM Realview is a really dumb piece of
> hardware: it has no control over the secondary CPUs.  When it powers
> up, all CPUs come out of reset simultaneously, and enter the boot
> monitor.

> The boot monitor then holds the secondary CPUs itself, allowing the
> boot CPU to run the rest of the boot monitor code, eventually falling
> through to the boot loader and ultimately to Linux.
> 
> Linux then sets the address of the CPU holding pen code, and pokes the
> boot monitor to release the secondary CPUs to that holding place - it
> results in all three secondary CPUs ending up there simultaneously.
> The holding pen's purpose is to hold the CPUs there while Linux
> continues the boot process to the point of requesting each CPU
> individually to come up.

Yes, that's exactly how it works.

> The secondary function of the pen release code is to deal with hotplug
> on this platform: once a secondary CPU is released into the kernel code,
> there is no way to take it back out of the kernel.  So, the CPU hotplug
> code has to "fake" taking a CPU offline.  It does this by taking the
> CPU out of coherency, and putting it into a low power state, waiting
> for an event to wake it back up.  That wakeup event is based upon the
> pen thing again selecting the appropriate CPU to "plug" back in.

At this point I have not been looking at its use in hotplug.

> This approach also has the problem that it is /not/ kexec friendly -
> since we can't get rid of the secondary CPUs, a kexec'd kernel ends up
> simply overwriting the code that the secondary CPUs happened to be
> executing at the time, probably crashing them.  In any case, the
> secondary CPUs will not come back up after a kexec since they'll probably
> have crashed.

Why is this even used then?  (Sorry, like I said, I've been
looking at SMP startup only.)

> In the case of a system where the secondary CPUs can be held in reset
> independently of the boot CPU, that hardware should be used when bringing
> up or taking down a secondary CPU.  If you have that facility, then there
> is absolutely no reason to use the "pen" stuff.

And what if there is no such facility?

> I consider any platform which copies the Realview SMP code to be broken
> by definition.  I do not wish this code to be turned into some kind of
> generic helper, because by doing so, we're endorsing it as a proper way
> to do something.  It is *not* a proper way to implement this.  It's the
> implementation used by ARM Ltd's platforms for the sole reason that this
> hardware has no form of controls on the secondary CPUs what so ever.

I have no ability to change how these machines control their
secondary CPUs.  Nor do I have knowledge of whether they
have the ability to independently hold secondary cores in
reset.  Even if they did, I don't have the information I'd
require to implement such a thing so they do it the right
way.

What I *can* do is identify a half dozen essentially identical
implementations of code littering the code base and consolidate
them into a single implementation.  In doing so I will not change
their behavior at all (unpleasant as it may be), but I will
improve the code overall.  It will make it transparent that
these are in fact all doing the same thing, and I hope it will
lead to it becoming easier to implement SMP startup in a better
way for all implementations if/when there is such a thing.
As I said above, my broader aim is to support a device-tree
specified way of booting secondary cores, and in doing so
I'm trying to be consistent with how it's being done for ARM64.

I'm not endorsing this way of starting secondary cores.  But
looking at all these similar (but not quite identical) blocks
of code makes it harder than it should be to get a handle on
what's going on, and suggests that maybe they *must* all
have custom implementations.

I understand your point that having a common set of
functions implies an endorsement of that way of doing
things.  But I'd argue that everyone using a shared
implementation (even if it's the wrong way) is better
than everyone *copying* the same implementation.  People
can be pressured at review time to do it differently if
there is a better way available.

I think everyone knows that duplicated code is bad, and
that's why I'm not the first one to set out to do this.
I'm about half done and I find that it adds about 120
new lines of common code but removes 340 lines that were
duplicated (including 5 files completely going away).
All of this duplicated code is under the arch/arm/mach-*
directories, by the way, so this helps the goal of
doing away with that stuff as well.

Even if "everybody is doing it wrong," I feel pretty
strongly that the net effect of consolidating is positive.

I hope you'll reconsider your "no."  I'll have the patches
available soon (to allow an objective look at the outcome).

					-Alex