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

[Jan Kiszka <jan.kiszka@web.de>]

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