ACK: [PATCH v3 0/6] ACPI compliance testing for MADT and its subtables

Fri Jan 22 01:01:08 UTC 2016

On 01/19/2016 08:44 AM, Colin Ian King wrote:
> On 19/01/16 15:24, Al Stone wrote:
>> On 01/19/2016 05:57 AM, Colin Ian King wrote:
>>> On 19/01/16 00:26, Al Stone wrote:
>>>> This patch series adds in specific ACPI compliance testing for the MADT
>>>> and all of its various subtables (16, currently).
>>>>
>>>> The first three patches add in the idea of host and target architectures --
>>>> host being the arch that FWTS is running on, and target the arch whose 
>>>> firmware is being tested.  This is needed later in the MADT tests since what
>>>> is proper changes based on the architecture the firmware supports.
>>>>
>>>> The fourth patch adds the detailed tests for the MADT and all but one of the
>>>> subtables currently defined in ACPI 6.0.  The last two patches add in the 
>>>> relatively new GIC ITS subtable and compliance tests for it.
>>>>
>>>> There are still multiple TODOs in the compliance checks; these will be
>>>> added as clarification of the spec becomes available.
>>>>
>>>> Changes for v3:
>>>>   -- Add in support for the --arch=<name> parameter to specify the arch
>>>>      for the target firmware (default is that host == target).
>>>>   -- Add in the fwts_architecture typedef plus some helper functions so that
>>>>      tests in the future can adapt their behavior as needed, and so that the
>>>>      MADT tests can set themselves up properly.
>>>>   -- Instead of creating a new sourc file src/acpi/compliance/madt.c, replace
>>>>      the existing src/acpi/madt/madt.c tests since we're providing a superset.
>>>>   -- Various minor style and syntax corrections (from Ian Colin King)
>>>>
>>>> Changes for v2:
>>>>   -- Clean up the white space problems
>>>>   -- Fix errors found by checkpatch (minor syntax things)
>>>>   -- Fix one logic error: while MADT and FADT table revisions *should* be
>>>>      in sync, they seldom are, so report this as a test failure and continue
>>>>      to test as much as possible instead of aborting completely, in some of
>>>>      those cases.
>>>>
>>>>
>>>> Al Stone (6):
>>>>   Start defining FWTS architectures as variables
>>>>   Define some utility functions for using the fwts_architecture enum
>>>>   Add mechanism to tell FWTS what architecture is being tested
>>>>   ACPI: MADT: add in compliance tests for the MADT and subtables
>>>>   ACPI: Add in MADT subtable description for GIC ITS subtable
>>>>   ACPI: MADT: add in compliance checks for the GIC ITS subtable
>>>>
>>>>  src/acpi/madt/madt.c             | 1551 +++++++++++++++++++++++++++++++-------
>>>>  src/lib/include/fwts.h           |    1 +
>>>>  src/lib/include/fwts_acpi.h      |   10 +
>>>>  src/lib/include/fwts_arch.h      |   41 +
>>>>  src/lib/include/fwts_framework.h |    3 +
>>>>  src/lib/src/Makefile.am          |    1 +
>>>>  src/lib/src/fwts_arch.c          |   88 +++
>>>>  src/lib/src/fwts_framework.c     |   25 +
>>>>  8 files changed, 1460 insertions(+), 260 deletions(-)
>>>>  create mode 100644 src/lib/include/fwts_arch.h
>>>>  create mode 100644 src/lib/src/fwts_arch.c
>>>>
>>>
>>> I'm going to bulk-ACK these 6 patches as they do improve the ACPI MADT
>>> checking considerably.  The MADT is such a mess, so this set of tests do
>>> seem to handle all the current combos of specification changes. Just a
>>> few comments:
>>>
>>> 1. Can you send a follow-up patch to update the man page for the new
>>> --arch option.  I'll fix up the fwts wiki accordingly.
>>
>> D'oh.  Of course.  I should have thought of that :(.
>>
>>> 2. The fwts regression tests need updating.  If this patchset gets ACK'd
>>> by the other team members then I'll fix these up for you as it is a
>>> little arcane to do this.
>>
>> Ah, thanks.  I'd be glad to follow along and learn, if I can be of any
>> help.  Is there a pointer to a place to start?
> 
> So you can see the failures by just running:
> 
> make check
> 
> and you see FAILS, e.g:
> 
> FAIL: fwts-test/arg-help-0001/test-0001.sh
> FAIL: fwts-test/arg-help-0001/test-0002.sh
> FAIL: fwts-test/arg-show-tests-0001/test-0001.sh
> FAIL: fwts-test/arg-show-tests-0001/test-0002.sh
> FAIL: fwts-test/arg-show-tests-full-0001/test-0001.sh
> FAIL: fwts-test/madt-0001/test-0001.sh
> FAIL: fwts-test/madt-0001/test-0002.sh
> 
> I use the following recipe: (let us suppose that fwts is in
> /home/king/repos/fwts)
> 
> cd /home/king/repos/fwts
> mkdir /tmp/fwts
> export
> export FWTS=/home/king/repos/fwts/src/fwts
> export TMP=/tmp/fwts
> export FAILURE_LOG=/tmp/fwts/failure.log
> export FWTSTESTDIR=/home/king/repos/fwts/fwts-test
> 
> And now to fix up the arg-help-0001 test
> 
> cd fwts-test/arg-help-0001
> 
> ..and edit test-0001.sh and comment out the line near the end, so change:
> 
> rm $TMPLOG ${TMPLOG}.orig
> 
> to:
> 
> #rm $TMPLOG ${TMPLOG}.orig
> 
> ..so that we don't remove these temp output files at the end of the test
> 
> Now run the test:
> 
> ./test-0001.sh
> FAILED: Test -h option, test-0001.sh
> 
> the /tmp/fwts/failure.log will show you what is missing/different
> between the expected output from the test and what we get with your
> changes.  If you look at the test you will see that the "new" output
> from fwts is dumped into /tmp/fwts/help.log.$$, so I just copy that over
> the original, e.g.:
> 
> cp /tmp/fwts/help.log.32394 arg-help-0001.log
> 
> and running the test again, it should pass:
> 
> ./test-0001.sh
> PASSED: Test -h option, test-0001.sh
> 
> Finally, modify the test to uncomment the rm $TMPLOG ${TMPLOG}.orig line
> and git diff should show the change required to make test-0001.sh run OK
> with your MADT patches:
> 
> git diff
> diff --git a/fwts-test/arg-help-0001/arg-help-0001.log
> b/fwts-test/arg-help-0001/arg-help-0001.log
> index 88eee36..b6ad5e2 100644
> --- a/fwts-test/arg-help-0001/arg-help-0001.log
> +++ b/fwts-test/arg-help-0001/arg-help-0001.log
> @@ -7,6 +7,10 @@
>  --acpitests                  Run general ACPI
>                               tests.
>  -a, --all                    Run all tests.
> +--arch                       Specify arch of the
> +                             tables being tested
> +                             (defaults to current
> +                             host).
>  -b, --batch                  Run non-Interactive
>                               tests.
>  --batch-experimental         Run Batch
> 
> Finally, remove the temp files in /tmp/fwts and then git add that
> arg-help-0001.log and then move on to the next test that failed the
> regression test.
> 
> This is painfully tedious. Hope that's enough info to work with.
> 
> Colin

The recipe worked perfectly -- not too bad to do, but there weren't too
many of them either.  I'll send the patchset to the list in a few minutes.

Thanks!

>>
>>> I've tested this on x86 and arm64 with ACPI tables from x86 and the
>>> --arch x86 option and it looks sane to me.  Passes CoverityScan builds
>>> so, +1 ACK'd from me.
>>
>> Right, and running on x86 with --arch=arm64 works well, conversely.
>>
>>> Thanks Al,
>>>
>>> Acked-by: Colin Ian King <colin.king at canonical.com>
>>>
>>
>> Thanks, Colin!
>>
> 

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