| Message ID | 20240627-san-v2-4-750bb0946dbd@daynix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Fix check-qtest-ppc64 sanitizer errors | expand |
On 27/06/2024 14:37, Akihiko Odaki wrote: > This fixes qemu_irq array leak. > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > --- > 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 8582ac0322eb..629d2d568137 100644 > --- a/hw/isa/vt82c686.c > +++ b/hw/isa/vt82c686.c > @@ -715,13 +715,14 @@ 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; > + 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); > + qdev_init_gpio_in_named(dev, via_isa_request_i8259_irq, "i8259", 1); > + isa_irq = qdev_get_gpio_in_named(dev, "i8259", 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, isa_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); Have you confirmed that the machines using the VIA can still boot correctly with this change? I have a vague memory that Phil tried something like this, but due to legacy code poking around directly in the ISA IRQ array after realize it caused some things to break. ATB, Mark.
On 2024/06/28 16:27, Mark Cave-Ayland wrote: > On 27/06/2024 14:37, Akihiko Odaki wrote: > >> This fixes qemu_irq array leak. >> >> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> >> --- >> 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 8582ac0322eb..629d2d568137 100644 >> --- a/hw/isa/vt82c686.c >> +++ b/hw/isa/vt82c686.c >> @@ -715,13 +715,14 @@ 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; >> + 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); >> + qdev_init_gpio_in_named(dev, via_isa_request_i8259_irq, "i8259", 1); >> + isa_irq = qdev_get_gpio_in_named(dev, "i8259", 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, isa_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); > > Have you confirmed that the machines using the VIA can still boot > correctly with this change? I have a vague memory that Phil tried > something like this, but due to legacy code poking around directly in > the ISA IRQ array after realize it caused some things to break. I tried to boot MorphOS on pegasos2 but it didn't boot even with QEMU 9.0.1. The command line I used is: qemu-system-ppc -M pegasos2 -rtc base=localtime -device ati-vga,guest_hwcursor=true,romfile= -cdrom morphos-3.18.iso -kernel boot.img -serial stdio That said, I believe no such code remain now. The IRQ array is held in a variable local in this function and nobody else can refer to it. Regards, Akihiko Odaki
On 2024/06/29 16:38, Akihiko Odaki wrote: > On 2024/06/28 16:27, Mark Cave-Ayland wrote: >> On 27/06/2024 14:37, Akihiko Odaki wrote: >> >>> This fixes qemu_irq array leak. >>> >>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> >>> --- >>> 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 8582ac0322eb..629d2d568137 100644 >>> --- a/hw/isa/vt82c686.c >>> +++ b/hw/isa/vt82c686.c >>> @@ -715,13 +715,14 @@ 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; >>> + 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); >>> + qdev_init_gpio_in_named(dev, via_isa_request_i8259_irq, "i8259", >>> 1); >>> + isa_irq = qdev_get_gpio_in_named(dev, "i8259", 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, isa_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); >> >> Have you confirmed that the machines using the VIA can still boot >> correctly with this change? I have a vague memory that Phil tried >> something like this, but due to legacy code poking around directly in >> the ISA IRQ array after realize it caused some things to break. > > I tried to boot MorphOS on pegasos2 but it didn't boot even with QEMU > 9.0.1. The command line I used is: > > qemu-system-ppc -M pegasos2 -rtc base=localtime -device > ati-vga,guest_hwcursor=true,romfile= -cdrom morphos-3.18.iso -kernel > boot.img -serial stdio Apparently I failed to run it properly. I tried again now and it booted with this change. Regards, Akihiko Odaki > > That said, I believe no such code remain now. The IRQ array is held in a > variable local in this function and nobody else can refer to it.
On Thu, 27 Jun 2024, Akihiko Odaki wrote: > This fixes qemu_irq array leak. > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > --- > 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 8582ac0322eb..629d2d568137 100644 > --- a/hw/isa/vt82c686.c > +++ b/hw/isa/vt82c686.c > @@ -715,13 +715,14 @@ 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; > + 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); > + qdev_init_gpio_in_named(dev, via_isa_request_i8259_irq, "i8259", 1); The chip has no such pin so this is a QEMU internal connection that I think should not be modelled with a named gpio. I think we really need a function to init a qemu_irq (for now we only have one that also allocares it) so then we can put this in ViaISAState and init in place. I'll make a patch. Regards, BALATON Zoltan > + isa_irq = qdev_get_gpio_in_named(dev, "i8259", 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, isa_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); > >
diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c index 8582ac0322eb..629d2d568137 100644 --- a/hw/isa/vt82c686.c +++ b/hw/isa/vt82c686.c @@ -715,13 +715,14 @@ 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; + 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); + qdev_init_gpio_in_named(dev, via_isa_request_i8259_irq, "i8259", 1); + isa_irq = qdev_get_gpio_in_named(dev, "i8259", 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, isa_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);
This fixes qemu_irq array leak. Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> --- hw/isa/vt82c686.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)