* [RFC PATCH v2 0/2] hw/riscv: hart: allow other cpu instance
@ 2023-09-14 8:07 Nikita Shubin
2023-09-14 8:07 ` [RFC PATCH v2 1/2] hw/riscv: hart: replace array access with qemu_get_cpu() Nikita Shubin
2023-09-14 8:07 ` [RFC PATCH v2 2/2] hw/riscv: hart: allow other cpu instance Nikita Shubin
0 siblings, 2 replies; 6+ messages in thread
From: Nikita Shubin @ 2023-09-14 8:07 UTC (permalink / raw)
To: Palmer Dabbelt, Alistair Francis, Bin Meng, Weiwei Li,
Daniel Henrique Barboza, Liu Zhiwei, Sunil V L
Cc: Nikita Shubin, qemu-riscv, qemu-devel
From: Nikita Shubin <n.shubin@yadro.com>
Currently it is not possible to overload instance of RISCVCPU,
i.e. something like this:
static const TypeInfo riscv_cpu_type_infos[] = {
{
.name = TYPE_ANOTHER_RISCV_CPU,
.parent = TYPE_RISCV_CPU,
.instance_size = sizeof(MyCPUState),
.instance_init = riscv_my_cpu_init,
}
};
Because we have RISCVHartArrayState.harts with exactly
the size of RISCVCPU.
Using own instances can be used to store some internal hart state.
Cc: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Link: https://patchwork.kernel.org/project/qemu-devel/patch/20230727080545.7908-1-nikita.shubin@maquefel.me/
Nikita Shubin (2):
hw/riscv: hart: replace array access with qemu_get_cpu()
hw/riscv: hart: allow other cpu instance
hw/riscv/boot.c | 6 ++++--
hw/riscv/riscv_hart.c | 20 ++++++++++++--------
hw/riscv/sifive_u.c | 7 +++++--
hw/riscv/spike.c | 17 ++++++++++-------
hw/riscv/virt-acpi-build.c | 2 +-
hw/riscv/virt.c | 17 +++++++++--------
include/hw/riscv/riscv_hart.h | 2 +-
7 files changed, 42 insertions(+), 29 deletions(-)
--
2.39.2
^ permalink raw reply [flat|nested] 6+ messages in thread* [RFC PATCH v2 1/2] hw/riscv: hart: replace array access with qemu_get_cpu() 2023-09-14 8:07 [RFC PATCH v2 0/2] hw/riscv: hart: allow other cpu instance Nikita Shubin @ 2023-09-14 8:07 ` Nikita Shubin 2023-09-18 1:50 ` Alistair Francis 2023-09-14 8:07 ` [RFC PATCH v2 2/2] hw/riscv: hart: allow other cpu instance Nikita Shubin 1 sibling, 1 reply; 6+ messages in thread From: Nikita Shubin @ 2023-09-14 8:07 UTC (permalink / raw) To: Palmer Dabbelt, Alistair Francis, Bin Meng, Weiwei Li, Daniel Henrique Barboza, Liu Zhiwei, Sunil V L Cc: Nikita Shubin, qemu-riscv, qemu-devel From: Nikita Shubin <n.shubin@yadro.com> Replace all RISCVHartArrayState->harts[idx] with qemu_get_cpu()/cpu_by_arch_id(). cpu_index is guaranteed to be continuus by cpu_get_free_index(), so they can be accessed in same order they were added. "Hart IDs might not necessarily be numbered contiguously in a multiprocessor system, but at least one hart must have a hart ID of zero." This states that hart ID zero should always be present, this makes using cpu_by_arch_id(0) safe. Signed-off-by: Nikita Shubin <n.shubin@yadro.com> --- hw/riscv/boot.c | 6 ++++-- hw/riscv/sifive_u.c | 7 +++++-- hw/riscv/spike.c | 17 ++++++++++------- hw/riscv/virt-acpi-build.c | 2 +- hw/riscv/virt.c | 17 +++++++++-------- 5 files changed, 29 insertions(+), 20 deletions(-) diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c index 52bf8e67de..041f966e58 100644 --- a/hw/riscv/boot.c +++ b/hw/riscv/boot.c @@ -36,7 +36,8 @@ bool riscv_is_32bit(RISCVHartArrayState *harts) { - return harts->harts[0].env.misa_mxl_max == MXL_RV32; + RISCVCPU *hart = RISCV_CPU(cpu_by_arch_id(0)); + return hart->env.misa_mxl_max == MXL_RV32; } /* @@ -385,6 +386,7 @@ void riscv_setup_rom_reset_vec(MachineState *machine, RISCVHartArrayState *harts uint64_t fdt_load_addr) { int i; + RISCVCPU *hart = RISCV_CPU(cpu_by_arch_id(0)); uint32_t start_addr_hi32 = 0x00000000; uint32_t fdt_load_addr_hi32 = 0x00000000; @@ -414,7 +416,7 @@ void riscv_setup_rom_reset_vec(MachineState *machine, RISCVHartArrayState *harts reset_vec[4] = 0x0182b283; /* ld t0, 24(t0) */ } - if (!harts->harts[0].cfg.ext_icsr) { + if (!hart->cfg.ext_icsr) { /* * The Zicsr extension has been disabled, so let's ensure we don't * run the CSR instruction. Let's fill the address with a non diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c index ec76dce6c9..3d09d0ee0e 100644 --- a/hw/riscv/sifive_u.c +++ b/hw/riscv/sifive_u.c @@ -168,6 +168,7 @@ static void create_fdt(SiFiveUState *s, const MemMapEntry *memmap, qemu_fdt_setprop_cell(fdt, "/cpus", "#address-cells", 0x1); for (cpu = ms->smp.cpus - 1; cpu >= 0; cpu--) { + RISCVCPU *hart; int cpu_phandle = phandle++; nodename = g_strdup_printf("/cpus/cpu@%d", cpu); char *intc = g_strdup_printf("/cpus/cpu@%d/interrupt-controller", cpu); @@ -180,9 +181,11 @@ 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]); + hart = RISCV_CPU(qemu_get_cpu(cpu - 1)); + isa = riscv_isa_string(hart); } else { - isa = riscv_isa_string(&s->soc.e_cpus.harts[0]); + hart = RISCV_CPU(qemu_get_cpu(0)); + isa = riscv_isa_string(hart); } qemu_fdt_setprop_string(fdt, nodename, "riscv,isa", isa); qemu_fdt_setprop_string(fdt, nodename, "compatible", "riscv"); diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c index 81f7e53aed..f3ec6427a1 100644 --- a/hw/riscv/spike.c +++ b/hw/riscv/spike.c @@ -97,29 +97,32 @@ static void create_fdt(SpikeState *s, const MemMapEntry *memmap, qemu_fdt_add_subnode(fdt, "/cpus/cpu-map"); for (socket = (riscv_socket_count(ms) - 1); socket >= 0; socket--) { + uint32_t num_harts = s->soc[socket].num_harts; + uint32_t hartid_base = s->soc[socket].hartid_base; + clust_name = g_strdup_printf("/cpus/cpu-map/cluster%d", socket); qemu_fdt_add_subnode(fdt, clust_name); - clint_cells = g_new0(uint32_t, s->soc[socket].num_harts * 4); + clint_cells = g_new0(uint32_t, num_harts * 4); - for (cpu = s->soc[socket].num_harts - 1; cpu >= 0; cpu--) { + for (cpu = num_harts - 1; cpu >= 0; cpu--) { + int cpu_index = hartid_base + cpu; + RISCVCPU *hart = RISCV_CPU(qemu_get_cpu(cpu_index)); cpu_phandle = phandle++; - cpu_name = g_strdup_printf("/cpus/cpu@%d", - s->soc[socket].hartid_base + cpu); + cpu_name = g_strdup_printf("/cpus/cpu@%d", cpu_index); qemu_fdt_add_subnode(fdt, cpu_name); if (is_32_bit) { qemu_fdt_setprop_string(fdt, cpu_name, "mmu-type", "riscv,sv32"); } else { qemu_fdt_setprop_string(fdt, cpu_name, "mmu-type", "riscv,sv48"); } - name = riscv_isa_string(&s->soc[socket].harts[cpu]); + name = riscv_isa_string(hart); qemu_fdt_setprop_string(fdt, cpu_name, "riscv,isa", name); g_free(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", - s->soc[socket].hartid_base + cpu); + qemu_fdt_setprop_cell(fdt, cpu_name, "reg", cpu_index); qemu_fdt_setprop_string(fdt, cpu_name, "device_type", "cpu"); riscv_socket_fdt_write_id(ms, cpu_name, socket); qemu_fdt_setprop_cell(fdt, cpu_name, "phandle", cpu_phandle); diff --git a/hw/riscv/virt-acpi-build.c b/hw/riscv/virt-acpi-build.c index 7331248f59..d885220cc9 100644 --- a/hw/riscv/virt-acpi-build.c +++ b/hw/riscv/virt-acpi-build.c @@ -158,7 +158,7 @@ static void build_rhct(GArray *table_data, isa_offset = table_data->len - table.table_offset; build_append_int_noprefix(table_data, 0, 2); /* Type 0 */ - cpu = &s->soc[0].harts[0]; + cpu = RISCV_CPU(cpu_by_arch_id(0)); isa = riscv_isa_string(cpu); len = 8 + strlen(isa) + 1; aligned_len = (len % 2) ? (len + 1) : len; diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c index 5edc1d98d2..f3132ecc1b 100644 --- a/hw/riscv/virt.c +++ b/hw/riscv/virt.c @@ -239,16 +239,18 @@ static void create_fdt_socket_cpus(RISCVVirtState *s, int socket, uint32_t cpu_phandle; MachineState *ms = MACHINE(s); char *name, *cpu_name, *core_name, *intc_name, *sv_name; + uint32_t num_harts = s->soc[socket].num_harts; + uint32_t hartid_base = s->soc[socket].hartid_base; bool is_32_bit = riscv_is_32bit(&s->soc[0]); uint8_t satp_mode_max; - for (cpu = s->soc[socket].num_harts - 1; cpu >= 0; cpu--) { - RISCVCPU *cpu_ptr = &s->soc[socket].harts[cpu]; + for (cpu = num_harts - 1; cpu >= 0; cpu--) { + int cpu_index = hartid_base + cpu; + RISCVCPU *cpu_ptr = RISCV_CPU(qemu_get_cpu(cpu_index)); cpu_phandle = (*phandle)++; - cpu_name = g_strdup_printf("/cpus/cpu@%d", - s->soc[socket].hartid_base + cpu); + cpu_name = g_strdup_printf("/cpus/cpu@%d", cpu_index); qemu_fdt_add_subnode(ms->fdt, cpu_name); if (cpu_ptr->cfg.satp_mode.supported != 0) { @@ -275,8 +277,7 @@ static void create_fdt_socket_cpus(RISCVVirtState *s, int socket, qemu_fdt_setprop_string(ms->fdt, cpu_name, "compatible", "riscv"); qemu_fdt_setprop_string(ms->fdt, cpu_name, "status", "okay"); - qemu_fdt_setprop_cell(ms->fdt, cpu_name, "reg", - s->soc[socket].hartid_base + cpu); + qemu_fdt_setprop_cell(ms->fdt, cpu_name, "reg", cpu_index); qemu_fdt_setprop_string(ms->fdt, cpu_name, "device_type", "cpu"); riscv_socket_fdt_write_id(ms, cpu_name, socket); qemu_fdt_setprop_cell(ms->fdt, cpu_name, "phandle", cpu_phandle); @@ -717,12 +718,12 @@ static void create_fdt_pmu(RISCVVirtState *s) { char *pmu_name; MachineState *ms = MACHINE(s); - RISCVCPU hart = s->soc[0].harts[0]; + RISCVCPU *hart = RISCV_CPU(qemu_get_cpu(0)); pmu_name = g_strdup_printf("/pmu"); qemu_fdt_add_subnode(ms->fdt, pmu_name); qemu_fdt_setprop_string(ms->fdt, pmu_name, "compatible", "riscv,pmu"); - riscv_pmu_generate_fdt_node(ms->fdt, hart.cfg.pmu_num, pmu_name); + riscv_pmu_generate_fdt_node(ms->fdt, hart->cfg.pmu_num, pmu_name); g_free(pmu_name); } -- 2.39.2 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC PATCH v2 1/2] hw/riscv: hart: replace array access with qemu_get_cpu() 2023-09-14 8:07 ` [RFC PATCH v2 1/2] hw/riscv: hart: replace array access with qemu_get_cpu() Nikita Shubin @ 2023-09-18 1:50 ` Alistair Francis 2023-09-18 10:22 ` Nikita Shubin 0 siblings, 1 reply; 6+ messages in thread From: Alistair Francis @ 2023-09-18 1:50 UTC (permalink / raw) To: Nikita Shubin Cc: Palmer Dabbelt, Alistair Francis, Bin Meng, Weiwei Li, Daniel Henrique Barboza, Liu Zhiwei, Sunil V L, Nikita Shubin, qemu-riscv, qemu-devel On Thu, Sep 14, 2023 at 6:09 PM Nikita Shubin <nikita.shubin@maquefel.me> wrote: > > From: Nikita Shubin <n.shubin@yadro.com> > > Replace all RISCVHartArrayState->harts[idx] with > qemu_get_cpu()/cpu_by_arch_id(). Thanks for the patch Why do we want this change though? > > cpu_index is guaranteed to be continuus by cpu_get_free_index(), so they > can be accessed in same order they were added. > > "Hart IDs might not necessarily be numbered contiguously in a > multiprocessor system, but at least one hart must have > a hart ID of zero." > > This states that hart ID zero should always be present, this makes using > cpu_by_arch_id(0) safe. > > Signed-off-by: Nikita Shubin <n.shubin@yadro.com> > --- > hw/riscv/boot.c | 6 ++++-- > hw/riscv/sifive_u.c | 7 +++++-- > hw/riscv/spike.c | 17 ++++++++++------- > hw/riscv/virt-acpi-build.c | 2 +- > hw/riscv/virt.c | 17 +++++++++-------- > 5 files changed, 29 insertions(+), 20 deletions(-) > > diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c > index 52bf8e67de..041f966e58 100644 > --- a/hw/riscv/boot.c > +++ b/hw/riscv/boot.c > @@ -36,7 +36,8 @@ > > bool riscv_is_32bit(RISCVHartArrayState *harts) > { > - return harts->harts[0].env.misa_mxl_max == MXL_RV32; > + RISCVCPU *hart = RISCV_CPU(cpu_by_arch_id(0)); > + return hart->env.misa_mxl_max == MXL_RV32; > } > > /* > @@ -385,6 +386,7 @@ void riscv_setup_rom_reset_vec(MachineState *machine, RISCVHartArrayState *harts > uint64_t fdt_load_addr) > { > int i; > + RISCVCPU *hart = RISCV_CPU(cpu_by_arch_id(0)); > uint32_t start_addr_hi32 = 0x00000000; > uint32_t fdt_load_addr_hi32 = 0x00000000; > > @@ -414,7 +416,7 @@ void riscv_setup_rom_reset_vec(MachineState *machine, RISCVHartArrayState *harts > reset_vec[4] = 0x0182b283; /* ld t0, 24(t0) */ > } > > - if (!harts->harts[0].cfg.ext_icsr) { > + if (!hart->cfg.ext_icsr) { > /* > * The Zicsr extension has been disabled, so let's ensure we don't > * run the CSR instruction. Let's fill the address with a non > diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c > index ec76dce6c9..3d09d0ee0e 100644 > --- a/hw/riscv/sifive_u.c > +++ b/hw/riscv/sifive_u.c > @@ -168,6 +168,7 @@ static void create_fdt(SiFiveUState *s, const MemMapEntry *memmap, > qemu_fdt_setprop_cell(fdt, "/cpus", "#address-cells", 0x1); > > for (cpu = ms->smp.cpus - 1; cpu >= 0; cpu--) { > + RISCVCPU *hart; > int cpu_phandle = phandle++; > nodename = g_strdup_printf("/cpus/cpu@%d", cpu); > char *intc = g_strdup_printf("/cpus/cpu@%d/interrupt-controller", cpu); > @@ -180,9 +181,11 @@ 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]); > + hart = RISCV_CPU(qemu_get_cpu(cpu - 1)); > + isa = riscv_isa_string(hart); This doesn't look right. The existing code accesses the u_cpus/e_cpus. The new code doesn't do that. You need to change this offset based on the number of e/u cpus (depending on order). Alistair > } else { > - isa = riscv_isa_string(&s->soc.e_cpus.harts[0]); > + hart = RISCV_CPU(qemu_get_cpu(0)); > + isa = riscv_isa_string(hart); > } > qemu_fdt_setprop_string(fdt, nodename, "riscv,isa", isa); > qemu_fdt_setprop_string(fdt, nodename, "compatible", "riscv"); > diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c > index 81f7e53aed..f3ec6427a1 100644 > --- a/hw/riscv/spike.c > +++ b/hw/riscv/spike.c > @@ -97,29 +97,32 @@ static void create_fdt(SpikeState *s, const MemMapEntry *memmap, > qemu_fdt_add_subnode(fdt, "/cpus/cpu-map"); > > for (socket = (riscv_socket_count(ms) - 1); socket >= 0; socket--) { > + uint32_t num_harts = s->soc[socket].num_harts; > + uint32_t hartid_base = s->soc[socket].hartid_base; > + > clust_name = g_strdup_printf("/cpus/cpu-map/cluster%d", socket); > qemu_fdt_add_subnode(fdt, clust_name); > > - clint_cells = g_new0(uint32_t, s->soc[socket].num_harts * 4); > + clint_cells = g_new0(uint32_t, num_harts * 4); > > - for (cpu = s->soc[socket].num_harts - 1; cpu >= 0; cpu--) { > + for (cpu = num_harts - 1; cpu >= 0; cpu--) { > + int cpu_index = hartid_base + cpu; > + RISCVCPU *hart = RISCV_CPU(qemu_get_cpu(cpu_index)); > cpu_phandle = phandle++; > > - cpu_name = g_strdup_printf("/cpus/cpu@%d", > - s->soc[socket].hartid_base + cpu); > + cpu_name = g_strdup_printf("/cpus/cpu@%d", cpu_index); > qemu_fdt_add_subnode(fdt, cpu_name); > if (is_32_bit) { > qemu_fdt_setprop_string(fdt, cpu_name, "mmu-type", "riscv,sv32"); > } else { > qemu_fdt_setprop_string(fdt, cpu_name, "mmu-type", "riscv,sv48"); > } > - name = riscv_isa_string(&s->soc[socket].harts[cpu]); > + name = riscv_isa_string(hart); > qemu_fdt_setprop_string(fdt, cpu_name, "riscv,isa", name); > g_free(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", > - s->soc[socket].hartid_base + cpu); > + qemu_fdt_setprop_cell(fdt, cpu_name, "reg", cpu_index); > qemu_fdt_setprop_string(fdt, cpu_name, "device_type", "cpu"); > riscv_socket_fdt_write_id(ms, cpu_name, socket); > qemu_fdt_setprop_cell(fdt, cpu_name, "phandle", cpu_phandle); > diff --git a/hw/riscv/virt-acpi-build.c b/hw/riscv/virt-acpi-build.c > index 7331248f59..d885220cc9 100644 > --- a/hw/riscv/virt-acpi-build.c > +++ b/hw/riscv/virt-acpi-build.c > @@ -158,7 +158,7 @@ static void build_rhct(GArray *table_data, > isa_offset = table_data->len - table.table_offset; > build_append_int_noprefix(table_data, 0, 2); /* Type 0 */ > > - cpu = &s->soc[0].harts[0]; > + cpu = RISCV_CPU(cpu_by_arch_id(0)); > isa = riscv_isa_string(cpu); > len = 8 + strlen(isa) + 1; > aligned_len = (len % 2) ? (len + 1) : len; > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c > index 5edc1d98d2..f3132ecc1b 100644 > --- a/hw/riscv/virt.c > +++ b/hw/riscv/virt.c > @@ -239,16 +239,18 @@ static void create_fdt_socket_cpus(RISCVVirtState *s, int socket, > uint32_t cpu_phandle; > MachineState *ms = MACHINE(s); > char *name, *cpu_name, *core_name, *intc_name, *sv_name; > + uint32_t num_harts = s->soc[socket].num_harts; > + uint32_t hartid_base = s->soc[socket].hartid_base; > bool is_32_bit = riscv_is_32bit(&s->soc[0]); > uint8_t satp_mode_max; > > - for (cpu = s->soc[socket].num_harts - 1; cpu >= 0; cpu--) { > - RISCVCPU *cpu_ptr = &s->soc[socket].harts[cpu]; > + for (cpu = num_harts - 1; cpu >= 0; cpu--) { > + int cpu_index = hartid_base + cpu; > + RISCVCPU *cpu_ptr = RISCV_CPU(qemu_get_cpu(cpu_index)); > > cpu_phandle = (*phandle)++; > > - cpu_name = g_strdup_printf("/cpus/cpu@%d", > - s->soc[socket].hartid_base + cpu); > + cpu_name = g_strdup_printf("/cpus/cpu@%d", cpu_index); > qemu_fdt_add_subnode(ms->fdt, cpu_name); > > if (cpu_ptr->cfg.satp_mode.supported != 0) { > @@ -275,8 +277,7 @@ static void create_fdt_socket_cpus(RISCVVirtState *s, int socket, > > qemu_fdt_setprop_string(ms->fdt, cpu_name, "compatible", "riscv"); > qemu_fdt_setprop_string(ms->fdt, cpu_name, "status", "okay"); > - qemu_fdt_setprop_cell(ms->fdt, cpu_name, "reg", > - s->soc[socket].hartid_base + cpu); > + qemu_fdt_setprop_cell(ms->fdt, cpu_name, "reg", cpu_index); > qemu_fdt_setprop_string(ms->fdt, cpu_name, "device_type", "cpu"); > riscv_socket_fdt_write_id(ms, cpu_name, socket); > qemu_fdt_setprop_cell(ms->fdt, cpu_name, "phandle", cpu_phandle); > @@ -717,12 +718,12 @@ static void create_fdt_pmu(RISCVVirtState *s) > { > char *pmu_name; > MachineState *ms = MACHINE(s); > - RISCVCPU hart = s->soc[0].harts[0]; > + RISCVCPU *hart = RISCV_CPU(qemu_get_cpu(0)); > > pmu_name = g_strdup_printf("/pmu"); > qemu_fdt_add_subnode(ms->fdt, pmu_name); > qemu_fdt_setprop_string(ms->fdt, pmu_name, "compatible", "riscv,pmu"); > - riscv_pmu_generate_fdt_node(ms->fdt, hart.cfg.pmu_num, pmu_name); > + riscv_pmu_generate_fdt_node(ms->fdt, hart->cfg.pmu_num, pmu_name); > > g_free(pmu_name); > } > -- > 2.39.2 > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH v2 1/2] hw/riscv: hart: replace array access with qemu_get_cpu() 2023-09-18 1:50 ` Alistair Francis @ 2023-09-18 10:22 ` Nikita Shubin 0 siblings, 0 replies; 6+ messages in thread From: Nikita Shubin @ 2023-09-18 10:22 UTC (permalink / raw) To: Alistair Francis Cc: Palmer Dabbelt, Alistair Francis, Bin Meng, Weiwei Li, Daniel Henrique Barboza, Liu Zhiwei, Sunil V L, Nikita Shubin, qemu-riscv, qemu-devel Hello Alistair! On Mon, 2023-09-18 at 11:50 +1000, Alistair Francis wrote: > On Thu, Sep 14, 2023 at 6:09 PM Nikita Shubin > <nikita.shubin@maquefel.me> wrote: > > > > From: Nikita Shubin <n.shubin@yadro.com> > > > > Replace all RISCVHartArrayState->harts[idx] with > > qemu_get_cpu()/cpu_by_arch_id(). > > Thanks for the patch > > Why do we want this change though? The main reason for these patches is that CPU Instance overloading would be nice. Currently it is not possible to overload instance of RISCVCPU, i.e. something like this: static const TypeInfo riscv_cpu_type_infos[] = { { .name = TYPE_ANOTHER_RISCV_CPU, .parent = TYPE_RISCV_CPU, .instance_size = sizeof(MyCPUState), .instance_init = riscv_my_cpu_init, } }; i.e. .instance_size != sizeof(RISCVCPU). Because we have RISCVHartArrayState.harts with exactly the size of RISCVCPU. So either we should avoid using RISCVHartArrayState (which is not desirable as we can't switch "cpu_type" for existing machines) in such case or make RISCVHartArrayState accept arbitrary "cpu_type" size. The previous attempt looks very ugly, indeed: ``` static inline RISCVCPU *riscv_array_get_hart(RISCVHartArrayState harts, int i) { return harts->harts[i]; } ``` https://patchwork.kernel.org/project/qemu-devel/patch/20230727080545.7908-1-nikita.shubin@maquefel.me/ So qemu_get_cpu()/cpu_by_arch_id() looks much more cleaner and universal. > > > > > cpu_index is guaranteed to be continuus by cpu_get_free_index(), so > > they > > can be accessed in same order they were added. > > > > "Hart IDs might not necessarily be numbered contiguously in a > > multiprocessor system, but at least one hart must have > > a hart ID of zero." > > > > This states that hart ID zero should always be present, this makes > > using > > cpu_by_arch_id(0) safe. > > > > Signed-off-by: Nikita Shubin <n.shubin@yadro.com> > > --- > > hw/riscv/boot.c | 6 ++++-- > > hw/riscv/sifive_u.c | 7 +++++-- > > hw/riscv/spike.c | 17 ++++++++++------- > > hw/riscv/virt-acpi-build.c | 2 +- > > hw/riscv/virt.c | 17 +++++++++-------- > > 5 files changed, 29 insertions(+), 20 deletions(-) > > > > diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c > > index 52bf8e67de..041f966e58 100644 > > --- a/hw/riscv/boot.c > > +++ b/hw/riscv/boot.c > > @@ -36,7 +36,8 @@ > > > > bool riscv_is_32bit(RISCVHartArrayState *harts) > > { > > - return harts->harts[0].env.misa_mxl_max == MXL_RV32; > > + RISCVCPU *hart = RISCV_CPU(cpu_by_arch_id(0)); > > + return hart->env.misa_mxl_max == MXL_RV32; > > } > > > > /* > > @@ -385,6 +386,7 @@ void riscv_setup_rom_reset_vec(MachineState > > *machine, RISCVHartArrayState *harts > > uint64_t fdt_load_addr) > > { > > int i; > > + RISCVCPU *hart = RISCV_CPU(cpu_by_arch_id(0)); > > uint32_t start_addr_hi32 = 0x00000000; > > uint32_t fdt_load_addr_hi32 = 0x00000000; > > > > @@ -414,7 +416,7 @@ void riscv_setup_rom_reset_vec(MachineState > > *machine, RISCVHartArrayState *harts > > reset_vec[4] = 0x0182b283; /* ld t0, 24(t0) */ > > } > > > > - if (!harts->harts[0].cfg.ext_icsr) { > > + if (!hart->cfg.ext_icsr) { > > /* > > * The Zicsr extension has been disabled, so let's ensure > > we don't > > * run the CSR instruction. Let's fill the address with a > > non > > diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c > > index ec76dce6c9..3d09d0ee0e 100644 > > --- a/hw/riscv/sifive_u.c > > +++ b/hw/riscv/sifive_u.c > > @@ -168,6 +168,7 @@ static void create_fdt(SiFiveUState *s, const > > MemMapEntry *memmap, > > qemu_fdt_setprop_cell(fdt, "/cpus", "#address-cells", 0x1); > > > > for (cpu = ms->smp.cpus - 1; cpu >= 0; cpu--) { > > + RISCVCPU *hart; > > int cpu_phandle = phandle++; > > nodename = g_strdup_printf("/cpus/cpu@%d", cpu); > > char *intc = g_strdup_printf("/cpus/cpu@%d/interrupt- > > controller", cpu); > > @@ -180,9 +181,11 @@ 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]); > > + hart = RISCV_CPU(qemu_get_cpu(cpu - 1)); > > + isa = riscv_isa_string(hart); > > This doesn't look right. The existing code accesses the > u_cpus/e_cpus. > The new code doesn't do that. You need to change this offset based on > the number of e/u cpus (depending on order). > > Alistair > > > } else { > > - isa = riscv_isa_string(&s->soc.e_cpus.harts[0]); > > + hart = RISCV_CPU(qemu_get_cpu(0)); > > + isa = riscv_isa_string(hart); > > } > > qemu_fdt_setprop_string(fdt, nodename, "riscv,isa", isa); > > qemu_fdt_setprop_string(fdt, nodename, "compatible", > > "riscv"); > > diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c > > index 81f7e53aed..f3ec6427a1 100644 > > --- a/hw/riscv/spike.c > > +++ b/hw/riscv/spike.c > > @@ -97,29 +97,32 @@ static void create_fdt(SpikeState *s, const > > MemMapEntry *memmap, > > qemu_fdt_add_subnode(fdt, "/cpus/cpu-map"); > > > > for (socket = (riscv_socket_count(ms) - 1); socket >= 0; > > socket--) { > > + uint32_t num_harts = s->soc[socket].num_harts; > > + uint32_t hartid_base = s->soc[socket].hartid_base; > > + > > clust_name = g_strdup_printf("/cpus/cpu-map/cluster%d", > > socket); > > qemu_fdt_add_subnode(fdt, clust_name); > > > > - clint_cells = g_new0(uint32_t, s->soc[socket].num_harts * > > 4); > > + clint_cells = g_new0(uint32_t, num_harts * 4); > > > > - for (cpu = s->soc[socket].num_harts - 1; cpu >= 0; cpu--) > > { > > + for (cpu = num_harts - 1; cpu >= 0; cpu--) { > > + int cpu_index = hartid_base + cpu; > > + RISCVCPU *hart = RISCV_CPU(qemu_get_cpu(cpu_index)); > > cpu_phandle = phandle++; > > > > - cpu_name = g_strdup_printf("/cpus/cpu@%d", > > - s->soc[socket].hartid_base + cpu); > > + cpu_name = g_strdup_printf("/cpus/cpu@%d", cpu_index); > > qemu_fdt_add_subnode(fdt, cpu_name); > > if (is_32_bit) { > > qemu_fdt_setprop_string(fdt, cpu_name, "mmu-type", > > "riscv,sv32"); > > } else { > > qemu_fdt_setprop_string(fdt, cpu_name, "mmu-type", > > "riscv,sv48"); > > } > > - name = riscv_isa_string(&s->soc[socket].harts[cpu]); > > + name = riscv_isa_string(hart); > > qemu_fdt_setprop_string(fdt, cpu_name, "riscv,isa", > > name); > > g_free(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", > > - s->soc[socket].hartid_base + cpu); > > + qemu_fdt_setprop_cell(fdt, cpu_name, "reg", > > cpu_index); > > qemu_fdt_setprop_string(fdt, cpu_name, "device_type", > > "cpu"); > > riscv_socket_fdt_write_id(ms, cpu_name, socket); > > qemu_fdt_setprop_cell(fdt, cpu_name, "phandle", > > cpu_phandle); > > diff --git a/hw/riscv/virt-acpi-build.c b/hw/riscv/virt-acpi- > > build.c > > index 7331248f59..d885220cc9 100644 > > --- a/hw/riscv/virt-acpi-build.c > > +++ b/hw/riscv/virt-acpi-build.c > > @@ -158,7 +158,7 @@ static void build_rhct(GArray *table_data, > > isa_offset = table_data->len - table.table_offset; > > build_append_int_noprefix(table_data, 0, 2); /* Type 0 */ > > > > - cpu = &s->soc[0].harts[0]; > > + cpu = RISCV_CPU(cpu_by_arch_id(0)); > > isa = riscv_isa_string(cpu); > > len = 8 + strlen(isa) + 1; > > aligned_len = (len % 2) ? (len + 1) : len; > > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c > > index 5edc1d98d2..f3132ecc1b 100644 > > --- a/hw/riscv/virt.c > > +++ b/hw/riscv/virt.c > > @@ -239,16 +239,18 @@ static void > > create_fdt_socket_cpus(RISCVVirtState *s, int socket, > > uint32_t cpu_phandle; > > MachineState *ms = MACHINE(s); > > char *name, *cpu_name, *core_name, *intc_name, *sv_name; > > + uint32_t num_harts = s->soc[socket].num_harts; > > + uint32_t hartid_base = s->soc[socket].hartid_base; > > bool is_32_bit = riscv_is_32bit(&s->soc[0]); > > uint8_t satp_mode_max; > > > > - for (cpu = s->soc[socket].num_harts - 1; cpu >= 0; cpu--) { > > - RISCVCPU *cpu_ptr = &s->soc[socket].harts[cpu]; > > + for (cpu = num_harts - 1; cpu >= 0; cpu--) { > > + int cpu_index = hartid_base + cpu; > > + RISCVCPU *cpu_ptr = RISCV_CPU(qemu_get_cpu(cpu_index)); > > > > cpu_phandle = (*phandle)++; > > > > - cpu_name = g_strdup_printf("/cpus/cpu@%d", > > - s->soc[socket].hartid_base + cpu); > > + cpu_name = g_strdup_printf("/cpus/cpu@%d", cpu_index); > > qemu_fdt_add_subnode(ms->fdt, cpu_name); > > > > if (cpu_ptr->cfg.satp_mode.supported != 0) { > > @@ -275,8 +277,7 @@ static void > > create_fdt_socket_cpus(RISCVVirtState *s, int socket, > > > > qemu_fdt_setprop_string(ms->fdt, cpu_name, "compatible", > > "riscv"); > > qemu_fdt_setprop_string(ms->fdt, cpu_name, "status", > > "okay"); > > - qemu_fdt_setprop_cell(ms->fdt, cpu_name, "reg", > > - s->soc[socket].hartid_base + cpu); > > + qemu_fdt_setprop_cell(ms->fdt, cpu_name, "reg", > > cpu_index); > > qemu_fdt_setprop_string(ms->fdt, cpu_name, "device_type", > > "cpu"); > > riscv_socket_fdt_write_id(ms, cpu_name, socket); > > qemu_fdt_setprop_cell(ms->fdt, cpu_name, "phandle", > > cpu_phandle); > > @@ -717,12 +718,12 @@ static void create_fdt_pmu(RISCVVirtState *s) > > { > > char *pmu_name; > > MachineState *ms = MACHINE(s); > > - RISCVCPU hart = s->soc[0].harts[0]; > > + RISCVCPU *hart = RISCV_CPU(qemu_get_cpu(0)); > > > > pmu_name = g_strdup_printf("/pmu"); > > qemu_fdt_add_subnode(ms->fdt, pmu_name); > > qemu_fdt_setprop_string(ms->fdt, pmu_name, "compatible", > > "riscv,pmu"); > > - riscv_pmu_generate_fdt_node(ms->fdt, hart.cfg.pmu_num, > > pmu_name); > > + riscv_pmu_generate_fdt_node(ms->fdt, hart->cfg.pmu_num, > > pmu_name); > > > > g_free(pmu_name); > > } > > -- > > 2.39.2 > > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* [RFC PATCH v2 2/2] hw/riscv: hart: allow other cpu instance 2023-09-14 8:07 [RFC PATCH v2 0/2] hw/riscv: hart: allow other cpu instance Nikita Shubin 2023-09-14 8:07 ` [RFC PATCH v2 1/2] hw/riscv: hart: replace array access with qemu_get_cpu() Nikita Shubin @ 2023-09-14 8:07 ` Nikita Shubin 2023-09-18 1:53 ` Alistair Francis 1 sibling, 1 reply; 6+ messages in thread From: Nikita Shubin @ 2023-09-14 8:07 UTC (permalink / raw) To: Palmer Dabbelt, Alistair Francis, Bin Meng, Weiwei Li, Daniel Henrique Barboza, Liu Zhiwei, Sunil V L Cc: Nikita Shubin, qemu-riscv, qemu-devel From: Nikita Shubin <n.shubin@yadro.com> Allow using instances derivative from RISCVCPU Signed-off-by: Nikita Shubin <n.shubin@yadro.com> --- hw/riscv/riscv_hart.c | 20 ++++++++++++-------- include/hw/riscv/riscv_hart.h | 2 +- 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/hw/riscv/riscv_hart.c b/hw/riscv/riscv_hart.c index 613ea2aaa0..020ba18e8b 100644 --- a/hw/riscv/riscv_hart.c +++ b/hw/riscv/riscv_hart.c @@ -43,24 +43,28 @@ static void riscv_harts_cpu_reset(void *opaque) } static bool riscv_hart_realize(RISCVHartArrayState *s, int idx, - char *cpu_type, Error **errp) + char *cpu_type, size_t size, Error **errp) { - object_initialize_child(OBJECT(s), "harts[*]", &s->harts[idx], cpu_type); - qdev_prop_set_uint64(DEVICE(&s->harts[idx]), "resetvec", s->resetvec); - s->harts[idx].env.mhartid = s->hartid_base + idx; - qemu_register_reset(riscv_harts_cpu_reset, &s->harts[idx]); - return qdev_realize(DEVICE(&s->harts[idx]), NULL, errp); + RISCVCPU *hart = s->harts[idx]; + object_initialize_child_internal(OBJECT(s), "harts[*]", + hart, size, cpu_type); + qdev_prop_set_uint64(DEVICE(hart), "resetvec", s->resetvec); + hart->env.mhartid = s->hartid_base + idx; + qemu_register_reset(riscv_harts_cpu_reset, hart); + return qdev_realize(DEVICE(hart), NULL, errp); } static void riscv_harts_realize(DeviceState *dev, Error **errp) { RISCVHartArrayState *s = RISCV_HART_ARRAY(dev); + size_t size = object_type_get_instance_size(s->cpu_type); int n; - s->harts = g_new0(RISCVCPU, s->num_harts); + s->harts = g_new0(RISCVCPU *, s->num_harts); for (n = 0; n < s->num_harts; n++) { - if (!riscv_hart_realize(s, n, s->cpu_type, errp)) { + s->harts[n] = RISCV_CPU(object_new(s->cpu_type)); + if (!riscv_hart_realize(s, n, s->cpu_type, size, errp)) { return; } } diff --git a/include/hw/riscv/riscv_hart.h b/include/hw/riscv/riscv_hart.h index 912b4a2682..5f6ef06411 100644 --- a/include/hw/riscv/riscv_hart.h +++ b/include/hw/riscv/riscv_hart.h @@ -38,7 +38,7 @@ struct RISCVHartArrayState { uint32_t hartid_base; char *cpu_type; uint64_t resetvec; - RISCVCPU *harts; + RISCVCPU **harts; }; #endif -- 2.39.2 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC PATCH v2 2/2] hw/riscv: hart: allow other cpu instance 2023-09-14 8:07 ` [RFC PATCH v2 2/2] hw/riscv: hart: allow other cpu instance Nikita Shubin @ 2023-09-18 1:53 ` Alistair Francis 0 siblings, 0 replies; 6+ messages in thread From: Alistair Francis @ 2023-09-18 1:53 UTC (permalink / raw) To: Nikita Shubin Cc: Palmer Dabbelt, Alistair Francis, Bin Meng, Weiwei Li, Daniel Henrique Barboza, Liu Zhiwei, Sunil V L, Nikita Shubin, qemu-riscv, qemu-devel On Thu, Sep 14, 2023 at 6:09 PM Nikita Shubin <nikita.shubin@maquefel.me> wrote: > > From: Nikita Shubin <n.shubin@yadro.com> > > Allow using instances derivative from RISCVCPU > > Signed-off-by: Nikita Shubin <n.shubin@yadro.com> > --- > hw/riscv/riscv_hart.c | 20 ++++++++++++-------- > include/hw/riscv/riscv_hart.h | 2 +- > 2 files changed, 13 insertions(+), 9 deletions(-) > > diff --git a/hw/riscv/riscv_hart.c b/hw/riscv/riscv_hart.c > index 613ea2aaa0..020ba18e8b 100644 > --- a/hw/riscv/riscv_hart.c > +++ b/hw/riscv/riscv_hart.c > @@ -43,24 +43,28 @@ static void riscv_harts_cpu_reset(void *opaque) > } > > static bool riscv_hart_realize(RISCVHartArrayState *s, int idx, > - char *cpu_type, Error **errp) > + char *cpu_type, size_t size, Error **errp) > { > - object_initialize_child(OBJECT(s), "harts[*]", &s->harts[idx], cpu_type); > - qdev_prop_set_uint64(DEVICE(&s->harts[idx]), "resetvec", s->resetvec); > - s->harts[idx].env.mhartid = s->hartid_base + idx; > - qemu_register_reset(riscv_harts_cpu_reset, &s->harts[idx]); > - return qdev_realize(DEVICE(&s->harts[idx]), NULL, errp); > + RISCVCPU *hart = s->harts[idx]; > + object_initialize_child_internal(OBJECT(s), "harts[*]", > + hart, size, cpu_type); > + qdev_prop_set_uint64(DEVICE(hart), "resetvec", s->resetvec); > + hart->env.mhartid = s->hartid_base + idx; > + qemu_register_reset(riscv_harts_cpu_reset, hart); > + return qdev_realize(DEVICE(hart), NULL, errp); > } > > static void riscv_harts_realize(DeviceState *dev, Error **errp) > { > RISCVHartArrayState *s = RISCV_HART_ARRAY(dev); > + size_t size = object_type_get_instance_size(s->cpu_type); > int n; > > - s->harts = g_new0(RISCVCPU, s->num_harts); > + s->harts = g_new0(RISCVCPU *, s->num_harts); We don't use this allocated memory, this line should be dropped Alistair > > for (n = 0; n < s->num_harts; n++) { > - if (!riscv_hart_realize(s, n, s->cpu_type, errp)) { > + s->harts[n] = RISCV_CPU(object_new(s->cpu_type)); > + if (!riscv_hart_realize(s, n, s->cpu_type, size, errp)) { > return; > } > } > diff --git a/include/hw/riscv/riscv_hart.h b/include/hw/riscv/riscv_hart.h > index 912b4a2682..5f6ef06411 100644 > --- a/include/hw/riscv/riscv_hart.h > +++ b/include/hw/riscv/riscv_hart.h > @@ -38,7 +38,7 @@ struct RISCVHartArrayState { > uint32_t hartid_base; > char *cpu_type; > uint64_t resetvec; > - RISCVCPU *harts; > + RISCVCPU **harts; > }; > > #endif > -- > 2.39.2 > > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-09-18 7:23 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-09-14 8:07 [RFC PATCH v2 0/2] hw/riscv: hart: allow other cpu instance Nikita Shubin 2023-09-14 8:07 ` [RFC PATCH v2 1/2] hw/riscv: hart: replace array access with qemu_get_cpu() Nikita Shubin 2023-09-18 1:50 ` Alistair Francis 2023-09-18 10:22 ` Nikita Shubin 2023-09-14 8:07 ` [RFC PATCH v2 2/2] hw/riscv: hart: allow other cpu instance Nikita Shubin 2023-09-18 1:53 ` Alistair Francis
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).