target-arm queue:

* mostly my latest v8M stuff, plus a couple of minor patches

The following changes since commit a0b261db8c030813e30a39eae47359ac2a37f7e2:

Merge remote-tracking branch 'remotes/ehabkost/tags/python-next-pull-request' into staging (2017-10-12 10:02:09 +0100)

are available in the git repository at:

git://git.linaro.org/people/pmaydell/qemu-arm.git tags/pull-target-arm-20171012

for you to fetch changes up to cf5f7937b05c84d5565134f058c00cd48304a117:

nvic: Fix miscalculation of offsets into ITNS array (2017-10-12 16:33:16 +0100)

* v8M: SG, BLXNS, secure-return

* v8M: fixes for coverity issues in previous patches

* arm: fix armv7m_init() declaration to match definition

* watchdog/aspeed: fix variable type to store reload value

Cédric Le Goater (1):

watchdog/aspeed: fix variable type to store reload value

Igor Mammedov (1):

arm: fix armv7m_init() declaration to match definition

Peter Maydell (11):

target/arm: Add M profile secure MMU index values to get_a32_user_mem_index()

target/arm: Implement SG instruction

target/arm: Implement BLXNS

target/arm: Implement secure function return

target-arm: Don't check for "Thumb2 or M profile" for not-Thumb1

target/arm: Pull Thumb insn word loads up to top level

target-arm: Simplify insn_crosses_page()

target/arm: Support some Thumb insns being always unconditional

target/arm: Implement SG instruction corner cases

nvic: Add missing 'break'

nvic: Fix miscalculation of offsets into ITNS array

include/hw/arm/arm.h | 2 +-

target/arm/helper.h | 1 +

target/arm/internals.h | 8 ++

hw/intc/armv7m_nvic.c | 5 +-

hw/watchdog/wdt_aspeed.c | 4 +-

target/arm/helper.c | 306 ++++++++++++++++++++++++++++++++++++++++++++--

target/arm/translate.c | 310 ++++++++++++++++++++++++++++++++---------------

7 files changed, 521 insertions(+), 115 deletions(-)

Secure function return happens when a non-secure function has been

called using BLXNS and so has a particular magic LR value (either

0xfefffffe or 0xfeffffff). The function return via BX behaves

specially when the new PC value is this magic value, in the same

way that exception returns are handled.

Adjust our BX excret guards so that they recognize the function

return magic number as well, and perform the function-return

unstacking in do_v7m_exception_exit().

target/arm/internals.h | 7 +++

target/arm/helper.c | 115 +++++++++++++++++++++++++++++++++++++++++++++----

target/arm/translate.c | 14 +++++-

3 files changed, 126 insertions(+), 10 deletions(-)

@@ -XXX,XX +XXX,XX @@ void HELPER(v7m_bxns)(CPUARMState *env, uint32_t dest)

* - if the return value is a magic value, do exception return (like BX)

* - otherwise bit 0 of the return value is the target security state

*/

- if (dest >= 0xff000000) {

+ uint32_t min_magic;

+

+ if (arm_feature(env, ARM_FEATURE_M_SECURITY)) {

+ /* Covers FNC_RETURN and EXC_RETURN magic */

+ min_magic = FNC_RETURN_MIN_MAGIC;

+ } else {

+ /* EXC_RETURN magic only */

+ min_magic = EXC_RETURN_MIN_MAGIC;

+ }

+

+ if (dest >= min_magic) {

/* This is an exception return magic value; put it where

* do_v7m_exception_exit() expects and raise EXCEPTION_EXIT.

* Note that if we ever add gen_ss_advance() singlestep support to

@@ -XXX,XX +XXX,XX @@ static void do_v7m_exception_exit(ARMCPU *cpu)

bool exc_secure = false;

bool return_to_secure;

- /* We can only get here from an EXCP_EXCEPTION_EXIT, and

- * gen_bx_excret() enforces the architectural rule

- * that jumps to magic addresses don't have magic behaviour unless

- * we're in Handler mode (compare pseudocode BXWritePC()).

+ /* If we're not in Handler mode then jumps to magic exception-exit

+ * addresses don't have magic behaviour. However for the v8M

+ * security extensions the magic secure-function-return has to

+ * work in thread mode too, so to avoid doing an extra check in

+ * the generated code we allow exception-exit magic to also cause the

+ * internal exception and bring us here in thread mode. Correct code

+ * will never try to do this (the following insn fetch will always

+ * fault) so we the overhead of having taken an unnecessary exception

+ * doesn't matter.

*/

- assert(arm_v7m_is_handler_mode(env));

+ if (!arm_v7m_is_handler_mode(env)) {

+ return;

+ }

/* In the spec pseudocode ExceptionReturn() is called directly

* from BXWritePC() and gets the full target PC value including

@@ -XXX,XX +XXX,XX @@ static void do_v7m_exception_exit(ARMCPU *cpu)

qemu_log_mask(CPU_LOG_INT, "...successful exception return\n");

}

+static bool do_v7m_function_return(ARMCPU *cpu)

+{

+ /* v8M security extensions magic function return.

+ * We may either:

+ * (1) throw an exception (longjump)

+ * (2) return true if we successfully handled the function return

+ * (3) return false if we failed a consistency check and have

+ * pended a UsageFault that needs to be taken now

+ *

+ * At this point the magic return value is split between env->regs[15]

+ * and env->thumb. We don't bother to reconstitute it because we don't

+ * need it (all values are handled the same way).

+ */

+ CPUARMState *env = &cpu->env;

+ uint32_t newpc, newpsr, newpsr_exc;

+

+ qemu_log_mask(CPU_LOG_INT, "...really v7M secure function return\n");

+

+ {

+ bool threadmode, spsel;

+ TCGMemOpIdx oi;

+ ARMMMUIdx mmu_idx;

+ uint32_t *frame_sp_p;

+ uint32_t frameptr;

+

+ /* Pull the return address and IPSR from the Secure stack */

+ threadmode = !arm_v7m_is_handler_mode(env);

+ spsel = env->v7m.control[M_REG_S] & R_V7M_CONTROL_SPSEL_MASK;

+

+ frame_sp_p = get_v7m_sp_ptr(env, true, threadmode, spsel);

+ frameptr = *frame_sp_p;

+

+ /* These loads may throw an exception (for MPU faults). We want to

+ * do them as secure, so work out what MMU index that is.

+ */

+ mmu_idx = arm_v7m_mmu_idx_for_secstate(env, true);

+ oi = make_memop_idx(MO_LE, arm_to_core_mmu_idx(mmu_idx));

+ newpc = helper_le_ldul_mmu(env, frameptr, oi, 0);

+ newpsr = helper_le_ldul_mmu(env, frameptr + 4, oi, 0);

+

+ /* Consistency checks on new IPSR */

+ newpsr_exc = newpsr & XPSR_EXCP;

+ if (!((env->v7m.exception == 0 && newpsr_exc == 0) ||

+ (env->v7m.exception == 1 && newpsr_exc != 0))) {

+ /* Pend the fault and tell our caller to take it */

+ env->v7m.cfsr[env->v7m.secure] |= R_V7M_CFSR_INVPC_MASK;

+ armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_USAGE,

+ env->v7m.secure);

+ qemu_log_mask(CPU_LOG_INT,

+ "...taking INVPC UsageFault: "

+ "IPSR consistency check failed\n");

+ return false;

+ }

+

+ *frame_sp_p = frameptr + 8;

+ }

+

+ /* This invalidates frame_sp_p */

+ switch_v7m_security_state(env, true);

+ env->v7m.exception = newpsr_exc;

+ env->v7m.control[M_REG_S] &= ~R_V7M_CONTROL_SFPA_MASK;

+ if (newpsr & XPSR_SFPA) {

+ env->v7m.control[M_REG_S] |= R_V7M_CONTROL_SFPA_MASK;

+ }

+ xpsr_write(env, 0, XPSR_IT);

+ env->thumb = newpc & 1;

+ env->regs[15] = newpc & ~1;

+

+ qemu_log_mask(CPU_LOG_INT, "...function return successful\n");

+ return true;

+}

+

static void arm_log_exception(int idx)

{

if (qemu_loglevel_mask(CPU_LOG_INT)) {

@@ -XXX,XX +XXX,XX @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)

case EXCP_IRQ:

break;

case EXCP_EXCEPTION_EXIT:

- do_v7m_exception_exit(cpu);

- return;

+ if (env->regs[15] < EXC_RETURN_MIN_MAGIC) {

+ /* Must be v8M security extension function return */

+ assert(env->regs[15] >= FNC_RETURN_MIN_MAGIC);

+ assert(arm_feature(env, ARM_FEATURE_M_SECURITY));

+ if (do_v7m_function_return(cpu)) {

+ return;

+ }

+ } else {

+ do_v7m_exception_exit(cpu);

+ return;

+ }

+ break;

default:

cpu_abort(cs, "Unhandled exception 0x%x\n", cs->exception_index);

return; /* Never happens. Keep compiler happy. */

diff --git a/target/arm/translate.c b/target/arm/translate.c

index XXXXXXX..XXXXXXX 100644

--- a/target/arm/translate.c

+++ b/target/arm/translate.c

@@ -XXX,XX +XXX,XX @@ static inline void gen_bx_excret(DisasContext *s, TCGv_i32 var)

* s->base.is_jmp that we need to do the rest of the work later.

*/

gen_bx(s, var);

- if (s->v7m_handler_mode && arm_dc_feature(s, ARM_FEATURE_M)) {

+ if (arm_dc_feature(s, ARM_FEATURE_M_SECURITY) ||

+ (s->v7m_handler_mode && arm_dc_feature(s, ARM_FEATURE_M))) {

s->base.is_jmp = DISAS_BX_EXCRET;

}

}

@@ -XXX,XX +XXX,XX @@ static inline void gen_bx_excret_final_code(DisasContext *s)

{

/* Generate the code to finish possible exception return and end the TB */

TCGLabel *excret_label = gen_new_label();

+ uint32_t min_magic;

+

+ if (arm_dc_feature(s, ARM_FEATURE_M_SECURITY)) {

+ /* Covers FNC_RETURN and EXC_RETURN magic */

+ min_magic = FNC_RETURN_MIN_MAGIC;

+ } else {

+ /* EXC_RETURN magic only */

+ min_magic = EXC_RETURN_MIN_MAGIC;

+ }

/* Is the new PC value in the magic range indicating exception return? */

- tcg_gen_brcondi_i32(TCG_COND_GEU, cpu_R[15], 0xff000000, excret_label);

+ tcg_gen_brcondi_i32(TCG_COND_GEU, cpu_R[15], min_magic, excret_label);

/* No: end the TB as we would for a DISAS_JMP */

if (is_singlestepping(s)) {

gen_singlestep_exception(s);

2.7.4

This calculation of the first exception vector in

the ITNS<n> register being accessed:

int startvec = 32 * (offset - 0x380) + NVIC_FIRST_IRQ;

is incorrect, because offset is in bytes, so we only want

to multiply by 8.

Spotted by Coverity (CID 1381484, CID 1381488), though it is

not correct that it actually overflows the buffer, because

we have a 'startvec + i < s->num_irq' guard.

Message-id: 1507650856-11718-1-git-send-email-peter.maydell@linaro.org

hw/intc/armv7m_nvic.c | 4 ++--

1 file changed, 2 insertions(+), 2 deletions(-)

return ((s->num_irq - NVIC_FIRST_IRQ) / 32) - 1;

case 0x380 ... 0x3bf: /* NVIC_ITNS<n> */

{

- int startvec = 32 * (offset - 0x380) + NVIC_FIRST_IRQ;

+ int startvec = 8 * (offset - 0x380) + NVIC_FIRST_IRQ;

int i;

if (!arm_feature(&cpu->env, ARM_FEATURE_V8)) {

switch (offset) {

case 0x380 ... 0x3bf: /* NVIC_ITNS<n> */

{

- int startvec = 32 * (offset - 0x380) + NVIC_FIRST_IRQ;

+ int startvec = 8 * (offset - 0x380) + NVIC_FIRST_IRQ;

int i;

if (!arm_feature(&cpu->env, ARM_FEATURE_V8)) {

2.7.4

The common situation of the SG instruction is that it is

executed from S&NSC memory by a CPU in NS state. That case

is handled by v7m_handle_execute_nsc(). However the instruction

also has defined behaviour in a couple of other cases:

* SG instruction in NS memory (behaves as a NOP)

* SG in S memory but CPU already secure (clears IT bits and

does nothing else)

* SG instruction in v8M without Security Extension (NOP)

These can be implemented in translate.c.

Message-id: 1507556919-24992-10-git-send-email-peter.maydell@linaro.org

target/arm/translate.c | 23 ++++++++++++++++++++++-

1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/target/arm/translate.c b/target/arm/translate.c

--- a/target/arm/translate.c

+++ b/target/arm/translate.c

@@ -XXX,XX +XXX,XX @@ static int disas_thumb2_insn(DisasContext *s, uint32_t insn)

* - load/store doubleword, load/store exclusive, ldacq/strel,

* table branch.

*/

- if (insn & 0x01200000) {

+ if (insn == 0xe97fe97f && arm_dc_feature(s, ARM_FEATURE_M) &&

+ arm_dc_feature(s, ARM_FEATURE_V8)) {

+ /* 0b1110_1001_0111_1111_1110_1001_0111_111

+ * - SG (v8M only)

+ * The bulk of the behaviour for this instruction is implemented

+ * in v7m_handle_execute_nsc(), which deals with the insn when

+ * it is executed by a CPU in non-secure state from memory

+ * which is Secure & NonSecure-Callable.

+ * Here we only need to handle the remaining cases:

+ * * in NS memory (including the "security extension not

+ * implemented" case) : NOP

+ * * in S memory but CPU already secure (clear IT bits)

+ * We know that the attribute for the memory this insn is

+ * in must match the current CPU state, because otherwise

+ * get_phys_addr_pmsav8 would have generated an exception.

+ */

+ if (s->v8m_secure) {

+ /* Like the IT insn, we don't need to generate any code */

+ s->condexec_cond = 0;

+ s->condexec_mask = 0;

+ }

+ } else if (insn & 0x01200000) {

/* 0b1110_1000_x11x_xxxx_xxxx_xxxx_xxxx_xxxx

* - load/store dual (post-indexed)

* 0b1111_1001_x10x_xxxx_xxxx_xxxx_xxxx_xxxx

2.7.4

Implement the BLXNS instruction, which allows secure code to

call non-secure code.

Message-id: 1507556919-24992-4-git-send-email-peter.maydell@linaro.org

target/arm/helper.h | 1 +

target/arm/internals.h | 1 +

target/arm/helper.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++

target/arm/translate.c | 17 +++++++++++++--

4 files changed, 76 insertions(+), 2 deletions(-)

diff --git a/target/arm/helper.h b/target/arm/helper.h

--- a/target/arm/helper.h

+++ b/target/arm/helper.h

@@ -XXX,XX +XXX,XX @@ DEF_HELPER_3(v7m_msr, void, env, i32, i32)

DEF_HELPER_2(v7m_mrs, i32, env, i32)

DEF_HELPER_2(v7m_bxns, void, env, i32)

+DEF_HELPER_2(v7m_blxns, void, env, i32)

DEF_HELPER_4(access_check_cp_reg, void, env, ptr, i32, i32)

DEF_HELPER_3(set_cp_reg, void, env, ptr, i32)

diff --git a/target/arm/internals.h b/target/arm/internals.h

index XXXXXXX..XXXXXXX 100644

--- a/target/arm/internals.h

+++ b/target/arm/internals.h

@@ -XXX,XX +XXX,XX @@ static inline bool excp_is_internal(int excp)

FIELD(V7M_CONTROL, NPRIV, 0, 1)

FIELD(V7M_CONTROL, SPSEL, 1, 1)

FIELD(V7M_CONTROL, FPCA, 2, 1)

+FIELD(V7M_CONTROL, SFPA, 3, 1)

/* Bit definitions for v7M exception return payload */

FIELD(V7M_EXCRET, ES, 0, 1)

diff --git a/target/arm/helper.c b/target/arm/helper.c

index XXXXXXX..XXXXXXX 100644

--- a/target/arm/helper.c

+++ b/target/arm/helper.c

@@ -XXX,XX +XXX,XX @@ void HELPER(v7m_bxns)(CPUARMState *env, uint32_t dest)

g_assert_not_reached();

+void HELPER(v7m_blxns)(CPUARMState *env, uint32_t dest)

+ /* translate.c should never generate calls here in user-only mode */

+ g_assert_not_reached();

void switch_mode(CPUARMState *env, int mode)

ARMCPU *cpu = arm_env_get_cpu(env);

@@ -XXX,XX +XXX,XX @@ void HELPER(v7m_bxns)(CPUARMState *env, uint32_t dest)

env->regs[15] = dest & ~1;

+void HELPER(v7m_blxns)(CPUARMState *env, uint32_t dest)

+{

+ /* Handle v7M BLXNS:

+ * - bit 0 of the destination address is the target security state

+ */

+ /* At this point regs[15] is the address just after the BLXNS */

+ uint32_t nextinst = env->regs[15] | 1;

+ uint32_t sp = env->regs[13] - 8;

+ uint32_t saved_psr;

+ /* translate.c will have made BLXNS UNDEF unless we're secure */

+ assert(env->v7m.secure);

+ if (dest & 1) {

+ /* target is Secure, so this is just a normal BLX,

+ * except that the low bit doesn't indicate Thumb/not.

+ */

+ env->regs[14] = nextinst;

+ env->thumb = 1;

+ env->regs[15] = dest & ~1;

+ return;

+

+ /* Target is non-secure: first push a stack frame */

+ if (!QEMU_IS_ALIGNED(sp, 8)) {

+ qemu_log_mask(LOG_GUEST_ERROR,

+ "BLXNS with misaligned SP is UNPREDICTABLE\n");

+

+ saved_psr = env->v7m.exception;

+ if (env->v7m.control[M_REG_S] & R_V7M_CONTROL_SFPA_MASK) {

+ saved_psr |= XPSR_SFPA;

+ }

+

+ /* Note that these stores can throw exceptions on MPU faults */

+ cpu_stl_data(env, sp, nextinst);

+ cpu_stl_data(env, sp + 4, saved_psr);

+

+ env->regs[13] = sp;

+ env->regs[14] = 0xfeffffff;

+ if (arm_v7m_is_handler_mode(env)) {

+ /* Write a dummy value to IPSR, to avoid leaking the current secure

+ * exception number to non-secure code. This is guaranteed not

+ * to cause write_v7m_exception() to actually change stacks.

+ */

+ write_v7m_exception(env, 1);

+ }

+ switch_v7m_security_state(env, 0);

+ env->thumb = 1;

+ env->regs[15] = dest;

+}

+

static uint32_t *get_v7m_sp_ptr(CPUARMState *env, bool secure, bool threadmode,

bool spsel)

{

diff --git a/target/arm/translate.c b/target/arm/translate.c

index XXXXXXX..XXXXXXX 100644

--- a/target/arm/translate.c

+++ b/target/arm/translate.c

@@ -XXX,XX +XXX,XX @@ static inline void gen_bxns(DisasContext *s, int rm)

s->base.is_jmp = DISAS_EXIT;

2.7.4

A few Thumb instructions are always unconditional even inside an

IT block (as opposed to being UNPREDICTABLE if used inside an

IT block): BKPT, the v8M SG instruction, and the A profile

HLT (debug halt) instruction.

This means we need to suppress the jump-over-instruction-on-condfail

code generation (though the IT state still advances as usual and

subsequent insns in the IT block may be conditional).

Message-id: 1507556919-24992-9-git-send-email-peter.maydell@linaro.org

target/arm/translate.c | 48 +++++++++++++++++++++++++++++++++++++++++++++++-

1 file changed, 47 insertions(+), 1 deletion(-)

diff --git a/target/arm/translate.c b/target/arm/translate.c

--- a/target/arm/translate.c

+++ b/target/arm/translate.c

@@ -XXX,XX +XXX,XX @@ static void arm_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)

in init_disas_context by adjusting max_insns. */

+static bool thumb_insn_is_unconditional(DisasContext *s, uint32_t insn)

+ /* Return true if this Thumb insn is always unconditional,

+ * even inside an IT block. This is true of only a very few

+ * instructions: BKPT, HLT, and SG.

+ *

+ * A larger class of instructions are UNPREDICTABLE if used

+ * inside an IT block; we do not need to detect those here, because

+ * what we do by default (perform the cc check and update the IT

+ * bits state machine) is a permitted CONSTRAINED UNPREDICTABLE

+ * choice for those situations.

+ *

+ * insn is either a 16-bit or a 32-bit instruction; the two are

+ * distinguishable because for the 16-bit case the top 16 bits

+ * are zeroes, and that isn't a valid 32-bit encoding.

+ if ((insn & 0xffffff00) == 0xbe00) {

+ /* BKPT */

+ return true;

+ }

+

+ if ((insn & 0xffffffc0) == 0xba80 && arm_dc_feature(s, ARM_FEATURE_V8) &&

+ !arm_dc_feature(s, ARM_FEATURE_M)) {

+ /* HLT: v8A only. This is unconditional even when it is going to

+ * UNDEF; see the v8A ARM ARM DDI0487B.a H3.3.

+ * For v7 cores this was a plain old undefined encoding and so

+ * honours its cc check. (We might be using the encoding as

+ * a semihosting trap, but we don't change the cc check behaviour

+ * on that account, because a debugger connected to a real v7A

+ * core and emulating semihosting traps by catching the UNDEF

+ * exception would also only see cases where the cc check passed.

+ * No guest code should be trying to do a HLT semihosting trap

+ * in an IT block anyway.

+ */

+ return true;

+ }

+

+ if (insn == 0xe97fe97f && arm_dc_feature(s, ARM_FEATURE_V8) &&

+ arm_dc_feature(s, ARM_FEATURE_M)) {

+ /* SG: v8M only */

+ return true;

+ }

+

+ return false;

static void thumb_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)

{

DisasContext *dc = container_of(dcbase, DisasContext, base);

@@ -XXX,XX +XXX,XX @@ static void thumb_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)

dc->pc += 2;

}

- if (dc->condexec_mask) {

+ if (dc->condexec_mask && !thumb_insn_is_unconditional(dc, insn)) {

uint32_t cond = dc->condexec_cond;

if (cond != 0x0e) { /* Skip conditional when condition is AL. */

2.7.4

The code which implements the Thumb1 split BL/BLX instructions

is guarded by a check on "not M or THUMB2". All we really need

to check here is "not THUMB2" (and we assume that elsewhere too,

eg in the ARCH(6T2) test that UNDEFs the Thumb2 insns).

This doesn't change behaviour because all M profile cores

have Thumb2 and so ARM_FEATURE_M implies ARM_FEATURE_THUMB2.

(v6M implements a very restricted subset of Thumb2, but we

can cross that bridge when we get to it with appropriate

feature bits.)

Message-id: 1507556919-24992-6-git-send-email-peter.maydell@linaro.org

target/arm/translate.c | 3 +--

1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/target/arm/translate.c b/target/arm/translate.c

--- a/target/arm/translate.c

+++ b/target/arm/translate.c

@@ -XXX,XX +XXX,XX @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw

int conds;

int logic_cc;

- if (!(arm_dc_feature(s, ARM_FEATURE_THUMB2)

- || arm_dc_feature(s, ARM_FEATURE_M))) {

+ if (!arm_dc_feature(s, ARM_FEATURE_THUMB2)) {

/* Thumb-1 cores may need to treat bl and blx as a pair of

16-bit instructions to get correct prefetch abort behavior. */

insn = insn_hw1;

2.7.4

Refactor the Thumb decode to do the loads of the instruction words at

the top level rather than only loading the second half of a 32-bit

Thumb insn in the middle of the decode.

This is simple apart from the awkward case of Thumb1, where the

BL/BLX prefix and suffix instructions live in what in Thumb2 is the

32-bit insn space. To handle these we decode enough to identify

whether we're looking at a prefix/suffix that we handle as a 16 bit

insn, or a prefix that we're going to merge with the following suffix

to consider as a 32 bit insn. The translation of the 16 bit cases

then moves from disas_thumb2_insn() to disas_thumb_insn().

The refactoring has the benefit that we don't need to pass the

CPUARMState* down into the decoder code any more, but the major