Re: [PATCH 2/2] x86/vdso: Add VCLOCK_HVCLOCK vDSO clock read method

From: Thomas Gleixner
Date: Fri Feb 10 2017 - 06:35:01 EST


On Fri, 10 Feb 2017, Vitaly Kuznetsov wrote:
> Thomas Gleixner <tglx@xxxxxxxxxxxxx> writes:
>
> > On Thu, 9 Feb 2017, Vitaly Kuznetsov wrote:
> >> +#ifdef CONFIG_HYPERV_TSCPAGE
> >> +static notrace u64 vread_hvclock(int *mode)
> >> +{
> >> + const struct ms_hyperv_tsc_page *tsc_pg =
> >> + (const struct ms_hyperv_tsc_page *)&hvclock_page;
> >> + u64 sequence, scale, offset, current_tick, cur_tsc;
> >> +
> >> + while (1) {
> >> + sequence = READ_ONCE(tsc_pg->tsc_sequence);
> >> + if (!sequence)
> >> + break;
> >> +
> >> + scale = READ_ONCE(tsc_pg->tsc_scale);
> >> + offset = READ_ONCE(tsc_pg->tsc_offset);
> >> + rdtscll(cur_tsc);
> >> +
> >> + current_tick = mul_u64_u64_shr(cur_tsc, scale, 64) + offset;
> >> +
> >> + if (READ_ONCE(tsc_pg->tsc_sequence) == sequence)
> >> + return current_tick;
> >
> > That sequence stuff lacks still a sensible explanation. It's fundamentally
> > different from the sequence counting we do in the kernel, so documentation
> > for it is really required.
>
> Sure, do you think the following would do?

Yes

> diff --git a/arch/x86/entry/vdso/vclock_gettime.c b/arch/x86/entry/vdso/vclock_gettime.c
> index 4af10b4..886b600 100644
> --- a/arch/x86/entry/vdso/vclock_gettime.c
> +++ b/arch/x86/entry/vdso/vclock_gettime.c
> @@ -154,6 +154,22 @@ static notrace u64 vread_hvclock(int *mode)
> (const struct ms_hyperv_tsc_page *)&hvclock_page;
> u64 sequence, scale, offset, current_tick, cur_tsc;
>
> + /*
> + * The protocol for reading Hyper-V TSC page is specified in Hypervisor
> + * Top-Level Functional Specification ver. 3.0 and above. To get the
> + * reference time we must do the following:
> + * - READ ReferenceTscSequence
> + * A special '0' value indicates the time source is unreliable and we
> + * need to use something else. The currently published specification
> + * versions (up to 4.0b) contain a mistake and wrongly claim '-1'
> + * instead of '0' as the special value, see commit c35b82ef0294.
> + * - ReferenceTime =
> + * ((RDTSC() * ReferenceTscScale) >> 64) + ReferenceTscOffset
> + * - READ ReferenceTscSequence again. In case its value has changed
> + * since our first reading we need to discard ReferenceTime and repeat
> + * the whole sequence as the hypervisor was updating the page in
> + * between.
> + */
> while (1) {
> sequence = READ_ONCE(tsc_pg->tsc_sequence);
> if (!sequence)
>
> --
> Vitaly
>