On 6/8/21 12:49 PM, Philippe Mathieu-Daudé wrote: > On 6/8/21 10:22 AM, Philippe Mathieu-Daudé wrote: >> On 6/7/21 3:22 PM, Thomas Huth wrote: >>> On 04/06/2021 17.51, Alex Bennée wrote: >>>> From: Philippe Mathieu-Daudé <phi...@redhat.com> >>>> >>>> Use the recently added generic qtest_has_accel() method to >>>> check if KVM is available. >>>> >>>> Suggested-by: Claudio Fontana <cfont...@suse.de> >>>> Reviewed-by: Andrew Jones <drjo...@redhat.com> >>>> Reviewed-by: Alex Bennée <alex.ben...@linaro.org> >>>> Signed-off-by: Philippe Mathieu-Daudé <phi...@redhat.com> >>>> Signed-off-by: Alex Bennée <alex.ben...@linaro.org> >>>> Message-Id: <20210505125806.1263441-5-phi...@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. > > Not enough coffee earlier. I think this is a documentation problem, > query-kvm returns a list of *runtime* accelerators: > https://www.mail-archive.com/qemu-devel@nongnu.org/msg811144.html > > IIUC what Paolo said, if something asks for an accelerator that > is not present at build-time, then this is a configuration problem, > not relevant for the management interface.
Argh no... sigh. So we have 4 cases: 1- accelerator not built 2- accelerator built in as module but not loaded 3- accelerator built in & loaded but not usable 4- accelerator built in & loaded and usable QMP query-accels returns "accelerator built in & loaded" without precising it is usable. qtest kvm_enabled() checks if the accelerator is enabled without checking it is built-in. QMP query-kvm returns 'present' as in built-in (so case 1), and 'enabled' (a.k.a. 'allowed') updated once init_machine() succeeded (so case 4). So, again, IIUC Paolo, what he said is for the management layer 1 and 2 are the same, the accelerator is not present. For qtests, we want the 'usable' case (4) right? Whether the accelerator is builtin / loaded is irrelevant. Could we improve the terminology here? Maybe is_present() and is_usable()? Suggestions? Do we need to add both query-present-accels query-usable-accels commands? Is it actually possible to return an array of 'usable' accelerators? Maybe simply add query-present-accels() -> [] and query-usable-accel(accel) -> bool. > >>> >>> Thus please drop / rework this patch. >> >> Indeed, this is unfortunate :( >>