Re: [Qemu-devel] [RFC PATCH 19/26] cpu-exec: reset exit flag before calling cpu_exec_nocache

[Alex Bennée]

Pavel Dovgalyuk <address@hidden> writes:

>> From: Paolo Bonzini [mailto:address@hidden
>> On 02/11/2017 12:33, Paolo Bonzini wrote:
>> > On 02/11/2017 12:24, Pavel Dovgalyuk wrote:
>> >>> I am not sure about this.  I think if instead you should return false
>> >>> from here and EXCP_INTERRUPT from cpu_exec.
>> >> The problem is inside the TB. It checks cpu->icount_decr.u16.high which 
>> >> is -1.
>> >> And we have to enter the TB to cause an exception (because it exists in 
>> >> replay log).
>> >> That is why we reset this flag and try to execute the TB.
>> >
>> > But if u16.high is -1, shouldn't you return EXCP_INTERRUPT first (via
>> > "Finally, check if we need to exit to the main loop" in
>> > cpu_handle_interrupt)?  Then only cause the exception when that one is
>> > processed.
>>
>> ... indeed, you probably need something like:
>>
>>     /* Clear the interrupt flag now since we're processing
>>      * cpu->interrupt_request and cpu->exit_request.
>>      */
>>     insns_left = atomic_read(&cpu->icount_decr.u32);
>>     atomic_set(&cpu->icount_decr.u16.high, 0);
>>     if (unlikely(insns_left < 0) {
>>         /* Ensure the zeroing of icount_decr comes before the next read
>>          * of cpu->exit_request or cpu->interrupt_request.
>>          */
>>         smb_mb();
>>     }
>>
>> at the top of cpu_handle_interrupt.  Then you can remove the same
>> atomic_set+smp_mb in cpu_loop_exec_tb, like
>>
>>     *last_tb = NULL;
>>     insns_left = atomic_read(&cpu->icount_decr.u32);
>>     if (insns_left < 0) {
>>         /* Something asked us to stop executing chained TBs; just
>>          * continue round the main loop. Whatever requested the exit
>>          * will also have set something else (eg exit_request or
>>          * interrupt_request) which will be handled by
>>          * cpu_handle_interrupt.  cpu_handle_interrupt will also
>>          * clear cpu->icount_decr.u16.high.
>>          */
>>         return;
>>     }
>
> I tried this approach and it didn't work.
> I think iothread sets u16.high flag after resetting it in 
> cpu_handle_interrupt.
>
> But there is another possible approach: set new tcg flag, which disables 
> checking
> the exit_request at the entry to the TB.

I'm instinctively wary of adding additional flags as it complicates the
number of exit states - especially as we went to the trouble of merging
it all into one (wide) flag.

Where in the TB is this exception we need to execute? It can't be more
than one instruction can it - otherwise the icount would be higher. Is
this an off-by-one issue?

Would another approach be to say:

  - icount is 0
  - we have a pending exception not yet generated

therefor

  - translate a new, non-cached, non-icount/exit checking, single
    instruction block for the exception

and then assert if an exception wasn't raised?

>
>
> Pavel Dovgalyuk


--
Alex Bennée