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

[BALATON Zoltan]

On Tue, 31 Jul 2018, Peter Maydell wrote:
> On 30 July 2018 at 23:37, BALATON Zoltan <address@hidden> 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.

> > 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