Re: [Qemu-devel] [PATCH for 4.1 v3 2/6] target/riscv: Fall back to generating a RISC-V CPU

[Igor Mammedov <imammedo@redhat.com>]

On Wed, 10 Apr 2019 23:10:25 +0000
Alistair Francis <Alistair.Francis@wdc.com> wrote:

> If a user specifies a CPU that we don't understand then we want to fall
> back to a CPU generated from the ISA string.
It might look like a nice thing to do at the beginning, but
fallbacks become a source of pain in future and get in the way
of refactorings if there is a promise to maintain defaults (fallbacks)
stable.

I suggest do not fallback to anything, just fail cleanly
with informative error telling users what is wrong and let user
fix their invalid CLI in the first place.

 
> At the moment the generated CPU is assumed to be a privledge spec
> version 1.10 CPU with an MMU. This can be changed in the future.
> 
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
> v3:
>  - Ensure a minimal length so we don't run off the end of the string.
>  - Don't parse the rv32/rv64 in the loop
>  target/riscv/cpu.c | 101 ++++++++++++++++++++++++++++++++++++++++++++-
>  target/riscv/cpu.h |   2 +
>  2 files changed, 102 insertions(+), 1 deletion(-)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index d61bce6d55..27be9e412a 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -19,6 +19,7 @@
>  
>  #include "qemu/osdep.h"
>  #include "qemu/log.h"
> +#include "qemu/error-report.h"
>  #include "cpu.h"
>  #include "exec/exec-all.h"
>  #include "qapi/error.h"
> @@ -103,6 +104,99 @@ static void set_resetvec(CPURISCVState *env, int resetvec)
>  #endif
>  }
>  
> +static void riscv_generate_cpu_init(Object *obj)
> +{
> +    RISCVCPU *cpu = RISCV_CPU(obj);
> +    CPURISCVState *env = &cpu->env;
> +    RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(cpu);
> +    const char *riscv_cpu = mcc->isa_str;
> +    target_ulong target_misa = 0;
> +    target_ulong rvxlen = 0;
> +    int i;
> +    bool valid = false;
> +
> +    /*
> +     * We need at least 5 charecters for the string to be valid. Check that
> +     * now so we can be lazier later.
> +     */
> +    if (strlen(riscv_cpu) < 5) {
> +        error_report("'%s' does not appear to be a valid RISC-V ISA string",
> +                     riscv_cpu);
> +        exit(1);
> +    }
> +
> +    if (riscv_cpu[0] == 'r' && riscv_cpu[1] == 'v') {
> +        /* Starts with "rv" */
> +        if (riscv_cpu[2] == '3' && riscv_cpu[3] == '2') {
> +            valid = true;
> +            rvxlen = RV32;
> +        }
> +        if (riscv_cpu[2] == '6' && riscv_cpu[3] == '4') {
> +            valid = true;
> +            rvxlen = RV64;
> +        }
> +    }
> +
> +    if (!valid) {
> +        error_report("'%s' does not appear to be a valid RISC-V CPU",
> +                     riscv_cpu);
> +        exit(1);
> +    }
> +
> +    for (i = 4; i < strlen(riscv_cpu); i++) {
> +        switch (riscv_cpu[i]) {
> +        case 'i':
> +            if (target_misa & RVE) {
> +                error_report("I and E extensions are incompatible");
> +                exit(1);
> +            }
> +            target_misa |= RVI;
> +            continue;
> +        case 'e':
> +            if (target_misa & RVI) {
> +                error_report("I and E extensions are incompatible");
> +                exit(1);
> +            }
> +            target_misa |= RVE;
> +            continue;
> +        case 'g':
> +            target_misa |= RVI | RVM | RVA | RVF | RVD;
> +            continue;
> +        case 'm':
> +            target_misa |= RVM;
> +            continue;
> +        case 'a':
> +            target_misa |= RVA;
> +            continue;
> +        case 'f':
> +            target_misa |= RVF;
> +            continue;
> +        case 'd':
> +            target_misa |= RVD;
> +            continue;
> +        case 'c':
> +            target_misa |= RVC;
> +            continue;
> +        case 's':
> +            target_misa |= RVS;
> +            continue;
> +        case 'u':
> +            target_misa |= RVU;
> +            continue;
> +        default:
> +            warn_report("QEMU does not support the %c extension",
> +                        riscv_cpu[i]);
> +            continue;
> +        }
> +    }
> +
> +    set_misa(env, rvxlen | target_misa);
> +    set_versions(env, USER_VERSION_2_02_0, PRIV_VERSION_1_10_0);
> +    set_resetvec(env, DEFAULT_RSTVEC);
> +    set_feature(env, RISCV_FEATURE_MMU);
> +    set_feature(env, RISCV_FEATURE_PMP);
> +}
> +
>  static void riscv_any_cpu_init(Object *obj)
>  {
>      CPURISCVState *env = &RISCV_CPU(obj)->env;
> @@ -178,6 +272,7 @@ static void rv64imacu_nommu_cpu_init(Object *obj)
>  static ObjectClass *riscv_cpu_class_by_name(const char *cpu_model)
>  {
>      ObjectClass *oc;
> +    RISCVCPUClass *mcc;
>      char *typename;
>      char **cpuname;
>  
> @@ -188,7 +283,10 @@ static ObjectClass *riscv_cpu_class_by_name(const char *cpu_model)
>      g_free(typename);
>      if (!oc || !object_class_dynamic_cast(oc, TYPE_RISCV_CPU) ||
>          object_class_is_abstract(oc)) {
> -        return NULL;
> +        /* No CPU found, try the generic CPU and pass in the ISA string */
> +        oc = object_class_by_name(TYPE_RISCV_CPU_GEN);
> +        mcc = RISCV_CPU_CLASS(oc);
> +        mcc->isa_str = g_strdup(cpu_model);
>      }
>      return oc;
>  }
> @@ -440,6 +538,7 @@ static const TypeInfo riscv_cpu_type_infos[] = {
>          .class_init = riscv_cpu_class_init,
>      },
>      DEFINE_CPU(TYPE_RISCV_CPU_ANY,              riscv_any_cpu_init),
> +    DEFINE_CPU(TYPE_RISCV_CPU_GEN,              riscv_generate_cpu_init),
>  #if defined(TARGET_RISCV32)
>      DEFINE_CPU(TYPE_RISCV_CPU_RV32GCSU_V1_09_1, rv32gcsu_priv1_09_1_cpu_init),
>      DEFINE_CPU(TYPE_RISCV_CPU_RV32GCSU_V1_10_0, rv32gcsu_priv1_10_0_cpu_init),
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 20bce8742e..453108a855 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -48,6 +48,7 @@
>  #define CPU_RESOLVING_TYPE TYPE_RISCV_CPU
>  
>  #define TYPE_RISCV_CPU_ANY              RISCV_CPU_TYPE_NAME("any")
> +#define TYPE_RISCV_CPU_GEN              RISCV_CPU_TYPE_NAME("rv*")
>  #define TYPE_RISCV_CPU_RV32GCSU_V1_09_1 RISCV_CPU_TYPE_NAME("rv32gcsu-v1.9.1")
>  #define TYPE_RISCV_CPU_RV32GCSU_V1_10_0 RISCV_CPU_TYPE_NAME("rv32gcsu-v1.10.0")
>  #define TYPE_RISCV_CPU_RV32IMACU_NOMMU  RISCV_CPU_TYPE_NAME("rv32imacu-nommu")
> @@ -211,6 +212,7 @@ typedef struct RISCVCPUClass {
>      /*< public >*/
>      DeviceRealize parent_realize;
>      void (*parent_reset)(CPUState *cpu);
> +    const char *isa_str;
>  } RISCVCPUClass;
>  
>  /**