Message-ID: <20230826-323be460a0dcd2a5fc2917b9@orel>
Date:   Sat, 26 Aug 2023 10:07:50 +0200
From:   Andrew Jones <ajones@...tanamicro.com>
To:     Evan Green <evan@...osinc.com>
Cc:     Palmer Dabbelt <palmer@...osinc.com>,
        Conor Dooley <conor.dooley@...rochip.com>,
        Albert Ou <aou@...s.berkeley.edu>,
        Anup Patel <apatel@...tanamicro.com>,
        Bagas Sanjaya <bagasdotme@...il.com>,
        Heiko Stuebner <heiko@...ech.de>,
        Jonathan Corbet <corbet@....net>,
        Palmer Dabbelt <palmer@...belt.com>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Sunil V L <sunilvl@...tanamicro.com>,
        linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-riscv@...ts.infradead.org
Subject: Re: [PATCH v5] RISC-V: Show accurate per-hart isa in /proc/cpuinfo

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@...osinc.com>
> Reviewed-by: Andrew Jones <ajones@...tanamicro.com>
> Reviewed-by: Conor Dooley <conor.dooley@...rochip.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
> 

Powered by blists - more mailing lists