qapi/machine-target.json | 43 ++++++++++++++++++++++++++++++++++++++ target/i386/kvm/kvm.c | 37 ++++++++++++++++++++++++++++++++ tests/qtest/qmp-cmd-test.c | 1 + 3 files changed, 81 insertions(+)
Introducing new qapi method 'query-kvm-cpuid'. This method can be used to
get virtualized cpu model info generated by QEMU during VM initialization in
the form of cpuid representation.
Diving into more details about virtual cpu generation: QEMU first parses '-cpu'
command line option. From there it takes the name of the model as the basis for
feature set of the new virtual cpu. After that it uses trailing '-cpu' options,
that state if additional cpu features should be present on the virtual cpu or
excluded from it (tokens '+'/'-' or '=on'/'=off').
After that QEMU checks if the host's cpu can actually support the derived
feature set and applies host limitations to it.
After this initialization procedure, virtual cpu has it's model and
vendor names, and a working feature set and is ready for identification
instructions such as CPUID.
Currently full output for this method is only supported for x86 cpus.
To learn exactly how virtual cpu is presented to the guest machine via CPUID
instruction, new qapi method can be used. By calling 'query-kvm-cpuid'
method, one can get a full listing of all CPUID leafs with subleafs which are
supported by the initialized virtual cpu.
Other than debug, the method is useful in cases when we would like to
utilize QEMU's virtual cpu initialization routines and put the retrieved
values into kernel CPUID overriding mechanics for more precise control
over how various processes perceive its underlying hardware with
container processes as a good example.
Output format:
The output is a plain list of leaf/subleaf agrument combinations, that
return 4 words in registers EAX, EBX, ECX, EDX.
Use example:
qmp_request: {
"execute": "query-kvm-cpuid"
}
qmp_response: {
"return": [
{
"eax": 1073741825,
"edx": 77,
"in-eax": 1073741824,
"ecx": 1447775574,
"ebx": 1263359563
},
{
"eax": 16777339,
"edx": 0,
"in-eax": 1073741825,
"ecx": 0,
"ebx": 0
},
{
"eax": 13,
"edx": 1231384169,
"in-eax": 0,
"ecx": 1818588270,
"ebx": 1970169159
},
{
"eax": 198354,
"edx": 126614527,
"in-eax": 1,
"ecx": 2176328193,
"ebx": 2048
},
....
{
"eax": 12328,
"edx": 0,
"in-eax": 2147483656,
"ecx": 0,
"ebx": 0
}
]
}
Signed-off-by: Valeriy Vdovin <valeriy.vdovin@virtuozzo.com>
---
v2: - Removed leaf/subleaf iterators.
- Modified cpu_x86_cpuid to return false in cases when count is
greater than supported subleaves.
v3: - Fixed structure name coding style.
- Added more comments
- Ensured buildability for non-x86 targets.
v4: - Fixed cpu_x86_cpuid return value logic and handling of 0xA leaf.
- Fixed comments.
- Removed target check in qmp_query_cpu_model_cpuid.
v5: - Added error handling code in qmp_query_cpu_model_cpuid
v6: - Fixed error handling code. Added method to query_error_class
v7: - Changed implementation in favor of cached cpuid_data for
KVM_SET_CPUID2
v8: - Renamed qmp method to query-kvm-cpuid and some fields in response.
- Modified documentation to qmp method
- Removed helper struct declaration
v9: - Renamed 'in_eax' / 'in_ecx' fields to 'in-eax' / 'in-ecx'
- Pasted more complete response to commit message.
qapi/machine-target.json | 43 ++++++++++++++++++++++++++++++++++++++
target/i386/kvm/kvm.c | 37 ++++++++++++++++++++++++++++++++
tests/qtest/qmp-cmd-test.c | 1 +
3 files changed, 81 insertions(+)
diff --git a/qapi/machine-target.json b/qapi/machine-target.json
index e7811654b7..1e591ba481 100644
--- a/qapi/machine-target.json
+++ b/qapi/machine-target.json
@@ -329,3 +329,46 @@
##
{ 'command': 'query-cpu-definitions', 'returns': ['CpuDefinitionInfo'],
'if': 'defined(TARGET_PPC) || defined(TARGET_ARM) || defined(TARGET_I386) || defined(TARGET_S390X) || defined(TARGET_MIPS)' }
+
+##
+# @CpuidEntry:
+#
+# A single entry of a CPUID response.
+#
+# One entry holds full set of information (leaf) returned to the guest in response
+# to it calling a CPUID instruction with eax, ecx used as the agruments to that
+# instruction. ecx is an optional argument as not all of the leaves support it.
+#
+# @in-eax: CPUID argument in eax
+# @in-ecx: CPUID argument in ecx
+# @eax: eax
+# @ebx: ebx
+# @ecx: ecx
+# @edx: edx
+#
+# Since: 6.1
+##
+{ 'struct': 'CpuidEntry',
+ 'data': { 'in-eax' : 'uint32',
+ '*in-ecx' : 'uint32',
+ 'eax' : 'uint32',
+ 'ebx' : 'uint32',
+ 'ecx' : 'uint32',
+ 'edx' : 'uint32'
+ },
+ 'if': 'defined(TARGET_I386) && defined(CONFIG_KVM)' }
+
+##
+# @query-kvm-cpuid:
+#
+# Returns raw data from the KVM CPUID table for the first VCPU.
+# The KVM CPUID table defines the response to the CPUID
+# instruction when executed by the guest operating system.
+#
+# Returns: a list of CpuidEntry
+#
+# Since: 6.1
+##
+{ 'command': 'query-kvm-cpuid',
+ 'returns': ['CpuidEntry'],
+ 'if': 'defined(TARGET_I386) && defined(CONFIG_KVM)' }
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 7fe9f52710..a59d6efa41 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -20,6 +20,7 @@
#include <linux/kvm.h>
#include "standard-headers/asm-x86/kvm_para.h"
+#include "qapi/qapi-commands-machine-target.h"
#include "cpu.h"
#include "sysemu/sysemu.h"
@@ -1464,6 +1465,38 @@ static Error *invtsc_mig_blocker;
#define KVM_MAX_CPUID_ENTRIES 100
+struct kvm_cpuid2 *cpuid_data_cached;
+
+CpuidEntryList *qmp_query_kvm_cpuid(Error **errp)
+{
+ int i;
+ struct kvm_cpuid_entry2 *kvm_entry;
+ CpuidEntryList *head = NULL, **tail = &head;
+ CpuidEntry *entry;
+
+ if (!cpuid_data_cached) {
+ error_setg(errp, "VCPU was not initialized yet");
+ return NULL;
+ }
+
+ for (i = 0; i < cpuid_data_cached->nent; ++i) {
+ kvm_entry = &cpuid_data_cached->entries[i];
+ entry = g_malloc0(sizeof(*entry));
+ entry->in_eax = kvm_entry->function;
+ if (kvm_entry->flags & KVM_CPUID_FLAG_SIGNIFCANT_INDEX) {
+ entry->in_ecx = kvm_entry->index;
+ entry->has_in_ecx = true;
+ }
+ entry->eax = kvm_entry->eax;
+ entry->ebx = kvm_entry->ebx;
+ entry->ecx = kvm_entry->ecx;
+ entry->edx = kvm_entry->edx;
+ QAPI_LIST_APPEND(tail, entry);
+ }
+
+ return head;
+}
+
int kvm_arch_init_vcpu(CPUState *cs)
{
struct {
@@ -1833,6 +1866,10 @@ int kvm_arch_init_vcpu(CPUState *cs)
if (r) {
goto fail;
}
+ if (!cpuid_data_cached) {
+ cpuid_data_cached = g_malloc0(sizeof(cpuid_data));
+ memcpy(cpuid_data_cached, &cpuid_data, sizeof(cpuid_data));
+ }
if (has_xsave) {
env->xsave_buf = qemu_memalign(4096, sizeof(struct kvm_xsave));
diff --git a/tests/qtest/qmp-cmd-test.c b/tests/qtest/qmp-cmd-test.c
index c98b78d033..48add3ada1 100644
--- a/tests/qtest/qmp-cmd-test.c
+++ b/tests/qtest/qmp-cmd-test.c
@@ -46,6 +46,7 @@ static int query_error_class(const char *cmd)
{ "query-balloon", ERROR_CLASS_DEVICE_NOT_ACTIVE },
{ "query-hotpluggable-cpus", ERROR_CLASS_GENERIC_ERROR },
{ "query-vm-generation-id", ERROR_CLASS_GENERIC_ERROR },
+ { "query-kvm-cpuid", ERROR_CLASS_GENERIC_ERROR },
{ NULL, -1 }
};
int i;
--
2.17.1
03.06.2021 12:07, Valeriy Vdovin wrote:
> Introducing new qapi method 'query-kvm-cpuid'. This method can be used to
> get virtualized cpu model info generated by QEMU during VM initialization in
> the form of cpuid representation.
>
> Diving into more details about virtual cpu generation: QEMU first parses '-cpu'
> command line option. From there it takes the name of the model as the basis for
> feature set of the new virtual cpu. After that it uses trailing '-cpu' options,
> that state if additional cpu features should be present on the virtual cpu or
> excluded from it (tokens '+'/'-' or '=on'/'=off').
> After that QEMU checks if the host's cpu can actually support the derived
> feature set and applies host limitations to it.
> After this initialization procedure, virtual cpu has it's model and
> vendor names, and a working feature set and is ready for identification
> instructions such as CPUID.
>
> Currently full output for this method is only supported for x86 cpus.
>
> To learn exactly how virtual cpu is presented to the guest machine via CPUID
> instruction, new qapi method can be used. By calling 'query-kvm-cpuid'
> method, one can get a full listing of all CPUID leafs with subleafs which are
> supported by the initialized virtual cpu.
>
> Other than debug, the method is useful in cases when we would like to
> utilize QEMU's virtual cpu initialization routines and put the retrieved
> values into kernel CPUID overriding mechanics for more precise control
> over how various processes perceive its underlying hardware with
> container processes as a good example.
>
> Output format:
> The output is a plain list of leaf/subleaf agrument combinations, that
> return 4 words in registers EAX, EBX, ECX, EDX.
>
> Use example:
> qmp_request: {
> "execute": "query-kvm-cpuid"
> }
>
> qmp_response: {
> "return": [
> {
> "eax": 1073741825,
> "edx": 77,
> "in-eax": 1073741824,
> "ecx": 1447775574,
> "ebx": 1263359563
> },
> {
> "eax": 16777339,
> "edx": 0,
> "in-eax": 1073741825,
> "ecx": 0,
> "ebx": 0
> },
> {
> "eax": 13,
> "edx": 1231384169,
> "in-eax": 0,
> "ecx": 1818588270,
> "ebx": 1970169159
> },
> {
> "eax": 198354,
> "edx": 126614527,
> "in-eax": 1,
> "ecx": 2176328193,
> "ebx": 2048
> },
> ....
> {
> "eax": 12328,
> "edx": 0,
> "in-eax": 2147483656,
> "ecx": 0,
> "ebx": 0
> }
> ]
> }
>
> Signed-off-by: Valeriy Vdovin<valeriy.vdovin@virtuozzo.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
--
Best regards,
Vladimir
Valeriy Vdovin <valeriy.vdovin@virtuozzo.com> writes:
> Introducing new qapi method 'query-kvm-cpuid'. This method can be used to
It's actually a QMP command. There are no "qapi methods".
> get virtualized cpu model info generated by QEMU during VM initialization in
> the form of cpuid representation.
>
> Diving into more details about virtual cpu generation: QEMU first parses '-cpu'
virtual CPU
> command line option. From there it takes the name of the model as the basis for
> feature set of the new virtual cpu. After that it uses trailing '-cpu' options,
> that state if additional cpu features should be present on the virtual cpu or
> excluded from it (tokens '+'/'-' or '=on'/'=off').
> After that QEMU checks if the host's cpu can actually support the derived
> feature set and applies host limitations to it.
> After this initialization procedure, virtual cpu has it's model and
> vendor names, and a working feature set and is ready for identification
> instructions such as CPUID.
>
> Currently full output for this method is only supported for x86 cpus.
Not sure about "currently": the interface looks quite x86-specific to me.
The commit message doesn't mention KVM except in the command name. The
schema provides the command only if defined(CONFIG_KVM).
Can you explain why you need the restriction to CONFIG_KVM?
> To learn exactly how virtual cpu is presented to the guest machine via CPUID
> instruction, new qapi method can be used. By calling 'query-kvm-cpuid'
QMP command again.
> method, one can get a full listing of all CPUID leafs with subleafs which are
> supported by the initialized virtual cpu.
>
> Other than debug, the method is useful in cases when we would like to
> utilize QEMU's virtual cpu initialization routines and put the retrieved
> values into kernel CPUID overriding mechanics for more precise control
> over how various processes perceive its underlying hardware with
> container processes as a good example.
>
> Output format:
> The output is a plain list of leaf/subleaf agrument combinations, that
Typo: argument
> return 4 words in registers EAX, EBX, ECX, EDX.
>
> Use example:
> qmp_request: {
> "execute": "query-kvm-cpuid"
> }
>
> qmp_response: {
> "return": [
> {
> "eax": 1073741825,
> "edx": 77,
> "in-eax": 1073741824,
> "ecx": 1447775574,
> "ebx": 1263359563
> },
> {
> "eax": 16777339,
> "edx": 0,
> "in-eax": 1073741825,
> "ecx": 0,
> "ebx": 0
> },
> {
> "eax": 13,
> "edx": 1231384169,
> "in-eax": 0,
> "ecx": 1818588270,
> "ebx": 1970169159
> },
> {
> "eax": 198354,
> "edx": 126614527,
> "in-eax": 1,
> "ecx": 2176328193,
> "ebx": 2048
> },
> ....
> {
> "eax": 12328,
> "edx": 0,
> "in-eax": 2147483656,
> "ecx": 0,
> "ebx": 0
> }
> ]
> }
>
> Signed-off-by: Valeriy Vdovin <valeriy.vdovin@virtuozzo.com>
> ---
> v2: - Removed leaf/subleaf iterators.
> - Modified cpu_x86_cpuid to return false in cases when count is
> greater than supported subleaves.
> v3: - Fixed structure name coding style.
> - Added more comments
> - Ensured buildability for non-x86 targets.
> v4: - Fixed cpu_x86_cpuid return value logic and handling of 0xA leaf.
> - Fixed comments.
> - Removed target check in qmp_query_cpu_model_cpuid.
> v5: - Added error handling code in qmp_query_cpu_model_cpuid
> v6: - Fixed error handling code. Added method to query_error_class
> v7: - Changed implementation in favor of cached cpuid_data for
> KVM_SET_CPUID2
> v8: - Renamed qmp method to query-kvm-cpuid and some fields in response.
> - Modified documentation to qmp method
> - Removed helper struct declaration
> v9: - Renamed 'in_eax' / 'in_ecx' fields to 'in-eax' / 'in-ecx'
> - Pasted more complete response to commit message.
>
> qapi/machine-target.json | 43 ++++++++++++++++++++++++++++++++++++++
> target/i386/kvm/kvm.c | 37 ++++++++++++++++++++++++++++++++
> tests/qtest/qmp-cmd-test.c | 1 +
> 3 files changed, 81 insertions(+)
>
> diff --git a/qapi/machine-target.json b/qapi/machine-target.json
> index e7811654b7..1e591ba481 100644
> --- a/qapi/machine-target.json
> +++ b/qapi/machine-target.json
> @@ -329,3 +329,46 @@
> ##
> { 'command': 'query-cpu-definitions', 'returns': ['CpuDefinitionInfo'],
> 'if': 'defined(TARGET_PPC) || defined(TARGET_ARM) || defined(TARGET_I386) || defined(TARGET_S390X) || defined(TARGET_MIPS)' }
> +
> +##
> +# @CpuidEntry:
> +#
> +# A single entry of a CPUID response.
> +#
> +# One entry holds full set of information (leaf) returned to the guest in response
> +# to it calling a CPUID instruction with eax, ecx used as the agruments to that
Typi: agruments
> +# instruction. ecx is an optional argument as not all of the leaves support it.
Please wrap doc comment lines around column 70.
> +#
> +# @in-eax: CPUID argument in eax
> +# @in-ecx: CPUID argument in ecx
> +# @eax: eax
> +# @ebx: ebx
> +# @ecx: ecx
> +# @edx: edx
Suggest
# @eax: CPUID result in eax
and so forth.
> +#
> +# Since: 6.1
> +##
> +{ 'struct': 'CpuidEntry',
> + 'data': { 'in-eax' : 'uint32',
> + '*in-ecx' : 'uint32',
> + 'eax' : 'uint32',
> + 'ebx' : 'uint32',
> + 'ecx' : 'uint32',
> + 'edx' : 'uint32'
> + },
> + 'if': 'defined(TARGET_I386) && defined(CONFIG_KVM)' }
> +
> +##
> +# @query-kvm-cpuid:
> +#
> +# Returns raw data from the KVM CPUID table for the first VCPU.
> +# The KVM CPUID table defines the response to the CPUID
> +# instruction when executed by the guest operating system.
> +#
> +# Returns: a list of CpuidEntry
> +#
> +# Since: 6.1
> +##
> +{ 'command': 'query-kvm-cpuid',
> + 'returns': ['CpuidEntry'],
> + 'if': 'defined(TARGET_I386) && defined(CONFIG_KVM)' }
Is this intended to be a stable interface? Interfaces intended just for
debugging usually aren't.
[...]
On Thu, Jun 17, 2021 at 07:22:36AM +0200, Markus Armbruster wrote:
> Valeriy Vdovin <valeriy.vdovin@virtuozzo.com> writes:
>
> > Introducing new qapi method 'query-kvm-cpuid'. This method can be used to
>
> It's actually a QMP command. There are no "qapi methods".
>
> > get virtualized cpu model info generated by QEMU during VM initialization in
> > the form of cpuid representation.
> >
> > Diving into more details about virtual cpu generation: QEMU first parses '-cpu'
>
> virtual CPU
>
> > command line option. From there it takes the name of the model as the basis for
> > feature set of the new virtual cpu. After that it uses trailing '-cpu' options,
> > that state if additional cpu features should be present on the virtual cpu or
> > excluded from it (tokens '+'/'-' or '=on'/'=off').
> > After that QEMU checks if the host's cpu can actually support the derived
> > feature set and applies host limitations to it.
> > After this initialization procedure, virtual cpu has it's model and
> > vendor names, and a working feature set and is ready for identification
> > instructions such as CPUID.
> >
> > Currently full output for this method is only supported for x86 cpus.
>
> Not sure about "currently": the interface looks quite x86-specific to me.
>
Yes, at some point I was thinking this interface could become generic,
but does not seem possible, so I'll remove this note.
> The commit message doesn't mention KVM except in the command name. The
> schema provides the command only if defined(CONFIG_KVM).
>
> Can you explain why you need the restriction to CONFIG_KVM?
>
This CONFIG_KVM is used as a solution to a broken build if --disable-kvm
flag is set. I was choosing between this and writing empty implementation into
kvm-stub.c
> > To learn exactly how virtual cpu is presented to the guest machine via CPUID
> > instruction, new qapi method can be used. By calling 'query-kvm-cpuid'
>
> QMP command again.
>
> > method, one can get a full listing of all CPUID leafs with subleafs which are
> > supported by the initialized virtual cpu.
> >
> > Other than debug, the method is useful in cases when we would like to
> > utilize QEMU's virtual cpu initialization routines and put the retrieved
> > values into kernel CPUID overriding mechanics for more precise control
> > over how various processes perceive its underlying hardware with
> > container processes as a good example.
> >
> > Output format:
> > The output is a plain list of leaf/subleaf agrument combinations, that
>
> Typo: argument
>
> > return 4 words in registers EAX, EBX, ECX, EDX.
> >
> > Use example:
> > qmp_request: {
> > "execute": "query-kvm-cpuid"
> > }
> >
> > qmp_response: {
> > "return": [
> > {
> > "eax": 1073741825,
> > "edx": 77,
> > "in-eax": 1073741824,
> > "ecx": 1447775574,
> > "ebx": 1263359563
> > },
> > {
> > "eax": 16777339,
> > "edx": 0,
> > "in-eax": 1073741825,
> > "ecx": 0,
> > "ebx": 0
> > },
> > {
> > "eax": 13,
> > "edx": 1231384169,
> > "in-eax": 0,
> > "ecx": 1818588270,
> > "ebx": 1970169159
> > },
> > {
> > "eax": 198354,
> > "edx": 126614527,
> > "in-eax": 1,
> > "ecx": 2176328193,
> > "ebx": 2048
> > },
> > ....
> > {
> > "eax": 12328,
> > "edx": 0,
> > "in-eax": 2147483656,
> > "ecx": 0,
> > "ebx": 0
> > }
> > ]
> > }
> >
> > Signed-off-by: Valeriy Vdovin <valeriy.vdovin@virtuozzo.com>
> > ---
> > v2: - Removed leaf/subleaf iterators.
> > - Modified cpu_x86_cpuid to return false in cases when count is
> > greater than supported subleaves.
> > v3: - Fixed structure name coding style.
> > - Added more comments
> > - Ensured buildability for non-x86 targets.
> > v4: - Fixed cpu_x86_cpuid return value logic and handling of 0xA leaf.
> > - Fixed comments.
> > - Removed target check in qmp_query_cpu_model_cpuid.
> > v5: - Added error handling code in qmp_query_cpu_model_cpuid
> > v6: - Fixed error handling code. Added method to query_error_class
> > v7: - Changed implementation in favor of cached cpuid_data for
> > KVM_SET_CPUID2
> > v8: - Renamed qmp method to query-kvm-cpuid and some fields in response.
> > - Modified documentation to qmp method
> > - Removed helper struct declaration
> > v9: - Renamed 'in_eax' / 'in_ecx' fields to 'in-eax' / 'in-ecx'
> > - Pasted more complete response to commit message.
> >
> > qapi/machine-target.json | 43 ++++++++++++++++++++++++++++++++++++++
> > target/i386/kvm/kvm.c | 37 ++++++++++++++++++++++++++++++++
> > tests/qtest/qmp-cmd-test.c | 1 +
> > 3 files changed, 81 insertions(+)
> >
> > diff --git a/qapi/machine-target.json b/qapi/machine-target.json
> > index e7811654b7..1e591ba481 100644
> > --- a/qapi/machine-target.json
> > +++ b/qapi/machine-target.json
> > @@ -329,3 +329,46 @@
> > ##
> > { 'command': 'query-cpu-definitions', 'returns': ['CpuDefinitionInfo'],
> > 'if': 'defined(TARGET_PPC) || defined(TARGET_ARM) || defined(TARGET_I386) || defined(TARGET_S390X) || defined(TARGET_MIPS)' }
> > +
> > +##
> > +# @CpuidEntry:
> > +#
> > +# A single entry of a CPUID response.
> > +#
> > +# One entry holds full set of information (leaf) returned to the guest in response
> > +# to it calling a CPUID instruction with eax, ecx used as the agruments to that
>
> Typi: agruments
>
> > +# instruction. ecx is an optional argument as not all of the leaves support it.
>
> Please wrap doc comment lines around column 70.
>
> > +#
> > +# @in-eax: CPUID argument in eax
> > +# @in-ecx: CPUID argument in ecx
> > +# @eax: eax
> > +# @ebx: ebx
> > +# @ecx: ecx
> > +# @edx: edx
>
> Suggest
>
> # @eax: CPUID result in eax
>
> and so forth.
>
> > +#
> > +# Since: 6.1
> > +##
> > +{ 'struct': 'CpuidEntry',
> > + 'data': { 'in-eax' : 'uint32',
> > + '*in-ecx' : 'uint32',
> > + 'eax' : 'uint32',
> > + 'ebx' : 'uint32',
> > + 'ecx' : 'uint32',
> > + 'edx' : 'uint32'
> > + },
> > + 'if': 'defined(TARGET_I386) && defined(CONFIG_KVM)' }
> > +
> > +##
> > +# @query-kvm-cpuid:
> > +#
> > +# Returns raw data from the KVM CPUID table for the first VCPU.
> > +# The KVM CPUID table defines the response to the CPUID
> > +# instruction when executed by the guest operating system.
> > +#
> > +# Returns: a list of CpuidEntry
> > +#
> > +# Since: 6.1
> > +##
> > +{ 'command': 'query-kvm-cpuid',
> > + 'returns': ['CpuidEntry'],
> > + 'if': 'defined(TARGET_I386) && defined(CONFIG_KVM)' }
>
> Is this intended to be a stable interface? Interfaces intended just for
> debugging usually aren't.
>
It is intented to be used as a stable interface.
> [...]
>
On 6/17/21 9:49 AM, Valeriy Vdovin wrote:
> On Thu, Jun 17, 2021 at 07:22:36AM +0200, Markus Armbruster wrote:
>> Valeriy Vdovin <valeriy.vdovin@virtuozzo.com> writes:
>>
>>> Introducing new qapi method 'query-kvm-cpuid'. This method can be used to
>>
>> It's actually a QMP command. There are no "qapi methods".
>>
>>> get virtualized cpu model info generated by QEMU during VM initialization in
>>> the form of cpuid representation.
>>>
>>> Diving into more details about virtual cpu generation: QEMU first parses '-cpu'
>>
>> virtual CPU
>>
>>> command line option. From there it takes the name of the model as the basis for
>>> feature set of the new virtual cpu. After that it uses trailing '-cpu' options,
>>> that state if additional cpu features should be present on the virtual cpu or
>>> excluded from it (tokens '+'/'-' or '=on'/'=off').
>>> After that QEMU checks if the host's cpu can actually support the derived
>>> feature set and applies host limitations to it.
>>> After this initialization procedure, virtual cpu has it's model and
>>> vendor names, and a working feature set and is ready for identification
>>> instructions such as CPUID.
>>>
>>> Currently full output for this method is only supported for x86 cpus.
>>
>> Not sure about "currently": the interface looks quite x86-specific to me.
>>
> Yes, at some point I was thinking this interface could become generic,
> but does not seem possible, so I'll remove this note.
Why is it impossible? What is the use case, is it something useful for example for ARM?
>
>> The commit message doesn't mention KVM except in the command name. The
>> schema provides the command only if defined(CONFIG_KVM).
>>
>> Can you explain why you need the restriction to CONFIG_KVM?
>>
> This CONFIG_KVM is used as a solution to a broken build if --disable-kvm
> flag is set. I was choosing between this and writing empty implementation into
> kvm-stub.c
>>> To learn exactly how virtual cpu is presented to the guest machine via CPUID
>>> instruction, new qapi method can be used. By calling 'query-kvm-cpuid'
>>
>> QMP command again.
>>
>>> method, one can get a full listing of all CPUID leafs with subleafs which are
>>> supported by the initialized virtual cpu.
>>>
>>> Other than debug, the method is useful in cases when we would like to
>>> utilize QEMU's virtual cpu initialization routines and put the retrieved
>>> values into kernel CPUID overriding mechanics for more precise control
>>> over how various processes perceive its underlying hardware with
>>> container processes as a good example.
>>>
>>> Output format:
>>> The output is a plain list of leaf/subleaf agrument combinations, that
>>
>> Typo: argument
>>
>>> return 4 words in registers EAX, EBX, ECX, EDX.
>>>
>>> Use example:
>>> qmp_request: {
>>> "execute": "query-kvm-cpuid"
>>> }
>>>
>>> qmp_response: {
>>> "return": [
>>> {
>>> "eax": 1073741825,
>>> "edx": 77,
>>> "in-eax": 1073741824,
>>> "ecx": 1447775574,
>>> "ebx": 1263359563
>>> },
>>> {
>>> "eax": 16777339,
>>> "edx": 0,
>>> "in-eax": 1073741825,
>>> "ecx": 0,
>>> "ebx": 0
>>> },
>>> {
>>> "eax": 13,
>>> "edx": 1231384169,
>>> "in-eax": 0,
>>> "ecx": 1818588270,
>>> "ebx": 1970169159
>>> },
>>> {
>>> "eax": 198354,
>>> "edx": 126614527,
>>> "in-eax": 1,
>>> "ecx": 2176328193,
>>> "ebx": 2048
>>> },
>>> ....
>>> {
>>> "eax": 12328,
>>> "edx": 0,
>>> "in-eax": 2147483656,
>>> "ecx": 0,
>>> "ebx": 0
>>> }
>>> ]
>>> }
>>>
>>> Signed-off-by: Valeriy Vdovin <valeriy.vdovin@virtuozzo.com>
>>> ---
>>> v2: - Removed leaf/subleaf iterators.
>>> - Modified cpu_x86_cpuid to return false in cases when count is
>>> greater than supported subleaves.
>>> v3: - Fixed structure name coding style.
>>> - Added more comments
>>> - Ensured buildability for non-x86 targets.
>>> v4: - Fixed cpu_x86_cpuid return value logic and handling of 0xA leaf.
>>> - Fixed comments.
>>> - Removed target check in qmp_query_cpu_model_cpuid.
>>> v5: - Added error handling code in qmp_query_cpu_model_cpuid
>>> v6: - Fixed error handling code. Added method to query_error_class
>>> v7: - Changed implementation in favor of cached cpuid_data for
>>> KVM_SET_CPUID2
>>> v8: - Renamed qmp method to query-kvm-cpuid and some fields in response.
>>> - Modified documentation to qmp method
>>> - Removed helper struct declaration
>>> v9: - Renamed 'in_eax' / 'in_ecx' fields to 'in-eax' / 'in-ecx'
>>> - Pasted more complete response to commit message.
>>>
>>> qapi/machine-target.json | 43 ++++++++++++++++++++++++++++++++++++++
>>> target/i386/kvm/kvm.c | 37 ++++++++++++++++++++++++++++++++
>>> tests/qtest/qmp-cmd-test.c | 1 +
>>> 3 files changed, 81 insertions(+)
>>>
>>> diff --git a/qapi/machine-target.json b/qapi/machine-target.json
>>> index e7811654b7..1e591ba481 100644
>>> --- a/qapi/machine-target.json
>>> +++ b/qapi/machine-target.json
>>> @@ -329,3 +329,46 @@
>>> ##
>>> { 'command': 'query-cpu-definitions', 'returns': ['CpuDefinitionInfo'],
>>> 'if': 'defined(TARGET_PPC) || defined(TARGET_ARM) || defined(TARGET_I386) || defined(TARGET_S390X) || defined(TARGET_MIPS)' }
>>> +
>>> +##
>>> +# @CpuidEntry:
>>> +#
>>> +# A single entry of a CPUID response.
>>> +#
>>> +# One entry holds full set of information (leaf) returned to the guest in response
>>> +# to it calling a CPUID instruction with eax, ecx used as the agruments to that
>>
>> Typi: agruments
>>
>>> +# instruction. ecx is an optional argument as not all of the leaves support it.
>>
>> Please wrap doc comment lines around column 70.
>>
>>> +#
>>> +# @in-eax: CPUID argument in eax
>>> +# @in-ecx: CPUID argument in ecx
>>> +# @eax: eax
>>> +# @ebx: ebx
>>> +# @ecx: ecx
>>> +# @edx: edx
>>
>> Suggest
>>
>> # @eax: CPUID result in eax
>>
>> and so forth.
>>
>>> +#
>>> +# Since: 6.1
>>> +##
>>> +{ 'struct': 'CpuidEntry',
>>> + 'data': { 'in-eax' : 'uint32',
>>> + '*in-ecx' : 'uint32',
>>> + 'eax' : 'uint32',
>>> + 'ebx' : 'uint32',
>>> + 'ecx' : 'uint32',
>>> + 'edx' : 'uint32'
>>> + },
>>> + 'if': 'defined(TARGET_I386) && defined(CONFIG_KVM)' }
>>> +
>>> +##
>>> +# @query-kvm-cpuid:
>>> +#
>>> +# Returns raw data from the KVM CPUID table for the first VCPU.
>>> +# The KVM CPUID table defines the response to the CPUID
>>> +# instruction when executed by the guest operating system.
>>> +#
>>> +# Returns: a list of CpuidEntry
>>> +#
>>> +# Since: 6.1
>>> +##
>>> +{ 'command': 'query-kvm-cpuid',
>>> + 'returns': ['CpuidEntry'],
>>> + 'if': 'defined(TARGET_I386) && defined(CONFIG_KVM)' }
>>
>> Is this intended to be a stable interface? Interfaces intended just for
>> debugging usually aren't.
>>
> It is intented to be used as a stable interface.
>> [...]
>>
>
On Thu, Jun 17, 2021 at 01:58:09PM +0200, Claudio Fontana wrote:
> On 6/17/21 9:49 AM, Valeriy Vdovin wrote:
> > On Thu, Jun 17, 2021 at 07:22:36AM +0200, Markus Armbruster wrote:
> >> Valeriy Vdovin <valeriy.vdovin@virtuozzo.com> writes:
> >>
> >>> Introducing new qapi method 'query-kvm-cpuid'. This method can be used to
> >>
> >> It's actually a QMP command. There are no "qapi methods".
> >>
> >>> get virtualized cpu model info generated by QEMU during VM initialization in
> >>> the form of cpuid representation.
> >>>
> >>> Diving into more details about virtual cpu generation: QEMU first parses '-cpu'
> >>
> >> virtual CPU
> >>
> >>> command line option. From there it takes the name of the model as the basis for
> >>> feature set of the new virtual cpu. After that it uses trailing '-cpu' options,
> >>> that state if additional cpu features should be present on the virtual cpu or
> >>> excluded from it (tokens '+'/'-' or '=on'/'=off').
> >>> After that QEMU checks if the host's cpu can actually support the derived
> >>> feature set and applies host limitations to it.
> >>> After this initialization procedure, virtual cpu has it's model and
> >>> vendor names, and a working feature set and is ready for identification
> >>> instructions such as CPUID.
> >>>
> >>> Currently full output for this method is only supported for x86 cpus.
> >>
> >> Not sure about "currently": the interface looks quite x86-specific to me.
> >>
> > Yes, at some point I was thinking this interface could become generic,
> > but does not seem possible, so I'll remove this note.
>
>
> Why is it impossible? What is the use case, is it something useful for example for ARM?
CPUID is an x86 instruction that KVM virtualizes and which responses we would like
to retrieve(the usecase) here via this new command.
In ARM there is no single CPUID instruction as such, so to get equivalent information
one would read a list of system registers with say an MRS instruction.
At first I was thinking that it would be easy to fit this into some
sophisticated abstraction of a key-value type with some conditionals,
but then I understood some things:
- the data types are conceptually different.
- By the comments in the first versions I've figured out complex code for nothing is
not welcome here, and this generalization will bring additional complexity and
confusion 100%.
I said it's impossible to generalize them, well of course actully it is possible to have
one command and one data structure that can represent both, but it would get uglier with
each new architecture added and each new architecture will bring the risk of expanding
the existing data structure potentially breaking old tools.
So what for?
>
>
> >
> >> The commit message doesn't mention KVM except in the command name. The
> >> schema provides the command only if defined(CONFIG_KVM).
> >>
> >> Can you explain why you need the restriction to CONFIG_KVM?
> >>
> > This CONFIG_KVM is used as a solution to a broken build if --disable-kvm
> > flag is set. I was choosing between this and writing empty implementation into
> > kvm-stub.c
> >>> To learn exactly how virtual cpu is presented to the guest machine via CPUID
> >>> instruction, new qapi method can be used. By calling 'query-kvm-cpuid'
> >>
> >> QMP command again.
> >>
> >>> method, one can get a full listing of all CPUID leafs with subleafs which are
> >>> supported by the initialized virtual cpu.
> >>>
> >>> Other than debug, the method is useful in cases when we would like to
> >>> utilize QEMU's virtual cpu initialization routines and put the retrieved
> >>> values into kernel CPUID overriding mechanics for more precise control
> >>> over how various processes perceive its underlying hardware with
> >>> container processes as a good example.
> >>>
> >>> Output format:
> >>> The output is a plain list of leaf/subleaf agrument combinations, that
> >>
> >> Typo: argument
> >>
> >>> return 4 words in registers EAX, EBX, ECX, EDX.
> >>>
> >>> Use example:
> >>> qmp_request: {
> >>> "execute": "query-kvm-cpuid"
> >>> }
> >>>
> >>> qmp_response: {
> >>> "return": [
> >>> {
> >>> "eax": 1073741825,
> >>> "edx": 77,
> >>> "in-eax": 1073741824,
> >>> "ecx": 1447775574,
> >>> "ebx": 1263359563
> >>> },
> >>> {
> >>> "eax": 16777339,
> >>> "edx": 0,
> >>> "in-eax": 1073741825,
> >>> "ecx": 0,
> >>> "ebx": 0
> >>> },
> >>> {
> >>> "eax": 13,
> >>> "edx": 1231384169,
> >>> "in-eax": 0,
> >>> "ecx": 1818588270,
> >>> "ebx": 1970169159
> >>> },
> >>> {
> >>> "eax": 198354,
> >>> "edx": 126614527,
> >>> "in-eax": 1,
> >>> "ecx": 2176328193,
> >>> "ebx": 2048
> >>> },
> >>> ....
> >>> {
> >>> "eax": 12328,
> >>> "edx": 0,
> >>> "in-eax": 2147483656,
> >>> "ecx": 0,
> >>> "ebx": 0
> >>> }
> >>> ]
> >>> }
> >>>
> >>> Signed-off-by: Valeriy Vdovin <valeriy.vdovin@virtuozzo.com>
> >>> ---
> >>> v2: - Removed leaf/subleaf iterators.
> >>> - Modified cpu_x86_cpuid to return false in cases when count is
> >>> greater than supported subleaves.
> >>> v3: - Fixed structure name coding style.
> >>> - Added more comments
> >>> - Ensured buildability for non-x86 targets.
> >>> v4: - Fixed cpu_x86_cpuid return value logic and handling of 0xA leaf.
> >>> - Fixed comments.
> >>> - Removed target check in qmp_query_cpu_model_cpuid.
> >>> v5: - Added error handling code in qmp_query_cpu_model_cpuid
> >>> v6: - Fixed error handling code. Added method to query_error_class
> >>> v7: - Changed implementation in favor of cached cpuid_data for
> >>> KVM_SET_CPUID2
> >>> v8: - Renamed qmp method to query-kvm-cpuid and some fields in response.
> >>> - Modified documentation to qmp method
> >>> - Removed helper struct declaration
> >>> v9: - Renamed 'in_eax' / 'in_ecx' fields to 'in-eax' / 'in-ecx'
> >>> - Pasted more complete response to commit message.
> >>>
> >>> qapi/machine-target.json | 43 ++++++++++++++++++++++++++++++++++++++
> >>> target/i386/kvm/kvm.c | 37 ++++++++++++++++++++++++++++++++
> >>> tests/qtest/qmp-cmd-test.c | 1 +
> >>> 3 files changed, 81 insertions(+)
> >>>
> >>> diff --git a/qapi/machine-target.json b/qapi/machine-target.json
> >>> index e7811654b7..1e591ba481 100644
> >>> --- a/qapi/machine-target.json
> >>> +++ b/qapi/machine-target.json
> >>> @@ -329,3 +329,46 @@
> >>> ##
> >>> { 'command': 'query-cpu-definitions', 'returns': ['CpuDefinitionInfo'],
> >>> 'if': 'defined(TARGET_PPC) || defined(TARGET_ARM) || defined(TARGET_I386) || defined(TARGET_S390X) || defined(TARGET_MIPS)' }
> >>> +
> >>> +##
> >>> +# @CpuidEntry:
> >>> +#
> >>> +# A single entry of a CPUID response.
> >>> +#
> >>> +# One entry holds full set of information (leaf) returned to the guest in response
> >>> +# to it calling a CPUID instruction with eax, ecx used as the agruments to that
> >>
> >> Typi: agruments
> >>
> >>> +# instruction. ecx is an optional argument as not all of the leaves support it.
> >>
> >> Please wrap doc comment lines around column 70.
> >>
> >>> +#
> >>> +# @in-eax: CPUID argument in eax
> >>> +# @in-ecx: CPUID argument in ecx
> >>> +# @eax: eax
> >>> +# @ebx: ebx
> >>> +# @ecx: ecx
> >>> +# @edx: edx
> >>
> >> Suggest
> >>
> >> # @eax: CPUID result in eax
> >>
> >> and so forth.
> >>
> >>> +#
> >>> +# Since: 6.1
> >>> +##
> >>> +{ 'struct': 'CpuidEntry',
> >>> + 'data': { 'in-eax' : 'uint32',
> >>> + '*in-ecx' : 'uint32',
> >>> + 'eax' : 'uint32',
> >>> + 'ebx' : 'uint32',
> >>> + 'ecx' : 'uint32',
> >>> + 'edx' : 'uint32'
> >>> + },
> >>> + 'if': 'defined(TARGET_I386) && defined(CONFIG_KVM)' }
> >>> +
> >>> +##
> >>> +# @query-kvm-cpuid:
> >>> +#
> >>> +# Returns raw data from the KVM CPUID table for the first VCPU.
> >>> +# The KVM CPUID table defines the response to the CPUID
> >>> +# instruction when executed by the guest operating system.
> >>> +#
> >>> +# Returns: a list of CpuidEntry
> >>> +#
> >>> +# Since: 6.1
> >>> +##
> >>> +{ 'command': 'query-kvm-cpuid',
> >>> + 'returns': ['CpuidEntry'],
> >>> + 'if': 'defined(TARGET_I386) && defined(CONFIG_KVM)' }
> >>
> >> Is this intended to be a stable interface? Interfaces intended just for
> >> debugging usually aren't.
> >>
> > It is intented to be used as a stable interface.
> >> [...]
> >>
> >
>
Valeriy Vdovin <valeriy.vdovin@virtuozzo.com> writes: > On Thu, Jun 17, 2021 at 07:22:36AM +0200, Markus Armbruster wrote: >> Valeriy Vdovin <valeriy.vdovin@virtuozzo.com> writes: >> >> > Introducing new qapi method 'query-kvm-cpuid'. This method can be used to >> >> It's actually a QMP command. There are no "qapi methods". >> >> > get virtualized cpu model info generated by QEMU during VM initialization in >> > the form of cpuid representation. >> > >> > Diving into more details about virtual cpu generation: QEMU first parses '-cpu' >> >> virtual CPU >> >> > command line option. From there it takes the name of the model as the basis for >> > feature set of the new virtual cpu. After that it uses trailing '-cpu' options, >> > that state if additional cpu features should be present on the virtual cpu or >> > excluded from it (tokens '+'/'-' or '=on'/'=off'). >> > After that QEMU checks if the host's cpu can actually support the derived >> > feature set and applies host limitations to it. >> > After this initialization procedure, virtual cpu has it's model and >> > vendor names, and a working feature set and is ready for identification >> > instructions such as CPUID. >> > >> > Currently full output for this method is only supported for x86 cpus. >> >> Not sure about "currently": the interface looks quite x86-specific to me. >> > Yes, at some point I was thinking this interface could become generic, > but does not seem possible, so I'll remove this note. > >> The commit message doesn't mention KVM except in the command name. The >> schema provides the command only if defined(CONFIG_KVM). >> >> Can you explain why you need the restriction to CONFIG_KVM? >> > This CONFIG_KVM is used as a solution to a broken build if --disable-kvm > flag is set. I was choosing between this and writing empty implementation into > kvm-stub.c If the command only makes sense for KVM, then it's named correctly, but the commit message lacks a (brief!) explanation why it only makes for KVM. If it just isn't implemented for anything but KVM, then putting "kvm" into the command name is a bad idea. Also, the commit message should briefly note the restriction to KVM. Pick one :) [...]
On 6/17/21 1:09 PM, Markus Armbruster wrote: > Valeriy Vdovin <valeriy.vdovin@virtuozzo.com> writes: > >> On Thu, Jun 17, 2021 at 07:22:36AM +0200, Markus Armbruster wrote: >>> Valeriy Vdovin <valeriy.vdovin@virtuozzo.com> writes: >>> >>>> Introducing new qapi method 'query-kvm-cpuid'. This method can be used to >>> >>> It's actually a QMP command. There are no "qapi methods". >>> >>>> get virtualized cpu model info generated by QEMU during VM initialization in >>>> the form of cpuid representation. >>>> >>>> Diving into more details about virtual cpu generation: QEMU first parses '-cpu' >>> >>> virtual CPU >>> >>>> command line option. From there it takes the name of the model as the basis for >>>> feature set of the new virtual cpu. After that it uses trailing '-cpu' options, >>>> that state if additional cpu features should be present on the virtual cpu or >>>> excluded from it (tokens '+'/'-' or '=on'/'=off'). >>>> After that QEMU checks if the host's cpu can actually support the derived >>>> feature set and applies host limitations to it. >>>> After this initialization procedure, virtual cpu has it's model and >>>> vendor names, and a working feature set and is ready for identification >>>> instructions such as CPUID. >>>> >>>> Currently full output for this method is only supported for x86 cpus. >>> >>> Not sure about "currently": the interface looks quite x86-specific to me. >>> >> Yes, at some point I was thinking this interface could become generic, >> but does not seem possible, so I'll remove this note. >> >>> The commit message doesn't mention KVM except in the command name. The >>> schema provides the command only if defined(CONFIG_KVM). >>> >>> Can you explain why you need the restriction to CONFIG_KVM? >>> >> This CONFIG_KVM is used as a solution to a broken build if --disable-kvm >> flag is set. I was choosing between this and writing empty implementation into >> kvm-stub.c > > If the command only makes sense for KVM, then it's named correctly, but > the commit message lacks a (brief!) explanation why it only makes for > KVM. Is it meaningful for HVF? > > If it just isn't implemented for anything but KVM, then putting "kvm" > into the command name is a bad idea. Also, the commit message should > briefly note the restriction to KVM. > > Pick one :) > > [...] > >
Claudio Fontana <cfontana@suse.de> writes:
> On 6/17/21 1:09 PM, Markus Armbruster wrote:
>> Valeriy Vdovin <valeriy.vdovin@virtuozzo.com> writes:
>>
>>> On Thu, Jun 17, 2021 at 07:22:36AM +0200, Markus Armbruster wrote:
>>>> Valeriy Vdovin <valeriy.vdovin@virtuozzo.com> writes:
>>>>
>>>>> Introducing new qapi method 'query-kvm-cpuid'. This method can be used to
>>>>
>>>> It's actually a QMP command. There are no "qapi methods".
>>>>
>>>>> get virtualized cpu model info generated by QEMU during VM initialization in
>>>>> the form of cpuid representation.
>>>>>
>>>>> Diving into more details about virtual cpu generation: QEMU first parses '-cpu'
>>>>
>>>> virtual CPU
>>>>
>>>>> command line option. From there it takes the name of the model as the basis for
>>>>> feature set of the new virtual cpu. After that it uses trailing '-cpu' options,
>>>>> that state if additional cpu features should be present on the virtual cpu or
>>>>> excluded from it (tokens '+'/'-' or '=on'/'=off').
>>>>> After that QEMU checks if the host's cpu can actually support the derived
>>>>> feature set and applies host limitations to it.
>>>>> After this initialization procedure, virtual cpu has it's model and
>>>>> vendor names, and a working feature set and is ready for identification
>>>>> instructions such as CPUID.
>>>>>
>>>>> Currently full output for this method is only supported for x86 cpus.
>>>>
>>>> Not sure about "currently": the interface looks quite x86-specific to me.
>>>>
>>> Yes, at some point I was thinking this interface could become generic,
>>> but does not seem possible, so I'll remove this note.
>>>
>>>> The commit message doesn't mention KVM except in the command name. The
>>>> schema provides the command only if defined(CONFIG_KVM).
>>>>
>>>> Can you explain why you need the restriction to CONFIG_KVM?
>>>>
>>> This CONFIG_KVM is used as a solution to a broken build if --disable-kvm
>>> flag is set. I was choosing between this and writing empty implementation into
>>> kvm-stub.c
>>
>> If the command only makes sense for KVM, then it's named correctly, but
>> the commit message lacks a (brief!) explanation why it only makes for
>> KVM.
>
>
> Is it meaningful for HVF?
I can't see why it couldn't be.
Different tack: if KVM is compiled out entirely, the command isn't
there, and trying to use it fails like
{"error": {"class": "CommandNotFound", "desc": "The command query-kvm-cpuid has not been found"}}
If KVM is compiled in, but disabled, e.g. with -machine accel=tcg, then
the command fails like
{"error": {"class": "GenericError", "desc": "VCPU was not initialized yet"}}
This is misleading. The VCPU is actually running, it's just the wrong
kind of VCPU.
>> If it just isn't implemented for anything but KVM, then putting "kvm"
>> into the command name is a bad idea. Also, the commit message should
>> briefly note the restriction to KVM.
Perhaps this one is closer to reality.
>> Pick one :)
>>
>> [...]
On Thu, Jun 17, 2021 at 04:14:17PM +0200, Markus Armbruster wrote:
> Claudio Fontana <cfontana@suse.de> writes:
>
> > On 6/17/21 1:09 PM, Markus Armbruster wrote:
> >> Valeriy Vdovin <valeriy.vdovin@virtuozzo.com> writes:
> >>
> >>> On Thu, Jun 17, 2021 at 07:22:36AM +0200, Markus Armbruster wrote:
> >>>> Valeriy Vdovin <valeriy.vdovin@virtuozzo.com> writes:
> >>>>
> >>>>> Introducing new qapi method 'query-kvm-cpuid'. This method can be used to
> >>>>
> >>>> It's actually a QMP command. There are no "qapi methods".
> >>>>
> >>>>> get virtualized cpu model info generated by QEMU during VM initialization in
> >>>>> the form of cpuid representation.
> >>>>>
> >>>>> Diving into more details about virtual cpu generation: QEMU first parses '-cpu'
> >>>>
> >>>> virtual CPU
> >>>>
> >>>>> command line option. From there it takes the name of the model as the basis for
> >>>>> feature set of the new virtual cpu. After that it uses trailing '-cpu' options,
> >>>>> that state if additional cpu features should be present on the virtual cpu or
> >>>>> excluded from it (tokens '+'/'-' or '=on'/'=off').
> >>>>> After that QEMU checks if the host's cpu can actually support the derived
> >>>>> feature set and applies host limitations to it.
> >>>>> After this initialization procedure, virtual cpu has it's model and
> >>>>> vendor names, and a working feature set and is ready for identification
> >>>>> instructions such as CPUID.
> >>>>>
> >>>>> Currently full output for this method is only supported for x86 cpus.
> >>>>
> >>>> Not sure about "currently": the interface looks quite x86-specific to me.
> >>>>
> >>> Yes, at some point I was thinking this interface could become generic,
> >>> but does not seem possible, so I'll remove this note.
> >>>
> >>>> The commit message doesn't mention KVM except in the command name. The
> >>>> schema provides the command only if defined(CONFIG_KVM).
> >>>>
> >>>> Can you explain why you need the restriction to CONFIG_KVM?
> >>>>
> >>> This CONFIG_KVM is used as a solution to a broken build if --disable-kvm
> >>> flag is set. I was choosing between this and writing empty implementation into
> >>> kvm-stub.c
> >>
> >> If the command only makes sense for KVM, then it's named correctly, but
> >> the commit message lacks a (brief!) explanation why it only makes for
> >> KVM.
> >
> >
> > Is it meaningful for HVF?
>
> I can't see why it couldn't be.
Should I also make some note about that in the commit message?
>
> Different tack: if KVM is compiled out entirely, the command isn't
> there, and trying to use it fails like
>
> {"error": {"class": "CommandNotFound", "desc": "The command query-kvm-cpuid has not been found"}}
>
> If KVM is compiled in, but disabled, e.g. with -machine accel=tcg, then
> the command fails like
>
> {"error": {"class": "GenericError", "desc": "VCPU was not initialized yet"}}
>
> This is misleading. The VCPU is actually running, it's just the wrong
> kind of VCPU.
>
> >> If it just isn't implemented for anything but KVM, then putting "kvm"
> >> into the command name is a bad idea. Also, the commit message should
> >> briefly note the restriction to KVM.
>
> Perhaps this one is closer to reality.
>
I agree.
What command name do you suggest?
> >> Pick one :)
> >>
> >> [...]
>
On 6/17/21 5:39 PM, Valeriy Vdovin wrote:
> On Thu, Jun 17, 2021 at 04:14:17PM +0200, Markus Armbruster wrote:
>> Claudio Fontana <cfontana@suse.de> writes:
>>
>>> On 6/17/21 1:09 PM, Markus Armbruster wrote:
>>>> Valeriy Vdovin <valeriy.vdovin@virtuozzo.com> writes:
>>>>
>>>>> On Thu, Jun 17, 2021 at 07:22:36AM +0200, Markus Armbruster wrote:
>>>>>> Valeriy Vdovin <valeriy.vdovin@virtuozzo.com> writes:
>>>>>>
>>>>>>> Introducing new qapi method 'query-kvm-cpuid'. This method can be used to
>>>>>>
>>>>>> It's actually a QMP command. There are no "qapi methods".
>>>>>>
>>>>>>> get virtualized cpu model info generated by QEMU during VM initialization in
>>>>>>> the form of cpuid representation.
>>>>>>>
>>>>>>> Diving into more details about virtual cpu generation: QEMU first parses '-cpu'
>>>>>>
>>>>>> virtual CPU
>>>>>>
>>>>>>> command line option. From there it takes the name of the model as the basis for
>>>>>>> feature set of the new virtual cpu. After that it uses trailing '-cpu' options,
>>>>>>> that state if additional cpu features should be present on the virtual cpu or
>>>>>>> excluded from it (tokens '+'/'-' or '=on'/'=off').
>>>>>>> After that QEMU checks if the host's cpu can actually support the derived
>>>>>>> feature set and applies host limitations to it.
>>>>>>> After this initialization procedure, virtual cpu has it's model and
>>>>>>> vendor names, and a working feature set and is ready for identification
>>>>>>> instructions such as CPUID.
>>>>>>>
>>>>>>> Currently full output for this method is only supported for x86 cpus.
>>>>>>
>>>>>> Not sure about "currently": the interface looks quite x86-specific to me.
>>>>>>
>>>>> Yes, at some point I was thinking this interface could become generic,
>>>>> but does not seem possible, so I'll remove this note.
>>>>>
>>>>>> The commit message doesn't mention KVM except in the command name. The
>>>>>> schema provides the command only if defined(CONFIG_KVM).
>>>>>>
>>>>>> Can you explain why you need the restriction to CONFIG_KVM?
>>>>>>
>>>>> This CONFIG_KVM is used as a solution to a broken build if --disable-kvm
>>>>> flag is set. I was choosing between this and writing empty implementation into
>>>>> kvm-stub.c
>>>>
>>>> If the command only makes sense for KVM, then it's named correctly, but
>>>> the commit message lacks a (brief!) explanation why it only makes for
>>>> KVM.
>>>
>>>
>>> Is it meaningful for HVF?
>>
>> I can't see why it couldn't be.
> Should I also make some note about that in the commit message?
>>
>> Different tack: if KVM is compiled out entirely, the command isn't
>> there, and trying to use it fails like
>>
>> {"error": {"class": "CommandNotFound", "desc": "The command query-kvm-cpuid has not been found"}}
>>
>> If KVM is compiled in, but disabled, e.g. with -machine accel=tcg, then
>> the command fails like
>>
>> {"error": {"class": "GenericError", "desc": "VCPU was not initialized yet"}}
>>
>> This is misleading. The VCPU is actually running, it's just the wrong
>> kind of VCPU.
>>
>>>> If it just isn't implemented for anything but KVM, then putting "kvm"
>>>> into the command name is a bad idea. Also, the commit message should
>>>> briefly note the restriction to KVM.
>>
>> Perhaps this one is closer to reality.
>>
> I agree.
> What command name do you suggest?
query-exposed-cpuid?
>>>> Pick one :)
>>>>
>>>> [...]
>>
On Thu, Jun 17, 2021 at 05:53:11PM +0200, Claudio Fontana wrote:
> On 6/17/21 5:39 PM, Valeriy Vdovin wrote:
> > On Thu, Jun 17, 2021 at 04:14:17PM +0200, Markus Armbruster wrote:
> >> Claudio Fontana <cfontana@suse.de> writes:
> >>
> >>> On 6/17/21 1:09 PM, Markus Armbruster wrote:
> >>>> Valeriy Vdovin <valeriy.vdovin@virtuozzo.com> writes:
> >>>>
> >>>>> On Thu, Jun 17, 2021 at 07:22:36AM +0200, Markus Armbruster wrote:
> >>>>>> Valeriy Vdovin <valeriy.vdovin@virtuozzo.com> writes:
> >>>>>>
> >>>>>>> Introducing new qapi method 'query-kvm-cpuid'. This method can be used to
> >>>>>>
> >>>>>> It's actually a QMP command. There are no "qapi methods".
> >>>>>>
> >>>>>>> get virtualized cpu model info generated by QEMU during VM initialization in
> >>>>>>> the form of cpuid representation.
> >>>>>>>
> >>>>>>> Diving into more details about virtual cpu generation: QEMU first parses '-cpu'
> >>>>>>
> >>>>>> virtual CPU
> >>>>>>
> >>>>>>> command line option. From there it takes the name of the model as the basis for
> >>>>>>> feature set of the new virtual cpu. After that it uses trailing '-cpu' options,
> >>>>>>> that state if additional cpu features should be present on the virtual cpu or
> >>>>>>> excluded from it (tokens '+'/'-' or '=on'/'=off').
> >>>>>>> After that QEMU checks if the host's cpu can actually support the derived
> >>>>>>> feature set and applies host limitations to it.
> >>>>>>> After this initialization procedure, virtual cpu has it's model and
> >>>>>>> vendor names, and a working feature set and is ready for identification
> >>>>>>> instructions such as CPUID.
> >>>>>>>
> >>>>>>> Currently full output for this method is only supported for x86 cpus.
> >>>>>>
> >>>>>> Not sure about "currently": the interface looks quite x86-specific to me.
> >>>>>>
> >>>>> Yes, at some point I was thinking this interface could become generic,
> >>>>> but does not seem possible, so I'll remove this note.
> >>>>>
> >>>>>> The commit message doesn't mention KVM except in the command name. The
> >>>>>> schema provides the command only if defined(CONFIG_KVM).
> >>>>>>
> >>>>>> Can you explain why you need the restriction to CONFIG_KVM?
> >>>>>>
> >>>>> This CONFIG_KVM is used as a solution to a broken build if --disable-kvm
> >>>>> flag is set. I was choosing between this and writing empty implementation into
> >>>>> kvm-stub.c
> >>>>
> >>>> If the command only makes sense for KVM, then it's named correctly, but
> >>>> the commit message lacks a (brief!) explanation why it only makes for
> >>>> KVM.
> >>>
> >>>
> >>> Is it meaningful for HVF?
> >>
> >> I can't see why it couldn't be.
> > Should I also make some note about that in the commit message?
> >>
> >> Different tack: if KVM is compiled out entirely, the command isn't
> >> there, and trying to use it fails like
> >>
> >> {"error": {"class": "CommandNotFound", "desc": "The command query-kvm-cpuid has not been found"}}
> >>
> >> If KVM is compiled in, but disabled, e.g. with -machine accel=tcg, then
> >> the command fails like
> >>
> >> {"error": {"class": "GenericError", "desc": "VCPU was not initialized yet"}}
> >>
> >> This is misleading. The VCPU is actually running, it's just the wrong
> >> kind of VCPU.
> >>
> >>>> If it just isn't implemented for anything but KVM, then putting "kvm"
> >>>> into the command name is a bad idea. Also, the commit message should
> >>>> briefly note the restriction to KVM.
> >>
> >> Perhaps this one is closer to reality.
> >>
> > I agree.
> > What command name do you suggest?
>
> query-exposed-cpuid?
Pasting the reply I sent at [1]:
I don't really mind how the command is called, but I would prefer
to add a more complex abstraction only if maintainers of other
accelerators are interested and volunteer to provide similar
functionality. I don't want to introduce complexity for use
cases that may not even exist.
I'm expecting this to be just a debugging mechanism, not a stable
API to be maintained and supported for decades. (Maybe a "x-"
prefix should be added to indicate that?)
[1] https://lore.kernel.org/qemu-devel/20210602204604.crsxvqixkkll4ef4@habkost.net
--
Eduardo
Eduardo Habkost <ehabkost@redhat.com> writes: > On Thu, Jun 17, 2021 at 05:53:11PM +0200, Claudio Fontana wrote: >> On 6/17/21 5:39 PM, Valeriy Vdovin wrote: >> > On Thu, Jun 17, 2021 at 04:14:17PM +0200, Markus Armbruster wrote: >> >> Claudio Fontana <cfontana@suse.de> writes: >> >> >> >>> On 6/17/21 1:09 PM, Markus Armbruster wrote: [...] >> >>>> If it just isn't implemented for anything but KVM, then putting "kvm" >> >>>> into the command name is a bad idea. Also, the commit message should >> >>>> briefly note the restriction to KVM. >> >> >> >> Perhaps this one is closer to reality. >> >> >> > I agree. >> > What command name do you suggest? >> >> query-exposed-cpuid? > > Pasting the reply I sent at [1]: > > I don't really mind how the command is called, but I would prefer > to add a more complex abstraction only if maintainers of other > accelerators are interested and volunteer to provide similar > functionality. I don't want to introduce complexity for use > cases that may not even exist. > > I'm expecting this to be just a debugging mechanism, not a stable > API to be maintained and supported for decades. (Maybe a "x-" > prefix should be added to indicate that?) > > [1] https://lore.kernel.org/qemu-devel/20210602204604.crsxvqixkkll4ef4@habkost.net x-query-x86_64-cpuid?
On Fri, Jun 18, 2021 at 07:52:47AM +0200, Markus Armbruster wrote: > Eduardo Habkost <ehabkost@redhat.com> writes: > > > On Thu, Jun 17, 2021 at 05:53:11PM +0200, Claudio Fontana wrote: > >> On 6/17/21 5:39 PM, Valeriy Vdovin wrote: > >> > On Thu, Jun 17, 2021 at 04:14:17PM +0200, Markus Armbruster wrote: > >> >> Claudio Fontana <cfontana@suse.de> writes: > >> >> > >> >>> On 6/17/21 1:09 PM, Markus Armbruster wrote: > > [...] > > >> >>>> If it just isn't implemented for anything but KVM, then putting "kvm" > >> >>>> into the command name is a bad idea. Also, the commit message should > >> >>>> briefly note the restriction to KVM. > >> >> > >> >> Perhaps this one is closer to reality. > >> >> > >> > I agree. > >> > What command name do you suggest? > >> > >> query-exposed-cpuid? > > > > Pasting the reply I sent at [1]: > > > > I don't really mind how the command is called, but I would prefer > > to add a more complex abstraction only if maintainers of other > > accelerators are interested and volunteer to provide similar > > functionality. I don't want to introduce complexity for use > > cases that may not even exist. > > > > I'm expecting this to be just a debugging mechanism, not a stable > > API to be maintained and supported for decades. (Maybe a "x-" > > prefix should be added to indicate that?) > > > > [1] https://lore.kernel.org/qemu-devel/20210602204604.crsxvqixkkll4ef4@habkost.net > > x-query-x86_64-cpuid? > Unless somebody wants to spend time designing a generic abstraction around this (and justify the extra complexity), this is a KVM-specific command. Is there a reason to avoid "kvm" in the command name? -- Eduardo
Eduardo Habkost <ehabkost@redhat.com> writes: > On Fri, Jun 18, 2021 at 07:52:47AM +0200, Markus Armbruster wrote: >> Eduardo Habkost <ehabkost@redhat.com> writes: >> >> > On Thu, Jun 17, 2021 at 05:53:11PM +0200, Claudio Fontana wrote: >> >> On 6/17/21 5:39 PM, Valeriy Vdovin wrote: >> >> > On Thu, Jun 17, 2021 at 04:14:17PM +0200, Markus Armbruster wrote: >> >> >> Claudio Fontana <cfontana@suse.de> writes: >> >> >> >> >> >>> On 6/17/21 1:09 PM, Markus Armbruster wrote: >> >> [...] >> >> >> >>>> If it just isn't implemented for anything but KVM, then putting "kvm" >> >> >>>> into the command name is a bad idea. Also, the commit message should >> >> >>>> briefly note the restriction to KVM. >> >> >> >> >> >> Perhaps this one is closer to reality. >> >> >> >> >> > I agree. >> >> > What command name do you suggest? >> >> >> >> query-exposed-cpuid? >> > >> > Pasting the reply I sent at [1]: >> > >> > I don't really mind how the command is called, but I would prefer >> > to add a more complex abstraction only if maintainers of other >> > accelerators are interested and volunteer to provide similar >> > functionality. I don't want to introduce complexity for use >> > cases that may not even exist. >> > >> > I'm expecting this to be just a debugging mechanism, not a stable >> > API to be maintained and supported for decades. (Maybe a "x-" >> > prefix should be added to indicate that?) >> > >> > [1] https://lore.kernel.org/qemu-devel/20210602204604.crsxvqixkkll4ef4@habkost.net >> >> x-query-x86_64-cpuid? >> > > Unless somebody wants to spend time designing a generic > abstraction around this (and justify the extra complexity), this > is a KVM-specific command. Is there a reason to avoid "kvm" in > the command name? I can't see what's specific to KVM in the interface (it's implemented only for KVM, but that's just a restriction). The doc comment looks like the command returns whatever the guest's cpuid instruction will write to registers. Can you help me understand the interface's KVM dependence?
On 6/18/21 10:40 PM, Eduardo Habkost wrote: > On Fri, Jun 18, 2021 at 07:52:47AM +0200, Markus Armbruster wrote: >> Eduardo Habkost <ehabkost@redhat.com> writes: >> >>> On Thu, Jun 17, 2021 at 05:53:11PM +0200, Claudio Fontana wrote: >>>> On 6/17/21 5:39 PM, Valeriy Vdovin wrote: >>>>> On Thu, Jun 17, 2021 at 04:14:17PM +0200, Markus Armbruster wrote: >>>>>> Claudio Fontana <cfontana@suse.de> writes: >>>>>> >>>>>>> On 6/17/21 1:09 PM, Markus Armbruster wrote: >> >> [...] >> >>>>>>>> If it just isn't implemented for anything but KVM, then putting "kvm" >>>>>>>> into the command name is a bad idea. Also, the commit message should >>>>>>>> briefly note the restriction to KVM. >>>>>> >>>>>> Perhaps this one is closer to reality. >>>>>> >>>>> I agree. >>>>> What command name do you suggest? >>>> >>>> query-exposed-cpuid? >>> >>> Pasting the reply I sent at [1]: >>> >>> I don't really mind how the command is called, but I would prefer >>> to add a more complex abstraction only if maintainers of other >>> accelerators are interested and volunteer to provide similar >>> functionality. I don't want to introduce complexity for use >>> cases that may not even exist. >>> >>> I'm expecting this to be just a debugging mechanism, not a stable >>> API to be maintained and supported for decades. (Maybe a "x-" >>> prefix should be added to indicate that?) >>> >>> [1] https://lore.kernel.org/qemu-devel/20210602204604.crsxvqixkkll4ef4@habkost.net >> >> x-query-x86_64-cpuid? >> > > Unless somebody wants to spend time designing a generic > abstraction around this (and justify the extra complexity), this > is a KVM-specific command. Is there a reason to avoid "kvm" in > the command name? > If the point of all of this is "please get me the cpuid, as seen by the guest", then I fail to see how this should be kvm-only. We can still return "not implemented" of some kind for HVF, TCG etc. But maybe I misread the use case? Thanks, C
On Mon, Jun 21, 2021 at 10:07:44AM +0200, Claudio Fontana wrote: > On 6/18/21 10:40 PM, Eduardo Habkost wrote: > > On Fri, Jun 18, 2021 at 07:52:47AM +0200, Markus Armbruster wrote: > >> Eduardo Habkost <ehabkost@redhat.com> writes: > >> > >>> On Thu, Jun 17, 2021 at 05:53:11PM +0200, Claudio Fontana wrote: > >>>> On 6/17/21 5:39 PM, Valeriy Vdovin wrote: > >>>>> On Thu, Jun 17, 2021 at 04:14:17PM +0200, Markus Armbruster wrote: > >>>>>> Claudio Fontana <cfontana@suse.de> writes: > >>>>>> > >>>>>>> On 6/17/21 1:09 PM, Markus Armbruster wrote: > >> > >> [...] > >> > >>>>>>>> If it just isn't implemented for anything but KVM, then putting "kvm" > >>>>>>>> into the command name is a bad idea. Also, the commit message should > >>>>>>>> briefly note the restriction to KVM. > >>>>>> > >>>>>> Perhaps this one is closer to reality. > >>>>>> > >>>>> I agree. > >>>>> What command name do you suggest? > >>>> > >>>> query-exposed-cpuid? > >>> > >>> Pasting the reply I sent at [1]: > >>> > >>> I don't really mind how the command is called, but I would prefer > >>> to add a more complex abstraction only if maintainers of other > >>> accelerators are interested and volunteer to provide similar > >>> functionality. I don't want to introduce complexity for use > >>> cases that may not even exist. > >>> > >>> I'm expecting this to be just a debugging mechanism, not a stable > >>> API to be maintained and supported for decades. (Maybe a "x-" > >>> prefix should be added to indicate that?) > >>> > >>> [1] https://lore.kernel.org/qemu-devel/20210602204604.crsxvqixkkll4ef4@habkost.net > >> > >> x-query-x86_64-cpuid? > >> > > > > Unless somebody wants to spend time designing a generic > > abstraction around this (and justify the extra complexity), this > > is a KVM-specific command. Is there a reason to avoid "kvm" in > > the command name? > > > > If the point of all of this is "please get me the cpuid, as seen by the guest", then I fail to see how this should be kvm-only. > We can still return "not implemented" of some kind for HVF, TCG etc. > > But maybe I misread the use case? A generic interface would require additional glue to connect the generic code to the accel-specific implementation. I'm trying to avoid wasting everybody's time with the extra complexity unless necessary. But if you all believe the extra complexity is worth it, I won't object. -- Eduardo
Eduardo Habkost <ehabkost@redhat.com> writes:
> On Mon, Jun 21, 2021 at 10:07:44AM +0200, Claudio Fontana wrote:
>> On 6/18/21 10:40 PM, Eduardo Habkost wrote:
>> > On Fri, Jun 18, 2021 at 07:52:47AM +0200, Markus Armbruster wrote:
>> >> Eduardo Habkost <ehabkost@redhat.com> writes:
>> >>
>> >>> On Thu, Jun 17, 2021 at 05:53:11PM +0200, Claudio Fontana wrote:
>> >>>> On 6/17/21 5:39 PM, Valeriy Vdovin wrote:
>> >>>>> On Thu, Jun 17, 2021 at 04:14:17PM +0200, Markus Armbruster wrote:
>> >>>>>> Claudio Fontana <cfontana@suse.de> writes:
>> >>>>>>
>> >>>>>>> On 6/17/21 1:09 PM, Markus Armbruster wrote:
>> >>
>> >> [...]
>> >>
>> >>>>>>>> If it just isn't implemented for anything but KVM, then putting "kvm"
>> >>>>>>>> into the command name is a bad idea. Also, the commit message should
>> >>>>>>>> briefly note the restriction to KVM.
>> >>>>>>
>> >>>>>> Perhaps this one is closer to reality.
>> >>>>>>
>> >>>>> I agree.
>> >>>>> What command name do you suggest?
>> >>>>
>> >>>> query-exposed-cpuid?
>> >>>
>> >>> Pasting the reply I sent at [1]:
>> >>>
>> >>> I don't really mind how the command is called, but I would prefer
>> >>> to add a more complex abstraction only if maintainers of other
>> >>> accelerators are interested and volunteer to provide similar
>> >>> functionality. I don't want to introduce complexity for use
>> >>> cases that may not even exist.
>> >>>
>> >>> I'm expecting this to be just a debugging mechanism, not a stable
>> >>> API to be maintained and supported for decades. (Maybe a "x-"
>> >>> prefix should be added to indicate that?)
>> >>>
>> >>> [1] https://lore.kernel.org/qemu-devel/20210602204604.crsxvqixkkll4ef4@habkost.net
>> >>
>> >> x-query-x86_64-cpuid?
>> >>
>> >
>> > Unless somebody wants to spend time designing a generic
>> > abstraction around this (and justify the extra complexity), this
>> > is a KVM-specific command. Is there a reason to avoid "kvm" in
>> > the command name?
>> >
>>
>> If the point of all of this is "please get me the cpuid, as seen by the guest", then I fail to see how this should be kvm-only.
>> We can still return "not implemented" of some kind for HVF, TCG etc.
>>
>> But maybe I misread the use case?
>
> A generic interface would require additional glue to connect the
> generic code to the accel-specific implementation. I'm trying to
> avoid wasting everybody's time with the extra complexity unless
> necessary.
If I read the patch correctly, the *interface* is specific to x86_64,
but not to any accelerator. It's *implemented* only for KVM, though.
Is that correct?
> But if you all believe the extra complexity is worth it, I won't
> object.
I'm not arguing for a complete implementation now.
I think the command name is a matter of taste.
The command exists only if defined(TARGET_I386). Putting -x86_64- or
similar in the name isn't strictly required, but why not. Maybe just
-x86-.
Putting -kvm- in the name signals (1) the command works only with KVM,
and (2) we don't intend to make it work with other accelerators. If we
later decide to make it work with other accelerators, we get to rename
the command.
Not putting -kvm- in the name doesn't commit us to anything.
Either way, the command exists and fails when defined(CONFIG_KVM) and
accel!=kvm.
Aside: I think having the command depend on defined(CONFIG_KVM)
accomplishes nothing but enlarging the test matrix:
defined(CONFIG_KVM) and accel=kvm command exists and works
defined(CONFIG_KVM) and accel!=kvm command exists and fails
!defined(CONFIG_KVM) command does not exist
Simpler:
accel=kvm command exists and works
accel!=kvm command exists and fails
On Mon, Jun 21, 2021 at 05:50:55PM +0200, Markus Armbruster wrote: > Eduardo Habkost <ehabkost@redhat.com> writes: > > > On Mon, Jun 21, 2021 at 10:07:44AM +0200, Claudio Fontana wrote: > >> On 6/18/21 10:40 PM, Eduardo Habkost wrote: > >> > On Fri, Jun 18, 2021 at 07:52:47AM +0200, Markus Armbruster wrote: > >> >> Eduardo Habkost <ehabkost@redhat.com> writes: > >> >> > >> >>> On Thu, Jun 17, 2021 at 05:53:11PM +0200, Claudio Fontana wrote: > >> >>>> On 6/17/21 5:39 PM, Valeriy Vdovin wrote: > >> >>>>> On Thu, Jun 17, 2021 at 04:14:17PM +0200, Markus Armbruster wrote: > >> >>>>>> Claudio Fontana <cfontana@suse.de> writes: > >> >>>>>> > >> >>>>>>> On 6/17/21 1:09 PM, Markus Armbruster wrote: > >> >> > >> >> [...] > >> >> > >> >>>>>>>> If it just isn't implemented for anything but KVM, then putting "kvm" > >> >>>>>>>> into the command name is a bad idea. Also, the commit message should > >> >>>>>>>> briefly note the restriction to KVM. > >> >>>>>> > >> >>>>>> Perhaps this one is closer to reality. > >> >>>>>> > >> >>>>> I agree. > >> >>>>> What command name do you suggest? > >> >>>> > >> >>>> query-exposed-cpuid? > >> >>> > >> >>> Pasting the reply I sent at [1]: > >> >>> > >> >>> I don't really mind how the command is called, but I would prefer > >> >>> to add a more complex abstraction only if maintainers of other > >> >>> accelerators are interested and volunteer to provide similar > >> >>> functionality. I don't want to introduce complexity for use > >> >>> cases that may not even exist. > >> >>> > >> >>> I'm expecting this to be just a debugging mechanism, not a stable > >> >>> API to be maintained and supported for decades. (Maybe a "x-" > >> >>> prefix should be added to indicate that?) > >> >>> > >> >>> [1] https://lore.kernel.org/qemu-devel/20210602204604.crsxvqixkkll4ef4@habkost.net > >> >> > >> >> x-query-x86_64-cpuid? > >> >> > >> > > >> > Unless somebody wants to spend time designing a generic > >> > abstraction around this (and justify the extra complexity), this > >> > is a KVM-specific command. Is there a reason to avoid "kvm" in > >> > the command name? > >> > > >> > >> If the point of all of this is "please get me the cpuid, as seen by the guest", then I fail to see how this should be kvm-only. > >> We can still return "not implemented" of some kind for HVF, TCG etc. > >> > >> But maybe I misread the use case? > > > > A generic interface would require additional glue to connect the > > generic code to the accel-specific implementation. I'm trying to > > avoid wasting everybody's time with the extra complexity unless > > necessary. > > If I read the patch correctly, the *interface* is specific to x86_64, > but not to any accelerator. It's *implemented* only for KVM, though. > Is that correct? > Yes, it's a x86 specific instruction, and KVM is a bit of implementation detail right now. It could actually have stubs in other accels instead of CONFIG_KVM. > > But if you all believe the extra complexity is worth it, I won't > > object. > > I'm not arguing for a complete implementation now. > > I think the command name is a matter of taste. > > The command exists only if defined(TARGET_I386). Putting -x86_64- or > similar in the name isn't strictly required, but why not. Maybe just > -x86-. > > Putting -kvm- in the name signals (1) the command works only with KVM, > and (2) we don't intend to make it work with other accelerators. If we > later decide to make it work with other accelerators, we get to rename > the command. > > Not putting -kvm- in the name doesn't commit us to anything. > > Either way, the command exists and fails when defined(CONFIG_KVM) and > accel!=kvm. > > Aside: I think having the command depend on defined(CONFIG_KVM) > accomplishes nothing but enlarging the test matrix: > > defined(CONFIG_KVM) and accel=kvm command exists and works > defined(CONFIG_KVM) and accel!=kvm command exists and fails > !defined(CONFIG_KVM) command does not exist > > Simpler: > > accel=kvm command exists and works > accel!=kvm command exists and fails > Well, it accomplishes not having stub implementations all over the place. But looks like having the right error message in stubs really seems more appropriate. Your reasoning is pretty clear and in the light of it I now fill that platform in the name is better that one of the possible accel implementations in the name. So should the command name be renamed from 'query-kvm-cpuid' to 'query-x86-cpuid'? And considering CONFIG_KVM, I guess it would be better to drop this ifdef and instead put stub functions in several places? If yes, please let me know the exact list of places that should have that stub, as well as the right way to state the "unimplemented" error for these stubs, (sorry, this last one is just to shorten some of the iterations) Thanks.
On Mon, Jun 21, 2021 at 07:09:51PM +0300, Valeriy Vdovin wrote: > On Mon, Jun 21, 2021 at 05:50:55PM +0200, Markus Armbruster wrote: > > Eduardo Habkost <ehabkost@redhat.com> writes: > > > > > On Mon, Jun 21, 2021 at 10:07:44AM +0200, Claudio Fontana wrote: > > >> On 6/18/21 10:40 PM, Eduardo Habkost wrote: > > >> > On Fri, Jun 18, 2021 at 07:52:47AM +0200, Markus Armbruster wrote: > > >> >> Eduardo Habkost <ehabkost@redhat.com> writes: > > >> >> > > >> >>> On Thu, Jun 17, 2021 at 05:53:11PM +0200, Claudio Fontana wrote: > > >> >>>> On 6/17/21 5:39 PM, Valeriy Vdovin wrote: > > >> >>>>> On Thu, Jun 17, 2021 at 04:14:17PM +0200, Markus Armbruster wrote: > > >> >>>>>> Claudio Fontana <cfontana@suse.de> writes: > > >> >>>>>> > > >> >>>>>>> On 6/17/21 1:09 PM, Markus Armbruster wrote: > > >> >> > > >> >> [...] > > >> >> > > >> >>>>>>>> If it just isn't implemented for anything but KVM, then putting "kvm" > > >> >>>>>>>> into the command name is a bad idea. Also, the commit message should > > >> >>>>>>>> briefly note the restriction to KVM. > > >> >>>>>> > > >> >>>>>> Perhaps this one is closer to reality. > > >> >>>>>> > > >> >>>>> I agree. > > >> >>>>> What command name do you suggest? > > >> >>>> > > >> >>>> query-exposed-cpuid? > > >> >>> > > >> >>> Pasting the reply I sent at [1]: > > >> >>> > > >> >>> I don't really mind how the command is called, but I would prefer > > >> >>> to add a more complex abstraction only if maintainers of other > > >> >>> accelerators are interested and volunteer to provide similar > > >> >>> functionality. I don't want to introduce complexity for use > > >> >>> cases that may not even exist. > > >> >>> > > >> >>> I'm expecting this to be just a debugging mechanism, not a stable > > >> >>> API to be maintained and supported for decades. (Maybe a "x-" > > >> >>> prefix should be added to indicate that?) > > >> >>> > > >> >>> [1] https://lore.kernel.org/qemu-devel/20210602204604.crsxvqixkkll4ef4@habkost.net > > >> >> > > >> >> x-query-x86_64-cpuid? > > >> >> > > >> > > > >> > Unless somebody wants to spend time designing a generic > > >> > abstraction around this (and justify the extra complexity), this > > >> > is a KVM-specific command. Is there a reason to avoid "kvm" in > > >> > the command name? > > >> > > > >> > > >> If the point of all of this is "please get me the cpuid, as seen by the guest", then I fail to see how this should be kvm-only. > > >> We can still return "not implemented" of some kind for HVF, TCG etc. > > >> > > >> But maybe I misread the use case? > > > > > > A generic interface would require additional glue to connect the > > > generic code to the accel-specific implementation. I'm trying to > > > avoid wasting everybody's time with the extra complexity unless > > > necessary. > > > > If I read the patch correctly, the *interface* is specific to x86_64, > > but not to any accelerator. It's *implemented* only for KVM, though. > > Is that correct? > > > Yes, it's a x86 specific instruction, and KVM is a bit of implementation > detail right now. It could actually have stubs in other accels instead > of CONFIG_KVM. > > > > But if you all believe the extra complexity is worth it, I won't > > > object. > > > > I'm not arguing for a complete implementation now. > > > > I think the command name is a matter of taste. > > > > The command exists only if defined(TARGET_I386). Putting -x86_64- or > > similar in the name isn't strictly required, but why not. Maybe just > > -x86-. > > > > Putting -kvm- in the name signals (1) the command works only with KVM, > > and (2) we don't intend to make it work with other accelerators. If we > > later decide to make it work with other accelerators, we get to rename > > the command. > > > > Not putting -kvm- in the name doesn't commit us to anything. > > > > Either way, the command exists and fails when defined(CONFIG_KVM) and > > accel!=kvm. > > > > Aside: I think having the command depend on defined(CONFIG_KVM) > > accomplishes nothing but enlarging the test matrix: > > > > defined(CONFIG_KVM) and accel=kvm command exists and works > > defined(CONFIG_KVM) and accel!=kvm command exists and fails > > !defined(CONFIG_KVM) command does not exist > > > > Simpler: > > > > accel=kvm command exists and works > > accel!=kvm command exists and fails > > > Well, it accomplishes not having stub implementations all over the place. > But looks like having the right error message in stubs really seems more > appropriate. > > Your reasoning is pretty clear and in the light of it I now fill that > platform in the name is better that one of the possible accel implementations > in the name. > > So should the command name be renamed from 'query-kvm-cpuid' to > 'query-x86-cpuid'? > > And considering CONFIG_KVM, I guess it would be better to drop this ifdef and > instead put stub functions in several places? If yes, please let me know > the exact list of places that should have that stub, as well as the right way > to state the "unimplemented" error for these stubs, (sorry, this last > one is just to shorten some of the iterations) > > Thanks. Hello. I'm waiting some kind of response/confirmation on this message, so I could continue.
© 2016 - 2024 Red Hat, Inc.