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