[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 2/2] target/riscv/cpu.c: do not skip misa logic in riscv_cpu_
|
From: |
Alistair Francis |
|
Subject: |
Re: [PATCH 2/2] target/riscv/cpu.c: do not skip misa logic in riscv_cpu_realize() |
|
Date: |
Wed, 11 Jan 2023 08:53:20 +1000 |
On Wed, Jan 11, 2023 at 6:17 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> All RISCV CPUs are setting cpu->cfg during their cpu_init() functions,
> meaning that there's no reason to skip all the misa validation and setup
> if misa_ext was set beforehand - especially since we're setting an
> updated value in set_misa() in the end.
>
> Put this code chunk into a new riscv_cpu_validate_set_extensions()
> helper and always execute it regardless of what the board set in
> env->misa_ext.
>
> This will put more responsibility in how each board is going to init
> their attributes and extensions if they're not using the defaults.
> It'll also allow realize() to do its job looking only at the extensions
> enabled per se, not corner cases that some CPUs might have, and we won't
> have to change multiple code paths to fix or change how extensions work.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Alistair
> ---
> target/riscv/cpu.c | 485 +++++++++++++++++++++++----------------------
> 1 file changed, 248 insertions(+), 237 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index b8c1edb7c2..33ed59a1b6 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -631,6 +631,250 @@ static void riscv_cpu_disas_set_info(CPUState *s,
> disassemble_info *info)
> }
> }
>
> +/*
> + * Check consistency between chosen extensions while setting
> + * cpu->cfg accordingly, doing a set_misa() in the end.
> + */
> +static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
> +{
> + CPURISCVState *env = &cpu->env;
> + uint32_t ext = 0;
> +
> + /* 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");
> + return;
> + }
> +
> + if (!cpu->cfg.ext_i && !cpu->cfg.ext_e) {
> + error_setg(errp,
> + "Either I or E extension must be set");
> + return;
> + }
> +
> + if (cpu->cfg.ext_s && !cpu->cfg.ext_u) {
> + error_setg(errp,
> + "Setting S extension without U extension is illegal");
> + return;
> + }
> +
> + if (cpu->cfg.ext_h && !cpu->cfg.ext_i) {
> + error_setg(errp,
> + "H depends on an I base integer ISA with 32 x registers");
> + return;
> + }
> +
> + if (cpu->cfg.ext_h && !cpu->cfg.ext_s) {
> + error_setg(errp, "H extension implicitly requires S-mode");
> + return;
> + }
> +
> + if (cpu->cfg.ext_f && !cpu->cfg.ext_icsr) {
> + error_setg(errp, "F extension requires Zicsr");
> + return;
> + }
> +
> + if ((cpu->cfg.ext_zawrs) && !cpu->cfg.ext_a) {
> + error_setg(errp, "Zawrs extension requires A extension");
> + return;
> + }
> +
> + if ((cpu->cfg.ext_zfh || cpu->cfg.ext_zfhmin) && !cpu->cfg.ext_f) {
> + error_setg(errp, "Zfh/Zfhmin extensions require F extension");
> + return;
> + }
> +
> + if (cpu->cfg.ext_d && !cpu->cfg.ext_f) {
> + error_setg(errp, "D extension requires F extension");
> + return;
> + }
> +
> + if (cpu->cfg.ext_v && !cpu->cfg.ext_d) {
> + error_setg(errp, "V extension requires D extension");
> + return;
> + }
> +
> + if ((cpu->cfg.ext_zve32f || cpu->cfg.ext_zve64f) && !cpu->cfg.ext_f) {
> + error_setg(errp, "Zve32f/Zve64f extensions require F extension");
> + return;
> + }
> +
> + /* Set the ISA extensions, checks should have happened above */
> + if (cpu->cfg.ext_zdinx || cpu->cfg.ext_zhinx ||
> + cpu->cfg.ext_zhinxmin) {
> + cpu->cfg.ext_zfinx = true;
> + }
> +
> + if (cpu->cfg.ext_zfinx) {
> + if (!cpu->cfg.ext_icsr) {
> + error_setg(errp, "Zfinx extension requires Zicsr");
> + return;
> + }
> + if (cpu->cfg.ext_f) {
> + error_setg(errp,
> + "Zfinx cannot be supported together with F extension");
> + return;
> + }
> + }
> +
> + if (cpu->cfg.ext_c) {
> + cpu->cfg.ext_zca = true;
> + if (cpu->cfg.ext_f && env->misa_mxl_max == MXL_RV32) {
> + cpu->cfg.ext_zcf = true;
> + }
> + if (cpu->cfg.ext_d) {
> + cpu->cfg.ext_zcd = true;
> + }
> + }
> +
> + if (env->misa_mxl_max != MXL_RV32 && cpu->cfg.ext_zcf) {
> + error_setg(errp, "Zcf extension is only relevant to RV32");
> + return;
> + }
> +
> + if (!cpu->cfg.ext_f && cpu->cfg.ext_zcf) {
> + error_setg(errp, "Zcf extension requires F extension");
> + return;
> + }
> +
> + if (!cpu->cfg.ext_d && cpu->cfg.ext_zcd) {
> + error_setg(errp, "Zcd extension requires D extension");
> + return;
> + }
> +
> + if ((cpu->cfg.ext_zcf || cpu->cfg.ext_zcd || cpu->cfg.ext_zcb ||
> + cpu->cfg.ext_zcmp || cpu->cfg.ext_zcmt) && !cpu->cfg.ext_zca) {
> + error_setg(errp, "Zcf/Zcd/Zcb/Zcmp/Zcmt extensions require Zca "
> + "extension");
> + return;
> + }
> +
> + if (cpu->cfg.ext_zcd && (cpu->cfg.ext_zcmp || cpu->cfg.ext_zcmt)) {
> + error_setg(errp, "Zcmp/Zcmt extensions are incompatible with "
> + "Zcd extension");
> + return;
> + }
> +
> + if (cpu->cfg.ext_zcmt && !cpu->cfg.ext_icsr) {
> + error_setg(errp, "Zcmt extension requires Zicsr extension");
> + return;
> + }
> +
> + if (cpu->cfg.ext_zk) {
> + cpu->cfg.ext_zkn = true;
> + cpu->cfg.ext_zkr = true;
> + cpu->cfg.ext_zkt = true;
> + }
> +
> + if (cpu->cfg.ext_zkn) {
> + cpu->cfg.ext_zbkb = true;
> + cpu->cfg.ext_zbkc = true;
> + cpu->cfg.ext_zbkx = true;
> + cpu->cfg.ext_zkne = true;
> + cpu->cfg.ext_zknd = true;
> + cpu->cfg.ext_zknh = true;
> + }
> +
> + if (cpu->cfg.ext_zks) {
> + cpu->cfg.ext_zbkb = true;
> + cpu->cfg.ext_zbkc = true;
> + cpu->cfg.ext_zbkx = true;
> + cpu->cfg.ext_zksed = true;
> + cpu->cfg.ext_zksh = true;
> + }
> +
> + if (cpu->cfg.ext_i) {
> + ext |= RVI;
> + }
> + if (cpu->cfg.ext_e) {
> + ext |= RVE;
> + }
> + if (cpu->cfg.ext_m) {
> + ext |= RVM;
> + }
> + if (cpu->cfg.ext_a) {
> + ext |= RVA;
> + }
> + if (cpu->cfg.ext_f) {
> + ext |= RVF;
> + }
> + if (cpu->cfg.ext_d) {
> + ext |= RVD;
> + }
> + if (cpu->cfg.ext_c) {
> + ext |= RVC;
> + }
> + if (cpu->cfg.ext_s) {
> + ext |= RVS;
> + }
> + if (cpu->cfg.ext_u) {
> + ext |= RVU;
> + }
> + if (cpu->cfg.ext_h) {
> + ext |= RVH;
> + }
> + if (cpu->cfg.ext_v) {
> + int vext_version = VEXT_VERSION_1_00_0;
> + ext |= RVV;
> + if (!is_power_of_2(cpu->cfg.vlen)) {
> + error_setg(errp,
> + "Vector extension VLEN must be power of 2");
> + return;
> + }
> + if (cpu->cfg.vlen > RV_VLEN_MAX || cpu->cfg.vlen < 128) {
> + error_setg(errp,
> + "Vector extension implementation only supports VLEN "
> + "in the range [128, %d]", RV_VLEN_MAX);
> + return;
> + }
> + if (!is_power_of_2(cpu->cfg.elen)) {
> + error_setg(errp,
> + "Vector extension ELEN must be power of 2");
> + return;
> + }
> + if (cpu->cfg.elen > 64 || cpu->cfg.elen < 8) {
> + error_setg(errp,
> + "Vector extension implementation only supports ELEN "
> + "in the range [8, 64]");
> + return;
> + }
> + if (cpu->cfg.vext_spec) {
> + if (!g_strcmp0(cpu->cfg.vext_spec, "v1.0")) {
> + vext_version = VEXT_VERSION_1_00_0;
> + } else {
> + error_setg(errp,
> + "Unsupported vector spec version '%s'",
> + cpu->cfg.vext_spec);
> + return;
> + }
> + } else {
> + qemu_log("vector version is not specified, "
> + "use the default value v1.0\n");
> + }
> + set_vext_version(env, vext_version);
> + }
> + if (cpu->cfg.ext_j) {
> + ext |= RVJ;
> + }
> +
> + set_misa(env, env->misa_mxl, ext);
> +}
> +
> static void riscv_cpu_realize(DeviceState *dev, Error **errp)
> {
> CPUState *cs = CPU(dev);
> @@ -726,243 +970,10 @@ static void riscv_cpu_realize(DeviceState *dev, Error
> **errp)
> }
> assert(env->misa_mxl_max == env->misa_mxl);
>
> - /* If only MISA_EXT is unset for misa, then set it from properties */
> - if (env->misa_ext == 0) {
> - uint32_t ext = 0;
> -
> - /* 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");
> - return;
> - }
> -
> - if (!cpu->cfg.ext_i && !cpu->cfg.ext_e) {
> - error_setg(errp,
> - "Either I or E extension must be set");
> - return;
> - }
> -
> - if (cpu->cfg.ext_s && !cpu->cfg.ext_u) {
> - error_setg(errp,
> - "Setting S extension without U extension is illegal");
> - return;
> - }
> -
> - if (cpu->cfg.ext_h && !cpu->cfg.ext_i) {
> - error_setg(errp,
> - "H depends on an I base integer ISA with 32 x
> registers");
> - return;
> - }
> -
> - if (cpu->cfg.ext_h && !cpu->cfg.ext_s) {
> - error_setg(errp, "H extension implicitly requires S-mode");
> - return;
> - }
> -
> - if (cpu->cfg.ext_f && !cpu->cfg.ext_icsr) {
> - error_setg(errp, "F extension requires Zicsr");
> - return;
> - }
> -
> - if ((cpu->cfg.ext_zawrs) && !cpu->cfg.ext_a) {
> - error_setg(errp, "Zawrs extension requires A extension");
> - return;
> - }
> -
> - if ((cpu->cfg.ext_zfh || cpu->cfg.ext_zfhmin) && !cpu->cfg.ext_f) {
> - error_setg(errp, "Zfh/Zfhmin extensions require F extension");
> - return;
> - }
> -
> - if (cpu->cfg.ext_d && !cpu->cfg.ext_f) {
> - error_setg(errp, "D extension requires F extension");
> - return;
> - }
> -
> - if (cpu->cfg.ext_v && !cpu->cfg.ext_d) {
> - error_setg(errp, "V extension requires D extension");
> - return;
> - }
> -
> - if ((cpu->cfg.ext_zve32f || cpu->cfg.ext_zve64f) && !cpu->cfg.ext_f)
> {
> - error_setg(errp, "Zve32f/Zve64f extensions require F extension");
> - return;
> - }
> -
> - /* Set the ISA extensions, checks should have happened above */
> - if (cpu->cfg.ext_zdinx || cpu->cfg.ext_zhinx ||
> - cpu->cfg.ext_zhinxmin) {
> - cpu->cfg.ext_zfinx = true;
> - }
> -
> - if (cpu->cfg.ext_zfinx) {
> - if (!cpu->cfg.ext_icsr) {
> - error_setg(errp, "Zfinx extension requires Zicsr");
> - return;
> - }
> - if (cpu->cfg.ext_f) {
> - error_setg(errp,
> - "Zfinx cannot be supported together with F extension");
> - return;
> - }
> - }
> -
> - if (cpu->cfg.ext_c) {
> - cpu->cfg.ext_zca = true;
> - if (cpu->cfg.ext_f && env->misa_mxl_max == MXL_RV32) {
> - cpu->cfg.ext_zcf = true;
> - }
> - if (cpu->cfg.ext_d) {
> - cpu->cfg.ext_zcd = true;
> - }
> - }
> -
> - if (env->misa_mxl_max != MXL_RV32 && cpu->cfg.ext_zcf) {
> - error_setg(errp, "Zcf extension is only relevant to RV32");
> - return;
> - }
> -
> - if (!cpu->cfg.ext_f && cpu->cfg.ext_zcf) {
> - error_setg(errp, "Zcf extension requires F extension");
> - return;
> - }
> -
> - if (!cpu->cfg.ext_d && cpu->cfg.ext_zcd) {
> - error_setg(errp, "Zcd extension requires D extension");
> - return;
> - }
> -
> - if ((cpu->cfg.ext_zcf || cpu->cfg.ext_zcd || cpu->cfg.ext_zcb ||
> - cpu->cfg.ext_zcmp || cpu->cfg.ext_zcmt) && !cpu->cfg.ext_zca) {
> - error_setg(errp, "Zcf/Zcd/Zcb/Zcmp/Zcmt extensions require Zca "
> - "extension");
> - return;
> - }
> -
> - if (cpu->cfg.ext_zcd && (cpu->cfg.ext_zcmp || cpu->cfg.ext_zcmt)) {
> - error_setg(errp, "Zcmp/Zcmt extensions are incompatible with "
> - "Zcd extension");
> - return;
> - }
> -
> - if (cpu->cfg.ext_zcmt && !cpu->cfg.ext_icsr) {
> - error_setg(errp, "Zcmt extension requires Zicsr extension");
> - return;
> - }
> -
> - if (cpu->cfg.ext_zk) {
> - cpu->cfg.ext_zkn = true;
> - cpu->cfg.ext_zkr = true;
> - cpu->cfg.ext_zkt = true;
> - }
> -
> - if (cpu->cfg.ext_zkn) {
> - cpu->cfg.ext_zbkb = true;
> - cpu->cfg.ext_zbkc = true;
> - cpu->cfg.ext_zbkx = true;
> - cpu->cfg.ext_zkne = true;
> - cpu->cfg.ext_zknd = true;
> - cpu->cfg.ext_zknh = true;
> - }
> -
> - if (cpu->cfg.ext_zks) {
> - cpu->cfg.ext_zbkb = true;
> - cpu->cfg.ext_zbkc = true;
> - cpu->cfg.ext_zbkx = true;
> - cpu->cfg.ext_zksed = true;
> - cpu->cfg.ext_zksh = true;
> - }
> -
> - if (cpu->cfg.ext_i) {
> - ext |= RVI;
> - }
> - if (cpu->cfg.ext_e) {
> - ext |= RVE;
> - }
> - if (cpu->cfg.ext_m) {
> - ext |= RVM;
> - }
> - if (cpu->cfg.ext_a) {
> - ext |= RVA;
> - }
> - if (cpu->cfg.ext_f) {
> - ext |= RVF;
> - }
> - if (cpu->cfg.ext_d) {
> - ext |= RVD;
> - }
> - if (cpu->cfg.ext_c) {
> - ext |= RVC;
> - }
> - if (cpu->cfg.ext_s) {
> - ext |= RVS;
> - }
> - if (cpu->cfg.ext_u) {
> - ext |= RVU;
> - }
> - if (cpu->cfg.ext_h) {
> - ext |= RVH;
> - }
> - if (cpu->cfg.ext_v) {
> - int vext_version = VEXT_VERSION_1_00_0;
> - ext |= RVV;
> - if (!is_power_of_2(cpu->cfg.vlen)) {
> - error_setg(errp,
> - "Vector extension VLEN must be power of 2");
> - return;
> - }
> - if (cpu->cfg.vlen > RV_VLEN_MAX || cpu->cfg.vlen < 128) {
> - error_setg(errp,
> - "Vector extension implementation only supports VLEN "
> - "in the range [128, %d]", RV_VLEN_MAX);
> - return;
> - }
> - if (!is_power_of_2(cpu->cfg.elen)) {
> - error_setg(errp,
> - "Vector extension ELEN must be power of 2");
> - return;
> - }
> - if (cpu->cfg.elen > 64 || cpu->cfg.elen < 8) {
> - error_setg(errp,
> - "Vector extension implementation only supports ELEN "
> - "in the range [8, 64]");
> - return;
> - }
> - if (cpu->cfg.vext_spec) {
> - if (!g_strcmp0(cpu->cfg.vext_spec, "v1.0")) {
> - vext_version = VEXT_VERSION_1_00_0;
> - } else {
> - error_setg(errp,
> - "Unsupported vector spec version '%s'",
> - cpu->cfg.vext_spec);
> - return;
> - }
> - } else {
> - qemu_log("vector version is not specified, "
> - "use the default value v1.0\n");
> - }
> - set_vext_version(env, vext_version);
> - }
> - if (cpu->cfg.ext_j) {
> - ext |= RVJ;
> - }
> -
> - set_misa(env, env->misa_mxl, ext);
> + riscv_cpu_validate_set_extensions(cpu, &local_err);
> + if (local_err != NULL) {
> + error_propagate(errp, local_err);
> + return;
> }
>
> #ifndef CONFIG_USER_ONLY
> --
> 2.39.0
>
>