From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39886) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WQJy2-00084l-Qn for qemu-devel@nongnu.org; Wed, 19 Mar 2014 13:07:55 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WQJxy-0006vp-Nq for qemu-devel@nongnu.org; Wed, 19 Mar 2014 13:07:50 -0400 Received: from mx1.redhat.com ([209.132.183.28]:42675) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WQJxy-0006vk-FN for qemu-devel@nongnu.org; Wed, 19 Mar 2014 13:07:46 -0400 Message-ID: <5329C5F4.2000405@redhat.com> Date: Wed, 19 Mar 2014 17:29:40 +0100 From: Paolo Bonzini MIME-Version: 1.0 References: <1394801281-18997-1-git-send-email-james.hogan@imgtec.com> <1394801281-18997-2-git-send-email-james.hogan@imgtec.com> In-Reply-To: <1394801281-18997-2-git-send-email-james.hogan@imgtec.com> Content-Type: text/plain; charset=ISO-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v4 01/10] hw/mips/cputimer: Don't start periodic timer in KVM mode List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: James Hogan , qemu-devel@nongnu.org Cc: Sanjay Lal , Gleb Natapov , Aurelien Jarno , kvm@vger.kernel.org Il 14/03/2014 13:47, James Hogan ha scritto: > From: Sanjay Lal > > Compare/Count timer interrupts are handled in-kernel for KVM, so don't > bother starting it in QEMU. > > Signed-off-by: Sanjay Lal > Signed-off-by: James Hogan > Reviewed-by: Aurelien Jarno > --- > Changes in v2: > - Expand commit message > - Rebase on v1.7.0 > - Wrap comment > --- > hw/mips/cputimer.c | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/hw/mips/cputimer.c b/hw/mips/cputimer.c > index c8b4b00..52570fd 100644 > --- a/hw/mips/cputimer.c > +++ b/hw/mips/cputimer.c > @@ -23,6 +23,7 @@ > #include "hw/hw.h" > #include "hw/mips/cpudevs.h" > #include "qemu/timer.h" > +#include "sysemu/kvm.h" > > #define TIMER_FREQ 100 * 1000 * 1000 > > @@ -141,7 +142,13 @@ static void mips_timer_cb (void *opaque) > > void cpu_mips_clock_init (CPUMIPSState *env) > { > - env->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, &mips_timer_cb, env); > - env->CP0_Compare = 0; > - cpu_mips_store_count(env, 1); > + /* > + * If we're in KVM mode, don't start the periodic timer, that is handled in > + * kernel. > + */ > + if (!kvm_enabled()) { > + env->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, &mips_timer_cb, env); > + env->CP0_Compare = 0; > + cpu_mips_store_count(env, 1); > + } > } > I hate to make you do unrelated changes, but... initializing CP0_Compare is unnecessary, it should already be 0; and for CP0_Count it should not be done here. but in cpu_state_reset function. Then here you can call qemu_register_reset to register another reset callback, and call cpu_mips_timer_update in that callback. I'm asking because while if (!kvm_enabled()) { env->timer = ... qemu_register_reset(...); } is fine, changing values of registers conditionally is not. Also, I noticed two things in the implementation of the CPU timer that should be fixed: 1) right now the hypervisor's frequency is hardcoded to 1/4th of the host, while QEMU's is 100 MHz. It would be nice to make them either consistent, or customizable (you can use another ONE_REG interface to set CPU parameters). 2) in KVM, CP0_Count does not start at the same value on guest reset. There is a comment that "Linux doesn't seem to write into COUNT", but QEMU does. So KVM should implement CP0_Count writes and adjust the "bias" of the guest CP0_Count. In fact, right now kvm_mips_te_put_cp0_registers should always return -EINVAL because KVM_REG_MIPS_CP0_COUNT is not handled in kvm_mips_get/set_reg. Am I missing something? Paolo