* Re: [PATCH v2 2/2] powerpc new toolchains fix (crt.S)
From: Segher Boessenkool @ 2010-05-13 17:27 UTC (permalink / raw)
To: Maxim Uvarov; +Cc: lists, kexec, horms, linuxppc-dev
In-Reply-To: <20100512070849.9572.84728.stgit@muvarov>
> Linker does not provide some vital functions when building
> freestanding
> applications with a new toolchain,
That's because the compiler provides those functions, not the linker.
> so we have to provide our own CRT.
...in libgcc. Why don't you link against that?
> p.s.
> Without the CRT we won't see any build errors (since the purgatory is
> linked with --no-undefined), but the purgatory code won't work,
It would be nice if the build would check if anything unexpectedly
undefined is left.
Segher
^ permalink raw reply
* Re: [PATCH v2 2/2] powerpc new toolchains fix (crt.S)
From: Anton Vorontsov @ 2010-05-13 19:07 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: Maxim Uvarov, lists, horms, kexec, linuxppc-dev
In-Reply-To: <15016EE5-7B40-40C9-9E45-58409644CE17@kernel.crashing.org>
On Thu, May 13, 2010 at 07:27:59PM +0200, Segher Boessenkool wrote:
> >Linker does not provide some vital functions when building
> >freestanding
> >applications with a new toolchain,
>
> That's because the compiler provides those functions, not the linker.
>
> >so we have to provide our own CRT.
>
> ...in libgcc. Why don't you link against that?
For the same reason we don't link kernel against libgcc?
I.e. just a matter of taste. There are two camps: those who
argue against libgcc, and those who argue for libgcc.
Hey! You were in this thread! :-)
http://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/msg17224.html
Purgatory is just a small (tiny) piece of code that runs
between kernels. Whether use libgcc or not, personally, I have
no strong opinion. Kexec didn't use it, so we kept it the same
way.
Thanks,
--
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2
^ permalink raw reply
* Re: [PATCH] pte_fsl_booke: fix instruction TLB error permission check
From: Kumar Gala @ 2010-05-13 19:14 UTC (permalink / raw)
To: Li Yang; +Cc: linuxppc-dev, Jin Qing
In-Reply-To: <1273221514-8683-1-git-send-email-leoli@freescale.com>
On May 7, 2010, at 3:38 AM, Li Yang wrote:
> Check the user/supervisor execution permission base on the code =
address.
> This fixes the following oops on module loading or removing.
>=20
> Unable to handle kernel paging request for instruction fetch
> Faulting instruction address: 0xf938d040
> Oops: Kernel access of bad area, sig: 11 [#1]
>=20
> Signed-off-by: Li Yang <leoli@freescale.com>
> Signed-off-by: Jin Qing <b24347@freescale.com>
> ---
> arch/powerpc/kernel/head_fsl_booke.S | 13 ++++++++++---
> 1 files changed, 10 insertions(+), 3 deletions(-)
applied to merge.
I've updated the commit message to be a bit more clear on why we need to =
do this.
Thanks for figuring this out.
- k=
^ permalink raw reply
* Re: "event-scan failed" logflood
From: Benjamin Herrenschmidt @ 2010-05-13 21:28 UTC (permalink / raw)
To: pacman; +Cc: linuxppc-dev
In-Reply-To: <20100513111603.3352.qmail@kosh.dhis.org>
On Thu, 2010-05-13 at 06:16 -0500, pacman@kosh.dhis.org wrote:
> Benjamin Herrenschmidt writes:
> >
> > Well, first it should be called once per second, not 60 times per
> > second, so something is wrong there...
>
> Actually I think it was happening a lot more than 60 times per second, and
> klogd was losing most of the messages because they came too fast. When
> running the new kernel, vmstat shows 170000 context switches per second (and
> CPU usage never going below 5%) even when everything should be idle.
>
> My next experiment was to hack rtas_init to return 0 unconditionally. Now
> everything still runs fine, and the context switches per second are back down
> in the 2-digit range.
>
> >
> > Then, it wouldn't surprise me if the Pegasos RTAS didn't implement
> > the event scan properly. Maybe we failed silently before that ? I
> > doubt it's necessary there anyways.
>
> If I'm reading things correctly, rtas-event-scan-rate is the key to the
> frequency of attempts. And rtasd.c didn't expect this:
>
> $ od -t x1 /proc/device-tree/rtas/rtas-event-scan-rate
> 0000000 00 00 00 00
> 0000004
Ok, that gives us a good thing to use to not do the scanning then. If
that's 0 then we assume it's a bogus OF and we disable the scan
completely. I'll do a patch later today, though feel free to beat me to
it.
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCH] pte_fsl_booke: fix instruction TLB error permission check
From: Benjamin Herrenschmidt @ 2010-05-13 21:46 UTC (permalink / raw)
To: Kumar Gala; +Cc: linuxppc-dev, Jin Qing
In-Reply-To: <0710E9DF-66DF-4D24-95DE-A04E6D869826@kernel.crashing.org>
On Thu, 2010-05-13 at 14:14 -0500, Kumar Gala wrote:
> On May 7, 2010, at 3:38 AM, Li Yang wrote:
>
> > Check the user/supervisor execution permission base on the code address.
> > This fixes the following oops on module loading or removing.
> >
> > Unable to handle kernel paging request for instruction fetch
> > Faulting instruction address: 0xf938d040
> > Oops: Kernel access of bad area, sig: 11 [#1]
> >
> > Signed-off-by: Li Yang <leoli@freescale.com>
> > Signed-off-by: Jin Qing <b24347@freescale.com>
> > ---
> > arch/powerpc/kernel/head_fsl_booke.S | 13 ++++++++++---
> > 1 files changed, 10 insertions(+), 3 deletions(-)
>
> applied to merge.
>
> I've updated the commit message to be a bit more clear on why we need to do this.
Not looking at the code right now ... but do we have the same issue on
64e ?
Cheers,
Ben.
^ permalink raw reply
* Re: ftrace syscalls, PowerPC: Fixes and PowerPC raw syscall tracepoint implementation
From: Benjamin Herrenschmidt @ 2010-05-13 23:55 UTC (permalink / raw)
To: rostedt
Cc: Frederic Weisbecker, Jason Baron, linux-kernel, Ingo Molnar,
Paul Mackerras, Ian Munsie, linuxppc-dev
In-Reply-To: <1273766771.27703.1069.camel@gandalf.stny.rr.com>
On Thu, 2010-05-13 at 12:06 -0400, Steven Rostedt wrote:
> Frederic,
>
> I'm fine with these patches, but since you mainly did the syscall work,
> I'll let you take them.
>
> The patches that touch the PowerPC code needs an acked-by from Ben or
> Paul.
Done :-)
Cheers,
Ben.
> -- Steve
>
>
> On Thu, 2010-05-13 at 17:43 +1000, Ian Munsie wrote:
> > This patch series implements raw system call tracepoints on PowerPC that can be
> > used with ftrace and perf. Some problems with the generic ftrace syscall
> > tracepoint code have also been addressed.
> >
> > The patches are based upon Ben's powerpc/next tree merged with tip/tracing/core
> >
> > Patch #1 removes all ftrace syscall events that fail to map the system call
> > name from the system call metadata with the system call's number, preventing
> > the events which will not work from showing up in perf list and removing them
> > from /sys/kernel/debug/tracing/events/syscalls.
> >
> > Patches #2 and #3 allow for archs with unusual system call tables (#2) or
> > unusual symbol names (#3) to override the appropriate functions so that they
> > can still work with ftrace syscalls.
> >
> > Patch #4 implements the actual raw system call tracepoints that ftrace syscalls
> > builds upon, allowing all of the system calls to be used with the raw_syscalls
> > events category and most to be used with the syscalls category.
> >
> >
> > Not all the raw_syscalls are currently mapped to ftrace syscalls - the syscalls
> > defined in /arch/powerpc/include/asm/syscalls.h do not use the SYSCALL_DEFINE
> > class of macros and as such have no meta data, likewise some of the ppc_*
> > syscalls have assembly wrappers. These are on their way, but I wanted to put
> > the work I have done so far out first.
> >
> > Some of those syscalls have different return types than the __SYSCALL_DEFINE
> > macro uses (unsigned long, int, time_t) and some have different prefixes (ppc,
> > ppc64) - I didn't particularly want to change them straight over without asking
> > the list first, and I certainly don't want to change the return types. I see
> > that Jason Baron ran into similar issues, but his "add compat syscall support"
> > patches have yet to be merged, and do not tackle the differing return types.
> >
>
^ permalink raw reply
* Re: [PATCH 3/4] ftrace syscalls: Allow arch specific syscall symbol matching
From: Benjamin Herrenschmidt @ 2010-05-13 23:54 UTC (permalink / raw)
To: Ian Munsie
Cc: Frederic Weisbecker, Jason Baron, linux-kernel, Steven Rostedt,
Ingo Molnar, Paul Mackerras, linuxppc-dev
In-Reply-To: <1273736594-19320-4-git-send-email-imunsie@au1.ibm.com>
On Thu, 2010-05-13 at 17:43 +1000, Ian Munsie wrote:
> From: Ian Munsie <imunsie@au1.ibm.com>
>
> Some architectures have unusual symbol names and the generic code to
> match the symbol name with the function name for the syscall metadata
> will fail. For example, symbols on PPC64 start with a period and the
> generic code will fail to match them.
>
> This patch splits out the match logic into a standalone weak function
> that can be overridden on archs with unusual symbol names.
>
> Signed-off-by: Ian Munsie <imunsie@au1.ibm.com>
> ---
Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
Ian, I assume you will implement the support for the "special" ppc_*
syscalls via a subsequent patch and not a respin of this one right ?
Cheers,
Ben.
> Documentation/trace/ftrace-design.txt | 3 +++
> include/linux/ftrace.h | 1 +
> kernel/trace/trace_syscalls.c | 19 ++++++++++++-------
> 3 files changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/trace/ftrace-design.txt b/Documentation/trace/ftrace-design.txt
> index 8369a1c..3936d5f 100644
> --- a/Documentation/trace/ftrace-design.txt
> +++ b/Documentation/trace/ftrace-design.txt
> @@ -247,6 +247,9 @@ You need very few things to get the syscalls tracing in an arch.
> - If the system call table on this arch is more complicated than a simple array
> of addresses of the system calls, implement an arch_syscall_addr to return
> the address of a given system call.
> +- If the symbol names of the system calls do not match the function names on
> + this arch, implement an arch_syscall_match_sym_name with the appropriate
> + logic to return true if the function name corresponds with the symbol name.
> - Tag this arch as HAVE_SYSCALL_TRACEPOINTS.
>
>
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index e0ae83b..26ad1f5 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -534,6 +534,7 @@ static inline void trace_hw_branch_oops(void) {}
> #ifdef CONFIG_FTRACE_SYSCALLS
>
> unsigned long arch_syscall_addr(int nr);
> +bool arch_syscall_match_sym_name(const char *sym, const char *name);
>
> #endif /* CONFIG_FTRACE_SYSCALLS */
>
> diff --git a/kernel/trace/trace_syscalls.c b/kernel/trace/trace_syscalls.c
> index 1c231d0..ebbc74d 100644
> --- a/kernel/trace/trace_syscalls.c
> +++ b/kernel/trace/trace_syscalls.c
> @@ -32,13 +32,7 @@ static struct syscall_metadata *find_syscall_meta(unsigned long syscall)
> kallsyms_lookup(syscall, NULL, NULL, NULL, str);
>
> for ( ; start < stop; start++) {
> - /*
> - * Only compare after the "sys" prefix. Archs that use
> - * syscall wrappers may have syscalls symbols aliases prefixed
> - * with "SyS" instead of "sys", leading to an unwanted
> - * mismatch.
> - */
> - if (start->name && !strcmp(start->name + 3, str + 3))
> + if (start->name && arch_syscall_match_sym_name(str, start->name))
> return start;
> }
> return NULL;
> @@ -408,6 +402,17 @@ unsigned long __init __weak arch_syscall_addr(int nr)
> return (unsigned long)sys_call_table[nr];
> }
>
> +bool __weak arch_syscall_match_sym_name(const char *sym, const char *name)
> +{
> + /*
> + * Only compare after the "sys" prefix. Archs that use
> + * syscall wrappers may have syscalls symbols aliases prefixed
> + * with "SyS" instead of "sys", leading to an unwanted
> + * mismatch.
> + */
> + return (!strcmp(sym + 3, name + 3));
> +}
> +
> int __init init_ftrace_syscalls(void)
> {
> struct syscall_metadata *meta;
^ permalink raw reply
* Re: [PATCH 1/2] Fix kexec on powerpc32
From: Christian Kujau @ 2010-05-14 0:43 UTC (permalink / raw)
To: Maxim Uvarov; +Cc: kexec, uvarov, linuxppc-dev, horms
In-Reply-To: <AANLkTik1aRVvqj871qOmGctuP_WjvSFwhU1w4gpJoqp1@mail.gmail.com>
On Thu, 13 May 2010 at 15:50, Maxim Uvarov wrote:
> zImage should not work with kexec. Only vmlinux.
"should"?
PPC64 has something nice there:
> kexec/arch/ppc64/kexec-zImage-ppc64.c:
> fprintf(stderr, "zImage support is still broken\n");
....but I could not figure out where this is called from.
Anyway, tried vmlinux now and it's working, yay!
Thanks a lot, Maxim!
Christian.
--
BOFH excuse #363:
Out of cards on drive D:
^ permalink raw reply
* RE: [PATCH] pte_fsl_booke: fix instruction TLB error permission check
From: Liu Dave-R63238 @ 2010-05-14 0:53 UTC (permalink / raw)
To: Kumar Gala, Li Yang-R58472; +Cc: linuxppc-dev, Jin Qing-B24347
In-Reply-To: <0710E9DF-66DF-4D24-95DE-A04E6D869826@kernel.crashing.org>
> I've updated the commit message to be a bit more clear on why=20
> we need to do this.
I'm curious why the _PAGE_EXEC have different definition in pte-book3e.h
and pte-fsl-booke.h?
It is UX permission in pte-book3e, but is SX permission in
pte-fsl-booke.h.
Thanks, Dave
^ permalink raw reply
* Re: [PATCH 4/4] trace, powerpc: Implement raw syscall tracepoints on PowerPC
From: Ian Munsie @ 2010-05-14 2:03 UTC (permalink / raw)
To: michael
Cc: Frederic Weisbecker, Jason Baron, linux-kernel, Steven Rostedt,
Ingo Molnar, Paul Mackerras, linuxppc-dev
In-Reply-To: <1273752571.8067.12.camel@concordia>
Excerpts from Michael Ellerman's message of Thu May 13 22:09:31 +1000 2010:
> On Thu, 2010-05-13 at 17:43 +1000, Ian Munsie wrote:
> > From: Ian Munsie <imunsie@au.ibm.com>
>
> Hi Ian,
>
> Just a few comments ..
>
> > This patch implements the raw syscall tracepoints on PowerPC required
> > for ftrace syscalls.
>
> OK. It also adds a bunch of code under CONFIG_FTRACE_*, so does it
> implement raw syscall tracepoints _and_ hook them up to ftrace?
Yes, that's correct. CONFIG_FTRACE_SYSCALLS depends solely on, and is
the primary consumer of, HAVE_SYSCALL_TRACEPOINTS. It makes little sense
to me to provide the raw syscall tracepoints without exporting the
syscall table for ftrace syscalls to use - otherwise they would be
available to select in make config, but broken.
> > To minimise reworking existing code, I slightly re-ordered the thread
> > info flags such that the new TIF_SYSCALL_TRACEPOINT bit would still fit
> > within the 16 bits of the andi instruction's UI field.
>
> Which andi instruction? That could use a bit more explaining.
The ones under /arch/powerpc/kernel/entry_{32,64}.S using andi to
and the _TIF_SYSCALL_T_OR_A with the thread flags to see if system call
tracing is enabled.
For instance, from entry_64.S:
ld r10,TI_FLAGS(r11)
andi. r11,r10,_TIF_SYSCALL_T_OR_A <-- that one
bne- syscall_dotrace
.......
syscall_dotrace:
bl .save_nvgprs
addi r3,r1,STACK_FRAME_OVERHEAD
bl .do_syscall_trace_enter
And similarly elsewhere in the same file:
ld r9,TI_FLAGS(r12)
li r11,-_LAST_ERRNO
andi. r0,r9,(_TIF_SYSCALL_T_OR_A|_TIF_SINGLESTEP|_TIF_USER_WORK_MASK|_TIF_PERSYSCALL_MASK)
bne- syscall_exit_work
.......
syscall_exit_work:
.......
bl .save_nvgprs
addi r3,r1,STACK_FRAME_OVERHEAD
bl .do_syscall_trace_leave
entry_32.S contains very similar assembly to the above.
The alternative to renumbering the thread flags would be to rework the
assembly to use and. instead of andi. to avoid having to squeeze that
flag into 16 bits.
> > In the case of 64bit PowerPC, arch_syscall_addr and
> > arch_syscall_match_sym_name are overridden to allow ftrace syscalls to
> > work given the unusual system call table structure and symbol names that
> > start with a period.
>
> Not unusual, just different (ie. better) than x86 ;)
Fair enough ;)
> > diff --git a/arch/powerpc/include/asm/syscall.h b/arch/powerpc/include/asm/syscall.h
> > index 23913e9..4098105 100644
> > --- a/arch/powerpc/include/asm/syscall.h
> > +++ b/arch/powerpc/include/asm/syscall.h
> > @@ -15,6 +15,15 @@
> >
> > #include <linux/sched.h>
> >
> > +/* ftrace syscalls requires exporting the sys_call_table */
> > +#ifdef CONFIG_FTRACE_SYSCALLS
> > +#ifdef CONFIG_PPC64
> > +extern const unsigned long long *sys_call_table;
>
> I'm not sure why this is ULL ? UL and ULL are both 64 bits (on 64bit),
> and it would save you this ifdef block and a cast in
> arch_syscall_addr().
Good point - I was just following the format from
/arch/powerpc/kernel/systbl.S, which uses pairs of .llong on PPC64 and
single .long on PPC32. Does the assembler treat .llong different from
.long on 64bit?
> > +#else /* !CONFIG_PPC64 */
> > +extern const unsigned long *sys_call_table;
> > +#endif /* CONFIG_PPC64 */
> > +#endif /* CONFIG_FTRACE_SYSCALLS */
> > +
> > static inline long syscall_get_nr(struct task_struct *task,
> > struct pt_regs *regs)
> > {
> > diff --git a/arch/powerpc/include/asm/thread_info.h b/arch/powerpc/include/asm/thread_info.h
> > index aa9d383..e7a8af2 100644
> > --- a/arch/powerpc/include/asm/thread_info.h
> > +++ b/arch/powerpc/include/asm/thread_info.h
> > @@ -110,7 +110,8 @@ static inline struct thread_info *current_thread_info(void)
> > #define TIF_NOERROR 12 /* Force successful syscall return */
> > #define TIF_NOTIFY_RESUME 13 /* callback before returning to user */
> > #define TIF_FREEZE 14 /* Freezing for suspend */
> > -#define TIF_RUNLATCH 15 /* Is the runlatch enabled? */
> > +#define TIF_SYSCALL_TRACEPOINT 15 /* syscall tracepoint instrumentation */
> > +#define TIF_RUNLATCH 16 /* Is the runlatch enabled? */
>
> I don't grok why this is good or safe, not that it isn't but please tell
> me why it is :)
Ok - now I could be wrong on this, but AFAICT the flags are only used
internally within the kernel by name and not exported to userspace (ie,
not part of the kernel ABI). That specific flag is only set and cleared
within /arch/powerpc/kernel/process.c so recompiling the kernel should
be sufficient to change those instances of TIF_RUNLATCH from 15 to 16.
> > diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
> > index 8773263..9c404bb 100644
> > --- a/arch/powerpc/kernel/Makefile
> > +++ b/arch/powerpc/kernel/Makefile
> > @@ -98,6 +98,7 @@ obj64-$(CONFIG_AUDIT) += compat_audit.o
> >
> > obj-$(CONFIG_DYNAMIC_FTRACE) += ftrace.o
> > obj-$(CONFIG_FUNCTION_GRAPH_TRACER) += ftrace.o
> > +obj-$(CONFIG_FTRACE_SYSCALLS) += ftrace.o
>
> You're following the existing pattern there, but it's a little odd.
> Seems like those three config options should really all depend on
> something common and that should trigger the build of ftrace.c
There isn't anything in the arch specific ftrace.c that depends purely on
ftrace. It's all stuff that depends on the above specific ftrace options
that have some arch specific implementation.
> > diff --git a/arch/powerpc/kernel/ftrace.c b/arch/powerpc/kernel/ftrace.c
> > index ce1f3e4..b34171e 100644
> > --- a/arch/powerpc/kernel/ftrace.c
> > +++ b/arch/powerpc/kernel/ftrace.c
> > @@ -22,6 +22,7 @@
> > #include <asm/cacheflush.h>
> > #include <asm/code-patching.h>
> > #include <asm/ftrace.h>
> > +#include <asm/syscall.h>
> >
> >
> > #ifdef CONFIG_DYNAMIC_FTRACE
> > @@ -600,3 +601,15 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr)
> > }
> > }
> > #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
> > +
> > +#if defined(CONFIG_FTRACE_SYSCALLS) && defined(CONFIG_PPC64)
>
> Does 32-bit just work using the existing routines? Or do we not support
> it on 32-bit (though that's not what your Kconfig change said).
It should work with the existing routines - I still need to test this on
feugo to make sure, but following from /arch/powerpc/kernel/systbl.S it
seems that the symbol names should match the function names and the
system call table is a trivial lookup table, both of which will work
with the generic implementation.
> > +unsigned long __init arch_syscall_addr(int nr)
> > +{
> > + return (unsigned long)sys_call_table[nr*2];
>
> That's the cast I was referring to.
Ok, I'll change that in v2.
> > +}
> > +
> > +inline bool arch_syscall_match_sym_name(const char *sym, const char *name)
> > +{
> > + return (!strcmp(sym + 4, name + 3));
>
> So the +4 is to skip ".sys" and the +3 is to skip "sys" ? That could use
> a comment IMHO :)
Yeah that's right, I'll add a comment there to clarify that.
>
> cheers
Thanks for the feedback,
-Ian
^ permalink raw reply
* Re: [PATCH 3/4] ftrace syscalls: Allow arch specific syscall symbol matching
From: Ian Munsie @ 2010-05-14 2:06 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Frederic Weisbecker, Jason Baron, linux-kernel, Steven Rostedt,
Ingo Molnar, Paul Mackerras, linuxppc-dev
In-Reply-To: <1273794896.21352.203.camel@pasglop>
Excerpts from Benjamin Herrenschmidt's message of Fri May 14 09:54:56 +1000 2010:
> On Thu, 2010-05-13 at 17:43 +1000, Ian Munsie wrote:
> > From: Ian Munsie <imunsie@au1.ibm.com>
> >
> > Some architectures have unusual symbol names and the generic code to
> > match the symbol name with the function name for the syscall metadata
> > will fail. For example, symbols on PPC64 start with a period and the
> > generic code will fail to match them.
> >
> > This patch splits out the match logic into a standalone weak function
> > that can be overridden on archs with unusual symbol names.
> >
> > Signed-off-by: Ian Munsie <imunsie@au1.ibm.com>
> > ---
>
> Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
>
> Ian, I assume you will implement the support for the "special" ppc_*
> syscalls via a subsequent patch and not a respin of this one right ?
>
> Cheers,
> Ben.
Yes, that will be in a separate patch or two.
Cheers,
-Ian
^ permalink raw reply
* RE: [PATCH] pte_fsl_booke: fix instruction TLB error permission check
From: Liu Dave-R63238 @ 2010-05-14 2:16 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Kumar Gala; +Cc: linuxppc-dev, Jin Qing-B24347
In-Reply-To: <1273787172.21352.198.camel@pasglop>
> Not looking at the code right now ... but do we have the same=20
> issue on 64e ?
Aaron pointed the issue on FSL BookE.
http://lists.ozlabs.org/pipermail/linuxppc-dev/2010-January/079738.html
^ permalink raw reply
* [PATCH v2] kexec-tools, ppc64: Fix segfault parsing DR memory property
From: Matt Evans @ 2010-05-14 3:07 UTC (permalink / raw)
To: kexec; +Cc: linuxppc-dev, Michael Neuling, Simon Horman
add_dyn_reconf_usable_mem_property() iterates over memory spans
in /ibm,dynamic-reconfiguration-memory/ibm,dynamic-memory and intersects
these with usablemem_rgns ranges. In doing so it used an unchecked
fixed-size array which will overrun on machines with lots of LMBs.
This patch removes the fixed-sized arrays from
add_dyn_reconf_usable_mem_property() and add_usable_mem_property(), in lieu of
malloc/realloc/free.
Signed-off-by: Matt Evans <matt@ozlabs.org>
---
kexec/arch/ppc64/fs2dt.c | 82 +++++++++++++++++++++++++++++++++++++++-------
1 files changed, 70 insertions(+), 12 deletions(-)
diff --git a/kexec/arch/ppc64/fs2dt.c b/kexec/arch/ppc64/fs2dt.c
index 762bf04..4400f13 100644
--- a/kexec/arch/ppc64/fs2dt.c
+++ b/kexec/arch/ppc64/fs2dt.c
@@ -37,7 +37,7 @@
#define NAMESPACE 16384 /* max bytes for property names */
#define INIT_TREE_WORDS 65536 /* Initial num words for prop values */
#define MEMRESERVE 256 /* max number of reserved memory blocks */
-#define MAX_MEMORY_RANGES 1024
+#define MEM_RANGE_CHUNK_SZ 2048 /* Initial num dwords for mem ranges */
static char pathname[MAXPATH], *pathstart;
static char propnames[NAMESPACE] = { 0 };
@@ -148,7 +148,8 @@ static void add_dyn_reconf_usable_mem_property(int fd)
{
char fname[MAXPATH], *bname;
uint64_t buf[32];
- uint64_t ranges[2*MAX_MEMORY_RANGES];
+ uint64_t *ranges;
+ int ranges_size = MEM_RANGE_CHUNK_SZ;
uint64_t base, end, loc_base, loc_end;
size_t i, rngs_cnt, range;
int rlen = 0;
@@ -165,6 +166,11 @@ static void add_dyn_reconf_usable_mem_property(int fd)
die("unrecoverable error: error seeking in \"%s\": %s\n",
pathname, strerror(errno));
+ ranges = malloc(ranges_size*8);
+ if (!ranges)
+ die("unrecoverable error: can't alloc %d bytes for ranges.\n",
+ ranges_size*8);
+
rlen = 0;
for (i = 0; i < num_of_lmbs; i++) {
if (read(fd, buf, 24) < 0)
@@ -180,24 +186,57 @@ static void add_dyn_reconf_usable_mem_property(int fd)
rngs_cnt = 0;
for (range = 0; range < usablemem_rgns.size; range++) {
+ int add = 0;
loc_base = usablemem_rgns.ranges[range].start;
loc_end = usablemem_rgns.ranges[range].end;
if (loc_base >= base && loc_end <= end) {
- ranges[rlen++] = loc_base;
- ranges[rlen++] = loc_end - loc_base;
- rngs_cnt++;
+ add = 1;
} else if (base < loc_end && end > loc_base) {
if (loc_base < base)
loc_base = base;
if (loc_end > end)
loc_end = end;
+ add = 1;
+ }
+
+ if (add) {
+ if (rlen >= (ranges_size-2)) {
+ ranges_size += MEM_RANGE_CHUNK_SZ;
+ ranges = realloc(ranges, ranges_size*8);
+ if (!ranges)
+ die("unrecoverable error: can't"
+ " realloc %d bytes for"
+ " ranges.\n",
+ ranges_size*8);
+ }
ranges[rlen++] = loc_base;
ranges[rlen++] = loc_end - loc_base;
rngs_cnt++;
}
}
- /* Store the count of (base, size) duple */
- ranges[tmp_indx] = rngs_cnt;
+ if (rngs_cnt == 0) {
+ /* We still need to add a counter for every LMB because
+ * the kernel parsing code is dumb. We just have
+ * a zero in this case, with no following base/len.
+ */
+ ranges[tmp_indx] = 0;
+ /* rlen is already just tmp_indx+1 as we didn't write
+ * anything. Check array size here, as we'll probably
+ * go on for a while writing zeros now.
+ */
+ if (rlen >= (ranges_size-1)) {
+ ranges_size += MEM_RANGE_CHUNK_SZ;
+ ranges = realloc(ranges, ranges_size*8);
+ if (!ranges)
+ die("unrecoverable error: can't"
+ " realloc %d bytes for"
+ " ranges.\n",
+ ranges_size*8);
+ }
+ } else {
+ /* Store the count of (base, size) duple */
+ ranges[tmp_indx] = rngs_cnt;
+ }
}
rlen = rlen * sizeof(uint64_t);
@@ -210,7 +249,8 @@ static void add_dyn_reconf_usable_mem_property(int fd)
*dt++ = propnum("linux,drconf-usable-memory");
if ((rlen >= 8) && ((unsigned long)dt & 0x4))
dt++;
- memcpy(dt, &ranges, rlen);
+ memcpy(dt, ranges, rlen);
+ free(ranges);
dt += (rlen + 3)/4;
}
@@ -218,7 +258,8 @@ static void add_usable_mem_property(int fd, size_t len)
{
char fname[MAXPATH], *bname;
uint64_t buf[2];
- uint64_t ranges[2*MAX_MEMORY_RANGES];
+ uint64_t *ranges;
+ int ranges_size = MEM_RANGE_CHUNK_SZ;
uint64_t base, end, loc_base, loc_end;
size_t range;
int rlen = 0;
@@ -247,17 +288,33 @@ static void add_usable_mem_property(int fd, size_t len)
base = buf[0];
end = base + buf[1];
+ ranges = malloc(ranges_size*8);
+ if (!ranges)
+ die("unrecoverable error: can't alloc %d bytes for ranges.\n",
+ ranges_size*8);
+
for (range = 0; range < usablemem_rgns.size; range++) {
+ int add = 0;
loc_base = usablemem_rgns.ranges[range].start;
loc_end = usablemem_rgns.ranges[range].end;
if (loc_base >= base && loc_end <= end) {
- ranges[rlen++] = loc_base;
- ranges[rlen++] = loc_end - loc_base;
+ add = 1;
} else if (base < loc_end && end > loc_base) {
if (loc_base < base)
loc_base = base;
if (loc_end > end)
loc_end = end;
+ add = 1;
+ }
+ if (add) {
+ if (rlen >= (ranges_size-2)) {
+ ranges_size += MEM_RANGE_CHUNK_SZ;
+ ranges = realloc(ranges, ranges_size*8);
+ if (!ranges)
+ die("unrecoverable error: can't realloc"
+ "%d bytes for ranges.\n",
+ ranges_size*8);
+ }
ranges[rlen++] = loc_base;
ranges[rlen++] = loc_end - loc_base;
}
@@ -283,7 +340,8 @@ static void add_usable_mem_property(int fd, size_t len)
*dt++ = propnum("linux,usable-memory");
if ((rlen >= 8) && ((unsigned long)dt & 0x4))
dt++;
- memcpy(dt,&ranges,rlen);
+ memcpy(dt, ranges, rlen);
+ free(ranges);
dt += (rlen + 3)/4;
}
--
1.6.3.3
^ permalink raw reply related
* Re: [PATCH] pte_fsl_booke: fix instruction TLB error permission check
From: Benjamin Herrenschmidt @ 2010-05-14 3:36 UTC (permalink / raw)
To: Kumar Gala; +Cc: linuxppc-dev, Jin Qing
In-Reply-To: <0710E9DF-66DF-4D24-95DE-A04E6D869826@kernel.crashing.org>
On Thu, 2010-05-13 at 14:14 -0500, Kumar Gala wrote:
> On May 7, 2010, at 3:38 AM, Li Yang wrote:
>
> > Check the user/supervisor execution permission base on the code address.
> > This fixes the following oops on module loading or removing.
> >
> > Unable to handle kernel paging request for instruction fetch
> > Faulting instruction address: 0xf938d040
> > Oops: Kernel access of bad area, sig: 11 [#1]
> >
> > Signed-off-by: Li Yang <leoli@freescale.com>
> > Signed-off-by: Jin Qing <b24347@freescale.com>
> > ---
> > arch/powerpc/kernel/head_fsl_booke.S | 13 ++++++++++---
> > 1 files changed, 10 insertions(+), 3 deletions(-)
>
> applied to merge.
>
> I've updated the commit message to be a bit more clear on why we need to do this.
>
> Thanks for figuring this out.
Should I pull & send to Linus ?
Cheers,
Ben.
^ permalink raw reply
* RE: [PATCH] pte_fsl_booke: fix instruction TLB error permission check
From: Benjamin Herrenschmidt @ 2010-05-14 3:42 UTC (permalink / raw)
To: Liu Dave-R63238; +Cc: Jin Qing-B24347, linuxppc-dev, Li Yang-R58472
In-Reply-To: <D7CCA83BB0796C49BC0BB53B6AB12089B974F9@zch01exm21.fsl.freescale.net>
On Fri, 2010-05-14 at 08:53 +0800, Liu Dave-R63238 wrote:
> > I've updated the commit message to be a bit more clear on why
> > we need to do this.
>
> I'm curious why the _PAGE_EXEC have different definition in pte-book3e.h
> and pte-fsl-booke.h?
>
> It is UX permission in pte-book3e, but is SX permission in
> pte-fsl-booke.h.
Oh well, there's a whole history here :-)
I'm not 100% sure of the "old" fsl-booke, but pte-book3e uses UX since
that's what _PAGE_EXEC really represents from a kernel standpoint,
user-execute.
On book3e, we need to account for the HW tablewalk, which means that the
PTE can be loaded as-is by the HW. We thus don't get a chance to test
whether we are user or supervisor or change the bits accordingly. We
thus need _PAGE_EXEC to fall right into UX since that's really what the
kernel manipulates it for.
It also means that user pages will not get execute permission in
supervisor mode which is a good thing :-)
For kernel-execute, we have a specific permission, which should be
obtained via PAGE_KERNEL_EXEC in vmalloc_exec().
Cheers,
Ben.
^ permalink raw reply
* [PATCH 1/2] powerpc, kexec: Fix race in kexec shutdown
From: Michael Neuling @ 2010-05-14 3:57 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: linuxppc-dev, kexec, Anton Blanchard, jlarrew
In-Reply-To: <1273559307.55666.645438163872.qpush@pale>
In kexec_prepare_cpus, the primary CPU IPIs the secondary CPUs to
kexec_smp_down(). kexec_smp_down() calls kexec_smp_wait() which sets
the hw_cpu_id() to -1. The primary does this while leaving IRQs on
which means the primary can take a timer interrupt which can lead to
the IPIing one of the secondary CPUs (say, for a scheduler re-balance)
but since the secondary CPU now has a hw_cpu_id = -1, we IPI CPU
-1... Kaboom!
We are hitting this case regularly on POWER7 machines.
There is also a second race, where the primary will tear down the MMU
mappings before knowing the secondaries have entered real mode.
Also, the secondaries are clearing out any pending IPIs before
guaranteeing that no more will be received.
This changes kexec_prepare_cpus() so that we turn off IRQs in the
primary CPU much earlier. It adds a paca flag to say that the
secondaries have entered the kexec_smp_down() IPI and turned off IRQs,
rather than overloading hw_cpu_id with -1. This new paca flag is
again used to in indicate when the secondaries has entered real mode.
It also ensures that all CPUs have their IRQs off before we clear out
any pending IPI requests (in kexec_cpu_down()) to ensure there are no
trailing IPIs left unacknowledged.
Signed-off-by: Michael Neuling <mikey@neuling.org>
---
arch/powerpc/include/asm/kexec.h | 4 ++
arch/powerpc/include/asm/paca.h | 1
arch/powerpc/kernel/asm-offsets.c | 1
arch/powerpc/kernel/machine_kexec_64.c | 48 +++++++++++++++++++++++----------
arch/powerpc/kernel/misc_64.S | 8 +++--
arch/powerpc/kernel/paca.c | 2 +
6 files changed, 47 insertions(+), 17 deletions(-)
Index: linux-2.6-ozlabs/arch/powerpc/include/asm/kexec.h
===================================================================
--- linux-2.6-ozlabs.orig/arch/powerpc/include/asm/kexec.h
+++ linux-2.6-ozlabs/arch/powerpc/include/asm/kexec.h
@@ -31,6 +31,10 @@
#define KEXEC_ARCH KEXEC_ARCH_PPC
#endif
+#define KEXEC_STATE_NONE 0
+#define KEXEC_STATE_IRQS_OFF 1
+#define KEXEC_STATE_REAL_MODE 2
+
#ifndef __ASSEMBLY__
#include <linux/cpumask.h>
#include <asm/reg.h>
Index: linux-2.6-ozlabs/arch/powerpc/include/asm/paca.h
===================================================================
--- linux-2.6-ozlabs.orig/arch/powerpc/include/asm/paca.h
+++ linux-2.6-ozlabs/arch/powerpc/include/asm/paca.h
@@ -82,6 +82,7 @@ struct paca_struct {
s16 hw_cpu_id; /* Physical processor number */
u8 cpu_start; /* At startup, processor spins until */
/* this becomes non-zero. */
+ u8 kexec_state; /* set when kexec down has irqs off */
#ifdef CONFIG_PPC_STD_MMU_64
struct slb_shadow *slb_shadow_ptr;
Index: linux-2.6-ozlabs/arch/powerpc/kernel/asm-offsets.c
===================================================================
--- linux-2.6-ozlabs.orig/arch/powerpc/kernel/asm-offsets.c
+++ linux-2.6-ozlabs/arch/powerpc/kernel/asm-offsets.c
@@ -184,6 +184,7 @@ int main(void)
#endif /* CONFIG_PPC_STD_MMU_64 */
DEFINE(PACAEMERGSP, offsetof(struct paca_struct, emergency_sp));
DEFINE(PACAHWCPUID, offsetof(struct paca_struct, hw_cpu_id));
+ DEFINE(PACAKEXECSTATE, offsetof(struct paca_struct, kexec_state));
DEFINE(PACA_STARTPURR, offsetof(struct paca_struct, startpurr));
DEFINE(PACA_STARTSPURR, offsetof(struct paca_struct, startspurr));
DEFINE(PACA_USER_TIME, offsetof(struct paca_struct, user_time));
Index: linux-2.6-ozlabs/arch/powerpc/kernel/machine_kexec_64.c
===================================================================
--- linux-2.6-ozlabs.orig/arch/powerpc/kernel/machine_kexec_64.c
+++ linux-2.6-ozlabs/arch/powerpc/kernel/machine_kexec_64.c
@@ -155,33 +155,38 @@ void kexec_copy_flush(struct kimage *ima
#ifdef CONFIG_SMP
-/* FIXME: we should schedule this function to be called on all cpus based
- * on calling the interrupts, but we would like to call it off irq level
- * so that the interrupt controller is clean.
- */
+static int kexec_all_irq_disabled = 0;
+
static void kexec_smp_down(void *arg)
{
+ local_irq_disable();
+ mb(); /* make sure our irqs are disabled before we say they are */
+ get_paca()->kexec_state = KEXEC_STATE_IRQS_OFF;
+ while(kexec_all_irq_disabled == 0)
+ cpu_relax();
+ mb(); /* make sure all irqs are disabled before this */
+ /*
+ * Now every CPU has IRQs off, we can clear out any pending
+ * IPIs and be sure that no more will come in after this.
+ */
if (ppc_md.kexec_cpu_down)
ppc_md.kexec_cpu_down(0, 1);
- local_irq_disable();
kexec_smp_wait();
/* NOTREACHED */
}
-static void kexec_prepare_cpus(void)
+static void kexec_prepare_cpus_wait(int wait_state)
{
int my_cpu, i, notified=-1;
- smp_call_function(kexec_smp_down, NULL, /* wait */0);
my_cpu = get_cpu();
-
- /* check the others cpus are now down (via paca hw cpu id == -1) */
+ /* check the others cpus are now down (via paca kexec_irqs_off == 1) */
for (i=0; i < NR_CPUS; i++) {
if (i == my_cpu)
continue;
- while (paca[i].hw_cpu_id != -1) {
+ while (paca[i].kexec_state < wait_state) {
barrier();
if (!cpu_possible(i)) {
printk("kexec: cpu %d hw_cpu_id %d is not"
@@ -201,20 +206,35 @@ static void kexec_prepare_cpus(void)
}
if (i != notified) {
printk( "kexec: waiting for cpu %d (physical"
- " %d) to go down\n",
- i, paca[i].hw_cpu_id);
+ " %d) to enter %i state\n",
+ i, paca[i].hw_cpu_id, wait_state);
notified = i;
}
}
}
+ mb();
+}
+
+static void kexec_prepare_cpus(void)
+{
+
+ smp_call_function(kexec_smp_down, NULL, /* wait */0);
+ local_irq_disable();
+ mb(); /* make sure IRQs are disabled before we say they are */
+ get_paca()->kexec_state = KEXEC_STATE_IRQS_OFF;
+
+ kexec_prepare_cpus_wait(KEXEC_STATE_IRQS_OFF);
+ /* we are sure every CPU has IRQs off at this point */
+ kexec_all_irq_disabled = 1;
/* after we tell the others to go down */
if (ppc_md.kexec_cpu_down)
ppc_md.kexec_cpu_down(0, 0);
- put_cpu();
+ /* Before removing MMU mapings make sure all CPUs have entered real mode */
+ kexec_prepare_cpus_wait(KEXEC_STATE_REAL_MODE);
- local_irq_disable();
+ put_cpu();
}
#else /* ! SMP */
Index: linux-2.6-ozlabs/arch/powerpc/kernel/misc_64.S
===================================================================
--- linux-2.6-ozlabs.orig/arch/powerpc/kernel/misc_64.S
+++ linux-2.6-ozlabs/arch/powerpc/kernel/misc_64.S
@@ -24,6 +24,7 @@
#include <asm/asm-offsets.h>
#include <asm/cputable.h>
#include <asm/thread_info.h>
+#include <asm/kexec.h>
.text
@@ -471,6 +472,10 @@ _GLOBAL(kexec_wait)
1: mflr r5
addi r5,r5,kexec_flag-1b
+ li r4,KEXEC_STATE_REAL_MODE
+ stb r4,PACAKEXECSTATE(r13)
+ SYNC
+
99: HMT_LOW
#ifdef CONFIG_KEXEC /* use no memory without kexec */
lwz r4,0(r5)
@@ -494,14 +499,11 @@ kexec_flag:
* note: this is a terminal routine, it does not save lr
*
* get phys id from paca
- * set paca id to -1 to say we got here
* switch to real mode
* join other cpus in kexec_wait(phys_id)
*/
_GLOBAL(kexec_smp_wait)
lhz r3,PACAHWCPUID(r13)
- li r4,-1
- sth r4,PACAHWCPUID(r13) /* let others know we left */
bl real_mode
b .kexec_wait
Index: linux-2.6-ozlabs/arch/powerpc/kernel/paca.c
===================================================================
--- linux-2.6-ozlabs.orig/arch/powerpc/kernel/paca.c
+++ linux-2.6-ozlabs/arch/powerpc/kernel/paca.c
@@ -18,6 +18,7 @@
#include <asm/pgtable.h>
#include <asm/iseries/lpar_map.h>
#include <asm/iseries/hv_types.h>
+#include <asm/kexec.h>
/* This symbol is provided by the linker - let it fill in the paca
* field correctly */
@@ -97,6 +98,7 @@ void __init initialise_paca(struct paca_
new_paca->kernelbase = (unsigned long) _stext;
new_paca->kernel_msr = MSR_KERNEL;
new_paca->hw_cpu_id = 0xffff;
+ new_paca->kexec_state = KEXEC_STATE_NONE;
new_paca->__current = &init_task;
#ifdef CONFIG_PPC_STD_MMU_64
new_paca->slb_shadow_ptr = &slb_shadow[cpu];
^ permalink raw reply
* [PATCH 2/2] powerpc, kdump: Fix race in kdump shutdown
From: Michael Neuling @ 2010-05-14 3:57 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: linuxppc-dev, kexec, Anton Blanchard, jlarrew
In-Reply-To: <1273559307.55666.645438163872.qpush@pale>
When we are crashing, the crashing/primary CPU IPIs the secondaries to
turn off IRQs, go into real mode and wait in kexec_wait. While this
is happening, the primary tears down all the MMU maps. Unfortunately
the primary doesn't check to make sure the secondaries have entered
real mode before doing this.
On PHYP machines, the secondaries can take a long time shutting down
the IRQ controller as RTAS calls are need. These RTAS calls need to
be serialised which resilts in the secondaries contending in
lock_rtas() and hence taking a long time to shut down.
We've hit this on large POWER7 machines, where some secondaries are
still waiting in lock_rtas(), when the primary tears down the HPTEs.
This patch makes sure all secondaries are in real mode before the
primary tears down the MMU. It uses the new kexec_state entry in the
paca. It times out if the secondaries don't reach real mode after
10sec.
Signed-off-by: Michael Neuling <mikey@neuling.org>
---
arch/powerpc/kernel/crash.c | 28 ++++++++++++++++++++++++++++
1 file changed, 28 insertions(+)
Index: linux-2.6-ozlabs/arch/powerpc/kernel/crash.c
===================================================================
--- linux-2.6-ozlabs.orig/arch/powerpc/kernel/crash.c
+++ linux-2.6-ozlabs/arch/powerpc/kernel/crash.c
@@ -162,6 +162,33 @@ static void crash_kexec_prepare_cpus(int
/* Leave the IPI callback set */
}
+/* wait for all the CPUs to hit real mode but timeout if they don't come in */
+static void crash_kexec_wait_realmode(int cpu)
+{
+ unsigned int msecs;
+ int i;
+
+ /* check the others cpus are now down (via paca kexec_irqs_off == 1) */
+ msecs = 10000;
+ for (i=0; i < NR_CPUS && msecs > 0; i++) {
+ if (i == cpu)
+ continue;
+
+ while (paca[i].kexec_state < KEXEC_STATE_REAL_MODE) {
+ barrier();
+ if (!cpu_possible(i)) {
+ break;
+ }
+ if (!cpu_online(i)) {
+ break;
+ }
+ msecs--;
+ mdelay(1);
+ }
+ }
+ mb();
+}
+
/*
* This function will be called by secondary cpus or by kexec cpu
* if soft-reset is activated to stop some CPUs.
@@ -419,6 +446,7 @@ void default_machine_crash_shutdown(stru
crash_kexec_prepare_cpus(crashing_cpu);
cpu_set(crashing_cpu, cpus_in_crash);
crash_kexec_stop_spus();
+ crash_kexec_wait_realmode(crashing_cpu);
if (ppc_md.kexec_cpu_down)
ppc_md.kexec_cpu_down(1, 0);
}
^ permalink raw reply
* Re: [PATCH] pte_fsl_booke: fix instruction TLB error permission check
From: Kumar Gala @ 2010-05-14 4:19 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: linuxppc-dev, Jin Qing
In-Reply-To: <1273808175.21352.481.camel@pasglop>
On May 13, 2010, at 10:36 PM, Benjamin Herrenschmidt wrote:
> On Thu, 2010-05-13 at 14:14 -0500, Kumar Gala wrote:
>> On May 7, 2010, at 3:38 AM, Li Yang wrote:
>>=20
>>> Check the user/supervisor execution permission base on the code =
address.
>>> This fixes the following oops on module loading or removing.
>>>=20
>>> Unable to handle kernel paging request for instruction fetch
>>> Faulting instruction address: 0xf938d040
>>> Oops: Kernel access of bad area, sig: 11 [#1]
>>>=20
>>> Signed-off-by: Li Yang <leoli@freescale.com>
>>> Signed-off-by: Jin Qing <b24347@freescale.com>
>>> ---
>>> arch/powerpc/kernel/head_fsl_booke.S | 13 ++++++++++---
>>> 1 files changed, 10 insertions(+), 3 deletions(-)
>>=20
>> applied to merge.
>>=20
>> I've updated the commit message to be a bit more clear on why we need =
to do this.
>>=20
>> Thanks for figuring this out.
>=20
> Should I pull & send to Linus ?
No, I've got another patch to fix a FTRACE issue with SMP CPU bringup. =
Will post a tree for you here shortly.
- k=
^ permalink raw reply
* [PATCH] powerpc/fsl-booke: Move loadcam_entry back to asm code to fix SMP ftrace
From: Kumar Gala @ 2010-05-14 4:25 UTC (permalink / raw)
To: linuxppc-dev
When we build with ftrace enabled its possible that loadcam_entry would
have used the stack pointer (even though the code doesn't need it). We
call loadcam_entry in __secondary_start before the stack is setup. To
ensure that loadcam_entry doesn't use the stack pointer the easiest
solution is to just have it in asm code.
Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
---
arch/powerpc/kernel/asm-offsets.c | 8 ++++++++
arch/powerpc/mm/fsl_booke_mmu.c | 25 +++----------------------
arch/powerpc/mm/mmu_decl.h | 10 +++++++++-
arch/powerpc/mm/tlb_nohash_low.S | 28 ++++++++++++++++++++++++++++
4 files changed, 48 insertions(+), 23 deletions(-)
diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-offsets.c
index c09138d..b894721 100644
--- a/arch/powerpc/kernel/asm-offsets.c
+++ b/arch/powerpc/kernel/asm-offsets.c
@@ -447,6 +447,14 @@ int main(void)
DEFINE(PGD_T_LOG2, PGD_T_LOG2);
DEFINE(PTE_T_LOG2, PTE_T_LOG2);
#endif
+#ifdef CONFIG_FSL_BOOKE
+ DEFINE(TLBCAM_SIZE, sizeof(struct tlbcam));
+ DEFINE(TLBCAM_MAS0, offsetof(struct tlbcam, MAS0));
+ DEFINE(TLBCAM_MAS1, offsetof(struct tlbcam, MAS1));
+ DEFINE(TLBCAM_MAS2, offsetof(struct tlbcam, MAS2));
+ DEFINE(TLBCAM_MAS3, offsetof(struct tlbcam, MAS3));
+ DEFINE(TLBCAM_MAS7, offsetof(struct tlbcam, MAS7));
+#endif
#ifdef CONFIG_KVM_EXIT_TIMING
DEFINE(VCPU_TIMING_EXIT_TBU, offsetof(struct kvm_vcpu,
diff --git a/arch/powerpc/mm/fsl_booke_mmu.c b/arch/powerpc/mm/fsl_booke_mmu.c
index 1ed6b52..cdc7526 100644
--- a/arch/powerpc/mm/fsl_booke_mmu.c
+++ b/arch/powerpc/mm/fsl_booke_mmu.c
@@ -2,7 +2,7 @@
* Modifications by Kumar Gala (galak@kernel.crashing.org) to support
* E500 Book E processors.
*
- * Copyright 2004 Freescale Semiconductor, Inc
+ * Copyright 2004,2010 Freescale Semiconductor, Inc.
*
* This file contains the routines for initializing the MMU
* on the 4xx series of chips.
@@ -56,19 +56,13 @@
unsigned int tlbcam_index;
-#define NUM_TLBCAMS (64)
#if defined(CONFIG_LOWMEM_CAM_NUM_BOOL) && (CONFIG_LOWMEM_CAM_NUM >= NUM_TLBCAMS)
#error "LOWMEM_CAM_NUM must be less than NUM_TLBCAMS"
#endif
-struct tlbcam {
- u32 MAS0;
- u32 MAS1;
- unsigned long MAS2;
- u32 MAS3;
- u32 MAS7;
-} TLBCAM[NUM_TLBCAMS];
+#define NUM_TLBCAMS (64)
+struct tlbcam TLBCAM[NUM_TLBCAMS];
struct tlbcamrange {
unsigned long start;
@@ -109,19 +103,6 @@ unsigned long p_mapped_by_tlbcam(phys_addr_t pa)
return 0;
}
-void loadcam_entry(int idx)
-{
- mtspr(SPRN_MAS0, TLBCAM[idx].MAS0);
- mtspr(SPRN_MAS1, TLBCAM[idx].MAS1);
- mtspr(SPRN_MAS2, TLBCAM[idx].MAS2);
- mtspr(SPRN_MAS3, TLBCAM[idx].MAS3);
-
- if (mmu_has_feature(MMU_FTR_BIG_PHYS))
- mtspr(SPRN_MAS7, TLBCAM[idx].MAS7);
-
- asm volatile("isync;tlbwe;isync" : : : "memory");
-}
-
/*
* Set up one of the I/D BAT (block address translation) register pairs.
* The parameters are not checked; in particular size must be a power
diff --git a/arch/powerpc/mm/mmu_decl.h b/arch/powerpc/mm/mmu_decl.h
index d49a775..0591f25 100644
--- a/arch/powerpc/mm/mmu_decl.h
+++ b/arch/powerpc/mm/mmu_decl.h
@@ -149,7 +149,15 @@ extern unsigned long mmu_mapin_ram(unsigned long top);
extern void MMU_init_hw(void);
extern unsigned long mmu_mapin_ram(unsigned long top);
extern void adjust_total_lowmem(void);
-
+extern void loadcam_entry(unsigned int index);
+
+struct tlbcam {
+ u32 MAS0;
+ u32 MAS1;
+ unsigned long MAS2;
+ u32 MAS3;
+ u32 MAS7;
+};
#elif defined(CONFIG_PPC32)
/* anything 32-bit except 4xx or 8xx */
extern void MMU_init_hw(void);
diff --git a/arch/powerpc/mm/tlb_nohash_low.S b/arch/powerpc/mm/tlb_nohash_low.S
index bbdc5b5..8656ecf 100644
--- a/arch/powerpc/mm/tlb_nohash_low.S
+++ b/arch/powerpc/mm/tlb_nohash_low.S
@@ -271,3 +271,31 @@ _GLOBAL(set_context)
#else
#error Unsupported processor type !
#endif
+
+#if defined(CONFIG_FSL_BOOKE)
+/*
+ * extern void loadcam_entry(unsigned int index)
+ *
+ * Load TLBCAM[index] entry in to the L2 CAM MMU
+ */
+_GLOBAL(loadcam_entry)
+ LOAD_REG_ADDR(r4, TLBCAM)
+ mulli r5,r3,TLBCAM_SIZE
+ add r3,r5,r4
+ lwz r4,TLBCAM_MAS0(r3)
+ mtspr SPRN_MAS0,r4
+ lwz r4,TLBCAM_MAS1(r3)
+ mtspr SPRN_MAS1,r4
+ PPC_LL r4,TLBCAM_MAS2(r3)
+ mtspr SPRN_MAS2,r4
+ lwz r4,TLBCAM_MAS3(r3)
+ mtspr SPRN_MAS3,r4
+BEGIN_MMU_FTR_SECTION
+ lwz r4,TLBCAM_MAS7(r3)
+ mtspr SPRN_MAS7,r4
+END_MMU_FTR_SECTION_IFSET(MMU_FTR_BIG_PHYS)
+ isync
+ tlbwe
+ isync
+ blr
+#endif
--
1.6.0.6
^ permalink raw reply related
* [git pull] Please pull powerpc.git merge branch
From: Kumar Gala @ 2010-05-14 4:28 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: linuxppc-dev
The following changes since commit 131c6c9eddfa252e376edb4aeff9c7fe1b96a798:
Benjamin Herrenschmidt (1):
Merge commit 'kumar/merge' into merge
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/galak/powerpc.git merge
Kumar Gala (1):
powerpc/fsl-booke: Move loadcam_entry back to asm code to fix SMP ftrace
Li Yang (1):
powerpc/fsl-booke: Fix InstructionTLBError execute permission check
arch/powerpc/kernel/asm-offsets.c | 8 ++++++++
arch/powerpc/kernel/head_fsl_booke.S | 13 ++++++++++---
arch/powerpc/mm/fsl_booke_mmu.c | 25 +++----------------------
arch/powerpc/mm/mmu_decl.h | 10 +++++++++-
arch/powerpc/mm/tlb_nohash_low.S | 28 ++++++++++++++++++++++++++++
5 files changed, 58 insertions(+), 26 deletions(-)
^ permalink raw reply
* [PATCH 1/2] powerpc, kexec: Fix race in kexec shutdown
From: Michael Neuling @ 2010-05-14 5:40 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: linuxppc-dev, kexec, Anton Blanchard, jlarrew
In-Reply-To: <20100514035700.8B6EDCED6E@localhost.localdomain>
In kexec_prepare_cpus, the primary CPU IPIs the secondary CPUs to
kexec_smp_down(). kexec_smp_down() calls kexec_smp_wait() which sets
the hw_cpu_id() to -1. The primary does this while leaving IRQs on
which means the primary can take a timer interrupt which can lead to
the IPIing one of the secondary CPUs (say, for a scheduler re-balance)
but since the secondary CPU now has a hw_cpu_id = -1, we IPI CPU
-1... Kaboom!
We are hitting this case regularly on POWER7 machines.
There is also a second race, where the primary will tear down the MMU
mappings before knowing the secondaries have entered real mode.
Also, the secondaries are clearing out any pending IPIs before
guaranteeing that no more will be received.
This changes kexec_prepare_cpus() so that we turn off IRQs in the
primary CPU much earlier. It adds a paca flag to say that the
secondaries have entered the kexec_smp_down() IPI and turned off IRQs,
rather than overloading hw_cpu_id with -1. This new paca flag is
again used to in indicate when the secondaries has entered real mode.
It also ensures that all CPUs have their IRQs off before we clear out
any pending IPI requests (in kexec_cpu_down()) to ensure there are no
trailing IPIs left unacknowledged.
Signed-off-by: Michael Neuling <mikey@neuling.org>
---
Oops, missed quilt refresh in the last version...
---
arch/powerpc/include/asm/kexec.h | 4 ++
arch/powerpc/include/asm/paca.h | 1
arch/powerpc/kernel/asm-offsets.c | 1
arch/powerpc/kernel/machine_kexec_64.c | 48 +++++++++++++++++++++++----------
arch/powerpc/kernel/misc_64.S | 8 +++--
arch/powerpc/kernel/paca.c | 2 +
6 files changed, 47 insertions(+), 17 deletions(-)
Index: linux-2.6-ozlabs/arch/powerpc/include/asm/kexec.h
===================================================================
--- linux-2.6-ozlabs.orig/arch/powerpc/include/asm/kexec.h
+++ linux-2.6-ozlabs/arch/powerpc/include/asm/kexec.h
@@ -31,6 +31,10 @@
#define KEXEC_ARCH KEXEC_ARCH_PPC
#endif
+#define KEXEC_STATE_NONE 0
+#define KEXEC_STATE_IRQS_OFF 1
+#define KEXEC_STATE_REAL_MODE 2
+
#ifndef __ASSEMBLY__
#include <linux/cpumask.h>
#include <asm/reg.h>
Index: linux-2.6-ozlabs/arch/powerpc/include/asm/paca.h
===================================================================
--- linux-2.6-ozlabs.orig/arch/powerpc/include/asm/paca.h
+++ linux-2.6-ozlabs/arch/powerpc/include/asm/paca.h
@@ -82,6 +82,7 @@ struct paca_struct {
s16 hw_cpu_id; /* Physical processor number */
u8 cpu_start; /* At startup, processor spins until */
/* this becomes non-zero. */
+ u8 kexec_state; /* set when kexec down has irqs off */
#ifdef CONFIG_PPC_STD_MMU_64
struct slb_shadow *slb_shadow_ptr;
Index: linux-2.6-ozlabs/arch/powerpc/kernel/asm-offsets.c
===================================================================
--- linux-2.6-ozlabs.orig/arch/powerpc/kernel/asm-offsets.c
+++ linux-2.6-ozlabs/arch/powerpc/kernel/asm-offsets.c
@@ -184,6 +184,7 @@ int main(void)
#endif /* CONFIG_PPC_STD_MMU_64 */
DEFINE(PACAEMERGSP, offsetof(struct paca_struct, emergency_sp));
DEFINE(PACAHWCPUID, offsetof(struct paca_struct, hw_cpu_id));
+ DEFINE(PACAKEXECSTATE, offsetof(struct paca_struct, kexec_state));
DEFINE(PACA_STARTPURR, offsetof(struct paca_struct, startpurr));
DEFINE(PACA_STARTSPURR, offsetof(struct paca_struct, startspurr));
DEFINE(PACA_USER_TIME, offsetof(struct paca_struct, user_time));
Index: linux-2.6-ozlabs/arch/powerpc/kernel/machine_kexec_64.c
===================================================================
--- linux-2.6-ozlabs.orig/arch/powerpc/kernel/machine_kexec_64.c
+++ linux-2.6-ozlabs/arch/powerpc/kernel/machine_kexec_64.c
@@ -155,33 +155,38 @@ void kexec_copy_flush(struct kimage *ima
#ifdef CONFIG_SMP
-/* FIXME: we should schedule this function to be called on all cpus based
- * on calling the interrupts, but we would like to call it off irq level
- * so that the interrupt controller is clean.
- */
+static int kexec_all_irq_disabled = 0;
+
static void kexec_smp_down(void *arg)
{
+ local_irq_disable();
+ mb(); /* make sure our irqs are disabled before we say they are */
+ get_paca()->kexec_state = KEXEC_STATE_IRQS_OFF;
+ while(kexec_all_irq_disabled == 0)
+ cpu_relax();
+ mb(); /* make sure all irqs are disabled before this */
+ /*
+ * Now every CPU has IRQs off, we can clear out any pending
+ * IPIs and be sure that no more will come in after this.
+ */
if (ppc_md.kexec_cpu_down)
ppc_md.kexec_cpu_down(0, 1);
- local_irq_disable();
kexec_smp_wait();
/* NOTREACHED */
}
-static void kexec_prepare_cpus(void)
+static void kexec_prepare_cpus_wait(int wait_state)
{
int my_cpu, i, notified=-1;
- smp_call_function(kexec_smp_down, NULL, /* wait */0);
my_cpu = get_cpu();
-
- /* check the others cpus are now down (via paca hw cpu id == -1) */
+ /* Make sure each CPU has atleast made it to the state we need */
for (i=0; i < NR_CPUS; i++) {
if (i == my_cpu)
continue;
- while (paca[i].hw_cpu_id != -1) {
+ while (paca[i].kexec_state < wait_state) {
barrier();
if (!cpu_possible(i)) {
printk("kexec: cpu %d hw_cpu_id %d is not"
@@ -201,20 +206,35 @@ static void kexec_prepare_cpus(void)
}
if (i != notified) {
printk( "kexec: waiting for cpu %d (physical"
- " %d) to go down\n",
- i, paca[i].hw_cpu_id);
+ " %d) to enter %i state\n",
+ i, paca[i].hw_cpu_id, wait_state);
notified = i;
}
}
}
+ mb();
+}
+
+static void kexec_prepare_cpus(void)
+{
+
+ smp_call_function(kexec_smp_down, NULL, /* wait */0);
+ local_irq_disable();
+ mb(); /* make sure IRQs are disabled before we say they are */
+ get_paca()->kexec_state = KEXEC_STATE_IRQS_OFF;
+
+ kexec_prepare_cpus_wait(KEXEC_STATE_IRQS_OFF);
+ /* we are sure every CPU has IRQs off at this point */
+ kexec_all_irq_disabled = 1;
/* after we tell the others to go down */
if (ppc_md.kexec_cpu_down)
ppc_md.kexec_cpu_down(0, 0);
- put_cpu();
+ /* Before removing MMU mapings make sure all CPUs have entered real mode */
+ kexec_prepare_cpus_wait(KEXEC_STATE_REAL_MODE);
- local_irq_disable();
+ put_cpu();
}
#else /* ! SMP */
Index: linux-2.6-ozlabs/arch/powerpc/kernel/misc_64.S
===================================================================
--- linux-2.6-ozlabs.orig/arch/powerpc/kernel/misc_64.S
+++ linux-2.6-ozlabs/arch/powerpc/kernel/misc_64.S
@@ -24,6 +24,7 @@
#include <asm/asm-offsets.h>
#include <asm/cputable.h>
#include <asm/thread_info.h>
+#include <asm/kexec.h>
.text
@@ -471,6 +472,10 @@ _GLOBAL(kexec_wait)
1: mflr r5
addi r5,r5,kexec_flag-1b
+ li r4,KEXEC_STATE_REAL_MODE
+ stb r4,PACAKEXECSTATE(r13)
+ SYNC
+
99: HMT_LOW
#ifdef CONFIG_KEXEC /* use no memory without kexec */
lwz r4,0(r5)
@@ -494,14 +499,11 @@ kexec_flag:
* note: this is a terminal routine, it does not save lr
*
* get phys id from paca
- * set paca id to -1 to say we got here
* switch to real mode
* join other cpus in kexec_wait(phys_id)
*/
_GLOBAL(kexec_smp_wait)
lhz r3,PACAHWCPUID(r13)
- li r4,-1
- sth r4,PACAHWCPUID(r13) /* let others know we left */
bl real_mode
b .kexec_wait
Index: linux-2.6-ozlabs/arch/powerpc/kernel/paca.c
===================================================================
--- linux-2.6-ozlabs.orig/arch/powerpc/kernel/paca.c
+++ linux-2.6-ozlabs/arch/powerpc/kernel/paca.c
@@ -18,6 +18,7 @@
#include <asm/pgtable.h>
#include <asm/iseries/lpar_map.h>
#include <asm/iseries/hv_types.h>
+#include <asm/kexec.h>
/* This symbol is provided by the linker - let it fill in the paca
* field correctly */
@@ -97,6 +98,7 @@ void __init initialise_paca(struct paca_
new_paca->kernelbase = (unsigned long) _stext;
new_paca->kernel_msr = MSR_KERNEL;
new_paca->hw_cpu_id = 0xffff;
+ new_paca->kexec_state = KEXEC_STATE_NONE;
new_paca->__current = &init_task;
#ifdef CONFIG_PPC_STD_MMU_64
new_paca->slb_shadow_ptr = &slb_shadow[cpu];
^ permalink raw reply
* [PATCH 2/2] powerpc, kdump: Fix race in kdump shutdown
From: Michael Neuling @ 2010-05-14 5:40 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: linuxppc-dev, kexec, Anton Blanchard, jlarrew
In-Reply-To: <20100514035700.8B6EDCED6E@localhost.localdomain>
When we are crashing, the crashing/primary CPU IPIs the secondaries to
turn off IRQs, go into real mode and wait in kexec_wait. While this
is happening, the primary tears down all the MMU maps. Unfortunately
the primary doesn't check to make sure the secondaries have entered
real mode before doing this.
On PHYP machines, the secondaries can take a long time shutting down
the IRQ controller as RTAS calls are need. These RTAS calls need to
be serialised which resilts in the secondaries contending in
lock_rtas() and hence taking a long time to shut down.
We've hit this on large POWER7 machines, where some secondaries are
still waiting in lock_rtas(), when the primary tears down the HPTEs.
This patch makes sure all secondaries are in real mode before the
primary tears down the MMU. It uses the new kexec_state entry in the
paca. It times out if the secondaries don't reach real mode after
10sec.
Signed-off-by: Michael Neuling <mikey@neuling.org>
---
arch/powerpc/kernel/crash.c | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)
Index: linux-2.6-ozlabs/arch/powerpc/kernel/crash.c
===================================================================
--- linux-2.6-ozlabs.orig/arch/powerpc/kernel/crash.c
+++ linux-2.6-ozlabs/arch/powerpc/kernel/crash.c
@@ -162,6 +162,32 @@ static void crash_kexec_prepare_cpus(int
/* Leave the IPI callback set */
}
+/* wait for all the CPUs to hit real mode but timeout if they don't come in */
+static void crash_kexec_wait_realmode(int cpu)
+{
+ unsigned int msecs;
+ int i;
+
+ msecs = 10000;
+ for (i=0; i < NR_CPUS && msecs > 0; i++) {
+ if (i == cpu)
+ continue;
+
+ while (paca[i].kexec_state < KEXEC_STATE_REAL_MODE) {
+ barrier();
+ if (!cpu_possible(i)) {
+ break;
+ }
+ if (!cpu_online(i)) {
+ break;
+ }
+ msecs--;
+ mdelay(1);
+ }
+ }
+ mb();
+}
+
/*
* This function will be called by secondary cpus or by kexec cpu
* if soft-reset is activated to stop some CPUs.
@@ -412,6 +438,7 @@ void default_machine_crash_shutdown(stru
crash_kexec_prepare_cpus(crashing_cpu);
cpu_set(crashing_cpu, cpus_in_crash);
crash_kexec_stop_spus();
+ crash_kexec_wait_realmode(crashing_cpu);
if (ppc_md.kexec_cpu_down)
ppc_md.kexec_cpu_down(1, 0);
}
^ permalink raw reply
* Re: [PATCH] media/IR: Add missing include file to rc-map.c
From: Paul Mundt @ 2010-05-14 6:02 UTC (permalink / raw)
To: Peter H?we
Cc: linux-mips, linux-sh, linux-kernel, David H?rdeman,
Mauro Carvalho Chehab, linuxppc-dev, linux-m68k, linux-media
In-Reply-To: <201005112042.14889.PeterHuewe@gmx.de>
On Tue, May 11, 2010 at 08:42:14PM +0200, Peter H?we wrote:
> Am Mittwoch 05 Mai 2010 17:20:21 schrieb Peter H?we:
> > From: Peter Huewe <peterhuewe@gmx.de>
> >
> > This patch adds a missing include linux/delay.h to prevent
> > build failures[1-5]
> >
> > Signed-off-by: Peter Huewe <peterhuewe@gmx.de>
> > ---
> Any updates on this patch?
> Issue still exists with today's linux-next tree
>
You might want to send this to the linux-next list at least. If the
people who introduced the breakage are unresponsive (as often tends to be
the case with -next) it's still worth getting trivial fixes rolled in for
the interim. This change doesn't exist outside of -next and whatever tree
introduced it, so there's not much else anyone can do about it at
present.
^ permalink raw reply
* Re: [Patch 2/2] PPC64-HWBKPT: Implement hw-breakpoints for PPC64
From: K.Prasad @ 2010-05-14 6:55 UTC (permalink / raw)
To: Paul Mackerras
Cc: Michael Neuling, Benjamin Herrenschmidt, shaggy,
Frederic Weisbecker, David Gibson, linuxppc-dev@ozlabs.org,
Alan Stern, Roland McGrath
In-Reply-To: <20100512063247.GC3548@drongo>
On Wed, May 12, 2010 at 04:32:47PM +1000, Paul Mackerras wrote:
> On Wed, May 05, 2010 at 02:03:03AM +0530, K.Prasad wrote:
>
> > It is true that the breakpoint exceptions will go amiss following the
> > alignment exception, and be restored when the thread single-steps due
> > to other requests causing undesirable effects. (Borrowing from some of
> > the discussions I had with BenH, earlier) There can be two ways of
> > changing the implementation to counter it:
> >
> > - To sense that the impending exception (alignment, page-fault,
> > single-step) is a successor to a hw-breakpoint exception (and that
> > restoration of debug register values is necessary), somewhere early in
> > exceptions-64s.S and jump to a common handler, say
> > do_single_step_dabr() which does a majority of
> > single_step_dabr_instruction().
> > - To modify emulate_single_step() to also do a notify_die(DIE_SSTEP,...)
> > in addition to its existing code. This would invoke
> > single_step_dabr_instruction() where the breakpoints can be restored.
>
> I thought you would change the explicit regs->msr modification in
> single_step_exception() to clear_single_step(), then just make
> emulate_single_step() call single_step_exception().
>
Okay. I will re-use single_step_exception() after modifications; it
appearsto have no in-kernel users for it. (single_step_exception() clears
MSR more than what clear_single_step() does, it shouldn't matter though).
> > I must admit that it is not clear to me when you say "doesn't return to
> > the instruction" and "instruction has been changed underneath". Are you
>
> Suppose the address at which the data breakpoint has been unmapped,
> and the process has a handler for the SIGSEGV signal. When we try to
> single-step the load or store, we will get a DSI (0x300) interrupt,
> call into do_page_fault, and end up sending the process a SIGSEGV.
> That will invoke the signal handler, which can then do anything it
> likes. It can do a blocking system call, it can longjmp() back into
> its main event, or it can return from the signal handler. Only in the
> last case will it retry the load or store, and then only if the signal
> handler hasn't modified the NIP value in the signal frame. That's
> what I mean by "doesn't return to the instruction".
>
At the outset, this seemed to be a scary thing to happen; but turns out
to be harmful only to the extent of generating a false hw-breakpoint
exception in certain cases. A case-by-case basis analysis reveals thus:
Consider an instruction stream i1, i2, i3, ... iN, where i1 has
finished execution and i2 is about to be executed but has generated a
DSI interrupt with the above-mentioned conditions i.e. DSISR indicates a
DABR match + Page-Table entry not found. Now according to do_hash_page
in exception-64s.S (as pasted below), do_page_fault() and do_dabr() are
invoked one after the other.
_STATIC(do_hash_page)
std r3,_DAR(r1)
std r4,_DSISR(r1)
andis. r0,r4,0xa410 /* weird error? */
bne- handle_page_fault /* if not, try to insert a HPTE */
andis. r0,r4,DSISR_DABRMATCH@h
bne- handle_dabr_fault
Thus, when control returns to user-space to instruction 'i2', the
hw_breakpoint_handler() has completed execution, and a SIGSEGV is pending
to be delivered and single-stepping enabled MSR_SE is set. Upon return to
user-space, the handler for SIGSEGV is executed and it may perform one of
the following (as you stated previously):
(a) Make a blocking syscall, eventually yielding the CPU to a new thread
(b) Jump to a different instruction in user-space, say iN, and not complete
the execution of instruction i2 at all.
(c) Return to instruction i2 and complete the execution.
In case of (a), the context-switches should not affect the ability to
single-step the instruction when the thread is eventually scheduled to
run. The thread, when scheduled onto the CPU will complete signal
handling, return to execute instruction i2, cause single-step exception,
restore breakpoints and run smoothly thereafter.
In case of (b), the new instruction iN is single-stepped, the breakpoint
values are restored and the hw-breakpoint exception callback is invoked
after iN is executed. The user of this breakpoint i.e. the caller of
register_user_hw_breakpoint() who had placed a breakpoint on addressed
accessed by instruction i2 will be confused to find that an unrelated
instruction (which may not be a load/store) has caused the breakpoint.
If so desired, we may adopt the 'trigger-before-execute' semantics for
user-space breakpoints wherein the hw-breakpoint callback (through
perf_bp_event()) is invoked in hw_breakpoint_handler() itself. This
would indicate to the user that the impending instruction causes a DABR
'hit' but it may or may not be executed due to the role of
signal-handler or due to self-modifying code (as mentioned below).
Kindly let me know what you think about it.
(c) is the normal execution path we desire. The instruction i2 will be
safely single-stepped and breakpoints are restored.
> The instruction could be changed underneath us if the program is
> multi-threaded and another thread writes another instruction to the
> instruction word where the load or store is. Or it could use mmap()
> to map some other page at the address of the load or store. Either
> way we could end up with a different instruction there.
>
If the instruction that originally caused the DABR exception is changed,
the new instruction in its place would still single-step to restore
breakpoint values. However the user of breakpoint interface will be
confused to find that the callback is invoked for an irrelevant
instruction.
It could be circumvented, to an extent, through the use of
trigger-before-execute semantics (as described before).
> > referring to the fact that the thread which generated breakpoints hits
> > could have moved out of the CPU due to a scheduler induced context
> > switch (which is an apparent cause for current->thread.last_hit_ubp to
> > turn stale) or is there a different source for such a change that I
> > don't realise?
> >
> > Given that 'last_hit_ubp' is safely ensconced inside 'thread_struct',
> > the ill-effects of a possible pre-emption during a page-fault will be
> > suitably handled i.e. the pending single-step exception will be
> > generated on the processor to which 'current' is migrated to, and the
> > breakpoint will be set on the new processor.
>
> If we do get a context switch, e.g. as a result of a page fault, and
> then switch back to the task, it looks to me like we will end up with
> MSR_SE and DABR both set. I don't suppose that will actually cause
> any real problem besides double-counting the hit.
>
Page fault exception will be handled before hw_breakpoint_handler(),
hence MSR_SE would not have been set if a context-switch happened in
pange-fault handling itself. I don't see a case where both MSR_SE and
DABR will be set together.
> > However, the possibility that current->thread.last_hit_ubp points to a
> > perf_event structure that is unregistered and freed does exist, and I
> > did not foresee the risk. An arch-specific function that hooks onto
> > release_bp_slot() would be required to perform the cleanup. I will
> > submit modify the patch to that effect. Thanks for pointing it out.
>
> Yes, I think we need that.
>
The same is implemented through arch_unregister_hw_breakpoint()
in version XVIII of the patch here: linuxppc-dev message-id:
20100512033315.GC6384@in.ibm.com.
> > In conjunction with what you've stated below, do you suggest that
> > emulate_step() be replaced with fix_alignment() which appears to be more
> > powerful at emulation (or carve out a helper function for fix_alignment()
> > that does only emulation and which can be invoked here)?
>
> Something like that eventually, but not for a first pass.
>
> Paul.
Thanks for the comments. Let me know if the analysis above is incorrect
or if I've failed to recognise any important issue that you pointed out.
I will send out a patch with changes to emulate_single_step() in the
next version of the patchset, if I don't hear any further comments.
Thanks,
K.Prasad
^ permalink raw reply
* [PATCH v2] trace, powerpc: Implement raw syscall tracepoints on PowerPC
From: Ian Munsie @ 2010-05-14 8:41 UTC (permalink / raw)
To: linux-kernel
Cc: Frederic Weisbecker, Jason Baron, Steven Rostedt, Ingo Molnar,
Paul Mackerras, Ian Munsie, linuxppc-dev
In-Reply-To: <1273736594-19320-5-git-send-email-imunsie@au1.ibm.com>
From: Ian Munsie <imunsie@au.ibm.com>
This patch implements the raw syscall tracepoints on PowerPC and exports
them for ftrace syscalls to use.
To minimise reworking existing code, I slightly re-ordered the thread
info flags such that the new TIF_SYSCALL_TRACEPOINT bit would still fit
within the 16 bits of the andi. instruction's UI field. The instructions
in question are in /arch/powerpc/kernel/entry_{32,64}.S to and the
_TIF_SYSCALL_T_OR_A with the thread flags to see if system call tracing
is enabled.
In the case of 64bit PowerPC, arch_syscall_addr and
arch_syscall_match_sym_name are overridden to allow ftrace syscalls to
work given the unusual system call table structure and symbol names that
start with a period.
Signed-off-by: Ian Munsie <imunsie@au1.ibm.com>
---
Changes since v1: No functional changes, just removed the redundant conditional
export of sys_call_table on PPC32 vs PPC64, removed a cast and added an extra
comment to explain the arch_syscall_match_sym_name based on the feedback from
Michael Ellerman.
arch/powerpc/Kconfig | 1 +
arch/powerpc/include/asm/syscall.h | 5 +++++
arch/powerpc/include/asm/thread_info.h | 7 +++++--
arch/powerpc/kernel/Makefile | 1 +
arch/powerpc/kernel/ftrace.c | 19 +++++++++++++++++++
arch/powerpc/kernel/ptrace.c | 10 ++++++++++
6 files changed, 41 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index c4c4549..41e2f3e 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -141,6 +141,7 @@ config PPC
select GENERIC_ATOMIC64 if PPC32
select HAVE_PERF_EVENTS
select HAVE_REGS_AND_STACK_ACCESS_API
+ select HAVE_SYSCALL_TRACEPOINTS
config EARLY_PRINTK
bool
diff --git a/arch/powerpc/include/asm/syscall.h b/arch/powerpc/include/asm/syscall.h
index 23913e9..b54b2ad 100644
--- a/arch/powerpc/include/asm/syscall.h
+++ b/arch/powerpc/include/asm/syscall.h
@@ -15,6 +15,11 @@
#include <linux/sched.h>
+/* ftrace syscalls requires exporting the sys_call_table */
+#ifdef CONFIG_FTRACE_SYSCALLS
+extern const unsigned long *sys_call_table;
+#endif /* CONFIG_FTRACE_SYSCALLS */
+
static inline long syscall_get_nr(struct task_struct *task,
struct pt_regs *regs)
{
diff --git a/arch/powerpc/include/asm/thread_info.h b/arch/powerpc/include/asm/thread_info.h
index aa9d383..e7a8af2 100644
--- a/arch/powerpc/include/asm/thread_info.h
+++ b/arch/powerpc/include/asm/thread_info.h
@@ -110,7 +110,8 @@ static inline struct thread_info *current_thread_info(void)
#define TIF_NOERROR 12 /* Force successful syscall return */
#define TIF_NOTIFY_RESUME 13 /* callback before returning to user */
#define TIF_FREEZE 14 /* Freezing for suspend */
-#define TIF_RUNLATCH 15 /* Is the runlatch enabled? */
+#define TIF_SYSCALL_TRACEPOINT 15 /* syscall tracepoint instrumentation */
+#define TIF_RUNLATCH 16 /* Is the runlatch enabled? */
/* as above, but as bit values */
#define _TIF_SYSCALL_TRACE (1<<TIF_SYSCALL_TRACE)
@@ -127,8 +128,10 @@ static inline struct thread_info *current_thread_info(void)
#define _TIF_NOERROR (1<<TIF_NOERROR)
#define _TIF_NOTIFY_RESUME (1<<TIF_NOTIFY_RESUME)
#define _TIF_FREEZE (1<<TIF_FREEZE)
+#define _TIF_SYSCALL_TRACEPOINT (1<<TIF_SYSCALL_TRACEPOINT)
#define _TIF_RUNLATCH (1<<TIF_RUNLATCH)
-#define _TIF_SYSCALL_T_OR_A (_TIF_SYSCALL_TRACE|_TIF_SYSCALL_AUDIT|_TIF_SECCOMP)
+#define _TIF_SYSCALL_T_OR_A (_TIF_SYSCALL_TRACE | _TIF_SYSCALL_AUDIT | \
+ _TIF_SECCOMP | _TIF_SYSCALL_TRACEPOINT)
#define _TIF_USER_WORK_MASK (_TIF_SIGPENDING | _TIF_NEED_RESCHED | \
_TIF_NOTIFY_RESUME)
diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
index 8773263..9c404bb 100644
--- a/arch/powerpc/kernel/Makefile
+++ b/arch/powerpc/kernel/Makefile
@@ -98,6 +98,7 @@ obj64-$(CONFIG_AUDIT) += compat_audit.o
obj-$(CONFIG_DYNAMIC_FTRACE) += ftrace.o
obj-$(CONFIG_FUNCTION_GRAPH_TRACER) += ftrace.o
+obj-$(CONFIG_FTRACE_SYSCALLS) += ftrace.o
obj-$(CONFIG_PERF_EVENTS) += perf_callchain.o
obj-$(CONFIG_PPC_PERF_CTRS) += perf_event.o
diff --git a/arch/powerpc/kernel/ftrace.c b/arch/powerpc/kernel/ftrace.c
index ce1f3e4..f5fadbb 100644
--- a/arch/powerpc/kernel/ftrace.c
+++ b/arch/powerpc/kernel/ftrace.c
@@ -22,6 +22,7 @@
#include <asm/cacheflush.h>
#include <asm/code-patching.h>
#include <asm/ftrace.h>
+#include <asm/syscall.h>
#ifdef CONFIG_DYNAMIC_FTRACE
@@ -600,3 +601,21 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr)
}
}
#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
+
+#if defined(CONFIG_FTRACE_SYSCALLS) && defined(CONFIG_PPC64)
+unsigned long __init arch_syscall_addr(int nr)
+{
+ return sys_call_table[nr*2];
+}
+
+inline bool arch_syscall_match_sym_name(const char *sym, const char *name)
+{
+ /*
+ * Compare the symbol name with the system call name. Skip the .sys or
+ * .SyS prefix from the symbol name and the sys prefix from the system
+ * call name and just match the rest. 32bit can use the generic
+ * function since their symbol names don't start with a period.
+ */
+ return (!strcmp(sym + 4, name + 3));
+}
+#endif /* CONFIG_FTRACE_SYSCALLS && CONFIG_PPC64 */
diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index 7a0c019..eb7eeb8 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -29,6 +29,7 @@
#include <linux/signal.h>
#include <linux/seccomp.h>
#include <linux/audit.h>
+#include <trace/syscall.h>
#ifdef CONFIG_PPC32
#include <linux/module.h>
#endif
@@ -38,6 +39,9 @@
#include <asm/pgtable.h>
#include <asm/system.h>
+#define CREATE_TRACE_POINTS
+#include <trace/events/syscalls.h>
+
/*
* The parameter save area on the stack is used to store arguments being passed
* to callee function and is located at fixed offset from stack pointer.
@@ -1615,6 +1619,9 @@ long do_syscall_trace_enter(struct pt_regs *regs)
*/
ret = -1L;
+ if (unlikely(test_thread_flag(TIF_SYSCALL_TRACEPOINT)))
+ trace_sys_enter(regs, regs->gpr[0]);
+
if (unlikely(current->audit_context)) {
#ifdef CONFIG_PPC64
if (!test_thread_flag(TIF_32BIT))
@@ -1643,6 +1650,9 @@ void do_syscall_trace_leave(struct pt_regs *regs)
audit_syscall_exit((regs->ccr&0x10000000)?AUDITSC_FAILURE:AUDITSC_SUCCESS,
regs->result);
+ if (unlikely(test_thread_flag(TIF_SYSCALL_TRACEPOINT)))
+ trace_sys_exit(regs, regs->result);
+
step = test_thread_flag(TIF_SINGLESTEP);
if (step || test_thread_flag(TIF_SYSCALL_TRACE))
tracehook_report_syscall_exit(regs, step);
--
1.7.1
^ permalink raw reply related
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