| Message ID | cab7205f1d7668f642fa242386543334af6bc1bd.1652583332.git.research_trasio@irq.a4lg.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/5] target/riscv: Fix coding style on "G" expansion | expand |
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!
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
On 24/05/2022 06:07, Tsukasa OI wrote: > 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 Hello! Thank you very much for the clarification, it was helpful. Still getting used to the riscv code base in QEMU Best regards,
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),
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(-)