Re: [RFC 0/3] qtest: pick tests that require KVM at runtime

[Igor Mammedov]

On Fri, 18 Jun 2021 14:43:46 +0200
Claudio Fontana <cfontana@suse.de> wrote:

> On 6/18/21 1:26 PM, Igor Mammedov wrote:
> > On Thu, 17 Jun 2021 18:49:17 +0200
> > Claudio Fontana <cfontana@suse.de> wrote:
> >   
> >> On 6/16/21 5:24 PM, Igor Mammedov wrote:  
> >>>
> >>> Sometimes it's necessary to execute a test that depends on KVM,
> >>> however qtest is not aware if tested QEMU binary supports KVM
> >>> on the host it the test is executed.    
> >>
> >> Hello,
> >>
> >> It seems to me that we are constantly re-implementing the same feature 
> >> with slight variations?
> >>
> >> Didn't we have a generic series to introduce qtest_has_accel() from 
> >> Philippe before?  
> > It's mentioned in cover letter (PS: part) and in [1/3] with rationale
> > why this was posted.  
> 
> Thought it was separate, but now I see that it uses query-accel underneath.
> 
> Seems strange to add another check to do the same thing, it may point to 
> qtest_has_accel() needing some update?
> You mention it is time consuming to use qtest_has_accel(), have you measured 
> an important overhead?
> With qtest_has_accel() not even being committed yet, is it already necessary 
> to work around it because it's too slow? 

Tests are already take a lot of time as is, so I'd try to avoid slowing
them down.

proposed qtest_has_accel() requires spawning QEMU to probe, which is slow.
Worst case would be:
 = qemu startup time * number of checks * number of targets

It's fine to run occasionally, I can take a coffee break while tests run.
But put it in context of CI and it multiplies by the number of push requests
and starts to eat not only time but also limited CI resources.

In current form qtest_has_accel() is only marginally better functionality
wise, as it reports all built in accelerators while qtest_has_kvm() accounts
only for KVM.

qtest_has_kvm() is collecting info about built-in accelerators at
configure/build time and that probably could be extended to other
accelerators (not a thing that I'm interested in at the moment).
So it could be extended to support the same accelerators
as currently proposed qtest_has_accel().

Given a less expensive approach exists, the qtest_has_accel()
in its current form might be not justifiable. 

   
> >> Does this series work with --disable-kvm builds? (TCG-only builds?)  
> > I'll test. But on the first glance it should work without issues.
> > (i.e. kvm only tests will be skipped).
> >   
> >>
> >> Thanks,
> >>
> >> CLaudio
> >>
> >>  
> >>>
> >>> For an example:
> >>>  test q35 machine with intel_iommu
> >>>  This test will run only is KVM is available and fail
> >>>  to start QEMU if it fallsback to TCG, thus failing whole test.
> >>>  So if test is executed in VM where nested KVM is not enabled
> >>>  or on other than x86 host, it will break 'make check-qtest'
> >>>
> >>> Series adds a lightweight qtest_has_kvm() check, which abuses
> >>> build system and should help to avoid running KVM only tests
> >>> on hosts that do not support it.
> >>>
> >>> PS:
> >>> there is an alternative 'query-accels' QMP command proposal
> >>> https://patchwork.kernel.org/project/qemu-devel/patch/20210503211020.894589-3-philmd@redhat.com/
> >>> which I think is more robust compared to qtest_has_kvm() and
> >>> could be extended to take into account machine type.
> >>> But it's more complex and what I dislike about it most,
> >>> it requires execution of 'probing' QEMU instance to find
> >>> execute 'query-accels' QMP command, which is rather resource
> >>> consuming. So I'd use query-accels approach only when it's
> >>> the only possible option to minimize load on CI systems.
> >>>
> >>> Igor Mammedov (2):
> >>>   tests: acpi: q35: test for x2APIC entries in SRAT
> >>>   tests: acpi: update expected tables blobs
> >>>
> >>> root (1):
> >>>   tests: qtest: add qtest_has_kvm() to check if tested bynary supports
> >>>     KVM
> >>>
> >>>  tests/qtest/libqos/libqtest.h    |   7 +++++++
> >>>  meson.build                      |   1 +
> >>>  tests/data/acpi/q35/APIC.numamem | Bin 0 -> 2686 bytes
> >>>  tests/data/acpi/q35/DSDT.numamem | Bin 7865 -> 35222 bytes
> >>>  tests/data/acpi/q35/FACP.numamem | Bin 0 -> 244 bytes
> >>>  tests/data/acpi/q35/SRAT.numamem | Bin 224 -> 5080 bytes
> >>>  tests/qtest/bios-tables-test.c   |  10 +++++++---
> >>>  tests/qtest/libqtest.c           |  20 ++++++++++++++++++++
> >>>  8 files changed, 35 insertions(+), 3 deletions(-)
> >>>  create mode 100644 tests/data/acpi/q35/APIC.numamem
> >>>  create mode 100644 tests/data/acpi/q35/FACP.numamem
> >>>     
> >>  
> >   
>