diff mbox series

acpi/tests/bios-tables-test: add an environment variable for iasl location

Message ID 20230517120751.14679-1-anisinha@redhat.com
State New
Headers show
Series acpi/tests/bios-tables-test: add an environment variable for iasl location | expand

Commit Message

Ani Sinha May 17, 2023, 12:07 p.m. UTC 
Currently the meson based QEMU build process locates the iasl binary from the
current PATH and other locations [1] and uses that to set CONFIG_IASL which is
then used by the test.

This has two disadvantages:
 - If iasl was not previously installed in the PATH, one has to install iasl
   and rebuild QEMU in order to pick up the iasl location. One cannot simply
   use the existing bios-tables-test binary because CONFIG_IASL is only set
   during the QEMU build time by meson and then bios-tables-test has to be
   rebuilt with CONFIG_IASL set in order to use iasl.
 - 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 introduce a new
environment variable IASL_PATH that can be set by the tester pointing to an
possibly non-standard iasl binary location. Bios-tables-test then uses this
environment variable to set its iasl location, possibly also overriding the
location that was pointed to by CONFIG_IASL that was set by meson. This way
developers can not only use this new environment variable to set iasl
location to quickly run bios-tables-test but also can point the test to a
custom iasl if required.

[1] https://mesonbuild.com/Reference-manual_functions.html#find_program

Signed-off-by: Ani Sinha <anisinha@redhat.com>
---
 tests/qtest/bios-tables-test.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

sample runs:

$ QTEST_QEMU_BINARY=./qemu-system-x86_64 V=2 ./tests/qtest/bios-tables-test
...
acpi-test: Warning! APIC binary file mismatch. Actual [aml:/tmp/aml-DLHA51], Expected [aml:tests/data/acpi/pc/APIC].
See source file tests/qtest/bios-tables-test.c for instructions on how to update expected files.
Using iasl: /usr/bin/iasl
acpi-test: Warning! APIC mismatch. Actual [asl:/tmp/asl-L9GA51.dsl, aml:/tmp/aml-DLHA51], Expected [asl:/tmp/asl-10EA51.dsl, aml:tests/data/acpi/pc/APIC].

$ QTEST_QEMU_BINARY=./qemu-system-x86_64 V=2 IASL_PATH=/home/anisinha/workspace/acpica/generate/unix/bin/iasl ./tests/qtest/bios-tables-test
...
acpi-test: Warning! APIC binary file mismatch. Actual [aml:/tmp/aml-5CQ341], Expected [aml:tests/data/acpi/pc/APIC].
See source file tests/qtest/bios-tables-test.c for instructions on how to update expected files.
User has provided an iasl path, using that: /home/anisinha/workspace/acpica/generate/unix/bin/iasl
acpi-test: Warning! APIC mismatch. Actual [asl:/tmp/asl-2GQ341.dsl, aml:/tmp/aml-5CQ341], Expected [asl:/tmp/asl-IBR341.dsl, aml:tests/data/acpi/pc/APIC].

Comments

Michael S. Tsirkin May 17, 2023, 2:17 p.m. UTC | #1 
On Wed, May 17, 2023 at 05:37:51PM +0530, Ani Sinha wrote:
> Currently the meson based QEMU build process locates the iasl binary from the
> current PATH and other locations [1] and uses that to set CONFIG_IASL which is
> then used by the test.
> 
> This has two disadvantages:
>  - If iasl was not previously installed in the PATH, one has to install iasl
>    and rebuild QEMU in order to pick up the iasl location. One cannot simply
>    use the existing bios-tables-test binary because CONFIG_IASL is only set
>    during the QEMU build time by meson and then bios-tables-test has to be
>    rebuilt with CONFIG_IASL set in order to use iasl.
>  - 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 introduce a new
> environment variable IASL_PATH that can be set by the tester pointing to an
> possibly non-standard iasl binary location. Bios-tables-test then uses this
> environment variable to set its iasl location, possibly also overriding the
> location that was pointed to by CONFIG_IASL that was set by meson. This way
> developers can not only use this new environment variable to set iasl
> location to quickly run bios-tables-test but also can point the test to a
> custom iasl if required.
> 
> [1] https://mesonbuild.com/Reference-manual_functions.html#find_program
> 
> Signed-off-by: Ani Sinha <anisinha@redhat.com>

Well I think the point was originally that meson can
also test the binary in a variety of ways.
Never surfaced so maybe never mind.

Would it be easier to just look iasl up on path
unless it was specified on command line?

> ---
>  tests/qtest/bios-tables-test.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> sample runs:
> 
> $ QTEST_QEMU_BINARY=./qemu-system-x86_64 V=2 ./tests/qtest/bios-tables-test
> ...
> acpi-test: Warning! APIC binary file mismatch. Actual [aml:/tmp/aml-DLHA51], Expected [aml:tests/data/acpi/pc/APIC].
> See source file tests/qtest/bios-tables-test.c for instructions on how to update expected files.
> Using iasl: /usr/bin/iasl
> acpi-test: Warning! APIC mismatch. Actual [asl:/tmp/asl-L9GA51.dsl, aml:/tmp/aml-DLHA51], Expected [asl:/tmp/asl-10EA51.dsl, aml:tests/data/acpi/pc/APIC].
> 
> $ QTEST_QEMU_BINARY=./qemu-system-x86_64 V=2 IASL_PATH=/home/anisinha/workspace/acpica/generate/unix/bin/iasl ./tests/qtest/bios-tables-test
> ...
> acpi-test: Warning! APIC binary file mismatch. Actual [aml:/tmp/aml-5CQ341], Expected [aml:tests/data/acpi/pc/APIC].
> See source file tests/qtest/bios-tables-test.c for instructions on how to update expected files.
> User has provided an iasl path, using that: /home/anisinha/workspace/acpica/generate/unix/bin/iasl
> acpi-test: Warning! APIC mismatch. Actual [asl:/tmp/asl-2GQ341.dsl, aml:/tmp/aml-5CQ341], Expected [asl:/tmp/asl-IBR341.dsl, aml:tests/data/acpi/pc/APIC].
> 
> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> index 7fd88b0e9c..37e8e484cb 100644
> --- a/tests/qtest/bios-tables-test.c
> +++ b/tests/qtest/bios-tables-test.c
> @@ -440,7 +440,12 @@ static void test_acpi_asl(test_data *data)
>      AcpiSdtTable *sdt, *exp_sdt;
>      test_data exp_data = {};
>      gboolean exp_err, err, all_tables_match = true;
> +    const char *user_iasl_path = getenv("IASL_PATH");
>  
> +    /* if user has provided a path to iasl, use that */
> +    if (user_iasl_path) {
> +        iasl = user_iasl_path;
> +    }
>      exp_data.tables = load_expected_aml(data);
>      dump_aml_files(data, false);
>      for (i = 0; i < data->tables->len; ++i) {
> @@ -473,6 +478,15 @@ static void test_acpi_asl(test_data *data)
>              continue;
>          }
>  
> +        if (verbosity_level >= 2) {
> +            if (user_iasl_path) {
> +                fprintf(stderr, "User has provided an iasl path," \
> +                        "using that: %s\n", user_iasl_path);
> +            } else {
> +                fprintf(stderr, "Using iasl: %s\n", iasl);
> +            }
> +        }
> +
>          err = load_asl(data->tables, sdt);
>          asl = normalize_asl(sdt->asl);
>  
> -- 
> 2.39.1
Ani Sinha May 17, 2023, 2:27 p.m. UTC | #2 
> On 17-May-2023, at 7:47 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> 
> On Wed, May 17, 2023 at 05:37:51PM +0530, Ani Sinha wrote:
>> Currently the meson based QEMU build process locates the iasl binary from the
>> current PATH and other locations [1] and uses that to set CONFIG_IASL which is
>> then used by the test.
>> 
>> This has two disadvantages:
>> - If iasl was not previously installed in the PATH, one has to install iasl
>>   and rebuild QEMU in order to pick up the iasl location. One cannot simply
>>   use the existing bios-tables-test binary because CONFIG_IASL is only set
>>   during the QEMU build time by meson and then bios-tables-test has to be
>>   rebuilt with CONFIG_IASL set in order to use iasl.
>> - 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 introduce a new
>> environment variable IASL_PATH that can be set by the tester pointing to an
>> possibly non-standard iasl binary location. Bios-tables-test then uses this
>> environment variable to set its iasl location, possibly also overriding the
>> location that was pointed to by CONFIG_IASL that was set by meson. This way
>> developers can not only use this new environment variable to set iasl
>> location to quickly run bios-tables-test but also can point the test to a
>> custom iasl if required.
>> 
>> [1] https://mesonbuild.com/Reference-manual_functions.html#find_program
>> 
>> Signed-off-by: Ani Sinha <anisinha@redhat.com>
> 
> Well I think the point was originally that meson can
> also test the binary in a variety of ways.
> Never surfaced so maybe never mind.
> 
> Would it be easier to just look iasl up on path

But that’s what meson is also doing, only QEMU build time.

> unless it was specified on command line?
> 
>> ---
>> tests/qtest/bios-tables-test.c | 14 ++++++++++++++
>> 1 file changed, 14 insertions(+)
>> 
>> sample runs:
>> 
>> $ QTEST_QEMU_BINARY=./qemu-system-x86_64 V=2 ./tests/qtest/bios-tables-test
>> ...
>> acpi-test: Warning! APIC binary file mismatch. Actual [aml:/tmp/aml-DLHA51], Expected [aml:tests/data/acpi/pc/APIC].
>> See source file tests/qtest/bios-tables-test.c for instructions on how to update expected files.
>> Using iasl: /usr/bin/iasl
>> acpi-test: Warning! APIC mismatch. Actual [asl:/tmp/asl-L9GA51.dsl, aml:/tmp/aml-DLHA51], Expected [asl:/tmp/asl-10EA51.dsl, aml:tests/data/acpi/pc/APIC].
>> 
>> $ QTEST_QEMU_BINARY=./qemu-system-x86_64 V=2 IASL_PATH=/home/anisinha/workspace/acpica/generate/unix/bin/iasl ./tests/qtest/bios-tables-test
>> ...
>> acpi-test: Warning! APIC binary file mismatch. Actual [aml:/tmp/aml-5CQ341], Expected [aml:tests/data/acpi/pc/APIC].
>> See source file tests/qtest/bios-tables-test.c for instructions on how to update expected files.
>> User has provided an iasl path, using that: /home/anisinha/workspace/acpica/generate/unix/bin/iasl
>> acpi-test: Warning! APIC mismatch. Actual [asl:/tmp/asl-2GQ341.dsl, aml:/tmp/aml-5CQ341], Expected [asl:/tmp/asl-IBR341.dsl, aml:tests/data/acpi/pc/APIC].
>> 
>> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
>> index 7fd88b0e9c..37e8e484cb 100644
>> --- a/tests/qtest/bios-tables-test.c
>> +++ b/tests/qtest/bios-tables-test.c
>> @@ -440,7 +440,12 @@ static void test_acpi_asl(test_data *data)
>>     AcpiSdtTable *sdt, *exp_sdt;
>>     test_data exp_data = {};
>>     gboolean exp_err, err, all_tables_match = true;
>> +    const char *user_iasl_path = getenv("IASL_PATH");
>> 
>> +    /* if user has provided a path to iasl, use that */
>> +    if (user_iasl_path) {
>> +        iasl = user_iasl_path;
>> +    }
>>     exp_data.tables = load_expected_aml(data);
>>     dump_aml_files(data, false);
>>     for (i = 0; i < data->tables->len; ++i) {
>> @@ -473,6 +478,15 @@ static void test_acpi_asl(test_data *data)
>>             continue;
>>         }
>> 
>> +        if (verbosity_level >= 2) {
>> +            if (user_iasl_path) {
>> +                fprintf(stderr, "User has provided an iasl path," \
>> +                        "using that: %s\n", user_iasl_path);
>> +            } else {
>> +                fprintf(stderr, "Using iasl: %s\n", iasl);
>> +            }
>> +        }
>> +
>>         err = load_asl(data->tables, sdt);
>>         asl = normalize_asl(sdt->asl);
>> 
>> -- 
>> 2.39.1
Michael S. Tsirkin May 17, 2023, 2:36 p.m. UTC | #3 
On Wed, May 17, 2023 at 07:57:53PM +0530, Ani Sinha wrote:
> 
> 
> > On 17-May-2023, at 7:47 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > 
> > On Wed, May 17, 2023 at 05:37:51PM +0530, Ani Sinha wrote:
> >> Currently the meson based QEMU build process locates the iasl binary from the
> >> current PATH and other locations [1] and uses that to set CONFIG_IASL which is
> >> then used by the test.
> >> 
> >> This has two disadvantages:
> >> - If iasl was not previously installed in the PATH, one has to install iasl
> >>   and rebuild QEMU in order to pick up the iasl location. One cannot simply
> >>   use the existing bios-tables-test binary because CONFIG_IASL is only set
> >>   during the QEMU build time by meson and then bios-tables-test has to be
> >>   rebuilt with CONFIG_IASL set in order to use iasl.
> >> - 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 introduce a new
> >> environment variable IASL_PATH that can be set by the tester pointing to an
> >> possibly non-standard iasl binary location. Bios-tables-test then uses this
> >> environment variable to set its iasl location, possibly also overriding the
> >> location that was pointed to by CONFIG_IASL that was set by meson. This way
> >> developers can not only use this new environment variable to set iasl
> >> location to quickly run bios-tables-test but also can point the test to a
> >> custom iasl if required.
> >> 
> >> [1] https://mesonbuild.com/Reference-manual_functions.html#find_program
> >> 
> >> Signed-off-by: Ani Sinha <anisinha@redhat.com>
> > 
> > Well I think the point was originally that meson can
> > also test the binary in a variety of ways.
> > Never surfaced so maybe never mind.
> > 
> > Would it be easier to just look iasl up on path
> 
> But that’s what meson is also doing, only QEMU build time.


So you were unhappy it's build time because it is not really
part of build and you want flexibility, right?


> > unless it was specified on command line?
> > 
> >> ---
> >> tests/qtest/bios-tables-test.c | 14 ++++++++++++++
> >> 1 file changed, 14 insertions(+)
> >> 
> >> sample runs:
> >> 
> >> $ QTEST_QEMU_BINARY=./qemu-system-x86_64 V=2 ./tests/qtest/bios-tables-test
> >> ...
> >> acpi-test: Warning! APIC binary file mismatch. Actual [aml:/tmp/aml-DLHA51], Expected [aml:tests/data/acpi/pc/APIC].
> >> See source file tests/qtest/bios-tables-test.c for instructions on how to update expected files.
> >> Using iasl: /usr/bin/iasl
> >> acpi-test: Warning! APIC mismatch. Actual [asl:/tmp/asl-L9GA51.dsl, aml:/tmp/aml-DLHA51], Expected [asl:/tmp/asl-10EA51.dsl, aml:tests/data/acpi/pc/APIC].
> >> 
> >> $ QTEST_QEMU_BINARY=./qemu-system-x86_64 V=2 IASL_PATH=/home/anisinha/workspace/acpica/generate/unix/bin/iasl ./tests/qtest/bios-tables-test
> >> ...
> >> acpi-test: Warning! APIC binary file mismatch. Actual [aml:/tmp/aml-5CQ341], Expected [aml:tests/data/acpi/pc/APIC].
> >> See source file tests/qtest/bios-tables-test.c for instructions on how to update expected files.
> >> User has provided an iasl path, using that: /home/anisinha/workspace/acpica/generate/unix/bin/iasl
> >> acpi-test: Warning! APIC mismatch. Actual [asl:/tmp/asl-2GQ341.dsl, aml:/tmp/aml-5CQ341], Expected [asl:/tmp/asl-IBR341.dsl, aml:tests/data/acpi/pc/APIC].
> >> 
> >> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> >> index 7fd88b0e9c..37e8e484cb 100644
> >> --- a/tests/qtest/bios-tables-test.c
> >> +++ b/tests/qtest/bios-tables-test.c
> >> @@ -440,7 +440,12 @@ static void test_acpi_asl(test_data *data)
> >>     AcpiSdtTable *sdt, *exp_sdt;
> >>     test_data exp_data = {};
> >>     gboolean exp_err, err, all_tables_match = true;
> >> +    const char *user_iasl_path = getenv("IASL_PATH");
> >> 
> >> +    /* if user has provided a path to iasl, use that */
> >> +    if (user_iasl_path) {
> >> +        iasl = user_iasl_path;
> >> +    }
> >>     exp_data.tables = load_expected_aml(data);
> >>     dump_aml_files(data, false);
> >>     for (i = 0; i < data->tables->len; ++i) {
> >> @@ -473,6 +478,15 @@ static void test_acpi_asl(test_data *data)
> >>             continue;
> >>         }
> >> 
> >> +        if (verbosity_level >= 2) {
> >> +            if (user_iasl_path) {
> >> +                fprintf(stderr, "User has provided an iasl path," \
> >> +                        "using that: %s\n", user_iasl_path);
> >> +            } else {
> >> +                fprintf(stderr, "Using iasl: %s\n", iasl);
> >> +            }
> >> +        }
> >> +
> >>         err = load_asl(data->tables, sdt);
> >>         asl = normalize_asl(sdt->asl);
> >> 
> >> -- 
> >> 2.39.1
Ani Sinha May 17, 2023, 2:49 p.m. UTC | #4 
> On 17-May-2023, at 8:06 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> 
> On Wed, May 17, 2023 at 07:57:53PM +0530, Ani Sinha wrote:
>> 
>> 
>>> On 17-May-2023, at 7:47 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>> 
>>> On Wed, May 17, 2023 at 05:37:51PM +0530, Ani Sinha wrote:
>>>> Currently the meson based QEMU build process locates the iasl binary from the
>>>> current PATH and other locations [1] and uses that to set CONFIG_IASL which is
>>>> then used by the test.
>>>> 
>>>> This has two disadvantages:
>>>> - If iasl was not previously installed in the PATH, one has to install iasl
>>>>  and rebuild QEMU in order to pick up the iasl location. One cannot simply
>>>>  use the existing bios-tables-test binary because CONFIG_IASL is only set
>>>>  during the QEMU build time by meson and then bios-tables-test has to be
>>>>  rebuilt with CONFIG_IASL set in order to use iasl.
>>>> - 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 introduce a new
>>>> environment variable IASL_PATH that can be set by the tester pointing to an
>>>> possibly non-standard iasl binary location. Bios-tables-test then uses this
>>>> environment variable to set its iasl location, possibly also overriding the
>>>> location that was pointed to by CONFIG_IASL that was set by meson. This way
>>>> developers can not only use this new environment variable to set iasl
>>>> location to quickly run bios-tables-test but also can point the test to a
>>>> custom iasl if required.
>>>> 
>>>> [1] https://mesonbuild.com/Reference-manual_functions.html#find_program
>>>> 
>>>> Signed-off-by: Ani Sinha <anisinha@redhat.com>
>>> 
>>> Well I think the point was originally that meson can
>>> also test the binary in a variety of ways.
>>> Never surfaced so maybe never mind.
>>> 
>>> Would it be easier to just look iasl up on path
>> 
>> But that’s what meson is also doing, only QEMU build time.
> 
> 
> So you were unhappy it's build time because it is not really
> part of build and you want flexibility, right?

Hmm, maybe in that case, we might want to resurrect iasl_installed(), basically reverting part of cc8fa0e80836c51ba644d910c.

To me its ok if I had to set IASL_PATH=`which iasl` before running the test. I do not have strong opinions.
 
> 
> 
>>> unless it was specified on command line?
>>> 
>>>> ---
>>>> tests/qtest/bios-tables-test.c | 14 ++++++++++++++
>>>> 1 file changed, 14 insertions(+)
>>>> 
>>>> sample runs:
>>>> 
>>>> $ QTEST_QEMU_BINARY=./qemu-system-x86_64 V=2 ./tests/qtest/bios-tables-test
>>>> ...
>>>> acpi-test: Warning! APIC binary file mismatch. Actual [aml:/tmp/aml-DLHA51], Expected [aml:tests/data/acpi/pc/APIC].
>>>> See source file tests/qtest/bios-tables-test.c for instructions on how to update expected files.
>>>> Using iasl: /usr/bin/iasl
>>>> acpi-test: Warning! APIC mismatch. Actual [asl:/tmp/asl-L9GA51.dsl, aml:/tmp/aml-DLHA51], Expected [asl:/tmp/asl-10EA51.dsl, aml:tests/data/acpi/pc/APIC].
>>>> 
>>>> $ QTEST_QEMU_BINARY=./qemu-system-x86_64 V=2 IASL_PATH=/home/anisinha/workspace/acpica/generate/unix/bin/iasl ./tests/qtest/bios-tables-test
>>>> ...
>>>> acpi-test: Warning! APIC binary file mismatch. Actual [aml:/tmp/aml-5CQ341], Expected [aml:tests/data/acpi/pc/APIC].
>>>> See source file tests/qtest/bios-tables-test.c for instructions on how to update expected files.
>>>> User has provided an iasl path, using that: /home/anisinha/workspace/acpica/generate/unix/bin/iasl
>>>> acpi-test: Warning! APIC mismatch. Actual [asl:/tmp/asl-2GQ341.dsl, aml:/tmp/aml-5CQ341], Expected [asl:/tmp/asl-IBR341.dsl, aml:tests/data/acpi/pc/APIC].
>>>> 
>>>> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
>>>> index 7fd88b0e9c..37e8e484cb 100644
>>>> --- a/tests/qtest/bios-tables-test.c
>>>> +++ b/tests/qtest/bios-tables-test.c
>>>> @@ -440,7 +440,12 @@ static void test_acpi_asl(test_data *data)
>>>>    AcpiSdtTable *sdt, *exp_sdt;
>>>>    test_data exp_data = {};
>>>>    gboolean exp_err, err, all_tables_match = true;
>>>> +    const char *user_iasl_path = getenv("IASL_PATH");
>>>> 
>>>> +    /* if user has provided a path to iasl, use that */
>>>> +    if (user_iasl_path) {
>>>> +        iasl = user_iasl_path;
>>>> +    }
>>>>    exp_data.tables = load_expected_aml(data);
>>>>    dump_aml_files(data, false);
>>>>    for (i = 0; i < data->tables->len; ++i) {
>>>> @@ -473,6 +478,15 @@ static void test_acpi_asl(test_data *data)
>>>>            continue;
>>>>        }
>>>> 
>>>> +        if (verbosity_level >= 2) {
>>>> +            if (user_iasl_path) {
>>>> +                fprintf(stderr, "User has provided an iasl path," \
>>>> +                        "using that: %s\n", user_iasl_path);
>>>> +            } else {
>>>> +                fprintf(stderr, "Using iasl: %s\n", iasl);
>>>> +            }
>>>> +        }
>>>> +
>>>>        err = load_asl(data->tables, sdt);
>>>>        asl = normalize_asl(sdt->asl);
>>>> 
>>>> -- 
>>>> 2.39.1
>
Alex Bennée May 17, 2023, 3:16 p.m. UTC | #5 
Ani Sinha <anisinha@redhat.com> writes:

>> On 17-May-2023, at 8:06 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> 
>> On Wed, May 17, 2023 at 07:57:53PM +0530, Ani Sinha wrote:
>>> 
>>> 
>>>> On 17-May-2023, at 7:47 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>> 
>>>> On Wed, May 17, 2023 at 05:37:51PM +0530, Ani Sinha wrote:
>>>>> Currently the meson based QEMU build process locates the iasl binary from the
>>>>> current PATH and other locations [1] and uses that to set CONFIG_IASL which is
>>>>> then used by the test.
>>>>> 
>>>>> This has two disadvantages:
>>>>> - If iasl was not previously installed in the PATH, one has to install iasl
>>>>>  and rebuild QEMU in order to pick up the iasl location. One cannot simply
>>>>>  use the existing bios-tables-test binary because CONFIG_IASL is only set
>>>>>  during the QEMU build time by meson and then bios-tables-test has to be
>>>>>  rebuilt with CONFIG_IASL set in order to use iasl.

Usually we work the other way by checking at configure time and skipping
the feature if the prerequisites are not in place. We do this with gdb:

  ../../configure --gdb=/home/alex/src/tools/binutils-gdb.git/builds/all/install/bin/gdb

which checks gdb is at least new enough to support the features we need:

  if test -n "$gdb_bin"; then
      gdb_version=$($gdb_bin --version | head -n 1)
      if version_ge ${gdb_version##* } 9.1; then
          echo "HAVE_GDB_BIN=$gdb_bin" >> $config_host_mak
          gdb_arches=$("$source_path/scripts/probe-gdb-support.py" $gdb_bin)
      else
          gdb_bin=""
      fi
  fi

>>>>> - Sometimes, the stock iasl that comes with distributions is simply not good
>>>>>  enough because it does not support the latest ACPI changes - newly
>>>>>  introduced tables or new table attributes etc. In order to test ACPI code
>>>>>  in QEMU, one has to clone the latest acpica upstream repository and
>>>>>  rebuild iasl in order to get support for it. In those cases, one may want
>>>>>  the test to use the iasl binary from a non-standard location.

I think configure should be checking if iasl is new enough and reporting
to the user at configure time they need to do something different. We
don't want to attempt to run tests that will fail unless the user has
added the right magic to their environment.

>>>>> 
>>>>> In order to overcome the above two disadvantages, we introduce a new
>>>>> environment variable IASL_PATH that can be set by the tester pointing to an
>>>>> possibly non-standard iasl binary location. Bios-tables-test then uses this
>>>>> environment variable to set its iasl location, possibly also overriding the
>>>>> location that was pointed to by CONFIG_IASL that was set by meson. This way
>>>>> developers can not only use this new environment variable to set iasl
>>>>> location to quickly run bios-tables-test but also can point the test to a
>>>>> custom iasl if required.
>>>>> 
>>>>> [1] https://mesonbuild.com/Reference-manual_functions.html#find_program
>>>>> 
>>>>> Signed-off-by: Ani Sinha <anisinha@redhat.com>
>>>> 
>>>> Well I think the point was originally that meson can
>>>> also test the binary in a variety of ways.
>>>> Never surfaced so maybe never mind.
>>>> 
>>>> Would it be easier to just look iasl up on path
>>> 
>>> But that’s what meson is also doing, only QEMU build time.
>> 
>> 
>> So you were unhappy it's build time because it is not really
>> part of build and you want flexibility, right?
>
> Hmm, maybe in that case, we might want to resurrect iasl_installed(),
> basically reverting part of cc8fa0e80836c51ba644d910c.
>
> To me its ok if I had to set IASL_PATH=`which iasl` before running the
> test. I do not have strong opinions.

I don't think so - we should be using the tools configure found, after
all that is its job.
Michael S. Tsirkin May 17, 2023, 3:25 p.m. UTC | #6 
On Wed, May 17, 2023 at 04:16:47PM +0100, Alex Bennée wrote:
> 
> Ani Sinha <anisinha@redhat.com> writes:
> 
> >> On 17-May-2023, at 8:06 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >> 
> >> On Wed, May 17, 2023 at 07:57:53PM +0530, Ani Sinha wrote:
> >>> 
> >>> 
> >>>> On 17-May-2023, at 7:47 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >>>> 
> >>>> On Wed, May 17, 2023 at 05:37:51PM +0530, Ani Sinha wrote:
> >>>>> Currently the meson based QEMU build process locates the iasl binary from the
> >>>>> current PATH and other locations [1] and uses that to set CONFIG_IASL which is
> >>>>> then used by the test.
> >>>>> 
> >>>>> This has two disadvantages:
> >>>>> - If iasl was not previously installed in the PATH, one has to install iasl
> >>>>>  and rebuild QEMU in order to pick up the iasl location. One cannot simply
> >>>>>  use the existing bios-tables-test binary because CONFIG_IASL is only set
> >>>>>  during the QEMU build time by meson and then bios-tables-test has to be
> >>>>>  rebuilt with CONFIG_IASL set in order to use iasl.
> 
> Usually we work the other way by checking at configure time and skipping
> the feature if the prerequisites are not in place. We do this with gdb:
> 
>   ../../configure --gdb=/home/alex/src/tools/binutils-gdb.git/builds/all/install/bin/gdb
> 
> which checks gdb is at least new enough to support the features we need:
> 
>   if test -n "$gdb_bin"; then
>       gdb_version=$($gdb_bin --version | head -n 1)
>       if version_ge ${gdb_version##* } 9.1; then
>           echo "HAVE_GDB_BIN=$gdb_bin" >> $config_host_mak
>           gdb_arches=$("$source_path/scripts/probe-gdb-support.py" $gdb_bin)
>       else
>           gdb_bin=""
>       fi
>   fi
> 
> >>>>> - Sometimes, the stock iasl that comes with distributions is simply not good
> >>>>>  enough because it does not support the latest ACPI changes - newly
> >>>>>  introduced tables or new table attributes etc. In order to test ACPI code
> >>>>>  in QEMU, one has to clone the latest acpica upstream repository and
> >>>>>  rebuild iasl in order to get support for it. In those cases, one may want
> >>>>>  the test to use the iasl binary from a non-standard location.
> 
> I think configure should be checking if iasl is new enough and reporting
> to the user at configure time they need to do something different. We
> don't want to attempt to run tests that will fail unless the user has
> added the right magic to their environment.

iasl is a disassembler we trigger for user convenience in case tests
fail. It will never cause tests to fail.

> >>>>> 
> >>>>> In order to overcome the above two disadvantages, we introduce a new
> >>>>> environment variable IASL_PATH that can be set by the tester pointing to an
> >>>>> possibly non-standard iasl binary location. Bios-tables-test then uses this
> >>>>> environment variable to set its iasl location, possibly also overriding the
> >>>>> location that was pointed to by CONFIG_IASL that was set by meson. This way
> >>>>> developers can not only use this new environment variable to set iasl
> >>>>> location to quickly run bios-tables-test but also can point the test to a
> >>>>> custom iasl if required.
> >>>>> 
> >>>>> [1] https://mesonbuild.com/Reference-manual_functions.html#find_program
> >>>>> 
> >>>>> Signed-off-by: Ani Sinha <anisinha@redhat.com>
> >>>> 
> >>>> Well I think the point was originally that meson can
> >>>> also test the binary in a variety of ways.
> >>>> Never surfaced so maybe never mind.
> >>>> 
> >>>> Would it be easier to just look iasl up on path
> >>> 
> >>> But that’s what meson is also doing, only QEMU build time.
> >> 
> >> 
> >> So you were unhappy it's build time because it is not really
> >> part of build and you want flexibility, right?
> >
> > Hmm, maybe in that case, we might want to resurrect iasl_installed(),
> > basically reverting part of cc8fa0e80836c51ba644d910c.
> >
> > To me its ok if I had to set IASL_PATH=`which iasl` before running the
> > test. I do not have strong opinions.
> 
> I don't think so - we should be using the tools configure found, after
> all that is its job.
> 
> -- 
> Alex Bennée
> Virtualisation Tech Lead @ Linaro


Let's say the whole problem does not seem that important to me either.
Ani Sinha May 17, 2023, 3:48 p.m. UTC | #7 
> On 17-May-2023, at 8:46 PM, Alex Bennée <alex.bennee@linaro.org> wrote:
> 
> 
> Ani Sinha <anisinha@redhat.com> writes:
> 
>>> On 17-May-2023, at 8:06 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>> 
>>> On Wed, May 17, 2023 at 07:57:53PM +0530, Ani Sinha wrote:
>>>> 
>>>> 
>>>>> On 17-May-2023, at 7:47 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>> 
>>>>> On Wed, May 17, 2023 at 05:37:51PM +0530, Ani Sinha wrote:
>>>>>> Currently the meson based QEMU build process locates the iasl binary from the
>>>>>> current PATH and other locations [1] and uses that to set CONFIG_IASL which is
>>>>>> then used by the test.
>>>>>> 
>>>>>> This has two disadvantages:
>>>>>> - If iasl was not previously installed in the PATH, one has to install iasl
>>>>>> and rebuild QEMU in order to pick up the iasl location. One cannot simply
>>>>>> use the existing bios-tables-test binary because CONFIG_IASL is only set
>>>>>> during the QEMU build time by meson and then bios-tables-test has to be
>>>>>> rebuilt with CONFIG_IASL set in order to use iasl.
> 
> Usually we work the other way by checking at configure time and skipping
> the feature if the prerequisites are not in place.

I think this is too hard a requirement for something that is only used for debugging aid. If iasl is absent, no big deal. We won’t get nice asl diffs but the test will fail and let the users know which blobs are in disagreement. For ACPI contributors and maintainers, in the previous incarnation, one could quickly install iasl and rerun the same bios-tables-test and get the asl diffs. Now we need an additional step of recompiling QEMU which is completely useless, confusing and burdensome. 

> We do this with gdb:
> 
>  ../../configure --gdb=/home/alex/src/tools/binutils-gdb.git/builds/all/install/bin/gdb
> 
> which checks gdb is at least new enough to support the features we need:
> 
>  if test -n "$gdb_bin"; then
>      gdb_version=$($gdb_bin --version | head -n 1)
>      if version_ge ${gdb_version##* } 9.1; then
>          echo "HAVE_GDB_BIN=$gdb_bin" >> $config_host_mak
>          gdb_arches=$("$source_path/scripts/probe-gdb-support.py" $gdb_bin)
>      else
>          gdb_bin=""
>      fi
>  fi
> 
>>>>>> - Sometimes, the stock iasl that comes with distributions is simply not good
>>>>>> enough because it does not support the latest ACPI changes - newly
>>>>>> introduced tables or new table attributes etc. In order to test ACPI code
>>>>>> in QEMU, one has to clone the latest acpica upstream repository and
>>>>>> rebuild iasl in order to get support for it. In those cases, one may want
>>>>>> the test to use the iasl binary from a non-standard location.
> 
> I think configure should be checking if iasl is new enough and reporting
> to the user at configure time they need to do something different. We
> don't want to attempt to run tests that will fail unless the user has
> added the right magic to their environment.

Iasl is not used directly for the test. It is used to provide disassembly of the acpi blobs when the test fails.
What version is new enough? There has been occasions when someone added tables that are new enough that only the upstream acpica git tree has support for it and no officially released version does. When we want to check their implementation in QEMU, we want to use that latest upstream iasl directly compiled from the git tree.

I think the current mechanism makes lives of ACPI contributors and maintainers harder, not easier.
  
> 
>>>>>> 
>>>>>> In order to overcome the above two disadvantages, we introduce a new
>>>>>> environment variable IASL_PATH that can be set by the tester pointing to an
>>>>>> possibly non-standard iasl binary location. Bios-tables-test then uses this
>>>>>> environment variable to set its iasl location, possibly also overriding the
>>>>>> location that was pointed to by CONFIG_IASL that was set by meson. This way
>>>>>> developers can not only use this new environment variable to set iasl
>>>>>> location to quickly run bios-tables-test but also can point the test to a
>>>>>> custom iasl if required.
>>>>>> 
>>>>>> [1] https://mesonbuild.com/Reference-manual_functions.html#find_program
>>>>>> 
>>>>>> Signed-off-by: Ani Sinha <anisinha@redhat.com>
>>>>> 
>>>>> Well I think the point was originally that meson can
>>>>> also test the binary in a variety of ways.
>>>>> Never surfaced so maybe never mind.
>>>>> 
>>>>> Would it be easier to just look iasl up on path
>>>> 
>>>> But that’s what meson is also doing, only QEMU build time.
>>> 
>>> 
>>> So you were unhappy it's build time because it is not really
>>> part of build and you want flexibility, right?
>> 
>> Hmm, maybe in that case, we might want to resurrect iasl_installed(),
>> basically reverting part of cc8fa0e80836c51ba644d910c.
>> 
>> To me its ok if I had to set IASL_PATH=`which iasl` before running the
>> test. I do not have strong opinions.
> 
> I don't think so - we should be using the tools configure found, after
> all that is its job.
> 
> -- 
> Alex Bennée
> Virtualisation Tech Lead @ Linaro
Alex Bennée May 17, 2023, 3:58 p.m. UTC | #8 
"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Wed, May 17, 2023 at 04:16:47PM +0100, Alex Bennée wrote:
>> 
>> Ani Sinha <anisinha@redhat.com> writes:
>> 
>> >> On 17-May-2023, at 8:06 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> >> 
>> >> On Wed, May 17, 2023 at 07:57:53PM +0530, Ani Sinha wrote:
>> >>> 
>> >>> 
>> >>>> On 17-May-2023, at 7:47 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> >>>> 
>> >>>> On Wed, May 17, 2023 at 05:37:51PM +0530, Ani Sinha wrote:
>> >>>>> Currently the meson based QEMU build process locates the iasl binary from the
>> >>>>> current PATH and other locations [1] and uses that to set CONFIG_IASL which is
>> >>>>> then used by the test.
>> >>>>> 
>> >>>>> This has two disadvantages:
>> >>>>> - If iasl was not previously installed in the PATH, one has to install iasl
>> >>>>>  and rebuild QEMU in order to pick up the iasl location. One cannot simply
>> >>>>>  use the existing bios-tables-test binary because CONFIG_IASL is only set
>> >>>>>  during the QEMU build time by meson and then bios-tables-test has to be
>> >>>>>  rebuilt with CONFIG_IASL set in order to use iasl.
>> 
>> Usually we work the other way by checking at configure time and skipping
>> the feature if the prerequisites are not in place. We do this with gdb:
>> 
>>   ../../configure --gdb=/home/alex/src/tools/binutils-gdb.git/builds/all/install/bin/gdb
>> 
>> which checks gdb is at least new enough to support the features we need:
>> 
>>   if test -n "$gdb_bin"; then
>>       gdb_version=$($gdb_bin --version | head -n 1)
>>       if version_ge ${gdb_version##* } 9.1; then
>>           echo "HAVE_GDB_BIN=$gdb_bin" >> $config_host_mak
>>           gdb_arches=$("$source_path/scripts/probe-gdb-support.py" $gdb_bin)
>>       else
>>           gdb_bin=""
>>       fi
>>   fi
>> 
>> >>>>> - Sometimes, the stock iasl that comes with distributions is simply not good
>> >>>>>  enough because it does not support the latest ACPI changes - newly
>> >>>>>  introduced tables or new table attributes etc. In order to test ACPI code
>> >>>>>  in QEMU, one has to clone the latest acpica upstream repository and
>> >>>>>  rebuild iasl in order to get support for it. In those cases, one may want
>> >>>>>  the test to use the iasl binary from a non-standard location.
>> 
>> I think configure should be checking if iasl is new enough and reporting
>> to the user at configure time they need to do something different. We
>> don't want to attempt to run tests that will fail unless the user has
>> added the right magic to their environment.
>
> iasl is a disassembler we trigger for user convenience in case tests
> fail. It will never cause tests to fail.

Fair enough. But I still think the place to report it is in configure.
Maybe something like:

    iasl                         : /usr/bin/iasl (version 20200925, might not handle all ACPI)           

in the Host Binaries section. Re-configuring shouldn't cause too much of
the build to be regenerated although we could certainly do better in
this regard.
Alex Bennée May 17, 2023, 4:07 p.m. UTC | #9 
Ani Sinha <anisinha@redhat.com> writes:

>> On 17-May-2023, at 8:46 PM, Alex Bennée <alex.bennee@linaro.org> wrote:
>> 
>> 
>> Ani Sinha <anisinha@redhat.com> writes:
>> 
>>>> On 17-May-2023, at 8:06 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>> 
>>>> On Wed, May 17, 2023 at 07:57:53PM +0530, Ani Sinha wrote:
>>>>> 
>>>>> 
>>>>>> On 17-May-2023, at 7:47 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>>> 
>>>>>> On Wed, May 17, 2023 at 05:37:51PM +0530, Ani Sinha wrote:
>>>>>>> Currently the meson based QEMU build process locates the iasl binary from the
>>>>>>> current PATH and other locations [1] and uses that to set CONFIG_IASL which is
>>>>>>> then used by the test.
>>>>>>> 
>>>>>>> This has two disadvantages:
>>>>>>> - If iasl was not previously installed in the PATH, one has to install iasl
>>>>>>> and rebuild QEMU in order to pick up the iasl location. One cannot simply
>>>>>>> use the existing bios-tables-test binary because CONFIG_IASL is only set
>>>>>>> during the QEMU build time by meson and then bios-tables-test has to be
>>>>>>> rebuilt with CONFIG_IASL set in order to use iasl.
>> 
>> Usually we work the other way by checking at configure time and skipping
>> the feature if the prerequisites are not in place.
>
> I think this is too hard a requirement for something that is only used
> for debugging aid. If iasl is absent, no big deal. We won’t get nice
> asl diffs but the test will fail and let the users know which blobs
> are in disagreement. For ACPI contributors and maintainers, in the
> previous incarnation, one could quickly install iasl and rerun the
> same bios-tables-test and get the asl diffs. Now we need an additional
> step of recompiling QEMU which is completely useless, confusing and
> burdensome.

That sounds like a bug in the build system - for example:

  ../../configure --disable-docs --target-list=aarch64-softmmu,arm-softmmu,aarch64-linux-user,arm-linux-user,aarch64_be-linux-user --gdb=$HOME/src/tools/binutils-gdb.git/builds/all/install/bin/gdb
  make -j9
  ... does complete build ...
  ../../configure --disable-docs --target-list=aarch64-softmmu,arm-softmmu,aarch64-linux-user,arm-linux-user,aarch64_be-linux-user
  make -j9
  ... regenerated 23 files ...

It certainly doesn't recompile all of QEMU and I'm sure some of the
header re generations the build system could be smarter about.

It's certainly not without precedent to use getenv() in tests, there are
a number of well defined cases. But if you are going to add it at least
document it docs/devel/testing.rst otherwise you will be the only person
who knows of this particular magic variable.
Michael S. Tsirkin May 17, 2023, 4:07 p.m. UTC | #10 
On Wed, May 17, 2023 at 04:58:06PM +0100, Alex Bennée wrote:
> 
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > On Wed, May 17, 2023 at 04:16:47PM +0100, Alex Bennée wrote:
> >> 
> >> Ani Sinha <anisinha@redhat.com> writes:
> >> 
> >> >> On 17-May-2023, at 8:06 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >> >> 
> >> >> On Wed, May 17, 2023 at 07:57:53PM +0530, Ani Sinha wrote:
> >> >>> 
> >> >>> 
> >> >>>> On 17-May-2023, at 7:47 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >> >>>> 
> >> >>>> On Wed, May 17, 2023 at 05:37:51PM +0530, Ani Sinha wrote:
> >> >>>>> Currently the meson based QEMU build process locates the iasl binary from the
> >> >>>>> current PATH and other locations [1] and uses that to set CONFIG_IASL which is
> >> >>>>> then used by the test.
> >> >>>>> 
> >> >>>>> This has two disadvantages:
> >> >>>>> - If iasl was not previously installed in the PATH, one has to install iasl
> >> >>>>>  and rebuild QEMU in order to pick up the iasl location. One cannot simply
> >> >>>>>  use the existing bios-tables-test binary because CONFIG_IASL is only set
> >> >>>>>  during the QEMU build time by meson and then bios-tables-test has to be
> >> >>>>>  rebuilt with CONFIG_IASL set in order to use iasl.
> >> 
> >> Usually we work the other way by checking at configure time and skipping
> >> the feature if the prerequisites are not in place. We do this with gdb:
> >> 
> >>   ../../configure --gdb=/home/alex/src/tools/binutils-gdb.git/builds/all/install/bin/gdb
> >> 
> >> which checks gdb is at least new enough to support the features we need:
> >> 
> >>   if test -n "$gdb_bin"; then
> >>       gdb_version=$($gdb_bin --version | head -n 1)
> >>       if version_ge ${gdb_version##* } 9.1; then
> >>           echo "HAVE_GDB_BIN=$gdb_bin" >> $config_host_mak
> >>           gdb_arches=$("$source_path/scripts/probe-gdb-support.py" $gdb_bin)
> >>       else
> >>           gdb_bin=""
> >>       fi
> >>   fi
> >> 
> >> >>>>> - Sometimes, the stock iasl that comes with distributions is simply not good
> >> >>>>>  enough because it does not support the latest ACPI changes - newly
> >> >>>>>  introduced tables or new table attributes etc. In order to test ACPI code
> >> >>>>>  in QEMU, one has to clone the latest acpica upstream repository and
> >> >>>>>  rebuild iasl in order to get support for it. In those cases, one may want
> >> >>>>>  the test to use the iasl binary from a non-standard location.
> >> 
> >> I think configure should be checking if iasl is new enough and reporting
> >> to the user at configure time they need to do something different. We
> >> don't want to attempt to run tests that will fail unless the user has
> >> added the right magic to their environment.
> >
> > iasl is a disassembler we trigger for user convenience in case tests
> > fail. It will never cause tests to fail.
> 
> Fair enough. But I still think the place to report it is in configure.
> Maybe something like:
> 
>     iasl                         : /usr/bin/iasl (version 20200925, might not handle all ACPI)           
> 
> in the Host Binaries section. Re-configuring shouldn't cause too much of
> the build to be regenerated although we could certainly do better in
> this regard.

won't all of it be regenerated? a header everyone includes changes.
Alex Bennée May 17, 2023, 4:20 p.m. UTC | #11 
"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Wed, May 17, 2023 at 04:58:06PM +0100, Alex Bennée wrote:
>> 
>> "Michael S. Tsirkin" <mst@redhat.com> writes:
>> 
>> > On Wed, May 17, 2023 at 04:16:47PM +0100, Alex Bennée wrote:
>> >> 
>> >> Ani Sinha <anisinha@redhat.com> writes:
>> >> 
>> >> >> On 17-May-2023, at 8:06 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> >> >> 
>> >> >> On Wed, May 17, 2023 at 07:57:53PM +0530, Ani Sinha wrote:
>> >> >>> 
>> >> >>> 
>> >> >>>> On 17-May-2023, at 7:47 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> >> >>>> 
>> >> >>>> On Wed, May 17, 2023 at 05:37:51PM +0530, Ani Sinha wrote:
>> >> >>>>> Currently the meson based QEMU build process locates the iasl binary from the
>> >> >>>>> current PATH and other locations [1] and uses that to set CONFIG_IASL which is
>> >> >>>>> then used by the test.
>> >> >>>>> 
>> >> >>>>> This has two disadvantages:
>> >> >>>>> - If iasl was not previously installed in the PATH, one has to install iasl
>> >> >>>>>  and rebuild QEMU in order to pick up the iasl location. One cannot simply
>> >> >>>>>  use the existing bios-tables-test binary because CONFIG_IASL is only set
>> >> >>>>>  during the QEMU build time by meson and then bios-tables-test has to be
>> >> >>>>>  rebuilt with CONFIG_IASL set in order to use iasl.
>> >> 
>> >> Usually we work the other way by checking at configure time and skipping
>> >> the feature if the prerequisites are not in place. We do this with gdb:
>> >> 
>> >>   ../../configure --gdb=/home/alex/src/tools/binutils-gdb.git/builds/all/install/bin/gdb
>> >> 
>> >> which checks gdb is at least new enough to support the features we need:
>> >> 
>> >>   if test -n "$gdb_bin"; then
>> >>       gdb_version=$($gdb_bin --version | head -n 1)
>> >>       if version_ge ${gdb_version##* } 9.1; then
>> >>           echo "HAVE_GDB_BIN=$gdb_bin" >> $config_host_mak
>> >>           gdb_arches=$("$source_path/scripts/probe-gdb-support.py" $gdb_bin)
>> >>       else
>> >>           gdb_bin=""
>> >>       fi
>> >>   fi
>> >> 
>> >> >>>>> - Sometimes, the stock iasl that comes with distributions is simply not good
>> >> >>>>>  enough because it does not support the latest ACPI changes - newly
>> >> >>>>>  introduced tables or new table attributes etc. In order to test ACPI code
>> >> >>>>>  in QEMU, one has to clone the latest acpica upstream repository and
>> >> >>>>>  rebuild iasl in order to get support for it. In those cases, one may want
>> >> >>>>>  the test to use the iasl binary from a non-standard location.
>> >> 
>> >> I think configure should be checking if iasl is new enough and reporting
>> >> to the user at configure time they need to do something different. We
>> >> don't want to attempt to run tests that will fail unless the user has
>> >> added the right magic to their environment.
>> >
>> > iasl is a disassembler we trigger for user convenience in case tests
>> > fail. It will never cause tests to fail.
>> 
>> Fair enough. But I still think the place to report it is in configure.
>> Maybe something like:
>> 
>>     iasl                         : /usr/bin/iasl (version 20200925, might not handle all ACPI)           
>> 
>> in the Host Binaries section. Re-configuring shouldn't cause too much of
>> the build to be regenerated although we could certainly do better in
>> this regard.
>
> won't all of it be regenerated? a header everyone includes changes.

Ahh I see meson is doing:

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

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

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

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

Paolo,

I see we expand all the qtests with:

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

is there any easy way to add arguments to individual tests or do we need
an explicit test expansion for bios-tables-test?
Bernhard Beschow May 17, 2023, 4:43 p.m. UTC | #12 
Am 17. Mai 2023 12:07:51 UTC schrieb Ani Sinha <anisinha@redhat.com>:
>Currently the meson based QEMU build process locates the iasl binary from the
>current PATH and other locations [1] and uses that to set CONFIG_IASL which is
>then used by the test.
>
>This has two disadvantages:
> - If iasl was not previously installed in the PATH, one has to install iasl
>   and rebuild QEMU in order to pick up the iasl location. One cannot simply
>   use the existing bios-tables-test binary because CONFIG_IASL is only set
>   during the QEMU build time by meson and then bios-tables-test has to be
>   rebuilt with CONFIG_IASL set in order to use iasl.
> - 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 introduce a new
>environment variable IASL_PATH that can be set by the tester pointing to an
>possibly non-standard iasl binary location.

Why not add a submodule -- like we do with dtc -- and use that? Then we could possibly convert the ACPI blobs used in bios-tables-test into text files which would make AML patches a bit more comprehensible. We also didn't have to trust the commit messages to contain the actual change because one would see it right in the patch.

Best regards,
Bernhard

>Bios-tables-test then uses this
>environment variable to set its iasl location, possibly also overriding the
>location that was pointed to by CONFIG_IASL that was set by meson. This way
>developers can not only use this new environment variable to set iasl
>location to quickly run bios-tables-test but also can point the test to a
>custom iasl if required.
>
>[1] https://mesonbuild.com/Reference-manual_functions.html#find_program
>
>Signed-off-by: Ani Sinha <anisinha@redhat.com>
>---
> tests/qtest/bios-tables-test.c | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
>sample runs:
>
>$ QTEST_QEMU_BINARY=./qemu-system-x86_64 V=2 ./tests/qtest/bios-tables-test
>...
>acpi-test: Warning! APIC binary file mismatch. Actual [aml:/tmp/aml-DLHA51], Expected [aml:tests/data/acpi/pc/APIC].
>See source file tests/qtest/bios-tables-test.c for instructions on how to update expected files.
>Using iasl: /usr/bin/iasl
>acpi-test: Warning! APIC mismatch. Actual [asl:/tmp/asl-L9GA51.dsl, aml:/tmp/aml-DLHA51], Expected [asl:/tmp/asl-10EA51.dsl, aml:tests/data/acpi/pc/APIC].
>
>$ QTEST_QEMU_BINARY=./qemu-system-x86_64 V=2 IASL_PATH=/home/anisinha/workspace/acpica/generate/unix/bin/iasl ./tests/qtest/bios-tables-test
>...
>acpi-test: Warning! APIC binary file mismatch. Actual [aml:/tmp/aml-5CQ341], Expected [aml:tests/data/acpi/pc/APIC].
>See source file tests/qtest/bios-tables-test.c for instructions on how to update expected files.
>User has provided an iasl path, using that: /home/anisinha/workspace/acpica/generate/unix/bin/iasl
>acpi-test: Warning! APIC mismatch. Actual [asl:/tmp/asl-2GQ341.dsl, aml:/tmp/aml-5CQ341], Expected [asl:/tmp/asl-IBR341.dsl, aml:tests/data/acpi/pc/APIC].
>
>diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
>index 7fd88b0e9c..37e8e484cb 100644
>--- a/tests/qtest/bios-tables-test.c
>+++ b/tests/qtest/bios-tables-test.c
>@@ -440,7 +440,12 @@ static void test_acpi_asl(test_data *data)
>     AcpiSdtTable *sdt, *exp_sdt;
>     test_data exp_data = {};
>     gboolean exp_err, err, all_tables_match = true;
>+    const char *user_iasl_path = getenv("IASL_PATH");
> 
>+    /* if user has provided a path to iasl, use that */
>+    if (user_iasl_path) {
>+        iasl = user_iasl_path;
>+    }
>     exp_data.tables = load_expected_aml(data);
>     dump_aml_files(data, false);
>     for (i = 0; i < data->tables->len; ++i) {
>@@ -473,6 +478,15 @@ static void test_acpi_asl(test_data *data)
>             continue;
>         }
> 
>+        if (verbosity_level >= 2) {
>+            if (user_iasl_path) {
>+                fprintf(stderr, "User has provided an iasl path," \
>+                        "using that: %s\n", user_iasl_path);
>+            } else {
>+                fprintf(stderr, "Using iasl: %s\n", iasl);
>+            }
>+        }
>+
>         err = load_asl(data->tables, sdt);
>         asl = normalize_asl(sdt->asl);
>
Ani Sinha May 18, 2023, 5:55 a.m. UTC | #13 
> On 17-May-2023, at 10:13 PM, Bernhard Beschow <shentey@gmail.com> wrote:
> 
> 
> 
> Am 17. Mai 2023 12:07:51 UTC schrieb Ani Sinha <anisinha@redhat.com>:
>> Currently the meson based QEMU build process locates the iasl binary from the
>> current PATH and other locations [1] and uses that to set CONFIG_IASL which is
>> then used by the test.
>> 
>> This has two disadvantages:
>> - If iasl was not previously installed in the PATH, one has to install iasl
>>  and rebuild QEMU in order to pick up the iasl location. One cannot simply
>>  use the existing bios-tables-test binary because CONFIG_IASL is only set
>>  during the QEMU build time by meson and then bios-tables-test has to be
>>  rebuilt with CONFIG_IASL set in order to use iasl.
>> - 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 introduce a new
>> environment variable IASL_PATH that can be set by the tester pointing to an
>> possibly non-standard iasl binary location.
> 
> Why not add a submodule -- l

Submodules are disliked with all fervour by the community. There is a long thread somewhere in the mailing list.

> ike we do with dtc -- and use that? Then we could possibly convert the ACPI blobs used in bios-tables-test into text files which would make AML patches a bit more comprehensible. We also didn't have to trust the commit messages to contain the actual change because one would see it right in the patch.
> 
> Best regards,
> Bernhard
> 
>> Bios-tables-test then uses this
>> environment variable to set its iasl location, possibly also overriding the
>> location that was pointed to by CONFIG_IASL that was set by meson. This way
>> developers can not only use this new environment variable to set iasl
>> location to quickly run bios-tables-test but also can point the test to a
>> custom iasl if required.
>> 
>> [1] https://mesonbuild.com/Reference-manual_functions.html#find_program
>> 
>> Signed-off-by: Ani Sinha <anisinha@redhat.com>
>> ---
>> tests/qtest/bios-tables-test.c | 14 ++++++++++++++
>> 1 file changed, 14 insertions(+)
>> 
>> sample runs:
>> 
>> $ QTEST_QEMU_BINARY=./qemu-system-x86_64 V=2 ./tests/qtest/bios-tables-test
>> ...
>> acpi-test: Warning! APIC binary file mismatch. Actual [aml:/tmp/aml-DLHA51], Expected [aml:tests/data/acpi/pc/APIC].
>> See source file tests/qtest/bios-tables-test.c for instructions on how to update expected files.
>> Using iasl: /usr/bin/iasl
>> acpi-test: Warning! APIC mismatch. Actual [asl:/tmp/asl-L9GA51.dsl, aml:/tmp/aml-DLHA51], Expected [asl:/tmp/asl-10EA51.dsl, aml:tests/data/acpi/pc/APIC].
>> 
>> $ QTEST_QEMU_BINARY=./qemu-system-x86_64 V=2 IASL_PATH=/home/anisinha/workspace/acpica/generate/unix/bin/iasl ./tests/qtest/bios-tables-test
>> ...
>> acpi-test: Warning! APIC binary file mismatch. Actual [aml:/tmp/aml-5CQ341], Expected [aml:tests/data/acpi/pc/APIC].
>> See source file tests/qtest/bios-tables-test.c for instructions on how to update expected files.
>> User has provided an iasl path, using that: /home/anisinha/workspace/acpica/generate/unix/bin/iasl
>> acpi-test: Warning! APIC mismatch. Actual [asl:/tmp/asl-2GQ341.dsl, aml:/tmp/aml-5CQ341], Expected [asl:/tmp/asl-IBR341.dsl, aml:tests/data/acpi/pc/APIC].
>> 
>> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
>> index 7fd88b0e9c..37e8e484cb 100644
>> --- a/tests/qtest/bios-tables-test.c
>> +++ b/tests/qtest/bios-tables-test.c
>> @@ -440,7 +440,12 @@ static void test_acpi_asl(test_data *data)
>>    AcpiSdtTable *sdt, *exp_sdt;
>>    test_data exp_data = {};
>>    gboolean exp_err, err, all_tables_match = true;
>> +    const char *user_iasl_path = getenv("IASL_PATH");
>> 
>> +    /* if user has provided a path to iasl, use that */
>> +    if (user_iasl_path) {
>> +        iasl = user_iasl_path;
>> +    }
>>    exp_data.tables = load_expected_aml(data);
>>    dump_aml_files(data, false);
>>    for (i = 0; i < data->tables->len; ++i) {
>> @@ -473,6 +478,15 @@ static void test_acpi_asl(test_data *data)
>>            continue;
>>        }
>> 
>> +        if (verbosity_level >= 2) {
>> +            if (user_iasl_path) {
>> +                fprintf(stderr, "User has provided an iasl path," \
>> +                        "using that: %s\n", user_iasl_path);
>> +            } else {
>> +                fprintf(stderr, "Using iasl: %s\n", iasl);
>> +            }
>> +        }
>> +
>>        err = load_asl(data->tables, sdt);
>>        asl = normalize_asl(sdt->asl);
>> 
>
Ani Sinha May 18, 2023, 6:01 a.m. UTC | #14 
> On 17-May-2023, at 9:50 PM, Alex Bennée <alex.bennee@linaro.org> wrote:
> 
> ./tests/bios-tables-test --iasl-path ${CONFIG_IASL}
> 
> and then you have the best of both worlds. You can run manually with a
> different path and you don't need to pollute config-host.h

It could also be an environment variable set by meson. Then bios-tables-test can do a genenv() just like it does for verbosity etc. The environment can also be used by other tools that might need iasl in the future. We do not need to introduce new command line option.
Ani Sinha May 18, 2023, 6:11 a.m. UTC | #15 
> On 17-May-2023, at 9:50 PM, Alex Bennée <alex.bennee@linaro.org> wrote:
> 
> is there any easy way to add arguments to individual tests

I looked at this before for bios bits work and seems it is not straightforward to add test specific command line options in our meson build system. It needs some work. I am also curious as to what Paolo thought about this.
Michael S. Tsirkin May 18, 2023, 10:27 a.m. UTC | #16 
On Wed, May 17, 2023 at 04:43:53PM +0000, Bernhard Beschow wrote:
> 
> 
> Am 17. Mai 2023 12:07:51 UTC schrieb Ani Sinha <anisinha@redhat.com>:
> >Currently the meson based QEMU build process locates the iasl binary from the
> >current PATH and other locations [1] and uses that to set CONFIG_IASL which is
> >then used by the test.
> >
> >This has two disadvantages:
> > - If iasl was not previously installed in the PATH, one has to install iasl
> >   and rebuild QEMU in order to pick up the iasl location. One cannot simply
> >   use the existing bios-tables-test binary because CONFIG_IASL is only set
> >   during the QEMU build time by meson and then bios-tables-test has to be
> >   rebuilt with CONFIG_IASL set in order to use iasl.
> > - 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 introduce a new
> >environment variable IASL_PATH that can be set by the tester pointing to an
> >possibly non-standard iasl binary location.
> 
> Why not add a submodule -- like we do with dtc -- and use that? Then
> we could possibly convert the ACPI blobs used in bios-tables-test into
> text files which would make AML patches a bit more comprehensible. We
> also didn't have to trust the commit messages to contain the actual
> change because one would see it right in the patch.
> 
> Best regards,
> Bernhard

Yea people dislike submodules but I guess we could come up
with something. I stumbled upon git-subrepo recently
but did not try it yet.

There's a long list of issues unrelated to having iasl installed though:
- IASL disassembler output is unstable, tends to change
  with each revision
- IASL disassembler lacks (or used to, last time I looked)
  support for all hosts that QEMU wants to support
  (e.g. I think it is still somewhat broken on BE)
- Tends to crash in weird ways on unexected ACPI

I talked to ACPICA guys about guarateed output with some flag
and they were uninterested.
We used to have expected ASL in git, was a little easier for
contributors but a pain for maintainers and users.
Michael S. Tsirkin May 18, 2023, 10:40 a.m. UTC | #17 
On Thu, May 18, 2023 at 11:31:47AM +0530, Ani Sinha wrote:
> 
> 
> > On 17-May-2023, at 9:50 PM, Alex Bennée <alex.bennee@linaro.org> wrote:
> > 
> > ./tests/bios-tables-test --iasl-path ${CONFIG_IASL}
> > 
> > and then you have the best of both worlds. You can run manually with a
> > different path and you don't need to pollute config-host.h
> 
> It could also be an environment variable set by meson. Then bios-tables-test can do a genenv() just like it does for verbosity etc. The environment can also be used by other tools that might need iasl in the future. We do not need to introduce new command line option.

The annoying thing with both these approaches is they work
less well than current code since iasl won't be invoked
at all if you do not remember the magic variable to set
or flag to pass. ATM it is self-contained.

Can we split this variable out to config-test.h maybe?
Then you can reconfigure with a different iasl and QEMU
will not be rebuilt, just the tests.
Ani Sinha May 18, 2023, 11:01 a.m. UTC | #18 
On Thu, 18 May 2023, Michael S. Tsirkin wrote:

> On Thu, May 18, 2023 at 11:31:47AM +0530, Ani Sinha wrote:
> >
> >
> > > On 17-May-2023, at 9:50 PM, Alex Bennée <alex.bennee@linaro.org> wrote:
> > >
> > > ./tests/bios-tables-test --iasl-path ${CONFIG_IASL}
> > >
> > > and then you have the best of both worlds. You can run manually with a
> > > different path and you don't need to pollute config-host.h
> >
> > It could also be an environment variable set by meson. Then bios-tables-test can do a genenv() just like it does for verbosity etc. The environment can also be used by other tools that might need iasl in the future. We do not need to introduce new command line option.
>
> The annoying thing with both these approaches is they work
> less well than current code since iasl won't be invoked
> at all if you do not remember the magic variable to set
> or flag to pass. ATM it is self-contained.
>
> Can we split this variable out to config-test.h maybe?
> Then you can reconfigure with a different iasl and QEMU
> will not be rebuilt, just the tests.

TBH, it looks more and more like our previous approach was simple and
better before we started tying iasl to meson.
Ani Sinha May 18, 2023, 11:19 a.m. UTC | #19 
> On 18-May-2023, at 4:10 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> 
> he annoying thing with both these approaches is they work
> less well than current code since iasl won't be invoked
> at all if you do not remember the magic variable to set
> or flag to pass. ATM it is self-contained.

We could address this by a log message just like we do today[1] by telling users to use the environment variable.

1. "to see ASL diff between mismatched files install IASL, rebuild QEMU from scratch and re-run tests with V=1"
Paolo Bonzini May 19, 2023, 5:13 p.m. UTC | #20 
On Thu, May 18, 2023 at 1:02 PM Ani Sinha <anisinha@redhat.com> wrote:
> > Can we split this variable out to config-test.h maybe?
> > Then you can reconfigure with a different iasl and QEMU
> > will not be rebuilt, just the tests.
>
> TBH, it looks more and more like our previous approach was simple and
> better before we started tying iasl to meson.

What was the previous one? Can you point to the commit that complicated things?

Paolo
Ani Sinha May 20, 2023, 7:25 a.m. UTC | #21 
> On 19-May-2023, at 10:43 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
> On Thu, May 18, 2023 at 1:02 PM Ani Sinha <anisinha@redhat.com> wrote:
>>> Can we split this variable out to config-test.h maybe?
>>> Then you can reconfigure with a different iasl and QEMU
>>> will not be rebuilt, just the tests.
>> 
>> TBH, it looks more and more like our previous approach was simple and
>> better before we started tying iasl to meson.
> 
> What was the previous one? Can you point to the commit that complicated things?

40c909f534e3f3cd2 from what I can see. It requires a full QEMU build in order to turn on CONFIG_IASL in bios-tables-test. At some point in the past, we could just install iasl and rerun the test and it would discover iasl in the path if CONFIG_IASL was not defined.

I have proposed a patch with the title "acpi/tests/bios-tables-test: pass iasl path through environment variable”.
I have tested possible scenarios and it would satisfy my uneasiness. One thing I could not find is how to discover OS environment variable from meson.build. From what I could gather, currently it is not supported. Hence, when both CONFIG_IASL is passed from the command line and meson discovers one of its own, the meson one would be enforced and not the one developer passed.
Paolo Bonzini May 20, 2023, 9:36 a.m. UTC | #22 
Il sab 20 mag 2023, 09:25 Ani Sinha <anisinha@redhat.com> ha scritto:

> 40c909f534e3f3cd2 from what I can see. It requires a full QEMU build in
> order to turn on CONFIG_IASL in bios-tables-test. At some point in the
> past, we could just install iasl and rerun the test and it would discover
> iasl in the path if CONFIG_IASL was not defined.
>

So you want CONFIG_IASL to *not* have the full path if you configure with
--iasl=iasl?

Paolo


> I have proposed a patch with the title "acpi/tests/bios-tables-test: pass
> iasl path through environment variable”.
> I have tested possible scenarios and it would satisfy my uneasiness. One
> thing I could not find is how to discover OS environment variable from
> meson.build. From what I could gather, currently it is not supported.
> Hence, when both CONFIG_IASL is passed from the command line and meson
> discovers one of its own, the meson one would be enforced and not the one
> developer passed.
>
>
>
Ani Sinha May 20, 2023, 3:13 p.m. UTC | #23 
> On 20-May-2023, at 3:06 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
> 
> 
> Il sab 20 mag 2023, 09:25 Ani Sinha <anisinha@redhat.com> ha scritto:
> 40c909f534e3f3cd2 from what I can see. It requires a full QEMU build in order to turn on CONFIG_IASL in bios-tables-test. At some point in the past, we could just install iasl and rerun the test and it would discover iasl in the path if CONFIG_IASL was not defined.
> 
> So you want CONFIG_IASL to *not* have the full path if you configure with --iasl=iasl?

Iasl is *not* a mandatory tool to run that test. So we do not want any configure option at all. It is absolutely not needed and makes the entire workflow more burdensome.
 
> 
> Paolo
> 
> 
> I have proposed a patch with the title "acpi/tests/bios-tables-test: pass iasl path through environment variable”.
> I have tested possible scenarios and it would satisfy my uneasiness. One thing I could not find is how to discover OS environment variable from meson.build. From what I could gather, currently it is not supported. Hence, when both CONFIG_IASL is passed from the command line and meson discovers one of its own, the meson one would be enforced and not the one developer passed.
> 
>
Michael S. Tsirkin May 21, 2023, 8:54 a.m. UTC | #24 
On Wed, May 17, 2023 at 05:37:51PM +0530, Ani Sinha wrote:
> Currently the meson based QEMU build process locates the iasl binary from the
> current PATH and other locations [1] and uses that to set CONFIG_IASL which is
> then used by the test.
> 
> This has two disadvantages:
>  - If iasl was not previously installed in the PATH, one has to install iasl
>    and rebuild QEMU in order to pick up the iasl location. One cannot simply
>    use the existing bios-tables-test binary because CONFIG_IASL is only set
>    during the QEMU build time by meson and then bios-tables-test has to be
>    rebuilt with CONFIG_IASL set in order to use iasl.
>  - 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 introduce a new
> environment variable IASL_PATH that can be set by the tester pointing to an
> possibly non-standard iasl binary location. Bios-tables-test then uses this
> environment variable to set its iasl location, possibly also overriding the
> location that was pointed to by CONFIG_IASL that was set by meson. This way
> developers can not only use this new environment variable to set iasl
> location to quickly run bios-tables-test but also can point the test to a
> custom iasl if required.
> 
> [1] https://mesonbuild.com/Reference-manual_functions.html#find_program
> 
> Signed-off-by: Ani Sinha <anisinha@redhat.com>

I don't much like it that the default is now a bit harder
to run. Case of playing with iasl is really esotetic.
I propose a simpler idea.
- add config-iasl.h with only CONFIG_IASL set to path
- include from bios test

Now if you change path only bios test is rebuilt.

Hmm?


> ---
>  tests/qtest/bios-tables-test.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> sample runs:
> 
> $ QTEST_QEMU_BINARY=./qemu-system-x86_64 V=2 ./tests/qtest/bios-tables-test
> ...
> acpi-test: Warning! APIC binary file mismatch. Actual [aml:/tmp/aml-DLHA51], Expected [aml:tests/data/acpi/pc/APIC].
> See source file tests/qtest/bios-tables-test.c for instructions on how to update expected files.
> Using iasl: /usr/bin/iasl
> acpi-test: Warning! APIC mismatch. Actual [asl:/tmp/asl-L9GA51.dsl, aml:/tmp/aml-DLHA51], Expected [asl:/tmp/asl-10EA51.dsl, aml:tests/data/acpi/pc/APIC].
> 
> $ QTEST_QEMU_BINARY=./qemu-system-x86_64 V=2 IASL_PATH=/home/anisinha/workspace/acpica/generate/unix/bin/iasl ./tests/qtest/bios-tables-test
> ...
> acpi-test: Warning! APIC binary file mismatch. Actual [aml:/tmp/aml-5CQ341], Expected [aml:tests/data/acpi/pc/APIC].
> See source file tests/qtest/bios-tables-test.c for instructions on how to update expected files.
> User has provided an iasl path, using that: /home/anisinha/workspace/acpica/generate/unix/bin/iasl
> acpi-test: Warning! APIC mismatch. Actual [asl:/tmp/asl-2GQ341.dsl, aml:/tmp/aml-5CQ341], Expected [asl:/tmp/asl-IBR341.dsl, aml:tests/data/acpi/pc/APIC].
> 
> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> index 7fd88b0e9c..37e8e484cb 100644
> --- a/tests/qtest/bios-tables-test.c
> +++ b/tests/qtest/bios-tables-test.c
> @@ -440,7 +440,12 @@ static void test_acpi_asl(test_data *data)
>      AcpiSdtTable *sdt, *exp_sdt;
>      test_data exp_data = {};
>      gboolean exp_err, err, all_tables_match = true;
> +    const char *user_iasl_path = getenv("IASL_PATH");
>  
> +    /* if user has provided a path to iasl, use that */
> +    if (user_iasl_path) {
> +        iasl = user_iasl_path;
> +    }
>      exp_data.tables = load_expected_aml(data);
>      dump_aml_files(data, false);
>      for (i = 0; i < data->tables->len; ++i) {
> @@ -473,6 +478,15 @@ static void test_acpi_asl(test_data *data)
>              continue;
>          }
>  
> +        if (verbosity_level >= 2) {
> +            if (user_iasl_path) {
> +                fprintf(stderr, "User has provided an iasl path," \
> +                        "using that: %s\n", user_iasl_path);
> +            } else {
> +                fprintf(stderr, "Using iasl: %s\n", iasl);
> +            }
> +        }
> +
>          err = load_asl(data->tables, sdt);
>          asl = normalize_asl(sdt->asl);
>  
> -- 
> 2.39.1
Ani Sinha May 21, 2023, 2:51 p.m. UTC | #25 
> On 21-May-2023, at 2:24 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> 
> On Wed, May 17, 2023 at 05:37:51PM +0530, Ani Sinha wrote:
>> Currently the meson based QEMU build process locates the iasl binary from the
>> current PATH and other locations [1] and uses that to set CONFIG_IASL which is
>> then used by the test.
>> 
>> This has two disadvantages:
>> - If iasl was not previously installed in the PATH, one has to install iasl
>>   and rebuild QEMU in order to pick up the iasl location. One cannot simply
>>   use the existing bios-tables-test binary because CONFIG_IASL is only set
>>   during the QEMU build time by meson and then bios-tables-test has to be
>>   rebuilt with CONFIG_IASL set in order to use iasl.
>> - 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 introduce a new
>> environment variable IASL_PATH that can be set by the tester pointing to an
>> possibly non-standard iasl binary location. Bios-tables-test then uses this
>> environment variable to set its iasl location, possibly also overriding the
>> location that was pointed to by CONFIG_IASL that was set by meson. This way
>> developers can not only use this new environment variable to set iasl
>> location to quickly run bios-tables-test but also can point the test to a
>> custom iasl if required.
>> 
>> [1] https://mesonbuild.com/Reference-manual_functions.html#find_program
>> 
>> Signed-off-by: Ani Sinha <anisinha@redhat.com>
> 
> I don't much like it that the default is now a bit harder
> to run. Case of playing with iasl is really esotetic.
> I propose a simpler idea.
> - add config-iasl.h with only CONFIG_IASL set to path
> - include from bios test
> 
> Now if you change path only bios test is rebuilt.
> 
> Hmm?

We can do this. 

If an environment variable CONFIG_IASL is set or a command line is passed (we can do one or the other), use that. Else use a default path /usr/bin/iasl. This way we do not even need to rebuild the test if the developer wishes to use another iasl binary from a different location. 
I prefer env var slightly because its easier to implement from meson. Test specific command lines in meson.build would need more work.

> 
> 
>> ---
>> tests/qtest/bios-tables-test.c | 14 ++++++++++++++
>> 1 file changed, 14 insertions(+)
>> 
>> sample runs:
>> 
>> $ QTEST_QEMU_BINARY=./qemu-system-x86_64 V=2 ./tests/qtest/bios-tables-test
>> ...
>> acpi-test: Warning! APIC binary file mismatch. Actual [aml:/tmp/aml-DLHA51], Expected [aml:tests/data/acpi/pc/APIC].
>> See source file tests/qtest/bios-tables-test.c for instructions on how to update expected files.
>> Using iasl: /usr/bin/iasl
>> acpi-test: Warning! APIC mismatch. Actual [asl:/tmp/asl-L9GA51.dsl, aml:/tmp/aml-DLHA51], Expected [asl:/tmp/asl-10EA51.dsl, aml:tests/data/acpi/pc/APIC].
>> 
>> $ QTEST_QEMU_BINARY=./qemu-system-x86_64 V=2 IASL_PATH=/home/anisinha/workspace/acpica/generate/unix/bin/iasl ./tests/qtest/bios-tables-test
>> ...
>> acpi-test: Warning! APIC binary file mismatch. Actual [aml:/tmp/aml-5CQ341], Expected [aml:tests/data/acpi/pc/APIC].
>> See source file tests/qtest/bios-tables-test.c for instructions on how to update expected files.
>> User has provided an iasl path, using that: /home/anisinha/workspace/acpica/generate/unix/bin/iasl
>> acpi-test: Warning! APIC mismatch. Actual [asl:/tmp/asl-2GQ341.dsl, aml:/tmp/aml-5CQ341], Expected [asl:/tmp/asl-IBR341.dsl, aml:tests/data/acpi/pc/APIC].
>> 
>> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
>> index 7fd88b0e9c..37e8e484cb 100644
>> --- a/tests/qtest/bios-tables-test.c
>> +++ b/tests/qtest/bios-tables-test.c
>> @@ -440,7 +440,12 @@ static void test_acpi_asl(test_data *data)
>>     AcpiSdtTable *sdt, *exp_sdt;
>>     test_data exp_data = {};
>>     gboolean exp_err, err, all_tables_match = true;
>> +    const char *user_iasl_path = getenv("IASL_PATH");
>> 
>> +    /* if user has provided a path to iasl, use that */
>> +    if (user_iasl_path) {
>> +        iasl = user_iasl_path;
>> +    }
>>     exp_data.tables = load_expected_aml(data);
>>     dump_aml_files(data, false);
>>     for (i = 0; i < data->tables->len; ++i) {
>> @@ -473,6 +478,15 @@ static void test_acpi_asl(test_data *data)
>>             continue;
>>         }
>> 
>> +        if (verbosity_level >= 2) {
>> +            if (user_iasl_path) {
>> +                fprintf(stderr, "User has provided an iasl path," \
>> +                        "using that: %s\n", user_iasl_path);
>> +            } else {
>> +                fprintf(stderr, "Using iasl: %s\n", iasl);
>> +            }
>> +        }
>> +
>>         err = load_asl(data->tables, sdt);
>>         asl = normalize_asl(sdt->asl);
>> 
>> -- 
>> 2.39.1
Paolo Bonzini May 22, 2023, 10:21 a.m. UTC | #26 
Il sab 20 mag 2023, 17:13 Ani Sinha <anisinha@redhat.com> ha scritto:

> > On 20-May-2023, at 3:06 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >
> >
> >
> > Il sab 20 mag 2023, 09:25 Ani Sinha <anisinha@redhat.com> ha scritto:
> > 40c909f534e3f3cd2 from what I can see. It requires a full QEMU build in
> order to turn on CONFIG_IASL in bios-tables-test. At some point in the
> past, we could just install iasl and rerun the test and it would discover
> iasl in the path if CONFIG_IASL was not defined.
> >
> > So you want CONFIG_IASL to *not* have the full path if you configure
> with --iasl=iasl?
>
> Iasl is *not* a mandatory tool to run that test. So we do not want any
> configure option at all. It is absolutely not needed and makes the entire
> workflow more burdensome.
>

Configure is the default place where people look for an option to customize
binaries. A magic environment variable is hard to discover.

What you want is:

- default is iasl
- default can be overridden with --iasl

Implemented as:

iasl = find_program(get_option('iasl'), required: false)
if iasl.found()
  config_host_data.set_quoted('CONFIG_IASL', iasl.full_path()
ending

- IASL environment variable wins

- if neither the preprocessor macro nor the environment variable is
present, the test is skipped

Paolo


>
> > Paolo
> >
> >
> > I have proposed a patch with the title "acpi/tests/bios-tables-test:
> pass iasl path through environment variable”.
> > I have tested possible scenarios and it would satisfy my uneasiness. One
> thing I could not find is how to discover OS environment variable from
> meson.build. From what I could gather, currently it is not supported.
> Hence, when both CONFIG_IASL is passed from the command line and meson
> discovers one of its own, the meson one would be enforced and not the one
> developer passed.
> >
> >
>
>
Ani Sinha May 22, 2023, 10:34 a.m. UTC | #27 
> On 21-May-2023, at 8:21 PM, Ani Sinha <anisinha@redhat.com> wrote:
> 
> 
> 
>> On 21-May-2023, at 2:24 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> 
>> On Wed, May 17, 2023 at 05:37:51PM +0530, Ani Sinha wrote:
>>> Currently the meson based QEMU build process locates the iasl binary from the
>>> current PATH and other locations [1] and uses that to set CONFIG_IASL which is
>>> then used by the test.
>>> 
>>> This has two disadvantages:
>>> - If iasl was not previously installed in the PATH, one has to install iasl
>>>  and rebuild QEMU in order to pick up the iasl location. One cannot simply
>>>  use the existing bios-tables-test binary because CONFIG_IASL is only set
>>>  during the QEMU build time by meson and then bios-tables-test has to be
>>>  rebuilt with CONFIG_IASL set in order to use iasl.
>>> - 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 introduce a new
>>> environment variable IASL_PATH that can be set by the tester pointing to an
>>> possibly non-standard iasl binary location. Bios-tables-test then uses this
>>> environment variable to set its iasl location, possibly also overriding the
>>> location that was pointed to by CONFIG_IASL that was set by meson. This way
>>> developers can not only use this new environment variable to set iasl
>>> location to quickly run bios-tables-test but also can point the test to a
>>> custom iasl if required.
>>> 
>>> [1] https://mesonbuild.com/Reference-manual_functions.html#find_program
>>> 
>>> Signed-off-by: Ani Sinha <anisinha@redhat.com>
>> 
>> I don't much like it that the default is now a bit harder
>> to run. Case of playing with iasl is really esotetic.
>> I propose a simpler idea.
>> - add config-iasl.h with only CONFIG_IASL set to path
>> - include from bios test
>> 
>> Now if you change path only bios test is rebuilt.
>> 
>> Hmm?
> 
> We can do this. 

I sent a patch " acpi/tests/bios-tables-test: make iasl tool handling simpler”
I think we should just go ahead with this simple idea and be done with this.


> If an environment variable CONFIG_IASL is set or a command line is passed (we can do one or the other), use that. Else use a default path /usr/bin/iasl. This way we do not even need to rebuild the test if the developer wishes to use another iasl binary from a different location. 
> I prefer env var slightly because its easier to implement from meson. Test specific command lines in meson.build would need more work.
> 
>> 
>> 
>>> ---
>>> tests/qtest/bios-tables-test.c | 14 ++++++++++++++
>>> 1 file changed, 14 insertions(+)
>>> 
>>> sample runs:
>>> 
>>> $ QTEST_QEMU_BINARY=./qemu-system-x86_64 V=2 ./tests/qtest/bios-tables-test
>>> ...
>>> acpi-test: Warning! APIC binary file mismatch. Actual [aml:/tmp/aml-DLHA51], Expected [aml:tests/data/acpi/pc/APIC].
>>> See source file tests/qtest/bios-tables-test.c for instructions on how to update expected files.
>>> Using iasl: /usr/bin/iasl
>>> acpi-test: Warning! APIC mismatch. Actual [asl:/tmp/asl-L9GA51.dsl, aml:/tmp/aml-DLHA51], Expected [asl:/tmp/asl-10EA51.dsl, aml:tests/data/acpi/pc/APIC].
>>> 
>>> $ QTEST_QEMU_BINARY=./qemu-system-x86_64 V=2 IASL_PATH=/home/anisinha/workspace/acpica/generate/unix/bin/iasl ./tests/qtest/bios-tables-test
>>> ...
>>> acpi-test: Warning! APIC binary file mismatch. Actual [aml:/tmp/aml-5CQ341], Expected [aml:tests/data/acpi/pc/APIC].
>>> See source file tests/qtest/bios-tables-test.c for instructions on how to update expected files.
>>> User has provided an iasl path, using that: /home/anisinha/workspace/acpica/generate/unix/bin/iasl
>>> acpi-test: Warning! APIC mismatch. Actual [asl:/tmp/asl-2GQ341.dsl, aml:/tmp/aml-5CQ341], Expected [asl:/tmp/asl-IBR341.dsl, aml:tests/data/acpi/pc/APIC].
>>> 
>>> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
>>> index 7fd88b0e9c..37e8e484cb 100644
>>> --- a/tests/qtest/bios-tables-test.c
>>> +++ b/tests/qtest/bios-tables-test.c
>>> @@ -440,7 +440,12 @@ static void test_acpi_asl(test_data *data)
>>>    AcpiSdtTable *sdt, *exp_sdt;
>>>    test_data exp_data = {};
>>>    gboolean exp_err, err, all_tables_match = true;
>>> +    const char *user_iasl_path = getenv("IASL_PATH");
>>> 
>>> +    /* if user has provided a path to iasl, use that */
>>> +    if (user_iasl_path) {
>>> +        iasl = user_iasl_path;
>>> +    }
>>>    exp_data.tables = load_expected_aml(data);
>>>    dump_aml_files(data, false);
>>>    for (i = 0; i < data->tables->len; ++i) {
>>> @@ -473,6 +478,15 @@ static void test_acpi_asl(test_data *data)
>>>            continue;
>>>        }
>>> 
>>> +        if (verbosity_level >= 2) {
>>> +            if (user_iasl_path) {
>>> +                fprintf(stderr, "User has provided an iasl path," \
>>> +                        "using that: %s\n", user_iasl_path);
>>> +            } else {
>>> +                fprintf(stderr, "Using iasl: %s\n", iasl);
>>> +            }
>>> +        }
>>> +
>>>        err = load_asl(data->tables, sdt);
>>>        asl = normalize_asl(sdt->asl);
>>> 
>>> -- 
>>> 2.39.1
diff mbox series

Patch

diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
index 7fd88b0e9c..37e8e484cb 100644
--- a/tests/qtest/bios-tables-test.c
+++ b/tests/qtest/bios-tables-test.c
@@ -440,7 +440,12 @@  static void test_acpi_asl(test_data *data)
     AcpiSdtTable *sdt, *exp_sdt;
     test_data exp_data = {};
     gboolean exp_err, err, all_tables_match = true;
+    const char *user_iasl_path = getenv("IASL_PATH");
 
+    /* if user has provided a path to iasl, use that */
+    if (user_iasl_path) {
+        iasl = user_iasl_path;
+    }
     exp_data.tables = load_expected_aml(data);
     dump_aml_files(data, false);
     for (i = 0; i < data->tables->len; ++i) {
@@ -473,6 +478,15 @@  static void test_acpi_asl(test_data *data)
             continue;
         }
 
+        if (verbosity_level >= 2) {
+            if (user_iasl_path) {
+                fprintf(stderr, "User has provided an iasl path," \
+                        "using that: %s\n", user_iasl_path);
+            } else {
+                fprintf(stderr, "Using iasl: %s\n", iasl);
+            }
+        }
+
         err = load_asl(data->tables, sdt);
         asl = normalize_asl(sdt->asl);