diff mbox series

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

Message ID 20230825231139.1145522-1-evan@rivosinc.com (mailing list archive)
State Changes Requested
Headers show
Series [v5] RISC-V: Show accurate per-hart isa in /proc/cpuinfo | expand

Checks

Context Check Description
conchuod/cover_letter success Single patches do not need cover letters
conchuod/tree_selection success Guessed tree name to be for-next at HEAD 9f944d2e0ab3
conchuod/fixes_present success Fixes tag not required for -next series
conchuod/maintainers_pattern success MAINTAINERS pattern errors before the patch: 4 and now 4
conchuod/verify_signedoff success Signed-off-by tag matches author and committer
conchuod/kdoc success Errors and warnings before: 0 this patch: 0
conchuod/build_rv64_clang_allmodconfig success Errors and warnings before: 9 this patch: 9
conchuod/module_param success Was 0 now: 0
conchuod/build_rv64_gcc_allmodconfig success Errors and warnings before: 10 this patch: 10
conchuod/build_rv32_defconfig success Build OK
conchuod/dtb_warn_rv64 success Errors and warnings before: 12 this patch: 12
conchuod/header_inline success No static functions without inline keyword in header files
conchuod/checkpatch success total: 0 errors, 0 warnings, 0 checks, 65 lines checked
conchuod/build_rv64_nommu_k210_defconfig success Build OK
conchuod/verify_fixes success No Fixes tag
conchuod/build_rv64_nommu_virt_defconfig success Build OK

Commit Message

Evan Green Aug. 25, 2023, 11:11 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.

Signed-off-by: Evan Green <evan@rivosinc.com>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

---

Changes in v5:
 - Documentation changes (only) (Conor)

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)

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

Comments

Conor Dooley Aug. 25, 2023, 11:26 p.m. UTC | #1 
On Fri, Aug 25, 2023 at 04:11:38PM -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.
> 
> Signed-off-by: Evan Green <evan@rivosinc.com>
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Can you drop this if you repost?

> +"isa" vs "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 cases, the presence of a feature
> +in these lines guarantees only that the hardware has the described capability.
> +Additional kernel support or policy control changes may be required before a
> +feature is fully usable by userspace programs.

I do not think that "in both cases" matches the expectations of
userspace for the existing line. It's too late at night for me to think
properly, but I think our existing implementation does work like you
have documented for FD/V. I think I previously mentioned that it could
misreport things for vector during the review of the vector series but
forgot about it until now.
Andrew Jones Aug. 26, 2023, 8:07 a.m. UTC | #2 
On Fri, Aug 25, 2023 at 04:11:38PM -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.
> 
> Signed-off-by: Evan Green <evan@rivosinc.com>
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> 
> ---
> 
> Changes in v5:
>  - Documentation changes (only) (Conor)
> 
> 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)
> 
>  Documentation/riscv/uabi.rst | 12 ++++++++++++
>  arch/riscv/kernel/cpu.c      | 22 ++++++++++++++++++----
>  2 files changed, 30 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/riscv/uabi.rst b/Documentation/riscv/uabi.rst
> index 8960fac42c40..a46017f57db2 100644
> --- a/Documentation/riscv/uabi.rst
> +++ b/Documentation/riscv/uabi.rst
> @@ -42,6 +42,18 @@ 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 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 cases, the presence of a feature
> +in these lines guarantees only that the hardware has the described capability.
> +Additional kernel support or policy control changes may be required before a
> +feature is fully usable by userspace programs.

These last words imply that extensions listed in /proc/cpuinfo are all
potentially usable by userspace, but we also advertise extensions which
are only usable in S-mode (which is fine because of VMs). We should just
make that clear here.

Also, I frequently bounce between the words 'feature' and 'extension',
but, for documentation, I think we should try to be consistent. Or, we
could write somewhere that 'feature' == 'extension'.

Thanks,
drew

> +
>  Misaligned accesses
>  -------------------
>  
> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> index 7b793c4321bb..100fb382b450 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
>
Conor Dooley Aug. 26, 2023, 9:56 a.m. UTC | #3 
On Sat, Aug 26, 2023 at 12:26:25AM +0100, Conor Dooley wrote:
> On Fri, Aug 25, 2023 at 04:11:38PM -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.
> > 
> > Signed-off-by: Evan Green <evan@rivosinc.com>
> > Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
> 
> > Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> 
> Can you drop this if you repost?
> 
> > +"isa" vs "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 cases, the presence of a feature
> > +in these lines guarantees only that the hardware has the described capability.
> > +Additional kernel support or policy control changes may be required before a
> > +feature is fully usable by userspace programs.
> 
> I do not think that "in both cases" matches the expectations of
> userspace for the existing line. It's too late at night for me to think
> properly, but I think our existing implementation does work like you
> have documented for FD/V. I think I previously mentioned that it could
> misreport things for vector during the review of the vector series but
> forgot about it until now.

I went and checked, and yes it does currently do that for vector. I
don't think that that is what userspace would expect, that Google
cpu_features project for example would draw incorrect conclusions.
Evan Green Aug. 28, 2023, 4:24 p.m. UTC | #4 
On Sat, Aug 26, 2023 at 2:56 AM Conor Dooley <conor@kernel.org> wrote:
>
> On Sat, Aug 26, 2023 at 12:26:25AM +0100, Conor Dooley wrote:
> > On Fri, Aug 25, 2023 at 04:11:38PM -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.
> > >
> > > Signed-off-by: Evan Green <evan@rivosinc.com>
> > > Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
> >
> > > Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> >
> > Can you drop this if you repost?

Will do.

> >
> > > +"isa" vs "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 cases, the presence of a feature
> > > +in these lines guarantees only that the hardware has the described capability.
> > > +Additional kernel support or policy control changes may be required before a
> > > +feature is fully usable by userspace programs.
> >
> > I do not think that "in both cases" matches the expectations of
> > userspace for the existing line. It's too late at night for me to think
> > properly, but I think our existing implementation does work like you
> > have documented for FD/V. I think I previously mentioned that it could
> > misreport things for vector during the review of the vector series but
> > forgot about it until now.
>
> I went and checked, and yes it does currently do that for vector. I
> don't think that that is what userspace would expect, that Google
> cpu_features project for example would draw incorrect conclusions.

I'm lost, could you explain a little more? My goal was to say that
there's no blanket guarantee that the feature is 100% ready to go for
userspace just because it's seen here. For some extensions, it may in
fact end up meaning just that (hence the "additional ... may be
required" rather than "is required"). This is true for FD (maybe,
depending on history?), or extensions whose minimal/zero kernel
support was unconditionally added at the same time as its parsing for
it. But it's not true solely by virtue of being in /proc/cpuinfo. In
other words, I'm trying to establish the floor of what /proc/cpuinfo
guarantees, without fully specifying the ceiling. Are you saying that
we need to spell out the guarantees for each extension? Or are you
saying the floor I've defined in general is incorrect or insufficient?
I'm also open to direct suggestions of wording if you've got something
in mind :)
-Evan
Conor Dooley Aug. 28, 2023, 4:53 p.m. UTC | #5 
On Mon, Aug 28, 2023 at 09:24:03AM -0700, Evan Green wrote:
> On Sat, Aug 26, 2023 at 2:56 AM Conor Dooley <conor@kernel.org> wrote:
> >
> > On Sat, Aug 26, 2023 at 12:26:25AM +0100, Conor Dooley wrote:
> > > On Fri, Aug 25, 2023 at 04:11:38PM -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.
> > > >
> > > > Signed-off-by: Evan Green <evan@rivosinc.com>
> > > > Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
> > >
> > > > Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> > >
> > > Can you drop this if you repost?
> 
> Will do.
> 
> > >
> > > > +"isa" vs "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 cases, the presence of a feature
> > > > +in these lines guarantees only that the hardware has the described capability.
> > > > +Additional kernel support or policy control changes may be required before a
> > > > +feature is fully usable by userspace programs.
> > >
> > > I do not think that "in both cases" matches the expectations of
> > > userspace for the existing line. It's too late at night for me to think
> > > properly, but I think our existing implementation does work like you
> > > have documented for FD/V. I think I previously mentioned that it could
> > > misreport things for vector during the review of the vector series but
> > > forgot about it until now.
> >
> > I went and checked, and yes it does currently do that for vector. I
> > don't think that that is what userspace would expect, that Google
> > cpu_features project for example would draw incorrect conclusions.
> 
> I'm lost, could you explain a little more?

There (may be/)are userspace programs that will interpret the appearance
of extensions in cpuinfo as meaning they can be used without performing
any further checks.

> My goal was to say that
> there's no blanket guarantee that the feature is 100% ready to go for
> userspace just because it's seen here.

Right. I was agreeing that this is true, but it is also not how some
userspace programs have interpreted things. Consider a platform & kernel
that support the V extension but vector has not been enabled by default
or by early userspace. If someone cats cpuinfo, they'll see v there, but
it won't be usable. That Google cpu_features project (or a punter) may
then assume they can use it, as that's been the case so far in general*.

The caveat producing the * being that the same problem actually exists
for F/D too AFAICT, but it's likely that nobody really encountered it
as they didn't build non-FP userspaces & then try to use FP in some
userspace applications.

> For some extensions, it may in
> fact end up meaning just that (hence the "additional ... may be
> required" rather than "is required").

> This is true for FD (maybe,
> depending on history?),

AFAICT, it's not true for FD. The FPU config option not being set, or
either of F and D being missing will lead to unusable extensions
appearing.

> or extensions whose minimal/zero kernel
> support was unconditionally added at the same time as its parsing for
> it. But it's not true solely by virtue of being in /proc/cpuinfo. In
> other words, I'm trying to establish the floor of what /proc/cpuinfo
> guarantees, without fully specifying the ceiling.

> Are you saying that
> we need to spell out the guarantees for each extension?

No, I don't want that!

> Or are you
> saying the floor I've defined in general is incorrect or insufficient?

I think the floor that you have defined is probably misleading to users.
It's also the floor that has existed for quite a while, so this might be
a case of the userspace devs messing up due to an absence of any
explanation of what to do here.
Things will get abhorrently messy if we try to do what these userspace
programs expect, and I don't think we should go there. We just need to
bear in mind that the behaviour we have & the behaviour that you are
documenting flys in the face of what some userspace expects.

> I'm also open to direct suggestions of wording if you've got something
> in mind :)

Someone mentioned it recently, but it really is starting to feel more
and more like lscpu should grow support for hwprobe and funnel people
into using that instead of /proc/cpuinfo when all they want is to see
what their hardware can do.
Evan Green Aug. 28, 2023, 5:18 p.m. UTC | #6 
On Mon, Aug 28, 2023 at 9:53 AM Conor Dooley <conor@kernel.org> wrote:
>
> On Mon, Aug 28, 2023 at 09:24:03AM -0700, Evan Green wrote:
> > On Sat, Aug 26, 2023 at 2:56 AM Conor Dooley <conor@kernel.org> wrote:
> > >
> > > On Sat, Aug 26, 2023 at 12:26:25AM +0100, Conor Dooley wrote:
> > > > On Fri, Aug 25, 2023 at 04:11:38PM -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.
> > > > >
> > > > > Signed-off-by: Evan Green <evan@rivosinc.com>
> > > > > Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
> > > >
> > > > > Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> > > >
> > > > Can you drop this if you repost?
> >
> > Will do.
> >
> > > >
> > > > > +"isa" vs "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 cases, the presence of a feature
> > > > > +in these lines guarantees only that the hardware has the described capability.
> > > > > +Additional kernel support or policy control changes may be required before a
> > > > > +feature is fully usable by userspace programs.
> > > >
> > > > I do not think that "in both cases" matches the expectations of
> > > > userspace for the existing line. It's too late at night for me to think
> > > > properly, but I think our existing implementation does work like you
> > > > have documented for FD/V. I think I previously mentioned that it could
> > > > misreport things for vector during the review of the vector series but
> > > > forgot about it until now.
> > >
> > > I went and checked, and yes it does currently do that for vector. I
> > > don't think that that is what userspace would expect, that Google
> > > cpu_features project for example would draw incorrect conclusions.
> >
> > I'm lost, could you explain a little more?
>
> There (may be/)are userspace programs that will interpret the appearance
> of extensions in cpuinfo as meaning they can be used without performing
> any further checks.
>
> > My goal was to say that
> > there's no blanket guarantee that the feature is 100% ready to go for
> > userspace just because it's seen here.
>
> Right. I was agreeing that this is true, but it is also not how some
> userspace programs have interpreted things. Consider a platform & kernel
> that support the V extension but vector has not been enabled by default
> or by early userspace. If someone cats cpuinfo, they'll see v there, but
> it won't be usable. That Google cpu_features project (or a punter) may
> then assume they can use it, as that's been the case so far in general*.
>
> The caveat producing the * being that the same problem actually exists
> for F/D too AFAICT, but it's likely that nobody really encountered it
> as they didn't build non-FP userspaces & then try to use FP in some
> userspace applications.
>
> > For some extensions, it may in
> > fact end up meaning just that (hence the "additional ... may be
> > required" rather than "is required").
>
> > This is true for FD (maybe,
> > depending on history?),
>
> AFAICT, it's not true for FD. The FPU config option not being set, or
> either of F and D being missing will lead to unusable extensions
> appearing.

Ah ok.

>
> > or extensions whose minimal/zero kernel
> > support was unconditionally added at the same time as its parsing for
> > it. But it's not true solely by virtue of being in /proc/cpuinfo. In
> > other words, I'm trying to establish the floor of what /proc/cpuinfo
> > guarantees, without fully specifying the ceiling.
>
> > Are you saying that
> > we need to spell out the guarantees for each extension?
>
> No, I don't want that!
>
> > Or are you
> > saying the floor I've defined in general is incorrect or insufficient?
>
> I think the floor that you have defined is probably misleading to users.
> It's also the floor that has existed for quite a while, so this might be
> a case of the userspace devs messing up due to an absence of any
> explanation of what to do here.
> Things will get abhorrently messy if we try to do what these userspace
> programs expect, and I don't think we should go there. We just need to
> bear in mind that the behaviour we have & the behaviour that you are
> documenting flys in the face of what some userspace expects.

Thanks, I think I understand now. You're saying the floor I'm defining
might surprise some users, who were expecting the floor to be "fully
enabled and ready to party". Given there was no documentation about it
before, and this documentation is consistent with what we actually do
(and there seems to be consensus this is a maintainable position to
hold), can we just tell those users they're holding it wrong?

>
> > I'm also open to direct suggestions of wording if you've got something
> > in mind :)
>
> Someone mentioned it recently, but it really is starting to feel more
> and more like lscpu should grow support for hwprobe and funnel people
> into using that instead of /proc/cpuinfo when all they want is to see
> what their hardware can do.

Maybe for the fiddly microarchitectural bits, yeah. But I'd think our
newly proposed documentation for /proc/cpuinfo of keeping it closer to
what the hardware can do would suit the lscpu folks' mission well. (In
ChromeOS at least, we didn't have lscpu, but snarfed /proc/cpuinfo
directly into feedback reports that consented to sending along system
info). Really I'd think it's the application/library writers who want
to know "am I ready to go right now" are who we should be pushing to
use hwprobe, since we can define those bits to be as specific as we
want (eg V is on AND it's a full moon, so go for it).

Depending on your thoughts on this, if there are changes requested on
this patch, let me know what they are.
-Evan
Conor Dooley Aug. 28, 2023, 5:58 p.m. UTC | #7 
On Mon, Aug 28, 2023 at 10:18:24AM -0700, Evan Green wrote:
> On Mon, Aug 28, 2023 at 9:53 AM Conor Dooley <conor@kernel.org> wrote:
> >
> > On Mon, Aug 28, 2023 at 09:24:03AM -0700, Evan Green wrote:
> > > On Sat, Aug 26, 2023 at 2:56 AM Conor Dooley <conor@kernel.org> wrote:
> > > >
> > > > On Sat, Aug 26, 2023 at 12:26:25AM +0100, Conor Dooley wrote:
> > > > > On Fri, Aug 25, 2023 at 04:11:38PM -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.
> > > > > >
> > > > > > Signed-off-by: Evan Green <evan@rivosinc.com>
> > > > > > Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
> > > > >
> > > > > > Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> > > > >
> > > > > Can you drop this if you repost?
> > >
> > > Will do.
> > >
> > > > >
> > > > > > +"isa" vs "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 cases, the presence of a feature
> > > > > > +in these lines guarantees only that the hardware has the described capability.
> > > > > > +Additional kernel support or policy control changes may be required before a
> > > > > > +feature is fully usable by userspace programs.
> > > > >
> > > > > I do not think that "in both cases" matches the expectations of
> > > > > userspace for the existing line. It's too late at night for me to think
> > > > > properly, but I think our existing implementation does work like you
> > > > > have documented for FD/V. I think I previously mentioned that it could
> > > > > misreport things for vector during the review of the vector series but
> > > > > forgot about it until now.
> > > >
> > > > I went and checked, and yes it does currently do that for vector. I
> > > > don't think that that is what userspace would expect, that Google
> > > > cpu_features project for example would draw incorrect conclusions.
> > >
> > > I'm lost, could you explain a little more?
> >
> > There (may be/)are userspace programs that will interpret the appearance
> > of extensions in cpuinfo as meaning they can be used without performing
> > any further checks.
> >
> > > My goal was to say that
> > > there's no blanket guarantee that the feature is 100% ready to go for
> > > userspace just because it's seen here.
> >
> > Right. I was agreeing that this is true, but it is also not how some
> > userspace programs have interpreted things. Consider a platform & kernel
> > that support the V extension but vector has not been enabled by default
> > or by early userspace. If someone cats cpuinfo, they'll see v there, but
> > it won't be usable. That Google cpu_features project (or a punter) may
> > then assume they can use it, as that's been the case so far in general*.
> >
> > The caveat producing the * being that the same problem actually exists
> > for F/D too AFAICT, but it's likely that nobody really encountered it
> > as they didn't build non-FP userspaces & then try to use FP in some
> > userspace applications.
> >
> > > For some extensions, it may in
> > > fact end up meaning just that (hence the "additional ... may be
> > > required" rather than "is required").
> >
> > > This is true for FD (maybe,
> > > depending on history?),
> >
> > AFAICT, it's not true for FD. The FPU config option not being set, or
> > either of F and D being missing will lead to unusable extensions
> > appearing.
> 
> Ah ok.
> 
> >
> > > or extensions whose minimal/zero kernel
> > > support was unconditionally added at the same time as its parsing for
> > > it. But it's not true solely by virtue of being in /proc/cpuinfo. In
> > > other words, I'm trying to establish the floor of what /proc/cpuinfo
> > > guarantees, without fully specifying the ceiling.
> >
> > > Are you saying that
> > > we need to spell out the guarantees for each extension?
> >
> > No, I don't want that!
> >
> > > Or are you
> > > saying the floor I've defined in general is incorrect or insufficient?
> >
> > I think the floor that you have defined is probably misleading to users.
> > It's also the floor that has existed for quite a while, so this might be
> > a case of the userspace devs messing up due to an absence of any
> > explanation of what to do here.
> > Things will get abhorrently messy if we try to do what these userspace
> > programs expect, and I don't think we should go there. We just need to
> > bear in mind that the behaviour we have & the behaviour that you are
> > documenting flys in the face of what some userspace expects.
> 
> Thanks, I think I understand now. You're saying the floor I'm defining
> might surprise some users, who were expecting the floor to be "fully
> enabled and ready to party".

Yes.

> Given there was no documentation about it
> before, and this documentation is consistent with what we actually do
> (and there seems to be consensus this is a maintainable position to
> hold), can we just tell those users they're holding it wrong?

I think we have to. Dealing with the vector prctls & sysctls here would
be horrid, as I discussed with Andy on one of the versions of that
series.

> > > I'm also open to direct suggestions of wording if you've got something
> > > in mind :)
> >
> > Someone mentioned it recently, but it really is starting to feel more
> > and more like lscpu should grow support for hwprobe and funnel people
> > into using that instead of /proc/cpuinfo when all they want is to see
> > what their hardware can do.
> 
> Maybe for the fiddly microarchitectural bits, yeah. But I'd think our
> newly proposed documentation for /proc/cpuinfo of keeping it closer to
> what the hardware can do would suit the lscpu folks' mission well. (In
> ChromeOS at least, we didn't have lscpu, but snarfed /proc/cpuinfo
> directly into feedback reports that consented to sending along system
> info). Really I'd think it's the application/library writers who want
> to know "am I ready to go right now" are who we should be pushing to
> use hwprobe, since we can define those bits to be as specific as we
> want (eg V is on AND it's a full moon, so go for it).
> 
> Depending on your thoughts on this, if there are changes requested on
> this patch, let me know what they are.

Nah, I think it's fine & the r-b I left on the previous version can stay
:)
diff mbox series

Patch

diff --git a/Documentation/riscv/uabi.rst b/Documentation/riscv/uabi.rst
index 8960fac42c40..a46017f57db2 100644
--- a/Documentation/riscv/uabi.rst
+++ b/Documentation/riscv/uabi.rst
@@ -42,6 +42,18 @@  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 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 cases, the presence of a feature
+in these lines guarantees only that the hardware has the described capability.
+Additional kernel support or policy control changes may be required before a
+feature is fully usable by userspace programs.
+
 Misaligned accesses
 -------------------
 
diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
index 7b793c4321bb..100fb382b450 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;