Marcelo Tosatti wrote:
Beth,
On Thu, May 14, 2009 at 12:20:29PM -0400, Beth Kon wrote:
Anthony Liguori wrote:
Vincent Minet wrote:
External ACPI tables are counted twice for the RSDT size and the load
address for the first external table is in the MADT (interrupt override
entries are overwritten).
Signed-off-by: Vincent Minet <vincent@xxxxxxxxxxxxxxxxx>
Beth,
I think you had a patch attempting to address the same issue. It was
a bit more involved though.
Which is the proper fix and are they both to the same problem?
They are for 2 different bases. My patch was for qemu's bochs bios and
this is for qemu-kvm/kvm/bios/rombios32.c. They are pretty divergent in
this area of setting up the ACPI tables. My patch is still needed for
the qemu base. I hope we'll be getting to one base soon :-)
Assuming the intent of the code was for MAX_RSDT_ENTRIES to include
external_tables, this patch looks correct. I think one additional check
would be needed (in my patch) to make sure that the code doesn't exceed
MAX_RSDT_ENTRIES when the external tables are being loaded.
My patch also puts all the code that calculates madt_size in the same
place, at the beginning of the table layout. I believe this is neater
and will avoid problems like this one in the future. As much as
possible, I think it best to get all the tables layed out, then fill
them in. If for some reason this is not acceptable, we need to add a big
note that no tables should be layed out after the madt because the madt
may grow further down in the code and overwrite the other table.
I like this better too, see questions/comments below.
Regards,
Anthony Liguori
---
kvm/bios/rombios32.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/kvm/bios/rombios32.c b/kvm/bios/rombios32.c
index cbd5f15..289361b 100755
--- a/kvm/bios/rombios32.c
+++ b/kvm/bios/rombios32.c
@@ -1626,7 +1626,7 @@ void acpi_bios_init(void)
addr = base_addr = ram_size - ACPI_DATA_SIZE;
rsdt_addr = addr;
rsdt = (void *)(addr);
- rsdt_size = sizeof(*rsdt) + external_tables * 4;
+ rsdt_size = sizeof(*rsdt);
addr += rsdt_size;
fadt_addr = addr;
@@ -1787,6 +1787,7 @@ void acpi_bios_init(void)
}
int_override++;
madt_size += sizeof(struct madt_int_override);
+ addr += sizeof(struct madt_int_override);
}
acpi_build_table_header((struct acpi_table_header *)madt,
"APIC", madt_size, 1);
diff --git a/kvm/bios/rombios32.c b/kvm/bios/rombios32.c
index cbd5f15..23835b6 100755
--- a/kvm/bios/rombios32.c
+++ b/kvm/bios/rombios32.c
@@ -1626,7 +1626,7 @@ void acpi_bios_init(void)
addr = base_addr = ram_size - ACPI_DATA_SIZE;
rsdt_addr = addr;
rsdt = (void *)(addr);
- rsdt_size = sizeof(*rsdt) + external_tables * 4;
+ rsdt_size = sizeof(*rsdt);
addr += rsdt_size;
fadt_addr = addr;
@@ -1665,6 +1665,7 @@ void acpi_bios_init(void)
addr = (addr + 7) & ~7;
madt_addr = addr;
+ madt = (void *)(addr);
madt_size = sizeof(*madt) +
sizeof(struct madt_processor_apic) * MAX_CPUS +
#ifdef BX_QEMU
@@ -1672,7 +1673,11 @@ void acpi_bios_init(void)
#else
sizeof(struct madt_io_apic);
#endif
- madt = (void *)(addr);
+ for ( i = 0; i < 16; i++ ) {
+ if ( PCI_ISA_IRQ_MASK & (1U << i) ) {
+ madt_size += sizeof(struct madt_int_override);
+ }
+ }
addr += madt_size;
This bug could only affect the HPET descriptor right?
I'm not sure what you're asking. There were 2 bugs that Vincent pointed
out. The first caused an incorrect rsdt_size to be reported, and the
second (missing addr += sizeof(struct madt_int_override)) caused
corruption of whatever came after the MADT. But even if his patch were
applied, any future code that added a table and manipulated addr between
the following points:
...
(about line 1676)
madt = (void *)(addr);
addr += madt_size;
...
(about line 1789)
madt_size += sizeof(struct madt_int_override);
addr += sizeof(struct madt_int_override);
would have wound up causing some kind of corruption, as happened with
the HPET. Also the "memset(madt, 0, madt_size)" around line 1740 was not
using the complete madt_size.
So this seems undesirable, and that's why I suggested moving all addr
manipulation (with the exception of additional tables at the very end)
to the same section of the table layout code. Seems best to manage
madt_size all in one place.
#ifdef BX_QEMU
@@ -1786,7 +1791,6 @@ void acpi_bios_init(void)
continue;
}
int_override++;
- madt_size += sizeof(struct madt_int_override);
}
acpi_build_table_header((struct acpi_table_header *)madt,
"APIC", madt_size, 1);
@@ -1868,17 +1872,6 @@ void acpi_bios_init(void)
acpi_build_table_header((struct acpi_table_header *)hpet,
"HPET", sizeof(*hpet), 1);
#endif
-
- acpi_additional_tables(); /* resets cfg to required entry */
- for(i = 0; i < external_tables; i++) {
- uint16_t len;
- if(acpi_load_table(i, addr, &len) < 0)
- BX_PANIC("Failed to load ACPI table from QEMU\n");
- rsdt->table_offset_entry[nb_rsdt_entries++] = cpu_to_le32(addr);
- addr += len;
- if(addr >= ram_size)
- BX_PANIC("ACPI table overflow\n");
- }
The external ACPI tables fix(es) are logically separate from the MADT
intoverride size calculation, and so they could be separate patches?
Yes that's true.
#endif
/* RSDT */
@@ -1891,6 +1884,16 @@ void acpi_bios_init(void)
// rsdt->table_offset_entry[nb_rsdt_entries++] = cpu_to_le32(hpet_addr);
if (nb_numa_nodes > 0)
rsdt->table_offset_entry[nb_rsdt_entries++] = cpu_to_le32(srat_addr);
+ acpi_additional_tables(); /* resets cfg to required entry */
+ for(i = 0; i < external_tables; i++) {
+ uint16_t len;
+ if(acpi_load_table(i, addr, &len) < 0)
+ BX_PANIC("Failed to load ACPI table from QEMU\n");
+ rsdt->table_offset_entry[nb_rsdt_entries++] = cpu_to_le32(addr);
+ addr += len;
+ if((addr >= ram_size) || (nb_rsdt_entries + 1 > MAX_RSDT_ENTRIES))
+ BX_PANIC("ACPI table overflow\n");
+ }
#endif
rsdt_size -= MAX_RSDT_ENTRIES * 4;
rsdt_size += nb_rsdt_entries * 4;
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
