Re: [PATCH v4 09/10] pvclock: add clocksource change notification on changing of tsc stable bit

[Marcelo Tosatti]

On Wed, Aug 02, 2017 at 05:38:09PM +0300, Denis Plotnikov wrote:
> It's needed to notify the KVM guest about critical changes in pvclock
> and make it to update its masterclock.
> 
> This is a part of the work aiming to make kvmclock be a clocksource
> providing valid cycles value for KVM masterclock, another words
> make possible to use KVM masterclock over kvmclock clocksource.
> 
> Signed-off-by: Denis Plotnikov <dplotnikov@xxxxxxxxxxxxx>
> ---
>  arch/x86/kernel/pvclock.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)

Please do an analysis similar to the comment which starts at 

"* Assuming a stable TSC across physical CPUS, and a stable TSC
 * across virtual CPUs, the following condition is possible.
 * Each numbered line represents an event visible to both
 * CPUs at the next numbered event."

Describing why its safe to use kvmclock as source for the masterclock
(honestly i haven't gone through the details, but someone should 
before this patch is merged).

For one thing, its only safe to use kvmclock masterclock if
the TSCs are synchronized in the host (so that you can read offset
on vcpu-0 using a TSC that has been initialized on vcpu-1). 
So masterclock in the L1 guest is necessary. Do you enforce that?

Also L2 guest TSCs must be synchronized. Where is that enforced?

Also why its safe to use non-TSC-clocksource (clock_read, tsc_read) + 
tsc offsets from that point.



> 
> diff --git a/arch/x86/kernel/pvclock.c b/arch/x86/kernel/pvclock.c
> index bece384..5898f20 100644
> --- a/arch/x86/kernel/pvclock.c
> +++ b/arch/x86/kernel/pvclock.c
> @@ -22,6 +22,7 @@
>  #include <linux/gfp.h>
>  #include <linux/bootmem.h>
>  #include <linux/nmi.h>
> +#include <linux/cs_notifier.h>
>  
>  #include <asm/fixmap.h>
>  #include <asm/pvclock.h>
> @@ -73,6 +74,8 @@ u8 pvclock_read_flags(struct pvclock_vcpu_time_info *src)
>  	return flags & valid_flags;
>  }
>  
> +static atomic_t clocksource_stable = ATOMIC_INIT(0);
> +
>  u64 pvclock_clocksource_read(struct pvclock_vcpu_time_info *src,
>  				u64 *cycles_stamp, u8 *flags_stamp)
>  {
> @@ -102,10 +105,20 @@ u64 pvclock_clocksource_read(struct pvclock_vcpu_time_info *src,
>  		pvclock_touch_watchdogs();
>  	}
>  
> -	if ((valid_flags & PVCLOCK_TSC_STABLE_BIT) &&
> -		(flags & PVCLOCK_TSC_STABLE_BIT))
> -		return ret;
> +	if (likely(valid_flags & PVCLOCK_TSC_STABLE_BIT)) {
> +		bool stable_now = !!(flags & PVCLOCK_TSC_STABLE_BIT);
> +		bool stable_last = (bool) atomic_read(&clocksource_stable);
> +
> +		if (unlikely(stable_now != stable_last)) {
> +			/* send notification once */
> +			if (stable_last == atomic_cmpxchg(
> +				&clocksource_stable, stable_last, stable_now))
> +				clocksource_changes_notify();
> +		}
>  
> +		if (stable_now)
> +			return ret;
> +	}
>  	/*
>  	 * Assumption here is that last_value, a global accumulator, always goes
>  	 * forward. If we are less than that, we should not be much smaller.
> -- 
> 2.7.4