| Message ID | 1406630950-32432-2-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Tue, 2014-07-29 at 12:49 +0200, Ard Biesheuvel wrote: > From: Mark Rutland <mark.rutland@arm.com> > > In certain cases the cpu-release-addr of a CPU may not fall in the > linear mapping (e.g. when the kernel is loaded above this address due to > the presence of other images in memory). This is problematic for the > spin-table code as it assumes that it can trivially convert a > cpu-release-addr to a valid VA in the linear map. > > This patch modifies the spin-table code to use a temporary cached > mapping to write to a given cpu-release-addr, enabling us to support > addresses regardless of whether they are covered by the linear mapping. > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > --- > arch/arm64/kernel/smp_spin_table.c | 21 ++++++++++++++++----- > 1 file changed, 16 insertions(+), 5 deletions(-) > > diff --git a/arch/arm64/kernel/smp_spin_table.c b/arch/arm64/kernel/smp_spin_table.c > index 0347d38eea29..70181c1bf42d 100644 > --- a/arch/arm64/kernel/smp_spin_table.c > +++ b/arch/arm64/kernel/smp_spin_table.c > @@ -20,6 +20,7 @@ > #include <linux/init.h> > #include <linux/of.h> > #include <linux/smp.h> > +#include <linux/types.h> > > #include <asm/cacheflush.h> > #include <asm/cpu_ops.h> > @@ -65,12 +66,21 @@ static int smp_spin_table_cpu_init(struct device_node *dn, unsigned int cpu) > > static int smp_spin_table_cpu_prepare(unsigned int cpu) > { > - void **release_addr; > + __le64 __iomem *release_addr; > > if (!cpu_release_addr[cpu]) > return -ENODEV; > > - release_addr = __va(cpu_release_addr[cpu]); > + /* > + * The cpu-release-addr may or may not be inside the linear mapping. > + * As ioremap_cache will either give us a new mapping or reuse the > + * existing linear mapping, we can use it to cover both cases. In > + * either case the memory will be MT_NORMAL. > + */ > + release_addr = ioremap_cache(cpu_release_addr[cpu], > + sizeof(*release_addr)); > + if (!release_addr) > + return -ENOMEM; > > /* > * We write the release address as LE regardless of the native > @@ -79,15 +89,16 @@ static int smp_spin_table_cpu_prepare(unsigned int cpu) > * boot-loader's endianess before jumping. This is mandated by > * the boot protocol. > */ > - release_addr[0] = (void *) cpu_to_le64(__pa(secondary_holding_pen)); > - > - __flush_dcache_area(release_addr, sizeof(release_addr[0])); > + writeq_relaxed(__pa(secondary_holding_pen), release_addr); > + __flush_dcache_area(release_addr, sizeof(*release_addr)); __flush_dcache_area((__force void *)release_addr, ... to avoid sparse warning. > > /* > * Send an event to wake up the secondary CPU. > */ > sev(); > > + iounmap(release_addr); > + > return 0; > } >
On Tue, 2014-07-29 at 11:15 -0400, Mark Salter wrote: > On Tue, 2014-07-29 at 12:49 +0200, Ard Biesheuvel wrote: > > From: Mark Rutland <mark.rutland@arm.com> > > > > In certain cases the cpu-release-addr of a CPU may not fall in the > > linear mapping (e.g. when the kernel is loaded above this address due to > > the presence of other images in memory). This is problematic for the > > spin-table code as it assumes that it can trivially convert a > > cpu-release-addr to a valid VA in the linear map. > > > > This patch modifies the spin-table code to use a temporary cached > > mapping to write to a given cpu-release-addr, enabling us to support > > addresses regardless of whether they are covered by the linear mapping. > > > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> Oops, forgot: Tested-by: Mark Salter <msalter@redhat.com> > > --- > > arch/arm64/kernel/smp_spin_table.c | 21 ++++++++++++++++----- > > 1 file changed, 16 insertions(+), 5 deletions(-) > > > > diff --git a/arch/arm64/kernel/smp_spin_table.c b/arch/arm64/kernel/smp_spin_table.c > > index 0347d38eea29..70181c1bf42d 100644 > > --- a/arch/arm64/kernel/smp_spin_table.c > > +++ b/arch/arm64/kernel/smp_spin_table.c > > @@ -20,6 +20,7 @@ > > #include <linux/init.h> > > #include <linux/of.h> > > #include <linux/smp.h> > > +#include <linux/types.h> > > > > #include <asm/cacheflush.h> > > #include <asm/cpu_ops.h> > > @@ -65,12 +66,21 @@ static int smp_spin_table_cpu_init(struct device_node *dn, unsigned int cpu) > > > > static int smp_spin_table_cpu_prepare(unsigned int cpu) > > { > > - void **release_addr; > > + __le64 __iomem *release_addr; > > > > if (!cpu_release_addr[cpu]) > > return -ENODEV; > > > > - release_addr = __va(cpu_release_addr[cpu]); > > + /* > > + * The cpu-release-addr may or may not be inside the linear mapping. > > + * As ioremap_cache will either give us a new mapping or reuse the > > + * existing linear mapping, we can use it to cover both cases. In > > + * either case the memory will be MT_NORMAL. > > + */ > > + release_addr = ioremap_cache(cpu_release_addr[cpu], > > + sizeof(*release_addr)); > > + if (!release_addr) > > + return -ENOMEM; > > > > /* > > * We write the release address as LE regardless of the native > > @@ -79,15 +89,16 @@ static int smp_spin_table_cpu_prepare(unsigned int cpu) > > * boot-loader's endianess before jumping. This is mandated by > > * the boot protocol. > > */ > > - release_addr[0] = (void *) cpu_to_le64(__pa(secondary_holding_pen)); > > - > > - __flush_dcache_area(release_addr, sizeof(release_addr[0])); > > + writeq_relaxed(__pa(secondary_holding_pen), release_addr); > > + __flush_dcache_area(release_addr, sizeof(*release_addr)); > > __flush_dcache_area((__force void *)release_addr, ... > > to avoid sparse warning. > > > > > /* > > * Send an event to wake up the secondary CPU. > > */ > > sev(); > > > > + iounmap(release_addr); > > + > > return 0; > > } > > >
On Tuesday 29 July 2014 11:15:45 Mark Salter wrote: > > - > > - __flush_dcache_area(release_addr, sizeof(release_addr[0])); > > + writeq_relaxed(__pa(secondary_holding_pen), release_addr); > > + __flush_dcache_area(release_addr, sizeof(*release_addr)); > > __flush_dcache_area((__force void *)release_addr, ... > > to avoid sparse warning. > I think it would be cleaner to drop the __iomem annotation and use vmap() rather than ioremap(). That requires having a 'struct page' though, which I'm not sure you have. Arnd
On Tue, 2014-07-29 at 17:20 +0200, Arnd Bergmann wrote: > On Tuesday 29 July 2014 11:15:45 Mark Salter wrote: > > > - > > > - __flush_dcache_area(release_addr, sizeof(release_addr[0])); > > > + writeq_relaxed(__pa(secondary_holding_pen), release_addr); > > > + __flush_dcache_area(release_addr, sizeof(*release_addr)); > > > > __flush_dcache_area((__force void *)release_addr, ... > > > > to avoid sparse warning. > > > > I think it would be cleaner to drop the __iomem annotation and use vmap() > rather than ioremap(). That requires having a 'struct page' though, which > I'm not sure you have. > You won't. If you did have a struct page, then __va() would work.
On Tuesday 29 July 2014 11:30:24 Mark Salter wrote: > On Tue, 2014-07-29 at 17:20 +0200, Arnd Bergmann wrote: > > On Tuesday 29 July 2014 11:15:45 Mark Salter wrote: > > > > - > > > > - __flush_dcache_area(release_addr, sizeof(release_addr[0])); > > > > + writeq_relaxed(__pa(secondary_holding_pen), release_addr); > > > > + __flush_dcache_area(release_addr, sizeof(*release_addr)); > > > > > > __flush_dcache_area((__force void *)release_addr, ... > > > > > > to avoid sparse warning. > > > > > > > I think it would be cleaner to drop the __iomem annotation and use vmap() > > rather than ioremap(). That requires having a 'struct page' though, which > > I'm not sure you have. > > > > You won't. If you did have a struct page, then __va() would work. Ah, right. I was thinking of highmem, which has a struct page but no virtual address. However, on arm64 there is obviously no highmem. Arnd
On Tue, Jul 29, 2014 at 04:20:46PM +0100, Arnd Bergmann wrote: > On Tuesday 29 July 2014 11:15:45 Mark Salter wrote: > > > - > > > - __flush_dcache_area(release_addr, sizeof(release_addr[0])); > > > + writeq_relaxed(__pa(secondary_holding_pen), release_addr); > > > + __flush_dcache_area(release_addr, sizeof(*release_addr)); > > > > __flush_dcache_area((__force void *)release_addr, ... > > > > to avoid sparse warning. Presumably we'd get this for the write_relaxed too? > > > > I think it would be cleaner to drop the __iomem annotation and use vmap() > rather than ioremap(). That requires having a 'struct page' though, which > I'm not sure you have. As far as I am aware, we'd only have a struct page for memory falling in the linear map, so for the cases this patch is actually required we wouldn't have a struct page. So it looks like I should just make release_addr a void __iomem *. Then this line can just be: __flush_dcache_area(release_addr, 8); Where we could replace 8 with sizeof(u64), sizeof(__le64), etc if 8 is too magic. How does that sound? Thanks, Mark.
On Tuesday 29 July 2014 17:03:03 Mark Rutland wrote: > On Tue, Jul 29, 2014 at 04:20:46PM +0100, Arnd Bergmann wrote: > > On Tuesday 29 July 2014 11:15:45 Mark Salter wrote: > > > > - > > > > - __flush_dcache_area(release_addr, sizeof(release_addr[0])); > > > > + writeq_relaxed(__pa(secondary_holding_pen), release_addr); > > > > + __flush_dcache_area(release_addr, sizeof(*release_addr)); > > > > > > __flush_dcache_area((__force void *)release_addr, ... > > > > > > to avoid sparse warning. > > Presumably we'd get this for the write_relaxed too? writeq_relaxed() actually expects an __iomem pointer > > > > I think it would be cleaner to drop the __iomem annotation and use vmap() > > rather than ioremap(). That requires having a 'struct page' though, which > > I'm not sure you have. > > As far as I am aware, we'd only have a struct page for memory falling in > the linear map, so for the cases this patch is actually required we > wouldn't have a struct page. > > So it looks like I should just make release_addr a void __iomem *. Then > this line can just be: You mean make it a 'void *' instead of 'void __iomem *', right? > __flush_dcache_area(release_addr, 8); > > Where we could replace 8 with sizeof(u64), sizeof(__le64), etc if 8 is > too magic. > > How does that sound? Not sure where you're getting at. Using a regular pointer sounds fine, but then you have to cast the result of ioremap and do a manual cpu_to_le64 conversion on the assignment. Keeping the iomem annotation will also work,a nd then we only need the cast in the __flush_dcache_area call. Arnd
On Tue, Jul 29, 2014 at 05:13:07PM +0100, Arnd Bergmann wrote: > On Tuesday 29 July 2014 17:03:03 Mark Rutland wrote: > > On Tue, Jul 29, 2014 at 04:20:46PM +0100, Arnd Bergmann wrote: > > > On Tuesday 29 July 2014 11:15:45 Mark Salter wrote: > > > > > - > > > > > - __flush_dcache_area(release_addr, sizeof(release_addr[0])); > > > > > + writeq_relaxed(__pa(secondary_holding_pen), release_addr); > > > > > + __flush_dcache_area(release_addr, sizeof(*release_addr)); > > > > > > > > __flush_dcache_area((__force void *)release_addr, ... > > > > > > > > to avoid sparse warning. > > > > Presumably we'd get this for the write_relaxed too? > > writeq_relaxed() actually expects an __iomem pointer Ah. I was being thick and thought this was about the underlying type of release_addr (le64 * vs void *) rather than the __iomem annotation. > > > > > > I think it would be cleaner to drop the __iomem annotation and use vmap() > > > rather than ioremap(). That requires having a 'struct page' though, which > > > I'm not sure you have. > > > > As far as I am aware, we'd only have a struct page for memory falling in > > the linear map, so for the cases this patch is actually required we > > wouldn't have a struct page. > > > > So it looks like I should just make release_addr a void __iomem *. Then > > this line can just be: > > You mean make it a 'void *' instead of 'void __iomem *', right? > > > __flush_dcache_area(release_addr, 8); > > > > Where we could replace 8 with sizeof(u64), sizeof(__le64), etc if 8 is > > too magic. > > > > How does that sound? > > Not sure where you're getting at. Using a regular pointer sounds fine, > but then you have to cast the result of ioremap and do a manual > cpu_to_le64 conversion on the assignment. > > Keeping the iomem annotation will also work,a nd then we only need > the cast in the __flush_dcache_area call. Sorry, I'd misunderstood the problem and my suggestion was nonsense deriving from that. Having the (__force void *) cast in the call to __flush_dcache_area sounds like the right solution to me. Thanks, Mark.
On Tuesday 29 July 2014 17:18:52 Mark Rutland wrote: > Sorry, I'd misunderstood the problem and my suggestion was nonsense > deriving from that. > > Having the (__force void *) cast in the call to __flush_dcache_area > sounds like the right solution to me. Well, there isn't really a good solution here I think, because you are not really dealing with an iomem pointer after all, just something a bit like that, but we don't have a 'memremap' function to do what you really mean. Arnd
diff --git a/arch/arm64/kernel/smp_spin_table.c b/arch/arm64/kernel/smp_spin_table.c index 0347d38eea29..70181c1bf42d 100644 --- a/arch/arm64/kernel/smp_spin_table.c +++ b/arch/arm64/kernel/smp_spin_table.c @@ -20,6 +20,7 @@ #include <linux/init.h> #include <linux/of.h> #include <linux/smp.h> +#include <linux/types.h> #include <asm/cacheflush.h> #include <asm/cpu_ops.h> @@ -65,12 +66,21 @@ static int smp_spin_table_cpu_init(struct device_node *dn, unsigned int cpu) static int smp_spin_table_cpu_prepare(unsigned int cpu) { - void **release_addr; + __le64 __iomem *release_addr; if (!cpu_release_addr[cpu]) return -ENODEV; - release_addr = __va(cpu_release_addr[cpu]); + /* + * The cpu-release-addr may or may not be inside the linear mapping. + * As ioremap_cache will either give us a new mapping or reuse the + * existing linear mapping, we can use it to cover both cases. In + * either case the memory will be MT_NORMAL. + */ + release_addr = ioremap_cache(cpu_release_addr[cpu], + sizeof(*release_addr)); + if (!release_addr) + return -ENOMEM; /* * We write the release address as LE regardless of the native @@ -79,15 +89,16 @@ static int smp_spin_table_cpu_prepare(unsigned int cpu) * boot-loader's endianess before jumping. This is mandated by * the boot protocol. */ - release_addr[0] = (void *) cpu_to_le64(__pa(secondary_holding_pen)); - - __flush_dcache_area(release_addr, sizeof(release_addr[0])); + writeq_relaxed(__pa(secondary_holding_pen), release_addr); + __flush_dcache_area(release_addr, sizeof(*release_addr)); /* * Send an event to wake up the secondary CPU. */ sev(); + iounmap(release_addr); + return 0; }