qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v1 03/21] RISC-V CPU Core Definition


From: Michael Clark
Subject: Re: [Qemu-devel] [PATCH v1 03/21] RISC-V CPU Core Definition
Date: Mon, 8 Jan 2018 19:55:06 +1300

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 <address@hidden> wrote:

>
>
> On Wed, Jan 3, 2018 at 6:21 PM, Richard Henderson <
> address@hidden> 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!
>


reply via email to

[Prev in Thread] Current Thread [Next in Thread]