* [RFC 09/10] hw/mos6522: Avoid using discrepant QEMU clock values
2021-08-24 10:09 [RFC 00/10] hw/mos6522: VIA timer emulation fixes and improvements Finn Thain
@ 2021-08-24 10:09 ` Finn Thain
2021-08-24 10:28 ` Philippe Mathieu-Daudé
2021-08-25 8:44 ` Mark Cave-Ayland
2021-08-24 10:09 ` [RFC 06/10] hw/mos6522: Implement oneshot mode Finn Thain
` (11 subsequent siblings)
12 siblings, 2 replies; 47+ messages in thread
From: Finn Thain @ 2021-08-24 10:09 UTC (permalink / raw)
To: Mark Cave-Ayland, David Gibson, Greg Kurz
Cc: qemu-ppc, Laurent Vivier, qemu-devel
mos6522_read() and mos6522_write() may call various functions to determine
timer irq state, timer counter value and QEMUTimer deadline. All called
functions must use the same value for the present time.
Signed-off-by: Finn Thain <fthain@linux-m68k.org>
---
hw/misc/mos6522.c | 51 +++++++++++++++++++++++++----------------------
1 file changed, 27 insertions(+), 24 deletions(-)
diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
index 0dd3ccf945..23a440b64f 100644
--- a/hw/misc/mos6522.c
+++ b/hw/misc/mos6522.c
@@ -39,9 +39,9 @@
/* XXX: implement all timer modes */
static void mos6522_timer1_update(MOS6522State *s, MOS6522Timer *ti,
- int64_t current_time);
+ int64_t now);
static void mos6522_timer2_update(MOS6522State *s, MOS6522Timer *ti,
- int64_t current_time);
+ int64_t now);
static void mos6522_update_irq(MOS6522State *s)
{
@@ -52,12 +52,12 @@ static void mos6522_update_irq(MOS6522State *s)
}
}
-static unsigned int get_counter(MOS6522State *s, MOS6522Timer *ti)
+static unsigned int get_counter(MOS6522State *s, MOS6522Timer *ti, int64_t now)
{
int64_t d;
unsigned int counter;
- d = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) - ti->load_time,
+ d = muldiv64(now - ti->load_time,
ti->frequency, NANOSECONDS_PER_SECOND);
if (ti->index == 0) {
@@ -89,7 +89,7 @@ static void set_counter(MOS6522State *s, MOS6522Timer *ti, unsigned int val)
}
static int64_t get_next_irq_time(MOS6522State *s, MOS6522Timer *ti,
- int64_t current_time)
+ int64_t now)
{
int64_t d, next_time;
unsigned int counter;
@@ -99,7 +99,7 @@ static int64_t get_next_irq_time(MOS6522State *s, MOS6522Timer *ti,
}
/* current counter value */
- d = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) - ti->load_time,
+ d = muldiv64(now - ti->load_time,
ti->frequency, NANOSECONDS_PER_SECOND);
/* the timer goes down from latch to -1 (period of latch + 2) */
@@ -123,20 +123,19 @@ static int64_t get_next_irq_time(MOS6522State *s, MOS6522Timer *ti,
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;
-
- if (next_time <= current_time) {
- next_time = current_time + 1;
- }
return next_time;
}
static void mos6522_timer1_update(MOS6522State *s, MOS6522Timer *ti,
- int64_t current_time)
+ int64_t now)
{
if (!ti->timer) {
return;
}
- ti->next_irq_time = get_next_irq_time(s, ti, current_time);
+ 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)) {
timer_del(ti->timer);
@@ -146,12 +145,15 @@ static void mos6522_timer1_update(MOS6522State *s, MOS6522Timer *ti,
}
static void mos6522_timer2_update(MOS6522State *s, MOS6522Timer *ti,
- int64_t current_time)
+ int64_t now)
{
if (!ti->timer) {
return;
}
- ti->next_irq_time = get_next_irq_time(s, ti, current_time);
+ 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) {
timer_del(ti->timer);
} else {
@@ -163,9 +165,10 @@ static void mos6522_timer1_expired(void *opaque)
{
MOS6522State *s = 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, ti->next_irq_time);
+ mos6522_timer1_update(s, ti, now);
s->ifr |= T1_INT;
mos6522_update_irq(s);
}
@@ -174,9 +177,10 @@ static void mos6522_timer2_expired(void *opaque)
{
MOS6522State *s = 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, ti->next_irq_time);
+ mos6522_timer2_update(s, ti, now);
s->ifr |= T2_INT;
mos6522_update_irq(s);
}
@@ -233,12 +237,12 @@ uint64_t mos6522_read(void *opaque, hwaddr addr, unsigned size)
val = s->dira;
break;
case VIA_REG_T1CL:
- val = get_counter(s, &s->timers[0]) & 0xff;
+ val = get_counter(s, &s->timers[0], now) & 0xff;
s->ifr &= ~T1_INT;
mos6522_update_irq(s);
break;
case VIA_REG_T1CH:
- val = get_counter(s, &s->timers[0]) >> 8;
+ val = get_counter(s, &s->timers[0], now) >> 8;
break;
case VIA_REG_T1LL:
val = s->timers[0].latch & 0xff;
@@ -247,12 +251,12 @@ uint64_t mos6522_read(void *opaque, hwaddr addr, unsigned size)
val = (s->timers[0].latch >> 8) & 0xff;
break;
case VIA_REG_T2CL:
- val = get_counter(s, &s->timers[1]) & 0xff;
+ val = get_counter(s, &s->timers[1], now) & 0xff;
s->ifr &= ~T2_INT;
mos6522_update_irq(s);
break;
case VIA_REG_T2CH:
- val = get_counter(s, &s->timers[1]) >> 8;
+ val = get_counter(s, &s->timers[1], now) >> 8;
break;
case VIA_REG_SR:
val = s->sr;
@@ -360,10 +364,9 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
}
mos6522_update_irq(s);
/* if IER is modified starts needed timers */
- mos6522_timer1_update(s, &s->timers[0],
- qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
- mos6522_timer2_update(s, &s->timers[1],
- qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
+ 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;
default:
g_assert_not_reached();
--
2.26.3
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [RFC 09/10] hw/mos6522: Avoid using discrepant QEMU clock values
2021-08-24 10:09 ` [RFC 09/10] hw/mos6522: Avoid using discrepant QEMU clock values Finn Thain
@ 2021-08-24 10:28 ` Philippe Mathieu-Daudé
2021-08-29 1:23 ` Finn Thain
2021-08-25 8:44 ` Mark Cave-Ayland
1 sibling, 1 reply; 47+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-08-24 10:28 UTC (permalink / raw)
To: Finn Thain, Mark Cave-Ayland, David Gibson, Greg Kurz
Cc: qemu-ppc, Laurent Vivier, qemu-devel
On 8/24/21 12:09 PM, Finn Thain wrote:
> mos6522_read() and mos6522_write() may call various functions to determine
> timer irq state, timer counter value and QEMUTimer deadline. All called
> functions must use the same value for the present time.
>
> Signed-off-by: Finn Thain <fthain@linux-m68k.org>
> ---
> hw/misc/mos6522.c | 51 +++++++++++++++++++++++++----------------------
> 1 file changed, 27 insertions(+), 24 deletions(-)
> @@ -123,20 +123,19 @@ static int64_t get_next_irq_time(MOS6522State *s, MOS6522Timer *ti,
> 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;
> -
> - if (next_time <= current_time) {
> - next_time = current_time + 1;
> - }
Maybe extract as an helper? Otherwise:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> return next_time;
> }
>
> static void mos6522_timer1_update(MOS6522State *s, MOS6522Timer *ti,
> - int64_t current_time)
> + int64_t now)
> {
> if (!ti->timer) {
> return;
> }
> - ti->next_irq_time = get_next_irq_time(s, ti, current_time);
> + 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)) {
> timer_del(ti->timer);
> @@ -146,12 +145,15 @@ static void mos6522_timer1_update(MOS6522State *s, MOS6522Timer *ti,
> }
>
> static void mos6522_timer2_update(MOS6522State *s, MOS6522Timer *ti,
> - int64_t current_time)
> + int64_t now)
> {
> if (!ti->timer) {
> return;
> }
> - ti->next_irq_time = get_next_irq_time(s, ti, current_time);
> + ti->next_irq_time = get_next_irq_time(s, ti, now);
> + if (ti->next_irq_time <= now) {
> + ti->next_irq_time = now + 1;
> + }
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [RFC 09/10] hw/mos6522: Avoid using discrepant QEMU clock values
2021-08-24 10:28 ` Philippe Mathieu-Daudé
@ 2021-08-29 1:23 ` Finn Thain
0 siblings, 0 replies; 47+ messages in thread
From: Finn Thain @ 2021-08-29 1:23 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Mark Cave-Ayland, qemu-devel, Laurent Vivier, Greg Kurz,
qemu-ppc, David Gibson
[-- Attachment #1: Type: text/plain, Size: 2423 bytes --]
On Tue, 24 Aug 2021, Philippe Mathieu-Daudé wrote:
> On 8/24/21 12:09 PM, Finn Thain wrote:
> > mos6522_read() and mos6522_write() may call various functions to determine
> > timer irq state, timer counter value and QEMUTimer deadline. All called
> > functions must use the same value for the present time.
> >
> > Signed-off-by: Finn Thain <fthain@linux-m68k.org>
> > ---
> > hw/misc/mos6522.c | 51 +++++++++++++++++++++++++----------------------
> > 1 file changed, 27 insertions(+), 24 deletions(-)
>
> > @@ -123,20 +123,19 @@ static int64_t get_next_irq_time(MOS6522State *s, MOS6522Timer *ti,
> > 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;
> > -
> > - if (next_time <= current_time) {
> > - next_time = current_time + 1;
> > - }
>
> Maybe extract as an helper?
There is a small amount of code duplication here but it gets resolved in
patch 10/10.
> Otherwise:
>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
> > return next_time;
> > }
> >
> > static void mos6522_timer1_update(MOS6522State *s, MOS6522Timer *ti,
> > - int64_t current_time)
> > + int64_t now)
> > {
> > if (!ti->timer) {
> > return;
> > }
> > - ti->next_irq_time = get_next_irq_time(s, ti, current_time);
> > + 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)) {
> > timer_del(ti->timer);
> > @@ -146,12 +145,15 @@ static void mos6522_timer1_update(MOS6522State *s, MOS6522Timer *ti,
> > }
> >
> > static void mos6522_timer2_update(MOS6522State *s, MOS6522Timer *ti,
> > - int64_t current_time)
> > + int64_t now)
> > {
> > if (!ti->timer) {
> > return;
> > }
> > - ti->next_irq_time = get_next_irq_time(s, ti, current_time);
> > + ti->next_irq_time = get_next_irq_time(s, ti, now);
> > + if (ti->next_irq_time <= now) {
> > + ti->next_irq_time = now + 1;
> > + }
>
>
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC 09/10] hw/mos6522: Avoid using discrepant QEMU clock values
2021-08-24 10:09 ` [RFC 09/10] hw/mos6522: Avoid using discrepant QEMU clock values Finn Thain
2021-08-24 10:28 ` Philippe Mathieu-Daudé
@ 2021-08-25 8:44 ` Mark Cave-Ayland
2021-08-29 1:55 ` Finn Thain
1 sibling, 1 reply; 47+ messages in thread
From: Mark Cave-Ayland @ 2021-08-25 8:44 UTC (permalink / raw)
To: Finn Thain, David Gibson, Greg Kurz; +Cc: qemu-ppc, Laurent Vivier, qemu-devel
On 24/08/2021 11:09, Finn Thain wrote:
> mos6522_read() and mos6522_write() may call various functions to determine
> timer irq state, timer counter value and QEMUTimer deadline. All called
> functions must use the same value for the present time.
>
> Signed-off-by: Finn Thain <fthain@linux-m68k.org>
> ---
> hw/misc/mos6522.c | 51 +++++++++++++++++++++++++----------------------
> 1 file changed, 27 insertions(+), 24 deletions(-)
>
> diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
> index 0dd3ccf945..23a440b64f 100644
> --- a/hw/misc/mos6522.c
> +++ b/hw/misc/mos6522.c
> @@ -39,9 +39,9 @@
> /* XXX: implement all timer modes */
>
> static void mos6522_timer1_update(MOS6522State *s, MOS6522Timer *ti,
> - int64_t current_time);
> + int64_t now);
> static void mos6522_timer2_update(MOS6522State *s, MOS6522Timer *ti,
> - int64_t current_time);
> + int64_t now);
>
> static void mos6522_update_irq(MOS6522State *s)
> {
> @@ -52,12 +52,12 @@ static void mos6522_update_irq(MOS6522State *s)
> }
> }
>
> -static unsigned int get_counter(MOS6522State *s, MOS6522Timer *ti)
> +static unsigned int get_counter(MOS6522State *s, MOS6522Timer *ti, int64_t now)
> {
> int64_t d;
> unsigned int counter;
>
> - d = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) - ti->load_time,
> + d = muldiv64(now - ti->load_time,
> ti->frequency, NANOSECONDS_PER_SECOND);
>
> if (ti->index == 0) {
> @@ -89,7 +89,7 @@ static void set_counter(MOS6522State *s, MOS6522Timer *ti, unsigned int val)
> }
>
> static int64_t get_next_irq_time(MOS6522State *s, MOS6522Timer *ti,
> - int64_t current_time)
> + int64_t now)
> {
> int64_t d, next_time;
> unsigned int counter;
> @@ -99,7 +99,7 @@ static int64_t get_next_irq_time(MOS6522State *s, MOS6522Timer *ti,
> }
>
> /* current counter value */
> - d = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) - ti->load_time,
> + d = muldiv64(now - ti->load_time,
> ti->frequency, NANOSECONDS_PER_SECOND);
>
> /* the timer goes down from latch to -1 (period of latch + 2) */
> @@ -123,20 +123,19 @@ static int64_t get_next_irq_time(MOS6522State *s, MOS6522Timer *ti,
> 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;
> -
> - if (next_time <= current_time) {
> - next_time = current_time + 1;
> - }
> return next_time;
> }
>
> static void mos6522_timer1_update(MOS6522State *s, MOS6522Timer *ti,
> - int64_t current_time)
> + int64_t now)
> {
> if (!ti->timer) {
> return;
> }
> - ti->next_irq_time = get_next_irq_time(s, ti, current_time);
> + 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)) {
> timer_del(ti->timer);
> @@ -146,12 +145,15 @@ static void mos6522_timer1_update(MOS6522State *s, MOS6522Timer *ti,
> }
>
> static void mos6522_timer2_update(MOS6522State *s, MOS6522Timer *ti,
> - int64_t current_time)
> + int64_t now)
> {
> if (!ti->timer) {
> return;
> }
> - ti->next_irq_time = get_next_irq_time(s, ti, current_time);
> + 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) {
> timer_del(ti->timer);
> } else {
> @@ -163,9 +165,10 @@ static void mos6522_timer1_expired(void *opaque)
> {
> MOS6522State *s = 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, ti->next_irq_time);
> + mos6522_timer1_update(s, ti, now);
Presumably using ti->next_irq_time has already fixed the current time to be that at
which the timer routine actually expired, rather than the current executing time. Are
you seeing large differences in these numbers that can cause timer drift? If so, I'm
wondering if this change should be in a separate patch.
> s->ifr |= T1_INT;
> mos6522_update_irq(s);
> }
> @@ -174,9 +177,10 @@ static void mos6522_timer2_expired(void *opaque)
> {
> MOS6522State *s = 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, ti->next_irq_time);
> + mos6522_timer2_update(s, ti, now);
And same again here.
> s->ifr |= T2_INT;
> mos6522_update_irq(s);
> }
> @@ -233,12 +237,12 @@ uint64_t mos6522_read(void *opaque, hwaddr addr, unsigned size)
> val = s->dira;
> break;
> case VIA_REG_T1CL:
> - val = get_counter(s, &s->timers[0]) & 0xff;
> + val = get_counter(s, &s->timers[0], now) & 0xff;
> s->ifr &= ~T1_INT;
> mos6522_update_irq(s);
> break;
> case VIA_REG_T1CH:
> - val = get_counter(s, &s->timers[0]) >> 8;
> + val = get_counter(s, &s->timers[0], now) >> 8;
> break;
> case VIA_REG_T1LL:
> val = s->timers[0].latch & 0xff;
> @@ -247,12 +251,12 @@ uint64_t mos6522_read(void *opaque, hwaddr addr, unsigned size)
> val = (s->timers[0].latch >> 8) & 0xff;
> break;
> case VIA_REG_T2CL:
> - val = get_counter(s, &s->timers[1]) & 0xff;
> + val = get_counter(s, &s->timers[1], now) & 0xff;
> s->ifr &= ~T2_INT;
> mos6522_update_irq(s);
> break;
> case VIA_REG_T2CH:
> - val = get_counter(s, &s->timers[1]) >> 8;
> + val = get_counter(s, &s->timers[1], now) >> 8;
> break;
> case VIA_REG_SR:
> val = s->sr;
> @@ -360,10 +364,9 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
> }
> mos6522_update_irq(s);
> /* if IER is modified starts needed timers */
> - mos6522_timer1_update(s, &s->timers[0],
> - qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
> - mos6522_timer2_update(s, &s->timers[1],
> - qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
> + 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;
> default:
> g_assert_not_reached();
In terms of functionality it shouldn't really matter (since you have a ns clock
compared with a timer that can manage small frequencies in comparison) but I can see
how having a constant clock time throughout the entire calculation process could be
useful for debugging.
ATB,
Mark.
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [RFC 09/10] hw/mos6522: Avoid using discrepant QEMU clock values
2021-08-25 8:44 ` Mark Cave-Ayland
@ 2021-08-29 1:55 ` Finn Thain
0 siblings, 0 replies; 47+ messages in thread
From: Finn Thain @ 2021-08-29 1:55 UTC (permalink / raw)
To: Mark Cave-Ayland
Cc: qemu-devel, Laurent Vivier, qemu-ppc, Greg Kurz, David Gibson
On Wed, 25 Aug 2021, Mark Cave-Ayland wrote:
> On 24/08/2021 11:09, Finn Thain wrote:
>
> > mos6522_read() and mos6522_write() may call various functions to determine
> > timer irq state, timer counter value and QEMUTimer deadline. All called
> > functions must use the same value for the present time.
> >
> > Signed-off-by: Finn Thain <fthain@linux-m68k.org>
> > ---
> > hw/misc/mos6522.c | 51 +++++++++++++++++++++++++----------------------
> > 1 file changed, 27 insertions(+), 24 deletions(-)
> >
> > diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
> > index 0dd3ccf945..23a440b64f 100644
> > --- a/hw/misc/mos6522.c
> > +++ b/hw/misc/mos6522.c
> > @@ -39,9 +39,9 @@
> > /* XXX: implement all timer modes */
> > static void mos6522_timer1_update(MOS6522State *s, MOS6522Timer *ti,
> > - int64_t current_time);
> > + int64_t now);
> > static void mos6522_timer2_update(MOS6522State *s, MOS6522Timer *ti,
> > - int64_t current_time);
> > + int64_t now);
> > static void mos6522_update_irq(MOS6522State *s)
> > {
> > @@ -52,12 +52,12 @@ static void mos6522_update_irq(MOS6522State *s)
> > }
> > }
> > -static unsigned int get_counter(MOS6522State *s, MOS6522Timer *ti)
> > +static unsigned int get_counter(MOS6522State *s, MOS6522Timer *ti, int64_t
> > now)
> > {
> > int64_t d;
> > unsigned int counter;
> > - d = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) - ti->load_time,
> > + d = muldiv64(now - ti->load_time,
> > ti->frequency, NANOSECONDS_PER_SECOND);
> > if (ti->index == 0) {
> > @@ -89,7 +89,7 @@ static void set_counter(MOS6522State *s, MOS6522Timer *ti,
> > unsigned int val)
> > }
> > static int64_t get_next_irq_time(MOS6522State *s, MOS6522Timer *ti,
> > - int64_t current_time)
> > + int64_t now)
> > {
> > int64_t d, next_time;
> > unsigned int counter;
> > @@ -99,7 +99,7 @@ static int64_t get_next_irq_time(MOS6522State *s,
> > MOS6522Timer *ti,
> > }
> > /* current counter value */
> > - d = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) - ti->load_time,
> > + d = muldiv64(now - ti->load_time,
> > ti->frequency, NANOSECONDS_PER_SECOND);
> > /* the timer goes down from latch to -1 (period of latch + 2) */
> > @@ -123,20 +123,19 @@ static int64_t get_next_irq_time(MOS6522State *s,
> > MOS6522Timer *ti,
> > 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;
> > -
> > - if (next_time <= current_time) {
> > - next_time = current_time + 1;
> > - }
> > return next_time;
> > }
> > static void mos6522_timer1_update(MOS6522State *s, MOS6522Timer *ti,
> > - int64_t current_time)
> > + int64_t now)
> > {
> > if (!ti->timer) {
> > return;
> > }
> > - ti->next_irq_time = get_next_irq_time(s, ti, current_time);
> > + 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)) {
> > timer_del(ti->timer);
> > @@ -146,12 +145,15 @@ static void mos6522_timer1_update(MOS6522State *s,
> > MOS6522Timer *ti,
> > }
> > static void mos6522_timer2_update(MOS6522State *s, MOS6522Timer *ti,
> > - int64_t current_time)
> > + int64_t now)
> > {
> > if (!ti->timer) {
> > return;
> > }
> > - ti->next_irq_time = get_next_irq_time(s, ti, current_time);
> > + 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)
> > {
> > timer_del(ti->timer);
> > } else {
> > @@ -163,9 +165,10 @@ static void mos6522_timer1_expired(void *opaque)
> > {
> > MOS6522State *s = 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, ti->next_irq_time);
> > + mos6522_timer1_update(s, ti, now);
>
> Presumably using ti->next_irq_time has already fixed the current time to
> be that at which the timer routine actually expired, rather than the
> current executing time. Are you seeing large differences in these
> numbers that can cause timer drift? If so, I'm wondering if this change
> should be in a separate patch.
>
You're right. This change has more relevance to the synchronization work
in the following patch. It's not really covered by the commit log here.
> > s->ifr |= T1_INT;
> > mos6522_update_irq(s);
> > }
> > @@ -174,9 +177,10 @@ static void mos6522_timer2_expired(void *opaque)
> > {
> > MOS6522State *s = 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, ti->next_irq_time);
> > + mos6522_timer2_update(s, ti, now);
>
> And same again here.
>
I'll find a better way to split up these patches.
> > s->ifr |= T2_INT;
> > mos6522_update_irq(s);
> > }
> > @@ -233,12 +237,12 @@ uint64_t mos6522_read(void *opaque, hwaddr addr,
> > unsigned size)
> > val = s->dira;
> > break;
> > case VIA_REG_T1CL:
> > - val = get_counter(s, &s->timers[0]) & 0xff;
> > + val = get_counter(s, &s->timers[0], now) & 0xff;
> > s->ifr &= ~T1_INT;
> > mos6522_update_irq(s);
> > break;
> > case VIA_REG_T1CH:
> > - val = get_counter(s, &s->timers[0]) >> 8;
> > + val = get_counter(s, &s->timers[0], now) >> 8;
> > break;
> > case VIA_REG_T1LL:
> > val = s->timers[0].latch & 0xff;
> > @@ -247,12 +251,12 @@ uint64_t mos6522_read(void *opaque, hwaddr addr,
> > unsigned size)
> > val = (s->timers[0].latch >> 8) & 0xff;
> > break;
> > case VIA_REG_T2CL:
> > - val = get_counter(s, &s->timers[1]) & 0xff;
> > + val = get_counter(s, &s->timers[1], now) & 0xff;
> > s->ifr &= ~T2_INT;
> > mos6522_update_irq(s);
> > break;
> > case VIA_REG_T2CH:
> > - val = get_counter(s, &s->timers[1]) >> 8;
> > + val = get_counter(s, &s->timers[1], now) >> 8;
> > break;
> > case VIA_REG_SR:
> > val = s->sr;
> > @@ -360,10 +364,9 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t
> > val, unsigned size)
> > }
> > mos6522_update_irq(s);
> > /* if IER is modified starts needed timers */
> > - mos6522_timer1_update(s, &s->timers[0],
> > - qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
> > - mos6522_timer2_update(s, &s->timers[1],
> > - qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
> > + 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;
> > default:
> > g_assert_not_reached();
>
> In terms of functionality it shouldn't really matter (since you have a
> ns clock compared with a timer that can manage small frequencies in
> comparison) but I can see how having a constant clock time throughout
> the entire calculation process could be useful for debugging.
>
I found it impossible to reason about program behaviour with so many calls
to qemu_clock_get_ns().
Thanks for your review.
^ permalink raw reply [flat|nested] 47+ messages in thread
* [RFC 06/10] hw/mos6522: Implement oneshot mode
2021-08-24 10:09 [RFC 00/10] hw/mos6522: VIA timer emulation fixes and improvements Finn Thain
2021-08-24 10:09 ` [RFC 09/10] hw/mos6522: Avoid using discrepant QEMU clock values Finn Thain
@ 2021-08-24 10:09 ` Finn Thain
2021-08-25 8:18 ` Mark Cave-Ayland
2021-08-24 10:09 ` [RFC 01/10] hw/mos6522: Remove get_load_time() methods and functions Finn Thain
` (10 subsequent siblings)
12 siblings, 1 reply; 47+ messages in thread
From: Finn Thain @ 2021-08-24 10:09 UTC (permalink / raw)
To: Mark Cave-Ayland, David Gibson, Greg Kurz
Cc: qemu-ppc, Laurent Vivier, qemu-devel
Signed-off-by: Finn Thain <fthain@linux-m68k.org>
---
hw/misc/mos6522.c | 19 ++++++++++++-------
include/hw/misc/mos6522.h | 3 +++
2 files changed, 15 insertions(+), 7 deletions(-)
diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
index ffff8991f4..5b1657ac0d 100644
--- a/hw/misc/mos6522.c
+++ b/hw/misc/mos6522.c
@@ -79,6 +79,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;
if (ti->index == 0) {
mos6522_timer1_update(s, ti, ti->load_time);
} else {
@@ -133,7 +134,8 @@ static void mos6522_timer1_update(MOS6522State *s, MOS6522Timer *ti,
return;
}
ti->next_irq_time = get_next_irq_time(s, ti, current_time);
- if ((s->ier & T1_INT) == 0 || (s->acr & T1MODE) != T1MODE_CONT) {
+ if ((s->ier & T1_INT) == 0 ||
+ ((s->acr & T1MODE) == T1MODE_ONESHOT && ti->oneshot_fired)) {
timer_del(ti->timer);
} else {
timer_mod(ti->timer, ti->next_irq_time);
@@ -147,7 +149,7 @@ static void mos6522_timer2_update(MOS6522State *s, MOS6522Timer *ti,
return;
}
ti->next_irq_time = get_next_irq_time(s, ti, current_time);
- if ((s->ier & T2_INT) == 0) {
+ if ((s->ier & T2_INT) == 0 || (s->acr & T2MODE) || ti->oneshot_fired) {
timer_del(ti->timer);
} else {
timer_mod(ti->timer, ti->next_irq_time);
@@ -159,6 +161,7 @@ static void mos6522_timer1_expired(void *opaque)
MOS6522State *s = opaque;
MOS6522Timer *ti = &s->timers[0];
+ ti->oneshot_fired = true;
mos6522_timer1_update(s, ti, ti->next_irq_time);
s->ifr |= T1_INT;
mos6522_update_irq(s);
@@ -169,6 +172,7 @@ static void mos6522_timer2_expired(void *opaque)
MOS6522State *s = opaque;
MOS6522Timer *ti = &s->timers[1];
+ ti->oneshot_fired = true;
mos6522_timer2_update(s, ti, ti->next_irq_time);
s->ifr |= T2_INT;
mos6522_update_irq(s);
@@ -198,10 +202,12 @@ uint64_t mos6522_read(void *opaque, hwaddr addr, unsigned size)
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;
}
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;
}
@@ -279,6 +285,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;
trace_mos6522_write(addr, val);
@@ -318,9 +325,6 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
s->timers[1].latch = (s->timers[1].latch & 0xff00) | val;
break;
case VIA_REG_T2CH:
- /* To ensure T2 generates an interrupt on zero crossing with the
- common timer code, write the value directly from the latch to
- the counter */
s->timers[1].latch = (s->timers[1].latch & 0xff) | (val << 8);
s->ifr &= ~T2_INT;
set_counter(s, &s->timers[1], s->timers[1].latch);
@@ -330,8 +334,9 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
break;
case VIA_REG_ACR:
s->acr = val;
- mos6522_timer1_update(s, &s->timers[0],
- qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
+ 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;
case VIA_REG_PCR:
s->pcr = val;
diff --git a/include/hw/misc/mos6522.h b/include/hw/misc/mos6522.h
index fc95d22b0f..94b1dc324c 100644
--- a/include/hw/misc/mos6522.h
+++ b/include/hw/misc/mos6522.h
@@ -50,8 +50,10 @@
#define T1_INT 0x40 /* Timer 1 interrupt */
/* Bits in ACR */
+#define T2MODE 0x20 /* Timer 2 mode */
#define T1MODE 0xc0 /* Timer 1 mode */
#define T1MODE_CONT 0x40 /* continuous interrupts */
+#define T1MODE_ONESHOT 0x00 /* timed interrupt */
/* VIA registers */
#define VIA_REG_B 0x00
@@ -83,6 +85,7 @@ typedef struct MOS6522Timer {
int64_t next_irq_time;
uint64_t frequency;
QEMUTimer *timer;
+ bool oneshot_fired;
} MOS6522Timer;
/**
--
2.26.3
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [RFC 06/10] hw/mos6522: Implement oneshot mode
2021-08-24 10:09 ` [RFC 06/10] hw/mos6522: Implement oneshot mode Finn Thain
@ 2021-08-25 8:18 ` Mark Cave-Ayland
2021-08-29 1:20 ` Finn Thain
0 siblings, 1 reply; 47+ messages in thread
From: Mark Cave-Ayland @ 2021-08-25 8:18 UTC (permalink / raw)
To: Finn Thain, David Gibson, Greg Kurz; +Cc: qemu-ppc, Laurent Vivier, qemu-devel
On 24/08/2021 11:09, Finn Thain wrote:
> Signed-off-by: Finn Thain <fthain@linux-m68k.org>
> ---
> hw/misc/mos6522.c | 19 ++++++++++++-------
> include/hw/misc/mos6522.h | 3 +++
> 2 files changed, 15 insertions(+), 7 deletions(-)
>
> diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
> index ffff8991f4..5b1657ac0d 100644
> --- a/hw/misc/mos6522.c
> +++ b/hw/misc/mos6522.c
> @@ -79,6 +79,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;
> if (ti->index == 0) {
> mos6522_timer1_update(s, ti, ti->load_time);
> } else {
> @@ -133,7 +134,8 @@ static void mos6522_timer1_update(MOS6522State *s, MOS6522Timer *ti,
> return;
> }
> ti->next_irq_time = get_next_irq_time(s, ti, current_time);
> - if ((s->ier & T1_INT) == 0 || (s->acr & T1MODE) != T1MODE_CONT) {
> + if ((s->ier & T1_INT) == 0 ||
> + ((s->acr & T1MODE) == T1MODE_ONESHOT && ti->oneshot_fired)) {
> timer_del(ti->timer);
> } else {
> timer_mod(ti->timer, ti->next_irq_time);
> @@ -147,7 +149,7 @@ static void mos6522_timer2_update(MOS6522State *s, MOS6522Timer *ti,
> return;
> }
> ti->next_irq_time = get_next_irq_time(s, ti, current_time);
> - if ((s->ier & T2_INT) == 0) {
> + if ((s->ier & T2_INT) == 0 || (s->acr & T2MODE) || ti->oneshot_fired) {
> timer_del(ti->timer);
> } else {
> timer_mod(ti->timer, ti->next_irq_time);
> @@ -159,6 +161,7 @@ static void mos6522_timer1_expired(void *opaque)
> MOS6522State *s = opaque;
> MOS6522Timer *ti = &s->timers[0];
>
> + ti->oneshot_fired = true;
> mos6522_timer1_update(s, ti, ti->next_irq_time);
> s->ifr |= T1_INT;
> mos6522_update_irq(s);
> @@ -169,6 +172,7 @@ static void mos6522_timer2_expired(void *opaque)
> MOS6522State *s = opaque;
> MOS6522Timer *ti = &s->timers[1];
>
> + ti->oneshot_fired = true;
> mos6522_timer2_update(s, ti, ti->next_irq_time);
> s->ifr |= T2_INT;
> mos6522_update_irq(s);
I was trying to understand why you need ti->oneshot_fired here since the
mos6522_timer*_update() functions should simply not re-arm the timer if not in
continuous mode...
> @@ -198,10 +202,12 @@ uint64_t mos6522_read(void *opaque, hwaddr addr, unsigned size)
> 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;
> }
> 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;
> }
...however this block above raises the timer interrupt outside of the timer callback.
This block isn't part of your original patch but was introduced as part of
cd8843ff25d ("mos6522: fix T1 and T2 timers") but I'm wondering if it is wrong.
If you remove both of the above if (now ... ) {} blocks then does one-shot mode work
by just adding the (s->acr & T2MODE) check in mos6522_timer2_update()? I'm guessing
that Linux/m68k does use one or both of the timers in one-shot mode?
> @@ -279,6 +285,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;
>
> trace_mos6522_write(addr, val);
>
> @@ -318,9 +325,6 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
> s->timers[1].latch = (s->timers[1].latch & 0xff00) | val;
> break;
> case VIA_REG_T2CH:
> - /* To ensure T2 generates an interrupt on zero crossing with the
> - common timer code, write the value directly from the latch to
> - the counter */
> s->timers[1].latch = (s->timers[1].latch & 0xff) | (val << 8);
> s->ifr &= ~T2_INT;
> set_counter(s, &s->timers[1], s->timers[1].latch);
> @@ -330,8 +334,9 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
> break;
> case VIA_REG_ACR:
> s->acr = val;
> - mos6522_timer1_update(s, &s->timers[0],
> - qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
> + 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;
> case VIA_REG_PCR:
> s->pcr = val;
> diff --git a/include/hw/misc/mos6522.h b/include/hw/misc/mos6522.h
> index fc95d22b0f..94b1dc324c 100644
> --- a/include/hw/misc/mos6522.h
> +++ b/include/hw/misc/mos6522.h
> @@ -50,8 +50,10 @@
> #define T1_INT 0x40 /* Timer 1 interrupt */
>
> /* Bits in ACR */
> +#define T2MODE 0x20 /* Timer 2 mode */
> #define T1MODE 0xc0 /* Timer 1 mode */
> #define T1MODE_CONT 0x40 /* continuous interrupts */
> +#define T1MODE_ONESHOT 0x00 /* timed interrupt */
>
> /* VIA registers */
> #define VIA_REG_B 0x00
> @@ -83,6 +85,7 @@ typedef struct MOS6522Timer {
> int64_t next_irq_time;
> uint64_t frequency;
> QEMUTimer *timer;
> + bool oneshot_fired;
> } MOS6522Timer;
>
> /**
ATB,
Mark.
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [RFC 06/10] hw/mos6522: Implement oneshot mode
2021-08-25 8:18 ` Mark Cave-Ayland
@ 2021-08-29 1:20 ` Finn Thain
0 siblings, 0 replies; 47+ messages in thread
From: Finn Thain @ 2021-08-29 1:20 UTC (permalink / raw)
To: Mark Cave-Ayland
Cc: qemu-devel, Laurent Vivier, qemu-ppc, Greg Kurz, David Gibson
On Wed, 25 Aug 2021, Mark Cave-Ayland wrote:
> On 24/08/2021 11:09, Finn Thain wrote:
>
> > Signed-off-by: Finn Thain <fthain@linux-m68k.org>
> > ---
> > hw/misc/mos6522.c | 19 ++++++++++++-------
> > include/hw/misc/mos6522.h | 3 +++
> > 2 files changed, 15 insertions(+), 7 deletions(-)
> >
> > diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
> > index ffff8991f4..5b1657ac0d 100644
> > --- a/hw/misc/mos6522.c
> > +++ b/hw/misc/mos6522.c
> > @@ -79,6 +79,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;
> > if (ti->index == 0) {
> > mos6522_timer1_update(s, ti, ti->load_time);
> > } else {
> > @@ -133,7 +134,8 @@ static void mos6522_timer1_update(MOS6522State *s,
> > MOS6522Timer *ti,
> > return;
> > }
> > ti->next_irq_time = get_next_irq_time(s, ti, current_time);
> > - if ((s->ier & T1_INT) == 0 || (s->acr & T1MODE) != T1MODE_CONT) {
> > + if ((s->ier & T1_INT) == 0 ||
> > + ((s->acr & T1MODE) == T1MODE_ONESHOT && ti->oneshot_fired)) {
> > timer_del(ti->timer);
> > } else {
> > timer_mod(ti->timer, ti->next_irq_time);
> > @@ -147,7 +149,7 @@ static void mos6522_timer2_update(MOS6522State *s,
> > MOS6522Timer *ti,
> > return;
> > }
> > ti->next_irq_time = get_next_irq_time(s, ti, current_time);
> > - if ((s->ier & T2_INT) == 0) {
> > + if ((s->ier & T2_INT) == 0 || (s->acr & T2MODE) || ti->oneshot_fired) {
> > timer_del(ti->timer);
> > } else {
> > timer_mod(ti->timer, ti->next_irq_time);
> > @@ -159,6 +161,7 @@ static void mos6522_timer1_expired(void *opaque)
> > MOS6522State *s = opaque;
> > MOS6522Timer *ti = &s->timers[0];
> > + ti->oneshot_fired = true;
> > mos6522_timer1_update(s, ti, ti->next_irq_time);
> > s->ifr |= T1_INT;
> > mos6522_update_irq(s);
> > @@ -169,6 +172,7 @@ static void mos6522_timer2_expired(void *opaque)
> > MOS6522State *s = opaque;
> > MOS6522Timer *ti = &s->timers[1];
> > + ti->oneshot_fired = true;
> > mos6522_timer2_update(s, ti, ti->next_irq_time);
> > s->ifr |= T2_INT;
> > mos6522_update_irq(s);
>
> I was trying to understand why you need ti->oneshot_fired here since the
> mos6522_timer*_update() functions should simply not re-arm the timer if
> not in continuous mode...
>
Not so. The timer has to be re-armed with timer_mod() when
(timer interrupt enabled and timer in continuous mode) ||
(timer interrupt enabled and timer in oneshot mode and no interrupt raised)
Conversely, the timer has to be cancelled with timer_del() when
(timer interrupt disabled) ||
(timer in oneshot mode and interrupt has been raised) ||
(timer in pulse-counting mode)
> > @@ -198,10 +202,12 @@ uint64_t mos6522_read(void *opaque, hwaddr addr,
> > unsigned size)
> > 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;
> > }
> > 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;
> > }
>
> ...however this block above raises the timer interrupt outside of the
> timer callback. This block isn't part of your original patch but was
> introduced as part of cd8843ff25d ("mos6522: fix T1 and T2 timers") but
> I'm wondering if it is wrong.
>
Maybe. I think a good answer would make reference to QEMU internals and
synchronization guarantees between the invocation of the callbacks and
methods in mos6522.c. I don't have a good answer, but it's moot...
> If you remove both of the above if (now ... ) {} blocks then does
> one-shot mode work by just adding the (s->acr & T2MODE) check in
> mos6522_timer2_update()?
>
Those blocks got removed in patch 10/10 because they aren't needed as long
as get_counter() gets called when necessary.
> I'm guessing that Linux/m68k does use one or both of the timers in
> one-shot mode?
>
Yes, but it's not in mainline yet. I wrote the code some months ago but
I can't push it upstream until QEMU supports it:
https://github.com/fthain/linux/commits/clockevent-oneshot
^ permalink raw reply [flat|nested] 47+ messages in thread
* [RFC 01/10] hw/mos6522: Remove get_load_time() methods and functions
2021-08-24 10:09 [RFC 00/10] hw/mos6522: VIA timer emulation fixes and improvements Finn Thain
2021-08-24 10:09 ` [RFC 09/10] hw/mos6522: Avoid using discrepant QEMU clock values Finn Thain
2021-08-24 10:09 ` [RFC 06/10] hw/mos6522: Implement oneshot mode Finn Thain
@ 2021-08-24 10:09 ` Finn Thain
2021-08-24 10:29 ` Philippe Mathieu-Daudé
2021-08-25 6:55 ` Mark Cave-Ayland
2021-08-24 10:09 ` [RFC 08/10] hw/mos6522: Call mos6522_update_irq() when appropriate Finn Thain
` (9 subsequent siblings)
12 siblings, 2 replies; 47+ messages in thread
From: Finn Thain @ 2021-08-24 10:09 UTC (permalink / raw)
To: Mark Cave-Ayland, David Gibson, Greg Kurz
Cc: qemu-ppc, Laurent Vivier, qemu-devel
This code appears to be unnecessary.
Signed-off-by: Finn Thain <fthain@linux-m68k.org>
---
hw/misc/mos6522.c | 22 +---------------------
1 file changed, 1 insertion(+), 21 deletions(-)
diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
index 1c57332b40..a478c1ca43 100644
--- a/hw/misc/mos6522.c
+++ b/hw/misc/mos6522.c
@@ -63,17 +63,6 @@ static uint64_t get_counter_value(MOS6522State *s, MOS6522Timer *ti)
}
}
-static uint64_t get_load_time(MOS6522State *s, MOS6522Timer *ti)
-{
- MOS6522DeviceClass *mdc = MOS6522_GET_CLASS(s);
-
- if (ti->index == 0) {
- return mdc->get_timer1_load_time(s, ti);
- } else {
- return mdc->get_timer2_load_time(s, ti);
- }
-}
-
static unsigned int get_counter(MOS6522State *s, MOS6522Timer *ti)
{
int64_t d;
@@ -98,7 +87,7 @@ static unsigned int get_counter(MOS6522State *s, MOS6522Timer *ti)
static void set_counter(MOS6522State *s, MOS6522Timer *ti, unsigned int val)
{
trace_mos6522_set_counter(1 + ti->index, val);
- ti->load_time = get_load_time(s, ti);
+ ti->load_time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
ti->counter_value = val;
if (ti->index == 0) {
mos6522_timer1_update(s, ti, ti->load_time);
@@ -208,13 +197,6 @@ static uint64_t mos6522_get_counter_value(MOS6522State *s, MOS6522Timer *ti)
ti->frequency, NANOSECONDS_PER_SECOND);
}
-static uint64_t mos6522_get_load_time(MOS6522State *s, MOS6522Timer *ti)
-{
- uint64_t load_time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
-
- return load_time;
-}
-
static void mos6522_portA_write(MOS6522State *s)
{
qemu_log_mask(LOG_UNIMP, "portA_write unimplemented\n");
@@ -518,8 +500,6 @@ static void mos6522_class_init(ObjectClass *oc, void *data)
mdc->update_irq = mos6522_update_irq;
mdc->get_timer1_counter_value = mos6522_get_counter_value;
mdc->get_timer2_counter_value = mos6522_get_counter_value;
- mdc->get_timer1_load_time = mos6522_get_load_time;
- mdc->get_timer2_load_time = mos6522_get_load_time;
}
static const TypeInfo mos6522_type_info = {
--
2.26.3
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [RFC 01/10] hw/mos6522: Remove get_load_time() methods and functions
2021-08-24 10:09 ` [RFC 01/10] hw/mos6522: Remove get_load_time() methods and functions Finn Thain
@ 2021-08-24 10:29 ` Philippe Mathieu-Daudé
2021-08-25 6:55 ` Mark Cave-Ayland
1 sibling, 0 replies; 47+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-08-24 10:29 UTC (permalink / raw)
To: Finn Thain, Mark Cave-Ayland, David Gibson, Greg Kurz
Cc: qemu-ppc, Laurent Vivier, qemu-devel
On 8/24/21 12:09 PM, Finn Thain wrote:
> This code appears to be unnecessary.
>
> Signed-off-by: Finn Thain <fthain@linux-m68k.org>
> ---
> hw/misc/mos6522.c | 22 +---------------------
> 1 file changed, 1 insertion(+), 21 deletions(-)
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC 01/10] hw/mos6522: Remove get_load_time() methods and functions
2021-08-24 10:09 ` [RFC 01/10] hw/mos6522: Remove get_load_time() methods and functions Finn Thain
2021-08-24 10:29 ` Philippe Mathieu-Daudé
@ 2021-08-25 6:55 ` Mark Cave-Ayland
2021-08-28 1:00 ` Finn Thain
1 sibling, 1 reply; 47+ messages in thread
From: Mark Cave-Ayland @ 2021-08-25 6:55 UTC (permalink / raw)
To: Finn Thain, David Gibson, Greg Kurz; +Cc: qemu-ppc, Laurent Vivier, qemu-devel
On 24/08/2021 11:09, Finn Thain wrote:
> This code appears to be unnecessary.
>
> Signed-off-by: Finn Thain <fthain@linux-m68k.org>
> ---
> hw/misc/mos6522.c | 22 +---------------------
> 1 file changed, 1 insertion(+), 21 deletions(-)
>
> diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
> index 1c57332b40..a478c1ca43 100644
> --- a/hw/misc/mos6522.c
> +++ b/hw/misc/mos6522.c
> @@ -63,17 +63,6 @@ static uint64_t get_counter_value(MOS6522State *s, MOS6522Timer *ti)
> }
> }
>
> -static uint64_t get_load_time(MOS6522State *s, MOS6522Timer *ti)
> -{
> - MOS6522DeviceClass *mdc = MOS6522_GET_CLASS(s);
> -
> - if (ti->index == 0) {
> - return mdc->get_timer1_load_time(s, ti);
> - } else {
> - return mdc->get_timer2_load_time(s, ti);
> - }
> -}
> -
> static unsigned int get_counter(MOS6522State *s, MOS6522Timer *ti)
> {
> int64_t d;
> @@ -98,7 +87,7 @@ static unsigned int get_counter(MOS6522State *s, MOS6522Timer *ti)
> static void set_counter(MOS6522State *s, MOS6522Timer *ti, unsigned int val)
> {
> trace_mos6522_set_counter(1 + ti->index, val);
> - ti->load_time = get_load_time(s, ti);
> + ti->load_time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> ti->counter_value = val;
> if (ti->index == 0) {
> mos6522_timer1_update(s, ti, ti->load_time);
> @@ -208,13 +197,6 @@ static uint64_t mos6522_get_counter_value(MOS6522State *s, MOS6522Timer *ti)
> ti->frequency, NANOSECONDS_PER_SECOND);
> }
>
> -static uint64_t mos6522_get_load_time(MOS6522State *s, MOS6522Timer *ti)
> -{
> - uint64_t load_time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> -
> - return load_time;
> -}
> -
> static void mos6522_portA_write(MOS6522State *s)
> {
> qemu_log_mask(LOG_UNIMP, "portA_write unimplemented\n");
> @@ -518,8 +500,6 @@ static void mos6522_class_init(ObjectClass *oc, void *data)
> mdc->update_irq = mos6522_update_irq;
> mdc->get_timer1_counter_value = mos6522_get_counter_value;
> mdc->get_timer2_counter_value = mos6522_get_counter_value;
> - mdc->get_timer1_load_time = mos6522_get_load_time;
> - mdc->get_timer2_load_time = mos6522_get_load_time;
> }
>
> static const TypeInfo mos6522_type_info = {
Both the get_counter_value() and get_load_time() callbacks are used as part of the
CUDA emulation in hw/misc/macio/cuda.c as per the comment:
/* MacOS uses timer 1 for calibration on startup, so we use
* the timebase frequency and cuda_get_counter_value() with
* cuda_get_load_time() to steer MacOS to calculate calibrate its timers
* correctly for both TCG and KVM (see commit b981289c49 "PPC: Cuda: Use cuda
* timer to expose tbfreq to guest" for more information) */
Certainly for the 6522 device it is worth configuring with --target-list="ppc-softmmu
m68k-softmmu" to make sure that you don't inadvertently break anything in the PPC world.
A bit of history here: the original mos6522.c was extracted from hw/misc/macio/cuda.c
when Laurent presented his initial q800 patches since they also had their own
implementation of the 6522, and it was better to move the implementation into a
separate QEMU device so that the logic could be shared.
The Darwin kernel timer calibration loop is quite hard to get right: see
https://opensource.apple.com/source/xnu/xnu-123.5/pexpert/ppc/pe_clock_speed_asm.s.auto.html
and
https://opensource.apple.com/source/xnu/xnu-123.5/pexpert/ppc/pe_clock_speed.c.auto.html.
Ben/Alex came up with the current mechanism to fool the calibration routine, and I
simply added in those callbacks to allow it to be implemented as part of the
now-generic 6522 device.
ATB,
Mark.
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [RFC 01/10] hw/mos6522: Remove get_load_time() methods and functions
2021-08-25 6:55 ` Mark Cave-Ayland
@ 2021-08-28 1:00 ` Finn Thain
0 siblings, 0 replies; 47+ messages in thread
From: Finn Thain @ 2021-08-28 1:00 UTC (permalink / raw)
To: Mark Cave-Ayland
Cc: qemu-devel, Laurent Vivier, qemu-ppc, Greg Kurz, David Gibson
On Wed, 25 Aug 2021, Mark Cave-Ayland wrote:
> On 24/08/2021 11:09, Finn Thain wrote:
>
> > This code appears to be unnecessary.
> >
> > Signed-off-by: Finn Thain <fthain@linux-m68k.org>
> > ---
> > hw/misc/mos6522.c | 22 +---------------------
> > 1 file changed, 1 insertion(+), 21 deletions(-)
> >
> > diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
> > index 1c57332b40..a478c1ca43 100644
> > --- a/hw/misc/mos6522.c
> > +++ b/hw/misc/mos6522.c
> > @@ -63,17 +63,6 @@ static uint64_t get_counter_value(MOS6522State *s,
> > MOS6522Timer *ti)
> > }
> > }
> > -static uint64_t get_load_time(MOS6522State *s, MOS6522Timer *ti)
> > -{
> > - MOS6522DeviceClass *mdc = MOS6522_GET_CLASS(s);
> > -
> > - if (ti->index == 0) {
> > - return mdc->get_timer1_load_time(s, ti);
> > - } else {
> > - return mdc->get_timer2_load_time(s, ti);
> > - }
> > -}
> > -
> > static unsigned int get_counter(MOS6522State *s, MOS6522Timer *ti)
> > {
> > int64_t d;
> > @@ -98,7 +87,7 @@ static unsigned int get_counter(MOS6522State *s,
> > MOS6522Timer *ti)
> > static void set_counter(MOS6522State *s, MOS6522Timer *ti, unsigned int
> > val)
> > {
> > trace_mos6522_set_counter(1 + ti->index, val);
> > - ti->load_time = get_load_time(s, ti);
> > + ti->load_time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> > ti->counter_value = val;
> > if (ti->index == 0) {
> > mos6522_timer1_update(s, ti, ti->load_time);
> > @@ -208,13 +197,6 @@ static uint64_t mos6522_get_counter_value(MOS6522State
> > *s, MOS6522Timer *ti)
> > ti->frequency, NANOSECONDS_PER_SECOND);
> > }
> > -static uint64_t mos6522_get_load_time(MOS6522State *s, MOS6522Timer *ti)
> > -{
> > - uint64_t load_time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> > -
> > - return load_time;
> > -}
> > -
> > static void mos6522_portA_write(MOS6522State *s)
> > {
> > qemu_log_mask(LOG_UNIMP, "portA_write unimplemented\n");
> > @@ -518,8 +500,6 @@ static void mos6522_class_init(ObjectClass *oc, void
> > *data)
> > mdc->update_irq = mos6522_update_irq;
> > mdc->get_timer1_counter_value = mos6522_get_counter_value;
> > mdc->get_timer2_counter_value = mos6522_get_counter_value;
> > - mdc->get_timer1_load_time = mos6522_get_load_time;
> > - mdc->get_timer2_load_time = mos6522_get_load_time;
> > }
> > static const TypeInfo mos6522_type_info = {
>
> Both the get_counter_value() and get_load_time() callbacks are used as part of
> the CUDA emulation in hw/misc/macio/cuda.c as per the comment:
>
> /* MacOS uses timer 1 for calibration on startup, so we use
> * the timebase frequency and cuda_get_counter_value() with
> * cuda_get_load_time() to steer MacOS to calculate calibrate its timers
> * correctly for both TCG and KVM (see commit b981289c49 "PPC: Cuda: Use cuda
> * timer to expose tbfreq to guest" for more information) */
>
> Certainly for the 6522 device it is worth configuring with
> --target-list="ppc-softmmu m68k-softmmu" to make sure that you don't
> inadvertently break anything in the PPC world.
>
No build failure here. Maybe your tree is different?
> A bit of history here: the original mos6522.c was extracted from
> hw/misc/macio/cuda.c when Laurent presented his initial q800 patches since
> they also had their own implementation of the 6522, and it was better to move
> the implementation into a separate QEMU device so that the logic could be
> shared.
>
> The Darwin kernel timer calibration loop is quite hard to get right: see
> https://opensource.apple.com/source/xnu/xnu-123.5/pexpert/ppc/pe_clock_speed_asm.s.auto.html
> and
> https://opensource.apple.com/source/xnu/xnu-123.5/pexpert/ppc/pe_clock_speed.c.auto.html.
> Ben/Alex came up with the current mechanism to fool the calibration routine,
> and I simply added in those callbacks to allow it to be implemented as part of
> the now-generic 6522 device.
>
I didn't find any references to these methods (get_timer1_counter_value,
get_timer2_counter_value, get_timer1_load_time and get_timer2_load_time).
It appears to be dead code, and it adds complexity and harms readability.
The Linux kernel project has a policy that no code is added if it lacks
any in-tree usage. Does QEMU have the same policy?
^ permalink raw reply [flat|nested] 47+ messages in thread
* [RFC 08/10] hw/mos6522: Call mos6522_update_irq() when appropriate
2021-08-24 10:09 [RFC 00/10] hw/mos6522: VIA timer emulation fixes and improvements Finn Thain
` (2 preceding siblings ...)
2021-08-24 10:09 ` [RFC 01/10] hw/mos6522: Remove get_load_time() methods and functions Finn Thain
@ 2021-08-24 10:09 ` Finn Thain
2021-08-24 10:22 ` Philippe Mathieu-Daudé
2021-08-25 8:26 ` Mark Cave-Ayland
2021-08-24 10:09 ` [RFC 07/10] hw/mos6522: Fix initial timer counter reload Finn Thain
` (8 subsequent siblings)
12 siblings, 2 replies; 47+ messages in thread
From: Finn Thain @ 2021-08-24 10:09 UTC (permalink / raw)
To: Mark Cave-Ayland, David Gibson, Greg Kurz
Cc: qemu-ppc, Laurent Vivier, qemu-devel
It necessary to call mos6522_update_irq() when the interrupt flags
change and unnecessary when they haven't.
Signed-off-by: Finn Thain <fthain@linux-m68k.org>
---
hw/misc/mos6522.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
index 0a241fe9f8..0dd3ccf945 100644
--- a/hw/misc/mos6522.c
+++ b/hw/misc/mos6522.c
@@ -208,11 +208,13 @@ uint64_t mos6522_read(void *opaque, hwaddr addr, unsigned size)
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:
@@ -237,7 +239,6 @@ uint64_t mos6522_read(void *opaque, hwaddr addr, unsigned size)
break;
case VIA_REG_T1CH:
val = get_counter(s, &s->timers[0]) >> 8;
- mos6522_update_irq(s);
break;
case VIA_REG_T1LL:
val = s->timers[0].latch & 0xff;
--
2.26.3
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [RFC 08/10] hw/mos6522: Call mos6522_update_irq() when appropriate
2021-08-24 10:09 ` [RFC 08/10] hw/mos6522: Call mos6522_update_irq() when appropriate Finn Thain
@ 2021-08-24 10:22 ` Philippe Mathieu-Daudé
2021-08-25 8:26 ` Mark Cave-Ayland
1 sibling, 0 replies; 47+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-08-24 10:22 UTC (permalink / raw)
To: Finn Thain, Mark Cave-Ayland, David Gibson, Greg Kurz
Cc: qemu-ppc, Laurent Vivier, qemu-devel
On 8/24/21 12:09 PM, Finn Thain wrote:
> It necessary to call mos6522_update_irq() when the interrupt flags
> change and unnecessary when they haven't.
>
> Signed-off-by: Finn Thain <fthain@linux-m68k.org>
> ---
> hw/misc/mos6522.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC 08/10] hw/mos6522: Call mos6522_update_irq() when appropriate
2021-08-24 10:09 ` [RFC 08/10] hw/mos6522: Call mos6522_update_irq() when appropriate Finn Thain
2021-08-24 10:22 ` Philippe Mathieu-Daudé
@ 2021-08-25 8:26 ` Mark Cave-Ayland
1 sibling, 0 replies; 47+ messages in thread
From: Mark Cave-Ayland @ 2021-08-25 8:26 UTC (permalink / raw)
To: Finn Thain, David Gibson, Greg Kurz; +Cc: qemu-ppc, Laurent Vivier, qemu-devel
On 24/08/2021 11:09, Finn Thain wrote:
> It necessary to call mos6522_update_irq() when the interrupt flags
> change and unnecessary when they haven't.
>
> Signed-off-by: Finn Thain <fthain@linux-m68k.org>
> ---
> hw/misc/mos6522.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
> index 0a241fe9f8..0dd3ccf945 100644
> --- a/hw/misc/mos6522.c
> +++ b/hw/misc/mos6522.c
> @@ -208,11 +208,13 @@ uint64_t mos6522_read(void *opaque, hwaddr addr, unsigned size)
> 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);
> }
Again this seems to be in the block of code I'm not sure is correct, so my first
instinct is to see if removing it helps first - although the patch logically seems
correct.
> switch (addr) {
> case VIA_REG_B:
> @@ -237,7 +239,6 @@ uint64_t mos6522_read(void *opaque, hwaddr addr, unsigned size)
> break;
> case VIA_REG_T1CH:
> val = get_counter(s, &s->timers[0]) >> 8;
> - mos6522_update_irq(s);
As get_counter() simply generates the current counter value I'd say this part is correct.
> break;
> case VIA_REG_T1LL:
> val = s->timers[0].latch & 0xff;
>
ATB,
Mark.
^ permalink raw reply [flat|nested] 47+ messages in thread
* [RFC 07/10] hw/mos6522: Fix initial timer counter reload
2021-08-24 10:09 [RFC 00/10] hw/mos6522: VIA timer emulation fixes and improvements Finn Thain
` (3 preceding siblings ...)
2021-08-24 10:09 ` [RFC 08/10] hw/mos6522: Call mos6522_update_irq() when appropriate Finn Thain
@ 2021-08-24 10:09 ` Finn Thain
2021-08-25 8:23 ` Mark Cave-Ayland
2021-08-24 10:09 ` [RFC 10/10] hw/mos6522: Synchronize timer interrupt and timer counter Finn Thain
` (7 subsequent siblings)
12 siblings, 1 reply; 47+ messages in thread
From: Finn Thain @ 2021-08-24 10:09 UTC (permalink / raw)
To: Mark Cave-Ayland, David Gibson, Greg Kurz
Cc: qemu-ppc, Laurent Vivier, qemu-devel
The first reload of timer 1 is early by half of a clock cycle as it gets
measured from a falling edge. By contrast, the succeeding reloads are
measured from rising edge to rising edge.
Neglecting that complication, the behaviour of the counter should be the
same from one reload to the next. The sequence is always:
N, N-1, N-2, ... 2, 1, 0, -1, N, N-1, N-2, ...
But at the first reload, the present driver does this instead:
N, N-1, N-2, ... 2, 1, 0, -1, N-1, N-2, ...
Fix this deviation for both timer 1 and timer 2, and allow for the fact
that on a real 6522 the timer 2 counter is not reloaded when it wraps.
Signed-off-by: Finn Thain <fthain@linux-m68k.org>
---
hw/misc/mos6522.c | 19 +++++++++++--------
1 file changed, 11 insertions(+), 8 deletions(-)
diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
index 5b1657ac0d..0a241fe9f8 100644
--- a/hw/misc/mos6522.c
+++ b/hw/misc/mos6522.c
@@ -63,15 +63,16 @@ static unsigned int get_counter(MOS6522State *s, MOS6522Timer *ti)
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) & 0xffff;
+ counter = ti->counter_value - d;
} else {
- counter = (d - (ti->counter_value + 1)) % (ti->latch + 2);
- counter = (ti->latch - counter) & 0xffff;
+ 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) & 0xffff;
+ counter = ti->counter_value - d;
}
- return counter;
+ return counter & 0xffff;
}
static void set_counter(MOS6522State *s, MOS6522Timer *ti, unsigned int val)
@@ -103,11 +104,13 @@ static int64_t get_next_irq_time(MOS6522State *s, MOS6522Timer *ti,
/* the timer goes down from latch to -1 (period of latch + 2) */
if (d <= (ti->counter_value + 1)) {
- counter = (ti->counter_value - d) & 0xffff;
+ counter = ti->counter_value - d;
} else {
- counter = (d - (ti->counter_value + 1)) % (ti->latch + 2);
- counter = (ti->latch - counter) & 0xffff;
+ 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) {
--
2.26.3
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [RFC 07/10] hw/mos6522: Fix initial timer counter reload
2021-08-24 10:09 ` [RFC 07/10] hw/mos6522: Fix initial timer counter reload Finn Thain
@ 2021-08-25 8:23 ` Mark Cave-Ayland
2021-08-28 0:46 ` Finn Thain
0 siblings, 1 reply; 47+ messages in thread
From: Mark Cave-Ayland @ 2021-08-25 8:23 UTC (permalink / raw)
To: Finn Thain, David Gibson, Greg Kurz; +Cc: qemu-ppc, Laurent Vivier, qemu-devel
On 24/08/2021 11:09, Finn Thain wrote:
> The first reload of timer 1 is early by half of a clock cycle as it gets
> measured from a falling edge. By contrast, the succeeding reloads are
> measured from rising edge to rising edge.
>
> Neglecting that complication, the behaviour of the counter should be the
> same from one reload to the next. The sequence is always:
>
> N, N-1, N-2, ... 2, 1, 0, -1, N, N-1, N-2, ...
>
> But at the first reload, the present driver does this instead:
>
> N, N-1, N-2, ... 2, 1, 0, -1, N-1, N-2, ...
>
> Fix this deviation for both timer 1 and timer 2, and allow for the fact
> that on a real 6522 the timer 2 counter is not reloaded when it wraps.
>
> Signed-off-by: Finn Thain <fthain@linux-m68k.org>
> ---
> hw/misc/mos6522.c | 19 +++++++++++--------
> 1 file changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
> index 5b1657ac0d..0a241fe9f8 100644
> --- a/hw/misc/mos6522.c
> +++ b/hw/misc/mos6522.c
> @@ -63,15 +63,16 @@ static unsigned int get_counter(MOS6522State *s, MOS6522Timer *ti)
> 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) & 0xffff;
> + counter = ti->counter_value - d;
> } else {
> - counter = (d - (ti->counter_value + 1)) % (ti->latch + 2);
> - counter = (ti->latch - counter) & 0xffff;
> + 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) & 0xffff;
> + counter = ti->counter_value - d;
> }
> - return counter;
> + return counter & 0xffff;
> }
>
> static void set_counter(MOS6522State *s, MOS6522Timer *ti, unsigned int val)
> @@ -103,11 +104,13 @@ static int64_t get_next_irq_time(MOS6522State *s, MOS6522Timer *ti,
>
> /* the timer goes down from latch to -1 (period of latch + 2) */
> if (d <= (ti->counter_value + 1)) {
> - counter = (ti->counter_value - d) & 0xffff;
> + counter = ti->counter_value - d;
> } else {
> - counter = (d - (ti->counter_value + 1)) % (ti->latch + 2);
> - counter = (ti->latch - counter) & 0xffff;
> + 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) {
I think the code looks right, but I couldn't see an explicit reference to this
behaviour in http://archive.6502.org/datasheets/mos_6522_preliminary_nov_1977.pdf.
Presumably this matches what you've observed on real hardware?
ATB,
Mark.
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [RFC 07/10] hw/mos6522: Fix initial timer counter reload
2021-08-25 8:23 ` Mark Cave-Ayland
@ 2021-08-28 0:46 ` Finn Thain
0 siblings, 0 replies; 47+ messages in thread
From: Finn Thain @ 2021-08-28 0:46 UTC (permalink / raw)
To: Mark Cave-Ayland
Cc: qemu-devel, Laurent Vivier, qemu-ppc, Greg Kurz, David Gibson
On Wed, 25 Aug 2021, Mark Cave-Ayland wrote:
> On 24/08/2021 11:09, Finn Thain wrote:
>
> > The first reload of timer 1 is early by half of a clock cycle as it gets
> > measured from a falling edge. By contrast, the succeeding reloads are
> > measured from rising edge to rising edge.
> >
> > Neglecting that complication, the behaviour of the counter should be the
> > same from one reload to the next. The sequence is always:
> >
> > N, N-1, N-2, ... 2, 1, 0, -1, N, N-1, N-2, ...
> >
> > But at the first reload, the present driver does this instead:
> >
> > N, N-1, N-2, ... 2, 1, 0, -1, N-1, N-2, ...
> >
> > Fix this deviation for both timer 1 and timer 2, and allow for the fact
> > that on a real 6522 the timer 2 counter is not reloaded when it wraps.
> >
> > Signed-off-by: Finn Thain <fthain@linux-m68k.org>
> > ---
> > hw/misc/mos6522.c | 19 +++++++++++--------
> > 1 file changed, 11 insertions(+), 8 deletions(-)
> >
> > diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
> > index 5b1657ac0d..0a241fe9f8 100644
> > --- a/hw/misc/mos6522.c
> > +++ b/hw/misc/mos6522.c
> > @@ -63,15 +63,16 @@ static unsigned int get_counter(MOS6522State *s,
> > MOS6522Timer *ti)
> > 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) & 0xffff;
> > + counter = ti->counter_value - d;
> > } else {
> > - counter = (d - (ti->counter_value + 1)) % (ti->latch + 2);
> > - counter = (ti->latch - counter) & 0xffff;
> > + 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) & 0xffff;
> > + counter = ti->counter_value - d;
> > }
> > - return counter;
> > + return counter & 0xffff;
> > }
> > static void set_counter(MOS6522State *s, MOS6522Timer *ti, unsigned int
> > val)
> > @@ -103,11 +104,13 @@ static int64_t get_next_irq_time(MOS6522State *s,
> > MOS6522Timer *ti,
> > /* the timer goes down from latch to -1 (period of latch + 2) */
> > if (d <= (ti->counter_value + 1)) {
> > - counter = (ti->counter_value - d) & 0xffff;
> > + counter = ti->counter_value - d;
> > } else {
> > - counter = (d - (ti->counter_value + 1)) % (ti->latch + 2);
> > - counter = (ti->latch - counter) & 0xffff;
> > + 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) {
>
> I think the code looks right, but I couldn't see an explicit reference to this
> behaviour in
> http://archive.6502.org/datasheets/mos_6522_preliminary_nov_1977.pdf.
Yes, that datasheet is missing a lot of information.
> Presumably this matches what you've observed on real hardware?
>
Yes.
^ permalink raw reply [flat|nested] 47+ messages in thread
* [RFC 10/10] hw/mos6522: Synchronize timer interrupt and timer counter
2021-08-24 10:09 [RFC 00/10] hw/mos6522: VIA timer emulation fixes and improvements Finn Thain
` (4 preceding siblings ...)
2021-08-24 10:09 ` [RFC 07/10] hw/mos6522: Fix initial timer counter reload Finn Thain
@ 2021-08-24 10:09 ` Finn Thain
2021-08-25 8:52 ` Mark Cave-Ayland
2021-08-24 10:09 ` [RFC 04/10] hw/mos6522: Rename timer callback functions Finn Thain
` (6 subsequent siblings)
12 siblings, 1 reply; 47+ messages in thread
From: Finn Thain @ 2021-08-24 10:09 UTC (permalink / raw)
To: Mark Cave-Ayland, David Gibson, Greg Kurz
Cc: qemu-ppc, Laurent Vivier, qemu-devel
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;
/**
--
2.26.3
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [RFC 10/10] hw/mos6522: Synchronize timer interrupt and timer counter
2021-08-24 10:09 ` [RFC 10/10] hw/mos6522: Synchronize timer interrupt and timer counter Finn Thain
@ 2021-08-25 8:52 ` Mark Cave-Ayland
2021-08-26 6:43 ` Finn Thain
0 siblings, 1 reply; 47+ messages in thread
From: Mark Cave-Ayland @ 2021-08-25 8:52 UTC (permalink / raw)
To: Finn Thain, David Gibson, Greg Kurz; +Cc: qemu-ppc, Laurent Vivier, qemu-devel
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.
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [RFC 10/10] hw/mos6522: Synchronize timer interrupt and timer counter
2021-08-25 8:52 ` Mark Cave-Ayland
@ 2021-08-26 6:43 ` Finn Thain
0 siblings, 0 replies; 47+ messages in thread
From: Finn Thain @ 2021-08-26 6:43 UTC (permalink / raw)
To: Mark Cave-Ayland
Cc: qemu-devel, Laurent Vivier, qemu-ppc, Greg Kurz, David Gibson
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.
^ permalink raw reply [flat|nested] 47+ messages in thread
* [RFC 04/10] hw/mos6522: Rename timer callback functions
2021-08-24 10:09 [RFC 00/10] hw/mos6522: VIA timer emulation fixes and improvements Finn Thain
` (5 preceding siblings ...)
2021-08-24 10:09 ` [RFC 10/10] hw/mos6522: Synchronize timer interrupt and timer counter Finn Thain
@ 2021-08-24 10:09 ` Finn Thain
2021-08-24 10:28 ` Philippe Mathieu-Daudé
2021-08-25 7:11 ` Mark Cave-Ayland
2021-08-24 10:09 ` [RFC 02/10] hw/mos6522: Remove get_counter_value() methods and functions Finn Thain
` (5 subsequent siblings)
12 siblings, 2 replies; 47+ messages in thread
From: Finn Thain @ 2021-08-24 10:09 UTC (permalink / raw)
To: Mark Cave-Ayland, David Gibson, Greg Kurz
Cc: qemu-ppc, Laurent Vivier, qemu-devel
This improves readability.
Signed-off-by: Finn Thain <fthain@linux-m68k.org>
---
hw/misc/mos6522.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
index 1d4a56077e..c0d6bee4cc 100644
--- a/hw/misc/mos6522.c
+++ b/hw/misc/mos6522.c
@@ -154,7 +154,7 @@ static void mos6522_timer2_update(MOS6522State *s, MOS6522Timer *ti,
}
}
-static void mos6522_timer1(void *opaque)
+static void mos6522_timer1_expired(void *opaque)
{
MOS6522State *s = opaque;
MOS6522Timer *ti = &s->timers[0];
@@ -164,7 +164,7 @@ static void mos6522_timer1(void *opaque)
mos6522_update_irq(s);
}
-static void mos6522_timer2(void *opaque)
+static void mos6522_timer2_expired(void *opaque)
{
MOS6522State *s = opaque;
MOS6522Timer *ti = &s->timers[1];
@@ -445,8 +445,10 @@ static void mos6522_init(Object *obj)
s->timers[i].index = i;
}
- s->timers[0].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, mos6522_timer1, s);
- s->timers[1].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, mos6522_timer2, s);
+ s->timers[0].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
+ mos6522_timer1_expired, s);
+ s->timers[1].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
+ mos6522_timer2_expired, s);
}
static void mos6522_finalize(Object *obj)
--
2.26.3
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [RFC 04/10] hw/mos6522: Rename timer callback functions
2021-08-24 10:09 ` [RFC 04/10] hw/mos6522: Rename timer callback functions Finn Thain
@ 2021-08-24 10:28 ` Philippe Mathieu-Daudé
2021-08-25 7:11 ` Mark Cave-Ayland
1 sibling, 0 replies; 47+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-08-24 10:28 UTC (permalink / raw)
To: Finn Thain, Mark Cave-Ayland, David Gibson, Greg Kurz
Cc: qemu-ppc, Laurent Vivier, qemu-devel
On 8/24/21 12:09 PM, Finn Thain wrote:
> This improves readability.
>
> Signed-off-by: Finn Thain <fthain@linux-m68k.org>
> ---
> hw/misc/mos6522.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC 04/10] hw/mos6522: Rename timer callback functions
2021-08-24 10:09 ` [RFC 04/10] hw/mos6522: Rename timer callback functions Finn Thain
2021-08-24 10:28 ` Philippe Mathieu-Daudé
@ 2021-08-25 7:11 ` Mark Cave-Ayland
2021-08-26 7:42 ` Philippe Mathieu-Daudé
1 sibling, 1 reply; 47+ messages in thread
From: Mark Cave-Ayland @ 2021-08-25 7:11 UTC (permalink / raw)
To: Finn Thain, David Gibson, Greg Kurz; +Cc: qemu-ppc, Laurent Vivier, qemu-devel
On 24/08/2021 11:09, Finn Thain wrote:
> This improves readability.
>
> Signed-off-by: Finn Thain <fthain@linux-m68k.org>
> ---
> hw/misc/mos6522.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
> index 1d4a56077e..c0d6bee4cc 100644
> --- a/hw/misc/mos6522.c
> +++ b/hw/misc/mos6522.c
> @@ -154,7 +154,7 @@ static void mos6522_timer2_update(MOS6522State *s, MOS6522Timer *ti,
> }
> }
>
> -static void mos6522_timer1(void *opaque)
> +static void mos6522_timer1_expired(void *opaque)
> {
> MOS6522State *s = opaque;
> MOS6522Timer *ti = &s->timers[0];
> @@ -164,7 +164,7 @@ static void mos6522_timer1(void *opaque)
> mos6522_update_irq(s);
> }
>
> -static void mos6522_timer2(void *opaque)
> +static void mos6522_timer2_expired(void *opaque)
> {
> MOS6522State *s = opaque;
> MOS6522Timer *ti = &s->timers[1];
> @@ -445,8 +445,10 @@ static void mos6522_init(Object *obj)
> s->timers[i].index = i;
> }
>
> - s->timers[0].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, mos6522_timer1, s);
> - s->timers[1].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, mos6522_timer2, s);
> + s->timers[0].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> + mos6522_timer1_expired, s);
> + s->timers[1].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> + mos6522_timer2_expired, s);
> }
>
> static void mos6522_finalize(Object *obj)
I'm not overly keen on this one: the general QEMU convention for a timer callback is
for it to be named *_timer() rather than *_expired(), so I'd prefer to keep this
consistent with the rest of the codebase.
ATB,
Mark.
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [RFC 04/10] hw/mos6522: Rename timer callback functions
2021-08-25 7:11 ` Mark Cave-Ayland
@ 2021-08-26 7:42 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 47+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-08-26 7:42 UTC (permalink / raw)
To: Mark Cave-Ayland, Finn Thain, David Gibson, Greg Kurz
Cc: qemu-ppc, Laurent Vivier, qemu-devel
On 8/25/21 9:11 AM, Mark Cave-Ayland wrote:
> On 24/08/2021 11:09, Finn Thain wrote:
>
>> This improves readability.
>>
>> Signed-off-by: Finn Thain <fthain@linux-m68k.org>
>> ---
>> hw/misc/mos6522.c | 10 ++++++----
>> 1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
>> index 1d4a56077e..c0d6bee4cc 100644
>> --- a/hw/misc/mos6522.c
>> +++ b/hw/misc/mos6522.c
>> @@ -154,7 +154,7 @@ static void mos6522_timer2_update(MOS6522State *s,
>> MOS6522Timer *ti,
>> }
>> }
>> -static void mos6522_timer1(void *opaque)
>> +static void mos6522_timer1_expired(void *opaque)
>> {
>> MOS6522State *s = opaque;
>> MOS6522Timer *ti = &s->timers[0];
>> @@ -164,7 +164,7 @@ static void mos6522_timer1(void *opaque)
>> mos6522_update_irq(s);
>> }
>> -static void mos6522_timer2(void *opaque)
>> +static void mos6522_timer2_expired(void *opaque)
>> {
>> MOS6522State *s = opaque;
>> MOS6522Timer *ti = &s->timers[1];
>> @@ -445,8 +445,10 @@ static void mos6522_init(Object *obj)
>> s->timers[i].index = i;
>> }
>> - s->timers[0].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
>> mos6522_timer1, s);
>> - s->timers[1].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
>> mos6522_timer2, s);
>> + s->timers[0].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
>> + mos6522_timer1_expired, s);
>> + s->timers[1].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
>> + mos6522_timer2_expired, s);
>> }
>> static void mos6522_finalize(Object *obj)
>
> I'm not overly keen on this one: the general QEMU convention for a timer
> callback is for it to be named *_timer() rather than *_expired(), so I'd
> prefer to keep this consistent with the rest of the codebase.
I can not find any convention, and 'git grep -A1 \ timer_new' doesn't
show any conventional pattern neither.
^ permalink raw reply [flat|nested] 47+ messages in thread
* [RFC 02/10] hw/mos6522: Remove get_counter_value() methods and functions
2021-08-24 10:09 [RFC 00/10] hw/mos6522: VIA timer emulation fixes and improvements Finn Thain
` (6 preceding siblings ...)
2021-08-24 10:09 ` [RFC 04/10] hw/mos6522: Rename timer callback functions Finn Thain
@ 2021-08-24 10:09 ` Finn Thain
2021-08-24 10:29 ` Philippe Mathieu-Daudé
2021-08-24 10:09 ` [RFC 05/10] hw/mos6522: Don't clear T1 interrupt flag on latch write Finn Thain
` (4 subsequent siblings)
12 siblings, 1 reply; 47+ messages in thread
From: Finn Thain @ 2021-08-24 10:09 UTC (permalink / raw)
To: Mark Cave-Ayland, David Gibson, Greg Kurz
Cc: qemu-ppc, Laurent Vivier, qemu-devel
This code appears to be unnecessary.
Also, these routines don't return the counter value but a time interval
between counter values, so they are misnamed.
Signed-off-by: Finn Thain <fthain@linux-m68k.org>
---
hw/misc/mos6522.c | 22 ++--------------------
1 file changed, 2 insertions(+), 20 deletions(-)
diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
index a478c1ca43..ff246b5437 100644
--- a/hw/misc/mos6522.c
+++ b/hw/misc/mos6522.c
@@ -52,23 +52,13 @@ static void mos6522_update_irq(MOS6522State *s)
}
}
-static uint64_t get_counter_value(MOS6522State *s, MOS6522Timer *ti)
-{
- MOS6522DeviceClass *mdc = MOS6522_GET_CLASS(s);
-
- if (ti->index == 0) {
- return mdc->get_timer1_counter_value(s, ti);
- } else {
- return mdc->get_timer2_counter_value(s, ti);
- }
-}
-
static unsigned int get_counter(MOS6522State *s, MOS6522Timer *ti)
{
int64_t d;
unsigned int counter;
- d = get_counter_value(s, ti);
+ d = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) - ti->load_time,
+ ti->frequency, NANOSECONDS_PER_SECOND);
if (ti->index == 0) {
/* the timer goes down from latch to -1 (period of latch + 2) */
@@ -191,12 +181,6 @@ static void mos6522_set_sr_int(MOS6522State *s)
mos6522_update_irq(s);
}
-static uint64_t mos6522_get_counter_value(MOS6522State *s, MOS6522Timer *ti)
-{
- return muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) - ti->load_time,
- ti->frequency, NANOSECONDS_PER_SECOND);
-}
-
static void mos6522_portA_write(MOS6522State *s)
{
qemu_log_mask(LOG_UNIMP, "portA_write unimplemented\n");
@@ -498,8 +482,6 @@ static void mos6522_class_init(ObjectClass *oc, void *data)
mdc->portB_write = mos6522_portB_write;
mdc->portA_write = mos6522_portA_write;
mdc->update_irq = mos6522_update_irq;
- mdc->get_timer1_counter_value = mos6522_get_counter_value;
- mdc->get_timer2_counter_value = mos6522_get_counter_value;
}
static const TypeInfo mos6522_type_info = {
--
2.26.3
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [RFC 02/10] hw/mos6522: Remove get_counter_value() methods and functions
2021-08-24 10:09 ` [RFC 02/10] hw/mos6522: Remove get_counter_value() methods and functions Finn Thain
@ 2021-08-24 10:29 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 47+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-08-24 10:29 UTC (permalink / raw)
To: Finn Thain, Mark Cave-Ayland, David Gibson, Greg Kurz
Cc: qemu-ppc, Laurent Vivier, qemu-devel
On 8/24/21 12:09 PM, Finn Thain wrote:
> This code appears to be unnecessary.
>
> Also, these routines don't return the counter value but a time interval
> between counter values, so they are misnamed.
>
> Signed-off-by: Finn Thain <fthain@linux-m68k.org>
> ---
> hw/misc/mos6522.c | 22 ++--------------------
> 1 file changed, 2 insertions(+), 20 deletions(-)
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
^ permalink raw reply [flat|nested] 47+ messages in thread
* [RFC 05/10] hw/mos6522: Don't clear T1 interrupt flag on latch write
2021-08-24 10:09 [RFC 00/10] hw/mos6522: VIA timer emulation fixes and improvements Finn Thain
` (7 preceding siblings ...)
2021-08-24 10:09 ` [RFC 02/10] hw/mos6522: Remove get_counter_value() methods and functions Finn Thain
@ 2021-08-24 10:09 ` Finn Thain
2021-08-25 7:20 ` Mark Cave-Ayland
2021-08-24 10:09 ` [RFC 03/10] hw/mos6522: Remove redundant mos6522_timer1_update() calls Finn Thain
` (3 subsequent siblings)
12 siblings, 1 reply; 47+ messages in thread
From: Finn Thain @ 2021-08-24 10:09 UTC (permalink / raw)
To: Mark Cave-Ayland, David Gibson, Greg Kurz
Cc: qemu-ppc, Laurent Vivier, qemu-devel
The Synertek datasheet says, "A write to T1L-H loads an 8-bit count value
into the latch. A read of T1L-H transfers the contents of the latch to
the data bus. Neither operation has an affect [sic] on the interrupt
flag."
Signed-off-by: Finn Thain <fthain@linux-m68k.org>
---
hw/misc/mos6522.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
index c0d6bee4cc..ffff8991f4 100644
--- a/hw/misc/mos6522.c
+++ b/hw/misc/mos6522.c
@@ -313,7 +313,6 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
break;
case VIA_REG_T1LH:
s->timers[0].latch = (s->timers[0].latch & 0xff) | (val << 8);
- s->ifr &= ~T1_INT;
break;
case VIA_REG_T2CL:
s->timers[1].latch = (s->timers[1].latch & 0xff00) | val;
--
2.26.3
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [RFC 05/10] hw/mos6522: Don't clear T1 interrupt flag on latch write
2021-08-24 10:09 ` [RFC 05/10] hw/mos6522: Don't clear T1 interrupt flag on latch write Finn Thain
@ 2021-08-25 7:20 ` Mark Cave-Ayland
2021-08-26 5:21 ` Finn Thain
0 siblings, 1 reply; 47+ messages in thread
From: Mark Cave-Ayland @ 2021-08-25 7:20 UTC (permalink / raw)
To: Finn Thain, David Gibson, Greg Kurz; +Cc: qemu-ppc, Laurent Vivier, qemu-devel
On 24/08/2021 11:09, Finn Thain wrote:
> The Synertek datasheet says, "A write to T1L-H loads an 8-bit count value
> into the latch. A read of T1L-H transfers the contents of the latch to
> the data bus. Neither operation has an affect [sic] on the interrupt
> flag."
>
> Signed-off-by: Finn Thain <fthain@linux-m68k.org>
> ---
> hw/misc/mos6522.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
> index c0d6bee4cc..ffff8991f4 100644
> --- a/hw/misc/mos6522.c
> +++ b/hw/misc/mos6522.c
> @@ -313,7 +313,6 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
> break;
> case VIA_REG_T1LH:
> s->timers[0].latch = (s->timers[0].latch & 0xff) | (val << 8);
> - s->ifr &= ~T1_INT;
> break;
> case VIA_REG_T2CL:
> s->timers[1].latch = (s->timers[1].latch & 0xff00) | val;
Hmmm. The reference document I used for QEMU's 6522 device is at
http://archive.6502.org/datasheets/mos_6522_preliminary_nov_1977.pdf and according to
page 6 and the section "Writing the Timer 1 Registers" writing to the high byte of
the latch does indeed clear the T1 interrupt flag.
Side note: there is reference in Gary Davidian's excellent CHM video that 6522s
obtained from different manufacturers had different behaviours, and there are also
web pages mentioning that 6522s integrated as part of other silicon e.g. IOSB/CUDA
also had their own bugs... :/
ATB,
Mark.
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC 05/10] hw/mos6522: Don't clear T1 interrupt flag on latch write
2021-08-25 7:20 ` Mark Cave-Ayland
@ 2021-08-26 5:21 ` Finn Thain
2021-09-01 14:32 ` Laurent Vivier
0 siblings, 1 reply; 47+ messages in thread
From: Finn Thain @ 2021-08-26 5:21 UTC (permalink / raw)
To: Mark Cave-Ayland
Cc: qemu-devel, Laurent Vivier, qemu-ppc, Greg Kurz, David Gibson
On Wed, 25 Aug 2021, Mark Cave-Ayland wrote:
> On 24/08/2021 11:09, Finn Thain wrote:
>
> > The Synertek datasheet says, "A write to T1L-H loads an 8-bit count value
> > into the latch. A read of T1L-H transfers the contents of the latch to
> > the data bus. Neither operation has an affect [sic] on the interrupt
> > flag."
> >
> > Signed-off-by: Finn Thain <fthain@linux-m68k.org>
> > ---
> > hw/misc/mos6522.c | 1 -
> > 1 file changed, 1 deletion(-)
> >
> > diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
> > index c0d6bee4cc..ffff8991f4 100644
> > --- a/hw/misc/mos6522.c
> > +++ b/hw/misc/mos6522.c
> > @@ -313,7 +313,6 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t
> > val, unsigned size)
> > break;
> > case VIA_REG_T1LH:
> > s->timers[0].latch = (s->timers[0].latch & 0xff) | (val << 8);
> > - s->ifr &= ~T1_INT;
> > break;
> > case VIA_REG_T2CL:
> > s->timers[1].latch = (s->timers[1].latch & 0xff00) | val;
>
> Hmmm. The reference document I used for QEMU's 6522 device is at
> http://archive.6502.org/datasheets/mos_6522_preliminary_nov_1977.pdf and
> according to page 6 and the section "Writing the Timer 1 Registers" writing to
> the high byte of the latch does indeed clear the T1 interrupt flag.
>
> Side note: there is reference in Gary Davidian's excellent CHM video that
> 6522s obtained from different manufacturers had different behaviours, and
> there are also web pages mentioning that 6522s integrated as part of other
> silicon e.g. IOSB/CUDA also had their own bugs... :/
>
The MOS document you've cited is much older than the Synertek and Rockwell
devices. The datasheets for the Synertek and Rockwell parts disagree with
MOS about T1LH behaviour. Apple certainly used SY6522 devices in my Mac II
and I'd assumed Apple would have used compatible logic cores in the custom
ICs found in later models. But I don't really trust assumptions and
datasheets so I wrote the Linux patch below and ran it on my Quadra 630.
diff --git a/arch/m68k/mac/via.c b/arch/m68k/mac/via.c
index 3d11d6219cdd..ed41f6ae2bf2 100644
--- a/arch/m68k/mac/via.c
+++ b/arch/m68k/mac/via.c
@@ -634,3 +634,27 @@ static u64 mac_read_clk(struct clocksource *cs)
return ticks;
}
+
+static int baz(void)
+{
+ u8 a, b, c;
+
+ local_irq_disable();
+
+ while (!(via1[vIFR] & VIA_TIMER_1_INT))
+ continue;
+ a = via1[vIFR] & VIA_TIMER_1_INT;
+ via1[vT1LH] = via1[vT1LH];
+ b = via1[vIFR] & VIA_TIMER_1_INT;
+ via1[vT1LL] = via1[vT1LL];
+ c = via1[vIFR] & VIA_TIMER_1_INT;
+
+ printk("a == %2x\n", a);
+ printk("b == %2x\n", b);
+ printk("c == %2x\n", c);
+
+ local_irq_enable();
+
+ return 0;
+}
+late_initcall(baz);
Based on the Synertek datasheet* one would expect to see b equal to a but
I got this result instead:
[ 10.450000] a == 40
[ 10.450000] b == 0
[ 10.450000] c == 0
This amounts to a MOS design flaw and I doubt that this result from my
Quadra 630 would apply to other Mac models. So it would be great to see
the output from a Quadra 800. But until then, let's disregard this patch.
* http://archive.6502.org/datasheets/synertek_sy6522.pdf
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [RFC 05/10] hw/mos6522: Don't clear T1 interrupt flag on latch write
2021-08-26 5:21 ` Finn Thain
@ 2021-09-01 14:32 ` Laurent Vivier
2021-09-01 22:26 ` Finn Thain
0 siblings, 1 reply; 47+ messages in thread
From: Laurent Vivier @ 2021-09-01 14:32 UTC (permalink / raw)
To: Finn Thain, Mark Cave-Ayland
Cc: qemu-devel, qemu-ppc, Greg Kurz, David Gibson
Le 26/08/2021 à 07:21, Finn Thain a écrit :
> On Wed, 25 Aug 2021, Mark Cave-Ayland wrote:
>
>> On 24/08/2021 11:09, Finn Thain wrote:
>>
>>> The Synertek datasheet says, "A write to T1L-H loads an 8-bit count value
>>> into the latch. A read of T1L-H transfers the contents of the latch to
>>> the data bus. Neither operation has an affect [sic] on the interrupt
>>> flag."
>>>
>>> Signed-off-by: Finn Thain <fthain@linux-m68k.org>
>>> ---
>>> hw/misc/mos6522.c | 1 -
>>> 1 file changed, 1 deletion(-)
>>>
>>> diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
>>> index c0d6bee4cc..ffff8991f4 100644
>>> --- a/hw/misc/mos6522.c
>>> +++ b/hw/misc/mos6522.c
>>> @@ -313,7 +313,6 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t
>>> val, unsigned size)
>>> break;
>>> case VIA_REG_T1LH:
>>> s->timers[0].latch = (s->timers[0].latch & 0xff) | (val << 8);
>>> - s->ifr &= ~T1_INT;
>>> break;
>>> case VIA_REG_T2CL:
>>> s->timers[1].latch = (s->timers[1].latch & 0xff00) | val;
>>
>> Hmmm. The reference document I used for QEMU's 6522 device is at
>> http://archive.6502.org/datasheets/mos_6522_preliminary_nov_1977.pdf and
>> according to page 6 and the section "Writing the Timer 1 Registers" writing to
>> the high byte of the latch does indeed clear the T1 interrupt flag.
>>
>> Side note: there is reference in Gary Davidian's excellent CHM video that
>> 6522s obtained from different manufacturers had different behaviours, and
>> there are also web pages mentioning that 6522s integrated as part of other
>> silicon e.g. IOSB/CUDA also had their own bugs... :/
>>
>
> The MOS document you've cited is much older than the Synertek and Rockwell
> devices. The datasheets for the Synertek and Rockwell parts disagree with
> MOS about T1LH behaviour. Apple certainly used SY6522 devices in my Mac II
> and I'd assumed Apple would have used compatible logic cores in the custom
> ICs found in later models. But I don't really trust assumptions and
> datasheets so I wrote the Linux patch below and ran it on my Quadra 630.
>
> diff --git a/arch/m68k/mac/via.c b/arch/m68k/mac/via.c
> index 3d11d6219cdd..ed41f6ae2bf2 100644
> --- a/arch/m68k/mac/via.c
> +++ b/arch/m68k/mac/via.c
> @@ -634,3 +634,27 @@ static u64 mac_read_clk(struct clocksource *cs)
>
> return ticks;
> }
> +
> +static int baz(void)
> +{
> + u8 a, b, c;
> +
> + local_irq_disable();
> +
> + while (!(via1[vIFR] & VIA_TIMER_1_INT))
> + continue;
> + a = via1[vIFR] & VIA_TIMER_1_INT;
> + via1[vT1LH] = via1[vT1LH];
> + b = via1[vIFR] & VIA_TIMER_1_INT;
> + via1[vT1LL] = via1[vT1LL];
> + c = via1[vIFR] & VIA_TIMER_1_INT;
> +
> + printk("a == %2x\n", a);
> + printk("b == %2x\n", b);
> + printk("c == %2x\n", c);
> +
> + local_irq_enable();
> +
> + return 0;
> +}
> +late_initcall(baz);
>
> Based on the Synertek datasheet* one would expect to see b equal to a but
> I got this result instead:
>
> [ 10.450000] a == 40
> [ 10.450000] b == 0
> [ 10.450000] c == 0
>
> This amounts to a MOS design flaw and I doubt that this result from my
> Quadra 630 would apply to other Mac models. So it would be great to see
> the output from a Quadra 800. But until then, let's disregard this patch.
>
> * http://archive.6502.org/datasheets/synertek_sy6522.pdf
>
Tested on my Quadra 800:
[ 4.730000] a == 40
[ 4.730000] b == 0
[ 4.730000] c == 0
Laurent
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [RFC 05/10] hw/mos6522: Don't clear T1 interrupt flag on latch write
2021-09-01 14:32 ` Laurent Vivier
@ 2021-09-01 22:26 ` Finn Thain
0 siblings, 0 replies; 47+ messages in thread
From: Finn Thain @ 2021-09-01 22:26 UTC (permalink / raw)
To: Laurent Vivier
Cc: qemu-devel, qemu-ppc, Mark Cave-Ayland, Greg Kurz, David Gibson
[-- Attachment #1: Type: text/plain, Size: 3807 bytes --]
On Wed, 1 Sep 2021, Laurent Vivier wrote:
> Le 26/08/2021 à 07:21, Finn Thain a écrit :
> > On Wed, 25 Aug 2021, Mark Cave-Ayland wrote:
> >
> >> On 24/08/2021 11:09, Finn Thain wrote:
> >>
> >>> The Synertek datasheet says, "A write to T1L-H loads an 8-bit count value
> >>> into the latch. A read of T1L-H transfers the contents of the latch to
> >>> the data bus. Neither operation has an affect [sic] on the interrupt
> >>> flag."
> >>>
> >>> Signed-off-by: Finn Thain <fthain@linux-m68k.org>
> >>> ---
> >>> hw/misc/mos6522.c | 1 -
> >>> 1 file changed, 1 deletion(-)
> >>>
> >>> diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
> >>> index c0d6bee4cc..ffff8991f4 100644
> >>> --- a/hw/misc/mos6522.c
> >>> +++ b/hw/misc/mos6522.c
> >>> @@ -313,7 +313,6 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t
> >>> val, unsigned size)
> >>> break;
> >>> case VIA_REG_T1LH:
> >>> s->timers[0].latch = (s->timers[0].latch & 0xff) | (val << 8);
> >>> - s->ifr &= ~T1_INT;
> >>> break;
> >>> case VIA_REG_T2CL:
> >>> s->timers[1].latch = (s->timers[1].latch & 0xff00) | val;
> >>
> >> Hmmm. The reference document I used for QEMU's 6522 device is at
> >> http://archive.6502.org/datasheets/mos_6522_preliminary_nov_1977.pdf and
> >> according to page 6 and the section "Writing the Timer 1 Registers" writing to
> >> the high byte of the latch does indeed clear the T1 interrupt flag.
> >>
> >> Side note: there is reference in Gary Davidian's excellent CHM video that
> >> 6522s obtained from different manufacturers had different behaviours, and
> >> there are also web pages mentioning that 6522s integrated as part of other
> >> silicon e.g. IOSB/CUDA also had their own bugs... :/
> >>
> >
> > The MOS document you've cited is much older than the Synertek and Rockwell
> > devices. The datasheets for the Synertek and Rockwell parts disagree with
> > MOS about T1LH behaviour. Apple certainly used SY6522 devices in my Mac II
> > and I'd assumed Apple would have used compatible logic cores in the custom
> > ICs found in later models. But I don't really trust assumptions and
> > datasheets so I wrote the Linux patch below and ran it on my Quadra 630.
> >
> > diff --git a/arch/m68k/mac/via.c b/arch/m68k/mac/via.c
> > index 3d11d6219cdd..ed41f6ae2bf2 100644
> > --- a/arch/m68k/mac/via.c
> > +++ b/arch/m68k/mac/via.c
> > @@ -634,3 +634,27 @@ static u64 mac_read_clk(struct clocksource *cs)
> >
> > return ticks;
> > }
> > +
> > +static int baz(void)
> > +{
> > + u8 a, b, c;
> > +
> > + local_irq_disable();
> > +
> > + while (!(via1[vIFR] & VIA_TIMER_1_INT))
> > + continue;
> > + a = via1[vIFR] & VIA_TIMER_1_INT;
> > + via1[vT1LH] = via1[vT1LH];
> > + b = via1[vIFR] & VIA_TIMER_1_INT;
> > + via1[vT1LL] = via1[vT1LL];
> > + c = via1[vIFR] & VIA_TIMER_1_INT;
> > +
> > + printk("a == %2x\n", a);
> > + printk("b == %2x\n", b);
> > + printk("c == %2x\n", c);
> > +
> > + local_irq_enable();
> > +
> > + return 0;
> > +}
> > +late_initcall(baz);
> >
> > Based on the Synertek datasheet* one would expect to see b equal to a but
> > I got this result instead:
> >
> > [ 10.450000] a == 40
> > [ 10.450000] b == 0
> > [ 10.450000] c == 0
> >
> > This amounts to a MOS design flaw and I doubt that this result from my
> > Quadra 630 would apply to other Mac models. So it would be great to see
> > the output from a Quadra 800. But until then, let's disregard this patch.
> >
> > * http://archive.6502.org/datasheets/synertek_sy6522.pdf
> >
>
> Tested on my Quadra 800:
>
> [ 4.730000] a == 40
> [ 4.730000] b == 0
> [ 4.730000] c == 0
>
Much appreciated. I will drop this patch.
^ permalink raw reply [flat|nested] 47+ messages in thread
* [RFC 03/10] hw/mos6522: Remove redundant mos6522_timer1_update() calls
2021-08-24 10:09 [RFC 00/10] hw/mos6522: VIA timer emulation fixes and improvements Finn Thain
` (8 preceding siblings ...)
2021-08-24 10:09 ` [RFC 05/10] hw/mos6522: Don't clear T1 interrupt flag on latch write Finn Thain
@ 2021-08-24 10:09 ` Finn Thain
2021-08-25 7:09 ` Mark Cave-Ayland
2021-08-24 10:34 ` [RFC 00/10] hw/mos6522: VIA timer emulation fixes and improvements Philippe Mathieu-Daudé
` (2 subsequent siblings)
12 siblings, 1 reply; 47+ messages in thread
From: Finn Thain @ 2021-08-24 10:09 UTC (permalink / raw)
To: Mark Cave-Ayland, David Gibson, Greg Kurz
Cc: qemu-ppc, Laurent Vivier, qemu-devel
Reads and writes to the TL and TC registers have no immediate effect on
a running timer, with the exception of a write to TCH. Hence these
mos6522_timer_update() calls are not needed.
Signed-off-by: Finn Thain <fthain@linux-m68k.org>
---
hw/misc/mos6522.c | 7 -------
1 file changed, 7 deletions(-)
diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
index ff246b5437..1d4a56077e 100644
--- a/hw/misc/mos6522.c
+++ b/hw/misc/mos6522.c
@@ -234,7 +234,6 @@ uint64_t mos6522_read(void *opaque, hwaddr addr, unsigned size)
val = s->timers[0].latch & 0xff;
break;
case VIA_REG_T1LH:
- /* XXX: check this */
val = (s->timers[0].latch >> 8) & 0xff;
break;
case VIA_REG_T2CL:
@@ -303,8 +302,6 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
break;
case VIA_REG_T1CL:
s->timers[0].latch = (s->timers[0].latch & 0xff00) | val;
- mos6522_timer1_update(s, &s->timers[0],
- qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
break;
case VIA_REG_T1CH:
s->timers[0].latch = (s->timers[0].latch & 0xff) | (val << 8);
@@ -313,14 +310,10 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
break;
case VIA_REG_T1LL:
s->timers[0].latch = (s->timers[0].latch & 0xff00) | val;
- mos6522_timer1_update(s, &s->timers[0],
- qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
break;
case VIA_REG_T1LH:
s->timers[0].latch = (s->timers[0].latch & 0xff) | (val << 8);
s->ifr &= ~T1_INT;
- mos6522_timer1_update(s, &s->timers[0],
- qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
break;
case VIA_REG_T2CL:
s->timers[1].latch = (s->timers[1].latch & 0xff00) | val;
--
2.26.3
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [RFC 03/10] hw/mos6522: Remove redundant mos6522_timer1_update() calls
2021-08-24 10:09 ` [RFC 03/10] hw/mos6522: Remove redundant mos6522_timer1_update() calls Finn Thain
@ 2021-08-25 7:09 ` Mark Cave-Ayland
0 siblings, 0 replies; 47+ messages in thread
From: Mark Cave-Ayland @ 2021-08-25 7:09 UTC (permalink / raw)
To: Finn Thain, David Gibson, Greg Kurz; +Cc: qemu-ppc, Laurent Vivier, qemu-devel
On 24/08/2021 11:09, Finn Thain wrote:
> Reads and writes to the TL and TC registers have no immediate effect on
> a running timer, with the exception of a write to TCH. Hence these
> mos6522_timer_update() calls are not needed.
>
> Signed-off-by: Finn Thain <fthain@linux-m68k.org>
Perhaps better to flip this description around i.e. mention that the low bytes are
written to a latch and then the full 16-bit value is transferred to the latch/counter
when the high byte is written?
Otherwise I think this looks okay.
> ---
> hw/misc/mos6522.c | 7 -------
> 1 file changed, 7 deletions(-)
>
> diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
> index ff246b5437..1d4a56077e 100644
> --- a/hw/misc/mos6522.c
> +++ b/hw/misc/mos6522.c
> @@ -234,7 +234,6 @@ uint64_t mos6522_read(void *opaque, hwaddr addr, unsigned size)
> val = s->timers[0].latch & 0xff;
> break;
> case VIA_REG_T1LH:
> - /* XXX: check this */
> val = (s->timers[0].latch >> 8) & 0xff;
> break;
> case VIA_REG_T2CL:
> @@ -303,8 +302,6 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
> break;
> case VIA_REG_T1CL:
> s->timers[0].latch = (s->timers[0].latch & 0xff00) | val;
> - mos6522_timer1_update(s, &s->timers[0],
> - qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
> break;
> case VIA_REG_T1CH:
> s->timers[0].latch = (s->timers[0].latch & 0xff) | (val << 8);
> @@ -313,14 +310,10 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
> break;
> case VIA_REG_T1LL:
> s->timers[0].latch = (s->timers[0].latch & 0xff00) | val;
> - mos6522_timer1_update(s, &s->timers[0],
> - qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
> break;
> case VIA_REG_T1LH:
> s->timers[0].latch = (s->timers[0].latch & 0xff) | (val << 8);
> s->ifr &= ~T1_INT;
> - mos6522_timer1_update(s, &s->timers[0],
> - qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
> break;
> case VIA_REG_T2CL:
> s->timers[1].latch = (s->timers[1].latch & 0xff00) | val;
>
ATB,
Mark.
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC 00/10] hw/mos6522: VIA timer emulation fixes and improvements
2021-08-24 10:09 [RFC 00/10] hw/mos6522: VIA timer emulation fixes and improvements Finn Thain
` (9 preceding siblings ...)
2021-08-24 10:09 ` [RFC 03/10] hw/mos6522: Remove redundant mos6522_timer1_update() calls Finn Thain
@ 2021-08-24 10:34 ` Philippe Mathieu-Daudé
2021-08-28 1:22 ` Finn Thain
2021-08-25 3:11 ` David Gibson
2021-08-25 9:10 ` Mark Cave-Ayland
12 siblings, 1 reply; 47+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-08-24 10:34 UTC (permalink / raw)
To: Finn Thain, David Gibson, Greg Kurz, Mark Cave-Ayland
Cc: Peter Maydell, qemu-ppc, Laurent Vivier, qemu-devel
On 8/24/21 12:09 PM, Finn Thain wrote:
> On a real Quadra, accesses to the SY6522 chips are slow because they are
> synchronous with the 783360 Hz "phase 2" clock. In QEMU, they are slow
> only because of the division operation in the timer count calculation.
>
> This patch series improves the fidelity of the emulated chip, but the
> price is more division ops. I haven't tried to measure this yet.
>
> The emulated 6522 still deviates from the behaviour of the real thing,
> however. For example, two consecutive accesses to a real 6522 timer
> counter can never yield the same value. This is not true of the 6522 in
> QEMU 6 wherein two consecutive accesses to a timer count register have
> been observed to yield the same value.
>
> Linux is not particularly robust in the face of a 6522 that deviates
> from the usual behaviour. The problem presently affecting a Linux guest
> is that its 'via' clocksource is prone to monotonicity failure. That is,
> the clocksource counter can jump backwards. This can be observed by
> patching Linux like so:
>
> diff --git a/arch/m68k/mac/via.c b/arch/m68k/mac/via.c
> --- a/arch/m68k/mac/via.c
> +++ b/arch/m68k/mac/via.c
> @@ -606,6 +606,8 @@ void __init via_init_clock(void)
> clocksource_register_hz(&mac_clk, VIA_CLOCK_FREQ);
> }
>
> +static u32 prev_ticks;
> +
> static u64 mac_read_clk(struct clocksource *cs)
> {
> unsigned long flags;
> @@ -631,6 +633,8 @@ static u64 mac_read_clk(struct clocksource *cs)
> count = count_high << 8;
> ticks = VIA_TIMER_CYCLES - count;
> ticks += clk_offset + clk_total;
> +if (ticks < prev_ticks) pr_warn("%s: %u < %u\n", __func__, ticks, prev_ticks);
> +prev_ticks = ticks;
> local_irq_restore(flags);
>
> return ticks;
>
> This problem can be partly blamed on a 6522 design limitation, which is
> that the timer counter has no overflow register. Hence, if a timer
> counter wraps around and the kernel is late to handle the subsequent
> interrupt, the kernel can't account for any missed ticks.
>
> On a real Quadra, the kernel mitigates this limitation by minimizing
> interrupt latency. But on QEMU, interrupt latency is unbounded. This
> can't be mitigated by the guest kernel at all and leads to clock drift.
> This can be observed by patching QEMU like so:
>
> diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
> --- a/hw/misc/mos6522.c
> +++ b/hw/misc/mos6522.c
> @@ -379,6 +379,12 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
> s->pcr = val;
> break;
> case VIA_REG_IFR:
> + if (val & T1_INT) {
> + static int64_t last_t1_int_cleared;
> + int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> + if (now - last_t1_int_cleared > 20000000) printf("\t%s: t1 int clear is late\n", __func__);
> + last_t1_int_cleared = now;
> + }
> /* reset bits */
> s->ifr &= ~val;
> mos6522_update_irq(s);
>
> This logic asserts that, given that Linux/m68k sets CONFIG_HZ to 100,
> the emulator will theoretically see each timer 1 interrupt cleared
> within 20 ms of the previous one. But that deadline is often missed on
> my QEMU host [4].
I wonder if using QEMU ptimer wouldn't help here, instead of
calling qemu_clock_get_ns() and doing the math by hand:
/* Starting to run with/setting counter to "0" won't trigger immediately,
* but after a one period for both oneshot and periodic modes. */
#define PTIMER_POLICY_NO_IMMEDIATE_TRIGGER (1 << 2)
/* Starting to run with/setting counter to "0" won't re-load counter
* immediately, but after a one period. */
#define PTIMER_POLICY_NO_IMMEDIATE_RELOAD (1 << 3)
/* Make counter value of the running timer represent the actual value and
* not the one less. */
#define PTIMER_POLICY_NO_COUNTER_ROUND_DOWN (1 << 4)
> On real Mac hardware you could observe the same scenario if a high
> priority interrupt were to sufficiently delay the timer interrupt
> handler. (This is the reason why the VIA1 interrupt priority gets
> increased from level 1 to level 5 when running on Quadras.)
>
> Anyway, for now, the clocksource monotonicity problem in Linux/mac68k
> guests is still unresolved. Nonetheless, I think this patch series does
> improve the situation.
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [RFC 00/10] hw/mos6522: VIA timer emulation fixes and improvements
2021-08-24 10:34 ` [RFC 00/10] hw/mos6522: VIA timer emulation fixes and improvements Philippe Mathieu-Daudé
@ 2021-08-28 1:22 ` Finn Thain
2021-08-31 21:14 ` Mark Cave-Ayland
0 siblings, 1 reply; 47+ messages in thread
From: Finn Thain @ 2021-08-28 1:22 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Peter Maydell, Mark Cave-Ayland, qemu-devel, Laurent Vivier,
Greg Kurz, qemu-ppc, David Gibson
[-- Attachment #1: Type: text/plain, Size: 4766 bytes --]
On Tue, 24 Aug 2021, Philippe Mathieu-Daudé wrote:
> On 8/24/21 12:09 PM, Finn Thain wrote:
>
> > On a real Quadra, accesses to the SY6522 chips are slow because they are
> > synchronous with the 783360 Hz "phase 2" clock. In QEMU, they are slow
> > only because of the division operation in the timer count calculation.
> >
> > This patch series improves the fidelity of the emulated chip, but the
> > price is more division ops. I haven't tried to measure this yet.
> >
> > The emulated 6522 still deviates from the behaviour of the real thing,
> > however. For example, two consecutive accesses to a real 6522 timer
> > counter can never yield the same value. This is not true of the 6522 in
> > QEMU 6 wherein two consecutive accesses to a timer count register have
> > been observed to yield the same value.
> >
> > Linux is not particularly robust in the face of a 6522 that deviates
> > from the usual behaviour. The problem presently affecting a Linux guest
> > is that its 'via' clocksource is prone to monotonicity failure. That is,
> > the clocksource counter can jump backwards. This can be observed by
> > patching Linux like so:
> >
> > diff --git a/arch/m68k/mac/via.c b/arch/m68k/mac/via.c
> > --- a/arch/m68k/mac/via.c
> > +++ b/arch/m68k/mac/via.c
> > @@ -606,6 +606,8 @@ void __init via_init_clock(void)
> > clocksource_register_hz(&mac_clk, VIA_CLOCK_FREQ);
> > }
> >
> > +static u32 prev_ticks;
> > +
> > static u64 mac_read_clk(struct clocksource *cs)
> > {
> > unsigned long flags;
> > @@ -631,6 +633,8 @@ static u64 mac_read_clk(struct clocksource *cs)
> > count = count_high << 8;
> > ticks = VIA_TIMER_CYCLES - count;
> > ticks += clk_offset + clk_total;
> > +if (ticks < prev_ticks) pr_warn("%s: %u < %u\n", __func__, ticks, prev_ticks);
> > +prev_ticks = ticks;
> > local_irq_restore(flags);
> >
> > return ticks;
> >
> > This problem can be partly blamed on a 6522 design limitation, which is
> > that the timer counter has no overflow register. Hence, if a timer
> > counter wraps around and the kernel is late to handle the subsequent
> > interrupt, the kernel can't account for any missed ticks.
> >
> > On a real Quadra, the kernel mitigates this limitation by minimizing
> > interrupt latency. But on QEMU, interrupt latency is unbounded. This
> > can't be mitigated by the guest kernel at all and leads to clock drift.
> > This can be observed by patching QEMU like so:
> >
> > diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
> > --- a/hw/misc/mos6522.c
> > +++ b/hw/misc/mos6522.c
> > @@ -379,6 +379,12 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
> > s->pcr = val;
> > break;
> > case VIA_REG_IFR:
> > + if (val & T1_INT) {
> > + static int64_t last_t1_int_cleared;
> > + int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> > + if (now - last_t1_int_cleared > 20000000) printf("\t%s: t1 int clear is late\n", __func__);
> > + last_t1_int_cleared = now;
> > + }
> > /* reset bits */
> > s->ifr &= ~val;
> > mos6522_update_irq(s);
> >
> > This logic asserts that, given that Linux/m68k sets CONFIG_HZ to 100,
> > the emulator will theoretically see each timer 1 interrupt cleared
> > within 20 ms of the previous one. But that deadline is often missed on
> > my QEMU host [4].
>
> I wonder if using QEMU ptimer wouldn't help here, instead of
> calling qemu_clock_get_ns() and doing the math by hand:
>
> /* Starting to run with/setting counter to "0" won't trigger immediately,
> * but after a one period for both oneshot and periodic modes. */
> #define PTIMER_POLICY_NO_IMMEDIATE_TRIGGER (1 << 2)
>
> /* Starting to run with/setting counter to "0" won't re-load counter
> * immediately, but after a one period. */
> #define PTIMER_POLICY_NO_IMMEDIATE_RELOAD (1 << 3)
>
> /* Make counter value of the running timer represent the actual value and
> * not the one less. */
> #define PTIMER_POLICY_NO_COUNTER_ROUND_DOWN (1 << 4)
>
It's often the case that a conversion to a new API is trivial for someone
who understands that API. But I still haven't got my head around the
implementation (hw/core/ptimer.c).
So I agree the ptimer API could simplify mos6522.c but adopting it is not
trivial for me.
QEMU's 6522 device does not attempt to model the relationship between the
"phase two" clock and timer counters and timer interrupts. I haven't
attempted to fix these deviations at all, as that's not trivial either.
But using the ptimer API could potentially make it easier to address those
fidelity issues.
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [RFC 00/10] hw/mos6522: VIA timer emulation fixes and improvements
2021-08-28 1:22 ` Finn Thain
@ 2021-08-31 21:14 ` Mark Cave-Ayland
2021-08-31 22:44 ` Finn Thain
2021-09-01 2:20 ` Finn Thain
0 siblings, 2 replies; 47+ messages in thread
From: Mark Cave-Ayland @ 2021-08-31 21:14 UTC (permalink / raw)
To: Finn Thain, Philippe Mathieu-Daudé
Cc: Peter Maydell, Greg Kurz, qemu-devel, Laurent Vivier, qemu-ppc,
David Gibson
On 28/08/2021 02:22, Finn Thain wrote:
>> On 8/24/21 12:09 PM, Finn Thain wrote:
>>
>>> On a real Quadra, accesses to the SY6522 chips are slow because they are
>>> synchronous with the 783360 Hz "phase 2" clock. In QEMU, they are slow
>>> only because of the division operation in the timer count calculation.
>>>
>>> This patch series improves the fidelity of the emulated chip, but the
>>> price is more division ops. I haven't tried to measure this yet.
>>>
>>> The emulated 6522 still deviates from the behaviour of the real thing,
>>> however. For example, two consecutive accesses to a real 6522 timer
>>> counter can never yield the same value. This is not true of the 6522 in
>>> QEMU 6 wherein two consecutive accesses to a timer count register have
>>> been observed to yield the same value.
>>>
>>> Linux is not particularly robust in the face of a 6522 that deviates
>>> from the usual behaviour. The problem presently affecting a Linux guest
>>> is that its 'via' clocksource is prone to monotonicity failure. That is,
>>> the clocksource counter can jump backwards. This can be observed by
>>> patching Linux like so:
>>>
>>> diff --git a/arch/m68k/mac/via.c b/arch/m68k/mac/via.c
>>> --- a/arch/m68k/mac/via.c
>>> +++ b/arch/m68k/mac/via.c
>>> @@ -606,6 +606,8 @@ void __init via_init_clock(void)
>>> clocksource_register_hz(&mac_clk, VIA_CLOCK_FREQ);
>>> }
>>>
>>> +static u32 prev_ticks;
>>> +
>>> static u64 mac_read_clk(struct clocksource *cs)
>>> {
>>> unsigned long flags;
>>> @@ -631,6 +633,8 @@ static u64 mac_read_clk(struct clocksource *cs)
>>> count = count_high << 8;
>>> ticks = VIA_TIMER_CYCLES - count;
>>> ticks += clk_offset + clk_total;
>>> +if (ticks < prev_ticks) pr_warn("%s: %u < %u\n", __func__, ticks, prev_ticks);
>>> +prev_ticks = ticks;
>>> local_irq_restore(flags);
>>>
>>> return ticks;
>>>
>>> This problem can be partly blamed on a 6522 design limitation, which is
>>> that the timer counter has no overflow register. Hence, if a timer
>>> counter wraps around and the kernel is late to handle the subsequent
>>> interrupt, the kernel can't account for any missed ticks.
>>>
>>> On a real Quadra, the kernel mitigates this limitation by minimizing
>>> interrupt latency. But on QEMU, interrupt latency is unbounded. This
>>> can't be mitigated by the guest kernel at all and leads to clock drift.
>>> This can be observed by patching QEMU like so:
>>>
>>> diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
>>> --- a/hw/misc/mos6522.c
>>> +++ b/hw/misc/mos6522.c
>>> @@ -379,6 +379,12 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
>>> s->pcr = val;
>>> break;
>>> case VIA_REG_IFR:
>>> + if (val & T1_INT) {
>>> + static int64_t last_t1_int_cleared;
>>> + int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>>> + if (now - last_t1_int_cleared > 20000000) printf("\t%s: t1 int clear is late\n", __func__);
>>> + last_t1_int_cleared = now;
>>> + }
>>> /* reset bits */
>>> s->ifr &= ~val;
>>> mos6522_update_irq(s);
>>>
>>> This logic asserts that, given that Linux/m68k sets CONFIG_HZ to 100,
>>> the emulator will theoretically see each timer 1 interrupt cleared
>>> within 20 ms of the previous one. But that deadline is often missed on
>>> my QEMU host [4].
>>
>> I wonder if using QEMU ptimer wouldn't help here, instead of
>> calling qemu_clock_get_ns() and doing the math by hand:
>>
>> /* Starting to run with/setting counter to "0" won't trigger immediately,
>> * but after a one period for both oneshot and periodic modes. */
>> #define PTIMER_POLICY_NO_IMMEDIATE_TRIGGER (1 << 2)
>>
>> /* Starting to run with/setting counter to "0" won't re-load counter
>> * immediately, but after a one period. */
>> #define PTIMER_POLICY_NO_IMMEDIATE_RELOAD (1 << 3)
>>
>> /* Make counter value of the running timer represent the actual value and
>> * not the one less. */
>> #define PTIMER_POLICY_NO_COUNTER_ROUND_DOWN (1 << 4)
>>
>
> It's often the case that a conversion to a new API is trivial for someone
> who understands that API. But I still haven't got my head around the
> implementation (hw/core/ptimer.c).
>
> So I agree the ptimer API could simplify mos6522.c but adopting it is not
> trivial for me.
>
> QEMU's 6522 device does not attempt to model the relationship between the
> "phase two" clock and timer counters and timer interrupts. I haven't
> attempted to fix these deviations at all, as that's not trivial either.
>
> But using the ptimer API could potentially make it easier to address those
> fidelity issues.
I had another look at the mos6522 code this evening, and certainly whilst there are
things that could be improved, I'm still puzzled as to how you would see time going
backwards:
- qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) eventually ends up calling cpu_get_clock()
where the comment for the function is "Return the monotonic time elapsed in VM"
- get_next_irq_time() calculates the counter value for the current clock, adds the
value of the counter (compensating for wraparound) and calculates the clock for the
next IRQ
- Using the current clock instead of ti->next_irq_time in the timer callbacks should
compensate for any latency when the callback is invoked
You mentioned that the OS may compensate for the fact that the 6522 doesn't have an
overflow flag: can you explain more as to how this works in Linux? Is the problem
here that even if you read the counter value in the interrupt handler to work out the
latency, the counter may still have already wrapped?
ATB,
Mark.
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [RFC 00/10] hw/mos6522: VIA timer emulation fixes and improvements
2021-08-31 21:14 ` Mark Cave-Ayland
@ 2021-08-31 22:44 ` Finn Thain
2021-09-01 7:57 ` Mark Cave-Ayland
2021-09-01 2:20 ` Finn Thain
1 sibling, 1 reply; 47+ messages in thread
From: Finn Thain @ 2021-08-31 22:44 UTC (permalink / raw)
To: Mark Cave-Ayland
Cc: Peter Maydell, qemu-devel, Greg Kurz, Laurent Vivier, qemu-ppc,
David Gibson, Philippe Mathieu-Daudé
On Tue, 31 Aug 2021, Mark Cave-Ayland wrote:
> On 28/08/2021 02:22, Finn Thain wrote:
>
> > > On 8/24/21 12:09 PM, Finn Thain wrote:
> > >
> > > > On a real Quadra, accesses to the SY6522 chips are slow because they are
> > > > synchronous with the 783360 Hz "phase 2" clock. In QEMU, they are slow
> > > > only because of the division operation in the timer count calculation.
> > > >
> > > > This patch series improves the fidelity of the emulated chip, but the
> > > > price is more division ops. I haven't tried to measure this yet.
> > > >
> > > > The emulated 6522 still deviates from the behaviour of the real thing,
> > > > however. For example, two consecutive accesses to a real 6522 timer
> > > > counter can never yield the same value. This is not true of the 6522 in
> > > > QEMU 6 wherein two consecutive accesses to a timer count register have
> > > > been observed to yield the same value.
> > > >
> > > > Linux is not particularly robust in the face of a 6522 that deviates
> > > > from the usual behaviour. The problem presently affecting a Linux guest
> > > > is that its 'via' clocksource is prone to monotonicity failure. That is,
> > > > the clocksource counter can jump backwards. This can be observed by
> > > > patching Linux like so:
> > > >
> > > > diff --git a/arch/m68k/mac/via.c b/arch/m68k/mac/via.c
> > > > --- a/arch/m68k/mac/via.c
> > > > +++ b/arch/m68k/mac/via.c
> > > > @@ -606,6 +606,8 @@ void __init via_init_clock(void)
> > > > clocksource_register_hz(&mac_clk, VIA_CLOCK_FREQ);
> > > > }
> > > > +static u32 prev_ticks;
> > > > +
> > > > static u64 mac_read_clk(struct clocksource *cs)
> > > > {
> > > > unsigned long flags;
> > > > @@ -631,6 +633,8 @@ static u64 mac_read_clk(struct clocksource *cs)
> > > > count = count_high << 8;
> > > > ticks = VIA_TIMER_CYCLES - count;
> > > > ticks += clk_offset + clk_total;
> > > > +if (ticks < prev_ticks) pr_warn("%s: %u < %u\n", __func__, ticks,
> > > > prev_ticks);
> > > > +prev_ticks = ticks;
> > > > local_irq_restore(flags);
> > > > return ticks;
> > > >
> > > > This problem can be partly blamed on a 6522 design limitation, which is
> > > > that the timer counter has no overflow register. Hence, if a timer
> > > > counter wraps around and the kernel is late to handle the subsequent
> > > > interrupt, the kernel can't account for any missed ticks.
> > > >
> > > > On a real Quadra, the kernel mitigates this limitation by minimizing
> > > > interrupt latency. But on QEMU, interrupt latency is unbounded. This
> > > > can't be mitigated by the guest kernel at all and leads to clock drift.
> > > > This can be observed by patching QEMU like so:
> > > >
> > > > diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
> > > > --- a/hw/misc/mos6522.c
> > > > +++ b/hw/misc/mos6522.c
> > > > @@ -379,6 +379,12 @@ void mos6522_write(void *opaque, hwaddr addr,
> > > > uint64_t val, unsigned size)
> > > > s->pcr = val;
> > > > break;
> > > > case VIA_REG_IFR:
> > > > + if (val & T1_INT) {
> > > > + static int64_t last_t1_int_cleared;
> > > > + int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> > > > + if (now - last_t1_int_cleared > 20000000) printf("\t%s: t1
> > > > int clear is late\n", __func__);
> > > > + last_t1_int_cleared = now;
> > > > + }
> > > > /* reset bits */
> > > > s->ifr &= ~val;
> > > > mos6522_update_irq(s);
> > > >
> > > > This logic asserts that, given that Linux/m68k sets CONFIG_HZ to 100,
> > > > the emulator will theoretically see each timer 1 interrupt cleared
> > > > within 20 ms of the previous one. But that deadline is often missed on
> > > > my QEMU host [4].
> > >
> > > I wonder if using QEMU ptimer wouldn't help here, instead of
> > > calling qemu_clock_get_ns() and doing the math by hand:
> > >
> > > /* Starting to run with/setting counter to "0" won't trigger immediately,
> > > * but after a one period for both oneshot and periodic modes. */
> > > #define PTIMER_POLICY_NO_IMMEDIATE_TRIGGER (1 << 2)
> > >
> > > /* Starting to run with/setting counter to "0" won't re-load counter
> > > * immediately, but after a one period. */
> > > #define PTIMER_POLICY_NO_IMMEDIATE_RELOAD (1 << 3)
> > >
> > > /* Make counter value of the running timer represent the actual value and
> > > * not the one less. */
> > > #define PTIMER_POLICY_NO_COUNTER_ROUND_DOWN (1 << 4)
> > >
> >
> > It's often the case that a conversion to a new API is trivial for someone
> > who understands that API. But I still haven't got my head around the
> > implementation (hw/core/ptimer.c).
> >
> > So I agree the ptimer API could simplify mos6522.c but adopting it is not
> > trivial for me.
> >
> > QEMU's 6522 device does not attempt to model the relationship between the
> > "phase two" clock and timer counters and timer interrupts. I haven't
> > attempted to fix these deviations at all, as that's not trivial either.
> >
> > But using the ptimer API could potentially make it easier to address those
> > fidelity issues.
>
> I had another look at the mos6522 code this evening, and certainly whilst
> there are things that could be improved, I'm still puzzled as to how you would
> see time going backwards:
>
I didn't say time goes backwards, I said the "clocksource counter can jump
backwards". You can easily observe this for yourself if you apply the
patch above.
> - qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) eventually ends up calling
> cpu_get_clock() where the comment for the function is "Return the
> monotonic time elapsed in VM"
>
AFAICT, QEMU_CLOCK_VIRTUAL is monotonic. I confirmed last year when
debugging mos6522.c.
> - get_next_irq_time() calculates the counter value for the current
> clock, adds the value of the counter (compensating for wraparound) and
> calculates the clock for the next IRQ
>
> - Using the current clock instead of ti->next_irq_time in the timer
> callbacks should compensate for any latency when the callback is invoked
>
That's the theory. In practice, however, there are known flaws in the
present implementation which this series addresses.
> You mentioned that the OS may compensate for the fact that the 6522
> doesn't have an overflow flag: can you explain more as to how this works
> in Linux? Is the problem here that even if you read the counter value in
> the interrupt handler to work out the latency, the counter may still
> have already wrapped?
>
Unbounded interrupt latency means that the guest kernel can not tell how
many times the timer counter wrapped between timer interrupts. There will
always be a race condition in the guest kernel.
It's possible to code around this in the kernel with a ratchet to prevent
backwards movement in the clocksource counter (I had to do something
similar for the Atari clocksource driver) but that would address only one
symptom.
And that kind of hack certainly doesn't help any other guest operating
system that's adversely affected by QEMU's design limitations (which is
behavioural simulation rather than gate-level simulation).
This patch series doesn't solve the timer problem but it does improve the
situation (for the reasons given in the cover letter).
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [RFC 00/10] hw/mos6522: VIA timer emulation fixes and improvements
2021-08-31 22:44 ` Finn Thain
@ 2021-09-01 7:57 ` Mark Cave-Ayland
2021-09-01 8:06 ` Mark Cave-Ayland
0 siblings, 1 reply; 47+ messages in thread
From: Mark Cave-Ayland @ 2021-09-01 7:57 UTC (permalink / raw)
To: Finn Thain
Cc: Peter Maydell, qemu-devel, Greg Kurz, Laurent Vivier, qemu-ppc,
David Gibson, Philippe Mathieu-Daudé
On 31/08/2021 23:44, Finn Thain wrote:
>> You mentioned that the OS may compensate for the fact that the 6522
>> doesn't have an overflow flag: can you explain more as to how this works
>> in Linux? Is the problem here that even if you read the counter value in
>> the interrupt handler to work out the latency, the counter may still
>> have already wrapped?
>>
>
> Unbounded interrupt latency means that the guest kernel can not tell how
> many times the timer counter wrapped between timer interrupts. There will
> always be a race condition in the guest kernel.
>
> It's possible to code around this in the kernel with a ratchet to prevent
> backwards movement in the clocksource counter (I had to do something
> similar for the Atari clocksource driver) but that would address only one
> symptom.
>
> And that kind of hack certainly doesn't help any other guest operating
> system that's adversely affected by QEMU's design limitations (which is
> behavioural simulation rather than gate-level simulation).
>
> This patch series doesn't solve the timer problem but it does improve the
> situation (for the reasons given in the cover letter).
I had a quick look at your via-timer branch at
https://github.com/fthain/qemu/commits/via-timer and spotted that your work is based
upon the v6.0 release. Before digging further into this, can you try using vanilla
git master or the v6.1 tag instead as there are a couple of related fixes from the
6.1 development cycle that you are missing:
82ff856fe7 ("mac_via: fix 60Hz VIA1 timer interval")
- This fixed the 60Hz VIA1 timer interval (it was running 100x too fast) and so could
limit the virtual CPUs ability to process the 100Hz timer.
30ca7eddc4 ("mac_via: remove VIA1 timer optimisations")
- This fixed an issue whereby constant writes to portB would reset the 60Hz VIA1
timer and 1Hz VIA1 timer (Probably not relevant here, but worth having).
ATB,
Mark.
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [RFC 00/10] hw/mos6522: VIA timer emulation fixes and improvements
2021-09-01 7:57 ` Mark Cave-Ayland
@ 2021-09-01 8:06 ` Mark Cave-Ayland
2021-09-10 17:29 ` Mark Cave-Ayland
0 siblings, 1 reply; 47+ messages in thread
From: Mark Cave-Ayland @ 2021-09-01 8:06 UTC (permalink / raw)
To: Finn Thain
Cc: Peter Maydell, Philippe Mathieu-Daudé,
Laurent Vivier, qemu-devel, Greg Kurz, qemu-ppc, David Gibson
On 01/09/2021 08:57, Mark Cave-Ayland wrote:
> I had a quick look at your via-timer branch at
> https://github.com/fthain/qemu/commits/via-timer and spotted that your work is based
> upon the v6.0 release. Before digging further into this, can you try using vanilla
> git master or the v6.1 tag instead as there are a couple of related fixes from the
> 6.1 development cycle that you are missing:
>
> 82ff856fe7 ("mac_via: fix 60Hz VIA1 timer interval")
> - This fixed the 60Hz VIA1 timer interval (it was running 100x too fast) and so could
> limit the virtual CPUs ability to process the 100Hz timer.
>
> 30ca7eddc4 ("mac_via: remove VIA1 timer optimisations")
> - This fixed an issue whereby constant writes to portB would reset the 60Hz VIA1
> timer and 1Hz VIA1 timer (Probably not relevant here, but worth having).
Ah my mistake, clearly not enough caffeine this morning - looks like these patches
did make v6.0 after all :/
I'll have a go at some basic timer measurements using your patch to see what sort of
numbers I get for the latency here. Obviously QEMU doesn't guarantee response times
but over 20ms does seem high.
ATB,
Mark.
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [RFC 00/10] hw/mos6522: VIA timer emulation fixes and improvements
2021-09-01 8:06 ` Mark Cave-Ayland
@ 2021-09-10 17:29 ` Mark Cave-Ayland
2021-09-11 0:08 ` Finn Thain
0 siblings, 1 reply; 47+ messages in thread
From: Mark Cave-Ayland @ 2021-09-10 17:29 UTC (permalink / raw)
To: Finn Thain
Cc: Peter Maydell, qemu-devel, Greg Kurz, Philippe Mathieu-Daudé,
qemu-ppc, David Gibson
On 01/09/2021 09:06, Mark Cave-Ayland wrote:
> I'll have a go at some basic timer measurements using your patch to see what sort of
> numbers I get for the latency here. Obviously QEMU doesn't guarantee response times
> but over 20ms does seem high.
I was able to spend some time today looking at this and came up with
https://github.com/mcayland/qemu/tree/via-hacks with the aim of starting with your
patches to reduce the calls to the timer update functions (to reduce jitter) and
fixing up the places where mos6522_update_irq() isn't called.
That seemed okay, but the part I'm struggling with at the moment is removing the
re-assertion of the timer interrupt if the timer has expired when any of the
registers are read i.e.
diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
index 3c33cbebde..9884d7e178 100644
--- a/hw/misc/mos6522.c
+++ b/hw/misc/mos6522.c
@@ -229,16 +229,7 @@ uint64_t mos6522_read(void *opaque, hwaddr addr, unsigned size)
{
MOS6522State *s = opaque;
uint32_t val;
- int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
- if (now >= s->timers[0].next_irq_time) {
- mos6522_timer1_update(s, &s->timers[0], now);
- s->ifr |= T1_INT;
- }
- if (now >= s->timers[1].next_irq_time) {
- mos6522_timer2_update(s, &s->timers[1], now);
- s->ifr |= T2_INT;
- }
switch (addr) {
case VIA_REG_B:
val = s->b;
If I apply this then I see a hang in about roughly 1 in 10 boots. Poking it with a
debugger shows that the VIA1 timer interrupts are constantly being fired as IER and
IFR have the timer 1 bit set, but it is being filtered out by SR being set to 0x2700
on the CPU.
The existing code above isn't correct since T1_INT should only be asserted by the
expiry of the timer 1 via mos6522_timer1(), so I'm wondering if this is tickling a
CPU bug somewhere? In what circumstances could interrupts be disabled via SR but not
enabled again?
ATB,
Mark.
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [RFC 00/10] hw/mos6522: VIA timer emulation fixes and improvements
2021-09-10 17:29 ` Mark Cave-Ayland
@ 2021-09-11 0:08 ` Finn Thain
0 siblings, 0 replies; 47+ messages in thread
From: Finn Thain @ 2021-09-11 0:08 UTC (permalink / raw)
To: Mark Cave-Ayland
Cc: Peter Maydell, qemu-devel, Greg Kurz, Philippe Mathieu-Daudé,
qemu-ppc, David Gibson
On Fri, 10 Sep 2021, Mark Cave-Ayland wrote:
> On 01/09/2021 09:06, Mark Cave-Ayland wrote:
>
> > I'll have a go at some basic timer measurements using your patch to
> > see what sort of numbers I get for the latency here. Obviously QEMU
> > doesn't guarantee response times but over 20ms does seem high.
>
> I was able to spend some time today looking at this and came up with
> https://github.com/mcayland/qemu/tree/via-hacks with the aim of starting
> with your patches to reduce the calls to the timer update functions (to
> reduce jitter) and fixing up the places where mos6522_update_irq() isn't
> called.
>
What kind of guest was that? What impact does jitter have on that guest?
Was the jitter measurement increased, decreased or unchanged by this patch
series?
> That seemed okay, but the part I'm struggling with at the moment is
> removing the re-assertion of the timer interrupt if the timer has
> expired when any of the registers are read i.e.
>
> diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
> index 3c33cbebde..9884d7e178 100644
> --- a/hw/misc/mos6522.c
> +++ b/hw/misc/mos6522.c
> @@ -229,16 +229,7 @@ uint64_t mos6522_read(void *opaque, hwaddr addr, unsigned
> size)
> {
> MOS6522State *s = opaque;
> uint32_t val;
> - int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>
> - if (now >= s->timers[0].next_irq_time) {
> - mos6522_timer1_update(s, &s->timers[0], now);
> - s->ifr |= T1_INT;
> - }
> - if (now >= s->timers[1].next_irq_time) {
> - mos6522_timer2_update(s, &s->timers[1], now);
> - s->ifr |= T2_INT;
> - }
> switch (addr) {
> case VIA_REG_B:
> val = s->b;
>
> If I apply this then I see a hang in about roughly 1 in 10 boots. Poking
> it with a debugger shows that the VIA1 timer interrupts are constantly
> being fired as IER and IFR have the timer 1 bit set, but it is being
> filtered out by SR being set to 0x2700 on the CPU.
>
> The existing code above isn't correct since T1_INT should only be
> asserted by the expiry of the timer 1 via mos6522_timer1(), so I'm
> wondering if this is tickling a CPU bug somewhere? In what circumstances
> could interrupts be disabled via SR but not enabled again?
>
The code you're patching here was part of Laurent's commit cd8843ff25
("mos6522: fix T1 and T2 timers"). You've mentioned that elsewhere in this
thread. My response is the same as before: this patch series removes that
code so it's moot.
Please test the patch series I sent, unmodified. If you find a problem
with my code, please do report it here. I believe that you will see no
hangs at all.
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC 00/10] hw/mos6522: VIA timer emulation fixes and improvements
2021-08-31 21:14 ` Mark Cave-Ayland
2021-08-31 22:44 ` Finn Thain
@ 2021-09-01 2:20 ` Finn Thain
1 sibling, 0 replies; 47+ messages in thread
From: Finn Thain @ 2021-09-01 2:20 UTC (permalink / raw)
To: Mark Cave-Ayland
Cc: Peter Maydell, qemu-devel, Laurent Vivier, Greg Kurz, qemu-ppc,
David Gibson, Philippe Mathieu-Daudé
On Tue, 31 Aug 2021, Mark Cave-Ayland wrote:
> You mentioned that the OS may compensate for the fact that the 6522
> doesn't have an overflow flag: can you explain more as to how this works
> in Linux?
When running on real hardware, Linux/mac68k does so by
- Elevating the interrupt priority of VIA 1 so that other drivers do not
interfere with timekeeping
- Constraining intervals during which the IPL is kept elevated (i.e.
local_irq_disable/enable).
When runing on QEMU, none of that is sufficient and the Linux/mac68k
kernel can do very little to influence interrupt latency.
Linux ports to other platforms typically have multiple timers and counters
with which to implement reliable clocksource devices.
When running on other virtualization platforms, Linux may solve the
problem using a paravirtual clock device. Please see
CONFIG_PARAVIRT_CLOCK, arch/x86/include/asm/pvclock-abi.h,
arch/x86/kernel/pvclock.c, arch/x86/include/asm/vdso/gettimeofday.h
arch/x86/xen/time.c and so on.
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC 00/10] hw/mos6522: VIA timer emulation fixes and improvements
2021-08-24 10:09 [RFC 00/10] hw/mos6522: VIA timer emulation fixes and improvements Finn Thain
` (10 preceding siblings ...)
2021-08-24 10:34 ` [RFC 00/10] hw/mos6522: VIA timer emulation fixes and improvements Philippe Mathieu-Daudé
@ 2021-08-25 3:11 ` David Gibson
2021-08-25 9:10 ` Mark Cave-Ayland
12 siblings, 0 replies; 47+ messages in thread
From: David Gibson @ 2021-08-25 3:11 UTC (permalink / raw)
To: Finn Thain
Cc: qemu-devel, qemu-ppc, Mark Cave-Ayland, Greg Kurz, Laurent Vivier
[-- Attachment #1: Type: text/plain, Size: 2297 bytes --]
On Tue, Aug 24, 2021 at 08:09:36PM +1000, Finn Thain wrote:
> This is a patch series that I started last year. The aim was to try to
> get a monotonic clocksource for Linux/m68k guests. That aim hasn't been
> achieved yet (for q800 machines) but I'm submitting the patch series as
> an RFC because,
>
> - It does improve 6522 emulation fidelity.
>
> - It allows Linux/m68k to make use of the additional timer that the
> hardware indeed offers but which QEMU omits. This has several
> benefits for Linux guests [1].
>
> - I see that Mark has been working on timer emulation issues in his
> github repo [2] and it seems likely that MacOS, NetBSD or A/UX guests
> will also require better 6522 emulation.
>
> To make collaboration easier these patches can also be fetched from
> github [3].
>
> On a real Quadra, accesses to the SY6522 chips are slow because they are
> synchronous with the 783360 Hz "phase 2" clock. In QEMU, they are slow
> only because of the division operation in the timer count calculation.
>
> This patch series improves the fidelity of the emulated chip, but the
> price is more division ops. I haven't tried to measure this yet.
>
> The emulated 6522 still deviates from the behaviour of the real thing,
> however. For example, two consecutive accesses to a real 6522 timer
> counter can never yield the same value. This is not true of the 6522 in
> QEMU 6 wherein two consecutive accesses to a timer count register have
> been observed to yield the same value.
>
> Linux is not particularly robust in the face of a 6522 that deviates
> from the usual behaviour. The problem presently affecting a Linux guest
> is that its 'via' clocksource is prone to monotonicity failure. That is,
> the clocksource counter can jump backwards. This can be observed by
> patching Linux like so:
I'm afraid I know very little about Mac hardware, and almost nothing
about m68k, so I can't really review these. I have a batch of updates
for MAINTAINERS underway, which will take me off the list for this
file.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [RFC 00/10] hw/mos6522: VIA timer emulation fixes and improvements
2021-08-24 10:09 [RFC 00/10] hw/mos6522: VIA timer emulation fixes and improvements Finn Thain
` (11 preceding siblings ...)
2021-08-25 3:11 ` David Gibson
@ 2021-08-25 9:10 ` Mark Cave-Ayland
2021-08-28 4:11 ` Finn Thain
12 siblings, 1 reply; 47+ messages in thread
From: Mark Cave-Ayland @ 2021-08-25 9:10 UTC (permalink / raw)
To: Finn Thain, David Gibson, Greg Kurz; +Cc: qemu-ppc, Laurent Vivier, qemu-devel
On 24/08/2021 11:09, Finn Thain wrote:
> This is a patch series that I started last year. The aim was to try to
> get a monotonic clocksource for Linux/m68k guests. That aim hasn't been
> achieved yet (for q800 machines) but I'm submitting the patch series as
> an RFC because,
>
> - It does improve 6522 emulation fidelity.
>
> - It allows Linux/m68k to make use of the additional timer that the
> hardware indeed offers but which QEMU omits. This has several
> benefits for Linux guests [1].
>
> - I see that Mark has been working on timer emulation issues in his
> github repo [2] and it seems likely that MacOS, NetBSD or A/UX guests
> will also require better 6522 emulation.
>
> To make collaboration easier these patches can also be fetched from
> github [3].
>
> On a real Quadra, accesses to the SY6522 chips are slow because they are
> synchronous with the 783360 Hz "phase 2" clock. In QEMU, they are slow
> only because of the division operation in the timer count calculation.
>
> This patch series improves the fidelity of the emulated chip, but the
> price is more division ops. I haven't tried to measure this yet.
>
> The emulated 6522 still deviates from the behaviour of the real thing,
> however. For example, two consecutive accesses to a real 6522 timer
> counter can never yield the same value. This is not true of the 6522 in
> QEMU 6 wherein two consecutive accesses to a timer count register have
> been observed to yield the same value.
>
> Linux is not particularly robust in the face of a 6522 that deviates
> from the usual behaviour. The problem presently affecting a Linux guest
> is that its 'via' clocksource is prone to monotonicity failure. That is,
> the clocksource counter can jump backwards. This can be observed by
> patching Linux like so:
>
> diff --git a/arch/m68k/mac/via.c b/arch/m68k/mac/via.c
> --- a/arch/m68k/mac/via.c
> +++ b/arch/m68k/mac/via.c
> @@ -606,6 +606,8 @@ void __init via_init_clock(void)
> clocksource_register_hz(&mac_clk, VIA_CLOCK_FREQ);
> }
>
> +static u32 prev_ticks;
> +
> static u64 mac_read_clk(struct clocksource *cs)
> {
> unsigned long flags;
> @@ -631,6 +633,8 @@ static u64 mac_read_clk(struct clocksource *cs)
> count = count_high << 8;
> ticks = VIA_TIMER_CYCLES - count;
> ticks += clk_offset + clk_total;
> +if (ticks < prev_ticks) pr_warn("%s: %u < %u\n", __func__, ticks, prev_ticks);
> +prev_ticks = ticks;
> local_irq_restore(flags);
>
> return ticks;
>
> This problem can be partly blamed on a 6522 design limitation, which is
> that the timer counter has no overflow register. Hence, if a timer
> counter wraps around and the kernel is late to handle the subsequent
> interrupt, the kernel can't account for any missed ticks.
>
> On a real Quadra, the kernel mitigates this limitation by minimizing
> interrupt latency. But on QEMU, interrupt latency is unbounded. This
> can't be mitigated by the guest kernel at all and leads to clock drift.
> This can be observed by patching QEMU like so:
>
> diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
> --- a/hw/misc/mos6522.c
> +++ b/hw/misc/mos6522.c
> @@ -379,6 +379,12 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
> s->pcr = val;
> break;
> case VIA_REG_IFR:
> + if (val & T1_INT) {
> + static int64_t last_t1_int_cleared;
> + int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> + if (now - last_t1_int_cleared > 20000000) printf("\t%s: t1 int clear is late\n", __func__);
> + last_t1_int_cleared = now;
> + }
> /* reset bits */
> s->ifr &= ~val;
> mos6522_update_irq(s);
>
> This logic asserts that, given that Linux/m68k sets CONFIG_HZ to 100,
> the emulator will theoretically see each timer 1 interrupt cleared
> within 20 ms of the previous one. But that deadline is often missed on
> my QEMU host [4].
>
> On real Mac hardware you could observe the same scenario if a high
> priority interrupt were to sufficiently delay the timer interrupt
> handler. (This is the reason why the VIA1 interrupt priority gets
> increased from level 1 to level 5 when running on Quadras.)
>
> Anyway, for now, the clocksource monotonicity problem in Linux/mac68k
> guests is still unresolved. Nonetheless, I think this patch series does
> improve the situation.
>
> [1] I've also been working on some improvements to Linux/m68k based on
> Arnd Bergman's clockevent RFC patch,
> https://lore.kernel.org/linux-m68k/20201008154651.1901126-14-arnd@arndb.de/
> The idea is to add a oneshot clockevent device by making use of the
> second VIA1 timer. This approach should help mitigate the clock drift
> problem as well as assist with GENERIC_CLOCKEVENTS adoption.
>
> [2] https://github.com/mcayland/qemu/commits/q800.upstream
>
> [3] https://github.com/fthain/qemu/commits/via-timer/
>
> [4] This theoretical 20 ms deadline is not missed prior to every
> backwards jump in the clocksource counter. AFAICT, that's because the
> true deadline is somewhat shorter than 20 ms.
>
>
> Finn Thain (10):
> hw/mos6522: Remove get_load_time() methods and functions
> hw/mos6522: Remove get_counter_value() methods and functions
> hw/mos6522: Remove redundant mos6522_timer1_update() calls
> hw/mos6522: Rename timer callback functions
> hw/mos6522: Don't clear T1 interrupt flag on latch write
> hw/mos6522: Implement oneshot mode
> hw/mos6522: Fix initial timer counter reload
> hw/mos6522: Call mos6522_update_irq() when appropriate
> hw/mos6522: Avoid using discrepant QEMU clock values
> hw/mos6522: Synchronize timer interrupt and timer counter
>
> hw/misc/mos6522.c | 232 +++++++++++++++++---------------------
> hw/misc/trace-events | 2 +-
> include/hw/misc/mos6522.h | 9 ++
> 3 files changed, 113 insertions(+), 130 deletions(-)
I just wanted to say that this patchset is obviously the result of a huge amount of
effort trying to figure out why the clock in Linux/m68k appears to jump backwards in
QEMU, and certainly references conditions in real hardware that is not explained in
the datasheet in sufficient detail.
From my perspective I'd suggest tackling the 2 main issues first: 1) ensuring that
the clock is monotonic and 2) adding the one shot timer mode. The other fixes/updates
can then be layered on top once we're confident that the underlying timing mechanism
works not just for Linux/m68k but also for cuda on PPC.
I'm also slightly suspicious of the if() blocks introduced in mos6522_read()
introduce via commit cd8843ff25d ("mos6522: fix T1 and T2 timers").
In your comments above you mention:
> On real Mac hardware you could observe the same scenario if a high
> priority interrupt were to sufficiently delay the timer interrupt
> handler. (This is the reason why the VIA1 interrupt priority gets
> increased from level 1 to level 5 when running on Quadras.)
This isn't currently true for QEMU: if you look at hw/m68k/q800.c you can see that
the VIA interrupts are hard-wired to levels 1 and 2 respectively. You can change the
VIA1 interrupt so it is routed to level 5 instead of level 1 with the following diff:
diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
index ac0a13060b..dc8dbe5c6f 100644
--- a/hw/m68k/q800.c
+++ b/hw/m68k/q800.c
@@ -281,7 +281,7 @@ static void q800_init(MachineState *machine)
sysbus_realize_and_unref(sysbus, &error_fatal);
sysbus_mmio_map(sysbus, 0, VIA_BASE);
qdev_connect_gpio_out_named(DEVICE(sysbus), "irq", 0,
- qdev_get_gpio_in(glue, 0));
+ qdev_get_gpio_in(glue, 4));
qdev_connect_gpio_out_named(DEVICE(sysbus), "irq", 1,
qdev_get_gpio_in(glue, 1));
The q800.upstream branch goes further and implements the dynamic interrupt routing
required by A/UX but the above should be a basic test to see if the increased
priority helps with your timing issue at all.
ATB,
Mark.
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [RFC 00/10] hw/mos6522: VIA timer emulation fixes and improvements
2021-08-25 9:10 ` Mark Cave-Ayland
@ 2021-08-28 4:11 ` Finn Thain
0 siblings, 0 replies; 47+ messages in thread
From: Finn Thain @ 2021-08-28 4:11 UTC (permalink / raw)
To: Mark Cave-Ayland
Cc: qemu-devel, Laurent Vivier, qemu-ppc, Greg Kurz, David Gibson
On Wed, 25 Aug 2021, Mark Cave-Ayland wrote:
> On 24/08/2021 11:09, Finn Thain wrote:
>
> > This is a patch series that I started last year. The aim was to try to
> > get a monotonic clocksource for Linux/m68k guests. That aim hasn't been
> > achieved yet (for q800 machines) but I'm submitting the patch series as
> > an RFC because,
> >
> > - It does improve 6522 emulation fidelity.
> >
> > - It allows Linux/m68k to make use of the additional timer that the
> > hardware indeed offers but which QEMU omits. This has several
> > benefits for Linux guests [1].
> >
> > - I see that Mark has been working on timer emulation issues in his
> > github repo [2] and it seems likely that MacOS, NetBSD or A/UX guests
> > will also require better 6522 emulation.
> >
> > To make collaboration easier these patches can also be fetched from
> > github [3].
> >
> > On a real Quadra, accesses to the SY6522 chips are slow because they are
> > synchronous with the 783360 Hz "phase 2" clock. In QEMU, they are slow
> > only because of the division operation in the timer count calculation.
> >
> > This patch series improves the fidelity of the emulated chip, but the
> > price is more division ops. I haven't tried to measure this yet.
> >
> > The emulated 6522 still deviates from the behaviour of the real thing,
> > however. For example, two consecutive accesses to a real 6522 timer
> > counter can never yield the same value. This is not true of the 6522 in
> > QEMU 6 wherein two consecutive accesses to a timer count register have
> > been observed to yield the same value.
> >
> > Linux is not particularly robust in the face of a 6522 that deviates
> > from the usual behaviour. The problem presently affecting a Linux guest
> > is that its 'via' clocksource is prone to monotonicity failure. That is,
> > the clocksource counter can jump backwards. This can be observed by
> > patching Linux like so:
> >
> > diff --git a/arch/m68k/mac/via.c b/arch/m68k/mac/via.c
> > --- a/arch/m68k/mac/via.c
> > +++ b/arch/m68k/mac/via.c
> > @@ -606,6 +606,8 @@ void __init via_init_clock(void)
> > clocksource_register_hz(&mac_clk, VIA_CLOCK_FREQ);
> > }
> > +static u32 prev_ticks;
> > +
> > static u64 mac_read_clk(struct clocksource *cs)
> > {
> > unsigned long flags;
> > @@ -631,6 +633,8 @@ static u64 mac_read_clk(struct clocksource *cs)
> > count = count_high << 8;
> > ticks = VIA_TIMER_CYCLES - count;
> > ticks += clk_offset + clk_total;
> > +if (ticks < prev_ticks) pr_warn("%s: %u < %u\n", __func__, ticks,
> > prev_ticks);
> > +prev_ticks = ticks;
> > local_irq_restore(flags);
> > return ticks;
> >
> > This problem can be partly blamed on a 6522 design limitation, which is
> > that the timer counter has no overflow register. Hence, if a timer
> > counter wraps around and the kernel is late to handle the subsequent
> > interrupt, the kernel can't account for any missed ticks.
> >
> > On a real Quadra, the kernel mitigates this limitation by minimizing
> > interrupt latency. But on QEMU, interrupt latency is unbounded. This
> > can't be mitigated by the guest kernel at all and leads to clock drift.
> > This can be observed by patching QEMU like so:
> >
> > diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
> > --- a/hw/misc/mos6522.c
> > +++ b/hw/misc/mos6522.c
> > @@ -379,6 +379,12 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t
> > val, unsigned size)
> > s->pcr = val;
> > break;
> > case VIA_REG_IFR:
> > + if (val & T1_INT) {
> > + static int64_t last_t1_int_cleared;
> > + int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> > + if (now - last_t1_int_cleared > 20000000) printf("\t%s: t1 int
> > clear is late\n", __func__);
> > + last_t1_int_cleared = now;
> > + }
> > /* reset bits */
> > s->ifr &= ~val;
> > mos6522_update_irq(s);
> >
> > This logic asserts that, given that Linux/m68k sets CONFIG_HZ to 100,
> > the emulator will theoretically see each timer 1 interrupt cleared
> > within 20 ms of the previous one. But that deadline is often missed on
> > my QEMU host [4].
> >
> > On real Mac hardware you could observe the same scenario if a high
> > priority interrupt were to sufficiently delay the timer interrupt
> > handler. (This is the reason why the VIA1 interrupt priority gets
> > increased from level 1 to level 5 when running on Quadras.)
> >
> > Anyway, for now, the clocksource monotonicity problem in Linux/mac68k
> > guests is still unresolved. Nonetheless, I think this patch series does
> > improve the situation.
> >
> > [1] I've also been working on some improvements to Linux/m68k based on
> > Arnd Bergman's clockevent RFC patch,
> > https://lore.kernel.org/linux-m68k/20201008154651.1901126-14-arnd@arndb.de/
> > The idea is to add a oneshot clockevent device by making use of the
> > second VIA1 timer. This approach should help mitigate the clock drift
> > problem as well as assist with GENERIC_CLOCKEVENTS adoption.
> >
> > [2] https://github.com/mcayland/qemu/commits/q800.upstream
> >
> > [3] https://github.com/fthain/qemu/commits/via-timer/
> >
> > [4] This theoretical 20 ms deadline is not missed prior to every
> > backwards jump in the clocksource counter. AFAICT, that's because the
> > true deadline is somewhat shorter than 20 ms.
> >
> >
> > Finn Thain (10):
> > hw/mos6522: Remove get_load_time() methods and functions
> > hw/mos6522: Remove get_counter_value() methods and functions
> > hw/mos6522: Remove redundant mos6522_timer1_update() calls
> > hw/mos6522: Rename timer callback functions
> > hw/mos6522: Don't clear T1 interrupt flag on latch write
> > hw/mos6522: Implement oneshot mode
> > hw/mos6522: Fix initial timer counter reload
> > hw/mos6522: Call mos6522_update_irq() when appropriate
> > hw/mos6522: Avoid using discrepant QEMU clock values
> > hw/mos6522: Synchronize timer interrupt and timer counter
> >
> > hw/misc/mos6522.c | 232 +++++++++++++++++---------------------
> > hw/misc/trace-events | 2 +-
> > include/hw/misc/mos6522.h | 9 ++
> > 3 files changed, 113 insertions(+), 130 deletions(-)
>
> I just wanted to say that this patchset is obviously the result of a huge
> amount of effort trying to figure out why the clock in Linux/m68k appears to
> jump backwards in QEMU, and certainly references conditions in real hardware
> that is not explained in the datasheet in sufficient detail.
>
> From my perspective I'd suggest tackling the 2 main issues first: 1) ensuring
> that the clock is monotonic and 2) adding the one shot timer mode.
Well, that's how I arrived at this series. The oneshot timer mode was
needed in order to implement a oneshot clockevent device.
> The other fixes/updates can then be layered on top once we're confident
> that the underlying timing mechanism works not just for Linux/m68k but
> also for cuda on PPC.
>
Fixes normally go at the start of the series, new features at the end.
This helps review, bisection and backporting.
> I'm also slightly suspicious of the if() blocks introduced in mos6522_read()
> introduce via commit cd8843ff25d ("mos6522: fix T1 and T2 timers").
>
Fair enough... I see that you also raised this in your reply to patch
6/10; I'll have to take another look at that code and respond in that
thread. That patch was written some time ago and I've forgotten the
rationale. Perhaps Laurent will comment on commit cd8843ff25d.
> In your comments above you mention:
>
> > On real Mac hardware you could observe the same scenario if a high
> > priority interrupt were to sufficiently delay the timer interrupt
> > handler. (This is the reason why the VIA1 interrupt priority gets
> > increased from level 1 to level 5 when running on Quadras.)
>
> This isn't currently true for QEMU: if you look at hw/m68k/q800.c you can see
> that the VIA interrupts are hard-wired to levels 1 and 2 respectively. You can
> change the VIA1 interrupt so it is routed to level 5 instead of level 1 with
> the following diff:
>
> diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
> index ac0a13060b..dc8dbe5c6f 100644
> --- a/hw/m68k/q800.c
> +++ b/hw/m68k/q800.c
> @@ -281,7 +281,7 @@ static void q800_init(MachineState *machine)
> sysbus_realize_and_unref(sysbus, &error_fatal);
> sysbus_mmio_map(sysbus, 0, VIA_BASE);
> qdev_connect_gpio_out_named(DEVICE(sysbus), "irq", 0,
> - qdev_get_gpio_in(glue, 0));
> + qdev_get_gpio_in(glue, 4));
> qdev_connect_gpio_out_named(DEVICE(sysbus), "irq", 1,
> qdev_get_gpio_in(glue, 1));
>
> The q800.upstream branch goes further and implements the dynamic interrupt
> routing required by A/UX but the above should be a basic test to see if the
> increased priority helps with your timing issue at all.
>
That would only help if the handlers for interrupts having greater
priority (IPL > 1) were slow. But the problem is not that. The root cause
here is unbounded interrupt latency in general. A real quadra does not
inflict that on the kernel.
^ permalink raw reply [flat|nested] 47+ messages in thread