Re: [Qemu-devel] [PATCH v1 2/6] kvm/x86: Hyper-V unify stimer_start() and stimer_restart()

[Andrey Smetanin]

On 01/07/2016 07:32 PM, Paolo Bonzini wrote:
> On 23/12/2015 12:28, Andrey Smetanin wrote:
> > This will be used in future to start Hyper-V SynIC timer
> > in several places by one logic in one function.
> >
> > Signed-off-by: Andrey Smetanin <address@hidden>
> > Reviewed-by: Roman Kagan <address@hidden>
> > CC: Gleb Natapov <address@hidden>
> > CC: Paolo Bonzini <address@hidden>
> > CC: Roman Kagan <address@hidden>
> > CC: Denis V. Lunev <address@hidden>
> > CC: address@hidden
> > ---
> >   arch/x86/kvm/hyperv.c | 37 ++++++++++++++++---------------------
> >   1 file changed, 16 insertions(+), 21 deletions(-)
> >
> > diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> > index ec3a900..8623aa6 100644
> > --- a/arch/x86/kvm/hyperv.c
> > +++ b/arch/x86/kvm/hyperv.c
> > @@ -408,6 +408,7 @@ static void stimer_cleanup(struct kvm_vcpu_hv_stimer 
> > *stimer)
> >         clear_bit(stimer->index,
> >                   vcpu_to_hv_vcpu(vcpu)->stimer_pending_bitmap);
> >         stimer->msg_pending = false;
> > +       stimer->exp_time = 0;
> >   }
> >
> >   static enum hrtimer_restart stimer_timer_callback(struct hrtimer *timer)
> > @@ -420,24 +421,6 @@ static enum hrtimer_restart stimer_timer_callback(struct 
> > hrtimer *timer)
> >         return HRTIMER_NORESTART;
> >   }
> >
> > -static void stimer_restart(struct kvm_vcpu_hv_stimer *stimer)
> > -{
> > -       u64 time_now;
> > -       ktime_t ktime_now;
> > -       u64 remainder;
> > -
> > -       time_now = get_time_ref_counter(stimer_to_vcpu(stimer)->kvm);
> > -       ktime_now = ktime_get();
> > -
> > -       div64_u64_rem(time_now - stimer->exp_time, stimer->count, &remainder);
> > -       stimer->exp_time = time_now + (stimer->count - remainder);
> > -
> > -       hrtimer_start(&stimer->timer,
> > -                     ktime_add_ns(ktime_now,
> > -                                  100 * (stimer->exp_time - time_now)),
> > -                     HRTIMER_MODE_ABS);
> > -}
> > -
> >   static int stimer_start(struct kvm_vcpu_hv_stimer *stimer)
> >   {
> >         u64 time_now;
> > @@ -450,9 +433,21 @@ static int stimer_start(struct kvm_vcpu_hv_stimer *stimer)
> >                 if (stimer->count == 0)
> >                         return -EINVAL;
> >
> > -               stimer->exp_time = time_now + stimer->count;
> > +               if (stimer->exp_time) {
> > +                       if (time_now >= stimer->exp_time) {
>
> Just for my education, is it possible to have this function called with
> stimer->exp_time != 0 && time_now < stimer->exp_time?
I think it's possible, assume the following situation:
GUEST                           HOST
periodic timer setup & start    start timer
                                timer expiration
                                    send timer expiration message
guest is busy and               .....
do not processing timer         timer expiration
message for longer than timer     try to send timer expiration message
period                              since message slot is still occupied
.....                               by guest set slot->msg_pending = 1
guest wake and now
 processed timer message,
  since slot->msg_pending = 1
   do wrmsr(HV_X64_MSR_EOM)
                                handle HV_X64_MSR_EOM
                                  kvm_hv_notify_acked_sint()
                                   schedule KVM_REQ_HV_STIMER
                                    kvm_hv_process_stimers()
                                      exp_time != 0 && now < exp_time

                                ....
                                timer expiration
                        
In this case we just start hrtimer again with
the same(previous) target exp_time, so I do not see any side effects.
> Paolo
>
> > +                               u64 remainder;
> > +
> > +                               div64_u64_rem(time_now - stimer->exp_time,
> > +                                             stimer->count, &remainder);
> > +                               stimer->exp_time =
> > +                                       time_now + (stimer->count - remainder);
> > +                       }
> > +               } else
> > +                       stimer->exp_time = time_now + stimer->count;
> > +
> >                 hrtimer_start(&stimer->timer,
> > -                             ktime_add_ns(ktime_now, 100 * stimer->count),
> > +                             ktime_add_ns(ktime_now,
> > +                                          100 * (stimer->exp_time - time_now)),
> >                               HRTIMER_MODE_ABS);
> >                 return 0;
> >         }
> > @@ -580,7 +575,7 @@ static void stimer_expiration(struct kvm_vcpu_hv_stimer 
> > *stimer)
> >         if (!(stimer->config & HV_STIMER_PERIODIC))
> >                 stimer->config |= ~HV_STIMER_ENABLE;
> >         else
> > -               stimer_restart(stimer);
> > +               stimer_start(stimer);
> >   }
> >
> >   void kvm_hv_process_stimers(struct kvm_vcpu *vcpu)