Hi, > From: Devel [mailto:devel-bounces@xxxxxxxxxx] On Behalf Of Zheng, Lv > Subject: Re: [Devel] [RFC PATCH 0/7] ACPICA: Tables: Cleanup table sanity checks > > Hi, > > > From: Hans de Goede [mailto:hdegoede@xxxxxxxxxx] > > Subject: Re: [RFC PATCH 0/7] ACPICA: Tables: Cleanup table sanity checks > > > > Hi, > > > > On 15-03-17 07:36, Lv Zheng wrote: > > > Originally AML table sanity check is only done for Load opcode, as > > > acpi_tb_compare_tables() is only invoked in > > > acpi_tb_install_standard_table(). While LoadTable opcode cannot be covered > > > as it directly invokes acpi_tb_load_table() without invoking > > > acpi_tb_install_standard_table(). Furthermore, standard table load also > > > cannot be covered as acpi_ns_load_table() is invoked instead of > > > acpi_tb_install_standard_table() and acpi_tb_load_table(). > > > > > > So it becomes a problem if a duplicate table is in RSDT/XSDT, there is no > > > such check implemented for static load tables and LoadTable opcode. > > > > > > This patchset combines code paths (in return reduces some redundant code > > > blocks and enhances checks/locks) so that duplicate table detection can be > > > applied to all cases. > > > > I can confirm that this series fixes the errors I was seeing on my > > GPD win machine. > > Ok, thanks for the test and the report. > > > Note as explained in my reply to Robert I believe that > > the last patch from the series can be dropped: > > > > "On 15-03-17 04:05, Moore, Robert wrote: > > > And I would suppose a refinement would be: > > > > > > Compare headers > > > If headers match, compare entire tables. > > > > The existing memcmp already does that since the header is in the > > front, and memcmp will stop on the first difference, so there > > is no need to change anything." > > > > As Robert mentions we really a should compare the entire table > > if the checksums match since an 8 bit checksum is really weak and > > if we want to do that a simple memcmp will suffice as that will > > already exit directly if the checksum mismatches as the checksum > > is before the actual data. > > Maybe the suggestion is made on the previous fact. > The previous design prevents such duplicate tables from being > installed. It surely is able to prevent same table from being > installed twice. > Under the previous design, we should do the duplicate table > detection in acpi_tb_verify_temp_table() which is exact the API used > before the table is installed to the global table list. > But there is a special logic for table install: > Linux can only use limited slots to map ACPI tables. > Thus most of the time, we can only map the table header. > > The series now follows your idea, split sanity checks into 2 levels: > 1. for table install, only check "address" and "memory types" > 2. for table load, check "table content" to detect duplicate tables > And since the table load doesn't have similar restriction, we seem > to be able to map the whole table to compare. > > And I noticed comments in acpi_tb_compare_tables(). It looks like > comparing the Whole table rather than comparing the table header is > a design decision made several years ago due to some reasons. > > As such: > 1. With new design, we are able to compare the whole table > 2. Comparing whole table rather than comparing table header looks > like a determined design choice > I made it the last patch and a separate one so that we can drop > it at any time. Dropped and the pull request is updated: https://github.com/acpica/acpica/pull/121 If everything works smoothly, you'll see it fixed by next ACPICA release. Thanks Lv > > > > > > Lv Zheng (7): > > > ACPICA: Tables: Cleanup table handler invokers > > > ACPICA: Tables: Add sanity check for load_table opcode > > > ACPICA: Tables: Add sanity check for table install > > > ACPICA: Tables: Fix an unwanted exception for table reloading > > > ACPICA: Tables: Enable acpi_ut_is_aml_table() for all ACPICA modules > > > ACPICA: Tables: Add sanity check for static table load > > > ACPICA: Tables: Change table comparison using table header for static > > > load tables > > > > > > drivers/acpi/acpica/actables.h | 7 +- > > > drivers/acpi/acpica/acutils.h | 4 +- > > > drivers/acpi/acpica/exconfig.c | 2 +- > > > drivers/acpi/acpica/nsload.c | 18 --- > > > drivers/acpi/acpica/tbdata.c | 288 ++++++++++++++++++++++++++++++++++++----- > > > drivers/acpi/acpica/tbinstal.c | 197 ++++++---------------------- > > > drivers/acpi/acpica/tbxfload.c | 46 +------ > > > drivers/acpi/acpica/utmisc.c | 27 ++-- > > > include/acpi/actbl.h | 1 + > > > 9 files changed, 325 insertions(+), 265 deletions(-) > > > > _______________________________________________ > Devel mailing list > Devel@xxxxxxxxxx > https://lists.acpica.org/mailman/listinfo/devel -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
