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?
> #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?
> #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
