From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric B Munson Subject: Re: [PATCH V3] Guest stop notification Date: Mon, 5 Dec 2011 07:58:54 -0500 Message-ID: <20111205125854.GA5673@mgebm.net> References: <1322853560-24152-1-git-send-email-emunson@mgebm.net> <4ED9331B.9060708@web.de> <20111202212726.GA5662@mgebm.net> <4ED9E6B0.2000903@web.de> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="2oS5YaxWCcQjTEyO" Cc: "qemu-devel@nongnu.org" , Avi Kivity , Marcelo Tosatti , "ryanh@linux.vnet.ibm.com" , "aliguori@us.ibm.com" , "kvm@vger.kernel.org" To: Jan Kiszka Return-path: Received: from oz.csail.mit.edu ([128.30.30.239]:32780 "EHLO mail.mgebm.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932100Ab1LEM65 (ORCPT ); Mon, 5 Dec 2011 07:58:57 -0500 Content-Disposition: inline In-Reply-To: <4ED9E6B0.2000903@web.de> Sender: kvm-owner@vger.kernel.org List-ID: --2oS5YaxWCcQjTEyO Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Sat, 03 Dec 2011, Jan Kiszka wrote: > On 2011-12-02 22:27, Eric B Munson wrote: > > On Fri, 02 Dec 2011, Jan Kiszka wrote: > >=20 > >> On 2011-12-02 20:19, Eric B Munson wrote: > >>> Often when a guest is stopped from the qemu console, it will report s= purious > >>> soft lockup warnings on resume. There are kernel patches being discu= ssed that > >>> will give the host the ability to tell the guest that it is being sto= pped and > >>> should ignore the soft lockup warning that generates. > >>> > >>> Signed-off-by: Eric B Munson > >>> Cc: Avi Kivity > >>> Cc: Marcelo Tosatti > >>> Cc: Jan Kiszka > >>> Cc: ryanh@linux.vnet.ibm.com > >>> Cc: aliguori@us.ibm.com > >>> Cc: kvm@vger.kernel.org > >>> > >>> --- > >>> Changes from V2: > >>> Move ioctl into hw/kvmclock.c so as other arches can use it as it is > >>> implemented > >>> > >>> Changes from V1: > >>> Remove unnecessary encapsulating function > >>> > >>> hw/kvmclock.c | 24 ++++++++++++++++++++++++ > >>> 1 files changed, 24 insertions(+), 0 deletions(-) > >>> > >>> diff --git a/hw/kvmclock.c b/hw/kvmclock.c > >>> index 5388bc4..756839f 100644 > >>> --- a/hw/kvmclock.c > >>> +++ b/hw/kvmclock.c > >>> @@ -16,6 +16,7 @@ > >>> #include "sysbus.h" > >>> #include "kvm.h" > >>> #include "kvmclock.h" > >>> +#include "cpu-all.h" > >>> =20 > >>> #include > >>> #include > >>> @@ -69,11 +70,34 @@ static void kvmclock_vm_state_change(void *opaque= , int running, > >>> } > >>> } > >>> =20 > >>> +static void kvmclock_vm_state_change_vcpu(void *opaque, int running, > >>> + RunState state) > >>> +{ > >>> + int ret; > >>> + CPUState *penv =3D first_cpu; > >>> + > >>> + if (running) { > >>> + while (penv) { > >> > >> or: for (cpu =3D first_cpu; cpu !=3D NULL; cpu =3D cpu->next_cpu) { > >> > >=20 > > Functionally equivalent and I see both in the code, is there a standard? >=20 > Not really. I once tried to introduce an iterator macro, but it was > refused. The above is just more compact. >=20 > But this is only a minor nit. >=20 Fair enough, since there will be a V4 I will switch to the for loop. > >=20 > >>> + ret =3D kvm_vcpu_ioctl(penv, KVM_GUEST_PAUSED, 0); > >>> + if (ret) { > >>> + if (ret !=3D ENOSYS) { > >>> + fprintf(stderr, > >>> + "kvmclock_vm_state_change_vcpu: %s\n", > >>> + strerror(-ret)); > >>> + } > >>> + return; > >>> + } > >>> + penv =3D (CPUState *)penv->next_cpu; > >> > >> Unneeded cast. > >> > >=20 > > Also following an example seen elsewhere. >=20 > Generally, we try to avoid those pointless casts. >=20 Will remove for V4. > >=20 > >>> + } > >>> + } > >>> +} > >>> + > >> > >> Again: please use checkpatch.pl. > >> > >=20 > > Sorry, tough to get used to hitting space bar that many times... > >=20 > >>> static int kvmclock_init(SysBusDevice *dev) > >>> { > >>> KVMClockState *s =3D FROM_SYSBUS(KVMClockState, dev); > >>> =20 > >>> qemu_add_vm_change_state_handler(kvmclock_vm_state_change, s); > >>> + qemu_add_vm_change_state_handler(kvmclock_vm_state_change_vcpu, = NULL); > >>> return 0; > >>> } > >>> =20 > >> > >> Why not extend the existing handler? > >=20 > > Because the new handler doesn't touch the KVMClockState object. If thi= s is > > preferred, I have no objection. >=20 > The separate registration looks strange to me. And the fact that you > don't need to object doesn't justify a callback of its own. >=20 I think you misunderstood me, I meant I have no object to doign it your way= if you have a strong opinion (as it seems you do). > >=20 > >> > >> I still wonder if the IOCTL interface is actually kvmclock specific. B= ut > >> Marcello asked for this, and we could still change it when some arch > >> comes around that provides it independent of kvmclock. > >=20 > > The flag itself is stored in the pvclock_vcpu_time_info structure, and = anything > > else that touches that structure uses ioctls. >=20 > That's the host-guest interface. But I'm talking about the kvm-qemu > interface here which has no relation to how the "was paused" information > is transferred to the guest. >=20 > Jan >=20 --2oS5YaxWCcQjTEyO Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) iQIcBAEBAgAGBQJO3MAOAAoJEKhG9nGc1bpJDnYP/0Y9rH5BEghTI0K1lS4lH51V cAnDfTPKK4Ccqsu4bGyliKD2DUZEuP7+hxDFHXJrsC2Gpvnqeid5dkNYfSYnKdfC Xz8rBjpBijl7k2QCAwCgu71skC03281m2B0TAy8r5PGPIMhzXSQerUOX78G2ea5e zk28rOrfgvwWjyLm73uvkNNbFGCviFiWDpALA+lEkABF/DP/mW5/b/sSR1o8tPr0 Mo8zFAJ6BAw0kaX0fOugs/2ndWF3V2Id+gWlXabe/ya9OAiUcMmaHBe1nz4+CAKj TZOfV4L2kryKWfxgdrkkl8bzKQoA2EtP4Q1oLn0e2Zdw2PPx/oiIQLoFIX2TaCSc /oCBzCna6RxfCoxUlsi48KCl22S+cl8m/DBRdixwN8OfVgfRqlsroSq0Zex6g25B 6ezuMFQTx/LfXbV2Fq2vRq2UVAaFZej2sVtBgRX87D5g78b1YUZUsbbhh2/8YhD5 lzgnBY2h3aAEETZExq8Vz3rABL+0FUFe/0gKkUfUX4PJnJMloug5i1o28J8vvD+S V5Us3v88PT4rdzpxTnjl7RaNL3Vnx6CtPw01bsDLe9o5vjbliZIRpJYcQCS5QjRa XPGnURc009Hcjsujj0O61FWDPC3hT2ELMxooG5as5Y6hIDzxI7kWSrqYMretxQa+ /5M6u2e9f7c0nf6MxIqR =6Dav -----END PGP SIGNATURE----- --2oS5YaxWCcQjTEyO--