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

[Bernhard Beschow]

Am 23. Februar 2023 23:47:58 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Thu, 23 Feb 2023, Bernhard Beschow wrote:
>> Am 23. Februar 2023 21:11:23 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>>> On Thu, 23 Feb 2023, 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);
>>> 
>>> Please no oversimplification. This replaces the connections to mv64361 gpp 
>>> pins made in mv64361_realize() and breaks the interrupts that can be 
>>> enabled in mv64361.
>> 
>> Let's split our work as follows: You'll do the audio and pegasos2 changes 
>> including exporting the pirq properties, I'll do the first three routing 
>> patches of this series as the base.
>
>I'm OK with doing audio as said and already did the PIRQ and pegaosos2 changes 
>(patch 2 and 3 in my series), just take those without deleting half of them.

I can only take the three VT82xx patches as I proposed since I don't know the 
Pegasos2 board as well as you do and I don't want to iterate on any review 
comments for the other patches. I'll send my series soonish.

Best regards,
Bernhard

>So drop the last two via-ac97 patches and do the IRQ fixes in your way but 
>keep working what now works.
>
>>> I've implemented that for something but can't remember now which guest 
>>> exactly,
>> 
>> Could you please put this information into the commit message or into the 
>> code? That would ease maintainance a lot.
>
>I did, see patch 3 in my series.
>
>>> but this would be needed so please restore my pegasos2 patch and move this 
>>> there connecting both mv64361 and via-isa to PCI interrupts as shown in the 
>>> schematics. That means you also need the PIRQ pins here. Can you do a new 
>>> version with that?
>> 
>> As proposed above I'd fold the first three patches into a separate series 
>> which you can build upon. I have no way to test the pegasos2 IRQ changes 
>> since the test cases I'm aware of either work or we agreed that they can be 
>> fixed later (-> USB).
>
>I did fix the USB just haven't sent a v2 yet due to this thread but it's just 
>the change I've sent yesterday, just add this before qemu_set_irq at the end 
>of via_isa_set_irq() in my series. This is what I have now:
>
>+    uint16_t old_state;
>+    if (old_state && s->isa_irq_state[isa_irq]) {
>+        /* FIXME: i8259 model does not support level sensitive mode */
>+        qemu_set_irq(s->isa_irqs[isa_irq], 0);
>+    }
>
>How to do that with your version I have no idea but this fixed the problem 
>with my series. I can send a v2 of this patch with this change if it's not 
>clear from this and the other message what I did.
>
>>> I'll try this one in the meantime
>> 
>> Sounds good to me -- that's what I wanted to achieve ;) Thanks!
>
>I've answered about that in the other message, I've tried with AmigaOS, Debian 
>Linux 8.11.0 netboot iso and MorphOS and they still boot but couldn't test 
>them much yet. MorphOS works on my series with sound and USB and does not hang 
>with the above workaround but found now it still hangs if I send something to 
>it over serial (e.g. press space or enter where you've typed boot cd boot.img 
>after it starts playing sound). This happens on both of our series but only 
>with the via-ac97 patch so probably related to that. This could easily be a 
>guest bug too so I don't care that much, the pegasos2 changes are more 
>interesting to get AmigaOS run well so that's my main focus now, MorphOS 
>already runs on other QEMU machines well. I'll still try to find this out but 
>AmigaOS can use other sound card so as long as the IRQ problems are fixed it 
>would work but we need more than one PCI cards working as we'd need sound card 
>and network card for it to be usable. This was tested to work with my series, 
>if you give alternative series I can ask to have those tested too.
>
>Regards,
>BALATON Zoltan