From: Andrew Jones <ajones@ventanamicro.com>
To: Conor Dooley <conor@kernel.org>
Cc: qemu-riscv@nongnu.org, Conor Dooley <conor.dooley@microchip.com>,
Alistair Francis <Alistair.Francis@wdc.com>,
Bin Meng <bin.meng@windriver.com>,
Palmer Dabbelt <palmer@dabbelt.com>,
Weiwei Li <liwei1518@gmail.com>,
Daniel Henrique Barboza <dbarboza@ventanamicro.com>,
Liu Zhiwei <zhiwei_liu@linux.alibaba.com>,
qemu-devel@nongnu.org
Subject: Re: [PATCH v3 2/2] target/riscv: support new isa extension detection devicetree properties
Date: Wed, 10 Jan 2024 12:42:01 +0100 [thread overview]
Message-ID: <20240110-1a8341d0b2a02f1396f14c5b@orel> (raw)
In-Reply-To: <20240110-sasquatch-vaporizer-b1d92e7ea9dc@spud>
On Wed, Jan 10, 2024 at 10:25:37AM +0000, 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>
> ---
> hw/riscv/sifive_u.c | 7 ++----
> hw/riscv/spike.c | 6 ++---
> hw/riscv/virt.c | 6 ++---
> target/riscv/cpu.c | 53 +++++++++++++++++++++++++++++++++++++++++++++
> target/riscv/cpu.h | 1 +
> 5 files changed, 60 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(&s->soc.u_cpus.harts[cpu - 1]);
> + riscv_isa_write_fdt(&s->soc.u_cpus.harts[cpu - 1], fdt, nodename);
> } else {
> - isa = riscv_isa_string(&s->soc.e_cpus.harts[0]);
> + riscv_isa_write_fdt(&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..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 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_write_fdt(&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 f9fd1341fc..c47b2d397a 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -215,7 +215,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;
>
> @@ -236,9 +236,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_write_fdt(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 5b5da970f2..1c8c81ca4c 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"
> @@ -1875,6 +1876,58 @@ 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);
> + 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->name; edata++) {
> + if (isa_ext_is_enabled(cpu, edata->ext_enable_offset)) {
> + extensions[*count] = g_strdup(edata->name);
> + (*count)++;
> + }
> + }
> +
> + return extensions;
> +}
> +
> +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;
> + int xlen = 16 << cpu->env.misa_mxl_max;
> +
> + riscv_isa = riscv_isa_string(cpu);
> + qemu_fdt_setprop_string(fdt, nodename, "riscv,isa", riscv_isa);
> +
> + snprintf(isa_base, maxlen, "rv%di", xlen);
> + 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]);
> + }
> +
> + g_free(isa_extensions);
> +}
> +#endif
> +
> #define DEFINE_CPU(type_name, initfn) \
> { \
> .name = type_name, \
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 5f3955c38d..192d0c2d31 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -510,6 +510,7 @@ char *riscv_isa_string(RISCVCPU *cpu);
> #define cpu_mmu_index riscv_cpu_mmu_index
>
> #ifndef CONFIG_USER_ONLY
> +void riscv_isa_write_fdt(RISCVCPU *cpu, void *fdt, char *nodename);
> void riscv_cpu_do_transaction_failed(CPUState *cs, hwaddr physaddr,
> vaddr addr, unsigned size,
> MMUAccessType access_type,
> --
> 2.39.2
>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
next prev parent reply other threads:[~2024-01-10 11:42 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-10 10:25 [PATCH v3 0/2] riscv: support new isa extension detection devicetree properties Conor Dooley
2024-01-10 10:25 ` [PATCH v3 1/2] target/riscv: use misa_mxl_max to populate isa string rather than TARGET_LONG_BITS Conor Dooley
2024-01-10 11:37 ` Andrew Jones
2024-01-22 5:17 ` Alistair Francis
2024-01-10 10:25 ` [PATCH v3 2/2] target/riscv: support new isa extension detection devicetree properties Conor Dooley
2024-01-10 11:42 ` Andrew Jones [this message]
2024-01-22 5:19 ` Alistair Francis
2024-01-22 5:24 ` [PATCH v3 0/2] riscv: " Alistair Francis
2024-01-22 12:25 ` Conor Dooley
2024-01-23 5:50 ` Alistair Francis
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240110-1a8341d0b2a02f1396f14c5b@orel \
--to=ajones@ventanamicro.com \
--cc=Alistair.Francis@wdc.com \
--cc=bin.meng@windriver.com \
--cc=conor.dooley@microchip.com \
--cc=conor@kernel.org \
--cc=dbarboza@ventanamicro.com \
--cc=liwei1518@gmail.com \
--cc=palmer@dabbelt.com \
--cc=qemu-devel@nongnu.org \
--cc=qemu-riscv@nongnu.org \
--cc=zhiwei_liu@linux.alibaba.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).