Re: [PATCH] riscv: disable Smdbltrp for the max cpu

[Daniel Henrique Barboza]

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)