Re: [PATCH 2/2] hw/isa/vt82c686.c: Embed i8259 irq in device state instead of allocating

[Bernhard Beschow]

Am 1. Juli 2024 12:58:15 UTC schrieb Peter Maydell <peter.maydell@linaro.org>:
>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,

Actally it is, in the form of the INTR pin. Assuming similar naming conventions 
in vt82xx and piix, one can confirm this by consulting the piix4 datasheet, 
"Figure 5. Interrupt Controller Block Diagram". Moreover, the pegasos2 
schematics (linked in the QEMU documentation) suggest that this pin is actually 
used there, although not modeled in QEMU. Compare this to how the "intr" pin is 
exposed by the piix4 device model and wired in the Malta board.

> 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.

I think there could be a general pattern of device creation/connection which 
I've outlined here: 
https://lore.kernel.org/qemu-devel/0FFB5FD2-08CE-4CEC-9001-E7AC24407A44@gmail.com/

Best regards,
Bernhard

>
>-- PMM
>