On 8/18/26 11:46 AM, Khushit Shah wrote:
>
>> 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?
Yes that's what I meant. You need to be able to tell I want this field
to be equal to x. If it is not host writable, the model will just read
host value and check this latter and x do match, otherwise fail the
init. However I advocate for not exposing the field as a true qemu
property as it is not host writable.
Eric
>
>>> 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
