some question about arm64 smp_spin_table.c

some question about arm64 smp_spin_table.c

[yoma sophian]

hi all:
in arm64/kernel/smp_spin_table.c
    --> smp_spin_table_prepare_cpu,
we suppose the cpu_release_addr[cpu] is located in kernel logical memory.

in Arm plarform, this part is implement by each platform driver, since
they register its own smp_operations.

And in Arm64, once other platform use different area, such as register
or device memory, to put cpu_release_addr[cpu], shall we use ioremap
to get the va like below patch?

Appreciate your kind in advance,

diff --git a/arch/arm64/kernel/smp_spin_table.c
b/arch/arm64/kernel/smp_spin_table.c
index 7c35fa6..9d945bd 100644
--- a/arch/arm64/kernel/smp_spin_table.c
+++ b/arch/arm64/kernel/smp_spin_table.c
@@ -21,6 +21,7 @@
 #include <linux/smp.h>

 #include <asm/cacheflush.h>
+#include <linux/io.h>

 static phys_addr_t cpu_release_addr[NR_CPUS];

@@ -47,10 +48,9 @@ static int __init smp_spin_table_prepare_cpu(int cpu)
        if (!cpu_release_addr[cpu])
                return -ENODEV;

-       release_addr = __va(cpu_release_addr[cpu]);
+       release_addr = ioremap(cpu_release_addr[cpu], SZ_4K);
        release_addr[0] = (void *)__pa(secondary_holding_pen);
-       __flush_dcache_area(release_addr, sizeof(release_addr[0]));
-
+       iounmap(release_addr);
        /*
         * Send an event to wake up the secondary CPU.
         */

some question about arm64 smp_spin_table.c

[Arnd Bergmann]

On Wednesday 27 May 2015 17:47:09 yoma sophian wrote:
> And in Arm64, once other platform use different area, such as register
> or device memory, to put cpu_release_addr[cpu], shall we use ioremap
> to get the va like below patch?
> 
> @@ -47,10 +48,9 @@ static int __init smp_spin_table_prepare_cpu(int cpu)
>         if (!cpu_release_addr[cpu])
>                 return -ENODEV;
> 
> -       release_addr = __va(cpu_release_addr[cpu]);
> +       release_addr = ioremap(cpu_release_addr[cpu], SZ_4K);
>         release_addr[0] = (void *)__pa(secondary_holding_pen);
> -       __flush_dcache_area(release_addr, sizeof(release_addr[0]));
> -
> +       iounmap(release_addr);
>         /*
> 

I believe that won't work: The other CPU is spinning on its L1 cache,
and with ioremap, you would be bypassing the cache.

Basically, if the CPU uses something other than normal memory, it is
not using the spin-table protocol.

	Arnd

some question about arm64 smp_spin_table.c

[yoma sophian]

hi Arnd:

2015-05-27 18:06 GMT+08:00 Arnd Bergmann <arnd@arndb.de>:
> On Wednesday 27 May 2015 17:47:09 yoma sophian wrote:
>> And in Arm64, once other platform use different area, such as register
>> or device memory, to put cpu_release_addr[cpu], shall we use ioremap
>> to get the va like below patch?
>>
>> @@ -47,10 +48,9 @@ static int __init smp_spin_table_prepare_cpu(int cpu)
>>         if (!cpu_release_addr[cpu])
>>                 return -ENODEV;
>>
>> -       release_addr = __va(cpu_release_addr[cpu]);
>> +       release_addr = ioremap(cpu_release_addr[cpu], SZ_4K);
>>         release_addr[0] = (void *)__pa(secondary_holding_pen);
>> -       __flush_dcache_area(release_addr, sizeof(release_addr[0]));
>> -
>> +       iounmap(release_addr);
>>         /*
>>
>
> I believe that won't work: The other CPU is spinning on its L1 cache,
> and with ioremap, you would be bypassing the cache.
I am not quite understand "The other CPU is spinning on its L1 cache,"
As far as I know,
1. cpu_release_addr[cpu] is where other core spin to
2. Before other cores jump to their kernel entry,
secondary_holding_pen, shouldn't they follow the booting.txt
mentioned:
- Caches, MMUs
  The MMU must be off.
  Instruction cache may be on or off.
  The address range corresponding to the loaded kernel image must be
  cleaned to the PoC. In the presence of a system cache or other
  coherent masters with caches enabled, this will typically require
  cache maintenance by VA rather than set/way operations.

Why you say The other CPU is spinning on its L1 cache?
And even it is it is spinning on its L1 cache,
"The address range corresponding to the loaded kernel image must be
  cleaned to the PoC."

And the other cpu will make sure before polling the spin address
before invalidate it, right?

Appreciate your kind help,

some question about arm64 smp_spin_table.c

[Mark Rutland]

On Wed, May 27, 2015 at 10:47:09AM +0100, yoma sophian wrote:
> hi all:

Hi,

> in arm64/kernel/smp_spin_table.c
>     --> smp_spin_table_prepare_cpu,
> we suppose the cpu_release_addr[cpu] is located in kernel logical memory.
> 
> in Arm plarform, this part is implement by each platform driver, since
> they register its own smp_operations.
> 
> And in Arm64, once other platform use different area, such as register
> or device memory, to put cpu_release_addr[cpu], shall we use ioremap
> to get the va like below patch?

Platforms need to place the release address in memory which can be
mapped with cacheable attributes. That is required by the semantics of
memreserve (which implies a cacheable mapping is fine).

Since commit 113954c6463d1d80 ("arm64: spin-table: handle unmapped
cpu-release-addrs") we allowed for cpu-release-addrs which did not fall
in the linear mapping, by using ioremap_cache. This still requires that
the memory can be mapped cacheable.

As Arnd says, if the cpu-release-addr is not in memory which can be
mapped as cacheable, then you're arguably not implementing spin-table.
There are a whole slew of coherency issues that the current
implementation avoids by being very simple, and I'm not keen about
prospect of changing that.

Is there any particular reason that you do not wish to place your
cpu-release-addrs in memory?

Thanks,
Mark.

> 
> Appreciate your kind in advance,
> 
> diff --git a/arch/arm64/kernel/smp_spin_table.c
> b/arch/arm64/kernel/smp_spin_table.c
> index 7c35fa6..9d945bd 100644
> --- a/arch/arm64/kernel/smp_spin_table.c
> +++ b/arch/arm64/kernel/smp_spin_table.c
> @@ -21,6 +21,7 @@
>  #include <linux/smp.h>
> 
>  #include <asm/cacheflush.h>
> +#include <linux/io.h>
> 
>  static phys_addr_t cpu_release_addr[NR_CPUS];
> 
> @@ -47,10 +48,9 @@ static int __init smp_spin_table_prepare_cpu(int cpu)
>         if (!cpu_release_addr[cpu])
>                 return -ENODEV;
> 
> -       release_addr = __va(cpu_release_addr[cpu]);
> +       release_addr = ioremap(cpu_release_addr[cpu], SZ_4K);
>         release_addr[0] = (void *)__pa(secondary_holding_pen);
> -       __flush_dcache_area(release_addr, sizeof(release_addr[0]));
> -
> +       iounmap(release_addr);
>         /*
>          * Send an event to wake up the secondary CPU.
>          */
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

some question about arm64 smp_spin_table.c

[yoma sophian]

hi Mark:

2015-05-27 18:36 GMT+08:00 Mark Rutland <mark.rutland@arm.com>:
> On Wed, May 27, 2015 at 10:47:09AM +0100, yoma sophian wrote:
>> hi all:
>
> Hi,
>
>> in arm64/kernel/smp_spin_table.c
>>     --> smp_spin_table_prepare_cpu,
>> we suppose the cpu_release_addr[cpu] is located in kernel logical memory.
>>
>> in Arm plarform, this part is implement by each platform driver, since
>> they register its own smp_operations.
>>
>> And in Arm64, once other platform use different area, such as register
>> or device memory, to put cpu_release_addr[cpu], shall we use ioremap
>> to get the va like below patch?
>
> Platforms need to place the release address in memory which can be
> mapped with cacheable attributes. That is required by the semantics of
> memreserve (which implies a cacheable mapping is fine).
>
> Since commit 113954c6463d1d80 ("arm64: spin-table: handle unmapped
> cpu-release-addrs") we allowed for cpu-release-addrs which did not fall
> in the linear mapping, by using ioremap_cache. This still requires that
> the memory can be mapped cacheable.
Oops, I use kernel 3.10 and that is why I didn't see your patch before :)

BTW, as I explained to Arnd, in booting.txt, it didn't mentioned the
cpu-release-addrs has to be cacheable.
why we need the address mapped as cacheable?

> As Arnd says, if the cpu-release-addr is not in memory which can be
> mapped as cacheable, then you're arguably not implementing spin-table.
> There are a whole slew of coherency issues that the current
> implementation avoids by being very simple, and I'm not keen about
> prospect of changing that.
is there any special requirement we need to follow when using spin-table mode?
>
> Is there any particular reason that you do not wish to place your
> cpu-release-addrs in memory?
for not make address with a hole to put spin address and put that area
as seperate power domain.
we use registers for other core spin jumping address.

Appreciate your kind help,

some question about arm64 smp_spin_table.c

[Mark Rutland]

On Wed, May 27, 2015 at 01:24:04PM +0100, yoma sophian wrote:
> hi Mark:
> 
> 2015-05-27 18:36 GMT+08:00 Mark Rutland <mark.rutland@arm.com>:
> > On Wed, May 27, 2015 at 10:47:09AM +0100, yoma sophian wrote:
> >> hi all:
> >
> > Hi,
> >
> >> in arm64/kernel/smp_spin_table.c
> >>     --> smp_spin_table_prepare_cpu,
> >> we suppose the cpu_release_addr[cpu] is located in kernel logical memory.
> >>
> >> in Arm plarform, this part is implement by each platform driver, since
> >> they register its own smp_operations.
> >>
> >> And in Arm64, once other platform use different area, such as register
> >> or device memory, to put cpu_release_addr[cpu], shall we use ioremap
> >> to get the va like below patch?
> >
> > Platforms need to place the release address in memory which can be
> > mapped with cacheable attributes. That is required by the semantics of
> > memreserve (which implies a cacheable mapping is fine).
> >
> > Since commit 113954c6463d1d80 ("arm64: spin-table: handle unmapped
> > cpu-release-addrs") we allowed for cpu-release-addrs which did not fall
> > in the linear mapping, by using ioremap_cache. This still requires that
> > the memory can be mapped cacheable.
> Oops, I use kernel 3.10 and that is why I didn't see your patch before :)
> 
> BTW, as I explained to Arnd, in booting.txt, it didn't mentioned the
> cpu-release-addrs has to be cacheable.

Unfortunately booting.txt misses lots of details that may have been
obvious to those writing it, but not so obvious to others.

> why we need the address mapped as cacheable?

There are a few reasons, including:

* The cpu-release-addr may fall inside the linear mapping, and hence
  there may be a cacheable alias anyway that we have to account for.

* The cpu-release-addr may previously have been mapped with a cacheable
  mapping, so cache maintenance is potentially necessary regardless of
  how the kernel maps the region.

* The CPUs spinning may have mapped the memory with cacheable
  attributes, so cache maintenance is potentially necessary regardless
  of how the kernel maps the memory.

* Any memory described in a /memreserve/ must be available to be mapped
  with cacheable attributes (following on from the rules in ePAPR). The
  original definition of spin-table required that the cpu-release-addr
  fell within memory and was forbidden from general allocation via a
  /memreserve/. The precise set of those attributes is not well-defined
  for the ARM architecture.

Any OS and/or spin-table implementation must function with all of these
constraints in mind.

The easiest way of getting that right from Linux is to map the region
with cacheable attributes and perform cache maintenance. The easiest
way to get this right from the firmware side is to not map memory with
cacheable attributes, and place the cpu-release-addr in main memory,
with an appropriate /memreserve/.

> > As Arnd says, if the cpu-release-addr is not in memory which can be
> > mapped as cacheable, then you're arguably not implementing spin-table.
> > There are a whole slew of coherency issues that the current
> > implementation avoids by being very simple, and I'm not keen about
> > prospect of changing that.
> is there any special requirement we need to follow when using spin-table mode?

I'm not entirely sure what you're asking. Perhaps you're asking about
some of the things I mention above?

> > Is there any particular reason that you do not wish to place your
> > cpu-release-addrs in memory?
> for not make address with a hole to put spin address and put that area
> as seperate power domain.

I don't see why you'd need a separate power domain for the
cpu-release-addr if this were in a (reserved) region of memory. Surely
all of your DRAM is in the same power domain?

> we use registers for other core spin jumping address.

It sound like you either need to make those CPUs poll a cpu-release-addr
in memory, or you could use PSCI instead (which would also give you CPU
hotplug, idle, and so on). Have you looked at the ARM Trusted Firmware?

Thanks,
Mark.