[PATCH 4/5] ACPI RSDP: flesh out the tests to check for full spec compliance.
Alex Hung
alex.hung at canonical.com
Wed Jan 27 03:32:01 UTC 2016
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 at 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)
>
--
Cheers,
Alex Hung
More information about the fwts-devel
mailing list