On 8 Oct 2013, at 17:56, Paolo Bonzini wrote: >> Arguably the patch could document why removing the check for deadline > >> INT32_MAX >> (the bug for bug compatibility) is safe, as I couldn't entirely convince >> myself it >> was, mostly because I couldn't see why it was doing it in the first place. > > I couldn't convince myself that it is _not_ safe :) and it made the code > more complicated. As soon as a deadline appears, qemu_clock_warp() will > be called again and update the icount_warp_timer. > > Ok to move that to a separate patch?
To be honest I put it in out of an abundance of caution. I am very tempted to take it out and see what breaks. As far as I can see all the time arithmetic is not int64_t; perhaps this was not always the case. I was more checking you hadn't removed it by accident. Perhaps just add "special casing deadlines > INT32_MAX removed as all arithmetic now 64 bit". There is another offender in tcg_cpu_exec. deadline = qemu_clock_deadline_ns_all(QEMU_CLOCK_VIRTUAL); /* Maintain prior (possibly buggy) behaviour where if no deadline * was set (as there is no QEMU_CLOCK_VIRTUAL timer) or it is more than * INT32_MAX nanoseconds ahead, we still use INT32_MAX * nanoseconds. */ if ((deadline < 0) || (deadline > INT32_MAX)) { deadline = INT32_MAX; } count = qemu_icount_round(deadline); qemu_icount += count; decr = (count > 0xffff) ? 0xffff : count; count -= decr; env->icount_decr.u16.low = decr; env->icount_extra = count; This implies that qemu_icount_round() cannot take a 64 bit int. static int64_t qemu_icount_round(int64_t count) { return (count + (1 << icount_time_shift) - 1) >> icount_time_shift; } I would have thought it better if qemu_icount_round just dealt sensibly with negative parameters. -- Alex Bligh