| Message ID | 20250403204029.47958-7-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/arm: GIC ITS=off ACPI tables fixes | expand |
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: 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 >
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 > > >
Hi Igor, On 4/9/25 11:05, Igor Mammedov wrote: > 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. 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 >>> >> >
On Wed, 9 Apr 2025 12:49:36 -0300 Gustavo Romero <gustavo.romero@linaro.org> wrote: > Hi Igor, > > On 4/9/25 11:05, Igor Mammedov wrote: > > 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. > > 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) blobs should be a separate commit (that way it's easier for maintainer to rebase them, if they clash during merge with some other change. > 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 I'd just mention it in commit log so that later no one would wonder why we are adding and then removing tables As for the rest of suggestions, it looks fine to me. > > 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 > >>> > >> > > >
Hi Igor, On 4/10/25 03:50, Igor Mammedov wrote: > On Wed, 9 Apr 2025 12:49:36 -0300 > Gustavo Romero <gustavo.romero@linaro.org> wrote: > >> Hi Igor, >> >> On 4/9/25 11:05, Igor Mammedov wrote: >>> 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. >> >> 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) > > blobs should be a separate commit (that way it's easier for maintainer to rebase them, > if they clash during merge with some other change. I see. What is a bit confusing here is that the series consists in one blob addition act (for the new test) and one blob update/removal act (after the fix). >> 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 > > I'd just mention it in commit log so that later no one would wonder why we are adding and then removing tables > > As for the rest of suggestions, it looks fine to me. Well, 2) won't make sense anymore since APIC.suffix would be already in the whitelist in the previous patch that added the empty blobs. Since there won't be a commit that adds _only_ the APIC.suffix to the whitelist, in preparation for the fix, this info is "lost" in the series, even tho it's possible to mention in the commit message. Hence, what I think is not ideal from a maintainer's/reviewer's perspective, is that in one commit all the blobs are updated/removed at once, which is confusing because the fix did not touch the FACP table (for instance) and this table is updated with APIC and with the removal of IORT, altogether, in the last commit. So, for this series, which adds new blobs and _also_ updates and removes some of them, how about the following organization: - Patch 1 : Add the new test, add the empty blobs *.suffix files, whitelist such a blobs - Patch 2 : Update the blobs in Patch 1 with the ones that make the new test pass and remove them from the whitelist - Patch 3 : Add the APIC.suffix blob to the whitelist (the table that changes due to the fix) - Patch 4 - n : Fix(es) - Patch (n+1) : Update the APIC.suffix blob, remove IORT.suffix blob, and remove the APIC.suffix blob from the whitelist * Add the APIC diff to the commit log * Mention in the commit log that IORT.suffix is removed because IORT table is no long generated after the fix This way: a) no commit fails the test and b) blobs are added/updated/removed in separate commits What do you think? Cheers, Gustavo
Hi Igor and Michael, On 4/10/25 13:22, Gustavo Romero wrote: > Hi Igor, > > On 4/10/25 03:50, Igor Mammedov wrote: >> On Wed, 9 Apr 2025 12:49:36 -0300 >> Gustavo Romero <gustavo.romero@linaro.org> wrote: >> >>> Hi Igor, >>> >>> On 4/9/25 11:05, Igor Mammedov wrote: >>>> 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. >>> >>> 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) >> >> blobs should be a separate commit (that way it's easier for maintainer to rebase them, >> if they clash during merge with some other change. > > I see. What is a bit confusing here is that the series consists in > one blob addition act (for the new test) and one blob update/removal act (after the fix). > > >>> 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 >> >> I'd just mention it in commit log so that later no one would wonder why we are adding and then removing tables >> >> As for the rest of suggestions, it looks fine to me. > > Well, 2) won't make sense anymore since APIC.suffix would be already in the > whitelist in the previous patch that added the empty blobs. Since there won't > be a commit that adds _only_ the APIC.suffix to the whitelist, in preparation > for the fix, this info is "lost" in the series, even tho it's possible to > mention in the commit message. > > Hence, what I think is not ideal from a maintainer's/reviewer's perspective, > is that in one commit all the blobs are updated/removed at once, which is > confusing because the fix did not touch the FACP table (for instance) and > this table is updated with APIC and with the removal of IORT, altogether, > in the last commit. > > So, for this series, which adds new blobs and _also_ updates and removes some > of them, how about the following organization: > > - Patch 1 : Add the new test, add the empty blobs *.suffix files, whitelist such a blobs > - Patch 2 : Update the blobs in Patch 1 with the ones that make the new test pass and remove them from the whitelist > > - Patch 3 : Add the APIC.suffix blob to the whitelist (the table that changes due to the fix) > - Patch 4 - n : Fix(es) > - Patch (n+1) : Update the APIC.suffix blob, remove IORT.suffix blob, and remove the APIC.suffix blob from the whitelist > * Add the APIC diff to the commit log > * Mention in the commit log that IORT.suffix is removed because IORT table is no long generated after the fix > > This way: a) no commit fails the test and b) blobs are added/updated/removed in separate commits > > What do you think? I’d really appreciate it if you could confirm whether this organization makes sense. Cheers, Gustavo
On Thu, Apr 17, 2025 at 06:06:14PM -0300, Gustavo Romero wrote: > Hi Igor and Michael, > > On 4/10/25 13:22, Gustavo Romero wrote: > > Hi Igor, > > > > On 4/10/25 03:50, Igor Mammedov wrote: > > > On Wed, 9 Apr 2025 12:49:36 -0300 > > > Gustavo Romero <gustavo.romero@linaro.org> wrote: > > > > > > > Hi Igor, > > > > > > > > On 4/9/25 11:05, Igor Mammedov wrote: > > > > > 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. > > > > > > > > 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) > > > > > > blobs should be a separate commit (that way it's easier for maintainer to rebase them, > > > if they clash during merge with some other change. > > > > I see. What is a bit confusing here is that the series consists in > > one blob addition act (for the new test) and one blob update/removal act (after the fix). > > > > > > > > 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 > > > > > > I'd just mention it in commit log so that later no one would wonder why we are adding and then removing tables > > > > > > As for the rest of suggestions, it looks fine to me. > > > > Well, 2) won't make sense anymore since APIC.suffix would be already in the > > whitelist in the previous patch that added the empty blobs. Since there won't > > be a commit that adds _only_ the APIC.suffix to the whitelist, in preparation > > for the fix, this info is "lost" in the series, even tho it's possible to > > mention in the commit message. > > > > Hence, what I think is not ideal from a maintainer's/reviewer's perspective, > > is that in one commit all the blobs are updated/removed at once, which is > > confusing because the fix did not touch the FACP table (for instance) and > > this table is updated with APIC and with the removal of IORT, altogether, > > in the last commit. > > > > So, for this series, which adds new blobs and _also_ updates and removes some > > of them, how about the following organization: > > > > - Patch 1 : Add the new test, add the empty blobs *.suffix files, whitelist such a blobs > > - Patch 2 : Update the blobs in Patch 1 with the ones that make the new test pass and remove them from the whitelist > > > > - Patch 3 : Add the APIC.suffix blob to the whitelist (the table that changes due to the fix) > > - Patch 4 - n : Fix(es) > > - Patch (n+1) : Update the APIC.suffix blob, remove IORT.suffix blob, and remove the APIC.suffix blob from the whitelist > > * Add the APIC diff to the commit log > > * Mention in the commit log that IORT.suffix is removed because IORT table is no long generated after the fix > > > > This way: a) no commit fails the test and b) blobs are added/updated/removed in separate commits > > > > What do you think? > > I’d really appreciate it if you could confirm whether this organization makes sense. > > > Cheers, > Gustavo looks ok. Igor?
On Thu, 10 Apr 2025 13:22:06 -0300 Gustavo Romero <gustavo.romero@linaro.org> wrote: > Hi Igor, > > On 4/10/25 03:50, Igor Mammedov wrote: > > On Wed, 9 Apr 2025 12:49:36 -0300 > > Gustavo Romero <gustavo.romero@linaro.org> wrote: > > > >> Hi Igor, > >> > >> On 4/9/25 11:05, Igor Mammedov wrote: > >>> 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. > >> > >> 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) > > > > blobs should be a separate commit (that way it's easier for maintainer to rebase them, > > if they clash during merge with some other change. > > I see. What is a bit confusing here is that the series consists in > one blob addition act (for the new test) and one blob update/removal act (after the fix). > > > >> 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 > > > > I'd just mention it in commit log so that later no one would wonder why we are adding and then removing tables > > > > As for the rest of suggestions, it looks fine to me. > > Well, 2) won't make sense anymore since APIC.suffix would be already in the > whitelist in the previous patch that added the empty blobs. Since there won't > be a commit that adds _only_ the APIC.suffix to the whitelist, in preparation > for the fix, this info is "lost" in the series, even tho it's possible to > mention in the commit message. > > Hence, what I think is not ideal from a maintainer's/reviewer's perspective, > is that in one commit all the blobs are updated/removed at once, which is > confusing because the fix did not touch the FACP table (for instance) and > this table is updated with APIC and with the removal of IORT, altogether, > in the last commit. > > So, for this series, which adds new blobs and _also_ updates and removes some > of them, how about the following organization: > > - Patch 1 : Add the new test, add the empty blobs *.suffix files, whitelist such a blobs > - Patch 2 : Update the blobs in Patch 1 with the ones that make the new test pass and remove them from the whitelist > > - Patch 3 : Add the APIC.suffix blob to the whitelist (the table that changes due to the fix) > - Patch 4 - n : Fix(es) 3 is not bynanry so it can be folded into 4 or be a separate patch (either way works for me) > - Patch (n+1) : Update the APIC.suffix blob, remove IORT.suffix blob, and remove the APIC.suffix blob from the whitelist > * Add the APIC diff to the commit log > * Mention in the commit log that IORT.suffix is removed because IORT table is no long generated after the fix > > This way: a) no commit fails the test and b) blobs are added/updated/removed in separate commits > > What do you think? LGTM > > Cheers, > Gustavo >
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", 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