> On 17-May-2023, at 8:06 PM, Michael S. Tsirkin <m...@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 <m...@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 <anisi...@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
>
