[1/4] rtc: fix 12-hour mode

Hours in 12-hour mode are in the 1-12 range, not 0-11.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/mc146818rtc.c |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)

On 11/22/2011 02:00 AM, Paolo Bonzini wrote:
> Hours in 12-hour mode are in the 1-12 range, not 0-11.
Interesting. I would like to know how you could find this problem. It 
seems linux driver never changes the format  and 24-hour is default in 
rtc emulation code. So how did it expose and how to test it?

Thanks.
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
> ---
>   hw/mc146818rtc.c |   11 +++++++----
>   1 files changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c
> index 2aaca2f..14c8cb9 100644
> --- a/hw/mc146818rtc.c
> +++ b/hw/mc146818rtc.c
> @@ -296,9 +296,11 @@ static void rtc_set_time(RTCState *s)
>       tm->tm_sec = rtc_from_bcd(s, s->cmos_data[RTC_SECONDS]);
>       tm->tm_min = rtc_from_bcd(s, s->cmos_data[RTC_MINUTES]);
>       tm->tm_hour = rtc_from_bcd(s, s->cmos_data[RTC_HOURS]&  0x7f);
> -    if (!(s->cmos_data[RTC_REG_B]&  REG_B_24H)&&
> -        (s->cmos_data[RTC_HOURS]&  0x80)) {
> -        tm->tm_hour += 12;
> +    if (!(s->cmos_data[RTC_REG_B]&  REG_B_24H)) {
> +        tm->tm_hour %= 12;
> +        if (s->cmos_data[RTC_HOURS]&  0x80) {
> +            tm->tm_hour += 12;
> +        }
>       }
>       tm->tm_wday = rtc_from_bcd(s, s->cmos_data[RTC_DAY_OF_WEEK]) - 1;
>       tm->tm_mday = rtc_from_bcd(s, s->cmos_data[RTC_DAY_OF_MONTH]);
> @@ -320,7 +324,8 @@ static void rtc_copy_date(RTCState *s)
>           s->cmos_data[RTC_HOURS] = rtc_to_bcd(s, tm->tm_hour);
>       } else {
>           /* 12 hour format */
> -        s->cmos_data[RTC_HOURS] = rtc_to_bcd(s, tm->tm_hour % 12);
> +        int h = (tm->tm_hour % 12) ? tm->tm_hour % 12 : 12;
> +        s->cmos_data[RTC_HOURS] = rtc_to_bcd(s, h);
>           if (tm->tm_hour>= 12)
>               s->cmos_data[RTC_HOURS] |= 0x80;
>       }

On 11/22/2011 07:39 AM, Mark Wu wrote:
>> Hours in 12-hour mode are in the 1-12 range, not 0-11.
> Interesting. I would like to know how you could find this problem. It
> seems linux driver never changes the format  and 24-hour is default in
> rtc emulation code. So how did it expose and how to test it?

It is exposed by reading the datasheet. :)

Honestly, to test it I just added two for loops, I didn't test it on a 
real VM.  But you could do all these tests using a DOS program for example.

Paolo

On 11/21/2011 08:00 PM, Paolo Bonzini wrote:
> Hours in 12-hour mode are in the 1-12 range, not 0-11.
>
> @@ -320,7 +324,8 @@ static void rtc_copy_date(RTCState *s)
>          s->cmos_data[RTC_HOURS] = rtc_to_bcd(s, tm->tm_hour);
>      } else {
>          /* 12 hour format */
> -        s->cmos_data[RTC_HOURS] = rtc_to_bcd(s, tm->tm_hour % 12);
> +        int h = (tm->tm_hour % 12) ? tm->tm_hour % 12 : 12;
> +        s->cmos_data[RTC_HOURS] = rtc_to_bcd(s, h);
>          if (tm->tm_hour >= 12)
>              s->cmos_data[RTC_HOURS] |= 0x80;
>      }

Nitpick, don't update patch on this account:

I dislike seeing int-to-bool conversion on anything that is not a
true/false or count value.  Things like if (has_some_property) or if
(!nr_items) read well and are easily understood.  But here, you're not
checking for "are there any tm_hours, if yes, use them, if not, use
12".  You're testing against the value 0 which has a special encoding in
12 hour mode.

The is usually manifested in

   if (!strcmp(a, b)) ...

strcmp() does not return a bool or a count, and in fact it reads exactly
the opposite of the intent: "if not string compare".  strcmp() returns
an enumeration, or perhaps a mapping of string trichotomy to integer
trichotomy.

Sorry about the pontification, back to the regularly scheduled
whitespace discussion.

On 01/01/2012 03:53 PM, Avi Kivity wrote:
> On 11/21/2011 08:00 PM, Paolo Bonzini wrote:
>> Hours in 12-hour mode are in the 1-12 range, not 0-11.
>>
>> @@ -320,7 +324,8 @@ static void rtc_copy_date(RTCState *s)
>>           s->cmos_data[RTC_HOURS] = rtc_to_bcd(s, tm->tm_hour);
>>       } else {
>>           /* 12 hour format */
>> -        s->cmos_data[RTC_HOURS] = rtc_to_bcd(s, tm->tm_hour % 12);
>> +        int h = (tm->tm_hour % 12) ? tm->tm_hour % 12 : 12;
>> +        s->cmos_data[RTC_HOURS] = rtc_to_bcd(s, h);
>>           if (tm->tm_hour>= 12)
>>               s->cmos_data[RTC_HOURS] |= 0x80;
>>       }
>
> Nitpick, don't update patch on this account:
>
> I dislike seeing int-to-bool conversion on anything that is not a
> true/false or count value.  Things like if (has_some_property) or if
> (!nr_items) read well and are easily understood.  But here, you're not
> checking for "are there any tm_hours, if yes, use them, if not, use
> 12".  You're testing against the value 0 which has a special encoding in
> 12 hour mode.
>
> The is usually manifested in
>
>     if (!strcmp(a, b)) ...
>
> strcmp() does not return a bool or a count, and in fact it reads exactly
> the opposite of the intent: "if not string compare".  strcmp() returns
> an enumeration, or perhaps a mapping of string trichotomy to integer
> trichotomy.
>
> Sorry about the pontification, back to the regularly scheduled
> whitespace discussion.

Yeah, I probably would have remarked the same if it was someone else's 
patch. :)

BTW, I have kvm-unit-tests tests for these and other RTC emulation 
problems.  Long term perhaps they should become qtests, but for now 
kvm-unit-tests is the best place.  However, without these four patches 
they hang which is a bit too heavy.  So I'll submit as soon as these are in.

Paolo