[06/11] target/arm: Reset btype for direct branches and syscalls

Message ID 20190110121736.23448-7-richard.henderson@linaro.org
State New
Headers show
Series
  • target/arm: Implement ARMv8.5-BTI
Related show

Commit Message

Richard Henderson Jan. 10, 2019, 12:17 p.m.
This is all of the non-exception cases of DISAS_NORETURN.

For the rest of the synchronous exceptions, the state of
SPSR_ELx.BTYPE is CONSTRAINED UNPREDICTABLE.  However, it
makes more sense to me to have syscalls reset BTYPE.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

---
 target/arm/translate-a64.c | 9 +++++++++
 1 file changed, 9 insertions(+)

-- 
2.17.2

Comments

Peter Maydell Jan. 22, 2019, 2:12 p.m. | #1
On Thu, 10 Jan 2019 at 12:17, Richard Henderson
<richard.henderson@linaro.org> wrote:
>

> This is all of the non-exception cases of DISAS_NORETURN.


What about the gen_helper_exit_atomic() exit cases ?

> For the rest of the synchronous exceptions, the state of

> SPSR_ELx.BTYPE is CONSTRAINED UNPREDICTABLE.  However, it

> makes more sense to me to have syscalls reset BTYPE.


The advantage of picking the other choice (SPSR_ELx.BTYPE ==
PSTATE.BTYPE) is that it means that the behaviour is identical
for all exceptions (async or sync of any type) and we don't
do the work of clearing the BTYPE field (which will happen
potentially in "normal" guest code if we're not in a guarded page,
I think).

thanks
-- PMM
Richard Henderson Jan. 28, 2019, 9:28 p.m. | #2
On 1/22/19 6:12 AM, Peter Maydell wrote:
> On Thu, 10 Jan 2019 at 12:17, Richard Henderson

> <richard.henderson@linaro.org> wrote:

>>

>> This is all of the non-exception cases of DISAS_NORETURN.

> 

> What about the gen_helper_exit_atomic() exit cases ?


In that case we are going to re-execute the same insn with a different
translation, so we do not want to change btype.

(Although I'm not sure how the guest could tell.  Given where we check for
btype mismatch, we would recognize the BTI exception before getting into the
ldst_ex path that generates the ATOMIC exception.  So any DataAbort exception
that the atomic insn itself might generate must also have BTYPE == 0.)

>> For the rest of the synchronous exceptions, the state of

>> SPSR_ELx.BTYPE is CONSTRAINED UNPREDICTABLE.  However, it

>> makes more sense to me to have syscalls reset BTYPE.

> 

> The advantage of picking the other choice (SPSR_ELx.BTYPE ==

> PSTATE.BTYPE) is that it means that the behaviour is identical

> for all exceptions (async or sync of any type) and we don't

> do the work of clearing the BTYPE field (which will happen

> potentially in "normal" guest code if we're not in a guarded page,

> I think).


Well, BTYPE is in the TB flags, so we know it's already zero in that case, so
there's no extra work.  But you're probably right about not making syscall
special.  I've removed that.


r~
Peter Maydell Jan. 29, 2019, 9:57 a.m. | #3
On Mon, 28 Jan 2019 at 21:28, Richard Henderson
<richard.henderson@linaro.org> wrote:
>

> On 1/22/19 6:12 AM, Peter Maydell wrote:

> > On Thu, 10 Jan 2019 at 12:17, Richard Henderson

> > <richard.henderson@linaro.org> wrote:

> >>

> >> This is all of the non-exception cases of DISAS_NORETURN.

> >

> > What about the gen_helper_exit_atomic() exit cases ?

>

> In that case we are going to re-execute the same insn with a different

> translation, so we do not want to change btype.

>

> (Although I'm not sure how the guest could tell.  Given where we check for

> btype mismatch, we would recognize the BTI exception before getting into the

> ldst_ex path that generates the ATOMIC exception.  So any DataAbort exception

> that the atomic insn itself might generate must also have BTYPE == 0.)


Yeah, I was mostly asking because they're the other
DISAS_NORETURN cases and they're neither changed in the
patch nor mentioned as special cases in the commit message.

> > The advantage of picking the other choice (SPSR_ELx.BTYPE ==

> > PSTATE.BTYPE) is that it means that the behaviour is identical

> > for all exceptions (async or sync of any type) and we don't

> > do the work of clearing the BTYPE field (which will happen

> > potentially in "normal" guest code if we're not in a guarded page,

> > I think).

>

> Well, BTYPE is in the TB flags, so we know it's already zero in that case, so

> there's no extra work.


It's not zero if we just did a BR Xn to get to this SVC insn, is it?

thanks
-- PMM
Richard Henderson Jan. 29, 2019, 2:05 p.m. | #4
On 1/29/19 1:57 AM, Peter Maydell wrote:
>>> The advantage of picking the other choice (SPSR_ELx.BTYPE ==

>>> PSTATE.BTYPE) is that it means that the behaviour is identical

>>> for all exceptions (async or sync of any type) and we don't

>>> do the work of clearing the BTYPE field (which will happen

>>> potentially in "normal" guest code if we're not in a guarded page,

>>> I think).

>>

>> Well, BTYPE is in the TB flags, so we know it's already zero in that case, so

>> there's no extra work.

> 

> It's not zero if we just did a BR Xn to get to this SVC insn, is it?


I guess I misunderstood what you meant by "extra" work.
It's not "extra" if btype is known to not be zero...

Anyway, in v2 the clearing of btype happens in cpu_loop,
more like what the kernel would have to do.


r~
Peter Maydell Jan. 29, 2019, 2:06 p.m. | #5
On Tue, 29 Jan 2019 at 14:05, Richard Henderson
<richard.henderson@linaro.org> wrote:
>

> On 1/29/19 1:57 AM, Peter Maydell wrote:

> >>> The advantage of picking the other choice (SPSR_ELx.BTYPE ==

> >>> PSTATE.BTYPE) is that it means that the behaviour is identical

> >>> for all exceptions (async or sync of any type) and we don't

> >>> do the work of clearing the BTYPE field (which will happen

> >>> potentially in "normal" guest code if we're not in a guarded page,

> >>> I think).

> >>

> >> Well, BTYPE is in the TB flags, so we know it's already zero in that case, so

> >> there's no extra work.

> >

> > It's not zero if we just did a BR Xn to get to this SVC insn, is it?

>

> I guess I misunderstood what you meant by "extra" work.

> It's not "extra" if btype is known to not be zero...


The architecture doesn't require it to be cleared in that
situation, unless I've misunderstood it. So unless the kernel
is explicitly clearing the BTYPE in the SPSR (which I don't
think it is obliged to do either) then clearing it is
work we don't need to do.

thanks
-- PMM

Patch

diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index 68eb27089a..f319fa000e 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -1362,6 +1362,7 @@  static void disas_uncond_b_imm(DisasContext *s, uint32_t insn)
     }
 
     /* B Branch / BL Branch with link */
+    reset_btype(s);
     gen_goto_tb(s, 0, addr);
 }
 
@@ -1386,6 +1387,7 @@  static void disas_comp_b_imm(DisasContext *s, uint32_t insn)
     tcg_cmp = read_cpu_reg(s, rt, sf);
     label_match = gen_new_label();
 
+    reset_btype(s);
     tcg_gen_brcondi_i64(op ? TCG_COND_NE : TCG_COND_EQ,
                         tcg_cmp, 0, label_match);
 
@@ -1415,6 +1417,8 @@  static void disas_test_b_imm(DisasContext *s, uint32_t insn)
     tcg_cmp = tcg_temp_new_i64();
     tcg_gen_andi_i64(tcg_cmp, cpu_reg(s, rt), (1ULL << bit_pos));
     label_match = gen_new_label();
+
+    reset_btype(s);
     tcg_gen_brcondi_i64(op ? TCG_COND_NE : TCG_COND_EQ,
                         tcg_cmp, 0, label_match);
     tcg_temp_free_i64(tcg_cmp);
@@ -1441,6 +1445,7 @@  static void disas_cond_b_imm(DisasContext *s, uint32_t insn)
     addr = s->pc + sextract32(insn, 5, 19) * 4 - 4;
     cond = extract32(insn, 0, 4);
 
+    reset_btype(s);
     if (cond < 0x0e) {
         /* genuinely conditional branches */
         TCGLabel *label_match = gen_new_label();
@@ -1605,6 +1610,7 @@  static void handle_sync(DisasContext *s, uint32_t insn,
          * a self-modified code correctly and also to take
          * any pending interrupts immediately.
          */
+        reset_btype(s);
         gen_goto_tb(s, 0, s->pc);
         return;
     default:
@@ -1885,6 +1891,7 @@  static void disas_exc(DisasContext *s, uint32_t insn)
         switch (op2_ll) {
         case 1:                                                     /* SVC */
             gen_ss_advance(s);
+            reset_btype(s);
             gen_exception_insn(s, 0, EXCP_SWI, syn_aa64_svc(imm16),
                                default_exception_el(s));
             break;
@@ -1899,6 +1906,7 @@  static void disas_exc(DisasContext *s, uint32_t insn)
             gen_a64_set_pc_im(s->pc - 4);
             gen_helper_pre_hvc(cpu_env);
             gen_ss_advance(s);
+            reset_btype(s);
             gen_exception_insn(s, 0, EXCP_HVC, syn_aa64_hvc(imm16), 2);
             break;
         case 3:                                                     /* SMC */
@@ -1911,6 +1919,7 @@  static void disas_exc(DisasContext *s, uint32_t insn)
             gen_helper_pre_smc(cpu_env, tmp);
             tcg_temp_free_i32(tmp);
             gen_ss_advance(s);
+            reset_btype(s);
             gen_exception_insn(s, 0, EXCP_SMC, syn_aa64_smc(imm16), 3);
             break;
         default: