On Sat, 1 Mar 2025 at 01:47, Sourojeet Adhikari
<s23ad...@csclub.uwaterloo.ca> wrote:
On 2025-02-27 10:17, Peter Maydell wrote:
On Thu, 27 Feb 2025 at 09:15, Sourojeet Adhikari
<s23ad...@csclub.uwaterloo.ca> wrote:
The systmr INTERRUPT_TIMER0..3 sysbus IRQ outputs are already
being wired up in the function bcm_soc_peripherals_common_realize()
in hw/arm/bcm2835_peripherals.c (to the TYPE_BCM2835_IC
interrupt controller), and it isn't valid to wire one input
directly to multiple outputs.
In fact it looks like we are currently getting this wrong for
all of the interrupts that need to be wired to both the
"legacy interrupt controller" and the GIC. I think at the moment
what happens is that the wiring to the GIC will happen last
and this overrides the earlier wiring to the legacy interrupt
controller, so code using the latter won't work correctly.
I'll try reading through the relevant sections and send an
updated patch later next week. From what I can tell it falls
under the bcm2835_pheripherals.c file, right?
Yes. To expand a bit, QEMU's qemu_irq abstraction must
always be wired exactly 1-to-1, from a single output to
a single input. Wiring either one input to multiple outputs
or one output to multiple inputs will not behave correctly
(and unfortunately we don't have an easy way to assert()
if code in QEMU gets this wrong).
So for cases where you want the one-to-many behaviour you need
to create an object of TYPE_SPLIT_IRQ. This has one input and
multiple outputs, so you can connect your wire from device A's
output to the splitter's input, and then connect outputs
from the splitter to devices B, C, etc. (In this case A
would be the timer, and B, C the two interrupt controllers.)
Searching the source code for TYPE_SPLIT_IRQ will give some
places where it's used. (Ignore the qdev_new(TYPE_SPLIT_IRQ)
ones, those are a code pattern we use in board models, not
in SoC device models.)
In this specific bcm2838 case, it's a little more awkward,
because one of the two interrupt controllers is created inside
bcm2835_peripherals.c and one of them is created outside it.
Since bcm2838 is already reaching inside the bcm2835_peripherals
object I guess the simplest thing is:
* create a splitter object in bcm2835_peripherals.c for
every IRQ line that needs to be connected to both
interrupt controllers (probably easiest to have an array
of splitter objects, irq_splitter[])
* in bcm2835_peripherals.c, connect the device's outbound
IRQ to the input of the appropriate splitter, and
connect output 0 of that splitter to the BCM2835_IC
correct interrupt controller input
* in bcm2838.c, connect output 0 of ps_base->irq_splitter[n]
to the correct GIC input
(This is kind of breaking the abstraction layer that ideally
exists where the code that creates and uses a device doesn't
try to look "inside" it at any subparts it might have. We
could, for instance, instead make the bcm2835_peripherals
object expose its own qemu_irq outputs which were the second
outputs of the splitters, so that the bcm2838.c code wasn't
looking inside and finding the splitters directly. But I
think that's more awkward than it's worth. It's also possible
that we have the split between the main SoC and the
peripheral object wrong and either both interrupt controllers
or neither should be inside the peripheral object; but
reshuffling things like that would be a lot of work too.)
This weekend I'll try my best to mess around, and get the solution
you proposed working. From what I can tell, I (personally) think , the
not-reshuffling things approach might be a bit better here. Since otherwise
it'd turn into a somewhat sizeable patch pretty quick, and is a lot of work,
for something that's not *too* big of an issue. I do have access to a raspberry
pi if you think I should do any kind of testing before doing the reshuffling.
Yeah, to be clear, what I'm suggesting is that we should
not do that reshuffling, exactly because it is a lot of
work and it's not that important. Better to just fix the bug.
On another note, do you think it's reasonable to add what you said
here into the development documentation (paraphrased, and if not
already documented). If I do write a patch to the documentation,
can/should I cc you on it?
In general, yes, we should at least document this kind of
beartrap. The difficulty is finding some good place to do it
(there are two broad locations: in a doc comment on the
function(s) for "connect a qemu_irq", and in some more
general "how to do device/board stuff" place in docs/devel/).
Feel free to cc me on any patch you send about that.
(PS: for the other "not 1:1" case, where you want to connect
many qemu_irqs outputs together into one input, the usual semantics
you want is to logically-OR the interrupt lines together, and
so you use TYPE_OR_IRQ for that.)
(oh oki, I'll make sure to do that on the upcoming patch then,
thank you!)
I think you won't need the OR gate part -- I mentioned it just
for completeness.
(P.S the patch probably won't be coming till next week since I
have quite a bit of work outside of my programming stuff to do.
Should hopfully be done by Wednesday next week though?)
That's fine -- since this is a bug fix we don't have to worry
about getting it in before softfreeze.
-- PMM
