Re: [Qemu-devel] [PATCH] hw/mc146818rtc.c: Fix reading and writing of time registers

[mdroth]

On Thu, Feb 14, 2013 at 08:54:20AM +0100, Antoine Mathys wrote:
> This patch consolidates the bit twidling involved in reading and
> writing the time registers to four functions that are used consistently.
> 
> This also has the effect of fixing bug 1090558.
> 
> Signed-off-by: Antoine Mathys <address@hidden>

This issue still seems to be present upstream and I'm looking at pulling
it in for stable. Does anyone want to apply this, or are we still waiting
on a test case for the bug it fixes?

> ---
>  hw/mc146818rtc.c |  163 
> +++++++++++++++++++++++++++++++++---------------------
>  1 file changed, 99 insertions(+), 64 deletions(-)
> 
> diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c
> index 2fb11f6..646cbd0 100644
> --- a/hw/mc146818rtc.c
> +++ b/hw/mc146818rtc.c
> @@ -86,8 +86,14 @@ typedef struct RTCState {
> 
>  static void rtc_set_time(RTCState *s);
>  static void rtc_update_time(RTCState *s);
> +
> +/* data encoding / decoding */
> +static inline uint8_t rtc_encode(RTCState *s, uint8_t a);
> +static inline int rtc_decode(RTCState *s, uint8_t a);
> +static inline uint8_t rtc_hour_encode(RTCState *s, uint8_t hour);
> +static inline int rtc_hour_decode(RTCState *s, uint8_t a);
> +
>  static void rtc_set_cmos(RTCState *s, const struct tm *tm);
> -static inline int rtc_from_bcd(RTCState *s, int a);
>  static uint64_t get_next_alarm(RTCState *s);
> 
>  static inline bool rtc_running(RTCState *s)
> @@ -254,17 +260,84 @@ static void check_update_timer(RTCState *s)
>      }
>  }
> 
> -static inline uint8_t convert_hour(RTCState *s, uint8_t hour)
> +/* data encoding / decoding */
> +
> +static inline uint8_t rtc_encode(RTCState *s, uint8_t a)
> +{
> +    if (s->cmos_data[RTC_REG_B] & REG_B_DM) {
> +        return a;
> +    } else {
> +        return to_bcd(a);
> +    }
> +}
> +
> +static inline int rtc_decode(RTCState *s, uint8_t a)
> +{
> +    if ((a & 0xc0) == 0xc0) {
> +        return -1;
> +    }
> +    if (s->cmos_data[RTC_REG_B] & REG_B_DM) {
> +        return a;
> +    } else {
> +        return from_bcd(a);
> +    }
> +}
> +
> +/*
> +  Note: The next two functions implement the following mapping between
> +  the 12 hour and 24 hour formats:
> +
> +  0        <->    12     AM
> +  1-11     <->    1 - 11 AM
> +  12       <->    12     PM
> +  13-23    <->    1 - 11 PM
> +*/
> +
> +static inline uint8_t rtc_hour_encode(RTCState *s, uint8_t hour)
> +{
> +    uint8_t tmp;
> +
> +    if (s->cmos_data[RTC_REG_B] & REG_B_24H) {
> +        /* 24 hour format */
> +        tmp = rtc_encode(s, hour);
> +    } else {
> +        /* 12 hour format */
> +        uint8_t h = (hour % 12) ? (hour % 12) : 12;
> +        tmp = rtc_encode(s, h);
> +        if (hour >= 12) {
> +            tmp |= 0x80;
> +        }
> +    }
> +    return tmp;
> +}
> +
> +static inline int rtc_hour_decode(RTCState *s, uint8_t a)
>  {
> -    if (!(s->cmos_data[RTC_REG_B] & REG_B_24H)) {
> +    uint8_t hour;
> +
> +    /* Note: in 12 hour mode we clear bit 7 before calling
> +       rtc_decode(), hence we cannot rely on the later to check the
> +       don't care condition. While we could skip this check in 24 hour
> +       mode it is simpler to do it in any case. */
> +    if ((a & 0xc0) == 0xc0) {
> +        return -1;
> +    }
> +
> +    if (s->cmos_data[RTC_REG_B] & REG_B_24H) {
> +        /* 24 hour format */
> +        hour = rtc_decode(s, a);
> +    } else {
> +        /* 12 hour format */
> +        hour = rtc_decode(s, a & 0x7f);
>          hour %= 12;
> -        if (s->cmos_data[RTC_HOURS] & 0x80) {
> +        if (a & 0x80) {
>              hour += 12;
>          }
>      }
>      return hour;
>  }
> 
> +
>  static uint64_t get_next_alarm(RTCState *s)
>  {
>      int32_t alarm_sec, alarm_min, alarm_hour, cur_hour, cur_min, cur_sec;
> @@ -272,15 +345,13 @@ static uint64_t get_next_alarm(RTCState *s)
> 
>      rtc_update_time(s);
> 
> -    alarm_sec = rtc_from_bcd(s, s->cmos_data[RTC_SECONDS_ALARM]);
> -    alarm_min = rtc_from_bcd(s, s->cmos_data[RTC_MINUTES_ALARM]);
> -    alarm_hour = rtc_from_bcd(s, s->cmos_data[RTC_HOURS_ALARM]);
> -    alarm_hour = alarm_hour == -1 ? -1 : convert_hour(s, alarm_hour);
> +    alarm_sec = rtc_decode(s, s->cmos_data[RTC_SECONDS_ALARM]);
> +    alarm_min = rtc_decode(s, s->cmos_data[RTC_MINUTES_ALARM]);
> +    alarm_hour = rtc_hour_decode(s, s->cmos_data[RTC_HOURS_ALARM]);
> 
> -    cur_sec = rtc_from_bcd(s, s->cmos_data[RTC_SECONDS]);
> -    cur_min = rtc_from_bcd(s, s->cmos_data[RTC_MINUTES]);
> -    cur_hour = rtc_from_bcd(s, s->cmos_data[RTC_HOURS]);
> -    cur_hour = convert_hour(s, cur_hour);
> +    cur_sec = rtc_decode(s, s->cmos_data[RTC_SECONDS]);
> +    cur_min = rtc_decode(s, s->cmos_data[RTC_MINUTES]);
> +    cur_hour = rtc_hour_decode(s, s->cmos_data[RTC_HOURS]);
> 
>      if (alarm_hour == -1) {
>          alarm_hour = cur_hour;
> @@ -486,44 +557,17 @@ static void cmos_ioport_write(void *opaque, hwaddr addr,
>      }
>  }
> 
> -static inline int rtc_to_bcd(RTCState *s, int a)
> -{
> -    if (s->cmos_data[RTC_REG_B] & REG_B_DM) {
> -        return a;
> -    } else {
> -        return ((a / 10) << 4) | (a % 10);
> -    }
> -}
> -
> -static inline int rtc_from_bcd(RTCState *s, int a)
> -{
> -    if ((a & 0xc0) == 0xc0) {
> -        return -1;
> -    }
> -    if (s->cmos_data[RTC_REG_B] & REG_B_DM) {
> -        return a;
> -    } else {
> -        return ((a >> 4) * 10) + (a & 0x0f);
> -    }
> -}
> -
>  static void rtc_get_time(RTCState *s, struct tm *tm)
>  {
> -    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)) {
> -        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]);
> -    tm->tm_mon = rtc_from_bcd(s, s->cmos_data[RTC_MONTH]) - 1;
> +    tm->tm_sec = rtc_decode(s, s->cmos_data[RTC_SECONDS]);
> +    tm->tm_min = rtc_decode(s, s->cmos_data[RTC_MINUTES]);
> +    tm->tm_hour = rtc_hour_decode(s, s->cmos_data[RTC_HOURS]);
> +    tm->tm_wday = rtc_decode(s, s->cmos_data[RTC_DAY_OF_WEEK]) - 1;
> +    tm->tm_mday = rtc_decode(s, s->cmos_data[RTC_DAY_OF_MONTH]);
> +    tm->tm_mon = rtc_decode(s, s->cmos_data[RTC_MONTH]) - 1;
>      tm->tm_year =
> -        rtc_from_bcd(s, s->cmos_data[RTC_YEAR]) + s->base_year +
> -        rtc_from_bcd(s, s->cmos_data[RTC_CENTURY]) * 100 - 1900;
> +        rtc_decode(s, s->cmos_data[RTC_YEAR]) + s->base_year +
> +        rtc_decode(s, s->cmos_data[RTC_CENTURY]) * 100 - 1900;
>  }
> 
>  static void rtc_set_time(RTCState *s)
> @@ -541,24 +585,15 @@ static void rtc_set_cmos(RTCState *s, const struct tm 
> *tm)
>  {
>      int year;
> 
> -    s->cmos_data[RTC_SECONDS] = rtc_to_bcd(s, tm->tm_sec);
> -    s->cmos_data[RTC_MINUTES] = rtc_to_bcd(s, tm->tm_min);
> -    if (s->cmos_data[RTC_REG_B] & REG_B_24H) {
> -        /* 24 hour format */
> -        s->cmos_data[RTC_HOURS] = rtc_to_bcd(s, tm->tm_hour);
> -    } else {
> -        /* 12 hour format */
> -        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;
> -    }
> -    s->cmos_data[RTC_DAY_OF_WEEK] = rtc_to_bcd(s, tm->tm_wday + 1);
> -    s->cmos_data[RTC_DAY_OF_MONTH] = rtc_to_bcd(s, tm->tm_mday);
> -    s->cmos_data[RTC_MONTH] = rtc_to_bcd(s, tm->tm_mon + 1);
> +    s->cmos_data[RTC_SECONDS] = rtc_encode(s, tm->tm_sec);
> +    s->cmos_data[RTC_MINUTES] = rtc_encode(s, tm->tm_min);
> +    s->cmos_data[RTC_HOURS] = rtc_hour_encode(s, tm->tm_hour);
> +    s->cmos_data[RTC_DAY_OF_WEEK] = rtc_encode(s, tm->tm_wday + 1);
> +    s->cmos_data[RTC_DAY_OF_MONTH] = rtc_encode(s, tm->tm_mday);
> +    s->cmos_data[RTC_MONTH] = rtc_encode(s, tm->tm_mon + 1);
>      year = tm->tm_year + 1900 - s->base_year;
> -    s->cmos_data[RTC_YEAR] = rtc_to_bcd(s, year % 100);
> -    s->cmos_data[RTC_CENTURY] = rtc_to_bcd(s, year / 100);
> +    s->cmos_data[RTC_YEAR] = rtc_encode(s, year % 100);
> +    s->cmos_data[RTC_CENTURY] = rtc_encode(s, year / 100);
>  }
> 
>  static void rtc_update_time(RTCState *s)
> -- 
> 1.7.10.4
> 
>