Skip to contentSkip to conversation

provide correct pci routing information in mptable

PATCH Superseded Gleb Natapov 2 participants 8 messages Dec 23, 2009 KVM

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Gleb Natapov

author
#0 lore raw

PATCH 1/1 provide correct pci routing information in mptable

OpenBSD uses irq routing from mptable, but doesn't create it correctly for PCI bus. This patch adds PCI routing info into mptable.

Signed-off-by Gleb Natapov <[email protected]>
src/mptable.c +38 -4
index 1920dfe..545a41b 100644
@@ -9,6 +9,8 @@
 #include "config.h" // CONFIG_*
 #include "mptable.h" // MPTABLE_SIGNATURE
 #include "paravirt.h" // qemu_cfg_irq0_override
+#include "pci.h"
+#include "pci_regs.h"
 
 void
 mptable_init(void)

@@ -21,9 +23,9 @@ mptable_init(void)
     // Allocate memory
     int length = (sizeof(struct mptable_config_s)
                   + sizeof(struct mpt_cpu) * MaxCountCPUs
-                  + sizeof(struct mpt_bus)
+                  + sizeof(struct mpt_bus) * 2
                   + sizeof(struct mpt_ioapic)
-                  + sizeof(struct mpt_intsrc) * 18);
+                  + sizeof(struct mpt_intsrc) * 34);
     struct mptable_config_s *config = malloc_fseg(length);
     struct mptable_floating_s *floating = malloc_fseg(sizeof(*floating));
     if (!config || !floating) {

@@ -85,9 +87,17 @@ mptable_init(void)
     struct mpt_bus *bus = (void*)cpu;
     memset(bus, 0, sizeof(*bus));
     bus->type = MPT_TYPE_BUS;
+    bus->busid = 1;
     memcpy(bus->bustype, "ISA   ", sizeof(bus->bustype));
     entrycount++;
 
+    bus++;
+    memset(bus, 0, sizeof(*bus));
+    bus->type = MPT_TYPE_BUS;
+    bus->busid = 0;
+    memcpy(bus->bustype, "PCI   ", sizeof(bus->bustype));
+    entrycount++;
+
     /* ioapic */
     u8 ioapic_id = CountCPUs;
     struct mpt_ioapic *ioapic = (void*)&bus[1];

@@ -101,9 +111,33 @@ mptable_init(void)
 
     /* irqs */
     struct mpt_intsrc *intsrcs = (void*)&ioapic[1], *intsrc = intsrcs;
+    int bdf, max;
+    unsigned short mask = 0;
+    foreachpci(bdf, max) {
+        int pin = pci_config_readb(bdf, PCI_INTERRUPT_PIN);
+        int irq = pci_config_readb(bdf, PCI_INTERRUPT_LINE);
+        if (pin == 0)
+        	continue;
+        mask |= (1 << irq);
+        memset(intsrc, 0, sizeof(*intsrc));
+        intsrc->type = MPT_TYPE_INTSRC;
+        intsrc->irqtype = 0; /* INT */
+        intsrc->irqflag = 1; /* active high */
+        intsrc->srcbus = 0; /* PCI bus */
+        intsrc->srcbusirq = (pci_bdf_to_dev(bdf) << 2) | (pin - 1);
+        intsrc->dstapic = ioapic_id;
+        intsrc->dstirq = irq;
+        intsrc++;
+    }
+
     for (i = 0; i < 16; i++) {
         memset(intsrc, 0, sizeof(*intsrc));
+        if (mask & (1 << i))
+        	continue;
         intsrc->type = MPT_TYPE_INTSRC;
+        intsrc->irqtype = 0; /* INT */
+        intsrc->irqflag = 0; /* conform to bus spec */
+        intsrc->srcbus = 1; /* ISA bus */
         intsrc->srcbusirq = i;
         intsrc->dstapic = ioapic_id;
         intsrc->dstirq = i;

@@ -123,7 +157,7 @@ mptable_init(void)
     intsrc->type = MPT_TYPE_LOCAL_INT;
     intsrc->irqtype = 3; /* ExtINT */
     intsrc->irqflag = 0; /* PO, EL default */
-    intsrc->srcbus = 0;
+    intsrc->srcbus = 1; /* ISA */
     intsrc->srcbusirq = 0;
     intsrc->dstapic = 0; /* BSP == APIC #0 */
     intsrc->dstirq = 0; /* LINTIN0 */

@@ -133,7 +167,7 @@ mptable_init(void)
     intsrc->type = MPT_TYPE_LOCAL_INT;
     intsrc->irqtype = 1; /* NMI */
     intsrc->irqflag = 0; /* PO, EL default */
-    intsrc->srcbus = 0;
+    intsrc->srcbus = 1; /* ISA */
     intsrc->srcbusirq = 0;
     intsrc->dstapic = 0; /* BSP == APIC #0 */
     intsrc->dstirq = 1; /* LINTIN1 */

--
Gleb.

#1 lore raw

Thanks Gleb.

Quoted 2 lines · Gleb Natapov
src/mptable.c +1 -1
-                  + sizeof(struct mpt_intsrc) * 18);
+                  + sizeof(struct mpt_intsrc) * 34);

[...]

Quoted 6 lines
src/mptable.c +6 -0
+    foreachpci(bdf, max) {
+        int pin = pci_config_readb(bdf, PCI_INTERRUPT_PIN);
+        int irq = pci_config_readb(bdf, PCI_INTERRUPT_LINE);
+        if (pin == 0)
+        	continue;
+        mask |= (1 << irq);

src/mptable.c mptable_init(void) · line 131

Quoted 10 lines
src/mptable.c +10 -0
+        memset(intsrc, 0, sizeof(*intsrc));
+        intsrc->type = MPT_TYPE_INTSRC;
+        intsrc->irqtype = 0; /* INT */
+        intsrc->irqflag = 1; /* active high */
+        intsrc->srcbus = 0; /* PCI bus */
+        intsrc->srcbusirq = (pci_bdf_to_dev(bdf) << 2) | (pin - 1);
+        intsrc->dstapic = ioapic_id;
+        intsrc->dstirq = irq;
+        intsrc++;
+    }

Why only increase the allocated storage for intsrc by 16? The loop above seems like it could add a large number of entries.

Also, the foreachpci() macro will iterate over every PCI function - there could be 8 functions to a pci device which could lead to duplicates in the table (two pci functions on the same device could use the same irq pin).

-Kevin

author
#2 lore raw
Quoted 23 lines · Kevin O'Connor

Thanks Gleb.

Quoted 2 lines · Gleb Natapov
src/mptable.c +1 -1
-                  + sizeof(struct mpt_intsrc) * 18);
+                  + sizeof(struct mpt_intsrc) * 34);

[...]

Quoted 6 lines
src/mptable.c +6 -0
+    foreachpci(bdf, max) {
+        int pin = pci_config_readb(bdf, PCI_INTERRUPT_PIN);
+        int irq = pci_config_readb(bdf, PCI_INTERRUPT_LINE);
+        if (pin == 0)
+        	continue;
+        mask |= (1 << irq);

src/mptable.c mptable_init(void) · line 131

Quoted 10 lines
src/mptable.c +10 -0
+        memset(intsrc, 0, sizeof(*intsrc));
+        intsrc->type = MPT_TYPE_INTSRC;
+        intsrc->irqtype = 0; /* INT */
+        intsrc->irqflag = 1; /* active high */
+        intsrc->srcbus = 0; /* PCI bus */
+        intsrc->srcbusirq = (pci_bdf_to_dev(bdf) << 2) | (pin - 1);
+        intsrc->dstapic = ioapic_id;
+        intsrc->dstirq = irq;
+        intsrc++;
+    }
Quoted 3 lines

Why only increase the allocated storage for intsrc by 16? The loop above seems like it could add a large number of entries.

Unfortunately we have to allocate all memory in advance. The table can have max 4 entries per device (one entry for each pin), so 16 is enough for 4 devices.

Quoted 5 lines

Also, the foreachpci() macro will iterate over every PCI function - there could be 8 functions to a pci device which could lead to duplicates in the table (two pci functions on the same device could use the same irq pin).

Yes, correct. Need to rework this loop.

--
Gleb.

#3 lore raw
Quoted 7 lines · Gleb Natapov
Quoted 3 lines · Kevin O'Connor

Why only increase the allocated storage for intsrc by 16? The loop above seems like it could add a large number of entries.

Unfortunately we have to allocate all memory in advance. The table can have max 4 entries per device (one entry for each pin), so 16 is enough for 4 devices.

Yeah - I suppose the table could be built in temp space and then copied into the f-segment when the actual size is known.

Quoted 6 lines
Quoted 5 lines

Also, the foreachpci() macro will iterate over every PCI function - there could be 8 functions to a pci device which could lead to duplicates in the table (two pci functions on the same device could use the same irq pin).

Yes, correct. Need to rework this loop.

Sounds like it should find every function 0 pci entry and then loop over the four possible pins?

-Kevin

author
#4 lore raw
Quoted 5 lines · Kevin O'Connor
Quoted 4 lines · Gleb Natapov
Quoted 3 lines · Kevin O'Connor

Why only increase the allocated storage for intsrc by 16? The loop above seems like it could add a large number of entries.

Quoted 7 lines
Quoted 3 lines

Unfortunately we have to allocate all memory in advance. The table can have max 4 entries per device (one entry for each pin), so 16 is enough for 4 devices.

Yeah - I suppose the table could be built in temp space and then copied into the f-segment when the actual size is known.

Yes, and this is not the only table that needs it. Lets assume 16 entries for now for simplicity.

Quoted 10 lines
Quoted 6 lines
Quoted 5 lines

Also, the foreachpci() macro will iterate over every PCI function - there could be 8 functions to a pci device which could lead to duplicates in the table (two pci functions on the same device could use the same irq pin).

Yes, correct. Need to rework this loop.

Sounds like it should find every function 0 pci entry and then loop over the four possible pins?

Each function connected only to one pin. It needs to loop over all function on a device and record pins that it creates entry for. If the same pin is used by more then one function of a device the record will be checked and second entry will not be created.

--
Gleb.

#5 lore raw
Quoted 10 lines · Gleb Natapov
Quoted 9 lines · Kevin O'Connor
Quoted 7 lines · Gleb Natapov
Quoted 3 lines · Kevin O'Connor

Why only increase the allocated storage for intsrc by 16? The loop above seems like it could add a large number of entries.

Unfortunately we have to allocate all memory in advance. The table can have max 4 entries per device (one entry for each pin), so 16 is enough for 4 devices.

Quoted 5 lines
Quoted 3 lines

Yeah - I suppose the table could be built in temp space and then copied into the f-segment when the actual size is known.

Yes, and this is not the only table that needs it. Lets assume 16 entries for now for simplicity.

The mptable is the only variable length table stored in the f-segment. I can add the table copying though.

BTW, this is only intended for PCI bus zero entries, right? (In theory, one could map a card with a bridge into kvm..)

Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
Quoted 5 lines

Also, the foreachpci() macro will iterate over every PCI function - there could be 8 functions to a pci device which could lead to duplicates in the table (two pci functions on the same device could use the same irq pin).

Yes, correct. Need to rework this loop.

Quoted 7 lines
Quoted 3 lines

Sounds like it should find every function 0 pci entry and then loop over the four possible pins?

Each function connected only to one pin. It needs to loop over all function on a device and record pins that it creates entry for. If the same pin is used by more then one function of a device the record will be checked and second entry will not be created.

I believe the intent of the irq routing in the mptable is to tell the OS what the pci pins are routed to so that it can change the irq to a different pin. If only one pin is described, the OS wont be able to switch to a different pin.

-Kevin

author
#6 lore raw
Quoted 11 lines · Kevin O'Connor
Quoted 10 lines · Gleb Natapov
Quoted 9 lines · Kevin O'Connor
Quoted 7 lines · Gleb Natapov
Quoted 3 lines · Kevin O'Connor

Why only increase the allocated storage for intsrc by 16? The loop above seems like it could add a large number of entries.

Unfortunately we have to allocate all memory in advance. The table can have max 4 entries per device (one entry for each pin), so 16 is enough for 4 devices.

Quoted 9 lines
Quoted 5 lines
Quoted 3 lines

Yeah - I suppose the table could be built in temp space and then copied into the f-segment when the actual size is known.

Yes, and this is not the only table that needs it. Lets assume 16 entries for now for simplicity.

The mptable is the only variable length table stored in the f-segment. I can add the table copying though.

Ah, OK.

Quoted 3 lines

BTW, this is only intended for PCI bus zero entries, right? (In theory, one could map a card with a bridge into kvm..)

Mptable should have description for each PCI bus. Devices behind PCI bridge are still on the same bus.

Quoted 15 lines
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
Quoted 5 lines

Also, the foreachpci() macro will iterate over every PCI function - there could be 8 functions to a pci device which could lead to duplicates in the table (two pci functions on the same device could use the same irq pin).

Yes, correct. Need to rework this loop.

Quoted 7 lines
Quoted 3 lines

Sounds like it should find every function 0 pci entry and then loop over the four possible pins?

Each function connected only to one pin. It needs to loop over all function on a device and record pins that it creates entry for. If the same pin is used by more then one function of a device the record will be checked and second entry will not be created.

Quoted 5 lines

I believe the intent of the irq routing in the mptable is to tell the OS what the pci pins are routed to so that it can change the irq to a different pin. If only one pin is described, the OS wont be able to switch to a different pin.

Each function hardwired to one pin only. It is impossible to change the pin. It is possible to route it to a different gsi though. Mptable maps pins of each device to gsis.

--
Gleb.

#7 lore raw
Quoted 6 lines · Gleb Natapov
Quoted 3 lines · Kevin O'Connor

BTW, this is only intended for PCI bus zero entries, right? (In theory, one could map a card with a bridge into kvm..)

Mptable should have description for each PCI bus. Devices behind PCI bridge are still on the same bus.

I believe mptable should describe each root pci bus. However, foreachpci() can iterate over both root pci buses and secondary buses
- so I think the code should check for (pci_bdf_to_bus(bdf) > 0) and break from the loop.

Though, admittedly, the code in pciinit.c doesn't handle secondary buses anyway.

Quoted 8 lines
Quoted 5 lines

I believe the intent of the irq routing in the mptable is to tell the OS what the pci pins are routed to so that it can change the irq to a different pin. If only one pin is described, the OS wont be able to switch to a different pin.

Each function hardwired to one pin only. It is impossible to change the pin. It is possible to route it to a different gsi though. Mptable maps pins of each device to gsis.

Oops - you are correct.

-Kevin