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

Ani Sinha posted 1 patch 11 months, 2 weeks ago
Failed in applying to current master (apply log)
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(-)
[PATCH v3] acpi/tests/bios-tables-test: make iasl tool handling simpler
Posted by Ani Sinha 11 months, 2 weeks ago
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>
---
 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
Re: [PATCH v3] acpi/tests/bios-tables-test: make iasl tool handling simpler
Posted by Michael S. Tsirkin 10 months, 1 week ago
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. 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
Re: [PATCH v3] acpi/tests/bios-tables-test: make iasl tool handling simpler
Posted by Ani Sinha 10 months, 1 week ago

> 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",


> 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
> 
Re: [PATCH v3] acpi/tests/bios-tables-test: make iasl tool handling simpler
Posted by Michael S. Tsirkin 10 months, 1 week ago
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
> >
Re: [PATCH v3] acpi/tests/bios-tables-test: make iasl tool handling simpler
Posted by Ani Sinha 10 months, 1 week ago

> 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
Re: [PATCH v3] acpi/tests/bios-tables-test: make iasl tool handling simpler
Posted by Michael S. Tsirkin 10 months, 1 week ago
On Wed, Jun 28, 2023 at 12:05:46PM +0530, Ani Sinha wrote:
> 
> 
> > 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,

Not people. configure script

> 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.

The just the below then? And we can let it be?

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>


diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
index ed1c69cf01..d0e1655d2e 100644
--- a/tests/qtest/bios-tables-test.c
+++ b/tests/qtest/bios-tables-test.c
@@ -102,6 +102,7 @@ typedef struct {
 
 static char disk[] = "tests/acpi-test-disk-XXXXXX";
 static const char *data_dir = "tests/data/acpi";
+/* If you want your own path, change the below to iasl = "/home/usr/bin/iasl" */
 #ifdef CONFIG_IASL
 static const char *iasl = CONFIG_IASL;
 #else
Re: [PATCH v3] acpi/tests/bios-tables-test: make iasl tool handling simpler
Posted by Ani Sinha 10 months, 1 week ago

> On 28-Jun-2023, at 4:24 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> 
> On Wed, Jun 28, 2023 at 12:05:46PM +0530, Ani Sinha wrote:
>> 
>> 
>>> 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,
> 
> Not people. configure script
> 
>> 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.
> 
> The just the below then? And we can let it be?

The original problem I had was that I had to rebuild entire QEMU just so that it picked up the newly installed iasl binary. This does not seem right and I am ok if anyone fixed this in any way they liked. I loved it when previously the test “just worked” when transitioning from no installed iasl to iasl installed in default path. In my opinion, currently what we do with iasl and meson is way too complex and is really not needed for the purpose we use the tool for.

> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> 
> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> index ed1c69cf01..d0e1655d2e 100644
> --- a/tests/qtest/bios-tables-test.c
> +++ b/tests/qtest/bios-tables-test.c
> @@ -102,6 +102,7 @@ typedef struct {
> 
> static char disk[] = "tests/acpi-test-disk-XXXXXX";
> static const char *data_dir = "tests/data/acpi";
> +/* If you want your own path, change the below to iasl = "/home/usr/bin/iasl" */
> #ifdef CONFIG_IASL
> static const char *iasl = CONFIG_IASL;
> #else
Re: [PATCH v3] acpi/tests/bios-tables-test: make iasl tool handling simpler
Posted by Paolo Bonzini 11 months, 2 weeks ago
Il lun 22 mag 2023, 12:30 Ani Sinha <anisinha@redhat.com> ha scritto:

> 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.
>

Since this is only for build time and our only supported Windows
environment is msys, I guess this is good enough.

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo


> [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>
> ---
>  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
>
>