* Re: [PATCH 3/3] powerpc: use __builtin_trap() in BUG/WARN macros.
From: Segher Boessenkool @ 2019-08-19 13:23 UTC (permalink / raw)
To: Christophe Leroy; +Cc: linuxppc-dev, Paul Mackerras, linux-kernel
In-Reply-To: <20510ce03cc9463f1c9e743c1d93b939de501b53.1566219503.git.christophe.leroy@c-s.fr>
On Mon, Aug 19, 2019 at 01:06:31PM +0000, Christophe Leroy wrote:
> Note that we keep using an assembly text using "twi 31, 0, 0" for
> inconditional traps because GCC drops all code after
> __builtin_trap() when the condition is always true at build time.
As I said, it can also do this for conditional traps, if it can prove
the condition is always true.
Can you put the bug table asm *before* the __builtin_trap maybe? That
should make it all work fine... If you somehow can tell what machine
instruction is that trap, anyway.
Segher
^ permalink raw reply
* Re: [PATCH v1 05/10] powerpc/mm: Do early ioremaps from top to bottom on PPC64 too.
From: Nicholas Piggin @ 2019-08-19 13:42 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Christophe Leroy, Michael Ellerman,
Paul Mackerras
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <019c5d90f7027ccff00e38a3bcd633d290f6af59.1565726867.git.christophe.leroy@c-s.fr>
Christophe Leroy's on August 14, 2019 6:11 am:
> Until vmalloc system is up and running, ioremap basically
> allocates addresses at the border of the IOREMAP area.
>
> On PPC32, addresses are allocated down from the top of the area
> while on PPC64, addresses are allocated up from the base of the
> area.
This series looks pretty good to me, but I'm not sure about this patch.
It seems like quite a small divergence in terms of code, and it looks
like the final result still has some ifdefs in these functions. Maybe
you could just keep existing behaviour for this cleanup series so it
does not risk triggering some obscure regression? Merging behaviour
could be proposed at the end.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH v4 1/2] powerpc/time: Only set CONFIG_ARCH_HAS_SCALED_CPUTIME on PPC64
From: Nicholas Piggin @ 2019-08-19 13:56 UTC (permalink / raw)
To: Christophe Leroy
Cc: linux-kernel, Frederic Weisbecker, Paul Mackerras, linuxppc-dev
In-Reply-To: <26969bb5-c01b-0674-5773-027f1851bd44@c-s.fr>
Christophe Leroy's on August 14, 2019 4:31 pm:
> Hi Nick,
>
>
> Le 07/06/2018 à 03:43, Nicholas Piggin a écrit :
>> On Wed, 6 Jun 2018 14:21:08 +0000 (UTC)
>> Christophe Leroy <christophe.leroy@c-s.fr> wrote:
>>
>>> scaled cputime is only meaningfull when the processor has
>>> SPURR and/or PURR, which means only on PPC64.
>>>
>
> [...]
>
>>
>> I wonder if we could make this depend on PPC_PSERIES or even
>> PPC_SPLPAR as well? (That would be for a later patch)
>
> Can we go further on this ?
>
> Do we know exactly which configuration support scaled cputime, in
> extenso have SPRN_SPURR and/or SPRN_PURR ?
>
> Ref https://github.com/linuxppc/issues/issues/171
Unfortunately I don't know enough about the timing stuff and who
uses it. SPURR is available on all configurations (guest, bare metal),
so it could account scaled time there too. I guess better just leave
it for now.
Thanks,
Nick
^ permalink raw reply
* [PATCH v3 1/3] powerpc: rewrite LOAD_REG_IMMEDIATE() as an intelligent macro
From: Christophe Leroy @ 2019-08-19 13:58 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, segher
Cc: linuxppc-dev, linux-kernel
Today LOAD_REG_IMMEDIATE() is a basic #define which loads all
parts on a value into a register, including the parts that are NUL.
This means always 2 instructions on PPC32 and always 5 instructions
on PPC64. And those instructions cannot run in parallele as they are
updating the same register.
Ex: LOAD_REG_IMMEDIATE(r1,THREAD_SIZE) in head_64.S results in:
3c 20 00 00 lis r1,0
60 21 00 00 ori r1,r1,0
78 21 07 c6 rldicr r1,r1,32,31
64 21 00 00 oris r1,r1,0
60 21 40 00 ori r1,r1,16384
Rewrite LOAD_REG_IMMEDIATE() with GAS macro in order to skip
the parts that are NUL.
Rename existing LOAD_REG_IMMEDIATE() as LOAD_REG_IMMEDIATE_SYM()
and use that one for loading value of symbols which are not known
at compile time.
Now LOAD_REG_IMMEDIATE(r1,THREAD_SIZE) in head_64.S results in:
38 20 40 00 li r1,16384
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
v2: Fixed the test from (\x) & 0xffffffff to (\x) >= 0x80000000 || (\x) < -0x80000000 in __LOAD_REG_IMMEDIATE()
v3: Replaced rldicr by sldi as suggested by Segher for readability
---
arch/powerpc/include/asm/ppc_asm.h | 42 +++++++++++++++++++++++++++++++-----
arch/powerpc/kernel/exceptions-64e.S | 10 ++++-----
arch/powerpc/kernel/head_64.S | 2 +-
3 files changed, 43 insertions(+), 11 deletions(-)
diff --git a/arch/powerpc/include/asm/ppc_asm.h b/arch/powerpc/include/asm/ppc_asm.h
index e0637730a8e7..aa8717c1571a 100644
--- a/arch/powerpc/include/asm/ppc_asm.h
+++ b/arch/powerpc/include/asm/ppc_asm.h
@@ -311,13 +311,43 @@ GLUE(.,name):
addis reg,reg,(name - 0b)@ha; \
addi reg,reg,(name - 0b)@l;
-#ifdef __powerpc64__
-#ifdef HAVE_AS_ATHIGH
+#if defined(__powerpc64__) && defined(HAVE_AS_ATHIGH)
#define __AS_ATHIGH high
#else
#define __AS_ATHIGH h
#endif
-#define LOAD_REG_IMMEDIATE(reg,expr) \
+
+.macro __LOAD_REG_IMMEDIATE_32 r, x
+ .if (\x) >= 0x8000 || (\x) < -0x8000
+ lis \r, (\x)@__AS_ATHIGH
+ .if (\x) & 0xffff != 0
+ ori \r, \r, (\x)@l
+ .endif
+ .else
+ li \r, (\x)@l
+ .endif
+.endm
+
+.macro __LOAD_REG_IMMEDIATE r, x
+ .if (\x) >= 0x80000000 || (\x) < -0x80000000
+ __LOAD_REG_IMMEDIATE_32 \r, (\x) >> 32
+ sldi \r, \r, 32
+ .if (\x) & 0xffff0000 != 0
+ oris \r, \r, (\x)@__AS_ATHIGH
+ .endif
+ .if (\x) & 0xffff != 0
+ oris \r, \r, (\x)@l
+ .endif
+ .else
+ __LOAD_REG_IMMEDIATE_32 \r, \x
+ .endif
+.endm
+
+#ifdef __powerpc64__
+
+#define LOAD_REG_IMMEDIATE(reg, expr) __LOAD_REG_IMMEDIATE reg, expr
+
+#define LOAD_REG_IMMEDIATE_SYM(reg,expr) \
lis reg,(expr)@highest; \
ori reg,reg,(expr)@higher; \
rldicr reg,reg,32,31; \
@@ -335,11 +365,13 @@ GLUE(.,name):
#else /* 32-bit */
-#define LOAD_REG_IMMEDIATE(reg,expr) \
+#define LOAD_REG_IMMEDIATE(reg, expr) __LOAD_REG_IMMEDIATE_32 reg, expr
+
+#define LOAD_REG_IMMEDIATE_SYM(reg,expr) \
lis reg,(expr)@ha; \
addi reg,reg,(expr)@l;
-#define LOAD_REG_ADDR(reg,name) LOAD_REG_IMMEDIATE(reg, name)
+#define LOAD_REG_ADDR(reg,name) LOAD_REG_IMMEDIATE_SYM(reg, name)
#define LOAD_REG_ADDRBASE(reg, name) lis reg,name@ha
#define ADDROFF(name) name@l
diff --git a/arch/powerpc/kernel/exceptions-64e.S b/arch/powerpc/kernel/exceptions-64e.S
index 1cfb3da4a84a..898aae6da167 100644
--- a/arch/powerpc/kernel/exceptions-64e.S
+++ b/arch/powerpc/kernel/exceptions-64e.S
@@ -751,8 +751,8 @@ END_FTR_SECTION_IFSET(CPU_FTR_ALTIVEC)
ld r14,interrupt_base_book3e@got(r15)
ld r15,__end_interrupts@got(r15)
#else
- LOAD_REG_IMMEDIATE(r14,interrupt_base_book3e)
- LOAD_REG_IMMEDIATE(r15,__end_interrupts)
+ LOAD_REG_IMMEDIATE_SYM(r14,interrupt_base_book3e)
+ LOAD_REG_IMMEDIATE_SYM(r15,__end_interrupts)
#endif
cmpld cr0,r10,r14
cmpld cr1,r10,r15
@@ -821,8 +821,8 @@ kernel_dbg_exc:
ld r14,interrupt_base_book3e@got(r15)
ld r15,__end_interrupts@got(r15)
#else
- LOAD_REG_IMMEDIATE(r14,interrupt_base_book3e)
- LOAD_REG_IMMEDIATE(r15,__end_interrupts)
+ LOAD_REG_IMMEDIATE_SYM(r14,interrupt_base_book3e)
+ LOAD_REG_IMMEDIATE_SYM(r15,__end_interrupts)
#endif
cmpld cr0,r10,r14
cmpld cr1,r10,r15
@@ -1449,7 +1449,7 @@ a2_tlbinit_code_start:
a2_tlbinit_after_linear_map:
/* Now we branch the new virtual address mapped by this entry */
- LOAD_REG_IMMEDIATE(r3,1f)
+ LOAD_REG_IMMEDIATE_SYM(r3,1f)
mtctr r3
bctr
diff --git a/arch/powerpc/kernel/head_64.S b/arch/powerpc/kernel/head_64.S
index 91d297e696dd..1fd44761e997 100644
--- a/arch/powerpc/kernel/head_64.S
+++ b/arch/powerpc/kernel/head_64.S
@@ -635,7 +635,7 @@ __after_prom_start:
sub r5,r5,r11
#else
/* just copy interrupts */
- LOAD_REG_IMMEDIATE(r5, FIXED_SYMBOL_ABS_ADDR(__end_interrupts))
+ LOAD_REG_IMMEDIATE_SYM(r5, FIXED_SYMBOL_ABS_ADDR(__end_interrupts))
#endif
b 5f
3:
--
2.13.3
^ permalink raw reply related
* [PATCH v3 2/3] powerpc/32: replace LOAD_MSR_KERNEL() by LOAD_REG_IMMEDIATE()
From: Christophe Leroy @ 2019-08-19 13:58 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, segher
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <be2b971c89b1af30d680cedd14e99a83138ef40a.1566223054.git.christophe.leroy@c-s.fr>
LOAD_MSR_KERNEL() and LOAD_REG_IMMEDIATE() are doing the same thing
in the same way. Drop LOAD_MSR_KERNEL()
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
v2: no change
v3: no change
---
arch/powerpc/kernel/entry_32.S | 18 +++++++++---------
arch/powerpc/kernel/head_32.h | 21 ++++-----------------
2 files changed, 13 insertions(+), 26 deletions(-)
diff --git a/arch/powerpc/kernel/entry_32.S b/arch/powerpc/kernel/entry_32.S
index 54fab22c9a43..972b05504a0a 100644
--- a/arch/powerpc/kernel/entry_32.S
+++ b/arch/powerpc/kernel/entry_32.S
@@ -230,7 +230,7 @@ transfer_to_handler_cont:
*/
lis r12,reenable_mmu@h
ori r12,r12,reenable_mmu@l
- LOAD_MSR_KERNEL(r0, MSR_KERNEL)
+ LOAD_REG_IMMEDIATE(r0, MSR_KERNEL)
mtspr SPRN_SRR0,r12
mtspr SPRN_SRR1,r0
SYNC
@@ -304,7 +304,7 @@ stack_ovf:
addi r1,r1,THREAD_SIZE-STACK_FRAME_OVERHEAD
lis r9,StackOverflow@ha
addi r9,r9,StackOverflow@l
- LOAD_MSR_KERNEL(r10,MSR_KERNEL)
+ LOAD_REG_IMMEDIATE(r10,MSR_KERNEL)
#if defined(CONFIG_PPC_8xx) && defined(CONFIG_PERF_EVENTS)
mtspr SPRN_NRI, r0
#endif
@@ -324,7 +324,7 @@ trace_syscall_entry_irq_off:
bl trace_hardirqs_on
/* Now enable for real */
- LOAD_MSR_KERNEL(r10, MSR_KERNEL | MSR_EE)
+ LOAD_REG_IMMEDIATE(r10, MSR_KERNEL | MSR_EE)
mtmsr r10
REST_GPR(0, r1)
@@ -394,7 +394,7 @@ ret_from_syscall:
#endif
mr r6,r3
/* disable interrupts so current_thread_info()->flags can't change */
- LOAD_MSR_KERNEL(r10,MSR_KERNEL) /* doesn't include MSR_EE */
+ LOAD_REG_IMMEDIATE(r10,MSR_KERNEL) /* doesn't include MSR_EE */
/* Note: We don't bother telling lockdep about it */
SYNC
MTMSRD(r10)
@@ -824,7 +824,7 @@ ret_from_except:
* can't change between when we test it and when we return
* from the interrupt. */
/* Note: We don't bother telling lockdep about it */
- LOAD_MSR_KERNEL(r10,MSR_KERNEL)
+ LOAD_REG_IMMEDIATE(r10,MSR_KERNEL)
SYNC /* Some chip revs have problems here... */
MTMSRD(r10) /* disable interrupts */
@@ -991,7 +991,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_NEED_PAIRED_STWCX)
* can restart the exception exit path at the label
* exc_exit_restart below. -- paulus
*/
- LOAD_MSR_KERNEL(r10,MSR_KERNEL & ~MSR_RI)
+ LOAD_REG_IMMEDIATE(r10,MSR_KERNEL & ~MSR_RI)
SYNC
MTMSRD(r10) /* clear the RI bit */
.globl exc_exit_restart
@@ -1066,7 +1066,7 @@ exc_exit_restart_end:
REST_NVGPRS(r1); \
lwz r3,_MSR(r1); \
andi. r3,r3,MSR_PR; \
- LOAD_MSR_KERNEL(r10,MSR_KERNEL); \
+ LOAD_REG_IMMEDIATE(r10,MSR_KERNEL); \
bne user_exc_return; \
lwz r0,GPR0(r1); \
lwz r2,GPR2(r1); \
@@ -1236,7 +1236,7 @@ recheck:
* neither. Those disable/enable cycles used to peek at
* TI_FLAGS aren't advertised.
*/
- LOAD_MSR_KERNEL(r10,MSR_KERNEL)
+ LOAD_REG_IMMEDIATE(r10,MSR_KERNEL)
SYNC
MTMSRD(r10) /* disable interrupts */
lwz r9,TI_FLAGS(r2)
@@ -1329,7 +1329,7 @@ _GLOBAL(enter_rtas)
lwz r4,RTASBASE(r4)
mfmsr r9
stw r9,8(r1)
- LOAD_MSR_KERNEL(r0,MSR_KERNEL)
+ LOAD_REG_IMMEDIATE(r0,MSR_KERNEL)
SYNC /* disable interrupts so SRR0/1 */
MTMSRD(r0) /* don't get trashed */
li r9,MSR_KERNEL & ~(MSR_IR|MSR_DR)
diff --git a/arch/powerpc/kernel/head_32.h b/arch/powerpc/kernel/head_32.h
index 4a692553651f..8abc7783dbe5 100644
--- a/arch/powerpc/kernel/head_32.h
+++ b/arch/powerpc/kernel/head_32.h
@@ -5,19 +5,6 @@
#include <asm/ptrace.h> /* for STACK_FRAME_REGS_MARKER */
/*
- * MSR_KERNEL is > 0x8000 on 4xx/Book-E since it include MSR_CE.
- */
-.macro __LOAD_MSR_KERNEL r, x
-.if \x >= 0x8000
- lis \r, (\x)@h
- ori \r, \r, (\x)@l
-.else
- li \r, (\x)
-.endif
-.endm
-#define LOAD_MSR_KERNEL(r, x) __LOAD_MSR_KERNEL r, x
-
-/*
* Exception entry code. This code runs with address translation
* turned off, i.e. using physical addresses.
* We assume sprg3 has the physical address of the current
@@ -92,7 +79,7 @@
#ifdef CONFIG_40x
rlwinm r9,r9,0,14,12 /* clear MSR_WE (necessary?) */
#else
- LOAD_MSR_KERNEL(r10, MSR_KERNEL & ~(MSR_IR|MSR_DR)) /* can take exceptions */
+ LOAD_REG_IMMEDIATE(r10, MSR_KERNEL & ~(MSR_IR|MSR_DR)) /* can take exceptions */
MTMSRD(r10) /* (except for mach check in rtas) */
#endif
lis r10,STACK_FRAME_REGS_MARKER@ha /* exception frame marker */
@@ -140,10 +127,10 @@
* otherwise we might risk taking an interrupt before we tell lockdep
* they are enabled.
*/
- LOAD_MSR_KERNEL(r10, MSR_KERNEL)
+ LOAD_REG_IMMEDIATE(r10, MSR_KERNEL)
rlwimi r10, r9, 0, MSR_EE
#else
- LOAD_MSR_KERNEL(r10, MSR_KERNEL | MSR_EE)
+ LOAD_REG_IMMEDIATE(r10, MSR_KERNEL | MSR_EE)
#endif
#if defined(CONFIG_PPC_8xx) && defined(CONFIG_PERF_EVENTS)
mtspr SPRN_NRI, r0
@@ -187,7 +174,7 @@
#define EXC_XFER_TEMPLATE(hdlr, trap, msr, tfer, ret) \
li r10,trap; \
stw r10,_TRAP(r11); \
- LOAD_MSR_KERNEL(r10, msr); \
+ LOAD_REG_IMMEDIATE(r10, msr); \
bl tfer; \
.long hdlr; \
.long ret
--
2.13.3
^ permalink raw reply related
* [PATCH v3 3/3] powerpc/64: optimise LOAD_REG_IMMEDIATE_SYM()
From: Christophe Leroy @ 2019-08-19 13:58 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, segher
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <be2b971c89b1af30d680cedd14e99a83138ef40a.1566223054.git.christophe.leroy@c-s.fr>
Optimise LOAD_REG_IMMEDIATE_SYM() using a temporary register to
parallelise operations.
It reduces the path from 5 to 3 instructions.
Suggested-by: Segher Boessenkool <segher@kernel.crashing.org>
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
v3: new
---
arch/powerpc/include/asm/ppc_asm.h | 12 ++++++------
arch/powerpc/kernel/exceptions-64e.S | 22 +++++++++++++---------
arch/powerpc/kernel/head_64.S | 2 +-
3 files changed, 20 insertions(+), 16 deletions(-)
diff --git a/arch/powerpc/include/asm/ppc_asm.h b/arch/powerpc/include/asm/ppc_asm.h
index aa8717c1571a..9d55bff9a73c 100644
--- a/arch/powerpc/include/asm/ppc_asm.h
+++ b/arch/powerpc/include/asm/ppc_asm.h
@@ -347,12 +347,12 @@ GLUE(.,name):
#define LOAD_REG_IMMEDIATE(reg, expr) __LOAD_REG_IMMEDIATE reg, expr
-#define LOAD_REG_IMMEDIATE_SYM(reg,expr) \
- lis reg,(expr)@highest; \
- ori reg,reg,(expr)@higher; \
- rldicr reg,reg,32,31; \
- oris reg,reg,(expr)@__AS_ATHIGH; \
- ori reg,reg,(expr)@l;
+#define LOAD_REG_IMMEDIATE_SYM(reg, tmp, expr) \
+ lis reg, (expr)@highest; \
+ lis tmp, (expr)@__AS_ATHIGH; \
+ ori reg, reg, (expr)@higher; \
+ ori tmp, reg, (expr)@l; \
+ rldimi reg, tmp, 32, 0
#define LOAD_REG_ADDR(reg,name) \
ld reg,name@got(r2)
diff --git a/arch/powerpc/kernel/exceptions-64e.S b/arch/powerpc/kernel/exceptions-64e.S
index 898aae6da167..829950b96d29 100644
--- a/arch/powerpc/kernel/exceptions-64e.S
+++ b/arch/powerpc/kernel/exceptions-64e.S
@@ -750,12 +750,14 @@ END_FTR_SECTION_IFSET(CPU_FTR_ALTIVEC)
ld r15,PACATOC(r13)
ld r14,interrupt_base_book3e@got(r15)
ld r15,__end_interrupts@got(r15)
-#else
- LOAD_REG_IMMEDIATE_SYM(r14,interrupt_base_book3e)
- LOAD_REG_IMMEDIATE_SYM(r15,__end_interrupts)
-#endif
cmpld cr0,r10,r14
cmpld cr1,r10,r15
+#else
+ LOAD_REG_IMMEDIATE_SYM(r14, r15, interrupt_base_book3e)
+ cmpld cr0, r10, r14
+ LOAD_REG_IMMEDIATE_SYM(r14, r15, __end_interrupts)
+ cmpld cr1, r10, r14
+#endif
blt+ cr0,1f
bge+ cr1,1f
@@ -820,12 +822,14 @@ kernel_dbg_exc:
ld r15,PACATOC(r13)
ld r14,interrupt_base_book3e@got(r15)
ld r15,__end_interrupts@got(r15)
-#else
- LOAD_REG_IMMEDIATE_SYM(r14,interrupt_base_book3e)
- LOAD_REG_IMMEDIATE_SYM(r15,__end_interrupts)
-#endif
cmpld cr0,r10,r14
cmpld cr1,r10,r15
+#else
+ LOAD_REG_IMMEDIATE_SYM(r14, r15, interrupt_base_book3e)
+ cmpld cr0, r10, r14
+ LOAD_REG_IMMEDIATE_SYM(r14, r15,__end_interrupts)
+ cmpld cr1, r10, r14
+#endif
blt+ cr0,1f
bge+ cr1,1f
@@ -1449,7 +1453,7 @@ a2_tlbinit_code_start:
a2_tlbinit_after_linear_map:
/* Now we branch the new virtual address mapped by this entry */
- LOAD_REG_IMMEDIATE_SYM(r3,1f)
+ LOAD_REG_IMMEDIATE_SYM(r3, r5, 1f)
mtctr r3
bctr
diff --git a/arch/powerpc/kernel/head_64.S b/arch/powerpc/kernel/head_64.S
index 1fd44761e997..0f2d61af47cc 100644
--- a/arch/powerpc/kernel/head_64.S
+++ b/arch/powerpc/kernel/head_64.S
@@ -635,7 +635,7 @@ __after_prom_start:
sub r5,r5,r11
#else
/* just copy interrupts */
- LOAD_REG_IMMEDIATE_SYM(r5, FIXED_SYMBOL_ABS_ADDR(__end_interrupts))
+ LOAD_REG_IMMEDIATE_SYM(r5, r11, FIXED_SYMBOL_ABS_ADDR(__end_interrupts))
#endif
b 5f
3:
--
2.13.3
^ permalink raw reply related
* Re: [PATCH v10 1/7] powerpc/mce: Schedule work from irq_work
From: Nicholas Piggin @ 2019-08-19 14:08 UTC (permalink / raw)
To: Linux Kernel, linuxppc-dev, Santosh Sivaraj
Cc: Aneesh Kumar K.V, Mahesh Salgaonkar, stable, Chandan Rajendra,
Mahesh Salgaonkar, Reza Arbab
In-Reply-To: <20190815003941.18655-2-santosh@fossix.org>
Santosh Sivaraj's on August 15, 2019 10:39 am:
> schedule_work() cannot be called from MCE exception context as MCE can
> interrupt even in interrupt disabled context.
The powernv code doesn't do this in general, rather defers kernel
MCEs. My patch series converts the pseries machine check exception
code over to the same.
However there remain special cases where that's not true for
powernv, e.g., the machine check stack overflow or unrecoverable
MCE paths try to force it through so something gets printed. We
probably shouldn't even try to do memory failure in these cases.
Still, shouldn't hurt to make this change and fixes the existing
"different" pseries code.
Thanks,
Nick
> fixes: 733e4a4c ("powerpc/mce: hookup memory_failure for UE errors")
> Suggested-by: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
> Signed-off-by: Santosh Sivaraj <santosh@fossix.org>
> Reviewed-by: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
> Acked-by: Balbir Singh <bsingharora@gmail.com>
> Cc: stable@vger.kernel.org # v4.15+
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
^ permalink raw reply
* Re: [PATCH 3/3] powerpc: use __builtin_trap() in BUG/WARN macros.
From: Christophe Leroy @ 2019-08-19 14:08 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: linuxppc-dev, Paul Mackerras, linux-kernel
In-Reply-To: <20190819132313.GH31406@gate.crashing.org>
Le 19/08/2019 à 15:23, Segher Boessenkool a écrit :
> On Mon, Aug 19, 2019 at 01:06:31PM +0000, Christophe Leroy wrote:
>> Note that we keep using an assembly text using "twi 31, 0, 0" for
>> inconditional traps because GCC drops all code after
>> __builtin_trap() when the condition is always true at build time.
>
> As I said, it can also do this for conditional traps, if it can prove
> the condition is always true.
But we have another branch for 'always true' and 'always false' using
__builtin_constant_p(), which don't use __builtin_trap(). Is there
anything wrong with that ?:
#define BUG_ON(x) do { \
if (__builtin_constant_p(x)) { \
if (x) \
BUG(); \
} else { \
if (x) \
__builtin_trap(); \
BUG_ENTRY("", 0); \
} \
} while (0)
#define WARN_ON(x) ({ \
int __ret_warn_on = !!(x); \
if (__builtin_constant_p(__ret_warn_on)) { \
if (__ret_warn_on) \
__WARN_TAINT(TAINT_WARN); \
} else { \
if (__ret_warn_on) \
__builtin_trap(); \
BUG_ENTRY("", BUGFLAG_WARNING | BUGFLAG_TAINT(TAINT_WARN)); \
} \
unlikely(__ret_warn_on); \
})
>
> Can you put the bug table asm *before* the __builtin_trap maybe? That
> should make it all work fine... If you somehow can tell what machine
> instruction is that trap, anyway.
And how can I tell that ?
When I put it *after*, it always points to the trap instruction. When I
put it *before* it usually points on the first instruction used to
prepare the registers for the trap condition.
Christophe
^ permalink raw reply
* Re: [PATCH v3 1/3] powerpc: rewrite LOAD_REG_IMMEDIATE() as an intelligent macro
From: Segher Boessenkool @ 2019-08-19 14:16 UTC (permalink / raw)
To: Christophe Leroy; +Cc: linuxppc-dev, Paul Mackerras, linux-kernel
In-Reply-To: <be2b971c89b1af30d680cedd14e99a83138ef40a.1566223054.git.christophe.leroy@c-s.fr>
Hi Christophe,
On Mon, Aug 19, 2019 at 01:58:10PM +0000, Christophe Leroy wrote:
> +.macro __LOAD_REG_IMMEDIATE r, x
> + .if (\x) >= 0x80000000 || (\x) < -0x80000000
> + __LOAD_REG_IMMEDIATE_32 \r, (\x) >> 32
> + sldi \r, \r, 32
> + .if (\x) & 0xffff0000 != 0
> + oris \r, \r, (\x)@__AS_ATHIGH
> + .endif
> + .if (\x) & 0xffff != 0
> + oris \r, \r, (\x)@l
> + .endif
> + .else
> + __LOAD_REG_IMMEDIATE_32 \r, \x
> + .endif
> +.endm
How did you test this? That last "oris" should be "ori"?
Rest looks good :-)
Segher
^ permalink raw reply
* Re: [PATCH v10 2/7] powerpc/mce: Fix MCE handling for huge pages
From: Nicholas Piggin @ 2019-08-19 14:22 UTC (permalink / raw)
To: Linux Kernel, linuxppc-dev, Santosh Sivaraj
Cc: Aneesh Kumar K.V, Mahesh Salgaonkar, stable, Chandan Rajendra,
Mahesh Salgaonkar, Reza Arbab
In-Reply-To: <20190815003941.18655-3-santosh@fossix.org>
Santosh Sivaraj's on August 15, 2019 10:39 am:
> From: Balbir Singh <bsingharora@gmail.com>
>
> The current code would fail on huge pages addresses, since the shift would
> be incorrect. Use the correct page shift value returned by
> __find_linux_pte() to get the correct physical address. The code is more
> generic and can handle both regular and compound pages.
>
> Fixes: ba41e1e1ccb9 ("powerpc/mce: Hookup derror (load/store) UE errors")
> Signed-off-by: Balbir Singh <bsingharora@gmail.com>
> [arbab@linux.ibm.com: Fixup pseries_do_memory_failure()]
> Signed-off-by: Reza Arbab <arbab@linux.ibm.com>
> Co-developed-by: Santosh Sivaraj <santosh@fossix.org>
> Signed-off-by: Santosh Sivaraj <santosh@fossix.org>
> Tested-by: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
> Cc: stable@vger.kernel.org # v4.15+
> ---
> arch/powerpc/include/asm/mce.h | 2 +-
> arch/powerpc/kernel/mce_power.c | 55 ++++++++++++++--------------
> arch/powerpc/platforms/pseries/ras.c | 9 ++---
> 3 files changed, 32 insertions(+), 34 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/mce.h b/arch/powerpc/include/asm/mce.h
> index a4c6a74ad2fb..f3a6036b6bc0 100644
> --- a/arch/powerpc/include/asm/mce.h
> +++ b/arch/powerpc/include/asm/mce.h
> @@ -209,7 +209,7 @@ extern void release_mce_event(void);
> extern void machine_check_queue_event(void);
> extern void machine_check_print_event_info(struct machine_check_event *evt,
> bool user_mode, bool in_guest);
> -unsigned long addr_to_pfn(struct pt_regs *regs, unsigned long addr);
> +unsigned long addr_to_phys(struct pt_regs *regs, unsigned long addr);
> #ifdef CONFIG_PPC_BOOK3S_64
> void flush_and_reload_slb(void);
> #endif /* CONFIG_PPC_BOOK3S_64 */
> diff --git a/arch/powerpc/kernel/mce_power.c b/arch/powerpc/kernel/mce_power.c
> index a814d2dfb5b0..e74816f045f8 100644
> --- a/arch/powerpc/kernel/mce_power.c
> +++ b/arch/powerpc/kernel/mce_power.c
> @@ -20,13 +20,14 @@
> #include <asm/exception-64s.h>
>
> /*
> - * Convert an address related to an mm to a PFN. NOTE: we are in real
> - * mode, we could potentially race with page table updates.
> + * Convert an address related to an mm to a physical address.
> + * NOTE: we are in real mode, we could potentially race with page table updates.
> */
> -unsigned long addr_to_pfn(struct pt_regs *regs, unsigned long addr)
> +unsigned long addr_to_phys(struct pt_regs *regs, unsigned long addr)
> {
> - pte_t *ptep;
> - unsigned long flags;
> + pte_t *ptep, pte;
> + unsigned int shift;
> + unsigned long flags, phys_addr;
> struct mm_struct *mm;
>
> if (user_mode(regs))
> @@ -35,14 +36,21 @@ unsigned long addr_to_pfn(struct pt_regs *regs, unsigned long addr)
> mm = &init_mm;
>
> local_irq_save(flags);
> - if (mm == current->mm)
> - ptep = find_current_mm_pte(mm->pgd, addr, NULL, NULL);
> - else
> - ptep = find_init_mm_pte(addr, NULL);
> + ptep = __find_linux_pte(mm->pgd, addr, NULL, &shift);
> local_irq_restore(flags);
> +
> if (!ptep || pte_special(*ptep))
> return ULONG_MAX;
> - return pte_pfn(*ptep);
> +
> + pte = *ptep;
> + if (shift > PAGE_SHIFT) {
> + unsigned long rpnmask = (1ul << shift) - PAGE_SIZE;
> +
> + pte = __pte(pte_val(pte) | (addr & rpnmask));
> + }
> + phys_addr = pte_pfn(pte) << PAGE_SHIFT;
> +
> + return phys_addr;
> }
This should remain addr_to_pfn I think. None of the callers care what
size page the EA was mapped with. 'pfn' is referring to the Linux pfn,
which is the small page number.
if (shift > PAGE_SHIFT)
return (pte_pfn(*ptep) | ((addr & ((1UL << shift) - 1)) >> PAGE_SHIFT);
else
return pte_pfn(*ptep);
Something roughly like that, then you don't have to change any callers
or am I missing something?
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH v3 3/3] powerpc/64: optimise LOAD_REG_IMMEDIATE_SYM()
From: Segher Boessenkool @ 2019-08-19 14:24 UTC (permalink / raw)
To: Christophe Leroy; +Cc: linuxppc-dev, Paul Mackerras, linux-kernel
In-Reply-To: <92bf50b31f5f78cc76ed055b11a492e8e9e2c731.1566223054.git.christophe.leroy@c-s.fr>
On Mon, Aug 19, 2019 at 01:58:12PM +0000, Christophe Leroy wrote:
> -#define LOAD_REG_IMMEDIATE_SYM(reg,expr) \
> - lis reg,(expr)@highest; \
> - ori reg,reg,(expr)@higher; \
> - rldicr reg,reg,32,31; \
> - oris reg,reg,(expr)@__AS_ATHIGH; \
> - ori reg,reg,(expr)@l;
> +#define LOAD_REG_IMMEDIATE_SYM(reg, tmp, expr) \
> + lis reg, (expr)@highest; \
> + lis tmp, (expr)@__AS_ATHIGH; \
> + ori reg, reg, (expr)@higher; \
> + ori tmp, reg, (expr)@l; \
> + rldimi reg, tmp, 32, 0
That should be
#define LOAD_REG_IMMEDIATE_SYM(reg, tmp, expr) \
lis tmp, (expr)@highest; \
ori tmp, tmp, (expr)@higher; \
lis reg, (expr)@__AS_ATHIGH; \
ori reg, reg, (expr)@l; \
rldimi reg, tmp, 32, 0
(tmp is the high half, reg is the low half, as inputs to that rldimi).
Segher
^ permalink raw reply
* Re: [PATCH 7/8] parisc: don't set ARCH_NO_COHERENT_DMA_MMAP
From: Christoph Hellwig @ 2019-08-19 14:25 UTC (permalink / raw)
To: James Bottomley
Cc: linux-xtensa, Michal Simek, Vladimir Murzin, linux-parisc,
linux-sh, Takashi Iwai, linuxppc-dev, Helge Deller, x86,
linux-m68k, linux-kernel, iommu, Robin Murphy, Christoph Hellwig,
linux-arm-kernel, Marek Szyprowski
In-Reply-To: <20190815105002.GA30805@lst.de>
Does my explanation from Thursday make sense or is it completely
off? Does the patch description need some update to be less
confusing to those used to different terminology?
On Thu, Aug 15, 2019 at 12:50:02PM +0200, Christoph Hellwig wrote:
> Except for the different naming scheme vs the code this matches my
> assumptions.
>
> In the code we have three cases (and a fourth EISA case mention in
> comments, but not actually implemented as far as I can tell):
>
> arch/parisc/kernel/pci-dma.c says in the top of file comments:
>
> ** AFAIK, all PA7100LC and PA7300LC platforms can use this code.
>
> and the handles two different case. for cpu_type == pcxl or pcxl2
> it maps the memory as uncached for dma_alloc_coherent, and for all
> other cpu types it fails the coherent allocations.
>
> In addition to that there are the ccio and sba iommu drivers, of which
> according to your above comment one is always present for pa8xxx.
>
> Which brings us back to this patch, which ensures that no cacheable
> memory is exported to userspace by removing ->mmap from ccio and sba.
> It then enabled dma_mmap_coherent for the pcxl or pcxl2 case that
> allocates uncached memory, which dma_mmap_coherent does not work
> because dma_alloc_coherent already failed for the !pcxl && !pcxl2
> and thus there is no memory to mmap.
>
> So if the description is too confusing please suggest a better
> one, I'm a little lost between all these code names and product
> names (arch/parisc/include/asm/dma-mapping.h uses yet another set).
---end quoted text---
^ permalink raw reply
* Re: [PATCH v10 6/7] powerpc/mce: Handle UE event for memcpy_mcsafe
From: Nicholas Piggin @ 2019-08-19 14:28 UTC (permalink / raw)
To: Linux Kernel, linuxppc-dev, Santosh Sivaraj
Cc: Aneesh Kumar K.V, Mahesh Salgaonkar, Chandan Rajendra,
Mahesh Salgaonkar, Reza Arbab
In-Reply-To: <20190815003941.18655-7-santosh@fossix.org>
Santosh Sivaraj's on August 15, 2019 10:39 am:
> From: Balbir Singh <bsingharora@gmail.com>
>
> If we take a UE on one of the instructions with a fixup entry, set nip
> to continue execution at the fixup entry. Stop processing the event
> further or print it.
The previous patch added these fixup entries and now you handle them
here. Which in theory seems to break bisecting. The patches should
either be merged, or this one moved ahead in the series.
I'm still not entirely happy with the ignore_event thing, but that's
probably more a symptom of the convoluted way machine check handling
and reporting is structured. For now it's probably fine.
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
>
> Co-developed-by: Reza Arbab <arbab@linux.ibm.com>
> Signed-off-by: Reza Arbab <arbab@linux.ibm.com>
> Signed-off-by: Balbir Singh <bsingharora@gmail.com>
> Signed-off-by: Santosh Sivaraj <santosh@fossix.org>
> Reviewed-by: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
> ---
> arch/powerpc/include/asm/mce.h | 4 +++-
> arch/powerpc/kernel/mce.c | 16 ++++++++++++++++
> arch/powerpc/kernel/mce_power.c | 15 +++++++++++++--
> 3 files changed, 32 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/mce.h b/arch/powerpc/include/asm/mce.h
> index f3a6036b6bc0..e1931c8c2743 100644
> --- a/arch/powerpc/include/asm/mce.h
> +++ b/arch/powerpc/include/asm/mce.h
> @@ -122,7 +122,8 @@ struct machine_check_event {
> enum MCE_UeErrorType ue_error_type:8;
> u8 effective_address_provided;
> u8 physical_address_provided;
> - u8 reserved_1[5];
> + u8 ignore_event;
> + u8 reserved_1[4];
> u64 effective_address;
> u64 physical_address;
> u8 reserved_2[8];
> @@ -193,6 +194,7 @@ struct mce_error_info {
> enum MCE_Initiator initiator:8;
> enum MCE_ErrorClass error_class:8;
> bool sync_error;
> + bool ignore_event;
> };
>
> #define MAX_MC_EVT 100
> diff --git a/arch/powerpc/kernel/mce.c b/arch/powerpc/kernel/mce.c
> index a3b122a685a5..ec4b3e1087be 100644
> --- a/arch/powerpc/kernel/mce.c
> +++ b/arch/powerpc/kernel/mce.c
> @@ -149,6 +149,7 @@ void save_mce_event(struct pt_regs *regs, long handled,
> if (phys_addr != ULONG_MAX) {
> mce->u.ue_error.physical_address_provided = true;
> mce->u.ue_error.physical_address = phys_addr;
> + mce->u.ue_error.ignore_event = mce_err->ignore_event;
> machine_check_ue_event(mce);
> }
> }
> @@ -266,8 +267,17 @@ static void machine_process_ue_event(struct work_struct *work)
> /*
> * This should probably queued elsewhere, but
> * oh! well
> + *
> + * Don't report this machine check because the caller has a
> + * asked us to ignore the event, it has a fixup handler which
> + * will do the appropriate error handling and reporting.
> */
> if (evt->error_type == MCE_ERROR_TYPE_UE) {
> + if (evt->u.ue_error.ignore_event) {
> + __this_cpu_dec(mce_ue_count);
> + continue;
> + }
> +
> if (evt->u.ue_error.physical_address_provided) {
> unsigned long pfn;
>
> @@ -301,6 +311,12 @@ static void machine_check_process_queued_event(struct irq_work *work)
> while (__this_cpu_read(mce_queue_count) > 0) {
> index = __this_cpu_read(mce_queue_count) - 1;
> evt = this_cpu_ptr(&mce_event_queue[index]);
> +
> + if (evt->error_type == MCE_ERROR_TYPE_UE &&
> + evt->u.ue_error.ignore_event) {
> + __this_cpu_dec(mce_queue_count);
> + continue;
> + }
> machine_check_print_event_info(evt, false, false);
> __this_cpu_dec(mce_queue_count);
> }
> diff --git a/arch/powerpc/kernel/mce_power.c b/arch/powerpc/kernel/mce_power.c
> index e74816f045f8..1dd87f6f5186 100644
> --- a/arch/powerpc/kernel/mce_power.c
> +++ b/arch/powerpc/kernel/mce_power.c
> @@ -11,6 +11,7 @@
>
> #include <linux/types.h>
> #include <linux/ptrace.h>
> +#include <linux/extable.h>
> #include <asm/mmu.h>
> #include <asm/mce.h>
> #include <asm/machdep.h>
> @@ -18,6 +19,7 @@
> #include <asm/pte-walk.h>
> #include <asm/sstep.h>
> #include <asm/exception-64s.h>
> +#include <asm/extable.h>
>
> /*
> * Convert an address related to an mm to a physical address.
> @@ -559,9 +561,18 @@ static int mce_handle_derror(struct pt_regs *regs,
> return 0;
> }
>
> -static long mce_handle_ue_error(struct pt_regs *regs)
> +static long mce_handle_ue_error(struct pt_regs *regs,
> + struct mce_error_info *mce_err)
> {
> long handled = 0;
> + const struct exception_table_entry *entry;
> +
> + entry = search_kernel_exception_table(regs->nip);
> + if (entry) {
> + mce_err->ignore_event = true;
> + regs->nip = extable_fixup(entry);
> + return 1;
> + }
>
> /*
> * On specific SCOM read via MMIO we may get a machine check
> @@ -594,7 +605,7 @@ static long mce_handle_error(struct pt_regs *regs,
> &phys_addr);
>
> if (!handled && mce_err.error_type == MCE_ERROR_TYPE_UE)
> - handled = mce_handle_ue_error(regs);
> + handled = mce_handle_ue_error(regs, &mce_err);
>
> save_mce_event(regs, handled, &mce_err, regs->nip, addr, phys_addr);
>
> --
> 2.21.0
>
>
^ permalink raw reply
* Re: [PATCH 3/3] powerpc: use __builtin_trap() in BUG/WARN macros.
From: Segher Boessenkool @ 2019-08-19 14:37 UTC (permalink / raw)
To: Christophe Leroy; +Cc: linuxppc-dev, Paul Mackerras, linux-kernel
In-Reply-To: <dbafc03a-6eda-d9a3-c451-d242f03b01d9@c-s.fr>
On Mon, Aug 19, 2019 at 04:08:43PM +0200, Christophe Leroy wrote:
> Le 19/08/2019 à 15:23, Segher Boessenkool a écrit :
> >On Mon, Aug 19, 2019 at 01:06:31PM +0000, Christophe Leroy wrote:
> >>Note that we keep using an assembly text using "twi 31, 0, 0" for
> >>inconditional traps because GCC drops all code after
> >>__builtin_trap() when the condition is always true at build time.
> >
> >As I said, it can also do this for conditional traps, if it can prove
> >the condition is always true.
>
> But we have another branch for 'always true' and 'always false' using
> __builtin_constant_p(), which don't use __builtin_trap(). Is there
> anything wrong with that ?:
The compiler might not realise it is constant when it evaluates the
__builtin_constant_p, but only realises it later. As the documentation
for the builtin says:
A return of 0 does not indicate that the
value is _not_ a constant, but merely that GCC cannot prove it is a
constant with the specified value of the '-O' option.
(and there should be many more and more serious warnings here).
> #define BUG_ON(x) do { \
> if (__builtin_constant_p(x)) { \
> if (x) \
> BUG(); \
> } else { \
> if (x) \
> __builtin_trap(); \
> BUG_ENTRY("", 0); \
> } \
> } while (0)
I think it may work if you do
#define BUG_ON(x) do { \
if (__builtin_constant_p(x)) { \
if (x) \
BUG(); \
} else { \
BUG_ENTRY("", 0); \
if (x) \
__builtin_trap(); \
} \
} while (0)
or even just
#define BUG_ON(x) do { \
BUG_ENTRY("", 0); \
if (x) \
__builtin_trap(); \
} \
} while (0)
if BUG_ENTRY can work for the trap insn *after* it.
> >Can you put the bug table asm *before* the __builtin_trap maybe? That
> >should make it all work fine... If you somehow can tell what machine
> >instruction is that trap, anyway.
>
> And how can I tell that ?
I don't know how BUG_ENTRY works exactly.
Segher
^ permalink raw reply
* Re: [PATCH 0/6] drm+dma: cache support for arm, etc
From: Rob Clark @ 2019-08-19 14:39 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Kate Stewart, Masayoshi Mizuma, Maciej W. Rozycki, Eric Biggers,
Catalin Marinas, Imre Deak, dri-devel, Chris Wilson,
Masahiro Yamada, Benjamin Gaignard, Mauro Carvalho Chehab,
Will Deacon, Emil Velikov, Deepak Sharma, Paul Burton,
Mike Rapoport, Geert Uytterhoeven,
moderated list:ARM64 PORT (AARCH64 ARCHITECTURE), Daniel Vetter,
open list:MIPS, Linus Walleij, Robin Murphy,
open list:DRM DRIVER FOR MSM ADRENO GPU, Joerg Roedel,
Arnd Bergmann, Anshuman Khandual, Hauke Mehrtens,
Jesper Dangaard Brouer, Wolfram Sang (Renesas),
open list:LINUX FOR POWERPC (32-BIT AND 64-BIT), Alexios Zavras,
Russell King, Doug Anderson, Thomas Gleixner, Sean Paul,
Allison Randal, Enrico Weigelt, Ard Biesheuvel,
Greg Kroah-Hartman, open list, Rob Clark, Souptick Joarder,
Andrew Morton, open list:DRM DRIVER FOR MSM ADRENO GPU,
christian.koenig
In-Reply-To: <20190819052337.GA16622@lst.de>
On Sun, Aug 18, 2019 at 10:23 PM Christoph Hellwig <hch@lst.de> wrote:
>
> On Fri, Aug 16, 2019 at 02:04:35PM -0700, Rob Clark wrote:
> > I don't disagree about needing an API to get uncached memory (or
> > ideally just something outside of the linear map). But I think this
> > is a separate problem.
> >
> > What I was hoping for, for v5.4, is a way to stop abusing dma_map/sync
> > for cache ops to get rid of the hack I had to make for v5.3. And also
> > to fix vgem on non-x86. (Unfortunately changing vgem to used cached
> > mappings breaks x86 CI, but fixes CI on arm/arm64..) We can do that
> > without any changes in allocation. There is still the possibility for
> > problems due to cached alias, but that has been a problem this whole
> > time, it isn't something new.
>
> But that just means we start exposing random low-level APIs that
> people will quickly abuse.. In fact even your simple plan to some
> extent already is an abuse of the intent of these functions, and
> it also requires a lot of knowledge in the driver that in the normal
> cases drivers can't know (e.g. is the device dma coherent or not).
I can agree that most drivers should use the higher level APIs.. but
not that we must prevent *all* drivers from using them. Most of what
DMA API is trying to solve doesn't apply to a driver like drm/msm..
which is how we ended up with hacks to try and misuse the high level
API to accomplish what we need.
Perhaps we can protect the prototypes with #ifdef LOWLEVEL_DMA_API /
#endif type thing to make it more obvious to other drivers that it
probably isn't the API they should use?
BR,
-R
^ permalink raw reply
* Re: [PATCH v3 3/3] powerpc/64: optimise LOAD_REG_IMMEDIATE_SYM()
From: Nicholas Piggin @ 2019-08-19 15:05 UTC (permalink / raw)
To: Christophe Leroy, Segher Boessenkool
Cc: Paul Mackerras, linuxppc-dev, linux-kernel
In-Reply-To: <20190819142459.GJ31406@gate.crashing.org>
Segher Boessenkool's on August 20, 2019 12:24 am:
> On Mon, Aug 19, 2019 at 01:58:12PM +0000, Christophe Leroy wrote:
>> -#define LOAD_REG_IMMEDIATE_SYM(reg,expr) \
>> - lis reg,(expr)@highest; \
>> - ori reg,reg,(expr)@higher; \
>> - rldicr reg,reg,32,31; \
>> - oris reg,reg,(expr)@__AS_ATHIGH; \
>> - ori reg,reg,(expr)@l;
>> +#define LOAD_REG_IMMEDIATE_SYM(reg, tmp, expr) \
>> + lis reg, (expr)@highest; \
>> + lis tmp, (expr)@__AS_ATHIGH; \
>> + ori reg, reg, (expr)@higher; \
>> + ori tmp, reg, (expr)@l; \
>> + rldimi reg, tmp, 32, 0
>
> That should be
>
> #define LOAD_REG_IMMEDIATE_SYM(reg, tmp, expr) \
> lis tmp, (expr)@highest; \
> ori tmp, tmp, (expr)@higher; \
> lis reg, (expr)@__AS_ATHIGH; \
> ori reg, reg, (expr)@l; \
> rldimi reg, tmp, 32, 0
>
> (tmp is the high half, reg is the low half, as inputs to that rldimi).
I guess the intention was also to try to fit the independent ops into
the earliest fetch/issue cycle possible.
#define LOAD_REG_IMMEDIATE_SYM(reg, tmp, expr) \
lis tmp, (expr)@highest; \
lis reg, (expr)@__AS_ATHIGH; \
ori tmp, tmp, (expr)@higher; \
ori reg, reg, (expr)@l; \
rldimi reg, tmp, 32, 0
Very cool series though.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH 3/3] powerpc: use __builtin_trap() in BUG/WARN macros.
From: Christophe Leroy @ 2019-08-19 15:05 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: linuxppc-dev, Paul Mackerras, linux-kernel
In-Reply-To: <20190819143700.GK31406@gate.crashing.org>
Le 19/08/2019 à 16:37, Segher Boessenkool a écrit :
> On Mon, Aug 19, 2019 at 04:08:43PM +0200, Christophe Leroy wrote:
>> Le 19/08/2019 à 15:23, Segher Boessenkool a écrit :
>>> On Mon, Aug 19, 2019 at 01:06:31PM +0000, Christophe Leroy wrote:
>>>> Note that we keep using an assembly text using "twi 31, 0, 0" for
>>>> inconditional traps because GCC drops all code after
>>>> __builtin_trap() when the condition is always true at build time.
>>>
>>> As I said, it can also do this for conditional traps, if it can prove
>>> the condition is always true.
>>
>> But we have another branch for 'always true' and 'always false' using
>> __builtin_constant_p(), which don't use __builtin_trap(). Is there
>> anything wrong with that ?:
>
> The compiler might not realise it is constant when it evaluates the
> __builtin_constant_p, but only realises it later. As the documentation
> for the builtin says:
> A return of 0 does not indicate that the
> value is _not_ a constant, but merely that GCC cannot prove it is a
> constant with the specified value of the '-O' option.
So you mean GCC would not be able to prove that
__builtin_constant_p(cond) is always true but it would be able to prove
that if (cond) is always true ?
And isn't there a away to tell GCC that '__builtin_trap()' is
recoverable in our case ?
>
> (and there should be many more and more serious warnings here).
>
>> #define BUG_ON(x) do { \
>> if (__builtin_constant_p(x)) { \
>> if (x) \
>> BUG(); \
>> } else { \
>> if (x) \
>> __builtin_trap(); \
>> BUG_ENTRY("", 0); \
>> } \
>> } while (0)
>
> I think it may work if you do
>
> #define BUG_ON(x) do { \
> if (__builtin_constant_p(x)) { \
> if (x) \
> BUG(); \
> } else { \
> BUG_ENTRY("", 0); \
> if (x) \
> __builtin_trap(); \
> } \
> } while (0)
It doesn't work:
void test_bug1(unsigned long long a)
{
BUG_ON(a);
}
00000090 <test_bug1>:
90: 7c 63 23 78 or r3,r3,r4
94: 0f 03 00 00 twnei r3,0
98: 4e 80 00 20 blr
RELOCATION RECORDS FOR [__bug_table]:
OFFSET TYPE VALUE
00000084 R_PPC_ADDR32 .text+0x00000090
As you see, the relocation in __bug_table points to the 'or' and not to
the 'twnei'.
>
> or even just
>
> #define BUG_ON(x) do { \
> BUG_ENTRY("", 0); \
> if (x) \
> __builtin_trap(); \
> } \
> } while (0)
>
> if BUG_ENTRY can work for the trap insn *after* it.
>
>>> Can you put the bug table asm *before* the __builtin_trap maybe? That
>>> should make it all work fine... If you somehow can tell what machine
>>> instruction is that trap, anyway.
>>
>> And how can I tell that ?
>
> I don't know how BUG_ENTRY works exactly.
It's basic, maybe too basic: it adds an inline asm with a label, and
adds a .long in the __bug_table section with the address of that label.
When putting it after the __builtin_trap(), I changed it to using the
address before the one of the label which is always the twxx instruction
as far as I can see.
#define BUG_ENTRY(insn, flags, ...) \
__asm__ __volatile__( \
"1: " insn "\n" \
".section __bug_table,\"aw\"\n" \
"2:\t" PPC_LONG "1b, %0\n" \
"\t.short %1, %2\n" \
".org 2b+%3\n" \
".previous\n" \
: : "i" (__FILE__), "i" (__LINE__), \
"i" (flags), \
"i" (sizeof(struct bug_entry)), \
##__VA_ARGS__)
Christophe
^ permalink raw reply
* Re: [PATCH 3/3] powerpc: use __builtin_trap() in BUG/WARN macros.
From: Segher Boessenkool @ 2019-08-19 15:45 UTC (permalink / raw)
To: Christophe Leroy; +Cc: linuxppc-dev, Paul Mackerras, linux-kernel
In-Reply-To: <44a19633-f2a9-79f9-da7c-16ba64a66600@c-s.fr>
On Mon, Aug 19, 2019 at 05:05:46PM +0200, Christophe Leroy wrote:
> Le 19/08/2019 à 16:37, Segher Boessenkool a écrit :
> >On Mon, Aug 19, 2019 at 04:08:43PM +0200, Christophe Leroy wrote:
> >>Le 19/08/2019 à 15:23, Segher Boessenkool a écrit :
> >>>On Mon, Aug 19, 2019 at 01:06:31PM +0000, Christophe Leroy wrote:
> >>>>Note that we keep using an assembly text using "twi 31, 0, 0" for
> >>>>inconditional traps because GCC drops all code after
> >>>>__builtin_trap() when the condition is always true at build time.
> >>>
> >>>As I said, it can also do this for conditional traps, if it can prove
> >>>the condition is always true.
> >>
> >>But we have another branch for 'always true' and 'always false' using
> >>__builtin_constant_p(), which don't use __builtin_trap(). Is there
> >>anything wrong with that ?:
> >
> >The compiler might not realise it is constant when it evaluates the
> >__builtin_constant_p, but only realises it later. As the documentation
> >for the builtin says:
> > A return of 0 does not indicate that the
> > value is _not_ a constant, but merely that GCC cannot prove it is a
> > constant with the specified value of the '-O' option.
>
> So you mean GCC would not be able to prove that
> __builtin_constant_p(cond) is always true but it would be able to prove
> that if (cond) is always true ?
Not sure what you mean, sorry.
> And isn't there a away to tell GCC that '__builtin_trap()' is
> recoverable in our case ?
No, GCC knows that a trap will never fall through.
> >I think it may work if you do
> >
> >#define BUG_ON(x) do { \
> > if (__builtin_constant_p(x)) { \
> > if (x) \
> > BUG(); \
> > } else { \
> > BUG_ENTRY("", 0); \
> > if (x) \
> > __builtin_trap(); \
> > } \
> >} while (0)
>
> It doesn't work:
You need to make a BUG_ENTRY so that it refers to the *following* trap
instruction, if you go this way.
> >I don't know how BUG_ENTRY works exactly.
>
> It's basic, maybe too basic: it adds an inline asm with a label, and
> adds a .long in the __bug_table section with the address of that label.
>
> When putting it after the __builtin_trap(), I changed it to using the
> address before the one of the label which is always the twxx instruction
> as far as I can see.
>
> #define BUG_ENTRY(insn, flags, ...) \
> __asm__ __volatile__( \
> "1: " insn "\n" \
> ".section __bug_table,\"aw\"\n" \
> "2:\t" PPC_LONG "1b, %0\n" \
> "\t.short %1, %2\n" \
> ".org 2b+%3\n" \
> ".previous\n" \
> : : "i" (__FILE__), "i" (__LINE__), \
> "i" (flags), \
> "i" (sizeof(struct bug_entry)), \
> ##__VA_ARGS__)
#define MY_BUG_ENTRY(lab, flags) \
__asm__ __volatile__( \
".section __bug_table,\"aw\"\n" \
"2:\t" PPC_LONG "%4, %0\n" \
"\t.short %1, %2\n" \
".org 2b+%3\n" \
".previous\n" \
: : "i" (__FILE__), "i" (__LINE__), \
"i" (flags), \
"i" (sizeof(struct bug_entry)), \
"i" (lab))
called as
#define BUG_ON(x) do { \
MY_BUG_ENTRY(&&lab, 0); \
lab: if (x) \
__builtin_trap(); \
} while (0)
not sure how reliable that works -- *if* it works, I just typed that in
without testing or anything -- but hopefully you get the idea.
Segher
^ permalink raw reply
* Re: [PATCH v4 11/25] powernv/fadump: register kernel metadata address with opal
From: Hari Bathini @ 2019-08-19 15:49 UTC (permalink / raw)
To: Mahesh Jagannath Salgaonkar
Cc: Ananth N Mavinakayanahalli, Mahesh J Salgaonkar, Nicholas Piggin,
linuxppc-dev, Oliver, Vasant Hegde, Stewart Smith, Daniel Axtens
In-Reply-To: <8d91f41e-c9c1-2397-1570-62de7a775093@linux.vnet.ibm.com>
On 14/08/19 3:51 PM, Mahesh Jagannath Salgaonkar wrote:
> On 8/14/19 12:36 PM, Hari Bathini wrote:
>>
>>
>> On 13/08/19 4:11 PM, Mahesh J Salgaonkar wrote:
>>> On 2019-07-16 17:03:15 Tue, Hari Bathini wrote:
>>>> OPAL allows registering address with it in the first kernel and
>>>> retrieving it after MPIPL. Setup kernel metadata and register its
>>>> address with OPAL to use it for processing the crash dump.
>>>>
>>>> Signed-off-by: Hari Bathini <hbathini@linux.ibm.com>
>>>> ---
[...]
>>
>>> What if kernel crashes before metadata area is initialized ?
>>
>> registered_regions would be '0'. So, it is treated as fadump is not registered case.
>> Let me
>> initialize metadata explicitly before registering the address with f/w to avoid any assumption...
>
> Do you want to do that before memblock reservation ? Should we move this
> to setup_fadump() ?
Better here as failing early would mean we could fall back to KDump..
^ permalink raw reply
* Re: [PATCH 1/3] powerpc: don't use __WARN() for WARN_ON()
From: Kees Cook @ 2019-08-19 16:28 UTC (permalink / raw)
To: Christophe Leroy
Cc: linux-kernel, Paul Mackerras, Drew Davenport, Andrew Morton,
linuxppc-dev
In-Reply-To: <a6781075192afe0c909ce7d091de7931183a5d93.1566219503.git.christophe.leroy@c-s.fr>
On Mon, Aug 19, 2019 at 01:06:28PM +0000, Christophe Leroy wrote:
> __WARN() used to just call __WARN_TAINT(TAINT_WARN)
>
> But a call to printk() has been added in the commit identified below
> to print a "---- cut here ----" line.
>
> This change only applies to warnings using __WARN(), which means
> WARN_ON() where the condition is constant at compile time.
> For WARN_ON() with a non constant condition, the additional line is
> not printed.
>
> In addition, adding a call to printk() forces GCC to add a stack frame
> and save volatile registers. Powerpc has been using traps to implement
> warnings in order to avoid that.
>
> So, call __WARN_TAINT(TAINT_WARN) directly instead of using __WARN()
> in order to restore the previous behaviour.
>
> If one day powerpc wants the decorative "---- cut here ----" line, it
> has to be done in the trap handler, not in the WARN_ON() macro.
>
> Fixes: 6b15f678fb7d ("include/asm-generic/bug.h: fix "cut here" for WARN_ON for __WARN_TAINT architectures")
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
Ah! Hmpf. Yeah, that wasn't an intended side-effect of this fix.
It seems PPC is not alone in this situation of making this code much
noisier. It looks like there needs to be a way to indicate to the trap
handler that a message was delivered or not. Perhaps we can add another
taint flag?
-kees
> ---
> arch/powerpc/include/asm/bug.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/include/asm/bug.h b/arch/powerpc/include/asm/bug.h
> index fed7e6241349..3928fdaebb71 100644
> --- a/arch/powerpc/include/asm/bug.h
> +++ b/arch/powerpc/include/asm/bug.h
> @@ -99,7 +99,7 @@
> int __ret_warn_on = !!(x); \
> if (__builtin_constant_p(__ret_warn_on)) { \
> if (__ret_warn_on) \
> - __WARN(); \
> + __WARN_TAINT(TAINT_WARN); \
> } else { \
> __asm__ __volatile__( \
> "1: "PPC_TLNEI" %4,0\n" \
> --
> 2.13.3
>
--
Kees Cook
^ permalink raw reply
* Re: [PATCH] powerpc/vdso32: Add support for CLOCK_{REALTIME/MONOTONIC}_COARSE
From: Nathan Lynch @ 2019-08-19 16:37 UTC (permalink / raw)
To: Christophe Leroy, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman
Cc: Naveen N. Rao, linuxppc-dev, Santosh Sivaraj, linux-kernel
In-Reply-To: <1eb059dcb634c48980e5e43f465aabd3d35ba7f7.1565960416.git.christophe.leroy@c-s.fr>
Hi,
Christophe Leroy <christophe.leroy@c-s.fr> writes:
> Benchmark from vdsotest:
I assume you also ran the verification/correctness parts of vdsotest...? :-)
^ permalink raw reply
* Re: [PATCH v5 1/4] nvdimm: Consider probe return -EOPNOTSUPP as success
From: Dan Williams @ 2019-08-19 16:57 UTC (permalink / raw)
To: Aneesh Kumar K.V; +Cc: Linux MM, linuxppc-dev, linux-nvdimm
In-Reply-To: <87y2zp1vph.fsf@linux.ibm.com>
On Mon, Aug 19, 2019 at 12:07 AM Aneesh Kumar K.V
<aneesh.kumar@linux.ibm.com> wrote:
>
> Dan Williams <dan.j.williams@intel.com> writes:
>
> > On Tue, Aug 13, 2019 at 9:22 PM Dan Williams <dan.j.williams@intel.com> wrote:
> >>
> >> Hi Aneesh, logic looks correct but there are some cleanups I'd like to
> >> see and a lead-in patch that I attached.
> >>
> >> I've started prefixing nvdimm patches with:
> >>
> >> libnvdimm/$component:
> >>
> >> ...since this patch mostly impacts the pmem driver lets prefix it
> >> "libnvdimm/pmem: "
> >>
> >> On Fri, Aug 9, 2019 at 12:45 AM Aneesh Kumar K.V
> >> <aneesh.kumar@linux.ibm.com> wrote:
> >> >
> >> > This patch add -EOPNOTSUPP as return from probe callback to
> >>
> >> s/This patch add/Add/
> >>
> >> No need to say "this patch" it's obviously a patch.
> >>
> >> > indicate we were not able to initialize a namespace due to pfn superblock
> >> > feature/version mismatch. We want to consider this a probe success so that
> >> > we can create new namesapce seed and there by avoid marking the failed
> >> > namespace as the seed namespace.
> >>
> >> Please replace usage of "we" with the exact agent involved as which
> >> "we" is being referred to gets confusing for the reader.
> >>
> >> i.e. "indicate that the pmem driver was not..." "The nvdimm core wants
> >> to consider this...".
> >>
> >> >
> >> > Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> >> > ---
> >> > drivers/nvdimm/bus.c | 2 +-
> >> > drivers/nvdimm/pmem.c | 26 ++++++++++++++++++++++----
> >> > 2 files changed, 23 insertions(+), 5 deletions(-)
> >> >
> >> > diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
> >> > index 798c5c4aea9c..16c35e6446a7 100644
> >> > --- a/drivers/nvdimm/bus.c
> >> > +++ b/drivers/nvdimm/bus.c
> >> > @@ -95,7 +95,7 @@ static int nvdimm_bus_probe(struct device *dev)
> >> > rc = nd_drv->probe(dev);
> >> > debug_nvdimm_unlock(dev);
> >> >
> >> > - if (rc == 0)
> >> > + if (rc == 0 || rc == -EOPNOTSUPP)
> >> > nd_region_probe_success(nvdimm_bus, dev);
> >>
> >> This now makes the nd_region_probe_success() helper obviously misnamed
> >> since it now wants to take actions on non-probe success. I attached a
> >> lead-in cleanup that you can pull into your series that renames that
> >> routine to nd_region_advance_seeds().
> >>
> >> When you rebase this needs a comment about why EOPNOTSUPP has special handling.
> >>
> >> > else
> >> > nd_region_disable(nvdimm_bus, dev);
> >> > diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
> >> > index 4c121dd03dd9..3f498881dd28 100644
> >> > --- a/drivers/nvdimm/pmem.c
> >> > +++ b/drivers/nvdimm/pmem.c
> >> > @@ -490,6 +490,7 @@ static int pmem_attach_disk(struct device *dev,
> >> >
> >> > static int nd_pmem_probe(struct device *dev)
> >> > {
> >> > + int ret;
> >> > struct nd_namespace_common *ndns;
> >> >
> >> > ndns = nvdimm_namespace_common_probe(dev);
> >> > @@ -505,12 +506,29 @@ static int nd_pmem_probe(struct device *dev)
> >> > if (is_nd_pfn(dev))
> >> > return pmem_attach_disk(dev, ndns);
> >> >
> >> > - /* if we find a valid info-block we'll come back as that personality */
> >> > - if (nd_btt_probe(dev, ndns) == 0 || nd_pfn_probe(dev, ndns) == 0
> >> > - || nd_dax_probe(dev, ndns) == 0)
> >>
> >> Similar need for an updated comment here to explain the special
> >> translation of error codes.
> >>
> >> > + ret = nd_btt_probe(dev, ndns);
> >> > + if (ret == 0)
> >> > return -ENXIO;
> >> > + else if (ret == -EOPNOTSUPP)
> >>
> >> Are there cases where the btt driver needs to return EOPNOTSUPP? I'd
> >> otherwise like to keep this special casing constrained to the pfn /
> >> dax info block cases.
> >
> > In fact I think EOPNOTSUPP is only something that the device-dax case
> > would be concerned with because that's the only interface that
> > attempts to guarantee a given mapping granularity.
>
> We need to do similar error handling w.r.t fsdax when the pfn superblock
> indicates different PAGE_SIZE and struct page size?
Only in the case where PAGE_SIZE is less than the pfn superblock page
size, the memmap is stored on pmem, and the reservation is too small.
Otherwise the PAGE_SIZE difference does not matter in practice for the
fsdax case... unless I'm overlooking another failure case?
> I don't think btt
> needs to support EOPNOTSUPP. But we can keep it for consistency?
That's not a sufficient argument in my mind. The comment about why
EOPNOTSUPP is treated specially should have a note about the known
usages, and since there is no BTT case for it lets leave it out.
^ permalink raw reply
* Clean up cut-here even harder (was Re: [PATCH 1/3] powerpc: don't use __WARN() for WARN_ON())
From: Kees Cook @ 2019-08-19 17:29 UTC (permalink / raw)
To: Christophe Leroy
Cc: Peter Zijlstra, linux-kernel, Paul Mackerras, Drew Davenport,
Andrew Morton, linuxppc-dev
In-Reply-To: <201908190917.9C65E23D6A@keescook>
On Mon, Aug 19, 2019 at 09:28:03AM -0700, Kees Cook wrote:
> On Mon, Aug 19, 2019 at 01:06:28PM +0000, Christophe Leroy wrote:
> > __WARN() used to just call __WARN_TAINT(TAINT_WARN)
> >
> > But a call to printk() has been added in the commit identified below
> > to print a "---- cut here ----" line.
> >
> > This change only applies to warnings using __WARN(), which means
> > WARN_ON() where the condition is constant at compile time.
> > For WARN_ON() with a non constant condition, the additional line is
> > not printed.
> >
> > In addition, adding a call to printk() forces GCC to add a stack frame
> > and save volatile registers. Powerpc has been using traps to implement
> > warnings in order to avoid that.
> >
> > So, call __WARN_TAINT(TAINT_WARN) directly instead of using __WARN()
> > in order to restore the previous behaviour.
> >
> > If one day powerpc wants the decorative "---- cut here ----" line, it
> > has to be done in the trap handler, not in the WARN_ON() macro.
> >
> > Fixes: 6b15f678fb7d ("include/asm-generic/bug.h: fix "cut here" for WARN_ON for __WARN_TAINT architectures")
> > Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>
> Ah! Hmpf. Yeah, that wasn't an intended side-effect of this fix.
>
> It seems PPC is not alone in this situation of making this code much
> noisier. It looks like there needs to be a way to indicate to the trap
> handler that a message was delivered or not. Perhaps we can add another
> taint flag?
I meant "bug flag" here, not taint. Here's a stab at it. This tries to
remove redundant defines, and moves the "cut here" up into the slow path
explicitly (out of _warn()) and creates a flag so the trap handler can
actually detect if things were already reported...
Thoughts?
diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index aa6c093d9ce9..c2b79878f24c 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -10,6 +10,7 @@
#define BUGFLAG_WARNING (1 << 0)
#define BUGFLAG_ONCE (1 << 1)
#define BUGFLAG_DONE (1 << 2)
+#define BUGFLAG_PRINTK (1 << 3)
#define BUGFLAG_TAINT(taint) ((taint) << 8)
#define BUG_GET_TAINT(bug) ((bug)->flags >> 8)
#endif
@@ -62,13 +63,11 @@ struct bug_entry {
#endif
#ifdef __WARN_FLAGS
-#define __WARN_TAINT(taint) __WARN_FLAGS(BUGFLAG_TAINT(taint))
-#define __WARN_ONCE_TAINT(taint) __WARN_FLAGS(BUGFLAG_ONCE|BUGFLAG_TAINT(taint))
-
#define WARN_ON_ONCE(condition) ({ \
int __ret_warn_on = !!(condition); \
if (unlikely(__ret_warn_on)) \
- __WARN_ONCE_TAINT(TAINT_WARN); \
+ __WARN_FLAGS(BUGFLAG_ONCE | \
+ BUGFLAG_TAINT(TAINT_WARN)); \
unlikely(__ret_warn_on); \
})
#endif
@@ -89,7 +88,7 @@ struct bug_entry {
*
* Use the versions with printk format strings to provide better diagnostics.
*/
-#ifndef __WARN_TAINT
+#ifndef __WARN_FLAGS
extern __printf(3, 4)
void warn_slowpath_fmt(const char *file, const int line,
const char *fmt, ...);
@@ -104,12 +103,12 @@ extern void warn_slowpath_null(const char *file, const int line);
warn_slowpath_fmt_taint(__FILE__, __LINE__, taint, arg)
#else
extern __printf(1, 2) void __warn_printk(const char *fmt, ...);
-#define __WARN() do { \
- printk(KERN_WARNING CUT_HERE); __WARN_TAINT(TAINT_WARN); \
-} while (0)
+#define __WARN() __WARN_FLAGS(BUGFLAG_TAINT(TAINT_WARN))
#define __WARN_printf(arg...) __WARN_printf_taint(TAINT_WARN, arg)
-#define __WARN_printf_taint(taint, arg...) \
- do { __warn_printk(arg); __WARN_TAINT(taint); } while (0)
+#define __WARN_printf_taint(taint, arg...) do { \
+ __warn_printk(arg); __WARN_FLAGS(BUGFLAG_PRINTK | \
+ BUGFLAG_TAINT(taint)); \
+ } while (0)
#endif
/* used internally by panic.c */
diff --git a/kernel/panic.c b/kernel/panic.c
index 057540b6eee9..03c98da6e3f7 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -551,9 +551,6 @@ void __warn(const char *file, int line, void *caller, unsigned taint,
{
disable_trace_on_warning();
- if (args)
- pr_warn(CUT_HERE);
-
if (file)
pr_warn("WARNING: CPU: %d PID: %d at %s:%d %pS\n",
raw_smp_processor_id(), current->pid, file, line,
@@ -596,6 +593,7 @@ void warn_slowpath_fmt(const char *file, int line, const char *fmt, ...)
{
struct warn_args args;
+ pr_warn(CUT_HERE);
args.fmt = fmt;
va_start(args.args, fmt);
__warn(file, line, __builtin_return_address(0), TAINT_WARN, NULL,
@@ -609,6 +607,7 @@ void warn_slowpath_fmt_taint(const char *file, int line,
{
struct warn_args args;
+ pr_warn(CUT_HERE);
args.fmt = fmt;
va_start(args.args, fmt);
__warn(file, line, __builtin_return_address(0), taint, NULL, &args);
diff --git a/lib/bug.c b/lib/bug.c
index 1077366f496b..73ce8f9d9eff 100644
--- a/lib/bug.c
+++ b/lib/bug.c
@@ -181,6 +181,10 @@ enum bug_trap_type report_bug(unsigned long bugaddr, struct pt_regs *regs)
}
}
+ /* Did this trap already report a printk line with "cut here"? */
+ if ((bug->flags & BUGFLAG_PRINTK) == 0)
+ printk(KERN_DEFAULT CUT_HERE);
+
if (warning) {
/* this is a WARN_ON rather than BUG/BUG_ON */
__warn(file, line, (void *)bugaddr, BUG_GET_TAINT(bug), regs,
@@ -188,8 +192,6 @@ enum bug_trap_type report_bug(unsigned long bugaddr, struct pt_regs *regs)
return BUG_TRAP_TYPE_WARN;
}
- printk(KERN_DEFAULT CUT_HERE);
-
if (file)
pr_crit("kernel BUG at %s:%u!\n", file, line);
else
--
Kees Cook
^ permalink raw reply related
* Re: [PATCH] btrfs: fix allocation of bitmap pages.
From: David Sterba @ 2019-08-19 17:46 UTC (permalink / raw)
To: Christophe Leroy
Cc: erhard_f, linux-mm, Josef Bacik, stable, linux-kernel,
Chris Mason, David Sterba, Andrew Morton, linuxppc-dev,
linux-btrfs
In-Reply-To: <20190817074439.84C6C1056A3@localhost.localdomain>
On Sat, Aug 17, 2019 at 07:44:39AM +0000, Christophe Leroy wrote:
> Various notifications of type "BUG kmalloc-4096 () : Redzone
> overwritten" have been observed recently in various parts of
> the kernel. After some time, it has been made a relation with
> the use of BTRFS filesystem.
>
> [ 22.809700] BUG kmalloc-4096 (Tainted: G W ): Redzone overwritten
> [ 22.809971] -----------------------------------------------------------------------------
>
> [ 22.810286] INFO: 0xbe1a5921-0xfbfc06cd. First byte 0x0 instead of 0xcc
> [ 22.810866] INFO: Allocated in __load_free_space_cache+0x588/0x780 [btrfs] age=22 cpu=0 pid=224
> [ 22.811193] __slab_alloc.constprop.26+0x44/0x70
> [ 22.811345] kmem_cache_alloc_trace+0xf0/0x2ec
> [ 22.811588] __load_free_space_cache+0x588/0x780 [btrfs]
> [ 22.811848] load_free_space_cache+0xf4/0x1b0 [btrfs]
> [ 22.812090] cache_block_group+0x1d0/0x3d0 [btrfs]
> [ 22.812321] find_free_extent+0x680/0x12a4 [btrfs]
> [ 22.812549] btrfs_reserve_extent+0xec/0x220 [btrfs]
> [ 22.812785] btrfs_alloc_tree_block+0x178/0x5f4 [btrfs]
> [ 22.813032] __btrfs_cow_block+0x150/0x5d4 [btrfs]
> [ 22.813262] btrfs_cow_block+0x194/0x298 [btrfs]
> [ 22.813484] commit_cowonly_roots+0x44/0x294 [btrfs]
> [ 22.813718] btrfs_commit_transaction+0x63c/0xc0c [btrfs]
> [ 22.813973] close_ctree+0xf8/0x2a4 [btrfs]
> [ 22.814107] generic_shutdown_super+0x80/0x110
> [ 22.814250] kill_anon_super+0x18/0x30
> [ 22.814437] btrfs_kill_super+0x18/0x90 [btrfs]
> [ 22.814590] INFO: Freed in proc_cgroup_show+0xc0/0x248 age=41 cpu=0 pid=83
> [ 22.814841] proc_cgroup_show+0xc0/0x248
> [ 22.814967] proc_single_show+0x54/0x98
> [ 22.815086] seq_read+0x278/0x45c
> [ 22.815190] __vfs_read+0x28/0x17c
> [ 22.815289] vfs_read+0xa8/0x14c
> [ 22.815381] ksys_read+0x50/0x94
> [ 22.815475] ret_from_syscall+0x0/0x38
>
> Commit 69d2480456d1 ("btrfs: use copy_page for copying pages instead
> of memcpy") changed the way bitmap blocks are copied. But allthough
> bitmaps have the size of a page, they were allocated with kzalloc().
>
> Most of the time, kzalloc() allocates aligned blocks of memory, so
> copy_page() can be used. But when some debug options like SLAB_DEBUG
> are activated, kzalloc() may return unaligned pointer.
>
> On powerpc, memcpy(), copy_page() and other copying functions use
> 'dcbz' instruction which provides an entire zeroed cacheline to avoid
> memory read when the intention is to overwrite a full line. Functions
> like memcpy() are writen to care about partial cachelines at the start
> and end of the destination, but copy_page() assumes it gets pages.
This assumption is not documented nor any pitfalls mentioned in
include/asm-generic/page.h that provides the generic implementation. I
as an API user cannot check each arch implementation for additional
constraints or I would expect that it deals with the boundary cases the
same way as arch-specific memcpy implementations.
Another thing that is lost is the slub debugging support for all
architectures, because get_zeroed_pages lacking the red zones and sanity
checks.
I find working with raw pages in this code a bit inconsistent with the
rest of btrfs code, but that's rather minor compared to the above.
Summing it up, I think that the proper fix should go to copy_page
implementation on architectures that require it or make it clear what
are the copy_page constraints.
^ permalink raw reply
* Re: [PATCH] btrfs: fix allocation of bitmap pages.
From: Christophe Leroy @ 2019-08-19 20:32 UTC (permalink / raw)
To: dsterba, erhard_f, Chris Mason, Josef Bacik, David Sterba,
Andrew Morton, linux-kernel, linuxppc-dev, linux-btrfs, linux-mm,
stable
In-Reply-To: <20190819174600.GN24086@twin.jikos.cz>
Le 19/08/2019 à 19:46, David Sterba a écrit :
> On Sat, Aug 17, 2019 at 07:44:39AM +0000, Christophe Leroy wrote:
>> Various notifications of type "BUG kmalloc-4096 () : Redzone
>> overwritten" have been observed recently in various parts of
>> the kernel. After some time, it has been made a relation with
>> the use of BTRFS filesystem.
>>
>> [ 22.809700] BUG kmalloc-4096 (Tainted: G W ): Redzone overwritten
>> [ 22.809971] -----------------------------------------------------------------------------
>>
>> [ 22.810286] INFO: 0xbe1a5921-0xfbfc06cd. First byte 0x0 instead of 0xcc
>> [ 22.810866] INFO: Allocated in __load_free_space_cache+0x588/0x780 [btrfs] age=22 cpu=0 pid=224
>> [ 22.811193] __slab_alloc.constprop.26+0x44/0x70
>> [ 22.811345] kmem_cache_alloc_trace+0xf0/0x2ec
>> [ 22.811588] __load_free_space_cache+0x588/0x780 [btrfs]
>> [ 22.811848] load_free_space_cache+0xf4/0x1b0 [btrfs]
>> [ 22.812090] cache_block_group+0x1d0/0x3d0 [btrfs]
>> [ 22.812321] find_free_extent+0x680/0x12a4 [btrfs]
>> [ 22.812549] btrfs_reserve_extent+0xec/0x220 [btrfs]
>> [ 22.812785] btrfs_alloc_tree_block+0x178/0x5f4 [btrfs]
>> [ 22.813032] __btrfs_cow_block+0x150/0x5d4 [btrfs]
>> [ 22.813262] btrfs_cow_block+0x194/0x298 [btrfs]
>> [ 22.813484] commit_cowonly_roots+0x44/0x294 [btrfs]
>> [ 22.813718] btrfs_commit_transaction+0x63c/0xc0c [btrfs]
>> [ 22.813973] close_ctree+0xf8/0x2a4 [btrfs]
>> [ 22.814107] generic_shutdown_super+0x80/0x110
>> [ 22.814250] kill_anon_super+0x18/0x30
>> [ 22.814437] btrfs_kill_super+0x18/0x90 [btrfs]
>> [ 22.814590] INFO: Freed in proc_cgroup_show+0xc0/0x248 age=41 cpu=0 pid=83
>> [ 22.814841] proc_cgroup_show+0xc0/0x248
>> [ 22.814967] proc_single_show+0x54/0x98
>> [ 22.815086] seq_read+0x278/0x45c
>> [ 22.815190] __vfs_read+0x28/0x17c
>> [ 22.815289] vfs_read+0xa8/0x14c
>> [ 22.815381] ksys_read+0x50/0x94
>> [ 22.815475] ret_from_syscall+0x0/0x38
>>
>> Commit 69d2480456d1 ("btrfs: use copy_page for copying pages instead
>> of memcpy") changed the way bitmap blocks are copied. But allthough
>> bitmaps have the size of a page, they were allocated with kzalloc().
>>
>> Most of the time, kzalloc() allocates aligned blocks of memory, so
>> copy_page() can be used. But when some debug options like SLAB_DEBUG
>> are activated, kzalloc() may return unaligned pointer.
>>
>> On powerpc, memcpy(), copy_page() and other copying functions use
>> 'dcbz' instruction which provides an entire zeroed cacheline to avoid
>> memory read when the intention is to overwrite a full line. Functions
>> like memcpy() are writen to care about partial cachelines at the start
>> and end of the destination, but copy_page() assumes it gets pages.
>
> This assumption is not documented nor any pitfalls mentioned in
> include/asm-generic/page.h that provides the generic implementation. I
> as an API user cannot check each arch implementation for additional
> constraints or I would expect that it deals with the boundary cases the
> same way as arch-specific memcpy implementations.
For me, copy_page() is there to ... copy pages. Not to copy any piece of
RAM having the size of a page.
But it happened to others. See commit
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d72e9a7a93e4f8e9e52491921d99e0c8aa89eb4e
>
> Another thing that is lost is the slub debugging support for all
> architectures, because get_zeroed_pages lacking the red zones and sanity
> checks.
>
> I find working with raw pages in this code a bit inconsistent with the
> rest of btrfs code, but that's rather minor compared to the above.
What about using kmem_cache instead ? I see kmem_cache is already widely
used in BTRFS, so using it also for block of memory of size PAGE_SIZE
should be ok ?
AFAICS, kmem_cache has the red zones and sanity checks.
>
> Summing it up, I think that the proper fix should go to copy_page
> implementation on architectures that require it or make it clear what
> are the copy_page constraints.
>
I guess anybody using copy_page() to copy something else than a page is
on his/her own.
But following that (bad) experience, I propose a patch to at least
detect it early, see https://patchwork.ozlabs.org/patch/1148033/
Christophe
^ 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