From mboxrd@z Thu Jan 1 00:00:00 1970 From: Beth Kon Subject: Re: [PATCH] bios: Fix MADT corruption and RSDT size when using -acpitable Date: Thu, 14 May 2009 12:20:29 -0400 Message-ID: <4A0C44CD.4070000@us.ibm.com> References: <1242262325-18444-1-git-send-email-vincent@vincent-minet.net> <4A0B6CF6.60609@codemonkey.ws> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="------------060305090204030905070204" Cc: Vincent Minet , kvm@vger.kernel.org To: Anthony Liguori Return-path: Received: from e34.co.us.ibm.com ([32.97.110.152]:46805 "EHLO e34.co.us.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756237AbZENQTh (ORCPT ); Thu, 14 May 2009 12:19:37 -0400 Received: from d03relay02.boulder.ibm.com (d03relay02.boulder.ibm.com [9.17.195.227]) by e34.co.us.ibm.com (8.13.1/8.13.1) with ESMTP id n4EGGwGV028767 for ; Thu, 14 May 2009 10:16:58 -0600 Received: from d03av01.boulder.ibm.com (d03av01.boulder.ibm.com [9.17.195.167]) by d03relay02.boulder.ibm.com (8.13.8/8.13.8/NCO v9.2) with ESMTP id n4EGJa25075648 for ; Thu, 14 May 2009 10:19:36 -0600 Received: from d03av01.boulder.ibm.com (loopback [127.0.0.1]) by d03av01.boulder.ibm.com (8.12.11.20060308/8.13.3) with ESMTP id n4EGJYl5016943 for ; Thu, 14 May 2009 10:19:34 -0600 In-Reply-To: <4A0B6CF6.60609@codemonkey.ws> Sender: kvm-owner@vger.kernel.org List-ID: This is a multi-part message in MIME format. --------------060305090204030905070204 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit 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 >> > > 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 majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html --------------060305090204030905070204 Content-Type: text/x-patch; name="acpi_tables_fix_2.patch" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="acpi_tables_fix_2.patch" 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; --------------060305090204030905070204--