| Message ID | 20230227163718.62003-5-miguel.luis@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | QEMU v7.2.0 aarch64 Nested Virtualization Support | expand |
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~
Hi Richard, > On 27 Feb 2023, at 18:24, Richard Henderson <richard.henderson@linaro.org> wrote: > > 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. > > Thank you for the explanation. I will take it into consideration on the next version. Miguel > r~
Hi Miguel, On 2/27/23 17: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; > } > > static inline bool isar_feature_aa64_ras(const ARMISARegisters *id) > diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c > index be8144a2b5..f7ebd731aa 100644 > --- a/target/arm/kvm64.c > +++ b/target/arm/kvm64.c > @@ -505,6 +505,7 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf) > */ > int fdarray[3]; > bool sve_supported; > + bool el2_supported; > bool pmu_supported = false; > uint64_t features = 0; > int err; > @@ -535,6 +536,14 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf) > init.features[0] |= 1 << KVM_ARM_VCPU_SVE; > } > > + /* > + * Ask for EL2 if supported. > + */ > + el2_supported = kvm_arm_el2_supported(); > + if (el2_supported) { > + init.features[0] |= 1 << KVM_ARM_VCPU_HAS_EL2; This doesn't work if your host both supports SVE & NV. The error output by qemu is not straightforward qemu-system-aarch64: can't apply global host-arm-cpu.sve=off: Property 'host-arm-cpu.sve' not found The problem is that we create a scratch VM with a CPU featuring both SVE and NV and this fails at kernel level, I think on vcpu reset. The trouble is that we do that even if sve=off at qemu level. So I think this is a more generic issue related to the way we validate host cpu features. Thanks Eric > + } > + > /* > * Ask for Pointer Authentication if supported, so that we get > * the unsanitized field values for AA64ISAR1_EL1. > @@ -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; > + } > + > ahcf->features = features; > > return true; > @@ -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; > + } > if (cpu_isar_feature(aa64_pauth, cpu)) { > cpu->kvm_init_features[0] |= (1 << KVM_ARM_VCPU_PTRAUTH_ADDRESS | > 1 << KVM_ARM_VCPU_PTRAUTH_GENERIC);
Hi Eric, Thanks in advance for your comment. > On 6 Jul 2023, at 08:16, Eric Auger <eauger@redhat.com> wrote: > > Hi Miguel, > > On 2/27/23 17: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; >> } >> >> static inline bool isar_feature_aa64_ras(const ARMISARegisters *id) >> diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c >> index be8144a2b5..f7ebd731aa 100644 >> --- a/target/arm/kvm64.c >> +++ b/target/arm/kvm64.c >> @@ -505,6 +505,7 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf) >> */ >> int fdarray[3]; >> bool sve_supported; >> + bool el2_supported; >> bool pmu_supported = false; >> uint64_t features = 0; >> int err; >> @@ -535,6 +536,14 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf) >> init.features[0] |= 1 << KVM_ARM_VCPU_SVE; >> } >> >> + /* >> + * Ask for EL2 if supported. >> + */ >> + el2_supported = kvm_arm_el2_supported(); >> + if (el2_supported) { >> + init.features[0] |= 1 << KVM_ARM_VCPU_HAS_EL2; > This doesn't work if your host both supports SVE & NV. > May I ask if this prevented your L1 or L2 guest from booting? I’ve addressed all the previous comments on the thread for the new RFC version and this topic is what I’m currently addressing. So far the L1 guest booted successfully but not the L2 guest. > The error output by qemu is not straightforward > > qemu-system-aarch64: can't apply global host-arm-cpu.sve=off: Property > 'host-arm-cpu.sve' not found > > The problem is that we create a scratch VM with a CPU featuring both SVE > and NV and this fails at kernel level, I think on vcpu reset. > > The trouble is that we do that even if sve=off at qemu level. So I think > this is a more generic issue related to the way we validate host cpu > features. > OK, I’m having a look into that. Thank you Miguel > Thanks > > Eric > > >> + } >> + >> /* >> * Ask for Pointer Authentication if supported, so that we get >> * the unsanitized field values for AA64ISAR1_EL1. >> @@ -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; >> + } >> + >> ahcf->features = features; >> >> return true; >> @@ -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; >> + } >> if (cpu_isar_feature(aa64_pauth, cpu)) { >> cpu->kvm_init_features[0] |= (1 << KVM_ARM_VCPU_PTRAUTH_ADDRESS | >> 1 << KVM_ARM_VCPU_PTRAUTH_GENERIC);
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; } static inline bool isar_feature_aa64_ras(const ARMISARegisters *id) diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c index be8144a2b5..f7ebd731aa 100644 --- a/target/arm/kvm64.c +++ b/target/arm/kvm64.c @@ -505,6 +505,7 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf) */ int fdarray[3]; bool sve_supported; + bool el2_supported; bool pmu_supported = false; uint64_t features = 0; int err; @@ -535,6 +536,14 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf) init.features[0] |= 1 << KVM_ARM_VCPU_SVE; } + /* + * Ask for EL2 if supported. + */ + el2_supported = kvm_arm_el2_supported(); + if (el2_supported) { + init.features[0] |= 1 << KVM_ARM_VCPU_HAS_EL2; + } + /* * Ask for Pointer Authentication if supported, so that we get * the unsanitized field values for AA64ISAR1_EL1. @@ -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; + } + ahcf->features = features; return true; @@ -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; + } if (cpu_isar_feature(aa64_pauth, cpu)) { cpu->kvm_init_features[0] |= (1 << KVM_ARM_VCPU_PTRAUTH_ADDRESS | 1 << KVM_ARM_VCPU_PTRAUTH_GENERIC);