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

2018-03-06 Thread Michael Clark 
On Tue, Mar 6, 2018 at 9:58 PM, Igor Mammedov  wrote:

> On Tue, 6 Mar 2018 11:24:02 +1300
> Michael Clark  wrote:
>
> > On Mon, Mar 5, 2018 at 10:44 PM, Igor Mammedov 
> wrote:
> >
> > > On Sat,  3 Mar 2018 02:51:31 +1300
> > > Michael Clark  wrote:
> > >
> > > > Add CPU state header, CPU definitions and initialization routines
> > > >
> > > > Reviewed-by: Richard Henderson 
> > > > Signed-off-by: Sagar Karandikar 
> > > > Signed-off-by: Michael Clark 
> > > > ---
> > > >  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 000..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(_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(_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

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

2018-03-06 Thread Igor Mammedov 
On Tue, 6 Mar 2018 09:58:34 +0100
Igor Mammedov  wrote:

> On Tue, 6 Mar 2018 11:24:02 +1300
> Michael Clark  wrote:
> 
> > On Mon, Mar 5, 2018 at 10:44 PM, Igor Mammedov  wrote:
> >   
> > > On Sat,  3 Mar 2018 02:51:31 +1300
> > > Michael Clark  wrote:
> > >
> > > > Add CPU state header, CPU definitions and initialization routines
> > > >
> > > > Reviewed-by: Richard Henderson 
> > > > Signed-off-by: Sagar Karandikar 
> > > > Signed-off-by: Michael Clark 
> > > > ---
> > > >  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 000..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(_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(_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

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

2018-03-06 Thread Igor Mammedov 
On Tue, 6 Mar 2018 11:24:02 +1300
Michael Clark  wrote:

> On Mon, Mar 5, 2018 at 10:44 PM, Igor Mammedov  wrote:
> 
> > On Sat,  3 Mar 2018 02:51:31 +1300
> > Michael Clark  wrote:
> >  
> > > Add CPU state header, CPU definitions and initialization routines
> > >
> > > Reviewed-by: Richard Henderson 
> > > Signed-off-by: Sagar Karandikar 
> > > Signed-off-by: Michael Clark 
> > > ---
> > >  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 000..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(_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(_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:

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

2018-03-05 Thread Michael Clark 
On Mon, Mar 5, 2018 at 10:44 PM, Igor Mammedov  wrote:

> On Sat,  3 Mar 2018 02:51:31 +1300
> Michael Clark  wrote:
>
> > Add CPU state header, CPU definitions and initialization routines
> >
> > Reviewed-by: Richard Henderson 
> > Signed-off-by: Sagar Karandikar 
> > Signed-off-by: Michael Clark 
> > ---
> >  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 000..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(_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(_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. 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.

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

2018-03-05 Thread Igor Mammedov 
On Sat,  3 Mar 2018 02:51:31 +1300
Michael Clark  wrote:

> Add CPU state header, CPU definitions and initialization routines
> 
> Reviewed-by: Richard Henderson 
> Signed-off-by: Sagar Karandikar 
> Signed-off-by: Michael Clark 
> ---
>  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 000..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(_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(_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

[...]

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

2018-03-02 Thread Michael Clark 
Paraphrase this as, we should be closer to reproducing the behaviour of the
SiFive E31, E51, U34 and U54 cores when running RISC-V and SiFive
verification tests. i.e. now if one attempts to configure the MMU on E
cores one will get an illegal instruction trap.

We still have an E21 core to add but I need some more details on
behavioural differences between the SiFive E21 and E31. We're happy to keep
this in the riscv branch for after the 2.12 release. We don't have any more
features to add pre QEMU 2.12, and if we do we will maintain them on a
branch in the https://github.com/riscv/riscv-qemu.git repository.

So now I think we're mostly at the point where we will be in a rebase loop,
assuming we make it in to qemu before 2.12.

If we make a v9 it will contain extremely minor changes. e.g. replacing
hard-coded values with constants and fixing typos.

- https://github.com/michaeljclark/riscv-qemu/commits/qemu-devel

I believe we have all of the necessary licensing changes and sign-offs done.

On Sat, Mar 3, 2018 at 3:23 PM, Michael Clark  wrote:

> We were able to remove several ifdefs and figured out a problem with
> masking out cores for qemu-system-riscv32 and qemu-system-riscv64.
>
> This version of the core patch seems cleaner to me and we have fixed a few
> spec compliance issues with regard to alignment of mtvec/stvec when the C
> extension is enabled or disabled.
>
> We haven't addressed runtime changes to 'misa' as it is legal for 'misa'
> to be read-only. A later "feature" patch will add runtime support for misa
> changes, likely just invalidating the translation cache due to the limited
> number of bits in mmu_index. I can say for sure we not going to do anything
> this risky so close to soft-freeze. The last minute changes were focused at
> specification compliance for MMU vs NOMMU and respecting the 'S' and 'U'
> misa bits with respect to allowable privilege modes. SiFive's E Series
> cores do not support S mode but they do support U mode.
>
> We also had an internal discussion about S-mode vs mmu, and it was decided
> that it is a legal combination to have a nommu core that implements S mode.
> In this use case, M mode could configure PMP (Physical Memory Protection),
> and it would potentially possible to port FDPIC nommu linux to work on a
> core with S mode and nommu. The other S mode registers besides 'satp'
> (Supervisor Address Translation Pointer) are available in S-mode with nommu.
>
> The v8 patch series just tightens up compliance with the specification,
> and makes use of the mmu register (satp), selecting S-mode or calling SRET
> cause illegal instruction traps.
>
> Writes to mstatus.mpp of un unsupported modes (i.e. 'S' or 'U' misa bits
> not present) are silently dropped, as that is the behaviour I am told
> should be implemented. We may have to raise some issues against the RISC-V
> Privileged ISA specification to make sure that this behaviour is specified,
> as it currently does not specify explicitly the behaviour for a core that
> supports S-mode with no-mmu.
>
> Regards,
> Michael.
>
> On Sat, Mar 3, 2018 at 2:51 AM, Michael Clark  wrote:
>
>> Add CPU state header, CPU definitions and initialization routines
>>
>> Reviewed-by: Richard Henderson 
>> Signed-off-by: Sagar Karandikar 
>> Signed-off-by: Michael Clark 
>> ---
>>  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 000..4851890
>> --- /dev/null
>> +++ b/target/riscv/cpu.c
>> @@ -0,0 +1,432 @@
>> +/*
>> + * QEMU RISC-V CPU
>> + *
>> + * Copyright (c) 2016-2017 Sagar Karandikar, sag...@eecs.berkeley.edu
>> + * Copyright (c) 2017-2018 SiFive, Inc.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> it
>> + * under the terms and conditions of the GNU General Public License,
>> + * version 2 or later, as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope it will be useful, but WITHOUT
>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
>> for
>> + * more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> along with
>> + * this program.  If not, see .
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qemu/log.h"
>> +#include "cpu.h"
>> +#include "exec/exec-all.h"
>> +#include "qapi/error.h"
>> +#include

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

2018-03-02 Thread Michael Clark 
We were able to remove several ifdefs and figured out a problem with
masking out cores for qemu-system-riscv32 and qemu-system-riscv64.

This version of the core patch seems cleaner to me and we have fixed a few
spec compliance issues with regard to alignment of mtvec/stvec when the C
extension is enabled or disabled.

We haven't addressed runtime changes to 'misa' as it is legal for 'misa' to
be read-only. A later "feature" patch will add runtime support for misa
changes, likely just invalidating the translation cache due to the limited
number of bits in mmu_index. I can say for sure we not going to do anything
this risky so close to soft-freeze. The last minute changes were focused at
specification compliance for MMU vs NOMMU and respecting the 'S' and 'U'
misa bits with respect to allowable privilege modes. SiFive's E Series
cores do not support S mode but they do support U mode.

We also had an internal discussion about S-mode vs mmu, and it was decided
that it is a legal combination to have a nommu core that implements S mode.
In this use case, M mode could configure PMP (Physical Memory Protection),
and it would potentially possible to port FDPIC nommu linux to work on a
core with S mode and nommu. The other S mode registers besides 'satp'
(Supervisor Address Translation Pointer) are available in S-mode with nommu.

The v8 patch series just tightens up compliance with the specification, and
makes use of the mmu register (satp), selecting S-mode or calling SRET
cause illegal instruction traps.

Writes to mstatus.mpp of un unsupported modes (i.e. 'S' or 'U' misa bits
not present) are silently dropped, as that is the behaviour I am told
should be implemented. We may have to raise some issues against the RISC-V
Privileged ISA specification to make sure that this behaviour is specified,
as it currently does not specify explicitly the behaviour for a core that
supports S-mode with no-mmu.

Regards,
Michael.

On Sat, Mar 3, 2018 at 2:51 AM, Michael Clark  wrote:

> Add CPU state header, CPU definitions and initialization routines
>
> Reviewed-by: Richard Henderson 
> Signed-off-by: Sagar Karandikar 
> Signed-off-by: Michael Clark 
> ---
>  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 000..4851890
> --- /dev/null
> +++ b/target/riscv/cpu.c
> @@ -0,0 +1,432 @@
> +/*
> + * QEMU RISC-V CPU
> + *
> + * Copyright (c) 2016-2017 Sagar Karandikar, sag...@eecs.berkeley.edu
> + * Copyright (c) 2017-2018 SiFive, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2 or later, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
> for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License
> along with
> + * this program.  If not, see .
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/log.h"
> +#include "cpu.h"
> +#include "exec/exec-all.h"
> +#include "qapi/error.h"
> +#include "migration/vmstate.h"
> +
> +/* RISC-V CPU definitions */
> +
> +static const char riscv_exts[26] = "IMAFDQECLBJTPVNSUHKORWXYZG";
> +
> +const char * const riscv_int_regnames[] = {
> +  "zero", "ra  ", "sp  ", "gp  ", "tp  ", "t0  ", "t1  ", "t2  ",
> +  "s0  ", "s1  ", "a0  ", "a1  ", "a2  ", "a3  ", "a4  ", "a5  ",
> +  "a6  ", "a7  ", "s2  ", "s3  ", "s4  ", "s5  ", "s6  ", "s7  ",
> +  "s8  ", "s9  ", "s10 ", "s11 ", "t3  ", "t4  ", "t5  ", "t6  "
> +};
> +
> +const char * const riscv_fpr_regnames[] = {
> +  "ft0 ", "ft1 ", "ft2 ", "ft3 ", "ft4 ", "ft5 ", "ft6 ",  "ft7 ",
> +  "fs0 ", "fs1 ", "fa0 ", "fa1 ", "fa2 ", "fa3 ", "fa4 ",  "fa5 ",
> +  "fa6 ", "fa7 ", "fs2 ", "fs3 ", "fs4 ", "fs5 ", "fs6 ",  "fs7 ",
> +  "fs8 ", "fs9 ", "fs10", "fs11", "ft8 ", "ft9 ", "ft10",  "ft11"
> +};
> +
> +const char * const riscv_excp_names[] = {
> +"misaligned_fetch",
> +"fault_fetch",
> +"illegal_instruction",
> +"breakpoint",
> +"misaligned_load",
> +"fault_load",
> +"misaligned_store",
> +"fault_store",
> +"user_ecall",
> +"supervisor_ecall",
> +"hypervisor_ecall",
> +"machine_ecall",
> +"exec_page_fault",
> +"load_page_fault",
> +"reserved",
> +

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

2018-03-02 Thread Michael Clark 
Add CPU state header, CPU definitions and initialization routines

Reviewed-by: Richard Henderson 
Signed-off-by: Sagar Karandikar 
Signed-off-by: Michael Clark 
---
 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 000..4851890
--- /dev/null
+++ b/target/riscv/cpu.c
@@ -0,0 +1,432 @@
+/*
+ * QEMU RISC-V CPU
+ *
+ * Copyright (c) 2016-2017 Sagar Karandikar, sag...@eecs.berkeley.edu
+ * Copyright (c) 2017-2018 SiFive, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2 or later, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see .
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "cpu.h"
+#include "exec/exec-all.h"
+#include "qapi/error.h"
+#include "migration/vmstate.h"
+
+/* RISC-V CPU definitions */
+
+static const char riscv_exts[26] = "IMAFDQECLBJTPVNSUHKORWXYZG";
+
+const char * const riscv_int_regnames[] = {
+  "zero", "ra  ", "sp  ", "gp  ", "tp  ", "t0  ", "t1  ", "t2  ",
+  "s0  ", "s1  ", "a0  ", "a1  ", "a2  ", "a3  ", "a4  ", "a5  ",
+  "a6  ", "a7  ", "s2  ", "s3  ", "s4  ", "s5  ", "s6  ", "s7  ",
+  "s8  ", "s9  ", "s10 ", "s11 ", "t3  ", "t4  ", "t5  ", "t6  "
+};
+
+const char * const riscv_fpr_regnames[] = {
+  "ft0 ", "ft1 ", "ft2 ", "ft3 ", "ft4 ", "ft5 ", "ft6 ",  "ft7 ",
+  "fs0 ", "fs1 ", "fa0 ", "fa1 ", "fa2 ", "fa3 ", "fa4 ",  "fa5 ",
+  "fa6 ", "fa7 ", "fs2 ", "fs3 ", "fs4 ", "fs5 ", "fs6 ",  "fs7 ",
+  "fs8 ", "fs9 ", "fs10", "fs11", "ft8 ", "ft9 ", "ft10",  "ft11"
+};
+
+const char * const riscv_excp_names[] = {
+"misaligned_fetch",
+"fault_fetch",
+"illegal_instruction",
+"breakpoint",
+"misaligned_load",
+"fault_load",
+"misaligned_store",
+"fault_store",
+"user_ecall",
+"supervisor_ecall",
+"hypervisor_ecall",
+"machine_ecall",
+"exec_page_fault",
+"load_page_fault",
+"reserved",
+"store_page_fault"
+};
+
+const char * const riscv_intr_names[] = {
+"u_software",
+"s_software",
+"h_software",
+"m_software",
+"u_timer",
+"s_timer",
+"h_timer",
+"m_timer",
+"u_external",
+"s_external",
+"h_external",
+"m_external",
+"coprocessor",
+"host"
+};
+
+typedef struct RISCVCPUInfo {
+const int bit_widths;
+const char *name;
+void (*initfn)(Object *obj);
+} RISCVCPUInfo;
+
+static void set_misa(CPURISCVState *env, target_ulong misa)
+{
+env->misa = misa;
+}
+
+static void set_versions(CPURISCVState *env, int user_ver, int priv_ver)
+{
+env->user_ver = user_ver;
+env->priv_ver = priv_ver;
+}
+
+static void set_feature(CPURISCVState *env, int feature)
+{
+env->features |= (1ULL << feature);
+}
+
+static void set_resetvec(CPURISCVState *env, int resetvec)
+{
+#ifndef CONFIG_USER_ONLY
+env->resetvec = resetvec;
+#endif
+}
+
+static void riscv_any_cpu_init(Object *obj)
+{
+CPURISCVState *env = _CPU(obj)->env;
+set_misa(env, RVXLEN | RVI | RVM | RVA | RVF | RVD | RVC | RVU);
+set_versions(env, USER_VERSION_2_02_0, PRIV_VERSION_1_10_0);
+set_resetvec(env, DEFAULT_RSTVEC);
+}
+
+static void rv32gcsu_priv1_09_1_cpu_init(Object *obj)
+{
+CPURISCVState *env = _CPU(obj)->env;
+set_misa(env, RV32 | RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
+set_versions(env, USER_VERSION_2_02_0, PRIV_VERSION_1_09_1);
+set_resetvec(env, DEFAULT_RSTVEC);
+set_feature(env, RISCV_FEATURE_MMU);
+}
+
+static void rv32gcsu_priv1_10_0_cpu_init(Object *obj)
+{
+CPURISCVState *env = _CPU(obj)->env;
+set_misa(env, RV32 | RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
+set_versions(env, USER_VERSION_2_02_0, PRIV_VERSION_1_10_0);
+set_resetvec(env, DEFAULT_RSTVEC);
+set_feature(env, RISCV_FEATURE_MMU);
+}
+
+static void rv32imacu_nommu_cpu_init(Object *obj)
+{
+CPURISCVState *env = _CPU(obj)->env;
+set_misa(env, RV32 | RVI | RVM | RVA | RVC | RVU);
+set_versions(env, USER_VERSION_2_02_0, PRIV_VERSION_1_10_0);
+set_resetvec(env, DEFAULT_RSTVEC);
+}
+
+static void rv64gcsu_priv1_09_1_cpu_init(Object *obj)
+{
+