Re: [PATCH] acpi/tests/bios-tables-test: add an environment variable for iasl location

[Alex Bennée]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Wed, May 17, 2023 at 04:58:06PM +0100, Alex Bennée wrote:
>> 
>> "Michael S. Tsirkin" <mst@redhat.com> writes:
>> 
>> > On Wed, May 17, 2023 at 04:16:47PM +0100, Alex Bennée wrote:
>> >> 
>> >> Ani Sinha <anisinha@redhat.com> writes:
>> >> 
>> >> >> On 17-May-2023, at 8:06 PM, Michael S. Tsirkin <mst@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 <mst@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. 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 a disassembler we trigger for user convenience in case tests
>> > fail. It will never cause tests to fail.
>> 
>> Fair enough. But I still think the place to report it is in configure.
>> Maybe something like:
>> 
>>     iasl                         : /usr/bin/iasl (version 20200925, might 
>> not handle all ACPI)           
>> 
>> in the Host Binaries section. Re-configuring shouldn't cause too much of
>> the build to be regenerated although we could certainly do better in
>> this regard.
>
> won't all of it be regenerated? a header everyone includes changes.

Ahh I see meson is doing:

  if iasl.found()
    config_host_data.set_quoted('CONFIG_IASL', iasl.full_path())
  endif

which causes the inclusion in config-host.h - this seems excessive. It
would seem simpler to get meson to apply CONFIG_IASL to the invocation
of bios-tables-test rather than embedding it in the binary, e.g.:

  ./tests/bios-tables-test --iasl-path ${CONFIG_IASL}

and then you have the best of both worlds. You can run manually with a
different path and you don't need to pollute config-host.h

Paolo,

I see we expand all the qtests with:

    test('qtest-@0@/@1@'.format(target_base, test),
         qtest_executables[test],
         depends: [test_deps, qtest_emulator, emulator_modules],
         env: qtest_env,
         args: ['--tap', '-k'],
         protocol: 'tap',
         timeout: slow_qtests.get(test, 30),
         priority: slow_qtests.get(test, 30),
         suite: ['qtest', 'qtest-' + target_base])

is there any easy way to add arguments to individual tests or do we need
an explicit test expansion for bios-tables-test?

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro