Add in several tests that extend the testing done on the RSDP. If these tests are all passed, the RSDP being examined should be compliant with the ACPI 6.0 spec.
As part of that, when creating a dummy RSDP to substitute for one that cannot be read or found, use either RsdtAddress or XsdtAddress as described in the spec. For non-arm64, either can be used, but for arm64, only the XsdtAddress is to be used.
Finally, update the regression tests for RSDP.
Changes for v2: -- Clean up failure messages to be more like expected messages (per Alex Hung) -- Bodge up both RSDT and XSDT pointers on x86 (Alex Hung)
Al Stone (5): RSDP: minor cleanups to rsdp.c for checkpatch issues utilities: add a helper function for printing out architecture names ACPI tables: when bodging up an RSDP, do the right thing for the arch ACPI RSDP: flesh out the tests to check for full spec compliance. Update regression test rsdp-0001/test-0001.sh
fwts-test/rsdp-0001/rsdp.log | 17 ++++- src/acpi/rsdp/rsdp.c | 158 +++++++++++++++++++++++++++++++++++++---- src/lib/include/fwts_arch.h | 1 + src/lib/src/fwts_acpi_tables.c | 25 ++++--- src/lib/src/fwts_arch.c | 18 +++++ 5 files changed, 195 insertions(+), 24 deletions(-)
Signed-off-by: Al Stone al.stone@linaro.org --- src/acpi/rsdp/rsdp.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/src/acpi/rsdp/rsdp.c b/src/acpi/rsdp/rsdp.c index 423163b..5bc0215 100644 --- a/src/acpi/rsdp/rsdp.c +++ b/src/acpi/rsdp/rsdp.c @@ -13,10 +13,6 @@ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the * GNU General Public License for more details. * - * You should have received a copy of the GNU General Public License - * along with this program; if not, write to the Free Software - * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. - * */ #include "fwts.h"
@@ -47,7 +43,7 @@ static int rsdp_init(fwts_framework *fw) */ static int rsdp_test1(fwts_framework *fw) { - fwts_acpi_table_rsdp *rsdp = (fwts_acpi_table_rsdp*)table->data; + fwts_acpi_table_rsdp *rsdp = (fwts_acpi_table_rsdp *)table->data; bool passed = true; size_t i;
@@ -93,4 +89,5 @@ static fwts_framework_ops rsdp_ops = { .minor_tests = rsdp_tests };
-FWTS_REGISTER("rsdp", &rsdp_ops, FWTS_TEST_ANYTIME, FWTS_FLAG_BATCH | FWTS_FLAG_TEST_ACPI) +FWTS_REGISTER("rsdp", &rsdp_ops, FWTS_TEST_ANYTIME, FWTS_FLAG_BATCH | + FWTS_FLAG_TEST_ACPI)
On 09/02/16 23:12, Al Stone wrote:
Signed-off-by: Al Stone al.stone@linaro.org
src/acpi/rsdp/rsdp.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/src/acpi/rsdp/rsdp.c b/src/acpi/rsdp/rsdp.c index 423163b..5bc0215 100644 --- a/src/acpi/rsdp/rsdp.c +++ b/src/acpi/rsdp/rsdp.c @@ -13,10 +13,6 @@
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- GNU General Public License for more details.
- You should have received a copy of the GNU General Public License
- along with this program; if not, write to the Free Software
- Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/
#include "fwts.h" @@ -47,7 +43,7 @@ static int rsdp_init(fwts_framework *fw) */ static int rsdp_test1(fwts_framework *fw) {
- fwts_acpi_table_rsdp *rsdp = (fwts_acpi_table_rsdp*)table->data;
- fwts_acpi_table_rsdp *rsdp = (fwts_acpi_table_rsdp *)table->data; bool passed = true; size_t i;
@@ -93,4 +89,5 @@ static fwts_framework_ops rsdp_ops = { .minor_tests = rsdp_tests }; -FWTS_REGISTER("rsdp", &rsdp_ops, FWTS_TEST_ANYTIME, FWTS_FLAG_BATCH | FWTS_FLAG_TEST_ACPI) +FWTS_REGISTER("rsdp", &rsdp_ops, FWTS_TEST_ANYTIME, FWTS_FLAG_BATCH |
FWTS_FLAG_TEST_ACPI)
Acked-by: Colin Ian King colin.king@canonical.com
Add in a small helper function that converts the arch enum value to a readable string; this helps with error output.
Signed-off-by: Al Stone al.stone@linaro.org --- src/lib/include/fwts_arch.h | 1 + src/lib/src/fwts_arch.c | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+)
diff --git a/src/lib/include/fwts_arch.h b/src/lib/include/fwts_arch.h index 082e5d3..4862da4 100644 --- a/src/lib/include/fwts_arch.h +++ b/src/lib/include/fwts_arch.h @@ -33,5 +33,6 @@ typedef enum { extern fwts_architecture fwts_arch_get_host(void); extern fwts_architecture fwts_arch_get_arch(const char *name); extern const char *fwts_arch_names(void); +extern const char *fwts_arch_get_name(const fwts_architecture arch);
#endif diff --git a/src/lib/src/fwts_arch.c b/src/lib/src/fwts_arch.c index 6f37e9f..f5e8b78 100644 --- a/src/lib/src/fwts_arch.c +++ b/src/lib/src/fwts_arch.c @@ -33,6 +33,13 @@ static const struct fwts_arch_info arch_info[] = { { FWTS_ARCH_OTHER, "other" } };
+static const struct fwts_arch_info arch_name[] = { + { FWTS_ARCH_X86, "x86" }, + { FWTS_ARCH_IA64, "ia64" }, + { FWTS_ARCH_ARM64, "arm64" }, + { FWTS_ARCH_OTHER, "other" }, +}; + static char *arch_names;
static fwts_architecture __fwts_arch_get_arch(const char *name) @@ -82,3 +89,14 @@ const char *fwts_arch_names(void)
return arch_names; } + +const char *fwts_arch_get_name(const fwts_architecture arch) +{ + const struct fwts_arch_info *ptr; + + for (ptr = arch_name; ptr->arch != FWTS_ARCH_OTHER; ptr++) + if (ptr->arch == arch) + break; + + return ptr->name; +}
If it is necessary to create an RSDP table because there is none that can be read, add in only the RSDT or XSDT pointers as needed. For x86, it can be either, but for arm64 it should only be the XSDT address that is used in the RSDP.
Signed-off-by: Al Stone al.stone@linaro.org --- src/lib/src/fwts_acpi_tables.c | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-)
diff --git a/src/lib/src/fwts_acpi_tables.c b/src/lib/src/fwts_acpi_tables.c index 0191b6b..59fd4ba 100644 --- a/src/lib/src/fwts_acpi_tables.c +++ b/src/lib/src/fwts_acpi_tables.c @@ -1101,14 +1101,23 @@ static int fwts_acpi_load_tables_fixup(fwts_framework *fw) }
/* Now we have all the tables, final fix up is required */ - if (rsdp->rsdt_address != rsdt_fake_addr) { - rsdp->rsdt_address = rsdt_fake_addr; - redo_rsdp_checksum = true; - } - if ((rsdp->revision > 0) && (rsdp->length >= 36) && - (rsdp->xsdt_address != xsdt_fake_addr)) { - rsdp->xsdt_address = xsdt_fake_addr; - redo_rsdp_checksum = true; + if (fw->target_arch == FWTS_ARCH_ARM64) { + if ((rsdp->revision > 0) && (rsdp->length >= 36) && + (rsdp->xsdt_address != xsdt_fake_addr)) { + rsdp->xsdt_address = xsdt_fake_addr; + redo_rsdp_checksum = true; + } + } else { + if (rsdp->rsdt_address != rsdt_fake_addr) { + rsdp->rsdt_address = rsdt_fake_addr; + redo_rsdp_checksum = true; + } + + if ((rsdp->revision > 0) && (rsdp->length >= 36) && + (rsdp->xsdt_address != xsdt_fake_addr)) { + rsdp->xsdt_address = xsdt_fake_addr; + redo_rsdp_checksum = true; + } } /* And update checksum if we've updated the rsdp */ if (redo_rsdp_checksum) {
Add in several more tests to the RSDP code that check for spec details that had not been checked for previously. This then allows us to tag the RSDP tests so that they will also be executed when running FWTS to check ACPI specification compliance (i.e., using the --acpicompliance parameter).
Signed-off-by: Al Stone al.stone@linaro.org --- src/acpi/rsdp/rsdp.c | 151 ++++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 142 insertions(+), 9 deletions(-)
diff --git a/src/acpi/rsdp/rsdp.c b/src/acpi/rsdp/rsdp.c index 5bc0215..bf830f0 100644 --- a/src/acpi/rsdp/rsdp.c +++ b/src/acpi/rsdp/rsdp.c @@ -31,9 +31,28 @@ static int rsdp_init(fwts_framework *fw) fwts_log_error(fw, "Cannot read ACPI tables."); return FWTS_ERROR; } - if (table == NULL || (table && table->length == 0)) { - fwts_log_error(fw, "ACPI RSDP table does not exist, skipping test"); - return FWTS_SKIP; + if (!table) { + /* + * Really old x86 machines may not have an RSDP, but + * ia64 and arm64 are both required to be UEFI, so + * therefore must have an RSDP. + */ + if (fw->target_arch == FWTS_ARCH_X86) + return FWTS_SKIP; + else { + fwts_log_error(fw, + "ACPI RSDP is required for the " + "%s target architecture.", + fwts_arch_get_name(fw->target_arch)); + return FWTS_ERROR; + } + } + + /* We know there is an RSDP now, so do a quick sanity check */ + if (table->length == 0) { + fwts_log_error(fw, + "ACPI RSDP table has zero length"); + return FWTS_ERROR; } return FWTS_OK; } @@ -46,6 +65,18 @@ static int rsdp_test1(fwts_framework *fw) fwts_acpi_table_rsdp *rsdp = (fwts_acpi_table_rsdp *)table->data; bool passed = true; size_t i; + int value; + uint8_t *ptr; + uint8_t checksum; + + /* verify first checksum */ + checksum = fwts_checksum(table->data, 20); + if (checksum == 0) + fwts_passed(fw, "RSDP first checksum is correct"); + else + fwts_failed(fw, LOG_LEVEL_MEDIUM, + "RSDPBadFirstChecksum", + "RSDP first checksum is incorrect: 0x%x", checksum);
for (i = 0; i < 6; i++) { if (!isprint(rsdp->oem_id[i])) { @@ -53,7 +84,11 @@ static int rsdp_test1(fwts_framework *fw) break; } } - if (!passed) { + if (passed) + fwts_passed(fw, + "RSDP: oem_id contains only printable " + "characters."); + else { fwts_failed(fw, LOG_LEVEL_LOW, "RSDPBadOEMId", "RSDP: oem_id contains non-printable " @@ -64,16 +99,114 @@ static int rsdp_test1(fwts_framework *fw) "this should be fixed if possible to make the " "firmware ACPI complaint."); } - if (rsdp->revision > 2) { - passed = false; + + if (rsdp->revision <= 2) + fwts_passed(fw, + "RSDP: revision is %" PRIu8 ".", rsdp->revision); + else fwts_failed(fw, LOG_LEVEL_MEDIUM, "RSDPBadRevisionId", "RSDP: revision is %" PRIu8 ", expected " "value less than 2.", rsdp->revision); - }
+ /* whether RSDT or XSDT depends arch */ + if (rsdp->rsdt_address == 0 && rsdp->xsdt_address == 0) + fwts_failed(fw, LOG_LEVEL_MEDIUM, + "RSDPNoAddressesFound", + "RSDP: either the RsdtAddress must be non-zero " + "or the XsdtAddress must be non-zero. Both " + "fields are zero."); + else + fwts_passed(fw, + "RSDP: at least one of RsdtAddress or XsdtAddress " + "is non-zero."); + + if (rsdp->rsdt_address != 0 && rsdp->xsdt_address != 0) + if ((uint64_t)rsdp->rsdt_address == rsdp->xsdt_address) { + fwts_warning(fw, + "Both RSDT and XSDT addresses are set. " + "Since they are the same address, this " + "is unambiguous to the OS."); + fwts_advice(fw, + "Set only one of the 32-bit RSDT or the " + "64-bit XSDT addresses. Recent versions " + "of the spec require that only one of " + "these be used but as a practical matter, " + "many vendors do use both. If both " + "fields must be used, make sure they at " + "least contain the same value so that " + "the OS can unambiguously determine " + "which address is the correct one."); + } else { + fwts_failed(fw, LOG_LEVEL_MEDIUM, + "RSDPBothAddressesFound", + "RSDP: only one of RsdtAddress or " + "XsdtAddress should be non-zero. Both " + "fields are non-zero."); + } + else + fwts_passed(fw, + "RSDP: only one of RsdtAddress or XsdtAddress " + "is non-zero."); + + passed = false; + switch (fw->target_arch) { + case FWTS_ARCH_X86: + if (rsdp->rsdt_address != 0 || rsdp->xsdt_address != 0) + passed = true; + break; + + case FWTS_ARCH_ARM64: + if (rsdp->xsdt_address != 0) + passed = true; + break; + + default: + passed = true; + fwts_log_advice(fw, + "No clear rule for RSDT/XSDT address usage " + "is provided for the %s architecture.", + fwts_arch_get_name(fw->target_arch)); + } if (passed) - fwts_passed(fw, "No issues found in RSDP table."); + fwts_passed(fw, + "RSDP: the correct RSDT/XSDT address is being " + "used."); + else + fwts_failed(fw, LOG_LEVEL_MEDIUM, + "RSDPBadAddresses", + "RSDP: the wrong RSDT/XSDT address is being " + "used for the %s architecture.", + fwts_arch_get_name(fw->target_arch)); + + /* check the length field */ + if (rsdp->length == 36) + fwts_passed(fw, "RSDP: the table is the correct length."); + else + fwts_failed(fw, LOG_LEVEL_MEDIUM, + "RSDPBadLength", + "RSDP: length is %d bytes but should be 36.", + rsdp->length); + + /* verify the extended checksum */ + checksum = fwts_checksum(table->data, 36); + if (checksum == 0) + fwts_passed(fw, "RSDP second checksum is correct"); + else + fwts_failed(fw, LOG_LEVEL_MEDIUM, + "RSDPBadSecondChecksum", + "RSDP second checksum incorrect: 0x%x", checksum); + + /* the reserved field should be zero */ + value = 0; + for (ptr = (uint8_t *)rsdp->reserved, i = 0; i < 3; i++) + value += *ptr++; + if (value) + fwts_failed(fw, LOG_LEVEL_MEDIUM, + "RSDPReserved", + "RSDP: reserved field is non-zero."); + else + fwts_passed(fw, "RSDP: the reserved field is zero.");
return FWTS_OK; } @@ -90,4 +223,4 @@ static fwts_framework_ops rsdp_ops = { };
FWTS_REGISTER("rsdp", &rsdp_ops, FWTS_TEST_ANYTIME, FWTS_FLAG_BATCH | - FWTS_FLAG_TEST_ACPI) + FWTS_FLAG_TEST_ACPI | FWTS_FLAG_TEST_COMPLIANCE_ACPI)
Signed-off-by: Al Stone al.stone@linaro.org --- fwts-test/rsdp-0001/rsdp.log | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/fwts-test/rsdp-0001/rsdp.log b/fwts-test/rsdp-0001/rsdp.log index 6a3efec..6a59fd0 100644 --- a/fwts-test/rsdp-0001/rsdp.log +++ b/fwts-test/rsdp-0001/rsdp.log @@ -1,9 +1,22 @@ rsdp rsdp: RSDP Root System Description Pointer test. rsdp ---------------------------------------------------------- rsdp Test 1 of 1: RSDP Root System Description Pointer test. -rsdp PASSED: Test 1, No issues found in RSDP table. +rsdp PASSED: Test 1, RSDP first checksum is correct +rsdp PASSED: Test 1, RSDP: oem_id contains only printable +rsdp characters. +rsdp PASSED: Test 1, RSDP: revision is 2. +rsdp PASSED: Test 1, RSDP: at least one of RsdtAddress or +rsdp XsdtAddress is non-zero. +rsdp FAILED [MEDIUM] RSDPBothAddressesFound: Test 1, RSDP: only +rsdp one of RsdtAddress or XsdtAddress should be non-zero. Both +rsdp fields are non-zero. +rsdp PASSED: Test 1, RSDP: the correct RSDT/XSDT address is +rsdp being used. +rsdp PASSED: Test 1, RSDP: the table is the correct length. +rsdp PASSED: Test 1, RSDP second checksum is correct +rsdp PASSED: Test 1, RSDP: the reserved field is zero. rsdp rsdp ========================================================== -rsdp 1 passed, 0 failed, 0 warning, 0 aborted, 0 skipped, 0 +rsdp 8 passed, 1 failed, 0 warning, 0 aborted, 0 skipped, 0 rsdp info only. rsdp ==========================================================
-
Al Stone -
Colin Ian King