> On 14 Aug 2026, at 7:34 PM, Eric Auger <[email protected]> wrote: > > !-------------------------------------------------------------------| > CAUTION: External Email > > |-------------------------------------------------------------------! > > Hi Khushit, > > On 8/3/26 6:03 PM, Khushit Shah wrote: >> >>> On 22 Jul 2026, at 5:35 PM, Eric Auger <[email protected]> wrote: >>> >>> !-------------------------------------------------------------------| >>> CAUTION: External Email >>> >>> |-------------------------------------------------------------------! >>> >>> Hi Khushit, >>> On 7/16/26 11:38 PM, Khushit Shah wrote: >>>> Some fields should not be written back to KVM for various reasons: >>>> - ID_AA64PFR0_EL1.GIC / ID_PFR1_EL1.GIC are fabricated by KVM based on >>>> the gic-version property. >>> so this is part of the overall handling of legacy composite options >>> versus sysreg settings. To be the fact they both are consistent should >>> be enforced upfront. >> I have one idea on how to handle this: >> - After we finalise cpu features, we recheck each command line prop >> passed. The final idregs[]’s prop value should be same as the user >> provided one from the cmdline. >> For e.g., invalid configurations like: >> -cpu host,sve=on,SYSREG_ID_AA64PFR0_EL1_SVE=0 >> Or >> -cpu host,pauth=off,SYSREG_ID_AA64ISAR1_EL1_APA=2 >> >> will be caught after finalise as both of those cannot be true at >> the same time. >> >> If this makes sense, I can implement it for v4. > > Yes consistency checking between low level props and legacy composite > props is one of the next topic to be addressed on the prerequisite series. > I think the next one is to improve the scratch vcpu dry run > in qmp_query_cpu_model_expansion which is not precise enough (it applies > low level settings on a scratch vcpu whose init struct is based on some > host assumptions rather than comprehensive property settings, this > should be refined). If you want to work on this consistency checking > problematic, feel free. Otherwise I will. I will take a stab at consistency checking between SYSREG_ props and legacy properties in v4.
Warm Regards, Khushit > Thanks > > Eric >> >>>> - KVM does not allow writing 0 to ID_DFR0_EL1.CopDbg, which breaks >>>> booting an AArch64-only guest on a host that also supports AArch32. >>>> - ID_DFR0_EL1.PerfMon is populated by KVM even for AArch64-only guests >>>> but is not writable there. >>>> - MPIDR_EL1 is managed by KVM based on vCPU index. >>>> - Hold off writing back CLIDR_EL1 until we properly support exposing >>>> cache topology to a KVM guest. >>> Sounds this deserves a prerequisite fix. I also have this hack in my >>> series but this should be dealt with separately I think. >>>> Some registers such as DCZID_EL0 are not exposed through the KVM cpreg >>>> list at all, but guest still sees the host value, we verify that vCPU's >>>> ID regs values match the host value. >>> why isn't it cpreg then? Shouldn't we add it instead? >> idk why KVM does not expose DCZID_EL0. >> >>>> To do this, refactor kvm_arm_writable_idregs_to_cpreg_list to >>>> kvm_arm_write_idregs_to_cpregs_list, which operates per field and now >>>> returns an error if a non-writable field mismatches with the host value. >>>> >>>> Signed-off-by: Khushit Shah <[email protected]> >>>> --- >>>> target/arm/cpu-idregs.c | 19 +++++++ >>>> target/arm/cpu-idregs.h | 4 ++ >>>> target/arm/kvm.c | 116 ++++++++++++++++++++++++++++++++++------ >>>> target/arm/trace-events | 2 +- >>>> 4 files changed, 125 insertions(+), 16 deletions(-) >>>> >>>> diff --git a/target/arm/cpu-idregs.c b/target/arm/cpu-idregs.c >>>> index c41d0ab0c4..6fa02af0f3 100644 >>>> --- a/target/arm/cpu-idregs.c >>>> +++ b/target/arm/cpu-idregs.c >>>> @@ -7,6 +7,7 @@ >>>> * SPDX-License-Identifier: GPL-2.0-or-later >>>> */ >>>> #include "qemu/osdep.h" >>>> +#include "qemu/bitops.h" >>>> #include "qemu/error-report.h" >>>> #include "qapi/error.h" >>>> #include "cpu.h" >>>> @@ -94,3 +95,21 @@ ARM64SysReg arm64_id_regs[NUM_ID_IDX] = { >>>> #include "cpu-idregs.h.inc" >>>> }; >>>> >>>> +uint64_t arm_get_field_mask(const ARM64SysRegField *field) >>>> +{ >>>> + return MAKE_64BIT_MASK(field->shift, field->length); >>>> +} >>>> + >>>> +bool arm_field_is_writable(const ARM64SysRegField *field) >>>> +{ >>>> + uint64_t field_mask = arm_get_field_mask(field); >>>> + >>>> + return (arm64_id_regs[field->index].writable_mask & field_mask) >>>> + == field_mask; >>>> +} >>>> + >>>> +bool field_matches(const ARM64SysRegField *field, ARMIDRegisterIdx index, >>>> + const char *name) >>>> +{ >>>> + return field->index == index && !strcmp(field->name, name); >>> why do you need to check both index and name. Only checking index should >>> be sufficient. >> Index gives register, name is field name. >> >>>> +} >>>> diff --git a/target/arm/cpu-idregs.h b/target/arm/cpu-idregs.h >>>> index 245f1c8103..d866bd9e0a 100644 >>>> --- a/target/arm/cpu-idregs.h >>>> +++ b/target/arm/cpu-idregs.h >>>> @@ -38,4 +38,8 @@ typedef struct ARM64SysReg { >>>> */ >>>> extern ARM64SysReg arm64_id_regs[NUM_ID_IDX]; >>>> >>>> +uint64_t arm_get_field_mask(const ARM64SysRegField *field); >>>> +bool arm_field_is_writable(const ARM64SysRegField *field); >>>> +bool field_matches(const ARM64SysRegField *field, ARMIDRegisterIdx index, >>>> + const char *name); >>>> #endif >>>> diff --git a/target/arm/kvm.c b/target/arm/kvm.c >>>> index 6974e5c551..c38b99cfce 100644 >>>> --- a/target/arm/kvm.c >>>> +++ b/target/arm/kvm.c >>>> @@ -951,6 +951,16 @@ static uint64_t *kvm_arm_get_cpreg_ptr(ARMCPU *cpu, >>>> uint64_t regidx) >>>> return &cpu->cpreg_values[res - cpu->cpreg_indexes]; >>>> } >>>> >>>> +/* Like kvm_arm_get_cpreg_ptr, but returns NULL if the register is not >>>> found. */ >>>> +static uint64_t *kvm_arm_find_cpreg_ptr(ARMCPU *cpu, uint64_t regidx) >>>> +{ >>>> + uint64_t *res; >>>> + res = bsearch(®idx, cpu->cpreg_indexes, cpu->cpreg_array_len, >>>> + sizeof(uint64_t), compare_u64); >>>> + >>>> + return res ? &cpu->cpreg_values[res - cpu->cpreg_indexes] : NULL; >>>> +} >>>> + >>>> /** >>>> * kvm_arm_reg_syncs_via_cpreg_list: >>>> * @regidx: KVM register index >>>> @@ -1252,32 +1262,105 @@ bool kvm_arm_cpu_post_load(ARMCPU *cpu) >>>> return true; >>>> } >>>> >>>> +static bool arm_field_skip_writeback_always(const ARM64SysRegField *field) >>>> +{ >>>> + /* >>>> + * GIC is controlled by the gic-version property and fabricated by KVM >>>> + * when the vGIC device is created, so a named CPU model must not >>>> touch >>>> + * it. KVM also rejects writing 0 to ID_DFR0_EL1.CopDbg, which breaks >>>> + * booting a model that lacks AArch32 support on a host that supports >>>> + * it. MPIDR_EL1 is managed by KVM based on number of vCPUs, skip >>>> + * writing it back. Similarly, skip writing back CLIDR_EL1 till we >>>> + * properly support exposing cache for KVM guest. >>>> + */ >>>> + return field_matches(field, ID_AA64PFR0_EL1_IDX, "GIC") >>>> + || field_matches(field, ID_PFR1_EL1_IDX, "GIC") >>>> + || field_matches(field, ID_DFR0_EL1_IDX, "CopDbg") >>>> + || field->index == MPIDR_EL1_IDX >>>> + || field->index == CLIDR_EL1_IDX; >>>> +} >>>> + >>>> +static bool arm_field_skip_writeback_if_not_writable(const >>>> ARM64SysRegField *field) >>>> +{ >>>> + /* >>>> + * KVM populates ID_DFR0_EL1.PerfMon even for AArch64-only guests but >>>> + * does not expose it as writable there, so skip it when it is not >>>> + * writable. >>>> + */ >>>> + return field_matches(field, ID_DFR0_EL1_IDX, "PerfMon"); >>>> +} >>>> + >>>> /* >>>> - * Copy writable ID regs from isar.idregs[] to cpreg_list >>>> - * in case their value differs from the original init cpreg value >>>> + * Copy writable ID reg fields from isar.idregs[] into the KVM cpreg list, >>>> + * so the subsequent write_list_to_kvmstate() pushes them to KVM. >>>> + * Only writable fields are copied; fields that must not be written back >>>> + * (see arm_field_skip_writeback_*) are skipped. >>>> + * Returns -1 if any vCPU's ID reg value differs from the host value and >>>> the >>>> + * field is not writable. >>>> */ >>>> -static void kvm_arm_writable_idregs_to_cpreg_list(ARMCPU *cpu) >>>> +static int kvm_arm_write_idregs_to_cpreg_list(ARMCPU *cpu) >>>> { >>>> for (int i = 0; i < NUM_ID_IDX; i++) { >>>> ARM64SysReg *sysregdesc = &arm64_id_regs[i]; >>>> ARMSysRegs sysreg = id_register_sysreg[i]; >>>> - uint64_t previous, new; >>>> + uint64_t writable_mask = sysregdesc->writable_mask; >>>> + uint64_t desired = cpu->isar.idregs[i]; >>>> + uint64_t previous, updated; >>>> uint64_t *cpreg; >>>> >>>> - if (!sysregdesc->writable_mask) { >>>> + cpreg = kvm_arm_find_cpreg_ptr(cpu, >>>> idregs_sysreg_to_kvm_reg(sysreg)); >>>> + /* >>>> + * Registers such as DCZID_EL0 are not exposed through the cpreg >>>> + * list and are therefore not writable. Use the host value >>>> + * snapshotted at probe time as the reference to check the vCPU ID >>>> + * regs have the same value. >>>> + */ >>>> + previous = cpreg ? *cpreg : arm_host_cpu_features.isar.idregs[i]; >>>> + >>>> + if (previous == desired) { >>>> continue; >>>> } >>> while at it you could also test all reserved fields here, RAZ, RES0/1 >>> and make sure host value complies with the description. >> Noted, will add it in v4 >> >> Warm Regards, >> Khushit >>>> - cpreg = kvm_arm_get_cpreg_ptr(cpu, >>>> idregs_sysreg_to_kvm_reg(sysreg)); >>>> - previous = *cpreg; >>>> - new = cpu->isar.idregs[i]; >>>> + for (int j = 0; j < sysregdesc->fields_count; j++) { >>>> + const ARM64SysRegField *field = &sysregdesc->fields[j]; >>>> + uint64_t field_mask = arm_get_field_mask(field); >>>> + uint64_t prev_val = previous & field_mask; >>>> + uint64_t new_val = desired & field_mask; >>>> + >>>> + if (prev_val == new_val) { >>>> + continue; >>>> + } >>>> + >>>> + if (arm_field_skip_writeback_always(field) || >>>> + (!arm_field_is_writable(field) && >>>> + arm_field_skip_writeback_if_not_writable(field))) { >>>> + /* Never write this field back; keep KVM's value. */ >>>> + writable_mask &= ~field_mask; >>>> + continue; >>>> + } >>>> + >>>> + if (!arm_field_is_writable(field)) { >>>> + error_report("%s.%s is not writable: host=0x%" PRIx64 >>>> + ", requested=0x%" PRIx64, >>>> + sysregdesc->name, field->name, >>>> + prev_val >> field->shift, new_val >> >>>> field->shift); >>>> + return -1; >>>> + } >>>> + } >>>> + >>>> + if (!cpreg || !writable_mask) { >>>> + continue; >>>> + } >>>> >>>> - if (previous != new) { >>>> - *cpreg = new; >>>> - trace_kvm_arm_writable_idregs_to_cpreg_list(sysregdesc->name, >>>> - previous, new); >>>> - } >>>> + updated = (previous & ~writable_mask) | (desired & writable_mask); >>>> + if (updated != previous) { >>>> + *cpreg = updated; >>>> + trace_kvm_arm_write_idregs_to_cpreg_list(sysregdesc->name, >>>> + previous, >>>> updated); >>>> + } >>>> } >>>> + >>>> + return 0; >>>> } >>>> >>>> void kvm_arm_reset_vcpu(ARMCPU *cpu) >>>> @@ -2235,8 +2318,11 @@ int kvm_arch_init_vcpu(CPUState *cs) >>>> if (ret) { >>>> return ret; >>>> } >>>> - /* overwrite writable ID regs with their updated property values */ >>>> - kvm_arm_writable_idregs_to_cpreg_list(cpu); >>>> + /* overwrite ID reg fields with their updated property values */ >>>> + ret = kvm_arm_write_idregs_to_cpreg_list(cpu); >>>> + if (ret) { >>>> + return ret; >>>> + } >>>> ret = write_list_to_kvmstate(cpu, KVM_PUT_FULL_STATE); >>>> if (!ret) { >>>> return -1; >>>> diff --git a/target/arm/trace-events b/target/arm/trace-events >>>> index f33b0d821d..f042ab59b8 100644 >>>> --- a/target/arm/trace-events >>>> +++ b/target/arm/trace-events >>>> @@ -14,7 +14,7 @@ arm_gt_update_irq(int timer, int irqstate) >>>> "gt_update_irq: timer %d irqstate %d" >>>> # kvm.c >>>> 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 >>>> +kvm_arm_write_idregs_to_cpreg_list(const char *name, uint64_t previous, >>>> uint64_t new) "%s overwrite default 0x%"PRIx64" with 0x%"PRIx64 >>>> >>>> # cpu64.c >>>> get_sysreg_prop(const char *name, uint64_t value) "%s 0x%"PRIx64 >>> Thanks >>> >>> Eric >> >
