Re: [Qemu-devel] [PATCH 04/18] bsd-user: move target arch and host OSdependent code out of main.cc

["Alex Bennée" <alex.bennee@linaro.org>]

sson@FreeBSD.org writes:

> This change moves the cpu initialization and main loop code from
> main.c to the OS and arch dependent directories. This eliminates
> many of the #ifdef's in main.c. The cpu initialization and loop
> code is now located in the arch directory along with target arch
> support code.
>
> Signed-off-by: Stacey Son <sson@FreeBSD.org>
<snip>
> +#if 0
> +                TaskState *ts = env->opaque;
> +                uint32_t opcode;
> +                int rc;
> +
> +                /* we handle the FPU emulation here, as Linux */
> +                /* we get the opcode */
> +                /* FIXME - what to do if get_user() fails? */
> +                get_user_u32(opcode, env->regs[15]);
> +
> +                rc = EmulateAll(opcode, &ts->fpa, env);
> +                if (rc == 0) { /* illegal instruction */
> +                    info.si_signo = SIGILL;
> +                    info.si_errno = 0;
> +                    info.si_code = TARGET_ILL_ILLOPN;
> +                    info._sifields._sigfault._addr = env->regs[15];
> +                    queue_signal(env, info.si_signo, &info);
> +
> +                } else if (rc < 0) { /* FP exception */
> +                    int arm_fpe = 0;
> +
> +                    /* translate softfloat flags to FPSR flags */
> +                    if (-rc & float_flag_invalid) {
> +                        arm_fpe |= BIT_IOC;
> +                    }
> +                    if (-rc & float_flag_divbyzero) {
> +                        arm_fpe |= BIT_DZC;
> +                    }
> +                    if (-rc & float_flag_overflow) {
> +                        arm_fpe |= BIT_OFC;
> +                    }
> +                    if (-rc & float_flag_underflow) {
> +                        arm_fpe |= BIT_UFC;
> +                    }
> +                    if (-rc & float_flag_inexact) {
> +                        arm_fpe |= BIT_IXC;
> +                    }
> +
> +                    FPSR fpsr = ts->fpa.fpsr;
> +                    /* printf("fpsr 0x%x, arm_fpe 0x%x\n",fpsr,arm_fpe); */
> +
> +                    if (fpsr & (arm_fpe << 16)) { /* exception enabled? */
> +                        info.si_signo = SIGFPE;
> +                        info.si_errno = 0;
> +
> +                        /* ordered by priority, least first */
> +                        if (arm_fpe & BIT_IXC) {
> +                            info.si_code = TARGET_FPE_FLTRES;
> +                        }
> +                        if (arm_fpe & BIT_UFC) {
> +                            info.si_code = TARGET_FPE_FLTUND;
> +                        }
> +                        if (arm_fpe & BIT_OFC) {
> +                            info.si_code = TARGET_FPE_FLTOVF;
> +                        }
> +                        if (arm_fpe & BIT_DZC) {
> +                            info.si_code = TARGET_FPE_FLTDIV;
> +                        }
> +                        if (arm_fpe & BIT_IOC) {
> +                            info.si_code = TARGET_FPE_FLTINV;
> +                        }
> +                        info._sifields._sigfault._addr = env->regs[15];
> +                        queue_signal(env, info.si_signo, &info);
> +                    } else {
> +                        env->regs[15] += 4;
> +                    }
> +
> +                    /* accumulate unenabled exceptions */
> +                    if ((!(fpsr & BIT_IXE)) && (arm_fpe & BIT_IXC)) {
> +                        fpsr |= BIT_IXC;
> +                    }
> +                    if ((!(fpsr & BIT_UFE)) && (arm_fpe & BIT_UFC)) {
> +                        fpsr |= BIT_UFC;
> +                    }
> +                    if ((!(fpsr & BIT_OFE)) && (arm_fpe & BIT_OFC)) {
> +                        fpsr |= BIT_OFC;
> +                    }
> +                    if ((!(fpsr & BIT_DZE)) && (arm_fpe & BIT_DZC)) {
> +                        fpsr |= BIT_DZC;
> +                    }
> +                    if ((!(fpsr & BIT_IOE)) && (arm_fpe & BIT_IOC)) {
> +                        fpsr |= BIT_IOC;
> +                    }
> +                    ts->fpa.fpsr = fpsr;
> +                } else { /* everything OK */
> +                    /* increment PC */
> +                    env->regs[15] += 4;
> +                }
> +            }
> +#endif

I'm fairly sure that should either be deleted or re-instated. We have
SCMs for a reason ;-)

> +            break;
> +        case EXCP_SWI:
> +        case EXCP_BKPT:
> +            {
> +                env->eabi = 1;
> +                /* system call */
> +                if (trapnr == EXCP_BKPT) {
> +                    if (env->thumb) {
> +                        /* FIXME - what to do if get_user() fails? */
> +#ifdef FREEBSD_ARM_OABI
> +                        get_user_u16(insn, env->regs[15]);
> +                        n = insn & 0xff;
> +#else
> +                        n = env->regs[7];
> +#endif
> +                        env->regs[15] += 2;
> +                    } else {
> +                        /* FIXME - what to do if get_user() fails? */
> +#ifdef FREEBSD_ARM_OABI
> +                        get_user_u32(insn, env->regs[15]);
> +                        n = (insn & 0xf) | ((insn >> 4) & 0xff0);
> +#else
> +                        n = env->regs[7];
> +#endif
> +                        env->regs[15] += 4;
> +                    }
> +                } else {
> +                    if (env->thumb) {
> +#ifdef FREEBSD_ARM_OABI
> +                        /* FIXME - what to do if get_user() fails? */
> +                        get_user_u16(insn, env->regs[15] - 2);
> +                        n = insn & 0xff;
> +#else
> +                        n = env->regs[7];
> +#endif
> +                    } else {
> +#ifdef FREEBSD_ARM_OABI
> +                        /* FIXME - what to do if get_user() fails? */
> +                        get_user_u32(insn, env->regs[15] - 4);
> +                        n = insn & 0xffffff;
> +#else
> +                        n = env->regs[7];
> +#endif
> +                    }
> +                }
> +
> +#ifdef DEBUG_ARM
> +        printf("AVANT CALL %d\n", n);
> +#endif

I think debug statements (rather than user visible logging) should
generally be wrapped up in a macro.

> +                if (bsd_type == target_freebsd) {
> +                    int ret;
> +                    abi_ulong params = get_sp_from_cpustate(env);
> +                    int32_t syscall_nr = n;
> +                    int32_t arg1, arg2, arg3, arg4, arg5, arg6, arg7, arg8;
> +

Another #if 0, there are several more in the patch.

> +#if 0 /* XXX FIXME */
<snip>
> +static inline abi_ulong get_sp_from_cpustate(CPUARMState *state)
> +{
> +    return state->regs[13]; /* sp */
> +}
> +
> +static inline void set_second_rval(CPUARMState *state, abi_ulong retval2)
> +{
> +    state->regs[1] = retval2;
> +}

It's a shame ARM hasn't got some un-ambigious #define's for registers

<snip>

More #if 0's

> +#if 0
> +        case EXCP0B_NOSEG:
> +        case EXCP0C_STACK:
<snip>


-- 
Alex Bennée