Re: [PATCH v5 3/7] hw/isa/vt82c686: Implement PCI IRQ routing

[Bernhard Beschow <shentey@gmail.com>]

Am 1. März 2023 11:15:02 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Wed, 1 Mar 2023, Bernhard Beschow wrote:
>> Am 1. März 2023 00:17:09 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>
>>> ---
>>> hw/isa/vt82c686.c | 38 +++++++++++++++++++++++++++++++++++++-
>>> 1 file changed, 37 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>>> index 01e0148967..018a119964 100644
>>> --- a/hw/isa/vt82c686.c
>>> +++ b/hw/isa/vt82c686.c
>>> @@ -604,6 +604,42 @@ 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 i, pic_level, pic_irq = via_isa_get_pci_irq(s, irq_num);
>>> +
>>> +    if (unlikely(pic_irq == 0 || pic_irq == 2 || pic_irq > 14)) {
>> 
>> Where does the "pic_irq > 14" come from? It's not mentioned in the datasheet.
>
>Check at 0x3c register of USB and AC97 functions. For the others it may be valid but unlikely to be used hence we just disallow it. (In my version which also mapped IDE here I've checkrf for each source but there's no way to do that in this version.)

I'm not sure what you mean. The 0x3c regs aren't related to the PCI IRQ routing regs.

Moreover, as I wrote in my other mail, I wonder what the datasheet tries to tell us here at all. The information there partly contradicts itself.

Can you please clarify?

Thanks,
Bernhard

>
>Regards,
>BALATON Zoltan
>
>>> +        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);
>>> @@ -615,9 +651,9 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
>>> 
>>>     qdev_init_gpio_out(dev, &s->cpu_intr, 1);
>>>     isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1);
>>> +    qdev_init_gpio_in_named(dev, via_isa_set_pci_irq, "pirq", PCI_NUM_PINS);
>>>     isa_bus = isa_bus_new(dev, pci_address_space(d), pci_address_space_io(d),
>>>                           errp);
>>> -
>>>     if (!isa_bus) {
>>>         return;
>>>     }
>> 
>>