Re: [RFC PATCH v3 14/19] target/arm/kvm: compute supported values for ID register fields

On 8/5/26 11:58 AM, Khushit Shah wrote:
>
>> On 26 Jul 2026, at 7:52 PM, Eric Auger <[email protected]> wrote:
>>
>> !-------------------------------------------------------------------|
>>  CAUTION: External Email
>>
>> |-------------------------------------------------------------------!
>>
>> Hi Khushit,
>>
>> On 7/16/26 11:38 PM, Khushit Shah wrote:
>>> Add arm_field_get_supported_values() which, for a given ID register
>>> field, builds the set of values KVM allows it to take on the live host.
>>>
>>> Non-writable fields are pinned to the host value. Writable fields follow
>>> their per field constraints. This function likely needs change when
>>> KVM starts exposing a new writable field that is not lower-safe, hence,
>>> the special handling of TGranX_2/SpecSEI/L1Ip/MIDR/REVIDR/AIDR.
>> so this looks quite risky. 
> I agree.
>
> What are your thoughts on having a authoritative list in QEMU of all
> writable fields? No matter what KVM exposes as writable, actual fields
> we allow to be writable will be intersection of KVM writable + QEMU
> writable. When The QEMU list is updated we make sure things like this
> function is updated.
>
> This get’s rid of cases where we silently support writing to a field.
> It is similar to x86, QEMU only supports writing to specific CPUID
> leafs (by the defined Properties) and not just any CPUID leafs.
>
>>> Cross-field constraints are not modelled; for example ID_AA64ZFR0_EL1
>>> is gated by SVE. Reproducing every inter-field dependency in QEMU
>>> would only duplicate KVM's logic and drift out of sync with it.
>> Isn't qmp_query_cpu_model_expansion sufficient? In general upper layers
>> will try to apply raw named models. If ajustements are needed between
>> source and destination, we know field candidate values (either the
>> source or dest one) and this latter can be directly tried using
>> qmp_query_cpu_model_expansion write. On top of that
>> qmp_query_cpu_model_expansion can be directly checked against a scratch
>> vcpu reusing the logic implemented at kernel level.
> I am not sure if it is acceptable for query_cpu_model_expansion should fail
> if model is not realisable. On at least x86 it does not. It just returns the
> state which QEMU would request KVM for the given configurations.
>
> Ref:
> https://github.com/qemu/qemu/blob/3e3ccab106f879b1512f8e0d51a827dd4de30e22/target/i386/cpu.c#L9689

I need to further study that. I will come back to you.

Currently with v7 you get:
(QEMU) query-cpu-model-expansion type=full
model={"name":"host","props":{"SYSREG_ID_AA64MMFR1_EL1_AFP":0x1}}
{"error": {"class": "GenericError", "desc": "failed to apply new value
0x1 for field AFP (previous is 0x0): Invalid argument"}}

This can be implemented elsewhere though

Eric
>
> cpu-definitions is maybe a better candidate for algo you are describing, but
> it will not take the user overrides into considerations and only say if a
> base model is usable or not
>
>
>>> Signed-off-by: Khushit Shah <[email protected]>
>>> ---
>>> target/arm/kvm.c     | 99 ++++++++++++++++++++++++++++++++++++++++++++
>>> target/arm/kvm_arm.h | 26 ++++++++++++
>>> 2 files changed, 125 insertions(+)
>>>
>>> diff --git a/target/arm/kvm.c b/target/arm/kvm.c
>>> index c38b99cfce..8f452f9570 100644
>>> --- a/target/arm/kvm.c
>>> +++ b/target/arm/kvm.c
>>> @@ -1262,6 +1262,105 @@ bool kvm_arm_cpu_post_load(ARMCPU *cpu)
>>>     return true;
>>> }
>>>
>>> +static bool arm_field_is_signed(const ARM64SysRegField *field)
>>> +{
>>> +    return field_matches(field, ID_AA64MMFR0_EL1_IDX, "TGran4")    ||
>>> +           field_matches(field, ID_AA64MMFR0_EL1_IDX, "TGran64")   ||
>>> +           field_matches(field, ID_MMFR0_EL1_IDX, "InnerShr")      ||
>>> +           field_matches(field, ID_MMFR0_EL1_IDX, "OuterShr")      ||
>>> +           field_matches(field, ID_AA64DFR0_EL1_IDX, "DoubleLock") ||
>>> +           field_matches(field, ID_AA64DFR0_EL1_IDX, "PMUVer")     ||
>>> +           field_matches(field, ID_DFR0_EL1_IDX, "PerfMon")        ||
>>> +           field_matches(field, ID_AA64PFR1_EL1_IDX, "MTE_frac")   ||
>>> +           field_matches(field, ID_AA64MMFR4_EL1_IDX, "E2H0")      ||
>>> +           field_matches(field, ID_DFR1_EL1_IDX, "MTPMU")          ||
>>> +           field_matches(field, ID_AA64PFR0_EL1_IDX, "FP")         ||
>>> +           field_matches(field, ID_AA64PFR0_EL1_IDX, "AdvSIMD");
>> Can't we extract this from Register.json instead?
> AFAIK, There is not signedness data in Register.json
>
>>> +}
>>> +
>>> +static void ranges_add(GArray *ranges, uint64_t min, uint64_t max)
>>> +{
>>> +    ArmFieldRange r = { .min = min, .max = max };
>>> +    g_array_append_val(ranges, r);
>>> +}
>>> +
>>> +void arm_field_get_supported_values(const ARM64SysRegField *field,
>>> +                                    const ARMISARegisters *host_isar,
>>> +                                    ArmFieldValueSet **value_set)
>>> +{
>>> +    bool is_signed = arm_field_is_signed(field);
>>> +    uint64_t host = extract64(host_isar->idregs[field->index],
>>> +                              field->shift, field->length);
>>> +    GArray *ranges = g_array_new(false, false, sizeof(ArmFieldRange));
>>> +
>>> +    /* A non-writable field can only ever hold the host value. */
>>> +    if (!arm_field_is_writable(field)) {
>>> +        ranges_add(ranges, host, host);
>>> +        goto done;
>> you already get this info from qmp_query_cpu_model_expansion
> Can you please elaborate how? I thought the current qmp_query_cpu_model_expansion
> (Your v6) only returns the writable field.
> (I have not yet looked at v7)
>
>>> +    }
>>> +
>>> +    if (field_matches(field, ID_AA64MMFR0_EL1_IDX, "TGran4_2")  ||
>>> +        field_matches(field, ID_AA64MMFR0_EL1_IDX, "TGran16_2") ||
>>> +        field_matches(field, ID_AA64MMFR0_EL1_IDX, "TGran64_2")) {
>>> +        /* Either support the host value or the "off" value */
>>> +        ranges_add(ranges, host, host);
>>> +        if (host != 1) { /* 1 = "off" */
>>> +            ranges_add(ranges, 1, 1);
>>> +        }
>>> +    } else if (field_matches(field, CTR_EL0_IDX, "L1Ip")) {
>>> +        /* Only safe to downgrade to VIPT, other values are reserved. */
>>> +        ranges_add(ranges, host, host);
>>> +        if (host != 2) { /* 2 = "VIPT" */
>>> +            ranges_add(ranges, 2, 2);
>>> +        }
>>> +    } else if (field_matches(field, ID_AA64MMFR1_EL1_IDX, "SpecSEI") ||
>>> +               field_matches(field, ID_MMFR4_EL1_IDX, "SpecSEI")) {
>>> +        /* It is safe to upgrade SpecSEI to 1, other values are reserved. */
>>> +        ranges_add(ranges, host, host);
>>> +        if (host != 1) {
>>> +            ranges_add(ranges, 1, 1);
>>> +        }
>>> +    } else if (field->index == MIDR_EL1_IDX ||
>>> +               field->index == REVIDR_EL1_IDX ||
>>> +               field->index == AIDR_EL1_IDX) {
>>> +        /*
>>> +         * No restriction on value that can be set for implementation ID
>>> +         * registers fields.
>>> +         */
>>> +        uint64_t max = 0;
>>> +        if (field->length == 64) {
>>> +            max = ~0ULL;
>>> +        } else {
>>> +            max = (1ULL << field->length) - 1;
>>> +        }
>>> +        ranges_add(ranges, 0, max);
>>> +    } else {
>>> +        /*
>>> +         * After handling the special cases, other writable fields are
>>> +         * either lower-safe or signed lower-safe.
>>> +         */
>>> +        if (field->arch_vals_count) {
>>> +            for (uint32_t i = 0; i < field->arch_vals_count; i++) {
>>> +                uint64_t av = field->arch_vals[i].value;
>>> +                int64_t v = is_signed ?
>>> +                    sextract64(av, 0, field->length) : (int64_t)av;
>>> +                int64_t hv = is_signed ?
>>> +                    sextract64(host, 0, field->length) : (int64_t)host;
>>> +                if (v <= hv) {
>>> +                    ranges_add(ranges, av, av);
>>> +                }
>>> +            }
>>> +        } else {
>>> +            g_assert(!is_signed); /* No signed field with no arch vals */
>>> +            ranges_add(ranges, 0, host);
>>> +        }
>>> +    }
>>> +done:
>>> +    *value_set = g_new0(ArmFieldValueSet, 1);
>>> +    (*value_set)->n_ranges = ranges->len;
>>> +    (*value_set)->ranges = (ArmFieldRange *)g_array_free(ranges, false);
>>> +}
>>> +
>>> static bool arm_field_skip_writeback_always(const ARM64SysRegField *field)
>>> {
>>>     /*
>>> diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
>>> index 133a026036..9f15c91c4e 100644
>>> --- a/target/arm/kvm_arm.h
>>> +++ b/target/arm/kvm_arm.h
>>> @@ -143,6 +143,32 @@ void kvm_arm_set_cpu_features_from_host(ARMCPU *cpu);
>>> void kvm_arm_add_vcpu_properties(ARMCPU *cpu);
>>>
>>> typedef struct ARM64SysReg ARM64SysReg;
>>> +typedef struct ARM64SysRegField ARM64SysRegField;
>>> +typedef struct ARMISARegisters ARMISARegisters;
>>> +
>>> +typedef struct ArmFieldRange {
>>> +    uint64_t min;
>>> +    uint64_t max;
>>> +} ArmFieldRange;
>>> +
>>> +typedef struct ArmFieldValueSet {
>>> +    ArmFieldRange *ranges;
>>> +    size_t n_ranges;
>>> +} ArmFieldValueSet;
>>> +
>>> +/**
>>> + * arm_field_get_supported_values:
>>> + * @field: The field to get the supported values for
>>> + * @host_isar: The host ISAR registers
>>> + * @value_set: The set of supported values for the @field
>>> + *
>>> + * Will be allocated and filled in with the supported values for the @field
>>> + * based on the host_isar and whether the field is writable or not.
>>> + * The caller must free the value_set.
>>> + */
>>> +void arm_field_get_supported_values(const ARM64SysRegField *field,
>>> +                                    const ARMISARegisters *host_isar,
>>> +                                    ArmFieldValueSet **value_set);
>>>
>>> /**
>>>  * kvm_arm_steal_time_finalize:
>> Thanks
>>
>> Eric
>