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

[Eduardo Habkost <ehabkost@redhat.com>]

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