* Re: [PATCH] powerpc/mm: Fix thread_pkey_regs_init()
From: Balbir Singh @ 2018-03-27 3:53 UTC (permalink / raw)
To: Ram Pai
Cc: Michael Ellerman, open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
Benjamin Herrenschmidt, Paul Mackerras, Aneesh Kumar KV,
Haren Myneni/Beaverton/IBM, Michal Hocko, Thiago Jung Bauermann,
fweimer, msuchanek
In-Reply-To: <1522118214-7040-1-git-send-email-linuxram@us.ibm.com>
On Tue, Mar 27, 2018 at 1:36 PM, Ram Pai <linuxram@us.ibm.com> wrote:
> thread_pkey_regs_init() initializes the pkey related registers
> instead of initializing the fields in the task structures. Fortunately
> those key related registers are re-set to zero when the task
> gets scheduled on the cpu. However its good to fix this glaringly
> visible error.
>
> cc: Michael Ellermen <mpe@ellerman.id.au>
> Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
> ---
Acked-by: Balbir Singh <bsingharora@gmail.com>
Balbir Singh
^ permalink raw reply
* Re: [PATCH v9 21/24] perf tools: Add support for the SPF perf event
From: Andi Kleen @ 2018-03-27 3:49 UTC (permalink / raw)
To: David Rientjes
Cc: Laurent Dufour, paulmck, peterz, akpm, kirill, mhocko, dave, jack,
Matthew Wilcox, benh, mpe, paulus, Thomas Gleixner, Ingo Molnar,
hpa, Will Deacon, Sergey Senozhatsky, Andrea Arcangeli,
Alexei Starovoitov, kemi.wang, sergey.senozhatsky.work,
Daniel Jordan, linux-kernel, linux-mm, haren, khandual, npiggin,
bsingharora, Tim Chen, linuxppc-dev, x86
In-Reply-To: <alpine.DEB.2.20.1803261443560.255554@chino.kir.corp.google.com>
On Mon, Mar 26, 2018 at 02:44:48PM -0700, David Rientjes wrote:
> On Tue, 13 Mar 2018, Laurent Dufour wrote:
>
> > Add support for the new speculative faults event.
> >
> > Signed-off-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
>
> Acked-by: David Rientjes <rientjes@google.com>
>
> Aside: should there be a new spec_flt field for struct task_struct that
> complements maj_flt and min_flt and be exported through /proc/pid/stat?
No. task_struct is already too bloated. If you need per process tracking
you can always get it through trace points.
-Andi
^ permalink raw reply
* Re: [PATCH v4] mm, pkey: treat pkey-0 special
From: Michael Ellerman @ 2018-03-27 3:48 UTC (permalink / raw)
To: Ram Pai, Balbir Singh
Cc: Ingo Molnar, akpm@linux-foundation.org,
open list:LINUX FOR POWERPC (32-BIT AND 64-BIT), linux-mm,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT), linux-arch,
linux-kernel@vger.kernel.org, Dave Hansen, Benjamin Herrenschmidt,
Paul Mackerras, Anshuman Khandual, Aneesh Kumar KV,
Haren Myneni/Beaverton/IBM, Michal Hocko, Thiago Jung Bauermann,
Eric W. Biederman, Jonathan Corbet, Arnd Bergmann, fweimer,
msuchanek, Thomas Gleixner, Ulrich.Weigand, Ram Pai
In-Reply-To: <20180316193152.GG1060@ram.oc3035372033.ibm.com>
Ram Pai <linuxram@us.ibm.com> writes:
> On Fri, Mar 16, 2018 at 10:02:22PM +1100, Balbir Singh wrote:
>> On Fri, Mar 16, 2018 at 9:33 PM, Ram Pai <linuxram@us.ibm.com> wrote:
>> > Applications need the ability to associate an address-range with some
>> > key and latter revert to its initial default key. Pkey-0 comes close to
>> > providing this function but falls short, because the current
>> > implementation disallows applications to explicitly associate pkey-0 to
>> > the address range.
>> >
>> > Clarify the semantics of pkey-0 and provide the corresponding
>> > implementation.
>> >
>> > Pkey-0 is special with the following semantics.
>> > (a) it is implicitly allocated and can never be freed. It always exists.
>> > (b) it is the default key assigned to any address-range.
>> > (c) it can be explicitly associated with any address-range.
>> >
>> > Tested on powerpc only. Could not test on x86.
>>
>> Ram,
>>
>> I was wondering if we should check the AMOR values on the ppc side to make sure
>> that pkey0 is indeed available for use as default. I am still of the
>> opinion that we
>
> AMOR cannot be read/written by the OS in priviledge-non-hypervisor-mode.
> We could try testing if key-0 is available to the OS by temproarily
> changing the bits key-0 bits of AMR or IAMR register. But will be
> dangeorous to do, for you might disable read,execute of all the pages,
> since all pages are asscoiated with key-0 bydefault.
No we should do what firmware tells us. If it says key 0 is available we
use it, otherwise we don't.
Now if you notice the way the firmware API (device tree property) is
defined, it tells us how many keys are available, counting from 0.
So for pkey 0 to be reserved there must be 0 keys available.
End of story.
cheers
^ permalink raw reply
* Re: [RFC] new SYSCALL_DEFINE/COMPAT_SYSCALL_DEFINE wrappers
From: Linus Torvalds @ 2018-03-27 3:40 UTC (permalink / raw)
To: John Paul Adrian Glaubitz
Cc: Al Viro, Ingo Molnar, Dominik Brodowski,
Linux Kernel Mailing List, Arnd Bergmann, linux-arch,
Ralf Baechle, James Hogan, linux-mips, Benjamin Herrenschmidt,
Paul Mackerras, Michael Ellerman, ppc-dev, Martin Schwidefsky,
Heiko Carstens, linux-s390, David S . Miller, sparclinux,
Ingo Molnar, Jiri Slaby, the arch/x86 maintainers
In-Reply-To: <8a8ee344-fb19-3ed9-f7dc-db63f703e6d3@physik.fu-berlin.de>
On Mon, Mar 26, 2018 at 4:37 PM, John Paul Adrian Glaubitz
<glaubitz@physik.fu-berlin.de> wrote:
>
> What about a tarball with a minimal Debian x32 chroot? Then you can
> install interesting packages you would like to test yourself.
That probably works fine.
Linus
^ permalink raw reply
* Re: [RFC] new SYSCALL_DEFINE/COMPAT_SYSCALL_DEFINE wrappers
From: John Paul Adrian Glaubitz @ 2018-03-27 2:37 UTC (permalink / raw)
To: Linus Torvalds
Cc: Al Viro, Ingo Molnar, Dominik Brodowski,
Linux Kernel Mailing List, Arnd Bergmann, linux-arch,
Ralf Baechle, James Hogan, linux-mips, Benjamin Herrenschmidt,
Paul Mackerras, Michael Ellerman, ppc-dev, Martin Schwidefsky,
Heiko Carstens, linux-s390, David S . Miller, sparclinux,
Ingo Molnar, Jiri Slaby, the arch/x86 maintainers
In-Reply-To: <CA+55aFwp-T-WFN95j7u5nn2BExxviJCx1-RgD3Mnu1AN_GYD3w@mail.gmail.com>
On 03/27/2018 10:03 AM, Linus Torvalds wrote:
> Hmm. Do you have a few statically built binaries that could be tested
> without installing a whole distribution? Something real and meaningful
> enough that it actually exercised a few real system calls, but not
> something that needs to bring in 50 different shared libraries?
>
> Something in /sbin, perhaps, that is still runnable by a regular user
> and doesn't require some distro-specific /etc layout etc, so that it
> would work even if you don't run Debian at all? Maybe some shell
> binary or something?
What about a tarball with a minimal Debian x32 chroot? Then you can
install interesting packages you would like to test yourself.
Adrian
--
.''`. John Paul Adrian Glaubitz
: :' : Debian Developer - glaubitz@debian.org
`. `' Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
`- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
^ permalink raw reply
* [PATCH] powerpc/mm: Fix thread_pkey_regs_init()
From: Ram Pai @ 2018-03-27 2:36 UTC (permalink / raw)
To: mpe
Cc: linuxppc-dev, benh, paulus, aneesh.kumar, bsingharora, hbabu,
mhocko, bauerman, linuxram, fweimer, msuchanek
thread_pkey_regs_init() initializes the pkey related registers
instead of initializing the fields in the task structures. Fortunately
those key related registers are re-set to zero when the task
gets scheduled on the cpu. However its good to fix this glaringly
visible error.
cc: Michael Ellermen <mpe@ellerman.id.au>
Signed-off-by: Ram Pai <linuxram@us.ibm.com>
Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
---
arch/powerpc/mm/pkeys.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
index 58bbb55..61dba4e 100644
--- a/arch/powerpc/mm/pkeys.c
+++ b/arch/powerpc/mm/pkeys.c
@@ -338,9 +338,9 @@ void thread_pkey_regs_init(struct thread_struct *thread)
if (static_branch_likely(&pkey_disabled))
return;
- write_amr(read_amr() & pkey_amr_uamor_mask);
- write_iamr(read_iamr() & pkey_iamr_mask);
- write_uamor(read_uamor() & pkey_amr_uamor_mask);
+ thread->amr = read_amr() & pkey_amr_uamor_mask;
+ thread->iamr = read_iamr() & pkey_iamr_mask;
+ thread->uamor = read_uamor() & pkey_amr_uamor_mask;
}
static inline bool pkey_allows_readwrite(int pkey)
--
1.8.3.1
^ permalink raw reply related
* Re: RFC on writel and writel_relaxed
From: Jason Gunthorpe @ 2018-03-27 1:39 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Sinan Kaya, Arnd Bergmann, David Laight, Oliver,
open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
linux-rdma@vger.kernel.org, Alexander Duyck, Will Deacon,
Paul E. McKenney
In-Reply-To: <1522108780.7364.24.camel@kernel.crashing.org>
On Tue, Mar 27, 2018 at 10:59:40AM +1100, Benjamin Herrenschmidt wrote:
> On Mon, 2018-03-26 at 16:50 -0600, Jason Gunthorpe wrote:
> > On Tue, Mar 27, 2018 at 09:36:11AM +1100, Benjamin Herrenschmidt wrote:
> > > On Mon, 2018-03-26 at 16:27 -0600, Jason Gunthorpe wrote:
> > > > > Otherwise almost all drivers out there are broken which I very much
> > > > > doubt :-)
> > > >
> > > > But.. Sinan is right, you look anywhere in the driver tree and you
> > > > find stuff like this:
> > > >
> > > > drivers/net/ethernet/intel/i40e/i40e_txrx.c
> > > >
> > > > /* Force memory writes to complete before letting h/w
> > > > * know there are new descriptors to fetch.
> > > > */
> > > > wmb();
> > > >
> > > >
> > > > It is *systemic*
> > >
> > > Yes, because they all copied e1000e :-) If you look at the comment in
> > > there, it does say it's only for weakly ordered archs such as ia64, and
> > > even then, probably predates Linus strong statement on the matter.
> >
> > Hahah, sure I'll buy that..
> >
> > But still, if this really is the case, a *strong* statement in
> > barriers.txt to that effect (and not an example demanding the wmb()!)
> > would be very helpful for those of us that have to review driver code!
>
> I agree, and that Mellanox bug you pointed me to seems to indicate that
> this may not even be true on x86 anymore ...
However, with bugs like that it is hard to know what is going on.. It
could be a CPU bug instead.
> I think we might need to revisit this properly...
I would love to hear a definitive statement from Intel on what wmb();
writel(); does on x86..
Sinan's patches are backwards if writel is ordered, instead of
using writel_relaxed, they should be eliminating the wmb().
But there is no way patches like that could go ahead until
barriers.txt is updated..
Jason
^ permalink raw reply
* [PATCH] powerpc: do not allow userspace to modify execute-only pkey
From: Ram Pai @ 2018-03-27 1:16 UTC (permalink / raw)
To: mpe
Cc: linuxppc-dev, benh, paulus, aneesh.kumar, bsingharora, hbabu,
mhocko, bauerman, linuxram, fweimer, msuchanek
When mprotect(....,PROT_EXEC) is called, the kernel allocates a
execute-only pkey and associates the pkey with the given address space.
The permission of this key should not be modifiable from userspace.
However a bug in the current implementation lets the permissions on the
key modifiable from userspace.
Whenever a key is allocated through mm_pkey_alloc(), the kernel programs
the UAMOR register to allow userspace to change permissions on the key.
This is fine for the keys explicitly allocated through the
sys_pkey_alloc(). But for execute-only pkey, this should not be allowed.
Restructured the code to fix the bug.
Signed-off-by: Ram Pai <linuxram@us.ibm.com>
---
arch/powerpc/include/asm/pkeys.h | 24 ++++++------------------
arch/powerpc/mm/pkeys.c | 30 ++++++++++++++++++++++++++++--
2 files changed, 34 insertions(+), 20 deletions(-)
diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
index b598fa9..0d3c630 100644
--- a/arch/powerpc/include/asm/pkeys.h
+++ b/arch/powerpc/include/asm/pkeys.h
@@ -113,6 +113,8 @@ static inline bool mm_pkey_is_allocated(struct mm_struct *mm, int pkey)
extern void __arch_activate_pkey(int pkey);
extern void __arch_deactivate_pkey(int pkey);
+extern int __mm_pkey_alloc(struct mm_struct *mm);
+
/*
* Returns a positive, 5-bit key on success, or -1 on failure.
* Relies on the mmap_sem to protect against concurrency in mm_pkey_alloc() and
@@ -120,29 +122,14 @@ static inline bool mm_pkey_is_allocated(struct mm_struct *mm, int pkey)
*/
static inline int mm_pkey_alloc(struct mm_struct *mm)
{
- /*
- * Note: this is the one and only place we make sure that the pkey is
- * valid as far as the hardware is concerned. The rest of the kernel
- * trusts that only good, valid pkeys come out of here.
- */
- u32 all_pkeys_mask = (u32)(~(0x0));
int ret;
if (static_branch_likely(&pkey_disabled))
return -1;
+ ret = __mm_pkey_alloc(mm);
/*
- * Are we out of pkeys? We must handle this specially because ffz()
- * behavior is undefined if there are no zeros.
- */
- if (mm_pkey_allocation_map(mm) == all_pkeys_mask)
- return -1;
-
- ret = ffz((u32)mm_pkey_allocation_map(mm));
- __mm_pkey_allocated(mm, ret);
-
- /*
- * Enable the key in the hardware
+ * Enable userspace to modify the key permissions.
*/
if (ret > 0)
__arch_activate_pkey(ret);
@@ -158,7 +145,8 @@ static inline int mm_pkey_free(struct mm_struct *mm, int pkey)
return -EINVAL;
/*
- * Disable the key in the hardware
+ * Reset the key and disable userspace
+ * from modifying the key permissions.
*/
__arch_deactivate_pkey(pkey);
__mm_pkey_free(mm, pkey);
diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
index e7a9e34..58bbb55 100644
--- a/arch/powerpc/mm/pkeys.c
+++ b/arch/powerpc/mm/pkeys.c
@@ -228,7 +228,10 @@ static void pkey_status_change(int pkey, bool enable)
init_amr(pkey, 0x0);
init_iamr(pkey, 0x0);
- /* Enable/disable key */
+ /*
+ * Enable/disable userspace to/from modifying the permissions
+ * on the key
+ */
old_uamor = read_uamor();
if (enable)
old_uamor |= (0x3ul << pkeyshift(pkey));
@@ -247,6 +250,29 @@ void __arch_deactivate_pkey(int pkey)
pkey_status_change(pkey, false);
}
+int __mm_pkey_alloc(struct mm_struct *mm)
+{
+ /*
+ * Note: this is the one and only place we make sure that the pkey is
+ * valid as far as the hardware is concerned. The rest of the kernel
+ * trusts that only good, valid pkeys come out of here.
+ */
+ u32 all_pkeys_mask = (u32)(~(0x0));
+ int ret;
+
+ /*
+ * Are we out of pkeys? We must handle this specially because ffz()
+ * behavior is undefined if there are no zeros.
+ */
+ if (mm_pkey_allocation_map(mm) == all_pkeys_mask)
+ return -1;
+
+ ret = ffz((u32)mm_pkey_allocation_map(mm));
+ __mm_pkey_allocated(mm, ret);
+
+ return ret;
+}
+
/*
* Set the access rights in AMR IAMR and UAMOR registers for @pkey to that
* specified in @init_val.
@@ -336,7 +362,7 @@ int __execute_only_pkey(struct mm_struct *mm)
/* Do we need to assign a pkey for mm's execute-only maps? */
if (execute_only_pkey == -1) {
/* Go allocate one to use, which might fail */
- execute_only_pkey = mm_pkey_alloc(mm);
+ execute_only_pkey = __mm_pkey_alloc(mm);
if (execute_only_pkey < 0)
return -1;
need_to_set_mm_pkey = true;
--
1.8.3.1
^ permalink raw reply related
* [PATCH v2] powerpc, pkey: make protection key 0 less special
From: Ram Pai @ 2018-03-27 1:05 UTC (permalink / raw)
To: mpe, mingo, akpm
Cc: linuxppc-dev, linux-kernel, dave.hansen, benh, paulus,
aneesh.kumar, bsingharora, hbabu, mhocko, bauerman, linuxram,
fweimer, msuchanek, tglx, shuah
Applications need the ability to associate an address-range with some
key and latter revert to its initial default key. Pkey-0 comes close to
providing this function but falls short, because the current
implementation disallows applications to explicitly associate pkey-0 to
the address range.
Lets make pkey-0 less special and treat it almost like any other key.
Thus it can be explicitly associated with any address range, and can be
freed. This gives the application more flexibility and power. The
ability to free pkey-0 must be used responsibily, since pkey-0 is
associated with almost all address-range by default.
Even with this change pkey-0 continues to be slightly more special
from the following point of view.
(a) it is implicitly allocated.
(b) it is the default key assigned to any address-range.
Tested on powerpc.
cc: Thomas Gleixner <tglx@linutronix.de>
cc: Dave Hansen <dave.hansen@intel.com>
cc: Michael Ellermen <mpe@ellerman.id.au>
cc: Ingo Molnar <mingo@kernel.org>
cc: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Ram Pai <linuxram@us.ibm.com>
---
History:
v2: mm_pkey_is_allocated() continued to treat pkey-0 special.
fixed it.
arch/powerpc/include/asm/pkeys.h | 20 ++++++++++++++++----
arch/powerpc/mm/pkeys.c | 20 ++++++++++++--------
2 files changed, 28 insertions(+), 12 deletions(-)
diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
index 0409c80..b598fa9 100644
--- a/arch/powerpc/include/asm/pkeys.h
+++ b/arch/powerpc/include/asm/pkeys.h
@@ -101,10 +101,14 @@ static inline u16 pte_to_pkey_bits(u64 pteflags)
static inline bool mm_pkey_is_allocated(struct mm_struct *mm, int pkey)
{
- /* A reserved key is never considered as 'explicitly allocated' */
- return ((pkey < arch_max_pkey()) &&
- !__mm_pkey_is_reserved(pkey) &&
- __mm_pkey_is_allocated(mm, pkey));
+ if (pkey < 0 || pkey >= arch_max_pkey())
+ return false;
+
+ /* Reserved keys are never allocated. */
+ if (__mm_pkey_is_reserved(pkey))
+ return false;
+
+ return __mm_pkey_is_allocated(mm, pkey);
}
extern void __arch_activate_pkey(int pkey);
@@ -200,6 +204,14 @@ static inline int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
{
if (static_branch_likely(&pkey_disabled))
return -EINVAL;
+
+ /*
+ * userspace is discouraged from changing permissions of
+ * pkey-0. powerpc hardware does not support it anyway.
+ */
+ if (!pkey)
+ return init_val ? -EINVAL : 0;
+
return __arch_set_user_pkey_access(tsk, pkey, init_val);
}
diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
index ba71c54..e7a9e34 100644
--- a/arch/powerpc/mm/pkeys.c
+++ b/arch/powerpc/mm/pkeys.c
@@ -119,19 +119,21 @@ int pkey_initialize(void)
#else
os_reserved = 0;
#endif
- /*
- * Bits are in LE format. NOTE: 1, 0 are reserved.
- * key 0 is the default key, which allows read/write/execute.
- * key 1 is recommended not to be used. PowerISA(3.0) page 1015,
- * programming note.
- */
+ /* Bits are in LE format. */
initial_allocation_mask = ~0x0;
/* register mask is in BE format */
pkey_amr_uamor_mask = ~0x0ul;
pkey_iamr_mask = ~0x0ul;
- for (i = 2; i < (pkeys_total - os_reserved); i++) {
+ for (i = 0; i < (pkeys_total - os_reserved); i++) {
+ /*
+ * key 1 is recommended not to be used.
+ * PowerISA(3.0) page 1015,
+ */
+ if (i == 1)
+ continue;
+
initial_allocation_mask &= ~(0x1 << i);
pkey_amr_uamor_mask &= ~(0x3ul << pkeyshift(i));
pkey_iamr_mask &= ~(0x1ul << pkeyshift(i));
@@ -145,7 +147,9 @@ void pkey_mm_init(struct mm_struct *mm)
{
if (static_branch_likely(&pkey_disabled))
return;
- mm_pkey_allocation_map(mm) = initial_allocation_mask;
+
+ /* allocate key-0 by default */
+ mm_pkey_allocation_map(mm) = initial_allocation_mask | 0x1;
/* -1 means unallocated or invalid */
mm->context.execute_only_pkey = -1;
}
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH] powerpc, pkey: make protection key 0 less special
From: Ram Pai @ 2018-03-27 1:03 UTC (permalink / raw)
To: mpe, mingo, akpm
Cc: linuxppc-dev, linux-kernel, dave.hansen, benh, paulus,
aneesh.kumar, bsingharora, hbabu, mhocko, bauerman, fweimer,
msuchanek, tglx, shuah
In-Reply-To: <1522107101-16083-1-git-send-email-linuxram@us.ibm.com>
On Mon, Mar 26, 2018 at 04:31:41PM -0700, Ram Pai wrote:
> Applications need the ability to associate an address-range with some
> key and latter revert to its initial default key. Pkey-0 comes close to
> providing this function but falls short, because the current
> implementation disallows applications to explicitly associate pkey-0 to
> the address range.
>
> Lets make pkey-0 less special and treat it almost like any other key.
> Thus it can be explicitly associated with any address range, and can be
> freed. This gives the application more flexibility and power. The
> ability to free pkey-0 must be used responsibily, since pkey-0 is
> associated with almost all address-range by default.
>
> Even with this change pkey-0 continues to be slightly more special
> from the following point of view.
> (a) it is implicitly allocated.
> (b) it is the default key assigned to any address-range.
>
> Tested on powerpc.
This patch is not entirely correct.
>
> cc: Thomas Gleixner <tglx@linutronix.de>
> cc: Dave Hansen <dave.hansen@intel.com>
> cc: Michael Ellermen <mpe@ellerman.id.au>
> cc: Ingo Molnar <mingo@kernel.org>
> cc: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> ---
> arch/powerpc/include/asm/pkeys.h | 24 ++++++++++++++++++++----
> 1 file changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
> index 0409c80..9c7d3bd 100644
> --- a/arch/powerpc/include/asm/pkeys.h
> +++ b/arch/powerpc/include/asm/pkeys.h
> @@ -101,10 +101,18 @@ static inline u16 pte_to_pkey_bits(u64 pteflags)
>
> static inline bool mm_pkey_is_allocated(struct mm_struct *mm, int pkey)
> {
> - /* A reserved key is never considered as 'explicitly allocated' */
> - return ((pkey < arch_max_pkey()) &&
> - !__mm_pkey_is_reserved(pkey) &&
> - __mm_pkey_is_allocated(mm, pkey));
> + /* pkey 0 is allocated by default. */
> + if (!pkey)
> + return true;
This is wrong. pkey-0 should not be treated any special here. Will fix
this and send a new patch. Sorry for the noise.
RP
^ permalink raw reply
* Re: [RFC] new SYSCALL_DEFINE/COMPAT_SYSCALL_DEFINE wrappers
From: Linus Torvalds @ 2018-03-27 1:03 UTC (permalink / raw)
To: John Paul Adrian Glaubitz
Cc: Al Viro, Ingo Molnar, Dominik Brodowski,
Linux Kernel Mailing List, Arnd Bergmann, linux-arch,
Ralf Baechle, James Hogan, linux-mips, Benjamin Herrenschmidt,
Paul Mackerras, Michael Ellerman, ppc-dev, Martin Schwidefsky,
Heiko Carstens, linux-s390, David S . Miller, sparclinux,
Ingo Molnar, Jiri Slaby, the arch/x86 maintainers
In-Reply-To: <428751c8-6920-096b-8694-a3f1b8990bdf@physik.fu-berlin.de>
On Sun, Mar 25, 2018 at 8:44 PM, John Paul Adrian Glaubitz
<glaubitz@physik.fu-berlin.de> wrote:
<
> FWIW, we are maintaining an x32 port in Debian and there are some people
> actually using it [1]. There is one build instance running on VMWare that
> I am hosting [2] and around 10800 out of 12900 source packages build fine
> on x32 (12900 being the number for x86_64).
Hmm. Do you have a few statically built binaries that could be tested
without installing a whole distribution? Something real and meaningful
enough that it actually exercised a few real system calls, but not
something that needs to bring in 50 different shared libraries?
Something in /sbin, perhaps, that is still runnable by a regular user
and doesn't require some distro-specific /etc layout etc, so that it
would work even if you don't run Debian at all? Maybe some shell
binary or something?
Linus
^ permalink raw reply
* Re: RFC on writel and writel_relaxed
From: Benjamin Herrenschmidt @ 2018-03-26 23:59 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Sinan Kaya, Arnd Bergmann, David Laight, Oliver,
open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
linux-rdma@vger.kernel.org, Alexander Duyck, Will Deacon,
Paul E. McKenney
In-Reply-To: <20180326225046.GL15554@ziepe.ca>
On Mon, 2018-03-26 at 16:50 -0600, Jason Gunthorpe wrote:
> On Tue, Mar 27, 2018 at 09:36:11AM +1100, Benjamin Herrenschmidt wrote:
> > On Mon, 2018-03-26 at 16:27 -0600, Jason Gunthorpe wrote:
> > > > Otherwise almost all drivers out there are broken which I very much
> > > > doubt :-)
> > >
> > > But.. Sinan is right, you look anywhere in the driver tree and you
> > > find stuff like this:
> > >
> > > drivers/net/ethernet/intel/i40e/i40e_txrx.c
> > >
> > > /* Force memory writes to complete before letting h/w
> > > * know there are new descriptors to fetch.
> > > */
> > > wmb();
> > >
> > >
> > > It is *systemic*
> >
> > Yes, because they all copied e1000e :-) If you look at the comment in
> > there, it does say it's only for weakly ordered archs such as ia64, and
> > even then, probably predates Linus strong statement on the matter.
>
> Hahah, sure I'll buy that..
>
> But still, if this really is the case, a *strong* statement in
> barriers.txt to that effect (and not an example demanding the wmb()!)
> would be very helpful for those of us that have to review driver code!
I agree, and that Mellanox bug you pointed me to seems to indicate that
this may not even be true on x86 anymore ...
I think we might need to revisit this properly...
Cheers,
Ben.
^ permalink raw reply
* [PATCH] powerpc, pkey: make protection key 0 less special
From: Ram Pai @ 2018-03-26 23:31 UTC (permalink / raw)
To: mpe, mingo, akpm
Cc: linuxppc-dev, linux-kernel, dave.hansen, benh, paulus,
aneesh.kumar, bsingharora, hbabu, mhocko, bauerman, linuxram,
fweimer, msuchanek, tglx, shuah
Applications need the ability to associate an address-range with some
key and latter revert to its initial default key. Pkey-0 comes close to
providing this function but falls short, because the current
implementation disallows applications to explicitly associate pkey-0 to
the address range.
Lets make pkey-0 less special and treat it almost like any other key.
Thus it can be explicitly associated with any address range, and can be
freed. This gives the application more flexibility and power. The
ability to free pkey-0 must be used responsibily, since pkey-0 is
associated with almost all address-range by default.
Even with this change pkey-0 continues to be slightly more special
from the following point of view.
(a) it is implicitly allocated.
(b) it is the default key assigned to any address-range.
Tested on powerpc.
cc: Thomas Gleixner <tglx@linutronix.de>
cc: Dave Hansen <dave.hansen@intel.com>
cc: Michael Ellermen <mpe@ellerman.id.au>
cc: Ingo Molnar <mingo@kernel.org>
cc: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Ram Pai <linuxram@us.ibm.com>
---
arch/powerpc/include/asm/pkeys.h | 24 ++++++++++++++++++++----
1 file changed, 20 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
index 0409c80..9c7d3bd 100644
--- a/arch/powerpc/include/asm/pkeys.h
+++ b/arch/powerpc/include/asm/pkeys.h
@@ -101,10 +101,18 @@ static inline u16 pte_to_pkey_bits(u64 pteflags)
static inline bool mm_pkey_is_allocated(struct mm_struct *mm, int pkey)
{
- /* A reserved key is never considered as 'explicitly allocated' */
- return ((pkey < arch_max_pkey()) &&
- !__mm_pkey_is_reserved(pkey) &&
- __mm_pkey_is_allocated(mm, pkey));
+ /* pkey 0 is allocated by default. */
+ if (!pkey)
+ return true;
+
+ if (pkey < 0 || pkey >= arch_max_pkey())
+ return false;
+
+ /* Reserved keys are never allocated. */
+ if (__mm_pkey_is_reserved(pkey))
+ return false;
+
+ return __mm_pkey_is_allocated(mm, pkey);
}
extern void __arch_activate_pkey(int pkey);
@@ -200,6 +208,14 @@ static inline int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
{
if (static_branch_likely(&pkey_disabled))
return -EINVAL;
+
+ /*
+ * userspace is discouraged from changing permissions of
+ * pkey-0. powerpc hardware does not support it anyway.
+ */
+ if (!pkey)
+ return init_val ? -EINVAL : 0;
+
return __arch_set_user_pkey_access(tsk, pkey, init_val);
}
--
1.8.3.1
^ permalink raw reply related
* linux-next: manual merge of the powerpc tree with the powerpc-fixes tree
From: Stephen Rothwell @ 2018-03-26 23:11 UTC (permalink / raw)
To: Michael Ellerman, Benjamin Herrenschmidt, PowerPC
Cc: Linux-Next Mailing List, Linux Kernel Mailing List,
Aneesh Kumar K.V, Paul Mackerras
[-- Attachment #1: Type: text/plain, Size: 6245 bytes --]
Hi all,
Today's linux-next merge of the powerpc tree got a conflict in:
arch/powerpc/include/asm/cputable.h
arch/powerpc/kernel/dt_cpu_ftrs.c
between commit:
a5d4b5891c2f ("powerpc/mm: Fixup tlbie vs store ordering issue on POWER9")
from the powerpc-fixes tree and commits:
9bbf0b576d32 ("powerpc: Free up CPU feature bits on 64-bit machines")
b5af4f279323 ("powerpc: Add CPU feature bits for TM bug workarounds on POWER9 v2.2")
from the powerpc tree.
I fixed it up (see below) and can carry the fix as necessary. This
is now fixed as far as linux-next is concerned, but any non trivial
conflicts should be mentioned to your upstream maintainer when your tree
is submitted for merging. You may also want to consider cooperating
with the maintainer of the conflicting tree to minimise any particularly
complex conflicts.
--
Cheers,
Stephen Rothwell
diff --cc arch/powerpc/include/asm/cputable.h
index 2e2bacbdf6ed,ecee84dea7e7..000000000000
--- a/arch/powerpc/include/asm/cputable.h
+++ b/arch/powerpc/include/asm/cputable.h
@@@ -174,38 -181,40 +181,41 @@@ static inline void cpu_feature_keys_ini
#define LONG_ASM_CONST(x) 0
#endif
- #define CPU_FTR_HVMODE LONG_ASM_CONST(0x0000000100000000)
- #define CPU_FTR_ARCH_201 LONG_ASM_CONST(0x0000000200000000)
- #define CPU_FTR_ARCH_206 LONG_ASM_CONST(0x0000000400000000)
- #define CPU_FTR_ARCH_207S LONG_ASM_CONST(0x0000000800000000)
- #define CPU_FTR_ARCH_300 LONG_ASM_CONST(0x0000001000000000)
- #define CPU_FTR_MMCRA LONG_ASM_CONST(0x0000002000000000)
- #define CPU_FTR_CTRL LONG_ASM_CONST(0x0000004000000000)
- #define CPU_FTR_SMT LONG_ASM_CONST(0x0000008000000000)
- #define CPU_FTR_PAUSE_ZERO LONG_ASM_CONST(0x0000010000000000)
- #define CPU_FTR_PURR LONG_ASM_CONST(0x0000020000000000)
- #define CPU_FTR_CELL_TB_BUG LONG_ASM_CONST(0x0000040000000000)
- #define CPU_FTR_SPURR LONG_ASM_CONST(0x0000080000000000)
- #define CPU_FTR_DSCR LONG_ASM_CONST(0x0000100000000000)
- #define CPU_FTR_VSX LONG_ASM_CONST(0x0000200000000000)
- #define CPU_FTR_SAO LONG_ASM_CONST(0x0000400000000000)
- #define CPU_FTR_CP_USE_DCBTZ LONG_ASM_CONST(0x0000800000000000)
- #define CPU_FTR_UNALIGNED_LD_STD LONG_ASM_CONST(0x0001000000000000)
- #define CPU_FTR_ASYM_SMT LONG_ASM_CONST(0x0002000000000000)
- #define CPU_FTR_STCX_CHECKS_ADDRESS LONG_ASM_CONST(0x0004000000000000)
- #define CPU_FTR_POPCNTB LONG_ASM_CONST(0x0008000000000000)
- #define CPU_FTR_POPCNTD LONG_ASM_CONST(0x0010000000000000)
- #define CPU_FTR_PKEY LONG_ASM_CONST(0x0020000000000000)
- #define CPU_FTR_VMX_COPY LONG_ASM_CONST(0x0040000000000000)
- #define CPU_FTR_TM LONG_ASM_CONST(0x0080000000000000)
- #define CPU_FTR_CFAR LONG_ASM_CONST(0x0100000000000000)
- #define CPU_FTR_HAS_PPR LONG_ASM_CONST(0x0200000000000000)
- #define CPU_FTR_DAWR LONG_ASM_CONST(0x0400000000000000)
- #define CPU_FTR_DABRX LONG_ASM_CONST(0x0800000000000000)
- #define CPU_FTR_PMAO_BUG LONG_ASM_CONST(0x1000000000000000)
- #define CPU_FTR_P9_TLBIE_BUG LONG_ASM_CONST(0x2000000000000000)
- #define CPU_FTR_POWER9_DD1 LONG_ASM_CONST(0x4000000000000000)
- #define CPU_FTR_POWER9_DD2_1 LONG_ASM_CONST(0x8000000000000000)
+ #define CPU_FTR_REAL_LE LONG_ASM_CONST(0x0000000000001000)
+ #define CPU_FTR_HVMODE LONG_ASM_CONST(0x0000000000002000)
+ #define CPU_FTR_ARCH_201 LONG_ASM_CONST(0x0000000000004000)
+ #define CPU_FTR_ARCH_206 LONG_ASM_CONST(0x0000000000008000)
+ #define CPU_FTR_ARCH_207S LONG_ASM_CONST(0x0000000000010000)
+ #define CPU_FTR_ARCH_300 LONG_ASM_CONST(0x0000000000020000)
+ #define CPU_FTR_MMCRA LONG_ASM_CONST(0x0000000000040000)
+ #define CPU_FTR_CTRL LONG_ASM_CONST(0x0000000000080000)
+ #define CPU_FTR_SMT LONG_ASM_CONST(0x0000000000100000)
+ #define CPU_FTR_PAUSE_ZERO LONG_ASM_CONST(0x0000000000200000)
+ #define CPU_FTR_PURR LONG_ASM_CONST(0x0000000000400000)
+ #define CPU_FTR_CELL_TB_BUG LONG_ASM_CONST(0x0000000000800000)
+ #define CPU_FTR_SPURR LONG_ASM_CONST(0x0000000001000000)
+ #define CPU_FTR_DSCR LONG_ASM_CONST(0x0000000002000000)
+ #define CPU_FTR_VSX LONG_ASM_CONST(0x0000000004000000)
+ #define CPU_FTR_SAO LONG_ASM_CONST(0x0000000008000000)
+ #define CPU_FTR_CP_USE_DCBTZ LONG_ASM_CONST(0x0000000010000000)
+ #define CPU_FTR_UNALIGNED_LD_STD LONG_ASM_CONST(0x0000000020000000)
+ #define CPU_FTR_ASYM_SMT LONG_ASM_CONST(0x0000000040000000)
+ #define CPU_FTR_STCX_CHECKS_ADDRESS LONG_ASM_CONST(0x0000000080000000)
+ #define CPU_FTR_POPCNTB LONG_ASM_CONST(0x0000000100000000)
+ #define CPU_FTR_POPCNTD LONG_ASM_CONST(0x0000000200000000)
+ #define CPU_FTR_PKEY LONG_ASM_CONST(0x0000000400000000)
+ #define CPU_FTR_VMX_COPY LONG_ASM_CONST(0x0000000800000000)
+ #define CPU_FTR_TM LONG_ASM_CONST(0x0000001000000000)
+ #define CPU_FTR_CFAR LONG_ASM_CONST(0x0000002000000000)
+ #define CPU_FTR_HAS_PPR LONG_ASM_CONST(0x0000004000000000)
+ #define CPU_FTR_DAWR LONG_ASM_CONST(0x0000008000000000)
+ #define CPU_FTR_DABRX LONG_ASM_CONST(0x0000010000000000)
+ #define CPU_FTR_PMAO_BUG LONG_ASM_CONST(0x0000020000000000)
+ #define CPU_FTR_POWER9_DD1 LONG_ASM_CONST(0x0000040000000000)
+ #define CPU_FTR_POWER9_DD2_1 LONG_ASM_CONST(0x0000080000000000)
+ #define CPU_FTR_P9_TM_HV_ASSIST LONG_ASM_CONST(0x0000100000000000)
+ #define CPU_FTR_P9_TM_XER_SO_BUG LONG_ASM_CONST(0x0000200000000000)
++#define CPU_FTR_P9_TLBIE_BUG LONG_ASM_CONST(0x0000400000000000)
#ifndef __ASSEMBLY__
diff --cc arch/powerpc/kernel/dt_cpu_ftrs.c
index 8ca5d5b74618,0a0c601c6ade..000000000000
--- a/arch/powerpc/kernel/dt_cpu_ftrs.c
+++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
@@@ -709,9 -710,9 +710,12 @@@ static __init void cpufeatures_cpu_quir
cur_cpu_spec->cpu_features |= CPU_FTR_POWER9_DD1;
else if ((version & 0xffffefff) == 0x004e0201)
cur_cpu_spec->cpu_features |= CPU_FTR_POWER9_DD2_1;
+ else if ((version & 0xffffefff) == 0x004e0202)
+ cur_cpu_spec->cpu_features |= CPU_FTR_P9_TM_HV_ASSIST |
+ CPU_FTR_P9_TM_XER_SO_BUG;
+
+ if ((version & 0xffff0000) == 0x004e0000)
+ cur_cpu_spec->cpu_features |= CPU_FTR_P9_TLBIE_BUG;
}
static void __init cpufeatures_setup_finished(void)
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: RFC on writel and writel_relaxed
From: Jason Gunthorpe @ 2018-03-26 22:50 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Sinan Kaya, Arnd Bergmann, David Laight, Oliver,
open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
linux-rdma@vger.kernel.org, Alexander Duyck, Will Deacon,
Paul E. McKenney
In-Reply-To: <1522103771.7364.20.camel@kernel.crashing.org>
On Tue, Mar 27, 2018 at 09:36:11AM +1100, Benjamin Herrenschmidt wrote:
> On Mon, 2018-03-26 at 16:27 -0600, Jason Gunthorpe wrote:
> > > Otherwise almost all drivers out there are broken which I very much
> > > doubt :-)
> >
> > But.. Sinan is right, you look anywhere in the driver tree and you
> > find stuff like this:
> >
> > drivers/net/ethernet/intel/i40e/i40e_txrx.c
> >
> > /* Force memory writes to complete before letting h/w
> > * know there are new descriptors to fetch.
> > */
> > wmb();
> >
> >
> > It is *systemic*
>
> Yes, because they all copied e1000e :-) If you look at the comment in
> there, it does say it's only for weakly ordered archs such as ia64, and
> even then, probably predates Linus strong statement on the matter.
Hahah, sure I'll buy that..
But still, if this really is the case, a *strong* statement in
barriers.txt to that effect (and not an example demanding the wmb()!)
would be very helpful for those of us that have to review driver code!
Jason
^ permalink raw reply
* Re: RFC on writel and writel_relaxed
From: Benjamin Herrenschmidt @ 2018-03-26 22:42 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Sinan Kaya, Arnd Bergmann, David Laight, Oliver,
open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
linux-rdma@vger.kernel.org, Alexander Duyck, Will Deacon,
Paul E. McKenney
In-Reply-To: <1522103771.7364.20.camel@kernel.crashing.org>
On Tue, 2018-03-27 at 09:36 +1100, Benjamin Herrenschmidt wrote:
> I don't kow, it used to be the case, at least that's what drove us to
> define things the way we did.
>
> Maybe things changed, but if that's the case, nobody knows for sure,
> and we probably want to get Linus POV on the matter.
>
> I know I still write drivers that do not add a wmb in that case because
> I expect things to work without it.
>
> If that has changed, we probably can relax some of the barriers in our
> implementations of writel on a number of architectures, but not before
> auditing a bunch more drivers to make sure they have the write wmb()'s
Note also that this was the entire point behind the definition of
the _relaxed() accessors, to lift that specific ordering guarantee.
If you now says that memory + writel requires a wmb() in between
then you made writel be identical to writel_relaxed.
You might notice that Documentation/driver-api/device-io.rst makes no
mention of wmb() at all.
Cheers,
Ben.
^ permalink raw reply
* Re: RFC on writel and writel_relaxed
From: Benjamin Herrenschmidt @ 2018-03-26 22:36 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Sinan Kaya, Arnd Bergmann, David Laight, Oliver,
open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
linux-rdma@vger.kernel.org, Alexander Duyck, Will Deacon,
Paul E. McKenney
In-Reply-To: <20180326222756.GJ15554@ziepe.ca>
On Mon, 2018-03-26 at 16:27 -0600, Jason Gunthorpe wrote:
> > Otherwise almost all drivers out there are broken which I very much
> > doubt :-)
>
> But.. Sinan is right, you look anywhere in the driver tree and you
> find stuff like this:
>
> drivers/net/ethernet/intel/i40e/i40e_txrx.c
>
> /* Force memory writes to complete before letting h/w
> * know there are new descriptors to fetch.
> */
> wmb();
>
>
> It is *systemic*
Yes, because they all copied e1000e :-) If you look at the comment in
there, it does say it's only for weakly ordered archs such as ia64, and
even then, probably predates Linus strong statement on the matter.
> I even see patches adding wmb() based on actual observed memory
> corruption during testing on Intel:
>
> https://patchwork.kernel.org/patch/10177207/
>
> So you think all of this is unnecessary and writel is totally strongly
> ordered, even on multi-socket Intel?
I don't kow, it used to be the case, at least that's what drove us to
define things the way we did.
Maybe things changed, but if that's the case, nobody knows for sure,
and we probably want to get Linus POV on the matter.
I know I still write drivers that do not add a wmb in that case because
I expect things to work without it.
If that has changed, we probably can relax some of the barriers in our
implementations of writel on a number of architectures, but not before
auditing a bunch more drivers to make sure they have the write wmb()'s
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCH v3 3/6] powerpc/kexec: Disable ftrace before switching to the new kernel
From: Michael Ellerman @ 2018-03-26 22:32 UTC (permalink / raw)
To: Naveen N. Rao, Steven Rostedt, Paul Mackerras
Cc: linuxppc-dev, Anton Blanchard, Nicholas Piggin, sathnaga
In-Reply-To: <877epylba7.fsf@concordia.ellerman.id.au>
Michael Ellerman <mpe@ellerman.id.au> writes:
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:
>> If function_graph tracer is enabled during kexec, we see the below
>> exception in the simulator:
>> root@(none):/# kexec -e
>> kvm: exiting hardware virtualization
>> kexec_core: Starting new kernel
>> [ 19.262020070,5] OPAL: Switch to big-endian OS
>> kexec: Starting switchover sequence.
>> Interrupt to 0xC000000000004380 from 0xC000000000004380
>> ** Execution stopped: Continuous Interrupt, Instruction caused exception, **
>>
>> Now that we have a more effective way to disable ftrace, let's use that
>> before switching to a new kernel during kexec.
>>
>> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
>> ---
>> arch/powerpc/kernel/machine_kexec.c | 6 ++----
>> 1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/powerpc/kernel/machine_kexec.c b/arch/powerpc/kernel/machine_kexec.c
>> index 2694d078741d..4a1b24a9dd61 100644
>> --- a/arch/powerpc/kernel/machine_kexec.c
>> +++ b/arch/powerpc/kernel/machine_kexec.c
>> @@ -95,16 +95,14 @@ void arch_crash_save_vmcoreinfo(void)
>> */
>> void machine_kexec(struct kimage *image)
>> {
>> - int save_ftrace_enabled;
>> -
>> - save_ftrace_enabled = __ftrace_enabled_save();
>> + get_paca()->ftrace_disabled = 1;
>
> This doesn't compile on 32-bit:
>
> Failed: powerpc-next/corenet32_smp_defconfig/powerpc-5.3 (http://kisskb.ozlabs.ibm.com/kisskb/buildresult/13314085/log/)
> /kisskb/src/arch/powerpc/kernel/machine_kexec.c:105:12: error: invalid type argument of '->' (have 'int')
> /kisskb/src/arch/powerpc/kernel/machine_kexec.c:98:12: error: invalid type argument of '->' (have 'int')
> /kisskb/src/arch/powerpc/kernel/machine_kexec.c:98:2: error: implicit declaration of function 'get_paca' [-Werror=implicit-function-declaration]
>
>
> A wrapper is probably best that does nothing on 32-bit.
Actually it should probably do __ftrace_enabled_save() etc. on 32-bit,
like the existing code does.
cheers
^ permalink raw reply
* Re: [PATCH v3 3/6] powerpc/kexec: Disable ftrace before switching to the new kernel
From: Michael Ellerman @ 2018-03-26 22:30 UTC (permalink / raw)
To: Naveen N. Rao, Steven Rostedt, Paul Mackerras
Cc: linuxppc-dev, Anton Blanchard, Nicholas Piggin, sathnaga
In-Reply-To: <5589149f6dd1b627f5eadafa086fb5e716bc0622.1521713762.git.naveen.n.rao@linux.vnet.ibm.com>
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:
> If function_graph tracer is enabled during kexec, we see the below
> exception in the simulator:
> root@(none):/# kexec -e
> kvm: exiting hardware virtualization
> kexec_core: Starting new kernel
> [ 19.262020070,5] OPAL: Switch to big-endian OS
> kexec: Starting switchover sequence.
> Interrupt to 0xC000000000004380 from 0xC000000000004380
> ** Execution stopped: Continuous Interrupt, Instruction caused exception, **
>
> Now that we have a more effective way to disable ftrace, let's use that
> before switching to a new kernel during kexec.
>
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
> arch/powerpc/kernel/machine_kexec.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/kernel/machine_kexec.c b/arch/powerpc/kernel/machine_kexec.c
> index 2694d078741d..4a1b24a9dd61 100644
> --- a/arch/powerpc/kernel/machine_kexec.c
> +++ b/arch/powerpc/kernel/machine_kexec.c
> @@ -95,16 +95,14 @@ void arch_crash_save_vmcoreinfo(void)
> */
> void machine_kexec(struct kimage *image)
> {
> - int save_ftrace_enabled;
> -
> - save_ftrace_enabled = __ftrace_enabled_save();
> + get_paca()->ftrace_disabled = 1;
This doesn't compile on 32-bit:
Failed: powerpc-next/corenet32_smp_defconfig/powerpc-5.3 (http://kisskb.ozlabs.ibm.com/kisskb/buildresult/13314085/log/)
/kisskb/src/arch/powerpc/kernel/machine_kexec.c:105:12: error: invalid type argument of '->' (have 'int')
/kisskb/src/arch/powerpc/kernel/machine_kexec.c:98:12: error: invalid type argument of '->' (have 'int')
/kisskb/src/arch/powerpc/kernel/machine_kexec.c:98:2: error: implicit declaration of function 'get_paca' [-Werror=implicit-function-declaration]
A wrapper is probably best that does nothing on 32-bit.
cheers
^ permalink raw reply
* Re: RFC on writel and writel_relaxed
From: Benjamin Herrenschmidt @ 2018-03-26 22:28 UTC (permalink / raw)
To: Sinan Kaya, Arnd Bergmann, Jason Gunthorpe
Cc: David Laight, Oliver,
open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
linux-rdma@vger.kernel.org, Alexander Duyck, Will Deacon,
Paul E. McKenney
In-Reply-To: <4d85c5e9-a16a-0761-4700-169d1c061812@codeaurora.org>
On Mon, 2018-03-26 at 18:08 -0400, Sinan Kaya wrote:
> On 3/26/2018 6:01 PM, Benjamin Herrenschmidt wrote:
> > On Mon, 2018-03-26 at 17:46 -0400, Sinan Kaya wrote:
> > > On 3/26/2018 5:30 PM, Arnd Bergmann wrote:
> > > > > But that was never a requirement of writel(),
> > > > > Documentation/memory-barriers.txt gives an explicit example demanding
> > > > > the wmb() before writel() for ordering system memory against writel.
> > > >
> > > > Indeed, but it's in an example for when to use dma_wmb(), not wmb().
> > > > Adding Alexander Duyck to Cc, he added that section as part of
> > > > 1077fa36f23e ("arch: Add lightweight memory barriers dma_rmb() and
> > > > dma_wmb()"). Also adding the other people that were involved with that.
> > > >
> > >
> > > ARM developers can get away with not including wmb() in their code and use
> > > writel() to observe memory writes due to implicit barriers.
> > >
> > > However, same code will not work on Intel.
> >
> > Wrong. It will.
> >
> > You do NOT need wmb between writes to memory and writel.
>
> If writel() provides such a guarantee, why do I see code sequences like
>
> wmb()
> writel()
>
> all over the place.
Because it was badly documented and people didn't know what to do, or
maybe the underlying mapping is WC ?
I don't know for sure but I can tell you Linus opinion on the matter
back in the days was very clear and that's why we implemented writel
the way we did on powerpc.
> >
> > > writel() has a compiler barrier in it for x86.
> > > wmb() has a sync operation in it for x86.
> > >
> > > Unless wmb() is called, PCIe device won't observe memory updates from the CPU.
> >
> > This is completely wrong. They will. Intel provides the necessary
> > ordering guarantees without an explicit wmb.
> >
>
> I'm still reserving my doubts here. I was told about an explicit
> wmb() requirement last week.
By whome ?
> > Otherwise almost all drivers out there are broken which I very much
> > doubt :-)
> >
> > Cheers,
> > Ben.
> >
> >
> >
>
>
^ permalink raw reply
* Re: RFC on writel and writel_relaxed
From: Jason Gunthorpe @ 2018-03-26 22:27 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Sinan Kaya, Arnd Bergmann, David Laight, Oliver,
open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
linux-rdma@vger.kernel.org, Alexander Duyck, Will Deacon,
Paul E. McKenney
In-Reply-To: <1522101717.7364.14.camel@kernel.crashing.org>
On Tue, Mar 27, 2018 at 09:01:57AM +1100, Benjamin Herrenschmidt wrote:
> On Mon, 2018-03-26 at 17:46 -0400, Sinan Kaya wrote:
> > On 3/26/2018 5:30 PM, Arnd Bergmann wrote:
> > > > But that was never a requirement of writel(),
> > > > Documentation/memory-barriers.txt gives an explicit example demanding
> > > > the wmb() before writel() for ordering system memory against writel.
> > >
> > > Indeed, but it's in an example for when to use dma_wmb(), not wmb().
> > > Adding Alexander Duyck to Cc, he added that section as part of
> > > 1077fa36f23e ("arch: Add lightweight memory barriers dma_rmb() and
> > > dma_wmb()"). Also adding the other people that were involved with that.
> > >
> >
> > ARM developers can get away with not including wmb() in their code and use
> > writel() to observe memory writes due to implicit barriers.
> >
> > However, same code will not work on Intel.
>
> Wrong. It will.
>
> You do NOT need wmb between writes to memory and writel.
>
> > writel() has a compiler barrier in it for x86.
> > wmb() has a sync operation in it for x86.
> >
> > Unless wmb() is called, PCIe device won't observe memory updates from the CPU.
>
> This is completely wrong. They will. Intel provides the necessary
> ordering guarantees without an explicit wmb.
>
> Otherwise almost all drivers out there are broken which I very much
> doubt :-)
But.. Sinan is right, you look anywhere in the driver tree and you
find stuff like this:
drivers/net/ethernet/intel/i40e/i40e_txrx.c
/* Force memory writes to complete before letting h/w
* know there are new descriptors to fetch.
*/
wmb();
It is *systemic*
I even see patches adding wmb() based on actual observed memory
corruption during testing on Intel:
https://patchwork.kernel.org/patch/10177207/
So you think all of this is unnecessary and writel is totally strongly
ordered, even on multi-socket Intel?
Jason
^ permalink raw reply
* Re: [PATCH 6/6] doc/devicetree: NVDIMM region documentation
From: Rob Herring @ 2018-03-26 22:24 UTC (permalink / raw)
To: Oliver O'Halloran; +Cc: linux-nvdimm, devicetree, linuxppc-dev
In-Reply-To: <20180323081209.31387-6-oohall@gmail.com>
On Fri, Mar 23, 2018 at 07:12:09PM +1100, Oliver O'Halloran wrote:
> Add device-tree binding documentation for the nvdimm region driver.
>
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
> ---
> .../devicetree/bindings/nvdimm/nvdimm-region.txt | 45 ++++++++++++++++++++++
> 1 file changed, 45 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt
>
> diff --git a/Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt b/Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt
> new file mode 100644
> index 000000000000..02091117ff16
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt
> @@ -0,0 +1,45 @@
> +Device-tree bindings for NVDIMM memory regions
> +-----------------------------------------------------
> +
> +Non-volatile DIMMs are memory modules used to provide (cacheable) main memory
Are DIMMs always going to be the only form factor for NV memory?
And if you have multiple DIMMs, does each DT node correspond to a DIMM?
If not, then what if we want/need to provide power control to a DIMM?
> +that retains its contents across power cycles. In more practical terms, they
> +are kind of storage device where the contents can be accessed by the CPU
> +directly, rather than indirectly via a storage controller or similar. The an
> +nvdimm-region specifies a physical address range that is hosted on an NVDIMM
> +device.
> +
> +Bindings for the region nodes:
> +-----------------------------
> +
> +Required properties:
> + - compatible = "nvdimm-region"
> +
> + - reg = <base, size>;
> + The system physical address range of this nvdimm region.
> +
> +Optional properties:
> + - Any relevant NUMA assocativity properties for the target platform.
> + - A "volatile" property indicating that this region is actually in
> + normal DRAM and does not require cache flushes after each write.
> +
> +A complete example:
> +--------------------
> +
> +/ {
> + #size-cells = <2>;
> + #address-cells = <2>;
> +
> + platform {
Perhaps we need a more well defined node here. Like we have 'memory' for
memory nodes.
> + region@5000 {
> + compatible = "nvdimm-region;
> + reg = <0x00000001 0x00000000 0x00000000 0x40000000>
> +
> + };
> +
> + region@6000 {
> + compatible = "nvdimm-region";
> + reg = <0x00000001 0x00000000 0x00000000 0x40000000>
Your reg property and unit-address don't match and you have overlapping
regions.
> + volatile;
> + };
> + };
> +};
> --
> 2.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: RFC on writel and writel_relaxed
From: Sinan Kaya @ 2018-03-26 22:08 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Arnd Bergmann, Jason Gunthorpe
Cc: David Laight, Oliver,
open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
linux-rdma@vger.kernel.org, Alexander Duyck, Will Deacon,
Paul E. McKenney
In-Reply-To: <1522101717.7364.14.camel@kernel.crashing.org>
On 3/26/2018 6:01 PM, Benjamin Herrenschmidt wrote:
> On Mon, 2018-03-26 at 17:46 -0400, Sinan Kaya wrote:
>> On 3/26/2018 5:30 PM, Arnd Bergmann wrote:
>>>> But that was never a requirement of writel(),
>>>> Documentation/memory-barriers.txt gives an explicit example demanding
>>>> the wmb() before writel() for ordering system memory against writel.
>>>
>>> Indeed, but it's in an example for when to use dma_wmb(), not wmb().
>>> Adding Alexander Duyck to Cc, he added that section as part of
>>> 1077fa36f23e ("arch: Add lightweight memory barriers dma_rmb() and
>>> dma_wmb()"). Also adding the other people that were involved with that.
>>>
>>
>> ARM developers can get away with not including wmb() in their code and use
>> writel() to observe memory writes due to implicit barriers.
>>
>> However, same code will not work on Intel.
>
> Wrong. It will.
>
> You do NOT need wmb between writes to memory and writel.
If writel() provides such a guarantee, why do I see code sequences like
wmb()
writel()
all over the place.
>
>> writel() has a compiler barrier in it for x86.
>> wmb() has a sync operation in it for x86.
>>
>> Unless wmb() is called, PCIe device won't observe memory updates from the CPU.
>
> This is completely wrong. They will. Intel provides the necessary
> ordering guarantees without an explicit wmb.
>
I'm still reserving my doubts here. I was told about an explicit
wmb() requirement last week.
> Otherwise almost all drivers out there are broken which I very much
> doubt :-)
>
> Cheers,
> Ben.
>
>
>
--
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply
* Re: RFC on writel and writel_relaxed
From: Benjamin Herrenschmidt @ 2018-03-26 22:01 UTC (permalink / raw)
To: Sinan Kaya, Arnd Bergmann, Jason Gunthorpe
Cc: David Laight, Oliver,
open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
linux-rdma@vger.kernel.org, Alexander Duyck, Will Deacon,
Paul E. McKenney
In-Reply-To: <ebb36fd4-62e2-394a-219f-686c45a6dc75@codeaurora.org>
On Mon, 2018-03-26 at 17:46 -0400, Sinan Kaya wrote:
> On 3/26/2018 5:30 PM, Arnd Bergmann wrote:
> > > But that was never a requirement of writel(),
> > > Documentation/memory-barriers.txt gives an explicit example demanding
> > > the wmb() before writel() for ordering system memory against writel.
> >
> > Indeed, but it's in an example for when to use dma_wmb(), not wmb().
> > Adding Alexander Duyck to Cc, he added that section as part of
> > 1077fa36f23e ("arch: Add lightweight memory barriers dma_rmb() and
> > dma_wmb()"). Also adding the other people that were involved with that.
> >
>
> ARM developers can get away with not including wmb() in their code and use
> writel() to observe memory writes due to implicit barriers.
>
> However, same code will not work on Intel.
Wrong. It will.
You do NOT need wmb between writes to memory and writel.
> writel() has a compiler barrier in it for x86.
> wmb() has a sync operation in it for x86.
>
> Unless wmb() is called, PCIe device won't observe memory updates from the CPU.
This is completely wrong. They will. Intel provides the necessary
ordering guarantees without an explicit wmb.
Otherwise almost all drivers out there are broken which I very much
doubt :-)
Cheers,
Ben.
^ permalink raw reply
* Re: RFC on writel and writel_relaxed
From: Benjamin Herrenschmidt @ 2018-03-26 22:00 UTC (permalink / raw)
To: Arnd Bergmann, Jason Gunthorpe
Cc: David Laight, Sinan Kaya, Oliver,
open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
linux-rdma@vger.kernel.org, Alexander Duyck, Will Deacon,
Paul E. McKenney
In-Reply-To: <CAK8P3a2UU1xAM0NLo7Q4-Xgo1SzY3De1uqpFudr+2ZW7nHEPmA@mail.gmail.com>
On Mon, 2018-03-26 at 23:30 +0200, Arnd Bergmann wrote:
> Most of the drivers have a unwound loop with writeq() or something to
> > do it.
>
> But isn't the writeq() barrier much more expensive than anything you'd
> do in function calls?
It is for us, and will break any write combining.
> > > > The same document says that _relaxed() does not give that guarentee.
> > > >
> > > > The lwn articule on this went into some depth on the interaction with
> > > > spinlocks.
> > > >
> > > > As far as I can see, containment in a spinlock seems to be the only
> > > > different between writel and writel_relaxed..
> > >
> > > I was always puzzled by this: The intention of _relaxed() on ARM
> > > (where it originates) was to skip the barrier that serializes DMA
> > > with MMIO, not to skip the serialization between MMIO and locks.
> >
> > But that was never a requirement of writel(),
> > Documentation/memory-barriers.txt gives an explicit example demanding
> > the wmb() before writel() for ordering system memory against writel.
This is a bug in the documentation.
> Indeed, but it's in an example for when to use dma_wmb(), not wmb().
> Adding Alexander Duyck to Cc, he added that section as part of
> 1077fa36f23e ("arch: Add lightweight memory barriers dma_rmb() and
> dma_wmb()"). Also adding the other people that were involved with that.
Linus himself made it very clear years ago. readl and writel have to
order vs memory accesses.
> > I actually have no idea why ARM had that barrier, I always assumed it
> > was to give program ordering to the accesses and that _relaxed allowed
> > re-ordering (the usual meaning of relaxed)..
> >
> > But the barrier document makes it pretty clear that the only
> > difference between the two is spinlock containment, and WillD wrote
> > this text, so I belive it is accurate for ARM.
> >
> > Very confusing.
>
> It does mention serialization with both DMA and locks in the
> section about readX_relaxed()/writeX_relaxed(). The part
> about DMA is very clear here, and I must have just forgotten
> the exact semantics with regards to spinlocks. I'm still not
> sure what prevents a writel() from leaking out the end of a
> spinlock section that doesn't happen with writel_relaxed(), since
> the barrier in writel() comes before the access, and the
> spin_unlock() shouldn't affect the external buses.
So...
Historically, what happened is that we (we means whoever participated
in the discussion on the list with Linus calling the shots really)
decided that there was no sane way for drivers to understand a world
where readl/writel didn't fully order things vs. memory accesses (ie,
DMA).
So it should always be correct to do:
- Write to some in-memory buffer
- writel() to kick the DMA read of that buffer
without any extra barrier.
The spinlock situation however got murky. Mostly that came up because
on architecture (I forgot who, might have been ia64) has a hard time
providing that consistency without making writel insanely expensive.
Thus they created mmiowb whose main purpose was precisely to order
writel with a following spin_unlock.
I decided not to go down that path on power because getting all drivers
"fixed" to do the right thing was going to be a losing battle, and
instead added per-cpu tracking of writel in order to "escalate" to a
heavier barrier in spin_unlock itself when necessary.
Now, all this happened more than a decade ago and it's possible that
the understanding or expectations "shifted" over time...
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