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

From: Finn Thain <fthain@linux-m68k.org>
To: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Cc: qemu-devel@nongnu.org, "Greg Kurz" <groug@kaod.org>,
	qemu-ppc@nongnu.org, "Laurent Vivier" <laurent@vivier.eu>,
	"Philippe Mathieu-Daudé" <f4bug@amsat.org>
Subject: Re: [PATCH v1 0/9] hw/mos6522: VIA timer emulation fixes and improvements
Date: Fri, 19 Nov 2021 19:39:10 +1100 (AEDT)	[thread overview]
Message-ID: <ad537ce9-ec40-b5e4-bb32-5f53e42db29@linux-m68k.org> (raw)
In-Reply-To: <2fb3d9f8-0f20-082d-d9f1-ab2984356866@ilande.co.uk>

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.