From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52900) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VTaoB-00036d-4G for qemu-devel@nongnu.org; Tue, 08 Oct 2013 13:11:01 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VTao5-0004sl-4y for qemu-devel@nongnu.org; Tue, 08 Oct 2013 13:10:55 -0400 Received: from mx1.redhat.com ([209.132.183.28]:40005) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VTao4-0004sN-Kh for qemu-devel@nongnu.org; Tue, 08 Oct 2013 13:10:48 -0400 Message-ID: <52543C90.40408@redhat.com> Date: Tue, 08 Oct 2013 19:10:40 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1381222058-16701-1-git-send-email-pbonzini@redhat.com> <1381222058-16701-6-git-send-email-pbonzini@redhat.com> <703C02BF-B3A3-493A-8713-7979746C6811@alex.org.uk> <5254394B.506@redhat.com> <63E54668-8F65-4B10-9602-34AC9B2D567D@alex.org.uk> In-Reply-To: <63E54668-8F65-4B10-9602-34AC9B2D567D@alex.org.uk> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 5/8] timers: prepare the code for future races in calling qemu_clock_warp List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alex Bligh Cc: qemu-devel@nongnu.org Il 08/10/2013 19:08, Alex Bligh ha scritto: > > 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. > I'll clean that up separately. Thanks, Paolo