hvm: Correct RTC time offset update error due to tm->tm_year

PATCH In review ANNIE LI 4 participants 11 messages Feb 20, 2012 – Feb 23, 2012

Share & tools

ANNIE LI

author

2012-02-20 07:19 UTC #0 lore raw

PATCH 1/1 hvm: Correct RTC time offset update error due to tm->tm_year

In rtc_set_time, mktime is called to calculate seconds since 1970/01/01, input parameters of mktime are required to be in normal date format. Such as: year=1980, mon=12, day=31, hour=23, min=59, sec=59. However, the current input parameter of mktime is tm->tm_year, and it is the number of years since 1900. (For example, if current time is 2012/12/31, and tm->tm_year is 112). This is not suitable for requirement of mktime. So I think tm->tm_year should be changed to tm->tm_year+1900 when calling mktime. Please check the patch attached.

Thanks,
Annie

# HG changeset patch
# Parent f2543f449a49b8979becbf6888e009973427089a
hvm: correct RTC time offset update error due to tm->tm_year

mktime requires input year in normal date format, i.e. 1980. So it is necessary to change tm->tm_year to tm->tm_year+1900. Otherwise, the calculation result of mktime is incorrect.

Signed-off-by Annie Li <[email protected]>

xen/arch/x86/hvm/rtc.c +2 -2

diff -r f2543f449a49 xen/arch/x86/hvm/rtc.c
@@ -165,7 +165,7 @@ static void rtc_set_time(RTCState *s)
       
     ASSERT(spin_is_locked(&s->lock));
 
-    before = mktime(tm->tm_year, tm->tm_mon + 1, tm->tm_mday,
+    before = mktime(tm->tm_year + 1900, tm->tm_mon + 1, tm->tm_mday,
 		    tm->tm_hour, tm->tm_min, tm->tm_sec);
     
     tm->tm_sec = from_bcd(s, s->hw.cmos_data[RTC_SECONDS]);

@@ -179,7 +179,7 @@ static void rtc_set_time(RTCState *s)
     tm->tm_mon = from_bcd(s, s->hw.cmos_data[RTC_MONTH]) - 1;
     tm->tm_year = from_bcd(s, s->hw.cmos_data[RTC_YEAR]) + 100;
 
-    after = mktime(tm->tm_year, tm->tm_mon + 1, tm->tm_mday,
+    after = mktime(tm->tm_year + 1900, tm->tm_mon + 1, tm->tm_mday,
                    tm->tm_hour, tm->tm_min, tm->tm_sec);
 
     /* We use the guest's setting of the RTC to define the local-time

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Zhang, Yang Z

2012-02-20 14:53 UTC #1 lore raw

Quoted 7 lines

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of ANNIE LI
Sent: Monday, February 20, 2012 3:20 PM
Hi

Quoted 9 lines

In rtc_set_time, mktime is called to calculate seconds since 1970/01/01, input parameters of mktime are required to be in normal date format.
Such as: year=1980, mon=12, day=31, hour=23, min=59, sec=59. However, the current input parameter of mktime is tm->tm_year, and it is the number of years since 1900. (For example, if current time is 2012/12/31, and tm->tm_year is 112). This is not suitable for requirement of mktime.
So I think tm->tm_year should be changed to tm->tm_year+1900 when calling mktime. Please check the patch attached.

No, tm_year is the number of years since 1900, not the normal date format.

best regards
yang

young zhang

2012-02-20 15:04 UTC #2 lore raw

The mktime which used in xen is different from standard C. I think the best way is change it same as normal mktime, or else, other people will make the same mistake too.

2012/2/20, Zhang, Yang Z <[email protected]>:

Quoted 28 lines

Quoted 7 lines
-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of ANNIE LI
Sent: Monday, February 20, 2012 3:20 PM
Hi
Quoted 12 lines
In rtc_set_time, mktime is called to calculate seconds since 1970/01/01, input
parameters of mktime are required to be in normal date format. Such as: year=1980, mon=12, day=31, hour=23, min=59, sec=59. However, the current input parameter of mktime is tm->tm_year, and it is the number of years
since 1900. (For example, if current time is 2012/12/31, and tm->tm_year is 112).
This is not suitable for requirement of mktime.
So I think tm->tm_year should be changed to tm->tm_year+1900 when calling mktime. Please check the patch attached.
No, tm_year is the number of years since 1900, not the normal date format.
best regards
yang
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Signature · 1 line

young

Zhang, Yang Z

2012-02-20 23:54 UTC #3 lore raw

Quoted 9 lines

-----Original Message-----
From: young zhang [mailto:[email protected]]
Sent: Monday, February 20, 2012 11:04 PM
To: Zhang, Yang Z
Cc: ANNIE LI; [email protected]; Kurt Hackel; Dan Magenheimer; Konrad Rzeszutek Wilk
Subject: Re: [Xen-devel] [PATCH] hvm: Correct RTC time offset update error due to tm->tm_year

Quoted 3 lines

The mktime which used in xen is different from standard C. I think the best way is change it same as normal mktime, or else, other people will make the same mistake too.

yes. The name will mislead the people who not look into the code, including me. :)

best regards
yang

ANNIE LI

author

2012-02-21 02:31 UTC #4 lore raw

Quoted 9 lines · Yang Z

Quoted 9 lines
-----Original Message-----
From: young zhang [mailto:[email protected]]
Sent: Monday, February 20, 2012 11:04 PM
To: Zhang, Yang Z
Cc: ANNIE LI; [email protected]; Kurt Hackel; Dan Magenheimer; Konrad Rzeszutek Wilk
Subject: Re: [Xen-devel] [PATCH] hvm: Correct RTC time offset update error due to tm->tm_year

Quoted 4 lines

Quoted 3 lines
The mktime which used in xen is different from standard C. I think the best way is change it same as normal mktime, or else, other people will make the same mistake too.
yes. The name will mislead the people who not look into the code, including me. :)

I compared the mktime of xen with mktime of linux. The code is almost the same, I do not understand why xen requires the input year is tm->tm_year, not tm->tm_year+1900. Am I missing somthing?

See following diff file which is created between mktime of linux and mktime of xen, (I did some tab/space format changes for comparison)

diff linux-mktime.c xen-mktime.c
2,4c2,4
< mktime(const unsigned int year0, const unsigned int mon0,
< const unsigned int day, const unsigned int hour,
< const unsigned int min, const unsigned int sec)
---

Quoted 3 lines

mktime (unsigned int year, unsigned int mon,
unsigned int day, unsigned int hour,
unsigned int min, unsigned int sec)

6,8c6
< unsigned int mon = mon0, year = year0;
<
< /* 1..12 -> 11,12,1..10 */
---

Quoted 1 line

      /* 1..12 -> 11,12,1..10: put Feb last since it has a leap day. */

10c8
< mon += 12; /* Puts Feb last since it has leap day */
---

Quoted 1 line

              mon += 12;

21d18
<

Thanks
Annie

Quoted 8 lines

best regards
yang
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Ian Campbell

2012-02-22 11:05 UTC #5 lore raw

Quoted 11 lines · ANNIE LI

Quoted 9 lines · Yang Z
Quoted 9 lines
-----Original Message-----
From: young zhang [mailto:[email protected]]
Sent: Monday, February 20, 2012 11:04 PM
To: Zhang, Yang Z
Cc: ANNIE LI; [email protected]; Kurt Hackel; Dan Magenheimer; Konrad Rzeszutek Wilk
Subject: Re: [Xen-devel] [PATCH] hvm: Correct RTC time offset update error due to tm->tm_year

Quoted 8 lines

Quoted 4 lines
Quoted 3 lines
The mktime which used in xen is different from standard C. I think the best way is change it same as normal mktime, or else, other people will make the same mistake too.
yes. The name will mislead the people who not look into the code, including me. :)
I compared the mktime of xen with mktime of linux. The code is almost the same, I do not understand why xen requires the input year is tm->tm_year, not tm->tm_year+1900. Am I missing somthing?

The Linux kernel's version of mktime and Xen's version both differ from standard C/POSIX defined function (in fact I expect Xen's is derived from Linux's). Not least because they take a bunch of values instead of a struct tm as arguments (i.e. they take unsigned int year, not tm->tm_year).

If you wanted to compare Xen vs. a POSIX compliant mktime you'd probably want the libc version. The Xen and Linux mktime()s certainly differ substantially from the eglibc one.

I don't think you can expect that the in-kernel mktime necessarily has the same interface as POSIX documents, there is certainly no particular reason why they should or must (a kernel is not a POSIX environment). The comment preceding Xen's mktime makes no mention of offsetting anything by 1900 (or anything else) and I believe its implementation is consistent with its defined interface.

Ian.

Quoted 40 lines

See following diff file which is created between mktime of linux and mktime of xen, (I did some tab/space format changes for comparison)
diff linux-mktime.c xen-mktime.c
2,4c2,4
< mktime(const unsigned int year0, const unsigned int mon0,
< const unsigned int day, const unsigned int hour,
< const unsigned int min, const unsigned int sec)
---
Quoted 3 lines
mktime (unsigned int year, unsigned int mon,
unsigned int day, unsigned int hour,
unsigned int min, unsigned int sec)
6,8c6
< unsigned int mon = mon0, year = year0;
<
< /* 1..12 -> 11,12,1..10 */
---
Quoted 1 line
      /* 1..12 -> 11,12,1..10: put Feb last since it has a leap day. */
10c8
< mon += 12; /* Puts Feb last since it has leap day */
---
Quoted 1 line
              mon += 12;
21d18
<
Thanks
Annie
Quoted 8 lines
best regards
yang
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

annie li

author

2012-02-22 13:10 UTC #6 lore raw

Thanks a lot for your reply, Ian.
I guess there is misunderstanding here, see following,

Quoted 6 lines

The Linux kernel's version of mktime and Xen's version both differ from standard C/POSIX defined function (in fact I expect Xen's is derived from Linux's). Not least because they take a bunch of values instead of a struct tm as arguments (i.e. they take unsigned int year, not tm->tm_year).

Yes, Xen's is same as Linux's.

Quoted 4 lines

If you wanted to compare Xen vs. a POSIX compliant mktime you'd probably want the libc version. The Xen and Linux mktime()s certainly differ substantially from the eglibc one.

Thanks, my original aim is to address an issue in rtc_set_time of \xen\arch\x86\hvm\rtc.c. (at least I thought it was, need your confirmation :-) )

Quoted 7 lines

I don't think you can expect that the in-kernel mktime necessarily has the same interface as POSIX documents, there is certainly no particular reason why they should or must (a kernel is not a POSIX environment). The comment preceding Xen's mktime makes no mention of offsetting anything by 1900 (or anything else) and I believe its implementation is consistent with its defined interface.

Yes, the comments does not mention of offsetting anything by 1900, and this is the reason why I created the patch.
In \xen\arch\x86\hvm\rtc.c, rtc_set_time call mktime to calculate the seconds since 1970/01/01, the input parameters of mktime are required to be in normal date format. Such as: year=1980, mon=12, day=31, hour=23, min=59, sec=59. (Just like Linux)

However, current xen code has some problem when dealing with tm->tm_year, see following,
tm->tm_year = from_bcd(s, s->hw.cmos_data[RTC_YEAR]) + 100;
after = mktime(tm->tm_year, tm->tm_mon + 1, tm->tm_mday,

                  tm->tm_hour, tm->tm_min, tm->tm_sec);

(For example, if current time is 2012/12/31, tm->tm_year is 112 here)

To meet the requirement of Xen's mktime, tm->tm_year should be changed to tm->tm_year+1900 when calling mktime in Xen. See following,
after = mktime(tm->tm_year+1900, tm->tm_mon+1, tm->tm_mday,

                  tm->tm_hour, tm->tm_min, tm->tm_sec);

Thanks,
Annie

Quoted 46 lines

Ian.
Quoted 40 lines
See following diff file which is created between mktime of linux and mktime of xen, (I did some tab/space format changes for comparison)
diff linux-mktime.c xen-mktime.c
2,4c2,4
< mktime(const unsigned int year0, const unsigned int mon0,
< const unsigned int day, const unsigned int hour,
< const unsigned int min, const unsigned int sec)
---
Quoted 3 lines
mktime (unsigned int year, unsigned int mon,
unsigned int day, unsigned int hour,
unsigned int min, unsigned int sec)
6,8c6
< unsigned int mon = mon0, year = year0;
<
< /* 1..12 -> 11,12,1..10 */
---
Quoted 1 line
      /* 1..12 -> 11,12,1..10: put Feb last since it has a leap day. */
10c8
< mon += 12; /* Puts Feb last since it has leap day */
---
Quoted 1 line
              mon += 12;
21d18
<
Thanks
Annie
Quoted 8 lines
best regards
yang
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Ian Campbell

2012-02-22 13:58 UTC #7 lore raw

Quoted 23 lines · annie li

Thanks a lot for your reply, Ian.
I guess there is misunderstanding here, see following,
Quoted 6 lines
The Linux kernel's version of mktime and Xen's version both differ from standard C/POSIX defined function (in fact I expect Xen's is derived from Linux's). Not least because they take a bunch of values instead of a struct tm as arguments (i.e. they take unsigned int year, not tm->tm_year).
Yes, Xen's is same as Linux's.
Quoted 4 lines
If you wanted to compare Xen vs. a POSIX compliant mktime you'd probably want the libc version. The Xen and Linux mktime()s certainly differ substantially from the eglibc one.
Thanks, my original aim is to address an issue in rtc_set_time of \xen\arch\x86\hvm\rtc.c. (at least I thought it was, need your confirmation :-) )
Quoted 7 lines
I don't think you can expect that the in-kernel mktime necessarily has the same interface as POSIX documents, there is certainly no particular reason why they should or must (a kernel is not a POSIX environment). The comment preceding Xen's mktime makes no mention of offsetting anything by 1900 (or anything else) and I believe its implementation is consistent with its defined interface.

Quoted 2 lines

Yes, the comments does not mention of offsetting anything by 1900, and this is the reason why I created the patch.

Oh -- I see now that the original patch is fixing the caller and it was someone else who introduced this red-herring about the mktime interface itself, sorry.

Quoted 10 lines

In \xen\arch\x86\hvm\rtc.c, rtc_set_time call mktime to calculate the seconds since 1970/01/01, the input parameters of mktime are required to be in normal date format. Such as: year=1980, mon=12, day=31, hour=23, min=59, sec=59. (Just like Linux)
However, current xen code has some problem when dealing with tm->tm_year, see following,
    tm->tm_year = from_bcd(s, s->hw.cmos_data[RTC_YEAR]) + 100;
    after = mktime(tm->tm_year, tm->tm_mon + 1, tm->tm_mday,
                   tm->tm_hour, tm->tm_min, tm->tm_sec);

Quoted 6 lines

(For example, if current time is 2012/12/31, tm->tm_year is 112 here)
To meet the requirement of Xen's mktime, tm->tm_year should be changed to tm->tm_year+1900 when calling mktime in Xen. See following,
    after = mktime(tm->tm_year+1900, tm->tm_mon+1, tm->tm_mday,
                   tm->tm_hour, tm->tm_min, tm->tm_sec);

Yes, this looks plausible to me (although I'm no expert on this code). Something similar happens in rtc_next_second.

Perhaps it would be better to add a function or macro to do the conversion, such that it is somewhat self documenting? Or at least make the 1900 a #define with a suitable name.

Ian.

annie li

author

2012-02-22 14:35 UTC #8 lore raw

Quoted 8 lines · Ian Campbell

Yes, this looks plausible to me (although I'm no expert on this code). Something similar happens in rtc_next_second.
Perhaps it would be better to add a function or macro to do the conversion, such that it is somewhat self documenting? Or at least make the 1900 a #define with a suitable name.

Sure.
How about
#define epoch_year 1900
#define get_year(x) (x + epoch_year)
?

Thanks,
Annie

Quoted 9 lines

Ian.
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

annie li

author

2012-02-23 14:52 UTC #9 lore raw

In reply to Ian Campbell

Quoted 8 lines · Ian Campbell

Yes, this looks plausible to me (although I'm no expert on this code). Something similar happens in rtc_next_second.
Perhaps it would be better to add a function or macro to do the conversion, such that it is somewhat self documenting? Or at least make the 1900 a #define with a suitable name.

I changed the 1900 to a macro and added a new function get_year. The new patch is attached.

Thanks
Annie

# HG changeset patch
# Parent f2543f449a49b8979becbf6888e009973427089a
hvm: correct RTC time offset update error due to tm->tm_year

tm->tm_year in rtc.c is year number offsetting from 1900. So it is necessary to add the offset 1900 when calling mktime funtion in Xen. Otherwise, the calculation result of mktime is incorrect.

Signed-off-by Annie Li <[email protected]>

xen/arch/x86/hvm/rtc.c +5 -3

diff -r f2543f449a49 xen/arch/x86/hvm/rtc.c
@@ -33,6 +33,8 @@
 #define vrtc_domain(x) (container_of((x), struct domain, \
                                      arch.hvm_domain.pl_time.vrtc))
 #define vrtc_vcpu(x)   (pt_global_vcpu_target(vrtc_domain(x)))
+#define epoch_year     1900
+#define get_year(x)    (x + epoch_year)
 
 static void rtc_periodic_cb(struct vcpu *v, void *opaque)
 {

@@ -165,7 +167,7 @@ static void rtc_set_time(RTCState *s)
       
     ASSERT(spin_is_locked(&s->lock));
 
-    before = mktime(tm->tm_year, tm->tm_mon + 1, tm->tm_mday,
+    before = mktime(get_year(tm->tm_year), tm->tm_mon + 1, tm->tm_mday,
 		    tm->tm_hour, tm->tm_min, tm->tm_sec);
     
     tm->tm_sec = from_bcd(s, s->hw.cmos_data[RTC_SECONDS]);

@@ -179,7 +181,7 @@ static void rtc_set_time(RTCState *s)
     tm->tm_mon = from_bcd(s, s->hw.cmos_data[RTC_MONTH]) - 1;
     tm->tm_year = from_bcd(s, s->hw.cmos_data[RTC_YEAR]) + 100;
 
-    after = mktime(tm->tm_year, tm->tm_mon + 1, tm->tm_mday,
+    after = mktime(get_year(tm->tm_year), tm->tm_mon + 1, tm->tm_mday,
                    tm->tm_hour, tm->tm_min, tm->tm_sec);
 
     /* We use the guest's setting of the RTC to define the local-time

@@ -257,7 +259,7 @@ static void rtc_next_second(RTCState *s)
                 if ( (unsigned)tm->tm_wday >= 7 )
                     tm->tm_wday = 0;
                 days_in_month = get_days_in_month(tm->tm_mon, 
-                                                  tm->tm_year + 1900);
+                                                  get_year(tm->tm_year));
                 tm->tm_mday++;
                 if ( tm->tm_mday < 1 )
                 {

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

annie li

author

2012-02-23 15:17 UTC #10 lore raw

Quoted 11 lines · annie li

Quoted 7 lines · Ian Campbell
Yes, this looks plausible to me (although I'm no expert on this code). Something similar happens in rtc_next_second.
Perhaps it would be better to add a function or macro to do the conversion, such that it is somewhat self documenting? Or at least make the 1900 a #define with a suitable name.
I changed the 1900 to a macro and added a new function get_year. The new patch is attached.

Sorry, two macro define: epoch_year and get_year.

Thanks
Annie