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

[Eric Auger via qemu development]

On 7/22/26 2:05 PM, Eric Auger wrote:
> 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 <khushit.s...@nutanix.com>
>> ---
>>  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.
forget this, index refers to the reg while name corresponds to the field
name. Thought name was the prop name ...

Eric


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