Re: [Qemu-devel] [PATCH] sam460ex: Fix PCI interrupt connections

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4017 bytes --]

On Tue, Jul 31, 2018 at 01:31:46AM +0200, BALATON Zoltan wrote:
> On Tue, 31 Jul 2018, Peter Maydell wrote:
> > On 30 July 2018 at 23:37, BALATON Zoltan <balaton@eik.bme.hu> wrote:
> > QEMU's implementation of qemu_irq signal lines is that the destination
> > end provides the qemu_irq, which under the hood is a pointer to a
> > struct containing a pointer to the function in the destination device
> > which gets called when the source end says "the line level has changed".
> > This means that there won't be a compile time or runtime error if you
> > pass that qemu_irq to multiple sources (ie device outputs) simultaneously.
> > But the behaviour will be wrong, because the destination will see all
> > the "level is 0", "level is 1" calls from all the sources intermingled, eg
> > 
> > device A output:   ____|^^^^^^^^^^^^^|______
> > 
> > device B output:   _______|^^^^^|___________
> > 
> > destination sees:  ____|^^^^^^^^|___________
> > 
> > (because the destination gets the "level now 0" call when B's output
> > goes to zero). To get the desired behaviour:
> > 
> > logical OR:        ____|^^^^^^^^^^^^^|_____
> > 
> > you need an OR gate. (I'm assuming wired-OR because that's the
> > usual thing when several devices share an interrupt.)
> > 
> > If your testing involves a setup which doesn't happen to assert
> > several of the interrupt lines simultaneously you won't notice this
> > problem.
> 
> I think the testing with SATA and USB mouse on a PCI card is likely to
> assert several interrupts simultanelously (eg. when moving the mouse while
> loading stuff from disk) but the OS might have some ways to still recover
> from this so maybe we won't notice it anyway. As this is now confirmed that
> using the same irq multiple times is wrong I think we need a better fix.
> 
> David, can you please drop this patch, we'll come up with a different fix.

Done.  Should have looked at that patch a bit closer.

> > > Probably we can just change the map function in ppc440_pcix.c to always
> > > return the first line then what's specified for other lines should not
> > > matter and the above problem is avoided. We could even get rid of those
> > > additional irqs by changing ppc440_pcix.c to only model a single line but
> > > I'd need someone with better understanding of this to confirm that I got
> > > this right.
> > 
> > I think that would also fix the bug. The logically preferable
> > approach would depend rather on what the actual hardware does:
> > is there a PCI controller chip with 4 interrupt outputs which
> > the board wires together to a single interrupt controller line,
> > or does the PCI controller chip itself have just one output
> > and do the merging of the different PCI interrupts itself?
> 
> Hmm, don't really know. The PCI controller is part of the SoC but we don't
> have the manual of this SoC. The physical board itself also has a single PCI
> slot so however it's wired internally it's only using one irq for that. Not
> sure what other internal PCI devices are there. A comment in U-Boot sources
> says this (it lists PCIB twice but maybe that's meant to be PCID?):
> 
> // IRQ2  = PCI_INT (PCIA, PCIB, PCIC, PCIB)
> 
> So that suggests probably there are 4 PCI irqs that are wired together but
> probably this is inside the SoC. It could also be read as there's only a
> single PCI_INT that delivers all four PCI interrupts. So if we go from that
> either adding an OR gate to sam460ex.c that ORs together the PCI lines and
> connects it to uic[1][0] would work, or changing ppc440_pcix.c to provide a
> single PCI interrupt? We don't really have definitive info other than this
> comment so whatever Sebastian prefers and you approve is fine with me.
> 
> Thank you,
> BALATON Zoltan
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]