* [PATCH v2 1/8] target/arm: Take an exception if PSTATE.IL is set
2021-08-21 19:59 [PATCH v2 0/8] target/arm: Fix insn exception priorities Richard Henderson
@ 2021-08-21 19:59 ` Richard Henderson
2021-08-21 19:59 ` [PATCH v2 2/8] target/arm: Merge disas_a64_insn into aarch64_tr_translate_insn Richard Henderson
` (7 subsequent siblings)
8 siblings, 0 replies; 23+ messages in thread
From: Richard Henderson @ 2021-08-21 19:59 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Maydell, qemu-arm
From: Peter Maydell <peter.maydell@linaro.org>
In v8A, the PSTATE.IL bit is set for various kinds of illegal
exception return or mode-change attempts. We already set PSTATE.IL
(or its AArch32 equivalent CPSR.IL) in all those cases, but we
weren't implementing the part of the behaviour where attempting to
execute an instruction with PSTATE.IL takes an immediate exception
with an appropriate syndrome value.
Add a new TB flags bit tracking PSTATE.IL/CPSR.IL, and generate code
to take an exception instead of whatever the instruction would have
been.
PSTATE.IL and CPSR.IL change only on exception entry, attempted
exception exit, and various AArch32 mode changes via cpsr_write().
These places generally already rebuild the hflags, so the only place
we need an extra rebuild_hflags call is in the illegal-return
codepath of the AArch64 exception_return helper.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Message-Id: <20210817162118.24319-1-peter.maydell@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
[rth: Added missing returns; set IL bit in syndrome]
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
target/arm/cpu.h | 1 +
target/arm/syndrome.h | 5 +++++
target/arm/translate.h | 2 ++
target/arm/helper-a64.c | 1 +
target/arm/helper.c | 8 ++++++++
target/arm/translate-a64.c | 11 +++++++++++
target/arm/translate.c | 21 +++++++++++++++++++++
7 files changed, 49 insertions(+)
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 9f0a5f84d5..be557bf5d8 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -3441,6 +3441,7 @@ FIELD(TBFLAG_ANY, FPEXC_EL, 8, 2)
FIELD(TBFLAG_ANY, DEBUG_TARGET_EL, 10, 2)
/* Memory operations require alignment: SCTLR_ELx.A or CCR.UNALIGN_TRP */
FIELD(TBFLAG_ANY, ALIGN_MEM, 12, 1)
+FIELD(TBFLAG_ANY, PSTATE__IL, 13, 1)
/*
* Bit usage when in AArch32 state, both A- and M-profile.
diff --git a/target/arm/syndrome.h b/target/arm/syndrome.h
index 39a31260f2..54d135897b 100644
--- a/target/arm/syndrome.h
+++ b/target/arm/syndrome.h
@@ -270,4 +270,9 @@ static inline uint32_t syn_wfx(int cv, int cond, int ti, bool is_16bit)
(cv << 24) | (cond << 20) | ti;
}
+static inline uint32_t syn_illegalstate(void)
+{
+ return (EC_ILLEGALSTATE << ARM_EL_EC_SHIFT) | ARM_EL_IL;
+}
+
#endif /* TARGET_ARM_SYNDROME_H */
diff --git a/target/arm/translate.h b/target/arm/translate.h
index 241596c5bd..af1b6fa03c 100644
--- a/target/arm/translate.h
+++ b/target/arm/translate.h
@@ -98,6 +98,8 @@ typedef struct DisasContext {
bool hstr_active;
/* True if memory operations require alignment */
bool align_mem;
+ /* True if PSTATE.IL is set */
+ bool pstate_il;
/*
* >= 0, a copy of PSTATE.BTYPE, which will be 0 without v8.5-BTI.
* < 0, set by the current instruction.
diff --git a/target/arm/helper-a64.c b/target/arm/helper-a64.c
index 26f79f9141..19445b3c94 100644
--- a/target/arm/helper-a64.c
+++ b/target/arm/helper-a64.c
@@ -1071,6 +1071,7 @@ illegal_return:
if (!arm_singlestep_active(env)) {
env->pstate &= ~PSTATE_SS;
}
+ helper_rebuild_hflags_a64(env, cur_el);
qemu_log_mask(LOG_GUEST_ERROR, "Illegal exception return at EL%d: "
"resuming execution at 0x%" PRIx64 "\n", cur_el, env->pc);
}
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 155d8bf239..201ecf8c67 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -13408,6 +13408,10 @@ static CPUARMTBFlags rebuild_hflags_a32(CPUARMState *env, int fp_el,
DP_TBFLAG_A32(flags, HSTR_ACTIVE, 1);
}
+ if (env->uncached_cpsr & CPSR_IL) {
+ DP_TBFLAG_ANY(flags, PSTATE__IL, 1);
+ }
+
return rebuild_hflags_common_32(env, fp_el, mmu_idx, flags);
}
@@ -13502,6 +13506,10 @@ static CPUARMTBFlags rebuild_hflags_a64(CPUARMState *env, int el, int fp_el,
}
}
+ if (env->pstate & PSTATE_IL) {
+ DP_TBFLAG_ANY(flags, PSTATE__IL, 1);
+ }
+
if (cpu_isar_feature(aa64_mte, env_archcpu(env))) {
/*
* Set MTE_ACTIVE if any access may be Checked, and leave clear
diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index 422e2ac0c9..230cc8d83b 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -14662,6 +14662,16 @@ static void disas_a64_insn(CPUARMState *env, DisasContext *s)
s->fp_access_checked = false;
s->sve_access_checked = false;
+ if (s->pstate_il) {
+ /*
+ * Illegal execution state. This has priority over BTI
+ * exceptions, but comes after instruction abort exceptions.
+ */
+ gen_exception_insn(s, s->pc_curr, EXCP_UDEF,
+ syn_illegalstate(), default_exception_el(s));
+ return;
+ }
+
if (dc_isar_feature(aa64_bti, s)) {
if (s->base.num_insns == 1) {
/*
@@ -14780,6 +14790,7 @@ static void aarch64_tr_init_disas_context(DisasContextBase *dcbase,
#endif
dc->fp_excp_el = EX_TBFLAG_ANY(tb_flags, FPEXC_EL);
dc->align_mem = EX_TBFLAG_ANY(tb_flags, ALIGN_MEM);
+ dc->pstate_il = EX_TBFLAG_ANY(tb_flags, PSTATE__IL);
dc->sve_excp_el = EX_TBFLAG_A64(tb_flags, SVEEXC_EL);
dc->sve_len = (EX_TBFLAG_A64(tb_flags, ZCR_LEN) + 1) * 16;
dc->pauth_active = EX_TBFLAG_A64(tb_flags, PAUTH_ACTIVE);
diff --git a/target/arm/translate.c b/target/arm/translate.c
index 80c282669f..5e0fc8a0a0 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -9045,6 +9045,16 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn)
return;
}
+ if (s->pstate_il) {
+ /*
+ * Illegal execution state. This has priority over BTI
+ * exceptions, but comes after instruction abort exceptions.
+ */
+ gen_exception_insn(s, s->pc_curr, EXCP_UDEF,
+ syn_illegalstate(), default_exception_el(s));
+ return;
+ }
+
if (cond == 0xf) {
/* In ARMv3 and v4 the NV condition is UNPREDICTABLE; we
* choose to UNDEF. In ARMv5 and above the space is used
@@ -9313,6 +9323,7 @@ static void arm_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
#endif
dc->fp_excp_el = EX_TBFLAG_ANY(tb_flags, FPEXC_EL);
dc->align_mem = EX_TBFLAG_ANY(tb_flags, ALIGN_MEM);
+ dc->pstate_il = EX_TBFLAG_ANY(tb_flags, PSTATE__IL);
if (arm_feature(env, ARM_FEATURE_M)) {
dc->vfp_enabled = 1;
@@ -9576,6 +9587,16 @@ static void thumb_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
}
dc->insn = insn;
+ if (dc->pstate_il) {
+ /*
+ * Illegal execution state. This has priority over BTI
+ * exceptions, but comes after instruction abort exceptions.
+ */
+ gen_exception_insn(dc, dc->pc_curr, EXCP_UDEF,
+ syn_illegalstate(), default_exception_el(dc));
+ return;
+ }
+
if (dc->eci) {
/*
* For M-profile continuable instructions, ECI/ICI handling
--
2.25.1
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH v2 2/8] target/arm: Merge disas_a64_insn into aarch64_tr_translate_insn
2021-08-21 19:59 [PATCH v2 0/8] target/arm: Fix insn exception priorities Richard Henderson
2021-08-21 19:59 ` [PATCH v2 1/8] target/arm: Take an exception if PSTATE.IL is set Richard Henderson
@ 2021-08-21 19:59 ` Richard Henderson
2021-08-21 19:59 ` [PATCH v2 3/8] linux-user/aarch64: Handle EC_PCALIGNMENT Richard Henderson
` (6 subsequent siblings)
8 siblings, 0 replies; 23+ messages in thread
From: Richard Henderson @ 2021-08-21 19:59 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Maydell, qemu-arm
It is confusing to have different exits from translation
for various conditions in separate functions.
Merge disas_a64_insn into its only caller. Standardize
on the "s" name for the DisasContext, as the code from
disas_a64_insn had more instances.
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
target/arm/translate-a64.c | 224 ++++++++++++++++++-------------------
1 file changed, 109 insertions(+), 115 deletions(-)
diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index 230cc8d83b..333bc836b2 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -14649,113 +14649,6 @@ static bool btype_destination_ok(uint32_t insn, bool bt, int btype)
return false;
}
-/* C3.1 A64 instruction index by encoding */
-static void disas_a64_insn(CPUARMState *env, DisasContext *s)
-{
- uint32_t insn;
-
- s->pc_curr = s->base.pc_next;
- insn = arm_ldl_code(env, s->base.pc_next, s->sctlr_b);
- s->insn = insn;
- s->base.pc_next += 4;
-
- s->fp_access_checked = false;
- s->sve_access_checked = false;
-
- if (s->pstate_il) {
- /*
- * Illegal execution state. This has priority over BTI
- * exceptions, but comes after instruction abort exceptions.
- */
- gen_exception_insn(s, s->pc_curr, EXCP_UDEF,
- syn_illegalstate(), default_exception_el(s));
- return;
- }
-
- if (dc_isar_feature(aa64_bti, s)) {
- if (s->base.num_insns == 1) {
- /*
- * At the first insn of the TB, compute s->guarded_page.
- * We delayed computing this until successfully reading
- * the first insn of the TB, above. This (mostly) ensures
- * that the softmmu tlb entry has been populated, and the
- * page table GP bit is available.
- *
- * Note that we need to compute this even if btype == 0,
- * because this value is used for BR instructions later
- * where ENV is not available.
- */
- s->guarded_page = is_guarded_page(env, s);
-
- /* First insn can have btype set to non-zero. */
- tcg_debug_assert(s->btype >= 0);
-
- /*
- * Note that the Branch Target Exception has fairly high
- * priority -- below debugging exceptions but above most
- * everything else. This allows us to handle this now
- * instead of waiting until the insn is otherwise decoded.
- */
- if (s->btype != 0
- && s->guarded_page
- && !btype_destination_ok(insn, s->bt, s->btype)) {
- gen_exception_insn(s, s->pc_curr, EXCP_UDEF,
- syn_btitrap(s->btype),
- default_exception_el(s));
- return;
- }
- } else {
- /* Not the first insn: btype must be 0. */
- tcg_debug_assert(s->btype == 0);
- }
- }
-
- switch (extract32(insn, 25, 4)) {
- case 0x0: case 0x1: case 0x3: /* UNALLOCATED */
- unallocated_encoding(s);
- break;
- case 0x2:
- if (!dc_isar_feature(aa64_sve, s) || !disas_sve(s, insn)) {
- unallocated_encoding(s);
- }
- break;
- case 0x8: case 0x9: /* Data processing - immediate */
- disas_data_proc_imm(s, insn);
- break;
- case 0xa: case 0xb: /* Branch, exception generation and system insns */
- disas_b_exc_sys(s, insn);
- break;
- case 0x4:
- case 0x6:
- case 0xc:
- case 0xe: /* Loads and stores */
- disas_ldst(s, insn);
- break;
- case 0x5:
- case 0xd: /* Data processing - register */
- disas_data_proc_reg(s, insn);
- break;
- case 0x7:
- case 0xf: /* Data processing - SIMD and floating point */
- disas_data_proc_simd_fp(s, insn);
- break;
- default:
- assert(FALSE); /* all 15 cases should be handled above */
- break;
- }
-
- /* if we allocated any temporaries, free them here */
- free_tmp_a64(s);
-
- /*
- * After execution of most insns, btype is reset to 0.
- * Note that we set btype == -1 when the insn sets btype.
- */
- if (s->btype > 0 && s->base.is_jmp != DISAS_NORETURN) {
- reset_btype(s);
- }
-}
-
static void aarch64_tr_init_disas_context(DisasContextBase *dcbase,
CPUState *cpu)
{
@@ -14857,10 +14750,11 @@ static void aarch64_tr_insn_start(DisasContextBase *dcbase, CPUState *cpu)
static void aarch64_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
{
- DisasContext *dc = container_of(dcbase, DisasContext, base);
+ DisasContext *s = container_of(dcbase, DisasContext, base);
CPUARMState *env = cpu->env_ptr;
+ uint32_t insn;
- if (dc->ss_active && !dc->pstate_ss) {
+ if (s->ss_active && !s->pstate_ss) {
/* Singlestep state is Active-pending.
* If we're in this state at the start of a TB then either
* a) we just took an exception to an EL which is being debugged
@@ -14871,14 +14765,114 @@ static void aarch64_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
* "did not step an insn" case, and so the syndrome ISV and EX
* bits should be zero.
*/
- assert(dc->base.num_insns == 1);
- gen_swstep_exception(dc, 0, 0);
- dc->base.is_jmp = DISAS_NORETURN;
- } else {
- disas_a64_insn(env, dc);
+ assert(s->base.num_insns == 1);
+ gen_swstep_exception(s, 0, 0);
+ s->base.is_jmp = DISAS_NORETURN;
+ return;
}
- translator_loop_temp_check(&dc->base);
+ s->pc_curr = s->base.pc_next;
+ insn = arm_ldl_code(env, s->base.pc_next, s->sctlr_b);
+ s->insn = insn;
+ s->base.pc_next += 4;
+
+ s->fp_access_checked = false;
+ s->sve_access_checked = false;
+
+ if (s->pstate_il) {
+ /*
+ * Illegal execution state. This has priority over BTI
+ * exceptions, but comes after instruction abort exceptions.
+ */
+ gen_exception_insn(s, s->pc_curr, EXCP_UDEF,
+ syn_illegalstate(), default_exception_el(s));
+ return;
+ }
+
+ if (dc_isar_feature(aa64_bti, s)) {
+ if (s->base.num_insns == 1) {
+ /*
+ * At the first insn of the TB, compute s->guarded_page.
+ * We delayed computing this until successfully reading
+ * the first insn of the TB, above. This (mostly) ensures
+ * that the softmmu tlb entry has been populated, and the
+ * page table GP bit is available.
+ *
+ * Note that we need to compute this even if btype == 0,
+ * because this value is used for BR instructions later
+ * where ENV is not available.
+ */
+ s->guarded_page = is_guarded_page(env, s);
+
+ /* First insn can have btype set to non-zero. */
+ tcg_debug_assert(s->btype >= 0);
+
+ /*
+ * Note that the Branch Target Exception has fairly high
+ * priority -- below debugging exceptions but above most
+ * everything else. This allows us to handle this now
+ * instead of waiting until the insn is otherwise decoded.
+ */
+ if (s->btype != 0
+ && s->guarded_page
+ && !btype_destination_ok(insn, s->bt, s->btype)) {
+ gen_exception_insn(s, s->pc_curr, EXCP_UDEF,
+ syn_btitrap(s->btype),
+ default_exception_el(s));
+ return;
+ }
+ } else {
+ /* Not the first insn: btype must be 0. */
+ tcg_debug_assert(s->btype == 0);
+ }
+ }
+
+ switch (extract32(insn, 25, 4)) {
+ case 0x0: case 0x1: case 0x3: /* UNALLOCATED */
+ unallocated_encoding(s);
+ break;
+ case 0x2:
+ if (!dc_isar_feature(aa64_sve, s) || !disas_sve(s, insn)) {
+ unallocated_encoding(s);
+ }
+ break;
+ case 0x8: case 0x9: /* Data processing - immediate */
+ disas_data_proc_imm(s, insn);
+ break;
+ case 0xa: case 0xb: /* Branch, exception generation and system insns */
+ disas_b_exc_sys(s, insn);
+ break;
+ case 0x4:
+ case 0x6:
+ case 0xc:
+ case 0xe: /* Loads and stores */
+ disas_ldst(s, insn);
+ break;
+ case 0x5:
+ case 0xd: /* Data processing - register */
+ disas_data_proc_reg(s, insn);
+ break;
+ case 0x7:
+ case 0xf: /* Data processing - SIMD and floating point */
+ disas_data_proc_simd_fp(s, insn);
+ break;
+ default:
+ assert(FALSE); /* all 15 cases should be handled above */
+ break;
+ }
+
+ /* if we allocated any temporaries, free them here */
+ free_tmp_a64(s);
+
+ /*
+ * After execution of most insns, btype is reset to 0.
+ * Note that we set btype == -1 when the insn sets btype.
+ */
+ if (s->btype > 0 && s->base.is_jmp != DISAS_NORETURN) {
+ reset_btype(s);
+ }
+
+ translator_loop_temp_check(&s->base);
}
static void aarch64_tr_tb_stop(DisasContextBase *dcbase, CPUState *cpu)
--
2.25.1
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH v2 3/8] linux-user/aarch64: Handle EC_PCALIGNMENT
2021-08-21 19:59 [PATCH v2 0/8] target/arm: Fix insn exception priorities Richard Henderson
2021-08-21 19:59 ` [PATCH v2 1/8] target/arm: Take an exception if PSTATE.IL is set Richard Henderson
2021-08-21 19:59 ` [PATCH v2 2/8] target/arm: Merge disas_a64_insn into aarch64_tr_translate_insn Richard Henderson
@ 2021-08-21 19:59 ` Richard Henderson
2021-08-26 13:27 ` Peter Maydell
2021-08-21 19:59 ` [PATCH v2 4/8] linux-user/arm: Report SIGBUS and SIGSEGV correctly Richard Henderson
` (5 subsequent siblings)
8 siblings, 1 reply; 23+ messages in thread
From: Richard Henderson @ 2021-08-21 19:59 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-arm
This will shortly be raised for execution with a misaligned pc.
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
linux-user/aarch64/cpu_loop.c | 44 +++++++++++++++++++++--------------
1 file changed, 27 insertions(+), 17 deletions(-)
diff --git a/linux-user/aarch64/cpu_loop.c b/linux-user/aarch64/cpu_loop.c
index 11e34cb100..6e03afb2bd 100644
--- a/linux-user/aarch64/cpu_loop.c
+++ b/linux-user/aarch64/cpu_loop.c
@@ -78,7 +78,7 @@
void cpu_loop(CPUARMState *env)
{
CPUState *cs = env_cpu(env);
- int trapnr, ec, fsc, si_code;
+ int trapnr, ec, fsc, si_sig, si_code;
abi_long ret;
for (;;) {
@@ -112,28 +112,38 @@ void cpu_loop(CPUARMState *env)
break;
case EXCP_PREFETCH_ABORT:
case EXCP_DATA_ABORT:
- /* We should only arrive here with EC in {DATAABORT, INSNABORT}. */
ec = syn_get_ec(env->exception.syndrome);
- assert(ec == EC_DATAABORT || ec == EC_INSNABORT);
-
- /* Both EC have the same format for FSC, or close enough. */
- fsc = extract32(env->exception.syndrome, 0, 6);
- switch (fsc) {
- case 0x04 ... 0x07: /* Translation fault, level {0-3} */
- si_code = TARGET_SEGV_MAPERR;
+ switch (ec) {
+ case EC_DATAABORT:
+ case EC_INSNABORT:
+ /* Both EC have the same format for FSC, or close enough. */
+ fsc = extract32(env->exception.syndrome, 0, 6);
+ switch (fsc) {
+ case 0x04 ... 0x07: /* Translation fault, level {0-3} */
+ si_sig = TARGET_SIGSEGV;
+ si_code = TARGET_SEGV_MAPERR;
+ break;
+ case 0x09 ... 0x0b: /* Access flag fault, level {1-3} */
+ case 0x0d ... 0x0f: /* Permission fault, level {1-3} */
+ si_sig = TARGET_SIGSEGV;
+ si_code = TARGET_SEGV_ACCERR;
+ break;
+ case 0x11: /* Synchronous Tag Check Fault */
+ si_sig = TARGET_SIGSEGV;
+ si_code = TARGET_SEGV_MTESERR;
+ break;
+ default:
+ g_assert_not_reached();
+ }
break;
- case 0x09 ... 0x0b: /* Access flag fault, level {1-3} */
- case 0x0d ... 0x0f: /* Permission fault, level {1-3} */
- si_code = TARGET_SEGV_ACCERR;
- break;
- case 0x11: /* Synchronous Tag Check Fault */
- si_code = TARGET_SEGV_MTESERR;
+ case EC_PCALIGNMENT:
+ si_sig = TARGET_SIGBUS;
+ si_code = TARGET_BUS_ADRALN;
break;
default:
g_assert_not_reached();
}
-
- force_sig_fault(TARGET_SIGSEGV, si_code, env->exception.vaddress);
+ force_sig_fault(si_sig, si_code, env->exception.vaddress);
break;
case EXCP_DEBUG:
case EXCP_BKPT:
--
2.25.1
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v2 3/8] linux-user/aarch64: Handle EC_PCALIGNMENT
2021-08-21 19:59 ` [PATCH v2 3/8] linux-user/aarch64: Handle EC_PCALIGNMENT Richard Henderson
@ 2021-08-26 13:27 ` Peter Maydell
0 siblings, 0 replies; 23+ messages in thread
From: Peter Maydell @ 2021-08-26 13:27 UTC (permalink / raw)
To: Richard Henderson; +Cc: qemu-arm, QEMU Developers
On Sat, 21 Aug 2021 at 21:03, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> This will shortly be raised for execution with a misaligned pc.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> linux-user/aarch64/cpu_loop.c | 44 +++++++++++++++++++++--------------
> 1 file changed, 27 insertions(+), 17 deletions(-)
I wasn't expecting PC misalignment to set the FAR (after all you can
get the bogus PC out of the exception link register), but conveniently
for us it does.
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
thanks
-- PMM
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 4/8] linux-user/arm: Report SIGBUS and SIGSEGV correctly
2021-08-21 19:59 [PATCH v2 0/8] target/arm: Fix insn exception priorities Richard Henderson
` (2 preceding siblings ...)
2021-08-21 19:59 ` [PATCH v2 3/8] linux-user/aarch64: Handle EC_PCALIGNMENT Richard Henderson
@ 2021-08-21 19:59 ` Richard Henderson
2021-08-26 13:31 ` Peter Maydell
2021-08-21 19:59 ` [PATCH v2 5/8] target/arm: Take an exception if PC is misaligned Richard Henderson
` (4 subsequent siblings)
8 siblings, 1 reply; 23+ messages in thread
From: Richard Henderson @ 2021-08-21 19:59 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-arm
Pull the fault information from where we placed it, in
arm_cpu_tlb_fill and arm_cpu_do_unaligned_access.
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
Pulled out from the larger unaligned data patch set.
For short-form FSC, pc misalignment is reported in the same way.
---
linux-user/arm/cpu_loop.c | 39 ++++++++++++++++++++++++++++++++++-----
1 file changed, 34 insertions(+), 5 deletions(-)
diff --git a/linux-user/arm/cpu_loop.c b/linux-user/arm/cpu_loop.c
index d4b4f0c71f..5731d3c937 100644
--- a/linux-user/arm/cpu_loop.c
+++ b/linux-user/arm/cpu_loop.c
@@ -24,6 +24,7 @@
#include "cpu_loop-common.h"
#include "signal-common.h"
#include "semihosting/common-semi.h"
+#include "target/arm/syndrome.h"
#define get_user_code_u32(x, gaddr, env) \
({ abi_long __r = get_user_u32((x), (gaddr)); \
@@ -279,8 +280,8 @@ static bool emulate_arm_fpa11(CPUARMState *env, uint32_t opcode)
void cpu_loop(CPUARMState *env)
{
CPUState *cs = env_cpu(env);
- int trapnr;
- unsigned int n, insn;
+ int trapnr, si_signo, si_code;
+ unsigned int n, insn, ec, fsc;
abi_ulong ret;
for(;;) {
@@ -422,9 +423,37 @@ void cpu_loop(CPUARMState *env)
break;
case EXCP_PREFETCH_ABORT:
case EXCP_DATA_ABORT:
- /* XXX: check env->error_code */
- force_sig_fault(TARGET_SIGSEGV, TARGET_SEGV_MAPERR,
- env->exception.vaddress);
+ /*
+ * For user-only we don't set TTBCR_EAE, so we always get
+ * short-form FSC, which then tells us to look at the FSR.
+ */
+ ec = syn_get_ec(env->exception.syndrome);
+ assert(ec == EC_DATAABORT || ec == EC_INSNABORT);
+ fsc = extract32(env->exception.syndrome, 0, 6);
+ assert(fsc == 0x3f);
+ switch (env->exception.fsr & 0x1f) {
+ case 0x1: /* Alignment */
+ si_signo = TARGET_SIGBUS;
+ si_code = TARGET_BUS_ADRALN;
+ break;
+ case 0x3: /* Access flag fault, level 1 */
+ case 0x6: /* Access flag fault, level 2 */
+ case 0x9: /* Domain fault, level 1 */
+ case 0xb: /* Domain fault, level 2 */
+ case 0xd: /* Permision fault, level 1 */
+ case 0xf: /* Permision fault, level 2 */
+ si_signo = TARGET_SIGSEGV;
+ si_code = TARGET_SEGV_ACCERR;
+ break;
+ case 0x5: /* Translation fault, level 1 */
+ case 0x7: /* Translation fault, level 2 */
+ si_signo = TARGET_SIGSEGV;
+ si_code = TARGET_SEGV_MAPERR;
+ break;
+ default:
+ g_assert_not_reached();
+ }
+ force_sig_fault(si_signo, si_code, env->exception.vaddress);
break;
case EXCP_DEBUG:
case EXCP_BKPT:
--
2.25.1
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v2 4/8] linux-user/arm: Report SIGBUS and SIGSEGV correctly
2021-08-21 19:59 ` [PATCH v2 4/8] linux-user/arm: Report SIGBUS and SIGSEGV correctly Richard Henderson
@ 2021-08-26 13:31 ` Peter Maydell
2021-09-08 9:19 ` Richard Henderson
2021-09-19 22:23 ` Richard Henderson
0 siblings, 2 replies; 23+ messages in thread
From: Peter Maydell @ 2021-08-26 13:31 UTC (permalink / raw)
To: Richard Henderson; +Cc: qemu-arm, QEMU Developers
On Sat, 21 Aug 2021 at 21:03, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Pull the fault information from where we placed it, in
> arm_cpu_tlb_fill and arm_cpu_do_unaligned_access.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> Pulled out from the larger unaligned data patch set.
> For short-form FSC, pc misalignment is reported in the same way.
> ---
> linux-user/arm/cpu_loop.c | 39 ++++++++++++++++++++++++++++++++++-----
> 1 file changed, 34 insertions(+), 5 deletions(-)
>
> diff --git a/linux-user/arm/cpu_loop.c b/linux-user/arm/cpu_loop.c
> + case 0x3: /* Access flag fault, level 1 */
> + case 0x6: /* Access flag fault, level 2 */
> + case 0x9: /* Domain fault, level 1 */
> + case 0xb: /* Domain fault, level 2 */
> + case 0xd: /* Permision fault, level 1 */
> + case 0xf: /* Permision fault, level 2 */
"Permission"
> + si_signo = TARGET_SIGSEGV;
> + si_code = TARGET_SEGV_ACCERR;
> + break;
> + case 0x5: /* Translation fault, level 1 */
> + case 0x7: /* Translation fault, level 2 */
> + si_signo = TARGET_SIGSEGV;
> + si_code = TARGET_SEGV_MAPERR;
> + break;
Side note: for cases like this where we can tell MAPERR from
ACCERR based on info the exception handler passes to us, should
we prefer that or the "check the page flags" approach that
force_sigsegv_for_addr() takes ? I feel like the former is
nicer, because in a multithreaded program some other thread
might have changed whether the page is mapped between our taking
the fault and getting here. But maybe that's always racy...
Anyway, other than the typo,
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
thanks
-- PMM
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 4/8] linux-user/arm: Report SIGBUS and SIGSEGV correctly
2021-08-26 13:31 ` Peter Maydell
@ 2021-09-08 9:19 ` Richard Henderson
2021-09-19 22:23 ` Richard Henderson
1 sibling, 0 replies; 23+ messages in thread
From: Richard Henderson @ 2021-09-08 9:19 UTC (permalink / raw)
To: Peter Maydell; +Cc: qemu-arm, QEMU Developers
On 8/26/21 3:31 PM, Peter Maydell wrote:
> Side note: for cases like this where we can tell MAPERR from
> ACCERR based on info the exception handler passes to us, should
> we prefer that or the "check the page flags" approach that
> force_sigsegv_for_addr() takes ? I feel like the former is
> nicer, because in a multithreaded program some other thread
> might have changed whether the page is mapped between our taking
> the fault and getting here. But maybe that's always racy...
Both ways are racy.
After having played with SIGBUS, what I believe should happen is that we clean up the
signal handling such that we can pass through the host MAPERR/ACCERR, remapping any fault
address, after filtering the write-protect case that we care about.
I'm not sure how much effort it would be to do that. Certainly the test matrix is pretty
darn large. But perhaps it would simplify the huge SIGBUS patch set, and thus make it all
worthwhile.
r~
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 4/8] linux-user/arm: Report SIGBUS and SIGSEGV correctly
2021-08-26 13:31 ` Peter Maydell
2021-09-08 9:19 ` Richard Henderson
@ 2021-09-19 22:23 ` Richard Henderson
1 sibling, 0 replies; 23+ messages in thread
From: Richard Henderson @ 2021-09-19 22:23 UTC (permalink / raw)
To: Peter Maydell; +Cc: qemu-arm, QEMU Developers
On 8/26/21 6:31 AM, Peter Maydell wrote:
>> + si_signo = TARGET_SIGSEGV;
>> + si_code = TARGET_SEGV_ACCERR;
>> + break;
>> + case 0x5: /* Translation fault, level 1 */
>> + case 0x7: /* Translation fault, level 2 */
>> + si_signo = TARGET_SIGSEGV;
>> + si_code = TARGET_SEGV_MAPERR;
>> + break;
>
> Side note: for cases like this where we can tell MAPERR from
> ACCERR based on info the exception handler passes to us, should
> we prefer that or the "check the page flags" approach that
> force_sigsegv_for_addr() takes ?
FYI, the v3 version of the sigsegv+siginfo patch set makes is vastly easier on the target
code. For the most part the target code goes away entirely. For the specific case of Arm
(both a32 and a64), we retain it because we are supposed to report the ESR and FAR as part
of the signal frame.
I'll note that a64 isn't filling in the esr_context and far_context structures. The
latter was invented for MTE, I believe, where the normal si_addr is untagged. I should
have a double-check around those at some point...
r~
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 5/8] target/arm: Take an exception if PC is misaligned
2021-08-21 19:59 [PATCH v2 0/8] target/arm: Fix insn exception priorities Richard Henderson
` (3 preceding siblings ...)
2021-08-21 19:59 ` [PATCH v2 4/8] linux-user/arm: Report SIGBUS and SIGSEGV correctly Richard Henderson
@ 2021-08-21 19:59 ` Richard Henderson
2021-08-26 13:45 ` Peter Maydell
2021-08-21 19:59 ` [PATCH v2 6/8] target/arm: Assert thumb pc is aligned Richard Henderson
` (3 subsequent siblings)
8 siblings, 1 reply; 23+ messages in thread
From: Richard Henderson @ 2021-08-21 19:59 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-arm
For A64, any input to an indirect branch can cause this.
For A32, many indirect branch paths force the branch to be aligned,
but BXWritePC does not. This includes the BX instruction but also
other interworking changes to PC. Prior to v8, this case is UNDEFINED.
With v8, this is CONSTRAINED UNPREDICTABLE and may either raise an
exception or force align the PC.
We choose to raise an exception because we have the infrastructure,
it makes the generated code for gen_bx simpler, and it has the
possibility of catching more guest bugs.
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
target/arm/helper.h | 1 +
target/arm/syndrome.h | 5 +++++
target/arm/tlb_helper.c | 24 +++++++++++++++++++++++
target/arm/translate-a64.c | 21 ++++++++++++++++++--
target/arm/translate.c | 39 +++++++++++++++++++++++++++++++-------
5 files changed, 81 insertions(+), 9 deletions(-)
diff --git a/target/arm/helper.h b/target/arm/helper.h
index 248569b0cd..d629ee6859 100644
--- a/target/arm/helper.h
+++ b/target/arm/helper.h
@@ -47,6 +47,7 @@ DEF_HELPER_FLAGS_3(sel_flags, TCG_CALL_NO_RWG_SE,
DEF_HELPER_2(exception_internal, void, env, i32)
DEF_HELPER_4(exception_with_syndrome, void, env, i32, i32, i32)
DEF_HELPER_2(exception_bkpt_insn, void, env, i32)
+DEF_HELPER_2(exception_pc_alignment, noreturn, env, tl)
DEF_HELPER_1(setend, void, env)
DEF_HELPER_2(wfi, void, env, i32)
DEF_HELPER_1(wfe, void, env)
diff --git a/target/arm/syndrome.h b/target/arm/syndrome.h
index 54d135897b..e9d97fac6e 100644
--- a/target/arm/syndrome.h
+++ b/target/arm/syndrome.h
@@ -275,4 +275,9 @@ static inline uint32_t syn_illegalstate(void)
return (EC_ILLEGALSTATE << ARM_EL_EC_SHIFT) | ARM_EL_IL;
}
+static inline uint32_t syn_pcalignment(void)
+{
+ return (EC_PCALIGNMENT << ARM_EL_EC_SHIFT) | ARM_EL_IL;
+}
+
#endif /* TARGET_ARM_SYNDROME_H */
diff --git a/target/arm/tlb_helper.c b/target/arm/tlb_helper.c
index 3107f9823e..25c422976e 100644
--- a/target/arm/tlb_helper.c
+++ b/target/arm/tlb_helper.c
@@ -9,6 +9,7 @@
#include "cpu.h"
#include "internals.h"
#include "exec/exec-all.h"
+#include "exec/helper-proto.h"
static inline uint32_t merge_syn_data_abort(uint32_t template_syn,
unsigned int target_el,
@@ -123,6 +124,29 @@ void arm_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr,
arm_deliver_fault(cpu, vaddr, access_type, mmu_idx, &fi);
}
+void helper_exception_pc_alignment(CPUARMState *env, target_ulong pc)
+{
+ int target_el = exception_target_el(env);
+
+ if (target_el == 2 || arm_el_is_aa64(env, target_el)) {
+ /*
+ * To aarch64 and aarch32 el2, pc alignment has a
+ * special exception class.
+ */
+ env->exception.vaddress = pc;
+ env->exception.fsr = 0;
+ raise_exception(env, EXCP_PREFETCH_ABORT, syn_pcalignment(), target_el);
+ } else {
+ /*
+ * To aarch32 el1, pc alignment is like data alignment
+ * except with a prefetch abort.
+ */
+ ARMMMUFaultInfo fi = { .type = ARMFault_Alignment };
+ arm_deliver_fault(env_archcpu(env), pc, MMU_INST_FETCH,
+ cpu_mmu_index(env, true), &fi);
+ }
+}
+
#if !defined(CONFIG_USER_ONLY)
/*
diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index 333bc836b2..39c2fb8c7e 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -14752,8 +14752,10 @@ static void aarch64_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
{
DisasContext *s = container_of(dcbase, DisasContext, base);
CPUARMState *env = cpu->env_ptr;
+ uint64_t pc = s->base.pc_next;
uint32_t insn;
+ /* Singlestep exceptions have the highest priority. */
if (s->ss_active && !s->pstate_ss) {
/* Singlestep state is Active-pending.
* If we're in this state at the start of a TB then either
@@ -14768,13 +14770,28 @@ static void aarch64_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
assert(s->base.num_insns == 1);
gen_swstep_exception(s, 0, 0);
s->base.is_jmp = DISAS_NORETURN;
+ s->base.pc_next = pc + 4;
return;
}
- s->pc_curr = s->base.pc_next;
+ if (pc & 3) {
+ /*
+ * PC alignment fault. This has priority over the instruction abort
+ * that we would receive from a translation fault via arm_ldl_code.
+ * This should only be possible after an indirect branch, at the
+ * start of the TB.
+ */
+ assert(s->base.num_insns == 1);
+ gen_helper_exception_pc_alignment(cpu_env, tcg_constant_tl(pc));
+ s->base.is_jmp = DISAS_NORETURN;
+ s->base.pc_next = QEMU_ALIGN_UP(pc, 4);
+ return;
+ }
+
+ s->pc_curr = pc;
insn = arm_ldl_code(env, s->base.pc_next, s->sctlr_b);
s->insn = insn;
- s->base.pc_next += 4;
+ s->base.pc_next = pc + 4;
s->fp_access_checked = false;
s->sve_access_checked = false;
diff --git a/target/arm/translate.c b/target/arm/translate.c
index 5e0fc8a0a0..dfeaa2321d 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -9452,7 +9452,7 @@ static void arm_tr_insn_start(DisasContextBase *dcbase, CPUState *cpu)
dc->insn_start = tcg_last_op();
}
-static bool arm_pre_translate_insn(DisasContext *dc)
+static bool arm_check_kernelpage(DisasContext *dc)
{
#ifdef CONFIG_USER_ONLY
/* Intercept jump to the magic kernel page. */
@@ -9464,7 +9464,11 @@ static bool arm_pre_translate_insn(DisasContext *dc)
return true;
}
#endif
+ return false;
+}
+static bool arm_check_ss_active(DisasContext *dc)
+{
if (dc->ss_active && !dc->pstate_ss) {
/* Singlestep state is Active-pending.
* If we're in this state at the start of a TB then either
@@ -9498,17 +9502,38 @@ static void arm_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
{
DisasContext *dc = container_of(dcbase, DisasContext, base);
CPUARMState *env = cpu->env_ptr;
+ uint32_t pc = dc->base.pc_next;
unsigned int insn;
- if (arm_pre_translate_insn(dc)) {
- dc->base.pc_next += 4;
+ /* Singlestep exceptions have the highest priority. */
+ if (arm_check_ss_active(dc)) {
+ dc->base.pc_next = pc + 4;
return;
}
- dc->pc_curr = dc->base.pc_next;
- insn = arm_ldl_code(env, dc->base.pc_next, dc->sctlr_b);
+ if (pc & 3) {
+ /*
+ * PC alignment fault. This has priority over the instruction abort
+ * that we would receive from a translation fault via arm_ldl_code
+ * (or the execution of the kernelpage entrypoint). This should only
+ * be possible after an indirect branch, at the start of the TB.
+ */
+ assert(dc->base.num_insns == 1);
+ gen_helper_exception_pc_alignment(cpu_env, tcg_constant_tl(pc));
+ dc->base.is_jmp = DISAS_NORETURN;
+ dc->base.pc_next = QEMU_ALIGN_UP(pc, 4);
+ return;
+ }
+
+ if (arm_check_kernelpage(dc)) {
+ dc->base.pc_next = pc + 4;
+ return;
+ }
+
+ dc->pc_curr = pc;
+ insn = arm_ldl_code(env, pc, dc->sctlr_b);
dc->insn = insn;
- dc->base.pc_next += 4;
+ dc->base.pc_next = pc + 4;
disas_arm_insn(dc, insn);
arm_post_translate_insn(dc);
@@ -9570,7 +9595,7 @@ static void thumb_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
uint32_t insn;
bool is_16bit;
- if (arm_pre_translate_insn(dc)) {
+ if (arm_check_ss_active(dc) || arm_check_kernelpage(dc)) {
dc->base.pc_next += 2;
return;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v2 5/8] target/arm: Take an exception if PC is misaligned
2021-08-21 19:59 ` [PATCH v2 5/8] target/arm: Take an exception if PC is misaligned Richard Henderson
@ 2021-08-26 13:45 ` Peter Maydell
2021-09-20 1:29 ` Richard Henderson
0 siblings, 1 reply; 23+ messages in thread
From: Peter Maydell @ 2021-08-26 13:45 UTC (permalink / raw)
To: Richard Henderson; +Cc: qemu-arm, QEMU Developers
On Sat, 21 Aug 2021 at 21:00, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> For A64, any input to an indirect branch can cause this.
>
> For A32, many indirect branch paths force the branch to be aligned,
> but BXWritePC does not. This includes the BX instruction but also
> other interworking changes to PC. Prior to v8, this case is UNDEFINED.
> With v8, this is CONSTRAINED UNPREDICTABLE and may either raise an
> exception or force align the PC.
>
> We choose to raise an exception because we have the infrastructure,
> it makes the generated code for gen_bx simpler, and it has the
> possibility of catching more guest bugs.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> target/arm/helper.h | 1 +
> target/arm/syndrome.h | 5 +++++
> target/arm/tlb_helper.c | 24 +++++++++++++++++++++++
> target/arm/translate-a64.c | 21 ++++++++++++++++++--
> target/arm/translate.c | 39 +++++++++++++++++++++++++++++++-------
> 5 files changed, 81 insertions(+), 9 deletions(-)
>
> diff --git a/target/arm/helper.h b/target/arm/helper.h
> index 248569b0cd..d629ee6859 100644
> --- a/target/arm/helper.h
> +++ b/target/arm/helper.h
> @@ -47,6 +47,7 @@ DEF_HELPER_FLAGS_3(sel_flags, TCG_CALL_NO_RWG_SE,
> DEF_HELPER_2(exception_internal, void, env, i32)
> DEF_HELPER_4(exception_with_syndrome, void, env, i32, i32, i32)
> DEF_HELPER_2(exception_bkpt_insn, void, env, i32)
> +DEF_HELPER_2(exception_pc_alignment, noreturn, env, tl)
> DEF_HELPER_1(setend, void, env)
> DEF_HELPER_2(wfi, void, env, i32)
> DEF_HELPER_1(wfe, void, env)
> diff --git a/target/arm/syndrome.h b/target/arm/syndrome.h
> index 54d135897b..e9d97fac6e 100644
> --- a/target/arm/syndrome.h
> +++ b/target/arm/syndrome.h
> @@ -275,4 +275,9 @@ static inline uint32_t syn_illegalstate(void)
> return (EC_ILLEGALSTATE << ARM_EL_EC_SHIFT) | ARM_EL_IL;
> }
>
> +static inline uint32_t syn_pcalignment(void)
> +{
> + return (EC_PCALIGNMENT << ARM_EL_EC_SHIFT) | ARM_EL_IL;
> +}
> +
> #endif /* TARGET_ARM_SYNDROME_H */
> diff --git a/target/arm/tlb_helper.c b/target/arm/tlb_helper.c
> index 3107f9823e..25c422976e 100644
> --- a/target/arm/tlb_helper.c
> +++ b/target/arm/tlb_helper.c
> @@ -9,6 +9,7 @@
> #include "cpu.h"
> #include "internals.h"
> #include "exec/exec-all.h"
> +#include "exec/helper-proto.h"
>
> static inline uint32_t merge_syn_data_abort(uint32_t template_syn,
> unsigned int target_el,
> @@ -123,6 +124,29 @@ void arm_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr,
> arm_deliver_fault(cpu, vaddr, access_type, mmu_idx, &fi);
> }
>
> +void helper_exception_pc_alignment(CPUARMState *env, target_ulong pc)
> +{
> + int target_el = exception_target_el(env);
> +
> + if (target_el == 2 || arm_el_is_aa64(env, target_el)) {
> + /*
> + * To aarch64 and aarch32 el2, pc alignment has a
> + * special exception class.
> + */
> + env->exception.vaddress = pc;
> + env->exception.fsr = 0;
> + raise_exception(env, EXCP_PREFETCH_ABORT, syn_pcalignment(), target_el);
> + } else {
> + /*
> + * To aarch32 el1, pc alignment is like data alignment
> + * except with a prefetch abort.
> + */
> + ARMMMUFaultInfo fi = { .type = ARMFault_Alignment };
> + arm_deliver_fault(env_archcpu(env), pc, MMU_INST_FETCH,
> + cpu_mmu_index(env, true), &fi);
I don't think you should need to special case AArch64 vs AArch32 like this;
you can do
env->exception.vaddress = pc;
env->exception.fsr = the_fsr;
raise_exception(env, EXCP_PREFETCH_ABORT, syn_pcalignment(), target_el);
for both. AArch64/AArch32-Hyp exception entry will ignore exception.fsr,
and AArch32-not-Hyp entry will ignore exception.syndrome.
We could probably do with factoring out the
"if (something) { fsr = arm_fi_to_lfsc(&fi) } else { fsr =
arm_fi_to_sfsc(&fi); }"
logic which we currently duplicate in arm_deliver_fault() and
do_ats_write() and arm_debug_exception_fsr() and also want here.
(NB I haven't checked these really are doing exactly the same
condition check...)
-- PMM
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v2 5/8] target/arm: Take an exception if PC is misaligned
2021-08-26 13:45 ` Peter Maydell
@ 2021-09-20 1:29 ` Richard Henderson
2021-09-20 8:08 ` Peter Maydell
0 siblings, 1 reply; 23+ messages in thread
From: Richard Henderson @ 2021-09-20 1:29 UTC (permalink / raw)
To: Peter Maydell; +Cc: qemu-arm, QEMU Developers
On 8/26/21 6:45 AM, Peter Maydell wrote:
> On Sat, 21 Aug 2021 at 21:00, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> For A64, any input to an indirect branch can cause this.
>>
>> For A32, many indirect branch paths force the branch to be aligned,
>> but BXWritePC does not. This includes the BX instruction but also
>> other interworking changes to PC. Prior to v8, this case is UNDEFINED.
>> With v8, this is CONSTRAINED UNPREDICTABLE and may either raise an
>> exception or force align the PC.
>>
>> We choose to raise an exception because we have the infrastructure,
>> it makes the generated code for gen_bx simpler, and it has the
>> possibility of catching more guest bugs.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>> target/arm/helper.h | 1 +
>> target/arm/syndrome.h | 5 +++++
>> target/arm/tlb_helper.c | 24 +++++++++++++++++++++++
>> target/arm/translate-a64.c | 21 ++++++++++++++++++--
>> target/arm/translate.c | 39 +++++++++++++++++++++++++++++++-------
>> 5 files changed, 81 insertions(+), 9 deletions(-)
>>
>> diff --git a/target/arm/helper.h b/target/arm/helper.h
>> index 248569b0cd..d629ee6859 100644
>> --- a/target/arm/helper.h
>> +++ b/target/arm/helper.h
>> @@ -47,6 +47,7 @@ DEF_HELPER_FLAGS_3(sel_flags, TCG_CALL_NO_RWG_SE,
>> DEF_HELPER_2(exception_internal, void, env, i32)
>> DEF_HELPER_4(exception_with_syndrome, void, env, i32, i32, i32)
>> DEF_HELPER_2(exception_bkpt_insn, void, env, i32)
>> +DEF_HELPER_2(exception_pc_alignment, noreturn, env, tl)
>> DEF_HELPER_1(setend, void, env)
>> DEF_HELPER_2(wfi, void, env, i32)
>> DEF_HELPER_1(wfe, void, env)
>> diff --git a/target/arm/syndrome.h b/target/arm/syndrome.h
>> index 54d135897b..e9d97fac6e 100644
>> --- a/target/arm/syndrome.h
>> +++ b/target/arm/syndrome.h
>> @@ -275,4 +275,9 @@ static inline uint32_t syn_illegalstate(void)
>> return (EC_ILLEGALSTATE << ARM_EL_EC_SHIFT) | ARM_EL_IL;
>> }
>>
>> +static inline uint32_t syn_pcalignment(void)
>> +{
>> + return (EC_PCALIGNMENT << ARM_EL_EC_SHIFT) | ARM_EL_IL;
>> +}
>> +
>> #endif /* TARGET_ARM_SYNDROME_H */
>> diff --git a/target/arm/tlb_helper.c b/target/arm/tlb_helper.c
>> index 3107f9823e..25c422976e 100644
>> --- a/target/arm/tlb_helper.c
>> +++ b/target/arm/tlb_helper.c
>> @@ -9,6 +9,7 @@
>> #include "cpu.h"
>> #include "internals.h"
>> #include "exec/exec-all.h"
>> +#include "exec/helper-proto.h"
>>
>> static inline uint32_t merge_syn_data_abort(uint32_t template_syn,
>> unsigned int target_el,
>> @@ -123,6 +124,29 @@ void arm_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr,
>> arm_deliver_fault(cpu, vaddr, access_type, mmu_idx, &fi);
>> }
>>
>> +void helper_exception_pc_alignment(CPUARMState *env, target_ulong pc)
>> +{
>> + int target_el = exception_target_el(env);
>> +
>> + if (target_el == 2 || arm_el_is_aa64(env, target_el)) {
>> + /*
>> + * To aarch64 and aarch32 el2, pc alignment has a
>> + * special exception class.
>> + */
>> + env->exception.vaddress = pc;
>> + env->exception.fsr = 0;
>> + raise_exception(env, EXCP_PREFETCH_ABORT, syn_pcalignment(), target_el);
>> + } else {
>> + /*
>> + * To aarch32 el1, pc alignment is like data alignment
>> + * except with a prefetch abort.
>> + */
>> + ARMMMUFaultInfo fi = { .type = ARMFault_Alignment };
>> + arm_deliver_fault(env_archcpu(env), pc, MMU_INST_FETCH,
>> + cpu_mmu_index(env, true), &fi);
>
> I don't think you should need to special case AArch64 vs AArch32 like this;
> you can do
> env->exception.vaddress = pc;
> env->exception.fsr = the_fsr;
> raise_exception(env, EXCP_PREFETCH_ABORT, syn_pcalignment(), target_el);
>
> for both. AArch64/AArch32-Hyp exception entry will ignore exception.fsr,
> and AArch32-not-Hyp entry will ignore exception.syndrome.
Not true. The latter case still requires syndrome with EC_INSNABORT, etc.
I played with this a bit, but IMO the cleanest solution is the original patch.
> We could probably do with factoring out the
> "if (something) { fsr = arm_fi_to_lfsc(&fi) } else { fsr =
> arm_fi_to_sfsc(&fi); }"
> logic which we currently duplicate in arm_deliver_fault() and
> do_ats_write() and arm_debug_exception_fsr() and also want here.
> (NB I haven't checked these really are doing exactly the same
> condition check...)
It is exactly the same two out of three; the debug_exception one is worded slightly
different. I think it would come out the same if one refactored
arm_s1_regime_using_lpae_format to not use the mmu_idx but instead regime_el(mmu_idx).
r~
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v2 5/8] target/arm: Take an exception if PC is misaligned
2021-09-20 1:29 ` Richard Henderson
@ 2021-09-20 8:08 ` Peter Maydell
2021-09-20 13:29 ` Richard Henderson
0 siblings, 1 reply; 23+ messages in thread
From: Peter Maydell @ 2021-09-20 8:08 UTC (permalink / raw)
To: Richard Henderson; +Cc: qemu-arm, QEMU Developers
On Mon, 20 Sept 2021 at 02:29, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 8/26/21 6:45 AM, Peter Maydell wrote:
> > I don't think you should need to special case AArch64 vs AArch32 like this;
> > you can do
> > env->exception.vaddress = pc;
> > env->exception.fsr = the_fsr;
> > raise_exception(env, EXCP_PREFETCH_ABORT, syn_pcalignment(), target_el);
> >
> > for both. AArch64/AArch32-Hyp exception entry will ignore exception.fsr,
> > and AArch32-not-Hyp entry will ignore exception.syndrome.
>
> Not true. The latter case still requires syndrome with EC_INSNABORT, etc.
For AArch32-not-Hyp ? Syndrome doesn't matter at all in that case
(only Hyp mode and AArch64 have syndrome registers); it just needs
to take the prefetch abort exception, which you get by using
EXCP_PREFETCH_ABORT.
-- PMM
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 5/8] target/arm: Take an exception if PC is misaligned
2021-09-20 8:08 ` Peter Maydell
@ 2021-09-20 13:29 ` Richard Henderson
0 siblings, 0 replies; 23+ messages in thread
From: Richard Henderson @ 2021-09-20 13:29 UTC (permalink / raw)
To: Peter Maydell; +Cc: qemu-arm, QEMU Developers
On 9/20/21 1:08 AM, Peter Maydell wrote:
> On Mon, 20 Sept 2021 at 02:29, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> On 8/26/21 6:45 AM, Peter Maydell wrote:
>>> I don't think you should need to special case AArch64 vs AArch32 like this;
>>> you can do
>>> env->exception.vaddress = pc;
>>> env->exception.fsr = the_fsr;
>>> raise_exception(env, EXCP_PREFETCH_ABORT, syn_pcalignment(), target_el);
>>>
>>> for both. AArch64/AArch32-Hyp exception entry will ignore exception.fsr,
>>> and AArch32-not-Hyp entry will ignore exception.syndrome.
>>
>> Not true. The latter case still requires syndrome with EC_INSNABORT, etc.
>
> For AArch32-not-Hyp ? Syndrome doesn't matter at all in that case
> (only Hyp mode and AArch64 have syndrome registers); it just needs
> to take the prefetch abort exception, which you get by using
> EXCP_PREFETCH_ABORT.
In which case I have incorrect asserts over in cpu_loop. Sigh.
r~
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 6/8] target/arm: Assert thumb pc is aligned
2021-08-21 19:59 [PATCH v2 0/8] target/arm: Fix insn exception priorities Richard Henderson
` (4 preceding siblings ...)
2021-08-21 19:59 ` [PATCH v2 5/8] target/arm: Take an exception if PC is misaligned Richard Henderson
@ 2021-08-21 19:59 ` Richard Henderson
2021-08-21 20:46 ` Philippe Mathieu-Daudé
2021-08-26 13:46 ` Peter Maydell
2021-08-21 19:59 ` [PATCH v2 7/8] target/arm: Suppress bp for exceptions with more priority Richard Henderson
` (2 subsequent siblings)
8 siblings, 2 replies; 23+ messages in thread
From: Richard Henderson @ 2021-08-21 19:59 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-arm
Misaligned thumb PC is architecturally impossible.
Assert is better than proceeding, in case we've missed
something somewhere.
Expand a comment about aligning the pc in gdbstub.
Fail an incoming migrate if a thumb pc is misaligned.
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
target/arm/gdbstub.c | 9 +++++++--
target/arm/machine.c | 9 +++++++++
target/arm/translate.c | 3 +++
3 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c
index 826601b341..a54b42418b 100644
--- a/target/arm/gdbstub.c
+++ b/target/arm/gdbstub.c
@@ -76,8 +76,13 @@ int arm_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
tmp = ldl_p(mem_buf);
- /* Mask out low bit of PC to workaround gdb bugs. This will probably
- cause problems if we ever implement the Jazelle DBX extensions. */
+ /*
+ * Mask out low bits of PC to workaround gdb bugs.
+ * This avoids an assert in thumb_tr_translate_insn, because it is
+ * architecturally impossible to misalign the pc.
+ * This will probably cause problems if we ever implement the
+ * Jazelle DBX extensions.
+ */
if (n == 15) {
tmp &= ~1;
}
diff --git a/target/arm/machine.c b/target/arm/machine.c
index 81e30de824..b5004a67e9 100644
--- a/target/arm/machine.c
+++ b/target/arm/machine.c
@@ -781,6 +781,15 @@ static int cpu_post_load(void *opaque, int version_id)
hw_breakpoint_update_all(cpu);
hw_watchpoint_update_all(cpu);
+ /*
+ * Misaligned thumb pc is architecturally impossible.
+ * We have an assert in thumb_tr_translate_insn to verify this.
+ * Fail an incoming migrate to avoid this assert.
+ */
+ if (!is_a64(env) && env->thumb && (env->regs[15] & 1)) {
+ return -1;
+ }
+
if (!kvm_enabled()) {
pmu_op_finish(&cpu->env);
}
diff --git a/target/arm/translate.c b/target/arm/translate.c
index dfeaa2321d..a93ea3c47c 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -9595,6 +9595,9 @@ static void thumb_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
uint32_t insn;
bool is_16bit;
+ /* Misaligned thumb PC is architecturally impossible. */
+ assert((dc->base.pc_next & 1) == 0);
+
if (arm_check_ss_active(dc) || arm_check_kernelpage(dc)) {
dc->base.pc_next += 2;
return;
--
2.25.1
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v2 6/8] target/arm: Assert thumb pc is aligned
2021-08-21 19:59 ` [PATCH v2 6/8] target/arm: Assert thumb pc is aligned Richard Henderson
@ 2021-08-21 20:46 ` Philippe Mathieu-Daudé
2021-09-19 22:34 ` Richard Henderson
2021-08-26 13:46 ` Peter Maydell
1 sibling, 1 reply; 23+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-08-21 20:46 UTC (permalink / raw)
To: Richard Henderson, qemu-devel; +Cc: qemu-arm
On 8/21/21 9:59 PM, Richard Henderson wrote:
> Misaligned thumb PC is architecturally impossible.
> Assert is better than proceeding, in case we've missed
> something somewhere.
>
> Expand a comment about aligning the pc in gdbstub.
> Fail an incoming migrate if a thumb pc is misaligned.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> target/arm/gdbstub.c | 9 +++++++--
> target/arm/machine.c | 9 +++++++++
> target/arm/translate.c | 3 +++
> 3 files changed, 19 insertions(+), 2 deletions(-)
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index dfeaa2321d..a93ea3c47c 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -9595,6 +9595,9 @@ static void thumb_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
> uint32_t insn;
> bool is_16bit;
>
> + /* Misaligned thumb PC is architecturally impossible. */
> + assert((dc->base.pc_next & 1) == 0);
What about using tcg_debug_assert() instead?
> if (arm_check_ss_active(dc) || arm_check_kernelpage(dc)) {
> dc->base.pc_next += 2;
> return;
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v2 6/8] target/arm: Assert thumb pc is aligned
2021-08-21 19:59 ` [PATCH v2 6/8] target/arm: Assert thumb pc is aligned Richard Henderson
2021-08-21 20:46 ` Philippe Mathieu-Daudé
@ 2021-08-26 13:46 ` Peter Maydell
1 sibling, 0 replies; 23+ messages in thread
From: Peter Maydell @ 2021-08-26 13:46 UTC (permalink / raw)
To: Richard Henderson; +Cc: qemu-arm, QEMU Developers
On Sat, 21 Aug 2021 at 21:04, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Misaligned thumb PC is architecturally impossible.
> Assert is better than proceeding, in case we've missed
> something somewhere.
>
> Expand a comment about aligning the pc in gdbstub.
> Fail an incoming migrate if a thumb pc is misaligned.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
thanks
-- PMM
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 7/8] target/arm: Suppress bp for exceptions with more priority
2021-08-21 19:59 [PATCH v2 0/8] target/arm: Fix insn exception priorities Richard Henderson
` (5 preceding siblings ...)
2021-08-21 19:59 ` [PATCH v2 6/8] target/arm: Assert thumb pc is aligned Richard Henderson
@ 2021-08-21 19:59 ` Richard Henderson
2021-08-21 19:59 ` [PATCH v2 8/8] tests/tcg: Add arm and aarch64 pc alignment tests Richard Henderson
2021-09-13 13:29 ` [PATCH v2 0/8] target/arm: Fix insn exception priorities Peter Maydell
8 siblings, 0 replies; 23+ messages in thread
From: Richard Henderson @ 2021-08-21 19:59 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Maydell, qemu-arm
Both single-step and pc alignment faults have priority over
breakpoint exceptions.
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
target/arm/debug_helper.c | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
diff --git a/target/arm/debug_helper.c b/target/arm/debug_helper.c
index 2983e36dd3..32f3caec23 100644
--- a/target/arm/debug_helper.c
+++ b/target/arm/debug_helper.c
@@ -220,6 +220,7 @@ bool arm_debug_check_breakpoint(CPUState *cs)
{
ARMCPU *cpu = ARM_CPU(cs);
CPUARMState *env = &cpu->env;
+ target_ulong pc;
int n;
/*
@@ -231,6 +232,28 @@ bool arm_debug_check_breakpoint(CPUState *cs)
return false;
}
+ /*
+ * Single-step exceptions have priority over breakpoint exceptions.
+ * If single-step state is active-pending, suppress the bp.
+ */
+ if (arm_singlestep_active(env) && !(env->pstate & PSTATE_SS)) {
+ return false;
+ }
+
+ /*
+ * PC alignment faults have priority over breakpoint exceptions.
+ */
+ pc = is_a64(env) ? env->pc : env->regs[15];
+ if ((is_a64(env) || !env->thumb) && (pc & 3) != 0) {
+ return false;
+ }
+
+ /*
+ * Instruction aborts have priority over breakpoint exceptions.
+ * TODO: We would need to look up the page for PC and verify that
+ * it is present and executable.
+ */
+
for (n = 0; n < ARRAY_SIZE(env->cpu_breakpoint); n++) {
if (bp_wp_matches(cpu, n, false)) {
return true;
--
2.25.1
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH v2 8/8] tests/tcg: Add arm and aarch64 pc alignment tests
2021-08-21 19:59 [PATCH v2 0/8] target/arm: Fix insn exception priorities Richard Henderson
` (6 preceding siblings ...)
2021-08-21 19:59 ` [PATCH v2 7/8] target/arm: Suppress bp for exceptions with more priority Richard Henderson
@ 2021-08-21 19:59 ` Richard Henderson
2021-08-26 13:54 ` Peter Maydell
2021-09-13 13:29 ` [PATCH v2 0/8] target/arm: Fix insn exception priorities Peter Maydell
8 siblings, 1 reply; 23+ messages in thread
From: Richard Henderson @ 2021-08-21 19:59 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-arm
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
tests/tcg/aarch64/pcalign-a64.c | 37 +++++++++++++++++++++++++
tests/tcg/arm/pcalign-a32.c | 46 +++++++++++++++++++++++++++++++
tests/tcg/aarch64/Makefile.target | 4 +--
tests/tcg/arm/Makefile.target | 4 +++
4 files changed, 89 insertions(+), 2 deletions(-)
create mode 100644 tests/tcg/aarch64/pcalign-a64.c
create mode 100644 tests/tcg/arm/pcalign-a32.c
diff --git a/tests/tcg/aarch64/pcalign-a64.c b/tests/tcg/aarch64/pcalign-a64.c
new file mode 100644
index 0000000000..6b9277f919
--- /dev/null
+++ b/tests/tcg/aarch64/pcalign-a64.c
@@ -0,0 +1,37 @@
+/* Test PC misalignment exception */
+
+#include <assert.h>
+#include <signal.h>
+#include <stdlib.h>
+#include <stdio.h>
+
+static void *expected;
+
+static void sigbus(int sig, siginfo_t *info, void *vuc)
+{
+ assert(info->si_code == BUS_ADRALN);
+ assert(info->si_addr == expected);
+ exit(EXIT_SUCCESS);
+}
+
+int main()
+{
+ void *tmp;
+
+ struct sigaction sa = {
+ .sa_sigaction = sigbus,
+ .sa_flags = SA_SIGINFO
+ };
+
+ if (sigaction(SIGBUS, &sa, NULL) < 0) {
+ perror("sigaction");
+ return EXIT_FAILURE;
+ }
+
+ asm volatile("adr %0, 1f + 1\n\t"
+ "str %0, %1\n\t"
+ "br %0\n"
+ "1:"
+ : "=&r"(tmp), "=m"(expected));
+ abort();
+}
diff --git a/tests/tcg/arm/pcalign-a32.c b/tests/tcg/arm/pcalign-a32.c
new file mode 100644
index 0000000000..3c9c8cc97b
--- /dev/null
+++ b/tests/tcg/arm/pcalign-a32.c
@@ -0,0 +1,46 @@
+/* Test PC misalignment exception */
+
+#ifdef __thumb__
+#error "This test must be compiled for ARM"
+#endif
+
+#include <assert.h>
+#include <signal.h>
+#include <stdlib.h>
+#include <stdio.h>
+
+static void *expected;
+
+static void sigbus(int sig, siginfo_t *info, void *vuc)
+{
+ assert(info->si_code == BUS_ADRALN);
+ assert(info->si_addr == expected);
+ exit(EXIT_SUCCESS);
+}
+
+int main()
+{
+ void *tmp;
+
+ struct sigaction sa = {
+ .sa_sigaction = sigbus,
+ .sa_flags = SA_SIGINFO
+ };
+
+ if (sigaction(SIGBUS, &sa, NULL) < 0) {
+ perror("sigaction");
+ return EXIT_FAILURE;
+ }
+
+ asm volatile("adr %0, 1f + 2\n\t"
+ "str %0, %1\n\t"
+ "bx %0\n"
+ "1:"
+ : "=&r"(tmp), "=m"(expected));
+
+ /*
+ * From v8, it is CONSTRAINED UNPREDICTABLE whether BXWritePC aligns
+ * the address or not. If so, we can legitimately fall through.
+ */
+ return EXIT_SUCCESS;
+}
diff --git a/tests/tcg/aarch64/Makefile.target b/tests/tcg/aarch64/Makefile.target
index 2c05c90d17..1d967901bd 100644
--- a/tests/tcg/aarch64/Makefile.target
+++ b/tests/tcg/aarch64/Makefile.target
@@ -8,8 +8,8 @@ VPATH += $(ARM_SRC)
AARCH64_SRC=$(SRC_PATH)/tests/tcg/aarch64
VPATH += $(AARCH64_SRC)
-# Float-convert Tests
-AARCH64_TESTS=fcvt
+# Base architecture tests
+AARCH64_TESTS=fcvt pcalign-a64
fcvt: LDFLAGS+=-lm
diff --git a/tests/tcg/arm/Makefile.target b/tests/tcg/arm/Makefile.target
index 5ab59ed6ce..f509d823d4 100644
--- a/tests/tcg/arm/Makefile.target
+++ b/tests/tcg/arm/Makefile.target
@@ -29,6 +29,10 @@ run-fcvt: fcvt
$(call run-test,fcvt,$(QEMU) $<,"$< on $(TARGET_NAME)")
$(call diff-out,fcvt,$(ARM_SRC)/fcvt.ref)
+# PC alignment test
+ARM_TESTS += pcalign-a32
+pcalign-a32: CFLAGS+=-marm
+
ifeq ($(CONFIG_ARM_COMPATIBLE_SEMIHOSTING),y)
# Semihosting smoke test for linux-user
--
2.25.1
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v2 8/8] tests/tcg: Add arm and aarch64 pc alignment tests
2021-08-21 19:59 ` [PATCH v2 8/8] tests/tcg: Add arm and aarch64 pc alignment tests Richard Henderson
@ 2021-08-26 13:54 ` Peter Maydell
2021-08-28 4:04 ` Richard Henderson
0 siblings, 1 reply; 23+ messages in thread
From: Peter Maydell @ 2021-08-26 13:54 UTC (permalink / raw)
To: Richard Henderson; +Cc: qemu-arm, QEMU Developers
On Sat, 21 Aug 2021 at 21:09, Richard Henderson
<richard.henderson@linaro.org> wrote:
> + /*
> + * From v8, it is CONSTRAINED UNPREDICTABLE whether BXWritePC aligns
> + * the address or not. If so, we can legitimately fall through.
> + */
> + return EXIT_SUCCESS;
> +}
Can we get the test harness to run the test on a cortex-a9 guest CPU
so we can avoid the UNPREDICTABLE and can always check the signal
happened ? The test is a bit weak if it doesn't actually test that
we take an exception...
Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
thanks
-- PMM
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 8/8] tests/tcg: Add arm and aarch64 pc alignment tests
2021-08-26 13:54 ` Peter Maydell
@ 2021-08-28 4:04 ` Richard Henderson
0 siblings, 0 replies; 23+ messages in thread
From: Richard Henderson @ 2021-08-28 4:04 UTC (permalink / raw)
To: Peter Maydell; +Cc: qemu-arm, QEMU Developers
On 8/26/21 6:54 AM, Peter Maydell wrote:
> On Sat, 21 Aug 2021 at 21:09, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>
>> + /*
>> + * From v8, it is CONSTRAINED UNPREDICTABLE whether BXWritePC aligns
>> + * the address or not. If so, we can legitimately fall through.
>> + */
>> + return EXIT_SUCCESS;
>> +}
>
> Can we get the test harness to run the test on a cortex-a9 guest CPU
> so we can avoid the UNPREDICTABLE and can always check the signal
> happened ? The test is a bit weak if it doesn't actually test that
> we take an exception...
Well, it'll always raise an exception under qemu.
I wrote it this way so that it would also always succeed when run on real hardware. Even
then, I have no specific knowledge of a cpu that does not raise an exception, but I only
tested cortex-a57.
r~
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 0/8] target/arm: Fix insn exception priorities
2021-08-21 19:59 [PATCH v2 0/8] target/arm: Fix insn exception priorities Richard Henderson
` (7 preceding siblings ...)
2021-08-21 19:59 ` [PATCH v2 8/8] tests/tcg: Add arm and aarch64 pc alignment tests Richard Henderson
@ 2021-09-13 13:29 ` Peter Maydell
8 siblings, 0 replies; 23+ messages in thread
From: Peter Maydell @ 2021-09-13 13:29 UTC (permalink / raw)
To: Richard Henderson; +Cc: qemu-arm, QEMU Developers
On Sat, 21 Aug 2021 at 21:02, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Raise pc alignment faults.
> Fix single-step and pc-align priority over breakpoints.
> Not yet fixing insn abort priority over breakpoints.
>
> Based-on: 20210813131809.28655-1-peter.maydell@linaro.org
> ("linux-user: Clean up siginfo_t handling for arm, aarch64")
>
> Changes for v2:
> * Handle the exceptions in cpu_loop.
> * Fix how the instruction is raised for aa32 el1.
> * Add pc alignment test cases.
I'm going to take patches 1 and 2 from this series into
target-arm.next.
thanks
-- PMM
^ permalink raw reply [flat|nested] 23+ messages in thread