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

Khushit Shah <[email protected]>
Newsgroups gmane.comp.emulators.qemu
Message-ID <[email protected]> 

> 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”.

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.
  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
>
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.