[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 2/5] target/riscv: Disable "G" by default
From: |
Tsukasa OI |
Subject: |
Re: [PATCH v2 2/5] target/riscv: Disable "G" by default |
Date: |
Tue, 24 May 2022 18:07:36 +0900 |
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
[PATCH v2 5/5] target/riscv: Move/refactor ISA extension checks, Tsukasa OI, 2022/05/14