| Message ID | 1360828461-2970-1-git-send-email-barsamin@gmail.com |
|---|---|
| State | New |
| Headers | show |
Il 14/02/2013 08:54, Antoine Mathys ha scritto: > 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 <barsamin@gmail.com> > --- Nice. Do you have a testcase? Please also test this patch with the two rtc-test patches at http://thread.gmane.org/gmane.comp.emulators.qemu/188244. Thanks, Paolo
On 02/14/2013 10:30 AM, Paolo Bonzini wrote: > Nice. Do you have a testcase? No, but the refactoring makes the code very easy to audit. > Please also test this patch with the two rtc-test patches at > http://thread.gmane.org/gmane.comp.emulators.qemu/188244. I did and the tests pass.
Il 14/02/2013 10:45, Antoine Mathys ha scritto: >> Nice. Do you have a testcase? > No, but the refactoring makes the code very easy to audit. I asked because I tried to reproduce the launchpad bug and failed. >> Please also test this patch with the two rtc-test patches at >> http://thread.gmane.org/gmane.comp.emulators.qemu/188244. > I did and the tests pass. Cool. Paolo
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 <barsamin@gmail.com> 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 > >
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)
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 <barsamin@gmail.com> --- hw/mc146818rtc.c | 163 +++++++++++++++++++++++++++++++++--------------------- 1 file changed, 99 insertions(+), 64 deletions(-)