* [PATCH v2 0/4] ACPI 2.0: Enable TermList interpretion for table loading [not found] <0e65135af51d94db0410c7059f3bc3a2300fc3b5> @ 2016-05-13 5:35 ` Lv Zheng 2016-05-13 5:35 ` [PATCH v2 1/4] ACPICA: Dispatcher: Fix an issue that the opregions created by the linked MLC were not tracked Lv Zheng ` (4 more replies) 2016-06-20 9:07 ` [PATCH v3 0/5] " Lv Zheng ` (2 subsequent siblings) 3 siblings, 5 replies; 40+ messages in thread From: Lv Zheng @ 2016-05-13 5:35 UTC (permalink / raw) To: Rafael J. Wysocki, Rafael J. Wysocki, Len Brown Cc: Lv Zheng, Lv Zheng, linux-kernel, linux-acpi MLC (module level code) is an ACPICA terminology describing the AML code out of any control method, currently only Type1Opcode (If/Else/While) wrapped MLC code blocks are executed by the AML interpreter after the table loading. But the issue which is fixed by this patchset is: Not only Type1Opcode, but also Type2Opcode will be executed as MLC and MLC is not executed after loading the table, but is executed right in place. The following AML code is assembled into a static loading SSDT, and used as an instrumentation to pry into the de-facto standard AML interpreter behaviors: Name (ECOK, Zero) Scope (\) { DBUG ("TermList 1") If (LEqual (ECOK, Zero)) { DBUG ("TermList 2") Device (MDEV) { DEBUG (TermList 3") If (CondRefOf (MDEV)) { DBUG ("MDEV exists") } If (CondRefOf (MDEV._STA)) { DBUG ("MDEV._STA exists") } If (CondRefOf (\_SB.PCI0.EC)) { DBUG ("\\_SB.PCI0.EC exists") } Name (_HID, EisaId ("PNP9999")) Method (_STA, 0, Serialized) { DEBUG ("\\_SB.MDEV._STA") Return (0x0F) } } DBUG ("TermList 4") } Method (_INI, 0, Serialized) { DBUG ("\\_SB._INI") } } Scope (_SB.PCI0) { Device (EC) { ... } } The DBUG function is a function to write the debugging messages into a SystemIo debug port. Running Windows with the BIOS providing this SSDT via RSDT, the following messages are obtained from the debug port: TermList 1 TermList 2 TermList 3 \_SB.MDEV exists TermList 4 \_SB._INI ... This test reveals the de-facto grammar for the AMLCode to us: 1. During the table loading, MLC will be executed by the interpreter, this is partially supported by the current ACPICA; 2. For SystemIo, not only after the _REG(1, 1) is evaluated (current ACPICA interpreter limitation), but when the table is being loaded, the SystemIo (the debugging port) is accessible, this is recently fixed in the upstream, now all early operation regions are accessible during the table loading; 3. Not only Type1Opcode, but also Type2Opcode will be executed as MLC and MLC is not executed after loading the table, but is executed right in place, the Linux upstream is not compliant to this behavior. The last compliance issue has already been clarified in ACPI 2.0 specification, so the compliance issue is not that Linux is not compliant to the de-facto standard OS, but that Linux is not compliant to ACPI 2.0. Definition block tables in fact is defined by the spec as TermList, which has no difference than the control methods, thus the interpretion of the table should be no difference that the control method evaluation: AMLCode := DefBlockHeader TermList DefMethod := MethodOp PkgLength NameString MethodFlags TermList Why ACPICA interpreter is acting so differently from this definition? This is because, there are many software entropies preventing this from being enabled, such entropies need to be cleaned up first in order not to trigger regressions for specific platforms. These entropies include: 1. ECDT support is broken. In fact, the original EC driver was correct, but devlopers started to use the namespace EC instead of ECDT just because several broken ECDT tables were reported on the bugzilla. They trusted the namespace EC settings rather than the ECDT ones, this led to the evaluation of _REG/_GPE/_CRS and namespace walk before executing the module level AML opcodes. And the fixes in fact finally disable early EC usages (used during table loading and early device enumeration processes). 2. _REG evaluations are wrong. ACPICA provides APIs for OSPMs to register operation region handlers. But for the early operation region accesses, ACPI spec declares that the evaluations of _REG are not required, but the ACPICA APIs do not avoid running _REG to meet this early requirements. Code to fix this is partially upstreamed during previous ACPICA release cycle. 3. _REG associations are wrong. ACPICA associates _REG control method to all operation region objects before executing the _REG control method. This can happen even when a control method is evaluated and operation regions defined in the method is initialized (acpi_ev_initialize_region). As a part of the ACPICA internal _REG evaluation state machine, it requires the namespace walk, and all namespace walk should be ensured to happen only "AFTER THE NAMESPACE IS INITIALIZED". But when this logic happens during the table loading, it may fail in finding the _REG method since the _REG method may not be created by the interpreter just because _REG is defined after the operation region object's declaration. 4. _REG(CONNECT)/_REG(DISCONNECT) executions are not balanced, this can lead to wrong table loading/unloading results. Since _REG evaluations require the releasing of all interpreter/namespace locks in order to allow another evaluation to happen, and ACPICA operand object destruction code can be invoked from different locking environment, this becomes difficult for the developers to provide one single function to make _REG(CONNECT)/_REG(DISCONNECT) balanced. 5. \_SB._INI is not the first control method evaluated by the interpreter. Many platforms put initialization code in \_SB._INI in order to have named objects initialized very early during the device enumeration process. Without this order strictly ensured, early operation region access enabling could break these platforms. 6. Linux initialization order is wrong, it is now: a. load namespace without executing root scope If/Else/While module level code blocks; b. probe ECDT and instal EmbeddedControl operation region handler with _REG evaluated; c. install SystemMemory, SystemIo, PciConfig operation region handlers without evaluating _REG; d. run _REG for SystemMemory, SystemIo, PciConfig operation regions; e. execute root scope If/Else/While module level code blocks; f. enable GPE and namespace EC. While the correct order should be: a. probe ECDT and install EmbeddedControl operation region handler without evaluating _REG; b. install SystemMemory, SystemIo, PciConfig operation region handlers without evaluating _REG; c. load namespace, in the meanshile, execute all module level AML opcodes; d. run _REG for SystemMemory, SystemIo, PciConfig operation regions; e. enable GPE and namespace EC which results in _REG evaluation for EC. Until now we've upstreamed most of the entropy fixes into the Linux kernel, tested the grammar switch in the ACPICA upstream using ASLTS and no significant regressions can be seen while we need more tests before it is merged: https://github.com/acpica/acpica/pull/134 This is the ASLTS running result after applying the grammar switch, the test cases include new "module" case for which ACPICA interpreter cannot pass without this grammar switch applied: ============================================================ Test cases specified for running: arithmetic bfield constant control descriptor logic manipulation name reference region synchronization table misc provoke oarg oconst olocal onamedloc onamedglob opackageel oreftonamed oreftopackageel oreturn rstore roptional rcopyobject rindecrement rexplicitconv badasl namespace exc exc_ref exc_operand2 exc_result2 exc_tbl mt_mutex extra extra_aslts bdemo bdemof condbranches TOTAL: (32-bit norm mode) PASS : 0 FAIL : 0 BLOCKED : 0 SKIPPED : 0 Tests : 0 Test Cases : 40 (of 47) Test Collections : 7 (of 8) Outstanding allocations after execution : 0 Outstanding allocations (ACPI Error) : 0 Large Reference Count (ACPI Error) : 0 Memory consumption total : 0 Kb TOTAL: (64-bit norm mode) PASS : 0 FAIL : 0 BLOCKED : 0 SKIPPED : 0 Tests : 0 Test Cases : 40 (of 47) Test Collections : 7 (of 8) Outstanding allocations after execution : 0 Outstanding allocations (ACPI Error) : 0 Large Reference Count (ACPI Error) : 0 Memory consumption total : 0 Kb TOTAL: (32-bit slack mode) PASS : 0 FAIL : 0 BLOCKED : 0 SKIPPED : 0 Tests : 0 Test Cases : 40 (of 47) Test Collections : 7 (of 8) Outstanding allocations after execution : 0 Outstanding allocations (ACPI Error) : 0 Large Reference Count (ACPI Error) : 0 Memory consumption total : 0 Kb TOTAL: (64-bit slack mode) PASS : 0 FAIL : 0 BLOCKED : 0 SKIPPED : 0 Tests : 0 Test Cases : 40 (of 47) Test Collections : 7 (of 8) Outstanding allocations after execution : 0 Outstanding allocations (ACPI Error) : 0 Large Reference Count (ACPI Error) : 0 Memory consumption total : 0 Kb ============================================================ Since we need more tests from the real users, we could make the grammar switch released from the Linux upstream. It's safe to do so because we have implemented regression protection (acpi_gbl_parse_table_as_term_list) in the fixes. The earlier the fix is tested by more real users, the better quality can be achieved by knowing the unknown cases (if any). Lv Zheng (4): ACPICA: Dispatcher: Fix an issue that the opregions created by the linked MLC were not tracked ACPICA: ACPI 2.0, Interpreter: Fix MLC issues by switching to new TermList grammar for table loading ACPI 2.0 / AML: Enable correct ACPI subsystem initialization order for new table loading mode ACPI 2.0 / AML: Fix module level execution by correctly parsing table as TermList drivers/acpi/acpica/acnamesp.h | 3 + drivers/acpi/acpica/acparser.h | 2 + drivers/acpi/acpica/dsopcode.c | 6 ++ drivers/acpi/acpica/evrgnini.c | 3 +- drivers/acpi/acpica/exconfig.c | 6 +- drivers/acpi/acpica/nsload.c | 3 +- drivers/acpi/acpica/nsparse.c | 163 ++++++++++++++++++++++++++++++++-------- drivers/acpi/acpica/psparse.c | 4 +- drivers/acpi/acpica/psxface.c | 73 ++++++++++++++++++ drivers/acpi/acpica/tbxfload.c | 3 +- drivers/acpi/acpica/utxfinit.c | 3 +- drivers/acpi/bus.c | 6 +- include/acpi/acpixf.h | 6 ++ 13 files changed, 241 insertions(+), 40 deletions(-) -- 1.7.10 ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v2 1/4] ACPICA: Dispatcher: Fix an issue that the opregions created by the linked MLC were not tracked 2016-05-13 5:35 ` [PATCH v2 0/4] ACPI 2.0: Enable TermList interpretion for table loading Lv Zheng @ 2016-05-13 5:35 ` Lv Zheng 2016-05-13 5:35 ` [PATCH v2 2/4] ACPICA: ACPI 2.0, Interpreter: Fix MLC issues by switching to new TermList grammar for table loading Lv Zheng ` (3 subsequent siblings) 4 siblings, 0 replies; 40+ messages in thread From: Lv Zheng @ 2016-05-13 5:35 UTC (permalink / raw) To: Rafael J. Wysocki, Rafael J. Wysocki, Len Brown Cc: Lv Zheng, Lv Zheng, linux-kernel, linux-acpi Operation regions created by MLC were not tracked by acpi_check_address_range(), this patch fixes this issue. ACPICA BZ 1279. Fixed by Lv Zheng. Link: https://bugs.acpica.org/show_bug.cgi?id=1279 Signed-off-by: Lv Zheng <lv.zheng@intel.com> --- drivers/acpi/acpica/dsopcode.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/drivers/acpi/acpica/dsopcode.c b/drivers/acpi/acpica/dsopcode.c index 4cc9d98..96f2eef 100644 --- a/drivers/acpi/acpica/dsopcode.c +++ b/drivers/acpi/acpica/dsopcode.c @@ -455,6 +455,12 @@ acpi_ds_eval_region_operands(struct acpi_walk_state *walk_state, /* Now the address and length are valid for this opregion */ obj_desc->region.flags |= AOPOBJ_DATA_VALID; + if (walk_state->parse_flags & ACPI_PARSE_MODULE_LEVEL) { + status = acpi_ut_add_address_range(obj_desc->region.space_id, + obj_desc->region.address, + obj_desc->region.length, + node); + } return_ACPI_STATUS(status); } -- 1.7.10 ^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH v2 2/4] ACPICA: ACPI 2.0, Interpreter: Fix MLC issues by switching to new TermList grammar for table loading 2016-05-13 5:35 ` [PATCH v2 0/4] ACPI 2.0: Enable TermList interpretion for table loading Lv Zheng 2016-05-13 5:35 ` [PATCH v2 1/4] ACPICA: Dispatcher: Fix an issue that the opregions created by the linked MLC were not tracked Lv Zheng @ 2016-05-13 5:35 ` Lv Zheng 2016-05-13 5:35 ` [PATCH v2 3/4] ACPI 2.0 / AML: Enable correct ACPI subsystem initialization order for new table loading mode Lv Zheng ` (2 subsequent siblings) 4 siblings, 0 replies; 40+ messages in thread From: Lv Zheng @ 2016-05-13 5:35 UTC (permalink / raw) To: Rafael J. Wysocki, Rafael J. Wysocki, Len Brown Cc: Lv Zheng, Lv Zheng, linux-kernel, linux-acpi The MLC (Module Level Code) is an ACPICA terminology describing the AML code out of any control method, its support is the main contention of the interpreter behavior during the table loading. The original implementation of MLC in ACPICA had several issues: 1. Out of any control method, besides of the object creating opcodes, only the code blocks wrapped by "If/Else/While" opcodes were supported. 2. The supported MLC code blocks were executed after loading the table rather than being executed right in place. ============================================================ The demo of this order issue is as follows: Name (OBJ1, 1) If (CND1 == 1) { Name (OBJ2, 2) } Name (OBJ3, 3) The original MLC support created OBJ2 after OBJ3's creation. ============================================================ Other than these limitations, MLC support in ACPICA looks correct. And supporting this should be easy/natural for ACPICA, but enabling of this was blocked by some ACPICA internal and OSPM specific initialization order issues we've fixed recently. The wrong support started from the following false bug fixing commit: Commit: 80d7951177315f70b5ffd8663985fbf725d07799 Subject: Add support for module-level executable AML code. We can confirm Windows interpreter behavior via reverse engineering means. It can be proven that not only If/Else/While wrapped code blocks, all opcodes can be executed at the module level, including operation region accesses. And it can be proven that the MLC should be executed right in place, not in such a deferred way executed after loading the table. And the above facts indeed reflect the spec words around ACPI definition block tables (DSDT/SSDT/...), the entire table and the Scope object is defined by the AML specification in BNF style as: AMLCode := DefBlockHeader TermList DefScope := ScopeOp PkgLength NameString TermList The bodies of the scope opening terms (AMLCode/Scope) are all TermList, thus the table loading should be no difference than the control method evaluations as the body of the Method is also defined by the AML specification as TermList: DefMethod := MethodOp PkgLength NameString MethodFlags TermList The only difference is: after evaluating control method, created named objects may be freed due to no reference, while named objects created by the table loading should only be freed after unloading the table. So this patch follows the spec and the de-facto standard behavior, enables the new grammar (TermList) for the table loading. By doing so, beyond the fixes to the above issues, we can see additional differences comparing to the old grammar based table loading: 1. Originally, beyond the scope opening terms (AMLCode/Scope), If/Else/While wrapped code blocks under the scope creating terms (Device/PowerResource/Processor/ThermalZone) are also supported as deferred MLC, which violates the spec defined grammar where ObjectList is enforced. With MLC support improved as non-deferred, the interpreter parses such scope creating terms as TermList rather ObjectList like the scope opening terms. After probing the Windows behavior and proving that it also parses these terms as TermList, we submitted an ECR (Engineering Change Request) to the ASWG (ACPI Specification Working Group) to clarify this. The ECR is titled as "ASL Grammar Clarification for Executable AML Opcodes" and has been accepted by the ASWG. The new grammar will appear in ACPI specification 6.2. 2. Originally, Buffer/Package/OperationRegion/CreateXXXField/BankField arguments are evaluated in a deferred way after loading the table. With MLC support improved, they are also parsed right in place during the table loading. This is also Windows compliant and the only difference is the removal of the debugging messages implemented before acpi_ds_execute_arguments(), see Link 1 for the details. A previous commit should have ensured that acpi_check_address_range() won't regress. Note that enabling this feature may cause regressions due to long term Linux ACPI support on top of the wrong grammar. So this patch also prepares a global option to be used to roll back to the old grammar during the period between a regression is reported and the regression is root-cause-fixed. ACPICA BZ 963, fixed by Lv Zheng. Link 1: https://bugzilla.kernel.org/show_bug.cgi?id=112911 Link 2: https://bugs.acpica.org/show_bug.cgi?id=963 Tested-by: Chris Bainbridge <chris.bainbridge@gmail.com> Signed-off-by: Lv Zheng <lv.zheng@intel.com> --- drivers/acpi/acpica/acnamesp.h | 3 + drivers/acpi/acpica/acparser.h | 2 + drivers/acpi/acpica/evrgnini.c | 3 +- drivers/acpi/acpica/exconfig.c | 6 +- drivers/acpi/acpica/nsload.c | 3 +- drivers/acpi/acpica/nsparse.c | 163 ++++++++++++++++++++++++++++++++-------- drivers/acpi/acpica/psparse.c | 4 +- drivers/acpi/acpica/psxface.c | 73 ++++++++++++++++++ drivers/acpi/acpica/tbxfload.c | 3 +- drivers/acpi/acpica/utxfinit.c | 3 +- include/acpi/acpixf.h | 6 ++ 11 files changed, 231 insertions(+), 38 deletions(-) diff --git a/drivers/acpi/acpica/acnamesp.h b/drivers/acpi/acpica/acnamesp.h index f33a4ba..829672a 100644 --- a/drivers/acpi/acpica/acnamesp.h +++ b/drivers/acpi/acpica/acnamesp.h @@ -130,6 +130,9 @@ acpi_status acpi_ns_parse_table(u32 table_index, struct acpi_namespace_node *start_node); acpi_status +acpi_ns_execute_table(u32 table_index, struct acpi_namespace_node *start_node); + +acpi_status acpi_ns_one_complete_parse(u32 pass_number, u32 table_index, struct acpi_namespace_node *start_node); diff --git a/drivers/acpi/acpica/acparser.h b/drivers/acpi/acpica/acparser.h index fc30577..939d411 100644 --- a/drivers/acpi/acpica/acparser.h +++ b/drivers/acpi/acpica/acparser.h @@ -78,6 +78,8 @@ extern const u8 acpi_gbl_long_op_index[]; */ acpi_status acpi_ps_execute_method(struct acpi_evaluate_info *info); +acpi_status acpi_ps_execute_table(struct acpi_evaluate_info *info); + /* * psargs - Parse AML opcode arguments */ diff --git a/drivers/acpi/acpica/evrgnini.c b/drivers/acpi/acpica/evrgnini.c index b6ea9c0..3843f1f 100644 --- a/drivers/acpi/acpica/evrgnini.c +++ b/drivers/acpi/acpica/evrgnini.c @@ -553,7 +553,8 @@ acpi_ev_initialize_region(union acpi_operand_object *region_obj, * * See acpi_ns_exec_module_code */ - if (obj_desc->method. + if (!acpi_gbl_parse_table_as_term_list && + obj_desc->method. info_flags & ACPI_METHOD_MODULE_LEVEL) { handler_obj = obj_desc->method.dispatch.handler; diff --git a/drivers/acpi/acpica/exconfig.c b/drivers/acpi/acpica/exconfig.c index a1d177d..3918c33 100644 --- a/drivers/acpi/acpica/exconfig.c +++ b/drivers/acpi/acpica/exconfig.c @@ -108,8 +108,10 @@ acpi_ex_add_table(u32 table_index, /* Add the table to the namespace */ + acpi_ex_exit_interpreter(); status = acpi_ns_load_table(table_index, parent_node); if (ACPI_FAILURE(status)) { + acpi_ex_enter_interpreter(); acpi_ut_remove_reference(obj_desc); *ddb_handle = NULL; return_ACPI_STATUS(status); @@ -117,8 +119,8 @@ acpi_ex_add_table(u32 table_index, /* Execute any module-level code that was found in the table */ - acpi_ex_exit_interpreter(); - if (acpi_gbl_group_module_level_code) { + if (!acpi_gbl_parse_table_as_term_list + && acpi_gbl_group_module_level_code) { acpi_ns_exec_module_code_list(); } acpi_ex_enter_interpreter(); diff --git a/drivers/acpi/acpica/nsload.c b/drivers/acpi/acpica/nsload.c index b5e2b0a..2daa9a09 100644 --- a/drivers/acpi/acpica/nsload.c +++ b/drivers/acpi/acpica/nsload.c @@ -162,7 +162,8 @@ unlock: * other ACPI implementations. Optionally, the execution can be deferred * until later, see acpi_initialize_objects. */ - if (!acpi_gbl_group_module_level_code) { + if (!acpi_gbl_parse_table_as_term_list + && !acpi_gbl_group_module_level_code) { acpi_ns_exec_module_code_list(); } diff --git a/drivers/acpi/acpica/nsparse.c b/drivers/acpi/acpica/nsparse.c index f631a47..2452bf3 100644 --- a/drivers/acpi/acpica/nsparse.c +++ b/drivers/acpi/acpica/nsparse.c @@ -47,12 +47,103 @@ #include "acparser.h" #include "acdispat.h" #include "actables.h" +#include "acinterp.h" #define _COMPONENT ACPI_NAMESPACE ACPI_MODULE_NAME("nsparse") /******************************************************************************* * + * FUNCTION: ns_execute_table + * + * PARAMETERS: table_desc - An ACPI table descriptor for table to parse + * start_node - Where to enter the table into the namespace + * + * RETURN: Status + * + * DESCRIPTION: Load ACPI/AML table by executing the entire table as a + * term_list. + * + ******************************************************************************/ +acpi_status +acpi_ns_execute_table(u32 table_index, struct acpi_namespace_node *start_node) +{ + acpi_status status; + struct acpi_table_header *table; + acpi_owner_id owner_id; + struct acpi_evaluate_info *info = NULL; + u32 aml_length; + u8 *aml_start; + union acpi_operand_object *method_obj = NULL; + + ACPI_FUNCTION_TRACE(ns_execute_table); + + status = acpi_get_table_by_index(table_index, &table); + if (ACPI_FAILURE(status)) { + return_ACPI_STATUS(status); + } + + /* Table must consist of at least a complete header */ + + if (table->length < sizeof(struct acpi_table_header)) { + return_ACPI_STATUS(AE_BAD_HEADER); + } + + aml_start = (u8 *)table + sizeof(struct acpi_table_header); + aml_length = table->length - sizeof(struct acpi_table_header); + + status = acpi_tb_get_owner_id(table_index, &owner_id); + if (ACPI_FAILURE(status)) { + return_ACPI_STATUS(status); + } + + /* Create, initialize, and link a new temporary method object */ + + method_obj = acpi_ut_create_internal_object(ACPI_TYPE_METHOD); + if (!method_obj) { + return_ACPI_STATUS(AE_NO_MEMORY); + } + + /* Allocate the evaluation information block */ + + info = ACPI_ALLOCATE_ZEROED(sizeof(struct acpi_evaluate_info)); + if (!info) { + status = AE_NO_MEMORY; + goto cleanup; + } + + ACPI_DEBUG_PRINT((ACPI_DB_PARSE, + "Create table code block: %p\n", method_obj)); + + method_obj->method.aml_start = aml_start; + method_obj->method.aml_length = aml_length; + method_obj->method.owner_id = owner_id; + method_obj->method.info_flags |= ACPI_METHOD_MODULE_LEVEL; + + info->pass_number = ACPI_IMODE_EXECUTE; + info->node = start_node; + info->obj_desc = method_obj; + info->node_flags = info->node->flags; + info->full_pathname = acpi_ns_get_normalized_pathname(info->node, TRUE); + if (!info->full_pathname) { + status = AE_NO_MEMORY; + goto cleanup; + } + + (void)acpi_ut_release_mutex(ACPI_MTX_NAMESPACE); + status = acpi_ps_execute_table(info); + (void)acpi_ut_acquire_mutex(ACPI_MTX_NAMESPACE); + +cleanup: + acpi_ut_remove_reference(method_obj); + ACPI_FREE(info->full_pathname); + info->full_pathname = NULL; + ACPI_FREE(info); + return_ACPI_STATUS(status); +} + +/******************************************************************************* + * * FUNCTION: ns_one_complete_parse * * PARAMETERS: pass_number - 1 or 2 @@ -63,6 +154,7 @@ ACPI_MODULE_NAME("nsparse") * DESCRIPTION: Perform one complete parse of an ACPI/AML table. * ******************************************************************************/ + acpi_status acpi_ns_one_complete_parse(u32 pass_number, u32 table_index, @@ -170,38 +262,47 @@ acpi_ns_parse_table(u32 table_index, struct acpi_namespace_node *start_node) ACPI_FUNCTION_TRACE(ns_parse_table); - /* - * AML Parse, pass 1 - * - * In this pass, we load most of the namespace. Control methods - * are not parsed until later. A parse tree is not created. Instead, - * each Parser Op subtree is deleted when it is finished. This saves - * a great deal of memory, and allows a small cache of parse objects - * to service the entire parse. The second pass of the parse then - * performs another complete parse of the AML. - */ - ACPI_DEBUG_PRINT((ACPI_DB_PARSE, "**** Start pass 1\n")); - - status = acpi_ns_one_complete_parse(ACPI_IMODE_LOAD_PASS1, - table_index, start_node); - if (ACPI_FAILURE(status)) { - return_ACPI_STATUS(status); - } + if (acpi_gbl_parse_table_as_term_list) { + ACPI_DEBUG_PRINT((ACPI_DB_PARSE, "**** Start load pass\n")); - /* - * AML Parse, pass 2 - * - * In this pass, we resolve forward references and other things - * that could not be completed during the first pass. - * Another complete parse of the AML is performed, but the - * overhead of this is compensated for by the fact that the - * parse objects are all cached. - */ - ACPI_DEBUG_PRINT((ACPI_DB_PARSE, "**** Start pass 2\n")); - status = acpi_ns_one_complete_parse(ACPI_IMODE_LOAD_PASS2, - table_index, start_node); - if (ACPI_FAILURE(status)) { - return_ACPI_STATUS(status); + status = acpi_ns_execute_table(table_index, start_node); + if (ACPI_FAILURE(status)) { + return_ACPI_STATUS(status); + } + } else { + /* + * AML Parse, pass 1 + * + * In this pass, we load most of the namespace. Control methods + * are not parsed until later. A parse tree is not created. + * Instead, each Parser Op subtree is deleted when it is finished. + * This saves a great deal of memory, and allows a small cache of + * parse objects to service the entire parse. The second pass of + * the parse then performs another complete parse of the AML. + */ + ACPI_DEBUG_PRINT((ACPI_DB_PARSE, "**** Start pass 1\n")); + + status = acpi_ns_one_complete_parse(ACPI_IMODE_LOAD_PASS1, + table_index, start_node); + if (ACPI_FAILURE(status)) { + return_ACPI_STATUS(status); + } + + /* + * AML Parse, pass 2 + * + * In this pass, we resolve forward references and other things + * that could not be completed during the first pass. + * Another complete parse of the AML is performed, but the + * overhead of this is compensated for by the fact that the + * parse objects are all cached. + */ + ACPI_DEBUG_PRINT((ACPI_DB_PARSE, "**** Start pass 2\n")); + status = acpi_ns_one_complete_parse(ACPI_IMODE_LOAD_PASS2, + table_index, start_node); + if (ACPI_FAILURE(status)) { + return_ACPI_STATUS(status); + } } return_ACPI_STATUS(status); diff --git a/drivers/acpi/acpica/psparse.c b/drivers/acpi/acpica/psparse.c index 0a23897..3aa9162 100644 --- a/drivers/acpi/acpica/psparse.c +++ b/drivers/acpi/acpica/psparse.c @@ -571,7 +571,9 @@ acpi_status acpi_ps_parse_aml(struct acpi_walk_state *walk_state) * cleanup to do */ if (((walk_state->parse_flags & ACPI_PARSE_MODE_MASK) == - ACPI_PARSE_EXECUTE) || (ACPI_FAILURE(status))) { + ACPI_PARSE_EXECUTE && + !(walk_state->parse_flags & ACPI_PARSE_MODULE_LEVEL)) || + (ACPI_FAILURE(status))) { acpi_ds_terminate_control_method(walk_state-> method_desc, walk_state); diff --git a/drivers/acpi/acpica/psxface.c b/drivers/acpi/acpica/psxface.c index cf30cd82..0f0500d 100644 --- a/drivers/acpi/acpica/psxface.c +++ b/drivers/acpi/acpica/psxface.c @@ -252,6 +252,79 @@ cleanup: /******************************************************************************* * + * FUNCTION: acpi_ps_execute_table + * + * PARAMETERS: info - Method info block, contains: + * node - Node to where the is entered into the + * namespace + * obj_desc - Pseudo method object describing the AML + * code of the entire table + * pass_number - Parse or execute pass + * + * RETURN: Status + * + * DESCRIPTION: Execute a table + * + ******************************************************************************/ + +acpi_status acpi_ps_execute_table(struct acpi_evaluate_info *info) +{ + acpi_status status; + union acpi_parse_object *op = NULL; + struct acpi_walk_state *walk_state = NULL; + + ACPI_FUNCTION_TRACE(ps_execute_table); + + /* Create and init a Root Node */ + + op = acpi_ps_create_scope_op(info->obj_desc->method.aml_start); + if (!op) { + status = AE_NO_MEMORY; + goto cleanup; + } + + /* Create and initialize a new walk state */ + + walk_state = + acpi_ds_create_walk_state(info->obj_desc->method.owner_id, NULL, + NULL, NULL); + if (!walk_state) { + status = AE_NO_MEMORY; + goto cleanup; + } + + status = acpi_ds_init_aml_walk(walk_state, op, info->node, + info->obj_desc->method.aml_start, + info->obj_desc->method.aml_length, info, + info->pass_number); + if (ACPI_FAILURE(status)) { + goto cleanup; + } + + if (info->obj_desc->method.info_flags & ACPI_METHOD_MODULE_LEVEL) { + walk_state->parse_flags |= ACPI_PARSE_MODULE_LEVEL; + } + + /* + * Parse the AML, walk_state will be deleted by parse_aml + */ + acpi_ex_enter_interpreter(); + status = acpi_ps_parse_aml(walk_state); + walk_state = NULL; + acpi_ex_exit_interpreter(); + +cleanup: + if (op) { + acpi_ps_delete_parse_tree(op); + } + if (walk_state) { + acpi_ds_delete_walk_state(walk_state); + } + return_ACPI_STATUS(status); +} + +/******************************************************************************* + * * FUNCTION: acpi_ps_update_parameter_list * * PARAMETERS: info - See struct acpi_evaluate_info diff --git a/drivers/acpi/acpica/tbxfload.c b/drivers/acpi/acpica/tbxfload.c index ac71abc..2b9e87f 100644 --- a/drivers/acpi/acpica/tbxfload.c +++ b/drivers/acpi/acpica/tbxfload.c @@ -103,7 +103,8 @@ acpi_status __init acpi_load_tables(void) "While loading namespace from ACPI tables")); } - if (!acpi_gbl_group_module_level_code) { + if (acpi_gbl_parse_table_as_term_list + || !acpi_gbl_group_module_level_code) { /* * Initialize the objects that remain uninitialized. This * runs the executable AML that may be part of the diff --git a/drivers/acpi/acpica/utxfinit.c b/drivers/acpi/acpica/utxfinit.c index 75b5f27..e41424a 100644 --- a/drivers/acpi/acpica/utxfinit.c +++ b/drivers/acpi/acpica/utxfinit.c @@ -265,7 +265,8 @@ acpi_status __init acpi_initialize_objects(u32 flags) * all of the tables have been loaded. It is a legacy option and is * not compatible with other ACPI implementations. See acpi_ns_load_table. */ - if (acpi_gbl_group_module_level_code) { + if (!acpi_gbl_parse_table_as_term_list + && acpi_gbl_group_module_level_code) { acpi_ns_exec_module_code_list(); /* diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h index 4e4c214..22b0397 100644 --- a/include/acpi/acpixf.h +++ b/include/acpi/acpixf.h @@ -195,6 +195,12 @@ ACPI_INIT_GLOBAL(u8, acpi_gbl_do_not_use_xsdt, FALSE); ACPI_INIT_GLOBAL(u8, acpi_gbl_group_module_level_code, FALSE); /* + * Optionally support module level code by parsing the entire table as + * a term_list. Default is FALSE, do not execute entire table. + */ +ACPI_INIT_GLOBAL(u8, acpi_gbl_parse_table_as_term_list, FALSE); + +/* * Optionally use 32-bit FADT addresses if and when there is a conflict * (address mismatch) between the 32-bit and 64-bit versions of the * address. Although ACPICA adheres to the ACPI specification which -- 1.7.10 ^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH v2 3/4] ACPI 2.0 / AML: Enable correct ACPI subsystem initialization order for new table loading mode 2016-05-13 5:35 ` [PATCH v2 0/4] ACPI 2.0: Enable TermList interpretion for table loading Lv Zheng 2016-05-13 5:35 ` [PATCH v2 1/4] ACPICA: Dispatcher: Fix an issue that the opregions created by the linked MLC were not tracked Lv Zheng 2016-05-13 5:35 ` [PATCH v2 2/4] ACPICA: ACPI 2.0, Interpreter: Fix MLC issues by switching to new TermList grammar for table loading Lv Zheng @ 2016-05-13 5:35 ` Lv Zheng 2016-05-13 5:35 ` [PATCH v2 4/4] ACPI 2.0 / AML: Fix module level execution by correctly parsing table as TermList Lv Zheng 2016-05-17 0:29 ` [PATCH v2 0/4] ACPI 2.0: Enable TermList interpretion for table loading Zheng, Lv 4 siblings, 0 replies; 40+ messages in thread From: Lv Zheng @ 2016-05-13 5:35 UTC (permalink / raw) To: Rafael J. Wysocki, Rafael J. Wysocki, Len Brown Cc: Lv Zheng, Lv Zheng, linux-kernel, linux-acpi This patch enables the following initialization order for the new table loading mode (which is enabled by setting acpi_gbl_parse_table_as_term_list to TRUE): 1. Install default region handlers (SystemMemory, SystemIo, PciConfig, EmbeddedControl via ECDT) without evaluating _REG; 2. Load the table and execute the module level AML opcodes instantly. Signed-off-by: Lv Zheng <lv.zheng@intel.com> --- drivers/acpi/bus.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c index 31e8da6..d177649 100644 --- a/drivers/acpi/bus.c +++ b/drivers/acpi/bus.c @@ -925,7 +925,8 @@ void __init acpi_early_init(void) goto error0; } - if (acpi_gbl_group_module_level_code) { + if (!acpi_gbl_parse_table_as_term_list && + acpi_gbl_group_module_level_code) { status = acpi_load_tables(); if (ACPI_FAILURE(status)) { printk(KERN_ERR PREFIX @@ -1008,7 +1009,8 @@ static int __init acpi_bus_init(void) status = acpi_ec_ecdt_probe(); /* Ignore result. Not having an ECDT is not fatal. */ - if (!acpi_gbl_group_module_level_code) { + if (acpi_gbl_parse_table_as_term_list || + !acpi_gbl_group_module_level_code) { status = acpi_load_tables(); if (ACPI_FAILURE(status)) { printk(KERN_ERR PREFIX -- 1.7.10 ^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH v2 4/4] ACPI 2.0 / AML: Fix module level execution by correctly parsing table as TermList 2016-05-13 5:35 ` [PATCH v2 0/4] ACPI 2.0: Enable TermList interpretion for table loading Lv Zheng ` (2 preceding siblings ...) 2016-05-13 5:35 ` [PATCH v2 3/4] ACPI 2.0 / AML: Enable correct ACPI subsystem initialization order for new table loading mode Lv Zheng @ 2016-05-13 5:35 ` Lv Zheng 2016-05-17 0:29 ` [PATCH v2 0/4] ACPI 2.0: Enable TermList interpretion for table loading Zheng, Lv 4 siblings, 0 replies; 40+ messages in thread From: Lv Zheng @ 2016-05-13 5:35 UTC (permalink / raw) To: Rafael J. Wysocki, Rafael J. Wysocki, Len Brown Cc: Lv Zheng, Lv Zheng, linux-kernel, linux-acpi This experiment follows de-facto standard behavior, parsing entire table as a single TermList, so that all module level executions are possible during the table loading. If regressions are found against the enabling of this experimental fix, this patch is the only one that should get bisected out. Please report the regressions to the kernel bugzilla for further root causing. Signed-off-by: Lv Zheng <lv.zheng@intel.com> --- include/acpi/acpixf.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h index 22b0397..e21bdd1 100644 --- a/include/acpi/acpixf.h +++ b/include/acpi/acpixf.h @@ -196,9 +196,9 @@ ACPI_INIT_GLOBAL(u8, acpi_gbl_group_module_level_code, FALSE); /* * Optionally support module level code by parsing the entire table as - * a term_list. Default is FALSE, do not execute entire table. + * a term_list. Default is TRUE, do execute entire table. */ -ACPI_INIT_GLOBAL(u8, acpi_gbl_parse_table_as_term_list, FALSE); +ACPI_INIT_GLOBAL(u8, acpi_gbl_parse_table_as_term_list, TRUE); /* * Optionally use 32-bit FADT addresses if and when there is a conflict -- 1.7.10 ^ permalink raw reply related [flat|nested] 40+ messages in thread
* RE: [PATCH v2 0/4] ACPI 2.0: Enable TermList interpretion for table loading 2016-05-13 5:35 ` [PATCH v2 0/4] ACPI 2.0: Enable TermList interpretion for table loading Lv Zheng ` (3 preceding siblings ...) 2016-05-13 5:35 ` [PATCH v2 4/4] ACPI 2.0 / AML: Fix module level execution by correctly parsing table as TermList Lv Zheng @ 2016-05-17 0:29 ` Zheng, Lv 2016-05-17 23:23 ` Rafael J. Wysocki 4 siblings, 1 reply; 40+ messages in thread From: Zheng, Lv @ 2016-05-17 0:29 UTC (permalink / raw) To: Wysocki, Rafael J, Rafael J. Wysocki, Brown, Len Cc: Lv Zheng, linux-kernel, linux-acpi Hi, Rafael Can we queue this up in linux-next? ASLTS recursive tests are done in ACPICA upstream and no regressions can be seen. We need more tests around this experimental change from the real users to have the chances to learn the unknown cases. If they reported regressions, we could stop the regressions by reverting [PATCH 4/4]. So it should be safe to do such experiments in the Linux upstream. Thanks in advance. Best regards -Lv > From: Zheng, Lv > Subject: [PATCH v2 0/4] ACPI 2.0: Enable TermList interpretion for table loading > > MLC (module level code) is an ACPICA terminology describing the AML code > out of any control method, currently only Type1Opcode (If/Else/While) > wrapped MLC code blocks are executed by the AML interpreter after the table > loading. But the issue which is fixed by this patchset is: > Not only Type1Opcode, but also Type2Opcode will be executed as MLC and > MLC is not executed after loading the table, but is executed right in > place. > > The following AML code is assembled into a static loading SSDT, and used > as an instrumentation to pry into the de-facto standard AML interpreter > behaviors: > Name (ECOK, Zero) > Scope (\) > { > DBUG ("TermList 1") > If (LEqual (ECOK, Zero)) > { > DBUG ("TermList 2") > Device (MDEV) > { > DEBUG (TermList 3") > If (CondRefOf (MDEV)) > { > DBUG ("MDEV exists") > } > If (CondRefOf (MDEV._STA)) > { > DBUG ("MDEV._STA exists") > } > If (CondRefOf (\_SB.PCI0.EC)) > { > DBUG ("\\_SB.PCI0.EC exists") > } > Name (_HID, EisaId ("PNP9999")) > Method (_STA, 0, Serialized) > { > DEBUG ("\\_SB.MDEV._STA") > Return (0x0F) > } > } > DBUG ("TermList 4") > } > Method (_INI, 0, Serialized) > { > DBUG ("\\_SB._INI") > } > } > Scope (_SB.PCI0) > { > Device (EC) > { > ... > } > } > The DBUG function is a function to write the debugging messages into a > SystemIo debug port. > Running Windows with the BIOS providing this SSDT via RSDT, the following > messages are obtained from the debug port: > TermList 1 > TermList 2 > TermList 3 > \_SB.MDEV exists > TermList 4 > \_SB._INI > ... > > This test reveals the de-facto grammar for the AMLCode to us: > 1. During the table loading, MLC will be executed by the interpreter, this > is partially supported by the current ACPICA; > 2. For SystemIo, not only after the _REG(1, 1) is evaluated (current ACPICA > interpreter limitation), but when the table is being loaded, the > SystemIo (the debugging port) is accessible, this is recently fixed in > the upstream, now all early operation regions are accessible during the > table loading; > 3. Not only Type1Opcode, but also Type2Opcode will be executed as MLC and > MLC is not executed after loading the table, but is executed right in > place, the Linux upstream is not compliant to this behavior. > > The last compliance issue has already been clarified in ACPI 2.0 > specification, so the compliance issue is not that Linux is not compliant > to the de-facto standard OS, but that Linux is not compliant to ACPI 2.0. > Definition block tables in fact is defined by the spec as TermList, which > has no difference than the control methods, thus the interpretion of the > table should be no difference that the control method evaluation: > AMLCode := DefBlockHeader TermList > DefMethod := MethodOp PkgLength NameString MethodFlags TermList > > Why ACPICA interpreter is acting so differently from this definition? This > is because, there are many software entropies preventing this from being > enabled, such entropies need to be cleaned up first in order not to trigger > regressions for specific platforms. These entropies include: > 1. ECDT support is broken. In fact, the original EC driver was correct, but > devlopers started to use the namespace EC instead of ECDT just because > several broken ECDT tables were reported on the bugzilla. They trusted > the namespace EC settings rather than the ECDT ones, this led to the > evaluation of _REG/_GPE/_CRS and namespace walk before executing the > module level AML opcodes. And the fixes in fact finally disable early EC > usages (used during table loading and early device enumeration > processes). > 2. _REG evaluations are wrong. ACPICA provides APIs for OSPMs to register > operation region handlers. But for the early operation region accesses, > ACPI spec declares that the evaluations of _REG are not required, but > the ACPICA APIs do not avoid running _REG to meet this early > requirements. Code to fix this is partially upstreamed during previous > ACPICA release cycle. > 3. _REG associations are wrong. ACPICA associates _REG control method to > all operation region objects before executing the _REG control method. > This can happen even when a control method is evaluated and operation > regions defined in the method is initialized > (acpi_ev_initialize_region). As a part of the ACPICA internal _REG > evaluation state machine, it requires the namespace walk, and all > namespace walk should be ensured to happen only "AFTER THE NAMESPACE > IS > INITIALIZED". But when this logic happens during the table loading, it > may fail in finding the _REG method since the _REG method may not be > created by the interpreter just because _REG is defined after the > operation region object's declaration. > 4. _REG(CONNECT)/_REG(DISCONNECT) executions are not balanced, this can > lead to wrong table loading/unloading results. Since _REG evaluations > require the releasing of all interpreter/namespace locks in order to > allow another evaluation to happen, and ACPICA operand object > destruction code can be invoked from different locking environment, this > becomes difficult for the developers to provide one single function to > make _REG(CONNECT)/_REG(DISCONNECT) balanced. > 5. \_SB._INI is not the first control method evaluated by the interpreter. > Many platforms put initialization code in \_SB._INI in order to have > named objects initialized very early during the device enumeration > process. Without this order strictly ensured, early operation region > access enabling could break these platforms. > 6. Linux initialization order is wrong, it is now: > a. load namespace without executing root scope If/Else/While module > level code blocks; > b. probe ECDT and instal EmbeddedControl operation region handler with > _REG evaluated; > c. install SystemMemory, SystemIo, PciConfig operation region handlers > without evaluating _REG; > d. run _REG for SystemMemory, SystemIo, PciConfig operation regions; > e. execute root scope If/Else/While module level code blocks; > f. enable GPE and namespace EC. > While the correct order should be: > a. probe ECDT and install EmbeddedControl operation region handler > without evaluating _REG; > b. install SystemMemory, SystemIo, PciConfig operation region handlers > without evaluating _REG; > c. load namespace, in the meanshile, execute all module level AML > opcodes; > d. run _REG for SystemMemory, SystemIo, PciConfig operation regions; > e. enable GPE and namespace EC which results in _REG evaluation for EC. > > Until now we've upstreamed most of the entropy fixes into the Linux kernel, > tested the grammar switch in the ACPICA upstream using ASLTS and no > significant regressions can be seen while we need more tests before it is > merged: > https://github.com/acpica/acpica/pull/134 > This is the ASLTS running result after applying the grammar switch, the > test cases include new "module" case for which ACPICA interpreter cannot > pass without this grammar switch applied: > ============================================================ > Test cases specified for running: > arithmetic > bfield > constant > control > descriptor > logic > manipulation > name > reference > region > synchronization > table > misc > provoke > oarg > oconst > olocal > onamedloc > onamedglob > opackageel > oreftonamed > oreftopackageel > oreturn > rstore > roptional > rcopyobject > rindecrement > rexplicitconv > badasl > namespace > exc > exc_ref > exc_operand2 > exc_result2 > exc_tbl > mt_mutex > extra > extra_aslts > bdemo > bdemof > condbranches > TOTAL: (32-bit norm mode) > PASS : 0 > FAIL : 0 > BLOCKED : 0 > SKIPPED : 0 > Tests : 0 > Test Cases : 40 (of 47) > Test Collections : 7 (of 8) > Outstanding allocations after execution : 0 > Outstanding allocations (ACPI Error) : 0 > Large Reference Count (ACPI Error) : 0 > Memory consumption total : 0 Kb > TOTAL: (64-bit norm mode) > PASS : 0 > FAIL : 0 > BLOCKED : 0 > SKIPPED : 0 > Tests : 0 > Test Cases : 40 (of 47) > Test Collections : 7 (of 8) > Outstanding allocations after execution : 0 > Outstanding allocations (ACPI Error) : 0 > Large Reference Count (ACPI Error) : 0 > Memory consumption total : 0 Kb > TOTAL: (32-bit slack mode) > PASS : 0 > FAIL : 0 > BLOCKED : 0 > SKIPPED : 0 > Tests : 0 > Test Cases : 40 (of 47) > Test Collections : 7 (of 8) > Outstanding allocations after execution : 0 > Outstanding allocations (ACPI Error) : 0 > Large Reference Count (ACPI Error) : 0 > Memory consumption total : 0 Kb > TOTAL: (64-bit slack mode) > PASS : 0 > FAIL : 0 > BLOCKED : 0 > SKIPPED : 0 > Tests : 0 > Test Cases : 40 (of 47) > Test Collections : 7 (of 8) > Outstanding allocations after execution : 0 > Outstanding allocations (ACPI Error) : 0 > Large Reference Count (ACPI Error) : 0 > Memory consumption total : 0 Kb > ============================================================ > > Since we need more tests from the real users, we could make the grammar > switch released from the Linux upstream. It's safe to do so because we have > implemented regression protection (acpi_gbl_parse_table_as_term_list) in > the fixes. The earlier the fix is tested by more real users, the better > quality can be achieved by knowing the unknown cases (if any). > > Lv Zheng (4): > ACPICA: Dispatcher: Fix an issue that the opregions created by the > linked MLC were not tracked > ACPICA: ACPI 2.0, Interpreter: Fix MLC issues by switching to new > TermList grammar for table loading > ACPI 2.0 / AML: Enable correct ACPI subsystem initialization order > for new table loading mode > ACPI 2.0 / AML: Fix module level execution by correctly parsing table > as TermList > > drivers/acpi/acpica/acnamesp.h | 3 + > drivers/acpi/acpica/acparser.h | 2 + > drivers/acpi/acpica/dsopcode.c | 6 ++ > drivers/acpi/acpica/evrgnini.c | 3 +- > drivers/acpi/acpica/exconfig.c | 6 +- > drivers/acpi/acpica/nsload.c | 3 +- > drivers/acpi/acpica/nsparse.c | 163 ++++++++++++++++++++++++++++++++-- > ------ > drivers/acpi/acpica/psparse.c | 4 +- > drivers/acpi/acpica/psxface.c | 73 ++++++++++++++++++ > drivers/acpi/acpica/tbxfload.c | 3 +- > drivers/acpi/acpica/utxfinit.c | 3 +- > drivers/acpi/bus.c | 6 +- > include/acpi/acpixf.h | 6 ++ > 13 files changed, 241 insertions(+), 40 deletions(-) > > -- > 1.7.10 ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2 0/4] ACPI 2.0: Enable TermList interpretion for table loading 2016-05-17 0:29 ` [PATCH v2 0/4] ACPI 2.0: Enable TermList interpretion for table loading Zheng, Lv @ 2016-05-17 23:23 ` Rafael J. Wysocki 2016-05-20 0:57 ` Zheng, Lv 0 siblings, 1 reply; 40+ messages in thread From: Rafael J. Wysocki @ 2016-05-17 23:23 UTC (permalink / raw) To: Zheng, Lv Cc: Wysocki, Rafael J, Rafael J. Wysocki, Brown, Len, Lv Zheng, linux-kernel, linux-acpi On Tue, May 17, 2016 at 2:29 AM, Zheng, Lv <lv.zheng@intel.com> wrote: > Hi, Rafael > > Can we queue this up in linux-next? > ASLTS recursive tests are done in ACPICA upstream and no regressions can be seen. > We need more tests around this experimental change from the real users to have the chances to learn the unknown cases. > If they reported regressions, we could stop the regressions by reverting [PATCH 4/4]. > So it should be safe to do such experiments in the Linux upstream. > Thanks in advance. There is a rule that during a merge window linux-next should only contain material for that merge window. That is, currently linux-next should only contain material targeted at v4.7. For this reason, I can't put the series into linux-next right now, but I'll do that as soon as 4.7-rc1 is released. ^ permalink raw reply [flat|nested] 40+ messages in thread
* RE: [PATCH v2 0/4] ACPI 2.0: Enable TermList interpretion for table loading 2016-05-17 23:23 ` Rafael J. Wysocki @ 2016-05-20 0:57 ` Zheng, Lv 2016-06-10 1:05 ` Rafael J. Wysocki 0 siblings, 1 reply; 40+ messages in thread From: Zheng, Lv @ 2016-05-20 0:57 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Wysocki, Rafael J, Rafael J. Wysocki, Brown, Len, Lv Zheng, linux-kernel, linux-acpi Hi, Rafael > From: rjwysocki@gmail.com [mailto:rjwysocki@gmail.com] On Behalf Of > Rafael J. Wysocki > Subject: Re: [PATCH v2 0/4] ACPI 2.0: Enable TermList interpretion for table > loading > > On Tue, May 17, 2016 at 2:29 AM, Zheng, Lv <lv.zheng@intel.com> wrote: > > Hi, Rafael > > > > Can we queue this up in linux-next? > > ASLTS recursive tests are done in ACPICA upstream and no regressions can be > seen. > > We need more tests around this experimental change from the real users to > have the chances to learn the unknown cases. > > If they reported regressions, we could stop the regressions by reverting > [PATCH 4/4]. > > So it should be safe to do such experiments in the Linux upstream. > > Thanks in advance. > > There is a rule that during a merge window linux-next should only > contain material for that merge window. That is, currently linux-next > should only contain material targeted at v4.7. > > For this reason, I can't put the series into linux-next right now, but > I'll do that as soon as 4.7-rc1 is released. [Lv Zheng] Great! Thanks for the information. Best regards -Lv ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2 0/4] ACPI 2.0: Enable TermList interpretion for table loading 2016-05-20 0:57 ` Zheng, Lv @ 2016-06-10 1:05 ` Rafael J. Wysocki 2016-06-12 1:02 ` Zheng, Lv 0 siblings, 1 reply; 40+ messages in thread From: Rafael J. Wysocki @ 2016-06-10 1:05 UTC (permalink / raw) To: Zheng, Lv, Rafael J. Wysocki Cc: Rafael J. Wysocki, Brown, Len, Lv Zheng, linux-kernel, linux-acpi On 5/20/2016 2:57 AM, Zheng, Lv wrote: > Hi, Rafael > >> From: rjwysocki@gmail.com [mailto:rjwysocki@gmail.com] On Behalf Of >> Rafael J. Wysocki >> Subject: Re: [PATCH v2 0/4] ACPI 2.0: Enable TermList interpretion for table >> loading >> >> On Tue, May 17, 2016 at 2:29 AM, Zheng, Lv <lv.zheng@intel.com> wrote: >>> Hi, Rafael >>> >>> Can we queue this up in linux-next? >>> ASLTS recursive tests are done in ACPICA upstream and no regressions can be >> seen. >>> We need more tests around this experimental change from the real users to >> have the chances to learn the unknown cases. >>> If they reported regressions, we could stop the regressions by reverting >> [PATCH 4/4]. >>> So it should be safe to do such experiments in the Linux upstream. >>> Thanks in advance. >> There is a rule that during a merge window linux-next should only >> contain material for that merge window. That is, currently linux-next >> should only contain material targeted at v4.7. >> >> For this reason, I can't put the series into linux-next right now, but >> I'll do that as soon as 4.7-rc1 is released. > [Lv Zheng] > Great! > Thanks for the information. Unfortunately, it is reported that the series actually causes problems to happen. I sent you a CC of my response to the report in question earlier today. Thanks, Rafael ^ permalink raw reply [flat|nested] 40+ messages in thread
* RE: [PATCH v2 0/4] ACPI 2.0: Enable TermList interpretion for table loading 2016-06-10 1:05 ` Rafael J. Wysocki @ 2016-06-12 1:02 ` Zheng, Lv 2016-06-13 22:59 ` Rafael J. Wysocki 0 siblings, 1 reply; 40+ messages in thread From: Zheng, Lv @ 2016-06-12 1:02 UTC (permalink / raw) To: Wysocki, Rafael J, Rafael J. Wysocki Cc: Rafael J. Wysocki, Brown, Len, Lv Zheng, linux-kernel, linux-acpi Hi, > From: Wysocki, Rafael J > Subject: Re: [PATCH v2 0/4] ACPI 2.0: Enable TermList interpretion for > table loading > > On 5/20/2016 2:57 AM, Zheng, Lv wrote: > > Hi, Rafael > > > >> From: rjwysocki@gmail.com [mailto:rjwysocki@gmail.com] On Behalf > Of > >> Rafael J. Wysocki > >> Subject: Re: [PATCH v2 0/4] ACPI 2.0: Enable TermList interpretion for > table > >> loading > >> > >> On Tue, May 17, 2016 at 2:29 AM, Zheng, Lv <lv.zheng@intel.com> > wrote: > >>> Hi, Rafael > >>> > >>> Can we queue this up in linux-next? > >>> ASLTS recursive tests are done in ACPICA upstream and no regressions > can be > >> seen. > >>> We need more tests around this experimental change from the real > users to > >> have the chances to learn the unknown cases. > >>> If they reported regressions, we could stop the regressions by > reverting > >> [PATCH 4/4]. > >>> So it should be safe to do such experiments in the Linux upstream. > >>> Thanks in advance. > >> There is a rule that during a merge window linux-next should only > >> contain material for that merge window. That is, currently linux-next > >> should only contain material targeted at v4.7. > >> > >> For this reason, I can't put the series into linux-next right now, but > >> I'll do that as soon as 4.7-rc1 is released. > > [Lv Zheng] > > Great! > > Thanks for the information. > > Unfortunately, it is reported that the series actually causes problems > to happen. > > I sent you a CC of my response to the report in question earlier today. [Lv Zheng] Could the problem be stopped by reverting PATCH 4/4? Let me check with the reporter to learn the case. Thanks and best regards -Lv ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2 0/4] ACPI 2.0: Enable TermList interpretion for table loading 2016-06-12 1:02 ` Zheng, Lv @ 2016-06-13 22:59 ` Rafael J. Wysocki 0 siblings, 0 replies; 40+ messages in thread From: Rafael J. Wysocki @ 2016-06-13 22:59 UTC (permalink / raw) To: Zheng, Lv, Rafael J. Wysocki Cc: Rafael J. Wysocki, Brown, Len, Lv Zheng, linux-kernel, linux-acpi On 6/12/2016 3:02 AM, Zheng, Lv wrote: > Hi, > >> From: Wysocki, Rafael J >> Subject: Re: [PATCH v2 0/4] ACPI 2.0: Enable TermList interpretion for >> table loading >> >> On 5/20/2016 2:57 AM, Zheng, Lv wrote: >>> Hi, Rafael >>> >>>> From: rjwysocki@gmail.com [mailto:rjwysocki@gmail.com] On Behalf >> Of >>>> Rafael J. Wysocki >>>> Subject: Re: [PATCH v2 0/4] ACPI 2.0: Enable TermList interpretion for >> table >>>> loading >>>> >>>> On Tue, May 17, 2016 at 2:29 AM, Zheng, Lv <lv.zheng@intel.com> >> wrote: >>>>> Hi, Rafael >>>>> >>>>> Can we queue this up in linux-next? >>>>> ASLTS recursive tests are done in ACPICA upstream and no regressions >> can be >>>> seen. >>>>> We need more tests around this experimental change from the real >> users to >>>> have the chances to learn the unknown cases. >>>>> If they reported regressions, we could stop the regressions by >> reverting >>>> [PATCH 4/4]. >>>>> So it should be safe to do such experiments in the Linux upstream. >>>>> Thanks in advance. >>>> There is a rule that during a merge window linux-next should only >>>> contain material for that merge window. That is, currently linux-next >>>> should only contain material targeted at v4.7. >>>> >>>> For this reason, I can't put the series into linux-next right now, but >>>> I'll do that as soon as 4.7-rc1 is released. >>> [Lv Zheng] >>> Great! >>> Thanks for the information. >> Unfortunately, it is reported that the series actually causes problems >> to happen. >> >> I sent you a CC of my response to the report in question earlier today. > [Lv Zheng] > Could the problem be stopped by reverting PATCH 4/4? Yes, it can. However, would the series be any useful without that patch? Thanks, Rafael ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v3 0/5] ACPI 2.0: Enable TermList interpretion for table loading [not found] <0e65135af51d94db0410c7059f3bc3a2300fc3b5> 2016-05-13 5:35 ` [PATCH v2 0/4] ACPI 2.0: Enable TermList interpretion for table loading Lv Zheng @ 2016-06-20 9:07 ` Lv Zheng 2016-06-20 9:07 ` [PATCH v3 1/5] ACPICA: Namespace: Fix a regression that MLC support triggers dead lock in dynamic " Lv Zheng ` (4 more replies) 2016-06-21 4:34 ` [PATCH v4 0/5] ACPI 2.0: Enable TermList interpretion for table loading Lv Zheng 2016-09-23 3:26 ` [PATCH v5 0/5] ACPI 2.0: Stop defer-executing module level code Lv Zheng 3 siblings, 5 replies; 40+ messages in thread From: Lv Zheng @ 2016-06-20 9:07 UTC (permalink / raw) To: Rafael J. Wysocki, Rafael J. Wysocki, Len Brown Cc: Lv Zheng, Lv Zheng, linux-kernel, linux-acpi MLC (module level code) is an ACPICA terminology describing the AML code out of any control method, currently only Type1Opcode (If/Else/While) wrapped MLC code blocks are executed by the AML interpreter after the table loading. But the issue which is fixed by this patchset is: Not only Type1Opcode, but also Type2Opcode will be executed as MLC and MLC is not executed after loading the table, but is executed right in place. The following AML code is assembled into a static loading SSDT, and used as an instrumentation to pry into the de-facto standard AML interpreter behaviors: Name (ECOK, Zero) Scope (\) { DBUG ("TermList 1") If (LEqual (ECOK, Zero)) { DBUG ("TermList 2") Device (MDEV) { DEBUG (TermList 3") If (CondRefOf (MDEV)) { DBUG ("MDEV exists") } If (CondRefOf (MDEV._STA)) { DBUG ("MDEV._STA exists") } If (CondRefOf (\_SB.PCI0.EC)) { DBUG ("\\_SB.PCI0.EC exists") } Name (_HID, EisaId ("PNP9999")) Method (_STA, 0, Serialized) { DEBUG ("\\_SB.MDEV._STA") Return (0x0F) } } DBUG ("TermList 4") } Method (_INI, 0, Serialized) { DBUG ("\\_SB._INI") } } Scope (_SB.PCI0) { Device (EC) { ... } } The DBUG function is a function to write the debugging messages into a SystemIo debug port. Running Windows with the BIOS providing this SSDT via RSDT, the following messages are obtained from the debug port: TermList 1 TermList 2 TermList 3 \_SB.MDEV exists TermList 4 \_SB._INI ... This test reveals the de-facto grammar for the AMLCode to us: 1. During the table loading, MLC will be executed by the interpreter, this is partially supported by the current ACPICA; 2. For SystemIo, not only after the _REG(1, 1) is evaluated (current ACPICA interpreter limitation), but when the table is being loaded, the SystemIo (the debugging port) is accessible, this is recently fixed in the upstream, now all early operation regions are accessible during the table loading; 3. Not only Type1Opcode, but also Type2Opcode will be executed as MLC and MLC is not executed after loading the table, but is executed right in place, the Linux upstream is not compliant to this behavior. The last compliance issue has already been clarified in ACPI 2.0 specification, so the compliance issue is not that Linux is not compliant to the de-facto standard OS, but that Linux is not compliant to ACPI 2.0. Definition block tables in fact is defined by the spec as TermList, which has no difference than the control methods, thus the interpretion of the table should be no difference that the control method evaluation: AMLCode := DefBlockHeader TermList DefMethod := MethodOp PkgLength NameString MethodFlags TermList Why ACPICA interpreter is acting so differently from this definition? This is because, there are many software entropies preventing this from being enabled, such entropies need to be cleaned up first in order not to trigger regressions for specific platforms. These entropies include: 1. ECDT support is broken. In fact, the original EC driver was correct, but devlopers started to use the namespace EC instead of ECDT just because several broken ECDT tables were reported on the bugzilla. They trusted the namespace EC settings rather than the ECDT ones, this led to the evaluation of _REG/_GPE/_CRS and namespace walk before executing the module level AML opcodes. And the fixes in fact finally disable early EC usages (used during table loading and early device enumeration processes). 2. _REG evaluations are wrong. ACPICA provides APIs for OSPMs to register operation region handlers. But for the early operation region accesses, ACPI spec declares that the evaluations of _REG are not required, but the ACPICA APIs do not avoid running _REG to meet this early requirements. Code to fix this is partially upstreamed during previous ACPICA release cycle. 3. _REG associations are wrong. ACPICA associates _REG control method to all operation region objects before executing the _REG control method. This can happen even when a control method is evaluated and operation regions defined in the method is initialized (acpi_ev_initialize_region). As a part of the ACPICA internal _REG evaluation state machine, it requires the namespace walk, and all namespace walk should be ensured to happen only "AFTER THE NAMESPACE IS INITIALIZED". But when this logic happens during the table loading, it may fail in finding the _REG method since the _REG method may not be created by the interpreter just because _REG is defined after the operation region object's declaration. 4. _REG(CONNECT)/_REG(DISCONNECT) executions are not balanced, this can lead to wrong table loading/unloading results. Since _REG evaluations require the releasing of all interpreter/namespace locks in order to allow another evaluation to happen, and ACPICA operand object destruction code can be invoked from different locking environment, this becomes difficult for the developers to provide one single function to make _REG(CONNECT)/_REG(DISCONNECT) balanced. 5. \_SB._INI is not the first control method evaluated by the interpreter. Many platforms put initialization code in \_SB._INI in order to have named objects initialized very early during the device enumeration process. Without this order strictly ensured, early operation region access enabling could break these platforms. 6. Linux initialization order is wrong, it is now: a. load namespace without executing root scope If/Else/While module level code blocks; b. probe ECDT and instal EmbeddedControl operation region handler with _REG evaluated; c. install SystemMemory, SystemIo, PciConfig operation region handlers without evaluating _REG; d. run _REG for SystemMemory, SystemIo, PciConfig operation regions; e. execute root scope If/Else/While module level code blocks; f. enable GPE and namespace EC. While the correct order should be: a. probe ECDT and install EmbeddedControl operation region handler without evaluating _REG; b. install SystemMemory, SystemIo, PciConfig operation region handlers without evaluating _REG; c. load namespace, in the meanshile, execute all module level AML opcodes; d. run _REG for SystemMemory, SystemIo, PciConfig operation regions; e. enable GPE and namespace EC which results in _REG evaluation for EC. Until now we've upstreamed most of the entropy fixes into the Linux kernel, tested the grammar switch in the ACPICA upstream using ASLTS and no significant regressions can be seen while we need more tests before it is merged: https://github.com/acpica/acpica/pull/134 This is the ASLTS running result after applying the grammar switch, the test cases include new "module" case for which ACPICA interpreter cannot pass without this grammar switch applied: ============================================================ Test cases specified for running: arithmetic bfield constant control descriptor logic manipulation name reference region synchronization table misc provoke oarg oconst olocal onamedloc onamedglob opackageel oreftonamed oreftopackageel oreturn rstore roptional rcopyobject rindecrement rexplicitconv badasl namespace exc exc_ref exc_operand2 exc_result2 exc_tbl mt_mutex extra extra_aslts bdemo bdemof condbranches TOTAL: (32-bit norm mode) PASS : 0 FAIL : 0 BLOCKED : 0 SKIPPED : 0 Tests : 0 Test Cases : 40 (of 47) Test Collections : 7 (of 8) Outstanding allocations after execution : 0 Outstanding allocations (ACPI Error) : 0 Large Reference Count (ACPI Error) : 0 Memory consumption total : 0 Kb TOTAL: (64-bit norm mode) PASS : 0 FAIL : 0 BLOCKED : 0 SKIPPED : 0 Tests : 0 Test Cases : 40 (of 47) Test Collections : 7 (of 8) Outstanding allocations after execution : 0 Outstanding allocations (ACPI Error) : 0 Large Reference Count (ACPI Error) : 0 Memory consumption total : 0 Kb TOTAL: (32-bit slack mode) PASS : 0 FAIL : 0 BLOCKED : 0 SKIPPED : 0 Tests : 0 Test Cases : 40 (of 47) Test Collections : 7 (of 8) Outstanding allocations after execution : 0 Outstanding allocations (ACPI Error) : 0 Large Reference Count (ACPI Error) : 0 Memory consumption total : 0 Kb TOTAL: (64-bit slack mode) PASS : 0 FAIL : 0 BLOCKED : 0 SKIPPED : 0 Tests : 0 Test Cases : 40 (of 47) Test Collections : 7 (of 8) Outstanding allocations after execution : 0 Outstanding allocations (ACPI Error) : 0 Large Reference Count (ACPI Error) : 0 Memory consumption total : 0 Kb ============================================================ Since we need more tests from the real users, we could make the grammar switch released from the Linux upstream. It's safe to do so because we have implemented regression protection (acpi_gbl_parse_table_as_term_list) in the fixes. The earlier the fix is tested by more real users, the better quality can be achieved by knowing the unknown cases (if any). Lv Zheng (5): ACPICA: Namespace: Fix a regression that MLC support triggers dead lock in dynamic table loading ACPICA: Dispatcher: Fix an issue that the opregions created by the linked MLC were not tracked ACPICA: ACPI 2.0, Interpreter: Fix MLC issues by switching to new TermList grammar for table loading ACPI 2.0 / AML: Enable correct ACPI subsystem initialization order for new table loading mode ACPI 2.0 / AML: Fix module level execution by correctly parsing table as TermList drivers/acpi/acpica/acnamesp.h | 3 + drivers/acpi/acpica/acparser.h | 2 + drivers/acpi/acpica/dsopcode.c | 6 ++ drivers/acpi/acpica/evrgnini.c | 3 +- drivers/acpi/acpica/exconfig.c | 5 +- drivers/acpi/acpica/nsload.c | 3 +- drivers/acpi/acpica/nsparse.c | 169 ++++++++++++++++++++++++++++++++-------- drivers/acpi/acpica/psparse.c | 4 +- drivers/acpi/acpica/psxface.c | 71 +++++++++++++++++ drivers/acpi/acpica/tbxfload.c | 3 +- drivers/acpi/acpica/utxfinit.c | 3 +- drivers/acpi/bus.c | 6 +- include/acpi/acpixf.h | 6 ++ 13 files changed, 245 insertions(+), 39 deletions(-) -- 1.7.10 ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v3 1/5] ACPICA: Namespace: Fix a regression that MLC support triggers dead lock in dynamic table loading 2016-06-20 9:07 ` [PATCH v3 0/5] " Lv Zheng @ 2016-06-20 9:07 ` Lv Zheng 2016-06-20 10:57 ` Mika Westerberg 2016-06-20 9:07 ` [PATCH v3 2/5] ACPICA: Dispatcher: Fix an issue that the opregions created by the linked MLC were not tracked Lv Zheng ` (3 subsequent siblings) 4 siblings, 1 reply; 40+ messages in thread From: Lv Zheng @ 2016-06-20 9:07 UTC (permalink / raw) To: Rafael J. Wysocki, Rafael J. Wysocki, Len Brown Cc: Lv Zheng, Lv Zheng, linux-kernel, linux-acpi The new MLC approach invokes MLC per-table basis. But the dynamic loading support of this is incorrect because of the lock order: acpi_ns_evaluate acpi_ex_enter_intperter acpi_ns_load_table (triggered by Load opcode) acpi_ns_exec_module_code_list acpi_ex_enter_intperter The regression is introduced by the following commit: Commit: 2785ce8d0da1cac9d8f78615e116cf929e9a9123 ACPICA Commit: 071eff738c59eda1792ac24b3b688b61691d7e7c Subject: ACPICA: Add per-table execution of module-level code This patch fixes this regression by unlocking the interpreter lock before invoking MLC. However the unlocking is done to the acpi_ns_load_table(), in which, the interpreter lock should be locked by acpi_ns_parse_table() but wasn't. Reported by Mika Westerberg. Fixed by Lv Zheng. Fixes: 2785ce8d0da1 ("ACPICA: Add per-table execution of module-level code") Reported-by: Mika Westerberg <mika.westerberg@linux.intel.com> Signed-off-by: Lv Zheng <lv.zheng@intel.com> --- drivers/acpi/acpica/exconfig.c | 2 ++ drivers/acpi/acpica/nsparse.c | 8 ++++++-- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/drivers/acpi/acpica/exconfig.c b/drivers/acpi/acpica/exconfig.c index a1d177d..21932d6 100644 --- a/drivers/acpi/acpica/exconfig.c +++ b/drivers/acpi/acpica/exconfig.c @@ -108,7 +108,9 @@ acpi_ex_add_table(u32 table_index, /* Add the table to the namespace */ + acpi_ex_exit_interpreter(); status = acpi_ns_load_table(table_index, parent_node); + acpi_ex_enter_interpreter(); if (ACPI_FAILURE(status)) { acpi_ut_remove_reference(obj_desc); *ddb_handle = NULL; diff --git a/drivers/acpi/acpica/nsparse.c b/drivers/acpi/acpica/nsparse.c index f631a47..0d29383 100644 --- a/drivers/acpi/acpica/nsparse.c +++ b/drivers/acpi/acpica/nsparse.c @@ -170,6 +170,8 @@ acpi_ns_parse_table(u32 table_index, struct acpi_namespace_node *start_node) ACPI_FUNCTION_TRACE(ns_parse_table); + acpi_ex_enter_interpreter(); + /* * AML Parse, pass 1 * @@ -185,7 +187,7 @@ acpi_ns_parse_table(u32 table_index, struct acpi_namespace_node *start_node) status = acpi_ns_one_complete_parse(ACPI_IMODE_LOAD_PASS1, table_index, start_node); if (ACPI_FAILURE(status)) { - return_ACPI_STATUS(status); + goto error_exit; } /* @@ -201,8 +203,10 @@ acpi_ns_parse_table(u32 table_index, struct acpi_namespace_node *start_node) status = acpi_ns_one_complete_parse(ACPI_IMODE_LOAD_PASS2, table_index, start_node); if (ACPI_FAILURE(status)) { - return_ACPI_STATUS(status); + goto error_exit; } +error_exit: + acpi_ex_exit_interpreter(); return_ACPI_STATUS(status); } -- 1.7.10 ^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [PATCH v3 1/5] ACPICA: Namespace: Fix a regression that MLC support triggers dead lock in dynamic table loading 2016-06-20 9:07 ` [PATCH v3 1/5] ACPICA: Namespace: Fix a regression that MLC support triggers dead lock in dynamic " Lv Zheng @ 2016-06-20 10:57 ` Mika Westerberg 2016-06-20 23:13 ` Zheng, Lv 0 siblings, 1 reply; 40+ messages in thread From: Mika Westerberg @ 2016-06-20 10:57 UTC (permalink / raw) To: Lv Zheng Cc: Rafael J. Wysocki, Rafael J. Wysocki, Len Brown, Lv Zheng, linux-kernel, linux-acpi On Mon, Jun 20, 2016 at 05:07:22PM +0800, Lv Zheng wrote: > The new MLC approach invokes MLC per-table basis. But the dynamic loading > support of this is incorrect because of the lock order: > acpi_ns_evaluate > acpi_ex_enter_intperter > acpi_ns_load_table (triggered by Load opcode) > acpi_ns_exec_module_code_list > acpi_ex_enter_intperter > The regression is introduced by the following commit: > Commit: 2785ce8d0da1cac9d8f78615e116cf929e9a9123 > ACPICA Commit: 071eff738c59eda1792ac24b3b688b61691d7e7c > Subject: ACPICA: Add per-table execution of module-level code > This patch fixes this regression by unlocking the interpreter lock before > invoking MLC. However the unlocking is done to the acpi_ns_load_table(), in > which, the interpreter lock should be locked by acpi_ns_parse_table() but > wasn't. Reported by Mika Westerberg. Fixed by Lv Zheng. > > Fixes: 2785ce8d0da1 ("ACPICA: Add per-table execution of module-level code") > Reported-by: Mika Westerberg <mika.westerberg@linux.intel.com> > Signed-off-by: Lv Zheng <lv.zheng@intel.com> > --- > drivers/acpi/acpica/exconfig.c | 2 ++ > drivers/acpi/acpica/nsparse.c | 8 ++++++-- > 2 files changed, 8 insertions(+), 2 deletions(-) > > diff --git a/drivers/acpi/acpica/exconfig.c b/drivers/acpi/acpica/exconfig.c > index a1d177d..21932d6 100644 > --- a/drivers/acpi/acpica/exconfig.c > +++ b/drivers/acpi/acpica/exconfig.c > @@ -108,7 +108,9 @@ acpi_ex_add_table(u32 table_index, > > /* Add the table to the namespace */ > > + acpi_ex_exit_interpreter(); > status = acpi_ns_load_table(table_index, parent_node); > + acpi_ex_enter_interpreter(); > if (ACPI_FAILURE(status)) { > acpi_ut_remove_reference(obj_desc); > *ddb_handle = NULL; > diff --git a/drivers/acpi/acpica/nsparse.c b/drivers/acpi/acpica/nsparse.c > index f631a47..0d29383 100644 > --- a/drivers/acpi/acpica/nsparse.c > +++ b/drivers/acpi/acpica/nsparse.c > @@ -170,6 +170,8 @@ acpi_ns_parse_table(u32 table_index, struct acpi_namespace_node *start_node) > > ACPI_FUNCTION_TRACE(ns_parse_table); > > + acpi_ex_enter_interpreter(); I'm getting these: drivers/acpi/acpica/nsparse.c: In function ‘acpi_ns_parse_table’: drivers/acpi/acpica/nsparse.c:173:2: error: implicit declaration of function ‘acpi_ex_enter_interpreter’ [-Werror=implicit-function-declaration] acpi_ex_enter_interpreter(); ^ drivers/acpi/acpica/nsparse.c:210:2: error: implicit declaration of function ‘acpi_ex_exit_interpreter’ [-Werror=implicit-function-declaration] acpi_ex_exit_interpreter(); ^ cc1: some warnings being treated as errors Do I need to have some other patch applied before this? ^ permalink raw reply [flat|nested] 40+ messages in thread
* RE: [PATCH v3 1/5] ACPICA: Namespace: Fix a regression that MLC support triggers dead lock in dynamic table loading 2016-06-20 10:57 ` Mika Westerberg @ 2016-06-20 23:13 ` Zheng, Lv 0 siblings, 0 replies; 40+ messages in thread From: Zheng, Lv @ 2016-06-20 23:13 UTC (permalink / raw) To: Mika Westerberg Cc: Wysocki, Rafael J, Rafael J. Wysocki, Brown, Len, Lv Zheng, linux-kernel, linux-acpi Hi, Mika > From: Mika Westerberg [mailto:mika.westerberg@linux.intel.com] > Subject: Re: [PATCH v3 1/5] ACPICA: Namespace: Fix a regression that MLC > support triggers dead lock in dynamic table loading > > On Mon, Jun 20, 2016 at 05:07:22PM +0800, Lv Zheng wrote: > > The new MLC approach invokes MLC per-table basis. But the dynamic > loading > > support of this is incorrect because of the lock order: > > acpi_ns_evaluate > > acpi_ex_enter_intperter > > acpi_ns_load_table (triggered by Load opcode) > > acpi_ns_exec_module_code_list > > acpi_ex_enter_intperter > > The regression is introduced by the following commit: > > Commit: 2785ce8d0da1cac9d8f78615e116cf929e9a9123 > > ACPICA Commit: 071eff738c59eda1792ac24b3b688b61691d7e7c > > Subject: ACPICA: Add per-table execution of module-level code > > This patch fixes this regression by unlocking the interpreter lock before > > invoking MLC. However the unlocking is done to the > acpi_ns_load_table(), in > > which, the interpreter lock should be locked by acpi_ns_parse_table() > but > > wasn't. Reported by Mika Westerberg. Fixed by Lv Zheng. > > > > Fixes: 2785ce8d0da1 ("ACPICA: Add per-table execution of module-level > code") > > Reported-by: Mika Westerberg <mika.westerberg@linux.intel.com> > > Signed-off-by: Lv Zheng <lv.zheng@intel.com> > > --- > > drivers/acpi/acpica/exconfig.c | 2 ++ > > drivers/acpi/acpica/nsparse.c | 8 ++++++-- > > 2 files changed, 8 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/acpi/acpica/exconfig.c > b/drivers/acpi/acpica/exconfig.c > > index a1d177d..21932d6 100644 > > --- a/drivers/acpi/acpica/exconfig.c > > +++ b/drivers/acpi/acpica/exconfig.c > > @@ -108,7 +108,9 @@ acpi_ex_add_table(u32 table_index, > > > > /* Add the table to the namespace */ > > > > + acpi_ex_exit_interpreter(); > > status = acpi_ns_load_table(table_index, parent_node); > > + acpi_ex_enter_interpreter(); > > if (ACPI_FAILURE(status)) { > > acpi_ut_remove_reference(obj_desc); > > *ddb_handle = NULL; > > diff --git a/drivers/acpi/acpica/nsparse.c b/drivers/acpi/acpica/nsparse.c > > index f631a47..0d29383 100644 > > --- a/drivers/acpi/acpica/nsparse.c > > +++ b/drivers/acpi/acpica/nsparse.c > > @@ -170,6 +170,8 @@ acpi_ns_parse_table(u32 table_index, struct > acpi_namespace_node *start_node) > > > > ACPI_FUNCTION_TRACE(ns_parse_table); > > > > + acpi_ex_enter_interpreter(); > > I'm getting these: > > drivers/acpi/acpica/nsparse.c: In function ‘acpi_ns_parse_table’: > drivers/acpi/acpica/nsparse.c:173:2: error: implicit declaration of function > ‘acpi_ex_enter_interpreter’ [-Werror=implicit-function-declaration] > acpi_ex_enter_interpreter(); > ^ > drivers/acpi/acpica/nsparse.c:210:2: error: implicit declaration of function > ‘acpi_ex_exit_interpreter’ [-Werror=implicit-function-declaration] > acpi_ex_exit_interpreter(); > ^ > cc1: some warnings being treated as errors > > Do I need to have some other patch applied before this? [Lv Zheng] The acinterp.h inclusion is in PATCH 03. I'll re-send the series with this corrected. Thanks and best regards -Lv ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v3 2/5] ACPICA: Dispatcher: Fix an issue that the opregions created by the linked MLC were not tracked 2016-06-20 9:07 ` [PATCH v3 0/5] " Lv Zheng 2016-06-20 9:07 ` [PATCH v3 1/5] ACPICA: Namespace: Fix a regression that MLC support triggers dead lock in dynamic " Lv Zheng @ 2016-06-20 9:07 ` Lv Zheng 2016-06-20 9:07 ` [PATCH v3 3/5] ACPICA: ACPI 2.0, Interpreter: Fix MLC issues by switching to new TermList grammar for table loading Lv Zheng ` (2 subsequent siblings) 4 siblings, 0 replies; 40+ messages in thread From: Lv Zheng @ 2016-06-20 9:07 UTC (permalink / raw) To: Rafael J. Wysocki, Rafael J. Wysocki, Len Brown Cc: Lv Zheng, Lv Zheng, linux-kernel, linux-acpi Operation regions created by MLC were not tracked by acpi_check_address_range(), this patch fixes this issue. ACPICA BZ 1279. Fixed by Lv Zheng. Link: https://bugs.acpica.org/show_bug.cgi?id=1279 Signed-off-by: Lv Zheng <lv.zheng@intel.com> --- drivers/acpi/acpica/dsopcode.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/drivers/acpi/acpica/dsopcode.c b/drivers/acpi/acpica/dsopcode.c index 4cc9d98..96f2eef 100644 --- a/drivers/acpi/acpica/dsopcode.c +++ b/drivers/acpi/acpica/dsopcode.c @@ -455,6 +455,12 @@ acpi_ds_eval_region_operands(struct acpi_walk_state *walk_state, /* Now the address and length are valid for this opregion */ obj_desc->region.flags |= AOPOBJ_DATA_VALID; + if (walk_state->parse_flags & ACPI_PARSE_MODULE_LEVEL) { + status = acpi_ut_add_address_range(obj_desc->region.space_id, + obj_desc->region.address, + obj_desc->region.length, + node); + } return_ACPI_STATUS(status); } -- 1.7.10 ^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH v3 3/5] ACPICA: ACPI 2.0, Interpreter: Fix MLC issues by switching to new TermList grammar for table loading 2016-06-20 9:07 ` [PATCH v3 0/5] " Lv Zheng 2016-06-20 9:07 ` [PATCH v3 1/5] ACPICA: Namespace: Fix a regression that MLC support triggers dead lock in dynamic " Lv Zheng 2016-06-20 9:07 ` [PATCH v3 2/5] ACPICA: Dispatcher: Fix an issue that the opregions created by the linked MLC were not tracked Lv Zheng @ 2016-06-20 9:07 ` Lv Zheng 2016-06-20 9:07 ` [PATCH v3 4/5] ACPI 2.0 / AML: Enable correct ACPI subsystem initialization order for new table loading mode Lv Zheng 2016-06-20 9:07 ` [PATCH v3 5/5] ACPI 2.0 / AML: Fix module level execution by correctly parsing table as TermList Lv Zheng 4 siblings, 0 replies; 40+ messages in thread From: Lv Zheng @ 2016-06-20 9:07 UTC (permalink / raw) To: Rafael J. Wysocki, Rafael J. Wysocki, Len Brown Cc: Lv Zheng, Lv Zheng, linux-kernel, linux-acpi The MLC (Module Level Code) is an ACPICA terminology describing the AML code out of any control method, its support is the main contention of the interpreter behavior during the table loading. The original implementation of MLC in ACPICA had several issues: 1. Out of any control method, besides of the object creating opcodes, only the code blocks wrapped by "If/Else/While" opcodes were supported. 2. The supported MLC code blocks were executed after loading the table rather than being executed right in place. ============================================================ The demo of this order issue is as follows: Name (OBJ1, 1) If (CND1 == 1) { Name (OBJ2, 2) } Name (OBJ3, 3) The original MLC support created OBJ2 after OBJ3's creation. ============================================================ Other than these limitations, MLC support in ACPICA looks correct. And supporting this should be easy/natural for ACPICA, but enabling of this was blocked by some ACPICA internal and OSPM specific initialization order issues we've fixed recently. The wrong support started from the following false bug fixing commit: Commit: 80d7951177315f70b5ffd8663985fbf725d07799 Subject: Add support for module-level executable AML code. We can confirm Windows interpreter behavior via reverse engineering means. It can be proven that not only If/Else/While wrapped code blocks, all opcodes can be executed at the module level, including operation region accesses. And it can be proven that the MLC should be executed right in place, not in such a deferred way executed after loading the table. And the above facts indeed reflect the spec words around ACPI definition block tables (DSDT/SSDT/...), the entire table and the Scope object is defined by the AML specification in BNF style as: AMLCode := DefBlockHeader TermList DefScope := ScopeOp PkgLength NameString TermList The bodies of the scope opening terms (AMLCode/Scope) are all TermList, thus the table loading should be no difference than the control method evaluations as the body of the Method is also defined by the AML specification as TermList: DefMethod := MethodOp PkgLength NameString MethodFlags TermList The only difference is: after evaluating control method, created named objects may be freed due to no reference, while named objects created by the table loading should only be freed after unloading the table. So this patch follows the spec and the de-facto standard behavior, enables the new grammar (TermList) for the table loading. By doing so, beyond the fixes to the above issues, we can see additional differences comparing to the old grammar based table loading: 1. Originally, beyond the scope opening terms (AMLCode/Scope), If/Else/While wrapped code blocks under the scope creating terms (Device/PowerResource/Processor/ThermalZone) are also supported as deferred MLC, which violates the spec defined grammar where ObjectList is enforced. With MLC support improved as non-deferred, the interpreter parses such scope creating terms as TermList rather ObjectList like the scope opening terms. After probing the Windows behavior and proving that it also parses these terms as TermList, we submitted an ECR (Engineering Change Request) to the ASWG (ACPI Specification Working Group) to clarify this. The ECR is titled as "ASL Grammar Clarification for Executable AML Opcodes" and has been accepted by the ASWG. The new grammar will appear in ACPI specification 6.2. 2. Originally, Buffer/Package/OperationRegion/CreateXXXField/BankField arguments are evaluated in a deferred way after loading the table. With MLC support improved, they are also parsed right in place during the table loading. This is also Windows compliant and the only difference is the removal of the debugging messages implemented before acpi_ds_execute_arguments(), see Link 1 for the details. A previous commit should have ensured that acpi_check_address_range() won't regress. Note that enabling this feature may cause regressions due to long term Linux ACPI support on top of the wrong grammar. So this patch also prepares a global option to be used to roll back to the old grammar during the period between a regression is reported and the regression is root-cause-fixed. ACPICA BZ 963, fixed by Lv Zheng. Link 1: https://bugzilla.kernel.org/show_bug.cgi?id=112911 Link 2: https://bugs.acpica.org/show_bug.cgi?id=963 Tested-by: Chris Bainbridge <chris.bainbridge@gmail.com> Signed-off-by: Lv Zheng <lv.zheng@intel.com> --- drivers/acpi/acpica/acnamesp.h | 3 + drivers/acpi/acpica/acparser.h | 2 + drivers/acpi/acpica/evrgnini.c | 3 +- drivers/acpi/acpica/exconfig.c | 3 +- drivers/acpi/acpica/nsload.c | 3 +- drivers/acpi/acpica/nsparse.c | 167 ++++++++++++++++++++++++++++++++-------- drivers/acpi/acpica/psparse.c | 4 +- drivers/acpi/acpica/psxface.c | 71 +++++++++++++++++ drivers/acpi/acpica/tbxfload.c | 3 +- drivers/acpi/acpica/utxfinit.c | 3 +- include/acpi/acpixf.h | 6 ++ 11 files changed, 230 insertions(+), 38 deletions(-) diff --git a/drivers/acpi/acpica/acnamesp.h b/drivers/acpi/acpica/acnamesp.h index f33a4ba..829672a 100644 --- a/drivers/acpi/acpica/acnamesp.h +++ b/drivers/acpi/acpica/acnamesp.h @@ -130,6 +130,9 @@ acpi_status acpi_ns_parse_table(u32 table_index, struct acpi_namespace_node *start_node); acpi_status +acpi_ns_execute_table(u32 table_index, struct acpi_namespace_node *start_node); + +acpi_status acpi_ns_one_complete_parse(u32 pass_number, u32 table_index, struct acpi_namespace_node *start_node); diff --git a/drivers/acpi/acpica/acparser.h b/drivers/acpi/acpica/acparser.h index fc30577..939d411 100644 --- a/drivers/acpi/acpica/acparser.h +++ b/drivers/acpi/acpica/acparser.h @@ -78,6 +78,8 @@ extern const u8 acpi_gbl_long_op_index[]; */ acpi_status acpi_ps_execute_method(struct acpi_evaluate_info *info); +acpi_status acpi_ps_execute_table(struct acpi_evaluate_info *info); + /* * psargs - Parse AML opcode arguments */ diff --git a/drivers/acpi/acpica/evrgnini.c b/drivers/acpi/acpica/evrgnini.c index b6ea9c0..3843f1f 100644 --- a/drivers/acpi/acpica/evrgnini.c +++ b/drivers/acpi/acpica/evrgnini.c @@ -553,7 +553,8 @@ acpi_ev_initialize_region(union acpi_operand_object *region_obj, * * See acpi_ns_exec_module_code */ - if (obj_desc->method. + if (!acpi_gbl_parse_table_as_term_list && + obj_desc->method. info_flags & ACPI_METHOD_MODULE_LEVEL) { handler_obj = obj_desc->method.dispatch.handler; diff --git a/drivers/acpi/acpica/exconfig.c b/drivers/acpi/acpica/exconfig.c index 21932d6..75679bc 100644 --- a/drivers/acpi/acpica/exconfig.c +++ b/drivers/acpi/acpica/exconfig.c @@ -120,7 +120,8 @@ acpi_ex_add_table(u32 table_index, /* Execute any module-level code that was found in the table */ acpi_ex_exit_interpreter(); - if (acpi_gbl_group_module_level_code) { + if (!acpi_gbl_parse_table_as_term_list + && acpi_gbl_group_module_level_code) { acpi_ns_exec_module_code_list(); } acpi_ex_enter_interpreter(); diff --git a/drivers/acpi/acpica/nsload.c b/drivers/acpi/acpica/nsload.c index b5e2b0a..2daa9a09 100644 --- a/drivers/acpi/acpica/nsload.c +++ b/drivers/acpi/acpica/nsload.c @@ -162,7 +162,8 @@ unlock: * other ACPI implementations. Optionally, the execution can be deferred * until later, see acpi_initialize_objects. */ - if (!acpi_gbl_group_module_level_code) { + if (!acpi_gbl_parse_table_as_term_list + && !acpi_gbl_group_module_level_code) { acpi_ns_exec_module_code_list(); } diff --git a/drivers/acpi/acpica/nsparse.c b/drivers/acpi/acpica/nsparse.c index 0d29383..f475889 100644 --- a/drivers/acpi/acpica/nsparse.c +++ b/drivers/acpi/acpica/nsparse.c @@ -47,12 +47,105 @@ #include "acparser.h" #include "acdispat.h" #include "actables.h" +#include "acinterp.h" #define _COMPONENT ACPI_NAMESPACE ACPI_MODULE_NAME("nsparse") /******************************************************************************* * + * FUNCTION: ns_execute_table + * + * PARAMETERS: table_desc - An ACPI table descriptor for table to parse + * start_node - Where to enter the table into the namespace + * + * RETURN: Status + * + * DESCRIPTION: Load ACPI/AML table by executing the entire table as a + * term_list. + * + ******************************************************************************/ +acpi_status +acpi_ns_execute_table(u32 table_index, struct acpi_namespace_node *start_node) +{ + acpi_status status; + struct acpi_table_header *table; + acpi_owner_id owner_id; + struct acpi_evaluate_info *info = NULL; + u32 aml_length; + u8 *aml_start; + union acpi_operand_object *method_obj = NULL; + + ACPI_FUNCTION_TRACE(ns_execute_table); + + status = acpi_get_table_by_index(table_index, &table); + if (ACPI_FAILURE(status)) { + return_ACPI_STATUS(status); + } + + /* Table must consist of at least a complete header */ + + if (table->length < sizeof(struct acpi_table_header)) { + return_ACPI_STATUS(AE_BAD_HEADER); + } + + aml_start = (u8 *)table + sizeof(struct acpi_table_header); + aml_length = table->length - sizeof(struct acpi_table_header); + + status = acpi_tb_get_owner_id(table_index, &owner_id); + if (ACPI_FAILURE(status)) { + return_ACPI_STATUS(status); + } + + /* Create, initialize, and link a new temporary method object */ + + method_obj = acpi_ut_create_internal_object(ACPI_TYPE_METHOD); + if (!method_obj) { + return_ACPI_STATUS(AE_NO_MEMORY); + } + + /* Allocate the evaluation information block */ + + info = ACPI_ALLOCATE_ZEROED(sizeof(struct acpi_evaluate_info)); + if (!info) { + status = AE_NO_MEMORY; + goto cleanup; + } + + ACPI_DEBUG_PRINT((ACPI_DB_PARSE, + "Create table code block: %p\n", method_obj)); + + method_obj->method.aml_start = aml_start; + method_obj->method.aml_length = aml_length; + method_obj->method.owner_id = owner_id; + method_obj->method.info_flags |= ACPI_METHOD_MODULE_LEVEL; + + info->pass_number = ACPI_IMODE_EXECUTE; + info->node = start_node; + info->obj_desc = method_obj; + info->node_flags = info->node->flags; + info->full_pathname = acpi_ns_get_normalized_pathname(info->node, TRUE); + if (!info->full_pathname) { + status = AE_NO_MEMORY; + goto cleanup; + } + + (void)acpi_ut_release_mutex(ACPI_MTX_NAMESPACE); + status = acpi_ps_execute_table(info); + (void)acpi_ut_acquire_mutex(ACPI_MTX_NAMESPACE); + +cleanup: + if (info) { + ACPI_FREE(info->full_pathname); + info->full_pathname = NULL; + } + ACPI_FREE(info); + acpi_ut_remove_reference(method_obj); + return_ACPI_STATUS(status); +} + +/******************************************************************************* + * * FUNCTION: ns_one_complete_parse * * PARAMETERS: pass_number - 1 or 2 @@ -63,6 +156,7 @@ ACPI_MODULE_NAME("nsparse") * DESCRIPTION: Perform one complete parse of an ACPI/AML table. * ******************************************************************************/ + acpi_status acpi_ns_one_complete_parse(u32 pass_number, u32 table_index, @@ -172,38 +266,47 @@ acpi_ns_parse_table(u32 table_index, struct acpi_namespace_node *start_node) acpi_ex_enter_interpreter(); - /* - * AML Parse, pass 1 - * - * In this pass, we load most of the namespace. Control methods - * are not parsed until later. A parse tree is not created. Instead, - * each Parser Op subtree is deleted when it is finished. This saves - * a great deal of memory, and allows a small cache of parse objects - * to service the entire parse. The second pass of the parse then - * performs another complete parse of the AML. - */ - ACPI_DEBUG_PRINT((ACPI_DB_PARSE, "**** Start pass 1\n")); - - status = acpi_ns_one_complete_parse(ACPI_IMODE_LOAD_PASS1, - table_index, start_node); - if (ACPI_FAILURE(status)) { - goto error_exit; - } - - /* - * AML Parse, pass 2 - * - * In this pass, we resolve forward references and other things - * that could not be completed during the first pass. - * Another complete parse of the AML is performed, but the - * overhead of this is compensated for by the fact that the - * parse objects are all cached. - */ - ACPI_DEBUG_PRINT((ACPI_DB_PARSE, "**** Start pass 2\n")); - status = acpi_ns_one_complete_parse(ACPI_IMODE_LOAD_PASS2, - table_index, start_node); - if (ACPI_FAILURE(status)) { - goto error_exit; + if (acpi_gbl_parse_table_as_term_list) { + ACPI_DEBUG_PRINT((ACPI_DB_PARSE, "**** Start load pass\n")); + + status = acpi_ns_execute_table(table_index, start_node); + if (ACPI_FAILURE(status)) { + goto error_exit; + } + } else { + /* + * AML Parse, pass 1 + * + * In this pass, we load most of the namespace. Control methods + * are not parsed until later. A parse tree is not created. + * Instead, each Parser Op subtree is deleted when it is finished. + * This saves a great deal of memory, and allows a small cache of + * parse objects to service the entire parse. The second pass of + * the parse then performs another complete parse of the AML. + */ + ACPI_DEBUG_PRINT((ACPI_DB_PARSE, "**** Start pass 1\n")); + + status = acpi_ns_one_complete_parse(ACPI_IMODE_LOAD_PASS1, + table_index, start_node); + if (ACPI_FAILURE(status)) { + goto error_exit; + } + + /* + * AML Parse, pass 2 + * + * In this pass, we resolve forward references and other things + * that could not be completed during the first pass. + * Another complete parse of the AML is performed, but the + * overhead of this is compensated for by the fact that the + * parse objects are all cached. + */ + ACPI_DEBUG_PRINT((ACPI_DB_PARSE, "**** Start pass 2\n")); + status = acpi_ns_one_complete_parse(ACPI_IMODE_LOAD_PASS2, + table_index, start_node); + if (ACPI_FAILURE(status)) { + goto error_exit; + } } error_exit: diff --git a/drivers/acpi/acpica/psparse.c b/drivers/acpi/acpica/psparse.c index 0a23897..3aa9162 100644 --- a/drivers/acpi/acpica/psparse.c +++ b/drivers/acpi/acpica/psparse.c @@ -571,7 +571,9 @@ acpi_status acpi_ps_parse_aml(struct acpi_walk_state *walk_state) * cleanup to do */ if (((walk_state->parse_flags & ACPI_PARSE_MODE_MASK) == - ACPI_PARSE_EXECUTE) || (ACPI_FAILURE(status))) { + ACPI_PARSE_EXECUTE && + !(walk_state->parse_flags & ACPI_PARSE_MODULE_LEVEL)) || + (ACPI_FAILURE(status))) { acpi_ds_terminate_control_method(walk_state-> method_desc, walk_state); diff --git a/drivers/acpi/acpica/psxface.c b/drivers/acpi/acpica/psxface.c index cf30cd82..fccc0e5 100644 --- a/drivers/acpi/acpica/psxface.c +++ b/drivers/acpi/acpica/psxface.c @@ -252,6 +252,77 @@ cleanup: /******************************************************************************* * + * FUNCTION: acpi_ps_execute_table + * + * PARAMETERS: info - Method info block, contains: + * node - Node to where the is entered into the + * namespace + * obj_desc - Pseudo method object describing the AML + * code of the entire table + * pass_number - Parse or execute pass + * + * RETURN: Status + * + * DESCRIPTION: Execute a table + * + ******************************************************************************/ + +acpi_status acpi_ps_execute_table(struct acpi_evaluate_info *info) +{ + acpi_status status; + union acpi_parse_object *op = NULL; + struct acpi_walk_state *walk_state = NULL; + + ACPI_FUNCTION_TRACE(ps_execute_table); + + /* Create and init a Root Node */ + + op = acpi_ps_create_scope_op(info->obj_desc->method.aml_start); + if (!op) { + status = AE_NO_MEMORY; + goto cleanup; + } + + /* Create and initialize a new walk state */ + + walk_state = + acpi_ds_create_walk_state(info->obj_desc->method.owner_id, NULL, + NULL, NULL); + if (!walk_state) { + status = AE_NO_MEMORY; + goto cleanup; + } + + status = acpi_ds_init_aml_walk(walk_state, op, info->node, + info->obj_desc->method.aml_start, + info->obj_desc->method.aml_length, info, + info->pass_number); + if (ACPI_FAILURE(status)) { + goto cleanup; + } + + if (info->obj_desc->method.info_flags & ACPI_METHOD_MODULE_LEVEL) { + walk_state->parse_flags |= ACPI_PARSE_MODULE_LEVEL; + } + + /* + * Parse the AML, walk_state will be deleted by parse_aml + */ + status = acpi_ps_parse_aml(walk_state); + walk_state = NULL; + +cleanup: + if (op) { + acpi_ps_delete_parse_tree(op); + } + if (walk_state) { + acpi_ds_delete_walk_state(walk_state); + } + return_ACPI_STATUS(status); +} + +/******************************************************************************* + * * FUNCTION: acpi_ps_update_parameter_list * * PARAMETERS: info - See struct acpi_evaluate_info diff --git a/drivers/acpi/acpica/tbxfload.c b/drivers/acpi/acpica/tbxfload.c index ac71abc..2b9e87f 100644 --- a/drivers/acpi/acpica/tbxfload.c +++ b/drivers/acpi/acpica/tbxfload.c @@ -103,7 +103,8 @@ acpi_status __init acpi_load_tables(void) "While loading namespace from ACPI tables")); } - if (!acpi_gbl_group_module_level_code) { + if (acpi_gbl_parse_table_as_term_list + || !acpi_gbl_group_module_level_code) { /* * Initialize the objects that remain uninitialized. This * runs the executable AML that may be part of the diff --git a/drivers/acpi/acpica/utxfinit.c b/drivers/acpi/acpica/utxfinit.c index 75b5f27..e41424a 100644 --- a/drivers/acpi/acpica/utxfinit.c +++ b/drivers/acpi/acpica/utxfinit.c @@ -265,7 +265,8 @@ acpi_status __init acpi_initialize_objects(u32 flags) * all of the tables have been loaded. It is a legacy option and is * not compatible with other ACPI implementations. See acpi_ns_load_table. */ - if (acpi_gbl_group_module_level_code) { + if (!acpi_gbl_parse_table_as_term_list + && acpi_gbl_group_module_level_code) { acpi_ns_exec_module_code_list(); /* diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h index 4e4c214..22b0397 100644 --- a/include/acpi/acpixf.h +++ b/include/acpi/acpixf.h @@ -195,6 +195,12 @@ ACPI_INIT_GLOBAL(u8, acpi_gbl_do_not_use_xsdt, FALSE); ACPI_INIT_GLOBAL(u8, acpi_gbl_group_module_level_code, FALSE); /* + * Optionally support module level code by parsing the entire table as + * a term_list. Default is FALSE, do not execute entire table. + */ +ACPI_INIT_GLOBAL(u8, acpi_gbl_parse_table_as_term_list, FALSE); + +/* * Optionally use 32-bit FADT addresses if and when there is a conflict * (address mismatch) between the 32-bit and 64-bit versions of the * address. Although ACPICA adheres to the ACPI specification which -- 1.7.10 ^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH v3 4/5] ACPI 2.0 / AML: Enable correct ACPI subsystem initialization order for new table loading mode 2016-06-20 9:07 ` [PATCH v3 0/5] " Lv Zheng ` (2 preceding siblings ...) 2016-06-20 9:07 ` [PATCH v3 3/5] ACPICA: ACPI 2.0, Interpreter: Fix MLC issues by switching to new TermList grammar for table loading Lv Zheng @ 2016-06-20 9:07 ` Lv Zheng 2016-06-20 9:07 ` [PATCH v3 5/5] ACPI 2.0 / AML: Fix module level execution by correctly parsing table as TermList Lv Zheng 4 siblings, 0 replies; 40+ messages in thread From: Lv Zheng @ 2016-06-20 9:07 UTC (permalink / raw) To: Rafael J. Wysocki, Rafael J. Wysocki, Len Brown Cc: Lv Zheng, Lv Zheng, linux-kernel, linux-acpi This patch enables the following initialization order for the new table loading mode (which is enabled by setting acpi_gbl_parse_table_as_term_list to TRUE): 1. Install default region handlers (SystemMemory, SystemIo, PciConfig, EmbeddedControl via ECDT) without evaluating _REG; 2. Load the table and execute the module level AML opcodes instantly. Signed-off-by: Lv Zheng <lv.zheng@intel.com> --- drivers/acpi/bus.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c index 262ca31..4582db3 100644 --- a/drivers/acpi/bus.c +++ b/drivers/acpi/bus.c @@ -925,7 +925,8 @@ void __init acpi_early_init(void) goto error0; } - if (acpi_gbl_group_module_level_code) { + if (!acpi_gbl_parse_table_as_term_list && + acpi_gbl_group_module_level_code) { status = acpi_load_tables(); if (ACPI_FAILURE(status)) { printk(KERN_ERR PREFIX @@ -1008,7 +1009,8 @@ static int __init acpi_bus_init(void) status = acpi_ec_ecdt_probe(); /* Ignore result. Not having an ECDT is not fatal. */ - if (!acpi_gbl_group_module_level_code) { + if (acpi_gbl_parse_table_as_term_list || + !acpi_gbl_group_module_level_code) { status = acpi_load_tables(); if (ACPI_FAILURE(status)) { printk(KERN_ERR PREFIX -- 1.7.10 ^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH v3 5/5] ACPI 2.0 / AML: Fix module level execution by correctly parsing table as TermList 2016-06-20 9:07 ` [PATCH v3 0/5] " Lv Zheng ` (3 preceding siblings ...) 2016-06-20 9:07 ` [PATCH v3 4/5] ACPI 2.0 / AML: Enable correct ACPI subsystem initialization order for new table loading mode Lv Zheng @ 2016-06-20 9:07 ` Lv Zheng 4 siblings, 0 replies; 40+ messages in thread From: Lv Zheng @ 2016-06-20 9:07 UTC (permalink / raw) To: Rafael J. Wysocki, Rafael J. Wysocki, Len Brown Cc: Lv Zheng, Lv Zheng, linux-kernel, linux-acpi This experiment follows de-facto standard behavior, parsing entire table as a single TermList, so that all module level executions are possible during the table loading. If regressions are found against the enabling of this experimental fix, this patch is the only one that should get bisected out. Please report the regressions to the kernel bugzilla for further root causing. Signed-off-by: Lv Zheng <lv.zheng@intel.com> --- include/acpi/acpixf.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h index 22b0397..e21bdd1 100644 --- a/include/acpi/acpixf.h +++ b/include/acpi/acpixf.h @@ -196,9 +196,9 @@ ACPI_INIT_GLOBAL(u8, acpi_gbl_group_module_level_code, FALSE); /* * Optionally support module level code by parsing the entire table as - * a term_list. Default is FALSE, do not execute entire table. + * a term_list. Default is TRUE, do execute entire table. */ -ACPI_INIT_GLOBAL(u8, acpi_gbl_parse_table_as_term_list, FALSE); +ACPI_INIT_GLOBAL(u8, acpi_gbl_parse_table_as_term_list, TRUE); /* * Optionally use 32-bit FADT addresses if and when there is a conflict -- 1.7.10 ^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH v4 0/5] ACPI 2.0: Enable TermList interpretion for table loading [not found] <0e65135af51d94db0410c7059f3bc3a2300fc3b5> 2016-05-13 5:35 ` [PATCH v2 0/4] ACPI 2.0: Enable TermList interpretion for table loading Lv Zheng 2016-06-20 9:07 ` [PATCH v3 0/5] " Lv Zheng @ 2016-06-21 4:34 ` Lv Zheng 2016-06-21 4:34 ` [PATCH v4 1/5] ACPICA: Namespace: Fix a regression that MLC support triggers dead lock in dynamic " Lv Zheng ` (4 more replies) 2016-09-23 3:26 ` [PATCH v5 0/5] ACPI 2.0: Stop defer-executing module level code Lv Zheng 3 siblings, 5 replies; 40+ messages in thread From: Lv Zheng @ 2016-06-21 4:34 UTC (permalink / raw) To: Rafael J. Wysocki, Rafael J. Wysocki, Len Brown Cc: Lv Zheng, Lv Zheng, linux-kernel, linux-acpi MLC (module level code) is an ACPICA terminology describing the AML code out of any control method, currently only Type1Opcode (If/Else/While) wrapped MLC code blocks are executed by the AML interpreter after the table loading. But the issue which is fixed by this patchset is: Not only Type1Opcode, but also Type2Opcode will be executed as MLC and MLC is not executed after loading the table, but is executed right in place. The following AML code is assembled into a static loading SSDT, and used as an instrumentation to pry into the de-facto standard AML interpreter behaviors: Name (ECOK, Zero) Scope (\) { DBUG ("TermList 1") If (LEqual (ECOK, Zero)) { DBUG ("TermList 2") Device (MDEV) { DEBUG (TermList 3") If (CondRefOf (MDEV)) { DBUG ("MDEV exists") } If (CondRefOf (MDEV._STA)) { DBUG ("MDEV._STA exists") } If (CondRefOf (\_SB.PCI0.EC)) { DBUG ("\\_SB.PCI0.EC exists") } Name (_HID, EisaId ("PNP9999")) Method (_STA, 0, Serialized) { DEBUG ("\\_SB.MDEV._STA") Return (0x0F) } } DBUG ("TermList 4") } Method (_INI, 0, Serialized) { DBUG ("\\_SB._INI") } } Scope (_SB.PCI0) { Device (EC) { ... } } The DBUG function is a function to write the debugging messages into a SystemIo debug port. Running Windows with the BIOS providing this SSDT via RSDT, the following messages are obtained from the debug port: TermList 1 TermList 2 TermList 3 \_SB.MDEV exists TermList 4 \_SB._INI ... This test reveals the de-facto grammar for the AMLCode to us: 1. During the table loading, MLC will be executed by the interpreter, this is partially supported by the current ACPICA; 2. For SystemIo, not only after the _REG(1, 1) is evaluated (current ACPICA interpreter limitation), but when the table is being loaded, the SystemIo (the debugging port) is accessible, this is recently fixed in the upstream, now all early operation regions are accessible during the table loading; 3. Not only Type1Opcode, but also Type2Opcode will be executed as MLC and MLC is not executed after loading the table, but is executed right in place, the Linux upstream is not compliant to this behavior. The last compliance issue has already been clarified in ACPI 2.0 specification, so the compliance issue is not that Linux is not compliant to the de-facto standard OS, but that Linux is not compliant to ACPI 2.0. Definition block tables in fact is defined by the spec as TermList, which has no difference than the control methods, thus the interpretion of the table should be no difference that the control method evaluation: AMLCode := DefBlockHeader TermList DefMethod := MethodOp PkgLength NameString MethodFlags TermList Why ACPICA interpreter is acting so differently from this definition? This is because, there are many software entropies preventing this from being enabled, such entropies need to be cleaned up first in order not to trigger regressions for specific platforms. These entropies include: 1. ECDT support is broken. In fact, the original EC driver was correct, but devlopers started to use the namespace EC instead of ECDT just because several broken ECDT tables were reported on the bugzilla. They trusted the namespace EC settings rather than the ECDT ones, this led to the evaluation of _REG/_GPE/_CRS and namespace walk before executing the module level AML opcodes. And the fixes in fact finally disable early EC usages (used during table loading and early device enumeration processes). 2. _REG evaluations are wrong. ACPICA provides APIs for OSPMs to register operation region handlers. But for the early operation region accesses, ACPI spec declares that the evaluations of _REG are not required, but the ACPICA APIs do not avoid running _REG to meet this early requirements. Code to fix this is partially upstreamed during previous ACPICA release cycle. 3. _REG associations are wrong. ACPICA associates _REG control method to all operation region objects before executing the _REG control method. This can happen even when a control method is evaluated and operation regions defined in the method is initialized (acpi_ev_initialize_region). As a part of the ACPICA internal _REG evaluation state machine, it requires the namespace walk, and all namespace walk should be ensured to happen only "AFTER THE NAMESPACE IS INITIALIZED". But when this logic happens during the table loading, it may fail in finding the _REG method since the _REG method may not be created by the interpreter just because _REG is defined after the operation region object's declaration. 4. _REG(CONNECT)/_REG(DISCONNECT) executions are not balanced, this can lead to wrong table loading/unloading results. Since _REG evaluations require the releasing of all interpreter/namespace locks in order to allow another evaluation to happen, and ACPICA operand object destruction code can be invoked from different locking environment, this becomes difficult for the developers to provide one single function to make _REG(CONNECT)/_REG(DISCONNECT) balanced. 5. \_SB._INI is not the first control method evaluated by the interpreter. Many platforms put initialization code in \_SB._INI in order to have named objects initialized very early during the device enumeration process. Without this order strictly ensured, early operation region access enabling could break these platforms. 6. Linux initialization order is wrong, it is now: a. load namespace without executing root scope If/Else/While module level code blocks; b. probe ECDT and instal EmbeddedControl operation region handler with _REG evaluated; c. install SystemMemory, SystemIo, PciConfig operation region handlers without evaluating _REG; d. run _REG for SystemMemory, SystemIo, PciConfig operation regions; e. execute root scope If/Else/While module level code blocks; f. enable GPE and namespace EC. While the correct order should be: a. probe ECDT and install EmbeddedControl operation region handler without evaluating _REG; b. install SystemMemory, SystemIo, PciConfig operation region handlers without evaluating _REG; c. load namespace, in the meanshile, execute all module level AML opcodes; d. run _REG for SystemMemory, SystemIo, PciConfig operation regions; e. enable GPE and namespace EC which results in _REG evaluation for EC. Until now we've upstreamed most of the entropy fixes into the Linux kernel, tested the grammar switch in the ACPICA upstream using ASLTS and no significant regressions can be seen while we need more tests before it is merged: https://github.com/acpica/acpica/pull/134 This is the ASLTS running result after applying the grammar switch, the test cases include new "module" case for which ACPICA interpreter cannot pass without this grammar switch applied: ============================================================ Test cases specified for running: arithmetic bfield constant control descriptor logic manipulation name reference region synchronization table misc provoke oarg oconst olocal onamedloc onamedglob opackageel oreftonamed oreftopackageel oreturn rstore roptional rcopyobject rindecrement rexplicitconv badasl namespace exc exc_ref exc_operand2 exc_result2 exc_tbl mt_mutex extra extra_aslts bdemo bdemof condbranches TOTAL: (32-bit norm mode) PASS : 0 FAIL : 0 BLOCKED : 0 SKIPPED : 0 Tests : 0 Test Cases : 40 (of 47) Test Collections : 7 (of 8) Outstanding allocations after execution : 0 Outstanding allocations (ACPI Error) : 0 Large Reference Count (ACPI Error) : 0 Memory consumption total : 0 Kb TOTAL: (64-bit norm mode) PASS : 0 FAIL : 0 BLOCKED : 0 SKIPPED : 0 Tests : 0 Test Cases : 40 (of 47) Test Collections : 7 (of 8) Outstanding allocations after execution : 0 Outstanding allocations (ACPI Error) : 0 Large Reference Count (ACPI Error) : 0 Memory consumption total : 0 Kb TOTAL: (32-bit slack mode) PASS : 0 FAIL : 0 BLOCKED : 0 SKIPPED : 0 Tests : 0 Test Cases : 40 (of 47) Test Collections : 7 (of 8) Outstanding allocations after execution : 0 Outstanding allocations (ACPI Error) : 0 Large Reference Count (ACPI Error) : 0 Memory consumption total : 0 Kb TOTAL: (64-bit slack mode) PASS : 0 FAIL : 0 BLOCKED : 0 SKIPPED : 0 Tests : 0 Test Cases : 40 (of 47) Test Collections : 7 (of 8) Outstanding allocations after execution : 0 Outstanding allocations (ACPI Error) : 0 Large Reference Count (ACPI Error) : 0 Memory consumption total : 0 Kb ============================================================ Since we need more tests from the real users, we could make the grammar switch released from the Linux upstream. It's safe to do so because we have implemented regression protection (acpi_gbl_parse_table_as_term_list) in the fixes. The earlier the fix is tested by more real users, the better quality can be achieved by knowing the unknown cases (if any). Lv Zheng (5): ACPICA: Namespace: Fix a regression that MLC support triggers dead lock in dynamic table loading ACPICA: Dispatcher: Fix an issue that the opregions created by the linked MLC were not tracked ACPICA: ACPI 2.0, Interpreter: Fix MLC issues by switching to new TermList grammar for table loading ACPI 2.0 / AML: Enable correct ACPI subsystem initialization order for new table loading mode ACPI 2.0 / AML: Fix module level execution by correctly parsing table as TermList drivers/acpi/acpica/acnamesp.h | 3 + drivers/acpi/acpica/acparser.h | 2 + drivers/acpi/acpica/dsopcode.c | 6 ++ drivers/acpi/acpica/evrgnini.c | 3 +- drivers/acpi/acpica/exconfig.c | 5 +- drivers/acpi/acpica/nsload.c | 3 +- drivers/acpi/acpica/nsparse.c | 169 ++++++++++++++++++++++++++++++++-------- drivers/acpi/acpica/psparse.c | 4 +- drivers/acpi/acpica/psxface.c | 71 +++++++++++++++++ drivers/acpi/acpica/tbxfload.c | 3 +- drivers/acpi/acpica/utxfinit.c | 3 +- drivers/acpi/bus.c | 6 +- include/acpi/acpixf.h | 6 ++ 13 files changed, 245 insertions(+), 39 deletions(-) -- 1.7.10 ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v4 1/5] ACPICA: Namespace: Fix a regression that MLC support triggers dead lock in dynamic table loading 2016-06-21 4:34 ` [PATCH v4 0/5] ACPI 2.0: Enable TermList interpretion for table loading Lv Zheng @ 2016-06-21 4:34 ` Lv Zheng 2016-06-21 7:58 ` Mika Westerberg 2016-06-21 4:34 ` [PATCH v4 2/5] ACPICA: Dispatcher: Fix an issue that the opregions created by the linked MLC were not tracked Lv Zheng ` (3 subsequent siblings) 4 siblings, 1 reply; 40+ messages in thread From: Lv Zheng @ 2016-06-21 4:34 UTC (permalink / raw) To: Rafael J. Wysocki, Rafael J. Wysocki, Len Brown Cc: Lv Zheng, Lv Zheng, linux-kernel, linux-acpi, Mika Westerberg The new MLC approach invokes MLC per-table basis. But the dynamic loading support of this is incorrect because of the lock order: acpi_ns_evaluate acpi_ex_enter_intperter acpi_ns_load_table (triggered by Load opcode) acpi_ns_exec_module_code_list acpi_ex_enter_intperter The regression is introduced by the following commit: Commit: 2785ce8d0da1cac9d8f78615e116cf929e9a9123 ACPICA Commit: 071eff738c59eda1792ac24b3b688b61691d7e7c Subject: ACPICA: Add per-table execution of module-level code This patch fixes this regression by unlocking the interpreter lock before invoking MLC. However the unlocking is done to the acpi_ns_load_table(), in which, the interpreter lock should be locked by acpi_ns_parse_table() but wasn't. Reported by Mika Westerberg. Fixed by Lv Zheng. Fixes: 2785ce8d0da1 ("ACPICA: Add per-table execution of module-level code") Cc: Mika Westerberg <mika.westerberg@linux.intel.com> Reported-by: Mika Westerberg <mika.westerberg@linux.intel.com> Signed-off-by: Lv Zheng <lv.zheng@intel.com> --- drivers/acpi/acpica/exconfig.c | 2 ++ drivers/acpi/acpica/nsparse.c | 9 +++++++-- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/drivers/acpi/acpica/exconfig.c b/drivers/acpi/acpica/exconfig.c index a1d177d..21932d6 100644 --- a/drivers/acpi/acpica/exconfig.c +++ b/drivers/acpi/acpica/exconfig.c @@ -108,7 +108,9 @@ acpi_ex_add_table(u32 table_index, /* Add the table to the namespace */ + acpi_ex_exit_interpreter(); status = acpi_ns_load_table(table_index, parent_node); + acpi_ex_enter_interpreter(); if (ACPI_FAILURE(status)) { acpi_ut_remove_reference(obj_desc); *ddb_handle = NULL; diff --git a/drivers/acpi/acpica/nsparse.c b/drivers/acpi/acpica/nsparse.c index f631a47..1783cd7 100644 --- a/drivers/acpi/acpica/nsparse.c +++ b/drivers/acpi/acpica/nsparse.c @@ -47,6 +47,7 @@ #include "acparser.h" #include "acdispat.h" #include "actables.h" +#include "acinterp.h" #define _COMPONENT ACPI_NAMESPACE ACPI_MODULE_NAME("nsparse") @@ -170,6 +171,8 @@ acpi_ns_parse_table(u32 table_index, struct acpi_namespace_node *start_node) ACPI_FUNCTION_TRACE(ns_parse_table); + acpi_ex_enter_interpreter(); + /* * AML Parse, pass 1 * @@ -185,7 +188,7 @@ acpi_ns_parse_table(u32 table_index, struct acpi_namespace_node *start_node) status = acpi_ns_one_complete_parse(ACPI_IMODE_LOAD_PASS1, table_index, start_node); if (ACPI_FAILURE(status)) { - return_ACPI_STATUS(status); + goto error_exit; } /* @@ -201,8 +204,10 @@ acpi_ns_parse_table(u32 table_index, struct acpi_namespace_node *start_node) status = acpi_ns_one_complete_parse(ACPI_IMODE_LOAD_PASS2, table_index, start_node); if (ACPI_FAILURE(status)) { - return_ACPI_STATUS(status); + goto error_exit; } +error_exit: + acpi_ex_exit_interpreter(); return_ACPI_STATUS(status); } -- 1.7.10 ^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [PATCH v4 1/5] ACPICA: Namespace: Fix a regression that MLC support triggers dead lock in dynamic table loading 2016-06-21 4:34 ` [PATCH v4 1/5] ACPICA: Namespace: Fix a regression that MLC support triggers dead lock in dynamic " Lv Zheng @ 2016-06-21 7:58 ` Mika Westerberg 2016-06-21 9:55 ` Zheng, Lv 0 siblings, 1 reply; 40+ messages in thread From: Mika Westerberg @ 2016-06-21 7:58 UTC (permalink / raw) To: Lv Zheng Cc: Rafael J. Wysocki, Rafael J. Wysocki, Len Brown, Lv Zheng, linux-kernel, linux-acpi On Tue, Jun 21, 2016 at 12:34:15PM +0800, Lv Zheng wrote: > The new MLC approach invokes MLC per-table basis. But the dynamic loading > support of this is incorrect because of the lock order: > acpi_ns_evaluate > acpi_ex_enter_intperter > acpi_ns_load_table (triggered by Load opcode) > acpi_ns_exec_module_code_list > acpi_ex_enter_intperter > The regression is introduced by the following commit: > Commit: 2785ce8d0da1cac9d8f78615e116cf929e9a9123 > ACPICA Commit: 071eff738c59eda1792ac24b3b688b61691d7e7c > Subject: ACPICA: Add per-table execution of module-level code > This patch fixes this regression by unlocking the interpreter lock before > invoking MLC. However the unlocking is done to the acpi_ns_load_table(), in > which, the interpreter lock should be locked by acpi_ns_parse_table() but > wasn't. Reported by Mika Westerberg. Fixed by Lv Zheng. > > Fixes: 2785ce8d0da1 ("ACPICA: Add per-table execution of module-level code") > Cc: Mika Westerberg <mika.westerberg@linux.intel.com> > Reported-by: Mika Westerberg <mika.westerberg@linux.intel.com> Now builds fine and fixes the hang, thanks :) Tested-by: Mika Westerberg <mika.westerberg@linux.intel.com> ^ permalink raw reply [flat|nested] 40+ messages in thread
* RE: [PATCH v4 1/5] ACPICA: Namespace: Fix a regression that MLC support triggers dead lock in dynamic table loading 2016-06-21 7:58 ` Mika Westerberg @ 2016-06-21 9:55 ` Zheng, Lv 2016-06-23 0:41 ` Rafael J. Wysocki 0 siblings, 1 reply; 40+ messages in thread From: Zheng, Lv @ 2016-06-21 9:55 UTC (permalink / raw) To: Mika Westerberg Cc: Wysocki, Rafael J, Rafael J. Wysocki, Brown, Len, Lv Zheng, linux-kernel, linux-acpi Hi, Mika > From: Mika Westerberg [mailto:mika.westerberg@linux.intel.com] > Subject: Re: [PATCH v4 1/5] ACPICA: Namespace: Fix a regression that MLC > support triggers dead lock in dynamic table loading > > On Tue, Jun 21, 2016 at 12:34:15PM +0800, Lv Zheng wrote: > > The new MLC approach invokes MLC per-table basis. But the dynamic > loading > > support of this is incorrect because of the lock order: > > acpi_ns_evaluate > > acpi_ex_enter_intperter > > acpi_ns_load_table (triggered by Load opcode) > > acpi_ns_exec_module_code_list > > acpi_ex_enter_intperter > > The regression is introduced by the following commit: > > Commit: 2785ce8d0da1cac9d8f78615e116cf929e9a9123 > > ACPICA Commit: 071eff738c59eda1792ac24b3b688b61691d7e7c > > Subject: ACPICA: Add per-table execution of module-level code > > This patch fixes this regression by unlocking the interpreter lock before > > invoking MLC. However the unlocking is done to the > acpi_ns_load_table(), in > > which, the interpreter lock should be locked by acpi_ns_parse_table() > but > > wasn't. Reported by Mika Westerberg. Fixed by Lv Zheng. > > > > Fixes: 2785ce8d0da1 ("ACPICA: Add per-table execution of module-level > code") > > Cc: Mika Westerberg <mika.westerberg@linux.intel.com> > > Reported-by: Mika Westerberg <mika.westerberg@linux.intel.com> > > Now builds fine and fixes the hang, thanks :) > > Tested-by: Mika Westerberg <mika.westerberg@linux.intel.com> [Lv Zheng] Great! :) I'll leave the SOB correction for Rafael in order not to bother others by re-sending an update. Thanks and best regards -Lv ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v4 1/5] ACPICA: Namespace: Fix a regression that MLC support triggers dead lock in dynamic table loading 2016-06-21 9:55 ` Zheng, Lv @ 2016-06-23 0:41 ` Rafael J. Wysocki 0 siblings, 0 replies; 40+ messages in thread From: Rafael J. Wysocki @ 2016-06-23 0:41 UTC (permalink / raw) To: Zheng, Lv Cc: Mika Westerberg, Wysocki, Rafael J, Brown, Len, Lv Zheng, linux-kernel, linux-acpi On Tuesday, June 21, 2016 09:55:53 AM Zheng, Lv wrote: > Hi, Mika > > > From: Mika Westerberg [mailto:mika.westerberg@linux.intel.com] > > Subject: Re: [PATCH v4 1/5] ACPICA: Namespace: Fix a regression that MLC > > support triggers dead lock in dynamic table loading > > > > On Tue, Jun 21, 2016 at 12:34:15PM +0800, Lv Zheng wrote: > > > The new MLC approach invokes MLC per-table basis. But the dynamic > > loading > > > support of this is incorrect because of the lock order: > > > acpi_ns_evaluate > > > acpi_ex_enter_intperter > > > acpi_ns_load_table (triggered by Load opcode) > > > acpi_ns_exec_module_code_list > > > acpi_ex_enter_intperter > > > The regression is introduced by the following commit: > > > Commit: 2785ce8d0da1cac9d8f78615e116cf929e9a9123 > > > ACPICA Commit: 071eff738c59eda1792ac24b3b688b61691d7e7c > > > Subject: ACPICA: Add per-table execution of module-level code > > > This patch fixes this regression by unlocking the interpreter lock before > > > invoking MLC. However the unlocking is done to the > > acpi_ns_load_table(), in > > > which, the interpreter lock should be locked by acpi_ns_parse_table() > > but > > > wasn't. Reported by Mika Westerberg. Fixed by Lv Zheng. > > > > > > Fixes: 2785ce8d0da1 ("ACPICA: Add per-table execution of module-level > > code") > > > Cc: Mika Westerberg <mika.westerberg@linux.intel.com> > > > Reported-by: Mika Westerberg <mika.westerberg@linux.intel.com> > > > > Now builds fine and fixes the hang, thanks :) > > > > Tested-by: Mika Westerberg <mika.westerberg@linux.intel.com> > [Lv Zheng] > Great! :) > I'll leave the SOB correction for Rafael in order not to bother others by re-sending an update. Applied (with tags), thanks! ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v4 2/5] ACPICA: Dispatcher: Fix an issue that the opregions created by the linked MLC were not tracked 2016-06-21 4:34 ` [PATCH v4 0/5] ACPI 2.0: Enable TermList interpretion for table loading Lv Zheng 2016-06-21 4:34 ` [PATCH v4 1/5] ACPICA: Namespace: Fix a regression that MLC support triggers dead lock in dynamic " Lv Zheng @ 2016-06-21 4:34 ` Lv Zheng 2016-06-21 4:34 ` [PATCH v4 3/5] ACPICA: ACPI 2.0, Interpreter: Fix MLC issues by switching to new TermList grammar for table loading Lv Zheng ` (2 subsequent siblings) 4 siblings, 0 replies; 40+ messages in thread From: Lv Zheng @ 2016-06-21 4:34 UTC (permalink / raw) To: Rafael J. Wysocki, Rafael J. Wysocki, Len Brown Cc: Lv Zheng, Lv Zheng, linux-kernel, linux-acpi Operation regions created by MLC were not tracked by acpi_check_address_range(), this patch fixes this issue. ACPICA BZ 1279. Fixed by Lv Zheng. Link: https://bugs.acpica.org/show_bug.cgi?id=1279 Signed-off-by: Lv Zheng <lv.zheng@intel.com> --- drivers/acpi/acpica/dsopcode.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/drivers/acpi/acpica/dsopcode.c b/drivers/acpi/acpica/dsopcode.c index 4cc9d98..96f2eef 100644 --- a/drivers/acpi/acpica/dsopcode.c +++ b/drivers/acpi/acpica/dsopcode.c @@ -455,6 +455,12 @@ acpi_ds_eval_region_operands(struct acpi_walk_state *walk_state, /* Now the address and length are valid for this opregion */ obj_desc->region.flags |= AOPOBJ_DATA_VALID; + if (walk_state->parse_flags & ACPI_PARSE_MODULE_LEVEL) { + status = acpi_ut_add_address_range(obj_desc->region.space_id, + obj_desc->region.address, + obj_desc->region.length, + node); + } return_ACPI_STATUS(status); } -- 1.7.10 ^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH v4 3/5] ACPICA: ACPI 2.0, Interpreter: Fix MLC issues by switching to new TermList grammar for table loading 2016-06-21 4:34 ` [PATCH v4 0/5] ACPI 2.0: Enable TermList interpretion for table loading Lv Zheng 2016-06-21 4:34 ` [PATCH v4 1/5] ACPICA: Namespace: Fix a regression that MLC support triggers dead lock in dynamic " Lv Zheng 2016-06-21 4:34 ` [PATCH v4 2/5] ACPICA: Dispatcher: Fix an issue that the opregions created by the linked MLC were not tracked Lv Zheng @ 2016-06-21 4:34 ` Lv Zheng 2016-07-04 23:42 ` Zheng, Lv 2016-06-21 4:34 ` [PATCH v4 4/5] ACPI 2.0 / AML: Enable correct ACPI subsystem initialization order for new table loading mode Lv Zheng 2016-06-21 4:34 ` [PATCH v4 5/5] ACPI 2.0 / AML: Fix module level execution by correctly parsing table as TermList Lv Zheng 4 siblings, 1 reply; 40+ messages in thread From: Lv Zheng @ 2016-06-21 4:34 UTC (permalink / raw) To: Rafael J. Wysocki, Rafael J. Wysocki, Len Brown Cc: Lv Zheng, Lv Zheng, linux-kernel, linux-acpi The MLC (Module Level Code) is an ACPICA terminology describing the AML code out of any control method, its support is the main contention of the interpreter behavior during the table loading. The original implementation of MLC in ACPICA had several issues: 1. Out of any control method, besides of the object creating opcodes, only the code blocks wrapped by "If/Else/While" opcodes were supported. 2. The supported MLC code blocks were executed after loading the table rather than being executed right in place. ============================================================ The demo of this order issue is as follows: Name (OBJ1, 1) If (CND1 == 1) { Name (OBJ2, 2) } Name (OBJ3, 3) The original MLC support created OBJ2 after OBJ3's creation. ============================================================ Other than these limitations, MLC support in ACPICA looks correct. And supporting this should be easy/natural for ACPICA, but enabling of this was blocked by some ACPICA internal and OSPM specific initialization order issues we've fixed recently. The wrong support started from the following false bug fixing commit: Commit: 80d7951177315f70b5ffd8663985fbf725d07799 Subject: Add support for module-level executable AML code. We can confirm Windows interpreter behavior via reverse engineering means. It can be proven that not only If/Else/While wrapped code blocks, all opcodes can be executed at the module level, including operation region accesses. And it can be proven that the MLC should be executed right in place, not in such a deferred way executed after loading the table. And the above facts indeed reflect the spec words around ACPI definition block tables (DSDT/SSDT/...), the entire table and the Scope object is defined by the AML specification in BNF style as: AMLCode := DefBlockHeader TermList DefScope := ScopeOp PkgLength NameString TermList The bodies of the scope opening terms (AMLCode/Scope) are all TermList, thus the table loading should be no difference than the control method evaluations as the body of the Method is also defined by the AML specification as TermList: DefMethod := MethodOp PkgLength NameString MethodFlags TermList The only difference is: after evaluating control method, created named objects may be freed due to no reference, while named objects created by the table loading should only be freed after unloading the table. So this patch follows the spec and the de-facto standard behavior, enables the new grammar (TermList) for the table loading. By doing so, beyond the fixes to the above issues, we can see additional differences comparing to the old grammar based table loading: 1. Originally, beyond the scope opening terms (AMLCode/Scope), If/Else/While wrapped code blocks under the scope creating terms (Device/PowerResource/Processor/ThermalZone) are also supported as deferred MLC, which violates the spec defined grammar where ObjectList is enforced. With MLC support improved as non-deferred, the interpreter parses such scope creating terms as TermList rather ObjectList like the scope opening terms. After probing the Windows behavior and proving that it also parses these terms as TermList, we submitted an ECR (Engineering Change Request) to the ASWG (ACPI Specification Working Group) to clarify this. The ECR is titled as "ASL Grammar Clarification for Executable AML Opcodes" and has been accepted by the ASWG. The new grammar will appear in ACPI specification 6.2. 2. Originally, Buffer/Package/OperationRegion/CreateXXXField/BankField arguments are evaluated in a deferred way after loading the table. With MLC support improved, they are also parsed right in place during the table loading. This is also Windows compliant and the only difference is the removal of the debugging messages implemented before acpi_ds_execute_arguments(), see Link 1 for the details. A previous commit should have ensured that acpi_check_address_range() won't regress. Note that enabling this feature may cause regressions due to long term Linux ACPI support on top of the wrong grammar. So this patch also prepares a global option to be used to roll back to the old grammar during the period between a regression is reported and the regression is root-cause-fixed. ACPICA BZ 963, fixed by Lv Zheng. Link 1: https://bugzilla.kernel.org/show_bug.cgi?id=112911 Link 2: https://bugs.acpica.org/show_bug.cgi?id=963 Tested-by: Chris Bainbridge <chris.bainbridge@gmail.com> Signed-off-by: Lv Zheng <lv.zheng@intel.com> --- drivers/acpi/acpica/acnamesp.h | 3 + drivers/acpi/acpica/acparser.h | 2 + drivers/acpi/acpica/evrgnini.c | 3 +- drivers/acpi/acpica/exconfig.c | 3 +- drivers/acpi/acpica/nsload.c | 3 +- drivers/acpi/acpica/nsparse.c | 166 ++++++++++++++++++++++++++++++++-------- drivers/acpi/acpica/psparse.c | 4 +- drivers/acpi/acpica/psxface.c | 71 +++++++++++++++++ drivers/acpi/acpica/tbxfload.c | 3 +- drivers/acpi/acpica/utxfinit.c | 3 +- include/acpi/acpixf.h | 6 ++ 11 files changed, 229 insertions(+), 38 deletions(-) diff --git a/drivers/acpi/acpica/acnamesp.h b/drivers/acpi/acpica/acnamesp.h index f33a4ba..829672a 100644 --- a/drivers/acpi/acpica/acnamesp.h +++ b/drivers/acpi/acpica/acnamesp.h @@ -130,6 +130,9 @@ acpi_status acpi_ns_parse_table(u32 table_index, struct acpi_namespace_node *start_node); acpi_status +acpi_ns_execute_table(u32 table_index, struct acpi_namespace_node *start_node); + +acpi_status acpi_ns_one_complete_parse(u32 pass_number, u32 table_index, struct acpi_namespace_node *start_node); diff --git a/drivers/acpi/acpica/acparser.h b/drivers/acpi/acpica/acparser.h index fc30577..939d411 100644 --- a/drivers/acpi/acpica/acparser.h +++ b/drivers/acpi/acpica/acparser.h @@ -78,6 +78,8 @@ extern const u8 acpi_gbl_long_op_index[]; */ acpi_status acpi_ps_execute_method(struct acpi_evaluate_info *info); +acpi_status acpi_ps_execute_table(struct acpi_evaluate_info *info); + /* * psargs - Parse AML opcode arguments */ diff --git a/drivers/acpi/acpica/evrgnini.c b/drivers/acpi/acpica/evrgnini.c index b6ea9c0..3843f1f 100644 --- a/drivers/acpi/acpica/evrgnini.c +++ b/drivers/acpi/acpica/evrgnini.c @@ -553,7 +553,8 @@ acpi_ev_initialize_region(union acpi_operand_object *region_obj, * * See acpi_ns_exec_module_code */ - if (obj_desc->method. + if (!acpi_gbl_parse_table_as_term_list && + obj_desc->method. info_flags & ACPI_METHOD_MODULE_LEVEL) { handler_obj = obj_desc->method.dispatch.handler; diff --git a/drivers/acpi/acpica/exconfig.c b/drivers/acpi/acpica/exconfig.c index 21932d6..75679bc 100644 --- a/drivers/acpi/acpica/exconfig.c +++ b/drivers/acpi/acpica/exconfig.c @@ -120,7 +120,8 @@ acpi_ex_add_table(u32 table_index, /* Execute any module-level code that was found in the table */ acpi_ex_exit_interpreter(); - if (acpi_gbl_group_module_level_code) { + if (!acpi_gbl_parse_table_as_term_list + && acpi_gbl_group_module_level_code) { acpi_ns_exec_module_code_list(); } acpi_ex_enter_interpreter(); diff --git a/drivers/acpi/acpica/nsload.c b/drivers/acpi/acpica/nsload.c index b5e2b0a..2daa9a09 100644 --- a/drivers/acpi/acpica/nsload.c +++ b/drivers/acpi/acpica/nsload.c @@ -162,7 +162,8 @@ unlock: * other ACPI implementations. Optionally, the execution can be deferred * until later, see acpi_initialize_objects. */ - if (!acpi_gbl_group_module_level_code) { + if (!acpi_gbl_parse_table_as_term_list + && !acpi_gbl_group_module_level_code) { acpi_ns_exec_module_code_list(); } diff --git a/drivers/acpi/acpica/nsparse.c b/drivers/acpi/acpica/nsparse.c index 1783cd7..f475889 100644 --- a/drivers/acpi/acpica/nsparse.c +++ b/drivers/acpi/acpica/nsparse.c @@ -54,6 +54,98 @@ ACPI_MODULE_NAME("nsparse") /******************************************************************************* * + * FUNCTION: ns_execute_table + * + * PARAMETERS: table_desc - An ACPI table descriptor for table to parse + * start_node - Where to enter the table into the namespace + * + * RETURN: Status + * + * DESCRIPTION: Load ACPI/AML table by executing the entire table as a + * term_list. + * + ******************************************************************************/ +acpi_status +acpi_ns_execute_table(u32 table_index, struct acpi_namespace_node *start_node) +{ + acpi_status status; + struct acpi_table_header *table; + acpi_owner_id owner_id; + struct acpi_evaluate_info *info = NULL; + u32 aml_length; + u8 *aml_start; + union acpi_operand_object *method_obj = NULL; + + ACPI_FUNCTION_TRACE(ns_execute_table); + + status = acpi_get_table_by_index(table_index, &table); + if (ACPI_FAILURE(status)) { + return_ACPI_STATUS(status); + } + + /* Table must consist of at least a complete header */ + + if (table->length < sizeof(struct acpi_table_header)) { + return_ACPI_STATUS(AE_BAD_HEADER); + } + + aml_start = (u8 *)table + sizeof(struct acpi_table_header); + aml_length = table->length - sizeof(struct acpi_table_header); + + status = acpi_tb_get_owner_id(table_index, &owner_id); + if (ACPI_FAILURE(status)) { + return_ACPI_STATUS(status); + } + + /* Create, initialize, and link a new temporary method object */ + + method_obj = acpi_ut_create_internal_object(ACPI_TYPE_METHOD); + if (!method_obj) { + return_ACPI_STATUS(AE_NO_MEMORY); + } + + /* Allocate the evaluation information block */ + + info = ACPI_ALLOCATE_ZEROED(sizeof(struct acpi_evaluate_info)); + if (!info) { + status = AE_NO_MEMORY; + goto cleanup; + } + + ACPI_DEBUG_PRINT((ACPI_DB_PARSE, + "Create table code block: %p\n", method_obj)); + + method_obj->method.aml_start = aml_start; + method_obj->method.aml_length = aml_length; + method_obj->method.owner_id = owner_id; + method_obj->method.info_flags |= ACPI_METHOD_MODULE_LEVEL; + + info->pass_number = ACPI_IMODE_EXECUTE; + info->node = start_node; + info->obj_desc = method_obj; + info->node_flags = info->node->flags; + info->full_pathname = acpi_ns_get_normalized_pathname(info->node, TRUE); + if (!info->full_pathname) { + status = AE_NO_MEMORY; + goto cleanup; + } + + (void)acpi_ut_release_mutex(ACPI_MTX_NAMESPACE); + status = acpi_ps_execute_table(info); + (void)acpi_ut_acquire_mutex(ACPI_MTX_NAMESPACE); + +cleanup: + if (info) { + ACPI_FREE(info->full_pathname); + info->full_pathname = NULL; + } + ACPI_FREE(info); + acpi_ut_remove_reference(method_obj); + return_ACPI_STATUS(status); +} + +/******************************************************************************* + * * FUNCTION: ns_one_complete_parse * * PARAMETERS: pass_number - 1 or 2 @@ -64,6 +156,7 @@ ACPI_MODULE_NAME("nsparse") * DESCRIPTION: Perform one complete parse of an ACPI/AML table. * ******************************************************************************/ + acpi_status acpi_ns_one_complete_parse(u32 pass_number, u32 table_index, @@ -173,38 +266,47 @@ acpi_ns_parse_table(u32 table_index, struct acpi_namespace_node *start_node) acpi_ex_enter_interpreter(); - /* - * AML Parse, pass 1 - * - * In this pass, we load most of the namespace. Control methods - * are not parsed until later. A parse tree is not created. Instead, - * each Parser Op subtree is deleted when it is finished. This saves - * a great deal of memory, and allows a small cache of parse objects - * to service the entire parse. The second pass of the parse then - * performs another complete parse of the AML. - */ - ACPI_DEBUG_PRINT((ACPI_DB_PARSE, "**** Start pass 1\n")); - - status = acpi_ns_one_complete_parse(ACPI_IMODE_LOAD_PASS1, - table_index, start_node); - if (ACPI_FAILURE(status)) { - goto error_exit; - } - - /* - * AML Parse, pass 2 - * - * In this pass, we resolve forward references and other things - * that could not be completed during the first pass. - * Another complete parse of the AML is performed, but the - * overhead of this is compensated for by the fact that the - * parse objects are all cached. - */ - ACPI_DEBUG_PRINT((ACPI_DB_PARSE, "**** Start pass 2\n")); - status = acpi_ns_one_complete_parse(ACPI_IMODE_LOAD_PASS2, - table_index, start_node); - if (ACPI_FAILURE(status)) { - goto error_exit; + if (acpi_gbl_parse_table_as_term_list) { + ACPI_DEBUG_PRINT((ACPI_DB_PARSE, "**** Start load pass\n")); + + status = acpi_ns_execute_table(table_index, start_node); + if (ACPI_FAILURE(status)) { + goto error_exit; + } + } else { + /* + * AML Parse, pass 1 + * + * In this pass, we load most of the namespace. Control methods + * are not parsed until later. A parse tree is not created. + * Instead, each Parser Op subtree is deleted when it is finished. + * This saves a great deal of memory, and allows a small cache of + * parse objects to service the entire parse. The second pass of + * the parse then performs another complete parse of the AML. + */ + ACPI_DEBUG_PRINT((ACPI_DB_PARSE, "**** Start pass 1\n")); + + status = acpi_ns_one_complete_parse(ACPI_IMODE_LOAD_PASS1, + table_index, start_node); + if (ACPI_FAILURE(status)) { + goto error_exit; + } + + /* + * AML Parse, pass 2 + * + * In this pass, we resolve forward references and other things + * that could not be completed during the first pass. + * Another complete parse of the AML is performed, but the + * overhead of this is compensated for by the fact that the + * parse objects are all cached. + */ + ACPI_DEBUG_PRINT((ACPI_DB_PARSE, "**** Start pass 2\n")); + status = acpi_ns_one_complete_parse(ACPI_IMODE_LOAD_PASS2, + table_index, start_node); + if (ACPI_FAILURE(status)) { + goto error_exit; + } } error_exit: diff --git a/drivers/acpi/acpica/psparse.c b/drivers/acpi/acpica/psparse.c index 0a23897..3aa9162 100644 --- a/drivers/acpi/acpica/psparse.c +++ b/drivers/acpi/acpica/psparse.c @@ -571,7 +571,9 @@ acpi_status acpi_ps_parse_aml(struct acpi_walk_state *walk_state) * cleanup to do */ if (((walk_state->parse_flags & ACPI_PARSE_MODE_MASK) == - ACPI_PARSE_EXECUTE) || (ACPI_FAILURE(status))) { + ACPI_PARSE_EXECUTE && + !(walk_state->parse_flags & ACPI_PARSE_MODULE_LEVEL)) || + (ACPI_FAILURE(status))) { acpi_ds_terminate_control_method(walk_state-> method_desc, walk_state); diff --git a/drivers/acpi/acpica/psxface.c b/drivers/acpi/acpica/psxface.c index cf30cd82..fccc0e5 100644 --- a/drivers/acpi/acpica/psxface.c +++ b/drivers/acpi/acpica/psxface.c @@ -252,6 +252,77 @@ cleanup: /******************************************************************************* * + * FUNCTION: acpi_ps_execute_table + * + * PARAMETERS: info - Method info block, contains: + * node - Node to where the is entered into the + * namespace + * obj_desc - Pseudo method object describing the AML + * code of the entire table + * pass_number - Parse or execute pass + * + * RETURN: Status + * + * DESCRIPTION: Execute a table + * + ******************************************************************************/ + +acpi_status acpi_ps_execute_table(struct acpi_evaluate_info *info) +{ + acpi_status status; + union acpi_parse_object *op = NULL; + struct acpi_walk_state *walk_state = NULL; + + ACPI_FUNCTION_TRACE(ps_execute_table); + + /* Create and init a Root Node */ + + op = acpi_ps_create_scope_op(info->obj_desc->method.aml_start); + if (!op) { + status = AE_NO_MEMORY; + goto cleanup; + } + + /* Create and initialize a new walk state */ + + walk_state = + acpi_ds_create_walk_state(info->obj_desc->method.owner_id, NULL, + NULL, NULL); + if (!walk_state) { + status = AE_NO_MEMORY; + goto cleanup; + } + + status = acpi_ds_init_aml_walk(walk_state, op, info->node, + info->obj_desc->method.aml_start, + info->obj_desc->method.aml_length, info, + info->pass_number); + if (ACPI_FAILURE(status)) { + goto cleanup; + } + + if (info->obj_desc->method.info_flags & ACPI_METHOD_MODULE_LEVEL) { + walk_state->parse_flags |= ACPI_PARSE_MODULE_LEVEL; + } + + /* + * Parse the AML, walk_state will be deleted by parse_aml + */ + status = acpi_ps_parse_aml(walk_state); + walk_state = NULL; + +cleanup: + if (op) { + acpi_ps_delete_parse_tree(op); + } + if (walk_state) { + acpi_ds_delete_walk_state(walk_state); + } + return_ACPI_STATUS(status); +} + +/******************************************************************************* + * * FUNCTION: acpi_ps_update_parameter_list * * PARAMETERS: info - See struct acpi_evaluate_info diff --git a/drivers/acpi/acpica/tbxfload.c b/drivers/acpi/acpica/tbxfload.c index ac71abc..2b9e87f 100644 --- a/drivers/acpi/acpica/tbxfload.c +++ b/drivers/acpi/acpica/tbxfload.c @@ -103,7 +103,8 @@ acpi_status __init acpi_load_tables(void) "While loading namespace from ACPI tables")); } - if (!acpi_gbl_group_module_level_code) { + if (acpi_gbl_parse_table_as_term_list + || !acpi_gbl_group_module_level_code) { /* * Initialize the objects that remain uninitialized. This * runs the executable AML that may be part of the diff --git a/drivers/acpi/acpica/utxfinit.c b/drivers/acpi/acpica/utxfinit.c index 75b5f27..e41424a 100644 --- a/drivers/acpi/acpica/utxfinit.c +++ b/drivers/acpi/acpica/utxfinit.c @@ -265,7 +265,8 @@ acpi_status __init acpi_initialize_objects(u32 flags) * all of the tables have been loaded. It is a legacy option and is * not compatible with other ACPI implementations. See acpi_ns_load_table. */ - if (acpi_gbl_group_module_level_code) { + if (!acpi_gbl_parse_table_as_term_list + && acpi_gbl_group_module_level_code) { acpi_ns_exec_module_code_list(); /* diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h index 4e4c214..22b0397 100644 --- a/include/acpi/acpixf.h +++ b/include/acpi/acpixf.h @@ -195,6 +195,12 @@ ACPI_INIT_GLOBAL(u8, acpi_gbl_do_not_use_xsdt, FALSE); ACPI_INIT_GLOBAL(u8, acpi_gbl_group_module_level_code, FALSE); /* + * Optionally support module level code by parsing the entire table as + * a term_list. Default is FALSE, do not execute entire table. + */ +ACPI_INIT_GLOBAL(u8, acpi_gbl_parse_table_as_term_list, FALSE); + +/* * Optionally use 32-bit FADT addresses if and when there is a conflict * (address mismatch) between the 32-bit and 64-bit versions of the * address. Although ACPICA adheres to the ACPI specification which -- 1.7.10 ^ permalink raw reply related [flat|nested] 40+ messages in thread
* RE: [PATCH v4 3/5] ACPICA: ACPI 2.0, Interpreter: Fix MLC issues by switching to new TermList grammar for table loading 2016-06-21 4:34 ` [PATCH v4 3/5] ACPICA: ACPI 2.0, Interpreter: Fix MLC issues by switching to new TermList grammar for table loading Lv Zheng @ 2016-07-04 23:42 ` Zheng, Lv 2016-07-05 0:09 ` Rafael J. Wysocki 0 siblings, 1 reply; 40+ messages in thread From: Zheng, Lv @ 2016-07-04 23:42 UTC (permalink / raw) To: Wysocki, Rafael J, Rafael J. Wysocki, Brown, Len Cc: Lv Zheng, linux-kernel, linux-acpi Hi, Rafael > From: Zheng, Lv > Subject: [PATCH v4 3/5] ACPICA: ACPI 2.0, Interpreter: Fix MLC issues by > switching to new TermList grammar for table loading > > The MLC (Module Level Code) is an ACPICA terminology describing the > AML > code out of any control method, its support is the main contention of the > interpreter behavior during the table loading. > > The original implementation of MLC in ACPICA had several issues: > 1. Out of any control method, besides of the object creating opcodes, only > the code blocks wrapped by "If/Else/While" opcodes were supported. > 2. The supported MLC code blocks were executed after loading the table > rather than being executed right in place. > > ========================================================== > == > The demo of this order issue is as follows: > Name (OBJ1, 1) > If (CND1 == 1) > { > Name (OBJ2, 2) > } > Name (OBJ3, 3) > The original MLC support created OBJ2 after OBJ3's creation. > > ========================================================== > == > Other than these limitations, MLC support in ACPICA looks correct. And > supporting this should be easy/natural for ACPICA, but enabling of this > was > blocked by some ACPICA internal and OSPM specific initialization order > issues we've fixed recently. The wrong support started from the following > false bug fixing commit: > Commit: 80d7951177315f70b5ffd8663985fbf725d07799 > Subject: Add support for module-level executable AML code. > > We can confirm Windows interpreter behavior via reverse engineering > means. > It can be proven that not only If/Else/While wrapped code blocks, all > opcodes can be executed at the module level, including operation region > accesses. And it can be proven that the MLC should be executed right in > place, not in such a deferred way executed after loading the table. > > And the above facts indeed reflect the spec words around ACPI definition > block tables (DSDT/SSDT/...), the entire table and the Scope object is > defined by the AML specification in BNF style as: > AMLCode := DefBlockHeader TermList > DefScope := ScopeOp PkgLength NameString TermList > The bodies of the scope opening terms (AMLCode/Scope) are all TermList, > thus the table loading should be no difference than the control method > evaluations as the body of the Method is also defined by the AML > specification as TermList: > DefMethod := MethodOp PkgLength NameString MethodFlags TermList > The only difference is: after evaluating control method, created named > objects may be freed due to no reference, while named objects created by > the table loading should only be freed after unloading the table. > > So this patch follows the spec and the de-facto standard behavior, enables > the new grammar (TermList) for the table loading. > > By doing so, beyond the fixes to the above issues, we can see additional > differences comparing to the old grammar based table loading: > 1. Originally, beyond the scope opening terms (AMLCode/Scope), > If/Else/While wrapped code blocks under the scope creating terms > (Device/PowerResource/Processor/ThermalZone) are also supported as > deferred MLC, which violates the spec defined grammar where ObjectList > is enforced. With MLC support improved as non-deferred, the interpreter > parses such scope creating terms as TermList rather ObjectList like the > scope opening terms. > After probing the Windows behavior and proving that it also parses > these > terms as TermList, we submitted an ECR (Engineering Change Request) to > the ASWG (ACPI Specification Working Group) to clarify this. The ECR is > titled as "ASL Grammar Clarification for Executable AML Opcodes" and > has > been accepted by the ASWG. The new grammar will appear in ACPI > specification 6.2. > 2. Originally, Buffer/Package/OperationRegion/CreateXXXField/BankField > arguments are evaluated in a deferred way after loading the table. With > MLC support improved, they are also parsed right in place during the > table loading. > This is also Windows compliant and the only difference is the removal > of the debugging messages implemented before > acpi_ds_execute_arguments(), > see Link 1 for the details. A previous commit should have ensured that > acpi_check_address_range() won't regress. > > Note that enabling this feature may cause regressions due to long term > Linux ACPI support on top of the wrong grammar. So this patch also > prepares > a global option to be used to roll back to the old grammar during the > period between a regression is reported and the regression is > root-cause-fixed. ACPICA BZ 963, fixed by Lv Zheng. > > Link 1: https://bugzilla.kernel.org/show_bug.cgi?id=112911 > Link 2: https://bugs.acpica.org/show_bug.cgi?id=963 > Tested-by: Chris Bainbridge <chris.bainbridge@gmail.com> > Signed-off-by: Lv Zheng <lv.zheng@intel.com> > --- > drivers/acpi/acpica/acnamesp.h | 3 + > drivers/acpi/acpica/acparser.h | 2 + > drivers/acpi/acpica/evrgnini.c | 3 +- > drivers/acpi/acpica/exconfig.c | 3 +- > drivers/acpi/acpica/nsload.c | 3 +- > drivers/acpi/acpica/nsparse.c | 166 > ++++++++++++++++++++++++++++++++-------- > drivers/acpi/acpica/psparse.c | 4 +- > drivers/acpi/acpica/psxface.c | 71 +++++++++++++++++ > drivers/acpi/acpica/tbxfload.c | 3 +- > drivers/acpi/acpica/utxfinit.c | 3 +- > include/acpi/acpixf.h | 6 ++ > 11 files changed, 229 insertions(+), 38 deletions(-) > > diff --git a/drivers/acpi/acpica/acnamesp.h > b/drivers/acpi/acpica/acnamesp.h > index f33a4ba..829672a 100644 > --- a/drivers/acpi/acpica/acnamesp.h > +++ b/drivers/acpi/acpica/acnamesp.h > @@ -130,6 +130,9 @@ acpi_status > acpi_ns_parse_table(u32 table_index, struct acpi_namespace_node > *start_node); > > acpi_status > +acpi_ns_execute_table(u32 table_index, struct acpi_namespace_node > *start_node); > + > +acpi_status > acpi_ns_one_complete_parse(u32 pass_number, > u32 table_index, > struct acpi_namespace_node *start_node); > diff --git a/drivers/acpi/acpica/acparser.h b/drivers/acpi/acpica/acparser.h > index fc30577..939d411 100644 > --- a/drivers/acpi/acpica/acparser.h > +++ b/drivers/acpi/acpica/acparser.h > @@ -78,6 +78,8 @@ extern const u8 acpi_gbl_long_op_index[]; > */ > acpi_status acpi_ps_execute_method(struct acpi_evaluate_info *info); > > +acpi_status acpi_ps_execute_table(struct acpi_evaluate_info *info); > + > /* > * psargs - Parse AML opcode arguments > */ > diff --git a/drivers/acpi/acpica/evrgnini.c b/drivers/acpi/acpica/evrgnini.c > index b6ea9c0..3843f1f 100644 > --- a/drivers/acpi/acpica/evrgnini.c > +++ b/drivers/acpi/acpica/evrgnini.c > @@ -553,7 +553,8 @@ acpi_ev_initialize_region(union > acpi_operand_object *region_obj, > * > * See acpi_ns_exec_module_code > */ > - if (obj_desc->method. > + if (!acpi_gbl_parse_table_as_term_list && > + obj_desc->method. > info_flags & > ACPI_METHOD_MODULE_LEVEL) { > handler_obj = > obj_desc- > >method.dispatch.handler; > diff --git a/drivers/acpi/acpica/exconfig.c b/drivers/acpi/acpica/exconfig.c > index 21932d6..75679bc 100644 > --- a/drivers/acpi/acpica/exconfig.c > +++ b/drivers/acpi/acpica/exconfig.c > @@ -120,7 +120,8 @@ acpi_ex_add_table(u32 table_index, > /* Execute any module-level code that was found in the table */ > > acpi_ex_exit_interpreter(); > - if (acpi_gbl_group_module_level_code) { > + if (!acpi_gbl_parse_table_as_term_list > + && acpi_gbl_group_module_level_code) { > acpi_ns_exec_module_code_list(); > } > acpi_ex_enter_interpreter(); > diff --git a/drivers/acpi/acpica/nsload.c b/drivers/acpi/acpica/nsload.c > index b5e2b0a..2daa9a09 100644 > --- a/drivers/acpi/acpica/nsload.c > +++ b/drivers/acpi/acpica/nsload.c > @@ -162,7 +162,8 @@ unlock: > * other ACPI implementations. Optionally, the execution can be > deferred > * until later, see acpi_initialize_objects. > */ > - if (!acpi_gbl_group_module_level_code) { > + if (!acpi_gbl_parse_table_as_term_list > + && !acpi_gbl_group_module_level_code) { > acpi_ns_exec_module_code_list(); > } > > diff --git a/drivers/acpi/acpica/nsparse.c b/drivers/acpi/acpica/nsparse.c > index 1783cd7..f475889 100644 > --- a/drivers/acpi/acpica/nsparse.c > +++ b/drivers/acpi/acpica/nsparse.c > @@ -54,6 +54,98 @@ ACPI_MODULE_NAME("nsparse") > > > /********************************************************* > ********************** > * > + * FUNCTION: ns_execute_table > + * > + * PARAMETERS: table_desc - An ACPI table descriptor for table to > parse > + * start_node - Where to enter the table into the namespace > + * > + * RETURN: Status > + * > + * DESCRIPTION: Load ACPI/AML table by executing the entire table as a > + * term_list. > + * > + > ********************************************************** > ********************/ > +acpi_status > +acpi_ns_execute_table(u32 table_index, struct acpi_namespace_node > *start_node) > +{ > + acpi_status status; > + struct acpi_table_header *table; > + acpi_owner_id owner_id; > + struct acpi_evaluate_info *info = NULL; > + u32 aml_length; > + u8 *aml_start; > + union acpi_operand_object *method_obj = NULL; > + > + ACPI_FUNCTION_TRACE(ns_execute_table); > + > + status = acpi_get_table_by_index(table_index, &table); > + if (ACPI_FAILURE(status)) { > + return_ACPI_STATUS(status); > + } > + > + /* Table must consist of at least a complete header */ > + > + if (table->length < sizeof(struct acpi_table_header)) { > + return_ACPI_STATUS(AE_BAD_HEADER); > + } > + > + aml_start = (u8 *)table + sizeof(struct acpi_table_header); > + aml_length = table->length - sizeof(struct acpi_table_header); > + > + status = acpi_tb_get_owner_id(table_index, &owner_id); > + if (ACPI_FAILURE(status)) { > + return_ACPI_STATUS(status); > + } > + > + /* Create, initialize, and link a new temporary method object */ > + > + method_obj = > acpi_ut_create_internal_object(ACPI_TYPE_METHOD); > + if (!method_obj) { > + return_ACPI_STATUS(AE_NO_MEMORY); > + } > + > + /* Allocate the evaluation information block */ > + > + info = ACPI_ALLOCATE_ZEROED(sizeof(struct acpi_evaluate_info)); > + if (!info) { > + status = AE_NO_MEMORY; > + goto cleanup; > + } > + > + ACPI_DEBUG_PRINT((ACPI_DB_PARSE, > + "Create table code block: %p\n", method_obj)); > + > + method_obj->method.aml_start = aml_start; > + method_obj->method.aml_length = aml_length; > + method_obj->method.owner_id = owner_id; > + method_obj->method.info_flags |= > ACPI_METHOD_MODULE_LEVEL; > + > + info->pass_number = ACPI_IMODE_EXECUTE; > + info->node = start_node; > + info->obj_desc = method_obj; > + info->node_flags = info->node->flags; > + info->full_pathname = acpi_ns_get_normalized_pathname(info- > >node, TRUE); > + if (!info->full_pathname) { > + status = AE_NO_MEMORY; > + goto cleanup; > + } > + > + (void)acpi_ut_release_mutex(ACPI_MTX_NAMESPACE); > + status = acpi_ps_execute_table(info); > + (void)acpi_ut_acquire_mutex(ACPI_MTX_NAMESPACE); [Lv Zheng] I got a report that we have lock order issue in the previous bug fix: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=2f38b1b1 And I'll send a quick fix for that later today. After investigation, we found that ACPICA namespace lock design actually requires a big change in order to support this grammar change. This means that this patch still need to be improved. And the above 3 lines are currently known to be not compliant to the current ACPICA namespace lock design. So please drop this series, and I'll re-send this series after improving the ACPICA namespace lock design. Sorry for the issues. Thanks and best regards -Lv > + > +cleanup: > + if (info) { > + ACPI_FREE(info->full_pathname); > + info->full_pathname = NULL; > + } > + ACPI_FREE(info); > + acpi_ut_remove_reference(method_obj); > + return_ACPI_STATUS(status); > +} > + > +/******************************************************** > *********************** > + * > * FUNCTION: ns_one_complete_parse > * > * PARAMETERS: pass_number - 1 or 2 > @@ -64,6 +156,7 @@ ACPI_MODULE_NAME("nsparse") > * DESCRIPTION: Perform one complete parse of an ACPI/AML table. > * > > ********************************************************** > ********************/ > + > acpi_status > acpi_ns_one_complete_parse(u32 pass_number, > u32 table_index, > @@ -173,38 +266,47 @@ acpi_ns_parse_table(u32 table_index, struct > acpi_namespace_node *start_node) > > acpi_ex_enter_interpreter(); > > - /* > - * AML Parse, pass 1 > - * > - * In this pass, we load most of the namespace. Control methods > - * are not parsed until later. A parse tree is not created. Instead, > - * each Parser Op subtree is deleted when it is finished. This saves > - * a great deal of memory, and allows a small cache of parse > objects > - * to service the entire parse. The second pass of the parse then > - * performs another complete parse of the AML. > - */ > - ACPI_DEBUG_PRINT((ACPI_DB_PARSE, "**** Start pass 1\n")); > - > - status = acpi_ns_one_complete_parse(ACPI_IMODE_LOAD_PASS1, > - table_index, start_node); > - if (ACPI_FAILURE(status)) { > - goto error_exit; > - } > - > - /* > - * AML Parse, pass 2 > - * > - * In this pass, we resolve forward references and other things > - * that could not be completed during the first pass. > - * Another complete parse of the AML is performed, but the > - * overhead of this is compensated for by the fact that the > - * parse objects are all cached. > - */ > - ACPI_DEBUG_PRINT((ACPI_DB_PARSE, "**** Start pass 2\n")); > - status = acpi_ns_one_complete_parse(ACPI_IMODE_LOAD_PASS2, > - table_index, start_node); > - if (ACPI_FAILURE(status)) { > - goto error_exit; > + if (acpi_gbl_parse_table_as_term_list) { > + ACPI_DEBUG_PRINT((ACPI_DB_PARSE, "**** Start load > pass\n")); > + > + status = acpi_ns_execute_table(table_index, start_node); > + if (ACPI_FAILURE(status)) { > + goto error_exit; > + } > + } else { > + /* > + * AML Parse, pass 1 > + * > + * In this pass, we load most of the namespace. Control > methods > + * are not parsed until later. A parse tree is not created. > + * Instead, each Parser Op subtree is deleted when it is > finished. > + * This saves a great deal of memory, and allows a small > cache of > + * parse objects to service the entire parse. The second > pass of > + * the parse then performs another complete parse of the > AML. > + */ > + ACPI_DEBUG_PRINT((ACPI_DB_PARSE, "**** Start pass > 1\n")); > + > + status = > acpi_ns_one_complete_parse(ACPI_IMODE_LOAD_PASS1, > + table_index, start_node); > + if (ACPI_FAILURE(status)) { > + goto error_exit; > + } > + > + /* > + * AML Parse, pass 2 > + * > + * In this pass, we resolve forward references and other > things > + * that could not be completed during the first pass. > + * Another complete parse of the AML is performed, but > the > + * overhead of this is compensated for by the fact that the > + * parse objects are all cached. > + */ > + ACPI_DEBUG_PRINT((ACPI_DB_PARSE, "**** Start pass > 2\n")); > + status = > acpi_ns_one_complete_parse(ACPI_IMODE_LOAD_PASS2, > + table_index, start_node); > + if (ACPI_FAILURE(status)) { > + goto error_exit; > + } > } > > error_exit: > diff --git a/drivers/acpi/acpica/psparse.c b/drivers/acpi/acpica/psparse.c > index 0a23897..3aa9162 100644 > --- a/drivers/acpi/acpica/psparse.c > +++ b/drivers/acpi/acpica/psparse.c > @@ -571,7 +571,9 @@ acpi_status acpi_ps_parse_aml(struct > acpi_walk_state *walk_state) > * cleanup to do > */ > if (((walk_state->parse_flags & ACPI_PARSE_MODE_MASK) > == > - ACPI_PARSE_EXECUTE) || (ACPI_FAILURE(status))) { > + ACPI_PARSE_EXECUTE && > + !(walk_state->parse_flags & > ACPI_PARSE_MODULE_LEVEL)) || > + (ACPI_FAILURE(status))) { > acpi_ds_terminate_control_method(walk_state-> > method_desc, > walk_state); > diff --git a/drivers/acpi/acpica/psxface.c b/drivers/acpi/acpica/psxface.c > index cf30cd82..fccc0e5 100644 > --- a/drivers/acpi/acpica/psxface.c > +++ b/drivers/acpi/acpica/psxface.c > @@ -252,6 +252,77 @@ cleanup: > > > /********************************************************* > ********************** > * > + * FUNCTION: acpi_ps_execute_table > + * > + * PARAMETERS: info - Method info block, contains: > + * node - Node to where the is entered into the > + * namespace > + * obj_desc - Pseudo method object describing the AML > + * code of the entire table > + * pass_number - Parse or execute pass > + * > + * RETURN: Status > + * > + * DESCRIPTION: Execute a table > + * > + > ********************************************************** > ********************/ > + > +acpi_status acpi_ps_execute_table(struct acpi_evaluate_info *info) > +{ > + acpi_status status; > + union acpi_parse_object *op = NULL; > + struct acpi_walk_state *walk_state = NULL; > + > + ACPI_FUNCTION_TRACE(ps_execute_table); > + > + /* Create and init a Root Node */ > + > + op = acpi_ps_create_scope_op(info->obj_desc- > >method.aml_start); > + if (!op) { > + status = AE_NO_MEMORY; > + goto cleanup; > + } > + > + /* Create and initialize a new walk state */ > + > + walk_state = > + acpi_ds_create_walk_state(info->obj_desc->method.owner_id, > NULL, > + NULL, NULL); > + if (!walk_state) { > + status = AE_NO_MEMORY; > + goto cleanup; > + } > + > + status = acpi_ds_init_aml_walk(walk_state, op, info->node, > + info->obj_desc->method.aml_start, > + info->obj_desc->method.aml_length, > info, > + info->pass_number); > + if (ACPI_FAILURE(status)) { > + goto cleanup; > + } > + > + if (info->obj_desc->method.info_flags & > ACPI_METHOD_MODULE_LEVEL) { > + walk_state->parse_flags |= ACPI_PARSE_MODULE_LEVEL; > + } > + > + /* > + * Parse the AML, walk_state will be deleted by parse_aml > + */ > + status = acpi_ps_parse_aml(walk_state); > + walk_state = NULL; > + > +cleanup: > + if (op) { > + acpi_ps_delete_parse_tree(op); > + } > + if (walk_state) { > + acpi_ds_delete_walk_state(walk_state); > + } > + return_ACPI_STATUS(status); > +} > + > +/******************************************************** > *********************** > + * > * FUNCTION: acpi_ps_update_parameter_list > * > * PARAMETERS: info - See struct acpi_evaluate_info > diff --git a/drivers/acpi/acpica/tbxfload.c b/drivers/acpi/acpica/tbxfload.c > index ac71abc..2b9e87f 100644 > --- a/drivers/acpi/acpica/tbxfload.c > +++ b/drivers/acpi/acpica/tbxfload.c > @@ -103,7 +103,8 @@ acpi_status __init acpi_load_tables(void) > "While loading namespace from ACPI > tables")); > } > > - if (!acpi_gbl_group_module_level_code) { > + if (acpi_gbl_parse_table_as_term_list > + || !acpi_gbl_group_module_level_code) { > /* > * Initialize the objects that remain uninitialized. This > * runs the executable AML that may be part of the > diff --git a/drivers/acpi/acpica/utxfinit.c b/drivers/acpi/acpica/utxfinit.c > index 75b5f27..e41424a 100644 > --- a/drivers/acpi/acpica/utxfinit.c > +++ b/drivers/acpi/acpica/utxfinit.c > @@ -265,7 +265,8 @@ acpi_status __init acpi_initialize_objects(u32 flags) > * all of the tables have been loaded. It is a legacy option and is > * not compatible with other ACPI implementations. See > acpi_ns_load_table. > */ > - if (acpi_gbl_group_module_level_code) { > + if (!acpi_gbl_parse_table_as_term_list > + && acpi_gbl_group_module_level_code) { > acpi_ns_exec_module_code_list(); > > /* > diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h > index 4e4c214..22b0397 100644 > --- a/include/acpi/acpixf.h > +++ b/include/acpi/acpixf.h > @@ -195,6 +195,12 @@ ACPI_INIT_GLOBAL(u8, > acpi_gbl_do_not_use_xsdt, FALSE); > ACPI_INIT_GLOBAL(u8, acpi_gbl_group_module_level_code, FALSE); > > /* > + * Optionally support module level code by parsing the entire table as > + * a term_list. Default is FALSE, do not execute entire table. > + */ > +ACPI_INIT_GLOBAL(u8, acpi_gbl_parse_table_as_term_list, FALSE); > + > +/* > * Optionally use 32-bit FADT addresses if and when there is a conflict > * (address mismatch) between the 32-bit and 64-bit versions of the > * address. Although ACPICA adheres to the ACPI specification which > -- > 1.7.10 ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v4 3/5] ACPICA: ACPI 2.0, Interpreter: Fix MLC issues by switching to new TermList grammar for table loading 2016-07-04 23:42 ` Zheng, Lv @ 2016-07-05 0:09 ` Rafael J. Wysocki 0 siblings, 0 replies; 40+ messages in thread From: Rafael J. Wysocki @ 2016-07-05 0:09 UTC (permalink / raw) To: Zheng, Lv Cc: Wysocki, Rafael J, Rafael J. Wysocki, Brown, Len, Lv Zheng, linux-kernel, linux-acpi On Tue, Jul 5, 2016 at 1:42 AM, Zheng, Lv <lv.zheng@intel.com> wrote: > Hi, Rafael > >> From: Zheng, Lv >> Subject: [PATCH v4 3/5] ACPICA: ACPI 2.0, Interpreter: Fix MLC issues by >> switching to new TermList grammar for table loading >> >> The MLC (Module Level Code) is an ACPICA terminology describing the >> AML >> code out of any control method, its support is the main contention of the >> interpreter behavior during the table loading. >> >> The original implementation of MLC in ACPICA had several issues: >> 1. Out of any control method, besides of the object creating opcodes, only >> the code blocks wrapped by "If/Else/While" opcodes were supported. >> 2. The supported MLC code blocks were executed after loading the table >> rather than being executed right in place. >> >> ========================================================== >> == >> The demo of this order issue is as follows: >> Name (OBJ1, 1) >> If (CND1 == 1) >> { >> Name (OBJ2, 2) >> } >> Name (OBJ3, 3) >> The original MLC support created OBJ2 after OBJ3's creation. >> >> ========================================================== >> == >> Other than these limitations, MLC support in ACPICA looks correct. And >> supporting this should be easy/natural for ACPICA, but enabling of this >> was >> blocked by some ACPICA internal and OSPM specific initialization order >> issues we've fixed recently. The wrong support started from the following >> false bug fixing commit: >> Commit: 80d7951177315f70b5ffd8663985fbf725d07799 >> Subject: Add support for module-level executable AML code. >> >> We can confirm Windows interpreter behavior via reverse engineering >> means. >> It can be proven that not only If/Else/While wrapped code blocks, all >> opcodes can be executed at the module level, including operation region >> accesses. And it can be proven that the MLC should be executed right in >> place, not in such a deferred way executed after loading the table. >> >> And the above facts indeed reflect the spec words around ACPI definition >> block tables (DSDT/SSDT/...), the entire table and the Scope object is >> defined by the AML specification in BNF style as: >> AMLCode := DefBlockHeader TermList >> DefScope := ScopeOp PkgLength NameString TermList >> The bodies of the scope opening terms (AMLCode/Scope) are all TermList, >> thus the table loading should be no difference than the control method >> evaluations as the body of the Method is also defined by the AML >> specification as TermList: >> DefMethod := MethodOp PkgLength NameString MethodFlags TermList >> The only difference is: after evaluating control method, created named >> objects may be freed due to no reference, while named objects created by >> the table loading should only be freed after unloading the table. >> >> So this patch follows the spec and the de-facto standard behavior, enables >> the new grammar (TermList) for the table loading. >> >> By doing so, beyond the fixes to the above issues, we can see additional >> differences comparing to the old grammar based table loading: >> 1. Originally, beyond the scope opening terms (AMLCode/Scope), >> If/Else/While wrapped code blocks under the scope creating terms >> (Device/PowerResource/Processor/ThermalZone) are also supported as >> deferred MLC, which violates the spec defined grammar where ObjectList >> is enforced. With MLC support improved as non-deferred, the interpreter >> parses such scope creating terms as TermList rather ObjectList like the >> scope opening terms. >> After probing the Windows behavior and proving that it also parses >> these >> terms as TermList, we submitted an ECR (Engineering Change Request) to >> the ASWG (ACPI Specification Working Group) to clarify this. The ECR is >> titled as "ASL Grammar Clarification for Executable AML Opcodes" and >> has >> been accepted by the ASWG. The new grammar will appear in ACPI >> specification 6.2. >> 2. Originally, Buffer/Package/OperationRegion/CreateXXXField/BankField >> arguments are evaluated in a deferred way after loading the table. With >> MLC support improved, they are also parsed right in place during the >> table loading. >> This is also Windows compliant and the only difference is the removal >> of the debugging messages implemented before >> acpi_ds_execute_arguments(), >> see Link 1 for the details. A previous commit should have ensured that >> acpi_check_address_range() won't regress. >> >> Note that enabling this feature may cause regressions due to long term >> Linux ACPI support on top of the wrong grammar. So this patch also >> prepares >> a global option to be used to roll back to the old grammar during the >> period between a regression is reported and the regression is >> root-cause-fixed. ACPICA BZ 963, fixed by Lv Zheng. >> >> Link 1: https://bugzilla.kernel.org/show_bug.cgi?id=112911 >> Link 2: https://bugs.acpica.org/show_bug.cgi?id=963 >> Tested-by: Chris Bainbridge <chris.bainbridge@gmail.com> >> Signed-off-by: Lv Zheng <lv.zheng@intel.com> >> --- >> drivers/acpi/acpica/acnamesp.h | 3 + >> drivers/acpi/acpica/acparser.h | 2 + >> drivers/acpi/acpica/evrgnini.c | 3 +- >> drivers/acpi/acpica/exconfig.c | 3 +- >> drivers/acpi/acpica/nsload.c | 3 +- >> drivers/acpi/acpica/nsparse.c | 166 >> ++++++++++++++++++++++++++++++++-------- >> drivers/acpi/acpica/psparse.c | 4 +- >> drivers/acpi/acpica/psxface.c | 71 +++++++++++++++++ >> drivers/acpi/acpica/tbxfload.c | 3 +- >> drivers/acpi/acpica/utxfinit.c | 3 +- >> include/acpi/acpixf.h | 6 ++ >> 11 files changed, 229 insertions(+), 38 deletions(-) >> >> diff --git a/drivers/acpi/acpica/acnamesp.h >> b/drivers/acpi/acpica/acnamesp.h >> index f33a4ba..829672a 100644 >> --- a/drivers/acpi/acpica/acnamesp.h >> +++ b/drivers/acpi/acpica/acnamesp.h >> @@ -130,6 +130,9 @@ acpi_status >> acpi_ns_parse_table(u32 table_index, struct acpi_namespace_node >> *start_node); >> >> acpi_status >> +acpi_ns_execute_table(u32 table_index, struct acpi_namespace_node >> *start_node); >> + >> +acpi_status >> acpi_ns_one_complete_parse(u32 pass_number, >> u32 table_index, >> struct acpi_namespace_node *start_node); >> diff --git a/drivers/acpi/acpica/acparser.h b/drivers/acpi/acpica/acparser.h >> index fc30577..939d411 100644 >> --- a/drivers/acpi/acpica/acparser.h >> +++ b/drivers/acpi/acpica/acparser.h >> @@ -78,6 +78,8 @@ extern const u8 acpi_gbl_long_op_index[]; >> */ >> acpi_status acpi_ps_execute_method(struct acpi_evaluate_info *info); >> >> +acpi_status acpi_ps_execute_table(struct acpi_evaluate_info *info); >> + >> /* >> * psargs - Parse AML opcode arguments >> */ >> diff --git a/drivers/acpi/acpica/evrgnini.c b/drivers/acpi/acpica/evrgnini.c >> index b6ea9c0..3843f1f 100644 >> --- a/drivers/acpi/acpica/evrgnini.c >> +++ b/drivers/acpi/acpica/evrgnini.c >> @@ -553,7 +553,8 @@ acpi_ev_initialize_region(union >> acpi_operand_object *region_obj, >> * >> * See acpi_ns_exec_module_code >> */ >> - if (obj_desc->method. >> + if (!acpi_gbl_parse_table_as_term_list && >> + obj_desc->method. >> info_flags & >> ACPI_METHOD_MODULE_LEVEL) { >> handler_obj = >> obj_desc- >> >method.dispatch.handler; >> diff --git a/drivers/acpi/acpica/exconfig.c b/drivers/acpi/acpica/exconfig.c >> index 21932d6..75679bc 100644 >> --- a/drivers/acpi/acpica/exconfig.c >> +++ b/drivers/acpi/acpica/exconfig.c >> @@ -120,7 +120,8 @@ acpi_ex_add_table(u32 table_index, >> /* Execute any module-level code that was found in the table */ >> >> acpi_ex_exit_interpreter(); >> - if (acpi_gbl_group_module_level_code) { >> + if (!acpi_gbl_parse_table_as_term_list >> + && acpi_gbl_group_module_level_code) { >> acpi_ns_exec_module_code_list(); >> } >> acpi_ex_enter_interpreter(); >> diff --git a/drivers/acpi/acpica/nsload.c b/drivers/acpi/acpica/nsload.c >> index b5e2b0a..2daa9a09 100644 >> --- a/drivers/acpi/acpica/nsload.c >> +++ b/drivers/acpi/acpica/nsload.c >> @@ -162,7 +162,8 @@ unlock: >> * other ACPI implementations. Optionally, the execution can be >> deferred >> * until later, see acpi_initialize_objects. >> */ >> - if (!acpi_gbl_group_module_level_code) { >> + if (!acpi_gbl_parse_table_as_term_list >> + && !acpi_gbl_group_module_level_code) { >> acpi_ns_exec_module_code_list(); >> } >> >> diff --git a/drivers/acpi/acpica/nsparse.c b/drivers/acpi/acpica/nsparse.c >> index 1783cd7..f475889 100644 >> --- a/drivers/acpi/acpica/nsparse.c >> +++ b/drivers/acpi/acpica/nsparse.c >> @@ -54,6 +54,98 @@ ACPI_MODULE_NAME("nsparse") >> >> >> /********************************************************* >> ********************** >> * >> + * FUNCTION: ns_execute_table >> + * >> + * PARAMETERS: table_desc - An ACPI table descriptor for table to >> parse >> + * start_node - Where to enter the table into the namespace >> + * >> + * RETURN: Status >> + * >> + * DESCRIPTION: Load ACPI/AML table by executing the entire table as a >> + * term_list. >> + * >> + >> ********************************************************** >> ********************/ >> +acpi_status >> +acpi_ns_execute_table(u32 table_index, struct acpi_namespace_node >> *start_node) >> +{ >> + acpi_status status; >> + struct acpi_table_header *table; >> + acpi_owner_id owner_id; >> + struct acpi_evaluate_info *info = NULL; >> + u32 aml_length; >> + u8 *aml_start; >> + union acpi_operand_object *method_obj = NULL; >> + >> + ACPI_FUNCTION_TRACE(ns_execute_table); >> + >> + status = acpi_get_table_by_index(table_index, &table); >> + if (ACPI_FAILURE(status)) { >> + return_ACPI_STATUS(status); >> + } >> + >> + /* Table must consist of at least a complete header */ >> + >> + if (table->length < sizeof(struct acpi_table_header)) { >> + return_ACPI_STATUS(AE_BAD_HEADER); >> + } >> + >> + aml_start = (u8 *)table + sizeof(struct acpi_table_header); >> + aml_length = table->length - sizeof(struct acpi_table_header); >> + >> + status = acpi_tb_get_owner_id(table_index, &owner_id); >> + if (ACPI_FAILURE(status)) { >> + return_ACPI_STATUS(status); >> + } >> + >> + /* Create, initialize, and link a new temporary method object */ >> + >> + method_obj = >> acpi_ut_create_internal_object(ACPI_TYPE_METHOD); >> + if (!method_obj) { >> + return_ACPI_STATUS(AE_NO_MEMORY); >> + } >> + >> + /* Allocate the evaluation information block */ >> + >> + info = ACPI_ALLOCATE_ZEROED(sizeof(struct acpi_evaluate_info)); >> + if (!info) { >> + status = AE_NO_MEMORY; >> + goto cleanup; >> + } >> + >> + ACPI_DEBUG_PRINT((ACPI_DB_PARSE, >> + "Create table code block: %p\n", method_obj)); >> + >> + method_obj->method.aml_start = aml_start; >> + method_obj->method.aml_length = aml_length; >> + method_obj->method.owner_id = owner_id; >> + method_obj->method.info_flags |= >> ACPI_METHOD_MODULE_LEVEL; >> + >> + info->pass_number = ACPI_IMODE_EXECUTE; >> + info->node = start_node; >> + info->obj_desc = method_obj; >> + info->node_flags = info->node->flags; >> + info->full_pathname = acpi_ns_get_normalized_pathname(info- >> >node, TRUE); >> + if (!info->full_pathname) { >> + status = AE_NO_MEMORY; >> + goto cleanup; >> + } >> + >> + (void)acpi_ut_release_mutex(ACPI_MTX_NAMESPACE); >> + status = acpi_ps_execute_table(info); >> + (void)acpi_ut_acquire_mutex(ACPI_MTX_NAMESPACE); > [Lv Zheng] > I got a report that we have lock order issue in the previous bug fix: > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=2f38b1b1 > And I'll send a quick fix for that later today. > > After investigation, we found that ACPICA namespace lock design actually requires a big change in order to support this grammar change. > This means that this patch still need to be improved. > And the above 3 lines are currently known to be not compliant to the current ACPICA namespace lock design. > > So please drop this series, and I'll re-send this series after improving the ACPICA namespace lock design. > Dropped, thanks! ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v4 4/5] ACPI 2.0 / AML: Enable correct ACPI subsystem initialization order for new table loading mode 2016-06-21 4:34 ` [PATCH v4 0/5] ACPI 2.0: Enable TermList interpretion for table loading Lv Zheng ` (2 preceding siblings ...) 2016-06-21 4:34 ` [PATCH v4 3/5] ACPICA: ACPI 2.0, Interpreter: Fix MLC issues by switching to new TermList grammar for table loading Lv Zheng @ 2016-06-21 4:34 ` Lv Zheng 2016-06-21 4:34 ` [PATCH v4 5/5] ACPI 2.0 / AML: Fix module level execution by correctly parsing table as TermList Lv Zheng 4 siblings, 0 replies; 40+ messages in thread From: Lv Zheng @ 2016-06-21 4:34 UTC (permalink / raw) To: Rafael J. Wysocki, Rafael J. Wysocki, Len Brown Cc: Lv Zheng, Lv Zheng, linux-kernel, linux-acpi This patch enables the following initialization order for the new table loading mode (which is enabled by setting acpi_gbl_parse_table_as_term_list to TRUE): 1. Install default region handlers (SystemMemory, SystemIo, PciConfig, EmbeddedControl via ECDT) without evaluating _REG; 2. Load the table and execute the module level AML opcodes instantly. Signed-off-by: Lv Zheng <lv.zheng@intel.com> --- drivers/acpi/bus.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c index 262ca31..4582db3 100644 --- a/drivers/acpi/bus.c +++ b/drivers/acpi/bus.c @@ -925,7 +925,8 @@ void __init acpi_early_init(void) goto error0; } - if (acpi_gbl_group_module_level_code) { + if (!acpi_gbl_parse_table_as_term_list && + acpi_gbl_group_module_level_code) { status = acpi_load_tables(); if (ACPI_FAILURE(status)) { printk(KERN_ERR PREFIX @@ -1008,7 +1009,8 @@ static int __init acpi_bus_init(void) status = acpi_ec_ecdt_probe(); /* Ignore result. Not having an ECDT is not fatal. */ - if (!acpi_gbl_group_module_level_code) { + if (acpi_gbl_parse_table_as_term_list || + !acpi_gbl_group_module_level_code) { status = acpi_load_tables(); if (ACPI_FAILURE(status)) { printk(KERN_ERR PREFIX -- 1.7.10 ^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH v4 5/5] ACPI 2.0 / AML: Fix module level execution by correctly parsing table as TermList 2016-06-21 4:34 ` [PATCH v4 0/5] ACPI 2.0: Enable TermList interpretion for table loading Lv Zheng ` (3 preceding siblings ...) 2016-06-21 4:34 ` [PATCH v4 4/5] ACPI 2.0 / AML: Enable correct ACPI subsystem initialization order for new table loading mode Lv Zheng @ 2016-06-21 4:34 ` Lv Zheng 4 siblings, 0 replies; 40+ messages in thread From: Lv Zheng @ 2016-06-21 4:34 UTC (permalink / raw) To: Rafael J. Wysocki, Rafael J. Wysocki, Len Brown Cc: Lv Zheng, Lv Zheng, linux-kernel, linux-acpi This experiment follows de-facto standard behavior, parsing entire table as a single TermList, so that all module level executions are possible during the table loading. If regressions are found against the enabling of this experimental fix, this patch is the only one that should get bisected out. Please report the regressions to the kernel bugzilla for further root causing. Signed-off-by: Lv Zheng <lv.zheng@intel.com> --- include/acpi/acpixf.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h index 22b0397..e21bdd1 100644 --- a/include/acpi/acpixf.h +++ b/include/acpi/acpixf.h @@ -196,9 +196,9 @@ ACPI_INIT_GLOBAL(u8, acpi_gbl_group_module_level_code, FALSE); /* * Optionally support module level code by parsing the entire table as - * a term_list. Default is FALSE, do not execute entire table. + * a term_list. Default is TRUE, do execute entire table. */ -ACPI_INIT_GLOBAL(u8, acpi_gbl_parse_table_as_term_list, FALSE); +ACPI_INIT_GLOBAL(u8, acpi_gbl_parse_table_as_term_list, TRUE); /* * Optionally use 32-bit FADT addresses if and when there is a conflict -- 1.7.10 ^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH v5 0/5] ACPI 2.0: Stop defer-executing module level code [not found] <0e65135af51d94db0410c7059f3bc3a2300fc3b5> ` (2 preceding siblings ...) 2016-06-21 4:34 ` [PATCH v4 0/5] ACPI 2.0: Enable TermList interpretion for table loading Lv Zheng @ 2016-09-23 3:26 ` Lv Zheng 2016-09-23 3:26 ` [PATCH v5 1/5] ACPICA: Tables: Fix "UNLOAD" code path lock issues Lv Zheng ` (5 more replies) 3 siblings, 6 replies; 40+ messages in thread From: Lv Zheng @ 2016-09-23 3:26 UTC (permalink / raw) To: Rafael J. Wysocki, Rafael J. Wysocki, Len Brown Cc: Lv Zheng, Lv Zheng, linux-kernel, linux-acpi After fixing ACPICA internal locking issues, we can enable the correct grammar support for the table loading. The new grammar treats the entire table as TermList rather than ObjectList, thus the module level code should be executed right in place. MLC (module level code) is an ACPICA terminology describing the AML code out of any control method, currently only Type1Opcode (If/Else/While) wrapped MLC code blocks are executed by the AML interpreter after the table loading. But the issue which is fixed by this patchset is: Not only Type1Opcode, but also Type2Opcode will be executed as MLC and MLC is not defer-executed after loading the table, but is executed right in place. The following AML code is assembled into a static loading SSDT, and used as an instrumentation to pry into the de-facto standard AML interpreter behaviors: Name (ECOK, Zero) Scope (\) { DBUG ("TermList 1") If (LEqual (ECOK, Zero)) { DBUG ("TermList 2") Device (MDEV) { DEBUG (TermList 3") If (CondRefOf (MDEV)) { DBUG ("MDEV exists") } If (CondRefOf (MDEV._STA)) { DBUG ("MDEV._STA exists") } If (CondRefOf (\_SB.PCI0.EC)) { DBUG ("\\_SB.PCI0.EC exists") } Name (_HID, EisaId ("PNP9999")) Method (_STA, 0, Serialized) { DEBUG ("\\_SB.MDEV._STA") Return (0x0F) } } DBUG ("TermList 4") } Method (_INI, 0, Serialized) { DBUG ("\\_SB._INI") } } Scope (_SB.PCI0) { Device (EC) { ... } } The DBUG function is a function to write the debugging messages into a SystemIo debug port. Running Windows with the BIOS providing this SSDT via RSDT, the following messages are obtained from the debug port: TermList 1 TermList 2 TermList 3 \_SB.MDEV exists TermList 4 \_SB._INI ... This test reveals the de-facto grammar for the AMLCode to us: 1. During the table loading, MLC will be executed by the interpreter, this is partially supported by the current ACPICA; 2. For SystemIo, not only after the _REG(1, 1) is evaluated (current ACPICA interpreter limitation), but when the table is being loaded, the SystemIo (the debugging port) is accessible, this is recently fixed in the upstream, now all early operation regions are accessible during the table loading; 3. Not only Type1Opcode, but also Type2Opcode will be executed as MLC and MLC is not executed after loading the table, but is executed right in place, the Linux upstream is not compliant to this behavior. The last compliance issue has already been clarified in ACPI 2.0 specification, so the compliance issue is not that Linux is not compliant to the de-facto standard OS, but that Linux is not compliant to ACPI 2.0. Definition block tables in fact is defined by the spec as TermList, which has no difference than the control methods, thus the interpretion of the table should be no difference that the control method evaluation: AMLCode := DefBlockHeader TermList DefMethod := MethodOp PkgLength NameString MethodFlags TermList Now the new upcoming ACPI specification has been clarified around the above grammar primitives, so we need to implement them for Linux. This patchset also contains 2 required fixes generated to fix the regressions detected by the ACPICA ASLTS tool. The ASLTS 'table' test suite has detected such regressions. The fixes have been accepted by the ACPICA upstream (please refer to the ACPICA upstream URL in the description of the 2 patches). And they are sent to Linux community as urgent materials along with the module level code enabling series. With the additional regression fixes, new grammar changes have passed all ASLTS 'table' cases: https://bugs.acpica.org/show_bug.cgi?id=1327 https://github.com/acpica/acpica/pull/177 Lv Zheng (5): ACPICA: Tables: Fix "UNLOAD" code path lock issues ACPICA: Parser: Fix a regression in LoadTable support ACPI 2.0 / AML: Enable correct ACPI subsystem initialization order for new table loading mode ACPI 2.0 / AML: Improve module level execution by moving the If/Else/While execution to per-table basis ACPI 2.0 / AML: Fix module level execution by correctly parsing table as TermList drivers/acpi/acpica/exconfig.c | 23 +++++++++++++++++++---- drivers/acpi/acpica/psxface.c | 11 +++++++++++ drivers/acpi/acpica/tbdata.c | 7 +------ drivers/acpi/acpica/tbxfload.c | 9 ++++++--- drivers/acpi/bus.c | 6 ++++-- include/acpi/acpixf.h | 7 +++---- 6 files changed, 44 insertions(+), 19 deletions(-) -- 1.7.10 ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v5 1/5] ACPICA: Tables: Fix "UNLOAD" code path lock issues 2016-09-23 3:26 ` [PATCH v5 0/5] ACPI 2.0: Stop defer-executing module level code Lv Zheng @ 2016-09-23 3:26 ` Lv Zheng 2016-09-23 3:26 ` [PATCH v5 2/5] ACPICA: Parser: Fix a regression in LoadTable support Lv Zheng ` (4 subsequent siblings) 5 siblings, 0 replies; 40+ messages in thread From: Lv Zheng @ 2016-09-23 3:26 UTC (permalink / raw) To: Rafael J. Wysocki, Rafael J. Wysocki, Len Brown Cc: Lv Zheng, Lv Zheng, linux-kernel, linux-acpi, Bob Moore ACPICA commit 39227380f5b99c51b897a3ffedd88508aa26789b The previous lock fixes didn't cover "Unload" opcode and table unload APIs, this patch fixes lock issues in the "Unload" code path. BZ 1325, Lv Zheng. Link: https://github.com/acpica/acpica/commit/39227380 Link: https://bugs.acpica.org/show_bug.cgi?id=1325 Signed-off-by: Lv Zheng <lv.zheng@intel.com> Signed-off-by: Bob Moore <robert.moore@intel.com> --- drivers/acpi/acpica/exconfig.c | 23 +++++++++++++++++++---- drivers/acpi/acpica/tbdata.c | 7 +------ drivers/acpi/acpica/tbxfload.c | 9 ++++++--- 3 files changed, 26 insertions(+), 13 deletions(-) diff --git a/drivers/acpi/acpica/exconfig.c b/drivers/acpi/acpica/exconfig.c index 421836a..718428b 100644 --- a/drivers/acpi/acpica/exconfig.c +++ b/drivers/acpi/acpica/exconfig.c @@ -532,10 +532,17 @@ acpi_status acpi_ex_unload_table(union acpi_operand_object *ddb_handle) table_index = table_desc->reference.value; + /* + * Release the interpreter lock so that the table lock won't have + * strict order requirement against it. + */ + acpi_ex_exit_interpreter(); + /* Ensure the table is still loaded */ if (!acpi_tb_is_table_loaded(table_index)) { - return_ACPI_STATUS(AE_NOT_EXIST); + status = AE_NOT_EXIST; + goto lock_and_exit; } /* Invoke table handler if present */ @@ -553,16 +560,24 @@ acpi_status acpi_ex_unload_table(union acpi_operand_object *ddb_handle) status = acpi_tb_delete_namespace_by_owner(table_index); if (ACPI_FAILURE(status)) { - return_ACPI_STATUS(status); + goto lock_and_exit; } (void)acpi_tb_release_owner_id(table_index); acpi_tb_set_table_loaded_flag(table_index, FALSE); +lock_and_exit: + + /* Re-acquire the interpreter lock */ + + acpi_ex_enter_interpreter(); + /* * Invalidate the handle. We do this because the handle may be stored * in a named object and may not be actually deleted until much later. */ - ddb_handle->common.flags &= ~AOPOBJ_DATA_VALID; - return_ACPI_STATUS(AE_OK); + if (ACPI_SUCCESS(status)) { + ddb_handle->common.flags &= ~AOPOBJ_DATA_VALID; + } + return_ACPI_STATUS(status); } diff --git a/drivers/acpi/acpica/tbdata.c b/drivers/acpi/acpica/tbdata.c index 7e93fd6..d9ca8c2 100644 --- a/drivers/acpi/acpica/tbdata.c +++ b/drivers/acpi/acpica/tbdata.c @@ -614,17 +614,12 @@ acpi_status acpi_tb_delete_namespace_by_owner(u32 table_index) * lock may block, and also since the execution of a namespace walk * must be allowed to use the interpreter. */ - (void)acpi_ut_release_mutex(ACPI_MTX_INTERPRETER); status = acpi_ut_acquire_write_lock(&acpi_gbl_namespace_rw_lock); - - acpi_ns_delete_namespace_by_owner(owner_id); if (ACPI_FAILURE(status)) { return_ACPI_STATUS(status); } - + acpi_ns_delete_namespace_by_owner(owner_id); acpi_ut_release_write_lock(&acpi_gbl_namespace_rw_lock); - - status = acpi_ut_acquire_mutex(ACPI_MTX_INTERPRETER); return_ACPI_STATUS(status); } diff --git a/drivers/acpi/acpica/tbxfload.c b/drivers/acpi/acpica/tbxfload.c index 870ad64..5569f63 100644 --- a/drivers/acpi/acpica/tbxfload.c +++ b/drivers/acpi/acpica/tbxfload.c @@ -378,9 +378,9 @@ acpi_status acpi_unload_parent_table(acpi_handle object) return_ACPI_STATUS(AE_TYPE); } - /* Must acquire the interpreter lock during this operation */ + /* Must acquire the table lock during this operation */ - status = acpi_ut_acquire_mutex(ACPI_MTX_INTERPRETER); + status = acpi_ut_acquire_mutex(ACPI_MTX_TABLES); if (ACPI_FAILURE(status)) { return_ACPI_STATUS(status); } @@ -407,8 +407,10 @@ acpi_status acpi_unload_parent_table(acpi_handle object) /* Ensure the table is actually loaded */ + (void)acpi_ut_release_mutex(ACPI_MTX_TABLES); if (!acpi_tb_is_table_loaded(i)) { status = AE_NOT_EXIST; + (void)acpi_ut_acquire_mutex(ACPI_MTX_TABLES); break; } @@ -434,10 +436,11 @@ acpi_status acpi_unload_parent_table(acpi_handle object) status = acpi_tb_release_owner_id(i); acpi_tb_set_table_loaded_flag(i, FALSE); + (void)acpi_ut_acquire_mutex(ACPI_MTX_TABLES); break; } - (void)acpi_ut_release_mutex(ACPI_MTX_INTERPRETER); + (void)acpi_ut_release_mutex(ACPI_MTX_TABLES); return_ACPI_STATUS(status); } -- 1.7.10 ^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH v5 2/5] ACPICA: Parser: Fix a regression in LoadTable support 2016-09-23 3:26 ` [PATCH v5 0/5] ACPI 2.0: Stop defer-executing module level code Lv Zheng 2016-09-23 3:26 ` [PATCH v5 1/5] ACPICA: Tables: Fix "UNLOAD" code path lock issues Lv Zheng @ 2016-09-23 3:26 ` Lv Zheng 2016-09-23 3:26 ` [PATCH v5 3/5] ACPI 2.0 / AML: Enable correct ACPI subsystem initialization order for new table loading mode Lv Zheng ` (3 subsequent siblings) 5 siblings, 0 replies; 40+ messages in thread From: Lv Zheng @ 2016-09-23 3:26 UTC (permalink / raw) To: Rafael J. Wysocki, Rafael J. Wysocki, Len Brown Cc: Lv Zheng, Lv Zheng, linux-kernel, linux-acpi, Bob Moore ACPICA commit a78506e0ce8ab1d20db2a055d99cf9143e89eb29 LoadTable allows an alternative RootPathString than the default "\", while the new table execution support fails to keep this logic. This regression can be detected by ASLTS - TLT0.tst4, this patch fixes this regression. Linux upstream is not affected by this regression as we haven't enabled the new table execution support there. BZ 1326, Lv Zheng. Link: https://github.com/acpica/acpica/commit/a78506e0 Link: https://bugs.acpica.org/show_bug.cgi?id=1326 Signed-off-by: Lv Zheng <lv.zheng@intel.com> Signed-off-by: Bob Moore <robert.moore@intel.com> --- drivers/acpi/acpica/psxface.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/drivers/acpi/acpica/psxface.c b/drivers/acpi/acpica/psxface.c index 8d7e5b5..f3c8726 100644 --- a/drivers/acpi/acpica/psxface.c +++ b/drivers/acpi/acpica/psxface.c @@ -305,6 +305,17 @@ acpi_status acpi_ps_execute_table(struct acpi_evaluate_info *info) walk_state->parse_flags |= ACPI_PARSE_MODULE_LEVEL; } + /* Info->Node is the default location to load the table */ + + if (info->node && info->node != acpi_gbl_root_node) { + status = + acpi_ds_scope_stack_push(info->node, ACPI_TYPE_METHOD, + walk_state); + if (ACPI_FAILURE(status)) { + goto cleanup; + } + } + /* * Parse the AML, walk_state will be deleted by parse_aml */ -- 1.7.10 ^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH v5 3/5] ACPI 2.0 / AML: Enable correct ACPI subsystem initialization order for new table loading mode 2016-09-23 3:26 ` [PATCH v5 0/5] ACPI 2.0: Stop defer-executing module level code Lv Zheng 2016-09-23 3:26 ` [PATCH v5 1/5] ACPICA: Tables: Fix "UNLOAD" code path lock issues Lv Zheng 2016-09-23 3:26 ` [PATCH v5 2/5] ACPICA: Parser: Fix a regression in LoadTable support Lv Zheng @ 2016-09-23 3:26 ` Lv Zheng 2016-09-23 3:26 ` [PATCH v5 4/5] ACPI 2.0 / AML: Improve module level execution by moving the If/Else/While execution to per-table basis Lv Zheng ` (2 subsequent siblings) 5 siblings, 0 replies; 40+ messages in thread From: Lv Zheng @ 2016-09-23 3:26 UTC (permalink / raw) To: Rafael J. Wysocki, Rafael J. Wysocki, Len Brown Cc: Lv Zheng, Lv Zheng, linux-kernel, linux-acpi This patch enables the following initialization order for the new table loading mode (which is enabled by setting acpi_gbl_parse_table_as_term_list to TRUE): 1. Install default region handlers (SystemMemory, SystemIo, PciConfig, EmbeddedControl via ECDT) without evaluating _REG; 2. Load the table and execute the module level AML opcodes instantly. Signed-off-by: Lv Zheng <lv.zheng@intel.com> --- drivers/acpi/bus.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c index 85b7d07..658b4c4 100644 --- a/drivers/acpi/bus.c +++ b/drivers/acpi/bus.c @@ -985,7 +985,8 @@ void __init acpi_early_init(void) goto error0; } - if (acpi_gbl_group_module_level_code) { + if (!acpi_gbl_parse_table_as_term_list && + acpi_gbl_group_module_level_code) { status = acpi_load_tables(); if (ACPI_FAILURE(status)) { printk(KERN_ERR PREFIX @@ -1074,7 +1075,8 @@ static int __init acpi_bus_init(void) status = acpi_ec_ecdt_probe(); /* Ignore result. Not having an ECDT is not fatal. */ - if (!acpi_gbl_group_module_level_code) { + if (acpi_gbl_parse_table_as_term_list || + !acpi_gbl_group_module_level_code) { status = acpi_load_tables(); if (ACPI_FAILURE(status)) { printk(KERN_ERR PREFIX -- 1.7.10 ^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH v5 4/5] ACPI 2.0 / AML: Improve module level execution by moving the If/Else/While execution to per-table basis 2016-09-23 3:26 ` [PATCH v5 0/5] ACPI 2.0: Stop defer-executing module level code Lv Zheng ` (2 preceding siblings ...) 2016-09-23 3:26 ` [PATCH v5 3/5] ACPI 2.0 / AML: Enable correct ACPI subsystem initialization order for new table loading mode Lv Zheng @ 2016-09-23 3:26 ` Lv Zheng 2016-11-24 21:16 ` Rafael J. Wysocki 2016-09-23 3:27 ` [PATCH v5 5/5] ACPI 2.0 / AML: Fix module level execution by correctly parsing table as TermList Lv Zheng 2016-09-24 0:32 ` [PATCH v5 0/5] ACPI 2.0: Stop defer-executing module level code Rafael J. Wysocki 5 siblings, 1 reply; 40+ messages in thread From: Lv Zheng @ 2016-09-23 3:26 UTC (permalink / raw) To: Rafael J. Wysocki, Rafael J. Wysocki, Len Brown Cc: Lv Zheng, Lv Zheng, linux-kernel, linux-acpi This reverts commit 00c611def8748a0a1cf1d31842e49b42dfdb3de1. Signed-off-by: Lv Zheng <lv.zheng@intel.com> --- include/acpi/acpixf.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h index c7b3a13..169ec81 100644 --- a/include/acpi/acpixf.h +++ b/include/acpi/acpixf.h @@ -192,7 +192,7 @@ ACPI_INIT_GLOBAL(u8, acpi_gbl_do_not_use_xsdt, FALSE); /* * Optionally support group module level code. */ -ACPI_INIT_GLOBAL(u8, acpi_gbl_group_module_level_code, TRUE); +ACPI_INIT_GLOBAL(u8, acpi_gbl_group_module_level_code, FALSE); /* * Optionally support module level code by parsing the entire table as -- 1.7.10 ^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [PATCH v5 4/5] ACPI 2.0 / AML: Improve module level execution by moving the If/Else/While execution to per-table basis 2016-09-23 3:26 ` [PATCH v5 4/5] ACPI 2.0 / AML: Improve module level execution by moving the If/Else/While execution to per-table basis Lv Zheng @ 2016-11-24 21:16 ` Rafael J. Wysocki 0 siblings, 0 replies; 40+ messages in thread From: Rafael J. Wysocki @ 2016-11-24 21:16 UTC (permalink / raw) To: Lv Zheng Cc: Rafael J. Wysocki, Rafael J. Wysocki, Len Brown, Lv Zheng, Linux Kernel Mailing List, ACPI Devel Maling List On Fri, Sep 23, 2016 at 5:26 AM, Lv Zheng <lv.zheng@intel.com> wrote: > This reverts commit 00c611def8748a0a1cf1d31842e49b42dfdb3de1. It would be good to say why it is now OK to revert it. Thanks, Rafael ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v5 5/5] ACPI 2.0 / AML: Fix module level execution by correctly parsing table as TermList 2016-09-23 3:26 ` [PATCH v5 0/5] ACPI 2.0: Stop defer-executing module level code Lv Zheng ` (3 preceding siblings ...) 2016-09-23 3:26 ` [PATCH v5 4/5] ACPI 2.0 / AML: Improve module level execution by moving the If/Else/While execution to per-table basis Lv Zheng @ 2016-09-23 3:27 ` Lv Zheng 2016-09-24 0:32 ` [PATCH v5 0/5] ACPI 2.0: Stop defer-executing module level code Rafael J. Wysocki 5 siblings, 0 replies; 40+ messages in thread From: Lv Zheng @ 2016-09-23 3:27 UTC (permalink / raw) To: Rafael J. Wysocki, Rafael J. Wysocki, Len Brown Cc: Lv Zheng, Lv Zheng, linux-kernel, linux-acpi This experiment follows de-facto standard behavior, parsing entire table as a single TermList, so that all module level executions are possible during the table loading. If regressions are found against the enabling of this experimental fix, this patch is the only one that should get bisected out. Please report the regressions to the kernel bugzilla for further root causing. Signed-off-by: Lv Zheng <lv.zheng@intel.com> --- include/acpi/acpixf.h | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h index 169ec81..2e22eae 100644 --- a/include/acpi/acpixf.h +++ b/include/acpi/acpixf.h @@ -196,10 +196,9 @@ ACPI_INIT_GLOBAL(u8, acpi_gbl_group_module_level_code, FALSE); /* * Optionally support module level code by parsing the entire table as - * a term_list. Default is FALSE, do not execute entire table until some - * lock order issues are fixed. + * a term_list. Default is TRUE, do execute entire table. */ -ACPI_INIT_GLOBAL(u8, acpi_gbl_parse_table_as_term_list, FALSE); +ACPI_INIT_GLOBAL(u8, acpi_gbl_parse_table_as_term_list, TRUE); /* * Optionally use 32-bit FADT addresses if and when there is a conflict -- 1.7.10 ^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [PATCH v5 0/5] ACPI 2.0: Stop defer-executing module level code 2016-09-23 3:26 ` [PATCH v5 0/5] ACPI 2.0: Stop defer-executing module level code Lv Zheng ` (4 preceding siblings ...) 2016-09-23 3:27 ` [PATCH v5 5/5] ACPI 2.0 / AML: Fix module level execution by correctly parsing table as TermList Lv Zheng @ 2016-09-24 0:32 ` Rafael J. Wysocki 2016-09-26 7:43 ` Zheng, Lv 5 siblings, 1 reply; 40+ messages in thread From: Rafael J. Wysocki @ 2016-09-24 0:32 UTC (permalink / raw) To: Lv Zheng, Len Brown; +Cc: Lv Zheng, linux-kernel, linux-acpi On Friday, September 23, 2016 11:26:26 AM Lv Zheng wrote: > After fixing ACPICA internal locking issues, we can enable the correct > grammar support for the table loading. The new grammar treats the entire > table as TermList rather than ObjectList, thus the module level code should > be executed right in place. > > MLC (module level code) is an ACPICA terminology describing the AML code > out of any control method, currently only Type1Opcode (If/Else/While) > wrapped MLC code blocks are executed by the AML interpreter after the table > loading. But the issue which is fixed by this patchset is: > Not only Type1Opcode, but also Type2Opcode will be executed as MLC and > MLC is not defer-executed after loading the table, but is executed right > in place. > > The following AML code is assembled into a static loading SSDT, and used > as an instrumentation to pry into the de-facto standard AML interpreter > behaviors: > Name (ECOK, Zero) > Scope (\) > { > DBUG ("TermList 1") > If (LEqual (ECOK, Zero)) > { > DBUG ("TermList 2") > Device (MDEV) > { > DEBUG (TermList 3") > If (CondRefOf (MDEV)) > { > DBUG ("MDEV exists") > } > If (CondRefOf (MDEV._STA)) > { > DBUG ("MDEV._STA exists") > } > If (CondRefOf (\_SB.PCI0.EC)) > { > DBUG ("\\_SB.PCI0.EC exists") > } > Name (_HID, EisaId ("PNP9999")) > Method (_STA, 0, Serialized) > { > DEBUG ("\\_SB.MDEV._STA") > Return (0x0F) > } > } > DBUG ("TermList 4") > } > Method (_INI, 0, Serialized) > { > DBUG ("\\_SB._INI") > } > } > Scope (_SB.PCI0) > { > Device (EC) > { > ... > } > } > The DBUG function is a function to write the debugging messages into a > SystemIo debug port. > Running Windows with the BIOS providing this SSDT via RSDT, the following > messages are obtained from the debug port: > TermList 1 > TermList 2 > TermList 3 > \_SB.MDEV exists > TermList 4 > \_SB._INI > ... > > This test reveals the de-facto grammar for the AMLCode to us: > 1. During the table loading, MLC will be executed by the interpreter, this > is partially supported by the current ACPICA; > 2. For SystemIo, not only after the _REG(1, 1) is evaluated (current ACPICA > interpreter limitation), but when the table is being loaded, the > SystemIo (the debugging port) is accessible, this is recently fixed in > the upstream, now all early operation regions are accessible during the > table loading; > 3. Not only Type1Opcode, but also Type2Opcode will be executed as MLC and > MLC is not executed after loading the table, but is executed right in > place, the Linux upstream is not compliant to this behavior. > > The last compliance issue has already been clarified in ACPI 2.0 > specification, so the compliance issue is not that Linux is not compliant > to the de-facto standard OS, but that Linux is not compliant to ACPI 2.0. > Definition block tables in fact is defined by the spec as TermList, which > has no difference than the control methods, thus the interpretion of the > table should be no difference that the control method evaluation: > AMLCode := DefBlockHeader TermList > DefMethod := MethodOp PkgLength NameString MethodFlags TermList > Now the new upcoming ACPI specification has been clarified around the above > grammar primitives, so we need to implement them for Linux. > > This patchset also contains 2 required fixes generated to fix the > regressions detected by the ACPICA ASLTS tool. The ASLTS 'table' test suite > has detected such regressions. The fixes have been accepted by the ACPICA > upstream (please refer to the ACPICA upstream URL in the description of the > 2 patches). And they are sent to Linux community as urgent materials along > with the module level code enabling series. With the additional regression > fixes, new grammar changes have passed all ASLTS 'table' cases: > https://bugs.acpica.org/show_bug.cgi?id=1327 > https://github.com/acpica/acpica/pull/177 > > Lv Zheng (5): > ACPICA: Tables: Fix "UNLOAD" code path lock issues > ACPICA: Parser: Fix a regression in LoadTable support > ACPI 2.0 / AML: Enable correct ACPI subsystem initialization order > for new table loading mode > ACPI 2.0 / AML: Improve module level execution by moving the > If/Else/While execution to per-table basis > ACPI 2.0 / AML: Fix module level execution by correctly parsing table > as TermList [1-2/5] queued up for 4.9. The rest I'd prefer to keep in linux-next for some time before they go in, so let's make them 4.10 material if not urgent for some reasons. Thanks, Rafael ^ permalink raw reply [flat|nested] 40+ messages in thread
* RE: [PATCH v5 0/5] ACPI 2.0: Stop defer-executing module level code 2016-09-24 0:32 ` [PATCH v5 0/5] ACPI 2.0: Stop defer-executing module level code Rafael J. Wysocki @ 2016-09-26 7:43 ` Zheng, Lv 2016-09-26 12:57 ` Rafael J. Wysocki 0 siblings, 1 reply; 40+ messages in thread From: Zheng, Lv @ 2016-09-26 7:43 UTC (permalink / raw) To: Rafael J. Wysocki, Brown, Len; +Cc: Lv Zheng, linux-kernel, linux-acpi Hi, Rafael > From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net] > Subject: Re: [PATCH v5 0/5] ACPI 2.0: Stop defer-executing module level code > > On Friday, September 23, 2016 11:26:26 AM Lv Zheng wrote: > > After fixing ACPICA internal locking issues, we can enable the correct > > grammar support for the table loading. The new grammar treats the entire > > table as TermList rather than ObjectList, thus the module level code should > > be executed right in place. > > > > MLC (module level code) is an ACPICA terminology describing the AML code > > out of any control method, currently only Type1Opcode (If/Else/While) > > wrapped MLC code blocks are executed by the AML interpreter after the table > > loading. But the issue which is fixed by this patchset is: > > Not only Type1Opcode, but also Type2Opcode will be executed as MLC and > > MLC is not defer-executed after loading the table, but is executed right > > in place. > > > > The following AML code is assembled into a static loading SSDT, and used > > as an instrumentation to pry into the de-facto standard AML interpreter > > behaviors: > > Name (ECOK, Zero) > > Scope (\) > > { > > DBUG ("TermList 1") > > If (LEqual (ECOK, Zero)) > > { > > DBUG ("TermList 2") > > Device (MDEV) > > { > > DEBUG (TermList 3") > > If (CondRefOf (MDEV)) > > { > > DBUG ("MDEV exists") > > } > > If (CondRefOf (MDEV._STA)) > > { > > DBUG ("MDEV._STA exists") > > } > > If (CondRefOf (\_SB.PCI0.EC)) > > { > > DBUG ("\\_SB.PCI0.EC exists") > > } > > Name (_HID, EisaId ("PNP9999")) > > Method (_STA, 0, Serialized) > > { > > DEBUG ("\\_SB.MDEV._STA") > > Return (0x0F) > > } > > } > > DBUG ("TermList 4") > > } > > Method (_INI, 0, Serialized) > > { > > DBUG ("\\_SB._INI") > > } > > } > > Scope (_SB.PCI0) > > { > > Device (EC) > > { > > ... > > } > > } > > The DBUG function is a function to write the debugging messages into a > > SystemIo debug port. > > Running Windows with the BIOS providing this SSDT via RSDT, the following > > messages are obtained from the debug port: > > TermList 1 > > TermList 2 > > TermList 3 > > \_SB.MDEV exists > > TermList 4 > > \_SB._INI > > ... > > > > This test reveals the de-facto grammar for the AMLCode to us: > > 1. During the table loading, MLC will be executed by the interpreter, this > > is partially supported by the current ACPICA; > > 2. For SystemIo, not only after the _REG(1, 1) is evaluated (current ACPICA > > interpreter limitation), but when the table is being loaded, the > > SystemIo (the debugging port) is accessible, this is recently fixed in > > the upstream, now all early operation regions are accessible during the > > table loading; > > 3. Not only Type1Opcode, but also Type2Opcode will be executed as MLC and > > MLC is not executed after loading the table, but is executed right in > > place, the Linux upstream is not compliant to this behavior. > > > > The last compliance issue has already been clarified in ACPI 2.0 > > specification, so the compliance issue is not that Linux is not compliant > > to the de-facto standard OS, but that Linux is not compliant to ACPI 2.0. > > Definition block tables in fact is defined by the spec as TermList, which > > has no difference than the control methods, thus the interpretion of the > > table should be no difference that the control method evaluation: > > AMLCode := DefBlockHeader TermList > > DefMethod := MethodOp PkgLength NameString MethodFlags TermList > > Now the new upcoming ACPI specification has been clarified around the above > > grammar primitives, so we need to implement them for Linux. > > > > This patchset also contains 2 required fixes generated to fix the > > regressions detected by the ACPICA ASLTS tool. The ASLTS 'table' test suite > > has detected such regressions. The fixes have been accepted by the ACPICA > > upstream (please refer to the ACPICA upstream URL in the description of the > > 2 patches). And they are sent to Linux community as urgent materials along > > with the module level code enabling series. With the additional regression > > fixes, new grammar changes have passed all ASLTS 'table' cases: > > https://bugs.acpica.org/show_bug.cgi?id=1327 > > https://github.com/acpica/acpica/pull/177 > > > > Lv Zheng (5): > > ACPICA: Tables: Fix "UNLOAD" code path lock issues > > ACPICA: Parser: Fix a regression in LoadTable support > > ACPI 2.0 / AML: Enable correct ACPI subsystem initialization order > > for new table loading mode > > ACPI 2.0 / AML: Improve module level execution by moving the > > If/Else/While execution to per-table basis > > ACPI 2.0 / AML: Fix module level execution by correctly parsing table > > as TermList > > [1-2/5] queued up for 4.9. > > The rest I'd prefer to keep in linux-next for some time before they go in, > so let's make them 4.10 material if not urgent for some reasons. I see. We could have [4-5/5] enabling patch pending for a period in order for more validations. But should we have 3/5 queued up? It's prepared for the 3rd mode of MLC support. So it is a no-op for the current Linux kernel. But having it upstreamed is useful to complete the mode (but not enable it). I've been asked for why ECDT is implemented in the current way. If PATCH 3/5 in the upstream, the reviewer should soon realize what the purpose of the ECDT change is. Thanks and best regards Lv ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v5 0/5] ACPI 2.0: Stop defer-executing module level code 2016-09-26 7:43 ` Zheng, Lv @ 2016-09-26 12:57 ` Rafael J. Wysocki 0 siblings, 0 replies; 40+ messages in thread From: Rafael J. Wysocki @ 2016-09-26 12:57 UTC (permalink / raw) To: Zheng, Lv; +Cc: Brown, Len, Lv Zheng, linux-kernel, linux-acpi On Monday, September 26, 2016 07:43:14 AM Zheng, Lv wrote: > Hi, Rafael > > > From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net] > > Subject: Re: [PATCH v5 0/5] ACPI 2.0: Stop defer-executing module level code > > > > On Friday, September 23, 2016 11:26:26 AM Lv Zheng wrote: > > > After fixing ACPICA internal locking issues, we can enable the correct > > > grammar support for the table loading. The new grammar treats the entire > > > table as TermList rather than ObjectList, thus the module level code should > > > be executed right in place. > > > > > > MLC (module level code) is an ACPICA terminology describing the AML code > > > out of any control method, currently only Type1Opcode (If/Else/While) > > > wrapped MLC code blocks are executed by the AML interpreter after the table > > > loading. But the issue which is fixed by this patchset is: > > > Not only Type1Opcode, but also Type2Opcode will be executed as MLC and > > > MLC is not defer-executed after loading the table, but is executed right > > > in place. > > > > > > The following AML code is assembled into a static loading SSDT, and used > > > as an instrumentation to pry into the de-facto standard AML interpreter > > > behaviors: > > > Name (ECOK, Zero) > > > Scope (\) > > > { > > > DBUG ("TermList 1") > > > If (LEqual (ECOK, Zero)) > > > { > > > DBUG ("TermList 2") > > > Device (MDEV) > > > { > > > DEBUG (TermList 3") > > > If (CondRefOf (MDEV)) > > > { > > > DBUG ("MDEV exists") > > > } > > > If (CondRefOf (MDEV._STA)) > > > { > > > DBUG ("MDEV._STA exists") > > > } > > > If (CondRefOf (\_SB.PCI0.EC)) > > > { > > > DBUG ("\\_SB.PCI0.EC exists") > > > } > > > Name (_HID, EisaId ("PNP9999")) > > > Method (_STA, 0, Serialized) > > > { > > > DEBUG ("\\_SB.MDEV._STA") > > > Return (0x0F) > > > } > > > } > > > DBUG ("TermList 4") > > > } > > > Method (_INI, 0, Serialized) > > > { > > > DBUG ("\\_SB._INI") > > > } > > > } > > > Scope (_SB.PCI0) > > > { > > > Device (EC) > > > { > > > ... > > > } > > > } > > > The DBUG function is a function to write the debugging messages into a > > > SystemIo debug port. > > > Running Windows with the BIOS providing this SSDT via RSDT, the following > > > messages are obtained from the debug port: > > > TermList 1 > > > TermList 2 > > > TermList 3 > > > \_SB.MDEV exists > > > TermList 4 > > > \_SB._INI > > > ... > > > > > > This test reveals the de-facto grammar for the AMLCode to us: > > > 1. During the table loading, MLC will be executed by the interpreter, this > > > is partially supported by the current ACPICA; > > > 2. For SystemIo, not only after the _REG(1, 1) is evaluated (current ACPICA > > > interpreter limitation), but when the table is being loaded, the > > > SystemIo (the debugging port) is accessible, this is recently fixed in > > > the upstream, now all early operation regions are accessible during the > > > table loading; > > > 3. Not only Type1Opcode, but also Type2Opcode will be executed as MLC and > > > MLC is not executed after loading the table, but is executed right in > > > place, the Linux upstream is not compliant to this behavior. > > > > > > The last compliance issue has already been clarified in ACPI 2.0 > > > specification, so the compliance issue is not that Linux is not compliant > > > to the de-facto standard OS, but that Linux is not compliant to ACPI 2.0. > > > Definition block tables in fact is defined by the spec as TermList, which > > > has no difference than the control methods, thus the interpretion of the > > > table should be no difference that the control method evaluation: > > > AMLCode := DefBlockHeader TermList > > > DefMethod := MethodOp PkgLength NameString MethodFlags TermList > > > Now the new upcoming ACPI specification has been clarified around the above > > > grammar primitives, so we need to implement them for Linux. > > > > > > This patchset also contains 2 required fixes generated to fix the > > > regressions detected by the ACPICA ASLTS tool. The ASLTS 'table' test suite > > > has detected such regressions. The fixes have been accepted by the ACPICA > > > upstream (please refer to the ACPICA upstream URL in the description of the > > > 2 patches). And they are sent to Linux community as urgent materials along > > > with the module level code enabling series. With the additional regression > > > fixes, new grammar changes have passed all ASLTS 'table' cases: > > > https://bugs.acpica.org/show_bug.cgi?id=1327 > > > https://github.com/acpica/acpica/pull/177 > > > > > > Lv Zheng (5): > > > ACPICA: Tables: Fix "UNLOAD" code path lock issues > > > ACPICA: Parser: Fix a regression in LoadTable support > > > ACPI 2.0 / AML: Enable correct ACPI subsystem initialization order > > > for new table loading mode > > > ACPI 2.0 / AML: Improve module level execution by moving the > > > If/Else/While execution to per-table basis > > > ACPI 2.0 / AML: Fix module level execution by correctly parsing table > > > as TermList > > > > [1-2/5] queued up for 4.9. > > > > The rest I'd prefer to keep in linux-next for some time before they go in, > > so let's make them 4.10 material if not urgent for some reasons. > > I see. > We could have [4-5/5] enabling patch pending for a period in order for more validations. > But should we have 3/5 queued up? > It's prepared for the 3rd mode of MLC support. > So it is a no-op for the current Linux kernel. > But having it upstreamed is useful to complete the mode (but not enable it). > > I've been asked for why ECDT is implemented in the current way. > If PATCH 3/5 in the upstream, the reviewer should soon realize what the purpose of the ECDT change is. OK, the [3/5] queued up too. Thanks, Rafael ^ permalink raw reply [flat|nested] 40+ messages in thread
end of thread, other threads:[~2016-11-24 21:16 UTC | newest] Thread overview: 40+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <0e65135af51d94db0410c7059f3bc3a2300fc3b5> 2016-05-13 5:35 ` [PATCH v2 0/4] ACPI 2.0: Enable TermList interpretion for table loading Lv Zheng 2016-05-13 5:35 ` [PATCH v2 1/4] ACPICA: Dispatcher: Fix an issue that the opregions created by the linked MLC were not tracked Lv Zheng 2016-05-13 5:35 ` [PATCH v2 2/4] ACPICA: ACPI 2.0, Interpreter: Fix MLC issues by switching to new TermList grammar for table loading Lv Zheng 2016-05-13 5:35 ` [PATCH v2 3/4] ACPI 2.0 / AML: Enable correct ACPI subsystem initialization order for new table loading mode Lv Zheng 2016-05-13 5:35 ` [PATCH v2 4/4] ACPI 2.0 / AML: Fix module level execution by correctly parsing table as TermList Lv Zheng 2016-05-17 0:29 ` [PATCH v2 0/4] ACPI 2.0: Enable TermList interpretion for table loading Zheng, Lv 2016-05-17 23:23 ` Rafael J. Wysocki 2016-05-20 0:57 ` Zheng, Lv 2016-06-10 1:05 ` Rafael J. Wysocki 2016-06-12 1:02 ` Zheng, Lv 2016-06-13 22:59 ` Rafael J. Wysocki 2016-06-20 9:07 ` [PATCH v3 0/5] " Lv Zheng 2016-06-20 9:07 ` [PATCH v3 1/5] ACPICA: Namespace: Fix a regression that MLC support triggers dead lock in dynamic " Lv Zheng 2016-06-20 10:57 ` Mika Westerberg 2016-06-20 23:13 ` Zheng, Lv 2016-06-20 9:07 ` [PATCH v3 2/5] ACPICA: Dispatcher: Fix an issue that the opregions created by the linked MLC were not tracked Lv Zheng 2016-06-20 9:07 ` [PATCH v3 3/5] ACPICA: ACPI 2.0, Interpreter: Fix MLC issues by switching to new TermList grammar for table loading Lv Zheng 2016-06-20 9:07 ` [PATCH v3 4/5] ACPI 2.0 / AML: Enable correct ACPI subsystem initialization order for new table loading mode Lv Zheng 2016-06-20 9:07 ` [PATCH v3 5/5] ACPI 2.0 / AML: Fix module level execution by correctly parsing table as TermList Lv Zheng 2016-06-21 4:34 ` [PATCH v4 0/5] ACPI 2.0: Enable TermList interpretion for table loading Lv Zheng 2016-06-21 4:34 ` [PATCH v4 1/5] ACPICA: Namespace: Fix a regression that MLC support triggers dead lock in dynamic " Lv Zheng 2016-06-21 7:58 ` Mika Westerberg 2016-06-21 9:55 ` Zheng, Lv 2016-06-23 0:41 ` Rafael J. Wysocki 2016-06-21 4:34 ` [PATCH v4 2/5] ACPICA: Dispatcher: Fix an issue that the opregions created by the linked MLC were not tracked Lv Zheng 2016-06-21 4:34 ` [PATCH v4 3/5] ACPICA: ACPI 2.0, Interpreter: Fix MLC issues by switching to new TermList grammar for table loading Lv Zheng 2016-07-04 23:42 ` Zheng, Lv 2016-07-05 0:09 ` Rafael J. Wysocki 2016-06-21 4:34 ` [PATCH v4 4/5] ACPI 2.0 / AML: Enable correct ACPI subsystem initialization order for new table loading mode Lv Zheng 2016-06-21 4:34 ` [PATCH v4 5/5] ACPI 2.0 / AML: Fix module level execution by correctly parsing table as TermList Lv Zheng 2016-09-23 3:26 ` [PATCH v5 0/5] ACPI 2.0: Stop defer-executing module level code Lv Zheng 2016-09-23 3:26 ` [PATCH v5 1/5] ACPICA: Tables: Fix "UNLOAD" code path lock issues Lv Zheng 2016-09-23 3:26 ` [PATCH v5 2/5] ACPICA: Parser: Fix a regression in LoadTable support Lv Zheng 2016-09-23 3:26 ` [PATCH v5 3/5] ACPI 2.0 / AML: Enable correct ACPI subsystem initialization order for new table loading mode Lv Zheng 2016-09-23 3:26 ` [PATCH v5 4/5] ACPI 2.0 / AML: Improve module level execution by moving the If/Else/While execution to per-table basis Lv Zheng 2016-11-24 21:16 ` Rafael J. Wysocki 2016-09-23 3:27 ` [PATCH v5 5/5] ACPI 2.0 / AML: Fix module level execution by correctly parsing table as TermList Lv Zheng 2016-09-24 0:32 ` [PATCH v5 0/5] ACPI 2.0: Stop defer-executing module level code Rafael J. Wysocki 2016-09-26 7:43 ` Zheng, Lv 2016-09-26 12:57 ` Rafael J. Wysocki
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).