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.
