From: Atish Patra <atish.patra@wdc.com> To: Palmer Dabbelt <palmer@sifive.com>, "johan@kernel.org" <johan@kernel.org> Cc: "robh@kernel.org" <robh@kernel.org>, "aou@eecs.berkeley.edu" <aou@eecs.berkeley.edu>, "jason@lakedaemon.net" <jason@lakedaemon.net>, "alankao@andestech.com" <alankao@andestech.com>, "dmitriy@oss-tech.org" <dmitriy@oss-tech.org>, "schwab@suse.de" <schwab@suse.de>, "daniel.lezcano@linaro.org" <daniel.lezcano@linaro.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "marc.zyngier@arm.com" <marc.zyngier@arm.com>, Paul Walmsley <paul.walmsley@sifive.com>, "anup@brainfault.org" <anup@brainfault.org>, "linux-riscv@lists.infradead.org" <linux-riscv@lists.infradead.org>, "tglx@linutronix.de" <tglx@linutronix.de> Subject: Re: [v4 PATCH 8/8] RISC-V: Assign hwcap as per comman capabilities. Date: Thu, 14 Feb 2019 15:49:09 -0800 [thread overview] Message-ID: <e3d0cb05-268a-6148-7723-fe6b94d96583@wdc.com> (raw) In-Reply-To: <mhng-d7d9894c-d6f6-485b-95e8-962c4ab0f3f1@palmer-si-x1c4> 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 >
WARNING: multiple messages have this Message-ID (diff)
From: Atish Patra <atish.patra@wdc.com> To: Palmer Dabbelt <palmer@sifive.com>, "johan@kernel.org" <johan@kernel.org> Cc: "robh@kernel.org" <robh@kernel.org>, "aou@eecs.berkeley.edu" <aou@eecs.berkeley.edu>, "jason@lakedaemon.net" <jason@lakedaemon.net>, "alankao@andestech.com" <alankao@andestech.com>, "dmitriy@oss-tech.org" <dmitriy@oss-tech.org>, "schwab@suse.de" <schwab@suse.de>, "anup@brainfault.org" <anup@brainfault.org>, "daniel.lezcano@linaro.org" <daniel.lezcano@linaro.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "marc.zyngier@arm.com" <marc.zyngier@arm.com>, Paul Walmsley <paul.walmsley@sifive.com>, "linux-riscv@lists.infradead.org" <linux-riscv@lists.infradead.org>, "tglx@linutronix.de" <tglx@linutronix.de> Subject: Re: [v4 PATCH 8/8] RISC-V: Assign hwcap as per comman capabilities. Date: Thu, 14 Feb 2019 15:49:09 -0800 [thread overview] Message-ID: <e3d0cb05-268a-6148-7723-fe6b94d96583@wdc.com> (raw) In-Reply-To: <mhng-d7d9894c-d6f6-485b-95e8-962c4ab0f3f1@palmer-si-x1c4> 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 > _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv
next prev parent reply other threads:[~2019-02-14 23:49 UTC|newest] Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-02-12 11:10 [v4 PATCH 0/8] Various SMP related fixes Atish Patra 2019-02-12 11:10 ` Atish Patra 2019-02-12 11:10 ` [v4 PATCH 1/8] RISC-V: Do not wait indefinitely in __cpu_up Atish Patra 2019-02-12 11:10 ` Atish Patra 2019-02-12 11:10 ` [v4 PATCH 2/8] RISC-V: Move cpuid to hartid mapping to SMP Atish Patra 2019-02-12 11:10 ` Atish Patra 2019-02-12 11:10 ` [v4 PATCH 3/8] RISC-V: Remove NR_CPUs check during hartid search from DT Atish Patra 2019-02-12 11:10 ` Atish Patra 2019-02-12 11:10 ` [v4 PATCH 4/8] RISC-V: Allow hartid-to-cpuid function to fail Atish Patra 2019-02-12 11:10 ` Atish Patra 2019-02-12 11:10 ` [v4 PATCH 5/8] RISC-V: Compare cpuid with NR_CPUS before mapping Atish Patra 2019-02-12 11:10 ` Atish Patra 2019-02-12 11:10 ` [v4 PATCH 6/8] clocksource/drivers/riscv: Add required checks during clock source init Atish Patra 2019-02-12 11:10 ` Atish Patra 2019-02-13 6:43 ` Anup Patel 2019-02-13 6:43 ` Anup Patel 2019-02-13 8:48 ` Daniel Lezcano 2019-02-13 8:48 ` Daniel Lezcano 2019-02-14 0:37 ` Palmer Dabbelt 2019-02-14 0:37 ` Palmer Dabbelt 2019-02-12 11:10 ` [v4 PATCH 7/8] irqchip/irq-sifive-plic: Check and continue in case of an invalid cpuid Atish Patra 2019-02-12 11:10 ` Atish Patra 2019-02-14 12:24 ` Marc Zyngier 2019-02-14 12:24 ` Marc Zyngier 2019-02-12 11:10 ` [v4 PATCH 8/8] RISC-V: Assign hwcap as per comman capabilities Atish Patra 2019-02-12 11:10 ` Atish Patra 2019-02-12 11:25 ` Johan Hovold 2019-02-12 11:25 ` Johan Hovold 2019-02-12 19:58 ` Atish Patra 2019-02-12 19:58 ` Atish Patra 2019-02-13 8:44 ` Johan Hovold 2019-02-13 8:44 ` Johan Hovold 2019-02-13 19:59 ` Atish Patra 2019-02-13 19:59 ` Atish Patra 2019-02-14 0:37 ` Palmer Dabbelt 2019-02-14 0:37 ` Palmer Dabbelt 2019-02-14 23:49 ` Atish Patra [this message] 2019-02-14 23:49 ` Atish Patra 2019-02-22 19:21 ` Atish Patra 2019-02-22 19:21 ` Atish Patra
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=e3d0cb05-268a-6148-7723-fe6b94d96583@wdc.com \ --to=atish.patra@wdc.com \ --cc=alankao@andestech.com \ --cc=anup@brainfault.org \ --cc=aou@eecs.berkeley.edu \ --cc=daniel.lezcano@linaro.org \ --cc=dmitriy@oss-tech.org \ --cc=jason@lakedaemon.net \ --cc=johan@kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-riscv@lists.infradead.org \ --cc=marc.zyngier@arm.com \ --cc=palmer@sifive.com \ --cc=paul.walmsley@sifive.com \ --cc=robh@kernel.org \ --cc=schwab@suse.de \ --cc=tglx@linutronix.de \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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.