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

[Alex Hung]

On Thu, Feb 18, 2016 at 5:45 AM, Al Stone <al.stone at linaro.org> wrote:
> 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?
>>

I think the argument --spec=5.1 is too much for this purpose, too.
After all, we will begin seeing more systems shipping for ACPI 6 or
6.1 one soon.  A warning and an advice should work as a good reminder
for firmware developer.

>
> 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
> -----------------------------------
>
> --
> fwts-devel mailing list
> fwts-devel at lists.ubuntu.com
> Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/fwts-devel



-- 
Cheers,
Alex Hung