Re: [Qemu-devel] [PATCH RFC v4 03/12] target/rx: CPU definition

[Richard Henderson]

On 3/20/19 7:16 AM, Yoshinori Sato wrote:
> +#define FPSW_MASK 0xfc007cff
> +#define FPSW_RM_MASK 0x00000003
> +#define FPSW_DN (1 << 8)

It's slightly confusing to have this as a mask,

> +#define FPSW_CAUSE_MASK 0x000000fc
> +#define FPSW_CAUSE_SHIFT 2
> +#define FPSW_CAUSE   2
> +#define FPSW_CAUSE_V 2

and these as a bit.  You might either rename to FPSW_DN_MASK, or define as a 
bit.

Alternately, convert all of these to use "hw/registerfields.h", a la

FIELD(FPSW, RM, 0, 2)
FIELD(FPSW, CAUSE, 2, 6)
FIELD(FPSW, CV, 2, 1)
...
FIELD(FPSW, DN, 8, 1)
FIELD(FPSW, ENABLE, 10, 5)
FIELD(FPSW, EV, 10, 1)
...
FIELD(FPSW, FLAG, 26, 5)
FIELD(FPSW, FV, 26, 1)
...
FIELD(FPSW, FS, 31, 1)

> +#define FPSW_CAUSE_O 3
> +#define FPSW_CAUSE_Z 4
> +#define FPSW_CAUSE_U 5
> +#define FPSW_CAUSE_X 6
> +#define FPSW_CAUSE_E 7
> +#define FPSW_ENABLE_MASK 0x00007c00
> +#define FPSW_ENABLE  10

If not using FIELD, perhaps FPSW_ENABLE_SHIFT to match FPSW_CAUSE_SHIFT?

> +#define RX_PSW_OP_NONE 0
> +#define RX_PSW_OP_SUB 1
> +#define RX_PSW_OP_ADD 2
> +#define RX_PSW_OP_SHLL 3

Unused.

> +typedef struct CPURXState {
> +    /* CPU registers */
> +    uint32_t regs[16];          /* general registers */
> +    uint32_t psw;               /* processor status */

As discussed in reply to patch 2, remove this.

> +    /* Flag operation */
> +    uint32_t psw_op;
> +    uint32_t psw_v[3];

Unused.

> +static inline void cpu_get_tb_cpu_state(CPURXState *env, target_ulong *pc,
> +                                        target_ulong *cs_base, uint32_t 
> *flags)
> +{
> +    *pc = env->pc;
> +    *cs_base = 0;
> +    *flags = deposit32(*flags, PSW_PM,  1, env->psw_pm);

It might be worthwhile to include PSW_U.  I'll discuss elsewhere

> +static inline uint32_t pack_psw(CPURXState *env)

Why is this missing rx_cpu_ prefix, to match rx_cpu_unpack_psw?

> +static bool rx_cpu_has_work(CPUState *cs)
> +{
> +    return cs->interrupt_request & CPU_INTERRUPT_HARD;

Surely CPU_INTERRUPT_FIR needs to be included here.

> +static void rx_cpu_reset(CPUState *s)
> +{
> +    RXCPU *cpu = RXCPU(s);
> +    RXCPUClass *rcc = RXCPU_GET_CLASS(cpu);
> +    CPURXState *env = &cpu->env;
> +    uint32_t *resetvec;
> +
> +    rcc->parent_reset(s);
> +
> +    memset(env, 0, offsetof(CPURXState, end_reset_fields));
> +
> +    resetvec = rom_ptr(0xfffffffc, 4);
> +    if (resetvec) {
> +        /* In the case of kernel, it is ignored because it is not set. */
> +        env->pc = ldl_p(resetvec);
> +    }
> +    env->psw = 0x00000000;

You can't just assign, you need.

  rx_cpu_unpack_psw(env, 0, 1);

And, as a reminder from patch 2 review, set fp_status here.

> +static uint32_t extable[32];
> +
> +void rx_load_image(RXCPU *cpu, const char *filename,
> +                   uint32_t start, uint32_t size)
> +{
> +    long kernel_size;
> +    int i;
> +
> +    kernel_size = load_image_targphys(filename, start, size);
> +    if (kernel_size < 0) {
> +        fprintf(stderr, "qemu: could not load kernel '%s'\n", filename);
> +        exit(1);
> +    }
> +    cpu->env.pc = start;
> +
> +    /* setup exception trap trampoline */
> +    for (i = 0; i < 32; i++) {
> +        extable[i] = 0x10 + i * 4;
> +    }

These assignments need to be in target-endianness, not host-endianness.

Since RX is currently little-endian only, perhaps just

    extable[i] = cpu_to_le32(0x10 + i * 4);

> +    rom_add_blob_fixed("extable", extable, sizeof(extable), 0xffffff80);
> +}
> 



r~