Re: [PATCH v4] qapi: introduce 'query-cpu-model-cpuid' action

[Vladimir Sementsov-Ogievskiy]

15.04.2021 13:44, 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 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"
> }


[..]

> +static void cpu_model_fill_cpuid(Object *cpu, CpuModelCpuidDescription *info,
> +                                 Error **errp)
> +{
> +    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;
> +    info->model_id = g_strdup(object_property_get_str(cpu, "model-id", errp));
> +    info->vendor = g_strdup(object_property_get_str(cpu, "vendor", errp));

you forget to fix errp :)

if you fix it to &error_abort, and therefore drop errp argument of 
cpu_model_fill_cpuid(), or otherwise, check output of object_property_get_str and 
handle error correctly (just by return of course), take my:

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

(a bit weak, as I don't have good knowledge of the subsystem. The logic seems OK for me, still I 
have no idea about object relations here, for example, how much sense in calling 
qdev_get_machine(), and then go to ms->possible_cpus->cpus[0]. Or, could we always get 
model-id property from cpu object or not (and use &error_abort). So, for me these things are 
"OK if it works". The logic of filling qapi struct is clear, as well as modification of 
cpu_x86_cpuid)

> +
> +    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);
> +        }
> +    }
> +}
> +

[..]


--
Best regards,
Vladimir