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

[Clément Léger]

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)
>