[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 04/10] target-arm: replace cpsr/xpsr/pstate_r
From: |
Peter Maydell |
Subject: |
Re: [Qemu-devel] [PATCH v2 04/10] target-arm: replace cpsr/xpsr/pstate_read calls |
Date: |
Mon, 4 Aug 2014 13:59:07 +0100 |
On 10 July 2014 16:50, Alex Bennée <address@hidden> wrote:
> Use the unified save_state_to_spsr. I've also updated the interrupt
> helpers to restore via the restore_state_from_spsr() functions. In the
> aarch32 case this also needs to call switch_mode() to do the appropriate
> fiddling.
>
> Signed-off-by: Alex Bennée <address@hidden>
>
> --
>
> v2
> - include xpsr_read conversions
> - checkpatch fixes
>
> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index 60777fe..577e1d3 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c
> @@ -321,7 +321,7 @@ static void elf_core_copy_regs(target_elf_gregset_t
> *regs, const CPUARMState *en
> (*regs)[14] = tswapreg(env->regs[14]);
> (*regs)[15] = tswapreg(env->regs[15]);
>
> - (*regs)[16] = tswapreg(cpsr_read((CPUARMState *)env));
> + (*regs)[16] = tswapreg(save_state_to_spsr((CPUARMState *)env));
Using a function named "save_something" to *read* CPU
state is really confusing.
Also, the CPSR format is not the same as the AArch32 SPSR
format -- for instance bit 20 is IL in the SPSR, but we must not
allow a guest in AArch32 to read or write PSTATE.IL via a
CPSR read or write insn. We should check whether the value
the kernel exposes in places like the signal handler context
structs is the SPSR format or not.
> (*regs)[17] = tswapreg(env->regs[0]); /* XXX */
> }
>
> @@ -509,7 +509,7 @@ static void elf_core_copy_regs(target_elf_gregset_t *regs,
> (*regs)[i] = tswapreg(env->xregs[i]);
> }
> (*regs)[32] = tswapreg(env->pc);
> - (*regs)[33] = tswapreg(pstate_read((CPUARMState *)env));
> + (*regs)[33] = tswapreg(save_state_to_spsr((CPUARMState *)env));
> }
>
> #define USE_ELF_CORE_DUMP
> diff --git a/linux-user/main.c b/linux-user/main.c
> index b453a39..8848e15 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -546,7 +546,7 @@ do_kernel_trap(CPUARMState *env)
> operations. However things like ldrex/strex are much harder so
> there's not much point trying. */
> start_exclusive();
> - cpsr = cpsr_read(env);
> + cpsr = save_state_to_spsr(env);
> addr = env->regs[2];
> /* FIXME: This should SEGV if the access fails. */
> if (get_user_u32(val, addr))
> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index 86fae3d..9c6727b 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -1224,7 +1224,7 @@ static int target_setup_sigframe(struct
> target_rt_sigframe *sf,
> }
> __put_user(env->xregs[31], &sf->uc.tuc_mcontext.sp);
> __put_user(env->pc, &sf->uc.tuc_mcontext.pc);
> - __put_user(pstate_read(env), &sf->uc.tuc_mcontext.pstate);
> + __put_user(save_state_to_spsr(env), &sf->uc.tuc_mcontext.pstate);
>
> __put_user(env->exception.vaddress, &sf->uc.tuc_mcontext.fault_address);
>
> @@ -1572,7 +1572,7 @@ setup_sigcontext(struct target_sigcontext *sc, /*struct
> _fpstate *fpstate,*/
> __put_user(env->regs[14], &sc->arm_lr);
> __put_user(env->regs[15], &sc->arm_pc);
> #ifdef TARGET_CONFIG_CPU_32
> - __put_user(cpsr_read(env), &sc->arm_cpsr);
> + __put_user(save_state_to_spsr(env), &sc->arm_cpsr);
> #endif
>
> __put_user(/* current->thread.trap_no */ 0, &sc->trap_no);
> @@ -1604,7 +1604,7 @@ setup_return(CPUARMState *env, struct target_sigaction
> *ka,
> abi_ulong handler = ka->_sa_handler;
> abi_ulong retcode;
> int thumb = handler & 1;
> - uint32_t cpsr = cpsr_read(env);
> + uint32_t cpsr = save_state_to_spsr(env);
>
> cpsr &= ~CPSR_IT;
> if (thumb) {
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index fe4d4f3..3f23167 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -480,30 +480,8 @@ int arm_cpu_handle_mmu_fault(CPUState *cpu, vaddr
> address, int rw,
> #define PSTATE_MODE_EL1t 4
> #define PSTATE_MODE_EL0t 0
>
> -/* ARMv8 ARM D1.7 Process state, PSTATE
> - *
> - * 31 28 27 24 23 22 21 20 22 21 20 19 16 15 8 7 5 4 0
> - * +------+------+-------+-----+--------+---+------+------+-----+------+
> - * | NZCV | DAIF | SS IL | EL | nRW SP | Q | GE | IT | JTE | Mode |
> - * +------+------+-------+-----+--------+---+------+------+-----+------+
> - *
> - * The PSTATE is an abstraction of a number of Return the current
> - * PSTATE value. This is only valid for A64 hardware although can be
> - * read when in AArch32 mode.
> - */
> -static inline uint32_t pstate_read(CPUARMState *env)
> -{
> - int ZF;
> -
> - g_assert(is_a64(env));
> -
> - ZF = (env->ZF == 0);
> - return (env->NF & 0x80000000) | (ZF << 30)
> - | (env->CF << 29) | ((env->VF & 0x80000000) >> 3)
> - | env->pstate | env->daif;
> -}
??? You just added this function and comment in an earlier
patch. Rebasing damage?
> -
> -/* Update the current PSTATE value. This doesn't include nRW which is */
> +/* Update the current PSTATE value. This doesn't include nRW which
> + * indicates if we are in 64 or 32 bit mode */
> static inline void pstate_write(CPUARMState *env, uint32_t val)
> {
> g_assert(is_a64(env));
> @@ -520,26 +498,10 @@ static inline void pstate_write(CPUARMState *env,
> uint32_t val)
> *
> * Unlike the above PSTATE implementation these functions will attempt
> * to switch processor mode when the M[4:0] bits are set.
> - */
> -uint32_t cpsr_read(CPUARMState *env);
> -/* Set the CPSR. Note that some bits of mask must be all-set or all-clear.
> */
> + *
> + * Note that some bits of mask must be all-set or all-clear. */
> void cpsr_write(CPUARMState *env, uint32_t val, uint32_t mask);
>
> -/* ARMv7-M ARM B1.4.2, special purpose program status register xPSR */
> -static inline uint32_t xpsr_read(CPUARMState *env)
> -{
> - int ZF;
> -
> - g_assert(!is_a64(env));
> -
> - ZF = (env->ZF == 0);
> - return (env->NF & 0x80000000) | (ZF << 30)
> - | (env->CF << 29) | ((env->VF & 0x80000000) >> 3) | (env->QF << 27)
> - | (env->thumb << 24) | ((env->condexec_bits & 3) << 25)
> - | ((env->condexec_bits & 0xfc) << 8)
> - | env->v7m.exception;
> -}
> -
> /* Set the xPSR. Note that some bits of mask must be all-set or all-clear.
> */
> static inline void xpsr_write(CPUARMState *env, uint32_t val, uint32_t mask)
> {
> diff --git a/target-arm/gdbstub.c b/target-arm/gdbstub.c
> index 1c34396..ec25f30 100644
> --- a/target-arm/gdbstub.c
> +++ b/target-arm/gdbstub.c
> @@ -53,7 +53,7 @@ int arm_cpu_gdb_read_register(CPUState *cs, uint8_t
> *mem_buf, int n)
> return gdb_get_reg32(mem_buf, 0);
> case 25:
> /* CPSR */
> - return gdb_get_reg32(mem_buf, cpsr_read(env));
> + return gdb_get_reg32(mem_buf, save_state_to_spsr(env));
> }
> /* Unknown register. */
> return 0;
> diff --git a/target-arm/gdbstub64.c b/target-arm/gdbstub64.c
> index 8f3b8d1..76d1b91 100644
> --- a/target-arm/gdbstub64.c
> +++ b/target-arm/gdbstub64.c
> @@ -35,7 +35,7 @@ int aarch64_cpu_gdb_read_register(CPUState *cs, uint8_t
> *mem_buf, int n)
> case 32:
> return gdb_get_reg64(mem_buf, env->pc);
> case 33:
> - return gdb_get_reg32(mem_buf, pstate_read(env));
> + return gdb_get_reg32(mem_buf, save_state_to_spsr(env));
> }
> /* Unknown register. */
> return 0;
> @@ -62,7 +62,7 @@ int aarch64_cpu_gdb_write_register(CPUState *cs, uint8_t
> *mem_buf, int n)
> env->pc = tmp;
> return 8;
> case 33:
> - /* CPSR */
> + /* SPSR */
> pstate_write(env, tmp);
> return 4;
> }
> diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
> index ec1fef5..03cd64f 100644
> --- a/target-arm/helper-a64.c
> +++ b/target-arm/helper-a64.c
> @@ -445,6 +445,7 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
> CPUARMState *env = &cpu->env;
> target_ulong addr = env->cp15.vbar_el[1];
> int i;
> + uint32_t spsr = save_state_to_spsr(env);
>
> if (arm_current_pl(env) == 0) {
> if (env->aarch64) {
> @@ -452,7 +453,7 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
> } else {
> addr += 0x600;
> }
> - } else if (pstate_read(env) & PSTATE_SP) {
> + } else if (spsr & PSTATE_SP) {
> addr += 0x200;
> }
>
> @@ -488,12 +489,12 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
> }
>
> if (is_a64(env)) {
> - env->banked_spsr[aarch64_banked_spsr_index(1)] = pstate_read(env);
> + env->banked_spsr[aarch64_banked_spsr_index(1)] = spsr;
> env->sp_el[arm_current_pl(env)] = env->xregs[31];
> env->xregs[31] = env->sp_el[1];
> env->elr_el[1] = env->pc;
> } else {
> - env->banked_spsr[0] = cpsr_read(env);
> + env->banked_spsr[0] = spsr;
> if (!env->thumb) {
> env->cp15.esr_el[1] |= 1 << 25;
> }
> @@ -506,6 +507,7 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
> env->condexec_bits = 0;
> }
>
> + // TODO: restore_state_from_spsr()
Don't leave TODO comments in patches...
> env->aarch64 = 1;
> pstate_write(env, PSTATE_DAIF | PSTATE_MODE_EL1h);
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index d343856..030bcdd 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -2990,17 +2990,6 @@ static int bad_mode_switch(CPUARMState *env, int mode)
> }
> }
>
> -uint32_t cpsr_read(CPUARMState *env)
> -{
> - int ZF;
> - ZF = (env->ZF == 0);
> - return env->uncached_cpsr | (env->NF & 0x80000000) | (ZF << 30) |
> - (env->CF << 29) | ((env->VF & 0x80000000) >> 3) | (env->QF << 27)
> - | (env->thumb << 5) | ((env->condexec_bits & 3) << 25)
> - | ((env->condexec_bits & 0xfc) << 8)
> - | (env->GE << 16) | (env->daif & CPSR_AIF);
> -}
> -
> void cpsr_write(CPUARMState *env, uint32_t val, uint32_t mask)
> {
> if (mask & CPSR_NZCV) {
> @@ -3278,7 +3267,7 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
> {
> ARMCPU *cpu = ARM_CPU(cs);
> CPUARMState *env = &cpu->env;
> - uint32_t xpsr = xpsr_read(env);
> + uint32_t xpsr = save_state_to_spsr(env);
> uint32_t lr;
> uint32_t addr;
>
> @@ -3479,7 +3468,8 @@ void arm_cpu_do_interrupt(CPUState *cs)
> addr += env->cp15.vbar_el[1];
> }
> switch_mode (env, new_mode);
> - env->spsr = cpsr_read(env);
> + env->spsr = save_state_to_spsr(env);
> + /* TODO: restore_state_from_spsr */
Another TODO comment here...
> /* Clear IT bits. */
> env->condexec_bits = 0;
> /* Switch to the new mode, and to the correct instruction set. */
> @@ -4212,19 +4202,19 @@ uint32_t HELPER(v7m_mrs)(CPUARMState *env, uint32_t
> reg)
>
> switch (reg) {
> case 0: /* APSR */
> - return xpsr_read(env) & 0xf8000000;
> + return save_state_to_spsr(env) & 0xf8000000;
> case 1: /* IAPSR */
> - return xpsr_read(env) & 0xf80001ff;
> + return save_state_to_spsr(env) & 0xf80001ff;
> case 2: /* EAPSR */
> - return xpsr_read(env) & 0xff00fc00;
> + return save_state_to_spsr(env) & 0xff00fc00;
> case 3: /* xPSR */
> - return xpsr_read(env) & 0xff00fdff;
> + return save_state_to_spsr(env) & 0xff00fdff;
> case 5: /* IPSR */
> - return xpsr_read(env) & 0x000001ff;
> + return save_state_to_spsr(env) & 0x000001ff;
> case 6: /* EPSR */
> - return xpsr_read(env) & 0x0700fc00;
> + return save_state_to_spsr(env) & 0x0700fc00;
> case 7: /* IEPSR */
> - return xpsr_read(env) & 0x0700edff;
> + return save_state_to_spsr(env) & 0x0700edff;
> case 8: /* MSP */
> return env->v7m.current_sp ? env->v7m.other_sp : env->regs[13];
> case 9: /* PSP */
This all seems more confusing rather than less. What's wrong
with still having functions like xpsr_read() ?
thanks
-- PMM
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [Qemu-devel] [PATCH v2 04/10] target-arm: replace cpsr/xpsr/pstate_read calls,
Peter Maydell <=