Re: [Qemu-devel] [PATCH 06/11] target/arm: Reset btype for direct branches and syscalls
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~
Re: [Qemu-devel] [PATCH 06/11] target/arm: Reset btype for direct branches and syscalls
On Tue, 29 Jan 2019 at 14:05, Richard Henderson 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
Re: [Qemu-devel] [PATCH 06/11] target/arm: Reset btype for direct branches and syscalls
On Mon, 28 Jan 2019 at 21:28, Richard Henderson wrote: > > On 1/22/19 6:12 AM, Peter Maydell wrote: > > On Thu, 10 Jan 2019 at 12:17, Richard Henderson > > 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
Re: [Qemu-devel] [PATCH 06/11] target/arm: Reset btype for direct branches and syscalls
On 1/22/19 6:12 AM, Peter Maydell wrote: > On Thu, 10 Jan 2019 at 12:17, Richard Henderson > 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~
Re: [Qemu-devel] [PATCH 06/11] target/arm: Reset btype for direct branches and syscalls
On Thu, 10 Jan 2019 at 12:17, Richard Henderson 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
[Qemu-devel] [PATCH 06/11] target/arm: Reset btype for direct branches and syscalls
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
---
target/arm/translate-a64.c | 9 +
1 file changed, 9 insertions(+)
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:
--
2.17.2
