Re: Re: [PATCH v2] riscv: support new isa extension detection devicetree properties
On Mon, Dec 18, 2023 at 02:37:55PM +1000, Alistair Francis wrote:
...
> > +void riscv_isa_write_fdt(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);
>
> This should dynamically come from misa_mxl_max not the compile time target
>
Need to also fix riscv_isa_string()
Thanks,
drew
Re: [PATCH v2] riscv: support new isa extension detection devicetree properties
On Fri, Dec 08, 2023 at 12:07:22PM +, Conor Dooley wrote:
> From: Conor Dooley
>
> 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
> ---
> Changes in v2:
> - use g_strdup() for multiletter extension string copying
> - wrap stuff in #ifndef to prevent breaking the user mode build
> - rename riscv_isa_set_props() -> riscv_isa_write_fdt()
>
> CC: Alistair Francis
> CC: Bin Meng
> CC: Palmer Dabbelt
> CC: Weiwei Li
> CC: Daniel Henrique Barboza
> CC: Liu Zhiwei
> CC: qemu-ri...@nongnu.org
> CC: qemu-devel@nongnu.org
> ---
> 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..2f227f15bc 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(>soc.u_cpus.harts[cpu - 1]);
> +riscv_isa_write_fdt(>soc.u_cpus.harts[cpu - 1], fdt,
> nodename);
> } else {
> -isa = riscv_isa_string(>soc.e_cpus.harts[0]);
> +riscv_isa_write_fdt(>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..64074395bc 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;
> -
Re: [PATCH v2] riscv: support new isa extension detection devicetree properties
On Fri, Dec 8, 2023 at 10:09 PM Conor Dooley wrote:
>
> From: Conor Dooley
>
> 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
> ---
> Changes in v2:
> - use g_strdup() for multiletter extension string copying
> - wrap stuff in #ifndef to prevent breaking the user mode build
> - rename riscv_isa_set_props() -> riscv_isa_write_fdt()
>
> CC: Alistair Francis
> CC: Bin Meng
> CC: Palmer Dabbelt
> CC: Weiwei Li
> CC: Daniel Henrique Barboza
> CC: Liu Zhiwei
> CC: qemu-ri...@nongnu.org
> CC: qemu-devel@nongnu.org
> ---
> 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..2f227f15bc 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(>soc.u_cpus.harts[cpu - 1]);
> +riscv_isa_write_fdt(>soc.u_cpus.harts[cpu - 1], fdt,
> nodename);
> } else {
> -isa = riscv_isa_string(>soc.e_cpus.harts[0]);
> +riscv_isa_write_fdt(>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..64074395bc 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,
Re: [PATCH v2] riscv: support new isa extension detection devicetree properties
On 12/8/23 09:07, Conor Dooley wrote:
From: Conor Dooley
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
---
Reviewed-by: Daniel Henrique Barboza
Changes in v2:
- use g_strdup() for multiletter extension string copying
- wrap stuff in #ifndef to prevent breaking the user mode build
- rename riscv_isa_set_props() -> riscv_isa_write_fdt()
CC: Alistair Francis
CC: Bin Meng
CC: Palmer Dabbelt
CC: Weiwei Li
CC: Daniel Henrique Barboza
CC: Liu Zhiwei
CC: qemu-ri...@nongnu.org
CC: qemu-devel@nongnu.org
---
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..2f227f15bc 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(>soc.u_cpus.harts[cpu - 1]);
+riscv_isa_write_fdt(>soc.u_cpus.harts[cpu - 1], fdt, nodename);
} else {
-isa = riscv_isa_string(>soc.e_cpus.harts[0]);
+riscv_isa_write_fdt(>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..64074395bc 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
