qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v6] qapi: introduce 'query-cpu-model-cpuid' action
@ 2021-04-20 16:19 Valeriy Vdovin
  2021-04-20 16:42 ` no-reply
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Valeriy Vdovin @ 2021-04-20 16:19 UTC (permalink / raw)
  To: qemu-devel
  Cc: Eric Blake, Markus Armbruster, Eduardo Habkost, Marcel Apfelbaum,
	Paolo Bonzini, Richard Henderson, Denis Lunev,
	Vladimir Sementsov-Ogievskiy, 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 core part of the returned JSON object can be described as a list of lists
with top level list contains leaf-level elements and the bottom level
containing subleafs, where 'leaf' is CPUID argument passed in EAX register and
'subleaf' is a value passed to CPUID in ECX register for some specific
leafs, that support that. Each most basic CPUID result is passed in a
maximum of 4 registers EAX, EBX, ECX and EDX, with most leafs not utilizing
all 4 registers at once.
Also note that 'subleaf' is a kind of extension, used by only a couple of
leafs, while most of the leafs don't have this. Nevertheless, the output
data structure presents ALL leafs as having at least a single 'subleaf'.
This is done for data structure uniformity, so that it could be
processed in a more straightforward manner, in this case no one suffers
from such simplification.

Use example:
virsh qemu-monitor-command VM --pretty '{ "execute": "query-cpu-model-cpuid" }'
{
  "return": {
    "cpuid": {
      "leafs": [
        {
          "leaf": 0,
          "subleafs": [
            {
              "eax": 13,
              "edx": 1231384169,
              "ecx": 1818588270,
              "ebx": 1970169159,
              "subleaf": 0
            }
          ]
        },
        {
          "leaf": 1,
          "subleafs": [
            {
              "eax": 329443,
              "edx": 529267711,
              "ecx": 4160369187,
              "ebx": 133120,
              "subleaf": 0
            }
          ]
        },
        {
          "leaf": 2,
          "subleafs": [
            {
              "eax": 1,
              "edx": 2895997,
              "ecx": 0,
              "ebx": 0,
              "subleaf": 0
            }
          ]
        },
      ]
    },
    "vendor": "GenuineIntel",
    "class-name": "Skylake-Client-IBRS-x86_64-cpu",
    "model-id": "Intel Core Processor (Skylake, IBRS)"
  },
  "id": "libvirt-40"
}

Also, cpu_x86_cpuid function has been modified to return bool
value depending on what are the arguments of leaf and subleaf
(or index and count). In cases where leaf/subleaf arguments are
invalid the function returns false.
Note, that this method is not really runnable in qmp-tests, as it needs
valid -machine and -cpu command line options, which qmp-tests do not
provide, that's why this method is expected to fail in some graceful
way.

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
 qapi/machine-target.json   | 123 ++++++++++++++++++++
 target/i386/cpu.h          |   2 +-
 target/i386/cpu.c          | 224 ++++++++++++++++++++++++++++++++++---
 tests/qtest/qmp-cmd-test.c |   1 +
 4 files changed, 331 insertions(+), 19 deletions(-)

diff --git a/qapi/machine-target.json b/qapi/machine-target.json
index e7811654b7..6abbc1fb16 100644
--- a/qapi/machine-target.json
+++ b/qapi/machine-target.json
@@ -329,3 +329,126 @@
 ##
 { 'command': 'query-cpu-definitions', 'returns': ['CpuDefinitionInfo'],
   'if': 'defined(TARGET_PPC) || defined(TARGET_ARM) || defined(TARGET_I386) || defined(TARGET_S390X) || defined(TARGET_MIPS)' }
+##
+
+
+# @CpuidSubleaf:
+#
+# CPUID leaf extension information, based on ECX value.
+#
+# CPUID x86 instruction has 'leaf' argument passed in EAX register. Leaf
+# argument identifies the type of information, the caller wants to retrieve in
+# single call to CPUID.
+# Some but not all leaves depend on the value passed in ECX register as an
+# additional argument to CPUID. This argument is present in cpuid documentation
+# as 'subleaf'.
+# If CPUID ignores the value in ECX, normally this means that leaf does not
+# have subleaves. Another way to see it is that each leaf has at least one
+# subleaf (one type of output).
+#
+# @subleaf: value passed to CPUID in ECX register. If CPUID leaf has only a
+#           single leaf, the value of ECX is ignored by CPU and should as well
+#           be ignored in this field.
+# @eax: value in eax after CPUID instruction
+# @ebx: value in ebx after CPUID instruction
+# @ecx: value in ecx after CPUID instruction
+# @edx: value in edx after CPUID instruction
+#
+# Since: 6.1
+##
+{ 'struct': 'CpuidSubleaf',
+  'data': { 'subleaf' : 'int',
+            'eax': 'int',
+            'ebx': 'int',
+            'ecx': 'int',
+            'edx': 'int'
+          }
+}
+
+##
+# @CpuidLeaf:
+#
+# A single CPUID leaf.
+#
+# 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. If the leaf has subleaves, it will
+# only have more than one item in 'subleaves' array. If the leaf has no
+# subleaves, only one item will be present in the 'subleaves' list.
+#
+# @leaf: CPUID leaf or the value of EAX prior to CPUID execution.
+# @subleaves: array of subleaves.
+#
+# Since: 6.1
+##
+{ 'struct': 'CpuidLeaf',
+  'data': { 'leaf' : 'int',
+            'subleaves' : [ 'CpuidSubleaf' ] },
+  'if': 'defined(TARGET_I386)' }
+
+##
+# @CpuModelCpuid:
+#
+# Virtual CPU model.
+#
+# A CPU model consists of the name of a CPU definition, to which
+# delta changes are applied (e.g. features added/removed). Most magic values
+# that an architecture might require should be hidden behind the name.
+# However, if required, architectures can expose relevant properties.
+#
+# @leaves: array of all available cpuid leaves
+#
+# Since: 6.1
+##
+{ 'struct': 'CpuModelCpuid',
+  'data': {  'leaves' : [ 'CpuidLeaf' ] },
+  'if': 'defined(TARGET_I386)' }
+
+##
+# @CpuModelCpuidDescription:
+#
+# Virtual CPU model.
+#
+# This describes information generated by QEMU and used by it to respond CPUID
+# requests from guest along with some general information about the cpu model,
+# that might be useful for the caller of qapi requests.
+#
+# @class-name: class name of the CPU model in qemu object model
+# @model-id: CPU model name string that will be passed in CPUID, EAX=0
+# @vendor: CPU vendor name string that will be passed in CPUID, EAX=0
+# @cpuid: Full tree of CPUID leaves, that is generated by QEMU at virtual cpu
+#         initialization step by parsing "-cpu " option and creating the virtual cpu
+#         model. CpuModelCpuidDescription can be examined to predict QEMU's response to
+#         CPUID instructions from the guest.
+#
+# Since: 6.1
+##
+{ 'struct': 'CpuModelCpuidDescription',
+  'data': {  'class-name': 'str',
+             'model-id': 'str',
+             'vendor' : 'str',
+             'cpuid' : 'CpuModelCpuid'
+          },
+  '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': 'CpuModelCpuidDescription',
+  'if': 'defined(TARGET_I386)' }
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 570f916878..19c1dfea60 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1926,7 +1926,7 @@ int cpu_x86_signal_handler(int host_signum, void *pinfo,
                            void *puc);
 
 /* cpu.c */
-void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
+bool cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
                    uint32_t *eax, uint32_t *ebx,
                    uint32_t *ecx, uint32_t *edx);
 void cpu_clear_apic_feature(CPUX86State *env);
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 6b3e9467f1..6cf17e2442 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -5148,6 +5148,169 @@ CpuDefinitionInfoList *qmp_query_cpu_definitions(Error **errp)
     return cpu_list;
 }
 
+/*
+ * struct CpuidLeafRange - helper struct that describes valid range of
+ * cpuid leaves as returned by CPUID in response to EAX=0 or EAX=0x80000000,
+ * etc.
+ *
+ * The purpose of this struct is to deal with a sparse nature of leaf value
+ * space. The CPUID logic of returning the maximum leaf is not straightforward
+ * and requires inner knowledge of what cpuid extensions are available on a
+ * specific cpu. Also this logic is designed to be expandable across many years
+ * ahead. QEMU code would have to be updated as well. That's why there should
+ * be only one point where all cpuid leaf ranges logic will be modified.
+ *
+ * In practice this will be used to detect if any arbitrary cpuid leaf value
+ * is valid for a specific cpu model. For that one will call
+ * 'cpuid_get_cpuid_leaf_ranges' to get all valid ranges for a provided cpu
+ * model and then call 'cpu_leaf_in_range' to find out which of the ranges
+ * contains the leaf in question.
+ */
+#define CPUID_MAX_LEAF_RANGES 4
+
+typedef struct CpuidLeafRange {
+    uint32_t min;
+    uint32_t max;
+} CpuidLeafRange;
+
+typedef struct CpuidLeafRanges {
+    struct CpuidLeafRange ranges[CPUID_MAX_LEAF_RANGES];
+    int count;
+} CpuidLeafRanges;
+
+static void cpuid_get_cpuid_leaf_ranges(CPUX86State *env,
+                                        struct CpuidLeafRanges *r)
+{
+    struct CpuidLeafRange *rng;
+
+    r->count = 0;
+    rng = &r->ranges[r->count++];
+    rng->min = 0x00000000;
+    rng->max = env->cpuid_level;
+
+    rng = &r->ranges[r->count++];
+    rng->min = 0x40000000;
+    rng->max = 0x40000001;
+
+    if (env->cpuid_xlevel) {
+        rng = &r->ranges[r->count++];
+        rng->min = 0x80000000;
+        rng->max = env->cpuid_xlevel;
+    }
+
+    if (env->cpuid_xlevel2) {
+        rng = &r->ranges[r->count++];
+        rng->min = 0xC0000000;
+        rng->max = env->cpuid_xlevel2;
+    }
+}
+
+static inline bool cpuid_leaf_in_range(uint32_t leaf,
+                                       struct CpuidLeafRange *r)
+{
+    return leaf >= r->min && leaf <= r->max;
+}
+
+static uint32_t cpuid_limit_from_leaf(CPUX86State *env, uint32_t leaf)
+{
+    CpuidLeafRanges ranges;
+    int i;
+
+    cpuid_get_cpuid_leaf_ranges(env, &ranges);
+    for (i = 0; i < ranges.count; ++i) {
+        if (cpuid_leaf_in_range(leaf, &ranges.ranges[i])) {
+            return ranges.ranges[i].max;
+        }
+    }
+    return 0;
+}
+
+static void cpu_model_fill_cpuid(Object *cpu, CpuModelCpuidDescription *info)
+{
+    CpuidLeafRanges ranges;
+    CpuidLeafRange *range;
+    uint32_t eax, ebx, ecx, edx;
+    CpuidLeaf *leaf;
+    CpuidLeafList **leaf_tail;
+    CpuidSubleaf *subleaf;
+    CpuidSubleafList **subleaf_tail;
+    X86CPU *x86_cpu = X86_CPU(cpu);
+
+    int range_idx;
+    int leaf_idx, subleaf_idx;
+
+    info->cpuid = g_malloc0(sizeof(*info->cpuid));
+    leaf_tail = &info->cpuid->leaves;
+
+    cpuid_get_cpuid_leaf_ranges(&x86_cpu->env, &ranges);
+    for (range_idx = 0; range_idx < ranges.count; ++range_idx) {
+        range = &ranges.ranges[range_idx];
+        for (leaf_idx = range->min; leaf_idx <= range->max; ++leaf_idx) {
+            subleaf_idx = 0;
+            if (!cpu_x86_cpuid(&x86_cpu->env, leaf_idx, subleaf_idx, &eax, &ebx,
+                               &ecx, &edx)) {
+                continue;
+            }
+
+            leaf = g_malloc0(sizeof(*leaf));
+            leaf->leaf = leaf_idx;
+            subleaf_tail = &leaf->subleaves;
+            do {
+                subleaf = g_malloc0(sizeof(*subleaf));
+                subleaf->subleaf = subleaf_idx;
+                subleaf->eax = eax;
+                subleaf->ebx = ebx;
+                subleaf->ecx = ecx;
+                subleaf->edx = edx;
+                QAPI_LIST_APPEND(subleaf_tail, subleaf);
+                subleaf_idx++;
+            } while (cpu_x86_cpuid(&x86_cpu->env, leaf_idx, subleaf_idx, &eax,
+                                   &ebx, &ecx, &edx));
+
+            QAPI_LIST_APPEND(leaf_tail, leaf);
+        }
+    }
+}
+
+CpuModelCpuidDescription *qmp_query_cpu_model_cpuid(Error **errp)
+{
+    MachineState *ms = MACHINE(qdev_get_machine());
+    const char *class_name;
+    CpuModelCpuidDescription *info;
+    Object *cpu;
+    char *model_id, *vendor;
+
+    /*
+     * Method requires initialized machine and cpu
+     */
+    if (!ms || !ms->possible_cpus) {
+      error_setg(errp, "Nothing to report");
+      return NULL;
+    }
+
+    cpu = ms->possible_cpus->cpus[0].cpu;
+
+    class_name = object_class_get_name(object_get_class(cpu));
+    model_id = object_property_get_str(cpu, "model-id", errp);
+    if (!model_id) {
+        error_setg(errp, "'model-id' property not found");
+        return NULL;
+    }
+    vendor = object_property_get_str(cpu, "vendor", errp);
+    if (!vendor) {
+        error_setg(errp, "'vendor' property not found");
+        return NULL;
+    }
+
+    info = g_malloc0(sizeof(*info));
+    info->model_id = g_strdup(model_id);
+    info->vendor = g_strdup(vendor);
+    info->class_name = g_strdup(class_name);
+    cpu_model_fill_cpuid(cpu, info);
+
+    return info;
+}
+
 static uint64_t x86_cpu_get_supported_feature_word(FeatureWord w,
                                                    bool migratable_only)
 {
@@ -5591,14 +5754,31 @@ void cpu_clear_apic_feature(CPUX86State *env)
 
 #endif /* !CONFIG_USER_ONLY */
 
-void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
+/*
+ * Emulates CPUID execution on a given x86 cpu model by filling the provided
+ * eax, ebx, ecx, edx output arguments according to CPUID instruction logic,
+ * where index is the value in EAX, when CPUID is called and count is ECX.
+ *
+ * Returns true if current index/count (leaf/subleaf) argument combination is
+ * valid and false in the opposite case.
+ * It's worth noticing that CPUID instruction already has it's own logic for
+ * responding to invalid leaf/subleaf argument combinations. Most of the time
+ * it will fill all output registers with zeroes. But there are cases when this
+ * function will zero out the result in response to a valid leaf/subleaf. Such
+ * response does not help caller to understand if the leaf/subleaf combination
+ * is valid or not, without knowing all the details about CPUID instruction.
+ *
+ * For such callers we explicitly return true of false to indicate exactly does
+ * the leaf or subleaf exist or not.
+ */
+bool cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
                    uint32_t *eax, uint32_t *ebx,
                    uint32_t *ecx, uint32_t *edx)
 {
     X86CPU *cpu = env_archcpu(env);
     CPUState *cs = env_cpu(env);
     uint32_t die_offset;
-    uint32_t limit;
+    uint32_t limit, nr_subleaves = 1;
     uint32_t signature[3];
     X86CPUTopoInfo topo_info;
 
@@ -5607,15 +5787,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
     topo_info.threads_per_core = cs->nr_threads;
 
     /* Calculate & apply limits for different index ranges */
-    if (index >= 0xC0000000) {
-        limit = env->cpuid_xlevel2;
-    } else if (index >= 0x80000000) {
-        limit = env->cpuid_xlevel;
-    } else if (index >= 0x40000000) {
-        limit = 0x40000001;
-    } else {
-        limit = env->cpuid_level;
-    }
+    limit = cpuid_limit_from_leaf(env, index);
 
     if (index > limit) {
         /* Intel documentation states that invalid EAX input will
@@ -5675,8 +5847,10 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
             if ((*eax & 31) && cs->nr_cores > 1) {
                 *eax |= (cs->nr_cores - 1) << 26;
             }
+            return *eax || *ebx || *ecx || *edx;
         } else {
             *eax = 0;
+            nr_subleaves = 3;
             switch (count) {
             case 0: /* L1 dcache info */
                 encode_cache_cpuid4(env->cache_info_cpuid4.l1d_cache,
@@ -5724,6 +5898,12 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
         break;
     case 7:
         /* Structured Extended Feature Flags Enumeration Leaf */
+
+        /*
+         * env->cpuid_level_func7 returns the maximum subleaf index, whereas
+         * nr_subleaves - is the count, that's why + 1.
+         */
+        nr_subleaves = env->cpuid_level_func7 + 1;
         if (count == 0) {
             /* Maximum ECX value for sub-leaves */
             *eax = env->cpuid_level_func7;
@@ -5761,28 +5941,31 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
             *ebx = kvm_arch_get_supported_cpuid(s, 0xA, count, R_EBX);
             *ecx = kvm_arch_get_supported_cpuid(s, 0xA, count, R_ECX);
             *edx = kvm_arch_get_supported_cpuid(s, 0xA, count, R_EDX);
+            return *eax || *ebx || *ecx || *edx;
         } else if (hvf_enabled() && cpu->enable_pmu) {
             *eax = hvf_get_supported_cpuid(0xA, count, R_EAX);
             *ebx = hvf_get_supported_cpuid(0xA, count, R_EBX);
             *ecx = hvf_get_supported_cpuid(0xA, count, R_ECX);
             *edx = hvf_get_supported_cpuid(0xA, count, R_EDX);
+            return *eax || *ebx || *ecx || *edx;
         } else {
             *eax = 0;
             *ebx = 0;
             *ecx = 0;
             *edx = 0;
+            return false;
         }
         break;
     case 0xB:
         /* Extended Topology Enumeration Leaf */
         if (!cpu->enable_cpuid_0xb) {
                 *eax = *ebx = *ecx = *edx = 0;
-                break;
+                return false;
         }
 
         *ecx = count & 0xff;
         *edx = cpu->apic_id;
-
+        nr_subleaves = 2;
         switch (count) {
         case 0:
             *eax = apicid_core_offset(&topo_info);
@@ -5812,6 +5995,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
 
         *ecx = count & 0xff;
         *edx = cpu->apic_id;
+        nr_subleaves = 3;
         switch (count) {
         case 0:
             *eax = apicid_core_offset(&topo_info);
@@ -5843,9 +6027,10 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
         *ecx = 0;
         *edx = 0;
         if (!(env->features[FEAT_1_ECX] & CPUID_EXT_XSAVE)) {
-            break;
+            return false;
         }
 
+        nr_subleaves = ARRAY_SIZE(x86_ext_save_areas);
         if (count == 0) {
             *ecx = xsave_area_size(x86_cpu_xsave_components(cpu));
             *eax = env->features[FEAT_XSAVE_COMP_LO];
@@ -5876,9 +6061,10 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
         *edx = 0;
         if (!(env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_INTEL_PT) ||
             !kvm_enabled()) {
-            break;
+            return false;
         }
 
+        nr_subleaves = 2;
         if (count == 0) {
             *eax = INTEL_PT_MAX_SUBLEAF;
             *ebx = INTEL_PT_MINIMAL_EBX;
@@ -6031,7 +6217,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
         *eax = 0;
         if (cpu->cache_info_passthrough) {
             host_cpuid(index, count, eax, ebx, ecx, edx);
-            break;
+            return *eax || *ebx || *ecx || *edx;
         }
         switch (count) {
         case 0: /* L1 dcache info */
@@ -6054,7 +6240,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
             *eax = *ebx = *ecx = *edx = 0;
             break;
         }
-        break;
+        return count <= 3;
     case 0x8000001E:
         if (cpu->core_id <= 255) {
             encode_topo_cpuid8000001e(cpu, &topo_info, eax, ebx, ecx, edx);
@@ -6063,6 +6249,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
             *ebx = 0;
             *ecx = 0;
             *edx = 0;
+            return false;
         }
         break;
     case 0xC0000000:
@@ -6101,8 +6288,9 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
         *ebx = 0;
         *ecx = 0;
         *edx = 0;
-        break;
+        return false;
     }
+    return count < nr_subleaves;
 }
 
 static void x86_cpu_reset(DeviceState *dev)
diff --git a/tests/qtest/qmp-cmd-test.c b/tests/qtest/qmp-cmd-test.c
index c98b78d033..8ce7c7b27a 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



^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v6] qapi: introduce 'query-cpu-model-cpuid' action
  2021-04-20 16:19 [PATCH v6] qapi: introduce 'query-cpu-model-cpuid' action Valeriy Vdovin
@ 2021-04-20 16:42 ` no-reply
  2021-04-20 17:00 ` Vladimir Sementsov-Ogievskiy
  2021-04-20 17:09 ` Eduardo Habkost
  2 siblings, 0 replies; 9+ messages in thread
From: no-reply @ 2021-04-20 16:42 UTC (permalink / raw)
  To: valeriy.vdovin
  Cc: vsementsov, ehabkost, richard.henderson, qemu-devel, armbru,
	valeriy.vdovin, den, pbonzini

Patchew URL: https://patchew.org/QEMU/20210420161940.24306-1-valeriy.vdovin@virtuozzo.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20210420161940.24306-1-valeriy.vdovin@virtuozzo.com
Subject: [PATCH v6] qapi: introduce 'query-cpu-model-cpuid' action

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/20210420161940.24306-1-valeriy.vdovin@virtuozzo.com -> patchew/20210420161940.24306-1-valeriy.vdovin@virtuozzo.com
Switched to a new branch 'test'
9ee1fb5 qapi: introduce 'query-cpu-model-cpuid' action

=== OUTPUT BEGIN ===
ERROR: suspect code indent for conditional statements (4, 6)
#393: FILE: target/i386/cpu.c:5298:
+    if (!ms || !ms->possible_cpus) {
+      error_setg(errp, "Nothing to report");

total: 1 errors, 0 warnings, 476 lines checked

Commit 9ee1fb5a3953 (qapi: introduce 'query-cpu-model-cpuid' action) has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20210420161940.24306-1-valeriy.vdovin@virtuozzo.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v6] qapi: introduce 'query-cpu-model-cpuid' action
  2021-04-20 16:19 [PATCH v6] qapi: introduce 'query-cpu-model-cpuid' action Valeriy Vdovin
  2021-04-20 16:42 ` no-reply
@ 2021-04-20 17:00 ` Vladimir Sementsov-Ogievskiy
  2021-04-20 17:09 ` Eduardo Habkost
  2 siblings, 0 replies; 9+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-04-20 17:00 UTC (permalink / raw)
  To: Valeriy Vdovin, qemu-devel
  Cc: Eric Blake, Markus Armbruster, Eduardo Habkost, Marcel Apfelbaum,
	Paolo Bonzini, Richard Henderson, Denis Lunev

20.04.2021 19:19, 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.
> 

[..]

> +CpuModelCpuidDescription *qmp_query_cpu_model_cpuid(Error **errp)
> +{
> +    MachineState *ms = MACHINE(qdev_get_machine());
> +    const char *class_name;
> +    CpuModelCpuidDescription *info;
> +    Object *cpu;
> +    char *model_id, *vendor;
> +
> +    /*
> +     * Method requires initialized machine and cpu
> +     */
> +    if (!ms || !ms->possible_cpus) {
> +      error_setg(errp, "Nothing to report");
> +      return NULL;

indentation should be fixed to 4+4=8 spaces totally, as patchew already said.

> +    }
> +
> +    cpu = ms->possible_cpus->cpus[0].cpu;
> +
> +    class_name = object_class_get_name(object_get_class(cpu));
> +    model_id = object_property_get_str(cpu, "model-id", errp);
> +    if (!model_id) {
> +        error_setg(errp, "'model-id' property not found");

object_property_get_str has errp argument, so it should care to set it on error path. You shouldn't call error_setg by hand here, it will crash (trying to set errp which is already set).

> +        return NULL;
> +    }
> +    vendor = object_property_get_str(cpu, "vendor", errp);
> +    if (!vendor) {
> +        error_setg(errp, "'vendor' property not found");

and here

> +        return NULL;
> +    }
> +
> +    info = g_malloc0(sizeof(*info));
> +    info->model_id = g_strdup(model_id);
> +    info->vendor = g_strdup(vendor);
> +    info->class_name = g_strdup(class_name);
> +    cpu_model_fill_cpuid(cpu, info);
> +
> +    return info;
> +}
> +

With fixed style and dropped extra error_setg() calls:

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>


-- 
Best regards,
Vladimir


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v6] qapi: introduce 'query-cpu-model-cpuid' action
  2021-04-20 16:19 [PATCH v6] qapi: introduce 'query-cpu-model-cpuid' action Valeriy Vdovin
  2021-04-20 16:42 ` no-reply
  2021-04-20 17:00 ` Vladimir Sementsov-Ogievskiy
@ 2021-04-20 17:09 ` Eduardo Habkost
  2021-04-21 17:39   ` Valeriy Vdovin
  2 siblings, 1 reply; 9+ messages in thread
From: Eduardo Habkost @ 2021-04-20 17:09 UTC (permalink / raw)
  To: Valeriy Vdovin
  Cc: Vladimir Sementsov-Ogievskiy, Richard Henderson, qemu-devel,
	Markus Armbruster, Denis Lunev, Paolo Bonzini

On Tue, Apr 20, 2021 at 07:19:40PM +0300, Valeriy Vdovin wrote:
[...]
> +##
> +# @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': 'CpuModelCpuidDescription',
> +  'if': 'defined(TARGET_I386)' }

I was assuming the command was going to get a CPU model name as
argument.

If you are only going to return info on the current CPUs, the
interface could be simplified a lot.

What about a simple `query-cpuid` command that only takes:

 { 'qom-path': 'str', # qom-path is returned by query-cpus-fast
   'eax': 'uint32',
   '*ecx': 'uint32' }

as argument, and returns

 { 'present': 'bool',
   'max_eax': 'uint32',    # max value of EAX for this range
   '*max_ecx': 'uint32',   # max value of ECX if there are subleaves
   'eax': 'uint32',
   'ebx': 'uint32',
   'ecx': 'uint32',
   'edx': 'uint32' }

?

-- 
Eduardo



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v6] qapi: introduce 'query-cpu-model-cpuid' action
  2021-04-20 17:09 ` Eduardo Habkost
@ 2021-04-21 17:39   ` Valeriy Vdovin
  2021-04-21 20:17     ` Eduardo Habkost
  0 siblings, 1 reply; 9+ messages in thread
From: Valeriy Vdovin @ 2021-04-21 17:39 UTC (permalink / raw)
  To: Eduardo Habkost
  Cc: qemu-devel, Eric Blake, Markus Armbruster, Marcel Apfelbaum,
	Paolo Bonzini, Richard Henderson, Denis Lunev,
	Vladimir Sementsov-Ogievskiy

On Tue, Apr 20, 2021 at 01:09:00PM -0400, Eduardo Habkost wrote:
> On Tue, Apr 20, 2021 at 07:19:40PM +0300, Valeriy Vdovin wrote:
> [...]
> > +##
> > +# @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': 'CpuModelCpuidDescription',
> > +  'if': 'defined(TARGET_I386)' }
> 
> I was assuming the command was going to get a CPU model name as
> argument.
> 
> If you are only going to return info on the current CPUs, the
> interface could be simplified a lot.
> 
> What about a simple `query-cpuid` command that only takes:
> 
>  { 'qom-path': 'str', # qom-path is returned by query-cpus-fast
>    'eax': 'uint32',
>    '*ecx': 'uint32' }
> 
> as argument, and returns
> 
>  { 'present': 'bool',
>    'max_eax': 'uint32',    # max value of EAX for this range
>    '*max_ecx': 'uint32',   # max value of ECX if there are subleaves
>    'eax': 'uint32',
>    'ebx': 'uint32',
>    'ecx': 'uint32',
>    'edx': 'uint32' }
> 
> ?
Hi. The interface that you suggest looks good. But it has one critical
point that deems it unusable for our initial needs. The point of this
whole new API is to take away the strain of knowing about leaf ranges
from the caller of this API. In my current patch this goal works. So
the caller does not need to know in advance what ranges there are in
original CPUID as well as in it's tweaked QEMU's version.

But you suggested API is not so kind to the caller, so he would need
to add some logic around the call that knows about exact leaf ranges.
If you have a solution to that also, I'll be happy to discuss it.

The obvious thing that comes to mind is changing the exists/max_ecx pair
to something like next_eax, next_ecx. But this idea will probably require
the same amount of complexity that I currently have in this patch.

Thanks.
> 
> -- 
> Eduardo
> 


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v6] qapi: introduce 'query-cpu-model-cpuid' action
  2021-04-21 17:39   ` Valeriy Vdovin
@ 2021-04-21 20:17     ` Eduardo Habkost
  2021-04-22  9:02       ` Valeriy Vdovin
  2021-04-22  9:41       ` Markus Armbruster
  0 siblings, 2 replies; 9+ messages in thread
From: Eduardo Habkost @ 2021-04-21 20:17 UTC (permalink / raw)
  To: Valeriy Vdovin
  Cc: Vladimir Sementsov-Ogievskiy, Richard Henderson, qemu-devel,
	Markus Armbruster, Denis Lunev, Paolo Bonzini

On Wed, Apr 21, 2021 at 08:39:42PM +0300, Valeriy Vdovin wrote:
> On Tue, Apr 20, 2021 at 01:09:00PM -0400, Eduardo Habkost wrote:
> > On Tue, Apr 20, 2021 at 07:19:40PM +0300, Valeriy Vdovin wrote:
> > [...]
> > > +##
> > > +# @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': 'CpuModelCpuidDescription',
> > > +  'if': 'defined(TARGET_I386)' }
> > 
> > I was assuming the command was going to get a CPU model name as
> > argument.
> > 
> > If you are only going to return info on the current CPUs, the
> > interface could be simplified a lot.
> > 
> > What about a simple `query-cpuid` command that only takes:
> > 
> >  { 'qom-path': 'str', # qom-path is returned by query-cpus-fast
> >    'eax': 'uint32',
> >    '*ecx': 'uint32' }
> > 
> > as argument, and returns
> > 
> >  { 'present': 'bool',
> >    'max_eax': 'uint32',    # max value of EAX for this range
> >    '*max_ecx': 'uint32',   # max value of ECX if there are subleaves
> >    'eax': 'uint32',
> >    'ebx': 'uint32',
> >    'ecx': 'uint32',
> >    'edx': 'uint32' }
> > 
> > ?
> Hi. The interface that you suggest looks good. But it has one critical
> point that deems it unusable for our initial needs. The point of this
> whole new API is to take away the strain of knowing about leaf ranges
> from the caller of this API. In my current patch this goal works. So
> the caller does not need to know in advance what ranges there are in
> original CPUID as well as in it's tweaked QEMU's version.
>

Raw CPUID data is a pretty low level interface, already.  Is it
really too much of a burden for callers to know that CPUID ranges
start at 0, 0x40000000, 0x80000000, and 0xC0000000?

(Especially considering that it would save us ~100 extra lines of
C code and maybe 50-100 extra lines of QAPI schema code.)


> But you suggested API is not so kind to the caller, so he would need
> to add some logic around the call that knows about exact leaf ranges.
> If you have a solution to that also, I'll be happy to discuss it.

Would be following (Python-like pseudocode) be too much of a
burden for consumers of the command?

    for start in (0, 0x40000000, 0x80000000, 0xC0000000):
        leaf = query_cpuid(qom_path, start)
        for eax in range(start, leaf.max_eax + 1):
            for ecx in range(0, leaf.get('max_ecx', 0) + 1):
                all_leaves.append(query_cpuid(qom_path, eax, ecx))

> 
> The obvious thing that comes to mind is changing the exists/max_ecx pair
> to something like next_eax, next_ecx. But this idea will probably require
> the same amount of complexity that I currently have in this patch.

I agree.  I'm trying to reduce the complexity of the interface
and of the command implementation.

-- 
Eduardo



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v6] qapi: introduce 'query-cpu-model-cpuid' action
  2021-04-21 20:17     ` Eduardo Habkost
@ 2021-04-22  9:02       ` Valeriy Vdovin
  2021-04-23 20:32         ` Eduardo Habkost
  2021-04-22  9:41       ` Markus Armbruster
  1 sibling, 1 reply; 9+ messages in thread
From: Valeriy Vdovin @ 2021-04-22  9:02 UTC (permalink / raw)
  To: Eduardo Habkost
  Cc: qemu-devel, Eric Blake, Markus Armbruster, Marcel Apfelbaum,
	Paolo Bonzini, Richard Henderson, Denis Lunev,
	Vladimir Sementsov-Ogievskiy

On Wed, Apr 21, 2021 at 04:17:59PM -0400, Eduardo Habkost wrote:
> On Wed, Apr 21, 2021 at 08:39:42PM +0300, Valeriy Vdovin wrote:
> > On Tue, Apr 20, 2021 at 01:09:00PM -0400, Eduardo Habkost wrote:
> > > On Tue, Apr 20, 2021 at 07:19:40PM +0300, Valeriy Vdovin wrote:
> > > [...]
> > > > +##
> > > > +# @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': 'CpuModelCpuidDescription',
> > > > +  'if': 'defined(TARGET_I386)' }
> > > 
> > > I was assuming the command was going to get a CPU model name as
> > > argument.
> > > 
> > > If you are only going to return info on the current CPUs, the
> > > interface could be simplified a lot.
> > > 
> > > What about a simple `query-cpuid` command that only takes:
> > > 
> > >  { 'qom-path': 'str', # qom-path is returned by query-cpus-fast
> > >    'eax': 'uint32',
> > >    '*ecx': 'uint32' }
> > > 
> > > as argument, and returns
> > > 
> > >  { 'present': 'bool',
> > >    'max_eax': 'uint32',    # max value of EAX for this range
> > >    '*max_ecx': 'uint32',   # max value of ECX if there are subleaves
> > >    'eax': 'uint32',
> > >    'ebx': 'uint32',
> > >    'ecx': 'uint32',
> > >    'edx': 'uint32' }
> > > 
> > > ?
> > Hi. The interface that you suggest looks good. But it has one critical
> > point that deems it unusable for our initial needs. The point of this
> > whole new API is to take away the strain of knowing about leaf ranges
> > from the caller of this API. In my current patch this goal works. So
> > the caller does not need to know in advance what ranges there are in
> > original CPUID as well as in it's tweaked QEMU's version.
> >
> 
> Raw CPUID data is a pretty low level interface, already.  Is it
> really too much of a burden for callers to know that CPUID ranges
> start at 0, 0x40000000, 0x80000000, and 0xC0000000?
> 
> (Especially considering that it would save us ~100 extra lines of
> C code and maybe 50-100 extra lines of QAPI schema code.)
> 
> 
> > But you suggested API is not so kind to the caller, so he would need
> > to add some logic around the call that knows about exact leaf ranges.
> > If you have a solution to that also, I'll be happy to discuss it.
> 
> Would be following (Python-like pseudocode) be too much of a
> burden for consumers of the command?
> 
>     for start in (0, 0x40000000, 0x80000000, 0xC0000000):
>         leaf = query_cpuid(qom_path, start)
>         for eax in range(start, leaf.max_eax + 1):
>             for ecx in range(0, leaf.get('max_ecx', 0) + 1):
>                 all_leaves.append(query_cpuid(qom_path, eax, ecx))
> 
This is a question of architecture and design. Number of lines is
secondary (up to some reasonable point of course).

I want to decouple components as much as possible. It's not a burden to pass
4 digits once you know them, but how exactly should a caller come to these 4 
digits? It's like a password. It's easy once you know it. Check out Intel's
Instruction Set Manual on CPUID - obvious place to learn about ranges for the
caller, yet you wont see exactly these numbers in plain text. And where is 
0x40000000 in this manual exactly? One should read QEMU git history to know 
what it is. Correct me here if I'm wrong.

The work of figuring out the required ranges should not be duplicated without
need. QEMU does that already, there is a nice way of passing them to the caller,
and it makes component interaction more generic (no private knowledge pased),
so why not do that.

Please take into account the design of applications that will use this
method. With less restrictive API, components could be more isolated, for
example one component could only know how to call qmp methods, the other would
have to khow how to process CPUID data, resulting in a clean layered architecture.

Also I'm not sure that these ranges would never change. Are you sure that some
other range won't appear soon? If so, shouldn't we try to make less locations,
where this would have to be supported?

So my pros are: loose coupling / generic interface, less places to
maintain in case of chages. These are pretty basic.
Cons: more lines of code as you've said, but otherwize more code will be
in the callers, and more callers == more duplicated code.

Thanks.
> > 
> > The obvious thing that comes to mind is changing the exists/max_ecx pair
> > to something like next_eax, next_ecx. But this idea will probably require
> > the same amount of complexity that I currently have in this patch.
> 
> I agree.  I'm trying to reduce the complexity of the interface
> and of the command implementation.
> 
> -- 
> Eduardo
> 


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v6] qapi: introduce 'query-cpu-model-cpuid' action
  2021-04-21 20:17     ` Eduardo Habkost
  2021-04-22  9:02       ` Valeriy Vdovin
@ 2021-04-22  9:41       ` Markus Armbruster
  1 sibling, 0 replies; 9+ messages in thread
From: Markus Armbruster @ 2021-04-22  9:41 UTC (permalink / raw)
  To: Eduardo Habkost
  Cc: Vladimir Sementsov-Ogievskiy, Daniel P. Berrangé,
	Richard Henderson, qemu-devel, Valeriy Vdovin, Paolo Bonzini,
	Denis Lunev

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Wed, Apr 21, 2021 at 08:39:42PM +0300, Valeriy Vdovin wrote:
>> On Tue, Apr 20, 2021 at 01:09:00PM -0400, Eduardo Habkost wrote:
>> > On Tue, Apr 20, 2021 at 07:19:40PM +0300, Valeriy Vdovin wrote:
>> > [...]
>> > > +##
>> > > +# @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': 'CpuModelCpuidDescription',
>> > > +  'if': 'defined(TARGET_I386)' }
>> > 
>> > I was assuming the command was going to get a CPU model name as
>> > argument.
>> > 
>> > If you are only going to return info on the current CPUs, the
>> > interface could be simplified a lot.
>> > 
>> > What about a simple `query-cpuid` command that only takes:
>> > 
>> >  { 'qom-path': 'str', # qom-path is returned by query-cpus-fast
>> >    'eax': 'uint32',
>> >    '*ecx': 'uint32' }
>> > 
>> > as argument, and returns
>> > 
>> >  { 'present': 'bool',
>> >    'max_eax': 'uint32',    # max value of EAX for this range
>> >    '*max_ecx': 'uint32',   # max value of ECX if there are subleaves
>> >    'eax': 'uint32',
>> >    'ebx': 'uint32',
>> >    'ecx': 'uint32',
>> >    'edx': 'uint32' }
>> > 
>> > ?
>> Hi. The interface that you suggest looks good. But it has one critical
>> point that deems it unusable for our initial needs. The point of this
>> whole new API is to take away the strain of knowing about leaf ranges
>> from the caller of this API. In my current patch this goal works. So
>> the caller does not need to know in advance what ranges there are in
>> original CPUID as well as in it's tweaked QEMU's version.
>>
>
> Raw CPUID data is a pretty low level interface, already.  Is it
> really too much of a burden for callers to know that CPUID ranges
> start at 0, 0x40000000, 0x80000000, and 0xC0000000?
>
> (Especially considering that it would save us ~100 extra lines of
> C code and maybe 50-100 extra lines of QAPI schema code.)
>
>
>> But you suggested API is not so kind to the caller, so he would need
>> to add some logic around the call that knows about exact leaf ranges.
>> If you have a solution to that also, I'll be happy to discuss it.
>
> Would be following (Python-like pseudocode) be too much of a
> burden for consumers of the command?
>
>     for start in (0, 0x40000000, 0x80000000, 0xC0000000):
>         leaf = query_cpuid(qom_path, start)
>         for eax in range(start, leaf.max_eax + 1):
>             for ecx in range(0, leaf.get('max_ecx', 0) + 1):
>                 all_leaves.append(query_cpuid(qom_path, eax, ecx))
>
>> 
>> The obvious thing that comes to mind is changing the exists/max_ecx pair
>> to something like next_eax, next_ecx. But this idea will probably require
>> the same amount of complexity that I currently have in this patch.
>
> I agree.  I'm trying to reduce the complexity of the interface
> and of the command implementation.

This command appears to be primarily motivated by a container use case
that doesn't involve QEMU (other than as a provider of a language to
construct CPU models)[1].  It has secondary applications that do involve
QEMU, but they seem quite limited (automated tests[2], debugging).

This is rather weak justification for QMP command.  It may suffice for a
really simple patch along the lines Eduardo proposed.  Any additional
complexity would be a hard sell, though.



[1] Message-ID: <20210329112153.GA413337@dhcp-172-16-24-191.sw.ru>
https://lists.gnu.org/archive/html/qemu-devel/2021-03/msg09463.html

[2] Message-ID: <20210419202336.shf3yo7lmr7tmzvp@habkost.net>
https://lists.gnu.org/archive/html/qemu-devel/2021-04/msg03697.html



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v6] qapi: introduce 'query-cpu-model-cpuid' action
  2021-04-22  9:02       ` Valeriy Vdovin
@ 2021-04-23 20:32         ` Eduardo Habkost
  0 siblings, 0 replies; 9+ messages in thread
From: Eduardo Habkost @ 2021-04-23 20:32 UTC (permalink / raw)
  To: Valeriy Vdovin
  Cc: Vladimir Sementsov-Ogievskiy, Richard Henderson, qemu-devel,
	Markus Armbruster, Denis Lunev, Paolo Bonzini

On Thu, Apr 22, 2021 at 12:02:15PM +0300, Valeriy Vdovin wrote:
> On Wed, Apr 21, 2021 at 04:17:59PM -0400, Eduardo Habkost wrote:
> > On Wed, Apr 21, 2021 at 08:39:42PM +0300, Valeriy Vdovin wrote:
> > > On Tue, Apr 20, 2021 at 01:09:00PM -0400, Eduardo Habkost wrote:
> > > > On Tue, Apr 20, 2021 at 07:19:40PM +0300, Valeriy Vdovin wrote:
> > > > [...]
> > > > > +##
> > > > > +# @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': 'CpuModelCpuidDescription',
> > > > > +  'if': 'defined(TARGET_I386)' }
> > > > 
> > > > I was assuming the command was going to get a CPU model name as
> > > > argument.
> > > > 
> > > > If you are only going to return info on the current CPUs, the
> > > > interface could be simplified a lot.
> > > > 
> > > > What about a simple `query-cpuid` command that only takes:
> > > > 
> > > >  { 'qom-path': 'str', # qom-path is returned by query-cpus-fast
> > > >    'eax': 'uint32',
> > > >    '*ecx': 'uint32' }
> > > > 
> > > > as argument, and returns
> > > > 
> > > >  { 'present': 'bool',
> > > >    'max_eax': 'uint32',    # max value of EAX for this range
> > > >    '*max_ecx': 'uint32',   # max value of ECX if there are subleaves
> > > >    'eax': 'uint32',
> > > >    'ebx': 'uint32',
> > > >    'ecx': 'uint32',
> > > >    'edx': 'uint32' }
> > > > 
> > > > ?
> > > Hi. The interface that you suggest looks good. But it has one critical
> > > point that deems it unusable for our initial needs. The point of this
> > > whole new API is to take away the strain of knowing about leaf ranges
> > > from the caller of this API. In my current patch this goal works. So
> > > the caller does not need to know in advance what ranges there are in
> > > original CPUID as well as in it's tweaked QEMU's version.
> > >
> > 
> > Raw CPUID data is a pretty low level interface, already.  Is it
> > really too much of a burden for callers to know that CPUID ranges
> > start at 0, 0x40000000, 0x80000000, and 0xC0000000?
> > 
> > (Especially considering that it would save us ~100 extra lines of
> > C code and maybe 50-100 extra lines of QAPI schema code.)
> > 
> > 
> > > But you suggested API is not so kind to the caller, so he would need
> > > to add some logic around the call that knows about exact leaf ranges.
> > > If you have a solution to that also, I'll be happy to discuss it.
> > 
> > Would be following (Python-like pseudocode) be too much of a
> > burden for consumers of the command?
> > 
> >     for start in (0, 0x40000000, 0x80000000, 0xC0000000):
> >         leaf = query_cpuid(qom_path, start)
> >         for eax in range(start, leaf.max_eax + 1):
> >             for ecx in range(0, leaf.get('max_ecx', 0) + 1):
> >                 all_leaves.append(query_cpuid(qom_path, eax, ecx))
> > 
> This is a question of architecture and design. Number of lines is
> secondary (up to some reasonable point of course).
> 
> I want to decouple components as much as possible. It's not a burden to pass
> 4 digits once you know them, but how exactly should a caller come to these 4 
> digits? It's like a password. It's easy once you know it. Check out Intel's
> Instruction Set Manual on CPUID - obvious place to learn about ranges for the
> caller, yet you wont see exactly these numbers in plain text. And where is 
> 0x40000000 in this manual exactly? One should read QEMU git history to know 
> what it is. Correct me here if I'm wrong.
> 
> The work of figuring out the required ranges should not be duplicated without
> need. QEMU does that already, there is a nice way of passing them to the caller,
> and it makes component interaction more generic (no private knowledge pased),
> so why not do that.
> 
> Please take into account the design of applications that will use this
> method. With less restrictive API, components could be more isolated, for
> example one component could only know how to call qmp methods, the other would
> have to khow how to process CPUID data, resulting in a clean layered architecture.

Raw CPUID data is pretty low level x86 detail.  You are asking
for low level data to be exported, but you are asking for that
data to be exported in a nice package that doesn't require
knowledge of low level x86 details.

I understand how that could be nice and useful.  I'm not sure
this is QEMU's job, though.

If exporting the raw CPUID data in a nice self-contained format
is so important for you, you can just publish something similar
to the 5 lines of pseudo-Python code above somewhere.  Maybe it
could be shipped inside QEMU's ./scripts directory.

> 
> Also I'm not sure that these ranges would never change. Are you sure that some
> other range won't appear soon? If so, shouldn't we try to make less locations,
> where this would have to be supported?
> 

Maybe a new range will appear during the next decade?  I don't
know.  If it happens, low level software components will have to
deal with it.

I'm sure the time I spent writing this email is probably more
than the time required to adapt your theoretical QMP callers to
support a new CPUID range.


> So my pros are: loose coupling / generic interface, less places to
> maintain in case of chages. These are pretty basic.
> Cons: more lines of code as you've said, but otherwize more code will be
> in the callers, and more callers == more duplicated code.

I believe this boils down to either this is a job for QEMU and
for the QEMU maintainers.  Most of the feedback you got goes in
the direction of not including the QMP command at all.

I'm trying reach a compromise here: I understand `query-cpuid`
wouldn't be the interface you wish you had, but it's the one that
seems more likely to be accepted (at least without additional
refactoring of internal QEMU code or long interface design
discussions).

-- 
Eduardo



^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-04-23 20:33 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-20 16:19 [PATCH v6] qapi: introduce 'query-cpu-model-cpuid' action Valeriy Vdovin
2021-04-20 16:42 ` no-reply
2021-04-20 17:00 ` Vladimir Sementsov-Ogievskiy
2021-04-20 17:09 ` Eduardo Habkost
2021-04-21 17:39   ` Valeriy Vdovin
2021-04-21 20:17     ` Eduardo Habkost
2021-04-22  9:02       ` Valeriy Vdovin
2021-04-23 20:32         ` Eduardo Habkost
2021-04-22  9:41       ` Markus Armbruster

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).