Re: [PATCH 2/2] arm64:vdso: Remove ISB from gettimeofday.

[Jon Masters]

Quick reply - didn't realize it could be speculatively read as described, but I should have. Makes sense now, thanks.

-- 
Computer Architect | Sent from my 64-bit #ARM Powered phone

> On May 6, 2017, at 13:38, Pinski, Andrew <Andrew.Pinski@xxxxxxxxxx> wrote:
> 
> Sorry sending again as plain text (I did not notice that before).
> 
> On 5/6/2017 9:29 AM, Jon Masters wrote:
>> On 04/23/2017 07:47 PM, Andrew Pinski wrote:
>> ISB is normally required before mrs CNTVCT if we want the
>> mrs to completed after the loads. In this case it is not.
>> As we are taking the difference and if that difference
>> was going to be negative, we just use the last counter value
>> instead.
>> 
>> Signed-off-by: Andrew Pinski <apinski@xxxxxxxxxx>
> 
> Humor me. Walk me through this?
> 
> Here is what the ARM ARM says (D6.1.3):
> Reads of CNTVCT_EL0 can occur speculatively and out of order relative to other instructions executed on the same PE.
> For example, if a read from memory is used to obtain a signal from another agent that indicates that CNTVCT_EL0 must be read, an ISB must be used to ensure that the read of CNTVCT_EL0 occurs after the signal has been read from memory
> --- CUT ---
> 
> So we could have the read of the virtual timer happen before the reading of the cycles last field of the vdso data.  In the current code, there is an ISB which forces that case not to happen.  Without the ISB it could happen but since we know the time (virtual timer) cannot go backwards, we just take the last cycle field as the current time; this could happen when the last cycle field was updated from another core.  I hope this walk through a little better than my original description.
> 
> Basically the current virtual timer is either newly read value or the read from last cycle counter.
> 
> Thanks,
> Andrew Pinski
> 
> 
>> ---
>>   arch/arm64/kernel/vdso/gettimeofday.c | 14 ++++++++++++--
>>   1 file changed, 12 insertions(+), 2 deletions(-)
>> 
>> diff --git a/arch/arm64/kernel/vdso/gettimeofday.c b/arch/arm64/kernel/vdso/gettimeofday.c
>> index a0ab8b1..cf3235a 100644
>> --- a/arch/arm64/kernel/vdso/gettimeofday.c
>> +++ b/arch/arm64/kernel/vdso/gettimeofday.c
>> @@ -117,10 +117,20 @@ static notrace u64 get_clock_shifted_nsec(u64 cycle_last, u64 mult)
>>         u64 res;
>>   
>>         /* Read the virtual counter. */
>> -     isb();
>> +     /*
>> +      * This normally requires an ISB but since we know the
>> +      * read of the last cycle will always be after the
>> +      * read of the values are valid word.
>> +      */
>>         asm volatile("mrs %0, cntvct_el0" : "=r" (res) :: "memory");
>>   
>> -     res = res - cycle_last;
>> +     /*
>> +      * If the current cycle is greater than the last,
>> +      *  then get the difference.
>> +      */
>> +     if (res > cycle_last)
>> +             res = res - cycle_last;
>> +
>>         /* We can only guarantee 56 bits of precision. */
>>         res &= ~(0xff00ul<<48);
>>         return res * mult;
>> 
> 
> 
> -- 
> Computer Architect
> 
>