Re: [PATCH v3] acpi/tests/bios-tables-test: make iasl tool handling simpler

[Ani Sinha]

> On 26-Jun-2023, at 6:49 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> 
> On Mon, Jun 26, 2023 at 06:33:14PM +0530, Ani Sinha wrote:
>> 
>> 
>>> On 26-Jun-2023, at 6:28 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>> 
>>> On Mon, May 22, 2023 at 04:00:39PM +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 in
>>>> config-host.h header.This is then used at compile time by bios-tables-test 
>>>> to
>>>> set iasl path.
>>>> 
>>>> This has two disadvantages:
>>>> - If iasl was not previously installed in the PATH, one has to install iasl
>>>>  and rebuild QEMU in order to regenerate the header and pick up the found
>>>>  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.
>>>> - 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.
>>>> 
>>>> In order to overcome the above two disadvantages, we set a default iasl 
>>>> path
>>>> as "/usr/bin/iasl". bios-tables-test also checks for the environment 
>>>> variable
>>>> IASL_PATH that can be set by the developer. IASL_PATH passed from the
>>>> environment overrides the default path. This way developers can point
>>>> IASL_PATH environment variable to a possibly a non-standard custom build
>>>> binary and quickly run bios-tables-test without rebuilding. If the default
>>>> path of iasl changes, one simply needs to update the default path and 
>>>> rebuild
>>>> just the test, not whole QEMU.
>>>> 
>>>> [1] https://mesonbuild.com/Reference-manual_functions.html#find_program
>>>> 
>>>> CC: alex.bennee@linaro.org
>>>> CC: pbonzini@redhat.com
>>>> Signed-off-by: Ani Sinha <anisinha@redhat.com>
>>> 
>>> I don't much like environment variables since they are
>>> not discoverable.
>> 
>> I do have this:
>> 
>> +                " Set IASL_PATH environment variable to the path of iasl 
>> binary\n"
>> +                " if iasl is installed somewhere other than %s.\n",
> 
> You only see this if there's a diff.
> 
> And then people stick this in their scripts and are scratching their
> heads trying to figure out why is a wrong iasl running.  Or someone
> comes up with a different use for IASL_PATH and they conflict.

OK in that case I think its ok to simply remove the environment variable part. 
If people are going to be changing a header file, they might as well change the 
DEFAULT_IASL_PATH in the test itself where its easier to find. What additional 
complication meson provides is that it uses find_program() to find the IASL 
binary in a list of predefined locations. I do not think this additional tie up 
with meson is worth it for the niche iasl use case.
Simple is beautiful.

> 
>> 
>>> My preference would be to have
>>> configure output CONFIG_IASL to a new header
>>> (e.g. config-bios-tables-test.h ?)
>>> 
>>> meson then will be smart enough not to rebuild everything
>>> if you just change this singe flag.
>>> 
>>> 
>>>> ---
>>>> meson.build                    | 10 ----------
>>>> meson_options.txt              |  2 --
>>>> scripts/meson-buildoptions.sh  |  2 --
>>>> tests/qtest/bios-tables-test.c | 28 ++++++++++++++++++++--------
>>>> 4 files changed, 20 insertions(+), 22 deletions(-)
>>>> 
>>>> changelog:
>>>> v3: incorporated suggestion from MST. Simplify it even more and
>>>> remove all dependency with meson build system. Set a default iasl
>>>> path which can be overridden by an environment variable.
>>>> 
>>>> v2:
>>>> addressed comments from v1. CONFIG_IASL is now an environment
>>>> variable and no new environment variable is introduced.
>>>> Top level meson.build now does not set CONFIG_IASL in the
>>>> platform header. References to iasl has been removed from other
>>>> files. Test doc is updated. For example:
>>>> 
>>>> "to see ASL diff between mismatched files install IASL, set CONFIG_IASL 
>>>> environment variable to the path of iasl binary,
>>>> and run 'QTEST_QEMU_BINARY=<path to QEMU binary to test> V=1 
>>>> ./tests/qtest/bios-tables-test' from build directory.
>>>> Alternatively run 'V=1 make check-qtest -B' from build dir."
>>>> 
>>>> 
>>>> One drawback of this approach is that meson overrides the values
>>>> of environment variables that are passed from the OS command line
>>>> with the values it sets. So if CONFIG_IASL is passed as an
>>>> env variable by the developer while running "make check-qtest" and
>>>> meson finds iasl in the path, meson will override the value the
>>>> developer provided with the one that it found. I have not seen a
>>>> way to check for OS env from meson.build like we do os.environ.get()
>>>> in python.
>>>> Other than the above, other cases are tested. In absence of iasl,
>>>> the developer can provide their own CONFIG_IASL and path to a custom
>>>> binary and the test picks it up when run from make check-qtest.
>>>> Once iasl is installed, make check-qtest -B will force meson to
>>>> retest iasl path and pass it to the test as an enviroinment.
>>>> When running the test directly, one has to explicitly pass the path
>>>> of iasl in the commnand line as no meson is involved there. There is
>>>> no automatic PATH discovery in the test.
>>>> 
>>>> diff --git a/meson.build b/meson.build
>>>> index 25a4b9f2c1..18c7b669d9 100644
>>>> --- a/meson.build
>>>> +++ b/meson.build
>>>> @@ -179,12 +179,6 @@ if 'dtrace' in get_option('trace_backends')
>>>>  endif
>>>> endif
>>>> 
>>>> -if get_option('iasl') == ''
>>>> -  iasl = find_program('iasl', required: false)
>>>> -else
>>>> -  iasl = find_program(get_option('iasl'), required: true)
>>>> -endif
>>>> -
>>>> ##################
>>>> # Compiler flags #
>>>> ##################
>>>> @@ -1791,9 +1785,6 @@ foreach k : get_option('trace_backends')
>>>> endforeach
>>>> config_host_data.set_quoted('CONFIG_TRACE_FILE', get_option('trace_file'))
>>>> config_host_data.set_quoted('CONFIG_TLS_PRIORITY', 
>>>> get_option('tls_priority'))
>>>> -if iasl.found()
>>>> -  config_host_data.set_quoted('CONFIG_IASL', iasl.full_path())
>>>> -endif
>>>> config_host_data.set_quoted('CONFIG_BINDIR', get_option('prefix') / 
>>>> get_option('bindir'))
>>>> config_host_data.set_quoted('CONFIG_PREFIX', get_option('prefix'))
>>>> config_host_data.set_quoted('CONFIG_QEMU_CONFDIR', get_option('prefix') / 
>>>> qemu_confdir)
>>>> @@ -3761,7 +3752,6 @@ summary_info += {'sphinx-build':      sphinx_build}
>>>> if config_host.has_key('HAVE_GDB_BIN')
>>>>  summary_info += {'gdb':             config_host['HAVE_GDB_BIN']}
>>>> endif
>>>> -summary_info += {'iasl':              iasl}
>>>> summary_info += {'genisoimage':       config_host['GENISOIMAGE']}
>>>> if targetos == 'windows' and have_ga
>>>>  summary_info += {'wixl':            wixl}
>>>> diff --git a/meson_options.txt b/meson_options.txt
>>>> index d8330a1f71..9149df8004 100644
>>>> --- a/meson_options.txt
>>>> +++ b/meson_options.txt
>>>> @@ -14,8 +14,6 @@ option('smbd', type : 'string', value : '',
>>>>       description: 'Path to smbd for slirp networking')
>>>> option('sphinx_build', type : 'string', value : 'sphinx-build',
>>>>       description: 'Use specified sphinx-build for building document')
>>>> -option('iasl', type : 'string', value : '',
>>>> -       description: 'Path to ACPI disassembler')
>>>> option('tls_priority', type : 'string', value : 'NORMAL',
>>>>       description: 'Default TLS protocol/cipher priority string')
>>>> option('default_devices', type : 'boolean', value : true,
>>>> diff --git a/scripts/meson-buildoptions.sh b/scripts/meson-buildoptions.sh
>>>> index 2805d1c145..98ca2e53af 100644
>>>> --- a/scripts/meson-buildoptions.sh
>>>> +++ b/scripts/meson-buildoptions.sh
>>>> @@ -48,7 +48,6 @@ meson_options_help() {
>>>>  printf "%s\n" '                           
>>>> dtrace/ftrace/log/nop/simple/syslog/ust)'
>>>>  printf "%s\n" '  --firmwarepath=VALUES    search PATH for firmware files 
>>>> [share/qemu-'
>>>>  printf "%s\n" '                           firmware]'
>>>> -  printf "%s\n" '  --iasl=VALUE             Path to ACPI disassembler'
>>>>  printf "%s\n" '  --includedir=VALUE       Header file directory [include]'
>>>>  printf "%s\n" '  --interp-prefix=VALUE    where to find shared libraries 
>>>> etc., use %M for'
>>>>  printf "%s\n" '                           cpu name [/usr/gnemul/qemu-%M]'
>>>> @@ -304,7 +303,6 @@ _meson_option_parse() {
>>>>    --disable-hexagon-idef-parser) printf "%s" -Dhexagon_idef_parser=false 
>>>> ;;
>>>>    --enable-hvf) printf "%s" -Dhvf=enabled ;;
>>>>    --disable-hvf) printf "%s" -Dhvf=disabled ;;
>>>> -    --iasl=*) quote_sh "-Diasl=$2" ;;
>>>>    --enable-iconv) printf "%s" -Diconv=enabled ;;
>>>>    --disable-iconv) printf "%s" -Diconv=disabled ;;
>>>>    --includedir=*) quote_sh "-Dincludedir=$2" ;;
>>>> diff --git a/tests/qtest/bios-tables-test.c 
>>>> b/tests/qtest/bios-tables-test.c
>>>> index 7fd88b0e9c..570f2d714d 100644
>>>> --- a/tests/qtest/bios-tables-test.c
>>>> +++ b/tests/qtest/bios-tables-test.c
>>>> @@ -69,6 +69,7 @@
>>>> #define MACHINE_Q35 "q35"
>>>> 
>>>> #define ACPI_REBUILD_EXPECTED_AML "TEST_ACPI_REBUILD_AML"
>>>> +#define DEFAULT_IASL_PATH "/usr/bin/iasl"
>>>> 
>>>> #define OEM_ID             "TEST"
>>>> #define OEM_TABLE_ID       "OEM"
>>>> @@ -102,11 +103,7 @@ typedef struct {
>>>> 
>>>> static char disk[] = "tests/acpi-test-disk-XXXXXX";
>>>> static const char *data_dir = "tests/data/acpi";
>>>> -#ifdef CONFIG_IASL
>>>> -static const char *iasl = CONFIG_IASL;
>>>> -#else
>>>> -static const char *iasl;
>>>> -#endif
>>>> +static const char *iasl = DEFAULT_IASL_PATH;
>>>> 
>>>> static int verbosity_level;
>>>> 
>>>> @@ -441,6 +438,14 @@ static void test_acpi_asl(test_data *data)
>>>>    test_data exp_data = {};
>>>>    gboolean exp_err, err, all_tables_match = true;
>>>> 
>>>> +    if (getenv("IASL_PATH")) {
>>>> +        iasl = getenv("IASL_PATH");
>>>> +    }
>>>> +
>>>> +    if (access(iasl, F_OK | X_OK) < 0) {
>>>> +        iasl = NULL;
>>>> +    }
>>>> +
>>>>    exp_data.tables = load_expected_aml(data);
>>>>    dump_aml_files(data, false);
>>>>    for (i = 0; i < data->tables->len; ++i) {
>>>> @@ -473,6 +478,10 @@ static void test_acpi_asl(test_data *data)
>>>>            continue;
>>>>        }
>>>> 
>>>> +        if (iasl && verbosity_level >= 2) {
>>>> +            fprintf(stderr, "Using iasl: %s\n", iasl);
>>>> +        }
>>>> +
>>>>        err = load_asl(data->tables, sdt);
>>>>        asl = normalize_asl(sdt->asl);
>>>> 
>>>> @@ -528,9 +537,12 @@ static void test_acpi_asl(test_data *data)
>>>>        g_string_free(exp_asl, true);
>>>>    }
>>>>    if (!iasl && !all_tables_match) {
>>>> -        fprintf(stderr, "to see ASL diff between mismatched files install 
>>>> IASL,"
>>>> -                " rebuild QEMU from scratch and re-run tests with V=1"
>>>> -                " environment variable set");
>>>> +        fprintf(stderr, "to see ASL diff between mismatched files 
>>>> install\n"
>>>> +                " IASL & re-run the test with V=1 environment variable 
>>>> set.\n"
>>>> +                " Set IASL_PATH environment variable to the path of iasl 
>>>> binary\n"
>>>> +                " if iasl is installed somewhere other than %s.\n",
>>>> +                DEFAULT_IASL_PATH
>>>> +                );
>>>>    }
>>>>    g_assert(all_tables_match);
>>>> 
>>>> -- 
>>>> 2.39.1