* [PATCH 2/2] [POWERPC] uprobes: powerpc port
From: Ananth N Mavinakayanahalli @ 2012-06-06 9:21 UTC (permalink / raw)
To: linuxppc-dev, lkml
Cc: Srikar Dronamraju, peterz, oleg, Paul Mackerras, Anton Blanchard,
Ingo Molnar
In-Reply-To: <20120606091950.GB6745@in.ibm.com>
From: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
This is the port of uprobes to powerpc. Usage is similar to x86.
One TODO in this port compared to x86 is the uprobe abort_xol() logic.
x86 depends on the thread_struct.trap_nr (absent in powerpc) to determine
if a signal was caused when the uprobed instruction was single-stepped/
emulated, in which case, we reset the instruction pointer to the probed
address and retry the probe again.
[root@xxxx ~]# ./bin/perf probe -x /lib64/libc.so.6 malloc
Added new event:
probe_libc:malloc (on 0xb4860)
You can now use it in all perf tools, such as:
perf record -e probe_libc:malloc -aR sleep 1
[root@xxxx ~]# ./bin/perf record -e probe_libc:malloc -aR sleep 20
[ perf record: Woken up 22 times to write data ]
[ perf record: Captured and wrote 5.843 MB perf.data (~255302 samples) ]
[root@xxxx ~]# ./bin/perf report --stdio
# ========
# captured on: Mon Jun 4 05:26:31 2012
# hostname : xxxx.ibm.com
# os release : 3.4.0-uprobe
# perf version : 3.4.0
# arch : ppc64
# nrcpus online : 4
# nrcpus avail : 4
# cpudesc : POWER6 (raw), altivec supported
# cpuid : 62,769
# total memory : 7310528 kB
# cmdline : /root/bin/perf record -e probe_libc:malloc -aR sleep 20
# event : name = probe_libc:malloc, type = 2, config = 0x124, config1 = 0x0, con
# HEADER_CPU_TOPOLOGY info available, use -I to display
# HEADER_NUMA_TOPOLOGY info available, use -I to display
# ========
#
# Samples: 83K of event 'probe_libc:malloc'
# Event count (approx.): 83484
#
# Overhead Command Shared Object Symbol
# ........ ............ ............. ..........
#
69.05% tar libc-2.12.so [.] malloc
28.57% rm libc-2.12.so [.] malloc
1.32% avahi-daemon libc-2.12.so [.] malloc
0.58% bash libc-2.12.so [.] malloc
0.28% sshd libc-2.12.so [.] malloc
0.08% irqbalance libc-2.12.so [.] malloc
0.05% bzip2 libc-2.12.so [.] malloc
0.04% sleep libc-2.12.so [.] malloc
0.03% multipathd libc-2.12.so [.] malloc
0.01% sendmail libc-2.12.so [.] malloc
0.01% automount libc-2.12.so [.] malloc
Signed-off-by: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
Index: linux-3.5-rc1/arch/powerpc/include/asm/thread_info.h
===================================================================
--- linux-3.5-rc1.orig/arch/powerpc/include/asm/thread_info.h 2012-06-03 06:59:26.000000000 +0530
+++ linux-3.5-rc1/arch/powerpc/include/asm/thread_info.h 2012-06-03 21:05:48.226233001 +0530
@@ -96,6 +96,7 @@
#define TIF_RESTOREALL 11 /* Restore all regs (implies NOERROR) */
#define TIF_NOERROR 12 /* Force successful syscall return */
#define TIF_NOTIFY_RESUME 13 /* callback before returning to user */
+#define TIF_UPROBE 14 /* breakpointed or single-stepping */
#define TIF_SYSCALL_TRACEPOINT 15 /* syscall tracepoint instrumentation */
/* as above, but as bit values */
@@ -112,12 +113,13 @@
#define _TIF_RESTOREALL (1<<TIF_RESTOREALL)
#define _TIF_NOERROR (1<<TIF_NOERROR)
#define _TIF_NOTIFY_RESUME (1<<TIF_NOTIFY_RESUME)
+#define _TIF_UPROBE (1<<TIF_UPROBE)
#define _TIF_SYSCALL_TRACEPOINT (1<<TIF_SYSCALL_TRACEPOINT)
#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)
+ _TIF_NOTIFY_RESUME | _TIF_UPROBE)
#define _TIF_PERSYSCALL_MASK (_TIF_RESTOREALL|_TIF_NOERROR)
/* Bits in local_flags */
Index: linux-3.5-rc1/arch/powerpc/include/asm/uprobes.h
===================================================================
--- linux-3.5-rc1.orig/arch/powerpc/include/asm/uprobes.h 2012-05-31 02:23:15.746233001 +0530
+++ linux-3.5-rc1/arch/powerpc/include/asm/uprobes.h 2012-06-03 21:05:48.226233001 +0530
@@ -0,0 +1,49 @@
+#ifndef _ASM_UPROBES_H
+#define _ASM_UPROBES_H
+/*
+ * User-space Probes (UProbes) for powerpc
+ *
+ * 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.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
+ *
+ * Copyright (C) IBM Corporation, 2007-2012
+ *
+ * Adapted from the x86 port by Ananth N Mavinakayanahalli <ananth@in.ibm.com>
+ */
+
+#include <linux/notifier.h>
+
+typedef unsigned int uprobe_opcode_t;
+
+#define MAX_UINSN_BYTES 4
+#define UPROBE_XOL_SLOT_BYTES (MAX_UINSN_BYTES)
+
+#define UPROBE_SWBP_INSN 0x7fe00008
+#define UPROBE_SWBP_INSN_SIZE 4 /* swbp insn size in bytes */
+
+struct arch_uprobe {
+ u8 insn[MAX_UINSN_BYTES];
+};
+
+struct arch_uprobe_task {
+};
+
+extern int arch_uprobe_analyze_insn(struct arch_uprobe *aup, struct mm_struct *mm, loff_t vaddr);
+extern unsigned long uprobe_get_swbp_addr(struct pt_regs *regs);
+extern int arch_uprobe_pre_xol(struct arch_uprobe *aup, struct pt_regs *regs);
+extern int arch_uprobe_post_xol(struct arch_uprobe *aup, struct pt_regs *regs);
+extern bool arch_uprobe_xol_was_trapped(struct task_struct *tsk);
+extern int arch_uprobe_exception_notify(struct notifier_block *self, unsigned long val, void *data);
+extern void arch_uprobe_abort_xol(struct arch_uprobe *aup, struct pt_regs *regs);
+#endif /* _ASM_UPROBES_H */
Index: linux-3.5-rc1/arch/powerpc/kernel/Makefile
===================================================================
--- linux-3.5-rc1.orig/arch/powerpc/kernel/Makefile 2012-06-03 06:59:26.000000000 +0530
+++ linux-3.5-rc1/arch/powerpc/kernel/Makefile 2012-06-03 21:05:48.226233001 +0530
@@ -96,6 +96,7 @@
obj-$(CONFIG_BOOTX_TEXT) += btext.o
obj-$(CONFIG_SMP) += smp.o
obj-$(CONFIG_KPROBES) += kprobes.o
+obj-$(CONFIG_UPROBES) += uprobes.o
obj-$(CONFIG_PPC_UDBG_16550) += legacy_serial.o udbg_16550.o
obj-$(CONFIG_STACKTRACE) += stacktrace.o
obj-$(CONFIG_SWIOTLB) += dma-swiotlb.o
Index: linux-3.5-rc1/arch/powerpc/kernel/signal.c
===================================================================
--- linux-3.5-rc1.orig/arch/powerpc/kernel/signal.c 2012-06-03 06:59:26.000000000 +0530
+++ linux-3.5-rc1/arch/powerpc/kernel/signal.c 2012-06-03 21:05:48.236233000 +0530
@@ -11,6 +11,7 @@
#include <linux/tracehook.h>
#include <linux/signal.h>
+#include <linux/uprobes.h>
#include <linux/key.h>
#include <asm/hw_breakpoint.h>
#include <asm/uaccess.h>
@@ -157,6 +158,11 @@
void do_notify_resume(struct pt_regs *regs, unsigned long thread_info_flags)
{
+ if (thread_info_flags & _TIF_UPROBE) {
+ clear_thread_flag(TIF_UPROBE);
+ uprobe_notify_resume(regs);
+ }
+
if (thread_info_flags & _TIF_SIGPENDING)
do_signal(regs);
Index: linux-3.5-rc1/arch/powerpc/kernel/uprobes.c
===================================================================
--- linux-3.5-rc1.orig/arch/powerpc/kernel/uprobes.c 2012-05-31 02:23:15.746233001 +0530
+++ linux-3.5-rc1/arch/powerpc/kernel/uprobes.c 2012-06-03 21:05:48.236233000 +0530
@@ -0,0 +1,163 @@
+/*
+ * User-space Probes (UProbes) for powerpc
+ *
+ * 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.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
+ *
+ * Copyright (C) IBM Corporation, 2007-2012
+ *
+ * Adapted from the x86 port by Ananth N Mavinakayanahalli <ananth@in.ibm.com>
+ */
+#include <linux/kernel.h>
+#include <linux/sched.h>
+#include <linux/ptrace.h>
+#include <linux/uprobes.h>
+#include <linux/uaccess.h>
+
+#include <linux/kdebug.h>
+#include <asm/sstep.h>
+
+/**
+ * arch_uprobe_analyze_insn
+ * @mm: the probed address space.
+ * @arch_uprobe: the probepoint information.
+ * Return 0 on success or a -ve number on error.
+ */
+int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm, loff_t vaddr)
+{
+ if (vaddr & 0x03)
+ return -EINVAL;
+ return 0;
+}
+
+/*
+ * arch_uprobe_pre_xol - prepare to execute out of line.
+ * @auprobe: the probepoint information.
+ * @regs: reflects the saved user state of current task.
+ */
+int arch_uprobe_pre_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
+{
+ /* FIXME: We don't support abort_xol on powerpc for now */
+ regs->nip = current->utask->xol_vaddr;
+ return 0;
+}
+
+/**
+ * uprobe_get_swbp_addr - compute address of swbp given post-swbp regs
+ * @regs: Reflects the saved state of the task after it has hit a breakpoint
+ * instruction.
+ * Return the address of the breakpoint instruction.
+ */
+unsigned long uprobe_get_swbp_addr(struct pt_regs *regs)
+{
+ return instruction_pointer(regs);
+}
+
+/*
+ * If xol insn itself traps and generates a signal (SIGILL/SIGSEGV/etc),
+ * then detect the case where a singlestepped instruction jumps back to its
+ * own address. It is assumed that anything like do_page_fault/do_trap/etc
+ * sets thread.trap_nr != -1.
+ *
+ * FIXME: powerpc however doesn't have thread.trap_nr yet.
+ *
+ * arch_uprobe_pre_xol/arch_uprobe_post_xol save/restore thread.trap_nr,
+ * arch_uprobe_xol_was_trapped() simply checks that ->trap_nr is not equal to
+ * UPROBE_TRAP_NR == -1 set by arch_uprobe_pre_xol().
+ */
+bool arch_uprobe_xol_was_trapped(struct task_struct *t)
+{
+ /* FIXME: We don't support abort_xol on powerpc for now */
+ return false;
+}
+
+/*
+ * Called after single-stepping. To avoid the SMP problems that can
+ * occur when we temporarily put back the original opcode to
+ * single-step, we single-stepped a copy of the instruction.
+ *
+ * This function prepares to resume execution after the single-step.
+ */
+int arch_uprobe_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
+{
+ /* FIXME: We don't support abort_xol on powerpc for now */
+
+ /*
+ * On powerpc, except for loads and stores, most instructions
+ * including ones that alter code flow (branches, calls, returns)
+ * are emulated in the kernel. We get here only if the emulation
+ * support doesn't exist and have to fix-up the next instruction
+ * to be executed.
+ */
+ regs->nip = current->utask->vaddr + MAX_UINSN_BYTES;
+ return 0;
+}
+
+/* callback routine for handling exceptions. */
+int arch_uprobe_exception_notify(struct notifier_block *self, unsigned long val, void *data)
+{
+ struct die_args *args = data;
+ struct pt_regs *regs = args->regs;
+ int ret = NOTIFY_DONE;
+
+ /* We are only interested in userspace traps */
+ if (regs && !user_mode(regs))
+ return NOTIFY_DONE;
+
+ switch (val) {
+ case DIE_BPT:
+ if (uprobe_pre_sstep_notifier(regs))
+ ret = NOTIFY_STOP;
+ break;
+ case DIE_SSTEP:
+ if (uprobe_post_sstep_notifier(regs))
+ ret = NOTIFY_STOP;
+ default:
+ break;
+ }
+ return ret;
+}
+
+/*
+ * This function gets called when XOL instruction either gets trapped or
+ * the thread has a fatal signal, so reset the instruction pointer to its
+ * probed address.
+ */
+void arch_uprobe_abort_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
+{
+ /* FIXME: We don't support abort_xol on powerpc for now */
+ return;
+}
+
+/*
+ * See if the instruction can be emulated.
+ * Returns true if instruction was emulated, false otherwise.
+ */
+bool arch_uprobe_skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs)
+{
+ int ret;
+ unsigned int insn;
+
+ memcpy(&insn, auprobe->insn, MAX_UINSN_BYTES);
+
+ /*
+ * emulate_step() returns 1 if the insn was successfully emulated.
+ * For all other cases, we need to single-step in hardware.
+ */
+ ret = emulate_step(regs, insn);
+ if (ret > 0)
+ return true;
+
+ return false;
+}
Index: linux-3.5-rc1/arch/powerpc/Kconfig
===================================================================
--- linux-3.5-rc1.orig/arch/powerpc/Kconfig 2012-06-03 06:59:26.000000000 +0530
+++ linux-3.5-rc1/arch/powerpc/Kconfig 2012-06-03 21:05:48.246233000 +0530
@@ -237,6 +237,9 @@
config ARCH_SUPPORTS_DEBUG_PAGEALLOC
def_bool y
+config ARCH_SUPPORTS_UPROBES
+ def_bool y
+
config PPC_ADV_DEBUG_REGS
bool
depends on 40x || BOOKE
^ permalink raw reply
* Re: [PATCH 1/2] uprobes: Pass probed vaddr to arch_uprobe_analyze_insn()
From: Peter Zijlstra @ 2012-06-06 9:23 UTC (permalink / raw)
To: ananth
Cc: Srikar Dronamraju, lkml, oleg, Paul Mackerras, Anton Blanchard,
Ingo Molnar, linuxppc-dev
In-Reply-To: <20120606091950.GB6745@in.ibm.com>
On Wed, 2012-06-06 at 14:49 +0530, Ananth N Mavinakayanahalli wrote:
> +int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_stru=
ct *mm, loff_t vaddr)
Don't we traditionally use unsigned long to pass vaddrs?
^ permalink raw reply
* Re: [PATCH 2/2] [POWERPC] uprobes: powerpc port
From: Peter Zijlstra @ 2012-06-06 9:27 UTC (permalink / raw)
To: ananth
Cc: Srikar Dronamraju, lkml, oleg, Paul Mackerras, Anton Blanchard,
Ingo Molnar, linuxppc-dev
In-Reply-To: <20120606092150.GC6745@in.ibm.com>
On Wed, 2012-06-06 at 14:51 +0530, Ananth N Mavinakayanahalli wrote:
> One TODO in this port compared to x86 is the uprobe abort_xol() logic.
> x86 depends on the thread_struct.trap_nr (absent in powerpc) to determine
> if a signal was caused when the uprobed instruction was single-stepped/
> emulated, in which case, we reset the instruction pointer to the probed
> address and retry the probe again.=20
Another curious difference is that x86 uses an instruction decoder and
contains massive tables to validate we can probe a particular
instruction.
Can we probe all possible PPC instructions?
^ permalink raw reply
* Re: [PATCH v5 1/5] powerpc/85xx: implement hardware timebase sync
From: Zhao Chenhui @ 2012-06-06 9:31 UTC (permalink / raw)
To: Scott Wood; +Cc: Matthew McClintock, linuxppc-dev, linux-kernel
In-Reply-To: <4FCE2ECD.4050107@freescale.com>
On Tue, Jun 05, 2012 at 11:07:41AM -0500, Scott Wood wrote:
> On 06/05/2012 04:08 AM, Zhao Chenhui wrote:
> > On Fri, Jun 01, 2012 at 10:40:00AM -0500, Scott Wood wrote:
> >> I know you say this is for dual-core chips only, but it would be nice if
> >> you'd write this in a way that doesn't assume that (even if the
> >> corenet-specific timebase freezing comes later).
> >
> > At this point, I have not thought about how to implement the cornet-specific timebase freezing.
>
> I wasn't asking you to. I was asking you to not have logic that breaks
> with more than 2 CPUs.
These routines only called in the dual-core case.
>
> >> Do we need an isync after setting the timebase, to ensure it's happened
> >> before we enable the timebase? Likewise, do we need a readback after
> >> disabling the timebase to ensure it's disabled before we read the
> >> timebase in give_timebase?
> >
> > I checked the e500 core manual (Chapter 2.16 Synchronization Requirements for SPRs).
> > Only some SPR registers need an isync. The timebase registers do not.
>
> I don't trust that, and the consequences of having the sync be imperfect
> are too unpleasant to chance it.
>
> > I did a readback in mpc85xx_timebase_freeze().
>
> Sorry, missed that somehow.
>
> >>> +#ifdef CONFIG_KEXEC
> >>> + np = of_find_matching_node(NULL, guts_ids);
> >>> + if (np) {
> >>> + guts = of_iomap(np, 0);
> >>> + smp_85xx_ops.give_timebase = mpc85xx_give_timebase;
> >>> + smp_85xx_ops.take_timebase = mpc85xx_take_timebase;
> >>> + of_node_put(np);
> >>> + } else {
> >>> + smp_85xx_ops.give_timebase = smp_generic_give_timebase;
> >>> + smp_85xx_ops.take_timebase = smp_generic_take_timebase;
> >>> + }
> >>
> >> Do not use smp_generic_give/take_timebase, ever. If you don't have the
> >> guts node, then just assume the timebase is already synced.
> >>
> >> -Scott
> >
> > smp_generic_give/take_timebase is the default in KEXEC before.
>
> That was a mistake.
>
> > If do not set them, it may make KEXEC fail on other platforms.
>
> What platforms?
>
> -Scott
Such as P4080, P3041, etc.
-Chenhui
^ permalink raw reply
* Re: [PATCH 2/2] [POWERPC] uprobes: powerpc port
From: Ananth N Mavinakayanahalli @ 2012-06-06 9:35 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Srikar Dronamraju, lkml, oleg, Paul Mackerras, Anton Blanchard,
Ingo Molnar, linuxppc-dev
In-Reply-To: <1338974822.2749.89.camel@twins>
On Wed, Jun 06, 2012 at 11:27:02AM +0200, Peter Zijlstra wrote:
> On Wed, 2012-06-06 at 14:51 +0530, Ananth N Mavinakayanahalli wrote:
> > One TODO in this port compared to x86 is the uprobe abort_xol() logic.
> > x86 depends on the thread_struct.trap_nr (absent in powerpc) to determine
> > if a signal was caused when the uprobed instruction was single-stepped/
> > emulated, in which case, we reset the instruction pointer to the probed
> > address and retry the probe again.
>
> Another curious difference is that x86 uses an instruction decoder and
> contains massive tables to validate we can probe a particular
> instruction.
>
> Can we probe all possible PPC instructions?
For the kernel, the only ones that are off limits are rfi (return from
interrupt), mtmsr (move to msr). All other instructions can be probed.
Both those instructions are supervisor level, so we won't see them in
userspace at all; so we should be able to probe all user level
instructions.
I am not aware of specific caveats for vector/altivec instructions;
maybe Paul or Ben are more suitable to comment on that.
Ananth
^ permalink raw reply
* Re: [PATCH 1/2] uprobes: Pass probed vaddr to arch_uprobe_analyze_insn()
From: Ananth N Mavinakayanahalli @ 2012-06-06 9:37 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Srikar Dronamraju, lkml, oleg, Paul Mackerras, Anton Blanchard,
Ingo Molnar, linuxppc-dev
In-Reply-To: <1338974632.2749.87.camel@twins>
On Wed, Jun 06, 2012 at 11:23:52AM +0200, Peter Zijlstra wrote:
> On Wed, 2012-06-06 at 14:49 +0530, Ananth N Mavinakayanahalli wrote:
> > +int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm, loff_t vaddr)
>
> Don't we traditionally use unsigned long to pass vaddrs?
Right. But the vaddr we pass here is vma_info->vaddr which is loff_t.
I guess I should've made that clear in the patch description.
Ananth
^ permalink raw reply
* Re: [PATCH 1/2] uprobes: Pass probed vaddr to arch_uprobe_analyze_insn()
From: Ingo Molnar @ 2012-06-06 9:40 UTC (permalink / raw)
To: Ananth N Mavinakayanahalli
Cc: Srikar Dronamraju, Peter Zijlstra, lkml, oleg, Paul Mackerras,
Anton Blanchard, Ingo Molnar, linuxppc-dev
In-Reply-To: <20120606093744.GB29580@in.ibm.com>
* Ananth N Mavinakayanahalli <ananth@in.ibm.com> wrote:
> On Wed, Jun 06, 2012 at 11:23:52AM +0200, Peter Zijlstra wrote:
> > On Wed, 2012-06-06 at 14:49 +0530, Ananth N Mavinakayanahalli wrote:
> > > +int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm, loff_t vaddr)
> >
> > Don't we traditionally use unsigned long to pass vaddrs?
>
> Right. But the vaddr we pass here is vma_info->vaddr which is loff_t.
> I guess I should've made that clear in the patch description.
Why not fix struct vma_info's vaddr type?
Thanks,
Ingo
^ permalink raw reply
* Re: [PATCH v5 2/5] powerpc/85xx: add HOTPLUG_CPU support
From: Zhao Chenhui @ 2012-06-06 9:59 UTC (permalink / raw)
To: Scott Wood; +Cc: linuxppc-dev, linux-kernel
In-Reply-To: <4FCE30B8.2070902@freescale.com>
On Tue, Jun 05, 2012 at 11:15:52AM -0500, Scott Wood wrote:
> On 06/05/2012 06:18 AM, Zhao Chenhui wrote:
> > On Mon, Jun 04, 2012 at 11:32:47AM -0500, Scott Wood wrote:
> >> On 06/04/2012 06:04 AM, Zhao Chenhui wrote:
> >>> On Fri, Jun 01, 2012 at 04:27:27PM -0500, Scott Wood wrote:
> >>>> On 05/11/2012 06:53 AM, Zhao Chenhui wrote:
> >>>>> -#ifdef CONFIG_KEXEC
> >>>>> +#if defined(CONFIG_KEXEC) || defined(CONFIG_HOTPLUG_CPU)
> >>>>
> >>>> Let's not grow lists like this. Is there any harm in building it
> >>>> unconditionally?
> >>>>
> >>>> -Scott
> >>>
> >>> We need this ifdef. We only set give_timebase/take_timebase
> >>> when CONFIG_KEXEC or CONFIG_HOTPLUG_CPU is defined.
> >>
> >> If we really need this to be a compile-time decision, make a new symbol
> >> for it, but I really think this should be decided at runtime. Just
> >> because we have kexec or hotplug support enabled doesn't mean that's
> >> actually what we're doing at the moment.
> >>
> >> -Scott
> >
> > If user does not enable kexec or hotplug, these codes are redundant.
> > So use CONFIG_KEXEC and CONFIG_HOTPLUG_CPU to gard them.
>
> My point is that these lists tend to grow and be a maintenance pain.
> For small things it's often better to not worry about saving a few
> bytes. For larger things that need to be conditional, define a new
> symbol rather than growing ORed lists like this.
>
> -Scott
I agree with you in principle. But there are only two config options
in this patch, and it is unlikely to grow.
-Chenhui
^ permalink raw reply
* Re: [PATCH v5 5/5] powerpc/85xx: add support to JOG feature using cpufreq interface
From: Zhao Chenhui @ 2012-06-06 10:19 UTC (permalink / raw)
To: Scott Wood; +Cc: linuxppc-dev, linux-kernel
In-Reply-To: <4FCE2CB1.5090308@freescale.com>
On Tue, Jun 05, 2012 at 10:58:41AM -0500, Scott Wood wrote:
> On 06/05/2012 05:59 AM, Zhao Chenhui wrote:
> > On Fri, Jun 01, 2012 at 06:30:55PM -0500, Scott Wood wrote:
> >> On 05/11/2012 06:53 AM, Zhao Chenhui wrote:
> >>> The jog mode frequency transition process on the MPC8536 is similar to
> >>> the deep sleep process. The driver need save the CPU state and restore
> >>> it after CPU warm reset.
> >>>
> >>> Note:
> >>> * The I/O peripherals such as PCIe and eTSEC may lose packets during
> >>> the jog mode frequency transition.
> >>
> >> That might be acceptable for eTSEC, but it is not acceptable to lose
> >> anything on PCIe. Especially not if you're going to make this "default y".
> >
> > It is a hardware limitation.
>
> Then make sure jog isn't used if PCIe is used.
>
> Maybe you could do something with the suspend infrastructure, but this
> is sufficiently heavyweight that transitions should be manually
> requested, not triggered by the automatic cpufreq governor.
>
> Does this apply to p1022, or just mpc8536?
Both of them.
>
> > Peripherals in the platform will not be operating
> > during the jog mode frequency transition process.
>
> What ensures this?
>
> -Scott
Hardware ensures it without software intervention.
-Chenhui
^ permalink raw reply
* Re: [PATCH 1/2] uprobes: Pass probed vaddr to arch_uprobe_analyze_insn()
From: Ananth N Mavinakayanahalli @ 2012-06-06 10:22 UTC (permalink / raw)
To: Ingo Molnar
Cc: Srikar Dronamraju, Peter Zijlstra, lkml, oleg, Paul Mackerras,
Anton Blanchard, Ingo Molnar, linuxppc-dev
In-Reply-To: <20120606094014.GD9495@gmail.com>
On Wed, Jun 06, 2012 at 11:40:15AM +0200, Ingo Molnar wrote:
>
> * Ananth N Mavinakayanahalli <ananth@in.ibm.com> wrote:
>
> > On Wed, Jun 06, 2012 at 11:23:52AM +0200, Peter Zijlstra wrote:
> > > On Wed, 2012-06-06 at 14:49 +0530, Ananth N Mavinakayanahalli wrote:
> > > > +int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm, loff_t vaddr)
> > >
> > > Don't we traditionally use unsigned long to pass vaddrs?
> >
> > Right. But the vaddr we pass here is vma_info->vaddr which is loff_t.
> > I guess I should've made that clear in the patch description.
>
> Why not fix struct vma_info's vaddr type?
Agreed. Will fix and send v2.
Ananth
^ permalink raw reply
* Re: [RFC PATCH] sched/numa: do load balance between remote nodes
From: Sergei Shtylyov @ 2012-06-06 10:53 UTC (permalink / raw)
To: Alex Shi
Cc: linux-mips, linux-ia64, linux-sh, dhowells, paulus, hpa,
sparclinux, mingo, sivanich, x86, greg.pearson, chris.mason,
arjan.van.de.ven, mattst88, pjt, fenghua.yu, seto.hidetoshi,
a.p.zijlstra, cmetcalf, ak, ink, anton, tglx, kamezawa.hiroyu,
rth, tony.luck, torvalds, linux-kernel, ralf, lethal, linux-alpha,
bob.picco, akpm, linuxppc-dev, davem
In-Reply-To: <1338965571-9812-1-git-send-email-alex.shi@intel.com>
Hello.
On 06-06-2012 10:52, Alex Shi wrote:
> commit cb83b629b
Please also specify that commit's summary in parens.
> remove the NODE sched domain and check if the node
> distance in SLIT table is farther than REMOTE_DISTANCE, if so, it will
> lose the load balance chance at exec/fork/wake_affine points.
> But actually, even the node distance is farther than REMOTE_DISTANCE,
> Modern CPUs also has QPI like connections, that make memory access is
"Is" not needed here.
> not too slow between nodes. So above losing on NUMA machine make a
> huge performance regression on benchmark: hackbench, tbench, netperf
> and oltp etc.
> This patch will recover the scheduler behavior to old mode on all my
> Intel platforms: NHM EP/EX, WSM EP, SNB EP/EP4S, and so remove the
> perfromance regressions. (all of them just has 2 kinds distance, 10 21)
> Signed-off-by: Alex Shi<alex.shi@intel.com>
WBR, Sergei
^ permalink raw reply
* Re: kernel panic during kernel module load (powerpc specific part)
From: Benjamin Herrenschmidt @ 2012-06-06 11:32 UTC (permalink / raw)
To: Steffen Rumler
Cc: Wrobel Heinz-R39252, Michael Ellerman,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <4FCF0897.2060405@nsn.com>
On Wed, 2012-06-06 at 09:36 +0200, Steffen Rumler wrote:
>
> how should we continue here ?
> There is the kernel panic, I've described.
>
> Technically, there is an conflict between the code generated by the
> compiler and the loader in module_32.c, at least by using -Os.
> Because the prologue/epilogue is part of the .text and init_module()
> is part of .init.text (in the case __init is applied, as usual),
> a directly reachable call is not always possible.
As we discussed earlier, if you could submit a patch to use r12 instead,
we should merge that.
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCH 1/2] uprobes: Pass probed vaddr to arch_uprobe_analyze_insn()
From: Srikar Dronamraju @ 2012-06-06 11:44 UTC (permalink / raw)
To: Ingo Molnar
Cc: Peter Zijlstra, lkml, oleg, Paul Mackerras, Anton Blanchard,
Ingo Molnar, linuxppc-dev
In-Reply-To: <20120606094014.GD9495@gmail.com>
* Ingo Molnar <mingo@kernel.org> [2012-06-06 11:40:15]:
>
> * Ananth N Mavinakayanahalli <ananth@in.ibm.com> wrote:
>
> > On Wed, Jun 06, 2012 at 11:23:52AM +0200, Peter Zijlstra wrote:
> > > On Wed, 2012-06-06 at 14:49 +0530, Ananth N Mavinakayanahalli wrote:
> > > > +int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm, loff_t vaddr)
> > >
> > > Don't we traditionally use unsigned long to pass vaddrs?
> >
> > Right. But the vaddr we pass here is vma_info->vaddr which is loff_t.
> > I guess I should've made that clear in the patch description.
>
> Why not fix struct vma_info's vaddr type?
>
Calculating and comparing vaddr results either uses variables of type loff_t.
To avoid typecasting and avoid overflow at each of these places, we used
loff_t.
Ananth, install_breakpoint() already has a variable of type addr of type
unsigned long. Why dont you use addr instead of vaddr.
--
Thanks and regards
Srikar
^ permalink raw reply
* Re: [PATCH] powerpc: Fix assmption of end_of_DRAM() returns end address
From: Andrea Arcangeli @ 2012-06-06 13:14 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org,
Bhushan Bharat-R65777, linuxppc-dev@lists.ozlabs.org,
David Miller
In-Reply-To: <1338960617.7150.163.camel@pasglop>
Hi,
On Wed, Jun 06, 2012 at 03:30:17PM +1000, Benjamin Herrenschmidt wrote:
> On Wed, 2012-06-06 at 00:46 +0000, Bhushan Bharat-R65777 wrote:
>
> > > >> memblock_end_of_DRAM() returns end_address + 1, not end address.
> > > >> While some code assumes that it returns end address.
> > > >
> > > > Shouldn't we instead fix it the other way around ? IE, make
> > > > memblock_end_of_DRAM() does what the name implies, which is to
> > return
> > > > the last byte of DRAM, and fix the -other- callers not to make bad
> > > > assumptions ?
> > >
> > > That was my impression too when I saw this patch.
> >
> > Initially I also intended to do so. I initiated a email on linux-mm@
> > subject "memblock_end_of_DRAM() return end address + 1" and the only
> > response I received from Andrea was:
> >
> > "
> > It's normal that "end" means "first byte offset out of the range". End
> > = not ok.
> > end = start+size.
> > This is true for vm_end too. So it's better to keep it that way.
> > My suggestion is to just fix point 1 below and audit the rest :)
> > "
>
> Oh well, I don't care enough to fight this battle in my current state so
I wish you to get well soon Ben!
> unless Dave has more stamina than I have today, I'm ok with the patch.
Well it doesn't really matter in the end what is decided as long as
something is decided :). I was asked through a forward so I only
expressed my preference...
^ permalink raw reply
* [PATCH] kernel panic during kernel module load (powerpc specific part)
From: Steffen Rumler @ 2012-06-06 14:37 UTC (permalink / raw)
To: ext Benjamin Herrenschmidt, paulus
Cc: Wrobel Heinz-R39252, Michael Ellerman,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <1338982323.7150.165.camel@pasglop>
Hi,
The patch below is intended to fix the following problem.
According to the PowerPC EABI specification, the GPR r11 is assigned
the dedicated function to point to the previous stack frame.
In the powerpc-specific kernel module loader, do_plt_call()
(in arch/powerpc/kernel/module_32.c), the GPR r11 is also used
to generate trampoline code.
This combination crashes the kernel, in the following case:
+ The compiler has been generated the prologue and epilogue,
which is part of the .text section.
+ The compiler has been generated the code for the module init entry point,
part of the .init.text section (in the case it is marked with __init).
+ By returning from the module init entry point, the epilogue is called by doing
a branch instruction.
+ If the epilogue is too far away, a relative branch instruction cannot be applied.
Instead trampoline code is generated in do_plt_call(), in order to jump via register.
Unfortunately the code generated by do_plt_call() destroys the content of GPR r11.
+ Because GPR r11 does not more keep the right stack frame pointer,
the kernel crashes right after the epilogue.
The fix just uses GPR r12 instead of GPR r11 for generating the trampoline code.
According to the statements from Freescale, this is also save from EABI perspective.
I've tested the fix for kernel 2.6.33 on MPC8541.
Signed-off-by: Steffen Rumler <steffen.rumler.ext@nsn.com>
---
--- orig/arch/powerpc/kernel/module_32.c 2012-06-06 16:04:28.956446788 +0200
+++ new/arch/powerpc/kernel/module_32.c 2012-06-06 16:04:17.746290683 +0200
@@ -187,8 +187,8 @@
static inline int entry_matches(struct ppc_plt_entry *entry, Elf32_Addr val)
{
- if (entry->jump[0] == 0x3d600000 + ((val + 0x8000) >> 16)
- && entry->jump[1] == 0x396b0000 + (val & 0xffff))
+ if (entry->jump[0] == 0x3d800000 + ((val + 0x8000) >> 16)
+ && entry->jump[1] == 0x398c0000 + (val & 0xffff))
return 1;
return 0;
}
@@ -215,10 +215,9 @@
entry++;
}
- /* Stolen from Paul Mackerras as well... */
- entry->jump[0] = 0x3d600000+((val+0x8000)>>16); /* lis r11,sym@ha */
- entry->jump[1] = 0x396b0000 + (val&0xffff); /* addi r11,r11,sym@l*/
- entry->jump[2] = 0x7d6903a6; /* mtctr r11 */
+ entry->jump[0] = 0x3d800000+((val+0x8000)>>16); /* lis r12,sym@ha */
+ entry->jump[1] = 0x398c0000 + (val&0xffff); /* addi r12,r12,sym@l*/
+ entry->jump[2] = 0x7d8903a6; /* mtctr r12 */
entry->jump[3] = 0x4e800420; /* bctr */
DEBUGP("Initialized plt for 0x%x at %p\n", val, entry);
^ permalink raw reply
* Re: [PATCH 1/2] uprobes: Pass probed vaddr to arch_uprobe_analyze_insn()
From: Oleg Nesterov @ 2012-06-06 15:08 UTC (permalink / raw)
To: Ananth N Mavinakayanahalli
Cc: Srikar Dronamraju, peterz, lkml, Paul Mackerras, Anton Blanchard,
Ingo Molnar, linuxppc-dev
In-Reply-To: <20120606091950.GB6745@in.ibm.com>
On 06/06, Ananth N Mavinakayanahalli wrote:
>
> From: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
>
> On RISC architectures like powerpc, instructions are fixed size.
> Instruction analysis on such platforms is just a matter of (insn % 4).
> Pass the vaddr at which the uprobe is to be inserted so that
> arch_uprobe_analyze_insn() can flag misaligned registration requests.
And the next patch checks "vaddr & 0x03".
But why do you need this new arg? arch_uprobe_analyze_insn() could
check "container_of(auprobe, struct uprobe, arch)->offset & 0x3" with
the same effect, no? vm_start/vm_pgoff are obviously page-aligned.
Oleg.
^ permalink raw reply
* Re: [PATCH] powerpc: Fix assmption of end_of_DRAM() returns end address
From: David Miller @ 2012-06-06 16:03 UTC (permalink / raw)
To: benh; +Cc: aarcange, linux-kernel, linux-mm, R65777, linuxppc-dev
In-Reply-To: <1338960617.7150.163.camel@pasglop>
From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Date: Wed, 06 Jun 2012 15:30:17 +1000
> On Wed, 2012-06-06 at 00:46 +0000, Bhushan Bharat-R65777 wrote:
>
>> > >> memblock_end_of_DRAM() returns end_address + 1, not end address.
>> > >> While some code assumes that it returns end address.
>> > >
>> > > Shouldn't we instead fix it the other way around ? IE, make
>> > > memblock_end_of_DRAM() does what the name implies, which is to
>> return
>> > > the last byte of DRAM, and fix the -other- callers not to make bad
>> > > assumptions ?
>> >
>> > That was my impression too when I saw this patch.
>>
>> Initially I also intended to do so. I initiated a email on linux-mm@
>> subject "memblock_end_of_DRAM() return end address + 1" and the only
>> response I received from Andrea was:
>>
>> "
>> It's normal that "end" means "first byte offset out of the range". End
>> = not ok.
>> end = start+size.
>> This is true for vm_end too. So it's better to keep it that way.
>> My suggestion is to just fix point 1 below and audit the rest :)
>> "
>
> Oh well, I don't care enough to fight this battle in my current state so
> unless Dave has more stamina than I have today, I'm ok with the patch.
I'm definitely without the samina to fight something like this right now :)
^ permalink raw reply
* Re: [PATCH 1/2] uprobes: Pass probed vaddr to arch_uprobe_analyze_insn()
From: Srikar Dronamraju @ 2012-06-06 16:30 UTC (permalink / raw)
To: Oleg Nesterov
Cc: peterz, lkml, Paul Mackerras, Anton Blanchard, Ingo Molnar,
linuxppc-dev
In-Reply-To: <20120606150848.GA24641@redhat.com>
* Oleg Nesterov <oleg@redhat.com> [2012-06-06 17:08:48]:
> On 06/06, Ananth N Mavinakayanahalli wrote:
> >
> > From: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
> >
> > On RISC architectures like powerpc, instructions are fixed size.
> > Instruction analysis on such platforms is just a matter of (insn % 4).
> > Pass the vaddr at which the uprobe is to be inserted so that
> > arch_uprobe_analyze_insn() can flag misaligned registration requests.
>
> And the next patch checks "vaddr & 0x03".
>
> But why do you need this new arg? arch_uprobe_analyze_insn() could
> check "container_of(auprobe, struct uprobe, arch)->offset & 0x3" with
> the same effect, no? vm_start/vm_pgoff are obviously page-aligned.
>
We cant use container_of because we moved the definition for struct
uprobe to kernel/events/uprobe.c. This was possible before when struct
uprobe definition was in include/uprobes.h
--
Thanks and Regards
Srikar
^ permalink raw reply
* Re: [PATCH] powerpc: Optimise the 64bit optimised __clear_user
From: Segher Boessenkool @ 2012-06-06 16:40 UTC (permalink / raw)
To: Anton Blanchard; +Cc: mikey, michael, paulus, olof, linuxppc-dev
In-Reply-To: <20120605120222.6722a3e3@kryten>
> +err1; dcbz r0,r3
There is no such instruction, you probably meant "dcbz 0,r3"?
Segher
^ permalink raw reply
* Re: [PATCH 2/2] [POWERPC] uprobes: powerpc port
From: Jim Keniston @ 2012-06-06 18:08 UTC (permalink / raw)
To: ananth
Cc: Srikar Dronamraju, Peter Zijlstra, oleg, lkml, Paul Mackerras,
Anton Blanchard, Ingo Molnar, linuxppc-dev
In-Reply-To: <20120606093541.GA29580@in.ibm.com>
On Wed, 2012-06-06 at 15:05 +0530, Ananth N Mavinakayanahalli wrote:
> On Wed, Jun 06, 2012 at 11:27:02AM +0200, Peter Zijlstra wrote:
> > On Wed, 2012-06-06 at 14:51 +0530, Ananth N Mavinakayanahalli wrote:
> > > One TODO in this port compared to x86 is the uprobe abort_xol() logic.
> > > x86 depends on the thread_struct.trap_nr (absent in powerpc) to determine
> > > if a signal was caused when the uprobed instruction was single-stepped/
> > > emulated, in which case, we reset the instruction pointer to the probed
> > > address and retry the probe again.
> >
> > Another curious difference is that x86 uses an instruction decoder and
> > contains massive tables to validate we can probe a particular
> > instruction.
Part of that difference is because the x86 instruction set is a lot more
complex. Another part is due to the lack, back when the x86 code was
created, of robust handling by uprobes of traps by probed instructions.
So we refused to probe instructions that we knew (or strongly suspected)
would generate traps in user mode -- e.g., privileged instructions,
illegal instructions. A couple of times we had to "legalize"
instructions or prefixes that we didn't originally expect to encounter.
> >
> > Can we probe all possible PPC instructions?
>
> For the kernel, the only ones that are off limits are rfi (return from
> interrupt), mtmsr (move to msr). All other instructions can be probed.
>
> Both those instructions are supervisor level, so we won't see them in
> userspace at all; so we should be able to probe all user level
> instructions.
Presumably rfi or mtmsr could show up in the instruction stream via an
erroneous or mischievous asm statement. It'd be good to verify that you
handle that gracefully.
>
> I am not aware of specific caveats for vector/altivec instructions;
> maybe Paul or Ben are more suitable to comment on that.
>
> Ananth
>
Jim
^ permalink raw reply
* Re: [PATCH v5 2/5] powerpc/85xx: add HOTPLUG_CPU support
From: Scott Wood @ 2012-06-06 18:19 UTC (permalink / raw)
To: Zhao Chenhui; +Cc: linuxppc-dev, linux-kernel
In-Reply-To: <20120606095910.GB23505@localhost.localdomain>
On 06/06/2012 04:59 AM, Zhao Chenhui wrote:
> On Tue, Jun 05, 2012 at 11:15:52AM -0500, Scott Wood wrote:
>> On 06/05/2012 06:18 AM, Zhao Chenhui wrote:
>>> If user does not enable kexec or hotplug, these codes are redundant.
>>> So use CONFIG_KEXEC and CONFIG_HOTPLUG_CPU to gard them.
>>
>> My point is that these lists tend to grow and be a maintenance pain.
>> For small things it's often better to not worry about saving a few
>> bytes. For larger things that need to be conditional, define a new
>> symbol rather than growing ORed lists like this.
>>
>> -Scott
>
> I agree with you in principle. But there are only two config options
> in this patch, and it is unlikely to grow.
That's what everybody says when these things start. :-)
-Scott
^ permalink raw reply
* Re: [PATCH v5 1/5] powerpc/85xx: implement hardware timebase sync
From: Scott Wood @ 2012-06-06 18:26 UTC (permalink / raw)
To: Zhao Chenhui; +Cc: Matthew McClintock, linuxppc-dev, linux-kernel
In-Reply-To: <20120606093142.GA23505@localhost.localdomain>
On 06/06/2012 04:31 AM, Zhao Chenhui wrote:
> On Tue, Jun 05, 2012 at 11:07:41AM -0500, Scott Wood wrote:
>> On 06/05/2012 04:08 AM, Zhao Chenhui wrote:
>>> On Fri, Jun 01, 2012 at 10:40:00AM -0500, Scott Wood wrote:
>>>> I know you say this is for dual-core chips only, but it would be nice if
>>>> you'd write this in a way that doesn't assume that (even if the
>>>> corenet-specific timebase freezing comes later).
>>>
>>> At this point, I have not thought about how to implement the cornet-specific timebase freezing.
>>
>> I wasn't asking you to. I was asking you to not have logic that breaks
>> with more than 2 CPUs.
>
> These routines only called in the dual-core case.
Come on, you know we have chips with more than two cores. Why design
such a limitation into it, just because you're not personally interested
in supporting anything but e500v2?
Is it so hard to make it work for an arbitrary number of cores?
>>> If do not set them, it may make KEXEC fail on other platforms.
>>
>> What platforms?
>
> Such as P4080, P3041, etc.
So we need to wait for corenet timebase sync before we stop causing
problems in virtualization, simulators, etc. if a kernel has kexec or
cpu hotplug enabled (whether used or not)?
Can you at least make sure we're actually in a kexec/hotplug scenario at
runtime?
Or just implement corenet timebase sync -- it's not that different.
-Scott
^ permalink raw reply
* Re: [PATCH v5 4/5] fsl_pmc: Add API to enable device as wakeup event source
From: Scott Wood @ 2012-06-06 18:29 UTC (permalink / raw)
To: Li Yang
Cc: Wood Scott-B07421, linuxppc-dev@lists.ozlabs.org, Li Yang-R58472,
linux-kernel@vger.kernel.org, Zhao Chenhui-B35336
In-Reply-To: <CADRPPNSzTNPkNExBnNu1iD2hqyRG=Cw+y+PnjTRWxjP_ib1Kww@mail.gmail.com>
On 06/05/2012 11:06 PM, Li Yang wrote:
> On Wed, Jun 6, 2012 at 2:05 AM, Scott Wood <scottwood@freescale.com> wrote:
>> You ignored "what about devices other than ethernet".
>
> No, I haven't. Other devices are so at least for now.
I don't understand that last sentence. Other devices are what?
-Scott
^ permalink raw reply
* Re: [PATCH] PPC: PCI: Fix pcibios_io_space_offset() so it works for 32-bit ptr/64-bit rsrcs
From: Scott Wood @ 2012-06-06 21:15 UTC (permalink / raw)
To: Ben Collins; +Cc: linuxppc-dev
In-Reply-To: <4DC27253-67FC-4A55-8C78-7782D9D0CF53@servergy.com>
On 06/05/2012 10:50 PM, Ben Collins wrote:
> The commit introducing pcibios_io_space_offset() was ignoring 32-bit to
> 64-bit sign extention, which is the case on ppc32 with 64-bit resource
> addresses. This only seems to have shown up while running under QEMU for
> e500mc target. It may or may be suboptimal that QEMU has an IO base
> address > 32-bits for the e500-pci implementation, but 1) it's still a
> regression and 2) it's more correct to handle things this way.
Where do you see addresses over 32 bits in QEMU's e500-pci, at least
with current mainline QEMU and the mpc8544ds model?
I/O space should be at 0xe1000000.
I'm also not sure what this has to do with the virtual address returned
by ioremap().
> Signed-off-by: Ben Collins <bcollins@ubuntu.com>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
> arch/powerpc/kernel/pci-common.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
> index 8e78e93..be9ced7 100644
> --- a/arch/powerpc/kernel/pci-common.c
> +++ b/arch/powerpc/kernel/pci-common.c
> @@ -1477,9 +1477,15 @@ int pcibios_enable_device(struct pci_dev *dev, int mask)
> return pci_enable_resources(dev, mask);
> }
>
> +/* Before assuming too much here, take care to realize that we need sign
> + * extension from 32-bit pointers to 64-bit resource addresses to work.
> + */
> resource_size_t pcibios_io_space_offset(struct pci_controller *hose)
> {
> - return (unsigned long) hose->io_base_virt - _IO_BASE;
> + long vbase = (long)hose->io_base_virt;
> + long io_base = _IO_BASE;
> +
> + return (resource_size_t)(vbase - io_base);
Why do we want sign extension here?
If we do want it, there are a lot of other places in this file where the
same calculation is done.
-Scott
^ permalink raw reply
* Re: [PATCH] powerpc: Optimise the 64bit optimised __clear_user
From: Benjamin Herrenschmidt @ 2012-06-06 21:20 UTC (permalink / raw)
To: Segher Boessenkool
Cc: mikey, michael, paulus, Anton Blanchard, olof, linuxppc-dev
In-Reply-To: <178E3BC0-C6E2-4E33-BA66-8144F192A151@kernel.crashing.org>
On Wed, 2012-06-06 at 18:40 +0200, Segher Boessenkool wrote:
> > +err1; dcbz r0,r3
>
> There is no such instruction, you probably meant "dcbz 0,r3"?
This reminds me... what would happen if we changed all our
#define r0 0
#define r1 1
etc... to:
#define r0 %r0
#define r1 %r1
?
I'm thinking it might help catch that sort of nasties (and some of them
can be really nasty, such as inverting mfspr/mtspr arguments, or vs ori,
etc... ). I'm sure we'd have a problem with a few macros & inline
constructs but nothing we can't fix..
(Haven't tested ... still home, officially sick :-)
Cheers,
Ben.
^ 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