* [PATCH 0/2] target/riscv/cpu: fix sifive_u 32/64bits boot in riscv-to-apply.next
@ 2023-01-10 20:14 Daniel Henrique Barboza
2023-01-10 20:14 ` [PATCH 1/2] target/riscv/cpu: set cpu->cfg in register_cpu_props() Daniel Henrique Barboza
` (3 more replies)
0 siblings, 4 replies; 13+ messages in thread
From: Daniel Henrique Barboza @ 2023-01-10 20:14 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-riscv, alistair.francis, richard.henderson, Daniel Henrique Barboza
Hi,
I found this bug when testing my avocado changes in riscv-to-apply.next.
The sifive_u board, both 32 and 64 bits, stopped booting OpenSBI. The
guest hangs indefinitely.
Git bisect points that this patch broke things:
8c3f35d25e7e98655c609b6c1e9f103b9240f8f8 is the first bad commit
commit 8c3f35d25e7e98655c609b6c1e9f103b9240f8f8
Author: Weiwei Li <liweiwei@iscas.ac.cn>
Date: Wed Dec 28 14:20:21 2022 +0800
target/riscv: add support for Zca extension
Modify the check for C extension to Zca (C implies Zca)
(https://github.com/alistair23/qemu/commit/8c3f35d25e7e98655c609b6c1e9f103b9240f8f8)
But this patch per se isn't doing anything wrong. The root of the
problem is that this patch makes assumptions based on the previous
patch:
commit a2b409aa6cadc1ed9715e1ab916ddd3dade0ba85
Author: Weiwei Li <liweiwei@iscas.ac.cn>
Date: Wed Dec 28 14:20:20 2022 +0800
target/riscv: add cfg properties for Zc* extension
(https://github.com/alistair23/qemu/commit/a2b409aa6cadc1ed9715e1ab916ddd3dade0ba85)
Which added a lot of logic and assumptions that are being skipped by all
the SiFive boards because, during riscv_cpu_realize(), we have this
code:
/* If only MISA_EXT is unset for misa, then set it from properties */
if (env->misa_ext == 0) {
uint32_t ext = 0;
(...)
}
In short, we have a lot of code that are being skipped by all SiFive
CPUs because these CPUs are setting a non-zero value in set_misa() in
their respective cpu_init() functions.
It's possible to just hack in and fix the SiFive problem in isolate, but
I believe we can do better and allow all riscv_cpu_realize() to be executed
for all CPUs, regardless of what they've done during their cpu_init().
Daniel Henrique Barboza (2):
target/riscv/cpu: set cpu->cfg in register_cpu_props()
target/riscv/cpu.c: do not skip misa logic in riscv_cpu_realize()
target/riscv/cpu.c | 525 +++++++++++++++++++++++++--------------------
target/riscv/cpu.h | 4 +
2 files changed, 292 insertions(+), 237 deletions(-)
--
2.39.0
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/2] target/riscv/cpu: set cpu->cfg in register_cpu_props()
2023-01-10 20:14 [PATCH 0/2] target/riscv/cpu: fix sifive_u 32/64bits boot in riscv-to-apply.next Daniel Henrique Barboza
@ 2023-01-10 20:14 ` Daniel Henrique Barboza
2023-01-10 22:52 ` Alistair Francis
` (2 more replies)
2023-01-10 20:14 ` [PATCH 2/2] target/riscv/cpu.c: do not skip misa logic in riscv_cpu_realize() Daniel Henrique Barboza
` (2 subsequent siblings)
3 siblings, 3 replies; 13+ messages in thread
From: Daniel Henrique Barboza @ 2023-01-10 20:14 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-riscv, alistair.francis, richard.henderson, Daniel Henrique Barboza
There is an informal contract between the cpu_init() functions and
riscv_cpu_realize(): if cpu->env.misa_ext is zero, assume that the
default settings were loaded via register_cpu_props() and do validations
to set env.misa_ext. If it's not zero, skip this whole process and
assume that the board somehow did everything.
At this moment, all SiFive CPUs are setting a non-zero misa_ext during
their cpu_init() and skipping a good chunk of riscv_cpu_realize().
This causes problems when the code being skipped in riscv_cpu_realize()
contains fixes or assumptions that affects all CPUs, meaning that SiFive
CPUs are missing out.
To allow this code to not be skipped anymore, all the cpu->cfg.ext_* attributes
needs to be set during cpu_init() time. At this moment this is being done in
register_cpu_props(). The SiFive oards are setting their own extensions during
cpu_init() though, meaning that they don't want all the defaults from
register_cpu_props().
Let's move the contract between *_cpu_init() and riscv_cpu_realize() to
register_cpu_props(). Inside this function we'll check if cpu->env.misa_ext
was set and, if that's the case, set all relevant cpu->cfg.ext_*
attributes, and only that. Leave the 'misa_ext' = 0 case as is today,
i.e. loading all the defaults from riscv_cpu_extensions[].
register_cpu_props() can then be called by all the cpu_init() functions,
including the SiFive ones. This will make all CPUs behave more in line
with that riscv_cpu_realize() expects.
Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
target/riscv/cpu.c | 40 ++++++++++++++++++++++++++++++++++++++++
target/riscv/cpu.h | 4 ++++
2 files changed, 44 insertions(+)
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index ee3659cc7e..b8c1edb7c2 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -262,6 +262,7 @@ static void rv64_sifive_u_cpu_init(Object *obj)
{
CPURISCVState *env = &RISCV_CPU(obj)->env;
set_misa(env, MXL_RV64, RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
+ register_cpu_props(DEVICE(obj));
set_priv_version(env, PRIV_VERSION_1_10_0);
}
@@ -271,6 +272,7 @@ static void rv64_sifive_e_cpu_init(Object *obj)
RISCVCPU *cpu = RISCV_CPU(obj);
set_misa(env, MXL_RV64, RVI | RVM | RVA | RVC | RVU);
+ register_cpu_props(DEVICE(obj));
set_priv_version(env, PRIV_VERSION_1_10_0);
cpu->cfg.mmu = false;
}
@@ -305,6 +307,7 @@ static void rv32_sifive_u_cpu_init(Object *obj)
{
CPURISCVState *env = &RISCV_CPU(obj)->env;
set_misa(env, MXL_RV32, RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
+ register_cpu_props(DEVICE(obj));
set_priv_version(env, PRIV_VERSION_1_10_0);
}
@@ -314,6 +317,7 @@ static void rv32_sifive_e_cpu_init(Object *obj)
RISCVCPU *cpu = RISCV_CPU(obj);
set_misa(env, MXL_RV32, RVI | RVM | RVA | RVC | RVU);
+ register_cpu_props(DEVICE(obj));
set_priv_version(env, PRIV_VERSION_1_10_0);
cpu->cfg.mmu = false;
}
@@ -324,6 +328,7 @@ static void rv32_ibex_cpu_init(Object *obj)
RISCVCPU *cpu = RISCV_CPU(obj);
set_misa(env, MXL_RV32, RVI | RVM | RVC | RVU);
+ register_cpu_props(DEVICE(obj));
set_priv_version(env, PRIV_VERSION_1_11_0);
cpu->cfg.mmu = false;
cpu->cfg.epmp = true;
@@ -335,6 +340,7 @@ static void rv32_imafcu_nommu_cpu_init(Object *obj)
RISCVCPU *cpu = RISCV_CPU(obj);
set_misa(env, MXL_RV32, RVI | RVM | RVA | RVF | RVC | RVU);
+ register_cpu_props(DEVICE(obj));
set_priv_version(env, PRIV_VERSION_1_10_0);
cpu->cfg.mmu = false;
}
@@ -1139,10 +1145,44 @@ static Property riscv_cpu_extensions[] = {
DEFINE_PROP_END_OF_LIST(),
};
+/*
+ * Register CPU props based on env.misa_ext. If a non-zero
+ * value was set, register only the required cpu->cfg.ext_*
+ * properties and leave. env.misa_ext = 0 means that we want
+ * all the default properties to be registered.
+ */
static void register_cpu_props(DeviceState *dev)
{
+ RISCVCPU *cpu = RISCV_CPU(OBJECT(dev));
+ uint32_t misa_ext = cpu->env.misa_ext;
Property *prop;
+ /*
+ * If misa_ext is not zero, set cfg properties now to
+ * allow them to be read during riscv_cpu_realize()
+ * later on.
+ */
+ if (cpu->env.misa_ext != 0) {
+ cpu->cfg.ext_i = misa_ext & RVI;
+ cpu->cfg.ext_e = misa_ext & RVE;
+ cpu->cfg.ext_m = misa_ext & RVM;
+ cpu->cfg.ext_a = misa_ext & RVA;
+ cpu->cfg.ext_f = misa_ext & RVF;
+ cpu->cfg.ext_d = misa_ext & RVD;
+ cpu->cfg.ext_v = misa_ext & RVV;
+ cpu->cfg.ext_c = misa_ext & RVC;
+ cpu->cfg.ext_s = misa_ext & RVS;
+ cpu->cfg.ext_u = misa_ext & RVU;
+ cpu->cfg.ext_h = misa_ext & RVH;
+ cpu->cfg.ext_j = misa_ext & RVJ;
+
+ /*
+ * We don't want to set the default riscv_cpu_extensions
+ * in this case.
+ */
+ return;
+ }
+
for (prop = riscv_cpu_extensions; prop && prop->name; prop++) {
qdev_property_add_static(dev, prop);
}
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 0158932dc5..798bd081de 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -63,6 +63,10 @@
#define RV(x) ((target_ulong)1 << (x - 'A'))
+/*
+ * Consider updating register_cpu_props() when adding
+ * new MISA bits here.
+ */
#define RVI RV('I')
#define RVE RV('E') /* E and I are mutually exclusive */
#define RVM RV('M')
--
2.39.0
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/2] target/riscv/cpu.c: do not skip misa logic in riscv_cpu_realize()
2023-01-10 20:14 [PATCH 0/2] target/riscv/cpu: fix sifive_u 32/64bits boot in riscv-to-apply.next Daniel Henrique Barboza
2023-01-10 20:14 ` [PATCH 1/2] target/riscv/cpu: set cpu->cfg in register_cpu_props() Daniel Henrique Barboza
@ 2023-01-10 20:14 ` Daniel Henrique Barboza
2023-01-10 22:53 ` Alistair Francis
2023-01-11 5:56 ` Bin Meng
2023-01-10 22:27 ` [PATCH 0/2] target/riscv/cpu: fix sifive_u 32/64bits boot in riscv-to-apply.next Daniel Henrique Barboza
2023-01-11 5:01 ` Alistair Francis
3 siblings, 2 replies; 13+ messages in thread
From: Daniel Henrique Barboza @ 2023-01-10 20:14 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-riscv, alistair.francis, richard.henderson, Daniel Henrique Barboza
All RISCV CPUs are setting cpu->cfg during their cpu_init() functions,
meaning that there's no reason to skip all the misa validation and setup
if misa_ext was set beforehand - especially since we're setting an
updated value in set_misa() in the end.
Put this code chunk into a new riscv_cpu_validate_set_extensions()
helper and always execute it regardless of what the board set in
env->misa_ext.
This will put more responsibility in how each board is going to init
their attributes and extensions if they're not using the defaults.
It'll also allow realize() to do its job looking only at the extensions
enabled per se, not corner cases that some CPUs might have, and we won't
have to change multiple code paths to fix or change how extensions work.
Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
target/riscv/cpu.c | 485 +++++++++++++++++++++++----------------------
1 file changed, 248 insertions(+), 237 deletions(-)
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index b8c1edb7c2..33ed59a1b6 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -631,6 +631,250 @@ static void riscv_cpu_disas_set_info(CPUState *s, disassemble_info *info)
}
}
+/*
+ * Check consistency between chosen extensions while setting
+ * cpu->cfg accordingly, doing a set_misa() in the end.
+ */
+static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
+{
+ CPURISCVState *env = &cpu->env;
+ uint32_t ext = 0;
+
+ /* Do some ISA extension error checking */
+ if (cpu->cfg.ext_g && !(cpu->cfg.ext_i && cpu->cfg.ext_m &&
+ cpu->cfg.ext_a && cpu->cfg.ext_f &&
+ cpu->cfg.ext_d &&
+ cpu->cfg.ext_icsr && cpu->cfg.ext_ifencei)) {
+ warn_report("Setting G will also set IMAFD_Zicsr_Zifencei");
+ cpu->cfg.ext_i = true;
+ cpu->cfg.ext_m = true;
+ cpu->cfg.ext_a = true;
+ cpu->cfg.ext_f = true;
+ cpu->cfg.ext_d = true;
+ cpu->cfg.ext_icsr = true;
+ cpu->cfg.ext_ifencei = true;
+ }
+
+ if (cpu->cfg.ext_i && cpu->cfg.ext_e) {
+ error_setg(errp,
+ "I and E extensions are incompatible");
+ return;
+ }
+
+ if (!cpu->cfg.ext_i && !cpu->cfg.ext_e) {
+ error_setg(errp,
+ "Either I or E extension must be set");
+ return;
+ }
+
+ if (cpu->cfg.ext_s && !cpu->cfg.ext_u) {
+ error_setg(errp,
+ "Setting S extension without U extension is illegal");
+ return;
+ }
+
+ if (cpu->cfg.ext_h && !cpu->cfg.ext_i) {
+ error_setg(errp,
+ "H depends on an I base integer ISA with 32 x registers");
+ return;
+ }
+
+ if (cpu->cfg.ext_h && !cpu->cfg.ext_s) {
+ error_setg(errp, "H extension implicitly requires S-mode");
+ return;
+ }
+
+ if (cpu->cfg.ext_f && !cpu->cfg.ext_icsr) {
+ error_setg(errp, "F extension requires Zicsr");
+ return;
+ }
+
+ if ((cpu->cfg.ext_zawrs) && !cpu->cfg.ext_a) {
+ error_setg(errp, "Zawrs extension requires A extension");
+ return;
+ }
+
+ if ((cpu->cfg.ext_zfh || cpu->cfg.ext_zfhmin) && !cpu->cfg.ext_f) {
+ error_setg(errp, "Zfh/Zfhmin extensions require F extension");
+ return;
+ }
+
+ if (cpu->cfg.ext_d && !cpu->cfg.ext_f) {
+ error_setg(errp, "D extension requires F extension");
+ return;
+ }
+
+ if (cpu->cfg.ext_v && !cpu->cfg.ext_d) {
+ error_setg(errp, "V extension requires D extension");
+ return;
+ }
+
+ if ((cpu->cfg.ext_zve32f || cpu->cfg.ext_zve64f) && !cpu->cfg.ext_f) {
+ error_setg(errp, "Zve32f/Zve64f extensions require F extension");
+ return;
+ }
+
+ /* Set the ISA extensions, checks should have happened above */
+ if (cpu->cfg.ext_zdinx || cpu->cfg.ext_zhinx ||
+ cpu->cfg.ext_zhinxmin) {
+ cpu->cfg.ext_zfinx = true;
+ }
+
+ if (cpu->cfg.ext_zfinx) {
+ if (!cpu->cfg.ext_icsr) {
+ error_setg(errp, "Zfinx extension requires Zicsr");
+ return;
+ }
+ if (cpu->cfg.ext_f) {
+ error_setg(errp,
+ "Zfinx cannot be supported together with F extension");
+ return;
+ }
+ }
+
+ if (cpu->cfg.ext_c) {
+ cpu->cfg.ext_zca = true;
+ if (cpu->cfg.ext_f && env->misa_mxl_max == MXL_RV32) {
+ cpu->cfg.ext_zcf = true;
+ }
+ if (cpu->cfg.ext_d) {
+ cpu->cfg.ext_zcd = true;
+ }
+ }
+
+ if (env->misa_mxl_max != MXL_RV32 && cpu->cfg.ext_zcf) {
+ error_setg(errp, "Zcf extension is only relevant to RV32");
+ return;
+ }
+
+ if (!cpu->cfg.ext_f && cpu->cfg.ext_zcf) {
+ error_setg(errp, "Zcf extension requires F extension");
+ return;
+ }
+
+ if (!cpu->cfg.ext_d && cpu->cfg.ext_zcd) {
+ error_setg(errp, "Zcd extension requires D extension");
+ return;
+ }
+
+ if ((cpu->cfg.ext_zcf || cpu->cfg.ext_zcd || cpu->cfg.ext_zcb ||
+ cpu->cfg.ext_zcmp || cpu->cfg.ext_zcmt) && !cpu->cfg.ext_zca) {
+ error_setg(errp, "Zcf/Zcd/Zcb/Zcmp/Zcmt extensions require Zca "
+ "extension");
+ return;
+ }
+
+ if (cpu->cfg.ext_zcd && (cpu->cfg.ext_zcmp || cpu->cfg.ext_zcmt)) {
+ error_setg(errp, "Zcmp/Zcmt extensions are incompatible with "
+ "Zcd extension");
+ return;
+ }
+
+ if (cpu->cfg.ext_zcmt && !cpu->cfg.ext_icsr) {
+ error_setg(errp, "Zcmt extension requires Zicsr extension");
+ return;
+ }
+
+ if (cpu->cfg.ext_zk) {
+ cpu->cfg.ext_zkn = true;
+ cpu->cfg.ext_zkr = true;
+ cpu->cfg.ext_zkt = true;
+ }
+
+ if (cpu->cfg.ext_zkn) {
+ cpu->cfg.ext_zbkb = true;
+ cpu->cfg.ext_zbkc = true;
+ cpu->cfg.ext_zbkx = true;
+ cpu->cfg.ext_zkne = true;
+ cpu->cfg.ext_zknd = true;
+ cpu->cfg.ext_zknh = true;
+ }
+
+ if (cpu->cfg.ext_zks) {
+ cpu->cfg.ext_zbkb = true;
+ cpu->cfg.ext_zbkc = true;
+ cpu->cfg.ext_zbkx = true;
+ cpu->cfg.ext_zksed = true;
+ cpu->cfg.ext_zksh = true;
+ }
+
+ if (cpu->cfg.ext_i) {
+ ext |= RVI;
+ }
+ if (cpu->cfg.ext_e) {
+ ext |= RVE;
+ }
+ if (cpu->cfg.ext_m) {
+ ext |= RVM;
+ }
+ if (cpu->cfg.ext_a) {
+ ext |= RVA;
+ }
+ if (cpu->cfg.ext_f) {
+ ext |= RVF;
+ }
+ if (cpu->cfg.ext_d) {
+ ext |= RVD;
+ }
+ if (cpu->cfg.ext_c) {
+ ext |= RVC;
+ }
+ if (cpu->cfg.ext_s) {
+ ext |= RVS;
+ }
+ if (cpu->cfg.ext_u) {
+ ext |= RVU;
+ }
+ if (cpu->cfg.ext_h) {
+ ext |= RVH;
+ }
+ if (cpu->cfg.ext_v) {
+ int vext_version = VEXT_VERSION_1_00_0;
+ ext |= RVV;
+ if (!is_power_of_2(cpu->cfg.vlen)) {
+ error_setg(errp,
+ "Vector extension VLEN must be power of 2");
+ return;
+ }
+ if (cpu->cfg.vlen > RV_VLEN_MAX || cpu->cfg.vlen < 128) {
+ error_setg(errp,
+ "Vector extension implementation only supports VLEN "
+ "in the range [128, %d]", RV_VLEN_MAX);
+ return;
+ }
+ if (!is_power_of_2(cpu->cfg.elen)) {
+ error_setg(errp,
+ "Vector extension ELEN must be power of 2");
+ return;
+ }
+ if (cpu->cfg.elen > 64 || cpu->cfg.elen < 8) {
+ error_setg(errp,
+ "Vector extension implementation only supports ELEN "
+ "in the range [8, 64]");
+ return;
+ }
+ if (cpu->cfg.vext_spec) {
+ if (!g_strcmp0(cpu->cfg.vext_spec, "v1.0")) {
+ vext_version = VEXT_VERSION_1_00_0;
+ } else {
+ error_setg(errp,
+ "Unsupported vector spec version '%s'",
+ cpu->cfg.vext_spec);
+ return;
+ }
+ } else {
+ qemu_log("vector version is not specified, "
+ "use the default value v1.0\n");
+ }
+ set_vext_version(env, vext_version);
+ }
+ if (cpu->cfg.ext_j) {
+ ext |= RVJ;
+ }
+
+ set_misa(env, env->misa_mxl, ext);
+}
+
static void riscv_cpu_realize(DeviceState *dev, Error **errp)
{
CPUState *cs = CPU(dev);
@@ -726,243 +970,10 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
}
assert(env->misa_mxl_max == env->misa_mxl);
- /* If only MISA_EXT is unset for misa, then set it from properties */
- if (env->misa_ext == 0) {
- uint32_t ext = 0;
-
- /* Do some ISA extension error checking */
- if (cpu->cfg.ext_g && !(cpu->cfg.ext_i && cpu->cfg.ext_m &&
- cpu->cfg.ext_a && cpu->cfg.ext_f &&
- cpu->cfg.ext_d &&
- cpu->cfg.ext_icsr && cpu->cfg.ext_ifencei)) {
- warn_report("Setting G will also set IMAFD_Zicsr_Zifencei");
- cpu->cfg.ext_i = true;
- cpu->cfg.ext_m = true;
- cpu->cfg.ext_a = true;
- cpu->cfg.ext_f = true;
- cpu->cfg.ext_d = true;
- cpu->cfg.ext_icsr = true;
- cpu->cfg.ext_ifencei = true;
- }
-
- if (cpu->cfg.ext_i && cpu->cfg.ext_e) {
- error_setg(errp,
- "I and E extensions are incompatible");
- return;
- }
-
- if (!cpu->cfg.ext_i && !cpu->cfg.ext_e) {
- error_setg(errp,
- "Either I or E extension must be set");
- return;
- }
-
- if (cpu->cfg.ext_s && !cpu->cfg.ext_u) {
- error_setg(errp,
- "Setting S extension without U extension is illegal");
- return;
- }
-
- if (cpu->cfg.ext_h && !cpu->cfg.ext_i) {
- error_setg(errp,
- "H depends on an I base integer ISA with 32 x registers");
- return;
- }
-
- if (cpu->cfg.ext_h && !cpu->cfg.ext_s) {
- error_setg(errp, "H extension implicitly requires S-mode");
- return;
- }
-
- if (cpu->cfg.ext_f && !cpu->cfg.ext_icsr) {
- error_setg(errp, "F extension requires Zicsr");
- return;
- }
-
- if ((cpu->cfg.ext_zawrs) && !cpu->cfg.ext_a) {
- error_setg(errp, "Zawrs extension requires A extension");
- return;
- }
-
- if ((cpu->cfg.ext_zfh || cpu->cfg.ext_zfhmin) && !cpu->cfg.ext_f) {
- error_setg(errp, "Zfh/Zfhmin extensions require F extension");
- return;
- }
-
- if (cpu->cfg.ext_d && !cpu->cfg.ext_f) {
- error_setg(errp, "D extension requires F extension");
- return;
- }
-
- if (cpu->cfg.ext_v && !cpu->cfg.ext_d) {
- error_setg(errp, "V extension requires D extension");
- return;
- }
-
- if ((cpu->cfg.ext_zve32f || cpu->cfg.ext_zve64f) && !cpu->cfg.ext_f) {
- error_setg(errp, "Zve32f/Zve64f extensions require F extension");
- return;
- }
-
- /* Set the ISA extensions, checks should have happened above */
- if (cpu->cfg.ext_zdinx || cpu->cfg.ext_zhinx ||
- cpu->cfg.ext_zhinxmin) {
- cpu->cfg.ext_zfinx = true;
- }
-
- if (cpu->cfg.ext_zfinx) {
- if (!cpu->cfg.ext_icsr) {
- error_setg(errp, "Zfinx extension requires Zicsr");
- return;
- }
- if (cpu->cfg.ext_f) {
- error_setg(errp,
- "Zfinx cannot be supported together with F extension");
- return;
- }
- }
-
- if (cpu->cfg.ext_c) {
- cpu->cfg.ext_zca = true;
- if (cpu->cfg.ext_f && env->misa_mxl_max == MXL_RV32) {
- cpu->cfg.ext_zcf = true;
- }
- if (cpu->cfg.ext_d) {
- cpu->cfg.ext_zcd = true;
- }
- }
-
- if (env->misa_mxl_max != MXL_RV32 && cpu->cfg.ext_zcf) {
- error_setg(errp, "Zcf extension is only relevant to RV32");
- return;
- }
-
- if (!cpu->cfg.ext_f && cpu->cfg.ext_zcf) {
- error_setg(errp, "Zcf extension requires F extension");
- return;
- }
-
- if (!cpu->cfg.ext_d && cpu->cfg.ext_zcd) {
- error_setg(errp, "Zcd extension requires D extension");
- return;
- }
-
- if ((cpu->cfg.ext_zcf || cpu->cfg.ext_zcd || cpu->cfg.ext_zcb ||
- cpu->cfg.ext_zcmp || cpu->cfg.ext_zcmt) && !cpu->cfg.ext_zca) {
- error_setg(errp, "Zcf/Zcd/Zcb/Zcmp/Zcmt extensions require Zca "
- "extension");
- return;
- }
-
- if (cpu->cfg.ext_zcd && (cpu->cfg.ext_zcmp || cpu->cfg.ext_zcmt)) {
- error_setg(errp, "Zcmp/Zcmt extensions are incompatible with "
- "Zcd extension");
- return;
- }
-
- if (cpu->cfg.ext_zcmt && !cpu->cfg.ext_icsr) {
- error_setg(errp, "Zcmt extension requires Zicsr extension");
- return;
- }
-
- if (cpu->cfg.ext_zk) {
- cpu->cfg.ext_zkn = true;
- cpu->cfg.ext_zkr = true;
- cpu->cfg.ext_zkt = true;
- }
-
- if (cpu->cfg.ext_zkn) {
- cpu->cfg.ext_zbkb = true;
- cpu->cfg.ext_zbkc = true;
- cpu->cfg.ext_zbkx = true;
- cpu->cfg.ext_zkne = true;
- cpu->cfg.ext_zknd = true;
- cpu->cfg.ext_zknh = true;
- }
-
- if (cpu->cfg.ext_zks) {
- cpu->cfg.ext_zbkb = true;
- cpu->cfg.ext_zbkc = true;
- cpu->cfg.ext_zbkx = true;
- cpu->cfg.ext_zksed = true;
- cpu->cfg.ext_zksh = true;
- }
-
- if (cpu->cfg.ext_i) {
- ext |= RVI;
- }
- if (cpu->cfg.ext_e) {
- ext |= RVE;
- }
- if (cpu->cfg.ext_m) {
- ext |= RVM;
- }
- if (cpu->cfg.ext_a) {
- ext |= RVA;
- }
- if (cpu->cfg.ext_f) {
- ext |= RVF;
- }
- if (cpu->cfg.ext_d) {
- ext |= RVD;
- }
- if (cpu->cfg.ext_c) {
- ext |= RVC;
- }
- if (cpu->cfg.ext_s) {
- ext |= RVS;
- }
- if (cpu->cfg.ext_u) {
- ext |= RVU;
- }
- if (cpu->cfg.ext_h) {
- ext |= RVH;
- }
- if (cpu->cfg.ext_v) {
- int vext_version = VEXT_VERSION_1_00_0;
- ext |= RVV;
- if (!is_power_of_2(cpu->cfg.vlen)) {
- error_setg(errp,
- "Vector extension VLEN must be power of 2");
- return;
- }
- if (cpu->cfg.vlen > RV_VLEN_MAX || cpu->cfg.vlen < 128) {
- error_setg(errp,
- "Vector extension implementation only supports VLEN "
- "in the range [128, %d]", RV_VLEN_MAX);
- return;
- }
- if (!is_power_of_2(cpu->cfg.elen)) {
- error_setg(errp,
- "Vector extension ELEN must be power of 2");
- return;
- }
- if (cpu->cfg.elen > 64 || cpu->cfg.elen < 8) {
- error_setg(errp,
- "Vector extension implementation only supports ELEN "
- "in the range [8, 64]");
- return;
- }
- if (cpu->cfg.vext_spec) {
- if (!g_strcmp0(cpu->cfg.vext_spec, "v1.0")) {
- vext_version = VEXT_VERSION_1_00_0;
- } else {
- error_setg(errp,
- "Unsupported vector spec version '%s'",
- cpu->cfg.vext_spec);
- return;
- }
- } else {
- qemu_log("vector version is not specified, "
- "use the default value v1.0\n");
- }
- set_vext_version(env, vext_version);
- }
- if (cpu->cfg.ext_j) {
- ext |= RVJ;
- }
-
- set_misa(env, env->misa_mxl, ext);
+ riscv_cpu_validate_set_extensions(cpu, &local_err);
+ if (local_err != NULL) {
+ error_propagate(errp, local_err);
+ return;
}
#ifndef CONFIG_USER_ONLY
--
2.39.0
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 0/2] target/riscv/cpu: fix sifive_u 32/64bits boot in riscv-to-apply.next
2023-01-10 20:14 [PATCH 0/2] target/riscv/cpu: fix sifive_u 32/64bits boot in riscv-to-apply.next Daniel Henrique Barboza
2023-01-10 20:14 ` [PATCH 1/2] target/riscv/cpu: set cpu->cfg in register_cpu_props() Daniel Henrique Barboza
2023-01-10 20:14 ` [PATCH 2/2] target/riscv/cpu.c: do not skip misa logic in riscv_cpu_realize() Daniel Henrique Barboza
@ 2023-01-10 22:27 ` Daniel Henrique Barboza
2023-01-11 5:01 ` Alistair Francis
3 siblings, 0 replies; 13+ messages in thread
From: Daniel Henrique Barboza @ 2023-01-10 22:27 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-riscv, alistair.francis, richard.henderson
Hi,
I mentioned that the bug were found in riscv-to-apply.next but forgot to
mentioned that the patches were also based on top of it as well:
https://github.com/alistair23/qemu/tree/riscv-to-apply.next
Thanks,
Daniel
On 1/10/23 17:14, Daniel Henrique Barboza wrote:
> Hi,
>
> I found this bug when testing my avocado changes in riscv-to-apply.next.
> The sifive_u board, both 32 and 64 bits, stopped booting OpenSBI. The
> guest hangs indefinitely.
>
> Git bisect points that this patch broke things:
>
> 8c3f35d25e7e98655c609b6c1e9f103b9240f8f8 is the first bad commit
> commit 8c3f35d25e7e98655c609b6c1e9f103b9240f8f8
> Author: Weiwei Li <liweiwei@iscas.ac.cn>
> Date: Wed Dec 28 14:20:21 2022 +0800
>
> target/riscv: add support for Zca extension
>
> Modify the check for C extension to Zca (C implies Zca)
> (https://github.com/alistair23/qemu/commit/8c3f35d25e7e98655c609b6c1e9f103b9240f8f8)
>
>
> But this patch per se isn't doing anything wrong. The root of the
> problem is that this patch makes assumptions based on the previous
> patch:
>
> commit a2b409aa6cadc1ed9715e1ab916ddd3dade0ba85
> Author: Weiwei Li <liweiwei@iscas.ac.cn>
> Date: Wed Dec 28 14:20:20 2022 +0800
>
> target/riscv: add cfg properties for Zc* extension
> (https://github.com/alistair23/qemu/commit/a2b409aa6cadc1ed9715e1ab916ddd3dade0ba85)
>
> Which added a lot of logic and assumptions that are being skipped by all
> the SiFive boards because, during riscv_cpu_realize(), we have this
> code:
>
> /* If only MISA_EXT is unset for misa, then set it from properties */
> if (env->misa_ext == 0) {
> uint32_t ext = 0;
> (...)
> }
>
> In short, we have a lot of code that are being skipped by all SiFive
> CPUs because these CPUs are setting a non-zero value in set_misa() in
> their respective cpu_init() functions.
>
> It's possible to just hack in and fix the SiFive problem in isolate, but
> I believe we can do better and allow all riscv_cpu_realize() to be executed
> for all CPUs, regardless of what they've done during their cpu_init().
>
>
> Daniel Henrique Barboza (2):
> target/riscv/cpu: set cpu->cfg in register_cpu_props()
> target/riscv/cpu.c: do not skip misa logic in riscv_cpu_realize()
>
> target/riscv/cpu.c | 525 +++++++++++++++++++++++++--------------------
> target/riscv/cpu.h | 4 +
> 2 files changed, 292 insertions(+), 237 deletions(-)
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] target/riscv/cpu: set cpu->cfg in register_cpu_props()
2023-01-10 20:14 ` [PATCH 1/2] target/riscv/cpu: set cpu->cfg in register_cpu_props() Daniel Henrique Barboza
@ 2023-01-10 22:52 ` Alistair Francis
2023-01-11 5:39 ` Richard Henderson
2023-01-11 5:55 ` Bin Meng
2 siblings, 0 replies; 13+ messages in thread
From: Alistair Francis @ 2023-01-10 22:52 UTC (permalink / raw)
To: Daniel Henrique Barboza
Cc: qemu-devel, qemu-riscv, alistair.francis, richard.henderson
On Wed, Jan 11, 2023 at 6:17 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> There is an informal contract between the cpu_init() functions and
> riscv_cpu_realize(): if cpu->env.misa_ext is zero, assume that the
> default settings were loaded via register_cpu_props() and do validations
> to set env.misa_ext. If it's not zero, skip this whole process and
> assume that the board somehow did everything.
>
> At this moment, all SiFive CPUs are setting a non-zero misa_ext during
> their cpu_init() and skipping a good chunk of riscv_cpu_realize().
> This causes problems when the code being skipped in riscv_cpu_realize()
> contains fixes or assumptions that affects all CPUs, meaning that SiFive
> CPUs are missing out.
>
> To allow this code to not be skipped anymore, all the cpu->cfg.ext_* attributes
> needs to be set during cpu_init() time. At this moment this is being done in
> register_cpu_props(). The SiFive oards are setting their own extensions during
> cpu_init() though, meaning that they don't want all the defaults from
> register_cpu_props().
>
> Let's move the contract between *_cpu_init() and riscv_cpu_realize() to
> register_cpu_props(). Inside this function we'll check if cpu->env.misa_ext
> was set and, if that's the case, set all relevant cpu->cfg.ext_*
> attributes, and only that. Leave the 'misa_ext' = 0 case as is today,
> i.e. loading all the defaults from riscv_cpu_extensions[].
>
> register_cpu_props() can then be called by all the cpu_init() functions,
> including the SiFive ones. This will make all CPUs behave more in line
> with that riscv_cpu_realize() expects.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Alistair
> ---
> target/riscv/cpu.c | 40 ++++++++++++++++++++++++++++++++++++++++
> target/riscv/cpu.h | 4 ++++
> 2 files changed, 44 insertions(+)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index ee3659cc7e..b8c1edb7c2 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -262,6 +262,7 @@ static void rv64_sifive_u_cpu_init(Object *obj)
> {
> CPURISCVState *env = &RISCV_CPU(obj)->env;
> set_misa(env, MXL_RV64, RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
> + register_cpu_props(DEVICE(obj));
> set_priv_version(env, PRIV_VERSION_1_10_0);
> }
>
> @@ -271,6 +272,7 @@ static void rv64_sifive_e_cpu_init(Object *obj)
> RISCVCPU *cpu = RISCV_CPU(obj);
>
> set_misa(env, MXL_RV64, RVI | RVM | RVA | RVC | RVU);
> + register_cpu_props(DEVICE(obj));
> set_priv_version(env, PRIV_VERSION_1_10_0);
> cpu->cfg.mmu = false;
> }
> @@ -305,6 +307,7 @@ static void rv32_sifive_u_cpu_init(Object *obj)
> {
> CPURISCVState *env = &RISCV_CPU(obj)->env;
> set_misa(env, MXL_RV32, RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
> + register_cpu_props(DEVICE(obj));
> set_priv_version(env, PRIV_VERSION_1_10_0);
> }
>
> @@ -314,6 +317,7 @@ static void rv32_sifive_e_cpu_init(Object *obj)
> RISCVCPU *cpu = RISCV_CPU(obj);
>
> set_misa(env, MXL_RV32, RVI | RVM | RVA | RVC | RVU);
> + register_cpu_props(DEVICE(obj));
> set_priv_version(env, PRIV_VERSION_1_10_0);
> cpu->cfg.mmu = false;
> }
> @@ -324,6 +328,7 @@ static void rv32_ibex_cpu_init(Object *obj)
> RISCVCPU *cpu = RISCV_CPU(obj);
>
> set_misa(env, MXL_RV32, RVI | RVM | RVC | RVU);
> + register_cpu_props(DEVICE(obj));
> set_priv_version(env, PRIV_VERSION_1_11_0);
> cpu->cfg.mmu = false;
> cpu->cfg.epmp = true;
> @@ -335,6 +340,7 @@ static void rv32_imafcu_nommu_cpu_init(Object *obj)
> RISCVCPU *cpu = RISCV_CPU(obj);
>
> set_misa(env, MXL_RV32, RVI | RVM | RVA | RVF | RVC | RVU);
> + register_cpu_props(DEVICE(obj));
> set_priv_version(env, PRIV_VERSION_1_10_0);
> cpu->cfg.mmu = false;
> }
> @@ -1139,10 +1145,44 @@ static Property riscv_cpu_extensions[] = {
> DEFINE_PROP_END_OF_LIST(),
> };
>
> +/*
> + * Register CPU props based on env.misa_ext. If a non-zero
> + * value was set, register only the required cpu->cfg.ext_*
> + * properties and leave. env.misa_ext = 0 means that we want
> + * all the default properties to be registered.
> + */
> static void register_cpu_props(DeviceState *dev)
> {
> + RISCVCPU *cpu = RISCV_CPU(OBJECT(dev));
> + uint32_t misa_ext = cpu->env.misa_ext;
> Property *prop;
>
> + /*
> + * If misa_ext is not zero, set cfg properties now to
> + * allow them to be read during riscv_cpu_realize()
> + * later on.
> + */
> + if (cpu->env.misa_ext != 0) {
> + cpu->cfg.ext_i = misa_ext & RVI;
> + cpu->cfg.ext_e = misa_ext & RVE;
> + cpu->cfg.ext_m = misa_ext & RVM;
> + cpu->cfg.ext_a = misa_ext & RVA;
> + cpu->cfg.ext_f = misa_ext & RVF;
> + cpu->cfg.ext_d = misa_ext & RVD;
> + cpu->cfg.ext_v = misa_ext & RVV;
> + cpu->cfg.ext_c = misa_ext & RVC;
> + cpu->cfg.ext_s = misa_ext & RVS;
> + cpu->cfg.ext_u = misa_ext & RVU;
> + cpu->cfg.ext_h = misa_ext & RVH;
> + cpu->cfg.ext_j = misa_ext & RVJ;
> +
> + /*
> + * We don't want to set the default riscv_cpu_extensions
> + * in this case.
> + */
> + return;
> + }
> +
> for (prop = riscv_cpu_extensions; prop && prop->name; prop++) {
> qdev_property_add_static(dev, prop);
> }
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 0158932dc5..798bd081de 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -63,6 +63,10 @@
>
> #define RV(x) ((target_ulong)1 << (x - 'A'))
>
> +/*
> + * Consider updating register_cpu_props() when adding
> + * new MISA bits here.
> + */
> #define RVI RV('I')
> #define RVE RV('E') /* E and I are mutually exclusive */
> #define RVM RV('M')
> --
> 2.39.0
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] target/riscv/cpu.c: do not skip misa logic in riscv_cpu_realize()
2023-01-10 20:14 ` [PATCH 2/2] target/riscv/cpu.c: do not skip misa logic in riscv_cpu_realize() Daniel Henrique Barboza
@ 2023-01-10 22:53 ` Alistair Francis
2023-01-11 5:56 ` Bin Meng
1 sibling, 0 replies; 13+ messages in thread
From: Alistair Francis @ 2023-01-10 22:53 UTC (permalink / raw)
To: Daniel Henrique Barboza
Cc: qemu-devel, qemu-riscv, alistair.francis, richard.henderson
On Wed, Jan 11, 2023 at 6:17 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> All RISCV CPUs are setting cpu->cfg during their cpu_init() functions,
> meaning that there's no reason to skip all the misa validation and setup
> if misa_ext was set beforehand - especially since we're setting an
> updated value in set_misa() in the end.
>
> Put this code chunk into a new riscv_cpu_validate_set_extensions()
> helper and always execute it regardless of what the board set in
> env->misa_ext.
>
> This will put more responsibility in how each board is going to init
> their attributes and extensions if they're not using the defaults.
> It'll also allow realize() to do its job looking only at the extensions
> enabled per se, not corner cases that some CPUs might have, and we won't
> have to change multiple code paths to fix or change how extensions work.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Alistair
> ---
> target/riscv/cpu.c | 485 +++++++++++++++++++++++----------------------
> 1 file changed, 248 insertions(+), 237 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index b8c1edb7c2..33ed59a1b6 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -631,6 +631,250 @@ static void riscv_cpu_disas_set_info(CPUState *s, disassemble_info *info)
> }
> }
>
> +/*
> + * Check consistency between chosen extensions while setting
> + * cpu->cfg accordingly, doing a set_misa() in the end.
> + */
> +static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
> +{
> + CPURISCVState *env = &cpu->env;
> + uint32_t ext = 0;
> +
> + /* Do some ISA extension error checking */
> + if (cpu->cfg.ext_g && !(cpu->cfg.ext_i && cpu->cfg.ext_m &&
> + cpu->cfg.ext_a && cpu->cfg.ext_f &&
> + cpu->cfg.ext_d &&
> + cpu->cfg.ext_icsr && cpu->cfg.ext_ifencei)) {
> + warn_report("Setting G will also set IMAFD_Zicsr_Zifencei");
> + cpu->cfg.ext_i = true;
> + cpu->cfg.ext_m = true;
> + cpu->cfg.ext_a = true;
> + cpu->cfg.ext_f = true;
> + cpu->cfg.ext_d = true;
> + cpu->cfg.ext_icsr = true;
> + cpu->cfg.ext_ifencei = true;
> + }
> +
> + if (cpu->cfg.ext_i && cpu->cfg.ext_e) {
> + error_setg(errp,
> + "I and E extensions are incompatible");
> + return;
> + }
> +
> + if (!cpu->cfg.ext_i && !cpu->cfg.ext_e) {
> + error_setg(errp,
> + "Either I or E extension must be set");
> + return;
> + }
> +
> + if (cpu->cfg.ext_s && !cpu->cfg.ext_u) {
> + error_setg(errp,
> + "Setting S extension without U extension is illegal");
> + return;
> + }
> +
> + if (cpu->cfg.ext_h && !cpu->cfg.ext_i) {
> + error_setg(errp,
> + "H depends on an I base integer ISA with 32 x registers");
> + return;
> + }
> +
> + if (cpu->cfg.ext_h && !cpu->cfg.ext_s) {
> + error_setg(errp, "H extension implicitly requires S-mode");
> + return;
> + }
> +
> + if (cpu->cfg.ext_f && !cpu->cfg.ext_icsr) {
> + error_setg(errp, "F extension requires Zicsr");
> + return;
> + }
> +
> + if ((cpu->cfg.ext_zawrs) && !cpu->cfg.ext_a) {
> + error_setg(errp, "Zawrs extension requires A extension");
> + return;
> + }
> +
> + if ((cpu->cfg.ext_zfh || cpu->cfg.ext_zfhmin) && !cpu->cfg.ext_f) {
> + error_setg(errp, "Zfh/Zfhmin extensions require F extension");
> + return;
> + }
> +
> + if (cpu->cfg.ext_d && !cpu->cfg.ext_f) {
> + error_setg(errp, "D extension requires F extension");
> + return;
> + }
> +
> + if (cpu->cfg.ext_v && !cpu->cfg.ext_d) {
> + error_setg(errp, "V extension requires D extension");
> + return;
> + }
> +
> + if ((cpu->cfg.ext_zve32f || cpu->cfg.ext_zve64f) && !cpu->cfg.ext_f) {
> + error_setg(errp, "Zve32f/Zve64f extensions require F extension");
> + return;
> + }
> +
> + /* Set the ISA extensions, checks should have happened above */
> + if (cpu->cfg.ext_zdinx || cpu->cfg.ext_zhinx ||
> + cpu->cfg.ext_zhinxmin) {
> + cpu->cfg.ext_zfinx = true;
> + }
> +
> + if (cpu->cfg.ext_zfinx) {
> + if (!cpu->cfg.ext_icsr) {
> + error_setg(errp, "Zfinx extension requires Zicsr");
> + return;
> + }
> + if (cpu->cfg.ext_f) {
> + error_setg(errp,
> + "Zfinx cannot be supported together with F extension");
> + return;
> + }
> + }
> +
> + if (cpu->cfg.ext_c) {
> + cpu->cfg.ext_zca = true;
> + if (cpu->cfg.ext_f && env->misa_mxl_max == MXL_RV32) {
> + cpu->cfg.ext_zcf = true;
> + }
> + if (cpu->cfg.ext_d) {
> + cpu->cfg.ext_zcd = true;
> + }
> + }
> +
> + if (env->misa_mxl_max != MXL_RV32 && cpu->cfg.ext_zcf) {
> + error_setg(errp, "Zcf extension is only relevant to RV32");
> + return;
> + }
> +
> + if (!cpu->cfg.ext_f && cpu->cfg.ext_zcf) {
> + error_setg(errp, "Zcf extension requires F extension");
> + return;
> + }
> +
> + if (!cpu->cfg.ext_d && cpu->cfg.ext_zcd) {
> + error_setg(errp, "Zcd extension requires D extension");
> + return;
> + }
> +
> + if ((cpu->cfg.ext_zcf || cpu->cfg.ext_zcd || cpu->cfg.ext_zcb ||
> + cpu->cfg.ext_zcmp || cpu->cfg.ext_zcmt) && !cpu->cfg.ext_zca) {
> + error_setg(errp, "Zcf/Zcd/Zcb/Zcmp/Zcmt extensions require Zca "
> + "extension");
> + return;
> + }
> +
> + if (cpu->cfg.ext_zcd && (cpu->cfg.ext_zcmp || cpu->cfg.ext_zcmt)) {
> + error_setg(errp, "Zcmp/Zcmt extensions are incompatible with "
> + "Zcd extension");
> + return;
> + }
> +
> + if (cpu->cfg.ext_zcmt && !cpu->cfg.ext_icsr) {
> + error_setg(errp, "Zcmt extension requires Zicsr extension");
> + return;
> + }
> +
> + if (cpu->cfg.ext_zk) {
> + cpu->cfg.ext_zkn = true;
> + cpu->cfg.ext_zkr = true;
> + cpu->cfg.ext_zkt = true;
> + }
> +
> + if (cpu->cfg.ext_zkn) {
> + cpu->cfg.ext_zbkb = true;
> + cpu->cfg.ext_zbkc = true;
> + cpu->cfg.ext_zbkx = true;
> + cpu->cfg.ext_zkne = true;
> + cpu->cfg.ext_zknd = true;
> + cpu->cfg.ext_zknh = true;
> + }
> +
> + if (cpu->cfg.ext_zks) {
> + cpu->cfg.ext_zbkb = true;
> + cpu->cfg.ext_zbkc = true;
> + cpu->cfg.ext_zbkx = true;
> + cpu->cfg.ext_zksed = true;
> + cpu->cfg.ext_zksh = true;
> + }
> +
> + if (cpu->cfg.ext_i) {
> + ext |= RVI;
> + }
> + if (cpu->cfg.ext_e) {
> + ext |= RVE;
> + }
> + if (cpu->cfg.ext_m) {
> + ext |= RVM;
> + }
> + if (cpu->cfg.ext_a) {
> + ext |= RVA;
> + }
> + if (cpu->cfg.ext_f) {
> + ext |= RVF;
> + }
> + if (cpu->cfg.ext_d) {
> + ext |= RVD;
> + }
> + if (cpu->cfg.ext_c) {
> + ext |= RVC;
> + }
> + if (cpu->cfg.ext_s) {
> + ext |= RVS;
> + }
> + if (cpu->cfg.ext_u) {
> + ext |= RVU;
> + }
> + if (cpu->cfg.ext_h) {
> + ext |= RVH;
> + }
> + if (cpu->cfg.ext_v) {
> + int vext_version = VEXT_VERSION_1_00_0;
> + ext |= RVV;
> + if (!is_power_of_2(cpu->cfg.vlen)) {
> + error_setg(errp,
> + "Vector extension VLEN must be power of 2");
> + return;
> + }
> + if (cpu->cfg.vlen > RV_VLEN_MAX || cpu->cfg.vlen < 128) {
> + error_setg(errp,
> + "Vector extension implementation only supports VLEN "
> + "in the range [128, %d]", RV_VLEN_MAX);
> + return;
> + }
> + if (!is_power_of_2(cpu->cfg.elen)) {
> + error_setg(errp,
> + "Vector extension ELEN must be power of 2");
> + return;
> + }
> + if (cpu->cfg.elen > 64 || cpu->cfg.elen < 8) {
> + error_setg(errp,
> + "Vector extension implementation only supports ELEN "
> + "in the range [8, 64]");
> + return;
> + }
> + if (cpu->cfg.vext_spec) {
> + if (!g_strcmp0(cpu->cfg.vext_spec, "v1.0")) {
> + vext_version = VEXT_VERSION_1_00_0;
> + } else {
> + error_setg(errp,
> + "Unsupported vector spec version '%s'",
> + cpu->cfg.vext_spec);
> + return;
> + }
> + } else {
> + qemu_log("vector version is not specified, "
> + "use the default value v1.0\n");
> + }
> + set_vext_version(env, vext_version);
> + }
> + if (cpu->cfg.ext_j) {
> + ext |= RVJ;
> + }
> +
> + set_misa(env, env->misa_mxl, ext);
> +}
> +
> static void riscv_cpu_realize(DeviceState *dev, Error **errp)
> {
> CPUState *cs = CPU(dev);
> @@ -726,243 +970,10 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
> }
> assert(env->misa_mxl_max == env->misa_mxl);
>
> - /* If only MISA_EXT is unset for misa, then set it from properties */
> - if (env->misa_ext == 0) {
> - uint32_t ext = 0;
> -
> - /* Do some ISA extension error checking */
> - if (cpu->cfg.ext_g && !(cpu->cfg.ext_i && cpu->cfg.ext_m &&
> - cpu->cfg.ext_a && cpu->cfg.ext_f &&
> - cpu->cfg.ext_d &&
> - cpu->cfg.ext_icsr && cpu->cfg.ext_ifencei)) {
> - warn_report("Setting G will also set IMAFD_Zicsr_Zifencei");
> - cpu->cfg.ext_i = true;
> - cpu->cfg.ext_m = true;
> - cpu->cfg.ext_a = true;
> - cpu->cfg.ext_f = true;
> - cpu->cfg.ext_d = true;
> - cpu->cfg.ext_icsr = true;
> - cpu->cfg.ext_ifencei = true;
> - }
> -
> - if (cpu->cfg.ext_i && cpu->cfg.ext_e) {
> - error_setg(errp,
> - "I and E extensions are incompatible");
> - return;
> - }
> -
> - if (!cpu->cfg.ext_i && !cpu->cfg.ext_e) {
> - error_setg(errp,
> - "Either I or E extension must be set");
> - return;
> - }
> -
> - if (cpu->cfg.ext_s && !cpu->cfg.ext_u) {
> - error_setg(errp,
> - "Setting S extension without U extension is illegal");
> - return;
> - }
> -
> - if (cpu->cfg.ext_h && !cpu->cfg.ext_i) {
> - error_setg(errp,
> - "H depends on an I base integer ISA with 32 x registers");
> - return;
> - }
> -
> - if (cpu->cfg.ext_h && !cpu->cfg.ext_s) {
> - error_setg(errp, "H extension implicitly requires S-mode");
> - return;
> - }
> -
> - if (cpu->cfg.ext_f && !cpu->cfg.ext_icsr) {
> - error_setg(errp, "F extension requires Zicsr");
> - return;
> - }
> -
> - if ((cpu->cfg.ext_zawrs) && !cpu->cfg.ext_a) {
> - error_setg(errp, "Zawrs extension requires A extension");
> - return;
> - }
> -
> - if ((cpu->cfg.ext_zfh || cpu->cfg.ext_zfhmin) && !cpu->cfg.ext_f) {
> - error_setg(errp, "Zfh/Zfhmin extensions require F extension");
> - return;
> - }
> -
> - if (cpu->cfg.ext_d && !cpu->cfg.ext_f) {
> - error_setg(errp, "D extension requires F extension");
> - return;
> - }
> -
> - if (cpu->cfg.ext_v && !cpu->cfg.ext_d) {
> - error_setg(errp, "V extension requires D extension");
> - return;
> - }
> -
> - if ((cpu->cfg.ext_zve32f || cpu->cfg.ext_zve64f) && !cpu->cfg.ext_f) {
> - error_setg(errp, "Zve32f/Zve64f extensions require F extension");
> - return;
> - }
> -
> - /* Set the ISA extensions, checks should have happened above */
> - if (cpu->cfg.ext_zdinx || cpu->cfg.ext_zhinx ||
> - cpu->cfg.ext_zhinxmin) {
> - cpu->cfg.ext_zfinx = true;
> - }
> -
> - if (cpu->cfg.ext_zfinx) {
> - if (!cpu->cfg.ext_icsr) {
> - error_setg(errp, "Zfinx extension requires Zicsr");
> - return;
> - }
> - if (cpu->cfg.ext_f) {
> - error_setg(errp,
> - "Zfinx cannot be supported together with F extension");
> - return;
> - }
> - }
> -
> - if (cpu->cfg.ext_c) {
> - cpu->cfg.ext_zca = true;
> - if (cpu->cfg.ext_f && env->misa_mxl_max == MXL_RV32) {
> - cpu->cfg.ext_zcf = true;
> - }
> - if (cpu->cfg.ext_d) {
> - cpu->cfg.ext_zcd = true;
> - }
> - }
> -
> - if (env->misa_mxl_max != MXL_RV32 && cpu->cfg.ext_zcf) {
> - error_setg(errp, "Zcf extension is only relevant to RV32");
> - return;
> - }
> -
> - if (!cpu->cfg.ext_f && cpu->cfg.ext_zcf) {
> - error_setg(errp, "Zcf extension requires F extension");
> - return;
> - }
> -
> - if (!cpu->cfg.ext_d && cpu->cfg.ext_zcd) {
> - error_setg(errp, "Zcd extension requires D extension");
> - return;
> - }
> -
> - if ((cpu->cfg.ext_zcf || cpu->cfg.ext_zcd || cpu->cfg.ext_zcb ||
> - cpu->cfg.ext_zcmp || cpu->cfg.ext_zcmt) && !cpu->cfg.ext_zca) {
> - error_setg(errp, "Zcf/Zcd/Zcb/Zcmp/Zcmt extensions require Zca "
> - "extension");
> - return;
> - }
> -
> - if (cpu->cfg.ext_zcd && (cpu->cfg.ext_zcmp || cpu->cfg.ext_zcmt)) {
> - error_setg(errp, "Zcmp/Zcmt extensions are incompatible with "
> - "Zcd extension");
> - return;
> - }
> -
> - if (cpu->cfg.ext_zcmt && !cpu->cfg.ext_icsr) {
> - error_setg(errp, "Zcmt extension requires Zicsr extension");
> - return;
> - }
> -
> - if (cpu->cfg.ext_zk) {
> - cpu->cfg.ext_zkn = true;
> - cpu->cfg.ext_zkr = true;
> - cpu->cfg.ext_zkt = true;
> - }
> -
> - if (cpu->cfg.ext_zkn) {
> - cpu->cfg.ext_zbkb = true;
> - cpu->cfg.ext_zbkc = true;
> - cpu->cfg.ext_zbkx = true;
> - cpu->cfg.ext_zkne = true;
> - cpu->cfg.ext_zknd = true;
> - cpu->cfg.ext_zknh = true;
> - }
> -
> - if (cpu->cfg.ext_zks) {
> - cpu->cfg.ext_zbkb = true;
> - cpu->cfg.ext_zbkc = true;
> - cpu->cfg.ext_zbkx = true;
> - cpu->cfg.ext_zksed = true;
> - cpu->cfg.ext_zksh = true;
> - }
> -
> - if (cpu->cfg.ext_i) {
> - ext |= RVI;
> - }
> - if (cpu->cfg.ext_e) {
> - ext |= RVE;
> - }
> - if (cpu->cfg.ext_m) {
> - ext |= RVM;
> - }
> - if (cpu->cfg.ext_a) {
> - ext |= RVA;
> - }
> - if (cpu->cfg.ext_f) {
> - ext |= RVF;
> - }
> - if (cpu->cfg.ext_d) {
> - ext |= RVD;
> - }
> - if (cpu->cfg.ext_c) {
> - ext |= RVC;
> - }
> - if (cpu->cfg.ext_s) {
> - ext |= RVS;
> - }
> - if (cpu->cfg.ext_u) {
> - ext |= RVU;
> - }
> - if (cpu->cfg.ext_h) {
> - ext |= RVH;
> - }
> - if (cpu->cfg.ext_v) {
> - int vext_version = VEXT_VERSION_1_00_0;
> - ext |= RVV;
> - if (!is_power_of_2(cpu->cfg.vlen)) {
> - error_setg(errp,
> - "Vector extension VLEN must be power of 2");
> - return;
> - }
> - if (cpu->cfg.vlen > RV_VLEN_MAX || cpu->cfg.vlen < 128) {
> - error_setg(errp,
> - "Vector extension implementation only supports VLEN "
> - "in the range [128, %d]", RV_VLEN_MAX);
> - return;
> - }
> - if (!is_power_of_2(cpu->cfg.elen)) {
> - error_setg(errp,
> - "Vector extension ELEN must be power of 2");
> - return;
> - }
> - if (cpu->cfg.elen > 64 || cpu->cfg.elen < 8) {
> - error_setg(errp,
> - "Vector extension implementation only supports ELEN "
> - "in the range [8, 64]");
> - return;
> - }
> - if (cpu->cfg.vext_spec) {
> - if (!g_strcmp0(cpu->cfg.vext_spec, "v1.0")) {
> - vext_version = VEXT_VERSION_1_00_0;
> - } else {
> - error_setg(errp,
> - "Unsupported vector spec version '%s'",
> - cpu->cfg.vext_spec);
> - return;
> - }
> - } else {
> - qemu_log("vector version is not specified, "
> - "use the default value v1.0\n");
> - }
> - set_vext_version(env, vext_version);
> - }
> - if (cpu->cfg.ext_j) {
> - ext |= RVJ;
> - }
> -
> - set_misa(env, env->misa_mxl, ext);
> + riscv_cpu_validate_set_extensions(cpu, &local_err);
> + if (local_err != NULL) {
> + error_propagate(errp, local_err);
> + return;
> }
>
> #ifndef CONFIG_USER_ONLY
> --
> 2.39.0
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/2] target/riscv/cpu: fix sifive_u 32/64bits boot in riscv-to-apply.next
2023-01-10 20:14 [PATCH 0/2] target/riscv/cpu: fix sifive_u 32/64bits boot in riscv-to-apply.next Daniel Henrique Barboza
` (2 preceding siblings ...)
2023-01-10 22:27 ` [PATCH 0/2] target/riscv/cpu: fix sifive_u 32/64bits boot in riscv-to-apply.next Daniel Henrique Barboza
@ 2023-01-11 5:01 ` Alistair Francis
2023-01-13 1:28 ` Bin Meng
3 siblings, 1 reply; 13+ messages in thread
From: Alistair Francis @ 2023-01-11 5:01 UTC (permalink / raw)
To: Daniel Henrique Barboza
Cc: qemu-devel, qemu-riscv, alistair.francis, richard.henderson
On Wed, Jan 11, 2023 at 6:17 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Hi,
>
> I found this bug when testing my avocado changes in riscv-to-apply.next.
> The sifive_u board, both 32 and 64 bits, stopped booting OpenSBI. The
> guest hangs indefinitely.
>
> Git bisect points that this patch broke things:
>
> 8c3f35d25e7e98655c609b6c1e9f103b9240f8f8 is the first bad commit
> commit 8c3f35d25e7e98655c609b6c1e9f103b9240f8f8
> Author: Weiwei Li <liweiwei@iscas.ac.cn>
> Date: Wed Dec 28 14:20:21 2022 +0800
>
> target/riscv: add support for Zca extension
>
> Modify the check for C extension to Zca (C implies Zca)
> (https://github.com/alistair23/qemu/commit/8c3f35d25e7e98655c609b6c1e9f103b9240f8f8)
>
>
> But this patch per se isn't doing anything wrong. The root of the
> problem is that this patch makes assumptions based on the previous
> patch:
>
> commit a2b409aa6cadc1ed9715e1ab916ddd3dade0ba85
> Author: Weiwei Li <liweiwei@iscas.ac.cn>
> Date: Wed Dec 28 14:20:20 2022 +0800
>
> target/riscv: add cfg properties for Zc* extension
> (https://github.com/alistair23/qemu/commit/a2b409aa6cadc1ed9715e1ab916ddd3dade0ba85)
>
> Which added a lot of logic and assumptions that are being skipped by all
> the SiFive boards because, during riscv_cpu_realize(), we have this
> code:
>
> /* If only MISA_EXT is unset for misa, then set it from properties */
> if (env->misa_ext == 0) {
> uint32_t ext = 0;
> (...)
> }
>
> In short, we have a lot of code that are being skipped by all SiFive
> CPUs because these CPUs are setting a non-zero value in set_misa() in
> their respective cpu_init() functions.
>
> It's possible to just hack in and fix the SiFive problem in isolate, but
> I believe we can do better and allow all riscv_cpu_realize() to be executed
> for all CPUs, regardless of what they've done during their cpu_init().
>
>
> Daniel Henrique Barboza (2):
> target/riscv/cpu: set cpu->cfg in register_cpu_props()
> target/riscv/cpu.c: do not skip misa logic in riscv_cpu_realize()
Thanks for the patches
I have rebased these onto the latest master and dropped the other
series. That way when the other series is applied we don't break
bisectability.
Alistair
>
> target/riscv/cpu.c | 525 +++++++++++++++++++++++++--------------------
> target/riscv/cpu.h | 4 +
> 2 files changed, 292 insertions(+), 237 deletions(-)
>
> --
> 2.39.0
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] target/riscv/cpu: set cpu->cfg in register_cpu_props()
2023-01-10 20:14 ` [PATCH 1/2] target/riscv/cpu: set cpu->cfg in register_cpu_props() Daniel Henrique Barboza
2023-01-10 22:52 ` Alistair Francis
@ 2023-01-11 5:39 ` Richard Henderson
2023-01-11 15:51 ` Daniel Henrique Barboza
2023-01-11 5:55 ` Bin Meng
2 siblings, 1 reply; 13+ messages in thread
From: Richard Henderson @ 2023-01-11 5:39 UTC (permalink / raw)
To: Daniel Henrique Barboza, qemu-devel; +Cc: qemu-riscv, alistair.francis
On 1/10/23 12:14, Daniel Henrique Barboza wrote:
> +/*
> + * Register CPU props based on env.misa_ext. If a non-zero
> + * value was set, register only the required cpu->cfg.ext_*
> + * properties and leave. env.misa_ext = 0 means that we want
> + * all the default properties to be registered.
> + */
> static void register_cpu_props(DeviceState *dev)
Suggest invoking this as .instance_post_init hook on TYPE_RISCV_CPU.
Then you don't need to manually call it on every cpu class.
r~
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] target/riscv/cpu: set cpu->cfg in register_cpu_props()
2023-01-10 20:14 ` [PATCH 1/2] target/riscv/cpu: set cpu->cfg in register_cpu_props() Daniel Henrique Barboza
2023-01-10 22:52 ` Alistair Francis
2023-01-11 5:39 ` Richard Henderson
@ 2023-01-11 5:55 ` Bin Meng
2 siblings, 0 replies; 13+ messages in thread
From: Bin Meng @ 2023-01-11 5:55 UTC (permalink / raw)
To: Daniel Henrique Barboza
Cc: qemu-devel, qemu-riscv, alistair.francis, richard.henderson
On Wed, Jan 11, 2023 at 4:17 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> There is an informal contract between the cpu_init() functions and
> riscv_cpu_realize(): if cpu->env.misa_ext is zero, assume that the
> default settings were loaded via register_cpu_props() and do validations
> to set env.misa_ext. If it's not zero, skip this whole process and
> assume that the board somehow did everything.
>
> At this moment, all SiFive CPUs are setting a non-zero misa_ext during
> their cpu_init() and skipping a good chunk of riscv_cpu_realize().
> This causes problems when the code being skipped in riscv_cpu_realize()
> contains fixes or assumptions that affects all CPUs, meaning that SiFive
> CPUs are missing out.
>
> To allow this code to not be skipped anymore, all the cpu->cfg.ext_* attributes
> needs to be set during cpu_init() time. At this moment this is being done in
> register_cpu_props(). The SiFive oards are setting their own extensions during
The SiFive boards
> cpu_init() though, meaning that they don't want all the defaults from
> register_cpu_props().
>
> Let's move the contract between *_cpu_init() and riscv_cpu_realize() to
> register_cpu_props(). Inside this function we'll check if cpu->env.misa_ext
> was set and, if that's the case, set all relevant cpu->cfg.ext_*
> attributes, and only that. Leave the 'misa_ext' = 0 case as is today,
> i.e. loading all the defaults from riscv_cpu_extensions[].
>
> register_cpu_props() can then be called by all the cpu_init() functions,
> including the SiFive ones. This will make all CPUs behave more in line
> with that riscv_cpu_realize() expects.
with what
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
> target/riscv/cpu.c | 40 ++++++++++++++++++++++++++++++++++++++++
> target/riscv/cpu.h | 4 ++++
> 2 files changed, 44 insertions(+)
>
Regards,
Bin
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] target/riscv/cpu.c: do not skip misa logic in riscv_cpu_realize()
2023-01-10 20:14 ` [PATCH 2/2] target/riscv/cpu.c: do not skip misa logic in riscv_cpu_realize() Daniel Henrique Barboza
2023-01-10 22:53 ` Alistair Francis
@ 2023-01-11 5:56 ` Bin Meng
1 sibling, 0 replies; 13+ messages in thread
From: Bin Meng @ 2023-01-11 5:56 UTC (permalink / raw)
To: Daniel Henrique Barboza
Cc: qemu-devel, qemu-riscv, alistair.francis, richard.henderson
On Wed, Jan 11, 2023 at 4:17 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> All RISCV CPUs are setting cpu->cfg during their cpu_init() functions,
> meaning that there's no reason to skip all the misa validation and setup
> if misa_ext was set beforehand - especially since we're setting an
> updated value in set_misa() in the end.
>
> Put this code chunk into a new riscv_cpu_validate_set_extensions()
> helper and always execute it regardless of what the board set in
> env->misa_ext.
>
> This will put more responsibility in how each board is going to init
> their attributes and extensions if they're not using the defaults.
> It'll also allow realize() to do its job looking only at the extensions
> enabled per se, not corner cases that some CPUs might have, and we won't
> have to change multiple code paths to fix or change how extensions work.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
> target/riscv/cpu.c | 485 +++++++++++++++++++++++----------------------
> 1 file changed, 248 insertions(+), 237 deletions(-)
>
Reviewed-by: Bin Meng <bmeng@tinylab.org>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] target/riscv/cpu: set cpu->cfg in register_cpu_props()
2023-01-11 5:39 ` Richard Henderson
@ 2023-01-11 15:51 ` Daniel Henrique Barboza
0 siblings, 0 replies; 13+ messages in thread
From: Daniel Henrique Barboza @ 2023-01-11 15:51 UTC (permalink / raw)
To: Richard Henderson, qemu-devel; +Cc: qemu-riscv, alistair.francis
On 1/11/23 02:39, Richard Henderson wrote:
> On 1/10/23 12:14, Daniel Henrique Barboza wrote:
>> +/*
>> + * Register CPU props based on env.misa_ext. If a non-zero
>> + * value was set, register only the required cpu->cfg.ext_*
>> + * properties and leave. env.misa_ext = 0 means that we want
>> + * all the default properties to be registered.
>> + */
>> static void register_cpu_props(DeviceState *dev)
>
> Suggest invoking this as .instance_post_init hook on TYPE_RISCV_CPU.
> Then you don't need to manually call it on every cpu class.
That would be nice but we have code such as:
@@ -317,7 +310,6 @@ static void rv32_sifive_e_cpu_init(Object *obj)
RISCVCPU *cpu = RISCV_CPU(obj);
set_misa(env, MXL_RV32, RVI | RVM | RVA | RVC | RVU);
register_cpu_props(DEVICE(obj));
set_priv_version(env, PRIV_VERSION_1_10_0);
cpu->cfg.mmu = false; <===========
That are setting cpu->cfg attrs after register_cpu_props(), i.e. "I want the
defaults and these specific settings on top of it".
I can think of a few ways to add a a post_init hook to reduce this code
repetition but I'll need to play around with it a bit first.
Thanks,
Daniel
>
>
> r~
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/2] target/riscv/cpu: fix sifive_u 32/64bits boot in riscv-to-apply.next
2023-01-11 5:01 ` Alistair Francis
@ 2023-01-13 1:28 ` Bin Meng
2023-01-13 10:20 ` Daniel Henrique Barboza
0 siblings, 1 reply; 13+ messages in thread
From: Bin Meng @ 2023-01-13 1:28 UTC (permalink / raw)
To: Alistair Francis
Cc: Daniel Henrique Barboza, qemu-devel, qemu-riscv,
alistair.francis, richard.henderson
Hi Daniel,
On Wed, Jan 11, 2023 at 1:03 PM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Wed, Jan 11, 2023 at 6:17 AM Daniel Henrique Barboza
> <dbarboza@ventanamicro.com> wrote:
> >
> > Hi,
> >
> > I found this bug when testing my avocado changes in riscv-to-apply.next.
> > The sifive_u board, both 32 and 64 bits, stopped booting OpenSBI. The
> > guest hangs indefinitely.
> >
> > Git bisect points that this patch broke things:
> >
> > 8c3f35d25e7e98655c609b6c1e9f103b9240f8f8 is the first bad commit
> > commit 8c3f35d25e7e98655c609b6c1e9f103b9240f8f8
> > Author: Weiwei Li <liweiwei@iscas.ac.cn>
> > Date: Wed Dec 28 14:20:21 2022 +0800
> >
> > target/riscv: add support for Zca extension
> >
> > Modify the check for C extension to Zca (C implies Zca)
> > (https://github.com/alistair23/qemu/commit/8c3f35d25e7e98655c609b6c1e9f103b9240f8f8)
> >
> >
> > But this patch per se isn't doing anything wrong. The root of the
> > problem is that this patch makes assumptions based on the previous
> > patch:
> >
> > commit a2b409aa6cadc1ed9715e1ab916ddd3dade0ba85
> > Author: Weiwei Li <liweiwei@iscas.ac.cn>
> > Date: Wed Dec 28 14:20:20 2022 +0800
> >
> > target/riscv: add cfg properties for Zc* extension
> > (https://github.com/alistair23/qemu/commit/a2b409aa6cadc1ed9715e1ab916ddd3dade0ba85)
> >
> > Which added a lot of logic and assumptions that are being skipped by all
> > the SiFive boards because, during riscv_cpu_realize(), we have this
> > code:
> >
> > /* If only MISA_EXT is unset for misa, then set it from properties */
> > if (env->misa_ext == 0) {
> > uint32_t ext = 0;
> > (...)
> > }
> >
> > In short, we have a lot of code that are being skipped by all SiFive
> > CPUs because these CPUs are setting a non-zero value in set_misa() in
> > their respective cpu_init() functions.
> >
> > It's possible to just hack in and fix the SiFive problem in isolate, but
> > I believe we can do better and allow all riscv_cpu_realize() to be executed
> > for all CPUs, regardless of what they've done during their cpu_init().
> >
> >
> > Daniel Henrique Barboza (2):
> > target/riscv/cpu: set cpu->cfg in register_cpu_props()
> > target/riscv/cpu.c: do not skip misa logic in riscv_cpu_realize()
>
> Thanks for the patches
>
> I have rebased these onto the latest master and dropped the other
> series. That way when the other series is applied we don't break
> bisectability.
It seems these 2 patches are already in Alistair's tree.
Richard had a suggestion for patch 1 and I had some minor comments
too. Do you plan to resend a v2 for that?
Regards,
Bin
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/2] target/riscv/cpu: fix sifive_u 32/64bits boot in riscv-to-apply.next
2023-01-13 1:28 ` Bin Meng
@ 2023-01-13 10:20 ` Daniel Henrique Barboza
0 siblings, 0 replies; 13+ messages in thread
From: Daniel Henrique Barboza @ 2023-01-13 10:20 UTC (permalink / raw)
To: Bin Meng, Alistair Francis
Cc: qemu-devel, qemu-riscv, alistair.francis, richard.henderson
Hi Bin!
On 1/12/23 22:28, Bin Meng wrote:
> Hi Daniel,
>
> On Wed, Jan 11, 2023 at 1:03 PM Alistair Francis <alistair23@gmail.com> wrote:
>> On Wed, Jan 11, 2023 at 6:17 AM Daniel Henrique Barboza
>> <dbarboza@ventanamicro.com> wrote:
>>> Hi,
>>>
>>> I found this bug when testing my avocado changes in riscv-to-apply.next.
>>> The sifive_u board, both 32 and 64 bits, stopped booting OpenSBI. The
>>> guest hangs indefinitely.
>>>
>>> Git bisect points that this patch broke things:
>>>
>>> 8c3f35d25e7e98655c609b6c1e9f103b9240f8f8 is the first bad commit
>>> commit 8c3f35d25e7e98655c609b6c1e9f103b9240f8f8
>>> Author: Weiwei Li <liweiwei@iscas.ac.cn>
>>> Date: Wed Dec 28 14:20:21 2022 +0800
>>>
>>> target/riscv: add support for Zca extension
>>>
>>> Modify the check for C extension to Zca (C implies Zca)
>>> (https://github.com/alistair23/qemu/commit/8c3f35d25e7e98655c609b6c1e9f103b9240f8f8)
>>>
>>>
>>> But this patch per se isn't doing anything wrong. The root of the
>>> problem is that this patch makes assumptions based on the previous
>>> patch:
>>>
>>> commit a2b409aa6cadc1ed9715e1ab916ddd3dade0ba85
>>> Author: Weiwei Li <liweiwei@iscas.ac.cn>
>>> Date: Wed Dec 28 14:20:20 2022 +0800
>>>
>>> target/riscv: add cfg properties for Zc* extension
>>> (https://github.com/alistair23/qemu/commit/a2b409aa6cadc1ed9715e1ab916ddd3dade0ba85)
>>>
>>> Which added a lot of logic and assumptions that are being skipped by all
>>> the SiFive boards because, during riscv_cpu_realize(), we have this
>>> code:
>>>
>>> /* If only MISA_EXT is unset for misa, then set it from properties */
>>> if (env->misa_ext == 0) {
>>> uint32_t ext = 0;
>>> (...)
>>> }
>>>
>>> In short, we have a lot of code that are being skipped by all SiFive
>>> CPUs because these CPUs are setting a non-zero value in set_misa() in
>>> their respective cpu_init() functions.
>>>
>>> It's possible to just hack in and fix the SiFive problem in isolate, but
>>> I believe we can do better and allow all riscv_cpu_realize() to be executed
>>> for all CPUs, regardless of what they've done during their cpu_init().
>>>
>>>
>>> Daniel Henrique Barboza (2):
>>> target/riscv/cpu: set cpu->cfg in register_cpu_props()
>>> target/riscv/cpu.c: do not skip misa logic in riscv_cpu_realize()
>> Thanks for the patches
>>
>> I have rebased these onto the latest master and dropped the other
>> series. That way when the other series is applied we don't break
>> bisectability.
> It seems these 2 patches are already in Alistair's tree.
>
> Richard had a suggestion for patch 1 and I had some minor comments
> too. Do you plan to resend a v2 for that?
I'll re-send the v2 with your comments addressed.
About Richard's suggestion, I believe I replied that it would require more thought
because, as it is now, it would break boards that are setting their properties after
register_cpu_props(). The overall simplification of the cpu_init() code across all
RISC-V boards is good thing to do in the future as a follow up, IMO.
Thanks,
Daniel
>
> Regards,
> Bin
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2023-01-13 10:21 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-10 20:14 [PATCH 0/2] target/riscv/cpu: fix sifive_u 32/64bits boot in riscv-to-apply.next Daniel Henrique Barboza
2023-01-10 20:14 ` [PATCH 1/2] target/riscv/cpu: set cpu->cfg in register_cpu_props() Daniel Henrique Barboza
2023-01-10 22:52 ` Alistair Francis
2023-01-11 5:39 ` Richard Henderson
2023-01-11 15:51 ` Daniel Henrique Barboza
2023-01-11 5:55 ` Bin Meng
2023-01-10 20:14 ` [PATCH 2/2] target/riscv/cpu.c: do not skip misa logic in riscv_cpu_realize() Daniel Henrique Barboza
2023-01-10 22:53 ` Alistair Francis
2023-01-11 5:56 ` Bin Meng
2023-01-10 22:27 ` [PATCH 0/2] target/riscv/cpu: fix sifive_u 32/64bits boot in riscv-to-apply.next Daniel Henrique Barboza
2023-01-11 5:01 ` Alistair Francis
2023-01-13 1:28 ` Bin Meng
2023-01-13 10:20 ` Daniel Henrique Barboza
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.