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

[Bernhard Beschow]

Am 26. Februar 2023 23:37:24 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Sun, 26 Feb 2023, Bernhard Beschow wrote:
>> Am 26. Februar 2023 22:27:50 UTC schrieb "Philippe Mathieu-Daudé" 
>> <philmd@linaro.org>:
>>> On 25/2/23 19:11, BALATON Zoltan wrote:
>>>> 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(+)
>>> 
>>>> +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) {
>>> 
>>> the ISA IRQ is stored in 4-bit so will always be in range.
>> 
>> Indeed. I'd turn this into an assert to keep this assum visible. I'll do 
>> another iteration of the PCI IRQ router series.
>
>If you do that please don't break it and make me redo this series again. 
>Sumbit a patch as a reply to this that can be substituted without changing 
>other patches and I can include in later respins.

I will only do the mist minimal changes satisfying the review comments. This 
should minimize the risk of breakage. Also, you can minimize the chance of 
breakage on your side by not introducing more changes than needed, e.g. by not 
doing any formatting changes.

>We don't have time to make extensive changes now, the freeze is too close.

I don't intend to make extensive changes unless review comments requre it.

Best regards,
Bernhard

>
>Regards,
>BALATON Zoltan
>
>> Best regards,
>> Bernhard
>>> 
>>>> +        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);
>>>> +    }
>>>> +}
>>> 
>>> 
>> 
>>