Re: [Qemu-devel] [PATCH v8 2/9] icount: exit cpu loop on expire

["Pavel Dovgalyuk" <dovgaluk@ispras.ru>]

> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> On 26/01/2017 15:32, Pavel Dovgalyuk wrote:
> >> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> >> On 26/01/2017 14:37, Pavel Dovgalyuk wrote:
> >>>> Simpler:
> >>>>
> >>>> 	use_icount &&
> >>>> 	((int32_t)cpu->icount_decr.u32 < 0 ||
> >>>> 	 cpu->icount_decr.u16.low + cpu->icount_extra == 0)
> >>> Right.
> >>>
> >>>> But I'm not sure that you need to test u32.  After all you're not
> >>> Checking u32 is needed, because sometimes it is less than zero.
> >>
> >> If cpu->icount_decr.u32 is less than zero, the next translation block
> >> would immediately exit with TB_EXIT_ICOUNT_EXPIRED, causing
> >>
> >>             cpu->exception_index = EXCP_INTERRUPT;
> >>             *last_tb = NULL;	
> >>             cpu_loop_exit(cpu);
> >>
> >> from cpu_loop_exec_tb's "case TB_EXIT_ICOUNT_EXPIRED".
> >>
> >> And the same is true for cpu->icount_decr.u16.low + cpu->icount_extra ==
> >> 0, so I don't understand why this part of the patch is necessary.
> >
> > I removed that lines because we have to check icount=0 not only when it is expired,
> > but also when all instructions were executed successfully.
> > If there are no instructions to execute, calling tb_find (and translation then)
> > may cause an exception at the wrong moment.
> 
> Ok, that makes sense for cpu->icount_decr.u16.low + cpu->icount_extra == 0.
> 
> But for decr.u32 < 0, the same reasoning of this comment is also true:
> 
>         /* 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 we will handle
>          * next time around the loop.  But we need to
>          * ensure the tcg_exit_req read in generated code
>          * comes before the next read of cpu->exit_request
>          * or cpu->interrupt_request.
>          */

Right. If the following lines will not be removed (as opposite to my patch) then checking
decr.u32 < 0 will not be needed.
-             cpu->exception_index = EXCP_INTERRUPT;
-             *last_tb = NULL;	
-             cpu_loop_exit(cpu);

What is your point about the new version of that patch?

Pavel Dovgalyuk