From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1Lzt8F-0001ls-7P for qemu-devel@nongnu.org; Fri, 01 May 2009 09:50:27 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1Lzt89-0001lg-BQ for qemu-devel@nongnu.org; Fri, 01 May 2009 09:50:25 -0400 Received: from [199.232.76.173] (port=50980 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Lzt89-0001ld-8L for qemu-devel@nongnu.org; Fri, 01 May 2009 09:50:21 -0400 Received: from pop-canoe.atl.sa.earthlink.net ([207.69.195.66]:35300) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1Lzt88-0004Qh-Qh for qemu-devel@nongnu.org; Fri, 01 May 2009 09:50:20 -0400 Message-ID: <49FAFD36.5040609@earthlink.net> Date: Fri, 01 May 2009 09:46:30 -0400 From: Robert Reif MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH] 64 bit I/O support v7 References: <49EDB109.5010009@earthlink.net> <200905011303.50885.paul@codesourcery.com> In-Reply-To: <200905011303.50885.paul@codesourcery.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paul Brook Cc: qemu-devel@nongnu.org Paul Brook wrote: >> The old array function call is supported so no existing drivers need >> to be modified. They can continue to do 2 32 bit accesses because 2 >> helper functions have been added to break up the accesses automatically. >> However, the helper functions should only be used until all drivers are >> converted to use the structure and then can be removed along >> with the old array functions api. The replacement of the arrays with >> structures in the drivers is very straightforward for drivers that don't >> do 64 bit I/O and the few that do can be cleaned up to remove the >> work arounds for the lack of true 64 bit I/O by their maintainers. >> > > This is going to be a bit of a pain, and a lot of duplication. My expectation > is that most devices don't know/care about 64-bit accesses and want them to > be automatically split into a pair of 32-bit accesses. I suggest pushing this > down into the lower level dispatch routines. By my reading your mem_writeq > helpers are broken if we happen to have multiple regions with the same opaque > value (this effects at least lsi53c895a.c). > > In the interests of avoiding duplication, I'd also implement > cpu_register_io_memory in terms of cpu_register_io_memory64. > > >> + return ((CPUReadMemoryFunc64*)(*mmio->mem_read[idx][3])) >> (mmio->opaque[idx][0][3], addr + mmio->region_offset[idx][0][3]); >> > > Eww. It would be a good idea to fix this at the same time. > > Paul > > The right way to do this is to convert the whole tree at once so we don't need the helper functions and two versions of cpu_register_io_memory. Unfortunately people have written hardware drivers that rely on the current weird behavior by caching half of a 64 bit access so it can be accessed later. This doesn't work if the registers can also be accessed as 32 bits because the cache trick implies a specific order of access. 95% of the hardware drivers could be trivially converted and work fine. It's those few that play games that need to be looked at carefully and converted properly to do the right thing. I think the authors or maintainers of those drivers have the best knowledge to back out the current workarounds and do 64 bit I/O properly. That's why I'm taking a 2 step approach using the helper functions and 2 cpu_register_io_memory functions. Once the entire tree is converted to use the structures, the helper functions and the second cpu_register_io_memory function would be removed. I can provide patches to convert sparc to the new API and also convert most of the simple drivers. However it's the ones that have workarounds for the current behavior that need careful scrutiny. The ideal time for applying this patch would be at the start of a new development cycle. 95% of the drivers could be converted immediately and the problem ones could be identified and fixed afterwards. The issues with the helper functions doing a linear scan and not supporting overlapping regions for 64 bit accesses would only be a problem for a short period of time (maybe a week) and only for some of the drivers that have 64 bit access issues.