Re: [PATCH] powerpc/64s: dt_cpu_ftrs boot time setup option
From: Nicholas Piggin <npiggin@gmail.com>
Date: 2017-05-30 00:18:42
On Mon, 29 May 2017 20:29:49 +1000 Michael Ellerman [off-list ref] wrote:
Nicholas Piggin [off-list ref] writes:quoted
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.quoted
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
quoted hunk ↗ jump to hunk
cheersdiff --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;