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

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

[Finn Thain]

This code appears to be unnecessary.

Signed-off-by: Finn Thain <fthain@linux-m68k.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.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

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

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

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

[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 <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 c0d6bee4cc..6bd60f2118 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)
@@ -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) {
-- 
2.26.3

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

[Finn Thain]

This code appears to be unnecessary.

Signed-off-by: Finn Thain <fthain@linux-m68k.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.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

[PATCH v1 4/9] hw/mos6522: Rename timer callback functions

[Finn Thain]

This improves readability.

Signed-off-by: Finn Thain <fthain@linux-m68k.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.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

[PATCH v1 0/9] hw/mos6522: VIA timer emulation fixes and improvements

[Finn Thain]

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,

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;


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:

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 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/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 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(-)

-- 
2.26.3

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

[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 <fthain@linux-m68k.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
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(-)

diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
index bfe1719b18..385ea81b62 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,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();
-- 
2.26.3

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

[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 <fthain@linux-m68k.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.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 6bd60f2118..bfe1719b18 100644
--- a/hw/misc/mos6522.c
+++ b/hw/misc/mos6522.c
@@ -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;
-- 
2.26.3

[PATCH v1 9/9] hw/mos6522: Implement oneshot mode

[Finn Thain]

Timer 1 has two modes: continuous interrupt and oneshot.

Signed-off-by: Finn Thain <fthain@linux-m68k.org>
---
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(-)

diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
index 8957c5e65c..bbed0b84c0 100644
--- a/hw/misc/mos6522.c
+++ b/hw/misc/mos6522.c
@@ -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;
diff --git a/include/hw/misc/mos6522.h b/include/hw/misc/mos6522.h
index a2df5fa942..4dbba6b273 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
-- 
2.26.3

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

[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 <fthain@linux-m68k.org>
---
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(-)

diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
index 385ea81b62..8957c5e65c 100644
--- a/hw/misc/mos6522.c
+++ b/hw/misc/mos6522.c
@@ -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;
diff --git a/hw/misc/trace-events b/hw/misc/trace-events
index ede413965b..bce49ce47c 100644
--- a/hw/misc/trace-events
+++ b/hw/misc/trace-events
@@ -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"
diff --git a/include/hw/misc/mos6522.h b/include/hw/misc/mos6522.h
index fc95d22b0f..a2df5fa942 100644
--- a/include/hw/misc/mos6522.h
+++ b/include/hw/misc/mos6522.h
@@ -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;
 
 /**
-- 
2.26.3

Re: [PATCH v1 0/9] hw/mos6522: VIA timer emulation fixes and improvements

[Finn Thain]

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.

>  - 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,
> 
> 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;
> 
> 
> 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:
> 
> 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 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/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 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(-)
> 
> 

Re: [PATCH v1 0/9] hw/mos6522: VIA timer emulation fixes and improvements

[Mark Cave-Ayland]

On 17/11/2021 03:03, Finn Thain wrote:

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

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.

Re: [PATCH v1 0/9] hw/mos6522: VIA timer emulation fixes and improvements

[Finn Thain]

On Thu, 18 Nov 2021, Mark Cave-Ayland wrote:

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

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

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

As I mentioned, it is monotonic here.

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

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

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

Re: [PATCH v1 0/9] hw/mos6522: VIA timer emulation fixes and improvements

[Mark Cave-Ayland]

On 19/11/2021 08:39, Finn Thain wrote:

> On Thu, 18 Nov 2021, Mark Cave-Ayland wrote:
> 
>>
>> 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.

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.

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

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

Phew :)

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

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.

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

Re: [PATCH v1 0/9] hw/mos6522: VIA timer emulation fixes and improvements

[Finn Thain]

On Sat, 20 Nov 2021, Mark Cave-Ayland wrote:

> On 19/11/2021 08:39, Finn Thain wrote:
> 
> > On Thu, 18 Nov 2021, Mark Cave-Ayland wrote:
> > 
> >>
> >> 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.
> 
> 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.

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

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

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

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.

> 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 <fthain@linux-m68k.org>

diff --git a/MAINTAINERS b/MAINTAINERS
index d3879aa3c1..f2e0ca8bbd 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -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*

Re: [PATCH v1 0/9] hw/mos6522: VIA timer emulation fixes and improvements

[Peter Maydell]

On Sat, 20 Nov 2021 at 23:40, Finn Thain <fthain@linux-m68k.org> wrote:
> 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

Re: [PATCH v1 0/9] hw/mos6522: VIA timer emulation fixes and improvements

[Finn Thain]

On Mon, 22 Nov 2021, Peter Maydell wrote:

> On Sat, 20 Nov 2021 at 23:40, Finn Thain <fthain@linux-m68k.org> wrote:
> > 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/cover.1629799776.git.fthain@linux-m68k.org/
https://lore.kernel.org/qemu-devel/cover.1632437396.git.fthain@linux-m68k.org/