Date: Wed, 15 Jan 2014 14:45:30 +0100 From: Julian Stecklina <jsteckli@os.inf.tu-dresden.de> To: Roger Pau =?ISO-8859-1?Q?Monn=E9?= <roger.pau@citrix.com> Cc: freebsd-virtualization@freebsd.org Subject: Re: KVM Clock Message-ID: <1389793530.2668.13.camel@janus.xn--pl-wia.net> In-Reply-To: <52D68831.2000503@citrix.com> References: <lb3jnb$qo8$1@ger.gmane.org> <52D5623C.7060401@freebsd.org> <lb5thm$3r3$1@ger.gmane.org> <52D68831.2000503@citrix.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1389793530.2668.13.camel>