From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43823) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z4Ihk-0006sM-1q for qemu-devel@nongnu.org; Sun, 14 Jun 2015 20:56:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z4Ihg-0002jx-R0 for qemu-devel@nongnu.org; Sun, 14 Jun 2015 20:56:48 -0400 Received: from mail-by2on0078.outbound.protection.outlook.com ([207.46.100.78]:36592 helo=na01-by2-obe.outbound.protection.outlook.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z4Ihg-0002iC-GC for qemu-devel@nongnu.org; Sun, 14 Jun 2015 20:56:44 -0400 Date: Mon, 15 Jun 2015 10:52:00 +1000 From: "Edgar E. Iglesias" Message-ID: <20150615005200.GP17878@toto> References: <1433500421-22879-1-git-send-email-edgar.iglesias@gmail.com> <1433500421-22879-2-git-send-email-edgar.iglesias@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH v4 1/6] target-arm: Add CNTVOFF_EL2 List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: "Edgar E. Iglesias" , Sergey Fedorov , Alex =?iso-8859-1?Q?Benn=E9e?= , QEMU Developers , Alexander Graf On Fri, Jun 12, 2015 at 05:44:24PM +0100, Peter Maydell wrote: > On 5 June 2015 at 11:33, Edgar E. Iglesias wrote: > > From: "Edgar E. Iglesias" > > > > Adds support for the virtual timer offset controlled by EL2. > > > > Signed-off-by: Edgar E. Iglesias > > --- > > > --- a/target-arm/helper.c > > +++ b/target-arm/helper.c > > @@ -1208,9 +1208,20 @@ static void gt_recalc_timer(ARMCPU *cpu, int timeridx) > > /* Timer enabled: calculate and set current ISTATUS, irq, and > > * reset timer to when ISTATUS next has to change > > */ > > + uint64_t offset = timeridx == GTIMER_VIRT ? > > + cpu->env.cp15.cntvoff_el2 : 0; > > uint64_t count = gt_get_countervalue(&cpu->env); > > - /* Note that this must be unsigned 64 bit arithmetic: */ > > - int istatus = count >= gt->cval; > > + /* The ARM spec says that count, offset and gt->cval are all > > + * unsigned 64bit values. > > + * The event trig is described as: > > + * (Counter[63:0] - Offset[63:0])[63:0] - CompareValue[63:0]) >= 0 > > + * > > + * We do the subtractions as unsigned values to avoid under/overflowing > > + * signed integers (undefined behaviour in C). > > + * To be able to do the compare >= 0 we cast the result into a > > + * signed int64_t. > > + */ > > + int istatus = (int64_t) (count - offset - gt->cval) >= 0; > > This is wrong. Consider the case where: > count is 0x1000,0000,0000,0002 (it's a really large unsigned number) > offset is zero > cval is 1 > > The ARM ARM required calculation gives you > 0x1000,0000,0000,0002 - 1 >= 0 > ie 0x1000,0000,0000,0001 >= 0 > which is true. (Note that ARM ARM pseudocode works with infinite > precision integers, not 2s-complement.) > > With your code: > (count - offset - gt->cval) is 0x1000,0000,0000,0001 > Cast to an int64_t this is negative (top bit is set) > Comparison against 0 is done as a signed value, and returns false. > > This is exactly the tricky case which is why we must do this as unsigned > arithmetic. > > What you want is > int istatus = count - offset >= gt->cval; > > which comes out to > 0x1000,0000,0000,0002 >= 1 > which is true. > > (That's the code we had before, but just "use 'count - offset' rather than > 'count'".) Thanks, I've changed it to what you suggest allthough I'm probably missing something cause I'm still finding the spec confusing :S > > @@ -1265,17 +1281,19 @@ static void gt_cval_write(CPUARMState *env, const ARMCPRegInfo *ri, > > static uint64_t gt_tval_read(CPUARMState *env, const ARMCPRegInfo *ri) > > { > > int timeridx = ri->crm & 1; > > + uint64_t offset = timeridx == GTIMER_VIRT ? env->cp15.cntvoff_el2 : 0; > > > > return (uint32_t)(env->cp15.c14_timer[timeridx].cval - > > - gt_get_countervalue(env)); > > + gt_get_countervalue(env) - offset); > > The docs say that the timerval read view is > (comparevalue - (counter - offset)) > not (comparevalue - counter - offset)... Fixed for next version. Cheers, Edgar > > > } > > Looks OK otherwise. > > thanks > -- PMM