| Message ID | 1433500421-22879-2-git-send-email-edgar.iglesias@gmail.com |
|---|---|
| State | New |
| Headers | show |
On 5 June 2015 at 11:33, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote: > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> > > Adds support for the virtual timer offset controlled by EL2. > > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> > --- > --- 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'".) > @@ -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)... > } Looks OK otherwise. thanks -- PMM
On Fri, Jun 12, 2015 at 05:44:24PM +0100, Peter Maydell wrote: > On 5 June 2015 at 11:33, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote: > > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> > > > > Adds support for the virtual timer offset controlled by EL2. > > > > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> > > --- > > > --- 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
On 15 June 2015 at 01:52, Edgar E. Iglesias <edgar.iglesias@xilinx.com> wrote: > On Fri, Jun 12, 2015 at 05:44:24PM +0100, Peter Maydell wrote: >> On 5 June 2015 at 11:33, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote: >> > + 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 If we had 128 bit integers we could do it your way, only casting to int128_t rather than int64_t (which would be approximating the pseudocode's infinite-precision signed integers with 128-bit ints, which works because we know we don't have anything out of that range). The tricky stuff with uint64_t is just because we don't want to have to go to 128-bit arithmetic if we can avoid it. As I say the thing it's easy to forget when reading ARM ARM pseudocode is that the integer and real data types are true mathematical integers and reals, not the limited-width uint32_t, uint64_t, float and double types the CPU actually deals with. There's always a conversion to/from bitstring somewhere along the line. (Appendix J9 has a description of the pseudocode data types and syntax.) -- PMM
diff --git a/target-arm/cpu.h b/target-arm/cpu.h index 21b5b8e..1a66aa4 100644 --- a/target-arm/cpu.h +++ b/target-arm/cpu.h @@ -355,6 +355,7 @@ typedef struct CPUARMState { }; uint64_t c14_cntfrq; /* Counter Frequency register */ uint64_t c14_cntkctl; /* Timer Control register */ + uint64_t cntvoff_el2; /* Counter Virtual Offset register */ ARMGenericTimer c14_timer[NUM_GTIMERS]; uint32_t c15_cpar; /* XScale Coprocessor Access Register */ uint32_t c15_ticonfig; /* TI925T configuration byte. */ diff --git a/target-arm/helper.c b/target-arm/helper.c index 3da0c05..7901da1 100644 --- 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; uint64_t nexttick; gt->ctl = deposit32(gt->ctl, 2, 1, istatus); @@ -1221,7 +1232,7 @@ static void gt_recalc_timer(ARMCPU *cpu, int timeridx) nexttick = UINT64_MAX; } else { /* Next transition is when we hit cval */ - nexttick = gt->cval; + nexttick = gt->cval + offset; } /* Note that the desired next expiry time might be beyond the * signed-64-bit range of a QEMUTimer -- in this case we just @@ -1253,6 +1264,11 @@ static uint64_t gt_cnt_read(CPUARMState *env, const ARMCPRegInfo *ri) return gt_get_countervalue(env); } +static uint64_t gt_virt_cnt_read(CPUARMState *env, const ARMCPRegInfo *ri) +{ + return gt_get_countervalue(env) - env->cp15.cntvoff_el2; +} + static void gt_cval_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value) { @@ -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); } static void gt_tval_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value) { int timeridx = ri->crm & 1; + uint64_t offset = timeridx == GTIMER_VIRT ? env->cp15.cntvoff_el2 : 0; - env->cp15.c14_timer[timeridx].cval = gt_get_countervalue(env) + + env->cp15.c14_timer[timeridx].cval = gt_get_countervalue(env) - offset + sextract64(value, 0, 32); gt_recalc_timer(arm_env_get_cpu(env), timeridx); } @@ -1300,6 +1318,15 @@ static void gt_ctl_write(CPUARMState *env, const ARMCPRegInfo *ri, } } +static void gt_cntvoff_write(CPUARMState *env, const ARMCPRegInfo *ri, + uint64_t value) +{ + ARMCPU *cpu = arm_env_get_cpu(env); + + raw_write(env, ri, value); + gt_recalc_timer(cpu, GTIMER_VIRT); +} + void arm_gt_ptimer_cb(void *opaque) { ARMCPU *cpu = opaque; @@ -1409,13 +1436,13 @@ static const ARMCPRegInfo generic_timer_cp_reginfo[] = { { .name = "CNTVCT", .cp = 15, .crm = 14, .opc1 = 1, .access = PL0_R, .type = ARM_CP_64BIT | ARM_CP_NO_RAW | ARM_CP_IO, .accessfn = gt_vct_access, - .readfn = gt_cnt_read, .resetfn = arm_cp_reset_ignore, + .readfn = gt_virt_cnt_read, .resetfn = arm_cp_reset_ignore, }, { .name = "CNTVCT_EL0", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 3, .crn = 14, .crm = 0, .opc2 = 2, .access = PL0_R, .type = ARM_CP_NO_RAW | ARM_CP_IO, .accessfn = gt_vct_access, - .readfn = gt_cnt_read, .resetfn = gt_cnt_reset, + .readfn = gt_virt_cnt_read, .resetfn = gt_cnt_reset, }, /* Comparison value, indicating when the timer goes off */ { .name = "CNTP_CVAL", .cp = 15, .crm = 14, .opc1 = 2, @@ -2539,6 +2566,12 @@ static const ARMCPRegInfo el3_no_el2_cp_reginfo[] = { { .name = "HTTBR", .cp = 15, .opc1 = 4, .crm = 2, .access = PL2_RW, .type = ARM_CP_64BIT | ARM_CP_CONST, .resetvalue = 0 }, + { .name = "CNTVOFF_EL2", .state = ARM_CP_STATE_AA64, + .opc0 = 3, .opc1 = 4, .crn = 14, .crm = 0, .opc2 = 3, + .access = PL2_RW, .type = ARM_CP_CONST, .resetvalue = 0 }, + { .name = "CNTVOFF", .cp = 15, .opc1 = 4, .crm = 14, + .access = PL2_RW, .type = ARM_CP_64BIT | ARM_CP_CONST, + .resetvalue = 0 }, REGINFO_SENTINEL }; @@ -2651,6 +2684,17 @@ static const ARMCPRegInfo el2_cp_reginfo[] = { .opc0 = 1, .opc1 = 4, .crn = 8, .crm = 3, .opc2 = 1, .type = ARM_CP_NO_RAW, .access = PL2_W, .writefn = tlbi_aa64_vaa_write }, +#ifndef CONFIG_USER_ONLY + { .name = "CNTVOFF_EL2", .state = ARM_CP_STATE_AA64, + .opc0 = 3, .opc1 = 4, .crn = 14, .crm = 0, .opc2 = 3, + .access = PL2_RW, .type = ARM_CP_IO, .resetvalue = 0, + .writefn = gt_cntvoff_write, + .fieldoffset = offsetof(CPUARMState, cp15.cntvoff_el2) }, + { .name = "CNTVOFF", .cp = 15, .opc1 = 4, .crm = 14, + .access = PL2_RW, .type = ARM_CP_64BIT | ARM_CP_ALIAS | ARM_CP_IO, + .writefn = gt_cntvoff_write, + .fieldoffset = offsetof(CPUARMState, cp15.cntvoff_el2) }, +#endif REGINFO_SENTINEL };