Re: [PATCH v2 2/5] target/riscv: Disable "G" by default

[Tsukasa OI]

On 2022/05/17 3:04, Víctor Colombo wrote:
> On 14/05/2022 23:56, Tsukasa OI wrote:
>> Because "G" virtual extension expands to "IMAFD", we cannot separately
>> disable extensions like "F" or "D" without disabling "G".  Because all
>> "IMAFD" are enabled by default, it's harmless to disable "G" by default.
>>
>> Signed-off-by: Tsukasa OI <research_trasio@irq.a4lg.com>
>> ---
>>   target/riscv/cpu.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>> index 00bf26ec8b..3ea68d5cd7 100644
>> --- a/target/riscv/cpu.c
>> +++ b/target/riscv/cpu.c
>> @@ -812,7 +812,7 @@ static Property riscv_cpu_properties[] = {
>>       /* Defaults for standard extensions */
>>       DEFINE_PROP_BOOL("i", RISCVCPU, cfg.ext_i, true),
>>       DEFINE_PROP_BOOL("e", RISCVCPU, cfg.ext_e, false),
>> -    DEFINE_PROP_BOOL("g", RISCVCPU, cfg.ext_g, true),
>> +    DEFINE_PROP_BOOL("g", RISCVCPU, cfg.ext_g, false),
>>       DEFINE_PROP_BOOL("m", RISCVCPU, cfg.ext_m, true),
>>       DEFINE_PROP_BOOL("a", RISCVCPU, cfg.ext_a, true),
>>       DEFINE_PROP_BOOL("f", RISCVCPU, cfg.ext_f, true),
>> -- 
>> 2.34.1
>>
>>
> 
> I think the logic looks ok, and (with my limited understanding of the
> code) I agree on the reasoning for the changes in patches 2 and 3.
> Just some clarification needed: where is the value of 'g' checked?
> can the behavior change in this patch cause a situation where
> IMAFD_Zicsr_Zifencei is set but 'g' is not, or does patch 3
> guarantee that in this case 'g' will be set?
> 
> 
> Thanks!
> 

Probably too late to answer but on Alistair's riscv-to-apply.next tree...

target/riscv/cpu.c (19f13a9) line 599-611:
if (cpu->cfg.ext_g && !(cpu->cfg.ext_i && cpu->cfg.ext_m &&
                        cpu->cfg.ext_a && cpu->cfg.ext_f &&
                        cpu->cfg.ext_d &&
                        cpu->cfg.ext_icsr && cpu->cfg.ext_ifencei)) {
    warn_report("Setting G will also set IMAFD_Zicsr_Zifencei");
    cpu->cfg.ext_i = true;
    cpu->cfg.ext_m = true;
    cpu->cfg.ext_a = true;
    cpu->cfg.ext_f = true;
    cpu->cfg.ext_d = true;
    cpu->cfg.ext_icsr = true;
    cpu->cfg.ext_ifencei = true;
}

This is the only place where "G" (ext_g) is read.  Here, if G is enabled
and not all IMAFD_Zicsr_Zifencei are enabled, it enables them with a
warning message.

So, even if "G" is disabled alone, it's harmless.  Note that
IMAFD_Zicsr_Zifencei are enabled by default.

Thanks,
Tsukasa