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
