[PATCH-for-10.0 2/5] qtest/bios-tables-test: Whitelist aarch64/virt/APIC.its_off blob

Philippe Mathieu-Daudé posted 5 patches 7 months, 2 weeks ago
[PATCH-for-10.0 2/5] qtest/bios-tables-test: Whitelist aarch64/virt/APIC.its_off blob
Posted by Philippe Mathieu-Daudé 7 months, 2 weeks ago
Prepare for ACPI table change in aarch64/virt/APIC.its_off.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 tests/qtest/bios-tables-test-allowed-diff.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
index dfb8523c8bf..bfc4d601243 100644
--- a/tests/qtest/bios-tables-test-allowed-diff.h
+++ b/tests/qtest/bios-tables-test-allowed-diff.h
@@ -1 +1,2 @@
 /* List of comma-separated changed AML files to ignore */
+"tests/data/acpi/aarch64/virt/APIC.its_off",
-- 
2.47.1


Re: [PATCH-for-10.0 2/5] qtest/bios-tables-test: Whitelist aarch64/virt/APIC.its_off blob
Posted by Gustavo Romero 7 months, 2 weeks ago
Hi Phil,

On 3/31/25 19:12, Philippe Mathieu-Daudé wrote:
> Prepare for ACPI table change in aarch64/virt/APIC.its_off.

The comment could be smth like:

Ignore APIC.its_off expected table (blob) for now until
we update it later, after fixing the code that generates
this table correctly.

?


> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   tests/qtest/bios-tables-test-allowed-diff.h | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
> index dfb8523c8bf..bfc4d601243 100644
> --- a/tests/qtest/bios-tables-test-allowed-diff.h
> +++ b/tests/qtest/bios-tables-test-allowed-diff.h
> @@ -1 +1,2 @@
>   /* List of comma-separated changed AML files to ignore */
> +"tests/data/acpi/aarch64/virt/APIC.its_off",

I think this patch should be merged into 1/2, accordingly to my
comment in 1/5. FACP and IORT .its_off files should be added to the
list as well.

Btw, IMHO the name of this header is a tad misleading, because actually
"allowed-diff" means that "we allow the machine's table to be different
from the tables listed in this header", so it doesn't look like an
allowlist (whitelist), it works more like an ignore list?


Cheers,
Gustavo

Re: [PATCH-for-10.0 2/5] qtest/bios-tables-test: Whitelist aarch64/virt/APIC.its_off blob
Posted by Philippe Mathieu-Daudé 7 months, 2 weeks ago
On 2/4/25 08:43, Gustavo Romero wrote:
> Hi Phil,
> 
> On 3/31/25 19:12, Philippe Mathieu-Daudé wrote:
>> Prepare for ACPI table change in aarch64/virt/APIC.its_off.
> 
> The comment could be smth like:
> 
> Ignore APIC.its_off expected table (blob) for now until
> we update it later, after fixing the code that generates
> this table correctly.
> 
> ?
> 
> 
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   tests/qtest/bios-tables-test-allowed-diff.h | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/ 
>> qtest/bios-tables-test-allowed-diff.h
>> index dfb8523c8bf..bfc4d601243 100644
>> --- a/tests/qtest/bios-tables-test-allowed-diff.h
>> +++ b/tests/qtest/bios-tables-test-allowed-diff.h
>> @@ -1 +1,2 @@
>>   /* List of comma-separated changed AML files to ignore */
>> +"tests/data/acpi/aarch64/virt/APIC.its_off",
> 
> I think this patch should be merged into 1/2, accordingly to my
> comment in 1/5. FACP and IORT .its_off files should be added to the
> list as well.

No, otherwise the test added in previous patch fails.

> 
> Btw, IMHO the name of this header is a tad misleading, because actually
> "allowed-diff" means that "we allow the machine's table to be different
> from the tables listed in this header", so it doesn't look like an
> allowlist (whitelist), it works more like an ignore list?
> 
> 
> Cheers,
> Gustavo


Re: [PATCH-for-10.0 2/5] qtest/bios-tables-test: Whitelist aarch64/virt/APIC.its_off blob
Posted by Gustavo Romero 7 months, 2 weeks ago
Hi Phil,

On 4/2/25 07:31, Philippe Mathieu-Daudé wrote:
> On 2/4/25 08:43, Gustavo Romero wrote:
>> Hi Phil,
>>
>> On 3/31/25 19:12, Philippe Mathieu-Daudé wrote:
>>> Prepare for ACPI table change in aarch64/virt/APIC.its_off.
>>
>> The comment could be smth like:
>>
>> Ignore APIC.its_off expected table (blob) for now until
>> we update it later, after fixing the code that generates
>> this table correctly.
>>
>> ?
>>
>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>>   tests/qtest/bios-tables-test-allowed-diff.h | 1 +
>>>   1 file changed, 1 insertion(+)
>>>
>>> diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/ qtest/bios-tables-test-allowed-diff.h
>>> index dfb8523c8bf..bfc4d601243 100644
>>> --- a/tests/qtest/bios-tables-test-allowed-diff.h
>>> +++ b/tests/qtest/bios-tables-test-allowed-diff.h
>>> @@ -1 +1,2 @@
>>>   /* List of comma-separated changed AML files to ignore */
>>> +"tests/data/acpi/aarch64/virt/APIC.its_off",
>>
>> I think this patch should be merged into 1/2, accordingly to my
>> comment in 1/5. FACP and IORT .its_off files should be added to the
>> list as well.
> 
> No, otherwise the test added in previous patch fails.

I can't see how adding the tests to the list in
tests/qtest/bios-tables-test-allowed-diff.h can cause any failure.
The list in this header file works as a "ignore list", so even if
the .its_off blobs in 1/5 were empty (for instance) the test would
pass if they are in this list.

That said, as per my comments in 1/5, this preparation is correct
to me: the fix will cause changes to APIC table so the current
expected blob (committed) needs to be ignored until it gets updated
later, in 5/5.

Reviewed-by: Gustavo Romero <gustavo.romero@linaro.org>


Cheers,
Gustavo