Am 3. Juli 2024 00:09:45 UTC schrieb BALATON Zoltan <bala...@eik.bme.hu>:
>On Tue, 2 Jul 2024, Bernhard Beschow wrote:
>> Am 2. Juli 2024 18:42:23 UTC schrieb Bernhard Beschow <shen...@gmail.com>:
>>> Am 1. Juli 2024 12:58:15 UTC schrieb Peter Maydell
>>> <peter.mayd...@linaro.org>:
>>>> On Sat, 29 Jun 2024 at 21:01, BALATON Zoltan <bala...@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 <bala...@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-e7ac24407...@gmail.com/
>>>
>>> Best regards,
>>> Bernhard
>>>
>>>>
>>>> -- PMM
>>>>
>>
>>
