Re: [Qemu-devel] [PATCH v4 1/6] target-arm: Add CNTVOFF_EL2

[Peter Maydell <peter.maydell@linaro.org>]

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