Re: [RFC PATCH v3 04/19] target/arm: expose all ID regs fields as properties

On 8/3/26 5:05 PM, Khushit Shah wrote:
>
>> On 21 Jul 2026, at 8:14 PM, Eric Auger <[email protected]> wrote:
>>
>> !-------------------------------------------------------------------|
>>  CAUTION: External Email
>>
>> |-------------------------------------------------------------------!
>>
>> Hi Khushit,
>>
>> On 7/16/26 11:38 PM, Khushit Shah wrote:
>>> Upcoming named cpu models layer requires exposing all the ID register
>>> fields as properties.
>>>
>>> While doing so also move the code to cpu64.c to make it
>>> accelerator-agnostic, and rename the function to
>>> `aarch64_add_idregs_properties()`.
>> Better to split that patch into 2 to ease the review:
>> 1) update the code in kvm to expose all ID regs including those which
>> are not writable and adaptation in set_sysreg_prop to handle RAZ,
>> RES0/1, ...
>> 2) Move to cpu64.c
> This makes more sense, thanks, will do that in v4.
>
>> To me it is still arguable whether it makes sense to expose settings
>> which cannot be really set. To me It pollutes the qmp introspection
>> output and I don't know whether there is any case history.
>> Why can't we:
>> - only expose writable id regs as props
>> - other non writable settings you need to check could be registered in a
>> hash table during named cpu model definition in props. 
>> in arm_apply_model_props() when you enumerate all props you can check if
>> there is an existing prop for this name, in which case you really call
>>
>> object_property_set_bool(), otherwise, you go through another path, which is not visible by the end user where you check the KVM value matches your "prop" value. No need to pretend this is writable.
> Please refer to my below text. 
>
>> Also I am doubtful about RES/RAZ.
>>
>> +    MODEL_PROP("SYSREG_CTR_EL0_RES1_31", NUM, 1),
>>
>> does not make sense to me
>>
>> Why can't you enumate all ID regs and go through those fields and check the KVM value matches. Why would we need to explicitly set a RAS/RES0/1 field. Why can't that be checked transparently.
> Why do we need non-writable field props?:
> - Assume some non-writable field mismatches for the model and
>   host, why do we want to stop users from explicitly overriding
>   the model's field value to host value?
> - Even with cpu -host, how will upper layer know, if they can
>   migrate between host A and host B? (Without actually doing
>   the migration)
>      -> Only if all non-writable fields in both host matches
>         and writable fields in both host are “compatible”.

I do agree with you we need to check non writable fields do match. What
I am challenging is the fact you want to expose non writable idregs
through qemu properties. If it is not writable why would we pretend the
end user can change it through the cmd line. You can set the expected
field value in the vcpu model description through DEFINEs or whatever
method. Then on init, the model will read the host value and fails the
init if they don't match. The fact no prop is available for this field
would teach the end user the field is not writable. 

The above MODEL_PROP() could be a wrapper that either sets the actual
property if it exists (for a writable field) or just silently populate a
table of expected non writable field values that are checked on init, no?
>
> Why would we need to explicitly set a RAS/RES0/1 field?
> - Here my argument is a little weaker but, what if RES1 field
>   is repurposed in the future? We still want existing models
>   to show 1.
AARCHMRS description tells you which fields are RESx. You can read the
host value and fails the init in case of mismatch.

Thanks

Eric
>   RES0 does not need any special handling, only RES1 field needs
>   this special handling.
>
>
> Does the above arguments make sense? 
>
> Warm Regards,
> Khushit
>
>> Thanks
>>
>> Eric
>>
>>> This makes porting this solution to other accelerators easy in future.
>>>
>>> Signed-off-by: Khushit Shah <[email protected]>
>>> ---
>>> target/arm/cpu64.c      | 116 +++++++++++++++++++++++++++++-
>>> target/arm/internals.h  |   3 +
>>> target/arm/kvm.c        | 151 ----------------------------------------
>>> target/arm/kvm_arm.h    |   8 ---
>>> target/arm/trace-events |   4 +-
>>> 5 files changed, 119 insertions(+), 163 deletions(-)
>>>
>>> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
>>> index 92a01d4ce7..cdad0fc46d 100644
>>> --- a/target/arm/cpu64.c
>>> +++ b/target/arm/cpu64.c
>>> @@ -38,6 +38,7 @@
>>> #include "internals.h"
>>> #include "cpu-features.h"
>>> #include "cpu-idregs.h"
>>> +#include "trace.h"
>>>
>>> /* convert between <register>_IDX and SYS_<register> */
>>> #define DEF(NAME, OP0, OP1, CRN, CRM, OP2)      \
>>> @@ -644,6 +645,113 @@ static const Property arm_cpu_pauth_qarma3_property =
>>> static Property arm_cpu_pauth_qarma5_property =
>>>     DEFINE_PROP_BOOL("pauth-qarma5", ARMCPU, prop_pauth_qarma5, false);
>>>
>>> +#ifndef CONFIG_USER_ONLY
>>> +static void set_sysreg_prop(Object *obj, Visitor *v,
>>> +                            const char *name, void *opaque,
>>> +                            Error **errp)
>>> +{
>>> +    ARM64SysRegField *field = (ARM64SysRegField *)opaque;
>>> +    ARMCPU *cpu = ARM_CPU(obj);
>>> +    uint64_t *idregs = cpu->isar.idregs;
>>> +    uint64_t old, value, mask;
>>> +    int lower = field->shift;
>>> +    int length = field->length;
>>> +    int index = field->index;
>>> +
>>> +    if (!visit_type_uint64(v, name, &value, errp)) {
>>> +        return;
>>> +    }
>>> +
>>> +    if (length < 64 && value > ((1ULL << length) - 1)) {
>>> +        error_setg(errp,
>>> +                   "idreg %s set value (0x%lx) exceeds length of field (%d)!",
>>> +                   name, value, length);
>>> +        return;
>>> +    }
>>> +
>>> +    if (field->arch_vals) {
>>> +        /* this field has some enum values */
>>> +        for (int i = 0; i < field->arch_vals_count; i++) {
>>> +            if (value == field->arch_vals[i].value) {
>>> +                goto valid;
>>> +            }
>>> +        }
>>> +        error_setg(errp,
>>> +                   "idreg %s set value (0x%lx) does not match any "
>>> +                   "arch valid enum value!", name, value);
>>> +        return;
>>> +    }
>>> +
>>> +    /* If the field is RESx or RAZ, it should not be written anything else */
>>> +    if (strstr(field->name, "RES0") && value != 0) {
>>> +        error_setg(errp, "idreg %s is RES0 field and cannot write %ld",
>>> +            name, value);
>>> +        return;
>>> +    } else if (strstr(field->name, "RAZ") && value != 0) {
>>> +        error_setg(errp, "idreg %s is RAZ field and cannot write %ld",
>>> +            name, value);
>>> +        return;
>>> +    } else if (strstr(field->name, "RES1") && value != 1) {
>>> +        error_setg(errp, "idreg %s is RES1 field and cannot write %ld",
>>> +            name, value);
>>> +        return;
>>> +    }
>>> +
>>> +valid:
>>> +    mask = MAKE_64BIT_MASK(lower, length);
>>> +    value = value << lower;
>>> +    old = idregs[index];
>>> +    idregs[index] = old & ~mask;
>>> +    idregs[index] |= value;
>>> +    trace_set_sysreg_prop(name, old, mask, value, idregs[index]);
>>> +}
>>> +
>>> +static void get_sysreg_prop(Object *obj, Visitor *v,
>>> +                            const char *name, void *opaque,
>>> +                            Error **errp)
>>> +{
>>> +    ARM64SysRegField *field = (ARM64SysRegField *)opaque;
>>> +    ARMCPU *cpu = ARM_CPU(obj);
>>> +    uint64_t *idregs = cpu->isar.idregs;
>>> +    uint64_t value, mask;
>>> +    int lower = field->shift;
>>> +    int length = field->length;
>>> +    int index = field->index;
>>> +
>>> +    mask = MAKE_64BIT_MASK(lower, length);
>>> +    value = (idregs[index] & mask) >> lower;
>>> +    visit_type_uint64(v, name, &value, errp);
>>> +    trace_get_sysreg_prop(name, value);
>>> +}
>>> +
>>> +static int
>>> +aarch64_add_idreg_properties(Object *obj, ARM64SysReg *reg)
>>> +{
>>> +    int nb_sysreg_props = 0;
>>> +
>>> +    for (int i = 0; i < reg->fields_count; i++) {
>>> +        ARM64SysRegField *field = &reg->fields[i];
>>> +        char *prop_name = g_strdup_printf("SYSREG_%s_%s", reg->name,
>>> +                                          field->name);
>>> +        object_property_add(obj, prop_name, "uint64",
>>> +                            get_sysreg_prop, set_sysreg_prop, NULL, field);
>>> +        g_free(prop_name);
>>> +        nb_sysreg_props++;
>>> +    }
>>> +
>>> +    trace_nb_sysreg_props(reg->name, nb_sysreg_props);
>>> +    return 0;
>>> +}
>>> +
>>> +void aarch64_add_idregs_properties(Object *obj)
>>> +{
>>> +    for (int i = 0; i < NUM_ID_IDX; i++) {
>>> +        ARM64SysReg *reg = &arm64_id_regs[i];
>>> +        aarch64_add_idreg_properties(obj, reg);
>>> +    }
>>> +}
>>> +#endif
>>> +
>>> void aarch64_add_pauth_properties(Object *obj)
>>> {
>>>     ARMCPU *cpu = ARM_CPU(obj);
>>> @@ -865,8 +973,12 @@ static void aarch64_host_initfn(Object *obj)
>>>     kvm_arm_set_cpu_features_from_host(cpu);
>>>     aarch64_add_sve_properties(obj);
>>>
>>> -    /* generate SYSREG properties according to writable masks */
>>> -    kvm_arm_expose_idreg_properties(cpu, arm64_id_regs);
>>> +#ifndef CONFIG_USER_ONLY
>>> +    /* generate SYSREG properties */
>>> +    if (kvm_enabled()) {
>>> +        aarch64_add_idregs_properties(obj);
>>> +    }
>>> +#endif
>>>
>>> #elif defined(CONFIG_HVF)
>>>     hvf_arm_set_cpu_features_from_host(cpu);
>>> diff --git a/target/arm/internals.h b/target/arm/internals.h
>>> index a632584a4e..7502b6db8a 100644
>>> --- a/target/arm/internals.h
>>> +++ b/target/arm/internals.h
>>> @@ -1754,6 +1754,9 @@ void aarch64_max_tcg_initfn(Object *obj);
>>> void aarch64_add_pauth_properties(Object *obj);
>>> void aarch64_add_sve_properties(Object *obj);
>>> void aarch64_add_sme_properties(Object *obj);
>>> +#ifndef CONFIG_USER_ONLY
>>> +void aarch64_add_idregs_properties(Object *obj);
>>> +#endif
>>>
>>> /* Return true if the gdbstub is presenting an AArch64 CPU */
>>> static inline bool arm_gdbstub_is_aarch64(ARMCPU *cpu)
>>> diff --git a/target/arm/kvm.c b/target/arm/kvm.c
>>> index 892433a612..76583db3f0 100644
>>> --- a/target/arm/kvm.c
>>> +++ b/target/arm/kvm.c
>>> @@ -327,157 +327,6 @@ static int get_host_cpu_idregs(ARMCPU *cpu, int fd, ARMHostCPUFeatures *ahcf)
>>>     return err;
>>> }
>>>
>>> -static ARM64SysRegField *get_field(int i, ARM64SysReg *reg)
>>> -{
>>> -    for (int f = 0; f < reg->fields_count; f++) {
>>> -        struct ARM64SysRegField *field = &reg->fields[f];
>>> -        int upper = field->shift + field->length - 1;
>>> -
>>> -        if (i >= field->shift && i <= upper) {
>>> -            return field;
>>> -        }
>>> -    }
>>> -    return NULL;
>>> -}
>>> -
>>> -static void set_sysreg_prop(Object *obj, Visitor *v,
>>> -                            const char *name, void *opaque,
>>> -                            Error **errp)
>>> -{
>>> -    ARM64SysRegField *field = (ARM64SysRegField *)opaque;
>>> -    ARMCPU *cpu = ARM_CPU(obj);
>>> -    uint64_t *idregs = cpu->isar.idregs;
>>> -    uint64_t old, value, mask;
>>> -    int lower = field->shift;
>>> -    int length = field->length;
>>> -    int index = field->index;
>>> -
>>> -    if (!visit_type_uint64(v, name, &value, errp)) {
>>> -        return;
>>> -    }
>>> -
>>> -    if (length < 64 && value > ((1 << length) - 1)) {
>>> -        error_setg(errp,
>>> -                   "idreg %s set value (0x%lx) exceeds length of field (%d)!",
>>> -                   name, value, length);
>>> -        return;
>>> -    }
>>> -
>>> -    if (field->arch_vals) {
>>> -        /* this field has some enum values */
>>> -        for (int i = 0; i < field->arch_vals_count; i++) {
>>> -            if (value == field->arch_vals[i].value) {
>>> -                goto valid;
>>> -            }
>>> -        }
>>> -        error_setg(errp,
>>> -                   "idreg %s set value (0x%lx) does not match any "
>>> -                   "arch valid enum value!", name, value);
>>> -        return;
>>> -    }
>>> -
>>> -valid:
>>> -
>>> -    mask = MAKE_64BIT_MASK(lower, length);
>>> -    value = value << lower;
>>> -    old = idregs[index];
>>> -    idregs[index] = old & ~mask;
>>> -    idregs[index] |= value;
>>> -    trace_set_sysreg_prop(name, old, mask, value, idregs[index]);
>>> -}
>>> -
>>> -static void get_sysreg_prop(Object *obj, Visitor *v,
>>> -                            const char *name, void *opaque,
>>> -                            Error **errp)
>>> -{
>>> -    ARM64SysRegField *field = (ARM64SysRegField *)opaque;
>>> -    ARMCPU *cpu = ARM_CPU(obj);
>>> -    uint64_t *idregs = cpu->isar.idregs;
>>> -    uint64_t value, mask;
>>> -    int lower = field->shift;
>>> -    int length = field->length;
>>> -    int index = field->index;
>>> -
>>> -    mask = MAKE_64BIT_MASK(lower, length);
>>> -    value = (idregs[index] & mask) >> lower;
>>> -    visit_type_uint64(v, name, &value, errp);
>>> -    trace_get_sysreg_prop(name, value);
>>> -}
>>> -
>>> -/*
>>> - * decode_idreg_writemap: Generate props for writable fields
>>> - *
>>> - * @obj: CPU object
>>> - * @reg: description of the sysreg
>>> - */
>>> -static int
>>> -decode_idreg_writemap(Object *obj, ARM64SysReg *reg)
>>> -{
>>> -    uint64_t map = reg->writable_mask;
>>> -    int i = ctz64(map);
>>> -    int nb_sysreg_props = 0;
>>> -
>>> -    while (map) {
>>> -        ARM64SysRegField *field = get_field(i, reg);
>>> -        int lower, upper;
>>> -        char *prop_name;
>>> -        uint64_t field_mask;
>>> -
>>> -        if (!field) {
>>> -            warn_report("%s bit %d of %s is writable but no named field "
>>> -                        "in target/arm/cpu-idregs.h.inc",
>>> -                        __func__, i, reg->name);
>>> -            warn_report("%s is target/arm/cpu-idregs.h.inc up-to-date?", __func__);
>>> -            map =  map & ~BIT_ULL(i);
>>> -            i = ctz64(map);
>>> -            continue;
>>> -        }
>>> -        lower = field->shift;
>>> -        upper = field->shift + field->length - 1;
>>> -
>>> -        /* Sanity check the field is not a reserved field */
>>> -        if (strstr(field->name, "RES0") || strstr(field->name, "RES1") ||
>>> -            strstr(field->name, "RAZ")) {
>>> -            trace_unexpected_writable_reserved_field(reg->name, field->name,
>>> -                                                     lower, upper);
>>> -            continue;
>>> -        }
>>> -        prop_name = g_strdup_printf("SYSREG_%s_%s", reg->name, field->name);
>>> -        trace_decode_idreg_writemap(field->name, lower, upper, prop_name);
>>> -        object_property_add(obj, prop_name, "uint64",
>>> -                            get_sysreg_prop, set_sysreg_prop, NULL, field);
>>> -        g_free(prop_name);
>>> -        nb_sysreg_props++;
>>> -
>>> -        field_mask = MAKE_64BIT_MASK(lower, field->length);
>>> -        map = map & ~field_mask;
>>> -        i = ctz64(map);
>>> -    }
>>> -    trace_nb_sysreg_props(reg->name, nb_sysreg_props);
>>> -    return 0;
>>> -}
>>> -
>>> -/* analyze the writable mask and generate properties for writable fields */
>>> -void kvm_arm_expose_idreg_properties(ARMCPU *cpu, ARM64SysReg *regs)
>>> -{
>>> -    Object *obj = OBJECT(cpu);
>>> -
>>> -    for (int i = 0; i < NUM_ID_IDX; i++) {
>>> -        ARM64SysReg *sysregdesc = &regs[i];
>>> -
>>> -        if (sysregdesc->writable_mask) {
>>> -            /*
>>> -             * special case REVIDR_EL1 and AIDR_EL1 which are writable but
>>> -             * do not expose named fields. They will need to be handled
>>> -             * separately
>>> -             */
>>> -            if (strcmp(sysregdesc->name, "REVIDR_EL1") &&
>>> -                strcmp(sysregdesc->name, "AIDR_EL1")) {
>>> -                decode_idreg_writemap(obj, sysregdesc);
>>> -            }
>>> -        }
>>> -    }
>>> -}
>>>
>>> static void
>>> kvm_arm_get_host_cpu_features(ARMCPU *cpu, ARMHostCPUFeatures *ahcf)
>>> diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
>>> index 2b3474cc36..133a026036 100644
>>> --- a/target/arm/kvm_arm.h
>>> +++ b/target/arm/kvm_arm.h
>>> @@ -143,14 +143,6 @@ void kvm_arm_set_cpu_features_from_host(ARMCPU *cpu);
>>> void kvm_arm_add_vcpu_properties(ARMCPU *cpu);
>>>
>>> typedef struct ARM64SysReg ARM64SysReg;
>>> -/**
>>> - * kvm_arm_expose_idreg_properties:
>>> - * @cpu: The CPU object to generate the properties for
>>> - * @reg: registers from the host
>>> - *
>>> - * analyze the writable mask and generate properties for writable fields
>>> - */
>>> -void kvm_arm_expose_idreg_properties(ARMCPU *cpu, ARM64SysReg *regs);
>>>
>>> /**
>>>  * kvm_arm_steal_time_finalize:
>>> diff --git a/target/arm/trace-events b/target/arm/trace-events
>>> index e67730de6b..f33b0d821d 100644
>>> --- a/target/arm/trace-events
>>> +++ b/target/arm/trace-events
>>> @@ -15,11 +15,11 @@ arm_gt_update_irq(int timer, int irqstate) "gt_update_irq: timer %d irqstate %d"
>>> kvm_arm_fixup_msi_route(uint64_t iova, uint64_t gpa) "MSI iova = 0x%"PRIx64" is translated into 0x%"PRIx64
>>> get_host_cpu_idregs(const char *name, uint64_t value) "scratch vcpu host value for %s is 0x%"PRIx64
>>> kvm_arm_writable_idregs_to_cpreg_list(const char *name, uint64_t previous, uint64_t new) "%s overwrite default 0x%"PRIx64" with 0x%"PRIx64
>>> -decode_idreg_writemap(const char* name, int lower, int upper, char *prop_name) "%s [%d:%d] is writable (prop %s)"
>>> +
>>> +# cpu64.c
>>> get_sysreg_prop(const char *name, uint64_t value) "%s 0x%"PRIx64
>>> set_sysreg_prop(const char *name, uint64_t old, uint64_t mask, uint64_t field_value, uint64_t new) "%s old reg value=0x%"PRIx64" mask=0x%"PRIx64" new field value=0x%"PRIx64" new reg value=0x%"PRIx64
>>> nb_sysreg_props(const char *name, int count) "%s: %d SYSREG properties"
>>> -unexpected_writable_reserved_field(const char *reg_name, const char *field_name, int lower, int upper) "Unexpected writable reserved field: %s.%s [%d,%d], skip it ..."
>>>
>>> # cpu.c
>>> arm_cpu_reset(uint64_t mp_aff) "cpu %" PRIu64