Re: [PATCH] KVM: vmx: fix underflow in TSC deadline calculation

[yunhong jiang]

On Tue, 28 Jun 2016 13:56:38 -0400 (EDT)
Paolo Bonzini <pbonzini@xxxxxxxxxx> wrote:

> 
> 
> > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> > > index fdc05ae..b15e32a 100644
> > > --- a/arch/x86/kvm/lapic.c
> > > +++ b/arch/x86/kvm/lapic.c
> > > @@ -1454,11 +1454,18 @@ static void start_apic_timer(struct
> > > kvm_lapic *apic) /* lapic timer in tsc deadline mode */
> > >  		u64 tscdeadline = apic->lapic_timer.tscdeadline;
> > >  
> > > -		if (kvm_x86_ops->set_hv_timer &&
> > > -		    !kvm_x86_ops->set_hv_timer(apic->vcpu,
> > > tscdeadline)) {
> > > -			apic->lapic_timer.hv_timer_in_use = true;
> > > -
> > > trace_kvm_hv_timer_state(apic->vcpu->vcpu_id,
> > > +		if (kvm_x86_ops->set_hv_timer) {
> > > +			if (kvm_x86_ops->set_hv_timer(apic->vcpu,
> > 
> > Would it be better that if set_hv_timer fails, we clear the vmx
> > timer (i.e. the VMCS field) before return the failure? I'm not sure
> > if it make sense to clear the previous setup if a new setup fails,
> > although it seems OK for me, since we have to cancel the hv_timer
> > anyway.
> 
> Good question.  I think we should abstract a little the set_hv_timer
> and cancel_hv_timer calls, for example with a new function
> start_hv_tscdeadline. It's also simpler to avoid the race window by
> disabling interrupts around the calls to set_hv_timer and
> hrtimer_cancel.  I'll see what I can come up with.

Paolo, thanks for reply.

Which race window you are talking about? Is it the
kvm_lapic_switch_to_hv_timer()? If yes, hope we will not forgot it once the
lapic timer is not pinned anymore in future.

Thanks
--jyh

> 
> Paolo