This patch causes the mptable to be built in temporary memory and then copied into the f-segment after it is complete. This makes better use of the limited space in the f-segment, and it makes it less likely an overflow will occur (eg, due to many PCI irq entries).
-Kevin
diff --git a/src/mptable.c b/src/mptable.c index 2409fcf..20c3d63 100644 --- a/src/mptable.c +++ b/src/mptable.c @@ -20,30 +20,12 @@ mptable_init(void)
dprintf(3, "init MPTable\n");
- // Allocate memory - int length = (sizeof(struct mptable_config_s) - + sizeof(struct mpt_cpu) * MaxCountCPUs - + sizeof(struct mpt_bus) * 2 - + sizeof(struct mpt_ioapic) - + 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) { - dprintf(1, "No room for MPTABLE!\n"); - free(config); - free(floating); + // Config structure in temp area. + struct mptable_config_s *config = malloc_tmphigh(32*1024); + if (!config) { + dprintf(1, "No space for temp mptable\n"); return; } - - /* floating pointer structure */ - memset(floating, 0, sizeof(*floating)); - floating->signature = MPTABLE_SIGNATURE; - floating->physaddr = (u32)config; - floating->length = 1; - floating->spec_rev = 4; - floating->checksum -= checksum(floating, sizeof(*floating)); - - // Config structure. memset(config, 0, sizeof(*config)); config->signature = MPCONFIG_SIGNATURE; config->spec = 4; @@ -115,6 +97,8 @@ mptable_init(void) unsigned short mask = 0, pinmask;
foreachpci(bdf, max) { + if (pci_bdf_to_bus(bdf) > 0) + break; int pin = pci_config_readb(bdf, PCI_INTERRUPT_PIN); int irq = pci_config_readb(bdf, PCI_INTERRUPT_LINE); if (pin == 0) @@ -183,10 +167,32 @@ mptable_init(void) entrycount++;
// Finalize config structure. + int length = (void*)intsrc - (void*)config; config->entrycount = entrycount; - config->length = (void*)intsrc - (void*)config; - config->checksum -= checksum(config, config->length); + config->length = length; + config->checksum -= checksum(config, length); + + // Allocate final memory locations + struct mptable_config_s *finalconfig = malloc_fseg(length); + struct mptable_floating_s *floating = malloc_fseg(sizeof(*floating)); + if (!finalconfig || !floating) { + dprintf(1, "No room for MPTABLE!\n"); + free(config); + free(finalconfig); + free(floating); + return; + } + memcpy(finalconfig, config, length); + free(config); + + /* floating pointer structure */ + memset(floating, 0, sizeof(*floating)); + floating->signature = MPTABLE_SIGNATURE; + floating->physaddr = (u32)finalconfig; + floating->length = 1; + floating->spec_rev = 4; + floating->checksum -= checksum(floating, sizeof(*floating));
dprintf(1, "MP table addr=%p MPC table addr=%p size=%d\n", - floating, config, config->length); + floating, finalconfig, length); }
On Thu, Dec 24, 2009 at 11:38:45AM -0500, Kevin O'Connor wrote:
This patch causes the mptable to be built in temporary memory and then copied into the f-segment after it is complete. This makes better use of the limited space in the f-segment, and it makes it less likely an overflow will occur (eg, due to many PCI irq entries).
-Kevin
diff --git a/src/mptable.c b/src/mptable.c index 2409fcf..20c3d63 100644 --- a/src/mptable.c +++ b/src/mptable.c @@ -20,30 +20,12 @@ mptable_init(void)
dprintf(3, "init MPTable\n");
- // Allocate memory
- int length = (sizeof(struct mptable_config_s)
+ sizeof(struct mpt_cpu) * MaxCountCPUs+ sizeof(struct mpt_bus) * 2+ sizeof(struct mpt_ioapic)+ 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) {
dprintf(1, "No room for MPTABLE!\n");free(config);free(floating);
- // Config structure in temp area.
- struct mptable_config_s *config = malloc_tmphigh(32*1024);
- if (!config) {
dprintf(1, "No space for temp mptable\n"); return;
- /* floating pointer structure */
- memset(floating, 0, sizeof(*floating));
- floating->signature = MPTABLE_SIGNATURE;
- floating->physaddr = (u32)config;
- floating->length = 1;
- floating->spec_rev = 4;
- floating->checksum -= checksum(floating, sizeof(*floating));
- // Config structure. memset(config, 0, sizeof(*config)); config->signature = MPCONFIG_SIGNATURE; config->spec = 4;
@@ -115,6 +97,8 @@ mptable_init(void) unsigned short mask = 0, pinmask;
foreachpci(bdf, max) {
if (pci_bdf_to_bus(bdf) > 0)break;
mptable routing info should be created for each pci bus. See example here: http://people.freebsd.org/~fsmp/SMP/mptable/t30
Hmm. It looks like we need to create io_bus entries dynamically for each newly discovered PCI bus.
-- Gleb.
On Thu, Dec 24, 2009 at 06:54:55PM +0200, Gleb Natapov wrote:
On Thu, Dec 24, 2009 at 11:38:45AM -0500, Kevin O'Connor wrote:
foreachpci(bdf, max) {
if (pci_bdf_to_bus(bdf) > 0)break;mptable routing info should be created for each pci bus.
My read of the spec is that independent PCI buses need to be added to the MPTable, but a bus created due to a PCI-to-PCI bridge (eg, on an add-on card) need not go into the table.
See example here: http://people.freebsd.org/~fsmp/SMP/mptable/t30
In that example, it looks like the machine has two independent buses - note how bus 1 has more than four irqs - so it can't be an add-on card.
Hmm. It looks like we need to create io_bus entries dynamically for each newly discovered PCI bus.
Right now, the pciinit.c code handles neither PCI-to-PCI bridges nor independent PCI bridges. I think it would be useful to support bridges (for mapping in add-on cards that have them), but I don't know why an emulator would want to have independent buses. Note that it's difficult to detect an independent bus - some sideband mechanism is needed (eg, see CONFIG_PCI_ROOT1 in src/config.h).
-Kevin
On Thu, Dec 24, 2009 at 12:22:12PM -0500, Kevin O'Connor wrote:
On Thu, Dec 24, 2009 at 06:54:55PM +0200, Gleb Natapov wrote:
On Thu, Dec 24, 2009 at 11:38:45AM -0500, Kevin O'Connor wrote:
foreachpci(bdf, max) {
if (pci_bdf_to_bus(bdf) > 0)break;mptable routing info should be created for each pci bus.
My read of the spec is that independent PCI buses need to be added to the MPTable, but a bus created due to a PCI-to-PCI bridge (eg, on an add-on card) need not go into the table.
See example here:
Ah so pci_bdf_to_bus(bdf) > 0 for the bus behind PCI-to-PCI bridge? I don't wee why you read spec this way. Can you point me to the part of the spec that makes you think so?
In that example, it looks like the machine has two independent buses - note how bus 1 has more than four irqs - so it can't be an add-on card.
Yes. In this example there are several PCI domains.
Hmm. It looks like we need to create io_bus entries dynamically for each newly discovered PCI bus.
Right now, the pciinit.c code handles neither PCI-to-PCI bridges nor independent PCI bridges. I think it would be useful to support bridges (for mapping in add-on cards that have them), but I don't know why an emulator would want to have independent buses. Note that it's difficult to detect an independent bus - some sideband mechanism is needed (eg, see CONFIG_PCI_ROOT1 in src/config.h).
-- Gleb.
On Thu, Dec 24, 2009 at 08:16:19PM +0200, Gleb Natapov wrote:
Ah so pci_bdf_to_bus(bdf) > 0 for the bus behind PCI-to-PCI bridge?
Yes - if it is configured properly (which pciinit.c doesn't).
I don't wee why you read spec this way. Can you point me to the part of the spec that makes you think so?
The purpose of the table is to relay to the OS info the OS can't find on its own - a pci-to-pci bridge on an add-on card is fully auto-detectable. The mpspec doesn't seem to require bridges of add-on cards be listed - from the spec:
Each bus in a system must have a unique BUS ID if any one of the following criteria are true: 1. The bus does not share its memory address space with another bus. 2. The bus does not share its I/O address space with another bus. 3. The bus does not share interrupt lines with another bus. 4. Any aspect of the bus as an independent entity is software visible (such as PCI configuration space).
The first three clearly don't apply. The fourth seems ambiguous to me.
That said, I don't see any reason why they couldn't be listed. However, the code right now would produce incorrect results for bus > 0.
BTW, section D.2 of the mpspec indicates that buses must be listed in ascending order - so the code should probably define the PCI bus first.
In any case, I'll separate out the memory part of the patch - we can debate the merits of pci buses separately.
-Kevin
On Thu, Dec 24, 2009 at 02:41:48PM -0500, Kevin O'Connor wrote:
On Thu, Dec 24, 2009 at 08:16:19PM +0200, Gleb Natapov wrote:
Ah so pci_bdf_to_bus(bdf) > 0 for the bus behind PCI-to-PCI bridge?
Yes - if it is configured properly (which pciinit.c doesn't).
I don't wee why you read spec this way. Can you point me to the part of the spec that makes you think so?
The purpose of the table is to relay to the OS info the OS can't find on its own - a pci-to-pci bridge on an add-on card is fully auto-detectable. The mpspec doesn't seem to require bridges of add-on cards be listed - from the spec:
Each bus in a system must have a unique BUS ID if any one of the following criteria are true:
- The bus does not share its memory address space with another bus.
- The bus does not share its I/O address space with another bus.
- The bus does not share interrupt lines with another bus.
- Any aspect of the bus as an independent entity is software visible (such as PCI configuration space).
The first three clearly don't apply. The fourth seems ambiguous to me.
To me it looks like the fourth was included specifically to cover PCI buses.
That said, I don't see any reason why they couldn't be listed. However, the code right now would produce incorrect results for bus > 0.
Unfortunately yes. We need to have one more loop to find all PCI buses and create bus entry for each. May be something like patch below?
diff --git a/src/mptable.c b/src/mptable.c index 2409fcf..54fef36 100644 --- a/src/mptable.c +++ b/src/mptable.c @@ -23,7 +23,7 @@ mptable_init(void) // Allocate memory int length = (sizeof(struct mptable_config_s) + sizeof(struct mpt_cpu) * MaxCountCPUs - + sizeof(struct mpt_bus) * 2 + + sizeof(struct mpt_bus) * 5 + sizeof(struct mpt_ioapic) + sizeof(struct mpt_intsrc) * 34); struct mptable_config_s *config = malloc_fseg(length); @@ -83,19 +83,28 @@ mptable_init(void) } int entrycount = cpu - cpus;
- /* isa bus */ 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++; + unsigned long busmask = 0; + int bdf, max; + foreachpci(bdf, max) { + int curbus = pci_bdf_to_bus(bdf); + if (busmask & (1 << curbus)) + continue; + busmask |= (1 << curbus); + memset(bus, 0, sizeof(*bus)); + bus->type = MPT_TYPE_BUS; + bus->busid = curbus; + memcpy(bus->bustype, "PCI ", sizeof(bus->bustype)); + bus++; + entrycount++; + }
- bus++; + /* isa bus */ + int isabusid; memset(bus, 0, sizeof(*bus)); bus->type = MPT_TYPE_BUS; - bus->busid = 0; - memcpy(bus->bustype, "PCI ", sizeof(bus->bustype)); + isabusid = bus->busid = __fls(busmask) + 1; + memcpy(bus->bustype, "ISA ", sizeof(bus->bustype)); entrycount++;
/* ioapic */ @@ -111,7 +120,7 @@ mptable_init(void)
/* irqs */ struct mpt_intsrc *intsrcs = (void*)&ioapic[1], *intsrc = intsrcs; - int bdf, max, dev = -1; + int dev = -1; unsigned short mask = 0, pinmask;
foreachpci(bdf, max) { @@ -131,7 +140,7 @@ mptable_init(void) intsrc->type = MPT_TYPE_INTSRC; intsrc->irqtype = 0; /* INT */ intsrc->irqflag = 1; /* active high */ - intsrc->srcbus = 0; /* PCI bus */ + intsrc->srcbus = pci_bdf_to_bus(bdf); /* PCI bus */ intsrc->srcbusirq = (dev << 2) | (pin - 1); intsrc->dstapic = ioapic_id; intsrc->dstirq = irq; @@ -145,7 +154,7 @@ mptable_init(void) intsrc->type = MPT_TYPE_INTSRC; intsrc->irqtype = 0; /* INT */ intsrc->irqflag = 0; /* conform to bus spec */ - intsrc->srcbus = 1; /* ISA bus */ + intsrc->srcbus = isabusid; /* ISA bus */ intsrc->srcbusirq = i; intsrc->dstapic = ioapic_id; intsrc->dstirq = i; @@ -165,7 +174,7 @@ mptable_init(void) intsrc->type = MPT_TYPE_LOCAL_INT; intsrc->irqtype = 3; /* ExtINT */ intsrc->irqflag = 0; /* PO, EL default */ - intsrc->srcbus = 1; /* ISA */ + intsrc->srcbus = isabusid; /* ISA */ intsrc->srcbusirq = 0; intsrc->dstapic = 0; /* BSP == APIC #0 */ intsrc->dstirq = 0; /* LINTIN0 */ @@ -175,7 +184,7 @@ mptable_init(void) intsrc->type = MPT_TYPE_LOCAL_INT; intsrc->irqtype = 1; /* NMI */ intsrc->irqflag = 0; /* PO, EL default */ - intsrc->srcbus = 1; /* ISA */ + intsrc->srcbus = isabusid; /* ISA */ intsrc->srcbusirq = 0; intsrc->dstapic = 0; /* BSP == APIC #0 */ intsrc->dstirq = 1; /* LINTIN1 */ -- Gleb.
On Sun, Dec 27, 2009 at 01:43:14PM +0200, Gleb Natapov wrote:
On Thu, Dec 24, 2009 at 02:41:48PM -0500, Kevin O'Connor wrote:
That said, I don't see any reason why they couldn't be listed. However, the code right now would produce incorrect results for bus > 0.
Unfortunately yes. We need to have one more loop to find all PCI buses and create bus entry for each. May be something like patch below?
Okay. It would help if you can base the patch on top of my memory allocation mptable patch.
- unsigned long busmask = 0;
- int bdf, max;
- foreachpci(bdf, max) {
int curbus = pci_bdf_to_bus(bdf);- if (busmask & (1 << curbus))
continue;- busmask |= (1 << curbus);
foreachpci will iterate in bus order - it's better to use a "lastbus" variable than busmask as it's possible to have a PCI bus greater than 32.
-Kevin
On Sun, Dec 27, 2009 at 10:36:54AM -0500, Kevin O'Connor wrote:
On Sun, Dec 27, 2009 at 01:43:14PM +0200, Gleb Natapov wrote:
On Thu, Dec 24, 2009 at 02:41:48PM -0500, Kevin O'Connor wrote:
That said, I don't see any reason why they couldn't be listed. However, the code right now would produce incorrect results for bus > 0.
Unfortunately yes. We need to have one more loop to find all PCI buses and create bus entry for each. May be something like patch below?
Okay. It would help if you can base the patch on top of my memory allocation mptable patch.
No problem. I'll rebase when you'll apply it.
- unsigned long busmask = 0;
- int bdf, max;
- foreachpci(bdf, max) {
int curbus = pci_bdf_to_bus(bdf);- if (busmask & (1 << curbus))
continue;- busmask |= (1 << curbus);
foreachpci will iterate in bus order - it's better to use a "lastbus" variable than busmask as it's possible to have a PCI bus greater than 32.
OK.
-- Gleb.
-
Gleb Natapov -
Kevin O'Connor