Re: [PATCH v9 25/26] arm-qmp-cmds: introspection for ID register props

[Eric Auger <eric.auger@redhat.com>]

Hi Khushit,

On 9/28/26 4:39 PM, Khushit Shah wrote:
>
>> On 16 Sep 2026, at 8:15 PM, Eric Auger <eric.auger@redhat.com> wrote:
>>
>> !-------------------------------------------------------------------|
>>  CAUTION: External Email
>>
>> |-------------------------------------------------------------------!
>>
>> From: Cornelia Huck <cohuck@redhat.com>
>>
>> Enhance the current host model query-cpu-model-expansion with the
>> capability to return SYSREG_* options and also test possible
>> values.
>>
>> If no SYSREG prop is set, query-cpu-model-expansion returns the
>> default host values for all SYSREG properties. They are extracted
>> from a scratch vcpu featuring all the KVM advertised capabilities.
>>
>> Here is a qmp-shell example:
>>
>> (QEMU) query-cpu-model-expansion type=full model={"name":"host"}
>> {"return": {"model": {"name": "host", "props": {"SYSREG_ID_AA64PFR0_EL1_EL3": 1,
>> "SYSREG_ID_AA64ISAR2_EL1_CLRBHB": 0, "SYSREG_CTR_EL0_L1Ip": 3,
>> "SYSREG_MIDR_EL1_PartNum": 3407, "SYSREG_CTR_EL0_DminLine": 4,
>> "SYSREG_ID_AA64MMFR0_EL1_PARange": 5, "SYSREG_ID_AA64MMFR1_EL1_ECBHB": 0
>> ../..
>>
>> It becomes also possible to test new SYSREG property values.
>>
>> With Full expansion of the host model, the SYSREG props are tested
>> against a scratch vcpu. With static expansion, no check is performed
>> against KVM but only against qemu code (the value does not exceed the
>> field size and if the field is associated to enum values, the set
>> value belongs to the list of arch enum values.
>>
>> Examples
>> (QEMU) query-cpu-model-expansion type=full model={"name":"host","props":{"SYSREG_ID_AA64ISAR0_EL1_DP":0x13}}
>> {"error": {"class": "GenericError", "desc": "idreg SYSREG_ID_AA64ISAR0_EL1_DP set value (0x13) exceeds length of field (4)!"}}
>>
>> (QEMU) query-cpu-model-expansion type=full model={"name":"host","props":{"SYSREG_ID_AA64ISAR0_EL1_DP":0x2}}
>> {"error": {"class": "GenericError", "desc": "idreg SYSREG_ID_AA64ISAR0_EL1_DP set value (0x2) does not match any arch valid enum value!"}}
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
>>
>> ---
>>
>> v8 -> v9:
>> - fix the leak reported by Khushit
>> - implement static mode for host
>>
>> v7 -> v8:
>> - do not check if the prop exists (this is checked elsewhere with a
>>  different error message)
>> - clarify that examples given in the commit msg are based on qmp-shell
>>
>> v5 -> v6:
>> - add the write capability
>> ---
>> target/arm/arm-qmp-cmds.c | 107 ++++++++++++++++++++++++++++++++++++--
>> 1 file changed, 104 insertions(+), 3 deletions(-)
>>
>> diff --git a/target/arm/arm-qmp-cmds.c b/target/arm/arm-qmp-cmds.c
>> index 83ec95c290..b52ca67f83 100644
>> --- a/target/arm/arm-qmp-cmds.c
>> +++ b/target/arm/arm-qmp-cmds.c
>> @@ -21,6 +21,7 @@
>>  */
>>
>> #include "qemu/osdep.h"
>> +#include "qemu/error-report.h"
>> #include "qemu/target-info.h"
>> #include "hw/core/boards.h"
>> #include "kvm_arm.h"
>> @@ -30,6 +31,7 @@
>> #include "qapi/qapi-commands-machine.h"
>> #include "qapi/qapi-commands-misc-arm.h"
>> #include "qobject/qdict.h"
>> +#include "qobject/qnum.h"
>> #include "qom/qom-qobject.h"
>> #include "cpu.h"
>>
>> @@ -83,15 +85,20 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
>>                                                      CpuModelInfo *model,
>>                                                      Error **errp)
>> {
>> +    bool use_scratch_vcpu = kvm_enabled() && type == CPU_MODEL_EXPANSION_TYPE_FULL;
>>     CpuModelExpansionInfo *expansion_info;
>> +    ObjectPropertyIterator iter;
>>     const QDict *qdict_in;
>> +    ObjectProperty *idregprop;
>>     QDict *qdict_out;
>>     ObjectClass *oc;
>>     Object *obj;
>>     const char *name;
>> +    int fdarray[3];
>>     int i;
>>
>> -    if (type != CPU_MODEL_EXPANSION_TYPE_FULL) {
>> +    /* we support static expansion for host model only */
>> +    if (type != CPU_MODEL_EXPANSION_TYPE_FULL && strcmp(model->name, "host")) {
>>         error_setg(errp, "The requested expansion type is not supported");
>>         return NULL;
>>     }
>> @@ -136,6 +143,7 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
>>     if (model->props) {
>>         Visitor *visitor;
>>         Error *err = NULL;
>> +        int fd = -1;
>>
>>         visitor = qobject_input_visitor_new(model->props);
>>         if (!visit_start_struct(visitor, "model.props", NULL, 0, errp)) {
>> @@ -146,6 +154,8 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
>>
>>         qdict_in = qobject_to(QDict, model->props);
>>         i = 0;
>> +
>> +        /* Test legacy composite option settings */
>>         while ((name = cpu_model_advertised_features[i++]) != NULL) {
>>             if (qdict_get(qdict_in, name)) {
>>                 if (!object_property_set(obj, name, visitor, &err)) {
>> @@ -154,11 +164,85 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
>>             }
>>         }
>>
>> +        if (!err) {
>> +            arm_cpu_finalize_features(ARM_CPU(obj), &err);
>> +        }
>> +
>> +        /**
>> +         * Test SYSREG option settings
>> +         * In full mode, build a scratch vcpu that reflects composite legacy
>> +         * options
>> +         */
>> +        if (use_scratch_vcpu) {
>> +            Error *local_err = NULL;
>> +            bool has_virt = object_property_get_bool(OBJECT(current_machine),
>> +                                                     "virtualization",
>> +                                                     &local_err);
>> +
>> +           if (local_err) {
> Hi Eric,
>
> Nit: indentation
sure
>> +                error_free(local_err); /* the machine property does not exist */
>> +            } else {
>> +                if (!has_virt && object_property_find(obj, "has_el2")) {
>> +                    object_property_set_bool(obj, "has_el2", false, NULL);
>> +                }
>> +            }
>> +            fd = kvm_arm_create_init_scratch_vcpu(ARM_CPU(obj), errp);
>> +            if (fd < 0) {
>> +                return NULL;
> Leaks obj/visitor.
Right, I fully revisited the error path
>> +            }
>> +        }
>> +
>> +        qdict_in = qobject_to(QDict, model->props);
>> +        for (const QDictEntry *entry = qdict_first(qdict_in);
>> +                 entry != NULL; entry = qdict_next(qdict_in, entry)) {
>> +            const char *key = qdict_entry_key(entry);
>> +            QObject *val_obj = qdict_entry_value(entry);
>> +            ObjectProperty *prop;
>> +            Visitor *v;
>> +            bool success;
>> +            uint64_t val;
>> +
>> +            prop = object_property_find(obj, key);
>> +
>> +            if (!g_str_has_prefix(key, "SYSREG_")) {
>> +                continue;
>> +            }
>> +
>> +            /* consume the prop to avoid unexpected parameter */
>> +            if (!visit_type_uint64(visitor, key, &val, errp)) {
>> +                goto bail_out;
>> +            }
>> +
>> +            v = qobject_input_visitor_new(val_obj);
>> +
>> +            if (!object_property_set(obj, key, v, errp)) {
>> +                goto bail_out;
>> +            }
>> +
>> +            if (use_scratch_vcpu) {
>> +                ARM64SysRegField *field = (ARM64SysRegField *)prop->opaque;
>> +                uint64_t newfv;
>> +
>> +                if (!visit_type_uint64(v, name, &newfv, errp)) {
> The name is not initialised in this scope… It carries value of last processed legacy prop
damned. now fixed with reusing previously retrieved val
>> +                    visit_free(v);
>> +                    goto bail_out;
>> +                }
>> +                success = kvm_idreg_write_scratch_vcpu(ARM_CPU(obj), fd,
>> +                                                       field, newfv, errp);
>> +                if (!success) {
>> +                    visit_free(v);
>> +                    goto bail_out;
>> +                }
>> +            }
>> +            visit_free(v);
>> +        }
>> +
>>         if (!err) {
>>             visit_check_struct(visitor, &err);
>>         }
>> -        if (!err) {
>> -            arm_cpu_finalize_features(ARM_CPU(obj), &err);
>> +
>> +        if (use_scratch_vcpu) {
>> +            kvm_arm_destroy_scratch_host_vcpu(fdarray);
>>         }
>>         visit_end_struct(visitor, NULL);
>>         visit_free(visitor);
>> @@ -190,6 +274,18 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
>>         }
>>     }
>>
>> +    object_property_iter_init(&iter, obj);
>> +
>> +    while ((idregprop = object_property_iter_next(&iter))) {
>> +        QObject *value;
>> +
>> +        if (!g_str_has_prefix(idregprop->name, "SYSREG_")) {
>> +            continue;
>> +        }
>> +        value = object_property_get_qobject(obj, idregprop->name, &error_abort);
>> +        qdict_put_obj(qdict_out, idregprop->name, value);
>> +    }
>> +
>>     if (!qdict_size(qdict_out)) {
>>         qobject_unref(qdict_out);
>>     } else {
>> @@ -199,6 +295,11 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
>>     object_unref(obj);
>>
>>     return expansion_info;
>> +bail_out:
> Leaks obj.
>> +    if (use_scratch_vcpu) {
>> +        kvm_arm_destroy_scratch_host_vcpu(fdarray);
> fdarray is actually never written to.
also fixed by passing the fdarray as arg of 

kvm_arm_create_init_scratch_vcpu()

Thanks!

Eric

>
> Warm Regards,
> Khushit
>> +    }
>> +    return NULL;
>> }
>>
>> static void arm_cpu_add_definition(gpointer data, gpointer user_data)
>> -- 
>> 2.53.0
>>