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 <[email protected]>
> Fixes: 2af005d610 ("target/riscv/tcg: validate profiles during finalize")
> Signed-off-by: Daniel Henrique Barboza <[email protected]>
>
> Signed-off-by: Daniel Henrique Barboza <[email protected]>
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 <[email protected]>
