On Thu, Mar 30, 2023 at 6:11 AM Daniel Henrique Barboza
<dbarb...@ventanamicro.com> wrote:
>
> We're doing env->priv_spec validation and assignment at the start of
> riscv_cpu_realize(), which is fine, but then we're doing a force disable
> on extensions that aren't compatible with the priv version.
>
> This second step is being done too early. The disabled extensions might be
> re-enabled again in riscv_cpu_validate_set_extensions() by accident. A
> better place to put this code is at the end of
> riscv_cpu_validate_set_extensions() after all the validations are
> completed.
>
> Add a new helper, riscv_cpu_disable_priv_spec_isa_exts(), to disable the
> extesions after the validation is done. While we're at it, create a
> riscv_cpu_validate_priv_spec() helper to host all env->priv_spec related
> validation to unclog riscv_cpu_realize a bit.
>
> Signed-off-by: Daniel Henrique Barboza <dbarb...@ventanamicro.com>
> Reviewed-by: LIU Zhiwei <zhiwei_...@linux.alibaba.com>
> Reviewed-by: Weiwei Li <liwei...@iscas.ac.cn>
Reviewed-by: Alistair Francis <alistair.fran...@wdc.com>
Alistair
> ---
> target/riscv/cpu.c | 91 ++++++++++++++++++++++++++++------------------
> 1 file changed, 56 insertions(+), 35 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 1743e9ede3..8f0620376c 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -820,6 +820,52 @@ static void riscv_cpu_validate_v(CPURISCVState *env,
> RISCVCPUConfig *cfg,
> env->vext_ver = vext_version;
> }
>
> +static void riscv_cpu_validate_priv_spec(RISCVCPU *cpu, Error **errp)
> +{
> + CPURISCVState *env = &cpu->env;
> + int priv_version = -1;
> +
> + if (cpu->cfg.priv_spec) {
> + if (!g_strcmp0(cpu->cfg.priv_spec, "v1.12.0")) {
> + priv_version = PRIV_VERSION_1_12_0;
> + } else if (!g_strcmp0(cpu->cfg.priv_spec, "v1.11.0")) {
> + priv_version = PRIV_VERSION_1_11_0;
> + } else if (!g_strcmp0(cpu->cfg.priv_spec, "v1.10.0")) {
> + priv_version = PRIV_VERSION_1_10_0;
> + } else {
> + error_setg(errp,
> + "Unsupported privilege spec version '%s'",
> + cpu->cfg.priv_spec);
> + return;
> + }
> +
> + env->priv_ver = priv_version;
> + }
> +}
> +
> +static void riscv_cpu_disable_priv_spec_isa_exts(RISCVCPU *cpu)
> +{
> + CPURISCVState *env = &cpu->env;
> + int i;
> +
> + /* Force disable extensions if priv spec version does not match */
> + for (i = 0; i < ARRAY_SIZE(isa_edata_arr); i++) {
> + if (isa_ext_is_enabled(cpu, &isa_edata_arr[i]) &&
> + (env->priv_ver < isa_edata_arr[i].min_version)) {
> + isa_ext_update_enabled(cpu, &isa_edata_arr[i], false);
> +#ifndef CONFIG_USER_ONLY
> + warn_report("disabling %s extension for hart 0x" TARGET_FMT_lx
> + " because privilege spec version does not match",
> + isa_edata_arr[i].name, env->mhartid);
> +#else
> + warn_report("disabling %s extension because "
> + "privilege spec version does not match",
> + isa_edata_arr[i].name);
> +#endif
> + }
> + }
> +}
> +
> /*
> * Check consistency between chosen extensions while setting
> * cpu->cfg accordingly.
> @@ -984,6 +1030,12 @@ static void riscv_cpu_validate_set_extensions(RISCVCPU
> *cpu, Error **errp)
> cpu->cfg.ext_zksed = true;
> cpu->cfg.ext_zksh = true;
> }
> +
> + /*
> + * Disable isa extensions based on priv spec after we
> + * validated and set everything we need.
> + */
> + riscv_cpu_disable_priv_spec_isa_exts(cpu);
> }
>
> #ifndef CONFIG_USER_ONLY
> @@ -1083,7 +1135,6 @@ static void riscv_cpu_realize(DeviceState *dev, Error
> **errp)
> CPURISCVState *env = &cpu->env;
> RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(dev);
> CPUClass *cc = CPU_CLASS(mcc);
> - int i, priv_version = -1;
> Error *local_err = NULL;
>
> cpu_exec_realizefn(cs, &local_err);
> @@ -1092,23 +1143,10 @@ static void riscv_cpu_realize(DeviceState *dev, Error
> **errp)
> return;
> }
>
> - if (cpu->cfg.priv_spec) {
> - if (!g_strcmp0(cpu->cfg.priv_spec, "v1.12.0")) {
> - priv_version = PRIV_VERSION_1_12_0;
> - } else if (!g_strcmp0(cpu->cfg.priv_spec, "v1.11.0")) {
> - priv_version = PRIV_VERSION_1_11_0;
> - } else if (!g_strcmp0(cpu->cfg.priv_spec, "v1.10.0")) {
> - priv_version = PRIV_VERSION_1_10_0;
> - } else {
> - error_setg(errp,
> - "Unsupported privilege spec version '%s'",
> - cpu->cfg.priv_spec);
> - return;
> - }
> - }
> -
> - if (priv_version >= PRIV_VERSION_1_10_0) {
> - env->priv_ver = priv_version;
> + riscv_cpu_validate_priv_spec(cpu, &local_err);
> + if (local_err != NULL) {
> + error_propagate(errp, local_err);
> + return;
> }
>
> riscv_cpu_validate_misa_priv(env, &local_err);
> @@ -1117,23 +1155,6 @@ static void riscv_cpu_realize(DeviceState *dev, Error
> **errp)
> return;
> }
>
> - /* Force disable extensions if priv spec version does not match */
> - for (i = 0; i < ARRAY_SIZE(isa_edata_arr); i++) {
> - if (isa_ext_is_enabled(cpu, &isa_edata_arr[i]) &&
> - (env->priv_ver < isa_edata_arr[i].min_version)) {
> - isa_ext_update_enabled(cpu, &isa_edata_arr[i], false);
> -#ifndef CONFIG_USER_ONLY
> - warn_report("disabling %s extension for hart 0x" TARGET_FMT_lx
> - " because privilege spec version does not match",
> - isa_edata_arr[i].name, env->mhartid);
> -#else
> - warn_report("disabling %s extension because "
> - "privilege spec version does not match",
> - isa_edata_arr[i].name);
> -#endif
> - }
> - }
> -
> if (cpu->cfg.epmp && !cpu->cfg.pmp) {
> /*
> * Enhanced PMP should only be available
> --
> 2.39.2
>
>
