Re: [PATCH 3/3] target/riscv: add profile->present flag
On Wed, May 21, 2025 at 3:24 AM 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/[email protected]/ > > Reported-by: Björn Töpel > Fixes: 2af005d610 ("target/riscv/tcg: validate profiles during finalize") > Signed-off-by: Daniel Henrique Barboza > > Signed-off-by: Daniel Henrique Barboza Reviewed-by: Alistair Francis 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 > >
Re: [PATCH 3/3] target/riscv: add profile->present flag
Daniel Henrique Barboza 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/[email protected]/ > > Reported-by: Björn Töpel > Fixes: 2af005d610 ("target/riscv/tcg: validate profiles during finalize") > Signed-off-by: Daniel Henrique Barboza Reviewed-by: Björn Töpel
Re: [PATCH 3/3] target/riscv: add profile->present flag
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/[email protected]/ > > Reported-by: Björn Töpel > Fixes: 2af005d610 ("target/riscv/tcg: validate profiles during finalize") > Signed-off-by: Daniel Henrique Barboza > > Signed-off-by: Daniel Henrique Barboza 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
