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
