|
| From: | BALATON Zoltan |
| Subject: | Re: [PATCH v7 3/6] hw/isa/vt82c686: Implement PCI IRQ routing |
| Date: | Mon, 6 Mar 2023 01:23:14 +0100 (CET) |
On Sun, 5 Mar 2023, Bernhard Beschow wrote:
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 pinsWhat I think should be done is not to mix the PIRQ pins with the IRQ lines.
I thought the same and implemented it that way first allowing this function to take interrupt source so it can know where the IRQ is coming from and then map the internal functions and PIRQ pins independently. That way it could also check for different Disabled and Reserved values. But then you've replaced that with this patch which does mix PIRQ with internal functions as those will also come in through this function. The internal functions aren't PCi devices but you've forced them to be modelled that way. Now there's no way to separately check different reserved and disabled IRQs for those and using the PIRQ restrictions here would be wrong as that would restrict the internal functions beyond what's documented. It's more likely a guest would set an allowed interrupt for the internal functions than it is to map a PCI IRQ to 0, 2 or 14/15 as it would ilkely not boot at all because that messes up timer, cascade or IDE. Therefore I'm quite sure you'll never find a guest that would trigger these checks you're now wanting me to add.
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).
IRQs 1 and 6 are also missing from that list so why not check for those then? Enough is enough. What are you trying to prove here? This is useless nitpicking now. Reserved likely means that real hardware does not check and would allow you to mess things up so we don't have to do anytihng with that here either. Do you actually have a use for such checks or you just can't let this patch go? This was tested as it is and any last minute change is increasing the chance of breaking it so I'd stay with this now as this could be changed later if you show me a guest that needs a check for reserved IRQ 8 and 13 here but at this point we don't have time to re-test this series again.
Regards, BALATON Zoltan
Best regards, Bernhardso 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)) {
| [Prev in Thread] | Current Thread | [Next in Thread] |