Re: [PATCH v3 4/8] hw/isa/vt82c686: Implement PCI IRQ routing

[Bernhard Beschow]

Am 26. Februar 2023 23:33:20 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Sun, 26 Feb 2023, Bernhard Beschow wrote:
>> Am 25. Februar 2023 18:11:49 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>>> From: Bernhard Beschow <shentey@gmail.com>
>>> 
>>> The real VIA south bridges implement a PCI IRQ router which is configured
>>> by the BIOS or the OS. In order to respect these configurations, QEMU
>>> needs to implement it as well.
>>> 
>>> Note: The implementation was taken from piix4_set_irq() in hw/isa/piix4.
>>> 
>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>> [balaton: declare gpio inputs instead of changing pci bus irqs so it can
>>> be connected in board code; remove some empty lines]
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> Tested-by: Rene Engel <ReneEngel80@emailn.de>
>>> ---
>>> hw/isa/vt82c686.c | 39 +++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 39 insertions(+)
>>> 
>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>>> index 3f9bd0c04d..4025f9bcdc 100644
>>> --- a/hw/isa/vt82c686.c
>>> +++ b/hw/isa/vt82c686.c
>>> @@ -604,6 +604,44 @@ static void via_isa_request_i8259_irq(void *opaque, 
>>> int irq, int level)
>>>     qemu_set_irq(s->cpu_intr, level);
>>> }
>>> 
>>> +static int via_isa_get_pci_irq(const ViaISAState *s, int irq_num)
>>> +{
>>> +    switch (irq_num) {
>>> +    case 0:
>>> +        return s->dev.config[0x55] >> 4;
>>> +    case 1:
>>> +        return s->dev.config[0x56] & 0xf;
>>> +    case 2:
>>> +        return s->dev.config[0x56] >> 4;
>>> +    case 3:
>>> +        return s->dev.config[0x57] >> 4;
>>> +    }
>>> +    return 0;
>>> +}
>>> +
>>> +static void via_isa_set_pci_irq(void *opaque, int irq_num, int level)
>>> +{
>>> +    ViaISAState *s = opaque;
>>> +    PCIBus *bus = pci_get_bus(&s->dev);
>>> +    int pic_irq;
>>> +
>>> +    /* now we change the pic irq level according to the via irq mappings */
>>> +    /* XXX: optimize */
>>> +    pic_irq = via_isa_get_pci_irq(s, irq_num);
>>> +    if (pic_irq < ISA_NUM_IRQS) {
>>> +        int i, pic_level;
>>> +
>>> +        /* The pic level is the logical OR of all the PCI irqs mapped to 
>>> it. */
>>> +        pic_level = 0;
>>> +        for (i = 0; i < PCI_NUM_PINS; i++) {
>>> +            if (pic_irq == via_isa_get_pci_irq(s, i)) {
>>> +                pic_level |= pci_bus_get_irq_level(bus, i);
>>> +            }
>>> +        }
>>> +        qemu_set_irq(s->isa_irqs[pic_irq], pic_level);
>>> +    }
>>> +}
>>> +
>>> static void via_isa_realize(PCIDevice *d, Error **errp)
>>> {
>>>     ViaISAState *s = VIA_ISA(d);
>>> @@ -614,6 +652,7 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
>>>     int i;
>>> 
>>>     qdev_init_gpio_out(dev, &s->cpu_intr, 1);
>>> +    qdev_init_gpio_in_named(dev, via_isa_set_pci_irq, "pirq", 
>>> PCI_NUM_PINS);
>> 
>> This line is a Pegasos2 specific addition for fixing its IRQ handling. Since 
>> this code must also work with the Fuloong2e board we should aim for a 
>> minimal changeset here which renders this line out of scope.
>> 
>> Let's keep the two series separate since now I need to watch two series for 
>> comments. Please use Based-on: tag next time instead.
>
>Well, it's not. It's part of the QDev model for VT8231 that allows it to be 
>connected by boards so I think this belongs here otherwise this won't even 
>compile because the function you've added would be unused and bail on -Werror. 
>Let's not make this more difficult than it is. I'm OK with reasonable changes 
>but what's your goal now? You can't get rid of this line as it's how QDev can 
>model it. Either I have to call into this model or have to export these pins 
>as gpios.

Exporting the pins is a separate aspect on top of implementing PCI IRQ routing. 
To make this clear and obvious this should be a dedicated patch. In case either 
patch has an issue this would also ease and therefore acellerate discussions.

Best regards,
Bernhard

>
>Regards,
>BALATON Zoltan
>
>> Thanks,
>> Bernhard
>> 
>>>     isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1);
>>>     isa_bus = isa_bus_new(dev, pci_address_space(d), 
>>> pci_address_space_io(d),
>>>                           errp);
>> 
>>