[Qemu-devel] [PATCH V3] Guest stop notification

[Qemu-devel] [PATCH V3] Guest stop notification

[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 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) {
+            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;
+        }
+    }
+}
+
 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;
 }
 
-- 
1.7.5.4

Re: [Qemu-devel] [PATCH V3] Guest stop notification

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2686 bytes --]

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) {

> +            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.

> +        }
> +    }
> +}
> +

Again: please use checkpatch.pl.

>  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?

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.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Qemu-devel] [PATCH V3] Guest stop notification

[Eric B Munson]

[-- Attachment #1: Type: text/plain, Size: 3353 bytes --]

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?

> > +            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.

> > +        }
> > +    }
> > +}
> > +
> 
> 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.

> 
> 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.

> 
> Jan
> 



[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH V3] Guest stop notification

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 3936 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Qemu-devel] [PATCH V3] Guest stop notification

[Marcelo Tosatti]

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.

Re: [Qemu-devel] [PATCH V3] Guest stop notification

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 4531 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Qemu-devel] [PATCH V3] Guest stop notification

[Marcelo Tosatti]

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.

Re: [Qemu-devel] [PATCH V3] Guest stop notification

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 5440 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Qemu-devel] [PATCH V3] Guest stop notification

[Marcelo Tosatti]

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.

Re: [Qemu-devel] [PATCH V3] Guest stop notification

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1249 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Qemu-devel] [PATCH V3] Guest stop notification

[Eric B Munson]

[-- Attachment #1: Type: text/plain, Size: 4466 bytes --]

On Sat, 03 Dec 2011, 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.
> 

Fair enough, since there will be a V4 I will switch to the for loop.

> > 
> >>> +            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.
> 

Will remove for V4.

> > 
> >>> +        }
> >>> +    }
> >>> +}
> >>> +
> >>
> >> 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 think you misunderstood me, I meant I have no object to doign it your way if
you have a strong opinion (as it seems you do).

> > 
> >>
> >> 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
> 



[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]