Re: [PATCH v3 00/18] hw/ide: Untangle ISA/PCI abuses of ide_init_ioport()

[Bernhard Beschow]

Am 3. März 2023 07:46:31 UTC schrieb Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk>:
>On 03/03/2023 06:58, David Woodhouse wrote:
>
>> On 2 March 2023 22:40:40 GMT, "Philippe Mathieu-Daudé" <philmd@linaro.org> 
>> 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.