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

[Richard Henderson <richard.henderson@linaro.org>]

On 4/5/21 7:31 AM, cupertinomiranda@gmail.com wrote:
> +    DEFINE_PROP_BOOL("byte-order", ARCCPU, cfg.byte_order, false),

"byte-order" makes no sense as a bool.
"little-endian" or "big-endian" would.

> +    info->endian = BFD_ENDIAN_LITTLE;

Not using the setting?

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

Should be redundant with .editorconfig.

> +#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])

Surely this is better as a enum of regnos?

I'm not especially keen on lvalue macros like this, especially when you can't 
reuse the enum for e.g. the tcg globals.

> +/* Flags in pstate */
> +#define Hf_b  (0)
> +#define AEf_b (5)
> +#define Uf_b  (7)
> +#define Lf_b  (12)
> +#define DZf_b (13)
> +#define SCf_b (14)
> +#define ESf_b (15)
> +#define ADf_b (19)
> +#define USf_b (20)
> +
> +/* Flags with their on fields */
> +#define IEf_b   (31)
> +#define IEf_bS  (1)
> +
> +#define Ef_b    (1)
> +#define Ef_bS   (4)
> +
> +#define DEf_b   (6)
> +#define DEf_bS  (1)
> +
> +#define Vf_b    (8)
> +#define Vf_bS   (1)
> +#define Cf_b    (9)
> +#define Cf_bS   (1)
> +#define Nf_b    (10)
> +#define Nf_bS   (1)
> +#define Zf_b    (11)
> +#define Zf_bS   (1)
> +
> +#define RBf_b   (16)
> +#define RBf_bS  (3)

We have include/hw/registerfields.h that's a bit better at defining, 
extracting, and setting fields.

> +#define SET_STATUS_BIT(STAT, BIT, VALUE) { \
> +    STAT.pstate &= ~(1 << BIT##_b); \
> +    STAT.pstate |= (VALUE << BIT##_b); \
> +}

do {
     (STAT).pstate = deposit32((STAT).pstate, BIT, 1, VALUE);
} while (0)

> +typedef struct {
> +    target_ulong pstate;
> +
> +    target_ulong RBf;
> +    target_ulong Ef;     /* irq priority treshold. */
> +    target_ulong Vf;     /*  overflow                */
> +    target_ulong Cf;     /*  carry                   */
> +    target_ulong Nf;     /*  negative                */
> +    target_ulong Zf;     /*  zero                    */
> +    target_ulong DEf;
> +    target_ulong IEf;

I understand why the 4 arithmetic flags are split out, but why are the others? 
  Surely they are not nearly so performance sensitive.

> +/* ARC PIC interrupt bancked regs. */
> +typedef struct {
> +    target_ulong priority;
> +    target_ulong trigger;
> +    target_ulong pulse_cancel;
> +    target_ulong enable;
> +    target_ulong pending;
> +    target_ulong status;
> +} ARCIrq;

This is cpu.h.  The PIC is not the cpu, so this should be elsewhere.

> +typedef struct CPUARCState {
> +    target_ulong        r[64];
> +
> +    ARCStatus stat, stat_l1, stat_er;
> +
> +    struct {
> +        target_ulong    S2;
> +        target_ulong    S1;
> +        target_ulong    CS;
> +    } macmod;
> +
> +    target_ulong intvec;
> +
> +    target_ulong eret;
> +    target_ulong erbta;
> +    target_ulong ecr;
> +    target_ulong efa;
> +    target_ulong bta;
> +    target_ulong bta_l1;
> +    target_ulong bta_l2;
> +
> +    target_ulong pc;     /*  program counter         */

What is this and why is it different from PCL, aka r[63]?

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

Not a fan.  Subfunctions should have the retaddr passed down directly, so that 
it's obvious that the value is not stale.

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

A very complicated way to just return GET_STATUS_BIT.
Or even GET_STATUS_BIT != 0, come to that.

> +
> +static inline void cpu_get_tb_cpu_state(CPUARCState *env, target_ulong *pc,
> +                                        target_ulong *cs_base,
> +                                        uint32_t *pflags)
> +{
> +    *pc = env->pc;
> +    *cs_base = 0;
> +#ifdef CONFIG_USER_ONLY
> +    assert(0); /* Not really supported at the moment. */

g_assert_not_reached() is our canonical "can't get here".
Though #error would be better in this instance.


r~