Il 29/05/2014 16:41, James Hogan ha scritto:
+
+ /* If VM clock stopped then state was already saved when it was stopped */
+ if (runstate_is_running()) {
+ ret = kvm_mips_save_count(cs);
+ if (ret < 0) {
+ return ret;
+ }
+ }
+
You're expecting that calls to kvm_mips_get_cp0_registers and
kvm_mips_put_cp0_registers are balanced and not nested. Perhaps you
should add an assert about it.
+ if (!(count_ctl & KVM_REG_MIPS_COUNT_CTL_DC)) {
+ count_ctl |= KVM_REG_MIPS_COUNT_CTL_DC;
+ ret = kvm_mips_put_one_reg64(cs, KVM_REG_MIPS_COUNT_CTL, &count_ctl);
+ if (ret < 0) {
+ return ret;
+ }
+ }
Would it make sense to return directly if the master disable bit is set?
The rest of this function is idempotent.
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? 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).
And if the VM is not running, you have timer_state.cpu_ticks_enabled ==
false, so cpu_get_clock_at() always returns timers_state.cpu_clock_offset.
So, if the COUNT_RESUME write is not necessary for a running VM, you can
then just write get_clock() to COUNT_RESUME, which seems to make sense
to me.
Paolo
