* [PATCH 0/3] target/riscv: profile handling fixes @ 2025-05-20 17:23 Daniel Henrique Barboza 2025-05-20 17:23 ` [PATCH 1/3] target/riscv/tcg: restrict satp_mode changes in cpu_set_profile Daniel Henrique Barboza ` (4 more replies) 0 siblings, 5 replies; 15+ messages in thread From: Daniel Henrique Barboza @ 2025-05-20 17:23 UTC (permalink / raw) To: qemu-devel Cc: qemu-riscv, alistair.francis, liwei1518, zhiwei_liu, palmer, ajones, bjorn, Daniel Henrique Barboza Hi, The motivation of this short series is to fix a reported in [1]. A couple of bugs were fixed along the way. Björn, these patches should remediate the situation you're experiencing. Patches based on master. [1] https://lore.kernel.org/qemu-riscv/87y0usiz22.fsf@all.your.base.are.belong.to.us/ Daniel Henrique Barboza (3): target/riscv/tcg: restrict satp_mode changes in cpu_set_profile target/riscv/tcg: decouple profile enablement from user prop target/riscv: add profile->present flag target/riscv/cpu.h | 15 ++++ target/riscv/riscv-qmp-cmds.c | 2 +- target/riscv/tcg/tcg-cpu.c | 138 +++++++++++++++++----------------- 3 files changed, 86 insertions(+), 69 deletions(-) -- 2.49.0 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/3] target/riscv/tcg: restrict satp_mode changes in cpu_set_profile 2025-05-20 17:23 [PATCH 0/3] target/riscv: profile handling fixes Daniel Henrique Barboza @ 2025-05-20 17:23 ` Daniel Henrique Barboza 2025-05-21 13:47 ` Andrew Jones ` (2 more replies) 2025-05-20 17:23 ` [PATCH 2/3] target/riscv/tcg: decouple profile enablement from user prop Daniel Henrique Barboza ` (3 subsequent siblings) 4 siblings, 3 replies; 15+ messages in thread From: Daniel Henrique Barboza @ 2025-05-20 17:23 UTC (permalink / raw) To: qemu-devel Cc: qemu-riscv, alistair.francis, liwei1518, zhiwei_liu, palmer, ajones, bjorn, Daniel Henrique Barboza We're changing 'mmu' to true regardless of whether the profile is being enabled or not, and at the same time we're changing satp_mode to profile->enabled. This will promote a situation where we'll set mmu=on without a virtual memory mode, which is a mistake. Only touch 'mmu' and satp_mode if the profile is being enabled. Suggested-by: Andrew Jones <ajones@ventanamicro.com> Fixes: 55398025e7 ("target/riscv: add satp_mode profile support") Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> --- target/riscv/tcg/tcg-cpu.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c index 55e00972b7..7f93414a76 100644 --- a/target/riscv/tcg/tcg-cpu.c +++ b/target/riscv/tcg/tcg-cpu.c @@ -1338,16 +1338,16 @@ static void cpu_set_profile(Object *obj, Visitor *v, const char *name, if (profile->enabled) { cpu->env.priv_ver = profile->priv_spec; - } #ifndef CONFIG_USER_ONLY - if (profile->satp_mode != RISCV_PROFILE_ATTR_UNUSED) { - object_property_set_bool(obj, "mmu", true, NULL); - const char *satp_prop = satp_mode_str(profile->satp_mode, - riscv_cpu_is_32bit(cpu)); - object_property_set_bool(obj, satp_prop, profile->enabled, NULL); - } + if (profile->satp_mode != RISCV_PROFILE_ATTR_UNUSED) { + object_property_set_bool(obj, "mmu", true, NULL); + const char *satp_prop = satp_mode_str(profile->satp_mode, + riscv_cpu_is_32bit(cpu)); + object_property_set_bool(obj, satp_prop, true, NULL); + } #endif + } for (i = 0; misa_bits[i] != 0; i++) { uint32_t bit = misa_bits[i]; -- 2.49.0 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] target/riscv/tcg: restrict satp_mode changes in cpu_set_profile 2025-05-20 17:23 ` [PATCH 1/3] target/riscv/tcg: restrict satp_mode changes in cpu_set_profile Daniel Henrique Barboza @ 2025-05-21 13:47 ` Andrew Jones 2025-05-21 16:30 ` Björn Töpel 2025-05-29 3:03 ` Alistair Francis 2 siblings, 0 replies; 15+ messages in thread From: Andrew Jones @ 2025-05-21 13:47 UTC (permalink / raw) To: Daniel Henrique Barboza Cc: qemu-devel, qemu-riscv, alistair.francis, liwei1518, zhiwei_liu, palmer, bjorn On Tue, May 20, 2025 at 02:23:34PM -0300, Daniel Henrique Barboza wrote: > We're changing 'mmu' to true regardless of whether the profile is > being enabled or not, and at the same time we're changing satp_mode to > profile->enabled. > > This will promote a situation where we'll set mmu=on without a virtual > memory mode, which is a mistake. > > Only touch 'mmu' and satp_mode if the profile is being enabled. > > Suggested-by: Andrew Jones <ajones@ventanamicro.com> > Fixes: 55398025e7 ("target/riscv: add satp_mode profile support") > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> > --- > target/riscv/tcg/tcg-cpu.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c > index 55e00972b7..7f93414a76 100644 > --- a/target/riscv/tcg/tcg-cpu.c > +++ b/target/riscv/tcg/tcg-cpu.c > @@ -1338,16 +1338,16 @@ static void cpu_set_profile(Object *obj, Visitor *v, const char *name, > > if (profile->enabled) { > cpu->env.priv_ver = profile->priv_spec; > - } > > #ifndef CONFIG_USER_ONLY > - if (profile->satp_mode != RISCV_PROFILE_ATTR_UNUSED) { > - object_property_set_bool(obj, "mmu", true, NULL); > - const char *satp_prop = satp_mode_str(profile->satp_mode, > - riscv_cpu_is_32bit(cpu)); > - object_property_set_bool(obj, satp_prop, profile->enabled, NULL); > - } > + if (profile->satp_mode != RISCV_PROFILE_ATTR_UNUSED) { > + object_property_set_bool(obj, "mmu", true, NULL); > + const char *satp_prop = satp_mode_str(profile->satp_mode, > + riscv_cpu_is_32bit(cpu)); > + object_property_set_bool(obj, satp_prop, true, NULL); > + } > #endif > + } > > for (i = 0; misa_bits[i] != 0; i++) { > uint32_t bit = misa_bits[i]; > -- > 2.49.0 > Reviewed-by: Andrew Jones <ajones@ventanamicro.com> ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] target/riscv/tcg: restrict satp_mode changes in cpu_set_profile 2025-05-20 17:23 ` [PATCH 1/3] target/riscv/tcg: restrict satp_mode changes in cpu_set_profile Daniel Henrique Barboza 2025-05-21 13:47 ` Andrew Jones @ 2025-05-21 16:30 ` Björn Töpel 2025-05-29 3:03 ` Alistair Francis 2 siblings, 0 replies; 15+ messages in thread From: Björn Töpel @ 2025-05-21 16:30 UTC (permalink / raw) To: Daniel Henrique Barboza, qemu-devel Cc: qemu-riscv, alistair.francis, liwei1518, zhiwei_liu, palmer, ajones, Daniel Henrique Barboza Daniel Henrique Barboza <dbarboza@ventanamicro.com> writes: > We're changing 'mmu' to true regardless of whether the profile is > being enabled or not, and at the same time we're changing satp_mode to > profile->enabled. > > This will promote a situation where we'll set mmu=on without a virtual > memory mode, which is a mistake. > > Only touch 'mmu' and satp_mode if the profile is being enabled. > > Suggested-by: Andrew Jones <ajones@ventanamicro.com> > Fixes: 55398025e7 ("target/riscv: add satp_mode profile support") > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> Reviewed-by: Björn Töpel <bjorn@rivosinc.com> ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] target/riscv/tcg: restrict satp_mode changes in cpu_set_profile 2025-05-20 17:23 ` [PATCH 1/3] target/riscv/tcg: restrict satp_mode changes in cpu_set_profile Daniel Henrique Barboza 2025-05-21 13:47 ` Andrew Jones 2025-05-21 16:30 ` Björn Töpel @ 2025-05-29 3:03 ` Alistair Francis 2 siblings, 0 replies; 15+ messages in thread From: Alistair Francis @ 2025-05-29 3:03 UTC (permalink / raw) To: Daniel Henrique Barboza Cc: qemu-devel, qemu-riscv, alistair.francis, liwei1518, zhiwei_liu, palmer, ajones, bjorn On Wed, May 21, 2025 at 3:24 AM Daniel Henrique Barboza <dbarboza@ventanamicro.com> wrote: > > We're changing 'mmu' to true regardless of whether the profile is > being enabled or not, and at the same time we're changing satp_mode to > profile->enabled. > > This will promote a situation where we'll set mmu=on without a virtual > memory mode, which is a mistake. > > Only touch 'mmu' and satp_mode if the profile is being enabled. > > Suggested-by: Andrew Jones <ajones@ventanamicro.com> > Fixes: 55398025e7 ("target/riscv: add satp_mode profile support") > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair > --- > target/riscv/tcg/tcg-cpu.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c > index 55e00972b7..7f93414a76 100644 > --- a/target/riscv/tcg/tcg-cpu.c > +++ b/target/riscv/tcg/tcg-cpu.c > @@ -1338,16 +1338,16 @@ static void cpu_set_profile(Object *obj, Visitor *v, const char *name, > > if (profile->enabled) { > cpu->env.priv_ver = profile->priv_spec; > - } > > #ifndef CONFIG_USER_ONLY > - if (profile->satp_mode != RISCV_PROFILE_ATTR_UNUSED) { > - object_property_set_bool(obj, "mmu", true, NULL); > - const char *satp_prop = satp_mode_str(profile->satp_mode, > - riscv_cpu_is_32bit(cpu)); > - object_property_set_bool(obj, satp_prop, profile->enabled, NULL); > - } > + if (profile->satp_mode != RISCV_PROFILE_ATTR_UNUSED) { > + object_property_set_bool(obj, "mmu", true, NULL); > + const char *satp_prop = satp_mode_str(profile->satp_mode, > + riscv_cpu_is_32bit(cpu)); > + object_property_set_bool(obj, satp_prop, true, NULL); > + } > #endif > + } > > for (i = 0; misa_bits[i] != 0; i++) { > uint32_t bit = misa_bits[i]; > -- > 2.49.0 > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/3] target/riscv/tcg: decouple profile enablement from user prop 2025-05-20 17:23 [PATCH 0/3] target/riscv: profile handling fixes Daniel Henrique Barboza 2025-05-20 17:23 ` [PATCH 1/3] target/riscv/tcg: restrict satp_mode changes in cpu_set_profile Daniel Henrique Barboza @ 2025-05-20 17:23 ` Daniel Henrique Barboza 2025-05-21 13:48 ` Andrew Jones ` (2 more replies) 2025-05-20 17:23 ` [PATCH 3/3] target/riscv: add profile->present flag Daniel Henrique Barboza ` (2 subsequent siblings) 4 siblings, 3 replies; 15+ messages in thread From: Daniel Henrique Barboza @ 2025-05-20 17:23 UTC (permalink / raw) To: qemu-devel Cc: qemu-riscv, alistair.francis, liwei1518, zhiwei_liu, palmer, ajones, bjorn, Daniel Henrique Barboza We have code in riscv_cpu_add_profiles() to enable a profile right away in case a CPU chose the profile during its cpu_init(). But we're using the user callback option to do so, setting profile->user_set. Create a new helper that does all the grunt work to enable/disable a given profile. Use this new helper in the cases where we want a CPU to be compatible to a certain profile, leaving the user callback to be used exclusively by users. Fixes: fba92a92e3 ("target/riscv: add 'rva22u64' CPU") Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> --- target/riscv/tcg/tcg-cpu.c | 127 +++++++++++++++++++------------------ 1 file changed, 67 insertions(+), 60 deletions(-) diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c index 7f93414a76..af202c92a3 100644 --- a/target/riscv/tcg/tcg-cpu.c +++ b/target/riscv/tcg/tcg-cpu.c @@ -1139,6 +1139,70 @@ static bool riscv_cpu_is_generic(Object *cpu_obj) return object_dynamic_cast(cpu_obj, TYPE_RISCV_DYNAMIC_CPU) != NULL; } +static void riscv_cpu_set_profile(RISCVCPU *cpu, + RISCVCPUProfile *profile, + bool enabled) +{ + int i, ext_offset; + + if (profile->u_parent != NULL) { + riscv_cpu_set_profile(cpu, profile->u_parent, enabled); + } + + if (profile->s_parent != NULL) { + riscv_cpu_set_profile(cpu, profile->s_parent, enabled); + } + + profile->enabled = enabled; + + if (profile->enabled) { + cpu->env.priv_ver = profile->priv_spec; + +#ifndef CONFIG_USER_ONLY + if (profile->satp_mode != RISCV_PROFILE_ATTR_UNUSED) { + object_property_set_bool(OBJECT(cpu), "mmu", true, NULL); + const char *satp_prop = satp_mode_str(profile->satp_mode, + riscv_cpu_is_32bit(cpu)); + object_property_set_bool(OBJECT(cpu), satp_prop, true, NULL); + } +#endif + } + + for (i = 0; misa_bits[i] != 0; i++) { + uint32_t bit = misa_bits[i]; + + if (!(profile->misa_ext & bit)) { + continue; + } + + if (bit == RVI && !profile->enabled) { + /* + * Disabling profiles will not disable the base + * ISA RV64I. + */ + continue; + } + + cpu_misa_ext_add_user_opt(bit, profile->enabled); + riscv_cpu_write_misa_bit(cpu, bit, profile->enabled); + } + + for (i = 0; profile->ext_offsets[i] != RISCV_PROFILE_EXT_LIST_END; i++) { + ext_offset = profile->ext_offsets[i]; + + if (profile->enabled) { + if (cpu_cfg_offset_is_named_feat(ext_offset)) { + riscv_cpu_enable_named_feat(cpu, ext_offset); + } + + cpu_bump_multi_ext_priv_ver(&cpu->env, ext_offset); + } + + cpu_cfg_ext_add_user_opt(ext_offset, profile->enabled); + isa_ext_update_enabled(cpu, ext_offset, profile->enabled); + } +} + /* * We'll get here via the following path: * @@ -1305,7 +1369,6 @@ static void cpu_set_profile(Object *obj, Visitor *v, const char *name, RISCVCPUProfile *profile = opaque; RISCVCPU *cpu = RISCV_CPU(obj); bool value; - int i, ext_offset; if (riscv_cpu_is_vendor(obj)) { error_setg(errp, "Profile %s is not available for vendor CPUs", @@ -1324,64 +1387,8 @@ static void cpu_set_profile(Object *obj, Visitor *v, const char *name, } profile->user_set = true; - profile->enabled = value; - - if (profile->u_parent != NULL) { - object_property_set_bool(obj, profile->u_parent->name, - profile->enabled, NULL); - } - - if (profile->s_parent != NULL) { - object_property_set_bool(obj, profile->s_parent->name, - profile->enabled, NULL); - } - - if (profile->enabled) { - cpu->env.priv_ver = profile->priv_spec; - -#ifndef CONFIG_USER_ONLY - if (profile->satp_mode != RISCV_PROFILE_ATTR_UNUSED) { - object_property_set_bool(obj, "mmu", true, NULL); - const char *satp_prop = satp_mode_str(profile->satp_mode, - riscv_cpu_is_32bit(cpu)); - object_property_set_bool(obj, satp_prop, true, NULL); - } -#endif - } - - for (i = 0; misa_bits[i] != 0; i++) { - uint32_t bit = misa_bits[i]; - - if (!(profile->misa_ext & bit)) { - continue; - } - if (bit == RVI && !profile->enabled) { - /* - * Disabling profiles will not disable the base - * ISA RV64I. - */ - continue; - } - - cpu_misa_ext_add_user_opt(bit, profile->enabled); - riscv_cpu_write_misa_bit(cpu, bit, profile->enabled); - } - - for (i = 0; profile->ext_offsets[i] != RISCV_PROFILE_EXT_LIST_END; i++) { - ext_offset = profile->ext_offsets[i]; - - if (profile->enabled) { - if (cpu_cfg_offset_is_named_feat(ext_offset)) { - riscv_cpu_enable_named_feat(cpu, ext_offset); - } - - cpu_bump_multi_ext_priv_ver(&cpu->env, ext_offset); - } - - cpu_cfg_ext_add_user_opt(ext_offset, profile->enabled); - isa_ext_update_enabled(cpu, ext_offset, profile->enabled); - } + riscv_cpu_set_profile(cpu, profile, value); } static void cpu_get_profile(Object *obj, Visitor *v, const char *name, @@ -1396,7 +1403,7 @@ static void cpu_get_profile(Object *obj, Visitor *v, const char *name, static void riscv_cpu_add_profiles(Object *cpu_obj) { for (int i = 0; riscv_profiles[i] != NULL; i++) { - const RISCVCPUProfile *profile = riscv_profiles[i]; + RISCVCPUProfile *profile = riscv_profiles[i]; object_property_add(cpu_obj, profile->name, "bool", cpu_get_profile, cpu_set_profile, @@ -1408,7 +1415,7 @@ static void riscv_cpu_add_profiles(Object *cpu_obj) * case. */ if (profile->enabled) { - object_property_set_bool(cpu_obj, profile->name, true, NULL); + riscv_cpu_set_profile(RISCV_CPU(cpu_obj), profile, true); } } } -- 2.49.0 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] target/riscv/tcg: decouple profile enablement from user prop 2025-05-20 17:23 ` [PATCH 2/3] target/riscv/tcg: decouple profile enablement from user prop Daniel Henrique Barboza @ 2025-05-21 13:48 ` Andrew Jones 2025-05-21 16:34 ` Björn Töpel 2025-05-29 3:08 ` Alistair Francis 2 siblings, 0 replies; 15+ messages in thread From: Andrew Jones @ 2025-05-21 13:48 UTC (permalink / raw) To: Daniel Henrique Barboza Cc: qemu-devel, qemu-riscv, alistair.francis, liwei1518, zhiwei_liu, palmer, bjorn On Tue, May 20, 2025 at 02:23:35PM -0300, Daniel Henrique Barboza wrote: > We have code in riscv_cpu_add_profiles() to enable a profile right away > in case a CPU chose the profile during its cpu_init(). But we're using > the user callback option to do so, setting profile->user_set. > > Create a new helper that does all the grunt work to enable/disable a > given profile. Use this new helper in the cases where we want a CPU to > be compatible to a certain profile, leaving the user callback to be used > exclusively by users. > > Fixes: fba92a92e3 ("target/riscv: add 'rva22u64' CPU") > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> > --- > target/riscv/tcg/tcg-cpu.c | 127 +++++++++++++++++++------------------ > 1 file changed, 67 insertions(+), 60 deletions(-) > > diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c > index 7f93414a76..af202c92a3 100644 > --- a/target/riscv/tcg/tcg-cpu.c > +++ b/target/riscv/tcg/tcg-cpu.c > @@ -1139,6 +1139,70 @@ static bool riscv_cpu_is_generic(Object *cpu_obj) > return object_dynamic_cast(cpu_obj, TYPE_RISCV_DYNAMIC_CPU) != NULL; > } > > +static void riscv_cpu_set_profile(RISCVCPU *cpu, > + RISCVCPUProfile *profile, > + bool enabled) > +{ > + int i, ext_offset; > + > + if (profile->u_parent != NULL) { > + riscv_cpu_set_profile(cpu, profile->u_parent, enabled); > + } > + > + if (profile->s_parent != NULL) { > + riscv_cpu_set_profile(cpu, profile->s_parent, enabled); > + } > + > + profile->enabled = enabled; > + > + if (profile->enabled) { > + cpu->env.priv_ver = profile->priv_spec; > + > +#ifndef CONFIG_USER_ONLY > + if (profile->satp_mode != RISCV_PROFILE_ATTR_UNUSED) { > + object_property_set_bool(OBJECT(cpu), "mmu", true, NULL); > + const char *satp_prop = satp_mode_str(profile->satp_mode, > + riscv_cpu_is_32bit(cpu)); > + object_property_set_bool(OBJECT(cpu), satp_prop, true, NULL); > + } > +#endif > + } > + > + for (i = 0; misa_bits[i] != 0; i++) { > + uint32_t bit = misa_bits[i]; > + > + if (!(profile->misa_ext & bit)) { > + continue; > + } > + > + if (bit == RVI && !profile->enabled) { > + /* > + * Disabling profiles will not disable the base > + * ISA RV64I. > + */ > + continue; > + } > + > + cpu_misa_ext_add_user_opt(bit, profile->enabled); > + riscv_cpu_write_misa_bit(cpu, bit, profile->enabled); > + } > + > + for (i = 0; profile->ext_offsets[i] != RISCV_PROFILE_EXT_LIST_END; i++) { > + ext_offset = profile->ext_offsets[i]; > + > + if (profile->enabled) { > + if (cpu_cfg_offset_is_named_feat(ext_offset)) { > + riscv_cpu_enable_named_feat(cpu, ext_offset); > + } > + > + cpu_bump_multi_ext_priv_ver(&cpu->env, ext_offset); > + } > + > + cpu_cfg_ext_add_user_opt(ext_offset, profile->enabled); > + isa_ext_update_enabled(cpu, ext_offset, profile->enabled); > + } > +} > + > /* > * We'll get here via the following path: > * > @@ -1305,7 +1369,6 @@ static void cpu_set_profile(Object *obj, Visitor *v, const char *name, > RISCVCPUProfile *profile = opaque; > RISCVCPU *cpu = RISCV_CPU(obj); > bool value; > - int i, ext_offset; > > if (riscv_cpu_is_vendor(obj)) { > error_setg(errp, "Profile %s is not available for vendor CPUs", > @@ -1324,64 +1387,8 @@ static void cpu_set_profile(Object *obj, Visitor *v, const char *name, > } > > profile->user_set = true; > - profile->enabled = value; > - > - if (profile->u_parent != NULL) { > - object_property_set_bool(obj, profile->u_parent->name, > - profile->enabled, NULL); > - } > - > - if (profile->s_parent != NULL) { > - object_property_set_bool(obj, profile->s_parent->name, > - profile->enabled, NULL); > - } > - > - if (profile->enabled) { > - cpu->env.priv_ver = profile->priv_spec; > - > -#ifndef CONFIG_USER_ONLY > - if (profile->satp_mode != RISCV_PROFILE_ATTR_UNUSED) { > - object_property_set_bool(obj, "mmu", true, NULL); > - const char *satp_prop = satp_mode_str(profile->satp_mode, > - riscv_cpu_is_32bit(cpu)); > - object_property_set_bool(obj, satp_prop, true, NULL); > - } > -#endif > - } > - > - for (i = 0; misa_bits[i] != 0; i++) { > - uint32_t bit = misa_bits[i]; > - > - if (!(profile->misa_ext & bit)) { > - continue; > - } > > - if (bit == RVI && !profile->enabled) { > - /* > - * Disabling profiles will not disable the base > - * ISA RV64I. > - */ > - continue; > - } > - > - cpu_misa_ext_add_user_opt(bit, profile->enabled); > - riscv_cpu_write_misa_bit(cpu, bit, profile->enabled); > - } > - > - for (i = 0; profile->ext_offsets[i] != RISCV_PROFILE_EXT_LIST_END; i++) { > - ext_offset = profile->ext_offsets[i]; > - > - if (profile->enabled) { > - if (cpu_cfg_offset_is_named_feat(ext_offset)) { > - riscv_cpu_enable_named_feat(cpu, ext_offset); > - } > - > - cpu_bump_multi_ext_priv_ver(&cpu->env, ext_offset); > - } > - > - cpu_cfg_ext_add_user_opt(ext_offset, profile->enabled); > - isa_ext_update_enabled(cpu, ext_offset, profile->enabled); > - } > + riscv_cpu_set_profile(cpu, profile, value); > } > > static void cpu_get_profile(Object *obj, Visitor *v, const char *name, > @@ -1396,7 +1403,7 @@ static void cpu_get_profile(Object *obj, Visitor *v, const char *name, > static void riscv_cpu_add_profiles(Object *cpu_obj) > { > for (int i = 0; riscv_profiles[i] != NULL; i++) { > - const RISCVCPUProfile *profile = riscv_profiles[i]; > + RISCVCPUProfile *profile = riscv_profiles[i]; > > object_property_add(cpu_obj, profile->name, "bool", > cpu_get_profile, cpu_set_profile, > @@ -1408,7 +1415,7 @@ static void riscv_cpu_add_profiles(Object *cpu_obj) > * case. > */ > if (profile->enabled) { > - object_property_set_bool(cpu_obj, profile->name, true, NULL); > + riscv_cpu_set_profile(RISCV_CPU(cpu_obj), profile, true); > } > } > } > -- > 2.49.0 > Reviewed-by: Andrew Jones <ajones@ventanamicro.com> ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] target/riscv/tcg: decouple profile enablement from user prop 2025-05-20 17:23 ` [PATCH 2/3] target/riscv/tcg: decouple profile enablement from user prop Daniel Henrique Barboza 2025-05-21 13:48 ` Andrew Jones @ 2025-05-21 16:34 ` Björn Töpel 2025-05-29 3:08 ` Alistair Francis 2 siblings, 0 replies; 15+ messages in thread From: Björn Töpel @ 2025-05-21 16:34 UTC (permalink / raw) To: Daniel Henrique Barboza, qemu-devel Cc: qemu-riscv, alistair.francis, liwei1518, zhiwei_liu, palmer, ajones, Daniel Henrique Barboza Daniel Henrique Barboza <dbarboza@ventanamicro.com> writes: > We have code in riscv_cpu_add_profiles() to enable a profile right away > in case a CPU chose the profile during its cpu_init(). But we're using > the user callback option to do so, setting profile->user_set. > > Create a new helper that does all the grunt work to enable/disable a > given profile. Use this new helper in the cases where we want a CPU to > be compatible to a certain profile, leaving the user callback to be used > exclusively by users. > > Fixes: fba92a92e3 ("target/riscv: add 'rva22u64' CPU") > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> Reviewed-by: Björn Töpel <bjorn@rivosinc.com> ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] target/riscv/tcg: decouple profile enablement from user prop 2025-05-20 17:23 ` [PATCH 2/3] target/riscv/tcg: decouple profile enablement from user prop Daniel Henrique Barboza 2025-05-21 13:48 ` Andrew Jones 2025-05-21 16:34 ` Björn Töpel @ 2025-05-29 3:08 ` Alistair Francis 2 siblings, 0 replies; 15+ messages in thread From: Alistair Francis @ 2025-05-29 3:08 UTC (permalink / raw) To: Daniel Henrique Barboza Cc: qemu-devel, qemu-riscv, alistair.francis, liwei1518, zhiwei_liu, palmer, ajones, bjorn On Wed, May 21, 2025 at 3:24 AM Daniel Henrique Barboza <dbarboza@ventanamicro.com> wrote: > > We have code in riscv_cpu_add_profiles() to enable a profile right away > in case a CPU chose the profile during its cpu_init(). But we're using > the user callback option to do so, setting profile->user_set. > > Create a new helper that does all the grunt work to enable/disable a > given profile. Use this new helper in the cases where we want a CPU to > be compatible to a certain profile, leaving the user callback to be used > exclusively by users. > > Fixes: fba92a92e3 ("target/riscv: add 'rva22u64' CPU") > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair > --- > target/riscv/tcg/tcg-cpu.c | 127 +++++++++++++++++++------------------ > 1 file changed, 67 insertions(+), 60 deletions(-) > > diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c > index 7f93414a76..af202c92a3 100644 > --- a/target/riscv/tcg/tcg-cpu.c > +++ b/target/riscv/tcg/tcg-cpu.c > @@ -1139,6 +1139,70 @@ static bool riscv_cpu_is_generic(Object *cpu_obj) > return object_dynamic_cast(cpu_obj, TYPE_RISCV_DYNAMIC_CPU) != NULL; > } > > +static void riscv_cpu_set_profile(RISCVCPU *cpu, > + RISCVCPUProfile *profile, > + bool enabled) > +{ > + int i, ext_offset; > + > + if (profile->u_parent != NULL) { > + riscv_cpu_set_profile(cpu, profile->u_parent, enabled); > + } > + > + if (profile->s_parent != NULL) { > + riscv_cpu_set_profile(cpu, profile->s_parent, enabled); > + } > + > + profile->enabled = enabled; > + > + if (profile->enabled) { > + cpu->env.priv_ver = profile->priv_spec; > + > +#ifndef CONFIG_USER_ONLY > + if (profile->satp_mode != RISCV_PROFILE_ATTR_UNUSED) { > + object_property_set_bool(OBJECT(cpu), "mmu", true, NULL); > + const char *satp_prop = satp_mode_str(profile->satp_mode, > + riscv_cpu_is_32bit(cpu)); > + object_property_set_bool(OBJECT(cpu), satp_prop, true, NULL); > + } > +#endif > + } > + > + for (i = 0; misa_bits[i] != 0; i++) { > + uint32_t bit = misa_bits[i]; > + > + if (!(profile->misa_ext & bit)) { > + continue; > + } > + > + if (bit == RVI && !profile->enabled) { > + /* > + * Disabling profiles will not disable the base > + * ISA RV64I. > + */ > + continue; > + } > + > + cpu_misa_ext_add_user_opt(bit, profile->enabled); > + riscv_cpu_write_misa_bit(cpu, bit, profile->enabled); > + } > + > + for (i = 0; profile->ext_offsets[i] != RISCV_PROFILE_EXT_LIST_END; i++) { > + ext_offset = profile->ext_offsets[i]; > + > + if (profile->enabled) { > + if (cpu_cfg_offset_is_named_feat(ext_offset)) { > + riscv_cpu_enable_named_feat(cpu, ext_offset); > + } > + > + cpu_bump_multi_ext_priv_ver(&cpu->env, ext_offset); > + } > + > + cpu_cfg_ext_add_user_opt(ext_offset, profile->enabled); > + isa_ext_update_enabled(cpu, ext_offset, profile->enabled); > + } > +} > + > /* > * We'll get here via the following path: > * > @@ -1305,7 +1369,6 @@ static void cpu_set_profile(Object *obj, Visitor *v, const char *name, > RISCVCPUProfile *profile = opaque; > RISCVCPU *cpu = RISCV_CPU(obj); > bool value; > - int i, ext_offset; > > if (riscv_cpu_is_vendor(obj)) { > error_setg(errp, "Profile %s is not available for vendor CPUs", > @@ -1324,64 +1387,8 @@ static void cpu_set_profile(Object *obj, Visitor *v, const char *name, > } > > profile->user_set = true; > - profile->enabled = value; > - > - if (profile->u_parent != NULL) { > - object_property_set_bool(obj, profile->u_parent->name, > - profile->enabled, NULL); > - } > - > - if (profile->s_parent != NULL) { > - object_property_set_bool(obj, profile->s_parent->name, > - profile->enabled, NULL); > - } > - > - if (profile->enabled) { > - cpu->env.priv_ver = profile->priv_spec; > - > -#ifndef CONFIG_USER_ONLY > - if (profile->satp_mode != RISCV_PROFILE_ATTR_UNUSED) { > - object_property_set_bool(obj, "mmu", true, NULL); > - const char *satp_prop = satp_mode_str(profile->satp_mode, > - riscv_cpu_is_32bit(cpu)); > - object_property_set_bool(obj, satp_prop, true, NULL); > - } > -#endif > - } > - > - for (i = 0; misa_bits[i] != 0; i++) { > - uint32_t bit = misa_bits[i]; > - > - if (!(profile->misa_ext & bit)) { > - continue; > - } > > - if (bit == RVI && !profile->enabled) { > - /* > - * Disabling profiles will not disable the base > - * ISA RV64I. > - */ > - continue; > - } > - > - cpu_misa_ext_add_user_opt(bit, profile->enabled); > - riscv_cpu_write_misa_bit(cpu, bit, profile->enabled); > - } > - > - for (i = 0; profile->ext_offsets[i] != RISCV_PROFILE_EXT_LIST_END; i++) { > - ext_offset = profile->ext_offsets[i]; > - > - if (profile->enabled) { > - if (cpu_cfg_offset_is_named_feat(ext_offset)) { > - riscv_cpu_enable_named_feat(cpu, ext_offset); > - } > - > - cpu_bump_multi_ext_priv_ver(&cpu->env, ext_offset); > - } > - > - cpu_cfg_ext_add_user_opt(ext_offset, profile->enabled); > - isa_ext_update_enabled(cpu, ext_offset, profile->enabled); > - } > + riscv_cpu_set_profile(cpu, profile, value); > } > > static void cpu_get_profile(Object *obj, Visitor *v, const char *name, > @@ -1396,7 +1403,7 @@ static void cpu_get_profile(Object *obj, Visitor *v, const char *name, > static void riscv_cpu_add_profiles(Object *cpu_obj) > { > for (int i = 0; riscv_profiles[i] != NULL; i++) { > - const RISCVCPUProfile *profile = riscv_profiles[i]; > + RISCVCPUProfile *profile = riscv_profiles[i]; > > object_property_add(cpu_obj, profile->name, "bool", > cpu_get_profile, cpu_set_profile, > @@ -1408,7 +1415,7 @@ static void riscv_cpu_add_profiles(Object *cpu_obj) > * case. > */ > if (profile->enabled) { > - object_property_set_bool(cpu_obj, profile->name, true, NULL); > + riscv_cpu_set_profile(RISCV_CPU(cpu_obj), profile, true); > } > } > } > -- > 2.49.0 > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 3/3] target/riscv: add profile->present flag 2025-05-20 17:23 [PATCH 0/3] target/riscv: profile handling fixes Daniel Henrique Barboza 2025-05-20 17:23 ` [PATCH 1/3] target/riscv/tcg: restrict satp_mode changes in cpu_set_profile Daniel Henrique Barboza 2025-05-20 17:23 ` [PATCH 2/3] target/riscv/tcg: decouple profile enablement from user prop Daniel Henrique Barboza @ 2025-05-20 17:23 ` Daniel Henrique Barboza 2025-05-21 13:47 ` Andrew Jones ` (2 more replies) 2025-05-21 16:27 ` [PATCH 0/3] target/riscv: profile handling fixes Björn Töpel 2025-05-29 4:32 ` Alistair Francis 4 siblings, 3 replies; 15+ messages in thread From: Daniel Henrique Barboza @ 2025-05-20 17:23 UTC (permalink / raw) To: qemu-devel Cc: qemu-riscv, alistair.francis, liwei1518, zhiwei_liu, palmer, ajones, bjorn, Daniel Henrique Barboza Björn reported in [1] a case where a rv64 CPU is going through the profile code path to enable satp mode. In this case,the amount of extensions on top of the rv64 CPU made it compliant with the RVA22S64 profile during the validation of CPU 0. When the subsequent CPUs were initialized the static profile object has the 'enable' flag set, enabling the profile code path for those CPUs. This happens because we are initializing and realizing each CPU before going to the next, i.e. init and realize CPU0, then init and realize CPU1 and so on. If we change any persistent state during the validation of CPU N it will interfere with the init/realization of CPU N+1. We're using the 'enabled' profile flag to do two distinct things: inform cpu_init() that we want profile extensions to be enabled, and telling QMP that a profile is currently enabled in the CPU. We want to be flexible enough to recognize profile support for all CPUs that has the extension prerequisites, but we do not want to force the profile code path if a profile wasn't set too. Add a new 'present' flag for profiles that will coexist with the 'enabled' flag. Enabling a profile means "we want to switch on all its mandatory extensions". A profile is 'present' if we asserted during validation that the CPU has the needed prerequisites. This means that the case reported by Björn now results in RVA22S64.enabled=false and RVA22S64.present=true. QMP will recognize it as a RVA22 compliant CPU and we won't force the CPU into the profile path. [1] https://lore.kernel.org/qemu-riscv/87y0usiz22.fsf@all.your.base.are.belong.to.us/ Reported-by: Björn Töpel <bjorn@kernel.org> Fixes: 2af005d610 ("target/riscv/tcg: validate profiles during finalize") Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> --- target/riscv/cpu.h | 15 +++++++++++++++ target/riscv/riscv-qmp-cmds.c | 2 +- target/riscv/tcg/tcg-cpu.c | 11 +++-------- 3 files changed, 19 insertions(+), 9 deletions(-) diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h index b56d3afa69..82ca41d55b 100644 --- a/target/riscv/cpu.h +++ b/target/riscv/cpu.h @@ -82,7 +82,22 @@ typedef struct riscv_cpu_profile { struct riscv_cpu_profile *s_parent; const char *name; uint32_t misa_ext; + /* + * The profile is enabled/disabled via command line or + * via cpu_init(). Enabling a profile will add all its + * mandatory extensions in the CPU during init(). + */ bool enabled; + /* + * The profile is present in the CPU, i.e. the current set of + * CPU extensions complies with it. A profile can be enabled + * and not present (e.g. the user disabled a mandatory extension) + * and the other way around (e.g. all mandatory extensions are + * present in a non-profile CPU). + * + * QMP uses this flag. + */ + bool present; bool user_set; int priv_spec; int satp_mode; diff --git a/target/riscv/riscv-qmp-cmds.c b/target/riscv/riscv-qmp-cmds.c index d0a324364d..ad8efd180d 100644 --- a/target/riscv/riscv-qmp-cmds.c +++ b/target/riscv/riscv-qmp-cmds.c @@ -121,7 +121,7 @@ static void riscv_obj_add_profiles_qdict(Object *obj, QDict *qdict_out) for (int i = 0; riscv_profiles[i] != NULL; i++) { profile = riscv_profiles[i]; - value = QOBJECT(qbool_from_bool(profile->enabled)); + value = QOBJECT(qbool_from_bool(profile->present)); qdict_put_obj(qdict_out, profile->name, value); } diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c index af202c92a3..396fac0938 100644 --- a/target/riscv/tcg/tcg-cpu.c +++ b/target/riscv/tcg/tcg-cpu.c @@ -840,16 +840,11 @@ static void riscv_cpu_check_parent_profile(RISCVCPU *cpu, RISCVCPUProfile *profile, RISCVCPUProfile *parent) { - const char *parent_name; - bool parent_enabled; - - if (!profile->enabled || !parent) { + if (!profile->present || !parent) { return; } - parent_name = parent->name; - parent_enabled = object_property_get_bool(OBJECT(cpu), parent_name, NULL); - profile->enabled = parent_enabled; + profile->present = parent->present; } static void riscv_cpu_validate_profile(RISCVCPU *cpu, @@ -910,7 +905,7 @@ static void riscv_cpu_validate_profile(RISCVCPU *cpu, } } - profile->enabled = profile_impl; + profile->present = profile_impl; riscv_cpu_check_parent_profile(cpu, profile, profile->u_parent); riscv_cpu_check_parent_profile(cpu, profile, profile->s_parent); -- 2.49.0 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 3/3] target/riscv: add profile->present flag 2025-05-20 17:23 ` [PATCH 3/3] target/riscv: add profile->present flag Daniel Henrique Barboza @ 2025-05-21 13:47 ` Andrew Jones 2025-05-21 16:33 ` Björn Töpel 2025-05-29 3:11 ` Alistair Francis 2 siblings, 0 replies; 15+ messages in thread From: Andrew Jones @ 2025-05-21 13:47 UTC (permalink / raw) To: Daniel Henrique Barboza Cc: qemu-devel, qemu-riscv, alistair.francis, liwei1518, zhiwei_liu, palmer, bjorn On Tue, May 20, 2025 at 02:23:36PM -0300, Daniel Henrique Barboza wrote: > Björn reported in [1] a case where a rv64 CPU is going through the > profile code path to enable satp mode. In this case,the amount of > extensions on top of the rv64 CPU made it compliant with the RVA22S64 > profile during the validation of CPU 0. When the subsequent CPUs were > initialized the static profile object has the 'enable' flag set, > enabling the profile code path for those CPUs. > > This happens because we are initializing and realizing each CPU before > going to the next, i.e. init and realize CPU0, then init and realize > CPU1 and so on. If we change any persistent state during the validation > of CPU N it will interfere with the init/realization of CPU N+1. > > We're using the 'enabled' profile flag to do two distinct things: inform > cpu_init() that we want profile extensions to be enabled, and telling > QMP that a profile is currently enabled in the CPU. We want to be > flexible enough to recognize profile support for all CPUs that has the > extension prerequisites, but we do not want to force the profile code > path if a profile wasn't set too. > > Add a new 'present' flag for profiles that will coexist with the 'enabled' > flag. Enabling a profile means "we want to switch on all its mandatory > extensions". A profile is 'present' if we asserted during validation > that the CPU has the needed prerequisites. > > This means that the case reported by Björn now results in > RVA22S64.enabled=false and RVA22S64.present=true. QMP will recognize it > as a RVA22 compliant CPU and we won't force the CPU into the profile > path. > > [1] https://lore.kernel.org/qemu-riscv/87y0usiz22.fsf@all.your.base.are.belong.to.us/ > > Reported-by: Björn Töpel <bjorn@kernel.org> > Fixes: 2af005d610 ("target/riscv/tcg: validate profiles during finalize") > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> > > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> Your s-o-b somehow got included twice. > --- > target/riscv/cpu.h | 15 +++++++++++++++ > target/riscv/riscv-qmp-cmds.c | 2 +- > target/riscv/tcg/tcg-cpu.c | 11 +++-------- > 3 files changed, 19 insertions(+), 9 deletions(-) > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h > index b56d3afa69..82ca41d55b 100644 > --- a/target/riscv/cpu.h > +++ b/target/riscv/cpu.h > @@ -82,7 +82,22 @@ typedef struct riscv_cpu_profile { > struct riscv_cpu_profile *s_parent; > const char *name; > uint32_t misa_ext; > + /* > + * The profile is enabled/disabled via command line or > + * via cpu_init(). Enabling a profile will add all its > + * mandatory extensions in the CPU during init(). > + */ > bool enabled; > + /* > + * The profile is present in the CPU, i.e. the current set of > + * CPU extensions complies with it. A profile can be enabled > + * and not present (e.g. the user disabled a mandatory extension) > + * and the other way around (e.g. all mandatory extensions are > + * present in a non-profile CPU). > + * > + * QMP uses this flag. > + */ > + bool present; > bool user_set; > int priv_spec; > int satp_mode; > diff --git a/target/riscv/riscv-qmp-cmds.c b/target/riscv/riscv-qmp-cmds.c > index d0a324364d..ad8efd180d 100644 > --- a/target/riscv/riscv-qmp-cmds.c > +++ b/target/riscv/riscv-qmp-cmds.c > @@ -121,7 +121,7 @@ static void riscv_obj_add_profiles_qdict(Object *obj, QDict *qdict_out) > > for (int i = 0; riscv_profiles[i] != NULL; i++) { > profile = riscv_profiles[i]; > - value = QOBJECT(qbool_from_bool(profile->enabled)); > + value = QOBJECT(qbool_from_bool(profile->present)); > > qdict_put_obj(qdict_out, profile->name, value); > } > diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c > index af202c92a3..396fac0938 100644 > --- a/target/riscv/tcg/tcg-cpu.c > +++ b/target/riscv/tcg/tcg-cpu.c > @@ -840,16 +840,11 @@ static void riscv_cpu_check_parent_profile(RISCVCPU *cpu, > RISCVCPUProfile *profile, > RISCVCPUProfile *parent) > { > - const char *parent_name; > - bool parent_enabled; > - > - if (!profile->enabled || !parent) { > + if (!profile->present || !parent) { > return; > } > > - parent_name = parent->name; > - parent_enabled = object_property_get_bool(OBJECT(cpu), parent_name, NULL); > - profile->enabled = parent_enabled; > + profile->present = parent->present; > } > > static void riscv_cpu_validate_profile(RISCVCPU *cpu, > @@ -910,7 +905,7 @@ static void riscv_cpu_validate_profile(RISCVCPU *cpu, > } > } > > - profile->enabled = profile_impl; > + profile->present = profile_impl; > > riscv_cpu_check_parent_profile(cpu, profile, profile->u_parent); > riscv_cpu_check_parent_profile(cpu, profile, profile->s_parent); > -- > 2.49.0 > Otherwise, Reviewed-by: Andrew Jones <ajones@ventanamicro.com> ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/3] target/riscv: add profile->present flag 2025-05-20 17:23 ` [PATCH 3/3] target/riscv: add profile->present flag Daniel Henrique Barboza 2025-05-21 13:47 ` Andrew Jones @ 2025-05-21 16:33 ` Björn Töpel 2025-05-29 3:11 ` Alistair Francis 2 siblings, 0 replies; 15+ messages in thread From: Björn Töpel @ 2025-05-21 16:33 UTC (permalink / raw) To: Daniel Henrique Barboza, qemu-devel Cc: qemu-riscv, alistair.francis, liwei1518, zhiwei_liu, palmer, ajones, Daniel Henrique Barboza Daniel Henrique Barboza <dbarboza@ventanamicro.com> writes: > Björn reported in [1] a case where a rv64 CPU is going through the > profile code path to enable satp mode. In this case,the amount of > extensions on top of the rv64 CPU made it compliant with the RVA22S64 > profile during the validation of CPU 0. When the subsequent CPUs were > initialized the static profile object has the 'enable' flag set, > enabling the profile code path for those CPUs. > > This happens because we are initializing and realizing each CPU before > going to the next, i.e. init and realize CPU0, then init and realize > CPU1 and so on. If we change any persistent state during the validation > of CPU N it will interfere with the init/realization of CPU N+1. > > We're using the 'enabled' profile flag to do two distinct things: inform > cpu_init() that we want profile extensions to be enabled, and telling > QMP that a profile is currently enabled in the CPU. We want to be > flexible enough to recognize profile support for all CPUs that has the > extension prerequisites, but we do not want to force the profile code > path if a profile wasn't set too. > > Add a new 'present' flag for profiles that will coexist with the 'enabled' > flag. Enabling a profile means "we want to switch on all its mandatory > extensions". A profile is 'present' if we asserted during validation > that the CPU has the needed prerequisites. > > This means that the case reported by Björn now results in > RVA22S64.enabled=false and RVA22S64.present=true. QMP will recognize it > as a RVA22 compliant CPU and we won't force the CPU into the profile > path. > > [1] https://lore.kernel.org/qemu-riscv/87y0usiz22.fsf@all.your.base.are.belong.to.us/ > > Reported-by: Björn Töpel <bjorn@kernel.org> > Fixes: 2af005d610 ("target/riscv/tcg: validate profiles during finalize") > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> Reviewed-by: Björn Töpel <bjorn@rivosinc.com> ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/3] target/riscv: add profile->present flag 2025-05-20 17:23 ` [PATCH 3/3] target/riscv: add profile->present flag Daniel Henrique Barboza 2025-05-21 13:47 ` Andrew Jones 2025-05-21 16:33 ` Björn Töpel @ 2025-05-29 3:11 ` Alistair Francis 2 siblings, 0 replies; 15+ messages in thread From: Alistair Francis @ 2025-05-29 3:11 UTC (permalink / raw) To: Daniel Henrique Barboza Cc: qemu-devel, qemu-riscv, alistair.francis, liwei1518, zhiwei_liu, palmer, ajones, bjorn On Wed, May 21, 2025 at 3:24 AM Daniel Henrique Barboza <dbarboza@ventanamicro.com> wrote: > > Björn reported in [1] a case where a rv64 CPU is going through the > profile code path to enable satp mode. In this case,the amount of > extensions on top of the rv64 CPU made it compliant with the RVA22S64 > profile during the validation of CPU 0. When the subsequent CPUs were > initialized the static profile object has the 'enable' flag set, > enabling the profile code path for those CPUs. > > This happens because we are initializing and realizing each CPU before > going to the next, i.e. init and realize CPU0, then init and realize > CPU1 and so on. If we change any persistent state during the validation > of CPU N it will interfere with the init/realization of CPU N+1. > > We're using the 'enabled' profile flag to do two distinct things: inform > cpu_init() that we want profile extensions to be enabled, and telling > QMP that a profile is currently enabled in the CPU. We want to be > flexible enough to recognize profile support for all CPUs that has the > extension prerequisites, but we do not want to force the profile code > path if a profile wasn't set too. > > Add a new 'present' flag for profiles that will coexist with the 'enabled' > flag. Enabling a profile means "we want to switch on all its mandatory > extensions". A profile is 'present' if we asserted during validation > that the CPU has the needed prerequisites. > > This means that the case reported by Björn now results in > RVA22S64.enabled=false and RVA22S64.present=true. QMP will recognize it > as a RVA22 compliant CPU and we won't force the CPU into the profile > path. > > [1] https://lore.kernel.org/qemu-riscv/87y0usiz22.fsf@all.your.base.are.belong.to.us/ > > Reported-by: Björn Töpel <bjorn@kernel.org> > Fixes: 2af005d610 ("target/riscv/tcg: validate profiles during finalize") > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> > > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair > --- > target/riscv/cpu.h | 15 +++++++++++++++ > target/riscv/riscv-qmp-cmds.c | 2 +- > target/riscv/tcg/tcg-cpu.c | 11 +++-------- > 3 files changed, 19 insertions(+), 9 deletions(-) > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h > index b56d3afa69..82ca41d55b 100644 > --- a/target/riscv/cpu.h > +++ b/target/riscv/cpu.h > @@ -82,7 +82,22 @@ typedef struct riscv_cpu_profile { > struct riscv_cpu_profile *s_parent; > const char *name; > uint32_t misa_ext; > + /* > + * The profile is enabled/disabled via command line or > + * via cpu_init(). Enabling a profile will add all its > + * mandatory extensions in the CPU during init(). > + */ > bool enabled; > + /* > + * The profile is present in the CPU, i.e. the current set of > + * CPU extensions complies with it. A profile can be enabled > + * and not present (e.g. the user disabled a mandatory extension) > + * and the other way around (e.g. all mandatory extensions are > + * present in a non-profile CPU). > + * > + * QMP uses this flag. > + */ > + bool present; > bool user_set; > int priv_spec; > int satp_mode; > diff --git a/target/riscv/riscv-qmp-cmds.c b/target/riscv/riscv-qmp-cmds.c > index d0a324364d..ad8efd180d 100644 > --- a/target/riscv/riscv-qmp-cmds.c > +++ b/target/riscv/riscv-qmp-cmds.c > @@ -121,7 +121,7 @@ static void riscv_obj_add_profiles_qdict(Object *obj, QDict *qdict_out) > > for (int i = 0; riscv_profiles[i] != NULL; i++) { > profile = riscv_profiles[i]; > - value = QOBJECT(qbool_from_bool(profile->enabled)); > + value = QOBJECT(qbool_from_bool(profile->present)); > > qdict_put_obj(qdict_out, profile->name, value); > } > diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c > index af202c92a3..396fac0938 100644 > --- a/target/riscv/tcg/tcg-cpu.c > +++ b/target/riscv/tcg/tcg-cpu.c > @@ -840,16 +840,11 @@ static void riscv_cpu_check_parent_profile(RISCVCPU *cpu, > RISCVCPUProfile *profile, > RISCVCPUProfile *parent) > { > - const char *parent_name; > - bool parent_enabled; > - > - if (!profile->enabled || !parent) { > + if (!profile->present || !parent) { > return; > } > > - parent_name = parent->name; > - parent_enabled = object_property_get_bool(OBJECT(cpu), parent_name, NULL); > - profile->enabled = parent_enabled; > + profile->present = parent->present; > } > > static void riscv_cpu_validate_profile(RISCVCPU *cpu, > @@ -910,7 +905,7 @@ static void riscv_cpu_validate_profile(RISCVCPU *cpu, > } > } > > - profile->enabled = profile_impl; > + profile->present = profile_impl; > > riscv_cpu_check_parent_profile(cpu, profile, profile->u_parent); > riscv_cpu_check_parent_profile(cpu, profile, profile->s_parent); > -- > 2.49.0 > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/3] target/riscv: profile handling fixes 2025-05-20 17:23 [PATCH 0/3] target/riscv: profile handling fixes Daniel Henrique Barboza ` (2 preceding siblings ...) 2025-05-20 17:23 ` [PATCH 3/3] target/riscv: add profile->present flag Daniel Henrique Barboza @ 2025-05-21 16:27 ` Björn Töpel 2025-05-29 4:32 ` Alistair Francis 4 siblings, 0 replies; 15+ messages in thread From: Björn Töpel @ 2025-05-21 16:27 UTC (permalink / raw) To: Daniel Henrique Barboza, qemu-devel Cc: qemu-riscv, alistair.francis, liwei1518, zhiwei_liu, palmer, ajones, Daniel Henrique Barboza Daniel! Daniel Henrique Barboza <dbarboza@ventanamicro.com> writes: > Hi, > > The motivation of this short series is to fix a reported in [1]. A > couple of bugs were fixed along the way. > > Björn, these patches should remediate the situation you're experiencing. Thanks for the fast turn-around! This indeed fixes the hart0 boot! Tested-by: Björn Töpel <bjorn@rivosinc.com> ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/3] target/riscv: profile handling fixes 2025-05-20 17:23 [PATCH 0/3] target/riscv: profile handling fixes Daniel Henrique Barboza ` (3 preceding siblings ...) 2025-05-21 16:27 ` [PATCH 0/3] target/riscv: profile handling fixes Björn Töpel @ 2025-05-29 4:32 ` Alistair Francis 4 siblings, 0 replies; 15+ messages in thread From: Alistair Francis @ 2025-05-29 4:32 UTC (permalink / raw) To: Daniel Henrique Barboza Cc: qemu-devel, qemu-riscv, alistair.francis, liwei1518, zhiwei_liu, palmer, ajones, bjorn On Wed, May 21, 2025 at 3:24 AM Daniel Henrique Barboza <dbarboza@ventanamicro.com> wrote: > > Hi, > > The motivation of this short series is to fix a reported in [1]. A > couple of bugs were fixed along the way. > > Björn, these patches should remediate the situation you're experiencing. > > Patches based on master. > > [1] https://lore.kernel.org/qemu-riscv/87y0usiz22.fsf@all.your.base.are.belong.to.us/ > > Daniel Henrique Barboza (3): > target/riscv/tcg: restrict satp_mode changes in cpu_set_profile > target/riscv/tcg: decouple profile enablement from user prop > target/riscv: add profile->present flag Thanks! Applied to riscv-to-apply.next Alistair > > target/riscv/cpu.h | 15 ++++ > target/riscv/riscv-qmp-cmds.c | 2 +- > target/riscv/tcg/tcg-cpu.c | 138 +++++++++++++++++----------------- > 3 files changed, 86 insertions(+), 69 deletions(-) > > -- > 2.49.0 > > ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2025-05-29 4:34 UTC | newest] Thread overview: 15+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-05-20 17:23 [PATCH 0/3] target/riscv: profile handling fixes Daniel Henrique Barboza 2025-05-20 17:23 ` [PATCH 1/3] target/riscv/tcg: restrict satp_mode changes in cpu_set_profile Daniel Henrique Barboza 2025-05-21 13:47 ` Andrew Jones 2025-05-21 16:30 ` Björn Töpel 2025-05-29 3:03 ` Alistair Francis 2025-05-20 17:23 ` [PATCH 2/3] target/riscv/tcg: decouple profile enablement from user prop Daniel Henrique Barboza 2025-05-21 13:48 ` Andrew Jones 2025-05-21 16:34 ` Björn Töpel 2025-05-29 3:08 ` Alistair Francis 2025-05-20 17:23 ` [PATCH 3/3] target/riscv: add profile->present flag Daniel Henrique Barboza 2025-05-21 13:47 ` Andrew Jones 2025-05-21 16:33 ` Björn Töpel 2025-05-29 3:11 ` Alistair Francis 2025-05-21 16:27 ` [PATCH 0/3] target/riscv: profile handling fixes Björn Töpel 2025-05-29 4:32 ` Alistair Francis
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).