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(-)

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

reason for doing this is that some Thumb instructions must be always

unconditional regardless of the IT state bits, so we need to know the

whole insn before we emit the "skip this insn if the IT bits and cond

state tell us to" code. (The always unconditional insns are BKPT,

HLT and SG; the last of these is 32 bits.)

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

1 file changed, 108 insertions(+), 70 deletions(-)

@@ -XXX,XX +XXX,XX @@ static void disas_arm_insn(DisasContext *s, unsigned int insn)

}

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

+ /* Return true if this is a 16 bit instruction. We must be precise

+ * about this (matching the decode). We assume that s->pc still

+ * points to the first 16 bits of the insn.

+ */

+ if ((insn >> 11) < 0x1d) {

+ /* Definitely a 16-bit instruction */

+ return true;

+ }

+

+ /* Top five bits 0b11101 / 0b11110 / 0b11111 : this is the

+ * first half of a 32-bit Thumb insn. Thumb-1 cores might

+ * end up actually treating this as two 16-bit insns, though,

+ * if it's half of a bl/blx pair that might span a page boundary.

+ */

+ if (arm_dc_feature(s, ARM_FEATURE_THUMB2)) {

+ /* Thumb2 cores (including all M profile ones) always treat

+ * 32-bit insns as 32-bit.

+ */

+ return false;

+ }

+

+ if ((insn >> 11) == 0x1e && (s->pc < s->next_page_start - 3)) {

+ /* 0b1111_0xxx_xxxx_xxxx : BL/BLX prefix, and the suffix

+ * is not on the next page; we merge this into a 32-bit

+ * insn.

+ */

+ return false;

+ }

+ /* 0b1110_1xxx_xxxx_xxxx : BLX suffix (or UNDEF);

+ * 0b1111_1xxx_xxxx_xxxx : BL suffix;

+ * 0b1111_0xxx_xxxx_xxxx : BL/BLX prefix on the end of a page

+ * -- handle as single 16 bit insn

+ */

+ return true;

/* Return true if this is a Thumb-2 logical op. */

static int

thumb2_logic_op(int op)

@@ -XXX,XX +XXX,XX @@ gen_thumb2_data_op(DisasContext *s, int op, int conds, uint32_t shifter_out,

/* Translate a 32-bit thumb instruction. Returns nonzero if the instruction

is not legal. */

-static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw1)

+static int disas_thumb2_insn(DisasContext *s, uint32_t insn)

- uint32_t insn, imm, shift, offset;

+ uint32_t imm, shift, offset;

uint32_t rd, rn, rm, rs;

TCGv_i32 tmp;

TCGv_i32 tmp2;

@@ -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)) {

- /* 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;

- if ((insn & (1 << 12)) == 0) {

- ARCH(5);

- /* Second half of blx. */

- offset = ((insn & 0x7ff) << 1);

- tmp = load_reg(s, 14);

- tcg_gen_addi_i32(tmp, tmp, offset);

- tcg_gen_andi_i32(tmp, tmp, 0xfffffffc);

-

- tmp2 = tcg_temp_new_i32();

- tcg_gen_movi_i32(tmp2, s->pc | 1);

- store_reg(s, 14, tmp2);

- gen_bx(s, tmp);

- return 0;

- }

- if (insn & (1 << 11)) {

- /* Second half of bl. */

- offset = ((insn & 0x7ff) << 1) | 1;

- tmp = load_reg(s, 14);

- tcg_gen_addi_i32(tmp, tmp, offset);

-

- tmp2 = tcg_temp_new_i32();

- tcg_gen_movi_i32(tmp2, s->pc | 1);

- store_reg(s, 14, tmp2);

- gen_bx(s, tmp);

- return 0;

- }

- if ((s->pc & ~TARGET_PAGE_MASK) == 0) {

- /* Instruction spans a page boundary. Implement it as two

- 16-bit instructions in case the second half causes an

- prefetch abort. */

- offset = ((int32_t)insn << 21) >> 9;

- tcg_gen_movi_i32(cpu_R[14], s->pc + 2 + offset);

- return 0;

- }

- /* Fall through to 32-bit decode. */

- }

-

- insn = arm_lduw_code(env, s->pc, s->sctlr_b);

- s->pc += 2;

- insn |= (uint32_t)insn_hw1 << 16;

-

+ /* The only 32 bit insn that's allowed for Thumb1 is the combined

+ * BL/BLX prefix and suffix.

+ */

if ((insn & 0xf800e800) != 0xf000e800) {

ARCH(6T2);

@@ -XXX,XX +XXX,XX @@ illegal_op:

return 1;

-static void disas_thumb_insn(CPUARMState *env, DisasContext *s)

+static void disas_thumb_insn(DisasContext *s, uint32_t insn)

- uint32_t val, insn, op, rm, rn, rd, shift, cond;

+ uint32_t val, op, rm, rn, rd, shift, cond;

int32_t offset;

int i;

TCGv_i32 tmp;

TCGv_i32 tmp2;

TCGv_i32 addr;

- if (s->condexec_mask) {

- cond = s->condexec_cond;

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

- s->condlabel = gen_new_label();

- arm_gen_test_cc(cond ^ 1, s->condlabel);

- s->condjmp = 1;

- }

- }

-

- insn = arm_lduw_code(env, s->pc, s->sctlr_b);

- s->pc += 2;

-

switch (insn >> 12) {

case 0: case 1:

@@ -XXX,XX +XXX,XX @@ static void disas_thumb_insn(CPUARMState *env, DisasContext *s)

case 14:

if (insn & (1 << 11)) {

- if (disas_thumb2_insn(env, s, insn))

- goto undef32;

+ /* thumb_insn_is_16bit() ensures we can't get here for

+ * a Thumb2 CPU, so this must be a thumb1 split BL/BLX:

+ * 0b1110_1xxx_xxxx_xxxx : BLX suffix (or UNDEF)

+ */

+ assert(!arm_dc_feature(s, ARM_FEATURE_THUMB2));

+ ARCH(5);

+ offset = ((insn & 0x7ff) << 1);

+ tmp = load_reg(s, 14);

+ tcg_gen_addi_i32(tmp, tmp, offset);

+ tcg_gen_andi_i32(tmp, tmp, 0xfffffffc);

+

+ tmp2 = tcg_temp_new_i32();

+ tcg_gen_movi_i32(tmp2, s->pc | 1);

+ store_reg(s, 14, tmp2);

+ gen_bx(s, tmp);

break;

}

/* unconditional branch */

@@ -XXX,XX +XXX,XX @@ static void disas_thumb_insn(CPUARMState *env, DisasContext *s)

break;

case 15:

- if (disas_thumb2_insn(env, s, insn))

- goto undef32;

+ /* thumb_insn_is_16bit() ensures we can't get here for

+ * a Thumb2 CPU, so this must be a thumb1 split BL/BLX.

+ */

+ assert(!arm_dc_feature(s, ARM_FEATURE_THUMB2));

+

+ if (insn & (1 << 11)) {

+ /* 0b1111_1xxx_xxxx_xxxx : BL suffix */

+ offset = ((insn & 0x7ff) << 1) | 1;

+ tmp = load_reg(s, 14);

+ tcg_gen_addi_i32(tmp, tmp, offset);

+

+ tmp2 = tcg_temp_new_i32();

+ tcg_gen_movi_i32(tmp2, s->pc | 1);

+ store_reg(s, 14, tmp2);

+ gen_bx(s, tmp);

+ } else {

+ /* 0b1111_0xxx_xxxx_xxxx : BL/BLX prefix */

+ uint32_t uoffset = ((int32_t)insn << 21) >> 9;

+

+ tcg_gen_movi_i32(cpu_R[14], s->pc + 2 + uoffset);

+ }

break;

return;

-undef32:

- gen_exception_insn(s, 4, EXCP_UDEF, syn_uncategorized(),

- default_exception_el(s));

- return;

illegal_op:

undef:

gen_exception_insn(s, 2, EXCP_UDEF, syn_uncategorized(),

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

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

CPUARMState *env = cpu->env_ptr;

+ uint32_t insn;

+ bool is_16bit;

if (arm_pre_translate_insn(dc)) {

return;

- disas_thumb_insn(env, dc);

+ insn = arm_lduw_code(env, dc->pc, dc->sctlr_b);

+ is_16bit = thumb_insn_is_16bit(dc, insn);

+ dc->pc += 2;

+ if (!is_16bit) {

+ uint32_t insn2 = arm_lduw_code(env, dc->pc, dc->sctlr_b);

+

+ insn = insn << 16 | insn2;

+ dc->pc += 2;

+ }

+

+ if (dc->condexec_mask) {

+ uint32_t cond = dc->condexec_cond;

+

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

+ dc->condlabel = gen_new_label();

+ arm_gen_test_cc(cond ^ 1, dc->condlabel);

+ dc->condjmp = 1;

+ }

+ }

+

+ if (is_16bit) {

+ disas_thumb_insn(dc, insn);

+ } else {

+ disas_thumb2_insn(dc, insn);

+ }

/* Advance the Thumb condexec condition. */

if (dc->condexec_mask) {

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.

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

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

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

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

Recent changes have left insn_crosses_page() more complicated

than it needed to be:

* it's only called from thumb_tr_translate_insn() so we know

for certain that we're looking at a Thumb insn

* the caller's check for dc->pc >= dc->next_page_start - 3

means that dc->pc can't possibly be 4 aligned, so there's

no need to check that (the check was partly there to ensure

that we didn't treat an ARM insn as Thumb, I think)

* we now have thumb_insn_is_16bit() which lets us do a precise

check of the length of the next insn, rather than opencoding

an inaccurate check

Simplify it down to just loading the first half of the insn

and calling thumb_insn_is_16bit() on it.

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

1 file changed, 6 insertions(+), 21 deletions(-)

@@ -XXX,XX +XXX,XX @@ static bool insn_crosses_page(CPUARMState *env, DisasContext *s)

/* Return true if the insn at dc->pc might cross a page boundary.

* (False positives are OK, false negatives are not.)

+ * We know this is a Thumb insn, and our caller ensures we are

+ * only called if dc->pc is less than 4 bytes from the page

+ * boundary, so we cross the page if the first 16 bits indicate

+ * that this is a 32 bit insn.

*/

- uint16_t insn;

+ uint16_t insn = arm_lduw_code(env, s->pc, s->sctlr_b);

- if ((s->pc & 3) == 0) {

- /* At a 4-aligned address we can't be crossing a page */

- return false;

- }

-

- /* This must be a Thumb insn */

- insn = arm_lduw_code(env, s->pc, s->sctlr_b);

-

- if ((insn >> 11) >= 0x1d) {

- /* Top five bits 0b11101 / 0b11110 / 0b11111 : this is the

- * First half of a 32-bit Thumb insn. Thumb-1 cores might

- * end up actually treating this as two 16-bit insns (see the

- * code at the start of disas_thumb2_insn()) but we don't bother

- * to check for that as it is unlikely, and false positives here

- * are harmless.

- */

- return true;

- }

- /* Definitely a 16-bit insn, can't be crossing a page. */

- return false;

+ return !thumb_insn_is_16bit(s, insn);

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.)

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

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

@@ -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;