Re: [PATCH 2/5] hw/isa/vt82c686: Implement PCI IRQ routing

[Bernhard Beschow]

Am 1. März 2023 14:20:54 UTC schrieb Mark Cave-Ayland 
<mark.cave-ayland@ilande.co.uk>:
>On 23/02/2023 20:20, Bernhard Beschow wrote:
>
>> 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>
>> ---
>>   hw/isa/vt82c686.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 44 insertions(+)
>> 
>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>> index 3f9bd0c04d..f24e387d63 100644
>> --- a/hw/isa/vt82c686.c
>> +++ b/hw/isa/vt82c686.c
>> @@ -604,6 +604,48 @@ 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);
>> @@ -676,6 +718,8 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
>>       if (!qdev_realize(DEVICE(&s->mc97), BUS(pci_bus), errp)) {
>>           return;
>>       }
>> +
>> +    pci_bus_irqs(pci_bus, via_isa_set_pci_irq, s, PCI_NUM_PINS);
>>   }
>>     /* TYPE_VT82C686B_ISA */
>
>This looks right, however generally a PCI device shouldn't really be setting 
>PCI bus IRQs: this is normally done by the PCI host bridge. Is it just the 
>case that the x86 world is different here for legacy reasons?

Well, looking at the pegasos2 schematics it seems to me that at least the intA 
+ intB lines are connected to both chips (and that they are even shared between 
the AGP slot and the PCI bus). On the Marvell north bridge they seem to be 
connected to GPIO pins and on the VIA chip to the PCI IRQ router.

Note that GPIO pins can usually be deactivated (tristated) and that the four 
VIA PCI IRQs can be deactivated by assigning "interrupt 0". Such a design would 
allow the lines to be hardwired to both "interrupt controllers", allowing 
system software to use either controller, or possibly even mixed (e.g. intA to 
be treated by the north bridge and intB by VIA).

Such designs are currently not easily implementable in QEMU since only one IRQ 
handler can be assigned to a PCI bus. As a workaround, one could assign a 
custom IRQ handler which implements special handling.

Getting back to your question, I think you are right that assigning the IRQ 
handler in the VIA model may break e.g. the Fuloong2e machine where the IRQ 
handler is set in the north bridge. Since the VIA chip is instantiated later it 
now effectively replaces the handler.

It would be really neat if QEMU allowed for assigning two or more IRQ handlers 
to a PCI bus...

Do you think that two interrupt controllers connected to IRQ lines like that 
sounds reasonable?

Best regards,
Bernhard

>
>
>ATB,
>
>Mark.