[Qemu-devel] [PATCH] Guest stop notification

[Qemu-devel] [PATCH] 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: ryanh@linux.vnet.ibm.com
Cc: aliguori@us.ibm.com
Cc: mtosatti@redhat.com
Cc: avi@redhat.com
Cc: kvm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 target-i386/kvm.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 5bfc21f..defd364 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -336,12 +336,18 @@ static int kvm_inject_mce_oldstyle(CPUState *env)
     return 0;
 }
 
+static void kvm_put_guest_paused(CPUState *penv)
+{
+    kvm_vcpu_ioctl(penv, KVM_GUEST_PAUSED, 0);
+}
+
 static void cpu_update_state(void *opaque, int running, RunState state)
 {
     CPUState *env = opaque;
 
     if (running) {
         env->tsc_valid = false;
+	kvm_put_guest_paused(env);
     }
 }
 
-- 
1.7.5.4

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

[Anthony Liguori]

On 11/29/2011 03:36 PM, 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>

Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

Regards,

Anthony Liguori

> Cc: ryanh@linux.vnet.ibm.com
> Cc: aliguori@us.ibm.com
> Cc: mtosatti@redhat.com
> Cc: avi@redhat.com
> Cc: kvm@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
>   target-i386/kvm.c |    6 ++++++
>   1 files changed, 6 insertions(+), 0 deletions(-)
>
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index 5bfc21f..defd364 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -336,12 +336,18 @@ static int kvm_inject_mce_oldstyle(CPUState *env)
>       return 0;
>   }
>
> +static void kvm_put_guest_paused(CPUState *penv)
> +{
> +    kvm_vcpu_ioctl(penv, KVM_GUEST_PAUSED, 0);
> +}
> +
>   static void cpu_update_state(void *opaque, int running, RunState state)
>   {
>       CPUState *env = opaque;
>
>       if (running) {
>           env->tsc_valid = false;
> +	kvm_put_guest_paused(env);
>       }
>   }
>

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

[Jan Kiszka]

On 2011-11-29 22:36, 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: ryanh@linux.vnet.ibm.com
> Cc: aliguori@us.ibm.com
> Cc: mtosatti@redhat.com
> Cc: avi@redhat.com
> Cc: kvm@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  target-i386/kvm.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index 5bfc21f..defd364 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -336,12 +336,18 @@ static int kvm_inject_mce_oldstyle(CPUState *env)
>      return 0;
>  }
>  
> +static void kvm_put_guest_paused(CPUState *penv)
> +{
> +    kvm_vcpu_ioctl(penv, KVM_GUEST_PAUSED, 0);
> +}

I see no need in encapsulating this in a separate function.

> +
>  static void cpu_update_state(void *opaque, int running, RunState state)
>  {
>      CPUState *env = opaque;
>  
>      if (running) {
>          env->tsc_valid = false;
> +	kvm_put_guest_paused(env);

checkpatch.pl would have asked you to remove this tab.

More general:

Why is this x86-only? If the kernel interface is x86-only, what prevents
making it generic right from the beginning?

Why do we need a new IOCTL for this? Was there no space left in the
kvm_run structure e.g. to pass this flag down on next vcpu execution? No
big deal, just wondering.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

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

[Eric B Munson]

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

On Thu, 01 Dec 2011, Jan Kiszka wrote:

> On 2011-11-29 22:36, 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: ryanh@linux.vnet.ibm.com
> > Cc: aliguori@us.ibm.com
> > Cc: mtosatti@redhat.com
> > Cc: avi@redhat.com
> > Cc: kvm@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org
> > ---
> >  target-i386/kvm.c |    6 ++++++
> >  1 files changed, 6 insertions(+), 0 deletions(-)
> > 
> > diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> > index 5bfc21f..defd364 100644
> > --- a/target-i386/kvm.c
> > +++ b/target-i386/kvm.c
> > @@ -336,12 +336,18 @@ static int kvm_inject_mce_oldstyle(CPUState *env)
> >      return 0;
> >  }
> >  
> > +static void kvm_put_guest_paused(CPUState *penv)
> > +{
> > +    kvm_vcpu_ioctl(penv, KVM_GUEST_PAUSED, 0);
> > +}
> 
> I see no need in encapsulating this in a separate function.
> 

The encapsulated function was from a previous idea, I will remove it for V2.

> > +
> >  static void cpu_update_state(void *opaque, int running, RunState state)
> >  {
> >      CPUState *env = opaque;
> >  
> >      if (running) {
> >          env->tsc_valid = false;
> > +	kvm_put_guest_paused(env);
> 
> checkpatch.pl would have asked you to remove this tab.

Will change to spaces for V2.

> 
> More general:
> 
> Why is this x86-only? If the kernel interface is x86-only, what prevents
> making it generic right from the beginning?
> 
> Why do we need a new IOCTL for this? Was there no space left in the
> kvm_run structure e.g. to pass this flag down on next vcpu execution? No
> big deal, just wondering.

Thanks for your review/feedback.

When I started looking into this problem, the ioctl was the first suggestion I
got for how to communicate from qemu to guest kernel.  I don't see a technical
reason that this could not be added to the kvm_run structure in one of the
bytes currently used as padding.  I would prefer to keep the ioctl because I
have the corresponding kernel patches out to work with this, however, if there
is a strong preference for using kvm_run, I can rework both sets.

Eric


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

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

[Arend van Spriel]

On 12/01/2011 06:19 PM, Eric B Munson wrote:
> On Thu, 01 Dec 2011, Jan Kiszka wrote:
> 
>> On 2011-11-29 22:36, Eric B Munson wrote:
>>> +
>>>  static void cpu_update_state(void *opaque, int running, RunState state)
>>>  {
>>>      CPUState *env = opaque;
>>>  
>>>      if (running) {
>>>          env->tsc_valid = false;
>>> +	kvm_put_guest_paused(env);
>>
>> checkpatch.pl would have asked you to remove this tab.
> 
> Will change to spaces for V2.
> 

Huh. Remove what tab? Indent are always 8-space TAB according
CodingStyle doc:

                Chapter 1: Indentation

Tabs are 8 characters, and thus indentations are also 8 characters.
There are heretic movements that try to make indentations 4 (or even 2!)
characters deep, and that is akin to trying to define the value of PI to
be 3.


Gr. AvS

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

[Jan Kiszka]

On 2011-12-01 18:31, Arend van Spriel wrote:
> On 12/01/2011 06:19 PM, Eric B Munson wrote:
>> On Thu, 01 Dec 2011, Jan Kiszka wrote:
>>
>>> On 2011-11-29 22:36, Eric B Munson wrote:
>>>> +
>>>>  static void cpu_update_state(void *opaque, int running, RunState state)
>>>>  {
>>>>      CPUState *env = opaque;
>>>>  
>>>>      if (running) {
>>>>          env->tsc_valid = false;
>>>> +	kvm_put_guest_paused(env);
>>>
>>> checkpatch.pl would have asked you to remove this tab.
>>
>> Will change to spaces for V2.
>>
> 
> Huh. Remove what tab? Indent are always 8-space TAB according
> CodingStyle doc:
> 
>                 Chapter 1: Indentation
> 
> Tabs are 8 characters, and thus indentations are also 8 characters.
> There are heretic movements that try to make indentations 4 (or even 2!)
> characters deep, and that is akin to trying to define the value of PI to
> be 3.

Don't worry, this is not kernel code. :) QEMU uses a different coding style.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

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

[Jan Kiszka]

On 2011-12-01 18:19, Eric B Munson wrote:
> On Thu, 01 Dec 2011, Jan Kiszka wrote:
> 
>> On 2011-11-29 22:36, 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:
>>> ryanh@linux.vnet.ibm.com Cc: aliguori@us.ibm.com Cc:
>>> mtosatti@redhat.com Cc: avi@redhat.com Cc: kvm@vger.kernel.org 
>>> Cc: linux-kernel@vger.kernel.org --- target-i386/kvm.c |    6
>>> ++++++ 1 files changed, 6 insertions(+), 0 deletions(-)
>>> 
>>> diff --git a/target-i386/kvm.c b/target-i386/kvm.c index
>>> 5bfc21f..defd364 100644 --- a/target-i386/kvm.c +++
>>> b/target-i386/kvm.c @@ -336,12 +336,18 @@ static int
>>> kvm_inject_mce_oldstyle(CPUState *env) return 0; }
>>> 
>>> +static void kvm_put_guest_paused(CPUState *penv) +{ +
>>> kvm_vcpu_ioctl(penv, KVM_GUEST_PAUSED, 0); +}
>> 
>> I see no need in encapsulating this in a separate function.
>> 
> 
> The encapsulated function was from a previous idea, I will remove
> it for V2.
> 
>>> + static void cpu_update_state(void *opaque, int running,
>>> RunState state) { CPUState *env = opaque;
>>> 
>>> if (running) { env->tsc_valid = false; +
>>> kvm_put_guest_paused(env);
>> 
>> checkpatch.pl would have asked you to remove this tab.
> 
> Will change to spaces for V2.
> 
>> 
>> More general:
>> 
>> Why is this x86-only? If the kernel interface is x86-only, what
>> prevents making it generic right from the beginning?
>> 
>> Why do we need a new IOCTL for this? Was there no space left in
>> the kvm_run structure e.g. to pass this flag down on next vcpu
>> execution? No big deal, just wondering.
> 
> Thanks for your review/feedback.
> 
> When I started looking into this problem, the ioctl was the first
> suggestion I got for how to communicate from qemu to guest kernel.
> I don't see a technical reason that this could not be added to the
> kvm_run structure in one of the bytes currently used as padding.  I
> would prefer to keep the ioctl because I have the corresponding
> kernel patches out to work with this, however, if there is a strong
> preference for using kvm_run, I can rework both sets.

My feeling is that a run field would be more elegant, but I might be
wrong on this as well. In any case: You need KVM_CAP in your kernel
interface to announce the new feature and you have to sync in the new
kernel header into QEMU to make it build.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

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

[Marcelo Tosatti]

On Thu, Dec 01, 2011 at 12:19:38PM -0500, Eric B Munson wrote:
> On Thu, 01 Dec 2011, Jan Kiszka wrote:
> 
> > On 2011-11-29 22:36, 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: ryanh@linux.vnet.ibm.com
> > > Cc: aliguori@us.ibm.com
> > > Cc: mtosatti@redhat.com
> > > Cc: avi@redhat.com
> > > Cc: kvm@vger.kernel.org
> > > Cc: linux-kernel@vger.kernel.org
> > > ---
> > >  target-i386/kvm.c |    6 ++++++
> > >  1 files changed, 6 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> > > index 5bfc21f..defd364 100644
> > > --- a/target-i386/kvm.c
> > > +++ b/target-i386/kvm.c
> > > @@ -336,12 +336,18 @@ static int kvm_inject_mce_oldstyle(CPUState *env)
> > >      return 0;
> > >  }
> > >  
> > > +static void kvm_put_guest_paused(CPUState *penv)
> > > +{
> > > +    kvm_vcpu_ioctl(penv, KVM_GUEST_PAUSED, 0);
> > > +}
> > 
> > I see no need in encapsulating this in a separate function.
> > 
> 
> The encapsulated function was from a previous idea, I will remove it for V2.
> 
> > > +
> > >  static void cpu_update_state(void *opaque, int running, RunState state)
> > >  {
> > >      CPUState *env = opaque;
> > >  
> > >      if (running) {
> > >          env->tsc_valid = false;
> > > +	kvm_put_guest_paused(env);
> > 
> > checkpatch.pl would have asked you to remove this tab.
> 
> Will change to spaces for V2.
> 
> > 
> > More general:
> > 
> > Why is this x86-only? If the kernel interface is x86-only, what prevents
> > making it generic right from the beginning?
> > 
> > Why do we need a new IOCTL for this? Was there no space left in the
> > kvm_run structure e.g. to pass this flag down on next vcpu execution? No
> > big deal, just wondering.
> 
> Thanks for your review/feedback.
> 
> When I started looking into this problem, the ioctl was the first suggestion I
> got for how to communicate from qemu to guest kernel.  I don't see a technical
> reason that this could not be added to the kvm_run structure in one of the
> bytes currently used as padding.  I would prefer to keep the ioctl because I
> have the corresponding kernel patches out to work with this, however, if there
> is a strong preference for using kvm_run, I can rework both sets.
> 
> Eric

This functionality being on top of kvmclock, it is more natural for this
command to be an ioctl (in similarity with other kvmclock commands).

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

[Eric B Munson]

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

On Thu, 01 Dec 2011, Jan Kiszka wrote:

> On 2011-11-29 22:36, 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: ryanh@linux.vnet.ibm.com
> > Cc: aliguori@us.ibm.com
> > Cc: mtosatti@redhat.com
> > Cc: avi@redhat.com
> > Cc: kvm@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org
> > ---
> >  target-i386/kvm.c |    6 ++++++
> >  1 files changed, 6 insertions(+), 0 deletions(-)
> > 
> > diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> > index 5bfc21f..defd364 100644
> > --- a/target-i386/kvm.c
> > +++ b/target-i386/kvm.c
> > @@ -336,12 +336,18 @@ static int kvm_inject_mce_oldstyle(CPUState *env)
> >      return 0;
> >  }
> >  
> > +static void kvm_put_guest_paused(CPUState *penv)
> > +{
> > +    kvm_vcpu_ioctl(penv, KVM_GUEST_PAUSED, 0);
> > +}
> 
> I see no need in encapsulating this in a separate function.
> 
> > +
> >  static void cpu_update_state(void *opaque, int running, RunState state)
> >  {
> >      CPUState *env = opaque;
> >  
> >      if (running) {
> >          env->tsc_valid = false;
> > +	kvm_put_guest_paused(env);
> 
> checkpatch.pl would have asked you to remove this tab.
> 
> More general:
> 
> Why is this x86-only? If the kernel interface is x86-only, what prevents
> making it generic right from the beginning?

Sorry, missed this question on the first pass, this is x86 only because the
flag used lives in the pvclock structure.  AFAICT, there aren't any other
architectures out there that implement paravirtualized clocks yet.

> 
> Why do we need a new IOCTL for this? Was there no space left in the
> kvm_run structure e.g. to pass this flag down on next vcpu execution? No
> big deal, just wondering.
> 
> Jan
> 
> -- 
> Siemens AG, Corporate Technology, CT T DE IT 1
> Corporate Competence Center Embedded Linux
> 

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

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

[Jan Kiszka]

On 2011-12-01 18:22, Eric B Munson wrote:
> On Thu, 01 Dec 2011, Jan Kiszka wrote:
> 
>> On 2011-11-29 22:36, 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: ryanh@linux.vnet.ibm.com
>>> Cc: aliguori@us.ibm.com
>>> Cc: mtosatti@redhat.com
>>> Cc: avi@redhat.com
>>> Cc: kvm@vger.kernel.org
>>> Cc: linux-kernel@vger.kernel.org
>>> ---
>>>  target-i386/kvm.c |    6 ++++++
>>>  1 files changed, 6 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
>>> index 5bfc21f..defd364 100644
>>> --- a/target-i386/kvm.c
>>> +++ b/target-i386/kvm.c
>>> @@ -336,12 +336,18 @@ static int kvm_inject_mce_oldstyle(CPUState *env)
>>>      return 0;
>>>  }
>>>  
>>> +static void kvm_put_guest_paused(CPUState *penv)
>>> +{
>>> +    kvm_vcpu_ioctl(penv, KVM_GUEST_PAUSED, 0);
>>> +}
>>
>> I see no need in encapsulating this in a separate function.
>>
>>> +
>>>  static void cpu_update_state(void *opaque, int running, RunState state)
>>>  {
>>>      CPUState *env = opaque;
>>>  
>>>      if (running) {
>>>          env->tsc_valid = false;
>>> +	kvm_put_guest_paused(env);
>>
>> checkpatch.pl would have asked you to remove this tab.
>>
>> More general:
>>
>> Why is this x86-only? If the kernel interface is x86-only, what prevents
>> making it generic right from the beginning?
> 
> Sorry, missed this question on the first pass, this is x86 only because the
> flag used lives in the pvclock structure.  AFAICT, there aren't any other
> architectures out there that implement paravirtualized clocks yet.

That's an implementation "detail" of the kernel. The interface (IOCTL or
kvm_run field) is generic, no?

I would just fire this notification from generic code, evaluate the
error (that was lacking so far), and only report it if it's something
else than "not supported".

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

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

[Marcelo Tosatti]

On Thu, Dec 01, 2011 at 06:36:17PM +0100, Jan Kiszka wrote:
> On 2011-12-01 18:22, Eric B Munson wrote:
> > On Thu, 01 Dec 2011, Jan Kiszka wrote:
> > 
> >> On 2011-11-29 22:36, 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: ryanh@linux.vnet.ibm.com
> >>> Cc: aliguori@us.ibm.com
> >>> Cc: mtosatti@redhat.com
> >>> Cc: avi@redhat.com
> >>> Cc: kvm@vger.kernel.org
> >>> Cc: linux-kernel@vger.kernel.org
> >>> ---
> >>>  target-i386/kvm.c |    6 ++++++
> >>>  1 files changed, 6 insertions(+), 0 deletions(-)
> >>>
> >>> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> >>> index 5bfc21f..defd364 100644
> >>> --- a/target-i386/kvm.c
> >>> +++ b/target-i386/kvm.c
> >>> @@ -336,12 +336,18 @@ static int kvm_inject_mce_oldstyle(CPUState *env)
> >>>      return 0;
> >>>  }
> >>>  
> >>> +static void kvm_put_guest_paused(CPUState *penv)
> >>> +{
> >>> +    kvm_vcpu_ioctl(penv, KVM_GUEST_PAUSED, 0);
> >>> +}
> >>
> >> I see no need in encapsulating this in a separate function.
> >>
> >>> +
> >>>  static void cpu_update_state(void *opaque, int running, RunState state)
> >>>  {
> >>>      CPUState *env = opaque;
> >>>  
> >>>      if (running) {
> >>>          env->tsc_valid = false;
> >>> +	kvm_put_guest_paused(env);
> >>
> >> checkpatch.pl would have asked you to remove this tab.
> >>
> >> More general:
> >>
> >> Why is this x86-only? If the kernel interface is x86-only, what prevents
> >> making it generic right from the beginning?
> > 
> > Sorry, missed this question on the first pass, this is x86 only because the
> > flag used lives in the pvclock structure.  AFAICT, there aren't any other
> > architectures out there that implement paravirtualized clocks yet.
> 
> That's an implementation "detail" of the kernel. The interface (IOCTL or
> kvm_run field) is generic, no?
> 
> I would just fire this notification from generic code, evaluate the
> error (that was lacking so far), and only report it if it's something
> else than "not supported".

Yes, it should live in hw/kvmclock.c preferably.

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

[Eric B Munson]

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

On Thu, 01 Dec 2011, Marcelo Tosatti wrote:

> On Thu, Dec 01, 2011 at 06:36:17PM +0100, Jan Kiszka wrote:
> > On 2011-12-01 18:22, Eric B Munson wrote:
> > > On Thu, 01 Dec 2011, Jan Kiszka wrote:
> > > 
> > >> On 2011-11-29 22:36, 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: ryanh@linux.vnet.ibm.com
> > >>> Cc: aliguori@us.ibm.com
> > >>> Cc: mtosatti@redhat.com
> > >>> Cc: avi@redhat.com
> > >>> Cc: kvm@vger.kernel.org
> > >>> Cc: linux-kernel@vger.kernel.org
> > >>> ---
> > >>>  target-i386/kvm.c |    6 ++++++
> > >>>  1 files changed, 6 insertions(+), 0 deletions(-)
> > >>>
> > >>> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> > >>> index 5bfc21f..defd364 100644
> > >>> --- a/target-i386/kvm.c
> > >>> +++ b/target-i386/kvm.c
> > >>> @@ -336,12 +336,18 @@ static int kvm_inject_mce_oldstyle(CPUState *env)
> > >>>      return 0;
> > >>>  }
> > >>>  
> > >>> +static void kvm_put_guest_paused(CPUState *penv)
> > >>> +{
> > >>> +    kvm_vcpu_ioctl(penv, KVM_GUEST_PAUSED, 0);
> > >>> +}
> > >>
> > >> I see no need in encapsulating this in a separate function.
> > >>
> > >>> +
> > >>>  static void cpu_update_state(void *opaque, int running, RunState state)
> > >>>  {
> > >>>      CPUState *env = opaque;
> > >>>  
> > >>>      if (running) {
> > >>>          env->tsc_valid = false;
> > >>> +	kvm_put_guest_paused(env);
> > >>
> > >> checkpatch.pl would have asked you to remove this tab.
> > >>
> > >> More general:
> > >>
> > >> Why is this x86-only? If the kernel interface is x86-only, what prevents
> > >> making it generic right from the beginning?
> > > 
> > > Sorry, missed this question on the first pass, this is x86 only because the
> > > flag used lives in the pvclock structure.  AFAICT, there aren't any other
> > > architectures out there that implement paravirtualized clocks yet.
> > 
> > That's an implementation "detail" of the kernel. The interface (IOCTL or
> > kvm_run field) is generic, no?
> > 
> > I would just fire this notification from generic code, evaluate the
> > error (that was lacking so far), and only report it if it's something
> > else than "not supported".
> 
> Yes, it should live in hw/kvmclock.c preferably.
> 

Okay, I get a V3 with this moved around out tomorrow.

Thanks for the feedback,
Eric

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