| Message ID | 1342731162-14285-2-git-send-email-ehabkost@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Jul 19, 2012 at 05:52:41PM -0300, Eduardo Habkost wrote: > When resetting an I/O APIC, its ID is set to 0, so set it to 0 on the > MADT table too. > Actually BIOS needs to configure ioapic id to a uniqe value. This does not really matter for KVM though. > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > src/acpi.c | 2 +- > src/config.h | 2 ++ > 2 files changed, 3 insertions(+), 1 deletion(-) > > diff --git a/src/acpi.c b/src/acpi.c > index 55e4607..3f55de9 100644 > --- a/src/acpi.c > +++ b/src/acpi.c > @@ -335,7 +335,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..878c691 100644 > --- a/src/config.h > +++ b/src/config.h > @@ -52,9 +52,11 @@ > #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 > > + > // Important real-mode segments > #define SEG_IVT 0x0000 > #define SEG_BDA 0x0040 > -- > 1.7.10.4 -- Gleb.
On Fri, Jul 20, 2012 at 12:18:59AM +0300, Gleb Natapov wrote: > On Thu, Jul 19, 2012 at 05:52:41PM -0300, Eduardo Habkost wrote: > > When resetting an I/O APIC, its ID is set to 0, so set it to 0 on the > > MADT table too. > > > Actually BIOS needs to configure ioapic id to a uniqe value. This does > not really matter for KVM though. Where does this requirement comes from? I am guessing it matters only when the I/O APIC is directly connected to the APIC bus (according to Intel SDM, that's the case only for old Pentium and P6 CPUs)[1]. Anyway, even if some hardware has this unique-ID requirement, today Seabios does not fulfill it, leaving the I/O APIC ID as 0. The patch at least makes the MADT table match reality. [1] I have checked 3 different machines, and all machines I have looked have an I/O APIC ID that conflicts with an existing Local APIC ID, on the ACPI MADT table. Some iasl dumps may be found online by googling for: "Subtable Type : 01" "I/O Apic ID" I looked at 5 or 6 matches, and almost every one have an I/O APIC ID conflicting with a Local APIC ID. > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > --- > > src/acpi.c | 2 +- > > src/config.h | 2 ++ > > 2 files changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/src/acpi.c b/src/acpi.c > > index 55e4607..3f55de9 100644 > > --- a/src/acpi.c > > +++ b/src/acpi.c > > @@ -335,7 +335,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..878c691 100644 > > --- a/src/config.h > > +++ b/src/config.h > > @@ -52,9 +52,11 @@ > > #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 > > > > + > > // Important real-mode segments > > #define SEG_IVT 0x0000 > > #define SEG_BDA 0x0040 > > -- > > 1.7.10.4 > > -- > Gleb. >
On Fri, Jul 20, 2012 at 01:22:43PM -0300, Eduardo Habkost wrote: > On Fri, Jul 20, 2012 at 12:18:59AM +0300, Gleb Natapov wrote: > > On Thu, Jul 19, 2012 at 05:52:41PM -0300, Eduardo Habkost wrote: > > > When resetting an I/O APIC, its ID is set to 0, so set it to 0 on the > > > MADT table too. > > > > > Actually BIOS needs to configure ioapic id to a uniqe value. This does > > not really matter for KVM though. > > Where does this requirement comes from? I am guessing it matters only > when the I/O APIC is directly connected to the APIC bus (according to > Intel SDM, that's the case only for old Pentium and P6 CPUs)[1]. > http://www.intel.com/design/chipsets/datashts/29056601.pdf says that it should be programmed to unique value. I checked 4 machines on 3 of them this was the case on one (AMD) there are 3 IOAPICs and they all overlap with APICs. > Anyway, even if some hardware has this unique-ID requirement, today > Seabios does not fulfill it, leaving the I/O APIC ID as 0. The patch at > least makes the MADT table match reality. > The currant code was written when we could only dream supporting 16 cpus and back than it did correct thing, but now it just broken. QEMU do not care about IOAPIC ID for sure so I am OK with the patch. > > [1] I have checked 3 different machines, and all machines I have looked > have an I/O APIC ID that conflicts with an existing Local APIC ID, on > the ACPI MADT table. > > Some iasl dumps may be found online by googling for: > "Subtable Type : 01" "I/O Apic ID" > > I looked at 5 or 6 matches, and almost every one have an I/O APIC ID > conflicting with a Local APIC ID. 1 from 4 for me :) > > > > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > > --- > > > src/acpi.c | 2 +- > > > src/config.h | 2 ++ > > > 2 files changed, 3 insertions(+), 1 deletion(-) > > > > > > diff --git a/src/acpi.c b/src/acpi.c > > > index 55e4607..3f55de9 100644 > > > --- a/src/acpi.c > > > +++ b/src/acpi.c > > > @@ -335,7 +335,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..878c691 100644 > > > --- a/src/config.h > > > +++ b/src/config.h > > > @@ -52,9 +52,11 @@ > > > #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 > > > > > > + > > > // Important real-mode segments > > > #define SEG_IVT 0x0000 > > > #define SEG_BDA 0x0040 > > > -- > > > 1.7.10.4 > > > > -- > > Gleb. > > > > -- > Eduardo -- Gleb.
On Mon, Jul 23, 2012 at 02:42:27PM +0300, Gleb Natapov wrote: > On Fri, Jul 20, 2012 at 01:22:43PM -0300, Eduardo Habkost wrote: > > On Fri, Jul 20, 2012 at 12:18:59AM +0300, Gleb Natapov wrote: > > > On Thu, Jul 19, 2012 at 05:52:41PM -0300, Eduardo Habkost wrote: > > > > When resetting an I/O APIC, its ID is set to 0, so set it to 0 on the > > > > MADT table too. > > > > > > > Actually BIOS needs to configure ioapic id to a uniqe value. This does > > > not really matter for KVM though. > > > > Where does this requirement comes from? I am guessing it matters only > > when the I/O APIC is directly connected to the APIC bus (according to > > Intel SDM, that's the case only for old Pentium and P6 CPUs)[1]. > > > http://www.intel.com/design/chipsets/datashts/29056601.pdf says that it > should be programmed to unique value. I checked 4 machines on 3 of them > this was the case on one (AMD) there are 3 IOAPICs and they all overlap with > APICs. True, but it says "All APIC devices using the APIC bus". Figure 10-1 on Intel SDM Volume 3 makes me believe the I/O APIC and Local APICs are not connected to the same APIC bus since Pentium 4. Anyway, if anybody wants to patch SeaBIOS to guarantee the I/O APIC ID not conflict with the Local APICs, it would be welcome. While this is not done, the ACPI tables need to match reality. (The MP-Table has to be changed too, I will send a new patch soon. Thanks for pointing that out). > > > Anyway, even if some hardware has this unique-ID requirement, today > > Seabios does not fulfill it, leaving the I/O APIC ID as 0. The patch at > > least makes the MADT table match reality. > > > The currant code was written when we could only dream supporting 16 cpus > and back than it did correct thing, but now it just broken. QEMU do not > care about IOAPIC ID for sure so I am OK with the patch. > > > > [1] I have checked 3 different machines, and all machines I have looked > > have an I/O APIC ID that conflicts with an existing Local APIC ID, on > > the ACPI MADT table. > > > > Some iasl dumps may be found online by googling for: > > "Subtable Type : 01" "I/O Apic ID" > > > > I looked at 5 or 6 matches, and almost every one have an I/O APIC ID > > conflicting with a Local APIC ID. > 1 from 4 for me :) > > > > > > > > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > > > --- > > > > src/acpi.c | 2 +- > > > > src/config.h | 2 ++ > > > > 2 files changed, 3 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/src/acpi.c b/src/acpi.c > > > > index 55e4607..3f55de9 100644 > > > > --- a/src/acpi.c > > > > +++ b/src/acpi.c > > > > @@ -335,7 +335,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..878c691 100644 > > > > --- a/src/config.h > > > > +++ b/src/config.h > > > > @@ -52,9 +52,11 @@ > > > > #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 > > > > > > > > + > > > > // Important real-mode segments > > > > #define SEG_IVT 0x0000 > > > > #define SEG_BDA 0x0040 > > > > -- > > > > 1.7.10.4 > > > > > > -- > > > Gleb. > > > > > > > -- > > Eduardo > > -- > Gleb. >
diff --git a/src/acpi.c b/src/acpi.c index 55e4607..3f55de9 100644 --- a/src/acpi.c +++ b/src/acpi.c @@ -335,7 +335,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..878c691 100644 --- a/src/config.h +++ b/src/config.h @@ -52,9 +52,11 @@ #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 + // Important real-mode segments #define SEG_IVT 0x0000 #define SEG_BDA 0x0040
When resetting an I/O APIC, its ID is set to 0, so set it to 0 on the MADT table too. Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- src/acpi.c | 2 +- src/config.h | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-)