Re: [PATCH 2/2] qemuxmlconftest: Add tests for the ACPI stripping hack on s390
On Thu, Aug 01, 2024 at 12:06:57PM GMT, Peter Krempa wrote:
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:
> > + <!-- machine type doesn't matter as long as it has no implicit USB
-->
> > + <type arch='aarch64'
machine='borzoi'>hvm</type>
>
> 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.
That explains it.
> > + <features>
> > + <acpi/>
> > + </features>
> > + <os>
> > + <type arch='riscv64'
machine='virt'>hvm</type>
> > + </os>
>
> 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.
I have a mild preference for the former. Keep in mind that there's a
very real chance that, in a while, I will forget this conversation,
re-discover the out-of-order elements, and post a patch shuffling
them around :D
> 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.
In that case it's very intentional: the *-minimal test cases are
supposed to show what kind of controllers and devices libvirt will
automatically add when presented with an XML that doesn't contain
any. The fact that some of the output files have memballoon and some
don't is exactly the point.
> > + 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.
Makes sense. A small comment would be appreciated.
--
Andrea Bolognani / Red Hat / Virtualization