Messages in this thread | | | Date | Wed, 13 Feb 2019 16:37:50 -0800 (PST) | Subject | Re: [v4 PATCH 8/8] RISC-V: Assign hwcap as per comman capabilities. | From | Palmer Dabbelt <> |
| |
On Wed, 13 Feb 2019 00:44:42 PST (-0800), johan@kernel.org wrote: > On Tue, Feb 12, 2019 at 11:58:10AM -0800, Atish Patra wrote: >> On 2/12/19 3:25 AM, Johan Hovold wrote: >> > On Tue, Feb 12, 2019 at 03:10:12AM -0800, Atish Patra wrote: >> >> Currently, we set hwcap based on first valid hart from DT. This may not >> >> be correct always as that hart might not be current booting cpu or may >> >> have a different capability. >> >> >> >> Set hwcap as the capabilities supported by all possible harts with "okay" >> >> status. >> >> >> >> Signed-off-by: Atish Patra <atish.patra@wdc.com> >> >> --- >> >> arch/riscv/kernel/cpufeature.c | 41 ++++++++++++++++++++++------------------- >> >> 1 file changed, 22 insertions(+), 19 deletions(-) >> >> >> >> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c >> >> index e7a4701f..a1e4fb34 100644 >> >> --- a/arch/riscv/kernel/cpufeature.c >> >> +++ b/arch/riscv/kernel/cpufeature.c >> >> @@ -20,6 +20,7 @@ >> >> #include <linux/of.h> >> >> #include <asm/processor.h> >> >> #include <asm/hwcap.h> >> >> +#include <asm/smp.h> >> >> >> >> unsigned long elf_hwcap __read_mostly; >> >> #ifdef CONFIG_FPU >> >> @@ -42,28 +43,30 @@ void riscv_fill_hwcap(void) >> >> >> >> elf_hwcap = 0; >> >> >> >> - /* >> >> - * We don't support running Linux on hertergenous ISA systems. For >> >> - * now, we just check the ISA of the first "okay" processor. >> >> - */ >> >> for_each_of_cpu_node(node) { >> >> - if (riscv_of_processor_hartid(node) >= 0) >> >> - break; >> >> - } >> >> - if (!node) { >> >> - pr_warn("Unable to find \"cpu\" devicetree entry\n"); >> >> - return; >> >> - } >> >> + unsigned long this_hwcap = 0; >> >> >> >> - if (of_property_read_string(node, "riscv,isa", &isa)) { >> >> - pr_warn("Unable to find \"riscv,isa\" devicetree entry\n"); >> >> - of_node_put(node); >> >> - return; >> >> - } >> >> - of_node_put(node); >> >> + if (riscv_of_processor_hartid(node) < 0) >> >> + continue; >> >> >> >> >> - for (i = 0; i < strlen(isa); ++i) >> >> - elf_hwcap |= isa2hwcap[(unsigned char)(isa[i])]; >> >> + if (of_property_read_string(node, "riscv,isa", &isa)) { >> >> + pr_warn("Unable to find \"riscv,isa\" devicetree entry\n"); >> >> + return; >> > >> > Did you want "continue" here to continue processing the other harts? >> >> Hmm. If a cpu node doesn't have isa in DT, that means DT is wrong. A >> "continue" here will let user space use other harts just with a warning >> message? >> >> Returning here will not set elf_hwcap which forces the user to fix the >> DT. I am not sure what should be the defined behavior in this case. >> >> Any thoughts ? > > The problem is that the proposed code might still set elf_hwcap -- it > all depends on the order of the hart nodes in dt (i.e. it will only be > left unset if the first node is malformed). > > For that reason, I'd say it's better to either bail out (hard or at > least with elf_hwcap unset) or to continue processing the other nodes. > > The former might break current systems with malformed dt, though. > > And since the harts are expected to have the same ISA, continuing the > processing while warning and ignoring the malformed node might be > acceptable.
Handling malformed device trees by providing a warning and an empty HWCAP seems like the right way to go to me.
> > Johan
| |