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

[Igor Mammedov <imammedo@redhat.com>]

On Tue, 6 Mar 2018 09:58:34 +0100
Igor Mammedov <imammedo@redhat.com> wrote:

> 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.

PS:

Considering upcomming soft-freeze,
It's fine to do this change on top of this series instead
of re-spinning whole series.