LinuxPPC-Dev Archive on lore.kernel.org
 help / Atom feed
* [PATCH 1/2] powerpc/64s: Work around spurious warning on old gccs with -fsanitize-coverage
@ 2019-02-07  5:33 Andrew Donnellan
  2019-02-07  5:33 ` [PATCH 2/2] powerpc: Enable kcov Andrew Donnellan
  2019-02-07  6:37 ` [PATCH 1/2] powerpc/64s: Work around spurious warning on old gccs with -fsanitize-coverage Segher Boessenkool
  0 siblings, 2 replies; 10+ messages in thread
From: Andrew Donnellan @ 2019-02-07  5:33 UTC (permalink / raw)
  To: linuxppc-dev; +Cc: syzkaller, dvyukov

Some older gccs (<GCC 7), when invoked with -fsanitize-coverage=trace-pc,
cause a spurious uninitialised variable warning in dt_cpu_ftrs.c:

  arch/powerpc/kernel/dt_cpu_ftrs.c: In function ‘cpufeatures_process_feature’:
  arch/powerpc/kernel/dt_cpu_ftrs.c:686:7: warning: ‘m’ may be used uninitialized in this function [-Wmaybe-uninitialized]
    if (m->cpu_ftr_bit_mask)

An upcoming patch will enable support for kcov, which requires
-fsanitize-coverage=trace-pc.

Work around this by explicitly initialising m to NULL.

Signed-off-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>
---
 arch/powerpc/kernel/dt_cpu_ftrs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c b/arch/powerpc/kernel/dt_cpu_ftrs.c
index 8be3721d9302..2192b2114513 100644
--- a/arch/powerpc/kernel/dt_cpu_ftrs.c
+++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
@@ -658,7 +658,7 @@ static void __init cpufeatures_setup_start(u32 isa)
 
 static bool __init cpufeatures_process_feature(struct dt_cpu_feature *f)
 {
-	const struct dt_cpu_feature_match *m;
+	const struct dt_cpu_feature_match *m = NULL;
 	bool known = false;
 	int i;
 
-- 
2.11.0


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 2/2] powerpc: Enable kcov
  2019-02-07  5:33 [PATCH 1/2] powerpc/64s: Work around spurious warning on old gccs with -fsanitize-coverage Andrew Donnellan
@ 2019-02-07  5:33 ` Andrew Donnellan
  2019-02-07  6:37 ` [PATCH 1/2] powerpc/64s: Work around spurious warning on old gccs with -fsanitize-coverage Segher Boessenkool
  1 sibling, 0 replies; 10+ messages in thread
From: Andrew Donnellan @ 2019-02-07  5:33 UTC (permalink / raw)
  To: linuxppc-dev; +Cc: syzkaller, dvyukov

kcov provides kernel coverage data that's useful for fuzzing tools like
syzkaller.

Wire up kcov support on powerpc. Disable kcov instrumentation on the same
files where we currently disable gcov and UBSan instrumentation.

Signed-off-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>
Acked-by: Dmitry Vyukov <dvyukov@google.com>
---
 arch/powerpc/Kconfig                | 1 +
 arch/powerpc/kernel/Makefile        | 7 ++++++-
 arch/powerpc/kernel/trace/Makefile  | 3 ++-
 arch/powerpc/kernel/vdso32/Makefile | 1 +
 arch/powerpc/kernel/vdso64/Makefile | 1 +
 arch/powerpc/xmon/Makefile          | 1 +
 6 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 2890d36eb531..d3698dae0e60 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -134,6 +134,7 @@ config PPC
 	select ARCH_HAS_ELF_RANDOMIZE
 	select ARCH_HAS_FORTIFY_SOURCE
 	select ARCH_HAS_GCOV_PROFILE_ALL
+	select ARCH_HAS_KCOV
 	select ARCH_HAS_PHYS_TO_DMA
 	select ARCH_HAS_PMEM_API                if PPC64
 	select ARCH_HAS_PTE_SPECIAL
diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
index cb7f0bb9ee71..961f44eabb65 100644
--- a/arch/powerpc/kernel/Makefile
+++ b/arch/powerpc/kernel/Makefile
@@ -142,16 +142,21 @@ endif
 obj-$(CONFIG_EPAPR_PARAVIRT)	+= epapr_paravirt.o epapr_hcalls.o
 obj-$(CONFIG_KVM_GUEST)		+= kvm.o kvm_emul.o
 
-# Disable GCOV & sanitizers in odd or sensitive code
+# Disable GCOV, KCOV & sanitizers in odd or sensitive code
 GCOV_PROFILE_prom_init.o := n
+KCOV_INSTRUMENT_prom_init.o := n
 UBSAN_SANITIZE_prom_init.o := n
 GCOV_PROFILE_machine_kexec_64.o := n
+KCOV_INSTRUMENT_machine_kexec_64.o := n
 UBSAN_SANITIZE_machine_kexec_64.o := n
 GCOV_PROFILE_machine_kexec_32.o := n
+KCOV_INSTRUMENT_machine_kexec_32.o := n
 UBSAN_SANITIZE_machine_kexec_32.o := n
 GCOV_PROFILE_kprobes.o := n
+KCOV_INSTRUMENT_kprobes.o := n
 UBSAN_SANITIZE_kprobes.o := n
 GCOV_PROFILE_kprobes-ftrace.o := n
+KCOV_INSTRUMENT_kprobes-ftrace.o := n
 UBSAN_SANITIZE_kprobes-ftrace.o := n
 UBSAN_SANITIZE_vdso.o := n
 
diff --git a/arch/powerpc/kernel/trace/Makefile b/arch/powerpc/kernel/trace/Makefile
index b1725ad3e13d..858503775c58 100644
--- a/arch/powerpc/kernel/trace/Makefile
+++ b/arch/powerpc/kernel/trace/Makefile
@@ -23,6 +23,7 @@ obj-$(CONFIG_TRACING)			+= trace_clock.o
 obj-$(CONFIG_PPC64)			+= $(obj64-y)
 obj-$(CONFIG_PPC32)			+= $(obj32-y)
 
-# Disable GCOV & sanitizers in odd or sensitive code
+# Disable GCOV, KCOV & sanitizers in odd or sensitive code
 GCOV_PROFILE_ftrace.o := n
+KCOV_INSTRUMENT_ftrace.o := n
 UBSAN_SANITIZE_ftrace.o := n
diff --git a/arch/powerpc/kernel/vdso32/Makefile b/arch/powerpc/kernel/vdso32/Makefile
index 50112d4473bb..ce199f6e4256 100644
--- a/arch/powerpc/kernel/vdso32/Makefile
+++ b/arch/powerpc/kernel/vdso32/Makefile
@@ -23,6 +23,7 @@ targets := $(obj-vdso32) vdso32.so vdso32.so.dbg
 obj-vdso32 := $(addprefix $(obj)/, $(obj-vdso32))
 
 GCOV_PROFILE := n
+KCOV_INSTRUMENT := n
 UBSAN_SANITIZE := n
 
 ccflags-y := -shared -fno-common -fno-builtin
diff --git a/arch/powerpc/kernel/vdso64/Makefile b/arch/powerpc/kernel/vdso64/Makefile
index 69cecb346269..28e7d112aa2f 100644
--- a/arch/powerpc/kernel/vdso64/Makefile
+++ b/arch/powerpc/kernel/vdso64/Makefile
@@ -9,6 +9,7 @@ targets := $(obj-vdso64) vdso64.so vdso64.so.dbg
 obj-vdso64 := $(addprefix $(obj)/, $(obj-vdso64))
 
 GCOV_PROFILE := n
+KCOV_INSTRUMENT := n
 UBSAN_SANITIZE := n
 
 ccflags-y := -shared -fno-common -fno-builtin
diff --git a/arch/powerpc/xmon/Makefile b/arch/powerpc/xmon/Makefile
index 878f9c1d3615..3050f9323254 100644
--- a/arch/powerpc/xmon/Makefile
+++ b/arch/powerpc/xmon/Makefile
@@ -5,6 +5,7 @@
 subdir-ccflags-y := $(call cc-disable-warning, builtin-requires-header)
 
 GCOV_PROFILE := n
+KCOV_INSTRUMENT := n
 UBSAN_SANITIZE := n
 
 # Disable ftrace for the entire directory
-- 
2.11.0


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] powerpc/64s: Work around spurious warning on old gccs with -fsanitize-coverage
  2019-02-07  5:33 [PATCH 1/2] powerpc/64s: Work around spurious warning on old gccs with -fsanitize-coverage Andrew Donnellan
  2019-02-07  5:33 ` [PATCH 2/2] powerpc: Enable kcov Andrew Donnellan
@ 2019-02-07  6:37 ` Segher Boessenkool
  2019-02-07  6:59   ` Andrew Donnellan
  1 sibling, 1 reply; 10+ messages in thread
From: Segher Boessenkool @ 2019-02-07  6:37 UTC (permalink / raw)
  To: Andrew Donnellan; +Cc: syzkaller, linuxppc-dev, dvyukov

On Thu, Feb 07, 2019 at 04:33:23PM +1100, Andrew Donnellan wrote:
> Some older gccs (<GCC 7), when invoked with -fsanitize-coverage=trace-pc,
> cause a spurious uninitialised variable warning in dt_cpu_ftrs.c:
> 
>   arch/powerpc/kernel/dt_cpu_ftrs.c: In function ‘cpufeatures_process_feature’:
>   arch/powerpc/kernel/dt_cpu_ftrs.c:686:7: warning: ‘m’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>     if (m->cpu_ftr_bit_mask)

It seems to me the warning is correct?  If enable_unknown is false and no
cpu_feature is found, it will in

	if (m->cpu_ftr_bit_mask)
		cur_cpu_spec->cpu_features |= m->cpu_ftr_bit_mask;

enable random features (whatever was last in the table), or indeed access
via NULL if the table is length 0?  So maybe this should be

	if (known && m->cpu_ftr_bit_mask)
		cur_cpu_spec->cpu_features |= m->cpu_ftr_bit_mask;

instead?  (The code would be much clearer if all the known vs. !known
codepath was fully separated here).


Segher

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] powerpc/64s: Work around spurious warning on old gccs with -fsanitize-coverage
  2019-02-07  6:37 ` [PATCH 1/2] powerpc/64s: Work around spurious warning on old gccs with -fsanitize-coverage Segher Boessenkool
@ 2019-02-07  6:59   ` Andrew Donnellan
  2019-02-07  7:49     ` Segher Boessenkool
  0 siblings, 1 reply; 10+ messages in thread
From: Andrew Donnellan @ 2019-02-07  6:59 UTC (permalink / raw)
  To: Segher Boessenkool; +Cc: syzkaller, linuxppc-dev, dvyukov

On 7/2/19 5:37 pm, Segher Boessenkool wrote:
> On Thu, Feb 07, 2019 at 04:33:23PM +1100, Andrew Donnellan wrote:
>> Some older gccs (<GCC 7), when invoked with -fsanitize-coverage=trace-pc,
>> cause a spurious uninitialised variable warning in dt_cpu_ftrs.c:
>>
>>    arch/powerpc/kernel/dt_cpu_ftrs.c: In function ‘cpufeatures_process_feature’:
>>    arch/powerpc/kernel/dt_cpu_ftrs.c:686:7: warning: ‘m’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>>      if (m->cpu_ftr_bit_mask)
> 
> It seems to me the warning is correct?  If enable_unknown is false and no
> cpu_feature is found, it will in
> 
> 	if (m->cpu_ftr_bit_mask)
> 		cur_cpu_spec->cpu_features |= m->cpu_ftr_bit_mask;
> 
> enable random features (whatever was last in the table), or indeed access
> via NULL if the table is length 0?  So maybe this should be
> 
> 	if (known && m->cpu_ftr_bit_mask)
> 		cur_cpu_spec->cpu_features |= m->cpu_ftr_bit_mask;
> 
> instead?  (The code would be much clearer if all the known vs. !known
> codepath was fully separated here).

The table is never length 0, it's a statically defined array.

> 
> 
> Segher
> 

-- 
Andrew Donnellan              OzLabs, ADL Canberra
andrew.donnellan@au1.ibm.com  IBM Australia Limited


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] powerpc/64s: Work around spurious warning on old gccs with -fsanitize-coverage
  2019-02-07  6:59   ` Andrew Donnellan
@ 2019-02-07  7:49     ` Segher Boessenkool
  2019-02-08  0:34       ` Andrew Donnellan
  0 siblings, 1 reply; 10+ messages in thread
From: Segher Boessenkool @ 2019-02-07  7:49 UTC (permalink / raw)
  To: Andrew Donnellan; +Cc: syzkaller, linuxppc-dev, dvyukov

On Thu, Feb 07, 2019 at 05:59:48PM +1100, Andrew Donnellan wrote:
> On 7/2/19 5:37 pm, Segher Boessenkool wrote:
> >On Thu, Feb 07, 2019 at 04:33:23PM +1100, Andrew Donnellan wrote:
> >>Some older gccs (<GCC 7), when invoked with -fsanitize-coverage=trace-pc,
> >>cause a spurious uninitialised variable warning in dt_cpu_ftrs.c:
> >>
> >>   arch/powerpc/kernel/dt_cpu_ftrs.c: In function 
> >>   ‘cpufeatures_process_feature’:
> >>   arch/powerpc/kernel/dt_cpu_ftrs.c:686:7: warning: ‘m’ may be used 
> >>   uninitialized in this function [-Wmaybe-uninitialized]
> >>     if (m->cpu_ftr_bit_mask)
> >
> >It seems to me the warning is correct?  If enable_unknown is false and no
> >cpu_feature is found, it will in
> >
> >	if (m->cpu_ftr_bit_mask)
> >		cur_cpu_spec->cpu_features |= m->cpu_ftr_bit_mask;
> >
> >enable random features (whatever was last in the table), or indeed access
> >via NULL if the table is length 0?  So maybe this should be
> >
> >	if (known && m->cpu_ftr_bit_mask)
> >		cur_cpu_spec->cpu_features |= m->cpu_ftr_bit_mask;
> >
> >instead?  (The code would be much clearer if all the known vs. !known
> >codepath was fully separated here).
> 
> The table is never length 0, it's a statically defined array.

Sure, and presumably that is why newer GCC doesn't warn.  But what about
the other point?  Is this code ever correct?  Enabling random features
(in cur_cpu_spec->cpu_features) when the name isn't found seems wrong.


Segher

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] powerpc/64s: Work around spurious warning on old gccs with -fsanitize-coverage
  2019-02-07  7:49     ` Segher Boessenkool
@ 2019-02-08  0:34       ` Andrew Donnellan
  2019-02-08  3:02         ` Michael Ellerman
  0 siblings, 1 reply; 10+ messages in thread
From: Andrew Donnellan @ 2019-02-08  0:34 UTC (permalink / raw)
  To: Segher Boessenkool, Nicholas Piggin; +Cc: syzkaller, linuxppc-dev, dvyukov

(+ Nick)

On 7/2/19 6:49 pm, Segher Boessenkool wrote:
> On Thu, Feb 07, 2019 at 05:59:48PM +1100, Andrew Donnellan wrote:
>> On 7/2/19 5:37 pm, Segher Boessenkool wrote:
>>> On Thu, Feb 07, 2019 at 04:33:23PM +1100, Andrew Donnellan wrote:
>>>> Some older gccs (<GCC 7), when invoked with -fsanitize-coverage=trace-pc,
>>>> cause a spurious uninitialised variable warning in dt_cpu_ftrs.c:
>>>>
>>>>    arch/powerpc/kernel/dt_cpu_ftrs.c: In function
>>>>    ‘cpufeatures_process_feature’:
>>>>    arch/powerpc/kernel/dt_cpu_ftrs.c:686:7: warning: ‘m’ may be used
>>>>    uninitialized in this function [-Wmaybe-uninitialized]
>>>>      if (m->cpu_ftr_bit_mask)
>>>
>>> It seems to me the warning is correct?  If enable_unknown is false and no
>>> cpu_feature is found, it will in
>>>
>>> 	if (m->cpu_ftr_bit_mask)
>>> 		cur_cpu_spec->cpu_features |= m->cpu_ftr_bit_mask;
>>>
>>> enable random features (whatever was last in the table), or indeed access
>>> via NULL if the table is length 0?  So maybe this should be
>>>
>>> 	if (known && m->cpu_ftr_bit_mask)
>>> 		cur_cpu_spec->cpu_features |= m->cpu_ftr_bit_mask;
>>>
>>> instead?  (The code would be much clearer if all the known vs. !known
>>> codepath was fully separated here).
>>
>> The table is never length 0, it's a statically defined array.
> 
> Sure, and presumably that is why newer GCC doesn't warn.  But what about
> the other point?  Is this code ever correct?  Enabling random features
> (in cur_cpu_spec->cpu_features) when the name isn't found seems wrong.

Now that I'm replying without being 2 minutes before a meeting :)

The warning is still spurious, but the logic looks very suspicious.

I think your solution looks correct, though the whole function could be 
cleaned up a bit.

I also notice that if enable_unknown == false, then I think an unknown 
feature will still print "enabling" and return true, which seems wrong.

How does something like the following look, which I could send instead 
and will probably solve the spurious warnings issues anyway?

diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c 
b/arch/powerpc/kernel/dt_cpu_ftrs.c
index 2192b2114513..0f13048dc0dd 100644
--- a/arch/powerpc/kernel/dt_cpu_ftrs.c
+++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
@@ -658,40 +658,31 @@ static void __init cpufeatures_setup_start(u32 isa)

  static bool __init cpufeatures_process_feature(struct dt_cpu_feature *f)
  {
-       const struct dt_cpu_feature_match *m = NULL;
-       bool known = false;
+       const struct dt_cpu_feature_match *m;
         int i;

         for (i = 0; i < ARRAY_SIZE(dt_cpu_feature_match_table); i++) {
                 m = &dt_cpu_feature_match_table[i];
                 if (!strcmp(f->name, m->name)) {
-                       known = true;
-                       if (m->enable(f))
-                               break;
-
-                       pr_info("not enabling: %s (disabled or 
unsupported by kernel)\n",
-                               f->name);
-                       return false;
-               }
-       }
-
-       if (!known && enable_unknown) {
-               if (!feat_try_enable_unknown(f)) {
-                       pr_info("not enabling: %s (unknown and 
unsupported by kernel)\n",
-                               f->name);
-                       return false;
+                       if (!m->enable(f)) {
+                               pr_info("not enabling: %s (disabled or 
unsupported by kernel)\n",
+                                       f->name);
+                               return false;
+                       }
+                       pr_debug("enabling: %s\n", f->name);
+                       cur_cpu_spec->cpu_features |= m->cpu_ftr_bit_mask;
+                       return true;
                 }
         }

-       if (m->cpu_ftr_bit_mask)
-               cur_cpu_spec->cpu_features |= m->cpu_ftr_bit_mask;
-
-       if (known)
-               pr_debug("enabling: %s\n", f->name);
-       else
+       if (enable_unknown && feat_try_enable_unknown(f)) {
                 pr_debug("enabling: %s (unknown)\n", f->name);
-
-       return true;
+               return true;
+       } else {
+               pr_info("not enabling: %s (unknown and unsupported by 
kernel)\n",
+                       f->name);
+               return false;
+       }
  }

  static __init void cpufeatures_cpu_quirks(void)


-- 
Andrew Donnellan              OzLabs, ADL Canberra
andrew.donnellan@au1.ibm.com  IBM Australia Limited


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] powerpc/64s: Work around spurious warning on old gccs with -fsanitize-coverage
  2019-02-08  0:34       ` Andrew Donnellan
@ 2019-02-08  3:02         ` Michael Ellerman
  2019-02-08  3:11           ` Andrew Donnellan
  2019-02-08 15:41           ` Segher Boessenkool
  0 siblings, 2 replies; 10+ messages in thread
From: Michael Ellerman @ 2019-02-08  3:02 UTC (permalink / raw)
  To: Andrew Donnellan, Segher Boessenkool, Nicholas Piggin
  Cc: syzkaller, linuxppc-dev, dvyukov

Andrew Donnellan <andrew.donnellan@au1.ibm.com> writes:
> (+ Nick)
>
> On 7/2/19 6:49 pm, Segher Boessenkool wrote:
>> On Thu, Feb 07, 2019 at 05:59:48PM +1100, Andrew Donnellan wrote:
>>> On 7/2/19 5:37 pm, Segher Boessenkool wrote:
>>>> On Thu, Feb 07, 2019 at 04:33:23PM +1100, Andrew Donnellan wrote:
>>>>> Some older gccs (<GCC 7), when invoked with -fsanitize-coverage=trace-pc,
>>>>> cause a spurious uninitialised variable warning in dt_cpu_ftrs.c:
>>>>>
>>>>>    arch/powerpc/kernel/dt_cpu_ftrs.c: In function
>>>>>    ‘cpufeatures_process_feature’:
>>>>>    arch/powerpc/kernel/dt_cpu_ftrs.c:686:7: warning: ‘m’ may be used
>>>>>    uninitialized in this function [-Wmaybe-uninitialized]
>>>>>      if (m->cpu_ftr_bit_mask)
>>>>
>>>> It seems to me the warning is correct?  If enable_unknown is false and no
>>>> cpu_feature is found, it will in
>>>>
>>>> 	if (m->cpu_ftr_bit_mask)
>>>> 		cur_cpu_spec->cpu_features |= m->cpu_ftr_bit_mask;
>>>>
>>>> enable random features (whatever was last in the table), or indeed access
>>>> via NULL if the table is length 0?  So maybe this should be
>>>>
>>>> 	if (known && m->cpu_ftr_bit_mask)
>>>> 		cur_cpu_spec->cpu_features |= m->cpu_ftr_bit_mask;
>>>>
>>>> instead?  (The code would be much clearer if all the known vs. !known
>>>> codepath was fully separated here).
>>>
>>> The table is never length 0, it's a statically defined array.
>> 
>> Sure, and presumably that is why newer GCC doesn't warn.  But what about
>> the other point?  Is this code ever correct?  Enabling random features
>> (in cur_cpu_spec->cpu_features) when the name isn't found seems wrong.
>
> Now that I'm replying without being 2 minutes before a meeting :)
>
> The warning is still spurious, but the logic looks very suspicious.
>
> I think your solution looks correct, though the whole function could be 
> cleaned up a bit.
>
> I also notice that if enable_unknown == false, then I think an unknown 
> feature will still print "enabling" and return true, which seems wrong.
>
> How does something like the following look, which I could send instead 
> and will probably solve the spurious warnings issues anyway?

I'd prefer a minimal fix that we can backport. How about:

diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c b/arch/powerpc/kernel/dt_cpu_ftrs.c
index 8be3721d9302..a1acccd25839 100644
--- a/arch/powerpc/kernel/dt_cpu_ftrs.c
+++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
@@ -675,12 +675,10 @@ static bool __init cpufeatures_process_feature(struct dt_cpu_feature *f)
 		}
 	}
 
-	if (!known && enable_unknown) {
-		if (!feat_try_enable_unknown(f)) {
-			pr_info("not enabling: %s (unknown and unsupported by kernel)\n",
-				f->name);
-			return false;
-		}
+	if (!known && (!enable_unknown || !feat_try_enable_unknown(f))) {
+		pr_info("not enabling: %s (unknown and unsupported by kernel)\n",
+			f->name);
+		return false;
 	}
 
 	if (m->cpu_ftr_bit_mask)


cheers

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] powerpc/64s: Work around spurious warning on old gccs with -fsanitize-coverage
  2019-02-08  3:02         ` Michael Ellerman
@ 2019-02-08  3:11           ` Andrew Donnellan
  2019-02-08 15:41           ` Segher Boessenkool
  1 sibling, 0 replies; 10+ messages in thread
From: Andrew Donnellan @ 2019-02-08  3:11 UTC (permalink / raw)
  To: Michael Ellerman, Segher Boessenkool, Nicholas Piggin
  Cc: syzkaller, linuxppc-dev, dvyukov

On 8/2/19 2:02 pm, Michael Ellerman wrote:> I'd prefer a minimal fix 
that we can backport. How about:
> 
> diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c b/arch/powerpc/kernel/dt_cpu_ftrs.c
> index 8be3721d9302..a1acccd25839 100644
> --- a/arch/powerpc/kernel/dt_cpu_ftrs.c
> +++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
> @@ -675,12 +675,10 @@ static bool __init cpufeatures_process_feature(struct dt_cpu_feature *f)
>   		}
>   	}
>   
> -	if (!known && enable_unknown) {
> -		if (!feat_try_enable_unknown(f)) {
> -			pr_info("not enabling: %s (unknown and unsupported by kernel)\n",
> -				f->name);
> -			return false;
> -		}
> +	if (!known && (!enable_unknown || !feat_try_enable_unknown(f))) {
> +		pr_info("not enabling: %s (unknown and unsupported by kernel)\n",
> +			f->name);
> +		return false;
>   	}
>   
>   	if (m->cpu_ftr_bit_mask)
> 

Sure, I can send a v2 with this fix and your signoff?

-- 
Andrew Donnellan              OzLabs, ADL Canberra
andrew.donnellan@au1.ibm.com  IBM Australia Limited


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] powerpc/64s: Work around spurious warning on old gccs with -fsanitize-coverage
  2019-02-08  3:02         ` Michael Ellerman
  2019-02-08  3:11           ` Andrew Donnellan
@ 2019-02-08 15:41           ` Segher Boessenkool
  2019-02-10  5:14             ` Andrew Donnellan
  1 sibling, 1 reply; 10+ messages in thread
From: Segher Boessenkool @ 2019-02-08 15:41 UTC (permalink / raw)
  To: Michael Ellerman
  Cc: dvyukov, syzkaller, linuxppc-dev, Nicholas Piggin, Andrew Donnellan

On Fri, Feb 08, 2019 at 02:02:24PM +1100, Michael Ellerman wrote:
> diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c b/arch/powerpc/kernel/dt_cpu_ftrs.c
> index 8be3721d9302..a1acccd25839 100644
> --- a/arch/powerpc/kernel/dt_cpu_ftrs.c
> +++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
> @@ -675,12 +675,10 @@ static bool __init cpufeatures_process_feature(struct dt_cpu_feature *f)
>  		}
>  	}
>  
> -	if (!known && enable_unknown) {
> -		if (!feat_try_enable_unknown(f)) {
> -			pr_info("not enabling: %s (unknown and unsupported by kernel)\n",
> -				f->name);
> -			return false;
> -		}
> +	if (!known && (!enable_unknown || !feat_try_enable_unknown(f))) {
> +		pr_info("not enabling: %s (unknown and unsupported by kernel)\n",
> +			f->name);
> +		return false;
>  	}
>  
>  	if (m->cpu_ftr_bit_mask)
   		cur_cpu_spec->cpu_features |= m->cpu_ftr_bit_mask;

This still set the wrong mask here, which is the bug you're trying to fix.
It should only do this if "known", afaics.


Segher

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] powerpc/64s: Work around spurious warning on old gccs with -fsanitize-coverage
  2019-02-08 15:41           ` Segher Boessenkool
@ 2019-02-10  5:14             ` Andrew Donnellan
  0 siblings, 0 replies; 10+ messages in thread
From: Andrew Donnellan @ 2019-02-10  5:14 UTC (permalink / raw)
  To: Segher Boessenkool, Michael Ellerman
  Cc: syzkaller, linuxppc-dev, dvyukov, Nicholas Piggin

On 9/2/19 2:41 am, Segher Boessenkool wrote:
> On Fri, Feb 08, 2019 at 02:02:24PM +1100, Michael Ellerman wrote:
>> diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c b/arch/powerpc/kernel/dt_cpu_ftrs.c
>> index 8be3721d9302..a1acccd25839 100644
>> --- a/arch/powerpc/kernel/dt_cpu_ftrs.c
>> +++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
>> @@ -675,12 +675,10 @@ static bool __init cpufeatures_process_feature(struct dt_cpu_feature *f)
>>   		}
>>   	}
>>   
>> -	if (!known && enable_unknown) {
>> -		if (!feat_try_enable_unknown(f)) {
>> -			pr_info("not enabling: %s (unknown and unsupported by kernel)\n",
>> -				f->name);
>> -			return false;
>> -		}
>> +	if (!known && (!enable_unknown || !feat_try_enable_unknown(f))) {
>> +		pr_info("not enabling: %s (unknown and unsupported by kernel)\n",
>> +			f->name);
>> +		return false;
>>   	}
>>   
>>   	if (m->cpu_ftr_bit_mask)
>     		cur_cpu_spec->cpu_features |= m->cpu_ftr_bit_mask;
> 
> This still set the wrong mask here, which is the bug you're trying to fix.
> It should only do this if "known", afaics.

I've got a v2 ready to send which fixes both things.

> 
> 
> Segher
> 

-- 
Andrew Donnellan              OzLabs, ADL Canberra
andrew.donnellan@au1.ibm.com  IBM Australia Limited


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, back to index

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-07  5:33 [PATCH 1/2] powerpc/64s: Work around spurious warning on old gccs with -fsanitize-coverage Andrew Donnellan
2019-02-07  5:33 ` [PATCH 2/2] powerpc: Enable kcov Andrew Donnellan
2019-02-07  6:37 ` [PATCH 1/2] powerpc/64s: Work around spurious warning on old gccs with -fsanitize-coverage Segher Boessenkool
2019-02-07  6:59   ` Andrew Donnellan
2019-02-07  7:49     ` Segher Boessenkool
2019-02-08  0:34       ` Andrew Donnellan
2019-02-08  3:02         ` Michael Ellerman
2019-02-08  3:11           ` Andrew Donnellan
2019-02-08 15:41           ` Segher Boessenkool
2019-02-10  5:14             ` Andrew Donnellan

LinuxPPC-Dev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linuxppc-dev/0 linuxppc-dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linuxppc-dev linuxppc-dev/ https://lore.kernel.org/linuxppc-dev \
		linuxppc-dev@lists.ozlabs.org linuxppc-dev@ozlabs.org linuxppc-dev@archiver.kernel.org
	public-inbox-index linuxppc-dev


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.ozlabs.lists.linuxppc-dev


AGPL code for this site: git clone https://public-inbox.org/ public-inbox