| Message ID | 1322602574-27072-1-git-send-email-emunson@mgebm.net |
|---|---|
| State | New |
| Headers | show |
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); > } > } >
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
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
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 >
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
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
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
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
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).
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.
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
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); } }
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(-)