[RFC PATCH 0/7] ACPICA: Tables: Cleanup table sanity checks

[RFC PATCH 0/7] ACPICA: Tables: Cleanup table sanity checks

[Lv Zheng]

Originally AML table sanity check is only done for Load opcode, as
acpi_tb_compare_tables() is only invoked in
acpi_tb_install_standard_table(). While LoadTable opcode cannot be covered
as it directly invokes acpi_tb_load_table() without invoking
acpi_tb_install_standard_table(). Furthermore, standard table load also
cannot be covered as acpi_ns_load_table() is invoked instead of
acpi_tb_install_standard_table() and acpi_tb_load_table().

So it becomes a problem if a duplicate table is in RSDT/XSDT, there is no
such check implemented for static load tables and LoadTable opcode.

This patchset combines code paths (in return reduces some redundant code
blocks and enhances checks/locks) so that duplicate table detection can be
applied to all cases.

Lv Zheng (7):
  ACPICA: Tables: Cleanup table handler invokers
  ACPICA: Tables: Add sanity check for load_table opcode
  ACPICA: Tables: Add sanity check for table install
  ACPICA: Tables: Fix an unwanted exception for table reloading
  ACPICA: Tables: Enable acpi_ut_is_aml_table() for all ACPICA modules
  ACPICA: Tables: Add sanity check for static table load
  ACPICA: Tables: Change table comparison using table header for static
    load tables

 drivers/acpi/acpica/actables.h |   7 +-
 drivers/acpi/acpica/acutils.h  |   4 +-
 drivers/acpi/acpica/exconfig.c |   2 +-
 drivers/acpi/acpica/nsload.c   |  18 ---
 drivers/acpi/acpica/tbdata.c   | 288 ++++++++++++++++++++++++++++++++++++-----
 drivers/acpi/acpica/tbinstal.c | 197 ++++++----------------------
 drivers/acpi/acpica/tbxfload.c |  46 +------
 drivers/acpi/acpica/utmisc.c   |  27 ++--
 include/acpi/actbl.h           |   1 +
 9 files changed, 325 insertions(+), 265 deletions(-)

-- 
2.7.4

[RFC PATCH 1/7] ACPICA: Tables: Cleanup table handler invokers

[Lv Zheng]

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.

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
Cc: Hans de Goede <hdegoede@redhat.com>
---
 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(-)

diff --git a/drivers/acpi/acpica/actables.h b/drivers/acpi/acpica/actables.h
index c8da453..89ed31b 100644
--- a/drivers/acpi/acpica/actables.h
+++ b/drivers/acpi/acpica/actables.h
@@ -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);
diff --git a/drivers/acpi/acpica/tbdata.c b/drivers/acpi/acpica/tbdata.c
index 27c5c27..42ea044 100644
--- a/drivers/acpi/acpica/tbdata.c
+++ b/drivers/acpi/acpica/tbdata.c
@@ -818,13 +818,9 @@ acpi_tb_load_table(u32 table_index, struct acpi_namespace_node *parent_node)
 		acpi_ev_update_gpes(owner_id);
 	}
 
-	/* Invoke table handler if present */
-
-	if (acpi_gbl_table_handler) {
-		(void)acpi_gbl_table_handler(ACPI_TABLE_EVENT_LOAD, table,
-					     acpi_gbl_table_handler_context);
-	}
+	/* Invoke table handler */
 
+	acpi_tb_notify_table(ACPI_TABLE_EVENT_LOAD, table);
 	return_ACPI_STATUS(status);
 }
 
@@ -892,15 +888,11 @@ acpi_status acpi_tb_unload_table(u32 table_index)
 		return_ACPI_STATUS(AE_NOT_EXIST);
 	}
 
-	/* Invoke table handler if present */
+	/* Invoke table handler */
 
-	if (acpi_gbl_table_handler) {
-		status = acpi_get_table_by_index(table_index, &table);
-		if (ACPI_SUCCESS(status)) {
-			(void)acpi_gbl_table_handler(ACPI_TABLE_EVENT_UNLOAD,
-						     table,
-						     acpi_gbl_table_handler_context);
-		}
+	status = acpi_get_table_by_index(table_index, &table);
+	if (ACPI_SUCCESS(status)) {
+		acpi_tb_notify_table(ACPI_TABLE_EVENT_UNLOAD, table);
 	}
 
 	/* Delete the portion of the namespace owned by this table */
@@ -914,3 +906,26 @@ acpi_status acpi_tb_unload_table(u32 table_index)
 	acpi_tb_set_table_loaded_flag(table_index, FALSE);
 	return_ACPI_STATUS(status);
 }
+
+/*******************************************************************************
+ *
+ * FUNCTION:    acpi_tb_notify_table
+ *
+ * PARAMETERS:  event               - Table event
+ *              table               - Validated table pointer
+ *
+ * RETURN:      None
+ *
+ * DESCRIPTION: Notify a table event to the users.
+ *
+ ******************************************************************************/
+
+void acpi_tb_notify_table(u32 event, void *table)
+{
+	/* Invoke table handler if present */
+
+	if (acpi_gbl_table_handler) {
+		(void)acpi_gbl_table_handler(event, table,
+					     acpi_gbl_table_handler_context);
+	}
+}
diff --git a/drivers/acpi/acpica/tbinstal.c b/drivers/acpi/acpica/tbinstal.c
index 4620f3c..ee74515 100644
--- a/drivers/acpi/acpica/tbinstal.c
+++ b/drivers/acpi/acpica/tbinstal.c
@@ -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:
-- 
2.7.4

[RFC PATCH 2/7] ACPICA: Tables: Add sanity check for load_table opcode

[Lv Zheng]

This patch refines table load sanity checks, moving them to
acpi_tb_load_table() so that it can cover load_table opcode. Note that table
index should be updated to the duplicate table according to the current
design (no table uninstall). Reported by Hans de Goede, fixed by Lv Zheng.

Reported-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
Cc: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/acpica/actables.h |   2 +-
 drivers/acpi/acpica/exconfig.c |   2 +-
 drivers/acpi/acpica/tbdata.c   | 174 ++++++++++++++++++++++++++++++++++++++---
 drivers/acpi/acpica/tbinstal.c | 160 +++----------------------------------
 4 files changed, 180 insertions(+), 158 deletions(-)

diff --git a/drivers/acpi/acpica/actables.h b/drivers/acpi/acpica/actables.h
index 89ed31b..1509797 100644
--- a/drivers/acpi/acpica/actables.h
+++ b/drivers/acpi/acpica/actables.h
@@ -124,7 +124,7 @@ acpi_tb_install_standard_table(acpi_physical_address address,
 void acpi_tb_uninstall_table(struct acpi_table_desc *table_desc);
 
 acpi_status
-acpi_tb_load_table(u32 table_index, struct acpi_namespace_node *parent_node);
+acpi_tb_load_table(u32 *table_index, struct acpi_namespace_node *parent_node);
 
 acpi_status
 acpi_tb_install_and_load_table(acpi_physical_address address,
diff --git a/drivers/acpi/acpica/exconfig.c b/drivers/acpi/acpica/exconfig.c
index 61813bd..c82ea9b 100644
--- a/drivers/acpi/acpica/exconfig.c
+++ b/drivers/acpi/acpica/exconfig.c
@@ -197,7 +197,7 @@ acpi_ex_load_table_op(struct acpi_walk_state *walk_state,
 
 	ACPI_INFO(("Dynamic OEM Table Load:"));
 	acpi_ex_exit_interpreter();
-	status = acpi_tb_load_table(table_index, parent_node);
+	status = acpi_tb_load_table(&table_index, parent_node);
 	acpi_ex_enter_interpreter();
 	if (ACPI_FAILURE(status)) {
 		return_ACPI_STATUS(status);
diff --git a/drivers/acpi/acpica/tbdata.c b/drivers/acpi/acpica/tbdata.c
index 42ea044..b7bff76 100644
--- a/drivers/acpi/acpica/tbdata.c
+++ b/drivers/acpi/acpica/tbdata.c
@@ -50,6 +50,14 @@
 #define _COMPONENT          ACPI_TABLES
 ACPI_MODULE_NAME("tbdata")
 
+/* Local prototypes */
+static u8
+acpi_tb_compare_tables(struct acpi_table_desc *table_desc, u32 table_index);
+
+static acpi_status
+acpi_tb_validate_table_for_load(u32 *table_index,
+				struct acpi_table_header **out_table);
+
 /*******************************************************************************
  *
  * FUNCTION:    acpi_tb_init_table_descriptor
@@ -64,6 +72,7 @@ ACPI_MODULE_NAME("tbdata")
  * DESCRIPTION: Initialize a new table descriptor
  *
  ******************************************************************************/
+
 void
 acpi_tb_init_table_descriptor(struct acpi_table_desc *table_desc,
 			      acpi_physical_address address,
@@ -770,6 +779,155 @@ void acpi_tb_set_table_loaded_flag(u32 table_index, u8 is_loaded)
 
 /*******************************************************************************
  *
+ * FUNCTION:    acpi_tb_compare_tables
+ *
+ * PARAMETERS:  table_desc          - Table 1 descriptor to be compared
+ *              table_index         - Index of table 2 to be compared
+ *
+ * RETURN:      TRUE if both tables are identical.
+ *
+ * DESCRIPTION: This function compares a table with another table that has
+ *              already been installed in the root table list.
+ *
+ ******************************************************************************/
+
+static u8
+acpi_tb_compare_tables(struct acpi_table_desc *table_desc, u32 table_index)
+{
+	acpi_status status = AE_OK;
+	u8 is_identical;
+	struct acpi_table_header *table;
+	u32 table_length;
+	u8 table_flags;
+
+	status =
+	    acpi_tb_acquire_table(&acpi_gbl_root_table_list.tables[table_index],
+				  &table, &table_length, &table_flags);
+	if (ACPI_FAILURE(status)) {
+		return (FALSE);
+	}
+
+	/*
+	 * Check for a table match on the entire table length,
+	 * not just the header.
+	 */
+	is_identical = (u8)((table_desc->length != table_length ||
+			     memcmp(table_desc->pointer, table, table_length)) ?
+			    FALSE : TRUE);
+
+	/* Release the acquired table */
+
+	acpi_tb_release_table(table, table_length, table_flags);
+	return (is_identical);
+}
+
+/*******************************************************************************
+ *
+ * FUNCTION:    acpi_tb_validate_table_for_load
+ *
+ * PARAMETERS:  table_index         - Index of table
+ *              out_table           - Validated table
+ *
+ * RETURN:      Status
+ *
+ * DESCRIPTION: This function returns failures if a table cannot be loaded.
+ *              For now, we use special logic to avoid reloading of tables,
+ *              as such, table index may be updated.
+ *
+ ******************************************************************************/
+
+static acpi_status
+acpi_tb_validate_table_for_load(u32 *table_index,
+				struct acpi_table_header **out_table)
+{
+	u32 i;
+	struct acpi_table_desc *table_desc;
+	acpi_status status = AE_OK;
+
+	/* Acquire the table lock */
+
+	(void)acpi_ut_acquire_mutex(ACPI_MTX_TABLES);
+
+	table_desc = &acpi_gbl_root_table_list.tables[*table_index];
+
+	/*
+	 * Note: Now table is "INSTALLED", it must be validated before
+	 * using.
+	 */
+	status = acpi_tb_get_table(table_desc, out_table);
+	if (ACPI_FAILURE(status)) {
+		goto unlock_and_exit;
+	}
+
+	/*
+	 * 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 ((table_desc->signature.ascii[0] != 0x00) &&
+	    (!ACPI_COMPARE_NAME(&table_desc->signature, ACPI_SIG_SSDT)) &&
+	    (strncmp(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(table_desc->signature.
+						       ascii) ? table_desc->
+				 signature.ascii : "????",
+				 table_desc->signature.integer));
+
+		status = AE_BAD_SIGNATURE;
+		goto unlock_and_exit;
+	}
+
+	/* Check if table is already loaded */
+
+	for (i = 0; i < acpi_gbl_root_table_list.current_table_count; ++i) {
+		/*
+		 * Check for a table match on the entire table length, not just the
+		 * header.
+		 */
+		if (!acpi_tb_compare_tables(table_desc, i)) {
+			continue;
+		}
+
+		/*
+		 * Note: the current mechanism does not unregister a table if it is
+		 * dynamically unloaded. The related namespace entries are deleted,
+		 * but the table remains in the root table list.
+		 *
+		 * The assumption here is that the number of different tables that
+		 * will be loaded is actually small, and there is minimal overhead
+		 * in just keeping the table in case it is needed again.
+		 *
+		 * If this assumption changes in the future (perhaps on large
+		 * machines with many table load/unload operations), tables will
+		 * need to be unregistered when they are unloaded, and slots in the
+		 * root table list should be reused when empty.
+		 */
+		*table_index = i;
+		if (acpi_gbl_root_table_list.tables[i].
+		    flags & ACPI_TABLE_IS_LOADED) {
+			status = AE_ALREADY_EXISTS;
+			goto unlock_and_exit;
+		}
+	}
+
+unlock_and_exit:
+
+	/* Release the table lock */
+
+	(void)acpi_ut_release_mutex(ACPI_MTX_TABLES);
+	return (status);
+}
+
+/*******************************************************************************
+ *
  * FUNCTION:    acpi_tb_load_table
  *
  * PARAMETERS:  table_index             - Table index
@@ -782,7 +940,7 @@ void acpi_tb_set_table_loaded_flag(u32 table_index, u8 is_loaded)
  ******************************************************************************/
 
 acpi_status
-acpi_tb_load_table(u32 table_index, struct acpi_namespace_node *parent_node)
+acpi_tb_load_table(u32 *table_index, struct acpi_namespace_node *parent_node)
 {
 	struct acpi_table_header *table;
 	acpi_status status;
@@ -790,16 +948,14 @@ acpi_tb_load_table(u32 table_index, struct acpi_namespace_node *parent_node)
 
 	ACPI_FUNCTION_TRACE(tb_load_table);
 
-	/*
-	 * Note: Now table is "INSTALLED", it must be validated before
-	 * using.
-	 */
-	status = acpi_get_table_by_index(table_index, &table);
+	/* Sanity checks */
+
+	status = acpi_tb_validate_table_for_load(table_index, &table);
 	if (ACPI_FAILURE(status)) {
 		return_ACPI_STATUS(status);
 	}
 
-	status = acpi_ns_load_table(table_index, parent_node);
+	status = acpi_ns_load_table(*table_index, parent_node);
 
 	/* Execute any module-level code that was found in the table */
 
@@ -813,7 +969,7 @@ acpi_tb_load_table(u32 table_index, struct acpi_namespace_node *parent_node)
 	 * responsible for discovering any new wake GPEs by running _PRW methods
 	 * that may have been loaded by this table.
 	 */
-	status = acpi_tb_get_owner_id(table_index, &owner_id);
+	status = acpi_tb_get_owner_id(*table_index, &owner_id);
 	if (ACPI_SUCCESS(status)) {
 		acpi_ev_update_gpes(owner_id);
 	}
@@ -856,7 +1012,7 @@ acpi_tb_install_and_load_table(acpi_physical_address address,
 		goto exit;
 	}
 
-	status = acpi_tb_load_table(i, acpi_gbl_root_node);
+	status = acpi_tb_load_table(&i, acpi_gbl_root_node);
 
 exit:
 	*table_index = i;
diff --git a/drivers/acpi/acpica/tbinstal.c b/drivers/acpi/acpica/tbinstal.c
index ee74515..5e3d445 100644
--- a/drivers/acpi/acpica/tbinstal.c
+++ b/drivers/acpi/acpica/tbinstal.c
@@ -48,54 +48,6 @@
 #define _COMPONENT          ACPI_TABLES
 ACPI_MODULE_NAME("tbinstal")
 
-/* Local prototypes */
-static u8
-acpi_tb_compare_tables(struct acpi_table_desc *table_desc, u32 table_index);
-
-/*******************************************************************************
- *
- * FUNCTION:    acpi_tb_compare_tables
- *
- * PARAMETERS:  table_desc          - Table 1 descriptor to be compared
- *              table_index         - Index of table 2 to be compared
- *
- * RETURN:      TRUE if both tables are identical.
- *
- * DESCRIPTION: This function compares a table with another table that has
- *              already been installed in the root table list.
- *
- ******************************************************************************/
-
-static u8
-acpi_tb_compare_tables(struct acpi_table_desc *table_desc, u32 table_index)
-{
-	acpi_status status = AE_OK;
-	u8 is_identical;
-	struct acpi_table_header *table;
-	u32 table_length;
-	u8 table_flags;
-
-	status =
-	    acpi_tb_acquire_table(&acpi_gbl_root_table_list.tables[table_index],
-				  &table, &table_length, &table_flags);
-	if (ACPI_FAILURE(status)) {
-		return (FALSE);
-	}
-
-	/*
-	 * Check for a table match on the entire table length,
-	 * not just the header.
-	 */
-	is_identical = (u8)((table_desc->length != table_length ||
-			     memcmp(table_desc->pointer, table, table_length)) ?
-			    FALSE : TRUE);
-
-	/* Release the acquired table */
-
-	acpi_tb_release_table(table, table_length, table_flags);
-	return (is_identical);
-}
-
 /*******************************************************************************
  *
  * FUNCTION:    acpi_tb_install_table_with_override
@@ -112,7 +64,6 @@ acpi_tb_compare_tables(struct acpi_table_desc *table_desc, u32 table_index)
  *              table array.
  *
  ******************************************************************************/
-
 void
 acpi_tb_install_table_with_override(struct acpi_table_desc *new_table_desc,
 				    u8 override, u32 *table_index)
@@ -120,11 +71,6 @@ acpi_tb_install_table_with_override(struct acpi_table_desc *new_table_desc,
 	u32 i;
 	acpi_status status;
 
-	status = acpi_tb_get_next_table_descriptor(&i, NULL);
-	if (ACPI_FAILURE(status)) {
-		return;
-	}
-
 	/*
 	 * ACPI Table Override:
 	 *
@@ -136,6 +82,15 @@ acpi_tb_install_table_with_override(struct acpi_table_desc *new_table_desc,
 		acpi_tb_override_table(new_table_desc);
 	}
 
+	/* Acquire the table lock */
+
+	(void)acpi_ut_acquire_mutex(ACPI_MTX_TABLES);
+
+	status = acpi_tb_get_next_table_descriptor(&i, NULL);
+	if (ACPI_FAILURE(status)) {
+		return;
+	}
+
 	acpi_tb_init_table_descriptor(&acpi_gbl_root_table_list.tables[i],
 				      new_table_desc->address,
 				      new_table_desc->flags,
@@ -153,6 +108,10 @@ acpi_tb_install_table_with_override(struct acpi_table_desc *new_table_desc,
 	if (i == acpi_gbl_dsdt_index) {
 		acpi_ut_set_integer_width(new_table_desc->pointer->revision);
 	}
+
+	/* Release the table lock */
+
+	(void)acpi_ut_release_mutex(ACPI_MTX_TABLES);
 }
 
 /*******************************************************************************
@@ -181,7 +140,6 @@ acpi_tb_install_standard_table(acpi_physical_address address,
 			       u8 flags,
 			       u8 reload, u8 override, u32 *table_index)
 {
-	u32 i;
 	acpi_status status = AE_OK;
 	struct acpi_table_desc new_table_desc;
 
@@ -217,90 +175,6 @@ acpi_tb_install_standard_table(acpi_physical_address address,
 		goto release_and_exit;
 	}
 
-	/* Acquire the table lock */
-
-	(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 */
-
-		for (i = 0; i < acpi_gbl_root_table_list.current_table_count;
-		     ++i) {
-			/*
-			 * Check for a table match on the entire table length,
-			 * not just the header.
-			 */
-			if (!acpi_tb_compare_tables(&new_table_desc, i)) {
-				continue;
-			}
-
-			/*
-			 * Note: the current mechanism does not unregister a table if it is
-			 * dynamically unloaded. The related namespace entries are deleted,
-			 * but the table remains in the root table list.
-			 *
-			 * The assumption here is that the number of different tables that
-			 * will be loaded is actually small, and there is minimal overhead
-			 * in just keeping the table in case it is needed again.
-			 *
-			 * If this assumption changes in the future (perhaps on large
-			 * machines with many table load/unload operations), tables will
-			 * need to be unregistered when they are unloaded, and slots in the
-			 * root table list should be reused when empty.
-			 */
-			if (acpi_gbl_root_table_list.tables[i].flags &
-			    ACPI_TABLE_IS_LOADED) {
-
-				/* Table is still loaded, this is an error */
-
-				status = AE_ALREADY_EXISTS;
-				goto unlock_and_exit;
-			} else {
-				/*
-				 * Table was unloaded, allow it to be reloaded.
-				 * As we are going to return AE_OK to the caller, we should
-				 * take the responsibility of freeing the input descriptor.
-				 * Refill the input descriptor to ensure
-				 * acpi_tb_install_table_with_override() can be called again to
-				 * indicate the re-installation.
-				 */
-				acpi_tb_uninstall_table(&new_table_desc);
-				(void)acpi_ut_release_mutex(ACPI_MTX_TABLES);
-				*table_index = i;
-				return_ACPI_STATUS(AE_OK);
-			}
-		}
-	}
-
 	/* Add the table to the global root table list */
 
 	acpi_tb_install_table_with_override(&new_table_desc, override,
@@ -308,15 +182,7 @@ acpi_tb_install_standard_table(acpi_physical_address address,
 
 	/* Invoke table handler */
 
-	(void)acpi_ut_release_mutex(ACPI_MTX_TABLES);
 	acpi_tb_notify_table(ACPI_TABLE_EVENT_INSTALL, new_table_desc.pointer);
-	(void)acpi_ut_acquire_mutex(ACPI_MTX_TABLES);
-
-unlock_and_exit:
-
-	/* Release the table lock */
-
-	(void)acpi_ut_release_mutex(ACPI_MTX_TABLES);
 
 release_and_exit:
 
-- 
2.7.4

[RFC PATCH 3/7] ACPICA: Tables: Add sanity check for table install

[Lv Zheng]

This patch adds sanity check for table install process, avoid installing
same tables to the global table list. Lv Zheng.

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
Cc: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/acpica/actables.h |  2 +-
 drivers/acpi/acpica/tbinstal.c | 33 +++++++++++++++++++++++++++------
 include/acpi/actbl.h           |  1 +
 3 files changed, 29 insertions(+), 7 deletions(-)

diff --git a/drivers/acpi/acpica/actables.h b/drivers/acpi/acpica/actables.h
index 1509797..db78af4 100644
--- a/drivers/acpi/acpica/actables.h
+++ b/drivers/acpi/acpica/actables.h
@@ -162,7 +162,7 @@ void acpi_tb_check_dsdt_header(void);
 
 struct acpi_table_header *acpi_tb_copy_dsdt(u32 table_index);
 
-void
+acpi_status
 acpi_tb_install_table_with_override(struct acpi_table_desc *new_table_desc,
 				    u8 override, u32 *table_index);
 
diff --git a/drivers/acpi/acpica/tbinstal.c b/drivers/acpi/acpica/tbinstal.c
index 5e3d445..fa82962 100644
--- a/drivers/acpi/acpica/tbinstal.c
+++ b/drivers/acpi/acpica/tbinstal.c
@@ -56,7 +56,7 @@ ACPI_MODULE_NAME("tbinstal")
  *              override                - Whether override should be performed
  *              table_index             - Where the table index is returned
  *
- * RETURN:      None
+ * RETURN:      Status
  *
  * DESCRIPTION: Install an ACPI table into the global data structure. The
  *              table override mechanism is called to allow the host
@@ -64,12 +64,13 @@ ACPI_MODULE_NAME("tbinstal")
  *              table array.
  *
  ******************************************************************************/
-void
+acpi_status
 acpi_tb_install_table_with_override(struct acpi_table_desc *new_table_desc,
 				    u8 override, u32 *table_index)
 {
 	u32 i;
-	acpi_status status;
+	acpi_status status = AE_OK;
+	struct acpi_table_desc *table_desc;
 
 	/*
 	 * ACPI Table Override:
@@ -86,9 +87,22 @@ acpi_tb_install_table_with_override(struct acpi_table_desc *new_table_desc,
 
 	(void)acpi_ut_acquire_mutex(ACPI_MTX_TABLES);
 
+	/* Check if the table has already been installed */
+
+	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->address &&
+		    new_table_desc->address == table_desc->address &&
+		    ACPI_TABLE_ORIGIN(new_table_desc) ==
+		    ACPI_TABLE_ORIGIN(table_desc)) {
+			*table_index = i;
+			goto unlock_and_exit;
+		}
+	}
+
 	status = acpi_tb_get_next_table_descriptor(&i, NULL);
 	if (ACPI_FAILURE(status)) {
-		return;
+		goto unlock_and_exit;
 	}
 
 	acpi_tb_init_table_descriptor(&acpi_gbl_root_table_list.tables[i],
@@ -109,9 +123,12 @@ acpi_tb_install_table_with_override(struct acpi_table_desc *new_table_desc,
 		acpi_ut_set_integer_width(new_table_desc->pointer->revision);
 	}
 
+unlock_and_exit:
+
 	/* Release the table lock */
 
 	(void)acpi_ut_release_mutex(ACPI_MTX_TABLES);
+	return (status);
 }
 
 /*******************************************************************************
@@ -177,8 +194,12 @@ acpi_tb_install_standard_table(acpi_physical_address address,
 
 	/* Add the table to the global root table list */
 
-	acpi_tb_install_table_with_override(&new_table_desc, override,
-					    table_index);
+	status =
+	    acpi_tb_install_table_with_override(&new_table_desc, override,
+						table_index);
+	if (ACPI_FAILURE(status)) {
+		goto release_and_exit;
+	}
 
 	/* Invoke table handler */
 
diff --git a/include/acpi/actbl.h b/include/acpi/actbl.h
index d92543f..7ebaa07 100644
--- a/include/acpi/actbl.h
+++ b/include/acpi/actbl.h
@@ -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_ORIGIN(desc)             ((desc)->flags & ACPI_TABLE_ORIGIN_MASK)
 #define ACPI_TABLE_IS_LOADED                (8)
 
 /*
-- 
2.7.4

[RFC PATCH 4/7] ACPICA: Tables: Fix an unwanted exception for table reloading

[Lv Zheng]

When Load/load_table opcode is called again for the same table, it is likely
that the table has already been loaded, so AE_ALREADY_EXISTS shouldn't be a
fatal error but a notice for the caller to skip acpi_ns_load_table(). Reported
by Hans de Goede, fixed by Lv Zheng.

Reported-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
Cc: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/acpica/tbdata.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/acpi/acpica/tbdata.c b/drivers/acpi/acpica/tbdata.c
index b7bff76..1a4371b 100644
--- a/drivers/acpi/acpica/tbdata.c
+++ b/drivers/acpi/acpica/tbdata.c
@@ -952,6 +952,13 @@ acpi_tb_load_table(u32 *table_index, struct acpi_namespace_node *parent_node)
 
 	status = acpi_tb_validate_table_for_load(table_index, &table);
 	if (ACPI_FAILURE(status)) {
+		if (status == AE_ALREADY_EXISTS) {
+			/*
+			 * Skip the table loading process but return the table index to
+			 * the caller with the exception muted.
+			 */
+			status = AE_OK;
+		}
 		return_ACPI_STATUS(status);
 	}
 
-- 
2.7.4

[RFC PATCH 5/7] ACPICA: Tables: Enable acpi_ut_is_aml_table() for all ACPICA modules

[Lv Zheng]

This patch enables acpi_ut_is_aml_table() for all ACPICA modules so that it can
be used by Linux kernel. Lv Zheng.

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
Cc: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/acpica/acutils.h  |  4 +---
 drivers/acpi/acpica/tbdata.c   | 17 +++--------------
 drivers/acpi/acpica/tbxfload.c | 10 +++-------
 drivers/acpi/acpica/utmisc.c   | 27 +++++++++++++++++----------
 4 files changed, 24 insertions(+), 34 deletions(-)

diff --git a/drivers/acpi/acpica/acutils.h b/drivers/acpi/acpica/acutils.h
index 6f28cfa..c0b697f 100644
--- a/drivers/acpi/acpica/acutils.h
+++ b/drivers/acpi/acpica/acutils.h
@@ -545,9 +545,7 @@ const struct acpi_exception_info *acpi_ut_validate_exception(acpi_status
 
 u8 acpi_ut_is_pci_root_bridge(char *id);
 
-#if (defined ACPI_ASL_COMPILER || defined ACPI_EXEC_APP || defined ACPI_NAMES_APP)
-u8 acpi_ut_is_aml_table(struct acpi_table_header *table);
-#endif
+u8 acpi_ut_is_aml_table(char *signature);
 
 acpi_status
 acpi_ut_walk_package_tree(union acpi_operand_object *source_object,
diff --git a/drivers/acpi/acpica/tbdata.c b/drivers/acpi/acpica/tbdata.c
index 1a4371b..1e1222d 100644
--- a/drivers/acpi/acpica/tbdata.c
+++ b/drivers/acpi/acpica/tbdata.c
@@ -859,20 +859,9 @@ acpi_tb_validate_table_for_load(u32 *table_index,
 		goto unlock_and_exit;
 	}
 
-	/*
-	 * 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 ((table_desc->signature.ascii[0] != 0x00) &&
-	    (!ACPI_COMPARE_NAME(&table_desc->signature, ACPI_SIG_SSDT)) &&
-	    (strncmp(table_desc->signature.ascii, "OEM", 3))) {
+	/* Validate the incoming table signature */
+
+	if (!acpi_ut_is_aml_table(table_desc->signature.ascii)) {
 		ACPI_BIOS_ERROR((AE_INFO,
 				 "Table has invalid signature [%4.4s] (0x%8.8X), "
 				 "must be SSDT or OEMx",
diff --git a/drivers/acpi/acpica/tbxfload.c b/drivers/acpi/acpica/tbxfload.c
index b71ce3b..9753176 100644
--- a/drivers/acpi/acpica/tbxfload.c
+++ b/drivers/acpi/acpica/tbxfload.c
@@ -206,13 +206,9 @@ 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 ||
-		    (!ACPI_COMPARE_NAME(table->signature.ascii, ACPI_SIG_SSDT)
-		     && !ACPI_COMPARE_NAME(table->signature.ascii,
-					   ACPI_SIG_PSDT)
-		     && !ACPI_COMPARE_NAME(table->signature.ascii,
-					   ACPI_SIG_OSDT))
-		    || ACPI_FAILURE(acpi_tb_validate_table(table))) {
+		if (!table->address || i == acpi_gbl_dsdt_index ||
+		    !acpi_ut_is_aml_table(table->signature.ascii) ||
+		    ACPI_FAILURE(acpi_tb_validate_table(table))) {
 			continue;
 		}
 
diff --git a/drivers/acpi/acpica/utmisc.c b/drivers/acpi/acpica/utmisc.c
index 443ffad..e5074d8 100644
--- a/drivers/acpi/acpica/utmisc.c
+++ b/drivers/acpi/acpica/utmisc.c
@@ -75,7 +75,6 @@ u8 acpi_ut_is_pci_root_bridge(char *id)
 	return (FALSE);
 }
 
-#if (defined ACPI_ASL_COMPILER || defined ACPI_EXEC_APP || defined ACPI_NAMES_APP)
 /*******************************************************************************
  *
  * FUNCTION:    acpi_ut_is_aml_table
@@ -90,21 +89,29 @@ u8 acpi_ut_is_pci_root_bridge(char *id)
  *
  ******************************************************************************/
 
-u8 acpi_ut_is_aml_table(struct acpi_table_header *table)
+u8 acpi_ut_is_aml_table(char *signature)
 {
-
-	/* These are the only tables that contain executable AML */
-
-	if (ACPI_COMPARE_NAME(table->signature, ACPI_SIG_DSDT) ||
-	    ACPI_COMPARE_NAME(table->signature, ACPI_SIG_PSDT) ||
-	    ACPI_COMPARE_NAME(table->signature, ACPI_SIG_SSDT) ||
-	    ACPI_COMPARE_NAME(table->signature, ACPI_SIG_OSDT)) {
+	/*
+	 * These are the only tables that contain executable AML
+	 * 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 (signature[0] == 0x00 ||
+	    ACPI_COMPARE_NAME(signature, ACPI_SIG_DSDT) ||
+	    ACPI_COMPARE_NAME(signature, ACPI_SIG_PSDT) ||
+	    ACPI_COMPARE_NAME(signature, ACPI_SIG_SSDT) ||
+	    ACPI_COMPARE_NAME(signature, ACPI_SIG_OSDT) ||
+	    strncmp(signature, "OEM", 3) == 0) {
 		return (TRUE);
 	}
 
 	return (FALSE);
 }
-#endif
 
 /*******************************************************************************
  *
-- 
2.7.4

RE: [RFC PATCH 5/7] ACPICA: Tables: Enable acpi_ut_is_aml_table() for all ACPICA modules

[Zheng, Lv]

Hi, Hans

Just FYI.
This commit is wrong.
It shouldn't change static table load signature check.
Should be replaced by this fix:
https://github.com/acpica/acpica/pull/121/commits/3d9eeb046c
Thanks Xiaolong for his test and report.

Thanks and best regards
Lv

> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Lv Zheng
> Subject: [RFC PATCH 5/7] ACPICA: Tables: Enable acpi_ut_is_aml_table() for all ACPICA modules
> 
> This patch enables acpi_ut_is_aml_table() for all ACPICA modules so that it can
> be used by Linux kernel. Lv Zheng.
> 
> Signed-off-by: Lv Zheng <lv.zheng@intel.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/acpi/acpica/acutils.h  |  4 +---
>  drivers/acpi/acpica/tbdata.c   | 17 +++--------------
>  drivers/acpi/acpica/tbxfload.c | 10 +++-------
>  drivers/acpi/acpica/utmisc.c   | 27 +++++++++++++++++----------
>  4 files changed, 24 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/acpi/acpica/acutils.h b/drivers/acpi/acpica/acutils.h
> index 6f28cfa..c0b697f 100644
> --- a/drivers/acpi/acpica/acutils.h
> +++ b/drivers/acpi/acpica/acutils.h
> @@ -545,9 +545,7 @@ const struct acpi_exception_info *acpi_ut_validate_exception(acpi_status
> 
>  u8 acpi_ut_is_pci_root_bridge(char *id);
> 
> -#if (defined ACPI_ASL_COMPILER || defined ACPI_EXEC_APP || defined ACPI_NAMES_APP)
> -u8 acpi_ut_is_aml_table(struct acpi_table_header *table);
> -#endif
> +u8 acpi_ut_is_aml_table(char *signature);
> 
>  acpi_status
>  acpi_ut_walk_package_tree(union acpi_operand_object *source_object,
> diff --git a/drivers/acpi/acpica/tbdata.c b/drivers/acpi/acpica/tbdata.c
> index 1a4371b..1e1222d 100644
> --- a/drivers/acpi/acpica/tbdata.c
> +++ b/drivers/acpi/acpica/tbdata.c
> @@ -859,20 +859,9 @@ acpi_tb_validate_table_for_load(u32 *table_index,
>  		goto unlock_and_exit;
>  	}
> 
> -	/*
> -	 * 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 ((table_desc->signature.ascii[0] != 0x00) &&
> -	    (!ACPI_COMPARE_NAME(&table_desc->signature, ACPI_SIG_SSDT)) &&
> -	    (strncmp(table_desc->signature.ascii, "OEM", 3))) {
> +	/* Validate the incoming table signature */
> +
> +	if (!acpi_ut_is_aml_table(table_desc->signature.ascii)) {
>  		ACPI_BIOS_ERROR((AE_INFO,
>  				 "Table has invalid signature [%4.4s] (0x%8.8X), "
>  				 "must be SSDT or OEMx",
> diff --git a/drivers/acpi/acpica/tbxfload.c b/drivers/acpi/acpica/tbxfload.c
> index b71ce3b..9753176 100644
> --- a/drivers/acpi/acpica/tbxfload.c
> +++ b/drivers/acpi/acpica/tbxfload.c
> @@ -206,13 +206,9 @@ 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 ||
> -		    (!ACPI_COMPARE_NAME(table->signature.ascii, ACPI_SIG_SSDT)
> -		     && !ACPI_COMPARE_NAME(table->signature.ascii,
> -					   ACPI_SIG_PSDT)
> -		     && !ACPI_COMPARE_NAME(table->signature.ascii,
> -					   ACPI_SIG_OSDT))
> -		    || ACPI_FAILURE(acpi_tb_validate_table(table))) {
> +		if (!table->address || i == acpi_gbl_dsdt_index ||
> +		    !acpi_ut_is_aml_table(table->signature.ascii) ||
> +		    ACPI_FAILURE(acpi_tb_validate_table(table))) {
>  			continue;
>  		}
> 
> diff --git a/drivers/acpi/acpica/utmisc.c b/drivers/acpi/acpica/utmisc.c
> index 443ffad..e5074d8 100644
> --- a/drivers/acpi/acpica/utmisc.c
> +++ b/drivers/acpi/acpica/utmisc.c
> @@ -75,7 +75,6 @@ u8 acpi_ut_is_pci_root_bridge(char *id)
>  	return (FALSE);
>  }
> 
> -#if (defined ACPI_ASL_COMPILER || defined ACPI_EXEC_APP || defined ACPI_NAMES_APP)
>  /*******************************************************************************
>   *
>   * FUNCTION:    acpi_ut_is_aml_table
> @@ -90,21 +89,29 @@ u8 acpi_ut_is_pci_root_bridge(char *id)
>   *
>   ******************************************************************************/
> 
> -u8 acpi_ut_is_aml_table(struct acpi_table_header *table)
> +u8 acpi_ut_is_aml_table(char *signature)
>  {
> -
> -	/* These are the only tables that contain executable AML */
> -
> -	if (ACPI_COMPARE_NAME(table->signature, ACPI_SIG_DSDT) ||
> -	    ACPI_COMPARE_NAME(table->signature, ACPI_SIG_PSDT) ||
> -	    ACPI_COMPARE_NAME(table->signature, ACPI_SIG_SSDT) ||
> -	    ACPI_COMPARE_NAME(table->signature, ACPI_SIG_OSDT)) {
> +	/*
> +	 * These are the only tables that contain executable AML
> +	 * 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 (signature[0] == 0x00 ||
> +	    ACPI_COMPARE_NAME(signature, ACPI_SIG_DSDT) ||
> +	    ACPI_COMPARE_NAME(signature, ACPI_SIG_PSDT) ||
> +	    ACPI_COMPARE_NAME(signature, ACPI_SIG_SSDT) ||
> +	    ACPI_COMPARE_NAME(signature, ACPI_SIG_OSDT) ||
> +	    strncmp(signature, "OEM", 3) == 0) {
>  		return (TRUE);
>  	}
> 
>  	return (FALSE);
>  }
> -#endif
> 
>  /*******************************************************************************
>   *
> --
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 5/7] ACPICA: Tables: Enable acpi_ut_is_aml_table() for all ACPICA modules

[Ye Xiaolong]

On 03/22, Zheng, Lv wrote:
>Hi, Hans
>
>Just FYI.
>This commit is wrong.
>It shouldn't change static table load signature check.
>Should be replaced by this fix:
>https://github.com/acpica/acpica/pull/121/commits/3d9eeb046c
>Thanks Xiaolong for his test and report.

I'm glad it helps. :)

Thanks,
Xiaolong
>
>Thanks and best regards
>Lv
>
>> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Lv Zheng
>> Subject: [RFC PATCH 5/7] ACPICA: Tables: Enable acpi_ut_is_aml_table() for all ACPICA modules
>> 
>> This patch enables acpi_ut_is_aml_table() for all ACPICA modules so that it can
>> be used by Linux kernel. Lv Zheng.
>> 
>> Signed-off-by: Lv Zheng <lv.zheng@intel.com>
>> Cc: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/acpi/acpica/acutils.h  |  4 +---
>>  drivers/acpi/acpica/tbdata.c   | 17 +++--------------
>>  drivers/acpi/acpica/tbxfload.c | 10 +++-------
>>  drivers/acpi/acpica/utmisc.c   | 27 +++++++++++++++++----------
>>  4 files changed, 24 insertions(+), 34 deletions(-)
>> 
>> diff --git a/drivers/acpi/acpica/acutils.h b/drivers/acpi/acpica/acutils.h
>> index 6f28cfa..c0b697f 100644
>> --- a/drivers/acpi/acpica/acutils.h
>> +++ b/drivers/acpi/acpica/acutils.h
>> @@ -545,9 +545,7 @@ const struct acpi_exception_info *acpi_ut_validate_exception(acpi_status
>> 
>>  u8 acpi_ut_is_pci_root_bridge(char *id);
>> 
>> -#if (defined ACPI_ASL_COMPILER || defined ACPI_EXEC_APP || defined ACPI_NAMES_APP)
>> -u8 acpi_ut_is_aml_table(struct acpi_table_header *table);
>> -#endif
>> +u8 acpi_ut_is_aml_table(char *signature);
>> 
>>  acpi_status
>>  acpi_ut_walk_package_tree(union acpi_operand_object *source_object,
>> diff --git a/drivers/acpi/acpica/tbdata.c b/drivers/acpi/acpica/tbdata.c
>> index 1a4371b..1e1222d 100644
>> --- a/drivers/acpi/acpica/tbdata.c
>> +++ b/drivers/acpi/acpica/tbdata.c
>> @@ -859,20 +859,9 @@ acpi_tb_validate_table_for_load(u32 *table_index,
>>  		goto unlock_and_exit;
>>  	}
>> 
>> -	/*
>> -	 * 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 ((table_desc->signature.ascii[0] != 0x00) &&
>> -	    (!ACPI_COMPARE_NAME(&table_desc->signature, ACPI_SIG_SSDT)) &&
>> -	    (strncmp(table_desc->signature.ascii, "OEM", 3))) {
>> +	/* Validate the incoming table signature */
>> +
>> +	if (!acpi_ut_is_aml_table(table_desc->signature.ascii)) {
>>  		ACPI_BIOS_ERROR((AE_INFO,
>>  				 "Table has invalid signature [%4.4s] (0x%8.8X), "
>>  				 "must be SSDT or OEMx",
>> diff --git a/drivers/acpi/acpica/tbxfload.c b/drivers/acpi/acpica/tbxfload.c
>> index b71ce3b..9753176 100644
>> --- a/drivers/acpi/acpica/tbxfload.c
>> +++ b/drivers/acpi/acpica/tbxfload.c
>> @@ -206,13 +206,9 @@ 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 ||
>> -		    (!ACPI_COMPARE_NAME(table->signature.ascii, ACPI_SIG_SSDT)
>> -		     && !ACPI_COMPARE_NAME(table->signature.ascii,
>> -					   ACPI_SIG_PSDT)
>> -		     && !ACPI_COMPARE_NAME(table->signature.ascii,
>> -					   ACPI_SIG_OSDT))
>> -		    || ACPI_FAILURE(acpi_tb_validate_table(table))) {
>> +		if (!table->address || i == acpi_gbl_dsdt_index ||
>> +		    !acpi_ut_is_aml_table(table->signature.ascii) ||
>> +		    ACPI_FAILURE(acpi_tb_validate_table(table))) {
>>  			continue;
>>  		}
>> 
>> diff --git a/drivers/acpi/acpica/utmisc.c b/drivers/acpi/acpica/utmisc.c
>> index 443ffad..e5074d8 100644
>> --- a/drivers/acpi/acpica/utmisc.c
>> +++ b/drivers/acpi/acpica/utmisc.c
>> @@ -75,7 +75,6 @@ u8 acpi_ut_is_pci_root_bridge(char *id)
>>  	return (FALSE);
>>  }
>> 
>> -#if (defined ACPI_ASL_COMPILER || defined ACPI_EXEC_APP || defined ACPI_NAMES_APP)
>>  /*******************************************************************************
>>   *
>>   * FUNCTION:    acpi_ut_is_aml_table
>> @@ -90,21 +89,29 @@ u8 acpi_ut_is_pci_root_bridge(char *id)
>>   *
>>   ******************************************************************************/
>> 
>> -u8 acpi_ut_is_aml_table(struct acpi_table_header *table)
>> +u8 acpi_ut_is_aml_table(char *signature)
>>  {
>> -
>> -	/* These are the only tables that contain executable AML */
>> -
>> -	if (ACPI_COMPARE_NAME(table->signature, ACPI_SIG_DSDT) ||
>> -	    ACPI_COMPARE_NAME(table->signature, ACPI_SIG_PSDT) ||
>> -	    ACPI_COMPARE_NAME(table->signature, ACPI_SIG_SSDT) ||
>> -	    ACPI_COMPARE_NAME(table->signature, ACPI_SIG_OSDT)) {
>> +	/*
>> +	 * These are the only tables that contain executable AML
>> +	 * 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 (signature[0] == 0x00 ||
>> +	    ACPI_COMPARE_NAME(signature, ACPI_SIG_DSDT) ||
>> +	    ACPI_COMPARE_NAME(signature, ACPI_SIG_PSDT) ||
>> +	    ACPI_COMPARE_NAME(signature, ACPI_SIG_SSDT) ||
>> +	    ACPI_COMPARE_NAME(signature, ACPI_SIG_OSDT) ||
>> +	    strncmp(signature, "OEM", 3) == 0) {
>>  		return (TRUE);
>>  	}
>> 
>>  	return (FALSE);
>>  }
>> -#endif
>> 
>>  /*******************************************************************************
>>   *
>> --
>> 2.7.4
>> 
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC PATCH 6/7] ACPICA: Tables: Add sanity check for static table load

[Lv Zheng]

This patch extends sanity check to static table load. Also enables
acpi_ut_is_aml_table() for Linux kernel. This patch achieves this by invoking
acpi_tb_load_table() instead of acpi_ns_load_table().

Some redundant code blocks are therefore reduced:
1. acpi_tb_validate_table() is replaced by acpi_get_table_by_index() invoked in
   acpi_tb_validate_table_for_load(), DSDT handling code is thus moved to this
   function as it must be invoked for validated tables;
2. MLC handling in both acpi_ns_load_table() and acpi_tb_load_table() can be
   combined into acpi_tb_load_table(), Reload parameter is added to
   acpi_tb_load_table() in order to distiguish different table loading stages
   to keep group MLC logics.
Reported by Hans de Goede, fixed by Lv Zheng.

Reported-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
Cc: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/acpica/actables.h |  3 +-
 drivers/acpi/acpica/exconfig.c |  2 +-
 drivers/acpi/acpica/nsload.c   | 18 -----------
 drivers/acpi/acpica/tbdata.c   | 69 +++++++++++++++++++++++++++++++++---------
 drivers/acpi/acpica/tbxfload.c | 40 +++---------------------
 5 files changed, 63 insertions(+), 69 deletions(-)

diff --git a/drivers/acpi/acpica/actables.h b/drivers/acpi/acpica/actables.h
index db78af4..e478576 100644
--- a/drivers/acpi/acpica/actables.h
+++ b/drivers/acpi/acpica/actables.h
@@ -124,7 +124,8 @@ acpi_tb_install_standard_table(acpi_physical_address address,
 void acpi_tb_uninstall_table(struct acpi_table_desc *table_desc);
 
 acpi_status
-acpi_tb_load_table(u32 *table_index, struct acpi_namespace_node *parent_node);
+acpi_tb_load_table(u32 *table_index,
+		   u8 reload, struct acpi_namespace_node *parent_node);
 
 acpi_status
 acpi_tb_install_and_load_table(acpi_physical_address address,
diff --git a/drivers/acpi/acpica/exconfig.c b/drivers/acpi/acpica/exconfig.c
index c82ea9b..a95c71d 100644
--- a/drivers/acpi/acpica/exconfig.c
+++ b/drivers/acpi/acpica/exconfig.c
@@ -197,7 +197,7 @@ acpi_ex_load_table_op(struct acpi_walk_state *walk_state,
 
 	ACPI_INFO(("Dynamic OEM Table Load:"));
 	acpi_ex_exit_interpreter();
-	status = acpi_tb_load_table(&table_index, parent_node);
+	status = acpi_tb_load_table(&table_index, TRUE, parent_node);
 	acpi_ex_enter_interpreter();
 	if (ACPI_FAILURE(status)) {
 		return_ACPI_STATUS(status);
diff --git a/drivers/acpi/acpica/nsload.c b/drivers/acpi/acpica/nsload.c
index d2915e1..1f3163a 100644
--- a/drivers/acpi/acpica/nsload.c
+++ b/drivers/acpi/acpica/nsload.c
@@ -144,24 +144,6 @@ acpi_ns_load_table(u32 table_index, struct acpi_namespace_node *node)
 	ACPI_DEBUG_PRINT((ACPI_DB_INFO,
 			  "**** Completed Table Object Initialization\n"));
 
-	/*
-	 * Execute any module-level code that was detected during the table load
-	 * phase. Although illegal since ACPI 2.0, there are many machines that
-	 * contain this type of code. Each block of detected executable AML code
-	 * outside of any control method is wrapped with a temporary control
-	 * method object and placed on a global list. The methods on this list
-	 * are executed below.
-	 *
-	 * This case executes the module-level code for each table immediately
-	 * after the table has been loaded. This provides compatibility with
-	 * other ACPI implementations. Optionally, the execution can be deferred
-	 * until later, see acpi_initialize_objects.
-	 */
-	if (!acpi_gbl_parse_table_as_term_list
-	    && !acpi_gbl_group_module_level_code) {
-		acpi_ns_exec_module_code_list();
-	}
-
 	return_ACPI_STATUS(status);
 }
 
diff --git a/drivers/acpi/acpica/tbdata.c b/drivers/acpi/acpica/tbdata.c
index 1e1222d..180c7a3 100644
--- a/drivers/acpi/acpica/tbdata.c
+++ b/drivers/acpi/acpica/tbdata.c
@@ -843,6 +843,7 @@ acpi_tb_validate_table_for_load(u32 *table_index,
 	u32 i;
 	struct acpi_table_desc *table_desc;
 	acpi_status status = AE_OK;
+	struct acpi_table_header *new_dsdt;
 
 	/* Acquire the table lock */
 
@@ -859,6 +860,37 @@ acpi_tb_validate_table_for_load(u32 *table_index,
 		goto unlock_and_exit;
 	}
 
+	if (*table_index == acpi_gbl_dsdt_index) {
+		/*
+		 * Save the DSDT pointer for simple access. This is the mapped memory
+		 * address. We must take care here because the address of the .Tables
+		 * array can change dynamically as tables are loaded at run-time.
+		 * Note: .Pointer field is not validated until after call to
+		 * acpi_get_table_by_index().
+		 */
+		acpi_gbl_DSDT = table_desc->pointer;
+
+		/*
+		 * Optionally copy the entire DSDT to local memory (instead of simply
+		 * mapping it.) There are some BIOSs that corrupt or replace the
+		 * original DSDT, creating the need for this option. Default is FALSE,
+		 * do not copy the DSDT.
+		 */
+		if (acpi_gbl_copy_dsdt_locally) {
+			new_dsdt = acpi_tb_copy_dsdt(acpi_gbl_dsdt_index);
+			if (new_dsdt) {
+				acpi_gbl_DSDT = new_dsdt;
+			}
+		}
+
+		/*
+		 * Save the original DSDT header for detection of table corruption
+		 * and/or replacement of the DSDT from outside the OS.
+		 */
+		memcpy(&acpi_gbl_original_dsdt_header, acpi_gbl_DSDT,
+		       sizeof(struct acpi_table_header));
+	}
+
 	/* Validate the incoming table signature */
 
 	if (!acpi_ut_is_aml_table(table_desc->signature.ascii)) {
@@ -920,6 +952,7 @@ acpi_tb_validate_table_for_load(u32 *table_index,
  * FUNCTION:    acpi_tb_load_table
  *
  * PARAMETERS:  table_index             - Table index
+ *              reload                  - Whether reload should be performed
  *              parent_node             - Where table index is returned
  *
  * RETURN:      Status
@@ -929,7 +962,8 @@ acpi_tb_validate_table_for_load(u32 *table_index,
  ******************************************************************************/
 
 acpi_status
-acpi_tb_load_table(u32 *table_index, struct acpi_namespace_node *parent_node)
+acpi_tb_load_table(u32 *table_index,
+		   u8 reload, struct acpi_namespace_node *parent_node)
 {
 	struct acpi_table_header *table;
 	acpi_status status;
@@ -953,21 +987,28 @@ acpi_tb_load_table(u32 *table_index, struct acpi_namespace_node *parent_node)
 
 	status = acpi_ns_load_table(*table_index, parent_node);
 
-	/* Execute any module-level code that was found in the table */
-
-	if (!acpi_gbl_parse_table_as_term_list
-	    && acpi_gbl_group_module_level_code) {
+	/*
+	 * Execute any module-level code that was found in the table. This only
+	 * applies when new grammar support is not set (as MLC are executed in
+	 * place) and dynamic table loading. For static table loading, executes
+	 * MLC here if group module level code is not set, otherwise, it is
+	 * executed in acpi_initialize_objects().
+	 */
+	if (!acpi_gbl_parse_table_as_term_list &&
+	    (reload || !acpi_gbl_group_module_level_code)) {
 		acpi_ns_exec_module_code_list();
 	}
 
-	/*
-	 * Update GPEs for any new _Lxx/_Exx methods. Ignore errors. The host is
-	 * responsible for discovering any new wake GPEs by running _PRW methods
-	 * that may have been loaded by this table.
-	 */
-	status = acpi_tb_get_owner_id(*table_index, &owner_id);
-	if (ACPI_SUCCESS(status)) {
-		acpi_ev_update_gpes(owner_id);
+	if (reload) {
+		/*
+		 * Update GPEs for any new _Lxx/_Exx methods. Ignore errors. The host
+		 * is responsible for discovering any new wake GPEs by running _PRW
+		 * methods that may have been loaded by this table.
+		 */
+		status = acpi_tb_get_owner_id(*table_index, &owner_id);
+		if (ACPI_SUCCESS(status)) {
+			acpi_ev_update_gpes(owner_id);
+		}
 	}
 
 	/* Invoke table handler */
@@ -1008,7 +1049,7 @@ acpi_tb_install_and_load_table(acpi_physical_address address,
 		goto exit;
 	}
 
-	status = acpi_tb_load_table(&i, acpi_gbl_root_node);
+	status = acpi_tb_load_table(&i, TRUE, acpi_gbl_root_node);
 
 exit:
 	*table_index = i;
diff --git a/drivers/acpi/acpica/tbxfload.c b/drivers/acpi/acpica/tbxfload.c
index 9753176..76f4c1b 100644
--- a/drivers/acpi/acpica/tbxfload.c
+++ b/drivers/acpi/acpica/tbxfload.c
@@ -139,7 +139,6 @@ acpi_status acpi_tb_load_namespace(void)
 {
 	acpi_status status;
 	u32 i;
-	struct acpi_table_header *new_dsdt;
 	struct acpi_table_desc *table;
 	u32 tables_loaded = 0;
 	u32 tables_failed = 0;
@@ -155,44 +154,16 @@ acpi_status acpi_tb_load_namespace(void)
 	table = &acpi_gbl_root_table_list.tables[acpi_gbl_dsdt_index];
 
 	if (!acpi_gbl_root_table_list.current_table_count ||
-	    !ACPI_COMPARE_NAME(table->signature.ascii, ACPI_SIG_DSDT) ||
-	    ACPI_FAILURE(acpi_tb_validate_table(table))) {
+	    !ACPI_COMPARE_NAME(table->signature.ascii, ACPI_SIG_DSDT)) {
 		status = AE_NO_ACPI_TABLES;
 		goto unlock_and_exit;
 	}
 
-	/*
-	 * Save the DSDT pointer for simple access. This is the mapped memory
-	 * address. We must take care here because the address of the .Tables
-	 * array can change dynamically as tables are loaded at run-time. Note:
-	 * .Pointer field is not validated until after call to acpi_tb_validate_table.
-	 */
-	acpi_gbl_DSDT = table->pointer;
-
-	/*
-	 * Optionally copy the entire DSDT to local memory (instead of simply
-	 * mapping it.) There are some BIOSs that corrupt or replace the original
-	 * DSDT, creating the need for this option. Default is FALSE, do not copy
-	 * the DSDT.
-	 */
-	if (acpi_gbl_copy_dsdt_locally) {
-		new_dsdt = acpi_tb_copy_dsdt(acpi_gbl_dsdt_index);
-		if (new_dsdt) {
-			acpi_gbl_DSDT = new_dsdt;
-		}
-	}
-
-	/*
-	 * Save the original DSDT header for detection of table corruption
-	 * and/or replacement of the DSDT from outside the OS.
-	 */
-	memcpy(&acpi_gbl_original_dsdt_header, acpi_gbl_DSDT,
-	       sizeof(struct acpi_table_header));
-
 	/* Load and parse tables */
 
 	(void)acpi_ut_release_mutex(ACPI_MTX_TABLES);
-	status = acpi_ns_load_table(acpi_gbl_dsdt_index, acpi_gbl_root_node);
+	status =
+	    acpi_tb_load_table(&acpi_gbl_dsdt_index, FALSE, acpi_gbl_root_node);
 	(void)acpi_ut_acquire_mutex(ACPI_MTX_TABLES);
 	if (ACPI_FAILURE(status)) {
 		ACPI_EXCEPTION((AE_INFO, status, "[DSDT] table load failed"));
@@ -207,15 +178,14 @@ acpi_status acpi_tb_load_namespace(void)
 		table = &acpi_gbl_root_table_list.tables[i];
 
 		if (!table->address || i == acpi_gbl_dsdt_index ||
-		    !acpi_ut_is_aml_table(table->signature.ascii) ||
-		    ACPI_FAILURE(acpi_tb_validate_table(table))) {
+		    !acpi_ut_is_aml_table(table->signature.ascii)) {
 			continue;
 		}
 
 		/* Ignore errors while loading tables, get as many as possible */
 
 		(void)acpi_ut_release_mutex(ACPI_MTX_TABLES);
-		status = acpi_ns_load_table(i, acpi_gbl_root_node);
+		status = acpi_tb_load_table(&i, FALSE, acpi_gbl_root_node);
 		(void)acpi_ut_acquire_mutex(ACPI_MTX_TABLES);
 		if (ACPI_FAILURE(status)) {
 			ACPI_EXCEPTION((AE_INFO, status,
-- 
2.7.4

[RFC PATCH 7/7] ACPICA: Tables: Change table comparison using table header for static load tables

[Lv Zheng]

For AML tables, checksum is available for us to detect a duplicate table,
so this patch removes byte-by-byte check for static load tables to improve
performance, and comparing header allows us to validate if
LEN/CSUM/OEM fields match. Suggested by Bob Moore, fixed by Lv Zheng.

Suggested-by: Bob Moore <robert.moore@intel.com>
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
Cc: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/acpica/tbdata.c | 28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/drivers/acpi/acpica/tbdata.c b/drivers/acpi/acpica/tbdata.c
index 180c7a3..09fab1d 100644
--- a/drivers/acpi/acpica/tbdata.c
+++ b/drivers/acpi/acpica/tbdata.c
@@ -52,10 +52,12 @@ ACPI_MODULE_NAME("tbdata")
 
 /* Local prototypes */
 static u8
-acpi_tb_compare_tables(struct acpi_table_desc *table_desc, u32 table_index);
+acpi_tb_compare_tables(struct acpi_table_desc *table_desc,
+		       u8 reload, u32 table_index);
 
 static acpi_status
 acpi_tb_validate_table_for_load(u32 *table_index,
+				u8 reload,
 				struct acpi_table_header **out_table);
 
 /*******************************************************************************
@@ -782,6 +784,7 @@ void acpi_tb_set_table_loaded_flag(u32 table_index, u8 is_loaded)
  * FUNCTION:    acpi_tb_compare_tables
  *
  * PARAMETERS:  table_desc          - Table 1 descriptor to be compared
+ *              reload              - Whether reload should be performed
  *              table_index         - Index of table 2 to be compared
  *
  * RETURN:      TRUE if both tables are identical.
@@ -792,7 +795,8 @@ void acpi_tb_set_table_loaded_flag(u32 table_index, u8 is_loaded)
  ******************************************************************************/
 
 static u8
-acpi_tb_compare_tables(struct acpi_table_desc *table_desc, u32 table_index)
+acpi_tb_compare_tables(struct acpi_table_desc *table_desc,
+		       u8 reload, u32 table_index)
 {
 	acpi_status status = AE_OK;
 	u8 is_identical;
@@ -811,9 +815,16 @@ acpi_tb_compare_tables(struct acpi_table_desc *table_desc, u32 table_index)
 	 * Check for a table match on the entire table length,
 	 * not just the header.
 	 */
-	is_identical = (u8)((table_desc->length != table_length ||
-			     memcmp(table_desc->pointer, table, table_length)) ?
-			    FALSE : TRUE);
+	if (reload) {
+		is_identical = (u8)((table_desc->length != table_length ||
+				     memcmp(table_desc->pointer, table,
+					    table_length)) ? FALSE : TRUE);
+	} else {
+		is_identical =
+		    (u8)(memcmp
+			 (table_desc->pointer, table,
+			  sizeof(struct acpi_table_header)) ? FALSE : TRUE);
+	}
 
 	/* Release the acquired table */
 
@@ -826,6 +837,7 @@ acpi_tb_compare_tables(struct acpi_table_desc *table_desc, u32 table_index)
  * FUNCTION:    acpi_tb_validate_table_for_load
  *
  * PARAMETERS:  table_index         - Index of table
+ *              reload              - Whether reload should be performed
  *              out_table           - Validated table
  *
  * RETURN:      Status
@@ -838,7 +850,7 @@ acpi_tb_compare_tables(struct acpi_table_desc *table_desc, u32 table_index)
 
 static acpi_status
 acpi_tb_validate_table_for_load(u32 *table_index,
-				struct acpi_table_header **out_table)
+				u8 reload, struct acpi_table_header **out_table)
 {
 	u32 i;
 	struct acpi_table_desc *table_desc;
@@ -913,7 +925,7 @@ acpi_tb_validate_table_for_load(u32 *table_index,
 		 * Check for a table match on the entire table length, not just the
 		 * header.
 		 */
-		if (!acpi_tb_compare_tables(table_desc, i)) {
+		if (!acpi_tb_compare_tables(table_desc, reload, i)) {
 			continue;
 		}
 
@@ -973,7 +985,7 @@ acpi_tb_load_table(u32 *table_index,
 
 	/* Sanity checks */
 
-	status = acpi_tb_validate_table_for_load(table_index, &table);
+	status = acpi_tb_validate_table_for_load(table_index, reload, &table);
 	if (ACPI_FAILURE(status)) {
 		if (status == AE_ALREADY_EXISTS) {
 			/*
-- 
2.7.4

Re: [RFC PATCH 0/7] ACPICA: Tables: Cleanup table sanity checks

[Hans de Goede]

Hi,

On 15-03-17 07:36, Lv Zheng wrote:
> Originally AML table sanity check is only done for Load opcode, as
> acpi_tb_compare_tables() is only invoked in
> acpi_tb_install_standard_table(). While LoadTable opcode cannot be covered
> as it directly invokes acpi_tb_load_table() without invoking
> acpi_tb_install_standard_table(). Furthermore, standard table load also
> cannot be covered as acpi_ns_load_table() is invoked instead of
> acpi_tb_install_standard_table() and acpi_tb_load_table().
>
> So it becomes a problem if a duplicate table is in RSDT/XSDT, there is no
> such check implemented for static load tables and LoadTable opcode.
>
> This patchset combines code paths (in return reduces some redundant code
> blocks and enhances checks/locks) so that duplicate table detection can be
> applied to all cases.

I can confirm that this series fixes the errors I was seeing on my
GPD win machine. Note as explained in my reply to Robert I believe that
the last patch from the series can be dropped:

"On 15-03-17 04:05, Moore, Robert wrote:
 > And I would suppose a refinement would be:
 >
 > Compare headers
 > If headers match, compare entire tables.

The existing memcmp already does that since the header is in the
front, and memcmp will stop on the first difference, so there
is no need to change anything."

As Robert mentions we really a should compare the entire table
if the checksums match since an 8 bit checksum is really weak and
if we want to do that a simple memcmp will suffice as that will
already exit directly if the checksum mismatches as the checksum
is before the actual data.

Regards,

Hans




>
> Lv Zheng (7):
>   ACPICA: Tables: Cleanup table handler invokers
>   ACPICA: Tables: Add sanity check for load_table opcode
>   ACPICA: Tables: Add sanity check for table install
>   ACPICA: Tables: Fix an unwanted exception for table reloading
>   ACPICA: Tables: Enable acpi_ut_is_aml_table() for all ACPICA modules
>   ACPICA: Tables: Add sanity check for static table load
>   ACPICA: Tables: Change table comparison using table header for static
>     load tables
>
>  drivers/acpi/acpica/actables.h |   7 +-
>  drivers/acpi/acpica/acutils.h  |   4 +-
>  drivers/acpi/acpica/exconfig.c |   2 +-
>  drivers/acpi/acpica/nsload.c   |  18 ---
>  drivers/acpi/acpica/tbdata.c   | 288 ++++++++++++++++++++++++++++++++++++-----
>  drivers/acpi/acpica/tbinstal.c | 197 ++++++----------------------
>  drivers/acpi/acpica/tbxfload.c |  46 +------
>  drivers/acpi/acpica/utmisc.c   |  27 ++--
>  include/acpi/actbl.h           |   1 +
>  9 files changed, 325 insertions(+), 265 deletions(-)
>

RE: [RFC PATCH 0/7] ACPICA: Tables: Cleanup table sanity checks

[Zheng, Lv]

Hi,

> From: Hans de Goede [mailto:hdegoede@redhat.com]
> Subject: Re: [RFC PATCH 0/7] ACPICA: Tables: Cleanup table sanity checks
> 
> Hi,
> 
> On 15-03-17 07:36, Lv Zheng wrote:
> > Originally AML table sanity check is only done for Load opcode, as
> > acpi_tb_compare_tables() is only invoked in
> > acpi_tb_install_standard_table(). While LoadTable opcode cannot be covered
> > as it directly invokes acpi_tb_load_table() without invoking
> > acpi_tb_install_standard_table(). Furthermore, standard table load also
> > cannot be covered as acpi_ns_load_table() is invoked instead of
> > acpi_tb_install_standard_table() and acpi_tb_load_table().
> >
> > So it becomes a problem if a duplicate table is in RSDT/XSDT, there is no
> > such check implemented for static load tables and LoadTable opcode.
> >
> > This patchset combines code paths (in return reduces some redundant code
> > blocks and enhances checks/locks) so that duplicate table detection can be
> > applied to all cases.
> 
> I can confirm that this series fixes the errors I was seeing on my
> GPD win machine.

Ok, thanks for the test and the report.

> Note as explained in my reply to Robert I believe that
> the last patch from the series can be dropped:
> 
> "On 15-03-17 04:05, Moore, Robert wrote:
>  > And I would suppose a refinement would be:
>  >
>  > Compare headers
>  > If headers match, compare entire tables.
> 
> The existing memcmp already does that since the header is in the
> front, and memcmp will stop on the first difference, so there
> is no need to change anything."
> 
> As Robert mentions we really a should compare the entire table
> if the checksums match since an 8 bit checksum is really weak and
> if we want to do that a simple memcmp will suffice as that will
> already exit directly if the checksum mismatches as the checksum
> is before the actual data.

Maybe the suggestion is made on the previous fact.
The previous design prevents such duplicate tables from being
installed. It surely is able to prevent same table from being
installed twice.
Under the previous design, we should do the duplicate table
detection in acpi_tb_verify_temp_table() which is exact the API used
before the table is installed to the global table list.
But there is a special logic for table install:
Linux can only use limited slots to map ACPI tables.
Thus most of the time, we can only map the table header.

The series now follows your idea, split sanity checks into 2 levels:
1. for table install, only check "address" and "memory types"
2. for table load, check "table content" to detect duplicate tables
And since the table load doesn't have similar restriction, we seem
to be able to map the whole table to compare.

And I noticed comments in acpi_tb_compare_tables(). It looks like
comparing the Whole table rather than comparing the table header is
a design decision made several years ago due to some reasons.

As such:
1. With new design, we are able to compare the whole table
2. Comparing whole table rather than comparing table header looks
   like a determined design choice
I made it the last patch and a separate one so that we can drop
it at any time.

Thanks and best regards
Lv

> 
> 
> 
> >
> > Lv Zheng (7):
> >   ACPICA: Tables: Cleanup table handler invokers
> >   ACPICA: Tables: Add sanity check for load_table opcode
> >   ACPICA: Tables: Add sanity check for table install
> >   ACPICA: Tables: Fix an unwanted exception for table reloading
> >   ACPICA: Tables: Enable acpi_ut_is_aml_table() for all ACPICA modules
> >   ACPICA: Tables: Add sanity check for static table load
> >   ACPICA: Tables: Change table comparison using table header for static
> >     load tables
> >
> >  drivers/acpi/acpica/actables.h |   7 +-
> >  drivers/acpi/acpica/acutils.h  |   4 +-
> >  drivers/acpi/acpica/exconfig.c |   2 +-
> >  drivers/acpi/acpica/nsload.c   |  18 ---
> >  drivers/acpi/acpica/tbdata.c   | 288 ++++++++++++++++++++++++++++++++++++-----
> >  drivers/acpi/acpica/tbinstal.c | 197 ++++++----------------------
> >  drivers/acpi/acpica/tbxfload.c |  46 +------
> >  drivers/acpi/acpica/utmisc.c   |  27 ++--
> >  include/acpi/actbl.h           |   1 +
> >  9 files changed, 325 insertions(+), 265 deletions(-)
> >

RE: [RFC PATCH 0/7] ACPICA: Tables: Cleanup table sanity checks

[Zheng, Lv]

Hi,

> From: Devel [mailto:devel-bounces@acpica.org] On Behalf Of Zheng, Lv
> Subject: Re: [Devel] [RFC PATCH 0/7] ACPICA: Tables: Cleanup table sanity checks
> 
> Hi,
> 
> > From: Hans de Goede [mailto:hdegoede@redhat.com]
> > Subject: Re: [RFC PATCH 0/7] ACPICA: Tables: Cleanup table sanity checks
> >
> > Hi,
> >
> > On 15-03-17 07:36, Lv Zheng wrote:
> > > Originally AML table sanity check is only done for Load opcode, as
> > > acpi_tb_compare_tables() is only invoked in
> > > acpi_tb_install_standard_table(). While LoadTable opcode cannot be covered
> > > as it directly invokes acpi_tb_load_table() without invoking
> > > acpi_tb_install_standard_table(). Furthermore, standard table load also
> > > cannot be covered as acpi_ns_load_table() is invoked instead of
> > > acpi_tb_install_standard_table() and acpi_tb_load_table().
> > >
> > > So it becomes a problem if a duplicate table is in RSDT/XSDT, there is no
> > > such check implemented for static load tables and LoadTable opcode.
> > >
> > > This patchset combines code paths (in return reduces some redundant code
> > > blocks and enhances checks/locks) so that duplicate table detection can be
> > > applied to all cases.
> >
> > I can confirm that this series fixes the errors I was seeing on my
> > GPD win machine.
> 
> Ok, thanks for the test and the report.
> 
> > Note as explained in my reply to Robert I believe that
> > the last patch from the series can be dropped:
> >
> > "On 15-03-17 04:05, Moore, Robert wrote:
> >  > And I would suppose a refinement would be:
> >  >
> >  > Compare headers
> >  > If headers match, compare entire tables.
> >
> > The existing memcmp already does that since the header is in the
> > front, and memcmp will stop on the first difference, so there
> > is no need to change anything."
> >
> > As Robert mentions we really a should compare the entire table
> > if the checksums match since an 8 bit checksum is really weak and
> > if we want to do that a simple memcmp will suffice as that will
> > already exit directly if the checksum mismatches as the checksum
> > is before the actual data.
> 
> Maybe the suggestion is made on the previous fact.
> The previous design prevents such duplicate tables from being
> installed. It surely is able to prevent same table from being
> installed twice.
> Under the previous design, we should do the duplicate table
> detection in acpi_tb_verify_temp_table() which is exact the API used
> before the table is installed to the global table list.
> But there is a special logic for table install:
> Linux can only use limited slots to map ACPI tables.
> Thus most of the time, we can only map the table header.
> 
> The series now follows your idea, split sanity checks into 2 levels:
> 1. for table install, only check "address" and "memory types"
> 2. for table load, check "table content" to detect duplicate tables
> And since the table load doesn't have similar restriction, we seem
> to be able to map the whole table to compare.
> 
> And I noticed comments in acpi_tb_compare_tables(). It looks like
> comparing the Whole table rather than comparing the table header is
> a design decision made several years ago due to some reasons.
> 
> As such:
> 1. With new design, we are able to compare the whole table
> 2. Comparing whole table rather than comparing table header looks
>    like a determined design choice
> I made it the last patch and a separate one so that we can drop
> it at any time.

Dropped and the pull request is updated:
https://github.com/acpica/acpica/pull/121
If everything works smoothly, you'll see it fixed by next ACPICA release.

Thanks
Lv

> > >
> > > Lv Zheng (7):
> > >   ACPICA: Tables: Cleanup table handler invokers
> > >   ACPICA: Tables: Add sanity check for load_table opcode
> > >   ACPICA: Tables: Add sanity check for table install
> > >   ACPICA: Tables: Fix an unwanted exception for table reloading
> > >   ACPICA: Tables: Enable acpi_ut_is_aml_table() for all ACPICA modules
> > >   ACPICA: Tables: Add sanity check for static table load
> > >   ACPICA: Tables: Change table comparison using table header for static
> > >     load tables
> > >
> > >  drivers/acpi/acpica/actables.h |   7 +-
> > >  drivers/acpi/acpica/acutils.h  |   4 +-
> > >  drivers/acpi/acpica/exconfig.c |   2 +-
> > >  drivers/acpi/acpica/nsload.c   |  18 ---
> > >  drivers/acpi/acpica/tbdata.c   | 288 ++++++++++++++++++++++++++++++++++++-----
> > >  drivers/acpi/acpica/tbinstal.c | 197 ++++++----------------------
> > >  drivers/acpi/acpica/tbxfload.c |  46 +------
> > >  drivers/acpi/acpica/utmisc.c   |  27 ++--
> > >  include/acpi/actbl.h           |   1 +
> > >  9 files changed, 325 insertions(+), 265 deletions(-)
> > >
> _______________________________________________
> Devel mailing list
> Devel@acpica.org
> https://lists.acpica.org/mailman/listinfo/devel