powerpc/64s: dt_cpu_ftrs boot time setup option
Subsystems (2)
Nicholas Piggin
reviewerPATCH 1/1 powerpc/64s: dt_cpu_ftrs boot time setup option
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.
--- 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(-)
Documentation/admin-guide/kernel-parameters.txt +10 -0
@@ -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
arch/powerpc/Kconfig +0 -5
@@ -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
arch/powerpc/kernel/dt_cpu_ftrs.c +32 -9
70 lines not on this page · raw message
Signature · 1 line
2.11.0
Quoted 2 lines · Nicholas Piggin
Provide a dt_cpu_ftrs= cmdline option to disable the
dt_cpu_ftrsCPU 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
Michael Ellerman
reviewerQuoted 5 lines · Paul Clarke
Quoted 2 lines · Nicholas Piggin
Provide a dt_cpu_ftrs= cmdline option to disable the
dt_cpu_ftrsCPU 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! :)
Quoted 1 line
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
Quoted 2 lines
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
Quoted 8 lines · Michael Ellerman
Quoted 5 lines · Paul Clarke
Quoted 2 lines · Nicholas Piggin
Provide a dt_cpu_ftrs= cmdline option to disable the
dt_cpu_ftrsCPU 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. ;-)
Quoted 1 line
Quoted 1 line
Was it too difficult to type "features"?
I see "ftrs" and think "footers".
Quoted 5 lines
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! ;-)
Quoted 7 lines
:P
Quoted 2 lines
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
Michael Ellerman
reviewerQuoted 8 lines · Paul Clarke
Quoted 7 lines · Michael Ellerman
Quoted 5 lines · Paul Clarke
Quoted 2 lines · Nicholas Piggin
Provide a dt_cpu_ftrs= cmdline option to disable the
dt_cpu_ftrsCPU feature discovery, and fall back to the "cputable" based version.This boat has already sailed, I think, but "ftrs"?
Quoted 7 lines
Quoted 1 line
What you think vowels grow on trees! :)
At least you're using lower-case, which takes less space. ;-)
Quoted 1 line
Quoted 1 line
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
Michael Ellerman
reviewerQuoted 2 lines · Nicholas Piggin
Provide a dt_cpu_ftrs= cmdline option to disable the
dt_cpu_ftrsCPU feature discovery, and fall back to the "cputable" based version.
I don't think this works properly.
Quoted 21 lines
arch/powerpc/kernel/dt_cpu_ftrs.c +10
-0@@ -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
arch/powerpc/kernel/dt_cpu_ftrs.c +24 -0
@@ -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;
Nicholas Piggin
reviewerQuoted 35 lines · On Mon, 29 May 2017 20:29:49 +1000 Michael Ellerman <[email protected]> wrote:
Quoted 2 lines · Nicholas Piggin
Provide a dt_cpu_ftrs= cmdline option to disable the
dt_cpu_ftrsCPU feature discovery, and fall back to the "cputable" based version.I don't think this works properly.
Quoted 21 lines
arch/powerpc/kernel/dt_cpu_ftrs.c +10
-0@@ -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 callingidentify_cpu().
Quoted 8 lines
So although passing dt_cpu_ftrs=off will skip the later logic, it doesn't cause us to call
identify_cpu()inearly_setup()which it should.In practice it works because the base CPU spec that we initialise in
dt_cpu_ftrs.cis mostly OK, and skiboot also adds the cpu-version property which causes us to callidentify_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.
Quoted 5 lines
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
Quoted 52 lines
cheers
arch/powerpc/kernel/dt_cpu_ftrs.c +24
-0@@ -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;
Michael Ellerman
reviewerQuoted 3 lines · Nicholas Piggin
Quoted 1 line · On Mon, 29 May 2017 20:29:49 +1000 Michael Ellerman <[email protected]> wrote:
Nicholas Piggin <[email protected]> writes:
...
Quoted 8 lines
Quoted 5 lines
In practice it works because the base CPU spec that we initialise in
dt_cpu_ftrs.cis mostly OK, and skiboot also adds the cpu-version property which causes us to callidentify_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.
Quoted 8 lines
Quoted 4 lines
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()usingcall_promfor 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
Quoted 72 lines · Nicholas Piggin
Provide a dt_cpu_ftrs= cmdline option to disable the
dt_cpu_ftrsCPU 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 <[email protected]>--- 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(-)Documentation/admin-guide/kernel-parameters.txt +10
-0@@ -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 determinearch/powerpc/Kconfig +0
-5@@ -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 PPC32arch/powerpc/kernel/dt_cpu_ftrs.c +8
-6@@ -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 ; +
arch/powerpc/kernel/dt_cpu_ftrs.c static struct dt_cpu_feature_match __initdata · line 691
Quoted 10 lines
arch/powerpc/kernel/dt_cpu_ftrs.c +10
-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.
Michael Ellerman
reviewerQuoted 27 lines · Balbir Singh
Quoted 26 lines · Nicholas Piggin
arch/powerpc/kernel/dt_cpu_ftrs.c +12
-6@@ -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;
Quoted 8 lines
arch/powerpc/kernel/dt_cpu_ftrs.c static struct dt_cpu_feature_match __initdata · line 691
Quoted 6 lines
arch/powerpc/kernel/dt_cpu_ftrs.c +6
-0+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
Michael Ellerman
AppliedQuoted 7 lines · Nicholas Piggin
Provide a dt_cpu_ftrs= cmdline option to disable the
dt_cpu_ftrsCPU 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 <[email protected]>
Applied to powerpc fixes, thanks.
https://git.kernel.org/powerpc/c/a2b05b7aa60e1e9b60faf01dfb1cca
cheers