Re: [PATCH v2 00/23] MIPS: KVM: Fixes and guest timer rewrite

[James Hogan <james.hogan@imgtec.com>]

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