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

> 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

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