Re: [Xen-devel] [PATCH] x86/hvm/hpet: avoid 'small' time diff test on resume

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Paul Durrant [mailto:paul.durrant@xxxxxxxxxx]
> Sent: 29 May 2019 14:10
> To: xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Paul Durrant <Paul.Durrant@xxxxxxxxxx>; Jan Beulich <jbeulich@xxxxxxxx>; 
> Andrew Cooper
> <Andrew.Cooper3@xxxxxxxxxx>; Wei Liu <wl@xxxxxxx>; Roger Pau Monne 
> <roger.pau@xxxxxxxxxx>
> Subject: [PATCH] x86/hvm/hpet: avoid 'small' time diff test on resume
> 
> It appears that even 64-bit versions of Windows 10, when not using syth-
> etic timers, will use 32-bit HPET non-periodic timers. There is a test
> in hpet_set_timer(), specific to 32-bit timers, that tries to disambiguate
> between a comparator value that is in the past and one that is sufficiently
> far in the future that it wraps. This is done by assuming that the delta
> between the main counter and comparator will be 'small' [1], if the

Sorry, forgot the ref. I'll send v2.

  Paul

> comparator value is in the past. Unfortunately, more often than not, this
> is not the case if the timer is being re-started after a migrate and so
> the timer is set to fire far in the future (in excess of a minute in
> several observed cases) rather then set to fire immediately. This has a
> rather odd symptom where the guest console is alive enough to be able to
> deal with mouse pointer re-rendering, but any keyboard activity or mouse
> clicks yield no response.
> 
> This patch simply adds a boolean argument to hpet_set_timer() so that the
> 'small' time test is omitted when the function is called to restart timers
> on resume, and thus any negative delta causes a timer to fire immediately.
> 
> Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> ---
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Cc: Wei Liu <wl@xxxxxxx>
> Cc: "Roger Pau Monné" <roger.pau@xxxxxxxxxx>
> 
> I notice that we seemingly don't handle main counter wrap in the HPET code.
> The spec. says that timers should fire at the point the counter wraps at the
> timer's width. I think the need for the 'small' time test would go away if
> this was implemented, but that's for another day.
> ---
>  xen/arch/x86/hvm/hpet.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/hpet.c b/xen/arch/x86/hvm/hpet.c
> index a916758106..49257986b5 100644
> --- a/xen/arch/x86/hvm/hpet.c
> +++ b/xen/arch/x86/hvm/hpet.c
> @@ -233,7 +233,7 @@ static void hpet_timer_fired(struct vcpu *v, void *data)
>  #define  HPET_TINY_TIME_SPAN  ((h->stime_freq >> 10) / STIME_PER_HPET_TICK)
> 
>  static void hpet_set_timer(HPETState *h, unsigned int tn,
> -                           uint64_t guest_time)
> +                           uint64_t guest_time, bool resume)
>  {
>      uint64_t tn_cmp, cur_tick, diff;
>      unsigned int irq;
> @@ -273,10 +273,13 @@ static void hpet_set_timer(HPETState *h, unsigned int 
> tn,
>       * Detect time values set in the past. This is hard to do for 32-bit
>       * comparators as the timer does not have to be set that far in the 
> future
>       * for the counter difference to wrap a 32-bit signed integer. We fudge
> -     * by looking for a 'small' time value in the past.
> +     * by looking for a 'small' time value in the past. However, if we
> +     * are resuming from suspend, treat any wrap as past since the value
> +     * is unlikely to be 'small'.
>       */
>      if ( (int64_t)diff < 0 )
> -        diff = (timer_is_32bit(h, tn) && (-diff > HPET_TINY_TIME_SPAN))
> +        diff = (timer_is_32bit(h, tn) && (-diff > HPET_TINY_TIME_SPAN) &&
> +                !resume)
>              ? (uint32_t)diff : 0;
> 
>      destroy_periodic_time(&h->pt[tn]);
> @@ -547,7 +550,7 @@ static int hpet_write(
>      {
>          i = find_first_set_bit(start_timers);
>          __clear_bit(i, &start_timers);
> -        hpet_set_timer(h, i, guest_time);
> +        hpet_set_timer(h, i, guest_time, false);
>      }
> 
>  #undef set_stop_timer
> @@ -692,7 +695,7 @@ static int hpet_load(struct domain *d, 
> hvm_domain_context_t *h)
>      if ( hpet_enabled(hp) )
>          for ( i = 0; i < HPET_TIMER_NUM; i++ )
>              if ( timer_enabled(hp, i) )
> -                hpet_set_timer(hp, i, guest_time);
> +                hpet_set_timer(hp, i, guest_time, true);
> 
>      write_unlock(&hp->lock);
> 
> --
> 2.20.1.2.gb21ebb671

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel