Re: [PATCH v4 01/17] arm64: cpufeature: Fix meta-capability cpufeature check

From: Suzuki K Poulose <suzuki.poulose@arm.com>
To: amit.kachhap@arm.com, linux-arm-kernel@lists.infradead.org
Cc: mark.rutland@arm.com, keescook@chromium.org,
	catalin.marinas@arm.com, kristina.martsenko@arm.com,
	dave.martin@arm.com, broonie@kernel.org, james.morse@arm.com,
	ramana.radhakrishnan@arm.com, Vincenzo.Frascino@arm.com,
	will@kernel.org, ardb@kernel.org
Subject: Re: [PATCH v4 01/17] arm64: cpufeature: Fix meta-capability cpufeature check
Date: Thu, 23 Jan 2020 23:07:16 +0000	[thread overview]
Message-ID: <13e3aa5c-a7e8-69be-3bbe-006d0c43fb8b@arm.com> (raw)
In-Reply-To: <1579774839-19562-2-git-send-email-amit.kachhap@arm.com>

Hi Amit

On 01/23/2020 10:20 AM, Amit Daniel Kachhap wrote:
> Some existing meta cpucaps match needs the presence of individual
> cpucaps. Currently the individual cpucaps checks it via an array
> based flag and this introduces dependency on the array entry order.
> This limitation exists only for system scope cpufeature.
> 
> This patch introduces a helper function (system_has_cap) to invoke
> the matching handler for system scope. This is similar to the
> existing helper function (this_cpu_has_cap) for local cpu scope.

It is similar (in name, see more on that below) but this must be limited
to very narrow "window" where :
  - We have the system wide safe registers set with all the SMP CPUs
   *and*
  - The SYSTEM_FEATURE cpu_hwcaps may not have been set.

The normal users must continue to use the existing
cpus_have_{const_}cap() helper(s).

> 
> Use this new helper function for an existing meta cpucaps.
> 
> Suggested-by: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com>
> ---
> Changes since v3:
>   * New patch.
>   * This is added as suggested by Catalin [1] to avoid the array entry order
>     dependency.
>   * A new helper system_has_cap is added.
> 
> [1]: http://lists.infradead.org/pipermail/linux-arm-kernel/2020-January/705845.html
> 
>   arch/arm64/include/asm/cpufeature.h |  1 +
>   arch/arm64/kernel/cpufeature.c      | 13 ++++++++++++-
>   2 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
> index 4261d55..695e538 100644
> --- a/arch/arm64/include/asm/cpufeature.h
> +++ b/arch/arm64/include/asm/cpufeature.h
> @@ -382,6 +382,7 @@ extern DECLARE_BITMAP(boot_capabilities, ARM64_NPATCHABLE);
>   	for_each_set_bit(cap, cpu_hwcaps, ARM64_NCAPS)
>   
>   bool this_cpu_has_cap(unsigned int cap);
> +bool system_has_cap(unsigned int cap);

You don't need this.

>   void cpu_set_feature(unsigned int num);
>   bool cpu_have_feature(unsigned int num);
>   unsigned long cpu_get_elf_hwcap(void);
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index 04cf64e..74d6c13 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -2008,6 +2008,17 @@ bool this_cpu_has_cap(unsigned int n)
>   	return false;
>   }
>   
> +bool system_has_cap(unsigned int n)
> +{

nit: This could be made static for now.

More importantly, the name is very much similar to "cap" specific
wrappers, e.g, system_has_fpsimd() etc, which could create a confusion
and make people use this instead of the "cpus_have_cap() or 
cpus_have_const_cap()". It would be a good idea to make this explicitly
look like an internal helper.
e.g:

__system_matches_cap() to avoid abusing this helper.

Otherwise looks good to me.

Suzuki

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