Re: [PATCH 01/15] arc: Add initial core cpu files

[Richard Henderson]

On 11/11/20 10:17 AM, cupertinomiranda@gmail.com wrote:
> +enum arc_cpu_family {
> +  ARC_OPCODE_NONE    = 0,
> +  ARC_OPCODE_DEFAULT = 1 << 0,
> +  ARC_OPCODE_ARC600  = 1 << 1,
> +  ARC_OPCODE_ARC700  = 1 << 2,
> +  ARC_OPCODE_ARCv2EM = 1 << 3,
> +  ARC_OPCODE_ARCv2HS = 1 << 4
> +};

Indentation is off.

> +/*-*-indent-tabs-mode:nil;tab-width:4;indent-line-function:'insert-tab'-*-*/
> +/* vim: set ts=4 sw=4 et: */

This ought to be covered by the top-level .editorconfig.

> +static void arc_cpu_set_pc(CPUState *cs, vaddr value)
> +{
> +  ARCCPU *cpu = ARC_CPU(cs);
> +
> +  CPU_PCL(&cpu->env) = value & 0xfffffffc;
> +  cpu->env.pc = value;
> +}

Indentation is off all through cpu.c.

> +#ifndef CPU_ARC_H
> +#define CPU_ARC_H
> +
> +#include "cpu-qom.h"
> +#include "exec/cpu-defs.h"
> +#include "fpu/softfloat.h"

You probably only need softfloat-types.h in cpu.h.

> +#define MMU_IDX       0

What's this?

> +#define PHYS_BASE_RAM 0x00000000
> +#define VIRT_BASE_RAM 0x00000000

This seems like board stuff, not cpu stuff.

> +enum gdb_regs {
> +    GDB_REG_0 = 0,
> +    GDB_REG_1,
> +    GDB_REG_2,
> +    GDB_REG_3,
> +    GDB_REG_4,
> +    GDB_REG_5,
> +    GDB_REG_6,
> +    GDB_REG_7,
> +    GDB_REG_8,
> +    GDB_REG_9,
> +    GDB_REG_10,
> +    GDB_REG_11,
> +    GDB_REG_12,
> +    GDB_REG_13,
> +    GDB_REG_14,
> +    GDB_REG_15,
> +    GDB_REG_16,
> +    GDB_REG_17,
> +    GDB_REG_18,
> +    GDB_REG_19,
> +    GDB_REG_20,
> +    GDB_REG_21,
> +    GDB_REG_22,
> +    GDB_REG_23,
> +    GDB_REG_24,
> +    GDB_REG_25,
> +    GDB_REG_26,         /* GP                         */
> +    GDB_REG_27,         /* FP                         */
> +    GDB_REG_28,         /* SP                         */
> +    GDB_REG_29,         /* ILINK                      */
> +    GDB_REG_30,         /* R30                        */
> +    GDB_REG_31,         /* BLINK                      */
> +    GDB_REG_58,         /* little_endian? ACCL : ACCH */
> +    GDB_REG_59,         /* little_endian? ACCH : ACCL */
> +    GDB_REG_60,         /* LP                         */
> +    GDB_REG_63,         /* Immediate                  */
> +    GDB_REG_LAST
> +};

Why is this in cpu.h and not gdbstub.c?

> +#define CPU_GP(env)     ((env)->r[26])
> +#define CPU_FP(env)     ((env)->r[27])
> +#define CPU_SP(env)     ((env)->r[28])
> +#define CPU_ILINK(env)  ((env)->r[29])
> +#define CPU_ILINK1(env) ((env)->r[29])
> +#define CPU_ILINK2(env) ((env)->r[30])
> +#define CPU_BLINK(env)  ((env)->r[31])
> +#define CPU_LP(env)     ((env)->r[60])
> +#define CPU_IMM(env)    ((env)->r[62])
> +#define CPU_PCL(env)    ((env)->r[63])

Does it make more sense to define these in terms of numbers than lvalue macros?

> +typedef struct status_register {
> +    uint32_t Hf;     /* halt                    */
> +    uint32_t Ef;     /* irq priority treshold. */
> +    uint32_t AEf;
> +    uint32_t DEf;
> +    uint32_t Uf;
> +    uint32_t Vf;     /*  overflow                */
> +    uint32_t Cf;     /*  carry                   */
> +    uint32_t Nf;     /*  negative                */
> +    uint32_t Zf;     /*  zero                    */
> +    uint32_t Lf;
> +    uint32_t DZf;
> +    uint32_t SCf;
> +    uint32_t ESf;
> +    uint32_t RBf;
> +    uint32_t ADf;
> +    uint32_t USf;
> +    uint32_t IEf;
> +
> +    /* Reserved bits */
> +
> +    /* Next instruction is a delayslot instruction */
> +    uint32_t is_delay_slot_instruction;
> +} status_t;

"status_t" is much too general a name, and is not unlikely to conflict with
something from somewhere else.

Why are you exploding all of the bits of status anyway?  I would think that
only VCNZ should be handled specially, and only for the current context.

You should be using CamelCase as per CODING_STYLE, and probably using a prefix
of Arc or ARC for *everything*.

> +
> +/* ARC processor timer module. */
> +typedef struct {
> +    /*
> +     * TODO: This volatile is needed to pass RTC tests. We need to
> +     * verify why.
> +     */
> +    volatile uint32_t T_Cntrl;
> +    volatile uint32_t T_Limit;
> +    volatile uint64_t last_clk;

That is a serious bug, probably incorrect usage of locks.

> +typedef struct CPUARCState {
> +    uint32_t        r[64];
> +
> +    status_t stat, stat_l1, stat_er;



> +
> +    struct {
> +        uint32_t    S2;
> +        uint32_t    S1;
> +        uint32_t    CS;
> +    } macmod;
> +
> +    uint32_t intvec;
> +
> +    uint32_t eret;
> +    uint32_t erbta;
> +    uint32_t ecr;
> +    uint32_t efa;
> +    uint32_t bta;
> +    uint32_t bta_l1;
> +    uint32_t bta_l2;
> +
> +    uint32_t pc;     /*  program counter         */

Why is this here?  Surely it's redundant with r[63].

> +    struct {
> +        uint32_t LD;     /*  load pending bit        */
> +        uint32_t SH;     /*  self halt               */
> +        uint32_t BH;     /*  breakpoint halt         */
> +        uint32_t UB;     /*  user mode break enabled */
> +        uint32_t ZZ;     /*  sleep mode              */
> +        uint32_t RA;     /*  reset applied           */
> +        uint32_t IS;     /*  single instruction step */
> +        uint32_t FH;     /*  force halt              */
> +        uint32_t SS;     /*  single step             */
> +    } debug;

Why are these bits expanded to words?

> +    /* used for propagatinng "hostpc/return address" to sub-functions */
> +    uintptr_t host_pc;

This is a bad idea, IMO.

> +    /* Fields after this point are preserved across CPU reset. */
> +    uint64_t features;
> +    uint32_t family;

Usually such fields belong in ArchCPU (ArcCPU) and not CPUArchState.

> +  /* ARC Configuration Settings. */
> +  struct {
> +    uint32_t addr_size;
> +    bool     aps_feature;
> +    bool     byte_order;
> +    bool     bitscan_option;
> +    uint32_t br_bc_entries;

Please sort the fields by size/alignment.  This ordering is wasteful.

> +/* are we in user mode? */
> +static inline bool is_user_mode(const CPUARCState *env)
> +{
> +    return env->stat.Uf != false;
> +}

Well, ignoring for the moment that stat should not be expanded to words, this
is a silly way to return a bool.  Better as just

    return env->stat.Uf.

> +static inline int arc_feature(const CPUARCState *env, int feature)

Return bool.

> +static inline void  arc_set_feature(CPUARCState *env, int feature)

extra space.

> +static inline int cpu_mmu_index(const CPUARCState *env, bool ifetch)
> +{
> +    return env->stat.Uf != 0 ? 1 : 0;
> +}

That's a silly way to write Uf != 0.

> +    *pc = env->pc;
> +    *cs_base = 0;
> +#ifdef CONFIG_USER_ONLY
> +    *pflags = TB_FLAGS_FP_ENABLE;
> +#else
> +    *pflags = cpu_mmu_index(env, 0);
> +#endif

Why does !CONFIG_USER_ONLY never set TB_FLAGS_FP_ENABLE?

> +static inline int cpu_interrupts_enabled(const CPUARCState *env)
> +{
> +    return env->stat.IEf;
> +}

This is not used.  Copy and paste from some other target?

> +/* these are the helper functions used both by translation and gdbstub */
> +target_ulong helper_lr(CPUARCState *env, uint32_t aux);
> +void helper_sr(CPUARCState *env, uint32_t val, uint32_t aux);

Don't declare these twice, just include "exec/helper-proto.h" in gdbstub.c.


r~