Hi Paolo,
On 29/05/14 18:03, Paolo Bonzini wrote:
>>> Also, perhaps this bit in kvm_mips_restore_count is unnecessary, and so
>>> is env->count_save_time in general:
>>>
>>>> + /* find time to resume the saved timer at */
>>>> + now = get_clock();
>>>> + count_resume = now - (cpu_get_clock_at(now) -
>>>> env->count_save_time);
>>>
>>> Is the COUNT_RESUME write necessary if the VM is running?
>>
>> Running at that instant or running continuously since the save?
>>
>> At this instant the VM is always running. Either it's just been started
>> and other state isn't dirty, or the registers have been put while the VM
>> is running.
>
> The possible transitions are:
>
> running, not dirty -> stopped
> need to freeze and load the registers
>
> stopped -> running, not dirty
> will reload the registers, need to modify COUNT_RESUME
>
> running, dirty -> stopped
> no need to do anything
>
> stopped -> running, dirty
> will not reload the registers until put, will need to modify
> COUNT_RESUME on the next transition to "running, not dirty"
>
> running, not dirty -> running, dirty
> need to freeze and load the registers
>
> running, dirty -> running, not dirty
> need to modify COUNT_RESUME if the machine had been stopped
> in the meanwhile
>
> The questions then is, can we skip tracking count_save_time and
> modifying COUNT_RESUME in kvm_mips_restore_count? Then you can just
> write get_clock() to COUNT_RESUME in kvm_mips_update_state, like this:
>
> if (!running) {
> if (!cs->kvm_vcpu_dirty) {
> save;
> }
> else {
> write get_clock() to COUNT_RESUME;
> if (!cs->kvm_vcpu_dirty) {
> restore;
> }
> }
>
> and even drop patch 1. COUNT_RESUME is not even ever read by QEMU nor
> stored in CPUState, so.
>
> The difference is that the guest "loses" the time between the "running,
> not dirty -> running, dirty" and "running, dirty -> stopped"
> transitions, while "gaining" the time between "stopped -> running,
> dirty" and "running, dirty -> running, not dirty". If this is right, I
> think the difference does not matter in practice and the new/simpler
> code even explains the definition of COUNT_RESUME better in my eyes.
Yes, I agree with your analysis and had considered something like this,
although it doesn't particularly appeal to my sense of perfectionism :).
It would be race free though, and if you're stopping the VM at all you
expect to lose some time anyway.
>
>>> Does the
>>> master disable bit just latch the values, or does it really stop the
>>> timer? (My reading of the code is the former, since writing
>>> COUNT_RESUME only modifies the bias: no write => no bias change => timer
>>> runs).
>>
>> It appears latched in the sense that starting it again will jump Count
>> forward to the time it would have been had it not been disabled (with no
>> loss of Compare interrupt in that time).
>
> Yes, this is the important part because it means that the guest clock
> does not get progressively more skewed. It also means that it is right
> to never write COUNT_RESUME except if you go through stop/continue.
Yes, if the VM hasn't been stopped the value written is unchanged from
that originally read (see below), so it could skip it in that case.
save:
count_save_time = cpu_clock_offset + COUNT_RESUME
restore:
COUNT_RESUME = get_clock() - (cpu_clock_offset + get_clock() -
count_save_time)
= count_save_time - cpu_clock_offset
= COUNT_RESUME
Cheers
James
