Changes v2 -> v3: - Report I/O APIC ID = 0 on MP-table, too
Changes v1 -> v2: - Patch 1/2: cosmetic whitespace change - Patch 2/2: use size suffixes on asm instructions on smp.c - New patch descriptions
Eduardo Habkost (2): report real I/O APIC ID (0) on MADT and MP-table (v3) allow CPUs to have non-contiguous Local APIC IDs (v2)
src/acpi-dsdt.dsl | 4 +++- src/acpi.c | 11 ++++++----- src/config.h | 1 + src/mptable.c | 4 ++-- src/smp.c | 17 +++++++++++++++++ src/util.h | 1 + 6 files changed, 30 insertions(+), 8 deletions(-)
When resetting an I/O APIC, its ID is set to 0, and SeaBIOS doesn't change it, so report it correctly on the ACPI MADT table and MP-table.
Some hardware may require the BIOS to initialize I/O APIC ID to an unique value, but SeaBIOS doesn't do that. This patch at least makes the tables reflect reality.
Changes v2 -> v3: - Fix MP-table too, not just ACPI MADT table
Changes v1 -> v2: - Cosmetic: whitespace change (removed extra newline) - New patch description
Signed-off-by: Eduardo Habkost ehabkost@redhat.com --- src/acpi.c | 2 +- src/config.h | 1 + src/mptable.c | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/acpi.c b/src/acpi.c index d39cbd9..da3bc57 100644 --- a/src/acpi.c +++ b/src/acpi.c @@ -336,7 +336,7 @@ build_madt(void) struct madt_io_apic *io_apic = (void*)apic; io_apic->type = APIC_IO; io_apic->length = sizeof(*io_apic); - io_apic->io_apic_id = CountCPUs; + io_apic->io_apic_id = BUILD_IOAPIC_ID; io_apic->address = cpu_to_le32(BUILD_IOAPIC_ADDR); io_apic->interrupt = cpu_to_le32(0);
diff --git a/src/config.h b/src/config.h index 3a70867..0d4066d 100644 --- a/src/config.h +++ b/src/config.h @@ -52,6 +52,7 @@ #define BUILD_PCIMEM64_END 0x10000000000ULL
#define BUILD_IOAPIC_ADDR 0xfec00000 +#define BUILD_IOAPIC_ID 0 #define BUILD_HPET_ADDRESS 0xfed00000 #define BUILD_APIC_ADDR 0xfee00000
diff --git a/src/mptable.c b/src/mptable.c index 103f462..74ed33d 100644 --- a/src/mptable.c +++ b/src/mptable.c @@ -92,7 +92,7 @@ mptable_init(void) entrycount += bus - buses;
/* ioapic */ - u8 ioapic_id = CountCPUs; + u8 ioapic_id = BUILD_IOAPIC_ID; struct mpt_ioapic *ioapic = (void*)bus; memset(ioapic, 0, sizeof(*ioapic)); ioapic->type = MPT_TYPE_IOAPIC;
Extract Local APIC IDs directly from the CPUs, and instead of check for "i < CountCPUs", check if the APIC ID was present on boot, when building ACPI tables and the MP-Table.
This keeps ACPI Processor ID == APIC ID, but allows the hardware<->SeaBIOS interface be completely APIC-ID based and not depend on any other kind of "CPU identifier". This way, SeaBIOS may change the way ACPI Processor IDs are chosen in the future.
As currently SeaBIOS supports only xAPIC and not x2APIC, the list of present-on-boot APIC IDs is a 256-bit bitmap. If one day SeaBIOS starts to support x2APIC, the data structure used to enumerate the APIC IDs will have to be changed (but this is an internal implementation detail, not visible to the OS or on any hardware<=>SeaBIOS interface).
For current QEMU versions (that always make the APIC IDs contiguous), the OS-visible behavior and resulting ACPI tables should be exactly the same. This patch will simply allow QEMU to start setting non-contiguous APIC IDs (that is a requirement for some sockets/cores/threads topology settings).
Changes v1 -> v2: - Use size suffixes on all asm instructions on smp.c - New patch description
Signed-off-by: Eduardo Habkost ehabkost@redhat.com --- src/acpi-dsdt.dsl | 4 +++- src/acpi.c | 9 +++++---- src/mptable.c | 2 +- src/smp.c | 17 +++++++++++++++++ src/util.h | 1 + 5 files changed, 27 insertions(+), 6 deletions(-)
diff --git a/src/acpi-dsdt.dsl b/src/acpi-dsdt.dsl index 2060686..72dc7d8 100644 --- a/src/acpi-dsdt.dsl +++ b/src/acpi-dsdt.dsl @@ -676,6 +676,7 @@ DefinitionBlock ( /* Methods called by run-time generated SSDT Processor objects */ Method (CPMA, 1, NotSerialized) { // _MAT method - create an madt apic buffer + // Arg0 = Processor ID = Local APIC ID // Local0 = CPON flag for this cpu Store(DerefOf(Index(CPON, Arg0)), Local0) // Local1 = Buffer (in madt apic form) to return @@ -688,6 +689,7 @@ DefinitionBlock ( } Method (CPST, 1, NotSerialized) { // _STA method - return ON status of cpu + // Arg0 = Processor ID = Local APIC ID // Local0 = CPON flag for this cpu Store(DerefOf(Index(CPON, Arg0)), Local0) If (Local0) { Return(0xF) } Else { Return(0x0) } @@ -708,7 +710,7 @@ DefinitionBlock ( Store (PRS, Local5) // Local2 = last read byte from bitmap Store (Zero, Local2) - // Local0 = cpuid iterator + // Local0 = Processor ID / APIC ID iterator Store (Zero, Local0) While (LLess(Local0, SizeOf(CPON))) { // Local1 = CPON flag for this cpu diff --git a/src/acpi.c b/src/acpi.c index da3bc57..39b7172 100644 --- a/src/acpi.c +++ b/src/acpi.c @@ -327,7 +327,7 @@ build_madt(void) apic->length = sizeof(*apic); apic->processor_id = i; apic->local_apic_id = i; - if (i < CountCPUs) + if (apic_id_is_present(apic->local_apic_id)) apic->flags = cpu_to_le32(1); else apic->flags = cpu_to_le32(0); @@ -445,6 +445,7 @@ build_ssdt(void) }
// build "Method(NTFY, 2) {If (LEqual(Arg0, 0x00)) {Notify(CP00, Arg1)} ...}" + // Arg0 = Processor ID = APIC ID *(ssdt_ptr++) = 0x14; // MethodOp ssdt_ptr = encodeLen(ssdt_ptr, 2+5+(12*acpi_cpus), 2); *(ssdt_ptr++) = 'N'; @@ -477,7 +478,7 @@ build_ssdt(void) ssdt_ptr = encodeLen(ssdt_ptr, 2+1+(1*acpi_cpus), 2); *(ssdt_ptr++) = acpi_cpus; for (i=0; i<acpi_cpus; i++) - *(ssdt_ptr++) = (i < CountCPUs) ? 0x01 : 0x00; + *(ssdt_ptr++) = (apic_id_is_present(i)) ? 0x01 : 0x00;
// store pci io windows: start, end, length // this way we don't have to do the math in the dsdt @@ -656,10 +657,10 @@ build_srat(void) core->proximity_lo = curnode; memset(core->proximity_hi, 0, 3); core->local_sapic_eid = 0; - if (i < CountCPUs) + if (apic_id_is_present(i)) core->flags = cpu_to_le32(1); else - core->flags = 0; + core->flags = cpu_to_le32(0); core++; }
diff --git a/src/mptable.c b/src/mptable.c index 74ed33d..3aa3427 100644 --- a/src/mptable.c +++ b/src/mptable.c @@ -59,7 +59,7 @@ mptable_init(void) cpu->apicid = i; cpu->apicver = apic_version; /* cpu flags: enabled, bootstrap cpu */ - cpu->cpuflag = ((i<CountCPUs) ? 0x01 : 0x00) | ((i==0) ? 0x02 : 0x00); + cpu->cpuflag = (apic_id_is_present(i) ? 0x01 : 0x00) | ((i==0) ? 0x02 : 0x00); cpu->cpusignature = cpuid_signature; cpu->featureflag = cpuid_features; cpu++; diff --git a/src/smp.c b/src/smp.c index 8c077a1..3c36f8c 100644 --- a/src/smp.c +++ b/src/smp.c @@ -36,6 +36,8 @@ wrmsr_smp(u32 index, u64 val)
u32 CountCPUs VAR16VISIBLE; u32 MaxCountCPUs VAR16VISIBLE; +// 256 bits for the found APIC IDs +u32 FoundAPICIDs[256/32] VAR16VISIBLE; extern void smp_ap_boot_code(void); ASM16( " .global smp_ap_boot_code\n" @@ -59,6 +61,12 @@ ASM16( " jmp 1b\n" "2:\n"
+ // get apic ID on EBX, set bit on FoundAPICIDs + " movl $1, %eax\n" + " cpuid\n" + " shrl $24, %ebx\n" + " lock btsl %ebx, FoundAPICIDs\n" + // Increment the cpu counter " lock incl CountCPUs\n"
@@ -67,6 +75,11 @@ ASM16( " jmp 1b\n" );
+int apic_id_is_present(u8 apic_id) +{ + return FoundAPICIDs[apic_id/32] & (1 << (apic_id % 32)); +} + // find and initialize the CPUs by launching a SIPI to them void smp_probe(void) @@ -82,6 +95,10 @@ smp_probe(void) return; }
+ // mark the BSP initial APIC ID as found, too: + u8 apic_id = ebx>>24; + FoundAPICIDs[apic_id/32] |= (1 << (apic_id % 32)); + // Init the counter. writel(&CountCPUs, 1);
diff --git a/src/util.h b/src/util.h index ef8ec7c..89e928c 100644 --- a/src/util.h +++ b/src/util.h @@ -355,6 +355,7 @@ extern u32 CountCPUs; extern u32 MaxCountCPUs; void wrmsr_smp(u32 index, u64 val); void smp_probe(void); +int apic_id_is_present(u8 apic_id);
// coreboot.c extern const char *CBvendor, *CBpart;
On 07/25/12 20:45, Eduardo Habkost wrote:
Extract Local APIC IDs directly from the CPUs, and instead of check for "i < CountCPUs", check if the APIC ID was present on boot, when building ACPI tables and the MP-Table.
This keeps ACPI Processor ID == APIC ID, but allows the hardware<->SeaBIOS interface be completely APIC-ID based and not depend on any other kind of "CPU identifier". This way, SeaBIOS may change the way ACPI Processor IDs are chosen in the future.
As currently SeaBIOS supports only xAPIC and not x2APIC, the list of present-on-boot APIC IDs is a 256-bit bitmap. If one day SeaBIOS starts to support x2APIC, the data structure used to enumerate the APIC IDs will have to be changed (but this is an internal implementation detail, not visible to the OS or on any hardware<=>SeaBIOS interface).
For current QEMU versions (that always make the APIC IDs contiguous), the OS-visible behavior and resulting ACPI tables should be exactly the same. This patch will simply allow QEMU to start setting non-contiguous APIC IDs (that is a requirement for some sockets/cores/threads topology settings).
Changes v1 -> v2:
- Use size suffixes on all asm instructions on smp.c
- New patch description
Signed-off-by: Eduardo Habkost ehabkost@redhat.com
src/acpi-dsdt.dsl | 4 +++- src/acpi.c | 9 +++++---- src/mptable.c | 2 +- src/smp.c | 17 +++++++++++++++++ src/util.h | 1 + 5 files changed, 27 insertions(+), 6 deletions(-)
diff --git a/src/acpi-dsdt.dsl b/src/acpi-dsdt.dsl index 2060686..72dc7d8 100644 --- a/src/acpi-dsdt.dsl +++ b/src/acpi-dsdt.dsl @@ -676,6 +676,7 @@ DefinitionBlock ( /* Methods called by run-time generated SSDT Processor objects */ Method (CPMA, 1, NotSerialized) { // _MAT method - create an madt apic buffer
// Arg0 = Processor ID = Local APIC ID // Local0 = CPON flag for this cpu Store(DerefOf(Index(CPON, Arg0)), Local0) // Local1 = Buffer (in madt apic form) to return@@ -688,6 +689,7 @@ DefinitionBlock ( } Method (CPST, 1, NotSerialized) { // _STA method - return ON status of cpu
// Arg0 = Processor ID = Local APIC ID // Local0 = CPON flag for this cpu Store(DerefOf(Index(CPON, Arg0)), Local0) If (Local0) { Return(0xF) } Else { Return(0x0) }@@ -708,7 +710,7 @@ DefinitionBlock ( Store (PRS, Local5) // Local2 = last read byte from bitmap Store (Zero, Local2)
// Local0 = cpuid iterator
// Local0 = Processor ID / APIC ID iterator Store (Zero, Local0) While (LLess(Local0, SizeOf(CPON))) { // Local1 = CPON flag for this cpudiff --git a/src/acpi.c b/src/acpi.c index da3bc57..39b7172 100644 --- a/src/acpi.c +++ b/src/acpi.c @@ -327,7 +327,7 @@ build_madt(void) apic->length = sizeof(*apic); apic->processor_id = i; apic->local_apic_id = i;
if (i < CountCPUs)
if (apic_id_is_present(apic->local_apic_id)) apic->flags = cpu_to_le32(1); else apic->flags = cpu_to_le32(0);@@ -445,6 +445,7 @@ build_ssdt(void) }
// build "Method(NTFY, 2) {If (LEqual(Arg0, 0x00)) {Notify(CP00, Arg1)} ...}"
- // Arg0 = Processor ID = APIC ID *(ssdt_ptr++) = 0x14; // MethodOp ssdt_ptr = encodeLen(ssdt_ptr, 2+5+(12*acpi_cpus), 2); *(ssdt_ptr++) = 'N';
@@ -477,7 +478,7 @@ build_ssdt(void) ssdt_ptr = encodeLen(ssdt_ptr, 2+1+(1*acpi_cpus), 2); *(ssdt_ptr++) = acpi_cpus; for (i=0; i<acpi_cpus; i++)
*(ssdt_ptr++) = (i < CountCPUs) ? 0x01 : 0x00;
*(ssdt_ptr++) = (apic_id_is_present(i)) ? 0x01 : 0x00;// store pci io windows: start, end, length // this way we don't have to do the math in the dsdt
@@ -656,10 +657,10 @@ build_srat(void) core->proximity_lo = curnode; memset(core->proximity_hi, 0, 3); core->local_sapic_eid = 0;
if (i < CountCPUs)
if (apic_id_is_present(i)) core->flags = cpu_to_le32(1); else
core->flags = 0;
core->flags = cpu_to_le32(0); core++;diff --git a/src/mptable.c b/src/mptable.c index 74ed33d..3aa3427 100644 --- a/src/mptable.c +++ b/src/mptable.c @@ -59,7 +59,7 @@ mptable_init(void) cpu->apicid = i; cpu->apicver = apic_version; /* cpu flags: enabled, bootstrap cpu */
cpu->cpuflag = ((i<CountCPUs) ? 0x01 : 0x00) | ((i==0) ? 0x02 : 0x00);
cpu->cpuflag = (apic_id_is_present(i) ? 0x01 : 0x00) | ((i==0) ? 0x02 : 0x00); cpu->cpusignature = cpuid_signature; cpu->featureflag = cpuid_features; cpu++;diff --git a/src/smp.c b/src/smp.c index 8c077a1..3c36f8c 100644 --- a/src/smp.c +++ b/src/smp.c @@ -36,6 +36,8 @@ wrmsr_smp(u32 index, u64 val)
u32 CountCPUs VAR16VISIBLE; u32 MaxCountCPUs VAR16VISIBLE; +// 256 bits for the found APIC IDs +u32 FoundAPICIDs[256/32] VAR16VISIBLE; extern void smp_ap_boot_code(void); ASM16( " .global smp_ap_boot_code\n" @@ -59,6 +61,12 @@ ASM16( " jmp 1b\n" "2:\n"
- // get apic ID on EBX, set bit on FoundAPICIDs
- " movl $1, %eax\n"
- " cpuid\n"
- " shrl $24, %ebx\n"
- " lock btsl %ebx, FoundAPICIDs\n"
- // Increment the cpu counter " lock incl CountCPUs\n"
@@ -67,6 +75,11 @@ ASM16( " jmp 1b\n" );
+int apic_id_is_present(u8 apic_id) +{
- return FoundAPICIDs[apic_id/32] & (1 << (apic_id % 32));
+}
// find and initialize the CPUs by launching a SIPI to them void smp_probe(void) @@ -82,6 +95,10 @@ smp_probe(void) return; }
- // mark the BSP initial APIC ID as found, too:
- u8 apic_id = ebx>>24;
- FoundAPICIDs[apic_id/32] |= (1 << (apic_id % 32));
- // Init the counter. writel(&CountCPUs, 1);
diff --git a/src/util.h b/src/util.h index ef8ec7c..89e928c 100644 --- a/src/util.h +++ b/src/util.h @@ -355,6 +355,7 @@ extern u32 CountCPUs; extern u32 MaxCountCPUs; void wrmsr_smp(u32 index, u64 val); void smp_probe(void); +int apic_id_is_present(u8 apic_id);
// coreboot.c extern const char *CBvendor, *CBpart;
The series looks to me; I sought to verify the ACPI changes against the 5.0 spec and the new assembly against the Intel documentation.
I'd like to suggest changing each constant 1 that is the left operand of a bit shift to 1u -- technically 1 << 31 is undefined behavior with ints having 31 value bits --, plus either changing the return type of apic_id_is_present() to u32, or subjecting its returned expression to !!.
Thanks, Laszlo
On Wed, Jul 25, 2012 at 03:45:28PM -0300, Eduardo Habkost wrote:
Changes v2 -> v3:
- Report I/O APIC ID = 0 on MP-table, too
Changes v1 -> v2:
- Patch 1/2: cosmetic whitespace change
- Patch 2/2: use size suffixes on asm instructions on smp.c
- New patch descriptions
Eduardo Habkost (2): report real I/O APIC ID (0) on MADT and MP-table (v3) allow CPUs to have non-contiguous Local APIC IDs (v2)
The series looks okay to me. However, I think Gleb and Avi had some comments - has that been resolved now?
-Kevin
On Tue, Jul 31, 2012 at 09:14:13PM -0400, Kevin O'Connor wrote:
On Wed, Jul 25, 2012 at 03:45:28PM -0300, Eduardo Habkost wrote:
Changes v2 -> v3:
- Report I/O APIC ID = 0 on MP-table, too
Changes v1 -> v2:
- Patch 1/2: cosmetic whitespace change
- Patch 2/2: use size suffixes on asm instructions on smp.c
- New patch descriptions
Eduardo Habkost (2): report real I/O APIC ID (0) on MADT and MP-table (v3) allow CPUs to have non-contiguous Local APIC IDs (v2)
The series looks okay to me. However, I think Gleb and Avi had some comments - has that been resolved now?
No, but the comment is very minor and can be resolved by follow up patch if ever.
-- Gleb.
On Wed, Aug 01, 2012 at 10:47:35AM +0300, Gleb Natapov wrote:
On Tue, Jul 31, 2012 at 09:14:13PM -0400, Kevin O'Connor wrote:
On Wed, Jul 25, 2012 at 03:45:28PM -0300, Eduardo Habkost wrote:
Changes v2 -> v3:
- Report I/O APIC ID = 0 on MP-table, too
Changes v1 -> v2:
- Patch 1/2: cosmetic whitespace change
- Patch 2/2: use size suffixes on asm instructions on smp.c
- New patch descriptions
Eduardo Habkost (2): report real I/O APIC ID (0) on MADT and MP-table (v3) allow CPUs to have non-contiguous Local APIC IDs (v2)
The series looks okay to me. However, I think Gleb and Avi had some comments - has that been resolved now?
No, but the comment is very minor and can be resolved by follow up patch if ever.
Okay - thanks. I pushed this series.
-Kevin
-
Eduardo Habkost -
Gleb Natapov -
Kevin O'Connor -
Laszlo Ersek