[PATCH] provide correct pci routing information in mptable

[PATCH] provide correct pci routing information in mptable

[Gleb Natapov]

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 <gleb@redhat.com>
diff --git a/src/mptable.c b/src/mptable.c
index 1920dfe..545a41b 100644
--- a/src/mptable.c
+++ b/src/mptable.c
@@ -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.

Re: [PATCH] provide correct pci routing information in mptable

[Kevin O'Connor]

Thanks Gleb.

On Wed, Dec 23, 2009 at 05:29:25PM +0200, Gleb Natapov wrote:
> -                  + sizeof(struct mpt_intsrc) * 18);
> +                  + sizeof(struct mpt_intsrc) * 34);
[...]
> +    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++;
> +    }

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

Re: [PATCH] provide correct pci routing information in mptable

[Gleb Natapov]

On Wed, Dec 23, 2009 at 01:11:43PM -0500, Kevin O'Connor wrote:
> Thanks Gleb.
> 
> On Wed, Dec 23, 2009 at 05:29:25PM +0200, Gleb Natapov wrote:
> > -                  + sizeof(struct mpt_intsrc) * 18);
> > +                  + sizeof(struct mpt_intsrc) * 34);
> [...]
> > +    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++;
> > +    }
> 
> 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.

> 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.

Re: [PATCH] provide correct pci routing information in mptable

[Kevin O'Connor]

On Wed, Dec 23, 2009 at 08:45:07PM +0200, Gleb Natapov wrote:
> On Wed, Dec 23, 2009 at 01:11:43PM -0500, Kevin O'Connor wrote:
> > 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.

> > 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

Re: [PATCH] provide correct pci routing information in mptable

[Gleb Natapov]

On Wed, Dec 23, 2009 at 02:48:24PM -0500, Kevin O'Connor wrote:
> On Wed, Dec 23, 2009 at 08:45:07PM +0200, Gleb Natapov wrote:
> > On Wed, Dec 23, 2009 at 01:11:43PM -0500, Kevin O'Connor wrote:
> > > 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.
> 
Yes, and this is not the only table that needs it. Lets assume 16
entries for now  for simplicity.

> > > 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.

Re: [PATCH] provide correct pci routing information in mptable

[Kevin O'Connor]

On Wed, Dec 23, 2009 at 09:56:23PM +0200, Gleb Natapov wrote:
> On Wed, Dec 23, 2009 at 02:48:24PM -0500, Kevin O'Connor wrote:
> > On Wed, Dec 23, 2009 at 08:45:07PM +0200, Gleb Natapov wrote:
> > > On Wed, Dec 23, 2009 at 01:11:43PM -0500, Kevin O'Connor wrote:
> > > > 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.
> > 
> 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..)

> > > > 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.

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

Re: [PATCH] provide correct pci routing information in mptable

[Gleb Natapov]

On Wed, Dec 23, 2009 at 03:26:55PM -0500, Kevin O'Connor wrote:
> On Wed, Dec 23, 2009 at 09:56:23PM +0200, Gleb Natapov wrote:
> > On Wed, Dec 23, 2009 at 02:48:24PM -0500, Kevin O'Connor wrote:
> > > On Wed, Dec 23, 2009 at 08:45:07PM +0200, Gleb Natapov wrote:
> > > > On Wed, Dec 23, 2009 at 01:11:43PM -0500, Kevin O'Connor wrote:
> > > > > 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.
> > > 
> > 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.

> 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.

> > > > > 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.
> 
> 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.

Re: [PATCH] provide correct pci routing information in mptable

[Kevin O'Connor]

On Wed, Dec 23, 2009 at 10:35:55PM +0200, Gleb Natapov wrote:
> On Wed, Dec 23, 2009 at 03:26:55PM -0500, Kevin O'Connor wrote:
> > 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.

> > 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