From mboxrd@z Thu Jan 1 00:00:00 1970 From: Raghavendra K T Subject: Re: [PATCH V8 1/1] Guest stop notification Date: Fri, 06 Apr 2012 15:19:19 +0530 Message-ID: <4F7EBC1F.5030300@linux.vnet.ibm.com> References: <20120406072057.28181.31911.sendpatchset@codeblue> <4F7EB056.4080104@suse.de> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-15; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: Anthony Liguori , Jan Kiszka , Qemu-devel , Srivatsa Vaddagiri , KVM , Marcelo Tosatti , Avi Kivity , "Michael J. Wolf" , Eric B Munson To: =?ISO-8859-15?Q?Andreas_F=E4rber?= Return-path: Received: from e28smtp02.in.ibm.com ([122.248.162.2]:51194 "EHLO e28smtp02.in.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754914Ab2DFJu2 (ORCPT ); Fri, 6 Apr 2012 05:50:28 -0400 Received: from /spool/local by e28smtp02.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 6 Apr 2012 15:20:25 +0530 Received: from d28av04.in.ibm.com (d28av04.in.ibm.com [9.184.220.66]) by d28relay01.in.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q369oBB14415694 for ; Fri, 6 Apr 2012 15:20:11 +0530 Received: from d28av04.in.ibm.com (loopback [127.0.0.1]) by d28av04.in.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q36FJwFk010708 for ; Sat, 7 Apr 2012 01:19:59 +1000 In-Reply-To: <4F7EB056.4080104@suse.de> Sender: kvm-owner@vger.kernel.org List-ID: On 04/06/2012 02:29 PM, Andreas F=E4rber wrote: > Am 06.04.2012 09:21, schrieb Raghavendra K T: >> From: Eric B Munson >> >> Often when a guest is stopped from the qemu console, it will report = spurious >> soft lockup warnings on resume. There are kernel patches being disc= ussed that >> will give the host the ability to tell the guest that it is being st= opped and >> should ignore the soft lockup warning that generates. This patch us= es the qemu >> Notifier system to tell the guest it is about to be stopped. >> >> Signed-off-by: Eric B Munson >> Signed-off-by: Raghavendra K T >> >> Cc: Eric B Munson >> Cc: Avi Kivity >> Cc: Marcelo Tosatti >> Cc: Anthony Liguori >> Cc: Jan Kiszka >> Cc: "Andreas F=C3=A4rber" >> --- >> Changes from V7: >> capabilty changed to KVM_CAP_KVMCLOCK_CTRL >> KVM_GUEST_PAUSED is pervcpu again >> CPUState renamed to CPUArchState > > Thanks, change looks right to me. > > Long-term I should probably consider supplying some cpu_foreach() mac= ro > to iterate over them, but that would still need manual declaration of= a > properly typed variable for the CPUArchState -> CPUState switch. > >> KVMCLOCK_GUEST_PAUSED changed to KVM_KVMCLOCK_CTRL >> >> Changes from V6: >> Remove unnecessary include >> >> Changes from V5: >> KVM_GUEST_PAUSED is now a per vm ioctl instead of per vcpu >> >> Changes from V4: >> Test if the guest paused capability is available before use >> >> Changes from V3: >> Collapse new state change notification function into existsing fun= ction. >> Correct whitespace issues >> Change ioctl name to KVMCLOCK_GUEST_PAUSED >> Use for loop to iterate vpcu's >> >> 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 >> --- >> >> diff --git a/hw/kvm/clock.c b/hw/kvm/clock.c >> index 446bd62..c8a34a5 100644 >> --- a/hw/kvm/clock.c >> +++ b/hw/kvm/clock.c >> @@ -64,10 +64,28 @@ static int kvmclock_post_load(void *opaque, int = version_id) >> static void kvmclock_vm_state_change(void *opaque, int running, >> RunState state) >> { >> + int ret; > > Minor nitpick: We usually assign opaque values first thing in the > function, so maybe order ret last if you resend? > >> KVMClockState *s =3D opaque; >> + CPUArchState *penv =3D first_cpu; >> + int cap_clock_ctrl =3D kvm_check_extension(kvm_state, KVM_CAP_K= VMCLOCK_CTRL); >> >> if (running) { >> s->clock_valid =3D false; >> + >> + if (!cap_clock_ctrl) { >> + return; >> + } >> + for (penv =3D first_cpu; penv !=3D NULL; penv =3D penv->nex= t_cpu) { >> + ret =3D kvm_vcpu_ioctl(penv, KVM_KVMCLOCK_CTRL, 0); >> + if (ret) { >> + if (ret !=3D -EINVAL) { >> + fprintf(stderr, >> + "kvmclock_vm_state_change: %s\n", >> + strerror(-ret)); > > I always recommend to use __func__. Otherwise looks okay to me. > > Andreas > >> + } >> + return; >> + } >> + } >> } >> } >> > Thanks for Review. Sending with comments incorporated. --- diff --git a/hw/kvm/clock.c b/hw/kvm/clock.c index 446bd62..a6aa6e4 100644 --- a/hw/kvm/clock.c +++ b/hw/kvm/clock.c @@ -65,9 +65,27 @@ static void kvmclock_vm_state_change(void *opaque,=20 int running, RunState state) { KVMClockState *s =3D opaque; + CPUArchState *penv =3D first_cpu; + int cap_clock_ctrl =3D kvm_check_extension(kvm_state,=20 KVM_CAP_KVMCLOCK_CTRL); + int ret; if (running) { s->clock_valid =3D false; + + if (!cap_clock_ctrl) { + return; + } + for (penv =3D first_cpu; penv !=3D NULL; penv =3D penv->next_c= pu) { + ret =3D kvm_vcpu_ioctl(penv, KVM_KVMCLOCK_CTRL, 0); + if (ret) { + if (ret !=3D -EINVAL) { + fprintf(stderr, + " %s: %s\n", __FUNCTION__, + strerror(-ret)); + } + return; + } + } } }