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 <m...@sifive.com> 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 0000000..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 */ >> >> FWIW, other targets have moved this timer to RISCVCPU struct. > > > I'll look into this. It seems like it should be a simple change and easy > to include in the next spin. > > > +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 0x0000000300000000 >> > +#define MSTATUS64_SXL 0x0000000C00000000 >> >> 64-bit constants must use ULL. Otherwise builds from a 32-bit host will >> fail. >> There are lots more instances within this file. > > > Got it. Will fix these in the next spin. > > Thanks! >
