On 2023/3/19 4:04, Daniel Henrique Barboza 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.
It is not by accident. We should make sure if two extensions have
dependency, their privilege
requirements should have the related dependency. And we should write
this dependency to the isa_edata_arr.
I have sent the patch to the mail list to make this comment here clearer.
https://lists.gnu.org/archive/html/qemu-devel/2023-03/msg05361.html
Therefore it doesn't matter when do the priv_check or the
validate_set_extension. Both work for me.
Based on that patch, this patch looks good to me.
Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
Zhiwei
> 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 <dbarboza@ventanamicro.com>
> ---
> 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 1ee322001b..17b301967c 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -833,6 +833,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, doing a set_misa() in the end.
> @@ -1002,6 +1048,12 @@ static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
> 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);
> +
> if (cpu->cfg.ext_i) {
> ext |= RVI;
> }
> @@ -1131,7 +1183,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);
> @@ -1140,40 +1191,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;
> - }
> -
> - /* 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
> - }
> + riscv_cpu_validate_priv_spec(cpu, &local_err);
> + if (local_err != NULL) {
> + error_propagate(errp, local_err);
> + return;
> }
>
> if (cpu->cfg.epmp && !cpu->cfg.pmp) {