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
