| Message ID | 20190807182316.28013-1-atish.patra@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v5] RISC-V: Fix unsupported isa string info. | expand |
> + for (e = mandatory_ext; *e != '\0'; ++e) { > + if (isa[0] != e[0]) { > +#if defined(CONFIG_FP) > + if ((isa[0] == 'f') || (isa[0] == 'd')) > + continue; > +#endif > + unsupported_isa[index] = e[0]; > + index++; > + } I'd just use if (IS_ENABLED()) here to get full compiler coverage. Also no need for the inner braces. > + if (isa[0] != '\0') { > + /* Add remainging isa strings */ > + for (e = isa; *e != '\0'; ++e) { > +#if !defined(CONFIG_VIRTUALIZATION) > + if (e[0] != 'h') > +#endif > + seq_write(f, e, 1); > + } > + } This one I don't get. Why do we want to check CONFIG_VIRTUALIZATION? > seq_puts(f, "\n"); > > /* > * If we were given an unsupported ISA in the device tree then print > * a bit of info describing what went wrong. > */ > - if (isa[0] != '\0') > - pr_info("unsupported ISA \"%s\" in device tree\n", orig_isa); > + if (unsupported_isa[0]) > + pr_info("unsupported ISA extensions \"%s\" in device tree for cpu [%ld]\n", > + unsupported_isa, cpuid); And I'm not even sure why we care about unsupported extensions. Sooner or late a few will op up and they should be harmless.
On Mon, 2019-08-12 at 08:02 -0700, Christoph Hellwig wrote: > > + for (e = mandatory_ext; *e != '\0'; ++e) { > > + if (isa[0] != e[0]) { > > +#if defined(CONFIG_FP) > > + if ((isa[0] == 'f') || (isa[0] == 'd')) > > + continue; > > +#endif > > + unsupported_isa[index] = e[0]; > > + index++; > > + } > > I'd just use if (IS_ENABLED()) here to get full compiler coverage. > Also no need for the inner braces. > Sure. I will do that. > > + if (isa[0] != '\0') { > > + /* Add remainging isa strings */ > > + for (e = isa; *e != '\0'; ++e) { > > +#if !defined(CONFIG_VIRTUALIZATION) > > + if (e[0] != 'h') > > +#endif > > + seq_write(f, e, 1); > > + } > > + } > > This one I don't get. Why do we want to check CONFIG_VIRTUALIZATION? > If CONFIG_VIRTUALIZATION is not enabled, it shouldn't print that hypervisor extension "h" in isa extensions. This can be extended to any other future extensions and related config. > > seq_puts(f, "\n");ther you want to know if a specific extension > > is enabled > > > > /* > > * If we were given an unsupported ISA in the device tree then > > print > > * a bit of info describing what went wrong. > > */ > > - if (isa[0] != '\0') > > - pr_info("unsupported ISA \"%s\" in device tree\n", > > orig_isa); > > + if (unsupported_isa[0]) > > + pr_info("unsupported ISA extensions \"%s\" in device > > tree for cpu [%ld]\n", > > + unsupported_isa, cpuid); > > And I'm not even sure why we care about unsupported > extensions. Sooner > or late a few will op up and they should be harmless. This is just an information to the userspace that some of the mandatory ISA extensions ("mafdcsu") are not supported in kernel which may lead to undesirable results. Regards, Atish
On Fri, Aug 16, 2019 at 07:21:52PM +0000, Atish Patra wrote: > > > + if (isa[0] != '\0') { > > > + /* Add remainging isa strings */ > > > + for (e = isa; *e != '\0'; ++e) { > > > +#if !defined(CONFIG_VIRTUALIZATION) > > > + if (e[0] != 'h') > > > +#endif > > > + seq_write(f, e, 1); > > > + } > > > + } > > > > This one I don't get. Why do we want to check CONFIG_VIRTUALIZATION? > > > > If CONFIG_VIRTUALIZATION is not enabled, it shouldn't print that > hypervisor extension "h" in isa extensions. CONFIG_VIRTUALIZATION doesn't change anything in the kernels capabilities, it just enables other config options. But more importantly the 'h' extension is only relevant for S-mode software anyway. > This is just an information to the userspace that some of the mandatory > ISA extensions ("mafdcsu") are not supported in kernel which may lead > to undesirable results. I think we need to sit down decide what the purpose of /proc/cpuinfo is. IIRC on other architectures is just prints what the hardware supports, not what you can actually make use of. How else would you find out that you'd need to enable more kernel options to fully utilize the hardware? Also printing this warning to the kernel log when someone reads the procfs file is very strange.
On 8/7/19, Atish Patra <atish.patra@wdc.com> wrote: > } > } > + if (isa[0] != '\0') { > + /* Add remainging isa strings */ That should be spelled "remaining" > + for (e = isa; *e != '\0'; ++e) { > +#if !defined(CONFIG_VIRTUALIZATION) > + if (e[0] != 'h') > +#endif > + seq_write(f, e, 1); > + } > + } > seq_puts(f, "\n"); Jacob Lifshay
On Sun, 2019-08-18 at 11:16 -0700, hch@infradead.org wrote: > On Fri, Aug 16, 2019 at 07:21:52PM +0000, Atish Patra wrote: > > > > + if (isa[0] != '\0') { > > > > + /* Add remainging isa strings */ > > > > + for (e = isa; *e != '\0'; ++e) { > > > > +#if !defined(CONFIG_VIRTUALIZATION) > > > > + if (e[0] != 'h') > > > > +#endif > > > > + seq_write(f, e, 1); > > > > + } > > > > + } > > > > > > This one I don't get. Why do we want to check > > > CONFIG_VIRTUALIZATION? > > > > > > > If CONFIG_VIRTUALIZATION is not enabled, it shouldn't print that > > hypervisor extension "h" in isa extensions. > > CONFIG_VIRTUALIZATION doesn't change anything in the kernels > capabilities, it just enables other config options. Yes. But it must be enabled if virtualization is supported in kernel. The idea was to let userspace know that if virtualization can be used or not. > But more > importantly the 'h' extension is only relevant for S-mode software > anyway. > Do you think we should just print all the extensions available in isa string as it is ? > > This is just an information to the userspace that some of the > > mandatory > > ISA extensions ("mafdcsu") are not supported in kernel which may > > lead > > to undesirable results. > > I think we need to sit down decide what the purpose of /proc/cpuinfo > is. IIRC on other architectures is just prints what the hardware > supports, not what you can actually make use of. How else would you > find out that you'd need to enable more kernel options to fully > utilize the hardware? > > Also printing this warning to the kernel log when someone reads the > procfs file is very strange. Agreed. May be something like this ? Let's say f/d is enabled in kernel but cpu doesn't support it. "unsupported isa" will only appear if there are any unsupported isa. processor : 3 hart : 4 isa : rv64imac unsupported isa : fd mmu : sv39 uarch : sifive,u54-mc May be I am just trying over optimize one corner case :) :). /proc/cpuinfo should just print all the isa string. That's it. Regards, Atish
On Tue, 2019-08-20 at 00:59 -0700, Atish Patra wrote: > On Sun, 2019-08-18 at 11:16 -0700, hch@infradead.org wrote: > > On Fri, Aug 16, 2019 at 07:21:52PM +0000, Atish Patra wrote: > > > > > + if (isa[0] != '\0') { > > > > > + /* Add remainging isa strings */ > > > > > + for (e = isa; *e != '\0'; ++e) { > > > > > +#if !defined(CONFIG_VIRTUALIZATION) > > > > > + if (e[0] != 'h') > > > > > +#endif > > > > > + seq_write(f, e, 1); > > > > > + } > > > > > + } > > > > > > > > This one I don't get. Why do we want to check > > > > CONFIG_VIRTUALIZATION? > > > > > > > > > > If CONFIG_VIRTUALIZATION is not enabled, it shouldn't print that > > > hypervisor extension "h" in isa extensions. > > > > CONFIG_VIRTUALIZATION doesn't change anything in the kernels > > capabilities, it just enables other config options. > > Yes. But it must be enabled if virtualization is supported in kernel. > The idea was to let userspace know that if virtualization can be used > or not. > > > > But more > > importantly the 'h' extension is only relevant for S-mode software > > anyway. > > > > Do you think we should just print all the extensions available in isa > string as it is ? > > > > This is just an information to the userspace that some of the > > > mandatory > > > ISA extensions ("mafdcsu") are not supported in kernel which may > > > lead > > > to undesirable results. > > > > I think we need to sit down decide what the purpose of > > /proc/cpuinfo > > is. IIRC on other architectures is just prints what the hardware > > supports, not what you can actually make use of. How else would > > you > > find out that you'd need to enable more kernel options to fully > > utilize the hardware? > > > > Also printing this warning to the kernel log when someone reads the > > procfs file is very strange. > > Agreed. May be something like this ? > > Let's say f/d is enabled in kernel but cpu doesn't support it. > "unsupported isa" will only appear if there are any unsupported isa. > > processor : 3 > hart : 4 > isa : rv64imac > unsupported isa : fd > mmu : sv39 > uarch : sifive,u54-mc > > May be I am just trying over optimize one corner case :) :). > /proc/cpuinfo should just print all the isa string. That's it. > Ping ? > Regards, > Atish
On Fri, Sep 06, 2019 at 11:27:57PM +0000, Atish Patra wrote: > > Agreed. May be something like this ? > > > > Let's say f/d is enabled in kernel but cpu doesn't support it. > > "unsupported isa" will only appear if there are any unsupported isa. > > > > processor : 3 > > hart : 4 > > isa : rv64imac > > unsupported isa : fd > > mmu : sv39 > > uarch : sifive,u54-mc > > > > May be I am just trying over optimize one corner case :) :). > > /proc/cpuinfo should just print all the isa string. That's it. > > > > Ping ? Yes, I agree with the "dumb" reporting of all capabilities.
On Mon, 09 Sep 2019 23:00:10 PDT (-0700), Christoph Hellwig wrote: > On Fri, Sep 06, 2019 at 11:27:57PM +0000, Atish Patra wrote: >> > Agreed. May be something like this ? >> > >> > Let's say f/d is enabled in kernel but cpu doesn't support it. >> > "unsupported isa" will only appear if there are any unsupported isa. >> > >> > processor : 3 >> > hart : 4 >> > isa : rv64imac >> > unsupported isa : fd >> > mmu : sv39 >> > uarch : sifive,u54-mc >> > >> > May be I am just trying over optimize one corner case :) :). >> > /proc/cpuinfo should just print all the isa string. That's it. >> > >> >> Ping ? > > Yes, I agree with the "dumb" reporting of all capabilities. I agree: it looks like other architectures are passing info (ie, x86 flags) that isn't supported by userspace. We have the ELF hwcap stuff for those who want to tell which instructions they can run, so it's sane to just keep this simple.
On Fri, 2019-09-13 at 13:47 -0700, Palmer Dabbelt wrote: > On Mon, 09 Sep 2019 23:00:10 PDT (-0700), Christoph Hellwig wrote: > > On Fri, Sep 06, 2019 at 11:27:57PM +0000, Atish Patra wrote: > > > > Agreed. May be something like this ? > > > > > > > > Let's say f/d is enabled in kernel but cpu doesn't support it. > > > > "unsupported isa" will only appear if there are any unsupported > > > > isa. > > > > > > > > processor : 3 > > > > hart : 4 > > > > isa : rv64imac > > > > unsupported isa : fd > > > > mmu : sv39 > > > > uarch : sifive,u54-mc > > > > > > > > May be I am just trying over optimize one corner case :) :). > > > > /proc/cpuinfo should just print all the isa string. That's it. > > > > > > > > > > Ping ? > > > > Yes, I agree with the "dumb" reporting of all capabilities. > > I agree: it looks like other architectures are passing info (ie, x86 > flags) > that isn't supported by userspace. We have the ELF hwcap stuff for > those who > want to tell which instructions they can run, so it's sane to just > keep this > simple. Great. That will simplify the code path as well. I will send a v6 with the fixes.
diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c index 7da3c6a93abd..e521142e8ac3 100644 --- a/arch/riscv/kernel/cpu.c +++ b/arch/riscv/kernel/cpu.c @@ -7,6 +7,7 @@ #include <linux/seq_file.h> #include <linux/of.h> #include <asm/smp.h> +#include <asm/hwcap.h> /* * Returns the hart ID of the given device tree node, or -ENODEV if the node @@ -46,11 +47,14 @@ int riscv_of_processor_hartid(struct device_node *node) #ifdef CONFIG_PROC_FS -static void print_isa(struct seq_file *f, const char *orig_isa) +static void print_isa(struct seq_file *f, const char *orig_isa, + unsigned long cpuid) { - static const char *ext = "mafdcsu"; + static const char *mandatory_ext = "mafdc"; const char *isa = orig_isa; const char *e; + char unsupported_isa[26] = {0}; + int index = 0; /* * Linux doesn't support rv32e or rv128i, and we only support booting @@ -70,27 +74,44 @@ static void print_isa(struct seq_file *f, const char *orig_isa) isa += 5; /* - * Check the rest of the ISA string for valid extensions, printing those - * we find. RISC-V ISA strings define an order, so we only print the - * extension bits when they're in order. Hide the supervisor (S) - * extension from userspace as it's not accessible from there. + * RISC-V ISA strings define an order, so we only print all the + * extension bits when they're in order. Throw a warning only if + * any mandatory extensions are not available and kernel is + * configured to have that mandatory extensions. */ - for (e = ext; *e != '\0'; ++e) { - if (isa[0] == e[0]) { - if (isa[0] != 's') - seq_write(f, isa, 1); - + for (e = mandatory_ext; *e != '\0'; ++e) { + if (isa[0] != e[0]) { +#if defined(CONFIG_FP) + if ((isa[0] == 'f') || (isa[0] == 'd')) + continue; +#endif + unsupported_isa[index] = e[0]; + index++; + } + /* Only write if part of isa string */ + if (isa[0] != '\0') { + seq_write(f, isa, 1); isa++; } } + if (isa[0] != '\0') { + /* Add remainging isa strings */ + for (e = isa; *e != '\0'; ++e) { +#if !defined(CONFIG_VIRTUALIZATION) + if (e[0] != 'h') +#endif + seq_write(f, e, 1); + } + } seq_puts(f, "\n"); /* * If we were given an unsupported ISA in the device tree then print * a bit of info describing what went wrong. */ - if (isa[0] != '\0') - pr_info("unsupported ISA \"%s\" in device tree\n", orig_isa); + if (unsupported_isa[0]) + pr_info("unsupported ISA extensions \"%s\" in device tree for cpu [%ld]\n", + unsupported_isa, cpuid); } static void print_mmu(struct seq_file *f, const char *mmu_type) @@ -134,7 +155,7 @@ static int c_show(struct seq_file *m, void *v) seq_printf(m, "processor\t: %lu\n", cpu_id); seq_printf(m, "hart\t\t: %lu\n", cpuid_to_hartid_map(cpu_id)); if (!of_property_read_string(node, "riscv,isa", &isa)) - print_isa(m, isa); + print_isa(m, isa, cpu_id); if (!of_property_read_string(node, "mmu-type", &mmu)) print_mmu(m, mmu); if (!of_property_read_string(node, "compatible", &compat)
Currently, kernel prints a info warning if any of the extensions from "mafdcsu" is missing in device tree. Here are few issues with this approach. 1. It is not entirely correct as Linux can boot with "f or d" extensions if kernel is configured accordingly. 2. It will continue to print the info string for future extensions such as hypervisor as well which is completely misleading. 3. As per the RISC-V user level specification, S extension can not exist in single letter extensions and must use multi-letter strings. There are no U extension defined at all. 4. /proc/cpuinfo also doesn't print any other extensions except "mafdcsu". Fix this by making sure that info log is only printed only if kernel is configured to have any mandatory extensions but device tree doesn't describe it. All the extensions present in device tree and follow the order described in the RISC-V specification are printed via /proc/cpuinfo always. Signed-off-by: Atish Patra <atish.patra@wdc.com> --- arch/riscv/kernel/cpu.c | 49 +++++++++++++++++++++++++++++------------ 1 file changed, 35 insertions(+), 14 deletions(-)