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

[Ganapatrao Kulkarni <gankulkarni@xxxxxxxxxxxxxxxxxxxxxx>]

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@xxxxxxxxxx>
> ---
>   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