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>
index | next in thread | previous in thread | raw e-mail
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. Julianhome | help
Want to link to this message? Use this
URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1389793530.2668.13.camel>