diff mbox series

[3/4] target/arm: Take an exception if PC is misaligned

Message ID 20210818010041.337010-4-richard.henderson@linaro.org
State New
Headers show
Series target/arm: Fix insn exception priorities | expand

Commit Message

Richard Henderson Aug. 18, 2021, 1 a.m. UTC 
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 UNDEFINED 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/syndrome.h      |  5 ++++
 target/arm/translate-a64.c | 12 +++++++++
 target/arm/translate.c     | 50 +++++++++++++++++++++++++++-----------
 3 files changed, 53 insertions(+), 14 deletions(-)

-- 
2.25.1

Comments

Richard Henderson Aug. 18, 2021, 4:44 p.m. UTC | #1 
On 8/17/21 3:00 PM, Richard Henderson wrote:
> With v8, this is CONSTRAINED UNDEFINED and may either raise an


Bah, UNPREDICTABLE, of course, not UNDEFINED.

r~
Peter Maydell Aug. 19, 2021, 1:40 p.m. UTC | #2 
On Wed, 18 Aug 2021 at 02:04, 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 UNDEFINED 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>

>  static void arm_post_translate_insn(DisasContext *dc)

>  {

>      if (dc->condjmp && !dc->base.is_jmp) {

> @@ -9500,7 +9504,25 @@ static void arm_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)

>      CPUARMState *env = cpu->env_ptr;

>      unsigned int insn;

>

> -    if (arm_pre_translate_insn(dc)) {

> +    /* Singlestep exceptions have the highest priority. */

> +    if (arm_check_ss_active(dc)) {

> +        dc->base.pc_next += 4;

> +        return;

> +    }

> +

> +    if (dc->base.pc_next & 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).

> +         */

> +        gen_exception_insn(dc, dc->base.pc_next, EXCP_UDEF,

> +                           syn_pcalignment(), default_exception_el(dc));

> +        dc->base.pc_next = QEMU_ALIGN_UP(dc->base.pc_next, 4);

> +        return;

> +    }

> +

> +    if (arm_check_kernelpage(dc)) {

>          dc->base.pc_next += 4;

>          return;

>      }

> @@ -9570,7 +9592,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)) {



Is it not possible to get a misaligned PC in the Thumb case ?

>          dc->base.pc_next += 2;

>          return;

>      }


-- PMM
Richard Henderson Aug. 19, 2021, 4:50 p.m. UTC | #3 
On 8/19/21 3:40 AM, Peter Maydell wrote:
>>       uint32_t insn;

>>       bool is_16bit;

>>

>> -    if (arm_pre_translate_insn(dc)) {

>> +    if (arm_check_ss_active(dc) || arm_check_kernelpage(dc)) {

> 

> 

> Is it not possible to get a misaligned PC in the Thumb case ?


No.  The thumb bit is always removed, leaving all pc aligned mod 2.
Both BXWritePC and BranchWritePC do this, as do we in gen_bx and store_reg.


r~
Richard Henderson Aug. 19, 2021, 4:57 p.m. UTC | #4 
On 8/19/21 6:50 AM, Richard Henderson wrote:
> On 8/19/21 3:40 AM, Peter Maydell wrote:

>>>       uint32_t insn;

>>>       bool is_16bit;

>>>

>>> -    if (arm_pre_translate_insn(dc)) {

>>> +    if (arm_check_ss_active(dc) || arm_check_kernelpage(dc)) {

>>

>>

>> Is it not possible to get a misaligned PC in the Thumb case ?

> 

> No.  The thumb bit is always removed, leaving all pc aligned mod 2.

> Both BXWritePC and BranchWritePC do this, as do we in gen_bx and store_reg.


Do you think it's worth an assert here to make sure we never miss a case?  I did an audit 
of the exception code and it looks like we mask everything correctly there, but...


r~
Peter Maydell Aug. 19, 2021, 7:18 p.m. UTC | #5 
On Wed, 18 Aug 2021 at 02:04, 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 UNDEFINED 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.


> @@ -9500,7 +9504,25 @@ static void arm_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)

>      CPUARMState *env = cpu->env_ptr;

>      unsigned int insn;

>

> -    if (arm_pre_translate_insn(dc)) {

> +    /* Singlestep exceptions have the highest priority. */

> +    if (arm_check_ss_active(dc)) {

> +        dc->base.pc_next += 4;

> +        return;

> +    }

> +

> +    if (dc->base.pc_next & 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).

> +         */

> +        gen_exception_insn(dc, dc->base.pc_next, EXCP_UDEF,

> +                           syn_pcalignment(), default_exception_el(dc));

> +        dc->base.pc_next = QEMU_ALIGN_UP(dc->base.pc_next, 4);


Just noticed that section G1.16.7 says that when we report
PC alignment faults to AArch32 they should be prefetch aborts,
not UDEF. The fault address and fault status registers also need
to be set (with slightly varying behaviour for when the fault
is taken to Hyp mode).

For AArch64 we should also be setting the FAR, which means
that for consistency it's better to use EXCP_PREFETCH_ABORT
and set exception.vaddress in the translate-a64.c code
(you get better logging in the exception-entry code)
even though these different EXCP_* all boil down to the
same synchronous-exception vector.

-- PMM
Peter Maydell Aug. 19, 2021, 7:24 p.m. UTC | #6 
On Thu, 19 Aug 2021 at 17:57, Richard Henderson
<richard.henderson@linaro.org> wrote:
>

> On 8/19/21 6:50 AM, Richard Henderson wrote:

> > On 8/19/21 3:40 AM, Peter Maydell wrote:

> >>>       uint32_t insn;

> >>>       bool is_16bit;

> >>>

> >>> -    if (arm_pre_translate_insn(dc)) {

> >>> +    if (arm_check_ss_active(dc) || arm_check_kernelpage(dc)) {

> >>

> >>

> >> Is it not possible to get a misaligned PC in the Thumb case ?

> >

> > No.  The thumb bit is always removed, leaving all pc aligned mod 2.

> > Both BXWritePC and BranchWritePC do this, as do we in gen_bx and store_reg.

>

> Do you think it's worth an assert here to make sure we never miss a case?  I did an audit

> of the exception code and it looks like we mask everything correctly there, but...


(Did you check the M-profile code too? That also architecturally I think
should never let PC have the low bit set; hopefully the code I wrote
actually ensures that...)

I guess an assert() is more helpful than ploughing ahead with
a misaligned PC value. If we don't assert we should at least have
a comment saying misaligned Thumb PCs are architecturally impossible.

If we do go for the assert, then the comment in arm_cpu_gdb_write_register()
about why we don't let GDB set bit 0 in the PC would need updating (there
would now be two reasons). We should probably also fail the migration if
we get an unaligned Thumb PC in the inbound data.

-- PMM
Peter Maydell Aug. 19, 2021, 7:46 p.m. UTC | #7 
On Thu, 19 Aug 2021 at 20:18, Peter Maydell <peter.maydell@linaro.org> wrote:
> Just noticed that section G1.16.7 says that when we report

> PC alignment faults to AArch32 they should be prefetch aborts,

> not UDEF. The fault address and fault status registers also need

> to be set (with slightly varying behaviour for when the fault

> is taken to Hyp mode).

>

> For AArch64 we should also be setting the FAR, which means

> that for consistency it's better to use EXCP_PREFETCH_ABORT

> and set exception.vaddress in the translate-a64.c code

> (you get better logging in the exception-entry code)

> even though these different EXCP_* all boil down to the

> same synchronous-exception vector.


Also, looking at kernel code while reviewing your alignment-checking
patchset, I realized that we should also catch this case of
a prefetch abort in linux-user/ and turn it into a
SIGBUS/BUS_ADRALN with the address being whatever the value
in the FAR is (for both arm and aarch64).

-- PMM
Richard Henderson Aug. 19, 2021, 8:34 p.m. UTC | #8 
On 8/19/21 9:24 AM, Peter Maydell wrote:
> (Did you check the M-profile code too? That also architecturally I think

> should never let PC have the low bit set; hopefully the code I wrote

> actually ensures that...)


Exception handling in m-profile is much harder to follow, but certainly normal updates to 
_NextInstrAddr are all force aligned.

r~
diff mbox series

Patch

diff --git a/target/arm/syndrome.h b/target/arm/syndrome.h
index c590a109da..569b0c1115 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;
 }
 
+static inline uint32_t syn_pcalignment(void)
+{
+    return EC_PCALIGNMENT << ARM_EL_EC_SHIFT;
+}
+
 #endif /* TARGET_ARM_SYNDROME_H */
diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index 333bc836b2..c394bddac6 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -14754,6 +14754,7 @@  static void aarch64_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
     CPUARMState *env = cpu->env_ptr;
     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
@@ -14771,6 +14772,17 @@  static void aarch64_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
         return;
     }
 
+    if (s->base.pc_next & 3) {
+        /*
+         * PC alignment fault.  This has priority over the instruction abort
+         * that we would receive from a translation fault via arm_ldl_code.
+         */
+        gen_exception_insn(s, s->base.pc_next, EXCP_UDEF,
+                           syn_pcalignment(), default_exception_el(s));
+        s->base.pc_next = QEMU_ALIGN_UP(s->base.pc_next, 4);
+        return;
+    }
+
     s->pc_curr = s->base.pc_next;
     insn = arm_ldl_code(env, s->base.pc_next, s->sctlr_b);
     s->insn = insn;
diff --git a/target/arm/translate.c b/target/arm/translate.c
index 5e0fc8a0a0..00ddd4879c 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -9452,19 +9452,8 @@  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_ss_active(DisasContext *dc)
 {
-#ifdef CONFIG_USER_ONLY
-    /* Intercept jump to the magic kernel page.  */
-    if (dc->base.pc_next >= 0xffff0000) {
-        /* We always get here via a jump, so know we are not in a
-           conditional execution block.  */
-        gen_exception_internal(EXCP_KERNEL_TRAP);
-        dc->base.is_jmp = DISAS_NORETURN;
-        return true;
-    }
-#endif
-
     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
@@ -9485,6 +9474,21 @@  static bool arm_pre_translate_insn(DisasContext *dc)
     return false;
 }
 
+static bool arm_check_kernelpage(DisasContext *dc)
+{
+#ifdef CONFIG_USER_ONLY
+    /* Intercept jump to the magic kernel page.  */
+    if (dc->base.pc_next >= 0xffff0000) {
+        /* We always get here via a jump, so know we are not in a
+           conditional execution block.  */
+        gen_exception_internal(EXCP_KERNEL_TRAP);
+        dc->base.is_jmp = DISAS_NORETURN;
+        return true;
+    }
+#endif
+    return false;
+}
+
 static void arm_post_translate_insn(DisasContext *dc)
 {
     if (dc->condjmp && !dc->base.is_jmp) {
@@ -9500,7 +9504,25 @@  static void arm_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
     CPUARMState *env = cpu->env_ptr;
     unsigned int insn;
 
-    if (arm_pre_translate_insn(dc)) {
+    /* Singlestep exceptions have the highest priority. */
+    if (arm_check_ss_active(dc)) {
+        dc->base.pc_next += 4;
+        return;
+    }
+
+    if (dc->base.pc_next & 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).
+         */
+        gen_exception_insn(dc, dc->base.pc_next, EXCP_UDEF,
+                           syn_pcalignment(), default_exception_el(dc));
+        dc->base.pc_next = QEMU_ALIGN_UP(dc->base.pc_next, 4);
+        return;
+    }
+
+    if (arm_check_kernelpage(dc)) {
         dc->base.pc_next += 4;
         return;
     }
@@ -9570,7 +9592,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;
     }