diff mbox series

[06/67] target/arm: Introduce pc_read

Message ID 20190726175032.6769-7-richard.henderson@linaro.org
State New
Headers show
Series target/arm: Convert aa32 base isa to decodetree | expand

Commit Message

Richard Henderson July 26, 2019, 5:49 p.m. UTC 
We currently have 3 different ways of computing the architectural
value of "PC" as seen in the ARM ARM.

The value of s->pc has been incremented past the current insn,
but that is all.  Thus for a32, PC = s->pc + 4; for t32, PC = s->pc;
for t16, PC = s->pc + 2.  These differing computations make it
impossible at present to unify the various code paths.

Let s->pc_read hold the architectural value of PC for all cases.

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

---
 target/arm/translate.h | 10 ++++++++
 target/arm/translate.c | 53 ++++++++++++++++++------------------------
 2 files changed, 32 insertions(+), 31 deletions(-)

-- 
2.17.1

Comments

Peter Maydell July 29, 2019, 2:05 p.m. UTC | #1 
On Fri, 26 Jul 2019 at 18:50, Richard Henderson
<richard.henderson@linaro.org> wrote:
>

> We currently have 3 different ways of computing the architectural

> value of "PC" as seen in the ARM ARM.

>

> The value of s->pc has been incremented past the current insn,

> but that is all.  Thus for a32, PC = s->pc + 4; for t32, PC = s->pc;

> for t16, PC = s->pc + 2.  These differing computations make it

> impossible at present to unify the various code paths.

>

> Let s->pc_read hold the architectural value of PC for all cases.

>

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

> ---

>  target/arm/translate.h | 10 ++++++++

>  target/arm/translate.c | 53 ++++++++++++++++++------------------------

>  2 files changed, 32 insertions(+), 31 deletions(-)

>

> diff --git a/target/arm/translate.h b/target/arm/translate.h

> index a20f6e2056..2dfdd8ca66 100644

> --- a/target/arm/translate.h

> +++ b/target/arm/translate.h

> @@ -9,7 +9,17 @@ typedef struct DisasContext {

>      DisasContextBase base;

>      const ARMISARegisters *isar;

>

> +    /*

> +     * Summary of the various values for "PC":

> +     * base.pc_next -- the start of the current insn

> +     * pc           -- the start of the next insn


These are confusingly named -- logically "pc_next" ought to
be the PC of the next instruction and "pc" ought to be
that of the current one...

> +     * pc_read      -- the value for "PC" in the ARM ARM;


nit: this line should end with a colon, rather than a semicolon

> +     *                 in arm mode, the current insn + 8;

> +     *                 in thumb mode, the current insn + 4;

> +     *                 in aa64 mode, unused.

> +     */

>      target_ulong pc;

> +    target_ulong pc_read;

>      target_ulong page_start;

>      uint32_t insn;


Why target_ulong rather than uint32_t, given this is
aarch32 only?

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>


thanks
-- PMM
Richard Henderson July 30, 2019, 12:38 a.m. UTC | #2 
On 7/29/19 7:05 AM, Peter Maydell wrote:
> On Fri, 26 Jul 2019 at 18:50, Richard Henderson

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

>>

>> We currently have 3 different ways of computing the architectural

>> value of "PC" as seen in the ARM ARM.

>>

>> The value of s->pc has been incremented past the current insn,

>> but that is all.  Thus for a32, PC = s->pc + 4; for t32, PC = s->pc;

>> for t16, PC = s->pc + 2.  These differing computations make it

>> impossible at present to unify the various code paths.

>>

>> Let s->pc_read hold the architectural value of PC for all cases.

>>

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

>> ---

>>  target/arm/translate.h | 10 ++++++++

>>  target/arm/translate.c | 53 ++++++++++++++++++------------------------

>>  2 files changed, 32 insertions(+), 31 deletions(-)

>>

>> diff --git a/target/arm/translate.h b/target/arm/translate.h

>> index a20f6e2056..2dfdd8ca66 100644

>> --- a/target/arm/translate.h

>> +++ b/target/arm/translate.h

>> @@ -9,7 +9,17 @@ typedef struct DisasContext {

>>      DisasContextBase base;

>>      const ARMISARegisters *isar;

>>

>> +    /*

>> +     * Summary of the various values for "PC":

>> +     * base.pc_next -- the start of the current insn

>> +     * pc           -- the start of the next insn

> 

> These are confusingly named -- logically "pc_next" ought to

> be the PC of the next instruction and "pc" ought to be

> that of the current one...


Yes, well.  I don't quite remember why "pc_next" was chosen for this field.  It
is the "next" upon entry to tr_foo_disas_insn().  Often the target will
increment s->base.pc_next immediately, so it will also be the "next" insn
throughout translation.  Though that isn't currently the case for ARM.

Once most of the uses of s->pc get moved to s->pc_read, it might be reasonable
to rename the remaining "s->base.pc_next" -> "s->pc_orig" and "s->pc" ->
"s->base.pc_next".  Perhaps that would be clearer, I'm not sure.


>> +     * pc_read      -- the value for "PC" in the ARM ARM;

> 

> nit: this line should end with a colon, rather than a semicolon


Oops, typo.


>> +     *                 in arm mode, the current insn + 8;

>> +     *                 in thumb mode, the current insn + 4;

>> +     *                 in aa64 mode, unused.

>> +     */

>>      target_ulong pc;

>> +    target_ulong pc_read;

>>      target_ulong page_start;

>>      uint32_t insn;

> 

> Why target_ulong rather than uint32_t, given this is

> aarch32 only?


I didn't know if it would stay aarch32 only and it seemed more natural to match
the types.


r~
Peter Maydell Aug. 6, 2019, 10 a.m. UTC | #3 
On Tue, 30 Jul 2019 at 01:39, Richard Henderson
<richard.henderson@linaro.org> wrote:
>

> On 7/29/19 7:05 AM, Peter Maydell wrote:

> > On Fri, 26 Jul 2019 at 18:50, Richard Henderson

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

> >>

> >> We currently have 3 different ways of computing the architectural

> >> value of "PC" as seen in the ARM ARM.

> >>

> >> The value of s->pc has been incremented past the current insn,

> >> but that is all.  Thus for a32, PC = s->pc + 4; for t32, PC = s->pc;

> >> for t16, PC = s->pc + 2.  These differing computations make it

> >> impossible at present to unify the various code paths.

> >>

> >> Let s->pc_read hold the architectural value of PC for all cases.

> >>

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

> >> ---

> >>  target/arm/translate.h | 10 ++++++++

> >>  target/arm/translate.c | 53 ++++++++++++++++++------------------------

> >>  2 files changed, 32 insertions(+), 31 deletions(-)

> >>

> >> diff --git a/target/arm/translate.h b/target/arm/translate.h

> >> index a20f6e2056..2dfdd8ca66 100644

> >> --- a/target/arm/translate.h

> >> +++ b/target/arm/translate.h

> >> @@ -9,7 +9,17 @@ typedef struct DisasContext {

> >>      DisasContextBase base;

> >>      const ARMISARegisters *isar;

> >>

> >> +    /*

> >> +     * Summary of the various values for "PC":

> >> +     * base.pc_next -- the start of the current insn

> >> +     * pc           -- the start of the next insn

> >

> > These are confusingly named -- logically "pc_next" ought to

> > be the PC of the next instruction and "pc" ought to be

> > that of the current one...

>

> Yes, well.  I don't quite remember why "pc_next" was chosen for this field.  It

> is the "next" upon entry to tr_foo_disas_insn().  Often the target will

> increment s->base.pc_next immediately, so it will also be the "next" insn

> throughout translation.  Though that isn't currently the case for ARM.

>

> Once most of the uses of s->pc get moved to s->pc_read, it might be reasonable

> to rename the remaining "s->base.pc_next" -> "s->pc_orig" and "s->pc" ->

> "s->base.pc_next".  Perhaps that would be clearer, I'm not sure.


Renaming pc_next would be a cross-target change, so let's put that
on the shelf for the moment. Maybe just put a TODO comment to the
effect that we could consider renaming in future ?

Or we could just copy s->base.pc_next into s->some_field_we_choose_the_name_of,
eating 8 unnecessary bytes in the DisasContext struct but giving us a more
uniform s->something for all of the different PC variants rather than
having one of them be in s->base.something.

thanks
-- PMM
Richard Henderson Aug. 6, 2019, 3:04 p.m. UTC | #4 
On 8/6/19 3:00 AM, Peter Maydell wrote:
> Renaming pc_next would be a cross-target change, so let's put that

> on the shelf for the moment. Maybe just put a TODO comment to the

> effect that we could consider renaming in future ?


I wasn't suggesting renaming the cross-target variable.

I was suggesting shuffling around the current names, and using "pc_next" for
what it sounds like -- the pc of the next insn.


r~
Peter Maydell Aug. 6, 2019, 3:21 p.m. UTC | #5 
On Tue, 6 Aug 2019 at 16:04, Richard Henderson
<richard.henderson@linaro.org> wrote:
>

> On 8/6/19 3:00 AM, Peter Maydell wrote:

> > Renaming pc_next would be a cross-target change, so let's put that

> > on the shelf for the moment. Maybe just put a TODO comment to the

> > effect that we could consider renaming in future ?

>

> I wasn't suggesting renaming the cross-target variable.

>

> I was suggesting shuffling around the current names, and using "pc_next" for

> what it sounds like -- the pc of the next insn.


Oh, I see, so incrementing base->pc_next after we load
the insn? Yeah, that would work too. Though it seems a bit
odd to me to have the target-specific code modifying
a field in the base struct -- that seems like it ought to
be purely for the generic TCG code to use.

thanks
-- PMM
Richard Henderson Aug. 6, 2019, 3:53 p.m. UTC | #6 
On 8/6/19 8:21 AM, Peter Maydell wrote:
> On Tue, 6 Aug 2019 at 16:04, Richard Henderson

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

>>

>> On 8/6/19 3:00 AM, Peter Maydell wrote:

>>> Renaming pc_next would be a cross-target change, so let's put that

>>> on the shelf for the moment. Maybe just put a TODO comment to the

>>> effect that we could consider renaming in future ?

>>

>> I wasn't suggesting renaming the cross-target variable.

>>

>> I was suggesting shuffling around the current names, and using "pc_next" for

>> what it sounds like -- the pc of the next insn.

> 

> Oh, I see, so incrementing base->pc_next after we load

> the insn? Yeah, that would work too. Though it seems a bit

> odd to me to have the target-specific code modifying

> a field in the base struct -- that seems like it ought to

> be purely for the generic TCG code to use.


Part of the contract with translator.c is that base.pc_next is updated.  See
arm_post_translate_insn.

The difference would be to update early, immediately after reading the insn
word, before decode, instead of delaying the update until the end after decode.


r~
diff mbox series

Patch

diff --git a/target/arm/translate.h b/target/arm/translate.h
index a20f6e2056..2dfdd8ca66 100644
--- a/target/arm/translate.h
+++ b/target/arm/translate.h
@@ -9,7 +9,17 @@  typedef struct DisasContext {
     DisasContextBase base;
     const ARMISARegisters *isar;
 
+    /*
+     * Summary of the various values for "PC":
+     * base.pc_next -- the start of the current insn
+     * pc           -- the start of the next insn
+     * pc_read      -- the value for "PC" in the ARM ARM;
+     *                 in arm mode, the current insn + 8;
+     *                 in thumb mode, the current insn + 4;
+     *                 in aa64 mode, unused.
+     */
     target_ulong pc;
+    target_ulong pc_read;
     target_ulong page_start;
     uint32_t insn;
     /* Nonzero if this instruction has been conditionally skipped.  */
diff --git a/target/arm/translate.c b/target/arm/translate.c
index 595385e1b1..a48e9a90f8 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -200,13 +200,7 @@  static inline void store_cpu_offset(TCGv_i32 var, int offset)
 static void load_reg_var(DisasContext *s, TCGv_i32 var, int reg)
 {
     if (reg == 15) {
-        uint32_t addr;
-        /* normally, since we updated PC, we need only to add one insn */
-        if (s->thumb)
-            addr = (long)s->pc + 2;
-        else
-            addr = (long)s->pc + 4;
-        tcg_gen_movi_i32(var, addr);
+        tcg_gen_movi_i32(var, s->pc_read);
     } else {
         tcg_gen_mov_i32(var, cpu_R[reg]);
     }
@@ -7868,16 +7862,14 @@  static void disas_arm_insn(DisasContext *s, unsigned int insn)
             /* branch link and change to thumb (blx <offset>) */
             int32_t offset;
 
-            val = (uint32_t)s->pc;
             tmp = tcg_temp_new_i32();
-            tcg_gen_movi_i32(tmp, val);
+            tcg_gen_movi_i32(tmp, s->pc);
             store_reg(s, 14, tmp);
             /* Sign-extend the 24-bit offset */
             offset = (((int32_t)insn) << 8) >> 8;
+            val = s->pc_read;
             /* offset * 4 + bit24 * 2 + (thumb bit) */
             val += (offset << 2) | ((insn >> 23) & 2) | 1;
-            /* pipeline offset */
-            val += 4;
             /* protected by ARCH(5); above, near the start of uncond block */
             gen_bx_im(s, val);
             return;
@@ -9154,9 +9146,8 @@  static void disas_arm_insn(DisasContext *s, unsigned int insn)
                             /* store */
                             if (i == 15) {
                                 /* special case: r15 = PC + 8 */
-                                val = (long)s->pc + 4;
                                 tmp = tcg_temp_new_i32();
-                                tcg_gen_movi_i32(tmp, val);
+                                tcg_gen_movi_i32(tmp, s->pc_read);
                             } else if (user) {
                                 tmp = tcg_temp_new_i32();
                                 tmp2 = tcg_const_i32(i);
@@ -9222,14 +9213,13 @@  static void disas_arm_insn(DisasContext *s, unsigned int insn)
                 int32_t offset;
 
                 /* branch (and link) */
-                val = (int32_t)s->pc;
                 if (insn & (1 << 24)) {
                     tmp = tcg_temp_new_i32();
-                    tcg_gen_movi_i32(tmp, val);
+                    tcg_gen_movi_i32(tmp, s->pc);
                     store_reg(s, 14, tmp);
                 }
                 offset = sextract32(insn << 2, 0, 26);
-                val += offset + 4;
+                val = s->pc_read + offset;
                 gen_jmp(s, val);
             }
             break;
@@ -9484,7 +9474,7 @@  static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
                         goto illegal_op;
                     }
                     addr = tcg_temp_new_i32();
-                    tcg_gen_movi_i32(addr, s->pc & ~3);
+                    tcg_gen_movi_i32(addr, s->pc_read & ~3);
                 } else {
                     addr = load_reg(s, rn);
                 }
@@ -9590,7 +9580,7 @@  static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
                 /* Table Branch.  */
                 if (rn == 15) {
                     addr = tcg_temp_new_i32();
-                    tcg_gen_movi_i32(addr, s->pc);
+                    tcg_gen_movi_i32(addr, s->pc_read);
                 } else {
                     addr = load_reg(s, rn);
                 }
@@ -9609,7 +9599,7 @@  static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
                 }
                 tcg_temp_free_i32(addr);
                 tcg_gen_shli_i32(tmp, tmp, 1);
-                tcg_gen_addi_i32(tmp, tmp, s->pc);
+                tcg_gen_addi_i32(tmp, tmp, s->pc_read);
                 store_reg(s, 15, tmp);
             } else {
                 bool is_lasr = false;
@@ -10342,7 +10332,7 @@  static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
                     tcg_gen_movi_i32(cpu_R[14], s->pc | 1);
                 }
 
-                offset += s->pc;
+                offset += s->pc_read;
                 if (insn & (1 << 12)) {
                     /* b/bl */
                     gen_jmp(s, offset);
@@ -10583,7 +10573,7 @@  static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
                 offset |= (insn & (1 << 11)) << 8;
 
                 /* jump to the offset */
-                gen_jmp(s, s->pc + offset);
+                gen_jmp(s, s->pc_read + offset);
             }
         } else {
             /*
@@ -10694,7 +10684,7 @@  static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
                     } else {
                         /* Add/sub 12-bit immediate.  */
                         if (rn == 15) {
-                            offset = s->pc & ~(uint32_t)3;
+                            offset = s->pc_read & ~(uint32_t)3;
                             if (insn & (1 << 23))
                                 offset -= imm;
                             else
@@ -10830,8 +10820,7 @@  static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
         if (rn == 15) {
             addr = tcg_temp_new_i32();
             /* PC relative.  */
-            /* s->pc has already been incremented by 4.  */
-            imm = s->pc & 0xfffffffc;
+            imm = s->pc_read & 0xfffffffc;
             if (insn & (1 << 23))
                 imm += insn & 0xfff;
             else
@@ -11077,7 +11066,7 @@  static void disas_thumb_insn(DisasContext *s, uint32_t insn)
         if (insn & (1 << 11)) {
             rd = (insn >> 8) & 7;
             /* load pc-relative.  Bit 1 of PC is ignored.  */
-            val = s->pc + 2 + ((insn & 0xff) * 4);
+            val = s->pc_read + ((insn & 0xff) * 4);
             val &= ~(uint32_t)2;
             addr = tcg_temp_new_i32();
             tcg_gen_movi_i32(addr, val);
@@ -11464,7 +11453,7 @@  static void disas_thumb_insn(DisasContext *s, uint32_t insn)
         } else {
             /* PC. bit 1 is ignored.  */
             tmp = tcg_temp_new_i32();
-            tcg_gen_movi_i32(tmp, (s->pc + 2) & ~(uint32_t)2);
+            tcg_gen_movi_i32(tmp, s->pc_read & ~(uint32_t)2);
         }
         val = (insn & 0xff) * 4;
         tcg_gen_addi_i32(tmp, tmp, val);
@@ -11584,7 +11573,7 @@  static void disas_thumb_insn(DisasContext *s, uint32_t insn)
                 tcg_gen_brcondi_i32(TCG_COND_NE, tmp, 0, s->condlabel);
             tcg_temp_free_i32(tmp);
             offset = ((insn & 0xf8) >> 2) | (insn & 0x200) >> 3;
-            val = (uint32_t)s->pc + 2;
+            val = s->pc_read;
             val += offset;
             gen_jmp(s, val);
             break;
@@ -11750,7 +11739,7 @@  static void disas_thumb_insn(DisasContext *s, uint32_t insn)
         arm_skip_unless(s, cond);
 
         /* jump to the offset */
-        val = (uint32_t)s->pc + 2;
+        val = s->pc_read;
         offset = ((int32_t)insn << 24) >> 24;
         val += offset << 1;
         gen_jmp(s, val);
@@ -11776,9 +11765,9 @@  static void disas_thumb_insn(DisasContext *s, uint32_t insn)
             break;
         }
         /* unconditional branch */
-        val = (uint32_t)s->pc;
+        val = s->pc_read;
         offset = ((int32_t)insn << 21) >> 21;
-        val += (offset << 1) + 2;
+        val += offset << 1;
         gen_jmp(s, val);
         break;
 
@@ -11802,7 +11791,7 @@  static void disas_thumb_insn(DisasContext *s, uint32_t insn)
             /* 0b1111_0xxx_xxxx_xxxx : BL/BLX prefix */
             uint32_t uoffset = ((int32_t)insn << 21) >> 9;
 
-            tcg_gen_movi_i32(cpu_R[14], s->pc + 2 + uoffset);
+            tcg_gen_movi_i32(cpu_R[14], s->pc_read + uoffset);
         }
         break;
     }
@@ -12055,6 +12044,7 @@  static void arm_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
 
     insn = arm_ldl_code(env, dc->pc, dc->sctlr_b);
     dc->insn = insn;
+    dc->pc_read = dc->pc + 8;
     dc->pc += 4;
     disas_arm_insn(dc, insn);
 
@@ -12123,6 +12113,7 @@  static void thumb_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
 
     insn = arm_lduw_code(env, dc->pc, dc->sctlr_b);
     is_16bit = thumb_insn_is_16bit(dc, insn);
+    dc->pc_read = dc->pc + 4;
     dc->pc += 2;
     if (!is_16bit) {
         uint32_t insn2 = arm_lduw_code(env, dc->pc, dc->sctlr_b);