Re: [Qemu-devel] [PATCH v8 03/23] RISC-V CPU Core Definition

[Igor Mammedov <imammedo@redhat.com>]

On Tue, 6 Mar 2018 11:24:02 +1300
Michael Clark <mjc@sifive.com> wrote:

> On Mon, Mar 5, 2018 at 10:44 PM, Igor Mammedov <imammedo@redhat.com> wrote:
> 
> > On Sat,  3 Mar 2018 02:51:31 +1300
> > Michael Clark <mjc@sifive.com> wrote:
> >  
> > > Add CPU state header, CPU definitions and initialization routines
> > >
> > > Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> > > Signed-off-by: Sagar Karandikar <sagark@eecs.berkeley.edu>
> > > Signed-off-by: Michael Clark <mjc@sifive.com>
> > > ---
> > >  target/riscv/cpu.c      | 432 ++++++++++++++++++++++++++++++  
> > ++++++++++++++++++  
> > >  target/riscv/cpu.h      | 296 +++++++++++++++++++++++++++++++++
> > >  target/riscv/cpu_bits.h | 411 ++++++++++++++++++++++++++++++  
> > +++++++++++++++  
> > >  3 files changed, 1139 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..4851890
> > > --- /dev/null
> > > +++ b/target/riscv/cpu.c  
> > [...]
> >  
> > > +
> > > +typedef struct RISCVCPUInfo {
> > > +    const int bit_widths;
> > > +    const char *name;
> > > +    void (*initfn)(Object *obj);
> > > +} RISCVCPUInfo;
> > > +  
> > [...]
> >  
> > > +static const RISCVCPUInfo riscv_cpus[] = {
> > > +    { 96, TYPE_RISCV_CPU_ANY,              riscv_any_cpu_init },
> > > +    { 32, TYPE_RISCV_CPU_RV32GCSU_V1_09_1,  
> > rv32gcsu_priv1_09_1_cpu_init },  
> > > +    { 32, TYPE_RISCV_CPU_RV32GCSU_V1_10_0,  
> > rv32gcsu_priv1_10_0_cpu_init },  
> > > +    { 32, TYPE_RISCV_CPU_RV32IMACU_NOMMU,  rv32imacu_nommu_cpu_init },
> > > +    { 32, TYPE_RISCV_CPU_SIFIVE_E31,       rv32imacu_nommu_cpu_init },
> > > +    { 32, TYPE_RISCV_CPU_SIFIVE_U34,       rv32gcsu_priv1_10_0_cpu_init  
> > },  
> > > +    { 64, TYPE_RISCV_CPU_RV64GCSU_V1_09_1,  
> > rv64gcsu_priv1_09_1_cpu_init },  
> > > +    { 64, TYPE_RISCV_CPU_RV64GCSU_V1_10_0,  
> > rv64gcsu_priv1_10_0_cpu_init },  
> > > +    { 64, TYPE_RISCV_CPU_RV64IMACU_NOMMU,  rv64imacu_nommu_cpu_init },
> > > +    { 64, TYPE_RISCV_CPU_SIFIVE_E51,       rv64imacu_nommu_cpu_init },
> > > +    { 64, TYPE_RISCV_CPU_SIFIVE_U54,       rv64gcsu_priv1_10_0_cpu_init  
> > },  
> > > +    { 0, 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);
> > > +}  
> > [...]
> >  
> > > +void riscv_cpu_list(FILE *f, fprintf_function cpu_fprintf)
> > > +{
> > > +    const RISCVCPUInfo *info = riscv_cpus;
> > > +
> > > +    while (info->name) {
> > > +        if (info->bit_widths & TARGET_LONG_BITS) {
> > > +            (*cpu_fprintf)(f, "%s\n", info->name);
> > > +        }
> > > +        info++;
> > > +    }
> > > +}
> > > +
> > > +static void riscv_cpu_register_types(void)
> > > +{
> > > +    const RISCVCPUInfo *info = riscv_cpus;
> > > +
> > > +    type_register_static(&riscv_cpu_type_info);
> > > +
> > > +    while (info->name) {
> > > +        if (info->bit_widths & TARGET_LONG_BITS) {
> > > +            cpu_register(info);
> > > +        }
> > > +        info++;
> > > +    }
> > > +}
> > > +
> > > +type_init(riscv_cpu_register_types)  
> > This still isn't fixed as requested
> >  http://lists.gnu.org/archive/html/qemu-devel/2018-02/msg06412.html  
> 
> 
> It's possibly because I explicitly requested a clarification. Pointing at a
> commit and being asked to infer what the desired change is, is not what I
> would call reasonable feedback. The code has already been reviewed.
Well, it's been pointed since v4 (it's not like change has been asked for
at the last moment) and no one asked for clarification.


> We have
> just expanded on it in a manner consistent with how the ARM port handled
> cpu initialization.
> I'm happy to comply if you give me detailed instructions on what is wrong,
> why, and how to fix it versus infer your problem from this commit to
> another architecture.
> 
> Apologies if i'm a bit slow, but I really don't understand the change you
> intend us to make.
There is nothing wrong and it's totally ok to use existing code to
start with writing new patches. The only thing is that it's moving
codebase and new patches shouldn't interfere with ongoing work done
by others. Hence sometimes you see comments requesting to use
a particular approach to do something that could be done in
various ways.

In this specific case used reference code (ARM) still uses old way
register CPU types that is phased out in favor of DEFINE_TYPES.

There is nothing that warrants usage of custom 'struct RISCVCPUInfo'
and riscv_cpu_register_types().
You should use pretty trivial approach used in 974e58d2, namely:

 1   rewrite riscv_cpu_list() to use object_class_get_list()
     instead of 'struct RISCVCPUInfo', example sh4_cpu_list()
 2.1 drop 'struct RISCVCPUInfo' and use TypeInfo array instead
     superh_cpu_type_infos[] and DEFINE_SUPERH_CPU_TYPE() could serve as example
 2.2 replace riscv_cpu_register_types/type_init with DEFINE_TYPES()

That way your code will be consistent with direction this part
moves towards and when others work on generalizing CPU related parts
they won't have to deal with one more instance of old style cpu
types init and listing.