| Message ID | 20240627-san-v2-4-750bb0946dbd@daynix.com |
|---|---|
| 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); > >
On 2024/06/29 22:08, BALATON Zoltan wrote: > 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. According to qdev_pass_gpios(), it is valid to have a GPIO line even if a QOM device to be connected with the GPIO line is actually included in one chip package. I didn't use qdev_pass_gpios() because it cannot expose a subset of the GPIO array. Regards, Akihiko Odaki
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(-)