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

[Pavel Dovgalyuk]

> From: Paolo Bonzini [mailto:address@hidden
> On 03/11/2017 09:27, Pavel Dovgalyuk wrote:
> >> 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 why is this a problem?  The TB would exit immediately and go again
> to cpu_handle_interrupt.  cpu_handle_interrupt returns true and
> cpu_handle_exception causes the exception via cpu_exec_nocache.

I've tested your variant more thoroughly.
It seems, that iothread calls cpu_exec between 
atomic_set(&cpu->icount_decr.u16.high, 0); 
in cpu_handle_interrupt and cpu_exec_nocache in cpu_handle_exception.
I see no other reason, because this happens not for the every time.
And cpu_handle_interrupt is not called again, because cpu_handle_exception 
returns true.
Therefore we have an infinite loop, because no other code here resets 
cpu->icount_decr.u16.high.

Pavel Dovgalyuk