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

[Bernhard Beschow]

Am 3. Juli 2024 11:13:08 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Wed, 3 Jul 2024, Bernhard Beschow wrote:
>> 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.
>
>This connects the gpio out before the device is realized. I don't think that's 
>the right fix and confuses all the users of this device as they will need to 
>remember to do this. I think the current interrupt forwarder is OK until i8259 
>is Qdev-ified and solves this within the device. I'm OK with the patch that 
>makes intr named if you can submit just that.

I've now sent a series with naming the gpio as the first patch: 
20240704205854.18537-1-shentey@gmail.com/">https://lore.kernel.org/qemu-devel/20240704205854.18537-1-shentey@gmail.com/

Best regards,
Bernhard

>
>Regards,
>BALATON Zoltan