From mboxrd@z Thu Jan  1 00:00:00 1970
From: Paolo Bonzini <pbonzini@redhat.com>
Subject: Re: [PATCH v2 00/23] MIPS: KVM: Fixes and guest timer rewrite
Date: Thu, 29 May 2014 17:23:36 +0200
Message-ID: <538750F8.7040202@redhat.com>
References: <1401355005-20370-1-git-send-email-james.hogan@imgtec.com> <53870DAD.7050900@redhat.com> <53874719.5070604@imgtec.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-15; format=flowed
Content-Transfer-Encoding: 7bit
Cc: Andreas Herrmann <andreas.herrmann@caviumnetworks.com>,
	Gleb Natapov <gleb@kernel.org>, kvm@vger.kernel.org,
	Ralf Baechle <ralf@linux-mips.org>, linux-mips@linux-mips.org,
	David Daney <david.daney@cavium.com>,
	Sanjay Lal <sanjayl@kymasys.com>
To: James Hogan <james.hogan@imgtec.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:36840 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1757620AbaE2PYx (ORCPT <rfc822;kvm@vger.kernel.org>);
	Thu, 29 May 2014 11:24:53 -0400
In-Reply-To: <53874719.5070604@imgtec.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

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