> On 14 Aug 2026, at 7:24 PM, Eric Auger <[email protected]> wrote: > > !-------------------------------------------------------------------| > CAUTION: External Email > > |-------------------------------------------------------------------! > > > > 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?
Okay, I think here the confusion lies in perspective of writability. From my perspective, a field is only host non-writable, i.e. every guest on host should have the same field value. But this should not restrict model’s field writability. Take the “pauth” example as explained earlier. A field being non-writable does not mean we cannot change model’s value, it only means we cannot change host value, right? >> >> 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. > I agree, will remove RAS/RES props. Warm Regards, Khushit > 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 = ®->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 = ®->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 = ®s[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 >
