qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v1 7/7] kvm/x86: Hyper-V SynIC timers


From: Roman Kagan
Subject: Re: [Qemu-devel] [PATCH v1 7/7] kvm/x86: Hyper-V SynIC timers
Date: Fri, 27 Nov 2015 11:12:46 +0300
User-agent: Mutt/1.5.24 (2015-08-30)

On Wed, Nov 25, 2015 at 06:20:21PM +0300, Andrey Smetanin wrote:
> Per Hyper-V specification (and as required by Hyper-V-aware guests),
> SynIC provides 4 per-vCPU timers.  Each timer is programmed via a pair
> of MSRs, and signals expiration by delivering a special format message
> to the configured SynIC message slot and triggering the corresponding
> synthetic interrupt.
> 
> Note: as implemented by this patch, all periodic timers are "lazy"
> (i.e. if the vCPU wasn't scheduled for more than the timer period the
> timer events are lost), regardless of the corresponding configuration
> MSR.  If deemed necessary, the "catch up" mode (the timer period is
> shortened until the timer catches up) will be implemented later.
> 
> Signed-off-by: Andrey Smetanin <address@hidden>
> CC: Gleb Natapov <address@hidden>
> CC: Paolo Bonzini <address@hidden>
> CC: "K. Y. Srinivasan" <address@hidden>
> CC: Haiyang Zhang <address@hidden>
> CC: Vitaly Kuznetsov <address@hidden>
> CC: Roman Kagan <address@hidden>
> CC: Denis V. Lunev <address@hidden>
> CC: address@hidden
> ---
>  arch/x86/include/asm/kvm_host.h    |  13 ++
>  arch/x86/include/uapi/asm/hyperv.h |   6 +
>  arch/x86/kvm/hyperv.c              | 325 
> ++++++++++++++++++++++++++++++++++++-
>  arch/x86/kvm/hyperv.h              |  24 +++
>  arch/x86/kvm/x86.c                 |   9 +
>  include/linux/kvm_host.h           |   1 +
>  6 files changed, 375 insertions(+), 3 deletions(-)

A couple of nitpicks:

> +static void stimer_restart(struct kvm_vcpu_hv_stimer *stimer)
> +{
> +     u64 time_now;
> +     ktime_t ktime_now;
> +     u64 n;
> +
> +     time_now = get_time_ref_counter(stimer_to_vcpu(stimer)->kvm);
> +     ktime_now = ktime_get();
> +
> +     /*
> +      * Calculate positive integer n for which condtion -
> +      * (stimer->exp_time + n * stimer->count) > time_now
> +      * is true. We will use (stimer->exp_time + n * stimer->count)
> +      * as new stimer->exp_time.
> +      */
> +
> +     n = div64_u64(time_now - stimer->exp_time, stimer->count) + 1;
> +     stimer->exp_time += n * stimer->count;

This is actually just a reminder calculation so I'd rather do it
directly with div64_u64_rem().

> +void kvm_hv_process_stimers(struct kvm_vcpu *vcpu)
> +{
> +     struct kvm_vcpu_hv *hv_vcpu = vcpu_to_hv_vcpu(vcpu);
> +     struct kvm_vcpu_hv_stimer *stimer;
> +     u64 time_now;
> +     int i;
> +
> +     for (i = 0; i < ARRAY_SIZE(hv_vcpu->stimer); i++)
> +             if (test_and_clear_bit(i, hv_vcpu->stimer_pending_bitmap)) {
> +                     stimer = &hv_vcpu->stimer[i];
> +                     stimer_stop(stimer);

I think there's no need in this explicit stop: I see no way to arrive
here with a running timer, and even if there were, it would be safe as
your timer callback only manipulates the bitmaps atomically.

Neither comment is critical so

Reviewed-by: Roman Kagan <address@hidden>

Roman.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]