Re: [Qemu-devel] [PATCH V8 1/1] Guest stop notificationorry for rduplicate mail

[Andreas Färber]

Am 06.04.2012 15:01, schrieb Raghavendra K T:
> On 04/06/2012 03:19 PM, Raghavendra K T wrote:
>> 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<address@hidden>
>>>>
>>>> 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<address@hidden>
>>>> Signed-off-by: Raghavendra K T<address@hidden>
>>>>
>>>> Cc: Eric B Munson<address@hidden>
>>>> Cc: Avi Kivity<address@hidden>
>>>> Cc: Marcelo Tosatti<address@hidden>
>>>> Cc: Anthony Liguori<address@hidden>
>>>> Cc: Jan Kiszka<address@hidden>
>>>> Cc: "Andreas FÀrber"<address@hidden>
>>>> ---
>>>> 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.
> 
> I think I should have added Acked-by and resent full patch. So here is
> it. sorry for duplicate mail.

No, it was not intended as such since I can't ack the ioctl. Resends are
best done with git-send-email, i.e. a v9 with change log (whether as
reply or not, opinions are divided) to make sure the right version gets
applied in the end.

> ---
> From: Eric B Munson <address@hidden>
> 
> 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.
> 
> Acked-by: "Andreas Färber" <address@hidden>
> Signed-off-by: Eric B Munson <address@hidden>
> Signed-off-by: Raghavendra K T <address@hidden>

*-bys should be added in chronological order, i.e. at the bottom.

> 
> Cc: Eric B Munson <address@hidden>
> Cc: Avi Kivity <address@hidden>
> Cc: Marcelo Tosatti <address@hidden>
> Cc: Anthony Liguori <address@hidden>
> Cc: Jan Kiszka <address@hidden>
> Cc: "Andreas Färber" <address@hidden>
> ---
> Changes from V7:
>  capabilty changed to KVM_CAP_KVMCLOCK_CTRL
>  KVM_GUEST_PAUSED is pervcpu again
>  CPUState renamed to CPUArchState
>  KVMCLOCK_GUEST_PAUSED changed to  KVM_KVMCLOCK_CTRL
>  incorporated Andrea's comments (__FUNCTION__) etc
> 
> 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..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__,

Is the whitespace before %s intentional? Wasn't there in v8.

The GCC manual recommends __func__, like I suggested, saying it's C99.
http://gcc.gnu.org/onlinedocs/gcc-4.7.0/gcc/Function-Names.html#Function-Names
__FUNCTION__ usage is currently 432 vs. __func__ 579, so not wrong.

If you want to leave it that way you can add my

Reviewed-by: Andreas Färber <address@hidden>

Andreas

> +                            strerror(-ret));
> +                }
> +                return;
> +            }
> +        }
>      }
>  }
> 

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg