All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] powerpc/64s: dt_cpu_ftrs boot time setup option
@ 2017-05-11 11:24 Nicholas Piggin
  2017-05-11 13:25 ` Paul Clarke
                   ` (3 more replies)
  0 siblings, 4 replies; 11+ messages in thread
From: Nicholas Piggin @ 2017-05-11 11:24 UTC (permalink / raw)
  To: linuxppc-dev; +Cc: Nicholas Piggin

Provide a dt_cpu_ftrs= cmdline option to disable the dt_cpu_ftrs CPU
feature discovery, and fall back to the "cputable" based version.

Also allow control of advertising unknown features to userspace and
with this parameter, and remove the clunky CONFIG option.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 Documentation/admin-guide/kernel-parameters.txt | 10 ++++++
 arch/powerpc/Kconfig                            |  5 ---
 arch/powerpc/kernel/dt_cpu_ftrs.c               | 41 +++++++++++++++++++------
 3 files changed, 42 insertions(+), 14 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index facc20a3f962..7a7c1cc80f9f 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -867,6 +867,16 @@
 
 	dscc4.setup=	[NET]
 
+	dt_cpu_ftrs=	[PPC]
+			Format: {"off" | "known"}
+			Control how the dt_cpu_ftrs device-tree binding is
+			used for CPU feature discovery and setup (if it
+			exists).
+			off: Do not use it, fall back to legacy cpu table.
+			known: Do not pass through unknown features to guests
+			or userspace, only those that the kernel is not aware
+			of.
+
 	dump_apple_properties	[X86]
 			Dump name and content of EFI device properties on
 			x86 Macs.  Useful for driver authors to determine
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 5bd868f2e813..c924710e5bc7 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -391,11 +391,6 @@ config PPC_DT_CPU_FTRS
 	  firmware provides this binding.
 	  If you're not sure say Y.
 
-config PPC_CPUFEATURES_ENABLE_UNKNOWN
-	bool "cpufeatures pass through unknown features to guest/userspace"
-	depends on PPC_DT_CPU_FTRS
-	default y
-
 config HIGHMEM
 	bool "High memory support"
 	depends on PPC32
diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c b/arch/powerpc/kernel/dt_cpu_ftrs.c
index fcc7588a96d6..050925b5b451 100644
--- a/arch/powerpc/kernel/dt_cpu_ftrs.c
+++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
@@ -671,12 +671,24 @@ static struct dt_cpu_feature_match __initdata
 	{"wait-v3", feat_enable, 0},
 };
 
-/* XXX: how to configure this? Default + boot time? */
-#ifdef CONFIG_PPC_CPUFEATURES_ENABLE_UNKNOWN
-#define CPU_FEATURE_ENABLE_UNKNOWN 1
-#else
-#define CPU_FEATURE_ENABLE_UNKNOWN 0
-#endif
+static bool __initdata using_dt_cpu_ftrs = true;
+static bool __initdata dt_cpu_ftrs_enable_unknown = true;
+
+static int __init dt_cpu_ftrs_parse(char *str)
+{
+	if (!str)
+		return 0;
+
+	if (!strcmp(str, "off"))
+		using_dt_cpu_ftrs = false;
+	else if (!strcmp(str, "known"))
+		dt_cpu_ftrs_enable_unknown = false;
+	else
+		return 1;
+
+	return 0;
+}
+early_param("dt_cpu_ftrs", dt_cpu_ftrs_parse);
 
 static void __init cpufeatures_setup_start(u32 isa)
 {
@@ -707,7 +719,7 @@ static bool __init cpufeatures_process_feature(struct dt_cpu_feature *f)
 		}
 	}
 
-	if (!known && CPU_FEATURE_ENABLE_UNKNOWN) {
+	if (!known && dt_cpu_ftrs_enable_unknown) {
 		if (!feat_try_enable_unknown(f)) {
 			pr_info("not enabling: %s (unknown and unsupported by kernel)\n",
 				f->name);
@@ -766,8 +778,6 @@ static int __init fdt_find_cpu_features(unsigned long node, const char *uname,
 	return 0;
 }
 
-static bool __initdata using_dt_cpu_ftrs = false;
-
 bool __init dt_cpu_ftrs_in_use(void)
 {
 	return using_dt_cpu_ftrs;
@@ -775,6 +785,16 @@ bool __init dt_cpu_ftrs_in_use(void)
 
 bool __init dt_cpu_ftrs_init(void *fdt)
 {
+	if (!using_dt_cpu_ftrs) {
+		/*
+		 * This should never happen because this runs before
+		 * early_praam, however if the init ordering changes,
+		 * test if early_param has disabled this.
+		 */
+		return false;
+	}
+	using_dt_cpu_ftrs = false;
+
 	/* Setup and verify the FDT, if it fails we just bail */
 	if (!early_init_dt_verify(fdt))
 		return false;
@@ -1027,5 +1047,8 @@ static int __init dt_cpu_ftrs_scan_callback(unsigned long node, const char
 
 void __init dt_cpu_ftrs_scan(void)
 {
+	if (!using_dt_cpu_ftrs)
+		return;
+
 	of_scan_flat_dt(dt_cpu_ftrs_scan_callback, NULL);
 }
-- 
2.11.0

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

* Re: [PATCH] powerpc/64s: dt_cpu_ftrs boot time setup option
  2017-05-11 11:24 [PATCH] powerpc/64s: dt_cpu_ftrs boot time setup option Nicholas Piggin
@ 2017-05-11 13:25 ` Paul Clarke
  2017-05-12  3:46   ` Michael Ellerman
  2017-05-29 10:29 ` Michael Ellerman
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 11+ messages in thread
From: Paul Clarke @ 2017-05-11 13:25 UTC (permalink / raw)
  To: Nicholas Piggin, linuxppc-dev

On 05/11/2017 06:24 AM, Nicholas Piggin wrote:
> Provide a dt_cpu_ftrs= cmdline option to disable the dt_cpu_ftrs CPU
> feature discovery, and fall back to the "cputable" based version.

This boat has already sailed, I think, but "ftrs"?  Was it too difficult to type "features"?  This seems like something that should be easy and intuitive to find and understand.

:-)

PC

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

* Re: [PATCH] powerpc/64s: dt_cpu_ftrs boot time setup option
  2017-05-11 13:25 ` Paul Clarke
@ 2017-05-12  3:46   ` Michael Ellerman
  2017-05-12 12:32     ` Paul Clarke
  0 siblings, 1 reply; 11+ messages in thread
From: Michael Ellerman @ 2017-05-12  3:46 UTC (permalink / raw)
  To: pc, Nicholas Piggin, linuxppc-dev

Paul Clarke <pc@us.ibm.com> writes:

> On 05/11/2017 06:24 AM, Nicholas Piggin wrote:
>> Provide a dt_cpu_ftrs= cmdline option to disable the dt_cpu_ftrs CPU
>> feature discovery, and fall back to the "cputable" based version.
>
> This boat has already sailed, I think, but "ftrs"?

What you think vowels grow on trees! :)

> Was it too difficult to type "features"?

For the command line option we could spell out features.

But should we also expand "dt", and "cpu" ?

device_tree_central_processing_unit_features=off

:P

> This seems like something that should be easy and intuitive to find
> and understand.

Maybe. Ideally no one will ever use it, certainly not end users, it's
primarily intended for developers doing bring-up.

cheers

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

* Re: [PATCH] powerpc/64s: dt_cpu_ftrs boot time setup option
  2017-05-12  3:46   ` Michael Ellerman
@ 2017-05-12 12:32     ` Paul Clarke
  2017-05-15  9:43       ` Michael Ellerman
  0 siblings, 1 reply; 11+ messages in thread
From: Paul Clarke @ 2017-05-12 12:32 UTC (permalink / raw)
  To: Michael Ellerman, Nicholas Piggin, linuxppc-dev

On 05/11/2017 10:46 PM, Michael Ellerman wrote:
> Paul Clarke <pc@us.ibm.com> writes:
>> On 05/11/2017 06:24 AM, Nicholas Piggin wrote:
>>> Provide a dt_cpu_ftrs= cmdline option to disable the dt_cpu_ftrs CPU
>>> feature discovery, and fall back to the "cputable" based version.
>>
>> This boat has already sailed, I think, but "ftrs"?
> 
> What you think vowels grow on trees! :)

At least you're using lower-case, which takes less space.  ;-)

>> Was it too difficult to type "features"?

I see "ftrs" and think "footers".

> For the command line option we could spell out features.
> 
> But should we also expand "dt", and "cpu" ?
> 
> device_tree_central_processing_unit_features=off

I'm not the biggest fan of "dt" ("devtree", anyone?), but that's certainly established.

So _that's_ what "cpu" stands for!  ;-)

> :P
> 
>> This seems like something that should be easy and intuitive to find
>> and understand.
> 
> Maybe. Ideally no one will ever use it, certainly not end users, it's
> primarily intended for developers doing bring-up.

I'm a developer, and I like to be able to understand something at a glance.  (Hmm, what are "CPU footers"?)

:-P

PC

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

* Re: [PATCH] powerpc/64s: dt_cpu_ftrs boot time setup option
  2017-05-12 12:32     ` Paul Clarke
@ 2017-05-15  9:43       ` Michael Ellerman
  0 siblings, 0 replies; 11+ messages in thread
From: Michael Ellerman @ 2017-05-15  9:43 UTC (permalink / raw)
  To: pc, Nicholas Piggin, linuxppc-dev

Paul Clarke <pc@us.ibm.com> writes:

> On 05/11/2017 10:46 PM, Michael Ellerman wrote:
>> Paul Clarke <pc@us.ibm.com> writes:
>>> On 05/11/2017 06:24 AM, Nicholas Piggin wrote:
>>>> Provide a dt_cpu_ftrs= cmdline option to disable the dt_cpu_ftrs CPU
>>>> feature discovery, and fall back to the "cputable" based version.
>>>
>>> This boat has already sailed, I think, but "ftrs"?
>> 
>> What you think vowels grow on trees! :)
>
> At least you're using lower-case, which takes less space.  ;-)
>
>>> Was it too difficult to type "features"?
>
> I see "ftrs" and think "footers".

"FTR" - the shouty version - has meant "feature" in the powerpc code
base since at least 2002.

So it may be a crappy abbreviation but it's at least consistent :)

cheers

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

* Re: [PATCH] powerpc/64s: dt_cpu_ftrs boot time setup option
  2017-05-11 11:24 [PATCH] powerpc/64s: dt_cpu_ftrs boot time setup option Nicholas Piggin
  2017-05-11 13:25 ` Paul Clarke
@ 2017-05-29 10:29 ` Michael Ellerman
  2017-05-30  0:18   ` Nicholas Piggin
  2017-05-29 22:29 ` Balbir Singh
  2017-06-01 13:31 ` Michael Ellerman
  3 siblings, 1 reply; 11+ messages in thread
From: Michael Ellerman @ 2017-05-29 10:29 UTC (permalink / raw)
  To: Nicholas Piggin, linuxppc-dev; +Cc: Nicholas Piggin, pc

Nicholas Piggin <npiggin@gmail.com> writes:

> Provide a dt_cpu_ftrs= cmdline option to disable the dt_cpu_ftrs CPU
> feature discovery, and fall back to the "cputable" based version.

I don't think this works properly.

> diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c b/arch/powerpc/kernel/dt_cpu_ftrs.c
> index fcc7588a96d6..050925b5b451 100644
> --- a/arch/powerpc/kernel/dt_cpu_ftrs.c
> +++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
> @@ -775,6 +785,16 @@ bool __init dt_cpu_ftrs_in_use(void)
>  
>  bool __init dt_cpu_ftrs_init(void *fdt)
>  {
> +	if (!using_dt_cpu_ftrs) {
> +		/*
> +		 * This should never happen because this runs before
> +		 * early_praam, however if the init ordering changes,
> +		 * test if early_param has disabled this.
> +		 */
> +		return false;
> +	}
> +	using_dt_cpu_ftrs = false;
> +
>  	/* Setup and verify the FDT, if it fails we just bail */
>  	if (!early_init_dt_verify(fdt))
>  		return false;
...
	return true;

Because this runs before early_param(), as you mention,
dt_cpu_ftrs_init() returns true, which means we skip calling
identify_cpu().

So although passing dt_cpu_ftrs=off will skip the later logic, it
doesn't cause us to call identify_cpu() in early_setup() which it
should.

In practice it works because the base CPU spec that we initialise in
dt_cpu_ftrs.c is mostly OK, and skiboot also adds the cpu-version
property which causes us to call identify_cpu() again later, but that's
all a bit fragile IMHO.

So unfortunately I think we need to add logic in dt_cpu_ftrs_init() to
look for dt_cpu_ftrs=off on the command line.

The patch below seems to work, but would appreciate more eyes on it.

cheers

diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c b/arch/powerpc/kernel/dt_cpu_ftrs.c
index d6f05e4dc328..9a560954498a 100644
--- a/arch/powerpc/kernel/dt_cpu_ftrs.c
+++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
@@ -8,6 +8,7 @@
 #include <linux/export.h>
 #include <linux/init.h>
 #include <linux/jump_label.h>
+#include <linux/libfdt.h>
 #include <linux/memblock.h>
 #include <linux/printk.h>
 #include <linux/sched.h>
@@ -767,6 +770,26 @@ static void __init cpufeatures_setup_finished(void)
 		cur_cpu_spec->cpu_features, cur_cpu_spec->mmu_features);
 }
 
+static int __init disabled_on_cmdline(void)
+{
+	unsigned long root, chosen;
+	const char *p;
+
+	root = of_get_flat_dt_root();
+	chosen = of_get_flat_dt_subnode_by_name(root, "chosen");
+	if (chosen == -FDT_ERR_NOTFOUND)
+		return false;
+
+	p = of_get_flat_dt_prop(chosen, "bootargs", NULL);
+	if (!p)
+		return false;
+
+	if (strstr(p, "dt_cpu_ftrs=off"))
+		return true;
+
+	return false;
+}
+
 static int __init fdt_find_cpu_features(unsigned long node, const char *uname,
 					int depth, void *data)
 {
@@ -801,6 +826,9 @@ bool __init dt_cpu_ftrs_init(void *fdt)
 	if (!of_scan_flat_dt(fdt_find_cpu_features, NULL))
 		return false;
 
+	if (disabled_on_cmdline())
+		return false;
+
 	cpufeatures_setup_cpu();
 
 	using_dt_cpu_ftrs = true;

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

* Re: [PATCH] powerpc/64s: dt_cpu_ftrs boot time setup option
  2017-05-11 11:24 [PATCH] powerpc/64s: dt_cpu_ftrs boot time setup option Nicholas Piggin
  2017-05-11 13:25 ` Paul Clarke
  2017-05-29 10:29 ` Michael Ellerman
@ 2017-05-29 22:29 ` Balbir Singh
  2017-05-30  5:29   ` Michael Ellerman
  2017-06-01 13:31 ` Michael Ellerman
  3 siblings, 1 reply; 11+ messages in thread
From: Balbir Singh @ 2017-05-29 22:29 UTC (permalink / raw)
  To: Nicholas Piggin, linuxppc-dev

On Thu, 2017-05-11 at 21:24 +1000, Nicholas Piggin wrote:
> Provide a dt_cpu_ftrs= cmdline option to disable the dt_cpu_ftrs CPU
> feature discovery, and fall back to the "cputable" based version.
> 
> Also allow control of advertising unknown features to userspace and
> with this parameter, and remove the clunky CONFIG option.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  Documentation/admin-guide/kernel-parameters.txt | 10 ++++++
>  arch/powerpc/Kconfig                            |  5 ---
>  arch/powerpc/kernel/dt_cpu_ftrs.c               | 41 +++++++++++++++++++------
>  3 files changed, 42 insertions(+), 14 deletions(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index facc20a3f962..7a7c1cc80f9f 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -867,6 +867,16 @@
>  
>  	dscc4.setup=	[NET]
>  
> +	dt_cpu_ftrs=	[PPC]
> +			Format: {"off" | "known"}
> +			Control how the dt_cpu_ftrs device-tree binding is
> +			used for CPU feature discovery and setup (if it
> +			exists).
> +			off: Do not use it, fall back to legacy cpu table.
> +			known: Do not pass through unknown features to guests
> +			or userspace, only those that the kernel is not aware
> +			of.
> +
>  	dump_apple_properties	[X86]
>  			Dump name and content of EFI device properties on
>  			x86 Macs.  Useful for driver authors to determine
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index 5bd868f2e813..c924710e5bc7 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -391,11 +391,6 @@ config PPC_DT_CPU_FTRS
>  	  firmware provides this binding.
>  	  If you're not sure say Y.
>  
> -config PPC_CPUFEATURES_ENABLE_UNKNOWN
> -	bool "cpufeatures pass through unknown features to guest/userspace"
> -	depends on PPC_DT_CPU_FTRS
> -	default y
> -
>  config HIGHMEM
>  	bool "High memory support"
>  	depends on PPC32
> diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c b/arch/powerpc/kernel/dt_cpu_ftrs.c
> index fcc7588a96d6..050925b5b451 100644
> --- a/arch/powerpc/kernel/dt_cpu_ftrs.c
> +++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
> @@ -671,12 +671,24 @@ static struct dt_cpu_feature_match __initdata
>  	{"wait-v3", feat_enable, 0},
>  };
>  
> -/* XXX: how to configure this? Default + boot time? */
> -#ifdef CONFIG_PPC_CPUFEATURES_ENABLE_UNKNOWN
> -#define CPU_FEATURE_ENABLE_UNKNOWN 1
> -#else
> -#define CPU_FEATURE_ENABLE_UNKNOWN 0
> -#endif
> +static bool __initdata using_dt_cpu_ftrs = true;
> +static bool __initdata dt_cpu_ftrs_enable_unknown = true;
> +
> +static int __init dt_cpu_ftrs_parse(char *str)
> +{
> +	if (!str)
> +		return 0;
> +
> +	if (!strcmp(str, "off"))
> +		using_dt_cpu_ftrs = false;
> +	else if (!strcmp(str, "known"))
> +		dt_cpu_ftrs_enable_unknown = false;
> +	else
> +		return 1;
> +
> +	return 0;
> +}
> +early_param("dt_cpu_ftrs", dt_cpu_ftrs_parse);

I wouldn't use strcmp with user passed parameters.

Balbir Singh.

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

* Re: [PATCH] powerpc/64s: dt_cpu_ftrs boot time setup option
  2017-05-29 10:29 ` Michael Ellerman
@ 2017-05-30  0:18   ` Nicholas Piggin
  2017-05-30  5:32     ` Michael Ellerman
  0 siblings, 1 reply; 11+ messages in thread
From: Nicholas Piggin @ 2017-05-30  0:18 UTC (permalink / raw)
  To: Michael Ellerman; +Cc: linuxppc-dev, pc

On Mon, 29 May 2017 20:29:49 +1000
Michael Ellerman <mpe@ellerman.id.au> wrote:

> Nicholas Piggin <npiggin@gmail.com> writes:
> 
> > Provide a dt_cpu_ftrs= cmdline option to disable the dt_cpu_ftrs CPU
> > feature discovery, and fall back to the "cputable" based version.  
> 
> I don't think this works properly.
> 
> > diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c b/arch/powerpc/kernel/dt_cpu_ftrs.c
> > index fcc7588a96d6..050925b5b451 100644
> > --- a/arch/powerpc/kernel/dt_cpu_ftrs.c
> > +++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
> > @@ -775,6 +785,16 @@ bool __init dt_cpu_ftrs_in_use(void)
> >  
> >  bool __init dt_cpu_ftrs_init(void *fdt)
> >  {
> > +	if (!using_dt_cpu_ftrs) {
> > +		/*
> > +		 * This should never happen because this runs before
> > +		 * early_praam, however if the init ordering changes,
> > +		 * test if early_param has disabled this.
> > +		 */
> > +		return false;
> > +	}
> > +	using_dt_cpu_ftrs = false;
> > +
> >  	/* Setup and verify the FDT, if it fails we just bail */
> >  	if (!early_init_dt_verify(fdt))
> >  		return false;  
> ...
> 	return true;
> 
> Because this runs before early_param(), as you mention,
> dt_cpu_ftrs_init() returns true, which means we skip calling
> identify_cpu().
> 
> So although passing dt_cpu_ftrs=off will skip the later logic, it
> doesn't cause us to call identify_cpu() in early_setup() which it
> should.
> 
> In practice it works because the base CPU spec that we initialise in
> dt_cpu_ftrs.c is mostly OK, and skiboot also adds the cpu-version
> property which causes us to call identify_cpu() again later, but that's
> all a bit fragile IMHO.

Yeah that's a huge bug :P Good catch. Possible something would fail on
CPUs that aren't POWER8/9.

> 
> So unfortunately I think we need to add logic in dt_cpu_ftrs_init() to
> look for dt_cpu_ftrs=off on the command line.
> 
> The patch below seems to work, but would appreciate more eyes on it.

I can't find a problem with it, but I don't know how this fdt/prom stuff
all fits together exactly. Why is early_cmdline_parse() using call_prom
for essentially the same thing?

Thanks,
Nick

> 
> cheers
> 
> diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c b/arch/powerpc/kernel/dt_cpu_ftrs.c
> index d6f05e4dc328..9a560954498a 100644
> --- a/arch/powerpc/kernel/dt_cpu_ftrs.c
> +++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
> @@ -8,6 +8,7 @@
>  #include <linux/export.h>
>  #include <linux/init.h>
>  #include <linux/jump_label.h>
> +#include <linux/libfdt.h>
>  #include <linux/memblock.h>
>  #include <linux/printk.h>
>  #include <linux/sched.h>
> @@ -767,6 +770,26 @@ static void __init cpufeatures_setup_finished(void)
>  		cur_cpu_spec->cpu_features, cur_cpu_spec->mmu_features);
>  }
>  
> +static int __init disabled_on_cmdline(void)
> +{
> +	unsigned long root, chosen;
> +	const char *p;
> +
> +	root = of_get_flat_dt_root();
> +	chosen = of_get_flat_dt_subnode_by_name(root, "chosen");
> +	if (chosen == -FDT_ERR_NOTFOUND)
> +		return false;
> +
> +	p = of_get_flat_dt_prop(chosen, "bootargs", NULL);
> +	if (!p)
> +		return false;
> +
> +	if (strstr(p, "dt_cpu_ftrs=off"))
> +		return true;
> +
> +	return false;
> +}
> +
>  static int __init fdt_find_cpu_features(unsigned long node, const char *uname,
>  					int depth, void *data)
>  {
> @@ -801,6 +826,9 @@ bool __init dt_cpu_ftrs_init(void *fdt)
>  	if (!of_scan_flat_dt(fdt_find_cpu_features, NULL))
>  		return false;
>  
> +	if (disabled_on_cmdline())
> +		return false;
> +
>  	cpufeatures_setup_cpu();
>  
>  	using_dt_cpu_ftrs = true;

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

* Re: [PATCH] powerpc/64s: dt_cpu_ftrs boot time setup option
  2017-05-29 22:29 ` Balbir Singh
@ 2017-05-30  5:29   ` Michael Ellerman
  0 siblings, 0 replies; 11+ messages in thread
From: Michael Ellerman @ 2017-05-30  5:29 UTC (permalink / raw)
  To: Balbir Singh, Nicholas Piggin, linuxppc-dev

Balbir Singh <bsingharora@gmail.com> writes:
> On Thu, 2017-05-11 at 21:24 +1000, Nicholas Piggin wrote:
>> diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c b/arch/powerpc/kernel/dt_cpu_ftrs.c
>> index fcc7588a96d6..050925b5b451 100644
>> --- a/arch/powerpc/kernel/dt_cpu_ftrs.c
>> +++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
>> @@ -671,12 +671,24 @@ static struct dt_cpu_feature_match __initdata
>>  	{"wait-v3", feat_enable, 0},
>>  };
>>  
>> -/* XXX: how to configure this? Default + boot time? */
>> -#ifdef CONFIG_PPC_CPUFEATURES_ENABLE_UNKNOWN
>> -#define CPU_FEATURE_ENABLE_UNKNOWN 1
>> -#else
>> -#define CPU_FEATURE_ENABLE_UNKNOWN 0
>> -#endif
>> +static bool __initdata using_dt_cpu_ftrs = true;
>> +static bool __initdata dt_cpu_ftrs_enable_unknown = true;
>> +
>> +static int __init dt_cpu_ftrs_parse(char *str)
>> +{
>> +	if (!str)
>> +		return 0;
>> +
>> +	if (!strcmp(str, "off"))
>> +		using_dt_cpu_ftrs = false;
>> +	else if (!strcmp(str, "known"))
>> +		dt_cpu_ftrs_enable_unknown = false;
>> +	else
>> +		return 1;
>> +
>> +	return 0;
>> +}
>> +early_param("dt_cpu_ftrs", dt_cpu_ftrs_parse);
>
> I wouldn't use strcmp with user passed parameters.

Why not? Especially considering the generic code has already parsed the
string to find the = delimiter, and is just passing us the tail.

cheers

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

* Re: [PATCH] powerpc/64s: dt_cpu_ftrs boot time setup option
  2017-05-30  0:18   ` Nicholas Piggin
@ 2017-05-30  5:32     ` Michael Ellerman
  0 siblings, 0 replies; 11+ messages in thread
From: Michael Ellerman @ 2017-05-30  5:32 UTC (permalink / raw)
  To: Nicholas Piggin; +Cc: linuxppc-dev, pc

Nicholas Piggin <npiggin@gmail.com> writes:
> On Mon, 29 May 2017 20:29:49 +1000
> Michael Ellerman <mpe@ellerman.id.au> wrote:
>> Nicholas Piggin <npiggin@gmail.com> writes:
...
>> 
>> In practice it works because the base CPU spec that we initialise in
>> dt_cpu_ftrs.c is mostly OK, and skiboot also adds the cpu-version
>> property which causes us to call identify_cpu() again later, but that's
>> all a bit fragile IMHO.
>
> Yeah that's a huge bug :P Good catch. Possible something would fail on
> CPUs that aren't POWER8/9.

Yeah I had to hack a bit to make it fail because there's a few fallbacks
that save us - but yeah on something more different it would break.
 
>> So unfortunately I think we need to add logic in dt_cpu_ftrs_init() to
>> look for dt_cpu_ftrs=off on the command line.
>> 
>> The patch below seems to work, but would appreciate more eyes on it.
>
> I can't find a problem with it, but I don't know how this fdt/prom stuff
> all fits together exactly. Why is early_cmdline_parse() using call_prom
> for essentially the same thing?

Yeah it's a mess.

early_cmdline_parse() is in prom_init.c, which is *totally* different to
prom.c, confusing much?

prom_init runs while OpenFirmware is still alive, so it calls OF (aka.
prom) for things, whereas prom.c is part of the kernel proper and only
has the flat device tree to use.


I'll squash this in and send a v2.

cheers

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

* Re: powerpc/64s: dt_cpu_ftrs boot time setup option
  2017-05-11 11:24 [PATCH] powerpc/64s: dt_cpu_ftrs boot time setup option Nicholas Piggin
                   ` (2 preceding siblings ...)
  2017-05-29 22:29 ` Balbir Singh
@ 2017-06-01 13:31 ` Michael Ellerman
  3 siblings, 0 replies; 11+ messages in thread
From: Michael Ellerman @ 2017-06-01 13:31 UTC (permalink / raw)
  To: Nicholas Piggin, linuxppc-dev; +Cc: Nicholas Piggin

On Thu, 2017-05-11 at 11:24:41 UTC, Nicholas Piggin wrote:
> Provide a dt_cpu_ftrs= cmdline option to disable the dt_cpu_ftrs CPU
> feature discovery, and fall back to the "cputable" based version.
> 
> Also allow control of advertising unknown features to userspace and
> with this parameter, and remove the clunky CONFIG option.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>

Applied to powerpc fixes, thanks.

https://git.kernel.org/powerpc/c/a2b05b7aa60e1e9b60faf01dfb1cca

cheers

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

end of thread, other threads:[~2017-06-01 13:31 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-11 11:24 [PATCH] powerpc/64s: dt_cpu_ftrs boot time setup option Nicholas Piggin
2017-05-11 13:25 ` Paul Clarke
2017-05-12  3:46   ` Michael Ellerman
2017-05-12 12:32     ` Paul Clarke
2017-05-15  9:43       ` Michael Ellerman
2017-05-29 10:29 ` Michael Ellerman
2017-05-30  0:18   ` Nicholas Piggin
2017-05-30  5:32     ` Michael Ellerman
2017-05-29 22:29 ` Balbir Singh
2017-05-30  5:29   ` Michael Ellerman
2017-06-01 13:31 ` Michael Ellerman

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.