From: Paul Durrant <Paul.Durrant@citrix.com> To: Paul Durrant <Paul.Durrant@citrix.com>, "xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>, Wei Liu <wl@xen.org>, Jan Beulich <jbeulich@suse.com>, Roger Pau Monne <roger.pau@citrix.com> Subject: Re: [PATCH] x86/hvm/hpet: avoid 'small' time diff test on resume Date: Wed, 29 May 2019 13:27:03 +0000 [thread overview] Message-ID: <4319e6a0879a4f6484c643fa8d8c68b1@AMSPEX02CL03.citrite.net> (raw) In-Reply-To: <20190529130948.5314-1-paul.durrant@citrix.com> > -----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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
WARNING: multiple messages have this Message-ID (diff)
From: Paul Durrant <Paul.Durrant@citrix.com> To: Paul Durrant <Paul.Durrant@citrix.com>, "xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>, Wei Liu <wl@xen.org>, Jan Beulich <jbeulich@suse.com>, Roger Pau Monne <roger.pau@citrix.com> Subject: Re: [Xen-devel] [PATCH] x86/hvm/hpet: avoid 'small' time diff test on resume Date: Wed, 29 May 2019 13:27:03 +0000 [thread overview] Message-ID: <4319e6a0879a4f6484c643fa8d8c68b1@AMSPEX02CL03.citrite.net> (raw) Message-ID: <20190529132703.hBZgi_Gru-Dul7WVbz8Pd8MjRUY9HxSaQ1wsl5r4mjk@z> (raw) In-Reply-To: <20190529130948.5314-1-paul.durrant@citrix.com> > -----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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
next prev parent reply other threads:[~2019-05-29 13:28 UTC|newest] Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-05-29 13:09 [PATCH] x86/hvm/hpet: avoid 'small' time diff test on resume Paul Durrant 2019-05-29 13:09 ` [Xen-devel] " Paul Durrant 2019-05-29 13:27 ` Paul Durrant [this message] 2019-05-29 13:27 ` Paul Durrant 2019-05-29 13:36 ` Jan Beulich 2019-05-29 13:36 ` [Xen-devel] " Jan Beulich 2019-05-29 13:43 ` Paul Durrant 2019-05-29 13:43 ` [Xen-devel] " Paul Durrant
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=4319e6a0879a4f6484c643fa8d8c68b1@AMSPEX02CL03.citrite.net \ --to=paul.durrant@citrix.com \ --cc=Andrew.Cooper3@citrix.com \ --cc=jbeulich@suse.com \ --cc=roger.pau@citrix.com \ --cc=wl@xen.org \ --cc=xen-devel@lists.xenproject.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).