> On 17-May-2023, at 8:46 PM, Alex Bennée <alex.ben...@linaro.org> wrote:
>
>
> Ani Sinha <anisi...@redhat.com> writes:
>
>>> On 17-May-2023, at 8:06 PM, Michael S. Tsirkin <m...@redhat.com> wrote:
>>>
>>> On Wed, May 17, 2023 at 07:57:53PM +0530, Ani Sinha wrote:
>>>>
>>>>
>>>>> On 17-May-2023, at 7:47 PM, Michael S. Tsirkin <m...@redhat.com> wrote:
>>>>>
>>>>> On Wed, May 17, 2023 at 05:37:51PM +0530, Ani Sinha wrote:
>>>>>> Currently the meson based QEMU build process locates the iasl binary
>>>>>> from the
>>>>>> current PATH and other locations [1] and uses that to set CONFIG_IASL
>>>>>> which is
>>>>>> then used by the test.
>>>>>>
>>>>>> This has two disadvantages:
>>>>>> - If iasl was not previously installed in the PATH, one has to install
>>>>>> iasl
>>>>>> and rebuild QEMU in order to pick up the iasl location. One cannot simply
>>>>>> use the existing bios-tables-test binary because CONFIG_IASL is only set
>>>>>> during the QEMU build time by meson and then bios-tables-test has to be
>>>>>> rebuilt with CONFIG_IASL set in order to use iasl.
>
> Usually we work the other way by checking at configure time and skipping
> the feature if the prerequisites are not in place.
I think this is too hard a requirement for something that is only used for
debugging aid. If iasl is absent, no big deal. We won’t get nice asl diffs but
the test will fail and let the users know which blobs are in disagreement. For
ACPI contributors and maintainers, in the previous incarnation, one could
quickly install iasl and rerun the same bios-tables-test and get the asl diffs.
Now we need an additional step of recompiling QEMU which is completely useless,
confusing and burdensome.
> We do this with gdb:
>
> ../../configure
> --gdb=/home/alex/src/tools/binutils-gdb.git/builds/all/install/bin/gdb
>
> which checks gdb is at least new enough to support the features we need:
>
> if test -n "$gdb_bin"; then
> gdb_version=$($gdb_bin --version | head -n 1)
> if version_ge ${gdb_version##* } 9.1; then
> echo "HAVE_GDB_BIN=$gdb_bin" >> $config_host_mak
> gdb_arches=$("$source_path/scripts/probe-gdb-support.py" $gdb_bin)
> else
> gdb_bin=""
> fi
> fi
>
>>>>>> - Sometimes, the stock iasl that comes with distributions is simply not
>>>>>> good
>>>>>> enough because it does not support the latest ACPI changes - newly
>>>>>> introduced tables or new table attributes etc. In order to test ACPI code
>>>>>> in QEMU, one has to clone the latest acpica upstream repository and
>>>>>> rebuild iasl in order to get support for it. In those cases, one may want
>>>>>> the test to use the iasl binary from a non-standard location.
>
> I think configure should be checking if iasl is new enough and reporting
> to the user at configure time they need to do something different. We
> don't want to attempt to run tests that will fail unless the user has
> added the right magic to their environment.
Iasl is not used directly for the test. It is used to provide disassembly of
the acpi blobs when the test fails.
What version is new enough? There has been occasions when someone added tables
that are new enough that only the upstream acpica git tree has support for it
and no officially released version does. When we want to check their
implementation in QEMU, we want to use that latest upstream iasl directly
compiled from the git tree.
I think the current mechanism makes lives of ACPI contributors and maintainers
harder, not easier.
>
>>>>>>
>>>>>> In order to overcome the above two disadvantages, we introduce a new
>>>>>> environment variable IASL_PATH that can be set by the tester pointing to
>>>>>> an
>>>>>> possibly non-standard iasl binary location. Bios-tables-test then uses
>>>>>> this
>>>>>> environment variable to set its iasl location, possibly also overriding
>>>>>> the
>>>>>> location that was pointed to by CONFIG_IASL that was set by meson. This
>>>>>> way
>>>>>> developers can not only use this new environment variable to set iasl
>>>>>> location to quickly run bios-tables-test but also can point the test to a
>>>>>> custom iasl if required.
>>>>>>
>>>>>> [1] https://mesonbuild.com/Reference-manual_functions.html#find_program
>>>>>>
>>>>>> Signed-off-by: Ani Sinha <anisi...@redhat.com>
>>>>>
>>>>> Well I think the point was originally that meson can
>>>>> also test the binary in a variety of ways.
>>>>> Never surfaced so maybe never mind.
>>>>>
>>>>> Would it be easier to just look iasl up on path
>>>>
>>>> But that’s what meson is also doing, only QEMU build time.
>>>
>>>
>>> So you were unhappy it's build time because it is not really
>>> part of build and you want flexibility, right?
>>
>> Hmm, maybe in that case, we might want to resurrect iasl_installed(),
>> basically reverting part of cc8fa0e80836c51ba644d910c.
>>
>> To me its ok if I had to set IASL_PATH=`which iasl` before running the
>> test. I do not have strong opinions.
>
> I don't think so - we should be using the tools configure found, after
> all that is its job.
>
> --
> Alex Bennée
> Virtualisation Tech Lead @ Linaro
