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
-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
kvm-devel mailing list
kvm-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-devel
