Messages in this thread | | | Subject | Re: [v4 PATCH 8/8] RISC-V: Assign hwcap as per comman capabilities. | From | Atish Patra <> | Date | Thu, 14 Feb 2019 15:49:09 -0800 |
| |
On 2/13/19 4:38 PM, Palmer Dabbelt wrote: > 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. >
If I understand you correctly, you prefer following things to be done in case of malformed DT.
1. Print a warning message 2. Unset the entire HWCAP 3. Return without processing other harts. This will most likely result in panic when user space starts.
Is this correct ?
Regards, Atish >> >> Johan >
| |