Re: [Qemu-devel] [PATCH v4 03/22] RISC-V CPU Core Definition

[Igor Mammedov <imammedo@redhat.com>]

On Thu, 8 Feb 2018 15:19:13 +1300
Michael Clark <mjc@sifive.com> wrote:

> On Wed, Feb 7, 2018 at 4:03 AM, Igor Mammedov <imammedo@redhat.com> wrote:
> 
> > On Tue, 6 Feb 2018 11:09:56 +1300
> > Michael Clark <mjc@sifive.com> wrote:
> >  
> > > On Tue, Feb 6, 2018 at 4:04 AM, Igor Mammedov <imammedo@redhat.com>  
> > wrote:  
> > >  
> > > > On Mon,  5 Feb 2018 19:22:28 +1300
> > > > Michael Clark <mjc@sifive.com> wrote:
> > > >  
> > > > > Add CPU state header, CPU definitions and initialization routines
> > > > >
> > > > > Signed-off-by: Michael Clark <mjc@sifive.com>
> > > > > ---
> > > > >  target/riscv/cpu.c      | 385 ++++++++++++++++++++++++++++++  
> > > > ++++++++++++++  
> > > > >  target/riscv/cpu.h      | 256 +++++++++++++++++++++++++++++
> > > > >  target/riscv/cpu_bits.h | 417 ++++++++++++++++++++++++++++++  
> > > > ++++++++++++++++++  
> > > > >  3 files changed, 1058 insertions(+)
> > > > >  create mode 100644 target/riscv/cpu.c
> > > > >  create mode 100644 target/riscv/cpu.h
> > > > >  create mode 100644 target/riscv/cpu_bits.h
> > > > >
> > > > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > > > > new file mode 100644
> > > > > index 0000000..684b78b
> > > > > --- /dev/null
> > > > > +++ b/target/riscv/cpu.c  
> > > > [...]  
> > > > > +
> > > > > +static const RISCVCPUInfo riscv_cpus[] = {
> > > > > +#ifdef CONFIG_USER_ONLY
> > > > > +    { TYPE_RISCV_CPU_ANY,                riscv_any_cpu_init },
> > > > > +#else
> > > > > +    { TYPE_RISCV_CPU_IMAFDCSU_PRIV_1_09,  
> > riscv_imafdcsu_priv1_9_cpu_init  
> > > > },  
> > > > > +    { TYPE_RISCV_CPU_IMAFDCSU_PRIV_1_10,  
> > riscv_imafdcsu_priv1_10_cpu_init  
> > > > },  
> > > > > +    { TYPE_RISCV_CPU_IMACU_PRIV_1_10,  
> > riscv_imacu_priv1_10_cpu_init  
> > > > },  
> > > > > +    { TYPE_RISCV_CPU_IMAC_PRIV_1_10,  
> >  riscv_imac_priv1_10_cpu_init },  
> > > > > +#endif
> > > > > +    { NULL, NULL }
> > > > > +};
> > > > > +  
> > > > [...]  
> > > > > +static void cpu_register(const RISCVCPUInfo *info)
> > > > > +{
> > > > > +    TypeInfo type_info = {
> > > > > +        .name = info->name,
> > > > > +        .parent = TYPE_RISCV_CPU,
> > > > > +        .instance_size = sizeof(RISCVCPU),
> > > > > +        .instance_init = info->initfn,
> > > > > +    };
> > > > > +
> > > > > +    type_register(&type_info);
> > > > > +}
> > > > > +
> > > > > +static const TypeInfo riscv_cpu_type_info = {
> > > > > +    .name = TYPE_RISCV_CPU,
> > > > > +    .parent = TYPE_CPU,
> > > > > +    .instance_size = sizeof(RISCVCPU),
> > > > > +    .instance_init = riscv_cpu_init,
> > > > > +    .abstract = false,
> > > > > +    .class_size = sizeof(RISCVCPUClass),
> > > > > +    .class_init = riscv_cpu_class_init,
> > > > > +};  
> > > > [...]
> > > >  
> > > > > +static void riscv_cpu_register_types(void)
> > > > > +{
> > > > > +    const RISCVCPUInfo *info = riscv_cpus;
> > > > > +
> > > > > +    type_register_static(&riscv_cpu_type_info);
> > > > > +
> > > > > +    while (info->name) {
> > > > > +        cpu_register(info);
> > > > > +        info++;
> > > > > +    }
> > > > > +}
> > > > > +
> > > > > +type_init(riscv_cpu_register_types)  
> > > > For simplistic type definitions like that,
> > > > above parts should use DEFINE_TYPES(), see c6678108 for reference.
> > > >
> > > >  
> > > > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > > > > new file mode 100644
> > > > > index 0000000..8b816ae
> > > > > --- /dev/null
> > > > > +++ b/target/riscv/cpu.h  
> > > > [...]  
> > > > > +#define TYPE_RISCV_CPU                    "riscv"
> > > > > +#define TYPE_RISCV_CPU_ANY                "riscv-any"
> > > > > +#define TYPE_RISCV_CPU_IMAFDCSU_PRIV_1_09 "riscv-imafdcsu-priv1.9"
> > > > > +#define TYPE_RISCV_CPU_IMAFDCSU_PRIV_1_10 "riscv-imafdcsu-priv1.10"
> > > > > +#define TYPE_RISCV_CPU_IMACU_PRIV_1_10    "riscv-imacu-priv1.10"
> > > > > +#define TYPE_RISCV_CPU_IMAC_PRIV_1_10     "riscv-imac-priv1.10"
Also you can use RISCV_CPU_TYPE_NAME() from blow to form above names,
like:

#define TYPE_RISCV_CPU                    "riscv-cpu"
#define TYPE_RISCV_CPU_ANY                RISCV_CPU_TYPE_NAME("any")
...

then whatever naming format is required, you'd be able to
change it just in RISCV_CPU_TYPE_NAME() without touching
the rest.

> > > > > +
> > > > > +#define RISCV_CPU_TYPE_PREFIX TYPE_RISCV_CPU "-"
> > > > > +#define RISCV_CPU_TYPE_NAME(name) (RISCV_CPU_TYPE_PREFIX name)  
> > > > it still uses prefix notation versus commonly used suffix in form of
> > > >  "targetFOO-cpu"
> > > > this prefix approach would get in the way if we try to generalize
> > > > naming <-> type conversion later[*].
> > > > So it would better to be consistent with approach qemu uses for cpu  
> > types  
> > > > (I believe power had prefix based pnv types but it has been fixed
> > > > to common suffix based pattern later).
> > > >
> > > > * discussion on thread "[PATCH v5 0/6]  Add a valid_cpu_types property"
> > > >  
> > >
> > > I can reverse them if needed, just it seems a little odd to have riscv on
> > > the right-hand side of the extensions. I can do this in the v5 spin...  
> > pls, do so + -cpu suffix.
> >  
> > > It may make more sense once we have actual CPU models. Currently, we have
> > > sets of extensions and privilege ISA versions. I guess these will become
> > > implicit properties of a specific CPU model and the extensions will be
> > > visible in the initialization function.  
> > That's what we have in x86 land, code names + _suffix corresponding to
> > real cpus (mostly) with implicit feature set per each.
> > Features could be properties so user would be able to enumerate/query/set
> > them.
> >  
> 
> I just realized I neglected the change in the current patch-set to use the
> suffix scheme i.e. "<extension>-<version>-riscv-cpu" versus the
> "riscv-<extension>-<version>" scheme that we have presently.
Yep, just saw v5 still has old variant,
You also forgot about comment:
"
> +type_init(riscv_cpu_register_types)  
For simplistic type definitions like that,
above parts should use DEFINE_TYPES(), see c6678108 for reference.
"
 
> I would like to discuss with RISC-V folk as how we should name these
> "generic" CPUs and how we should handle extensions and versioning. This is
> certainly going to be an area that is going to be worked on in the future
> so whether we evolve this in-tree or out-of-tree is up for discussion. We
> also have to decide whether we will add CPU models for all typical
> combinations of extensions or if this is handled some other way. There will
> also be actual CPU modules now silicon is becoming more widely available.
> The version number we have currently is essentially the supervisor
> specification version. That won't be necessary once we have real cpu models.
> 
> There is also a future patch that will allow 'misa' extensions to be
> changed on the fly, which will require the extensions to be exposed in
> cpu_get_tb_cpu_state flags.
> 
> One thing is for certain, is that there is no direct comparison to x86 land
> as the RISC-V extension scheme is presently quite different.
See, following thread which might be of interest to you:
  "how to handle QOM 'container' objects whose contents depend on QOM properties?"