Re: [PATCH 2/2] qemuxmlconftest: Add tests for the ACPI stripping hack on s390
On Thu, Aug 01, 2024 at 02:46:34 -0700, Andrea Bolognani wrote:
On Wed, Jul 31, 2024 at 01:02:27PM GMT, Peter Krempa wrote:
> +++ b/tests/qemuxmlconfdata/aarch64-nousb-acpi.xml
> @@ -0,0 +1,18 @@
> +<domain type='kvm'>
> + <name>aarch64test</name>
> + <uuid>6ba410c5-1e5c-4d57-bee7-2228e7ffa32f</uuid>
> + <memory unit='KiB'>1048576</memory>
> + <vcpu placement='static'>1</vcpu>
> + <os>
> + <!-- machine type doesn't matter as long as it has no implicit USB
-->
> + <type arch='aarch64' machine='borzoi'>hvm</type>
> + </os>
The relationship between having implicit USB and being able to use
ACPI is not explained. I could probably figure it out by looking at
the code, but I think it would be better if the comment was expanded
to include this information.
The comment is a leftover from copying aarch64-nousb-minimal. I should
have removed the nousb part and went with a specific machine type name.
> +++ b/tests/qemuxmlconfdata/riscv64-virt-acpi.xml
> @@ -0,0 +1,15 @@
> +<domain type='qemu'>
> + <name>guest</name>
> + <uuid>1ccfd97d-5eb4-478a-bbe6-88d254c16db7</uuid>
> + <memory>4194304</memory>
> + <vcpu>4</vcpu>
> + <features>
> + <acpi/>
> + </features>
> + <os>
> + <type arch='riscv64' machine='virt'>hvm</type>
> + </os>
> + <devices>
> + <emulator>/usr/bin/qemu-system-riscv64</emulator>
> + </devices>
> +</domain>
Here and in a few other input files you've put the <features> element
before the <os> element, which of course our parser is perfectly
capable of handling but it just looks... Off to the human eye :)
Welp. I've added it manually. I can move it or we can say it is testing
the XML parser.
For this input file in particular, you could add
<memballoon model='none'/>
Once again, I've copied riscv64-virt-minimal. Seems like it's missing
there in the first place.
which would make for slighly smaller output files. I'd personally
just include that (as well as the USB equivalent) in every file, just
to ensure minimal hardware while making it easier to compare them.
> +++ b/tests/qemuxmlconftest.c
> @@ -1732,7 +1732,18 @@ mymain(void)
>
> DO_TEST_CAPS_LATEST("input-usbmouse");
> DO_TEST_CAPS_LATEST("input-usbtablet");
> - DO_TEST_CAPS_LATEST("misc-acpi");
> +
> + /* tests for ACPI support handling:
> + * - x86(_64) and aarch attempt
Incomplete sentence? Also it's aarch64.
eh, right
> + * - other architectures base the decision based on how qemu reports
> + * the support for ACPI
> + * - s390x has hack to strip ACPI to preserve migration of old configs */
> + DO_TEST_CAPS_LATEST("x86_64-q35-acpi");
> + DO_TEST_CAPS_ARCH_LATEST_PARSE_ERROR("aarch64-nousb-acpi",
"aarch64");
We should have a positive test for aarch64 which uses the virt
machine type and has ACPI enabled.
We do have that already in the code which is testing also the
appripriate UEFI firmware selection for it, which is the exact reason I
didn't duplicate it here. I can add a comment if you feel like that.