KVM Clock

[Roger Pau Monné]

On 15/01/14 14:45, Julian Stecklina wrote:
> 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.

According to the operator precedence and associativity in C
(http://en.wikipedia.org/wiki/Operators_in_C_and_C%2B%2B#Operator_precedence),
if I'm reading it right, the condition in the while line will be
evaluated as follows (because of the left-to-right associativity of the
| operator):

1. (src->version & 1)
2. (dst->version ^ src->version)
3. result of 1 | result of 2

So AFAICT the flow that you describe could never happen, because
(src->version & 1) is always done before (dst->version ^ src->version).

Roger.