Commit Message

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@mgebm.net>
Cc: Avi Kivity <avi@redhat.com>
Cc: Marcelo Tosatti <mtosatti@redhat.com>
Cc: Jan Kiszka <jan.kiszka@siemens.com>
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(-)

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 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@mgebm.net>>>>>>>> Cc: Avi Kivity <avi@redhat.com>>>>>>>> Cc: Marcelo Tosatti <mtosatti@redhat.com>>>>>>>> Cc: Jan Kiszka <jan.kiszka@siemens.com>>>>>>>> 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 <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.>>>>>>> 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.
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

On Sat, Dec 03, 2011 at 12:45:51PM +0100, Jan Kiszka wrote:
> >> 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.> > 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
I do not see the need to lift this infrastructure to arch independent
status at the moment, without clear semantics on that arch independent
level.
So I am fine with the current GUEST_PAUSED naming (which can later be
extended with GUEST_RESUMED etc, if necessary, for use by other archs
for example), and implementation in hw/kvmclock.c.

On 2011-12-05 14:35, Marcelo Tosatti wrote:
> On Sat, Dec 03, 2011 at 12:45:51PM +0100, Jan Kiszka wrote:>>>> 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.>>>> 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> > I do not see the need to lift this infrastructure to arch independent> status at the moment, without clear semantics on that arch independent> level.> > So I am fine with the current GUEST_PAUSED naming (which can later be> extended with GUEST_RESUMED etc, if necessary, for use by other archs> for example), and implementation in hw/kvmclock.c.>
Yes, let's keep it as suggested last (addition of kvmclock, unchanged
IOCTL interface).
Jan