Re: [PATCH-for-10.1 v3 6/9] qtest/bios-tables-test: Whitelist aarch64/virt 'its_off' variant blobs

[Igor Mammedov <imammedo@redhat.com>]

On Fri, 4 Apr 2025 00:01:22 -0300
Gustavo Romero <gustavo.romero@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.romero@linaro.org>
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@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.

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.

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