* Re: [PATCH v5 17/17] powerpc/32s: Enable CONFIG_VMAP_STACK
From: Guenter Roeck @ 2020-02-07 13:44 UTC (permalink / raw)
To: Christophe Leroy
Cc: linux-mm, linuxppc-dev, Paul Mackerras, linux-kernel, dja
In-Reply-To: <29bd8702-9b8f-6931-2bbc-db7e444907d5@c-s.fr>
On 2/7/20 5:29 AM, Christophe Leroy wrote:
>
>
> On 02/07/2020 01:08 PM, Guenter Roeck wrote:
>> On 2/7/20 12:28 AM, Christophe Leroy wrote:
>>>
>>>
>>> On 02/07/2020 06:13 AM, Christophe Leroy wrote:
>>>>
>>>>
>>>> Le 06/02/2020 à 21:31, Guenter Roeck a écrit :
>>>>> On Sat, Dec 21, 2019 at 08:32:38AM +0000, Christophe Leroy wrote:
>>>>>> A few changes to retrieve DAR and DSISR from struct regs
>>>>>> instead of retrieving them directly, as they may have
>>>>>> changed due to a TLB miss.
>>>>>>
>>>>>> Also modifies hash_page() and friends to work with virtual
>>>>>> data addresses instead of physical ones. Same on load_up_fpu()
>>>>>> and load_up_altivec().
>>>>>>
>>>>>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>>>>>
>>>>> This patch results in qemu boot failures (mac99 with pmac32_defconfig).
>>>>> Images fail silently; there is no console output. Reverting the patch
>>>>> fixes the problem. Bisect log is attached below.
>>>>>
>>>>> Assuming this was tested on real hardware, am I correct to assume that qemu
>>>>> for ppc32 (more specifically, qemu's mac99 and g3beige machines) no longer
>>>>> works with the upstream kernel ?
>>>>
>>>> Before submitting the series, I successfully tested:
>>>> - Real HW with powerpc 8xx
>>>> - Real HW with powerpc 832x
>>>> - Qemu's mac99
>>>>
>>>> I'll re-check the upstream kernel.
>>>>
>>>
>>> This is still working for me with the upstream kernel:
>>>
>>
>> Interesting. What is your kernel configuration, your qemu version, and
>> your qemu command line ?
>
> Config is pmac32_defconfig + CONFIG_DEVTMPFS (But kernel also boots without CONFIG_DEVTMPFS)
>
> QEMU emulator version 2.11.2
>
> qemu-system-ppc -kernel vmlinux -M mac99 -initrd rootfs.cpio.gz -s -m 1024
>
> Works with both GCC 5.5 and GCC 8.1
>
Actually, the problem is that I have locking selftest options enabled
in my tests. Everything works if I disable those. The "culprit" seems
to be CONFIG_PROVE_LOCKING. Can you retest with CONFIG_PROVE_LOCKING=y ?
Thanks,
Guenter
^ permalink raw reply
* Re: [PATCH v5 01/10] capabilities: introduce CAP_PERFMON to kernel and user space
From: Alexey Budankov @ 2020-02-07 13:39 UTC (permalink / raw)
To: Thomas Gleixner, Stephen Smalley, Serge Hallyn, James Morris
Cc: Mark Rutland, Song Liu, Peter Zijlstra,
joonas.lahtinen@linux.intel.com, Will Deacon, Alexei Starovoitov,
Lionel Landwerlin, Paul Mackerras, Jiri Olsa, Alexei Starovoitov,
Andi Kleen, Igor Lubashev, Alexander Shishkin, Ingo Molnar,
oprofile-list, linux-arm-kernel, Robert Richter,
selinux@vger.kernel.org, intel-gfx@lists.freedesktop.org,
jani.nikula@linux.intel.com, Arnaldo Carvalho de Melo,
rodrigo.vivi@intel.com, Namhyung Kim, Stephane Eranian,
linux-parisc@vger.kernel.org, linux-kernel, Andy Lutomirski,
linux-perf-users@vger.kernel.org,
linux-security-module@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <875zgizkyk.fsf@nanos.tec.linutronix.de>
On 07.02.2020 14:38, Thomas Gleixner wrote:
> Alexey Budankov <alexey.budankov@linux.intel.com> writes:
>> On 22.01.2020 17:25, Alexey Budankov wrote:
>>> On 22.01.2020 17:07, Stephen Smalley wrote:
>>>>> It keeps the implementation simple and readable. The implementation is more
>>>>> performant in the sense of calling the API - one capable() call for CAP_PERFMON
>>>>> privileged process.
>>>>>
>>>>> Yes, it bloats audit log for CAP_SYS_ADMIN privileged and unprivileged processes,
>>>>> but this bloating also advertises and leverages using more secure CAP_PERFMON
>>>>> based approach to use perf_event_open system call.
>>>>
>>>> I can live with that. We just need to document that when you see
>>>> both a CAP_PERFMON and a CAP_SYS_ADMIN audit message for a process,
>>>> try only allowing CAP_PERFMON first and see if that resolves the
>>>> issue. We have a similar issue with CAP_DAC_READ_SEARCH versus
>>>> CAP_DAC_OVERRIDE.
>>>
>>> perf security [1] document can be updated, at least, to align and document
>>> this audit logging specifics.
>>
>> And I plan to update the document right after this patch set is accepted.
>> Feel free to let me know of the places in the kernel docs that also
>> require update w.r.t CAP_PERFMON extension.
>
> The documentation update wants be part of the patch set and not planned
> to be done _after_ the patch set is merged.
Well, accepted. It is going to make patches #11 and beyond.
Thanks,
Alexey
>
> Thanks,
>
> tglx
>
^ permalink raw reply
* Re: [PATCH v5 17/17] powerpc/32s: Enable CONFIG_VMAP_STACK
From: Christophe Leroy @ 2020-02-07 13:29 UTC (permalink / raw)
To: Guenter Roeck; +Cc: linux-mm, linuxppc-dev, Paul Mackerras, linux-kernel, dja
In-Reply-To: <551bad84-3e80-265b-93ab-25eae4aa9807@roeck-us.net>
On 02/07/2020 01:08 PM, Guenter Roeck wrote:
> On 2/7/20 12:28 AM, Christophe Leroy wrote:
>>
>>
>> On 02/07/2020 06:13 AM, Christophe Leroy wrote:
>>>
>>>
>>> Le 06/02/2020 à 21:31, Guenter Roeck a écrit :
>>>> On Sat, Dec 21, 2019 at 08:32:38AM +0000, Christophe Leroy wrote:
>>>>> A few changes to retrieve DAR and DSISR from struct regs
>>>>> instead of retrieving them directly, as they may have
>>>>> changed due to a TLB miss.
>>>>>
>>>>> Also modifies hash_page() and friends to work with virtual
>>>>> data addresses instead of physical ones. Same on load_up_fpu()
>>>>> and load_up_altivec().
>>>>>
>>>>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>>>>
>>>> This patch results in qemu boot failures (mac99 with pmac32_defconfig).
>>>> Images fail silently; there is no console output. Reverting the patch
>>>> fixes the problem. Bisect log is attached below.
>>>>
>>>> Assuming this was tested on real hardware, am I correct to assume
>>>> that qemu
>>>> for ppc32 (more specifically, qemu's mac99 and g3beige machines) no
>>>> longer
>>>> works with the upstream kernel ?
>>>
>>> Before submitting the series, I successfully tested:
>>> - Real HW with powerpc 8xx
>>> - Real HW with powerpc 832x
>>> - Qemu's mac99
>>>
>>> I'll re-check the upstream kernel.
>>>
>>
>> This is still working for me with the upstream kernel:
>>
>
> Interesting. What is your kernel configuration, your qemu version, and
> your qemu command line ?
Config is pmac32_defconfig + CONFIG_DEVTMPFS (But kernel also boots
without CONFIG_DEVTMPFS)
QEMU emulator version 2.11.2
qemu-system-ppc -kernel vmlinux -M mac99 -initrd rootfs.cpio.gz -s -m 1024
Works with both GCC 5.5 and GCC 8.1
Christophe
^ permalink raw reply
* Re: [PATCH v5 17/17] powerpc/32s: Enable CONFIG_VMAP_STACK
From: Guenter Roeck @ 2020-02-07 13:08 UTC (permalink / raw)
To: Christophe Leroy
Cc: linux-mm, linuxppc-dev, Paul Mackerras, linux-kernel, dja
In-Reply-To: <0f866131-4292-a66b-2637-c34139277486@c-s.fr>
On 2/7/20 12:28 AM, Christophe Leroy wrote:
>
>
> On 02/07/2020 06:13 AM, Christophe Leroy wrote:
>>
>>
>> Le 06/02/2020 à 21:31, Guenter Roeck a écrit :
>>> On Sat, Dec 21, 2019 at 08:32:38AM +0000, Christophe Leroy wrote:
>>>> A few changes to retrieve DAR and DSISR from struct regs
>>>> instead of retrieving them directly, as they may have
>>>> changed due to a TLB miss.
>>>>
>>>> Also modifies hash_page() and friends to work with virtual
>>>> data addresses instead of physical ones. Same on load_up_fpu()
>>>> and load_up_altivec().
>>>>
>>>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>>>
>>> This patch results in qemu boot failures (mac99 with pmac32_defconfig).
>>> Images fail silently; there is no console output. Reverting the patch
>>> fixes the problem. Bisect log is attached below.
>>>
>>> Assuming this was tested on real hardware, am I correct to assume that qemu
>>> for ppc32 (more specifically, qemu's mac99 and g3beige machines) no longer
>>> works with the upstream kernel ?
>>
>> Before submitting the series, I successfully tested:
>> - Real HW with powerpc 8xx
>> - Real HW with powerpc 832x
>> - Qemu's mac99
>>
>> I'll re-check the upstream kernel.
>>
>
> This is still working for me with the upstream kernel:
>
Interesting. What is your kernel configuration, your qemu version, and
your qemu command line ?
It works for me with CONFIG_VMAP_STACK=n, but not with pmac32_defconfig.
Thanks,
Guenter
^ permalink raw reply
* Re: [PATCH] powerpc/futex: Fix incorrect user access blocking
From: Daniel Axtens @ 2020-02-07 12:30 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev
In-Reply-To: <20200207122145.11928-1-mpe@ellerman.id.au>
Michael Ellerman <mpe@ellerman.id.au> writes:
> The early versions of our kernel user access prevention (KUAP) were
> written by Russell and Christophe, and didn't have separate
> read/write access.
>
> At some point I picked up the series and added the read/write access,
> but I failed to update the usages in futex.h to correctly allow read
> and write.
>
> However we didn't notice because of another bug which was causing the
> low-level code to always enable read and write. That bug was fixed
> recently in commit 1d8f739b07bd ("powerpc/kuap: Fix set direction in
> allow/prevent_user_access()").
>
> futex_atomic_cmpxchg_inatomic() is passed the user address as %3 and
> does:
>
> 1: lwarx %1, 0, %3
> cmpw 0, %1, %4
> bne- 3f
> 2: stwcx. %5, 0, %3
>
> Which clearly loads and stores from/to %3. The logic in
> arch_futex_atomic_op_inuser() is similar, so fix both of them to use
> allow_read_write_user().
>
> Without this fix, and with PPC_KUAP_DEBUG=y, we see eg:
>
> Bug: Read fault blocked by AMR!
> WARNING: CPU: 94 PID: 149215 at arch/powerpc/include/asm/book3s/64/kup-radix.h:126 __do_page_fault+0x600/0xf30
> CPU: 94 PID: 149215 Comm: futex_requeue_p Tainted: G W 5.5.0-rc7-gcc9x-g4c25df5640ae #1
> ...
> NIP [c000000000070680] __do_page_fault+0x600/0xf30
> LR [c00000000007067c] __do_page_fault+0x5fc/0xf30
> Call Trace:
> [c00020138e5637e0] [c00000000007067c] __do_page_fault+0x5fc/0xf30 (unreliable)
> [c00020138e5638c0] [c00000000000ada8] handle_page_fault+0x10/0x30
> --- interrupt: 301 at cmpxchg_futex_value_locked+0x68/0xd0
> LR = futex_lock_pi_atomic+0xe0/0x1f0
> [c00020138e563bc0] [c000000000217b50] futex_lock_pi_atomic+0x80/0x1f0 (unreliable)
> [c00020138e563c30] [c00000000021b668] futex_requeue+0x438/0xb60
> [c00020138e563d60] [c00000000021c6cc] do_futex+0x1ec/0x2b0
> [c00020138e563d90] [c00000000021c8b8] sys_futex+0x128/0x200
> [c00020138e563e20] [c00000000000b7ac] system_call+0x5c/0x68
>
> Fixes: de78a9c42a79 ("powerpc: Add a framework for Kernel Userspace Access Protection")
Reported-by: syzbot+e808452bad7c375cbee6@syzkaller-ppc64.appspotmail.com
> Cc: stable@vger.kernel.org # v5.2+
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
^ permalink raw reply
* [PATCH] powerpc/futex: Fix incorrect user access blocking
From: Michael Ellerman @ 2020-02-07 12:21 UTC (permalink / raw)
To: linuxppc-dev; +Cc: dja
The early versions of our kernel user access prevention (KUAP) were
written by Russell and Christophe, and didn't have separate
read/write access.
At some point I picked up the series and added the read/write access,
but I failed to update the usages in futex.h to correctly allow read
and write.
However we didn't notice because of another bug which was causing the
low-level code to always enable read and write. That bug was fixed
recently in commit 1d8f739b07bd ("powerpc/kuap: Fix set direction in
allow/prevent_user_access()").
futex_atomic_cmpxchg_inatomic() is passed the user address as %3 and
does:
1: lwarx %1, 0, %3
cmpw 0, %1, %4
bne- 3f
2: stwcx. %5, 0, %3
Which clearly loads and stores from/to %3. The logic in
arch_futex_atomic_op_inuser() is similar, so fix both of them to use
allow_read_write_user().
Without this fix, and with PPC_KUAP_DEBUG=y, we see eg:
Bug: Read fault blocked by AMR!
WARNING: CPU: 94 PID: 149215 at arch/powerpc/include/asm/book3s/64/kup-radix.h:126 __do_page_fault+0x600/0xf30
CPU: 94 PID: 149215 Comm: futex_requeue_p Tainted: G W 5.5.0-rc7-gcc9x-g4c25df5640ae #1
...
NIP [c000000000070680] __do_page_fault+0x600/0xf30
LR [c00000000007067c] __do_page_fault+0x5fc/0xf30
Call Trace:
[c00020138e5637e0] [c00000000007067c] __do_page_fault+0x5fc/0xf30 (unreliable)
[c00020138e5638c0] [c00000000000ada8] handle_page_fault+0x10/0x30
--- interrupt: 301 at cmpxchg_futex_value_locked+0x68/0xd0
LR = futex_lock_pi_atomic+0xe0/0x1f0
[c00020138e563bc0] [c000000000217b50] futex_lock_pi_atomic+0x80/0x1f0 (unreliable)
[c00020138e563c30] [c00000000021b668] futex_requeue+0x438/0xb60
[c00020138e563d60] [c00000000021c6cc] do_futex+0x1ec/0x2b0
[c00020138e563d90] [c00000000021c8b8] sys_futex+0x128/0x200
[c00020138e563e20] [c00000000000b7ac] system_call+0x5c/0x68
Fixes: de78a9c42a79 ("powerpc: Add a framework for Kernel Userspace Access Protection")
Cc: stable@vger.kernel.org # v5.2+
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
arch/powerpc/include/asm/futex.h | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/include/asm/futex.h b/arch/powerpc/include/asm/futex.h
index eea28ca679db..bc7d9d06a6d9 100644
--- a/arch/powerpc/include/asm/futex.h
+++ b/arch/powerpc/include/asm/futex.h
@@ -35,7 +35,7 @@ static inline int arch_futex_atomic_op_inuser(int op, int oparg, int *oval,
{
int oldval = 0, ret;
- allow_write_to_user(uaddr, sizeof(*uaddr));
+ allow_read_write_user(uaddr, uaddr, sizeof(*uaddr));
pagefault_disable();
switch (op) {
@@ -62,7 +62,7 @@ static inline int arch_futex_atomic_op_inuser(int op, int oparg, int *oval,
*oval = oldval;
- prevent_write_to_user(uaddr, sizeof(*uaddr));
+ prevent_read_write_user(uaddr, uaddr, sizeof(*uaddr));
return ret;
}
@@ -76,7 +76,8 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr,
if (!access_ok(uaddr, sizeof(u32)))
return -EFAULT;
- allow_write_to_user(uaddr, sizeof(*uaddr));
+ allow_read_write_user(uaddr, uaddr, sizeof(*uaddr));
+
__asm__ __volatile__ (
PPC_ATOMIC_ENTRY_BARRIER
"1: lwarx %1,0,%3 # futex_atomic_cmpxchg_inatomic\n\
@@ -97,7 +98,8 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr,
: "cc", "memory");
*uval = prev;
- prevent_write_to_user(uaddr, sizeof(*uaddr));
+ prevent_read_write_user(uaddr, uaddr, sizeof(*uaddr));
+
return ret;
}
--
2.21.1
^ permalink raw reply related
* Re: [PATCH v5 01/10] capabilities: introduce CAP_PERFMON to kernel and user space
From: Thomas Gleixner @ 2020-02-07 11:38 UTC (permalink / raw)
To: Alexey Budankov, Stephen Smalley, Serge Hallyn, James Morris
Cc: Mark Rutland, Song Liu, Peter Zijlstra,
joonas.lahtinen@linux.intel.com, Will Deacon, Alexei Starovoitov,
Lionel Landwerlin, Paul Mackerras, Jiri Olsa, Alexei Starovoitov,
Andi Kleen, Igor Lubashev, Alexander Shishkin, Ingo Molnar,
oprofile-list, linux-arm-kernel, Robert Richter,
selinux@vger.kernel.org, intel-gfx@lists.freedesktop.org,
jani.nikula@linux.intel.com, Arnaldo Carvalho de Melo,
rodrigo.vivi@intel.com, Namhyung Kim, Stephane Eranian,
linux-parisc@vger.kernel.org, linux-kernel, Andy Lutomirski,
linux-perf-users@vger.kernel.org,
linux-security-module@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <2b608e26-354b-3df9-aea9-58e56dc0c5e5@linux.intel.com>
Alexey Budankov <alexey.budankov@linux.intel.com> writes:
> On 22.01.2020 17:25, Alexey Budankov wrote:
>> On 22.01.2020 17:07, Stephen Smalley wrote:
>>>> It keeps the implementation simple and readable. The implementation is more
>>>> performant in the sense of calling the API - one capable() call for CAP_PERFMON
>>>> privileged process.
>>>>
>>>> Yes, it bloats audit log for CAP_SYS_ADMIN privileged and unprivileged processes,
>>>> but this bloating also advertises and leverages using more secure CAP_PERFMON
>>>> based approach to use perf_event_open system call.
>>>
>>> I can live with that. We just need to document that when you see
>>> both a CAP_PERFMON and a CAP_SYS_ADMIN audit message for a process,
>>> try only allowing CAP_PERFMON first and see if that resolves the
>>> issue. We have a similar issue with CAP_DAC_READ_SEARCH versus
>>> CAP_DAC_OVERRIDE.
>>
>> perf security [1] document can be updated, at least, to align and document
>> this audit logging specifics.
>
> And I plan to update the document right after this patch set is accepted.
> Feel free to let me know of the places in the kernel docs that also
> require update w.r.t CAP_PERFMON extension.
The documentation update wants be part of the patch set and not planned
to be done _after_ the patch set is merged.
Thanks,
tglx
^ permalink raw reply
* Re: [PATCH 18/18] powerpc/fault: Use analyse_instr() to check for store with updates to sp
From: Greg Kurz @ 2020-02-07 8:15 UTC (permalink / raw)
To: Daniel Axtens; +Cc: Jordan Niethe, linuxppc-dev, alistair
In-Reply-To: <87immdu3je.fsf@dja-thinkpad.axtens.net>
On Thu, 19 Dec 2019 01:11:33 +1100
Daniel Axtens <dja@axtens.net> wrote:
> Jordan Niethe <jniethe5@gmail.com> writes:
>
> > A user-mode access to an address a long way below the stack pointer is
> > only valid if the instruction is one that would update the stack pointer
> > to the address accessed. This is checked by directly looking at the
> > instructions op-code. As a result is does not take into account prefixed
> > instructions. Instead of looking at the instruction our self, use
> > analyse_instr() determine if this a store instruction that will update
> > the stack pointer.
> >
> > Something to note is that there currently are not any store with update
> > prefixed instructions. Actually there is no plan for prefixed
> > update-form loads and stores. So this patch is probably not needed but
> > it might be preferable to use analyse_instr() rather than open coding
> > the test anyway.
>
> Yes please. I was looking through this code recently and was
> horrified. This improves things a lot and I think is justification
> enough as-is.
>
Except it doesn't work... I'm now experiencing a systematic crash of
systemd at boot in my fedora31 guest:
[ 3.322912] systemd[1]: segfault (11) at 7ffff3eaf550 nip 7ce4d42f8d78 lr 9d82c098fc0 code 1 in libsystemd-shared-243.so[7ce4d4150000+2e0000]
[ 3.323112] systemd[1]: code: 00000480 60420000 3c4c001e 3842edb0 7c0802a6 3d81fff0 fb81ffe0 fba1ffe8
[ 3.323244] systemd[1]: code: fbc1fff0 fbe1fff8 f8010010 7c200b78 <f801f001> 7c216000 4082fff8 f801ff71
f801f001 is
0x1a8d78 <serialize_item_format+40>: stdu r0,-4096(r1)
which analyse_instr() is supposed to decode as a STORE that
updates r1 so we should be good... Unfortunately analyse_instr()
forbids partial register sets, since it might return op->val
based on some register content depending on the instruction:
/* Following cases refer to regs->gpr[], so we need all regs */
if (!FULL_REGS(regs))
return -1;
analyse_instr() was introduced with instruction emulation in mind, which
goes far beyond the need we have in store_updates_sp(). Especially the
fault path doesn't care for the register content at all...
Not sure how to cope with that correctly (refactor analyse_instr() ? ) but
until someone comes up with a solution, please don't merge this patch.
Cheers,
--
Greg
> Regards,
> Daniel
> >
> > Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> > ---
> > arch/powerpc/mm/fault.c | 39 +++++++++++----------------------------
> > 1 file changed, 11 insertions(+), 28 deletions(-)
> >
> > diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> > index b5047f9b5dec..cb78b3ca1800 100644
> > --- a/arch/powerpc/mm/fault.c
> > +++ b/arch/powerpc/mm/fault.c
> > @@ -41,37 +41,17 @@
> > #include <asm/siginfo.h>
> > #include <asm/debug.h>
> > #include <asm/kup.h>
> > +#include <asm/sstep.h>
> >
> > /*
> > * Check whether the instruction inst is a store using
> > * an update addressing form which will update r1.
> > */
> > -static bool store_updates_sp(unsigned int inst)
> > +static bool store_updates_sp(struct instruction_op *op)
> > {
> > - /* check for 1 in the rA field */
> > - if (((inst >> 16) & 0x1f) != 1)
> > - return false;
> > - /* check major opcode */
> > - switch (inst >> 26) {
> > - case OP_STWU:
> > - case OP_STBU:
> > - case OP_STHU:
> > - case OP_STFSU:
> > - case OP_STFDU:
> > - return true;
> > - case OP_STD: /* std or stdu */
> > - return (inst & 3) == 1;
> > - case OP_31:
> > - /* check minor opcode */
> > - switch ((inst >> 1) & 0x3ff) {
> > - case OP_31_XOP_STDUX:
> > - case OP_31_XOP_STWUX:
> > - case OP_31_XOP_STBUX:
> > - case OP_31_XOP_STHUX:
> > - case OP_31_XOP_STFSUX:
> > - case OP_31_XOP_STFDUX:
> > + if (GETTYPE(op->type) == STORE) {
> > + if ((op->type & UPDATE) && (op->update_reg == 1))
> > return true;
> > - }
> > }
> > return false;
> > }
> > @@ -278,14 +258,17 @@ static bool bad_stack_expansion(struct pt_regs *regs, unsigned long address,
> >
> > if ((flags & FAULT_FLAG_WRITE) && (flags & FAULT_FLAG_USER) &&
> > access_ok(nip, sizeof(*nip))) {
> > - unsigned int inst;
> > + unsigned int inst, sufx;
> > + struct instruction_op op;
> > int res;
> >
> > pagefault_disable();
> > - res = __get_user_inatomic(inst, nip);
> > + res = __get_user_instr_inatomic(inst, sufx, nip);
> > pagefault_enable();
> > - if (!res)
> > - return !store_updates_sp(inst);
> > + if (!res) {
> > + analyse_instr(&op, uregs, inst, sufx);
> > + return !store_updates_sp(&op);
> > + }
> > *must_retry = true;
> > }
> > return true;
> > --
> > 2.20.1
^ permalink raw reply
* Re: [PATCH 1/1] powerpc/eeh: fix deadlock handling dead PHB
From: Frederic Barrat @ 2020-02-07 8:56 UTC (permalink / raw)
To: Sam Bobroff, linuxppc-dev
In-Reply-To: <0547e82dbf90ee0729a2979a8cac5c91665c621f.1581051445.git.sbobroff@linux.ibm.com>
Le 07/02/2020 à 05:57, Sam Bobroff a écrit :
> Recovering a dead PHB can currently cause a deadlock as the PCI
> rescan/remove lock is taken twice.
>
> This is caused as part of an existing bug in
> eeh_handle_special_event(). The pe is processed while traversing the
> PHBs even though the pe is unrelated to the loop. This causes the pe
> to be, incorrectly, processed more than once.
>
> Untangling this section can move the pe processing out of the loop and
> also outside the locked section, correcting both problems.
>
> Signed-off-by: Sam Bobroff <sbobroff@linux.ibm.com>
> ---
Reviewed-by: Frederic Barrat <fbarrat@linux.ibm.com>
Tested-by: Frederic Barrat <fbarrat@linux.ibm.com>
I think it also needs:
Fixes: 2e25505147b8 ("powerpc/eeh: Fix crash when edev->pdev changes")
Cc: stable@vger.kernel.org # 5.4+
Fred
> I have only compile tested this fix, Frederic Barrat (who discovered it) has
> offered to test it (thanks!).
>
> arch/powerpc/kernel/eeh_driver.c | 21 +++++++++++----------
> 1 file changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/arch/powerpc/kernel/eeh_driver.c b/arch/powerpc/kernel/eeh_driver.c
> index 3dd1a422fc29..d6e75a8a14ce 100644
> --- a/arch/powerpc/kernel/eeh_driver.c
> +++ b/arch/powerpc/kernel/eeh_driver.c
> @@ -1190,6 +1190,17 @@ void eeh_handle_special_event(void)
> eeh_pe_state_mark(pe, EEH_PE_RECOVERING);
> eeh_handle_normal_event(pe);
> } else {
> + eeh_for_each_pe(pe, tmp_pe)
> + eeh_pe_for_each_dev(tmp_pe, edev, tmp_edev)
> + edev->mode &= ~EEH_DEV_NO_HANDLER;
> +
> + /* Notify all devices to be down */
> + eeh_pe_state_clear(pe, EEH_PE_PRI_BUS, true);
> + eeh_set_channel_state(pe, pci_channel_io_perm_failure);
> + eeh_pe_report(
> + "error_detected(permanent failure)", pe,
> + eeh_report_failure, NULL);
> +
> pci_lock_rescan_remove();
> list_for_each_entry(hose, &hose_list, list_node) {
> phb_pe = eeh_phb_pe_get(hose);
> @@ -1198,16 +1209,6 @@ void eeh_handle_special_event(void)
> (phb_pe->state & EEH_PE_RECOVERING))
> continue;
>
> - eeh_for_each_pe(pe, tmp_pe)
> - eeh_pe_for_each_dev(tmp_pe, edev, tmp_edev)
> - edev->mode &= ~EEH_DEV_NO_HANDLER;
> -
> - /* Notify all devices to be down */
> - eeh_pe_state_clear(pe, EEH_PE_PRI_BUS, true);
> - eeh_set_channel_state(pe, pci_channel_io_perm_failure);
> - eeh_pe_report(
> - "error_detected(permanent failure)", pe,
> - eeh_report_failure, NULL);
> bus = eeh_pe_bus_get(phb_pe);
> if (!bus) {
> pr_err("%s: Cannot find PCI bus for "
>
^ permalink raw reply
* Re: [PATCH v5 17/17] powerpc/32s: Enable CONFIG_VMAP_STACK
From: Christophe Leroy @ 2020-02-07 8:28 UTC (permalink / raw)
To: Guenter Roeck; +Cc: linux-mm, linuxppc-dev, Paul Mackerras, linux-kernel, dja
In-Reply-To: <c6285f2a-f8f5-0d97-2d80-061da1f1a7fc@c-s.fr>
On 02/07/2020 06:13 AM, Christophe Leroy wrote:
>
>
> Le 06/02/2020 à 21:31, Guenter Roeck a écrit :
>> On Sat, Dec 21, 2019 at 08:32:38AM +0000, Christophe Leroy wrote:
>>> A few changes to retrieve DAR and DSISR from struct regs
>>> instead of retrieving them directly, as they may have
>>> changed due to a TLB miss.
>>>
>>> Also modifies hash_page() and friends to work with virtual
>>> data addresses instead of physical ones. Same on load_up_fpu()
>>> and load_up_altivec().
>>>
>>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>>
>> This patch results in qemu boot failures (mac99 with pmac32_defconfig).
>> Images fail silently; there is no console output. Reverting the patch
>> fixes the problem. Bisect log is attached below.
>>
>> Assuming this was tested on real hardware, am I correct to assume that
>> qemu
>> for ppc32 (more specifically, qemu's mac99 and g3beige machines) no
>> longer
>> works with the upstream kernel ?
>
> Before submitting the series, I successfully tested:
> - Real HW with powerpc 8xx
> - Real HW with powerpc 832x
> - Qemu's mac99
>
> I'll re-check the upstream kernel.
>
This is still working for me with the upstream kernel:
>> =============================================================
>> OpenBIOS 1.1 [Oct 19 2017 07:00]
>> Configuration device id QEMU version 1 machine id 1
>> CPUs: 1
>> Memory: 1024M
>> UUID: 00000000-0000-0000-0000-000000000000
>> CPU type PowerPC,G4
milliseconds isn't unique.
Welcome to OpenBIOS v1.1 built on Oct 19 2017 07:00
>> [ppc] Kernel already loaded (0x01000000 + 0x00902a90) (initrd
0x01a03000 + 0x001d1a3b)
>> [ppc] Kernel command line: early_ioremap_debug console=ttyS0
OF stdout device is: /pci@f2000000/mac-io@c/escc@13000/ch-a@13020
Preparing to boot Linux version 5.5.0+ (root@pc16570vm.idsi0.si.c-s.fr)
(gcc version 5.5.0 (GCC)) #3111 Fri Feb 7 07:23:08 UTC 2020
Detected machine type: 00000400
command line:
memory layout at init:
memory_limit : 00000000 (16 MB aligned)
alloc_bottom : 01bd5000
alloc_top : 30000000
alloc_top_hi : 40000000
rmo_top : 30000000
ram_top : 40000000
copying OF device tree...
Building dt strings...
Building dt structure...
Device tree strings 0x01bd6000 -> 0x01bd50a4
Device tree struct 0x01bd7000 -> 0x3fde7ef8
Quiescing Open Firmware ...
Booting Linux via __start() @ 0x01000000 ...
Hello World !
Total memory = 1024MB; using 2048kB for hash table
Activating Kernel Userspace Execution Prevention
Activating Kernel Userspace Access Protection
Linux version 5.5.0+ (root@pc16570vm.idsi0.si.c-s.fr) (gcc version 5.5.0
(GCC)) #3111 Fri Feb 7 07:23:08 UTC 2020
Found initrd at 0xc1a03000:0xc1bd4a3b
ioremap() called early from pmac_feature_init+0xd8/0xad0. Use
early_ioremap() instead
Found UniNorth memory controller & host bridge @ 0xf8000000 revision:
0xa2cfbc60
Mapped at 0xff3c0000
ioremap() called early from probe_one_macio+0x124/0x228. Use
early_ioremap() instead
Found a Keylargo mac-io controller, rev: 0, mapped at 0x(ptrval)
PowerMac motherboard: PowerMac G4 AGP Graphics
ioremap() called early from udbg_scc_init+0x164/0x35c. Use
early_ioremap() instead
boot stdout isn't a display !
ioremap() called early from find_via_cuda+0x9c/0x3d4. Use
early_ioremap() instead
Using PowerMac machine description
printk: bootconsole [udbg0] enabled
-----------------------------------------------------
phys_mem_size = 0x40000000
dcache_bsize = 0x20
icache_bsize = 0x20
cpu_features = 0x000000000401a00a
possible = 0x00000000277de14b
always = 0x0000000000000000
cpu_user_features = 0x9c000001 0x00000000
mmu_features = 0x00000001
Hash_size = 0x200000
Hash_mask = 0x7fff
-----------------------------------------------------
ioremap() called early from pmac_pci_init+0x1b8/0x6e4. Use
early_ioremap() instead
ioremap() called early from pmac_pci_init+0x1d0/0x6e4. Use
early_ioremap() instead
Found UniNorth PCI host bridge at 0x00000000f2000000. Firmware bus
number: 0->0
PCI host bridge /pci@f2000000 (primary) ranges:
IO 0x00000000f2000000..0x00000000f27fffff -> 0x0000000000000000
ioremap() called early from pci_process_bridge_OF_ranges+0x1e4/0x2c0.
Use early_ioremap() instead
MEM 0x0000000080000000..0x000000008fffffff -> 0x0000000080000000
ioremap() called early from pmac_setup_arch+0x10c/0x294. Use
early_ioremap() instead
ioremap() called early from pmac_nvram_init+0x140/0x4f8. Use
early_ioremap() instead
nvram: Checking bank 0...
Invalid signature
Invalid checksum
nvram: gen0=0, gen1=0
nvram: Active bank is: 0
nvram: OF partition at 0xffffffff
nvram: XP partition at 0xffffffff
nvram: NR partition at 0xffffffff
Zone ranges:
DMA [mem 0x0000000000000000-0x000000002fffffff]
Normal empty
HighMem [mem 0x0000000030000000-0x000000003fffffff]
Movable zone start for each node
Early memory node ranges
node 0: [mem 0x0000000000000000-0x000000003fffffff]
Initmem setup node 0 [mem 0x0000000000000000-0x000000003fffffff]
Built 1 zonelists, mobility grouping on. Total pages: 260608
Kernel command line: early_ioremap_debug console=ttyS0
Dentry cache hash table entries: 131072 (order: 7, 524288 bytes, linear)
Inode-cache hash table entries: 65536 (order: 6, 262144 bytes, linear)
mem auto-init: stack:off, heap alloc:off, heap free:off
Memory: 1026364K/1048576K available (6476K kernel code, 684K rwdata,
1612K rodata, 268K init, 186K bss, 22212K reserved, 0K cma-reserved,
262144K highmem)
Kernel virtual memory layout:
* 0xffbcf000..0xfffff000 : fixmap
* 0xff400000..0xff800000 : highmem PTEs
* 0xfeb36000..0xff400000 : early ioremap
* 0xf1000000..0xfeb36000 : vmalloc & ioremap
SLUB: HWalign=32, Order=0-3, MinObjects=0, CPUs=1, Nodes=1
NR_IRQS: 512, nr_irqs: 512, preallocated irqs: 16
mpic: Setting up MPIC " MPIC 1 " version 1.2 at 80040000, max 1 CPUs
mpic: ISU size: 64, shift: 6, mask: 3f
mpic: Initializing for 64 sources
GMT Delta read from XPRAM: 0 minutes, DST: on
clocksource: timebase: mask: 0xffffffffffffffff max_cycles:
0x171024e7e0, max_idle_ns: 440795205315 ns
clocksource: timebase mult[a000000] shift[24] registered
Console: colour dummy device 80x25
pid_max: default: 32768 minimum: 301
Mount-cache hash table entries: 2048 (order: 1, 8192 bytes, linear)
Mountpoint-cache hash table entries: 2048 (order: 1, 8192 bytes, linear)
devtmpfs: initialized
random: get_random_u32 called from bucket_table_alloc.isra.27+0x64/0x188
with crng_init=0
clocksource: jiffies: mask: 0xffffffff max_cycles: 0xffffffff,
max_idle_ns: 7645041785100000 ns
futex hash table entries: 256 (order: -1, 3072 bytes, linear)
NET: Registered protocol family 16
PCI: Probing PCI hardware
PCI host bridge to bus 0000:00
pci_bus 0000:00: root bus resource [io 0x0000-0x7fffff]
pci_bus 0000:00: root bus resource [mem 0x80000000-0x8fffffff]
pci_bus 0000:00: root bus resource [bus 00-ff]
pci_bus 0000:00: busn_res: [bus 00-ff] end is updated to ff
pci 0000:00:0b.0: [106b:0020] type 00 class 0x060000
pci 0000:00:0c.0: [106b:0022] type 00 class 0xff0000
pci 0000:00:0c.0: reg 0x10: [mem 0x80000000-0x8007ffff]
pci 0000:00:0d.0: [106b:003f] type 00 class 0x0c0310
pci 0000:00:0d.0: reg 0x10: [mem 0x80080000-0x800800ff]
pci 0000:00:0e.0: [10ec:8029] type 00 class 0x020000
pci 0000:00:0e.0: reg 0x10: [io 0x1000-0x10ff]
pci 0000:00:0e.0: reg 0x30: [mem 0x800c0000-0x800fffff pref]
pci_bus 0000:00: busn_res: [bus 00-ff] end is updated to 00
pci_bus 0000:00: resource 4 [io 0x0000-0x7fffff]
pci_bus 0000:00: resource 5 [mem 0x80000000-0x8fffffff]
vgaarb: loaded
SCSI subsystem initialized
usbcore: registered new interface driver usbfs
usbcore: registered new interface driver hub
usbcore: registered new device driver usb
clocksource: Switched to clocksource timebase
NET: Registered protocol family 2
tcp_listen_portaddr_hash hash table entries: 512 (order: 0, 4096 bytes,
linear)
TCP established hash table entries: 8192 (order: 3, 32768 bytes, linear)
TCP bind hash table entries: 8192 (order: 3, 32768 bytes, linear)
TCP: Hash tables configured (established 8192 bind 8192)
UDP hash table entries: 512 (order: 1, 8192 bytes, linear)
UDP-Lite hash table entries: 512 (order: 1, 8192 bytes, linear)
NET: Registered protocol family 1
RPC: Registered named UNIX socket transport module.
RPC: Registered udp transport module.
RPC: Registered tcp transport module.
RPC: Registered tcp NFSv4.1 backchannel transport module.
PCI: CLS 0 bytes, default 32
Trying to unpack rootfs image as initramfs...
Freeing initrd memory: 1860K
Initialise system trusted keyrings
workingset: timestamp_bits=30 max_order=18 bucket_order=0
NFS: Registering the id_resolver key type
Key type id_resolver registered
Key type id_legacy registered
Key type asymmetric registered
Asymmetric key parser 'x509' registered
bounce: pool size: 64 pages
Block layer SCSI generic (bsg) driver version 0.4 loaded (major 253)
io scheduler mq-deadline registered
io scheduler kyber registered
Non-volatile memory driver v1.3
brd: module loaded
loop: module loaded
MacIO PCI driver attached to Keylargo chipset
Macintosh Cuda and Egret driver.
mesh: configured for synchronous 5 MB/s
st: Version 20160209, fixed bufsize 32768, s/g segs 256
Detected ADB keyboard, type ANSI.
input: ADB keyboard as /devices/virtual/input/input0
random: fast init done
input: ADB mouse as /devices/virtual/input/input1
pata-macio 0.00020000:ata-3: Activating pata-macio chipset KeyLargo
ATA-3, Apple bus ID 0
scsi host0: pata_macio
ata1: PATA max MWDMA2 irq 16
pata-macio 0.00021000:ata-3: Activating pata-macio chipset KeyLargo
ATA-3, Apple bus ID 1
scsi host1: pata_macio
ata2: PATA max MWDMA2 irq 18
pcnet32: pcnet32.c:v1.35 21.Apr.2008 tsbogend@alpha.franken.de
PPP generic driver version 2.4.2
PPP Deflate Compression module registered
ohci_hcd: USB 1.1 'Open' Host Controller (OHCI) Driver
ohci-pci: OHCI PCI platform driver
ohci-pci 0000:00:0d.0: OHCI PCI host controller
ohci-pci 0000:00:0d.0: new USB bus registered, assigned bus number 1
ohci-pci 0000:00:0d.0: irq 28, io mem 0x80080000
hub 1-0:1.0: USB hub found
hub 1-0:1.0: 3 ports detected
usbcore: registered new interface driver appletouch
rtc-generic rtc-generic: registered as rtc0
APM Battery Driver
usbcore: registered new interface driver usbhid
usbhid: USB HID core driver
oprofile: using timer interrupt.
Initializing XFRM netlink socket
NET: Registered protocol family 17
NET: Registered protocol family 15
Key type dns_resolver registered
drmem: No dynamic reconfiguration memory found
Loading compiled-in X.509 certificates
rtc-generic rtc-generic: setting system clock to 2020-02-07T07:46:01 UTC
(1581061561)
Warning: unable to open an initial console.
ata2.00: ATAPI: QEMU DVD-ROM, 2.5+, max UDMA/100
scsi 1:0:0:0: CD-ROM QEMU QEMU DVD-ROM 2.5+ PQ: 0 ANSI: 5
sr 1:0:0:0: [sr0] scsi3-mmc drive: 4x/4x cd/rw xa/form2 tray
cdrom: Uniform CD-ROM driver Revision: 3.20
sr 1:0:0:0: Attached scsi generic sg0 type 5
Freeing unused kernel memory: 268K
This architecture does not have kernel memory protection.
Run /init as init process
Christophe
^ permalink raw reply
* Re: [PATCH v6 02/11] mm/gup: Use functions to track lockless pgtbl walks on gup_pgd_range
From: kbuild test robot @ 2020-02-07 8:01 UTC (permalink / raw)
To: Leonardo Bras
Cc: linux-mm, Paul Mackerras, Mike Rapoport, linux-arch,
Aneesh Kumar K.V, Steven Price, Mahesh Salgaonkar, Leonardo Bras,
Michal Suchanek, Arnd Bergmann, Robin Murphy, Nicholas Piggin,
kvm-ppc, Thomas Gleixner, Reza Arbab, Allison Randal, kbuild-all,
Greg Kroah-Hartman, linux-kernel, Andrew Morton, linuxppc-dev
In-Reply-To: <20200206030900.147032-3-leonardo@linux.ibm.com>
[-- Attachment #1: Type: text/plain, Size: 3299 bytes --]
Hi Leonardo,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on powerpc/next]
[also build test ERROR on asm-generic/master paulus-powerpc/kvm-ppc-next linus/master v5.5 next-20200207]
[cannot apply to kvm-ppc/kvm-ppc-next]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Leonardo-Bras/Introduces-new-functions-for-tracking-lockless-pagetable-walks/20200207-071035
base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: mips-randconfig-a001-20200207 (attached as .config)
compiler: mipsel-linux-gcc (GCC) 5.5.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=5.5.0 make.cross ARCH=mips
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
mm/gup.c: In function 'get_user_pages_fast':
>> mm/gup.c:2435:27: error: 'IRQS_ENABLED' undeclared (first use in this function)
end_lockless_pgtbl_walk(IRQS_ENABLED);
^
mm/gup.c:2435:27: note: each undeclared identifier is reported only once for each function it appears in
vim +/IRQS_ENABLED +2435 mm/gup.c
2395
2396 /**
2397 * get_user_pages_fast() - pin user pages in memory
2398 * @start: starting user address
2399 * @nr_pages: number of pages from start to pin
2400 * @gup_flags: flags modifying pin behaviour
2401 * @pages: array that receives pointers to the pages pinned.
2402 * Should be at least nr_pages long.
2403 *
2404 * Attempt to pin user pages in memory without taking mm->mmap_sem.
2405 * If not successful, it will fall back to taking the lock and
2406 * calling get_user_pages().
2407 *
2408 * Returns number of pages pinned. This may be fewer than the number
2409 * requested. If nr_pages is 0 or negative, returns 0. If no pages
2410 * were pinned, returns -errno.
2411 */
2412 int get_user_pages_fast(unsigned long start, int nr_pages,
2413 unsigned int gup_flags, struct page **pages)
2414 {
2415 unsigned long addr, len, end;
2416 int nr = 0, ret = 0;
2417
2418 if (WARN_ON_ONCE(gup_flags & ~(FOLL_WRITE | FOLL_LONGTERM)))
2419 return -EINVAL;
2420
2421 start = untagged_addr(start) & PAGE_MASK;
2422 addr = start;
2423 len = (unsigned long) nr_pages << PAGE_SHIFT;
2424 end = start + len;
2425
2426 if (end <= start)
2427 return 0;
2428 if (unlikely(!access_ok((void __user *)start, len)))
2429 return -EFAULT;
2430
2431 if (IS_ENABLED(CONFIG_HAVE_FAST_GUP) &&
2432 gup_fast_permitted(start, end)) {
2433 begin_lockless_pgtbl_walk();
2434 gup_pgd_range(addr, end, gup_flags, pages, &nr);
> 2435 end_lockless_pgtbl_walk(IRQS_ENABLED);
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 24349 bytes --]
^ permalink raw reply
* Re: [PATCH] powerpc/vdso32: mark __kernel_datapage_offset as STV_PROTECTED
From: Nathan Chancellor @ 2020-02-07 6:42 UTC (permalink / raw)
To: Christophe Leroy
Cc: Fangrui Song, linux-kernel, clang-built-linux, Paul Mackerras,
linuxppc-dev
In-Reply-To: <10e3d362-ec29-3816-88ff-8415d5c78e3b@c-s.fr>
On Wed, Feb 05, 2020 at 07:25:59AM +0100, Christophe Leroy wrote:
>
>
> Le 05/02/2020 à 01:50, Fangrui Song a écrit :
> > A PC-relative relocation (R_PPC_REL16_LO in this case) referencing a
> > preemptible symbol in a -shared link is not allowed. GNU ld's powerpc
> > port is permissive and allows it [1], but lld will report an error after
> > https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git/commit/?id=ec0895f08f99515194e9fcfe1338becf6f759d38
>
> Note that there is a series whose first two patches aim at dropping
> __kernel_datapage_offset . See
> https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=156045 and
> especially patches https://patchwork.ozlabs.org/patch/1231467/ and
> https://patchwork.ozlabs.org/patch/1231461/
>
> Those patches can be applied independentely of the rest.
>
> Christophe
If that is the case, it would be nice if those could be fast tracked to
5.6 because as it stands now, all PowerPC builds that were working with
ld.lld are now broken. Either that or take this patch and rebase that
series on this one.
Cheers,
Nathan
^ permalink raw reply
* Re: [PATCH v5 17/17] powerpc/32s: Enable CONFIG_VMAP_STACK
From: Christophe Leroy @ 2020-02-07 6:13 UTC (permalink / raw)
To: Guenter Roeck; +Cc: linux-kernel, linux-mm, Paul Mackerras, linuxppc-dev, dja
In-Reply-To: <20200206203146.GA23248@roeck-us.net>
Le 06/02/2020 à 21:31, Guenter Roeck a écrit :
> On Sat, Dec 21, 2019 at 08:32:38AM +0000, Christophe Leroy wrote:
>> A few changes to retrieve DAR and DSISR from struct regs
>> instead of retrieving them directly, as they may have
>> changed due to a TLB miss.
>>
>> Also modifies hash_page() and friends to work with virtual
>> data addresses instead of physical ones. Same on load_up_fpu()
>> and load_up_altivec().
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>
> This patch results in qemu boot failures (mac99 with pmac32_defconfig).
> Images fail silently; there is no console output. Reverting the patch
> fixes the problem. Bisect log is attached below.
>
> Assuming this was tested on real hardware, am I correct to assume that qemu
> for ppc32 (more specifically, qemu's mac99 and g3beige machines) no longer
> works with the upstream kernel ?
Before submitting the series, I successfully tested:
- Real HW with powerpc 8xx
- Real HW with powerpc 832x
- Qemu's mac99
I'll re-check the upstream kernel.
In the mean time, you can still unselect CONFIG_VMAP_STACK in your config.
Christophe
^ permalink raw reply
* Re: [PATCH V5 06/14] powerpc/vas: Setup thread IRQ handler per VAS instance
From: Michael Neuling @ 2020-02-07 5:57 UTC (permalink / raw)
To: Haren Myneni, mpe; +Cc: herbert, npiggin, hch, oohall, sukadev, linuxppc-dev
In-Reply-To: <1579680639.26081.31.camel@hbabu-laptop>
> /*
> + * Process CRBs that we receive on the fault window.
> + */
> +irqreturn_t vas_fault_handler(int irq, void *data)
> +{
> + struct vas_instance *vinst = data;
> + struct coprocessor_request_block buf, *crb;
> + struct vas_window *window;
> + void *fifo;
> +
> + /*
> + * VAS can interrupt with multiple page faults. So process all
> + * valid CRBs within fault FIFO until reaches invalid CRB.
> + * NX updates nx_fault_stamp in CRB and pastes in fault FIFO.
> + * kernel retrives send window from parition send window ID
> + * (pswid) in nx_fault_stamp. So pswid should be non-zero and
> + * use this to check whether CRB is valid.
> + * After reading CRB entry, it is reset with 0's in fault FIFO.
> + *
> + * In case kernel receives another interrupt with different page
> + * fault and CRBs are processed by the previous handling, will be
> + * returned from this function when it sees invalid CRB (means 0's).
> + */
> + do {
> + mutex_lock(&vinst->mutex);
This isn't going to work.
From Documentation/locking/mutex-design.rst
- Mutexes may not be used in hardware or software interrupt
contexts such as tasklets and timers.
Mikey
^ permalink raw reply
* Re: [PATCH V5 09/14] powerpc/vas: Update CSB and notify process for fault CRBs
From: Michael Neuling @ 2020-02-07 5:46 UTC (permalink / raw)
To: Haren Myneni, mpe; +Cc: herbert, npiggin, hch, oohall, sukadev, linuxppc-dev
In-Reply-To: <1579681061.26081.48.camel@hbabu-laptop>
On Wed, 2020-01-22 at 00:17 -0800, Haren Myneni wrote:
> For each fault CRB, update fault address in CRB (fault_storage_addr)
> and translation error status in CSB so that user space can touch the
> fault address and resend the request. If the user space passed invalid
> CSB address send signal to process with SIGSEGV.
>
> Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
> Signed-off-by: Haren Myneni <haren@linux.ibm.com>
> ---
> arch/powerpc/platforms/powernv/vas-fault.c | 116
> +++++++++++++++++++++++++++++
> 1 file changed, 116 insertions(+)
>
> diff --git a/arch/powerpc/platforms/powernv/vas-fault.c
> b/arch/powerpc/platforms/powernv/vas-fault.c
> index 5c2cada..2cfab0c 100644
> --- a/arch/powerpc/platforms/powernv/vas-fault.c
> +++ b/arch/powerpc/platforms/powernv/vas-fault.c
> @@ -11,6 +11,7 @@
> #include <linux/slab.h>
> #include <linux/uaccess.h>
> #include <linux/kthread.h>
> +#include <linux/sched/signal.h>
> #include <linux/mmu_context.h>
> #include <asm/icswx.h>
>
> @@ -26,6 +27,120 @@
> #define VAS_FAULT_WIN_FIFO_SIZE (4 << 20)
>
> /*
> + * Update the CSB to indicate a translation error.
> + *
> + * If the fault is in the CSB address itself or if we are unable to
> + * update the CSB, send a signal to the process, because we have no
> + * other way of notifying the user process.
> + *
> + * Remaining settings in the CSB are based on wait_for_csb() of
> + * NX-GZIP.
> + */
> +static void update_csb(struct vas_window *window,
> + struct coprocessor_request_block *crb)
> +{
> + int rc;
> + struct pid *pid;
> + void __user *csb_addr;
> + struct task_struct *tsk;
> + struct kernel_siginfo info;
> + struct coprocessor_status_block csb;
> +
> + /*
> + * NX user space windows can not be opened for task->mm=NULL
> + * and faults will not be generated for kernel requests.
> + */
> + if (!window->mm || !window->user_win)
> + return;
> +
> + csb_addr = (void *)be64_to_cpu(crb->csb_addr);
> +
> + csb.cc = CSB_CC_TRANSLATION;
> + csb.ce = CSB_CE_TERMINATION;
> + csb.cs = 0;
> + csb.count = 0;
> +
> + /*
> + * Returns the fault address in CPU format since it is passed with
> + * signal. But if the user space expects BE format, need changes.
> + * i.e either kernel (here) or user should convert to CPU format.
> + * Not both!
> + */
> + csb.address = be64_to_cpu(crb->stamp.nx.fault_storage_addr);
This looks wrong and I don't understand the comment. You need to convert this
back to be64 to write it to csb.address. ie.
csb.address = cpu_to_be64(be64_to_cpu(crb->stamp.nx.fault_storage_addr));
Which I think you can just avoid the endian conversion all together.
> + csb.flags = 0;
> +
> + pid = window->pid;
> + tsk = get_pid_task(pid, PIDTYPE_PID);
> + /*
> + * Send window will be closed after processing all NX requests
> + * and process exits after closing all windows. In multi-thread
> + * applications, thread may not exists, but does not close FD
> + * (means send window) upon exit. Parent thread (tgid) can use
> + * and close the window later.
> + * pid and mm references are taken when window is opened by
> + * process (pid). So tgid is used only when child thread opens
> + * a window and exits without closing it in multithread tasks.
> + */
> + if (!tsk) {
> + pid = window->tgid;
> + tsk = get_pid_task(pid, PIDTYPE_PID);
> + /*
> + * Parent thread will be closing window during its exit.
> + * So should not get here.
> + */
> + if (!tsk)
> + return;
> + }
> +
> + /* Return if the task is exiting. */
> + if (tsk->flags & PF_EXITING) {
> + put_task_struct(tsk);
> + return;
> + }
> +
> + use_mm(window->mm);
> + rc = copy_to_user(csb_addr, &csb, sizeof(csb));
> + /*
> + * User space polls on csb.flags (first byte). So add barrier
> + * then copy first byte with csb flags update.
> + */
> + smp_mb();
> + if (!rc) {
> + csb.flags = CSB_V;
> + rc = copy_to_user(csb_addr, &csb, sizeof(u8));
> + }
> + unuse_mm(window->mm);
> + put_task_struct(tsk);
> +
> + /* Success */
> + if (!rc)
> + return;
> +
> + pr_err("Invalid CSB address 0x%p signalling pid(%d)\n",
> + csb_addr, pid_vnr(pid));
This is a userspace error, not a kernel error. This should not be a pr_err().
Userspace could spam the console with this.
> +
> + clear_siginfo(&info);
> + info.si_signo = SIGSEGV;
> + info.si_errno = EFAULT;
> + info.si_code = SEGV_MAPERR;
> + info.si_addr = csb_addr;
> +
> + /*
> + * process will be polling on csb.flags after request is sent to
> + * NX. So generally CSB update should not fail except when an
> + * application does not follow the process properly. So an error
> + * message will be displayed and leave it to user space whether
> + * to ignore or handle this signal.
> + */
> + rcu_read_lock();
> + rc = kill_pid_info(SIGSEGV, &info, pid);
> + rcu_read_unlock();
why the rcu_read_un/lock() here?
> +
> + pr_devel("%s(): pid %d kill_proc_info() rc %d\n", __func__,
> + pid_vnr(pid), rc);
> +}
> +
> +/*
> * Process CRBs that we receive on the fault window.
> */
> irqreturn_t vas_fault_handler(int irq, void *data)
> @@ -104,6 +219,7 @@ irqreturn_t vas_fault_handler(int irq, void *data)
> return IRQ_HANDLED;
> }
>
> + update_csb(window, crb);
> } while (true);
>
> return IRQ_HANDLED;
^ permalink raw reply
* Re: [PATCH v6 01/11] asm-generic/pgtable: Adds generic functions to track lockless pgtable walks
From: kbuild test robot @ 2020-02-07 5:39 UTC (permalink / raw)
To: Leonardo Bras
Cc: linux-mm, Paul Mackerras, Mike Rapoport, linux-arch,
Aneesh Kumar K.V, Steven Price, Mahesh Salgaonkar, Leonardo Bras,
Michal Suchanek, Arnd Bergmann, Robin Murphy, Nicholas Piggin,
kvm-ppc, Thomas Gleixner, Reza Arbab, Allison Randal, kbuild-all,
Greg Kroah-Hartman, linux-kernel, Andrew Morton, linuxppc-dev
In-Reply-To: <20200206030900.147032-2-leonardo@linux.ibm.com>
[-- Attachment #1: Type: text/plain, Size: 3443 bytes --]
Hi Leonardo,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on powerpc/next]
[also build test ERROR on asm-generic/master paulus-powerpc/kvm-ppc-next linus/master v5.5 next-20200207]
[cannot apply to kvm-ppc/kvm-ppc-next]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Leonardo-Bras/Introduces-new-functions-for-tracking-lockless-pagetable-walks/20200207-071035
base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: xtensa-common_defconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 7.5.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.5.0 make.cross ARCH=xtensa
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
include/asm-generic/pgtable.h: Assembler messages:
include/asm-generic/pgtable.h:1230: Error: unknown opcode or format name 'static'
include/asm-generic/pgtable.h:1231: Error: unknown opcode or format name 'unsigned'
include/asm-generic/pgtable.h:1233: Error: unknown opcode or format name 'unsigned'
include/asm-generic/pgtable.h:1240: Error: unknown opcode or format name 'local_irq_save'
include/asm-generic/pgtable.h:1248: Error: unknown opcode or format name 'smp_mb'
include/asm-generic/pgtable.h:1250: Error: unknown opcode or format name 'return'
>> include/asm-generic/pgtable.h:1251: Error: couldn't find a valid instruction format
ops were:
include/asm-generic/pgtable.h:1257: Error: unknown opcode or format name 'static'
include/asm-generic/pgtable.h:1265: Error: unknown opcode or format name 'smp_mb'
include/asm-generic/pgtable.h:1272: Error: unknown opcode or format name 'local_irq_restore'
include/asm-generic/pgtable.h:1273: Error: couldn't find a valid instruction format
ops were:
vim +1251 include/asm-generic/pgtable.h
1224
1225 #ifndef __HAVE_ARCH_LOCKLESS_PGTBL_WALK_CONTROL
1226 /*
1227 * begin_lockless_pgtbl_walk: Must be inserted before a function call that does
1228 * lockless pagetable walks, such as __find_linux_pte()
1229 */
1230 static inline
1231 unsigned long begin_lockless_pgtbl_walk(void)
1232 {
1233 unsigned long irq_mask;
1234
1235 /*
1236 * Interrupts must be disabled during the lockless page table walk.
1237 * That's because the deleting or splitting involves flushing TLBs,
1238 * which in turn issues interrupts, that will block when disabled.
1239 */
1240 local_irq_save(irq_mask);
1241
1242 /*
1243 * This memory barrier pairs with any code that is either trying to
1244 * delete page tables, or split huge pages. Without this barrier,
1245 * the page tables could be read speculatively outside of interrupt
1246 * disabling.
1247 */
1248 smp_mb();
1249
1250 return irq_mask;
> 1251 }
1252
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 10561 bytes --]
^ permalink raw reply
* [PATCH 1/1] powerpc/eeh: fix deadlock handling dead PHB
From: Sam Bobroff @ 2020-02-07 4:57 UTC (permalink / raw)
To: linuxppc-dev; +Cc: fbarrat
Recovering a dead PHB can currently cause a deadlock as the PCI
rescan/remove lock is taken twice.
This is caused as part of an existing bug in
eeh_handle_special_event(). The pe is processed while traversing the
PHBs even though the pe is unrelated to the loop. This causes the pe
to be, incorrectly, processed more than once.
Untangling this section can move the pe processing out of the loop and
also outside the locked section, correcting both problems.
Signed-off-by: Sam Bobroff <sbobroff@linux.ibm.com>
---
I have only compile tested this fix, Frederic Barrat (who discovered it) has
offered to test it (thanks!).
arch/powerpc/kernel/eeh_driver.c | 21 +++++++++++----------
1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/arch/powerpc/kernel/eeh_driver.c b/arch/powerpc/kernel/eeh_driver.c
index 3dd1a422fc29..d6e75a8a14ce 100644
--- a/arch/powerpc/kernel/eeh_driver.c
+++ b/arch/powerpc/kernel/eeh_driver.c
@@ -1190,6 +1190,17 @@ void eeh_handle_special_event(void)
eeh_pe_state_mark(pe, EEH_PE_RECOVERING);
eeh_handle_normal_event(pe);
} else {
+ eeh_for_each_pe(pe, tmp_pe)
+ eeh_pe_for_each_dev(tmp_pe, edev, tmp_edev)
+ edev->mode &= ~EEH_DEV_NO_HANDLER;
+
+ /* Notify all devices to be down */
+ eeh_pe_state_clear(pe, EEH_PE_PRI_BUS, true);
+ eeh_set_channel_state(pe, pci_channel_io_perm_failure);
+ eeh_pe_report(
+ "error_detected(permanent failure)", pe,
+ eeh_report_failure, NULL);
+
pci_lock_rescan_remove();
list_for_each_entry(hose, &hose_list, list_node) {
phb_pe = eeh_phb_pe_get(hose);
@@ -1198,16 +1209,6 @@ void eeh_handle_special_event(void)
(phb_pe->state & EEH_PE_RECOVERING))
continue;
- eeh_for_each_pe(pe, tmp_pe)
- eeh_pe_for_each_dev(tmp_pe, edev, tmp_edev)
- edev->mode &= ~EEH_DEV_NO_HANDLER;
-
- /* Notify all devices to be down */
- eeh_pe_state_clear(pe, EEH_PE_PRI_BUS, true);
- eeh_set_channel_state(pe, pci_channel_io_perm_failure);
- eeh_pe_report(
- "error_detected(permanent failure)", pe,
- eeh_report_failure, NULL);
bus = eeh_pe_bus_get(phb_pe);
if (!bus) {
pr_err("%s: Cannot find PCI bus for "
--
2.22.0.216.g00a2a96fc9
^ permalink raw reply related
* Re: [PATCH 1/1] vfio-pci/nvlink2: Allow fallback to ibm,mmio-atsd[0]
From: Sam Bobroff @ 2020-02-07 4:43 UTC (permalink / raw)
To: Alexey Kardashevskiy; +Cc: Alex Williamson, linuxppc-dev, kvm
In-Reply-To: <20200207023914.GC21238@osmium>
[-- Attachment #1: Type: text/plain, Size: 2710 bytes --]
On Fri, Feb 07, 2020 at 01:39:14PM +1100, Sam Bobroff wrote:
> On Thu, Feb 06, 2020 at 03:23:03PM +1100, Alexey Kardashevskiy wrote:
> >
> >
> > On 06/02/2020 14:17, Sam Bobroff wrote:
> > > Older versions of skiboot only provide a single value in the device
> > > tree property "ibm,mmio-atsd", even when multiple Address Translation
> > > Shoot Down (ATSD) registers are present. This prevents NVLink2 devices
> > > (other than the first) from being used with vfio-pci because vfio-pci
> > > expects to be able to assign a dedicated ATSD register to each NVLink2
> > > device.
> > >
> > > However, ATSD registers can be shared among devices. This change
> > > allows vfio-pci to fall back to sharing the register at index 0 if
> > > necessary.
> > >
> > > Signed-off-by: Sam Bobroff <sbobroff@linux.ibm.com>
> > > ---
> > > drivers/vfio/pci/vfio_pci_nvlink2.c | 13 +++++++++++--
> > > 1 file changed, 11 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/vfio/pci/vfio_pci_nvlink2.c b/drivers/vfio/pci/vfio_pci_nvlink2.c
> > > index f2983f0f84be..851ba673882b 100644
> > > --- a/drivers/vfio/pci/vfio_pci_nvlink2.c
> > > +++ b/drivers/vfio/pci/vfio_pci_nvlink2.c
> > > @@ -420,8 +420,17 @@ int vfio_pci_ibm_npu2_init(struct vfio_pci_device *vdev)
> > >
> > > if (of_property_read_u64_index(hose->dn, "ibm,mmio-atsd", nvlink_index,
> > > &mmio_atsd)) {
> > > - dev_warn(&vdev->pdev->dev, "No available ATSD found\n");
> > > - mmio_atsd = 0;
> > > + dev_warn(&vdev->pdev->dev,
> > > + "No ibm,mmio-atsd[%d] found: trying ibm,mmio-atsd[0]\n",
> > > + nvlink_index);
> >
> >
> > We do not really need this warning (nvlink_index doesn't matter that
> > much, we can work out from the device tree what happened), warnings
> > below are enough (if you really want, you can print nvlink_index there).
> >
> > Either way,
> >
> > Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> >
>
> Thanks,
>
> I'll change it if there's some reason to do another version but
> otherwise leave it as is.
>
> Sam.
Oh! I almost forgot: maybe this should carry a fixes tag?
Fixes: 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2] subdriver")
> >
> >
> >
> > > + if (of_property_read_u64_index(hose->dn, "ibm,mmio-atsd", 0,
> > > + &mmio_atsd)) {
> > > + dev_warn(&vdev->pdev->dev, "No available ATSD found\n");
> > > + mmio_atsd = 0;
> > > + } else {
> > > + dev_warn(&vdev->pdev->dev,
> > > + "Using fallback ibm,mmio-atsd[0] for ATSD.\n");
> > > + }
> > > }
> > >
> > > if (of_property_read_u64(npu_node, "ibm,device-tgt-addr", &tgt)) {
> > >
> >
> > --
> > Alexey
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH v6 03/11] powerpc/mm: Adds arch-specificic functions to track lockless pgtable walks
From: Leonardo Bras @ 2020-02-07 4:38 UTC (permalink / raw)
To: Christophe Leroy, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman, Arnd Bergmann, Andrew Morton, Aneesh Kumar K.V,
Nicholas Piggin, Steven Price, Robin Murphy, Mahesh Salgaonkar,
Balbir Singh, Reza Arbab, Thomas Gleixner, Allison Randal,
Greg Kroah-Hartman, Mike Rapoport, Michal Suchanek
Cc: linux-arch, linux-mm, linuxppc-dev, linux-kernel, kvm-ppc
In-Reply-To: <1311ce1c-7e5a-f7c4-2ab2-c03e124ca1c1@c-s.fr>
[-- Attachment #1: Type: text/plain, Size: 7765 bytes --]
On Thu, 2020-02-06 at 06:46 +0100, Christophe Leroy wrote:
>
> Le 06/02/2020 à 04:08, Leonardo Bras a écrit :
> > On powerpc, we need to do some lockless pagetable walks from functions
> > that already have disabled interrupts, specially from real mode with
> > MSR[EE=0].
> >
> > In these contexts, disabling/enabling interrupts can be very troubling.
>
> When interrupts are already disabled, the flag returned when disabling
> it will be such that when we restore it later, interrupts remain
> disabled, so what's the problem ?
There are places in code, like patch 8, where it explicitly avoids
doing irq_save/restore by using a function parameter (realmode).
http://patchwork.ozlabs.org/patch/1234130/
I am not sure why it's that way, but I decided to keep it as is.
It was introduced by Aneesh Kumar in 2015
(691e95fd7396905a38d98919e9c150dbc3ea21a3).
> > So, this arch-specific implementation features functions with an extra
> > argument that allows interrupt enable/disable to be skipped:
> > __begin_lockless_pgtbl_walk() and __end_lockless_pgtbl_walk().
> >
> > Functions similar to the generic ones are also exported, by calling
> > the above functions with parameter {en,dis}able_irq = true.
> >
> > Signed-off-by: Leonardo Bras <leonardo@linux.ibm.com>
> > ---
> > arch/powerpc/include/asm/book3s/64/pgtable.h | 6 ++
> > arch/powerpc/mm/book3s64/pgtable.c | 86 +++++++++++++++++++-
> > 2 files changed, 91 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h b/arch/powerpc/include/asm/book3s/64/pgtable.h
> > index 201a69e6a355..78f6ffb1bb3e 100644
> > --- a/arch/powerpc/include/asm/book3s/64/pgtable.h
> > +++ b/arch/powerpc/include/asm/book3s/64/pgtable.h
> > @@ -1375,5 +1375,11 @@ static inline bool pgd_is_leaf(pgd_t pgd)
> > return !!(pgd_raw(pgd) & cpu_to_be64(_PAGE_PTE));
> > }
> >
> > +#define __HAVE_ARCH_LOCKLESS_PGTBL_WALK_CONTROL
> > +unsigned long begin_lockless_pgtbl_walk(void);
> > +unsigned long __begin_lockless_pgtbl_walk(bool disable_irq);
> > +void end_lockless_pgtbl_walk(unsigned long irq_mask);
> > +void __end_lockless_pgtbl_walk(unsigned long irq_mask, bool enable_irq);
> > +
>
> Why not make them static inline just like the generic ones ?
>
Sure, can be done. It would save some function calls.
For that I will define the per-cpu variable in .c and declare it in .h
All new function can be moved to .h, while changing adding the inline
modifier.
> > #endif /* __ASSEMBLY__ */
> > #endif /* _ASM_POWERPC_BOOK3S_64_PGTABLE_H_ */
> > diff --git a/arch/powerpc/mm/book3s64/pgtable.c b/arch/powerpc/mm/book3s64/pgtable.c
> > index 2bf7e1b4fd82..535613030363 100644
> > --- a/arch/powerpc/mm/book3s64/pgtable.c
> > +++ b/arch/powerpc/mm/book3s64/pgtable.c
> > @@ -82,6 +82,7 @@ static void do_nothing(void *unused)
> > {
> >
> > }
> > +
>
> Is this blank line related to the patch ?
>
Nope, just something I 'fixed' while reading, and gone past my pre-send
patch reviewing. If it bothers, I can remove.
> > /*
> > * Serialize against find_current_mm_pte which does lock-less
> > * lookup in page tables with local interrupts disabled. For huge pages
> > @@ -98,6 +99,89 @@ void serialize_against_pte_lookup(struct mm_struct *mm)
> > smp_call_function_many(mm_cpumask(mm), do_nothing, NULL, 1);
> > }
> >
> > +/* begin_lockless_pgtbl_walk: Must be inserted before a function call that does
> > + * lockless pagetable walks, such as __find_linux_pte().
> > + * This version allows setting disable_irq=false, so irqs are not touched, which
> > + * is quite useful for running when ints are already disabled (like real-mode)
> > + */
> > +inline
> > +unsigned long __begin_lockless_pgtbl_walk(bool disable_irq)
> > +{
> > + unsigned long irq_mask = 0;
> > +
> > + /*
> > + * Interrupts must be disabled during the lockless page table walk.
> > + * That's because the deleting or splitting involves flushing TLBs,
> > + * which in turn issues interrupts, that will block when disabled.
> > + *
> > + * When this function is called from realmode with MSR[EE=0],
> > + * it's not needed to touch irq, since it's already disabled.
> > + */
> > + if (disable_irq)
> > + local_irq_save(irq_mask);
> > +
> > + /*
> > + * This memory barrier pairs with any code that is either trying to
> > + * delete page tables, or split huge pages. Without this barrier,
> > + * the page tables could be read speculatively outside of interrupt
> > + * disabling or reference counting.
> > + */
> > + smp_mb();
> > +
> > + return irq_mask;
> > +}
> > +EXPORT_SYMBOL(__begin_lockless_pgtbl_walk);
> > +
> > +/* begin_lockless_pgtbl_walk: Must be inserted before a function call that does
> > + * lockless pagetable walks, such as __find_linux_pte().
> > + * This version is used by generic code, and always assume irqs will be disabled
> > + */
> > +unsigned long begin_lockless_pgtbl_walk(void)
> > +{
> > + return __begin_lockless_pgtbl_walk(true);
> > +}
> > +EXPORT_SYMBOL(begin_lockless_pgtbl_walk);
>
> Even more than begin_lockless_pgtbl_walk(), this one is worth being
> static inline in the H file.
Same as above, can be done.
>
> > +
> > +/*
> > + * __end_lockless_pgtbl_walk: Must be inserted after the last use of a pointer
> > + * returned by a lockless pagetable walk, such as __find_linux_pte()
> > + * This version allows setting enable_irq=false, so irqs are not touched, which
> > + * is quite useful for running when ints are already disabled (like real-mode)
> > + */
> > +inline void __end_lockless_pgtbl_walk(unsigned long irq_mask, bool enable_irq)
> > +{
> > + /*
> > + * This memory barrier pairs with any code that is either trying to
> > + * delete page tables, or split huge pages. Without this barrier,
> > + * the page tables could be read speculatively outside of interrupt
> > + * disabling or reference counting.
> > + */
> > + smp_mb();
> > +
> > + /*
> > + * Interrupts must be disabled during the lockless page table walk.
> > + * That's because the deleting or splitting involves flushing TLBs,
> > + * which in turn issues interrupts, that will block when disabled.
> > + *
> > + * When this function is called from realmode with MSR[EE=0],
> > + * it's not needed to touch irq, since it's already disabled.
> > + */
> > + if (enable_irq)
> > + local_irq_restore(irq_mask);
> > +}
> > +EXPORT_SYMBOL(__end_lockless_pgtbl_walk);
> > +
> > +/*
> > + * end_lockless_pgtbl_walk: Must be inserted after the last use of a pointer
> > + * returned by a lockless pagetable walk, such as __find_linux_pte()
> > + * This version is used by generic code, and always assume irqs will be enabled
> > + */
> > +void end_lockless_pgtbl_walk(unsigned long irq_mask)
> > +{
> > + __end_lockless_pgtbl_walk(irq_mask, true);
> > +}
> > +EXPORT_SYMBOL(end_lockless_pgtbl_walk);
> > +
> > /*
> > * We use this to invalidate a pmdp entry before switching from a
> > * hugepte to regular pmd entry.
> > @@ -487,7 +571,7 @@ static int __init setup_disable_tlbie(char *str)
> > tlbie_capable = false;
> > tlbie_enabled = false;
> >
> > - return 1;
> > + return 1;
>
> Is that related to this patch at all ?
Nope, just another something I 'fixed' while reading, and gone past my
pre-send patch reviewing. If it bothers, I can remove.
>
> > }
> > __setup("disable_tlbie", setup_disable_tlbie);
> >
> >
>
> Christophe
Found other unbalanced begin/end in kvmppc_do_h_enter(). I will change
that too.
Thanks for the feedback,
Leonardo Bras
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v6 04/11] powerpc/mce_power: Use functions to track lockless pgtbl walks
From: Leonardo Bras @ 2020-02-07 4:00 UTC (permalink / raw)
To: Christophe Leroy, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman, Arnd Bergmann, Andrew Morton, Aneesh Kumar K.V,
Nicholas Piggin, Steven Price, Robin Murphy, Mahesh Salgaonkar,
Balbir Singh, Reza Arbab, Thomas Gleixner, Allison Randal,
Greg Kroah-Hartman, Mike Rapoport, Michal Suchanek
Cc: linux-arch, linux-mm, linuxppc-dev, linux-kernel, kvm-ppc
In-Reply-To: <e8a5db5a-d1a5-ef25-af46-c44949750e8a@c-s.fr>
[-- Attachment #1: Type: text/plain, Size: 1134 bytes --]
On Thu, 2020-02-06 at 06:48 +0100, Christophe Leroy wrote:
> > --- a/arch/powerpc/kernel/mce_power.c
> > +++ b/arch/powerpc/kernel/mce_power.c
> > @@ -29,7 +29,7 @@ unsigned long addr_to_pfn(struct pt_regs *regs, unsigned long addr)
> > {
> > pte_t *ptep;
> > unsigned int shift;
> > - unsigned long pfn, flags;
> > + unsigned long pfn, irq_mask;
>
> Why change the name ? flags is a well known historical name.
Oh, this commit missed the reasoning for flags name change.
For local_irq_{save,restore} a parameter named flags makes sense, for
the reader, as it could only be flags regarding irq.
I thougt passing flags to {begin,end}_lockless_pgtbl_walk would lose
it's meaning, given that it would only mean "flags for these functions"
So, changing it to irq_mask would make the reader more aware of what it
does mean.
For other commits, I added:
"Variable that saves the irq mask was renamed from flags to irq_mask so
it doesn't lose meaning now it's not directly passed to local_irq_*
functions."
I can add it to this commit message.
Thanks for the feedback,
Leonardo Bras
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 5/6] powerpc/eeh: Make early EEH init pseries specific
From: Oliver O'Halloran @ 2020-02-07 3:56 UTC (permalink / raw)
To: Sam Bobroff; +Cc: Tyrel Datwyler, linuxppc-dev
In-Reply-To: <CAOSf1CEBuv-5GLbOX6Kaux1zA2uNVheXRtDfjVkbx+wcKtZHng@mail.gmail.com>
On Fri, Feb 7, 2020 at 2:35 PM Oliver O'Halloran <oohall@gmail.com> wrote:
>
> On Fri, Feb 7, 2020 at 1:24 PM Sam Bobroff <sbobroff@linux.ibm.com> wrote:
> >
> > On Mon, Feb 03, 2020 at 07:35:20PM +1100, Oliver O'Halloran wrote:
> > > The eeh_ops->probe() function is called from two different contexts:
> > >
> > > 1. On pseries, where set set EEH_PROBE_MODE_DEVTREE, it's called in
> > "set set" -> "we set"
> > > eeh_add_device_early() which is supposed to run before we create
> > > a pci_dev.
> > >
> > > 2. On PowerNV, where we set EEH_PROBE_MODE_DEV, it's called in
> > > eeh_device_add_late() which is supposed to run *after* the
> > > pci_dev is created.
> > >
> > > The "early" probe is required because PAPR requires that we perform an RTAS
> > > call to enable EEH support on a device before we start interacting with it
> > > via config space or MMIO. This requirement doesn't exist on PowerNV and
> > > shoehorning two completely separate initialisation paths into a common
> > > interface just results in a convoluted code everywhere.
> > >
> > > Additionally the early probe requires the probe function to take an pci_dn
> > > rather than a pci_dev argument. We'd like to make pci_dn a pseries specific
> > > data structure since there's no real requirement for them on PowerNV. To
> > > help both goals move the early probe into the pseries containment zone
> > > so the platform depedence is more explicit.
> > >
> > I had a look around near your comment:
> > > + // XXX: uh, do we have the rescan lock held here?
> > And we definitely don't have the lock when it gets called via the module
> > init path (as rpaphp is loaded) -- I tried it and there was no deadlock.
> > I don't think we have the lock in other situations but I haven't
> > unravelled it all enough yet to tell, either.
>
> The other hotplug drivers seem to be taking the lock manually in their
> enable_slot() callback. So I guess we need to be doing it there too.
> I'll fix it in another patch since this one is a bit big.
On closer inspection I think we'll need to have a deeper look at this.
This function isn't used for operations on the hotplug slot. Instead
it's used for DLPAR operations including adding / removing whole PHBs.
There doesn't appear to be any code in the DLPAR add / remove paths
which takes the PCI rescan / remove lock so I think we'll need to have
a careful look at what's going on there. Great stuff...
^ permalink raw reply
* Re: [PATCH v6 06/11] powerpc/mm/book3s64/hash: Use functions to track lockless pgtbl walks
From: Leonardo Bras @ 2020-02-07 3:49 UTC (permalink / raw)
To: Christophe Leroy, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman, Arnd Bergmann, Andrew Morton, Aneesh Kumar K.V,
Nicholas Piggin, Steven Price, Robin Murphy, Mahesh Salgaonkar,
Balbir Singh, Reza Arbab, Thomas Gleixner, Allison Randal,
Greg Kroah-Hartman, Mike Rapoport, Michal Suchanek
Cc: linux-arch, linux-mm, linuxppc-dev, linux-kernel, kvm-ppc
In-Reply-To: <1b65f1a8-42a8-6ffc-3a06-08fbb34edab5@c-s.fr>
[-- Attachment #1: Type: text/plain, Size: 4413 bytes --]
On Thu, 2020-02-06 at 07:06 +0100, Christophe Leroy wrote:
> > - /* Get PTE and page size from page tables */
> > + /* Get PTE and page size from page tables :
> > + * Called in from DataAccess interrupt (data_access_common: 0x300),
> > + * interrupts are disabled here.
> > + */
>
> Comments formatting is not in line with Linux kernel rules. Should be no
> text on the first /* line.
My mistake. Will be corrected in v7.
> > + __begin_lockless_pgtbl_walk(false);
>
> I think it would be better to not use __begin_lockless_pgtbl_walk()
> directly but keep it in a single place, and define something like
> begin_lockless_pgtbl_walk_noirq() similar to begin_lockless_pgtbl_walk()
There are places where touching irq is decided by a boolean, like in
patch 8: http://patchwork.ozlabs.org/patch/1234130/
If I were to change this, I would have to place ifs and decide to call
either normal or *_noirq() versions in every function.
What you suggest?
> > ptep = find_linux_pte(pgdir, ea, &is_thp, &hugeshift);
> > if (ptep == NULL || !pte_present(*ptep)) {
> > DBG_LOW(" no PTE !\n");
> > rc = 1;
> > - goto bail;
> > + goto bail_pgtbl_walk;
>
> What's the point in changing the name of this label ? There is only one
> label, why polute the function with so huge name ?
>
> For me, everyone understand what 'bail' means. Unneccessary changes
> should be avoided. If you really really want to do it, it should be
> another patch.
>
> See kernel codying style, chapter 'naming':
> "LOCAL variable names should be short". This also applies to labels.
>
> "C is a Spartan language, and so should your naming be. Unlike Modula-2
> and Pascal programmers, C programmers do not use cute names like
> ThisVariableIsATemporaryCounter. A C programmer would call that variable
> tmp, which is much easier to write, and not the least more difficult to
> understand."
>
It's not label name changing. There are two possible bails in
hash_page_mm(): one for before __begin_lockless_pagetable_walk() and
other for after it. The new one also runs __end_lockless_pgtbl_walk()
before running what the previous did:
> > +bail_pgtbl_walk:
> > + __end_lockless_pgtbl_walk(0, false);
> > bail:
> > exception_exit(prev_state);
> > return rc;
As for the label name lengh, I see no problem changing it to something
like bail_ptw.
> > @@ -1545,7 +1551,7 @@ static void hash_preload(struct mm_struct *mm, unsigned long ea,
> > unsigned long vsid;
> > pgd_t *pgdir;
> > pte_t *ptep;
> > - unsigned long flags;
> > + unsigned long irq_mask;
> > int rc, ssize, update_flags = 0;
> > unsigned long access = _PAGE_PRESENT | _PAGE_READ | (is_exec ? _PAGE_EXEC : 0);
> >
> > @@ -1567,11 +1573,12 @@ static void hash_preload(struct mm_struct *mm, unsigned long ea,
> > vsid = get_user_vsid(&mm->context, ea, ssize);
> > if (!vsid)
> > return;
> > +
>
> Is this new line related to the patch ?
Nope. I have added while reading code and it just went trough my pre-
sending revision. I can remove it, if it bothers.
>
> > /*
> > * Hash doesn't like irqs. Walking linux page table with irq disabled
> > * saves us from holding multiple locks.
> > */
> > - local_irq_save(flags);
> > + irq_mask = begin_lockless_pgtbl_walk();
> >
> > /*
> > * THP pages use update_mmu_cache_pmd. We don't do
> > @@ -1616,7 +1623,7 @@ static void hash_preload(struct mm_struct *mm, unsigned long ea,
> > mm_ctx_user_psize(&mm->context),
> > pte_val(*ptep));
> > out_exit:
> > - local_irq_restore(flags);
> > + end_lockless_pgtbl_walk(irq_mask);
> > }
> >
> > /*
> > @@ -1679,16 +1686,16 @@ u16 get_mm_addr_key(struct mm_struct *mm, unsigned long address)
> > {
> > pte_t *ptep;
> > u16 pkey = 0;
> > - unsigned long flags;
> > + unsigned long irq_mask;
> >
> > if (!mm || !mm->pgd)
> > return 0;
> >
> > - local_irq_save(flags);
> > + irq_mask = begin_lockless_pgtbl_walk();
> > ptep = find_linux_pte(mm->pgd, address, NULL, NULL);
> > if (ptep)
> > pkey = pte_to_pkey_bits(pte_val(READ_ONCE(*ptep)));
> > - local_irq_restore(flags);
> > + end_lockless_pgtbl_walk(irq_mask);
> >
> > return pkey;
> > }
> >
>
> Christophe
Thanks for giving feedback,
Leonardo Bras
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 5/6] powerpc/eeh: Make early EEH init pseries specific
From: Oliver O'Halloran @ 2020-02-07 3:35 UTC (permalink / raw)
To: Sam Bobroff; +Cc: Tyrel Datwyler, linuxppc-dev
In-Reply-To: <20200207022412.GA21238@osmium>
On Fri, Feb 7, 2020 at 1:24 PM Sam Bobroff <sbobroff@linux.ibm.com> wrote:
>
> On Mon, Feb 03, 2020 at 07:35:20PM +1100, Oliver O'Halloran wrote:
> > The eeh_ops->probe() function is called from two different contexts:
> >
> > 1. On pseries, where set set EEH_PROBE_MODE_DEVTREE, it's called in
> "set set" -> "we set"
> > eeh_add_device_early() which is supposed to run before we create
> > a pci_dev.
> >
> > 2. On PowerNV, where we set EEH_PROBE_MODE_DEV, it's called in
> > eeh_device_add_late() which is supposed to run *after* the
> > pci_dev is created.
> >
> > The "early" probe is required because PAPR requires that we perform an RTAS
> > call to enable EEH support on a device before we start interacting with it
> > via config space or MMIO. This requirement doesn't exist on PowerNV and
> > shoehorning two completely separate initialisation paths into a common
> > interface just results in a convoluted code everywhere.
> >
> > Additionally the early probe requires the probe function to take an pci_dn
> > rather than a pci_dev argument. We'd like to make pci_dn a pseries specific
> > data structure since there's no real requirement for them on PowerNV. To
> > help both goals move the early probe into the pseries containment zone
> > so the platform depedence is more explicit.
> >
> I had a look around near your comment:
> > + // XXX: uh, do we have the rescan lock held here?
> And we definitely don't have the lock when it gets called via the module
> init path (as rpaphp is loaded) -- I tried it and there was no deadlock.
> I don't think we have the lock in other situations but I haven't
> unravelled it all enough yet to tell, either.
The other hotplug drivers seem to be taking the lock manually in their
enable_slot() callback. So I guess we need to be doing it there too.
I'll fix it in another patch since this one is a bit big.
^ permalink raw reply
* Re: [PATCH 1/6] powerpc/eeh: Add sysfs files in late probe
From: Oliver O'Halloran @ 2020-02-07 3:22 UTC (permalink / raw)
To: Sam Bobroff; +Cc: Tyrel Datwyler, linuxppc-dev
In-Reply-To: <20200206041311.GD15629@osmium>
On Thu, Feb 6, 2020 at 3:13 PM Sam Bobroff <sbobroff@linux.ibm.com> wrote:
>
> On Mon, Feb 03, 2020 at 07:35:16PM +1100, Oliver O'Halloran wrote:
> > Move creating the EEH specific sysfs files into eeh_add_device_late()
> > rather than being open-coded all over the place. Calling the function is
> > generally done immediately after calling eeh_add_device_late() anyway. The
> > two cases where it's not done there (OF based PCI probing and the pseries
> > VFs) don't seem to have any issues with the re-ordering.
>
> I haven't tested it explicitly, but I suspect the re-ordering will
> actually improve things: in some error cases it will no longer add sysfs
> files for devices that have failed to init, because bailing out in
> eeh_add_device_late() (or eeh_probve_device()) will now prevent
> eeh_sysfs_add_device() from being called.
>
> Nice cleanup.
>
> Reviewed-by: Sam Bobroff <sbobroff@linux.ibm.com>
Ah, good point. I'll update the commit message.
^ permalink raw reply
* Re: [PATCH v6 07/11] powerpc/kvm/e500: Use functions to track lockless pgtbl walks
From: Leonardo Bras @ 2020-02-07 3:10 UTC (permalink / raw)
To: Christophe Leroy, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman, Arnd Bergmann, Andrew Morton, Aneesh Kumar K.V,
Nicholas Piggin, Steven Price, Robin Murphy, Mahesh Salgaonkar,
Balbir Singh, Reza Arbab, Thomas Gleixner, Allison Randal,
Greg Kroah-Hartman, Mike Rapoport, Michal Suchanek
Cc: linux-arch, linux-mm, linuxppc-dev, linux-kernel, kvm-ppc
In-Reply-To: <fae235d5-78b6-87aa-ed3f-1a908d61abf4@c-s.fr>
[-- Attachment #1: Type: text/plain, Size: 548 bytes --]
Hello Christophe,
On Thu, 2020-02-06 at 07:18 +0100, Christophe Leroy wrote:
>
> I don't really like unbalanced begin/end.
>
> Something like the following would be cleaner:
>
>
> begin_lockless_pgtbl_walk()
> ptep = find()
> if (ptep) {
> pte = READ_ONCE()
> if (pte_present(pte))
> wing=
> else
> ret = -EINVAL;
> }
> end_lockless_pgtbl_walk()
>
> if (ret) {
> pr_err_rate...()
> goto out;
> }
>
>
Sure, looks better that way. I will change that for v7.
Thanks for the feedback,
Leonardo Bras
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ 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