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. > - 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? > > 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. > +} > 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. > > - 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
