[Qemu-devel] [PATCH for 4.1 v3 0/6] RISC-V: Allow specifying CPU ISA via command line

[Qemu-devel] [PATCH for 4.1 v3 0/6] RISC-V: Allow specifying CPU ISA via command line

[Alistair Francis]

This patch series adds a generic RISC-V CPU that can be generated at run
time based on the ISA string specified to QEMU via the -cpu argument. This
is supported on the virt and spike boards allowing users to specify the

RISC-V extensions as well as the ISA version.
As part of the conversion we have deprecated the version specifi Spike
machines.

v3:
 - Ensure a minimal length so we don't run off the end of the string.
 - Don't parse the rv32/rv64 in the riscv_generate_cpu_init() loop
v2:
 - Keep the any CPU for linux-user

Alistair Francis (6):
  linux-user/riscv: Add the CPU type as a comment
  target/riscv: Fall back to generating a RISC-V CPU
  target/riscv: Create settable CPU properties
  riscv: virt: Allow specifying a CPU via commandline
  target/riscv: Remove the generic no MMU CPUs
  riscv: Add a generic spike machine

 hw/riscv/spike.c              | 106 +++++++++++++++++++++++-
 hw/riscv/virt.c               |   3 +-
 linux-user/riscv/target_elf.h |   1 +
 target/riscv/cpu.c            | 147 +++++++++++++++++++++++++++++++++-
 target/riscv/cpu.h            |  12 ++-
 5 files changed, 262 insertions(+), 7 deletions(-)

-- 
2.21.0

[Qemu-devel] [PATCH for 4.1 v3 0/6] RISC-V: Allow specifying CPU ISA via command line

[Alistair Francis]

This patch series adds a generic RISC-V CPU that can be generated at run
time based on the ISA string specified to QEMU via the -cpu argument. This
is supported on the virt and spike boards allowing users to specify the

RISC-V extensions as well as the ISA version.
As part of the conversion we have deprecated the version specifi Spike
machines.

v3:
 - Ensure a minimal length so we don't run off the end of the string.
 - Don't parse the rv32/rv64 in the riscv_generate_cpu_init() loop
v2:
 - Keep the any CPU for linux-user

Alistair Francis (6):
  linux-user/riscv: Add the CPU type as a comment
  target/riscv: Fall back to generating a RISC-V CPU
  target/riscv: Create settable CPU properties
  riscv: virt: Allow specifying a CPU via commandline
  target/riscv: Remove the generic no MMU CPUs
  riscv: Add a generic spike machine

 hw/riscv/spike.c              | 106 +++++++++++++++++++++++-
 hw/riscv/virt.c               |   3 +-
 linux-user/riscv/target_elf.h |   1 +
 target/riscv/cpu.c            | 147 +++++++++++++++++++++++++++++++++-
 target/riscv/cpu.h            |  12 ++-
 5 files changed, 262 insertions(+), 7 deletions(-)

-- 
2.21.0

[Qemu-devel] [PATCH for 4.1 v3 1/6] linux-user/riscv: Add the CPU type as a comment

[Alistair Francis]

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 linux-user/riscv/target_elf.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/linux-user/riscv/target_elf.h b/linux-user/riscv/target_elf.h
index a6716a6aac..9dd65652ee 100644
--- a/linux-user/riscv/target_elf.h
+++ b/linux-user/riscv/target_elf.h
@@ -9,6 +9,7 @@
 #define RISCV_TARGET_ELF_H
 static inline const char *cpu_get_model(uint32_t eflags)
 {
+    /* TYPE_RISCV_CPU_ANY */
     return "any";
 }
 #endif
-- 
2.21.0

[Qemu-devel] [PATCH for 4.1 v3 1/6] linux-user/riscv: Add the CPU type as a comment

[Alistair Francis]

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 linux-user/riscv/target_elf.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/linux-user/riscv/target_elf.h b/linux-user/riscv/target_elf.h
index a6716a6aac..9dd65652ee 100644
--- a/linux-user/riscv/target_elf.h
+++ b/linux-user/riscv/target_elf.h
@@ -9,6 +9,7 @@
 #define RISCV_TARGET_ELF_H
 static inline const char *cpu_get_model(uint32_t eflags)
 {
+    /* TYPE_RISCV_CPU_ANY */
     return "any";
 }
 #endif
-- 
2.21.0

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

[Alistair Francis]

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.

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;
 
 /**
-- 
2.21.0

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

[Alistair Francis]

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.

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;
 
 /**
-- 
2.21.0

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

[Igor Mammedov]

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;
>  
>  /**

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

[Igor Mammedov]

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;
>  
>  /**

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

[Alistair Francis]

On Thu, Apr 11, 2019 at 5:18 AM Igor Mammedov <imammedo@redhat.com> wrote:
>
> 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.

Maybe fall back isn't the right word then, as this is the goal of this
series. Here is what I want to happen:

User runs QEMU RISC-V without -cpu option:
    The user gets a CPU that supports "standard" ISA extensions, as
what happens now

User runs QEMU RISC-V with a CPU model that is hard coded in cpu.h
(such as TYPE_RISCV_CPU_SIFIVE_U54):
    The user gets a CPU that matches that hard coded CPU string/init

User runs QEMU RISC-V with a RISC-V ISA string that isn't hardcoded
(for example rv64gcsuh):
    QEMU will dynamically create a CPU based on the specified bit
length (rv32/rv64) and the ISA extensions supported
    The goal of this is to allow new extensions to be enabled and
disabled as required. For example when we add a new extension maybe we
don't want it on by default but we want an option to enable it. Or
maybe a user wants to test their code on a model without compressed
instruction (c extension) support.

Alistair

>
>
> > 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;
> >
> >  /**
>

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

[Alistair Francis]

On Thu, Apr 11, 2019 at 5:18 AM Igor Mammedov <imammedo@redhat.com> wrote:
>
> 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.

Maybe fall back isn't the right word then, as this is the goal of this
series. Here is what I want to happen:

User runs QEMU RISC-V without -cpu option:
    The user gets a CPU that supports "standard" ISA extensions, as
what happens now

User runs QEMU RISC-V with a CPU model that is hard coded in cpu.h
(such as TYPE_RISCV_CPU_SIFIVE_U54):
    The user gets a CPU that matches that hard coded CPU string/init

User runs QEMU RISC-V with a RISC-V ISA string that isn't hardcoded
(for example rv64gcsuh):
    QEMU will dynamically create a CPU based on the specified bit
length (rv32/rv64) and the ISA extensions supported
    The goal of this is to allow new extensions to be enabled and
disabled as required. For example when we add a new extension maybe we
don't want it on by default but we want an option to enable it. Or
maybe a user wants to test their code on a model without compressed
instruction (c extension) support.

Alistair

>
>
> > 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;
> >
> >  /**
>

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

[Igor Mammedov]

On Thu, 11 Apr 2019 13:42:20 -0700
Alistair Francis <alistair23@gmail.com> wrote:

> On Thu, Apr 11, 2019 at 5:18 AM Igor Mammedov <imammedo@redhat.com> wrote:
> >
> > 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.  
> 
> Maybe fall back isn't the right word then, as this is the goal of this
> series. Here is what I want to happen:
> 
> User runs QEMU RISC-V without -cpu option:
>     The user gets a CPU that supports "standard" ISA extensions, as
> what happens now
> 
> User runs QEMU RISC-V with a CPU model that is hard coded in cpu.h
> (such as TYPE_RISCV_CPU_SIFIVE_U54):
>     The user gets a CPU that matches that hard coded CPU string/init
> 
> User runs QEMU RISC-V with a RISC-V ISA string that isn't hardcoded
> (for example rv64gcsuh):
>     QEMU will dynamically create a CPU based on the specified bit
> length (rv32/rv64) and the ISA extensions supported
>     The goal of this is to allow new extensions to be enabled and
> disabled as required. For example when we add a new extension maybe we
> don't want it on by default but we want an option to enable it. Or
> maybe a user wants to test their code on a model without compressed
> instruction (c extension) support.

From commit message I"g read it as fallback, so as far as user gets
deterministic (always the same) CPU according to what they asked on CLI
it's fine. In that case commit message probably should be rephrased.

more bellow
> Alistair
> 
> >
> >  
> > > 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 it's programming error it should be assert
but in general instance_init()  should never fail.

the same applies to errors or warnings within this function.

> > > +    }
> > > +
> > > +    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]);
that's what looks to me like fallback, and what makes
CPU features for this CPU type non deterministic.
I'm not sure what you are trying to achieve here, but it look like
you are trying to verify MachineClass field in object constructor
along with other things.

I'd put all MachineClass checks in class_init and abort on error
since invalid mcc->isa_str is codding error.
If you can make some checks compile time it would be even better.


> > > +            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;
> > >
> > >  /**  
> >  

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

[Igor Mammedov]

On Thu, 11 Apr 2019 13:42:20 -0700
Alistair Francis <alistair23@gmail.com> wrote:

> On Thu, Apr 11, 2019 at 5:18 AM Igor Mammedov <imammedo@redhat.com> wrote:
> >
> > 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.  
> 
> Maybe fall back isn't the right word then, as this is the goal of this
> series. Here is what I want to happen:
> 
> User runs QEMU RISC-V without -cpu option:
>     The user gets a CPU that supports "standard" ISA extensions, as
> what happens now
> 
> User runs QEMU RISC-V with a CPU model that is hard coded in cpu.h
> (such as TYPE_RISCV_CPU_SIFIVE_U54):
>     The user gets a CPU that matches that hard coded CPU string/init
> 
> User runs QEMU RISC-V with a RISC-V ISA string that isn't hardcoded
> (for example rv64gcsuh):
>     QEMU will dynamically create a CPU based on the specified bit
> length (rv32/rv64) and the ISA extensions supported
>     The goal of this is to allow new extensions to be enabled and
> disabled as required. For example when we add a new extension maybe we
> don't want it on by default but we want an option to enable it. Or
> maybe a user wants to test their code on a model without compressed
> instruction (c extension) support.

From commit message I"g read it as fallback, so as far as user gets
deterministic (always the same) CPU according to what they asked on CLI
it's fine. In that case commit message probably should be rephrased.

more bellow
> Alistair
> 
> >
> >  
> > > 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 it's programming error it should be assert
but in general instance_init()  should never fail.

the same applies to errors or warnings within this function.

> > > +    }
> > > +
> > > +    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]);
that's what looks to me like fallback, and what makes
CPU features for this CPU type non deterministic.
I'm not sure what you are trying to achieve here, but it look like
you are trying to verify MachineClass field in object constructor
along with other things.

I'd put all MachineClass checks in class_init and abort on error
since invalid mcc->isa_str is codding error.
If you can make some checks compile time it would be even better.


> > > +            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;
> > >
> > >  /**  
> >  

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

[Alistair Francis]

On Fri, Apr 12, 2019 at 1:36 AM Igor Mammedov <imammedo@redhat.com> wrote:
>
> On Thu, 11 Apr 2019 13:42:20 -0700
> Alistair Francis <alistair23@gmail.com> wrote:
>
> > On Thu, Apr 11, 2019 at 5:18 AM Igor Mammedov <imammedo@redhat.com> wrote:
> > >
> > > 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.
> >
> > Maybe fall back isn't the right word then, as this is the goal of this
> > series. Here is what I want to happen:
> >
> > User runs QEMU RISC-V without -cpu option:
> >     The user gets a CPU that supports "standard" ISA extensions, as
> > what happens now
> >
> > User runs QEMU RISC-V with a CPU model that is hard coded in cpu.h
> > (such as TYPE_RISCV_CPU_SIFIVE_U54):
> >     The user gets a CPU that matches that hard coded CPU string/init
> >
> > User runs QEMU RISC-V with a RISC-V ISA string that isn't hardcoded
> > (for example rv64gcsuh):
> >     QEMU will dynamically create a CPU based on the specified bit
> > length (rv32/rv64) and the ISA extensions supported
> >     The goal of this is to allow new extensions to be enabled and
> > disabled as required. For example when we add a new extension maybe we
> > don't want it on by default but we want an option to enable it. Or
> > maybe a user wants to test their code on a model without compressed
> > instruction (c extension) support.
>
> From commit message I"g read it as fallback, so as far as user gets
> deterministic (always the same) CPU according to what they asked on CLI
> it's fine. In that case commit message probably should be rephrased.
>
> more bellow
> > Alistair
> >
> > >
> > >
> > > > 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 it's programming error it should be assert
> but in general instance_init()  should never fail.

We are checking for a user specified CPU string, not a programming
error so an assert() isn't correct here.

If *init() can't fail how should this be handled?

>
> the same applies to errors or warnings within this function.
>
> > > > +    }
> > > > +
> > > > +    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]);
> that's what looks to me like fallback, and what makes
> CPU features for this CPU type non deterministic.

What do you mean? QEMU will always parse the specified CPU string and
create a CPU based on the features specified by the user. For each
version of QEMU it will always result in the same outcome for the same
user supplied command line argument.

Newer QEMU versions will support more features though, so they will
accept different options.

> I'm not sure what you are trying to achieve here, but it look like
> you are trying to verify MachineClass field in object constructor
> along with other things.

I'm just trying to parse the -cpu string option.

>
> I'd put all MachineClass checks in class_init and abort on error
> since invalid mcc->isa_str is codding error.

No, it's a user error.

Alistair

> If you can make some checks compile time it would be even better.
>
>
> > > > +            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;
> > > >
> > > >  /**
> > >
>

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

[Alistair Francis]

On Fri, Apr 12, 2019 at 1:36 AM Igor Mammedov <imammedo@redhat.com> wrote:
>
> On Thu, 11 Apr 2019 13:42:20 -0700
> Alistair Francis <alistair23@gmail.com> wrote:
>
> > On Thu, Apr 11, 2019 at 5:18 AM Igor Mammedov <imammedo@redhat.com> wrote:
> > >
> > > 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.
> >
> > Maybe fall back isn't the right word then, as this is the goal of this
> > series. Here is what I want to happen:
> >
> > User runs QEMU RISC-V without -cpu option:
> >     The user gets a CPU that supports "standard" ISA extensions, as
> > what happens now
> >
> > User runs QEMU RISC-V with a CPU model that is hard coded in cpu.h
> > (such as TYPE_RISCV_CPU_SIFIVE_U54):
> >     The user gets a CPU that matches that hard coded CPU string/init
> >
> > User runs QEMU RISC-V with a RISC-V ISA string that isn't hardcoded
> > (for example rv64gcsuh):
> >     QEMU will dynamically create a CPU based on the specified bit
> > length (rv32/rv64) and the ISA extensions supported
> >     The goal of this is to allow new extensions to be enabled and
> > disabled as required. For example when we add a new extension maybe we
> > don't want it on by default but we want an option to enable it. Or
> > maybe a user wants to test their code on a model without compressed
> > instruction (c extension) support.
>
> From commit message I"g read it as fallback, so as far as user gets
> deterministic (always the same) CPU according to what they asked on CLI
> it's fine. In that case commit message probably should be rephrased.
>
> more bellow
> > Alistair
> >
> > >
> > >
> > > > 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 it's programming error it should be assert
> but in general instance_init()  should never fail.

We are checking for a user specified CPU string, not a programming
error so an assert() isn't correct here.

If *init() can't fail how should this be handled?

>
> the same applies to errors or warnings within this function.
>
> > > > +    }
> > > > +
> > > > +    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]);
> that's what looks to me like fallback, and what makes
> CPU features for this CPU type non deterministic.

What do you mean? QEMU will always parse the specified CPU string and
create a CPU based on the features specified by the user. For each
version of QEMU it will always result in the same outcome for the same
user supplied command line argument.

Newer QEMU versions will support more features though, so they will
accept different options.

> I'm not sure what you are trying to achieve here, but it look like
> you are trying to verify MachineClass field in object constructor
> along with other things.

I'm just trying to parse the -cpu string option.

>
> I'd put all MachineClass checks in class_init and abort on error
> since invalid mcc->isa_str is codding error.

No, it's a user error.

Alistair

> If you can make some checks compile time it would be even better.
>
>
> > > > +            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;
> > > >
> > > >  /**
> > >
>

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

[Igor Mammedov]

On Fri, 12 Apr 2019 14:19:16 -0700
Alistair Francis <alistair23@gmail.com> wrote:

> On Fri, Apr 12, 2019 at 1:36 AM Igor Mammedov <imammedo@redhat.com> wrote:
> >
> > On Thu, 11 Apr 2019 13:42:20 -0700
> > Alistair Francis <alistair23@gmail.com> wrote:
> >
> > > On Thu, Apr 11, 2019 at 5:18 AM Igor Mammedov <imammedo@redhat.com> wrote:
> > > >
> > > > 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.
> > >
> > > Maybe fall back isn't the right word then, as this is the goal of this
> > > series. Here is what I want to happen:
> > >
> > > User runs QEMU RISC-V without -cpu option:
> > >     The user gets a CPU that supports "standard" ISA extensions, as
> > > what happens now
> > >
> > > User runs QEMU RISC-V with a CPU model that is hard coded in cpu.h
> > > (such as TYPE_RISCV_CPU_SIFIVE_U54):
> > >     The user gets a CPU that matches that hard coded CPU string/init
> > >
> > > User runs QEMU RISC-V with a RISC-V ISA string that isn't hardcoded
> > > (for example rv64gcsuh):
> > >     QEMU will dynamically create a CPU based on the specified bit
> > > length (rv32/rv64) and the ISA extensions supported
> > >     The goal of this is to allow new extensions to be enabled and
> > > disabled as required. For example when we add a new extension maybe we
> > > don't want it on by default but we want an option to enable it. Or
> > > maybe a user wants to test their code on a model without compressed
> > > instruction (c extension) support.
> >
> > From commit message I"g read it as fallback, so as far as user gets
> > deterministic (always the same) CPU according to what they asked on CLI
> > it's fine. In that case commit message probably should be rephrased.
> >
> > more bellow
> > > Alistair
> > >
> > > >
> > > >
> > > > > 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 it's programming error it should be assert
> > but in general instance_init()  should never fail.
> 
> We are checking for a user specified CPU string, not a programming
> error so an assert() isn't correct here.
> 
> If *init() can't fail how should this be handled?
typical flow for device object is

 object_new(foo) -> foo_init_fn()
 set properties on foo
 set 'realize' property to 'true' -> foo_realize_fn()

all checks should be performed in realize() or during property setting.

> > the same applies to errors or warnings within this function.
> >
> > > > > +    }
> > > > > +
> > > > > +    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]);
> > that's what looks to me like fallback, and what makes
> > CPU features for this CPU type non deterministic.
> 
> What do you mean? QEMU will always parse the specified CPU string and
> create a CPU based on the features specified by the user. For each
> version of QEMU it will always result in the same outcome for the same
> user supplied command line argument.

I've meant that here you would print warning and happily continue parsing
user input ignoring input error. One should error out instead and make
user fix error.

> 
> Newer QEMU versions will support more features though, so they will
> accept different options.
> 
> > I'm not sure what you are trying to achieve here, but it look like
> > you are trying to verify MachineClass field in object constructor
> > along with other things.
> 
> I'm just trying to parse the -cpu string option.
> 
> >
> > I'd put all MachineClass checks in class_init and abort on error
> > since invalid mcc->isa_str is codding error.
> 
> No, it's a user error.

Parsing -cpu by assigning to class members user input looks unusual.

We had a custom parsing for x86 & sparc cpus, but that was factored out
into compat utilities and we shouldn't do it anywhere else without very
compelling reason.

Currently it's preferred to use canonical form for cpu properties, like:

  -cpu foo,prop1=x,prop2=y,...

and let the generic parser to do the job. For it to work you would need
to create a property per a cpu feature.

This way it will work fine with -cpu and -device/device_add without any
custom parsing involved.

> Alistair
> 
> > If you can make some checks compile time it would be even better.
> >
> >
> > > > > +            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;
> > > > >
> > > > >  /**
> > > >
> >

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

[Igor Mammedov]

On Fri, 12 Apr 2019 14:19:16 -0700
Alistair Francis <alistair23@gmail.com> wrote:

> On Fri, Apr 12, 2019 at 1:36 AM Igor Mammedov <imammedo@redhat.com> wrote:
> >
> > On Thu, 11 Apr 2019 13:42:20 -0700
> > Alistair Francis <alistair23@gmail.com> wrote:
> >
> > > On Thu, Apr 11, 2019 at 5:18 AM Igor Mammedov <imammedo@redhat.com> wrote:
> > > >
> > > > 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.
> > >
> > > Maybe fall back isn't the right word then, as this is the goal of this
> > > series. Here is what I want to happen:
> > >
> > > User runs QEMU RISC-V without -cpu option:
> > >     The user gets a CPU that supports "standard" ISA extensions, as
> > > what happens now
> > >
> > > User runs QEMU RISC-V with a CPU model that is hard coded in cpu.h
> > > (such as TYPE_RISCV_CPU_SIFIVE_U54):
> > >     The user gets a CPU that matches that hard coded CPU string/init
> > >
> > > User runs QEMU RISC-V with a RISC-V ISA string that isn't hardcoded
> > > (for example rv64gcsuh):
> > >     QEMU will dynamically create a CPU based on the specified bit
> > > length (rv32/rv64) and the ISA extensions supported
> > >     The goal of this is to allow new extensions to be enabled and
> > > disabled as required. For example when we add a new extension maybe we
> > > don't want it on by default but we want an option to enable it. Or
> > > maybe a user wants to test their code on a model without compressed
> > > instruction (c extension) support.
> >
> > From commit message I"g read it as fallback, so as far as user gets
> > deterministic (always the same) CPU according to what they asked on CLI
> > it's fine. In that case commit message probably should be rephrased.
> >
> > more bellow
> > > Alistair
> > >
> > > >
> > > >
> > > > > 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 it's programming error it should be assert
> > but in general instance_init()  should never fail.
> 
> We are checking for a user specified CPU string, not a programming
> error so an assert() isn't correct here.
> 
> If *init() can't fail how should this be handled?
typical flow for device object is

 object_new(foo) -> foo_init_fn()
 set properties on foo
 set 'realize' property to 'true' -> foo_realize_fn()

all checks should be performed in realize() or during property setting.

> > the same applies to errors or warnings within this function.
> >
> > > > > +    }
> > > > > +
> > > > > +    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]);
> > that's what looks to me like fallback, and what makes
> > CPU features for this CPU type non deterministic.
> 
> What do you mean? QEMU will always parse the specified CPU string and
> create a CPU based on the features specified by the user. For each
> version of QEMU it will always result in the same outcome for the same
> user supplied command line argument.

I've meant that here you would print warning and happily continue parsing
user input ignoring input error. One should error out instead and make
user fix error.

> 
> Newer QEMU versions will support more features though, so they will
> accept different options.
> 
> > I'm not sure what you are trying to achieve here, but it look like
> > you are trying to verify MachineClass field in object constructor
> > along with other things.
> 
> I'm just trying to parse the -cpu string option.
> 
> >
> > I'd put all MachineClass checks in class_init and abort on error
> > since invalid mcc->isa_str is codding error.
> 
> No, it's a user error.

Parsing -cpu by assigning to class members user input looks unusual.

We had a custom parsing for x86 & sparc cpus, but that was factored out
into compat utilities and we shouldn't do it anywhere else without very
compelling reason.

Currently it's preferred to use canonical form for cpu properties, like:

  -cpu foo,prop1=x,prop2=y,...

and let the generic parser to do the job. For it to work you would need
to create a property per a cpu feature.

This way it will work fine with -cpu and -device/device_add without any
custom parsing involved.

> Alistair
> 
> > If you can make some checks compile time it would be even better.
> >
> >
> > > > > +            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;
> > > > >
> > > > >  /**
> > > >
> >

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

[Alistair Francis]

On Mon, Apr 15, 2019 at 1:38 AM Igor Mammedov <imammedo@redhat.com> wrote:
>
> On Fri, 12 Apr 2019 14:19:16 -0700
> Alistair Francis <alistair23@gmail.com> wrote:
>

...

> > >
> > > if it's programming error it should be assert
> > > but in general instance_init()  should never fail.
> >
> > We are checking for a user specified CPU string, not a programming
> > error so an assert() isn't correct here.
> >
> > If *init() can't fail how should this be handled?
> typical flow for device object is
>
>  object_new(foo) -> foo_init_fn()
>  set properties on foo
>  set 'realize' property to 'true' -> foo_realize_fn()
>
> all checks should be performed in realize() or during property setting.

I thought there was a reason realise didn't work, but now I can't see
what it is. I can move it to realise.

>
> > > the same applies to errors or warnings within this function.
> > >
> > > > > > +    }
> > > > > > +
> > > > > > +    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]);
> > > that's what looks to me like fallback, and what makes
> > > CPU features for this CPU type non deterministic.
> >
> > What do you mean? QEMU will always parse the specified CPU string and
> > create a CPU based on the features specified by the user. For each
> > version of QEMU it will always result in the same outcome for the same
> > user supplied command line argument.
>
> I've meant that here you would print warning and happily continue parsing
> user input ignoring input error. One should error out instead and make
> user fix error.

Ah, ok. I can change this one to an error.

>
> >
> > Newer QEMU versions will support more features though, so they will
> > accept different options.
> >
> > > I'm not sure what you are trying to achieve here, but it look like
> > > you are trying to verify MachineClass field in object constructor
> > > along with other things.
> >
> > I'm just trying to parse the -cpu string option.
> >
> > >
> > > I'd put all MachineClass checks in class_init and abort on error
> > > since invalid mcc->isa_str is codding error.
> >
> > No, it's a user error.
>
> Parsing -cpu by assigning to class members user input looks unusual.
>
> We had a custom parsing for x86 & sparc cpus, but that was factored out
> into compat utilities and we shouldn't do it anywhere else without very
> compelling reason.

I did see that, but it didn't seem to work the same way as this.

>
> Currently it's preferred to use canonical form for cpu properties, like:
>
>   -cpu foo,prop1=x,prop2=y,...

Yeah, but I don't see how that would work nicely.

Instead of:
    -cpu rv64gcsu
we would have:
    -cpu rvgen,xlen=64,g=true,c=true,s=true,u=true
which seems more complex for users

>
> and let the generic parser to do the job. For it to work you would need
> to create a property per a cpu feature.
>
> This way it will work fine with -cpu and -device/device_add without any
> custom parsing involved.

Although I think it's messy for users, it is the cleanest
implementation in terms of QOM. Maybe that will be the only solution.

Alistair
...

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

[Alistair Francis]

On Mon, Apr 15, 2019 at 1:38 AM Igor Mammedov <imammedo@redhat.com> wrote:
>
> On Fri, 12 Apr 2019 14:19:16 -0700
> Alistair Francis <alistair23@gmail.com> wrote:
>

...

> > >
> > > if it's programming error it should be assert
> > > but in general instance_init()  should never fail.
> >
> > We are checking for a user specified CPU string, not a programming
> > error so an assert() isn't correct here.
> >
> > If *init() can't fail how should this be handled?
> typical flow for device object is
>
>  object_new(foo) -> foo_init_fn()
>  set properties on foo
>  set 'realize' property to 'true' -> foo_realize_fn()
>
> all checks should be performed in realize() or during property setting.

I thought there was a reason realise didn't work, but now I can't see
what it is. I can move it to realise.

>
> > > the same applies to errors or warnings within this function.
> > >
> > > > > > +    }
> > > > > > +
> > > > > > +    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]);
> > > that's what looks to me like fallback, and what makes
> > > CPU features for this CPU type non deterministic.
> >
> > What do you mean? QEMU will always parse the specified CPU string and
> > create a CPU based on the features specified by the user. For each
> > version of QEMU it will always result in the same outcome for the same
> > user supplied command line argument.
>
> I've meant that here you would print warning and happily continue parsing
> user input ignoring input error. One should error out instead and make
> user fix error.

Ah, ok. I can change this one to an error.

>
> >
> > Newer QEMU versions will support more features though, so they will
> > accept different options.
> >
> > > I'm not sure what you are trying to achieve here, but it look like
> > > you are trying to verify MachineClass field in object constructor
> > > along with other things.
> >
> > I'm just trying to parse the -cpu string option.
> >
> > >
> > > I'd put all MachineClass checks in class_init and abort on error
> > > since invalid mcc->isa_str is codding error.
> >
> > No, it's a user error.
>
> Parsing -cpu by assigning to class members user input looks unusual.
>
> We had a custom parsing for x86 & sparc cpus, but that was factored out
> into compat utilities and we shouldn't do it anywhere else without very
> compelling reason.

I did see that, but it didn't seem to work the same way as this.

>
> Currently it's preferred to use canonical form for cpu properties, like:
>
>   -cpu foo,prop1=x,prop2=y,...

Yeah, but I don't see how that would work nicely.

Instead of:
    -cpu rv64gcsu
we would have:
    -cpu rvgen,xlen=64,g=true,c=true,s=true,u=true
which seems more complex for users

>
> and let the generic parser to do the job. For it to work you would need
> to create a property per a cpu feature.
>
> This way it will work fine with -cpu and -device/device_add without any
> custom parsing involved.

Although I think it's messy for users, it is the cleanest
implementation in terms of QOM. Maybe that will be the only solution.

Alistair
...

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

[Igor Mammedov]

On Mon, 15 Apr 2019 16:56:08 -0700
Alistair Francis <alistair23@gmail.com> wrote:

> On Mon, Apr 15, 2019 at 1:38 AM Igor Mammedov <imammedo@redhat.com> wrote:
> >
> > On Fri, 12 Apr 2019 14:19:16 -0700
> > Alistair Francis <alistair23@gmail.com> wrote:
> >  
> 
> ...
> 
> > > >
> > > > if it's programming error it should be assert
> > > > but in general instance_init()  should never fail.  
> > >
> > > We are checking for a user specified CPU string, not a programming
> > > error so an assert() isn't correct here.
> > >
> > > If *init() can't fail how should this be handled?  
> > typical flow for device object is
> >
> >  object_new(foo) -> foo_init_fn()
> >  set properties on foo
> >  set 'realize' property to 'true' -> foo_realize_fn()
> >
> > all checks should be performed in realize() or during property setting.  
> 
> I thought there was a reason realise didn't work, but now I can't see
> what it is. I can move it to realise.
> 
> >  
> > > > the same applies to errors or warnings within this function.
> > > >  
> > > > > > > +    }
> > > > > > > +
> > > > > > > +    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]);  
> > > > that's what looks to me like fallback, and what makes
> > > > CPU features for this CPU type non deterministic.  
> > >
> > > What do you mean? QEMU will always parse the specified CPU string and
> > > create a CPU based on the features specified by the user. For each
> > > version of QEMU it will always result in the same outcome for the same
> > > user supplied command line argument.  
> >
> > I've meant that here you would print warning and happily continue parsing
> > user input ignoring input error. One should error out instead and make
> > user fix error.  
> 
> Ah, ok. I can change this one to an error.
> 
> >  
> > >
> > > Newer QEMU versions will support more features though, so they will
> > > accept different options.
> > >  
> > > > I'm not sure what you are trying to achieve here, but it look like
> > > > you are trying to verify MachineClass field in object constructor
> > > > along with other things.  
> > >
> > > I'm just trying to parse the -cpu string option.
> > >  
> > > >
> > > > I'd put all MachineClass checks in class_init and abort on error
> > > > since invalid mcc->isa_str is codding error.  
> > >
> > > No, it's a user error.  
> >
> > Parsing -cpu by assigning to class members user input looks unusual.
> >
> > We had a custom parsing for x86 & sparc cpus, but that was factored out
> > into compat utilities and we shouldn't do it anywhere else without very
> > compelling reason.  
> 
> I did see that, but it didn't seem to work the same way as this.
> 
> >
> > Currently it's preferred to use canonical form for cpu properties, like:
> >
> >   -cpu foo,prop1=x,prop2=y,...  
> 
> Yeah, but I don't see how that would work nicely.
> 
> Instead of:
>     -cpu rv64gcsu
> we would have:
>     -cpu rvgen,xlen=64,g=true,c=true,s=true,u=true
> which seems more complex for users
yes, it's clunky and verbose but it's uniform for all cpus,
which is doubly important for machine users and other interface QEMU
has to communicate with users.

secondly, adding set of symbols to cpu name (ideally it should be cpu's typename)
complicates parsing and adds one more user to the old compat hooks, which
prevents us from deprecating those.
We've put a lot of of effort in consolidating these parts across QEMU,
so adding yet another custom way to parse -cpu looks like regression to me.


> > and let the generic parser to do the job. For it to work you would need
> > to create a property per a cpu feature.
> >
> > This way it will work fine with -cpu and -device/device_add without any
> > custom parsing involved.  
> 
> Although I think it's messy for users, it is the cleanest
> implementation in terms of QOM. Maybe that will be the only solution.
I'd go for this one as it's less controversial and follows common patterns.
Human users can write a wrapper script if necessary.

> 
> Alistair
> ...

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

[Igor Mammedov]

On Mon, 15 Apr 2019 16:56:08 -0700
Alistair Francis <alistair23@gmail.com> wrote:

> On Mon, Apr 15, 2019 at 1:38 AM Igor Mammedov <imammedo@redhat.com> wrote:
> >
> > On Fri, 12 Apr 2019 14:19:16 -0700
> > Alistair Francis <alistair23@gmail.com> wrote:
> >  
> 
> ...
> 
> > > >
> > > > if it's programming error it should be assert
> > > > but in general instance_init()  should never fail.  
> > >
> > > We are checking for a user specified CPU string, not a programming
> > > error so an assert() isn't correct here.
> > >
> > > If *init() can't fail how should this be handled?  
> > typical flow for device object is
> >
> >  object_new(foo) -> foo_init_fn()
> >  set properties on foo
> >  set 'realize' property to 'true' -> foo_realize_fn()
> >
> > all checks should be performed in realize() or during property setting.  
> 
> I thought there was a reason realise didn't work, but now I can't see
> what it is. I can move it to realise.
> 
> >  
> > > > the same applies to errors or warnings within this function.
> > > >  
> > > > > > > +    }
> > > > > > > +
> > > > > > > +    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]);  
> > > > that's what looks to me like fallback, and what makes
> > > > CPU features for this CPU type non deterministic.  
> > >
> > > What do you mean? QEMU will always parse the specified CPU string and
> > > create a CPU based on the features specified by the user. For each
> > > version of QEMU it will always result in the same outcome for the same
> > > user supplied command line argument.  
> >
> > I've meant that here you would print warning and happily continue parsing
> > user input ignoring input error. One should error out instead and make
> > user fix error.  
> 
> Ah, ok. I can change this one to an error.
> 
> >  
> > >
> > > Newer QEMU versions will support more features though, so they will
> > > accept different options.
> > >  
> > > > I'm not sure what you are trying to achieve here, but it look like
> > > > you are trying to verify MachineClass field in object constructor
> > > > along with other things.  
> > >
> > > I'm just trying to parse the -cpu string option.
> > >  
> > > >
> > > > I'd put all MachineClass checks in class_init and abort on error
> > > > since invalid mcc->isa_str is codding error.  
> > >
> > > No, it's a user error.  
> >
> > Parsing -cpu by assigning to class members user input looks unusual.
> >
> > We had a custom parsing for x86 & sparc cpus, but that was factored out
> > into compat utilities and we shouldn't do it anywhere else without very
> > compelling reason.  
> 
> I did see that, but it didn't seem to work the same way as this.
> 
> >
> > Currently it's preferred to use canonical form for cpu properties, like:
> >
> >   -cpu foo,prop1=x,prop2=y,...  
> 
> Yeah, but I don't see how that would work nicely.
> 
> Instead of:
>     -cpu rv64gcsu
> we would have:
>     -cpu rvgen,xlen=64,g=true,c=true,s=true,u=true
> which seems more complex for users
yes, it's clunky and verbose but it's uniform for all cpus,
which is doubly important for machine users and other interface QEMU
has to communicate with users.

secondly, adding set of symbols to cpu name (ideally it should be cpu's typename)
complicates parsing and adds one more user to the old compat hooks, which
prevents us from deprecating those.
We've put a lot of of effort in consolidating these parts across QEMU,
so adding yet another custom way to parse -cpu looks like regression to me.


> > and let the generic parser to do the job. For it to work you would need
> > to create a property per a cpu feature.
> >
> > This way it will work fine with -cpu and -device/device_add without any
> > custom parsing involved.  
> 
> Although I think it's messy for users, it is the cleanest
> implementation in terms of QOM. Maybe that will be the only solution.
I'd go for this one as it's less controversial and follows common patterns.
Human users can write a wrapper script if necessary.

> 
> Alistair
> ...

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

[Daniel P. Berrangé]

On Wed, Apr 10, 2019 at 11:10:25PM +0000, Alistair Francis 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.
> 
> 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);
> +}

This whole approach feels undesirable to me, as it is quite different to
way CPUs are represented in the other architectures in QEMU and as a result
does not fit in the QAPI commands we've been building in QEMU for dealing
with CPU model representation. This will make for increased maint burden
in both QEMU and apps managing QEMU

IIUC, this code is taking an arbitrary CPU model string and looking
at individual characters in that string & turning on individual features
according to what characters it sees. There's several problems with this

 - There's no way to enumerate valid CPU model names

 - There can be many different names that all result
   in the same CPU model. eg "fdcs", "scdf", a"ffddccss"
   all result in the same features getting enabled
   by this loop above.

 - There's no way to check compatibility between CPUs

 - There will be no way to version the CPUs if we need
   to tie them to machine types for back compatibility.

If we have a base CPU model, with a bunch of features that can optionally
be enabled, then IMHO we should represent that the same way was we do
on x86, whre we have a CPU model name, and then a comma sepaprated list
of features.

If on the other hand we don't want to support the combinatorial matrix
of all possible feature flags, then we should just list all the expected
named CPU models that are required explicitly.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Daniel P. Berrangé]

On Wed, Apr 10, 2019 at 11:10:25PM +0000, Alistair Francis 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.
> 
> 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);
> +}

This whole approach feels undesirable to me, as it is quite different to
way CPUs are represented in the other architectures in QEMU and as a result
does not fit in the QAPI commands we've been building in QEMU for dealing
with CPU model representation. This will make for increased maint burden
in both QEMU and apps managing QEMU

IIUC, this code is taking an arbitrary CPU model string and looking
at individual characters in that string & turning on individual features
according to what characters it sees. There's several problems with this

 - There's no way to enumerate valid CPU model names

 - There can be many different names that all result
   in the same CPU model. eg "fdcs", "scdf", a"ffddccss"
   all result in the same features getting enabled
   by this loop above.

 - There's no way to check compatibility between CPUs

 - There will be no way to version the CPUs if we need
   to tie them to machine types for back compatibility.

If we have a base CPU model, with a bunch of features that can optionally
be enabled, then IMHO we should represent that the same way was we do
on x86, whre we have a CPU model name, and then a comma sepaprated list
of features.

If on the other hand we don't want to support the combinatorial matrix
of all possible feature flags, then we should just list all the expected
named CPU models that are required explicitly.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Alistair Francis]

On Tue, Apr 16, 2019 at 6:23 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Wed, Apr 10, 2019 at 11:10:25PM +0000, Alistair Francis 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.
> >
> > 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);
> > +}
>
> This whole approach feels undesirable to me, as it is quite different to
> way CPUs are represented in the other architectures in QEMU and as a result
> does not fit in the QAPI commands we've been building in QEMU for dealing
> with CPU model representation. This will make for increased maint burden
> in both QEMU and apps managing QEMU
>
> IIUC, this code is taking an arbitrary CPU model string and looking
> at individual characters in that string & turning on individual features
> according to what characters it sees. There's several problems with this
>
>  - There's no way to enumerate valid CPU model names
>
>  - There can be many different names that all result
>    in the same CPU model. eg "fdcs", "scdf", a"ffddccss"
>    all result in the same features getting enabled
>    by this loop above.
>
>  - There's no way to check compatibility between CPUs
>
>  - There will be no way to version the CPUs if we need
>    to tie them to machine types for back compatibility.
>
> If we have a base CPU model, with a bunch of features that can optionally
> be enabled, then IMHO we should represent that the same way was we do
> on x86, whre we have a CPU model name, and then a comma sepaprated list
> of features.
>
> If on the other hand we don't want to support the combinatorial matrix
> of all possible feature flags, then we should just list all the expected
> named CPU models that are required explicitly.

Ok, I'm about to send out a new series that is based on this series.
The new series is called "RISC-V: Add properties to the CPUs". It has
dropped the ISA string part (this patch) and just adds some standard
CPU properties. If everyone is happy with that approach I'll continue
to expand it to add the extensions as CPU properties.

Alistair

>
> Regards,
> Daniel
> --
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Alistair Francis]

On Tue, Apr 16, 2019 at 6:23 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Wed, Apr 10, 2019 at 11:10:25PM +0000, Alistair Francis 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.
> >
> > 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);
> > +}
>
> This whole approach feels undesirable to me, as it is quite different to
> way CPUs are represented in the other architectures in QEMU and as a result
> does not fit in the QAPI commands we've been building in QEMU for dealing
> with CPU model representation. This will make for increased maint burden
> in both QEMU and apps managing QEMU
>
> IIUC, this code is taking an arbitrary CPU model string and looking
> at individual characters in that string & turning on individual features
> according to what characters it sees. There's several problems with this
>
>  - There's no way to enumerate valid CPU model names
>
>  - There can be many different names that all result
>    in the same CPU model. eg "fdcs", "scdf", a"ffddccss"
>    all result in the same features getting enabled
>    by this loop above.
>
>  - There's no way to check compatibility between CPUs
>
>  - There will be no way to version the CPUs if we need
>    to tie them to machine types for back compatibility.
>
> If we have a base CPU model, with a bunch of features that can optionally
> be enabled, then IMHO we should represent that the same way was we do
> on x86, whre we have a CPU model name, and then a comma sepaprated list
> of features.
>
> If on the other hand we don't want to support the combinatorial matrix
> of all possible feature flags, then we should just list all the expected
> named CPU models that are required explicitly.

Ok, I'm about to send out a new series that is based on this series.
The new series is called "RISC-V: Add properties to the CPUs". It has
dropped the ISA string part (this patch) and just adds some standard
CPU properties. If everyone is happy with that approach I'll continue
to expand it to add the extensions as CPU properties.

Alistair

>
> Regards,
> Daniel
> --
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

[Qemu-devel] [PATCH for 4.1 v3 3/6] target/riscv: Create settable CPU properties

[Alistair Francis]

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 target/riscv/cpu.c | 52 ++++++++++++++++++++++++++++++++++++++++++----
 target/riscv/cpu.h |  8 +++++++
 2 files changed, 56 insertions(+), 4 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 27be9e412a..c792bacd24 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -23,6 +23,7 @@
 #include "cpu.h"
 #include "exec/exec-all.h"
 #include "qapi/error.h"
+#include "hw/qdev-properties.h"
 #include "migration/vmstate.h"
 
 /* RISC-V CPU definitions */
@@ -191,12 +192,9 @@ static void riscv_generate_cpu_init(Object *obj)
     }
 
     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;
@@ -394,7 +392,11 @@ static void riscv_cpu_disas_set_info(CPUState *s, disassemble_info *info)
 static void riscv_cpu_realize(DeviceState *dev, Error **errp)
 {
     CPUState *cs = CPU(dev);
+    RISCVCPU *cpu = RISCV_CPU(dev);
+    CPURISCVState *env = &cpu->env;
     RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(dev);
+    int priv_version = PRIV_VERSION_1_10_0;
+    int user_version = USER_VERSION_2_02_0;
     Error *local_err = NULL;
 
     cpu_exec_realizefn(cs, &local_err);
@@ -403,6 +405,39 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
         return;
     }
 
+    if (cpu->cfg.priv_spec) {
+        if (!g_strcmp0(cpu->cfg.priv_spec, "v1.10.0")) {
+            priv_version = PRIV_VERSION_1_10_0;
+        } else if (!g_strcmp0(cpu->cfg.priv_spec, "v1.9.1")) {
+            priv_version = PRIV_VERSION_1_09_1;
+        } else {
+            error_report("Unsupported privilege spec version '%s'",
+                         cpu->cfg.priv_spec);
+            exit(1);
+        }
+    }
+
+    if (cpu->cfg.user_spec) {
+        if (!g_strcmp0(cpu->cfg.user_spec, "v2.02.0")) {
+            user_version = USER_VERSION_2_02_0;
+        } else {
+            error_report("Unsupported user spec version '%s'",
+                         cpu->cfg.user_spec);
+            exit(1);
+        }
+    }
+
+    set_versions(env, user_version, priv_version);
+    set_resetvec(env, DEFAULT_RSTVEC);
+
+    if (cpu->cfg.mmu) {
+        set_feature(env, RISCV_FEATURE_MMU);
+    }
+
+    if (cpu->cfg.pmp) {
+        set_feature(env, RISCV_FEATURE_PMP);
+    }
+
     riscv_cpu_register_gdb_regs_for_features(cs);
 
     qemu_init_vcpu(cs);
@@ -424,6 +459,14 @@ static const VMStateDescription vmstate_riscv_cpu = {
     .unmigratable = 1,
 };
 
+static Property riscv_cpu_properties[] = {
+    DEFINE_PROP_STRING("priv_spec", RISCVCPU, cfg.priv_spec),
+    DEFINE_PROP_STRING("user_spec", RISCVCPU, cfg.user_spec),
+    DEFINE_PROP_BOOL("mmu", RISCVCPU, cfg.mmu, true),
+    DEFINE_PROP_BOOL("pmp", RISCVCPU, cfg.pmp, true),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void riscv_cpu_class_init(ObjectClass *c, void *data)
 {
     RISCVCPUClass *mcc = RISCV_CPU_CLASS(c);
@@ -464,6 +507,7 @@ static void riscv_cpu_class_init(ObjectClass *c, void *data)
 #endif
     /* For now, mark unmigratable: */
     cc->vmsd = &vmstate_riscv_cpu;
+    dc->props = riscv_cpu_properties;
 }
 
 char *riscv_isa_string(RISCVCPU *cpu)
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 453108a855..bc877d8107 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -226,6 +226,14 @@ typedef struct RISCVCPU {
     CPUState parent_obj;
     /*< public >*/
     CPURISCVState env;
+
+    /* Configuration Settings */
+    struct {
+        char *priv_spec;
+        char *user_spec;
+        bool mmu;
+        bool pmp;
+    } cfg;
 } RISCVCPU;
 
 static inline RISCVCPU *riscv_env_get_cpu(CPURISCVState *env)
-- 
2.21.0

[Qemu-devel] [PATCH for 4.1 v3 3/6] target/riscv: Create settable CPU properties

[Alistair Francis]

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 target/riscv/cpu.c | 52 ++++++++++++++++++++++++++++++++++++++++++----
 target/riscv/cpu.h |  8 +++++++
 2 files changed, 56 insertions(+), 4 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 27be9e412a..c792bacd24 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -23,6 +23,7 @@
 #include "cpu.h"
 #include "exec/exec-all.h"
 #include "qapi/error.h"
+#include "hw/qdev-properties.h"
 #include "migration/vmstate.h"
 
 /* RISC-V CPU definitions */
@@ -191,12 +192,9 @@ static void riscv_generate_cpu_init(Object *obj)
     }
 
     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;
@@ -394,7 +392,11 @@ static void riscv_cpu_disas_set_info(CPUState *s, disassemble_info *info)
 static void riscv_cpu_realize(DeviceState *dev, Error **errp)
 {
     CPUState *cs = CPU(dev);
+    RISCVCPU *cpu = RISCV_CPU(dev);
+    CPURISCVState *env = &cpu->env;
     RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(dev);
+    int priv_version = PRIV_VERSION_1_10_0;
+    int user_version = USER_VERSION_2_02_0;
     Error *local_err = NULL;
 
     cpu_exec_realizefn(cs, &local_err);
@@ -403,6 +405,39 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
         return;
     }
 
+    if (cpu->cfg.priv_spec) {
+        if (!g_strcmp0(cpu->cfg.priv_spec, "v1.10.0")) {
+            priv_version = PRIV_VERSION_1_10_0;
+        } else if (!g_strcmp0(cpu->cfg.priv_spec, "v1.9.1")) {
+            priv_version = PRIV_VERSION_1_09_1;
+        } else {
+            error_report("Unsupported privilege spec version '%s'",
+                         cpu->cfg.priv_spec);
+            exit(1);
+        }
+    }
+
+    if (cpu->cfg.user_spec) {
+        if (!g_strcmp0(cpu->cfg.user_spec, "v2.02.0")) {
+            user_version = USER_VERSION_2_02_0;
+        } else {
+            error_report("Unsupported user spec version '%s'",
+                         cpu->cfg.user_spec);
+            exit(1);
+        }
+    }
+
+    set_versions(env, user_version, priv_version);
+    set_resetvec(env, DEFAULT_RSTVEC);
+
+    if (cpu->cfg.mmu) {
+        set_feature(env, RISCV_FEATURE_MMU);
+    }
+
+    if (cpu->cfg.pmp) {
+        set_feature(env, RISCV_FEATURE_PMP);
+    }
+
     riscv_cpu_register_gdb_regs_for_features(cs);
 
     qemu_init_vcpu(cs);
@@ -424,6 +459,14 @@ static const VMStateDescription vmstate_riscv_cpu = {
     .unmigratable = 1,
 };
 
+static Property riscv_cpu_properties[] = {
+    DEFINE_PROP_STRING("priv_spec", RISCVCPU, cfg.priv_spec),
+    DEFINE_PROP_STRING("user_spec", RISCVCPU, cfg.user_spec),
+    DEFINE_PROP_BOOL("mmu", RISCVCPU, cfg.mmu, true),
+    DEFINE_PROP_BOOL("pmp", RISCVCPU, cfg.pmp, true),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void riscv_cpu_class_init(ObjectClass *c, void *data)
 {
     RISCVCPUClass *mcc = RISCV_CPU_CLASS(c);
@@ -464,6 +507,7 @@ static void riscv_cpu_class_init(ObjectClass *c, void *data)
 #endif
     /* For now, mark unmigratable: */
     cc->vmsd = &vmstate_riscv_cpu;
+    dc->props = riscv_cpu_properties;
 }
 
 char *riscv_isa_string(RISCVCPU *cpu)
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 453108a855..bc877d8107 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -226,6 +226,14 @@ typedef struct RISCVCPU {
     CPUState parent_obj;
     /*< public >*/
     CPURISCVState env;
+
+    /* Configuration Settings */
+    struct {
+        char *priv_spec;
+        char *user_spec;
+        bool mmu;
+        bool pmp;
+    } cfg;
 } RISCVCPU;
 
 static inline RISCVCPU *riscv_env_get_cpu(CPURISCVState *env)
-- 
2.21.0

[Qemu-devel] [PATCH for 4.1 v3 4/6] riscv: virt: Allow specifying a CPU via commandline

[Alistair Francis]

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 hw/riscv/virt.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index fc4c6b306e..5b25f028ad 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -400,7 +400,7 @@ static void riscv_virt_board_init(MachineState *machine)
     /* Initialize SOC */
     object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
                             TYPE_RISCV_HART_ARRAY, &error_abort, NULL);
-    object_property_set_str(OBJECT(&s->soc), VIRT_CPU, "cpu-type",
+    object_property_set_str(OBJECT(&s->soc), machine->cpu_type, "cpu-type",
                             &error_abort);
     object_property_set_int(OBJECT(&s->soc), smp_cpus, "num-harts",
                             &error_abort);
@@ -526,6 +526,7 @@ static void riscv_virt_board_machine_init(MachineClass *mc)
     mc->desc = "RISC-V VirtIO Board (Privileged ISA v1.10)";
     mc->init = riscv_virt_board_init;
     mc->max_cpus = 8; /* hardcoded limit in BBL */
+    mc->default_cpu_type = VIRT_CPU;
 }
 
 DEFINE_MACHINE("virt", riscv_virt_board_machine_init)
-- 
2.21.0

[Qemu-devel] [PATCH for 4.1 v3 4/6] riscv: virt: Allow specifying a CPU via commandline

[Alistair Francis]

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 hw/riscv/virt.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index fc4c6b306e..5b25f028ad 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -400,7 +400,7 @@ static void riscv_virt_board_init(MachineState *machine)
     /* Initialize SOC */
     object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
                             TYPE_RISCV_HART_ARRAY, &error_abort, NULL);
-    object_property_set_str(OBJECT(&s->soc), VIRT_CPU, "cpu-type",
+    object_property_set_str(OBJECT(&s->soc), machine->cpu_type, "cpu-type",
                             &error_abort);
     object_property_set_int(OBJECT(&s->soc), smp_cpus, "num-harts",
                             &error_abort);
@@ -526,6 +526,7 @@ static void riscv_virt_board_machine_init(MachineClass *mc)
     mc->desc = "RISC-V VirtIO Board (Privileged ISA v1.10)";
     mc->init = riscv_virt_board_init;
     mc->max_cpus = 8; /* hardcoded limit in BBL */
+    mc->default_cpu_type = VIRT_CPU;
 }
 
 DEFINE_MACHINE("virt", riscv_virt_board_machine_init)
-- 
2.21.0

Re: [Qemu-devel] [PATCH for 4.1 v3 4/6] riscv: virt: Allow specifying a CPU via commandline

[Igor Mammedov]

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

> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/riscv/virt.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index fc4c6b306e..5b25f028ad 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -400,7 +400,7 @@ static void riscv_virt_board_init(MachineState *machine)
>      /* Initialize SOC */
>      object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
>                              TYPE_RISCV_HART_ARRAY, &error_abort, NULL);
> -    object_property_set_str(OBJECT(&s->soc), VIRT_CPU, "cpu-type",
> +    object_property_set_str(OBJECT(&s->soc), machine->cpu_type, "cpu-type",
>                              &error_abort);
>      object_property_set_int(OBJECT(&s->soc), smp_cpus, "num-harts",
>                              &error_abort);
> @@ -526,6 +526,7 @@ static void riscv_virt_board_machine_init(MachineClass *mc)
>      mc->desc = "RISC-V VirtIO Board (Privileged ISA v1.10)";
>      mc->init = riscv_virt_board_init;
>      mc->max_cpus = 8; /* hardcoded limit in BBL */
> +    mc->default_cpu_type = VIRT_CPU;
>  }
>  
>  DEFINE_MACHINE("virt", riscv_virt_board_machine_init)

Re: [Qemu-devel] [PATCH for 4.1 v3 4/6] riscv: virt: Allow specifying a CPU via commandline

[Igor Mammedov]

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

> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/riscv/virt.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index fc4c6b306e..5b25f028ad 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -400,7 +400,7 @@ static void riscv_virt_board_init(MachineState *machine)
>      /* Initialize SOC */
>      object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
>                              TYPE_RISCV_HART_ARRAY, &error_abort, NULL);
> -    object_property_set_str(OBJECT(&s->soc), VIRT_CPU, "cpu-type",
> +    object_property_set_str(OBJECT(&s->soc), machine->cpu_type, "cpu-type",
>                              &error_abort);
>      object_property_set_int(OBJECT(&s->soc), smp_cpus, "num-harts",
>                              &error_abort);
> @@ -526,6 +526,7 @@ static void riscv_virt_board_machine_init(MachineClass *mc)
>      mc->desc = "RISC-V VirtIO Board (Privileged ISA v1.10)";
>      mc->init = riscv_virt_board_init;
>      mc->max_cpus = 8; /* hardcoded limit in BBL */
> +    mc->default_cpu_type = VIRT_CPU;
>  }
>  
>  DEFINE_MACHINE("virt", riscv_virt_board_machine_init)

[Qemu-devel] [PATCH for 4.1 v3 5/6] target/riscv: Remove the generic no MMU CPUs

[Alistair Francis]

These can now be specified via the command line so we no longer need
these.

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 target/riscv/cpu.c | 2 --
 target/riscv/cpu.h | 2 --
 2 files changed, 4 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index c792bacd24..9ba77a1983 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -586,13 +586,11 @@ static const TypeInfo riscv_cpu_type_infos[] = {
 #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),
-    DEFINE_CPU(TYPE_RISCV_CPU_RV32IMACU_NOMMU,  rv32imacu_nommu_cpu_init),
     DEFINE_CPU(TYPE_RISCV_CPU_SIFIVE_E31,       rv32imacu_nommu_cpu_init),
     DEFINE_CPU(TYPE_RISCV_CPU_SIFIVE_U34,       rv32gcsu_priv1_10_0_cpu_init)
 #elif defined(TARGET_RISCV64)
     DEFINE_CPU(TYPE_RISCV_CPU_RV64GCSU_V1_09_1, rv64gcsu_priv1_09_1_cpu_init),
     DEFINE_CPU(TYPE_RISCV_CPU_RV64GCSU_V1_10_0, rv64gcsu_priv1_10_0_cpu_init),
-    DEFINE_CPU(TYPE_RISCV_CPU_RV64IMACU_NOMMU,  rv64imacu_nommu_cpu_init),
     DEFINE_CPU(TYPE_RISCV_CPU_SIFIVE_E51,       rv64imacu_nommu_cpu_init),
     DEFINE_CPU(TYPE_RISCV_CPU_SIFIVE_U54,       rv64gcsu_priv1_10_0_cpu_init)
 #endif
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index bc877d8107..6806f602b5 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -51,10 +51,8 @@
 #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")
 #define TYPE_RISCV_CPU_RV64GCSU_V1_09_1 RISCV_CPU_TYPE_NAME("rv64gcsu-v1.9.1")
 #define TYPE_RISCV_CPU_RV64GCSU_V1_10_0 RISCV_CPU_TYPE_NAME("rv64gcsu-v1.10.0")
-#define TYPE_RISCV_CPU_RV64IMACU_NOMMU  RISCV_CPU_TYPE_NAME("rv64imacu-nommu")
 #define TYPE_RISCV_CPU_SIFIVE_E31       RISCV_CPU_TYPE_NAME("sifive-e31")
 #define TYPE_RISCV_CPU_SIFIVE_E51       RISCV_CPU_TYPE_NAME("sifive-e51")
 #define TYPE_RISCV_CPU_SIFIVE_U34       RISCV_CPU_TYPE_NAME("sifive-u34")
-- 
2.21.0

[Qemu-devel] [PATCH for 4.1 v3 5/6] target/riscv: Remove the generic no MMU CPUs

[Alistair Francis]

These can now be specified via the command line so we no longer need
these.

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 target/riscv/cpu.c | 2 --
 target/riscv/cpu.h | 2 --
 2 files changed, 4 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index c792bacd24..9ba77a1983 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -586,13 +586,11 @@ static const TypeInfo riscv_cpu_type_infos[] = {
 #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),
-    DEFINE_CPU(TYPE_RISCV_CPU_RV32IMACU_NOMMU,  rv32imacu_nommu_cpu_init),
     DEFINE_CPU(TYPE_RISCV_CPU_SIFIVE_E31,       rv32imacu_nommu_cpu_init),
     DEFINE_CPU(TYPE_RISCV_CPU_SIFIVE_U34,       rv32gcsu_priv1_10_0_cpu_init)
 #elif defined(TARGET_RISCV64)
     DEFINE_CPU(TYPE_RISCV_CPU_RV64GCSU_V1_09_1, rv64gcsu_priv1_09_1_cpu_init),
     DEFINE_CPU(TYPE_RISCV_CPU_RV64GCSU_V1_10_0, rv64gcsu_priv1_10_0_cpu_init),
-    DEFINE_CPU(TYPE_RISCV_CPU_RV64IMACU_NOMMU,  rv64imacu_nommu_cpu_init),
     DEFINE_CPU(TYPE_RISCV_CPU_SIFIVE_E51,       rv64imacu_nommu_cpu_init),
     DEFINE_CPU(TYPE_RISCV_CPU_SIFIVE_U54,       rv64gcsu_priv1_10_0_cpu_init)
 #endif
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index bc877d8107..6806f602b5 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -51,10 +51,8 @@
 #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")
 #define TYPE_RISCV_CPU_RV64GCSU_V1_09_1 RISCV_CPU_TYPE_NAME("rv64gcsu-v1.9.1")
 #define TYPE_RISCV_CPU_RV64GCSU_V1_10_0 RISCV_CPU_TYPE_NAME("rv64gcsu-v1.10.0")
-#define TYPE_RISCV_CPU_RV64IMACU_NOMMU  RISCV_CPU_TYPE_NAME("rv64imacu-nommu")
 #define TYPE_RISCV_CPU_SIFIVE_E31       RISCV_CPU_TYPE_NAME("sifive-e31")
 #define TYPE_RISCV_CPU_SIFIVE_E51       RISCV_CPU_TYPE_NAME("sifive-e51")
 #define TYPE_RISCV_CPU_SIFIVE_U34       RISCV_CPU_TYPE_NAME("sifive-u34")
-- 
2.21.0

[Qemu-devel] [PATCH for 4.1 v3 6/6] riscv: Add a generic spike machine

[Alistair Francis]

Add a generic spike machine (not tied to a version) and deprecate the
spike mahines that are tied to a specific version. As we can now specify
the CPU via the command line we no londer need specific versions of the
spike machines.

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 hw/riscv/spike.c | 106 ++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 105 insertions(+), 1 deletion(-)

diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
index 2a000a5800..9d3f7cec4d 100644
--- a/hw/riscv/spike.c
+++ b/hw/riscv/spike.c
@@ -39,6 +39,7 @@
 #include "chardev/char.h"
 #include "sysemu/arch_init.h"
 #include "sysemu/device_tree.h"
+#include "sysemu/qtest.h"
 #include "exec/address-spaces.h"
 #include "elf.h"
 
@@ -160,7 +161,89 @@ static void create_fdt(SpikeState *s, const struct MemmapEntry *memmap,
         qemu_fdt_add_subnode(fdt, "/chosen");
         qemu_fdt_setprop_string(fdt, "/chosen", "bootargs", cmdline);
     }
- }
+}
+
+static void spike_board_init(MachineState *machine)
+{
+    const struct MemmapEntry *memmap = spike_memmap;
+
+    SpikeState *s = g_new0(SpikeState, 1);
+    MemoryRegion *system_memory = get_system_memory();
+    MemoryRegion *main_mem = g_new(MemoryRegion, 1);
+    MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
+    int i;
+
+    /* Initialize SOC */
+    object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
+                            TYPE_RISCV_HART_ARRAY, &error_abort, NULL);
+    object_property_set_str(OBJECT(&s->soc), machine->cpu_type, "cpu-type",
+                            &error_abort);
+    object_property_set_int(OBJECT(&s->soc), smp_cpus, "num-harts",
+                            &error_abort);
+    object_property_set_bool(OBJECT(&s->soc), true, "realized",
+                            &error_abort);
+
+    /* register system main memory (actual RAM) */
+    memory_region_init_ram(main_mem, NULL, "riscv.spike.ram",
+                           machine->ram_size, &error_fatal);
+    memory_region_add_subregion(system_memory, memmap[SPIKE_DRAM].base,
+        main_mem);
+
+    /* create device tree */
+    create_fdt(s, memmap, machine->ram_size, machine->kernel_cmdline);
+
+    /* boot rom */
+    memory_region_init_rom(mask_rom, NULL, "riscv.spike.mrom",
+                           memmap[SPIKE_MROM].size, &error_fatal);
+    memory_region_add_subregion(system_memory, memmap[SPIKE_MROM].base,
+                                mask_rom);
+
+    if (machine->kernel_filename) {
+        load_kernel(machine->kernel_filename);
+    }
+
+    /* reset vector */
+    uint32_t reset_vec[8] = {
+        0x00000297,                  /* 1:  auipc  t0, %pcrel_hi(dtb) */
+        0x02028593,                  /*     addi   a1, t0, %pcrel_lo(1b) */
+        0xf1402573,                  /*     csrr   a0, mhartid  */
+#if defined(TARGET_RISCV32)
+        0x0182a283,                  /*     lw     t0, 24(t0) */
+#elif defined(TARGET_RISCV64)
+        0x0182b283,                  /*     ld     t0, 24(t0) */
+#endif
+        0x00028067,                  /*     jr     t0 */
+        0x00000000,
+        memmap[SPIKE_DRAM].base,     /* start: .dword DRAM_BASE */
+        0x00000000,
+                                     /* dtb: */
+    };
+
+    /* copy in the reset vector in little_endian byte order */
+    for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
+        reset_vec[i] = cpu_to_le32(reset_vec[i]);
+    }
+    rom_add_blob_fixed_as("mrom.reset", reset_vec, sizeof(reset_vec),
+                          memmap[SPIKE_MROM].base, &address_space_memory);
+
+    /* copy in the device tree */
+    if (fdt_pack(s->fdt) || fdt_totalsize(s->fdt) >
+            memmap[SPIKE_MROM].size - sizeof(reset_vec)) {
+        error_report("not enough space to store device-tree");
+        exit(1);
+    }
+    qemu_fdt_dumpdtb(s->fdt, fdt_totalsize(s->fdt));
+    rom_add_blob_fixed_as("mrom.fdt", s->fdt, fdt_totalsize(s->fdt),
+                          memmap[SPIKE_MROM].base + sizeof(reset_vec),
+                          &address_space_memory);
+
+    /* initialize HTIF using symbols found in load_kernel */
+    htif_mm_init(system_memory, mask_rom, &s->soc.harts[0].env, serial_hd(0));
+
+    /* Core Local Interruptor (timer and IPI) */
+    sifive_clint_create(memmap[SPIKE_CLINT].base, memmap[SPIKE_CLINT].size,
+        smp_cpus, SIFIVE_SIP_BASE, SIFIVE_TIMECMP_BASE, SIFIVE_TIME_BASE);
+}
 
 static void spike_v1_10_0_board_init(MachineState *machine)
 {
@@ -172,6 +255,12 @@ static void spike_v1_10_0_board_init(MachineState *machine)
     MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
     int i;
 
+    if (!qtest_enabled()) {
+        info_report("The Spike v1.10.0 machine has been depreceated. "
+                    "Please use the deneric spike machine and specify the ISA "
+                    "versions using -cpu.");
+    }
+
     /* Initialize SOC */
     object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
                             TYPE_RISCV_HART_ARRAY, &error_abort, NULL);
@@ -254,6 +343,12 @@ static void spike_v1_09_1_board_init(MachineState *machine)
     MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
     int i;
 
+    if (!qtest_enabled()) {
+        info_report("The Spike v1.09.1 machine has been depreceated. "
+                    "Please use the deneric spike machine and specify the ISA "
+                    "versions using -cpu.");
+    }
+
     /* Initialize SOC */
     object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
                             TYPE_RISCV_HART_ARRAY, &error_abort, NULL);
@@ -359,8 +454,17 @@ static void spike_v1_10_0_machine_init(MachineClass *mc)
     mc->desc = "RISC-V Spike Board (Privileged ISA v1.10)";
     mc->init = spike_v1_10_0_board_init;
     mc->max_cpus = 1;
+}
+
+static void spike_machine_init(MachineClass *mc)
+{
+    mc->desc = "RISC-V Spike Board";
+    mc->init = spike_board_init;
+    mc->max_cpus = 1;
     mc->is_default = 1;
+    mc->default_cpu_type = SPIKE_V1_10_0_CPU;
 }
 
 DEFINE_MACHINE("spike_v1.9.1", spike_v1_09_1_machine_init)
 DEFINE_MACHINE("spike_v1.10", spike_v1_10_0_machine_init)
+DEFINE_MACHINE("spike", spike_machine_init)
-- 
2.21.0

[Qemu-devel] [PATCH for 4.1 v3 6/6] riscv: Add a generic spike machine

[Alistair Francis]

Add a generic spike machine (not tied to a version) and deprecate the
spike mahines that are tied to a specific version. As we can now specify
the CPU via the command line we no londer need specific versions of the
spike machines.

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 hw/riscv/spike.c | 106 ++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 105 insertions(+), 1 deletion(-)

diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
index 2a000a5800..9d3f7cec4d 100644
--- a/hw/riscv/spike.c
+++ b/hw/riscv/spike.c
@@ -39,6 +39,7 @@
 #include "chardev/char.h"
 #include "sysemu/arch_init.h"
 #include "sysemu/device_tree.h"
+#include "sysemu/qtest.h"
 #include "exec/address-spaces.h"
 #include "elf.h"
 
@@ -160,7 +161,89 @@ static void create_fdt(SpikeState *s, const struct MemmapEntry *memmap,
         qemu_fdt_add_subnode(fdt, "/chosen");
         qemu_fdt_setprop_string(fdt, "/chosen", "bootargs", cmdline);
     }
- }
+}
+
+static void spike_board_init(MachineState *machine)
+{
+    const struct MemmapEntry *memmap = spike_memmap;
+
+    SpikeState *s = g_new0(SpikeState, 1);
+    MemoryRegion *system_memory = get_system_memory();
+    MemoryRegion *main_mem = g_new(MemoryRegion, 1);
+    MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
+    int i;
+
+    /* Initialize SOC */
+    object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
+                            TYPE_RISCV_HART_ARRAY, &error_abort, NULL);
+    object_property_set_str(OBJECT(&s->soc), machine->cpu_type, "cpu-type",
+                            &error_abort);
+    object_property_set_int(OBJECT(&s->soc), smp_cpus, "num-harts",
+                            &error_abort);
+    object_property_set_bool(OBJECT(&s->soc), true, "realized",
+                            &error_abort);
+
+    /* register system main memory (actual RAM) */
+    memory_region_init_ram(main_mem, NULL, "riscv.spike.ram",
+                           machine->ram_size, &error_fatal);
+    memory_region_add_subregion(system_memory, memmap[SPIKE_DRAM].base,
+        main_mem);
+
+    /* create device tree */
+    create_fdt(s, memmap, machine->ram_size, machine->kernel_cmdline);
+
+    /* boot rom */
+    memory_region_init_rom(mask_rom, NULL, "riscv.spike.mrom",
+                           memmap[SPIKE_MROM].size, &error_fatal);
+    memory_region_add_subregion(system_memory, memmap[SPIKE_MROM].base,
+                                mask_rom);
+
+    if (machine->kernel_filename) {
+        load_kernel(machine->kernel_filename);
+    }
+
+    /* reset vector */
+    uint32_t reset_vec[8] = {
+        0x00000297,                  /* 1:  auipc  t0, %pcrel_hi(dtb) */
+        0x02028593,                  /*     addi   a1, t0, %pcrel_lo(1b) */
+        0xf1402573,                  /*     csrr   a0, mhartid  */
+#if defined(TARGET_RISCV32)
+        0x0182a283,                  /*     lw     t0, 24(t0) */
+#elif defined(TARGET_RISCV64)
+        0x0182b283,                  /*     ld     t0, 24(t0) */
+#endif
+        0x00028067,                  /*     jr     t0 */
+        0x00000000,
+        memmap[SPIKE_DRAM].base,     /* start: .dword DRAM_BASE */
+        0x00000000,
+                                     /* dtb: */
+    };
+
+    /* copy in the reset vector in little_endian byte order */
+    for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
+        reset_vec[i] = cpu_to_le32(reset_vec[i]);
+    }
+    rom_add_blob_fixed_as("mrom.reset", reset_vec, sizeof(reset_vec),
+                          memmap[SPIKE_MROM].base, &address_space_memory);
+
+    /* copy in the device tree */
+    if (fdt_pack(s->fdt) || fdt_totalsize(s->fdt) >
+            memmap[SPIKE_MROM].size - sizeof(reset_vec)) {
+        error_report("not enough space to store device-tree");
+        exit(1);
+    }
+    qemu_fdt_dumpdtb(s->fdt, fdt_totalsize(s->fdt));
+    rom_add_blob_fixed_as("mrom.fdt", s->fdt, fdt_totalsize(s->fdt),
+                          memmap[SPIKE_MROM].base + sizeof(reset_vec),
+                          &address_space_memory);
+
+    /* initialize HTIF using symbols found in load_kernel */
+    htif_mm_init(system_memory, mask_rom, &s->soc.harts[0].env, serial_hd(0));
+
+    /* Core Local Interruptor (timer and IPI) */
+    sifive_clint_create(memmap[SPIKE_CLINT].base, memmap[SPIKE_CLINT].size,
+        smp_cpus, SIFIVE_SIP_BASE, SIFIVE_TIMECMP_BASE, SIFIVE_TIME_BASE);
+}
 
 static void spike_v1_10_0_board_init(MachineState *machine)
 {
@@ -172,6 +255,12 @@ static void spike_v1_10_0_board_init(MachineState *machine)
     MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
     int i;
 
+    if (!qtest_enabled()) {
+        info_report("The Spike v1.10.0 machine has been depreceated. "
+                    "Please use the deneric spike machine and specify the ISA "
+                    "versions using -cpu.");
+    }
+
     /* Initialize SOC */
     object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
                             TYPE_RISCV_HART_ARRAY, &error_abort, NULL);
@@ -254,6 +343,12 @@ static void spike_v1_09_1_board_init(MachineState *machine)
     MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
     int i;
 
+    if (!qtest_enabled()) {
+        info_report("The Spike v1.09.1 machine has been depreceated. "
+                    "Please use the deneric spike machine and specify the ISA "
+                    "versions using -cpu.");
+    }
+
     /* Initialize SOC */
     object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
                             TYPE_RISCV_HART_ARRAY, &error_abort, NULL);
@@ -359,8 +454,17 @@ static void spike_v1_10_0_machine_init(MachineClass *mc)
     mc->desc = "RISC-V Spike Board (Privileged ISA v1.10)";
     mc->init = spike_v1_10_0_board_init;
     mc->max_cpus = 1;
+}
+
+static void spike_machine_init(MachineClass *mc)
+{
+    mc->desc = "RISC-V Spike Board";
+    mc->init = spike_board_init;
+    mc->max_cpus = 1;
     mc->is_default = 1;
+    mc->default_cpu_type = SPIKE_V1_10_0_CPU;
 }
 
 DEFINE_MACHINE("spike_v1.9.1", spike_v1_09_1_machine_init)
 DEFINE_MACHINE("spike_v1.10", spike_v1_10_0_machine_init)
+DEFINE_MACHINE("spike", spike_machine_init)
-- 
2.21.0

Re: [Qemu-devel] [PATCH for 4.1 v3 6/6] riscv: Add a generic spike machine

[Igor Mammedov]

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

> Add a generic spike machine (not tied to a version) and deprecate the
> spike mahines that are tied to a specific version. As we can now specify
> the CPU via the command line we no londer need specific versions of the
> spike machines.
> 
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>

For cpu and initial RAM related parts:

Acked-by: Igor Mammedov <imammedo@redhat.com>

a couple of questions below.
> ---
>  hw/riscv/spike.c | 106 ++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 105 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
> index 2a000a5800..9d3f7cec4d 100644
> --- a/hw/riscv/spike.c
> +++ b/hw/riscv/spike.c
> @@ -39,6 +39,7 @@
>  #include "chardev/char.h"
>  #include "sysemu/arch_init.h"
>  #include "sysemu/device_tree.h"
> +#include "sysemu/qtest.h"
>  #include "exec/address-spaces.h"
>  #include "elf.h"
>  
> @@ -160,7 +161,89 @@ static void create_fdt(SpikeState *s, const struct MemmapEntry *memmap,
>          qemu_fdt_add_subnode(fdt, "/chosen");
>          qemu_fdt_setprop_string(fdt, "/chosen", "bootargs", cmdline);
>      }
> - }
> +}
> +
> +static void spike_board_init(MachineState *machine)
> +{
> +    const struct MemmapEntry *memmap = spike_memmap;
> +
> +    SpikeState *s = g_new0(SpikeState, 1);
> +    MemoryRegion *system_memory = get_system_memory();
> +    MemoryRegion *main_mem = g_new(MemoryRegion, 1);
> +    MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
> +    int i;
> +
> +    /* Initialize SOC */
> +    object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
> +                            TYPE_RISCV_HART_ARRAY, &error_abort, NULL);
> +    object_property_set_str(OBJECT(&s->soc), machine->cpu_type, "cpu-type",
> +                            &error_abort);
> +    object_property_set_int(OBJECT(&s->soc), smp_cpus, "num-harts",
> +                            &error_abort);
> +    object_property_set_bool(OBJECT(&s->soc), true, "realized",
> +                            &error_abort);
> +
> +    /* register system main memory (actual RAM) */
> +    memory_region_init_ram(main_mem, NULL, "riscv.spike.ram",
> +                           machine->ram_size, &error_fatal);
do you really care about migration? if not then _nomigrate flavor would
be more suitable.

> +    memory_region_add_subregion(system_memory, memmap[SPIKE_DRAM].base,
> +        main_mem);
> +
> +    /* create device tree */
> +    create_fdt(s, memmap, machine->ram_size, machine->kernel_cmdline);
> +
> +    /* boot rom */
> +    memory_region_init_rom(mask_rom, NULL, "riscv.spike.mrom",
> +                           memmap[SPIKE_MROM].size, &error_fatal);
> +    memory_region_add_subregion(system_memory, memmap[SPIKE_MROM].base,
> +                                mask_rom);
> +
> +    if (machine->kernel_filename) {
> +        load_kernel(machine->kernel_filename);
> +    }
> +
> +    /* reset vector */
> +    uint32_t reset_vec[8] = {
> +        0x00000297,                  /* 1:  auipc  t0, %pcrel_hi(dtb) */
> +        0x02028593,                  /*     addi   a1, t0, %pcrel_lo(1b) */
> +        0xf1402573,                  /*     csrr   a0, mhartid  */
> +#if defined(TARGET_RISCV32)
> +        0x0182a283,                  /*     lw     t0, 24(t0) */
> +#elif defined(TARGET_RISCV64)
> +        0x0182b283,                  /*     ld     t0, 24(t0) */
> +#endif
> +        0x00028067,                  /*     jr     t0 */
> +        0x00000000,
> +        memmap[SPIKE_DRAM].base,     /* start: .dword DRAM_BASE */
> +        0x00000000,
> +                                     /* dtb: */
> +    };
> +
> +    /* copy in the reset vector in little_endian byte order */
> +    for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
> +        reset_vec[i] = cpu_to_le32(reset_vec[i]);
> +    }
> +    rom_add_blob_fixed_as("mrom.reset", reset_vec, sizeof(reset_vec),
> +                          memmap[SPIKE_MROM].base, &address_space_memory);
> +
> +    /* copy in the device tree */
> +    if (fdt_pack(s->fdt) || fdt_totalsize(s->fdt) >
> +            memmap[SPIKE_MROM].size - sizeof(reset_vec)) {
> +        error_report("not enough space to store device-tree");
> +        exit(1);
> +    }
> +    qemu_fdt_dumpdtb(s->fdt, fdt_totalsize(s->fdt));
> +    rom_add_blob_fixed_as("mrom.fdt", s->fdt, fdt_totalsize(s->fdt),
> +                          memmap[SPIKE_MROM].base + sizeof(reset_vec),
> +                          &address_space_memory);
> +
> +    /* initialize HTIF using symbols found in load_kernel */
> +    htif_mm_init(system_memory, mask_rom, &s->soc.harts[0].env, serial_hd(0));
> +
> +    /* Core Local Interruptor (timer and IPI) */
> +    sifive_clint_create(memmap[SPIKE_CLINT].base, memmap[SPIKE_CLINT].size,
> +        smp_cpus, SIFIVE_SIP_BASE, SIFIVE_TIMECMP_BASE, SIFIVE_TIME_BASE);
> +}
>  
>  static void spike_v1_10_0_board_init(MachineState *machine)
>  {
> @@ -172,6 +255,12 @@ static void spike_v1_10_0_board_init(MachineState *machine)
>      MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
>      int i;
>  
> +    if (!qtest_enabled()) {
> +        info_report("The Spike v1.10.0 machine has been depreceated. "
> +                    "Please use the deneric spike machine and specify the ISA "
> +                    "versions using -cpu.");
> +    }
Did you mean deprecated in sense that machines will be removed in 2 releases
according to QEMU's deprecation policy?

> +
>      /* Initialize SOC */
>      object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
>                              TYPE_RISCV_HART_ARRAY, &error_abort, NULL);
> @@ -254,6 +343,12 @@ static void spike_v1_09_1_board_init(MachineState *machine)
>      MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
>      int i;
>  
> +    if (!qtest_enabled()) {
> +        info_report("The Spike v1.09.1 machine has been depreceated. "
> +                    "Please use the deneric spike machine and specify the ISA "
> +                    "versions using -cpu.");
> +    }
> +
>      /* Initialize SOC */
>      object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
>                              TYPE_RISCV_HART_ARRAY, &error_abort, NULL);
> @@ -359,8 +454,17 @@ static void spike_v1_10_0_machine_init(MachineClass *mc)
>      mc->desc = "RISC-V Spike Board (Privileged ISA v1.10)";
>      mc->init = spike_v1_10_0_board_init;
>      mc->max_cpus = 1;
> +}
> +
> +static void spike_machine_init(MachineClass *mc)
> +{
> +    mc->desc = "RISC-V Spike Board";
> +    mc->init = spike_board_init;
> +    mc->max_cpus = 1;
>      mc->is_default = 1;
> +    mc->default_cpu_type = SPIKE_V1_10_0_CPU;
>  }
>  
>  DEFINE_MACHINE("spike_v1.9.1", spike_v1_09_1_machine_init)
>  DEFINE_MACHINE("spike_v1.10", spike_v1_10_0_machine_init)
> +DEFINE_MACHINE("spike", spike_machine_init)

Re: [Qemu-devel] [PATCH for 4.1 v3 6/6] riscv: Add a generic spike machine

[Igor Mammedov]

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

> Add a generic spike machine (not tied to a version) and deprecate the
> spike mahines that are tied to a specific version. As we can now specify
> the CPU via the command line we no londer need specific versions of the
> spike machines.
> 
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>

For cpu and initial RAM related parts:

Acked-by: Igor Mammedov <imammedo@redhat.com>

a couple of questions below.
> ---
>  hw/riscv/spike.c | 106 ++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 105 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
> index 2a000a5800..9d3f7cec4d 100644
> --- a/hw/riscv/spike.c
> +++ b/hw/riscv/spike.c
> @@ -39,6 +39,7 @@
>  #include "chardev/char.h"
>  #include "sysemu/arch_init.h"
>  #include "sysemu/device_tree.h"
> +#include "sysemu/qtest.h"
>  #include "exec/address-spaces.h"
>  #include "elf.h"
>  
> @@ -160,7 +161,89 @@ static void create_fdt(SpikeState *s, const struct MemmapEntry *memmap,
>          qemu_fdt_add_subnode(fdt, "/chosen");
>          qemu_fdt_setprop_string(fdt, "/chosen", "bootargs", cmdline);
>      }
> - }
> +}
> +
> +static void spike_board_init(MachineState *machine)
> +{
> +    const struct MemmapEntry *memmap = spike_memmap;
> +
> +    SpikeState *s = g_new0(SpikeState, 1);
> +    MemoryRegion *system_memory = get_system_memory();
> +    MemoryRegion *main_mem = g_new(MemoryRegion, 1);
> +    MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
> +    int i;
> +
> +    /* Initialize SOC */
> +    object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
> +                            TYPE_RISCV_HART_ARRAY, &error_abort, NULL);
> +    object_property_set_str(OBJECT(&s->soc), machine->cpu_type, "cpu-type",
> +                            &error_abort);
> +    object_property_set_int(OBJECT(&s->soc), smp_cpus, "num-harts",
> +                            &error_abort);
> +    object_property_set_bool(OBJECT(&s->soc), true, "realized",
> +                            &error_abort);
> +
> +    /* register system main memory (actual RAM) */
> +    memory_region_init_ram(main_mem, NULL, "riscv.spike.ram",
> +                           machine->ram_size, &error_fatal);
do you really care about migration? if not then _nomigrate flavor would
be more suitable.

> +    memory_region_add_subregion(system_memory, memmap[SPIKE_DRAM].base,
> +        main_mem);
> +
> +    /* create device tree */
> +    create_fdt(s, memmap, machine->ram_size, machine->kernel_cmdline);
> +
> +    /* boot rom */
> +    memory_region_init_rom(mask_rom, NULL, "riscv.spike.mrom",
> +                           memmap[SPIKE_MROM].size, &error_fatal);
> +    memory_region_add_subregion(system_memory, memmap[SPIKE_MROM].base,
> +                                mask_rom);
> +
> +    if (machine->kernel_filename) {
> +        load_kernel(machine->kernel_filename);
> +    }
> +
> +    /* reset vector */
> +    uint32_t reset_vec[8] = {
> +        0x00000297,                  /* 1:  auipc  t0, %pcrel_hi(dtb) */
> +        0x02028593,                  /*     addi   a1, t0, %pcrel_lo(1b) */
> +        0xf1402573,                  /*     csrr   a0, mhartid  */
> +#if defined(TARGET_RISCV32)
> +        0x0182a283,                  /*     lw     t0, 24(t0) */
> +#elif defined(TARGET_RISCV64)
> +        0x0182b283,                  /*     ld     t0, 24(t0) */
> +#endif
> +        0x00028067,                  /*     jr     t0 */
> +        0x00000000,
> +        memmap[SPIKE_DRAM].base,     /* start: .dword DRAM_BASE */
> +        0x00000000,
> +                                     /* dtb: */
> +    };
> +
> +    /* copy in the reset vector in little_endian byte order */
> +    for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
> +        reset_vec[i] = cpu_to_le32(reset_vec[i]);
> +    }
> +    rom_add_blob_fixed_as("mrom.reset", reset_vec, sizeof(reset_vec),
> +                          memmap[SPIKE_MROM].base, &address_space_memory);
> +
> +    /* copy in the device tree */
> +    if (fdt_pack(s->fdt) || fdt_totalsize(s->fdt) >
> +            memmap[SPIKE_MROM].size - sizeof(reset_vec)) {
> +        error_report("not enough space to store device-tree");
> +        exit(1);
> +    }
> +    qemu_fdt_dumpdtb(s->fdt, fdt_totalsize(s->fdt));
> +    rom_add_blob_fixed_as("mrom.fdt", s->fdt, fdt_totalsize(s->fdt),
> +                          memmap[SPIKE_MROM].base + sizeof(reset_vec),
> +                          &address_space_memory);
> +
> +    /* initialize HTIF using symbols found in load_kernel */
> +    htif_mm_init(system_memory, mask_rom, &s->soc.harts[0].env, serial_hd(0));
> +
> +    /* Core Local Interruptor (timer and IPI) */
> +    sifive_clint_create(memmap[SPIKE_CLINT].base, memmap[SPIKE_CLINT].size,
> +        smp_cpus, SIFIVE_SIP_BASE, SIFIVE_TIMECMP_BASE, SIFIVE_TIME_BASE);
> +}
>  
>  static void spike_v1_10_0_board_init(MachineState *machine)
>  {
> @@ -172,6 +255,12 @@ static void spike_v1_10_0_board_init(MachineState *machine)
>      MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
>      int i;
>  
> +    if (!qtest_enabled()) {
> +        info_report("The Spike v1.10.0 machine has been depreceated. "
> +                    "Please use the deneric spike machine and specify the ISA "
> +                    "versions using -cpu.");
> +    }
Did you mean deprecated in sense that machines will be removed in 2 releases
according to QEMU's deprecation policy?

> +
>      /* Initialize SOC */
>      object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
>                              TYPE_RISCV_HART_ARRAY, &error_abort, NULL);
> @@ -254,6 +343,12 @@ static void spike_v1_09_1_board_init(MachineState *machine)
>      MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
>      int i;
>  
> +    if (!qtest_enabled()) {
> +        info_report("The Spike v1.09.1 machine has been depreceated. "
> +                    "Please use the deneric spike machine and specify the ISA "
> +                    "versions using -cpu.");
> +    }
> +
>      /* Initialize SOC */
>      object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
>                              TYPE_RISCV_HART_ARRAY, &error_abort, NULL);
> @@ -359,8 +454,17 @@ static void spike_v1_10_0_machine_init(MachineClass *mc)
>      mc->desc = "RISC-V Spike Board (Privileged ISA v1.10)";
>      mc->init = spike_v1_10_0_board_init;
>      mc->max_cpus = 1;
> +}
> +
> +static void spike_machine_init(MachineClass *mc)
> +{
> +    mc->desc = "RISC-V Spike Board";
> +    mc->init = spike_board_init;
> +    mc->max_cpus = 1;
>      mc->is_default = 1;
> +    mc->default_cpu_type = SPIKE_V1_10_0_CPU;
>  }
>  
>  DEFINE_MACHINE("spike_v1.9.1", spike_v1_09_1_machine_init)
>  DEFINE_MACHINE("spike_v1.10", spike_v1_10_0_machine_init)
> +DEFINE_MACHINE("spike", spike_machine_init)

Re: [Qemu-devel] [PATCH for 4.1 v3 6/6] riscv: Add a generic spike machine

[Peter Maydell]

On Thu, 11 Apr 2019 at 13:07, Igor Mammedov <imammedo@redhat.com> wrote:
>
> On Wed, 10 Apr 2019 23:11:00 +0000
> Alistair Francis <Alistair.Francis@wdc.com> wrote:
> > +    /* register system main memory (actual RAM) */
> > +    memory_region_init_ram(main_mem, NULL, "riscv.spike.ram",
> > +                           machine->ram_size, &error_fatal);
> do you really care about migration? if not then _nomigrate flavor would
> be more suitable.

Every machine model should care about migration, at least to
the extent of making savevm/loadvm snapshots work. Using
memory_region_init_ram_nomigrate() is a bad idea in new
code: it exists only for the benefit of older code which
is handling the migration of the backing RAM by hand for
migration-compatibility reasons.

> >      /* Initialize SOC */
> >      object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
> >                              TYPE_RISCV_HART_ARRAY, &error_abort, NULL);
> > @@ -254,6 +343,12 @@ static void spike_v1_09_1_board_init(MachineState *machine)
> >      MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
> >      int i;
> >
> > +    if (!qtest_enabled()) {
> > +        info_report("The Spike v1.09.1 machine has been depreceated. "
> > +                    "Please use the deneric spike machine and specify the ISA "

"generic"

> > +                    "versions using -cpu.");
> > +    }
> > +

thanks
-- PMM

Re: [Qemu-devel] [PATCH for 4.1 v3 6/6] riscv: Add a generic spike machine

[Peter Maydell]

On Thu, 11 Apr 2019 at 13:07, Igor Mammedov <imammedo@redhat.com> wrote:
>
> On Wed, 10 Apr 2019 23:11:00 +0000
> Alistair Francis <Alistair.Francis@wdc.com> wrote:
> > +    /* register system main memory (actual RAM) */
> > +    memory_region_init_ram(main_mem, NULL, "riscv.spike.ram",
> > +                           machine->ram_size, &error_fatal);
> do you really care about migration? if not then _nomigrate flavor would
> be more suitable.

Every machine model should care about migration, at least to
the extent of making savevm/loadvm snapshots work. Using
memory_region_init_ram_nomigrate() is a bad idea in new
code: it exists only for the benefit of older code which
is handling the migration of the backing RAM by hand for
migration-compatibility reasons.

> >      /* Initialize SOC */
> >      object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
> >                              TYPE_RISCV_HART_ARRAY, &error_abort, NULL);
> > @@ -254,6 +343,12 @@ static void spike_v1_09_1_board_init(MachineState *machine)
> >      MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
> >      int i;
> >
> > +    if (!qtest_enabled()) {
> > +        info_report("The Spike v1.09.1 machine has been depreceated. "
> > +                    "Please use the deneric spike machine and specify the ISA "

"generic"

> > +                    "versions using -cpu.");
> > +    }
> > +

thanks
-- PMM

Re: [Qemu-devel] [PATCH for 4.1 v3 6/6] riscv: Add a generic spike machine

[Alistair Francis]

On Thu, Apr 11, 2019 at 5:18 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Thu, 11 Apr 2019 at 13:07, Igor Mammedov <imammedo@redhat.com> wrote:
> >
> > On Wed, 10 Apr 2019 23:11:00 +0000
> > Alistair Francis <Alistair.Francis@wdc.com> wrote:
> > > +    /* register system main memory (actual RAM) */
> > > +    memory_region_init_ram(main_mem, NULL, "riscv.spike.ram",
> > > +                           machine->ram_size, &error_fatal);
> > do you really care about migration? if not then _nomigrate flavor would
> > be more suitable.
>
> Every machine model should care about migration, at least to
> the extent of making savevm/loadvm snapshots work. Using
> memory_region_init_ram_nomigrate() is a bad idea in new
> code: it exists only for the benefit of older code which
> is handling the migration of the backing RAM by hand for
> migration-compatibility reasons.

Ok, I'll leave this as is then.

>
> > >      /* Initialize SOC */
> > >      object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
> > >                              TYPE_RISCV_HART_ARRAY, &error_abort, NULL);
> > > @@ -254,6 +343,12 @@ static void spike_v1_09_1_board_init(MachineState *machine)
> > >      MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
> > >      int i;
> > >
> > > +    if (!qtest_enabled()) {
> > > +        info_report("The Spike v1.09.1 machine has been depreceated. "
> > > +                    "Please use the deneric spike machine and specify the ISA "
>
> "generic"

Fixed in v4.

Alistair

>
> > > +                    "versions using -cpu.");
> > > +    }
> > > +
>
> thanks
> -- PMM

Re: [Qemu-devel] [PATCH for 4.1 v3 6/6] riscv: Add a generic spike machine

[Alistair Francis]

On Thu, Apr 11, 2019 at 5:18 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Thu, 11 Apr 2019 at 13:07, Igor Mammedov <imammedo@redhat.com> wrote:
> >
> > On Wed, 10 Apr 2019 23:11:00 +0000
> > Alistair Francis <Alistair.Francis@wdc.com> wrote:
> > > +    /* register system main memory (actual RAM) */
> > > +    memory_region_init_ram(main_mem, NULL, "riscv.spike.ram",
> > > +                           machine->ram_size, &error_fatal);
> > do you really care about migration? if not then _nomigrate flavor would
> > be more suitable.
>
> Every machine model should care about migration, at least to
> the extent of making savevm/loadvm snapshots work. Using
> memory_region_init_ram_nomigrate() is a bad idea in new
> code: it exists only for the benefit of older code which
> is handling the migration of the backing RAM by hand for
> migration-compatibility reasons.

Ok, I'll leave this as is then.

>
> > >      /* Initialize SOC */
> > >      object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
> > >                              TYPE_RISCV_HART_ARRAY, &error_abort, NULL);
> > > @@ -254,6 +343,12 @@ static void spike_v1_09_1_board_init(MachineState *machine)
> > >      MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
> > >      int i;
> > >
> > > +    if (!qtest_enabled()) {
> > > +        info_report("The Spike v1.09.1 machine has been depreceated. "
> > > +                    "Please use the deneric spike machine and specify the ISA "
>
> "generic"

Fixed in v4.

Alistair

>
> > > +                    "versions using -cpu.");
> > > +    }
> > > +
>
> thanks
> -- PMM

Re: [Qemu-devel] [PATCH for 4.1 v3 6/6] riscv: Add a generic spike machine

[Ian Campbell]

On Thu, 2019-04-11 at 13:35 -0700, Alistair Francis wrote:
> > > > +    if (!qtest_enabled()) {
> > > > +        info_report("The Spike v1.09.1 machine has been
> > > > depreceated. "
> > > > +                    "Please use the deneric spike machine and
> > > > specify the ISA "
> > 
> > "generic"

Also "deprecated" not "depreceated".

Ian.

Re: [Qemu-devel] [PATCH for 4.1 v3 6/6] riscv: Add a generic spike machine

[Ian Campbell]

On Thu, 2019-04-11 at 13:35 -0700, Alistair Francis wrote:
> > > > +    if (!qtest_enabled()) {
> > > > +        info_report("The Spike v1.09.1 machine has been
> > > > depreceated. "
> > > > +                    "Please use the deneric spike machine and
> > > > specify the ISA "
> > 
> > "generic"

Also "deprecated" not "depreceated".

Ian.

Re: [Qemu-devel] [PATCH for 4.1 v3 6/6] riscv: Add a generic spike machine

[Alistair Francis]

On Thu, Apr 11, 2019 at 5:06 AM Igor Mammedov <imammedo@redhat.com> wrote:
>
> On Wed, 10 Apr 2019 23:11:00 +0000
> Alistair Francis <Alistair.Francis@wdc.com> wrote:
>
> > Add a generic spike machine (not tied to a version) and deprecate the
> > spike mahines that are tied to a specific version. As we can now specify
> > the CPU via the command line we no londer need specific versions of the
> > spike machines.
> >
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
>
> For cpu and initial RAM related parts:
>
> Acked-by: Igor Mammedov <imammedo@redhat.com>
>
> a couple of questions below.
> > ---
> >  hw/riscv/spike.c | 106 ++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 105 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
> > index 2a000a5800..9d3f7cec4d 100644
> > --- a/hw/riscv/spike.c
> > +++ b/hw/riscv/spike.c
> > @@ -39,6 +39,7 @@
> >  #include "chardev/char.h"
> >  #include "sysemu/arch_init.h"
> >  #include "sysemu/device_tree.h"
> > +#include "sysemu/qtest.h"
> >  #include "exec/address-spaces.h"
> >  #include "elf.h"
> >
> > @@ -160,7 +161,89 @@ static void create_fdt(SpikeState *s, const struct MemmapEntry *memmap,
> >          qemu_fdt_add_subnode(fdt, "/chosen");
> >          qemu_fdt_setprop_string(fdt, "/chosen", "bootargs", cmdline);
> >      }
> > - }
> > +}
> > +
> > +static void spike_board_init(MachineState *machine)
> > +{
> > +    const struct MemmapEntry *memmap = spike_memmap;
> > +
> > +    SpikeState *s = g_new0(SpikeState, 1);
> > +    MemoryRegion *system_memory = get_system_memory();
> > +    MemoryRegion *main_mem = g_new(MemoryRegion, 1);
> > +    MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
> > +    int i;
> > +
> > +    /* Initialize SOC */
> > +    object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
> > +                            TYPE_RISCV_HART_ARRAY, &error_abort, NULL);
> > +    object_property_set_str(OBJECT(&s->soc), machine->cpu_type, "cpu-type",
> > +                            &error_abort);
> > +    object_property_set_int(OBJECT(&s->soc), smp_cpus, "num-harts",
> > +                            &error_abort);
> > +    object_property_set_bool(OBJECT(&s->soc), true, "realized",
> > +                            &error_abort);
> > +
> > +    /* register system main memory (actual RAM) */
> > +    memory_region_init_ram(main_mem, NULL, "riscv.spike.ram",
> > +                           machine->ram_size, &error_fatal);
> do you really care about migration? if not then _nomigrate flavor would
> be more suitable.
>
> > +    memory_region_add_subregion(system_memory, memmap[SPIKE_DRAM].base,
> > +        main_mem);
> > +
> > +    /* create device tree */
> > +    create_fdt(s, memmap, machine->ram_size, machine->kernel_cmdline);
> > +
> > +    /* boot rom */
> > +    memory_region_init_rom(mask_rom, NULL, "riscv.spike.mrom",
> > +                           memmap[SPIKE_MROM].size, &error_fatal);
> > +    memory_region_add_subregion(system_memory, memmap[SPIKE_MROM].base,
> > +                                mask_rom);
> > +
> > +    if (machine->kernel_filename) {
> > +        load_kernel(machine->kernel_filename);
> > +    }
> > +
> > +    /* reset vector */
> > +    uint32_t reset_vec[8] = {
> > +        0x00000297,                  /* 1:  auipc  t0, %pcrel_hi(dtb) */
> > +        0x02028593,                  /*     addi   a1, t0, %pcrel_lo(1b) */
> > +        0xf1402573,                  /*     csrr   a0, mhartid  */
> > +#if defined(TARGET_RISCV32)
> > +        0x0182a283,                  /*     lw     t0, 24(t0) */
> > +#elif defined(TARGET_RISCV64)
> > +        0x0182b283,                  /*     ld     t0, 24(t0) */
> > +#endif
> > +        0x00028067,                  /*     jr     t0 */
> > +        0x00000000,
> > +        memmap[SPIKE_DRAM].base,     /* start: .dword DRAM_BASE */
> > +        0x00000000,
> > +                                     /* dtb: */
> > +    };
> > +
> > +    /* copy in the reset vector in little_endian byte order */
> > +    for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
> > +        reset_vec[i] = cpu_to_le32(reset_vec[i]);
> > +    }
> > +    rom_add_blob_fixed_as("mrom.reset", reset_vec, sizeof(reset_vec),
> > +                          memmap[SPIKE_MROM].base, &address_space_memory);
> > +
> > +    /* copy in the device tree */
> > +    if (fdt_pack(s->fdt) || fdt_totalsize(s->fdt) >
> > +            memmap[SPIKE_MROM].size - sizeof(reset_vec)) {
> > +        error_report("not enough space to store device-tree");
> > +        exit(1);
> > +    }
> > +    qemu_fdt_dumpdtb(s->fdt, fdt_totalsize(s->fdt));
> > +    rom_add_blob_fixed_as("mrom.fdt", s->fdt, fdt_totalsize(s->fdt),
> > +                          memmap[SPIKE_MROM].base + sizeof(reset_vec),
> > +                          &address_space_memory);
> > +
> > +    /* initialize HTIF using symbols found in load_kernel */
> > +    htif_mm_init(system_memory, mask_rom, &s->soc.harts[0].env, serial_hd(0));
> > +
> > +    /* Core Local Interruptor (timer and IPI) */
> > +    sifive_clint_create(memmap[SPIKE_CLINT].base, memmap[SPIKE_CLINT].size,
> > +        smp_cpus, SIFIVE_SIP_BASE, SIFIVE_TIMECMP_BASE, SIFIVE_TIME_BASE);
> > +}
> >
> >  static void spike_v1_10_0_board_init(MachineState *machine)
> >  {
> > @@ -172,6 +255,12 @@ static void spike_v1_10_0_board_init(MachineState *machine)
> >      MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
> >      int i;
> >
> > +    if (!qtest_enabled()) {
> > +        info_report("The Spike v1.10.0 machine has been depreceated. "
> > +                    "Please use the deneric spike machine and specify the ISA "
> > +                    "versions using -cpu.");
> > +    }
> Did you mean deprecated in sense that machines will be removed in 2 releases
> according to QEMU's deprecation policy?

That is the plan.

Alistair

>
> > +
> >      /* Initialize SOC */
> >      object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
> >                              TYPE_RISCV_HART_ARRAY, &error_abort, NULL);
> > @@ -254,6 +343,12 @@ static void spike_v1_09_1_board_init(MachineState *machine)
> >      MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
> >      int i;
> >
> > +    if (!qtest_enabled()) {
> > +        info_report("The Spike v1.09.1 machine has been depreceated. "
> > +                    "Please use the deneric spike machine and specify the ISA "
> > +                    "versions using -cpu.");
> > +    }
> > +
> >      /* Initialize SOC */
> >      object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
> >                              TYPE_RISCV_HART_ARRAY, &error_abort, NULL);
> > @@ -359,8 +454,17 @@ static void spike_v1_10_0_machine_init(MachineClass *mc)
> >      mc->desc = "RISC-V Spike Board (Privileged ISA v1.10)";
> >      mc->init = spike_v1_10_0_board_init;
> >      mc->max_cpus = 1;
> > +}
> > +
> > +static void spike_machine_init(MachineClass *mc)
> > +{
> > +    mc->desc = "RISC-V Spike Board";
> > +    mc->init = spike_board_init;
> > +    mc->max_cpus = 1;
> >      mc->is_default = 1;
> > +    mc->default_cpu_type = SPIKE_V1_10_0_CPU;
> >  }
> >
> >  DEFINE_MACHINE("spike_v1.9.1", spike_v1_09_1_machine_init)
> >  DEFINE_MACHINE("spike_v1.10", spike_v1_10_0_machine_init)
> > +DEFINE_MACHINE("spike", spike_machine_init)
>

Re: [Qemu-devel] [PATCH for 4.1 v3 6/6] riscv: Add a generic spike machine

[Alistair Francis]

On Thu, Apr 11, 2019 at 5:06 AM Igor Mammedov <imammedo@redhat.com> wrote:
>
> On Wed, 10 Apr 2019 23:11:00 +0000
> Alistair Francis <Alistair.Francis@wdc.com> wrote:
>
> > Add a generic spike machine (not tied to a version) and deprecate the
> > spike mahines that are tied to a specific version. As we can now specify
> > the CPU via the command line we no londer need specific versions of the
> > spike machines.
> >
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
>
> For cpu and initial RAM related parts:
>
> Acked-by: Igor Mammedov <imammedo@redhat.com>
>
> a couple of questions below.
> > ---
> >  hw/riscv/spike.c | 106 ++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 105 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
> > index 2a000a5800..9d3f7cec4d 100644
> > --- a/hw/riscv/spike.c
> > +++ b/hw/riscv/spike.c
> > @@ -39,6 +39,7 @@
> >  #include "chardev/char.h"
> >  #include "sysemu/arch_init.h"
> >  #include "sysemu/device_tree.h"
> > +#include "sysemu/qtest.h"
> >  #include "exec/address-spaces.h"
> >  #include "elf.h"
> >
> > @@ -160,7 +161,89 @@ static void create_fdt(SpikeState *s, const struct MemmapEntry *memmap,
> >          qemu_fdt_add_subnode(fdt, "/chosen");
> >          qemu_fdt_setprop_string(fdt, "/chosen", "bootargs", cmdline);
> >      }
> > - }
> > +}
> > +
> > +static void spike_board_init(MachineState *machine)
> > +{
> > +    const struct MemmapEntry *memmap = spike_memmap;
> > +
> > +    SpikeState *s = g_new0(SpikeState, 1);
> > +    MemoryRegion *system_memory = get_system_memory();
> > +    MemoryRegion *main_mem = g_new(MemoryRegion, 1);
> > +    MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
> > +    int i;
> > +
> > +    /* Initialize SOC */
> > +    object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
> > +                            TYPE_RISCV_HART_ARRAY, &error_abort, NULL);
> > +    object_property_set_str(OBJECT(&s->soc), machine->cpu_type, "cpu-type",
> > +                            &error_abort);
> > +    object_property_set_int(OBJECT(&s->soc), smp_cpus, "num-harts",
> > +                            &error_abort);
> > +    object_property_set_bool(OBJECT(&s->soc), true, "realized",
> > +                            &error_abort);
> > +
> > +    /* register system main memory (actual RAM) */
> > +    memory_region_init_ram(main_mem, NULL, "riscv.spike.ram",
> > +                           machine->ram_size, &error_fatal);
> do you really care about migration? if not then _nomigrate flavor would
> be more suitable.
>
> > +    memory_region_add_subregion(system_memory, memmap[SPIKE_DRAM].base,
> > +        main_mem);
> > +
> > +    /* create device tree */
> > +    create_fdt(s, memmap, machine->ram_size, machine->kernel_cmdline);
> > +
> > +    /* boot rom */
> > +    memory_region_init_rom(mask_rom, NULL, "riscv.spike.mrom",
> > +                           memmap[SPIKE_MROM].size, &error_fatal);
> > +    memory_region_add_subregion(system_memory, memmap[SPIKE_MROM].base,
> > +                                mask_rom);
> > +
> > +    if (machine->kernel_filename) {
> > +        load_kernel(machine->kernel_filename);
> > +    }
> > +
> > +    /* reset vector */
> > +    uint32_t reset_vec[8] = {
> > +        0x00000297,                  /* 1:  auipc  t0, %pcrel_hi(dtb) */
> > +        0x02028593,                  /*     addi   a1, t0, %pcrel_lo(1b) */
> > +        0xf1402573,                  /*     csrr   a0, mhartid  */
> > +#if defined(TARGET_RISCV32)
> > +        0x0182a283,                  /*     lw     t0, 24(t0) */
> > +#elif defined(TARGET_RISCV64)
> > +        0x0182b283,                  /*     ld     t0, 24(t0) */
> > +#endif
> > +        0x00028067,                  /*     jr     t0 */
> > +        0x00000000,
> > +        memmap[SPIKE_DRAM].base,     /* start: .dword DRAM_BASE */
> > +        0x00000000,
> > +                                     /* dtb: */
> > +    };
> > +
> > +    /* copy in the reset vector in little_endian byte order */
> > +    for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
> > +        reset_vec[i] = cpu_to_le32(reset_vec[i]);
> > +    }
> > +    rom_add_blob_fixed_as("mrom.reset", reset_vec, sizeof(reset_vec),
> > +                          memmap[SPIKE_MROM].base, &address_space_memory);
> > +
> > +    /* copy in the device tree */
> > +    if (fdt_pack(s->fdt) || fdt_totalsize(s->fdt) >
> > +            memmap[SPIKE_MROM].size - sizeof(reset_vec)) {
> > +        error_report("not enough space to store device-tree");
> > +        exit(1);
> > +    }
> > +    qemu_fdt_dumpdtb(s->fdt, fdt_totalsize(s->fdt));
> > +    rom_add_blob_fixed_as("mrom.fdt", s->fdt, fdt_totalsize(s->fdt),
> > +                          memmap[SPIKE_MROM].base + sizeof(reset_vec),
> > +                          &address_space_memory);
> > +
> > +    /* initialize HTIF using symbols found in load_kernel */
> > +    htif_mm_init(system_memory, mask_rom, &s->soc.harts[0].env, serial_hd(0));
> > +
> > +    /* Core Local Interruptor (timer and IPI) */
> > +    sifive_clint_create(memmap[SPIKE_CLINT].base, memmap[SPIKE_CLINT].size,
> > +        smp_cpus, SIFIVE_SIP_BASE, SIFIVE_TIMECMP_BASE, SIFIVE_TIME_BASE);
> > +}
> >
> >  static void spike_v1_10_0_board_init(MachineState *machine)
> >  {
> > @@ -172,6 +255,12 @@ static void spike_v1_10_0_board_init(MachineState *machine)
> >      MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
> >      int i;
> >
> > +    if (!qtest_enabled()) {
> > +        info_report("The Spike v1.10.0 machine has been depreceated. "
> > +                    "Please use the deneric spike machine and specify the ISA "
> > +                    "versions using -cpu.");
> > +    }
> Did you mean deprecated in sense that machines will be removed in 2 releases
> according to QEMU's deprecation policy?

That is the plan.

Alistair

>
> > +
> >      /* Initialize SOC */
> >      object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
> >                              TYPE_RISCV_HART_ARRAY, &error_abort, NULL);
> > @@ -254,6 +343,12 @@ static void spike_v1_09_1_board_init(MachineState *machine)
> >      MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
> >      int i;
> >
> > +    if (!qtest_enabled()) {
> > +        info_report("The Spike v1.09.1 machine has been depreceated. "
> > +                    "Please use the deneric spike machine and specify the ISA "
> > +                    "versions using -cpu.");
> > +    }
> > +
> >      /* Initialize SOC */
> >      object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
> >                              TYPE_RISCV_HART_ARRAY, &error_abort, NULL);
> > @@ -359,8 +454,17 @@ static void spike_v1_10_0_machine_init(MachineClass *mc)
> >      mc->desc = "RISC-V Spike Board (Privileged ISA v1.10)";
> >      mc->init = spike_v1_10_0_board_init;
> >      mc->max_cpus = 1;
> > +}
> > +
> > +static void spike_machine_init(MachineClass *mc)
> > +{
> > +    mc->desc = "RISC-V Spike Board";
> > +    mc->init = spike_board_init;
> > +    mc->max_cpus = 1;
> >      mc->is_default = 1;
> > +    mc->default_cpu_type = SPIKE_V1_10_0_CPU;
> >  }
> >
> >  DEFINE_MACHINE("spike_v1.9.1", spike_v1_09_1_machine_init)
> >  DEFINE_MACHINE("spike_v1.10", spike_v1_10_0_machine_init)
> > +DEFINE_MACHINE("spike", spike_machine_init)
>

Re: [Qemu-devel] [PATCH for 4.1 v3 6/6] riscv: Add a generic spike machine

[Igor Mammedov]

On Thu, 11 Apr 2019 13:34:16 -0700
Alistair Francis <alistair23@gmail.com> wrote:

> On Thu, Apr 11, 2019 at 5:06 AM Igor Mammedov <imammedo@redhat.com> wrote:
> >
> > On Wed, 10 Apr 2019 23:11:00 +0000
> > Alistair Francis <Alistair.Francis@wdc.com> wrote:
> >  
> > > Add a generic spike machine (not tied to a version) and deprecate the
> > > spike mahines that are tied to a specific version. As we can now specify
> > > the CPU via the command line we no londer need specific versions of the
> > > spike machines.
> > >
> > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>  
> >
> > For cpu and initial RAM related parts:
> >
> > Acked-by: Igor Mammedov <imammedo@redhat.com>
> >
> > a couple of questions below.  
> > > ---
> > >  hw/riscv/spike.c | 106 ++++++++++++++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 105 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
> > > index 2a000a5800..9d3f7cec4d 100644
> > > --- a/hw/riscv/spike.c
> > > +++ b/hw/riscv/spike.c
> > > @@ -39,6 +39,7 @@
> > >  #include "chardev/char.h"
> > >  #include "sysemu/arch_init.h"
> > >  #include "sysemu/device_tree.h"
> > > +#include "sysemu/qtest.h"
> > >  #include "exec/address-spaces.h"
> > >  #include "elf.h"
> > >
> > > @@ -160,7 +161,89 @@ static void create_fdt(SpikeState *s, const struct MemmapEntry *memmap,
> > >          qemu_fdt_add_subnode(fdt, "/chosen");
> > >          qemu_fdt_setprop_string(fdt, "/chosen", "bootargs", cmdline);
> > >      }
> > > - }
> > > +}
> > > +
> > > +static void spike_board_init(MachineState *machine)
> > > +{
> > > +    const struct MemmapEntry *memmap = spike_memmap;
> > > +
> > > +    SpikeState *s = g_new0(SpikeState, 1);
> > > +    MemoryRegion *system_memory = get_system_memory();
> > > +    MemoryRegion *main_mem = g_new(MemoryRegion, 1);
> > > +    MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
> > > +    int i;
> > > +
> > > +    /* Initialize SOC */
> > > +    object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
> > > +                            TYPE_RISCV_HART_ARRAY, &error_abort, NULL);
> > > +    object_property_set_str(OBJECT(&s->soc), machine->cpu_type, "cpu-type",
> > > +                            &error_abort);
> > > +    object_property_set_int(OBJECT(&s->soc), smp_cpus, "num-harts",
> > > +                            &error_abort);
> > > +    object_property_set_bool(OBJECT(&s->soc), true, "realized",
> > > +                            &error_abort);
> > > +
> > > +    /* register system main memory (actual RAM) */
> > > +    memory_region_init_ram(main_mem, NULL, "riscv.spike.ram",
> > > +                           machine->ram_size, &error_fatal);  
> > do you really care about migration? if not then _nomigrate flavor would
> > be more suitable.
> >  
> > > +    memory_region_add_subregion(system_memory, memmap[SPIKE_DRAM].base,
> > > +        main_mem);
> > > +
> > > +    /* create device tree */
> > > +    create_fdt(s, memmap, machine->ram_size, machine->kernel_cmdline);
> > > +
> > > +    /* boot rom */
> > > +    memory_region_init_rom(mask_rom, NULL, "riscv.spike.mrom",
> > > +                           memmap[SPIKE_MROM].size, &error_fatal);
> > > +    memory_region_add_subregion(system_memory, memmap[SPIKE_MROM].base,
> > > +                                mask_rom);
> > > +
> > > +    if (machine->kernel_filename) {
> > > +        load_kernel(machine->kernel_filename);
> > > +    }
> > > +
> > > +    /* reset vector */
> > > +    uint32_t reset_vec[8] = {
> > > +        0x00000297,                  /* 1:  auipc  t0, %pcrel_hi(dtb) */
> > > +        0x02028593,                  /*     addi   a1, t0, %pcrel_lo(1b) */
> > > +        0xf1402573,                  /*     csrr   a0, mhartid  */
> > > +#if defined(TARGET_RISCV32)
> > > +        0x0182a283,                  /*     lw     t0, 24(t0) */
> > > +#elif defined(TARGET_RISCV64)
> > > +        0x0182b283,                  /*     ld     t0, 24(t0) */
> > > +#endif
> > > +        0x00028067,                  /*     jr     t0 */
> > > +        0x00000000,
> > > +        memmap[SPIKE_DRAM].base,     /* start: .dword DRAM_BASE */
> > > +        0x00000000,
> > > +                                     /* dtb: */
> > > +    };
> > > +
> > > +    /* copy in the reset vector in little_endian byte order */
> > > +    for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
> > > +        reset_vec[i] = cpu_to_le32(reset_vec[i]);
> > > +    }
> > > +    rom_add_blob_fixed_as("mrom.reset", reset_vec, sizeof(reset_vec),
> > > +                          memmap[SPIKE_MROM].base, &address_space_memory);
> > > +
> > > +    /* copy in the device tree */
> > > +    if (fdt_pack(s->fdt) || fdt_totalsize(s->fdt) >
> > > +            memmap[SPIKE_MROM].size - sizeof(reset_vec)) {
> > > +        error_report("not enough space to store device-tree");
> > > +        exit(1);
> > > +    }
> > > +    qemu_fdt_dumpdtb(s->fdt, fdt_totalsize(s->fdt));
> > > +    rom_add_blob_fixed_as("mrom.fdt", s->fdt, fdt_totalsize(s->fdt),
> > > +                          memmap[SPIKE_MROM].base + sizeof(reset_vec),
> > > +                          &address_space_memory);
> > > +
> > > +    /* initialize HTIF using symbols found in load_kernel */
> > > +    htif_mm_init(system_memory, mask_rom, &s->soc.harts[0].env, serial_hd(0));
> > > +
> > > +    /* Core Local Interruptor (timer and IPI) */
> > > +    sifive_clint_create(memmap[SPIKE_CLINT].base, memmap[SPIKE_CLINT].size,
> > > +        smp_cpus, SIFIVE_SIP_BASE, SIFIVE_TIMECMP_BASE, SIFIVE_TIME_BASE);
> > > +}
> > >
> > >  static void spike_v1_10_0_board_init(MachineState *machine)
> > >  {
> > > @@ -172,6 +255,12 @@ static void spike_v1_10_0_board_init(MachineState *machine)
> > >      MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
> > >      int i;
> > >
> > > +    if (!qtest_enabled()) {
> > > +        info_report("The Spike v1.10.0 machine has been depreceated. "
> > > +                    "Please use the deneric spike machine and specify the ISA "
> > > +                    "versions using -cpu.");
> > > +    }  
> > Did you mean deprecated in sense that machines will be removed in 2 releases
> > according to QEMU's deprecation policy?  
> 
> That is the plan.

In this case update qemu-deprecated.texi as part of the patch

> 
> Alistair
> 
> >  
> > > +
> > >      /* Initialize SOC */
> > >      object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
> > >                              TYPE_RISCV_HART_ARRAY, &error_abort, NULL);
> > > @@ -254,6 +343,12 @@ static void spike_v1_09_1_board_init(MachineState *machine)
> > >      MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
> > >      int i;
> > >
> > > +    if (!qtest_enabled()) {
> > > +        info_report("The Spike v1.09.1 machine has been depreceated. "
> > > +                    "Please use the deneric spike machine and specify the ISA "
> > > +                    "versions using -cpu.");
> > > +    }
> > > +
> > >      /* Initialize SOC */
> > >      object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
> > >                              TYPE_RISCV_HART_ARRAY, &error_abort, NULL);
> > > @@ -359,8 +454,17 @@ static void spike_v1_10_0_machine_init(MachineClass *mc)
> > >      mc->desc = "RISC-V Spike Board (Privileged ISA v1.10)";
> > >      mc->init = spike_v1_10_0_board_init;
> > >      mc->max_cpus = 1;
> > > +}
> > > +
> > > +static void spike_machine_init(MachineClass *mc)
> > > +{
> > > +    mc->desc = "RISC-V Spike Board";
> > > +    mc->init = spike_board_init;
> > > +    mc->max_cpus = 1;
> > >      mc->is_default = 1;
> > > +    mc->default_cpu_type = SPIKE_V1_10_0_CPU;
> > >  }
> > >
> > >  DEFINE_MACHINE("spike_v1.9.1", spike_v1_09_1_machine_init)
> > >  DEFINE_MACHINE("spike_v1.10", spike_v1_10_0_machine_init)
> > > +DEFINE_MACHINE("spike", spike_machine_init)  
> >  

Re: [Qemu-devel] [PATCH for 4.1 v3 6/6] riscv: Add a generic spike machine

[Igor Mammedov]

On Thu, 11 Apr 2019 13:34:16 -0700
Alistair Francis <alistair23@gmail.com> wrote:

> On Thu, Apr 11, 2019 at 5:06 AM Igor Mammedov <imammedo@redhat.com> wrote:
> >
> > On Wed, 10 Apr 2019 23:11:00 +0000
> > Alistair Francis <Alistair.Francis@wdc.com> wrote:
> >  
> > > Add a generic spike machine (not tied to a version) and deprecate the
> > > spike mahines that are tied to a specific version. As we can now specify
> > > the CPU via the command line we no londer need specific versions of the
> > > spike machines.
> > >
> > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>  
> >
> > For cpu and initial RAM related parts:
> >
> > Acked-by: Igor Mammedov <imammedo@redhat.com>
> >
> > a couple of questions below.  
> > > ---
> > >  hw/riscv/spike.c | 106 ++++++++++++++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 105 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
> > > index 2a000a5800..9d3f7cec4d 100644
> > > --- a/hw/riscv/spike.c
> > > +++ b/hw/riscv/spike.c
> > > @@ -39,6 +39,7 @@
> > >  #include "chardev/char.h"
> > >  #include "sysemu/arch_init.h"
> > >  #include "sysemu/device_tree.h"
> > > +#include "sysemu/qtest.h"
> > >  #include "exec/address-spaces.h"
> > >  #include "elf.h"
> > >
> > > @@ -160,7 +161,89 @@ static void create_fdt(SpikeState *s, const struct MemmapEntry *memmap,
> > >          qemu_fdt_add_subnode(fdt, "/chosen");
> > >          qemu_fdt_setprop_string(fdt, "/chosen", "bootargs", cmdline);
> > >      }
> > > - }
> > > +}
> > > +
> > > +static void spike_board_init(MachineState *machine)
> > > +{
> > > +    const struct MemmapEntry *memmap = spike_memmap;
> > > +
> > > +    SpikeState *s = g_new0(SpikeState, 1);
> > > +    MemoryRegion *system_memory = get_system_memory();
> > > +    MemoryRegion *main_mem = g_new(MemoryRegion, 1);
> > > +    MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
> > > +    int i;
> > > +
> > > +    /* Initialize SOC */
> > > +    object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
> > > +                            TYPE_RISCV_HART_ARRAY, &error_abort, NULL);
> > > +    object_property_set_str(OBJECT(&s->soc), machine->cpu_type, "cpu-type",
> > > +                            &error_abort);
> > > +    object_property_set_int(OBJECT(&s->soc), smp_cpus, "num-harts",
> > > +                            &error_abort);
> > > +    object_property_set_bool(OBJECT(&s->soc), true, "realized",
> > > +                            &error_abort);
> > > +
> > > +    /* register system main memory (actual RAM) */
> > > +    memory_region_init_ram(main_mem, NULL, "riscv.spike.ram",
> > > +                           machine->ram_size, &error_fatal);  
> > do you really care about migration? if not then _nomigrate flavor would
> > be more suitable.
> >  
> > > +    memory_region_add_subregion(system_memory, memmap[SPIKE_DRAM].base,
> > > +        main_mem);
> > > +
> > > +    /* create device tree */
> > > +    create_fdt(s, memmap, machine->ram_size, machine->kernel_cmdline);
> > > +
> > > +    /* boot rom */
> > > +    memory_region_init_rom(mask_rom, NULL, "riscv.spike.mrom",
> > > +                           memmap[SPIKE_MROM].size, &error_fatal);
> > > +    memory_region_add_subregion(system_memory, memmap[SPIKE_MROM].base,
> > > +                                mask_rom);
> > > +
> > > +    if (machine->kernel_filename) {
> > > +        load_kernel(machine->kernel_filename);
> > > +    }
> > > +
> > > +    /* reset vector */
> > > +    uint32_t reset_vec[8] = {
> > > +        0x00000297,                  /* 1:  auipc  t0, %pcrel_hi(dtb) */
> > > +        0x02028593,                  /*     addi   a1, t0, %pcrel_lo(1b) */
> > > +        0xf1402573,                  /*     csrr   a0, mhartid  */
> > > +#if defined(TARGET_RISCV32)
> > > +        0x0182a283,                  /*     lw     t0, 24(t0) */
> > > +#elif defined(TARGET_RISCV64)
> > > +        0x0182b283,                  /*     ld     t0, 24(t0) */
> > > +#endif
> > > +        0x00028067,                  /*     jr     t0 */
> > > +        0x00000000,
> > > +        memmap[SPIKE_DRAM].base,     /* start: .dword DRAM_BASE */
> > > +        0x00000000,
> > > +                                     /* dtb: */
> > > +    };
> > > +
> > > +    /* copy in the reset vector in little_endian byte order */
> > > +    for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
> > > +        reset_vec[i] = cpu_to_le32(reset_vec[i]);
> > > +    }
> > > +    rom_add_blob_fixed_as("mrom.reset", reset_vec, sizeof(reset_vec),
> > > +                          memmap[SPIKE_MROM].base, &address_space_memory);
> > > +
> > > +    /* copy in the device tree */
> > > +    if (fdt_pack(s->fdt) || fdt_totalsize(s->fdt) >
> > > +            memmap[SPIKE_MROM].size - sizeof(reset_vec)) {
> > > +        error_report("not enough space to store device-tree");
> > > +        exit(1);
> > > +    }
> > > +    qemu_fdt_dumpdtb(s->fdt, fdt_totalsize(s->fdt));
> > > +    rom_add_blob_fixed_as("mrom.fdt", s->fdt, fdt_totalsize(s->fdt),
> > > +                          memmap[SPIKE_MROM].base + sizeof(reset_vec),
> > > +                          &address_space_memory);
> > > +
> > > +    /* initialize HTIF using symbols found in load_kernel */
> > > +    htif_mm_init(system_memory, mask_rom, &s->soc.harts[0].env, serial_hd(0));
> > > +
> > > +    /* Core Local Interruptor (timer and IPI) */
> > > +    sifive_clint_create(memmap[SPIKE_CLINT].base, memmap[SPIKE_CLINT].size,
> > > +        smp_cpus, SIFIVE_SIP_BASE, SIFIVE_TIMECMP_BASE, SIFIVE_TIME_BASE);
> > > +}
> > >
> > >  static void spike_v1_10_0_board_init(MachineState *machine)
> > >  {
> > > @@ -172,6 +255,12 @@ static void spike_v1_10_0_board_init(MachineState *machine)
> > >      MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
> > >      int i;
> > >
> > > +    if (!qtest_enabled()) {
> > > +        info_report("The Spike v1.10.0 machine has been depreceated. "
> > > +                    "Please use the deneric spike machine and specify the ISA "
> > > +                    "versions using -cpu.");
> > > +    }  
> > Did you mean deprecated in sense that machines will be removed in 2 releases
> > according to QEMU's deprecation policy?  
> 
> That is the plan.

In this case update qemu-deprecated.texi as part of the patch

> 
> Alistair
> 
> >  
> > > +
> > >      /* Initialize SOC */
> > >      object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
> > >                              TYPE_RISCV_HART_ARRAY, &error_abort, NULL);
> > > @@ -254,6 +343,12 @@ static void spike_v1_09_1_board_init(MachineState *machine)
> > >      MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
> > >      int i;
> > >
> > > +    if (!qtest_enabled()) {
> > > +        info_report("The Spike v1.09.1 machine has been depreceated. "
> > > +                    "Please use the deneric spike machine and specify the ISA "
> > > +                    "versions using -cpu.");
> > > +    }
> > > +
> > >      /* Initialize SOC */
> > >      object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
> > >                              TYPE_RISCV_HART_ARRAY, &error_abort, NULL);
> > > @@ -359,8 +454,17 @@ static void spike_v1_10_0_machine_init(MachineClass *mc)
> > >      mc->desc = "RISC-V Spike Board (Privileged ISA v1.10)";
> > >      mc->init = spike_v1_10_0_board_init;
> > >      mc->max_cpus = 1;
> > > +}
> > > +
> > > +static void spike_machine_init(MachineClass *mc)
> > > +{
> > > +    mc->desc = "RISC-V Spike Board";
> > > +    mc->init = spike_board_init;
> > > +    mc->max_cpus = 1;
> > >      mc->is_default = 1;
> > > +    mc->default_cpu_type = SPIKE_V1_10_0_CPU;
> > >  }
> > >
> > >  DEFINE_MACHINE("spike_v1.9.1", spike_v1_09_1_machine_init)
> > >  DEFINE_MACHINE("spike_v1.10", spike_v1_10_0_machine_init)
> > > +DEFINE_MACHINE("spike", spike_machine_init)  
> >