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

> On 14 Aug 2026, at 9:04 PM, Eric Auger <[email protected]> wrote:
> 
> !-------------------------------------------------------------------|
>  CAUTION: External Email
> 
> |-------------------------------------------------------------------!
> 
> 
> 
> 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://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_qemu_qemu_blob_3e3ccab106f879b1512f8e0d51a827dd4de30e22_target_i386_cpu.c-23L9689&d=DwIDaQ&c=s883GpUCOChKOHiocYtGcg&r=PGWMyignA0NiDmTlyP7vOTHozBws_VN86yrVmSMkBp0&m=tZWDbcURUU2AEyKCp81oLgldfjQN9gi4FxC0NbxQBfBwXkSy_sHD0WmkMjAdn63_&s=rZTBXpZq8zAQtvwrV_3-hGO_pG2vDKk35_zRgEGq4II&e=
> 
> 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
Maybe in “cpu-definitions” or preferably adding “usable” to the
"cpu-model-expansion” schema?

Warm regards,
Khushit

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