Time Skewing on Windows XP

Time Skewing on Windows XP

[Suravee Suthikulpanit]

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

Re: Time Skewing on Windows XP

[Steven Haigh]

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

Re: Time Skewing on Windows XP

[Jan Beulich]

>>> 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

Re: Time Skewing on Windows XP

[Paul Durrant]

> -----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

Re: Time Skewing on Windows XP

[Suravee Suthikulanit]

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).

Suravee

Re: Time Skewing on Windows XP

[Boris Ostrovsky]

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

Re: Time Skewing on Windows XP

[Suravee Suthikulanit]

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).
>
> Suravee
Ok, 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
>

Re: Time Skewing on Windows XP

[Suravee Suthikulanit]

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

Re: Time Skewing on Windows XP

[Jan Beulich]

>>> 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

Re: Time Skewing on Windows XP

[Suravee Suthikulanit]

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
>
>

Re: Time Skewing on Windows XP

[Tim Deegan]

[-- Attachment #1: Type: text/plain, Size: 1080 bytes --]

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.

[-- Attachment #2: x --]
[-- Type: text/plain, Size: 1603 bytes --]

diff --git a/xen/arch/x86/hvm/rtc.c b/xen/arch/x86/hvm/rtc.c
index c1e09d8..e9aa81a 100644
--- a/xen/arch/x86/hvm/rtc.c
+++ b/xen/arch/x86/hvm/rtc.c
@@ -80,7 +80,7 @@ void rtc_periodic_interrupt(void *opaque)
  * RTC_RATE_SELECT settings */
 static void rtc_timer_update(RTCState *s)
 {
-    int period_code, period;
+    int period_code, period, delta;
     struct vcpu *v = vrtc_vcpu(s);
 
     ASSERT(spin_is_locked(&s->lock));
@@ -98,7 +98,8 @@ 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);
+            delta = period - ((NOW() - s->start_time) % period);
+            create_periodic_time(v, &s->pt, delta, period, RTC_IRQ, NULL, s);
             break;
         }
         /* fall through */
@@ -740,6 +741,7 @@ void rtc_init(struct domain *d)
     s->hw.cmos_data[RTC_REG_D] = RTC_VRT;
 
     s->current_tm = gmtime(get_localtime(d));
+    s->start_time = NOW();
 
     rtc_copy_date(s);
 
diff --git a/xen/include/asm-x86/hvm/vpt.h b/xen/include/asm-x86/hvm/vpt.h
index c75297b..d6155b8 100644
--- a/xen/include/asm-x86/hvm/vpt.h
+++ b/xen/include/asm-x86/hvm/vpt.h
@@ -105,7 +105,7 @@ typedef struct RTCState {
     /* RTC's idea of the current time */
     struct tm current_tm;
     /* second update */
-    int64_t next_second_time;
+    s_time_t start_time;
     struct periodic_time pt;
     /* update-ended timer */
     struct timer update_timer;

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: Time Skewing on Windows XP

[Jan Beulich]

>>> 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

Re: Time Skewing on Windows XP

[Tim Deegan]

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.

Re: Time Skewing on Windows XP

[Jan Beulich]

>>> 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

Re: Time Skewing on Windows XP

[Suravee Suthikulanit]

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?
>
> Jan
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.

Suravee
>
>

Re: Time Skewing on Windows XP

[Jan Beulich]

>>> 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

Re: Time Skewing on Windows XP

[Jan Beulich]

>>> 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

Re: Time Skewing on Windows XP

[Tim Deegan]

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).

Re: Time Skewing on Windows XP

[Suravee Suthikulanit]

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);

Re: Time Skewing on Windows XP

[Jan Beulich]

>>> 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);

Re: Time Skewing on Windows XP

[Tim Deegan]

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

Re: Time Skewing on Windows XP

[Jan Beulich]

>>> 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