Introduce the 'query-accels' QMP command which returns a list
of built-in accelerators names.
- Accelerator is an QAPI enum of all existing accelerators,
- AcceleratorInfo is a QAPI structure providing accelerator
specific information. Currently the common structure base
provides the name of the accelerator, while the specific
part is empty, but each accelerator can expand it.
- 'query-accels' QMP command returns a list of @AcceleratorInfo
For example on a KVM-only build we get:
{ "execute": "query-accels" }
{
"return": [
{
"type": "qtest"
},
{
"type": "kvm"
}
]
}
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
qapi/machine.json | 55 +++++++++++++++++++++++++++++++++++++++++++++++
accel/accel-qmp.c | 47 ++++++++++++++++++++++++++++++++++++++++
accel/meson.build | 2 +-
3 files changed, 103 insertions(+), 1 deletion(-)
create mode 100644 accel/accel-qmp.c
diff --git a/qapi/machine.json b/qapi/machine.json
index 330189efe3d..ffbf28e5d50 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -1471,3 +1471,58 @@
##
{ 'event': 'MEM_UNPLUG_ERROR',
'data': { 'device': 'str', 'msg': 'str' } }
+
+##
+# @Accelerator:
+#
+# An enumeration of accelerator names.
+#
+# Since: 6.0
+##
+{ 'enum': 'Accelerator',
+ 'data': [ { 'name': 'qtest' },
+ { 'name': 'tcg' },
+ { 'name': 'kvm' },
+ { 'name': 'hax' },
+ { 'name': 'hvf' },
+ { 'name': 'whpx' },
+ { 'name': 'xen' } ] }
+
+##
+# @AcceleratorInfo:
+#
+# Accelerator information.
+#
+# @name: The accelerator name.
+#
+# Since: 6.0
+##
+{ 'union': 'AcceleratorInfo',
+ 'base': {'name': 'Accelerator'},
+ 'discriminator': 'name',
+ 'data': { } }
+
+##
+# @query-accels:
+#
+# Get a list of AcceleratorInfo for all built-in accelerators.
+#
+# Returns: a list of @AcceleratorInfo describing each accelerator.
+#
+# Since: 6.0
+#
+# Example:
+#
+# -> { "execute": "query-accels" }
+# <- { "return": [
+# {
+# "type": "qtest"
+# },
+# {
+# "type": "kvm"
+# }
+# ] }
+#
+##
+{ 'command': 'query-accels',
+ 'returns': ['AcceleratorInfo'] }
diff --git a/accel/accel-qmp.c b/accel/accel-qmp.c
new file mode 100644
index 00000000000..f16e49b8956
--- /dev/null
+++ b/accel/accel-qmp.c
@@ -0,0 +1,47 @@
+/*
+ * QEMU accelerators, QMP commands
+ *
+ * Copyright (c) 2021 Red Hat Inc.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/qapi-commands-machine.h"
+
+static const Accelerator accel_list[] = {
+ ACCELERATOR_QTEST,
+#ifdef CONFIG_TCG
+ ACCELERATOR_TCG,
+#endif
+#ifdef CONFIG_KVM
+ ACCELERATOR_KVM,
+#endif
+#ifdef CONFIG_HAX
+ ACCELERATOR_HAX,
+#endif
+#ifdef CONFIG_HVF
+ ACCELERATOR_HVF,
+#endif
+#ifdef CONFIG_WHPX
+ ACCELERATOR_WHPX,
+#endif
+#ifdef CONFIG_XEN_BACKEND
+ ACCELERATOR_XEN,
+#endif
+};
+
+AcceleratorInfoList *qmp_query_accels(Error **errp)
+{
+ AcceleratorInfoList *list = NULL, **tail = &list;
+
+ for (unsigned i = 0; i < ARRAY_SIZE(accel_list); i++) {
+ AcceleratorInfo *info = g_new0(AcceleratorInfo, 1);
+
+ info->name = accel_list[i];
+
+ QAPI_LIST_APPEND(tail, info);
+ }
+
+ return list;
+}
diff --git a/accel/meson.build b/accel/meson.build
index b44ba30c864..7a48f6d568d 100644
--- a/accel/meson.build
+++ b/accel/meson.build
@@ -1,4 +1,4 @@
-specific_ss.add(files('accel-common.c'))
+specific_ss.add(files('accel-common.c', 'accel-qmp.c'))
softmmu_ss.add(files('accel-softmmu.c'))
user_ss.add(files('accel-user.c'))
--
2.26.2
On 3/11/21 5:11 PM, Philippe Mathieu-Daudé wrote:
> Introduce the 'query-accels' QMP command which returns a list
> of built-in accelerators names.
accelerator names
>
> - Accelerator is an QAPI enum of all existing accelerators,
is a QAPI enum
>
> - AcceleratorInfo is a QAPI structure providing accelerator
> specific information. Currently the common structure base
> provides the name of the accelerator, while the specific
> part is empty, but each accelerator can expand it.
Do we expand it later in this series? If not,...
>
> - 'query-accels' QMP command returns a list of @AcceleratorInfo
>
> For example on a KVM-only build we get:
>
> { "execute": "query-accels" }
> {
> "return": [
> {
> "type": "qtest"
> },
> {
> "type": "kvm"
> }
Inconsistent with the code, already pointed out in other reviews.
> ]
> }
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> +##
> +# @AcceleratorInfo:
> +#
> +# Accelerator information.
> +#
> +# @name: The accelerator name.
> +#
> +# Since: 6.0
> +##
> +{ 'union': 'AcceleratorInfo',
> + 'base': {'name': 'Accelerator'},
> + 'discriminator': 'name',
> + 'data': { } }
...it feels a bit over-engineered (we can turn it into a union later
while still preserving back-compat). On the other hand, since you are
using conditional compilation...
> +
> +##
> +# @query-accels:
> +#
> +# Get a list of AcceleratorInfo for all built-in accelerators.
> +#
> +# Returns: a list of @AcceleratorInfo describing each accelerator.
> +#
> +# Since: 6.0
> +#
> +# Example:
> +#
> +# -> { "execute": "query-accels" }
> +# <- { "return": [
> +# {
> +# "type": "qtest"
> +# },
> +# {
> +# "type": "kvm"
Another inconsistent example.
> +# }
> +# ] }
> +#
> +##
> +{ 'command': 'query-accels',
> + 'returns': ['AcceleratorInfo'] }
> diff --git a/accel/accel-qmp.c b/accel/accel-qmp.c
> new file mode 100644
> index 00000000000..f16e49b8956
> --- /dev/null
> +++ b/accel/accel-qmp.c
> @@ -0,0 +1,47 @@
> +/*
> + * QEMU accelerators, QMP commands
> + *
> + * Copyright (c) 2021 Red Hat Inc.
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/qapi-commands-machine.h"
> +
> +static const Accelerator accel_list[] = {
> + ACCELERATOR_QTEST,
> +#ifdef CONFIG_TCG
> + ACCELERATOR_TCG,
> +#endif
> +#ifdef CONFIG_KVM
> + ACCELERATOR_KVM,
> +#endif
...would it be worth compiling the enum to only list enum values that
were actually compiled in? That would change it to:
{ 'enum': 'Accelerator',
'data': [ 'qtest',
{ 'name': 'tcg', 'if': 'defined(CONFIG_TCG)' },
...
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Eric Blake <eblake@redhat.com> writes:
> On 3/11/21 5:11 PM, Philippe Mathieu-Daudé wrote:
[...]
>> diff --git a/accel/accel-qmp.c b/accel/accel-qmp.c
>> new file mode 100644
>> index 00000000000..f16e49b8956
>> --- /dev/null
>> +++ b/accel/accel-qmp.c
>> @@ -0,0 +1,47 @@
>> +/*
>> + * QEMU accelerators, QMP commands
>> + *
>> + * Copyright (c) 2021 Red Hat Inc.
>> + *
>> + * SPDX-License-Identifier: GPL-2.0-or-later
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qapi/qapi-commands-machine.h"
>> +
>> +static const Accelerator accel_list[] = {
>> + ACCELERATOR_QTEST,
>> +#ifdef CONFIG_TCG
>> + ACCELERATOR_TCG,
>> +#endif
>> +#ifdef CONFIG_KVM
>> + ACCELERATOR_KVM,
>> +#endif
>
> ...would it be worth compiling the enum to only list enum values that
> were actually compiled in? That would change it to:
>
> { 'enum': 'Accelerator',
> 'data': [ 'qtest',
> { 'name': 'tcg', 'if': 'defined(CONFIG_TCG)' },
> ...
Makes introspection more useful. Management applications can get the
information the list of compiled-in accelerators from query-qmp-schema.
They don't have to be taught to use query-accels.
In fact, query-accels becomes useless except as a tool to force
visibility of Accelerator in query-qmp-schema. We wouldn't have to
force if we had CLI introspection that shows the type of -accel's
parameter @accel. Adding a query command is a common work-around for
our anemic CLI introspection capabilities.
The query command could be made more useful than introspection if it
reflected run time state, i.e. it showed an accelerator only when the
host system actually supports it. Can't say how practical that would
be.
>>
>> +AcceleratorInfoList *qmp_query_accels(Error **errp)
>> +{
>> + AcceleratorInfoList *list = NULL, **tail = &list;
>> +
>> + for (unsigned i = 0; i < ARRAY_SIZE(accel_list); i++) {
>> + AcceleratorInfo *info = g_new0(AcceleratorInfo, 1);
>> +
>> + info->name = accel_list[i];
>> +
>> + QAPI_LIST_APPEND(tail, info);
>> + }
>> +
>> + return list;
>> +}
You could then use something like
for (accel = 0; accel < ACCELERATOR__MAX; accel++) {
AcceleratorInfo *info = g_new0(AcceleratorInfo, 1);
info->name = Accelerator_str(accel);
QAPI_LIST_APPEND(tail, info);
}
On 16/03/21 07:51, Markus Armbruster wrote: > The query command could be made more useful than introspection if it > reflected run time state, i.e. it showed an accelerator only when the > host system actually supports it. Can't say how practical that would > be. At least for KVM there is runtime state that can be included in query-accels. Paolo
On 3/16/21 7:51 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
>
>> On 3/11/21 5:11 PM, Philippe Mathieu-Daudé wrote:
> [...]
>>> diff --git a/accel/accel-qmp.c b/accel/accel-qmp.c
>>> new file mode 100644
>>> index 00000000000..f16e49b8956
>>> --- /dev/null
>>> +++ b/accel/accel-qmp.c
>>> @@ -0,0 +1,47 @@
>>> +/*
>>> + * QEMU accelerators, QMP commands
>>> + *
>>> + * Copyright (c) 2021 Red Hat Inc.
>>> + *
>>> + * SPDX-License-Identifier: GPL-2.0-or-later
>>> + */
>>> +
>>> +#include "qemu/osdep.h"
>>> +#include "qapi/qapi-commands-machine.h"
>>> +
>>> +static const Accelerator accel_list[] = {
>>> + ACCELERATOR_QTEST,
>>> +#ifdef CONFIG_TCG
>>> + ACCELERATOR_TCG,
>>> +#endif
>>> +#ifdef CONFIG_KVM
>>> + ACCELERATOR_KVM,
>>> +#endif
>>
>> ...would it be worth compiling the enum to only list enum values that
>> were actually compiled in? That would change it to:
>>
>> { 'enum': 'Accelerator',
>> 'data': [ 'qtest',
>> { 'name': 'tcg', 'if': 'defined(CONFIG_TCG)' },
>> ...
These accelerator definitions are supposed to be poisoned in generic
code... But I like the simplicity of your suggestion, so I'll give it
a try and see what happens with removing the poisoned definitions.
> Makes introspection more useful. Management applications can get the
> information the list of compiled-in accelerators from query-qmp-schema.
> They don't have to be taught to use query-accels.
>
> In fact, query-accels becomes useless except as a tool to force
> visibility of Accelerator in query-qmp-schema. We wouldn't have to
> force if we had CLI introspection that shows the type of -accel's
> parameter @accel. Adding a query command is a common work-around for
> our anemic CLI introspection capabilities.
>
> The query command could be made more useful than introspection if it
> reflected run time state, i.e. it showed an accelerator only when the
> host system actually supports it. Can't say how practical that would
> be.
>
>>>
>>> +AcceleratorInfoList *qmp_query_accels(Error **errp)
>>> +{
>>> + AcceleratorInfoList *list = NULL, **tail = &list;
>>> +
>>> + for (unsigned i = 0; i < ARRAY_SIZE(accel_list); i++) {
>>> + AcceleratorInfo *info = g_new0(AcceleratorInfo, 1);
>>> +
>>> + info->name = accel_list[i];
>>> +
>>> + QAPI_LIST_APPEND(tail, info);
>>> + }
>>> +
>>> + return list;
>>> +}
>
> You could then use something like
>
> for (accel = 0; accel < ACCELERATOR__MAX; accel++) {
> AcceleratorInfo *info = g_new0(AcceleratorInfo, 1);
>
> info->name = Accelerator_str(accel);
>
> QAPI_LIST_APPEND(tail, info);
> }
>
On 3/16/21 10:02 AM, Philippe Mathieu-Daudé wrote:
> On 3/16/21 7:51 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>>
>>> On 3/11/21 5:11 PM, Philippe Mathieu-Daudé wrote:
>> [...]
>>>> diff --git a/accel/accel-qmp.c b/accel/accel-qmp.c
>>>> new file mode 100644
>>>> index 00000000000..f16e49b8956
>>>> --- /dev/null
>>>> +++ b/accel/accel-qmp.c
>>>> @@ -0,0 +1,47 @@
>>>> +/*
>>>> + * QEMU accelerators, QMP commands
>>>> + *
>>>> + * Copyright (c) 2021 Red Hat Inc.
>>>> + *
>>>> + * SPDX-License-Identifier: GPL-2.0-or-later
>>>> + */
>>>> +
>>>> +#include "qemu/osdep.h"
>>>> +#include "qapi/qapi-commands-machine.h"
>>>> +
>>>> +static const Accelerator accel_list[] = {
>>>> + ACCELERATOR_QTEST,
>>>> +#ifdef CONFIG_TCG
>>>> + ACCELERATOR_TCG,
>>>> +#endif
>>>> +#ifdef CONFIG_KVM
>>>> + ACCELERATOR_KVM,
>>>> +#endif
>>>
>>> ...would it be worth compiling the enum to only list enum values that
>>> were actually compiled in? That would change it to:
>>>
>>> { 'enum': 'Accelerator',
>>> 'data': [ 'qtest',
>>> { 'name': 'tcg', 'if': 'defined(CONFIG_TCG)' },
>>> ...
>
> These accelerator definitions are supposed to be poisoned in generic
> code... But I like the simplicity of your suggestion, so I'll give it
> a try and see what happens with removing the poisoned definitions.
This is actually quite interesting :) Accelerator definitions are
declared in config-target.h, but acceleration is host specific...
We certainly don't want to make qapi target-specific.
Philippe Mathieu-Daudé <philmd@redhat.com> writes:
> On 3/16/21 10:02 AM, Philippe Mathieu-Daudé wrote:
>> On 3/16/21 7:51 AM, Markus Armbruster wrote:
>>> Eric Blake <eblake@redhat.com> writes:
>>>
>>>> On 3/11/21 5:11 PM, Philippe Mathieu-Daudé wrote:
>>> [...]
>>>>> diff --git a/accel/accel-qmp.c b/accel/accel-qmp.c
>>>>> new file mode 100644
>>>>> index 00000000000..f16e49b8956
>>>>> --- /dev/null
>>>>> +++ b/accel/accel-qmp.c
>>>>> @@ -0,0 +1,47 @@
>>>>> +/*
>>>>> + * QEMU accelerators, QMP commands
>>>>> + *
>>>>> + * Copyright (c) 2021 Red Hat Inc.
>>>>> + *
>>>>> + * SPDX-License-Identifier: GPL-2.0-or-later
>>>>> + */
>>>>> +
>>>>> +#include "qemu/osdep.h"
>>>>> +#include "qapi/qapi-commands-machine.h"
>>>>> +
>>>>> +static const Accelerator accel_list[] = {
>>>>> + ACCELERATOR_QTEST,
>>>>> +#ifdef CONFIG_TCG
>>>>> + ACCELERATOR_TCG,
>>>>> +#endif
>>>>> +#ifdef CONFIG_KVM
>>>>> + ACCELERATOR_KVM,
>>>>> +#endif
>>>>
>>>> ...would it be worth compiling the enum to only list enum values that
>>>> were actually compiled in? That would change it to:
>>>>
>>>> { 'enum': 'Accelerator',
>>>> 'data': [ 'qtest',
>>>> { 'name': 'tcg', 'if': 'defined(CONFIG_TCG)' },
>>>> ...
>>
>> These accelerator definitions are supposed to be poisoned in generic
>> code... But I like the simplicity of your suggestion, so I'll give it
>> a try and see what happens with removing the poisoned definitions.
>
> This is actually quite interesting :) Accelerator definitions are
> declared in config-target.h, but acceleration is host specific...
>
> We certainly don't want to make qapi target-specific.
We certainly don't want to make all the generated QAPI headers
target-specific.
We have made *selected* QAPI sub-modules target-specific, namely the
ones ending with "-target", currently qapi/machine-target.json and
qapi/misc-target.json. Only these may use poisoned symbols in 'if'
conditions. Example:
{ 'command': 'query-sev', 'returns': 'SevInfo',
'if': 'defined(TARGET_I386)' }
On 3/16/21 11:26 AM, Philippe Mathieu-Daudé wrote:
> On 3/16/21 10:02 AM, Philippe Mathieu-Daudé wrote:
>> On 3/16/21 7:51 AM, Markus Armbruster wrote:
>>> Eric Blake <eblake@redhat.com> writes:
>>>
>>>> On 3/11/21 5:11 PM, Philippe Mathieu-Daudé wrote:
>>> [...]
>>>>> diff --git a/accel/accel-qmp.c b/accel/accel-qmp.c
>>>>> new file mode 100644
>>>>> index 00000000000..f16e49b8956
>>>>> --- /dev/null
>>>>> +++ b/accel/accel-qmp.c
>>>>> @@ -0,0 +1,47 @@
>>>>> +/*
>>>>> + * QEMU accelerators, QMP commands
>>>>> + *
>>>>> + * Copyright (c) 2021 Red Hat Inc.
>>>>> + *
>>>>> + * SPDX-License-Identifier: GPL-2.0-or-later
>>>>> + */
>>>>> +
>>>>> +#include "qemu/osdep.h"
>>>>> +#include "qapi/qapi-commands-machine.h"
>>>>> +
>>>>> +static const Accelerator accel_list[] = {
>>>>> + ACCELERATOR_QTEST,
>>>>> +#ifdef CONFIG_TCG
>>>>> + ACCELERATOR_TCG,
>>>>> +#endif
>>>>> +#ifdef CONFIG_KVM
>>>>> + ACCELERATOR_KVM,
>>>>> +#endif
>>>>
>>>> ...would it be worth compiling the enum to only list enum values that
>>>> were actually compiled in? That would change it to:
>>>>
>>>> { 'enum': 'Accelerator',
>>>> 'data': [ 'qtest',
>>>> { 'name': 'tcg', 'if': 'defined(CONFIG_TCG)' },
>>>> ...
>>
>> These accelerator definitions are supposed to be poisoned in generic
>> code... But I like the simplicity of your suggestion, so I'll give it
>> a try and see what happens with removing the poisoned definitions.
>
> This is actually quite interesting :) Accelerator definitions are
> declared in config-target.h, but acceleration is host specific...
Thomas, I guess I hit Claudio's reported bug again...
1/ generic libqemuutil.a is built without any CONFIG_accel definition.
So this qapi-generated enum ... :
typedef enum Accelerator {
ACCELERATOR_QTEST,
#if defined(CONFIG_TCG)
ACCELERATOR_TCG,
#endif /* defined(CONFIG_TCG) */
#if defined(CONFIG_KVM)
ACCELERATOR_KVM,
#endif /* defined(CONFIG_KVM) */
#if defined(CONFIG_HAX)
ACCELERATOR_HAX,
#endif /* defined(CONFIG_HAX) */
#if defined(CONFIG_HVF)
ACCELERATOR_HVF,
#endif /* defined(CONFIG_HVF) */
#if defined(CONFIG_WHPX)
ACCELERATOR_WHPX,
#endif /* defined(CONFIG_WHPX) */
#if defined(CONFIG_XEN_BACKEND)
ACCELERATOR_XEN,
#endif /* defined(CONFIG_XEN_BACKEND) */
ACCELERATOR__MAX,
} Accelerator;
... is expanded to:
typedef enum Accelerator {
ACCELERATOR_QTEST,
ACCELERATOR__MAX,
} Accelerator;
2/ softmmu code and qtest do get the definition, the enum
is different:
typedef enum Accelerator {
ACCELERATOR_QTEST,
ACCELERATOR_KVM,
ACCELERATOR__MAX,
} Accelerator;
qmp_query_accels() fills AcceleratorInfoList with 2 entries
3/ trying to understand what's happening, query-qmp-schema
returns:
{
"name": "206",
"tag": "name",
"variants": [
{
"case": "qtest",
"type": "0"
},
{
"case": "kvm",
"type": "0"
}
],
"members": [
{
"name": "name",
"type": "403"
}
],
"meta-type": "object"
},
{
"name": "403",
"meta-type": "enum",
"values": [
"qtest",
"kvm"
]
},
So accelerators are listed, but with the same enum index?
4/ Running 'query-accels' aborts in qapi_enum_lookup():
The first entry 'qtest' is visited correctly.
When the second entry 'kvm' is visited, this assertion fires:
assert(val >= 0 && val < lookup->size);
because lookup->size = 1 (remember from 1/ Accelerator_lookup
has been compiled without definitions, in libqemuutil.a).
I'll keep the current patch, as it works, and address the rest
of this series comments.
Thanks,
Phil.
Philippe Mathieu-Daudé <philmd@redhat.com> writes:
> On 3/16/21 11:26 AM, Philippe Mathieu-Daudé wrote:
>> On 3/16/21 10:02 AM, Philippe Mathieu-Daudé wrote:
>>> On 3/16/21 7:51 AM, Markus Armbruster wrote:
>>>> Eric Blake <eblake@redhat.com> writes:
>>>>
>>>>> On 3/11/21 5:11 PM, Philippe Mathieu-Daudé wrote:
>>>> [...]
>>>>>> diff --git a/accel/accel-qmp.c b/accel/accel-qmp.c
>>>>>> new file mode 100644
>>>>>> index 00000000000..f16e49b8956
>>>>>> --- /dev/null
>>>>>> +++ b/accel/accel-qmp.c
>>>>>> @@ -0,0 +1,47 @@
>>>>>> +/*
>>>>>> + * QEMU accelerators, QMP commands
>>>>>> + *
>>>>>> + * Copyright (c) 2021 Red Hat Inc.
>>>>>> + *
>>>>>> + * SPDX-License-Identifier: GPL-2.0-or-later
>>>>>> + */
>>>>>> +
>>>>>> +#include "qemu/osdep.h"
>>>>>> +#include "qapi/qapi-commands-machine.h"
>>>>>> +
>>>>>> +static const Accelerator accel_list[] = {
>>>>>> + ACCELERATOR_QTEST,
>>>>>> +#ifdef CONFIG_TCG
>>>>>> + ACCELERATOR_TCG,
>>>>>> +#endif
>>>>>> +#ifdef CONFIG_KVM
>>>>>> + ACCELERATOR_KVM,
>>>>>> +#endif
>>>>>
>>>>> ...would it be worth compiling the enum to only list enum values that
>>>>> were actually compiled in? That would change it to:
>>>>>
>>>>> { 'enum': 'Accelerator',
>>>>> 'data': [ 'qtest',
>>>>> { 'name': 'tcg', 'if': 'defined(CONFIG_TCG)' },
>>>>> ...
>>>
>>> These accelerator definitions are supposed to be poisoned in generic
>>> code... But I like the simplicity of your suggestion, so I'll give it
>>> a try and see what happens with removing the poisoned definitions.
>>
>> This is actually quite interesting :) Accelerator definitions are
>> declared in config-target.h, but acceleration is host specific...
>
> Thomas, I guess I hit Claudio's reported bug again...
>
> 1/ generic libqemuutil.a is built without any CONFIG_accel definition.
>
> So this qapi-generated enum ... :
>
> typedef enum Accelerator {
> ACCELERATOR_QTEST,
> #if defined(CONFIG_TCG)
> ACCELERATOR_TCG,
> #endif /* defined(CONFIG_TCG) */
> #if defined(CONFIG_KVM)
> ACCELERATOR_KVM,
> #endif /* defined(CONFIG_KVM) */
> #if defined(CONFIG_HAX)
> ACCELERATOR_HAX,
> #endif /* defined(CONFIG_HAX) */
> #if defined(CONFIG_HVF)
> ACCELERATOR_HVF,
> #endif /* defined(CONFIG_HVF) */
> #if defined(CONFIG_WHPX)
> ACCELERATOR_WHPX,
> #endif /* defined(CONFIG_WHPX) */
> #if defined(CONFIG_XEN_BACKEND)
> ACCELERATOR_XEN,
> #endif /* defined(CONFIG_XEN_BACKEND) */
> ACCELERATOR__MAX,
> } Accelerator;
>
> ... is expanded to:
>
> typedef enum Accelerator {
> ACCELERATOR_QTEST,
> ACCELERATOR__MAX,
> } Accelerator;
CONFIG_KVM, CONFIG_TCG, ... are defined in ${target}-config-target.h,
and may only be used in target-specific code.
If the enum ends up in libqemuutil.a, there are uses outside
target-specific code.
exec/poison.h lacks CONFIG_KVM, CONFIG_TCG, ... Should they be added?
[...]
On 16/03/2021 13.41, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>
>> On 3/16/21 11:26 AM, Philippe Mathieu-Daudé wrote:
>>> On 3/16/21 10:02 AM, Philippe Mathieu-Daudé wrote:
>>>> On 3/16/21 7:51 AM, Markus Armbruster wrote:
>>>>> Eric Blake <eblake@redhat.com> writes:
>>>>>
>>>>>> On 3/11/21 5:11 PM, Philippe Mathieu-Daudé wrote:
>>>>> [...]
>>>>>>> diff --git a/accel/accel-qmp.c b/accel/accel-qmp.c
>>>>>>> new file mode 100644
>>>>>>> index 00000000000..f16e49b8956
>>>>>>> --- /dev/null
>>>>>>> +++ b/accel/accel-qmp.c
>>>>>>> @@ -0,0 +1,47 @@
>>>>>>> +/*
>>>>>>> + * QEMU accelerators, QMP commands
>>>>>>> + *
>>>>>>> + * Copyright (c) 2021 Red Hat Inc.
>>>>>>> + *
>>>>>>> + * SPDX-License-Identifier: GPL-2.0-or-later
>>>>>>> + */
>>>>>>> +
>>>>>>> +#include "qemu/osdep.h"
>>>>>>> +#include "qapi/qapi-commands-machine.h"
>>>>>>> +
>>>>>>> +static const Accelerator accel_list[] = {
>>>>>>> + ACCELERATOR_QTEST,
>>>>>>> +#ifdef CONFIG_TCG
>>>>>>> + ACCELERATOR_TCG,
>>>>>>> +#endif
>>>>>>> +#ifdef CONFIG_KVM
>>>>>>> + ACCELERATOR_KVM,
>>>>>>> +#endif
>>>>>>
>>>>>> ...would it be worth compiling the enum to only list enum values that
>>>>>> were actually compiled in? That would change it to:
>>>>>>
>>>>>> { 'enum': 'Accelerator',
>>>>>> 'data': [ 'qtest',
>>>>>> { 'name': 'tcg', 'if': 'defined(CONFIG_TCG)' },
>>>>>> ...
>>>>
>>>> These accelerator definitions are supposed to be poisoned in generic
>>>> code... But I like the simplicity of your suggestion, so I'll give it
>>>> a try and see what happens with removing the poisoned definitions.
>>>
>>> This is actually quite interesting :) Accelerator definitions are
>>> declared in config-target.h, but acceleration is host specific...
>>
>> Thomas, I guess I hit Claudio's reported bug again...
>>
>> 1/ generic libqemuutil.a is built without any CONFIG_accel definition.
>>
>> So this qapi-generated enum ... :
>>
>> typedef enum Accelerator {
>> ACCELERATOR_QTEST,
>> #if defined(CONFIG_TCG)
>> ACCELERATOR_TCG,
>> #endif /* defined(CONFIG_TCG) */
>> #if defined(CONFIG_KVM)
>> ACCELERATOR_KVM,
>> #endif /* defined(CONFIG_KVM) */
>> #if defined(CONFIG_HAX)
>> ACCELERATOR_HAX,
>> #endif /* defined(CONFIG_HAX) */
>> #if defined(CONFIG_HVF)
>> ACCELERATOR_HVF,
>> #endif /* defined(CONFIG_HVF) */
>> #if defined(CONFIG_WHPX)
>> ACCELERATOR_WHPX,
>> #endif /* defined(CONFIG_WHPX) */
>> #if defined(CONFIG_XEN_BACKEND)
>> ACCELERATOR_XEN,
>> #endif /* defined(CONFIG_XEN_BACKEND) */
>> ACCELERATOR__MAX,
>> } Accelerator;
>>
>> ... is expanded to:
>>
>> typedef enum Accelerator {
>> ACCELERATOR_QTEST,
>> ACCELERATOR__MAX,
>> } Accelerator;
>
> CONFIG_KVM, CONFIG_TCG, ... are defined in ${target}-config-target.h,
> and may only be used in target-specific code.
>
> If the enum ends up in libqemuutil.a, there are uses outside
> target-specific code.
>
> exec/poison.h lacks CONFIG_KVM, CONFIG_TCG, ... Should they be added?
CONFIG_KVM is already in poison.h, and CONFIG_TCG cannot be added there
since it is also defined in config-host.h. But the other accelerator
switches should be marked as poisoned, too. I've got a patch for this in the
works already - I'll send it out in a minute...
Thomas
On 3/16/21 1:48 PM, Thomas Huth wrote:
> On 16/03/2021 13.41, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>>
>>> On 3/16/21 11:26 AM, Philippe Mathieu-Daudé wrote:
>>>> On 3/16/21 10:02 AM, Philippe Mathieu-Daudé wrote:
>>>>> On 3/16/21 7:51 AM, Markus Armbruster wrote:
>>>>>> Eric Blake <eblake@redhat.com> writes:
>>>>>>
>>>>>>> On 3/11/21 5:11 PM, Philippe Mathieu-Daudé wrote:
>>>>>> [...]
>>>>>>>> diff --git a/accel/accel-qmp.c b/accel/accel-qmp.c
>>>>>>>> new file mode 100644
>>>>>>>> index 00000000000..f16e49b8956
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/accel/accel-qmp.c
>>>>>>>> @@ -0,0 +1,47 @@
>>>>>>>> +/*
>>>>>>>> + * QEMU accelerators, QMP commands
>>>>>>>> + *
>>>>>>>> + * Copyright (c) 2021 Red Hat Inc.
>>>>>>>> + *
>>>>>>>> + * SPDX-License-Identifier: GPL-2.0-or-later
>>>>>>>> + */
>>>>>>>> +
>>>>>>>> +#include "qemu/osdep.h"
>>>>>>>> +#include "qapi/qapi-commands-machine.h"
>>>>>>>> +
>>>>>>>> +static const Accelerator accel_list[] = {
>>>>>>>> + ACCELERATOR_QTEST,
>>>>>>>> +#ifdef CONFIG_TCG
>>>>>>>> + ACCELERATOR_TCG,
>>>>>>>> +#endif
>>>>>>>> +#ifdef CONFIG_KVM
>>>>>>>> + ACCELERATOR_KVM,
>>>>>>>> +#endif
>>>>>>>
>>>>>>> ...would it be worth compiling the enum to only list enum values
>>>>>>> that
>>>>>>> were actually compiled in? That would change it to:
>>>>>>>
>>>>>>> { 'enum': 'Accelerator',
>>>>>>> 'data': [ 'qtest',
>>>>>>> { 'name': 'tcg', 'if': 'defined(CONFIG_TCG)' },
>>>>>>> ...
>>>>>
>>>>> These accelerator definitions are supposed to be poisoned in generic
>>>>> code... But I like the simplicity of your suggestion, so I'll give it
>>>>> a try and see what happens with removing the poisoned definitions.
>>>>
>>>> This is actually quite interesting :) Accelerator definitions are
>>>> declared in config-target.h, but acceleration is host specific...
>>>
>>> Thomas, I guess I hit Claudio's reported bug again...
>>>
>>> 1/ generic libqemuutil.a is built without any CONFIG_accel definition.
>>>
>>> So this qapi-generated enum ... :
>>>
>>> typedef enum Accelerator {
>>> ACCELERATOR_QTEST,
>>> #if defined(CONFIG_TCG)
>>> ACCELERATOR_TCG,
>>> #endif /* defined(CONFIG_TCG) */
>>> #if defined(CONFIG_KVM)
>>> ACCELERATOR_KVM,
>>> #endif /* defined(CONFIG_KVM) */
>>> #if defined(CONFIG_HAX)
>>> ACCELERATOR_HAX,
>>> #endif /* defined(CONFIG_HAX) */
>>> #if defined(CONFIG_HVF)
>>> ACCELERATOR_HVF,
>>> #endif /* defined(CONFIG_HVF) */
>>> #if defined(CONFIG_WHPX)
>>> ACCELERATOR_WHPX,
>>> #endif /* defined(CONFIG_WHPX) */
>>> #if defined(CONFIG_XEN_BACKEND)
>>> ACCELERATOR_XEN,
>>> #endif /* defined(CONFIG_XEN_BACKEND) */
>>> ACCELERATOR__MAX,
>>> } Accelerator;
>>>
>>> ... is expanded to:
>>>
>>> typedef enum Accelerator {
>>> ACCELERATOR_QTEST,
>>> ACCELERATOR__MAX,
>>> } Accelerator;
>>
>> CONFIG_KVM, CONFIG_TCG, ... are defined in ${target}-config-target.h,
>> and may only be used in target-specific code.
>>
>> If the enum ends up in libqemuutil.a, there are uses outside
>> target-specific code.
>>
>> exec/poison.h lacks CONFIG_KVM, CONFIG_TCG, ... Should they be added?
>
> CONFIG_KVM is already in poison.h, and CONFIG_TCG cannot be added there
> since it is also defined in config-host.h. But the other accelerator
> switches should be marked as poisoned, too.
Maybe we hit "exec/poison.h" limit. Maybe it is too wide / generic,
and need split, or multiple ones.
Should we redefine on which code area we want these definitions
poisoned?
AFAIK accel/ is not target specific but host specific.
My list of area / useful to poison:
- user-mode
. non-tcg accel
. hardware emulation
- generic hardware emulation
. target specific
. accel specific
For this one it is already too late:
- target acceleration
. hardware emulation
Thoughts?
On Fri, Mar 12, 2021 at 3:14 AM Philippe Mathieu-Daudé <philmd@redhat.com>
wrote:
> Introduce the 'query-accels' QMP command which returns a list
> of built-in accelerators names.
>
> - Accelerator is an QAPI enum of all existing accelerators,
>
> - AcceleratorInfo is a QAPI structure providing accelerator
> specific information. Currently the common structure base
> provides the name of the accelerator, while the specific
> part is empty, but each accelerator can expand it.
>
> - 'query-accels' QMP command returns a list of @AcceleratorInfo
>
> For example on a KVM-only build we get:
>
> { "execute": "query-accels" }
> {
> "return": [
> {
> "type": "qtest"
> },
> {
> "type": "kvm"
> }
>
s/type/name (in this version)
]
> }
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> qapi/machine.json | 55 +++++++++++++++++++++++++++++++++++++++++++++++
> accel/accel-qmp.c | 47 ++++++++++++++++++++++++++++++++++++++++
> accel/meson.build | 2 +-
> 3 files changed, 103 insertions(+), 1 deletion(-)
> create mode 100644 accel/accel-qmp.c
>
> diff --git a/qapi/machine.json b/qapi/machine.json
> index 330189efe3d..ffbf28e5d50 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -1471,3 +1471,58 @@
> ##
> { 'event': 'MEM_UNPLUG_ERROR',
> 'data': { 'device': 'str', 'msg': 'str' } }
> +
> +##
> +# @Accelerator:
> +#
> +# An enumeration of accelerator names.
> +#
> +# Since: 6.0
> +##
> +{ 'enum': 'Accelerator',
> + 'data': [ { 'name': 'qtest' },
> + { 'name': 'tcg' },
> + { 'name': 'kvm' },
> + { 'name': 'hax' },
> + { 'name': 'hvf' },
> + { 'name': 'whpx' },
> + { 'name': 'xen' } ] }
> +
>
Why not use a simple enum?
+##
> +# @AcceleratorInfo:
> +#
> +# Accelerator information.
> +#
> +# @name: The accelerator name.
> +#
> +# Since: 6.0
> +##
> +{ 'union': 'AcceleratorInfo',
> + 'base': {'name': 'Accelerator'},
> + 'discriminator': 'name',
> + 'data': { } }
>
+
>
Making room for future details, why not.
+##
> +# @query-accels:
> +#
> +# Get a list of AcceleratorInfo for all built-in accelerators.
> +#
> +# Returns: a list of @AcceleratorInfo describing each accelerator.
> +#
> +# Since: 6.0
> +#
> +# Example:
> +#
> +# -> { "execute": "query-accels" }
> +# <- { "return": [
> +# {
> +# "type": "qtest"
> +# },
> +# {
> +# "type": "kvm"
> +# }
> +# ] }
> +#
> +##
> +{ 'command': 'query-accels',
> + 'returns': ['AcceleratorInfo'] }
>
That's nice, but how do you know which accels are actually enabled?
diff --git a/accel/accel-qmp.c b/accel/accel-qmp.c
> new file mode 100644
> index 00000000000..f16e49b8956
> --- /dev/null
> +++ b/accel/accel-qmp.c
> @@ -0,0 +1,47 @@
> +/*
> + * QEMU accelerators, QMP commands
> + *
> + * Copyright (c) 2021 Red Hat Inc.
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/qapi-commands-machine.h"
> +
> +static const Accelerator accel_list[] = {
> + ACCELERATOR_QTEST,
> +#ifdef CONFIG_TCG
> + ACCELERATOR_TCG,
> +#endif
> +#ifdef CONFIG_KVM
> + ACCELERATOR_KVM,
> +#endif
> +#ifdef CONFIG_HAX
> + ACCELERATOR_HAX,
> +#endif
> +#ifdef CONFIG_HVF
> + ACCELERATOR_HVF,
> +#endif
> +#ifdef CONFIG_WHPX
> + ACCELERATOR_WHPX,
> +#endif
> +#ifdef CONFIG_XEN_BACKEND
> + ACCELERATOR_XEN,
> +#endif
> +};
> +
> +AcceleratorInfoList *qmp_query_accels(Error **errp)
> +{
> + AcceleratorInfoList *list = NULL, **tail = &list;
> +
> + for (unsigned i = 0; i < ARRAY_SIZE(accel_list); i++) {
> + AcceleratorInfo *info = g_new0(AcceleratorInfo, 1);
> +
> + info->name = accel_list[i];
> +
> + QAPI_LIST_APPEND(tail, info);
> + }
> +
> + return list;
> +}
> diff --git a/accel/meson.build b/accel/meson.build
> index b44ba30c864..7a48f6d568d 100644
> --- a/accel/meson.build
> +++ b/accel/meson.build
> @@ -1,4 +1,4 @@
> -specific_ss.add(files('accel-common.c'))
> +specific_ss.add(files('accel-common.c', 'accel-qmp.c'))
> softmmu_ss.add(files('accel-softmmu.c'))
> user_ss.add(files('accel-user.c'))
>
> --
> 2.26.2
>
>
>
--
Marc-André Lureau
On 3/12/21 8:42 AM, Marc-André Lureau wrote:
> On Fri, Mar 12, 2021 at 3:14 AM Philippe Mathieu-Daudé <philmd@redhat.com>
> wrote:
>
>> Introduce the 'query-accels' QMP command which returns a list
>> of built-in accelerators names.
>>
>> - Accelerator is an QAPI enum of all existing accelerators,
>>
>> - AcceleratorInfo is a QAPI structure providing accelerator
>> specific information. Currently the common structure base
>> provides the name of the accelerator, while the specific
>> part is empty, but each accelerator can expand it.
>>
>> - 'query-accels' QMP command returns a list of @AcceleratorInfo
>>
>> For example on a KVM-only build we get:
>>
>> { "execute": "query-accels" }
>> {
>> "return": [
>> {
>> "type": "qtest"
>> },
>> {
>> "type": "kvm"
>> }
>>
>
> s/type/name (in this version)
>
> ]
>> }
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>> qapi/machine.json | 55 +++++++++++++++++++++++++++++++++++++++++++++++
>> accel/accel-qmp.c | 47 ++++++++++++++++++++++++++++++++++++++++
>> accel/meson.build | 2 +-
>> 3 files changed, 103 insertions(+), 1 deletion(-)
>> create mode 100644 accel/accel-qmp.c
>>
>> diff --git a/qapi/machine.json b/qapi/machine.json
>> index 330189efe3d..ffbf28e5d50 100644
>> --- a/qapi/machine.json
>> +++ b/qapi/machine.json
>> @@ -1471,3 +1471,58 @@
>> ##
>> { 'event': 'MEM_UNPLUG_ERROR',
>> 'data': { 'device': 'str', 'msg': 'str' } }
>> +
>> +##
>> +# @Accelerator:
>> +#
>> +# An enumeration of accelerator names.
>> +#
>> +# Since: 6.0
>> +##
>> +{ 'enum': 'Accelerator',
>> + 'data': [ { 'name': 'qtest' },
>> + { 'name': 'tcg' },
>> + { 'name': 'kvm' },
>> + { 'name': 'hax' },
>> + { 'name': 'hvf' },
>> + { 'name': 'whpx' },
>> + { 'name': 'xen' } ] }
>> +
>>
>
> Why not use a simple enum?
>
> +##
>> +# @AcceleratorInfo:
>> +#
>> +# Accelerator information.
>> +#
>> +# @name: The accelerator name.
>> +#
>> +# Since: 6.0
>> +##
>> +{ 'union': 'AcceleratorInfo',
>> + 'base': {'name': 'Accelerator'},
>> + 'discriminator': 'name',
>> + 'data': { } }
>>
> +
>>
>
> Making room for future details, why not.
>
> +##
>> +# @query-accels:
>> +#
>> +# Get a list of AcceleratorInfo for all built-in accelerators.
>> +#
>> +# Returns: a list of @AcceleratorInfo describing each accelerator.
>> +#
>> +# Since: 6.0
>> +#
>> +# Example:
>> +#
>> +# -> { "execute": "query-accels" }
>> +# <- { "return": [
>> +# {
>> +# "type": "qtest"
>> +# },
>> +# {
>> +# "type": "kvm"
>> +# }
>> +# ] }
>> +#
>> +##
>> +{ 'command': 'query-accels',
>> + 'returns': ['AcceleratorInfo'] }
>>
>
> That's nice, but how do you know which accels are actually enabled?
Maybe for clarity this could be 'query-accels-available' (which is probably the goal of this series).
Possibly a separate one would be 'query-accel-enabled'?
Can we see these commands being used for libvirt too, to improve feature detection? Are these useful beyond the confines of just testing?
I would think so right?
Ciao,
Claudio
>
> diff --git a/accel/accel-qmp.c b/accel/accel-qmp.c
>> new file mode 100644
>> index 00000000000..f16e49b8956
>> --- /dev/null
>> +++ b/accel/accel-qmp.c
>> @@ -0,0 +1,47 @@
>> +/*
>> + * QEMU accelerators, QMP commands
>> + *
>> + * Copyright (c) 2021 Red Hat Inc.
>> + *
>> + * SPDX-License-Identifier: GPL-2.0-or-later
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qapi/qapi-commands-machine.h"
>> +
>> +static const Accelerator accel_list[] = {
>> + ACCELERATOR_QTEST,
>> +#ifdef CONFIG_TCG
>> + ACCELERATOR_TCG,
>> +#endif
>> +#ifdef CONFIG_KVM
>> + ACCELERATOR_KVM,
>> +#endif
>> +#ifdef CONFIG_HAX
>> + ACCELERATOR_HAX,
>> +#endif
>> +#ifdef CONFIG_HVF
>> + ACCELERATOR_HVF,
>> +#endif
>> +#ifdef CONFIG_WHPX
>> + ACCELERATOR_WHPX,
>> +#endif
>> +#ifdef CONFIG_XEN_BACKEND
>> + ACCELERATOR_XEN,
>> +#endif
>> +};
>> +
>> +AcceleratorInfoList *qmp_query_accels(Error **errp)
>> +{
>> + AcceleratorInfoList *list = NULL, **tail = &list;
>> +
>> + for (unsigned i = 0; i < ARRAY_SIZE(accel_list); i++) {
>> + AcceleratorInfo *info = g_new0(AcceleratorInfo, 1);
>> +
>> + info->name = accel_list[i];
>> +
>> + QAPI_LIST_APPEND(tail, info);
>> + }
>> +
>> + return list;
>> +}
>> diff --git a/accel/meson.build b/accel/meson.build
>> index b44ba30c864..7a48f6d568d 100644
>> --- a/accel/meson.build
>> +++ b/accel/meson.build
>> @@ -1,4 +1,4 @@
>> -specific_ss.add(files('accel-common.c'))
>> +specific_ss.add(files('accel-common.c', 'accel-qmp.c'))
>> softmmu_ss.add(files('accel-softmmu.c'))
>> user_ss.add(files('accel-user.c'))
>>
>> --
>> 2.26.2
>>
>>
>>
>
On 12/03/21 09:46, Claudio Fontana wrote: > > Maybe for clarity this could be 'query-accels-available' (which is probably the goal of this series). > Possibly a separate one would be 'query-accel-enabled'? The accelerator object is not included in the QOM tree. I think it should be added to /machine/accel so that you can get the enabled accelerator with "qom-get /machine/accel type". Paolo
On 12/03/2021 08.42, Marc-André Lureau wrote:
>
>
> On Fri, Mar 12, 2021 at 3:14 AM Philippe Mathieu-Daudé <philmd@redhat.com
> <mailto:philmd@redhat.com>> wrote:
>
[...]
> +##
> +# @AcceleratorInfo:
> +#
> +# Accelerator information.
> +#
> +# @name: The accelerator name.
> +#
> +# Since: 6.0
> +##
> +{ 'union': 'AcceleratorInfo',
> + 'base': {'name': 'Accelerator'},
> + 'discriminator': 'name',
> + 'data': { } }
>
> +
>
>
> Making room for future details, why not.
I think we definitely need the additional data section here: For KVM on
POWER, it would be good to know whether it's KVM-HV or KVM-PR, for KVM on
MIPS it would be good to know whether it's KVM_VM_MIPS_VZ or KVM_VM_MIPS_TE,
for KVM on x86 whether it's the AMD flavor or Intel, ...
> +##
> +# @query-accels:
> +#
> +# Get a list of AcceleratorInfo for all built-in accelerators.
> +#
> +# Returns: a list of @AcceleratorInfo describing each accelerator.
> +#
> +# Since: 6.0
> +#
> +# Example:
> +#
> +# -> { "execute": "query-accels" }
> +# <- { "return": [
> +# {
> +# "type": "qtest"
> +# },
> +# {
> +# "type": "kvm"
> +# }
> +# ] }
> +#
> +##
> +{ 'command': 'query-accels',
> + 'returns': ['AcceleratorInfo'] }
>
>
> That's nice, but how do you know which accels are actually enabled?
I guess we need two commands in the end, one for querying which accelerators
are available, and one for querying the data from the currently active
accelerator...?
Thomas
On Fri, Mar 12, 2021 at 09:11:45AM +0100, Thomas Huth wrote:
> On 12/03/2021 08.42, Marc-André Lureau wrote:
> >
> >
> > On Fri, Mar 12, 2021 at 3:14 AM Philippe Mathieu-Daudé
> > <philmd@redhat.com <mailto:philmd@redhat.com>> wrote:
> >
> [...]
> > +##
> > +# @AcceleratorInfo:
> > +#
> > +# Accelerator information.
> > +#
> > +# @name: The accelerator name.
> > +#
> > +# Since: 6.0
> > +##
> > +{ 'union': 'AcceleratorInfo',
> > + 'base': {'name': 'Accelerator'},
> > + 'discriminator': 'name',
> > + 'data': { } }
> >
> > +
> >
> >
> > Making room for future details, why not.
>
> I think we definitely need the additional data section here: For KVM on
> POWER, it would be good to know whether it's KVM-HV or KVM-PR, for KVM on
> MIPS it would be good to know whether it's KVM_VM_MIPS_VZ or KVM_VM_MIPS_TE,
> for KVM on x86 whether it's the AMD flavor or Intel, ...
Could also pre-expand all of these and list them individually.
>
> > +##
> > +# @query-accels:
> > +#
> > +# Get a list of AcceleratorInfo for all built-in accelerators.
> > +#
> > +# Returns: a list of @AcceleratorInfo describing each accelerator.
> > +#
> > +# Since: 6.0
> > +#
> > +# Example:
> > +#
> > +# -> { "execute": "query-accels" }
> > +# <- { "return": [
> > +# {
> > +# "type": "qtest"
> > +# },
> > +# {
> > +# "type": "kvm"
> > +# }
> > +# ] }
> > +#
> > +##
> > +{ 'command': 'query-accels',
> > + 'returns': ['AcceleratorInfo'] }
> >
> >
> > That's nice, but how do you know which accels are actually enabled?
>
> I guess we need two commands in the end, one for querying which accelerators
> are available, and one for querying the data from the currently active
> accelerator...?
>
If we listed each accelerator individually, then we could use booleans
for them, where only the active one would be true. If we can't come up
with another need for the accelerator-specific info now, then I'd leave
it to be added at a later time when it is needed.
Thanks,
drew
On 3/12/21 9:48 AM, Andrew Jones wrote:
> On Fri, Mar 12, 2021 at 09:11:45AM +0100, Thomas Huth wrote:
>> On 12/03/2021 08.42, Marc-André Lureau wrote:
>>>
>>>
>>> On Fri, Mar 12, 2021 at 3:14 AM Philippe Mathieu-Daudé
>>> <philmd@redhat.com <mailto:philmd@redhat.com>> wrote:
>>>
>> [...]
>>> +##
>>> +# @AcceleratorInfo:
>>> +#
>>> +# Accelerator information.
>>> +#
>>> +# @name: The accelerator name.
>>> +#
>>> +# Since: 6.0
>>> +##
>>> +{ 'union': 'AcceleratorInfo',
>>> + 'base': {'name': 'Accelerator'},
>>> + 'discriminator': 'name',
>>> + 'data': { } }
>>>
>>> +
>>>
>>>
>>> Making room for future details, why not.
>>
>> I think we definitely need the additional data section here: For KVM on
>> POWER, it would be good to know whether it's KVM-HV or KVM-PR, for KVM on
>> MIPS it would be good to know whether it's KVM_VM_MIPS_VZ or KVM_VM_MIPS_TE,
>> for KVM on x86 whether it's the AMD flavor or Intel, ...
>
> Could also pre-expand all of these and list them individually.
Let us consider simplicity for the reader, and which use cases we expect for this.
>
>>
>>> +##
>>> +# @query-accels:
>>> +#
>>> +# Get a list of AcceleratorInfo for all built-in accelerators.
>>> +#
>>> +# Returns: a list of @AcceleratorInfo describing each accelerator.
>>> +#
>>> +# Since: 6.0
>>> +#
>>> +# Example:
>>> +#
>>> +# -> { "execute": "query-accels" }
>>> +# <- { "return": [
>>> +# {
>>> +# "type": "qtest"
>>> +# },
>>> +# {
>>> +# "type": "kvm"
>>> +# }
>>> +# ] }
>>> +#
>>> +##
>>> +{ 'command': 'query-accels',
>>> + 'returns': ['AcceleratorInfo'] }
>>>
>>>
>>> That's nice, but how do you know which accels are actually enabled?
>>
>> I guess we need two commands in the end, one for querying which accelerators
>> are available, and one for querying the data from the currently active
>> accelerator...?
>>
>
> If we listed each accelerator individually, then we could use booleans
> for them, where only the active one would be true. If we can't come up
> with another need for the accelerator-specific info now, then I'd leave
> it to be added at a later time when it is needed.
>
> Thanks,
> drew
>
On Fri, Mar 12, 2021 at 09:52:33AM +0100, Claudio Fontana wrote:
> On 3/12/21 9:48 AM, Andrew Jones wrote:
> > On Fri, Mar 12, 2021 at 09:11:45AM +0100, Thomas Huth wrote:
> >> On 12/03/2021 08.42, Marc-André Lureau wrote:
> >>>
> >>>
> >>> On Fri, Mar 12, 2021 at 3:14 AM Philippe Mathieu-Daudé
> >>> <philmd@redhat.com <mailto:philmd@redhat.com>> wrote:
> >>>
> >> [...]
> >>> +##
> >>> +# @AcceleratorInfo:
> >>> +#
> >>> +# Accelerator information.
> >>> +#
> >>> +# @name: The accelerator name.
> >>> +#
> >>> +# Since: 6.0
> >>> +##
> >>> +{ 'union': 'AcceleratorInfo',
> >>> + 'base': {'name': 'Accelerator'},
> >>> + 'discriminator': 'name',
> >>> + 'data': { } }
> >>>
> >>> +
> >>>
> >>>
> >>> Making room for future details, why not.
> >>
> >> I think we definitely need the additional data section here: For KVM on
> >> POWER, it would be good to know whether it's KVM-HV or KVM-PR, for KVM on
> >> MIPS it would be good to know whether it's KVM_VM_MIPS_VZ or KVM_VM_MIPS_TE,
> >> for KVM on x86 whether it's the AMD flavor or Intel, ...
> >
> > Could also pre-expand all of these and list them individually.
>
>
>
> Let us consider simplicity for the reader, and which use cases we expect for this.
What do you mean by "reader"? If you mean the QMP client that needs this
information, then I can't think of anything more simple than a single list
of booleans with descriptive names to process. If you mean that the
expected use cases don't care about all those variants, then you don't
need to worry about that. Only the ones compiled in will be in the list.
The expected use cases will never see the other ones.
Thanks,
drew
On 12/03/21 09:48, Andrew Jones wrote: >> I think we definitely need the additional data section here: For KVM on >> POWER, it would be good to know whether it's KVM-HV or KVM-PR, for KVM on >> MIPS it would be good to know whether it's KVM_VM_MIPS_VZ or KVM_VM_MIPS_TE, >> for KVM on x86 whether it's the AMD flavor or Intel, ... > > Could also pre-expand all of these and list them individually. That seems worse (in general) because in a lot of cases you don't care; the basic query-accels output should match the argument to "-accel". Paolo
On Fri, Mar 12, 2021 at 10:01:43AM +0100, Paolo Bonzini wrote: > On 12/03/21 09:48, Andrew Jones wrote: > > > I think we definitely need the additional data section here: For KVM on > > > POWER, it would be good to know whether it's KVM-HV or KVM-PR, for KVM on > > > MIPS it would be good to know whether it's KVM_VM_MIPS_VZ or KVM_VM_MIPS_TE, > > > for KVM on x86 whether it's the AMD flavor or Intel, ... > > > > Could also pre-expand all of these and list them individually. > > That seems worse (in general) because in a lot of cases you don't care; the > basic query-accels output should match the argument to "-accel". > For these special subtypes, what's the property/state that indicates it when just using '-accel kvm' on the command line? Because if this qmp list matches the '-accel' parameter list, then qtest and other qmp clients may need to query that other information too, in order for this accel query to be useful. And, do we need an accel-specific qmp query for it? Or, is that information already available? Thanks, drew
On 12/03/21 10:17, Andrew Jones wrote: > On Fri, Mar 12, 2021 at 10:01:43AM +0100, Paolo Bonzini wrote: >> On 12/03/21 09:48, Andrew Jones wrote: >>>> I think we definitely need the additional data section here: For KVM on >>>> POWER, it would be good to know whether it's KVM-HV or KVM-PR, for KVM on >>>> MIPS it would be good to know whether it's KVM_VM_MIPS_VZ or KVM_VM_MIPS_TE, >>>> for KVM on x86 whether it's the AMD flavor or Intel, ... >>> >>> Could also pre-expand all of these and list them individually. >> >> That seems worse (in general) because in a lot of cases you don't care; the >> basic query-accels output should match the argument to "-accel". >> > > For these special subtypes, what's the property/state that indicates it > when just using '-accel kvm' on the command line? Because if this qmp > list matches the '-accel' parameter list, then qtest and other qmp clients > may need to query that other information too, in order for this accel > query to be useful. And, do we need an accel-specific qmp query for it? > Or, is that information already available? It depends. On PPC (if I remember/understand correctly) only pseries supports both HV and PR, while all other machines only support KVM-PR. So in that case it's a kvm-type machine property that is defined only for the pseries machine. On MIPS instead there's no option and VZ always wins over TE. I think it could be made an option on -accel, but I'm not familiar with MIPS machine types. Something like "name: 'kvm', types: ['book3s-hv', 'pr']" would work nicely for KVM-PPC, and likewise for MIPS. Paolo
© 2016 - 2026 Red Hat, Inc.