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

[Alex Bennée]

Richard Henderson <address@hidden> writes:

> On 07/07/2017 07:36 AM, Peter Maydell wrote:
>> On 7 July 2017 at 18:32, Alex Bennée <address@hidden> 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