Hi,
While I was investigating the following issue on Windows XP (both 32-bit and
64-bit):
* AMD NPT performance regression after c/s 24770:7f79475d3de7
Reference:http://marc.info/?l=xen-devel&m=135075376805215
On the latest source form xen-unstable, I ran into an issue where the timing on
the HVM guests skewing about 2x slower than the actual wall clock time. This
results in the system time slowing down. This is regardless of the cpufreq
governor scaling. (I tried with both ondemand and performance).
However, I don''t see the same behavior on the Win7 HVM guests. Is this
a known issue. I assume that XP and Win7 uses different mechanism for keeping
time (e.g. rdtsc vs. HPET)?
Thank you,
Suravee
On 03/12/2013 03:40 PM, Suravee Suthikulpanit wrote:> Hi, > > While I was investigating the following issue on Windows XP (both 32-bit > and 64-bit): > > * AMD NPT performance regression after c/s 24770:7f79475d3de7 > Reference:http://marc.info/?l=xen-devel&m=135075376805215 > > > On the latest source form xen-unstable, I ran into an issue where the > timing on the HVM guests skewing about 2x slower than the actual wall > clock time. This results in the system time slowing down. This is > regardless of the cpufreq governor scaling. (I tried with both ondemand > and performance). > > However, I don''t see the same behavior on the Win7 HVM guests. Is this a > known issue. I assume that XP and Win7 uses different mechanism for > keeping time (e.g. rdtsc vs. HPET)?I found this recently as well... I didn''t managed to come across a solution - however mine is on an Intel CPU. In the end, I just gave up and installed an NTP client on the WinXP systems. This updated the clock every hour so it doesn''t drift enough to worry about anymore. If I disable this, it drifts a few hours each day. -- Steven Haigh Email: netwiz@crc.id.au Web: https://www.crc.id.au Phone: (03) 9001 6090 - 0412 935 897 Fax: (03) 8338 0299
>>> On 12.03.13 at 05:40, Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> wrote: > While I was investigating the following issue on Windows XP (both 32-bit and > 64-bit): > > * AMD NPT performance regression after c/s 24770:7f79475d3de7 > Reference:http://marc.info/?l=xen-devel&m=135075376805215 > > On the latest source form xen-unstable, I ran into an issue where the timing > on the HVM guests skewing about 2x slower than the actual wall clock time. > This results in the system time slowing down. This is regardless of the > cpufreq governor scaling. (I tried with both ondemand and performance). > > However, I don''t see the same behavior on the Win7 HVM guests. Is this a > known issue.While I recall respective reports, the issue should not be present on current -unstable, so if you do have observations like this they will need investigation.> I assume that XP and Win7 uses different mechanism for keeping > time (e.g. rdtsc vs. HPET)?Quite possible, but I don''t know. Nor do I know whether there are, just like for Linux, ways to make Windows pick a different clock source (for comparison purposes). Jan
> -----Original Message----- > From: xen-devel-bounces@lists.xen.org [mailto:xen-devel- > bounces@lists.xen.org] On Behalf Of Suravee Suthikulpanit > Sent: 12 March 2013 04:40 > To: George Dunlap; Jan Beulich > Cc: xen-devel@lists.xen.org > Subject: [Xen-devel] Time Skewing on Windows XP > > Hi, > > While I was investigating the following issue on Windows XP (both 32-bit and > 64-bit): > > * AMD NPT performance regression after c/s 24770:7f79475d3de7 > Reference:http://marc.info/?l=xen-devel&m=135075376805215 > > > On the latest source form xen-unstable, I ran into an issue where the timing > on the HVM guests skewing about 2x slower than the actual wall clock time. > This results in the system time slowing down. This is regardless of the > cpufreq governor scaling. (I tried with both ondemand and performance). > > However, I don''t see the same behavior on the Win7 HVM guests. Is this a > known issue. I assume that XP and Win7 uses different mechanism for > keeping time (e.g. rdtsc vs. HPET)? >I think XP uses TSC and more recent versions of Windows use PM TIMER. Try booting XP with the /USEPMTIMER switch in boot.ini. Paul
On 3/12/2013 4:13 AM, Paul Durrant wrote:>> -----Original Message----- >> From: xen-devel-bounces@lists.xen.org [mailto:xen-devel- >> bounces@lists.xen.org] On Behalf Of Suravee Suthikulpanit >> Sent: 12 March 2013 04:40 >> To: George Dunlap; Jan Beulich >> Cc: xen-devel@lists.xen.org >> Subject: [Xen-devel] Time Skewing on Windows XP >> >> Hi, >> >> While I was investigating the following issue on Windows XP (both 32-bit and >> 64-bit): >> >> * AMD NPT performance regression after c/s 24770:7f79475d3de7 >> Reference:http://marc.info/?l=xen-devel&m=135075376805215 >> >> >> On the latest source form xen-unstable, I ran into an issue where the timing >> on the HVM guests skewing about 2x slower than the actual wall clock time. >> This results in the system time slowing down. This is regardless of the >> cpufreq governor scaling. (I tried with both ondemand and performance). >> >> However, I don''t see the same behavior on the Win7 HVM guests. Is this a >> known issue. I assume that XP and Win7 uses different mechanism for >> keeping time (e.g. rdtsc vs. HPET)? >> > I think XP uses TSC and more recent versions of Windows use PM TIMER. Try booting XP with the /USEPMTIMER switch in boot.ini. > > PaulPaul, That still doesn''t help in this case. I still see the WindowsXP guest time skewing. Also, I would not think the normal RDTSC skewing should be this severe (0.5x comparing to the wall-clock time). Suravee
On 03/12/2013 11:55 AM, Suravee Suthikulanit wrote:> On 3/12/2013 4:13 AM, Paul Durrant wrote: >>> -----Original Message----- >>> From: xen-devel-bounces@lists.xen.org [mailto:xen-devel- >>> bounces@lists.xen.org] On Behalf Of Suravee Suthikulpanit >>> Sent: 12 March 2013 04:40 >>> To: George Dunlap; Jan Beulich >>> Cc: xen-devel@lists.xen.org >>> Subject: [Xen-devel] Time Skewing on Windows XP >>> >>> Hi, >>> >>> While I was investigating the following issue on Windows XP (both >>> 32-bit and >>> 64-bit): >>> >>> * AMD NPT performance regression after c/s 24770:7f79475d3de7 >>> Reference:http://marc.info/?l=xen-devel&m=135075376805215 >>> >>> >>> On the latest source form xen-unstable, I ran into an issue where >>> the timing >>> on the HVM guests skewing about 2x slower than the actual wall clock >>> time. >>> This results in the system time slowing down. This is regardless of the >>> cpufreq governor scaling. (I tried with both ondemand and performance). >>> >>> However, I don''t see the same behavior on the Win7 HVM guests. Is >>> this a >>> known issue. I assume that XP and Win7 uses different mechanism for >>> keeping time (e.g. rdtsc vs. HPET)? >>> >> I think XP uses TSC and more recent versions of Windows use PM TIMER. >> Try booting XP with the /USEPMTIMER switch in boot.ini. >> >> Paul > Paul, > > That still doesn''t help in this case. I still see the WindowsXP guest > time skewing. Also, I would not think the normal RDTSC skewing should > be this severe (0.5x comparing to the wall-clock time).I believe TSC is usually emulated. Try adding "tsc_mode=2" to your configuration so that RDTSC(P) is never intercepted. -boris
On 3/12/2013 10:55 AM, Suravee Suthikulanit wrote:> On 3/12/2013 4:13 AM, Paul Durrant wrote: >>> -----Original Message----- >>> From: xen-devel-bounces@lists.xen.org [mailto:xen-devel- >>> bounces@lists.xen.org] On Behalf Of Suravee Suthikulpanit >>> Sent: 12 March 2013 04:40 >>> To: George Dunlap; Jan Beulich >>> Cc: xen-devel@lists.xen.org >>> Subject: [Xen-devel] Time Skewing on Windows XP >>> >>> Hi, >>> >>> While I was investigating the following issue on Windows XP (both >>> 32-bit and >>> 64-bit): >>> >>> * AMD NPT performance regression after c/s 24770:7f79475d3de7 >>> Reference:http://marc.info/?l=xen-devel&m=135075376805215 >>> >>> >>> On the latest source form xen-unstable, I ran into an issue where >>> the timing >>> on the HVM guests skewing about 2x slower than the actual wall clock >>> time. >>> This results in the system time slowing down. This is regardless of the >>> cpufreq governor scaling. (I tried with both ondemand and performance). >>> >>> However, I don''t see the same behavior on the Win7 HVM guests. Is >>> this a >>> known issue. I assume that XP and Win7 uses different mechanism for >>> keeping time (e.g. rdtsc vs. HPET)? >>> >> I think XP uses TSC and more recent versions of Windows use PM TIMER. >> Try booting XP with the /USEPMTIMER switch in boot.ini. >> >> Paul > Paul, > > That still doesn''t help in this case. I still see the WindowsXP guest > time skewing. Also, I would not think the normal RDTSC skewing should > be this severe (0.5x comparing to the wall-clock time). > > SuraveeOk, I found that the WinXP is actually using PMTIMER. After inserting the debug messages, I can see that the function pmt_update_timer() is continuously being called by the handle_pmt_io(). Suravee> _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel >
On 3/12/2013 3:03 AM, Jan Beulich wrote:> While I recall respective reports, the issue should not be present > on current -unstable, so if you do have observations like this they > will need investigation.Jan, I finally traced the issue back to the patch that this first happened. This bug started in the patch : H86/HVM: assorted RTC emulation adjustment (w/ git commit id 620d5dad54008e40798c4a0c4322aef274c36fa3) I believe there are some issues with the changes in rtc_ioport_read in the arch/x86/hvm/rtc.c and in the pt_update_irq(). Suravee
>>> On 14.03.13 at 16:16, Suravee Suthikulanit <suravee.suthikulpanit@amd.com> wrote: > I finally traced the issue back to the patch that this first happened. This > bug started in the patch : > > H86/HVM: assorted RTC emulation adjustment (w/ git commit id > 620d5dad54008e40798c4a0c4322aef274c36fa3) > > I believe there are some issues with the changes in rtc_ioport_read in the > arch/x86/hvm/rtc.c and in the pt_update_irq().One thing you may want to try is remove the call from REG_C read to rtc_timer_update() again - on a second thought it may be wrong to do it here, as (other than check_update_timer() and alarm_timer_update()) the function doesn''t change with RTC_PF getting cleared (i.e. I may have wrongly added the call in analogy to the other two). I would expect the issue to be that create_periodic_time() pointlessly destroys and then recreates an identical rate timer. I''m puzzled though that some Windows versions depend on the RTC to maintain their wall clock time... Jan
On 3/14/2013 11:06 AM, Jan Beulich wrote:>>>> On 14.03.13 at 16:16, Suravee Suthikulanit<suravee.suthikulpanit@amd.com> wrote: >> I finally traced the issue back to the patch that this first happened. This >> bug started in the patch : >> >> H86/HVM: assorted RTC emulation adjustment (w/ git commit id >> 620d5dad54008e40798c4a0c4322aef274c36fa3) >> >> I believe there are some issues with the changes in rtc_ioport_read in the >> arch/x86/hvm/rtc.c and in the pt_update_irq(). > One thing you may want to try is remove the call from REG_C > read to rtc_timer_update() again - on a second thought it may > be wrong to do it here, as (other than check_update_timer() > and alarm_timer_update()) the function doesn''t change with > RTC_PF getting cleared (i.e. I may have wrongly added the call > in analogy to the other two).If I do the followings, things start working again: 1. remove the rtc_timer_update() in the rtc_ioport_read() 2. Revert back the "rtc_periodic_cb()" and remove the "rtc_periodic_interrupt()" What was the purpose of changing the "rtc_periodic_cb()" to "rtc_periodic_interrupt()" ? Suravee> I would expect the issue to be that create_periodic_time() > pointlessly destroys and then recreates an identical rate timer. > > I''m puzzled though that some Windows versions depend on > the RTC to maintain their wall clock time... > > Jan > >
At 10:16 -0500 on 14 Mar (1363256187), Suravee Suthikulanit wrote:> On 3/12/2013 3:03 AM, Jan Beulich wrote: > > >While I recall respective reports, the issue should not be present > >on current -unstable, so if you do have observations like this they > >will need investigation. > > Jan, > > I finally traced the issue back to the patch that this first happened. > This bug started in the patch : > > H86/HVM: assorted RTC emulation adjustment (w/ git commit id > 620d5dad54008e40798c4a0c4322aef274c36fa3) > > I believe there are some issues with the changes in rtc_ioport_read in the > arch/x86/hvm/rtc.c > and in the pt_update_irq().Yes, it looks like the new call to rtc_timer_update() will reset the RTC timer every time RTC_C is read, even if it''s already running. So each second will start from the last read of RTC_C, not the start of the last second. The attached patch makes the drift better, but doesn''t fix it entirely. I''m not sure that it''s the best approach though - it seems like in the common case we shouldn''t be restarting the timer at all. Tim. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
>>> On 14.03.13 at 17:24, Tim Deegan <tim@xen.org> wrote: > The attached patch makes the drift better, but doesn''t fix it entirely. > I''m not sure that it''s the best approach though - it seems like in the > common case we shouldn''t be restarting the timer at all.Which is in line with me now thinking that I added this call in error (see my earlier reply to Suravee). Jan
At 16:06 +0000 on 14 Mar (1363277205), Jan Beulich wrote:> >>> On 14.03.13 at 16:16, Suravee Suthikulanit <suravee.suthikulpanit@amd.com> wrote: > > I finally traced the issue back to the patch that this first happened. This > > bug started in the patch : > > > > H86/HVM: assorted RTC emulation adjustment (w/ git commit id > > 620d5dad54008e40798c4a0c4322aef274c36fa3) > > > > I believe there are some issues with the changes in rtc_ioport_read in the > > arch/x86/hvm/rtc.c and in the pt_update_irq(). > > One thing you may want to try is remove the call from REG_C > read to rtc_timer_update() again - on a second thought it may > be wrong to do it here, as (other than check_update_timer() > and alarm_timer_update()) the function doesn''t change with > RTC_PF getting cleared (i.e. I may have wrongly added the call > in analogy to the other two).The call is needed because of the new code in rtc_periodic_interrupt() that disables the timer if a second passes with no RTC_C read to clear the RTC_PF flag. I suspect that getting rid of that (i.e. going back to running the timer all the time if the guest asks for interrupts) will fix it.> I would expect the issue to be that create_periodic_time() > pointlessly destroys and then recreates an identical rate timer. > > I''m puzzled though that some Windows versions depend on > the RTC to maintain their wall clock time...XP was a while ago. :) It uses the RTC in other surprising ways too -- e.g. doesn''t stop the timer when updating the CMOS wallclock, just writes the fields one at a time. Tim.
>>> On 14.03.13 at 17:21, Suravee Suthikulanit <suravee.suthikulpanit@amd.com> wrote: > On 3/14/2013 11:06 AM, Jan Beulich wrote: >>>>> On 14.03.13 at 16:16, Suravee Suthikulanit<suravee.suthikulpanit@amd.com> wrote: >>> I finally traced the issue back to the patch that this first happened. This >>> bug started in the patch : >>> >>> H86/HVM: assorted RTC emulation adjustment (w/ git commit id >>> 620d5dad54008e40798c4a0c4322aef274c36fa3) >>> >>> I believe there are some issues with the changes in rtc_ioport_read in the >>> arch/x86/hvm/rtc.c and in the pt_update_irq(). >> One thing you may want to try is remove the call from REG_C >> read to rtc_timer_update() again - on a second thought it may >> be wrong to do it here, as (other than check_update_timer() >> and alarm_timer_update()) the function doesn''t change with >> RTC_PF getting cleared (i.e. I may have wrongly added the call >> in analogy to the other two). > If I do the followings, things start working again: > 1. remove the rtc_timer_update() in the rtc_ioport_read() > 2. Revert back the "rtc_periodic_cb()" and remove the > "rtc_periodic_interrupt()" > > What was the purpose of changing the "rtc_periodic_cb()" to > "rtc_periodic_interrupt()" ?The main point was to be able to call rtc_toggle_irq() from that function (to get RTC_IRQF properly set), instead of pt_update_irq() doing only the IRQ deassert/assert pair. So removing just the questionable call to rtc_timer_update() doesn''t help? Jan
On 3/14/2013 11:34 AM, Jan Beulich wrote:>>>> On 14.03.13 at 17:21, Suravee Suthikulanit <suravee.suthikulpanit@amd.com> wrote: >> On 3/14/2013 11:06 AM, Jan Beulich wrote: >>>>>> On 14.03.13 at 16:16, Suravee Suthikulanit<suravee.suthikulpanit@amd.com> wrote: >>>> I finally traced the issue back to the patch that this first happened. This >>>> bug started in the patch : >>>> >>>> H86/HVM: assorted RTC emulation adjustment (w/ git commit id >>>> 620d5dad54008e40798c4a0c4322aef274c36fa3) >>>> >>>> I believe there are some issues with the changes in rtc_ioport_read in the >>>> arch/x86/hvm/rtc.c and in the pt_update_irq(). >>> One thing you may want to try is remove the call from REG_C >>> read to rtc_timer_update() again - on a second thought it may >>> be wrong to do it here, as (other than check_update_timer() >>> and alarm_timer_update()) the function doesn''t change with >>> RTC_PF getting cleared (i.e. I may have wrongly added the call >>> in analogy to the other two). >> If I do the followings, things start working again: >> 1. remove the rtc_timer_update() in the rtc_ioport_read() >> 2. Revert back the "rtc_periodic_cb()" and remove the >> "rtc_periodic_interrupt()" >> >> What was the purpose of changing the "rtc_periodic_cb()" to >> "rtc_periodic_interrupt()" ? > The main point was to be able to call rtc_toggle_irq() from that > function (to get RTC_IRQF properly set), instead of > pt_update_irq() doing only the IRQ deassert/assert pair. > > So removing just the questionable call to rtc_timer_update() > doesn''t help? > > JanRemoving just the call to "rtc_timer_update" causes XP failed to boot and hanged. It seems like it did not get the right timer interrupt it was looking for. Suravee> >
>>> On 14.03.13 at 17:30, Tim Deegan <tim@xen.org> wrote: > At 16:06 +0000 on 14 Mar (1363277205), Jan Beulich wrote: >> >>> On 14.03.13 at 16:16, Suravee Suthikulanit <suravee.suthikulpanit@amd.com> > wrote: >> > I finally traced the issue back to the patch that this first happened. > This >> > bug started in the patch : >> > >> > H86/HVM: assorted RTC emulation adjustment (w/ git commit id >> > 620d5dad54008e40798c4a0c4322aef274c36fa3) >> > >> > I believe there are some issues with the changes in rtc_ioport_read in the >> > arch/x86/hvm/rtc.c and in the pt_update_irq(). >> >> One thing you may want to try is remove the call from REG_C >> read to rtc_timer_update() again - on a second thought it may >> be wrong to do it here, as (other than check_update_timer() >> and alarm_timer_update()) the function doesn''t change with >> RTC_PF getting cleared (i.e. I may have wrongly added the call >> in analogy to the other two). > > The call is needed because of the new code in rtc_periodic_interrupt() > that disables the timer if a second passes with no RTC_C read to clear > the RTC_PF flag.Ah, right, of course. You see that it was many months back that I wrote that code. So we could filter on this periodic timer not currently being active (which ought to be better than your calling into create_periodic_time() with just the time delta adjusted).> I suspect that getting rid of that (i.e. going back to running the timer > all the time if the guest asks for interrupts) will fix it.But would get us back into the not spec conforming way of working... Jan
>>> On 14.03.13 at 17:38, Suravee Suthikulanit <suravee.suthikulpanit@amd.com>wrote:> On 3/14/2013 11:34 AM, Jan Beulich wrote: >>>>> On 14.03.13 at 17:21, Suravee Suthikulanit <suravee.suthikulpanit@amd.com> > wrote: >>> On 3/14/2013 11:06 AM, Jan Beulich wrote: >>>>>>> On 14.03.13 at 16:16, Suravee Suthikulanit<suravee.suthikulpanit@amd.com> > wrote: >>>>> I finally traced the issue back to the patch that this first happened. This >>>>> bug started in the patch : >>>>> >>>>> H86/HVM: assorted RTC emulation adjustment (w/ git commit id >>>>> 620d5dad54008e40798c4a0c4322aef274c36fa3) >>>>> >>>>> I believe there are some issues with the changes in rtc_ioport_read in the >>>>> arch/x86/hvm/rtc.c and in the pt_update_irq(). >>>> One thing you may want to try is remove the call from REG_C >>>> read to rtc_timer_update() again - on a second thought it may >>>> be wrong to do it here, as (other than check_update_timer() >>>> and alarm_timer_update()) the function doesn''t change with >>>> RTC_PF getting cleared (i.e. I may have wrongly added the call >>>> in analogy to the other two). >>> If I do the followings, things start working again: >>> 1. remove the rtc_timer_update() in the rtc_ioport_read() >>> 2. Revert back the "rtc_periodic_cb()" and remove the >>> "rtc_periodic_interrupt()" >>> >>> What was the purpose of changing the "rtc_periodic_cb()" to >>> "rtc_periodic_interrupt()" ? >> The main point was to be able to call rtc_toggle_irq() from that >> function (to get RTC_IRQF properly set), instead of >> pt_update_irq() doing only the IRQ deassert/assert pair. >> >> So removing just the questionable call to rtc_timer_update() >> doesn''t help? >> > Removing just the call to "rtc_timer_update" causes XP failed to boot > and hanged. It seems like it did not get the right timer interrupt it > was looking for.As was meanwhile also clarified by Tim. And as said in a response to him - we may want to gate the call on a check whether the periodic timer is actually inactive. Jan
At 16:43 +0000 on 14 Mar (1363279436), Jan Beulich wrote:> >>> On 14.03.13 at 17:30, Tim Deegan <tim@xen.org> wrote: > > At 16:06 +0000 on 14 Mar (1363277205), Jan Beulich wrote: > >> >>> On 14.03.13 at 16:16, Suravee Suthikulanit <suravee.suthikulpanit@amd.com> > > wrote: > >> > I finally traced the issue back to the patch that this first happened. > > This > >> > bug started in the patch : > >> > > >> > H86/HVM: assorted RTC emulation adjustment (w/ git commit id > >> > 620d5dad54008e40798c4a0c4322aef274c36fa3) > >> > > >> > I believe there are some issues with the changes in rtc_ioport_read in the > >> > arch/x86/hvm/rtc.c and in the pt_update_irq(). > >> > >> One thing you may want to try is remove the call from REG_C > >> read to rtc_timer_update() again - on a second thought it may > >> be wrong to do it here, as (other than check_update_timer() > >> and alarm_timer_update()) the function doesn''t change with > >> RTC_PF getting cleared (i.e. I may have wrongly added the call > >> in analogy to the other two). > > > > The call is needed because of the new code in rtc_periodic_interrupt() > > that disables the timer if a second passes with no RTC_C read to clear > > the RTC_PF flag. > > Ah, right, of course. You see that it was many months back that I > wrote that code. > > So we could filter on this periodic timer not currently being active > (which ought to be better than your calling into > create_periodic_time() with just the time delta adjusted).I tried adding a flag and gating on it but it didn''t fix the drift. I may have got it wrong, or there may be something about piix4 RTCs that Windows expects. Reading the piix4 spec it''s not at all clear that leaving RTC_PF set suppresses interrupts (even if that''s how an RTC ought to behave).
So far, this patch allows me to get the correct timing on the XP.
Suravee
diff --git a/xen/arch/x86/hvm/rtc.c b/xen/arch/x86/hvm/rtc.c
index c1e09d8..a85af5c 100644
--- a/xen/arch/x86/hvm/rtc.c
+++ b/xen/arch/x86/hvm/rtc.c
@@ -76,6 +76,15 @@ void rtc_periodic_interrupt(void *opaque)
spin_unlock(&s->lock);
}
+static void rtc_periodic_cb(struct vcpu *v, void *opaque)
+{
+ RTCState *s = opaque;
+
+ spin_lock(&s->lock);
+ s->hw.cmos_data[RTC_REG_C] |= RTC_PF | RTC_IRQF;
+ spin_unlock(&s->lock);
+}
+
/* Enable/configure/disable the periodic timer based on the RTC_PIE and
* RTC_RATE_SELECT settings */
static void rtc_timer_update(RTCState *s)
@@ -98,7 +107,7 @@ static void rtc_timer_update(RTCState *s)
{
period = 1 << (period_code - 1); /* period in 32 Khz cycles
*/
period = DIV_ROUND(period * 1000000000ULL, 32768); /* in ns */
- create_periodic_time(v, &s->pt, period, period, RTC_IRQ,
NULL, s);
+ create_periodic_time(v, &s->pt, period, period, RTC_IRQ,
rtc_periodic_cb, s);
break;
}
/* fall through */
@@ -619,7 +628,6 @@ static uint32_t rtc_ioport_read(RTCState *s,
uint32_t addr)
s->hw.cmos_data[RTC_REG_C] = 0x00;
check_update_timer(s);
alarm_timer_update(s);
- rtc_timer_update(s);
break;
default:
ret = s->hw.cmos_data[s->hw.cmos_index];
diff --git a/xen/arch/x86/hvm/vpt.c b/xen/arch/x86/hvm/vpt.c
index 46d3ec6..9c1dbfb 100644
--- a/xen/arch/x86/hvm/vpt.c
+++ b/xen/arch/x86/hvm/vpt.c
@@ -259,8 +259,6 @@ int pt_update_irq(struct vcpu *v)
if ( is_lapic )
vlapic_set_irq(vcpu_vlapic(v), irq, 0);
- else if ( irq == RTC_IRQ && pt_priv )
- rtc_periodic_interrupt(pt_priv);
else
{
hvm_isa_irq_deassert(v->domain, irq);
>>> On 14.03.13 at 21:23, Suravee Suthikulanit <suravee.suthikulpanit@amd.com> wrote: > So far, this patch allows me to get the correct timing on the XP.At the price of never stopping the periodic time - other than said yesterday, _that_ was the main point of the flow change. I.e. making the host wake up more frequently even on an otherwise idle system. It ought to be possible to keep the timer off when unused by the guest _and_ have the time remain stable...> --- a/xen/arch/x86/hvm/rtc.c > +++ b/xen/arch/x86/hvm/rtc.c > @@ -76,6 +76,15 @@ void rtc_periodic_interrupt(void *opaque) > spin_unlock(&s->lock); > } > > +static void rtc_periodic_cb(struct vcpu *v, void *opaque) > +{ > + RTCState *s = opaque; > + > + spin_lock(&s->lock); > + s->hw.cmos_data[RTC_REG_C] |= RTC_PF | RTC_IRQF; > + spin_unlock(&s->lock); > +}And if we indeed revert that part of the original patch, then properly - this function then ought to replace the now dead rtc_periodic_interrupt(). Jan> + > /* Enable/configure/disable the periodic timer based on the RTC_PIE and > * RTC_RATE_SELECT settings */ > static void rtc_timer_update(RTCState *s) > @@ -98,7 +107,7 @@ static void rtc_timer_update(RTCState *s) > { > period = 1 << (period_code - 1); /* period in 32 Khz cycles */ > period = DIV_ROUND(period * 1000000000ULL, 32768); /* in ns */ > - create_periodic_time(v, &s->pt, period, period, RTC_IRQ, > NULL, s); > + create_periodic_time(v, &s->pt, period, period, RTC_IRQ, > rtc_periodic_cb, s); > break; > } > /* fall through */ > @@ -619,7 +628,6 @@ static uint32_t rtc_ioport_read(RTCState *s, > uint32_t addr) > s->hw.cmos_data[RTC_REG_C] = 0x00; > check_update_timer(s); > alarm_timer_update(s); > - rtc_timer_update(s); > break; > default: > ret = s->hw.cmos_data[s->hw.cmos_index]; > diff --git a/xen/arch/x86/hvm/vpt.c b/xen/arch/x86/hvm/vpt.c > index 46d3ec6..9c1dbfb 100644 > --- a/xen/arch/x86/hvm/vpt.c > +++ b/xen/arch/x86/hvm/vpt.c > @@ -259,8 +259,6 @@ int pt_update_irq(struct vcpu *v) > > if ( is_lapic ) > vlapic_set_irq(vcpu_vlapic(v), irq, 0); > - else if ( irq == RTC_IRQ && pt_priv ) > - rtc_periodic_interrupt(pt_priv); > else > { > hvm_isa_irq_deassert(v->domain, irq);
At 08:23 +0000 on 15 Mar (1363335830), Jan Beulich wrote:> >>> On 14.03.13 at 21:23, Suravee Suthikulanit <suravee.suthikulpanit@amd.com> wrote: > > So far, this patch allows me to get the correct timing on the XP. > > At the price of never stopping the periodic time - other than said > yesterday, _that_ was the main point of the flow change. I.e. > making the host wake up more frequently even on an otherwise > idle system. > > It ought to be possible to keep the timer off when unused by > the guest _and_ have the time remain stable...Sure, but right now, timekeeping in XP is totally borked. So we should probably revert 620d5dad54008e40798c4a0c4322aef274c36fa3 until a proper fix can be found. Cheers, Tim.> > --- a/xen/arch/x86/hvm/rtc.c > > +++ b/xen/arch/x86/hvm/rtc.c > > @@ -76,6 +76,15 @@ void rtc_periodic_interrupt(void *opaque) > > spin_unlock(&s->lock); > > } > > > > +static void rtc_periodic_cb(struct vcpu *v, void *opaque) > > +{ > > + RTCState *s = opaque; > > + > > + spin_lock(&s->lock); > > + s->hw.cmos_data[RTC_REG_C] |= RTC_PF | RTC_IRQF; > > + spin_unlock(&s->lock); > > +} > > And if we indeed revert that part of the original patch, then > properly - this function then ought to replace the now dead > rtc_periodic_interrupt(). > > Jan > > > + > > /* Enable/configure/disable the periodic timer based on the RTC_PIE and > > * RTC_RATE_SELECT settings */ > > static void rtc_timer_update(RTCState *s) > > @@ -98,7 +107,7 @@ static void rtc_timer_update(RTCState *s) > > { > > period = 1 << (period_code - 1); /* period in 32 Khz cycles */ > > period = DIV_ROUND(period * 1000000000ULL, 32768); /* in ns */ > > - create_periodic_time(v, &s->pt, period, period, RTC_IRQ, > > NULL, s); > > + create_periodic_time(v, &s->pt, period, period, RTC_IRQ, > > rtc_periodic_cb, s); > > break; > > } > > /* fall through */ > > @@ -619,7 +628,6 @@ static uint32_t rtc_ioport_read(RTCState *s, > > uint32_t addr) > > s->hw.cmos_data[RTC_REG_C] = 0x00; > > check_update_timer(s); > > alarm_timer_update(s); > > - rtc_timer_update(s); > > break; > > default: > > ret = s->hw.cmos_data[s->hw.cmos_index]; > > diff --git a/xen/arch/x86/hvm/vpt.c b/xen/arch/x86/hvm/vpt.c > > index 46d3ec6..9c1dbfb 100644 > > --- a/xen/arch/x86/hvm/vpt.c > > +++ b/xen/arch/x86/hvm/vpt.c > > @@ -259,8 +259,6 @@ int pt_update_irq(struct vcpu *v) > > > > if ( is_lapic ) > > vlapic_set_irq(vcpu_vlapic(v), irq, 0); > > - else if ( irq == RTC_IRQ && pt_priv ) > > - rtc_periodic_interrupt(pt_priv); > > else > > { > > hvm_isa_irq_deassert(v->domain, irq); > > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
>>> On 21.03.13 at 13:33, Tim Deegan <tim@xen.org> wrote: > At 08:23 +0000 on 15 Mar (1363335830), Jan Beulich wrote: >> >>> On 14.03.13 at 21:23, Suravee Suthikulanit <suravee.suthikulpanit@amd.com> > wrote: >> > So far, this patch allows me to get the correct timing on the XP. >> >> At the price of never stopping the periodic time - other than said >> yesterday, _that_ was the main point of the flow change. I.e. >> making the host wake up more frequently even on an otherwise >> idle system. >> >> It ought to be possible to keep the timer off when unused by >> the guest _and_ have the time remain stable... > > Sure, but right now, timekeeping in XP is totally borked. So we should > probably revert 620d5dad54008e40798c4a0c4322aef274c36fa3 until a proper > fix can be found.Hmm, perhaps (but then I''d like to try to keep the alarm timer related changes in). In particular if your attempt to gate the call to create_periodic_timer() on a flag was together with the start time relative calculation that you had sent in patch form. If it wasn''t, it''d like to give that one a try first before going the revert route. Jan