Re: [RFC PATCH v3 08/19] target/arm/kvm: Handle writeback for special ID register fields

> 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(&regidx, 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
>> 
>