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

[Bernhard Beschow]

Am 3. Juli 2024 00:09:45 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Tue, 2 Jul 2024, Bernhard Beschow wrote:
>> Am 2. Juli 2024 18:42:23 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>>> 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
>
>The INTR pin corresponds to qemu_irq cpu_intr not the i8259_irq.
>
>>> 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.
>> 
>> Well, QEMU does actually wire the intr pin in the pegasos2 board code, 
>> except that it isn't a named gpio like in piix4. If we allow this pin to
>
>I could make that named to make it clearer, now it's the only output gpio so 
>did not name it as usually devices that only have one output don't use named 
>gpios for that.
>
>> be wired before the south bridge's realize we might be able to eliminate the 
>> "intermediate irq forwarder" as Phil used to name it, resulting in less and 
>> more efficient code. This solution would basically follow the pattern I 
>> outlined under below link.
>
>I think the problem here is that i8259 does not provide an output gpio for 
>this interrupt that the VT82xx could pass on but instead i8259_init() needs a 
>qemu_irq to be passed rhat the i8259 model will set. This seems to be a legacy 
>init function so the fix may be to Qdev-ify i8259 and add an output irq to it 
>then its users could instantiate and connect its IRQs as usual and we don't 
>need to create a qemu_irq to pass it to i8259_init().

I've implemented the approach avoiding the intermediate IRQ forwarders here: 
https://github.com/shentok/qemu/commits/upstream/vt82c686-irq/ . I'd send this 
series to the list as soon as I resolve some email authentication issues.

Best regards,
Bernhard

>
>Regards,
>BALATON Zoltan
>
>> Best regards,
>> Bernhard
>> 
>>> 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
>>>> 
>> 
>>