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

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

[ANNIE LI]

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

Hi

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

[-- Attachment #2: rtc-timeoffset.patch --]
[-- Type: text/plain, Size: 1329 bytes --]

# 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 <annie.li@oracle.com>

diff -r f2543f449a49 xen/arch/x86/hvm/rtc.c
--- a/xen/arch/x86/hvm/rtc.c	Mon Feb 13 17:57:47 2012 +0000
+++ b/xen/arch/x86/hvm/rtc.c	Mon Feb 20 14:39:00 2012 +0800
@@ -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 

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

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

Re: [PATCH] hvm: Correct RTC time offset update error due to tm->tm_year

[Zhang, Yang Z]

> -----Original Message-----
> From: xen-devel-bounces@lists.xensource.com
> [mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of ANNIE LI
> Sent: Monday, February 20, 2012 3:20 PM
> 
> Hi
> 
> 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

Re: [PATCH] hvm: Correct RTC time offset update error due to tm->tm_year

[young zhang]

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 <yang.z.zhang@intel.com>:
>> -----Original Message-----
>> From: xen-devel-bounces@lists.xensource.com
>> [mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of ANNIE LI
>> Sent: Monday, February 20, 2012 3:20 PM
>>
>> Hi
>>
>> 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
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
>


-- 
young

Re: [PATCH] hvm: Correct RTC time offset update error due to tm->tm_year

[Zhang, Yang Z]

> -----Original Message-----
> From: young zhang [mailto:young.zhang.free@gmail.com]
> Sent: Monday, February 20, 2012 11:04 PM
> To: Zhang, Yang Z
> Cc: ANNIE LI; xen-devel@lists.xensource.com; Kurt Hackel; Dan Magenheimer;
> Konrad Rzeszutek Wilk
> Subject: Re: [Xen-devel] [PATCH] hvm: Correct RTC time offset update error due
> to tm->tm_year
> 
> 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

Re: [PATCH] hvm: Correct RTC time offset update error due to tm->tm_year

[ANNIE LI]

On 2012-2-21 7:54, Zhang, Yang Z wrote:
>> -----Original Message-----
>> From: young zhang [mailto:young.zhang.free@gmail.com]
>> Sent: Monday, February 20, 2012 11:04 PM
>> To: Zhang, Yang Z
>> Cc: ANNIE LI; xen-devel@lists.xensource.com; Kurt Hackel; Dan Magenheimer;
>> Konrad Rzeszutek Wilk
>> Subject: Re: [Xen-devel] [PATCH] hvm: Correct RTC time offset update error due
>> to tm->tm_year
>>
>> 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)
---
 > 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 */
---
 >       /* 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 */
---
 >               mon += 12;
21d18
<

Thanks
Annie
>
> best regards
> yang
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xensource.com/xen-devel

Re: [PATCH] hvm: Correct RTC time offset update error due to tm->tm_year

[Ian Campbell]

On Tue, 2012-02-21 at 02:31 +0000, ANNIE LI wrote:
> 
> On 2012-2-21 7:54, Zhang, Yang Z wrote:
> >> -----Original Message-----
> >> From: young zhang [mailto:young.zhang.free@gmail.com]
> >> Sent: Monday, February 20, 2012 11:04 PM
> >> To: Zhang, Yang Z
> >> Cc: ANNIE LI; xen-devel@lists.xensource.com; Kurt Hackel; Dan Magenheimer;
> >> Konrad Rzeszutek Wilk
> >> Subject: Re: [Xen-devel] [PATCH] hvm: Correct RTC time offset update error due
> >> to tm->tm_year
> >>
> >> 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.

> 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)
> ---
>  > 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 */
> ---
>  >       /* 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 */
> ---
>  >               mon += 12;
> 21d18
> <
> 
> Thanks
> Annie
> >
> > best regards
> > yang
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xensource.com/xen-devel
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xensource.com/xen-devel

Re: [PATCH] hvm: Correct RTC time offset update error due to tm->tm_year

[annie li]

Thanks a lot for your reply, Ian.
I guess there is misunderstanding here, see following,
> 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.
> 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 :-) )
> 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
> Ian.
>
>   
>> 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)
>> ---
>>  > 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 */
>> ---
>>  >       /* 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 */
>> ---
>>  >               mon += 12;
>> 21d18
>> <
>>
>> Thanks
>> Annie
>>     
>>> best regards
>>> yang
>>>
>>> _______________________________________________
>>> Xen-devel mailing list
>>> Xen-devel@lists.xen.org
>>> http://lists.xensource.com/xen-devel
>>>       
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xensource.com/xen-devel
>>     
>
>
>   

Re: [PATCH] hvm: Correct RTC time offset update error due to tm->tm_year

[Ian Campbell]

On Wed, 2012-02-22 at 13:10 +0000, annie li wrote:
> Thanks a lot for your reply, Ian.
> I guess there is misunderstanding here, see following,
> > 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.
> > 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 :-) )
> > 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.

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.

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

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.

Re: [PATCH] hvm: Correct RTC time offset update error due to tm->tm_year

[annie li]

On 2012-2-22 21:58, Ian Campbell wrote:
>
> 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
> Ian.
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>   

Re: [PATCH] hvm: Correct RTC time offset update error due to tm->tm_year

[annie li]

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



On 2012-2-22 21:58, Ian Campbell wrote:
>
> 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

[-- Attachment #2: rtc-timeoffset.patch --]
[-- Type: text/plain, Size: 2147 bytes --]

# 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 <annie.li@oracle.com>

diff -r f2543f449a49 xen/arch/x86/hvm/rtc.c
--- a/xen/arch/x86/hvm/rtc.c	Mon Feb 13 17:57:47 2012 +0000
+++ b/xen/arch/x86/hvm/rtc.c	Thu Feb 23 18:21:19 2012 +0800
@@ -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 )
                 {

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

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

Re: [PATCH] hvm: Correct RTC time offset update error due to tm->tm_year

[annie li]

On 2012-2-23 22:52, annie li wrote:
>
>
> On 2012-2-22 21:58, Ian Campbell wrote:
>>
>> 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

                {