[v9,23/25] target-arm: introduce ARM_CP_EXIT_PC

Message ID 20170201150553.9381-24-alex.bennee@linaro.org
State New
Headers show
Series
  • MTTCG Base enabling patches with ARM enablement
Related show

Commit Message

Alex Bennée Feb. 1, 2017, 3:05 p.m.
Some helpers may trigger an immediate exit of the cpu_loop. If this
happens the PC need to be rectified to ensure the restart will begin
on the next instruction.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
 target/arm/cpu.h           | 3 ++-
 target/arm/translate-a64.c | 4 ++++
 target/arm/translate.c     | 4 ++++
 3 files changed, 10 insertions(+), 1 deletion(-)

-- 
2.11.0

Comments

Peter Maydell Feb. 3, 2017, 11:22 a.m. | #1
On 1 February 2017 at 15:05, Alex Bennée <alex.bennee@linaro.org> wrote:
> Some helpers may trigger an immediate exit of the cpu_loop. If this

> happens the PC need to be rectified to ensure the restart will begin

> on the next instruction.

>

> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

> ---

>  target/arm/cpu.h           | 3 ++-

>  target/arm/translate-a64.c | 4 ++++

>  target/arm/translate.c     | 4 ++++

>  3 files changed, 10 insertions(+), 1 deletion(-)

>

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

> index d61793ca06..a3c4d07817 100644

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

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

> @@ -1465,7 +1465,8 @@ static inline uint64_t cpreg_to_kvm_id(uint32_t cpregid)

>  #define ARM_CP_NZCV            (ARM_CP_SPECIAL | (3 << 8))

>  #define ARM_CP_CURRENTEL       (ARM_CP_SPECIAL | (4 << 8))

>  #define ARM_CP_DC_ZVA          (ARM_CP_SPECIAL | (5 << 8))

> -#define ARM_LAST_SPECIAL       ARM_CP_DC_ZVA

> +#define ARM_CP_EXIT_PC         (ARM_CP_SPECIAL | (6 << 8))

> +#define ARM_LAST_SPECIAL       ARM_CP_EXIT_PC


This shouldn't be a "special", because those are for
"this is a special case that is handled entirely in the translate
code", not "I need some extra behaviour on the code generated
for calling the helper functions" (which is what the
plain non-special ARM_CP flags do). Notice that all the other
"special" cases completely define the behaviour of the cp that
uses them, and the code implementing them ends the case
statement with "return", not "break".

Missing documentation comment change.

That said, I'm definitely becoming more strongly of the
opinion that longjumping out of this helper is not the
best way to implement this, so these remarks are a bit moot.

>  /* Used only as a terminator for ARMCPRegInfo lists */

>  #define ARM_CP_SENTINEL 0xffff

>  /* Mask of only the flag bits in a type field */

> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c

> index 7e7131fe2f..98d4fac070 100644

> --- a/target/arm/translate-a64.c

> +++ b/target/arm/translate-a64.c

> @@ -1561,6 +1561,10 @@ static void handle_sys(DisasContext *s, uint32_t insn, bool isread,

>          tcg_rt = cpu_reg(s, rt);

>          gen_helper_dc_zva(cpu_env, tcg_rt);

>          return;

> +    case ARM_CP_EXIT_PC:

> +        /* The helper may exit the cpu_loop so ensure PC is correct */

> +        gen_a64_set_pc_im(s->pc);

> +        break;

>      default:

>          break;

>      }

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

> index 24faa7c60c..e1f4a48720 100644

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

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

> @@ -7510,6 +7510,10 @@ static int disas_coproc_insn(DisasContext *s, uint32_t insn)

>              gen_set_pc_im(s, s->pc);

>              s->is_jmp = DISAS_WFI;

>              return 0;

> +        case ARM_CP_EXIT_PC:

> +            /* The helper may exit the cpu_loop so ensure PC is correct */

> +            gen_set_pc_im(s, s->pc);

> +            break;

>          default:

>              break;

>          }


thanks
-- PMM
Alex Bennée Feb. 3, 2017, 11:33 a.m. | #2
Peter Maydell <peter.maydell@linaro.org> writes:

> On 1 February 2017 at 15:05, Alex Bennée <alex.bennee@linaro.org> wrote:

>> Some helpers may trigger an immediate exit of the cpu_loop. If this

>> happens the PC need to be rectified to ensure the restart will begin

>> on the next instruction.

>>

>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

>> ---

>>  target/arm/cpu.h           | 3 ++-

>>  target/arm/translate-a64.c | 4 ++++

>>  target/arm/translate.c     | 4 ++++

>>  3 files changed, 10 insertions(+), 1 deletion(-)

>>

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

>> index d61793ca06..a3c4d07817 100644

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

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

>> @@ -1465,7 +1465,8 @@ static inline uint64_t cpreg_to_kvm_id(uint32_t cpregid)

>>  #define ARM_CP_NZCV            (ARM_CP_SPECIAL | (3 << 8))

>>  #define ARM_CP_CURRENTEL       (ARM_CP_SPECIAL | (4 << 8))

>>  #define ARM_CP_DC_ZVA          (ARM_CP_SPECIAL | (5 << 8))

>> -#define ARM_LAST_SPECIAL       ARM_CP_DC_ZVA

>> +#define ARM_CP_EXIT_PC         (ARM_CP_SPECIAL | (6 << 8))

>> +#define ARM_LAST_SPECIAL       ARM_CP_EXIT_PC

>

> This shouldn't be a "special", because those are for

> "this is a special case that is handled entirely in the translate

> code", not "I need some extra behaviour on the code generated

> for calling the helper functions" (which is what the

> plain non-special ARM_CP flags do). Notice that all the other

> "special" cases completely define the behaviour of the cp that

> uses them, and the code implementing them ends the case

> statement with "return", not "break".

>

> Missing documentation comment change.


I posted this before you commented on the last version. Anyway see
bellow.

>

> That said, I'm definitely becoming more strongly of the

> opinion that longjumping out of this helper is not the

> best way to implement this, so these remarks are a bit moot.


Yep the tree:

  https://github.com/stsquad/qemu/commits/mttcg/base-patches-v10

Reverts the this change and changes the cputlb flush code to return and
let the guest translation code exit the normal way. I was hoping to get
some feedback from Paolo and Richard before I roll the fixes together
and post v10 which will be later today.

>

>>  /* Used only as a terminator for ARMCPRegInfo lists */

>>  #define ARM_CP_SENTINEL 0xffff

>>  /* Mask of only the flag bits in a type field */

>> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c

>> index 7e7131fe2f..98d4fac070 100644

>> --- a/target/arm/translate-a64.c

>> +++ b/target/arm/translate-a64.c

>> @@ -1561,6 +1561,10 @@ static void handle_sys(DisasContext *s, uint32_t insn, bool isread,

>>          tcg_rt = cpu_reg(s, rt);

>>          gen_helper_dc_zva(cpu_env, tcg_rt);

>>          return;

>> +    case ARM_CP_EXIT_PC:

>> +        /* The helper may exit the cpu_loop so ensure PC is correct */

>> +        gen_a64_set_pc_im(s->pc);

>> +        break;

>>      default:

>>          break;

>>      }

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

>> index 24faa7c60c..e1f4a48720 100644

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

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

>> @@ -7510,6 +7510,10 @@ static int disas_coproc_insn(DisasContext *s, uint32_t insn)

>>              gen_set_pc_im(s, s->pc);

>>              s->is_jmp = DISAS_WFI;

>>              return 0;

>> +        case ARM_CP_EXIT_PC:

>> +            /* The helper may exit the cpu_loop so ensure PC is correct */

>> +            gen_set_pc_im(s, s->pc);

>> +            break;

>>          default:

>>              break;

>>          }

>

> thanks

> -- PMM



--
Alex Bennée

Patch

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index d61793ca06..a3c4d07817 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1465,7 +1465,8 @@  static inline uint64_t cpreg_to_kvm_id(uint32_t cpregid)
 #define ARM_CP_NZCV            (ARM_CP_SPECIAL | (3 << 8))
 #define ARM_CP_CURRENTEL       (ARM_CP_SPECIAL | (4 << 8))
 #define ARM_CP_DC_ZVA          (ARM_CP_SPECIAL | (5 << 8))
-#define ARM_LAST_SPECIAL       ARM_CP_DC_ZVA
+#define ARM_CP_EXIT_PC         (ARM_CP_SPECIAL | (6 << 8))
+#define ARM_LAST_SPECIAL       ARM_CP_EXIT_PC
 /* Used only as a terminator for ARMCPRegInfo lists */
 #define ARM_CP_SENTINEL 0xffff
 /* Mask of only the flag bits in a type field */
diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index 7e7131fe2f..98d4fac070 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -1561,6 +1561,10 @@  static void handle_sys(DisasContext *s, uint32_t insn, bool isread,
         tcg_rt = cpu_reg(s, rt);
         gen_helper_dc_zva(cpu_env, tcg_rt);
         return;
+    case ARM_CP_EXIT_PC:
+        /* The helper may exit the cpu_loop so ensure PC is correct */
+        gen_a64_set_pc_im(s->pc);
+        break;
     default:
         break;
     }
diff --git a/target/arm/translate.c b/target/arm/translate.c
index 24faa7c60c..e1f4a48720 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -7510,6 +7510,10 @@  static int disas_coproc_insn(DisasContext *s, uint32_t insn)
             gen_set_pc_im(s, s->pc);
             s->is_jmp = DISAS_WFI;
             return 0;
+        case ARM_CP_EXIT_PC:
+            /* The helper may exit the cpu_loop so ensure PC is correct */
+            gen_set_pc_im(s, s->pc);
+            break;
         default:
             break;
         }