Re: [PATCH v1] riscv: support new isa extension detection devicetree properties

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 6651 bytes --]

On Wed, Nov 29, 2023 at 11:37:00AM -0300, Daniel Henrique Barboza wrote:
> On 11/29/23 07:39, Conor Dooley wrote:
> > From: Conor Dooley <conor.dooley@microchip.com>
> > 
> > A few months ago I submitted a patch to various lists, deprecating
> > "riscv,isa" with a lengthy commit message [0] that is now commit
> > aeb71e42caae ("dt-bindings: riscv: deprecate riscv,isa") in the Linux
> > kernel tree. Primarily, the goal was to replace "riscv,isa" with a new
> > set of properties that allowed for strictly defining the meaning of
> > various extensions, where "riscv,isa" was tied to whatever definitions
> > inflicted upon us by the ISA manual, which have seen some variance over
> > time.
> > 
> > Two new properties were introduced: "riscv,isa-base" and
> > "riscv,isa-extensions". The former is a simple string to communicate the
> > base ISA implemented by a hart and the latter an array of strings used
> > to communicate the set of ISA extensions supported, per the definitions
> > of each substring in extensions.yaml [1]. A beneficial side effect was
> > also the ability to define vendor extensions in a more "official" way,
> > as the ISA manual and other RVI specifications only covered the format
> > for vendor extensions in the ISA string, but not the meaning of vendor
> > extensions, for obvious reasons.
> > 
> > Add support for setting these two new properties in the devicetrees for
> > the various devicetree platforms supported by QEMU for RISC-V. The Linux
> > kernel already supports parsing ISA extensions from these new
> > properties, and documenting them in the dt-binding is a requirement for
> > new extension detection being added to the kernel.
> > 
> > A side effect of the implementation is that the meaning for elements in
> > "riscv,isa" and in "riscv,isa-extensions" are now tied together as they
> > are constructed from the same source. The same applies to the ISA string
> > provided in ACPI tables, but there does not appear to be any strict
> > definitions of meanings in ACPI land either.
> > 
> > Link: https://lore.kernel.org/qemu-riscv/20230702-eats-scorebook-c951f170d29f@spud/ [0]
> > Link: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/riscv/extensions.yaml [1]
> > Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> > ---
> 
> What about new extensions? Do we still need them to be present in riscv,isa
> or can we just add them to riscv,isa-extensions? Eventually we'll want applications
> to move on and stop using riscv,isa altogether, but if we keep adding new
> extensions to it this process will take longer.

Yeah, I kinda didn't want to go there any time soon though, since this
is only the second codebase I've added the properties to so far. If
preventing adding new things to "riscv,isa" is something the maintainers
of the RISC-V QEMU support are down to do, I certainly would not object.

> 
> > The apart from the bonding of ACPI and DT definitions which concerns me
> > a bit, I'm a wee bit worried about the vendor extensions diverging from
> > what ends up in bindings. Ideally I think there should be acked binding
> > patches for extensions, but that's well outside of my jurisdiction!
> > 
> > There's two 80 char line length violations in this, but the file already
> > has some > 80 char lines, so I figured that it'd be fine...
> 
> It's fine to me.

Cool.

> > +void riscv_isa_set_props(RISCVCPU *cpu, void *fdt, char *nodename)
> > +{
> > +    const size_t maxlen = sizeof("rv128i");
> > +    g_autofree char *isa_base = g_new(char, maxlen);
> > +    g_autofree char *riscv_isa;
> > +    char **isa_extensions;
> > +    int count = 0;
> > +
> > +    riscv_isa = riscv_isa_string(cpu);
> > +    qemu_fdt_setprop_string(fdt, nodename, "riscv,isa", riscv_isa);
> > +
> > +    snprintf(isa_base, maxlen, "rv%di", TARGET_LONG_BITS);
> > +    qemu_fdt_setprop_string(fdt, nodename, "riscv,isa-base", isa_base);
> > +
> > +    isa_extensions = riscv_isa_extensions_list(cpu, &count);
> > +    qemu_fdt_setprop_string_array(fdt, nodename, "riscv,isa-extensions",
> > +                                  isa_extensions, count);
> > +
> > +    for (int i = 0; i < count; i++) {
> > +        g_free(isa_extensions[i]);
> > +    }
> > +}
> 
> This will break user-mode build:
> 
> [2025/2626] Linking target qemu-riscv64
> FAILED: qemu-riscv64
> (...)
> /usr/bin/ld: libqemu-riscv64-linux-user.fa.p/target_riscv_cpu.c.o: in function `riscv_isa_set_props':
> /home/danielhb/work/qemu/build/../target/riscv/cpu.c:1774: undefined reference to `qemu_fdt_setprop_string'
> /usr/bin/ld: /home/danielhb/work/qemu/build/../target/riscv/cpu.c:1777: undefined reference to `qemu_fdt_setprop_string'
> /usr/bin/ld: /home/danielhb/work/qemu/build/../target/riscv/cpu.c:1780: undefined reference to `qemu_fdt_setprop_string_array'
> collect2: error: ld returned 1 exit status

ngl, I have never used user-mode before. I'll fix that up. I'm not
really an indef in c files fan, but if that is par for the course then
it works for me :)

> As a bonus, I would rename riscv_isa_set_props() to something like riscv_write_isa_fdt()
> to be closer to what the function is doing.
> 
> 
> Finally, is this the intended result? I got this DTs by using a simple buildroot with
> rv64:
> 
> # cat  /proc/device-tree/cpus/cpu@0/riscv,isa-base
> rv64i#
> #
> # cat  /proc/device-tree/cpus/cpu@0/riscv,isa
> rv64imafdch_zicbom_zicboz_zicntr_zicsr_zifencei_zihintntl_zihintpause_zihpm_zawrs_zfa_zca_zcd_zba_zbb_zbc_zbs_sstc_svadu#
> #
> # cat  /proc/device-tree/cpus/cpu@0/riscv,isa-extensions
> imafdchzicbomzicbozzicntrzicsrzifenceizihintntlzihintpausezihpmzawrszfazcazcdzbazbbzbczbssstcsvadu#

Yup! It just looks confusing with that proc file.
# cat /proc/device-tree/cpus/cpu@0/riscv,isa-extensions
imafdchzicbomzicbozzicntrzicsrzifenceizihintntlzihintpausezihpmzawrszfazcazcdzbazbbzbczbssstcsvadu# 
which then shows up in /proc/cpuinfo as
isa             : rv64imafdch_zicbom_zicboz_zicntr_zicsr_zifencei_zihintpause_zihpm_zba_zbb_zbs_sstc

Dumping the dtb and running it backwards through dtc you can see the
property looks like:
riscv,isa-extensions = "i\0m\0a\0f\0d\0c\0h\0zicbom\0zicboz\0zicntr\0zicsr\0zifencei\0zihintntl\0zihintpause\0zihpm\0zawrs\0zfa\0zca\0zcd\0zba\0zbb\0zbc\0zbs\0sstc\0svadu";

That's the same way that compatibles appear:
# cat /proc/device-tree/soc/clint@2000000/compatible 
sifive,clint0riscv,clint0#

The null terminating chars being the "issue" in both cases.

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]