* [PATCH] powerpc/smp: Fix OOPS in topology_init()
From: Christophe Leroy @ 2021-08-04 18:24 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linuxppc-dev, linux-kernel
Running an SMP kernel on an UP platform not prepared for it,
I encountered the following OOPS:
BUG: Kernel NULL pointer dereference on read at 0x00000034
Faulting instruction address: 0xc0a04110
Oops: Kernel access of bad area, sig: 11 [#1]
BE PAGE_SIZE=4K SMP NR_CPUS=2 CMPCPRO
Modules linked in:
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.13.0-pmac-00001-g230fedfaad21 #5234
NIP: c0a04110 LR: c0a040d8 CTR: c0a04084
REGS: e100dda0 TRAP: 0300 Not tainted (5.13.0-pmac-00001-g230fedfaad21)
MSR: 00009032 <EE,ME,IR,DR,RI> CR: 84000284 XER: 00000000
DAR: 00000034 DSISR: 20000000
GPR00: c0006bd4 e100de60 c1033320 00000000 00000000 c0942274 00000000 00000000
GPR08: 00000000 00000000 00000001 00000063 00000007 00000000 c0006f30 00000000
GPR16: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000005
GPR24: c0c67d74 c0c67f1c c0c60000 c0c67d70 c0c0c558 1efdf000 c0c00020 00000000
NIP [c0a04110] topology_init+0x8c/0x138
LR [c0a040d8] topology_init+0x54/0x138
Call Trace:
[e100de60] [80808080] 0x80808080 (unreliable)
[e100de90] [c0006bd4] do_one_initcall+0x48/0x1bc
[e100def0] [c0a0150c] kernel_init_freeable+0x1c8/0x278
[e100df20] [c0006f44] kernel_init+0x14/0x10c
[e100df30] [c00190fc] ret_from_kernel_thread+0x14/0x1c
Instruction dump:
7c692e70 7d290194 7c035040 7c7f1b78 5529103a 546706fe 5468103a 39400001
7c641b78 40800054 80c690b4 7fb9402e <81060034> 7fbeea14 2c080000 7fa3eb78
---[ end trace b246ffbc6bbbb6fb ]---
Fix it by checking smp_ops before using it, as already done in
several other places in the arch/powerpc/kernel/smp.c
Fixes: 39f87561454d ("powerpc/smp: Move ppc_md.cpu_die() to smp_ops.cpu_offline_self()")
Cc: stable@vger.kernel.org
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/kernel/sysfs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/kernel/sysfs.c b/arch/powerpc/kernel/sysfs.c
index 5ff0e55d0db1..defecb3b1b15 100644
--- a/arch/powerpc/kernel/sysfs.c
+++ b/arch/powerpc/kernel/sysfs.c
@@ -1167,7 +1167,7 @@ static int __init topology_init(void)
* CPU. For instance, the boot cpu might never be valid
* for hotplugging.
*/
- if (smp_ops->cpu_offline_self)
+ if (smp_ops && smp_ops->cpu_offline_self)
c->hotpluggable = 1;
#endif
--
2.25.0
^ permalink raw reply related
* Re: [PATCH] powerpc: Always inline radix_enabled() to fix build failure
From: Erhard F. @ 2021-08-04 16:03 UTC (permalink / raw)
To: Jordan Niethe; +Cc: linuxppc-dev
In-Reply-To: <20210804013724.514468-1-jniethe5@gmail.com>
On Wed, 4 Aug 2021 11:37:24 +1000
Jordan Niethe <jniethe5@gmail.com> wrote:
> This is the same as commit acdad8fb4a15 ("powerpc: Force inlining of
> mmu_has_feature to fix build failure") but for radix_enabled(). The
> config in the linked bugzilla causes the following build failure:
> [...]
> The code relies on constant folding of MMU_FTRS_POSSIBLE at buildtime
> and elimination of non possible parts of code at compile time. For this
> to work radix_enabled() must be inlined so make it __always_inline.
>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=213803
> Reported-by: Erhard F. <erhard_f@mailbox.org>
> Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
> Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> ---
> arch/powerpc/include/asm/mmu.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/include/asm/mmu.h b/arch/powerpc/include/asm/mmu.h
> index 27016b98ecb2..8abe8e42e045 100644
> --- a/arch/powerpc/include/asm/mmu.h
> +++ b/arch/powerpc/include/asm/mmu.h
> @@ -324,7 +324,7 @@ static inline void assert_pte_locked(struct mm_struct *mm, unsigned long addr)
> }
> #endif /* !CONFIG_DEBUG_VM */
>
> -static inline bool radix_enabled(void)
> +static __always_inline bool radix_enabled(void)
> {
> return mmu_has_feature(MMU_FTR_TYPE_RADIX);
> }
> --
> 2.25.1
Thanks Jordan!
Your patch works well and my kernel build completes. Tested on v5.14-rc4. Only getting some warnings now:
[...]
CHK include/generated/autoksyms.h
GEN .version
CHK include/generated/compile.h
LD vmlinux.o
MODPOST vmlinux.symvers
MODINFO modules.builtin.modinfo
GEN modules.builtin
LD .tmp_vmlinux.kallsyms1
ld: warning: creating DT_TEXTREL in a PIE
KSYMS .tmp_vmlinux.kallsyms1.S
AS .tmp_vmlinux.kallsyms1.S
LD .tmp_vmlinux.kallsyms2
ld: warning: creating DT_TEXTREL in a PIE
KSYMS .tmp_vmlinux.kallsyms2.S
AS .tmp_vmlinux.kallsyms2.S
LD vmlinux
ld: warning: creating DT_TEXTREL in a PIE
SORTTAB vmlinux
SYSMAP System.map
CHKHEAD vmlinux
CHKREL vmlinux
GEN .version
CHK include/generated/compile.h
UPD include/generated/compile.h
CC init/version.o
AR init/built-in.a
LD vmlinux.o
MODPOST vmlinux.symvers
MODINFO modules.builtin.modinfo
GEN modules.builtin
LD .tmp_vmlinux.kallsyms1
ld: warning: creating DT_TEXTREL in a PIE
KSYMS .tmp_vmlinux.kallsyms1.S
AS .tmp_vmlinux.kallsyms1.S
LD .tmp_vmlinux.kallsyms2
ld: warning: creating DT_TEXTREL in a PIE
KSYMS .tmp_vmlinux.kallsyms2.S
AS .tmp_vmlinux.kallsyms2.S
LD vmlinux
ld: warning: creating DT_TEXTREL in a PIE
SORTTAB vmlinux
SYSMAP System.map
CHKHEAD vmlinux
CHKREL vmlinux
MODPOST modules-only.symvers
[...]
^ permalink raw reply
* Re: [PATCH printk v1 03/10] kgdb: delay roundup if holding printk cpulock
From: Daniel Thompson @ 2021-08-04 15:04 UTC (permalink / raw)
To: Petr Mladek
Cc: Gautham R. Shenoy, Douglas Anderson, Srikar Dronamraju,
Peter Zijlstra, linux-kernel, Paul Mackerras, H. Peter Anvin,
Chengyang Fan, Bhaskar Chowdhury, x86, Ingo Molnar,
kgdb-bugreport, John Ogness, Nicholas Piggin, Borislav Petkov,
Steven Rostedt, Thomas Gleixner, Gustavo A. R. Silva,
Sergey Senozhatsky, Jason Wessel, linuxppc-dev,
Cédric Le Goater
In-Reply-To: <YQqEJtmNFxVxH3U/@alley>
On Wed, Aug 04, 2021 at 02:12:22PM +0200, Petr Mladek wrote:
> On Wed 2021-08-04 12:31:59, Daniel Thompson wrote:
> > On Tue, Aug 03, 2021 at 05:36:32PM +0206, John Ogness wrote:
> > > On 2021-08-03, Daniel Thompson <daniel.thompson@linaro.org> wrote:
> > > > On Tue, Aug 03, 2021 at 03:18:54PM +0206, John Ogness wrote:
> > > >> kgdb makes use of its own cpulock (@dbg_master_lock, @kgdb_active)
> > > >> during cpu roundup. This will conflict with the printk cpulock.
> > > >
> > > > When the full vision is realized what will be the purpose of the printk
> > > > cpulock?
> > > >
> > > > I'm asking largely because it's current role is actively unhelpful
> > > > w.r.t. kdb. It is possible that cautious use of in_dbg_master() might
> > > > be a better (and safer) solution. However it sounds like there is a
> > > > larger role planned for the printk cpulock...
> > >
> > > The printk cpulock is used as a synchronization mechanism for
> > > implementing atomic consoles, which need to be able to safely interrupt
> > > the console write() activity at any time and immediately continue with
> > > their own printing. The ultimate goal is to move all console printing
> > > into per-console dedicated kthreads, so the primary function of the
> > > printk cpulock is really to immediately _stop_ the CPU/kthread
> > > performing write() in order to allow write_atomic() (from any context on
> > > any CPU) to safely and reliably take over.
> >
> > I see.
> >
> > Is there any mileage in allowing in_dbg_master() to suppress taking
> > the console lock?
> >
> > There's a couple of reasons to worry about the current approach.
> >
> > The first is that we don't want this code to trigger in the case when
> > kgdb is enabled and kdb is not since it is only kdb (a self-hosted
> > debugger) than uses the consoles. This case is relatively trivial to
> > address since we can rename it kdb_roundup_delay() and alter the way it
> > is conditionally compiled.
> >
> > The second is more of a problem however. kdb will only call into the
> > console code from the debug master. By default this is the CPU that
> > takes the debug trap so initial prints will work fine. However it is
> > possible to switch to a different master (so we can read per-CPU
> > registers and things like that). This will result in one of the CPUs
> > that did the IPI round up calling into console code and this is unsafe
> > in that instance.
> >
> > There are a couple of tricks we could adopt to work around this but
> > given the slightly odd calling context for kdb (all CPUs quiesced, no
> > log interleaving possible) it sounds like it would remain safe to
> > bypass the lock if in_dbg_master() is true.
> >
> > Bypassing an inconvenient lock might sound icky but:
> >
> > 1. If the lock is not owned by any CPU then what kdb will do is safe.
> >
> > 2. If the lock is owned by any CPU then we have quiesced it anyway
> > and this makes is safe for the owning CPU to share its ownership
> > (since it isn't much different to recursive acquisition on a single
> > CPU)
>
> I think about the following:
>
> void kgdb_roundup_cpus(void)
> {
> __printk_cpu_lock();
> __kgdb_roundup_cpus();
> }
>
> , where __printk_cpu_lock() waits/takes printk_cpu_lock()
> __kgdb_roundup_cpus() is the original kgdb_roundup_cpus();
>
>
> The idea is that kgdb_roundup_cpus() caller takes the printk_cpu lock.
> The owner will be well defined.
>
> As a result any other CPU will not be able to take the printk_cpu lock
> as long as it is owned by the kgdb lock. But as you say, kgdb will
> make sure that everything is serialized at this stage. So that
> the original raw_printk_cpu_lock_irqsave() might just disable
> IRQs when called under debugger.
>
> Does it make any sense?
Yes but I think it is still has problems.
Primarily is doesn't solve the issue I raised. It would still be unsafe
to change debug master: we can guarantee the initial master owns the
lock but if it has been multiply acquired we cannot transfer ownership
when we want to change master.
Additionally it will delay the round up of cores that do not own the
lock. The quiescing is never atomic and the operator needs to know
that but the longer CPUs are allows to execute for the more confusing
things can become for the operator.
Finally on machines without an NMI this could cause trouble with the
interrupt disable in raw_printk_cpu_lock_irqsave() (or any outer level
interrupt disable). If the master get the lock then the other processes
will become incapable of being rounded up if they are waiting for the
printk lock).
> I have to say that it is a bit hairy. But it looks slightly better
> than the delayed/repeated IPI proposed by this patch.
I'd like to reserve judgement for now which one is least worst...
largely because if the purpose of the lock simply to prevent interleaving
of console output then the debugger quiescing code should already have
this covered.
It leaves me wondering if a change like the one below is sufficient
(based on code without John's patches but hopefully still clear enough).
I've given the new code it's own branch which it doesn't, strictly
speaking, need but it is easier to comment this way... and perhaps also
just a little easier for people who have not set CONFIG_KGDB to
ignore ;-).
~~~
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 142a58d124d9..41a7e103bb66 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3599,6 +3599,18 @@ int __printk_cpu_trylock(void)
/* This CPU is already the owner. */
atomic_inc(&printk_cpulock_nested);
return 1;
+ } else if (in_dbg_master()) {
+ /*
+ * If we are executing as the primary CPU and with the debugger
+ * active than all other CPUs in the system are quiesced by
+ * the time kdb winds up calling this function. To execute this
+ * branch then the lock must be owned by one of the quiesced CPUs.
+ * Happily, because it is quiesced and cannot release it, it is
+ * safe for us to allow the lock to be taken from a different CPU!
+ * The lock will be released prior to resuming the real owner.
+ */
+ atomic_inc(&printk_cpulock_nested);
+ return 1;
}
return 0;
~~~
Daniel.
PS In the interested of full disclosure there is a special case
in the debugger to allow it to try to cope if it fails to
quiesce a CPU and I deliberately omitted this from the long
comment above. That special case is expected to be unstable
but since the alternative is likely to be a permanent deadlock
without any indication of why we choose to take the risk of
continuing. Personally I don't recommend reasoning about
console safety based on this emergency case hence omitting the
comment.
^ permalink raw reply related
* Re: [PATCH] powerpc/32s: Fix napping restore in data storage interrupt (DSI)
From: Nicholas Piggin @ 2021-08-04 14:45 UTC (permalink / raw)
To: Christophe Leroy
Cc: userm57, Finn Thain, linux-kernel, Paul Mackerras, linuxppc-dev
In-Reply-To: <d8e1f924-ca60-4dcc-ac5f-3801ea226edf@csgroup.eu>
Excerpts from Christophe Leroy's message of August 4, 2021 11:28 pm:
>
>
> Le 04/08/2021 à 13:36, Nicholas Piggin a écrit :
>> Excerpts from Christophe Leroy's message of August 4, 2021 4:21 pm:
>>> Hi Nic,
>>>
>>> I think I'll need your help on that one.
>>>
>>> Le 04/08/2021 à 08:07, Christophe Leroy a écrit :
>>>>
>>>>
>>>> Le 04/08/2021 à 06:04, Finn Thain a écrit :
>>
>> Hi Finn!
>>
>>>>> On Tue, 3 Aug 2021, Christophe Leroy wrote:
>>>>>
>>> ...
>>>>>
>>>>> ------------[ cut here ]------------
>>>>> kernel BUG at arch/powerpc/kernel/interrupt.c:49!
>>>>> Oops: Exception in kernel mode, sig: 5 [#1]
>>>>> BE PAGE_SIZE=4K MMU=Hash SMP NR_CPUS=2 PowerMac
>>>>> Modules linked in:
>>>>> CPU: 0 PID: 1859 Comm: xfce4-session Not tainted 5.13.0-pmac-VMAP #10
>>>>> NIP: c0011474 LR: c0011464 CTR: 00000000
>>>>> REGS: e2f75e40 TRAP: 0700 Not tainted (5.13.0-pmac-VMAP)
>>>>> MSR: 00021032 <ME,IR,DR,RI> CR: 2400446c XER: 20000000
>>>>>
>>>>> GPR00: c001604c e2f75f00 ca284a60 00000000 00000000 a5205eb0 00000008 00000020
>>>>> GPR08: ffffffc0 00000001 501200d9 ce030005 ca285010 00c1f778 00000000 00000000
>>>>> GPR16: 00945b20 009402f8 00000001 a6b87550 a51fd000 afb73220 a6b22c78 a6a6aecc
>>>>> GPR24: 00000000 ffffffc0 00000020 00000008 a5205eb0 00000000 e2f75f40 000000ae
>>>>> NIP [c0011474] system_call_exception+0x60/0x164
>>>>> LR [c0011464] system_call_exception+0x50/0x164
>>>>> Call Trace:
>>>>> [e2f75f00] [00009000] 0x9000 (unreliable)
>>>>> [e2f75f30] [c001604c] ret_from_syscall+0x0/0x28
>>>>> --- interrupt: c00 at 0xa69d6cb0
>>>>> NIP: a69d6cb0 LR: a69d6c3c CTR: 00000000
>>>>> REGS: e2f75f40 TRAP: 0c00 Not tainted (5.13.0-pmac-VMAP)
>>>>> MSR: 0000d032 <EE,PR,ME,IR,DR,RI> CR: 2400446c XER: 20000000
>>>>>
>>>>> GPR00: 000000ae a5205de0 a5687ca0 00000000 00000000 a5205eb0 00000008 00000020
>>>>> GPR08: ffffffc0 401201ea 401200d9 ffffffff c158f230 00c1f778 00000000 00000000
>>>>> GPR16: 00945b20 009402f8 00000001 a6b87550 a51fd000 afb73220 a6b22c78 a6a6aecc
>>>>> GPR24: afb72fc8 00000000 00000001 a5205f30 afb733dc 00000000 a6b85ff4 a5205eb0
>>>>> NIP [a69d6cb0] 0xa69d6cb0
>>>>> LR [a69d6c3c] 0xa69d6c3c
>>>>> --- interrupt: c00
>>>>> Instruction dump:
>>>>> 7cdb3378 93810020 7cbc2b78 93a10024 7c9d2378 93e1002c 7d3f4b78 4800d629
>>>>> 817e0084 931e0088 69690002 5529fffe <0f090000> 69694000 552997fe 0f090000
>>>>> ---[ end trace c66c6c3c44806276 ]---
>>>>>
>>>
>>> Getting a BUG at arch/powerpc/kernel/interrupt.c:49 meaning MSR_RI is not set, but the c00 interrupt
>>> frame shows MSR_RI properly set, so what ?
>>
>> Could the stack be correct but regs pointer incorrect?
>>
>> Instruction dump is
>>
>> 0: 78 33 db 7c mr r27,r6
>> 4: 20 00 81 93 stw r28,32(r1)
>> 8: 78 2b bc 7c mr r28,r5
>> c: 24 00 a1 93 stw r29,36(r1)
>> 10: 78 23 9d 7c mr r29,r4
>> 14: 2c 00 e1 93 stw r31,44(r1)
>> 18: 78 4b 3f 7d mr r31,r9
>> 1c: 29 d6 00 48 bl 0xd644
>> 20: 84 00 7e 81 lwz r11,132(r30)
>> 24: 88 00 1e 93 stw r24,136(r30)
>> 28: 02 00 69 69 xori r9,r11,2
>> 2c: fe ff 29 55 rlwinm r9,r9,31,31,31
>> 30: 00 00 09 0f twnei r9,0
>> 34: 00 40 69 69 xori r9,r11,16384
>> 38: fe 97 29 55 rlwinm r9,r9,18,31,31
>> 3c: 00 00 09 0f twnei r9,0
>>
>> regs->msr is in r11 == 0xce030005 so some kernel address?
>>
>> r1 == 0xe2f75f00
>> r30 == 0xe2f75f40
>>
>> I think that matches if the function allocates 48 bytes of stack.
>> STACK_FRAME_OVERHEAD is 16, so the difference would be 0x40 in that
>> case. Seems okay.
>>
>> I'm not sure then. Can you get a hash fault interrupt come in here
>> because of the vmap stack access and clobber r11? Hmm...
>>
>> fast_hash_page_return:
>> andis. r10, r9, SRR1_ISI_NOPT@h /* Set on ISI, cleared on DSI */
>>
>> Is that really right? DSI can set this bit for NOHPTE as well no?
>
> On DSI, the error bits are in DSISR while they are in SRR1 on ISI.
>
> r9 is supposed to contain SRR1 in both cases. Powerpc 32 bits programming manual explicitely says
> that bits 1-4 and 10-15 of SRR1 are cleared on DSI.
Ah right, I had in mind it was DSISR on DSI and SRR1 on ISI because
we put them together early on 64s. Can't think of anything else at the
moment.
Thanks,
Nick
^ permalink raw reply
* [PATCH] powerpc/kprobes: Fix kprobe Oops happens in booke
From: Pu Lehui @ 2021-08-04 14:37 UTC (permalink / raw)
To: oleg, mpe, benh, paulus, naveen.n.rao, mhiramat, christophe.leroy,
peterz, npiggin, ruscur
Cc: zhangjinhao2, xukuohai, linuxppc-dev, linux-kernel, pulehui
When using kprobe on powerpc booke series processor, Oops happens
as show bellow:
[ 35.861352] Oops: Exception in kernel mode, sig: 5 [#1]
[ 35.861676] BE PAGE_SIZE=4K SMP NR_CPUS=24 QEMU e500
[ 35.861905] Modules linked in:
[ 35.862144] CPU: 0 PID: 76 Comm: sh Not tainted 5.14.0-rc3-00060-g7e96bf476270 #18
[ 35.862610] NIP: c0b96470 LR: c00107b4 CTR: c0161c80
[ 35.862805] REGS: c387fe70 TRAP: 0700 Not tainted (5.14.0-rc3-00060-g7e96bf476270)
[ 35.863198] MSR: 00029002 <CE,EE,ME> CR: 24022824 XER: 20000000
[ 35.863577]
[ 35.863577] GPR00: c0015218 c387ff20 c313e300 c387ff50 00000004 40000002 40000000 0a1a2cce
[ 35.863577] GPR08: 00000000 00000004 00000000 59764000 24022422 102490c2 00000000 00000000
[ 35.863577] GPR16: 00000000 00000000 00000040 10240000 10240000 10240000 10240000 10220000
[ 35.863577] GPR24: ffffffff 10240000 00000000 00000000 bfc655e8 00000800 c387ff50 00000000
[ 35.865367] NIP [c0b96470] schedule+0x0/0x130
[ 35.865606] LR [c00107b4] interrupt_exit_user_prepare_main+0xf4/0x100
[ 35.865974] Call Trace:
[ 35.866142] [c387ff20] [c0053224] irq_exit+0x114/0x120 (unreliable)
[ 35.866472] [c387ff40] [c0015218] interrupt_return+0x14/0x13c
[ 35.866728] --- interrupt: 900 at 0x100af3dc
[ 35.866963] NIP: 100af3dc LR: 100de020 CTR: 00000000
[ 35.867177] REGS: c387ff50 TRAP: 0900 Not tainted (5.14.0-rc3-00060-g7e96bf476270)
[ 35.867488] MSR: 0002f902 <CE,EE,PR,FP,ME> CR: 20022422 XER: 20000000
[ 35.867808]
[ 35.867808] GPR00: c001509c bfc65570 1024b4d0 00000000 100de020 20022422 bfc655a8 100af3dc
[ 35.867808] GPR08: 0002f902 00000000 00000000 00000000 72656773 102490c2 00000000 00000000
[ 35.867808] GPR16: 00000000 00000000 00000040 10240000 10240000 10240000 10240000 10220000
[ 35.867808] GPR24: ffffffff 10240000 00000000 00000000 bfc655e8 10245910 ffffffff 00000001
[ 35.869406] NIP [100af3dc] 0x100af3dc
[ 35.869578] LR [100de020] 0x100de020
[ 35.869751] --- interrupt: 900
[ 35.870001] Instruction dump:
[ 35.870283] 40c20010 815e0518 714a0100 41e2fd04 39200000 913e00c0 3b1e0450 4bfffd80
[ 35.870666] 0fe00000 92a10024 4bfff1a9 60000000 <7fe00008> 7c0802a6 93e1001c 7c5f1378
[ 35.871339] ---[ end trace 23ff848139efa9b9 ]---
There is no real mode for booke arch and the MMU translation is
always on. The corresponding MSR_IS/MSR_DS bit in booke is used
to switch the address space, but not for real mode judgment.
Fixes: 21f8b2fa3ca5 ("powerpc/kprobes: Ignore traps that happened in real mode")
Signed-off-by: Pu Lehui <pulehui@huawei.com>
---
arch/powerpc/include/asm/ptrace.h | 6 ++++++
arch/powerpc/kernel/kprobes.c | 5 +----
2 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h
index 3e5d470a6155..4aec1a97024b 100644
--- a/arch/powerpc/include/asm/ptrace.h
+++ b/arch/powerpc/include/asm/ptrace.h
@@ -187,6 +187,12 @@ static inline unsigned long frame_pointer(struct pt_regs *regs)
#define user_mode(regs) (((regs)->msr & MSR_PR) != 0)
#endif
+#ifdef CONFIG_BOOKE
+#define real_mode(regs) 0
+#else
+#define real_mode(regs) (!((regs)->msr & MSR_IR) || !((regs)->msr & MSR_DR))
+#endif
+
#define force_successful_syscall_return() \
do { \
set_thread_flag(TIF_NOERROR); \
diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index cbc28d1a2e1b..fac9a5974718 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -289,10 +289,7 @@ int kprobe_handler(struct pt_regs *regs)
unsigned int *addr = (unsigned int *)regs->nip;
struct kprobe_ctlblk *kcb;
- if (user_mode(regs))
- return 0;
-
- if (!(regs->msr & MSR_IR) || !(regs->msr & MSR_DR))
+ if (user_mode(regs) || real_mode(regs))
return 0;
/*
--
2.17.1
^ permalink raw reply related
* [Bug 213961] Oops while loading radeon driver
From: bugzilla-daemon @ 2021-08-04 14:26 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <bug-213961-206035@https.bugzilla.kernel.org/>
https://bugzilla.kernel.org/show_bug.cgi?id=213961
--- Comment #4 from Christophe Leroy (christophe.leroy@csgroup.eu) ---
Thanks, unfortunately the info is still not usable because we don't have
function names in the dump:
[ 15.810083] Call Trace:
[ 15.810090] [f2b6dc30] [c1022318] 0xc1022318 (unreliable)
[ 15.810102] [f2b6dc50] [be987860] 0xbe987860
[ 15.810112] [f2b6dc80] [be85074c] 0xbe85074c
[ 15.810121] [f2b6dcb0] [be984484] 0xbe984484
[ 15.810130] [f2b6dcd0] [c037fd8c] 0xc037fd8c
[ 15.810140] [f2b6dd00] [c03e1460] 0xc03e1460
[ 15.810149] [f2b6dd20] [c03e17f0] 0xc03e17f0
[ 15.810158] [f2b6dd40] [c03e18f4] 0xc03e18f4
[ 15.810168] [f2b6dd70] [c03e21f4] 0xc03e21f4
[ 15.810177] [f2b6dd90] [c03deaf4] 0xc03deaf4
[ 15.810187] [f2b6ddc0] [c03e04d8] 0xc03e04d8
[ 15.810196] [f2b6ddf0] [c03e2d6c] 0xc03e2d6c
[ 15.810205] [f2b6de10] [c0006c34] 0xc0006c34
[ 15.810215] [f2b6de80] [c00cb0d4] 0xc00cb0d4
[ 15.810224] [f2b6deb0] [c00cc2d4] 0xc00cc2d4
[ 15.810233] [f2b6df30] [c0017098] 0xc0017098
Looking at the .config, it seems like CONFIG_KALLSYMS is missing.
Can you retry with CONFIG_KALLSYMS ?
Thanks
--
You may reply to this email to add a comment.
You are receiving this mail because:
You are watching the assignee of the bug.
^ permalink raw reply
* Re: [PATCH] powerpc/32: Fix critical and debug interrupts on BOOKE
From: Radu Rendec @ 2021-08-04 14:01 UTC (permalink / raw)
To: Christophe Leroy; +Cc: linuxppc-dev, Paul Mackerras, linux-kernel
In-Reply-To: <c9f3a3e2-e5ed-6074-b700-99911d925012@csgroup.eu>
Hi Christophe,
On Wed, 2021-08-04 at 07:52 +0200, Christophe Leroy wrote:
> Le 07/07/2021 à 07:55, Christophe Leroy a écrit :
> > 32 bits BOOKE have special interrupts for debug and other
> > critical events.
>
> Were you able to test this patch ?
I tested it three weeks ago and it works like a charm!
I'm so sorry I forgot to let you know. I got distracted testing the
old ptrace() problem. In fact, I wouldn't have been able to test that
if your interrupts patch hadn't been working.
Thanks,
Radu
> > When handling those interrupts, dedicated registers are saved
> > in the stack frame in addition to the standard registers, leading
> > to a shift of the pt_regs struct.
> >
> > Since commit db297c3b07af ("powerpc/32: Don't save thread.regs on
> > interrupt entry"), the pt_regs struct is expected to be at the
> > same place all the time.
> >
> > Instead of handling a special struct in addition to pt_regs, just
> > add those special registers to struct pt_regs.
> >
> > Reported-by: Radu Rendec <radu.rendec@gmail.com>
> > Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> > Fixes: db297c3b07af ("powerpc/32: Don't save thread.regs on
> > interrupt entry")
> > Cc: stable@vger.kernel.org
> > ---
> > arch/powerpc/include/asm/ptrace.h | 16 ++++++++++++++++
> > arch/powerpc/kernel/asm-offsets.c | 31
> > ++++++++++++++-----------------
> > arch/powerpc/kernel/head_booke.h | 27
> > +++------------------------
> > 3 files changed, 33 insertions(+), 41 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/ptrace.h
> > b/arch/powerpc/include/asm/ptrace.h
> > index 3e5d470a6155..14422e851494 100644
> > --- a/arch/powerpc/include/asm/ptrace.h
> > +++ b/arch/powerpc/include/asm/ptrace.h
> > @@ -70,6 +70,22 @@ struct pt_regs
> > unsigned long __pad[4]; /* Maintain 16 byte
> > interrupt stack alignment */
> > };
> > #endif
> > +#if defined(CONFIG_PPC32) && defined(CONFIG_BOOKE)
> > + struct { /* Must be a multiple of 16 bytes */
> > + unsigned long mas0;
> > + unsigned long mas1;
> > + unsigned long mas2;
> > + unsigned long mas3;
> > + unsigned long mas6;
> > + unsigned long mas7;
> > + unsigned long srr0;
> > + unsigned long srr1;
> > + unsigned long csrr0;
> > + unsigned long csrr1;
> > + unsigned long dsrr0;
> > + unsigned long dsrr1;
> > + };
> > +#endif
> > };
> > #endif
> >
> > diff --git a/arch/powerpc/kernel/asm-offsets.c
> > b/arch/powerpc/kernel/asm-offsets.c
> > index a47eefa09bcb..5bee245d832b 100644
> > --- a/arch/powerpc/kernel/asm-offsets.c
> > +++ b/arch/powerpc/kernel/asm-offsets.c
> > @@ -309,24 +309,21 @@ int main(void)
> > STACK_PT_REGS_OFFSET(STACK_REGS_IAMR, iamr);
> > #endif
> >
> > -#if defined(CONFIG_PPC32)
> > -#if defined(CONFIG_BOOKE) || defined(CONFIG_40x)
> > - DEFINE(EXC_LVL_SIZE, STACK_EXC_LVL_FRAME_SIZE);
> > - DEFINE(MAS0, STACK_INT_FRAME_SIZE+offsetof(struct
> > exception_regs, mas0));
> > +#if defined(CONFIG_PPC32) && defined(CONFIG_BOOKE)
> > + STACK_PT_REGS_OFFSET(MAS0, mas0);
> > /* we overload MMUCR for 44x on MAS0 since they are
> > mutually exclusive */
> > - DEFINE(MMUCR, STACK_INT_FRAME_SIZE+offsetof(struct
> > exception_regs, mas0));
> > - DEFINE(MAS1, STACK_INT_FRAME_SIZE+offsetof(struct
> > exception_regs, mas1));
> > - DEFINE(MAS2, STACK_INT_FRAME_SIZE+offsetof(struct
> > exception_regs, mas2));
> > - DEFINE(MAS3, STACK_INT_FRAME_SIZE+offsetof(struct
> > exception_regs, mas3));
> > - DEFINE(MAS6, STACK_INT_FRAME_SIZE+offsetof(struct
> > exception_regs, mas6));
> > - DEFINE(MAS7, STACK_INT_FRAME_SIZE+offsetof(struct
> > exception_regs, mas7));
> > - DEFINE(_SRR0, STACK_INT_FRAME_SIZE+offsetof(struct
> > exception_regs, srr0));
> > - DEFINE(_SRR1, STACK_INT_FRAME_SIZE+offsetof(struct
> > exception_regs, srr1));
> > - DEFINE(_CSRR0, STACK_INT_FRAME_SIZE+offsetof(struct
> > exception_regs, csrr0));
> > - DEFINE(_CSRR1, STACK_INT_FRAME_SIZE+offsetof(struct
> > exception_regs, csrr1));
> > - DEFINE(_DSRR0, STACK_INT_FRAME_SIZE+offsetof(struct
> > exception_regs, dsrr0));
> > - DEFINE(_DSRR1, STACK_INT_FRAME_SIZE+offsetof(struct
> > exception_regs, dsrr1));
> > -#endif
> > + STACK_PT_REGS_OFFSET(MMUCR, mas0);
> > + STACK_PT_REGS_OFFSET(MAS1, mas1);
> > + STACK_PT_REGS_OFFSET(MAS2, mas2);
> > + STACK_PT_REGS_OFFSET(MAS3, mas3);
> > + STACK_PT_REGS_OFFSET(MAS6, mas6);
> > + STACK_PT_REGS_OFFSET(MAS7, mas7);
> > + STACK_PT_REGS_OFFSET(_SRR0, srr0);
> > + STACK_PT_REGS_OFFSET(_SRR1, srr1);
> > + STACK_PT_REGS_OFFSET(_CSRR0, csrr0);
> > + STACK_PT_REGS_OFFSET(_CSRR1, csrr1);
> > + STACK_PT_REGS_OFFSET(_DSRR0, dsrr0);
> > + STACK_PT_REGS_OFFSET(_DSRR1, dsrr1);
> > #endif
> >
> > /* About the CPU features table */
> > diff --git a/arch/powerpc/kernel/head_booke.h
> > b/arch/powerpc/kernel/head_booke.h
> > index 87b806e8eded..e5503420b6c6 100644
> > --- a/arch/powerpc/kernel/head_booke.h
> > +++ b/arch/powerpc/kernel/head_booke.h
> > @@ -168,20 +168,18 @@ ALT_FTR_SECTION_END_IFSET(CPU_FTR_EMB_HV)
> > /* only on e500mc */
> > #define DBG_STACK_BASE dbgirq_ctx
> >
> > -#define EXC_LVL_FRAME_OVERHEAD (THREAD_SIZE - INT_FRAME_SIZE -
> > EXC_LVL_SIZE)
> > -
> > #ifdef CONFIG_SMP
> > #define BOOKE_LOAD_EXC_LEVEL_STACK(level) \
> > mfspr r8,SPRN_PIR; \
> > slwi r8,r8,2; \
> > addis r8,r8,level##_STACK_BASE@ha; \
> > lwz r8,level##_STACK_BASE@l(r8); \
> > - addi r8,r8,EXC_LVL_FRAME_OVERHEAD;
> > + addi r8,r8,THREAD_SIZE - INT_FRAME_SIZE;
> > #else
> > #define BOOKE_LOAD_EXC_LEVEL_STACK(level) \
> > lis r8,level##_STACK_BASE@ha; \
> > lwz r8,level##_STACK_BASE@l(r8); \
> > - addi r8,r8,EXC_LVL_FRAME_OVERHEAD;
> > + addi r8,r8,THREAD_SIZE - INT_FRAME_SIZE;
> > #endif
> >
> > /*
> > @@ -208,7 +206,7 @@ ALT_FTR_SECTION_END_IFSET(CPU_FTR_EMB_HV)
> > mtmsr r11; \
> > mfspr r11,SPRN_SPRG_THREAD; /* if from user, start at
> > top of */\
> > lwz r11, TASK_STACK - THREAD(r11); /* this thread's
> > kernel stack */\
> > - addi r11,r11,EXC_LVL_FRAME_OVERHEAD; /* allocate stack
> > frame */\
> > + addi r11,r11,THREAD_SIZE - INT_FRAME_SIZE; /* allocate
> > stack frame */\
> > beq 1f;
> > \
> > /* COMING FROM USER MODE
> > */ \
> > stw r9,_CCR(r11); /* save
> > CR */\
> > @@ -516,24 +514,5 @@ ALT_FTR_SECTION_END_IFSET(CPU_FTR_EMB_HV)
> > bl kernel_fp_unavailable_exception;
> > \
> > b interrupt_return
> >
> > -#else /* __ASSEMBLY__ */
> > -struct exception_regs {
> > - unsigned long mas0;
> > - unsigned long mas1;
> > - unsigned long mas2;
> > - unsigned long mas3;
> > - unsigned long mas6;
> > - unsigned long mas7;
> > - unsigned long srr0;
> > - unsigned long srr1;
> > - unsigned long csrr0;
> > - unsigned long csrr1;
> > - unsigned long dsrr0;
> > - unsigned long dsrr1;
> > -};
> > -
> > -/* ensure this structure is always sized to a multiple of the
> > stack alignment */
> > -#define STACK_EXC_LVL_FRAME_SIZE ALIGN(sizeof (struct
> > exception_regs), 16)
> > -
> > #endif /* __ASSEMBLY__ */
> > #endif /* __HEAD_BOOKE_H__ */
> >
^ permalink raw reply
* [Bug 213961] Oops while loading radeon driver
From: bugzilla-daemon @ 2021-08-04 13:30 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <bug-213961-206035@https.bugzilla.kernel.org/>
https://bugzilla.kernel.org/show_bug.cgi?id=213961
--- Comment #3 from Elimar Riesebieter (riesebie@lxtec.de) ---
Created attachment 298197
--> https://bugzilla.kernel.org/attachment.cgi?id=298197&action=edit
.config
--
You may reply to this email to add a comment.
You are receiving this mail because:
You are watching the assignee of the bug.
^ permalink raw reply
* [Bug 213961] Oops while loading radeon driver
From: bugzilla-daemon @ 2021-08-04 13:29 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <bug-213961-206035@https.bugzilla.kernel.org/>
https://bugzilla.kernel.org/show_bug.cgi?id=213961
--- Comment #2 from Elimar Riesebieter (riesebie@lxtec.de) ---
Created attachment 298195
--> https://bugzilla.kernel.org/attachment.cgi?id=298195&action=edit
dmesg dump
--
You may reply to this email to add a comment.
You are receiving this mail because:
You are watching the assignee of the bug.
^ permalink raw reply
* Re: [PATCH] powerpc/32s: Fix napping restore in data storage interrupt (DSI)
From: Christophe Leroy @ 2021-08-04 13:28 UTC (permalink / raw)
To: Nicholas Piggin
Cc: userm57, Finn Thain, linux-kernel, Paul Mackerras, linuxppc-dev
In-Reply-To: <1628068469.gv4bl1fw7w.astroid@bobo.none>
Le 04/08/2021 à 13:36, Nicholas Piggin a écrit :
> Excerpts from Christophe Leroy's message of August 4, 2021 4:21 pm:
>> Hi Nic,
>>
>> I think I'll need your help on that one.
>>
>> Le 04/08/2021 à 08:07, Christophe Leroy a écrit :
>>>
>>>
>>> Le 04/08/2021 à 06:04, Finn Thain a écrit :
>
> Hi Finn!
>
>>>> On Tue, 3 Aug 2021, Christophe Leroy wrote:
>>>>
>> ...
>>>>
>>>> ------------[ cut here ]------------
>>>> kernel BUG at arch/powerpc/kernel/interrupt.c:49!
>>>> Oops: Exception in kernel mode, sig: 5 [#1]
>>>> BE PAGE_SIZE=4K MMU=Hash SMP NR_CPUS=2 PowerMac
>>>> Modules linked in:
>>>> CPU: 0 PID: 1859 Comm: xfce4-session Not tainted 5.13.0-pmac-VMAP #10
>>>> NIP: c0011474 LR: c0011464 CTR: 00000000
>>>> REGS: e2f75e40 TRAP: 0700 Not tainted (5.13.0-pmac-VMAP)
>>>> MSR: 00021032 <ME,IR,DR,RI> CR: 2400446c XER: 20000000
>>>>
>>>> GPR00: c001604c e2f75f00 ca284a60 00000000 00000000 a5205eb0 00000008 00000020
>>>> GPR08: ffffffc0 00000001 501200d9 ce030005 ca285010 00c1f778 00000000 00000000
>>>> GPR16: 00945b20 009402f8 00000001 a6b87550 a51fd000 afb73220 a6b22c78 a6a6aecc
>>>> GPR24: 00000000 ffffffc0 00000020 00000008 a5205eb0 00000000 e2f75f40 000000ae
>>>> NIP [c0011474] system_call_exception+0x60/0x164
>>>> LR [c0011464] system_call_exception+0x50/0x164
>>>> Call Trace:
>>>> [e2f75f00] [00009000] 0x9000 (unreliable)
>>>> [e2f75f30] [c001604c] ret_from_syscall+0x0/0x28
>>>> --- interrupt: c00 at 0xa69d6cb0
>>>> NIP: a69d6cb0 LR: a69d6c3c CTR: 00000000
>>>> REGS: e2f75f40 TRAP: 0c00 Not tainted (5.13.0-pmac-VMAP)
>>>> MSR: 0000d032 <EE,PR,ME,IR,DR,RI> CR: 2400446c XER: 20000000
>>>>
>>>> GPR00: 000000ae a5205de0 a5687ca0 00000000 00000000 a5205eb0 00000008 00000020
>>>> GPR08: ffffffc0 401201ea 401200d9 ffffffff c158f230 00c1f778 00000000 00000000
>>>> GPR16: 00945b20 009402f8 00000001 a6b87550 a51fd000 afb73220 a6b22c78 a6a6aecc
>>>> GPR24: afb72fc8 00000000 00000001 a5205f30 afb733dc 00000000 a6b85ff4 a5205eb0
>>>> NIP [a69d6cb0] 0xa69d6cb0
>>>> LR [a69d6c3c] 0xa69d6c3c
>>>> --- interrupt: c00
>>>> Instruction dump:
>>>> 7cdb3378 93810020 7cbc2b78 93a10024 7c9d2378 93e1002c 7d3f4b78 4800d629
>>>> 817e0084 931e0088 69690002 5529fffe <0f090000> 69694000 552997fe 0f090000
>>>> ---[ end trace c66c6c3c44806276 ]---
>>>>
>>
>> Getting a BUG at arch/powerpc/kernel/interrupt.c:49 meaning MSR_RI is not set, but the c00 interrupt
>> frame shows MSR_RI properly set, so what ?
>
> Could the stack be correct but regs pointer incorrect?
>
> Instruction dump is
>
> 0: 78 33 db 7c mr r27,r6
> 4: 20 00 81 93 stw r28,32(r1)
> 8: 78 2b bc 7c mr r28,r5
> c: 24 00 a1 93 stw r29,36(r1)
> 10: 78 23 9d 7c mr r29,r4
> 14: 2c 00 e1 93 stw r31,44(r1)
> 18: 78 4b 3f 7d mr r31,r9
> 1c: 29 d6 00 48 bl 0xd644
> 20: 84 00 7e 81 lwz r11,132(r30)
> 24: 88 00 1e 93 stw r24,136(r30)
> 28: 02 00 69 69 xori r9,r11,2
> 2c: fe ff 29 55 rlwinm r9,r9,31,31,31
> 30: 00 00 09 0f twnei r9,0
> 34: 00 40 69 69 xori r9,r11,16384
> 38: fe 97 29 55 rlwinm r9,r9,18,31,31
> 3c: 00 00 09 0f twnei r9,0
>
> regs->msr is in r11 == 0xce030005 so some kernel address?
>
> r1 == 0xe2f75f00
> r30 == 0xe2f75f40
>
> I think that matches if the function allocates 48 bytes of stack.
> STACK_FRAME_OVERHEAD is 16, so the difference would be 0x40 in that
> case. Seems okay.
>
> I'm not sure then. Can you get a hash fault interrupt come in here
> because of the vmap stack access and clobber r11? Hmm...
>
> fast_hash_page_return:
> andis. r10, r9, SRR1_ISI_NOPT@h /* Set on ISI, cleared on DSI */
>
> Is that really right? DSI can set this bit for NOHPTE as well no?
On DSI, the error bits are in DSISR while they are in SRR1 on ISI.
r9 is supposed to contain SRR1 in both cases. Powerpc 32 bits programming manual explicitely says
that bits 1-4 and 10-15 of SRR1 are cleared on DSI.
Thanks,
Christophe
^ permalink raw reply
* Re: [PATCH printk v1 03/10] kgdb: delay roundup if holding printk cpulock
From: Petr Mladek @ 2021-08-04 12:31 UTC (permalink / raw)
To: John Ogness
Cc: Gautham R. Shenoy, Douglas Anderson, Srikar Dronamraju,
Peter Zijlstra, linux-kernel, Paul Mackerras, H. Peter Anvin,
Chengyang Fan, Daniel Thompson, Bhaskar Chowdhury, x86,
Ingo Molnar, kgdb-bugreport, Nicholas Piggin, Borislav Petkov,
Steven Rostedt, Thomas Gleixner, Gustavo A. R. Silva,
Sergey Senozhatsky, Jason Wessel, linuxppc-dev,
Cédric Le Goater
In-Reply-To: <87tuk635rb.fsf@jogness.linutronix.de>
On Tue 2021-08-03 17:36:32, John Ogness wrote:
> On 2021-08-03, Daniel Thompson <daniel.thompson@linaro.org> wrote:
> > On Tue, Aug 03, 2021 at 03:18:54PM +0206, John Ogness wrote:
> >> kgdb makes use of its own cpulock (@dbg_master_lock, @kgdb_active)
> >> during cpu roundup. This will conflict with the printk cpulock.
> >
> >> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> >> index 3d0c933937b4..1b546e117f10 100644
> >> --- a/kernel/printk/printk.c
> >> +++ b/kernel/printk/printk.c
> >> @@ -214,6 +215,7 @@ int devkmsg_sysctl_set_loglvl(struct ctl_table *table, int write,
> >> #ifdef CONFIG_SMP
> >> static atomic_t printk_cpulock_owner = ATOMIC_INIT(-1);
> >> static atomic_t printk_cpulock_nested = ATOMIC_INIT(0);
> >> +static unsigned int kgdb_cpu = -1;
> >
> > Is this the flag to provoke retriggering? It appears to be a write-only
> > variable (at least in this patch). How is it consumed?
>
> Critical catch! Thank you. I am quite unhappy to see these hunks were
> accidentally dropped when generating this series.
>
> @@ -3673,6 +3675,9 @@ EXPORT_SYMBOL(__printk_cpu_trylock);
> */
> void __printk_cpu_unlock(void)
> {
> + bool trigger_kgdb = false;
> + unsigned int cpu;
> +
> if (atomic_read(&printk_cpulock_nested)) {
> atomic_dec(&printk_cpulock_nested);
> return;
> @@ -3683,6 +3688,12 @@ void __printk_cpu_unlock(void)
> * LMM(__printk_cpu_unlock:A)
> */
>
> + cpu = smp_processor_id();
> + if (kgdb_cpu == cpu) {
> + trigger_kgdb = true;
> + kgdb_cpu = -1;
> + }
Just in case that this approach is used in the end.
This code looks racy. kgdb_roundup_delay() seems to be called in NMI
context. NMI might happen at this point and set kgdb_cpu after
it was checked.
I am afraid that it won't be easy to make this safe using a single
global variable. A solution might be a per-CPU variable set
by kgdb_roundup_delay() when it owns printk_cpu_lock.
__printk_cpu_unlock() would call kgdb_roundup_cpu(cpu) when
the variable is set.
Nit: The name "kgdb_cpu" is too generic. It is not clear what is
so special about this CPU. I would call the per-CPU variable
"kgdb_delayed_roundup" or so.
Best Regards,
Petr
> /*
> * Guarantee loads and stores from this CPU when it was the
> * lock owner are visible to the next lock owner. This pairs
> @@ -3703,6 +3714,21 @@ void __printk_cpu_unlock(void)
> */
> atomic_set_release(&printk_cpulock_owner,
> -1); /* LMM(__printk_cpu_unlock:B) */
> +
> + if (trigger_kgdb) {
> + pr_warn("re-triggering kgdb roundup for CPU#%d\n", cpu);
> + kgdb_roundup_cpu(cpu);
> + }
> }
^ permalink raw reply
* Re: [PATCH printk v1 03/10] kgdb: delay roundup if holding printk cpulock
From: Petr Mladek @ 2021-08-04 12:12 UTC (permalink / raw)
To: Daniel Thompson
Cc: Gautham R. Shenoy, Douglas Anderson, Srikar Dronamraju,
Peter Zijlstra, linux-kernel, Paul Mackerras, H. Peter Anvin,
Chengyang Fan, Bhaskar Chowdhury, x86, Ingo Molnar,
kgdb-bugreport, John Ogness, Nicholas Piggin, Borislav Petkov,
Steven Rostedt, Thomas Gleixner, Gustavo A. R. Silva,
Sergey Senozhatsky, Jason Wessel, linuxppc-dev,
Cédric Le Goater
In-Reply-To: <20210804113159.lsnoyylifg6v5i35@maple.lan>
On Wed 2021-08-04 12:31:59, Daniel Thompson wrote:
> On Tue, Aug 03, 2021 at 05:36:32PM +0206, John Ogness wrote:
> > On 2021-08-03, Daniel Thompson <daniel.thompson@linaro.org> wrote:
> > > On Tue, Aug 03, 2021 at 03:18:54PM +0206, John Ogness wrote:
> > >> kgdb makes use of its own cpulock (@dbg_master_lock, @kgdb_active)
> > >> during cpu roundup. This will conflict with the printk cpulock.
> > >
> > > When the full vision is realized what will be the purpose of the printk
> > > cpulock?
> > >
> > > I'm asking largely because it's current role is actively unhelpful
> > > w.r.t. kdb. It is possible that cautious use of in_dbg_master() might
> > > be a better (and safer) solution. However it sounds like there is a
> > > larger role planned for the printk cpulock...
> >
> > The printk cpulock is used as a synchronization mechanism for
> > implementing atomic consoles, which need to be able to safely interrupt
> > the console write() activity at any time and immediately continue with
> > their own printing. The ultimate goal is to move all console printing
> > into per-console dedicated kthreads, so the primary function of the
> > printk cpulock is really to immediately _stop_ the CPU/kthread
> > performing write() in order to allow write_atomic() (from any context on
> > any CPU) to safely and reliably take over.
>
> I see.
>
> Is there any mileage in allowing in_dbg_master() to suppress taking
> the console lock?
>
> There's a couple of reasons to worry about the current approach.
>
> The first is that we don't want this code to trigger in the case when
> kgdb is enabled and kdb is not since it is only kdb (a self-hosted
> debugger) than uses the consoles. This case is relatively trivial to
> address since we can rename it kdb_roundup_delay() and alter the way it
> is conditionally compiled.
>
> The second is more of a problem however. kdb will only call into the
> console code from the debug master. By default this is the CPU that
> takes the debug trap so initial prints will work fine. However it is
> possible to switch to a different master (so we can read per-CPU
> registers and things like that). This will result in one of the CPUs
> that did the IPI round up calling into console code and this is unsafe
> in that instance.
>
> There are a couple of tricks we could adopt to work around this but
> given the slightly odd calling context for kdb (all CPUs quiesced, no
> log interleaving possible) it sounds like it would remain safe to
> bypass the lock if in_dbg_master() is true.
>
> Bypassing an inconvenient lock might sound icky but:
>
> 1. If the lock is not owned by any CPU then what kdb will do is safe.
>
> 2. If the lock is owned by any CPU then we have quiesced it anyway
> and this makes is safe for the owning CPU to share its ownership
> (since it isn't much different to recursive acquisition on a single
> CPU)
I think about the following:
void kgdb_roundup_cpus(void)
{
__printk_cpu_lock();
__kgdb_roundup_cpus();
}
, where __printk_cpu_lock() waits/takes printk_cpu_lock()
__kgdb_roundup_cpus() is the original kgdb_roundup_cpus();
The idea is that kgdb_roundup_cpus() caller takes the printk_cpu lock.
The owner will be well defined.
As a result any other CPU will not be able to take the printk_cpu lock
as long as it is owned by the kgdb lock. But as you say, kgdb will
make sure that everything is serialized at this stage. So that
the original raw_printk_cpu_lock_irqsave() might just disable
IRQs when called under debugger.
Does it make any sense?
I have to say that it is a bit hairy. But it looks slightly better
than the delayed/repeated IPI proposed by this patch.
Best Regards,
Petr
^ permalink raw reply
* Re: [PATCH] powerpc/32s: Fix napping restore in data storage interrupt (DSI)
From: Nicholas Piggin @ 2021-08-04 11:36 UTC (permalink / raw)
To: Christophe Leroy
Cc: userm57, Finn Thain, linux-kernel, Paul Mackerras, linuxppc-dev
In-Reply-To: <8fb08f68-ed01-65f9-fb9e-66abf2b18a00@csgroup.eu>
Excerpts from Christophe Leroy's message of August 4, 2021 4:21 pm:
> Hi Nic,
>
> I think I'll need your help on that one.
>
> Le 04/08/2021 à 08:07, Christophe Leroy a écrit :
>>
>>
>> Le 04/08/2021 à 06:04, Finn Thain a écrit :
Hi Finn!
>>> On Tue, 3 Aug 2021, Christophe Leroy wrote:
>>>
> ...
>>>
>>> ------------[ cut here ]------------
>>> kernel BUG at arch/powerpc/kernel/interrupt.c:49!
>>> Oops: Exception in kernel mode, sig: 5 [#1]
>>> BE PAGE_SIZE=4K MMU=Hash SMP NR_CPUS=2 PowerMac
>>> Modules linked in:
>>> CPU: 0 PID: 1859 Comm: xfce4-session Not tainted 5.13.0-pmac-VMAP #10
>>> NIP: c0011474 LR: c0011464 CTR: 00000000
>>> REGS: e2f75e40 TRAP: 0700 Not tainted (5.13.0-pmac-VMAP)
>>> MSR: 00021032 <ME,IR,DR,RI> CR: 2400446c XER: 20000000
>>>
>>> GPR00: c001604c e2f75f00 ca284a60 00000000 00000000 a5205eb0 00000008 00000020
>>> GPR08: ffffffc0 00000001 501200d9 ce030005 ca285010 00c1f778 00000000 00000000
>>> GPR16: 00945b20 009402f8 00000001 a6b87550 a51fd000 afb73220 a6b22c78 a6a6aecc
>>> GPR24: 00000000 ffffffc0 00000020 00000008 a5205eb0 00000000 e2f75f40 000000ae
>>> NIP [c0011474] system_call_exception+0x60/0x164
>>> LR [c0011464] system_call_exception+0x50/0x164
>>> Call Trace:
>>> [e2f75f00] [00009000] 0x9000 (unreliable)
>>> [e2f75f30] [c001604c] ret_from_syscall+0x0/0x28
>>> --- interrupt: c00 at 0xa69d6cb0
>>> NIP: a69d6cb0 LR: a69d6c3c CTR: 00000000
>>> REGS: e2f75f40 TRAP: 0c00 Not tainted (5.13.0-pmac-VMAP)
>>> MSR: 0000d032 <EE,PR,ME,IR,DR,RI> CR: 2400446c XER: 20000000
>>>
>>> GPR00: 000000ae a5205de0 a5687ca0 00000000 00000000 a5205eb0 00000008 00000020
>>> GPR08: ffffffc0 401201ea 401200d9 ffffffff c158f230 00c1f778 00000000 00000000
>>> GPR16: 00945b20 009402f8 00000001 a6b87550 a51fd000 afb73220 a6b22c78 a6a6aecc
>>> GPR24: afb72fc8 00000000 00000001 a5205f30 afb733dc 00000000 a6b85ff4 a5205eb0
>>> NIP [a69d6cb0] 0xa69d6cb0
>>> LR [a69d6c3c] 0xa69d6c3c
>>> --- interrupt: c00
>>> Instruction dump:
>>> 7cdb3378 93810020 7cbc2b78 93a10024 7c9d2378 93e1002c 7d3f4b78 4800d629
>>> 817e0084 931e0088 69690002 5529fffe <0f090000> 69694000 552997fe 0f090000
>>> ---[ end trace c66c6c3c44806276 ]---
>>>
>
> Getting a BUG at arch/powerpc/kernel/interrupt.c:49 meaning MSR_RI is not set, but the c00 interrupt
> frame shows MSR_RI properly set, so what ?
Could the stack be correct but regs pointer incorrect?
Instruction dump is
0: 78 33 db 7c mr r27,r6
4: 20 00 81 93 stw r28,32(r1)
8: 78 2b bc 7c mr r28,r5
c: 24 00 a1 93 stw r29,36(r1)
10: 78 23 9d 7c mr r29,r4
14: 2c 00 e1 93 stw r31,44(r1)
18: 78 4b 3f 7d mr r31,r9
1c: 29 d6 00 48 bl 0xd644
20: 84 00 7e 81 lwz r11,132(r30)
24: 88 00 1e 93 stw r24,136(r30)
28: 02 00 69 69 xori r9,r11,2
2c: fe ff 29 55 rlwinm r9,r9,31,31,31
30: 00 00 09 0f twnei r9,0
34: 00 40 69 69 xori r9,r11,16384
38: fe 97 29 55 rlwinm r9,r9,18,31,31
3c: 00 00 09 0f twnei r9,0
regs->msr is in r11 == 0xce030005 so some kernel address?
r1 == 0xe2f75f00
r30 == 0xe2f75f40
I think that matches if the function allocates 48 bytes of stack.
STACK_FRAME_OVERHEAD is 16, so the difference would be 0x40 in that
case. Seems okay.
I'm not sure then. Can you get a hash fault interrupt come in here
because of the vmap stack access and clobber r11? Hmm...
fast_hash_page_return:
andis. r10, r9, SRR1_ISI_NOPT@h /* Set on ISI, cleared on DSI */
Is that really right? DSI can set this bit for NOHPTE as well no?
That'd do it.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH printk v1 03/10] kgdb: delay roundup if holding printk cpulock
From: Daniel Thompson @ 2021-08-04 11:31 UTC (permalink / raw)
To: John Ogness
Cc: Gautham R. Shenoy, Douglas Anderson, Srikar Dronamraju,
Peter Zijlstra, linux-kernel, Paul Mackerras, H. Peter Anvin,
Chengyang Fan, Bhaskar Chowdhury, x86, Ingo Molnar,
kgdb-bugreport, Petr Mladek, Nicholas Piggin, Borislav Petkov,
Steven Rostedt, Thomas Gleixner, Gustavo A. R. Silva,
Sergey Senozhatsky, Jason Wessel, linuxppc-dev,
Cédric Le Goater
In-Reply-To: <87tuk635rb.fsf@jogness.linutronix.de>
On Tue, Aug 03, 2021 at 05:36:32PM +0206, John Ogness wrote:
> On 2021-08-03, Daniel Thompson <daniel.thompson@linaro.org> wrote:
> > On Tue, Aug 03, 2021 at 03:18:54PM +0206, John Ogness wrote:
> >> kgdb makes use of its own cpulock (@dbg_master_lock, @kgdb_active)
> >> during cpu roundup. This will conflict with the printk cpulock.
> >
> > When the full vision is realized what will be the purpose of the printk
> > cpulock?
> >
> > I'm asking largely because it's current role is actively unhelpful
> > w.r.t. kdb. It is possible that cautious use of in_dbg_master() might
> > be a better (and safer) solution. However it sounds like there is a
> > larger role planned for the printk cpulock...
>
> The printk cpulock is used as a synchronization mechanism for
> implementing atomic consoles, which need to be able to safely interrupt
> the console write() activity at any time and immediately continue with
> their own printing. The ultimate goal is to move all console printing
> into per-console dedicated kthreads, so the primary function of the
> printk cpulock is really to immediately _stop_ the CPU/kthread
> performing write() in order to allow write_atomic() (from any context on
> any CPU) to safely and reliably take over.
I see.
Is there any mileage in allowing in_dbg_master() to suppress taking
the console lock?
There's a couple of reasons to worry about the current approach.
The first is that we don't want this code to trigger in the case when
kgdb is enabled and kdb is not since it is only kdb (a self-hosted
debugger) than uses the consoles. This case is relatively trivial to
address since we can rename it kdb_roundup_delay() and alter the way it
is conditionally compiled.
The second is more of a problem however. kdb will only call into the
console code from the debug master. By default this is the CPU that
takes the debug trap so initial prints will work fine. However it is
possible to switch to a different master (so we can read per-CPU
registers and things like that). This will result in one of the CPUs
that did the IPI round up calling into console code and this is unsafe
in that instance.
There are a couple of tricks we could adopt to work around this but
given the slightly odd calling context for kdb (all CPUs quiesced, no
log interleaving possible) it sounds like it would remain safe to
bypass the lock if in_dbg_master() is true.
Bypassing an inconvenient lock might sound icky but:
1. If the lock is not owned by any CPU then what kdb will do is safe.
2. If the lock is owned by any CPU then we have quiesced it anyway
and this makes is safe for the owning CPU to share its ownership
(since it isn't much different to recursive acquisition on a single
CPU)
> Atomic consoles are actually quite similar to the kgdb_io ops. For
> example, comparing:
>
> serial8250_console_write_atomic() + serial8250_console_putchar_locked()
>
> with
>
> serial8250_put_poll_char()
>
> The difference is that serial8250_console_write_atomic() is line-based
> and synchronizing with serial8250_console_write() so that if the kernel
> crashes while outputing to the console, write() can be interrupted by
> write_atomic() and cleanly formatted crash data can be output.
>
> Also serial8250_put_poll_char() is calling into __pm_runtime_resume(),
> which includes a spinlock and possibly sleeping. This would not be
> acceptable for atomic consoles.
spinlocks aren't allowed in polled I/O either.
However IIRC there is a rather nasty trick being played here to allow
code sharing. I believe there was a deliberate unbalanced resume in the
poll_init() function that results (again IIRC) in the PM calls in
poll_char() becoming nothing but atomic add and subtract (e.g. enabling
polled I/O effectively suppresses PM activity).
Daniel.
> Although, as Andy pointed out [0], I
> will need to figure out how to deal with suspended consoles. Or just
> implement a policy that registered atomic consoles may never be
> suspended.
>
> I had not considered merging kgdb_io ops with atomic console ops. But
> now that I look at it more closely, there may be some useful overlap. I
> will consider this. Thank you for this idea.
>
> >> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> >> index 3d0c933937b4..1b546e117f10 100644
> >> --- a/kernel/printk/printk.c
> >> +++ b/kernel/printk/printk.c
> >> @@ -214,6 +215,7 @@ int devkmsg_sysctl_set_loglvl(struct ctl_table *table, int write,
> >> #ifdef CONFIG_SMP
> >> static atomic_t printk_cpulock_owner = ATOMIC_INIT(-1);
> >> static atomic_t printk_cpulock_nested = ATOMIC_INIT(0);
> >> +static unsigned int kgdb_cpu = -1;
> >
> > Is this the flag to provoke retriggering? It appears to be a write-only
> > variable (at least in this patch). How is it consumed?
>
> Critical catch! Thank you. I am quite unhappy to see these hunks were
> accidentally dropped when generating this series.
>
> @@ -3673,6 +3675,9 @@ EXPORT_SYMBOL(__printk_cpu_trylock);
> */
> void __printk_cpu_unlock(void)
> {
> + bool trigger_kgdb = false;
> + unsigned int cpu;
> +
> if (atomic_read(&printk_cpulock_nested)) {
> atomic_dec(&printk_cpulock_nested);
> return;
> @@ -3683,6 +3688,12 @@ void __printk_cpu_unlock(void)
> * LMM(__printk_cpu_unlock:A)
> */
>
> + cpu = smp_processor_id();
> + if (kgdb_cpu == cpu) {
> + trigger_kgdb = true;
> + kgdb_cpu = -1;
> + }
> +
> /*
> * Guarantee loads and stores from this CPU when it was the
> * lock owner are visible to the next lock owner. This pairs
> @@ -3703,6 +3714,21 @@ void __printk_cpu_unlock(void)
> */
> atomic_set_release(&printk_cpulock_owner,
> -1); /* LMM(__printk_cpu_unlock:B) */
> +
> + if (trigger_kgdb) {
> + pr_warn("re-triggering kgdb roundup for CPU#%d\n", cpu);
> + kgdb_roundup_cpu(cpu);
> + }
> }
> EXPORT_SYMBOL(__printk_cpu_unlock);
>
> John Ogness
>
> [0] https://lore.kernel.org/lkml/YQlKAeXS9MPmE284@smile.fi.intel.com
^ permalink raw reply
* Re: [PATCH v2 1/2] sched/topology: Skip updating masks for non-online nodes
From: Valentin Schneider @ 2021-08-04 10:20 UTC (permalink / raw)
To: Srikar Dronamraju
Cc: Nathan Lynch, Gautham R Shenoy, Vincent Guittot, Rik van Riel,
Peter Zijlstra, linuxppc-dev, Geetika Moolchandani, LKML,
Dietmar Eggemann, Thomas Gleixner, Laurent Dufour, Mel Gorman,
Ingo Molnar
In-Reply-To: <20210804100155.GE4072958@linux.vnet.ibm.com>
On 04/08/21 15:31, Srikar Dronamraju wrote:
> * Srikar Dronamraju <srikar@linux.vnet.ibm.com> [2021-07-23 20:09:14]:
>
>> * Valentin Schneider <valentin.schneider@arm.com> [2021-07-13 17:32:14]:
>>
>> > On 12/07/21 18:18, Srikar Dronamraju wrote:
>> > > Hi Valentin,
>> > >
>> > >> On 01/07/21 09:45, Srikar Dronamraju wrote:
>> > >> > @@ -1891,12 +1894,30 @@ void sched_init_numa(void)
>> > >> > void sched_domains_numa_masks_set(unsigned int cpu)
>> > >> > {
>> > >
>
> Hey Valentin / Peter
>
> Did you get a chance to look at this?
>
Barely, I wanted to set some time aside to stare at this and have been
failing miserably. Let me bump it up my todolist, I'll get to it before the
end of the week.
> --
> Thanks and Regards
> Srikar Dronamraju
^ permalink raw reply
* Re: [PATCH v2 1/2] sched/topology: Skip updating masks for non-online nodes
From: Srikar Dronamraju @ 2021-08-04 10:01 UTC (permalink / raw)
To: Valentin Schneider
Cc: Nathan Lynch, Gautham R Shenoy, Vincent Guittot, Rik van Riel,
Peter Zijlstra, linuxppc-dev, Geetika Moolchandani, LKML,
Dietmar Eggemann, Thomas Gleixner, Laurent Dufour, Mel Gorman,
Ingo Molnar
In-Reply-To: <20210723143914.GI3836887@linux.vnet.ibm.com>
* Srikar Dronamraju <srikar@linux.vnet.ibm.com> [2021-07-23 20:09:14]:
> * Valentin Schneider <valentin.schneider@arm.com> [2021-07-13 17:32:14]:
>
> > On 12/07/21 18:18, Srikar Dronamraju wrote:
> > > Hi Valentin,
> > >
> > >> On 01/07/21 09:45, Srikar Dronamraju wrote:
> > >> > @@ -1891,12 +1894,30 @@ void sched_init_numa(void)
> > >> > void sched_domains_numa_masks_set(unsigned int cpu)
> > >> > {
> > >
Hey Valentin / Peter
Did you get a chance to look at this?
--
Thanks and Regards
Srikar Dronamraju
^ permalink raw reply
* Re: [PATCH] powerpc: Remove MSR_PR check in interrupt_exit_{user/kernel}_prepare()
From: Nicholas Piggin @ 2021-08-04 8:53 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Christophe Leroy, Michael Ellerman,
Paul Mackerras
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <cd5f54fd-fbf4-e471-9971-1e8c86755754@csgroup.eu>
Excerpts from Christophe Leroy's message of August 4, 2021 6:37 pm:
>
>
> Le 04/08/2021 à 10:08, Nicholas Piggin a écrit :
>> Excerpts from Christophe Leroy's message of August 4, 2021 3:27 pm:
>>> In those hot functions that are called at every interrupt, any saved
>>> cycle is worth it.
>>>
>>> interrupt_exit_user_prepare() and interrupt_exit_kernel_prepare() are
>>> called from three places:
>>> - From entry_32.S
>>> - From interrupt_64.S
>>> - From interrupt_exit_user_restart() and interrupt_exit_kernel_restart()
>>>
>>> In entry_32.S, there are inambiguously called based on MSR_PR:
>>>
>>> interrupt_return:
>>> lwz r4,_MSR(r1)
>>> addi r3,r1,STACK_FRAME_OVERHEAD
>>> andi. r0,r4,MSR_PR
>>> beq .Lkernel_interrupt_return
>>> bl interrupt_exit_user_prepare
>>> ...
>>> .Lkernel_interrupt_return:
>>> bl interrupt_exit_kernel_prepare
>>>
>>> In interrupt_64.S, that's similar:
>>>
>>> interrupt_return_\srr\():
>>> ld r4,_MSR(r1)
>>> andi. r0,r4,MSR_PR
>>> beq interrupt_return_\srr\()_kernel
>>> interrupt_return_\srr\()_user: /* make backtraces match the _kernel variant */
>>> addi r3,r1,STACK_FRAME_OVERHEAD
>>> bl interrupt_exit_user_prepare
>>> ...
>>> interrupt_return_\srr\()_kernel:
>>> addi r3,r1,STACK_FRAME_OVERHEAD
>>> bl interrupt_exit_kernel_prepare
>>>
>>> In interrupt_exit_user_restart() and interrupt_exit_kernel_restart(),
>>> MSR_PR is verified respectively by BUG_ON(!user_mode(regs)) and
>>> BUG_ON(user_mode(regs)) prior to calling interrupt_exit_user_prepare()
>>> and interrupt_exit_kernel_prepare().
>>>
>>> The verification in interrupt_exit_user_prepare() and
>>> interrupt_exit_kernel_prepare() are therefore useless and can be removed.
>>>
>>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>>
>> Probably okay to do now things are ironing out.
>>
>> Unless we want to make a new define for interrupt handler debug and put
>> a bunch of these asserts under it. There's quite a lot more here, and
>> in asm/interrupt.h, etc.
>
> But that one is so trivial that I'm not sure there is any point in keeping it even as a kind of
> additional DEBUG level, unless you want those BUG_ONs because you don't trust the compiler.
Fair point.
Acked-by: Nicholas Piggin <npiggin@gmail.com>
^ permalink raw reply
* Re: [PATCH] powerpc: Remove MSR_PR check in interrupt_exit_{user/kernel}_prepare()
From: Christophe Leroy @ 2021-08-04 8:37 UTC (permalink / raw)
To: Nicholas Piggin, Benjamin Herrenschmidt, Michael Ellerman,
Paul Mackerras
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <1628064412.48kzr1eula.astroid@bobo.none>
Le 04/08/2021 à 10:08, Nicholas Piggin a écrit :
> Excerpts from Christophe Leroy's message of August 4, 2021 3:27 pm:
>> In those hot functions that are called at every interrupt, any saved
>> cycle is worth it.
>>
>> interrupt_exit_user_prepare() and interrupt_exit_kernel_prepare() are
>> called from three places:
>> - From entry_32.S
>> - From interrupt_64.S
>> - From interrupt_exit_user_restart() and interrupt_exit_kernel_restart()
>>
>> In entry_32.S, there are inambiguously called based on MSR_PR:
>>
>> interrupt_return:
>> lwz r4,_MSR(r1)
>> addi r3,r1,STACK_FRAME_OVERHEAD
>> andi. r0,r4,MSR_PR
>> beq .Lkernel_interrupt_return
>> bl interrupt_exit_user_prepare
>> ...
>> .Lkernel_interrupt_return:
>> bl interrupt_exit_kernel_prepare
>>
>> In interrupt_64.S, that's similar:
>>
>> interrupt_return_\srr\():
>> ld r4,_MSR(r1)
>> andi. r0,r4,MSR_PR
>> beq interrupt_return_\srr\()_kernel
>> interrupt_return_\srr\()_user: /* make backtraces match the _kernel variant */
>> addi r3,r1,STACK_FRAME_OVERHEAD
>> bl interrupt_exit_user_prepare
>> ...
>> interrupt_return_\srr\()_kernel:
>> addi r3,r1,STACK_FRAME_OVERHEAD
>> bl interrupt_exit_kernel_prepare
>>
>> In interrupt_exit_user_restart() and interrupt_exit_kernel_restart(),
>> MSR_PR is verified respectively by BUG_ON(!user_mode(regs)) and
>> BUG_ON(user_mode(regs)) prior to calling interrupt_exit_user_prepare()
>> and interrupt_exit_kernel_prepare().
>>
>> The verification in interrupt_exit_user_prepare() and
>> interrupt_exit_kernel_prepare() are therefore useless and can be removed.
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>
> Probably okay to do now things are ironing out.
>
> Unless we want to make a new define for interrupt handler debug and put
> a bunch of these asserts under it. There's quite a lot more here, and
> in asm/interrupt.h, etc.
But that one is so trivial that I'm not sure there is any point in keeping it even as a kind of
additional DEBUG level, unless you want those BUG_ONs because you don't trust the compiler.
Christophe
>
> Thanks,
> Nick
>
>> ---
>> arch/powerpc/kernel/interrupt.c | 2 --
>> 1 file changed, 2 deletions(-)
>>
>> diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/interrupt.c
>> index 21bbd615ca41..f26caf911ab5 100644
>> --- a/arch/powerpc/kernel/interrupt.c
>> +++ b/arch/powerpc/kernel/interrupt.c
>> @@ -465,7 +465,6 @@ notrace unsigned long interrupt_exit_user_prepare(struct pt_regs *regs)
>>
>> if (!IS_ENABLED(CONFIG_BOOKE) && !IS_ENABLED(CONFIG_40x))
>> BUG_ON(!(regs->msr & MSR_RI));
>> - BUG_ON(!(regs->msr & MSR_PR));
>> BUG_ON(arch_irq_disabled_regs(regs));
>> CT_WARN_ON(ct_state() == CONTEXT_USER);
>>
>> @@ -499,7 +498,6 @@ notrace unsigned long interrupt_exit_kernel_prepare(struct pt_regs *regs)
>> if (!IS_ENABLED(CONFIG_BOOKE) && !IS_ENABLED(CONFIG_40x) &&
>> unlikely(!(regs->msr & MSR_RI)))
>> unrecoverable_exception(regs);
>> - BUG_ON(regs->msr & MSR_PR);
>> /*
>> * CT_WARN_ON comes here via program_check_exception,
>> * so avoid recursion.
>> --
>> 2.25.0
>>
>>
^ permalink raw reply
* Re: [PATCH] powerpc: Remove MSR_PR check in interrupt_exit_{user/kernel}_prepare()
From: Nicholas Piggin @ 2021-08-04 8:08 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Christophe Leroy, Michael Ellerman,
Paul Mackerras
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <b36623df00ef3d2296f928487b6e23f93a217afa.1628054802.git.christophe.leroy@csgroup.eu>
Excerpts from Christophe Leroy's message of August 4, 2021 3:27 pm:
> In those hot functions that are called at every interrupt, any saved
> cycle is worth it.
>
> interrupt_exit_user_prepare() and interrupt_exit_kernel_prepare() are
> called from three places:
> - From entry_32.S
> - From interrupt_64.S
> - From interrupt_exit_user_restart() and interrupt_exit_kernel_restart()
>
> In entry_32.S, there are inambiguously called based on MSR_PR:
>
> interrupt_return:
> lwz r4,_MSR(r1)
> addi r3,r1,STACK_FRAME_OVERHEAD
> andi. r0,r4,MSR_PR
> beq .Lkernel_interrupt_return
> bl interrupt_exit_user_prepare
> ...
> .Lkernel_interrupt_return:
> bl interrupt_exit_kernel_prepare
>
> In interrupt_64.S, that's similar:
>
> interrupt_return_\srr\():
> ld r4,_MSR(r1)
> andi. r0,r4,MSR_PR
> beq interrupt_return_\srr\()_kernel
> interrupt_return_\srr\()_user: /* make backtraces match the _kernel variant */
> addi r3,r1,STACK_FRAME_OVERHEAD
> bl interrupt_exit_user_prepare
> ...
> interrupt_return_\srr\()_kernel:
> addi r3,r1,STACK_FRAME_OVERHEAD
> bl interrupt_exit_kernel_prepare
>
> In interrupt_exit_user_restart() and interrupt_exit_kernel_restart(),
> MSR_PR is verified respectively by BUG_ON(!user_mode(regs)) and
> BUG_ON(user_mode(regs)) prior to calling interrupt_exit_user_prepare()
> and interrupt_exit_kernel_prepare().
>
> The verification in interrupt_exit_user_prepare() and
> interrupt_exit_kernel_prepare() are therefore useless and can be removed.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Probably okay to do now things are ironing out.
Unless we want to make a new define for interrupt handler debug and put
a bunch of these asserts under it. There's quite a lot more here, and
in asm/interrupt.h, etc.
Thanks,
Nick
> ---
> arch/powerpc/kernel/interrupt.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/interrupt.c
> index 21bbd615ca41..f26caf911ab5 100644
> --- a/arch/powerpc/kernel/interrupt.c
> +++ b/arch/powerpc/kernel/interrupt.c
> @@ -465,7 +465,6 @@ notrace unsigned long interrupt_exit_user_prepare(struct pt_regs *regs)
>
> if (!IS_ENABLED(CONFIG_BOOKE) && !IS_ENABLED(CONFIG_40x))
> BUG_ON(!(regs->msr & MSR_RI));
> - BUG_ON(!(regs->msr & MSR_PR));
> BUG_ON(arch_irq_disabled_regs(regs));
> CT_WARN_ON(ct_state() == CONTEXT_USER);
>
> @@ -499,7 +498,6 @@ notrace unsigned long interrupt_exit_kernel_prepare(struct pt_regs *regs)
> if (!IS_ENABLED(CONFIG_BOOKE) && !IS_ENABLED(CONFIG_40x) &&
> unlikely(!(regs->msr & MSR_RI)))
> unrecoverable_exception(regs);
> - BUG_ON(regs->msr & MSR_PR);
> /*
> * CT_WARN_ON comes here via program_check_exception,
> * so avoid recursion.
> --
> 2.25.0
>
>
^ permalink raw reply
* Re: [RFC PATCH] powerpc/book3s64/radix: Upgrade va tlbie to PID tlbie if we cross PMD_SIZE
From: Peter Zijlstra @ 2021-08-04 7:34 UTC (permalink / raw)
To: Nicholas Piggin
Cc: linux-arch, Will Deacon, Aneesh Kumar K.V, linux-mm, linuxppc-dev
In-Reply-To: <1628058455.rphrivzjzb.astroid@bobo.none>
On Wed, Aug 04, 2021 at 04:39:44PM +1000, Nicholas Piggin wrote:
> For that matter, I wonder if we shouldn't do something like this
> (untested) so the low level batch flush has visibility to the high
> level flush range.
>
> x86 could use this too AFAIKS, just needs to pass the range a bit
> further down, but in practice I'm not sure it would ever really
> matter for them because the 2MB level exceeds the single page flush
> ceiling for 4k pages unlike powerpc with 64k pages. But in corner
> cases where the unmap crossed a bunch of small vmas or the ceiling
> was increased then in theory it could be of use.
x86 can do single invalidates for 2M pages if that's the only type
encountered in the range, at which point we can do 33*2M = 66M, which is
well below the 1G range for the next level of huge.
> Subject: [PATCH v1] mm/mmu_gather: provide archs with the entire range that is
> to be flushed, not just the particular gather
>
> This allows archs to optimise flushing heuristics better, in the face of
> flush operations forcing smaller flush batches. For example, an
> architecture may choose a more costly per-page invalidation for small
> ranges of pages with the assumption that the full TLB flush cost would
> be more expensive in terms of refills. However if a very large range is
> forced into flushing small ranges, the faster full-process flush may
> have been the better choice.
What is splitting up the flushes for you? That page_size power-special?
^ permalink raw reply
* Re: [RFC PATCH] powerpc/book3s64/radix: Upgrade va tlbie to PID tlbie if we cross PMD_SIZE
From: Michael Ellerman @ 2021-08-04 6:59 UTC (permalink / raw)
To: Nicholas Piggin, Aneesh Kumar K.V, linuxppc-dev
In-Reply-To: <1628053302.0qclx0xcj9.astroid@bobo.none>
Nicholas Piggin <npiggin@gmail.com> writes:
> Excerpts from Aneesh Kumar K.V's message of August 4, 2021 12:37 am:
>> With shared mapping, even though we are unmapping a large range, the kernel
>> will force a TLB flush with ptl lock held to avoid the race mentioned in
>> commit 1cf35d47712d ("mm: split 'tlb_flush_mmu()' into tlb flushing and memory freeing parts")
>> This results in the kernel issuing a high number of TLB flushes even for a large
>> range. This can be improved by making sure the kernel switch to pid based flush if the
>> kernel is unmapping a 2M range.
>
> It would be good to have a bit more description here.
>
> In any patch that changes a heuristic like this, I would like to see
> some justification or reasoning that could be refuted or used as a
> supporting argument if we ever wanted to change the heuristic later.
> Ideally with some of the obvious downsides listed as well.
>
> This "improves" things here, but what if it hurt things elsewhere, how
> would we come in later and decide to change it back?
>
> THP flushes for example, I think now they'll do PID flushes (if they
> have to be broadcast, which they will tend to be when khugepaged does
> them). So now that might increase jitter for THP and cause it to be a
> loss for more workloads.
>
> So where do you notice this? What's the benefit?
Ack. Needs some numbers and supporting evidence.
>> diff --git a/arch/powerpc/mm/book3s64/radix_tlb.c b/arch/powerpc/mm/book3s64/radix_tlb.c
>> index aefc100d79a7..21d0f098e43b 100644
>> --- a/arch/powerpc/mm/book3s64/radix_tlb.c
>> +++ b/arch/powerpc/mm/book3s64/radix_tlb.c
>> @@ -1106,7 +1106,7 @@ EXPORT_SYMBOL(radix__flush_tlb_kernel_range);
>> * invalidating a full PID, so it has a far lower threshold to change from
>> * individual page flushes to full-pid flushes.
>> */
>> -static unsigned long tlb_single_page_flush_ceiling __read_mostly = 33;
>> +static unsigned long tlb_single_page_flush_ceiling __read_mostly = 32;
>> static unsigned long tlb_local_single_page_flush_ceiling __read_mostly = POWER9_TLB_SETS_RADIX * 2;
>>
>> static inline void __radix__flush_tlb_range(struct mm_struct *mm,
>> @@ -1133,7 +1133,7 @@ static inline void __radix__flush_tlb_range(struct mm_struct *mm,
>> if (fullmm)
>> flush_pid = true;
>> else if (type == FLUSH_TYPE_GLOBAL)
>> - flush_pid = nr_pages > tlb_single_page_flush_ceiling;
>> + flush_pid = nr_pages >= tlb_single_page_flush_ceiling;
>
> Arguably >= is nicer than > here, but this shouldn't be in the same
> patch as the value change.
>
>> else
>> flush_pid = nr_pages > tlb_local_single_page_flush_ceiling;
>
> And it should change everything to be consistent. Although I'm not sure
> it's worth changing even though I highly doubt any administrator would
> be tweaking this.
This made me look at how an administrator tweaks these thresholds, and
AFAICS the answer is "recompile the kernel"?
It looks like x86 have a debugfs file for tlb_single_page_flush_ceiling,
but we don't. I guess we meant to copy that but never did?
So at the moment both thresholds could just be #defines.
Making them tweakable at runtime would be nice, it would give us an
escape hatch if we ever hit a workload in production that wants a
different value.
cheers
^ permalink raw reply
* Re: [RFC PATCH] powerpc/book3s64/radix: Upgrade va tlbie to PID tlbie if we cross PMD_SIZE
From: Nicholas Piggin @ 2021-08-04 6:39 UTC (permalink / raw)
To: Aneesh Kumar K.V, linuxppc-dev, mpe
Cc: Peter Zijlstra, linux-arch, Will Deacon, linux-mm
In-Reply-To: <1628053302.0qclx0xcj9.astroid@bobo.none>
Excerpts from Nicholas Piggin's message of August 4, 2021 3:14 pm:
> Excerpts from Aneesh Kumar K.V's message of August 4, 2021 12:37 am:
>> With shared mapping, even though we are unmapping a large range, the kernel
>> will force a TLB flush with ptl lock held to avoid the race mentioned in
>> commit 1cf35d47712d ("mm: split 'tlb_flush_mmu()' into tlb flushing and memory freeing parts")
>> This results in the kernel issuing a high number of TLB flushes even for a large
>> range. This can be improved by making sure the kernel switch to pid based flush if the
>> kernel is unmapping a 2M range.
>
> It would be good to have a bit more description here.
>
> In any patch that changes a heuristic like this, I would like to see
> some justification or reasoning that could be refuted or used as a
> supporting argument if we ever wanted to change the heuristic later.
> Ideally with some of the obvious downsides listed as well.
>
> This "improves" things here, but what if it hurt things elsewhere, how
> would we come in later and decide to change it back?
>
> THP flushes for example, I think now they'll do PID flushes (if they
> have to be broadcast, which they will tend to be when khugepaged does
> them). So now that might increase jitter for THP and cause it to be a
> loss for more workloads.
>
> So where do you notice this? What's the benefit?
For that matter, I wonder if we shouldn't do something like this
(untested) so the low level batch flush has visibility to the high
level flush range.
x86 could use this too AFAIKS, just needs to pass the range a bit
further down, but in practice I'm not sure it would ever really
matter for them because the 2MB level exceeds the single page flush
ceiling for 4k pages unlike powerpc with 64k pages. But in corner
cases where the unmap crossed a bunch of small vmas or the ceiling
was increased then in theory it could be of use.
Subject: [PATCH v1] mm/mmu_gather: provide archs with the entire range that is
to be flushed, not just the particular gather
This allows archs to optimise flushing heuristics better, in the face of
flush operations forcing smaller flush batches. For example, an
architecture may choose a more costly per-page invalidation for small
ranges of pages with the assumption that the full TLB flush cost would
be more expensive in terms of refills. However if a very large range is
forced into flushing small ranges, the faster full-process flush may
have been the better choice.
---
arch/powerpc/mm/book3s64/radix_tlb.c | 33 ++++++++++++++++------------
fs/exec.c | 3 ++-
include/asm-generic/tlb.h | 9 ++++++++
include/linux/mm_types.h | 3 ++-
mm/hugetlb.c | 2 +-
mm/madvise.c | 6 ++---
mm/memory.c | 4 ++--
mm/mmap.c | 2 +-
mm/mmu_gather.c | 10 ++++++---
mm/oom_kill.c | 2 +-
10 files changed, 47 insertions(+), 27 deletions(-)
diff --git a/arch/powerpc/mm/book3s64/radix_tlb.c b/arch/powerpc/mm/book3s64/radix_tlb.c
index aefc100d79a7..e1072d85d72e 100644
--- a/arch/powerpc/mm/book3s64/radix_tlb.c
+++ b/arch/powerpc/mm/book3s64/radix_tlb.c
@@ -1110,12 +1110,13 @@ static unsigned long tlb_single_page_flush_ceiling __read_mostly = 33;
static unsigned long tlb_local_single_page_flush_ceiling __read_mostly = POWER9_TLB_SETS_RADIX * 2;
static inline void __radix__flush_tlb_range(struct mm_struct *mm,
- unsigned long start, unsigned long end)
+ unsigned long start, unsigned long end,
+ unsigned long entire_range)
{
unsigned long pid;
unsigned int page_shift = mmu_psize_defs[mmu_virtual_psize].shift;
unsigned long page_size = 1UL << page_shift;
- unsigned long nr_pages = (end - start) >> page_shift;
+ unsigned long entire_nr_pages = entire_range >> page_shift;
bool fullmm = (end == TLB_FLUSH_ALL);
bool flush_pid, flush_pwc = false;
enum tlb_flush_type type;
@@ -1133,9 +1134,9 @@ static inline void __radix__flush_tlb_range(struct mm_struct *mm,
if (fullmm)
flush_pid = true;
else if (type == FLUSH_TYPE_GLOBAL)
- flush_pid = nr_pages > tlb_single_page_flush_ceiling;
+ flush_pid = entire_nr_pages > tlb_single_page_flush_ceiling;
else
- flush_pid = nr_pages > tlb_local_single_page_flush_ceiling;
+ flush_pid = entire_nr_pages > tlb_local_single_page_flush_ceiling;
/*
* full pid flush already does the PWC flush. if it is not full pid
* flush check the range is more than PMD and force a pwc flush
@@ -1220,7 +1221,7 @@ void radix__flush_tlb_range(struct vm_area_struct *vma, unsigned long start,
return radix__flush_hugetlb_tlb_range(vma, start, end);
#endif
- __radix__flush_tlb_range(vma->vm_mm, start, end);
+ __radix__flush_tlb_range(vma->vm_mm, start, end, end - start);
}
EXPORT_SYMBOL(radix__flush_tlb_range);
@@ -1278,6 +1279,11 @@ void radix__flush_all_lpid_guest(unsigned int lpid)
_tlbie_lpid_guest(lpid, RIC_FLUSH_ALL);
}
+static void __radix__flush_tlb_range_psize(struct mm_struct *mm,
+ unsigned long start, unsigned long end,
+ unsigned long entire_range,
+ int psize, bool also_pwc);
+
void radix__tlb_flush(struct mmu_gather *tlb)
{
int psize = 0;
@@ -1285,6 +1291,7 @@ void radix__tlb_flush(struct mmu_gather *tlb)
int page_size = tlb->page_size;
unsigned long start = tlb->start;
unsigned long end = tlb->end;
+ unsigned long entire_range = tlb->entire_end - tlb->entire_start;
/*
* if page size is not something we understand, do a full mm flush
@@ -1301,21 +1308,19 @@ void radix__tlb_flush(struct mmu_gather *tlb)
else
radix__flush_all_mm(mm);
} else {
- if (!tlb->freed_tables)
- radix__flush_tlb_range_psize(mm, start, end, psize);
- else
- radix__flush_tlb_pwc_range_psize(mm, start, end, psize);
+ __radix__flush_tlb_range_psize(mm, start, end, entire_range, psize, tlb->freed_tables);
}
}
static void __radix__flush_tlb_range_psize(struct mm_struct *mm,
unsigned long start, unsigned long end,
+ unsigned long entire_range,
int psize, bool also_pwc)
{
unsigned long pid;
unsigned int page_shift = mmu_psize_defs[psize].shift;
unsigned long page_size = 1UL << page_shift;
- unsigned long nr_pages = (end - start) >> page_shift;
+ unsigned long entire_nr_pages = entire_range >> page_shift;
bool fullmm = (end == TLB_FLUSH_ALL);
bool flush_pid;
enum tlb_flush_type type;
@@ -1335,9 +1340,9 @@ static void __radix__flush_tlb_range_psize(struct mm_struct *mm,
if (fullmm)
flush_pid = true;
else if (type == FLUSH_TYPE_GLOBAL)
- flush_pid = nr_pages > tlb_single_page_flush_ceiling;
+ flush_pid = entire_nr_pages > tlb_single_page_flush_ceiling;
else
- flush_pid = nr_pages > tlb_local_single_page_flush_ceiling;
+ flush_pid = entire_nr_pages > tlb_local_single_page_flush_ceiling;
if (!mmu_has_feature(MMU_FTR_GTSE) && type == FLUSH_TYPE_GLOBAL) {
unsigned long tgt = H_RPTI_TARGET_CMMU;
@@ -1381,13 +1386,13 @@ static void __radix__flush_tlb_range_psize(struct mm_struct *mm,
void radix__flush_tlb_range_psize(struct mm_struct *mm, unsigned long start,
unsigned long end, int psize)
{
- return __radix__flush_tlb_range_psize(mm, start, end, psize, false);
+ return __radix__flush_tlb_range_psize(mm, start, end, end - start, psize, false);
}
void radix__flush_tlb_pwc_range_psize(struct mm_struct *mm, unsigned long start,
unsigned long end, int psize)
{
- __radix__flush_tlb_range_psize(mm, start, end, psize, true);
+ __radix__flush_tlb_range_psize(mm, start, end, end - start, psize, true);
}
#ifdef CONFIG_TRANSPARENT_HUGEPAGE
diff --git a/fs/exec.c b/fs/exec.c
index 38f63451b928..c769c12bdf56 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -705,11 +705,11 @@ static int shift_arg_pages(struct vm_area_struct *vma, unsigned long shift)
return -ENOMEM;
lru_add_drain();
- tlb_gather_mmu(&tlb, mm);
if (new_end > old_start) {
/*
* when the old and new regions overlap clear from new_end.
*/
+ tlb_gather_mmu(&tlb, mm, new_end, old_end);
free_pgd_range(&tlb, new_end, old_end, new_end,
vma->vm_next ? vma->vm_next->vm_start : USER_PGTABLES_CEILING);
} else {
@@ -719,6 +719,7 @@ static int shift_arg_pages(struct vm_area_struct *vma, unsigned long shift)
* have constraints on va-space that make this illegal (IA64) -
* for the others its just a little faster.
*/
+ tlb_gather_mmu(&tlb, mm, old_start, old_end);
free_pgd_range(&tlb, old_start, old_end, new_end,
vma->vm_next ? vma->vm_next->vm_start : USER_PGTABLES_CEILING);
}
diff --git a/include/asm-generic/tlb.h b/include/asm-generic/tlb.h
index 2c68a545ffa7..857fd83af695 100644
--- a/include/asm-generic/tlb.h
+++ b/include/asm-generic/tlb.h
@@ -256,6 +256,15 @@ struct mmu_gather {
struct mmu_table_batch *batch;
#endif
+ /*
+ * This is the range of the "entire" logical flush
+ * operation being performed. It does not relate to
+ * the current batch to flush, but it can inform
+ * heuristics that choose the best flushing strategy.
+ */
+ unsigned long entire_start;
+ unsigned long entire_end;
+
unsigned long start;
unsigned long end;
/*
diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
index 52bbd2b7cb46..9d2ff06b574c 100644
--- a/include/linux/mm_types.h
+++ b/include/linux/mm_types.h
@@ -599,7 +599,8 @@ static inline cpumask_t *mm_cpumask(struct mm_struct *mm)
}
struct mmu_gather;
-extern void tlb_gather_mmu(struct mmu_gather *tlb, struct mm_struct *mm);
+extern void tlb_gather_mmu(struct mmu_gather *tlb, struct mm_struct *mm,
+ unsigned long start, unsigned long end);
extern void tlb_gather_mmu_fullmm(struct mmu_gather *tlb, struct mm_struct *mm);
extern void tlb_finish_mmu(struct mmu_gather *tlb);
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index dfc940d5221d..e41106eb4df7 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -4458,7 +4458,7 @@ void unmap_hugepage_range(struct vm_area_struct *vma, unsigned long start,
{
struct mmu_gather tlb;
- tlb_gather_mmu(&tlb, vma->vm_mm);
+ tlb_gather_mmu(&tlb, vma->vm_mm, start, end);
__unmap_hugepage_range(&tlb, vma, start, end, ref_page);
tlb_finish_mmu(&tlb);
}
diff --git a/mm/madvise.c b/mm/madvise.c
index 6d3d348b17f4..b3634672aeb9 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -508,7 +508,7 @@ static long madvise_cold(struct vm_area_struct *vma,
return -EINVAL;
lru_add_drain();
- tlb_gather_mmu(&tlb, mm);
+ tlb_gather_mmu(&tlb, mm, start_addr, end_addr);
madvise_cold_page_range(&tlb, vma, start_addr, end_addr);
tlb_finish_mmu(&tlb);
@@ -561,7 +561,7 @@ static long madvise_pageout(struct vm_area_struct *vma,
return 0;
lru_add_drain();
- tlb_gather_mmu(&tlb, mm);
+ tlb_gather_mmu(&tlb, mm, start_addr, end_addr);
madvise_pageout_page_range(&tlb, vma, start_addr, end_addr);
tlb_finish_mmu(&tlb);
@@ -726,7 +726,7 @@ static int madvise_free_single_vma(struct vm_area_struct *vma,
range.start, range.end);
lru_add_drain();
- tlb_gather_mmu(&tlb, mm);
+ tlb_gather_mmu(&tlb, mm, range.start, range.end);
update_hiwater_rss(mm);
mmu_notifier_invalidate_range_start(&range);
diff --git a/mm/memory.c b/mm/memory.c
index 25fc46e87214..61c303e84baf 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -1647,7 +1647,7 @@ void zap_page_range(struct vm_area_struct *vma, unsigned long start,
lru_add_drain();
mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, vma, vma->vm_mm,
start, start + size);
- tlb_gather_mmu(&tlb, vma->vm_mm);
+ tlb_gather_mmu(&tlb, vma->vm_mm, range.start, range.end);
update_hiwater_rss(vma->vm_mm);
mmu_notifier_invalidate_range_start(&range);
for ( ; vma && vma->vm_start < range.end; vma = vma->vm_next)
@@ -1674,7 +1674,7 @@ static void zap_page_range_single(struct vm_area_struct *vma, unsigned long addr
lru_add_drain();
mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, vma, vma->vm_mm,
address, address + size);
- tlb_gather_mmu(&tlb, vma->vm_mm);
+ tlb_gather_mmu(&tlb, vma->vm_mm, address, range.end);
update_hiwater_rss(vma->vm_mm);
mmu_notifier_invalidate_range_start(&range);
unmap_single_vma(&tlb, vma, address, range.end, details);
diff --git a/mm/mmap.c b/mm/mmap.c
index ca54d36d203a..f2808febde40 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -2672,7 +2672,7 @@ static void unmap_region(struct mm_struct *mm,
struct mmu_gather tlb;
lru_add_drain();
- tlb_gather_mmu(&tlb, mm);
+ tlb_gather_mmu(&tlb, mm, start, end);
update_hiwater_rss(mm);
unmap_vmas(&tlb, vma, start, end);
free_pgtables(&tlb, vma, prev ? prev->vm_end : FIRST_USER_ADDRESS,
diff --git a/mm/mmu_gather.c b/mm/mmu_gather.c
index 1b9837419bf9..863a5bd7e650 100644
--- a/mm/mmu_gather.c
+++ b/mm/mmu_gather.c
@@ -250,9 +250,12 @@ void tlb_flush_mmu(struct mmu_gather *tlb)
}
static void __tlb_gather_mmu(struct mmu_gather *tlb, struct mm_struct *mm,
+ unsigned long start, unsigned long end,
bool fullmm)
{
tlb->mm = mm;
+ tlb->entire_start = start;
+ tlb->entire_end = end;
tlb->fullmm = fullmm;
#ifndef CONFIG_MMU_GATHER_NO_GATHER
@@ -281,9 +284,10 @@ static void __tlb_gather_mmu(struct mmu_gather *tlb, struct mm_struct *mm,
* Called to initialize an (on-stack) mmu_gather structure for page-table
* tear-down from @mm.
*/
-void tlb_gather_mmu(struct mmu_gather *tlb, struct mm_struct *mm)
+void tlb_gather_mmu(struct mmu_gather *tlb, struct mm_struct *mm,
+ unsigned long start, unsigned long end)
{
- __tlb_gather_mmu(tlb, mm, false);
+ __tlb_gather_mmu(tlb, mm, start, end, false);
}
/**
@@ -299,7 +303,7 @@ void tlb_gather_mmu(struct mmu_gather *tlb, struct mm_struct *mm)
*/
void tlb_gather_mmu_fullmm(struct mmu_gather *tlb, struct mm_struct *mm)
{
- __tlb_gather_mmu(tlb, mm, true);
+ __tlb_gather_mmu(tlb, mm, 0, ~0UL, true);
}
/**
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index c729a4c4a1ac..bfcc9cbdfb20 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -545,7 +545,7 @@ bool __oom_reap_task_mm(struct mm_struct *mm)
mmu_notifier_range_init(&range, MMU_NOTIFY_UNMAP, 0,
vma, mm, vma->vm_start,
vma->vm_end);
- tlb_gather_mmu(&tlb, mm);
+ tlb_gather_mmu(&tlb, mm, vma->vm_start, vma->vm_end);
if (mmu_notifier_invalidate_range_start_nonblock(&range)) {
tlb_finish_mmu(&tlb);
ret = false;
--
2.23.0
^ permalink raw reply related
* Re: Debian SID kernel doesn't boot on PowerBook 3400c
From: Christophe Leroy @ 2021-08-04 6:28 UTC (permalink / raw)
To: Finn Thain; +Cc: Debian PowerPC, linuxppc-dev, Stan Johnson
In-Reply-To: <c535cc2b-3f45-2415-1e81-32ea24b4ec@linux-m68k.org>
Le 04/08/2021 à 02:34, Finn Thain a écrit :
>
> On Tue, 3 Aug 2021, Christophe Leroy wrote:
>
>>
>> Looks like the memory errors are linked to KUAP (Kernel Userspace Access
>> Protection). Based on the places the problems happen, I don't think
>> there are any invalid access, so there must be something wrong in the
>> KUAP logic, probably linked to some interrupts happenning in kernel mode
>> while the KUAP window is opened. And because is not selected by default
>> on book3s/32 until 5.14, probably nobody ever tested it in a real
>> environment before you.
>>
>> I think the issue may be linked to commit
>> https://github.com/linuxppc/linux/commit/c16728835 which happened
>> between 5.12 and 5.13.
>
> The messages, "Kernel attempted to write user page (c6207c) - exploit
> attempt? (uid: 0)", appear in the console logs generated by v5.13. Those
> logs come from the Powerbook G3 discussion in the other thread. Could that
> be the same bug?
>
Yes, most likely.
So you confirm this appears with 5.13 and not 5.12 ?
Can you check if they happen at commit c16728835
Can you check if they DO NOT happen at preceding commit c16728835~
Could you test without CONFIG_PPC_KUAP
Could you test with CONFIG_PPC_KUAP and CONFIG_PPC_KUAP_DEBUG
Thanks
Christophe
^ permalink raw reply
* Re: [PATCH] powerpc/32s: Fix napping restore in data storage interrupt (DSI)
From: Christophe Leroy @ 2021-08-04 6:21 UTC (permalink / raw)
To: Nicholas Piggin
Cc: Finn Thain, userm57, linux-kernel, Paul Mackerras, linuxppc-dev
In-Reply-To: <b04a90a9-9d62-2192-f896-ea99be911604@csgroup.eu>
Hi Nic,
I think I'll need your help on that one.
Le 04/08/2021 à 08:07, Christophe Leroy a écrit :
>
>
> Le 04/08/2021 à 06:04, Finn Thain a écrit :
>> On Tue, 3 Aug 2021, Christophe Leroy wrote:
>>
...
>>
>> ------------[ cut here ]------------
>> kernel BUG at arch/powerpc/kernel/interrupt.c:49!
>> Oops: Exception in kernel mode, sig: 5 [#1]
>> BE PAGE_SIZE=4K MMU=Hash SMP NR_CPUS=2 PowerMac
>> Modules linked in:
>> CPU: 0 PID: 1859 Comm: xfce4-session Not tainted 5.13.0-pmac-VMAP #10
>> NIP: c0011474 LR: c0011464 CTR: 00000000
>> REGS: e2f75e40 TRAP: 0700 Not tainted (5.13.0-pmac-VMAP)
>> MSR: 00021032 <ME,IR,DR,RI> CR: 2400446c XER: 20000000
>>
>> GPR00: c001604c e2f75f00 ca284a60 00000000 00000000 a5205eb0 00000008 00000020
>> GPR08: ffffffc0 00000001 501200d9 ce030005 ca285010 00c1f778 00000000 00000000
>> GPR16: 00945b20 009402f8 00000001 a6b87550 a51fd000 afb73220 a6b22c78 a6a6aecc
>> GPR24: 00000000 ffffffc0 00000020 00000008 a5205eb0 00000000 e2f75f40 000000ae
>> NIP [c0011474] system_call_exception+0x60/0x164
>> LR [c0011464] system_call_exception+0x50/0x164
>> Call Trace:
>> [e2f75f00] [00009000] 0x9000 (unreliable)
>> [e2f75f30] [c001604c] ret_from_syscall+0x0/0x28
>> --- interrupt: c00 at 0xa69d6cb0
>> NIP: a69d6cb0 LR: a69d6c3c CTR: 00000000
>> REGS: e2f75f40 TRAP: 0c00 Not tainted (5.13.0-pmac-VMAP)
>> MSR: 0000d032 <EE,PR,ME,IR,DR,RI> CR: 2400446c XER: 20000000
>>
>> GPR00: 000000ae a5205de0 a5687ca0 00000000 00000000 a5205eb0 00000008 00000020
>> GPR08: ffffffc0 401201ea 401200d9 ffffffff c158f230 00c1f778 00000000 00000000
>> GPR16: 00945b20 009402f8 00000001 a6b87550 a51fd000 afb73220 a6b22c78 a6a6aecc
>> GPR24: afb72fc8 00000000 00000001 a5205f30 afb733dc 00000000 a6b85ff4 a5205eb0
>> NIP [a69d6cb0] 0xa69d6cb0
>> LR [a69d6c3c] 0xa69d6c3c
>> --- interrupt: c00
>> Instruction dump:
>> 7cdb3378 93810020 7cbc2b78 93a10024 7c9d2378 93e1002c 7d3f4b78 4800d629
>> 817e0084 931e0088 69690002 5529fffe <0f090000> 69694000 552997fe 0f090000
>> ---[ end trace c66c6c3c44806276 ]---
>>
Getting a BUG at arch/powerpc/kernel/interrupt.c:49 meaning MSR_RI is not set, but the c00 interrupt
frame shows MSR_RI properly set, so what ?
Thanks
Christophe
^ permalink raw reply
* [powerpc:next-test] BUILD SUCCESS c09f799bf0fdc0eeb44db87df07a7a9632c3420c
From: kernel test robot @ 2021-08-04 6:18 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev
tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next-test
branch HEAD: c09f799bf0fdc0eeb44db87df07a7a9632c3420c pseries/drmem: update LMBs after LPM
elapsed time: 1007m
configs tested: 100
configs skipped: 3
The following configs have been built successfully.
More configs may be tested in the coming days.
gcc tested configs:
arm defconfig
arm64 allyesconfig
arm64 defconfig
arm allyesconfig
arm allmodconfig
i386 randconfig-c001-20210803
powerpc tqm8541_defconfig
nios2 3c120_defconfig
arm pleb_defconfig
powerpc pseries_defconfig
arm trizeps4_defconfig
arm imote2_defconfig
powerpc bluestone_defconfig
mips rm200_defconfig
powerpc ppc64e_defconfig
powerpc ppc6xx_defconfig
sh sh7710voipgw_defconfig
powerpc allnoconfig
powerpc mpc8540_ads_defconfig
powerpc ebony_defconfig
powerpc bamboo_defconfig
powerpc kilauea_defconfig
mips bcm63xx_defconfig
powerpc mpc8315_rdb_defconfig
xtensa generic_kc705_defconfig
mips ip27_defconfig
powerpc64 defconfig
arm sama5_defconfig
sh sdk7786_defconfig
arc vdk_hs38_smp_defconfig
x86_64 allnoconfig
ia64 allmodconfig
ia64 defconfig
ia64 allyesconfig
m68k allmodconfig
m68k defconfig
m68k allyesconfig
nds32 defconfig
nios2 allyesconfig
csky defconfig
alpha defconfig
alpha allyesconfig
xtensa allyesconfig
h8300 allyesconfig
arc defconfig
sh allmodconfig
parisc defconfig
s390 allyesconfig
s390 allmodconfig
parisc allyesconfig
s390 defconfig
i386 allyesconfig
sparc allyesconfig
sparc defconfig
i386 defconfig
nios2 defconfig
arc allyesconfig
nds32 allnoconfig
mips allyesconfig
mips allmodconfig
powerpc allyesconfig
powerpc allmodconfig
x86_64 randconfig-a002-20210803
x86_64 randconfig-a004-20210803
x86_64 randconfig-a006-20210803
x86_64 randconfig-a003-20210803
x86_64 randconfig-a001-20210803
x86_64 randconfig-a005-20210803
i386 randconfig-a004-20210802
i386 randconfig-a005-20210802
i386 randconfig-a002-20210802
i386 randconfig-a006-20210802
i386 randconfig-a001-20210802
i386 randconfig-a003-20210802
i386 randconfig-a012-20210803
i386 randconfig-a011-20210803
i386 randconfig-a015-20210803
i386 randconfig-a013-20210803
i386 randconfig-a014-20210803
i386 randconfig-a016-20210803
riscv nommu_k210_defconfig
riscv allyesconfig
riscv nommu_virt_defconfig
riscv allnoconfig
riscv defconfig
riscv rv32_defconfig
riscv allmodconfig
um x86_64_defconfig
um i386_defconfig
x86_64 allyesconfig
x86_64 rhel-8.3-kselftests
x86_64 defconfig
x86_64 rhel-8.3
x86_64 kexec
clang tested configs:
x86_64 randconfig-a012-20210803
x86_64 randconfig-a016-20210803
x86_64 randconfig-a013-20210803
x86_64 randconfig-a011-20210803
x86_64 randconfig-a014-20210803
x86_64 randconfig-a015-20210803
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
^ 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