| Message ID | A9667DDFB95DB7438FA9D7D576C3D87E018CDB@SHSMSX101.ccr.corp.intel.com |
|---|---|
| State | New |
| Headers | show |
On 2012-01-06 05:37, Zhang, Yang Z wrote: > change the RTC update logic to use host time with offset to calculate RTC clock. > There have no need to use two periodic timers to maintain an internal timer for RTC clock update and alarm check. Instead, we calculate the real RTC time by the host time with an offset. For alarm and updated-end interrupt, if guest enabled it, then we setup a timer, or else, stop it. > > Signed-off-by: Yang Zhang <yang.z.zhang@intel.com> I appreciate this effort! However, not having looked at details yet, two things jumped at me: - You cannot simply change the vmstate format without caring about migration from older qemu versions. Ideally, if possible the existing format is kept and translated to new internal representations on save/load. - Please respect the coding style. It's documented, and we also have a checker. Thanks, Jab
> -----Original Message----- > From: Jan Kiszka [mailto:jan.kiszka@web.de] > Sent: Saturday, January 07, 2012 1:27 AM > However, not having looked at details yet, two things jumped at me: > - You cannot simply change the vmstate format without caring about > migration from older qemu versions. Ideally, if possible the existing > format is kept and translated to new internal representations on > save/load. Good point! > - Please respect the coding style. It's documented, and we also have a > checker. You are right. I use the wrong vi setting. best regards yang
On 01/06/2012 08:37 AM, Zhang, Yang Z wrote: > VMSTATE_BUFFER(cmos_data, RTCState), > VMSTATE_UINT8(cmos_index, RTCState), > - VMSTATE_INT32(current_tm.tm_sec, RTCState), > - VMSTATE_INT32(current_tm.tm_min, RTCState), > - VMSTATE_INT32(current_tm.tm_hour, RTCState), > - VMSTATE_INT32(current_tm.tm_wday, RTCState), > - VMSTATE_INT32(current_tm.tm_mday, RTCState), > - VMSTATE_INT32(current_tm.tm_mon, RTCState), > - VMSTATE_INT32(current_tm.tm_year, RTCState), > + VMSTATE_INT64(offset, RTCState), > VMSTATE_TIMER(periodic_timer, RTCState), > VMSTATE_INT64(next_periodic_time, RTCState), > - VMSTATE_INT64(next_second_time, RTCState), > - VMSTATE_TIMER(second_timer, RTCState), > - VMSTATE_TIMER(second_timer2, RTCState), > + VMSTATE_TIMER(update_timer, RTCState), > + VMSTATE_INT64(next_update_time, RTCState), > + VMSTATE_TIMER(alarm_timer, RTCState), > + VMSTATE_INT64(next_alarm_time, RTCState), Hi Yang, this looks like a very nice piece of work, but this unfortunately breaks migration. Also, I'm not sure if the update in progress flag still works. Clients are supposed to wait for UIP=0 before reading the RTC, and an update is supposed to be at least 220 microseconds away when UIP=0. Also, it would be nice if you could based these patches on the 4-patch series I sent recently that fixes some bugs with interrupts and 12-hour emulation. There is another aspect of RTC emulation that is missing in the current code; after setting the clock, the next second tick will occur in exactly 500 ms. I have patches to fix this, but it looks like it could be incorporated in your series, too. Paolo
> -----Original Message----- > From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo > Bonzini > Sent: Monday, January 09, 2012 4:19 PM > To: Zhang, Yang Z > Cc: qemu-devel@nongnu.org; avi@redhat.com; aliguori@us.ibm.com; Zhang, > Xiantao; Shan, Haitao; kvm@vger.kernel.org > Subject: Re: [PATCH 3/3] stop the periodic RTC update timer > > On 01/06/2012 08:37 AM, Zhang, Yang Z wrote: > > VMSTATE_BUFFER(cmos_data, RTCState), > > VMSTATE_UINT8(cmos_index, RTCState), > > - VMSTATE_INT32(current_tm.tm_sec, RTCState), > > - VMSTATE_INT32(current_tm.tm_min, RTCState), > > - VMSTATE_INT32(current_tm.tm_hour, RTCState), > > - VMSTATE_INT32(current_tm.tm_wday, RTCState), > > - VMSTATE_INT32(current_tm.tm_mday, RTCState), > > - VMSTATE_INT32(current_tm.tm_mon, RTCState), > > - VMSTATE_INT32(current_tm.tm_year, RTCState), > > + VMSTATE_INT64(offset, RTCState), > > VMSTATE_TIMER(periodic_timer, RTCState), > > VMSTATE_INT64(next_periodic_time, RTCState), > > - VMSTATE_INT64(next_second_time, RTCState), > > - VMSTATE_TIMER(second_timer, RTCState), > > - VMSTATE_TIMER(second_timer2, RTCState), > > + VMSTATE_TIMER(update_timer, RTCState), > > + VMSTATE_INT64(next_update_time, RTCState), > > + VMSTATE_TIMER(alarm_timer, RTCState), > > + VMSTATE_INT64(next_alarm_time, RTCState), > > Hi Yang, > > this looks like a very nice piece of work, but this unfortunately breaks migration. You are right. In next version, I will add the qemu version check. > Also, I'm not sure if the update in progress flag still works. Clients are supposed > to wait for UIP=0 before reading the RTC, and an update is supposed to be at > least 220 microseconds away when UIP=0. Hardware need a period time to update clock and it would not provide the right value during the update. So it uses UIP to notify the software doesn't believe the value if the UIP is set. For emulation, you can read RTC at any time and it always gives you the right value. So there is no need to emulate UIP. > Also, it would be nice if you could based these patches on the 4-patch series I > sent recently that fixes some bugs with interrupts and 12-hour emulation. When it will be check in? > There is another aspect of RTC emulation that is missing in the current code; after > setting the clock, the next second tick will occur in exactly 500 ms. I have > patches to fix this, but it looks like it could be incorporated in your series, too. I do not quite understand it. What does "setting the clock" mean? And what is the next second tick? > Paolo
On 01/10/2012 07:37 AM, Zhang, Yang Z wrote: >> Also, I'm not sure if the update in progress flag still works. >> Clients are supposed to wait for UIP=0 before reading the RTC, >> and an update is supposed to be at least 220 microseconds away >> when UIP=0. > > Hardware need a period time to update clock and it would not provide > the right value during the update. So it uses UIP to notify the > software doesn't believe the value if the UIP is set. For emulation, > you can read RTC at any time and it always gives you the right value. > So there is no need to emulate UIP. This is incorrect, for two reasons. First, the UIP is in the spec, and we have to implement it. Second, reading the clock is not atomic, and waiting for UIP=0 gives you 220 microseconds during which you know that the read will appear atomic. >> Also, it would be nice if you could based these patches on the >> 4-patch series I sent recently that fixes some bugs with >> interrupts and 12-hour emulation. > > When it will be check in? I don't know. :) >> There is another aspect of RTC emulation that is missing in the >> current code; after setting the clock, the next second tick will >> occur in exactly 500 ms. I have patches to fix this, but it >> looks like it could be incorporated in your series, too. > > I do not quite understand it. What does "setting the clock" mean? And > what is the next second tick? It means that the (not externally visible) millisecond value is set to 500 when you modify the current time of the RTC. The next update of the clock will happen exactly 500 ms after you reset bit 7 of register B. Paolo
> -----Original Message----- > From: Paolo Bonzini [mailto:pbonzini@redhat.com] > Sent: Tuesday, January 10, 2012 5:25 PM > >> Also, I'm not sure if the update in progress flag still works. > >> Clients are supposed to wait for UIP=0 before reading the RTC, and an > >> update is supposed to be at least 220 microseconds away when UIP=0. > > > > Hardware need a period time to update clock and it would not provide > > the right value during the update. So it uses UIP to notify the > > software doesn't believe the value if the UIP is set. For emulation, > > you can read RTC at any time and it always gives you the right value. > > So there is no need to emulate UIP. > > This is incorrect, for two reasons. First, the UIP is in the spec, and we have to > implement it. Second, reading the clock is not atomic, and waiting for UIP=0 > gives you 220 microseconds during which you know that the read will appear > atomic. For a simulator, we need to follow the spec strictly and simulate hardware as precisely as possible. But QEMU is a generic machine emulator and virtualizer. It's not a hardware simulator. If there is an easy way we can provide the same function, why we chose the complicated one? Also, is there an actual case that break with my patch? > >> Also, it would be nice if you could based these patches on the > >> 4-patch series I sent recently that fixes some bugs with interrupts > >> and 12-hour emulation. > > > > When it will be check in? > > I don't know. :) > > >> There is another aspect of RTC emulation that is missing in the > >> current code; after setting the clock, the next second tick will > >> occur in exactly 500 ms. I have patches to fix this, but it looks > >> like it could be incorporated in your series, too. > > > > I do not quite understand it. What does "setting the clock" mean? And > > what is the next second tick? > > It means that the (not externally visible) millisecond value is set to > 500 when you modify the current time of the RTC. The next update of the clock > will happen exactly 500 ms after you reset bit 7 of register B. Same question, any reason need to complicate the current logic? Or any actual usage model need to add this? best regards yang
On 01/11/2012 01:56 AM, Zhang, Yang Z wrote: >>>> Clients are supposed to wait for UIP=0 before reading the RTC, >>>> and an update is supposed to be at least 220 microseconds away >>>> when UIP=0. >>> >>> Hardware need a period time to update clock and it would not >>> provide the right value during the update. So it uses UIP to >>> notify the software doesn't believe the value if the UIP is set. >>> For emulation, you can read RTC at any time and it always gives >>> you the right value. So there is no need to emulate UIP. >> >> This is incorrect, for two reasons. First, the UIP is in the spec, >> and we have to implement it. Second, reading the clock is not >> atomic, and waiting for UIP=0 gives you 220 microseconds during >> which you know that the read will appear atomic. > > For a simulator, we need to follow the spec strictly and simulate > hardware as precisely as possible. But QEMU is a generic machine > emulator and virtualizer. It's not a hardware simulator. If there is > an easy way we can provide the same function, why we chose the > complicated one? Because it's not in the spec because some engineer thought it was cool. It's in the spec because it gives you a way to do atomic reads. QEMU not being a simulator means that we always assume that the RTC is programmed for a 32768 Hz clock, for example, because any other setting would not make sense on a PC. We can use a 1-second (or higher, as in your patches) timer, rather than a 32768 Hz timer which anyway would not work well. So we're taking shortcuts, but each of them must be evaluated separately, and _this_ shortcut is not acceptable. > Also, is there an actual case that break with my patch? Any decent unit test for the RTC would break. >> It means that the (not externally visible) millisecond value is set >> to 500 when you modify the current time of the RTC. The next >> update of the clock will happen exactly 500 ms after you reset bit >> 7 of register B. > > Same question, any reason need to complicate the current logic? Or > any actual usage model need to add this? Is it really so difficult to implement? Note that this case is mentioned in drivers/rtc/rtc-cmos.c in the Linux source code, even though it is not used. Paolo
Hello, On Wednesday 11 January 2012 01:56:25 Zhang, Yang Z wrote: > > -----Original Message----- > > From: Paolo Bonzini [mailto:pbonzini@redhat.com] > > Sent: Tuesday, January 10, 2012 5:25 PM > > > > >> Also, I'm not sure if the update in progress flag still works. > > >> Clients are supposed to wait for UIP=0 before reading the RTC, and an > > >> update is supposed to be at least 220 microseconds away when UIP=0. > > > > > > Hardware need a period time to update clock and it would not provide > > > the right value during the update. So it uses UIP to notify the > > > software doesn't believe the value if the UIP is set. For emulation, > > > you can read RTC at any time and it always gives you the right value. > > > So there is no need to emulate UIP. > > > > This is incorrect, for two reasons. First, the UIP is in the spec, and > > we have to implement it. Second, reading the clock is not atomic, and > > waiting for UIP=0 gives you 220 microseconds during which you know that > > the read will appear atomic. > > For a simulator, we need to follow the spec strictly and simulate hardware > as precisely as possible. But QEMU is a generic machine emulator and > virtualizer. It's not a hardware simulator. If there is an easy way we can > provide the same function, why we chose the complicated one? Also, is there > an actual case that break with my patch? FYI: But you must not break existing implementations (of any (closed-source) OS), which depend on that behaviour of the RTC. Have a look at get_cmos_time() of the Xen hypervisor for example (that is the one I have been looking at at the past few hours), which explicitliy waits for the falling edge of UIP to get sub-second precision. This would break if you no longer simulate UIP. Sincerely Philipp
On Fri, Jan 06, 2012 at 07:37:31AM +0000, Zhang, Yang Z wrote: > change the RTC update logic to use host time with offset to calculate RTC clock. > There have no need to use two periodic timers to maintain an internal timer for RTC clock update and alarm check. Instead, we calculate the real RTC time by the host time with an offset. For alarm and updated-end interrupt, if guest enabled it, then we setup a timer, or else, stop it. > > Signed-off-by: Yang Zhang <yang.z.zhang@intel.com> > > diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c > index 9cbd052..ac1854e 100644 > --- a/hw/mc146818rtc.c > +++ b/hw/mc146818rtc.c > @@ -84,7 +84,7 @@ typedef struct RTCState { > MemoryRegion io; > uint8_t cmos_data[128]; > uint8_t cmos_index; > - struct tm current_tm; > + int64_t offset; > int32_t base_year; > qemu_irq irq; > qemu_irq sqw_irq; > @@ -93,19 +93,18 @@ typedef struct RTCState { > QEMUTimer *periodic_timer; > int64_t next_periodic_time; > /* second update */ > - int64_t next_second_time; > + QEMUTimer *update_timer; > + int64_t next_update_time; > + /* alarm */ > + QEMUTimer *alarm_timer; > + int64_t next_alarm_time; > uint16_t irq_reinject_on_ack_count; > uint32_t irq_coalesced; > uint32_t period; > QEMUTimer *coalesced_timer; > - QEMUTimer *second_timer; > - QEMUTimer *second_timer2; > Notifier clock_reset_notifier; > } RTCState; > > -static void rtc_set_time(RTCState *s); > -static void rtc_copy_date(RTCState *s); > - > #ifdef TARGET_I386 > static void rtc_coalesced_timer_update(RTCState *s) > { > @@ -140,6 +139,72 @@ static void rtc_coalesced_timer(void *opaque) > } > #endif > > +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 (s->cmos_data[RTC_REG_B] & REG_B_DM) { > + return a; > + } else { > + return ((a >> 4) * 10) + (a & 0x0f); > + } > +} > + > +static void rtc_set_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) && > + (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_year = rtc_from_bcd(s, s->cmos_data[RTC_YEAR]) + s->base_year - 1900; > + > + s->offset = qemu_timedate_diff(&tm); > + > + rtc_change_mon_event(&tm); > +} > + > +static void rtc_update_time(RTCState *s) > +{ > + struct tm tm; > + int year; > + > + qemu_get_timedate(&tm, s->offset); > + > + 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 */ > + s->cmos_data[RTC_HOURS] = rtc_to_bcd(s, tm.tm_hour % 12); > + 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); > + year = (tm.tm_year - s->base_year) % 100; > + if (year < 0) > + year += 100; > + s->cmos_data[RTC_YEAR] = rtc_to_bcd(s, year); > +} > + Please have this code move in a separate, earlier patch. > static void rtc_timer_update(RTCState *s, int64_t current_time) > { > int period_code, period; > @@ -174,7 +239,7 @@ static void rtc_timer_update(RTCState *s, int64_t current_time) > } > } > > -static void rtc_periodic_timer(void *opaque) > +static void rtc_periodic_interrupt(void *opaque) > { > RTCState *s = opaque; > > @@ -204,6 +269,92 @@ static void rtc_periodic_timer(void *opaque) > } > } > > +static void rtc_enable_update_interrupt(void *opaque) > +{ > + RTCState *s = opaque; > + > + s->next_update_time = qemu_get_clock_ns(rtc_clock) + get_ticks_per_sec(); > + qemu_mod_timer(s->update_timer, s->next_update_time); > +} > + > +static void rtc_disable_update_interrupt(void *opaque) > +{ > + RTCState *s = opaque; > + > + qemu_del_timer(s->update_timer); > +} > + > +static void rtc_update_interrupt(void *opaque) > +{ > + RTCState *s = opaque; > + > + /* update ended interrupt */ > + s->cmos_data[RTC_REG_C] |= REG_C_UF; > + if (s->cmos_data[RTC_REG_B] & REG_B_UIE) { > + s->cmos_data[RTC_REG_C] |= REG_C_IRQF; > + qemu_irq_raise(s->irq); > + > + s->next_update_time += get_ticks_per_sec(); > + qemu_mod_timer(s->update_timer, s->next_update_time); > + } else > + rtc_disable_update_interrupt(s); > +} > + > +static void rtc_enable_alarm(void *opaque) > +{ > + RTCState *s = opaque; > + > + s->next_alarm_time = qemu_get_clock_ns(rtc_clock) + get_ticks_per_sec(); > + s->next_alarm_time /= get_ticks_per_sec(); > + s->next_alarm_time *= get_ticks_per_sec(); > + > + qemu_mod_timer(s->alarm_timer, s->next_alarm_time); > +} > + > +static void rtc_disable_alarm(void *opaque) > +{ > + RTCState *s = opaque; > + > + qemu_del_timer(s->alarm_timer); > +} > + > +static void rtc_alarm_interrupt(void *opaque) > +{ > + RTCState *s = opaque; > + struct tm tm; > + int hour = 0; > + > + qemu_get_timedate(&tm, s->offset); > + > + if ((s->cmos_data[RTC_HOURS_ALARM] & 0xc0) != 0xc0) { > + hour = rtc_from_bcd(s, s->cmos_data[RTC_HOURS_ALARM] & 0x7f); > + if (!(s->cmos_data[RTC_REG_B] & REG_B_24H) && > + (s->cmos_data[RTC_HOURS_ALARM] & 0x80)) { > + hour += 12; > + } > + } > + > + /* check alarm */ > + if (s->cmos_data[RTC_REG_B] & REG_B_AIE) { > + if (((s->cmos_data[RTC_SECONDS_ALARM] & 0xc0) == 0xc0 || > + rtc_from_bcd(s, s->cmos_data[RTC_SECONDS_ALARM]) == tm.tm_sec) && > + ((s->cmos_data[RTC_MINUTES_ALARM] & 0xc0) == 0xc0 || > + rtc_from_bcd(s, s->cmos_data[RTC_MINUTES_ALARM]) == tm.tm_min) && > + ((s->cmos_data[RTC_HOURS_ALARM] & 0xc0) == 0xc0 || > + hour == tm.tm_hour)) { > + > + printf("raise irq\n"); printf. > + s->cmos_data[RTC_REG_C] |= 0xa0; > + qemu_irq_raise(s->irq); > + } > + > + s->next_alarm_time += get_ticks_per_sec(); > + qemu_mod_timer(s->alarm_timer, s->next_alarm_time); > + } else > + rtc_disable_alarm(s); > + > +} > + > static void cmos_ioport_write(void *opaque, uint32_t addr, uint32_t data) > { > RTCState *s = opaque; > @@ -239,26 +390,37 @@ static void cmos_ioport_write(void *opaque, uint32_t addr, uint32_t data) > rtc_timer_update(s, qemu_get_clock_ns(rtc_clock)); > break; > case RTC_REG_B: > - if (data & REG_B_SET) { > - /* set mode: reset UIP mode */ > - s->cmos_data[RTC_REG_A] &= ~REG_A_UIP; > - data &= ~REG_B_UIE; > - } else { > + if (data & REG_B_SET) > + rtc_update_time(s); > + else { > /* if disabling set mode, update the time */ > if (s->cmos_data[RTC_REG_B] & REG_B_SET) { > rtc_set_time(s); > } > } > + > if (((s->cmos_data[RTC_REG_B] ^ data) & (REG_B_DM | REG_B_24H)) && > !(data & REG_B_SET)) { > /* If the time format has changed and not in set mode, > update the registers immediately. */ > s->cmos_data[RTC_REG_B] = data; > - rtc_copy_date(s); > - } else { > + rtc_update_time(s); > + } else > s->cmos_data[RTC_REG_B] = data; > - } + > + /* check alarm interrupt */ > + if ((s->cmos_data[RTC_REG_B] & REG_B_AIE) && > + !(s->cmos_data[RTC_REG_B] & REG_B_SET) ) > + rtc_enable_alarm(s); > + > + /* check update-ended interrupt */ > + if ((s->cmos_data[RTC_REG_B] & REG_B_UIE) && > + !(s->cmos_data[RTC_REG_B] & REG_B_SET)) > + rtc_enable_update_interrupt(s); > + > + /* check periodic interrupt or square-wave */ > rtc_timer_update(s, qemu_get_clock_ns(rtc_clock)); > + Regarding the UIP bit, a guest could read it in a loop and wait for the value to change. But you can emulate it in cmos_ioport_read by reading the host time, that is, return 1 during 244us, 0 for remaining of the second, and have that in sync with update-cycle-ended interrupt if its enabled.
> -----Original Message----- > From: Marcelo Tosatti [mailto:mtosatti@redhat.com] > > Regarding the UIP bit, a guest could read it in a loop and wait for the value to > change. But you can emulate it in cmos_ioport_read by reading the host time, > that is, return 1 during 244us, 0 for remaining of the second, and have that in sync > with update-cycle-ended interrupt if its enabled. Yes. Guest may use the loop to read RTC, but the point is the guest is waiting for the UIP changed to 0. If this bit always equal to 0 , guest will never go into the loop. For real RTC, this may wrong, because the RTC cannot give you the valid value during the update cycle. But the virtual RTC doesn't' need this logic, whenever you read it, it will always return the right value to you. best regards yang
> -----Original Message----- > From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo > Bonzini > Because it's not in the spec because some engineer thought it was cool. It not cool. We need to do some optimizations to get Better Performance. > It's in the spec because it gives you a way to do atomic reads. > > QEMU not being a simulator means that we always assume that the RTC is > programmed for a 32768 Hz clock, for example, because any other setting would > not make sense on a PC. We can use a 1-second (or higher, as in your patches) > timer, rather than a 32768 Hz timer which anyway would not work well. > > So we're taking shortcuts, but each of them must be evaluated separately, and > _this_ shortcut is not acceptable. > > > Also, is there an actual case that break with my patch? > > Any decent unit test for the RTC would break. Any decent unit test break the following logic too. The spec provide three ways for you to program, why we only focus on 0x20? Because this is for emulation not for hardware simulation. Because no real OS set it to other value. static void rtc_update_second(void *opaque) { RTCState *s = opaque; int64_t delay; /* if the oscillator is not in normal operation, we do not update */ if ((s->cmos_data[RTC_REG_A] & 0x70) != 0x20) { ..... } > >> It means that the (not externally visible) millisecond value is set > >> to 500 when you modify the current time of the RTC. The next update > >> of the clock will happen exactly 500 ms after you reset bit > >> 7 of register B. > > > > Same question, any reason need to complicate the current logic? Or any > > actual usage model need to add this? > > Is it really so difficult to implement? I think what we are talking is do we really need it? Not how difficult to add it. > Note that this case is mentioned in drivers/rtc/rtc-cmos.c in the Linux source > code, even though it is not used. Yes, it just mentioned the next update will happen in 500ms later. What's wrong with this? The highest resolution of RTC is 1 second, if any software intend to use RTC do some check within 1 second, it should be wrong. Anyway, I agree with your point. If we really need to add those features, I will add it in next version. Before it, we need figure out whether it is necessary. Best regards yang
On 01/12/2012 01:00 AM, Zhang, Yang Z wrote: >> Regarding the UIP bit, a guest could read it in a loop and wait >> for the value to change. But you can emulate it in >> cmos_ioport_read by reading the host time, that is, return 1 >> during 244us, 0 for remaining of the second, and have that in >> sync with update-cycle-ended interrupt if its enabled. > > Yes. Guest may use the loop to read RTC, but the point is the guest > is waiting for the UIP changed to 0. If this bit always equal to 0 , > guest will never go into the loop. For real RTC, this may wrong, > because the RTC cannot give you the valid value during the update > cycle. But the virtual RTC doesn't' need this logic, whenever you > read it, it will always return the right value to you. The point is not _correctness_. It is _atomicity_. Paolo
On Thu, Jan 12, 2012 at 12:00:06AM +0000, Zhang, Yang Z wrote: > > -----Original Message----- > > From: Marcelo Tosatti [mailto:mtosatti@redhat.com] > > > > Regarding the UIP bit, a guest could read it in a loop and wait for the value to > > change. But you can emulate it in cmos_ioport_read by reading the host time, > > that is, return 1 during 244us, 0 for remaining of the second, and have that in sync > > with update-cycle-ended interrupt if its enabled. > Yes. Guest may use the loop to read RTC, but the point is the guest is waiting for the UIP changed to 0. If this bit always equal to 0 , guest will never go into the loop. For real RTC, this may wrong, because the RTC cannot give you the valid value during the update cycle. But the virtual RTC doesn't' need this logic, whenever you read it, it will always return the right value to you. Can't it wait a change from 0 to 1?
On Thu, Jan 12, 2012 at 07:59:06AM -0200, Marcelo Tosatti wrote: > On Thu, Jan 12, 2012 at 12:00:06AM +0000, Zhang, Yang Z wrote: > > > -----Original Message----- > > > From: Marcelo Tosatti [mailto:mtosatti@redhat.com] > > > > > > Regarding the UIP bit, a guest could read it in a loop and wait for the value to > > > change. But you can emulate it in cmos_ioport_read by reading the host time, > > > that is, return 1 during 244us, 0 for remaining of the second, and have that in sync > > > with update-cycle-ended interrupt if its enabled. > > Yes. Guest may use the loop to read RTC, but the point is the guest is waiting for the UIP changed to 0. If this bit always equal to 0 , guest will never go into the loop. For real RTC, this may wrong, because the RTC cannot give you the valid value during the update cycle. But the virtual RTC doesn't' need this logic, whenever you read it, it will always return the right value to you. > > Can't it wait a change from 0 to 1? The point is the guest can use the hardware as it pleases, not only as is suggested in the hardware documentation.
> -----Original Message----- > From: Marcelo Tosatti [mailto:mtosatti@redhat.com] > Sent: Thursday, January 12, 2012 6:03 PM > To: Zhang, Yang Z > Cc: qemu-devel@nongnu.org; avi@redhat.com; aliguori@us.ibm.com; Zhang, > Xiantao; Shan, Haitao; kvm@vger.kernel.org > Subject: Re: [PATCH 3/3] stop the periodic RTC update timer > > On Thu, Jan 12, 2012 at 07:59:06AM -0200, Marcelo Tosatti wrote: > > On Thu, Jan 12, 2012 at 12:00:06AM +0000, Zhang, Yang Z wrote: > > > > -----Original Message----- > > > > From: Marcelo Tosatti [mailto:mtosatti@redhat.com] > > > > > > > > Regarding the UIP bit, a guest could read it in a loop and wait > > > > for the value to change. But you can emulate it in > > > > cmos_ioport_read by reading the host time, that is, return 1 > > > > during 244us, 0 for remaining of the second, and have that in sync with > update-cycle-ended interrupt if its enabled. > > > Yes. Guest may use the loop to read RTC, but the point is the guest is waiting > for the UIP changed to 0. If this bit always equal to 0 , guest will never go into the > loop. For real RTC, this may wrong, because the RTC cannot give you the valid > value during the update cycle. But the virtual RTC doesn't' need this logic, > whenever you read it, it will always return the right value to you. > > > > Can't it wait a change from 0 to 1? > > The point is the guest can use the hardware as it pleases, not only as is suggested > in the hardware documentation. You are right. I will add it to next version. best regards yang
On Thu, Jan 12, 2012 at 10:26:17AM +0100, Paolo Bonzini wrote: > On 01/12/2012 01:00 AM, Zhang, Yang Z wrote: > >>Regarding the UIP bit, a guest could read it in a loop and wait > >>for the value to change. But you can emulate it in > >>cmos_ioport_read by reading the host time, that is, return 1 > >>during 244us, 0 for remaining of the second, and have that in > >>sync with update-cycle-ended interrupt if its enabled. > > > >Yes. Guest may use the loop to read RTC, but the point is the guest > >is waiting for the UIP changed to 0. If this bit always equal to 0 , > >guest will never go into the loop. For real RTC, this may wrong, > >because the RTC cannot give you the valid value during the update > >cycle. But the virtual RTC doesn't' need this logic, whenever you > >read it, it will always return the right value to you. > > The point is not _correctness_. It is _atomicity_. Quoting you earlier "This is incorrect, for two reasons. First, the UIP is in the spec, and we have to implement it. Second, reading the clock is not atomic, and waiting for UIP=0 gives you 220 microseconds during which you know that the read will appear atomic." Agree with the first point, but the second, the emulated RTC never returns a bogus read. So what is your point, exactly? And what that has to do with UIP always returning 0?
On 01/12/2012 11:20 AM, Marcelo Tosatti wrote: >> > >> > The point is not_correctness_. It is_atomicity_. > Quoting you earlier > > "This is incorrect, for two reasons. First, the UIP is in the spec, > and we have to implement it. Second, reading the clock is not atomic, > and waiting for UIP=0 gives you 220 microseconds during which you know > that the read will appear atomic." (The actual figure is 244, not 220; my mistake). > Agree with the first point, but the second, the emulated RTC never > returns a bogus read. That's true: the update cycle takes 1984 us on the real RTC, and 0 us on QEMU. However, the data sheet says that the update cycle begins 244 us _after_ the rising edge of UIP. In other words, UIP is set for 1984 + 244 us on the real RTC, and should be set for 0 + 244 us on the emulated RTC. This is done on purpose: the data sheet says, "if a low is read on the UIP bit, the user has at least 244 us before the time/calendar data will be changed". As long as you read the time within 244 us, the following cannot happen: read UIP => 0 read HOURS => 10 read MINUTES => 59 read SECONDS => 59 ... read UIP => 0 read HOURS => 10 read MINUTES => 59 update happens! (on real RTC: update cycle starts) read SECONDS => 0 (on real RTC: undefined) You are a kernel junkie and as such you are too much accustomed to seqlocks. ;) If you know the read will take more than 244 us, the data sheet suggests that you use the update-ended interrupt. But you can also wait for the falling edge of UIP, like Xen does. The falling edge of UIP will never happen if the emulated RTC always returns 0 for UIP. Paolo
diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c index 9cbd052..ac1854e 100644 --- a/hw/mc146818rtc.c +++ b/hw/mc146818rtc.c @@ -84,7 +84,7 @@ typedef struct RTCState { MemoryRegion io; uint8_t cmos_data[128]; uint8_t cmos_index; - struct tm current_tm; + int64_t offset; int32_t base_year; qemu_irq irq; qemu_irq sqw_irq; @@ -93,19 +93,18 @@ typedef struct RTCState { QEMUTimer *periodic_timer; int64_t next_periodic_time; /* second update */ - int64_t next_second_time; + QEMUTimer *update_timer; + int64_t next_update_time; + /* alarm */ + QEMUTimer *alarm_timer; + int64_t next_alarm_time; uint16_t irq_reinject_on_ack_count; uint32_t irq_coalesced; uint32_t period; QEMUTimer *coalesced_timer; - QEMUTimer *second_timer; - QEMUTimer *second_timer2; Notifier clock_reset_notifier; } RTCState; -static void rtc_set_time(RTCState *s); -static void rtc_copy_date(RTCState *s); - #ifdef TARGET_I386 static void rtc_coalesced_timer_update(RTCState *s) { @@ -140,6 +139,72 @@ static void rtc_coalesced_timer(void *opaque) } #endif +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 (s->cmos_data[RTC_REG_B] & REG_B_DM) { + return a; + } else { + return ((a >> 4) * 10) + (a & 0x0f); + } +} + +static void rtc_set_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) && + (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_year = rtc_from_bcd(s, s->cmos_data[RTC_YEAR]) + s->base_year - 1900; + + s->offset = qemu_timedate_diff(&tm); + + rtc_change_mon_event(&tm); +} + +static void rtc_update_time(RTCState *s) +{ + struct tm tm; + int year; + + qemu_get_timedate(&tm, s->offset); + + 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 */ + s->cmos_data[RTC_HOURS] = rtc_to_bcd(s, tm.tm_hour % 12); + 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); + year = (tm.tm_year - s->base_year) % 100; + if (year < 0) + year += 100; + s->cmos_data[RTC_YEAR] = rtc_to_bcd(s, year); +} + static void rtc_timer_update(RTCState *s, int64_t current_time) { int period_code, period; @@ -174,7 +239,7 @@ static void rtc_timer_update(RTCState *s, int64_t current_time) } } -static void rtc_periodic_timer(void *opaque) +static void rtc_periodic_interrupt(void *opaque) { RTCState *s = opaque; @@ -204,6 +269,92 @@ static void rtc_periodic_timer(void *opaque) } } +static void rtc_enable_update_interrupt(void *opaque) +{ + RTCState *s = opaque; + + s->next_update_time = qemu_get_clock_ns(rtc_clock) + get_ticks_per_sec(); + qemu_mod_timer(s->update_timer, s->next_update_time); +} + +static void rtc_disable_update_interrupt(void *opaque) +{ + RTCState *s = opaque; + + qemu_del_timer(s->update_timer); +} + +static void rtc_update_interrupt(void *opaque) +{ + RTCState *s = opaque; + + /* update ended interrupt */ + s->cmos_data[RTC_REG_C] |= REG_C_UF; + if (s->cmos_data[RTC_REG_B] & REG_B_UIE) { + s->cmos_data[RTC_REG_C] |= REG_C_IRQF; + qemu_irq_raise(s->irq); + + s->next_update_time += get_ticks_per_sec(); + qemu_mod_timer(s->update_timer, s->next_update_time); + } else + rtc_disable_update_interrupt(s); +} + +static void rtc_enable_alarm(void *opaque) +{ + RTCState *s = opaque; + + s->next_alarm_time = qemu_get_clock_ns(rtc_clock) + get_ticks_per_sec(); + s->next_alarm_time /= get_ticks_per_sec(); + s->next_alarm_time *= get_ticks_per_sec(); + + qemu_mod_timer(s->alarm_timer, s->next_alarm_time); +} + +static void rtc_disable_alarm(void *opaque) +{ + RTCState *s = opaque; + + qemu_del_timer(s->alarm_timer); +} + +static void rtc_alarm_interrupt(void *opaque) +{ + RTCState *s = opaque; + struct tm tm; + int hour = 0; + + qemu_get_timedate(&tm, s->offset); + + if ((s->cmos_data[RTC_HOURS_ALARM] & 0xc0) != 0xc0) { + hour = rtc_from_bcd(s, s->cmos_data[RTC_HOURS_ALARM] & 0x7f); + if (!(s->cmos_data[RTC_REG_B] & REG_B_24H) && + (s->cmos_data[RTC_HOURS_ALARM] & 0x80)) { + hour += 12; + } + } + + /* check alarm */ + if (s->cmos_data[RTC_REG_B] & REG_B_AIE) { + if (((s->cmos_data[RTC_SECONDS_ALARM] & 0xc0) == 0xc0 || + rtc_from_bcd(s, s->cmos_data[RTC_SECONDS_ALARM]) == tm.tm_sec) && + ((s->cmos_data[RTC_MINUTES_ALARM] & 0xc0) == 0xc0 || + rtc_from_bcd(s, s->cmos_data[RTC_MINUTES_ALARM]) == tm.tm_min) && + ((s->cmos_data[RTC_HOURS_ALARM] & 0xc0) == 0xc0 || + hour == tm.tm_hour)) { + + printf("raise irq\n"); + s->cmos_data[RTC_REG_C] |= 0xa0; + qemu_irq_raise(s->irq); + } + + s->next_alarm_time += get_ticks_per_sec(); + qemu_mod_timer(s->alarm_timer, s->next_alarm_time); + } else + rtc_disable_alarm(s); + +} + static void cmos_ioport_write(void *opaque, uint32_t addr, uint32_t data) { RTCState *s = opaque; @@ -239,26 +390,37 @@ static void cmos_ioport_write(void *opaque, uint32_t addr, uint32_t data) rtc_timer_update(s, qemu_get_clock_ns(rtc_clock)); break; case RTC_REG_B: - if (data & REG_B_SET) { - /* set mode: reset UIP mode */ - s->cmos_data[RTC_REG_A] &= ~REG_A_UIP; - data &= ~REG_B_UIE; - } else { + if (data & REG_B_SET) + rtc_update_time(s); + else { /* if disabling set mode, update the time */ if (s->cmos_data[RTC_REG_B] & REG_B_SET) { rtc_set_time(s); } } + if (((s->cmos_data[RTC_REG_B] ^ data) & (REG_B_DM | REG_B_24H)) && !(data & REG_B_SET)) { /* If the time format has changed and not in set mode, update the registers immediately. */ s->cmos_data[RTC_REG_B] = data; - rtc_copy_date(s); - } else { + rtc_update_time(s); + } else s->cmos_data[RTC_REG_B] = data; - } + + /* check alarm interrupt */ + if ((s->cmos_data[RTC_REG_B] & REG_B_AIE) && + !(s->cmos_data[RTC_REG_B] & REG_B_SET) ) + rtc_enable_alarm(s); + + /* check update-ended interrupt */ + if ((s->cmos_data[RTC_REG_B] & REG_B_UIE) && + !(s->cmos_data[RTC_REG_B] & REG_B_SET)) + rtc_enable_update_interrupt(s); + + /* check periodic interrupt or square-wave */ rtc_timer_update(s, qemu_get_clock_ns(rtc_clock)); + break; case RTC_REG_C: case RTC_REG_D: @@ -271,184 +433,6 @@ static void cmos_ioport_write(void *opaque, uint32_t addr, uint32_t data) } } -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 (s->cmos_data[RTC_REG_B] & REG_B_DM) { - return a; - } else { - return ((a >> 4) * 10) + (a & 0x0f); - } -} - -static void rtc_set_time(RTCState *s) -{ - struct tm *tm = &s->current_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) && - (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_year = rtc_from_bcd(s, s->cmos_data[RTC_YEAR]) + s->base_year - 1900; - - rtc_change_mon_event(tm); -} - -static void rtc_copy_date(RTCState *s) -{ - const struct tm *tm = &s->current_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 */ - s->cmos_data[RTC_HOURS] = rtc_to_bcd(s, tm->tm_hour % 12); - 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); - year = (tm->tm_year - s->base_year) % 100; - if (year < 0) - year += 100; - s->cmos_data[RTC_YEAR] = rtc_to_bcd(s, year); -} - -/* month is between 0 and 11. */ -static int get_days_in_month(int month, int year) -{ - static const int days_tab[12] = { - 31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31 - }; - int d; - if ((unsigned )month >= 12) - return 31; - d = days_tab[month]; - if (month == 1) { - if ((year % 4) == 0 && ((year % 100) != 0 || (year % 400) == 0)) - d++; - } - return d; -} - -/* update 'tm' to the next second */ -static void rtc_next_second(struct tm *tm) -{ - int days_in_month; - - tm->tm_sec++; - if ((unsigned)tm->tm_sec >= 60) { - tm->tm_sec = 0; - tm->tm_min++; - if ((unsigned)tm->tm_min >= 60) { - tm->tm_min = 0; - tm->tm_hour++; - if ((unsigned)tm->tm_hour >= 24) { - tm->tm_hour = 0; - /* next day */ - tm->tm_wday++; - if ((unsigned)tm->tm_wday >= 7) - tm->tm_wday = 0; - days_in_month = get_days_in_month(tm->tm_mon, - tm->tm_year + 1900); - tm->tm_mday++; - if (tm->tm_mday < 1) { - tm->tm_mday = 1; - } else if (tm->tm_mday > days_in_month) { - tm->tm_mday = 1; - tm->tm_mon++; - if (tm->tm_mon >= 12) { - tm->tm_mon = 0; - tm->tm_year++; - } - } - } - } - } -} - - -static void rtc_update_second(void *opaque) -{ - RTCState *s = opaque; - int64_t delay; - - /* if the oscillator is not in normal operation, we do not update */ - if ((s->cmos_data[RTC_REG_A] & 0x70) != 0x20) { - s->next_second_time += get_ticks_per_sec(); - qemu_mod_timer(s->second_timer, s->next_second_time); - } else { - rtc_next_second(&s->current_tm); - - if (!(s->cmos_data[RTC_REG_B] & REG_B_SET)) { - /* update in progress bit */ - s->cmos_data[RTC_REG_A] |= REG_A_UIP; - } - /* should be 244 us = 8 / 32768 seconds, but currently the - timers do not have the necessary resolution. */ - delay = (get_ticks_per_sec() * 1) / 100; - if (delay < 1) - delay = 1; - qemu_mod_timer(s->second_timer2, - s->next_second_time + delay); - } -} - -static void rtc_update_second2(void *opaque) -{ - RTCState *s = opaque; - - if (!(s->cmos_data[RTC_REG_B] & REG_B_SET)) { - rtc_copy_date(s); - } - - /* check alarm */ - if (s->cmos_data[RTC_REG_B] & REG_B_AIE) { - if (((s->cmos_data[RTC_SECONDS_ALARM] & 0xc0) == 0xc0 || - rtc_from_bcd(s, s->cmos_data[RTC_SECONDS_ALARM]) == s->current_tm.tm_sec) && - ((s->cmos_data[RTC_MINUTES_ALARM] & 0xc0) == 0xc0 || - rtc_from_bcd(s, s->cmos_data[RTC_MINUTES_ALARM]) == s->current_tm.tm_min) && - ((s->cmos_data[RTC_HOURS_ALARM] & 0xc0) == 0xc0 || - rtc_from_bcd(s, s->cmos_data[RTC_HOURS_ALARM]) == s->current_tm.tm_hour)) { - - s->cmos_data[RTC_REG_C] |= 0xa0; - qemu_irq_raise(s->irq); - } - } - - /* update ended interrupt */ - s->cmos_data[RTC_REG_C] |= REG_C_UF; - if (s->cmos_data[RTC_REG_B] & REG_B_UIE) { - s->cmos_data[RTC_REG_C] |= REG_C_IRQF; - qemu_irq_raise(s->irq); - } - - /* clear update in progress bit */ - s->cmos_data[RTC_REG_A] &= ~REG_A_UIP; - - s->next_second_time += get_ticks_per_sec(); - qemu_mod_timer(s->second_timer, s->next_second_time); -} - static uint32_t cmos_ioport_read(void *opaque, uint32_t addr) { RTCState *s = opaque; @@ -464,9 +448,9 @@ static uint32_t cmos_ioport_read(void *opaque, uint32_t addr) case RTC_DAY_OF_MONTH: case RTC_MONTH: case RTC_YEAR: - ret = s->cmos_data[s->cmos_index]; - break; - case RTC_REG_A: + /* if not in set mode, update the time before read */ + if (!(s->cmos_data[RTC_REG_B] & REG_B_SET)) + rtc_update_time(s); ret = s->cmos_data[s->cmos_index]; break; case RTC_REG_C: @@ -507,13 +491,6 @@ void rtc_set_memory(ISADevice *dev, int addr, int val) s->cmos_data[addr] = val; } -void rtc_set_date(ISADevice *dev, const struct tm *tm) -{ - RTCState *s = DO_UPCAST(RTCState, dev, dev); - s->current_tm = *tm; - rtc_copy_date(s); -} - /* PC cmos mappings */ #define REG_IBM_CENTURY_BYTE 0x32 #define REG_IBM_PS2_CENTURY_BYTE 0x37 @@ -524,10 +501,11 @@ static void rtc_set_date_from_host(ISADevice *dev) struct tm tm; int val; + s->offset = 0; /* set the CMOS date */ - qemu_get_timedate(&tm, 0); - rtc_set_date(dev, &tm); + rtc_update_time(s); + qemu_get_timedate(&tm, s->offset); val = rtc_to_bcd(s, (tm.tm_year / 100) + 19); rtc_set_memory(dev, REG_IBM_CENTURY_BYTE, val); rtc_set_memory(dev, REG_IBM_PS2_CENTURY_BYTE, val); @@ -556,18 +534,13 @@ static const VMStateDescription vmstate_rtc = { .fields = (VMStateField []) { VMSTATE_BUFFER(cmos_data, RTCState), VMSTATE_UINT8(cmos_index, RTCState), - VMSTATE_INT32(current_tm.tm_sec, RTCState), - VMSTATE_INT32(current_tm.tm_min, RTCState), - VMSTATE_INT32(current_tm.tm_hour, RTCState), - VMSTATE_INT32(current_tm.tm_wday, RTCState), - VMSTATE_INT32(current_tm.tm_mday, RTCState), - VMSTATE_INT32(current_tm.tm_mon, RTCState), - VMSTATE_INT32(current_tm.tm_year, RTCState), + VMSTATE_INT64(offset, RTCState), VMSTATE_TIMER(periodic_timer, RTCState), VMSTATE_INT64(next_periodic_time, RTCState), - VMSTATE_INT64(next_second_time, RTCState), - VMSTATE_TIMER(second_timer, RTCState), - VMSTATE_TIMER(second_timer2, RTCState), + VMSTATE_TIMER(update_timer, RTCState), + VMSTATE_INT64(next_update_time, RTCState), + VMSTATE_TIMER(alarm_timer, RTCState), + VMSTATE_INT64(next_alarm_time, RTCState), VMSTATE_UINT32_V(irq_coalesced, RTCState, 2), VMSTATE_UINT32_V(period, RTCState, 2), VMSTATE_END_OF_LIST() @@ -580,8 +553,6 @@ static void rtc_notify_clock_reset(Notifier *notifier, void *data) int64_t now = *(int64_t *)data; rtc_set_date_from_host(&s->dev); - s->next_second_time = now + (get_ticks_per_sec() * 99) / 100; - qemu_mod_timer(s->second_timer2, s->next_second_time); rtc_timer_update(s, now); #ifdef TARGET_I386 if (rtc_td_hack) { @@ -627,13 +598,15 @@ static void rtc_get_date(DeviceState *dev, Visitor *v, void *opaque, ISADevice *isa = DO_UPCAST(ISADevice, qdev, dev); RTCState *s = DO_UPCAST(RTCState, dev, isa); + struct tm tm; + qemu_get_timedate(&tm, s->offset); visit_start_struct(v, NULL, "struct tm", name, 0, errp); - visit_type_int32(v, &s->current_tm.tm_year, "tm_year", errp); - visit_type_int32(v, &s->current_tm.tm_mon, "tm_mon", errp); - visit_type_int32(v, &s->current_tm.tm_mday, "tm_mday", errp); - visit_type_int32(v, &s->current_tm.tm_hour, "tm_hour", errp); - visit_type_int32(v, &s->current_tm.tm_min, "tm_min", errp); - visit_type_int32(v, &s->current_tm.tm_sec, "tm_sec", errp); + visit_type_int32(v, &tm.tm_year, "tm_year", errp); + visit_type_int32(v, &tm.tm_mon, "tm_mon", errp); + visit_type_int32(v, &tm.tm_mday, "tm_mday", errp); + visit_type_int32(v, &tm.tm_hour, "tm_hour", errp); + visit_type_int32(v, &tm.tm_min, "tm_min", errp); + visit_type_int32(v, &tm.tm_sec, "tm_sec", errp); visit_end_struct(v, errp); } @@ -649,22 +622,18 @@ static int rtc_initfn(ISADevice *dev) rtc_set_date_from_host(dev); - s->periodic_timer = qemu_new_timer_ns(rtc_clock, rtc_periodic_timer, s); + s->periodic_timer = qemu_new_timer_ns(rtc_clock, rtc_periodic_interrupt, s); #ifdef TARGET_I386 if (rtc_td_hack) s->coalesced_timer = qemu_new_timer_ns(rtc_clock, rtc_coalesced_timer, s); #endif - s->second_timer = qemu_new_timer_ns(rtc_clock, rtc_update_second, s); - s->second_timer2 = qemu_new_timer_ns(rtc_clock, rtc_update_second2, s); + s->update_timer = qemu_new_timer_ns(rtc_clock, rtc_update_interrupt, s); + s->alarm_timer = qemu_new_timer_ns(rtc_clock, rtc_alarm_interrupt, s); s->clock_reset_notifier.notify = rtc_notify_clock_reset; qemu_register_clock_reset_notifier(rtc_clock, &s->clock_reset_notifier); - s->next_second_time = - qemu_get_clock_ns(rtc_clock) + (get_ticks_per_sec() * 99) / 100; - qemu_mod_timer(s->second_timer2, s->next_second_time); - memory_region_init_io(&s->io, &cmos_ops, s, "rtc", 2); isa_register_ioport(dev, &s->io, base);
change the RTC update logic to use host time with offset to calculate RTC clock. There have no need to use two periodic timers to maintain an internal timer for RTC clock update and alarm check. Instead, we calculate the real RTC time by the host time with an offset. For alarm and updated-end interrupt, if guest enabled it, then we setup a timer, or else, stop it. Signed-off-by: Yang Zhang <yang.z.zhang@intel.com>