hw/mos6522: VIA timer emulation fixes and improvements

PATCH In review Finn Thain 3 participants 17 messages Sep 24, 2021 – Nov 22, 2021

Subsystems (2)

m68k architecture m68k on Apple macintosh

lore

Share & tools

lore

Finn Thain

author

2021-09-24 00:09 UTC #0 lore raw

cover hw/mos6522: VIA timer emulation fixes and improvements

This is a patch series for QEMU that I started last year. The aim was to try to get a monotonic clocksource for Linux/m68k guests. That hasn't been achieved yet (for q800 machines). I'm submitting the patch series because,

- it improves 6522 emulation fidelity, although slightly slower, and

- it allows Linux/m68k to make use of the additional timer that the hardware indeed offers, but which QEMU omits, and which may be of benefit to Linux guests [1], and

- 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 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.

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 emulated 6522 in QEMU 6, wherein two consecutive accesses to a timer count register have been observed to yield the same value.

Two problems presently affecting a Linux guest are clock drift and monotonicity failure in the 'via' clocksource. That is, the clocksource counter can jump backwards. This can be observed by patching Linux like so,

arch/m68k/mac/via.c +4 -0

@@ -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;

Or just enable CONFIG_DEBUG_TIMEKEEPING:

[ 1872.720000] INFO: timekeeping: Cycle offset (4294966426) is larger than the 'via1' clock's 50% safety margin (2147483647)
[ 1872.720000] timekeeping: Your kernel is still fine, but is feeling a bit nervous 
[ 1907.510000] INFO: timekeeping: Cycle offset (4294962989) is larger than the 'via1' clock's 50% safety margin (2147483647) 
[ 1907.510000] timekeeping: Your kernel is still fine, but is feeling a bit nervous 
[ 1907.900000] INFO: timekeeping: Cycle offset (4294963248) is larger than the 'via1' clock's 50% safety margin (2147483647) 
[ 1907.900000] timekeeping: Your kernel is still fine, but is feeling a bit nervous

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 and leads to clock drift.

This latency can be observed by patching QEMU like so:

hw/misc/mos6522.c +6 -0

@@ -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 6 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/[email protected]/
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 CONFIG_GENERIC_CLOCKEVENTS adoption, which would enable CONFIG_NO_HZ_IDLE etc.

[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.

---
Changed since RFC:
- Added Reviewed-by tags.
- Re-ordered some patches to make fixes available earlier in the series.
- Dropped patch 5/10 "Don't clear T1 interrupt flag on latch write".
- Rebased on v6.1.0 release.

Finn Thain (9):
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: 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/mos6522: Implement oneshot mode

 hw/misc/mos6522.c         | 245 ++++++++++++++++++--------------------
 hw/misc/trace-events      |   2 +-
 include/hw/misc/mos6522.h |   9 ++
 3 files changed, 123 insertions(+), 133 deletions(-)

Signature · 1 line

2.26.3

Finn Thain

author

2021-09-24 00:06 UTC #1 lore raw

PATCH 1/9 hw/mos6522: Remove get_load_time() methods and functions

This code appears to be unnecessary.

Signed-off-by Finn Thain <[email protected]>

Reviewed-by Philippe Mathieu-Daudé <[email protected]>

---
 hw/misc/mos6522.c | 22 +---------------------
 1 file changed, 1 insertion(+), 21 deletions(-)

hw/misc/mos6522.c +1 ~~-21~~

index 1c57332b40..a478c1ca43 100644
@@ -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 = {

Signature · 1 line

2.26.3

Finn Thain

author

2021-09-24 00:06 UTC #2 lore raw

PATCH 2/9 hw/mos6522: Remove get_counter_value() methods and functions

In reply to Finn Thain

This code appears to be unnecessary.

Signed-off-by Finn Thain <[email protected]>

Reviewed-by Philippe Mathieu-Daudé <[email protected]>

---
 hw/misc/mos6522.c | 22 ++--------------------
 1 file changed, 2 insertions(+), 20 deletions(-)

hw/misc/mos6522.c +2 ~~-20~~

index a478c1ca43..ff246b5437 100644
@@ -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 = {

Signature · 1 line

2.26.3

Finn Thain

author

2021-09-24 00:06 UTC #3 lore raw

PATCH 3/9 hw/mos6522: Remove redundant mos6522_timer1_update() calls

In reply to Finn Thain

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 <[email protected]>

---
 hw/misc/mos6522.c | 7 -------
 1 file changed, 7 deletions(-)

hw/misc/mos6522.c +0 -7

index ff246b5437..1d4a56077e 100644
@@ -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;

Signature · 1 line

2.26.3

Finn Thain

author

2021-09-24 00:09 UTC #4 lore raw

PATCH 4/9 hw/mos6522: Rename timer callback functions

In reply to Finn Thain

This improves readability.

Signed-off-by Finn Thain <[email protected]>

Reviewed-by Philippe Mathieu-Daudé <[email protected]>

---
 hw/misc/mos6522.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

hw/misc/mos6522.c +6 -4

index 1d4a56077e..c0d6bee4cc 100644
@@ -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)

Signature · 1 line

2.26.3

Finn Thain

author

2021-09-24 00:06 UTC #5 lore raw

PATCH 5/9 hw/mos6522: Fix initial timer counter reload

In reply to Finn Thain

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 <[email protected]>

---
 hw/misc/mos6522.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

hw/misc/mos6522.c +11 -8

index c0d6bee4cc..6bd60f2118 100644
@@ -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)

@@ -102,11 +103,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) {

Signature · 1 line

2.26.3

Finn Thain

author

2021-09-24 00:12 UTC #6 lore raw

PATCH 6/9 hw/mos6522: Call mos6522_update_irq() when appropriate

In reply to Finn Thain

It necessary to call mos6522_update_irq() when the interrupt flags change and unnecessary when they haven't.

Signed-off-by Finn Thain <[email protected]>

Reviewed-by Philippe Mathieu-Daudé <[email protected]>

---
 hw/misc/mos6522.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

hw/misc/mos6522.c +2 -1

index 6bd60f2118..bfe1719b18 100644
@@ -203,10 +203,12 @@ uint64_t mos6522_read(void *opaque, hwaddr addr, unsigned size)
     if (now >= s->timers[0].next_irq_time) {
         mos6522_timer1_update(s, &s->timers[0], now);
         s->ifr |= T1_INT;
+        mos6522_update_irq(s);
     }
     if (now >= s->timers[1].next_irq_time) {
         mos6522_timer2_update(s, &s->timers[1], now);
         s->ifr |= T2_INT;
+        mos6522_update_irq(s);
     }
     switch (addr) {
     case VIA_REG_B:

@@ -231,7 +233,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;

Signature · 1 line

2.26.3

Finn Thain

author

2021-09-24 00:10 UTC #7 lore raw

PATCH 7/9 hw/mos6522: Avoid using discrepant QEMU clock values

In reply to Finn Thain

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 <[email protected]>

Reviewed-by Philippe Mathieu-Daudé <[email protected]>

---
Changed since RFC
- Moved the changes to QEMUTimer callbacks to the next patch.
- Fix whitespace.

---
 hw/misc/mos6522.c | 47 +++++++++++++++++++++++------------------------
 1 file changed, 23 insertions(+), 24 deletions(-)

hw/misc/mos6522.c +23 ~~-24~~

index bfe1719b18..385ea81b62 100644
@@ -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,13 +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,
-                 ti->frequency, NANOSECONDS_PER_SECOND);
+    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) */

@@ -88,7 +87,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;

@@ -98,8 +97,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,
-                 ti->frequency, NANOSECONDS_PER_SECOND);
+    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)) {

@@ -122,20 +120,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_CONT) {
         timer_del(ti->timer);
     } else {

@@ -144,12 +141,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) {
         timer_del(ti->timer);
     } else {

@@ -227,12 +227,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;

@@ -241,12 +241,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;

@@ -356,10 +356,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();

Signature · 1 line

2.26.3

Finn Thain

author

2021-09-24 00:13 UTC #8 lore raw

PATCH 8/9 hw/mos6522: Synchronize timer interrupt and timer counter

In reply to Finn Thain

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 intended sequence of states. Document the known deviations from a real 6522 device.

Eliminate the duplication of logic in get_counter() and get_next_irq_time() by calling the former before the latter.

Call get_counter() prior to clearing interrupt flags so that the flags remain synchronized with the timer counter.

Call get_counter() prior to changing the latch so that the old latch value may be used to reload the counter. Every reload must use the value of the latch as it was at the moment of the reload (not some later value).

Remove excess calls to qemu_clock_get_ns() to help avoid the possiblity of inconsistency between timer interrupt flags and counters.

Signed-off-by Finn Thain <[email protected]>

---
Changed since RFC:
- Improved code commentary and commit log text.
- Removed qemu_clock_get_ns() call from set_counter() for simplicity.
- Moved the changes to the QEMUTimer callbacks from the previous patch into this one because they relate more to syncronization than to register accesses.

---
 hw/misc/mos6522.c         | 171 +++++++++++++++++++++-----------------
 hw/misc/trace-events      |   2 +-
 include/hw/misc/mos6522.h |   7 ++
 3 files changed, 103 insertions(+), 77 deletions(-)

hw/misc/mos6522.c +95 ~~-76~~

index 385ea81b62..8957c5e65c 100644
@@ -52,33 +52,72 @@ 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.
+     *
+     * TODO
+     * This implementation deviates from hardware behaviour because it omits
+     * the phase two clock. On a real 6522, the counter is decremented on a
+     * falling edge and the interrupt is asserted on a rising edge. Register
+     * accesses are synchronous with this clock. That means successive
+     * accesses to T1CL or T2CL can't yield the same value because
+     * they can't happen in the same clock cycle.
+     */
     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;
 }
 
-static void set_counter(MOS6522State *s, MOS6522Timer *ti, unsigned int val)
+static void set_counter(MOS6522State *s, MOS6522Timer *ti, unsigned int val,
+                        int64_t now)
 {
     trace_mos6522_set_counter(1 + ti->index, val);
-    ti->load_time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+    ti->load_time = now;
     ti->counter_value = val;
+    ti->state = decrement;
     if (ti->index == 0) {
         mos6522_timer1_update(s, ti, ti->load_time);
     } else {

@@ -89,37 +128,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;
 }

@@ -129,10 +146,8 @@ 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_CONT) {
         timer_del(ti->timer);
     } else {

@@ -146,10 +161,8 @@ 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) {
         timer_del(ti->timer);
     } else {

@@ -161,20 +174,18 @@ 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);
 
-    mos6522_timer1_update(s, ti, ti->next_irq_time);
-    s->ifr |= T1_INT;
-    mos6522_update_irq(s);
+    mos6522_timer1_update(s, ti, now);
 }
 
 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);
 
-    mos6522_timer2_update(s, ti, ti->next_irq_time);
-    s->ifr |= T2_INT;
-    mos6522_update_irq(s);
+    mos6522_timer2_update(s, ti, now);
 }
 
 static void mos6522_set_sr_int(MOS6522State *s)

@@ -200,16 +211,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) {
-        mos6522_timer1_update(s, &s->timers[0], now);
-        s->ifr |= T1_INT;
-        mos6522_update_irq(s);
-    }
-    if (now >= s->timers[1].next_irq_time) {
-        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;

@@ -228,8 +229,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;

@@ -242,8 +246,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;

@@ -283,6 +290,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 = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
 
     trace_mos6522_write(addr, val);

@@ -305,44 +313,55 @@ 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:
         s->timers[0].latch = (s->timers[0].latch & 0xff) | (val << 8);
         s->ifr &= ~T1_INT;
-        set_counter(s, &s->timers[0], s->timers[0].latch);
+        set_counter(s, &s->timers[0], s->timers[0].latch, now);
         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);
         s->ifr &= ~T1_INT;
         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:
-        /* 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);
+        set_counter(s, &s->timers[1], s->timers[1].latch, now);
         break;
     case VIA_REG_SR:
         s->sr = val;
         break;
     case VIA_REG_ACR:
         s->acr = val;
-        mos6522_timer1_update(s, &s->timers[0],
-                              qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
+        mos6522_timer1_update(s, &s->timers[0], now);
         break;
     case VIA_REG_PCR:
         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;

@@ -356,7 +375,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;

@@ -426,7 +444,8 @@ static void mos6522_reset(DeviceState *dev)
 
     s->timers[0].frequency = s->frequency;
     s->timers[0].latch = 0xffff;
-    set_counter(s, &s->timers[0], 0xffff);
+    set_counter(s, &s->timers[0], 0xffff,
+                qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
     timer_del(s->timers[0].timer);
 
     s->timers[1].frequency = s->frequency;

hw/misc/trace-events +1 -1

index ede413965b..bce49ce47c 100644
@@ -93,7 +93,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"

include/hw/misc/mos6522.h +7 -0

index fc95d22b0f..a2df5fa942 100644
@@ -71,6 +71,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

@@ -83,6 +89,7 @@ typedef struct MOS6522Timer {
     int64_t next_irq_time;
     uint64_t frequency;
     QEMUTimer *timer;
+    enum timer_state state;
 } MOS6522Timer;
 
 /**

Signature · 1 line

2.26.3

Finn Thain

author

2021-09-24 00:12 UTC #9 lore raw

PATCH 9/9 hw/mos6522: Implement oneshot mode

In reply to Finn Thain

Timer 1 has two modes: continuous interrupt and oneshot.

Signed-off-by Finn Thain <[email protected]>

---
Changed since RFC:
- Moved to end of series. This patch is quite a bit shorter here.

---
 hw/misc/mos6522.c         | 6 ++++--
 include/hw/misc/mos6522.h | 2 ++
 2 files changed, 6 insertions(+), 2 deletions(-)

hw/misc/mos6522.c +4 -2

index 8957c5e65c..bbed0b84c0 100644
@@ -148,7 +148,8 @@ static void mos6522_timer1_update(MOS6522State *s, MOS6522Timer *ti,
     }
     get_counter(s, ti, now);
     ti->next_irq_time = get_next_irq_time(s, ti, now);
-    if ((s->ier & T1_INT) == 0 || (s->acr & T1MODE) != T1MODE_CONT) {
+    if ((s->ier & T1_INT) == 0 ||
+        ((s->acr & T1MODE) == T1MODE_ONESHOT && ti->state >= irq)) {
         timer_del(ti->timer);
     } else {
         timer_mod(ti->timer, ti->next_irq_time);

@@ -163,7 +164,7 @@ static void mos6522_timer2_update(MOS6522State *s, MOS6522Timer *ti,
     }
     get_counter(s, ti, now);
     ti->next_irq_time = get_next_irq_time(s, ti, now);
-    if ((s->ier & T2_INT) == 0) {
+    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);

@@ -345,6 +346,7 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
     case VIA_REG_ACR:
         s->acr = val;
         mos6522_timer1_update(s, &s->timers[0], now);
+        mos6522_timer2_update(s, &s->timers[1], now);
         break;
     case VIA_REG_PCR:
         s->pcr = val;

include/hw/misc/mos6522.h +2 -0

index a2df5fa942..4dbba6b273 100644
@@ -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

Signature · 1 line

2.26.3

Finn Thain

author

2021-11-17 03:04 UTC #10 lore raw

In reply to Finn Thain

Quoted 7 lines · On Fri, 24 Sep 2021, I wrote:

This is a patch series for QEMU that I started last year. The aim was to try to get a monotonic clocksource for Linux/m68k guests. That hasn't been achieved yet (for q800 machines). I'm submitting the patch series because,
- it improves 6522 emulation fidelity, although slightly slower, and

I did some more benchmarking to examine the performance implications.

I measured a performance improvement with this patch series. For a Linux/m68k guest, the execution time for a gettimeofday syscall dropped from 9 us to 5 us. (This is a fairly common syscall.)

The host CPU time consumed by qemu in starting the guest kernel and executing a benchmark involving 20 million gettimeofday calls was as follows.

qemu-system-m68k mainline (42f6c9179b):

    real     198 s
    sys      123 s
    user     73 s
    sys/user 1.68

qemu-system-m68k patched (0a0bca4711):

    real     112 s
    sys      63 s
    user     47 s
    sys/user 1.34

As with any microbenchmark, this workload is not a real-world one. For comparison, here are measurements of the time to fully startup and immediately shut down Debian Linux/m68k SID (systemd):

qemu-system-m68k mainline (42f6c9179b)

    real     31.5 s
    sys      1.59 s
    user     27.4 s
    sys/user 0.06

qemu-system-m68k patched (0a0bca4711)

    real     31.2 s
    sys      1.17 s
    user     27.6 s
    sys/user 0.04

The decrease in sys/user ratio reflects the small cost that has to be paid for the improvement in 6522 emulation fidelity and timer accuracy. But note that in both benchmarks wallclock execution time dropped, meaning that the system is faster overall.

The gettimeofday testing revealed that the Linux kernel does not properly protect userland from pathological hardware timers, and the gettimeofday result was seen to jump backwards (that was unexpected, though Mark did predict it).

This backwards jump was often observed in the mainline build during the gettimeofday benchmark and is result of bugs in mos6522.c. The patched build did not exhibit this problem (as yet).

The two benefits described here are offered in addition to all of the other benefits described in the patches themselves. Please consider merging this series.

Quoted 145 lines

- it allows Linux/m68k to make use of the additional timer that the hardware indeed offers, but which QEMU omits, and which may be of benefit to Linux guests [1], and
- 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 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.
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 emulated 6522 in QEMU 6, wherein two consecutive accesses to a timer count register have been observed to yield the same value.
Two problems presently affecting a Linux guest are clock drift and monotonicity failure in the 'via' clocksource. That is, the clocksource counter can jump backwards. This can be observed by patching Linux like so,
arch/m68k/mac/via.c +4 -0
@@ -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;
Or just enable CONFIG_DEBUG_TIMEKEEPING:
[ 1872.720000] INFO: timekeeping: Cycle offset (4294966426) is larger than the 'via1' clock's 50% safety margin (2147483647)
[ 1872.720000] timekeeping: Your kernel is still fine, but is feeling a bit nervous 
[ 1907.510000] INFO: timekeeping: Cycle offset (4294962989) is larger than the 'via1' clock's 50% safety margin (2147483647) 
[ 1907.510000] timekeeping: Your kernel is still fine, but is feeling a bit nervous 
[ 1907.900000] INFO: timekeeping: Cycle offset (4294963248) is larger than the 'via1' clock's 50% safety margin (2147483647) 
[ 1907.900000] timekeeping: Your kernel is still fine, but is feeling a bit nervous
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 and leads to clock drift.
This latency can be observed by patching QEMU like so:
hw/misc/mos6522.c +6 -0
@@ -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 6 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/[email protected]/
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 CONFIG_GENERIC_CLOCKEVENTS adoption, which would enable CONFIG_NO_HZ_IDLE etc.
[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.
---
Changed since RFC:
- Added Reviewed-by tags.
- Re-ordered some patches to make fixes available earlier in the series.
- Dropped patch 5/10 "Don't clear T1 interrupt flag on latch write".
- Rebased on v6.1.0 release.
Finn Thain (9):
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: 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/mos6522: Implement oneshot mode
 hw/misc/mos6522.c         | 245 ++++++++++++++++++--------------------
 hw/misc/trace-events      |   2 +-
 include/hw/misc/mos6522.h |   9 ++
 3 files changed, 123 insertions(+), 133 deletions(-)

Mark Cave-Ayland

2021-11-18 11:14 UTC #11 lore raw

Quoted 58 lines · Finn Thain

Quoted 7 lines · On Fri, 24 Sep 2021, I wrote:
This is a patch series for QEMU that I started last year. The aim was to try to get a monotonic clocksource for Linux/m68k guests. That hasn't been achieved yet (for q800 machines). I'm submitting the patch series because,
- it improves 6522 emulation fidelity, although slightly slower, and
I did some more benchmarking to examine the performance implications.
I measured a performance improvement with this patch series. For a Linux/m68k guest, the execution time for a gettimeofday syscall dropped from 9 us to 5 us. (This is a fairly common syscall.)
The host CPU time consumed by qemu in starting the guest kernel and executing a benchmark involving 20 million gettimeofday calls was as follows.
qemu-system-m68k mainline (42f6c9179b):
     real     198 s
     sys      123 s
     user     73 s
     sys/user 1.68
qemu-system-m68k patched (0a0bca4711):
     real     112 s
     sys      63 s
     user     47 s
     sys/user 1.34
As with any microbenchmark, this workload is not a real-world one. For comparison, here are measurements of the time to fully startup and immediately shut down Debian Linux/m68k SID (systemd):
qemu-system-m68k mainline (42f6c9179b)
     real     31.5 s
     sys      1.59 s
     user     27.4 s
     sys/user 0.06
qemu-system-m68k patched (0a0bca4711)
     real     31.2 s
     sys      1.17 s
     user     27.6 s
     sys/user 0.04
The decrease in sys/user ratio reflects the small cost that has to be paid for the improvement in 6522 emulation fidelity and timer accuracy. But note that in both benchmarks wallclock execution time dropped, meaning that the system is faster overall.
The gettimeofday testing revealed that the Linux kernel does not properly protect userland from pathological hardware timers, and the gettimeofday result was seen to jump backwards (that was unexpected, though Mark did predict it).

Quoted 7 lines

This backwards jump was often observed in the mainline build during the gettimeofday benchmark and is result of bugs in mos6522.c. The patched build did not exhibit this problem (as yet).
The two benefits described here are offered in addition to all of the other benefits described in the patches themselves. Please consider merging this series.

Hi Finn,

I've not forgotten about this series - we're now in 6.2 freeze, but it's on my TODO list to revisit in the next development cycle this along with the ESP stress-ng changes which I've also been looking at. As mentioned in my previous reviews the patch will need some further analysis: particularly the logic in mos6522_read() that can generate a spurious interrupt on a register read needs to be removed, and also testing is required to ensure that these changes don't affect the CUDA clock warping which allows OS X to boot under qemu-system-ppc.

I'm confident that qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) is monotonic, since if it were not then there would be huge numbers of complaints from QEMU users. It appears that Linux can potentially alter the ticks in mac_read_clk() at https://github.com/torvalds/linux/blob/master/arch/m68k/mac/via.c#L624 which suggests the issue is related to timer wraparound. I'd like to confirm exactly which part of your series fixes the specific problem of the clock jumping backwards before merging these changes.

I realized that I could easily cross-compile a 5.14 kernel to test this theory with the test root image and .config you supplied at https://gitlab.com/qemu-project/qemu/-/issues/611 using the QEMU docker-m68k-cross image to avoid having to build a complete toolchain by hand. The kernel builds successfully using this method, but during boot it hangs sending the first SCSI CDB to the ESP device, failing to send the last byte using PDMA.

Are there known issues cross-compiling an m68k kernel on an x86 host? Or are your current kernels being built from a separate branch outside of mainline Linux git?

ATB,

Mark.

Finn Thain

author

2021-11-19 08:41 UTC #12 lore raw

Quoted 9 lines · Mark Cave-Ayland

Hi Finn,
I've not forgotten about this series - we're now in 6.2 freeze, but it's on my TODO list to revisit in the next development cycle this along with the ESP stress-ng changes which I've also been looking at. As mentioned in my previous reviews the patch will need some further analysis:
particularly the logic in mos6522_read() that can generate a spurious interrupt on a register read needs to be removed,

If mos6522 fails to raise its IRQ then the counter value observed by the guest will not make sense. This relationship was explained in the description of patch 8. If you have a problem with that patch, please reply there so that your misapprehension can be placed in context.

Quoted 3 lines

and also testing is required to ensure that these changes don't affect the CUDA clock warping which allows OS X to boot under qemu-system-ppc.

I'm not expecting any issues. What is required in order to confirm this? Would it be sufficient to boot a Mac OS X 10.4 installer DVD?

Quoted 1 line

I'm confident that qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) is monotonic,

As I mentioned, it is monotonic here.

Quoted 8 lines

since if it were not then there would be huge numbers of complaints from QEMU users. It appears that Linux can potentially alter the ticks in mac_read_clk() at
https://github.com/torvalds/linux/blob/master/arch/m68k/mac/via.c#L624
which suggests the issue is related to timer wraparound. I'd like to confirm exactly which part of your series fixes the specific problem of the clock jumping backwards before merging these changes.

You really only need a good datasheet to review these patches. You don't need a deep understanding of any particular guest, and you shouldn't be targetting any particular guest.

One of the purposes of this patch series is to allow the guest to change to better exploit actual, physical hardware. Since QEMU regression testing is part of the kernel development process, regressions need to be avoided.

That means QEMU's shortcomings hinder Linux development.

Therefore, QEMU should not target the via timer driver in Linux v2.6 or the one in v5.15 or the one in NetBSD etc. QEMU should target correctness -- especially when that can be had for cheap. Wouldn't you agree?

QEMU deviates in numerous ways from the behaviour of real mos6522 timer. This patch series does not address all of these deviations (see patch 8). Instead, this patch series addresses only the most aggregious ones, and they do impact existing guests.

Quoted 9 lines

I realized that I could easily cross-compile a 5.14 kernel to test this theory with the test root image and .config you supplied at
https://gitlab.com/qemu-project/qemu/-/issues/611 using the QEMU docker-m68k-cross image to avoid having to build a complete toolchain by hand. The kernel builds successfully using this method, but during boot it hangs sending the first SCSI CDB to the ESP device, failing to send the last byte using PDMA.
Are there known issues cross-compiling an m68k kernel on an x86 host?

Not that I'm aware of.

Quoted 3 lines

Or are your current kernels being built from a separate branch outside of mainline Linux git?

I use mainline Linux when testing QEMU. I provided a mainline build, attached to the same bug report, so you don't have to build it.

Mark Cave-Ayland

2021-11-20 21:55 UTC #13 lore raw

Quoted 12 lines · Finn Thain

Quoted 9 lines · Mark Cave-Ayland
Hi Finn,
I've not forgotten about this series - we're now in 6.2 freeze, but it's on my TODO list to revisit in the next development cycle this along with the ESP stress-ng changes which I've also been looking at. As mentioned in my previous reviews the patch will need some further analysis:
particularly the logic in mos6522_read() that can generate a spurious interrupt on a register read needs to be removed,

Quoted 4 lines

If mos6522 fails to raise its IRQ then the counter value observed by the guest will not make sense. This relationship was explained in the description of patch 8. If you have a problem with that patch, please reply there so that your misapprehension can be placed in context.

It is the existing code in mos6522_read() which is doing the wrong thing here, which I mentioned in https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg02883.html.

Quoted 6 lines

Quoted 3 lines
and also testing is required to ensure that these changes don't affect the CUDA clock warping which allows OS X to boot under qemu-system-ppc.
I'm not expecting any issues. What is required in order to confirm this? Would it be sufficient to boot a Mac OS X 10.4 installer DVD?

Possibly: I've only been testing various images since after the timing workaround was added so I'm not sure exactly what the symptoms are, but the links sent earlier in the thread are still valid.

Quoted 3 lines

Quoted 1 line
I'm confident that qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) is monotonic,
As I mentioned, it is monotonic here.

Phew :)

Quoted 19 lines

Quoted 8 lines
since if it were not then there would be huge numbers of complaints from QEMU users. It appears that Linux can potentially alter the ticks in mac_read_clk() at
https://github.com/torvalds/linux/blob/master/arch/m68k/mac/via.c#L624
which suggests the issue is related to timer wraparound. I'd like to confirm exactly which part of your series fixes the specific problem of the clock jumping backwards before merging these changes.
You really only need a good datasheet to review these patches. You don't need a deep understanding of any particular guest, and you shouldn't be targetting any particular guest.
One of the purposes of this patch series is to allow the guest to change to better exploit actual, physical hardware. Since QEMU regression testing is part of the kernel development process, regressions need to be avoided.
That means QEMU's shortcomings hinder Linux development.

Quoted 8 lines

Therefore, QEMU should not target the via timer driver in Linux v2.6 or the one in v5.15 or the one in NetBSD etc. QEMU should target correctness -- especially when that can be had for cheap. Wouldn't you agree?
QEMU deviates in numerous ways from the behaviour of real mos6522 timer. This patch series does not address all of these deviations (see patch 8). Instead, this patch series addresses only the most aggregious ones, and they do impact existing guests.

That is true, but as per the link above there is an existing bug in the mos6522 device, and the patchset builds on this in its current form, including adding a state field which shouldn't be required.

From looking at mac_read_clk() presumably the problem here is that the timer IRQ fires on 0 rather than on 0xffff when it overflows? If so, this should be a very small and simple patch. Once these 2 fixes are in place, it will be much easier to test the remaining changes.

Quoted 10 lines

Quoted 9 lines
I realized that I could easily cross-compile a 5.14 kernel to test this theory with the test root image and .config you supplied at
https://gitlab.com/qemu-project/qemu/-/issues/611 using the QEMU docker-m68k-cross image to avoid having to build a complete toolchain by hand. The kernel builds successfully using this method, but during boot it hangs sending the first SCSI CDB to the ESP device, failing to send the last byte using PDMA.
Are there known issues cross-compiling an m68k kernel on an x86 host?

Quoted 7 lines

Not that I'm aware of.
Quoted 3 lines
Or are your current kernels being built from a separate branch outside of mainline Linux git?
I use mainline Linux when testing QEMU. I provided a mainline build, attached to the same bug report, so you don't have to build it.

The problem here is that I have no way to reproduce your results and test any patches other than to try and build a kernel with your extra warning and run it. Even then I don't know how long to wait for the clock to jump, how much it jumps by, or if there is anything else that needs to be done to trigger the warning.

Any help with being able to build a working cross-m68k kernel to test this would be gratefully received, otherwise I don't feel I have enough knowledge of the m68k kernel to be able to validate the fixes and review the changes in order to merge them.

ATB,

Mark.

Finn Thain

author

2021-11-20 23:39 UTC #14 lore raw

Quoted 14 lines · Mark Cave-Ayland

Quoted 12 lines · Finn Thain
Quoted 9 lines · Mark Cave-Ayland
Hi Finn,
I've not forgotten about this series - we're now in 6.2 freeze, but it's on my TODO list to revisit in the next development cycle this along with the ESP stress-ng changes which I've also been looking at. As mentioned in my previous reviews the patch will need some further analysis:
particularly the logic in mos6522_read() that can generate a spurious interrupt on a register read needs to be removed,

Quoted 9 lines

Quoted 4 lines
If mos6522 fails to raise its IRQ then the counter value observed by the guest will not make sense. This relationship was explained in the description of patch 8. If you have a problem with that patch, please reply there so that your misapprehension can be placed in context.
It is the existing code in mos6522_read() which is doing the wrong thing here, which I mentioned in
https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg02883.html.

How disingenous. I responded to that message 2 months ago and you completely ignored my response.

Basically, you found a bug in your own modified version of mainline code, and you claimed that this is somehow relevant to this patch series. (Apparently you failed to find that bug in my code.)

Once again, if you have an objection to existing code in mainline QEMU, please take it up with the author of that code (commit cd8843ff25) which is Laurent.

This patch series is a separate issue. It doesn't add anything objectionable (commit cd8843ff25 was not objected to), it fixes bugs, improves emulation fidelity, improves performance and reduces guest malfunctions.

Quoted 5 lines

That is true, but as per the link above there is an existing bug in the mos6522 device, and the patchset builds on this in its current form, including adding a state field which shouldn't be required.

The state enum is required for the oneshot feature (patch 9). It is also needed to produce the correct relationship between irq and counter (patch 8), and between interrupt flag set and clear operations. Finally, it is also useful for debugging purposes.

Quoted 2 lines

From looking at mac_read_clk() presumably the problem here is that the timer IRQ fires on 0 rather than on 0xffff when it overflows?

Guests depend on the correct relationship between timer irq flag and timer counter value. If QEMU can't get that right, the Linux clocksource can't work without race conditions. This problem is almost certain to affect other guests too.

You are being foolish to insist that this is somehow a Linux quirk.

Quoted 18 lines

If so, this should be a very small and simple patch. Once these 2 fixes are in place, it will be much easier to test the remaining changes.
Quoted 15 lines
Quoted 9 lines
I realized that I could easily cross-compile a 5.14 kernel to test this theory with the test root image and .config you supplied at
https://gitlab.com/qemu-project/qemu/-/issues/611 using the QEMU docker-m68k-cross image to avoid having to build a complete toolchain by hand. The kernel builds successfully using this method, but during boot it hangs sending the first SCSI CDB to the ESP device, failing to send the last byte using PDMA.
Are there known issues cross-compiling an m68k kernel on an x86 host?
Not that I'm aware of.
Quoted 3 lines
Or are your current kernels being built from a separate branch outside of mainline Linux git?

Quoted 6 lines

Quoted 2 lines
I use mainline Linux when testing QEMU. I provided a mainline build, attached to the same bug report, so you don't have to build it.
The problem here is that I have no way to reproduce your results and test any patches other than to try and build a kernel with your extra warning and run it.

Nonsense. Any programmer can easily observe the gettimeofday problem. Just run it in a loop. (How else would you establish monotonicity?)

Similarly, anyone who can understand mos6522.c and can read patches could easily add assertions to establish any of the deficiencies claimed in these patches.

The problem isn't that you "have no way to reproduce results". The problem is that you are unwilling or unable to understand the datasheet and the patches.

Quoted 9 lines

Even then I don't know how long to wait for the clock to jump, how much it jumps by, or if there is anything else that needs to be done to trigger the warning.
Any help with being able to build a working cross-m68k kernel to test this would be gratefully received, otherwise I don't feel I have enough knowledge of the m68k kernel to be able to validate the fixes and review the changes in order to merge them.

I've already helped you by supplying a mainline vmlinux binary. But you don't even need that. If you don't believe what I've said about mos6522.c behaviour, just install Debian/m68k.

Anyway, thanks for taking the time to write. A competent reviewer has to do much more than that, but I'm not paying for competence so I suppose I'm asking too much.

The absence of constructive review over the last few months is partly the result of get_maintainer.pl directing this submission to the wrong inbox. Phillippe, Laurent, please consider the following patch as well.

Signed-off-by Finn Thain <[email protected]>

MAINTAINERS +0 -2

index d3879aa3c1..f2e0ca8bbd 100644
@@ -1285,11 +1285,9 @@ F: hw/ppc/mac_newworld.c
 F: hw/pci-host/uninorth.c
 F: hw/pci-bridge/dec.[hc]
 F: hw/misc/macio/
-F: hw/misc/mos6522.c
 F: hw/nvram/mac_nvram.c
 F: hw/input/adb*
 F: include/hw/misc/macio/
-F: include/hw/misc/mos6522.h
 F: include/hw/ppc/mac_dbdma.h
 F: include/hw/pci-host/uninorth.h
 F: include/hw/input/adb*

Peter Maydell

2021-11-22 17:06 UTC #15 lore raw

Quoted 3 lines · Finn Thain

Anyway, thanks for taking the time to write. A competent reviewer has to do much more than that, but I'm not paying for competence so I suppose I'm asking too much.

Please dial back the aggressive tone here, Finn: this kind of thing is way out of line. We're all trying to help improve QEMU here, and sniping at Mark is not constructive.

-- PMM

Finn Thain

author

2021-11-22 22:36 UTC #16 lore raw

Quoted 9 lines · Peter Maydell

Quoted 3 lines · Finn Thain
Anyway, thanks for taking the time to write. A competent reviewer has to do much more than that, but I'm not paying for competence so I suppose I'm asking too much.
Please dial back the aggressive tone here, Finn: this kind of thing is way out of line. We're all trying to help improve QEMU here, and sniping at Mark is not constructive.

Peter, you seem to have misunderstood what I wrote. What I said was not sniping. "Incompetent" was my conclusion after I judiciously rejected "malicious". Here's what I mean by incompetent.

CONTRIBUTOR: Here's a patch.

MAINTAINER: I personally don't like that pattern. End of story.

CONTRIBUTOR: I don't think I'll contribute further to this project.

[Everyone loses.]

Now, here's what I would consider "competent":

CONTRIBUTOR: Here's a patch.

MAINTAINER: That pattern (I've quoted it to help further the discussion) is widely deprecated. You should use a different pattern instead. [Or read this reference. Or refer to this code.]

CONTRIBUTOR: OK, I see that this really is a problem, and I see that there really is a better way. However, the antipattern is already part of existing code, and my changes don't worsen the problem, and don't require that the problem persist.

MAINTAINER: You're right. My bad (I'm new to this). Since I never bothered to fix the existing antipattern, and no-one else thought it was worth fixing either, clearly it's not that important, and I should not have sought to veto your work, which is substantially unrelated, and beneficial either way.

CONTRIBUTOR: No problem.

[Everyone wins.]

Finally, here's the background for you to ponder, in case you would like to intervene to produce a better outcome. (I think you are potentially well positioned for that.)

https://lore.kernel.org/qemu-devel/[email protected]/
https://lore.kernel.org/qemu-devel/[email protected]/