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

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

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

> 
>> 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@canonical.com>
>>
> 
> Thanks, Colin!
>

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

Commit Message

Comments

Patch