On 04/05/2017 13:59, guangrong.xiao@xxxxxxxxx wrote:
> From: Tai Yunfang <yunfangtai@xxxxxxxxxxx>
>
> There are two issues in current code:
> 1) If the period is changed by re-configuring RegA, the coalesced
> irq will be scaled to reflect the new period, however, it
> calculates the new interrupt number like this:
> s->irq_coalesced = (s->irq_coalesced * s->period) / period;
>
> There are some clocks will be lost if they are not enough to
> be squeezed to a single new period that will cause the VM clock
> slower
>
> In order to fix the issue, we calculate the interrupt window
> based on the precise clock rather than period, then the clocks
> lost during period is scaled can be compensated properly
>
> 2) If periodic_timer_update() is called due to RegA reconfiguration,
> i.e, the period is updated, current time is not the start point
> for the next periodic timer, instead, which should start from the
> last interrupt, otherwise, the clock in VM will become slow
>
> This patch takes the clocks from last interrupt to current clock
> into account and compensates the clocks for the next interrupt,
> especially，if a complete interrupt was lost in this window, the
> time can be caught up by LOST_TICK_POLICY_SLEW
>
> [ Xiao: redesign the algorithm based on Yunfang's original work. ]
>
> Signed-off-by: Xiao Guangrong <xiaoguangrong@xxxxxxxxxxx>
> Signed-off-by: Tai Yunfang <yunfangtai@xxxxxxxxxxx>
> ---
> hw/timer/mc146818rtc.c | 116 ++++++++++++++++++++++++++++++++++++++++---------
> 1 file changed, 96 insertions(+), 20 deletions(-)
>
> diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
> index 5cccb2a..14bde1a 100644
> --- a/hw/timer/mc146818rtc.c
> +++ b/hw/timer/mc146818rtc.c
> @@ -146,31 +146,104 @@ static void rtc_coalesced_timer(void *opaque)
> }
> #endif
>
> -/* handle periodic timer */
> -static void periodic_timer_update(RTCState *s, int64_t current_time)
> +static int period_code_to_clock(int period_code)
> +{
> + /* periodic timer is disabled. */
> + if (!period_code) {
> + return 0;
> + }
> +
> + if (period_code <= 2) {
> + period_code += 7;
> + }
> +
> + /* period in 32 Khz cycles */
> + return 1 << (period_code - 1);
> +}
> +
> +/*
> + * handle periodic timer. @old_period indicates the periodic timer update
> + * is just due to period adjustment.
> + */
> +static void
> +periodic_timer_update(RTCState *s, int64_t current_time, int old_period)
We need old_period to distinguish reconfiguration from interrupts ("if
(old_period)" below), but it can be a bool. Instead of passing the old
period value, we can use s->period which is
period_code_to_clock(old_period).
That is, just do
old_period = s->period;
s->period = rtc_periodic_clock_ticks(s);
/*
* if the periodic timer's update is due to period re-configuration,
* the clock should count since last interrupt.
*/
if (s->period != 0) {
...
if (!from_interrupt) {
}
...
}
where rtc_periodic_clock_ticks takes into account both regA and regB,
returning the result in 32 kHZ ticks.
> {
> int period_code, period;
> - int64_t cur_clock, next_irq_clock;
> + int64_t cur_clock, next_irq_clock, lost_clock = 0;
>
> period_code = s->cmos_data[RTC_REG_A] & 0x0f;
> if (period_code != 0
> && (s->cmos_data[RTC_REG_B] & REG_B_PIE)) {
> - if (period_code <= 2)
> - period_code += 7;
> - /* period in 32 Khz cycles */
> - period = 1 << (period_code - 1);
> -#ifdef TARGET_I386
> - if (period != s->period) {
> - s->irq_coalesced = (s->irq_coalesced * s->period) / period;
> - DPRINTF_C("cmos: coalesced irqs scaled to %d\n", s->irq_coalesced);
> - }
> - s->period = period;
> -#endif
> + period = period_code_to_clock(period_code);
Since we have old_period, we can assign s->period here.
> /* compute 32 khz clock */
> cur_clock =
> muldiv64(current_time, RTC_CLOCK_RATE, NANOSECONDS_PER_SECOND);
>
> - next_irq_clock = (cur_clock & ~(period - 1)) + period;
> + /*
> + * if the periodic timer's update is due to period re-configuration,
> + * we should count the clock since last interrupt.
> + */
> + if (old_period) {
> + int64_t last_periodic_clock;
> +
> + last_periodic_clock = muldiv64(s->next_periodic_time,
> + RTC_CLOCK_RATE, NANOSECONDS_PER_SECOND);
> + /*
> + * if the next interrupt has not happened yet, we recall the last
> + * interrupt based on the original period.
> + */
> + if (last_periodic_clock > cur_clock) {
> + last_periodic_clock -= period_code_to_clock(old_period);
This becomes "last_periodic_clock -= old_period".
But, I'm not sure how it can happen that last_periodic_clock <=
cur_clock. More precisely, if it happened, you have lost a tick and you
should add it to s->irq_coalesced it below. The simplest way to do it,
is to *always* do the subtraction.
> +
> + /* the last interrupt must have happened. */
> + assert(cur_clock >= last_periodic_clock);
> + }
> +
> + /* calculate the clock since last interrupt. */
> + lost_clock = cur_clock - last_periodic_clock;
> + }
I think the "assert(lost_clock >= 0)" should be here. Then you can also
remove the "the last interrupt must have happened" assertion, since the
two test exactly the same formula:
lost_clock >= 0
cur_clock - last_periodic_clock >= 0
cur_clock >= last_periodic_clock.
Putting everything together, this becomes:
if (!from_interrupt) {
int64_t next_periodic_clock, last_periodic_clock;
next_periodic_clock = muldiv64(s->next_periodic_time,
RTC_CLOCK_RATE,
NANOSECONDS_PER_SECOND);
last_periodic_clock = next_periodic_clock - old_period;
lost_clock = cur_clock - last_periodic_clock;
assert(lost_clock >= 0);
}
if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW) {
...
} else {
lost_clock = MIN(lost_clock, s->period);
}
assert(lost_clock >= 0 && lost_clock < s->period);
> + /*
> +#ifdef TARGET_I386
> + /*
> + * recalculate the coalesced irqs for two reasons:
> + * a) the lost_clock is more that a period, i,e. the timer
> + * interrupt has been lost, we should catch up the time.
> + *
> + * b) the period may be reconfigured, under this case, when
> + * switching from a shorter to a longer period, scale down
> + * the missing ticks since we expect the OS handler to
> + * treat the delayed ticks as longer. Any leftovers are
> + * put back into lost_clock.
> + * When switching to a shorter period, scale up the missing
> + * ticks since we expect the OS handler to treat the delayed
> + * ticks as shorter.
> + */
> + if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW) {
> + uint32_t cur_irq_coalesced = s->irq_coalesced;
> + uint32_t cur_period = s->period;
These are really "old" values rather than current.
> +
> + lost_clock += cur_irq_coalesced * cur_period;
> + s->irq_coalesced = lost_clock / period;
> + lost_clock %= period;
> + s->period = period;
> + if ((cur_irq_coalesced != s->irq_coalesced) ||
> + (cur_period |= s->period)) {
Typo here, so this becomes:
uint32_t old_irq_coalesced = s->irq_coalesced;
lost_clock += old_irq_coalesced * old_period;
s->irq_coalesced = lost_clock / s->period;
lost_clock %= s->period;
if (old_irq_coalesced != s->irq_coalesced ||
old_period != s->period) {
DPRINTF ...
rtc_coalesced_timer_update(s);
}
> + DPRINTF_C("cmos: coalesced irqs scaled from %d to %d, "
> + "period scaled from %d to %d\n", cur_irq_coalesced,
> + s->irq_coalesced, cur_period, s->period);
> + rtc_coalesced_timer_update(s);
> + }
> + }
> +#endif
> + /*
> + * no way to compensate the interrupt if LOST_TICK_POLICY_SLEW
> + * is not used, we should make the time progress anyway.
> + */
> + lost_clock = MIN(lost_clock, period);
See above---let's put it under "else", since it is unnecessary in the
SLEW case.
This is already much more understandable, and all of it makes a lot of
sense. The next version should be perfect. :)
Paolo
> + assert(lost_clock >= 0);
> +
> + next_irq_clock = cur_clock + period - lost_clock;
> s->next_periodic_time = muldiv64(next_irq_clock, NANOSECONDS_PER_SECOND,
> RTC_CLOCK_RATE) + 1;
> timer_mod(s->periodic_timer, s->next_periodic_time);
> @@ -186,7 +259,7 @@ static void rtc_periodic_timer(void *opaque)
> {
> RTCState *s = opaque;
>
> - periodic_timer_update(s, s->next_periodic_time);
> + periodic_timer_update(s, s->next_periodic_time, 0);
> 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;
> @@ -391,6 +464,7 @@ static void cmos_ioport_write(void *opaque, hwaddr addr,
> uint64_t data, unsigned size)
> {
> RTCState *s = opaque;
> + int cur_period;
> bool update_periodic_timer;
>
> if ((addr & 1) == 0) {
> @@ -424,6 +498,7 @@ static void cmos_ioport_write(void *opaque, hwaddr addr,
> }
> break;
> case RTC_REG_A:
> + cur_period = s->cmos_data[RTC_REG_A] & 0xf;
> update_periodic_timer = (s->cmos_data[RTC_REG_A] ^ data) & 0x0f;
>
> if ((data & 0x60) == 0x60) {
> @@ -450,7 +525,8 @@ static void cmos_ioport_write(void *opaque, hwaddr addr,
> (s->cmos_data[RTC_REG_A] & REG_A_UIP);
>
> if (update_periodic_timer) {
> - periodic_timer_update(s, qemu_clock_get_ns(rtc_clock));
> + periodic_timer_update(s, qemu_clock_get_ns(rtc_clock),
> + cur_period);
> }
>
> check_update_timer(s);
> @@ -487,7 +563,7 @@ static void cmos_ioport_write(void *opaque, hwaddr addr,
> s->cmos_data[RTC_REG_B] = data;
>
> if (update_periodic_timer) {
> - periodic_timer_update(s, qemu_clock_get_ns(rtc_clock));
> + periodic_timer_update(s, qemu_clock_get_ns(rtc_clock), 0);
> }
>
> check_update_timer(s);
> @@ -757,7 +833,7 @@ static int rtc_post_load(void *opaque, int version_id)
> uint64_t now = qemu_clock_get_ns(rtc_clock);
> if (now < s->next_periodic_time ||
> now > (s->next_periodic_time + get_max_clock_jump())) {
> - periodic_timer_update(s, qemu_clock_get_ns(rtc_clock));
> + periodic_timer_update(s, qemu_clock_get_ns(rtc_clock), 0);
> }
> }
>
> @@ -822,7 +898,7 @@ static void rtc_notify_clock_reset(Notifier *notifier, void *data)
> int64_t now = *(int64_t *)data;
>
> rtc_set_date_from_host(ISA_DEVICE(s));
> - periodic_timer_update(s, now);
> + periodic_timer_update(s, now, 0);
> check_update_timer(s);
> #ifdef TARGET_I386
> if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW) {
>
