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

[Víctor Colombo]

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,

--
Víctor Cora Colombo
Instituto de Pesquisas ELDORADO
Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>