On 6/8/21 2:39 PM, Philippe Mathieu-Daudé wrote:
> 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?
Let me try some terms:
1 - bool accelerator_built() -> accelerator has been part of this QEMU
build.
2 - bool accelerator_loaded() -> accelerator has been loaded, either
because it is built-in, or because its code has been dynamically loaded
and probably the management layers do not care about the distinction between 1,
2.
Maybe instead of "usable", "available" is better?
3 - bool accelerator_available()
Then we have the accelerator that is actually active, selected or chosen.
4 - bool accelerator_enabled()? bool accelerator_active()? bool
accelerator_chosen()? bool accelerator_used()?
We already use the term "allowed", with different meaning and implementations
depending on --enable-xxx,
which is quite confusing I think..
Ciao,
Claudoi
>
> 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 :(
>>>
>
