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

[Igor Mammedov <imammedo@redhat.com>]

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"
> > > +
> > > +#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.