From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47985) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Uslvv-0004X2-Az for qemu-devel@nongnu.org; Fri, 28 Jun 2013 23:34:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Uslvt-0005hH-NU for qemu-devel@nongnu.org; Fri, 28 Jun 2013 23:34:43 -0400 Received: from mail-ie0-f169.google.com ([209.85.223.169]:63671) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Uslvt-0005h3-GO for qemu-devel@nongnu.org; Fri, 28 Jun 2013 23:34:41 -0400 Received: by mail-ie0-f169.google.com with SMTP id 10so5613726ied.14 for ; Fri, 28 Jun 2013 20:34:40 -0700 (PDT) Date: Fri, 28 Jun 2013 02:01:51 -0500 From: Rob Landley In-Reply-To: (from peter.maydell@linaro.org on Mon Apr 8 15:16:18 2013) Message-Id: <1372402911.2776.155@driftwood> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii; DelSp=Yes; Format=Flowed Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v4 00/11] Fix versatile_pci List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: Arnd Bergmann , patches@linaro.org, "Michael S. Tsirkin" , Will Deacon , qemu-devel@nongnu.org, Paul Brook , Andreas =?iso-8859-1?q?F=E4rber?= On 04/08/2013 03:16:18 PM, Peter Maydell wrote: > On 8 April 2013 18:37, Rob Landley wrote: > > On 04/06/2013 10:44:25 AM, Peter Maydell wrote: > >> > >> This patch series fixes a number of serious bugs in our emulation =20 > of > >> the PCI controller found on VersatilePB and the early Realview =20 > boards: > >> * our interrupt mapping was totally wrong > > > > > > Yes. Yes it was. However, what you were doing matched the kernel =20 > for many > > years. >=20 > > The kernel guys have screwed this up three consecutive times, as =20 > described > > here: > > > > http://landley.net/hg/aboriginal/rev/7bf850767bb8 > > > > Because as far as I can tell, nobody has any test hardware for this =20 > anymore. >=20 > There is some but it's pretty rare. Now that the next kernel's about to come out, I'm trying to get my arm =20 versatile image to work under qemu 1.5.0. The old kernel doesn't work, =20 and the current vanilla kernel doesn't work. This change broke it. I'm testing current linux-git both with and without this patch: --- linux/arch/arm/mach-versatile/pci.c 2013-04-28 19:36:01.000000000 =20 -0500 +++ linux.bak/arch/arm/mach-versatile/pci.c 2013-04-29 =20 19:09:44.857097553 -0500 @@ -333,7 +333,7 @@ * 26 1 IRQ_SIC_PCI2 * 27 1 IRQ_SIC_PCI3 */ - irq =3D IRQ_SIC_PCI0 + ((slot - 24 + pin - 1) & 3); + irq =3D 59; //IRQ_SIC_PCI0 + ((slot - 24 + pin - 1) & 3); return irq; } With the patch, qemu 1.2.0 works, but qemu 1.5 versatile does not work =20 with or without the patch. Here's a test image to demonstrate the problem: it works fine under =20 qemu 1.2.0, but doesn't under 1.5.0: http://landley.net/aboriginal/bin/system-image-armv5l.tar.bz2 Extract it and ./run-emulator.sh. You get a shell prompt from 1.2.0, =20 from 1.5.0 it never gets a scsi interrupt, and after a timeout goes =20 into an abort/reset loop. > >> This version of the patchset avoids breaking legacy Linux guests > >> by automatically detecting those broken kernels and switching back > >> to the old mapping. As testing with the above image confirms: it does not. > >> This works by looking at the values the kernel > >> writes to the PCI_INTERRUPT_LINE register in the config space, =20 > which > >> is effectively the interrupt number the kernel expects the device > >> to be using. If this doesn't match reality we use the broken =20 > mapping. > >> (Thanks to Michael S. Tsirkin for this suggestion.) Define "reality". The kernel changed what irqs it was expecting _three_times_ last year, =20 and as far as I can tell none of them were what they were _trying_ to =20 do. Here's my blog entry and the source control comments where I diagnosed =20 this stuff to show that the kernel guys have no flipping CLUE how an =20 arm versatile board actually works: 1) http://landley.net/notes-2013.html#15-03-2013 2) http://landley.net/hg/aboriginal/rev/1588 3) http://landley.net/hg/aboriginal/rev/1589 Here's the text from #2: =3D=3D=3D=3D=3D The arm guys have now screwed up the ARM versatile board's IRQ routing =20 three consecutive times. I'm impressed. The ARM versatile scsi IRQ is position 27 on the IRQ controller. That's =20 what QEMU implemented, that's what worked for years. In commit 1bc39ac5dab2 =20 the kernel guys screwed up their math (among other things doing -24 and =20 then &3 on the result.. can we say NOP?) so it was now trying to use IRQ 28 once the unnecessary "swizzle" function got done with it. (I started =20 reverting that 6 months ago in aboriginal changeset 1534.) Then in commit =20 f5565295892e they incremented the IRQ controller start by 32... and didn't adjust =20 map_irq() so it was still requesting 28, now before the start of the IRQ controller's range. Then in commit e3e92a7be693 they noticed it was broken, and added 64 to it. (So now it's requesting 64+28=3D92, when it _should_ be requesting 32+27=3D59. Their own description of what changed =20 does not support what the patch did, and yet...) =3D=3D=3D=3D=3D All that was about the SCSI IRQ. Entry #3 above was about the =20 _ethernet_ IRQ, which they screwed up in a different way (they did an =20 &3 in a way that wrapped)about how they did the math for the ethernet =20 irq wrong in a _different_ way than they did the scsi irq math wrong. The line that's actually calculating the IRQ is: irq =3D 27 + ((slot - 24 + pin - 1) & 3); Since 24 is divisible by 3, subtracting 24 and then & 3 on the result =20 is a NOP so this math can't POSSIBLY do what they think it's doing. The kernel code in this area is CRAP, has changed multiple times in =20 semi-random ways, has obviously NEVER been tested on real hardware, and =20 if you've implemented what the actual versatile documentation says it's =20 clearly not what the kernel is doing. > > My concern here was that you'd going to change qemu so it doesn't =20 > run the > > old images, and require a very new qemu to run the new images, so =20 > there will > > be an incompatible flag day. >=20 > No, it doesn't break old images, as the paragraph you quote clearly > states. Yes, if the kernel guys fix the kernel you'll need a newer > QEMU to run that. However as the kernel patches have been floating =20 > around > unapplied since 2011 you can draw your own conclusions about how =20 > quickly > the kernel will get fixed. I have an old image that used to work with qemu 1.2. it does not work =20 with qemu 1.5. (I skip 1.3 and 1.4 because tcg had random failures.) I removed my irq fix patch and just let the kernel do its own math, and =20 that doesn't work with qemu 1.5 either. > > Yay improving the latest and greatest, but when I'm regression =20 > testing as > > many different platforms as I can get working, stuff tends to =20 > break. (The > > most recent target-specific one for me on the QEMU side was =20 > probably the > > powerpc video thing, > > http://landley.net/notes-2012.html#17-11-2012 . Other than the =20 > 1.3/1.4 > > general TCG instability which was the translation unit size being =20 > calculated > > wrong.) This is why I can't demand my users upgrade each time I do a > > release: the target they're interested in might not work because my =20 > images > > depend on things you don't regression test much. >=20 > Yes, TCG and minor platform emulation is to some extent on a 'best > effort' basis. Unfortunately we have neither the developer resources > nor the automated test infrastructure to do better; if you can help > on either front do say. I have images that boot to a shell prompt, and I can build linux from =20 scratch in an automated manner under the result. (That's how I found =20 tcg instability: it would die in random places during the various =20 native package builds.) > > I'm glad to see you're addressing backwards compatability, but it =20 > looks like > > I might have to patch my kernels to write the _old_ value to this =20 > register > > in order to get something that runs on old qemu? >=20 > If you want to run on a QEMU predating this patchset then you need > to have a kernel which expects the old and broken behaviour, yes. > This patchset is to some extent futureproofing in that it ensures > that if the kernel guys actually fix the versatilepb PCI code at > some future date the QEMU that is out in the wild will still cope. Do you have a kernel that runs under current qemu arm versatile =20 emulation? I can poke around and figure out which irqs it expects =20 _now_, but it's not "right" and presumably you're just going to change =20 it again when you realize what qemu is doing and what the unpatched =20 kernel is doing don't match. > > Because I broke my kernel > > to fit what qemu was doing, and I dunno when this register write =20 > changed. (I > > _do_ know that what the kernel guys have been doing to versatile is =20 > insane > > and inconsistent, and if you change your code to humor them they'll =20 > probably > > break it again. Their register mapping in 3.8 was so wrong even _I_ =20 > could > > tell.) >=20 > I don't have any interest in adding workarounds to QEMU for kernel > versions which didn't work on any QEMU version or on real hardware. They worked fine on qemu 1.2 and several versions before that. > I do care about making QEMU work like the hardware, and about keeping > it working with kernels that assume the old longstanding QEMU > behaviour. This patchset satisfies both those goals. If it did, I wouldn't be emailing you. Try the system image above, on =20 1.2 and on current. > If you have issues with the kernel breaking things then you need to > take it up with the kernel developers. I did. They're only interested in new boards, none of which are both =20 supported by qemu and emulate a PCI bus. I'm happy to patch the kernel. But the same binary that ran under qemu =20 1.2 does not run under 1.5, and that's a regression. It would be an =20 acceptable regression if an unpatched kernel ran under 1.5, but it =20 doesn't. > -- PMM Rob=