| Message ID | A9667DDFB95DB7438FA9D7D576C3D87E09E751@SHSMSX101.ccr.corp.intel.com |
|---|---|
| State | New |
| Headers | show |
On Mon, 19 Mar 2012, Zhang, Yang Z wrote: > Use a timer to emulate update cycle. When update cycle ended and UIE is setting, then raise an interrupt. The timer runs only when UF or AF is cleared. The idea is that if the user requests the update-ended interrupt (UIE) we setup a timer to inject it at the right time into the guest and another timer to update the UIP bit, correct? But do we actually need the second timer? Why can't we just update the UIP bit whenever the user tries to read reg A, as we do when UIE is not set? Also I am not sure it is a great idea to rename rtc_timer_update into periodic_timer_update in the same patch where you introduce this new logic. > Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com> > --- > hw/mc146818rtc.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++++++---- > 1 files changed, 80 insertions(+), 6 deletions(-) > > diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c > index 5e7fbb5..fae049e 100644 > --- a/hw/mc146818rtc.c > +++ b/hw/mc146818rtc.c > @@ -97,6 +97,11 @@ typedef struct RTCState { > /* periodic timer */ > QEMUTimer *periodic_timer; > int64_t next_periodic_time; > + /* update-ended timer */ > + QEMUTimer *update_timer; > + QEMUTimer *update_timer2; > + uint64_t next_update_time; > + uint32_t use_timer; > uint16_t irq_reinject_on_ack_count; > uint32_t irq_coalesced; > uint32_t period; > @@ -157,7 +162,8 @@ static void rtc_coalesced_timer(void *opaque) > } > #endif > > -static void rtc_timer_update(RTCState *s, int64_t current_time) > +/* handle periodic timer */ > +static void periodic_timer_update(RTCState *s, int64_t current_time) > { > int period_code, period; > int64_t cur_clock, next_irq_clock; > @@ -195,7 +201,7 @@ static void rtc_periodic_timer(void *opaque) > { > RTCState *s = opaque; > > - rtc_timer_update(s, s->next_periodic_time); > + periodic_timer_update(s, s->next_periodic_time); > s->cmos_data[RTC_REG_C] |= REG_C_PF; > if (s->cmos_data[RTC_REG_B] & REG_B_PIE) { > s->cmos_data[RTC_REG_C] |= REG_C_IRQF; > @@ -222,6 +228,58 @@ static void rtc_periodic_timer(void *opaque) > } > } > > +/* handle update-ended timer */ > +static void check_update_timer(RTCState *s) > +{ > + uint64_t next_update_time, expire_time; > + uint64_t guest_usec; > + qemu_del_timer(s->update_timer); > + qemu_del_timer(s->update_timer2); > + > + if (!((s->cmos_data[RTC_REG_C] & (REG_C_UF | REG_C_AF)) == > + (REG_C_UF | REG_C_AF)) && !(s->cmos_data[RTC_REG_B] & REG_B_SET)) { > + s->use_timer = 1; > + guest_usec = get_guest_rtc_us(s) % USEC_PER_SEC; > + if (guest_usec >= (USEC_PER_SEC - 244)) { > + /* RTC is in update cycle when enabling UIE */ > + s->cmos_data[RTC_REG_A] |= REG_A_UIP; > + next_update_time = (USEC_PER_SEC - guest_usec) * NS_PER_USEC; > + expire_time = qemu_get_clock_ns(rtc_clock) + next_update_time; > + qemu_mod_timer(s->update_timer2, expire_time); > + } else { > + next_update_time = (USEC_PER_SEC - guest_usec - 244) * NS_PER_USEC; > + expire_time = qemu_get_clock_ns(rtc_clock) + next_update_time; > + s->next_update_time = expire_time; > + qemu_mod_timer(s->update_timer, expire_time); > + } > + } else { > + s->use_timer = 0; > + } > +} > +static void rtc_update_timer(void *opaque) > +{ > + RTCState *s = opaque; > + > + if (!(s->cmos_data[RTC_REG_B] & REG_B_SET)) { > + s->cmos_data[RTC_REG_A] |= REG_A_UIP; > + qemu_mod_timer(s->update_timer2, s->next_update_time + 244000UL); > + } > +} > + > +static void rtc_update_timer2(void *opaque) > +{ > + RTCState *s = opaque; > + > + if (!(s->cmos_data[RTC_REG_B] & REG_B_SET)) { > + s->cmos_data[RTC_REG_C] |= REG_C_UF; > + s->cmos_data[RTC_REG_A] &= ~REG_A_UIP; > + s->cmos_data[RTC_REG_C] |= REG_C_IRQF; > + qemu_irq_raise(s->irq); > + } > + check_update_timer(s); > +} > + > static void rtc_set_offset(RTCState *s, int32_t start_usec) > { > struct tm *tm = &s->current_tm; > @@ -283,13 +341,14 @@ static void cmos_ioport_write(void *opaque, uint32_t addr, uint32_t data) > rtc_calibrate_time(s); > rtc_set_offset(s, 500000); > s->cmos_data[RTC_REG_A] &= ~REG_A_UIP; > + check_update_timer(s); > divider_reset = 0; > } > } > /* UIP bit is read only */ > s->cmos_data[RTC_REG_A] = (data & ~REG_A_UIP) | > (s->cmos_data[RTC_REG_A] & REG_A_UIP); > - rtc_timer_update(s, qemu_get_clock_ns(rtc_clock)); > + periodic_timer_update(s, qemu_get_clock_ns(rtc_clock)); > break; > case RTC_REG_B: > if (data & REG_B_SET) { > @@ -315,7 +374,8 @@ static void cmos_ioport_write(void *opaque, uint32_t addr, uint32_t data) > } > } > s->cmos_data[RTC_REG_B] = data; > - rtc_timer_update(s, qemu_get_clock_ns(rtc_clock)); > + periodic_timer_update(s, qemu_get_clock_ns(rtc_clock)); > + check_update_timer(s); > break; > case RTC_REG_C: > case RTC_REG_D: > @@ -445,7 +505,7 @@ static uint32_t cmos_ioport_read(void *opaque, uint32_t addr) > break; > case RTC_REG_A: > ret = s->cmos_data[s->cmos_index]; > - if (update_in_progress(s)) { > + if ((s->use_timer == 0) && update_in_progress(s)) { > ret |= REG_A_UIP; > } > break; > @@ -453,6 +513,12 @@ static uint32_t cmos_ioport_read(void *opaque, uint32_t addr) > ret = s->cmos_data[s->cmos_index]; > qemu_irq_lower(s->irq); > s->cmos_data[RTC_REG_C] = 0x00; > + if (!(s->cmos_data[RTC_REG_B] & REG_B_SET)) { > + rtc_calibrate_time(s); > + } > + if (ret & (REG_C_UF | REG_C_AF)) { > + check_update_timer(s); > + } > #ifdef TARGET_I386 > if(s->irq_coalesced && > (s->cmos_data[RTC_REG_B] & REG_B_PIE) && > @@ -545,6 +611,9 @@ static const VMStateDescription vmstate_rtc = { > VMSTATE_INT32(offset_usec, RTCState), > VMSTATE_TIMER(periodic_timer, RTCState), > VMSTATE_INT64(next_periodic_time, RTCState), > + VMSTATE_TIMER(update_timer, RTCState), > + VMSTATE_TIMER(update_timer2, RTCState), > + VMSTATE_UINT64(next_update_time, RTCState), > VMSTATE_UINT32_V(irq_coalesced, RTCState, 2), > VMSTATE_UINT32_V(period, RTCState, 2), > VMSTATE_END_OF_LIST() > @@ -557,7 +626,8 @@ static void rtc_notify_clock_reset(Notifier *notifier, void *data) > int64_t now = *(int64_t *)data; > > rtc_set_date_from_host(&s->dev); > - rtc_timer_update(s, now); > + periodic_timer_update(s, now); > + check_update_timer(s); > #ifdef TARGET_I386 > if (s->lost_tick_policy == LOST_TICK_SLEW) { > rtc_coalesced_timer_update(s); > @@ -579,6 +649,7 @@ static void rtc_reset(void *opaque) > > s->cmos_data[RTC_REG_B] &= ~(REG_B_PIE | REG_B_AIE | REG_B_SQWE); > s->cmos_data[RTC_REG_C] &= ~(REG_C_UF | REG_C_IRQF | REG_C_PF | REG_C_AF); > + check_update_timer(s); > > qemu_irq_lower(s->irq); > > @@ -649,6 +720,9 @@ static int rtc_initfn(ISADevice *dev) > #endif > > s->periodic_timer = qemu_new_timer_ns(rtc_clock, rtc_periodic_timer, s); > + s->update_timer = qemu_new_timer_ns(rtc_clock, rtc_update_timer, s); > + s->update_timer2 = qemu_new_timer_ns(rtc_clock, rtc_update_timer2, s); > + check_update_timer(s); > > s->clock_reset_notifier.notify = rtc_notify_clock_reset; > qemu_register_clock_reset_notifier(rtc_clock, &s->clock_reset_notifier); > -- > 1.7.1 >
> +/* handle update-ended timer */ > +static void check_update_timer(RTCState *s) > +{ > + uint64_t next_update_time, expire_time; > + uint64_t guest_usec; > + qemu_del_timer(s->update_timer); > + qemu_del_timer(s->update_timer2); > + > + if (!((s->cmos_data[RTC_REG_C] & (REG_C_UF | REG_C_AF)) == > + (REG_C_UF | REG_C_AF)) && !(s->cmos_data[RTC_REG_B] & REG_B_SET)) { > + s->use_timer = 1; > + guest_usec = get_guest_rtc_us(s) % USEC_PER_SEC; > + if (guest_usec >= (USEC_PER_SEC - 244)) { > + /* RTC is in update cycle when enabling UIE */ > + s->cmos_data[RTC_REG_A] |= REG_A_UIP; > + next_update_time = (USEC_PER_SEC - guest_usec) * NS_PER_USEC; > + expire_time = qemu_get_clock_ns(rtc_clock) + next_update_time; > + qemu_mod_timer(s->update_timer2, expire_time); > + } else { > + next_update_time = (USEC_PER_SEC - guest_usec - 244) * NS_PER_USEC; > + expire_time = qemu_get_clock_ns(rtc_clock) + next_update_time; > + s->next_update_time = expire_time; > + qemu_mod_timer(s->update_timer, expire_time); > + } > + } else { > + s->use_timer = 0; > + } > +} This is the function that is used to figure out whether we need the timers or not, the condition seems to be: (Not (REG_C_UF | REG_C_AF)) And (Not (REG_B_SET)) Shouldn't actually check for UIE being enabled?
Il 20/03/2012 19:35, Stefano Stabellini ha scritto: > This is the function that is used to figure out whether we need the > timers or not, the condition seems to be: > > (Not (REG_C_UF | REG_C_AF)) And (Not (REG_B_SET)) > > Shouldn't actually check for UIE being enabled? No, you need to set UF in case the code observes it without actually enabling interrupt delivery on the ISA bus. Paolo
On Wed, 21 Mar 2012, Paolo Bonzini wrote: > Il 20/03/2012 19:35, Stefano Stabellini ha scritto: > > This is the function that is used to figure out whether we need the > > timers or not, the condition seems to be: > > > > (Not (REG_C_UF | REG_C_AF)) And (Not (REG_B_SET)) > > > > Shouldn't actually check for UIE being enabled? > > No, you need to set UF in case the code observes it without actually > enabling interrupt delivery on the ISA bus. Well, if it is just about updating UF, can we do it only when the user reads REG_C? I am just trying to limit the need for the update_timers...
Il 21/03/2012 17:54, Stefano Stabellini ha scritto: >> > >> > No, you need to set UF in case the code observes it without actually >> > enabling interrupt delivery on the ISA bus. > > Well, if it is just about updating UF, can we do it only when the user > reads REG_C? Perhaps, but for AF it is much harder. Note that if the guest does not care, this will run just once. Paolo
> -----Original Message----- > From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com] > Sent: Wednesday, March 21, 2012 2:04 AM > > On Mon, 19 Mar 2012, Zhang, Yang Z wrote: > > Use a timer to emulate update cycle. When update cycle ended and UIE is > setting, then raise an interrupt. The timer runs only when UF or AF is cleared. > > The idea is that if the user requests the update-ended interrupt (UIE) > we setup a timer to inject it at the right time into the guest and > another timer to update the UIP bit, correct? No, the timer runs whenever the UF and AF is cleared, not only UIE or AIE is set. > But do we actually need the second timer? Why can't we just update the > UIP bit whenever the user tries to read reg A, as we do when UIE is not set? The purpose of using two timer is trying to keep the UF, AF and UIP synchronous. User can poll UIP to check UF and AF bit. If we use timer for UF/AF bit track and check UIP by another way, since the timer will be fired with delay, then the problem is encountered: the UIP is cleared, but due to the delay of timer, the UF/AF bit is not set. So we need to check them on a same level. Although we can update UF/AF when reading it, the logic is too complicated, especially for AF bit. > > Also I am not sure it is a great idea to rename rtc_timer_update into > periodic_timer_update in the same patch where you introduce this new > logic. Ok, it make sense. best regards yang
On Thu, 22 Mar 2012, Zhang, Yang Z wrote: > > -----Original Message----- > > From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com] > > Sent: Wednesday, March 21, 2012 2:04 AM > > > > On Mon, 19 Mar 2012, Zhang, Yang Z wrote: > > > Use a timer to emulate update cycle. When update cycle ended and UIE is > > setting, then raise an interrupt. The timer runs only when UF or AF is cleared. > > > > The idea is that if the user requests the update-ended interrupt (UIE) > > we setup a timer to inject it at the right time into the guest and > > another timer to update the UIP bit, correct? > No, the timer runs whenever the UF and AF is cleared, not only UIE or AIE is set. > > > But do we actually need the second timer? Why can't we just update the > > UIP bit whenever the user tries to read reg A, as we do when UIE is not set? > The purpose of using two timer is trying to keep the UF, AF and UIP synchronous. User can poll UIP to check UF and AF bit. If we use timer for UF/AF bit track and check UIP by another way, since the timer will be fired with delay, then the problem is encountered: the UIP is cleared, but due to the delay of timer, the UF/AF bit is not set. So we need to check them on a same level. Although we can update UF/AF when reading it, the logic is too complicated, especially for AF bit. fair enough
On Wed, 21 Mar 2012, Paolo Bonzini wrote: > Il 21/03/2012 17:54, Stefano Stabellini ha scritto: > >> > > >> > No, you need to set UF in case the code observes it without actually > >> > enabling interrupt delivery on the ISA bus. > > > > Well, if it is just about updating UF, can we do it only when the user > > reads REG_C? > > Perhaps, but for AF it is much harder. Note that if the guest does not > care, this will run just once. Right, probably not worth the effort.
Il 22/03/2012 11:29, Stefano Stabellini ha scritto: >> The purpose of using two timer is trying to keep the UF, AF and UIP >> synchronous. User can poll UIP to check UF and AF bit. If we use >> timer for UF/AF bit track and check UIP by another way, since the >> timer will be fired with delay, then the problem is encountered: >> the UIP is cleared, but due to the delay of timer, the UF/AF bit is >> not set. So we need to check them on a same level. Although we can >> update UF/AF when reading it, the logic is too complicated, >> especially for AF bit. FWIW, the solution I used when I removed the second timer from the current code (not yet posted, I do hope your patches can be fixed!) was to set UIP when reading A, and clear it in the timer. You have something like that, but keep the two timers. Paolo
diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c index 5e7fbb5..fae049e 100644 --- a/hw/mc146818rtc.c +++ b/hw/mc146818rtc.c @@ -97,6 +97,11 @@ typedef struct RTCState { /* periodic timer */ QEMUTimer *periodic_timer; int64_t next_periodic_time; + /* update-ended timer */ + QEMUTimer *update_timer; + QEMUTimer *update_timer2; + uint64_t next_update_time; + uint32_t use_timer; uint16_t irq_reinject_on_ack_count; uint32_t irq_coalesced; uint32_t period; @@ -157,7 +162,8 @@ static void rtc_coalesced_timer(void *opaque) } #endif -static void rtc_timer_update(RTCState *s, int64_t current_time) +/* handle periodic timer */ +static void periodic_timer_update(RTCState *s, int64_t current_time) { int period_code, period; int64_t cur_clock, next_irq_clock; @@ -195,7 +201,7 @@ static void rtc_periodic_timer(void *opaque) { RTCState *s = opaque; - rtc_timer_update(s, s->next_periodic_time); + periodic_timer_update(s, s->next_periodic_time); s->cmos_data[RTC_REG_C] |= REG_C_PF; if (s->cmos_data[RTC_REG_B] & REG_B_PIE) { s->cmos_data[RTC_REG_C] |= REG_C_IRQF; @@ -222,6 +228,58 @@ static void rtc_periodic_timer(void *opaque) } } +/* handle update-ended timer */ +static void check_update_timer(RTCState *s) +{ + uint64_t next_update_time, expire_time; + uint64_t guest_usec; + qemu_del_timer(s->update_timer); + qemu_del_timer(s->update_timer2); + + if (!((s->cmos_data[RTC_REG_C] & (REG_C_UF | REG_C_AF)) == + (REG_C_UF | REG_C_AF)) && !(s->cmos_data[RTC_REG_B] & REG_B_SET)) { + s->use_timer = 1; + guest_usec = get_guest_rtc_us(s) % USEC_PER_SEC; + if (guest_usec >= (USEC_PER_SEC - 244)) { + /* RTC is in update cycle when enabling UIE */ + s->cmos_data[RTC_REG_A] |= REG_A_UIP; + next_update_time = (USEC_PER_SEC - guest_usec) * NS_PER_USEC; + expire_time = qemu_get_clock_ns(rtc_clock) + next_update_time; + qemu_mod_timer(s->update_timer2, expire_time); + } else { + next_update_time = (USEC_PER_SEC - guest_usec - 244) * NS_PER_USEC; + expire_time = qemu_get_clock_ns(rtc_clock) + next_update_time; + s->next_update_time = expire_time; + qemu_mod_timer(s->update_timer, expire_time); + } + } else { + s->use_timer = 0; + } +} + +static void rtc_update_timer(void *opaque) +{ + RTCState *s = opaque; + + if (!(s->cmos_data[RTC_REG_B] & REG_B_SET)) { + s->cmos_data[RTC_REG_A] |= REG_A_UIP; + qemu_mod_timer(s->update_timer2, s->next_update_time + 244000UL); + } +} + +static void rtc_update_timer2(void *opaque) +{ + RTCState *s = opaque; + + if (!(s->cmos_data[RTC_REG_B] & REG_B_SET)) { + s->cmos_data[RTC_REG_C] |= REG_C_UF; + s->cmos_data[RTC_REG_A] &= ~REG_A_UIP; + s->cmos_data[RTC_REG_C] |= REG_C_IRQF; + qemu_irq_raise(s->irq); + } + check_update_timer(s); +} + static void rtc_set_offset(RTCState *s, int32_t start_usec) { struct tm *tm = &s->current_tm; @@ -283,13 +341,14 @@ static void cmos_ioport_write(void *opaque, uint32_t addr, uint32_t data) rtc_calibrate_time(s); rtc_set_offset(s, 500000); s->cmos_data[RTC_REG_A] &= ~REG_A_UIP; + check_update_timer(s); divider_reset = 0; } } /* UIP bit is read only */ s->cmos_data[RTC_REG_A] = (data & ~REG_A_UIP) | (s->cmos_data[RTC_REG_A] & REG_A_UIP); - rtc_timer_update(s, qemu_get_clock_ns(rtc_clock)); + periodic_timer_update(s, qemu_get_clock_ns(rtc_clock)); break; case RTC_REG_B: if (data & REG_B_SET) { @@ -315,7 +374,8 @@ static void cmos_ioport_write(void *opaque, uint32_t addr, uint32_t data) } } s->cmos_data[RTC_REG_B] = data; - rtc_timer_update(s, qemu_get_clock_ns(rtc_clock)); + periodic_timer_update(s, qemu_get_clock_ns(rtc_clock)); + check_update_timer(s); break; case RTC_REG_C: case RTC_REG_D: @@ -445,7 +505,7 @@ static uint32_t cmos_ioport_read(void *opaque, uint32_t addr) break; case RTC_REG_A: ret = s->cmos_data[s->cmos_index]; - if (update_in_progress(s)) { + if ((s->use_timer == 0) && update_in_progress(s)) { ret |= REG_A_UIP; } break; @@ -453,6 +513,12 @@ static uint32_t cmos_ioport_read(void *opaque, uint32_t addr) ret = s->cmos_data[s->cmos_index]; qemu_irq_lower(s->irq); s->cmos_data[RTC_REG_C] = 0x00; + if (!(s->cmos_data[RTC_REG_B] & REG_B_SET)) { + rtc_calibrate_time(s); + } + if (ret & (REG_C_UF | REG_C_AF)) { + check_update_timer(s); + } #ifdef TARGET_I386 if(s->irq_coalesced && (s->cmos_data[RTC_REG_B] & REG_B_PIE) && @@ -545,6 +611,9 @@ static const VMStateDescription vmstate_rtc = { VMSTATE_INT32(offset_usec, RTCState), VMSTATE_TIMER(periodic_timer, RTCState), VMSTATE_INT64(next_periodic_time, RTCState), + VMSTATE_TIMER(update_timer, RTCState), + VMSTATE_TIMER(update_timer2, RTCState), + VMSTATE_UINT64(next_update_time, RTCState), VMSTATE_UINT32_V(irq_coalesced, RTCState, 2), VMSTATE_UINT32_V(period, RTCState, 2), VMSTATE_END_OF_LIST() @@ -557,7 +626,8 @@ static void rtc_notify_clock_reset(Notifier *notifier, void *data) int64_t now = *(int64_t *)data; rtc_set_date_from_host(&s->dev); - rtc_timer_update(s, now); + periodic_timer_update(s, now); + check_update_timer(s); #ifdef TARGET_I386 if (s->lost_tick_policy == LOST_TICK_SLEW) { rtc_coalesced_timer_update(s); @@ -579,6 +649,7 @@ static void rtc_reset(void *opaque) s->cmos_data[RTC_REG_B] &= ~(REG_B_PIE | REG_B_AIE | REG_B_SQWE); s->cmos_data[RTC_REG_C] &= ~(REG_C_UF | REG_C_IRQF | REG_C_PF | REG_C_AF); + check_update_timer(s); qemu_irq_lower(s->irq); @@ -649,6 +720,9 @@ static int rtc_initfn(ISADevice *dev) #endif s->periodic_timer = qemu_new_timer_ns(rtc_clock, rtc_periodic_timer, s); + s->update_timer = qemu_new_timer_ns(rtc_clock, rtc_update_timer, s); + s->update_timer2 = qemu_new_timer_ns(rtc_clock, rtc_update_timer2, s); + check_update_timer(s); s->clock_reset_notifier.notify = rtc_notify_clock_reset; qemu_register_clock_reset_notifier(rtc_clock, &s->clock_reset_notifier);
Use a timer to emulate update cycle. When update cycle ended and UIE is setting, then raise an interrupt. The timer runs only when UF or AF is cleared. Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com> --- hw/mc146818rtc.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++++++---- 1 files changed, 80 insertions(+), 6 deletions(-) -- 1.7.1