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

[Richard Henderson]

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

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


r~