Re: [PATCH v7 04/25] arm64: Substitute gettimeofday with C implementation

[Vincenzo Frascino]

On 24/06/2019 14:36, Will Deacon wrote:
> Hi Vincenzo,
> 
> On Fri, Jun 21, 2019 at 10:52:31AM +0100, Vincenzo Frascino wrote:
>> To take advantage of the commonly defined vdso interface for
>> gettimeofday the architectural code requires an adaptation.
>>
>> Re-implement the gettimeofday vdso in C in order to use lib/vdso.
>>
>> With the new implementation arm64 gains support for CLOCK_BOOTTIME
>> and CLOCK_TAI.
>>
>> Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
>> Cc: Will Deacon <will.deacon@xxxxxxx>
>> Signed-off-by: Vincenzo Frascino <vincenzo.frascino@xxxxxxx>
>> Tested-by: Shijith Thotton <sthotton@xxxxxxxxxxx>
>> Tested-by: Andre Przywara <andre.przywara@xxxxxxx>
>> ---
>>  arch/arm64/Kconfig                         |   2 +
>>  arch/arm64/include/asm/vdso/gettimeofday.h |  86 ++++++
>>  arch/arm64/include/asm/vdso/vsyscall.h     |  53 ++++
>>  arch/arm64/include/asm/vdso_datapage.h     |  48 ---
>>  arch/arm64/kernel/asm-offsets.c            |  33 +-
>>  arch/arm64/kernel/vdso.c                   |  51 +---
>>  arch/arm64/kernel/vdso/Makefile            |  34 ++-
>>  arch/arm64/kernel/vdso/gettimeofday.S      | 334 ---------------------
>>  arch/arm64/kernel/vdso/vgettimeofday.c     |  28 ++
> 
> I'm concerned about an apparent semantic change introduced by your patch:
> 
>> +static __always_inline u64 __arch_get_hw_counter(s32 clock_mode)
>> +{
>> +	u64 res;
>> +
>> +	asm volatile("mrs %0, cntvct_el0" : "=r" (res) :: "memory");
>> +
>> +	return res;
>> +}
> 
> vs:
> 
>> -	.macro	get_clock_shifted_nsec res, cycle_last, mult
>> -	/* Read the virtual counter. */
>> -	isb
>> -	mrs	x_tmp, cntvct_el0
>> -	/* Calculate cycle delta and convert to ns. */
>> -	sub	\res, x_tmp, \cycle_last
>> -	/* We can only guarantee 56 bits of precision. */
>> -	movn	x_tmp, #0xff00, lsl #48
>> -	and	\res, x_tmp, \res
>> -	mul	\res, \res, \mult
>> -	/*
>> -	 * Fake address dependency from the value computed from the counter
>> -	 * register to subsequent data page accesses so that the sequence
>> -	 * locking also orders the read of the counter.
>> -	 */
>> -	and	x_tmp, \res, xzr
>> -	add	vdso_data, vdso_data, x_tmp
>> -	.endm
> 
> It looks like you're dropping both the preceding ISB (allowing the counter
> value to be speculated) and also the subsequent dependency (allowing the
> seq lock to be speculated). If I've missed them, apologies, but I couldn't
> spot them elsewhere in this patch.
> 
> __arch_get_hw_counter should probably be identical to __arch_counter_get_cntvct
> to avoid these problems. I guess we don't need to care about the case where
> the counter is unstable, since we'll just disable the vDSO altogether on
> such systems?
> 

Oops, I forgot to mirror your patch that introduces this change. I will post a
fix in reply to this email.

> Will
> 

-- 
Regards,
Vincenzo