| Message ID | 20190529130948.5314-1-paul.durrant@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/hvm/hpet: avoid 'small' time diff test on resume | expand |
> -----Original Message----- > From: Paul Durrant [mailto:paul.durrant@citrix.com] > Sent: 29 May 2019 14:10 > To: xen-devel@lists.xenproject.org > Cc: Paul Durrant <Paul.Durrant@citrix.com>; Jan Beulich <jbeulich@suse.com>; Andrew Cooper > <Andrew.Cooper3@citrix.com>; Wei Liu <wl@xen.org>; Roger Pau Monne <roger.pau@citrix.com> > 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@citrix.com> > --- > Cc: Jan Beulich <jbeulich@suse.com> > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > Cc: Wei Liu <wl@xen.org> > Cc: "Roger Pau Monné" <roger.pau@citrix.com> > > 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
>>> On 29.05.19 at 15:09, <paul.durrant@citrix.com> wrote: > 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. Oh, indeed. I wasn't even (actively) aware of this. (I haven't been able to spot a statement to this effect though for wrapping of a 64-bit timer, just 32-bit ones.) > @@ -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'. > */ "resuming" and "suspend" are at best ambiguous - to me the terms relate more to ACPI S-states than to migrate/save/restore. Could I get you to agree to using "restoring after migration" or some such? With this in mind - is a new bool parameter needed at all? Can't you instead key this off of vhpet_domain(h)->creation_finished? > 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) Logically I would see the new part of the condition go first, but that's really minor as all three checks are sufficiently cheap. Jan
> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: 29 May 2019 14:37 > To: Paul Durrant <Paul.Durrant@citrix.com> > Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Roger Pau Monne <roger.pau@citrix.com>; xen-devel <xen- > devel@lists.xenproject.org>; WeiLiu <wl@xen.org> > Subject: Re: [PATCH] x86/hvm/hpet: avoid 'small' time diff test on resume > > >>> On 29.05.19 at 15:09, <paul.durrant@citrix.com> wrote: > > 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. > > Oh, indeed. I wasn't even (actively) aware of this. (I haven't been able to > spot a statement to this effect though for wrapping of a 64-bit timer, just > 32-bit ones.) I could have sworn I read that for 64-bit too, but upon re-reading it does appear to only apply to 32-bit timers. > > > @@ -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'. > > */ > > "resuming" and "suspend" are at best ambiguous - to me the terms > relate more to ACPI S-states than to migrate/save/restore. Could > I get you to agree to using "restoring after migration" or some such? Sure, I agree suspend and resume are somewhat overloaded. > > With this in mind - is a new bool parameter needed at all? Can't you > instead key this off of vhpet_domain(h)->creation_finished? Oh, I'd not considered that... I'll give that a try. > > > 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) > > Logically I would see the new part of the condition go first, but that's > really minor as all three checks are sufficiently cheap. No problem. I'll re-arrange. Paul > > Jan >
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);
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 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@citrix.com> --- Cc: Jan Beulich <jbeulich@suse.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> Cc: Wei Liu <wl@xen.org> Cc: "Roger Pau Monné" <roger.pau@citrix.com> 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(-)