ACPICA: Detect duplicate SSDT tables
Subsystems (2)
PATCH 1/1 ACPICA: Detect duplicate SSDT tables
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.
--- drivers/acpi/acpica/tbxfload.c | 41 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-)
drivers/acpi/acpica/tbxfload.c +40 -1
@@ -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; }
Signature · 1 line
2.9.3
Zheng
Hi,
Quoted 5 lines
From: Hans de Goede [mailto:hdegoede(a)redhat.com]
Subject: [PATCH] ACPICA: Detect duplicate SSDT tablesSome machines have the exact (byte for byte) same SSDT tables multiple times in the
root_table_list.
Could you give a machine list here?
Quoted 2 lines
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
Quoted 67 lines
Signed-off-by Hans de Goede <hdegoede(a)redhat.com>--- drivers/acpi/acpica/tbxfload.c | 41 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-)drivers/acpi/acpica/tbxfload.c +40
-2@@ -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
Hi,
Quoted 9 lines · Lv
Hi,
Quoted 5 lines
From: Hans de Goede [mailto:[email protected]]
Subject: [PATCH] ACPICA: Detect duplicate SSDT tablesSome 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:
I thought I was seeing it on more machines, but those have different apci table loading errors...
Quoted 4 lines
Quoted 2 lines
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.
Quoted 1 line
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.
Quoted 1 line
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
Quoted 72 lines
Thanks
LvQuoted 67 lines
Signed-off-by Hans de Goede <[email protected]>--- drivers/acpi/acpica/tbxfload.c | 41 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-)drivers/acpi/acpica/tbxfload.c +40
-2@@ -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
Moore
Does the machine or machines work properly with Windows? This is always one of our early questions. Bob
Quoted 164 lines
-----Original Message-----
From: Hans de Goede [mailto:hdegoede(a)redhat.com]
Sent: Tuesday, February 28, 2017 6:32 AM
To: Zheng, Lv <lv.zheng(a)intel.com>; Rafael J . Wysocki <rjw(a)rjwysocki.net>; Len Brown <lenb(a)kernel.org>; Moore, Robert <robert.moore(a)intel.com>
Cc: linux-acpi(a)vger.kernel.org; devel(a)acpica.org
Subject: Re: [PATCH] ACPICA: Detect duplicate SSDT tablesHi,
Quoted 9 lines · Lv
Hi,
Quoted 5 lines
From: Hans de Goede [mailto:hdegoede(a)redhat.com]
Subject: [PATCH] ACPICA: Detect duplicate SSDT tablesSome 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:
I thought I was seeing it on more machines, but those have different apci table loading errors...
Quoted 4 lines
Quoted 2 lines
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.
Quoted 1 line
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.
Quoted 1 line
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_EXCEPTIONcalls 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
Quoted 11 lines
Thanks
LvQuoted 7 lines
Signed-off-by Hans de Goede <hdegoede(a)redhat.com>--- drivers/acpi/acpica/tbxfload.c | 41 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-)Quoted 7 lines
drivers/acpi/acpica/tbxfload.c ACPI_EXPORT_SYMBOL_INIT(acpi_load_tables) · line 126
Quoted 7 lines
(quoted diff) +0
-0
b/drivers/acpi/acpica/tbxfload.cindex 82019c0..1971cd7 100644drivers/acpi/acpica/tbxfload.c +0
-0@@ -125,6 +125,44 @@ ACPI_EXPORT_SYMBOL_INIT(acpi_load_tables)/***********************************************************************
********Quoted 4 lines
drivers/acpi/acpica/tbxfload.c ACPI_EXPORT_SYMBOL_INIT(acpi_load_tables)
Quoted 4 lines
drivers/acpi/acpica/tbxfload.c +3
-0* +* FUNCTION: acpi_tb_find_duplicate_ssdt +* +* PARAMETERS: table - validated acpi_table_desc of table to check
Quoted 1 line
drivers/acpi/acpica/tbxfload.c ACPI_EXPORT_SYMBOL_INIT(acpi_load_tables) · line 131
Quoted 1 line
drivers/acpi/acpica/tbxfload.c +1
-0+* index - index of table to find a duplicate of
Quoted 4 lines
drivers/acpi/acpica/tbxfload.c ACPI_EXPORT_SYMBOL_INIT(acpi_load_tables)
Quoted 4 lines
drivers/acpi/acpica/tbxfload.c +4
-0+* +* RETURN: TRUE if a duplicate is found, FALSE if not +* +* DESCRIPTION: Private helper function for acpi_tb_load_namespace to
Quoted 48 lines
Quoted 47 lines
drivers/acpi/acpica/tbxfload.c +34
-2+* 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
Hi,
Quoted 1 line · Robert
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
Quoted 167 lines
Bob
Quoted 164 lines
-----Original Message-----
From: Hans de Goede [mailto:[email protected]]
Sent: Tuesday, February 28, 2017 6:32 AM
To: Zheng, Lv <[email protected]>; Rafael J . Wysocki <[email protected]>; Len Brown <[email protected]>; Moore, Robert <[email protected]>
Cc: [email protected]; [email protected]
Subject: Re: [PATCH] ACPICA: Detect duplicate SSDT tablesHi,
Quoted 9 lines · Lv
Hi,
Quoted 5 lines
From: Hans de Goede [mailto:[email protected]]
Subject: [PATCH] ACPICA: Detect duplicate SSDT tablesSome 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:
I thought I was seeing it on more machines, but those have different apci table loading errors...
Quoted 4 lines
Quoted 2 lines
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.
Quoted 1 line
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.
Quoted 1 line
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_EXCEPTIONcalls 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
Quoted 11 lines
Thanks
LvQuoted 7 lines
Signed-off-by Hans de Goede <[email protected]>--- drivers/acpi/acpica/tbxfload.c | 41 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-)Quoted 7 lines
drivers/acpi/acpica/tbxfload.c ACPI_EXPORT_SYMBOL_INIT(acpi_load_tables) · line 126
Quoted 7 lines
(quoted diff) +0
-0
b/drivers/acpi/acpica/tbxfload.cindex 82019c0..1971cd7 100644drivers/acpi/acpica/tbxfload.c +0
-0@@ -125,6 +125,44 @@ ACPI_EXPORT_SYMBOL_INIT(acpi_load_tables)/***********************************************************************
********Quoted 4 lines
drivers/acpi/acpica/tbxfload.c ACPI_EXPORT_SYMBOL_INIT(acpi_load_tables)
Quoted 4 lines
drivers/acpi/acpica/tbxfload.c +3
-0* +* FUNCTION: acpi_tb_find_duplicate_ssdt +* +* PARAMETERS: table - validated acpi_table_desc of table to check
Quoted 1 line
drivers/acpi/acpica/tbxfload.c ACPI_EXPORT_SYMBOL_INIT(acpi_load_tables) · line 131
Quoted 1 line
drivers/acpi/acpica/tbxfload.c +1
-0+* index - index of table to find a duplicate of
Quoted 4 lines
drivers/acpi/acpica/tbxfload.c ACPI_EXPORT_SYMBOL_INIT(acpi_load_tables)
Quoted 4 lines
drivers/acpi/acpica/tbxfload.c +4
-0+* +* RETURN: TRUE if a duplicate is found, FALSE if not +* +* DESCRIPTION: Private helper function for acpi_tb_load_namespace to
Quoted 48 lines
Quoted 47 lines
drivers/acpi/acpica/tbxfload.c +34
-2+* 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
Moore
Quoted 9 lines
-----Original Message-----
From: Hans de Goede [mailto:hdegoede(a)redhat.com]
Sent: Tuesday, February 28, 2017 3:45 PM
To: Moore, Robert; Zheng, Lv; Rafael J . Wysocki; Len Brown
Cc: linux-acpi(a)vger.kernel.org; devel(a)acpica.org
Subject: Re: [PATCH] ACPICA: Detect duplicate SSDT tables
Quoted 9 lines
Hi,
Quoted 1 line · Robert
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.
Quoted 178 lines
Regards,
Hans
Quoted 46 lines
Bob
Quoted 43 lines
-----Original Message-----
From: Hans de Goede [mailto:hdegoede(a)redhat.com]
Sent: Tuesday, February 28, 2017 6:32 AM
To: Zheng, Lv <lv.zheng(a)intel.com>; Rafael J . Wysocki <rjw(a)rjwysocki.net>; Len Brown <lenb(a)kernel.org>; Moore, Robert <robert.moore(a)intel.com>
Cc: linux-acpi(a)vger.kernel.org; devel(a)acpica.org
Subject: Re: [PATCH] ACPICA: Detect duplicate SSDT tablesHi,
Quoted 9 lines · Lv
Hi,
Quoted 5 lines
From: Hans de Goede [mailto:hdegoede(a)redhat.com]
Subject: [PATCH] ACPICA: Detect duplicate SSDT tablesSome 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:
I thought I was seeing it on more machines, but those have different apci table loading errors...
Quoted 4 lines
Quoted 2 lines
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"
Quoted 74 lines
Quoted 74 lines
product requirement.
Quoted 1 line
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.
Quoted 1 line
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 aspossible */
How about setting a global flag while loading these tables and making
ACPI_EXCEPTIONcalls 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
Quoted 11 lines
Thanks
LvQuoted 7 lines
Signed-off-by Hans de Goede <hdegoede(a)redhat.com>--- drivers/acpi/acpica/tbxfload.c | 41 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-)Quoted 7 lines
drivers/acpi/acpica/tbxfload.c ACPI_EXPORT_SYMBOL_INIT(acpi_load_tables) · line 126
Quoted 7 lines
(quoted diff) +0
-0
b/drivers/acpi/acpica/tbxfload.cindex 82019c0..1971cd7 100644drivers/acpi/acpica/tbxfload.c +0
-0@@ -125,6 +125,44 @@ ACPI_EXPORT_SYMBOL_INIT(acpi_load_tables)/********************************************************************
***
********Quoted 4 lines
drivers/acpi/acpica/tbxfload.c ACPI_EXPORT_SYMBOL_INIT(acpi_load_tables)
Quoted 4 lines
drivers/acpi/acpica/tbxfload.c +3
-0* +* FUNCTION: acpi_tb_find_duplicate_ssdt +* +* PARAMETERS: table - validated acpi_table_desc of table to check
Quoted 1 line
drivers/acpi/acpica/tbxfload.c ACPI_EXPORT_SYMBOL_INIT(acpi_load_tables) · line 131
Quoted 1 line
drivers/acpi/acpica/tbxfload.c +1
-0+* index - index of table to find a duplicate of
Quoted 4 lines
drivers/acpi/acpica/tbxfload.c ACPI_EXPORT_SYMBOL_INIT(acpi_load_tables)
Quoted 4 lines
drivers/acpi/acpica/tbxfload.c +4
-0+* +* RETURN: TRUE if a duplicate is found, FALSE if not +* +* DESCRIPTION: Private helper function for acpi_tb_load_namespace to
Quoted 14 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
drivers/acpi/acpica/tbxfload.c +6
-0+* avoid trying to load duplicate ssdt tables +* + +****************************************************************** +** **********/ static u8 acpi_tb_find_duplicate_ssdt (struct +acpi_table_desc *table,u32 index) {Quoted 8 lines
Quoted 8 lines
drivers/acpi/acpica/tbxfload.c ACPI_EXPORT_SYMBOL_INIT(acpi_load_tables)
Quoted 8 lines
drivers/acpi/acpica/tbxfload.c +8
-0+ 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)Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
drivers/acpi/acpica/tbxfload.c ACPI_EXPORT_SYMBOL_INIT(acpi_load_tables)
Quoted 10 lines
drivers/acpi/acpica/tbxfload.c +10
-0+ && !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)
Quoted 25 lines
Quoted 25 lines
Quoted 25 lines
Quoted 24 lines
drivers/acpi/acpica/tbxfload.c +11
-2+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
Zheng
Hi,
Quoted 14 lines
From: Hans de Goede [mailto:hdegoede(a)redhat.com]
Subject: Re: [PATCH] ACPICA: Detect duplicate SSDT tablesHi,
Quoted 8 lines · Lv
Hi,
Quoted 5 lines
From: Hans de Goede [mailto:hdegoede(a)redhat.com]
Subject: [PATCH] ACPICA: Detect duplicate SSDT tablesSome machines have the exact (byte for byte) same SSDT tables multiple times in the
root_table_list.
Quoted 8 lines
Quoted 1 line
Could you give a machine list here?
Currently I'm seeing this on a GPD win machine:
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.
Quoted 17 lines
Quoted 5 lines
Quoted 3 lines
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.
Quoted 6 lines
Quoted 1 line
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.
Quoted 15 lines
Quoted 2 lines
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
Quoted 8 lines
ACPI_EXCEPTIONcalls 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
Quoted 80 lines
Regards,
Hans
Quoted 72 lines
Thanks
LvQuoted 67 lines
Signed-off-by Hans de Goede <hdegoede(a)redhat.com>--- drivers/acpi/acpica/tbxfload.c | 41 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-)drivers/acpi/acpica/tbxfload.c +40
-2@@ -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
Hi,
Quoted 32 lines · Lv
Hi,
Quoted 14 lines
From: Hans de Goede [mailto:[email protected]]
Subject: Re: [PATCH] ACPICA: Detect duplicate SSDT tablesHi,
Quoted 8 lines · Lv
Hi,
Quoted 5 lines
From: Hans de Goede [mailto:[email protected]]
Subject: [PATCH] ACPICA: Detect duplicate SSDT tablesSome machines have the exact (byte for byte) same SSDT tables multiple times in the
root_table_list.Quoted 8 lines
Quoted 1 line
Could you give a machine list here?
Currently I'm seeing this on a GPD win machine:
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.
Quoted 2 lines
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.
Quoted 21 lines
So I don't think this commit goes the right direction on the right track.
Quoted 19 lines
Quoted 5 lines
Quoted 3 lines
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.
Quoted 1 line
And should we add non-costless steps just in order to reduce errors,
Quoted 7 lines
Quoted 4 lines
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.
Quoted 17 lines
Quoted 17 lines
Quoted 1 line
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_EXCEPTIONcalls log the exceptions with a log level of warning as well as turning the final:
Quoted 7 lines
Quoted 5 lines
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()intopr_warn()in theprintk.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.
Quoted 3 lines
ACPI_EXCEPTIONhere actually means blue screen in Windows.
Maybe it's correct, maybe not.
Linux doesn't run into panic inACPI_EXCEPTIIONjust 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
Quoted 67 lines
Quoted 67 lines
Quoted 67 lines
Quoted 66 lines
Signed-off-by Hans de Goede <[email protected]>--- drivers/acpi/acpica/tbxfload.c | 41 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-)drivers/acpi/acpica/tbxfload.c +40
-2@@ -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
Moore
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)
Quoted 234 lines
-----Original Message-----
From: Hans de Goede [mailto:hdegoede(a)redhat.com]
Sent: Wednesday, March 1, 2017 1:19 AM
To: Zheng, Lv <lv.zheng(a)intel.com>; Rafael J . Wysocki <rjw(a)rjwysocki.net>; Len Brown <lenb(a)kernel.org>; Moore, Robert <robert.moore(a)intel.com>
Cc: linux-acpi(a)vger.kernel.org; devel(a)acpica.org
Subject: Re: [PATCH] ACPICA: Detect duplicate SSDT tablesHi,
Quoted 25 lines · Lv
Hi,
Quoted 14 lines
From: Hans de Goede [mailto:hdegoede(a)redhat.com]
Subject: Re: [PATCH] ACPICA: Detect duplicate SSDT tablesHi,
Quoted 8 lines · Lv
Hi,
Quoted 5 lines
From: Hans de Goede [mailto:hdegoede(a)redhat.com]
Subject: [PATCH] ACPICA: Detect duplicate SSDT tablesSome machines have the exact (byte for byte) same SSDT tables multiple times in the
root_table_list.Quoted 8 lines
Quoted 1 line
Could you give a machine list here?
Currently I'm seeing this on a GPD win machine:
I thought I was seeing it on more machines, but those have different apci table loading errors...
Quoted 8 lines
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.
Quoted 1 line
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.Quoted 1 line
So I don't think this commit goes the right direction on the right
track.
Quoted 18 lines
Quoted 18 lines
Quoted 6 lines
Quoted 4 lines
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.
Quoted 8 lines
Quoted 6 lines
Quoted 1 line
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.
Quoted 1 line
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.
Quoted 1 line
Quoted 1 line
Quoted 1 line
while the errors are on the contrary useful (in1dicating platform
issues)?
Quoted 17 lines
Quoted 17 lines
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 aspossible */
How about setting a global flag while loading these tables and making
ACPI_EXCEPTIONcalls log the exceptions with a log level of warning as well as turning the final:Quoted 7 lines
Quoted 5 lines
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()intopr_warn()inthe
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_EXCEPTIONcalls while doing this asACPI_WARNINGcalls.Quoted 3 lines
ACPI_EXCEPTIONhere actually means blue screen in Windows. Maybe it's correct, maybe not.
Linux doesn't run into panic inACPI_EXCEPTIIONjust because LinuxACPI 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
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Signed-off-by Hans de Goede <hdegoede(a)redhat.com>--- drivers/acpi/acpica/tbxfload.c | 41 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-)Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
drivers/acpi/acpica/tbxfload.c ACPI_EXPORT_SYMBOL_INIT(acpi_load_tables) · line 126
Quoted 7 lines
(quoted diff) +0
-0
b/drivers/acpi/acpica/tbxfload.cindex 82019c0..1971cd7 100644drivers/acpi/acpica/tbxfload.c +0
-0@@ -125,6 +125,44 @@ ACPI_EXPORT_SYMBOL_INIT(acpi_load_tables)/***********************************************************************
********Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
drivers/acpi/acpica/tbxfload.c ACPI_EXPORT_SYMBOL_INIT(acpi_load_tables)
Quoted 4 lines
drivers/acpi/acpica/tbxfload.c +3
-0* +* FUNCTION: acpi_tb_find_duplicate_ssdt +* +* PARAMETERS: table - validated acpi_table_desc of table to check
Quoted 1 line
Quoted 1 line
Quoted 1 line
drivers/acpi/acpica/tbxfload.c ACPI_EXPORT_SYMBOL_INIT(acpi_load_tables) · line 131
Quoted 1 line
drivers/acpi/acpica/tbxfload.c +1
-0+* index - index of table to find a duplicate of
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
drivers/acpi/acpica/tbxfload.c ACPI_EXPORT_SYMBOL_INIT(acpi_load_tables)
Quoted 4 lines
drivers/acpi/acpica/tbxfload.c +4
-0+* +* RETURN: TRUE if a duplicate is found, FALSE if not +* +* DESCRIPTION: Private helper function for acpi_tb_load_namespace to
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
drivers/acpi/acpica/tbxfload.c +6
-0+* avoid trying to load duplicate ssdt tables +* + +****************************************************************** +************/ static u8 acpi_tb_find_duplicate_ssdt (struct +acpi_table_desc *table,u32 index) {Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
drivers/acpi/acpica/tbxfload.c ACPI_EXPORT_SYMBOL_INIT(acpi_load_tables)
Quoted 8 lines
drivers/acpi/acpica/tbxfload.c +8
-0+ 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)Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
drivers/acpi/acpica/tbxfload.c ACPI_EXPORT_SYMBOL_INIT(acpi_load_tables)
Quoted 10 lines
drivers/acpi/acpica/tbxfload.c +10
-0+ && !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)
Quoted 24 lines
Quoted 24 lines
Quoted 24 lines
Quoted 23 lines
drivers/acpi/acpica/tbxfload.c +10
-2+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
Rafael J. Wysocki
maintainerQuoted 20 lines · Robert
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
0x0000000000492954000024 (v02 Intel )
ACPI: XSDT0x000000000068C9380000B4 (v00 Intel AcpiExec 00001001 INTL 20170224)
ACPI: FACP0x0000000000492978000114 (v05 Intel AcpiExec 00001001 INTL 20170224)
ACPI: DSDT0x0000000000690048022FD6 (v02 LENOVO CB-01 00000001 ACPI 00040000)
ACPI: FACS0x0000000000492AB0000040
ACPI: SSDT0x00000000006B30280004B7 (v02 LENOVO CB-01 00000001 ACPI 00040000)
ACPI: SSDT0x000000000068D940000B49 (v02 LENOVO CB-01 00000001 ACPI 00040000)
ACPI: SSDT0x00000000006B34E80004A3 (v02 LENOVO CB-01 00000001 ACPI 00040000)
ACPI: SSDT0x00000000006B3998000E58 (v02 LENOVO CB-01 00000001 ACPI 00040000)
ACPI: SSDT0x00000000006B47F800037F (v02 PmRef Cpu0Cst 00003001 INTL 20121220)
ACPI: SSDT0x000000000068E498000660 (v02 PmRef Cpu0Ist 00003000 INTL 20121220)
ACPI: SSDT0x00000000006B4B800005AA (v02 PmRef ApIst 00003000 INTL 20121220)
ACPI: SSDT0x000000000068EB00000119 (v02 PmRef ApCst 00003000 INTL 20121220)
Quoted 9 lines
ACPI: SSDT
0x000000000068EC2800004B (v02 LENOVO CB-01 00000001 ACPI 00040000)
ACPI: SSDT0x00000000006B51380052EE (v02 LENOVO CB-01 00000001 ACPI 00040000)
ACPI: SSDT0x00000000006BA430002C4A (v02 LENOVO CB-01 00000001 ACPI 00040000)
ACPI: SSDT0x00000000006BD0880004C8 (v02 LENOVO CB-01 00000001 ACPI 00040000)
ACPI: SSDT0x000000000068EC800002D4 (v01 LENOVO CB-01 00000001 ACPI 00040000)
ACPI: SSDT0x00000000006BD558002BAE (v02 LENOVO CB-01 00000001 ACPI 00040000)
ACPI: SSDT0x00000000006C011000019A (v02 LENOVO CB-01 00000001 ACPI 00040000)
ACPI: SSDT0x00000000006C02B80006DC (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
Zheng
Hi,
Quoted 36 lines
From: Hans de Goede [mailto:hdegoede(a)redhat.com]
Subject: Re: [PATCH] ACPICA: Detect duplicate SSDT tablesHi,
Quoted 22 lines · Lv
Hi,
Quoted 20 lines
From: Hans de Goede [mailto:hdegoede(a)redhat.com]
Subject: Re: [PATCH] ACPICA: Detect duplicate SSDT tablesHi,
Quoted 9 lines · Lv
Hi,
Quoted 5 lines
From: Hans de Goede [mailto:hdegoede(a)redhat.com]
Subject: [PATCH] ACPICA: Detect duplicate SSDT tablesSome 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:
Quoted 7 lines
Quoted 2 lines
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 positionagainst existing node, or
Quoted 8 lines
Quoted 4 lines
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.
Quoted 1 line
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.
Quoted 6 lines
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:
Quoted 8 lines
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.
Quoted 30 lines
Quoted 22 lines
So I don't think this commit goes the right direction on the right track.
Quoted 19 lines
Quoted 5 lines
Quoted 3 lines
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.
Quoted 1 line
And should we add non-costless steps just in order to reduce errors,
Quoted 7 lines
Quoted 4 lines
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.
Quoted 8 lines
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.
Quoted 22 lines
Quoted 22 lines
Quoted 22 lines
Quoted 2 lines
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_EXCEPTIONcalls 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));
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
Into a warning ?
So will Linux just unconditionally change
pr_err()intopr_warn()in theprintk.h?I'm not talking about unconditionally doing this,
OK, I see.
Quoted 4 lines
the acpica code itself
contains indrivers/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.
Quoted 5 lines
Since acpica is ignoring errors here, it would seem reasonable to me for acpica to treat all
ACPI_ERROR/ACPI_EXCEPTIONcalls while doing this asACPI_WARNINGcalls.
Quoted 7 lines
Quoted 3 lines
ACPI_EXCEPTIONhere actually means blue screen in Windows.
Maybe it's correct, maybe not.
Linux doesn't run into panic inACPI_EXCEPTIIONjust because Linux ACPI implementation still hascompliance 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.
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Signed-off-by Hans de Goede <hdegoede(a)redhat.com>--- drivers/acpi/acpica/tbxfload.c | 41 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-)drivers/acpi/acpica/tbxfload.c +0
-0@@ -125,6 +125,44 @@ ACPI_EXPORT_SYMBOL_INIT(acpi_load_tables)
Quoted 14 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
drivers/acpi/acpica/tbxfload.c +4
-0/******************************************************************************* * +* 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 Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
drivers/acpi/acpica/tbxfload.c ACPI_EXPORT_SYMBOL_INIT(acpi_load_tables) · line 139
Quoted 8 lines
drivers/acpi/acpica/tbxfload.c +8
-0+* +* 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.
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/acpi/acpica/tbxfload.c ACPI_EXPORT_SYMBOL_INIT(acpi_load_tables) · line 144
Quoted 5 lines
drivers/acpi/acpica/tbxfload.c +5
-0+{ +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.
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
drivers/acpi/acpica/tbxfload.c ACPI_EXPORT_SYMBOL_INIT(acpi_load_tables) · line 147
Quoted 3 lines
drivers/acpi/acpica/tbxfload.c +3
-0+ 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.
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/acpi/acpica/tbxfload.c ACPI_EXPORT_SYMBOL_INIT(acpi_load_tables) · line 152
Quoted 5 lines
drivers/acpi/acpica/tbxfload.c +5
-0+ (!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.
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
drivers/acpi/acpica/tbxfload.c ACPI_EXPORT_SYMBOL_INIT(acpi_load_tables) · line 153
Quoted 1 line
drivers/acpi/acpica/tbxfload.c +1
-0+ || 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.
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/acpi/acpica/tbxfload.c ACPI_EXPORT_SYMBOL_INIT(acpi_load_tables) · line 158
Quoted 5 lines
drivers/acpi/acpica/tbxfload.c +5
-0+ || 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.
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
drivers/acpi/acpica/tbxfload.c +7
-0+return TRUE; + } +return FALSE; +} + +/******************************************************************************* +* * FUNCTION: acpi_tb_load_namespace *
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
drivers/acpi/acpica/tbxfload.c acpi_status acpi_tb_load_namespace(void) · line 254
Quoted 8 lines
drivers/acpi/acpica/tbxfload.c +2
-1* 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
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
drivers/acpi/acpica/tbxfload.c +0
-1continue ; } --2.9.3
Hi,
Quoted 23 lines · Lv
Hi,
Quoted 21 lines
From: Hans de Goede [mailto:[email protected]]
Subject: Re: [PATCH] ACPICA: Detect duplicate SSDT tablesHi,
Quoted 15 lines · Lv
Hi,
Quoted 13 lines
From: Hans de Goede [mailto:[email protected]]
Subject: Re: [PATCH] ACPICA: Detect duplicate SSDT tablesHi,
Quoted 7 lines · Lv
Hi,
Quoted 5 lines
From: Hans de Goede [mailto:[email protected]]
Subject: [PATCH] ACPICA: Detect duplicate SSDT tablesSome machines have the exact (byte for byte) same SSDT tables multiple times in the
root_table_list.
<snip>
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Detect this and silently skip the duplicates rather then printing a scary looking set of errors.
<snip>
Quoted 32 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Signed-off-by Hans de Goede <[email protected]>--- drivers/acpi/acpica/tbxfload.c | 41 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-)drivers/acpi/acpica/tbxfload.c +0
-0@@ -125,6 +125,44 @@ ACPI_EXPORT_SYMBOL_INIT(acpi_load_tables)Quoted 14 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
drivers/acpi/acpica/tbxfload.c +4
-0/******************************************************************************* * +* 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 Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
drivers/acpi/acpica/tbxfload.c ACPI_EXPORT_SYMBOL_INIT(acpi_load_tables) · line 139
Quoted 8 lines
drivers/acpi/acpica/tbxfload.c +8
-0+* +* 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 inacpi_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.
Quoted 1 line
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
Zheng
Hi,
Quoted 69 lines
From: Hans de Goede [mailto:hdegoede(a)redhat.com]
Subject: Re: [PATCH] ACPICA: Detect duplicate SSDT tablesHi,
Quoted 23 lines · Lv
Hi,
Quoted 21 lines
From: Hans de Goede [mailto:hdegoede(a)redhat.com]
Subject: Re: [PATCH] ACPICA: Detect duplicate SSDT tablesHi,
Quoted 15 lines · Lv
Hi,
Quoted 13 lines
From: Hans de Goede [mailto:hdegoede(a)redhat.com]
Subject: Re: [PATCH] ACPICA: Detect duplicate SSDT tablesHi,
Quoted 7 lines · Lv
Hi,
Quoted 5 lines
From: Hans de Goede [mailto:hdegoede(a)redhat.com]
Subject: [PATCH] ACPICA: Detect duplicate SSDT tablesSome machines have the exact (byte for byte) same SSDT tables multiple times in the
root_table_list.<snip>
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Detect this and silently skip the duplicates rather then printing a scary looking set of errors.
<snip>
Quoted 32 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Signed-off-by Hans de Goede <hdegoede(a)redhat.com>--- drivers/acpi/acpica/tbxfload.c | 41 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-)drivers/acpi/acpica/tbxfload.c +0
-0@@ -125,6 +125,44 @@ ACPI_EXPORT_SYMBOL_INIT(acpi_load_tables)Quoted 14 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
drivers/acpi/acpica/tbxfload.c +4
-0/******************************************************************************* * +* 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 Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
drivers/acpi/acpica/tbxfload.c ACPI_EXPORT_SYMBOL_INIT(acpi_load_tables) · line 139
Quoted 8 lines
drivers/acpi/acpica/tbxfload.c +8
-0+* +* 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 inacpi_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.
Quoted 6 lines
Quoted 1 line
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
Hi,
Quoted 73 lines · Lv
Hi,
Quoted 71 lines
From: Hans de Goede [mailto:[email protected]]
Subject: Re: [PATCH] ACPICA: Detect duplicate SSDT tablesHi,
Quoted 23 lines · Lv
Hi,
Quoted 21 lines
From: Hans de Goede [mailto:[email protected]]
Subject: Re: [PATCH] ACPICA: Detect duplicate SSDT tablesHi,
Quoted 15 lines · Lv
Hi,
Quoted 13 lines
From: Hans de Goede [mailto:[email protected]]
Subject: Re: [PATCH] ACPICA: Detect duplicate SSDT tablesHi,
Quoted 7 lines · Lv
Hi,
Quoted 5 lines
From: Hans de Goede [mailto:[email protected]]
Subject: [PATCH] ACPICA: Detect duplicate SSDT tablesSome machines have the exact (byte for byte) same SSDT tables multiple times in the
root_table_list.<snip>
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Detect this and silently skip the duplicates rather then printing a scary looking set of errors.
<snip>
Quoted 33 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Signed-off-by Hans de Goede <[email protected]>--- drivers/acpi/acpica/tbxfload.c | 41 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-)drivers/acpi/acpica/tbxfload.c +0
-0@@ -125,6 +125,44 @@ ACPI_EXPORT_SYMBOL_INIT(acpi_load_tables)Quoted 14 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
drivers/acpi/acpica/tbxfload.c +4
-0/******************************************************************************* * +* 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 Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
drivers/acpi/acpica/tbxfload.c ACPI_EXPORT_SYMBOL_INIT(acpi_load_tables) · line 139
Quoted 8 lines
drivers/acpi/acpica/tbxfload.c +8
-0+* +* 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 inacpi_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.
Quoted 7 lines
Quoted 4 lines
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
Zheng
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
Quoted 93 lines
From: Hans de Goede [mailto:hdegoede(a)redhat.com]
Subject: Re: [PATCH] ACPICA: Detect duplicate SSDT tablesHi,
Quoted 73 lines · Lv
Hi,
Quoted 71 lines
From: Hans de Goede [mailto:hdegoede(a)redhat.com]
Subject: Re: [PATCH] ACPICA: Detect duplicate SSDT tablesHi,
Quoted 23 lines · Lv
Hi,
Quoted 21 lines
From: Hans de Goede [mailto:hdegoede(a)redhat.com]
Subject: Re: [PATCH] ACPICA: Detect duplicate SSDT tablesHi,
Quoted 15 lines · Lv
Hi,
Quoted 13 lines
From: Hans de Goede [mailto:hdegoede(a)redhat.com]
Subject: Re: [PATCH] ACPICA: Detect duplicate SSDT tablesHi,
Quoted 7 lines · Lv
Hi,
Quoted 5 lines
From: Hans de Goede [mailto:hdegoede(a)redhat.com]
Subject: [PATCH] ACPICA: Detect duplicate SSDT tablesSome machines have the exact (byte for byte) same SSDT tables multiple times in the
root_table_list.<snip>
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Detect this and silently skip the duplicates rather then printing a scary looking set of errors.
<snip>
Quoted 33 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Signed-off-by Hans de Goede <hdegoede(a)redhat.com>--- drivers/acpi/acpica/tbxfload.c | 41 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-)drivers/acpi/acpica/tbxfload.c +0
-0@@ -125,6 +125,44 @@ ACPI_EXPORT_SYMBOL_INIT(acpi_load_tables)Quoted 14 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
drivers/acpi/acpica/tbxfload.c +4
-0/******************************************************************************* * +* 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 Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
drivers/acpi/acpica/tbxfload.c ACPI_EXPORT_SYMBOL_INIT(acpi_load_tables) · line 139
Quoted 8 lines
drivers/acpi/acpica/tbxfload.c +8
-0+* +* 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 inacpi_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.Quoted 7 lines
Quoted 4 lines
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
Lv Zheng
v2 cover ACPICA: Tables: Add deferred verification support
In reply to Hans de GoedeOn 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].
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(-)
Signature · 1 line
2.7.4
Lv Zheng
RFC PATCH v2 1/5 ACPICA: Tables: Cleanup table handler invokers
Recently, we allows the table mutex to be held in both early and late stage APIs. This patch further cleans up the related code to reduce redundant code related to acpi_gbl_table_handler. Lv Zheng.
--- drivers/acpi/acpica/actables.h | 2 ++ drivers/acpi/acpica/tbdata.c | 43 ++++++++++++++++++++++++++++-------------- drivers/acpi/acpica/tbinstal.c | 8 ++------ 3 files changed, 33 insertions(+), 20 deletions(-)
drivers/acpi/acpica/actables.h +2 -0
@@ -132,6 +132,8 @@ acpi_tb_install_and_load_table(acpi_physical_address address,acpi_status acpi_tb_unload_table (u32 table_index); +void acpi_tb_notify_table (u32 event,void *table); +void acpi_tb_terminate (void );acpi_status acpi_tb_delete_namespace_by_owner (u32 table_index);
drivers/acpi/acpica/tbdata.c +29 -14
60 lines not on this page · raw message
drivers/acpi/acpica/tbinstal.c +2 -6
@@ -306,14 +306,10 @@ acpi_tb_install_standard_table(acpi_physical_address address,acpi_tb_install_table_with_override (&new_table_desc, override, table_index); -/* Invoke table handler if present */ +/* Invoke table handler */ (void )acpi_ut_release_mutex (ACPI_MTX_TABLES ); -if (acpi_gbl_table_handler) { - (void )acpi_gbl_table_handler (ACPI_TABLE_EVENT_INSTALL , - new_table_desc.pointer, - acpi_gbl_table_handler_context); - } +acpi_tb_notify_table (ACPI_TABLE_EVENT_INSTALL , new_table_desc.pointer); (void )acpi_ut_acquire_mutex (ACPI_MTX_TABLES ); unlock_and_exit:
Signature · 1 line
2.7.4
Lv Zheng
RFC PATCH v2 2/5 ACPICA: Tables: Do not validate signature for dynamic table load
In reply to Lv ZhengWindows seems to allow arbitrary table signatures for Load/load_table opcodes:
ACPI BIOS Error (bug): Table has invalid signature [PRAD] (0x44415250)
So this patch removes dynamic load signature checks. However we need to find a way to avoid table loading against tables like MADT. This is not covered by this commit.
This Windows behavior has been validated on link #1. An end user bug report can also be found on link #2.
This patch also includes simple cleanup for static load signature check code. Reported by Ye Xiaolong, Fixed by Lv Zheng.
--- drivers/acpi/acpica/tbinstal.c | 28 ---------------------------- drivers/acpi/acpica/tbxfload.c | 2 +- 2 files changed, 1 insertion(+), 29 deletions(-)
drivers/acpi/acpica/tbinstal.c +0 -28
@@ -222,34 +222,6 @@ acpi_tb_install_standard_table(acpi_physical_address address, (void )acpi_ut_acquire_mutex (ACPI_MTX_TABLES );if (reload) { -/* -* Validate the incoming table signature. -* -* 1) Originally, we checked the table signature for "SSDT" or "PSDT". -* 2) We added support for OEMx tables, signature "OEM". -* 3) Valid tables were encountered with a null signature, so we just -* gave up on validating the signature, (05/2008). -* 4) We encountered non-AML tables such as the MADT, which caused -* interpreter errors and kernel faults. So now, we once again allow -* only "SSDT", "OEMx", and now, also a null signature. (05/2011). -*/ -if ((new_table_desc.signature.ascii[0 ] !=0x00 ) && - (!ACPI_COMPARE_NAME - (&new_table_desc.signature,ACPI_SIG_SSDT )) - && (strncmp (new_table_desc.signature.ascii,"OEM" ,3 ))) { -ACPI_BIOS_ERROR ((AE_INFO , -"Table has invalid signature [%4.4s] (0x%8.8X), " -"must be SSDT or OEMx" , -acpi_ut_valid_nameseg (new_table_desc. - signature. - ascii) ? - new_table_desc.signature. - ascii :"????" , - new_table_desc.signature.integer)); - - status =AE_BAD_SIGNATURE ; - goto unlock_and_exit; - }/* Check if table is already registered */
drivers/acpi/acpica/tbxfload.c +1 -1
@@ -206,7 +206,7 @@ acpi_status acpi_tb_load_namespace(void)for (i =0 ; i < acpi_gbl_root_table_list.current_table_count; ++i) { table = &acpi_gbl_root_table_list.tables[i]; - if (!acpi_gbl_root_table_list.tables[i].address || + if (!table->address || (!ACPI_COMPARE_NAME(table->signature.ascii, ACPI_SIG_SSDT) && !ACPI_COMPARE_NAME(table->signature.ascii, ACPI_SIG_PSDT)
Signature · 1 line
2.7.4
Lv Zheng
RFC PATCH v2 3/5 ACPICA: Tables: Change table duplication check to be related to acpi_gbl_verify_table_checksum
In reply to Lv Zhengacpi_gbl_verify_table_checksum is used to avoid validating (mapping) an entire table in OS boot stage. 2nd "Reload" check in acpi_tb_install_standard_table() is prepared for the same purpose. So this patch combines them together using a renamed acpi_gbl_enable_table_validation flag. Lv Zheng.
--- drivers/acpi/acpica/tbdata.c | 4 ++-- drivers/acpi/acpica/tbinstal.c | 2 +- drivers/acpi/acpica/tbxface.c | 8 ++++++++ drivers/acpi/bus.c | 2 +- drivers/acpi/tables.c | 4 ++-- include/acpi/acpixf.h | 15 ++++++++------- 6 files changed, 22 insertions(+), 13 deletions(-)
drivers/acpi/acpica/tbdata.c +2 -2
@@ -338,7 +338,7 @@ void acpi_tb_invalidate_table(struct acpi_table_desc *table_desc)acpi_status acpi_tb_validate_temp_table (struct acpi_table_desc *table_desc) { -if (!table_desc->pointer && !acpi_gbl_verify_table_checksum) { +if (!table_desc->pointer && !acpi_gbl_enable_table_validation) {/* * Only validates the header of the table. * Note that Length contains the size of the mapping after invoking
@@ -394,7 +394,7 @@ acpi_tb_verify_temp_table(struct acpi_table_desc *table_desc, char *signature)/* Verify the checksum */ -if (acpi_gbl_verify_table_checksum) { +if (acpi_gbl_enable_table_validation) { status =acpi_tb_verify_checksum (table_desc->pointer, table_desc->length);
drivers/acpi/acpica/tbinstal.c +1 -1
@@ -221,7 +221,7 @@ acpi_tb_install_standard_table(acpi_physical_address address, (void )acpi_ut_acquire_mutex (ACPI_MTX_TABLES ); -if (reload) { +if (acpi_gbl_enable_table_validation) {/* Check if table is already registered */
drivers/acpi/acpica/tbxface.c +8 -0
@@ -194,6 +194,14 @@ acpi_status ACPI_INIT_FUNCTION acpi_reallocate_root_table(void) } } +if (!acpi_gbl_enable_table_validation) { +/* +* Now it's safe to do full table validation. We can do deferred +* table initilization here once the flag is set. +*/ + acpi_gbl_enable_table_validation = TRUE; + } + acpi_gbl_root_table_list.flags |=ACPI_ROOT_ALLOW_RESIZE ; status =acpi_tb_resize_root_table_list ();
drivers/acpi/bus.c +1 -1
@@ -1010,7 +1010,7 @@ void __init acpi_early_init(void) printk(KERN_INFO PREFIX "Core revision %08x\n", ACPI_CA_VERSION);/* It's safe to verify table checksums during late stage */ - acpi_gbl_verify_table_checksum = TRUE; + acpi_gbl_enable_table_validation = TRUE;/* enable workarounds, unless strict ACPI spec. compliance */ if (!acpi_strict)
drivers/acpi/tables.c +2 -2
@@ -740,10 +740,10 @@ int __init acpi_table_init(void)if (acpi_verify_table_checksum) {pr_info ("Early table checksum verification enabled\n" ); - acpi_gbl_verify_table_checksum = TRUE; + acpi_gbl_enable_table_validation = TRUE; }else {pr_info ("Early table checksum verification disabled\n" ); - acpi_gbl_verify_table_checksum = FALSE; + acpi_gbl_enable_table_validation = FALSE; } status =acpi_initialize_tables (initial_tables,ACPI_MAX_TABLES ,0 );
include/acpi/acpixf.h +8 -7
@@ -160,13 +160,14 @@ ACPI_INIT_GLOBAL(u8, acpi_gbl_create_osi_method, TRUE);ACPI_INIT_GLOBAL (u8, acpi_gbl_use_default_register_widths, TRUE);/* -* Whether or not to verify the table checksum before installation. Set -* this to TRUE to verify the table checksum before install it to the table -* manager. Note that enabling this option causes errors to happen in some -* OSPMs during early initialization stages. Default behavior is to do such -* verification. -*/ -ACPI_INIT_GLOBAL (u8, acpi_gbl_verify_table_checksum, TRUE); +* Whether or not to validate (map) an entire table to verify +* checksum/duplication in early stage before install. Set this to TRUE to +* allow early table validation before install it to the table manager. +* Note that enabling this option causes errors to happen in some OSPMs +* during early initialization stages. Default behavior is to allow such +* validation. +*/ +ACPI_INIT_GLOBAL (u8, acpi_gbl_enable_table_validation, TRUE);/* * Optionally enable output from the AML Debug Object.
Signature · 1 line
2.7.4
Lv Zheng
RFC PATCH v2 4/5 ACPICA: Tables: Combine checksum/duplication verification together
In reply to Lv ZhengThey are all mechanism used to verify if a table is qualified to be installed and controlled by acpi_gbl_enable_table_validation, so combine them together. Lv Zheng.
--- drivers/acpi/acpica/actables.h | 3 +- drivers/acpi/acpica/tbdata.c | 148 +++++++++++++++++++++++++++++++++++++++-- drivers/acpi/acpica/tbinstal.c | 124 ++++++---------------------------- 3 files changed, 165 insertions(+), 110 deletions(-)
drivers/acpi/acpica/actables.h +2 -1
@@ -76,7 +76,8 @@ void acpi_tb_release_temp_table(struct acpi_table_desc *table_desc);acpi_status acpi_tb_validate_temp_table (struct acpi_table_desc *table_desc);acpi_status -acpi_tb_verify_temp_table (struct acpi_table_desc *table_desc,char *signature); +acpi_tb_verify_temp_table (struct acpi_table_desc *table_desc, +char *signature,u32 *table_index);u8 acpi_tb_is_table_loaded (u32 table_index);
drivers/acpi/acpica/tbdata.c +144 -4
193 lines not on this page · raw message
drivers/acpi/acpica/tbinstal.c +19 -105
156 lines not on this page · raw message
Signature · 1 line
2.7.4
Lv Zheng
RFC PATCH v2 5/5 ACPICA: Tables: Add deferred table verification support
In reply to Lv ZhengThis patch allows tables not verified in early stage verfied in acpi_reallocate_root_table(). This is useful for OSPMs like linux where tables cannot be verified in early stage due to early ioremp limitations on some architectures. Reported by Hans de Geode, fixed by Lv Zheng.
--- drivers/acpi/acpica/tbdata.c | 31 ++++++++++++++++++++++++------- drivers/acpi/acpica/tbinstal.c | 1 + drivers/acpi/acpica/tbxface.c | 28 +++++++++++++++++++++++----- drivers/acpi/bus.c | 3 --- include/acpi/actbl.h | 1 + 5 files changed, 49 insertions(+), 15 deletions(-)
drivers/acpi/acpica/tbdata.c +24 -7
@@ -426,12 +426,18 @@ acpi_status acpi_tb_check_duplication(struct acpi_table_desc *table_desc, u32 *table_index) { u32 i; +u32 max_table_index;ACPI_FUNCTION_TRACE (tb_check_duplication);/* Check if table is already registered */ -for (i =0 ; i < acpi_gbl_root_table_list.current_table_count; ++i) { +if (table_index) { + max_table_index = *table_index; + }else { + max_table_index = acpi_gbl_root_table_list.current_table_count; + } +for (i =0 ; i < max_table_index; ++i) {/* * Check for a table match on the entire table length, * not just the header.
@@ -551,6 +557,8 @@ acpi_tb_verify_temp_table(struct acpi_table_desc *table_desc, goto invalidate_and_exit; } } + + table_desc->flags |=ACPI_TABLE_IS_VERIFIED ; }return_ACPI_STATUS (status);
@@ -576,6 +584,8 @@ acpi_status acpi_tb_resize_root_table_list(void) {struct acpi_table_desc *tables;u32 table_count; +u32 current_table_count, max_table_count; +u32 i;ACPI_FUNCTION_TRACE (tb_resize_root_table_list);
@@ -595,8 +605,8 @@ acpi_status acpi_tb_resize_root_table_list(void) table_count = acpi_gbl_root_table_list.current_table_count; } - tables =ACPI_ALLOCATE_ZEROED (((acpi_size )table_count + -ACPI_ROOT_TABLE_SIZE_INCREMENT ) * + max_table_count = table_count +ACPI_ROOT_TABLE_SIZE_INCREMENT ; + tables =ACPI_ALLOCATE_ZEROED (((acpi_size )max_table_count) *sizeof (struct acpi_table_desc ));if (!tables) { ACPI_ERROR((AE_INFO,
@@ -606,9 +616,16 @@ acpi_status acpi_tb_resize_root_table_list(void)/* Copy and free the previous table array */ + current_table_count =0 ;if (acpi_gbl_root_table_list.tables) { -memcpy (tables, acpi_gbl_root_table_list.tables, - (acpi_size )table_count *sizeof (struct acpi_table_desc )); +for (i =0 ; i < table_count; i++) { +if (acpi_gbl_root_table_list.tables[i].address) { +memcpy (tables + current_table_count, + acpi_gbl_root_table_list.tables + i, +sizeof (struct acpi_table_desc )); + current_table_count++; + } + }if (acpi_gbl_root_table_list.flags &ACPI_ROOT_ORIGIN_ALLOCATED ) {ACPI_FREE (acpi_gbl_root_table_list.tables);
@@ -616,8 +633,8 @@ acpi_status acpi_tb_resize_root_table_list(void) } acpi_gbl_root_table_list.tables = tables; - acpi_gbl_root_table_list.max_table_count = - table_count +ACPI_ROOT_TABLE_SIZE_INCREMENT ; + acpi_gbl_root_table_list.max_table_count = max_table_count; + acpi_gbl_root_table_list.current_table_count = current_table_count; acpi_gbl_root_table_list.flags |=ACPI_ROOT_ORIGIN_ALLOCATED ;return_ACPI_STATUS (AE_OK );
drivers/acpi/acpica/tbinstal.c +1 -0
@@ -167,6 +167,7 @@ acpi_tb_install_standard_table(acpi_physical_address address,/* Validate and verify a table before installation */ + i = acpi_gbl_root_table_list.current_table_count; status =acpi_tb_verify_temp_table (&new_table_desc,NULL , &i);if (ACPI_FAILURE (status)) {if (status ==AE_CTRL_TERMINATE ) {
drivers/acpi/acpica/tbxface.c +23 -5
@@ -167,7 +167,8 @@ ACPI_EXPORT_SYMBOL_INIT(acpi_initialize_tables) acpi_status ACPI_INIT_FUNCTION acpi_reallocate_root_table(void) {acpi_status status; -u32 i; +struct acpi_table_desc *table_desc; +u32 i, j;ACPI_FUNCTION_TRACE (acpi_reallocate_root_table);
@@ -179,6 +180,8 @@ acpi_status ACPI_INIT_FUNCTION acpi_reallocate_root_table(void)return_ACPI_STATUS (AE_SUPPORT ); } + (void )acpi_ut_acquire_mutex (ACPI_MTX_TABLES ); +/* * Ensure OS early boot logic, which is required by some hosts. If the * table state is reported to be wrong, developers should fix the
@@ -186,11 +189,11 @@ acpi_status ACPI_INIT_FUNCTION acpi_reallocate_root_table(void)* early stage. */ for (i =0 ; i < acpi_gbl_root_table_list.current_table_count; ++i) { -if (acpi_gbl_root_table_list.tables[i].pointer) { + table_desc = &acpi_gbl_root_table_list.tables[i]; +if (table_desc->pointer) {ACPI_ERROR ((AE_INFO ,"Table [%4.4s] is not invalidated during early boot stage" , - acpi_gbl_root_table_list.tables[i]. - signature.ascii)); + table_desc->signature.ascii)); } }
@@ -200,11 +203,26 @@ acpi_status ACPI_INIT_FUNCTION acpi_reallocate_root_table(void)* table initilization here once the flag is set. */ acpi_gbl_enable_table_validation = TRUE; +for (i =0 ; i < acpi_gbl_root_table_list.current_table_count; + ++i) { + table_desc = &acpi_gbl_root_table_list.tables[i]; +if (!(table_desc->flags &ACPI_TABLE_IS_VERIFIED )) { + j = i; + status = +acpi_tb_verify_temp_table (table_desc,NULL , + &j); +if (ACPI_FAILURE (status)) { +acpi_tb_uninstall_table (table_desc); + } + } + } } acpi_gbl_root_table_list.flags |=ACPI_ROOT_ALLOW_RESIZE ; - status =acpi_tb_resize_root_table_list (); + acpi_gbl_root_table_list.flags |=ACPI_ROOT_ORIGIN_ALLOCATED ; + + (void )acpi_ut_release_mutex (ACPI_MTX_TABLES );return_ACPI_STATUS (status); }
drivers/acpi/bus.c +0 -3
@@ -1009,9 +1009,6 @@ void __init acpi_early_init(void) printk(KERN_INFO PREFIX "Core revision %08x\n", ACPI_CA_VERSION); -/* It's safe to verify table checksums during late stage */ - acpi_gbl_enable_table_validation = TRUE; -/* enable workarounds, unless strict ACPI spec. compliance */ if (!acpi_strict) acpi_gbl_enable_interpreter_slack = TRUE;
include/acpi/actbl.h +1 -0
@@ -380,6 +380,7 @@ struct acpi_table_desc {#define ACPI_TABLE_ORIGIN_INTERNAL_PHYSICAL (1)/* Physical address, internally mapped */ #define ACPI_TABLE_ORIGIN_INTERNAL_VIRTUAL (2)/* Virtual address, internallly allocated */ #define ACPI_TABLE_ORIGIN_MASK (3) +#define ACPI_TABLE_IS_VERIFIED (4)#define ACPI_TABLE_IS_LOADED (8)/*
Signature · 1 line
2.7.4
Re: [RFC PATCH v2 0/5] ACPICA: Tables: Add deferred verification support
In reply to Lv ZhengHi,
Quoted 29 lines · Lv Zheng
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].https://bugzilla.kernel.org/show_bug.cgi?id=118601 [#5]
Quoted 7 lines
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
Zheng
Re: [Devel] [RFC PATCH v2 0/5] ACPICA: Tables: Add deferred verification support
Hi, Hans
Quoted 43 lines
From: Hans de Goede [mailto:hdegoede(a)redhat.com]
Subject: Re: [RFC PATCH v2 0/5] ACPICA: Tables: Add deferred verification supportHi,
Quoted 29 lines · Lv Zheng
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].https://bugzilla.kernel.org/show_bug.cgi?id=118601 [#5]Quoted 7 lines
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
Quoted 7 lines
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
Thanks and best regards
Lv
Re: [RFC PATCH v2 0/5] ACPICA: Tables: Add deferred verification support
Hi,
Quoted 68 lines · Lv
Hi, Hans
Quoted 43 lines
From: Hans de Goede [mailto:[email protected]]
Subject: Re: [RFC PATCH v2 0/5] ACPICA: Tables: Add deferred verification supportHi,
Quoted 29 lines · Lv Zheng
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].https://bugzilla.kernel.org/show_bug.cgi?id=118601 [#5]Quoted 7 lines
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 supportQuoted 7 lines
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
Quoted 3 lines
# 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
Lv Zheng
v3 cover ACPICA: Tables: Add deferred verification support
In reply to Hans de GoedeOn 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.
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(-)
Signature · 1 line
2.7.4
Lv Zheng
RFC PATCH v3 1/5 ACPICA: Tables: Cleanup table handler invokers
Recently, we allows the table mutex to be held in both early and late stage APIs. This patch further cleans up the related code to reduce redundant code related to acpi_gbl_table_handler. Lv Zheng.
--- drivers/acpi/acpica/actables.h | 2 ++ drivers/acpi/acpica/tbdata.c | 43 ++++++++++++++++++++++++++++-------------- drivers/acpi/acpica/tbinstal.c | 8 ++------ 3 files changed, 33 insertions(+), 20 deletions(-)
drivers/acpi/acpica/actables.h +2 -0
@@ -132,6 +132,8 @@ acpi_tb_install_and_load_table(acpi_physical_address address,acpi_status acpi_tb_unload_table (u32 table_index); +void acpi_tb_notify_table (u32 event,void *table); +void acpi_tb_terminate (void );acpi_status acpi_tb_delete_namespace_by_owner (u32 table_index);
drivers/acpi/acpica/tbdata.c +29 -14
60 lines not on this page · raw message
drivers/acpi/acpica/tbinstal.c +2 -6
@@ -306,14 +306,10 @@ acpi_tb_install_standard_table(acpi_physical_address address,acpi_tb_install_table_with_override (&new_table_desc, override, table_index); -/* Invoke table handler if present */ +/* Invoke table handler */ (void )acpi_ut_release_mutex (ACPI_MTX_TABLES ); -if (acpi_gbl_table_handler) { - (void )acpi_gbl_table_handler (ACPI_TABLE_EVENT_INSTALL , - new_table_desc.pointer, - acpi_gbl_table_handler_context); - } +acpi_tb_notify_table (ACPI_TABLE_EVENT_INSTALL , new_table_desc.pointer); (void )acpi_ut_acquire_mutex (ACPI_MTX_TABLES ); unlock_and_exit:
Signature · 1 line
2.7.4
Lv Zheng
RFC PATCH v3 2/5 ACPICA: Tables: Do not validate signature for dynamic table load
In reply to Lv ZhengWindows seems to allow arbitrary table signatures for Load/load_table opcodes:
ACPI BIOS Error (bug): Table has invalid signature [PRAD] (0x44415250)
So this patch removes dynamic load signature checks. However we need to find a way to avoid table loading against tables like MADT. This is not covered by this commit.
This Windows behavior has been validated on link #1. An end user bug report can also be found on link #2.
This patch also includes simple cleanup for static load signature check code. Reported by Ye Xiaolong, Fixed by Lv Zheng.
--- drivers/acpi/acpica/tbinstal.c | 28 ---------------------------- drivers/acpi/acpica/tbxfload.c | 2 +- 2 files changed, 1 insertion(+), 29 deletions(-)
drivers/acpi/acpica/tbinstal.c +0 -28
@@ -222,34 +222,6 @@ acpi_tb_install_standard_table(acpi_physical_address address, (void )acpi_ut_acquire_mutex (ACPI_MTX_TABLES );if (reload) { -/* -* Validate the incoming table signature. -* -* 1) Originally, we checked the table signature for "SSDT" or "PSDT". -* 2) We added support for OEMx tables, signature "OEM". -* 3) Valid tables were encountered with a null signature, so we just -* gave up on validating the signature, (05/2008). -* 4) We encountered non-AML tables such as the MADT, which caused -* interpreter errors and kernel faults. So now, we once again allow -* only "SSDT", "OEMx", and now, also a null signature. (05/2011). -*/ -if ((new_table_desc.signature.ascii[0 ] !=0x00 ) && - (!ACPI_COMPARE_NAME - (&new_table_desc.signature,ACPI_SIG_SSDT )) - && (strncmp (new_table_desc.signature.ascii,"OEM" ,3 ))) { -ACPI_BIOS_ERROR ((AE_INFO , -"Table has invalid signature [%4.4s] (0x%8.8X), " -"must be SSDT or OEMx" , -acpi_ut_valid_nameseg (new_table_desc. - signature. - ascii) ? - new_table_desc.signature. - ascii :"????" , - new_table_desc.signature.integer)); - - status =AE_BAD_SIGNATURE ; - goto unlock_and_exit; - }/* Check if table is already registered */
drivers/acpi/acpica/tbxfload.c +1 -1
@@ -206,7 +206,7 @@ acpi_status acpi_tb_load_namespace(void)for (i =0 ; i < acpi_gbl_root_table_list.current_table_count; ++i) { table = &acpi_gbl_root_table_list.tables[i]; - if (!acpi_gbl_root_table_list.tables[i].address || + if (!table->address || (!ACPI_COMPARE_NAME(table->signature.ascii, ACPI_SIG_SSDT) && !ACPI_COMPARE_NAME(table->signature.ascii, ACPI_SIG_PSDT)
Signature · 1 line
2.7.4
Lv Zheng
RFC PATCH v3 3/5 ACPICA: Tables: Change table duplication check to be related to acpi_gbl_verify_table_checksum
In reply to Lv Zhengacpi_gbl_verify_table_checksum is used to avoid validating (mapping) an entire table in OS boot stage. 2nd "Reload" check in acpi_tb_install_standard_table() is prepared for the same purpose. So this patch combines them together using a renamed acpi_gbl_enable_table_validation flag. Lv Zheng.
--- drivers/acpi/acpica/tbdata.c | 4 ++-- drivers/acpi/acpica/tbinstal.c | 2 +- drivers/acpi/acpica/tbxface.c | 8 ++++++++ drivers/acpi/bus.c | 3 --- drivers/acpi/tables.c | 4 ++-- include/acpi/acpixf.h | 15 ++++++++------- 6 files changed, 21 insertions(+), 15 deletions(-)
drivers/acpi/acpica/tbdata.c +2 -2
@@ -338,7 +338,7 @@ void acpi_tb_invalidate_table(struct acpi_table_desc *table_desc)acpi_status acpi_tb_validate_temp_table (struct acpi_table_desc *table_desc) { -if (!table_desc->pointer && !acpi_gbl_verify_table_checksum) { +if (!table_desc->pointer && !acpi_gbl_enable_table_validation) {/* * Only validates the header of the table. * Note that Length contains the size of the mapping after invoking
@@ -394,7 +394,7 @@ acpi_tb_verify_temp_table(struct acpi_table_desc *table_desc, char *signature)/* Verify the checksum */ -if (acpi_gbl_verify_table_checksum) { +if (acpi_gbl_enable_table_validation) { status =acpi_tb_verify_checksum (table_desc->pointer, table_desc->length);
drivers/acpi/acpica/tbinstal.c +1 -1
@@ -221,7 +221,7 @@ acpi_tb_install_standard_table(acpi_physical_address address, (void )acpi_ut_acquire_mutex (ACPI_MTX_TABLES ); -if (reload) { +if (acpi_gbl_enable_table_validation) {/* Check if table is already registered */
drivers/acpi/acpica/tbxface.c +8 -0
@@ -194,6 +194,14 @@ acpi_status ACPI_INIT_FUNCTION acpi_reallocate_root_table(void) } } +if (!acpi_gbl_enable_table_validation) { +/* +* Now it's safe to do full table validation. We can do deferred +* table initilization here once the flag is set. +*/ + acpi_gbl_enable_table_validation = TRUE; + } + acpi_gbl_root_table_list.flags |=ACPI_ROOT_ALLOW_RESIZE ; status =acpi_tb_resize_root_table_list ();
drivers/acpi/bus.c +0 -3
@@ -1009,9 +1009,6 @@ void __init acpi_early_init(void) printk(KERN_INFO PREFIX "Core revision %08x\n", ACPI_CA_VERSION); -/* It's safe to verify table checksums during late stage */ - acpi_gbl_verify_table_checksum = TRUE; -/* enable workarounds, unless strict ACPI spec. compliance */ if (!acpi_strict) acpi_gbl_enable_interpreter_slack = TRUE;
drivers/acpi/tables.c +2 -2
@@ -740,10 +740,10 @@ int __init acpi_table_init(void)if (acpi_verify_table_checksum) {pr_info ("Early table checksum verification enabled\n" ); - acpi_gbl_verify_table_checksum = TRUE; + acpi_gbl_enable_table_validation = TRUE; }else {pr_info ("Early table checksum verification disabled\n" ); - acpi_gbl_verify_table_checksum = FALSE; + acpi_gbl_enable_table_validation = FALSE; } status =acpi_initialize_tables (initial_tables,ACPI_MAX_TABLES ,0 );
include/acpi/acpixf.h +8 -7
@@ -160,13 +160,14 @@ ACPI_INIT_GLOBAL(u8, acpi_gbl_create_osi_method, TRUE);ACPI_INIT_GLOBAL (u8, acpi_gbl_use_default_register_widths, TRUE);/* -* Whether or not to verify the table checksum before installation. Set -* this to TRUE to verify the table checksum before install it to the table -* manager. Note that enabling this option causes errors to happen in some -* OSPMs during early initialization stages. Default behavior is to do such -* verification. -*/ -ACPI_INIT_GLOBAL (u8, acpi_gbl_verify_table_checksum, TRUE); +* Whether or not to validate (map) an entire table to verify +* checksum/duplication in early stage before install. Set this to TRUE to +* allow early table validation before install it to the table manager. +* Note that enabling this option causes errors to happen in some OSPMs +* during early initialization stages. Default behavior is to allow such +* validation. +*/ +ACPI_INIT_GLOBAL (u8, acpi_gbl_enable_table_validation, TRUE);/* * Optionally enable output from the AML Debug Object.
Signature · 1 line
2.7.4
Lv Zheng
RFC PATCH v3 4/5 ACPICA: Tables: Combine checksum/duplication verification together
In reply to Lv ZhengThey are all mechanisms used to verify if a table is qualified to be installed and controlled by acpi_gbl_enable_table_validation, so combine them together. By doing so, table duplication check is applied to the statically loaded tables (however whether it is actually enabled is still determined by acpi_gbl_enable_table_validation). Lv Zheng.
--- drivers/acpi/acpica/actables.h | 3 +- drivers/acpi/acpica/tbdata.c | 151 +++++++++++++++++++++++++++++++++++++++-- drivers/acpi/acpica/tbinstal.c | 125 ++++++---------------------------- 3 files changed, 169 insertions(+), 110 deletions(-)
drivers/acpi/acpica/actables.h +2 -1
@@ -76,7 +76,8 @@ void acpi_tb_release_temp_table(struct acpi_table_desc *table_desc);acpi_status acpi_tb_validate_temp_table (struct acpi_table_desc *table_desc);acpi_status -acpi_tb_verify_temp_table (struct acpi_table_desc *table_desc,char *signature); +acpi_tb_verify_temp_table (struct acpi_table_desc *table_desc, +char *signature,u32 *table_index);u8 acpi_tb_is_table_loaded (u32 table_index);
drivers/acpi/acpica/tbdata.c +147 -4
196 lines not on this page · raw message
drivers/acpi/acpica/tbinstal.c +20 -105
157 lines not on this page · raw message
Signature · 1 line
2.7.4
Lv Zheng
RFC PATCH v3 5/5 ACPICA: Tables: Add deferred table verification support
In reply to Lv ZhengThis patch allows tables not verified in early stage verfied in acpi_reallocate_root_table(). This is useful for OSPMs like linux where tables cannot be verified in early stage due to early ioremp limitations on some architectures. Reported by Hans de Geode, fixed by Lv Zheng.
--- drivers/acpi/acpica/tbdata.c | 32 ++++++++++++++++++++++++++------ drivers/acpi/acpica/tbxface.c | 25 +++++++++++++++++++++---- include/acpi/actbl.h | 1 + 3 files changed, 48 insertions(+), 10 deletions(-)
drivers/acpi/acpica/tbdata.c +26 -6
@@ -432,6 +432,15 @@ acpi_tb_check_duplication(struct acpi_table_desc *table_desc, u32 *table_index)/* Check if table is already registered */ for (i =0 ; i < acpi_gbl_root_table_list.current_table_count; ++i) { + +/* Do not compare with unverified tables */ + +if (! + (acpi_gbl_root_table_list.tables[i]. + flags &ACPI_TABLE_IS_VERIFIED )) { +continue ; + } +/* * Check for a table match on the entire table length, * not just the header.
@@ -554,6 +563,8 @@ acpi_tb_verify_temp_table(struct acpi_table_desc *table_desc, goto invalidate_and_exit; } } + + table_desc->flags |=ACPI_TABLE_IS_VERIFIED ; }return_ACPI_STATUS (status);
@@ -579,6 +590,8 @@ acpi_status acpi_tb_resize_root_table_list(void) {struct acpi_table_desc *tables;u32 table_count; +u32 current_table_count, max_table_count; +u32 i;ACPI_FUNCTION_TRACE (tb_resize_root_table_list);
@@ -598,8 +611,8 @@ acpi_status acpi_tb_resize_root_table_list(void) table_count = acpi_gbl_root_table_list.current_table_count; } - tables =ACPI_ALLOCATE_ZEROED (((acpi_size )table_count + -ACPI_ROOT_TABLE_SIZE_INCREMENT ) * + max_table_count = table_count +ACPI_ROOT_TABLE_SIZE_INCREMENT ; + tables =ACPI_ALLOCATE_ZEROED (((acpi_size )max_table_count) *sizeof (struct acpi_table_desc ));if (!tables) { ACPI_ERROR((AE_INFO,
@@ -609,9 +622,16 @@ acpi_status acpi_tb_resize_root_table_list(void)/* Copy and free the previous table array */ + current_table_count =0 ;if (acpi_gbl_root_table_list.tables) { -memcpy (tables, acpi_gbl_root_table_list.tables, - (acpi_size )table_count *sizeof (struct acpi_table_desc )); +for (i =0 ; i < table_count; i++) { +if (acpi_gbl_root_table_list.tables[i].address) { +memcpy (tables + current_table_count, + acpi_gbl_root_table_list.tables + i, +sizeof (struct acpi_table_desc )); + current_table_count++; + } + }if (acpi_gbl_root_table_list.flags &ACPI_ROOT_ORIGIN_ALLOCATED ) {ACPI_FREE (acpi_gbl_root_table_list.tables);
@@ -619,8 +639,8 @@ acpi_status acpi_tb_resize_root_table_list(void) } acpi_gbl_root_table_list.tables = tables; - acpi_gbl_root_table_list.max_table_count = - table_count +ACPI_ROOT_TABLE_SIZE_INCREMENT ; + acpi_gbl_root_table_list.max_table_count = max_table_count; + acpi_gbl_root_table_list.current_table_count = current_table_count; acpi_gbl_root_table_list.flags |=ACPI_ROOT_ORIGIN_ALLOCATED ;return_ACPI_STATUS (AE_OK );
drivers/acpi/acpica/tbxface.c +21 -4
@@ -167,6 +167,7 @@ ACPI_EXPORT_SYMBOL_INIT(acpi_initialize_tables) acpi_status ACPI_INIT_FUNCTION acpi_reallocate_root_table(void) {acpi_status status; +struct acpi_table_desc *table_desc;u32 i;ACPI_FUNCTION_TRACE (acpi_reallocate_root_table);
@@ -179,6 +180,8 @@ acpi_status ACPI_INIT_FUNCTION acpi_reallocate_root_table(void)return_ACPI_STATUS (AE_SUPPORT ); } + (void )acpi_ut_acquire_mutex (ACPI_MTX_TABLES ); +/* * Ensure OS early boot logic, which is required by some hosts. If the * table state is reported to be wrong, developers should fix the
@@ -186,11 +189,11 @@ acpi_status ACPI_INIT_FUNCTION acpi_reallocate_root_table(void)* early stage. */ for (i =0 ; i < acpi_gbl_root_table_list.current_table_count; ++i) { -if (acpi_gbl_root_table_list.tables[i].pointer) { + table_desc = &acpi_gbl_root_table_list.tables[i]; +if (table_desc->pointer) {ACPI_ERROR ((AE_INFO ,"Table [%4.4s] is not invalidated during early boot stage" , - acpi_gbl_root_table_list.tables[i]. - signature.ascii)); + table_desc->signature.ascii)); } }
@@ -200,11 +203,25 @@ acpi_status ACPI_INIT_FUNCTION acpi_reallocate_root_table(void)* table initilization here once the flag is set. */ acpi_gbl_enable_table_validation = TRUE; +for (i =0 ; i < acpi_gbl_root_table_list.current_table_count; + ++i) { + table_desc = &acpi_gbl_root_table_list.tables[i]; +if (!(table_desc->flags &ACPI_TABLE_IS_VERIFIED )) { + status = +acpi_tb_verify_temp_table (table_desc,NULL , +NULL ); +if (ACPI_FAILURE (status)) { +acpi_tb_uninstall_table (table_desc); + } + } + } } acpi_gbl_root_table_list.flags |=ACPI_ROOT_ALLOW_RESIZE ; - status =acpi_tb_resize_root_table_list (); + acpi_gbl_root_table_list.flags |=ACPI_ROOT_ORIGIN_ALLOCATED ; + + (void )acpi_ut_release_mutex (ACPI_MTX_TABLES );return_ACPI_STATUS (status); }
include/acpi/actbl.h +1 -0
@@ -380,6 +380,7 @@ struct acpi_table_desc {#define ACPI_TABLE_ORIGIN_INTERNAL_PHYSICAL (1)/* Physical address, internally mapped */ #define ACPI_TABLE_ORIGIN_INTERNAL_VIRTUAL (2)/* Virtual address, internallly allocated */ #define ACPI_TABLE_ORIGIN_MASK (3) +#define ACPI_TABLE_IS_VERIFIED (4)#define ACPI_TABLE_IS_LOADED (8)/*
Signature · 1 line
2.7.4