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~
