Re: [PATCH RFC 5/6] x86/kvm: pass stable clocksource to guests when running nested on Hyper-V

["Radim Krčmář" <rkrcmar@redhat.com>]

2017-12-01 14:13+0100, Vitaly Kuznetsov:
> Currently, KVM is able to work in 'masterclock' mode passing
> PVCLOCK_TSC_STABLE_BIT to guests when the clocksource we use on the host
> is TSC. When running nested on Hyper-V we normally use a different one:
> TSC page which is resistant to TSC frequency changes on event like L1
> migration. Add support for it in KVM.
> 
> The only non-trivial change in the patch is in vgettsc(): when updating
> our gtod copy we now need to get both the clockread and tsc value.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> @@ -1374,6 +1375,11 @@ static u64 compute_guest_tsc(struct kvm_vcpu *vcpu, s64 kernel_ns)
> +static inline int gtod_cs_mode_good(int mode)

"good" isn't saying much; I'd like to express that TSC is the underlying
clock ...

What about "bool gtod_is_based_on_tsc()"?

> +{
> +	return mode == VCLOCK_TSC || mode == VCLOCK_HVCLOCK;
> +}
> +
> @@ -1606,9 +1625,17 @@ static inline u64 vgettsc(u64 *cycle_now)
>  	long v;
>  	struct pvclock_gtod_data *gtod = &pvclock_gtod_data;
>  
> -	*cycle_now = read_tsc();
> +	if (gtod->clock.vclock_mode == VCLOCK_HVCLOCK) {
> +		u64 tsc_pg_val;
> +
> +		tsc_pg_val = hv_read_tsc_page_tsc(hv_get_tsc_page(), cycle_now);

This function might fail to update cycle_now and return -1.
I guess we should propagate the failure in that case.

> +		v = (tsc_pg_val - gtod->clock.cycle_last) & gtod->clock.mask;
> +	} else {
> +		/* VCLOCK_TSC */
> +		*cycle_now = read_tsc();
> +		v = (*cycle_now - gtod->clock.cycle_last) & gtod->clock.mask;

cycle_now is getting pretty confusing -- it still is TSC timestamp, but
now we also have the current cycle of gtod, which might be the TSC page
timestamp.  Please rename cycle_now to tsc_timestamp in the call tree,

thanks.