hw/riscv: describe only the PLIC contexts that exist
Subsystems (2)
v3 cover hw/riscv: describe only the PLIC contexts that exist
A PLIC context is addressed by its position in the 'interrupts-extended' property, so the device tree and the device have to agree on which harts contribute a context and in which order. On the virt machine they do
not: the device is built from riscv_plic_hart_config_string(), which gives a hart a supervisor context only if it has supervisor mode, while the device tree lists a machine and a supervisor context for every hart, unconditionally. Boot with
-cpu rv64,h=false,s=false,u=false -smp 4
and the PLIC implements four contexts while the device tree advertises eight. A guest that takes its context number from the property then gets it wrong for every hart but the first. This is what an M-mode kernel on virt runs into. sifive_u hardcodes the fu540 topology instead and has the same latent problem, reachable with -cpu since the board restricts neither the CPU type nor its extensions.
1 to 3 are the fix, 4 is cleanup and can be dropped, the rest does not depend on it.
Tested by comparing the PLIC's 'interrupts-extended' against the hart-config string of the PLIC device over 14 configurations of both
boards: 7 disagree before the series, all agree after, and the device tree is unchanged wherever the default CPU is used. The KVM paths could not be exercised, for lack of hardware.
v3:
- 4/4: the fdt helper takes has_m_mode from the board rather than calling kvm_enabled() itself, keeping accelerator considerations in the boards (Daniel)
v2:
- ask riscv_cpu_has_supervisor_mode() instead of reading RVS out of the hart's CPURISCVState (Daniel)
- fold the KVM branch into the common loop, it describes the same rule
- fix sifive_u as well, and share the cell building between both boards
v1: https://lore.kernel.org/qemu-devel/[email protected]/
v2: https://lore.kernel.org/qemu-devel/[email protected]/
Ralf Ramsauer (4):
target/riscv: add riscv_cpu_has_supervisor_mode() hw/riscv/virt: only describe PLIC contexts that exist hw/riscv/sifive_u: derive PLIC contexts from the harts hw/riscv: share the PLIC interrupt cells between boards
hw/riscv/fdt-common.c | 18 ++++++++++++++++++ hw/riscv/sifive_u.c | 19 ++++++++----------- hw/riscv/virt.c | 26 +++++++++----------------- include/hw/riscv/fdt-common.h | 3 +++ target/riscv/cpu.c | 14 ++++++++++++++ target/riscv/cpu.h | 1 + 6 files changed, 53 insertions(+), 28 deletions(-)
base-commit: 4f4225874d4d3747ec1142e935dbd314218992fa
Signature · 1 line
2.54.0
PATCH v3 1/4 target/riscv: add riscv_cpu_has_supervisor_mode()
From: Ralf Ramsauer <[email protected]>
Whether a hart has supervisor mode is decided in two places: KVM harts always have it, and for TCG it follows from RVS in the ISA string. riscv_plic_hart_config_string() already spells out both cases.
Board code needs the same answer to describe the interrupt controller, but it should not have to reach into CPURISCVState for it. Add a helper next to riscv_cpu_is_vendor() so the next patches can ask the CPU instead of inspecting its internals.
--- target/riscv/cpu.c | 14 ++++++++++++++ target/riscv/cpu.h | 1 + 2 files changed, 15 insertions(+)
target/riscv/cpu.c +14 -0
@@ -338,6 +338,20 @@ bool riscv_cpu_is_vendor(Object *cpu_obj)return object_dynamic_cast (cpu_obj,TYPE_RISCV_VENDOR_CPU ) !=NULL ; } +bool riscv_cpu_has_supervisor_mode (RISCVCPU *cpu) +{ +#ifndef CONFIG_USER_ONLY +/* KVM always provides supervisor mode, regardless of the ISA string */ +if (kvm_enabled ()) { +return true; + } + +return riscv_has_ext (&cpu->env,RVS ); +#else +return false; +#endif +} +const char *const riscv_int_regnames[] = {"x0/zero" ,"x1/ra" ,"x2/sp" ,"x3/gp" ,"x4/tp" ,"x5/t0" ,"x6/t1" ,"x7/t2" ,"x8/s0" ,"x9/s1" ,"x10/a0" ,"x11/a1" ,"x12/a2" ,"x13/a3" ,
target/riscv/cpu.h +1 -0
@@ -995,6 +995,7 @@ void isa_ext_update_enabled(RISCVCPU *cpu, uint32_t ext_offset, bool en);bool isa_ext_is_enabled (RISCVCPU *cpu,uint32_t ext_offset);void riscv_cpu_set_misa_ext (CPURISCVState *env,uint32_t ext);bool riscv_cpu_is_vendor (Object *cpu_obj); +bool riscv_cpu_has_supervisor_mode (RISCVCPU *cpu); typedef struct isa_ext_data { const char *name;
Signature · 1 line
2.54.0
Re: [PATCH v3 1/4] target/riscv: add riscv_cpu_has_supervisor_mode()
Quoted 6 lines · Ralf Ramsauer
From: Ralf Ramsauer <[email protected]>
Whether a hart has supervisor mode is decided in two places: KVM harts always have it, and for TCG it follows from RVS in the ISA string.
riscv_plic_hart_config_string()already spells out both cases.
Quoted 8 lines
Board code needs the same answer to describe the interrupt controller, but it should not have to reach into CPURISCVState for it. Add a helper next to
riscv_cpu_is_vendor()so the next patches can ask the CPU instead of inspecting its internals.Suggested-by Daniel Henrique Barboza <[email protected]>Signed-off-by Ralf Ramsauer <[email protected]>Reviewed-by Daniel Henrique Barboza <[email protected]>
Thanks,
Chao
Quoted 45 lines
--- target/riscv/cpu.c | 14 ++++++++++++++ target/riscv/cpu.h | 1 + 2 files changed, 15 insertions(+)target/riscv/cpu.c +14
-0@@ -338,6 +338,20 @@ bool riscv_cpu_is_vendor(Object *cpu_obj)return object_dynamic_cast (cpu_obj,TYPE_RISCV_VENDOR_CPU ) !=NULL ; } +bool riscv_cpu_has_supervisor_mode (RISCVCPU *cpu) +{ +#ifndef CONFIG_USER_ONLY +/* KVM always provides supervisor mode, regardless of the ISA string */ +if (kvm_enabled ()) { +return true; + } + +return riscv_has_ext (&cpu->env,RVS ); +#else +return false; +#endif +} +const char *const riscv_int_regnames[] = {"x0/zero" ,"x1/ra" ,"x2/sp" ,"x3/gp" ,"x4/tp" ,"x5/t0" ,"x6/t1" ,"x7/t2" ,"x8/s0" ,"x9/s1" ,"x10/a0" ,"x11/a1" ,"x12/a2" ,"x13/a3" ,target/riscv/cpu.h +1
-0@@ -995,6 +995,7 @@ void isa_ext_update_enabled(RISCVCPU *cpu, uint32_t ext_offset, bool en);bool isa_ext_is_enabled (RISCVCPU *cpu,uint32_t ext_offset);void riscv_cpu_set_misa_ext (CPURISCVState *env,uint32_t ext);bool riscv_cpu_is_vendor (Object *cpu_obj); +bool riscv_cpu_has_supervisor_mode (RISCVCPU *cpu); typedef struct isa_ext_data { const char *name;Signature · 1 line
2.54.0
Re: [PATCH v3 1/4] target/riscv: add riscv_cpu_has_supervisor_mode()
In reply to Ralf RamsauerQuoted 14 lines · Ralf Ramsauer
From: Ralf Ramsauer <[email protected]>
Whether a hart has supervisor mode is decided in two places: KVM harts
always have it, and for TCG it follows from RVS in the ISA string.riscv_plic_hart_config_string()already spells out both cases.Board code needs the same answer to describe the interrupt controller,
but it should not have to reach into CPURISCVState for it. Add a helper
next toriscv_cpu_is_vendor()so the next patches can ask the CPU instead of inspecting its internals.
Quoted 5 lines
Suggested-by: Daniel Henrique Barboza
<[email protected]>Signed-off-by Ralf Ramsauer <[email protected]>Reviewed-by Daniel Henrique Barboza
Alistair
Quoted 47 lines
---
target/riscv/cpu.c | 14 ++++++++++++++
target/riscv/cpu.h | 1 +2 files changed, 15 insertions(+)target/riscv/cpu.c +0
-0@@ -338,6 +338,20 @@ bool riscv_cpu_is_vendor(Object *cpu_obj)return
object_dynamic_cast(cpu_obj, TYPE_RISCV_VENDOR_CPU)!= NULL;
}target/riscv/cpu.c +4
-0+bool riscv_cpu_has_supervisor_mode (RISCVCPU *cpu) +{ +#ifndef CONFIG_USER_ONLY + /* KVM always provides supervisor mode, regardless of the ISAstring */
+ if (kvm_enabled()) {
+ return true;
+ }target/riscv/cpu.c +7
-0+ + return riscv_has_ext(&cpu->env, RVS); +#else + return false; +#endif +} +const char * const riscv_int_regnames[] = {
"x0/zero", "x1/ra", "x2/sp", "x3/gp", "x4/tp", "x5/t0", "x6/t1",
"x7/t2", "x8/s0", "x9/s1", "x10/a0", "x11/a1", "x12/a2", "x13/a3",target/riscv/cpu.h +1
-0@@ -995,6 +995,7 @@ void isa_ext_update_enabled(RISCVCPU *cpu, uint32_t ext_offset, bool en); bool isa_ext_is_enabled(RISCVCPU *cpu, uint32_t ext_offset); void riscv_cpu_set_misa_ext(CPURISCVState *env, uint32_t ext); bool riscv_cpu_is_vendor(Object *cpu_obj); +bool riscv_cpu_has_supervisor_mode (RISCVCPU *cpu);
typedef structisa_ext_data{
const char *name;
PATCH v3 2/4 hw/riscv/virt: only describe PLIC contexts that exist
In reply to Ralf RamsauerFrom: Ralf Ramsauer <[email protected]>
The PLIC device is created from the topology string built by riscv_plic_hart_config_string(), which gives a hart a supervisor context only if that hart actually has supervisor mode. The device tree, in contrast, lists a machine and a supervisor context for every hart, unconditionally.
The two have disagreed since commit 7d10ff8a4d ("hw/riscv: virt: Use the PLIC config helper function") taught the device about harts without S-mode and left the device tree behind. Boot with
-cpu rv64,s=false,u=false -smp 4
and the PLIC implements four contexts while the device tree advertises eight. Contexts are numbered by their position in 'interrupts-extended', so a guest that takes its context number from the property gets it wrong for every hart but the first: hart 1 drives the context of hart 2, and the upper harts address contexts the PLIC never created. This is what an M-mode kernel on the virt machine runs into.
Emit the supervisor entry only for the harts that have supervisor mode. The KVM case then becomes the same rule, not a separate one: its harts have no M-mode and always have supervisor mode, so the common loop describes the single supervisor context per hart that it used to spell out on its own.
--- hw/riscv/virt.c | 28 ++++++++++++---------------- 1 file changed, 12 insertions(+), 16 deletions(-)
hw/riscv/virt.c +12 -16
@@ -156,23 +156,19 @@ static void create_fdt_socket_plic(RISCVVirtState *s, plic_name =g_strdup_printf ("/soc/interrupt-controller@%lx" , plic_addr); -if (kvm_enabled ()) { - cells_length = s->soc[socket].num_harts *2 ; - plic_cells =g_new0 (uint32_t, cells_length); - -for (cpu =0 ; cpu < s->soc[socket].num_harts; cpu++) { - plic_cells[cpu *2 +0 ] =cpu_to_be32 (intc_phandles[cpu]); - plic_cells[cpu *2 +1 ] =cpu_to_be32 (IRQ_S_EXT ); + plic_cells =g_new0 (uint32_t, + s->soc[socket].num_harts * (kvm_enabled () ?2 :4 )); + cells_length =0 ; + +for (cpu =0 ; cpu < s->soc[socket].num_harts; cpu++) { +if (!kvm_enabled ()) { + plic_cells[cells_length++] =cpu_to_be32 (intc_phandles[cpu]); + plic_cells[cells_length++] =cpu_to_be32 (IRQ_M_EXT ); } - }else { - cells_length = s->soc[socket].num_harts *4 ; - plic_cells =g_new0 (uint32_t, cells_length); - -for (cpu =0 ; cpu < s->soc[socket].num_harts; cpu++) { - plic_cells[cpu *4 +0 ] =cpu_to_be32 (intc_phandles[cpu]); - plic_cells[cpu *4 +1 ] =cpu_to_be32 (IRQ_M_EXT ); - plic_cells[cpu *4 +2 ] =cpu_to_be32 (intc_phandles[cpu]); - plic_cells[cpu *4 +3 ] =cpu_to_be32 (IRQ_S_EXT ); + +if (riscv_cpu_has_supervisor_mode (&s->soc[socket].harts[cpu])) { + plic_cells[cells_length++] =cpu_to_be32 (intc_phandles[cpu]); + plic_cells[cells_length++] =cpu_to_be32 (IRQ_S_EXT ); } }
Signature · 1 line
2.54.0
Re: [PATCH v3 2/4] hw/riscv/virt: only describe PLIC contexts that exist
Quoted 27 lines · Ralf Ramsauer
From: Ralf Ramsauer <[email protected]>
The PLIC device is created from the topology string built by
riscv_plic_hart_config_string(), which gives a hart a supervisor context only if that hart actually has supervisor mode. The device tree, in contrast, lists a machine and a supervisor context for every hart, unconditionally.The two have disagreed since commit
7d10ff8a4d("hw/riscv: virt: Use the PLIC config helper function") taught the device about harts without S-mode and left the device tree behind. Boot with-cpu rv64,s=false,u=false -smp 4and the PLIC implements four contexts while the device tree advertises eight. Contexts are numbered by their position in 'interrupts-extended', so a guest that takes its context number from the property gets it wrong for every hart but the first: hart 1 drives the context of hart 2, and the upper harts address contexts the PLIC never created. This is what an M-mode kernel on the virt machine runs into.
Emit the supervisor entry only for the harts that have supervisor mode. The KVM case then becomes the same rule, not a separate one: its harts have no M-mode and always have supervisor mode, so the common loop describes the single supervisor context per hart that it used to spell out on its own.
Quoted 3 lines
Fixes 7d10ff8a4de7 ("hw/riscv: virt: Use the PLIC config helper function")Signed-off-by Ralf Ramsauer <[email protected]>Reviewed-by Daniel Henrique Barboza <[email protected]>
Thanks,
Chao
Quoted 47 lines
--- hw/riscv/virt.c | 28 ++++++++++++---------------- 1 file changed, 12 insertions(+), 16 deletions(-)hw/riscv/virt.c +12
-16@@ -156,23 +156,19 @@ static void create_fdt_socket_plic(RISCVVirtState *s, plic_name =g_strdup_printf ("/soc/interrupt-controller@%lx" , plic_addr); -if (kvm_enabled ()) { - cells_length = s->soc[socket].num_harts *2 ; - plic_cells =g_new0 (uint32_t, cells_length); - -for (cpu =0 ; cpu < s->soc[socket].num_harts; cpu++) { - plic_cells[cpu *2 +0 ] =cpu_to_be32 (intc_phandles[cpu]); - plic_cells[cpu *2 +1 ] =cpu_to_be32 (IRQ_S_EXT ); + plic_cells =g_new0 (uint32_t, + s->soc[socket].num_harts * (kvm_enabled () ?2 :4 )); + cells_length =0 ; + +for (cpu =0 ; cpu < s->soc[socket].num_harts; cpu++) { +if (!kvm_enabled ()) { + plic_cells[cells_length++] =cpu_to_be32 (intc_phandles[cpu]); + plic_cells[cells_length++] =cpu_to_be32 (IRQ_M_EXT ); } - }else { - cells_length = s->soc[socket].num_harts *4 ; - plic_cells =g_new0 (uint32_t, cells_length); - -for (cpu =0 ; cpu < s->soc[socket].num_harts; cpu++) { - plic_cells[cpu *4 +0 ] =cpu_to_be32 (intc_phandles[cpu]); - plic_cells[cpu *4 +1 ] =cpu_to_be32 (IRQ_M_EXT ); - plic_cells[cpu *4 +2 ] =cpu_to_be32 (intc_phandles[cpu]); - plic_cells[cpu *4 +3 ] =cpu_to_be32 (IRQ_S_EXT ); + +if (riscv_cpu_has_supervisor_mode (&s->soc[socket].harts[cpu])) { + plic_cells[cells_length++] =cpu_to_be32 (intc_phandles[cpu]); + plic_cells[cells_length++] =cpu_to_be32 (IRQ_S_EXT ); } }Signature · 1 line
2.54.0
Re: [PATCH v3 2/4] hw/riscv/virt: only describe PLIC contexts that exist
In reply to Ralf RamsauerQuoted 37 lines · Ralf Ramsauer
From: Ralf Ramsauer <[email protected]>
The PLIC device is created from the topology string built by
riscv_plic_hart_config_string(), which gives a hart a supervisor context
only if that hart actually has supervisor mode. The device tree, in contrast, lists a machine and a supervisor context for every hart, unconditionally.The two have disagreed since commit
7d10ff8a4d("hw/riscv: virt: Use the
PLIC config helper function") taught the device about harts without S-mode and left the device tree behind. Boot with-cpu rv64,s=false,u=false -smp 4
and the PLIC implements four contexts while the device tree advertises
eight. Contexts are numbered by their position in 'interrupts- extended',
so a guest that takes its context number from the property gets it wrong
for every hart but the first: hart 1 drives the context of hart 2, and
the upper harts address contexts the PLIC never created. This is what an
M-mode kernel on the virt machine runs into.Emit the supervisor entry only for the harts that have supervisor mode.
The KVM case then becomes the same rule, not a separate one: its harts
have no M-mode and always have supervisor mode, so the common loop describes the single supervisor context per hart that it used to spell
out on its own.
Quoted 5 lines
Fixes 7d10ff8a4de7 ("hw/riscv: virt: Use the PLIC config helperfunction")
Signed-off-by Ralf Ramsauer <[email protected]>Reviewed-by Daniel Henrique Barboza
Alistair
Quoted 53 lines
---
hw/riscv/virt.c | 28 ++++++++++++----------------1 file changed, 12 insertions(+), 16 deletions(-)hw/riscv/virt.c +0
-0@@ -156,23 +156,19 @@ static void create_fdt_socket_plic(RISCVVirtState *s,
plic_name=g_strdup_printf("/soc/interrupt-controller@%lx", plic_addr);
- if (kvm_enabled()) {
-cells_length= s->soc[socket].num_harts * 2;
-plic_cells=g_new0(uint32_t, cells_length);
-
- for (cpu = 0; cpu < s->soc[socket].num_harts; cpu++) { - plic_cells[cpu * 2 + 0] =cpu_to_be32(intc_phandles[cpu]);
- plic_cells[cpu * 2 + 1] =cpu_to_be32(IRQ_S_EXT); +plic_cells= g_new0(uint32_t,
+ s->soc[socket].num_harts * (kvm_enabled()? 2 : 4));
+cells_length= 0;
+
+ for (cpu = 0; cpu < s->soc[socket].num_harts; cpu++) { + if (!kvm_enabled()) {
+ plic_cells[cells_length++] =cpu_to_be32(intc_phandles[cpu]);
+ plic_cells[cells_length++] =cpu_to_be32(IRQ_M_EXT); }
- } else {
-cells_length= s->soc[socket].num_harts * 4;
-plic_cells=g_new0(uint32_t, cells_length);
-
- for (cpu = 0; cpu < s->soc[socket].num_harts; cpu++) { - plic_cells[cpu * 4 + 0] =cpu_to_be32(intc_phandles[cpu]);
- plic_cells[cpu * 4 + 1] =cpu_to_be32(IRQ_M_EXT); - plic_cells[cpu * 4 + 2] =cpu_to_be32(intc_phandles[cpu]);
- plic_cells[cpu * 4 + 3] =cpu_to_be32(IRQ_S_EXT); +
+ if (riscv_cpu_has_supervisor_mode(&s-Quoted 1 line
soc[socket].harts[cpu])) {
+ plic_cells[cells_length++] =
cpu_to_be32(intc_phandles[cpu]);
+ plic_cells[cells_length++] =cpu_to_be32(IRQ_S_EXT); }
}
PATCH v3 3/4 hw/riscv/sifive_u: derive PLIC contexts from the harts
In reply to Ralf RamsauerFrom: Ralf Ramsauer <[email protected]>
The PLIC device is created from riscv_plic_hart_config_string(), which gives a hart a supervisor context only if that hart has supervisor mode. The device tree instead hardcodes the fu540 topology: no supervisor context for the management hart, one for every other hart.
Both agree as long as the application harts keep the default CPU type. The board sets no valid_cpu_types, though, and passes machine->cpu_type on to the U cluster, so -cpu rv64,s=false,u=false builds a PLIC without any supervisor context while the device tree still advertises one per application hart. Contexts are numbered by their position in 'interrupts-extended', so from the second hart on the numbering drifts apart, as it does on the virt machine.
Ask the harts instead of assuming, like create_fdt_socket_plic() does. No change for the default CPU type: the E51 management hart has no RVS, the U54 application harts have.
--- hw/riscv/sifive_u.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-)
hw/riscv/sifive_u.c +11 -10
@@ -99,6 +99,7 @@ static void create_fdt(SiFiveUState *s, const MemMapEntry *memmap,MachineState *ms =MACHINE (s);void *fdt;int cpu; +RISCVCPU *hart;uint32_t *cells, cells_length;char *nodename;uint32_t plic_phandle, prci_phandle, gpio_phandle, phandle =1 ;
@@ -195,18 +196,18 @@ static void create_fdt(SiFiveUState *s, const MemMapEntry *memmap,g_free (nodename); plic_phandle = phandle++; - cells_length = ms->smp.cpus *4 -2 ; - cells =g_new0 (uint32_t, cells_length); + cells =g_new0 (uint32_t, ms->smp.cpus *4 -2 ); + cells_length =0 ;for (cpu =0 ; cpu < ms->smp.cpus; cpu++) {/* cpu 0 is the management hart that does not have S-mode */ -if (cpu ==0 ) { - cells[0 ] =cpu_to_be32 (intc_phandles[cpu]); - cells[1 ] =cpu_to_be32 (IRQ_M_EXT ); - }else { - cells[cpu *4 -2 ] =cpu_to_be32 (intc_phandles[cpu]); - cells[cpu *4 -1 ] =cpu_to_be32 (IRQ_M_EXT ); - cells[cpu *4 +0 ] =cpu_to_be32 (intc_phandles[cpu]); - cells[cpu *4 +1 ] =cpu_to_be32 (IRQ_S_EXT ); + hart = cpu ? &s->soc.u_cpus.harts[cpu -1 ] : &s->soc.e_cpus.harts[0 ]; + + cells[cells_length++] =cpu_to_be32 (intc_phandles[cpu]); + cells[cells_length++] =cpu_to_be32 (IRQ_M_EXT ); + +if (riscv_cpu_has_supervisor_mode (hart)) { + cells[cells_length++] =cpu_to_be32 (intc_phandles[cpu]); + cells[cells_length++] =cpu_to_be32 (IRQ_S_EXT ); } }
Signature · 1 line
2.54.0
Re: [PATCH v3 3/4] hw/riscv/sifive_u: derive PLIC contexts from the harts
Quoted 15 lines · Ralf Ramsauer
From: Ralf Ramsauer <[email protected]>
The PLIC device is created from
riscv_plic_hart_config_string(), which gives a hart a supervisor context only if that hart has supervisor mode. The device tree instead hardcodes the fu540 topology: no supervisor context for the management hart, one for every other hart.Both agree as long as the application harts keep the default CPU type. The board sets no
valid_cpu_types, though, and passesmachine->cpu_typeon to the U cluster, so -cpu rv64,s=false,u=false builds a PLIC without any supervisor context while the device tree still advertises one per application hart. Contexts are numbered by their position in 'interrupts-extended', so from the second hart on the numbering drifts apart, as it does on the virt machine.
Quoted 6 lines
Ask the harts instead of assuming, like
create_fdt_socket_plic()does. No change for the default CPU type: the E51 management hart has no RVS, the U54 application harts have.Signed-off-by Ralf Ramsauer <[email protected]>Reviewed-by Daniel Henrique Barboza <[email protected]>
Thanks,
Chao
Quoted 48 lines
--- hw/riscv/sifive_u.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-)hw/riscv/sifive_u.c +11
-10@@ -99,6 +99,7 @@ static void create_fdt(SiFiveUState *s, const MemMapEntry *memmap,MachineState *ms =MACHINE (s);void *fdt;int cpu; +RISCVCPU *hart;uint32_t *cells, cells_length;char *nodename;uint32_t plic_phandle, prci_phandle, gpio_phandle, phandle =1 ;@@ -195,18 +196,18 @@ static void create_fdt(SiFiveUState *s, const MemMapEntry *memmap,g_free (nodename); plic_phandle = phandle++; - cells_length = ms->smp.cpus *4 -2 ; - cells =g_new0 (uint32_t, cells_length); + cells =g_new0 (uint32_t, ms->smp.cpus *4 -2 ); + cells_length =0 ;for (cpu =0 ; cpu < ms->smp.cpus; cpu++) {/* cpu 0 is the management hart that does not have S-mode */ -if (cpu ==0 ) { - cells[0 ] =cpu_to_be32 (intc_phandles[cpu]); - cells[1 ] =cpu_to_be32 (IRQ_M_EXT ); - }else { - cells[cpu *4 -2 ] =cpu_to_be32 (intc_phandles[cpu]); - cells[cpu *4 -1 ] =cpu_to_be32 (IRQ_M_EXT ); - cells[cpu *4 +0 ] =cpu_to_be32 (intc_phandles[cpu]); - cells[cpu *4 +1 ] =cpu_to_be32 (IRQ_S_EXT ); + hart = cpu ? &s->soc.u_cpus.harts[cpu -1 ] : &s->soc.e_cpus.harts[0 ]; + + cells[cells_length++] =cpu_to_be32 (intc_phandles[cpu]); + cells[cells_length++] =cpu_to_be32 (IRQ_M_EXT ); + +if (riscv_cpu_has_supervisor_mode (hart)) { + cells[cells_length++] =cpu_to_be32 (intc_phandles[cpu]); + cells[cells_length++] =cpu_to_be32 (IRQ_S_EXT ); } }Signature · 1 line
2.54.0
Re: [PATCH v3 3/4] hw/riscv/sifive_u: derive PLIC contexts from the harts
In reply to Ralf RamsauerQuoted 27 lines · Ralf Ramsauer
From: Ralf Ramsauer <[email protected]>
The PLIC device is created from
riscv_plic_hart_config_string(), which
gives a hart a supervisor context only if that hart has supervisor mode.
The device tree instead hardcodes the fu540 topology: no supervisor context for the management hart, one for every other hart.Both agree as long as the application harts keep the default CPU type.
The board sets novalid_cpu_types, though, and passes machine-Quoted 1 line
cpu_typeon to the U cluster, so -cpu rv64,s=false,u=false builds a PLIC without
any supervisor context while the device tree still advertises one per application hart. Contexts are numbered by their position in 'interrupts-extended', so from the second hart on the numbering drifts
apart, as it does on the virt machine.Ask the harts instead of assuming, like
create_fdt_socket_plic()does.
No change for the default CPU type: the E51 management hart has no RVS,
the U54 application harts have.
Quoted 3 lines
Signed-off-by Ralf Ramsauer <[email protected]>Reviewed-by Daniel Henrique Barboza
Alistair
Quoted 48 lines
---
hw/riscv/sifive_u.c | 21 +++++++++++----------1 file changed, 11 insertions(+), 10 deletions(-)hw/riscv/sifive_u.c +1
-0@@ -99,6 +99,7 @@ static void create_fdt(SiFiveUState *s, constMemMapEntry *memmap, MachineState *ms = MACHINE(s); void *fdt; int cpu; + RISCVCPU *hart; uint32_t *cells, cells_length; char *nodename;
uint32_tplic_phandle,prci_phandle,gpio_phandle, phandle = 1;hw/riscv/sifive_u.c +0
-0@@ -195,18 +196,18 @@ static void create_fdt(SiFiveUState *s, constMemMapEntry *memmap, g_free(nodename);
plic_phandle= phandle++;
-cells_length=ms->smp.cpus* 4 - 2;
- cells =g_new0(uint32_t, cells_length);
+ cells =g_new0(uint32_t, ms->smp.cpus * 4 - 2);
+cells_length= 0;
for (cpu = 0; cpu <ms->smp.cpus; cpu++) {
/* cpu 0 is the management hart that does not have S-mode */ - if (cpu == 0) {
- cells[0] =cpu_to_be32(intc_phandles[cpu]);
- cells[1] =cpu_to_be32(IRQ_M_EXT);
- } else {
- cells[cpu * 4 - 2] =cpu_to_be32(intc_phandles[cpu]); - cells[cpu * 4 - 1] =cpu_to_be32(IRQ_M_EXT); - cells[cpu * 4 + 0] =cpu_to_be32(intc_phandles[cpu]); - cells[cpu * 4 + 1] =cpu_to_be32(IRQ_S_EXT); + hart = cpu ? &s->soc.u_cpus.harts[cpu - 1] : &s-Quoted 1 line
soc.e_cpus.harts[0];
hw/riscv/sifive_u.c +7
-0+ + cells[cells_length++] = cpu_to_be32(intc_phandles[cpu]); + cells[cells_length++] = cpu_to_be32(IRQ_M_EXT); + + if (riscv_cpu_has_supervisor_mode(hart)) { + cells[cells_length++] = cpu_to_be32(intc_phandles[cpu]); + cells[cells_length++] = cpu_to_be32(IRQ_S_EXT);}
}
PATCH v3 4/4 hw/riscv: share the PLIC interrupt cells between boards
In reply to Ralf RamsauerFrom: Ralf Ramsauer <[email protected]>
virt and sifive_u now describe a hart's PLIC contexts with the same rule:
a machine context unless the hart runs under KVM, where there is no M-mode, and a supervisor context if the hart has supervisor mode. Writing that rule down twice is what let the virt device tree drift away from riscv_plic_hart_config_string().
This makes the sifive_u device tree ask whether KVM is in use, which it did not do before. Its PLIC device already does, so the two now agree.
---
Cleanup, not a fix. Can be dropped, the rest does not depend on it.
hw/riscv/fdt-common.c | 18 ++++++++++++++++++ hw/riscv/sifive_u.c | 12 ++++-------- hw/riscv/virt.c | 16 ++++++---------- include/hw/riscv/fdt-common.h | 3 +++ 4 files changed, 31 insertions(+), 18 deletions(-)
hw/riscv/fdt-common.c +18 -0
@@ -212,6 +212,24 @@ riscv_create_fdt_socket_cpu_sifive(void *fdt, char *clust_name, phandle, intc_phandles, false, false); } +uint32_t riscv_plic_hart_fdt_cells (uint32_t *cells,RISCVCPU *hart, +uint32_t intc_phandle,bool has_m_mode) +{ +uint32_t len =0 ; + +if (has_m_mode) { + cells[len++] =cpu_to_be32 (intc_phandle); + cells[len++] =cpu_to_be32 (IRQ_M_EXT ); + } + +if (riscv_cpu_has_supervisor_mode (hart)) { + cells[len++] =cpu_to_be32 (intc_phandle); + cells[len++] =cpu_to_be32 (IRQ_S_EXT ); + } + +return len; +} + void riscv_create_fdt_plic(void *fdt, hwaddr addr, uint64_t size, uint32_t plic_phandle, uint32_t int_cells, uint32_t addr_cells, uint32_t *plic_cells,
hw/riscv/sifive_u.c +4 -8
@@ -58,6 +58,7 @@#include "chardev/char.h" #include "net/eth.h" #include "system/device_tree.h" +#include "system/kvm.h" #include "system/runstate.h" #include "system/system.h"
@@ -201,14 +202,9 @@ static void create_fdt(SiFiveUState *s, const MemMapEntry *memmap,for (cpu =0 ; cpu < ms->smp.cpus; cpu++) {/* cpu 0 is the management hart that does not have S-mode */ hart = cpu ? &s->soc.u_cpus.harts[cpu -1 ] : &s->soc.e_cpus.harts[0 ]; - - cells[cells_length++] =cpu_to_be32 (intc_phandles[cpu]); - cells[cells_length++] =cpu_to_be32 (IRQ_M_EXT ); - -if (riscv_cpu_has_supervisor_mode (hart)) { - cells[cells_length++] =cpu_to_be32 (intc_phandles[cpu]); - cells[cells_length++] =cpu_to_be32 (IRQ_S_EXT ); - } + cells_length +=riscv_plic_hart_fdt_cells (&cells[cells_length], hart, + intc_phandles[cpu], + !kvm_enabled ()); } riscv_create_fdt_plic(fdt, memmap[SIFIVE_U_DEV_PLIC].base,
hw/riscv/virt.c +6 -10
@@ -152,24 +152,20 @@ static void create_fdt_socket_plic(RISCVVirtState *s,unsigned long plic_addr = s->memmap[VIRT_PLIC ].base + (s->memmap[VIRT_PLIC ].size * socket);bool numa_enabled =riscv_numa_enabled (MACHINE (s)); +bool has_m_mode = !kvm_enabled ();uint32_t cells_length; plic_name =g_strdup_printf ("/soc/interrupt-controller@%lx" , plic_addr); plic_cells =g_new0 (uint32_t, - s->soc[socket].num_harts * (kvm_enabled () ?2 :4 )); + s->soc[socket].num_harts * (has_m_mode ?4 :2 )); cells_length =0 ;for (cpu =0 ; cpu < s->soc[socket].num_harts; cpu++) { -if (!kvm_enabled ()) { - plic_cells[cells_length++] =cpu_to_be32 (intc_phandles[cpu]); - plic_cells[cells_length++] =cpu_to_be32 (IRQ_M_EXT ); - } - -if (riscv_cpu_has_supervisor_mode (&s->soc[socket].harts[cpu])) { - plic_cells[cells_length++] =cpu_to_be32 (intc_phandles[cpu]); - plic_cells[cells_length++] =cpu_to_be32 (IRQ_S_EXT ); - } + cells_length +=riscv_plic_hart_fdt_cells (&plic_cells[cells_length], + &s->soc[socket].harts[cpu], + intc_phandles[cpu], + has_m_mode); } plic_phandles[socket] = (*phandle)++;
include/hw/riscv/fdt-common.h +3 -0
@@ -93,6 +93,9 @@ void riscv_create_fdt_socket_cpu_sifive(void *fdt, char *clust_name,int socket_hartid_base, uint32_t *phandle, uint32_t *intc_phandles); +/* Describe one hart's PLIC contexts, returns the number of cells written */ +uint32_t riscv_plic_hart_fdt_cells (uint32_t *cells,RISCVCPU *hart, +uint32_t intc_phandle,bool has_m_mode); void riscv_create_fdt_plic(void *fdt, hwaddr addr, uint64_t size, uint32_t plic_phandle, uint32_t int_cells, uint32_t addr_cells, uint32_t *plic_cells,
Signature · 1 line
2.54.0
Re: [PATCH v3 4/4] hw/riscv: share the PLIC interrupt cells between boards
Quoted 10 lines · Ralf Ramsauer
From: Ralf Ramsauer <[email protected]>
virt and
sifive_unow describe a hart's PLIC contexts with the same rule:
a machine context unless the hart runs under KVM, where there is no M-mode, and a supervisor context if the hart has supervisor mode. Writing that rule down twice is what let the virt device tree drift away
fromriscv_plic_hart_config_string().
Quoted 8 lines
This makes the
sifive_udevice tree ask whether KVM is in use, which it
did not do before. Its PLIC device already does, so the two now agree.Signed-off-by Ralf Ramsauer <[email protected]>Reviewed-by Daniel Henrique Barboza
Alistair
Quoted 142 lines
---
Cleanup, not a fix. Can be dropped, the rest does not depend on it.hw/riscv/fdt-common.c | 18 ++++++++++++++++++ hw/riscv/sifive_u.c | 12 ++++-------- hw/riscv/virt.c | 16 ++++++----------
include/hw/riscv/fdt-common.h| 3 +++4 files changed, 31 insertions(+), 18 deletions(-)hw/riscv/fdt-common.c +0
-0@@ -212,6 +212,24 @@ riscv_create_fdt_socket_cpu_sifive(void *fdt,char *clust_name, phandle, intc_phandles, false, false); }
+uint32_t riscv_plic_hart_fdt_cells(uint32_t *cells, RISCVCPU *hart, +uint32_tintc_phandle, boolhas_m_mode)hw/riscv/fdt-common.c +16
-0+{ + uint32_t len = 0; + + if (has_m_mode) { + cells[len++] = cpu_to_be32(intc_phandle); + cells[len++] = cpu_to_be32(IRQ_M_EXT); + } + + if (riscv_cpu_has_supervisor_mode(hart)) { + cells[len++] = cpu_to_be32(intc_phandle); + cells[len++] = cpu_to_be32(IRQ_S_EXT); + } + + return len; +} +void riscv_create_fdt_plic(void *fdt, hwaddr addr,
uint64_tsize,uint32_tplic_phandle,uint32_tint_cells,
uint32_taddr_cells,uint32_t*plic_cells,hw/riscv/sifive_u.c +1
-0@@ -58,6 +58,7 @@ #include "chardev/char.h" #include "net/eth.h" #include "system/device_tree.h" +#include "system/kvm.h" #include "system/runstate.h" #include "system/system.h"hw/riscv/sifive_u.c +0
-0@@ -201,14 +202,9 @@ static void create_fdt(SiFiveUState *s, const MemMapEntry *memmap, for (cpu = 0; cpu < ms->smp.cpus; cpu++) { /* cpu 0 is the management hart that does not have S-mode */hart = cpu ? &s->soc.u_cpus.harts[cpu - 1] : &s-
Quoted 1 line
soc.e_cpus.harts[0];
hw/riscv/sifive_u.c +1
-8- - cells[cells_length++] = cpu_to_be32(intc_phandles[cpu]); - cells[cells_length++] = cpu_to_be32(IRQ_M_EXT); - - if (riscv_cpu_has_supervisor_mode(hart)) { - cells[cells_length++] = cpu_to_be32(intc_phandles[cpu]); - cells[cells_length++] = cpu_to_be32(IRQ_S_EXT); - } + cells_length +=riscv_plic_hart_fdt_cells(&cells[cells_length], hart,
+
intc_phandles[cpu],
+ !kvm_enabled()); }
riscv_create_fdt_plic(fdt, memmap[SIFIVE_U_DEV_PLIC].base,hw/riscv/virt.c +0
-0@@ -152,24 +152,20 @@ static void create_fdt_socket_plic(RISCVVirtState *s,unsigned long
plic_addr= s->memmap[VIRT_PLIC].base + (s->memmap[VIRT_PLIC].size * socket); boolnuma_enabled= riscv_numa_enabled(MACHINE(s));
+ boolhas_m_mode= !kvm_enabled();
uint32_tcells_length;
plic_name=g_strdup_printf("/soc/interrupt-controller@%lx", plic_addr);
plic_cells= g_new0(uint32_t,
- s->soc[socket].num_harts * (kvm_enabled()? 2 : 4));
+ s->soc[socket].num_harts * (has_m_mode? 4 :
2));
cells_length= 0;
for (cpu = 0; cpu < s->soc[socket].num_harts; cpu++) { - if (!kvm_enabled()) {
- plic_cells[cells_length++] =cpu_to_be32(intc_phandles[cpu]);
- plic_cells[cells_length++] =cpu_to_be32(IRQ_M_EXT); - }
-
- if (riscv_cpu_has_supervisor_mode(&s-Quoted 1 line
soc[socket].harts[cpu])) {
- plic_cells[cells_length++] =
cpu_to_be32(intc_phandles[cpu]);
- plic_cells[cells_length++] =cpu_to_be32(IRQ_S_EXT); - }
+cells_length+= riscv_plic_hart_fdt_cells(&plic_cells[cells_length],
+ &s-Quoted 1 line
soc[socket].harts[cpu],
+
intc_phandles[cpu],
+has_m_mode); }
plic_phandles[socket] = (*phandle)++;include/hw/riscv/fdt- renamed from include/hw/riscv/fdt-common.h +0
-0
common.h
indexeb9db8278d..58e635e9ed100644include/hw/riscv/fdt-common.h +1
-0@@ -93,6 +93,9 @@ void riscv_create_fdt_socket_cpu_sifive(void *fdt,char *clust_name, int socket_hartid_base, uint32_t *phandle, uint32_t *intc_phandles); +/* Describe one hart's PLIC contexts, returns the number of cells written */
+uint32_t riscv_plic_hart_fdt_cells(uint32_t *cells, RISCVCPU *hart, +uint32_tintc_phandle, boolhas_m_mode);
void riscv_create_fdt_plic(void *fdt, hwaddr addr,uint64_tsize,uint32_tplic_phandle,uint32_tint_cells,
uint32_taddr_cells,uint32_t*plic_cells,
Alistair Francis
AppliedQuoted 57 lines · Ralf Ramsauer
A PLIC context is addressed by its position in the 'interrupts- extended'
property, so the device tree and the device have to agree on which harts
contribute a context and in which order. On the virt machine they do
not: the device is built fromriscv_plic_hart_config_string(), which gives a hart a supervisor context only if it has supervisor mode, while
the device tree lists a machine and a supervisor context for every hart,
unconditionally. Boot with-cpu rv64,h=false,s=false,u=false -smp 4
and the PLIC implements four contexts while the device tree advertises
eight. A guest that takes its context number from the property then gets
it wrong for every hart but the first. This is what an M-mode kernel on
virt runs into.sifive_uhardcodes the fu540 topology instead and has
the same latent problem, reachable with -cpu since the board restricts
neither the CPU type nor its extensions.1 to 3 are the fix, 4 is cleanup and can be dropped, the rest does not
depend on it.Tested by comparing the PLIC's 'interrupts-extended' against the hart-config string of the PLIC device over 14 configurations of both
boards: 7 disagree before the series, all agree after, and the device tree is unchanged wherever the default CPU is used. The KVM paths could
not be exercised, for lack of hardware.v3:
- 4/4: the fdt helper takeshas_m_modefrom the board rather than callingkvm_enabled()itself, keeping accelerator considerations in
the boards (Daniel)v2:
- askriscv_cpu_has_supervisor_mode()instead of reading RVS out of the
hart's CPURISCVState (Daniel)
- fold the KVM branch into the common loop, it describes the same rule
- fixsifive_uas well, and share the cell building between both boardsv1:
https://lore.kernel.org/qemu-devel/[email protected]/
v2:
https://lore.kernel.org/qemu-devel/[email protected]/
Quoted 5 lines
Ralf Ramsauer (4):
target/riscv: addriscv_cpu_has_supervisor_mode()hw/riscv/virt: only describe PLIC contexts that exist hw/riscv/sifive_u: derive PLIC contexts from the harts hw/riscv: share the PLIC interrupt cells between boards
Thanks!
Applied to riscv-to-apply.next
Alistair
Quoted 11 lines
hw/riscv/fdt-common.c | 18 ++++++++++++++++++ hw/riscv/sifive_u.c | 19 ++++++++----------- hw/riscv/virt.c | 26 +++++++++-----------------
include/hw/riscv/fdt-common.h| 3 +++ target/riscv/cpu.c | 14 ++++++++++++++ target/riscv/cpu.h | 1 +6 files changed, 53 insertions(+), 28 deletions(-)base-commit:
4f4225874d4d3747ec1142e935dbd314218992fa