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

[Yoshinori Sato]

On Fri, 22 Mar 2019 04:28:03 +0900,
Richard Henderson wrote:
> 
> 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)

OK. PSW and FPSW converted to registerfield.

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

OK. It rewrite register field API.

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

OK. removed.

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

OK. removed.

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

Likewise.

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

PSW_U is not affected translation now.
If translate switches ISP / USP when it refers to the stack pointer,
it needs to include the U bit here.
But translate now uses the copied value, so there is no need to switch.

> > +static inline uint32_t pack_psw(CPURXState *env)
> 
> Why is this missing rx_cpu_ prefix, to match rx_cpu_unpack_psw?

OK. Added prefix.

> > +static bool rx_cpu_has_work(CPUState *cs)
> > +{
> > +    return cs->interrupt_request & CPU_INTERRUPT_HARD;
> 
> Surely CPU_INTERRUPT_FIR needs to be included here.

Yes. FIR is special hardware interrupt.

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

OK.

> > +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);

OK.
This part use only little-endian mode. no problem.

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

-- 
Yosinori Sato