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(-)
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] 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(-)
--
2.39.2
On Sat, Mar 4, 2023 at 12:40 PM Bernhard Beschow <shentey@gmail.com> 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. > Testing done: * `make check` * `make check-avocado` * Start MorphOS ISO > > [1] > 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(-) > > -- > 2.39.2 > >
On Sat, Mar 04, 2023 at 12:40:38PM +0100, 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] https://lore.kernel.org/qemu-devel/20230302224058.43315-1-philmd@linaro.org/ Well philmd merged that one so I'll let him untangle it. However please separate fixes and cleanups. Cleanups can't go in now, fixes still can. Thanks! > 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(-) > > -- > 2.39.2 >
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] 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. 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. 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. Finally I think patches 4-5 are good, however given how close we are to freeze I'd drop them for now and present them as a separate series at the start of the 8.1 dev cycle when everyone has a bit more breathing space. ATB, Mark.
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] >> 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.
On Sat, Mar 04, 2023 at 12:40:38PM +0100, 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. "We pulled your tooth oh and we removed your appendix too while we were at it". Pls don't do this kind of thing unless strictly necessary. > [1] 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(-) > > -- > 2.39.2 >
On Sat, 4 Mar 2023, 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] 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() I'd postopne the last two API change patches to the next release. Ideally the device itself should know if it's multifunction or not and the board instantiating it should not do anything different than instantiating a single function device so we's only need pci_new or pci_create_simple without multifunction parameter or variant. So my question is why do we need these at all and could this be simplified more? But there's not enough time to answer that now so I'd ask to leave these alone for now and come back to this in next devel cycle. The other 3 patches fix a breakaga in current master so can be considered but I'd need to know a decision if this will be taken or a revert as I need to rebase my pending patches accordingly. A maintainer please speak up here. Regards, BALATON Zoltan > 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(-) > > -- > 2.39.2 > > >
On Sat, 4 Mar 2023 at 13:30, BALATON Zoltan <balaton@eik.bme.hu> wrote: > > On Sat, 4 Mar 2023, 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] 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() > > I'd postopne the last two API change patches to the next release. Ideally > the device itself should know if it's multifunction or not and the board > instantiating it should not do anything different than instantiating a > single function device so we's only need pci_new or pci_create_simple > without multifunction parameter or variant. So my question is why do we > need these at all and could this be simplified more? But there's not > enough time to answer that now so I'd ask to leave these alone for now and > come back to this in next devel cycle. > > The other 3 patches fix a breakaga in current master so can be considered > but I'd need to know a decision if this will be taken or a revert as I > need to rebase my pending patches accordingly. A maintainer please speak > up here. If we're happy that patches 1-3 fix the regressions and look OK code-wise then applying them is probably the simplest thing. thanks -- PMM
© 2016 - 2024 Red Hat, Inc.