| Message ID | 20230110201405.247785-2-dbarboza@ventanamicro.com |
|---|---|
| State | New |
| Headers | show |
| Series | target/riscv/cpu: fix sifive_u 32/64bits boot in riscv-to-apply.next | expand |
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 > >
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~
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
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~
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')
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(+)