[v4] RISC-V: Show accurate per-hart isa in /proc/cpuinfo

Commit Message

Evan Green July 11, 2023, 8:18 p.m. UTC

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 <[e-mail]>
Reviewed-by: Andrew Jones <[e-mail]>
Reviewed-by: Conor Dooley <[e-mail]>

---

Changes in v4:
 - Documentation: Made the underline match the text line (Conor)
 - Documentation: hanged "in question" to "being described" (Andrew)

Changes in v3:
 - Add some documentation (Conor)

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.

---
 Documentation/riscv/uabi.rst | 10 ++++++++++
 arch/riscv/kernel/cpu.c      | 22 ++++++++++++++++++----
 2 files changed, 28 insertions(+), 4 deletions(-)

Comments

Conor Dooley Aug. 24, 2023, 12:20 p.m. UTC | #1

On Tue, Jul 11, 2023 at 01:18:30PM -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 <[e-mail]>
> Reviewed-by: Andrew Jones <[e-mail]>
> Reviewed-by: Conor Dooley <[e-mail]>
> 
> ---
> 
> Changes in v4:
>  - Documentation: Made the underline match the text line (Conor)
>  - Documentation: hanged "in question" to "being described" (Andrew)
> 
> Changes in v3:
>  - Add some documentation (Conor)
> 
> 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.
> 
> ---
>  Documentation/riscv/uabi.rst | 10 ++++++++++
>  arch/riscv/kernel/cpu.c      | 22 ++++++++++++++++++----
>  2 files changed, 28 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/riscv/uabi.rst b/Documentation/riscv/uabi.rst
> index 8960fac42c40..afdda580e5a2 100644
> --- a/Documentation/riscv/uabi.rst
> +++ b/Documentation/riscv/uabi.rst
> @@ -42,6 +42,16 @@ An example string following the order is::
>  
>     rv64imadc_zifoo_zigoo_zafoo_sbar_scar_zxmbaz_xqux_xrux
>  
> +"isa" vs "hart isa" lines in /proc/cpuinfo
> +------------------------------------------
> +
> +The "isa" line in /proc/cpuinfo describes the lowest common denominator of
> +RISC-V ISA extensions understood by the kernel and implemented on all harts. The
> +"hart isa" line, in contrast, describes the set of extensions understood by the
> +kernel on the particular hart being described, even if those extensions may not
> +be present on all harts in the system. The "hart isa" line is consistent with
> +what's returned by __riscv_hwprobe() when querying for that specific CPU.

Thinking about this again, I don't think this is true. hwprobe uses
has_fpu(), has_vector() etc that interact with Kconfig options but the
percpu isa bitmap isn't affected by these.


> +
>  Misaligned accesses
>  -------------------
>  
> 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
>

Evan Green Aug. 24, 2023, 4:18 p.m. UTC | #2

On Thu, Aug 24, 2023 at 5:20 AM Conor Dooley <[e-mail]> wrote:
>
> On Tue, Jul 11, 2023 at 01:18:30PM -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 <[e-mail]>
> > Reviewed-by: Andrew Jones <[e-mail]>
> > Reviewed-by: Conor Dooley <[e-mail]>
> >
> > ---
> >
> > Changes in v4:
> >  - Documentation: Made the underline match the text line (Conor)
> >  - Documentation: hanged "in question" to "being described" (Andrew)
> >
> > Changes in v3:
> >  - Add some documentation (Conor)
> >
> > 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.
> >
> > ---
> >  Documentation/riscv/uabi.rst | 10 ++++++++++
> >  arch/riscv/kernel/cpu.c      | 22 ++++++++++++++++++----
> >  2 files changed, 28 insertions(+), 4 deletions(-)
> >
> > diff --git a/Documentation/riscv/uabi.rst b/Documentation/riscv/uabi.rst
> > index 8960fac42c40..afdda580e5a2 100644
> > --- a/Documentation/riscv/uabi.rst
> > +++ b/Documentation/riscv/uabi.rst
> > @@ -42,6 +42,16 @@ An example string following the order is::
> >
> >     rv64imadc_zifoo_zigoo_zafoo_sbar_scar_zxmbaz_xqux_xrux
> >
> > +"isa" vs "hart isa" lines in /proc/cpuinfo
> > +------------------------------------------
> > +
> > +The "isa" line in /proc/cpuinfo describes the lowest common denominator of
> > +RISC-V ISA extensions understood by the kernel and implemented on all harts. The
> > +"hart isa" line, in contrast, describes the set of extensions understood by the
> > +kernel on the particular hart being described, even if those extensions may not
> > +be present on all harts in the system. The "hart isa" line is consistent with
> > +what's returned by __riscv_hwprobe() when querying for that specific CPU.
>
> Thinking about this again, I don't think this is true. hwprobe uses
> has_fpu(), has_vector() etc that interact with Kconfig options but the
> percpu isa bitmap isn't affected by these.

Ugh yeah it's kind of a mishmash isn't it. hwprobe_isa_ext0() uses the
lowest common denominator for FD, C, V, but per-hart info for
Zba,Zbb,Zbs. Given the interface, per-hart info seems like what we
should have done there, and the FD, C, and V were my bad. The good
news is we can define new bits that do the right thing, though maybe
we should wait until someone actually wants them. For this patch we
should just remove this sentence. We can also correct the
documentation in hwprobe to mention the shortcoming in FD,C,V.

Palmer, do you want a spin of this patch or a followup on top to
remove the above sentence?

Conor Dooley Aug. 24, 2023, 5:29 p.m. UTC | #3

On Thu, Aug 24, 2023 at 09:18:16AM -0700, Evan Green wrote:
> On Thu, Aug 24, 2023 at 5:20 AM Conor Dooley <[e-mail]> wrote:
> >
> > On Tue, Jul 11, 2023 at 01:18:30PM -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 <[e-mail]>
> > > Reviewed-by: Andrew Jones <[e-mail]>
> > > Reviewed-by: Conor Dooley <[e-mail]>
> > >
> > > ---
> > >
> > > Changes in v4:
> > >  - Documentation: Made the underline match the text line (Conor)
> > >  - Documentation: hanged "in question" to "being described" (Andrew)
> > >
> > > Changes in v3:
> > >  - Add some documentation (Conor)
> > >
> > > 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.
> > >
> > > ---
> > >  Documentation/riscv/uabi.rst | 10 ++++++++++
> > >  arch/riscv/kernel/cpu.c      | 22 ++++++++++++++++++----
> > >  2 files changed, 28 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/Documentation/riscv/uabi.rst b/Documentation/riscv/uabi.rst
> > > index 8960fac42c40..afdda580e5a2 100644
> > > --- a/Documentation/riscv/uabi.rst
> > > +++ b/Documentation/riscv/uabi.rst
> > > @@ -42,6 +42,16 @@ An example string following the order is::
> > >
> > >     rv64imadc_zifoo_zigoo_zafoo_sbar_scar_zxmbaz_xqux_xrux
> > >
> > > +"isa" vs "hart isa" lines in /proc/cpuinfo
> > > +------------------------------------------
> > > +
> > > +The "isa" line in /proc/cpuinfo describes the lowest common denominator of
> > > +RISC-V ISA extensions understood by the kernel and implemented on all harts. The
> > > +"hart isa" line, in contrast, describes the set of extensions understood by the
> > > +kernel on the particular hart being described, even if those extensions may not
> > > +be present on all harts in the system. The "hart isa" line is consistent with
> > > +what's returned by __riscv_hwprobe() when querying for that specific CPU.
> >
> > Thinking about this again, I don't think this is true. hwprobe uses
> > has_fpu(), has_vector() etc that interact with Kconfig options but the
> > percpu isa bitmap isn't affected by these.
> 
> Ugh yeah it's kind of a mishmash isn't it. hwprobe_isa_ext0() uses the
> lowest common denominator for FD, C, V, but per-hart info for
> Zba,Zbb,Zbs. Given the interface, per-hart info seems like what we
> should have done there, and the FD, C, and V were my bad. The good
> news is we can define new bits that do the right thing, though maybe
> we should wait until someone actually wants them. For this patch we
> should just remove this sentence. We can also correct the
> documentation in hwprobe to mention the shortcoming in FD,C,V.

I'm not really sure it's all that much of a shortcoming for V or FD,
since without the kernel support you shouldn't be using those extensions
anyway. A hwprobe thing for that sounds like a footgun to me & I think
the current behaviour is how it should be for these extensions.
It not being per-cpu is arguably a bug I suppose? But I would contend
that we are conveying support for the extension on a per-hart level,
with it then also gated by the kernel supporting V or FD, which is on a
system-wide basis.
Any other extensions that require Kconfig-gated kernel support should
also not report via hwprobe that the extension is supported when the
Kconfig option is disabled. It just so happens that the set of
extensions that hwprobe supports that are Kconfig-gated and those that
require all-hart support are one and the same right now, so we can kinda
just conflate the two & use has_vector() et al that handles both
kconfig-gating and all-hart support. Until something comes along that needs
anything different, I'd leave well enough alone for hwprobe...

> Palmer, do you want a spin of this patch or a followup on top to
> remove the above sentence?

It's not actually been applied yet, right?

Do you want to have this new thing in cpuinfo tell the user "this hart
has xyz extensions that are supported by a kernel, but maybe not this
kernel" or to tell the user "this hart has xyz extensions that are
supported by this kernel"? Your text above says "understood by the
kernel", but I think that's a poor definition that needs to be improved
to spell out exactly what you mean. IOW does "understood" mean the
kernel will parse them into a structure, or does it mean "yes you can
use this extension on this particular hart".

Thanks,
Conor.

Evan Green Aug. 24, 2023, 10:06 p.m. UTC | #4

On Thu, Aug 24, 2023 at 10:29 AM Conor Dooley <[e-mail]> wrote:
>
> On Thu, Aug 24, 2023 at 09:18:16AM -0700, Evan Green wrote:
> > On Thu, Aug 24, 2023 at 5:20 AM Conor Dooley <[e-mail]> wrote:
> > >
> > > On Tue, Jul 11, 2023 at 01:18:30PM -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 <[e-mail]>
> > > > Reviewed-by: Andrew Jones <[e-mail]>
> > > > Reviewed-by: Conor Dooley <[e-mail]>
> > > >
> > > > ---
> > > >
> > > > Changes in v4:
> > > >  - Documentation: Made the underline match the text line (Conor)
> > > >  - Documentation: hanged "in question" to "being described" (Andrew)
> > > >
> > > > Changes in v3:
> > > >  - Add some documentation (Conor)
> > > >
> > > > 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.
> > > >
> > > > ---
> > > >  Documentation/riscv/uabi.rst | 10 ++++++++++
> > > >  arch/riscv/kernel/cpu.c      | 22 ++++++++++++++++++----
> > > >  2 files changed, 28 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/Documentation/riscv/uabi.rst b/Documentation/riscv/uabi.rst
> > > > index 8960fac42c40..afdda580e5a2 100644
> > > > --- a/Documentation/riscv/uabi.rst
> > > > +++ b/Documentation/riscv/uabi.rst
> > > > @@ -42,6 +42,16 @@ An example string following the order is::
> > > >
> > > >     rv64imadc_zifoo_zigoo_zafoo_sbar_scar_zxmbaz_xqux_xrux
> > > >
> > > > +"isa" vs "hart isa" lines in /proc/cpuinfo
> > > > +------------------------------------------
> > > > +
> > > > +The "isa" line in /proc/cpuinfo describes the lowest common denominator of
> > > > +RISC-V ISA extensions understood by the kernel and implemented on all harts. The
> > > > +"hart isa" line, in contrast, describes the set of extensions understood by the
> > > > +kernel on the particular hart being described, even if those extensions may not
> > > > +be present on all harts in the system. The "hart isa" line is consistent with
> > > > +what's returned by __riscv_hwprobe() when querying for that specific CPU.
> > >
> > > Thinking about this again, I don't think this is true. hwprobe uses
> > > has_fpu(), has_vector() etc that interact with Kconfig options but the
> > > percpu isa bitmap isn't affected by these.
> >
> > Ugh yeah it's kind of a mishmash isn't it. hwprobe_isa_ext0() uses the
> > lowest common denominator for FD, C, V, but per-hart info for
> > Zba,Zbb,Zbs. Given the interface, per-hart info seems like what we
> > should have done there, and the FD, C, and V were my bad. The good
> > news is we can define new bits that do the right thing, though maybe
> > we should wait until someone actually wants them. For this patch we
> > should just remove this sentence. We can also correct the
> > documentation in hwprobe to mention the shortcoming in FD,C,V.
>
> I'm not really sure it's all that much of a shortcoming for V or FD,
> since without the kernel support you shouldn't be using those extensions
> anyway. A hwprobe thing for that sounds like a footgun to me & I think
> the current behaviour is how it should be for these extensions.
> It not being per-cpu is arguably a bug I suppose? But I would contend

Yeah it was mostly the not being per-cpu I was pointing to in my previous email.

> that we are conveying support for the extension on a per-hart level,
> with it then also gated by the kernel supporting V or FD, which is on a
> system-wide basis.
> Any other extensions that require Kconfig-gated kernel support should
> also not report via hwprobe that the extension is supported when the
> Kconfig option is disabled. It just so happens that the set of
> extensions that hwprobe supports that are Kconfig-gated and those that
> require all-hart support are one and the same right now, so we can kinda
> just conflate the two & use has_vector() et al that handles both
> kconfig-gating and all-hart support. Until something comes along that needs
> anything different, I'd leave well enough alone for hwprobe...

Sounds good.

>
> > Palmer, do you want a spin of this patch or a followup on top to
> > remove the above sentence?
>
> It's not actually been applied yet, right?
>
> Do you want to have this new thing in cpuinfo tell the user "this hart
> has xyz extensions that are supported by a kernel, but maybe not this
> kernel" or to tell the user "this hart has xyz extensions that are
> supported by this kernel"? Your text above says "understood by the
> kernel", but I think that's a poor definition that needs to be improved
> to spell out exactly what you mean. IOW does "understood" mean the
> kernel will parse them into a structure, or does it mean "yes you can
> use this extension on this particular hart".

I'm imagining /proc/cpuinfo being closer to "the CPU has it and the
kernel at least vaguely understands it, but may not have full support
for it enabled". I'm assuming /proc/cpuinfo is mostly used by 1)
humans wanting to know if they have hardware support for a feature,
and 2) administrators collecting telemetry to manage fleets (ie do I
have any hardware deployed that supports X). Programmers looking to
see "is the kernel support for this feature ready right now" would
ideally not be parsing /proc/cpuinfo text, as more direct mechanisms
like specific hwprobe bits for "am I fully ready to go" would be
easier to work with. Feel free to yell at me if this overall vision
seems flawed.

I tried to look to see if there was consensus among the other
architectures. Aarch64 seems to go with "supported and fully enabled",
as their cpu_has_feature() directly tests elf_hwcap, and elements in
arm64_elf_hwcaps[] are Kconfig gated. X86 is complicated, but IIRC is
more along the lines of "hardware has it". They have two macros,
cpu_has() for "raw capability" and cpu_feature_enabled() for "kernel
can do it too", and they use cpu_has() for /proc/cpuinfo flags.

-Evan

Conor Dooley Aug. 24, 2023, 10:27 p.m. UTC | #5

On Thu, Aug 24, 2023 at 03:06:53PM -0700, Evan Green wrote:
> On Thu, Aug 24, 2023 at 10:29 AM Conor Dooley <[e-mail]> wrote:
> > On Thu, Aug 24, 2023 at 09:18:16AM -0700, Evan Green wrote:
> > > On Thu, Aug 24, 2023 at 5:20 AM Conor Dooley <[e-mail]> wrote:
> > > > On Tue, Jul 11, 2023 at 01:18:30PM -0700, Evan Green wrote:

> > > > > +"isa" vs "hart isa" lines in /proc/cpuinfo
> > > > > +------------------------------------------
> > > > > +
> > > > > +The "isa" line in /proc/cpuinfo describes the lowest common denominator of
> > > > > +RISC-V ISA extensions understood by the kernel and implemented on all harts. The
> > > > > +"hart isa" line, in contrast, describes the set of extensions understood by the
> > > > > +kernel on the particular hart being described, even if those extensions may not
> > > > > +be present on all harts in the system. The "hart isa" line is consistent with
> > > > > +what's returned by __riscv_hwprobe() when querying for that specific CPU.
> > > >
> > > > Thinking about this again, I don't think this is true. hwprobe uses
> > > > has_fpu(), has_vector() etc that interact with Kconfig options but the
> > > > percpu isa bitmap isn't affected by these.
> > >
> > > Ugh yeah it's kind of a mishmash isn't it. hwprobe_isa_ext0() uses the
> > > lowest common denominator for FD, C, V, but per-hart info for
> > > Zba,Zbb,Zbs. Given the interface, per-hart info seems like what we
> > > should have done there, and the FD, C, and V were my bad. The good
> > > news is we can define new bits that do the right thing, though maybe
> > > we should wait until someone actually wants them. For this patch we
> > > should just remove this sentence. We can also correct the
> > > documentation in hwprobe to mention the shortcoming in FD,C,V.
> >
> > I'm not really sure it's all that much of a shortcoming for V or FD,
> > since without the kernel support you shouldn't be using those extensions
> > anyway. A hwprobe thing for that sounds like a footgun to me & I think
> > the current behaviour is how it should be for these extensions.
> > It not being per-cpu is arguably a bug I suppose? But I would contend
> 
> Yeah it was mostly the not being per-cpu I was pointing to in my previous email.
> 
> > that we are conveying support for the extension on a per-hart level,
> > with it then also gated by the kernel supporting V or FD, which is on a
> > system-wide basis.
> > Any other extensions that require Kconfig-gated kernel support should
> > also not report via hwprobe that the extension is supported when the
> > Kconfig option is disabled. It just so happens that the set of
> > extensions that hwprobe supports that are Kconfig-gated and those that
> > require all-hart support are one and the same right now, so we can kinda
> > just conflate the two & use has_vector() et al that handles both
> > kconfig-gating and all-hart support. Until something comes along that needs
> > anything different, I'd leave well enough alone for hwprobe...
> 
> Sounds good.
> 
> >
> > > Palmer, do you want a spin of this patch or a followup on top to
> > > remove the above sentence?
> >
> > It's not actually been applied yet, right?
> >
> > Do you want to have this new thing in cpuinfo tell the user "this hart
> > has xyz extensions that are supported by a kernel, but maybe not this
> > kernel" or to tell the user "this hart has xyz extensions that are
> > supported by this kernel"? Your text above says "understood by the
> > kernel", but I think that's a poor definition that needs to be improved
> > to spell out exactly what you mean. IOW does "understood" mean the
> > kernel will parse them into a structure, or does it mean "yes you can
> > use this extension on this particular hart".
> 
> I'm imagining /proc/cpuinfo being closer to "the CPU has it and the
> kernel at least vaguely understands it, but may not have full support
> for it enabled". I'm assuming /proc/cpuinfo is mostly used by 1)
> humans wanting to know if they have hardware support for a feature,
> and 2) administrators collecting telemetry to manage fleets (ie do I
> have any hardware deployed that supports X). Programmers looking to
> see "is the kernel support for this feature ready right now" would
> ideally not be parsing /proc/cpuinfo text, as more direct mechanisms
> like specific hwprobe bits for "am I fully ready to go" would be
> easier to work with. Feel free to yell at me if this overall vision
> seems flawed.
> 
> I tried to look to see if there was consensus among the other
> architectures. Aarch64 seems to go with "supported and fully enabled",
> as their cpu_has_feature() directly tests elf_hwcap, and elements in
> arm64_elf_hwcaps[] are Kconfig gated. X86 is complicated, but IIRC is
> more along the lines of "hardware has it". They have two macros,
> cpu_has() for "raw capability" and cpu_feature_enabled() for "kernel
> can do it too", and they use cpu_has() for /proc/cpuinfo flags.

I'm fine with the per-cpu stuff meaning "the hardware has it and a kernel,
but not necessarily this one, supports it" - just please make the
documentation clear about it.

Evan Green Aug. 24, 2023, 10:45 p.m. UTC | #6

On Thu, Aug 24, 2023 at 3:28 PM Conor Dooley <[e-mail]> wrote:
>
> On Thu, Aug 24, 2023 at 03:06:53PM -0700, Evan Green wrote:
> > On Thu, Aug 24, 2023 at 10:29 AM Conor Dooley <[e-mail]> wrote:
> > > On Thu, Aug 24, 2023 at 09:18:16AM -0700, Evan Green wrote:
> > > > On Thu, Aug 24, 2023 at 5:20 AM Conor Dooley <[e-mail]> wrote:
> > > > > On Tue, Jul 11, 2023 at 01:18:30PM -0700, Evan Green wrote:
>
> > > > > > +"isa" vs "hart isa" lines in /proc/cpuinfo
> > > > > > +------------------------------------------
> > > > > > +
> > > > > > +The "isa" line in /proc/cpuinfo describes the lowest common denominator of
> > > > > > +RISC-V ISA extensions understood by the kernel and implemented on all harts. The
> > > > > > +"hart isa" line, in contrast, describes the set of extensions understood by the
> > > > > > +kernel on the particular hart being described, even if those extensions may not
> > > > > > +be present on all harts in the system. The "hart isa" line is consistent with
> > > > > > +what's returned by __riscv_hwprobe() when querying for that specific CPU.
> > > > >
> > > > > Thinking about this again, I don't think this is true. hwprobe uses
> > > > > has_fpu(), has_vector() etc that interact with Kconfig options but the
> > > > > percpu isa bitmap isn't affected by these.
> > > >
> > > > Ugh yeah it's kind of a mishmash isn't it. hwprobe_isa_ext0() uses the
> > > > lowest common denominator for FD, C, V, but per-hart info for
> > > > Zba,Zbb,Zbs. Given the interface, per-hart info seems like what we
> > > > should have done there, and the FD, C, and V were my bad. The good
> > > > news is we can define new bits that do the right thing, though maybe
> > > > we should wait until someone actually wants them. For this patch we
> > > > should just remove this sentence. We can also correct the
> > > > documentation in hwprobe to mention the shortcoming in FD,C,V.
> > >
> > > I'm not really sure it's all that much of a shortcoming for V or FD,
> > > since without the kernel support you shouldn't be using those extensions
> > > anyway. A hwprobe thing for that sounds like a footgun to me & I think
> > > the current behaviour is how it should be for these extensions.
> > > It not being per-cpu is arguably a bug I suppose? But I would contend
> >
> > Yeah it was mostly the not being per-cpu I was pointing to in my previous email.
> >
> > > that we are conveying support for the extension on a per-hart level,
> > > with it then also gated by the kernel supporting V or FD, which is on a
> > > system-wide basis.
> > > Any other extensions that require Kconfig-gated kernel support should
> > > also not report via hwprobe that the extension is supported when the
> > > Kconfig option is disabled. It just so happens that the set of
> > > extensions that hwprobe supports that are Kconfig-gated and those that
> > > require all-hart support are one and the same right now, so we can kinda
> > > just conflate the two & use has_vector() et al that handles both
> > > kconfig-gating and all-hart support. Until something comes along that needs
> > > anything different, I'd leave well enough alone for hwprobe...
> >
> > Sounds good.
> >
> > >
> > > > Palmer, do you want a spin of this patch or a followup on top to
> > > > remove the above sentence?
> > >
> > > It's not actually been applied yet, right?
> > >
> > > Do you want to have this new thing in cpuinfo tell the user "this hart
> > > has xyz extensions that are supported by a kernel, but maybe not this
> > > kernel" or to tell the user "this hart has xyz extensions that are
> > > supported by this kernel"? Your text above says "understood by the
> > > kernel", but I think that's a poor definition that needs to be improved
> > > to spell out exactly what you mean. IOW does "understood" mean the
> > > kernel will parse them into a structure, or does it mean "yes you can
> > > use this extension on this particular hart".
> >
> > I'm imagining /proc/cpuinfo being closer to "the CPU has it and the
> > kernel at least vaguely understands it, but may not have full support
> > for it enabled". I'm assuming /proc/cpuinfo is mostly used by 1)
> > humans wanting to know if they have hardware support for a feature,
> > and 2) administrators collecting telemetry to manage fleets (ie do I
> > have any hardware deployed that supports X). Programmers looking to
> > see "is the kernel support for this feature ready right now" would
> > ideally not be parsing /proc/cpuinfo text, as more direct mechanisms
> > like specific hwprobe bits for "am I fully ready to go" would be
> > easier to work with. Feel free to yell at me if this overall vision
> > seems flawed.
> >
> > I tried to look to see if there was consensus among the other
> > architectures. Aarch64 seems to go with "supported and fully enabled",
> > as their cpu_has_feature() directly tests elf_hwcap, and elements in
> > arm64_elf_hwcaps[] are Kconfig gated. X86 is complicated, but IIRC is
> > more along the lines of "hardware has it". They have two macros,
> > cpu_has() for "raw capability" and cpu_feature_enabled() for "kernel
> > can do it too", and they use cpu_has() for /proc/cpuinfo flags.
>
> I'm fine with the per-cpu stuff meaning "the hardware has it and a kernel,
> but not necessarily this one, supports it" - just please make the
> documentation clear about it.

Sounds good, will spin.
-Evan

Andrew Jones Aug. 25, 2023, 8:16 a.m. UTC | #7

On Thu, Aug 24, 2023 at 03:06:53PM -0700, Evan Green wrote:
> On Thu, Aug 24, 2023 at 10:29 AM Conor Dooley <[e-mail]> wrote:
...
> > Do you want to have this new thing in cpuinfo tell the user "this hart
> > has xyz extensions that are supported by a kernel, but maybe not this
> > kernel" or to tell the user "this hart has xyz extensions that are
> > supported by this kernel"? Your text above says "understood by the
> > kernel", but I think that's a poor definition that needs to be improved
> > to spell out exactly what you mean. IOW does "understood" mean the
> > kernel will parse them into a structure, or does it mean "yes you can
> > use this extension on this particular hart".
> 
> I'm imagining /proc/cpuinfo being closer to "the CPU has it and the
> kernel at least vaguely understands it, but may not have full support
> for it enabled". I'm assuming /proc/cpuinfo is mostly used by 1)
> humans wanting to know if they have hardware support for a feature,
> and 2) administrators collecting telemetry to manage fleets (ie do I
> have any hardware deployed that supports X).

Is (2) a special case of (1)? (I want to make sure I understand all the
cases.)

> Programmers looking to
> see "is the kernel support for this feature ready right now" would
> ideally not be parsing /proc/cpuinfo text, as more direct mechanisms
> like specific hwprobe bits for "am I fully ready to go" would be
> easier to work with. Feel free to yell at me if this overall vision
> seems flawed.
> 
> I tried to look to see if there was consensus among the other
> architectures. Aarch64 seems to go with "supported and fully enabled",
> as their cpu_has_feature() directly tests elf_hwcap, and elements in
> arm64_elf_hwcaps[] are Kconfig gated. X86 is complicated, but IIRC is
> more along the lines of "hardware has it". They have two macros,
> cpu_has() for "raw capability" and cpu_feature_enabled() for "kernel
> can do it too", and they use cpu_has() for /proc/cpuinfo flags.
>

I'd lean more towards the way AArch64 goes, because, unless /proc/cpuinfo
is just a blind regurgitation of an isa string from DT / ACPI, then the
kernel must at least know something about it. Advertising a feature which
is known, but also known not to work, seems odd to me. So my vote is that
only features which are present and enabled in the kernel or present and
not necessary to be enabled in the kernel in order for userspace or
virtual machines to use be advertised in /proc/cpuinfo.

We still have SMBIOS (dmidecode) to blindly dump what the hardware
supports for cases (1) and (2) above.

Thanks,
drew

Evan Green Aug. 25, 2023, 10:51 p.m. UTC | #8

On Fri, Aug 25, 2023 at 1:16 AM Andrew Jones <[e-mail]> wrote:
>
> On Thu, Aug 24, 2023 at 03:06:53PM -0700, Evan Green wrote:
> > On Thu, Aug 24, 2023 at 10:29 AM Conor Dooley <[e-mail]> wrote:
> ...
> > > Do you want to have this new thing in cpuinfo tell the user "this hart
> > > has xyz extensions that are supported by a kernel, but maybe not this
> > > kernel" or to tell the user "this hart has xyz extensions that are
> > > supported by this kernel"? Your text above says "understood by the
> > > kernel", but I think that's a poor definition that needs to be improved
> > > to spell out exactly what you mean. IOW does "understood" mean the
> > > kernel will parse them into a structure, or does it mean "yes you can
> > > use this extension on this particular hart".
> >
> > I'm imagining /proc/cpuinfo being closer to "the CPU has it and the
> > kernel at least vaguely understands it, but may not have full support
> > for it enabled". I'm assuming /proc/cpuinfo is mostly used by 1)
> > humans wanting to know if they have hardware support for a feature,
> > and 2) administrators collecting telemetry to manage fleets (ie do I
> > have any hardware deployed that supports X).
>
> Is (2) a special case of (1)? (I want to make sure I understand all the
> cases.)

More or less, yes. In bucket two are also folks wondering things like
"are all these crash reports I'm getting specific to machines with X".

>
> > Programmers looking to
> > see "is the kernel support for this feature ready right now" would
> > ideally not be parsing /proc/cpuinfo text, as more direct mechanisms
> > like specific hwprobe bits for "am I fully ready to go" would be
> > easier to work with. Feel free to yell at me if this overall vision
> > seems flawed.
> >
> > I tried to look to see if there was consensus among the other
> > architectures. Aarch64 seems to go with "supported and fully enabled",
> > as their cpu_has_feature() directly tests elf_hwcap, and elements in
> > arm64_elf_hwcaps[] are Kconfig gated. X86 is complicated, but IIRC is
> > more along the lines of "hardware has it". They have two macros,
> > cpu_has() for "raw capability" and cpu_feature_enabled() for "kernel
> > can do it too", and they use cpu_has() for /proc/cpuinfo flags.
> >
>
> I'd lean more towards the way AArch64 goes, because, unless /proc/cpuinfo
> is just a blind regurgitation of an isa string from DT / ACPI, then the
> kernel must at least know something about it. Advertising a feature which
> is known, but also known not to work, seems odd to me. So my vote is that
> only features which are present and enabled in the kernel or present and
> not necessary to be enabled in the kernel in order for userspace or
> virtual machines to use be advertised in /proc/cpuinfo.
>
> We still have SMBIOS (dmidecode) to blindly dump what the hardware
> supports for cases (1) and (2) above.

Yeah, there's an argument to be made for that. My worry is it's a
difficult line to hold. The bar you're really trying to describe (or
at least what people might take away from it) is "if it's listed here
then it's fully ready to be used in userspace". But then things get
squishy when there are additional ways to control the use of the
feature (prctls() in init to turn it on, usermode policy to turn it
off, security doodads that disable it, etc). I'm assuming nobody wants
a version of /proc/cpuinfo that changes depending on which process is
asking. So then the line would have to be more carefully described as
"well, the hardware can do it, and the kernel COULD do it under some
circumstances, but YMMV", which ends up falling somewhat short of the
original goal. In my mind keeping /proc/cpuinfo as close to "here's
what the hardware can do" seems like a more defensible position.
-Evan

Andrew Jones Aug. 26, 2023, 8:01 a.m. UTC | #9

On Fri, Aug 25, 2023 at 03:51:28PM -0700, Evan Green wrote:
> On Fri, Aug 25, 2023 at 1:16 AM Andrew Jones <[e-mail]> wrote:
> >
> > On Thu, Aug 24, 2023 at 03:06:53PM -0700, Evan Green wrote:
> > > On Thu, Aug 24, 2023 at 10:29 AM Conor Dooley <[e-mail]> wrote:
> > ...
> > > > Do you want to have this new thing in cpuinfo tell the user "this hart
> > > > has xyz extensions that are supported by a kernel, but maybe not this
> > > > kernel" or to tell the user "this hart has xyz extensions that are
> > > > supported by this kernel"? Your text above says "understood by the
> > > > kernel", but I think that's a poor definition that needs to be improved
> > > > to spell out exactly what you mean. IOW does "understood" mean the
> > > > kernel will parse them into a structure, or does it mean "yes you can
> > > > use this extension on this particular hart".
> > >
> > > I'm imagining /proc/cpuinfo being closer to "the CPU has it and the
> > > kernel at least vaguely understands it, but may not have full support
> > > for it enabled". I'm assuming /proc/cpuinfo is mostly used by 1)
> > > humans wanting to know if they have hardware support for a feature,
> > > and 2) administrators collecting telemetry to manage fleets (ie do I
> > > have any hardware deployed that supports X).
> >
> > Is (2) a special case of (1)? (I want to make sure I understand all the
> > cases.)
> 
> More or less, yes. In bucket two are also folks wondering things like
> "are all these crash reports I'm getting specific to machines with X".
> 
> >
> > > Programmers looking to
> > > see "is the kernel support for this feature ready right now" would
> > > ideally not be parsing /proc/cpuinfo text, as more direct mechanisms
> > > like specific hwprobe bits for "am I fully ready to go" would be
> > > easier to work with. Feel free to yell at me if this overall vision
> > > seems flawed.
> > >
> > > I tried to look to see if there was consensus among the other
> > > architectures. Aarch64 seems to go with "supported and fully enabled",
> > > as their cpu_has_feature() directly tests elf_hwcap, and elements in
> > > arm64_elf_hwcaps[] are Kconfig gated. X86 is complicated, but IIRC is
> > > more along the lines of "hardware has it". They have two macros,
> > > cpu_has() for "raw capability" and cpu_feature_enabled() for "kernel
> > > can do it too", and they use cpu_has() for /proc/cpuinfo flags.
> > >
> >
> > I'd lean more towards the way AArch64 goes, because, unless /proc/cpuinfo
> > is just a blind regurgitation of an isa string from DT / ACPI, then the
> > kernel must at least know something about it. Advertising a feature which
> > is known, but also known not to work, seems odd to me. So my vote is that
> > only features which are present and enabled in the kernel or present and
> > not necessary to be enabled in the kernel in order for userspace or
> > virtual machines to use be advertised in /proc/cpuinfo.
> >
> > We still have SMBIOS (dmidecode) to blindly dump what the hardware
> > supports for cases (1) and (2) above.
> 
> Yeah, there's an argument to be made for that. My worry is it's a
> difficult line to hold. The bar you're really trying to describe (or
> at least what people might take away from it) is "if it's listed here
> then it's fully ready to be used in userspace". But then things get
> squishy when there are additional ways to control the use of the
> feature (prctls() in init to turn it on, usermode policy to turn it
> off, security doodads that disable it, etc). I'm assuming nobody wants
> a version of /proc/cpuinfo that changes depending on which process is
> asking. So then the line would have to be more carefully described as
> "well, the hardware can do it, and the kernel COULD do it under some
> circumstances, but YMMV", which ends up falling somewhat short of the
> original goal. In my mind keeping /proc/cpuinfo as close to "here's
> what the hardware can do" seems like a more defensible position.
> -Evan

I agree with that. I was actually even trying to say the same thing,
but only by bringing up virtual machines. Once we decide we'll expose
extensions to VMs, whether or not the host kernel enables them, then
none of the other host kernel configurations matter with respect to
advertising the feature, since the guest kernel may have a completely
different set of configurations.

So I think we should only be filtering out extensions that are disabled
because they're broken (have a detected erratum), have been "hidden"
(have a kernel command line allowing them to be treated as if not
present), or cannot be used at all due to missing accompanying hardware
descriptions (such as block size info for CBO extensions). In all cases,
I presume we'd wire checks up in riscv_isa_extension_check() and no
checks would be gated on Kconfigs or anything else. And, since
/proc/cpuinfo gets its list from the bitmap that's already filtered by
riscv_isa_extension_check(), then, long story short, we're good to go :-)

But maybe we can try to spell that policy out a bit more in
Documentation/riscv/uabi.rst.

Thanks,
drew

Evan Green Aug. 28, 2023, 4:44 p.m. UTC | #10

On Sat, Aug 26, 2023 at 1:01 AM Andrew Jones <[e-mail]> wrote:
>
> On Fri, Aug 25, 2023 at 03:51:28PM -0700, Evan Green wrote:
> > On Fri, Aug 25, 2023 at 1:16 AM Andrew Jones <[e-mail]> wrote:
> > >
> > > On Thu, Aug 24, 2023 at 03:06:53PM -0700, Evan Green wrote:
> > > > On Thu, Aug 24, 2023 at 10:29 AM Conor Dooley <[e-mail]> wrote:
> > > ...
> > > > > Do you want to have this new thing in cpuinfo tell the user "this hart
> > > > > has xyz extensions that are supported by a kernel, but maybe not this
> > > > > kernel" or to tell the user "this hart has xyz extensions that are
> > > > > supported by this kernel"? Your text above says "understood by the
> > > > > kernel", but I think that's a poor definition that needs to be improved
> > > > > to spell out exactly what you mean. IOW does "understood" mean the
> > > > > kernel will parse them into a structure, or does it mean "yes you can
> > > > > use this extension on this particular hart".
> > > >
> > > > I'm imagining /proc/cpuinfo being closer to "the CPU has it and the
> > > > kernel at least vaguely understands it, but may not have full support
> > > > for it enabled". I'm assuming /proc/cpuinfo is mostly used by 1)
> > > > humans wanting to know if they have hardware support for a feature,
> > > > and 2) administrators collecting telemetry to manage fleets (ie do I
> > > > have any hardware deployed that supports X).
> > >
> > > Is (2) a special case of (1)? (I want to make sure I understand all the
> > > cases.)
> >
> > More or less, yes. In bucket two are also folks wondering things like
> > "are all these crash reports I'm getting specific to machines with X".
> >
> > >
> > > > Programmers looking to
> > > > see "is the kernel support for this feature ready right now" would
> > > > ideally not be parsing /proc/cpuinfo text, as more direct mechanisms
> > > > like specific hwprobe bits for "am I fully ready to go" would be
> > > > easier to work with. Feel free to yell at me if this overall vision
> > > > seems flawed.
> > > >
> > > > I tried to look to see if there was consensus among the other
> > > > architectures. Aarch64 seems to go with "supported and fully enabled",
> > > > as their cpu_has_feature() directly tests elf_hwcap, and elements in
> > > > arm64_elf_hwcaps[] are Kconfig gated. X86 is complicated, but IIRC is
> > > > more along the lines of "hardware has it". They have two macros,
> > > > cpu_has() for "raw capability" and cpu_feature_enabled() for "kernel
> > > > can do it too", and they use cpu_has() for /proc/cpuinfo flags.
> > > >
> > >
> > > I'd lean more towards the way AArch64 goes, because, unless /proc/cpuinfo
> > > is just a blind regurgitation of an isa string from DT / ACPI, then the
> > > kernel must at least know something about it. Advertising a feature which
> > > is known, but also known not to work, seems odd to me. So my vote is that
> > > only features which are present and enabled in the kernel or present and
> > > not necessary to be enabled in the kernel in order for userspace or
> > > virtual machines to use be advertised in /proc/cpuinfo.
> > >
> > > We still have SMBIOS (dmidecode) to blindly dump what the hardware
> > > supports for cases (1) and (2) above.
> >
> > Yeah, there's an argument to be made for that. My worry is it's a
> > difficult line to hold. The bar you're really trying to describe (or
> > at least what people might take away from it) is "if it's listed here
> > then it's fully ready to be used in userspace". But then things get
> > squishy when there are additional ways to control the use of the
> > feature (prctls() in init to turn it on, usermode policy to turn it
> > off, security doodads that disable it, etc). I'm assuming nobody wants
> > a version of /proc/cpuinfo that changes depending on which process is
> > asking. So then the line would have to be more carefully described as
> > "well, the hardware can do it, and the kernel COULD do it under some
> > circumstances, but YMMV", which ends up falling somewhat short of the
> > original goal. In my mind keeping /proc/cpuinfo as close to "here's
> > what the hardware can do" seems like a more defensible position.
> > -Evan
>
> I agree with that. I was actually even trying to say the same thing,
> but only by bringing up virtual machines. Once we decide we'll expose
> extensions to VMs, whether or not the host kernel enables them, then
> none of the other host kernel configurations matter with respect to
> advertising the feature, since the guest kernel may have a completely
> different set of configurations.

My head spins a little when I try to picture a feature which 1)
requires kernel support to use, 2) has that kernel support turned off
in the host kernel, but 3) is passed down into guest kernels.
Generally though, I agree that trying to tie the guarantees of
features in /proc/cpuinfo too much to software gets confusing when
viewed through the double lens of virtualization.

>
> So I think we should only be filtering out extensions that are disabled
> because they're broken (have a detected erratum), have been "hidden"
> (have a kernel command line allowing them to be treated as if not
> present), or cannot be used at all due to missing accompanying hardware
> descriptions (such as block size info for CBO extensions). In all cases,
> I presume we'd wire checks up in riscv_isa_extension_check() and no
> checks would be gated on Kconfigs or anything else. And, since
> /proc/cpuinfo gets its list from the bitmap that's already filtered by
> riscv_isa_extension_check(), then, long story short, we're good to go :-)
>
> But maybe we can try to spell that policy out a bit more in
> Documentation/riscv/uabi.rst.

Right, that sounds reasonable to me, and is consistent with the
behavior we already have. With this documentation change I was only
trying to define the lower bound, rather than the complete definition
for every case. In other words, seeing a feature in cpuinfo guarantees
only that the hardware (or virtualized hardware) supports the feature,
but that's all the language says. So for instance NOT seeing a feature
in cpuinfo doesn't necessarily mean the hardware doesn't support it.
Software turning it off for the reasons you describe IMO doesn't
contradict what's written here. I was planning to leave that tacit,
but if you have suggestions on how to spell that out I'd take them.

-Evan

Conor Dooley Aug. 28, 2023, 5:13 p.m. UTC | #11

On Mon, Aug 28, 2023 at 09:44:55AM -0700, Evan Green wrote:
> On Sat, Aug 26, 2023 at 1:01 AM Andrew Jones <[e-mail]> wrote:
> >
> > On Fri, Aug 25, 2023 at 03:51:28PM -0700, Evan Green wrote:
> > > On Fri, Aug 25, 2023 at 1:16 AM Andrew Jones <[e-mail]> wrote:
> > > >
> > > > On Thu, Aug 24, 2023 at 03:06:53PM -0700, Evan Green wrote:
> > > > > On Thu, Aug 24, 2023 at 10:29 AM Conor Dooley <[e-mail]> wrote:
> > > > ...
> > > > > > Do you want to have this new thing in cpuinfo tell the user "this hart
> > > > > > has xyz extensions that are supported by a kernel, but maybe not this
> > > > > > kernel" or to tell the user "this hart has xyz extensions that are
> > > > > > supported by this kernel"? Your text above says "understood by the
> > > > > > kernel", but I think that's a poor definition that needs to be improved
> > > > > > to spell out exactly what you mean. IOW does "understood" mean the
> > > > > > kernel will parse them into a structure, or does it mean "yes you can
> > > > > > use this extension on this particular hart".
> > > > >
> > > > > I'm imagining /proc/cpuinfo being closer to "the CPU has it and the
> > > > > kernel at least vaguely understands it, but may not have full support
> > > > > for it enabled". I'm assuming /proc/cpuinfo is mostly used by 1)
> > > > > humans wanting to know if they have hardware support for a feature,
> > > > > and 2) administrators collecting telemetry to manage fleets (ie do I
> > > > > have any hardware deployed that supports X).
> > > >
> > > > Is (2) a special case of (1)? (I want to make sure I understand all the
> > > > cases.)
> > >
> > > More or less, yes. In bucket two are also folks wondering things like
> > > "are all these crash reports I'm getting specific to machines with X".
> > >
> > > >
> > > > > Programmers looking to
> > > > > see "is the kernel support for this feature ready right now" would
> > > > > ideally not be parsing /proc/cpuinfo text, as more direct mechanisms
> > > > > like specific hwprobe bits for "am I fully ready to go" would be
> > > > > easier to work with. Feel free to yell at me if this overall vision
> > > > > seems flawed.
> > > > >
> > > > > I tried to look to see if there was consensus among the other
> > > > > architectures. Aarch64 seems to go with "supported and fully enabled",
> > > > > as their cpu_has_feature() directly tests elf_hwcap, and elements in
> > > > > arm64_elf_hwcaps[] are Kconfig gated. X86 is complicated, but IIRC is
> > > > > more along the lines of "hardware has it". They have two macros,
> > > > > cpu_has() for "raw capability" and cpu_feature_enabled() for "kernel
> > > > > can do it too", and they use cpu_has() for /proc/cpuinfo flags.
> > > > >
> > > >
> > > > I'd lean more towards the way AArch64 goes, because, unless /proc/cpuinfo
> > > > is just a blind regurgitation of an isa string from DT / ACPI, then the
> > > > kernel must at least know something about it. Advertising a feature which
> > > > is known, but also known not to work, seems odd to me. So my vote is that
> > > > only features which are present and enabled in the kernel or present and
> > > > not necessary to be enabled in the kernel in order for userspace or
> > > > virtual machines to use be advertised in /proc/cpuinfo.
> > > >
> > > > We still have SMBIOS (dmidecode) to blindly dump what the hardware
> > > > supports for cases (1) and (2) above.
> > >
> > > Yeah, there's an argument to be made for that. My worry is it's a
> > > difficult line to hold. The bar you're really trying to describe (or
> > > at least what people might take away from it) is "if it's listed here
> > > then it's fully ready to be used in userspace". But then things get
> > > squishy when there are additional ways to control the use of the
> > > feature (prctls() in init to turn it on, usermode policy to turn it
> > > off, security doodads that disable it, etc). I'm assuming nobody wants
> > > a version of /proc/cpuinfo that changes depending on which process is
> > > asking. So then the line would have to be more carefully described as
> > > "well, the hardware can do it, and the kernel COULD do it under some
> > > circumstances, but YMMV", which ends up falling somewhat short of the
> > > original goal. In my mind keeping /proc/cpuinfo as close to "here's
> > > what the hardware can do" seems like a more defensible position.
> > > -Evan
> >
> > I agree with that. I was actually even trying to say the same thing,
> > but only by bringing up virtual machines. Once we decide we'll expose
> > extensions to VMs, whether or not the host kernel enables them, then
> > none of the other host kernel configurations matter with respect to
> > advertising the feature, since the guest kernel may have a completely
> > different set of configurations.
> 
> My head spins a little when I try to picture a feature which 1)
> requires kernel support to use, 2) has that kernel support turned off
> in the host kernel, but 3) is passed down into guest kernels.

Mine did too, but apparently these already exist for kvm guests. I can't
find the exact email, but either Drew or Anup told me that Svpbmt can be
used by a guest even if support for it is not present in the host
kernel.

Thanks,
Conor.

> Generally though, I agree that trying to tie the guarantees of
> features in /proc/cpuinfo too much to software gets confusing when
> viewed through the double lens of virtualization.
> 
> >
> > So I think we should only be filtering out extensions that are disabled
> > because they're broken (have a detected erratum), have been "hidden"
> > (have a kernel command line allowing them to be treated as if not
> > present), or cannot be used at all due to missing accompanying hardware
> > descriptions (such as block size info for CBO extensions). In all cases,
> > I presume we'd wire checks up in riscv_isa_extension_check() and no
> > checks would be gated on Kconfigs or anything else. And, since
> > /proc/cpuinfo gets its list from the bitmap that's already filtered by
> > riscv_isa_extension_check(), then, long story short, we're good to go :-)
> >
> > But maybe we can try to spell that policy out a bit more in
> > Documentation/riscv/uabi.rst.
> 
> Right, that sounds reasonable to me, and is consistent with the
> behavior we already have. With this documentation change I was only
> trying to define the lower bound, rather than the complete definition
> for every case. In other words, seeing a feature in cpuinfo guarantees
> only that the hardware (or virtualized hardware) supports the feature,
> but that's all the language says. So for instance NOT seeing a feature
> in cpuinfo doesn't necessarily mean the hardware doesn't support it.
> Software turning it off for the reasons you describe IMO doesn't
> contradict what's written here. I was planning to leave that tacit,
> but if you have suggestions on how to spell that out I'd take them.
> 
> -Evan

Andrew Jones Aug. 29, 2023, 8:48 a.m. UTC | #12

Hi Evan,

Here's my stab at new wording.

On Tue, Jul 11, 2023 at 01:18:30PM -0700, Evan Green wrote:
...
> diff --git a/Documentation/riscv/uabi.rst b/Documentation/riscv/uabi.rst
> index 8960fac42c40..afdda580e5a2 100644
> --- a/Documentation/riscv/uabi.rst
> +++ b/Documentation/riscv/uabi.rst
> @@ -42,6 +42,16 @@ An example string following the order is::
>  
>     rv64imadc_zifoo_zigoo_zafoo_sbar_scar_zxmbaz_xqux_xrux
>  
> +"isa" vs "hart isa" lines in /proc/cpuinfo
> +------------------------------------------
> +
> +The "isa" line in /proc/cpuinfo describes the lowest common denominator of
> +RISC-V ISA extensions understood by the kernel and implemented on all harts. The
> +"hart isa" line, in contrast, describes the set of extensions understood by the
> +kernel on the particular hart being described, even if those extensions may not
> +be present on all harts in the system. The "hart isa" line is consistent with
> +what's returned by __riscv_hwprobe() when querying for that specific CPU.
> +

The "isa" and "hart isa" lines in /proc/cpuinfo list RISC-V ISA extensions
which the kernel can identify (the kernel recognizes the extension's name)
and have not been filtered out due to effectively not being present.  An
extension is effectively not present when it is unusable, either due to
defects (which the kernel is aware of), due to missing information which
is necessary to complete the extension's description, or due to being
explicitly "hidden", such as when a kernel command line parameter
instructs the kernel to pretend the extension is not present.  Note, an
extension's presence in a list does not imply the kernel is using the
extension, nor does it imply that userspace or guest kernels may use the
extension (__riscv_hwprobe() should be queried for userspace usability.
The hypervisor should be queried, using hypervisor-specific APIs, to
check guest kernel usability.)

The "isa" line describes the lowest common denominator of extensions,
which are the extensions implemented on all harts.  In contrast, the
extensions listed in the "hart isa" line need not be implemented by
any other hart than the hart corresponding to the line.

---

I've specifically dropped the 'The "hart isa" line is consistent with
what's returned by __riscv_hwprobe()...' part because I suspect hwprobe
could at least lag what gets put in "hart isa", since the kernel may be
taught about an extension for a different purpose first, neglecting
hwprobe. And, there may be cases that hwprobe never enumerates an
extension which the kernel does.

Thanks,
drew

Evan Green Aug. 29, 2023, 5:20 p.m. UTC | #13

On Tue, Aug 29, 2023 at 1:48 AM Andrew Jones <[e-mail]> wrote:
>
> Hi Evan,
>
> Here's my stab at new wording.
>
> On Tue, Jul 11, 2023 at 01:18:30PM -0700, Evan Green wrote:
> ...
> > diff --git a/Documentation/riscv/uabi.rst b/Documentation/riscv/uabi.rst
> > index 8960fac42c40..afdda580e5a2 100644
> > --- a/Documentation/riscv/uabi.rst
> > +++ b/Documentation/riscv/uabi.rst
> > @@ -42,6 +42,16 @@ An example string following the order is::
> >
> >     rv64imadc_zifoo_zigoo_zafoo_sbar_scar_zxmbaz_xqux_xrux
> >
> > +"isa" vs "hart isa" lines in /proc/cpuinfo
> > +------------------------------------------
> > +
> > +The "isa" line in /proc/cpuinfo describes the lowest common denominator of
> > +RISC-V ISA extensions understood by the kernel and implemented on all harts. The
> > +"hart isa" line, in contrast, describes the set of extensions understood by the
> > +kernel on the particular hart being described, even if those extensions may not
> > +be present on all harts in the system. The "hart isa" line is consistent with
> > +what's returned by __riscv_hwprobe() when querying for that specific CPU.
> > +
>
> The "isa" and "hart isa" lines in /proc/cpuinfo list RISC-V ISA extensions
> which the kernel can identify (the kernel recognizes the extension's name)
> and have not been filtered out due to effectively not being present.  An
> extension is effectively not present when it is unusable, either due to
> defects (which the kernel is aware of), due to missing information which
> is necessary to complete the extension's description, or due to being
> explicitly "hidden", such as when a kernel command line parameter
> instructs the kernel to pretend the extension is not present.  Note, an
> extension's presence in a list does not imply the kernel is using the
> extension, nor does it imply that userspace or guest kernels may use the
> extension (__riscv_hwprobe() should be queried for userspace usability.
> The hypervisor should be queried, using hypervisor-specific APIs, to
> check guest kernel usability.)
>
> The "isa" line describes the lowest common denominator of extensions,
> which are the extensions implemented on all harts.  In contrast, the
> extensions listed in the "hart isa" line need not be implemented by
> any other hart than the hart corresponding to the line.
>
> ---
>
> I've specifically dropped the 'The "hart isa" line is consistent with
> what's returned by __riscv_hwprobe()...' part because I suspect hwprobe
> could at least lag what gets put in "hart isa", since the kernel may be
> taught about an extension for a different purpose first, neglecting
> hwprobe. And, there may be cases that hwprobe never enumerates an
> extension which the kernel does.

Thanks for this. My v5 had also dropped the hwprobe part. A few thoughts:

 * It seems like you want to make sure we call out the fact that the
kernel may trim out, for various reasons, ISA extensions that the
hardware does in fact support. This seems reasonable, but I don't
think we need to enumerate the complete list of "why" this might
happen, as that list is likely to go stale.
 * The "kernel is using the extension" part is a slightly confusing
perspective in this context, as it sort of implies the kernel has its
own agenda :). I'd expect users looking at /proc/cpuinfo are mostly
trying to figure out what extensions they themselves can use, and the
kernel's behavior factors in only insofar as it's required to support
the user in using a feature. Mostly I guess this is a phrasing nit.
 * The bringing up of guest kernels also seems confusing to me in the
context of /proc/cpuinfo. I'd expect discussions on how host ISA
extensions filter into guest OSes to be in a hypervisor-specifc
document, or at least a section dedicated to virtualization.
 * I hesitated in adding prescriptive guidance on what users should
do, as I think this section will hold up better over time if it just
describes current characteristics, rather than attempting to prescribe
behavior. If we want a prescriptive documentation on "use this for
that", that should probably be its own section

If I try to fold the gist of what you wrote into v5, I get something
like this (also with a very slight section heading change). Let me
know what you think:

"isa" and "hart isa" lines in /proc/cpuinfo
------------------------------------------

The "isa" line in /proc/cpuinfo describes the lowest common denominator of
RISC-V ISA extensions recognized by the kernel and implemented on all harts. The
"hart isa" line, in contrast, describes the set of extensions recognized by the
kernel on the particular hart being described, even if those extensions may not
be present on all harts in the system.

In both lines, the presence of an extension guarantees only that the
hardware has the described capability.
Additional kernel support or policy changes may be required before an
extension's capability is fully usable by userspace programs.

Inversely, the absence of an extension in these lines does not
necessarily mean the hardware does not support that feature. The
running kernel may not recognize the extension, or may have
deliberately disabled access to it.

-Evan

Andrew Jones Aug. 30, 2023, 9:03 a.m. UTC | #14

On Tue, Aug 29, 2023 at 10:20:04AM -0700, Evan Green wrote:
> On Tue, Aug 29, 2023 at 1:48 AM Andrew Jones <[e-mail]> wrote:
> >
> > Hi Evan,
> >
> > Here's my stab at new wording.
> >
> > On Tue, Jul 11, 2023 at 01:18:30PM -0700, Evan Green wrote:
> > ...
> > > diff --git a/Documentation/riscv/uabi.rst b/Documentation/riscv/uabi.rst
> > > index 8960fac42c40..afdda580e5a2 100644
> > > --- a/Documentation/riscv/uabi.rst
> > > +++ b/Documentation/riscv/uabi.rst
> > > @@ -42,6 +42,16 @@ An example string following the order is::
> > >
> > >     rv64imadc_zifoo_zigoo_zafoo_sbar_scar_zxmbaz_xqux_xrux
> > >
> > > +"isa" vs "hart isa" lines in /proc/cpuinfo
> > > +------------------------------------------
> > > +
> > > +The "isa" line in /proc/cpuinfo describes the lowest common denominator of
> > > +RISC-V ISA extensions understood by the kernel and implemented on all harts. The
> > > +"hart isa" line, in contrast, describes the set of extensions understood by the
> > > +kernel on the particular hart being described, even if those extensions may not
> > > +be present on all harts in the system. The "hart isa" line is consistent with
> > > +what's returned by __riscv_hwprobe() when querying for that specific CPU.
> > > +
> >
> > The "isa" and "hart isa" lines in /proc/cpuinfo list RISC-V ISA extensions
> > which the kernel can identify (the kernel recognizes the extension's name)
> > and have not been filtered out due to effectively not being present.  An
> > extension is effectively not present when it is unusable, either due to
> > defects (which the kernel is aware of), due to missing information which
> > is necessary to complete the extension's description, or due to being
> > explicitly "hidden", such as when a kernel command line parameter
> > instructs the kernel to pretend the extension is not present.  Note, an
> > extension's presence in a list does not imply the kernel is using the
> > extension, nor does it imply that userspace or guest kernels may use the
> > extension (__riscv_hwprobe() should be queried for userspace usability.
> > The hypervisor should be queried, using hypervisor-specific APIs, to
> > check guest kernel usability.)
> >
> > The "isa" line describes the lowest common denominator of extensions,
> > which are the extensions implemented on all harts.  In contrast, the
> > extensions listed in the "hart isa" line need not be implemented by
> > any other hart than the hart corresponding to the line.
> >
> > ---
> >
> > I've specifically dropped the 'The "hart isa" line is consistent with
> > what's returned by __riscv_hwprobe()...' part because I suspect hwprobe
> > could at least lag what gets put in "hart isa", since the kernel may be
> > taught about an extension for a different purpose first, neglecting
> > hwprobe. And, there may be cases that hwprobe never enumerates an
> > extension which the kernel does.
> 
> Thanks for this. My v5 had also dropped the hwprobe part. A few thoughts:
> 
>  * It seems like you want to make sure we call out the fact that the
> kernel may trim out, for various reasons, ISA extensions that the
> hardware does in fact support. This seems reasonable, but I don't
> think we need to enumerate the complete list of "why" this might
> happen, as that list is likely to go stale.

I agree it's better to not [try to] list all the possibilities, assuming
we can come up with good, general words to capture the idea.

>  * The "kernel is using the extension" part is a slightly confusing
> perspective in this context, as it sort of implies the kernel has its
> own agenda :). I'd expect users looking at /proc/cpuinfo are mostly
> trying to figure out what extensions they themselves can use, and the
> kernel's behavior factors in only insofar as it's required to support
> the user in using a feature. Mostly I guess this is a phrasing nit.

We'll have plenty of S-mode extensions listed in these strings. Users
who recognize S-mode extensions may want to know if they're listed because
the kernel is applying them (and wouldn't be listed otherwise), or whether
they're listed simply because they exist on the hart(s).

>  * The bringing up of guest kernels also seems confusing to me in the
> context of /proc/cpuinfo. I'd expect discussions on how host ISA
> extensions filter into guest OSes to be in a hypervisor-specifc
> document, or at least a section dedicated to virtualization.

If there weren't S-mode extensions being listed, then I would agree,
but, since there are, it seems odd to not explain what it means for
them to be there wrt host and guest kernels.

>  * I hesitated in adding prescriptive guidance on what users should
> do, as I think this section will hold up better over time if it just
> describes current characteristics, rather than attempting to prescribe
> behavior. If we want a prescriptive documentation on "use this for
> that", that should probably be its own section

I guess the guidance you're referring to is the "(__riscv_hwprobe() should
be queried for userspace usability.  The hypervisor should be queried,
using hypervisor-specific APIs, to check guest kernel usability.)" bit.
I'm fine with dropping that or moving it to another section, but I think
the more we point out hwprobe, the better. If developers are reading this
/proc/cpuinfo section because they want to detect extensions, then I'd
prefer the section redirects them to hwprobe.

> 
> If I try to fold the gist of what you wrote into v5, I get something
> like this (also with a very slight section heading change). Let me
> know what you think:
> 
> "isa" and "hart isa" lines in /proc/cpuinfo
> ------------------------------------------

need one more _

> 
> The "isa" line in /proc/cpuinfo describes the lowest common denominator of
> RISC-V ISA extensions recognized by the kernel and implemented on all harts. The
> "hart isa" line, in contrast, describes the set of extensions recognized by the
> kernel on the particular hart being described, even if those extensions may not
> be present on all harts in the system.
> 
> In both lines, the presence of an extension guarantees only that the
> hardware has the described capability.
> Additional kernel support or policy changes may be required before an
> extension's capability is fully usable by userspace programs.

or guest kernels in the case of S-mode extensions.

> 
> Inversely, the absence of an extension in these lines does not
> necessarily mean the hardware does not support that feature. The
> running kernel may not recognize the extension, or may have
> deliberately disabled access to it.

I'm not sure about the word "disabled". The kernel can only disable U-mode
extensions and S-mode extensions for guests. S-mode extensions for the
current kernel would have to be disabled by its next higher privilege
level.

How about "...may not recognize the extension, or may have deliberately
removed it from the listing."

(But then readers will wonder why an extension would be deliberately
removed from the listing, which brings us back to trying to come up
with general words to capture the cases I listed. Or, maybe we don't
have to care if they wonder why in this section/document.)

Thanks,
drew

Palmer Dabbelt Aug. 30, 2023, 1:24 p.m. UTC | #15

On Tue, 11 Jul 2023 13:18:30 -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.
> 
> [...]

Applied, thanks!

[1/1] RISC-V: Show accurate per-hart isa in /proc/cpuinfo
      https://git.kernel.org/palmer/c/3d44f547b677

Best regards,

Evan Green Aug. 30, 2023, 5:33 p.m. UTC | #16

On Wed, Aug 30, 2023 at 2:03 AM Andrew Jones <[e-mail]> wrote:
>
> On Tue, Aug 29, 2023 at 10:20:04AM -0700, Evan Green wrote:
> > On Tue, Aug 29, 2023 at 1:48 AM Andrew Jones <[e-mail]> wrote:
> > >
> > > Hi Evan,
> > >
> > > Here's my stab at new wording.
> > >
> > > On Tue, Jul 11, 2023 at 01:18:30PM -0700, Evan Green wrote:
> > > ...
> > > > diff --git a/Documentation/riscv/uabi.rst b/Documentation/riscv/uabi.rst
> > > > index 8960fac42c40..afdda580e5a2 100644
> > > > --- a/Documentation/riscv/uabi.rst
> > > > +++ b/Documentation/riscv/uabi.rst
> > > > @@ -42,6 +42,16 @@ An example string following the order is::
> > > >
> > > >     rv64imadc_zifoo_zigoo_zafoo_sbar_scar_zxmbaz_xqux_xrux
> > > >
> > > > +"isa" vs "hart isa" lines in /proc/cpuinfo
> > > > +------------------------------------------
> > > > +
> > > > +The "isa" line in /proc/cpuinfo describes the lowest common denominator of
> > > > +RISC-V ISA extensions understood by the kernel and implemented on all harts. The
> > > > +"hart isa" line, in contrast, describes the set of extensions understood by the
> > > > +kernel on the particular hart being described, even if those extensions may not
> > > > +be present on all harts in the system. The "hart isa" line is consistent with
> > > > +what's returned by __riscv_hwprobe() when querying for that specific CPU.
> > > > +
> > >
> > > The "isa" and "hart isa" lines in /proc/cpuinfo list RISC-V ISA extensions
> > > which the kernel can identify (the kernel recognizes the extension's name)
> > > and have not been filtered out due to effectively not being present.  An
> > > extension is effectively not present when it is unusable, either due to
> > > defects (which the kernel is aware of), due to missing information which
> > > is necessary to complete the extension's description, or due to being
> > > explicitly "hidden", such as when a kernel command line parameter
> > > instructs the kernel to pretend the extension is not present.  Note, an
> > > extension's presence in a list does not imply the kernel is using the
> > > extension, nor does it imply that userspace or guest kernels may use the
> > > extension (__riscv_hwprobe() should be queried for userspace usability.
> > > The hypervisor should be queried, using hypervisor-specific APIs, to
> > > check guest kernel usability.)
> > >
> > > The "isa" line describes the lowest common denominator of extensions,
> > > which are the extensions implemented on all harts.  In contrast, the
> > > extensions listed in the "hart isa" line need not be implemented by
> > > any other hart than the hart corresponding to the line.
> > >
> > > ---
> > >
> > > I've specifically dropped the 'The "hart isa" line is consistent with
> > > what's returned by __riscv_hwprobe()...' part because I suspect hwprobe
> > > could at least lag what gets put in "hart isa", since the kernel may be
> > > taught about an extension for a different purpose first, neglecting
> > > hwprobe. And, there may be cases that hwprobe never enumerates an
> > > extension which the kernel does.
> >
> > Thanks for this. My v5 had also dropped the hwprobe part. A few thoughts:
> >
> >  * It seems like you want to make sure we call out the fact that the
> > kernel may trim out, for various reasons, ISA extensions that the
> > hardware does in fact support. This seems reasonable, but I don't
> > think we need to enumerate the complete list of "why" this might
> > happen, as that list is likely to go stale.
>
> I agree it's better to not [try to] list all the possibilities, assuming
> we can come up with good, general words to capture the idea.
>
> >  * The "kernel is using the extension" part is a slightly confusing
> > perspective in this context, as it sort of implies the kernel has its
> > own agenda :). I'd expect users looking at /proc/cpuinfo are mostly
> > trying to figure out what extensions they themselves can use, and the
> > kernel's behavior factors in only insofar as it's required to support
> > the user in using a feature. Mostly I guess this is a phrasing nit.
>
> We'll have plenty of S-mode extensions listed in these strings. Users
> who recognize S-mode extensions may want to know if they're listed because
> the kernel is applying them (and wouldn't be listed otherwise), or whether
> they're listed simply because they exist on the hart(s).

I see. You're right I was thinking only about U-mode extensions.

>
> >  * The bringing up of guest kernels also seems confusing to me in the
> > context of /proc/cpuinfo. I'd expect discussions on how host ISA
> > extensions filter into guest OSes to be in a hypervisor-specifc
> > document, or at least a section dedicated to virtualization.
>
> If there weren't S-mode extensions being listed, then I would agree,
> but, since there are, it seems odd to not explain what it means for
> them to be there wrt host and guest kernels.

I'm not a virtualization guy, but my impression was people didn't have
expectations that everything they saw in cpuinfo would be wholesale
presented to guest VMs. There's always that layer of hypervisor
configuration that may strip out some features. So I'm still not super
convinced guest VMs need a carveout/caveat here, but let me see if I
can fold it in.

>
> >  * I hesitated in adding prescriptive guidance on what users should
> > do, as I think this section will hold up better over time if it just
> > describes current characteristics, rather than attempting to prescribe
> > behavior. If we want a prescriptive documentation on "use this for
> > that", that should probably be its own section
>
> I guess the guidance you're referring to is the "(__riscv_hwprobe() should
> be queried for userspace usability.  The hypervisor should be queried,
> using hypervisor-specific APIs, to check guest kernel usability.)" bit.
> I'm fine with dropping that or moving it to another section, but I think
> the more we point out hwprobe, the better. If developers are reading this
> /proc/cpuinfo section because they want to detect extensions, then I'd
> prefer the section redirects them to hwprobe.

Fair enough. I still haven't brought myself to wedge in an ad for
hwprobe, but I also don't disagree with this :)

>
> >
> > If I try to fold the gist of what you wrote into v5, I get something
> > like this (also with a very slight section heading change). Let me
> > know what you think:
> >
> > "isa" and "hart isa" lines in /proc/cpuinfo
> > ------------------------------------------
>
> need one more _
>
> >
> > The "isa" line in /proc/cpuinfo describes the lowest common denominator of
> > RISC-V ISA extensions recognized by the kernel and implemented on all harts. The
> > "hart isa" line, in contrast, describes the set of extensions recognized by the
> > kernel on the particular hart being described, even if those extensions may not
> > be present on all harts in the system.
> >
> > In both lines, the presence of an extension guarantees only that the
> > hardware has the described capability.
> > Additional kernel support or policy changes may be required before an
> > extension's capability is fully usable by userspace programs.
>
> or guest kernels in the case of S-mode extensions.
>
> >
> > Inversely, the absence of an extension in these lines does not
> > necessarily mean the hardware does not support that feature. The
> > running kernel may not recognize the extension, or may have
> > deliberately disabled access to it.
>
> I'm not sure about the word "disabled". The kernel can only disable U-mode
> extensions and S-mode extensions for guests. S-mode extensions for the
> current kernel would have to be disabled by its next higher privilege
> level.
>
> How about "...may not recognize the extension, or may have deliberately
> removed it from the listing."

Perfect.

>
> (But then readers will wonder why an extension would be deliberately
> removed from the listing, which brings us back to trying to come up
> with general words to capture the cases I listed. Or, maybe we don't
> have to care if they wonder why in this section/document.)

I know, I felt the same pull to explain why as well. But I think given
that our goal with this section is mostly to make the distinction of
"presence != active", explaining exactly why the kernel may remove it
is not strictly necessary. All of my attempts to tack something on end
up being enumerated lists, which don't come out well and muddle the
message.

Here's another shot (line endings may be wonky):

"isa" and "hart isa" lines in /proc/cpuinfo
-------------------------------------------

The "isa" line in /proc/cpuinfo describes the lowest common denominator of
RISC-V ISA extensions recognized by the kernel and implemented on all harts. The
"hart isa" line, in contrast, describes the set of extensions recognized by the
kernel on the particular hart being described, even if those extensions may not
be present on all harts in the system.

In both lines, the presence of an extension guarantees only that the
hardware has the described capability.
Additional kernel support or policy changes may be required before an
extension's capability is fully usable by userspace programs.
Similarly, for S-mode extensions, presence in one of these lines does
not guarantee that the kernel is taking advantage of the extension, or
that the feature will be visible in guest VMs managed by this kernel.

Inversely, the absence of an extension in these lines does not
necessarily mean the hardware does not support that feature. The
running kernel may not recognize the extension, or may have
deliberately removed it from the listing.

-Evan

>
> Thanks,
> drew

Andrew Jones Aug. 30, 2023, 5:55 p.m. UTC | #17

On Wed, Aug 30, 2023 at 10:33:04AM -0700, Evan Green wrote:
> On Wed, Aug 30, 2023 at 2:03 AM Andrew Jones <[e-mail]> wrote:
> >
> > On Tue, Aug 29, 2023 at 10:20:04AM -0700, Evan Green wrote:
> > > On Tue, Aug 29, 2023 at 1:48 AM Andrew Jones <[e-mail]> wrote:
> > > >
> > > > Hi Evan,
> > > >
> > > > Here's my stab at new wording.
> > > >
> > > > On Tue, Jul 11, 2023 at 01:18:30PM -0700, Evan Green wrote:
> > > > ...
> > > > > diff --git a/Documentation/riscv/uabi.rst b/Documentation/riscv/uabi.rst
> > > > > index 8960fac42c40..afdda580e5a2 100644
> > > > > --- a/Documentation/riscv/uabi.rst
> > > > > +++ b/Documentation/riscv/uabi.rst
> > > > > @@ -42,6 +42,16 @@ An example string following the order is::
> > > > >
> > > > >     rv64imadc_zifoo_zigoo_zafoo_sbar_scar_zxmbaz_xqux_xrux
> > > > >
> > > > > +"isa" vs "hart isa" lines in /proc/cpuinfo
> > > > > +------------------------------------------
> > > > > +
> > > > > +The "isa" line in /proc/cpuinfo describes the lowest common denominator of
> > > > > +RISC-V ISA extensions understood by the kernel and implemented on all harts. The
> > > > > +"hart isa" line, in contrast, describes the set of extensions understood by the
> > > > > +kernel on the particular hart being described, even if those extensions may not
> > > > > +be present on all harts in the system. The "hart isa" line is consistent with
> > > > > +what's returned by __riscv_hwprobe() when querying for that specific CPU.
> > > > > +
> > > >
> > > > The "isa" and "hart isa" lines in /proc/cpuinfo list RISC-V ISA extensions
> > > > which the kernel can identify (the kernel recognizes the extension's name)
> > > > and have not been filtered out due to effectively not being present.  An
> > > > extension is effectively not present when it is unusable, either due to
> > > > defects (which the kernel is aware of), due to missing information which
> > > > is necessary to complete the extension's description, or due to being
> > > > explicitly "hidden", such as when a kernel command line parameter
> > > > instructs the kernel to pretend the extension is not present.  Note, an
> > > > extension's presence in a list does not imply the kernel is using the
> > > > extension, nor does it imply that userspace or guest kernels may use the
> > > > extension (__riscv_hwprobe() should be queried for userspace usability.
> > > > The hypervisor should be queried, using hypervisor-specific APIs, to
> > > > check guest kernel usability.)
> > > >
> > > > The "isa" line describes the lowest common denominator of extensions,
> > > > which are the extensions implemented on all harts.  In contrast, the
> > > > extensions listed in the "hart isa" line need not be implemented by
> > > > any other hart than the hart corresponding to the line.
> > > >
> > > > ---
> > > >
> > > > I've specifically dropped the 'The "hart isa" line is consistent with
> > > > what's returned by __riscv_hwprobe()...' part because I suspect hwprobe
> > > > could at least lag what gets put in "hart isa", since the kernel may be
> > > > taught about an extension for a different purpose first, neglecting
> > > > hwprobe. And, there may be cases that hwprobe never enumerates an
> > > > extension which the kernel does.
> > >
> > > Thanks for this. My v5 had also dropped the hwprobe part. A few thoughts:
> > >
> > >  * It seems like you want to make sure we call out the fact that the
> > > kernel may trim out, for various reasons, ISA extensions that the
> > > hardware does in fact support. This seems reasonable, but I don't
> > > think we need to enumerate the complete list of "why" this might
> > > happen, as that list is likely to go stale.
> >
> > I agree it's better to not [try to] list all the possibilities, assuming
> > we can come up with good, general words to capture the idea.
> >
> > >  * The "kernel is using the extension" part is a slightly confusing
> > > perspective in this context, as it sort of implies the kernel has its
> > > own agenda :). I'd expect users looking at /proc/cpuinfo are mostly
> > > trying to figure out what extensions they themselves can use, and the
> > > kernel's behavior factors in only insofar as it's required to support
> > > the user in using a feature. Mostly I guess this is a phrasing nit.
> >
> > We'll have plenty of S-mode extensions listed in these strings. Users
> > who recognize S-mode extensions may want to know if they're listed because
> > the kernel is applying them (and wouldn't be listed otherwise), or whether
> > they're listed simply because they exist on the hart(s).
> 
> I see. You're right I was thinking only about U-mode extensions.
> 
> >
> > >  * The bringing up of guest kernels also seems confusing to me in the
> > > context of /proc/cpuinfo. I'd expect discussions on how host ISA
> > > extensions filter into guest OSes to be in a hypervisor-specifc
> > > document, or at least a section dedicated to virtualization.
> >
> > If there weren't S-mode extensions being listed, then I would agree,
> > but, since there are, it seems odd to not explain what it means for
> > them to be there wrt host and guest kernels.
> 
> I'm not a virtualization guy, but my impression was people didn't have
> expectations that everything they saw in cpuinfo would be wholesale
> presented to guest VMs. There's always that layer of hypervisor
> configuration that may strip out some features. So I'm still not super
> convinced guest VMs need a carveout/caveat here, but let me see if I
> can fold it in.
> 
> >
> > >  * I hesitated in adding prescriptive guidance on what users should
> > > do, as I think this section will hold up better over time if it just
> > > describes current characteristics, rather than attempting to prescribe
> > > behavior. If we want a prescriptive documentation on "use this for
> > > that", that should probably be its own section
> >
> > I guess the guidance you're referring to is the "(__riscv_hwprobe() should
> > be queried for userspace usability.  The hypervisor should be queried,
> > using hypervisor-specific APIs, to check guest kernel usability.)" bit.
> > I'm fine with dropping that or moving it to another section, but I think
> > the more we point out hwprobe, the better. If developers are reading this
> > /proc/cpuinfo section because they want to detect extensions, then I'd
> > prefer the section redirects them to hwprobe.
> 
> Fair enough. I still haven't brought myself to wedge in an ad for
> hwprobe, but I also don't disagree with this :)
> 
> >
> > >
> > > If I try to fold the gist of what you wrote into v5, I get something
> > > like this (also with a very slight section heading change). Let me
> > > know what you think:
> > >
> > > "isa" and "hart isa" lines in /proc/cpuinfo
> > > ------------------------------------------
> >
> > need one more _
> >
> > >
> > > The "isa" line in /proc/cpuinfo describes the lowest common denominator of
> > > RISC-V ISA extensions recognized by the kernel and implemented on all harts. The
> > > "hart isa" line, in contrast, describes the set of extensions recognized by the
> > > kernel on the particular hart being described, even if those extensions may not
> > > be present on all harts in the system.
> > >
> > > In both lines, the presence of an extension guarantees only that the
> > > hardware has the described capability.
> > > Additional kernel support or policy changes may be required before an
> > > extension's capability is fully usable by userspace programs.
> >
> > or guest kernels in the case of S-mode extensions.
> >
> > >
> > > Inversely, the absence of an extension in these lines does not
> > > necessarily mean the hardware does not support that feature. The
> > > running kernel may not recognize the extension, or may have
> > > deliberately disabled access to it.
> >
> > I'm not sure about the word "disabled". The kernel can only disable U-mode
> > extensions and S-mode extensions for guests. S-mode extensions for the
> > current kernel would have to be disabled by its next higher privilege
> > level.
> >
> > How about "...may not recognize the extension, or may have deliberately
> > removed it from the listing."
> 
> Perfect.
> 
> >
> > (But then readers will wonder why an extension would be deliberately
> > removed from the listing, which brings us back to trying to come up
> > with general words to capture the cases I listed. Or, maybe we don't
> > have to care if they wonder why in this section/document.)
> 
> I know, I felt the same pull to explain why as well. But I think given
> that our goal with this section is mostly to make the distinction of
> "presence != active", explaining exactly why the kernel may remove it
> is not strictly necessary. All of my attempts to tack something on end
> up being enumerated lists, which don't come out well and muddle the
> message.
> 
> Here's another shot (line endings may be wonky):
> 
> "isa" and "hart isa" lines in /proc/cpuinfo
> -------------------------------------------
> 
> The "isa" line in /proc/cpuinfo describes the lowest common denominator of
> RISC-V ISA extensions recognized by the kernel and implemented on all harts. The
> "hart isa" line, in contrast, describes the set of extensions recognized by the
> kernel on the particular hart being described, even if those extensions may not
> be present on all harts in the system.
> 
> In both lines, the presence of an extension guarantees only that the
> hardware has the described capability.
> Additional kernel support or policy changes may be required before an
> extension's capability is fully usable by userspace programs.
> Similarly, for S-mode extensions, presence in one of these lines does
> not guarantee that the kernel is taking advantage of the extension, or
> that the feature will be visible in guest VMs managed by this kernel.
> 
> Inversely, the absence of an extension in these lines does not
> necessarily mean the hardware does not support that feature. The
> running kernel may not recognize the extension, or may have
> deliberately removed it from the listing.

Looks good to me!

Thanks,
drew

Patch

diff --git a/Documentation/riscv/uabi.rst b/Documentation/riscv/uabi.rst
index 8960fac42c40..afdda580e5a2 100644
--- a/Documentation/riscv/uabi.rst
+++ b/Documentation/riscv/uabi.rst
@@ -42,6 +42,16 @@  An example string following the order is::
 
    rv64imadc_zifoo_zigoo_zafoo_sbar_scar_zxmbaz_xqux_xrux
 
+"isa" vs "hart isa" lines in /proc/cpuinfo
+------------------------------------------
+
+The "isa" line in /proc/cpuinfo describes the lowest common denominator of
+RISC-V ISA extensions understood by the kernel and implemented on all harts. The
+"hart isa" line, in contrast, describes the set of extensions understood by the
+kernel on the particular hart being described, even if those extensions may not
+be present on all harts in the system. The "hart isa" line is consistent with
+what's returned by __riscv_hwprobe() when querying for that specific CPU.
+
 Misaligned accesses
 -------------------
 
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;