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
>