From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49261) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WQaAq-0005tc-Ru for qemu-devel@nongnu.org; Thu, 20 Mar 2014 06:26:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WQaAj-0000fo-3o for qemu-devel@nongnu.org; Thu, 20 Mar 2014 06:26:08 -0400 Received: from [217.156.133.130] (port=55502 helo=imgpgp01.kl.imgtec.org) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WQaAi-0000fI-PQ for qemu-devel@nongnu.org; Thu, 20 Mar 2014 06:26:01 -0400 Message-ID: <532ABBA1.3060507@imgtec.com> Date: Thu, 20 Mar 2014 09:57:53 +0000 From: James Hogan 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> <5329C5F4.2000405@redhat.com> In-Reply-To: <5329C5F4.2000405@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="jX3EVp9BbJV8esxK8Agp5SHddMBubN0lm" 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: Paolo Bonzini Cc: Aurelien Jarno , Gleb Natapov , qemu-devel@nongnu.org, kvm@vger.kernel.org, Sanjay Lal --jX3EVp9BbJV8esxK8Agp5SHddMBubN0lm Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: quoted-printable On 19/03/14 16:29, Paolo Bonzini wrote: > 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 =3D timer_new_ns(QEMU_CLOCK_VIRTUAL, &mips_timer_cb, e= nv); >> - env->CP0_Compare =3D 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 =3D timer_new_ns(QEMU_CLOCK_VIRTUAL, &mips_timer_c= b, >> env); >> + env->CP0_Compare =3D 0; >> + cpu_mips_store_count(env, 1); >> + } >> } >> >=20 > I hate to make you do unrelated changes, but... initializing CP0_Compar= e > is unnecessary, it should already be 0; You mean because of the memset in object_initialize_with_type, when object_new is called? Although that wouldn't handle reset, although technically the reset state of Compare is undefined. > 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. >=20 > I'm asking because while >=20 > if (!kvm_enabled()) { > env->timer =3D ... > qemu_register_reset(...); > } >=20 > is fine, changing values of registers conditionally is not. Okay, makes sense. >=20 > Also, I noticed two things in the implementation of the CPU timer that > should be fixed: >=20 > 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). Agreed. I'm in the middle of fixing the count/compare timer in KVM to be based on real time (ktime_get()), so I'll make it default to 100MHz to match QEMU for now. I can imagine it being useful to be able to control it too depending on whether you're running on a slow FPGA/emulator or fast silicon. > 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. True, I hadn't considered qemu writing those registers yet. Am I right that the correct way to prevent clock drift is for kvm_arch_put_registers to only set the Count register if level !=3D KVM_PUT_RUNTIME_STATE? > 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? Yes, you appear to be right! Thanks a lot for reviewing Cheers James --jX3EVp9BbJV8esxK8Agp5SHddMBubN0lm Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.13 (GNU/Linux) iQIcBAEBAgAGBQJTKruhAAoJEGwLaZPeOHZ6Xm0QALzlznKYbb2o7j+6gOyMc9FG 3FgOFBlqYDatdsqiuVrF+WwFs0YCeVbZzHLxx3OiRmeongFUs2rexinpUKXHMjHU enajXwkRBY7wKjvLru5ECZ+rkM1wKHXdvv5c4+KG3yHSmqgCxQryUJ5sMRwIIAqN oUCuWEeOxdN1vU6EHnc4KByGDFEVkyy/1RGHwLsZhi0L+iUaIOGpVRF52wxFB8Vj Q9AxwHPfXHM1rVM8kXQG75U6/KUsovmmmbtKdnPBG3dT3MPLeHn/ABb+ls+r2Wwo B2t15t5AsaCce9xeY3Y4dF7NNM2S2MK8i0mTwsTWq75nQtcKMFDp2Byvr5Pf5PXQ OV5bQTcWn/7j8DgaseFtsOISah1v+GUKYljTn+OUPJAYKlPq/D3sEhNcbbYhBgQ0 z9HK0r5b4vwuLS/H4AtJs6Er/p9JN1IFj14R1e0EJvBRKGgw12aDBsSS6TbDziW4 29Y/vFhfx5gMyKJN8saSHtfmOZfHgA/Nzi5ijakV1pIetl/b+ln9neddTEacSnCW WHCw+dVJCQDBPXCOXpwT9cc6m+GRjASf6DjhtopaNPLCYsUPOrpWR62NJ7osr0/S p/6viLiXGWRVhU18LKN3e7A1OgwKQi74C+fZ5eIYn2frZ7+jDjVJ1JgCdLYfnUno orCUkhe+eAOhMl1bQ/Xm =mDL5 -----END PGP SIGNATURE----- --jX3EVp9BbJV8esxK8Agp5SHddMBubN0lm--