Re: kvmclock - the host part.

[Avi Kivity <avi@qumranet.com>]

Glauber de Oliveira Costa wrote:
>>>  void kvm_inject_pending_timer_irqs(struct kvm_vcpu *vcpu)
>>>  {
>>> +	vcpu->time_needs_update = 1;
>>>   
>>>       
>> Why here and not in __vcpu_run()?  It isn't timer irq related.
>>     
> Because my plan was exactly, updating it at each timer interrupt.
>   

I think kvm_inject_pending_timer_irqs() is called every __vcpu_run(), so 
your cunning plan has been foiled.

Did you mean each guest interrupt of host interrupt?

> There's a trade off between
> updating every run (hopefully more precision, but more overhead), versus
> updating at timer irqs, or other events.
>
> What would you prefer?
>   

I think that we should update it every time a heavyweight exit has been 
taken.  That takes care of the tradeoff quite nicely -- heavyweight 
exits are already dog slow.

>   
>
>>> +	/* Updating the tsc count is the first thing we do */
>>> +	kvm_get_msr(vcpu, MSR_IA32_TIME_STAMP_COUNTER, &vcpu->hv_clock.last_tsc);
>>> +	ktime_get_ts(&ts);
>>> +	vcpu->hv_clock.now_ns = ts.tv_nsec + (NSEC_PER_SEC * (u64)ts.tv_sec);
>>> +	vcpu->hv_clock.wc_sec = get_seconds();
>>> +	vcpu->hv_clock.version++;
>>> +
>>> +	clock_addr = vcpu->clock_addr;
>>> +	memcpy(clock_addr, &vcpu->hv_clock, sizeof(vcpu->hv_clock));
>>> +	mark_page_dirty(vcpu->kvm, vcpu->clock_gfn);
>>>   
>>>       
>> Just use kvm_write_guest().
>>     
> Too slow. Updating guest time, even only in timer interrupts, was a too
> frequent operation, and the kmap / kunmap (atomic) at every iteration
> deemed the whole thing
> unusable.
>   

kvm_write_guest() will eventually be a copy_to_user(), so you need not 
fear the overhead.


>>>  
>>> +	ret = 0;
>>>  	switch (nr) {
>>> +	case  KVM_HCALL_REGISTER_CLOCK: {
>>> +		struct kvm_vcpu *dst_vcpu;
>>> +
>>> +		if (!((a1 < KVM_MAX_VCPUS) && (vcpu->kvm->vcpus[a1]))) {
>>> +			ret = -KVM_EINVAL;
>>> +			break;
>>> +		}
>>> +
>>> +		dst_vcpu = vcpu->kvm->vcpus[a1];
>>>   
>>>       
>> What if !dst_vcpu?  What about locking?
>>
>> Suggest simply using vcpu.  Every guest cpu can register its own
>>     
> Earlier version had a check for !dst_vcpu, you are absolutely right.
>
> Locking was not a problem in practice, because these operations are done
>  serialized, by the same cpu.
>   

Think evil guest that cares not for the well-being of the host.

> This hypercall is called by cpu_up, which, at least in the beginning,
> it's called by cpu0. And that's why each vcpu cannot register its own.
> (And why we don't need locking).
>
> Well, theorectically each vcpu do can register its own clocksource, it
> will just be a little bit more complicated, we have to fire out an IPI,
> and have the other cpu to catch it, and call the hypercall.
>
>   

Can it not be done via the processor startup sequence?  Then there's no 
need for ipis and locking.

I imagine a normal guest initializes the apic in the same way.

> But I honestly don't like it.
> Usually, the cpu leaves start_secondary with a clock already registered,
> so the kernel relies on it.
>
>   
>>> +		dst_vcpu->clock_page = gfn_to_page(vcpu->kvm, a0 >> PAGE_SHIFT);
>>>   
>>>       
>> Shift right?  Why?
>>     
> a0 is not a gfn, but a physical address.
>   

What if the guest wants to place it in address 5GB?  That's unlikely for 
Linux and Windows, but let's do it right anyway.

-- 
error compiling committee.c: too many arguments to function