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

[Paolo Bonzini <pbonzini@redhat.com>]

Il 29/05/2014 18:27, James Hogan ha scritto:
> (although as it stands CP0_Count never represents the offset from the VM
> clock for KVM like it does with a running Count with TCG, so the vmstate
> is technically incompatible between TCG/KVM).

That can be fixed in cpu_save/cpu_load hooks, like

     if (kvm_enabled()) {
         uint32_t TCGlike_CP0_Count = ...
         qemu_put_sbe32s(f, &TCGlike_CP0_Count);
     } else {
         qemu_put_sbe32s(f, &env->CP0_Count);
     }

...

     if (kvm_enabled()) {
         uint32_t TCGlike_CP0_Count;
         qemu_get_sbe32s(f, &TCGlike_CP0_Count);
         env->CP0_Count = ...
     } else {
         qemu_get_sbe32s(f, &env->CP0_Count);
     }

>> 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.

>> 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.

Paolo