On Sun, 5 Mar 2023, Bernhard Beschow wrote:
Am 5. März 2023 17:08:30 UTC schrieb BALATON Zoltan <bala...@eik.bme.hu>:
On Sun, 5 Mar 2023, Bernhard Beschow wrote:
Am 5. März 2023 14:05:49 UTC schrieb BALATON Zoltan <bala...@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 <shen...@gmail.com>
Signed-off-by: BALATON Zoltan <bala...@eik.bme.hu>
Tested-by: Rene Engel <reneenge...@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 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,
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)) {
