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);

-- 
2.45.2

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

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.