| Message ID | 20231120131027.854038-2-maz@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Nested Virtualization support (FEAT_NV2 only) | expand |
On 20-11-2023 06:39 pm, Marc Zyngier wrote: > To anyone who has played with FEAT_NV, it is obvious that the level > of performance is rather low due to the trap amplification that it > imposes on the host hypervisor. FEAT_NV2 solves a number of the > problems that FEAT_NV had. > > It also turns out that all the existing hardware that has FEAT_NV > also has FEAT_NV2. Finally, it is now allowed by the architecture > to build FEAT_NV2 *only* (as denoted by ID_AA64MMFR4_EL1.NV_frac), > which effectively seals the fate of FEAT_NV. > > Restrict the NV support to NV2, and be done with it. Nobody will > cry over the old crap. > > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > arch/arm64/kernel/cpufeature.c | 22 +++++++++++++++------- > arch/arm64/tools/cpucaps | 2 ++ > 2 files changed, 17 insertions(+), 7 deletions(-) > > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c > index 7dcda39537f8..95a677cf8c04 100644 > --- a/arch/arm64/kernel/cpufeature.c > +++ b/arch/arm64/kernel/cpufeature.c > @@ -439,6 +439,7 @@ static const struct arm64_ftr_bits ftr_id_aa64mmfr3[] = { > > static const struct arm64_ftr_bits ftr_id_aa64mmfr4[] = { > S_ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64MMFR4_EL1_E2H0_SHIFT, 4, 0), > + ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_HIGHER_SAFE, ID_AA64MMFR4_EL1_NV_frac_SHIFT, 4, 0), > ARM64_FTR_END, > }; > > @@ -2080,12 +2081,8 @@ static bool has_nested_virt_support(const struct arm64_cpu_capabilities *cap, > if (kvm_get_mode() != KVM_MODE_NV) > return false; > > - if (!has_cpuid_feature(cap, scope)) { > - pr_warn("unavailable: %s\n", cap->desc); > - return false; > - } > - > - return true; > + return (__system_matches_cap(ARM64_HAS_NV2) | > + __system_matches_cap(ARM64_HAS_NV2_ONLY)); This seems to be typo and should it be logical OR? > } > > static bool hvhe_possible(const struct arm64_cpu_capabilities *entry, > @@ -2391,12 +2388,23 @@ static const struct arm64_cpu_capabilities arm64_features[] = { > .matches = runs_at_el2, > .cpu_enable = cpu_copy_el2regs, > }, > + { > + .capability = ARM64_HAS_NV2, > + .type = ARM64_CPUCAP_SYSTEM_FEATURE, > + .matches = has_cpuid_feature, > + ARM64_CPUID_FIELDS(ID_AA64MMFR2_EL1, NV, NV2) > + }, > + { > + .capability = ARM64_HAS_NV2_ONLY, > + .type = ARM64_CPUCAP_SYSTEM_FEATURE, > + .matches = has_cpuid_feature, > + ARM64_CPUID_FIELDS(ID_AA64MMFR4_EL1, NV_frac, NV2_ONLY) > + }, > { > .desc = "Nested Virtualization Support", > .capability = ARM64_HAS_NESTED_VIRT, > .type = ARM64_CPUCAP_SYSTEM_FEATURE, > .matches = has_nested_virt_support, > - ARM64_CPUID_FIELDS(ID_AA64MMFR2_EL1, NV, IMP) Since only NV2 is supported, is it more appropriate to have description as "Enhanced Nested Virtualization Support"? > }, > { > .capability = ARM64_HAS_32BIT_EL0_DO_NOT_USE, > diff --git a/arch/arm64/tools/cpucaps b/arch/arm64/tools/cpucaps > index fea24bcd6252..480de648cd03 100644 > --- a/arch/arm64/tools/cpucaps > +++ b/arch/arm64/tools/cpucaps > @@ -41,6 +41,8 @@ HAS_LSE_ATOMICS > HAS_MOPS > HAS_NESTED_VIRT > HAS_NO_HW_PREFETCH > +HAS_NV2 > +HAS_NV2_ONLY > HAS_PAN > HAS_S1PIE > HAS_RAS_EXTN Thanks, Ganapat
On Tue, 21 Nov 2023 09:07:30 +0000, Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote: > > > > On 20-11-2023 06:39 pm, Marc Zyngier wrote: > > To anyone who has played with FEAT_NV, it is obvious that the level > > of performance is rather low due to the trap amplification that it > > imposes on the host hypervisor. FEAT_NV2 solves a number of the > > problems that FEAT_NV had. > > > > It also turns out that all the existing hardware that has FEAT_NV > > also has FEAT_NV2. Finally, it is now allowed by the architecture > > to build FEAT_NV2 *only* (as denoted by ID_AA64MMFR4_EL1.NV_frac), > > which effectively seals the fate of FEAT_NV. > > > > Restrict the NV support to NV2, and be done with it. Nobody will > > cry over the old crap. > > > > Signed-off-by: Marc Zyngier <maz@kernel.org> > > --- > > arch/arm64/kernel/cpufeature.c | 22 +++++++++++++++------- > > arch/arm64/tools/cpucaps | 2 ++ > > 2 files changed, 17 insertions(+), 7 deletions(-) > > > > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c > > index 7dcda39537f8..95a677cf8c04 100644 > > --- a/arch/arm64/kernel/cpufeature.c > > +++ b/arch/arm64/kernel/cpufeature.c > > @@ -439,6 +439,7 @@ static const struct arm64_ftr_bits ftr_id_aa64mmfr3[] = { > > static const struct arm64_ftr_bits ftr_id_aa64mmfr4[] = { > > S_ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64MMFR4_EL1_E2H0_SHIFT, 4, 0), > > + ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_HIGHER_SAFE, ID_AA64MMFR4_EL1_NV_frac_SHIFT, 4, 0), > > ARM64_FTR_END, > > }; > > @@ -2080,12 +2081,8 @@ static bool has_nested_virt_support(const > > struct arm64_cpu_capabilities *cap, > > if (kvm_get_mode() != KVM_MODE_NV) > > return false; > > - if (!has_cpuid_feature(cap, scope)) { > > - pr_warn("unavailable: %s\n", cap->desc); > > - return false; > > - } > > - > > - return true; > > + return (__system_matches_cap(ARM64_HAS_NV2) | > > + __system_matches_cap(ARM64_HAS_NV2_ONLY)); > > This seems to be typo and should it be logical OR? Indeed, this is a bug. Not that it will have any effect as __system_matches_cap() returns a bool, so | and || are strictly equivalent. Worth addressing though. > > > } > > static bool hvhe_possible(const struct arm64_cpu_capabilities > > *entry, > > @@ -2391,12 +2388,23 @@ static const struct arm64_cpu_capabilities arm64_features[] = { > > .matches = runs_at_el2, > > .cpu_enable = cpu_copy_el2regs, > > }, > > + { > > + .capability = ARM64_HAS_NV2, > > + .type = ARM64_CPUCAP_SYSTEM_FEATURE, > > + .matches = has_cpuid_feature, > > + ARM64_CPUID_FIELDS(ID_AA64MMFR2_EL1, NV, NV2) > > + }, > > + { > > + .capability = ARM64_HAS_NV2_ONLY, > > + .type = ARM64_CPUCAP_SYSTEM_FEATURE, > > + .matches = has_cpuid_feature, > > + ARM64_CPUID_FIELDS(ID_AA64MMFR4_EL1, NV_frac, NV2_ONLY) > > + }, > > { > > .desc = "Nested Virtualization Support", > > .capability = ARM64_HAS_NESTED_VIRT, > > .type = ARM64_CPUCAP_SYSTEM_FEATURE, > > .matches = has_nested_virt_support, > > - ARM64_CPUID_FIELDS(ID_AA64MMFR2_EL1, NV, IMP) > > Since only NV2 is supported, is it more appropriate to have > description as "Enhanced Nested Virtualization Support"? Nah. There is nothing 'enhanced' about NV2. It is NV that should have been named "Unusable Nested Virt"... So I'm perfectly happy to leave it as is. And to be honest, I'd rather we display FEAT_* rather than some interpretation of it, but I'm not going to repaint cpufeature.c. M.
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c index 7dcda39537f8..95a677cf8c04 100644 --- a/arch/arm64/kernel/cpufeature.c +++ b/arch/arm64/kernel/cpufeature.c @@ -439,6 +439,7 @@ static const struct arm64_ftr_bits ftr_id_aa64mmfr3[] = { static const struct arm64_ftr_bits ftr_id_aa64mmfr4[] = { S_ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64MMFR4_EL1_E2H0_SHIFT, 4, 0), + ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_HIGHER_SAFE, ID_AA64MMFR4_EL1_NV_frac_SHIFT, 4, 0), ARM64_FTR_END, }; @@ -2080,12 +2081,8 @@ static bool has_nested_virt_support(const struct arm64_cpu_capabilities *cap, if (kvm_get_mode() != KVM_MODE_NV) return false; - if (!has_cpuid_feature(cap, scope)) { - pr_warn("unavailable: %s\n", cap->desc); - return false; - } - - return true; + return (__system_matches_cap(ARM64_HAS_NV2) | + __system_matches_cap(ARM64_HAS_NV2_ONLY)); } static bool hvhe_possible(const struct arm64_cpu_capabilities *entry, @@ -2391,12 +2388,23 @@ static const struct arm64_cpu_capabilities arm64_features[] = { .matches = runs_at_el2, .cpu_enable = cpu_copy_el2regs, }, + { + .capability = ARM64_HAS_NV2, + .type = ARM64_CPUCAP_SYSTEM_FEATURE, + .matches = has_cpuid_feature, + ARM64_CPUID_FIELDS(ID_AA64MMFR2_EL1, NV, NV2) + }, + { + .capability = ARM64_HAS_NV2_ONLY, + .type = ARM64_CPUCAP_SYSTEM_FEATURE, + .matches = has_cpuid_feature, + ARM64_CPUID_FIELDS(ID_AA64MMFR4_EL1, NV_frac, NV2_ONLY) + }, { .desc = "Nested Virtualization Support", .capability = ARM64_HAS_NESTED_VIRT, .type = ARM64_CPUCAP_SYSTEM_FEATURE, .matches = has_nested_virt_support, - ARM64_CPUID_FIELDS(ID_AA64MMFR2_EL1, NV, IMP) }, { .capability = ARM64_HAS_32BIT_EL0_DO_NOT_USE, diff --git a/arch/arm64/tools/cpucaps b/arch/arm64/tools/cpucaps index fea24bcd6252..480de648cd03 100644 --- a/arch/arm64/tools/cpucaps +++ b/arch/arm64/tools/cpucaps @@ -41,6 +41,8 @@ HAS_LSE_ATOMICS HAS_MOPS HAS_NESTED_VIRT HAS_NO_HW_PREFETCH +HAS_NV2 +HAS_NV2_ONLY HAS_PAN HAS_S1PIE HAS_RAS_EXTN
To anyone who has played with FEAT_NV, it is obvious that the level of performance is rather low due to the trap amplification that it imposes on the host hypervisor. FEAT_NV2 solves a number of the problems that FEAT_NV had. It also turns out that all the existing hardware that has FEAT_NV also has FEAT_NV2. Finally, it is now allowed by the architecture to build FEAT_NV2 *only* (as denoted by ID_AA64MMFR4_EL1.NV_frac), which effectively seals the fate of FEAT_NV. Restrict the NV support to NV2, and be done with it. Nobody will cry over the old crap. Signed-off-by: Marc Zyngier <maz@kernel.org> --- arch/arm64/kernel/cpufeature.c | 22 +++++++++++++++------- arch/arm64/tools/cpucaps | 2 ++ 2 files changed, 17 insertions(+), 7 deletions(-)