Re: [PATCH 07/20] target/riscv/cpu.c: add .instance_post_init()

[Daniel Henrique Barboza]

On 8/31/23 08:00, Andrew Jones wrote:
> On Fri, Aug 25, 2023 at 10:08:40AM -0300, Daniel Henrique Barboza wrote:
> > All generic CPUs call riscv_cpu_add_user_properties(). The 'max' CPU
> > calls riscv_init_max_cpu_extensions(). Both can be moved to a common
> > instance_post_init() callback, implemented in riscv_cpu_post_init(),
> > called by all CPUs. The call order then becomes:
> >
> > riscv_cpu_init() -> cpu_init() of each CPU -> .instance_post_init()
> >
> > A CPU class that wants to add user flags will let us know via the
> > 'user_extension_properties' property. Likewise, 'cfg.max_features' will
> > determine if any given CPU, regardless of being the 'max' CPU or not,
> > wants to enable the maximum amount of extensions.
> >
> > In the near future riscv_cpu_post_init() will call the init() function
> > of the current accelerator, providing a hook for KVM and TCG accel
> > classes to change the init() process of the CPU.
> >
> > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> > ---
> >   target/riscv/cpu.c | 20 +++++++++++++++-----
> >   1 file changed, 15 insertions(+), 5 deletions(-)
> >
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index c35d58c64b..f67b782675 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -430,8 +430,6 @@ static void riscv_max_cpu_init(Object *obj)
> >       mlx = MXL_RV32;
> >   #endif
> >       set_misa(env, mlx, 0);
> > -    riscv_cpu_add_user_properties(obj);
> > -    riscv_init_max_cpu_extensions(obj);
> >       env->priv_ver = PRIV_VERSION_LATEST;
> >   #ifndef CONFIG_USER_ONLY
> >       set_satp_mode_max_supported(RISCV_CPU(obj), mlx == MXL_RV32 ?
> > @@ -445,7 +443,6 @@ static void rv64_base_cpu_init(Object *obj)
> >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> >       /* We set this in the realise function */
> >       set_misa(env, MXL_RV64, 0);
> > -    riscv_cpu_add_user_properties(obj);
> >       /* Set latest version of privileged specification */
> >       env->priv_ver = PRIV_VERSION_LATEST;
> >   #ifndef CONFIG_USER_ONLY
> > @@ -569,7 +566,6 @@ static void rv128_base_cpu_init(Object *obj)
> >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> >       /* We set this in the realise function */
> >       set_misa(env, MXL_RV128, 0);
> > -    riscv_cpu_add_user_properties(obj);
> >       /* Set latest version of privileged specification */
> >       env->priv_ver = PRIV_VERSION_LATEST;
> >   #ifndef CONFIG_USER_ONLY
> > @@ -582,7 +578,6 @@ static void rv32_base_cpu_init(Object *obj)
> >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> >       /* We set this in the realise function */
> >       set_misa(env, MXL_RV32, 0);
> > -    riscv_cpu_add_user_properties(obj);
> >       /* Set latest version of privileged specification */
> >       env->priv_ver = PRIV_VERSION_LATEST;
> >   #ifndef CONFIG_USER_ONLY
> > @@ -1212,6 +1207,20 @@ static void riscv_cpu_set_irq(void *opaque, int irq, int 
> > level)
> >   }
> >   #endif /* CONFIG_USER_ONLY */
> >   
> > +static void riscv_cpu_post_init(Object *obj)
> > +{
> > +    RISCVCPU *cpu = RISCV_CPU(obj);
> > +    RISCVCPUClass *rcc = RISCV_CPU_GET_CLASS(cpu);
> > +
> > +    if (rcc->user_extension_properties) {
>
> It's not yet clear to me why we need 'user_extension_properties'. Can't we
> just do the 'object_dynamic_cast(obj, TYPE_RISCV_DYNAMIC_CPU) != NULL'
> check here?

I'll answer here but this also applies for patches 19 and 20.

The idea in my head was to create a flexible way of defining new CPU types in
the future that doesn't necessarily fits the 3 big molds we have: generic CPUs
(I'm considering 'max' CPU as a generic CPU on steroids), vendor CPUs and the
KVM only 'host' CPU. For example, it would be possible to create a vendor-style
CPU that has all extensions enabled and runs KVM.

This idea is probably better in my head than in reality, and there's a very high
chance that I'm adding extra stuff in the CPU class and we won't add any new
'funky' CPU type in the future to justify it.

I'll drop patches 5 and 6 with 'user_extension_properties' and 'max_features'
flag and do a regular CPU type check in post_init().'tcg_supported' in patch 19
is indeed a bit silly today since every CPU but 'host' will enable it, so we can
do a 'cpu is host' kind of check and live without it. We can still throw generic
errors in all these checks regardless of how we're doing the validation.

Patch 20 has another underlying discussion that I'd rather have there. Thanks,


Daniel





> > +        riscv_cpu_add_user_properties(obj);
> > +    }
> > +
> > +    if (cpu->cfg.max_features) {
>
> It's also not yet clear why we need max_features. I can't think of any
> other models that want max_features besides 'max'. Checking the cpu type
> here should be sufficient, no?
>
> > +        riscv_init_max_cpu_extensions(obj);
> > +    }
> > +}
> > +
> >   static void riscv_cpu_init(Object *obj)
> >   {
> >       RISCVCPU *cpu = RISCV_CPU(obj);
> > @@ -2019,6 +2028,7 @@ static const TypeInfo riscv_cpu_type_infos[] = {
> >           .instance_size = sizeof(RISCVCPU),
> >           .instance_align = __alignof__(RISCVCPU),
> >           .instance_init = riscv_cpu_init,
> > +        .instance_post_init = riscv_cpu_post_init,
> >           .abstract = true,
> >           .class_size = sizeof(RISCVCPUClass),
> >           .class_init = riscv_cpu_class_init,
> > --
> > 2.41.0
>
> Thanks,
> drew