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

[Cornelia Huck <cohuck@redhat.com>]

On Wed, Mar 05 2025, Richard Henderson <richard.henderson@linaro.org> wrote:

> 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

Ok, now I see where you're going with the DEF(...) magic.

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

Hm, I'm pretty positive that I did intermediate compiles while sorting
the patches here... let me see if I can improve this.