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

[Philippe Mathieu-Daudé]

On 6/7/21 3:22 PM, Thomas Huth wrote:
> 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.

Indeed, this is unfortunate :(