Re: [Qemu-devel] [PATCH v1 03/21] RISC-V CPU Core Definition
Hi Antony,
As of v8 of the RISC-V QEMU target patch series, you can now define the
reset vector in your CPU initializer:
https://github.com/riscv/riscv-qemu/blob/qemu-upstream-v8/target/riscv/cpu.c#L110-L168
Michael.
On Fri, Jan 5, 2018 at 6:53 AM, Antony Pavlov
wrote:
> On Thu, 4 Jan 2018 20:33:57 +1300
> Michael Clark wrote:
>
> > On Thu, Jan 4, 2018 at 7:47 PM, Antony Pavlov
> > wrote:
> >
> > > On Wed, 3 Jan 2018 13:44:07 +1300
> > > Michael Clark wrote:
> > >
> > > > Add CPU state header, CPU definitions and initialization routines
> > > >
> > > > Signed-off-by: Michael Clark
> > > > ---
> > > > target/riscv/cpu.c | 338 ++
> +
> > > > target/riscv/cpu.h | 363 ++
> > >
> > > > target/riscv/cpu_bits.h | 411 ++
> > > ++
> > > > 3 files changed, 1112 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
> > >
> > > ...
> > >
> > > > +static void riscv_cpu_reset(CPUState *cs)
> > > > +{
> > > > +RISCVCPU *cpu = RISCV_CPU(cs);
> > > > +RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(cpu);
> > > > +CPURISCVState *env = &cpu->env;
> > > > +
> > > > +mcc->parent_reset(cs);
> > > > +#ifndef CONFIG_USER_ONLY
> > > > +tlb_flush(cs);
> > > > +env->priv = PRV_M;
> > > > +env->mtvec = DEFAULT_MTVEC;
> > > > +#endif
> > > > +env->pc = DEFAULT_RSTVEC;
> > >
> > > The RISC-V Privileged Architecture Manual v1.10 states that
> > >
> > > The pc is set to an implementation-defined reset vector.
> > >
> > > But hard-coded DEFAULT_RSTVEC leaves no chance for changing reset
> vector.
> > >
> > > Can we add a mechanism for changing reset vector?
> > >
> >
> > That can be added very easily at some point when necessary.
> >
> > All 5 RISC-V machines in the QEMU port currently have their emulated Mask
> > ROMs at 0x1000 so its not necessary until we add a machine that needs a
> > different value. I certainly wouldn't reject a patch that adds that
> > functionality if we had a machine with a different reset vector, although
> > given we have 5 machines using the same vector, it may remain a sensible
> > default. I would think twice about adding a property that no machines
> sets,
> > or duplicate code and have all machines set their reset vector even when
> > they are all the same? Shall we add the functionality when we need it?
>
> Actually it is me who needs this functionality.
>
> At the moment my board code needs this dirty hack:
>
> https://github.com/miet-riscv-workgroup/riscv-qemu/commit/
> bfc8221d89b9bb828f3742f17eb89d8513a75aae#diff-
> 429448b1b26e0bc4256cc290758c0ab5
>
> >
> > I'd categorise this as a feature request. #define DEFAULT_RSTVEC
> 0x1000
> > is the "implementation-defined reset vector"
> >
> > Folk on the RISC-V mailing list are actually seeking guidance on the
> blanks
> > in the RISC-V specification so it may be that a de-facto standard emerges
> > for some of these "implementation defined" blanks, in which case it may
> > become part of a platform spec (vs the ISA spec).
> >
> > E.g. there is the "reset-hivecs" property in the ARM emulation code
> > > so SoC-specific code can change reset vector.
>
> --
> Best regards,
> Antony Pavlov
>
Re: [Qemu-devel] [PATCH v1 03/21] RISC-V CPU Core Definition
FYI,
I've been working on the patch review comments in order of the patches and
am focusing on cleanup up the cpu and helpers.
OK [PATCH v1 0001/0021] RISC-V Maintainers
OK [PATCH v1 0002/0021] RISC-V ELF Machine Definition
INPROGRESS [PATCH v1 0003/0021] RISC-V CPU Core Definition
DONE [PATCH v1 0004/0021] RISC-V Disassembler
INPROGRESS [PATCH v1 0005/0021] RISC-V CPU Helpers
Changelog
- Remove redundant NULL terminators from disassembler register name arrays
- Change disassembler register name arrays to const
- Refine disassembler internal function names
- Update dates in disassembler copyright message
- Remove #ifdef CONFIG_USER_ONLY version of cpu_has_work
- Use ULL suffix on 64-bit constants so that builds on 32-bit hosts will
work
- Move riscv_cpu_mmu_index from cpu.h to helper.c
- Move riscv_cpu_hw_interrupts_pending from cpu.h to helper.c
- Remove redundant TARGET_HAS_ICE from cpu.h
- Use qemu_irq instead of void* for irq definition in cpu.h
- Remove duplicate typedef from struct CPURISCVState
- Remove redundant g_strdup from cpu_register
- Remove redundant tlb_flush from riscv_cpu_reset
- Remove redundant mode calculation from get_physical_address
- Remove redundant debug mode printf and dcsr comment
- Remove redundant clearing of MSB for bare physical addresses
- Use g_assert_not_reached for invalid mode in get_physical_address
- Use g_assert_not_reached for unreachable permission checks in
get_physical_address
- Use g_assert_not_reached for unreachable access type in
raise_mmu_exception
- Return misalign exception instead of aborting for misalined fetches
- Move exception defines from cpu.h to cpu_bits.h
- Remove redundant breakpoint control definitions from cpu_bits.h
- Implement riscv_cpu_unassigned_access exception handling
- Log and raise exceptions for unimplemented CSRs
Here is my development tree:
- https://github.com/michaeljclark/riscv-qemu/tree/qemu-devel
I've got individual commit entries for each change to make the deltas
easier to review and will archive this branch before squashing for the v2
spin:
- https://github.com/michaeljclark/riscv-qemu/commits/qemu-devel
Hopefully, I'll have a new spin relatively soon... I'm making good progress
on getting target/riscv clean enough for re-submission...
Michael.
On Thu, Jan 4, 2018 at 11:30 AM, Michael Clark wrote:
>
>
> On Wed, Jan 3, 2018 at 6:21 PM, Richard Henderson <
> richard.hender...@linaro.org> wrote:
>
>> On 01/02/2018 04:44 PM, Michael Clark wrote:
>> > +#ifdef CONFIG_USER_ONLY
>> > +static bool riscv_cpu_has_work(CPUState *cs)
>> > +{
>> > +return 0;
>> > +}
>> > +#else
>> > +static bool riscv_cpu_has_work(CPUState *cs)
>> > +{
>> > +return cs->interrupt_request & CPU_INTERRUPT_HARD;
>> > +}
>> > +#endif
>>
>> There's no need to conditionalize this.
>
>
> Got it. Will be in the next spin.
>
>
>> > +static void riscv_cpu_reset(CPUState *cs)
>> > +{
>> > +RISCVCPU *cpu = RISCV_CPU(cs);
>> > +RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(cpu);
>> > +CPURISCVState *env = &cpu->env;
>> > +
>> > +mcc->parent_reset(cs);
>> > +#ifndef CONFIG_USER_ONLY
>> > +tlb_flush(cs);
>>
>> Flush is now generic. Remove it from here.
>
>
> OK.
>
> > +static void riscv_cpu_realize(DeviceState *dev, Error **errp)
>> > +{
>> > +CPUState *cs = CPU(dev);
>> > +RISCVCPU *cpu = RISCV_CPU(dev);
>> > +RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(dev);
>> > +CPURISCVState *env = &cpu->env;
>> > +Error *local_err = NULL;
>> > +
>> > +cpu_exec_realizefn(cs, &local_err);
>> > +if (local_err != NULL) {
>> > +error_propagate(errp, local_err);
>> > +return;
>> > +}
>> > +
>> > +if (env->misa & RVM) {
>> > +set_feature(env, RISCV_FEATURE_RVM);
>> > +}
>>
>> What's the point of replicating this information?
>>
>
> This is inherited code. I noticed this too. In this version they are
> actually in sync with each other, which they weren't several weeks ago :-D
>
> It may well be that the features flags pre-date the addition of the 'misa'
> register in the privilege spec.
>
> This will take a bit of re-work as a reasonable amount of code uses the
> FEATURE flags vs misa.
>
> Are you happy for this to be a pending work item? I don't like it either
> and eventually want to fix, and already did some work to sync it with
> 'misa', but it's not a critical issue.
>
> > +static void cpu_register(const RISCVCPUInfo *info)
>> > +{
>> > +TypeInfo type_info = {
>> > +.name = g_strdup(info->name),
>> > +.parent = TYPE_RISCV_CPU,
>> > +.instance_size = sizeof(RISCVCPU),
>> > +.instance_init = info->initfn,
>> > +};
>> > +
>> > +type_register(&type_info);
>> > +g_free((void *)type_info.name);
>> > +}
>>
>> I think type_register does its own strdup; you don't need to do your own.
>
>
> Got it.
>
>
>> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
>> > new file mode 100644
>> > index 000..0480127
>> > --- /dev/null
>> > +++ b/target/r
Re: [Qemu-devel] [PATCH v1 03/21] RISC-V CPU Core Definition
On Fri, 5 Jan 2018 at 6:39 AM, Antony Pavlov
wrote:
> On Thu, 4 Jan 2018 20:33:57 +1300
> Michael Clark wrote:
>
> > On Thu, Jan 4, 2018 at 7:47 PM, Antony Pavlov
> > wrote:
> >
> > > On Wed, 3 Jan 2018 13:44:07 +1300
> > > Michael Clark wrote:
> > >
> > > > Add CPU state header, CPU definitions and initialization routines
> > > >
> > > > Signed-off-by: Michael Clark
> > > > ---
> > > > target/riscv/cpu.c | 338
> +++
> > > > target/riscv/cpu.h | 363 ++
> > >
> > > > target/riscv/cpu_bits.h | 411 ++
> > > ++
> > > > 3 files changed, 1112 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
> > >
> > > ...
> > >
> > > > +static void riscv_cpu_reset(CPUState *cs)
> > > > +{
> > > > +RISCVCPU *cpu = RISCV_CPU(cs);
> > > > +RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(cpu);
> > > > +CPURISCVState *env = &cpu->env;
> > > > +
> > > > +mcc->parent_reset(cs);
> > > > +#ifndef CONFIG_USER_ONLY
> > > > +tlb_flush(cs);
> > > > +env->priv = PRV_M;
> > > > +env->mtvec = DEFAULT_MTVEC;
> > > > +#endif
> > > > +env->pc = DEFAULT_RSTVEC;
> > >
> > > The RISC-V Privileged Architecture Manual v1.10 states that
> > >
> > > The pc is set to an implementation-defined reset vector.
> > >
> > > But hard-coded DEFAULT_RSTVEC leaves no chance for changing reset
> vector.
> > >
> > > Can we add a mechanism for changing reset vector?
> > >
> >
> > That can be added very easily at some point when necessary.
> >
> > All 5 RISC-V machines in the QEMU port currently have their emulated Mask
> > ROMs at 0x1000 so its not necessary until we add a machine that needs a
> > different value. I certainly wouldn't reject a patch that adds that
> > functionality if we had a machine with a different reset vector, although
> > given we have 5 machines using the same vector, it may remain a sensible
> > default. I would think twice about adding a property that no machines
> sets,
> > or duplicate code and have all machines set their reset vector even when
> > they are all the same? Shall we add the functionality when we need it?
>
> Actually it is me who needs this functionality.
>
> At the moment my board code needs this dirty hack:
>
>
> https://github.com/miet-riscv-workgroup/riscv-qemu/commit/bfc8221d89b9bb828f3742f17eb89d8513a75aae#diff-429448b1b26e0bc4256cc290758c0ab5
Okay, we can add a property. It’s also possible to register a cpu_reset
callback and set the pc there, within the machine.
>
> > I'd categorise this as a feature request. #define DEFAULT_RSTVEC
> 0x1000
> > is the "implementation-defined reset vector"
> >
> > Folk on the RISC-V mailing list are actually seeking guidance on the
> blanks
> > in the RISC-V specification so it may be that a de-facto standard emerges
> > for some of these "implementation defined" blanks, in which case it may
> > become part of a platform spec (vs the ISA spec).
> >
> > E.g. there is the "reset-hivecs" property in the ARM emulation code
> > > so SoC-specific code can change reset vector.
>
> --
> Best regards,
> Antony Pavlov
>
Re: [Qemu-devel] [PATCH v1 03/21] RISC-V CPU Core Definition
On Thu, 4 Jan 2018 20:33:57 +1300
Michael Clark wrote:
> On Thu, Jan 4, 2018 at 7:47 PM, Antony Pavlov
> wrote:
>
> > On Wed, 3 Jan 2018 13:44:07 +1300
> > Michael Clark wrote:
> >
> > > Add CPU state header, CPU definitions and initialization routines
> > >
> > > Signed-off-by: Michael Clark
> > > ---
> > > target/riscv/cpu.c | 338 +++
> > > target/riscv/cpu.h | 363 ++
> >
> > > target/riscv/cpu_bits.h | 411 ++
> > ++
> > > 3 files changed, 1112 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
> >
> > ...
> >
> > > +static void riscv_cpu_reset(CPUState *cs)
> > > +{
> > > +RISCVCPU *cpu = RISCV_CPU(cs);
> > > +RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(cpu);
> > > +CPURISCVState *env = &cpu->env;
> > > +
> > > +mcc->parent_reset(cs);
> > > +#ifndef CONFIG_USER_ONLY
> > > +tlb_flush(cs);
> > > +env->priv = PRV_M;
> > > +env->mtvec = DEFAULT_MTVEC;
> > > +#endif
> > > +env->pc = DEFAULT_RSTVEC;
> >
> > The RISC-V Privileged Architecture Manual v1.10 states that
> >
> > The pc is set to an implementation-defined reset vector.
> >
> > But hard-coded DEFAULT_RSTVEC leaves no chance for changing reset vector.
> >
> > Can we add a mechanism for changing reset vector?
> >
>
> That can be added very easily at some point when necessary.
>
> All 5 RISC-V machines in the QEMU port currently have their emulated Mask
> ROMs at 0x1000 so its not necessary until we add a machine that needs a
> different value. I certainly wouldn't reject a patch that adds that
> functionality if we had a machine with a different reset vector, although
> given we have 5 machines using the same vector, it may remain a sensible
> default. I would think twice about adding a property that no machines sets,
> or duplicate code and have all machines set their reset vector even when
> they are all the same? Shall we add the functionality when we need it?
Actually it is me who needs this functionality.
At the moment my board code needs this dirty hack:
https://github.com/miet-riscv-workgroup/riscv-qemu/commit/bfc8221d89b9bb828f3742f17eb89d8513a75aae#diff-429448b1b26e0bc4256cc290758c0ab5
>
> I'd categorise this as a feature request. #define DEFAULT_RSTVEC 0x1000
> is the "implementation-defined reset vector"
>
> Folk on the RISC-V mailing list are actually seeking guidance on the blanks
> in the RISC-V specification so it may be that a de-facto standard emerges
> for some of these "implementation defined" blanks, in which case it may
> become part of a platform spec (vs the ISA spec).
>
> E.g. there is the "reset-hivecs" property in the ARM emulation code
> > so SoC-specific code can change reset vector.
--
Best regards,
Antony Pavlov
Re: [Qemu-devel] [PATCH v1 03/21] RISC-V CPU Core Definition
On Thu, Jan 4, 2018 at 7:47 PM, Antony Pavlov
wrote:
> On Wed, 3 Jan 2018 13:44:07 +1300
> Michael Clark wrote:
>
> > Add CPU state header, CPU definitions and initialization routines
> >
> > Signed-off-by: Michael Clark
> > ---
> > target/riscv/cpu.c | 338 +++
> > target/riscv/cpu.h | 363 ++
>
> > target/riscv/cpu_bits.h | 411 ++
> ++
> > 3 files changed, 1112 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
>
> ...
>
> > +static void riscv_cpu_reset(CPUState *cs)
> > +{
> > +RISCVCPU *cpu = RISCV_CPU(cs);
> > +RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(cpu);
> > +CPURISCVState *env = &cpu->env;
> > +
> > +mcc->parent_reset(cs);
> > +#ifndef CONFIG_USER_ONLY
> > +tlb_flush(cs);
> > +env->priv = PRV_M;
> > +env->mtvec = DEFAULT_MTVEC;
> > +#endif
> > +env->pc = DEFAULT_RSTVEC;
>
> The RISC-V Privileged Architecture Manual v1.10 states that
>
> The pc is set to an implementation-defined reset vector.
>
> But hard-coded DEFAULT_RSTVEC leaves no chance for changing reset vector.
>
> Can we add a mechanism for changing reset vector?
>
That can be added very easily at some point when necessary.
All 5 RISC-V machines in the QEMU port currently have their emulated Mask
ROMs at 0x1000 so its not necessary until we add a machine that needs a
different value. I certainly wouldn't reject a patch that adds that
functionality if we had a machine with a different reset vector, although
given we have 5 machines using the same vector, it may remain a sensible
default. I would think twice about adding a property that no machines sets,
or duplicate code and have all machines set their reset vector even when
they are all the same? Shall we add the functionality when we need it?
I'd categorise this as a feature request. #define DEFAULT_RSTVEC 0x1000
is the "implementation-defined reset vector"
Folk on the RISC-V mailing list are actually seeking guidance on the blanks
in the RISC-V specification so it may be that a de-facto standard emerges
for some of these "implementation defined" blanks, in which case it may
become part of a platform spec (vs the ISA spec).
E.g. there is the "reset-hivecs" property in the ARM emulation code
> so SoC-specific code can change reset vector.
>
>
> > +cs->exception_index = EXCP_NONE;
> > +set_default_nan_mode(1, &env->fp_status);
> > +}
> > +
> > +static void riscv_cpu_disas_set_info(CPUState *s, disassemble_info
> *info)
> > +{
> > +#if defined(TARGET_RISCV32)
> > +info->print_insn = print_insn_riscv32;
> > +#elif defined(TARGET_RISCV64)
> > +info->print_insn = print_insn_riscv64;
> > +#endif
> > +}
> > +
> > +static void riscv_cpu_realize(DeviceState *dev, Error **errp)
> > +{
> > +CPUState *cs = CPU(dev);
> > +RISCVCPU *cpu = RISCV_CPU(dev);
> > +RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(dev);
> > +CPURISCVState *env = &cpu->env;
> > +Error *local_err = NULL;
> > +
> > +cpu_exec_realizefn(cs, &local_err);
> > +if (local_err != NULL) {
> > +error_propagate(errp, local_err);
> > +return;
> > +}
> > +
> > +if (env->misa & RVM) {
> > +set_feature(env, RISCV_FEATURE_RVM);
> > +}
> > +if (env->misa & RVA) {
> > +set_feature(env, RISCV_FEATURE_RVA);
> > +}
> > +if (env->misa & RVF) {
> > +set_feature(env, RISCV_FEATURE_RVF);
> > +}
> > +if (env->misa & RVD) {
> > +set_feature(env, RISCV_FEATURE_RVD);
> > +}
> > +if (env->misa & RVC) {
> > +set_feature(env, RISCV_FEATURE_RVC);
> > +}
> > +
> > +qemu_init_vcpu(cs);
> > +cpu_reset(cs);
> > +
> > +mcc->parent_realize(dev, errp);
> > +}
> > +
> > +static void riscv_cpu_init(Object *obj)
> > +{
> > +CPUState *cs = CPU(obj);
> > +RISCVCPU *cpu = RISCV_CPU(obj);
> > +
> > +cs->env_ptr = &cpu->env;
> > +}
> > +
> > +static const VMStateDescription vmstate_riscv_cpu = {
> > +.name = "cpu",
> > +.unmigratable = 1,
> > +};
> > +
> > +static void riscv_cpu_class_init(ObjectClass *c, void *data)
> > +{
> > +RISCVCPUClass *mcc = RISCV_CPU_CLASS(c);
> > +CPUClass *cc = CPU_CLASS(c);
> > +DeviceClass *dc = DEVICE_CLASS(c);
> > +
> > +mcc->parent_realize = dc->realize;
> > +dc->realize = riscv_cpu_realize;
> > +
> > +mcc->parent_reset = cc->reset;
> > +cc->reset = riscv_cpu_reset;
> > +
> > +cc->class_by_name = riscv_cpu_class_by_name;
> > +cc->has_work = riscv_cpu_has_work;
> > +cc->do_interrupt = riscv_cpu_do_interrupt;
> > +cc->cpu_exec_interrupt = riscv_cpu_exec_interrupt;
> > +cc->dump_state = riscv_cpu_dump_state;
> > +cc->set_pc = riscv_cpu_set_pc;
> > +cc->synchronize_from_tb = riscv_cpu_synchroni
Re: [Qemu-devel] [PATCH v1 03/21] RISC-V CPU Core Definition
On Wed, 3 Jan 2018 13:44:07 +1300
Michael Clark wrote:
> Add CPU state header, CPU definitions and initialization routines
>
> Signed-off-by: Michael Clark
> ---
> target/riscv/cpu.c | 338 +++
> target/riscv/cpu.h | 363 ++
> target/riscv/cpu_bits.h | 411
>
> 3 files changed, 1112 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
...
> +static void riscv_cpu_reset(CPUState *cs)
> +{
> +RISCVCPU *cpu = RISCV_CPU(cs);
> +RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(cpu);
> +CPURISCVState *env = &cpu->env;
> +
> +mcc->parent_reset(cs);
> +#ifndef CONFIG_USER_ONLY
> +tlb_flush(cs);
> +env->priv = PRV_M;
> +env->mtvec = DEFAULT_MTVEC;
> +#endif
> +env->pc = DEFAULT_RSTVEC;
The RISC-V Privileged Architecture Manual v1.10 states that
The pc is set to an implementation-defined reset vector.
But hard-coded DEFAULT_RSTVEC leaves no chance for changing reset vector.
Can we add a mechanism for changing reset vector?
E.g. there is the "reset-hivecs" property in the ARM emulation code
so SoC-specific code can change reset vector.
> +cs->exception_index = EXCP_NONE;
> +set_default_nan_mode(1, &env->fp_status);
> +}
> +
> +static void riscv_cpu_disas_set_info(CPUState *s, disassemble_info *info)
> +{
> +#if defined(TARGET_RISCV32)
> +info->print_insn = print_insn_riscv32;
> +#elif defined(TARGET_RISCV64)
> +info->print_insn = print_insn_riscv64;
> +#endif
> +}
> +
> +static void riscv_cpu_realize(DeviceState *dev, Error **errp)
> +{
> +CPUState *cs = CPU(dev);
> +RISCVCPU *cpu = RISCV_CPU(dev);
> +RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(dev);
> +CPURISCVState *env = &cpu->env;
> +Error *local_err = NULL;
> +
> +cpu_exec_realizefn(cs, &local_err);
> +if (local_err != NULL) {
> +error_propagate(errp, local_err);
> +return;
> +}
> +
> +if (env->misa & RVM) {
> +set_feature(env, RISCV_FEATURE_RVM);
> +}
> +if (env->misa & RVA) {
> +set_feature(env, RISCV_FEATURE_RVA);
> +}
> +if (env->misa & RVF) {
> +set_feature(env, RISCV_FEATURE_RVF);
> +}
> +if (env->misa & RVD) {
> +set_feature(env, RISCV_FEATURE_RVD);
> +}
> +if (env->misa & RVC) {
> +set_feature(env, RISCV_FEATURE_RVC);
> +}
> +
> +qemu_init_vcpu(cs);
> +cpu_reset(cs);
> +
> +mcc->parent_realize(dev, errp);
> +}
> +
> +static void riscv_cpu_init(Object *obj)
> +{
> +CPUState *cs = CPU(obj);
> +RISCVCPU *cpu = RISCV_CPU(obj);
> +
> +cs->env_ptr = &cpu->env;
> +}
> +
> +static const VMStateDescription vmstate_riscv_cpu = {
> +.name = "cpu",
> +.unmigratable = 1,
> +};
> +
> +static void riscv_cpu_class_init(ObjectClass *c, void *data)
> +{
> +RISCVCPUClass *mcc = RISCV_CPU_CLASS(c);
> +CPUClass *cc = CPU_CLASS(c);
> +DeviceClass *dc = DEVICE_CLASS(c);
> +
> +mcc->parent_realize = dc->realize;
> +dc->realize = riscv_cpu_realize;
> +
> +mcc->parent_reset = cc->reset;
> +cc->reset = riscv_cpu_reset;
> +
> +cc->class_by_name = riscv_cpu_class_by_name;
> +cc->has_work = riscv_cpu_has_work;
> +cc->do_interrupt = riscv_cpu_do_interrupt;
> +cc->cpu_exec_interrupt = riscv_cpu_exec_interrupt;
> +cc->dump_state = riscv_cpu_dump_state;
> +cc->set_pc = riscv_cpu_set_pc;
> +cc->synchronize_from_tb = riscv_cpu_synchronize_from_tb;
> +cc->gdb_read_register = riscv_cpu_gdb_read_register;
> +cc->gdb_write_register = riscv_cpu_gdb_write_register;
> +cc->gdb_num_core_regs = 65;
> +cc->gdb_stop_before_watchpoint = true;
> +cc->disas_set_info = riscv_cpu_disas_set_info;
> +#ifdef CONFIG_USER_ONLY
> +cc->handle_mmu_fault = riscv_cpu_handle_mmu_fault;
> +#else
> +cc->do_unassigned_access = riscv_cpu_unassigned_access;
> +cc->do_unaligned_access = riscv_cpu_do_unaligned_access;
> +cc->get_phys_page_debug = riscv_cpu_get_phys_page_debug;
> +#endif
> +#ifdef CONFIG_TCG
> +cc->tcg_initialize = riscv_translate_init;
> +#endif
> +/* For now, mark unmigratable: */
> +cc->vmsd = &vmstate_riscv_cpu;
> +}
> +
> +static void cpu_register(const RISCVCPUInfo *info)
> +{
> +TypeInfo type_info = {
> +.name = g_strdup(info->name),
> +.parent = TYPE_RISCV_CPU,
> +.instance_size = sizeof(RISCVCPU),
> +.instance_init = info->initfn,
> +};
> +
> +type_register(&type_info);
> +g_free((void *)type_info.name);
> +}
> +
> +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(RISCVCPUCl
Re: [Qemu-devel] [PATCH v1 03/21] RISC-V CPU Core Definition
On Wed, Jan 3, 2018 at 6:21 PM, Richard Henderson <
richard.hender...@linaro.org> wrote:
> On 01/02/2018 04:44 PM, Michael Clark wrote:
> > +#ifdef CONFIG_USER_ONLY
> > +static bool riscv_cpu_has_work(CPUState *cs)
> > +{
> > +return 0;
> > +}
> > +#else
> > +static bool riscv_cpu_has_work(CPUState *cs)
> > +{
> > +return cs->interrupt_request & CPU_INTERRUPT_HARD;
> > +}
> > +#endif
>
> There's no need to conditionalize this.
Got it. Will be in the next spin.
> > +static void riscv_cpu_reset(CPUState *cs)
> > +{
> > +RISCVCPU *cpu = RISCV_CPU(cs);
> > +RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(cpu);
> > +CPURISCVState *env = &cpu->env;
> > +
> > +mcc->parent_reset(cs);
> > +#ifndef CONFIG_USER_ONLY
> > +tlb_flush(cs);
>
> Flush is now generic. Remove it from here.
OK.
> +static void riscv_cpu_realize(DeviceState *dev, Error **errp)
> > +{
> > +CPUState *cs = CPU(dev);
> > +RISCVCPU *cpu = RISCV_CPU(dev);
> > +RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(dev);
> > +CPURISCVState *env = &cpu->env;
> > +Error *local_err = NULL;
> > +
> > +cpu_exec_realizefn(cs, &local_err);
> > +if (local_err != NULL) {
> > +error_propagate(errp, local_err);
> > +return;
> > +}
> > +
> > +if (env->misa & RVM) {
> > +set_feature(env, RISCV_FEATURE_RVM);
> > +}
>
> What's the point of replicating this information?
>
This is inherited code. I noticed this too. In this version they are
actually in sync with each other, which they weren't several weeks ago :-D
It may well be that the features flags pre-date the addition of the 'misa'
register in the privilege spec.
This will take a bit of re-work as a reasonable amount of code uses the
FEATURE flags vs misa.
Are you happy for this to be a pending work item? I don't like it either
and eventually want to fix, and already did some work to sync it with
'misa', but it's not a critical issue.
> +static void cpu_register(const RISCVCPUInfo *info)
> > +{
> > +TypeInfo type_info = {
> > +.name = g_strdup(info->name),
> > +.parent = TYPE_RISCV_CPU,
> > +.instance_size = sizeof(RISCVCPU),
> > +.instance_init = info->initfn,
> > +};
> > +
> > +type_register(&type_info);
> > +g_free((void *)type_info.name);
> > +}
>
> I think type_register does its own strdup; you don't need to do your own.
Got it.
> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > new file mode 100644
> > index 000..0480127
> > --- /dev/null
> > +++ b/target/riscv/cpu.h
> > @@ -0,0 +1,363 @@
> > +#ifndef RISCV_CPU_H
>
> Header comment and license?
>
> > +#define TARGET_HAS_ICE 1
>
> What's this for?
It's redundant. Inherited code. Looks like it came from nios2. Will remove.
> > +#define RV(x) (1L << (x - 'A'))
>
> L is useless since the type of long is variable. Either U or ULL.
>
> > +typedef struct CPURISCVState CPURISCVState;
> > +
> > +#include "pmp.h"
> > +
> > +typedef struct CPURISCVState {
>
> Duplicate typedef.
>
Got it.
> > +target_ulong gpr[32];
> > +uint64_t fpr[32]; /* assume both F and D extensions */
> > +target_ulong pc;
> > +target_ulong load_res;
> > +
> > +target_ulong frm;
> > +target_ulong fstatus;
> > +target_ulong fflags;
> > +
> > +target_ulong badaddr;
> > +
> > +uint32_t mucounteren;
> > +
> > +target_ulong user_ver;
> > +target_ulong priv_ver;
> > +target_ulong misa_mask;
> > +target_ulong misa;
> > +
> > +#ifdef CONFIG_USER_ONLY
> > +uint32_t amoinsn;
> > +target_long amoaddr;
> > +target_long amotest;
> > +#else
> > +target_ulong priv;
> > +
> > +target_ulong mhartid;
> > +target_ulong mstatus;
> > +target_ulong mip;
> > +target_ulong mie;
> > +target_ulong mideleg;
> > +
> > +target_ulong sptbr; /* until: priv-1.9.1 */
> > +target_ulong satp; /* since: priv-1.10.0 */
> > +target_ulong sbadaddr;
> > +target_ulong mbadaddr;
> > +target_ulong medeleg;
> > +
> > +target_ulong stvec;
> > +target_ulong sepc;
> > +target_ulong scause;
> > +
> > +target_ulong mtvec;
> > +target_ulong mepc;
> > +target_ulong mcause;
> > +target_ulong mtval; /* since: priv-1.10.0 */
> > +
> > +uint32_t mscounteren;
> > +target_ulong scounteren; /* since: priv-1.10.0 */
> > +target_ulong mcounteren; /* since: priv-1.10.0 */
> > +
> > +target_ulong sscratch;
> > +target_ulong mscratch;
> > +
> > +/* temporary htif regs */
> > +uint64_t mfromhost;
> > +uint64_t mtohost;
> > +uint64_t timecmp;
> > +
> > +/* physical memory protection */
> > +pmp_table_t pmp_state;
> > +#endif
> > +
> > +float_status fp_status;
> > +
> > +/* Internal CPU feature flags. */
> > +uint64_t features;
> > +
> > +/* QEMU */
> > +CPU_COMMON
> > +
> > +/* Fields from here on are preserved across CPU reset. */
> > +void *irq[8];
> > +QEMUTimer *timer; /* Internal timer
Re: [Qemu-devel] [PATCH v1 03/21] RISC-V CPU Core Definition
On 01/02/2018 04:44 PM, Michael Clark wrote:
> +#ifdef CONFIG_USER_ONLY
> +static bool riscv_cpu_has_work(CPUState *cs)
> +{
> +return 0;
> +}
> +#else
> +static bool riscv_cpu_has_work(CPUState *cs)
> +{
> +return cs->interrupt_request & CPU_INTERRUPT_HARD;
> +}
> +#endif
There's no need to conditionalize this.
> +static void riscv_cpu_reset(CPUState *cs)
> +{
> +RISCVCPU *cpu = RISCV_CPU(cs);
> +RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(cpu);
> +CPURISCVState *env = &cpu->env;
> +
> +mcc->parent_reset(cs);
> +#ifndef CONFIG_USER_ONLY
> +tlb_flush(cs);
Flush is now generic. Remove it from here.
> +static void riscv_cpu_realize(DeviceState *dev, Error **errp)
> +{
> +CPUState *cs = CPU(dev);
> +RISCVCPU *cpu = RISCV_CPU(dev);
> +RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(dev);
> +CPURISCVState *env = &cpu->env;
> +Error *local_err = NULL;
> +
> +cpu_exec_realizefn(cs, &local_err);
> +if (local_err != NULL) {
> +error_propagate(errp, local_err);
> +return;
> +}
> +
> +if (env->misa & RVM) {
> +set_feature(env, RISCV_FEATURE_RVM);
> +}
What's the point of replicating this information?
> +static void cpu_register(const RISCVCPUInfo *info)
> +{
> +TypeInfo type_info = {
> +.name = g_strdup(info->name),
> +.parent = TYPE_RISCV_CPU,
> +.instance_size = sizeof(RISCVCPU),
> +.instance_init = info->initfn,
> +};
> +
> +type_register(&type_info);
> +g_free((void *)type_info.name);
> +}
I think type_register does its own strdup; you don't need to do your own.
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> new file mode 100644
> index 000..0480127
> --- /dev/null
> +++ b/target/riscv/cpu.h
> @@ -0,0 +1,363 @@
> +#ifndef RISCV_CPU_H
Header comment and license?
> +#define TARGET_HAS_ICE 1
What's this for?
> +#define RV(x) (1L << (x - 'A'))
L is useless since the type of long is variable. Either U or ULL.
> +typedef struct CPURISCVState CPURISCVState;
> +
> +#include "pmp.h"
> +
> +typedef struct CPURISCVState {
Duplicate typedef.
> +target_ulong gpr[32];
> +uint64_t fpr[32]; /* assume both F and D extensions */
> +target_ulong pc;
> +target_ulong load_res;
> +
> +target_ulong frm;
> +target_ulong fstatus;
> +target_ulong fflags;
> +
> +target_ulong badaddr;
> +
> +uint32_t mucounteren;
> +
> +target_ulong user_ver;
> +target_ulong priv_ver;
> +target_ulong misa_mask;
> +target_ulong misa;
> +
> +#ifdef CONFIG_USER_ONLY
> +uint32_t amoinsn;
> +target_long amoaddr;
> +target_long amotest;
> +#else
> +target_ulong priv;
> +
> +target_ulong mhartid;
> +target_ulong mstatus;
> +target_ulong mip;
> +target_ulong mie;
> +target_ulong mideleg;
> +
> +target_ulong sptbr; /* until: priv-1.9.1 */
> +target_ulong satp; /* since: priv-1.10.0 */
> +target_ulong sbadaddr;
> +target_ulong mbadaddr;
> +target_ulong medeleg;
> +
> +target_ulong stvec;
> +target_ulong sepc;
> +target_ulong scause;
> +
> +target_ulong mtvec;
> +target_ulong mepc;
> +target_ulong mcause;
> +target_ulong mtval; /* since: priv-1.10.0 */
> +
> +uint32_t mscounteren;
> +target_ulong scounteren; /* since: priv-1.10.0 */
> +target_ulong mcounteren; /* since: priv-1.10.0 */
> +
> +target_ulong sscratch;
> +target_ulong mscratch;
> +
> +/* temporary htif regs */
> +uint64_t mfromhost;
> +uint64_t mtohost;
> +uint64_t timecmp;
> +
> +/* physical memory protection */
> +pmp_table_t pmp_state;
> +#endif
> +
> +float_status fp_status;
> +
> +/* Internal CPU feature flags. */
> +uint64_t features;
> +
> +/* QEMU */
> +CPU_COMMON
> +
> +/* Fields from here on are preserved across CPU reset. */
> +void *irq[8];
> +QEMUTimer *timer; /* Internal timer */
FWIW, other targets have moved this timer to RISCVCPU struct.
> +static inline void cpu_get_tb_cpu_state(CPURISCVState *env, target_ulong *pc,
> +target_ulong *cs_base, uint32_t
> *flags)
> +{
> +*pc = env->pc;
> +*cs_base = 0;
> +*flags = 0; /* necessary to avoid compiler warning */
Remove the comment -- the assignment is necessary full stop.
> +#define MSTATUS64_UXL 0x0003
> +#define MSTATUS64_SXL 0x000C
64-bit constants must use ULL. Otherwise builds from a 32-bit host will fail.
There are lots more instances within this file.
r~
