Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1512656 > unrolled thread

[PATCH v2 2/3] arm64: Add hypervisor safe helper for checking constant capabilities

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 2/3] arm64: Add hypervisor safe helper for checking constant capabilities Suzuki K Poulose <suzuki.poulose@arm.com> - 2016-10-31 17:10 +0100
    Re: [PATCH v2 2/3] arm64: Add hypervisor safe helper for checking  constant capabilities Will Deacon <will.deacon@arm.com> - 2016-11-01 15:10 +0100
      Re: [PATCH v2 2/3] arm64: Add hypervisor safe helper for checking  constant capabilities Suzuki K Poulose <Suzuki.Poulose@arm.com> - 2016-11-01 15:40 +0100

#1512656 — [PATCH v2 2/3] arm64: Add hypervisor safe helper for checking constant capabilities

The hypervisor may not have full access to the kernel data structures
and hence cannot safely use cpus_have_cap() helper for checking the
system capability. Add a safe helper for hypervisors to check a constant
system capability, which *doesn't* fall back to checking the bitmap
maintained by the kernel.

Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 arch/arm64/include/asm/cpufeature.h | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
index 758d74f..ae5e994 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -9,8 +9,6 @@
 #ifndef __ASM_CPUFEATURE_H
 #define __ASM_CPUFEATURE_H
 
-#include <linux/jump_label.h>
-
 #include <asm/hwcap.h>
 #include <asm/sysreg.h>
 
@@ -45,6 +43,8 @@
 
 #ifndef __ASSEMBLY__
 
+#include <linux/bug.h>
+#include <linux/jump_label.h>
 #include <linux/kernel.h>
 
 /* CPU feature register tracking */
@@ -122,6 +122,16 @@ static inline bool cpu_have_feature(unsigned int num)
 	return elf_hwcap & (1UL << num);
 }
 
+/* System capability check for constant caps */
+static inline bool cpus_have_const_cap(int num)
+{
+	if (__builtin_constant_p(num))
+		return static_branch_unlikely(&cpu_hwcap_keys[num]);
+	BUILD_BUG();
+	/* unreachable */
+	return false;
+}
+
 static inline bool cpus_have_cap(unsigned int num)
 {
 	if (num >= ARM64_NCAPS)
@@ -218,7 +228,7 @@ static inline bool cpu_supports_mixed_endian_el0(void)
 
 static inline bool system_supports_32bit_el0(void)
 {
-	return cpus_have_cap(ARM64_HAS_32BIT_EL0);
+	return cpus_have_const_cap(ARM64_HAS_32BIT_EL0);
 }
 
 static inline bool system_supports_mixed_endian_el0(void)
-- 
2.7.4

[toc] | [next] | [standalone]

#1513279 — Re: [PATCH v2 2/3] arm64: Add hypervisor safe helper for checking constant capabilities

On Mon, Oct 31, 2016 at 04:03:44PM +0000, Suzuki K Poulose wrote:
> The hypervisor may not have full access to the kernel data structures
> and hence cannot safely use cpus_have_cap() helper for checking the
> system capability. Add a safe helper for hypervisors to check a constant
> system capability, which *doesn't* fall back to checking the bitmap
> maintained by the kernel.
> 
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
>  arch/arm64/include/asm/cpufeature.h | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
> index 758d74f..ae5e994 100644
> --- a/arch/arm64/include/asm/cpufeature.h
> +++ b/arch/arm64/include/asm/cpufeature.h
> @@ -9,8 +9,6 @@
>  #ifndef __ASM_CPUFEATURE_H
>  #define __ASM_CPUFEATURE_H
>  
> -#include <linux/jump_label.h>
> -
>  #include <asm/hwcap.h>
>  #include <asm/sysreg.h>
>  
> @@ -45,6 +43,8 @@
>  
>  #ifndef __ASSEMBLY__
>  
> +#include <linux/bug.h>
> +#include <linux/jump_label.h>
>  #include <linux/kernel.h>
>  
>  /* CPU feature register tracking */
> @@ -122,6 +122,16 @@ static inline bool cpu_have_feature(unsigned int num)
>  	return elf_hwcap & (1UL << num);
>  }
>  
> +/* System capability check for constant caps */
> +static inline bool cpus_have_const_cap(int num)
> +{
> +	if (__builtin_constant_p(num))
> +		return static_branch_unlikely(&cpu_hwcap_keys[num]);
> +	BUILD_BUG();

I think you'll already get a build failure if you pass a non-const num
to static_branch_unlikely, so this seems unnecessary. Furthermore, if
we're going to introduce a "const-only" version of this function, maybe
it's best to kill the __builtin_constant_p checks altogether, including
in the existing cpus_have_cap code? That way, the caller can make the
decision about whether or not they want to use static keys.

> +	/* unreachable */
> +	return false;
> +}
> +
>  static inline bool cpus_have_cap(unsigned int num)
>  {
>  	if (num >= ARM64_NCAPS)

It seems odd to check aginst ARM64_NCAPS here, but not in your new function.
Is the check actually necessary in either case? If so, we should probably
duplicate it for consistency.

Will

[toc] | [prev] | [next] | [standalone]

#1513295 — Re: [PATCH v2 2/3] arm64: Add hypervisor safe helper for checking constant capabilities

On 01/11/16 14:03, Will Deacon wrote:
> On Mon, Oct 31, 2016 at 04:03:44PM +0000, Suzuki K Poulose wrote:
>> The hypervisor may not have full access to the kernel data structures
>> and hence cannot safely use cpus_have_cap() helper for checking the
>> system capability. Add a safe helper for hypervisors to check a constant
>> system capability, which *doesn't* fall back to checking the bitmap
>> maintained by the kernel.
>>
>>
>> +/* System capability check for constant caps */
>> +static inline bool cpus_have_const_cap(int num)
>> +{
>> +	if (__builtin_constant_p(num))
>> +		return static_branch_unlikely(&cpu_hwcap_keys[num]);
>> +	BUILD_BUG();
>
> I think you'll already get a build failure if you pass a non-const num
> to static_branch_unlikely, so this seems unnecessary. Furthermore, if
> we're going to introduce a "const-only" version of this function, maybe
> it's best to kill the __builtin_constant_p checks altogether, including
> in the existing cpus_have_cap code? That way, the caller can make the
> decision about whether or not they want to use static keys.

I didn't think that non-const to  static_branch_* would trigger a build
failure. Thanks for that hint. Yes, with that we can safely kill the builtin
checks and define the const version to simply use the static keys.

>
>> +	/* unreachable */
>> +	return false;
>> +}
>> +
>>  static inline bool cpus_have_cap(unsigned int num)
>>  {
>>  	if (num >= ARM64_NCAPS)
>
> It seems odd to check aginst ARM64_NCAPS here, but not in your new function.
> Is the check actually necessary in either case? If so, we should probably
> duplicate it for consistency.

Thanks for catching that. It is needed to make sure we don't access beyond the
size of the bitmask (and the static key array). So it is required. I will fix
those.

Suzuki

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web