| Message ID | 20230705172931.1099183-1-evan@rivosinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] RISC-V: Show accurate per-hart isa in /proc/cpuinfo | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/tree_selection | fail | Failed to apply to next/pending-fixes, riscv/for-next or riscv/master |
On Wed, Jul 05, 2023 at 10:29:31AM -0700, Evan Green wrote: > In /proc/cpuinfo, most of the information we show for each processor is > specific to that hart: marchid, mvendorid, mimpid, processor, hart, > compatible, and the mmu size. But the ISA string gets filtered through a > lowest common denominator mask, so that if one CPU is missing an ISA > extension, no CPUs will show it. > > Now that we track the ISA extensions for each hart, let's report ISA > extension info accurately per-hart in /proc/cpuinfo. We cannot change > the "isa:" line, as usermode may be relying on that line to show only > the common set of extensions supported across all harts. Add a new "hart > isa" line instead, which reports the true set of extensions for that > hart. This matches what is returned in riscv_hwprobe() when querying a > given hart. > > Signed-off-by: Evan Green <evan@rivosinc.com> > > --- > > Changes in v2: > - Added new "hart isa" line rather than altering behavior of existing > "isa" line (Conor, Palmer) > > > I based this series on top of Conor's riscv-extensions-strings branch > from July 3rd, since otherwise this change gets hopelessly entangled > with that series. > > I was unsure if I could snuggle the new "hart isa" line in just below > "isa". Aesthetically it would be quite pleasing, but it runs the risk of > breaking brittle usermode parsers that are assuming a specific line > order. So I put it at the end. Actually, they're probably only aesthetically pleasing when they match. If there are differences, then I'd guess having them side by side, almost the same, but different, would make them even harder to look at then they already are. So I think I'll be happier with them separated by a few lines anyway. > > --- > arch/riscv/kernel/cpu.c | 22 ++++++++++++++++++---- > 1 file changed, 18 insertions(+), 4 deletions(-) > > diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c > index 1acf3679600d..6264b7b94945 100644 > --- a/arch/riscv/kernel/cpu.c > +++ b/arch/riscv/kernel/cpu.c > @@ -197,9 +197,8 @@ arch_initcall(riscv_cpuinfo_init); > > #ifdef CONFIG_PROC_FS > > -static void print_isa(struct seq_file *f) > +static void print_isa(struct seq_file *f, const unsigned long *isa_bitmap) > { > - seq_puts(f, "isa\t\t: "); > > if (IS_ENABLED(CONFIG_32BIT)) > seq_write(f, "rv32", 4); > @@ -207,7 +206,7 @@ static void print_isa(struct seq_file *f) > seq_write(f, "rv64", 4); > > for (int i = 0; i < riscv_isa_ext_count; i++) { > - if (!__riscv_isa_extension_available(NULL, riscv_isa_ext[i].id)) > + if (!__riscv_isa_extension_available(isa_bitmap, riscv_isa_ext[i].id)) > continue; > > /* Only multi-letter extensions are split by underscores */ > @@ -271,7 +270,15 @@ 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)); > - print_isa(m); > + > + /* > + * For historical raisins, the isa: line is limited to the lowest common > + * denominator of extensions supported across all harts. A true list of > + * extensions supported on this hart is printed later in the hart_isa: > + * line. > + */ > + seq_puts(m, "isa\t\t: "); > + print_isa(m, NULL); > print_mmu(m); > > if (acpi_disabled) { > @@ -287,6 +294,13 @@ static int c_show(struct seq_file *m, void *v) > seq_printf(m, "mvendorid\t: 0x%lx\n", ci->mvendorid); > seq_printf(m, "marchid\t\t: 0x%lx\n", ci->marchid); > seq_printf(m, "mimpid\t\t: 0x%lx\n", ci->mimpid); > + > + /* > + * Print the ISA extensions specific to this hart, which may show > + * additional extensions not present across all harts. > + */ > + seq_puts(m, "hart isa\t: "); > + print_isa(m, hart_isa[cpu_id].isa); > seq_puts(m, "\n"); > > return 0; > -- > 2.34.1 > Reviewed-by: Andrew Jones <ajones@ventanamicro.com> Thanks, drew
On Thu, Jul 06, 2023 at 10:01:31AM +0200, Andrew Jones wrote: > On Wed, Jul 05, 2023 at 10:29:31AM -0700, Evan Green wrote: > > In /proc/cpuinfo, most of the information we show for each processor is > > specific to that hart: marchid, mvendorid, mimpid, processor, hart, > > compatible, and the mmu size. But the ISA string gets filtered through a > > lowest common denominator mask, so that if one CPU is missing an ISA > > extension, no CPUs will show it. > > > > Now that we track the ISA extensions for each hart, let's report ISA > > extension info accurately per-hart in /proc/cpuinfo. We cannot change > > the "isa:" line, as usermode may be relying on that line to show only > > the common set of extensions supported across all harts. Add a new "hart > > isa" line instead, which reports the true set of extensions for that > > hart. This matches what is returned in riscv_hwprobe() when querying a > > given hart. > > > > Signed-off-by: Evan Green <evan@rivosinc.com> > > > > --- > > > > Changes in v2: > > - Added new "hart isa" line rather than altering behavior of existing > > "isa" line (Conor, Palmer) > > > > > > I based this series on top of Conor's riscv-extensions-strings branch > > from July 3rd, since otherwise this change gets hopelessly entangled > > with that series. > > > > I was unsure if I could snuggle the new "hart isa" line in just below > > "isa". Aesthetically it would be quite pleasing, but it runs the risk of > > breaking brittle usermode parsers that are assuming a specific line > > order. So I put it at the end. > > Actually, they're probably only aesthetically pleasing when they match. If > there are differences, then I'd guess having them side by side, almost the > same, but different, would make them even harder to look at then they > already are. So I think I'll be happier with them separated by a few lines > anyway. This list is eventually going to be so big that I don't think doing by-eye anything is going to be useful, so aesthetics be damned. That said, a parser that relies on the order of individual lines like that might deserve to be broken ;) Anyway, change looks good to me: Reviewed-by: Conor Dooley <conor.dooley@microchip.com> I was thinking the uabi doc might need an update - should we add to it that "isa" means the common set & "hart isa"? Cheers, Conor. > > --- > > arch/riscv/kernel/cpu.c | 22 ++++++++++++++++++---- > > 1 file changed, 18 insertions(+), 4 deletions(-) > > > > diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c > > index 1acf3679600d..6264b7b94945 100644 > > --- a/arch/riscv/kernel/cpu.c > > +++ b/arch/riscv/kernel/cpu.c > > @@ -197,9 +197,8 @@ arch_initcall(riscv_cpuinfo_init); > > > > #ifdef CONFIG_PROC_FS > > > > -static void print_isa(struct seq_file *f) > > +static void print_isa(struct seq_file *f, const unsigned long *isa_bitmap) > > { > > - seq_puts(f, "isa\t\t: "); > > > > if (IS_ENABLED(CONFIG_32BIT)) > > seq_write(f, "rv32", 4); > > @@ -207,7 +206,7 @@ static void print_isa(struct seq_file *f) > > seq_write(f, "rv64", 4); > > > > for (int i = 0; i < riscv_isa_ext_count; i++) { > > - if (!__riscv_isa_extension_available(NULL, riscv_isa_ext[i].id)) > > + if (!__riscv_isa_extension_available(isa_bitmap, riscv_isa_ext[i].id)) > > continue; > > > > /* Only multi-letter extensions are split by underscores */ > > @@ -271,7 +270,15 @@ 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)); > > - print_isa(m); > > + > > + /* > > + * For historical raisins, the isa: line is limited to the lowest common > > + * denominator of extensions supported across all harts. A true list of > > + * extensions supported on this hart is printed later in the hart_isa: > > + * line. > > + */ > > + seq_puts(m, "isa\t\t: "); > > + print_isa(m, NULL); > > print_mmu(m); > > > > if (acpi_disabled) { > > @@ -287,6 +294,13 @@ static int c_show(struct seq_file *m, void *v) > > seq_printf(m, "mvendorid\t: 0x%lx\n", ci->mvendorid); > > seq_printf(m, "marchid\t\t: 0x%lx\n", ci->marchid); > > seq_printf(m, "mimpid\t\t: 0x%lx\n", ci->mimpid); > > + > > + /* > > + * Print the ISA extensions specific to this hart, which may show > > + * additional extensions not present across all harts. > > + */ > > + seq_puts(m, "hart isa\t: "); > > + print_isa(m, hart_isa[cpu_id].isa); > > seq_puts(m, "\n"); > > > > return 0; > > -- > > 2.34.1 > >
On Thu, Jul 6, 2023 at 3:53 AM Conor Dooley <conor.dooley@microchip.com> wrote: > > On Thu, Jul 06, 2023 at 10:01:31AM +0200, Andrew Jones wrote: > > On Wed, Jul 05, 2023 at 10:29:31AM -0700, Evan Green wrote: > > > In /proc/cpuinfo, most of the information we show for each processor is > > > specific to that hart: marchid, mvendorid, mimpid, processor, hart, > > > compatible, and the mmu size. But the ISA string gets filtered through a > > > lowest common denominator mask, so that if one CPU is missing an ISA > > > extension, no CPUs will show it. > > > > > > Now that we track the ISA extensions for each hart, let's report ISA > > > extension info accurately per-hart in /proc/cpuinfo. We cannot change > > > the "isa:" line, as usermode may be relying on that line to show only > > > the common set of extensions supported across all harts. Add a new "hart > > > isa" line instead, which reports the true set of extensions for that > > > hart. This matches what is returned in riscv_hwprobe() when querying a > > > given hart. > > > > > > Signed-off-by: Evan Green <evan@rivosinc.com> > > > > > > --- > > > > > > Changes in v2: > > > - Added new "hart isa" line rather than altering behavior of existing > > > "isa" line (Conor, Palmer) > > > > > > > > > I based this series on top of Conor's riscv-extensions-strings branch > > > from July 3rd, since otherwise this change gets hopelessly entangled > > > with that series. > > > > > > I was unsure if I could snuggle the new "hart isa" line in just below > > > "isa". Aesthetically it would be quite pleasing, but it runs the risk of > > > breaking brittle usermode parsers that are assuming a specific line > > > order. So I put it at the end. > > > > Actually, they're probably only aesthetically pleasing when they match. If > > there are differences, then I'd guess having them side by side, almost the > > same, but different, would make them even harder to look at then they > > already are. So I think I'll be happier with them separated by a few lines > > anyway. > > This list is eventually going to be so big that I don't think doing > by-eye anything is going to be useful, so aesthetics be damned. > That said, a parser that relies on the order of individual lines like > that might deserve to be broken ;) > > Anyway, change looks good to me: > Reviewed-by: Conor Dooley <conor.dooley@microchip.com> > > I was thinking the uabi doc might need an update - should we add to it > that "isa" means the common set & "hart isa"? I did do a little search for documentation to update, but failed to come up with any. Which doc are you thinking of? -Evan
On Thu, 06 Jul 2023 12:34:30 PDT (-0700), Evan Green wrote: > On Thu, Jul 6, 2023 at 3:53 AM Conor Dooley <conor.dooley@microchip.com> wrote: >> >> On Thu, Jul 06, 2023 at 10:01:31AM +0200, Andrew Jones wrote: >> > On Wed, Jul 05, 2023 at 10:29:31AM -0700, Evan Green wrote: >> > > In /proc/cpuinfo, most of the information we show for each processor is >> > > specific to that hart: marchid, mvendorid, mimpid, processor, hart, >> > > compatible, and the mmu size. But the ISA string gets filtered through a >> > > lowest common denominator mask, so that if one CPU is missing an ISA >> > > extension, no CPUs will show it. >> > > >> > > Now that we track the ISA extensions for each hart, let's report ISA >> > > extension info accurately per-hart in /proc/cpuinfo. We cannot change >> > > the "isa:" line, as usermode may be relying on that line to show only >> > > the common set of extensions supported across all harts. Add a new "hart >> > > isa" line instead, which reports the true set of extensions for that >> > > hart. This matches what is returned in riscv_hwprobe() when querying a >> > > given hart. >> > > >> > > Signed-off-by: Evan Green <evan@rivosinc.com> >> > > >> > > --- >> > > >> > > Changes in v2: >> > > - Added new "hart isa" line rather than altering behavior of existing >> > > "isa" line (Conor, Palmer) >> > > >> > > >> > > I based this series on top of Conor's riscv-extensions-strings branch >> > > from July 3rd, since otherwise this change gets hopelessly entangled >> > > with that series. >> > > >> > > I was unsure if I could snuggle the new "hart isa" line in just below >> > > "isa". Aesthetically it would be quite pleasing, but it runs the risk of >> > > breaking brittle usermode parsers that are assuming a specific line >> > > order. So I put it at the end. >> > >> > Actually, they're probably only aesthetically pleasing when they match. If >> > there are differences, then I'd guess having them side by side, almost the >> > same, but different, would make them even harder to look at then they >> > already are. So I think I'll be happier with them separated by a few lines >> > anyway. >> >> This list is eventually going to be so big that I don't think doing >> by-eye anything is going to be useful, so aesthetics be damned. >> That said, a parser that relies on the order of individual lines like >> that might deserve to be broken ;) >> >> Anyway, change looks good to me: >> Reviewed-by: Conor Dooley <conor.dooley@microchip.com> >> >> I was thinking the uabi doc might need an update - should we add to it >> that "isa" means the common set & "hart isa"? > > I did do a little search for documentation to update, but failed to > come up with any. Which doc are you thinking of? Should be somewhere like Documentation/arch/riscv/uabi.rst
On Thu, Jul 6, 2023 at 12:45 PM Palmer Dabbelt <palmer@dabbelt.com> wrote: > > On Thu, 06 Jul 2023 12:34:30 PDT (-0700), Evan Green wrote: > > On Thu, Jul 6, 2023 at 3:53 AM Conor Dooley <conor.dooley@microchip.com> wrote: > >> > >> On Thu, Jul 06, 2023 at 10:01:31AM +0200, Andrew Jones wrote: > >> > On Wed, Jul 05, 2023 at 10:29:31AM -0700, Evan Green wrote: > >> > > In /proc/cpuinfo, most of the information we show for each processor is > >> > > specific to that hart: marchid, mvendorid, mimpid, processor, hart, > >> > > compatible, and the mmu size. But the ISA string gets filtered through a > >> > > lowest common denominator mask, so that if one CPU is missing an ISA > >> > > extension, no CPUs will show it. > >> > > > >> > > Now that we track the ISA extensions for each hart, let's report ISA > >> > > extension info accurately per-hart in /proc/cpuinfo. We cannot change > >> > > the "isa:" line, as usermode may be relying on that line to show only > >> > > the common set of extensions supported across all harts. Add a new "hart > >> > > isa" line instead, which reports the true set of extensions for that > >> > > hart. This matches what is returned in riscv_hwprobe() when querying a > >> > > given hart. > >> > > > >> > > Signed-off-by: Evan Green <evan@rivosinc.com> > >> > > > >> > > --- > >> > > > >> > > Changes in v2: > >> > > - Added new "hart isa" line rather than altering behavior of existing > >> > > "isa" line (Conor, Palmer) > >> > > > >> > > > >> > > I based this series on top of Conor's riscv-extensions-strings branch > >> > > from July 3rd, since otherwise this change gets hopelessly entangled > >> > > with that series. > >> > > > >> > > I was unsure if I could snuggle the new "hart isa" line in just below > >> > > "isa". Aesthetically it would be quite pleasing, but it runs the risk of > >> > > breaking brittle usermode parsers that are assuming a specific line > >> > > order. So I put it at the end. > >> > > >> > Actually, they're probably only aesthetically pleasing when they match. If > >> > there are differences, then I'd guess having them side by side, almost the > >> > same, but different, would make them even harder to look at then they > >> > already are. So I think I'll be happier with them separated by a few lines > >> > anyway. > >> > >> This list is eventually going to be so big that I don't think doing > >> by-eye anything is going to be useful, so aesthetics be damned. > >> That said, a parser that relies on the order of individual lines like > >> that might deserve to be broken ;) > >> > >> Anyway, change looks good to me: > >> Reviewed-by: Conor Dooley <conor.dooley@microchip.com> > >> > >> I was thinking the uabi doc might need an update - should we add to it > >> that "isa" means the common set & "hart isa"? > > > > I did do a little search for documentation to update, but failed to > > come up with any. Which doc are you thinking of? > > Should be somewhere like Documentation/arch/riscv/uabi.rst Thanks, I'll spin this patch and add something here. -Evan
diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c index 1acf3679600d..6264b7b94945 100644 --- a/arch/riscv/kernel/cpu.c +++ b/arch/riscv/kernel/cpu.c @@ -197,9 +197,8 @@ arch_initcall(riscv_cpuinfo_init); #ifdef CONFIG_PROC_FS -static void print_isa(struct seq_file *f) +static void print_isa(struct seq_file *f, const unsigned long *isa_bitmap) { - seq_puts(f, "isa\t\t: "); if (IS_ENABLED(CONFIG_32BIT)) seq_write(f, "rv32", 4); @@ -207,7 +206,7 @@ static void print_isa(struct seq_file *f) seq_write(f, "rv64", 4); for (int i = 0; i < riscv_isa_ext_count; i++) { - if (!__riscv_isa_extension_available(NULL, riscv_isa_ext[i].id)) + if (!__riscv_isa_extension_available(isa_bitmap, riscv_isa_ext[i].id)) continue; /* Only multi-letter extensions are split by underscores */ @@ -271,7 +270,15 @@ 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)); - print_isa(m); + + /* + * For historical raisins, the isa: line is limited to the lowest common + * denominator of extensions supported across all harts. A true list of + * extensions supported on this hart is printed later in the hart_isa: + * line. + */ + seq_puts(m, "isa\t\t: "); + print_isa(m, NULL); print_mmu(m); if (acpi_disabled) { @@ -287,6 +294,13 @@ static int c_show(struct seq_file *m, void *v) seq_printf(m, "mvendorid\t: 0x%lx\n", ci->mvendorid); seq_printf(m, "marchid\t\t: 0x%lx\n", ci->marchid); seq_printf(m, "mimpid\t\t: 0x%lx\n", ci->mimpid); + + /* + * Print the ISA extensions specific to this hart, which may show + * additional extensions not present across all harts. + */ + seq_puts(m, "hart isa\t: "); + print_isa(m, hart_isa[cpu_id].isa); seq_puts(m, "\n"); return 0;
In /proc/cpuinfo, most of the information we show for each processor is specific to that hart: marchid, mvendorid, mimpid, processor, hart, compatible, and the mmu size. But the ISA string gets filtered through a lowest common denominator mask, so that if one CPU is missing an ISA extension, no CPUs will show it. Now that we track the ISA extensions for each hart, let's report ISA extension info accurately per-hart in /proc/cpuinfo. We cannot change the "isa:" line, as usermode may be relying on that line to show only the common set of extensions supported across all harts. Add a new "hart isa" line instead, which reports the true set of extensions for that hart. This matches what is returned in riscv_hwprobe() when querying a given hart. Signed-off-by: Evan Green <evan@rivosinc.com> --- Changes in v2: - Added new "hart isa" line rather than altering behavior of existing "isa" line (Conor, Palmer) I based this series on top of Conor's riscv-extensions-strings branch from July 3rd, since otherwise this change gets hopelessly entangled with that series. I was unsure if I could snuggle the new "hart isa" line in just below "isa". Aesthetically it would be quite pleasing, but it runs the risk of breaking brittle usermode parsers that are assuming a specific line order. So I put it at the end. --- arch/riscv/kernel/cpu.c | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-)