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

[Stacey Son]

The arm code came from another source as noted in the cover letter.  It could 
use a lot more work.

-stacey.


On Oct 16, 2013, at 10:38 AM, Alex Bennée <address@hidden> wrote:

> 
> address@hidden 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 <address@hidden>
> <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