| Message ID | 20250331221239.87150-2-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/arm/virt-acpi: Do not advertise disabled GIC ITS in MADT table | expand |
Hi Phil, On 3/31/25 19:12, Philippe Mathieu-Daudé wrote: > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> Please, put commit message (body) into the commits. For example, the commit message here could quickly explain that the FACP table changed because virtualization=on (due to PSCI conduit). I'm assuming virtualization is set to on because gic-version=max and so GICv4 is selected for testing. It also could be that we want to exercise its=off when Arm Virtualization Extensions are enabled, which is the common use case (I understand that ITS can be used also with virtualization=off). Finally, the commit message could mention at the end which struct vanishes in APIC table and why IO remapping table is affected by ITS on/off. A good commit message always help in code spelunking :) > --- > tests/qtest/bios-tables-test.c | 22 ++++++++++++++++++++++ > 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 > 4 files changed, 22 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.c b/tests/qtest/bios-tables-test.c > index 0a333ec4353..55366bf4956 100644 > --- a/tests/qtest/bios-tables-test.c > +++ b/tests/qtest/bios-tables-test.c > @@ -2146,6 +2146,26 @@ static void test_acpi_aarch64_virt_tcg_topology(void) > free_test_data(&data); > } > > +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", > + .cd = "tests/data/uefi-boot-images/bios-tables-test.aarch64.iso.qcow2", > + .ram_start = 0x40000000ULL, > + .scan_len = 128ULL * 1024 * 1024, > + }; > + > + test_acpi_one("-cpu cortex-a57 " > + "-M virtualization=on,secure=off " > + "-M gic-version=max,its=off,iommu=smmuv3", &data); > + free_test_data(&data); > +} > + > static void test_acpi_q35_viot(void) > { > test_data data = { > @@ -2577,6 +2597,8 @@ int main(int argc, char *argv[]) > test_acpi_aarch64_virt_tcg_acpi_hmat); > qtest_add_func("acpi/virt/topology", > test_acpi_aarch64_virt_tcg_topology); > + qtest_add_func("acpi/virt/its_off", > + test_acpi_aarch64_virt_tcg_its_off); > qtest_add_func("acpi/virt/numamem", > test_acpi_aarch64_virt_tcg_numamem); > qtest_add_func("acpi/virt/memhp", test_acpi_aarch64_virt_tcg_memhp); > 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 > I think the prescription for the acrobatics to update the ACPI expected tables says the blobs here should be empty (blob files are added empty) and at the same time they are listed in tests/qtest/bios-tables-test-allowed-diff.h: * 1. add empty files for new tables, if any, under tests/data/acpi * 2. list any changed files in tests/qtest/bios-tables-test-allowed-diff.h * 3. commit the above *before* making changes that affect the tables (from tests/qtest/bios-tables-test.c header) If that's correct, this patch should be merged with the following one (2/5) and IORT.its_off and FACP.its_off should also be listed in tests/qtest/bios-tables-test-allowed-diff.h so the empty blobs won't trigger a test failure. Then patch 5/5 should add the expected/updated blobs and drop the *.its_off from bios-tables-test-allowed-diff.h. Patches 3/5 and 4/5 are sandwiched between (1/5 + 2/5) and (5/5). At least that's what I get from: * The resulting patchset/pull request then looks like this: * - patch 1: list changed files in tests/qtest/bios-tables-test-allowed-diff.h. * - patches 2 - n: real changes, may contain multiple patches. * - patch n + 1: update golden master binaries and empty tests/qtest/bios-tables-test-allowed-diff.h Otherwise the change looks good. Cheers, Gustavo
On 2/4/25 08:41, Gustavo Romero wrote: > Hi Phil, > > On 3/31/25 19:12, Philippe Mathieu-Daudé wrote: >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > > Please, put commit message (body) into the commits. > > For example, the commit message here could quickly explain that the FACP > table > changed because virtualization=on (due to PSCI conduit). I'm assuming > virtualization is set to on because gic-version=max and so GICv4 is > selected for > testing. It also could be that we want to exercise its=off when Arm > Virtualization > Extensions are enabled, which is the common use case (I understand that ITS > can be used also with virtualization=off). > > Finally, the commit message could mention at the end which struct > vanishes in APIC table and why IO remapping table is affected by > ITS on/off. > > A good commit message always help in code spelunking :) I simply copied the reproducer from the issue, so I'll mention that. (https://gitlab.com/qemu-project/qemu/-/issues/2886) > > >> --- >> tests/qtest/bios-tables-test.c | 22 ++++++++++++++++++++++ >> 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 >> 4 files changed, 22 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.c b/tests/qtest/bios-tables- >> test.c >> index 0a333ec4353..55366bf4956 100644 >> --- a/tests/qtest/bios-tables-test.c >> +++ b/tests/qtest/bios-tables-test.c >> @@ -2146,6 +2146,26 @@ static void >> test_acpi_aarch64_virt_tcg_topology(void) >> free_test_data(&data); >> } >> +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", >> + .cd = "tests/data/uefi-boot-images/bios-tables- >> test.aarch64.iso.qcow2", >> + .ram_start = 0x40000000ULL, >> + .scan_len = 128ULL * 1024 * 1024, >> + }; >> + >> + test_acpi_one("-cpu cortex-a57 " >> + "-M virtualization=on,secure=off " >> + "-M gic-version=max,its=off,iommu=smmuv3", &data); >> + free_test_data(&data); >> +} >> + >> static void test_acpi_q35_viot(void) >> { >> test_data data = { >> @@ -2577,6 +2597,8 @@ int main(int argc, char *argv[]) >> test_acpi_aarch64_virt_tcg_acpi_hmat); >> qtest_add_func("acpi/virt/topology", >> test_acpi_aarch64_virt_tcg_topology); >> + qtest_add_func("acpi/virt/its_off", >> + test_acpi_aarch64_virt_tcg_its_off); >> qtest_add_func("acpi/virt/numamem", >> test_acpi_aarch64_virt_tcg_numamem); >> qtest_add_func("acpi/virt/memhp", >> test_acpi_aarch64_virt_tcg_memhp); >> 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 >> > > I think the prescription for the acrobatics to update the ACPI expected > tables says the blobs here should be empty (blob files are added empty) > and at the same time they are listed in tests/qtest/bios-tables-test- > allowed-diff.h: > > * 1. add empty files for new tables, if any, under tests/data/acpi > * 2. list any changed files in tests/qtest/bios-tables-test-allowed- > diff.h > * 3. commit the above *before* making changes that affect the tables > > (from tests/qtest/bios-tables-test.c header) > > If that's correct, this patch should be merged with the following one > (2/5) and > IORT.its_off and FACP.its_off should also be listed in > tests/qtest/bios-tables-test-allowed-diff.h so the empty blobs won't > trigger > a test failure. I shouldn't have included the ACPI data in this patch but in the following. IIUC, if no data/$TABLE.$variant, then the generic data/$TABLE is used. > > Then patch 5/5 should add the expected/updated blobs and drop the > *.its_off from > bios-tables-test-allowed-diff.h. Patches 3/5 and 4/5 are sandwiched > between (1/5 + 2/5) and (5/5). > > At least that's what I get from: > > * The resulting patchset/pull request then looks like this: > * - patch 1: list changed files in tests/qtest/bios-tables-test- > allowed-diff.h. > * - patches 2 - n: real changes, may contain multiple patches. > * - patch n + 1: update golden master binaries and empty tests/qtest/ > bios-tables-test-allowed-diff.h > > Otherwise the change looks good. > > > Cheers, > Gustavo
diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c index 0a333ec4353..55366bf4956 100644 --- a/tests/qtest/bios-tables-test.c +++ b/tests/qtest/bios-tables-test.c @@ -2146,6 +2146,26 @@ static void test_acpi_aarch64_virt_tcg_topology(void) free_test_data(&data); } +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", + .cd = "tests/data/uefi-boot-images/bios-tables-test.aarch64.iso.qcow2", + .ram_start = 0x40000000ULL, + .scan_len = 128ULL * 1024 * 1024, + }; + + test_acpi_one("-cpu cortex-a57 " + "-M virtualization=on,secure=off " + "-M gic-version=max,its=off,iommu=smmuv3", &data); + free_test_data(&data); +} + static void test_acpi_q35_viot(void) { test_data data = { @@ -2577,6 +2597,8 @@ int main(int argc, char *argv[]) test_acpi_aarch64_virt_tcg_acpi_hmat); qtest_add_func("acpi/virt/topology", test_acpi_aarch64_virt_tcg_topology); + qtest_add_func("acpi/virt/its_off", + test_acpi_aarch64_virt_tcg_its_off); qtest_add_func("acpi/virt/numamem", test_acpi_aarch64_virt_tcg_numamem); qtest_add_func("acpi/virt/memhp", test_acpi_aarch64_virt_tcg_memhp); 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
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- tests/qtest/bios-tables-test.c | 22 ++++++++++++++++++++++ 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 4 files changed, 22 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 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