| Message ID | 20250116092352.1630278-1-cleger@rivosinc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | riscv: disable Smdbltrp for the max cpu | expand |
On 1/16/25 6:23 AM, Clément Léger wrote: > When present, Smdbltrp is enabled by default and MDT needs to be cleared > to avoid generating a double trap. Since not all firmwares are currently > ready to handle that, disable it for the max cpu. > > Reported-by: Atish Patra <atishp@rivosinc.com> > Signed-off-by: Clément Léger <cleger@rivosinc.com> > This breaks 'make check-functional' indeed. Not sure why we didn't notice it earlier. The change is fine but it should be made in the patch that introduced the error since it's not merged upstream yet. The patch is: [PATCH v8 9/9] target/riscv: Add Smdbltrp ISA extension enable switch Otherwise we'll have a gap of patches where 'make check-functional' won't work and it'll make our lives harder when bisecting stuff. This is the same review I gave Frank in the v10 of the 'smrnmi' series: https://lore.kernel.org/qemu-riscv/26ecf1ca-07eb-4aed-9d06-a12c036c0723@ventanamicro.com/ You can re-send "target/riscv: Add Smdbltrp ISA extension enable switch" as a v9 (I believe it's fine to send it standalone, no need to re-send the whole series) with this patch squashed in. Alternatively Alistair can squash in this change in his tree if he's up to it. Whatever works. But an extra patch is only justifiable if the change that broke stuff already made upstream and there's nothing we can do about it. This is not the case, and we should fix it properly while we can. Thanks, Daniel > --- > target/riscv/tcg/tcg-cpu.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c > index 48be24bbbe..0a137281de 100644 > --- a/target/riscv/tcg/tcg-cpu.c > +++ b/target/riscv/tcg/tcg-cpu.c > @@ -1439,6 +1439,16 @@ static void riscv_init_max_cpu_extensions(Object *obj) > isa_ext_update_enabled(cpu, CPU_CFG_OFFSET(ext_smrnmi), false); > qemu_log("Smrnmi is disabled in the 'max' type CPU\n"); > } > + > + /* > + * ext_smdbltrp requires the firmware to clear MSTATUS.MDT on startup to > + * avoid generating a double trap. OpenSBI does not currently support it, > + * disable it for now. > + */ > + if (cpu->cfg.ext_smdbltrp) { > + isa_ext_update_enabled(cpu, CPU_CFG_OFFSET(ext_smdbltrp), false); > + qemu_log("Smdbltrp is disabled in the 'max' type CPU\n"); > + } > } > > static bool riscv_cpu_has_max_extensions(Object *cpu_obj)
On 16/01/2025 13:15, Daniel Henrique Barboza wrote: > > > On 1/16/25 6:23 AM, Clément Léger wrote: >> When present, Smdbltrp is enabled by default and MDT needs to be cleared >> to avoid generating a double trap. Since not all firmwares are currently >> ready to handle that, disable it for the max cpu. >> >> Reported-by: Atish Patra <atishp@rivosinc.com> >> Signed-off-by: Clément Léger <cleger@rivosinc.com> >> > > This breaks 'make check-functional' indeed. Not sure why we didn't > notice it > earlier. > > The change is fine but it should be made in the patch that introduced > the error > since it's not merged upstream yet. The patch is: > > [PATCH v8 9/9] target/riscv: Add Smdbltrp ISA extension enable switch > > Otherwise we'll have a gap of patches where 'make check-functional' > won't work > and it'll make our lives harder when bisecting stuff. This is the same > review I > gave Frank in the v10 of the 'smrnmi' series: > > https://lore.kernel.org/qemu-riscv/26ecf1ca-07eb-4aed-9d06- > a12c036c0723@ventanamicro.com/ > > You can re-send "target/riscv: Add Smdbltrp ISA extension enable switch" > as a v9 (I > believe it's fine to send it standalone, no need to re-send the whole > series) with > this patch squashed in. Alternatively Alistair can squash in this change > in his tree > if he's up to it. Whatever works. I'll send a V9, thanks for the explanations. > > But an extra patch is only justifiable if the change that broke stuff > already made > upstream and there's nothing we can do about it. This is not the case, > and we should > fix it properly while we can. Yes, this perfectly makes sense, Thanks, Clément > > > Thanks, > > Daniel > > >> --- >> target/riscv/tcg/tcg-cpu.c | 10 ++++++++++ >> 1 file changed, 10 insertions(+) >> >> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c >> index 48be24bbbe..0a137281de 100644 >> --- a/target/riscv/tcg/tcg-cpu.c >> +++ b/target/riscv/tcg/tcg-cpu.c >> @@ -1439,6 +1439,16 @@ static void >> riscv_init_max_cpu_extensions(Object *obj) >> isa_ext_update_enabled(cpu, CPU_CFG_OFFSET(ext_smrnmi), false); >> qemu_log("Smrnmi is disabled in the 'max' type CPU\n"); >> } >> + >> + /* >> + * ext_smdbltrp requires the firmware to clear MSTATUS.MDT on >> startup to >> + * avoid generating a double trap. OpenSBI does not currently >> support it, >> + * disable it for now. >> + */ >> + if (cpu->cfg.ext_smdbltrp) { >> + isa_ext_update_enabled(cpu, CPU_CFG_OFFSET(ext_smdbltrp), >> false); >> + qemu_log("Smdbltrp is disabled in the 'max' type CPU\n"); >> + } >> } >> static bool riscv_cpu_has_max_extensions(Object *cpu_obj) >
diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c index 48be24bbbe..0a137281de 100644 --- a/target/riscv/tcg/tcg-cpu.c +++ b/target/riscv/tcg/tcg-cpu.c @@ -1439,6 +1439,16 @@ static void riscv_init_max_cpu_extensions(Object *obj) isa_ext_update_enabled(cpu, CPU_CFG_OFFSET(ext_smrnmi), false); qemu_log("Smrnmi is disabled in the 'max' type CPU\n"); } + + /* + * ext_smdbltrp requires the firmware to clear MSTATUS.MDT on startup to + * avoid generating a double trap. OpenSBI does not currently support it, + * disable it for now. + */ + if (cpu->cfg.ext_smdbltrp) { + isa_ext_update_enabled(cpu, CPU_CFG_OFFSET(ext_smdbltrp), false); + qemu_log("Smdbltrp is disabled in the 'max' type CPU\n"); + } } static bool riscv_cpu_has_max_extensions(Object *cpu_obj)
When present, Smdbltrp is enabled by default and MDT needs to be cleared to avoid generating a double trap. Since not all firmwares are currently ready to handle that, disable it for the max cpu. Reported-by: Atish Patra <atishp@rivosinc.com> Signed-off-by: Clément Léger <cleger@rivosinc.com> --- target/riscv/tcg/tcg-cpu.c | 10 ++++++++++ 1 file changed, 10 insertions(+)