Re: [PATCH v2 02/14] arm/kvm: add accessors for storing host features into idregs

[Richard Henderson <richard.henderson@linaro.org>]

On 3/5/25 08:38, Cornelia Huck wrote:
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
>   target/arm/cpu-sysregs.h |  3 +++
>   target/arm/cpu64.c       | 25 +++++++++++++++++++++++++
>   target/arm/kvm.c         | 12 ++++++++++++
>   3 files changed, 40 insertions(+)
> 
> diff --git a/target/arm/cpu-sysregs.h b/target/arm/cpu-sysregs.h
> index de09ebae91a5..54a4fadbf0c1 100644
> --- a/target/arm/cpu-sysregs.h
> +++ b/target/arm/cpu-sysregs.h
> @@ -128,4 +128,7 @@ static const uint32_t id_register_sysreg[NUM_ID_IDX] = {
>       [CTR_EL0_IDX] = SYS_CTR_EL0,
>   };
>   
> +int get_sysreg_idx(ARMSysRegs sysreg);
> +uint64_t idregs_sysreg_to_kvm_reg(ARMSysRegs sysreg);
> +
>   #endif /* ARM_CPU_SYSREGS_H */
> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> index 8188ede5cc8a..9ae78253cb34 100644
> --- a/target/arm/cpu64.c
> +++ b/target/arm/cpu64.c
> @@ -736,6 +736,31 @@ static void aarch64_a53_initfn(Object *obj)
>       define_cortex_a72_a57_a53_cp_reginfo(cpu);
>   }
>   
> +#ifdef CONFIG_KVM
> +
> +int get_sysreg_idx(ARMSysRegs sysreg)
> +{
> +    int i;
> +
> +    for (i = 0; i < NUM_ID_IDX; i++) {
> +        if (id_register_sysreg[i] == sysreg) {
> +            return i;
> +        }
> +    }
> +    return -1;
> +}
> +
> +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,
> +                         (sysreg & CP_REG_ARM64_SYSREG_OP1_MASK) >> CP_REG_ARM64_SYSREG_OP1_SHIFT,
> +                         (sysreg & CP_REG_ARM64_SYSREG_CRN_MASK) >> CP_REG_ARM64_SYSREG_CRN_SHIFT,
> +                         (sysreg & CP_REG_ARM64_SYSREG_CRM_MASK) >> CP_REG_ARM64_SYSREG_CRM_SHIFT,
> +                         (sysreg & CP_REG_ARM64_SYSREG_OP2_MASK) >> CP_REG_ARM64_SYSREG_OP2_SHIFT);
> +}
> +
> +#endif

Why are these here, with an ifdef, instead of in kvm.c?

Rather than a loop over an array, you could do

#define DEF(NAME, OP0, OP1, CRN, CRM, OP2) \
     case SYS_##NAME: return NAME##_IDX;

int get_sysreg_idx(ARMSysRegs sysreg)
{
     switch (sysreg) {
#include "cpu-sysregs.h.inc"
     }
     g_assert_not_reached();
}

#undef DEF

> diff --git a/target/arm/kvm.c b/target/arm/kvm.c
> index da30bdbb2349..2381c87e4ba1 100644
> --- a/target/arm/kvm.c
> +++ b/target/arm/kvm.c
> @@ -246,6 +246,18 @@ static bool kvm_arm_pauth_supported(void)
>               kvm_check_extension(kvm_state, KVM_CAP_ARM_PTRAUTH_GENERIC));
>   }
>   
> +/* read a sysreg value and store it in the idregs */
> +static int get_host_cpu_reg(int fd, ARMHostCPUFeatures *ahcf, ARMIDRegisterIdx index)
> +{
> +    uint64_t *reg;
> +    int ret;
> +
> +    reg = &ahcf->isar.idregs[index];
> +    ret = read_sys_reg64(fd, reg,
> +                         idregs_sysreg_to_kvm_reg(id_register_sysreg[index]));
> +    return ret;
> +}

Surely this patch doesn't compile by itself,
because this will Werror for the unused function.


r~