[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3] acpi/tests/bios-tables-test: make iasl tool handling simp
From: |
Michael S. Tsirkin |
Subject: |
Re: [PATCH v3] acpi/tests/bios-tables-test: make iasl tool handling simpler |
Date: |
Mon, 26 Jun 2023 09:19:18 -0400 |
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.
>
> > 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
> >