diff mbox

[v1,3/3] tcg-runtime: short-circuit lookup_tb_ptr on IRQs

Message ID 871sqmo3xb.fsf@linaro.org
State New
Headers show

Commit Message

Alex Bennée June 14, 2017, 5:49 p.m. UTC 
Paolo Bonzini <pbonzini@redhat.com> writes:

> On 14/06/2017 17:45, Richard Henderson wrote:
>> While the next TB would detect the exit flag has been set there is no
>> point if we can exit sooner. We also check cpu->interrupt_request as
>> some front-ends can set it rather than using the cpu_interrupt() API
>> call and would normally be expecting the IRQ to get picked up on the
>> previously fairly regular exits from the run loop.
>
> This is not what happens actually; it's not about front-ends setting
> cpu->interrupt_request, it's about front-ends doing exit_tb when they
> wanted to re-evaluate cpu_handle_interrupt.
>
> cpu_exit is used when device code causes a rising edge in
> cpu->interrupt_request.  What we have here is that the MSR write causes
> cc->cpu_exec_interrupt's return value to change from false to true.
>
> I think this is a band-aid, and would rather fix the front-ends as in
> Emilio's patch.

It seems a shame to cause all msr accesses to trigger and exit when we
only care about the unmasking case. How about:


Author: Alex Bennée <alex.bennee@linaro.org>
Date:   Wed Jun 14 18:46:01 2017 +0100

    target/arm/op_helper: ensure we exit the run-loop

    When IRQs are un-masked we need to ensure the run-loop is exited so we
    can evaluate arm_cpu_do_interrupt.

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

Comments

Richard Henderson June 14, 2017, 5:53 p.m. UTC | #1 
On 06/14/2017 10:49 AM, Alex Bennée wrote:
>> I think this is a band-aid, and would rather fix the front-ends as in
>> Emilio's patch.
> 
> It seems a shame to cause all msr accesses to trigger and exit when we
> only care about the unmasking case. How about:
> 
> 
> Author: Alex Bennée <alex.bennee@linaro.org>
> Date:   Wed Jun 14 18:46:01 2017 +0100
> 
>      target/arm/op_helper: ensure we exit the run-loop
> 
>      When IRQs are un-masked we need to ensure the run-loop is exited so we
>      can evaluate arm_cpu_do_interrupt.
> 
>      Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> 
> diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
> index 2a85666579..7e67bb3db2 100644
> --- a/target/arm/op_helper.c
> +++ b/target/arm/op_helper.c
> @@ -835,6 +835,9 @@ void HELPER(msr_i_pstate)(CPUARMState *env, uint32_t op, uint32_t imm)
>           break;
>       case 0x1f: /* DAIFClear */
>           env->daif &= ~((imm << 6) & PSTATE_DAIF);
> +        /* This may result in pending IRQs being unmasked so ensure we
> +           exit the loop */
> +        cpu_exit(ENV_GET_CPU(env));

That works for me.  And I guess that takes care of any potential problems with 
A32 as well?


r~
Peter Maydell June 14, 2017, 7:11 p.m. UTC | #2 
On 14 June 2017 at 18:49, Alex Bennée <alex.bennee@linaro.org> wrote:
> diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
> index 2a85666579..7e67bb3db2 100644
> --- a/target/arm/op_helper.c
> +++ b/target/arm/op_helper.c
> @@ -835,6 +835,9 @@ void HELPER(msr_i_pstate)(CPUARMState *env, uint32_t op, uint32_t imm)
>          break;
>      case 0x1f: /* DAIFClear */
>          env->daif &= ~((imm << 6) & PSTATE_DAIF);
> +        /* This may result in pending IRQs being unmasked so ensure we
> +           exit the loop */
> +        cpu_exit(ENV_GET_CPU(env));
>          break;
>      default:
>          g_assert_not_reached();

The 'op' field we're switching on here is just a constant
from the instruction encoding, so I'd rather see us
identify that in translate-a64.c and end the TB or
whatever when we need to, rather than doing the
longjump-out-of-here that cpu_exit() does at runtime.

thanks
-- PMM
Richard Henderson June 14, 2017, 7:19 p.m. UTC | #3 
On 06/14/2017 12:11 PM, Peter Maydell wrote:
> On 14 June 2017 at 18:49, Alex Bennée <alex.bennee@linaro.org> wrote:
>> diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
>> index 2a85666579..7e67bb3db2 100644
>> --- a/target/arm/op_helper.c
>> +++ b/target/arm/op_helper.c
>> @@ -835,6 +835,9 @@ void HELPER(msr_i_pstate)(CPUARMState *env, uint32_t op, uint32_t imm)
>>           break;
>>       case 0x1f: /* DAIFClear */
>>           env->daif &= ~((imm << 6) & PSTATE_DAIF);
>> +        /* This may result in pending IRQs being unmasked so ensure we
>> +           exit the loop */
>> +        cpu_exit(ENV_GET_CPU(env));
>>           break;
>>       default:
>>           g_assert_not_reached();
> 
> The 'op' field we're switching on here is just a constant
> from the instruction encoding, so I'd rather see us
> identify that in translate-a64.c and end the TB or
> whatever when we need to, rather than doing the
> longjump-out-of-here that cpu_exit() does at runtime.

cpu_exit isn't the longjmp; this is just a set of exit_request and icount_decr.

That said, you're right that we can do this more directly from the translator.


r~
diff mbox

Patch

diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
index 2a85666579..7e67bb3db2 100644
--- a/target/arm/op_helper.c
+++ b/target/arm/op_helper.c
@@ -835,6 +835,9 @@  void HELPER(msr_i_pstate)(CPUARMState *env, uint32_t op, uint32_t imm)
         break;
     case 0x1f: /* DAIFClear */
         env->daif &= ~((imm << 6) & PSTATE_DAIF);
+        /* This may result in pending IRQs being unmasked so ensure we
+           exit the loop */
+        cpu_exit(ENV_GET_CPU(env));
         break;
     default:
         g_assert_not_reached();