[PATCH v2 0/3] arm64: Support systems without FP/ASIMD

[PATCH v2 0/3] arm64: Support systems without FP/ASIMD

[Suzuki K Poulose]

This series adds supports to the kernel and KVM hyp to handle
systems without FP/ASIMD properly. At the moment the kernel
doesn't check if the FP unit is available before accessing
the registers (e.g during context switch). Also for KVM,
we trap the FP/ASIMD accesses and handle it by injecting an
undefined instruction into the VM on systems without FP.

Tested on a FVP_Base-AEM-v8A model by disabling VFP on at
least one CPU ( -C clusterX.cpuY.vfp-present=0 ).

Applies on aarch64 : for-next/core


Suzuki K Poulose (3):
  arm64: crypto/aes-ce-ccm: Cleanup hwcap check
  arm64: Add hypervisor safe helper for checking constant capabilities
  arm64: Support systems without FP/ASIMD

 arch/arm64/crypto/aes-ce-ccm-glue.c |  5 ++---
 arch/arm64/include/asm/cpufeature.h | 24 ++++++++++++++++++++----
 arch/arm64/include/asm/neon.h       |  3 ++-
 arch/arm64/kernel/cpufeature.c      | 15 +++++++++++++++
 arch/arm64/kernel/fpsimd.c          | 14 ++++++++++++++
 arch/arm64/kvm/handle_exit.c        | 11 +++++++++++
 arch/arm64/kvm/hyp/hyp-entry.S      |  9 ++++++++-
 arch/arm64/kvm/hyp/switch.c         |  5 ++++-
 8 files changed, 76 insertions(+), 10 deletions(-)

-- 
2.7.4

[PATCH v2 1/3] arm64: crypto/aes-ce-ccm: Cleanup hwcap check

[Suzuki K Poulose]

Use the module_cpu_feature_match to make sure the system has
HWCAP_AES to use the module.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 arch/arm64/crypto/aes-ce-ccm-glue.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/crypto/aes-ce-ccm-glue.c b/arch/arm64/crypto/aes-ce-ccm-glue.c
index f4bf2f2..fa82eaa 100644
--- a/arch/arm64/crypto/aes-ce-ccm-glue.c
+++ b/arch/arm64/crypto/aes-ce-ccm-glue.c
@@ -14,6 +14,7 @@
 #include <crypto/algapi.h>
 #include <crypto/scatterwalk.h>
 #include <crypto/internal/aead.h>
+#include <linux/cpufeature.h>
 #include <linux/module.h>
 
 #include "aes-ce-setkey.h"
@@ -296,8 +297,6 @@ static struct aead_alg ccm_aes_alg = {
 
 static int __init aes_mod_init(void)
 {
-	if (!(elf_hwcap & HWCAP_AES))
-		return -ENODEV;
 	return crypto_register_aead(&ccm_aes_alg);
 }
 
@@ -306,7 +305,7 @@ static void __exit aes_mod_exit(void)
 	crypto_unregister_aead(&ccm_aes_alg);
 }
 
-module_init(aes_mod_init);
+module_cpu_feature_match(AES, aes_mod_init);
 module_exit(aes_mod_exit);
 
 MODULE_DESCRIPTION("Synchronous AES in CCM mode using ARMv8 Crypto Extensions");
-- 
2.7.4

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

[Suzuki K Poulose]

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

[PATCH v2 3/3] arm64: Support systems without FP/ASIMD

[Suzuki K Poulose]

The arm64 kernel assumes that FP/ASIMD units are always present
and accesses the FP/ASIMD specific registers unconditionally. This
could cause problems when they are absent. This patch adds the
support for kernel handling systems without FP/ASIMD by skipping the
register access within the kernel. For kvm, we trap the accesses
to FP/ASIMD and inject an undefined instruction exception to the VM.

The callers of the exported kernel_neon_begin_parital() should
make sure that the FP/ASIMD is supported.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Christoffer Dall <christoffer.dall@linaro.org>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
Changes since V1:
 - Dropped explicit FP/ASIMD check from Crypto modules.
---
 arch/arm64/include/asm/cpufeature.h |  8 +++++++-
 arch/arm64/include/asm/neon.h       |  3 ++-
 arch/arm64/kernel/cpufeature.c      | 15 +++++++++++++++
 arch/arm64/kernel/fpsimd.c          | 14 ++++++++++++++
 arch/arm64/kvm/handle_exit.c        | 11 +++++++++++
 arch/arm64/kvm/hyp/hyp-entry.S      |  9 ++++++++-
 arch/arm64/kvm/hyp/switch.c         |  5 ++++-
 7 files changed, 61 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
index ae5e994..63d739c 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -38,8 +38,9 @@
 #define ARM64_HAS_32BIT_EL0			13
 #define ARM64_HYP_OFFSET_LOW			14
 #define ARM64_MISMATCHED_CACHE_LINE_SIZE	15
+#define ARM64_HAS_NO_FPSIMD			16
 
-#define ARM64_NCAPS				16
+#define ARM64_NCAPS				17
 
 #ifndef __ASSEMBLY__
 
@@ -236,6 +237,11 @@ static inline bool system_supports_mixed_endian_el0(void)
 	return id_aa64mmfr0_mixed_endian_el0(read_system_reg(SYS_ID_AA64MMFR0_EL1));
 }
 
+static inline bool system_supports_fpsimd(void)
+{
+	return !cpus_have_const_cap(ARM64_HAS_NO_FPSIMD);
+}
+
 #endif /* __ASSEMBLY__ */
 
 #endif
diff --git a/arch/arm64/include/asm/neon.h b/arch/arm64/include/asm/neon.h
index 13ce4cc..ad4cdc9 100644
--- a/arch/arm64/include/asm/neon.h
+++ b/arch/arm64/include/asm/neon.h
@@ -9,8 +9,9 @@
  */
 
 #include <linux/types.h>
+#include <asm/fpsimd.h>
 
-#define cpu_has_neon()		(1)
+#define cpu_has_neon()		system_supports_fpsimd()
 
 #define kernel_neon_begin()	kernel_neon_begin_partial(32)
 
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index d577f26..8f22544 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -744,6 +744,14 @@ static bool hyp_offset_low(const struct arm64_cpu_capabilities *entry,
 	return idmap_addr > GENMASK(VA_BITS - 2, 0) && !is_kernel_in_hyp_mode();
 }
 
+static bool has_no_fpsimd(const struct arm64_cpu_capabilities *entry, int __unused)
+{
+	u64 pfr0 = read_system_reg(SYS_ID_AA64PFR0_EL1);
+
+	return cpuid_feature_extract_signed_field(pfr0,
+					ID_AA64PFR0_FP_SHIFT) < 0;
+}
+
 static const struct arm64_cpu_capabilities arm64_features[] = {
 	{
 		.desc = "GIC system register CPU interface",
@@ -827,6 +835,13 @@ static const struct arm64_cpu_capabilities arm64_features[] = {
 		.def_scope = SCOPE_SYSTEM,
 		.matches = hyp_offset_low,
 	},
+	{
+		/* FP/SIMD is not implemented */
+		.capability = ARM64_HAS_NO_FPSIMD,
+		.def_scope = SCOPE_SYSTEM,
+		.min_field_value = 0,
+		.matches = has_no_fpsimd,
+	},
 	{},
 };
 
diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index 975b274..80da5aa 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -127,6 +127,8 @@ void do_fpsimd_exc(unsigned int esr, struct pt_regs *regs)
 
 void fpsimd_thread_switch(struct task_struct *next)
 {
+	if (!system_supports_fpsimd())
+		return;
 	/*
 	 * Save the current FPSIMD state to memory, but only if whatever is in
 	 * the registers is in fact the most recent userland FPSIMD state of
@@ -157,6 +159,8 @@ void fpsimd_thread_switch(struct task_struct *next)
 
 void fpsimd_flush_thread(void)
 {
+	if (!system_supports_fpsimd())
+		return;
 	memset(&current->thread.fpsimd_state, 0, sizeof(struct fpsimd_state));
 	fpsimd_flush_task_state(current);
 	set_thread_flag(TIF_FOREIGN_FPSTATE);
@@ -168,6 +172,8 @@ void fpsimd_flush_thread(void)
  */
 void fpsimd_preserve_current_state(void)
 {
+	if (!system_supports_fpsimd())
+		return;
 	preempt_disable();
 	if (!test_thread_flag(TIF_FOREIGN_FPSTATE))
 		fpsimd_save_state(&current->thread.fpsimd_state);
@@ -181,6 +187,8 @@ void fpsimd_preserve_current_state(void)
  */
 void fpsimd_restore_current_state(void)
 {
+	if (!system_supports_fpsimd())
+		return;
 	preempt_disable();
 	if (test_and_clear_thread_flag(TIF_FOREIGN_FPSTATE)) {
 		struct fpsimd_state *st = &current->thread.fpsimd_state;
@@ -199,6 +207,8 @@ void fpsimd_restore_current_state(void)
  */
 void fpsimd_update_current_state(struct fpsimd_state *state)
 {
+	if (!system_supports_fpsimd())
+		return;
 	preempt_disable();
 	fpsimd_load_state(state);
 	if (test_and_clear_thread_flag(TIF_FOREIGN_FPSTATE)) {
@@ -228,6 +238,8 @@ static DEFINE_PER_CPU(struct fpsimd_partial_state, softirq_fpsimdstate);
  */
 void kernel_neon_begin_partial(u32 num_regs)
 {
+	if (WARN_ON(!system_supports_fpsimd()))
+		return;
 	if (in_interrupt()) {
 		struct fpsimd_partial_state *s = this_cpu_ptr(
 			in_irq() ? &hardirq_fpsimdstate : &softirq_fpsimdstate);
@@ -252,6 +264,8 @@ EXPORT_SYMBOL(kernel_neon_begin_partial);
 
 void kernel_neon_end(void)
 {
+	if (!system_supports_fpsimd())
+		return;
 	if (in_interrupt()) {
 		struct fpsimd_partial_state *s = this_cpu_ptr(
 			in_irq() ? &hardirq_fpsimdstate : &softirq_fpsimdstate);
diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
index fa96fe2..39e055a 100644
--- a/arch/arm64/kvm/handle_exit.c
+++ b/arch/arm64/kvm/handle_exit.c
@@ -57,6 +57,16 @@ static int handle_smc(struct kvm_vcpu *vcpu, struct kvm_run *run)
 	return 1;
 }
 
+/*
+ * Guest access to FP/ASIMD registers are routed to this handler only
+ * when the system doesn't support FP/ASIMD.
+ */
+static int handle_no_fpsimd(struct kvm_vcpu *vcpu, struct kvm_run *run)
+{
+	kvm_inject_undefined(vcpu);
+	return 1;
+}
+
 /**
  * kvm_handle_wfx - handle a wait-for-interrupts or wait-for-event
  *		    instruction executed by a guest
@@ -144,6 +154,7 @@ static exit_handle_fn arm_exit_handlers[] = {
 	[ESR_ELx_EC_BREAKPT_LOW]= kvm_handle_guest_debug,
 	[ESR_ELx_EC_BKPT32]	= kvm_handle_guest_debug,
 	[ESR_ELx_EC_BRK64]	= kvm_handle_guest_debug,
+	[ESR_ELx_EC_FP_ASIMD]	= handle_no_fpsimd,
 };
 
 static exit_handle_fn kvm_get_exit_handler(struct kvm_vcpu *vcpu)
diff --git a/arch/arm64/kvm/hyp/hyp-entry.S b/arch/arm64/kvm/hyp/hyp-entry.S
index f6d9694..16167d7 100644
--- a/arch/arm64/kvm/hyp/hyp-entry.S
+++ b/arch/arm64/kvm/hyp/hyp-entry.S
@@ -117,9 +117,16 @@ el1_trap:
 	 * x2: ESR_EC
 	 */
 
-	/* Guest accessed VFP/SIMD registers, save host, restore Guest */
+	/*
+	 * We trap the first access to the FP/SIMD to save the host context
+	 * and restore the guest context lazily.
+	 * If FP/SIMD is not implemented, handle the trap and inject an
+	 * undefined instruction exception to the guest.
+	 */
+alternative_if_not ARM64_HAS_NO_FPSIMD
 	cmp	x2, #ESR_ELx_EC_FP_ASIMD
 	b.eq	__fpsimd_guest_restore
+alternative_else_nop_endif
 
 	mrs	x0, tpidr_el2
 	mov	x1, #ARM_EXCEPTION_TRAP
diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
index ae7855f..f2d0c4f 100644
--- a/arch/arm64/kvm/hyp/switch.c
+++ b/arch/arm64/kvm/hyp/switch.c
@@ -18,6 +18,7 @@
 #include <linux/types.h>
 #include <asm/kvm_asm.h>
 #include <asm/kvm_hyp.h>
+#include <asm/fpsimd.h>
 
 static bool __hyp_text __fpsimd_enabled_nvhe(void)
 {
@@ -73,9 +74,11 @@ static void __hyp_text __activate_traps(struct kvm_vcpu *vcpu)
 	 * traps are only taken to EL2 if the operation would not otherwise
 	 * trap to EL1.  Therefore, always make sure that for 32-bit guests,
 	 * we set FPEXC.EN to prevent traps to EL1, when setting the TFP bit.
+	 * If FP/ASIMD is not implemented, FPEXC is UNDEFINED and any access to
+	 * it will cause an exception.
 	 */
 	val = vcpu->arch.hcr_el2;
-	if (!(val & HCR_RW)) {
+	if (!(val & HCR_RW) && system_supports_fpsimd()) {
 		write_sysreg(1 << 30, fpexc32_el2);
 		isb();
 	}
-- 
2.7.4

[PATCH v2 1/3] arm64: crypto/aes-ce-ccm: Cleanup hwcap check

[Ard Biesheuvel]

Hi Suzuki,

On 31 October 2016 at 16:03, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
> Use the module_cpu_feature_match to make sure the system has
> HWCAP_AES to use the module.
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
>  arch/arm64/crypto/aes-ce-ccm-glue.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm64/crypto/aes-ce-ccm-glue.c b/arch/arm64/crypto/aes-ce-ccm-glue.c
> index f4bf2f2..fa82eaa 100644
> --- a/arch/arm64/crypto/aes-ce-ccm-glue.c
> +++ b/arch/arm64/crypto/aes-ce-ccm-glue.c
> @@ -14,6 +14,7 @@
>  #include <crypto/algapi.h>
>  #include <crypto/scatterwalk.h>
>  #include <crypto/internal/aead.h>
> +#include <linux/cpufeature.h>
>  #include <linux/module.h>
>
>  #include "aes-ce-setkey.h"
> @@ -296,8 +297,6 @@ static struct aead_alg ccm_aes_alg = {
>
>  static int __init aes_mod_init(void)
>  {
> -       if (!(elf_hwcap & HWCAP_AES))
> -               return -ENODEV;
>         return crypto_register_aead(&ccm_aes_alg);
>  }
>
> @@ -306,7 +305,7 @@ static void __exit aes_mod_exit(void)
>         crypto_unregister_aead(&ccm_aes_alg);
>  }
>
> -module_init(aes_mod_init);
> +module_cpu_feature_match(AES, aes_mod_init);

I don't think this change is correct. This will result in the AES
instruction dependency to be exposed via the module alias, causing the
module to be loaded automatically as soon as udev detects that the CPU
implements those instructions. For plain AES, that makes sense, but
AES in CCM mode is specific to CCMP (WPA2) on mac80211 controllers
that have no hardware AES support, and to IPsec VPN. For this reason,
the algo type is exposed via the module alias instead (i.e,
'ccm(aes)'), which will result in the module being loaded as soon as
the crypto algo manager instantiates the transform. On CPUs that don't
implement the AES instructions, this will fail, and it will fall back
to the generic CCM driver instead.

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

[Will Deacon]

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

[PATCH v2 1/3] arm64: crypto/aes-ce-ccm: Cleanup hwcap check

[Suzuki K Poulose]

On 01/11/16 14:03, Ard Biesheuvel wrote:
> Hi Suzuki,
>
> On 31 October 2016 at 16:03, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
>> Use the module_cpu_feature_match to make sure the system has
>> HWCAP_AES to use the module.
>>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>> ---
>>  arch/arm64/crypto/aes-ce-ccm-glue.c | 5 ++---
>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm64/crypto/aes-ce-ccm-glue.c b/arch/arm64/crypto/aes-ce-ccm-glue.c
>> index f4bf2f2..fa82eaa 100644
>> --- a/arch/arm64/crypto/aes-ce-ccm-glue.c
>> +++ b/arch/arm64/crypto/aes-ce-ccm-glue.c
...
>> -module_init(aes_mod_init);
>> +module_cpu_feature_match(AES, aes_mod_init);
>
> I don't think this change is correct. This will result in the AES
> instruction dependency to be exposed via the module alias, causing the
> module to be loaded automatically as soon as udev detects that the CPU
> implements those instructions. For plain AES, that makes sense, but
> AES in CCM mode is specific to CCMP (WPA2) on mac80211 controllers
> that have no hardware AES support, and to IPsec VPN. For this reason,
> the algo type is exposed via the module alias instead (i.e,
> 'ccm(aes)'), which will result in the module being loaded as soon as
> the crypto algo manager instantiates the transform. On CPUs that don't
> implement the AES instructions, this will fail, and it will fall back
> to the generic CCM driver instead.

Ah, thanks for the explanation. I will drop it.

Cheers
Suzuki

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

[Suzuki K Poulose]

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