From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:37652) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RWo2Y-0005Sm-51 for qemu-devel@nongnu.org; Sat, 03 Dec 2011 06:45:59 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RWo2W-0007Vv-E7 for qemu-devel@nongnu.org; Sat, 03 Dec 2011 06:45:58 -0500 Received: from fmmailgate02.web.de ([217.72.192.227]:35656) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RWo2W-0007Vn-25 for qemu-devel@nongnu.org; Sat, 03 Dec 2011 06:45:56 -0500 Received: from moweb001.kundenserver.de (moweb001.kundenserver.de [172.19.20.114]) by fmmailgate02.web.de (Postfix) with ESMTP id CEB781BB1CA0C for ; Sat, 3 Dec 2011 12:45:54 +0100 (CET) Message-ID: <4EDA0BEF.7010204@web.de> Date: Sat, 03 Dec 2011 12:45:51 +0100 From: Jan Kiszka MIME-Version: 1.0 References: <1322853560-24152-1-git-send-email-emunson@mgebm.net> <4ED9331B.9060708@web.de> <20111202212726.GA5662@mgebm.net> <4ED9E6B0.2000903@web.de> <20111203111935.GA25573@amt.cnet> <4EDA0731.80302@web.de> <20111203114241.GA25871@amt.cnet> In-Reply-To: <20111203114241.GA25871@amt.cnet> Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="------------enigC5E51E688D2DE7D2126C5BD2" Subject: Re: [Qemu-devel] [PATCH V3] Guest stop notification List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Marcelo Tosatti Cc: "@amt.cnet@amt.cnet, Eric B Munson , ryanh@linux.vnet.ibm.com , aliguori@us.ibm.com , kvm@vger.kernel.org , qemu-devel@nongnu.org" , Avi Kivity This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enigC5E51E688D2DE7D2126C5BD2 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable On 2011-12-03 12:42, Marcelo Tosatti wrote: > On Sat, Dec 03, 2011 at 12:25:37PM +0100, Jan Kiszka wrote: >> On 2011-12-03 12:19, Marcelo Tosatti wrote: >>> On Sat, Dec 03, 2011 at 10:06:56AM +0100, Jan Kiszka wrote: >>>> On 2011-12-02 22:27, Eric B Munson wrote: >>>>> On Fri, 02 Dec 2011, Jan Kiszka wrote: >>>>> >>>>>> On 2011-12-02 20:19, Eric B Munson wrote: >>>>>>> Often when a guest is stopped from the qemu console, it will repo= rt spurious >>>>>>> soft lockup warnings on resume. There are kernel patches being d= iscussed that >>>>>>> will give the host the ability to tell the guest that it is being= stopped 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 i= t 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 *op= aque, int running, >>>>>>> } >>>>>>> } >>>>>>> =20 >>>>>>> +static void kvmclock_vm_state_change_vcpu(void *opaque, int runn= ing, >>>>>>> + 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) = { >>>>>> >>>>> >>>>> Functionally equivalent and I see both in the code, is there a stan= dard? >>>> >>>> Not really. I once tried to introduce an iterator macro, but it was >>>> refused. The above is just more compact. >>>> >>>> But this is only a minor nit. >>>> >>>>> >>>>>>> + 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. >>>>>> >>>>> >>>>> Also following an example seen elsewhere. >>>> >>>> Generally, we try to avoid those pointless casts. >>>> >>>>> >>>>>>> + } >>>>>>> + } >>>>>>> +} >>>>>>> + >>>>>> >>>>>> Again: please use checkpatch.pl. >>>>>> >>>>> >>>>> Sorry, tough to get used to hitting space bar that many times... >>>>> >>>>>>> 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_vc= pu, NULL); >>>>>>> return 0; >>>>>>> } >>>>>>> =20 >>>>>> >>>>>> Why not extend the existing handler? >>>>> >>>>> Because the new handler doesn't touch the KVMClockState object. If= this is >>>>> preferred, I have no objection. >>>> >>>> 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. >>>> >>>>> >>>>>> >>>>>> I still wonder if the IOCTL interface is actually kvmclock specifi= c. But >>>>>> Marcello asked for this, and we could still change it when some ar= ch >>>>>> comes around that provides it independent of kvmclock. >>>>> >>>>> The flag itself is stored in the pvclock_vcpu_time_info structure, = and anything >>>>> else that touches that structure uses ioctls. >>>> >>>> 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" informa= tion >>>> is transferred to the guest. >>>> >>>> Jan >>> >>> It is one simple, rarely used command. I don't see why another interf= ace >>> such as kvm_run would be beneficial for this case. >>> >> >> I was referring to the relation between the IOCTL and kvmclock, but >> IOCTL vs. kvm_run. >> >> Jan >=20 > Ah, OK. Yes, we better characterize it as KVMCLOCK specific (a generic > "guest is paused" command is not the scope of this patch). >=20 > So appending KVMCLOCK_ to the ioctl definitions would make that more > explicit. IMHO, that would move things in the wrong direction. The IOCTL in itself has _nothing_ to do with kvmclock. It's just that its x86 backend is implemented on top of that infrastructure. For me the IOCTL is pretty generic, can be backed by kvmclock, but need not be on all future archs. Jan --------------enigC5E51E688D2DE7D2126C5BD2 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.16 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iEYEARECAAYFAk7aC+8ACgkQitSsb3rl5xTlpwCfV/VpSmIl9q2OA3obJMqE2UgA bmcAn29xEycG144q4dv6sN6PHSLKAqEU =6Us2 -----END PGP SIGNATURE----- --------------enigC5E51E688D2DE7D2126C5BD2--