| Message ID | cff0bcc8963e6b339716ad095a41687533cd64fd.1629799776.git.fthain@linux-m68k.org |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,01/10] hw/mos6522: Remove get_load_time() methods and functions | expand |
On 24/08/2021 11:09, Finn Thain wrote: > We rely on a QEMUTimer callback to set the interrupt flag, and this races > with counter register accesses, such that the guest might see the counter > reloaded but might not see the interrupt flagged. > > According to the datasheet, a real 6522 device counts down to FFFF, then > raises the relevant IRQ. After the FFFF count, the counter reloads from > the latch (for timer 1) or continues to decrement thru FFFE (for timer 2). > > Therefore, the guest operating system may read zero from T1CH and infer > that the counter has not yet wrapped (given another full count hasn't > yet elapsed.) > > Similarly, the guest may find the timer interrupt flag to be set and > infer that the counter is non-zero (given another full count hasn't yet > elapsed). > > Synchronize the timer counter and interrupt flag such that the guest will > observe the correct sequence of states. (It's still not right, because in > reality it's not possible to access the registers more than once per > "phase 2" clock cycle.) > > Eliminate the duplication of logic in get_counter() and > get_next_irq_time() by calling the former before the latter. > > Note that get_counter() is called prior to changing the latch. This is > because get_counter() may need to use the old latch value in order to > reload the counter. > > Signed-off-by: Finn Thain <fthain@linux-m68k.org> > --- > hw/misc/mos6522.c | 154 ++++++++++++++++++++------------------ > hw/misc/trace-events | 2 +- > include/hw/misc/mos6522.h | 8 +- > 3 files changed, 88 insertions(+), 76 deletions(-) > > diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c > index 23a440b64f..bd5df4963b 100644 > --- a/hw/misc/mos6522.c > +++ b/hw/misc/mos6522.c > @@ -52,26 +52,58 @@ static void mos6522_update_irq(MOS6522State *s) > } > } > > +static void mos6522_timer_raise_irq(MOS6522State *s, MOS6522Timer *ti) > +{ > + if (ti->state == irq) { > + return; > + } > + ti->state = irq; > + if (ti->index == 0) { > + s->ifr |= T1_INT; > + } else { > + s->ifr |= T2_INT; > + } > + mos6522_update_irq(s); > +} > + > static unsigned int get_counter(MOS6522State *s, MOS6522Timer *ti, int64_t now) > { > int64_t d; > unsigned int counter; > - > + bool reload; > + > + /* > + * Timer 1 counts down from the latch value to -1 (period of latch + 2), > + * then raises its interrupt and reloads. > + * Timer 2 counts down from the latch value to -1, then raises its > + * interrupt and continues to -2 and so on without any further interrupts. > + * (In reality, the first count should be measured from the falling edge > + * of the "phase two" clock, making its period N + 1.5. The subsequent > + * counts have period N + 2. This detail has been ignored here.) > + */ > d = muldiv64(now - ti->load_time, > ti->frequency, NANOSECONDS_PER_SECOND); > > - if (ti->index == 0) { > - /* the timer goes down from latch to -1 (period of latch + 2) */ > - if (d <= (ti->counter_value + 1)) { > - counter = ti->counter_value - d; > - } else { > - int64_t d_post_reload = d - (ti->counter_value + 2); > - /* XXX this calculation assumes that ti->latch has not changed */ > - counter = ti->latch - (d_post_reload % (ti->latch + 2)); > - } > - } else { > - counter = ti->counter_value - d; > + reload = (d >= ti->counter_value + 2); > + > + if (ti->index == 0 && reload) { > + int64_t more_reloads; > + > + d -= ti->counter_value + 2; > + more_reloads = d / (ti->latch + 2); > + d -= more_reloads * (ti->latch + 2); > + ti->load_time += muldiv64(ti->counter_value + 2 + > + more_reloads * (ti->latch + 2), > + NANOSECONDS_PER_SECOND, ti->frequency); > + ti->counter_value = ti->latch; > } > + > + counter = ti->counter_value - d; > + > + if (reload) { > + mos6522_timer_raise_irq(s, ti); > + } > + > return counter & 0xffff; > } > > @@ -80,7 +112,7 @@ static void set_counter(MOS6522State *s, MOS6522Timer *ti, unsigned int val) > trace_mos6522_set_counter(1 + ti->index, val); > ti->load_time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); > ti->counter_value = val; > - ti->oneshot_fired = false; > + ti->state = decrement; > if (ti->index == 0) { > mos6522_timer1_update(s, ti, ti->load_time); > } else { > @@ -91,38 +123,15 @@ static void set_counter(MOS6522State *s, MOS6522Timer *ti, unsigned int val) > static int64_t get_next_irq_time(MOS6522State *s, MOS6522Timer *ti, > int64_t now) > { > - int64_t d, next_time; > - unsigned int counter; > + int64_t next_time; > > if (ti->frequency == 0) { > return INT64_MAX; > } > > - /* current counter value */ > - d = muldiv64(now - ti->load_time, > - ti->frequency, NANOSECONDS_PER_SECOND); > - > - /* the timer goes down from latch to -1 (period of latch + 2) */ > - if (d <= (ti->counter_value + 1)) { > - counter = ti->counter_value - d; > - } else { > - int64_t d_post_reload = d - (ti->counter_value + 2); > - /* XXX this calculation assumes that ti->latch has not changed */ > - counter = ti->latch - (d_post_reload % (ti->latch + 2)); > - } > - counter &= 0xffff; > - > - /* Note: we consider the irq is raised on 0 */ > - if (counter == 0xffff) { > - next_time = d + ti->latch + 1; > - } else if (counter == 0) { > - next_time = d + ti->latch + 2; > - } else { > - next_time = d + counter; > - } > - trace_mos6522_get_next_irq_time(ti->latch, d, next_time - d); > - next_time = muldiv64(next_time, NANOSECONDS_PER_SECOND, ti->frequency) + > - ti->load_time; > + next_time = ti->load_time + muldiv64(ti->counter_value + 2, > + NANOSECONDS_PER_SECOND, ti->frequency); > + trace_mos6522_get_next_irq_time(ti->latch, ti->load_time, next_time); > return next_time; > } > > @@ -132,12 +141,10 @@ static void mos6522_timer1_update(MOS6522State *s, MOS6522Timer *ti, > if (!ti->timer) { > return; > } > + get_counter(s, ti, now); > ti->next_irq_time = get_next_irq_time(s, ti, now); > - if (ti->next_irq_time <= now) { > - ti->next_irq_time = now + 1; > - } > if ((s->ier & T1_INT) == 0 || > - ((s->acr & T1MODE) == T1MODE_ONESHOT && ti->oneshot_fired)) { > + ((s->acr & T1MODE) == T1MODE_ONESHOT && ti->state >= irq)) { > timer_del(ti->timer); > } else { > timer_mod(ti->timer, ti->next_irq_time); > @@ -150,11 +157,9 @@ static void mos6522_timer2_update(MOS6522State *s, MOS6522Timer *ti, > if (!ti->timer) { > return; > } > + get_counter(s, ti, now); > ti->next_irq_time = get_next_irq_time(s, ti, now); > - if (ti->next_irq_time <= now) { > - ti->next_irq_time = now + 1; > - } > - if ((s->ier & T2_INT) == 0 || (s->acr & T2MODE) || ti->oneshot_fired) { > + if ((s->ier & T2_INT) == 0 || (s->acr & T2MODE) || ti->state >= irq) { > timer_del(ti->timer); > } else { > timer_mod(ti->timer, ti->next_irq_time); > @@ -167,10 +172,7 @@ static void mos6522_timer1_expired(void *opaque) > MOS6522Timer *ti = &s->timers[0]; > int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); > > - ti->oneshot_fired = true; > mos6522_timer1_update(s, ti, now); > - s->ifr |= T1_INT; > - mos6522_update_irq(s); > } > > static void mos6522_timer2_expired(void *opaque) > @@ -179,10 +181,7 @@ static void mos6522_timer2_expired(void *opaque) > MOS6522Timer *ti = &s->timers[1]; > int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); > > - ti->oneshot_fired = true; > mos6522_timer2_update(s, ti, now); > - s->ifr |= T2_INT; > - mos6522_update_irq(s); > } > > static void mos6522_set_sr_int(MOS6522State *s) > @@ -208,18 +207,6 @@ uint64_t mos6522_read(void *opaque, hwaddr addr, unsigned size) > uint32_t val; > int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); > > - if (now >= s->timers[0].next_irq_time) { > - s->timers[0].oneshot_fired = true; > - mos6522_timer1_update(s, &s->timers[0], now); > - s->ifr |= T1_INT; > - mos6522_update_irq(s); > - } > - if (now >= s->timers[1].next_irq_time) { > - s->timers[1].oneshot_fired = true; > - mos6522_timer2_update(s, &s->timers[1], now); > - s->ifr |= T2_INT; > - mos6522_update_irq(s); > - } > switch (addr) { > case VIA_REG_B: > val = s->b; > @@ -238,8 +225,11 @@ uint64_t mos6522_read(void *opaque, hwaddr addr, unsigned size) > break; > case VIA_REG_T1CL: > val = get_counter(s, &s->timers[0], now) & 0xff; > - s->ifr &= ~T1_INT; > - mos6522_update_irq(s); > + if (s->timers[0].state >= irq) { > + s->timers[0].state = irq_cleared; > + s->ifr &= ~T1_INT; > + mos6522_update_irq(s); > + } > break; > case VIA_REG_T1CH: > val = get_counter(s, &s->timers[0], now) >> 8; > @@ -252,8 +242,11 @@ uint64_t mos6522_read(void *opaque, hwaddr addr, unsigned size) > break; > case VIA_REG_T2CL: > val = get_counter(s, &s->timers[1], now) & 0xff; > - s->ifr &= ~T2_INT; > - mos6522_update_irq(s); > + if (s->timers[1].state >= irq) { > + s->timers[1].state = irq_cleared; > + s->ifr &= ~T2_INT; > + mos6522_update_irq(s); > + } > break; > case VIA_REG_T2CH: > val = get_counter(s, &s->timers[1], now) >> 8; > @@ -293,7 +286,7 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size) > { > MOS6522State *s = opaque; > MOS6522DeviceClass *mdc = MOS6522_GET_CLASS(s); > - int64_t now; > + int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); > > trace_mos6522_write(addr, val); > > @@ -316,6 +309,7 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size) > s->dira = val; > break; > case VIA_REG_T1CL: > + get_counter(s, &s->timers[0], now); > s->timers[0].latch = (s->timers[0].latch & 0xff00) | val; > break; > case VIA_REG_T1CH: > @@ -324,12 +318,15 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size) > set_counter(s, &s->timers[0], s->timers[0].latch); > break; > case VIA_REG_T1LL: > + get_counter(s, &s->timers[0], now); > s->timers[0].latch = (s->timers[0].latch & 0xff00) | val; > break; > case VIA_REG_T1LH: > + get_counter(s, &s->timers[0], now); > s->timers[0].latch = (s->timers[0].latch & 0xff) | (val << 8); > break; > case VIA_REG_T2CL: > + get_counter(s, &s->timers[1], now); > s->timers[1].latch = (s->timers[1].latch & 0xff00) | val; > break; > case VIA_REG_T2CH: > @@ -342,7 +339,6 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size) > break; > case VIA_REG_ACR: > s->acr = val; > - now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); > mos6522_timer1_update(s, &s->timers[0], now); > mos6522_timer2_update(s, &s->timers[1], now); > break; > @@ -350,7 +346,18 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size) > s->pcr = val; > break; > case VIA_REG_IFR: > - /* reset bits */ > + if (val & T1_INT) { > + get_counter(s, &s->timers[0], now); > + if ((s->ifr & T1_INT) && s->timers[0].state == irq) { > + s->timers[0].state = irq_cleared; > + } > + } > + if (val & T2_INT) { > + get_counter(s, &s->timers[1], now); > + if ((s->ifr & T2_INT) && s->timers[1].state == irq) { > + s->timers[1].state = irq_cleared; > + } > + } > s->ifr &= ~val; > mos6522_update_irq(s); > break; > @@ -364,7 +371,6 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size) > } > mos6522_update_irq(s); > /* if IER is modified starts needed timers */ > - now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); > mos6522_timer1_update(s, &s->timers[0], now); > mos6522_timer2_update(s, &s->timers[1], now); > break; > diff --git a/hw/misc/trace-events b/hw/misc/trace-events > index d0a89eb059..6c1bb02150 100644 > --- a/hw/misc/trace-events > +++ b/hw/misc/trace-events > @@ -103,7 +103,7 @@ imx7_gpr_write(uint64_t offset, uint64_t value) "addr 0x%08" PRIx64 "value 0x%08 > > # mos6522.c > mos6522_set_counter(int index, unsigned int val) "T%d.counter=%d" > -mos6522_get_next_irq_time(uint16_t latch, int64_t d, int64_t delta) "latch=%d counter=0x%"PRId64 " delta_next=0x%"PRId64 > +mos6522_get_next_irq_time(uint16_t latch, int64_t load_time, int64_t next_time) "latch=%d counter=%" PRId64 " next_time=%" PRId64 > mos6522_set_sr_int(void) "set sr_int" > mos6522_write(uint64_t addr, uint64_t val) "reg=0x%"PRIx64 " val=0x%"PRIx64 > mos6522_read(uint64_t addr, unsigned val) "reg=0x%"PRIx64 " val=0x%x" > diff --git a/include/hw/misc/mos6522.h b/include/hw/misc/mos6522.h > index 94b1dc324c..4dbba6b273 100644 > --- a/include/hw/misc/mos6522.h > +++ b/include/hw/misc/mos6522.h > @@ -73,6 +73,12 @@ > #define VIA_REG_IER 0x0e > #define VIA_REG_ANH 0x0f > > +enum timer_state { > + decrement, > + irq, > + irq_cleared, > +}; > + > /** > * MOS6522Timer: > * @counter_value: counter value at load time > @@ -85,7 +91,7 @@ typedef struct MOS6522Timer { > int64_t next_irq_time; > uint64_t frequency; > QEMUTimer *timer; > - bool oneshot_fired; > + enum timer_state state; > } MOS6522Timer; > > /** Unfortunately the datasheet I was using for reference doesn't appear to have the relevant detail here. Have you got a reference to the datasheet you're using which shows what happens to the timers at the zero crossing point? ATB, Mark.
On Wed, 25 Aug 2021, Mark Cave-Ayland wrote: > > Unfortunately the datasheet I was using for reference doesn't appear to > have the relevant detail here. Have you got a reference to the datasheet > you're using which shows what happens to the timers at the zero crossing > point? > The datasheets which I normally refer to (Rockwell and Synertek) agree about this. Also, I established the sequence experimentally when I was writing the clocksource drivers for Linux/m68k.
diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c index 23a440b64f..bd5df4963b 100644 --- a/hw/misc/mos6522.c +++ b/hw/misc/mos6522.c @@ -52,26 +52,58 @@ static void mos6522_update_irq(MOS6522State *s) } } +static void mos6522_timer_raise_irq(MOS6522State *s, MOS6522Timer *ti) +{ + if (ti->state == irq) { + return; + } + ti->state = irq; + if (ti->index == 0) { + s->ifr |= T1_INT; + } else { + s->ifr |= T2_INT; + } + mos6522_update_irq(s); +} + static unsigned int get_counter(MOS6522State *s, MOS6522Timer *ti, int64_t now) { int64_t d; unsigned int counter; - + bool reload; + + /* + * Timer 1 counts down from the latch value to -1 (period of latch + 2), + * then raises its interrupt and reloads. + * Timer 2 counts down from the latch value to -1, then raises its + * interrupt and continues to -2 and so on without any further interrupts. + * (In reality, the first count should be measured from the falling edge + * of the "phase two" clock, making its period N + 1.5. The subsequent + * counts have period N + 2. This detail has been ignored here.) + */ d = muldiv64(now - ti->load_time, ti->frequency, NANOSECONDS_PER_SECOND); - if (ti->index == 0) { - /* the timer goes down from latch to -1 (period of latch + 2) */ - if (d <= (ti->counter_value + 1)) { - counter = ti->counter_value - d; - } else { - int64_t d_post_reload = d - (ti->counter_value + 2); - /* XXX this calculation assumes that ti->latch has not changed */ - counter = ti->latch - (d_post_reload % (ti->latch + 2)); - } - } else { - counter = ti->counter_value - d; + reload = (d >= ti->counter_value + 2); + + if (ti->index == 0 && reload) { + int64_t more_reloads; + + d -= ti->counter_value + 2; + more_reloads = d / (ti->latch + 2); + d -= more_reloads * (ti->latch + 2); + ti->load_time += muldiv64(ti->counter_value + 2 + + more_reloads * (ti->latch + 2), + NANOSECONDS_PER_SECOND, ti->frequency); + ti->counter_value = ti->latch; } + + counter = ti->counter_value - d; + + if (reload) { + mos6522_timer_raise_irq(s, ti); + } + return counter & 0xffff; } @@ -80,7 +112,7 @@ static void set_counter(MOS6522State *s, MOS6522Timer *ti, unsigned int val) trace_mos6522_set_counter(1 + ti->index, val); ti->load_time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); ti->counter_value = val; - ti->oneshot_fired = false; + ti->state = decrement; if (ti->index == 0) { mos6522_timer1_update(s, ti, ti->load_time); } else { @@ -91,38 +123,15 @@ static void set_counter(MOS6522State *s, MOS6522Timer *ti, unsigned int val) static int64_t get_next_irq_time(MOS6522State *s, MOS6522Timer *ti, int64_t now) { - int64_t d, next_time; - unsigned int counter; + int64_t next_time; if (ti->frequency == 0) { return INT64_MAX; } - /* current counter value */ - d = muldiv64(now - ti->load_time, - ti->frequency, NANOSECONDS_PER_SECOND); - - /* the timer goes down from latch to -1 (period of latch + 2) */ - if (d <= (ti->counter_value + 1)) { - counter = ti->counter_value - d; - } else { - int64_t d_post_reload = d - (ti->counter_value + 2); - /* XXX this calculation assumes that ti->latch has not changed */ - counter = ti->latch - (d_post_reload % (ti->latch + 2)); - } - counter &= 0xffff; - - /* Note: we consider the irq is raised on 0 */ - if (counter == 0xffff) { - next_time = d + ti->latch + 1; - } else if (counter == 0) { - next_time = d + ti->latch + 2; - } else { - next_time = d + counter; - } - trace_mos6522_get_next_irq_time(ti->latch, d, next_time - d); - next_time = muldiv64(next_time, NANOSECONDS_PER_SECOND, ti->frequency) + - ti->load_time; + next_time = ti->load_time + muldiv64(ti->counter_value + 2, + NANOSECONDS_PER_SECOND, ti->frequency); + trace_mos6522_get_next_irq_time(ti->latch, ti->load_time, next_time); return next_time; } @@ -132,12 +141,10 @@ static void mos6522_timer1_update(MOS6522State *s, MOS6522Timer *ti, if (!ti->timer) { return; } + get_counter(s, ti, now); ti->next_irq_time = get_next_irq_time(s, ti, now); - if (ti->next_irq_time <= now) { - ti->next_irq_time = now + 1; - } if ((s->ier & T1_INT) == 0 || - ((s->acr & T1MODE) == T1MODE_ONESHOT && ti->oneshot_fired)) { + ((s->acr & T1MODE) == T1MODE_ONESHOT && ti->state >= irq)) { timer_del(ti->timer); } else { timer_mod(ti->timer, ti->next_irq_time); @@ -150,11 +157,9 @@ static void mos6522_timer2_update(MOS6522State *s, MOS6522Timer *ti, if (!ti->timer) { return; } + get_counter(s, ti, now); ti->next_irq_time = get_next_irq_time(s, ti, now); - if (ti->next_irq_time <= now) { - ti->next_irq_time = now + 1; - } - if ((s->ier & T2_INT) == 0 || (s->acr & T2MODE) || ti->oneshot_fired) { + if ((s->ier & T2_INT) == 0 || (s->acr & T2MODE) || ti->state >= irq) { timer_del(ti->timer); } else { timer_mod(ti->timer, ti->next_irq_time); @@ -167,10 +172,7 @@ static void mos6522_timer1_expired(void *opaque) MOS6522Timer *ti = &s->timers[0]; int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); - ti->oneshot_fired = true; mos6522_timer1_update(s, ti, now); - s->ifr |= T1_INT; - mos6522_update_irq(s); } static void mos6522_timer2_expired(void *opaque) @@ -179,10 +181,7 @@ static void mos6522_timer2_expired(void *opaque) MOS6522Timer *ti = &s->timers[1]; int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); - ti->oneshot_fired = true; mos6522_timer2_update(s, ti, now); - s->ifr |= T2_INT; - mos6522_update_irq(s); } static void mos6522_set_sr_int(MOS6522State *s) @@ -208,18 +207,6 @@ uint64_t mos6522_read(void *opaque, hwaddr addr, unsigned size) uint32_t val; int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); - if (now >= s->timers[0].next_irq_time) { - s->timers[0].oneshot_fired = true; - mos6522_timer1_update(s, &s->timers[0], now); - s->ifr |= T1_INT; - mos6522_update_irq(s); - } - if (now >= s->timers[1].next_irq_time) { - s->timers[1].oneshot_fired = true; - mos6522_timer2_update(s, &s->timers[1], now); - s->ifr |= T2_INT; - mos6522_update_irq(s); - } switch (addr) { case VIA_REG_B: val = s->b; @@ -238,8 +225,11 @@ uint64_t mos6522_read(void *opaque, hwaddr addr, unsigned size) break; case VIA_REG_T1CL: val = get_counter(s, &s->timers[0], now) & 0xff; - s->ifr &= ~T1_INT; - mos6522_update_irq(s); + if (s->timers[0].state >= irq) { + s->timers[0].state = irq_cleared; + s->ifr &= ~T1_INT; + mos6522_update_irq(s); + } break; case VIA_REG_T1CH: val = get_counter(s, &s->timers[0], now) >> 8; @@ -252,8 +242,11 @@ uint64_t mos6522_read(void *opaque, hwaddr addr, unsigned size) break; case VIA_REG_T2CL: val = get_counter(s, &s->timers[1], now) & 0xff; - s->ifr &= ~T2_INT; - mos6522_update_irq(s); + if (s->timers[1].state >= irq) { + s->timers[1].state = irq_cleared; + s->ifr &= ~T2_INT; + mos6522_update_irq(s); + } break; case VIA_REG_T2CH: val = get_counter(s, &s->timers[1], now) >> 8; @@ -293,7 +286,7 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size) { MOS6522State *s = opaque; MOS6522DeviceClass *mdc = MOS6522_GET_CLASS(s); - int64_t now; + int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); trace_mos6522_write(addr, val); @@ -316,6 +309,7 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size) s->dira = val; break; case VIA_REG_T1CL: + get_counter(s, &s->timers[0], now); s->timers[0].latch = (s->timers[0].latch & 0xff00) | val; break; case VIA_REG_T1CH: @@ -324,12 +318,15 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size) set_counter(s, &s->timers[0], s->timers[0].latch); break; case VIA_REG_T1LL: + get_counter(s, &s->timers[0], now); s->timers[0].latch = (s->timers[0].latch & 0xff00) | val; break; case VIA_REG_T1LH: + get_counter(s, &s->timers[0], now); s->timers[0].latch = (s->timers[0].latch & 0xff) | (val << 8); break; case VIA_REG_T2CL: + get_counter(s, &s->timers[1], now); s->timers[1].latch = (s->timers[1].latch & 0xff00) | val; break; case VIA_REG_T2CH: @@ -342,7 +339,6 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size) break; case VIA_REG_ACR: s->acr = val; - now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); mos6522_timer1_update(s, &s->timers[0], now); mos6522_timer2_update(s, &s->timers[1], now); break; @@ -350,7 +346,18 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size) s->pcr = val; break; case VIA_REG_IFR: - /* reset bits */ + if (val & T1_INT) { + get_counter(s, &s->timers[0], now); + if ((s->ifr & T1_INT) && s->timers[0].state == irq) { + s->timers[0].state = irq_cleared; + } + } + if (val & T2_INT) { + get_counter(s, &s->timers[1], now); + if ((s->ifr & T2_INT) && s->timers[1].state == irq) { + s->timers[1].state = irq_cleared; + } + } s->ifr &= ~val; mos6522_update_irq(s); break; @@ -364,7 +371,6 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size) } mos6522_update_irq(s); /* if IER is modified starts needed timers */ - now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); mos6522_timer1_update(s, &s->timers[0], now); mos6522_timer2_update(s, &s->timers[1], now); break; diff --git a/hw/misc/trace-events b/hw/misc/trace-events index d0a89eb059..6c1bb02150 100644 --- a/hw/misc/trace-events +++ b/hw/misc/trace-events @@ -103,7 +103,7 @@ imx7_gpr_write(uint64_t offset, uint64_t value) "addr 0x%08" PRIx64 "value 0x%08 # mos6522.c mos6522_set_counter(int index, unsigned int val) "T%d.counter=%d" -mos6522_get_next_irq_time(uint16_t latch, int64_t d, int64_t delta) "latch=%d counter=0x%"PRId64 " delta_next=0x%"PRId64 +mos6522_get_next_irq_time(uint16_t latch, int64_t load_time, int64_t next_time) "latch=%d counter=%" PRId64 " next_time=%" PRId64 mos6522_set_sr_int(void) "set sr_int" mos6522_write(uint64_t addr, uint64_t val) "reg=0x%"PRIx64 " val=0x%"PRIx64 mos6522_read(uint64_t addr, unsigned val) "reg=0x%"PRIx64 " val=0x%x" diff --git a/include/hw/misc/mos6522.h b/include/hw/misc/mos6522.h index 94b1dc324c..4dbba6b273 100644 --- a/include/hw/misc/mos6522.h +++ b/include/hw/misc/mos6522.h @@ -73,6 +73,12 @@ #define VIA_REG_IER 0x0e #define VIA_REG_ANH 0x0f +enum timer_state { + decrement, + irq, + irq_cleared, +}; + /** * MOS6522Timer: * @counter_value: counter value at load time @@ -85,7 +91,7 @@ typedef struct MOS6522Timer { int64_t next_irq_time; uint64_t frequency; QEMUTimer *timer; - bool oneshot_fired; + enum timer_state state; } MOS6522Timer; /**
We rely on a QEMUTimer callback to set the interrupt flag, and this races with counter register accesses, such that the guest might see the counter reloaded but might not see the interrupt flagged. According to the datasheet, a real 6522 device counts down to FFFF, then raises the relevant IRQ. After the FFFF count, the counter reloads from the latch (for timer 1) or continues to decrement thru FFFE (for timer 2). Therefore, the guest operating system may read zero from T1CH and infer that the counter has not yet wrapped (given another full count hasn't yet elapsed.) Similarly, the guest may find the timer interrupt flag to be set and infer that the counter is non-zero (given another full count hasn't yet elapsed). Synchronize the timer counter and interrupt flag such that the guest will observe the correct sequence of states. (It's still not right, because in reality it's not possible to access the registers more than once per "phase 2" clock cycle.) Eliminate the duplication of logic in get_counter() and get_next_irq_time() by calling the former before the latter. Note that get_counter() is called prior to changing the latch. This is because get_counter() may need to use the old latch value in order to reload the counter. Signed-off-by: Finn Thain <fthain@linux-m68k.org> --- hw/misc/mos6522.c | 154 ++++++++++++++++++++------------------ hw/misc/trace-events | 2 +- include/hw/misc/mos6522.h | 8 +- 3 files changed, 88 insertions(+), 76 deletions(-)