Re: [PATCH 5/5] KVM: paravirt time source

Anthony Liguori
2007-06-18T21:52:57Z

Jeremy Fitzhardinge wrote:
> Anthony Liguori wrote:
>
>> +static cycle_t read_hyper(void)
>> +{
>> + struct timespec now;
>> + int ret;
>> +
>> + ret = kvm_hypercall(KVM_HYPERCALL_GET_KTIME, (u32)&now, 0, 0, 0);
>> + WARN_ON(ret);
>> +
>> + return now.tv_nsec + now.tv_sec * (cycles_t)1e9;
>>
>>
>
> Hm, use of FP looks pretty odd. I guess its OK to assume the compiler
> will completely remove all the FP stuff at compile time. Or you could
> use NSEC_PER_SEC.
>

Agreed.

>> +}
>> +
>> +static struct clocksource clocksource_hyper = {
>> + .name = "hyper",
>> + .rating = 200,
>>
>>
>
> We should probably standardize on this. I guess that if you're in a
> paravirt environment, and there's a paravirt clocksource, that would
> always be the best clocksource to use.
>
>
>> + .read = read_hyper,
>> + .mask = CLOCKSOURCE_MASK(64),
>> + .mult = 1,
>> + .shift = 0,
>>
>>
>
> It would be better to use a scale and shift here, so that adjtime has
> something to work with when warping time.
>

Okay. I may remove this patch from the patch series and attempt to sit
down next week and work out something more complete that also implements
stolen time accounting.

Regards,

Anthony Liguori

> J