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.

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

-- 
2.27.0

Patchew URL: https://patchew.org/QEMU/20210616152455.1270264-1-imammedo@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20210616152455.1270264-1-imammedo@redhat.com
Subject: [RFC 0/3] qtest: pick tests that require KVM at runtime

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 - [tag update]      patchew/20210616011209.1446045-1-richard.henderson@linaro.org -> patchew/20210616011209.1446045-1-richard.henderson@linaro.org
 * [new tag]         patchew/20210616152455.1270264-1-imammedo@redhat.com -> patchew/20210616152455.1270264-1-imammedo@redhat.com
Switched to a new branch 'test'
99b622f tests: acpi: update expected tables blobs
8aad11d tests: acpi: q35: test for x2APIC entries in SRAT
5bab45e tests: qtest: add qtest_has_kvm() to check if tested binary supports KVM

=== OUTPUT BEGIN ===
1/3 Checking commit 5bab45eeb20a (tests: qtest: add qtest_has_kvm() to check if tested binary supports KVM)
2/3 Checking commit 8aad11d7634d (tests: acpi: q35: test for x2APIC entries in SRAT)
ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/data/acpi/q35/FACP.numamem and tests/qtest/bios-tables-test.c found

ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/data/acpi/q35/FACP.numamem and tests/qtest/bios-tables-test.c found

total: 2 errors, 0 warnings, 39 lines checked

Patch 2/3 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

3/3 Checking commit 99b622fa110f (tests: acpi: update expected tables blobs)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20210616152455.1270264-1-imammedo@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

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?

Does this series work with --disable-kvm builds? (TCG-only builds?)

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
> 

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.

> 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
> >   
> 

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? 
 
> 
>> 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
>>>   
>>
> 

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
> >>>     
> >>  
> >   
> 

Igor Mammedov <imammedo@redhat.com> writes:

> 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().

One minor downside is this forever ties the tests to the build. I have
spoken with people before about the idea of separating the test
artefacts from the build so they can be used either as a) cached test
objects or b) other testing environments, for example verifying the
kernel has not regressed. However we don't do either of those things at
the moment so it's not a major concern.

If the worry is about extending test times by having an extra round trip
of a spawn and query step for every test could we not consider caching
the information somewhere? Really any given binary should only need to
be queried once per run, not per test.

>
> 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
>> >>>     
>> >>  
>> >   
>> 


-- 
Alex Bennée

On 6/22/21 10:07 AM, Alex Bennée wrote:
> Igor Mammedov <imammedo@redhat.com> writes:
>> 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().
> 
> One minor downside is this forever ties the tests to the build. I have
> spoken with people before about the idea of separating the test
> artefacts from the build so they can be used either as a) cached test
> objects or b) other testing environments, for example verifying the
> kernel has not regressed. However we don't do either of those things at
> the moment so it's not a major concern.

This is the feature that is interesting RedHat QE too, run the latest
qtests on various released binaries to compare performances between
releases.

> If the worry is about extending test times by having an extra round trip
> of a spawn and query step for every test could we not consider caching
> the information somewhere? Really any given binary should only need to
> be queried once per run, not per test.

Good idea.

>> 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
>>>>>>     
>>>>>  
>>>>   
>>>
> 
> 

On Tue, 22 Jun 2021 10:22:19 +0200
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> On 6/22/21 10:07 AM, Alex Bennée wrote:
> > Igor Mammedov <imammedo@redhat.com> writes:  
> >> 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().  
> > 
> > One minor downside is this forever ties the tests to the build. I have
> > spoken with people before about the idea of separating the test
> > artefacts from the build so they can be used either as a) cached test
> > objects or b) other testing environments, for example verifying the
> > kernel has not regressed. However we don't do either of those things at
> > the moment so it's not a major concern.  
> 
> This is the feature that is interesting RedHat QE too, run the latest
> qtests on various released binaries to compare performances between
> releases.

Currently qtest is only a build only and hard tied to concrete release tests.
And it's usually mercilessly altered to fit any QEMU interface changes
new version brings, which in turn breaks idea of testing other QEMU versions
with it.

I'd guess it would take *a lot* of effort to make and maintain it
as external test harness for various QEMU versions.

I think build time qtest used as public CI and external test suite
are conflicting goals, for the former we probably minimize used
compute resources while the later is probably run by QA in a more
controlled manner (unless one integrates that into another CI)

If I needed something that were to be usable as external test suite,
I'd look towards acceptance tests which are less depended on QEMU
internals and versions.
 
> > If the worry is about extending test times by having an extra round trip
> > of a spawn and query step for every test could we not consider caching
> > the information somewhere? Really any given binary should only need to
> > be queried once per run, not per test.  
> 
> Good idea.

Yep, it should be less taxing, than the currently posted qtest_has_accel()
version.
It will reduce worst complexity little bit to
   probbing_time * #test_binaries * number_targets * #push_req (all_forks)
but that's still explosive path.

> >> 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
> >>>>>>       
> >>>>>    
> >>>>     
> >>>  
> > 
> >   
> 

+Daniel (sorry Daniel)

On 6/22/21 12:36 PM, Igor Mammedov wrote:
> On Tue, 22 Jun 2021 10:22:19 +0200
> Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>> On 6/22/21 10:07 AM, Alex Bennée wrote:
>>> Igor Mammedov <imammedo@redhat.com> writes:  
>>>> 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().  
>>>
>>> One minor downside is this forever ties the tests to the build. I have
>>> spoken with people before about the idea of separating the test
>>> artefacts from the build so they can be used either as a) cached test
>>> objects or b) other testing environments, for example verifying the
>>> kernel has not regressed. However we don't do either of those things at
>>> the moment so it's not a major concern.  
>>
>> This is the feature that is interesting RedHat QE too, run the latest
>> qtests on various released binaries to compare performances between
>> releases.
> 
> Currently qtest is only a build only and hard tied to concrete release tests.
> And it's usually mercilessly altered to fit any QEMU interface changes
> new version brings, which in turn breaks idea of testing other QEMU versions
> with it.
> 
> I'd guess it would take *a lot* of effort to make and maintain it
> as external test harness for various QEMU versions.

I had the understanding the outcome of the last 6 months discussions
was "do not tie testing with built binary, we have a machine protocol
to introspect the binary" and "tests should focus on the feature to
test, let's remove the build system burden".

> I think build time qtest used as public CI and external test suite
> are conflicting goals, for the former we probably minimize used
> compute resources while the later is probably run by QA in a more
> controlled manner (unless one integrates that into another CI)

I think we are mixing orthogonal topics here:

* CI time is a constraint, no doubt, we don't want to waste it.

* Tests being buildsys-agnostic

If CI can't run all our tests, it is certainly not a reason for
not adding more tests... We should be able to add as many tests
as we want to the repository. Then we should select which tests
we want to run, because we can't run them all.

Otherwise let's declare no more tests can be added in tests/qtest/
because our CI time is already full ;)

> If I needed something that were to be usable as external test suite,
> I'd look towards acceptance tests which are less depended on QEMU
> internals and versions.
>  
>>> If the worry is about extending test times by having an extra round trip
>>> of a spawn and query step for every test could we not consider caching
>>> the information somewhere? Really any given binary should only need to
>>> be queried once per run, not per test.  
>>
>> Good idea.
> 
> Yep, it should be less taxing, than the currently posted qtest_has_accel()
> version.
> It will reduce worst complexity little bit to
>    probbing_time * #test_binaries * number_targets * #push_req (all_forks)
> but that's still explosive path.

On Fri, 18 Jun 2021 13:26:47 +0200
Igor Mammedov <imammedo@redhat.com> wrote:

> On Thu, 17 Jun 2021 18:49:17 +0200
> Claudio Fontana <cfontana@suse.de> wrote:

[...]

> > 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).

it didn't work, built fine but still tries to execute KVM test.
Fixed v3 is on the way

> 
> > 
> > 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
> > >     
> >   
> 
> 

On 6/18/21 5:58 PM, Igor Mammedov wrote:
> On Fri, 18 Jun 2021 13:26:47 +0200
> Igor Mammedov <imammedo@redhat.com> wrote:
> 
>> On Thu, 17 Jun 2021 18:49:17 +0200
>> Claudio Fontana <cfontana@suse.de> wrote:
> 
> [...]
> 
>>> 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).
> 
> it didn't work, built fine but still tries to execute KVM test.
> Fixed v3 is on the way

I am thinking, what about doing the checks we need to do at the beginning of the tests,
and cache the results for all the tests, instead of checking every time?

This way we could use a more general implementation qtest_has_accel("kvm"), and mitigate its cost?

Thanks

C


> 
>>
>>>
>>> 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
>>>>     
>>>   
>>
>>
> 
> 

On 16/06/2021 17.24, 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.
> 
> 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.

You also might want to update the check in tests/qtest/migration-test.c 
while you're at 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.

Could you please get in touch with Philippe directly and discuss which way 
to go? We certainly don't need two approaches in the end...

  Thanks,
   Thomas

On 6/22/21 9:20 AM, Thomas Huth wrote:
> On 16/06/2021 17.24, 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.
>>
>> 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.
> 
> You also might want to update the check in tests/qtest/migration-test.c
> while you're at 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.
> 
> Could you please get in touch with Philippe directly and discuss which
> way to go? We certainly don't need two approaches in the end...

I'm certainly fine if Igor wants to restart this effort :)

Maybe doing as Claudio suggested, start have qtest_has_accel()
tests accel with Igor's shortpath first, then fallback to
'query-accels' QMP command?

On 22/06/2021 09.26, Philippe Mathieu-Daudé wrote:
> On 6/22/21 9:20 AM, Thomas Huth wrote:
>> On 16/06/2021 17.24, 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.
>>>
>>> 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.
>>
>> You also might want to update the check in tests/qtest/migration-test.c
>> while you're at 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.
>>
>> Could you please get in touch with Philippe directly and discuss which
>> way to go? We certainly don't need two approaches in the end...
> 
> I'm certainly fine if Igor wants to restart this effort :)
> 
> Maybe doing as Claudio suggested, start have qtest_has_accel()
> tests accel with Igor's shortpath first, then fallback to
> 'query-accels' QMP command?

Yeah, that's maybe a good idea ...
But I'm currently wondering whether we need query-accels at all? For 
detecting kvm, we already have "query-kvm" ... and we don't have tests for 
any other accelerators yet (hax, hvf, etc.) ... so it's likely just about 
having a way to detect whether TCG is compiled into the binary? Is there 
already another command that could be used to check for the availability of TCG?

  Thomas

On Tue, 22 Jun 2021 09:59:48 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 22/06/2021 09.26, Philippe Mathieu-Daudé wrote:
> > On 6/22/21 9:20 AM, Thomas Huth wrote:  
> >> On 16/06/2021 17.24, 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.
> >>>
> >>> 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.  
> >>
> >> You also might want to update the check in tests/qtest/migration-test.c
> >> while you're at it.
sure (this series was just a discussion starter).


> >>> 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.  
> >>
> >> Could you please get in touch with Philippe directly and discuss which
> >> way to go? We certainly don't need two approaches in the end...  
> > 
> > I'm certainly fine if Igor wants to restart this effort :)
> > 
> > Maybe doing as Claudio suggested, start have qtest_has_accel()
> > tests accel with Igor's shortpath first, then fallback to
> > 'query-accels' QMP command?  

it should save time when shortpath result is positive,
but won't help on fallback path.

Maybe we can add a build option to enable fallback, or even better
to swap build time impl. with runtime impl., so ones who need qtest
as external harness can build and use it on request.

> Yeah, that's maybe a good idea ...
> But I'm currently wondering whether we need query-accels at all? For 
> detecting kvm, we already have "query-kvm" ... and we don't have tests for 
> any other accelerators yet (hax, hvf, etc.) ... so it's likely just about 
> having a way to detect whether TCG is compiled into the binary?

if we ignore other accelerators, it's probably not hard to add detection
of TCG at build time, I'll look into it and also try to change API to
qtest_has_accel() instead of kvm specific one. This way whatever
solution we end up with, won't affect code that uses it.

> Is there 
> already another command that could be used to check for the availability of TCG?
> 
>   Thomas
>