[Qemu-devel] [RFC PATCH] target/arm: ensure eret exits the run-loop

[Qemu-devel] [RFC PATCH] target/arm: ensure eret exits the run-loop

[Alex Bennée]

Previously DISAS_JUMP did ensure this but with the optimisation of
8a6b28c7 (optimize indirect branches) we might not leave the loop.
This means if any pending interrupts are cleared by changing IRQ flags
we might never get around to servicing them. You usually notice this
by seeing the lookup_tb_ptr() helper gainfully chaining TBs together
while cpu->interrupt_request remains high and the exit_request has not
been set.

This breaks amongst other things the OPTEE test suite which executes
an eret from the secure world after a non-secure world IRQ has gone
pending which then never gets serviced.

An alternate approach might be for the exception helpers to ensure the
exit request flag is set if an IRQ is now unmasked.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
CC: Etienne Carriere <etienne.carriere@linaro.org>
CC: Joakim Bech <joakim.bech@linaro.org>
CC: Peter Maydell <peter.maydell@linaro.org>
CC: Emilio G. Cota <cota@braap.org>
CC: Richard Henderson <rth@twiddle.net>
---
 target/arm/translate-a64.c | 3 ++-
 target/arm/translate.c     | 6 ++++--
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index e55547d95d..3ee88b2590 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -1788,7 +1788,8 @@ static void disas_uncond_b_reg(DisasContext *s, uint32_t insn)
             return;
         }
         gen_helper_exception_return(cpu_env);
-        s->is_jmp = DISAS_JUMP;
+        /* Must exit loop to check un-masked IRQs */
+        s->is_jmp = DISAS_EXIT;
         return;
     case 5: /* DRPS */
         if (rn != 0x1f) {
diff --git a/target/arm/translate.c b/target/arm/translate.c
index 0862f9e4aa..920fb41395 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -4475,7 +4475,8 @@ static void gen_rfe(DisasContext *s, TCGv_i32 pc, TCGv_i32 cpsr)
      */
     gen_helper_cpsr_write_eret(cpu_env, cpsr);
     tcg_temp_free_i32(cpsr);
-    s->is_jmp = DISAS_JUMP;
+    /* Must exit loop to check un-masked IRQs */
+    s->is_jmp = DISAS_EXIT;
 }
 
 /* Generate an old-style exception return. Marks pc as dead. */
@@ -9519,7 +9520,8 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn)
                     tmp = load_cpu_field(spsr);
                     gen_helper_cpsr_write_eret(cpu_env, tmp);
                     tcg_temp_free_i32(tmp);
-                    s->is_jmp = DISAS_JUMP;
+                    /* Must exit loop to check un-masked IRQs */
+                    s->is_jmp = DISAS_EXIT;
                 }
             }
             break;
-- 
2.13.0

Re: [Qemu-devel] [RFC PATCH] target/arm: ensure eret exits the run-loop

[Alex Bennée]

Alex Bennée <alex.bennee@linaro.org> writes:

> Previously DISAS_JUMP did ensure this but with the optimisation of
> 8a6b28c7 (optimize indirect branches) we might not leave the loop.
> This means if any pending interrupts are cleared by changing IRQ flags
> we might never get around to servicing them. You usually notice this
> by seeing the lookup_tb_ptr() helper gainfully chaining TBs together
> while cpu->interrupt_request remains high and the exit_request has not
> been set.
>
> This breaks amongst other things the OPTEE test suite which executes
> an eret from the secure world after a non-secure world IRQ has gone
> pending which then never gets serviced.
>
> An alternate approach might be for the exception helpers to ensure the
> exit request flag is set if an IRQ is now unmasked.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> CC: Etienne Carriere <etienne.carriere@linaro.org>
> CC: Joakim Bech <joakim.bech@linaro.org>
> CC: Peter Maydell <peter.maydell@linaro.org>
> CC: Emilio G. Cota <cota@braap.org>
> CC: Richard Henderson <rth@twiddle.net>
> ---
>  target/arm/translate-a64.c | 3 ++-
>  target/arm/translate.c     | 6 ++++--
>  2 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
> index e55547d95d..3ee88b2590 100644
> --- a/target/arm/translate-a64.c
> +++ b/target/arm/translate-a64.c
> @@ -1788,7 +1788,8 @@ static void disas_uncond_b_reg(DisasContext *s, uint32_t insn)
>              return;
>          }
>          gen_helper_exception_return(cpu_env);
> -        s->is_jmp = DISAS_JUMP;
> +        /* Must exit loop to check un-masked IRQs */
> +        s->is_jmp = DISAS_EXIT;

Given the wording of:

/* is_jmp field values */
#define DISAS_NEXT    0 /* next instruction can be analyzed */
#define DISAS_JUMP    1 /* only pc was modified dynamically */
#define DISAS_UPDATE  2 /* cpu state was modified dynamically */
#define DISAS_TB_JUMP 3 /* only pc was modified statically */

I'm thinking that really these DISAS_JUMP's should be DISAS_UPDATEs and
we need to disable the TB chaining optimisations for this. I'll prepare
a more comprehensive series next week. However testing this patch for
known failure modes will be useful.


>          return;
>      case 5: /* DRPS */
>          if (rn != 0x1f) {
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index 0862f9e4aa..920fb41395 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -4475,7 +4475,8 @@ static void gen_rfe(DisasContext *s, TCGv_i32 pc, TCGv_i32 cpsr)
>       */
>      gen_helper_cpsr_write_eret(cpu_env, cpsr);
>      tcg_temp_free_i32(cpsr);
> -    s->is_jmp = DISAS_JUMP;
> +    /* Must exit loop to check un-masked IRQs */
> +    s->is_jmp = DISAS_EXIT;
>  }
>
>  /* Generate an old-style exception return. Marks pc as dead. */
> @@ -9519,7 +9520,8 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn)
>                      tmp = load_cpu_field(spsr);
>                      gen_helper_cpsr_write_eret(cpu_env, tmp);
>                      tcg_temp_free_i32(tmp);
> -                    s->is_jmp = DISAS_JUMP;
> +                    /* Must exit loop to check un-masked IRQs */
> +                    s->is_jmp = DISAS_EXIT;
>                  }
>              }
>              break;


--
Alex Bennée

Re: [Qemu-devel] [RFC PATCH] target/arm: ensure eret exits the run-loop

[Peter Maydell]

On 7 July 2017 at 18:32, Alex Bennée <alex.bennee@linaro.org> wrote:
> Given the wording of:
>
> /* is_jmp field values */
> #define DISAS_NEXT    0 /* next instruction can be analyzed */
> #define DISAS_JUMP    1 /* only pc was modified dynamically */
> #define DISAS_UPDATE  2 /* cpu state was modified dynamically */
> #define DISAS_TB_JUMP 3 /* only pc was modified statically */
>
> I'm thinking that really these DISAS_JUMP's should be DISAS_UPDATEs and
> we need to disable the TB chaining optimisations for this. I'll prepare
> a more comprehensive series next week. However testing this patch for
> known failure modes will be useful.

The problem I think is that a lot of our current code assumes
that DISAS_UPDATE means "go back to the top loop".

In any case I think you are right that DISAS_UPDATE should not
allow chaining to the next TB, because it implies that perhaps
a TB flags bit got changed, so you need to look in the hash
table again.

thanks
-- PMM

Re: [Qemu-devel] [RFC PATCH] target/arm: ensure eret exits the run-loop

[Richard Henderson]

On 07/07/2017 07:36 AM, Peter Maydell wrote:
> On 7 July 2017 at 18:32, Alex Bennée <alex.bennee@linaro.org> wrote:
>> Given the wording of:
>>
>> /* is_jmp field values */
>> #define DISAS_NEXT    0 /* next instruction can be analyzed */
>> #define DISAS_JUMP    1 /* only pc was modified dynamically */
>> #define DISAS_UPDATE  2 /* cpu state was modified dynamically */
>> #define DISAS_TB_JUMP 3 /* only pc was modified statically */
>>
>> I'm thinking that really these DISAS_JUMP's should be DISAS_UPDATEs and
>> we need to disable the TB chaining optimisations for this. I'll prepare
>> a more comprehensive series next week. However testing this patch for
>> known failure modes will be useful.
> 
> The problem I think is that a lot of our current code assumes
> that DISAS_UPDATE means "go back to the top loop".
> 
> In any case I think you are right that DISAS_UPDATE should not
> allow chaining to the next TB, because it implies that perhaps
> a TB flags bit got changed, so you need to look in the hash
> table again.

Changes to TB flags are not a problem; helper_lookup_tb does examine them, and 
indeed go back to the hash table.  The only thing it doesn't do is re-evaluate 
the interrupt state.

Naming all of these different exit conditions is certainly non-trivial.


r~

Re: [Qemu-devel] [RFC PATCH] target/arm: ensure eret exits the run-loop

[Alex Bennée]

Richard Henderson <rth@twiddle.net> writes:

> On 07/07/2017 07:36 AM, Peter Maydell wrote:
>> On 7 July 2017 at 18:32, Alex Bennée <alex.bennee@linaro.org> wrote:
>>> Given the wording of:
>>>
>>> /* is_jmp field values */
>>> #define DISAS_NEXT    0 /* next instruction can be analyzed */
>>> #define DISAS_JUMP    1 /* only pc was modified dynamically */
>>> #define DISAS_UPDATE  2 /* cpu state was modified dynamically */
>>> #define DISAS_TB_JUMP 3 /* only pc was modified statically */
>>>
>>> I'm thinking that really these DISAS_JUMP's should be DISAS_UPDATEs and
>>> we need to disable the TB chaining optimisations for this. I'll prepare
>>> a more comprehensive series next week. However testing this patch for
>>> known failure modes will be useful.
>>
>> The problem I think is that a lot of our current code assumes
>> that DISAS_UPDATE means "go back to the top loop".
>>
>> In any case I think you are right that DISAS_UPDATE should not
>> allow chaining to the next TB, because it implies that perhaps
>> a TB flags bit got changed, so you need to look in the hash
>> table again.
>
> Changes to TB flags are not a problem; helper_lookup_tb does examine
> them, and indeed go back to the hash table.  The only thing it doesn't
> do is re-evaluate the interrupt state.

Last time around we I did have a patch that checked for
cpu->interrupt_request in the lookup helper. Of course this is a bit
blunt because really its a case of is the cpu->interrupt_request live
and the masking has changed to enable things to be run. Unfortunately
the current API of cc->cpu_exec_interrupt() isn't amenable to querying
the state so maybe we should just exit back to the prologue when the
request is up.

> Naming all of these different exit conditions is certainly
> non-trivial.

Given the variation of usage this is something that should probably be
done after Lluís common run loop goes in and we can beef up the
semantics of the various exit conditions.

One thing I have noticed in the ARM translator is DISAS_UPDATE does a:

    gen_a64_set_pc_im(dc->pc);

I think this is to deal with handling exceptions either side of various
instructions. Am I right in thinking this is superfluous now as we can
derive the PC from the translated code address?


>
>
> r~


--
Alex Bennée

Re: [Qemu-devel] [RFC PATCH] target/arm: ensure eret exits the run-loop

[Richard Henderson]

On 07/07/2017 08:29 AM, Alex Bennée wrote:
>> Naming all of these different exit conditions is certainly
>> non-trivial.
> 
> Given the variation of usage this is something that should probably be
> done after Lluís common run loop goes in and we can beef up the
> semantics of the various exit conditions.

Definitely.

> One thing I have noticed in the ARM translator is DISAS_UPDATE does a:
> 
>      gen_a64_set_pc_im(dc->pc);
> 
> I think this is to deal with handling exceptions either side of various
> instructions. Am I right in thinking this is superfluous now as we can
> derive the PC from the translated code address?

Yes and no.

We have typically distinguished between two kinds of exceptions: those that are 
dynamic (fp state, page permissions) and those that are static (illegal opcodes).

For the dynamic, we used to pessimistically save state, but now use the 
unwinder to restore it.  The unwinding is expensive but is used infrequently 
(especially compared to the number of load/store insns executed).

For the static, we know the exception must be raised, and we know the state 
that must be saved.  By doing that, we save the expense of the unwinding.

So, for the static case you're talking about, we could get the PC (and other 
state) back, and remove the explicit stores, but we shouldn't.


r~

Re: [Qemu-devel] [Qemu-arm] [RFC PATCH] target/arm: ensure eret exits the run-loop

[Philippe Mathieu-Daudé]

Hi Alex,

On 07/07/2017 03:52 PM, Richard Henderson wrote:
> On 07/07/2017 08:29 AM, Alex Bennée wrote:
>>> Naming all of these different exit conditions is certainly
>>> non-trivial.
>>
>> Given the variation of usage this is something that should probably be
>> done after Lluís common run loop goes in and we can beef up the
>> semantics of the various exit conditions.
> 
> Definitely.
> 
>> One thing I have noticed in the ARM translator is DISAS_UPDATE does a:
>>
>>      gen_a64_set_pc_im(dc->pc);
>>
>> I think this is to deal with handling exceptions either side of various
>> instructions. Am I right in thinking this is superfluous now as we can
>> derive the PC from the translated code address?
> 
> Yes and no.
> 
> We have typically distinguished between two kinds of exceptions: those 
> that are dynamic (fp state, page permissions) and those that are static 
> (illegal opcodes).
> 
> For the dynamic, we used to pessimistically save state, but now use the 
> unwinder to restore it.  The unwinding is expensive but is used 
> infrequently (especially compared to the number of load/store insns 
> executed).
> 
> For the static, we know the exception must be raised, and we know the 
> state that must be saved.  By doing that, we save the expense of the 
> unwinding.
> 
> So, for the static case you're talking about, we could get the PC (and 
> other state) back, and remove the explicit stores, but we shouldn't.

While doing changes in this file, can you add a comment around 
DISAS_UPDATE about Richard explanation?

> 
> 
> r~
> 

Re: [Qemu-devel] [RFC PATCH] target/arm: ensure eret exits the run-loop

[Alex Bennée]

Richard Henderson <rth@twiddle.net> writes:

> On 07/07/2017 08:29 AM, Alex Bennée wrote:
>>> Naming all of these different exit conditions is certainly
>>> non-trivial.
>>
>> Given the variation of usage this is something that should probably be
>> done after Lluís common run loop goes in and we can beef up the
>> semantics of the various exit conditions.
>
> Definitely.
>
>> One thing I have noticed in the ARM translator is DISAS_UPDATE does a:
>>
>>      gen_a64_set_pc_im(dc->pc);
>>
>> I think this is to deal with handling exceptions either side of various
>> instructions. Am I right in thinking this is superfluous now as we can
>> derive the PC from the translated code address?
>
> Yes and no.
>
> We have typically distinguished between two kinds of exceptions: those
> that are dynamic (fp state, page permissions) and those that are
> static (illegal opcodes).
>
> For the dynamic, we used to pessimistically save state, but now use
> the unwinder to restore it.  The unwinding is expensive but is used
> infrequently (especially compared to the number of load/store insns
> executed).
>
> For the static, we know the exception must be raised, and we know the
> state that must be saved.  By doing that, we save the expense of the
> unwinding.
>
> So, for the static case you're talking about, we could get the PC (and
> other state) back, and remove the explicit stores, but we shouldn't.

I'm thinking of how to bring this into line with the other translators.
Pretty much everyone else generates an exit block although m68k does do
a update_cc_op(dc); In the ARM translator we have the backend specific
DISAS_EXIT which behaves like DISAS_UPDATE everywhere else.

For consistency DISAS_UPDATE should work the same across all arches but
doing:

  gen_set_pc_im(dc, dc->pc);

if we are going to use DISAS_UPDATE instead of DISAS_EXIT is going to
break things like eret as dc->pc will certainly not be the correct PC.
Looking at translate.c (32 bit arm), we have:

  gen_srs
  gen_mrs_banked
  gen_msr_banked

These all manually set:

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

before their respective helpers. I think setting the PC after the helper
is superfluous given we are will at that point be exiting the block. The
situation is the same for translate-a64.c. So I think the correct
changes are:

  - strengthen commentary in exec-all.h
  - don't set pc on our way out of a DISAS_UPDATE
  - convert eret to DISAS_UPDATE
  - get rid of DISAS_EXIT and use DISAS_UPDATE instead

My only worry is inadvertently breaking something because something does
need the post-foo set_pc. But I can't think what does. Peter?


--
Alex Bennée

Re: [Qemu-devel] [RFC PATCH] target/arm: ensure eret exits the run-loop

[Peter Maydell]

On 10 July 2017 at 13:15, Alex Bennée <alex.bennee@linaro.org> wrote:
> Looking at translate.c (32 bit arm), we have:
>
>   gen_srs
>   gen_mrs_banked
>   gen_msr_banked
>
> These all manually set:
>
>   gen_set_pc_im(s, s->pc - 4);
>
> before their respective helpers. I think setting the PC after the helper
> is superfluous given we are will at that point be exiting the block.

No, you need both. We do
  gen_set_pc_im(s, s->pc - 4);
before calling the helper because the helper might throw an
exception, in which case the PC needs to point to that insn.
We then call
 gen_set_pc_im(dc, dc->pc);
before exiting the block because if we leave execution by
falling off the end of the block then the PC should point to
the insn that comes next (ie just after the last one in the block)

thanks
-- PMM

Re: [Qemu-devel] [RFC PATCH] target/arm: ensure eret exits the run-loop

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 10 July 2017 at 13:15, Alex Bennée <alex.bennee@linaro.org> wrote:
>> Looking at translate.c (32 bit arm), we have:
>>
>>   gen_srs
>>   gen_mrs_banked
>>   gen_msr_banked
>>
>> These all manually set:
>>
>>   gen_set_pc_im(s, s->pc - 4);
>>
>> before their respective helpers. I think setting the PC after the helper
>> is superfluous given we are will at that point be exiting the block.
>
> No, you need both. We do
>   gen_set_pc_im(s, s->pc - 4);
> before calling the helper because the helper might throw an
> exception, in which case the PC needs to point to that insn.
> We then call
>  gen_set_pc_im(dc, dc->pc);
> before exiting the block because if we leave execution by
> falling off the end of the block then the PC should point to
> the insn that comes next (ie just after the last one in the block)

OK I see now. I think I have a plan going forward.

>
> thanks
> -- PMM


--
Alex Bennée