diff mbox series

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

Message ID 2c1aa2fad58fffa4e26e8e271243ed30ecd9d41d.1678023358.git.balaton@eik.bme.hu
State New
Headers show
Series Pegasos2 fixes and audio output support | expand

Commit Message

BALATON Zoltan March 5, 2023, 2:05 p.m. UTC 
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(+)

Comments

Bernhard Beschow March 5, 2023, 4:44 p.m. UTC | #1 
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.

Best regards,
Bernhard

>+    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)) {
BALATON Zoltan March 5, 2023, 5:08 p.m. UTC | #2 
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 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)) {
>
>
Bernhard Beschow March 5, 2023, 9:19 p.m. UTC | #3 
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)) {
>> 
>>
BALATON Zoltan March 6, 2023, 12:23 a.m. UTC | #4 
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 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)) {
>>>
>>>
>
>
diff mbox series

Patch

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 */
+    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)) {