Re: [RFC PATCH v4 17/25] target/arm: fail named CPU models on unknown non-zero feature ID regs

[Eric Auger <eric.auger@redhat.com>]

Hi Khushit
On 9/21/26 2:32 PM, Khushit Shah wrote:
> Named CPU models zero the ID registers QEMU knows about and then apply
> SYSREG_ properties. However, the host and KVM might expose ID
> registers that QEMU does not know about yet. In this scenario, named
this may also happen with reserved fields becoming named, no?
> CPU models might
\n
> leak host features to the guest.
>
> Part of the problem can be addressed because the Arm Architecture
> Reference Manual specifies that unused encodings in the space (op1==0,
> CRn==0, CRm=={2-7}) may be treated as RES0. This range includes all
> ID_AA64* registers.
actually the KVM writable API covers AArch64 System register space

with op0==3, op1=={0, 1, 3}, CRn==0, CRm=={0-7}, op2=={0-7}.
so do you mean you only check part of the space. In the positive don't we need to be more precise and check for a given opcode if it is known


>
> Error out if KVM returns a non-zero value for a register in the above
> space which QEMU does not know about yet.
>
> get_sysreg_idx() now returns -1 for encodings that are not known ID
> registers, and kvm_arm_read_sys_reg64() exposes a KVM GET_ONE_REG
> read for this probe.
>
> Signed-off-by: Khushit Shah <khushit.shah@nutanix.com>
> ---
>  target/arm/cpu-models.c  | 74 ++++++++++++++++++++++++++++++++++++++++
>  target/arm/cpu-sysregs.h |  2 +-
>  target/arm/cpu64.c       |  3 +-
>  target/arm/kvm-stub.c    |  7 ++++
>  target/arm/kvm.c         |  7 ++++
>  target/arm/kvm_arm.h     | 12 +++++++
>  6 files changed, 103 insertions(+), 2 deletions(-)
>
> diff --git a/target/arm/cpu-models.c b/target/arm/cpu-models.c
> index 4e92909755..855eb5ee6b 100644
> --- a/target/arm/cpu-models.c
> +++ b/target/arm/cpu-models.c
> @@ -362,6 +362,78 @@ static void reset_res1_fields(ARMCPU *cpu)
>      }
>  }
>  
> +/*
> + * From ARM ARM:
> + *
> + *     For unused encodings in the part of the space that has op1==0 and
> + *     CRm=={2-7}, direct reads that are not trapped behave as Reserved,
> + *     RES0 to ensure correct behavior if the encodings are used for ID
> + *     registers in the future.
> + *
> + * It includes every ID_AA64* register.
> + *
> + * For named CPU models, we can check if any of the unknown ID registers
> + * that fall into this space is non-zero on the host. If so, the named CPU
> + * model leaks those host features. Error out in such case.
> + *
> + * We cannot do the same for the remaining Feature ID space (op1==0 and
> + * CRm==0, or op1=={1,3}). Unused encodings there are UNDEFINED. The
> + * implemented registers in that space (MIDR_EL1, CTR_EL0, DCZID_EL0,
> + * CCSIDR_EL1, ...) are identification or cache geometry.
> + */
> +static void arm_named_cpu_zero_unknown_feature_ids(ARMCPU *cpu, Error **errp)
> +{
> +    uint32_t init_features0 = 0;
> +    static bool checked;
> +    uint32_t target;
> +    int fdarray[3];
> +    int crm, op2;
> +    int fd;
> +
> +    if (checked) {
> +        return;
> +    }
> +
> +    if (!kvm_arm_create_scratch_max_host_vcpu(fdarray, &init_features0,
> +                                              &target, errp)) {
can't we do that when we retrieve the host features on the 1st created
scratch vcpu. We shall avoid creating too many of them if possible
> +        return;
> +    }
> +
> +    fd = fdarray[2];
> +    for (crm = 2; crm <= 7; crm++) {
> +        for (op2 = 0; op2 <= 7; op2++) {
> +            uint64_t val = 0;
> +            int ret;
> +
> +            if (get_sysreg_idx(ENCODE_ID_REG(3, 0, 0, crm, op2)) != -1) {
> +                continue;
> +            }
> +
> +            ret = kvm_arm_read_sys_reg64(fd, &val, 3, 0, 0, crm, op2);
> +            if (ret) {
> +                continue;
> +            }
> +
> +            /*
> +             * TODO: if this encoding is writable, try to write 0?
> +             */
> +            if (val) {
> +                error_setg(errp,
> +                           "unknown Feature ID register "
> +                           "(op0=3, op1=0, CRn=0, CRm=%d, op2=%d) "
> +                           "reads as 0x%" PRIx64 " on the host; named CPU "
> +                           "models cannot hide it",
> +                           crm, op2, val);
> +                kvm_arm_destroy_scratch_host_vcpu(fdarray);
> +                return;
> +            }
> +        }
> +    }
> +
> +    kvm_arm_destroy_scratch_host_vcpu(fdarray);
> +    checked = true;
> +}
> +
>  static void arm_named_cpu_initfn(Object *obj)
>  {
>      ARMCPU *cpu = ARM_CPU(obj);
> @@ -401,6 +473,8 @@ static void arm_named_cpu_initfn(Object *obj)
>  
>      /* 3. Realize the model chain. */
>      arm_realize_model_chain(obj, model, &error_abort);
> +
> +    arm_named_cpu_zero_unknown_feature_ids(cpu, &error_abort);
>  }
>  
>  void arm_register_named_cpu_models(void)
> diff --git a/target/arm/cpu-sysregs.h b/target/arm/cpu-sysregs.h
> index 7877a3b06a..d48b44705f 100644
> --- a/target/arm/cpu-sysregs.h
> +++ b/target/arm/cpu-sysregs.h
> @@ -37,6 +37,6 @@ typedef enum ARMSysRegs {
>  
>  extern const uint32_t id_register_sysreg[NUM_ID_IDX];
>  
> -int get_sysreg_idx(ARMSysRegs sysreg);
> +int get_sysreg_idx(ARMSysRegs sysreg); /* -1 if @sysreg is not a known ID register */
>  
>  #endif /* ARM_CPU_SYSREGS_H */
> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> index a0114b0fa2..3cc698d65a 100644
> --- a/target/arm/cpu64.c
> +++ b/target/arm/cpu64.c
> @@ -57,8 +57,9 @@ int get_sysreg_idx(ARMSysRegs sysreg)
>  {
>      switch (sysreg) {
>  #include "cpu-sysregs.h.inc"
> +    default:
> +        return -1;
>      }
> -    g_assert_not_reached();
>  }
>  
>  #undef DEF
> diff --git a/target/arm/kvm-stub.c b/target/arm/kvm-stub.c
> index eae2e1ec2c..d263adc921 100644
> --- a/target/arm/kvm-stub.c
> +++ b/target/arm/kvm-stub.c
> @@ -50,6 +50,13 @@ void kvm_arm_set_cpu_features_from_host(ARMCPU *cpu)
>      g_assert_not_reached();
>  }
>  
> +int kvm_arm_read_sys_reg64(int fd, uint64_t *val,
> +                           uint8_t op0, uint8_t op1,
> +                           uint8_t crn, uint8_t crm, uint8_t op2)
> +{
> +    g_assert_not_reached();
> +}
> +
>  void kvm_arm_add_vcpu_properties(ARMCPU *cpu)
>  {
>      g_assert_not_reached();
> diff --git a/target/arm/kvm.c b/target/arm/kvm.c
> index 70d9a07580..6f704c0509 100644
> --- a/target/arm/kvm.c
> +++ b/target/arm/kvm.c
> @@ -263,6 +263,13 @@ static int read_sys_reg64(int fd, uint64_t *pret, uint64_t id)
>      return ioctl(fd, KVM_GET_ONE_REG, &idreg);
>  }
>  
> +int kvm_arm_read_sys_reg64(int fd, uint64_t *val,
> +                           uint8_t op0, uint8_t op1,
> +                           uint8_t crn, uint8_t crm, uint8_t op2)
> +{
> +    return read_sys_reg64(fd, val, ARM64_SYS_REG(op0, op1, crn, crm, op2));
> +}
> +
>  static uint64_t idregs_sysreg_to_kvm_reg(ARMSysRegs sysreg)
>  {
>      return ARM64_SYS_REG((sysreg & CP_REG_ARM64_SYSREG_OP0_MASK) >> CP_REG_ARM64_SYSREG_OP0_SHIFT,
> diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
> index 997120966f..4e563fab96 100644
> --- a/target/arm/kvm_arm.h
> +++ b/target/arm/kvm_arm.h
> @@ -154,6 +154,18 @@ bool kvm_arm_create_scratch_max_host_vcpu(int *fdarray,
>   */
>  void kvm_arm_set_cpu_features_from_host(ARMCPU *cpu);
>  
> +/**
> + * kvm_arm_read_sys_reg64:
> + * @fd: vCPU fd
> + * @val: filled with the register value on success
> + * @op0/@op1/@crn/@crm/@op2: System ID register encoding
> + *
> + * Returns: 0 on success, -1 on failure.
> + */
> +int kvm_arm_read_sys_reg64(int fd, uint64_t *val,
> +                           uint8_t op0, uint8_t op1,
> +                           uint8_t crn, uint8_t crm, uint8_t op2);
> +
>  /**
>   * kvm_arm_add_vcpu_properties:
>   * @cpu: The CPU object to add the properties to
Thanks

Eric