Re: [PATCH 0/5] Fix recent PIC -> CPU interrupt wiring regressions

[Philippe Mathieu-Daudé]

Hi Bernhard, Mark,

On 7/3/23 00:59, Mark Cave-Ayland wrote:
> On 04/03/2023 11:40, Bernhard Beschow wrote:
>
> > A recent series [1] attempted to remove some PIC -> CPU interrupt 
> > indirections.
> > This inadvertantly caused NULL qemu_irqs to be passed to the i8259 
> > because the
> > qemu_irqs aren't initialized at that time yet. This series provides a 
> > fix by
> > initializing the qemu_irq of the respective south bridges before they
> > are passed to i2859_init().
> >
> > Furthermore -- as an optional extension -- this series also fixes some 
> > usability
> > issues in the API for creating multifunction PCI devices.
> >
> > The series is structured as follows: The first three commits fix the
> > regressions, the last two fix the public API for creating 
> > multifunction PCI
> > devices.
> >
> > [1] 
> > 20230302224058.43315-1-philmd@linaro.org/">https://lore.kernel.org/qemu-devel/20230302224058.43315-1-philmd@linaro.org/
> >
> > Bernhard Beschow (5):
> >    hw/isa/vt82c686: Fix wiring of PIC -> CPU interrupt
> >    hw/alpha/dp264: Fix wiring of PIC -> CPU interrupt
> >    hw/ppc/prep: Fix wiring of PIC -> CPU interrupt
> >    hw/pci/pci: Remove multifunction parameter from
> >      pci_create_simple_multifunction()
> >    hw/pci/pci: Remove multifunction parameter from
> >      pci_new_multifunction()
> >
> >   include/hw/pci/pci.h |  4 +---
> >   hw/alpha/dp264.c     |  8 +++++---
> >   hw/i386/pc_piix.c    |  2 +-
> >   hw/i386/pc_q35.c     | 10 +++++-----
> >   hw/isa/vt82c686.c    |  3 ++-
> >   hw/mips/boston.c     |  3 +--
> >   hw/mips/fuloong2e.c  |  9 +++++----
> >   hw/mips/malta.c      |  2 +-
> >   hw/pci-host/sabre.c  |  6 ++----
> >   hw/pci/pci.c         | 18 ++++++++++++------
> >   hw/ppc/pegasos2.c    |  9 +++++----
> >   hw/ppc/prep.c        |  4 +++-
> >   hw/sparc64/sun4u.c   |  5 ++---
> >   13 files changed, 45 insertions(+), 38 deletions(-)
>
> Thanks for doing this! The patches basically look good, the only minor 
> niggle is that normally wiring of gpios is done *after* realize() for 
> consistency because some qdev_init_gpio_*() functions may use a property 
> to define the gpio array size.

Sorry this took me so long. The series LGTM too, but I wanted to well
understand the overall problem and run more tests.
Bernhard noticed that the bug is that we access the qdev gpios _before_
the device is realized.

The (undocumented) sysbus_connect_irq() API -- which calls
qdev_connect_gpio_out() -- is expected to be called _after_
DeviceRealize.

Bernhard's fix is to call qdev_connect_gpio_out() _before_
DeviceRealize.

> Having said that it is a nice tidy-up, so I'd be okay with patches 1-3 
> if you added a small comment above the qdev_connect_gpio_out() lines 
> pointing out that this is a temporary solution (hack?) until the 8259 
> device is converted to use gpios.

I agree, while this works, it is a "temporary solution" until we decide
and clarify the QDev/SysBus APIs w.r.t. IRQs.

> However given that Phil wrote the patches I'd wait for him to decide 
> whether he'd prefer a plain revert over the changes in this series 
> before going ahead with a v2.

As discussed with Peter / Mark / David on IRC, a revert is wiser for
this release.