| Message ID | 20170227093432.3308-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Hi, > From: Hans de Goede [mailto:hdegoede@redhat.com] > Subject: [PATCH] ACPICA: Detect duplicate SSDT tables > > Some machines have the exact (byte for byte) same SSDT tables multiple > times in the root_table_list. Could you give a machine list here? > Detect this and silently skip the duplicates > rather then printing a scary looking set of errors. Why will this matter to OSPMs? And should we add non-costless steps just in order to reduce errors, while the errors are on the contrary useful (indicating platform issues)? Thanks Lv > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/acpi/acpica/tbxfload.c | 41 ++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 40 insertions(+), 1 deletion(-) > > diff --git a/drivers/acpi/acpica/tbxfload.c b/drivers/acpi/acpica/tbxfload.c > index 82019c0..1971cd7 100644 > --- a/drivers/acpi/acpica/tbxfload.c > +++ b/drivers/acpi/acpica/tbxfload.c > @@ -125,6 +125,44 @@ ACPI_EXPORT_SYMBOL_INIT(acpi_load_tables) > > /******************************************************************************* > * > + * FUNCTION: acpi_tb_find_duplicate_ssdt > + * > + * PARAMETERS: table - validated acpi_table_desc of table to check > + * index - index of table to find a duplicate of > + * > + * RETURN: TRUE if a duplicate is found, FALSE if not > + * > + * DESCRIPTION: Private helper function for acpi_tb_load_namespace to > + * avoid trying to load duplicate ssdt tables > + * > + ******************************************************************************/ > +static u8 acpi_tb_find_duplicate_ssdt(struct acpi_table_desc *table, u32 index) > +{ > + struct acpi_table_desc *dup; > + u32 i; > + > + for (i = 0; i < index; ++i) { > + dup = &acpi_gbl_root_table_list.tables[i]; > + > + if (!acpi_gbl_root_table_list.tables[i].address || > + (!ACPI_COMPARE_NAME(dup->signature.ascii, ACPI_SIG_SSDT) > + && !ACPI_COMPARE_NAME(dup->signature.ascii, > + ACPI_SIG_PSDT) > + && !ACPI_COMPARE_NAME(dup->signature.ascii, > + ACPI_SIG_OSDT)) > + || ACPI_FAILURE(acpi_tb_validate_table(dup)) > + || dup->length != table->length) { > + continue; > + } > + > + if (memcmp(dup->pointer, table->pointer, table->length) == 0) > + return TRUE; > + } > + return FALSE; > +} > + > +/******************************************************************************* > + * > * FUNCTION: acpi_tb_load_namespace > * > * PARAMETERS: None > @@ -212,7 +250,8 @@ acpi_status acpi_tb_load_namespace(void) > ACPI_SIG_PSDT) > && !ACPI_COMPARE_NAME(table->signature.ascii, > ACPI_SIG_OSDT)) > - || ACPI_FAILURE(acpi_tb_validate_table(table))) { > + || ACPI_FAILURE(acpi_tb_validate_table(table)) > + || acpi_tb_find_duplicate_ssdt(table, i)) { > continue; > } > > -- > 2.9.3 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 28-02-17 06:19, Zheng, Lv wrote: > Hi, > >> From: Hans de Goede [mailto:hdegoede@redhat.com] >> Subject: [PATCH] ACPICA: Detect duplicate SSDT tables >> >> Some machines have the exact (byte for byte) same SSDT tables multiple >> times in the root_table_list. > > Could you give a machine list here? Currently I'm seeing this on a GPD win machine: http://www.gpd.hk/gpdwin.asp I thought I was seeing it on more machines, but those have different apci table loading errors... >> Detect this and silently skip the duplicates >> rather then printing a scary looking set of errors. > > Why will this matter to OSPMs? Not sure what you mean with OSPMs but I can tell you why this matters in general, Linux distributions like e.g. Fedora have been putting a lot of work in a smooth boot experience where end users do not get any scary text messages. For some more embedded like systems this even is a hard requirement. The kernel supports quiet kernel cmdline argument to silence normal kernel messages, which is part of what is needed but messages with a log level of error still get shown, breaking the "no scary text messages" product requirement. > And should we add non-costless steps just in order to reduce errors, Yes we should, work on that front has been happening for years, also the CPU cost of this check is quite small, memcmp will only happen on identically sized tables and even then it will exit as soon as a single byte differs. > while the errors are on the contrary useful (in1dicating platform issues)? These errors are useful for developers / during testing but not in production setups, esp. in the case of duplicate tables where not loading the duplicate leads to 0 bad side effects. I've an alternative proposal though, since this check just fixes a small part of the early boot messages caused by SSDT loading and since the code itself chooses to ignore any errors: /* Ignore errors while loading tables, get as many as possible */ How about setting a global flag while loading these tables and making ACPI_EXCEPTION calls log the exceptions with a log level of warning as well as turning the final: ACPI_ERROR((AE_INFO, "%u table load failures, %u successful", tables_failed, tables_loaded)); Into a warning ? Regards, Hans > > Thanks > Lv > >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> drivers/acpi/acpica/tbxfload.c | 41 ++++++++++++++++++++++++++++++++++++++++- >> 1 file changed, 40 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/acpi/acpica/tbxfload.c b/drivers/acpi/acpica/tbxfload.c >> index 82019c0..1971cd7 100644 >> --- a/drivers/acpi/acpica/tbxfload.c >> +++ b/drivers/acpi/acpica/tbxfload.c >> @@ -125,6 +125,44 @@ ACPI_EXPORT_SYMBOL_INIT(acpi_load_tables) >> >> /******************************************************************************* >> * >> + * FUNCTION: acpi_tb_find_duplicate_ssdt >> + * >> + * PARAMETERS: table - validated acpi_table_desc of table to check >> + * index - index of table to find a duplicate of >> + * >> + * RETURN: TRUE if a duplicate is found, FALSE if not >> + * >> + * DESCRIPTION: Private helper function for acpi_tb_load_namespace to >> + * avoid trying to load duplicate ssdt tables >> + * >> + ******************************************************************************/ >> +static u8 acpi_tb_find_duplicate_ssdt(struct acpi_table_desc *table, u32 index) >> +{ >> + struct acpi_table_desc *dup; >> + u32 i; >> + >> + for (i = 0; i < index; ++i) { >> + dup = &acpi_gbl_root_table_list.tables[i]; >> + >> + if (!acpi_gbl_root_table_list.tables[i].address || >> + (!ACPI_COMPARE_NAME(dup->signature.ascii, ACPI_SIG_SSDT) >> + && !ACPI_COMPARE_NAME(dup->signature.ascii, >> + ACPI_SIG_PSDT) >> + && !ACPI_COMPARE_NAME(dup->signature.ascii, >> + ACPI_SIG_OSDT)) >> + || ACPI_FAILURE(acpi_tb_validate_table(dup)) >> + || dup->length != table->length) { >> + continue; >> + } >> + >> + if (memcmp(dup->pointer, table->pointer, table->length) == 0) >> + return TRUE; >> + } >> + return FALSE; >> +} >> + >> +/******************************************************************************* >> + * >> * FUNCTION: acpi_tb_load_namespace >> * >> * PARAMETERS: None >> @@ -212,7 +250,8 @@ acpi_status acpi_tb_load_namespace(void) >> ACPI_SIG_PSDT) >> && !ACPI_COMPARE_NAME(table->signature.ascii, >> ACPI_SIG_OSDT)) >> - || ACPI_FAILURE(acpi_tb_validate_table(table))) { >> + || ACPI_FAILURE(acpi_tb_validate_table(table)) >> + || acpi_tb_find_duplicate_ssdt(table, i)) { >> continue; >> } >> >> -- >> 2.9.3 > -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Does the machine or machines work properly with Windows? This is always one of our early questions. Bob > -----Original Message----- > From: Hans de Goede [mailto:hdegoede@redhat.com] > Sent: Tuesday, February 28, 2017 6:32 AM > To: Zheng, Lv <lv.zheng@intel.com>; Rafael J . Wysocki > <rjw@rjwysocki.net>; Len Brown <lenb@kernel.org>; Moore, Robert > <robert.moore@intel.com> > Cc: linux-acpi@vger.kernel.org; devel@acpica.org > Subject: Re: [PATCH] ACPICA: Detect duplicate SSDT tables > > Hi, > > On 28-02-17 06:19, Zheng, Lv wrote: > > Hi, > > > >> From: Hans de Goede [mailto:hdegoede@redhat.com] > >> Subject: [PATCH] ACPICA: Detect duplicate SSDT tables > >> > >> Some machines have the exact (byte for byte) same SSDT tables > >> multiple times in the root_table_list. > > > > Could you give a machine list here? > > Currently I'm seeing this on a GPD win machine: > > http://www.gpd.hk/gpdwin.asp > > I thought I was seeing it on more machines, but those have different > apci table loading errors... > > >> Detect this and silently skip the duplicates rather then printing a > >> scary looking set of errors. > > > > Why will this matter to OSPMs? > > Not sure what you mean with OSPMs but I can tell you why this matters in > general, Linux distributions like e.g. Fedora have been putting a lot of > work in a smooth boot experience where end users do not get any scary > text messages. For some more embedded like systems this even is a hard > requirement. > > The kernel supports quiet kernel cmdline argument to silence normal > kernel messages, which is part of what is needed but messages with a log > level of error still get shown, breaking the "no scary text messages" > product requirement. > > > And should we add non-costless steps just in order to reduce errors, > > Yes we should, work on that front has been happening for years, also the > CPU cost of this check is quite small, memcmp will only happen on > identically sized tables and even then it will exit as soon as a single > byte differs. > > > while the errors are on the contrary useful (in1dicating platform > issues)? > > These errors are useful for developers / during testing but not in > production setups, esp. in the case of duplicate tables where not > loading the duplicate leads to 0 bad side effects. > > I've an alternative proposal though, since this check just fixes a small > part of the early boot messages caused by SSDT loading and since the > code itself chooses to ignore any errors: > > /* Ignore errors while loading tables, get as many as possible > */ > > How about setting a global flag while loading these tables and making > > ACPI_EXCEPTION calls log the exceptions with a log level of warning as > well as turning the final: > > ACPI_ERROR((AE_INFO, > "%u table load failures, %u successful", > tables_failed, tables_loaded)); > > Into a warning ? > > Regards, > > Hans > > > > > > > > Thanks > > Lv > > > >> > >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > >> --- > >> drivers/acpi/acpica/tbxfload.c | 41 > >> ++++++++++++++++++++++++++++++++++++++++- > >> 1 file changed, 40 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/acpi/acpica/tbxfload.c > >> b/drivers/acpi/acpica/tbxfload.c index 82019c0..1971cd7 100644 > >> --- a/drivers/acpi/acpica/tbxfload.c > >> +++ b/drivers/acpi/acpica/tbxfload.c > >> @@ -125,6 +125,44 @@ ACPI_EXPORT_SYMBOL_INIT(acpi_load_tables) > >> > >> > /*********************************************************************** > ******** > >> * > >> + * FUNCTION: acpi_tb_find_duplicate_ssdt > >> + * > >> + * PARAMETERS: table - validated acpi_table_desc of table > to check > >> + * index - index of table to find a duplicate > of > >> + * > >> + * RETURN: TRUE if a duplicate is found, FALSE if not > >> + * > >> + * DESCRIPTION: Private helper function for acpi_tb_load_namespace > to > >> + * avoid trying to load duplicate ssdt tables > >> + * > >> + > >> +******************************************************************** > >> +**********/ static u8 acpi_tb_find_duplicate_ssdt(struct > >> +acpi_table_desc *table, u32 index) { > >> + struct acpi_table_desc *dup; > >> + u32 i; > >> + > >> + for (i = 0; i < index; ++i) { > >> + dup = &acpi_gbl_root_table_list.tables[i]; > >> + > >> + if (!acpi_gbl_root_table_list.tables[i].address || > >> + (!ACPI_COMPARE_NAME(dup->signature.ascii, ACPI_SIG_SSDT) > >> + && !ACPI_COMPARE_NAME(dup->signature.ascii, > >> + ACPI_SIG_PSDT) > >> + && !ACPI_COMPARE_NAME(dup->signature.ascii, > >> + ACPI_SIG_OSDT)) > >> + || ACPI_FAILURE(acpi_tb_validate_table(dup)) > >> + || dup->length != table->length) { > >> + continue; > >> + } > >> + > >> + if (memcmp(dup->pointer, table->pointer, table->length) == 0) > >> + return TRUE; > >> + } > >> + return FALSE; > >> +} > >> + > >> +/******************************************************************* > >> +************ > >> + * > >> * FUNCTION: acpi_tb_load_namespace > >> * > >> * PARAMETERS: None > >> @@ -212,7 +250,8 @@ acpi_status acpi_tb_load_namespace(void) > >> ACPI_SIG_PSDT) > >> && !ACPI_COMPARE_NAME(table->signature.ascii, > >> ACPI_SIG_OSDT)) > >> - || ACPI_FAILURE(acpi_tb_validate_table(table))) { > >> + || ACPI_FAILURE(acpi_tb_validate_table(table)) > >> + || acpi_tb_find_duplicate_ssdt(table, i)) { > >> continue; > >> } > >> > >> -- > >> 2.9.3 > > -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 28-02-17 16:46, Moore, Robert wrote: > Does the machine or machines work properly with Windows? This is always one of our early questions. Yes although I do not see how that is really relevant or a discussion about changing the log level of certain errors ... Regards, Hans > Bob > > >> -----Original Message----- >> From: Hans de Goede [mailto:hdegoede@redhat.com] >> Sent: Tuesday, February 28, 2017 6:32 AM >> To: Zheng, Lv <lv.zheng@intel.com>; Rafael J . Wysocki >> <rjw@rjwysocki.net>; Len Brown <lenb@kernel.org>; Moore, Robert >> <robert.moore@intel.com> >> Cc: linux-acpi@vger.kernel.org; devel@acpica.org >> Subject: Re: [PATCH] ACPICA: Detect duplicate SSDT tables >> >> Hi, >> >> On 28-02-17 06:19, Zheng, Lv wrote: >>> Hi, >>> >>>> From: Hans de Goede [mailto:hdegoede@redhat.com] >>>> Subject: [PATCH] ACPICA: Detect duplicate SSDT tables >>>> >>>> Some machines have the exact (byte for byte) same SSDT tables >>>> multiple times in the root_table_list. >>> >>> Could you give a machine list here? >> >> Currently I'm seeing this on a GPD win machine: >> >> http://www.gpd.hk/gpdwin.asp >> >> I thought I was seeing it on more machines, but those have different >> apci table loading errors... >> >>>> Detect this and silently skip the duplicates rather then printing a >>>> scary looking set of errors. >>> >>> Why will this matter to OSPMs? >> >> Not sure what you mean with OSPMs but I can tell you why this matters in >> general, Linux distributions like e.g. Fedora have been putting a lot of >> work in a smooth boot experience where end users do not get any scary >> text messages. For some more embedded like systems this even is a hard >> requirement. >> >> The kernel supports quiet kernel cmdline argument to silence normal >> kernel messages, which is part of what is needed but messages with a log >> level of error still get shown, breaking the "no scary text messages" >> product requirement. >> >>> And should we add non-costless steps just in order to reduce errors, >> >> Yes we should, work on that front has been happening for years, also the >> CPU cost of this check is quite small, memcmp will only happen on >> identically sized tables and even then it will exit as soon as a single >> byte differs. >> >>> while the errors are on the contrary useful (in1dicating platform >> issues)? >> >> These errors are useful for developers / during testing but not in >> production setups, esp. in the case of duplicate tables where not >> loading the duplicate leads to 0 bad side effects. >> >> I've an alternative proposal though, since this check just fixes a small >> part of the early boot messages caused by SSDT loading and since the >> code itself chooses to ignore any errors: >> >> /* Ignore errors while loading tables, get as many as possible >> */ >> >> How about setting a global flag while loading these tables and making >> >> ACPI_EXCEPTION calls log the exceptions with a log level of warning as >> well as turning the final: >> >> ACPI_ERROR((AE_INFO, >> "%u table load failures, %u successful", >> tables_failed, tables_loaded)); >> >> Into a warning ? >> >> Regards, >> >> Hans >> >> >> >> >>> >>> Thanks >>> Lv >>> >>>> >>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>> --- >>>> drivers/acpi/acpica/tbxfload.c | 41 >>>> ++++++++++++++++++++++++++++++++++++++++- >>>> 1 file changed, 40 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/acpi/acpica/tbxfload.c >>>> b/drivers/acpi/acpica/tbxfload.c index 82019c0..1971cd7 100644 >>>> --- a/drivers/acpi/acpica/tbxfload.c >>>> +++ b/drivers/acpi/acpica/tbxfload.c >>>> @@ -125,6 +125,44 @@ ACPI_EXPORT_SYMBOL_INIT(acpi_load_tables) >>>> >>>> >> /*********************************************************************** >> ******** >>>> * >>>> + * FUNCTION: acpi_tb_find_duplicate_ssdt >>>> + * >>>> + * PARAMETERS: table - validated acpi_table_desc of table >> to check >>>> + * index - index of table to find a duplicate >> of >>>> + * >>>> + * RETURN: TRUE if a duplicate is found, FALSE if not >>>> + * >>>> + * DESCRIPTION: Private helper function for acpi_tb_load_namespace >> to >>>> + * avoid trying to load duplicate ssdt tables >>>> + * >>>> + >>>> +******************************************************************** >>>> +**********/ static u8 acpi_tb_find_duplicate_ssdt(struct >>>> +acpi_table_desc *table, u32 index) { >>>> + struct acpi_table_desc *dup; >>>> + u32 i; >>>> + >>>> + for (i = 0; i < index; ++i) { >>>> + dup = &acpi_gbl_root_table_list.tables[i]; >>>> + >>>> + if (!acpi_gbl_root_table_list.tables[i].address || >>>> + (!ACPI_COMPARE_NAME(dup->signature.ascii, ACPI_SIG_SSDT) >>>> + && !ACPI_COMPARE_NAME(dup->signature.ascii, >>>> + ACPI_SIG_PSDT) >>>> + && !ACPI_COMPARE_NAME(dup->signature.ascii, >>>> + ACPI_SIG_OSDT)) >>>> + || ACPI_FAILURE(acpi_tb_validate_table(dup)) >>>> + || dup->length != table->length) { >>>> + continue; >>>> + } >>>> + >>>> + if (memcmp(dup->pointer, table->pointer, table->length) == 0) >>>> + return TRUE; >>>> + } >>>> + return FALSE; >>>> +} >>>> + >>>> +/******************************************************************* >>>> +************ >>>> + * >>>> * FUNCTION: acpi_tb_load_namespace >>>> * >>>> * PARAMETERS: None >>>> @@ -212,7 +250,8 @@ acpi_status acpi_tb_load_namespace(void) >>>> ACPI_SIG_PSDT) >>>> && !ACPI_COMPARE_NAME(table->signature.ascii, >>>> ACPI_SIG_OSDT)) >>>> - || ACPI_FAILURE(acpi_tb_validate_table(table))) { >>>> + || ACPI_FAILURE(acpi_tb_validate_table(table)) >>>> + || acpi_tb_find_duplicate_ssdt(table, i)) { >>>> continue; >>>> } >>>> >>>> -- >>>> 2.9.3 >>> -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Hans de Goede [mailto:hdegoede@redhat.com] > Sent: Tuesday, February 28, 2017 3:45 PM > To: Moore, Robert; Zheng, Lv; Rafael J . Wysocki; Len Brown > Cc: linux-acpi@vger.kernel.org; devel@acpica.org > Subject: Re: [PATCH] ACPICA: Detect duplicate SSDT tables > > Hi, > > On 28-02-17 16:46, Moore, Robert wrote: > > Does the machine or machines work properly with Windows? This is always > one of our early questions. > > Yes although I do not see how that is really relevant or a discussion > about changing the log level of certain errors ... > Gee, I thought the original discussion was about multiple identical SSDTs, which led to this: acpi_tb_find_duplicate_ssdt I would imagine that ACPICA would generate lots of errors as the duplicate symbols were found. What Windows would do is my question. > Regards, > > Hans > > > > > Bob > > > > > >> -----Original Message----- > >> From: Hans de Goede [mailto:hdegoede@redhat.com] > >> Sent: Tuesday, February 28, 2017 6:32 AM > >> To: Zheng, Lv <lv.zheng@intel.com>; Rafael J . Wysocki > >> <rjw@rjwysocki.net>; Len Brown <lenb@kernel.org>; Moore, Robert > >> <robert.moore@intel.com> > >> Cc: linux-acpi@vger.kernel.org; devel@acpica.org > >> Subject: Re: [PATCH] ACPICA: Detect duplicate SSDT tables > >> > >> Hi, > >> > >> On 28-02-17 06:19, Zheng, Lv wrote: > >>> Hi, > >>> > >>>> From: Hans de Goede [mailto:hdegoede@redhat.com] > >>>> Subject: [PATCH] ACPICA: Detect duplicate SSDT tables > >>>> > >>>> Some machines have the exact (byte for byte) same SSDT tables > >>>> multiple times in the root_table_list. > >>> > >>> Could you give a machine list here? > >> > >> Currently I'm seeing this on a GPD win machine: > >> > >> http://www.gpd.hk/gpdwin.asp > >> > >> I thought I was seeing it on more machines, but those have different > >> apci table loading errors... > >> > >>>> Detect this and silently skip the duplicates rather then printing a > >>>> scary looking set of errors. > >>> > >>> Why will this matter to OSPMs? > >> > >> Not sure what you mean with OSPMs but I can tell you why this matters > >> in general, Linux distributions like e.g. Fedora have been putting a > >> lot of work in a smooth boot experience where end users do not get > >> any scary text messages. For some more embedded like systems this > >> even is a hard requirement. > >> > >> The kernel supports quiet kernel cmdline argument to silence normal > >> kernel messages, which is part of what is needed but messages with a > >> log level of error still get shown, breaking the "no scary text > messages" > >> product requirement. > >> > >>> And should we add non-costless steps just in order to reduce errors, > >> > >> Yes we should, work on that front has been happening for years, also > >> the CPU cost of this check is quite small, memcmp will only happen on > >> identically sized tables and even then it will exit as soon as a > >> single byte differs. > >> > >>> while the errors are on the contrary useful (in1dicating platform > >> issues)? > >> > >> These errors are useful for developers / during testing but not in > >> production setups, esp. in the case of duplicate tables where not > >> loading the duplicate leads to 0 bad side effects. > >> > >> I've an alternative proposal though, since this check just fixes a > >> small part of the early boot messages caused by SSDT loading and > >> since the code itself chooses to ignore any errors: > >> > >> /* Ignore errors while loading tables, get as many as > >> possible */ > >> > >> How about setting a global flag while loading these tables and making > >> > >> ACPI_EXCEPTION calls log the exceptions with a log level of warning > >> as well as turning the final: > >> > >> ACPI_ERROR((AE_INFO, > >> "%u table load failures, %u successful", > >> tables_failed, tables_loaded)); > >> > >> Into a warning ? > >> > >> Regards, > >> > >> Hans > >> > >> > >> > >> > >>> > >>> Thanks > >>> Lv > >>> > >>>> > >>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > >>>> --- > >>>> drivers/acpi/acpica/tbxfload.c | 41 > >>>> ++++++++++++++++++++++++++++++++++++++++- > >>>> 1 file changed, 40 insertions(+), 1 deletion(-) > >>>> > >>>> diff --git a/drivers/acpi/acpica/tbxfload.c > >>>> b/drivers/acpi/acpica/tbxfload.c index 82019c0..1971cd7 100644 > >>>> --- a/drivers/acpi/acpica/tbxfload.c > >>>> +++ b/drivers/acpi/acpica/tbxfload.c > >>>> @@ -125,6 +125,44 @@ ACPI_EXPORT_SYMBOL_INIT(acpi_load_tables) > >>>> > >>>> > >> /******************************************************************** > >> *** > >> ******** > >>>> * > >>>> + * FUNCTION: acpi_tb_find_duplicate_ssdt > >>>> + * > >>>> + * PARAMETERS: table - validated acpi_table_desc of table > >> to check > >>>> + * index - index of table to find a duplicate > >> of > >>>> + * > >>>> + * RETURN: TRUE if a duplicate is found, FALSE if not > >>>> + * > >>>> + * DESCRIPTION: Private helper function for acpi_tb_load_namespace > >> to > >>>> + * avoid trying to load duplicate ssdt tables > >>>> + * > >>>> + > >>>> +****************************************************************** > >>>> +** **********/ static u8 acpi_tb_find_duplicate_ssdt(struct > >>>> +acpi_table_desc *table, u32 index) { > >>>> + struct acpi_table_desc *dup; > >>>> + u32 i; > >>>> + > >>>> + for (i = 0; i < index; ++i) { > >>>> + dup = &acpi_gbl_root_table_list.tables[i]; > >>>> + > >>>> + if (!acpi_gbl_root_table_list.tables[i].address || > >>>> + (!ACPI_COMPARE_NAME(dup->signature.ascii, > ACPI_SIG_SSDT) > >>>> + && !ACPI_COMPARE_NAME(dup->signature.ascii, > >>>> + ACPI_SIG_PSDT) > >>>> + && !ACPI_COMPARE_NAME(dup->signature.ascii, > >>>> + ACPI_SIG_OSDT)) > >>>> + || ACPI_FAILURE(acpi_tb_validate_table(dup)) > >>>> + || dup->length != table->length) { > >>>> + continue; > >>>> + } > >>>> + > >>>> + if (memcmp(dup->pointer, table->pointer, table->length) > == 0) > >>>> + return TRUE; > >>>> + } > >>>> + return FALSE; > >>>> +} > >>>> + > >>>> +/***************************************************************** > >>>> +** > >>>> +************ > >>>> + * > >>>> * FUNCTION: acpi_tb_load_namespace > >>>> * > >>>> * PARAMETERS: None > >>>> @@ -212,7 +250,8 @@ acpi_status acpi_tb_load_namespace(void) > >>>> ACPI_SIG_PSDT) > >>>> && !ACPI_COMPARE_NAME(table->signature.ascii, > >>>> ACPI_SIG_OSDT)) > >>>> - || ACPI_FAILURE(acpi_tb_validate_table(table))) { > >>>> + || ACPI_FAILURE(acpi_tb_validate_table(table)) > >>>> + || acpi_tb_find_duplicate_ssdt(table, i)) { > >>>> continue; > >>>> } > >>>> > >>>> -- > >>>> 2.9.3 > >>> -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, > From: Hans de Goede [mailto:hdegoede@redhat.com] > Subject: Re: [PATCH] ACPICA: Detect duplicate SSDT tables > > Hi, > > On 28-02-17 06:19, Zheng, Lv wrote: > > Hi, > > > >> From: Hans de Goede [mailto:hdegoede@redhat.com] > >> Subject: [PATCH] ACPICA: Detect duplicate SSDT tables > >> > >> Some machines have the exact (byte for byte) same SSDT tables multiple > >> times in the root_table_list. > > > > Could you give a machine list here? > > Currently I'm seeing this on a GPD win machine: > > http://www.gpd.hk/gpdwin.asp > > I thought I was seeing it on more machines, but those have > different apci table loading errors... I'm not sure what the Windows clones will behave in this case. Upon seeing a duplicate table, will Windows: 1. override old namespace node, or 2. discard (maybe silently) new namespace node that has conflict namespace hierarchy position against existing node, or 3. just complain us with a blue screen, or 4. compare all tables first. Before knowing the de-facto standard behavior, I'm not sure if the behavior introduced by this commit is correct. We should be able to judge if this case is real after knowing the Windows behavior. So I don't think this commit goes the right direction on the right track. > > >> Detect this and silently skip the duplicates > >> rather then printing a scary looking set of errors. > > > > Why will this matter to OSPMs? > > Not sure what you mean with OSPMs but I can tell you why this > matters in general, Linux distributions like e.g. Fedora have > been putting a lot of work in a smooth boot experience where > end users do not get any scary text messages. For some more > embedded like systems this even is a hard requirement. > > The kernel supports quiet kernel cmdline argument to silence > normal kernel messages, which is part of what is needed but > messages with a log level of error still get shown, breaking > the "no scary text messages" product requirement. > > > And should we add non-costless steps just in order to reduce errors, > > Yes we should, work on that front has been happening for years, > also the CPU cost of this check is quite small, memcmp will > only happen on identically sized tables and even then it will > exit as soon as a single byte differs. Even though, there are server systems containing many tables, almost one/two/three table(s) per CPU. And you surely need to compare each of them against each of the others. > > > while the errors are on the contrary useful (in1dicating platform issues)? > > These errors are useful for developers / during testing but > not in production setups, esp. in the case of duplicate tables > where not loading the duplicate leads to 0 bad side effects. > > I've an alternative proposal though, since this check just fixes > a small part of the early boot messages caused by SSDT loading > and since the code itself chooses to ignore any errors: > > /* Ignore errors while loading tables, get as many as possible */ > > How about setting a global flag while loading these tables and making > > ACPI_EXCEPTION calls log the exceptions with a log level of warning > as well as turning the final: > > ACPI_ERROR((AE_INFO, > "%u table load failures, %u successful", > tables_failed, tables_loaded)); > > Into a warning ? So will Linux just unconditionally change pr_err() into pr_warn() in the printk.h? ACPI_EXCEPTION here actually means blue screen in Windows. Maybe it's correct, maybe not. Linux doesn't run into panic in ACPI_EXCEPTIION just because Linux ACPI implementation still has compliance issues against the de-facto standard. Anyway, we should tune the logging level according to the de-facto standard behavior. Thanks and best regards Lv > > Regards, > > Hans > > > > > > > > Thanks > > Lv > > > >> > >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > >> --- > >> drivers/acpi/acpica/tbxfload.c | 41 ++++++++++++++++++++++++++++++++++++++++- > >> 1 file changed, 40 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/acpi/acpica/tbxfload.c b/drivers/acpi/acpica/tbxfload.c > >> index 82019c0..1971cd7 100644 > >> --- a/drivers/acpi/acpica/tbxfload.c > >> +++ b/drivers/acpi/acpica/tbxfload.c > >> @@ -125,6 +125,44 @@ ACPI_EXPORT_SYMBOL_INIT(acpi_load_tables) > >> > >> /******************************************************************************* > >> * > >> + * FUNCTION: acpi_tb_find_duplicate_ssdt > >> + * > >> + * PARAMETERS: table - validated acpi_table_desc of table to check > >> + * index - index of table to find a duplicate of > >> + * > >> + * RETURN: TRUE if a duplicate is found, FALSE if not > >> + * > >> + * DESCRIPTION: Private helper function for acpi_tb_load_namespace to > >> + * avoid trying to load duplicate ssdt tables > >> + * > >> + ******************************************************************************/ > >> +static u8 acpi_tb_find_duplicate_ssdt(struct acpi_table_desc *table, u32 index) > >> +{ > >> + struct acpi_table_desc *dup; > >> + u32 i; > >> + > >> + for (i = 0; i < index; ++i) { > >> + dup = &acpi_gbl_root_table_list.tables[i]; > >> + > >> + if (!acpi_gbl_root_table_list.tables[i].address || > >> + (!ACPI_COMPARE_NAME(dup->signature.ascii, ACPI_SIG_SSDT) > >> + && !ACPI_COMPARE_NAME(dup->signature.ascii, > >> + ACPI_SIG_PSDT) > >> + && !ACPI_COMPARE_NAME(dup->signature.ascii, > >> + ACPI_SIG_OSDT)) > >> + || ACPI_FAILURE(acpi_tb_validate_table(dup)) > >> + || dup->length != table->length) { > >> + continue; > >> + } > >> + > >> + if (memcmp(dup->pointer, table->pointer, table->length) == 0) > >> + return TRUE; > >> + } > >> + return FALSE; > >> +} > >> + > >> +/******************************************************************************* > >> + * > >> * FUNCTION: acpi_tb_load_namespace > >> * > >> * PARAMETERS: None > >> @@ -212,7 +250,8 @@ acpi_status acpi_tb_load_namespace(void) > >> ACPI_SIG_PSDT) > >> && !ACPI_COMPARE_NAME(table->signature.ascii, > >> ACPI_SIG_OSDT)) > >> - || ACPI_FAILURE(acpi_tb_validate_table(table))) { > >> + || ACPI_FAILURE(acpi_tb_validate_table(table)) > >> + || acpi_tb_find_duplicate_ssdt(table, i)) { > >> continue; > >> } > >> > >> -- > >> 2.9.3 > > -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 01-03-17 04:21, Zheng, Lv wrote: > Hi, > >> From: Hans de Goede [mailto:hdegoede@redhat.com] >> Subject: Re: [PATCH] ACPICA: Detect duplicate SSDT tables >> >> Hi, >> >> On 28-02-17 06:19, Zheng, Lv wrote: >>> Hi, >>> >>>> From: Hans de Goede [mailto:hdegoede@redhat.com] >>>> Subject: [PATCH] ACPICA: Detect duplicate SSDT tables >>>> >>>> Some machines have the exact (byte for byte) same SSDT tables multiple >>>> times in the root_table_list. >>> >>> Could you give a machine list here? >> >> Currently I'm seeing this on a GPD win machine: >> >> http://www.gpd.hk/gpdwin.asp >> >> I thought I was seeing it on more machines, but those have >> different apci table loading errors... > > I'm not sure what the Windows clones will behave in this case. > Upon seeing a duplicate table, will Windows: > 1. override old namespace node, or > 2. discard (maybe silently) new namespace node that has conflict namespace hierarchy position against existing node, or > 3. just complain us with a blue screen, or > 4. compare all tables first. > > Before knowing the de-facto standard behavior, I'm not sure if the behavior introduced by this commit is correct. > We should be able to judge if this case is real after knowing the Windows behavior. It is impossible to know what Windows does under the hood, but it does work without complaints on this device, so it certainly does not do 3. As for 1., 2. and 4. since these are identical tables the end result is the same in all 3 cases, it is as if only a single copy was used: 1. Overriding with the exact same table is a no-op 2. Silently discarding means the old copy is used 4. Comparing tables and presumable then not loading duplicate ones will result in the old copy being used. So it really does not matter which route windows goes, the end result is: Things work without the user being show scary error messages during boot. > So I don't think this commit goes the right direction on the right track. > >> >>>> Detect this and silently skip the duplicates >>>> rather then printing a scary looking set of errors. >>> >>> Why will this matter to OSPMs? >> >> Not sure what you mean with OSPMs but I can tell you why this >> matters in general, Linux distributions like e.g. Fedora have >> been putting a lot of work in a smooth boot experience where >> end users do not get any scary text messages. For some more >> embedded like systems this even is a hard requirement. >> >> The kernel supports quiet kernel cmdline argument to silence >> normal kernel messages, which is part of what is needed but >> messages with a log level of error still get shown, breaking >> the "no scary text messages" product requirement. >> >>> And should we add non-costless steps just in order to reduce errors, >> >> Yes we should, work on that front has been happening for years, >> also the CPU cost of this check is quite small, memcmp will >> only happen on identically sized tables and even then it will >> exit as soon as a single byte differs. > > Even though, there are server systems containing many tables, almost one/two/three table(s) per CPU. > And you surely need to compare each of them against each of the others. And those machines typically take quite a lot time to boot anyways. Note my patch is only checking previously loaded tables (we do want to load the first copy). So all it is doing is accessing system memory from the CPU. I think you will find in impossible to even measure the extra boot time these few extra (likely cached) system memory accesses take, let alone that it will be anywhere near relevant for the total boot time. >>> while the errors are on the contrary useful (in1dicating platform issues)? >> >> These errors are useful for developers / during testing but >> not in production setups, esp. in the case of duplicate tables >> where not loading the duplicate leads to 0 bad side effects. >> >> I've an alternative proposal though, since this check just fixes >> a small part of the early boot messages caused by SSDT loading >> and since the code itself chooses to ignore any errors: >> >> /* Ignore errors while loading tables, get as many as possible */ >> >> How about setting a global flag while loading these tables and making >> >> ACPI_EXCEPTION calls log the exceptions with a log level of warning >> as well as turning the final: >> >> ACPI_ERROR((AE_INFO, >> "%u table load failures, %u successful", >> tables_failed, tables_loaded)); >> >> Into a warning ? > > So will Linux just unconditionally change pr_err() into pr_warn() in the printk.h? I'm not talking about unconditionally doing this, the acpica code itself contains in drivers/acpi/acpica/tbxfload.c: /* Ignore errors while loading tables, get as many as possible */ Since acpica is ignoring errors here, it would seem reasonable to me for acpica to treat all ACPI_ERROR / ACPI_EXCEPTION calls while doing this as ACPI_WARNING calls. > ACPI_EXCEPTION here actually means blue screen in Windows. > Maybe it's correct, maybe not. > Linux doesn't run into panic in ACPI_EXCEPTIION just because Linux ACPI implementation still has compliance issues against the de-facto standard. De-facto standard ? ACPI is a written standard, not a de-facto standard. I surely hope ACPICA tries to implements the standard as written... Regards, Hans >>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>> --- >>>> drivers/acpi/acpica/tbxfload.c | 41 ++++++++++++++++++++++++++++++++++++++++- >>>> 1 file changed, 40 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/acpi/acpica/tbxfload.c b/drivers/acpi/acpica/tbxfload.c >>>> index 82019c0..1971cd7 100644 >>>> --- a/drivers/acpi/acpica/tbxfload.c >>>> +++ b/drivers/acpi/acpica/tbxfload.c >>>> @@ -125,6 +125,44 @@ ACPI_EXPORT_SYMBOL_INIT(acpi_load_tables) >>>> >>>> /******************************************************************************* >>>> * >>>> + * FUNCTION: acpi_tb_find_duplicate_ssdt >>>> + * >>>> + * PARAMETERS: table - validated acpi_table_desc of table to check >>>> + * index - index of table to find a duplicate of >>>> + * >>>> + * RETURN: TRUE if a duplicate is found, FALSE if not >>>> + * >>>> + * DESCRIPTION: Private helper function for acpi_tb_load_namespace to >>>> + * avoid trying to load duplicate ssdt tables >>>> + * >>>> + ******************************************************************************/ >>>> +static u8 acpi_tb_find_duplicate_ssdt(struct acpi_table_desc *table, u32 index) >>>> +{ >>>> + struct acpi_table_desc *dup; >>>> + u32 i; >>>> + >>>> + for (i = 0; i < index; ++i) { >>>> + dup = &acpi_gbl_root_table_list.tables[i]; >>>> + >>>> + if (!acpi_gbl_root_table_list.tables[i].address || >>>> + (!ACPI_COMPARE_NAME(dup->signature.ascii, ACPI_SIG_SSDT) >>>> + && !ACPI_COMPARE_NAME(dup->signature.ascii, >>>> + ACPI_SIG_PSDT) >>>> + && !ACPI_COMPARE_NAME(dup->signature.ascii, >>>> + ACPI_SIG_OSDT)) >>>> + || ACPI_FAILURE(acpi_tb_validate_table(dup)) >>>> + || dup->length != table->length) { >>>> + continue; >>>> + } >>>> + >>>> + if (memcmp(dup->pointer, table->pointer, table->length) == 0) >>>> + return TRUE; >>>> + } >>>> + return FALSE; >>>> +} >>>> + >>>> +/******************************************************************************* >>>> + * >>>> * FUNCTION: acpi_tb_load_namespace >>>> * >>>> * PARAMETERS: None >>>> @@ -212,7 +250,8 @@ acpi_status acpi_tb_load_namespace(void) >>>> ACPI_SIG_PSDT) >>>> && !ACPI_COMPARE_NAME(table->signature.ascii, >>>> ACPI_SIG_OSDT)) >>>> - || ACPI_FAILURE(acpi_tb_validate_table(table))) { >>>> + || ACPI_FAILURE(acpi_tb_validate_table(table)) >>>> + || acpi_tb_find_duplicate_ssdt(table, i)) { >>>> continue; >>>> } >>>> >>>> -- >>>> 2.9.3 >>> -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Sorry if I've missed any of this conversation, but I have a couple of thoughts: 1) It might be sufficient to just compare the incoming table header (36 bytes). The header contains both the table length and the checksum -- as well as the OEM ID and OEM version. I take it that the XSDT pointers to the duplicate tables are different. 2) As far as comparing every incoming SSDT byte-for-byte against all previously loaded SSDTs, I have to think "what about the boot time?". Especially since SSDTs are increasing in both size and number (See example from a real machine below). ACPI: RSDP 0x0000000000492954 000024 (v02 Intel ) ACPI: XSDT 0x000000000068C938 0000B4 (v00 Intel AcpiExec 00001001 INTL 20170224) ACPI: FACP 0x0000000000492978 000114 (v05 Intel AcpiExec 00001001 INTL 20170224) ACPI: DSDT 0x0000000000690048 022FD6 (v02 LENOVO CB-01 00000001 ACPI 00040000) ACPI: FACS 0x0000000000492AB0 000040 ACPI: SSDT 0x00000000006B3028 0004B7 (v02 LENOVO CB-01 00000001 ACPI 00040000) ACPI: SSDT 0x000000000068D940 000B49 (v02 LENOVO CB-01 00000001 ACPI 00040000) ACPI: SSDT 0x00000000006B34E8 0004A3 (v02 LENOVO CB-01 00000001 ACPI 00040000) ACPI: SSDT 0x00000000006B3998 000E58 (v02 LENOVO CB-01 00000001 ACPI 00040000) ACPI: SSDT 0x00000000006B47F8 00037F (v02 PmRef Cpu0Cst 00003001 INTL 20121220) ACPI: SSDT 0x000000000068E498 000660 (v02 PmRef Cpu0Ist 00003000 INTL 20121220) ACPI: SSDT 0x00000000006B4B80 0005AA (v02 PmRef ApIst 00003000 INTL 20121220) ACPI: SSDT 0x000000000068EB00 000119 (v02 PmRef ApCst 00003000 INTL 20121220) ACPI: SSDT 0x000000000068EC28 00004B (v02 LENOVO CB-01 00000001 ACPI 00040000) ACPI: SSDT 0x00000000006B5138 0052EE (v02 LENOVO CB-01 00000001 ACPI 00040000) ACPI: SSDT 0x00000000006BA430 002C4A (v02 LENOVO CB-01 00000001 ACPI 00040000) ACPI: SSDT 0x00000000006BD088 0004C8 (v02 LENOVO CB-01 00000001 ACPI 00040000) ACPI: SSDT 0x000000000068EC80 0002D4 (v01 LENOVO CB-01 00000001 ACPI 00040000) ACPI: SSDT 0x00000000006BD558 002BAE (v02 LENOVO CB-01 00000001 ACPI 00040000) ACPI: SSDT 0x00000000006C0110 00019A (v02 LENOVO CB-01 00000001 ACPI 00040000) ACPI: SSDT 0x00000000006C02B8 0006DC (v02 LENOVO CB-01 00000001 ACPI 00040000) > -----Original Message----- > From: Hans de Goede [mailto:hdegoede@redhat.com] > Sent: Wednesday, March 1, 2017 1:19 AM > To: Zheng, Lv <lv.zheng@intel.com>; Rafael J . Wysocki > <rjw@rjwysocki.net>; Len Brown <lenb@kernel.org>; Moore, Robert > <robert.moore@intel.com> > Cc: linux-acpi@vger.kernel.org; devel@acpica.org > Subject: Re: [PATCH] ACPICA: Detect duplicate SSDT tables > > Hi, > > On 01-03-17 04:21, Zheng, Lv wrote: > > Hi, > > > >> From: Hans de Goede [mailto:hdegoede@redhat.com] > >> Subject: Re: [PATCH] ACPICA: Detect duplicate SSDT tables > >> > >> Hi, > >> > >> On 28-02-17 06:19, Zheng, Lv wrote: > >>> Hi, > >>> > >>>> From: Hans de Goede [mailto:hdegoede@redhat.com] > >>>> Subject: [PATCH] ACPICA: Detect duplicate SSDT tables > >>>> > >>>> Some machines have the exact (byte for byte) same SSDT tables > >>>> multiple times in the root_table_list. > >>> > >>> Could you give a machine list here? > >> > >> Currently I'm seeing this on a GPD win machine: > >> > >> http://www.gpd.hk/gpdwin.asp > >> > >> I thought I was seeing it on more machines, but those have different > >> apci table loading errors... > > > > I'm not sure what the Windows clones will behave in this case. > > Upon seeing a duplicate table, will Windows: > > 1. override old namespace node, or > > 2. discard (maybe silently) new namespace node that has conflict > > namespace hierarchy position against existing node, or 3. just > > complain us with a blue screen, or 4. compare all tables first. > > > > Before knowing the de-facto standard behavior, I'm not sure if the > behavior introduced by this commit is correct. > > We should be able to judge if this case is real after knowing the > Windows behavior. > > It is impossible to know what Windows does under the hood, but it does > work without complaints on this device, so it certainly does not do 3. > As for 1., 2. and 4. since these are identical tables the end result is > the same in all 3 cases, it is as if only a single copy was used: > > 1. Overriding with the exact same table is a no-op 2. Silently > discarding means the old copy is used 4. Comparing tables and presumable > then not loading duplicate > ones will result in the old copy being used. > > So it really does not matter which route windows goes, the end result > is: Things work without the user being show scary error messages during > boot. > > > So I don't think this commit goes the right direction on the right > track. > > > >> > >>>> Detect this and silently skip the duplicates rather then printing a > >>>> scary looking set of errors. > >>> > >>> Why will this matter to OSPMs? > >> > >> Not sure what you mean with OSPMs but I can tell you why this matters > >> in general, Linux distributions like e.g. Fedora have been putting a > >> lot of work in a smooth boot experience where end users do not get > >> any scary text messages. For some more embedded like systems this > >> even is a hard requirement. > >> > >> The kernel supports quiet kernel cmdline argument to silence normal > >> kernel messages, which is part of what is needed but messages with a > >> log level of error still get shown, breaking the "no scary text > >> messages" product requirement. > >> > >>> And should we add non-costless steps just in order to reduce errors, > >> > >> Yes we should, work on that front has been happening for years, also > >> the CPU cost of this check is quite small, memcmp will only happen on > >> identically sized tables and even then it will exit as soon as a > >> single byte differs. > > > > Even though, there are server systems containing many tables, almost > one/two/three table(s) per CPU. > > And you surely need to compare each of them against each of the > others. > > And those machines typically take quite a lot time to boot anyways. > > Note my patch is only checking previously loaded tables (we do want to > load the first copy). So all it is doing is accessing system memory from > the CPU. I think you will find in impossible to even measure the extra > boot time these few extra (likely cached) system memory accesses take, > let alone that it will be anywhere near relevant for the total boot > time. > > >>> while the errors are on the contrary useful (in1dicating platform > issues)? > >> > >> These errors are useful for developers / during testing but not in > >> production setups, esp. in the case of duplicate tables where not > >> loading the duplicate leads to 0 bad side effects. > >> > >> I've an alternative proposal though, since this check just fixes a > >> small part of the early boot messages caused by SSDT loading and > >> since the code itself chooses to ignore any errors: > >> > >> /* Ignore errors while loading tables, get as many as > >> possible */ > >> > >> How about setting a global flag while loading these tables and making > >> > >> ACPI_EXCEPTION calls log the exceptions with a log level of warning > >> as well as turning the final: > >> > >> ACPI_ERROR((AE_INFO, > >> "%u table load failures, %u successful", > >> tables_failed, tables_loaded)); > >> > >> Into a warning ? > > > > So will Linux just unconditionally change pr_err() into pr_warn() in > the printk.h? > > I'm not talking about unconditionally doing this, the acpica code itself > contains in drivers/acpi/acpica/tbxfload.c: > > /* Ignore errors while loading tables, get as many as possible > */ > > Since acpica is ignoring errors here, it would seem reasonable to me for > acpica to treat all ACPI_ERROR / ACPI_EXCEPTION calls while doing this > as ACPI_WARNING calls. > > > ACPI_EXCEPTION here actually means blue screen in Windows. > > Maybe it's correct, maybe not. > > Linux doesn't run into panic in ACPI_EXCEPTIION just because Linux > ACPI implementation still has compliance issues against the de-facto > standard. > > De-facto standard ? ACPI is a written standard, not a de-facto standard. > I surely hope ACPICA tries to implements the standard as written... > > Regards, > > Hans > > > > >>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > >>>> --- > >>>> drivers/acpi/acpica/tbxfload.c | 41 > >>>> ++++++++++++++++++++++++++++++++++++++++- > >>>> 1 file changed, 40 insertions(+), 1 deletion(-) > >>>> > >>>> diff --git a/drivers/acpi/acpica/tbxfload.c > >>>> b/drivers/acpi/acpica/tbxfload.c index 82019c0..1971cd7 100644 > >>>> --- a/drivers/acpi/acpica/tbxfload.c > >>>> +++ b/drivers/acpi/acpica/tbxfload.c > >>>> @@ -125,6 +125,44 @@ ACPI_EXPORT_SYMBOL_INIT(acpi_load_tables) > >>>> > >>>> > /*********************************************************************** > ******** > >>>> * > >>>> + * FUNCTION: acpi_tb_find_duplicate_ssdt > >>>> + * > >>>> + * PARAMETERS: table - validated acpi_table_desc of table > to check > >>>> + * index - index of table to find a duplicate > of > >>>> + * > >>>> + * RETURN: TRUE if a duplicate is found, FALSE if not > >>>> + * > >>>> + * DESCRIPTION: Private helper function for acpi_tb_load_namespace > to > >>>> + * avoid trying to load duplicate ssdt tables > >>>> + * > >>>> + > >>>> +****************************************************************** > >>>> +************/ static u8 acpi_tb_find_duplicate_ssdt(struct > >>>> +acpi_table_desc *table, u32 index) { > >>>> + struct acpi_table_desc *dup; > >>>> + u32 i; > >>>> + > >>>> + for (i = 0; i < index; ++i) { > >>>> + dup = &acpi_gbl_root_table_list.tables[i]; > >>>> + > >>>> + if (!acpi_gbl_root_table_list.tables[i].address || > >>>> + (!ACPI_COMPARE_NAME(dup->signature.ascii, > ACPI_SIG_SSDT) > >>>> + && !ACPI_COMPARE_NAME(dup->signature.ascii, > >>>> + ACPI_SIG_PSDT) > >>>> + && !ACPI_COMPARE_NAME(dup->signature.ascii, > >>>> + ACPI_SIG_OSDT)) > >>>> + || ACPI_FAILURE(acpi_tb_validate_table(dup)) > >>>> + || dup->length != table->length) { > >>>> + continue; > >>>> + } > >>>> + > >>>> + if (memcmp(dup->pointer, table->pointer, table->length) > == 0) > >>>> + return TRUE; > >>>> + } > >>>> + return FALSE; > >>>> +} > >>>> + > >>>> +/***************************************************************** > >>>> +************** > >>>> + * > >>>> * FUNCTION: acpi_tb_load_namespace > >>>> * > >>>> * PARAMETERS: None > >>>> @@ -212,7 +250,8 @@ acpi_status acpi_tb_load_namespace(void) > >>>> ACPI_SIG_PSDT) > >>>> && !ACPI_COMPARE_NAME(table->signature.ascii, > >>>> ACPI_SIG_OSDT)) > >>>> - || ACPI_FAILURE(acpi_tb_validate_table(table))) { > >>>> + || ACPI_FAILURE(acpi_tb_validate_table(table)) > >>>> + || acpi_tb_find_duplicate_ssdt(table, i)) { > >>>> continue; > >>>> } > >>>> > >>>> -- > >>>> 2.9.3 > >>> -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 1, 2017 at 9:38 PM, Moore, Robert <robert.moore@intel.com> wrote: > Sorry if I've missed any of this conversation, but I have a couple of thoughts: > > 1) It might be sufficient to just compare the incoming table header (36 bytes). The header contains both the table length and the checksum -- as well as the OEM ID and OEM version. I take it that the XSDT pointers to the duplicate tables are different. > > 2) As far as comparing every incoming SSDT byte-for-byte against all previously loaded SSDTs, I have to think "what about the boot time?". Especially since SSDTs are increasing in both size and number (See example from a real machine below). > > > ACPI: RSDP 0x0000000000492954 000024 (v02 Intel ) > ACPI: XSDT 0x000000000068C938 0000B4 (v00 Intel AcpiExec 00001001 INTL 20170224) > ACPI: FACP 0x0000000000492978 000114 (v05 Intel AcpiExec 00001001 INTL 20170224) > ACPI: DSDT 0x0000000000690048 022FD6 (v02 LENOVO CB-01 00000001 ACPI 00040000) > ACPI: FACS 0x0000000000492AB0 000040 > ACPI: SSDT 0x00000000006B3028 0004B7 (v02 LENOVO CB-01 00000001 ACPI 00040000) > ACPI: SSDT 0x000000000068D940 000B49 (v02 LENOVO CB-01 00000001 ACPI 00040000) > ACPI: SSDT 0x00000000006B34E8 0004A3 (v02 LENOVO CB-01 00000001 ACPI 00040000) > ACPI: SSDT 0x00000000006B3998 000E58 (v02 LENOVO CB-01 00000001 ACPI 00040000) > ACPI: SSDT 0x00000000006B47F8 00037F (v02 PmRef Cpu0Cst 00003001 INTL 20121220) > ACPI: SSDT 0x000000000068E498 000660 (v02 PmRef Cpu0Ist 00003000 INTL 20121220) > ACPI: SSDT 0x00000000006B4B80 0005AA (v02 PmRef ApIst 00003000 INTL 20121220) > ACPI: SSDT 0x000000000068EB00 000119 (v02 PmRef ApCst 00003000 INTL 20121220) > ACPI: SSDT 0x000000000068EC28 00004B (v02 LENOVO CB-01 00000001 ACPI 00040000) > ACPI: SSDT 0x00000000006B5138 0052EE (v02 LENOVO CB-01 00000001 ACPI 00040000) > ACPI: SSDT 0x00000000006BA430 002C4A (v02 LENOVO CB-01 00000001 ACPI 00040000) > ACPI: SSDT 0x00000000006BD088 0004C8 (v02 LENOVO CB-01 00000001 ACPI 00040000) > ACPI: SSDT 0x000000000068EC80 0002D4 (v01 LENOVO CB-01 00000001 ACPI 00040000) > ACPI: SSDT 0x00000000006BD558 002BAE (v02 LENOVO CB-01 00000001 ACPI 00040000) > ACPI: SSDT 0x00000000006C0110 00019A (v02 LENOVO CB-01 00000001 ACPI 00040000) > ACPI: SSDT 0x00000000006C02B8 0006DC (v02 LENOVO CB-01 00000001 ACPI 00040000) > Right. Whatever allows us to make a "we've loaded this table already, skip it" decision without comparing byte-for-byte, please. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, > From: Hans de Goede [mailto:hdegoede@redhat.com] > Subject: Re: [PATCH] ACPICA: Detect duplicate SSDT tables > > Hi, > > On 01-03-17 04:21, Zheng, Lv wrote: > > Hi, > > > >> From: Hans de Goede [mailto:hdegoede@redhat.com] > >> Subject: Re: [PATCH] ACPICA: Detect duplicate SSDT tables > >> > >> Hi, > >> > >> On 28-02-17 06:19, Zheng, Lv wrote: > >>> Hi, > >>> > >>>> From: Hans de Goede [mailto:hdegoede@redhat.com] > >>>> Subject: [PATCH] ACPICA: Detect duplicate SSDT tables > >>>> > >>>> Some machines have the exact (byte for byte) same SSDT tables multiple > >>>> times in the root_table_list. > >>> > >>> Could you give a machine list here? > >> > >> Currently I'm seeing this on a GPD win machine: > >> > >> http://www.gpd.hk/gpdwin.asp > >> > >> I thought I was seeing it on more machines, but those have > >> different apci table loading errors... > > > > I'm not sure what the Windows clones will behave in this case. > > Upon seeing a duplicate table, will Windows: > > 1. override old namespace node, or > > 2. discard (maybe silently) new namespace node that has conflict namespace hierarchy position > against existing node, or > > 3. just complain us with a blue screen, or > > 4. compare all tables first. > > > > Before knowing the de-facto standard behavior, I'm not sure if the behavior introduced by this > commit is correct. > > We should be able to judge if this case is real after knowing the Windows behavior. > > It is impossible to know what Windows does under the hood, It's possible. By injecting acpitable for qemu and obtain the test result via writing to a debugging port operation region field. But you won't know if this error is silent or not. Or if you have checked build windows clones, it is possible to see logs in windbg with amli enabled. And you'll know if the error is silent or not. > but it > does work without complaints on this device, so it certainly does > not do 3. As for 1., 2. and 4. since these are identical tables > the end result is the same in all 3 cases, it is as if only a > single copy was used: > > 1. Overriding with the exact same table is a no-op > 2. Silently discarding means the old copy is used > 4. Comparing tables and presumable then not loading duplicate > ones will result in the old copy being used. > > So it really does not matter which route windows goes, the > end result is: Things work without the user being show scary > error messages during boot. OK, I can see you really prefer the workaround way rather than the natural way to achieve the same engineering result. Then I'll give you suggestions on how it could be done better below. > > > So I don't think this commit goes the right direction on the right track. > > > >> > >>>> Detect this and silently skip the duplicates > >>>> rather then printing a scary looking set of errors. > >>> > >>> Why will this matter to OSPMs? > >> > >> Not sure what you mean with OSPMs but I can tell you why this > >> matters in general, Linux distributions like e.g. Fedora have > >> been putting a lot of work in a smooth boot experience where > >> end users do not get any scary text messages. For some more > >> embedded like systems this even is a hard requirement. > >> > >> The kernel supports quiet kernel cmdline argument to silence > >> normal kernel messages, which is part of what is needed but > >> messages with a log level of error still get shown, breaking > >> the "no scary text messages" product requirement. > >> > >>> And should we add non-costless steps just in order to reduce errors, > >> > >> Yes we should, work on that front has been happening for years, > >> also the CPU cost of this check is quite small, memcmp will > >> only happen on identically sized tables and even then it will > >> exit as soon as a single byte differs. > > > > Even though, there are server systems containing many tables, almost one/two/three table(s) per CPU. > > And you surely need to compare each of them against each of the others. > > And those machines typically take quite a lot time to boot anyways. > > Note my patch is only checking previously loaded tables (we do want > to load the first copy). So all it is doing is accessing system memory > from the CPU. I think you will find in impossible to even measure the > extra boot time these few extra (likely cached) system memory accesses > take, let alone that it will be anywhere near relevant for the total > boot time. But if it is not done in natural way, I'm afraid other functionality won't get any benefit from doing this. And after the root cause is determined and the right approach is upstreamed, the workarounds will need a cleanup. So the whole thing looks to me is: It's just increase a burden of maintenance by adding useless logics in order to reduce several error messages. If it's not done in the right place, it's affection won't be minimized and actually makes things worse. > > >>> while the errors are on the contrary useful (in1dicating platform issues)? > >> > >> These errors are useful for developers / during testing but > >> not in production setups, esp. in the case of duplicate tables > >> where not loading the duplicate leads to 0 bad side effects. > >> > >> I've an alternative proposal though, since this check just fixes > >> a small part of the early boot messages caused by SSDT loading > >> and since the code itself chooses to ignore any errors: > >> > >> /* Ignore errors while loading tables, get as many as possible */ > >> > >> How about setting a global flag while loading these tables and making > >> > >> ACPI_EXCEPTION calls log the exceptions with a log level of warning > >> as well as turning the final: > >> > >> ACPI_ERROR((AE_INFO, > >> "%u table load failures, %u successful", > >> tables_failed, tables_loaded)); > >> > >> Into a warning ? > > > > So will Linux just unconditionally change pr_err() into pr_warn() in the printk.h? > > I'm not talking about unconditionally doing this, OK, I see. > the acpica code itself > contains in drivers/acpi/acpica/tbxfload.c: > > /* Ignore errors while loading tables, get as many as possible */ Actually, if Windows interpreter encounters an error during table load, there is only 1 result - blue screen. During runtime, there is no blue screen but only exceptions returned to the drivers who call the method evaluations. So this comment just means a compromise to me. > > Since acpica is ignoring errors here, it would seem reasonable to me for > acpica to treat all ACPI_ERROR / ACPI_EXCEPTION calls while doing this > as ACPI_WARNING calls. > > > ACPI_EXCEPTION here actually means blue screen in Windows. > > Maybe it's correct, maybe not. > > Linux doesn't run into panic in ACPI_EXCEPTIION just because Linux ACPI implementation still has > compliance issues against the de-facto standard. > > De-facto standard ? ACPI is a written standard, not a de-facto standard. > I surely hope ACPICA tries to implements the standard as written... By calling de-facto standard, I mean Windows interpreter behavior. Windows is the de-facto standard and almost all BIOSen are tested by running one of Windows clones. And we are following Windows behavior rather than following the spec words. > >>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > >>>> --- > >>>> drivers/acpi/acpica/tbxfload.c | 41 ++++++++++++++++++++++++++++++++++++++++- > >>>> 1 file changed, 40 insertions(+), 1 deletion(-) > >>>> > >>>> diff --git a/drivers/acpi/acpica/tbxfload.c b/drivers/acpi/acpica/tbxfload.c > >>>> index 82019c0..1971cd7 100644 > >>>> --- a/drivers/acpi/acpica/tbxfload.c > >>>> +++ b/drivers/acpi/acpica/tbxfload.c > >>>> @@ -125,6 +125,44 @@ ACPI_EXPORT_SYMBOL_INIT(acpi_load_tables) > >>>> > >>>> /******************************************************************************* > >>>> * > >>>> + * FUNCTION: acpi_tb_find_duplicate_ssdt > >>>> + * > >>>> + * PARAMETERS: table - validated acpi_table_desc of table to check > >>>> + * index - index of table to find a duplicate of > >>>> + * > >>>> + * RETURN: TRUE if a duplicate is found, FALSE if not > >>>> + * > >>>> + * DESCRIPTION: Private helper function for acpi_tb_load_namespace to > >>>> + * avoid trying to load duplicate ssdt tables > >>>> + * > >>>> + ******************************************************************************/ > >>>> +static u8 acpi_tb_find_duplicate_ssdt(struct acpi_table_desc *table, u32 index) If you really want to add this. My suggestion is: Invoke this function in acpi_tb_verify_temp_table(). This is the function we use to verify if a table is OK. Otherwise it won't be installed to the global table list. And the name should contain "aml_table" or "aml" rather than "ssdt" according to your check below. > >>>> +{ > >>>> + struct acpi_table_desc *dup; > >>>> + u32 i; > >>>> + > >>>> + for (i = 0; i < index; ++i) { Parameter of index is not useful here. You should use acpi_gbl_root_table_list.current_table_count instead. > >>>> + dup = &acpi_gbl_root_table_list.tables[i]; > >>>> + > >>>> + if (!acpi_gbl_root_table_list.tables[i].address || The address check then is useless as for now we never uninstall tables once they are in acpi_gbl_root_table_list. > >>>> + (!ACPI_COMPARE_NAME(dup->signature.ascii, ACPI_SIG_SSDT) > >>>> + && !ACPI_COMPARE_NAME(dup->signature.ascii, > >>>> + ACPI_SIG_PSDT) > >>>> + && !ACPI_COMPARE_NAME(dup->signature.ascii, > >>>> + ACPI_SIG_OSDT)) All above ones might be changed to just check if it is an AML table. You can enable the function of acpi_ut_is_aml_table() for linux and invoke it here. > >>>> + || ACPI_FAILURE(acpi_tb_validate_table(dup)) You won't need this, as you should invoke this function from a point in acpi_tb_verify_temp_table() where the table should have already been validated. > >>>> + || dup->length != table->length) { > >>>> + continue; > >>>> + } > >>>> + > >>>> + if (memcmp(dup->pointer, table->pointer, table->length) == 0) I'm not sure if you know the story of acpi_gbl_verify_table_checksum. IMO, you can only do this when acpi_gbl_verify_table_checksum is TRUE. So in early stage, the functionality you introduced may still be a no-op for the same reason. > >>>> + return TRUE; > >>>> + } > >>>> + return FALSE; > >>>> +} > >>>> + > >>>> +/******************************************************************************* > >>>> + * > >>>> * FUNCTION: acpi_tb_load_namespace > >>>> * > >>>> * PARAMETERS: None > >>>> @@ -212,7 +250,8 @@ acpi_status acpi_tb_load_namespace(void) > >>>> ACPI_SIG_PSDT) > >>>> && !ACPI_COMPARE_NAME(table->signature.ascii, > >>>> ACPI_SIG_OSDT)) > >>>> - || ACPI_FAILURE(acpi_tb_validate_table(table))) { > >>>> + || ACPI_FAILURE(acpi_tb_validate_table(table)) > >>>> + || acpi_tb_find_duplicate_ssdt(table, i)) { So it shouldn't be invoked in acpi_tb_load_namespace(). If you really want it to be verified in case "acpi_gbl_verify_table_checksum=false" skipped the check. You should find a right position (should be where the acpi_gbl_verify_table_checksum is set to TRUE), Invoking acpi_tb_verify_temp_table() for those unverified tables (maybe a flag in acpi_table_desc). That could be the right approach. Thanks Lv > >>>> continue; > >>>> } > >>>> > >>>> -- > >>>> 2.9.3 > >>> -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 02-03-17 02:59, Zheng, Lv wrote: > Hi, > >> From: Hans de Goede [mailto:hdegoede@redhat.com] >> Subject: Re: [PATCH] ACPICA: Detect duplicate SSDT tables >> >> Hi, >> >> On 01-03-17 04:21, Zheng, Lv wrote: >>> Hi, >>> >>>> From: Hans de Goede [mailto:hdegoede@redhat.com] >>>> Subject: Re: [PATCH] ACPICA: Detect duplicate SSDT tables >>>> >>>> Hi, >>>> >>>> On 28-02-17 06:19, Zheng, Lv wrote: >>>>> Hi, >>>>> >>>>>> From: Hans de Goede [mailto:hdegoede@redhat.com] >>>>>> Subject: [PATCH] ACPICA: Detect duplicate SSDT tables >>>>>> >>>>>> Some machines have the exact (byte for byte) same SSDT tables multiple >>>>>> times in the root_table_list. <snip> >>>>>> Detect this and silently skip the duplicates >>>>>> rather then printing a scary looking set of errors. <snip> >>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>>>> --- >>>>>> drivers/acpi/acpica/tbxfload.c | 41 ++++++++++++++++++++++++++++++++++++++++- >>>>>> 1 file changed, 40 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/drivers/acpi/acpica/tbxfload.c b/drivers/acpi/acpica/tbxfload.c >>>>>> index 82019c0..1971cd7 100644 >>>>>> --- a/drivers/acpi/acpica/tbxfload.c >>>>>> +++ b/drivers/acpi/acpica/tbxfload.c >>>>>> @@ -125,6 +125,44 @@ ACPI_EXPORT_SYMBOL_INIT(acpi_load_tables) >>>>>> >>>>>> /******************************************************************************* >>>>>> * >>>>>> + * FUNCTION: acpi_tb_find_duplicate_ssdt >>>>>> + * >>>>>> + * PARAMETERS: table - validated acpi_table_desc of table to check >>>>>> + * index - index of table to find a duplicate of >>>>>> + * >>>>>> + * RETURN: TRUE if a duplicate is found, FALSE if not >>>>>> + * >>>>>> + * DESCRIPTION: Private helper function for acpi_tb_load_namespace to >>>>>> + * avoid trying to load duplicate ssdt tables >>>>>> + * >>>>>> + ******************************************************************************/ >>>>>> +static u8 acpi_tb_find_duplicate_ssdt(struct acpi_table_desc *table, u32 index) > > If you really want to add this. > My suggestion is: > Invoke this function in acpi_tb_verify_temp_table(). > This is the function we use to verify if a table is OK. > Otherwise it won't be installed to the global table list. > > And the name should contain "aml_table" or "aml" rather than "ssdt" according to your check below. Ok, when I've some time I will do a new version moving the functionality to acpi_tb_verify_temp_table(). I will post a v2 with this change when it is ready. I will also just compare the headers then as suggested in other replies in this thread. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, > From: Hans de Goede [mailto:hdegoede@redhat.com] > Subject: Re: [PATCH] ACPICA: Detect duplicate SSDT tables > > Hi, > > On 02-03-17 02:59, Zheng, Lv wrote: > > Hi, > > > >> From: Hans de Goede [mailto:hdegoede@redhat.com] > >> Subject: Re: [PATCH] ACPICA: Detect duplicate SSDT tables > >> > >> Hi, > >> > >> On 01-03-17 04:21, Zheng, Lv wrote: > >>> Hi, > >>> > >>>> From: Hans de Goede [mailto:hdegoede@redhat.com] > >>>> Subject: Re: [PATCH] ACPICA: Detect duplicate SSDT tables > >>>> > >>>> Hi, > >>>> > >>>> On 28-02-17 06:19, Zheng, Lv wrote: > >>>>> Hi, > >>>>> > >>>>>> From: Hans de Goede [mailto:hdegoede@redhat.com] > >>>>>> Subject: [PATCH] ACPICA: Detect duplicate SSDT tables > >>>>>> > >>>>>> Some machines have the exact (byte for byte) same SSDT tables multiple > >>>>>> times in the root_table_list. > > <snip> > > >>>>>> Detect this and silently skip the duplicates > >>>>>> rather then printing a scary looking set of errors. > > <snip> > > >>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > >>>>>> --- > >>>>>> drivers/acpi/acpica/tbxfload.c | 41 ++++++++++++++++++++++++++++++++++++++++- > >>>>>> 1 file changed, 40 insertions(+), 1 deletion(-) > >>>>>> > >>>>>> diff --git a/drivers/acpi/acpica/tbxfload.c b/drivers/acpi/acpica/tbxfload.c > >>>>>> index 82019c0..1971cd7 100644 > >>>>>> --- a/drivers/acpi/acpica/tbxfload.c > >>>>>> +++ b/drivers/acpi/acpica/tbxfload.c > >>>>>> @@ -125,6 +125,44 @@ ACPI_EXPORT_SYMBOL_INIT(acpi_load_tables) > >>>>>> > >>>>>> /******************************************************************************* > >>>>>> * > >>>>>> + * FUNCTION: acpi_tb_find_duplicate_ssdt > >>>>>> + * > >>>>>> + * PARAMETERS: table - validated acpi_table_desc of table to check > >>>>>> + * index - index of table to find a duplicate of > >>>>>> + * > >>>>>> + * RETURN: TRUE if a duplicate is found, FALSE if not > >>>>>> + * > >>>>>> + * DESCRIPTION: Private helper function for acpi_tb_load_namespace to > >>>>>> + * avoid trying to load duplicate ssdt tables > >>>>>> + * > >>>>>> + ******************************************************************************/ > >>>>>> +static u8 acpi_tb_find_duplicate_ssdt(struct acpi_table_desc *table, u32 index) > > > > If you really want to add this. > > My suggestion is: > > Invoke this function in acpi_tb_verify_temp_table(). > > This is the function we use to verify if a table is OK. > > Otherwise it won't be installed to the global table list. > > > > And the name should contain "aml_table" or "aml" rather than "ssdt" according to your check below. > > Ok, when I've some time I will do a new version moving the functionality > to acpi_tb_verify_temp_table(). I will post a v2 with this change when > it is ready. I will also just compare the headers then as suggested in > other replies in this thread. If you wish, I can offer help here. It actually won't take me much time to generate such a change. Thanks and best regards Lv -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 03-03-17 03:50, Zheng, Lv wrote: > Hi, > >> From: Hans de Goede [mailto:hdegoede@redhat.com] >> Subject: Re: [PATCH] ACPICA: Detect duplicate SSDT tables >> >> Hi, >> >> On 02-03-17 02:59, Zheng, Lv wrote: >>> Hi, >>> >>>> From: Hans de Goede [mailto:hdegoede@redhat.com] >>>> Subject: Re: [PATCH] ACPICA: Detect duplicate SSDT tables >>>> >>>> Hi, >>>> >>>> On 01-03-17 04:21, Zheng, Lv wrote: >>>>> Hi, >>>>> >>>>>> From: Hans de Goede [mailto:hdegoede@redhat.com] >>>>>> Subject: Re: [PATCH] ACPICA: Detect duplicate SSDT tables >>>>>> >>>>>> Hi, >>>>>> >>>>>> On 28-02-17 06:19, Zheng, Lv wrote: >>>>>>> Hi, >>>>>>> >>>>>>>> From: Hans de Goede [mailto:hdegoede@redhat.com] >>>>>>>> Subject: [PATCH] ACPICA: Detect duplicate SSDT tables >>>>>>>> >>>>>>>> Some machines have the exact (byte for byte) same SSDT tables multiple >>>>>>>> times in the root_table_list. >> >> <snip> >> >>>>>>>> Detect this and silently skip the duplicates >>>>>>>> rather then printing a scary looking set of errors. >> >> <snip> >> >>>>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>>>>>> --- >>>>>>>> drivers/acpi/acpica/tbxfload.c | 41 ++++++++++++++++++++++++++++++++++++++++- >>>>>>>> 1 file changed, 40 insertions(+), 1 deletion(-) >>>>>>>> >>>>>>>> diff --git a/drivers/acpi/acpica/tbxfload.c b/drivers/acpi/acpica/tbxfload.c >>>>>>>> index 82019c0..1971cd7 100644 >>>>>>>> --- a/drivers/acpi/acpica/tbxfload.c >>>>>>>> +++ b/drivers/acpi/acpica/tbxfload.c >>>>>>>> @@ -125,6 +125,44 @@ ACPI_EXPORT_SYMBOL_INIT(acpi_load_tables) >>>>>>>> >>>>>>>> /******************************************************************************* >>>>>>>> * >>>>>>>> + * FUNCTION: acpi_tb_find_duplicate_ssdt >>>>>>>> + * >>>>>>>> + * PARAMETERS: table - validated acpi_table_desc of table to check >>>>>>>> + * index - index of table to find a duplicate of >>>>>>>> + * >>>>>>>> + * RETURN: TRUE if a duplicate is found, FALSE if not >>>>>>>> + * >>>>>>>> + * DESCRIPTION: Private helper function for acpi_tb_load_namespace to >>>>>>>> + * avoid trying to load duplicate ssdt tables >>>>>>>> + * >>>>>>>> + ******************************************************************************/ >>>>>>>> +static u8 acpi_tb_find_duplicate_ssdt(struct acpi_table_desc *table, u32 index) >>> >>> If you really want to add this. >>> My suggestion is: >>> Invoke this function in acpi_tb_verify_temp_table(). >>> This is the function we use to verify if a table is OK. >>> Otherwise it won't be installed to the global table list. >>> >>> And the name should contain "aml_table" or "aml" rather than "ssdt" according to your check below. >> >> Ok, when I've some time I will do a new version moving the functionality >> to acpi_tb_verify_temp_table(). I will post a v2 with this change when >> it is ready. I will also just compare the headers then as suggested in >> other replies in this thread. > > If you wish, I can offer help here. > It actually won't take me much time to generate such a change. Great, if you can send me a new patch to test I'm more then willing to make time to test it. Thanks & Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Hans
I noticed that in AcpiTbInstallStandardTable(), there is a similar check:
for (i = 0; i < AcpiGbl_RootTableList.CurrentTableCount; ++i)
{
/*
* Check for a table match on the entire table length,
* not just the header.
*/
if (!AcpiTbCompareTables (&NewTableDesc, i))
{
continue;
}
Could you check and tell us why this cannot cover your use case?
Is your use case related to the LoadTable opcode?
Thanks in advance.
Best regards
Lv
> From: Hans de Goede [mailto:hdegoede@redhat.com]
> Subject: Re: [PATCH] ACPICA: Detect duplicate SSDT tables
>
> Hi,
>
> On 03-03-17 03:50, Zheng, Lv wrote:
> > Hi,
> >
> >> From: Hans de Goede [mailto:hdegoede@redhat.com]
> >> Subject: Re: [PATCH] ACPICA: Detect duplicate SSDT tables
> >>
> >> Hi,
> >>
> >> On 02-03-17 02:59, Zheng, Lv wrote:
> >>> Hi,
> >>>
> >>>> From: Hans de Goede [mailto:hdegoede@redhat.com]
> >>>> Subject: Re: [PATCH] ACPICA: Detect duplicate SSDT tables
> >>>>
> >>>> Hi,
> >>>>
> >>>> On 01-03-17 04:21, Zheng, Lv wrote:
> >>>>> Hi,
> >>>>>
> >>>>>> From: Hans de Goede [mailto:hdegoede@redhat.com]
> >>>>>> Subject: Re: [PATCH] ACPICA: Detect duplicate SSDT tables
> >>>>>>
> >>>>>> Hi,
> >>>>>>
> >>>>>> On 28-02-17 06:19, Zheng, Lv wrote:
> >>>>>>> Hi,
> >>>>>>>
> >>>>>>>> From: Hans de Goede [mailto:hdegoede@redhat.com]
> >>>>>>>> Subject: [PATCH] ACPICA: Detect duplicate SSDT tables
> >>>>>>>>
> >>>>>>>> Some machines have the exact (byte for byte) same SSDT tables multiple
> >>>>>>>> times in the root_table_list.
> >>
> >> <snip>
> >>
> >>>>>>>> Detect this and silently skip the duplicates
> >>>>>>>> rather then printing a scary looking set of errors.
> >>
> >> <snip>
> >>
> >>>>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >>>>>>>> ---
> >>>>>>>> drivers/acpi/acpica/tbxfload.c | 41 ++++++++++++++++++++++++++++++++++++++++-
> >>>>>>>> 1 file changed, 40 insertions(+), 1 deletion(-)
> >>>>>>>>
> >>>>>>>> diff --git a/drivers/acpi/acpica/tbxfload.c b/drivers/acpi/acpica/tbxfload.c
> >>>>>>>> index 82019c0..1971cd7 100644
> >>>>>>>> --- a/drivers/acpi/acpica/tbxfload.c
> >>>>>>>> +++ b/drivers/acpi/acpica/tbxfload.c
> >>>>>>>> @@ -125,6 +125,44 @@ ACPI_EXPORT_SYMBOL_INIT(acpi_load_tables)
> >>>>>>>>
> >>>>>>>> /*******************************************************************************
> >>>>>>>> *
> >>>>>>>> + * FUNCTION: acpi_tb_find_duplicate_ssdt
> >>>>>>>> + *
> >>>>>>>> + * PARAMETERS: table - validated acpi_table_desc of table to check
> >>>>>>>> + * index - index of table to find a duplicate of
> >>>>>>>> + *
> >>>>>>>> + * RETURN: TRUE if a duplicate is found, FALSE if not
> >>>>>>>> + *
> >>>>>>>> + * DESCRIPTION: Private helper function for acpi_tb_load_namespace to
> >>>>>>>> + * avoid trying to load duplicate ssdt tables
> >>>>>>>> + *
> >>>>>>>> + ******************************************************************************/
> >>>>>>>> +static u8 acpi_tb_find_duplicate_ssdt(struct acpi_table_desc *table, u32 index)
> >>>
> >>> If you really want to add this.
> >>> My suggestion is:
> >>> Invoke this function in acpi_tb_verify_temp_table().
> >>> This is the function we use to verify if a table is OK.
> >>> Otherwise it won't be installed to the global table list.
> >>>
> >>> And the name should contain "aml_table" or "aml" rather than "ssdt" according to your check below.
> >>
> >> Ok, when I've some time I will do a new version moving the functionality
> >> to acpi_tb_verify_temp_table(). I will post a v2 with this change when
> >> it is ready. I will also just compare the headers then as suggested in
> >> other replies in this thread.
> >
> > If you wish, I can offer help here.
> > It actually won't take me much time to generate such a change.
>
> Great, if you can send me a new patch to test I'm more then willing
> to make time to test it.
>
> Thanks & Regards,
>
> Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On linux, we can not verify ACPI tables before installing them into the
global table list in early stage due to the number of slot limitations for
early ioremap. This leaves a problem that we cannot detect duplicated
tables in early stage, causing unwanted error messages seen when the
duplicated tables are failed to be loaded [link #1].
This patchset as ACPICA PR [link #2] adds support to verify tables in
acpi_reallocate_root_tables() which is invoked after installing tables in
early stage, being ready for late ioremap and beofre loading table in late
stage. If duplicated tables are detected, acpi_reallocate_root_tables()
stops installing them to the final reallocated global table list.
In v1 patchset [link #3], problems can be seen for dynamic loading tables
which are allocated as virtuall allocated tables. Such tables are
incremented in the global table list as the sanity check done in install
step only compares the table addresses while the virtual allocated
addresses are not stable.
In v2 patchset, we follow original design discussion made in ACPICA devel
mailing list, implements deferred table verification so that duplicate
tables are still can be detected in install step rather than in load step.
Also we removed table signature check according to verified windows
behavior [link #4] and reported issues [link #5].
Link: http://www.spinics.net/lists/linux-acpi/msg72215.html [#1]
Link: https://github.com//acpica/acpica/pull/265 [#2]
Link: http://www.spinics.net/lists/linux-acpi/msg72589.html [#3]
Link: https://github.com//acpica/acpica/pull/121 [#4]
https://bugzilla.kernel.org/show_bug.cgi?id=118601 [#5]
Lv Zheng (5):
ACPICA: Tables: Cleanup table handler invokers
ACPICA: Tables: Do not validate signature for dynamic table load
ACPICA: Tables: Change table duplication check to be related to
acpi_gbl_verify_table_checksum
ACPICA: Tables: Combine checksum/duplication verification together
ACPICA: Tables: Add deferred table verification support
drivers/acpi/acpica/actables.h | 5 +-
drivers/acpi/acpica/tbdata.c | 222 ++++++++++++++++++++++++++++++++++++-----
drivers/acpi/acpica/tbinstal.c | 161 ++++--------------------------
drivers/acpi/acpica/tbxface.c | 36 ++++++-
drivers/acpi/acpica/tbxfload.c | 2 +-
drivers/acpi/bus.c | 3 -
drivers/acpi/tables.c | 4 +-
include/acpi/acpixf.h | 15 +--
include/acpi/actbl.h | 1 +
9 files changed, 266 insertions(+), 183 deletions(-)
On linux, we can not verify ACPI tables before installing them into the
global table list in early stage due to the number of slot limitations for
early ioremap. This leaves a problem that we cannot detect duplicated
tables in early stage, causing unwanted error messages seen when the
duplicated tables are failed to be loaded [link #1].
This patchset as ACPICA PR [link #2] adds support to verify tables in
acpi_reallocate_root_tables() which is invoked after installing tables in
early stage, being ready for late ioremap and beofre loading table in late
stage. If duplicated tables are detected, acpi_reallocate_root_tables()
stops installing them to the final reallocated global table list.
In v1 patchset [link #3], problems can be seen for dynamic loading tables
which are allocated as virtuall allocated tables. Such tables are
incremented in the global table list as the sanity check done in install
step only compares the table addresses while the virtual allocated
addresses are not stable.
In v2 patchset, we follow original design discussion made in ACPICA devel
mailing list, implements deferred table verification so that duplicate
tables are still can be detected in install step rather than in load step.
Also we removed table signature check according to verified windows
behavior [link #4] and reported issues [link #5].
In v3 patchset, one serious problem is detected around returning
AE_CTRL_TERMINATE for duplicate tables. The problem is detected by ASLTS.
Also it contains small improvements. ACPICA pull request [link #2] is
also updated.
Link: http://www.spinics.net/lists/linux-acpi/msg72215.html [#1]
Link: https://github.com//acpica/acpica/pull/265 [#2]
Link: http://www.spinics.net/lists/linux-acpi/msg72589.html [#3]
Link: https://github.com//acpica/acpica/pull/121 [#4]
https://bugzilla.kernel.org/show_bug.cgi?id=118601 [#5]
Lv Zheng (5):
ACPICA: Tables: Cleanup table handler invokers
ACPICA: Tables: Do not validate signature for dynamic table load
ACPICA: Tables: Change table duplication check to be related to
acpi_gbl_verify_table_checksum
ACPICA: Tables: Combine checksum/duplication verification together
ACPICA: Tables: Add deferred table verification support
drivers/acpi/acpica/actables.h | 5 +-
drivers/acpi/acpica/tbdata.c | 228 ++++++++++++++++++++++++++++++++++++-----
drivers/acpi/acpica/tbinstal.c | 161 ++++-------------------------
drivers/acpi/acpica/tbxface.c | 33 +++++-
drivers/acpi/acpica/tbxfload.c | 2 +-
drivers/acpi/bus.c | 3 -
drivers/acpi/tables.c | 4 +-
include/acpi/acpixf.h | 15 +--
include/acpi/actbl.h | 1 +
9 files changed, 270 insertions(+), 182 deletions(-)
Hi, On 16-05-17 09:13, Lv Zheng wrote: > On linux, we can not verify ACPI tables before installing them into the > global table list in early stage due to the number of slot limitations for > early ioremap. This leaves a problem that we cannot detect duplicated > tables in early stage, causing unwanted error messages seen when the > duplicated tables are failed to be loaded [link #1]. > > This patchset as ACPICA PR [link #2] adds support to verify tables in > acpi_reallocate_root_tables() which is invoked after installing tables in > early stage, being ready for late ioremap and beofre loading table in late > stage. If duplicated tables are detected, acpi_reallocate_root_tables() > stops installing them to the final reallocated global table list. > > In v1 patchset [link #3], problems can be seen for dynamic loading tables > which are allocated as virtuall allocated tables. Such tables are > incremented in the global table list as the sanity check done in install > step only compares the table addresses while the virtual allocated > addresses are not stable. > In v2 patchset, we follow original design discussion made in ACPICA devel > mailing list, implements deferred table verification so that duplicate > tables are still can be detected in install step rather than in load step. > Also we removed table signature check according to verified windows > behavior [link #4] and reported issues [link #5]. > > Link: http://www.spinics.net/lists/linux-acpi/msg72215.html [#1] > Link: https://github.com//acpica/acpica/pull/265 [#2] > Link: http://www.spinics.net/lists/linux-acpi/msg72589.html [#3] > Link: https://github.com//acpica/acpica/pull/121 [#4] > https://bugzilla.kernel.org/show_bug.cgi?id=118601 [#5] > > Lv Zheng (5): > ACPICA: Tables: Cleanup table handler invokers > ACPICA: Tables: Do not validate signature for dynamic table load > ACPICA: Tables: Change table duplication check to be related to > acpi_gbl_verify_table_checksum > ACPICA: Tables: Combine checksum/duplication verification together > ACPICA: Tables: Add deferred table verification support I've tested these patches and can confirm that they fix the ACPI errors on the system which started this all. Note I've tested "v2" of the patch-set, I was on the Cc for the cover letter for v3, but I did not receive the actual patches, if you want me test v3 please send me the patches (or point me to a git branch with them). Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
SGksIEhhbnMNCg0KPiBGcm9tOiBIYW5zIGRlIEdvZWRlIFttYWlsdG86aGRlZ29lZGVAcmVkaGF0 LmNvbV0NCj4gU3ViamVjdDogUmU6IFtSRkMgUEFUQ0ggdjIgMC81XSBBQ1BJQ0E6IFRhYmxlczog QWRkIGRlZmVycmVkIHZlcmlmaWNhdGlvbiBzdXBwb3J0DQo+IA0KPiBIaSwNCj4gDQo+IE9uIDE2 LTA1LTE3IDA5OjEzLCBMdiBaaGVuZyB3cm90ZToNCj4gPiBPbiBsaW51eCwgd2UgY2FuIG5vdCB2 ZXJpZnkgQUNQSSB0YWJsZXMgYmVmb3JlIGluc3RhbGxpbmcgdGhlbSBpbnRvIHRoZQ0KPiA+IGds b2JhbCB0YWJsZSBsaXN0IGluIGVhcmx5IHN0YWdlIGR1ZSB0byB0aGUgbnVtYmVyIG9mIHNsb3Qg bGltaXRhdGlvbnMgZm9yDQo+ID4gZWFybHkgaW9yZW1hcC4gVGhpcyBsZWF2ZXMgYSBwcm9ibGVt IHRoYXQgd2UgY2Fubm90IGRldGVjdCBkdXBsaWNhdGVkDQo+ID4gdGFibGVzIGluIGVhcmx5IHN0 YWdlLCBjYXVzaW5nIHVud2FudGVkIGVycm9yIG1lc3NhZ2VzIHNlZW4gd2hlbiB0aGUNCj4gPiBk dXBsaWNhdGVkIHRhYmxlcyBhcmUgZmFpbGVkIHRvIGJlIGxvYWRlZCBbbGluayAjMV0uDQo+ID4N Cj4gPiBUaGlzIHBhdGNoc2V0IGFzIEFDUElDQSBQUiBbbGluayAjMl0gYWRkcyBzdXBwb3J0IHRv IHZlcmlmeSB0YWJsZXMgaW4NCj4gPiBhY3BpX3JlYWxsb2NhdGVfcm9vdF90YWJsZXMoKSB3aGlj aCBpcyBpbnZva2VkIGFmdGVyIGluc3RhbGxpbmcgdGFibGVzIGluDQo+ID4gZWFybHkgc3RhZ2Us IGJlaW5nIHJlYWR5IGZvciBsYXRlIGlvcmVtYXAgYW5kIGJlb2ZyZSBsb2FkaW5nIHRhYmxlIGlu IGxhdGUNCj4gPiBzdGFnZS4gSWYgZHVwbGljYXRlZCB0YWJsZXMgYXJlIGRldGVjdGVkLCBhY3Bp X3JlYWxsb2NhdGVfcm9vdF90YWJsZXMoKQ0KPiA+IHN0b3BzIGluc3RhbGxpbmcgdGhlbSB0byB0 aGUgZmluYWwgcmVhbGxvY2F0ZWQgZ2xvYmFsIHRhYmxlIGxpc3QuDQo+ID4NCj4gPiBJbiB2MSBw YXRjaHNldCBbbGluayAjM10sIHByb2JsZW1zIGNhbiBiZSBzZWVuIGZvciBkeW5hbWljIGxvYWRp bmcgdGFibGVzDQo+ID4gd2hpY2ggYXJlIGFsbG9jYXRlZCBhcyB2aXJ0dWFsbCBhbGxvY2F0ZWQg dGFibGVzLiBTdWNoIHRhYmxlcyBhcmUNCj4gPiBpbmNyZW1lbnRlZCBpbiB0aGUgZ2xvYmFsIHRh YmxlIGxpc3QgYXMgdGhlIHNhbml0eSBjaGVjayBkb25lIGluIGluc3RhbGwNCj4gPiBzdGVwIG9u bHkgY29tcGFyZXMgdGhlIHRhYmxlIGFkZHJlc3NlcyB3aGlsZSB0aGUgdmlydHVhbCBhbGxvY2F0 ZWQNCj4gPiBhZGRyZXNzZXMgYXJlIG5vdCBzdGFibGUuDQo+ID4gSW4gdjIgcGF0Y2hzZXQsIHdl IGZvbGxvdyBvcmlnaW5hbCBkZXNpZ24gZGlzY3Vzc2lvbiBtYWRlIGluIEFDUElDQSBkZXZlbA0K PiA+IG1haWxpbmcgbGlzdCwgaW1wbGVtZW50cyBkZWZlcnJlZCB0YWJsZSB2ZXJpZmljYXRpb24g c28gdGhhdCBkdXBsaWNhdGUNCj4gPiB0YWJsZXMgYXJlIHN0aWxsIGNhbiBiZSBkZXRlY3RlZCBp biBpbnN0YWxsIHN0ZXAgcmF0aGVyIHRoYW4gaW4gbG9hZCBzdGVwLg0KPiA+IEFsc28gd2UgcmVt b3ZlZCB0YWJsZSBzaWduYXR1cmUgY2hlY2sgYWNjb3JkaW5nIHRvIHZlcmlmaWVkIHdpbmRvd3MN Cj4gPiBiZWhhdmlvciBbbGluayAjNF0gYW5kIHJlcG9ydGVkIGlzc3VlcyBbbGluayAjNV0uDQo+ ID4NCj4gPiBMaW5rOiBodHRwOi8vd3d3LnNwaW5pY3MubmV0L2xpc3RzL2xpbnV4LWFjcGkvbXNn NzIyMTUuaHRtbCBbIzFdDQo+ID4gTGluazogaHR0cHM6Ly9naXRodWIuY29tLy9hY3BpY2EvYWNw aWNhL3B1bGwvMjY1ICAgICAgICAgICAgWyMyXQ0KPiA+IExpbms6IGh0dHA6Ly93d3cuc3Bpbmlj cy5uZXQvbGlzdHMvbGludXgtYWNwaS9tc2c3MjU4OS5odG1sIFsjM10NCj4gPiBMaW5rOiBodHRw czovL2dpdGh1Yi5jb20vL2FjcGljYS9hY3BpY2EvcHVsbC8xMjEgICAgICAgICAgICBbIzRdDQo+ ID4gICAgICAgIGh0dHBzOi8vYnVnemlsbGEua2VybmVsLm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTE4 NjAxICAgIFsjNV0NCj4gPg0KPiA+IEx2IFpoZW5nICg1KToNCj4gPiAgICBBQ1BJQ0E6IFRhYmxl czogQ2xlYW51cCB0YWJsZSBoYW5kbGVyIGludm9rZXJzDQo+ID4gICAgQUNQSUNBOiBUYWJsZXM6 IERvIG5vdCB2YWxpZGF0ZSBzaWduYXR1cmUgZm9yIGR5bmFtaWMgdGFibGUgbG9hZA0KPiA+ICAg IEFDUElDQTogVGFibGVzOiBDaGFuZ2UgdGFibGUgZHVwbGljYXRpb24gY2hlY2sgdG8gYmUgcmVs YXRlZCB0bw0KPiA+ICAgICAgYWNwaV9nYmxfdmVyaWZ5X3RhYmxlX2NoZWNrc3VtDQo+ID4gICAg QUNQSUNBOiBUYWJsZXM6IENvbWJpbmUgY2hlY2tzdW0vZHVwbGljYXRpb24gdmVyaWZpY2F0aW9u IHRvZ2V0aGVyDQo+ID4gICAgQUNQSUNBOiBUYWJsZXM6IEFkZCBkZWZlcnJlZCB0YWJsZSB2ZXJp ZmljYXRpb24gc3VwcG9ydA0KPiANCj4gSSd2ZSB0ZXN0ZWQgdGhlc2UgcGF0Y2hlcyBhbmQgY2Fu IGNvbmZpcm0gdGhhdCB0aGV5IGZpeCB0aGUgQUNQSQ0KPiBlcnJvcnMgb24gdGhlIHN5c3RlbSB3 aGljaCBzdGFydGVkIHRoaXMgYWxsLg0KPiANCj4gTm90ZSBJJ3ZlIHRlc3RlZCAidjIiIG9mIHRo ZSBwYXRjaC1zZXQsIEkgd2FzIG9uIHRoZSBDYyBmb3IgdGhlDQo+IGNvdmVyIGxldHRlciBmb3Ig djMsIGJ1dCBJIGRpZCBub3QgcmVjZWl2ZSB0aGUgYWN0dWFsIHBhdGNoZXMsDQo+IGlmIHlvdSB3 YW50IG1lIHRlc3QgdjMgcGxlYXNlIHNlbmQgbWUgdGhlIHBhdGNoZXMgKG9yIHBvaW50DQo+IG1l IHRvIGEgZ2l0IGJyYW5jaCB3aXRoIHRoZW0pLg0KDQpXaGF0IEkgYWRkZWQgaXM6DQoJCSp0YWJs ZV9pbmRleCA9IGk7DQpJbiBhY3BpX3RiX2luc3RhbGxfc3RhbmRhcmRfdGFibGUoKS4NCldpdGhv dXQgdGhpcyBsaW5lLCBMb2FkIGEgdGFibGUgMm5kIHRpbWUgYWZ0ZXIgdGhlIDFzdCB0aW1lIExv YWQvVW5sb2FkIHdpbGwgZmFpbC4NCk90aGVyIGNoYW5nZXMgYXJlIG5vdCBmdW5jdGlvbmFsLiBJ J20gc29ycnkgZm9yIHRoYXQgbWlzdGFrZS4NCg0KVGVzdGluZyB2MiBjYW4gYmUgc3VmZmljaWVu dCBhcyB3aGF0IHYzIGltcHJvdmVzIGlzIG5vdCBzdHJpY3RseSByZWxhdGVkIHRvIHlvdXIgaXNz dWUuDQpBbmQgd2UgY2FuIGVuc3VyZSB0aGUgdjMgaW1wcm92ZW1lbnQncyBxdWFsaXR5IGxvY2Fs bHkgd2hpbGUgZm9yIHlvdXIgaXNzdWUgSSBjYW5ub3QuDQoNClNvIGl0J3MgT0sgaWYgeW91IGRv bid0IGRvIGFueSBmdXJ0aGVyIHRlc3RpbmcsIGJ1dCBpZiB5b3Ugd2FudCB0byB0cnk6DQpZb3Ug Y2FuIHJlYWNoIHRoZSBjb2RlIHZpYSB0aGUgZm9sbG93aW5nIGdpdCByZXBvc2l0b3J5Og0KaHR0 cHM6Ly9naXRodWIuY29tL3pldGFsb2cvbGludXgvDQpUaGV5IGFyZSB0b3AgNSBjb21taXRzIGlu IGFjcGljYS10YWJsZXMyIGJyYW5jaDoNCmh0dHBzOi8vZ2l0aHViLmNvbS96ZXRhbG9nL2xpbnV4 L2NvbW1pdHMvYWNwaWNhLXRhYmxlczINCg0KIyBnaXQgcmVtb3RlIGFkZCBsaW51eC1hY3BpY2Eg aHR0cHM6Ly9naXRodWIuY29tL3pldGFsb2cvbGludXgvDQojIGdpdCBmZXRjaCBsaW51eC1hY3Bp Y2ENCiMgZ2l0IGNoZWNrb3V0IGFjcGljYS10YWJsZXMyDQoNClRoYW5rcyBhbmQgYmVzdCByZWdh cmRzDQpMdg0K -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 19-05-17 09:59, Zheng, Lv wrote: > Hi, Hans > >> From: Hans de Goede [mailto:hdegoede@redhat.com] >> Subject: Re: [RFC PATCH v2 0/5] ACPICA: Tables: Add deferred verification support >> >> Hi, >> >> On 16-05-17 09:13, Lv Zheng wrote: >>> On linux, we can not verify ACPI tables before installing them into the >>> global table list in early stage due to the number of slot limitations for >>> early ioremap. This leaves a problem that we cannot detect duplicated >>> tables in early stage, causing unwanted error messages seen when the >>> duplicated tables are failed to be loaded [link #1]. >>> >>> This patchset as ACPICA PR [link #2] adds support to verify tables in >>> acpi_reallocate_root_tables() which is invoked after installing tables in >>> early stage, being ready for late ioremap and beofre loading table in late >>> stage. If duplicated tables are detected, acpi_reallocate_root_tables() >>> stops installing them to the final reallocated global table list. >>> >>> In v1 patchset [link #3], problems can be seen for dynamic loading tables >>> which are allocated as virtuall allocated tables. Such tables are >>> incremented in the global table list as the sanity check done in install >>> step only compares the table addresses while the virtual allocated >>> addresses are not stable. >>> In v2 patchset, we follow original design discussion made in ACPICA devel >>> mailing list, implements deferred table verification so that duplicate >>> tables are still can be detected in install step rather than in load step. >>> Also we removed table signature check according to verified windows >>> behavior [link #4] and reported issues [link #5]. >>> >>> Link: http://www.spinics.net/lists/linux-acpi/msg72215.html [#1] >>> Link: https://github.com//acpica/acpica/pull/265 [#2] >>> Link: http://www.spinics.net/lists/linux-acpi/msg72589.html [#3] >>> Link: https://github.com//acpica/acpica/pull/121 [#4] >>> https://bugzilla.kernel.org/show_bug.cgi?id=118601 [#5] >>> >>> Lv Zheng (5): >>> ACPICA: Tables: Cleanup table handler invokers >>> ACPICA: Tables: Do not validate signature for dynamic table load >>> ACPICA: Tables: Change table duplication check to be related to >>> acpi_gbl_verify_table_checksum >>> ACPICA: Tables: Combine checksum/duplication verification together >>> ACPICA: Tables: Add deferred table verification support >> >> I've tested these patches and can confirm that they fix the ACPI >> errors on the system which started this all. >> >> Note I've tested "v2" of the patch-set, I was on the Cc for the >> cover letter for v3, but I did not receive the actual patches, >> if you want me test v3 please send me the patches (or point >> me to a git branch with them). > > What I added is: > *table_index = i; > In acpi_tb_install_standard_table(). > Without this line, Load a table 2nd time after the 1st time Load/Unload will fail. > Other changes are not functional. I'm sorry for that mistake. > > Testing v2 can be sufficient as what v3 improves is not strictly related to your issue. > And we can ensure the v3 improvement's quality locally while for your issue I cannot. > > So it's OK if you don't do any further testing, but if you want to try: > You can reach the code via the following git repository: > https://github.com/zetalog/linux/ > They are top 5 commits in acpica-tables2 branch: > https://github.com/zetalog/linux/commits/acpica-tables2 > > # git remote add linux-acpica https://github.com/zetalog/linux/ > # git fetch linux-acpica > # git checkout acpica-tables2 Thank you for the info. I've added the missing *table_index = i; statement to my local tree. I will let you know if I hit any issues with this patch-set. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" 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/drivers/acpi/acpica/tbxfload.c b/drivers/acpi/acpica/tbxfload.c index 82019c0..1971cd7 100644 --- a/drivers/acpi/acpica/tbxfload.c +++ b/drivers/acpi/acpica/tbxfload.c @@ -125,6 +125,44 @@ ACPI_EXPORT_SYMBOL_INIT(acpi_load_tables) /******************************************************************************* * + * FUNCTION: acpi_tb_find_duplicate_ssdt + * + * PARAMETERS: table - validated acpi_table_desc of table to check + * index - index of table to find a duplicate of + * + * RETURN: TRUE if a duplicate is found, FALSE if not + * + * DESCRIPTION: Private helper function for acpi_tb_load_namespace to + * avoid trying to load duplicate ssdt tables + * + ******************************************************************************/ +static u8 acpi_tb_find_duplicate_ssdt(struct acpi_table_desc *table, u32 index) +{ + struct acpi_table_desc *dup; + u32 i; + + for (i = 0; i < index; ++i) { + dup = &acpi_gbl_root_table_list.tables[i]; + + if (!acpi_gbl_root_table_list.tables[i].address || + (!ACPI_COMPARE_NAME(dup->signature.ascii, ACPI_SIG_SSDT) + && !ACPI_COMPARE_NAME(dup->signature.ascii, + ACPI_SIG_PSDT) + && !ACPI_COMPARE_NAME(dup->signature.ascii, + ACPI_SIG_OSDT)) + || ACPI_FAILURE(acpi_tb_validate_table(dup)) + || dup->length != table->length) { + continue; + } + + if (memcmp(dup->pointer, table->pointer, table->length) == 0) + return TRUE; + } + return FALSE; +} + +/******************************************************************************* + * * FUNCTION: acpi_tb_load_namespace * * PARAMETERS: None @@ -212,7 +250,8 @@ acpi_status acpi_tb_load_namespace(void) ACPI_SIG_PSDT) && !ACPI_COMPARE_NAME(table->signature.ascii, ACPI_SIG_OSDT)) - || ACPI_FAILURE(acpi_tb_validate_table(table))) { + || ACPI_FAILURE(acpi_tb_validate_table(table)) + || acpi_tb_find_duplicate_ssdt(table, i)) { continue; }
Some machines have the exact (byte for byte) same SSDT tables multiple times in the root_table_list. Detect this and silently skip the duplicates rather then printing a scary looking set of errors. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/acpi/acpica/tbxfload.c | 41 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-)