From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:53551) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RWnzk-0004Iu-5A for qemu-devel@nongnu.org; Sat, 03 Dec 2011 06:43:05 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RWnzi-0006vG-Nq for qemu-devel@nongnu.org; Sat, 03 Dec 2011 06:43:04 -0500 Received: from mx1.redhat.com ([209.132.183.28]:27112) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RWnzi-0006vA-FR for qemu-devel@nongnu.org; Sat, 03 Dec 2011 06:43:02 -0500 Date: Sat, 3 Dec 2011 09:42:41 -0200 From: Marcelo Tosatti Message-ID: <20111203114241.GA25871@amt.cnet> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4EDA0731.80302@web.de> Subject: Re: [Qemu-devel] [PATCH V3] Guest stop notification List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jan Kiszka , "@amt.cnet"@amt.cnet Cc: "ryanh@linux.vnet.ibm.com" , "aliguori@us.ibm.com" , "kvm@vger.kernel.org" , "qemu-devel@nongnu.org" , Avi Kivity , Eric B Munson 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 report spurious > >>>>> soft lockup warnings on resume. There are kernel patches being discussed 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 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" > >>>>> > >>>>> #include > >>>>> #include > >>>>> @@ -69,11 +70,34 @@ static void kvmclock_vm_state_change(void *opaque, int running, > >>>>> } > >>>>> } > >>>>> > >>>>> +static void kvmclock_vm_state_change_vcpu(void *opaque, int running, > >>>>> + RunState state) > >>>>> +{ > >>>>> + int ret; > >>>>> + CPUState *penv = first_cpu; > >>>>> + > >>>>> + if (running) { > >>>>> + while (penv) { > >>>> > >>>> or: for (cpu = first_cpu; cpu != NULL; cpu = cpu->next_cpu) { > >>>> > >>> > >>> Functionally equivalent and I see both in the code, is there a standard? > >> > >> 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 = kvm_vcpu_ioctl(penv, KVM_GUEST_PAUSED, 0); > >>>>> + if (ret) { > >>>>> + if (ret != ENOSYS) { > >>>>> + fprintf(stderr, > >>>>> + "kvmclock_vm_state_change_vcpu: %s\n", > >>>>> + strerror(-ret)); > >>>>> + } > >>>>> + return; > >>>>> + } > >>>>> + penv = (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 = FROM_SYSBUS(KVMClockState, dev); > >>>>> > >>>>> 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; > >>>>> } > >>>>> > >>>> > >>>> 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 specific. But > >>>> Marcello asked for this, and we could still change it when some arch > >>>> 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" information > >> is transferred to the guest. > >> > >> Jan > > > > It is one simple, rarely used command. I don't see why another interface > > 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 Ah, OK. Yes, we better characterize it as KVMCLOCK specific (a generic "guest is paused" command is not the scope of this patch). So appending KVMCLOCK_ to the ioctl definitions would make that more explicit.