Re: [PATCH V8 1/1] Guest stop notification

[Raghavendra K T <raghavendra.kt@linux.vnet.ibm.com>]

On 04/06/2012 02:29 PM, Andreas Färber wrote:
> Am 06.04.2012 09:21, schrieb Raghavendra K T:
>> From: Eric B Munson<emunson@mgebm.net>
>>
>> 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.  This patch uses the qemu
>> Notifier system to tell the guest it is about to be stopped.
>>
>> Signed-off-by: Eric B Munson<emunson@mgebm.net>
>> Signed-off-by: Raghavendra K T<raghavendra.kt@linux.vnet.ibm.com>
>>
>> Cc: Eric B Munson<emunson@mgebm.net>
>> Cc: Avi Kivity<avi@redhat.com>
>> Cc: Marcelo Tosatti<mtosatti@redhat.com>
>> Cc: Anthony Liguori<aliguori@us.ibm.com>
>> Cc: Jan Kiszka<jan.kiszka@siemens.com>
>> Cc: "Andreas FÀrber"<afaerber@suse.de>
>> ---
>> Changes from V7:
>>   capabilty changed to KVM_CAP_KVMCLOCK_CTRL
>>   KVM_GUEST_PAUSED is pervcpu again
>>   CPUState renamed to CPUArchState
>
> Thanks, change looks right to me.
>
> Long-term I should probably consider supplying some cpu_foreach() macro
> to iterate over them, but that would still need manual declaration of a
> properly typed variable for the CPUArchState ->  CPUState switch.
>
>>   KVMCLOCK_GUEST_PAUSED changed to  KVM_KVMCLOCK_CTRL
>>
>> Changes from V6:
>>   Remove unnecessary include
>>
>> Changes from V5:
>>   KVM_GUEST_PAUSED is now a per vm ioctl instead of per vcpu
>>
>> Changes from V4:
>>   Test if the guest paused capability is available before use
>>
>> Changes from V3:
>>   Collapse new state change notification function into existsing function.
>>   Correct whitespace issues
>>   Change ioctl name to KVMCLOCK_GUEST_PAUSED
>>   Use for loop to iterate vpcu's
>>
>> 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
>> ---
>>
>> diff --git a/hw/kvm/clock.c b/hw/kvm/clock.c
>> index 446bd62..c8a34a5 100644
>> --- a/hw/kvm/clock.c
>> +++ b/hw/kvm/clock.c
>> @@ -64,10 +64,28 @@ static int kvmclock_post_load(void *opaque, int version_id)
>>   static void kvmclock_vm_state_change(void *opaque, int running,
>>                                        RunState state)
>>   {
>> +    int ret;
>
> Minor nitpick: We usually assign opaque values first thing in the
> function, so maybe order ret last if you resend?
>
>>       KVMClockState *s = opaque;
>> +    CPUArchState *penv = first_cpu;
>> +    int cap_clock_ctrl = kvm_check_extension(kvm_state, KVM_CAP_KVMCLOCK_CTRL);
>>
>>       if (running) {
>>           s->clock_valid = false;
>> +
>> +        if (!cap_clock_ctrl) {
>> +            return;
>> +        }
>> +        for (penv = first_cpu; penv != NULL; penv = penv->next_cpu) {
>> +            ret = kvm_vcpu_ioctl(penv, KVM_KVMCLOCK_CTRL, 0);
>> +            if (ret) {
>> +                if (ret != -EINVAL) {
>> +                    fprintf(stderr,
>> +                            "kvmclock_vm_state_change: %s\n",
>> +                            strerror(-ret));
>
> I always recommend to use __func__. Otherwise looks okay to me.
>
> Andreas
>
>> +                }
>> +                return;
>> +            }
>> +        }
>>       }
>>   }
>>
>
Thanks for Review. Sending with comments incorporated.
---
diff --git a/hw/kvm/clock.c b/hw/kvm/clock.c
index 446bd62..a6aa6e4 100644
--- a/hw/kvm/clock.c
+++ b/hw/kvm/clock.c
@@ -65,9 +65,27 @@ static void kvmclock_vm_state_change(void *opaque, 
int running,
                                       RunState state)
  {
      KVMClockState *s = opaque;
+    CPUArchState *penv = first_cpu;
+    int cap_clock_ctrl = kvm_check_extension(kvm_state, 
KVM_CAP_KVMCLOCK_CTRL);
+    int ret;

      if (running) {
          s->clock_valid = false;
+
+        if (!cap_clock_ctrl) {
+            return;
+        }
+        for (penv = first_cpu; penv != NULL; penv = penv->next_cpu) {
+            ret = kvm_vcpu_ioctl(penv, KVM_KVMCLOCK_CTRL, 0);
+            if (ret) {
+                if (ret != -EINVAL) {
+                    fprintf(stderr,
+                            " %s: %s\n", __FUNCTION__,
+                            strerror(-ret));
+                }
+                return;
+            }
+        }
      }
  }