[PATCH 1/3] pvclock: Add CPU barries to get correct version value

[PATCH 1/3] pvclock: Add CPU barries to get correct version value

[Minfei Huang]

Protocol for the "version" fields is: hypervisor raises it (making it
uneven) before it starts updating the fields and raises it again (making
it even) when it is done.  Thus the guest can make sure the time values
it got are consistent by checking the version before and after reading
them.

Add CPU barries after getting version value just like what function
vread_pvclock does, because all of callees in this function is inline.

Signed-off-by: Minfei Huang <mnghuan@gmail.com>
---
 arch/x86/include/asm/pvclock.h | 2 ++
 arch/x86/kernel/pvclock.c      | 4 ++++
 2 files changed, 6 insertions(+)

diff --git a/arch/x86/include/asm/pvclock.h b/arch/x86/include/asm/pvclock.h
index fdcc040..538ae94 100644
--- a/arch/x86/include/asm/pvclock.h
+++ b/arch/x86/include/asm/pvclock.h
@@ -85,6 +85,8 @@ unsigned __pvclock_read_cycles(const struct pvclock_vcpu_time_info *src,
 	u8 ret_flags;
 
 	version = src->version;
+	/* Make the latest version visible */
+	smp_rmb();
 
 	offset = pvclock_get_nsec_offset(src);
 	ret = src->system_time + offset;
diff --git a/arch/x86/kernel/pvclock.c b/arch/x86/kernel/pvclock.c
index 99bfc02..7f82fe0 100644
--- a/arch/x86/kernel/pvclock.c
+++ b/arch/x86/kernel/pvclock.c
@@ -66,6 +66,8 @@ u8 pvclock_read_flags(struct pvclock_vcpu_time_info *src)
 
 	do {
 		version = __pvclock_read_cycles(src, &ret, &flags);
+		/* Make sure that the version double-check is last. */
+		smp_rmb();
 	} while ((src->version & 1) || version != src->version);
 
 	return flags & valid_flags;
@@ -80,6 +82,8 @@ cycle_t pvclock_clocksource_read(struct pvclock_vcpu_time_info *src)
 
 	do {
 		version = __pvclock_read_cycles(src, &ret, &flags);
+		/* Make sure that the version double-check is last. */
+		smp_rmb();
 	} while ((src->version & 1) || version != src->version);
 
 	if (unlikely((flags & PVCLOCK_GUEST_STOPPED) != 0)) {
-- 
2.6.3

[PATCH 2/3] pvclock: Cleanup to remove function pvclock_get_nsec_offset

[Minfei Huang]

Function __pvclock_read_cycles is short enough, so there is no need to
have another function pvclock_get_nsec_offset to calculate tsc delta.
It's better to combine it into function __pvclock_read_cycles.

Remove useless variables in function __pvclock_read_cycles.

Signed-off-by: Minfei Huang <mnghuan@gmail.com>
---
 arch/x86/include/asm/pvclock.h | 23 +++++++----------------
 1 file changed, 7 insertions(+), 16 deletions(-)

diff --git a/arch/x86/include/asm/pvclock.h b/arch/x86/include/asm/pvclock.h
index 538ae94..7c1c895 100644
--- a/arch/x86/include/asm/pvclock.h
+++ b/arch/x86/include/asm/pvclock.h
@@ -69,31 +69,22 @@ static inline u64 pvclock_scale_delta(u64 delta, u32 mul_frac, int shift)
 }
 
 static __always_inline
-u64 pvclock_get_nsec_offset(const struct pvclock_vcpu_time_info *src)
-{
-	u64 delta = rdtsc_ordered() - src->tsc_timestamp;
-	return pvclock_scale_delta(delta, src->tsc_to_system_mul,
-				   src->tsc_shift);
-}
-
-static __always_inline
 unsigned __pvclock_read_cycles(const struct pvclock_vcpu_time_info *src,
 			       cycle_t *cycles, u8 *flags)
 {
 	unsigned version;
-	cycle_t ret, offset;
-	u8 ret_flags;
+	cycle_t offset;
+	u64 delta;
 
 	version = src->version;
 	/* Make the latest version visible */
 	smp_rmb();
 
-	offset = pvclock_get_nsec_offset(src);
-	ret = src->system_time + offset;
-	ret_flags = src->flags;
-
-	*cycles = ret;
-	*flags = ret_flags;
+	delta = rdtsc_ordered() - src->tsc_timestamp;
+	offset = pvclock_scale_delta(delta, src->tsc_to_system_mul,
+				   src->tsc_shift);
+	*cycles = src->system_time + offset;
+	*flags = src->flags;
 	return version;
 }
 
-- 
2.6.3

Re: [PATCH 2/3] pvclock: Cleanup to remove function pvclock_get_nsec_offset

[Andy Lutomirski]

On Thu, May 26, 2016 at 11:17 PM, Minfei Huang <mnghuan@gmail.com> wrote:
> Function __pvclock_read_cycles is short enough, so there is no need to
> have another function pvclock_get_nsec_offset to calculate tsc delta.
> It's better to combine it into function __pvclock_read_cycles.
>
> Remove useless variables in function __pvclock_read_cycles.

LGTM.

[PATCH 3/3] pvclock: Add a new wrapper function to only get variable flags

[Minfei Huang]

There is a generic function __pvclock_read_cycles to be used to get both
flags and cycles. For function pvclock_read_flags, it's useless to get
cycles value. To make this function be more effective, add a new wrapper
function to only get variable flags.

Signed-off-by: Minfei Huang <mnghuan@gmail.com>
---
 arch/x86/include/asm/pvclock.h | 12 ++++++++++++
 arch/x86/kernel/pvclock.c      |  3 +--
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/pvclock.h b/arch/x86/include/asm/pvclock.h
index 7c1c895..3c4e5b7 100644
--- a/arch/x86/include/asm/pvclock.h
+++ b/arch/x86/include/asm/pvclock.h
@@ -88,6 +88,18 @@ unsigned __pvclock_read_cycles(const struct pvclock_vcpu_time_info *src,
 	return version;
 }
 
+static __always_inline unsigned int __pvclock_read_flags(
+		const struct pvclock_vcpu_time_info *src, u8 *flags)
+{
+	unsigned int version;
+
+	version = src->version;
+	/* Make the latest version visible */
+	smp_rmb();
+	*flags = src->flags;
+	return version;
+}
+
 struct pvclock_vsyscall_time_info {
 	struct pvclock_vcpu_time_info pvti;
 } __attribute__((__aligned__(SMP_CACHE_BYTES)));
diff --git a/arch/x86/kernel/pvclock.c b/arch/x86/kernel/pvclock.c
index 7f82fe0..138772c 100644
--- a/arch/x86/kernel/pvclock.c
+++ b/arch/x86/kernel/pvclock.c
@@ -61,11 +61,10 @@ void pvclock_resume(void)
 u8 pvclock_read_flags(struct pvclock_vcpu_time_info *src)
 {
 	unsigned version;
-	cycle_t ret;
 	u8 flags;
 
 	do {
-		version = __pvclock_read_cycles(src, &ret, &flags);
+		version = __pvclock_read_flags(src, &flags);
 		/* Make sure that the version double-check is last. */
 		smp_rmb();
 	} while ((src->version & 1) || version != src->version);
-- 
2.6.3

Re: [PATCH 3/3] pvclock: Add a new wrapper function to only get variable flags

[Andy Lutomirski]

On Thu, May 26, 2016 at 11:17 PM, Minfei Huang <mnghuan@gmail.com> wrote:
> There is a generic function __pvclock_read_cycles to be used to get both
> flags and cycles. For function pvclock_read_flags, it's useless to get
> cycles value. To make this function be more effective, add a new wrapper
> function to only get variable flags.

Why not just get rid of this function entirely?

Re: [PATCH 3/3] pvclock: Add a new wrapper function to only get variable flags

[Paolo Bonzini]

On 27/05/2016 17:40, Andy Lutomirski wrote:
> On Thu, May 26, 2016 at 11:17 PM, Minfei Huang <mnghuan@gmail.com> wrote:
>> There is a generic function __pvclock_read_cycles to be used to get both
>> flags and cycles. For function pvclock_read_flags, it's useless to get
>> cycles value. To make this function be more effective, add a new wrapper
>> function to only get variable flags.
> 
> Why not just get rid of this function entirely?

I agree.  If you want to abstract the retry logic, perhaps you can add
pvclock_read_{begin,retry} functions like seqlock_read_{begin,retry}?

Thanks,

Paolo

Re: [PATCH 3/3] pvclock: Add a new wrapper function to only get variable flags

[Minfei Huang]

On 05/27/16 at 06:06P, Paolo Bonzini wrote:
> 
> 
> On 27/05/2016 17:40, Andy Lutomirski wrote:
> > On Thu, May 26, 2016 at 11:17 PM, Minfei Huang <mnghuan@gmail.com> wrote:
> >> There is a generic function __pvclock_read_cycles to be used to get both
> >> flags and cycles. For function pvclock_read_flags, it's useless to get
> >> cycles value. To make this function be more effective, add a new wrapper
> >> function to only get variable flags.
> > 
> > Why not just get rid of this function entirely?
> 
> I agree.  If you want to abstract the retry logic, perhaps you can add

Hmm, I will get rid of pvclock_read_flags, and repost it.

Thanks
Minfei

> pvclock_read_{begin,retry} functions like seqlock_read_{begin,retry}?
> 
> Thanks,
> 
> Paolo

[PATCH 3/3 V2] pvclock: Get rid of __pvclock_read_cycles in function pvclock_read_flags

[Minfei Huang]

There is a generic function __pvclock_read_cycles to be used to get both
flags and cycles. For function pvclock_read_flags, it's useless to get
cycles value. To make this function be more effective, get this variable
flags directly in function.

Signed-off-by: Minfei Huang <mnghuan@gmail.com>
---
v1:
- Get rid of __pvclock_read_cycles according to Andy's suggestion
---
 arch/x86/kernel/pvclock.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/pvclock.c b/arch/x86/kernel/pvclock.c
index 7f82fe0..06c58ce 100644
--- a/arch/x86/kernel/pvclock.c
+++ b/arch/x86/kernel/pvclock.c
@@ -61,11 +61,14 @@ void pvclock_resume(void)
 u8 pvclock_read_flags(struct pvclock_vcpu_time_info *src)
 {
 	unsigned version;
-	cycle_t ret;
 	u8 flags;
 
 	do {
-		version = __pvclock_read_cycles(src, &ret, &flags);
+		version = src->version;
+		/* Make the latest version visible */
+		smp_rmb();
+
+		flags = src->flags;
 		/* Make sure that the version double-check is last. */
 		smp_rmb();
 	} while ((src->version & 1) || version != src->version);
-- 
2.6.3

Re: [PATCH 3/3 V2] pvclock: Get rid of __pvclock_read_cycles in function pvclock_read_flags

[Minfei Huang]

ping. Any comment is appreciate.

Thanks
Minfei

On 05/28/16 at 08:27P, Minfei Huang wrote:
> There is a generic function __pvclock_read_cycles to be used to get both
> flags and cycles. For function pvclock_read_flags, it's useless to get
> cycles value. To make this function be more effective, get this variable
> flags directly in function.
> 
> Signed-off-by: Minfei Huang <mnghuan@gmail.com>
> ---
> v1:
> - Get rid of __pvclock_read_cycles according to Andy's suggestion
> ---
>  arch/x86/kernel/pvclock.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/pvclock.c b/arch/x86/kernel/pvclock.c
> index 7f82fe0..06c58ce 100644
> --- a/arch/x86/kernel/pvclock.c
> +++ b/arch/x86/kernel/pvclock.c
> @@ -61,11 +61,14 @@ void pvclock_resume(void)
>  u8 pvclock_read_flags(struct pvclock_vcpu_time_info *src)
>  {
>  	unsigned version;
> -	cycle_t ret;
>  	u8 flags;
>  
>  	do {
> -		version = __pvclock_read_cycles(src, &ret, &flags);
> +		version = src->version;
> +		/* Make the latest version visible */
> +		smp_rmb();
> +
> +		flags = src->flags;
>  		/* Make sure that the version double-check is last. */
>  		smp_rmb();
>  	} while ((src->version & 1) || version != src->version);
> -- 
> 2.6.3
> 

Re: [PATCH 3/3 V2] pvclock: Get rid of __pvclock_read_cycles in function pvclock_read_flags

[Paolo Bonzini]

On 07/06/2016 15:16, Minfei Huang wrote:
> ping. Any comment is appreciate.

The patch looks good, I'll follow up with another to introduce the
seqcount-like read_begin/read_retry API.

Thanks,

Paolo

> On 05/28/16 at 08:27P, Minfei Huang wrote:
>> There is a generic function __pvclock_read_cycles to be used to get both
>> flags and cycles. For function pvclock_read_flags, it's useless to get
>> cycles value. To make this function be more effective, get this variable
>> flags directly in function.
>>
>> Signed-off-by: Minfei Huang <mnghuan@gmail.com>
>> ---
>> v1:
>> - Get rid of __pvclock_read_cycles according to Andy's suggestion
>> ---
>>  arch/x86/kernel/pvclock.c | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/kernel/pvclock.c b/arch/x86/kernel/pvclock.c
>> index 7f82fe0..06c58ce 100644
>> --- a/arch/x86/kernel/pvclock.c
>> +++ b/arch/x86/kernel/pvclock.c
>> @@ -61,11 +61,14 @@ void pvclock_resume(void)
>>  u8 pvclock_read_flags(struct pvclock_vcpu_time_info *src)
>>  {
>>  	unsigned version;
>> -	cycle_t ret;
>>  	u8 flags;
>>  
>>  	do {
>> -		version = __pvclock_read_cycles(src, &ret, &flags);
>> +		version = src->version;
>> +		/* Make the latest version visible */
>> +		smp_rmb();
>> +
>> +		flags = src->flags;
>>  		/* Make sure that the version double-check is last. */
>>  		smp_rmb();
>>  	} while ((src->version & 1) || version != src->version);
>> -- 
>> 2.6.3
>>
> 

Re: [PATCH 3/3 V2] pvclock: Get rid of __pvclock_read_cycles in function pvclock_read_flags

[Borislav Petkov]

On Sat, May 28, 2016 at 08:27:43PM +0800, Minfei Huang wrote:
> There is a generic function __pvclock_read_cycles to be used to get both
> flags and cycles. For function pvclock_read_flags, it's useless to get
> cycles value. To make this function be more effective, get this variable
> flags directly in function.
> 
> Signed-off-by: Minfei Huang <mnghuan@gmail.com>
> ---
> v1:
> - Get rid of __pvclock_read_cycles according to Andy's suggestion
> ---
>  arch/x86/kernel/pvclock.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/pvclock.c b/arch/x86/kernel/pvclock.c
> index 7f82fe0..06c58ce 100644
> --- a/arch/x86/kernel/pvclock.c
> +++ b/arch/x86/kernel/pvclock.c
> @@ -61,11 +61,14 @@ void pvclock_resume(void)
>  u8 pvclock_read_flags(struct pvclock_vcpu_time_info *src)
>  {
>  	unsigned version;
> -	cycle_t ret;
>  	u8 flags;
>  
>  	do {
> -		version = __pvclock_read_cycles(src, &ret, &flags);
> +		version = src->version;
> +		/* Make the latest version visible */
> +		smp_rmb();
> +
> +		flags = src->flags;
>  		/* Make sure that the version double-check is last. */

What does that comment mean over the barrier? It should be over the
"while" line IMO.

>  		smp_rmb();

Why the two barriers back-to-back? Can't have one at the end for all?

>  	} while ((src->version & 1) || version != src->version);

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

Re: [PATCH 3/3 V2] pvclock: Get rid of __pvclock_read_cycles in function pvclock_read_flags

[Paolo Bonzini]

On 08/06/2016 10:17, Borislav Petkov wrote:
>> > -		version = __pvclock_read_cycles(src, &ret, &flags);
>> > +		version = src->version;
>> > +		/* Make the latest version visible */
>> > +		smp_rmb();
>> > +
>> > +		flags = src->flags;
>> >  		/* Make sure that the version double-check is last. */
> What does that comment mean over the barrier? It should be over the
> "while" line IMO.
> 
>> >  		smp_rmb();
> Why the two barriers back-to-back? Can't have one at the end for all?
> 

This is basically implementing a seqcount.  It needs two barriers and,
technically, they should be virt_rmb() -- it really doesn't matter of
course because reads are never reordered on x86.

Paolo

Re: [PATCH 3/3 V2] pvclock: Get rid of __pvclock_read_cycles in function pvclock_read_flags

[Borislav Petkov]

On Thu, Jun 09, 2016 at 01:21:18PM +0200, Paolo Bonzini wrote:
> This is basically implementing a seqcount.  It needs two barriers and,

Why does it need the two barriers? More details please.

> technically, they should be virt_rmb() -- it really doesn't matter of
> course because reads are never reordered on x86.

You mean

	version = src->version;
	flags = src->flags;

are not reordered?

I don't think so.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

Re: [PATCH 3/3 V2] pvclock: Get rid of __pvclock_read_cycles in function pvclock_read_flags

[Paolo Bonzini]

On 09/06/2016 13:28, Borislav Petkov wrote:
>> > technically, they should be virt_rmb() -- it really doesn't matter of
>> > course because reads are never reordered on x86.
> You mean
> 
> 	version = src->version;
> 	flags = src->flags;
> 
> are not reordered?
> 
> I don't think so.

The compiler can reorder them, so smp_rmb() and virt_rmb() have to be
barrier(), but the processor won't.

x86 will only move a store after a subsequent load, if you exclude
special cases such as write combining, non-temporal moves and the like.
LFENCE and SFENCE are only needed for those special cases.

Paolo

Re: [PATCH 3/3 V2] pvclock: Get rid of __pvclock_read_cycles in function pvclock_read_flags

[Minfei Huang]

On 06/09/16 at 01:28P, Borislav Petkov wrote:
> On Thu, Jun 09, 2016 at 01:21:18PM +0200, Paolo Bonzini wrote:
> > This is basically implementing a seqcount.  It needs two barriers and,
> 
> Why does it need the two barriers? More details please.

Hi, Borislav.

It's a seqcount-like. We should confirm that reading flags occurs
between two reading version. To make complier do not reorder, two
barriers are necessary.

Thanks
Minfei

Re: [PATCH 3/3 V2] pvclock: Get rid of __pvclock_read_cycles in function pvclock_read_flags

[Peter Zijlstra]

On Sat, May 28, 2016 at 08:27:43PM +0800, Minfei Huang wrote:
> +++ b/arch/x86/kernel/pvclock.c
> @@ -61,11 +61,14 @@ void pvclock_resume(void)
>  u8 pvclock_read_flags(struct pvclock_vcpu_time_info *src)
>  {
>  	unsigned version;
> -	cycle_t ret;
>  	u8 flags;
>  
>  	do {
> -		version = __pvclock_read_cycles(src, &ret, &flags);
> +		version = src->version;
> +		/* Make the latest version visible */
> +		smp_rmb();
> +
> +		flags = src->flags;

Using a seqcount to load a single byte is insane ;-)

>  		/* Make sure that the version double-check is last. */
>  		smp_rmb();
>  	} while ((src->version & 1) || version != src->version);


What's wrong with:

	u8 flags = READ_ONCE(src->flags);

?

(and have the flags store be done using WRITE_ONCE() of course).

Sure, if your total state is larger than one word you need the
seqcount for integrity, but reading _one_ byte, shees.

Re: [PATCH 3/3 V2] pvclock: Get rid of __pvclock_read_cycles in function pvclock_read_flags

[Peter Zijlstra]

On Thu, Jun 09, 2016 at 02:16:03PM +0200, Peter Zijlstra wrote:
> What's wrong with:
> 
> 	u8 flags = READ_ONCE(src->flags);
> 
> ?
> 
> (and have the flags store be done using WRITE_ONCE() of course).
> 
> Sure, if your total state is larger than one word you need the
> seqcount for integrity, but reading _one_ byte, shees.

Something like so; although given that I've never seen this code before
I can easily have missed an update site.

diff --git a/arch/x86/kernel/pvclock.c b/arch/x86/kernel/pvclock.c
index 99bfc025111d..7a4bf59aa929 100644
--- a/arch/x86/kernel/pvclock.c
+++ b/arch/x86/kernel/pvclock.c
@@ -60,15 +60,7 @@ void pvclock_resume(void)
 
 u8 pvclock_read_flags(struct pvclock_vcpu_time_info *src)
 {
-	unsigned version;
-	cycle_t ret;
-	u8 flags;
-
-	do {
-		version = __pvclock_read_cycles(src, &ret, &flags);
-	} while ((src->version & 1) || version != src->version);
-
-	return flags & valid_flags;
+	return READ_ONCE(src->flags) & valid_flags;
 }
 
 cycle_t pvclock_clocksource_read(struct pvclock_vcpu_time_info *src)
@@ -83,7 +75,8 @@ cycle_t pvclock_clocksource_read(struct pvclock_vcpu_time_info *src)
 	} while ((src->version & 1) || version != src->version);
 
 	if (unlikely((flags & PVCLOCK_GUEST_STOPPED) != 0)) {
-		src->flags &= ~PVCLOCK_GUEST_STOPPED;
+		flags &= ~PVCLOCK_GUEST_STOPPED;
+		WRITE_ONCE(src->flags, flags);
 		pvclock_touch_watchdogs();
 	}
 
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 1ba3b7d3cae9..54458acce4f5 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1832,7 +1832,7 @@ static int kvm_guest_time_update(struct kvm_vcpu *v)
 	if (use_master_clock)
 		pvclock_flags |= PVCLOCK_TSC_STABLE_BIT;
 
-	vcpu->hv_clock.flags = pvclock_flags;
+	WRITE_ONCE(vcpu->hv_clock.flags, pvclock_flags);
 
 	trace_kvm_pvclock_update(v->vcpu_id, &vcpu->hv_clock);
 

Re: [PATCH 3/3 V2] pvclock: Get rid of __pvclock_read_cycles in function pvclock_read_flags

[Paolo Bonzini]

On 09/06/2016 14:16, Peter Zijlstra wrote:
> On Sat, May 28, 2016 at 08:27:43PM +0800, Minfei Huang wrote:
>> +++ b/arch/x86/kernel/pvclock.c
>> @@ -61,11 +61,14 @@ void pvclock_resume(void)
>>  u8 pvclock_read_flags(struct pvclock_vcpu_time_info *src)
>>  {
>>  	unsigned version;
>> -	cycle_t ret;
>>  	u8 flags;
>>  
>>  	do {
>> -		version = __pvclock_read_cycles(src, &ret, &flags);
>> +		version = src->version;
>> +		/* Make the latest version visible */
>> +		smp_rmb();
>> +
>> +		flags = src->flags;
> 
> Using a seqcount to load a single byte is insane ;-)

Only if you know that the writer will not write that byte twice within a
critical section...

Which I guess we do know in this case because the write side is just a
memcpy, but it's still a bit safer when it's not specified by the
pvclock API.  It's not a fast path anyway, it runs literally twice at
startup.

Paolo

Re: [PATCH 3/3 V2] pvclock: Get rid of __pvclock_read_cycles in function pvclock_read_flags

[Peter Zijlstra]

On Thu, Jun 09, 2016 at 02:26:59PM +0200, Paolo Bonzini wrote:
> 
> 
> On 09/06/2016 14:16, Peter Zijlstra wrote:
> > On Sat, May 28, 2016 at 08:27:43PM +0800, Minfei Huang wrote:
> >> +++ b/arch/x86/kernel/pvclock.c
> >> @@ -61,11 +61,14 @@ void pvclock_resume(void)
> >>  u8 pvclock_read_flags(struct pvclock_vcpu_time_info *src)
> >>  {
> >>  	unsigned version;
> >> -	cycle_t ret;
> >>  	u8 flags;
> >>  
> >>  	do {
> >> -		version = __pvclock_read_cycles(src, &ret, &flags);
> >> +		version = src->version;
> >> +		/* Make the latest version visible */
> >> +		smp_rmb();
> >> +
> >> +		flags = src->flags;
> > 
> > Using a seqcount to load a single byte is insane ;-)
> 
> Only if you know that the writer will not write that byte twice within a
> critical section...
> 
> Which I guess we do know in this case because the write side is just a
> memcpy, but it's still a bit safer when it's not specified by the
> pvclock API.  It's not a fast path anyway, it runs literally twice at
> startup.

Fair enough; just thought it was really silly code.

Re: [PATCH 1/3] pvclock: Add CPU barries to get correct version value

[Andy Lutomirski]

On Thu, May 26, 2016 at 11:17 PM, Minfei Huang <mnghuan@gmail.com> wrote:
> Protocol for the "version" fields is: hypervisor raises it (making it
> uneven) before it starts updating the fields and raises it again (making
> it even) when it is done.  Thus the guest can make sure the time values
> it got are consistent by checking the version before and after reading
> them.
>
> Add CPU barries after getting version value just like what function
> vread_pvclock does, because all of callees in this function is inline.

LGTM.