[PATCH v7 0/1] qapi: introduce 'query-cpu-model-cpuid' action

[PATCH v7 0/1] qapi: introduce 'query-cpu-model-cpuid' action

[Valeriy Vdovin]

In version 7 of the subject patch, I wanted to try to implement this qmp
method with the help of existing kvm ioctl KVM_GET_CPUID2. The reasoning
behind my decision is the fact that QEMU uses KVM_SET_CPUID2 to set full
cpuid response table to the virtual cpu, and KVM_GET_CPUID2 is a convenient
and complementary method for extracting the same cpuid entry table back
from the kernel.

So I've pushed a patch into Linux KVM subtree to fine tune ioctl
KVM_GET_CPUID2 for this type of application. This has triggered a small
discussion.
One of the responses here https://lkml.org/lkml/2021/5/4/109 is from
Paolo Bonzini. There he suggests to cache the the whole cpuid entries
table that we pass to the kernel via KVM_SET_CPUID2 and later just output
this cached data instead of calling the same table via KVM_GET_CPUID2.
Current patch is the reflection of that idea.

Valeriy Vdovin (1):
  qapi: introduce 'query-cpu-model-cpuid' action

 qapi/machine-target.json   | 51 ++++++++++++++++++++++++++++++++++++++
 target/i386/kvm/kvm.c      | 45 ++++++++++++++++++++++++++++++---
 tests/qtest/qmp-cmd-test.c |  1 +
 3 files changed, 93 insertions(+), 4 deletions(-)

-- 
2.17.1

[PATCH v7 1/1] qapi: introduce 'query-cpu-model-cpuid' action

[Valeriy Vdovin]

Introducing new qapi method 'query-cpu-model-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-cpu-model-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-cpu-model-cpuid"
}

qmp_response: [
  {
    "eax": 1073741825,
    "edx": 77,
    "leaf": 1073741824,
    "ecx": 1447775574,
    "ebx": 1263359563,
    "subleaf": 0
  },
  {
    "eax": 16777339,
    "edx": 0,
    "leaf": 1073741825,
    "ecx": 0,
    "ebx": 0,
    "subleaf": 0
  },
  {
    "eax": 13,
    "edx": 1231384169,
    "leaf": 0,
    "ecx": 1818588270,
    "ebx": 1970169159,
    "subleaf": 0
  },
  {
    "eax": 198354,
    "edx": 126614527,
  ....

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
 qapi/machine-target.json   | 51 ++++++++++++++++++++++++++++++++++++++
 target/i386/kvm/kvm.c      | 45 ++++++++++++++++++++++++++++++---
 tests/qtest/qmp-cmd-test.c |  1 +
 3 files changed, 93 insertions(+), 4 deletions(-)

diff --git a/qapi/machine-target.json b/qapi/machine-target.json
index e7811654b7..ad816a50b6 100644
--- a/qapi/machine-target.json
+++ b/qapi/machine-target.json
@@ -329,3 +329,54 @@
 ##
 { '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.
+#
+# CPUID instruction accepts 'leaf' argument passed in EAX register.
+# A 'leaf' is a single group of information about the CPU, that is returned
+# to the caller in EAX, EBX, ECX and EDX registers. A few of the leaves will
+# also have 'subleaves', the group of information would partially depend on the
+# value passed in the ECX register. The value of ECX is reflected in the 'subleaf'
+# field of this structure.
+#
+# @leaf: CPUID leaf or the value of EAX prior to CPUID execution.
+# @subleaf: value of ECX for leaf that has varying output depending on ECX.
+# @eax: eax
+# @ebx: ebx
+# @ecx: ecx
+# @edx: edx
+#
+# Since: 6.1
+##
+{ 'struct': 'CpuidEntry',
+  'data': { 'leaf' : 'uint32',
+            'subleaf' : 'uint32',
+            'eax' : 'uint32',
+            'ebx' : 'uint32',
+            'ecx' : 'uint32',
+            'edx' : 'uint32'
+          },
+  'if': 'defined(TARGET_I386)' }
+
+##
+# @query-cpu-model-cpuid:
+#
+# Returns description of a virtual CPU model, created by QEMU after cpu
+# initialization routines. The resulting information is a reflection of a parsed
+# '-cpu' command line option, filtered by available host cpu features.
+#
+# Returns:  @CpuModelCpuidDescription
+#
+# Example:
+#
+# -> { "execute": "query-cpu-model-cpuid" }
+# <- { "return": 'CpuModelCpuidDescription' }
+#
+# Since: 6.1
+##
+{ 'command': 'query-cpu-model-cpuid',
+  'returns': ['CpuidEntry'],
+  'if': 'defined(TARGET_I386)' }
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 7fe9f52710..edc4262efb 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,16 +1465,48 @@ static Error *invtsc_mig_blocker;
 
 #define KVM_MAX_CPUID_ENTRIES  100
 
+struct CPUIDEntriesInfo {
+    struct kvm_cpuid2 cpuid;
+    struct kvm_cpuid_entry2 entries[KVM_MAX_CPUID_ENTRIES];
+};
+
+struct CPUIDEntriesInfo *cpuid_data_cached;
+
+CpuidEntryList *
+qmp_query_cpu_model_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, "cpuid_data cache not ready");
+        return NULL;
+    }
+
+    for (i = 0; i < cpuid_data_cached->cpuid.nent; ++i) {
+        kvm_entry = &cpuid_data_cached->entries[i];
+        entry = g_malloc0(sizeof(*entry));
+        entry->leaf = kvm_entry->function;
+        entry->subleaf = kvm_entry->index;
+        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 {
-        struct kvm_cpuid2 cpuid;
-        struct kvm_cpuid_entry2 entries[KVM_MAX_CPUID_ENTRIES];
-    } cpuid_data;
     /*
      * The kernel defines these structs with padding fields so there
      * should be no extra padding in our cpuid_data struct.
      */
+    struct CPUIDEntriesInfo cpuid_data;
     QEMU_BUILD_BUG_ON(sizeof(cpuid_data) !=
                       sizeof(struct kvm_cpuid2) +
                       sizeof(struct kvm_cpuid_entry2) * KVM_MAX_CPUID_ENTRIES);
@@ -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..f5a926b61b 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-cpu-model-cpuid", ERROR_CLASS_GENERIC_ERROR },
         { NULL, -1 }
     };
     int i;
-- 
2.17.1

Re: [PATCH v7 1/1] qapi: introduce 'query-cpu-model-cpuid' action

[Eduardo Habkost]

On Tue, May 04, 2021 at 03:26:39PM +0300, Valeriy Vdovin wrote:
> Introducing new qapi method 'query-cpu-model-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-cpu-model-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-cpu-model-cpuid"
> }
> 
> qmp_response: [
>   {
>     "eax": 1073741825,
>     "edx": 77,
>     "leaf": 1073741824,
>     "ecx": 1447775574,
>     "ebx": 1263359563,
>     "subleaf": 0
>   },
>   {
>     "eax": 16777339,
>     "edx": 0,
>     "leaf": 1073741825,
>     "ecx": 0,
>     "ebx": 0,
>     "subleaf": 0
>   },
>   {
>     "eax": 13,
>     "edx": 1231384169,
>     "leaf": 0,
>     "ecx": 1818588270,
>     "ebx": 1970169159,
>     "subleaf": 0
>   },
>   {
>     "eax": 198354,
>     "edx": 126614527,
>   ....
> 
> Signed-off-by: Valeriy Vdovin <valeriy.vdovin@virtuozzo.com>

This breaks --disable-kvm builds (see below[1]), but I like the
simplicity of this solution.

I think it will be an acceptable and welcome mechanism if we name
and document it as KVM-specific.

A debugging command like this that returns the raw CPUID data
directly from the KVM tables would be very useful for automated
testing of our KVM CPUID initialization code.  We have some test
cases for CPU configuration code, but they trust what the CPU
objects tell us and won't catch mistakes in target/i386/kvm.c
CPUID code.

[1] Build error when using --disable-kvm:

  [449/821] Linking target qemu-system-x86_64
  FAILED: qemu-system-x86_64
  c++  -o qemu-system-x86_64 qemu-system-x86_64.p/softmmu_main.c.o libcommon.fa.p/hw_char_virtio-console.c.o [...]
  /usr/bin/ld: libqemu-x86_64-softmmu.fa.p/meson-generated_.._qapi_qapi-commands-machine-target.c.o: in function `qmp_marshal_query_cpu_model_cpuid':
  /home/ehabkost/rh/proj/virt/qemu/build/qapi/qapi-commands-machine-target.c:278: undefined reference to `qmp_query_cpu_model_cpuid'
  collect2: error: ld returned 1 exit status


> 
> ---
> 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
>  qapi/machine-target.json   | 51 ++++++++++++++++++++++++++++++++++++++
>  target/i386/kvm/kvm.c      | 45 ++++++++++++++++++++++++++++++---
>  tests/qtest/qmp-cmd-test.c |  1 +
>  3 files changed, 93 insertions(+), 4 deletions(-)
> 
> diff --git a/qapi/machine-target.json b/qapi/machine-target.json
> index e7811654b7..ad816a50b6 100644
> --- a/qapi/machine-target.json
> +++ b/qapi/machine-target.json
> @@ -329,3 +329,54 @@
>  ##
>  { '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.
> +#
> +# CPUID instruction accepts 'leaf' argument passed in EAX register.
> +# A 'leaf' is a single group of information about the CPU, that is returned
> +# to the caller in EAX, EBX, ECX and EDX registers. A few of the leaves will
> +# also have 'subleaves', the group of information would partially depend on the
> +# value passed in the ECX register. The value of ECX is reflected in the 'subleaf'
> +# field of this structure.
> +#
> +# @leaf: CPUID leaf or the value of EAX prior to CPUID execution.
> +# @subleaf: value of ECX for leaf that has varying output depending on ECX.

Instead of having to explain what "leaf" and "subleaf" means,
maybe it would be simpler to just call them "in_eax" and
"in_ecx"?

> +# @eax: eax
> +# @ebx: ebx
> +# @ecx: ecx
> +# @edx: edx
> +#
> +# Since: 6.1
> +##
> +{ 'struct': 'CpuidEntry',
> +  'data': { 'leaf' : 'uint32',
> +            'subleaf' : 'uint32',

I would make subleaf/in_ecx an optional field.  We don't need to
return it unless KVM_CPUID_FLAG_SIGNIFCANT_INDEX is set.

> +            'eax' : 'uint32',
> +            'ebx' : 'uint32',
> +            'ecx' : 'uint32',
> +            'edx' : 'uint32'
> +          },
> +  'if': 'defined(TARGET_I386)' }
> +
> +##
> +# @query-cpu-model-cpuid:

I would choose a name that indicates that the command is
KVM-specific, like "query-kvm-cpuid" or "query-kvm-cpuid-table".

> +#
> +# Returns description of a virtual CPU model, created by QEMU after cpu
> +# initialization routines. The resulting information is a reflection of a parsed
> +# '-cpu' command line option, filtered by available host cpu features.

I don't think "description of a virtual CPU model" is an accurate
description of this.  I would document it as "returns raw data
from the KVM CPUID table for the first VCPU".

I wonder if the "The resulting information is a reflection of a
parsed [...] cpu features." part is really necessary.  If you
believe people don't understand how "-cpu" works, this is not
exactly the right place to explain that.

If you want to clarify what exactly is returned, maybe something
like the following would work?

  "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:  @CpuModelCpuidDescription
> +#
> +# Example:
> +#
> +# -> { "execute": "query-cpu-model-cpuid" }
> +# <- { "return": 'CpuModelCpuidDescription' }
> +#
> +# Since: 6.1
> +##
> +{ 'command': 'query-cpu-model-cpuid',
> +  'returns': ['CpuidEntry'],
> +  'if': 'defined(TARGET_I386)' }
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index 7fe9f52710..edc4262efb 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,16 +1465,48 @@ static Error *invtsc_mig_blocker;
>  
>  #define KVM_MAX_CPUID_ENTRIES  100
>  
> +struct CPUIDEntriesInfo {
> +    struct kvm_cpuid2 cpuid;
> +    struct kvm_cpuid_entry2 entries[KVM_MAX_CPUID_ENTRIES];
> +};

You don't need this new struct definition, as
(&cpuid_data.cpuid.entries[0]) and (&cpuid_data.entries[0]) are
exactly the same.  a kvm_cpuid2 pointer would be enough.

> +
> +struct CPUIDEntriesInfo *cpuid_data_cached;
> +
> +CpuidEntryList *
> +qmp_query_cpu_model_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, "cpuid_data cache not ready");
> +        return NULL;

I would return a more meaningful error message.  Nobody except
the developers who wrote and reviewed this code knows what
"cpuid_data cache" means.

A message like "VCPU was not initialized yet" would make more
sense.


> +    }
> +
> +    for (i = 0; i < cpuid_data_cached->cpuid.nent; ++i) {
> +        kvm_entry = &cpuid_data_cached->entries[i];
> +        entry = g_malloc0(sizeof(*entry));
> +        entry->leaf = kvm_entry->function;
> +        entry->subleaf = kvm_entry->index;
> +        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 {
> -        struct kvm_cpuid2 cpuid;
> -        struct kvm_cpuid_entry2 entries[KVM_MAX_CPUID_ENTRIES];
> -    } cpuid_data;
>      /*
>       * The kernel defines these structs with padding fields so there
>       * should be no extra padding in our cpuid_data struct.
>       */
> +    struct CPUIDEntriesInfo cpuid_data;
>      QEMU_BUILD_BUG_ON(sizeof(cpuid_data) !=
>                        sizeof(struct kvm_cpuid2) +
>                        sizeof(struct kvm_cpuid_entry2) * KVM_MAX_CPUID_ENTRIES);
> @@ -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));

You are going to copy more entries than necessary, but on the
other hand I like the simplicity of not having to calculate the
struct size before allocating.


> +    }

Now I'm wondering if we want to cache the CPUID tables for all
VCPUs (not just the first one).

Being a debugging command, maybe it's an acceptable compromise to
copy the data only from one VCPU.  If the need to return data for
other VCPUs arise, we can extend the command later.


>  
>      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..f5a926b61b 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-cpu-model-cpuid", ERROR_CLASS_GENERIC_ERROR },
>          { NULL, -1 }
>      };
>      int i;
> -- 
> 2.17.1
> 

-- 
Eduardo

Re: [PATCH v7 1/1] qapi: introduce 'query-cpu-model-cpuid' action

[Valeriy Vdovin]

On Wed, May 26, 2021 at 05:44:24PM -0400, Eduardo Habkost wrote:
> On Tue, May 04, 2021 at 03:26:39PM +0300, Valeriy Vdovin wrote:
> > Introducing new qapi method 'query-cpu-model-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-cpu-model-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-cpu-model-cpuid"
> > }
> > 
> > qmp_response: [
> >   {
> >     "eax": 1073741825,
> >     "edx": 77,
> >     "leaf": 1073741824,
> >     "ecx": 1447775574,
> >     "ebx": 1263359563,
> >     "subleaf": 0
> >   },
> >   {
> >     "eax": 16777339,
> >     "edx": 0,
> >     "leaf": 1073741825,
> >     "ecx": 0,
> >     "ebx": 0,
> >     "subleaf": 0
> >   },
> >   {
> >     "eax": 13,
> >     "edx": 1231384169,
> >     "leaf": 0,
> >     "ecx": 1818588270,
> >     "ebx": 1970169159,
> >     "subleaf": 0
> >   },
> >   {
> >     "eax": 198354,
> >     "edx": 126614527,
> >   ....
> > 
> > Signed-off-by: Valeriy Vdovin <valeriy.vdovin@virtuozzo.com>
> 
> This breaks --disable-kvm builds (see below[1]), but I like the
> simplicity of this solution.
> 
> I think it will be an acceptable and welcome mechanism if we name
> and document it as KVM-specific.
> 
> A debugging command like this that returns the raw CPUID data
> directly from the KVM tables would be very useful for automated
> testing of our KVM CPUID initialization code.  We have some test
> cases for CPU configuration code, but they trust what the CPU
> objects tell us and won't catch mistakes in target/i386/kvm.c
> CPUID code.
> 
> [1] Build error when using --disable-kvm:
> 
>   [449/821] Linking target qemu-system-x86_64
>   FAILED: qemu-system-x86_64
>   c++  -o qemu-system-x86_64 qemu-system-x86_64.p/softmmu_main.c.o libcommon.fa.p/hw_char_virtio-console.c.o [...]
>   /usr/bin/ld: libqemu-x86_64-softmmu.fa.p/meson-generated_.._qapi_qapi-commands-machine-target.c.o: in function `qmp_marshal_query_cpu_model_cpuid':
>   /home/ehabkost/rh/proj/virt/qemu/build/qapi/qapi-commands-machine-target.c:278: undefined reference to `qmp_query_cpu_model_cpuid'
>   collect2: error: ld returned 1 exit status
> 
> 
I'll add if defined(CONFIG_KVM) to this qmp method description.
> > 
> > ---
> > 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
> >  qapi/machine-target.json   | 51 ++++++++++++++++++++++++++++++++++++++
> >  target/i386/kvm/kvm.c      | 45 ++++++++++++++++++++++++++++++---
> >  tests/qtest/qmp-cmd-test.c |  1 +
> >  3 files changed, 93 insertions(+), 4 deletions(-)
> > 
> > diff --git a/qapi/machine-target.json b/qapi/machine-target.json
> > index e7811654b7..ad816a50b6 100644
> > --- a/qapi/machine-target.json
> > +++ b/qapi/machine-target.json
> > @@ -329,3 +329,54 @@
> >  ##
> >  { '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.
> > +#
> > +# CPUID instruction accepts 'leaf' argument passed in EAX register.
> > +# A 'leaf' is a single group of information about the CPU, that is returned
> > +# to the caller in EAX, EBX, ECX and EDX registers. A few of the leaves will
> > +# also have 'subleaves', the group of information would partially depend on the
> > +# value passed in the ECX register. The value of ECX is reflected in the 'subleaf'
> > +# field of this structure.
> > +#
> > +# @leaf: CPUID leaf or the value of EAX prior to CPUID execution.
> > +# @subleaf: value of ECX for leaf that has varying output depending on ECX.
> 
> Instead of having to explain what "leaf" and "subleaf" means,
> maybe it would be simpler to just call them "in_eax" and
> "in_ecx"?
> 
> > +# @eax: eax
> > +# @ebx: ebx
> > +# @ecx: ecx
> > +# @edx: edx
> > +#
> > +# Since: 6.1
> > +##
> > +{ 'struct': 'CpuidEntry',
> > +  'data': { 'leaf' : 'uint32',
> > +            'subleaf' : 'uint32',
> 
> I would make subleaf/in_ecx an optional field.  We don't need to
> return it unless KVM_CPUID_FLAG_SIGNIFCANT_INDEX is set.
> 
> > +            'eax' : 'uint32',
> > +            'ebx' : 'uint32',
> > +            'ecx' : 'uint32',
> > +            'edx' : 'uint32'
> > +          },
> > +  'if': 'defined(TARGET_I386)' }
> > +
> > +##
> > +# @query-cpu-model-cpuid:
> 
> I would choose a name that indicates that the command is
> KVM-specific, like "query-kvm-cpuid" or "query-kvm-cpuid-table".
> 
> > +#
> > +# Returns description of a virtual CPU model, created by QEMU after cpu
> > +# initialization routines. The resulting information is a reflection of a parsed
> > +# '-cpu' command line option, filtered by available host cpu features.
> 
> I don't think "description of a virtual CPU model" is an accurate
> description of this.  I would document it as "returns raw data
> from the KVM CPUID table for the first VCPU".
> 
> I wonder if the "The resulting information is a reflection of a
> parsed [...] cpu features." part is really necessary.  If you
> believe people don't understand how "-cpu" works, this is not
> exactly the right place to explain that.
> 
> If you want to clarify what exactly is returned, maybe something
> like the following would work?
> 
>   "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:  @CpuModelCpuidDescription
> > +#
> > +# Example:
> > +#
> > +# -> { "execute": "query-cpu-model-cpuid" }
> > +# <- { "return": 'CpuModelCpuidDescription' }
> > +#
> > +# Since: 6.1
> > +##
> > +{ 'command': 'query-cpu-model-cpuid',
> > +  'returns': ['CpuidEntry'],
> > +  'if': 'defined(TARGET_I386)' }
> > diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> > index 7fe9f52710..edc4262efb 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,16 +1465,48 @@ static Error *invtsc_mig_blocker;
> >  
> >  #define KVM_MAX_CPUID_ENTRIES  100
> >  
> > +struct CPUIDEntriesInfo {
> > +    struct kvm_cpuid2 cpuid;
> > +    struct kvm_cpuid_entry2 entries[KVM_MAX_CPUID_ENTRIES];
> > +};
> 
> You don't need this new struct definition, as
> (&cpuid_data.cpuid.entries[0]) and (&cpuid_data.entries[0]) are
> exactly the same.  a kvm_cpuid2 pointer would be enough.
> 
> > +
> > +struct CPUIDEntriesInfo *cpuid_data_cached;
> > +
> > +CpuidEntryList *
> > +qmp_query_cpu_model_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, "cpuid_data cache not ready");
> > +        return NULL;
> 
> I would return a more meaningful error message.  Nobody except
> the developers who wrote and reviewed this code knows what
> "cpuid_data cache" means.
> 
> A message like "VCPU was not initialized yet" would make more
> sense.
> 
> 
> > +    }
> > +
> > +    for (i = 0; i < cpuid_data_cached->cpuid.nent; ++i) {
> > +        kvm_entry = &cpuid_data_cached->entries[i];
> > +        entry = g_malloc0(sizeof(*entry));
> > +        entry->leaf = kvm_entry->function;
> > +        entry->subleaf = kvm_entry->index;
> > +        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 {
> > -        struct kvm_cpuid2 cpuid;
> > -        struct kvm_cpuid_entry2 entries[KVM_MAX_CPUID_ENTRIES];
> > -    } cpuid_data;
> >      /*
> >       * The kernel defines these structs with padding fields so there
> >       * should be no extra padding in our cpuid_data struct.
> >       */
> > +    struct CPUIDEntriesInfo cpuid_data;
> >      QEMU_BUILD_BUG_ON(sizeof(cpuid_data) !=
> >                        sizeof(struct kvm_cpuid2) +
> >                        sizeof(struct kvm_cpuid_entry2) * KVM_MAX_CPUID_ENTRIES);
> > @@ -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));
> 
> You are going to copy more entries than necessary, but on the
> other hand I like the simplicity of not having to calculate the
> struct size before allocating.
> 
> 
> > +    }
> 
> Now I'm wondering if we want to cache the CPUID tables for all
> VCPUs (not just the first one).
> 
> Being a debugging command, maybe it's an acceptable compromise to
> copy the data only from one VCPU.  If the need to return data for
> other VCPUs arise, we can extend the command later.
> 
> 
Yes.
As long as there is no specific demand for having multiple VCPU's cached
we can get away with less code. But extending this command would be pretty
straightforward.
> >  
> >      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..f5a926b61b 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-cpu-model-cpuid", ERROR_CLASS_GENERIC_ERROR },
> >          { NULL, -1 }
> >      };
> >      int i;
> > -- 
> > 2.17.1
> > 
> 
> -- 
> Eduardo
>