| Message ID | 1453425260-21576-5-git-send-email-al.stone@linaro.org |
|---|---|
| State | Rejected |
| Headers | show |
On 2016-01-22 09:14 AM, Al Stone wrote: > 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 | 132 +++++++++++++++++++++++++++++++++++++++++++++++---- > 1 file changed, 123 insertions(+), 9 deletions(-) > > diff --git a/src/acpi/rsdp/rsdp.c b/src/acpi/rsdp/rsdp.c > index 5bc0215..0593a47 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. > + */ UEFI and ACPI are independent specs although some of their sections refer to each other. It is possible to use one without each other, example, UEFI + Devicetree and ACPI + something else in theory, but that may not the case in practice. Al, will you be able to share some information thoughts on this topic? > + 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 We usually add an adjective, for example - RSDPBadFirstChecksum, so users know what's wrong with this failure, but there aren't strict rules. > + fwts_failed(fw, LOG_LEVEL_MEDIUM, > + "RSDPFirstChecksum", > + "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,95 @@ 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, > + "RSDPNoAddresses", > + "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) On the transition from 32 bit to 64 bit on x86, some firmware vendors set both many 32-bit and 64-bit addresses equal, and it is later time that ACPI requires one of them to be zero. I personally think 32-bit address = 64-bit address is an acceptable case too. Any comments from anyone? > + fwts_failed(fw, LOG_LEVEL_MEDIUM, > + "RSDPBothAddresses", > + "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 How about "RSDPBadAddresses" since it is actually bad? > + fwts_failed(fw, LOG_LEVEL_MEDIUM, > + "RSDPGoodAddresses", > + "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, How about "RSDPBadSecondChecksum"? > + "RSDPSecondChecksum", > + "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 +204,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) >
On 01/26/2016 08:32 PM, Alex Hung wrote: > On 2016-01-22 09:14 AM, Al Stone wrote: >> 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 | 132 +++++++++++++++++++++++++++++++++++++++++++++++---- >> 1 file changed, 123 insertions(+), 9 deletions(-) >> >> diff --git a/src/acpi/rsdp/rsdp.c b/src/acpi/rsdp/rsdp.c >> index 5bc0215..0593a47 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. >> + */ > UEFI and ACPI are independent specs although some of their sections refer to > each other. It is possible to use one without each other, example, UEFI + > Devicetree and ACPI + something else in theory, but that may not the case in > practice. Absolutely correct. Anything is possible with a bit of hacking :). However, the point here is to try to enforce standards, with some flexibility for the things that have been done in the past. > Al, will you be able to share some information thoughts on this topic? Of course. I based this on section 5.2.5.2 of the ACPI spec, and the notion that ia64 has *always* been EFI, and that arm64, if using ACPI, is required by the SBBR to use UEFI (I believe that's in the Linux kernel documentation, too). There's also a bit of deduction involved. The ACPI spec does not define any other method for locating the RSDP than UEFI for non-x86 machines. So, even if some other mechanism is used, it still would need to provide something like the EFI system table in order for ACPI to find the RSDP. Does that make sense? >> + 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 > We usually add an adjective, for example - RSDPBadFirstChecksum, so users know > what's wrong with this failure, but there aren't strict rules. Ah. Good to know. I'll touch all these up. >> + fwts_failed(fw, LOG_LEVEL_MEDIUM, >> + "RSDPFirstChecksum", >> + "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,95 @@ 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, >> + "RSDPNoAddresses", >> + "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) > On the transition from 32 bit to 64 bit on x86, some firmware vendors set both > many 32-bit and 64-bit addresses equal, and it is later time that ACPI requires > one of them to be zero. I personally think 32-bit address = 64-bit address is > an acceptable case too. Any comments from anyone? I've been wrestling with this one myself. It was not explicit in older versions of the spec but based on discussions with the some of the authors of those versions of the spec, the *intent* was to use one or the other, not both fields -- which is why I put in the change to make it explicit. The reality is that vendors used both fields anyway. Perhaps the test needs to be more specific? For example, maybe it is only a failure on non-x86 machines, and on x86-machines when the FADT has a major version >= 6? >> + fwts_failed(fw, LOG_LEVEL_MEDIUM, >> + "RSDPBothAddresses", >> + "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 > How about "RSDPBadAddresses" since it is actually bad? Sounds good. Thanks. >> + fwts_failed(fw, LOG_LEVEL_MEDIUM, >> + "RSDPGoodAddresses", >> + "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, > How about "RSDPBadSecondChecksum"? ACK. >> + "RSDPSecondChecksum", >> + "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 +204,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) >> > > Thanks for the feedback, Alex!
diff --git a/src/acpi/rsdp/rsdp.c b/src/acpi/rsdp/rsdp.c index 5bc0215..0593a47 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, + "RSDPFirstChecksum", + "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,95 @@ 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, + "RSDPNoAddresses", + "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) + fwts_failed(fw, LOG_LEVEL_MEDIUM, + "RSDPBothAddresses", + "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, + "RSDPGoodAddresses", + "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, + "RSDPSecondChecksum", + "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 +204,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)
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 | 132 +++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 123 insertions(+), 9 deletions(-)