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*