Hi Igor,
On 4/9/25 11:05, Igor Mammedov wrote:
On Fri, 4 Apr 2025 00:01:22 -0300
Gustavo Romero <gustavo.rom...@linaro.org> wrote:
Hi Phil,
On 4/3/25 17:40, Philippe Mathieu-Daudé wrote:
We are going to fix the test_acpi_aarch64_virt_tcg_its_off()
test. In preparation, copy the ACPI tables which will be
altered as 'its_off' variants, and whitelist them.
Reviewed-by: Gustavo Romero <gustavo.rom...@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <phi...@linaro.org>
---
tests/qtest/bios-tables-test-allowed-diff.h | 3 +++
tests/qtest/bios-tables-test.c | 1 +
tests/data/acpi/aarch64/virt/APIC.its_off | Bin 0 -> 184 bytes
tests/data/acpi/aarch64/virt/FACP.its_off | Bin 0 -> 276 bytes
tests/data/acpi/aarch64/virt/IORT.its_off | Bin 0 -> 236 bytes
5 files changed, 4 insertions(+)
create mode 100644 tests/data/acpi/aarch64/virt/APIC.its_off
create mode 100644 tests/data/acpi/aarch64/virt/FACP.its_off
create mode 100644 tests/data/acpi/aarch64/virt/IORT.its_off
diff --git a/tests/qtest/bios-tables-test-allowed-diff.h
b/tests/qtest/bios-tables-test-allowed-diff.h
index dfb8523c8bf..3421dd5adf3 100644
--- a/tests/qtest/bios-tables-test-allowed-diff.h
+++ b/tests/qtest/bios-tables-test-allowed-diff.h
@@ -1 +1,4 @@
/* List of comma-separated changed AML files to ignore */
+"tests/data/acpi/aarch64/virt/APIC.its_off",
+"tests/data/acpi/aarch64/virt/FACP.its_off",
+"tests/data/acpi/aarch64/virt/IORT.its_off",
I think your first approach is the correct one: you add the blobs
when adding the new test, so they would go into patch 5/9 in this series,
making the test pass without adding anything to bios-tables-test-allowed-diff.h.
Then in this patch only add the APIC.its_off table to the
bios-tables-test-allowed-diff.h
since that's the table that changes when the fix is in place, as you did in:
if APIC.its_off is the only one that's changing, but FACP/IORT blobs are the
same
as suffix-less blobs, one can omit copying FACP/IORT as test harness will
fallback
to suffix-less blob if the one with suffix isn't found.
OK. Just clarifying and for the records, this is not the case for this series
if blobs are different from defaults then create empty blobs and whitelist them
in the same patch
then do your changes and then update blobs & wipeout withe list.
Thanks for confirming it. That's what I suggested to Phil in my first review
and what
I understood from the prescription in bios-tables-test.c.
However, on second thoughts, for this particular series, isn't it better to
have the following commit sequence instead:
1) Add the new test and the new blobs that make the test pass, i.e.
APIC.suffix, FACP.suffix, and IORT.suffix (they are different than the default
suffix-less blobs)
2) Whitelist only the APIC.suffix since that's the table that will change with
the fix
3) Add the fix (which changes the APIC table so a new APIC.suffix blob is
needed and also stops generating the IORT table, so no more IORT.suffix blob is
necessary)
4) Finally, update only the APIC.suffix blob and remove the IORT.suffix blob
and wipe out the whitelist
This way:
A) It's clear that only ACPI blob changed with the fix, because there is no
addition of a FACP.suffix blob in 4) (it remains the same)
B) It's clear that the IORT table is removed with the fix and is not relevant
anymore for the test
What do you think?
Cheers,
Gustavo
Phil,
the process is described in doc comment at the top of
tests/qtest/bios-tables-test.c
https://mail.gnu.org/archive/html/qemu-devel/2025-03/msg07082.html
Plus, adding the blobs, which are actually related to the test in the other
patch, and ignoring them at the same time looks confusing to me. I understand
that only the blob that changes (APIC.its_off) with the fix should be
temporarily
ignored and, later, updated, as in your first series.
Cheers,
Gustavo
diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
index baaf199e01c..55366bf4956 100644
--- a/tests/qtest/bios-tables-test.c
+++ b/tests/qtest/bios-tables-test.c
@@ -2151,6 +2151,7 @@ static void test_acpi_aarch64_virt_tcg_its_off(void)
test_data data = {
.machine = "virt",
.arch = "aarch64",
+ .variant = ".its_off",
.tcg_only = true,
.uefi_fl1 = "pc-bios/edk2-aarch64-code.fd",
.uefi_fl2 = "pc-bios/edk2-arm-vars.fd",
diff --git a/tests/data/acpi/aarch64/virt/APIC.its_off
b/tests/data/acpi/aarch64/virt/APIC.its_off
new file mode 100644
index
0000000000000000000000000000000000000000..c37d05d6e05805304f10afe73eb7cb9100fcccfa
GIT binary patch
literal 184
zcmZ<^@O0k6z`($=+{xeBBUr&HBEVSz2pEB4AU24G0Uik$i-7~iVgWL^17JJ`2AFzr
bgb+@aBn}xq0gwb2)Q)cq{30-g9B_L93G4|0
literal 0
HcmV?d00001
diff --git a/tests/data/acpi/aarch64/virt/FACP.its_off
b/tests/data/acpi/aarch64/virt/FACP.its_off
new file mode 100644
index
0000000000000000000000000000000000000000..606dac3fe4b55c31fd68b25d3a4127eeef227434
GIT binary patch
literal 276
zcmZ>BbPf<<WME(uaq@Te2v%^42yj*a0-z8Bhz+8t3j|P&V`N}P6&N^PpsQ~v$aVnZ
CVg~^L
literal 0
HcmV?d00001
diff --git a/tests/data/acpi/aarch64/virt/IORT.its_off
b/tests/data/acpi/aarch64/virt/IORT.its_off
new file mode 100644
index
0000000000000000000000000000000000000000..0fceb820d509e852ca0849baf568a8e93e426738
GIT binary patch
literal 236
zcmebD4+?q1z`(#9?&R<65v<@85#X!<1dKp25F11@1F-=RgMkDCNC*yK9F_<M77!bR
zUBI%eoFED&4;F$FSwK1)h;xBB2Py`m{{M%tVD>TjFfcO#g+N#Zh@s|zoCF3AP#UU@
R!2`+%Dg6Hr$N|zYvjDIZ5CH%H
literal 0
HcmV?d00001
