KVM Clock

[Julian Stecklina]

On Mi, 2014-01-15 at 14:08 +0100, Roger Pau Monné wrote:
> On 15/01/14 13:05, Julian Stecklina wrote:
> > On 01/14/2014 05:13 PM, Peter Grehan wrote:
> >> Hi Julian,
> >> 
> >>> is anyone working on KVM clock support for FreeBSD? If not, I
> >>> might take a shot at it.
> >> 
> >> None I know of: go for it :)
> > 
> > Works for me so far: 
> > https://github.com/blitz/freebsd/commit/cdc5f872b3e48cc0dda031fc7d6bdedc65c3148f
> 
> Looking
> > 
> at the code it seems some common routines could be shared
> between the KVM PV clock and the Xen PV clock
> (sys/dev/xen/timer/timer.c). The data passed from the hypervisor to
> the guest has exactly the same format (see struct vcpu_time_info in
> Xen public headers).

Yes, I know. Didn't get around to making it pretty yesterday evening. ;)
I'll post an updated patch, when I have some time. Any suggestions where
to put the two common functions?

> At a first sight the KVM clock can benefit from using scale_delta
> (which is going to be faster than the C version implemented in
> kvmclock_get_timecount),

At least somewhat on amd64. 32-bit looks pretty identical.

>  and xen_fetch_vcpu_tinfo is exactly the same
> as kvmclock_fetch.

I think xen_fetch_vcpu_tinfo has a subtle bug:
  217         do {
  218                 dst->version = src->version;
  219                 rmb();
  220                 dst->tsc_timestamp = src->tsc_timestamp;
  221                 dst->system_time = src->system_time;
  222                 dst->tsc_to_system_mul = src->tsc_to_system_mul;
  223                 dst->tsc_shift = src->tsc_shift;
  224                 rmb();
  225         } while ((src->version & 1) | (dst->version ^
src->version));

In line 225 src->version is potentially read twice. If you consider the
following schedule:

host starts updating data, sets src->version to 1
guest reads src->version (1) and stores it into dst->version.
guest copies inconsistent data
guest reads src->version (1) and computes xor with dst->version.
host finishes updating data and sets src->version to 2
guest reads src->version (2) and checks whether lower bit is not set.
while loop exits with inconsistent data!

I think the C standard (at least C11) permits this as it does not
specify in which order the two reads in line 225 need to happen.

Julian