* [PATCH 1/2] perf/powerpc/hv-24x7: Use per-cpu page buffer
From: Sukadev Bhattiprolu @ 2014-12-10 7:06 UTC (permalink / raw)
To: Michael Ellerman
Cc: linuxppc-dev, dev, Jiri Olsa, linux-kernel,
Arnaldo Carvalho de Melo
>From 470c16c8955672103a9529c78dffbb239e9e27b8 Mon Sep 17 00:00:00 2001
From: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
Date: Tue, 9 Dec 2014 22:17:46 -0500
Subject: [PATCH 1/2] perf/poweprc/hv-24x7: Use per-cpu page buffer
The 24x7 counters are continuously running and not updated on an interrupt.
So we record the event counts when stopping the event or deleting it.
But to "read" a single counter in 24x7, we allocate a page and pass it
into the hypervisor (The HV returns the page full of counters from which
we extract the specific counter for this event).
We allocate a page using GFP_USER and when deleting the event, we end up
with the following warning because we are blocking in interrupt context.
[ 698.641709] BUG: scheduling while atomic: swapper/0/0/0x10010000
We could use GFP_ATOMIC but that could result in failures. Pre-allocate
a buffer so we don't have to allocate in interrupt context. Further as
Michael Ellerman suggested, use Per-CPU buffer so we only need to allocate
once per CPU.
Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
---
arch/powerpc/perf/hv-24x7.c | 21 +++++++++------------
1 file changed, 9 insertions(+), 12 deletions(-)
diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
index dba3408..18e1f49 100644
--- a/arch/powerpc/perf/hv-24x7.c
+++ b/arch/powerpc/perf/hv-24x7.c
@@ -217,11 +217,14 @@ static bool is_physical_domain(int domain)
domain == HV_24X7_PERF_DOMAIN_PHYSICAL_CORE;
}
+DEFINE_PER_CPU(char, hv_24x7_reqb[4096]);
+DEFINE_PER_CPU(char, hv_24x7_resb[4096]);
+
static unsigned long single_24x7_request(u8 domain, u32 offset, u16 ix,
u16 lpar, u64 *res,
bool success_expected)
{
- unsigned long ret = -ENOMEM;
+ unsigned long ret;
/*
* request_buffer and result_buffer are not required to be 4k aligned,
@@ -243,13 +246,11 @@ static unsigned long single_24x7_request(u8 domain, u32 offset, u16 ix,
BUILD_BUG_ON(sizeof(*request_buffer) > 4096);
BUILD_BUG_ON(sizeof(*result_buffer) > 4096);
- request_buffer = kmem_cache_zalloc(hv_page_cache, GFP_USER);
- if (!request_buffer)
- goto out;
+ request_buffer = (void *)get_cpu_var(hv_24x7_reqb);
+ result_buffer = (void *)get_cpu_var(hv_24x7_resb);
- result_buffer = kmem_cache_zalloc(hv_page_cache, GFP_USER);
- if (!result_buffer)
- goto out_free_request_buffer;
+ memset(request_buffer, 0, 4096);
+ memset(result_buffer, 0, 4096);
*request_buffer = (struct reqb) {
.buf = {
@@ -278,15 +279,11 @@ static unsigned long single_24x7_request(u8 domain, u32 offset, u16 ix,
domain, offset, ix, lpar, ret, ret,
result_buffer->buf.detailed_rc,
result_buffer->buf.failing_request_ix);
- goto out_free_result_buffer;
+ goto out;
}
*res = be64_to_cpu(result_buffer->result);
-out_free_result_buffer:
- kfree(result_buffer);
-out_free_request_buffer:
- kfree(request_buffer);
out:
return ret;
}
--
1.8.3.1
^ permalink raw reply related
* [PATCH 2/2] perf/power/hv-24x7: Use kmem_cache_free() instead of kfree
From: Sukadev Bhattiprolu @ 2014-12-10 7:09 UTC (permalink / raw)
To: Michael Ellerman
Cc: linuxppc-dev, dev, Jiri Olsa, linux-kernel,
Arnaldo Carvalho de Melo
In-Reply-To: <20141210070637.GA27377@us.ibm.com>
>From 982bf5a660ae33fbe2f9187187caa6752c66783d Mon Sep 17 00:00:00 2001
From: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
Date: Wed, 10 Dec 2014 01:43:34 -0500
Subject: [PATCH 2/2] power/perf: hv-24x7: Use kmem_cache_free() instead of
kfree
Use kmem_cache_free() to free a buffer allocated with kmem_cache_alloc().
Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
p# set2.log
---
arch/powerpc/perf/hv-24x7.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
index 18e1f49..a2c6211 100644
--- a/arch/powerpc/perf/hv-24x7.c
+++ b/arch/powerpc/perf/hv-24x7.c
@@ -177,7 +177,7 @@ static ssize_t _name##_show(struct device *dev, \
} \
ret = sprintf(buf, _fmt, _expr); \
e_free: \
- kfree(page); \
+ kmem_cache_free(hv_page_cache, page); \
return ret; \
} \
static DEVICE_ATTR_RO(_name)
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH 1/2] perf/powerpc/hv-24x7: Use per-cpu page buffer
From: Michael Ellerman @ 2014-12-10 9:09 UTC (permalink / raw)
To: Sukadev Bhattiprolu
Cc: linuxppc-dev, Jiri Olsa, dev, linux-kernel,
Arnaldo Carvalho de Melo
In-Reply-To: <20141210070637.GA27377@us.ibm.com>
On Tue, 2014-12-09 at 23:06 -0800, Sukadev Bhattiprolu wrote:
> From 470c16c8955672103a9529c78dffbb239e9e27b8 Mon Sep 17 00:00:00 2001
> From: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
> Date: Tue, 9 Dec 2014 22:17:46 -0500
> Subject: [PATCH 1/2] perf/poweprc/hv-24x7: Use per-cpu page buffer
>
> diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
> index dba3408..18e1f49 100644
> --- a/arch/powerpc/perf/hv-24x7.c
> +++ b/arch/powerpc/perf/hv-24x7.c
> @@ -217,11 +217,14 @@ static bool is_physical_domain(int domain)
> domain == HV_24X7_PERF_DOMAIN_PHYSICAL_CORE;
> }
>
> +DEFINE_PER_CPU(char, hv_24x7_reqb[4096]);
> +DEFINE_PER_CPU(char, hv_24x7_resb[4096]);
Do we need it to be 4K aligned also? I would guess so.
Rather than declaring these as char arrays and then casting below, can you pull
the struct definitions up and then declare the per cpu variables with the
proper type.
> static unsigned long single_24x7_request(u8 domain, u32 offset, u16 ix,
> u16 lpar, u64 *res,
> bool success_expected)
> {
> - unsigned long ret = -ENOMEM;
> + unsigned long ret;
>
> /*
> * request_buffer and result_buffer are not required to be 4k aligned,
> @@ -243,13 +246,11 @@ static unsigned long single_24x7_request(u8 domain, u32 offset, u16 ix,
> BUILD_BUG_ON(sizeof(*request_buffer) > 4096);
> BUILD_BUG_ON(sizeof(*result_buffer) > 4096);
>
> - request_buffer = kmem_cache_zalloc(hv_page_cache, GFP_USER);
> - if (!request_buffer)
> - goto out;
> + request_buffer = (void *)get_cpu_var(hv_24x7_reqb);
> + result_buffer = (void *)get_cpu_var(hv_24x7_resb);
>
> - result_buffer = kmem_cache_zalloc(hv_page_cache, GFP_USER);
> - if (!result_buffer)
> - goto out_free_request_buffer;
> + memset(request_buffer, 0, 4096);
> + memset(result_buffer, 0, 4096);
Do we have to memset them? That's not going to speed things up.
cheers
^ permalink raw reply
* Re: [PATCH 2/2] perf/power/hv-24x7: Use kmem_cache_free() instead of kfree
From: Michael Ellerman @ 2014-12-10 9:11 UTC (permalink / raw)
To: Sukadev Bhattiprolu
Cc: linuxppc-dev, Jiri Olsa, dev, linux-kernel,
Arnaldo Carvalho de Melo
In-Reply-To: <20141210070917.GB27377@us.ibm.com>
On Tue, 2014-12-09 at 23:09 -0800, Sukadev Bhattiprolu wrote:
> From 982bf5a660ae33fbe2f9187187caa6752c66783d Mon Sep 17 00:00:00 2001
> From: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
> Date: Wed, 10 Dec 2014 01:43:34 -0500
> Subject: [PATCH 2/2] power/perf: hv-24x7: Use kmem_cache_free() instead of
> kfree
This got a bit munged, ...
> Use kmem_cache_free() to free a buffer allocated with kmem_cache_alloc().
>
> Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
>
> p# set2.log
.. and here too.
I can fix it up this time.
cheers
^ permalink raw reply
* Re: [RFC PATCH 2/8] perf probe powerpc: Fix symbol fixup issues due to ELF type
From: Naveen N. Rao @ 2014-12-10 9:35 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo; +Cc: linuxppc-dev, linux-kernel
In-Reply-To: <20141209210717.GA8788@kernel.org>
On 2014/12/09 06:07PM, Arnaldo Carvalho de Melo wrote:
> Em Tue, Dec 09, 2014 at 11:04:00PM +0530, Naveen N. Rao escreveu:
> > If using the symbol table, symbol addresses are not being fixed up
> > properly, resulting in probes being placed at wrong addresses:
> >
> > # perf probe do_fork
> > Added new event:
> > probe:do_fork (on do_fork)
> >
> > You can now use it in all perf tools, such as:
> >
> > perf record -e probe:do_fork -aR sleep 1
> >
> > # cat /sys/kernel/debug/tracing/kprobe_events
> > p:probe/do_fork _text+635952
> > # printf "%x" 635952
> > 9b430
> > # grep do_fork /boot/System.map
> > c0000000000ab430 T .do_fork
> >
> > Fix by checking for ELF type ET_DYN used by ppc64 kernels.
>
> Are you sure this doesn't need to be enclosed in ifdef PPC64?
I felt this change is architecture-independent, though I'm actually not
sure if there are other architectures using ET_DYN for their kernel. I
can restrict this to ppc64 if you think that would be better.
- Naveen
>
> - Arnaldo
>
> > Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> > ---
> > tools/perf/util/symbol-elf.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
> > index 1e23a5b..67e4392 100644
> > --- a/tools/perf/util/symbol-elf.c
> > +++ b/tools/perf/util/symbol-elf.c
> > @@ -629,7 +629,8 @@ int symsrc__init(struct symsrc *ss, struct dso *dso, const char *name,
> > NULL) != NULL);
> > } else {
> > ss->adjust_symbols = ehdr.e_type == ET_EXEC ||
> > - ehdr.e_type == ET_REL;
> > + ehdr.e_type == ET_REL ||
> > + ehdr.e_type == ET_DYN;
> > }
> >
> > ss->name = strdup(name);
> > --
> > 2.1.3
>
^ permalink raw reply
* Re: [RFC PATCH 1/8] kprobes: Fix kallsyms lookup across powerpc ABIv1 and ABIv2
From: Michael Ellerman @ 2014-12-10 9:37 UTC (permalink / raw)
To: Naveen N. Rao; +Cc: linuxppc-dev, linux-kernel, acme
In-Reply-To: <7a96a9c262a818112955da93f8ff1ada1bfe5c59.1418146300.git.naveen.n.rao@linux.vnet.ibm.com>
On Tue, 2014-12-09 at 23:03 +0530, Naveen N. Rao wrote:
> Currently, all non-dot symbols are being treated as function descriptors
> in ABIv1. This is incorrect and is resulting in perf probe not working:
I don't understand that first sentence. With ABIv1 non-dot symbols *are*
function descriptors?
> # perf probe do_fork
> Added new event:
> Failed to write event: Invalid argument
> Error: Failed to add events.
> # dmesg | tail -1
> [192268.073063] Could not insert probe at _text+768432: -22
>
> _text is being resolved incorrectly and is resulting in the above error.
> Fix this by changing how we lookup symbol addresses on ppc64. We first
> check for the dot variant of a symbol and look at the non-dot variant
> only if that fails. In this manner, we avoid having to look at the
> function descriptor.
I'm not clear that ppc_local_function_entry() makes sense. On ABIv2 you return
the local entry point, which is fine. But on ABIv1 you just return the
unmodified address, which will be the descriptor if you actually passed it a
function pointer. I think you're assuming that you're passed the text address,
but if that's the case the function is badly named at least.
I also don't understand why we need to ever guess which ABI we're using. We
know which ABI we're built with, so there should be no guess work required.
So at the very least this needs much more explanation.
But to be honest I'm not clear why it even needs a kernel change, don't we just
need perf to understand dot symbols?
cheers
^ permalink raw reply
* Re: [RFC PATCH 2/8] perf probe powerpc: Fix symbol fixup issues due to ELF type
From: Michael Ellerman @ 2014-12-10 9:50 UTC (permalink / raw)
To: Naveen N. Rao; +Cc: linuxppc-dev, linux-kernel, acme
In-Reply-To: <0412a00009a22762f92c208964df6f842796464e.1418146300.git.naveen.n.rao@linux.vnet.ibm.com>
On Tue, 2014-12-09 at 23:04 +0530, Naveen N. Rao wrote:
> If using the symbol table, symbol addresses are not being fixed up
> properly, resulting in probes being placed at wrong addresses:
>
> # perf probe do_fork
> Added new event:
> probe:do_fork (on do_fork)
>
> You can now use it in all perf tools, such as:
>
> perf record -e probe:do_fork -aR sleep 1
>
> # cat /sys/kernel/debug/tracing/kprobe_events
> p:probe/do_fork _text+635952
> # printf "%x" 635952
> 9b430
> # grep do_fork /boot/System.map
> c0000000000ab430 T .do_fork
OK, but why is that happening? And why does checking for ET_DYN fix it?
> Fix by checking for ELF type ET_DYN used by ppc64 kernels.
We sometimes produce ET_DYN kernels, but only if CONFIG_RELOCATABLE=y.
cheers
^ permalink raw reply
* Re: [RFC PATCH 3/8] perf probe: Improve detection of file/function name in the probe pattern
From: Michael Ellerman @ 2014-12-10 10:00 UTC (permalink / raw)
To: Naveen N. Rao; +Cc: linuxppc-dev, linux-kernel, acme
In-Reply-To: <de6c0cd8236de1cabc179e456514c2a399cbc7b5.1418146300.git.naveen.n.rao@linux.vnet.ibm.com>
On Tue, 2014-12-09 at 23:04 +0530, Naveen N. Rao wrote:
> Currently, perf probe considers patterns including a '.' to be a file.
> However, this causes problems on powerpc ABIv1 where all functions have
> a leading '.':
>
> $ perf probe -F | grep schedule_timeout_interruptible
> .schedule_timeout_interruptible
> $ perf probe .schedule_timeout_interruptible
> Semantic error :File always requires line number or lazy pattern.
> Error: Command Parse Error.
>
> Fix this by checking the probe pattern in more detail.
>
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
> tools/perf/util/probe-event.c | 23 ++++++++++++++++++++---
> 1 file changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index c150ca4..c7e01ef 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -999,6 +999,24 @@ static int parse_perf_probe_point(char *arg, struct perf_probe_event *pev)
> arg = tmp;
> }
>
> + /*
> + * Check arg is function or file name and copy it.
> + *
> + * We consider arg to be a file spec if and only if it satisfies
> + * all of the below criteria::
> + * - it does not include any of "+@%",
> + * - it includes one of ":;", and
> + * - it has a period '.' in the name.
I don't think we need to be this elaborate.
AFAIK there are no source files in the kernel that start with '.'
So if the arg starts with '.' it must be a function?
cheers
^ permalink raw reply
* Re: [RFC PATCH 4/8] perf probe powerpc: Handle powerpc dot symbols
From: Michael Ellerman @ 2014-12-10 10:01 UTC (permalink / raw)
To: Naveen N. Rao; +Cc: linuxppc-dev, linux-kernel, acme
In-Reply-To: <6e97fb2cf77d91057fdcda0a75c58cfa63a7313c.1418146300.git.naveen.n.rao@linux.vnet.ibm.com>
On Tue, 2014-12-09 at 23:04 +0530, Naveen N. Rao wrote:
> Fix up various perf aspects related to ppc64's usage of dot functions:
> - ignore leading '.' when generating event names and when looking for
> existing events.
> - use the proper prefix when ignoring SyS symbol lookups.
>
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index c7e01ef..d465f7c 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -2080,6 +2080,10 @@ static int get_new_event_name(char *buf, size_t len, const char *base,
> {
> int i, ret;
>
> + /* Skip the leading dot on powerpc */
> + if (*base == '.')
> + base++;
> +
> /* Try no suffix */
> ret = e_snprintf(buf, len, "%s", base);
> if (ret < 0) {
> @@ -2538,6 +2542,10 @@ int del_perf_probe_events(struct strlist *dellist)
> event = str;
> }
>
> + /* Skip the leading dot on powerpc */
> + if (event && *event == '.')
> + event++;
I'll defer to the perf guys, but I think you want these abstracted in an
architecture specific helper.
> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index 0783311..cc04475 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -137,6 +137,12 @@ static int choose_best_symbol(struct symbol *syma, struct symbol *symb)
> if (na >= 10 && !strncmp(syma->name, "compat_SyS", 10))
> return SYMBOL_B;
>
> + /* On powerpc, ignore the dot variants */
> + if (na >= 4 && !strncmp(syma->name, ".SyS", 4))
> + return SYMBOL_B;
> + if (na >= 11 && !strncmp(syma->name, ".compat_SyS", 11))
> + return SYMBOL_B;
And possibly this too.
cheers
^ permalink raw reply
* Re: [RFC PATCH 5/8] perf probe powerpc: Allow matching against dot symbols
From: Michael Ellerman @ 2014-12-10 10:03 UTC (permalink / raw)
To: Naveen N. Rao; +Cc: linuxppc-dev, linux-kernel, acme
In-Reply-To: <d23252a0bb8a34b2eac9049b34d674aa2d6a2b6a.1418146300.git.naveen.n.rao@linux.vnet.ibm.com>
On Tue, 2014-12-09 at 23:04 +0530, Naveen N. Rao wrote:
> Allow perf probe to work on powerpc ABIv1 without the need to specify the
> leading dot '.' for functions. 'perf probe do_fork' works with this patch.
>
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
> tools/perf/util/probe-event.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index d465f7c..174c22e 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -2221,6 +2221,15 @@ static int probe_function_filter(struct map *map __maybe_unused,
> num_matched_functions++;
> return 0;
> }
> +#ifdef __powerpc64__
> + /* Allow matching against the dot variant */
> + if (sym->name[0] == '.' && looking_function_name[0] != '.' &&
> + (sym->binding == STB_GLOBAL || sym->binding == STB_LOCAL) &&
> + strcmp(looking_function_name, sym->name+1) == 0) {
> + num_matched_functions++;
> + return 0;
> + }
> +#endif
As for the previous patch, I think this should be in an arch helper.
cheers
^ permalink raw reply
* Re: [RFC PATCH 6/8] perf tools powerpc: Fix PPC64 ELF ABIv2 symbol decoding
From: Michael Ellerman @ 2014-12-10 10:13 UTC (permalink / raw)
To: Naveen N. Rao; +Cc: linuxppc-dev, linux-kernel, acme
In-Reply-To: <fc380ddad57d90d338ea79f319c5f5d1dd6639b9.1418146300.git.naveen.n.rao@linux.vnet.ibm.com>
On Tue, 2014-12-09 at 23:04 +0530, Naveen N. Rao wrote:
> PPC64 ELF ABIv2 has a Global Entry Point (GEP) and a Local Entry Point
> (LEP). For purposes of probing, we need the LEP. Offset to the LEP is
> encoded in st_other.
>
> diff --git a/tools/perf/arch/powerpc/util/elf-sym-decode.c b/tools/perf/arch/powerpc/util/elf-sym-decode.c
> new file mode 100644
> index 0000000..7434656
> --- /dev/null
> +++ b/tools/perf/arch/powerpc/util/elf-sym-decode.c
> @@ -0,0 +1,27 @@
> +/*
> + * Decode offset from Global Entry Point to Local Entry Point on PPC64
> + * ELF ABIv2.
> + *
> + * Derived from definitions in arch/powerpc/kernel/module_64.c
> + *
> + * Copyright (C) 2014 Ananth N Mavinakayanahalli, IBM Corporation.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + */
> +
> +#include <gelf.h>
> +#include "elf_sym.h"
> +
> +/* PowerPC64 ABIv2 specific values for the ELF64_Sym st_other field. */
> +#define STO_PPC64_LOCAL_BIT 5
> +#define STO_PPC64_LOCAL_MASK (7 << STO_PPC64_LOCAL_BIT)
> +#define PPC64_LOCAL_ENTRY_OFFSET(other) \
> + (((1 << (((other) & STO_PPC64_LOCAL_MASK) >> STO_PPC64_LOCAL_BIT)) >> 2) << 2)
You're in userspace, you should be able to get these from elf.h
> +unsigned int arch_elf_sym_decode_offset(GElf_Sym *sym)
> +{
> + return PPC64_LOCAL_ENTRY_OFFSET(sym->st_other);
What happens on ABIv1 ? We hope st_other is zero?
> diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
> index 67e4392..92a424e 100644
> --- a/tools/perf/util/symbol-elf.c
> +++ b/tools/perf/util/symbol-elf.c
> @@ -10,6 +10,7 @@
> #include "vdso.h"
> #include <symbol/kallsyms.h>
> #include "debug.h"
> +#include "elf_sym.h"
>
> #ifndef HAVE_ELF_GETPHDRNUM_SUPPORT
> static int elf_getphdrnum(Elf *elf, size_t *dst)
> @@ -848,6 +849,13 @@ int dso__load_sym(struct dso *dso, struct map *map,
> (sym.st_value & 1))
> --sym.st_value;
>
> + /*
> + * PPC64 ELF ABIv2 encodes Local Entry Point offset in
> + * the st_other field
> + */
> + if ((map->type == MAP__FUNCTION) && sym.st_other)
> + sym.st_value += arch_elf_sym_decode_offset(&sym);
I guess no other arch has needed to do anything like this.
But if they did it's unlikely they'll want to do the exact same logic, ie.
check st_other and add some value to st_value. To make it more generically
useful you could just make it:
> + if (map->type == MAP__FUNCTION)
> + arch_elf_sym_decode(&sym);
And do any other checks in the arch routine.
cheers
^ permalink raw reply
* Re: [RFC PATCH 7/8] perf probe powerpc: Use DWARF info only if necessary
From: Michael Ellerman @ 2014-12-10 10:17 UTC (permalink / raw)
To: Naveen N. Rao; +Cc: linuxppc-dev, linux-kernel, acme
In-Reply-To: <50dd0fedb50e3a56ff8f25c44098db3e2a2d6374.1418146300.git.naveen.n.rao@linux.vnet.ibm.com>
On Tue, 2014-12-09 at 23:04 +0530, Naveen N. Rao wrote:
> Use symbol table lookups by default if DWARF is not necessary, since
> powerpc ABIv2 encodes local entry points in the symbol table and the
> function entry address in DWARF may not be appropriate for kprobes,
> as described here:
> https://sourceware.org/bugzilla/show_bug.cgi?id=17638
Needs a better changelog.
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index 174c22e..adcdbd2 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -2382,6 +2382,14 @@ static int convert_to_probe_trace_events(struct perf_probe_event *pev,
> }
> }
>
> +#if defined(__powerpc64__) && defined(_CALL_ELF) && _CALL_ELF == 2
> + if (!perf_probe_event_need_dwarf(pev)) {
> + ret = find_probe_trace_events_from_map(pev, tevs, max_tevs, target);
> + if (ret > 0)
> + return ret; /* Found in symbol table */
> + }
> +#endif
And should be in an arch helper, not a big powerpc wart dropped in the middle
of the generic code.
cheers
^ permalink raw reply
* Re: [RFC PATCH 1/8] kprobes: Fix kallsyms lookup across powerpc ABIv1 and ABIv2
From: Naveen N. Rao @ 2014-12-10 10:26 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev, linux-kernel, acme
In-Reply-To: <1418204242.9279.4.camel@ellerman.id.au>
On 2014/12/10 08:37PM, Michael Ellerman wrote:
> On Tue, 2014-12-09 at 23:03 +0530, Naveen N. Rao wrote:
> > Currently, all non-dot symbols are being treated as function descriptors
> > in ABIv1. This is incorrect and is resulting in perf probe not working:
>
> I don't understand that first sentence. With ABIv1 non-dot symbols *are*
> function descriptors?
Not always. '_text' is an example of a symbol that is not a function
descriptor. However, most functions have a dot variant constituting the
actual entry point and a non-dot variant constituting the function
descriptor.
>
> > # perf probe do_fork
> > Added new event:
> > Failed to write event: Invalid argument
> > Error: Failed to add events.
> > # dmesg | tail -1
> > [192268.073063] Could not insert probe at _text+768432: -22
> >
> > _text is being resolved incorrectly and is resulting in the above error.
> > Fix this by changing how we lookup symbol addresses on ppc64. We first
> > check for the dot variant of a symbol and look at the non-dot variant
> > only if that fails. In this manner, we avoid having to look at the
> > function descriptor.
>
> I'm not clear that ppc_local_function_entry() makes sense. On ABIv2 you return
> the local entry point, which is fine. But on ABIv1 you just return the
> unmodified address, which will be the descriptor if you actually passed it a
> function pointer. I think you're assuming that you're passed the text address,
> but if that's the case the function is badly named at least.
>
> I also don't understand why we need to ever guess which ABI we're using. We
> know which ABI we're built with, so there should be no guess work required.
>
> So at the very least this needs much more explanation.
>
> But to be honest I'm not clear why it even needs a kernel change, don't we just
> need perf to understand dot symbols?
The problem in this case is in the kernel. perf probe is now basing all
probe addresses on _text and writes, for example, "p:probe/do_fork
_text+768432" to /sys/kernel/debug/tracing/kprobe_events.
This ends up in kprobe_lookup_name() for resolving address of _text,
which invokes ppc_function_entry(), which ends up thinking _text is a
function descriptor.
Even though we know we are compiled for ABIv1, there is no easy way to
identify if a given symbol is the actual entry point or if it is a
function descriptor. To address this, my approach is to always check for
a dot symbol first and if that exists, we know we have the actual
function entry. If not, we know this isn't a function descriptor (since
there is no related dot symbol).
I agree that the function is named badly though. The real problem is
that kprobe_lookup_name is a macro and I can't have a #ifdef to call
ppc_function_entry() only for ABIv2.
Thoughts? Suggestions?
Thanks,
Naveen
^ permalink raw reply
* Re: [RFC PATCH 2/8] perf probe powerpc: Fix symbol fixup issues due to ELF type
From: Naveen N. Rao @ 2014-12-10 10:41 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev, linux-kernel, acme
In-Reply-To: <1418205050.9279.5.camel@ellerman.id.au>
On 2014/12/10 08:50PM, Michael Ellerman wrote:
> On Tue, 2014-12-09 at 23:04 +0530, Naveen N. Rao wrote:
> > If using the symbol table, symbol addresses are not being fixed up
> > properly, resulting in probes being placed at wrong addresses:
> >
> > # perf probe do_fork
> > Added new event:
> > probe:do_fork (on do_fork)
> >
> > You can now use it in all perf tools, such as:
> >
> > perf record -e probe:do_fork -aR sleep 1
> >
> > # cat /sys/kernel/debug/tracing/kprobe_events
> > p:probe/do_fork _text+635952
> > # printf "%x" 635952
> > 9b430
> > # grep do_fork /boot/System.map
> > c0000000000ab430 T .do_fork
>
> OK, but why is that happening? And why does checking for ET_DYN fix it?
The section header indicates 0x10000 as the offset:
Section Headers:
[Nr] Name Type Address Offset
Size EntSize Flags Link Info Align
[ 0] NULL 0000000000000000 00000000
0000000000000000 0000000000000000 0 0 0
[ 1] .text PROGBITS c000000000000000 00010000
0000000000806678 0000000000000000 AX 0 0 256
This is used during fixup and perf only expects this to be needed for
ET_EXEC, though we use ET_DYN on ppc64.
> > Fix by checking for ELF type ET_DYN used by ppc64 kernels.
>
> We sometimes produce ET_DYN kernels, but only if CONFIG_RELOCATABLE=y.
Ok.
- Naveen
^ permalink raw reply
* Re: [RFC PATCH 3/8] perf probe: Improve detection of file/function name in the probe pattern
From: Naveen N. Rao @ 2014-12-10 10:59 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev, linux-kernel, acme
In-Reply-To: <1418205606.9279.6.camel@ellerman.id.au>
On 2014/12/10 09:00PM, Michael Ellerman wrote:
> On Tue, 2014-12-09 at 23:04 +0530, Naveen N. Rao wrote:
> > Currently, perf probe considers patterns including a '.' to be a file.
> > However, this causes problems on powerpc ABIv1 where all functions have
> > a leading '.':
> >
> > $ perf probe -F | grep schedule_timeout_interruptible
> > .schedule_timeout_interruptible
> > $ perf probe .schedule_timeout_interruptible
> > Semantic error :File always requires line number or lazy pattern.
> > Error: Command Parse Error.
> >
> > Fix this by checking the probe pattern in more detail.
> >
> > Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> > ---
> > tools/perf/util/probe-event.c | 23 ++++++++++++++++++++---
> > 1 file changed, 20 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> > index c150ca4..c7e01ef 100644
> > --- a/tools/perf/util/probe-event.c
> > +++ b/tools/perf/util/probe-event.c
> > @@ -999,6 +999,24 @@ static int parse_perf_probe_point(char *arg, struct perf_probe_event *pev)
> > arg = tmp;
> > }
> >
> > + /*
> > + * Check arg is function or file name and copy it.
> > + *
> > + * We consider arg to be a file spec if and only if it satisfies
> > + * all of the below criteria::
> > + * - it does not include any of "+@%",
> > + * - it includes one of ":;", and
> > + * - it has a period '.' in the name.
>
> I don't think we need to be this elaborate.
>
> AFAIK there are no source files in the kernel that start with '.'
>
> So if the arg starts with '.' it must be a function?
Indeed, but this is also used for parsing uprobes. So, I coded this
based on the spec for the probe pattern.
- Naveen
^ permalink raw reply
* Re: [RFC PATCH 3/8] perf probe: Improve detection of file/function name in the probe pattern
From: Michael Ellerman @ 2014-12-10 11:12 UTC (permalink / raw)
To: Naveen N. Rao; +Cc: linuxppc-dev, linux-kernel, acme
In-Reply-To: <20141210105900.GC16045@naverao1-tp.in.ibm.com>
On Wed, 2014-12-10 at 16:29 +0530, Naveen N. Rao wrote:
> On 2014/12/10 09:00PM, Michael Ellerman wrote:
> > On Tue, 2014-12-09 at 23:04 +0530, Naveen N. Rao wrote:
> > > Currently, perf probe considers patterns including a '.' to be a file.
> > > However, this causes problems on powerpc ABIv1 where all functions have
> > > a leading '.':
> > >
> > > $ perf probe -F | grep schedule_timeout_interruptible
> > > .schedule_timeout_interruptible
> > > $ perf probe .schedule_timeout_interruptible
> > > Semantic error :File always requires line number or lazy pattern.
> > > Error: Command Parse Error.
> > >
> > > Fix this by checking the probe pattern in more detail.
> > >
> > > Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> > > ---
> > > tools/perf/util/probe-event.c | 23 ++++++++++++++++++++---
> > > 1 file changed, 20 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> > > index c150ca4..c7e01ef 100644
> > > --- a/tools/perf/util/probe-event.c
> > > +++ b/tools/perf/util/probe-event.c
> > > @@ -999,6 +999,24 @@ static int parse_perf_probe_point(char *arg, struct perf_probe_event *pev)
> > > arg = tmp;
> > > }
> > >
> > > + /*
> > > + * Check arg is function or file name and copy it.
> > > + *
> > > + * We consider arg to be a file spec if and only if it satisfies
> > > + * all of the below criteria::
> > > + * - it does not include any of "+@%",
> > > + * - it includes one of ":;", and
> > > + * - it has a period '.' in the name.
> >
> > I don't think we need to be this elaborate.
> >
> > AFAIK there are no source files in the kernel that start with '.'
> >
> > So if the arg starts with '.' it must be a function?
>
> Indeed, but this is also used for parsing uprobes. So, I coded this
> based on the spec for the probe pattern.
OK. It also seems unlikely you'll want a uprobe on a file that starts with a . ?
I'll leave it up to the perf guys to decide if they're happy with it.
cheers
^ permalink raw reply
* Re: [RFC PATCH 6/8] perf tools powerpc: Fix PPC64 ELF ABIv2 symbol decoding
From: Naveen N. Rao @ 2014-12-10 11:21 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev, linux-kernel, acme
In-Reply-To: <1418206413.9279.9.camel@ellerman.id.au>
On 2014/12/10 09:13PM, Michael Ellerman wrote:
> On Tue, 2014-12-09 at 23:04 +0530, Naveen N. Rao wrote:
> > PPC64 ELF ABIv2 has a Global Entry Point (GEP) and a Local Entry Point
> > (LEP). For purposes of probing, we need the LEP. Offset to the LEP is
> > encoded in st_other.
> >
> > diff --git a/tools/perf/arch/powerpc/util/elf-sym-decode.c b/tools/perf/arch/powerpc/util/elf-sym-decode.c
> > new file mode 100644
> > index 0000000..7434656
> > --- /dev/null
> > +++ b/tools/perf/arch/powerpc/util/elf-sym-decode.c
> > @@ -0,0 +1,27 @@
> > +/*
> > + * Decode offset from Global Entry Point to Local Entry Point on PPC64
> > + * ELF ABIv2.
> > + *
> > + * Derived from definitions in arch/powerpc/kernel/module_64.c
> > + *
> > + * Copyright (C) 2014 Ananth N Mavinakayanahalli, IBM Corporation.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; either version
> > + * 2 of the License, or (at your option) any later version.
> > + */
> > +
> > +#include <gelf.h>
> > +#include "elf_sym.h"
> > +
> > +/* PowerPC64 ABIv2 specific values for the ELF64_Sym st_other field. */
> > +#define STO_PPC64_LOCAL_BIT 5
> > +#define STO_PPC64_LOCAL_MASK (7 << STO_PPC64_LOCAL_BIT)
> > +#define PPC64_LOCAL_ENTRY_OFFSET(other) \
> > + (((1 << (((other) & STO_PPC64_LOCAL_MASK) >> STO_PPC64_LOCAL_BIT)) >> 2) << 2)
>
> You're in userspace, you should be able to get these from elf.h
Ah, ok.
>
> > +unsigned int arch_elf_sym_decode_offset(GElf_Sym *sym)
> > +{
> > + return PPC64_LOCAL_ENTRY_OFFSET(sym->st_other);
>
> What happens on ABIv1 ? We hope st_other is zero?
Yes, st_other is zero in ABIv1.
>
> > diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
> > index 67e4392..92a424e 100644
> > --- a/tools/perf/util/symbol-elf.c
> > +++ b/tools/perf/util/symbol-elf.c
> > @@ -10,6 +10,7 @@
> > #include "vdso.h"
> > #include <symbol/kallsyms.h>
> > #include "debug.h"
> > +#include "elf_sym.h"
> >
> > #ifndef HAVE_ELF_GETPHDRNUM_SUPPORT
> > static int elf_getphdrnum(Elf *elf, size_t *dst)
> > @@ -848,6 +849,13 @@ int dso__load_sym(struct dso *dso, struct map *map,
> > (sym.st_value & 1))
> > --sym.st_value;
> >
> > + /*
> > + * PPC64 ELF ABIv2 encodes Local Entry Point offset in
> > + * the st_other field
> > + */
> > + if ((map->type == MAP__FUNCTION) && sym.st_other)
> > + sym.st_value += arch_elf_sym_decode_offset(&sym);
>
> I guess no other arch has needed to do anything like this.
>
> But if they did it's unlikely they'll want to do the exact same logic, ie.
> check st_other and add some value to st_value. To make it more generically
> useful you could just make it:
>
> > + if (map->type == MAP__FUNCTION)
> > + arch_elf_sym_decode(&sym);
>
> And do any other checks in the arch routine.
Sure. Makes sense.
- Naveen
^ permalink raw reply
* Re: [RFC PATCH 7/8] perf probe powerpc: Use DWARF info only if necessary
From: Naveen N. Rao @ 2014-12-10 11:48 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev, linux-kernel, acme
In-Reply-To: <1418206640.9279.10.camel@ellerman.id.au>
On 2014/12/10 09:17PM, Michael Ellerman wrote:
> On Tue, 2014-12-09 at 23:04 +0530, Naveen N. Rao wrote:
> > Use symbol table lookups by default if DWARF is not necessary, since
> > powerpc ABIv2 encodes local entry points in the symbol table and the
> > function entry address in DWARF may not be appropriate for kprobes,
> > as described here:
> > https://sourceware.org/bugzilla/show_bug.cgi?id=17638
>
> Needs a better changelog.
Ok. Will add, but to elaborate quickly: DWARF will only include the
entire function in low_pc/entry_pc and high_pc. It can't indicate the
local entry point. Hence, we need to use the symbol table instead of
DWARF on ABIv2.
>
> > diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> > index 174c22e..adcdbd2 100644
> > --- a/tools/perf/util/probe-event.c
> > +++ b/tools/perf/util/probe-event.c
> > @@ -2382,6 +2382,14 @@ static int convert_to_probe_trace_events(struct perf_probe_event *pev,
> > }
> > }
> >
> > +#if defined(__powerpc64__) && defined(_CALL_ELF) && _CALL_ELF == 2
> > + if (!perf_probe_event_need_dwarf(pev)) {
> > + ret = find_probe_trace_events_from_map(pev, tevs, max_tevs, target);
> > + if (ret > 0)
> > + return ret; /* Found in symbol table */
> > + }
> > +#endif
>
> And should be in an arch helper, not a big powerpc wart dropped in the middle
> of the generic code.
Sure - will change.
Thanks for the review!
- Naveen
^ permalink raw reply
* Re: [PATCH] powerpc: secondary CPUs signal to master before setting active and online (fixes kernel BUG at kernel/smpboot.c:134!)
From: Thomas Gleixner @ 2014-12-10 14:08 UTC (permalink / raw)
To: Linus Torvalds
Cc: Peter Zijlstra, Yuanhan Liu, Benjamin Segall, Paul Mackerras,
Ingo Molnar, Rafael J. Wysocki, Ingo Molnar, Paul Turner,
yuyang.du, Daniel J Blueman, Steven Rostedt, Anton Blanchard,
subbaram, Wu Fengguang, lkp, Linux Kernel Mailing List,
Slava Pestov, Tejun Heo, Andrew Morton, Brian Norris, ppc-dev
In-Reply-To: <CA+55aFyR5PzNkojvEej1NXnpEFOPOYfnxCcPDAjg1rE79tQv7A@mail.gmail.com>
On Tue, 9 Dec 2014, Linus Torvalds wrote:
> On Mon, Dec 8, 2014 at 3:58 PM, Anton Blanchard <anton@samba.org> wrote:
> > Hi Ingo,
> >
> >> At that point I thought the previous task_cpu() was somewhat ingrained
> >> in the scheduler and came up with the patch. If not, we could go on a
> >> hunt to see what else needs fixing.
> >
> > I had another look. The scheduled does indeed make assumptions about the
> > previous task_cpu, but we have a hammer to fix it up called
> > select_fallback_rq.
> >
> > I annotated select_fallback_rq, and did hit a case where the CPU was
> > not active. ppc64 patch below.
>
> Anton, I'll assume I will get this through the usual powerpc pull requests?
>
> > I think x86 have a similar (although harder to hit) issue.
Indeed way harder to hit:
CPU 0 CPU 1
set_cpu_online(1, true) {
while (!cpu_online(cpu1)) cpumask_set_cpu(1, to_cpumask(cpu_online_bits));
relax();
wakeup_thread_on_cpu1();
cpumask_set_cpu(1, to_cpumask(cpu_active_bits));
On bare metal probably impossible, but on virt it should be
observable. Fix is simple.
Thanks,
tglx
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 668d8f2a8781..534f3384f03f 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -222,7 +222,6 @@ static void notrace start_secondary(void *unused)
lock_vector_lock();
set_cpu_online(smp_processor_id(), true);
unlock_vector_lock();
- per_cpu(cpu_state, smp_processor_id()) = CPU_ONLINE;
x86_platform.nmi_init();
/* enable local interrupts */
@@ -234,6 +233,7 @@ static void notrace start_secondary(void *unused)
x86_cpuinit.setup_percpu_clockev();
wmb();
+ per_cpu(cpu_state, smp_processor_id()) = CPU_ONLINE;
cpu_startup_entry(CPUHP_ONLINE);
}
@@ -932,7 +932,7 @@ int native_cpu_up(unsigned int cpu, struct task_struct *tidle)
check_tsc_sync_source(cpu);
local_irq_restore(flags);
- while (!cpu_online(cpu)) {
+ while (per_cpu(cpu_state,cpu) != CPU_ONLINE) {
cpu_relax();
touch_nmi_watchdog();
}
^ permalink raw reply related
* [PATCH] powerpc/powernv: Print the M64 range information
From: Wei Yang @ 2014-12-10 14:29 UTC (permalink / raw)
To: gwshan; +Cc: Wei Yang, linuxppc-dev
The M64 range information is missed, which would be helpful in debug.
This patch print the M64 range information in the same format as M32.
Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
---
arch/powerpc/platforms/powernv/pci-ioda.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
index 21c8b4d..34f1a27 100644
--- a/arch/powerpc/platforms/powernv/pci-ioda.c
+++ b/arch/powerpc/platforms/powernv/pci-ioda.c
@@ -369,6 +369,9 @@ static void __init pnv_ioda_parse_m64_window(struct pnv_phb *phb)
phb->ioda.m64_segsize = phb->ioda.m64_size / phb->ioda.total_pe;
phb->ioda.m64_base = pci_addr;
+ printk(KERN_INFO " MEM64 0x%016llx..0x%016llx -> 0x%016llx\n",
+ res->start, res->end, pci_addr);
+
/* Use last M64 BAR to cover M64 window */
phb->ioda.m64_bar_idx = 15;
phb->init_m64 = pnv_ioda2_init_m64;
--
1.7.9.5
^ permalink raw reply related
* [PATCH 4/4] powerpc32/8xx: invert _PAGE_RW bit in PTE accessors
From: Christophe Leroy @ 2014-12-10 18:00 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
scottwood
Cc: linuxppc-dev, linux-kernel
The 8xx inverts _PAGE_RW. Lets to it in PTE accessors.
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
arch/powerpc/include/asm/pte-8xx.h | 1 +
arch/powerpc/kernel/head_8xx.S | 3 ---
2 files changed, 1 insertion(+), 3 deletions(-)
diff --git a/arch/powerpc/include/asm/pte-8xx.h b/arch/powerpc/include/asm/pte-8xx.h
index daa4616..57bad0a 100644
--- a/arch/powerpc/include/asm/pte-8xx.h
+++ b/arch/powerpc/include/asm/pte-8xx.h
@@ -57,6 +57,7 @@
#define _PMD_PAGE_8M 0x000c
#define _PTE_NONE_MASK _PAGE_KNLRO
+#define _PTE_HW_INVERTED _PAGE_RW
/* Until my rework is finished, 8xx still needs atomic PTE updates */
#define PTE_ATOMIC_UPDATES 1
diff --git a/arch/powerpc/kernel/head_8xx.S b/arch/powerpc/kernel/head_8xx.S
index 3d4b8ee..807b0db 100644
--- a/arch/powerpc/kernel/head_8xx.S
+++ b/arch/powerpc/kernel/head_8xx.S
@@ -441,9 +441,6 @@ DataStoreTLBMiss:
and r11, r11, r10
rlwimi r10, r11, 0, _PAGE_PRESENT
#endif
- /* invert RW */
- xori r10, r10, _PAGE_RW
-
/* The Linux PTE won't go exactly into the MMU TLB.
* Software indicator bits 22 and 28 must be clear.
* Software indicator bits 24, 25, 26, and 27 must be
--
2.1.0
^ permalink raw reply related
* [PATCH 3/4] powerpc32: adds direct support to hardware inverted values in pte accessors
From: Christophe Leroy @ 2014-12-10 18:00 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
scottwood
Cc: linuxppc-dev, linux-kernel
Some powerpc like the 8xx do invert some PTE bits in HW. In order to avoid
having to invert the bits each time we set them into hardware, this patch adds
the invertion logic into the PTE accessors in order to get them already properly
inversed in the tables.
Inverted bits can be defined by _PTE_HW_INVERTED into the pte-XXX.h file
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
arch/powerpc/include/asm/page.h | 8 +++----
arch/powerpc/include/asm/pgtable-ppc32.h | 37 +++++++++++++++++++++-----------
arch/powerpc/include/asm/pte-common.h | 3 +++
3 files changed, 32 insertions(+), 16 deletions(-)
diff --git a/arch/powerpc/include/asm/page.h b/arch/powerpc/include/asm/page.h
index 26fe1ae..2e7a3ff 100644
--- a/arch/powerpc/include/asm/page.h
+++ b/arch/powerpc/include/asm/page.h
@@ -285,8 +285,8 @@ extern long long virt_phys_offset;
/* PTE level */
typedef struct { pte_basic_t pte; } pte_t;
-#define pte_val(x) ((x).pte)
-#define __pte(x) ((pte_t) { (x) })
+#define pte_val(x) ((x).pte ^ _PTE_HW_INVERTED)
+#define __pte(x) ((pte_t) { ((x) ^ _PTE_HW_INVERTED) })
/* 64k pages additionally define a bigger "real PTE" type that gathers
* the "second half" part of the PTE for pseudo 64k pages
@@ -328,8 +328,8 @@ typedef struct { unsigned long pgprot; } pgprot_t;
*/
typedef pte_basic_t pte_t;
-#define pte_val(x) (x)
-#define __pte(x) (x)
+#define pte_val(x) ((x) ^ _PTE_HW_INVERTED)
+#define __pte(x) ((x) ^ _PTE_HW_INVERTED)
#if defined(CONFIG_PPC_64K_PAGES) && defined(CONFIG_PPC_STD_MMU_64)
typedef struct { pte_t pte; unsigned long hidx; } real_pte_t;
diff --git a/arch/powerpc/include/asm/pgtable-ppc32.h b/arch/powerpc/include/asm/pgtable-ppc32.h
index 543bb8e..0e4a2a7 100644
--- a/arch/powerpc/include/asm/pgtable-ppc32.h
+++ b/arch/powerpc/include/asm/pgtable-ppc32.h
@@ -162,6 +162,12 @@ extern void flush_hash_entry(struct mm_struct *mm, pte_t *ptep,
* to properly flush the virtually tagged instruction cache of
* those implementations.
*/
+#if _PTE_HW_INVERTED == 0
+#define PTE_INVERT(val,flags)
+#else
+#define PTE_INVERT(val,flags) "xori "#val","#val","#flags"\n"
+#endif
+
#ifndef CONFIG_PTE_64BIT
static inline unsigned long pte_update(pte_t *p,
unsigned long clr,
@@ -174,30 +180,34 @@ static inline unsigned long pte_update(pte_t *p,
unsigned long tmp2;
__asm__ __volatile__("\
-1: lwarx %0,0,%4\n\
- andc %1,%0,%5\n\
+1: lwarx %0,0,%4\n"
+ PTE_INVERT(%0,%8)
+" andc %1,%0,%5\n\
or %1,%1,%6\n\
/* 0x200 == Extended encoding, bit 22 */ \
/* Bit 22 has to be 1 if neither _PAGE_USER nor _PAGE_RW are set */ \
rlwimi %1,%1,32-2,0x200\n /* get _PAGE_USER */ \
rlwinm %3,%1,32-1,0x200\n /* get _PAGE_RW */ \
- or %1,%3,%1\n\
- xori %1,%1,0x200\n"
-" stwcx. %1,0,%4\n\
+ or %1,%3,%1\n \
+ xori %1,%1,%9\n\
+ stwcx. %1,0,%4\n\
bne- 1b"
: "=&r" (old), "=&r" (tmp), "=m" (*p), "=&r" (tmp2)
- : "r" (p), "r" (clr), "r" (set), "m" (*p)
+ : "r" (p), "r" (clr), "r" (set), "m" (*p), "i"(_PTE_HW_INVERTED),
+ "i"(_PTE_HW_INVERTED|0x200)
: "cc" );
#else /* CONFIG_PPC_8xx */
__asm__ __volatile__("\
-1: lwarx %0,0,%3\n\
- andc %1,%0,%4\n\
+1: lwarx %0,0,%3\n"
+ PTE_INVERT(%0,%7)
+" andc %1,%0,%4\n\
or %1,%1,%5\n"
+ PTE_INVERT(%1,%7)
PPC405_ERR77(0,%3)
" stwcx. %1,0,%3\n\
bne- 1b"
: "=&r" (old), "=&r" (tmp), "=m" (*p)
- : "r" (p), "r" (clr), "r" (set), "m" (*p)
+ : "r" (p), "r" (clr), "r" (set), "m" (*p), "i"(_PTE_HW_INVERTED)
: "cc" );
#endif /* CONFIG_PPC_8xx */
#else /* PTE_ATOMIC_UPDATES */
@@ -222,14 +232,17 @@ static inline unsigned long long pte_update(pte_t *p,
__asm__ __volatile__("\
1: lwarx %L0,0,%4\n\
- lwzx %0,0,%3\n\
- andc %1,%L0,%5\n\
+ lwzx %0,0,%3\n"
+ PTE_INVERT(%L0,%8)
+" andc %1,%L0,%5\n\
or %1,%1,%6\n"
+ PTE_INVERT(%1,%8)
PPC405_ERR77(0,%3)
" stwcx. %1,0,%4\n\
bne- 1b"
: "=&r" (old), "=&r" (tmp), "=m" (*p)
- : "r" (p), "r" ((unsigned long)(p) + 4), "r" (clr), "r" (set), "m" (*p)
+ : "r" (p), "r" ((unsigned long)(p) + 4), "r" (clr), "r" (set), "m" (*p),
+ "i"(_PTE_HW_INVERTED)
: "cc" );
#else /* PTE_ATOMIC_UPDATES */
unsigned long long old = pte_val(*p);
diff --git a/arch/powerpc/include/asm/pte-common.h b/arch/powerpc/include/asm/pte-common.h
index e040c35..3d635fb 100644
--- a/arch/powerpc/include/asm/pte-common.h
+++ b/arch/powerpc/include/asm/pte-common.h
@@ -59,6 +59,9 @@
#ifndef _PTE_NONE_MASK
#define _PTE_NONE_MASK _PAGE_HPTEFLAGS
#endif
+#ifndef _PTE_HW_INVERTED
+#define _PTE_HW_INVERTED 0
+#endif
/* Make sure we get a link error if PMD_PAGE_SIZE is ever called on a
* kernel without large page PMD support
--
2.1.0
^ permalink raw reply related
* [PATCH 2/4] powerpc32: properly clear page table when 0 is not a good default PTE value
From: Christophe Leroy @ 2014-12-10 18:00 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
scottwood
Cc: linuxppc-dev, linux-kernel
Some HW invert some PTE bits. In some case, __pte(0) is not 0 so the PTEs shall
be properly set prior to being used.
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
arch/powerpc/mm/pgtable_32.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/arch/powerpc/mm/pgtable_32.c b/arch/powerpc/mm/pgtable_32.c
index a349089..71a2821 100644
--- a/arch/powerpc/mm/pgtable_32.c
+++ b/arch/powerpc/mm/pgtable_32.c
@@ -96,6 +96,14 @@ void pgd_free(struct mm_struct *mm, pgd_t *pgd)
#endif
}
+static inline void pte_alloc_clear(pte_t *pte)
+{
+ int i;
+
+ for (i = 0; i < PTRS_PER_PTE; i++)
+ pte[i] = __pte(0);
+}
+
__init_refok pte_t *pte_alloc_one_kernel(struct mm_struct *mm, unsigned long address)
{
pte_t *pte;
@@ -109,18 +117,24 @@ __init_refok pte_t *pte_alloc_one_kernel(struct mm_struct *mm, unsigned long add
if (pte)
clear_page(pte);
}
+ if (pte && !pte_none(*pte))
+ pte_alloc_clear(pte);
return pte;
}
pgtable_t pte_alloc_one(struct mm_struct *mm, unsigned long address)
{
struct page *ptepage;
+ pte_t *pte;
gfp_t flags = GFP_KERNEL | __GFP_REPEAT | __GFP_ZERO;
ptepage = alloc_pages(flags, 0);
if (!ptepage)
return NULL;
+ pte = (pte_t *)pfn_to_kaddr(page_to_pfn(ptepage));
+ if (!pte_none(*pte))
+ pte_alloc_clear(pte);
if (!pgtable_page_ctor(ptepage)) {
__free_page(ptepage);
return NULL;
--
2.1.0
^ permalink raw reply related
* [PATCH 1/4] powerpc32: misuse of accessors to pte_t objects
From: Christophe Leroy @ 2014-12-10 18:00 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
scottwood
Cc: linuxppc-dev, linux-kernel
pte_val() is not meant to be used as L value.
__pte() has to be used to assign value to pte_t.
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
arch/powerpc/include/asm/pgtable.h | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/arch/powerpc/include/asm/pgtable.h b/arch/powerpc/include/asm/pgtable.h
index 316f9a5..5d4fdcc 100644
--- a/arch/powerpc/include/asm/pgtable.h
+++ b/arch/powerpc/include/asm/pgtable.h
@@ -115,24 +115,25 @@ static inline unsigned long pte_pfn(pte_t pte) {
/* Generic modifiers for PTE bits */
static inline pte_t pte_wrprotect(pte_t pte) {
- pte_val(pte) &= ~(_PAGE_RW | _PAGE_HWWRITE); return pte; }
+ pte = __pte(pte_val(pte) & ~(_PAGE_RW | _PAGE_HWWRITE)); return pte; }
static inline pte_t pte_mkclean(pte_t pte) {
- pte_val(pte) &= ~(_PAGE_DIRTY | _PAGE_HWWRITE); return pte; }
+ pte = __pte(pte_val(pte) & ~(_PAGE_DIRTY | _PAGE_HWWRITE));
+ return pte; }
static inline pte_t pte_mkold(pte_t pte) {
- pte_val(pte) &= ~_PAGE_ACCESSED; return pte; }
+ pte = __pte(pte_val(pte) & ~_PAGE_ACCESSED); return pte; }
static inline pte_t pte_mkwrite(pte_t pte) {
- pte_val(pte) |= _PAGE_RW; return pte; }
+ pte = __pte(pte_val(pte) | _PAGE_RW); return pte; }
static inline pte_t pte_mkdirty(pte_t pte) {
- pte_val(pte) |= _PAGE_DIRTY; return pte; }
+ pte = __pte(pte_val(pte) | _PAGE_DIRTY); return pte; }
static inline pte_t pte_mkyoung(pte_t pte) {
- pte_val(pte) |= _PAGE_ACCESSED; return pte; }
+ pte = __pte(pte_val(pte) | _PAGE_ACCESSED); return pte; }
static inline pte_t pte_mkspecial(pte_t pte) {
- pte_val(pte) |= _PAGE_SPECIAL; return pte; }
+ pte = __pte(pte_val(pte) | _PAGE_SPECIAL); return pte; }
static inline pte_t pte_mkhuge(pte_t pte) {
return pte; }
static inline pte_t pte_modify(pte_t pte, pgprot_t newprot)
{
- pte_val(pte) = (pte_val(pte) & _PAGE_CHG_MASK) | pgprot_val(newprot);
+ pte = __pte((pte_val(pte) & _PAGE_CHG_MASK) | pgprot_val(newprot));
return pte;
}
--
2.1.0
^ permalink raw reply related
* [PATCH 0/4] powerpc32: fix of PTE accessors and handle inverted HW bits via PTE accessors
From: Christophe Leroy @ 2014-12-10 18:00 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras
Cc: scottwood, linuxppc-dev, linux-kernel
Some powerpc like the 8xx do invert some PTE bits in HW. In order to avoid
having to invert the bits each time we set them into hardware, this patch adds
the invertion logic into the PTE accessors in order to get them already properly
inversed in the tables.
Patchset:
1) powerpc32: misuse of accessors to pte_t objects
2) powerpc32: properly clear page table when 0 is not a good default PTE value
3) powerpc32: adds direct support to hardware inverted values in pte accessors
4) powerpc32/8xx: invert _PAGE_RW bit in PTE accessors
All changes have been successfully tested on MPC885
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
Tested-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
arch/powerpc/include/asm/page.h | 8 +++----
arch/powerpc/include/asm/pgtable-ppc32.h | 37 +++++++++++++++++++++-----------
arch/powerpc/include/asm/pgtable.h | 17 ++++++++-------
arch/powerpc/include/asm/pte-8xx.h | 1 +
arch/powerpc/include/asm/pte-common.h | 3 +++
arch/powerpc/kernel/head_8xx.S | 3 ---
arch/powerpc/mm/pgtable_32.c | 14 ++++++++++++
7 files changed, 56 insertions(+), 27 deletions(-)
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox