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

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

 



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



[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux