Hi Richard, > On 27 Feb 2023, at 18:24, Richard Henderson <richard.hender...@linaro.org> > wrote: > > On 2/27/23 06:37, Miguel Luis wrote: >> From: Haibo Xu <haibo...@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...@linaro.org/ >> Signed-off-by: Haibo Xu <haibo...@linaro.org> >> [Miguel Luis: use of ID_AA64PFR0 for cpu features] >> Signed-off-by: Miguel Luis <miguel.l...@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. > >
Thank you for the explanation. I will take it into consideration on the next version. Miguel > r~
