| Message ID | 20240704205854.18537-1-shentey@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Resolve vt82c686 and piix4 qemu_irq memory leaks | expand |
On Thu, Jul 04, 2024 at 10:58:51PM +0200, Bernhard Beschow wrote: > This series first turns vt82c686's "INTR" pin into a named GPIO for better > comprehensibility. It then continues fixing qemu_irq memory leaks in vt82c686 > and piix4 by connecting out IRQs of the south bridges before they get realized. > This approach is already used in the pc machines after it had been discussed at > KVM forum `23. > > Observe that out IRQs are callbacks such as an INTR IRQ handler in a CPU which a > south bridge wants to trigger. If, as an implementation detail, the south bridge > wants to pass this callback to a child device, such as the PIC, then this > callback must be known to the south bridge before it gets realized. Otherwise > board code had to wire the PIC device itself, breaking encapsulation. This means > that qdev_connect_gpio_out*() has to be called before realize() which this > series implements. Another way to look at it is that callbacks apparently are > resouces such as memory regions which are also populated before realize(). > > Please check if above paragraph makes sense. > > Best regards, > Bernhard Bernhard, do you intend to resolve Zoltan's comments on this patchset? Thanks! > See also: > * https://lore.kernel.org/qemu-devel/0FFB5FD2-08CE-4CEC-9001-E7AC24407A44@gmail. > com/ > * "Remove intermediate IRQ forwarder" patches in > https://lore.kernel.org/qemu-devel/20230210163744.32182-1-philmd@linaro.org/ > > Testing done: > * Boot amigaone machine into Linux > * Boot pegasos2 machine into MorphOS > * Start fuloong2e machine and check that it doesn't abort > * Boot malta machine with https://people.debian.org/~gio/dqib/ > > Bernhard Beschow (3): > hw/isa/vt82c686: Turn "intr" irq into a named gpio > hw/isa/vt82c686: Resolve intermediate IRQ forwarder > hw/isa/piix: Resolve intermediate IRQ forwarder > > hw/isa/piix.c | 13 ++----------- > hw/isa/vt82c686.c | 12 ++---------- > hw/mips/fuloong2e.c | 2 +- > hw/mips/malta.c | 4 +--- > hw/ppc/amigaone.c | 8 ++++---- > hw/ppc/pegasos2.c | 4 ++-- > 6 files changed, 12 insertions(+), 31 deletions(-) > > -- > 2.45.2 >
On Sat, 20 Jul 2024, Michael S. Tsirkin wrote: > On Thu, Jul 04, 2024 at 10:58:51PM +0200, Bernhard Beschow wrote: >> This series first turns vt82c686's "INTR" pin into a named GPIO for better >> comprehensibility. It then continues fixing qemu_irq memory leaks in vt82c686 >> and piix4 by connecting out IRQs of the south bridges before they get realized. >> This approach is already used in the pc machines after it had been discussed at >> KVM forum `23. >> >> Observe that out IRQs are callbacks such as an INTR IRQ handler in a CPU which a >> south bridge wants to trigger. If, as an implementation detail, the south bridge >> wants to pass this callback to a child device, such as the PIC, then this >> callback must be known to the south bridge before it gets realized. Otherwise >> board code had to wire the PIC device itself, breaking encapsulation. This means >> that qdev_connect_gpio_out*() has to be called before realize() which this >> series implements. Another way to look at it is that callbacks apparently are >> resouces such as memory regions which are also populated before realize(). >> >> Please check if above paragraph makes sense. >> >> Best regards, >> Bernhard > > > Bernhard, do you intend to resolve Zoltan's comments on this > patchset? You can take this alternative for now: https://patchew.org/QEMU/cover.1719690591.git.balaton@eik.bme.hu/ I think solving it the right way would need to QDev-ify i8259 and make it use gpio out and get rid of the legacy init function but as that's an old piece of code used by multiple machines, it's not easily done so maybe not for few days before the freeze. For now I think either embedding the IRQ in the state struct (or alternatively adding an unrealize or finalize or whatever is the right method to free it) is better than changing all users of this device. Regards, BALATON Zoltan
Am 20. Juli 2024 18:38:42 UTC schrieb "Michael S. Tsirkin" <mst@redhat.com>: >On Thu, Jul 04, 2024 at 10:58:51PM +0200, Bernhard Beschow wrote: >> This series first turns vt82c686's "INTR" pin into a named GPIO for better >> comprehensibility. It then continues fixing qemu_irq memory leaks in vt82c686 >> and piix4 by connecting out IRQs of the south bridges before they get realized. >> This approach is already used in the pc machines after it had been discussed at >> KVM forum `23. >> >> Observe that out IRQs are callbacks such as an INTR IRQ handler in a CPU which a >> south bridge wants to trigger. If, as an implementation detail, the south bridge >> wants to pass this callback to a child device, such as the PIC, then this >> callback must be known to the south bridge before it gets realized. Otherwise >> board code had to wire the PIC device itself, breaking encapsulation. This means >> that qdev_connect_gpio_out*() has to be called before realize() which this >> series implements. Another way to look at it is that callbacks apparently are >> resouces such as memory regions which are also populated before realize(). >> >> Please check if above paragraph makes sense. >> >> Best regards, >> Bernhard > > >Bernhard, do you intend to resolve Zoltan's comments on this >patchset? Hi Michael, I was hoping for some more comments on the last two commits. These resolve various issues in one go, some of which have been addressed in the past. Since this is apparently a new style to handle out-GPIOs, I'd like to get an OK from the community. As Zoltan writes the ideal solution might be to qdev'ify the PIC. This is a bigger task which is more than I intended to do in this series. Best regards, Bernhard > >Thanks! > > >> See also: >> * https://lore.kernel.org/qemu-devel/0FFB5FD2-08CE-4CEC-9001-E7AC24407A44@gmail. >> com/ >> * "Remove intermediate IRQ forwarder" patches in >> https://lore.kernel.org/qemu-devel/20230210163744.32182-1-philmd@linaro.org/ >> >> Testing done: >> * Boot amigaone machine into Linux >> * Boot pegasos2 machine into MorphOS >> * Start fuloong2e machine and check that it doesn't abort >> * Boot malta machine with https://people.debian.org/~gio/dqib/ >> >> Bernhard Beschow (3): >> hw/isa/vt82c686: Turn "intr" irq into a named gpio >> hw/isa/vt82c686: Resolve intermediate IRQ forwarder >> hw/isa/piix: Resolve intermediate IRQ forwarder >> >> hw/isa/piix.c | 13 ++----------- >> hw/isa/vt82c686.c | 12 ++---------- >> hw/mips/fuloong2e.c | 2 +- >> hw/mips/malta.c | 4 +--- >> hw/ppc/amigaone.c | 8 ++++---- >> hw/ppc/pegasos2.c | 4 ++-- >> 6 files changed, 12 insertions(+), 31 deletions(-) >> >> -- >> 2.45.2 >> >
On Mon, Jul 22, 2024 at 10:21:30PM +0000, Bernhard Beschow wrote: > > > Am 20. Juli 2024 18:38:42 UTC schrieb "Michael S. Tsirkin" <mst@redhat.com>: > >On Thu, Jul 04, 2024 at 10:58:51PM +0200, Bernhard Beschow wrote: > >> This series first turns vt82c686's "INTR" pin into a named GPIO for better > >> comprehensibility. It then continues fixing qemu_irq memory leaks in vt82c686 > >> and piix4 by connecting out IRQs of the south bridges before they get realized. > >> This approach is already used in the pc machines after it had been discussed at > >> KVM forum `23. > >> > >> Observe that out IRQs are callbacks such as an INTR IRQ handler in a CPU which a > >> south bridge wants to trigger. If, as an implementation detail, the south bridge > >> wants to pass this callback to a child device, such as the PIC, then this > >> callback must be known to the south bridge before it gets realized. Otherwise > >> board code had to wire the PIC device itself, breaking encapsulation. This means > >> that qdev_connect_gpio_out*() has to be called before realize() which this > >> series implements. Another way to look at it is that callbacks apparently are > >> resouces such as memory regions which are also populated before realize(). > >> > >> Please check if above paragraph makes sense. > >> > >> Best regards, > >> Bernhard > > > > > >Bernhard, do you intend to resolve Zoltan's comments on this > >patchset? > > Hi Michael, > > I was hoping for some more comments on the last two commits. These resolve various issues in one go, some of which have been addressed in the past. Since this is apparently a new style to handle out-GPIOs, I'd like to get an OK from the community. > > As Zoltan writes the ideal solution might be to qdev'ify the PIC. This is a bigger task which is more than I intended to do in this series. > > Best regards, > Bernhard ATM I'm just looking at whether we can fix any bugs for the coming release. > > > >Thanks! > > > > > >> See also: > >> * https://lore.kernel.org/qemu-devel/0FFB5FD2-08CE-4CEC-9001-E7AC24407A44@gmail. > >> com/ > >> * "Remove intermediate IRQ forwarder" patches in > >> https://lore.kernel.org/qemu-devel/20230210163744.32182-1-philmd@linaro.org/ > >> > >> Testing done: > >> * Boot amigaone machine into Linux > >> * Boot pegasos2 machine into MorphOS > >> * Start fuloong2e machine and check that it doesn't abort > >> * Boot malta machine with https://people.debian.org/~gio/dqib/ > >> > >> Bernhard Beschow (3): > >> hw/isa/vt82c686: Turn "intr" irq into a named gpio > >> hw/isa/vt82c686: Resolve intermediate IRQ forwarder > >> hw/isa/piix: Resolve intermediate IRQ forwarder > >> > >> hw/isa/piix.c | 13 ++----------- > >> hw/isa/vt82c686.c | 12 ++---------- > >> hw/mips/fuloong2e.c | 2 +- > >> hw/mips/malta.c | 4 +--- > >> hw/ppc/amigaone.c | 8 ++++---- > >> hw/ppc/pegasos2.c | 4 ++-- > >> 6 files changed, 12 insertions(+), 31 deletions(-) > >> > >> -- > >> 2.45.2 > >> > >
Am 23. Juli 2024 00:21:32 UTC schrieb "Michael S. Tsirkin" <mst@redhat.com>: >On Mon, Jul 22, 2024 at 10:21:30PM +0000, Bernhard Beschow wrote: >> >> >> Am 20. Juli 2024 18:38:42 UTC schrieb "Michael S. Tsirkin" <mst@redhat.com>: >> >On Thu, Jul 04, 2024 at 10:58:51PM +0200, Bernhard Beschow wrote: >> >> This series first turns vt82c686's "INTR" pin into a named GPIO for better >> >> comprehensibility. It then continues fixing qemu_irq memory leaks in vt82c686 >> >> and piix4 by connecting out IRQs of the south bridges before they get realized. >> >> This approach is already used in the pc machines after it had been discussed at >> >> KVM forum `23. >> >> >> >> Observe that out IRQs are callbacks such as an INTR IRQ handler in a CPU which a >> >> south bridge wants to trigger. If, as an implementation detail, the south bridge >> >> wants to pass this callback to a child device, such as the PIC, then this >> >> callback must be known to the south bridge before it gets realized. Otherwise >> >> board code had to wire the PIC device itself, breaking encapsulation. This means >> >> that qdev_connect_gpio_out*() has to be called before realize() which this >> >> series implements. Another way to look at it is that callbacks apparently are >> >> resouces such as memory regions which are also populated before realize(). >> >> >> >> Please check if above paragraph makes sense. >> >> >> >> Best regards, >> >> Bernhard >> > >> > >> >Bernhard, do you intend to resolve Zoltan's comments on this >> >patchset? >> >> Hi Michael, >> >> I was hoping for some more comments on the last two commits. These resolve various issues in one go, some of which have been addressed in the past. Since this is apparently a new style to handle out-GPIOs, I'd like to get an OK from the community. >> >> As Zoltan writes the ideal solution might be to qdev'ify the PIC. This is a bigger task which is more than I intended to do in this series. >> >> Best regards, >> Bernhard > > >ATM I'm just looking at whether we can fix any bugs for the coming >release. Yes, makes sense. Both Zoltan's and my proposal qualify for bug fixing IMO, and both are an improvement over the current code (fixing memory leaks). I'm not too attached to either proposal, but I'd prefer a third opinion on the matter. @Phil: As maintainer and one who worked on the very same issues before you look like a good candidate. Though I'm not sure if your time and focus allows for that right now. Best regards, Bernhard > >> > >> >Thanks! >> > >> > >> >> See also: >> >> * https://lore.kernel.org/qemu-devel/0FFB5FD2-08CE-4CEC-9001-E7AC24407A44@gmail. >> >> com/ >> >> * "Remove intermediate IRQ forwarder" patches in >> >> https://lore.kernel.org/qemu-devel/20230210163744.32182-1-philmd@linaro.org/ >> >> >> >> Testing done: >> >> * Boot amigaone machine into Linux >> >> * Boot pegasos2 machine into MorphOS >> >> * Start fuloong2e machine and check that it doesn't abort >> >> * Boot malta machine with https://people.debian.org/~gio/dqib/ >> >> >> >> Bernhard Beschow (3): >> >> hw/isa/vt82c686: Turn "intr" irq into a named gpio >> >> hw/isa/vt82c686: Resolve intermediate IRQ forwarder >> >> hw/isa/piix: Resolve intermediate IRQ forwarder >> >> >> >> hw/isa/piix.c | 13 ++----------- >> >> hw/isa/vt82c686.c | 12 ++---------- >> >> hw/mips/fuloong2e.c | 2 +- >> >> hw/mips/malta.c | 4 +--- >> >> hw/ppc/amigaone.c | 8 ++++---- >> >> hw/ppc/pegasos2.c | 4 ++-- >> >> 6 files changed, 12 insertions(+), 31 deletions(-) >> >> >> >> -- >> >> 2.45.2 >> >> >> > >
On Wed, 24 Jul 2024, Bernhard Beschow wrote: > Am 23. Juli 2024 00:21:32 UTC schrieb "Michael S. Tsirkin" <mst@redhat.com>: >> On Mon, Jul 22, 2024 at 10:21:30PM +0000, Bernhard Beschow wrote: >>> >>> >>> Am 20. Juli 2024 18:38:42 UTC schrieb "Michael S. Tsirkin" <mst@redhat.com>: >>>> On Thu, Jul 04, 2024 at 10:58:51PM +0200, Bernhard Beschow wrote: >>>>> This series first turns vt82c686's "INTR" pin into a named GPIO for better >>>>> comprehensibility. It then continues fixing qemu_irq memory leaks in vt82c686 >>>>> and piix4 by connecting out IRQs of the south bridges before they get realized. >>>>> This approach is already used in the pc machines after it had been discussed at >>>>> KVM forum `23. >>>>> >>>>> Observe that out IRQs are callbacks such as an INTR IRQ handler in a CPU which a >>>>> south bridge wants to trigger. If, as an implementation detail, the south bridge >>>>> wants to pass this callback to a child device, such as the PIC, then this >>>>> callback must be known to the south bridge before it gets realized. Otherwise >>>>> board code had to wire the PIC device itself, breaking encapsulation. This means >>>>> that qdev_connect_gpio_out*() has to be called before realize() which this >>>>> series implements. Another way to look at it is that callbacks apparently are >>>>> resouces such as memory regions which are also populated before realize(). >>>>> >>>>> Please check if above paragraph makes sense. >>>>> >>>>> Best regards, >>>>> Bernhard >>>> >>>> >>>> Bernhard, do you intend to resolve Zoltan's comments on this >>>> patchset? >>> >>> Hi Michael, >>> >>> I was hoping for some more comments on the last two commits. These resolve various issues in one go, some of which have been addressed in the past. Since this is apparently a new style to handle out-GPIOs, I'd like to get an OK from the community. >>> >>> As Zoltan writes the ideal solution might be to qdev'ify the PIC. This is a bigger task which is more than I intended to do in this series. >>> >>> Best regards, >>> Bernhard >> >> >> ATM I'm just looking at whether we can fix any bugs for the coming >> release. > > Yes, makes sense. Both Zoltan's and my proposal qualify for bug fixing > IMO, and both are an improvement over the current code (fixing memory > leaks). I'm not too attached to either proposal, but I'd prefer a third Maybe there is no real bugs to fix. We don't allocate these devices other than creating it once in the beginning with the machine that lives throughout the QEMU session. So plugging memory leaks is only to silence some analysers that complain about this but it won't fix any real bug. It's still good to silence the warnings which may even lead to failed compilation with some compilers. > opinion on the matter. @Phil: As maintainer and one who worked on the > very same issues before you look like a good candidate. Though I'm not > sure if your time and focus allows for that right now. I already said that my problem with your approach is that it relies on confusing rules for using gpio outs and gpio ins (one would need to be connected before realize and the other one after). I think qemu_irq is already a confusing part of QEMU hiding a pointer, qemu_gpio adds another level of complexity to thar and adding more rules making it inconsistent and behave differently in hard to remember ways is too much for anybody to use it correctly. So instead of making this more complex for users and changing all the call sites, it's better to fix it at the root and allow users to simply use it. Currently closest to the root is within vt82c686 as i8259 isn't yet completely QDev-ified (some very old code that is used by a lot of other devices like i8259 is still pre QDev using legacy init). The qemu_gpio is a QDev thing and QDev is meant to model devices so they can eventually be combined into a machine without board code just using a declarative description. So gpios should be used for outside facing pins of the device and it's more intuitive to connect these pins after the device is realized than to connect some of them before realize and some of them after. Using gpio for internal connections just because it's freed with the device is also wrong. For that embedding qemu_irq would be the way that's currently used for similar cases of one device internally using another (like SoCs). (I have a problem with embedding devices too as that exposes their state struct and thus implementation details outside of object implementation but that's another issue. Maybe the best solution would be creating resources and objects as children of the device so it would be freed with the device but some of these can be used outside of the device so then those places would need to reatain/release or ref/unref them which we likely forget to do so it would break either way.) Regards, BALATON Zoltan > Best regards, > Bernhard > >> >>>> >>>> Thanks! >>>> >>>> >>>>> See also: >>>>> * https://lore.kernel.org/qemu-devel/0FFB5FD2-08CE-4CEC-9001-E7AC24407A44@gmail. >>>>> com/ >>>>> * "Remove intermediate IRQ forwarder" patches in >>>>> https://lore.kernel.org/qemu-devel/20230210163744.32182-1-philmd@linaro.org/ >>>>> >>>>> Testing done: >>>>> * Boot amigaone machine into Linux >>>>> * Boot pegasos2 machine into MorphOS >>>>> * Start fuloong2e machine and check that it doesn't abort >>>>> * Boot malta machine with https://people.debian.org/~gio/dqib/ >>>>> >>>>> Bernhard Beschow (3): >>>>> hw/isa/vt82c686: Turn "intr" irq into a named gpio >>>>> hw/isa/vt82c686: Resolve intermediate IRQ forwarder >>>>> hw/isa/piix: Resolve intermediate IRQ forwarder >>>>> >>>>> hw/isa/piix.c | 13 ++----------- >>>>> hw/isa/vt82c686.c | 12 ++---------- >>>>> hw/mips/fuloong2e.c | 2 +- >>>>> hw/mips/malta.c | 4 +--- >>>>> hw/ppc/amigaone.c | 8 ++++---- >>>>> hw/ppc/pegasos2.c | 4 ++-- >>>>> 6 files changed, 12 insertions(+), 31 deletions(-) >>>>> >>>>> -- >>>>> 2.45.2 >>>>> >>>> >> > >