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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html