Re: [PATCH v1 1/2] riscv: don't read riscv, isa in the riscv cpu's get_desc()

[Leo Liang <ycliang@andestech.com>]

On Mon, Mar 18, 2024 at 03:16:02PM +0000, Conor Dooley wrote:
> From: Conor Dooley <conor.dooley@microchip.com>
> 
> cpu_get_desc() for the RISC-V CPU currently reads "riscv,isa" to get
> the description, but it is no longer a required property and cannot be
> assummed to always be present, as the new "riscv,isa-extensions" and
> "riscv,isa-base" properties may be present instead.
> 
> On RISC-V, cpu_get_desc() has two main uses - firstly providing an
> informational name for the CPU for smbios or at boot with
> DISPLAY_CPUINFO etc and secondly it forms the basis of ISA extension
> detection in supports_extension() as it returns (a portion of) an ISA
> string.
> 
> cpu_get_desc() returns a string, which aligned with "riscv,isa" but
> the new property is a list of strings. Rather than add support for
> the list of strings property, which would require creating an isa
> string from "riscv,isa-extensions", modify the RISC-V CPU's
> implementaion of cpu_get_desc() return the first compatible as the
> cpu description instead. This may be fine for the informational cases,
> but it would break extension dtection, given supports_extension()
> expects cpu_get_desc() to return an ISA string.
> 
> Call dev_read_string() directly in supports_extension() to get the
> contents of "riscv,isa" so that extension detection remains functional.
> As a knock-on affect of this change, extension detection is no longer
> broken for long ISA strings. Previously if the ISA string exceeded the
> 32 element array that supports_extension() passed to cpu_get_desc(),
> it would return ENOSPC and no extensions would be detected.
> This bug probably had no impact as U-Boot does not currently do anything
> meaningful with the results of supports_extension() and most SoCs
> supported by U-Boot don't have anywhere near that complex of an ISA
> string. The QEMU virt machine's CPUs do however, so extension detection
> doesn't work there.
> 
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
> I'm not really sure if I am happy with this patch - people could
> definitely have got use out of the cpu info printout of the ISA string
> before this patch - they'd have seen something like
> CPU:	rv64imafdc
> at boot, but now they will see
> CPU:	sifive,u74
> If it really is desired, cpu_get_desc() could be made to assemble
> an isa string out of "riscv,isa-extensions", but I think it's always
> gonna be a bit flawed, since that string can run to almost arbitrary
> length now - one I saw for a CPU last week was 320 characters long
> and these things are only going to grow.
> ---
>  arch/riscv/cpu/cpu.c    | 12 +++++++-----
>  drivers/cpu/riscv_cpu.c |  8 ++++----
>  2 files changed, 11 insertions(+), 9 deletions(-)

LGTM!

reviewed-by: Leo Yu-Chi Liang <ycliang@andestech.com>