[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v16 04/99] qtest/arm-cpu-features: Use generic qtest_has_acce
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [PATCH v16 04/99] qtest/arm-cpu-features: Use generic qtest_has_accel() to check for KVM |
Date: |
Tue, 8 Jun 2021 14:39:03 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.10.1 |
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é <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.
>
> 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 :(
>>
- [PATCH v16 02/99] accel: Introduce 'query-accels' QMP command, (continued)
[PATCH v16 05/99] qtest/arm-cpu-features: Restrict sve_tests_sve_off_kvm test to KVM, Alex Bennée, 2021/06/04
[PATCH v16 03/99] qtest: Add qtest_has_accel() method, Alex Bennée, 2021/06/04
[PATCH v16 08/99] qtest/migration-test: Skip tests if KVM not builtin on s390x/ppc64, Alex Bennée, 2021/06/04