| Message ID | 20230314164948.539135-17-dbarboza@ventanamicro.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/riscv: rework CPU extensions validation | expand |
On 2023/3/15 00:49, Daniel Henrique Barboza wrote: > We can set all RVG related extensions during realize time, before > validate_set_extensions() itself. It will also avoid re-enabling > RVG via write_misa() when the CSR start to using the same validation > code realize() does. > > Note that we're setting both cfg->ext_N and env->misa_ext bits, instead > of just setting cfg->ext_N. The intention here is to start syncing all > misa_ext operations with its cpu->cfg flags, in preparation to allow for > the validate function to operate using a misa_ext. This doesn't make any > difference for the current code state, but will be a requirement for > write_misa() later on. > > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> > --- > target/riscv/cpu.c | 55 +++++++++++++++++++++++++++++++++------------- > 1 file changed, 40 insertions(+), 15 deletions(-) > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c > index 48ad7372b9..133807e39f 100644 > --- a/target/riscv/cpu.c > +++ b/target/riscv/cpu.c > @@ -281,6 +281,42 @@ static uint32_t riscv_get_misa_ext_with_cpucfg(RISCVCPUConfig *cfg) > return ext; > } > > +static void riscv_set_G_virt_ext(RISCVCPU *cpu) > +{ > + CPURISCVState *env = &cpu->env; > + RISCVCPUConfig *cfg = &cpu->cfg; > + > + if (!(cfg->ext_i && cfg->ext_m && cfg->ext_a && > + cfg->ext_f && cfg->ext_d && > + cfg->ext_icsr && cfg->ext_ifencei)) { > + > + warn_report("Setting G will also set IMAFD_Zicsr_Zifencei"); > + cfg->ext_i = true; > + env->misa_ext |= RVI; > + > + cfg->ext_m = true; > + env->misa_ext |= RVM; > + > + cfg->ext_a = true; > + env->misa_ext |= RVA; > + > + cfg->ext_f = true; > + env->misa_ext |= RVF; > + > + cfg->ext_d = true; > + env->misa_ext |= RVD; > + > + cfg->ext_icsr = true; > + cfg->ext_ifencei = true; > + > + /* > + * Update misa_ext_mask since this is called > + * only during riscv_cpu_realize(). > + */ > + env->misa_ext_mask = env->misa_ext; > + } Another two question: - whether we should set 'G' when all these extensions are supported? - whether 'G'should be disabled if some of the extensions are disabled by write_misa? Regards, Weiwei Li > +} > + > static void riscv_set_cpucfg_with_misa(RISCVCPUConfig *cfg, > uint32_t misa_ext) > { > @@ -1036,21 +1072,6 @@ static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp) > return; > } > > - /* Do some ISA extension error checking */ > - 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; > - } > - > if (cpu->cfg.ext_i && cpu->cfg.ext_e) { > error_setg(errp, > "I and E extensions are incompatible"); > @@ -1313,6 +1334,10 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp) > return; > } > > + if (cpu->cfg.ext_g) { > + riscv_set_G_virt_ext(cpu); > + } > + > riscv_cpu_validate_set_extensions(cpu, &local_err); > if (local_err != NULL) { > error_propagate(errp, local_err);
On 3/15/23 01:43, liweiwei wrote: > > On 2023/3/15 00:49, Daniel Henrique Barboza wrote: >> We can set all RVG related extensions during realize time, before >> validate_set_extensions() itself. It will also avoid re-enabling >> RVG via write_misa() when the CSR start to using the same validation >> code realize() does. >> >> Note that we're setting both cfg->ext_N and env->misa_ext bits, instead >> of just setting cfg->ext_N. The intention here is to start syncing all >> misa_ext operations with its cpu->cfg flags, in preparation to allow for >> the validate function to operate using a misa_ext. This doesn't make any >> difference for the current code state, but will be a requirement for >> write_misa() later on. >> >> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> >> --- >> target/riscv/cpu.c | 55 +++++++++++++++++++++++++++++++++------------- >> 1 file changed, 40 insertions(+), 15 deletions(-) >> >> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c >> index 48ad7372b9..133807e39f 100644 >> --- a/target/riscv/cpu.c >> +++ b/target/riscv/cpu.c >> @@ -281,6 +281,42 @@ static uint32_t riscv_get_misa_ext_with_cpucfg(RISCVCPUConfig *cfg) >> return ext; >> } >> +static void riscv_set_G_virt_ext(RISCVCPU *cpu) >> +{ >> + CPURISCVState *env = &cpu->env; >> + RISCVCPUConfig *cfg = &cpu->cfg; >> + >> + if (!(cfg->ext_i && cfg->ext_m && cfg->ext_a && >> + cfg->ext_f && cfg->ext_d && >> + cfg->ext_icsr && cfg->ext_ifencei)) { >> + >> + warn_report("Setting G will also set IMAFD_Zicsr_Zifencei"); >> + cfg->ext_i = true; >> + env->misa_ext |= RVI; >> + >> + cfg->ext_m = true; >> + env->misa_ext |= RVM; >> + >> + cfg->ext_a = true; >> + env->misa_ext |= RVA; >> + >> + cfg->ext_f = true; >> + env->misa_ext |= RVF; >> + >> + cfg->ext_d = true; >> + env->misa_ext |= RVD; >> + >> + cfg->ext_icsr = true; >> + cfg->ext_ifencei = true; >> + >> + /* >> + * Update misa_ext_mask since this is called >> + * only during riscv_cpu_realize(). >> + */ >> + env->misa_ext_mask = env->misa_ext; >> + } > > Another two question: > > - whether we should set 'G' when all these extensions are supported? As far as I can tell, no. RVG is being treated as a shortcut to enabled this set of extensions, but it doesn't mean that if the user happens to chose them manually we should enable RVG. > > - whether 'G'should be disabled if some of the extensions are disabled by write_misa? Good point. Yes, we would need to disable RVG if RVG is enabled in the hart but then another letter extension to the group (I,M,A F or D) is disabled. Which is something that isn't being handled now. A simple solution is, in patch 15, forbid IMAFD to be disabled if RVG is already set. In fact, this kind of logic is something that we would need to do for the future profile extension, so in a way RVG is being handled almost like a profile now. Thanks, Daniel > > Regards, > > Weiwei Li > >> +} >> + >> static void riscv_set_cpucfg_with_misa(RISCVCPUConfig *cfg, >> uint32_t misa_ext) >> { >> @@ -1036,21 +1072,6 @@ static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp) >> return; >> } >> - /* Do some ISA extension error checking */ >> - 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; >> - } >> - >> if (cpu->cfg.ext_i && cpu->cfg.ext_e) { >> error_setg(errp, >> "I and E extensions are incompatible"); >> @@ -1313,6 +1334,10 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp) >> return; >> } >> + if (cpu->cfg.ext_g) { >> + riscv_set_G_virt_ext(cpu); >> + } >> + >> riscv_cpu_validate_set_extensions(cpu, &local_err); >> if (local_err != NULL) { >> error_propagate(errp, local_err); >
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c index 48ad7372b9..133807e39f 100644 --- a/target/riscv/cpu.c +++ b/target/riscv/cpu.c @@ -281,6 +281,42 @@ static uint32_t riscv_get_misa_ext_with_cpucfg(RISCVCPUConfig *cfg) return ext; } +static void riscv_set_G_virt_ext(RISCVCPU *cpu) +{ + CPURISCVState *env = &cpu->env; + RISCVCPUConfig *cfg = &cpu->cfg; + + if (!(cfg->ext_i && cfg->ext_m && cfg->ext_a && + cfg->ext_f && cfg->ext_d && + cfg->ext_icsr && cfg->ext_ifencei)) { + + warn_report("Setting G will also set IMAFD_Zicsr_Zifencei"); + cfg->ext_i = true; + env->misa_ext |= RVI; + + cfg->ext_m = true; + env->misa_ext |= RVM; + + cfg->ext_a = true; + env->misa_ext |= RVA; + + cfg->ext_f = true; + env->misa_ext |= RVF; + + cfg->ext_d = true; + env->misa_ext |= RVD; + + cfg->ext_icsr = true; + cfg->ext_ifencei = true; + + /* + * Update misa_ext_mask since this is called + * only during riscv_cpu_realize(). + */ + env->misa_ext_mask = env->misa_ext; + } +} + static void riscv_set_cpucfg_with_misa(RISCVCPUConfig *cfg, uint32_t misa_ext) { @@ -1036,21 +1072,6 @@ static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp) return; } - /* Do some ISA extension error checking */ - 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; - } - if (cpu->cfg.ext_i && cpu->cfg.ext_e) { error_setg(errp, "I and E extensions are incompatible"); @@ -1313,6 +1334,10 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp) return; } + if (cpu->cfg.ext_g) { + riscv_set_G_virt_ext(cpu); + } + riscv_cpu_validate_set_extensions(cpu, &local_err); if (local_err != NULL) { error_propagate(errp, local_err);
We can set all RVG related extensions during realize time, before validate_set_extensions() itself. It will also avoid re-enabling RVG via write_misa() when the CSR start to using the same validation code realize() does. Note that we're setting both cfg->ext_N and env->misa_ext bits, instead of just setting cfg->ext_N. The intention here is to start syncing all misa_ext operations with its cpu->cfg flags, in preparation to allow for the validate function to operate using a misa_ext. This doesn't make any difference for the current code state, but will be a requirement for write_misa() later on. Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> --- target/riscv/cpu.c | 55 +++++++++++++++++++++++++++++++++------------- 1 file changed, 40 insertions(+), 15 deletions(-)