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

[Michael Clark]

On Wed, Feb 7, 2018 at 4:03 AM, Igor Mammedov <address@hidden> wrote:

> On Tue, 6 Feb 2018 11:09:56 +1300
> Michael Clark <address@hidden> wrote:
>
> > On Tue, Feb 6, 2018 at 4:04 AM, Igor Mammedov <address@hidden>
> wrote:
> >
> > > On Mon,  5 Feb 2018 19:22:28 +1300
> > > Michael Clark <address@hidden> wrote:
> > >
> > > > Add CPU state header, CPU definitions and initialization routines
> > > >
> > > > Signed-off-by: Michael Clark <address@hidden>
> > > > ---
> > > >  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.
>

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.

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.