Am 3. März 2023 07:46:31 UTC schrieb Mark Cave-Ayland <[email protected]>: >On 03/03/2023 06:58, David Woodhouse wrote: > >> On 2 March 2023 22:40:40 GMT, "Philippe Mathieu-Daudé" <[email protected]> >> wrote: >>> Since v2: rebased >>> >>> I'm posting this series as it to not block Bernhard's PIIX >>> cleanup work. I don't have code change planned, but eventually >>> reword / improve commit descriptions. >>> >>> Tested commit after commit to be sure it is bisectable. Sadly >>> this was before Zoltan & Thomas report a problem with commit >>> bb98e0f59c ("hw/isa/vt82c686: Remove intermediate IRQ forwarder"). >> >> However much I stare at the partial revert which fixes it, I just cannot >> believe that the change could make any difference at all. There's got to be >> something weird going on there. >> >> I was going to ask if the level mode for the PIT made any difference, but >> this is the output IRQ from the PIT to the CPU itself so I don't see how it >> would. >> >> Would like to see a report with tracing from pic_update_irq, the CPU >> interrupt "handler" and the intermediate IRQ handler. With the intermediate >> present and without it. To compare the two. > >I suspect it's related to the removal of the allocation of the qemu_irq: qdev >gpios work by adding a child IRQ object to the device, so it could be possible >that something in the gpio internals isn't being updated correctly when the >value is overwritten directly.
I've just sent a series fixing the issue. The problem was that cpu_intr gets populated by qdev_connect_gpio_out() in board code which happens after via's realize method has been executed. So in via's realize method cpu_intr is still NULL which causes a NULL qemu_irq to be passed to the i8259. One way to fix this is to move qdev_connect_gpio_out() in board code between pci_new_multifunction() and pci_realize_and_unref(). By having an intermediate IRQ handler the problem didn't appear since the (non-NULL) qemu_irq holding the intermediate handler is passed to the i8259. The intermediate handler delays reading cpu_intr to runtime, so initializing it after realize() is no problem. The price, however, is that an indirection occurs at runtime every time cpu_intr is triggered. BTW, the PIC proxy in my PIIX consolidation series attempted to solve the same problem: The ISABus IRQs need to be already populated in piix-ide's realize method, otherwise NULL qemu_irqs are used. As long as piix-ide is realized in board code, separately from the piix south bridge, the ISABus IRQs can be populated in between. However, once piix-ide is realized in the south bridge, the ISA IRQs must be populated before the south bridge's realize(). The PIC proxy solved this by introducing intermediate ISA IRQs while the latest incarnation of the PIIX consolidation series uses the same approach as described above. Best regards, Bernhard > >Is the problem picked up when running a binary built with --enable-sanitizers? >That's normally quite good at detecting this kind of issue. > > >ATB, > >Mark.
