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

Commit Message

Conor Dooley Nov. 29, 2023, 10:39 a.m. UTC

From: Conor Dooley <[e-mail]>

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 <[e-mail]>
---
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...

CC: Alistair Francis <[e-mail]>
CC: Bin Meng <[e-mail]>
CC: Palmer Dabbelt <[e-mail]>
CC: Weiwei Li <[e-mail]>
CC: Daniel Henrique Barboza <[e-mail]>
CC: Liu Zhiwei <[e-mail]>
CC: [e-mail]
CC: [e-mail]
---
 hw/riscv/sifive_u.c |  7 ++-----
 hw/riscv/spike.c    |  6 ++----
 hw/riscv/virt.c     |  6 ++----
 target/riscv/cpu.c  | 50 +++++++++++++++++++++++++++++++++++++++++++++
 target/riscv/cpu.h  |  1 +
 5 files changed, 57 insertions(+), 13 deletions(-)

Comments

Richard Henderson Nov. 29, 2023, 1:44 p.m. UTC | #1

On 11/29/23 04:39, Conor Dooley wrote:
> From: Conor Dooley <[e-mail]>
> 
> 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 <[e-mail]>
> ---
> 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...
> 
> CC: Alistair Francis <[e-mail]>
> CC: Bin Meng <[e-mail]>
> CC: Palmer Dabbelt <[e-mail]>
> CC: Weiwei Li <[e-mail]>
> CC: Daniel Henrique Barboza <[e-mail]>
> CC: Liu Zhiwei <[e-mail]>
> CC: [e-mail]
> CC: [e-mail]
> ---
>   hw/riscv/sifive_u.c |  7 ++-----
>   hw/riscv/spike.c    |  6 ++----
>   hw/riscv/virt.c     |  6 ++----
>   target/riscv/cpu.c  | 50 +++++++++++++++++++++++++++++++++++++++++++++
>   target/riscv/cpu.h  |  1 +
>   5 files changed, 57 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> index ec76dce6c9..4e6eed863b 100644
> --- a/hw/riscv/sifive_u.c
> +++ b/hw/riscv/sifive_u.c
> @@ -171,7 +171,6 @@ static void create_fdt(SiFiveUState *s, const MemMapEntry *memmap,
>           int cpu_phandle = phandle++;
>           nodename = g_strdup_printf("/cpus/cpu@%d", cpu);
>           char *intc = g_strdup_printf("/cpus/cpu@%d/interrupt-controller", cpu);
> -        char *isa;
>           qemu_fdt_add_subnode(fdt, nodename);
>           /* cpu 0 is the management hart that does not have mmu */
>           if (cpu != 0) {
> @@ -180,11 +179,10 @@ static void create_fdt(SiFiveUState *s, const MemMapEntry *memmap,
>               } else {
>                   qemu_fdt_setprop_string(fdt, nodename, "mmu-type", "riscv,sv48");
>               }
> -            isa = riscv_isa_string(&s->soc.u_cpus.harts[cpu - 1]);
> +            riscv_isa_set_props(&s->soc.u_cpus.harts[cpu - 1], fdt, nodename);
>           } else {
> -            isa = riscv_isa_string(&s->soc.e_cpus.harts[0]);
> +            riscv_isa_set_props(&s->soc.e_cpus.harts[0], fdt, nodename);
>           }
> -        qemu_fdt_setprop_string(fdt, nodename, "riscv,isa", isa);
>           qemu_fdt_setprop_string(fdt, nodename, "compatible", "riscv");
>           qemu_fdt_setprop_string(fdt, nodename, "status", "okay");
>           qemu_fdt_setprop_cell(fdt, nodename, "reg", cpu);
> @@ -194,7 +192,6 @@ static void create_fdt(SiFiveUState *s, const MemMapEntry *memmap,
>           qemu_fdt_setprop_string(fdt, intc, "compatible", "riscv,cpu-intc");
>           qemu_fdt_setprop(fdt, intc, "interrupt-controller", NULL, 0);
>           qemu_fdt_setprop_cell(fdt, intc, "#interrupt-cells", 1);
> -        g_free(isa);
>           g_free(intc);
>           g_free(nodename);
>       }
> diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
> index 81f7e53aed..1657554d7b 100644
> --- a/hw/riscv/spike.c
> +++ b/hw/riscv/spike.c
> @@ -59,7 +59,7 @@ static void create_fdt(SpikeState *s, const MemMapEntry *memmap,
>       MachineState *ms = MACHINE(s);
>       uint32_t *clint_cells;
>       uint32_t cpu_phandle, intc_phandle, phandle = 1;
> -    char *name, *mem_name, *clint_name, *clust_name;
> +    char *mem_name, *clint_name, *clust_name;
>       char *core_name, *cpu_name, *intc_name;
>       static const char * const clint_compat[2] = {
>           "sifive,clint0", "riscv,clint0"
> @@ -113,9 +113,7 @@ static void create_fdt(SpikeState *s, const MemMapEntry *memmap,
>               } else {
>                   qemu_fdt_setprop_string(fdt, cpu_name, "mmu-type", "riscv,sv48");
>               }
> -            name = riscv_isa_string(&s->soc[socket].harts[cpu]);
> -            qemu_fdt_setprop_string(fdt, cpu_name, "riscv,isa", name);
> -            g_free(name);
> +            riscv_isa_set_props(&s->soc[socket].harts[cpu], fdt, cpu_name);
>               qemu_fdt_setprop_string(fdt, cpu_name, "compatible", "riscv");
>               qemu_fdt_setprop_string(fdt, cpu_name, "status", "okay");
>               qemu_fdt_setprop_cell(fdt, cpu_name, "reg",
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index c7fc97e273..435ad66cd3 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -238,7 +238,7 @@ static void create_fdt_socket_cpus(RISCVVirtState *s, int socket,
>       int cpu;
>       uint32_t cpu_phandle;
>       MachineState *ms = MACHINE(s);
> -    char *name, *cpu_name, *core_name, *intc_name, *sv_name;
> +    char *cpu_name, *core_name, *intc_name, *sv_name;
>       bool is_32_bit = riscv_is_32bit(&s->soc[0]);
>       uint8_t satp_mode_max;
>   
> @@ -259,9 +259,7 @@ static void create_fdt_socket_cpus(RISCVVirtState *s, int socket,
>               g_free(sv_name);
>           }
>   
> -        name = riscv_isa_string(cpu_ptr);
> -        qemu_fdt_setprop_string(ms->fdt, cpu_name, "riscv,isa", name);
> -        g_free(name);
> +        riscv_isa_set_props(cpu_ptr, ms->fdt, cpu_name);
>   
>           if (cpu_ptr->cfg.ext_zicbom) {
>               qemu_fdt_setprop_cell(ms->fdt, cpu_name, "riscv,cbom-block-size",
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 83c7c0cf07..5a7e8d06ad 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -31,6 +31,7 @@
>   #include "hw/qdev-properties.h"
>   #include "migration/vmstate.h"
>   #include "fpu/softfloat-helpers.h"
> +#include "sysemu/device_tree.h"
>   #include "sysemu/kvm.h"
>   #include "sysemu/tcg.h"
>   #include "kvm/kvm_riscv.h"
> @@ -1735,6 +1736,55 @@ char *riscv_isa_string(RISCVCPU *cpu)
>       return isa_str;
>   }
>   
> +static char **riscv_isa_extensions_list(RISCVCPU *cpu, int *count)
> +{
> +    int maxlen = ARRAY_SIZE(riscv_single_letter_exts) + ARRAY_SIZE(isa_edata_arr);
> +    char **extensions = g_new(char *, maxlen);
> +
> +    for (int i = 0; i < sizeof(riscv_single_letter_exts) - 1; i++) {
> +        if (cpu->env.misa_ext & RV(riscv_single_letter_exts[i])) {
> +            extensions[*count] = g_new(char, 2);
> +            snprintf(extensions[*count], 2, "%c",
> +                     qemu_tolower(riscv_single_letter_exts[i]));
> +            (*count)++;
> +        }
> +    }
> +
> +    for (const RISCVIsaExtData *edata = isa_edata_arr; edata && edata->name; edata++) {
> +        if (isa_ext_is_enabled(cpu, edata->ext_enable_offset)) {
> +            extensions[*count] = g_new(char, strlen(edata->name) + 1);
> +            snprintf(extensions[*count], strlen(edata->name) + 1, "%s",
> +                     edata->name);

g_strdup.

Daniel Henrique Barboza Nov. 29, 2023, 2:37 p.m. UTC | #2

On 11/29/23 07:39, Conor Dooley wrote:
> From: Conor Dooley <[e-mail]>
> 
> 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 <[e-mail]>
> ---

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.

> 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.


One comment down below:

> 
> CC: Alistair Francis <[e-mail]>
> CC: Bin Meng <[e-mail]>
> CC: Palmer Dabbelt <[e-mail]>
> CC: Weiwei Li <[e-mail]>
> CC: Daniel Henrique Barboza <[e-mail]>
> CC: Liu Zhiwei <[e-mail]>
> CC: [e-mail]
> CC: [e-mail]
> ---
>   hw/riscv/sifive_u.c |  7 ++-----
>   hw/riscv/spike.c    |  6 ++----
>   hw/riscv/virt.c     |  6 ++----
>   target/riscv/cpu.c  | 50 +++++++++++++++++++++++++++++++++++++++++++++
>   target/riscv/cpu.h  |  1 +
>   5 files changed, 57 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> index ec76dce6c9..4e6eed863b 100644
> --- a/hw/riscv/sifive_u.c
> +++ b/hw/riscv/sifive_u.c
> @@ -171,7 +171,6 @@ static void create_fdt(SiFiveUState *s, const MemMapEntry *memmap,
>           int cpu_phandle = phandle++;
>           nodename = g_strdup_printf("/cpus/cpu@%d", cpu);
>           char *intc = g_strdup_printf("/cpus/cpu@%d/interrupt-controller", cpu);
> -        char *isa;
>           qemu_fdt_add_subnode(fdt, nodename);
>           /* cpu 0 is the management hart that does not have mmu */
>           if (cpu != 0) {
> @@ -180,11 +179,10 @@ static void create_fdt(SiFiveUState *s, const MemMapEntry *memmap,
>               } else {
>                   qemu_fdt_setprop_string(fdt, nodename, "mmu-type", "riscv,sv48");
>               }
> -            isa = riscv_isa_string(&s->soc.u_cpus.harts[cpu - 1]);
> +            riscv_isa_set_props(&s->soc.u_cpus.harts[cpu - 1], fdt, nodename);
>           } else {
> -            isa = riscv_isa_string(&s->soc.e_cpus.harts[0]);
> +            riscv_isa_set_props(&s->soc.e_cpus.harts[0], fdt, nodename);
>           }
> -        qemu_fdt_setprop_string(fdt, nodename, "riscv,isa", isa);
>           qemu_fdt_setprop_string(fdt, nodename, "compatible", "riscv");
>           qemu_fdt_setprop_string(fdt, nodename, "status", "okay");
>           qemu_fdt_setprop_cell(fdt, nodename, "reg", cpu);
> @@ -194,7 +192,6 @@ static void create_fdt(SiFiveUState *s, const MemMapEntry *memmap,
>           qemu_fdt_setprop_string(fdt, intc, "compatible", "riscv,cpu-intc");
>           qemu_fdt_setprop(fdt, intc, "interrupt-controller", NULL, 0);
>           qemu_fdt_setprop_cell(fdt, intc, "#interrupt-cells", 1);
> -        g_free(isa);
>           g_free(intc);
>           g_free(nodename);
>       }
> diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
> index 81f7e53aed..1657554d7b 100644
> --- a/hw/riscv/spike.c
> +++ b/hw/riscv/spike.c
> @@ -59,7 +59,7 @@ static void create_fdt(SpikeState *s, const MemMapEntry *memmap,
>       MachineState *ms = MACHINE(s);
>       uint32_t *clint_cells;
>       uint32_t cpu_phandle, intc_phandle, phandle = 1;
> -    char *name, *mem_name, *clint_name, *clust_name;
> +    char *mem_name, *clint_name, *clust_name;
>       char *core_name, *cpu_name, *intc_name;
>       static const char * const clint_compat[2] = {
>           "sifive,clint0", "riscv,clint0"
> @@ -113,9 +113,7 @@ static void create_fdt(SpikeState *s, const MemMapEntry *memmap,
>               } else {
>                   qemu_fdt_setprop_string(fdt, cpu_name, "mmu-type", "riscv,sv48");
>               }
> -            name = riscv_isa_string(&s->soc[socket].harts[cpu]);
> -            qemu_fdt_setprop_string(fdt, cpu_name, "riscv,isa", name);
> -            g_free(name);
> +            riscv_isa_set_props(&s->soc[socket].harts[cpu], fdt, cpu_name);
>               qemu_fdt_setprop_string(fdt, cpu_name, "compatible", "riscv");
>               qemu_fdt_setprop_string(fdt, cpu_name, "status", "okay");
>               qemu_fdt_setprop_cell(fdt, cpu_name, "reg",
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index c7fc97e273..435ad66cd3 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -238,7 +238,7 @@ static void create_fdt_socket_cpus(RISCVVirtState *s, int socket,
>       int cpu;
>       uint32_t cpu_phandle;
>       MachineState *ms = MACHINE(s);
> -    char *name, *cpu_name, *core_name, *intc_name, *sv_name;
> +    char *cpu_name, *core_name, *intc_name, *sv_name;
>       bool is_32_bit = riscv_is_32bit(&s->soc[0]);
>       uint8_t satp_mode_max;
>   
> @@ -259,9 +259,7 @@ static void create_fdt_socket_cpus(RISCVVirtState *s, int socket,
>               g_free(sv_name);
>           }
>   
> -        name = riscv_isa_string(cpu_ptr);
> -        qemu_fdt_setprop_string(ms->fdt, cpu_name, "riscv,isa", name);
> -        g_free(name);
> +        riscv_isa_set_props(cpu_ptr, ms->fdt, cpu_name);
>   
>           if (cpu_ptr->cfg.ext_zicbom) {
>               qemu_fdt_setprop_cell(ms->fdt, cpu_name, "riscv,cbom-block-size",
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 83c7c0cf07..5a7e8d06ad 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -31,6 +31,7 @@
>   #include "hw/qdev-properties.h"
>   #include "migration/vmstate.h"
>   #include "fpu/softfloat-helpers.h"
> +#include "sysemu/device_tree.h"
>   #include "sysemu/kvm.h"
>   #include "sysemu/tcg.h"
>   #include "kvm/kvm_riscv.h"
> @@ -1735,6 +1736,55 @@ char *riscv_isa_string(RISCVCPU *cpu)
>       return isa_str;
>   }
>   
> +static char **riscv_isa_extensions_list(RISCVCPU *cpu, int *count)
> +{
> +    int maxlen = ARRAY_SIZE(riscv_single_letter_exts) + ARRAY_SIZE(isa_edata_arr);
> +    char **extensions = g_new(char *, maxlen);
> +
> +    for (int i = 0; i < sizeof(riscv_single_letter_exts) - 1; i++) {
> +        if (cpu->env.misa_ext & RV(riscv_single_letter_exts[i])) {
> +            extensions[*count] = g_new(char, 2);
> +            snprintf(extensions[*count], 2, "%c",
> +                     qemu_tolower(riscv_single_letter_exts[i]));
> +            (*count)++;
> +        }
> +    }
> +
> +    for (const RISCVIsaExtData *edata = isa_edata_arr; edata && edata->name; edata++) {
> +        if (isa_ext_is_enabled(cpu, edata->ext_enable_offset)) {
> +            extensions[*count] = g_new(char, strlen(edata->name) + 1);
> +            snprintf(extensions[*count], strlen(edata->name) + 1, "%s",
> +                     edata->name);
> +            (*count)++;
> +        }
> +    }
> +
> +    return extensions;
> +}
> +
> +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

The reason here is that the function has qemu_fdt* functions that user-mode doesn't
know about.

A quick fix would be to fold both this function and riscv_isa_extensions_list()
in #ifndef CONFIG_USER_ONLY. You'll also need to move the function declaration in cpu.h to
the #ifndef CONFIG_USER_ONLY block a handful of lines after where riscv_isa_set_props()
is being declared. Like this:


diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 5a7e8d06ad..8c3e31ab44 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1736,6 +1736,7 @@ char *riscv_isa_string(RISCVCPU *cpu)
      return isa_str;
  }
  
+#ifndef CONFIG_USER_ONLY
  static char **riscv_isa_extensions_list(RISCVCPU *cpu, int *count)
  {
      int maxlen = ARRAY_SIZE(riscv_single_letter_exts) + ARRAY_SIZE(isa_edata_arr);
@@ -1784,6 +1785,7 @@ void riscv_isa_set_props(RISCVCPU *cpu, void *fdt, char *nodename)
          g_free(isa_extensions[i]);
      }
  }
+#endif

   static gint riscv_cpu_list_compare(gconstpointer a, gconstpointer b)
  {
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 021796fe01..763769403e 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -490,13 +490,13 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
                          MMUAccessType access_type, int mmu_idx,
                          bool probe, uintptr_t retaddr);
  char *riscv_isa_string(RISCVCPU *cpu);
-void riscv_isa_set_props(RISCVCPU *cpu, void *fdt, char *nodename);
  void riscv_cpu_list(void);
  
  #define cpu_list riscv_cpu_list
  #define cpu_mmu_index riscv_cpu_mmu_index
  
  #ifndef CONFIG_USER_ONLY
+void riscv_isa_set_props(RISCVCPU *cpu, void *fdt, char *nodename);
  void riscv_cpu_do_transaction_failed(CPUState *cs, hwaddr physaddr,
                                       vaddr addr, unsigned size,
                                       MMUAccessType access_type,



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#



Thanks,


Daniel


> +
>   static gint riscv_cpu_list_compare(gconstpointer a, gconstpointer b)
>   {
>       ObjectClass *class_a = (ObjectClass *)a;
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index bf58b0f0b5..d2dae07665 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -490,6 +490,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>                           MMUAccessType access_type, int mmu_idx,
>                           bool probe, uintptr_t retaddr);
>   char *riscv_isa_string(RISCVCPU *cpu);
> +void riscv_isa_set_props(RISCVCPU *cpu, void *fdt, char *nodename);
>   void riscv_cpu_list(void);
>   
>   #define cpu_list riscv_cpu_list

Conor Dooley Nov. 29, 2023, 3:12 p.m. UTC | #3

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 <[e-mail]>
> > 
> > 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 <[e-mail]>
> > ---
> 
> 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.

Patch

diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index ec76dce6c9..4e6eed863b 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -171,7 +171,6 @@  static void create_fdt(SiFiveUState *s, const MemMapEntry *memmap,
         int cpu_phandle = phandle++;
         nodename = g_strdup_printf("/cpus/cpu@%d", cpu);
         char *intc = g_strdup_printf("/cpus/cpu@%d/interrupt-controller", cpu);
-        char *isa;
         qemu_fdt_add_subnode(fdt, nodename);
         /* cpu 0 is the management hart that does not have mmu */
         if (cpu != 0) {
@@ -180,11 +179,10 @@  static void create_fdt(SiFiveUState *s, const MemMapEntry *memmap,
             } else {
                 qemu_fdt_setprop_string(fdt, nodename, "mmu-type", "riscv,sv48");
             }
-            isa = riscv_isa_string(&s->soc.u_cpus.harts[cpu - 1]);
+            riscv_isa_set_props(&s->soc.u_cpus.harts[cpu - 1], fdt, nodename);
         } else {
-            isa = riscv_isa_string(&s->soc.e_cpus.harts[0]);
+            riscv_isa_set_props(&s->soc.e_cpus.harts[0], fdt, nodename);
         }
-        qemu_fdt_setprop_string(fdt, nodename, "riscv,isa", isa);
         qemu_fdt_setprop_string(fdt, nodename, "compatible", "riscv");
         qemu_fdt_setprop_string(fdt, nodename, "status", "okay");
         qemu_fdt_setprop_cell(fdt, nodename, "reg", cpu);
@@ -194,7 +192,6 @@  static void create_fdt(SiFiveUState *s, const MemMapEntry *memmap,
         qemu_fdt_setprop_string(fdt, intc, "compatible", "riscv,cpu-intc");
         qemu_fdt_setprop(fdt, intc, "interrupt-controller", NULL, 0);
         qemu_fdt_setprop_cell(fdt, intc, "#interrupt-cells", 1);
-        g_free(isa);
         g_free(intc);
         g_free(nodename);
     }
diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
index 81f7e53aed..1657554d7b 100644
--- a/hw/riscv/spike.c
+++ b/hw/riscv/spike.c
@@ -59,7 +59,7 @@  static void create_fdt(SpikeState *s, const MemMapEntry *memmap,
     MachineState *ms = MACHINE(s);
     uint32_t *clint_cells;
     uint32_t cpu_phandle, intc_phandle, phandle = 1;
-    char *name, *mem_name, *clint_name, *clust_name;
+    char *mem_name, *clint_name, *clust_name;
     char *core_name, *cpu_name, *intc_name;
     static const char * const clint_compat[2] = {
         "sifive,clint0", "riscv,clint0"
@@ -113,9 +113,7 @@  static void create_fdt(SpikeState *s, const MemMapEntry *memmap,
             } else {
                 qemu_fdt_setprop_string(fdt, cpu_name, "mmu-type", "riscv,sv48");
             }
-            name = riscv_isa_string(&s->soc[socket].harts[cpu]);
-            qemu_fdt_setprop_string(fdt, cpu_name, "riscv,isa", name);
-            g_free(name);
+            riscv_isa_set_props(&s->soc[socket].harts[cpu], fdt, cpu_name);
             qemu_fdt_setprop_string(fdt, cpu_name, "compatible", "riscv");
             qemu_fdt_setprop_string(fdt, cpu_name, "status", "okay");
             qemu_fdt_setprop_cell(fdt, cpu_name, "reg",
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index c7fc97e273..435ad66cd3 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -238,7 +238,7 @@  static void create_fdt_socket_cpus(RISCVVirtState *s, int socket,
     int cpu;
     uint32_t cpu_phandle;
     MachineState *ms = MACHINE(s);
-    char *name, *cpu_name, *core_name, *intc_name, *sv_name;
+    char *cpu_name, *core_name, *intc_name, *sv_name;
     bool is_32_bit = riscv_is_32bit(&s->soc[0]);
     uint8_t satp_mode_max;
 
@@ -259,9 +259,7 @@  static void create_fdt_socket_cpus(RISCVVirtState *s, int socket,
             g_free(sv_name);
         }
 
-        name = riscv_isa_string(cpu_ptr);
-        qemu_fdt_setprop_string(ms->fdt, cpu_name, "riscv,isa", name);
-        g_free(name);
+        riscv_isa_set_props(cpu_ptr, ms->fdt, cpu_name);
 
         if (cpu_ptr->cfg.ext_zicbom) {
             qemu_fdt_setprop_cell(ms->fdt, cpu_name, "riscv,cbom-block-size",
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 83c7c0cf07..5a7e8d06ad 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -31,6 +31,7 @@ 
 #include "hw/qdev-properties.h"
 #include "migration/vmstate.h"
 #include "fpu/softfloat-helpers.h"
+#include "sysemu/device_tree.h"
 #include "sysemu/kvm.h"
 #include "sysemu/tcg.h"
 #include "kvm/kvm_riscv.h"
@@ -1735,6 +1736,55 @@  char *riscv_isa_string(RISCVCPU *cpu)
     return isa_str;
 }
 
+static char **riscv_isa_extensions_list(RISCVCPU *cpu, int *count)
+{
+    int maxlen = ARRAY_SIZE(riscv_single_letter_exts) + ARRAY_SIZE(isa_edata_arr);
+    char **extensions = g_new(char *, maxlen);
+
+    for (int i = 0; i < sizeof(riscv_single_letter_exts) - 1; i++) {
+        if (cpu->env.misa_ext & RV(riscv_single_letter_exts[i])) {
+            extensions[*count] = g_new(char, 2);
+            snprintf(extensions[*count], 2, "%c",
+                     qemu_tolower(riscv_single_letter_exts[i]));
+            (*count)++;
+        }
+    }
+
+    for (const RISCVIsaExtData *edata = isa_edata_arr; edata && edata->name; edata++) {
+        if (isa_ext_is_enabled(cpu, edata->ext_enable_offset)) {
+            extensions[*count] = g_new(char, strlen(edata->name) + 1);
+            snprintf(extensions[*count], strlen(edata->name) + 1, "%s",
+                     edata->name);
+            (*count)++;
+        }
+    }
+
+    return extensions;
+}
+
+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]);
+    }
+}
+
 static gint riscv_cpu_list_compare(gconstpointer a, gconstpointer b)
 {
     ObjectClass *class_a = (ObjectClass *)a;
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index bf58b0f0b5..d2dae07665 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -490,6 +490,7 @@  bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
                         MMUAccessType access_type, int mmu_idx,
                         bool probe, uintptr_t retaddr);
 char *riscv_isa_string(RISCVCPU *cpu);
+void riscv_isa_set_props(RISCVCPU *cpu, void *fdt, char *nodename);
 void riscv_cpu_list(void);
 
 #define cpu_list riscv_cpu_list