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@xxxxxxxxxxxxx>
>
> 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
>
