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