Re: [PATCH v7 3/6] hw/isa/vt82c686: Implement PCI IRQ routing

[Bernhard Beschow]

Am 5. März 2023 17:08:30 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Sun, 5 Mar 2023, Bernhard Beschow wrote:
>> Am 5. März 2023 14:05:49 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>>> 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. The real chip may allow routing IRQs from
>>> internal functions independently of PCI interrupts but since guests
>>> usually configute it to a single shared interrupt we don't model that
>>> here for simplicity.
>>> 
>>> Note: The implementation was taken from piix4_set_irq() in hw/isa/piix4.
>>> 
>>> Suggested-by: Bernhard Beschow <shentey@gmail.com>
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> Tested-by: Rene Engel <ReneEngel80@emailn.de>
>>> ---
>>> hw/isa/vt82c686.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 42 insertions(+)
>>> 
>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>>> index 8900d87f59..e5aa467506 100644
>>> --- a/hw/isa/vt82c686.c
>>> +++ b/hw/isa/vt82c686.c
>>> @@ -600,6 +600,46 @@ void via_isa_set_irq(PCIDevice *d, int n, int level)
>>>     qemu_set_irq(s->isa_irqs_in[n], 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 i, pic_level, pic_irq = via_isa_get_pci_irq(s, irq_num);
>>> +
>>> +    /* IRQ 0 and 15 mean disabled, IRQ 2 is reserved */
>> 
>> The vt82c686b datasheet mentions that IRQs 2, 8 and 13 are reserved (-> 
>> guest errors) while only 0 means disabled. I think below code should reflect 
>> this.
>
>We can't because 8 and 13 are allowed for USB and ac97 but 15 means disabled 
>for those. My original implementation would have allowed to implement that but 
>this one from you mixes everyting with the PIRQ pins

What I think should be done is not to mix the PIRQ pins with the IRQ lines. 
I.e. we should only stick to the PCI IRQ routing register descriptions being 
implemented here. See how IRQs 8 & 13 are missing for PCI usage in section "IRQ 
resources" of page 3-22 in 
https://cdn.viaembedded.com/eol_products/docs/epia-v/user_manual/epia-v_manual_v1.2.pdf
 (manual of a main board using the VT8231).

Best regards,
Bernhard

>so we can't implement different reserved/disabled values for different IRQ 
>sources so this is the best we can do. IRQ 0 is already handled as disabled by 
>the code below.
>
>Regards,
>BALATON Zoltan
>
>>> +    if (unlikely(pic_irq == 0 || pic_irq == 2 || pic_irq > 14)) {
>>> +        if (pic_irq == 2) {
>>> +            qemu_log_mask(LOG_GUEST_ERROR, "Invalid ISA IRQ routing");
>>> +        }
>>> +        return;
>>> +    }
>>> +
>>> +    /* 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);
>>> +        }
>>> +    }
>>> +    /* Now we change the pic irq level according to the via irq mappings. 
>>> */
>>> +    qemu_set_irq(s->isa_irqs_in[pic_irq], pic_level);
>>> +}
>>> +
>>> static void via_isa_realize(PCIDevice *d, Error **errp)
>>> {
>>>     ViaISAState *s = VIA_ISA(d);
>>> @@ -620,6 +660,8 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
>>>     i8254_pit_init(isa_bus, 0x40, 0, NULL);
>>>     i8257_dma_init(isa_bus, 0);
>>> 
>>> +    qdev_init_gpio_in_named(dev, via_isa_set_pci_irq, "pirq", 
>>> PCI_NUM_PINS);
>>> +
>>>     /* RTC */
>>>     qdev_prop_set_int32(DEVICE(&s->rtc), "base_year", 2000);
>>>     if (!qdev_realize(DEVICE(&s->rtc), BUS(isa_bus), errp)) {
>> 
>>