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 <emunson@xxxxxxxxx> > >>> Cc: Avi Kivity <avi@xxxxxxxxxx> > >>> Cc: Marcelo Tosatti <mtosatti@xxxxxxxxxx> > >>> Cc: Jan Kiszka <jan.kiszka@xxxxxxxxxxx> > >>> Cc: ryanh@xxxxxxxxxxxxxxxxxx > >>> Cc: aliguori@xxxxxxxxxx > >>> Cc: kvm@xxxxxxxxxxxxxxx > >>> > >>> --- > >>> 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 <linux/kvm.h> > >>> #include <linux/kvm_para.h> > >>> @@ -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. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html