Re: [PATCH v11 01/43] arm64: cpufeatures: Restrict NV support to FEAT_NV2

[Marc Zyngier <maz@kernel.org>]

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.

-- 
Without deviation from the norm, progress is not possible.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel