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 <asmetanin@xxxxxxxxxxxxx>
> CC: Gleb Natapov <gleb@xxxxxxxxxx>
> CC: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> CC: "K. Y. Srinivasan" <kys@xxxxxxxxxxxxx>
> CC: Haiyang Zhang <haiyangz@xxxxxxxxxxxxx>
> CC: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx>
> CC: Roman Kagan <rkagan@xxxxxxxxxxxxx>
> CC: Denis V. Lunev <den@xxxxxxxxxx>
> CC: qemu-devel@xxxxxxxxxx
> ---
> 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 <rkagan@xxxxxxxxxxxxx>
Roman.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
