RE: [PATCH 01/15] hyperv: consolidate TSC ref page definitions

[KY Srinivasan]

> -----Original Message-----
> From: Roman Kagan [mailto:rkagan@xxxxxxxxxxxxx]
> Sent: Tuesday, December 20, 2016 7:56 AM
> To: Paolo Bonzini <pbonzini@xxxxxxxxxx>; Radim Krčmář
> <rkrcmar@xxxxxxxxxx>; KY Srinivasan <kys@xxxxxxxxxxxxx>; Vitaly
> Kuznetsov <vkuznets@xxxxxxxxxx>
> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>; Ingo Molnar
> <mingo@xxxxxxxxxx>; H. Peter Anvin <hpa@xxxxxxxxx>; x86@xxxxxxxxxx;
> Haiyang Zhang <haiyangz@xxxxxxxxxxxxx>; kvm@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; devel@xxxxxxxxxxxxxxxxxxxxxx; Denis V . Lunev
> <den@xxxxxxxxxx>; Roman Kagan <rkagan@xxxxxxxxxxxxx>
> Subject: [PATCH 01/15] hyperv: consolidate TSC ref page definitions
> 
> Consolidate the guest-side and kvm-side definitions for Hyper-V TSC
> reference page.
> 
> While at this, rewrite read_hv_clock_tsc using the existing helpers.

Why not beak this into separate patches.
> 
> Signed-off-by: Roman Kagan <rkagan@xxxxxxxxxxxxx>
> ---
>  arch/x86/include/asm/kvm_host.h    |  2 +-
>  arch/x86/include/uapi/asm/hyperv.h |  4 +--
>  drivers/hv/hyperv_vmbus.h          |  8 ------
>  arch/x86/kvm/hyperv.c              |  4 +--
>  drivers/hv/hv.c                    | 54 +++++++++++++++-----------------------
>  5 files changed, 26 insertions(+), 46 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h
> b/arch/x86/include/asm/kvm_host.h
> index 2e25038..2b85f49 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -713,7 +713,7 @@ struct kvm_hv {
>  	u64 hv_crash_param[HV_X64_MSR_CRASH_PARAMS];
>  	u64 hv_crash_ctl;
> 
> -	HV_REFERENCE_TSC_PAGE tsc_ref;
> +	struct hv_ref_tsc_page tsc_ref;
>  };
> 
>  struct kvm_arch {
> diff --git a/arch/x86/include/uapi/asm/hyperv.h
> b/arch/x86/include/uapi/asm/hyperv.h
> index 9b1a918..6098ab5 100644
> --- a/arch/x86/include/uapi/asm/hyperv.h
> +++ b/arch/x86/include/uapi/asm/hyperv.h
> @@ -252,12 +252,12 @@
>  #define HV_STATUS_INVALID_CONNECTION_ID		18
>  #define HV_STATUS_INSUFFICIENT_BUFFERS		19
> 
> -typedef struct _HV_REFERENCE_TSC_PAGE {
> +struct hv_ref_tsc_page {
>  	__u32 tsc_sequence;
>  	__u32 res1;
>  	__u64 tsc_scale;
>  	__s64 tsc_offset;
> -} HV_REFERENCE_TSC_PAGE, *PHV_REFERENCE_TSC_PAGE;
> +};
> 
>  /* Define the number of synthetic interrupt sources. */
>  #define HV_SYNIC_SINT_COUNT		(16)
> diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
> index 0675b39..4516498 100644
> --- a/drivers/hv/hyperv_vmbus.h
> +++ b/drivers/hv/hyperv_vmbus.h
> @@ -475,14 +475,6 @@ struct hv_context {
> 
>  extern struct hv_context hv_context;
> 
> -struct ms_hyperv_tsc_page {
> -	volatile u32 tsc_sequence;
> -	u32 reserved1;
> -	volatile u64 tsc_scale;
> -	volatile s64 tsc_offset;
> -	u64 reserved2[509];
> -};
> -
>  struct hv_ring_buffer_debug_info {
>  	u32 current_interrupt_mask;
>  	u32 current_read_index;
> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> index 1572c35..c7db112 100644
> --- a/arch/x86/kvm/hyperv.c
> +++ b/arch/x86/kvm/hyperv.c
> @@ -806,7 +806,7 @@ static int kvm_hv_msr_set_crash_data(struct
> kvm_vcpu *vcpu,
>   * These two equivalencies are implemented in this function.
>   */
>  static bool compute_tsc_page_parameters(struct pvclock_vcpu_time_info
> *hv_clock,
> -					HV_REFERENCE_TSC_PAGE *tsc_ref)
> +					struct hv_ref_tsc_page *tsc_ref)
>  {
>  	u64 max_mul;
> 
> @@ -847,7 +847,7 @@ void kvm_hv_setup_tsc_page(struct kvm *kvm,
>  	u64 gfn;
> 
>  	BUILD_BUG_ON(sizeof(tsc_seq) != sizeof(hv-
> >tsc_ref.tsc_sequence));
> -	BUILD_BUG_ON(offsetof(HV_REFERENCE_TSC_PAGE,
> tsc_sequence) != 0);
> +	BUILD_BUG_ON(offsetof(struct hv_ref_tsc_page, tsc_sequence) !=
> 0);
> 
>  	if (!(hv->hv_tsc_page & HV_X64_MSR_TSC_REFERENCE_ENABLE))
>  		return;
> diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c
> index 446802a..a7256ec 100644
> --- a/drivers/hv/hv.c
> +++ b/drivers/hv/hv.c
> @@ -137,41 +137,29 @@ EXPORT_SYMBOL_GPL(hv_do_hypercall);
>  #ifdef CONFIG_X86_64
>  static cycle_t read_hv_clock_tsc(struct clocksource *arg)
>  {
> -	cycle_t current_tick;
> -	struct ms_hyperv_tsc_page *tsc_pg = hv_context.tsc_page;
> +	struct hv_ref_tsc_page *tsc_pg = hv_context.tsc_page;
> +	u32 sequence;
> +	u64 scale;
> +	s64 offset;
> +
> +	do {
> +		sequence = tsc_pg->tsc_sequence;
> +		virt_rmb();
> +
> +		if (!sequence) {
> +			/* fallback to MSR */
> +			cycle_t current_tick;
> +			rdmsrl(HV_X64_MSR_TIME_REF_COUNT,
> current_tick);
> +			return current_tick;
> +		}
> 
> -	if (tsc_pg->tsc_sequence != 0) {
> -		/*
> -		 * Use the tsc page to compute the value.
> -		 */
> +		scale = tsc_pg->tsc_scale;
> +		offset = tsc_pg->tsc_offset;
> 
> -		while (1) {
> -			cycle_t tmp;
> -			u32 sequence = tsc_pg->tsc_sequence;
> -			u64 cur_tsc;
> -			u64 scale = tsc_pg->tsc_scale;
> -			s64 offset = tsc_pg->tsc_offset;
> -
> -			rdtscll(cur_tsc);
> -			/* current_tick = ((cur_tsc *scale) >> 64) + offset */
> -			asm("mulq %3"
> -				: "=d" (current_tick), "=a" (tmp)
> -				: "a" (cur_tsc), "r" (scale));
> -
> -			current_tick += offset;
> -			if (tsc_pg->tsc_sequence == sequence)
> -				return current_tick;
> -
> -			if (tsc_pg->tsc_sequence != 0)
> -				continue;
> -			/*
> -			 * Fallback using MSR method.
> -			 */
> -			break;
> -		}
> -	}
> -	rdmsrl(HV_X64_MSR_TIME_REF_COUNT, current_tick);
> -	return current_tick;
> +		virt_rmb();
> +	} while (tsc_pg->tsc_sequence != sequence);
> +
> +	return mul_u64_u64_shr(rdtsc_ordered(), scale, 64) + offset;
>  }
> 
>  static struct clocksource hyperv_cs_tsc = {
> --
> 2.9.3