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 <vinc...@vincent-minet.net>

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.




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

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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;
 
 #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");
-    }
 #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;

Reply via email to