* 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 v2 09/44] powerpc/64s/pseries: machine check convert to use common event code
From: Nicholas Piggin @ 2019-08-19 13:09 UTC (permalink / raw)
To: kbuild test robot, Michael Ellerman; +Cc: linuxppc-dev, kbuild-all
In-Reply-To: <875zmw7np7.fsf@concordia.ellerman.id.au>
Michael Ellerman's on August 17, 2019 8:25 am:
> kbuild test robot <lkp@intel.com> writes:
>> Hi Nicholas,
>>
>> I love your patch! Yet something to improve:
>>
>> [auto build test ERROR on linus/master]
>> [cannot apply to v5.3-rc3 next-20190807]
>> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
>>
>> url: https://github.com/0day-ci/linux/commits/Nicholas-Piggin/powerpc-64s-exception-cleanup-and-macrofiy/20190802-222211
>> config: powerpc-defconfig (attached as .config)
>> compiler: powerpc64-linux-gcc (GCC) 7.4.0
>> reproduce:
>> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>> chmod +x ~/bin/make.cross
>> # save the attached .config to linux build tree
>> GCC_VERSION=7.4.0 make.cross ARCH=powerpc
>>
>> If you fix the issue, kindly add following tag
>> Reported-by: kbuild test robot <lkp@intel.com>
>>
>> All errors (new ones prefixed by >>):
>>
>> arch/powerpc/platforms/pseries/ras.c: In function 'mce_handle_error':
>>>> arch/powerpc/platforms/pseries/ras.c:563:28: error: this statement may fall through [-Werror=implicit-fallthrough=]
>> mce_err.u.ue_error_type = MCE_UE_ERROR_IFETCH;
>> ~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~
>> arch/powerpc/platforms/pseries/ras.c:564:3: note: here
>> case MC_ERROR_UE_PAGE_TABLE_WALK_IFETCH:
>> ^~~~
>> arch/powerpc/platforms/pseries/ras.c:565:28: error: this statement may fall through [-Werror=implicit-fallthrough=]
>> mce_err.u.ue_error_type = MCE_UE_ERROR_PAGE_TABLE_WALK_IFETCH;
>> ~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> arch/powerpc/platforms/pseries/ras.c:566:3: note: here
>> case MC_ERROR_UE_LOAD_STORE:
>> ^~~~
>> arch/powerpc/platforms/pseries/ras.c:567:28: error: this statement may fall through [-Werror=implicit-fallthrough=]
>> mce_err.u.ue_error_type = MCE_UE_ERROR_LOAD_STORE;
>> ~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~
>> arch/powerpc/platforms/pseries/ras.c:568:3: note: here
>> case MC_ERROR_UE_PAGE_TABLE_WALK_LOAD_STORE:
>> ^~~~
>> arch/powerpc/platforms/pseries/ras.c:569:28: error: this statement may fall through [-Werror=implicit-fallthrough=]
>> mce_err.u.ue_error_type = MCE_UE_ERROR_PAGE_TABLE_WALK_LOAD_STORE;
>> ~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> arch/powerpc/platforms/pseries/ras.c:570:3: note: here
>> case MC_ERROR_UE_INDETERMINATE:
>> ^~~~
>> cc1: all warnings being treated as errors
>
> I think you meant to break in all these cases?
Yes I did. I might have had a couple of other minor fixes in the
series and have since retested guest mce injection so I'd perhaps
better resend the series.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH 1/2] powerpc/64s: remplement power4_idle code in C
From: Nicholas Piggin @ 2019-08-19 13:07 UTC (permalink / raw)
To: linuxppc-dev, Michael Ellerman
In-Reply-To: <8736hz6sls.fsf@concordia.ellerman.id.au>
Michael Ellerman's on August 18, 2019 1:49 pm:
> Nicholas Piggin <npiggin@gmail.com> writes:
>> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
>> index eee5bef736c8..64d5ffbb07d1 100644
>> --- a/arch/powerpc/kernel/exceptions-64s.S
>> +++ b/arch/powerpc/kernel/exceptions-64s.S
>> @@ -2286,15 +2286,6 @@ USE_FIXED_SECTION(virt_trampolines)
>> __end_interrupts:
>> DEFINE_FIXED_SYMBOL(__end_interrupts)
>>
>> -#ifdef CONFIG_PPC_970_NAP
>> -EXC_COMMON_BEGIN(power4_fixup_nap)
>> - andc r9,r9,r10
>> - std r9,TI_LOCAL_FLAGS(r11)
>> - ld r10,_LINK(r1) /* make idle task do the */
>> - std r10,_NIP(r1) /* equivalent of a blr */
>> - blr
>> -#endif
>
> This breaks ppc64_defconfig build with:
>
> ERROR: start_text address is c000000000008100, should be c000000000008000
>
> Due to:
>
> c000000000008000 <0000001a.long_branch.power4_fixup_nap>:
> c000000000008000: 48 03 5a b4 b c00000000003dab4 <power4_fixup_nap>
>
>
> Moving power4_fixup_nap back into exceptions-64s.S seems to fix it.
Okay that should be fine if you can update it.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH 2/3] powerpc/64s/radix: all CPUs should flush local translation structure before turning MMU on
From: Nicholas Piggin @ 2019-08-19 13:06 UTC (permalink / raw)
To: linuxppc-dev, Michael Ellerman; +Cc: Aneesh Kumar K.V, kvm-ppc
In-Reply-To: <87zhk56hjg.fsf@concordia.ellerman.id.au>
Michael Ellerman's on August 19, 2019 12:00 pm:
> Nicholas Piggin <npiggin@gmail.com> writes:
>> Rather than sprinkle various translation structure invalidations
>> around different places in early boot, have each CPU flush everything
>> from its local translation structures before enabling its MMU.
>>
>> Radix guests can execute tlbie(l), so have them tlbiel_all in the same
>> place as radix host does.
>>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>> arch/powerpc/mm/book3s64/radix_pgtable.c | 11 ++---------
>> 1 file changed, 2 insertions(+), 9 deletions(-)
>>
>> diff --git a/arch/powerpc/mm/book3s64/radix_pgtable.c b/arch/powerpc/mm/book3s64/radix_pgtable.c
>> index d60cfa05447a..839e01795211 100644
>> --- a/arch/powerpc/mm/book3s64/radix_pgtable.c
>> +++ b/arch/powerpc/mm/book3s64/radix_pgtable.c
>> @@ -382,11 +382,6 @@ static void __init radix_init_pgtable(void)
>> */
>> register_process_table(__pa(process_tb), 0, PRTB_SIZE_SHIFT - 12);
>> pr_info("Process table %p and radix root for kernel: %p\n", process_tb, init_mm.pgd);
>> - asm volatile("ptesync" : : : "memory");
>> - asm volatile(PPC_TLBIE_5(%0,%1,2,1,1) : :
>> - "r" (TLBIEL_INVAL_SET_LPID), "r" (0));
>> - asm volatile("eieio; tlbsync; ptesync" : : : "memory");
>> - trace_tlbie(0, 0, TLBIEL_INVAL_SET_LPID, 0, 2, 1, 1);
>>
>> /*
>> * The init_mm context is given the first available (non-zero) PID,
>> @@ -633,8 +628,7 @@ void __init radix__early_init_mmu(void)
>> radix_init_pgtable();
>> /* Switch to the guard PID before turning on MMU */
>> radix__switch_mmu_context(NULL, &init_mm);
>> - if (cpu_has_feature(CPU_FTR_HVMODE))
>> - tlbiel_all();
>> + tlbiel_all();
>> }
>
> This is oopsing for me in a guest on Power9:
>
> [ 0.000000] radix-mmu: Page sizes from device-tree:
> [ 0.000000] radix-mmu: Page size shift = 12 AP=0x0
> [ 0.000000] radix-mmu: Page size shift = 16 AP=0x5
> [ 0.000000] radix-mmu: Page size shift = 21 AP=0x1
> [ 0.000000] radix-mmu: Page size shift = 30 AP=0x2
> [ 0.000000] -> fw_vec5_feature_init()
> [ 0.000000] <- fw_vec5_feature_init()
> [ 0.000000] -> fw_hypertas_feature_init()
> [ 0.000000] <- fw_hypertas_feature_init()
> [ 0.000000] radix-mmu: Activating Kernel Userspace Execution Prevention
> [ 0.000000] radix-mmu: Activating Kernel Userspace Access Prevention
> [ 0.000000] lpar: Using radix MMU under hypervisor
> [ 0.000000] radix-mmu: Mapped 0x0000000000000000-0x0000000040000000 with 1.00 GiB pages (exec)
> [ 0.000000] radix-mmu: Mapped 0x0000000040000000-0x0000000100000000 with 1.00 GiB pages
> [ 0.000000] radix-mmu: Process table (____ptrval____) and radix root for kernel: (____ptrval____)
> [ 0.000000] Oops: Exception in kernel mode, sig: 4 [#1]
> [ 0.000000] LE PAGE_SIZE=64K MMU=Radix MMU=Hash SMP NR_CPUS=2048 NUMA
> [ 0.000000] Modules linked in:
> [ 0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted 5.3.0-rc2-gcc-8.2.0-00063-gef906dcf7b75 #633
> [ 0.000000] NIP: c0000000000838f8 LR: c000000001066864 CTR: c0000000000838c0
> [ 0.000000] REGS: c000000001647c40 TRAP: 0700 Not tainted (5.3.0-rc2-gcc-8.2.0-00063-gef906dcf7b75)
> [ 0.000000] MSR: 8000000000043003 <SF,FP,ME,RI,LE> CR: 48000222 XER: 20040000
> [ 0.000000] CFAR: c0000000000839b4 IRQMASK: 1
> [ 0.000000] GPR00: c000000001066864 c000000001647ed0 c000000001649700 0000000000000000
> [ 0.000000] GPR04: c000000001608830 0000000000000000 0000000000000010 2000000000000000
> [ 0.000000] GPR08: 0000000000000c00 0000000000000000 0000000000000002 726f6620746f6f72
> [ 0.000000] GPR12: c0000000000838c0 c000000001930000 000000000dc5bef0 0000000001309e10
> [ 0.000000] GPR16: 0000000001309c90 fffffffffffffffd 000000000dc5bef0 0000000001339800
> [ 0.000000] GPR20: 0000000000000014 0000000001ac0000 000000000dc5bf38 000000000daf0000
> [ 0.000000] GPR24: 0000000001f4000c c000000000000000 0000000000400000 c000000001802858
> [ 0.000000] GPR28: c007ffffffffffff c000000001803954 c000000001681cb0 c000000001608830
> [ 0.000000] NIP [c0000000000838f8] radix__tlbiel_all+0x48/0x110
> [ 0.000000] LR [c000000001066864] radix__early_init_mmu+0x494/0x4c8
> [ 0.000000] Call Trace:
> [ 0.000000] [c000000001647ed0] [c000000001066820] radix__early_init_mmu+0x450/0x4c8 (unreliable)
> [ 0.000000] [c000000001647f60] [c00000000105c628] early_setup+0x160/0x198
> [ 0.000000] [c000000001647f90] [000000000000b460] 0xb460
> [ 0.000000] Instruction dump:
> [ 0.000000] 2b830001 39000002 409e00e8 3d220003 3929c318 e9290000 e9290010 75290002
> [ 0.000000] 41820088 7c4004ac 39200000 79085564 <7d294224> 3940007f 39201000 38e00000
> [ 0.000000] random: get_random_bytes called from print_oops_end_marker+0x40/0x80 with crng_init=0
> [ 0.000000] ---[ end trace 0000000000000000 ]---
>
>
> So I think we still need a HV check in there somewhere.
Oh it's the partition scope flush by the looks. Simulator doesn't seem
to be trapping that one.
I'll resend, have to fix the kernel invalidation to also do tlbie for
nest.
Thanks,
Nick
^ permalink raw reply
* [PATCH 3/3] powerpc: use __builtin_trap() in BUG/WARN macros.
From: Christophe Leroy @ 2019-08-19 13:06 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, segher
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <a6781075192afe0c909ce7d091de7931183a5d93.1566219503.git.christophe.leroy@c-s.fr>
The below exemples of use of WARN_ON() show that the result
is sub-optimal in regard of the capabilities of powerpc.
void test_warn1(unsigned long long a)
{
WARN_ON(a);
}
void test_warn2(unsigned long a)
{
WARN_ON(a);
}
void test_warn3(unsigned long a, unsigned long b)
{
WARN_ON(a < b);
}
void test_warn4(unsigned long a, unsigned long b)
{
WARN_ON(!a);
}
void test_warn5(unsigned long a, unsigned long b)
{
WARN_ON(!a && b);
}
00000000 <test_warn1>:
0: 7c 64 23 78 or r4,r3,r4
4: 31 24 ff ff addic r9,r4,-1
8: 7c 89 21 10 subfe r4,r9,r4
c: 0f 04 00 00 twnei r4,0
10: 4e 80 00 20 blr
00000014 <test_warn2>:
14: 31 23 ff ff addic r9,r3,-1
18: 7c 69 19 10 subfe r3,r9,r3
1c: 0f 03 00 00 twnei r3,0
20: 4e 80 00 20 blr
00000024 <test_warn3>:
24: 7c 84 18 10 subfc r4,r4,r3
28: 7d 29 49 10 subfe r9,r9,r9
2c: 7d 29 00 d0 neg r9,r9
30: 0f 09 00 00 twnei r9,0
34: 4e 80 00 20 blr
00000038 <test_warn4>:
38: 7c 63 00 34 cntlzw r3,r3
3c: 54 63 d9 7e rlwinm r3,r3,27,5,31
40: 0f 03 00 00 twnei r3,0
44: 4e 80 00 20 blr
00000048 <test_warn5>:
48: 2f 83 00 00 cmpwi cr7,r3,0
4c: 39 20 00 00 li r9,0
50: 41 9e 00 0c beq cr7,5c <test_warn5+0x14>
54: 7c 84 00 34 cntlzw r4,r4
58: 54 89 d9 7e rlwinm r9,r4,27,5,31
5c: 0f 09 00 00 twnei r9,0
60: 4e 80 00 20 blr
RELOCATION RECORDS FOR [__bug_table]:
OFFSET TYPE VALUE
00000000 R_PPC_ADDR32 .text+0x0000000c
0000000c R_PPC_ADDR32 .text+0x0000001c
00000018 R_PPC_ADDR32 .text+0x00000030
00000018 R_PPC_ADDR32 .text+0x00000030
00000024 R_PPC_ADDR32 .text+0x00000040
00000030 R_PPC_ADDR32 .text+0x0000005c
Using __builtin_trap() instead of inline assembly of twnei/tdnei
provides a far better result:
00000000 <test_warn1>:
0: 7c 64 23 78 or r4,r3,r4
4: 0f 04 00 00 twnei r4,0
8: 4e 80 00 20 blr
0000000c <test_warn2>:
c: 0f 03 00 00 twnei r3,0
10: 4e 80 00 20 blr
00000014 <test_warn3>:
14: 7c 43 20 08 twllt r3,r4
18: 4e 80 00 20 blr
0000001c <test_warn4>:
1c: 0c 83 00 00 tweqi r3,0
20: 4e 80 00 20 blr
00000024 <test_warn5>:
24: 2f 83 00 00 cmpwi cr7,r3,0
28: 41 9e 00 08 beq cr7,30 <test_warn5+0xc>
2c: 0c 84 00 00 tweqi r4,0
30: 4e 80 00 20 blr
RELOCATION RECORDS FOR [__bug_table]:
OFFSET TYPE VALUE
00000000 R_PPC_ADDR32 .text+0x00000004
0000000c R_PPC_ADDR32 .text+0x0000000c
00000018 R_PPC_ADDR32 .text+0x00000014
00000024 R_PPC_ADDR32 .text+0x0000001c
00000030 R_PPC_ADDR32 .text+0x0000002c
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.
In addition, this patch also fixes bugs in the BUG_ON(x) macro
which unlike WARN_ON(x) uses (x) directly as the condition by
forcing it to long instead of using !!(x). This leads to
upper part of an unsigned long long being ignored on PPC32 and
may produce bugs on PPC64 if (x) is a smaller type like an int.
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
arch/powerpc/include/asm/bug.h | 17 ++++++++++-------
1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/arch/powerpc/include/asm/bug.h b/arch/powerpc/include/asm/bug.h
index dbf7da90f507..a229130ffcf9 100644
--- a/arch/powerpc/include/asm/bug.h
+++ b/arch/powerpc/include/asm/bug.h
@@ -44,14 +44,14 @@
#ifdef CONFIG_DEBUG_BUGVERBOSE
#define _EMIT_BUG_ENTRY \
".section __bug_table,\"aw\"\n" \
- "2:\t" PPC_LONG "1b, %0\n" \
+ "2:\t" PPC_LONG "1b - 4, %0\n" \
"\t.short %1, %2\n" \
".org 2b+%3\n" \
".previous\n"
#else
#define _EMIT_BUG_ENTRY \
".section __bug_table,\"aw\"\n" \
- "2:\t" PPC_LONG "1b\n" \
+ "2:\t" PPC_LONG "1b - 4\n" \
"\t.short %2\n" \
".org 2b+%3\n" \
".previous\n"
@@ -59,7 +59,8 @@
#define BUG_ENTRY(insn, flags, ...) \
__asm__ __volatile__( \
- "1: " insn "\n" \
+ insn "\n" \
+ "1:\n" \
_EMIT_BUG_ENTRY \
: : "i" (__FILE__), "i" (__LINE__), \
"i" (flags), \
@@ -82,7 +83,9 @@
if (x) \
BUG(); \
} else { \
- BUG_ENTRY(PPC_TLNEI " %4, 0", 0, "r" ((__force long)(x))); \
+ if (x) \
+ __builtin_trap(); \
+ BUG_ENTRY("", 0); \
} \
} while (0)
@@ -94,9 +97,9 @@
if (__ret_warn_on) \
__WARN_TAINT(TAINT_WARN); \
} else { \
- BUG_ENTRY(PPC_TLNEI " %4, 0", \
- BUGFLAG_WARNING | BUGFLAG_TAINT(TAINT_WARN), \
- "r" (__ret_warn_on)); \
+ if (__ret_warn_on) \
+ __builtin_trap(); \
+ BUG_ENTRY("", BUGFLAG_WARNING | BUGFLAG_TAINT(TAINT_WARN)); \
} \
unlikely(__ret_warn_on); \
})
--
2.13.3
^ permalink raw reply related
* [PATCH 1/3] powerpc: don't use __WARN() for WARN_ON()
From: Christophe Leroy @ 2019-08-19 13:06 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, segher,
Drew Davenport, Kees Cook, Andrew Morton
Cc: linuxppc-dev, linux-kernel
__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>
---
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
^ permalink raw reply related
* [PATCH 2/3] powerpc: refactoring BUG/WARN macros
From: Christophe Leroy @ 2019-08-19 13:06 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, segher
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <a6781075192afe0c909ce7d091de7931183a5d93.1566219503.git.christophe.leroy@c-s.fr>
BUG(), WARN() and friends are using a similar inline
assembly to implement various traps with various flags.
Lets refactor via a new BUG_ENTRY() macro.
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
arch/powerpc/include/asm/bug.h | 41 +++++++++++++++--------------------------
1 file changed, 15 insertions(+), 26 deletions(-)
diff --git a/arch/powerpc/include/asm/bug.h b/arch/powerpc/include/asm/bug.h
index 3928fdaebb71..dbf7da90f507 100644
--- a/arch/powerpc/include/asm/bug.h
+++ b/arch/powerpc/include/asm/bug.h
@@ -57,6 +57,15 @@
".previous\n"
#endif
+#define BUG_ENTRY(insn, flags, ...) \
+ __asm__ __volatile__( \
+ "1: " insn "\n" \
+ _EMIT_BUG_ENTRY \
+ : : "i" (__FILE__), "i" (__LINE__), \
+ "i" (flags), \
+ "i" (sizeof(struct bug_entry)), \
+ ##__VA_ARGS__)
+
/*
* BUG_ON() and WARN_ON() do their best to cooperate with compile-time
* optimisations. However depending on the complexity of the condition
@@ -64,11 +73,7 @@
*/
#define BUG() do { \
- __asm__ __volatile__( \
- "1: twi 31,0,0\n" \
- _EMIT_BUG_ENTRY \
- : : "i" (__FILE__), "i" (__LINE__), \
- "i" (0), "i" (sizeof(struct bug_entry))); \
+ BUG_ENTRY("twi 31, 0, 0", 0); \
unreachable(); \
} while (0)
@@ -77,23 +82,11 @@
if (x) \
BUG(); \
} else { \
- __asm__ __volatile__( \
- "1: "PPC_TLNEI" %4,0\n" \
- _EMIT_BUG_ENTRY \
- : : "i" (__FILE__), "i" (__LINE__), "i" (0), \
- "i" (sizeof(struct bug_entry)), \
- "r" ((__force long)(x))); \
+ BUG_ENTRY(PPC_TLNEI " %4, 0", 0, "r" ((__force long)(x))); \
} \
} while (0)
-#define __WARN_FLAGS(flags) do { \
- __asm__ __volatile__( \
- "1: twi 31,0,0\n" \
- _EMIT_BUG_ENTRY \
- : : "i" (__FILE__), "i" (__LINE__), \
- "i" (BUGFLAG_WARNING|(flags)), \
- "i" (sizeof(struct bug_entry))); \
-} while (0)
+#define __WARN_FLAGS(flags) BUG_ENTRY("twi 31, 0, 0", BUGFLAG_WARNING | (flags))
#define WARN_ON(x) ({ \
int __ret_warn_on = !!(x); \
@@ -101,13 +94,9 @@
if (__ret_warn_on) \
__WARN_TAINT(TAINT_WARN); \
} else { \
- __asm__ __volatile__( \
- "1: "PPC_TLNEI" %4,0\n" \
- _EMIT_BUG_ENTRY \
- : : "i" (__FILE__), "i" (__LINE__), \
- "i" (BUGFLAG_WARNING|BUGFLAG_TAINT(TAINT_WARN)),\
- "i" (sizeof(struct bug_entry)), \
- "r" (__ret_warn_on)); \
+ BUG_ENTRY(PPC_TLNEI " %4, 0", \
+ BUGFLAG_WARNING | BUGFLAG_TAINT(TAINT_WARN), \
+ "r" (__ret_warn_on)); \
} \
unlikely(__ret_warn_on); \
})
--
2.13.3
^ permalink raw reply related
* Re: [PATCH v1 07/10] powerpc/mm: move iounmap() into ioremap.c and drop __iounmap()
From: Michael Ellerman @ 2019-08-19 12:55 UTC (permalink / raw)
To: Christophe Leroy, Benjamin Herrenschmidt, Paul Mackerras, npiggin
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <005a2b9321c10e23ca399e6ff4b19960009561f5.1565726867.git.christophe.leroy@c-s.fr>
Christophe Leroy <christophe.leroy@c-s.fr> writes:
> diff --git a/arch/powerpc/mm/ioremap.c b/arch/powerpc/mm/ioremap.c
> index 0c23660522ca..57d742509cec 100644
> --- a/arch/powerpc/mm/ioremap.c
> +++ b/arch/powerpc/mm/ioremap.c
> @@ -72,3 +75,31 @@ void __iomem *ioremap_prot(phys_addr_t addr, unsigned long size, unsigned long f
> return __ioremap_caller(addr, size, pte_pgprot(pte), caller);
> }
> EXPORT_SYMBOL(ioremap_prot);
> +
> +/*
> + * Unmap an IO region and remove it from vmalloc'd list.
> + * Access to IO memory should be serialized by driver.
> + */
> +void iounmap(volatile void __iomem *token)
> +{
> + void *addr;
> +
> + /*
> + * If mapped by BATs then there is nothing to do.
> + */
> + if (v_block_mapped((unsigned long)token))
> + return;
> +
> + if (!slab_is_available())
> + return;
> +
> + addr = (void *)((unsigned long __force)PCI_FIX_ADDR(token) & PAGE_MASK);
> + if (WARN_ON((unsigned long)addr < IOREMAP_BASE))
> + return;
This pops a bunch, as we seem to have various places that want to call
iounmap(NULL) in error paths, much like kfree().
One example:
[ 85.062269] WARNING: CPU: 6 PID: 3643 at arch/powerpc/mm/ioremap.c:97 .iounmap+0x58/0xb0
[ 85.062276] Modules linked in: snd_powermac(+) snd_pcm snd_timer snd soundcore
[ 85.062314] CPU: 6 PID: 3643 Comm: modprobe Tainted: G W 5.3.0-rc2-gcc-8.2.0-00051-ga8e8d67f314c #655
[ 85.062325] NIP: c000000000078e08 LR: c000000000078dd0 CTR: c000000000078db0
[ 85.062335] REGS: c0000000f44f6e40 TRAP: 0700 Tainted: G W (5.3.0-rc2-gcc-8.2.0-00051-ga8e8d67f314c)
[ 85.062342] MSR: 8000000002029032 <SF,VEC,EE,ME,IR,DR,RI> CR: 24228884 XER: 00000000
[ 85.062377] CFAR: c000000000339650 IRQMASK: 0
GPR00: c000000000078dd0 c0000000f44f70d0 c000000001a2ff00 0000000000000000
GPR04: c008000000336518 0000000000000000 c000000001a66b80 0000000000000001
GPR08: c0000000013296c8 c00a000080000000 0000000000000001 c00800000032ba08
GPR12: c000000000078db0 c00000003fff8e80 0000000000000004 c008000000350000
GPR16: c000000002637730 c000000000d69868 0000000000000000 c000000002637740
GPR20: c00000000197ad08 0000000000000000 0000000000000100 0000000000000028
GPR24: c0000000f736eac0 c0000000f44f7370 c0000000f5065000 c008000000336510
GPR28: c0000000026aafc8 ffffffffffffffed 0000000000000000 0000000000000000
[ 85.062554] NIP [c000000000078e08] .iounmap+0x58/0xb0
[ 85.062564] LR [c000000000078dd0] .iounmap+0x20/0xb0
[ 85.062572] Call Trace:
[ 85.062591] [c0000000f44f70d0] [c000000000078dd0] .iounmap+0x20/0xb0 (unreliable)
[ 85.062623] [c0000000f44f7150] [c008000000321e24] .snd_pmac_free+0x164/0x270 [snd_powermac]
[ 85.062709] [c0000000f44f71e0] [c008000000322fa4] .snd_pmac_new+0x884/0xf30 [snd_powermac]
[ 85.062798] [c0000000f44f72f0] [c00800000032015c] .snd_pmac_probe+0x7c/0x450 [snd_powermac]
[ 85.062849] [c0000000f44f73a0] [c0000000007b0628] .platform_drv_probe+0x68/0x100
[ 85.062863] [c0000000f44f7430] [c0000000007aca94] .really_probe+0x144/0x3c0
[ 85.062880] [c0000000f44f74d0] [c0000000007acfe0] .driver_probe_device+0x80/0x170
[ 85.062899] [c0000000f44f7560] [c0000000007a9be4] .bus_for_each_drv+0xb4/0x130
[ 85.062922] [c0000000f44f7610] [c0000000007ac89c] .__device_attach+0x11c/0x1a0
[ 85.062941] [c0000000f44f76c0] [c0000000007ab3d8] .bus_probe_device+0xe8/0x100
[ 85.062961] [c0000000f44f7750] [c0000000007a6164] .device_add+0x504/0x7d0
[ 85.062978] [c0000000f44f7820] [c0000000007b02fc] .platform_device_add+0x14c/0x310
[ 85.062994] [c0000000f44f78c0] [c0000000007b14c0] .platform_device_register_full+0x130/0x210
[ 85.063084] [c0000000f44f7940] [c00800000032b850] .alsa_card_pmac_init+0x80/0xc4 [snd_powermac]
[ 85.063106] [c0000000f44f7a10] [c000000000010e58] .do_one_initcall+0x88/0x448
[ 85.063158] [c0000000f44f7b00] [c0000000002416e4] .do_init_module+0x74/0x2e0
[ 85.063203] [c0000000f44f7ba0] [c000000000243bd4] .load_module+0x20b4/0x26d0
[ 85.063219] [c0000000f44f7cf0] [c000000000244470] .__se_sys_finit_module+0xe0/0x140
[ 85.063237] [c0000000f44f7e20] [c00000000000c46c] system_call+0x5c/0x70
I think we can just do:
void iounmap(volatile void __iomem *token)
{
void *addr;
if (!addr)
return;
...
??
cheers
^ permalink raw reply
* [PATCH v5 0/2] powerpc: Enabling IMA arch specific secure boot policies
From: Nayna Jain @ 2019-08-19 12:35 UTC (permalink / raw)
To: linuxppc-dev, linux-integrity, linux-kernel
Cc: Ard Biesheuvel, Eric Ricther, Nayna Jain, Claudio Carvalho,
Mimi Zohar, Matthew Garret, Paul Mackerras, Jeremy Kerr,
Elaine Palmer, George Wilson
IMA subsystem supports custom, built-in, arch-specific policies to define
the files to be measured and appraised. These policies are honored based
on the priority where arch-specific policies is the highest and custom
is the lowest.
OpenPOWER systems rely on IMA for signature verification of the kernel.
This patchset adds support for powerpc specific arch policies that are
defined based on system's OS secureboot state. The OS secureboot state
of the system is determined via device-tree entry.
Changelog:
v5:
* secureboot state is now read via device tree entry rather than OPAL
secure variables
* ima arch policies are updated to use policy based template for
measurement rules
v4:
* Fixed the build issue as reported by Satheesh Rajendran.
v3:
* OPAL APIs in Patch 1 are updated to provide generic interface based on
key/keylen. This patchset updates kernel OPAL APIs to be compatible with
generic interface.
* Patch 2 is cleaned up to use new OPAL APIs.
* Since OPAL can support different types of backend which can vary in the
variable interpretation, the Patch 2 is updated to add a check for the
backend version
* OPAL API now expects consumer to first check the supported backend version
before calling other secvar OPAL APIs. This check is now added in patch 2.
* IMA policies in Patch 3 is updated to specify appended signature and
per policy template.
* The patches now are free of any EFIisms.
v2:
* Removed Patch 1: powerpc/include: Override unneeded early ioremap
functions
* Updated Subject line and patch description of the Patch 1 of this series
* Removed dependency of OPAL_SECVAR on EFI, CPU_BIG_ENDIAN and UCS2_STRING
* Changed OPAL APIs from static to non-static. Added opal-secvar.h for the
same
* Removed EFI hooks from opal_secvar.c
* Removed opal_secvar_get_next(), opal_secvar_enqueue() and
opal_query_variable_info() function
* get_powerpc_sb_mode() in secboot.c now directly calls OPAL Runtime API
rather than via EFI hooks.
* Fixed log messages in get_powerpc_sb_mode() function.
* Added dependency for PPC_SECURE_BOOT on configs PPC64 and OPAL_SECVAR
* Replaced obj-$(CONFIG_IMA) with obj-$(CONFIG_PPC_SECURE_BOOT) in
arch/powerpc/kernel/Makefile
Nayna Jain (2):
powerpc: detect the secure boot mode of the system
powerpc: Add support to initialize ima policy rules
arch/powerpc/Kconfig | 13 ++++++
arch/powerpc/include/asm/secboot.h | 27 ++++++++++++
arch/powerpc/kernel/Makefile | 2 +
arch/powerpc/kernel/ima_arch.c | 50 +++++++++++++++++++++
arch/powerpc/kernel/secboot.c | 71 ++++++++++++++++++++++++++++++
include/linux/ima.h | 3 +-
6 files changed, 165 insertions(+), 1 deletion(-)
create mode 100644 arch/powerpc/include/asm/secboot.h
create mode 100644 arch/powerpc/kernel/ima_arch.c
create mode 100644 arch/powerpc/kernel/secboot.c
--
2.20.1
^ permalink raw reply
* [PATCH v5 2/2] powerpc: Add support to initialize ima policy rules
From: Nayna Jain @ 2019-08-19 12:35 UTC (permalink / raw)
To: linuxppc-dev, linux-integrity, linux-kernel
Cc: Ard Biesheuvel, Eric Ricther, Nayna Jain, Claudio Carvalho,
Mimi Zohar, Matthew Garret, Paul Mackerras, Jeremy Kerr,
Elaine Palmer, George Wilson
In-Reply-To: <1566218108-12705-1-git-send-email-nayna@linux.ibm.com>
POWER secure boot relies on the kernel IMA security subsystem to
perform the OS kernel image signature verification. Since each secure
boot mode has different IMA policy requirements, dynamic definition of
the policy rules based on the runtime secure boot mode of the system is
required. On systems that support secure boot, but have it disabled,
only measurement policy rules of the kernel image and modules are
defined.
This patch defines the arch-specific implementation to retrieve the
secure boot mode of the system and accordingly configures the IMA policy
rules.
This patch provides arch-specific IMA policies if PPC_SECURE_BOOT
config is enabled.
Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
---
arch/powerpc/Kconfig | 2 ++
arch/powerpc/kernel/Makefile | 2 +-
arch/powerpc/kernel/ima_arch.c | 50 ++++++++++++++++++++++++++++++++++
include/linux/ima.h | 3 +-
4 files changed, 55 insertions(+), 2 deletions(-)
create mode 100644 arch/powerpc/kernel/ima_arch.c
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index c902a39124dc..42109682b727 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -917,6 +917,8 @@ config PPC_SECURE_BOOT
bool
default n
depends on PPC64
+ depends on IMA
+ depends on IMA_ARCH_POLICY
help
Linux on POWER with firmware secure boot enabled needs to define
security policies to extend secure boot to the OS.This config
diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
index d310ebb4e526..520b1c814197 100644
--- a/arch/powerpc/kernel/Makefile
+++ b/arch/powerpc/kernel/Makefile
@@ -157,7 +157,7 @@ endif
obj-$(CONFIG_EPAPR_PARAVIRT) += epapr_paravirt.o epapr_hcalls.o
obj-$(CONFIG_KVM_GUEST) += kvm.o kvm_emul.o
-obj-$(CONFIG_PPC_SECURE_BOOT) += secboot.o
+obj-$(CONFIG_PPC_SECURE_BOOT) += secboot.o ima_arch.o
# Disable GCOV, KCOV & sanitizers in odd or sensitive code
GCOV_PROFILE_prom_init.o := n
diff --git a/arch/powerpc/kernel/ima_arch.c b/arch/powerpc/kernel/ima_arch.c
new file mode 100644
index 000000000000..ac90fac83338
--- /dev/null
+++ b/arch/powerpc/kernel/ima_arch.c
@@ -0,0 +1,50 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 IBM Corporation
+ * Author: Nayna Jain <nayna@linux.ibm.com>
+ *
+ * ima_arch.c
+ * - initialize ima policies for PowerPC Secure Boot
+ */
+
+#include <linux/ima.h>
+#include <asm/secboot.h>
+
+bool arch_ima_get_secureboot(void)
+{
+ return get_powerpc_secureboot();
+}
+
+/*
+ * File signature verification is not needed, include only measurements
+ */
+static const char *const default_arch_rules[] = {
+ "measure func=KEXEC_KERNEL_CHECK",
+ "measure func=MODULE_CHECK",
+ NULL
+};
+
+/* Both file signature verification and measurements are needed */
+static const char *const sb_arch_rules[] = {
+ "measure func=KEXEC_KERNEL_CHECK template=ima-modsig",
+ "appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig",
+#if IS_ENABLED(CONFIG_MODULE_SIG)
+ "measure func=MODULE_CHECK",
+#else
+ "measure func=MODULE_CHECK template=ima-modsig",
+ "appraise func=MODULE_CHECK appraise_type=imasig|modsig",
+#endif
+ NULL
+};
+
+/*
+ * On PowerPC, file measurements are to be added to the IMA measurement list
+ * irrespective of the secure boot state of the system. Signature verification
+ * is conditionally enabled based on the secure boot state.
+ */
+const char *const *arch_get_ima_policy(void)
+{
+ if (IS_ENABLED(CONFIG_IMA_ARCH_POLICY) && arch_ima_get_secureboot())
+ return sb_arch_rules;
+ return default_arch_rules;
+}
diff --git a/include/linux/ima.h b/include/linux/ima.h
index a20ad398d260..10af09b5b478 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -29,7 +29,8 @@ extern void ima_kexec_cmdline(const void *buf, int size);
extern void ima_add_kexec_buffer(struct kimage *image);
#endif
-#if (defined(CONFIG_X86) && defined(CONFIG_EFI)) || defined(CONFIG_S390)
+#if (defined(CONFIG_X86) && defined(CONFIG_EFI)) || defined(CONFIG_S390) \
+ || defined(CONFIG_PPC_SECURE_BOOT)
extern bool arch_ima_get_secureboot(void);
extern const char * const *arch_get_ima_policy(void);
#else
--
2.20.1
^ permalink raw reply related
* [PATCH v5 1/2] powerpc: detect the secure boot mode of the system
From: Nayna Jain @ 2019-08-19 12:35 UTC (permalink / raw)
To: linuxppc-dev, linux-integrity, linux-kernel
Cc: Ard Biesheuvel, Eric Ricther, Nayna Jain, Claudio Carvalho,
Mimi Zohar, Matthew Garret, Paul Mackerras, Jeremy Kerr,
Elaine Palmer, George Wilson
In-Reply-To: <1566218108-12705-1-git-send-email-nayna@linux.ibm.com>
Secure boot on POWER defines different IMA policies based on the secure
boot state of the system.
This patch defines a function to detect the secure boot state of the
system.
The PPC_SECURE_BOOT config represents the base enablement of secureboot
on POWER.
Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
---
arch/powerpc/Kconfig | 11 +++++
arch/powerpc/include/asm/secboot.h | 27 ++++++++++++
arch/powerpc/kernel/Makefile | 2 +
arch/powerpc/kernel/secboot.c | 71 ++++++++++++++++++++++++++++++
4 files changed, 111 insertions(+)
create mode 100644 arch/powerpc/include/asm/secboot.h
create mode 100644 arch/powerpc/kernel/secboot.c
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 77f6ebf97113..c902a39124dc 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -912,6 +912,17 @@ config PPC_MEM_KEYS
If unsure, say y.
+config PPC_SECURE_BOOT
+ prompt "Enable PowerPC Secure Boot"
+ bool
+ default n
+ depends on PPC64
+ help
+ Linux on POWER with firmware secure boot enabled needs to define
+ security policies to extend secure boot to the OS.This config
+ allows user to enable OS Secure Boot on PowerPC systems that
+ have firmware secure boot support.
+
endmenu
config ISA_DMA_API
diff --git a/arch/powerpc/include/asm/secboot.h b/arch/powerpc/include/asm/secboot.h
new file mode 100644
index 000000000000..e726261bb00b
--- /dev/null
+++ b/arch/powerpc/include/asm/secboot.h
@@ -0,0 +1,27 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * PowerPC secure boot definitions
+ *
+ * Copyright (C) 2019 IBM Corporation
+ * Author: Nayna Jain <nayna@linux.ibm.com>
+ *
+ */
+#ifndef POWERPC_SECBOOT_H
+#define POWERPC_SECBOOT_H
+
+#ifdef CONFIG_PPC_SECURE_BOOT
+extern struct device_node *is_powerpc_secvar_supported(void);
+extern bool get_powerpc_secureboot(void);
+#else
+static inline struct device_node *is_powerpc_secvar_supported(void)
+{
+ return NULL;
+}
+
+static inline bool get_powerpc_secureboot(void)
+{
+ return false;
+}
+
+#endif
+#endif
diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
index ea0c69236789..d310ebb4e526 100644
--- a/arch/powerpc/kernel/Makefile
+++ b/arch/powerpc/kernel/Makefile
@@ -157,6 +157,8 @@ endif
obj-$(CONFIG_EPAPR_PARAVIRT) += epapr_paravirt.o epapr_hcalls.o
obj-$(CONFIG_KVM_GUEST) += kvm.o kvm_emul.o
+obj-$(CONFIG_PPC_SECURE_BOOT) += secboot.o
+
# Disable GCOV, KCOV & sanitizers in odd or sensitive code
GCOV_PROFILE_prom_init.o := n
KCOV_INSTRUMENT_prom_init.o := n
diff --git a/arch/powerpc/kernel/secboot.c b/arch/powerpc/kernel/secboot.c
new file mode 100644
index 000000000000..5ea0d52d64ef
--- /dev/null
+++ b/arch/powerpc/kernel/secboot.c
@@ -0,0 +1,71 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 IBM Corporation
+ * Author: Nayna Jain <nayna@linux.ibm.com>
+ *
+ * secboot.c
+ * - util function to get powerpc secboot state
+ */
+#include <linux/types.h>
+#include <linux/of.h>
+#include <asm/secboot.h>
+
+struct device_node *is_powerpc_secvar_supported(void)
+{
+ struct device_node *np;
+ int status;
+
+ np = of_find_node_by_name(NULL, "ibm,secureboot");
+ if (!np) {
+ pr_info("secureboot node is not found\n");
+ return NULL;
+ }
+
+ status = of_device_is_compatible(np, "ibm,secureboot-v3");
+ if (!status) {
+ pr_info("Secure variables are not supported by this firmware\n");
+ return NULL;
+ }
+
+ return np;
+}
+
+bool get_powerpc_secureboot(void)
+{
+ struct device_node *np;
+ struct device_node *secvar_np;
+ const u64 *psecboot;
+ u64 secboot = 0;
+
+ np = is_powerpc_secvar_supported();
+ if (!np)
+ goto disabled;
+
+ /* Fail-safe for any failure related to secvar */
+ secvar_np = of_get_child_by_name(np, "secvar");
+ if (!secvar_np) {
+ pr_err("Expected secure variables support, fail-safe\n");
+ goto enabled;
+ }
+
+ if (!of_device_is_available(secvar_np)) {
+ pr_err("Secure variables support is in error state, fail-safe\n");
+ goto enabled;
+ }
+
+ psecboot = of_get_property(secvar_np, "secure-mode", NULL);
+ if (!psecboot)
+ goto enabled;
+
+ secboot = be64_to_cpup((__be64 *)psecboot);
+ if (!(secboot & (~0x0)))
+ goto disabled;
+
+enabled:
+ pr_info("secureboot mode enabled\n");
+ return true;
+
+disabled:
+ pr_info("secureboot mode disabled\n");
+ return false;
+}
--
2.20.1
^ permalink raw reply related
* Re: [PATCH 2/2] powerpc: support KASAN instrumentation of bitops
From: Christophe Leroy @ 2019-08-19 11:37 UTC (permalink / raw)
To: Daniel Axtens, linux-s390, linux-arch, x86, linuxppc-dev
Cc: Nicholas Piggin, kasan-dev
In-Reply-To: <20190819062814.5315-2-dja@axtens.net>
Le 19/08/2019 à 08:28, Daniel Axtens a écrit :
> In KASAN development I noticed that the powerpc-specific bitops
> were not being picked up by the KASAN test suite.
I'm not sure anybody cares about who noticed the problem. This sentence
could be rephrased as:
The powerpc-specific bitops are not being picked up by the KASAN test suite.
>
> Instrumentation is done via the bitops/instrumented-{atomic,lock}.h
> headers. They require that arch-specific versions of bitop functions
> are renamed to arch_*. Do this renaming.
>
> For clear_bit_unlock_is_negative_byte, the current implementation
> uses the PG_waiters constant. This works because it's a preprocessor
> macro - so it's only actually evaluated in contexts where PG_waiters
> is defined. With instrumentation however, it becomes a static inline
> function, and all of a sudden we need the actual value of PG_waiters.
> Because of the order of header includes, it's not available and we
> fail to compile. Instead, manually specify that we care about bit 7.
> This is still correct: bit 7 is the bit that would mark a negative
> byte.
>
> Cc: Nicholas Piggin <npiggin@gmail.com> # clear_bit_unlock_negative_byte
> Signed-off-by: Daniel Axtens <dja@axtens.net>
Reviewed-by: Christophe Leroy <christophe.leroy@c-s.fr>
Note that this patch might be an opportunity to replace all the
'__inline__' by the standard 'inline' keyword.
Some () alignment to be fixes as well, see checkpatch warnings/checks at
https://openpower.xyz/job/snowpatch/job/snowpatch-linux-checkpatch/8601//artifact/linux/checkpatch.log
> ---
> arch/powerpc/include/asm/bitops.h | 31 +++++++++++++++++++------------
> 1 file changed, 19 insertions(+), 12 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/bitops.h b/arch/powerpc/include/asm/bitops.h
> index 603aed229af7..8615b2bc35fe 100644
> --- a/arch/powerpc/include/asm/bitops.h
> +++ b/arch/powerpc/include/asm/bitops.h
> @@ -86,22 +86,22 @@ DEFINE_BITOP(clear_bits, andc, "")
> DEFINE_BITOP(clear_bits_unlock, andc, PPC_RELEASE_BARRIER)
> DEFINE_BITOP(change_bits, xor, "")
>
> -static __inline__ void set_bit(int nr, volatile unsigned long *addr)
> +static __inline__ void arch_set_bit(int nr, volatile unsigned long *addr)
> {
> set_bits(BIT_MASK(nr), addr + BIT_WORD(nr));
> }
>
> -static __inline__ void clear_bit(int nr, volatile unsigned long *addr)
> +static __inline__ void arch_clear_bit(int nr, volatile unsigned long *addr)
> {
> clear_bits(BIT_MASK(nr), addr + BIT_WORD(nr));
> }
>
> -static __inline__ void clear_bit_unlock(int nr, volatile unsigned long *addr)
> +static __inline__ void arch_clear_bit_unlock(int nr, volatile unsigned long *addr)
> {
> clear_bits_unlock(BIT_MASK(nr), addr + BIT_WORD(nr));
> }
>
> -static __inline__ void change_bit(int nr, volatile unsigned long *addr)
> +static __inline__ void arch_change_bit(int nr, volatile unsigned long *addr)
> {
> change_bits(BIT_MASK(nr), addr + BIT_WORD(nr));
> }
> @@ -138,26 +138,26 @@ DEFINE_TESTOP(test_and_clear_bits, andc, PPC_ATOMIC_ENTRY_BARRIER,
> DEFINE_TESTOP(test_and_change_bits, xor, PPC_ATOMIC_ENTRY_BARRIER,
> PPC_ATOMIC_EXIT_BARRIER, 0)
>
> -static __inline__ int test_and_set_bit(unsigned long nr,
> +static __inline__ int arch_test_and_set_bit(unsigned long nr,
> volatile unsigned long *addr)
> {
> return test_and_set_bits(BIT_MASK(nr), addr + BIT_WORD(nr)) != 0;
> }
>
> -static __inline__ int test_and_set_bit_lock(unsigned long nr,
> +static __inline__ int arch_test_and_set_bit_lock(unsigned long nr,
> volatile unsigned long *addr)
> {
> return test_and_set_bits_lock(BIT_MASK(nr),
> addr + BIT_WORD(nr)) != 0;
> }
>
> -static __inline__ int test_and_clear_bit(unsigned long nr,
> +static __inline__ int arch_test_and_clear_bit(unsigned long nr,
> volatile unsigned long *addr)
> {
> return test_and_clear_bits(BIT_MASK(nr), addr + BIT_WORD(nr)) != 0;
> }
>
> -static __inline__ int test_and_change_bit(unsigned long nr,
> +static __inline__ int arch_test_and_change_bit(unsigned long nr,
> volatile unsigned long *addr)
> {
> return test_and_change_bits(BIT_MASK(nr), addr + BIT_WORD(nr)) != 0;
> @@ -185,15 +185,18 @@ static __inline__ unsigned long clear_bit_unlock_return_word(int nr,
> return old;
> }
>
> -/* This is a special function for mm/filemap.c */
> -#define clear_bit_unlock_is_negative_byte(nr, addr) \
> - (clear_bit_unlock_return_word(nr, addr) & BIT_MASK(PG_waiters))
> +/*
> + * This is a special function for mm/filemap.c
> + * Bit 7 corresponds to PG_waiters.
> + */
> +#define arch_clear_bit_unlock_is_negative_byte(nr, addr) \
> + (clear_bit_unlock_return_word(nr, addr) & BIT_MASK(7))
>
> #endif /* CONFIG_PPC64 */
>
> #include <asm-generic/bitops/non-atomic.h>
>
> -static __inline__ void __clear_bit_unlock(int nr, volatile unsigned long *addr)
> +static __inline__ void arch___clear_bit_unlock(int nr, volatile unsigned long *addr)
> {
> __asm__ __volatile__(PPC_RELEASE_BARRIER "" ::: "memory");
> __clear_bit(nr, addr);
> @@ -239,6 +242,10 @@ unsigned long __arch_hweight64(__u64 w);
>
> #include <asm-generic/bitops/find.h>
>
> +/* wrappers that deal with KASAN instrumentation */
> +#include <asm-generic/bitops/instrumented-atomic.h>
> +#include <asm-generic/bitops/instrumented-lock.h>
> +
> /* Little-endian versions */
> #include <asm-generic/bitops/le.h>
>
>
^ permalink raw reply
* Re: [PATCH 1/2] kasan: support instrumented bitops combined with generic bitops
From: Christophe Leroy @ 2019-08-19 11:13 UTC (permalink / raw)
To: Daniel Axtens, linux-s390, linux-arch, x86, linuxppc-dev; +Cc: kasan-dev
In-Reply-To: <20190819062814.5315-1-dja@axtens.net>
Le 19/08/2019 à 08:28, Daniel Axtens a écrit :
> Currently bitops-instrumented.h assumes that the architecture provides
> atomic, non-atomic and locking bitops (e.g. both set_bit and __set_bit).
> This is true on x86 and s390, but is not always true: there is a
> generic bitops/non-atomic.h header that provides generic non-atomic
> operations, and also a generic bitops/lock.h for locking operations.
>
> powerpc uses the generic non-atomic version, so it does not have it's
> own e.g. __set_bit that could be renamed arch___set_bit.
>
> Split up bitops-instrumented.h to mirror the atomic/non-atomic/lock
> split. This allows arches to only include the headers where they
> have arch-specific versions to rename. Update x86 and s390.
>
> (The generic operations are automatically instrumented because they're
> written in C, not asm.)
>
> Suggested-by: Christophe Leroy <christophe.leroy@c-s.fr>
> Signed-off-by: Daniel Axtens <dja@axtens.net>
Reviewed-by: Christophe Leroy <christophe.leroy@c-s.fr>
> ---
> Documentation/core-api/kernel-api.rst | 17 +-
> arch/s390/include/asm/bitops.h | 4 +-
> arch/x86/include/asm/bitops.h | 4 +-
> include/asm-generic/bitops-instrumented.h | 263 ------------------
> .../asm-generic/bitops/instrumented-atomic.h | 100 +++++++
> .../asm-generic/bitops/instrumented-lock.h | 81 ++++++
> .../bitops/instrumented-non-atomic.h | 114 ++++++++
> 7 files changed, 317 insertions(+), 266 deletions(-)
> delete mode 100644 include/asm-generic/bitops-instrumented.h
> create mode 100644 include/asm-generic/bitops/instrumented-atomic.h
> create mode 100644 include/asm-generic/bitops/instrumented-lock.h
> create mode 100644 include/asm-generic/bitops/instrumented-non-atomic.h
>
> diff --git a/Documentation/core-api/kernel-api.rst b/Documentation/core-api/kernel-api.rst
> index 08af5caf036d..2e21248277e3 100644
> --- a/Documentation/core-api/kernel-api.rst
> +++ b/Documentation/core-api/kernel-api.rst
> @@ -54,7 +54,22 @@ The Linux kernel provides more basic utility functions.
> Bit Operations
> --------------
>
> -.. kernel-doc:: include/asm-generic/bitops-instrumented.h
> +Atomic Operations
> +~~~~~~~~~~~~~~~~~
> +
> +.. kernel-doc:: include/asm-generic/bitops/instrumented-atomic.h
> + :internal:
> +
> +Non-atomic Operations
> +~~~~~~~~~~~~~~~~~~~~~
> +
> +.. kernel-doc:: include/asm-generic/bitops/instrumented-non-atomic.h
> + :internal:
> +
> +Locking Operations
> +~~~~~~~~~~~~~~~~~~
> +
> +.. kernel-doc:: include/asm-generic/bitops/instrumented-lock.h
> :internal:
>
> Bitmap Operations
> diff --git a/arch/s390/include/asm/bitops.h b/arch/s390/include/asm/bitops.h
> index b8833ac983fa..0ceb12593a68 100644
> --- a/arch/s390/include/asm/bitops.h
> +++ b/arch/s390/include/asm/bitops.h
> @@ -241,7 +241,9 @@ static inline void arch___clear_bit_unlock(unsigned long nr,
> arch___clear_bit(nr, ptr);
> }
>
> -#include <asm-generic/bitops-instrumented.h>
> +#include <asm-generic/bitops/instrumented-atomic.h>
> +#include <asm-generic/bitops/instrumented-non-atomic.h>
> +#include <asm-generic/bitops/instrumented-lock.h>
>
> /*
> * Functions which use MSB0 bit numbering.
> diff --git a/arch/x86/include/asm/bitops.h b/arch/x86/include/asm/bitops.h
> index ba15d53c1ca7..4a2e2432238f 100644
> --- a/arch/x86/include/asm/bitops.h
> +++ b/arch/x86/include/asm/bitops.h
> @@ -389,7 +389,9 @@ static __always_inline int fls64(__u64 x)
>
> #include <asm-generic/bitops/const_hweight.h>
>
> -#include <asm-generic/bitops-instrumented.h>
> +#include <asm-generic/bitops/instrumented-atomic.h>
> +#include <asm-generic/bitops/instrumented-non-atomic.h>
> +#include <asm-generic/bitops/instrumented-lock.h>
>
> #include <asm-generic/bitops/le.h>
>
> diff --git a/include/asm-generic/bitops-instrumented.h b/include/asm-generic/bitops-instrumented.h
> deleted file mode 100644
> index ddd1c6d9d8db..000000000000
> --- a/include/asm-generic/bitops-instrumented.h
> +++ /dev/null
> @@ -1,263 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0 */
> -
> -/*
> - * This file provides wrappers with sanitizer instrumentation for bit
> - * operations.
> - *
> - * To use this functionality, an arch's bitops.h file needs to define each of
> - * the below bit operations with an arch_ prefix (e.g. arch_set_bit(),
> - * arch___set_bit(), etc.).
> - */
> -#ifndef _ASM_GENERIC_BITOPS_INSTRUMENTED_H
> -#define _ASM_GENERIC_BITOPS_INSTRUMENTED_H
> -
> -#include <linux/kasan-checks.h>
> -
> -/**
> - * set_bit - Atomically set a bit in memory
> - * @nr: the bit to set
> - * @addr: the address to start counting from
> - *
> - * This is a relaxed atomic operation (no implied memory barriers).
> - *
> - * Note that @nr may be almost arbitrarily large; this function is not
> - * restricted to acting on a single-word quantity.
> - */
> -static inline void set_bit(long nr, volatile unsigned long *addr)
> -{
> - kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
> - arch_set_bit(nr, addr);
> -}
> -
> -/**
> - * __set_bit - Set a bit in memory
> - * @nr: the bit to set
> - * @addr: the address to start counting from
> - *
> - * Unlike set_bit(), this function is non-atomic. If it is called on the same
> - * region of memory concurrently, the effect may be that only one operation
> - * succeeds.
> - */
> -static inline void __set_bit(long nr, volatile unsigned long *addr)
> -{
> - kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
> - arch___set_bit(nr, addr);
> -}
> -
> -/**
> - * clear_bit - Clears a bit in memory
> - * @nr: Bit to clear
> - * @addr: Address to start counting from
> - *
> - * This is a relaxed atomic operation (no implied memory barriers).
> - */
> -static inline void clear_bit(long nr, volatile unsigned long *addr)
> -{
> - kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
> - arch_clear_bit(nr, addr);
> -}
> -
> -/**
> - * __clear_bit - Clears a bit in memory
> - * @nr: the bit to clear
> - * @addr: the address to start counting from
> - *
> - * Unlike clear_bit(), this function is non-atomic. If it is called on the same
> - * region of memory concurrently, the effect may be that only one operation
> - * succeeds.
> - */
> -static inline void __clear_bit(long nr, volatile unsigned long *addr)
> -{
> - kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
> - arch___clear_bit(nr, addr);
> -}
> -
> -/**
> - * clear_bit_unlock - Clear a bit in memory, for unlock
> - * @nr: the bit to set
> - * @addr: the address to start counting from
> - *
> - * This operation is atomic and provides release barrier semantics.
> - */
> -static inline void clear_bit_unlock(long nr, volatile unsigned long *addr)
> -{
> - kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
> - arch_clear_bit_unlock(nr, addr);
> -}
> -
> -/**
> - * __clear_bit_unlock - Clears a bit in memory
> - * @nr: Bit to clear
> - * @addr: Address to start counting from
> - *
> - * This is a non-atomic operation but implies a release barrier before the
> - * memory operation. It can be used for an unlock if no other CPUs can
> - * concurrently modify other bits in the word.
> - */
> -static inline void __clear_bit_unlock(long nr, volatile unsigned long *addr)
> -{
> - kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
> - arch___clear_bit_unlock(nr, addr);
> -}
> -
> -/**
> - * change_bit - Toggle a bit in memory
> - * @nr: Bit to change
> - * @addr: Address to start counting from
> - *
> - * This is a relaxed atomic operation (no implied memory barriers).
> - *
> - * Note that @nr may be almost arbitrarily large; this function is not
> - * restricted to acting on a single-word quantity.
> - */
> -static inline void change_bit(long nr, volatile unsigned long *addr)
> -{
> - kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
> - arch_change_bit(nr, addr);
> -}
> -
> -/**
> - * __change_bit - Toggle a bit in memory
> - * @nr: the bit to change
> - * @addr: the address to start counting from
> - *
> - * Unlike change_bit(), this function is non-atomic. If it is called on the same
> - * region of memory concurrently, the effect may be that only one operation
> - * succeeds.
> - */
> -static inline void __change_bit(long nr, volatile unsigned long *addr)
> -{
> - kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
> - arch___change_bit(nr, addr);
> -}
> -
> -/**
> - * test_and_set_bit - Set a bit and return its old value
> - * @nr: Bit to set
> - * @addr: Address to count from
> - *
> - * This is an atomic fully-ordered operation (implied full memory barrier).
> - */
> -static inline bool test_and_set_bit(long nr, volatile unsigned long *addr)
> -{
> - kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
> - return arch_test_and_set_bit(nr, addr);
> -}
> -
> -/**
> - * __test_and_set_bit - Set a bit and return its old value
> - * @nr: Bit to set
> - * @addr: Address to count from
> - *
> - * This operation is non-atomic. If two instances of this operation race, one
> - * can appear to succeed but actually fail.
> - */
> -static inline bool __test_and_set_bit(long nr, volatile unsigned long *addr)
> -{
> - kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
> - return arch___test_and_set_bit(nr, addr);
> -}
> -
> -/**
> - * test_and_set_bit_lock - Set a bit and return its old value, for lock
> - * @nr: Bit to set
> - * @addr: Address to count from
> - *
> - * This operation is atomic and provides acquire barrier semantics if
> - * the returned value is 0.
> - * It can be used to implement bit locks.
> - */
> -static inline bool test_and_set_bit_lock(long nr, volatile unsigned long *addr)
> -{
> - kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
> - return arch_test_and_set_bit_lock(nr, addr);
> -}
> -
> -/**
> - * test_and_clear_bit - Clear a bit and return its old value
> - * @nr: Bit to clear
> - * @addr: Address to count from
> - *
> - * This is an atomic fully-ordered operation (implied full memory barrier).
> - */
> -static inline bool test_and_clear_bit(long nr, volatile unsigned long *addr)
> -{
> - kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
> - return arch_test_and_clear_bit(nr, addr);
> -}
> -
> -/**
> - * __test_and_clear_bit - Clear a bit and return its old value
> - * @nr: Bit to clear
> - * @addr: Address to count from
> - *
> - * This operation is non-atomic. If two instances of this operation race, one
> - * can appear to succeed but actually fail.
> - */
> -static inline bool __test_and_clear_bit(long nr, volatile unsigned long *addr)
> -{
> - kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
> - return arch___test_and_clear_bit(nr, addr);
> -}
> -
> -/**
> - * test_and_change_bit - Change a bit and return its old value
> - * @nr: Bit to change
> - * @addr: Address to count from
> - *
> - * This is an atomic fully-ordered operation (implied full memory barrier).
> - */
> -static inline bool test_and_change_bit(long nr, volatile unsigned long *addr)
> -{
> - kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
> - return arch_test_and_change_bit(nr, addr);
> -}
> -
> -/**
> - * __test_and_change_bit - Change a bit and return its old value
> - * @nr: Bit to change
> - * @addr: Address to count from
> - *
> - * This operation is non-atomic. If two instances of this operation race, one
> - * can appear to succeed but actually fail.
> - */
> -static inline bool __test_and_change_bit(long nr, volatile unsigned long *addr)
> -{
> - kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
> - return arch___test_and_change_bit(nr, addr);
> -}
> -
> -/**
> - * test_bit - Determine whether a bit is set
> - * @nr: bit number to test
> - * @addr: Address to start counting from
> - */
> -static inline bool test_bit(long nr, const volatile unsigned long *addr)
> -{
> - kasan_check_read(addr + BIT_WORD(nr), sizeof(long));
> - return arch_test_bit(nr, addr);
> -}
> -
> -#if defined(arch_clear_bit_unlock_is_negative_byte)
> -/**
> - * clear_bit_unlock_is_negative_byte - Clear a bit in memory and test if bottom
> - * byte is negative, for unlock.
> - * @nr: the bit to clear
> - * @addr: the address to start counting from
> - *
> - * This operation is atomic and provides release barrier semantics.
> - *
> - * This is a bit of a one-trick-pony for the filemap code, which clears
> - * PG_locked and tests PG_waiters,
> - */
> -static inline bool
> -clear_bit_unlock_is_negative_byte(long nr, volatile unsigned long *addr)
> -{
> - kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
> - return arch_clear_bit_unlock_is_negative_byte(nr, addr);
> -}
> -/* Let everybody know we have it. */
> -#define clear_bit_unlock_is_negative_byte clear_bit_unlock_is_negative_byte
> -#endif
> -
> -#endif /* _ASM_GENERIC_BITOPS_INSTRUMENTED_H */
> diff --git a/include/asm-generic/bitops/instrumented-atomic.h b/include/asm-generic/bitops/instrumented-atomic.h
> new file mode 100644
> index 000000000000..18ce3c9e8eec
> --- /dev/null
> +++ b/include/asm-generic/bitops/instrumented-atomic.h
> @@ -0,0 +1,100 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +/*
> + * This file provides wrappers with sanitizer instrumentation for atomic bit
> + * operations.
> + *
> + * To use this functionality, an arch's bitops.h file needs to define each of
> + * the below bit operations with an arch_ prefix (e.g. arch_set_bit(),
> + * arch___set_bit(), etc.).
> + */
> +#ifndef _ASM_GENERIC_BITOPS_INSTRUMENTED_ATOMIC_H
> +#define _ASM_GENERIC_BITOPS_INSTRUMENTED_ATOMIC_H
> +
> +#include <linux/kasan-checks.h>
> +
> +/**
> + * set_bit - Atomically set a bit in memory
> + * @nr: the bit to set
> + * @addr: the address to start counting from
> + *
> + * This is a relaxed atomic operation (no implied memory barriers).
> + *
> + * Note that @nr may be almost arbitrarily large; this function is not
> + * restricted to acting on a single-word quantity.
> + */
> +static inline void set_bit(long nr, volatile unsigned long *addr)
> +{
> + kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
> + arch_set_bit(nr, addr);
> +}
> +
> +/**
> + * clear_bit - Clears a bit in memory
> + * @nr: Bit to clear
> + * @addr: Address to start counting from
> + *
> + * This is a relaxed atomic operation (no implied memory barriers).
> + */
> +static inline void clear_bit(long nr, volatile unsigned long *addr)
> +{
> + kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
> + arch_clear_bit(nr, addr);
> +}
> +
> +/**
> + * change_bit - Toggle a bit in memory
> + * @nr: Bit to change
> + * @addr: Address to start counting from
> + *
> + * This is a relaxed atomic operation (no implied memory barriers).
> + *
> + * Note that @nr may be almost arbitrarily large; this function is not
> + * restricted to acting on a single-word quantity.
> + */
> +static inline void change_bit(long nr, volatile unsigned long *addr)
> +{
> + kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
> + arch_change_bit(nr, addr);
> +}
> +
> +/**
> + * test_and_set_bit - Set a bit and return its old value
> + * @nr: Bit to set
> + * @addr: Address to count from
> + *
> + * This is an atomic fully-ordered operation (implied full memory barrier).
> + */
> +static inline bool test_and_set_bit(long nr, volatile unsigned long *addr)
> +{
> + kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
> + return arch_test_and_set_bit(nr, addr);
> +}
> +
> +/**
> + * test_and_clear_bit - Clear a bit and return its old value
> + * @nr: Bit to clear
> + * @addr: Address to count from
> + *
> + * This is an atomic fully-ordered operation (implied full memory barrier).
> + */
> +static inline bool test_and_clear_bit(long nr, volatile unsigned long *addr)
> +{
> + kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
> + return arch_test_and_clear_bit(nr, addr);
> +}
> +
> +/**
> + * test_and_change_bit - Change a bit and return its old value
> + * @nr: Bit to change
> + * @addr: Address to count from
> + *
> + * This is an atomic fully-ordered operation (implied full memory barrier).
> + */
> +static inline bool test_and_change_bit(long nr, volatile unsigned long *addr)
> +{
> + kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
> + return arch_test_and_change_bit(nr, addr);
> +}
> +
> +#endif /* _ASM_GENERIC_BITOPS_INSTRUMENTED_NON_ATOMIC_H */
> diff --git a/include/asm-generic/bitops/instrumented-lock.h b/include/asm-generic/bitops/instrumented-lock.h
> new file mode 100644
> index 000000000000..ec53fdeea9ec
> --- /dev/null
> +++ b/include/asm-generic/bitops/instrumented-lock.h
> @@ -0,0 +1,81 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +/*
> + * This file provides wrappers with sanitizer instrumentation for bit
> + * locking operations.
> + *
> + * To use this functionality, an arch's bitops.h file needs to define each of
> + * the below bit operations with an arch_ prefix (e.g. arch_set_bit(),
> + * arch___set_bit(), etc.).
> + */
> +#ifndef _ASM_GENERIC_BITOPS_INSTRUMENTED_LOCK_H
> +#define _ASM_GENERIC_BITOPS_INSTRUMENTED_LOCK_H
> +
> +#include <linux/kasan-checks.h>
> +
> +/**
> + * clear_bit_unlock - Clear a bit in memory, for unlock
> + * @nr: the bit to set
> + * @addr: the address to start counting from
> + *
> + * This operation is atomic and provides release barrier semantics.
> + */
> +static inline void clear_bit_unlock(long nr, volatile unsigned long *addr)
> +{
> + kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
> + arch_clear_bit_unlock(nr, addr);
> +}
> +
> +/**
> + * __clear_bit_unlock - Clears a bit in memory
> + * @nr: Bit to clear
> + * @addr: Address to start counting from
> + *
> + * This is a non-atomic operation but implies a release barrier before the
> + * memory operation. It can be used for an unlock if no other CPUs can
> + * concurrently modify other bits in the word.
> + */
> +static inline void __clear_bit_unlock(long nr, volatile unsigned long *addr)
> +{
> + kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
> + arch___clear_bit_unlock(nr, addr);
> +}
> +
> +/**
> + * test_and_set_bit_lock - Set a bit and return its old value, for lock
> + * @nr: Bit to set
> + * @addr: Address to count from
> + *
> + * This operation is atomic and provides acquire barrier semantics if
> + * the returned value is 0.
> + * It can be used to implement bit locks.
> + */
> +static inline bool test_and_set_bit_lock(long nr, volatile unsigned long *addr)
> +{
> + kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
> + return arch_test_and_set_bit_lock(nr, addr);
> +}
> +
> +#if defined(arch_clear_bit_unlock_is_negative_byte)
> +/**
> + * clear_bit_unlock_is_negative_byte - Clear a bit in memory and test if bottom
> + * byte is negative, for unlock.
> + * @nr: the bit to clear
> + * @addr: the address to start counting from
> + *
> + * This operation is atomic and provides release barrier semantics.
> + *
> + * This is a bit of a one-trick-pony for the filemap code, which clears
> + * PG_locked and tests PG_waiters,
> + */
> +static inline bool
> +clear_bit_unlock_is_negative_byte(long nr, volatile unsigned long *addr)
> +{
> + kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
> + return arch_clear_bit_unlock_is_negative_byte(nr, addr);
> +}
> +/* Let everybody know we have it. */
> +#define clear_bit_unlock_is_negative_byte clear_bit_unlock_is_negative_byte
> +#endif
> +
> +#endif /* _ASM_GENERIC_BITOPS_INSTRUMENTED_LOCK_H */
> diff --git a/include/asm-generic/bitops/instrumented-non-atomic.h b/include/asm-generic/bitops/instrumented-non-atomic.h
> new file mode 100644
> index 000000000000..95ff28d128a1
> --- /dev/null
> +++ b/include/asm-generic/bitops/instrumented-non-atomic.h
> @@ -0,0 +1,114 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +/*
> + * This file provides wrappers with sanitizer instrumentation for non-atomic
> + * bit operations.
> + *
> + * To use this functionality, an arch's bitops.h file needs to define each of
> + * the below bit operations with an arch_ prefix (e.g. arch_set_bit(),
> + * arch___set_bit(), etc.).
> + */
> +#ifndef _ASM_GENERIC_BITOPS_INSTRUMENTED_NON_ATOMIC_H
> +#define _ASM_GENERIC_BITOPS_INSTRUMENTED_NON_ATOMIC_H
> +
> +#include <linux/kasan-checks.h>
> +
> +/**
> + * __set_bit - Set a bit in memory
> + * @nr: the bit to set
> + * @addr: the address to start counting from
> + *
> + * Unlike set_bit(), this function is non-atomic. If it is called on the same
> + * region of memory concurrently, the effect may be that only one operation
> + * succeeds.
> + */
> +static inline void __set_bit(long nr, volatile unsigned long *addr)
> +{
> + kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
> + arch___set_bit(nr, addr);
> +}
> +
> +/**
> + * __clear_bit - Clears a bit in memory
> + * @nr: the bit to clear
> + * @addr: the address to start counting from
> + *
> + * Unlike clear_bit(), this function is non-atomic. If it is called on the same
> + * region of memory concurrently, the effect may be that only one operation
> + * succeeds.
> + */
> +static inline void __clear_bit(long nr, volatile unsigned long *addr)
> +{
> + kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
> + arch___clear_bit(nr, addr);
> +}
> +
> +/**
> + * __change_bit - Toggle a bit in memory
> + * @nr: the bit to change
> + * @addr: the address to start counting from
> + *
> + * Unlike change_bit(), this function is non-atomic. If it is called on the same
> + * region of memory concurrently, the effect may be that only one operation
> + * succeeds.
> + */
> +static inline void __change_bit(long nr, volatile unsigned long *addr)
> +{
> + kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
> + arch___change_bit(nr, addr);
> +}
> +
> +/**
> + * __test_and_set_bit - Set a bit and return its old value
> + * @nr: Bit to set
> + * @addr: Address to count from
> + *
> + * This operation is non-atomic. If two instances of this operation race, one
> + * can appear to succeed but actually fail.
> + */
> +static inline bool __test_and_set_bit(long nr, volatile unsigned long *addr)
> +{
> + kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
> + return arch___test_and_set_bit(nr, addr);
> +}
> +
> +/**
> + * __test_and_clear_bit - Clear a bit and return its old value
> + * @nr: Bit to clear
> + * @addr: Address to count from
> + *
> + * This operation is non-atomic. If two instances of this operation race, one
> + * can appear to succeed but actually fail.
> + */
> +static inline bool __test_and_clear_bit(long nr, volatile unsigned long *addr)
> +{
> + kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
> + return arch___test_and_clear_bit(nr, addr);
> +}
> +
> +/**
> + * __test_and_change_bit - Change a bit and return its old value
> + * @nr: Bit to change
> + * @addr: Address to count from
> + *
> + * This operation is non-atomic. If two instances of this operation race, one
> + * can appear to succeed but actually fail.
> + */
> +static inline bool __test_and_change_bit(long nr, volatile unsigned long *addr)
> +{
> + kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
> + return arch___test_and_change_bit(nr, addr);
> +}
> +
> +/**
> + * test_bit - Determine whether a bit is set
> + * @nr: bit number to test
> + * @addr: Address to start counting from
> + */
> +static inline bool test_bit(long nr, const volatile unsigned long *addr)
> +{
> + kasan_check_read(addr + BIT_WORD(nr), sizeof(long));
> + return arch_test_bit(nr, addr);
> +}
> +
> +#endif /* _ASM_GENERIC_BITOPS_INSTRUMENTED_NON_ATOMIC_H */
>
^ permalink raw reply
* Re: [5.3.0-rc4-next][bisected 882632][qla2xxx] WARNING: CPU: 10 PID: 425 at drivers/scsi/qla2xxx/qla_isr.c:2784 qla2x00_status_entry.isra
From: Abdul Haleem @ 2019-08-19 10:26 UTC (permalink / raw)
To: Bart Van Assche
Cc: sachinp, Stephen Rothwell, linux-scsi, martin.petersen, manvanth,
linux-kernel, linux-next, hmadhani, linuxppc-dev
In-Reply-To: <7fc59d4c-b3d5-5ec8-cb7c-51cb863f2a77@acm.org>
On Wed, 2019-08-14 at 20:42 -0700, Bart Van Assche wrote:
> On 8/14/19 10:18 AM, Abdul Haleem wrote:
> > On Wed, 2019-08-14 at 10:05 -0700, Bart Van Assche wrote:
> >> On 8/14/19 9:52 AM, Abdul Haleem wrote:
> >>> Greeting's
> >>>
> >>> Today's linux-next kernel (5.3.0-rc4-next-20190813) booted with warning on my powerpc power 8 lpar
> >>>
> >>> The WARN_ON_ONCE() was introduced by commit 88263208 (scsi: qla2xxx: Complain if sp->done() is not...)
> >>>
> >>> boot logs:
> >>>
> >>> WARNING: CPU: 10 PID: 425 at drivers/scsi/qla2xxx/qla_isr.c:2784
> >>
> >> Hi Abdul,
> >>
> >> Thank you for having reported this. Is that the only warning reported on your setup by the qla2xxx
> >> driver? If that warning is commented out, does the qla2xxx driver work as expected?
> >
> > boot warning did not show up when the commit is reverted.
> >
> > should I comment out only the WARN_ON_ONCE() which is causing the issue,
> > and not the other one ?
>
> Yes please. Commit 88263208 introduced five kernel warnings but I think
> only one of these should be removed again, e.g. as follows:
>
> diff --git a/drivers/scsi/qla2xxx/qla_isr.c b/drivers/scsi/qla2xxx/qla_isr.c
> index cd39ac18c5fd..d81b5ecce24b 100644
> --- a/drivers/scsi/qla2xxx/qla_isr.c
> +++ b/drivers/scsi/qla2xxx/qla_isr.c
> @@ -2780,8 +2780,6 @@ qla2x00_status_entry(scsi_qla_host_t *vha, struct
> rsp_que *rsp, void *pkt)
>
> if (rsp->status_srb == NULL)
> sp->done(sp, res);
> - else
> - WARN_ON_ONCE(true);
> }
>
> /**
Applying above patch on system boots fine.
i.e no warnings pop up when keeping all WARN_ON_ONCE() except above one.
Reported-and-Tested-by: Abdul Haleem <abdhalee@linux.vnet.ibm.com>
--
Regard's
Abdul Haleem
IBM Linux Technology Centre
^ permalink raw reply
* Re: [PATCH v4 1/3] kasan: support backing vmalloc space with real shadow memory
From: Mark Rutland @ 2019-08-19 10:15 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Vasily Gorbik, X86 ML, LKML, kasan-dev, Linux-MM,
Alexander Potapenko, Andrey Ryabinin, linuxppc-dev, Dmitry Vyukov,
Daniel Axtens
In-Reply-To: <CALCETrUn4FNjvRoJW77DNi5vdwO+EURUC_46tysjPQD0MM3THQ@mail.gmail.com>
On Fri, Aug 16, 2019 at 10:41:00AM -0700, Andy Lutomirski wrote:
> On Fri, Aug 16, 2019 at 10:08 AM Mark Rutland <mark.rutland@arm.com> wrote:
> >
> > Hi Christophe,
> >
> > On Fri, Aug 16, 2019 at 09:47:00AM +0200, Christophe Leroy wrote:
> > > Le 15/08/2019 à 02:16, Daniel Axtens a écrit :
> > > > Hook into vmalloc and vmap, and dynamically allocate real shadow
> > > > memory to back the mappings.
> > > >
> > > > Most mappings in vmalloc space are small, requiring less than a full
> > > > page of shadow space. Allocating a full shadow page per mapping would
> > > > therefore be wasteful. Furthermore, to ensure that different mappings
> > > > use different shadow pages, mappings would have to be aligned to
> > > > KASAN_SHADOW_SCALE_SIZE * PAGE_SIZE.
> > > >
> > > > Instead, share backing space across multiple mappings. Allocate
> > > > a backing page the first time a mapping in vmalloc space uses a
> > > > particular page of the shadow region. Keep this page around
> > > > regardless of whether the mapping is later freed - in the mean time
> > > > the page could have become shared by another vmalloc mapping.
> > > >
> > > > This can in theory lead to unbounded memory growth, but the vmalloc
> > > > allocator is pretty good at reusing addresses, so the practical memory
> > > > usage grows at first but then stays fairly stable.
> > >
> > > I guess people having gigabytes of memory don't mind, but I'm concerned
> > > about tiny targets with very little amount of memory. I have boards with as
> > > little as 32Mbytes of RAM. The shadow region for the linear space already
> > > takes one eighth of the RAM. I'd rather avoid keeping unused shadow pages
> > > busy.
> >
> > I think this depends on how much shadow would be in constant use vs what
> > would get left unused. If the amount in constant use is sufficiently
> > large (or the residue is sufficiently small), then it may not be
> > worthwhile to support KASAN_VMALLOC on such small systems.
> >
> > > Each page of shadow memory represent 8 pages of real memory. Could we use
> > > page_ref to count how many pieces of a shadow page are used so that we can
> > > free it when the ref count decreases to 0.
> > >
> > > > This requires architecture support to actually use: arches must stop
> > > > mapping the read-only zero page over portion of the shadow region that
> > > > covers the vmalloc space and instead leave it unmapped.
> > >
> > > Why 'must' ? Couldn't we switch back and forth from the zero page to real
> > > page on demand ?
> > >
> > > If the zero page is not mapped for unused vmalloc space, bad memory accesses
> > > will Oops on the shadow memory access instead of Oopsing on the real bad
> > > access, making it more difficult to locate and identify the issue.
> >
> > I agree this isn't nice, though FWIW this can already happen today for
> > bad addresses that fall outside of the usual kernel address space. We
> > could make the !KASAN_INLINE checks resilient to this by using
> > probe_kernel_read() to check the shadow, and treating unmapped shadow as
> > poison.
>
> Could we instead modify the page fault handlers to detect this case
> and print a useful message?
In general we can't know if a bad access was a KASAN shadow lookup (e.g.
since the shadow of NULL falls outside of the shadow region), but we
could always print a message using kasan_shadow_to_mem() for any
unhandled fault to suggeest what the "real" address might have been.
Thanks,
Mark.
^ permalink raw reply
* Re: [PATCH v5 3/4] mm/nvdimm: Use correct #defines instead of open coding
From: Aneesh Kumar K.V @ 2019-08-19 9:30 UTC (permalink / raw)
To: Dan Williams; +Cc: Linux MM, linuxppc-dev, linux-nvdimm
In-Reply-To: <87v9ut1vev.fsf@linux.ibm.com>
Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> writes:
> Dan Williams <dan.j.williams@intel.com> writes:
>
>> On Fri, Aug 9, 2019 at 12:45 AM Aneesh Kumar K.V
>> <aneesh.kumar@linux.ibm.com> wrote:
>>>
>>
...
>>> diff --git a/drivers/nvdimm/pfn_devs.c b/drivers/nvdimm/pfn_devs.c
>>> index 37e96811c2fc..c1d9be609322 100644
>>> --- a/drivers/nvdimm/pfn_devs.c
>>> +++ b/drivers/nvdimm/pfn_devs.c
>>> @@ -725,7 +725,8 @@ static int nd_pfn_init(struct nd_pfn *nd_pfn)
>>> * when populating the vmemmap. This *should* be equal to
>>> * PMD_SIZE for most architectures.
>>> */
>>> - offset = ALIGN(start + SZ_8K + 64 * npfns, align) - start;
>>> + offset = ALIGN(start + SZ_8K + sizeof(struct page) * npfns,
>>
>> I'd prefer if this was not dynamic and was instead set to the maximum
>> size of 'struct page' across all archs just to enhance cross-arch
>> compatibility. I think that answer is '64'.
>
>
> That still doesn't take care of the case where we add new elements to
> struct page later. If we have struct page size changing across
> architectures, we should still be ok as long as new size is less than what is
> stored in pfn superblock? I understand the desire to keep it
> non-dynamic. But we also need to make sure we don't reserve less space
> when creating a new namespace on a config that got struct page size >
> 64?
How about
libnvdimm/pfn_dev: Add a build check to make sure we notice when struct page size change
When namespace is created with map device as pmem device, struct page is stored in the
reserve block area. We need to make sure we account for the right struct page
size while doing this. Instead of directly depending on sizeof(struct page)
which can change based on different kernel config option, use the max struct
page size (64) while calculating the reserve block area. This makes sure pmem
device can be used across kernels built with different configs.
If the above assumption of max struct page size change, we need to update the
reserve block allocation space for new namespaces created.
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
1 file changed, 7 insertions(+)
drivers/nvdimm/pfn_devs.c | 7 +++++++
modified drivers/nvdimm/pfn_devs.c
@@ -722,7 +722,14 @@ static int nd_pfn_init(struct nd_pfn *nd_pfn)
* The altmap should be padded out to the block size used
* when populating the vmemmap. This *should* be equal to
* PMD_SIZE for most architectures.
+ *
+ * Also make sure size of struct page is less than 64. We
+ * want to make sure we use large enough size here so that
+ * we don't have a dynamic reserve space depending on
+ * struct page size. But we also want to make sure we notice
+ * if we end up adding new elements to struct page.
*/
+ BUILD_BUG_ON(64 < sizeof(struct page));
offset = ALIGN(start + SZ_8K + 64 * npfns, align) - start;
} else if (nd_pfn->mode == PFN_MODE_RAM)
offset = ALIGN(start + SZ_8K, align) - start;
-aneesh
^ permalink raw reply
* Re: [PATCH] powerpc: Don't add -mabi= flags when building with Clang
From: Segher Boessenkool @ 2019-08-19 9:19 UTC (permalink / raw)
To: Nathan Chancellor
Cc: linux-kernel, clang-built-linux, Paul Mackerras, linuxppc-dev
In-Reply-To: <20190818191321.58185-1-natechancellor@gmail.com>
On Sun, Aug 18, 2019 at 12:13:21PM -0700, Nathan Chancellor wrote:
> When building pseries_defconfig, building vdso32 errors out:
>
> error: unknown target ABI 'elfv1'
>
> Commit 4dc831aa8813 ("powerpc: Fix compiling a BE kernel with a
> powerpc64le toolchain") added these flags to fix building GCC but
> clang is multitargeted and does not need these flags. The ABI is
> properly set based on the target triple, which is derived from
> CROSS_COMPILE.
You mean that LLVM does not *allow* you to select a different ABI, or
different ABI options, you always have to use the default. (Everything
else you say is true for GCC as well).
(-mabi= does not set a "target ABI", fwiw, it is more subtle; please see
the documentation. Unless LLVM is incompatible in that respect as well?)
Segher
^ permalink raw reply
* Re: [PATCH v2 3/3] arm: Add support for function error injection
From: Leo Yan @ 2019-08-19 9:18 UTC (permalink / raw)
To: Russell King, Oleg Nesterov, Catalin Marinas, Will Deacon,
Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin,
x86, Arnd Bergmann, Alexei Starovoitov, Daniel Borkmann,
Martin KaFai Lau, Song Liu, Yonghong Song, Naveen N. Rao,
linux-arm-kernel, linux-kernel, linuxppc-dev, linux-arch, netdev,
bpf, clang-built-linux, Masami Hiramatsu
In-Reply-To: <20190806100015.11256-4-leo.yan@linaro.org>
Hi Russell,
On Tue, Aug 06, 2019 at 06:00:15PM +0800, Leo Yan wrote:
> This patch implements arm specific functions regs_set_return_value() and
> override_function_with_return() to support function error injection.
>
> In the exception flow, it updates pt_regs::ARM_pc with pt_regs::ARM_lr
> so can override the probed function return.
Gentle ping ... Could you review this patch?
Thanks,
Leo.
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
> arch/arm/Kconfig | 1 +
> arch/arm/include/asm/ptrace.h | 5 +++++
> arch/arm/lib/Makefile | 2 ++
> arch/arm/lib/error-inject.c | 19 +++++++++++++++++++
> 4 files changed, 27 insertions(+)
> create mode 100644 arch/arm/lib/error-inject.c
>
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 33b00579beff..2d3d44a037f6 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -77,6 +77,7 @@ config ARM
> select HAVE_EXIT_THREAD
> select HAVE_FAST_GUP if ARM_LPAE
> select HAVE_FTRACE_MCOUNT_RECORD if !XIP_KERNEL
> + select HAVE_FUNCTION_ERROR_INJECTION if !THUMB2_KERNEL
> select HAVE_FUNCTION_GRAPH_TRACER if !THUMB2_KERNEL && !CC_IS_CLANG
> select HAVE_FUNCTION_TRACER if !XIP_KERNEL
> select HAVE_GCC_PLUGINS
> diff --git a/arch/arm/include/asm/ptrace.h b/arch/arm/include/asm/ptrace.h
> index 91d6b7856be4..3b41f37b361a 100644
> --- a/arch/arm/include/asm/ptrace.h
> +++ b/arch/arm/include/asm/ptrace.h
> @@ -89,6 +89,11 @@ static inline long regs_return_value(struct pt_regs *regs)
> return regs->ARM_r0;
> }
>
> +static inline void regs_set_return_value(struct pt_regs *regs, unsigned long rc)
> +{
> + regs->ARM_r0 = rc;
> +}
> +
> #define instruction_pointer(regs) (regs)->ARM_pc
>
> #ifdef CONFIG_THUMB2_KERNEL
> diff --git a/arch/arm/lib/Makefile b/arch/arm/lib/Makefile
> index b25c54585048..8f56484a7156 100644
> --- a/arch/arm/lib/Makefile
> +++ b/arch/arm/lib/Makefile
> @@ -42,3 +42,5 @@ ifeq ($(CONFIG_KERNEL_MODE_NEON),y)
> CFLAGS_xor-neon.o += $(NEON_FLAGS)
> obj-$(CONFIG_XOR_BLOCKS) += xor-neon.o
> endif
> +
> +obj-$(CONFIG_FUNCTION_ERROR_INJECTION) += error-inject.o
> diff --git a/arch/arm/lib/error-inject.c b/arch/arm/lib/error-inject.c
> new file mode 100644
> index 000000000000..2d696dc94893
> --- /dev/null
> +++ b/arch/arm/lib/error-inject.c
> @@ -0,0 +1,19 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/error-injection.h>
> +#include <linux/kprobes.h>
> +
> +void override_function_with_return(struct pt_regs *regs)
> +{
> + /*
> + * 'regs' represents the state on entry of a predefined function in
> + * the kernel/module and which is captured on a kprobe.
> + *
> + * 'regs->ARM_lr' contains the the link register for the probed
> + * function, when kprobe returns back from exception it will override
> + * the end of probed function and directly return to the predefined
> + * function's caller.
> + */
> + instruction_pointer_set(regs, regs->ARM_lr);
> +}
> +NOKPROBE_SYMBOL(override_function_with_return);
> --
> 2.17.1
>
^ permalink raw reply
* Re: [PATCH v5 1/3] PM: wakeup: Add routine to help fetch wakeup source object.
From: Rafael J. Wysocki @ 2019-08-19 8:54 UTC (permalink / raw)
To: Ran Wang
Cc: Mark Rutland, Biwen Li, Len Brown, Rafael J. Wysocki,
Greg Kroah-Hartman, linux-pm@vger.kernel.org,
linux-kernel@vger.kernel.org, Leo Li, devicetree@vger.kernel.org,
Rob Herring, Pavel Machek, linuxppc-dev@lists.ozlabs.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <DB8PR04MB6826475ACA623AE6D63617D7F1A80@DB8PR04MB6826.eurprd04.prod.outlook.com>
On Monday, August 19, 2019 10:33:25 AM CEST Ran Wang wrote:
> Hi Rafael,
>
> On Monday, August 19, 2019 16:20, Rafael J. Wysocki wrote:
> >
> > On Mon, Aug 19, 2019 at 10:15 AM Ran Wang <ran.wang_1@nxp.com> wrote:
> > >
> > > Hi Rafael,
> > >
> > > On Monday, August 05, 2019 17:59, Rafael J. Wysocki wrote:
> > > >
> > > > On Wednesday, July 24, 2019 9:47:20 AM CEST Ran Wang wrote:
> > > > > Some user might want to go through all registered wakeup sources
> > > > > and doing things accordingly. For example, SoC PM driver might
> > > > > need to do HW programming to prevent powering down specific IP
> > > > > which wakeup source depending on. So add this API to help walk
> > > > > through all registered wakeup source objects on that list and return them
> > one by one.
> > > > >
> > > > > Signed-off-by: Ran Wang <ran.wang_1@nxp.com>
> > > > > ---
> > > > > Change in v5:
> > > > > - Update commit message, add decription of walk through all wakeup
> > > > > source objects.
> > > > > - Add SCU protection in function wakeup_source_get_next().
> > > > > - Rename wakeup_source member 'attached_dev' to 'dev' and move
> > > > > it
> > > > up
> > > > > (before wakeirq).
> > > > >
> > > > > Change in v4:
> > > > > - None.
> > > > >
> > > > > Change in v3:
> > > > > - Adjust indentation of *attached_dev;.
> > > > >
> > > > > Change in v2:
> > > > > - None.
> > > > >
> > > > > drivers/base/power/wakeup.c | 24 ++++++++++++++++++++++++
> > > > > include/linux/pm_wakeup.h | 3 +++
> > > > > 2 files changed, 27 insertions(+)
> > > > >
> > > > > diff --git a/drivers/base/power/wakeup.c
> > > > > b/drivers/base/power/wakeup.c index ee31d4f..2fba891 100644
> > > > > --- a/drivers/base/power/wakeup.c
> > > > > +++ b/drivers/base/power/wakeup.c
> > > > > @@ -14,6 +14,7 @@
> > > > > #include <linux/suspend.h>
> > > > > #include <linux/seq_file.h>
> > > > > #include <linux/debugfs.h>
> > > > > +#include <linux/of_device.h>
> > > > > #include <linux/pm_wakeirq.h>
> > > > > #include <trace/events/power.h>
> > > > >
> > > > > @@ -226,6 +227,28 @@ void wakeup_source_unregister(struct
> > > > wakeup_source *ws)
> > > > > }
> > > > > }
> > > > > EXPORT_SYMBOL_GPL(wakeup_source_unregister);
> > > > > +/**
> > > > > + * wakeup_source_get_next - Get next wakeup source from the list
> > > > > + * @ws: Previous wakeup source object, null means caller want first one.
> > > > > + */
> > > > > +struct wakeup_source *wakeup_source_get_next(struct wakeup_source
> > > > > +*ws) {
> > > > > + struct list_head *ws_head = &wakeup_sources;
> > > > > + struct wakeup_source *next_ws = NULL;
> > > > > + int idx;
> > > > > +
> > > > > + idx = srcu_read_lock(&wakeup_srcu);
> > > > > + if (ws)
> > > > > + next_ws = list_next_or_null_rcu(ws_head, &ws->entry,
> > > > > + struct wakeup_source, entry);
> > > > > + else
> > > > > + next_ws = list_entry_rcu(ws_head->next,
> > > > > + struct wakeup_source, entry);
> > > > > + srcu_read_unlock(&wakeup_srcu, idx);
> > > > > +
> > > >
> > > > This is incorrect.
> > > >
> > > > The SRCU cannot be unlocked until the caller of this is done with
> > > > the object returned by it, or that object can be freed while it is still being
> > accessed.
> > >
> > > Thanks for the comment. Looks like I was not fully understanding your
> > > point on
> > > v4 discussion. So I will implement 3 APIs by referring
> > > wakeup_sources_stats_seq_start/next/stop()
> > >
> > > > Besides, this patch conflicts with some general wakeup sources
> > > > changes in the works, so it needs to be deferred and rebased on top of those
> > changes.
> > >
> > > Could you please tell me which is the right code base I should developing on?
> > > I just tried applying v5 patch on latest
> > > git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git branch master
> > (d1abaeb Linux 5.3-rc5) and no conflict encountered.
> >
> > It is better to use the most recent -rc from Linus (5.3-rc5 as of
> > today) as the base unless your patches depend on some changes that are not in
> > there.
>
> OK, So I need to implement on latest git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git branch master, am I right?
>
> However, I just checked v5.3-rc5 code and found it has the same HEAD (d1abaeb Linux 5.3-rc5
> on which I did not observe v5 patch apply conflict, did I miss something? Thanks.
The conflict I mentioned earlier was with another patch series in the works
which is not in 5.3-rc5. However, there are problems with that series and it
is not linux-next now even, so please just base your series on top of -rc5.
^ permalink raw reply
* RE: [PATCH v5 1/3] PM: wakeup: Add routine to help fetch wakeup source object.
From: Ran Wang @ 2019-08-19 8:33 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Mark Rutland, Biwen Li, Len Brown, Greg Kroah-Hartman,
linux-pm@vger.kernel.org, Rafael J. Wysocki,
linux-kernel@vger.kernel.org, Leo Li, devicetree@vger.kernel.org,
Rob Herring, Pavel Machek, linuxppc-dev@lists.ozlabs.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <CAJZ5v0i58p-GsswzMGEsgD5OXDqJ_G5zXDYf8jq8JJbWxZv+nQ@mail.gmail.com>
Hi Rafael,
On Monday, August 19, 2019 16:20, Rafael J. Wysocki wrote:
>
> On Mon, Aug 19, 2019 at 10:15 AM Ran Wang <ran.wang_1@nxp.com> wrote:
> >
> > Hi Rafael,
> >
> > On Monday, August 05, 2019 17:59, Rafael J. Wysocki wrote:
> > >
> > > On Wednesday, July 24, 2019 9:47:20 AM CEST Ran Wang wrote:
> > > > Some user might want to go through all registered wakeup sources
> > > > and doing things accordingly. For example, SoC PM driver might
> > > > need to do HW programming to prevent powering down specific IP
> > > > which wakeup source depending on. So add this API to help walk
> > > > through all registered wakeup source objects on that list and return them
> one by one.
> > > >
> > > > Signed-off-by: Ran Wang <ran.wang_1@nxp.com>
> > > > ---
> > > > Change in v5:
> > > > - Update commit message, add decription of walk through all wakeup
> > > > source objects.
> > > > - Add SCU protection in function wakeup_source_get_next().
> > > > - Rename wakeup_source member 'attached_dev' to 'dev' and move
> > > > it
> > > up
> > > > (before wakeirq).
> > > >
> > > > Change in v4:
> > > > - None.
> > > >
> > > > Change in v3:
> > > > - Adjust indentation of *attached_dev;.
> > > >
> > > > Change in v2:
> > > > - None.
> > > >
> > > > drivers/base/power/wakeup.c | 24 ++++++++++++++++++++++++
> > > > include/linux/pm_wakeup.h | 3 +++
> > > > 2 files changed, 27 insertions(+)
> > > >
> > > > diff --git a/drivers/base/power/wakeup.c
> > > > b/drivers/base/power/wakeup.c index ee31d4f..2fba891 100644
> > > > --- a/drivers/base/power/wakeup.c
> > > > +++ b/drivers/base/power/wakeup.c
> > > > @@ -14,6 +14,7 @@
> > > > #include <linux/suspend.h>
> > > > #include <linux/seq_file.h>
> > > > #include <linux/debugfs.h>
> > > > +#include <linux/of_device.h>
> > > > #include <linux/pm_wakeirq.h>
> > > > #include <trace/events/power.h>
> > > >
> > > > @@ -226,6 +227,28 @@ void wakeup_source_unregister(struct
> > > wakeup_source *ws)
> > > > }
> > > > }
> > > > EXPORT_SYMBOL_GPL(wakeup_source_unregister);
> > > > +/**
> > > > + * wakeup_source_get_next - Get next wakeup source from the list
> > > > + * @ws: Previous wakeup source object, null means caller want first one.
> > > > + */
> > > > +struct wakeup_source *wakeup_source_get_next(struct wakeup_source
> > > > +*ws) {
> > > > + struct list_head *ws_head = &wakeup_sources;
> > > > + struct wakeup_source *next_ws = NULL;
> > > > + int idx;
> > > > +
> > > > + idx = srcu_read_lock(&wakeup_srcu);
> > > > + if (ws)
> > > > + next_ws = list_next_or_null_rcu(ws_head, &ws->entry,
> > > > + struct wakeup_source, entry);
> > > > + else
> > > > + next_ws = list_entry_rcu(ws_head->next,
> > > > + struct wakeup_source, entry);
> > > > + srcu_read_unlock(&wakeup_srcu, idx);
> > > > +
> > >
> > > This is incorrect.
> > >
> > > The SRCU cannot be unlocked until the caller of this is done with
> > > the object returned by it, or that object can be freed while it is still being
> accessed.
> >
> > Thanks for the comment. Looks like I was not fully understanding your
> > point on
> > v4 discussion. So I will implement 3 APIs by referring
> > wakeup_sources_stats_seq_start/next/stop()
> >
> > > Besides, this patch conflicts with some general wakeup sources
> > > changes in the works, so it needs to be deferred and rebased on top of those
> changes.
> >
> > Could you please tell me which is the right code base I should developing on?
> > I just tried applying v5 patch on latest
> > git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git branch master
> (d1abaeb Linux 5.3-rc5) and no conflict encountered.
>
> It is better to use the most recent -rc from Linus (5.3-rc5 as of
> today) as the base unless your patches depend on some changes that are not in
> there.
OK, So I need to implement on latest git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git branch master, am I right?
However, I just checked v5.3-rc5 code and found it has the same HEAD (d1abaeb Linux 5.3-rc5
on which I did not observe v5 patch apply conflict, did I miss something? Thanks.
Regards,
Ran
^ permalink raw reply
* Re: [PATCH v5 1/3] PM: wakeup: Add routine to help fetch wakeup source object.
From: Rafael J. Wysocki @ 2019-08-19 8:20 UTC (permalink / raw)
To: Ran Wang
Cc: Mark Rutland, Biwen Li, Len Brown, Greg Kroah-Hartman,
linux-pm@vger.kernel.org, Rafael J. Wysocki,
linux-kernel@vger.kernel.org, Leo Li, devicetree@vger.kernel.org,
Rob Herring, Pavel Machek, linuxppc-dev@lists.ozlabs.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <DB8PR04MB682632A586827032F8D6EA2DF1A80@DB8PR04MB6826.eurprd04.prod.outlook.com>
On Mon, Aug 19, 2019 at 10:15 AM Ran Wang <ran.wang_1@nxp.com> wrote:
>
> Hi Rafael,
>
> On Monday, August 05, 2019 17:59, Rafael J. Wysocki wrote:
> >
> > On Wednesday, July 24, 2019 9:47:20 AM CEST Ran Wang wrote:
> > > Some user might want to go through all registered wakeup sources and
> > > doing things accordingly. For example, SoC PM driver might need to do
> > > HW programming to prevent powering down specific IP which wakeup
> > > source depending on. So add this API to help walk through all
> > > registered wakeup source objects on that list and return them one by one.
> > >
> > > Signed-off-by: Ran Wang <ran.wang_1@nxp.com>
> > > ---
> > > Change in v5:
> > > - Update commit message, add decription of walk through all wakeup
> > > source objects.
> > > - Add SCU protection in function wakeup_source_get_next().
> > > - Rename wakeup_source member 'attached_dev' to 'dev' and move it
> > up
> > > (before wakeirq).
> > >
> > > Change in v4:
> > > - None.
> > >
> > > Change in v3:
> > > - Adjust indentation of *attached_dev;.
> > >
> > > Change in v2:
> > > - None.
> > >
> > > drivers/base/power/wakeup.c | 24 ++++++++++++++++++++++++
> > > include/linux/pm_wakeup.h | 3 +++
> > > 2 files changed, 27 insertions(+)
> > >
> > > diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> > > index ee31d4f..2fba891 100644
> > > --- a/drivers/base/power/wakeup.c
> > > +++ b/drivers/base/power/wakeup.c
> > > @@ -14,6 +14,7 @@
> > > #include <linux/suspend.h>
> > > #include <linux/seq_file.h>
> > > #include <linux/debugfs.h>
> > > +#include <linux/of_device.h>
> > > #include <linux/pm_wakeirq.h>
> > > #include <trace/events/power.h>
> > >
> > > @@ -226,6 +227,28 @@ void wakeup_source_unregister(struct
> > wakeup_source *ws)
> > > }
> > > }
> > > EXPORT_SYMBOL_GPL(wakeup_source_unregister);
> > > +/**
> > > + * wakeup_source_get_next - Get next wakeup source from the list
> > > + * @ws: Previous wakeup source object, null means caller want first one.
> > > + */
> > > +struct wakeup_source *wakeup_source_get_next(struct wakeup_source
> > > +*ws) {
> > > + struct list_head *ws_head = &wakeup_sources;
> > > + struct wakeup_source *next_ws = NULL;
> > > + int idx;
> > > +
> > > + idx = srcu_read_lock(&wakeup_srcu);
> > > + if (ws)
> > > + next_ws = list_next_or_null_rcu(ws_head, &ws->entry,
> > > + struct wakeup_source, entry);
> > > + else
> > > + next_ws = list_entry_rcu(ws_head->next,
> > > + struct wakeup_source, entry);
> > > + srcu_read_unlock(&wakeup_srcu, idx);
> > > +
> >
> > This is incorrect.
> >
> > The SRCU cannot be unlocked until the caller of this is done with the object
> > returned by it, or that object can be freed while it is still being accessed.
>
> Thanks for the comment. Looks like I was not fully understanding your point on
> v4 discussion. So I will implement 3 APIs by referring wakeup_sources_stats_seq_start/next/stop()
>
> > Besides, this patch conflicts with some general wakeup sources changes in the
> > works, so it needs to be deferred and rebased on top of those changes.
>
> Could you please tell me which is the right code base I should developing on?
> I just tried applying v5 patch on latest git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git branch master (d1abaeb Linux 5.3-rc5)
> and no conflict encountered.
It is better to use the most recent -rc from Linus (5.3-rc5 as of
today) as the base unless your patches depend on some changes that are
not in there.
^ permalink raw reply
* WARN_ON(1) generates ugly code since commit 6b15f678fb7d
From: Christophe Leroy @ 2019-08-19 8:18 UTC (permalink / raw)
To: Drew Davenport
Cc: Andrew Morton, linuxppc-dev@lists.ozlabs.org,
linux-kernel@vger.kernel.org, Kees Cook
Hi Drew,
I recently noticed gcc suddenly generating ugly code for WARN_ON(1).
It looks like commit 6b15f678fb7d ("include/asm-generic/bug.h: fix "cut
here" for WARN_ON for __WARN_TAINT architectures") is the culprit.
unsigned long test_mul1(unsigned long a, unsigned long b)
{
unsigned long long r = (unsigned long long)a * (unsigned long long)b;
if (r > 0xffffffff)
WARN_ON(1);
return r;
}
Before that patch, I was getting the following code:
00000008 <test_mul1>:
8: 7d 23 20 16 mulhwu r9,r3,r4
c: 7c 63 21 d6 mullw r3,r3,r4
10: 2f 89 00 00 cmpwi cr7,r9,0
14: 4d 9e 00 20 beqlr cr7
18: 0f e0 00 00 twui r0,0
1c: 4e 80 00 20 blr
Now I get:
0000002c <test_mul1>:
2c: 7d 23 20 16 mulhwu r9,r3,r4
30: 94 21 ff f0 stwu r1,-16(r1)
34: 7c 08 02 a6 mflr r0
38: 93 e1 00 0c stw r31,12(r1)
3c: 90 01 00 14 stw r0,20(r1)
40: 7f e3 21 d6 mullw r31,r3,r4
44: 2f 89 00 00 cmpwi cr7,r9,0
48: 40 9e 00 1c bne cr7,64 <test_mul1+0x38>
4c: 80 01 00 14 lwz r0,20(r1)
50: 7f e3 fb 78 mr r3,r31
54: 83 e1 00 0c lwz r31,12(r1)
58: 7c 08 03 a6 mtlr r0
5c: 38 21 00 10 addi r1,r1,16
60: 4e 80 00 20 blr
64: 3c 60 00 00 lis r3,0
66: R_PPC_ADDR16_HA .rodata.str1.4
68: 38 63 00 00 addi r3,r3,0
6a: R_PPC_ADDR16_LO .rodata.str1.4
6c: 48 00 00 01 bl 6c <test_mul1+0x40>
6c: R_PPC_REL24 printk
70: 0f e0 00 00 twui r0,0
74: 4b ff ff d8 b 4c <test_mul1+0x20>
As you can see, a call to printk() is added, which means setting up a
stack frame, saving volatile registers, etc ...
That's all the things we want to avoid when using WARN_ON().
And digging a bit more, I see that you are only adding this 'cut here'
to calls like WARN_ON(1), ie where the condition is a constant.
For calls where the condition is not a constant, there is no change and
no 'cut here' line added:
unsigned long test_mul2(unsigned long a, unsigned long b)
{
unsigned long long r = (unsigned long long)a * (unsigned long long)b;
WARN_ON(r > 0xffffffff);
return r;
}
Before and after your patch, the code is clean and no call to add any
'cut here' line.
00000078 <test_mul2>:
78: 7d 43 20 16 mulhwu r10,r3,r4
7c: 7c 63 21 d6 mullw r3,r3,r4
80: 31 2a ff ff addic r9,r10,-1
84: 7d 29 51 10 subfe r9,r9,r10
88: 0f 09 00 00 twnei r9,0
8c: 4e 80 00 20 blr
Was it your intention to modify the behaviour and kill the lightweight
implementations of WARN_ON() ?
Looking into arch/powerpc/include/bug.h, I see that when the condition
is constant, WARN_ON() uses __WARN(), which itself calls __WARN_FLAGS()
with relevant flags.
In the old days, __WARN() was implemented in arch/powerpc/include/bug.h
Commit b2be05273a17 ("panic: Allow warnings to set different taint
flags") replaced __WARN() by __WARN_TAINT() and added a generic
definition of __WARN()
In the begining I thought the __WARN() call in
arch/powerpc/include/bug.h was forgotten, but looking into the commit in
full, it looks like it was intentional to make __WARN() generic and have
arches use it.
Then commit 19d436268dde ("debug: Add _ONCE() logic to report_bug()")
replaced __WARN_TAINT() by __WARN_FLAGS().
So by changing the generic __WARN() you are impacting all users include
those using 'trap' like instruction in order to avoid function calls.
What is to be done for getting back a clean code which doesn't call
printk() on the hot path ?
Thanks,
Christophe
^ permalink raw reply
* RE: [PATCH v5 1/3] PM: wakeup: Add routine to help fetch wakeup source object.
From: Ran Wang @ 2019-08-19 8:14 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Mark Rutland, Biwen Li, Len Brown, Greg Kroah-Hartman,
linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org, Leo Li,
devicetree@vger.kernel.org, Rob Herring, Pavel Machek,
linuxppc-dev@lists.ozlabs.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <4158639.B12JYek7R7@kreacher>
Hi Rafael,
On Monday, August 05, 2019 17:59, Rafael J. Wysocki wrote:
>
> On Wednesday, July 24, 2019 9:47:20 AM CEST Ran Wang wrote:
> > Some user might want to go through all registered wakeup sources and
> > doing things accordingly. For example, SoC PM driver might need to do
> > HW programming to prevent powering down specific IP which wakeup
> > source depending on. So add this API to help walk through all
> > registered wakeup source objects on that list and return them one by one.
> >
> > Signed-off-by: Ran Wang <ran.wang_1@nxp.com>
> > ---
> > Change in v5:
> > - Update commit message, add decription of walk through all wakeup
> > source objects.
> > - Add SCU protection in function wakeup_source_get_next().
> > - Rename wakeup_source member 'attached_dev' to 'dev' and move it
> up
> > (before wakeirq).
> >
> > Change in v4:
> > - None.
> >
> > Change in v3:
> > - Adjust indentation of *attached_dev;.
> >
> > Change in v2:
> > - None.
> >
> > drivers/base/power/wakeup.c | 24 ++++++++++++++++++++++++
> > include/linux/pm_wakeup.h | 3 +++
> > 2 files changed, 27 insertions(+)
> >
> > diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> > index ee31d4f..2fba891 100644
> > --- a/drivers/base/power/wakeup.c
> > +++ b/drivers/base/power/wakeup.c
> > @@ -14,6 +14,7 @@
> > #include <linux/suspend.h>
> > #include <linux/seq_file.h>
> > #include <linux/debugfs.h>
> > +#include <linux/of_device.h>
> > #include <linux/pm_wakeirq.h>
> > #include <trace/events/power.h>
> >
> > @@ -226,6 +227,28 @@ void wakeup_source_unregister(struct
> wakeup_source *ws)
> > }
> > }
> > EXPORT_SYMBOL_GPL(wakeup_source_unregister);
> > +/**
> > + * wakeup_source_get_next - Get next wakeup source from the list
> > + * @ws: Previous wakeup source object, null means caller want first one.
> > + */
> > +struct wakeup_source *wakeup_source_get_next(struct wakeup_source
> > +*ws) {
> > + struct list_head *ws_head = &wakeup_sources;
> > + struct wakeup_source *next_ws = NULL;
> > + int idx;
> > +
> > + idx = srcu_read_lock(&wakeup_srcu);
> > + if (ws)
> > + next_ws = list_next_or_null_rcu(ws_head, &ws->entry,
> > + struct wakeup_source, entry);
> > + else
> > + next_ws = list_entry_rcu(ws_head->next,
> > + struct wakeup_source, entry);
> > + srcu_read_unlock(&wakeup_srcu, idx);
> > +
>
> This is incorrect.
>
> The SRCU cannot be unlocked until the caller of this is done with the object
> returned by it, or that object can be freed while it is still being accessed.
Thanks for the comment. Looks like I was not fully understanding your point on
v4 discussion. So I will implement 3 APIs by referring wakeup_sources_stats_seq_start/next/stop()
> Besides, this patch conflicts with some general wakeup sources changes in the
> works, so it needs to be deferred and rebased on top of those changes.
Could you please tell me which is the right code base I should developing on?
I just tried applying v5 patch on latest git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git branch master (d1abaeb Linux 5.3-rc5)
and no conflict encountered.
Thanks & Regards,
Ran
^ permalink raw reply
* Re: [PATCH] powerpc: optimise WARN_ON()
From: Segher Boessenkool @ 2019-08-19 7:57 UTC (permalink / raw)
To: Christophe Leroy; +Cc: linuxppc-dev, Paul Mackerras, linux-kernel
In-Reply-To: <f1c0d9d9-d978-794f-82ce-494d2e52d743@c-s.fr>
On Mon, Aug 19, 2019 at 07:40:42AM +0200, Christophe Leroy wrote:
> Le 18/08/2019 à 14:01, Segher Boessenkool a écrit :
> >On Sat, Aug 17, 2019 at 09:04:42AM +0000, Christophe Leroy wrote:
> >>Unlike BUG_ON(x), WARN_ON(x) uses !!(x) as the trigger
> >>of the t(d/w)nei instruction instead of using directly the
> >>value of x.
> >>
> >>This leads to GCC adding unnecessary pair of addic/subfe.
> >
> >And it has to, it is passed as an "r" to an asm, GCC has to put the "!!"
> >value into a register.
> >
> >>By using (x) instead of !!(x) like BUG_ON() does, the additional
> >>instructions go away:
> >
> >But is it correct? What happens if you pass an int to WARN_ON, on a
> >64-bit kernel?
>
> On a 64-bit kernel, an int is still in a 64-bit register, so there would
> be no problem with tdnei, would it ? an int 0 is the same as an long 0,
> right ?
The top 32 bits of a 64-bit register holding an int are undefined. Take
as example
int x = 42;
x = ~x;
which may put ffff_ffff_ffff_ffd5 into the reg, not 0000_0000_ffff_ffd5
as you might expect or want. For tw instructions this makes no difference
(they only look at the low 32 bits anyway); for td insns, it does.
> It is on 32-bit kernel that I see a problem, if one passes a long long
> to WARN_ON(), the forced cast to long will just drop the upper size of
> it. So as of today, BUG_ON() is buggy for that.
Sure, it isn't defined what types you can pass to that macro. Another
thing that makes inline functions much saner to use.
> >(You might want to have 64-bit generate either tw or td. But, with
> >your __builtin_trap patch, all that will be automatic).
>
> Yes I'll discard this patch and focus on the __builtin_trap() one which
> should solve most issues.
But see my comment there about the compiler knowing all code after an
unconditional trap is dead.
Segher
^ permalink raw reply
* Re: [PATCH] powerpc: Don't add -mabi= flags when building with Clang
From: Daniel Axtens @ 2019-08-19 7:25 UTC (permalink / raw)
To: Nathan Chancellor, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman
Cc: clang-built-linux, Nathan Chancellor, linuxppc-dev, linux-kernel
In-Reply-To: <20190818191321.58185-1-natechancellor@gmail.com>
Hi Nathan,
> When building pseries_defconfig, building vdso32 errors out:
>
> error: unknown target ABI 'elfv1'
>
> Commit 4dc831aa8813 ("powerpc: Fix compiling a BE kernel with a
> powerpc64le toolchain") added these flags to fix building GCC but
> clang is multitargeted and does not need these flags. The ABI is
> properly set based on the target triple, which is derived from
> CROSS_COMPILE.
>
> https://github.com/llvm/llvm-project/blob/llvmorg-9.0.0-rc2/clang/lib/Driver/ToolChains/Clang.cpp#L1782-L1804
>
> -mcall-aixdesc is not an implemented flag in clang so it can be
> safely excluded as well, see commit 238abecde8ad ("powerpc: Don't
> use gcc specific options on clang").
>
This all looks good to me, thanks for picking it up, and sorry I hadn't
got around to it!
The makefile is a bit messy and there are a few ways it could probably
be reorganised to reduce ifdefs. But I don't think this is the right
place to do that. With that in mind,
Reviewed-by: Daniel Axtens <dja@axtens.net>
Regards,
Daniel
> pseries_defconfig successfully builds after this patch and
> powernv_defconfig and ppc44x_defconfig don't regress.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/240
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
> arch/powerpc/Makefile | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
> index c345b79414a9..971b04bc753d 100644
> --- a/arch/powerpc/Makefile
> +++ b/arch/powerpc/Makefile
> @@ -93,11 +93,13 @@ MULTIPLEWORD := -mmultiple
> endif
>
> ifdef CONFIG_PPC64
> +ifndef CONFIG_CC_IS_CLANG
> cflags-$(CONFIG_CPU_BIG_ENDIAN) += $(call cc-option,-mabi=elfv1)
> cflags-$(CONFIG_CPU_BIG_ENDIAN) += $(call cc-option,-mcall-aixdesc)
> aflags-$(CONFIG_CPU_BIG_ENDIAN) += $(call cc-option,-mabi=elfv1)
> aflags-$(CONFIG_CPU_LITTLE_ENDIAN) += -mabi=elfv2
> endif
> +endif
>
> ifndef CONFIG_CC_IS_CLANG
> cflags-$(CONFIG_CPU_LITTLE_ENDIAN) += -mno-strict-align
> @@ -144,6 +146,7 @@ endif
> endif
>
> CFLAGS-$(CONFIG_PPC64) := $(call cc-option,-mtraceback=no)
> +ifndef CONFIG_CC_IS_CLANG
> ifdef CONFIG_CPU_LITTLE_ENDIAN
> CFLAGS-$(CONFIG_PPC64) += $(call cc-option,-mabi=elfv2,$(call cc-option,-mcall-aixdesc))
> AFLAGS-$(CONFIG_PPC64) += $(call cc-option,-mabi=elfv2)
> @@ -152,6 +155,7 @@ CFLAGS-$(CONFIG_PPC64) += $(call cc-option,-mabi=elfv1)
> CFLAGS-$(CONFIG_PPC64) += $(call cc-option,-mcall-aixdesc)
> AFLAGS-$(CONFIG_PPC64) += $(call cc-option,-mabi=elfv1)
> endif
> +endif
> CFLAGS-$(CONFIG_PPC64) += $(call cc-option,-mcmodel=medium,$(call cc-option,-mminimal-toc))
> CFLAGS-$(CONFIG_PPC64) += $(call cc-option,-mno-pointers-to-nested-functions)
>
> --
> 2.23.0
^ 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