[PATCH 08/21] FADT: minor cleanup and initial compliance tests

[Al Stone]

On 02/16/2016 10:35 PM, Alex Hung wrote:
> On 2016-02-09 09:32 AM, Al Stone wrote:
>> The primary purpose of this patch is to catch some very minor white
>> space edits while adding in two very simple compliance tests -- is the
>> table checksum correct, and is the revision number current?
>>
>> Signed-off-by: Al Stone <al.stone at linaro.org>
>> ---
>>   src/acpi/fadt/fadt.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++----
>>   1 file changed, 47 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/acpi/fadt/fadt.c b/src/acpi/fadt/fadt.c
>> index 5106e4e..6e5ee26 100644
>> --- a/src/acpi/fadt/fadt.c
>> +++ b/src/acpi/fadt/fadt.c
>> @@ -20,8 +20,8 @@
>>    */
>>   #include "fwts.h"
>>
>> -#include <stdlib.h>
>>   #include <stdio.h>
>> +#include <stdlib.h>
>>   #include <sys/types.h>
>>   #include <sys/stat.h>
>>   #ifdef FWTS_ARCH_INTEL
>> @@ -53,7 +53,7 @@ static int fadt_init(fwts_framework *fw)
>>           fwts_log_error(fw, "ACPI table FACP does not exist!");
>>           return FWTS_ERROR;
>>       }
>> -    fadt = (const fwts_acpi_table_fadt*)table->data;
>> +    fadt = (const fwts_acpi_table_fadt *)table->data;
>>       fadt_size = table->length;
>>
>>       /*  Not having a FADT is not a failure on x86 */
>> @@ -152,6 +152,47 @@ static int fadt_info(fwts_framework *fw)
>>       return FWTS_OK;
>>   }
>>
>> +static int fadt_checksum(fwts_framework *fw)
>> +{
>> +    const uint8_t *data = (const uint8_t *)fadt;
>> +    ssize_t length = fadt->header.length;
>> +    uint8_t checksum = 0;
>> +
>> +    /* verify the table checksum */
>> +    checksum = fwts_checksum(data, length);
>> +    if (checksum == 0)
>> +        fwts_passed(fw, "FADT checksum is correct");
>> +    else
>> +        fwts_failed(fw, LOG_LEVEL_MEDIUM,
>> +                "SPECMADTChecksum",
>> +                "FADT checksum is incorrect: 0x%x", checksum);
>> +
>> +    return FWTS_OK;
>> +}
>> +
>> +static int fadt_revision(fwts_framework *fw)
>> +{
>> +    uint8_t major;
>> +    uint8_t minor;
>> +
>> +    major = fadt->header.revision;
>> +    minor = 0;
>> +    if (major >= 5 && fadt->header.length >= 268)
>> +        minor = fadt->minor_version;   /* field added ACPI 5.1 */
>> +
>> +    fwts_log_info(fw, "FADT revision: %d.%d", major, minor);
>> +    fwts_log_info(fw, "FADT table length: %d", fadt->header.length);
>> +
>> +    if (major == 6)
>> +        fwts_passed(fw, "FADT revision is up to date.");
>> +    else
>> +        fwts_failed(fw, LOG_LEVEL_MEDIUM, "SPECFADTRevision",
>> +                "FADT revision is outdated: %d.%d", major, minor);
> 
> If fwts is run on older systems with FADT revision not equal to 6, this
> generates an error even though the systems were cutting-edge at their shipping
> time.  I am not very sure a medium failure is the best choice.
> 
> How about an info or a warning with an advice so either a firmware developer can
> update it or an user can check for a firmware update?
> 
> Any comments from anyone?
>

Hrm.  I'll change this to a warning and an advice.

This does bring up a much more difficult problem that I've been trying to
think through: what I would like to be able to do is something like this:

   # ./fwts --acpicompliance --spec=5.1

and check for to make sure the ACPI tables are 5.1 compliant, or --spec=6.0
or --spec=6.1 .... unfortunately, that seems to touch on a level of complexity
that may ultimately not be useful ...

It would, however, make this particular test clearer.  An FADT version 1 would
be incorrect for the ACPI 6.0 spec, but okay for ACPI 2.0, and so on.  My
biggest concern with this is that some quick sampling of machines shows that
this is _not_ the way firmware vendors -- nor OS developers -- have been using
the ACPI tables historically.  The test results would probably end up far more
confusing than useful.

In the meantime, I'll touch up this test in particular for now.

>> +
>> +    return FWTS_OK;
>> +}
>> +
>> +
>>   static void acpi_table_check_fadt_firmware_control(
>>       fwts_framework *fw,
>>       const fwts_acpi_table_fadt *fadt,
>> @@ -635,8 +676,10 @@ static int fadt_test3(fwts_framework *fw)
>>   }
>>
>>   static fwts_framework_minor_test fadt_tests[] = {
>> -    { fadt_info, "FADT ACPI Description Table flag info." },
>> -    { fadt_test1, "Test FADT ACPI Description Table tests." },
>> +    { fadt_info, "ACPI FADT Description Table flag info." },
>> +    { fadt_checksum, "FADT checksum test." },
>> +    { fadt_revision, "FADT revision test." },
>> +    { fadt_test1, "ACPI FADT Description Table tests." },
>>       { fadt_test2, "Test FADT SCI_EN bit is enabled." },
>>       { fadt_test3, "Test FADT reset register." },
>>       { NULL, NULL }
>>
> 
> 


-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Linaro Enterprise Group
al.stone at linaro.org
-----------------------------------