diff mbox series

[06/67] target/arm: Introduce pc_read

Message ID 20190726175032.6769-7-richard.henderson@linaro.org (mailing list archive)
State New, archived
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(-)

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);