| Message ID | 4A0C44CD.4070000@us.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@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. 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@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. >> > > 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@vger.kernel.org > 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@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;