Re: [RFC PATCH 4/5] target/arm: enable feature ARM_FEATURE_EL2 if EL2 is supported

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

On 2/27/23 06:37, Miguel Luis wrote:
> From: Haibo Xu <haibo.xu@linaro.org>
> 
> KVM_CAP_ARM_EL2 must be supported by the cpu to enable ARM_FEATURE_EL2.
> EL2 bits on ID_AA64PFR0 state unsupported on the value 0b0000.
> 
> Ref: https://lore.kernel.org/qemu-devel/b7c2626e6c720ccc43e57197dff3dac72d613640.1616052890.git.haibo.xu@linaro.org/
> 
> Signed-off-by: Haibo Xu <haibo.xu@linaro.org>
> [Miguel Luis: use of ID_AA64PFR0 for cpu features]
> Signed-off-by: Miguel Luis <miguel.luis@oracle.com>
> ---
>   target/arm/cpu.h   |  2 +-
>   target/arm/kvm64.c | 16 ++++++++++++++++
>   2 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 9aeed3c848..de2a88b43e 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -3961,7 +3961,7 @@ static inline bool isar_feature_aa64_aa32_el1(const ARMISARegisters *id)
>   
>   static inline bool isar_feature_aa64_aa32_el2(const ARMISARegisters *id)
>   {
> -    return FIELD_EX64(id->id_aa64pfr0, ID_AA64PFR0, EL2) >= 2;
> +    return FIELD_EX64(id->id_aa64pfr0, ID_AA64PFR0, EL2) != 0;
>   }

No, this predicate is testing if EL2 supports AArch32 more.

> @@ -714,6 +723,10 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
>       features |= 1ULL << ARM_FEATURE_PMU;
>       features |= 1ULL << ARM_FEATURE_GENERIC_TIMER;
>   
> +    if (el2_supported) {
> +        features |= 1ULL << ARM_FEATURE_EL2;
> +    }

This is the test you want...

> @@ -881,6 +894,9 @@ int kvm_arch_init_vcpu(CPUState *cs)
>           assert(kvm_arm_sve_supported());
>           cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_SVE;
>       }
> +    if (cpu_isar_feature(aa64_aa32_el2, cpu)) {
> +        cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_HAS_EL2;
> +    }

... here.

While you could add a new isar_feature predicate for EL2 supported in AArch64 mode, the 
feature test is equivalent and good enough, and is more obviously tied to the required KVM 
support.


r~