Subject: Re: [PATCH v4 00/10] make L2's kvm-clock stable, get rid of
 pvclock_gtod_copy in KVM
To: Paolo Bonzini <pbonzini@redhat.com>,
        Thomas Gleixner <tglx@linutronix.de>
Cc: John Stultz <john.stultz@linaro.org>,
        Radim Krcmar <rkrcmar@redhat.com>, kvm list <kvm@vger.kernel.org>,
        Ingo Molnar <mingo@redhat.com>, "H. Peter Anvin" <hpa@zytor.com>,
        lkml <linux-kernel@vger.kernel.org>, x86@kernel.org,
        rkagan@virtuozzo.com, den@virtuozzo.com,
        Marcelo Tosatti <mtosatti@redhat.com>,
        Peter Zijlstra <peterz@infradead.org>
References: <1501684690-211093-1-git-send-email-dplotnikov@virtuozzo.com>
 <CALAqxLVngARqG4g8URMQGQtZec_p5NVP7hFXQGS6+JWp74FVMA@mail.gmail.com>
 <b913b3b2-b3bb-348a-4069-c251a22233cd@redhat.com>
 <f93105c1-0d6f-f9d4-fc43-31efae1c9aeb@virtuozzo.com>
 <CALAqxLWGf6BAdji=rqKPnyvFzzUQscA5rXU8SY4RcWXymyVL1Q@mail.gmail.com>
 <894362115.582988.1503435653874.JavaMail.zimbra@redhat.com>
 <alpine.DEB.2.20.1708230908330.1821@nanos>
 <ea24b142-21b2-4462-b2be-7716933f1745@redhat.com>
 <43d87f0d-62c3-878e-108a-aaf7fb68fcb3@redhat.com>
From: Denis Plotnikov <dplotnikov@virtuozzo.com>
Message-ID: <ca8c2155-707d-14f0-d818-053e5e57f8bd@virtuozzo.com>
Date: Mon, 28 Aug 2017 10:28:06 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.2.1
MIME-Version: 1.0
In-Reply-To: <43d87f0d-62c3-878e-108a-aaf7fb68fcb3@redhat.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 28 Aug 2017 07:29:07.2795 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM4PR0802MB2212
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 3685
Lines: 112


On 24.08.2017 11:00, Paolo Bonzini wrote:
> On 23/08/2017 18:02, Paolo Bonzini wrote:
>>
>> More duct tape would have been just:
>>
>> -	if (pvclock_gtod_data.clock.vclock_mode != VCLOCK_TSC)
>> +	mode = READ_ONCE(pvclock_gtod_data.clock.vclock_mode);
>> +	if (mode != VCLOCK_TSC &&
>> +	    (mode != VCLOCK_PVCLOCK || !pvclock_nested_virt_magic())
>> 		return false;
>>
>> -	return do_realtime(ts, cycle_now) == VCLOCK_TSC;
>> +	switch (mode) {
>> +	case VCLOCK_TSC:
>> +		return do_realtime_tsc(ts, cycle_now);
>> +	case VCLOCK_PVCLOCK:
>> +		return do_realtime_pvclock(ts, cycle_now);
>> +	}
>>
>> Nested virtualization does need a clocksource change notifier on top,
>> but we can cross that bridge later.  Maybe Denis can post just those
>> patches to begin with.
> 
> For what it's worth, this is all that's needed (with patches 1-2-3-4-5-7)
> to support kvmclock on top of Hyper-V clock.  It's trivial.
> 
> Even if we could add paravirtualization magic to KVM live migration, we
> certainly couldn't do that for other hypervisors.
> 
> diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
> index 5b882cc0c0e9..3bab935b021a 100644
> --- a/arch/x86/hyperv/hv_init.c
> +++ b/arch/x86/hyperv/hv_init.c
> @@ -46,10 +46,24 @@ static u64 read_hv_clock_tsc(struct clocksource *arg)
>   	return current_tick;
>   }
>   
> +static bool read_hv_clock_tsc_with_stamp(struct clocksource *arg,
> +					 u64 *cycles, u64 *cycles_stamp)
> +{
> +	*cycles = __hv_read_tsc_page(tsc_pg, &cycles_stamp);
> +
> +	if (*cycles == U64_MAX) {
> +		*cycles = rdmsrl(HV_X64_MSR_TIME_REF_COUNT);
> +		return false;
> +	}
> +
> +	return true;
> +}
> +
>   static struct clocksource hyperv_cs_tsc = {
>   		.name		= "hyperv_clocksource_tsc_page",
>   		.rating		= 400,
>   		.read		= read_hv_clock_tsc,
> +		.read_with_stamp = read_hv_clock_tsc_with_stamp,
>   		.mask		= CLOCKSOURCE_MASK(64),
>   		.flags		= CLOCK_SOURCE_IS_CONTINUOUS,
>   };
> diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h
> index 2b58c8c1eeaa..5aff66e9fff7 100644
> --- a/arch/x86/include/asm/mshyperv.h
> +++ b/arch/x86/include/asm/mshyperv.h
> @@ -176,9 +176,9 @@ void hyperv_cleanup(void);
>   #endif
>   #ifdef CONFIG_HYPERV_TSCPAGE
>   struct ms_hyperv_tsc_page *hv_get_tsc_page(void);
> -static inline u64 hv_read_tsc_page(const struct ms_hyperv_tsc_page *tsc_pg)
> +static inline u64 __hv_read_tsc_page(const struct ms_hyperv_tsc_page *tsc_pg, u64 *cur_tsc)
>   {
> -	u64 scale, offset, cur_tsc;
> +	u64 scale, offset;
>   	u32 sequence;
>   
>   	/*
> @@ -209,7 +209,7 @@ static inline u64 hv_read_tsc_page(const struct ms_hyperv_tsc_page *tsc_pg)
>   
>   		scale = READ_ONCE(tsc_pg->tsc_scale);
>   		offset = READ_ONCE(tsc_pg->tsc_offset);
> -		cur_tsc = rdtsc_ordered();
> +		*cur_tsc = rdtsc_ordered();
>   
>   		/*
>   		 * Make sure we read sequence after we read all other values
> @@ -219,9 +219,14 @@ static inline u64 hv_read_tsc_page(const struct ms_hyperv_tsc_page *tsc_pg)
>   
>   	} while (READ_ONCE(tsc_pg->tsc_sequence) != sequence);
>   
> -	return mul_u64_u64_shr(cur_tsc, scale, 64) + offset;
> +	return mul_u64_u64_shr(*cur_tsc, scale, 64) + offset;
>   }
>   
> +static inline u64 hv_read_tsc_page(const struct ms_hyperv_tsc_page *tsc_pg)
> +{
> +	u64 cur_tsc;
> +	return __hv_read_tsc_page(tsc_pg, &cur_tsc);
> +}
>   #else
>   static inline struct ms_hyperv_tsc_page *hv_get_tsc_page(void)
>   {
> 
> 
> Denis, could you try redoing patch 7 to use the pvclock_gtod_notifier
> instead of the new one you're adding, and only send that first part?  I
> think it's a worthwhile cleanup anyway, so let's start with that.
> 
> Paolo
>
Ok, I'll do that

Denis