Re: [PATCH v16 04/99] qtest/arm-cpu-features: Use generic qtest_has_accel() to check for KVM

[Thomas Huth]

On 04/06/2021 17.51, Alex Bennée wrote:
> From: Philippe Mathieu-Daudé <philmd@redhat.com>
>
> Use the recently added generic qtest_has_accel() method to
> check if KVM is available.
>
> Suggested-by: Claudio Fontana <cfontana@suse.de>
> Reviewed-by: Andrew Jones <drjones@redhat.com>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Message-Id: <20210505125806.1263441-5-philmd@redhat.com>
> ---
>   tests/qtest/arm-cpu-features.c | 25 +------------------------
>   1 file changed, 1 insertion(+), 24 deletions(-)
>
> diff --git a/tests/qtest/arm-cpu-features.c b/tests/qtest/arm-cpu-features.c
> index 8252b85bb8..7f4b252127 100644
> --- a/tests/qtest/arm-cpu-features.c
> +++ b/tests/qtest/arm-cpu-features.c
> @@ -26,21 +26,6 @@
>                       "  'arguments': { 'type': 'full', "
>   #define QUERY_TAIL  "}}"
>   
> -static bool kvm_enabled(QTestState *qts)
> -{
> -    QDict *resp, *qdict;
> -    bool enabled;
> -
> -    resp = qtest_qmp(qts, "{ 'execute': 'query-kvm' }");
> -    g_assert(qdict_haskey(resp, "return"));
> -    qdict = qdict_get_qdict(resp, "return");
> -    g_assert(qdict_haskey(qdict, "enabled"));
> -    enabled = qdict_get_bool(qdict, "enabled");
> -    qobject_unref(resp);
> -
> -    return enabled;
> -}
> -
>   static QDict *do_query_no_props(QTestState *qts, const char *cpu_type)
>   {
>       return qtest_qmp(qts, QUERY_HEAD "'model': { 'name': %s }"
> @@ -493,14 +478,6 @@ static void test_query_cpu_model_expansion_kvm(const void 
> *data)
>   
>       qts = qtest_init(MACHINE_KVM "-cpu max");
>   
> -    /*
> -     * These tests target the 'host' CPU type, so KVM must be enabled.
> -     */
> -    if (!kvm_enabled(qts)) {
> -        qtest_quit(qts);
> -        return;
> -    }
> -
>       /* Enabling and disabling kvm-no-adjvtime should always work. */
>       assert_has_feature_disabled(qts, "host", "kvm-no-adjvtime");
>       assert_set_feature(qts, "host", "kvm-no-adjvtime", true);
> @@ -624,7 +601,7 @@ int main(int argc, char **argv)
>        * order avoid attempting to run an AArch32 QEMU with KVM on
>        * AArch64 hosts. That won't work and isn't easy to detect.
>        */
> -    if (g_str_equal(qtest_get_arch(), "aarch64")) {
> +    if (g_str_equal(qtest_get_arch(), "aarch64") && qtest_has_accel("kvm")) {
>           qtest_add_data_func("/arm/kvm/query-cpu-model-expansion",
>                               NULL, test_query_cpu_model_expansion_kvm);

I think this is wrong: query-kvm checks whether kvm is *enabled*, while your 
new function only checks whether kvm has been built into the binary. There 
is still the possibility that kvm has been built into the binary, but is not 
available on the host, so in that case the test will fail now.

Thus please drop / rework this patch.

 Thomas