Re: [Qemu-devel] [PATCH 5/8] timers: prepare the code for future races in calling qemu_clock_warp

[Alex Bligh]

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