* Re: [PATCH 0/2] PCI/MSI: Remove arch_msi_check_device()
From: Michael Ellerman @ 2014-09-09 0:43 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-pci@vger.kernel.org, Alexander Gordeev, linuxppc-dev,
linux-kernel@vger.kernel.org
In-Reply-To: <CAErSpo4XN01hgTUpcca=i-=LFV_bM-Cok8614LJzrOfVbx8MAA@mail.gmail.com>
On Fri, 2014-09-05 at 15:27 -0600, Bjorn Helgaas wrote:
> On Fri, Sep 5, 2014 at 3:25 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> > On Sat, Jul 12, 2014 at 01:21:06PM +0200, Alexander Gordeev wrote:
> >> Hello,
> >>
> >> This is a cleanup effort to get rid of useless arch_msi_check_device().
> >> I am not sure what were the reasons for its existence in the first place,
> >> but at the moment it appears totally unnecessary.
> >>
> >> Thanks!
> >>
> >> Cc: linuxppc-dev@lists.ozlabs.org
> >> Cc: linux-pci@vger.kernel.org
> >>
> >> Alexander Gordeev (2):
> >> PCI/MSI/PPC: Remove arch_msi_check_device()
> >> PCI/MSI: Remove arch_msi_check_device()
> >
> > I applied these (with Michael's ack on the first, and v2 of the second) to
> > pci/msi for v3.18, thanks!
>
> Oh, I forgot -- if you'd rather take the first one through the PPC
> tree, you can do that and I can merge the second one later. Let me
> know if you want to do that.
You take them all, that code shouldn't be changing much in the powerpc tree.
If the merge gets messy closer to the merge window we can always take the first
patch into the powerpc tree then.
cheers
^ permalink raw reply
* Re: [PATCH V2 0/2] cpufreq/powernv: Set core pstate to a minimum just before hotplugging it out
From: Rafael J. Wysocki @ 2014-09-09 0:16 UTC (permalink / raw)
To: Viresh Kumar, Preeti U Murthy
Cc: linux-pm@vger.kernel.org, Dirk Brandewie, Shreyas Prabhu,
Linux Kernel Mailing List, Patrick Marlier,
linuxppc-dev@ozlabs.org, Dirk Brandewie
In-Reply-To: <CAKohpo=chEW_TtXWCHa3OEwrqU25TPBES4icjMVxL6nezrf8+w@mail.gmail.com>
On Friday, September 05, 2014 01:11:22 PM Viresh Kumar wrote:
> On 5 September 2014 13:09, Preeti U Murthy <preeti@linux.vnet.ibm.com> wrote:
> > Today cpus go to winkle when they are offlined. Since it is the deepest
> > idle state that we have, it is expected to save good amount of power as compared
> > to online state, where cores can enter nap/fastsleep only which are
> > shallower idle states.
> > However we observed no powersavings with winkle as compared to nap/fastsleep
> > and traced the problem to the pstate of the core being kept at a high even
> > when the core is offline. This can keep the socket pstate high, thus burning
> > power unnecessarily. This patchset fixes this issue.
> >
> > Changes in V2: Changed smp_call_function_any() to smp_call_function_single() in Patch[2/2]
>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
I've queued up the two patches for 3.18, thanks!
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply
* Re: bit fields && data tearing
From: Peter Hurley @ 2014-09-08 23:30 UTC (permalink / raw)
To: James Bottomley
Cc: Jakub Jelinek, One Thousand Gnomes, linux-arch, linux-ia64,
Mikael Pettersson, Oleg Nesterov, linux-kernel, Tony Luck,
Paul Mackerras, H. Peter Anvin, paulmck, linuxppc-dev,
Miroslav Franc, Richard Henderson
In-Reply-To: <1410155407.2027.29.camel@jarvis.lan>
On 09/08/2014 01:50 AM, James Bottomley wrote:
> On Sun, 2014-09-07 at 16:41 -0400, Peter Hurley wrote:
>> On 09/07/2014 03:04 PM, James Bottomley wrote:
>>> On Sun, 2014-09-07 at 09:21 -0700, Paul E. McKenney wrote:
>>>> On Sat, Sep 06, 2014 at 10:07:22PM -0700, James Bottomley wrote:
>>>>> On Thu, 2014-09-04 at 21:06 -0700, Paul E. McKenney wrote:
>>>>>> On Thu, Sep 04, 2014 at 10:47:24PM -0400, Peter Hurley wrote:
>>>>>>> Hi James,
>>>>>>>
>>>>>>> On 09/04/2014 10:11 PM, James Bottomley wrote:
>>>>>>>> On Thu, 2014-09-04 at 17:17 -0700, Paul E. McKenney wrote:
>>>>>>>>> +And there are anti-guarantees:
>>>>>>>>> +
>>>>>>>>> + (*) These guarantees do not apply to bitfields, because compilers often
>>>>>>>>> + generate code to modify these using non-atomic read-modify-write
>>>>>>>>> + sequences. Do not attempt to use bitfields to synchronize parallel
>>>>>>>>> + algorithms.
>>>>>>>>> +
>>>>>>>>> + (*) Even in cases where bitfields are protected by locks, all fields
>>>>>>>>> + in a given bitfield must be protected by one lock. If two fields
>>>>>>>>> + in a given bitfield are protected by different locks, the compiler's
>>>>>>>>> + non-atomic read-modify-write sequences can cause an update to one
>>>>>>>>> + field to corrupt the value of an adjacent field.
>>>>>>>>> +
>>>>>>>>> + (*) These guarantees apply only to properly aligned and sized scalar
>>>>>>>>> + variables. "Properly sized" currently means "int" and "long",
>>>>>>>>> + because some CPU families do not support loads and stores of
>>>>>>>>> + other sizes. ("Some CPU families" is currently believed to
>>>>>>>>> + be only Alpha 21064. If this is actually the case, a different
>>>>>>>>> + non-guarantee is likely to be formulated.)
>>>>>>>>
>>>>>>>> This is a bit unclear. Presumably you're talking about definiteness of
>>>>>>>> the outcome (as in what's seen after multiple stores to the same
>>>>>>>> variable).
>>>>>>>
>>>>>>> No, the last conditions refers to adjacent byte stores from different
>>>>>>> cpu contexts (either interrupt or SMP).
>>>>>>>
>>>>>>>> The guarantees are only for natural width on Parisc as well,
>>>>>>>> so you would get a mess if you did byte stores to adjacent memory
>>>>>>>> locations.
>>>>>>>
>>>>>>> For a simple test like:
>>>>>>>
>>>>>>> struct x {
>>>>>>> long a;
>>>>>>> char b;
>>>>>>> char c;
>>>>>>> char d;
>>>>>>> char e;
>>>>>>> };
>>>>>>>
>>>>>>> void store_bc(struct x *p) {
>>>>>>> p->b = 1;
>>>>>>> p->c = 2;
>>>>>>> }
>>>>>>>
>>>>>>> on parisc, gcc generates separate byte stores
>>>>>>>
>>>>>>> void store_bc(struct x *p) {
>>>>>>> 0: 34 1c 00 02 ldi 1,ret0
>>>>>>> 4: 0f 5c 12 08 stb ret0,4(r26)
>>>>>>> 8: 34 1c 00 04 ldi 2,ret0
>>>>>>> c: e8 40 c0 00 bv r0(rp)
>>>>>>> 10: 0f 5c 12 0a stb ret0,5(r26)
>>>>>>>
>>>>>>> which appears to confirm that on parisc adjacent byte data
>>>>>>> is safe from corruption by concurrent cpu updates; that is,
>>>>>>>
>>>>>>> CPU 0 | CPU 1
>>>>>>> |
>>>>>>> p->b = 1 | p->c = 2
>>>>>>> |
>>>>>>>
>>>>>>> will result in p->b == 1 && p->c == 2 (assume both values
>>>>>>> were 0 before the call to store_bc()).
>>>>>>
>>>>>> What Peter said. I would ask for suggestions for better wording, but
>>>>>> I would much rather be able to say that single-byte reads and writes
>>>>>> are atomic and that aligned-short reads and writes are also atomic.
>>>>>>
>>>>>> Thus far, it looks like we lose only very old Alpha systems, so unless
>>>>>> I hear otherwise, I update my patch to outlaw these very old systems.
>>>>>
>>>>> This isn't universally true according to the architecture manual. The
>>>>> PARISC CPU can make byte to long word stores atomic against the memory
>>>>> bus but not against the I/O bus for instance. Atomicity is a property
>>>>> of the underlying substrate, not of the CPU. Implying that atomicity is
>>>>> a CPU property is incorrect.
>>
>> To go back to this briefly, while it's true that atomicity is not
>> a CPU property, atomicity is not possible if the CPU is not cooperating.
>
> You mean if it doesn't have the bus logic to emit? Like ia32 mostly
> can't do 64 bit writes ... sure.
Yeah, or Alphas that do rmw for byte storage.
>>>> OK, fair point.
>>>>
>>>> But are there in-use-for-Linux PARISC memory fabrics (for normal memory,
>>>> not I/O) that do not support single-byte and double-byte stores?
>>>
>>> For aligned access, I believe that's always the case for the memory bus
>>> (on both 32 and 64 bit systems). However, it only applies to machine
>>> instruction loads and stores of the same width.. If you mix the widths
>>> on the loads and stores, all bets are off. That means you have to
>>> beware of the gcc penchant for coalescing loads and stores: if it sees
>>> two adjacent byte stores it can coalesce them into a short store
>>> instead ... that screws up the atomicity guarantees.
>>
>> Two things: I think that gcc has given up on combining adjacent writes,
>> perhaps because unaligned writes on some arches are prohibitive, so
>> whatever minor optimization was believed to be gained was quickly lost,
>> multi-fold. (Although this might not be the reason since one would
>> expect that gcc would know the alignment of the promoted store).
>
> Um, gcc assumes architecturally correct alignment; that's why it pads
> structures. Only when accessing packed structures will it use the
> lowest unit load/store.
>
> if you have a struct { char a, char b }; and load first a then b with a
> constant gcc will obligingly optimise to a short store.
Um, please re-look at the test above. The exact test you describe is
coded above and compiled with gcc 4.6.3 cross-compiler for parisc using
the kernel compiler options.
In the generated code, please note the _absence_ of a combined write
to two adjacent byte stores.
>> But additionally, even if gcc combines adjacent writes _that are part
>> of the program flow_ then I believe the situation is no worse than
>> would otherwise exist.
>>
>> For instance, given the following:
>>
>> struct x {
>> spinlock_t lock;
>> long a;
>> byte b;
>> byte c;
>> };
>>
>> void locked_store_b(struct x *p)
>> {
>> spin_lock(&p->lock);
>> p->b = 1;
>> spin_unlock(&p->lock);
>> p->c = 2;
>> }
>>
>> Granted, the author probably expects ordered writes of
>> STORE B
>> STORE C
>> but that's not guaranteed because there is no memory barrier
>> ordering B before C.
>
> Yes, there is: loads and stores may not migrate into or out of critical
> sections.
That's a common misconception.
The processor is free to re-order this to:
STORE C
STORE B
UNLOCK
That's because the unlock() only guarantees that:
Stores before the unlock in program order are guaranteed to complete
before the unlock completes. Stores after the unlock _may_ complete
before the unlock completes.
My point was that even if compiler barriers had the same semantics
as memory barriers, the situation would be no worse. That is, code
that is sensitive to memory barriers (like the example I gave above)
would merely have the same fragility with one-way compiler barriers
(with respect to the compiler combining writes).
That's what I meant by "no worse than would otherwise exist".
>> I see no problem with gcc write-combining in the absence of
>> memory barriers (as long as alignment is still respected,
>> ie., the size-promoted write is still naturally aligned). The
>> combined write is still atomic.
>
> Actual alignment is pretty irrelevant. That's why all architectures
> which require alignment also have to implement misaligned traps ... this
> is a fundamental requirement of the networking code, for instance.
>
> The main problem is gcc thinking there's a misalignment (or a packed
> structure). That causes splitting of loads and stores and that destroys
> atomicity.
Yeah, the extra requirement I added is basically nonsense, since the
only issue is what instructions the compiler is emitting. So if compiler
thinks the alignment is natural and combines the writes -- ok. If the
compiler thinks the alignment is off and doesn't combine the writes --
also ok.
Regards,
Peter Hurley
^ permalink raw reply
* Re: bit fields && data tearing
From: Peter Hurley @ 2014-09-08 22:47 UTC (permalink / raw)
To: H. Peter Anvin, One Thousand Gnomes
Cc: Jakub Jelinek, linux-arch@vger.kernel.org, Tony Luck,
linux-ia64@vger.kernel.org, Oleg Nesterov,
linux-kernel@vger.kernel.org, David Laight, Paul Mackerras,
linux-alpha, Paul E. McKenney, linuxppc-dev@lists.ozlabs.org,
Miroslav Franc, Richard Henderson
In-Reply-To: <540DEE6C.2060904@zytor.com>
On 09/08/2014 01:59 PM, H. Peter Anvin wrote:
> On 09/08/2014 10:52 AM, One Thousand Gnomes wrote:
>> On Fri, 05 Sep 2014 08:41:52 -0700
>> "H. Peter Anvin" <hpa@zytor.com> wrote:
>>
>>> On 09/05/2014 08:31 AM, Peter Hurley wrote:
>>>>
>>>> Which is a bit ironic because I remember when Digital had a team
>>>> working on emulating native x86 apps on Alpha/NT.
>>>>
>>>
>>> Right, because the x86 architecture was obsolete and would never scale...
>>
>> Talking about "not scaling" can anyone explain how a "you need to use
>> set_bit() and friends" bug report scaled into a hundred message plus
>> discussion about ambiguous properties of processors (and nobody has
>> audited all the embedded platforms we support yet, or the weirder ARMs)
>> and a propsal to remove Alpha support.
>>
>> Wouldn't it be *much* simpler to do what I suggested in the first place
>> and use the existing intended for purpose, deliberately put there,
>> functions for atomic bitops, because they are fast on sane processors and
>> they work on everything else.
>>
>> I think the whole "removing Alpha EV5" support is basically bonkers. Just
>> use set_bit in the tty layer. Alpha will continue to work as well as it
>> always has done and you won't design out support for any future processor
>> that turns out not to do byte aligned stores.
>>
>> Alan
>>
>
> Is *that* what we are talking about? I was added to this conversation
> in the middle where it had already generalized, so I had no idea.
No, this is just what brought this craziness to my attention.
For example, byte- and short-sized circular buffers could not possibly
be safe either, when the head nears the tail.
Who has audited global storage and ensured that _every_ byte-sized write
doesn't happen to be adjacent to some other storage that may not happen
to be protected by the same (or any) lock?
Regards,
Peter Hurley
^ permalink raw reply
* Re: bit fields && data tearing
From: James Bottomley @ 2014-09-08 22:43 UTC (permalink / raw)
To: Chris Metcalf
Cc: Jakub Jelinek, One Thousand Gnomes, linux-arch, linux-ia64,
Peter Hurley, Mikael Pettersson, Oleg Nesterov, linux-kernel,
Tony Luck, Paul Mackerras, H. Peter Anvin, paulmck, linuxppc-dev,
Miroslav Franc, Richard Henderson
In-Reply-To: <540E1587.3060108@tilera.com>
On Mon, 2014-09-08 at 16:45 -0400, Chris Metcalf wrote:
> On 9/8/2014 1:50 AM, James Bottomley wrote:
> > Actual alignment is pretty irrelevant. That's why all architectures
> > which require alignment also have to implement misaligned traps ... this
> > is a fundamental requirement of the networking code, for instance.
>
> Can you clarify what you think the requirement is? The tile architecture
> doesn't support misaligned load/store in general, but we do support it for
> userspace (using a nifty JIT approach with a direct-map hash table kept
> in userspace), and also for get_user/put_user. But that's it, and,
> the networking subsystem works fine for us.
This was years ago (possibly decades). We had to implement in-kernel
unaligned traps for the networking layer because it could access short
and int fields that weren't of the correct alignment when processing
packets. It that's all corrected now, we wouldn't really notice (except
a bit of a speed up since an unaligned trap effectively places the
broken out instructions into the bit stream).
James
> Occasionally we report bugs for driver code that doesn't use the
> get_unaligned_xxx() macros and friends, and our fixes are generally taken
> upstream.
^ permalink raw reply
* Re: bit fields && data tearing
From: James Bottomley @ 2014-09-08 22:39 UTC (permalink / raw)
To: H. Peter Anvin
Cc: Jakub Jelinek, One Thousand Gnomes, Tony Luck, linux-ia64,
Peter Hurley, Mikael Pettersson, Oleg Nesterov, linux-kernel,
Paul Mackerras, linux-arch, paulmck, linuxppc-dev, Miroslav Franc,
Richard Henderson
In-Reply-To: <540DFFB2.4000509@zytor.com>
On Mon, 2014-09-08 at 12:12 -0700, H. Peter Anvin wrote:
> On 09/08/2014 12:09 PM, James Bottomley wrote:
> >
> > Um, I think you need to re-read the thread; that's not what I said at
> > all. It's even written lower down: "PA can't do atomic bit sets (no
> > atomic RMW except the ldcw operation) it can do atomic writes to
> > fundamental sizes (byte, short, int, long) provided gcc emits the
> > correct primitive". The original question was whether atomicity
> > required native bus width access, which we currently assume, so there's
> > no extant problem.
> >
>
> The issue at hand was whether or not partially overlapped (but natually
> aligned) writes can pass each other. *This* is the aggressive
> relaxation to which I am referring.
I don't understand what you mean by "pass each other". Atomicity
guarantees are not ordering guarantees in a SMP environment. The
guarantee is that if you follow the rules when two CPUs update the same
natural width aligned object simultaneously using the same primitive,
the result is either one or the other of their updates. Which one wins
(the ordering) isn't defined.
James
> I would guess that that is a very unusual constraint.
^ permalink raw reply
* Re: bit fields && data tearing
From: Chris Metcalf @ 2014-09-08 20:45 UTC (permalink / raw)
To: James Bottomley, Peter Hurley
Cc: Jakub Jelinek, One Thousand Gnomes, linux-arch, linux-ia64,
Mikael Pettersson, Oleg Nesterov, linux-kernel, Tony Luck,
Paul Mackerras, H. Peter Anvin, paulmck, linuxppc-dev,
Miroslav Franc, Richard Henderson
In-Reply-To: <1410155407.2027.29.camel@jarvis.lan>
On 9/8/2014 1:50 AM, James Bottomley wrote:
> Actual alignment is pretty irrelevant. That's why all architectures
> which require alignment also have to implement misaligned traps ... this
> is a fundamental requirement of the networking code, for instance.
Can you clarify what you think the requirement is? The tile architecture
doesn't support misaligned load/store in general, but we do support it for
userspace (using a nifty JIT approach with a direct-map hash table kept
in userspace), and also for get_user/put_user. But that's it, and,
the networking subsystem works fine for us.
Occasionally we report bugs for driver code that doesn't use the
get_unaligned_xxx() macros and friends, and our fixes are generally taken
upstream.
--
Chris Metcalf, Tilera Corp.
http://www.tilera.com
^ permalink raw reply
* [PATCH 3/3] sched: BUG when stack end location is over written
From: Aaron Tomlin @ 2014-09-08 19:23 UTC (permalink / raw)
To: peterz
Cc: dzickus, jcastillo, riel, minchan, bmr, x86, oleg, rostedt,
linux-kernel, hannes, mingo, aneesh.kumar, akpm, atomlin, tglx,
linuxppc-dev, akpm, pzijlstr
In-Reply-To: <1410204197-31204-1-git-send-email-atomlin@redhat.com>
Currently in the event of a stack overrun a call to schedule()
does not check for this type of corruption. This corruption is
often silent and can go unnoticed. However once the corrupted
region is examined at a later stage, the outcome is undefined
and often results in a sporadic page fault which cannot be
handled.
This patch checks for a stack overrun and takes appropriate
action since the damage is already done, there is no point
in continuing.
Signed-off-by: Aaron Tomlin <atomlin@redhat.com>
---
kernel/sched/core.c | 4 ++++
lib/Kconfig.debug | 12 ++++++++++++
2 files changed, 16 insertions(+)
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index ec1a286..182b2d1 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -2660,6 +2660,10 @@ static noinline void __schedule_bug(struct task_struct *prev)
*/
static inline void schedule_debug(struct task_struct *prev)
{
+#ifdef CONFIG_SCHED_STACK_END_CHECK
+ if (unlikely(task_stack_end_corrupted(prev)))
+ BUG();
+#endif
/*
* Test if we are atomic. Since do_exit() needs to call into
* schedule() atomically, we ignore that path. Otherwise whine
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index a285900..2a8280a 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -824,6 +824,18 @@ config SCHEDSTATS
application, you can say N to avoid the very slight overhead
this adds.
+config SCHED_STACK_END_CHECK
+ bool "Detect stack corruption on calls to schedule()"
+ depends on DEBUG_KERNEL
+ default y
+ help
+ This option checks for a stack overrun on calls to schedule().
+ If the stack end location is found to be over written always panic as
+ the content of the corrupted region can no longer be trusted.
+ This is to ensure no erroneous behaviour occurs which could result in
+ data corruption or a sporadic crash at a later stage once the region
+ is examined. The runtime overhead introduced is minimal.
+
config TIMER_STATS
bool "Collect kernel timers statistics"
depends on DEBUG_KERNEL && PROC_FS
--
1.9.3
^ permalink raw reply related
* [PATCH v2 0/3] sched: Always check the integrity of the canary
From: Aaron Tomlin @ 2014-09-08 19:23 UTC (permalink / raw)
To: peterz
Cc: dzickus, jcastillo, riel, minchan, bmr, x86, oleg, rostedt,
linux-kernel, hannes, mingo, aneesh.kumar, akpm, atomlin, tglx,
linuxppc-dev, akpm, pzijlstr
In-Reply-To: <20140904161124.GD1436@atomlin.usersys.redhat.com>
Currently in the event of a stack overrun a call to schedule()
does not check for this type of corruption. This corruption is
often silent and can go unnoticed. However once the corrupted
region is examined at a later stage, the outcome is undefined
and often results in a sporadic page fault which cannot be
handled.
The first patch adds a canary to init_task's end of stack.
While the second patch provides a helper to determine the
integrity of the canary. The third checks for a stack
overrun and takes appropriate action since the damage
is already done, there is no point in continuing.
Changes since v1:
* Rebased against v3.17-rc4
* Add a canary to init_task - Oleg Nesterov
* Fix various code formatting issues - Peter Zijlstra
* Introduce Kconfig option - Peter Zijlstra
Aaron Tomlin (3):
init/main.c: Give init_task a canary
sched: Add helper for task stack page overrun checking
sched: BUG when stack end location is over written
arch/powerpc/mm/fault.c | 5 +----
arch/x86/mm/fault.c | 5 +----
include/linux/sched.h | 4 ++++
init/main.c | 1 +
kernel/fork.c | 12 +++++++++---
kernel/sched/core.c | 4 ++++
kernel/trace/trace_stack.c | 4 +---
lib/Kconfig.debug | 12 ++++++++++++
8 files changed, 33 insertions(+), 14 deletions(-)
--
1.9.3
^ permalink raw reply
* [PATCH 1/3] init/main.c: Give init_task a canary
From: Aaron Tomlin @ 2014-09-08 19:23 UTC (permalink / raw)
To: peterz
Cc: dzickus, jcastillo, riel, minchan, bmr, x86, oleg, rostedt,
linux-kernel, hannes, mingo, aneesh.kumar, akpm, atomlin, tglx,
linuxppc-dev, akpm, pzijlstr
In-Reply-To: <1410204197-31204-1-git-send-email-atomlin@redhat.com>
Tasks get their end of stack set to STACK_END_MAGIC with the
aim to catch stack overruns. Currently this feature does not
apply to init_task. This patch removes this restriction.
Note that a similar patch was posted by Prarit Bhargava [1]
some time ago but was never merged.
[1]: http://marc.info/?l=linux-kernel&m=127144305403241&w=2
Signed-off-by: Aaron Tomlin <atomlin@redhat.com>
---
arch/powerpc/mm/fault.c | 3 +--
arch/x86/mm/fault.c | 3 +--
include/linux/sched.h | 2 ++
init/main.c | 1 +
kernel/fork.c | 12 +++++++++---
kernel/trace/trace_stack.c | 4 +---
6 files changed, 15 insertions(+), 10 deletions(-)
diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index 51ab9e7..35d0760c 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -30,7 +30,6 @@
#include <linux/kprobes.h>
#include <linux/kdebug.h>
#include <linux/perf_event.h>
-#include <linux/magic.h>
#include <linux/ratelimit.h>
#include <linux/context_tracking.h>
@@ -538,7 +537,7 @@ void bad_page_fault(struct pt_regs *regs, unsigned long address, int sig)
regs->nip);
stackend = end_of_stack(current);
- if (current != &init_task && *stackend != STACK_END_MAGIC)
+ if (*stackend != STACK_END_MAGIC)
printk(KERN_ALERT "Thread overran stack, or stack corrupted\n");
die("Kernel access of bad area", regs, sig);
diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index a241946..bc23a70 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -3,7 +3,6 @@
* Copyright (C) 2001, 2002 Andi Kleen, SuSE Labs.
* Copyright (C) 2008-2009, Red Hat Inc., Ingo Molnar
*/
-#include <linux/magic.h> /* STACK_END_MAGIC */
#include <linux/sched.h> /* test_thread_flag(), ... */
#include <linux/kdebug.h> /* oops_begin/end, ... */
#include <linux/module.h> /* search_exception_table */
@@ -710,7 +709,7 @@ no_context(struct pt_regs *regs, unsigned long error_code,
show_fault_oops(regs, error_code, address);
stackend = end_of_stack(tsk);
- if (tsk != &init_task && *stackend != STACK_END_MAGIC)
+ if (*stackend != STACK_END_MAGIC)
printk(KERN_EMERG "Thread overran stack, or stack corrupted\n");
tsk->thread.cr2 = address;
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 5c2c885..4dee9d7 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -57,6 +57,7 @@ struct sched_param {
#include <linux/llist.h>
#include <linux/uidgid.h>
#include <linux/gfp.h>
+#include <linux/magic.h>
#include <asm/processor.h>
@@ -2636,6 +2637,7 @@ static inline unsigned long stack_not_used(struct task_struct *p)
return (unsigned long)n - (unsigned long)end_of_stack(p);
}
#endif
+extern void task_stack_end_magic(struct task_struct *tsk);
/* set thread flags in other task's structures
* - see asm/thread_info.h for TIF_xxxx flags available
diff --git a/init/main.c b/init/main.c
index bb1aed9..bcdabf3a 100644
--- a/init/main.c
+++ b/init/main.c
@@ -508,6 +508,7 @@ asmlinkage __visible void __init start_kernel(void)
* lockdep hash:
*/
lockdep_init();
+ task_stack_end_magic(&init_task);
smp_setup_processor_id();
debug_objects_early_init();
diff --git a/kernel/fork.c b/kernel/fork.c
index 0cf9cdb..67cb1e3 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -294,11 +294,18 @@ int __weak arch_dup_task_struct(struct task_struct *dst,
return 0;
}
+void task_stack_end_magic(struct task_struct *tsk)
+{
+ unsigned long *stackend;
+
+ stackend = end_of_stack(tsk);
+ *stackend = STACK_END_MAGIC; /* for overflow detection */
+}
+
static struct task_struct *dup_task_struct(struct task_struct *orig)
{
struct task_struct *tsk;
struct thread_info *ti;
- unsigned long *stackend;
int node = tsk_fork_get_node(orig);
int err;
@@ -328,8 +335,7 @@ static struct task_struct *dup_task_struct(struct task_struct *orig)
setup_thread_stack(tsk, orig);
clear_user_return_notifier(tsk);
clear_tsk_need_resched(tsk);
- stackend = end_of_stack(tsk);
- *stackend = STACK_END_MAGIC; /* for overflow detection */
+ task_stack_end_magic(tsk);
#ifdef CONFIG_CC_STACKPROTECTOR
tsk->stack_canary = get_random_int();
diff --git a/kernel/trace/trace_stack.c b/kernel/trace/trace_stack.c
index 8a4e5cb..1636e41 100644
--- a/kernel/trace/trace_stack.c
+++ b/kernel/trace/trace_stack.c
@@ -13,7 +13,6 @@
#include <linux/sysctl.h>
#include <linux/init.h>
#include <linux/fs.h>
-#include <linux/magic.h>
#include <asm/setup.h>
@@ -171,8 +170,7 @@ check_stack(unsigned long ip, unsigned long *stack)
i++;
}
- if ((current != &init_task &&
- *(end_of_stack(current)) != STACK_END_MAGIC)) {
+ if (*end_of_stack(current) != STACK_END_MAGIC) {
print_max_stack();
BUG();
}
--
1.9.3
^ permalink raw reply related
* [PATCH 2/3] sched: Add helper for task stack page overrun checking
From: Aaron Tomlin @ 2014-09-08 19:23 UTC (permalink / raw)
To: peterz
Cc: dzickus, jcastillo, riel, minchan, bmr, x86, oleg, rostedt,
linux-kernel, hannes, mingo, aneesh.kumar, akpm, atomlin, tglx,
linuxppc-dev, akpm, pzijlstr
In-Reply-To: <1410204197-31204-1-git-send-email-atomlin@redhat.com>
This facility is used in a few places so let's introduce
a helper function to improve code readability.
Signed-off-by: Aaron Tomlin <atomlin@redhat.com>
---
arch/powerpc/mm/fault.c | 4 +---
arch/x86/mm/fault.c | 4 +---
include/linux/sched.h | 2 ++
kernel/trace/trace_stack.c | 2 +-
4 files changed, 5 insertions(+), 7 deletions(-)
diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index 35d0760c..99b2f27 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -507,7 +507,6 @@ bail:
void bad_page_fault(struct pt_regs *regs, unsigned long address, int sig)
{
const struct exception_table_entry *entry;
- unsigned long *stackend;
/* Are we prepared to handle this fault? */
if ((entry = search_exception_tables(regs->nip)) != NULL) {
@@ -536,8 +535,7 @@ void bad_page_fault(struct pt_regs *regs, unsigned long address, int sig)
printk(KERN_ALERT "Faulting instruction address: 0x%08lx\n",
regs->nip);
- stackend = end_of_stack(current);
- if (*stackend != STACK_END_MAGIC)
+ if (task_stack_end_corrupted(current))
printk(KERN_ALERT "Thread overran stack, or stack corrupted\n");
die("Kernel access of bad area", regs, sig);
diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index bc23a70..6240bc7 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -648,7 +648,6 @@ no_context(struct pt_regs *regs, unsigned long error_code,
unsigned long address, int signal, int si_code)
{
struct task_struct *tsk = current;
- unsigned long *stackend;
unsigned long flags;
int sig;
@@ -708,8 +707,7 @@ no_context(struct pt_regs *regs, unsigned long error_code,
show_fault_oops(regs, error_code, address);
- stackend = end_of_stack(tsk);
- if (*stackend != STACK_END_MAGIC)
+ if (task_stack_end_corrupted(tsk))
printk(KERN_EMERG "Thread overran stack, or stack corrupted\n");
tsk->thread.cr2 = address;
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 4dee9d7..6e07cb9 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -2615,6 +2615,8 @@ static inline unsigned long *end_of_stack(struct task_struct *p)
}
#endif
+#define task_stack_end_corrupted(task) \
+ (*(end_of_stack(task)) != STACK_END_MAGIC)
static inline int object_is_on_stack(void *obj)
{
diff --git a/kernel/trace/trace_stack.c b/kernel/trace/trace_stack.c
index 1636e41..16eddb3 100644
--- a/kernel/trace/trace_stack.c
+++ b/kernel/trace/trace_stack.c
@@ -170,7 +170,7 @@ check_stack(unsigned long ip, unsigned long *stack)
i++;
}
- if (*end_of_stack(current) != STACK_END_MAGIC) {
+ if (task_stack_end_corrupted(current)) {
print_max_stack();
BUG();
}
--
1.9.3
^ permalink raw reply related
* Re: bit fields && data tearing
From: H. Peter Anvin @ 2014-09-08 19:12 UTC (permalink / raw)
To: James Bottomley
Cc: Jakub Jelinek, One Thousand Gnomes, Tony Luck, linux-ia64,
Peter Hurley, Mikael Pettersson, Oleg Nesterov, linux-kernel,
Paul Mackerras, linux-arch, paulmck, linuxppc-dev, Miroslav Franc,
Richard Henderson
In-Reply-To: <1410203369.2027.56.camel@jarvis.lan>
On 09/08/2014 12:09 PM, James Bottomley wrote:
>
> Um, I think you need to re-read the thread; that's not what I said at
> all. It's even written lower down: "PA can't do atomic bit sets (no
> atomic RMW except the ldcw operation) it can do atomic writes to
> fundamental sizes (byte, short, int, long) provided gcc emits the
> correct primitive". The original question was whether atomicity
> required native bus width access, which we currently assume, so there's
> no extant problem.
>
The issue at hand was whether or not partially overlapped (but natually
aligned) writes can pass each other. *This* is the aggressive
relaxation to which I am referring.
I would guess that that is a very unusual constraint.
-hpa
^ permalink raw reply
* Re: bit fields && data tearing
From: One Thousand Gnomes @ 2014-09-08 19:17 UTC (permalink / raw)
To: H. Peter Anvin
Cc: Jakub Jelinek, linux-arch@vger.kernel.org, Tony Luck,
linux-ia64@vger.kernel.org, Peter Hurley, Oleg Nesterov,
linux-kernel@vger.kernel.org, David Laight, Paul Mackerras,
linux-alpha, Paul E. McKenney, linuxppc-dev@lists.ozlabs.org,
Miroslav Franc, Richard Henderson
In-Reply-To: <540DEE6C.2060904@zytor.com>
> > I think the whole "removing Alpha EV5" support is basically bonkers. Just
> > use set_bit in the tty layer. Alpha will continue to work as well as it
> > always has done and you won't design out support for any future processor
> > that turns out not to do byte aligned stores.
> >
> > Alan
> >
>
> Is *that* what we are talking about? I was added to this conversation
> in the middle where it had already generalized, so I had no idea.
>
> -hpa
Yes there are some flags in the tty layer that are vulnerable to this
(although they've been vulnerable to it and missing a lock since last
century with no known ill effects).
It's not as if this is causing any work to anyone beyond using the
standard API, no API needs to change.
Alan
^ permalink raw reply
* Re: bit fields && data tearing
From: H. Peter Anvin @ 2014-09-08 19:12 UTC (permalink / raw)
To: James Bottomley
Cc: Jakub Jelinek, One Thousand Gnomes, Tony Luck, linux-ia64,
Peter Hurley, Mikael Pettersson, Oleg Nesterov, linux-kernel,
Paul Mackerras, linux-arch, paulmck, linuxppc-dev, Miroslav Franc,
Richard Henderson
In-Reply-To: <1410203369.2027.56.camel@jarvis.lan>
On 09/08/2014 12:09 PM, James Bottomley wrote:
>
> Um, I think you need to re-read the thread; that's not what I said at
> all. It's even written lower down: "PA can't do atomic bit sets (no
> atomic RMW except the ldcw operation) it can do atomic writes to
> fundamental sizes (byte, short, int, long) provided gcc emits the
> correct primitive". The original question was whether atomicity
> required native bus width access, which we currently assume, so there's
> no extant problem.
>
The issue at hand was whether or not partially overlapped (but natually
aligned) writes can pass each other. *This* is the aggressive
relaxation to which I am referring.
I would guess that that is a very unusual constraint.
-hpa
^ permalink raw reply
* Re: bit fields && data tearing
From: James Bottomley @ 2014-09-08 19:09 UTC (permalink / raw)
To: H. Peter Anvin
Cc: Jakub Jelinek, One Thousand Gnomes, Tony Luck, linux-ia64,
Peter Hurley, Mikael Pettersson, Oleg Nesterov, linux-kernel,
Paul Mackerras, linux-arch, paulmck, linuxppc-dev, Miroslav Franc,
Richard Henderson
In-Reply-To: <540DF17C.9080509@zytor.com>
On Mon, 2014-09-08 at 11:12 -0700, H. Peter Anvin wrote:
> On 09/07/2014 10:56 PM, James Bottomley wrote:
> > On Sun, 2014-09-07 at 16:39 -0700, H. Peter Anvin wrote:
> >> How many PARISC systems do we have that actually do real work on Linux?
> >
> > I'd be very surprised if this problem didn't exist on all alignment
> > requiring architectures, like PPC and Sparc as well. I know it would be
> > very convenient if all the world were an x86 ... but it would also be
> > very boring as well.
>
> I wouldn't be so sure about that. That is a pretty aggressive
> relaxation of ordering that PARISC has enacted here, kind of like the
> Alpha "we don't need no stinking byte accesses".
Um, I think you need to re-read the thread; that's not what I said at
all. It's even written lower down: "PA can't do atomic bit sets (no
atomic RMW except the ldcw operation) it can do atomic writes to
fundamental sizes (byte, short, int, long) provided gcc emits the
correct primitive". The original question was whether atomicity
required native bus width access, which we currently assume, so there's
no extant problem.
> > The rules for coping with it are well known and a relaxation of what we
> > currently do in the kernel, so I don't see what the actual problem is.
> >
> > In the context of this thread, PA can't do atomic bit sets (no atomic
> > RMW except the ldcw operation) it can do atomic writes to fundamental
> > sizes (byte, short, int, long) provided gcc emits the correct primitive
> > (there are lots of gotchas in this, but that's not an architectural
> > problem). These atomicity guarantees depend on the underlying storage
> > and are respected for main memory but not for any other type of bus.
>
> So I'm not trying to push the "all the world is an x86"... certainly not
> given that x86 has abnormally strict ordering rules and so is itself an
> outlier. What I *don't* really want to have to deal with is going
> through more than causal effort to accommodate outliers which no longer
> have any real value -- we have too much work to do.
What accommodation? I was just explaining the architectural atomicity
rules on PA. How they're bus dependent (which isn't a PA restriction)
and how the compiler can mess them up. None of the current PA atomicity
rules conflicts with anything we do today ... the danger is that moving
to implicit atomicity gives us more scope for compiler problems ... but
that's not PA specific either; any architecture requiring alignment has
this problem.
James
^ permalink raw reply
* Re: bit fields && data tearing
From: James Bottomley @ 2014-09-08 18:13 UTC (permalink / raw)
To: One Thousand Gnomes
Cc: Jakub Jelinek, linux-arch@vger.kernel.org, Tony Luck,
linux-ia64@vger.kernel.org, Peter Hurley, linux-alpha,
Oleg Nesterov, linux-kernel@vger.kernel.org, David Laight,
Paul Mackerras, H. Peter Anvin, Paul E. McKenney,
linuxppc-dev@lists.ozlabs.org, Miroslav Franc, Richard Henderson
In-Reply-To: <20140908185240.21f52ca0@alan.etchedpixels.co.uk>
On Mon, 2014-09-08 at 18:52 +0100, One Thousand Gnomes wrote:
> On Fri, 05 Sep 2014 08:41:52 -0700
> "H. Peter Anvin" <hpa@zytor.com> wrote:
>
> > On 09/05/2014 08:31 AM, Peter Hurley wrote:
> > >
> > > Which is a bit ironic because I remember when Digital had a team
> > > working on emulating native x86 apps on Alpha/NT.
> > >
> >
> > Right, because the x86 architecture was obsolete and would never scale...
>
> Talking about "not scaling" can anyone explain how a "you need to use
> set_bit() and friends" bug report scaled into a hundred message plus
> discussion about ambiguous properties of processors (and nobody has
> audited all the embedded platforms we support yet, or the weirder ARMs)
> and a propsal to remove Alpha support.
>
> Wouldn't it be *much* simpler to do what I suggested in the first place
> and use the existing intended for purpose, deliberately put there,
> functions for atomic bitops, because they are fast on sane processors and
> they work on everything else.
>
> I think the whole "removing Alpha EV5" support is basically bonkers. Just
> use set_bit in the tty layer. Alpha will continue to work as well as it
> always has done and you won't design out support for any future processor
> that turns out not to do byte aligned stores.
Seconded. We implement via hashed spinlocks on PA ... but hey, we're
not the fastest architecture anyway and semantically it just works.
James
^ permalink raw reply
* Re: bit fields && data tearing
From: H. Peter Anvin @ 2014-09-08 18:12 UTC (permalink / raw)
To: James Bottomley
Cc: Jakub Jelinek, One Thousand Gnomes, Tony Luck, linux-ia64,
Peter Hurley, Mikael Pettersson, Oleg Nesterov, linux-kernel,
Paul Mackerras, linux-arch, paulmck, linuxppc-dev, Miroslav Franc,
Richard Henderson
In-Reply-To: <1410155802.2027.36.camel@jarvis.lan>
On 09/07/2014 10:56 PM, James Bottomley wrote:
> On Sun, 2014-09-07 at 16:39 -0700, H. Peter Anvin wrote:
>> How many PARISC systems do we have that actually do real work on Linux?
>
> I'd be very surprised if this problem didn't exist on all alignment
> requiring architectures, like PPC and Sparc as well. I know it would be
> very convenient if all the world were an x86 ... but it would also be
> very boring as well.
I wouldn't be so sure about that. That is a pretty aggressive
relaxation of ordering that PARISC has enacted here, kind of like the
Alpha "we don't need no stinking byte accesses".
> The rules for coping with it are well known and a relaxation of what we
> currently do in the kernel, so I don't see what the actual problem is.
>
> In the context of this thread, PA can't do atomic bit sets (no atomic
> RMW except the ldcw operation) it can do atomic writes to fundamental
> sizes (byte, short, int, long) provided gcc emits the correct primitive
> (there are lots of gotchas in this, but that's not an architectural
> problem). These atomicity guarantees depend on the underlying storage
> and are respected for main memory but not for any other type of bus.
So I'm not trying to push the "all the world is an x86"... certainly not
given that x86 has abnormally strict ordering rules and so is itself an
outlier. What I *don't* really want to have to deal with is going
through more than causal effort to accommodate outliers which no longer
have any real value -- we have too much work to do.
-hpa
^ permalink raw reply
* Re: bit fields && data tearing
From: H. Peter Anvin @ 2014-09-08 17:59 UTC (permalink / raw)
To: One Thousand Gnomes
Cc: Jakub Jelinek, linux-arch@vger.kernel.org, Tony Luck,
linux-ia64@vger.kernel.org, Peter Hurley, Oleg Nesterov,
linux-kernel@vger.kernel.org, David Laight, Paul Mackerras,
linux-alpha, Paul E. McKenney, linuxppc-dev@lists.ozlabs.org,
Miroslav Franc, Richard Henderson
In-Reply-To: <20140908185240.21f52ca0@alan.etchedpixels.co.uk>
On 09/08/2014 10:52 AM, One Thousand Gnomes wrote:
> On Fri, 05 Sep 2014 08:41:52 -0700
> "H. Peter Anvin" <hpa@zytor.com> wrote:
>
>> On 09/05/2014 08:31 AM, Peter Hurley wrote:
>>>
>>> Which is a bit ironic because I remember when Digital had a team
>>> working on emulating native x86 apps on Alpha/NT.
>>>
>>
>> Right, because the x86 architecture was obsolete and would never scale...
>
> Talking about "not scaling" can anyone explain how a "you need to use
> set_bit() and friends" bug report scaled into a hundred message plus
> discussion about ambiguous properties of processors (and nobody has
> audited all the embedded platforms we support yet, or the weirder ARMs)
> and a propsal to remove Alpha support.
>
> Wouldn't it be *much* simpler to do what I suggested in the first place
> and use the existing intended for purpose, deliberately put there,
> functions for atomic bitops, because they are fast on sane processors and
> they work on everything else.
>
> I think the whole "removing Alpha EV5" support is basically bonkers. Just
> use set_bit in the tty layer. Alpha will continue to work as well as it
> always has done and you won't design out support for any future processor
> that turns out not to do byte aligned stores.
>
> Alan
>
Is *that* what we are talking about? I was added to this conversation
in the middle where it had already generalized, so I had no idea.
-hpa
^ permalink raw reply
* Re: bit fields && data tearing
From: One Thousand Gnomes @ 2014-09-08 17:52 UTC (permalink / raw)
To: H. Peter Anvin
Cc: Jakub Jelinek, linux-arch@vger.kernel.org, Tony Luck,
linux-ia64@vger.kernel.org, Peter Hurley, Oleg Nesterov,
linux-kernel@vger.kernel.org, David Laight, Paul Mackerras,
linux-alpha, Paul E. McKenney, linuxppc-dev@lists.ozlabs.org,
Miroslav Franc, Richard Henderson
In-Reply-To: <5409D9C0.7030403@zytor.com>
On Fri, 05 Sep 2014 08:41:52 -0700
"H. Peter Anvin" <hpa@zytor.com> wrote:
> On 09/05/2014 08:31 AM, Peter Hurley wrote:
> >
> > Which is a bit ironic because I remember when Digital had a team
> > working on emulating native x86 apps on Alpha/NT.
> >
>
> Right, because the x86 architecture was obsolete and would never scale...
Talking about "not scaling" can anyone explain how a "you need to use
set_bit() and friends" bug report scaled into a hundred message plus
discussion about ambiguous properties of processors (and nobody has
audited all the embedded platforms we support yet, or the weirder ARMs)
and a propsal to remove Alpha support.
Wouldn't it be *much* simpler to do what I suggested in the first place
and use the existing intended for purpose, deliberately put there,
functions for atomic bitops, because they are fast on sane processors and
they work on everything else.
I think the whole "removing Alpha EV5" support is basically bonkers. Just
use set_bit in the tty layer. Alpha will continue to work as well as it
always has done and you won't design out support for any future processor
that turns out not to do byte aligned stores.
Alan
^ permalink raw reply
* Re: [PATCH] pseries: Fix endianness in cpu hotplug and hotremove
From: Thomas Falcon @ 2014-09-08 15:14 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <540DB82D.9070403@linux.vnet.ibm.com>
[-- Attachment #1: Type: text/plain, Size: 9653 bytes --]
I guess we were both working on it independently. I had made the
changes to hotplug a cpu a few weeks ago, but was blocked on removing a
cpu. Last week I realized what was blocking me and fixed cpu removal,
so I sent the patch along. Since Bharata has already submitted a patch
that will handle hotplugging, I'll resubmit this with only the changes
needed to remove a cpu.
Tom
On 09/08/2014 09:07 AM, Nathan Fontenot wrote:
> It looks like you have a lot of the same changes as the patch Bharata
> sent out last week. Including the one issue I saw in Bharata's patch
> below.
>
> On 09/05/2014 02:09 PM, Thomas Falcon wrote:
>> This patch attempts to ensure that all values are in the proper
>> endianness format when both hotadding and hotremoving cpus.
>>
>> Signed-off-by: Thomas Falcon <tlfalcon@linux.vnet.ibm.com>
>> ---
>> arch/powerpc/platforms/pseries/dlpar.c | 56 ++++++++++++++--------------
>> arch/powerpc/platforms/pseries/hotplug-cpu.c | 20 +++++-----
>> 2 files changed, 38 insertions(+), 38 deletions(-)
>>
>> diff --git a/arch/powerpc/platforms/pseries/dlpar.c b/arch/powerpc/platforms/pseries/dlpar.c
>> index a2450b8..c1d7e40 100644
>> --- a/arch/powerpc/platforms/pseries/dlpar.c
>> +++ b/arch/powerpc/platforms/pseries/dlpar.c
>> @@ -24,11 +24,11 @@
>> #include <asm/rtas.h>
>>
>> struct cc_workarea {
>> - u32 drc_index;
>> - u32 zero;
>> - u32 name_offset;
>> - u32 prop_length;
>> - u32 prop_offset;
>> + __be32 drc_index;
>> + __be32 zero;
>> + __be32 name_offset;
>> + __be32 prop_length;
>> + __be32 prop_offset;
>> };
>>
>> void dlpar_free_cc_property(struct property *prop)
>> @@ -48,11 +48,11 @@ static struct property *dlpar_parse_cc_property(struct cc_workarea *ccwa)
>> if (!prop)
>> return NULL;
>>
>> - name = (char *)ccwa + ccwa->name_offset;
>> + name = (char *)ccwa + be32_to_cpu(ccwa->name_offset);
>> prop->name = kstrdup(name, GFP_KERNEL);
>>
>> - prop->length = ccwa->prop_length;
>> - value = (char *)ccwa + ccwa->prop_offset;
>> + prop->length = be32_to_cpu(ccwa->prop_length);
>> + value = (char *)ccwa + be32_to_cpu(ccwa->prop_offset);
>> prop->value = kmemdup(value, prop->length, GFP_KERNEL);
>> if (!prop->value) {
>> dlpar_free_cc_property(prop);
>> @@ -78,7 +78,7 @@ static struct device_node *dlpar_parse_cc_node(struct cc_workarea *ccwa,
>> if (!dn)
>> return NULL;
>>
>> - name = (char *)ccwa + ccwa->name_offset;
>> + name = (char *)ccwa + be32_to_cpu(ccwa->name_offset);
>> dn->full_name = kasprintf(GFP_KERNEL, "%s/%s", path, name);
>> if (!dn->full_name) {
>> kfree(dn);
>> @@ -148,7 +148,7 @@ struct device_node *dlpar_configure_connector(u32 drc_index,
>> return NULL;
>>
>> ccwa = (struct cc_workarea *)&data_buf[0];
>> - ccwa->drc_index = drc_index;
>> + ccwa->drc_index = cpu_to_be32(drc_index);
> This will break partition migration.
>
> The drc index valued passed into dlpar_configure_connector() from the migration
> path, pseries_devicetree_update(), is already in BE format.
>
> -Nathan
>
>> ccwa->zero = 0;
>>
>> do {
>> @@ -363,10 +363,10 @@ static int dlpar_online_cpu(struct device_node *dn)
>> int rc = 0;
>> unsigned int cpu;
>> int len, nthreads, i;
>> - const u32 *intserv;
>> + const __be32 *intserv_be;
>>
>> - intserv = of_get_property(dn, "ibm,ppc-interrupt-server#s", &len);
>> - if (!intserv)
>> + intserv_be = of_get_property(dn, "ibm,ppc-interrupt-server#s", &len);
>> + if (!intserv_be)
>> return -EINVAL;
>>
>> nthreads = len / sizeof(u32);
>> @@ -374,7 +374,7 @@ static int dlpar_online_cpu(struct device_node *dn)
>> cpu_maps_update_begin();
>> for (i = 0; i < nthreads; i++) {
>> for_each_present_cpu(cpu) {
>> - if (get_hard_smp_processor_id(cpu) != intserv[i])
>> + if (get_hard_smp_processor_id(cpu) != be32_to_cpu(intserv_be[i]))
>> continue;
>> BUG_ON(get_cpu_current_state(cpu)
>> != CPU_STATE_OFFLINE);
>> @@ -388,7 +388,7 @@ static int dlpar_online_cpu(struct device_node *dn)
>> }
>> if (cpu == num_possible_cpus())
>> printk(KERN_WARNING "Could not find cpu to online "
>> - "with physical id 0x%x\n", intserv[i]);
>> + "with physical id 0x%x\n", be32_to_cpu(intserv_be[i]));
>> }
>> cpu_maps_update_done();
>>
>> @@ -442,18 +442,17 @@ static int dlpar_offline_cpu(struct device_node *dn)
>> int rc = 0;
>> unsigned int cpu;
>> int len, nthreads, i;
>> - const u32 *intserv;
>> + const __be32 *intserv_be;
>>
>> - intserv = of_get_property(dn, "ibm,ppc-interrupt-server#s", &len);
>> - if (!intserv)
>> + intserv_be = of_get_property(dn, "ibm,ppc-interrupt-server#s", &len);
>> + if (!intserv_be)
>> return -EINVAL;
>>
>> nthreads = len / sizeof(u32);
>> -
>> cpu_maps_update_begin();
>> for (i = 0; i < nthreads; i++) {
>> for_each_present_cpu(cpu) {
>> - if (get_hard_smp_processor_id(cpu) != intserv[i])
>> + if (get_hard_smp_processor_id(cpu) != be32_to_cpu(intserv_be[i]))
>> continue;
>>
>> if (get_cpu_current_state(cpu) == CPU_STATE_OFFLINE)
>> @@ -469,20 +468,19 @@ static int dlpar_offline_cpu(struct device_node *dn)
>> break;
>>
>> }
>> -
>> /*
>> * The cpu is in CPU_STATE_INACTIVE.
>> * Upgrade it's state to CPU_STATE_OFFLINE.
>> */
>> set_preferred_offline_state(cpu, CPU_STATE_OFFLINE);
>> - BUG_ON(plpar_hcall_norets(H_PROD, intserv[i])
>> + BUG_ON(plpar_hcall_norets(H_PROD, be32_to_cpu(intserv_be[i]))
>> != H_SUCCESS);
>> __cpu_die(cpu);
>> break;
>> }
>> if (cpu == num_possible_cpus())
>> printk(KERN_WARNING "Could not find cpu to offline "
>> - "with physical id 0x%x\n", intserv[i]);
>> + "with physical id 0x%x\n", be32_to_cpu(intserv_be[i]));
>> }
>> cpu_maps_update_done();
>>
>> @@ -494,26 +492,28 @@ out:
>> static ssize_t dlpar_cpu_release(const char *buf, size_t count)
>> {
>> struct device_node *dn;
>> - const u32 *drc_index;
>> + const __be32 *drc_index_be;
>> + u32 drc_index;
>> int rc;
>>
>> dn = of_find_node_by_path(buf);
>> if (!dn)
>> return -EINVAL;
>>
>> - drc_index = of_get_property(dn, "ibm,my-drc-index", NULL);
>> - if (!drc_index) {
>> + drc_index_be = of_get_property(dn, "ibm,my-drc-index", NULL);
>> + if (!drc_index_be) {
>> of_node_put(dn);
>> return -EINVAL;
>> }
>>
>> + drc_index = be32_to_cpup(drc_index_be);
>> rc = dlpar_offline_cpu(dn);
>> if (rc) {
>> of_node_put(dn);
>> return -EINVAL;
>> }
>>
>> - rc = dlpar_release_drc(*drc_index);
>> + rc = dlpar_release_drc(drc_index);
>> if (rc) {
>> of_node_put(dn);
>> return rc;
>> @@ -521,7 +521,7 @@ static ssize_t dlpar_cpu_release(const char *buf, size_t count)
>>
>> rc = dlpar_detach_node(dn);
>> if (rc) {
>> - dlpar_acquire_drc(*drc_index);
>> + dlpar_acquire_drc(drc_index);
>> return rc;
>> }
>>
>> diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
>> index 20d6297..fc9c2cd 100644
>> --- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
>> +++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
>> @@ -90,7 +90,7 @@ static void rtas_stop_self(void)
>> {
>> static struct rtas_args args = {
>> .nargs = 0,
>> - .nret = 1,
>> + .nret = cpu_to_be32(1),
>> .rets = &args.args[0],
>> };
>>
>> @@ -247,10 +247,10 @@ static int pseries_add_processor(struct device_node *np)
>> unsigned int cpu;
>> cpumask_var_t candidate_mask, tmp;
>> int err = -ENOSPC, len, nthreads, i;
>> - const u32 *intserv;
>> + const __be32 *intserv_be;
>>
>> - intserv = of_get_property(np, "ibm,ppc-interrupt-server#s", &len);
>> - if (!intserv)
>> + intserv_be = of_get_property(np, "ibm,ppc-interrupt-server#s", &len);
>> + if (!intserv_be)
>> return 0;
>>
>> zalloc_cpumask_var(&candidate_mask, GFP_KERNEL);
>> @@ -293,7 +293,7 @@ static int pseries_add_processor(struct device_node *np)
>> for_each_cpu(cpu, tmp) {
>> BUG_ON(cpu_present(cpu));
>> set_cpu_present(cpu, true);
>> - set_hard_smp_processor_id(cpu, *intserv++);
>> + set_hard_smp_processor_id(cpu, be32_to_cpu(*intserv_be++));
>> }
>> err = 0;
>> out_unlock:
>> @@ -312,10 +312,10 @@ static void pseries_remove_processor(struct device_node *np)
>> {
>> unsigned int cpu;
>> int len, nthreads, i;
>> - const u32 *intserv;
>> + const __be32 *intserv_be;
>>
>> - intserv = of_get_property(np, "ibm,ppc-interrupt-server#s", &len);
>> - if (!intserv)
>> + intserv_be = of_get_property(np, "ibm,ppc-interrupt-server#s", &len);
>> + if (!intserv_be)
>> return;
>>
>> nthreads = len / sizeof(u32);
>> @@ -323,7 +323,7 @@ static void pseries_remove_processor(struct device_node *np)
>> cpu_maps_update_begin();
>> for (i = 0; i < nthreads; i++) {
>> for_each_present_cpu(cpu) {
>> - if (get_hard_smp_processor_id(cpu) != intserv[i])
>> + if (get_hard_smp_processor_id(cpu) != be32_to_cpu(intserv_be[i]))
>> continue;
>> BUG_ON(cpu_online(cpu));
>> set_cpu_present(cpu, false);
>> @@ -332,7 +332,7 @@ static void pseries_remove_processor(struct device_node *np)
>> }
>> if (cpu >= nr_cpu_ids)
>> printk(KERN_WARNING "Could not find cpu to remove "
>> - "with physical id 0x%x\n", intserv[i]);
>> + "with physical id 0x%x\n", be32_to_cpu(intserv_be[i]));
>> }
>> cpu_maps_update_done();
>> }
>>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
--
[-- Attachment #2: Type: text/html, Size: 9844 bytes --]
^ permalink raw reply
* Re: [PATCH] of: make sure of_alias is initialized before accessing it
From: Laurentiu Tudor @ 2014-09-08 14:13 UTC (permalink / raw)
To: Grant Likely, devicetree, linux-kernel, linuxppc-dev; +Cc: Laurentiu Tudor
In-Reply-To: <20140908132956.646FEC40AE5@trevor.secretlab.ca>
On 09/08/2014 04:29 PM, Grant Likely wrote:
> On Wed, 27 Aug 2014 17:09:39 +0300, Laurentiu Tudor <b10716@freescale.com> wrote:
>> Simply swap of_alias and of_chosen initialization so
>> that of_alias ends up read first. This must be done
>> because it is accessed couple of lines below when
>> trying to initialize the of_stdout using the alias
>> based legacy method.
>>
>> [Fixes a752ee5 - tty: Update hypervisor tty drivers to
>> use core stdout parsing code]
>>
>> Signed-off-by: Laurentiu Tudor <Laurentiu.Tudor@freescale.com>
>> Cc: Grant Likely <grant.likely@linaro.org>
>> ---
>> drivers/of/base.c | 8 ++++----
>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>> index d8574ad..52f8506 100644
>> --- a/drivers/of/base.c
>> +++ b/drivers/of/base.c
>> @@ -1847,6 +1847,10 @@ void of_alias_scan(void * (*dt_alloc)(u64 size, u64 align))
>> {
>> struct property *pp;
>>
>> + of_aliases = of_find_node_by_path("/aliases");
>> + if (!of_aliases)
>> + return;
>> +
>> of_chosen = of_find_node_by_path("/chosen");
>> if (of_chosen == NULL)
>> of_chosen = of_find_node_by_path("/chosen@0");
>> @@ -1862,10 +1866,6 @@ void of_alias_scan(void * (*dt_alloc)(u64 size, u64 align))
>> of_stdout = of_find_node_by_path(name);
>> }
>>
>> - of_aliases = of_find_node_by_path("/aliases");
>> - if (!of_aliases)
>> - return;
>> -
>
> Close, but not quite. The 'if (!of_aliases)' test should not be moved.
> Only the search for of_find_node_by_path().
Eek, completely missed this. Sorry.
> I've fixed it up and applied.
Thanks!
---
Best Regards, Laurentiu
^ permalink raw reply
* Re: [PATCH] pseries: Fix endianness in cpu hotplug and hotremove
From: Nathan Fontenot @ 2014-09-08 14:07 UTC (permalink / raw)
To: Thomas Falcon, linuxppc-dev
In-Reply-To: <1409944146-6813-1-git-send-email-tlfalcon@linux.vnet.ibm.com>
It looks like you have a lot of the same changes as the patch Bharata
sent out last week. Including the one issue I saw in Bharata's patch
below.
On 09/05/2014 02:09 PM, Thomas Falcon wrote:
> This patch attempts to ensure that all values are in the proper
> endianness format when both hotadding and hotremoving cpus.
>
> Signed-off-by: Thomas Falcon <tlfalcon@linux.vnet.ibm.com>
> ---
> arch/powerpc/platforms/pseries/dlpar.c | 56 ++++++++++++++--------------
> arch/powerpc/platforms/pseries/hotplug-cpu.c | 20 +++++-----
> 2 files changed, 38 insertions(+), 38 deletions(-)
>
> diff --git a/arch/powerpc/platforms/pseries/dlpar.c b/arch/powerpc/platforms/pseries/dlpar.c
> index a2450b8..c1d7e40 100644
> --- a/arch/powerpc/platforms/pseries/dlpar.c
> +++ b/arch/powerpc/platforms/pseries/dlpar.c
> @@ -24,11 +24,11 @@
> #include <asm/rtas.h>
>
> struct cc_workarea {
> - u32 drc_index;
> - u32 zero;
> - u32 name_offset;
> - u32 prop_length;
> - u32 prop_offset;
> + __be32 drc_index;
> + __be32 zero;
> + __be32 name_offset;
> + __be32 prop_length;
> + __be32 prop_offset;
> };
>
> void dlpar_free_cc_property(struct property *prop)
> @@ -48,11 +48,11 @@ static struct property *dlpar_parse_cc_property(struct cc_workarea *ccwa)
> if (!prop)
> return NULL;
>
> - name = (char *)ccwa + ccwa->name_offset;
> + name = (char *)ccwa + be32_to_cpu(ccwa->name_offset);
> prop->name = kstrdup(name, GFP_KERNEL);
>
> - prop->length = ccwa->prop_length;
> - value = (char *)ccwa + ccwa->prop_offset;
> + prop->length = be32_to_cpu(ccwa->prop_length);
> + value = (char *)ccwa + be32_to_cpu(ccwa->prop_offset);
> prop->value = kmemdup(value, prop->length, GFP_KERNEL);
> if (!prop->value) {
> dlpar_free_cc_property(prop);
> @@ -78,7 +78,7 @@ static struct device_node *dlpar_parse_cc_node(struct cc_workarea *ccwa,
> if (!dn)
> return NULL;
>
> - name = (char *)ccwa + ccwa->name_offset;
> + name = (char *)ccwa + be32_to_cpu(ccwa->name_offset);
> dn->full_name = kasprintf(GFP_KERNEL, "%s/%s", path, name);
> if (!dn->full_name) {
> kfree(dn);
> @@ -148,7 +148,7 @@ struct device_node *dlpar_configure_connector(u32 drc_index,
> return NULL;
>
> ccwa = (struct cc_workarea *)&data_buf[0];
> - ccwa->drc_index = drc_index;
> + ccwa->drc_index = cpu_to_be32(drc_index);
This will break partition migration.
The drc index valued passed into dlpar_configure_connector() from the migration
path, pseries_devicetree_update(), is already in BE format.
-Nathan
> ccwa->zero = 0;
>
> do {
> @@ -363,10 +363,10 @@ static int dlpar_online_cpu(struct device_node *dn)
> int rc = 0;
> unsigned int cpu;
> int len, nthreads, i;
> - const u32 *intserv;
> + const __be32 *intserv_be;
>
> - intserv = of_get_property(dn, "ibm,ppc-interrupt-server#s", &len);
> - if (!intserv)
> + intserv_be = of_get_property(dn, "ibm,ppc-interrupt-server#s", &len);
> + if (!intserv_be)
> return -EINVAL;
>
> nthreads = len / sizeof(u32);
> @@ -374,7 +374,7 @@ static int dlpar_online_cpu(struct device_node *dn)
> cpu_maps_update_begin();
> for (i = 0; i < nthreads; i++) {
> for_each_present_cpu(cpu) {
> - if (get_hard_smp_processor_id(cpu) != intserv[i])
> + if (get_hard_smp_processor_id(cpu) != be32_to_cpu(intserv_be[i]))
> continue;
> BUG_ON(get_cpu_current_state(cpu)
> != CPU_STATE_OFFLINE);
> @@ -388,7 +388,7 @@ static int dlpar_online_cpu(struct device_node *dn)
> }
> if (cpu == num_possible_cpus())
> printk(KERN_WARNING "Could not find cpu to online "
> - "with physical id 0x%x\n", intserv[i]);
> + "with physical id 0x%x\n", be32_to_cpu(intserv_be[i]));
> }
> cpu_maps_update_done();
>
> @@ -442,18 +442,17 @@ static int dlpar_offline_cpu(struct device_node *dn)
> int rc = 0;
> unsigned int cpu;
> int len, nthreads, i;
> - const u32 *intserv;
> + const __be32 *intserv_be;
>
> - intserv = of_get_property(dn, "ibm,ppc-interrupt-server#s", &len);
> - if (!intserv)
> + intserv_be = of_get_property(dn, "ibm,ppc-interrupt-server#s", &len);
> + if (!intserv_be)
> return -EINVAL;
>
> nthreads = len / sizeof(u32);
> -
> cpu_maps_update_begin();
> for (i = 0; i < nthreads; i++) {
> for_each_present_cpu(cpu) {
> - if (get_hard_smp_processor_id(cpu) != intserv[i])
> + if (get_hard_smp_processor_id(cpu) != be32_to_cpu(intserv_be[i]))
> continue;
>
> if (get_cpu_current_state(cpu) == CPU_STATE_OFFLINE)
> @@ -469,20 +468,19 @@ static int dlpar_offline_cpu(struct device_node *dn)
> break;
>
> }
> -
> /*
> * The cpu is in CPU_STATE_INACTIVE.
> * Upgrade it's state to CPU_STATE_OFFLINE.
> */
> set_preferred_offline_state(cpu, CPU_STATE_OFFLINE);
> - BUG_ON(plpar_hcall_norets(H_PROD, intserv[i])
> + BUG_ON(plpar_hcall_norets(H_PROD, be32_to_cpu(intserv_be[i]))
> != H_SUCCESS);
> __cpu_die(cpu);
> break;
> }
> if (cpu == num_possible_cpus())
> printk(KERN_WARNING "Could not find cpu to offline "
> - "with physical id 0x%x\n", intserv[i]);
> + "with physical id 0x%x\n", be32_to_cpu(intserv_be[i]));
> }
> cpu_maps_update_done();
>
> @@ -494,26 +492,28 @@ out:
> static ssize_t dlpar_cpu_release(const char *buf, size_t count)
> {
> struct device_node *dn;
> - const u32 *drc_index;
> + const __be32 *drc_index_be;
> + u32 drc_index;
> int rc;
>
> dn = of_find_node_by_path(buf);
> if (!dn)
> return -EINVAL;
>
> - drc_index = of_get_property(dn, "ibm,my-drc-index", NULL);
> - if (!drc_index) {
> + drc_index_be = of_get_property(dn, "ibm,my-drc-index", NULL);
> + if (!drc_index_be) {
> of_node_put(dn);
> return -EINVAL;
> }
>
> + drc_index = be32_to_cpup(drc_index_be);
> rc = dlpar_offline_cpu(dn);
> if (rc) {
> of_node_put(dn);
> return -EINVAL;
> }
>
> - rc = dlpar_release_drc(*drc_index);
> + rc = dlpar_release_drc(drc_index);
> if (rc) {
> of_node_put(dn);
> return rc;
> @@ -521,7 +521,7 @@ static ssize_t dlpar_cpu_release(const char *buf, size_t count)
>
> rc = dlpar_detach_node(dn);
> if (rc) {
> - dlpar_acquire_drc(*drc_index);
> + dlpar_acquire_drc(drc_index);
> return rc;
> }
>
> diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> index 20d6297..fc9c2cd 100644
> --- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
> +++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> @@ -90,7 +90,7 @@ static void rtas_stop_self(void)
> {
> static struct rtas_args args = {
> .nargs = 0,
> - .nret = 1,
> + .nret = cpu_to_be32(1),
> .rets = &args.args[0],
> };
>
> @@ -247,10 +247,10 @@ static int pseries_add_processor(struct device_node *np)
> unsigned int cpu;
> cpumask_var_t candidate_mask, tmp;
> int err = -ENOSPC, len, nthreads, i;
> - const u32 *intserv;
> + const __be32 *intserv_be;
>
> - intserv = of_get_property(np, "ibm,ppc-interrupt-server#s", &len);
> - if (!intserv)
> + intserv_be = of_get_property(np, "ibm,ppc-interrupt-server#s", &len);
> + if (!intserv_be)
> return 0;
>
> zalloc_cpumask_var(&candidate_mask, GFP_KERNEL);
> @@ -293,7 +293,7 @@ static int pseries_add_processor(struct device_node *np)
> for_each_cpu(cpu, tmp) {
> BUG_ON(cpu_present(cpu));
> set_cpu_present(cpu, true);
> - set_hard_smp_processor_id(cpu, *intserv++);
> + set_hard_smp_processor_id(cpu, be32_to_cpu(*intserv_be++));
> }
> err = 0;
> out_unlock:
> @@ -312,10 +312,10 @@ static void pseries_remove_processor(struct device_node *np)
> {
> unsigned int cpu;
> int len, nthreads, i;
> - const u32 *intserv;
> + const __be32 *intserv_be;
>
> - intserv = of_get_property(np, "ibm,ppc-interrupt-server#s", &len);
> - if (!intserv)
> + intserv_be = of_get_property(np, "ibm,ppc-interrupt-server#s", &len);
> + if (!intserv_be)
> return;
>
> nthreads = len / sizeof(u32);
> @@ -323,7 +323,7 @@ static void pseries_remove_processor(struct device_node *np)
> cpu_maps_update_begin();
> for (i = 0; i < nthreads; i++) {
> for_each_present_cpu(cpu) {
> - if (get_hard_smp_processor_id(cpu) != intserv[i])
> + if (get_hard_smp_processor_id(cpu) != be32_to_cpu(intserv_be[i]))
> continue;
> BUG_ON(cpu_online(cpu));
> set_cpu_present(cpu, false);
> @@ -332,7 +332,7 @@ static void pseries_remove_processor(struct device_node *np)
> }
> if (cpu >= nr_cpu_ids)
> printk(KERN_WARNING "Could not find cpu to remove "
> - "with physical id 0x%x\n", intserv[i]);
> + "with physical id 0x%x\n", be32_to_cpu(intserv_be[i]));
> }
> cpu_maps_update_done();
> }
>
^ permalink raw reply
* Re: [PATCH] of: make sure of_alias is initialized before accessing it
From: Grant Likely @ 2014-09-08 13:29 UTC (permalink / raw)
To: Laurentiu Tudor, devicetree, linux-kernel, linuxppc-dev; +Cc: Laurentiu Tudor
In-Reply-To: <53FDE6A3.3080303@freescale.com>
On Wed, 27 Aug 2014 17:09:39 +0300, Laurentiu Tudor <b10716@freescale.com> wrote:
> Simply swap of_alias and of_chosen initialization so
> that of_alias ends up read first. This must be done
> because it is accessed couple of lines below when
> trying to initialize the of_stdout using the alias
> based legacy method.
>
> [Fixes a752ee5 - tty: Update hypervisor tty drivers to
> use core stdout parsing code]
>
> Signed-off-by: Laurentiu Tudor <Laurentiu.Tudor@freescale.com>
> Cc: Grant Likely <grant.likely@linaro.org>
> ---
> drivers/of/base.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index d8574ad..52f8506 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -1847,6 +1847,10 @@ void of_alias_scan(void * (*dt_alloc)(u64 size, u64 align))
> {
> struct property *pp;
>
> + of_aliases = of_find_node_by_path("/aliases");
> + if (!of_aliases)
> + return;
> +
> of_chosen = of_find_node_by_path("/chosen");
> if (of_chosen == NULL)
> of_chosen = of_find_node_by_path("/chosen@0");
> @@ -1862,10 +1866,6 @@ void of_alias_scan(void * (*dt_alloc)(u64 size, u64 align))
> of_stdout = of_find_node_by_path(name);
> }
>
> - of_aliases = of_find_node_by_path("/aliases");
> - if (!of_aliases)
> - return;
> -
Close, but not quite. The 'if (!of_aliases)' test should not be moved.
Only the search for of_find_node_by_path(). I've fixed it up and
applied.
g.
> for_each_property_of_node(of_aliases, pp) {
> const char *start = pp->name;
> const char *end = start + strlen(start);
> --
> 1.8.3.1
^ permalink raw reply
* Re: [PATCH 2/2 v5] powerpc/kvm: common sw breakpoint instr across ppc
From: Alexander Graf @ 2014-09-08 13:09 UTC (permalink / raw)
To: Madhavan Srinivasan, benh, paulus, mpe; +Cc: linuxppc-dev, kvm-ppc, kvm
In-Reply-To: <1410107494-25556-3-git-send-email-maddy@linux.vnet.ibm.com>
On 07.09.14 18:31, Madhavan Srinivasan wrote:
> This patch extends the use of illegal instruction as software
> breakpoint instruction across the ppc platform. Patch extends
> booke program interrupt code to support software breakpoint.
>
> Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
> ---
>
> Patch is only compile tested. Will really help if
> someone can try it out and let me know comments.
>
> arch/powerpc/kvm/booke.c | 18 ++++++++++++++++--
> 1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> index b4c89fa..1b84853 100644
> --- a/arch/powerpc/kvm/booke.c
> +++ b/arch/powerpc/kvm/booke.c
> @@ -870,6 +870,10 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
> case BOOKE_INTERRUPT_HV_PRIV:
> emulated = kvmppc_get_last_inst(vcpu, false, &last_inst);
> break;
> + case BOOKE_INTERRUPT_PROGRAM:
> + /*SW breakpoints arrive as illegal instructions on HV */
Is it my email client or is there a space missing again? ;)
Also, please only fetch the last instruction if debugging is active.
> + emulated = kvmppc_get_last_inst(vcpu, false, &last_inst);
> + break;
> default:
> break;
> }
> @@ -947,7 +951,17 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
> break;
>
> case BOOKE_INTERRUPT_PROGRAM:
> - if (vcpu->arch.shared->msr & (MSR_PR | MSR_GS)) {
> + if ((vcpu->arch.shared->msr & (MSR_PR | MSR_GS)) &&
> + (last_inst == KVMPPC_INST_SW_BREAKPOINT)) {
I think this is changing the logic from "if the guest is in user mode or
we're in HV, deflect" to "if the guest is in user mode or an HV guest
and the instruction is a breakpoint, treat it as debug. Otherwise
deflect". So you're essentially breaking PR KVM here from what I can tell.
Why don't you just split the whole thing out to the beginning of
BOOKE_INTERRUPT_PROGRAM and check for
a) debug is enabled
b) instruction is sw breakpoint
instead?
> + /*
> + * We are here because of an SW breakpoint instr,
> + * so lets return to host to handle.
> + */
> + r = kvmppc_handle_debug(run, vcpu);
> + run->exit_reason = KVM_EXIT_DEBUG;
> + kvmppc_account_exit(vcpu, DEBUG_EXITS);
> + break;
> + } else {
> /*
> * Program traps generated by user-level software must
> * be handled by the guest kernel.
> @@ -1505,7 +1519,7 @@ int kvm_vcpu_ioctl_get_one_reg(struct kvm_vcpu *vcpu, struct kvm_one_reg *reg)
> val = get_reg_val(reg->id, vcpu->arch.tsr);
> break;
> case KVM_REG_PPC_DEBUG_INST:
> - val = get_reg_val(reg->id, KVMPPC_INST_EHPRIV_DEBUG);
Please also remove the definition of EHPRIV_DEBUG.
Alex
> + val = get_reg_val(reg->id, KVMPPC_INST_SW_BREAKPOINT);
> break;
> case KVM_REG_PPC_VRSAVE:
> val = get_reg_val(reg->id, vcpu->arch.vrsave);
>
^ permalink raw reply
* Re: [PATCH 1/2 v5] powerpc/kvm: support to handle sw breakpoint
From: Alexander Graf @ 2014-09-08 13:05 UTC (permalink / raw)
To: Madhavan Srinivasan, benh, paulus, mpe; +Cc: linuxppc-dev, kvm-ppc, kvm
In-Reply-To: <1410107494-25556-2-git-send-email-maddy@linux.vnet.ibm.com>
On 07.09.14 18:31, Madhavan Srinivasan wrote:
> This patch adds kernel side support for software breakpoint.
> Design is that, by using an illegal instruction, we trap to hypervisor
> via Emulation Assistance interrupt, where we check for the illegal instruction
> and accordingly we return to Host or Guest. Patch also adds support for
> software breakpoint in PR KVM.
>
> Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
> ---
> arch/powerpc/include/asm/kvm_ppc.h | 6 ++++++
> arch/powerpc/kvm/book3s.c | 3 ++-
> arch/powerpc/kvm/book3s_hv.c | 41 ++++++++++++++++++++++++++++++++++----
> arch/powerpc/kvm/book3s_pr.c | 3 +++
> arch/powerpc/kvm/emulate.c | 18 +++++++++++++++++
> 5 files changed, 66 insertions(+), 5 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h
> index fb86a22..dd83c9a 100644
> --- a/arch/powerpc/include/asm/kvm_ppc.h
> +++ b/arch/powerpc/include/asm/kvm_ppc.h
> @@ -38,6 +38,12 @@
> #include <asm/paca.h>
> #endif
>
> +/*
> + * KVMPPC_INST_SW_BREAKPOINT is debug Instruction
> + * for supporting software breakpoint.
> + */
> +#define KVMPPC_INST_SW_BREAKPOINT 0x00dddd00
> +
> enum emulation_result {
> EMULATE_DONE, /* no further processing */
> EMULATE_DO_MMIO, /* kvm_run filled with MMIO request */
> diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c
> index dd03f6b..00e9c9f 100644
> --- a/arch/powerpc/kvm/book3s.c
> +++ b/arch/powerpc/kvm/book3s.c
> @@ -778,7 +778,8 @@ int kvm_arch_vcpu_ioctl_translate(struct kvm_vcpu *vcpu,
> int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
> struct kvm_guest_debug *dbg)
> {
> - return -EINVAL;
> + vcpu->guest_debug = dbg->control;
> + return 0;
> }
>
> void kvmppc_decrementer_func(unsigned long data)
> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index 27cced9..3a2414c 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -725,6 +725,30 @@ static int kvmppc_hcall_impl_hv(unsigned long cmd)
> return kvmppc_hcall_impl_hv_realmode(cmd);
> }
>
> +static int kvmppc_emulate_debug_inst(struct kvm_run *run,
> + struct kvm_vcpu *vcpu)
> +{
> + u32 last_inst;
> +
> + if(kvmppc_get_last_inst(vcpu, INST_GENERIC, &last_inst) !=
> + EMULATE_DONE) {
> + /*
> + * Fetch failed, so return to guest and
> + * try executing it again.
> + */
> + return RESUME_GUEST;
Please end the if() here. In the kernel we usually treat abort
situations as separate.
> + } else {
> + if (last_inst == KVMPPC_INST_SW_BREAKPOINT) {
> + run->exit_reason = KVM_EXIT_DEBUG;
> + run->debug.arch.address = kvmppc_get_pc(vcpu);
> + return RESUME_HOST;
> + } else {
> + kvmppc_core_queue_program(vcpu, SRR1_PROGILL);
> + return RESUME_GUEST;
> + }
> + }
> +}
> +
> static int kvmppc_handle_exit_hv(struct kvm_run *run, struct kvm_vcpu *vcpu,
> struct task_struct *tsk)
> {
> @@ -807,12 +831,18 @@ static int kvmppc_handle_exit_hv(struct kvm_run *run, struct kvm_vcpu *vcpu,
> break;
> /*
> * This occurs if the guest executes an illegal instruction.
> - * We just generate a program interrupt to the guest, since
> - * we don't emulate any guest instructions at this stage.
> + * If the guest debug is disabled, generate a program interrupt
> + * to the guest. If guest debug is enabled, we need to check
> + * whether the instruction is a software breakpoint instruction.
> + * Accordingly return to Guest or Host.
> */
> case BOOK3S_INTERRUPT_H_EMUL_ASSIST:
> - kvmppc_core_queue_program(vcpu, SRR1_PROGILL);
> - r = RESUME_GUEST;
> + if(vcpu->guest_debug & KVM_GUESTDBG_USE_SW_BP) {
> + r = kvmppc_emulate_debug_inst(run, vcpu);
> + } else {
> + kvmppc_core_queue_program(vcpu, SRR1_PROGILL);
> + r = RESUME_GUEST;
> + }
> break;
> /*
> * This occurs if the guest (kernel or userspace), does something that
> @@ -922,6 +952,9 @@ static int kvmppc_get_one_reg_hv(struct kvm_vcpu *vcpu, u64 id,
> long int i;
>
> switch (id) {
> + case KVM_REG_PPC_DEBUG_INST:
> + *val = get_reg_val(id, KVMPPC_INST_SW_BREAKPOINT);
> + break;
> case KVM_REG_PPC_HIOR:
> *val = get_reg_val(id, 0);
> break;
> diff --git a/arch/powerpc/kvm/book3s_pr.c b/arch/powerpc/kvm/book3s_pr.c
> index faffb27..6d73708 100644
> --- a/arch/powerpc/kvm/book3s_pr.c
> +++ b/arch/powerpc/kvm/book3s_pr.c
> @@ -1319,6 +1319,9 @@ static int kvmppc_get_one_reg_pr(struct kvm_vcpu *vcpu, u64 id,
> int r = 0;
>
> switch (id) {
> + case KVM_REG_PPC_DEBUG_INST:
> + *val = get_reg_val(id, KVMPPC_INST_SW_BREAKPOINT);
> + break;
> case KVM_REG_PPC_HIOR:
> *val = get_reg_val(id, to_book3s(vcpu)->hior);
> break;
> diff --git a/arch/powerpc/kvm/emulate.c b/arch/powerpc/kvm/emulate.c
> index e96b50d..30f326b 100644
> --- a/arch/powerpc/kvm/emulate.c
> +++ b/arch/powerpc/kvm/emulate.c
> @@ -274,6 +274,24 @@ int kvmppc_emulate_instruction(struct kvm_run *run, struct kvm_vcpu *vcpu)
> }
> break;
>
> + case 0:
> + /*
> + * Instruction with primary opcode 0. Based on PowerISA
> + * these are illegal instructions.
> + */
> + if(inst == KVMPPC_INST_SW_BREAKPOINT) {
Here you're missing a space between if and (. Are you sure that you ran
checkpatch.pl?
> + run->exit_reason = KVM_EXIT_DEBUG;
> + run->debug.arch.address = kvmppc_get_pc(vcpu);
> + emulated = EMULATE_EXIT_USER;
> + advance = 0;
> + printk(KERN_INFO "pr_emulate: debug instr \n");
> + } else {
> + kvmppc_core_queue_program(vcpu, SRR1_PROGILL);
> + emulated = EMULATE_DONE;
> + advance = 0;
Just set emulated = EMULATE_FAIL here, the same as default:.
Alex
> + }
> + break;
> +
> default:
> emulated = EMULATE_FAIL;
> }
>
^ 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