Date: Fri, 1 Dec 2017 18:45:30 +0100
From: Radim =?utf-8?B?S3LEjW3DocWZ?= <rkrcmar@redhat.com>
To: Vitaly Kuznetsov <vkuznets@redhat.com>
Cc: kvm@vger.kernel.org, x86@kernel.org,
        Paolo Bonzini <pbonzini@redhat.com>,
        Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar <mingo@redhat.com>,
        "H. Peter Anvin" <hpa@zytor.com>,
        "K. Y. Srinivasan" <kys@microsoft.com>,
        Haiyang Zhang <haiyangz@microsoft.com>,
        Stephen Hemminger <sthemmin@microsoft.com>,
        Andy Lutomirski <luto@kernel.org>, linux-kernel@vger.kernel.org,
        devel@linuxdriverproject.org
Subject: Re: [PATCH RFC 5/6] x86/kvm: pass stable clocksource to guests when
 running nested on Hyper-V
Message-ID: <20171201174529.GA17073@flask>
References: <20171201131321.918-1-vkuznets@redhat.com>
 <20171201131321.918-6-vkuznets@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20171201131321.918-6-vkuznets@redhat.com>
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1828
Lines: 49

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.