Re: [PATCH RFC] target: riscv: Fix satp mode initialization based on profile

[Daniel Henrique Barboza <dbarboza@ventanamicro.com>]

On 5/20/25 7:53 AM, Paolo Bonzini wrote:
> On 5/19/25 14:07, Björn Töpel wrote:
>> When realizing the cpus, the first cpu calls riscv_cpu_add_profiles()
>> all profiles are disabled, whereas for the other cpu calls to
>> riscv_cpu_add_profiles() have some profiles enabled. Having some
>> profiles enabled, will issue a call to cpu_set_profile() that will
>> enforce the satp code that Alex removes in this patch.
> Ah so the problem is that *parent* profiles are not enabled until
> riscv_cpu_add_profiles().

The problem in Björn's case is that we shouldn't take the profile code path
for any CPUs since he's not enabling any profiles. There's a bug in how we're
detecting a profile presence for QMP for CPU0 and how this detection is changing
the cpu_init of CPU1 and above. I'll send a fix for it today.

> 
> With my patches to introduce RISCVCPUDef, it's a pretty easy fix:
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 629ac37501e..04b929af41c 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1083,6 +1083,19 @@ static bool riscv_cpu_is_dynamic(Object *cpu_obj)
>       return object_dynamic_cast(cpu_obj, TYPE_RISCV_DYNAMIC_CPU) != NULL;
>   }
> 
> +static void riscv_cpu_enable_profile(RISCVCPU *cpu,
> +                                     RISCVCPUProfile *profile)
> +{
> +    profile->enabled = true;
> +
> +    if (profile->u_parent) {
> +        riscv_cpu_enable_profile(cpu, profile->u_parent);
> +    }
> +    if (profile->s_parent) {
> +        riscv_cpu_enable_profile(cpu, profile->s_parent);
> +    }
> +}
> +
>   static void riscv_cpu_init(Object *obj)
>   {
>       RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(obj);
> @@ -1121,7 +1134,7 @@ static void riscv_cpu_init(Object *obj)
>       cpu->cfg.max_satp_mode = -1;
> 
>       if (mcc->def->profile) {
> -        mcc->def->profile->enabled = true;
> +        riscv_cpu_enable_profile(cpu, mcc->def->profile);
>       }
> 
>       env->misa_ext_mask = env->misa_ext = mcc->def->misa_ext;
> 
> Since they're all reviewed and Alistair has flushed his queue, I'll
> send them in my next pull request.
> 
> On top of them, probably profiles should also be converted to use
> RISCVCPUCfg and riscv_cpu_enable_profile() can then enable all the
> flags with riscv_cpu_cfg_merge().
> 

If we can do the same thing with less abstractions than what we're using
ATM I'll all for it.

> In general a lot (if not all) of the profile code should be moved out
> of tcg-cpu.c and into riscv_cpu_class_base_init().  I didn't do that
> because I didn't want to balloon an already-large series, but it's a
> pretty obvious extension of the RISCVCPUDef concept to include all
> profile features.

Note that KVM RISC-V does not have the same profile support as TCG - I'm not
sure if KVM RISC-V has RVA22 support, let alone RVA23. If we move the profile
logic to cpu.c we need to be careful with TCG assumptions affecting KVM CPUs.


Thanks,


Daniel

> 
> Paolo
>