| Message ID | b70b9e72063b4dd4005bf4bc040b84f2bb617bf4.1719690591.git.balaton@eik.bme.hu (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Solve vt82c686 qemu_irq leak. | expand |
On Sat, 29 Jun 2024 at 21:01, BALATON Zoltan <balaton@eik.bme.hu> wrote: > > To avoid a warning about unfreed qemu_irq embed the i8259 irq in the > device state instead of allocating it. > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> > --- > hw/isa/vt82c686.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c > index 8582ac0322..834051abeb 100644 > --- a/hw/isa/vt82c686.c > +++ b/hw/isa/vt82c686.c > @@ -592,6 +592,8 @@ OBJECT_DECLARE_SIMPLE_TYPE(ViaISAState, VIA_ISA) > > struct ViaISAState { > PCIDevice dev; > + > + IRQState i8259_irq; > qemu_irq cpu_intr; > qemu_irq *isa_irqs_in; > uint16_t irq_state[ISA_NUM_IRQS]; > @@ -715,13 +717,12 @@ static void via_isa_realize(PCIDevice *d, Error **errp) > ViaISAState *s = VIA_ISA(d); > DeviceState *dev = DEVICE(d); > PCIBus *pci_bus = pci_get_bus(d); > - qemu_irq *isa_irq; > ISABus *isa_bus; > int i; > > qdev_init_gpio_out(dev, &s->cpu_intr, 1); > qdev_init_gpio_in_named(dev, via_isa_pirq, "pirq", PCI_NUM_PINS); > - isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1); > + qemu_init_irq(&s->i8259_irq, via_isa_request_i8259_irq, s, 0); > isa_bus = isa_bus_new(dev, pci_address_space(d), pci_address_space_io(d), > errp); So if I understand correctly, this IRQ line isn't visible from outside this chip, we're just trying to wire together two internal components of the chip? If so, I agree that this seems a better way than creating a named GPIO that we then have to document as a "not really an external connection, don't try to use this" line. (We've done that before I think in other devices, and it works but it's a bit odd-looking.) That said, I do notice that the via_isa_request_i8259_irq() function doesn't do anything except pass the level onto another qemu_irq, so I think the theoretical ideal would be if we could arrange to plumb things directly through rather than needing this extra qemu_irq and function. There's probably a reason (order of device creation/connection?) that doesn't work though. -- PMM
On Mon, 1 Jul 2024, Peter Maydell wrote: > On Sat, 29 Jun 2024 at 21:01, BALATON Zoltan <balaton@eik.bme.hu> wrote: >> >> To avoid a warning about unfreed qemu_irq embed the i8259 irq in the >> device state instead of allocating it. >> >> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >> --- >> hw/isa/vt82c686.c | 7 ++++--- >> 1 file changed, 4 insertions(+), 3 deletions(-) >> >> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c >> index 8582ac0322..834051abeb 100644 >> --- a/hw/isa/vt82c686.c >> +++ b/hw/isa/vt82c686.c >> @@ -592,6 +592,8 @@ OBJECT_DECLARE_SIMPLE_TYPE(ViaISAState, VIA_ISA) >> >> struct ViaISAState { >> PCIDevice dev; >> + >> + IRQState i8259_irq; >> qemu_irq cpu_intr; >> qemu_irq *isa_irqs_in; >> uint16_t irq_state[ISA_NUM_IRQS]; >> @@ -715,13 +717,12 @@ static void via_isa_realize(PCIDevice *d, Error **errp) >> ViaISAState *s = VIA_ISA(d); >> DeviceState *dev = DEVICE(d); >> PCIBus *pci_bus = pci_get_bus(d); >> - qemu_irq *isa_irq; >> ISABus *isa_bus; >> int i; >> >> qdev_init_gpio_out(dev, &s->cpu_intr, 1); >> qdev_init_gpio_in_named(dev, via_isa_pirq, "pirq", PCI_NUM_PINS); >> - isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1); >> + qemu_init_irq(&s->i8259_irq, via_isa_request_i8259_irq, s, 0); >> isa_bus = isa_bus_new(dev, pci_address_space(d), pci_address_space_io(d), >> errp); > > So if I understand correctly, this IRQ line isn't visible > from outside this chip, we're just trying to wire together > two internal components of the chip? If so, I agree that > this seems a better way than creating a named GPIO that > we then have to document as a "not really an external > connection, don't try to use this" line. (We've done that > before I think in other devices, and it works but it's > a bit odd-looking.) > > That said, I do notice that the via_isa_request_i8259_irq() > function doesn't do anything except pass the level onto > another qemu_irq, so I think the theoretical ideal would be > if we could arrange to plumb things directly through rather > than needing this extra qemu_irq and function. There's > probably a reason (order of device creation/connection?) > that doesn't work though. Philippe tried that before but was found not working. There's some history that I don't remember aby more in commit 38200011319b58. Regards, BALATON Zoltan
On 01/07/2024 13:58, Peter Maydell wrote: > On Sat, 29 Jun 2024 at 21:01, BALATON Zoltan <balaton@eik.bme.hu> wrote: >> >> To avoid a warning about unfreed qemu_irq embed the i8259 irq in the >> device state instead of allocating it. >> >> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >> --- >> hw/isa/vt82c686.c | 7 ++++--- >> 1 file changed, 4 insertions(+), 3 deletions(-) >> >> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c >> index 8582ac0322..834051abeb 100644 >> --- a/hw/isa/vt82c686.c >> +++ b/hw/isa/vt82c686.c >> @@ -592,6 +592,8 @@ OBJECT_DECLARE_SIMPLE_TYPE(ViaISAState, VIA_ISA) >> >> struct ViaISAState { >> PCIDevice dev; >> + >> + IRQState i8259_irq; >> qemu_irq cpu_intr; >> qemu_irq *isa_irqs_in; >> uint16_t irq_state[ISA_NUM_IRQS]; >> @@ -715,13 +717,12 @@ static void via_isa_realize(PCIDevice *d, Error **errp) >> ViaISAState *s = VIA_ISA(d); >> DeviceState *dev = DEVICE(d); >> PCIBus *pci_bus = pci_get_bus(d); >> - qemu_irq *isa_irq; >> ISABus *isa_bus; >> int i; >> >> qdev_init_gpio_out(dev, &s->cpu_intr, 1); >> qdev_init_gpio_in_named(dev, via_isa_pirq, "pirq", PCI_NUM_PINS); >> - isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1); >> + qemu_init_irq(&s->i8259_irq, via_isa_request_i8259_irq, s, 0); >> isa_bus = isa_bus_new(dev, pci_address_space(d), pci_address_space_io(d), >> errp); > > So if I understand correctly, this IRQ line isn't visible > from outside this chip, we're just trying to wire together > two internal components of the chip? If so, I agree that > this seems a better way than creating a named GPIO that > we then have to document as a "not really an external > connection, don't try to use this" line. (We've done that > before I think in other devices, and it works but it's > a bit odd-looking.) It's basically wiring up the 8259 PIC (ISA) IRQs which aren't implemented using qdev gpios to an internal IRQ handler. If the 8259 PIC (ISA) IRQs were already converted to qdev gpios, then presumably using a qdev gpio would be the correct solution for this? I'm conscious that if we do decide to introduce another method for wiring IRQs then there should be clear criteria for developers and reviewers as to which method is appropriate for each use case. ATB, Mark.
diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c index 8582ac0322..834051abeb 100644 --- a/hw/isa/vt82c686.c +++ b/hw/isa/vt82c686.c @@ -592,6 +592,8 @@ OBJECT_DECLARE_SIMPLE_TYPE(ViaISAState, VIA_ISA) struct ViaISAState { PCIDevice dev; + + IRQState i8259_irq; qemu_irq cpu_intr; qemu_irq *isa_irqs_in; uint16_t irq_state[ISA_NUM_IRQS]; @@ -715,13 +717,12 @@ static void via_isa_realize(PCIDevice *d, Error **errp) ViaISAState *s = VIA_ISA(d); DeviceState *dev = DEVICE(d); PCIBus *pci_bus = pci_get_bus(d); - qemu_irq *isa_irq; ISABus *isa_bus; int i; qdev_init_gpio_out(dev, &s->cpu_intr, 1); qdev_init_gpio_in_named(dev, via_isa_pirq, "pirq", PCI_NUM_PINS); - isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1); + qemu_init_irq(&s->i8259_irq, via_isa_request_i8259_irq, s, 0); isa_bus = isa_bus_new(dev, pci_address_space(d), pci_address_space_io(d), errp); @@ -729,7 +730,7 @@ static void via_isa_realize(PCIDevice *d, Error **errp) return; } - s->isa_irqs_in = i8259_init(isa_bus, *isa_irq); + s->isa_irqs_in = i8259_init(isa_bus, &s->i8259_irq); isa_bus_register_input_irqs(isa_bus, s->isa_irqs_in); i8254_pit_init(isa_bus, 0x40, 0, NULL); i8257_dma_init(OBJECT(d), isa_bus, 0);
To avoid a warning about unfreed qemu_irq embed the i8259 irq in the device state instead of allocating it. Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> --- hw/isa/vt82c686.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)