Message ID | cover.1632437396.git.fthain@linux-m68k.org |
---|---|
State | New |
Headers | show |
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(-) > >
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.
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.
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.
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*
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
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/
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);