[PATCH v2 0/7] arm64: Fix support for no FP/SIMD

[PATCH v2 0/7] arm64: Fix support for no FP/SIMD

[Suzuki K Poulose]

This series fixes the support for systems without FP/SIMD unit.

We detect the absence of FP/SIMD after the SMP cpus are brought
online (i.e, SYSTEM scope). This means, we allow a hotplugged
CPU to boot successfully even if it doesn't have the FP/SIMD
when we have decided otherwise at boot and have now advertised
the ELF HWCAP for the userspace. Fix this by turning this to a
BOOT_RESTRICTED_CPU_LOCAL feature to allow the detection of the
feature the very moment a CPU turns up without FP/SIMD and also
prevent a conflict after SMP boot.

The COMPAT ELF_HWCAPs were statically set to indicate the
availability of VFP. Make it dynamic to set the appropriate
bits.

Also, some of the early kernel threads (including init) could run
with their TIF_FOREIGN_FPSTATE flag set which might be inherited
by applications forked by them (e.g, modprobe from initramfs).
Now, if we detect the absence of FP/SIMD we stop clearing the
TIF flag in fpsimd_restore_current_state(). This could cause
the applications stuck in do_notify_resume() looping forever
to clear the flag. Fix this by clearing the TIF flag in
fpsimd_restore_current_state() for the tasks that may
have it set.

This series also categorises the functions dealing with fpsimd
into two :

 - Call permitted with missing FP/SIMD support. But we bail
   out early in the function. This is for functions exposed
   to the generic kernel code.

 - Calls not permitted with missing FP/SIMD support. These
   are functions which deal with the CPU/Task FP/SIMD registers
   and/or meta-data. The callers must check for the support
   before invoking them.

See the last patch in the series for details. 

Also make sure that the SVE is initialised where supported,
before the FP/SIMD is used by the kernel.

Tested with debian armel initramfs and rootfs. The arm64 doesn't
have a soft-float ABI, thus haven't tested it with 64bit userspace.

Applies on v5.5-rc2.

Suzuki K Poulose (7):
  arm64: Introduce system_capabilities_finalized() marker
  arm64: fpsimd: Make sure SVE setup is complete before SIMD is used
  arm64: cpufeature: Fix the type of no FP/SIMD capability
  arm64: cpufeature: Set the FP/SIMD compat HWCAP bits properly
  arm64: ptrace: nofpsimd: Fail FP/SIMD regset operations
  arm64: signal: nofpsimd: Handle fp/simd context for signal frames
  arm64: nofpsmid: Handle TIF_FOREIGN_FPSTATE flag cleanly

 arch/arm64/include/asm/cpufeature.h |  5 +++
 arch/arm64/include/asm/kvm_host.h   |  2 +-
 arch/arm64/include/asm/mmu.h        |  2 +-
 arch/arm64/include/asm/simd.h       |  8 +++-
 arch/arm64/kernel/cpufeature.c      | 65 +++++++++++++++++++----------
 arch/arm64/kernel/fpsimd.c          | 32 ++++++++++++--
 arch/arm64/kernel/process.c         |  2 +-
 arch/arm64/kernel/ptrace.c          | 12 ++++++
 arch/arm64/kernel/signal.c          | 17 +++++++-
 arch/arm64/kernel/signal32.c        | 12 +++++-
 arch/arm64/kvm/hyp/switch.c         |  9 ++++
 11 files changed, 132 insertions(+), 34 deletions(-)

-- 
2.23.0

[PATCH v2 0/7] arm64: Fix support for no FP/SIMD

[Suzuki K Poulose]

This series fixes the support for systems without FP/SIMD unit.

We detect the absence of FP/SIMD after the SMP cpus are brought
online (i.e, SYSTEM scope). This means, we allow a hotplugged
CPU to boot successfully even if it doesn't have the FP/SIMD
when we have decided otherwise at boot and have now advertised
the ELF HWCAP for the userspace. Fix this by turning this to a
BOOT_RESTRICTED_CPU_LOCAL feature to allow the detection of the
feature the very moment a CPU turns up without FP/SIMD and also
prevent a conflict after SMP boot.

The COMPAT ELF_HWCAPs were statically set to indicate the
availability of VFP. Make it dynamic to set the appropriate
bits.

Also, some of the early kernel threads (including init) could run
with their TIF_FOREIGN_FPSTATE flag set which might be inherited
by applications forked by them (e.g, modprobe from initramfs).
Now, if we detect the absence of FP/SIMD we stop clearing the
TIF flag in fpsimd_restore_current_state(). This could cause
the applications stuck in do_notify_resume() looping forever
to clear the flag. Fix this by clearing the TIF flag in
fpsimd_restore_current_state() for the tasks that may
have it set.

This series also categorises the functions dealing with fpsimd
into two :

 - Call permitted with missing FP/SIMD support. But we bail
   out early in the function. This is for functions exposed
   to the generic kernel code.

 - Calls not permitted with missing FP/SIMD support. These
   are functions which deal with the CPU/Task FP/SIMD registers
   and/or meta-data. The callers must check for the support
   before invoking them.

See the last patch in the series for details. 

Also make sure that the SVE is initialised where supported,
before the FP/SIMD is used by the kernel.

Tested with debian armel initramfs and rootfs. The arm64 doesn't
have a soft-float ABI, thus haven't tested it with 64bit userspace.

Applies on v5.5-rc2.

Suzuki K Poulose (7):
  arm64: Introduce system_capabilities_finalized() marker
  arm64: fpsimd: Make sure SVE setup is complete before SIMD is used
  arm64: cpufeature: Fix the type of no FP/SIMD capability
  arm64: cpufeature: Set the FP/SIMD compat HWCAP bits properly
  arm64: ptrace: nofpsimd: Fail FP/SIMD regset operations
  arm64: signal: nofpsimd: Handle fp/simd context for signal frames
  arm64: nofpsmid: Handle TIF_FOREIGN_FPSTATE flag cleanly

 arch/arm64/include/asm/cpufeature.h |  5 +++
 arch/arm64/include/asm/kvm_host.h   |  2 +-
 arch/arm64/include/asm/mmu.h        |  2 +-
 arch/arm64/include/asm/simd.h       |  8 +++-
 arch/arm64/kernel/cpufeature.c      | 65 +++++++++++++++++++----------
 arch/arm64/kernel/fpsimd.c          | 32 ++++++++++++--
 arch/arm64/kernel/process.c         |  2 +-
 arch/arm64/kernel/ptrace.c          | 12 ++++++
 arch/arm64/kernel/signal.c          | 17 +++++++-
 arch/arm64/kernel/signal32.c        | 12 +++++-
 arch/arm64/kvm/hyp/switch.c         |  9 ++++
 11 files changed, 132 insertions(+), 34 deletions(-)

-- 
2.23.0


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

[PATCH v2 1/7] arm64: Introduce system_capabilities_finalized() marker

[Suzuki K Poulose]

We finalize the system wide capabilities after the SMP CPUs
are booted by the kernel. This is used as a marker for deciding
various checks in the kernel. e.g, sanity check the hotplugged
CPUs for missing mandatory features.

However there is no explicit helper available for this in the
kernel. There is sys_caps_initialised, which is not exposed.
The other closest one we have is the jump_label arm64_const_caps_ready
which denotes that the capabilities are set and the capability checks
could use the individual jump_labels for fast path. This is
performed before setting the ELF Hwcaps, which must be checked
against the new CPUs. We also perform some of the other initialization
e.g, SVE setup, which is important for the use of FP/SIMD
where SVE is supported. Normally userspace doesn't get to run
before we finish this. However the in-kernel users may
potentially start using the neon mode. So, we need to
reject uses of neon mode before we are set. Instead of defining
a new marker for the completion of SVE setup, we could simply
reuse the arm64_const_caps_ready and enable it once we have
finished all the setup. Also we could expose this to the
various users as "system_capabilities_finalized()" to make
it more meaningful than "const_caps_ready".

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 arch/arm64/include/asm/cpufeature.h |  5 +++++
 arch/arm64/include/asm/kvm_host.h   |  2 +-
 arch/arm64/include/asm/mmu.h        |  2 +-
 arch/arm64/kernel/cpufeature.c      | 26 +++++++++-----------------
 arch/arm64/kernel/process.c         |  2 +-
 5 files changed, 17 insertions(+), 20 deletions(-)

diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
index 4261d55e8506..92ef9539874a 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -613,6 +613,11 @@ static inline bool system_has_prio_mask_debugging(void)
 	       system_uses_irq_prio_masking();
 }
 
+static inline bool system_capabilities_finalized(void)
+{
+	return static_branch_likely(&arm64_const_caps_ready);
+}
+
 #define ARM64_BP_HARDEN_UNKNOWN		-1
 #define ARM64_BP_HARDEN_WA_NEEDED	0
 #define ARM64_BP_HARDEN_NOT_REQUIRED	1
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index c61260cf63c5..48ce54639eb5 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -547,7 +547,7 @@ static inline void __cpu_init_hyp_mode(phys_addr_t pgd_ptr,
 	 * wrong, and hyp will crash and burn when it uses any
 	 * cpus_have_const_cap() wrapper.
 	 */
-	BUG_ON(!static_branch_likely(&arm64_const_caps_ready));
+	BUG_ON(!system_capabilities_finalized());
 	__kvm_call_hyp((void *)pgd_ptr, hyp_stack_ptr, vector_ptr, tpidr_el2);
 
 	/*
diff --git a/arch/arm64/include/asm/mmu.h b/arch/arm64/include/asm/mmu.h
index f217e3292919..691ee7cfd521 100644
--- a/arch/arm64/include/asm/mmu.h
+++ b/arch/arm64/include/asm/mmu.h
@@ -64,7 +64,7 @@ static inline bool arm64_kernel_use_ng_mappings(void)
 	if (!IS_ENABLED(CONFIG_CAVIUM_ERRATUM_27456)) {
 		tx1_bug = false;
 #ifndef MODULE
-	} else if (!static_branch_likely(&arm64_const_caps_ready)) {
+	} else if (!system_capabilities_finalized()) {
 		extern const struct midr_range cavium_erratum_27456_cpus[];
 
 		tx1_bug = is_midr_in_range_list(read_cpuid_id(),
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 04cf64e9f0c9..d25ad65bfac2 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -53,13 +53,14 @@ DECLARE_BITMAP(boot_capabilities, ARM64_NPATCHABLE);
  * will be used to determine if a new booting CPU should
  * go through the verification process to make sure that it
  * supports the system capabilities, without using a hotplug
- * notifier.
+ * notifier. This is also used to decide if we could use
+ * the fast path for checking constant CPU caps.
  */
-static bool sys_caps_initialised;
-
-static inline void set_sys_caps_initialised(void)
+DEFINE_STATIC_KEY_FALSE(arm64_const_caps_ready);
+EXPORT_SYMBOL(arm64_const_caps_ready);
+static inline void finalize_system_capabilities(void)
 {
-	sys_caps_initialised = true;
+	static_branch_enable(&arm64_const_caps_ready);
 }
 
 static int dump_cpu_hwcaps(struct notifier_block *self, unsigned long v, void *p)
@@ -785,7 +786,7 @@ void update_cpu_features(int cpu,
 
 		/* Probe vector lengths, unless we already gave up on SVE */
 		if (id_aa64pfr0_sve(read_sanitised_ftr_reg(SYS_ID_AA64PFR0_EL1)) &&
-		    !sys_caps_initialised)
+		    !system_capabilities_finalized())
 			sve_update_vq_map();
 	}
 
@@ -1974,7 +1975,7 @@ void check_local_cpu_capabilities(void)
 	 * Otherwise, this CPU should verify that it has all the system
 	 * advertised capabilities.
 	 */
-	if (!sys_caps_initialised)
+	if (!system_capabilities_finalized())
 		update_cpu_capabilities(SCOPE_LOCAL_CPU);
 	else
 		verify_local_cpu_capabilities();
@@ -1988,14 +1989,6 @@ static void __init setup_boot_cpu_capabilities(void)
 	enable_cpu_capabilities(SCOPE_BOOT_CPU);
 }
 
-DEFINE_STATIC_KEY_FALSE(arm64_const_caps_ready);
-EXPORT_SYMBOL(arm64_const_caps_ready);
-
-static void __init mark_const_caps_ready(void)
-{
-	static_branch_enable(&arm64_const_caps_ready);
-}
-
 bool this_cpu_has_cap(unsigned int n)
 {
 	if (!WARN_ON(preemptible()) && n < ARM64_NCAPS) {
@@ -2054,7 +2047,6 @@ void __init setup_cpu_features(void)
 	u32 cwg;
 
 	setup_system_capabilities();
-	mark_const_caps_ready();
 	setup_elf_hwcaps(arm64_elf_hwcaps);
 
 	if (system_supports_32bit_el0())
@@ -2067,7 +2059,7 @@ void __init setup_cpu_features(void)
 	minsigstksz_setup();
 
 	/* Advertise that we have computed the system capabilities */
-	set_sys_caps_initialised();
+	finalize_system_capabilities();
 
 	/*
 	 * Check for sane CTR_EL0.CWG value.
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index 71f788cd2b18..48a38144ea7b 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -646,6 +646,6 @@ asmlinkage void __sched arm64_preempt_schedule_irq(void)
 	 * Only allow a task to be preempted once cpufeatures have been
 	 * enabled.
 	 */
-	if (static_branch_likely(&arm64_const_caps_ready))
+	if (system_capabilities_finalized())
 		preempt_schedule_irq();
 }
-- 
2.23.0

[PATCH v2 1/7] arm64: Introduce system_capabilities_finalized() marker

[Suzuki K Poulose]

We finalize the system wide capabilities after the SMP CPUs
are booted by the kernel. This is used as a marker for deciding
various checks in the kernel. e.g, sanity check the hotplugged
CPUs for missing mandatory features.

However there is no explicit helper available for this in the
kernel. There is sys_caps_initialised, which is not exposed.
The other closest one we have is the jump_label arm64_const_caps_ready
which denotes that the capabilities are set and the capability checks
could use the individual jump_labels for fast path. This is
performed before setting the ELF Hwcaps, which must be checked
against the new CPUs. We also perform some of the other initialization
e.g, SVE setup, which is important for the use of FP/SIMD
where SVE is supported. Normally userspace doesn't get to run
before we finish this. However the in-kernel users may
potentially start using the neon mode. So, we need to
reject uses of neon mode before we are set. Instead of defining
a new marker for the completion of SVE setup, we could simply
reuse the arm64_const_caps_ready and enable it once we have
finished all the setup. Also we could expose this to the
various users as "system_capabilities_finalized()" to make
it more meaningful than "const_caps_ready".

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 arch/arm64/include/asm/cpufeature.h |  5 +++++
 arch/arm64/include/asm/kvm_host.h   |  2 +-
 arch/arm64/include/asm/mmu.h        |  2 +-
 arch/arm64/kernel/cpufeature.c      | 26 +++++++++-----------------
 arch/arm64/kernel/process.c         |  2 +-
 5 files changed, 17 insertions(+), 20 deletions(-)

diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
index 4261d55e8506..92ef9539874a 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -613,6 +613,11 @@ static inline bool system_has_prio_mask_debugging(void)
 	       system_uses_irq_prio_masking();
 }
 
+static inline bool system_capabilities_finalized(void)
+{
+	return static_branch_likely(&arm64_const_caps_ready);
+}
+
 #define ARM64_BP_HARDEN_UNKNOWN		-1
 #define ARM64_BP_HARDEN_WA_NEEDED	0
 #define ARM64_BP_HARDEN_NOT_REQUIRED	1
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index c61260cf63c5..48ce54639eb5 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -547,7 +547,7 @@ static inline void __cpu_init_hyp_mode(phys_addr_t pgd_ptr,
 	 * wrong, and hyp will crash and burn when it uses any
 	 * cpus_have_const_cap() wrapper.
 	 */
-	BUG_ON(!static_branch_likely(&arm64_const_caps_ready));
+	BUG_ON(!system_capabilities_finalized());
 	__kvm_call_hyp((void *)pgd_ptr, hyp_stack_ptr, vector_ptr, tpidr_el2);
 
 	/*
diff --git a/arch/arm64/include/asm/mmu.h b/arch/arm64/include/asm/mmu.h
index f217e3292919..691ee7cfd521 100644
--- a/arch/arm64/include/asm/mmu.h
+++ b/arch/arm64/include/asm/mmu.h
@@ -64,7 +64,7 @@ static inline bool arm64_kernel_use_ng_mappings(void)
 	if (!IS_ENABLED(CONFIG_CAVIUM_ERRATUM_27456)) {
 		tx1_bug = false;
 #ifndef MODULE
-	} else if (!static_branch_likely(&arm64_const_caps_ready)) {
+	} else if (!system_capabilities_finalized()) {
 		extern const struct midr_range cavium_erratum_27456_cpus[];
 
 		tx1_bug = is_midr_in_range_list(read_cpuid_id(),
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 04cf64e9f0c9..d25ad65bfac2 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -53,13 +53,14 @@ DECLARE_BITMAP(boot_capabilities, ARM64_NPATCHABLE);
  * will be used to determine if a new booting CPU should
  * go through the verification process to make sure that it
  * supports the system capabilities, without using a hotplug
- * notifier.
+ * notifier. This is also used to decide if we could use
+ * the fast path for checking constant CPU caps.
  */
-static bool sys_caps_initialised;
-
-static inline void set_sys_caps_initialised(void)
+DEFINE_STATIC_KEY_FALSE(arm64_const_caps_ready);
+EXPORT_SYMBOL(arm64_const_caps_ready);
+static inline void finalize_system_capabilities(void)
 {
-	sys_caps_initialised = true;
+	static_branch_enable(&arm64_const_caps_ready);
 }
 
 static int dump_cpu_hwcaps(struct notifier_block *self, unsigned long v, void *p)
@@ -785,7 +786,7 @@ void update_cpu_features(int cpu,
 
 		/* Probe vector lengths, unless we already gave up on SVE */
 		if (id_aa64pfr0_sve(read_sanitised_ftr_reg(SYS_ID_AA64PFR0_EL1)) &&
-		    !sys_caps_initialised)
+		    !system_capabilities_finalized())
 			sve_update_vq_map();
 	}
 
@@ -1974,7 +1975,7 @@ void check_local_cpu_capabilities(void)
 	 * Otherwise, this CPU should verify that it has all the system
 	 * advertised capabilities.
 	 */
-	if (!sys_caps_initialised)
+	if (!system_capabilities_finalized())
 		update_cpu_capabilities(SCOPE_LOCAL_CPU);
 	else
 		verify_local_cpu_capabilities();
@@ -1988,14 +1989,6 @@ static void __init setup_boot_cpu_capabilities(void)
 	enable_cpu_capabilities(SCOPE_BOOT_CPU);
 }
 
-DEFINE_STATIC_KEY_FALSE(arm64_const_caps_ready);
-EXPORT_SYMBOL(arm64_const_caps_ready);
-
-static void __init mark_const_caps_ready(void)
-{
-	static_branch_enable(&arm64_const_caps_ready);
-}
-
 bool this_cpu_has_cap(unsigned int n)
 {
 	if (!WARN_ON(preemptible()) && n < ARM64_NCAPS) {
@@ -2054,7 +2047,6 @@ void __init setup_cpu_features(void)
 	u32 cwg;
 
 	setup_system_capabilities();
-	mark_const_caps_ready();
 	setup_elf_hwcaps(arm64_elf_hwcaps);
 
 	if (system_supports_32bit_el0())
@@ -2067,7 +2059,7 @@ void __init setup_cpu_features(void)
 	minsigstksz_setup();
 
 	/* Advertise that we have computed the system capabilities */
-	set_sys_caps_initialised();
+	finalize_system_capabilities();
 
 	/*
 	 * Check for sane CTR_EL0.CWG value.
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index 71f788cd2b18..48a38144ea7b 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -646,6 +646,6 @@ asmlinkage void __sched arm64_preempt_schedule_irq(void)
 	 * Only allow a task to be preempted once cpufeatures have been
 	 * enabled.
 	 */
-	if (static_branch_likely(&arm64_const_caps_ready))
+	if (system_capabilities_finalized())
 		preempt_schedule_irq();
 }
-- 
2.23.0


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

Re: [PATCH v2 1/7] arm64: Introduce system_capabilities_finalized() marker

[Catalin Marinas]

On Tue, Dec 17, 2019 at 06:33:56PM +0000, Suzuki K Poulose wrote:
> We finalize the system wide capabilities after the SMP CPUs
> are booted by the kernel. This is used as a marker for deciding
> various checks in the kernel. e.g, sanity check the hotplugged
> CPUs for missing mandatory features.
> 
> However there is no explicit helper available for this in the
> kernel. There is sys_caps_initialised, which is not exposed.
> The other closest one we have is the jump_label arm64_const_caps_ready
> which denotes that the capabilities are set and the capability checks
> could use the individual jump_labels for fast path. This is
> performed before setting the ELF Hwcaps, which must be checked
> against the new CPUs. We also perform some of the other initialization
> e.g, SVE setup, which is important for the use of FP/SIMD
> where SVE is supported. Normally userspace doesn't get to run
> before we finish this. However the in-kernel users may
> potentially start using the neon mode. So, we need to
> reject uses of neon mode before we are set. Instead of defining
> a new marker for the completion of SVE setup, we could simply
> reuse the arm64_const_caps_ready and enable it once we have
> finished all the setup. Also we could expose this to the
> various users as "system_capabilities_finalized()" to make
> it more meaningful than "const_caps_ready".
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>

Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

Re: [PATCH v2 1/7] arm64: Introduce system_capabilities_finalized() marker

[Catalin Marinas]

On Tue, Dec 17, 2019 at 06:33:56PM +0000, Suzuki K Poulose wrote:
> We finalize the system wide capabilities after the SMP CPUs
> are booted by the kernel. This is used as a marker for deciding
> various checks in the kernel. e.g, sanity check the hotplugged
> CPUs for missing mandatory features.
> 
> However there is no explicit helper available for this in the
> kernel. There is sys_caps_initialised, which is not exposed.
> The other closest one we have is the jump_label arm64_const_caps_ready
> which denotes that the capabilities are set and the capability checks
> could use the individual jump_labels for fast path. This is
> performed before setting the ELF Hwcaps, which must be checked
> against the new CPUs. We also perform some of the other initialization
> e.g, SVE setup, which is important for the use of FP/SIMD
> where SVE is supported. Normally userspace doesn't get to run
> before we finish this. However the in-kernel users may
> potentially start using the neon mode. So, we need to
> reject uses of neon mode before we are set. Instead of defining
> a new marker for the completion of SVE setup, we could simply
> reuse the arm64_const_caps_ready and enable it once we have
> finished all the setup. Also we could expose this to the
> various users as "system_capabilities_finalized()" to make
> it more meaningful than "const_caps_ready".
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>

Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

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

[PATCH v2 2/7] arm64: fpsimd: Make sure SVE setup is complete before SIMD is used

[Suzuki K Poulose]

In-kernel users of NEON rely on may_use_simd() to check if the SIMD
can be used. However, we must initialize the SVE before SIMD can
be used. Add a sanity check to make sure that we have completed the
SVE setup before anyone uses the SIMD.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
Discussion here : https://lkml.kernel.org/r/20191014145204.GS27757@arm.com
---
 arch/arm64/include/asm/simd.h | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/simd.h b/arch/arm64/include/asm/simd.h
index 7434844036d3..afce6409be51 100644
--- a/arch/arm64/include/asm/simd.h
+++ b/arch/arm64/include/asm/simd.h
@@ -26,6 +26,8 @@ DECLARE_PER_CPU(bool, fpsimd_context_busy);
 static __must_check inline bool may_use_simd(void)
 {
 	/*
+	 * We must make sure that the SVE has been initialized properly
+	 * before using the SIMD in kernel.
 	 * fpsimd_context_busy is only set while preemption is disabled,
 	 * and is clear whenever preemption is enabled. Since
 	 * this_cpu_read() is atomic w.r.t. preemption, fpsimd_context_busy
@@ -33,8 +35,10 @@ static __must_check inline bool may_use_simd(void)
 	 * migrated, and if it's clear we cannot be migrated to a CPU
 	 * where it is set.
 	 */
-	return !in_irq() && !irqs_disabled() && !in_nmi() &&
-		!this_cpu_read(fpsimd_context_busy);
+	return system_capabilities_finalized() &&
+	       system_supports_fpsimd() &&
+	       !in_irq() && !irqs_disabled() && !in_nmi() &&
+	       !this_cpu_read(fpsimd_context_busy);
 }
 
 #else /* ! CONFIG_KERNEL_MODE_NEON */
-- 
2.23.0

[PATCH v2 2/7] arm64: fpsimd: Make sure SVE setup is complete before SIMD is used

[Suzuki K Poulose]

In-kernel users of NEON rely on may_use_simd() to check if the SIMD
can be used. However, we must initialize the SVE before SIMD can
be used. Add a sanity check to make sure that we have completed the
SVE setup before anyone uses the SIMD.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
Discussion here : https://lkml.kernel.org/r/20191014145204.GS27757@arm.com
---
 arch/arm64/include/asm/simd.h | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/simd.h b/arch/arm64/include/asm/simd.h
index 7434844036d3..afce6409be51 100644
--- a/arch/arm64/include/asm/simd.h
+++ b/arch/arm64/include/asm/simd.h
@@ -26,6 +26,8 @@ DECLARE_PER_CPU(bool, fpsimd_context_busy);
 static __must_check inline bool may_use_simd(void)
 {
 	/*
+	 * We must make sure that the SVE has been initialized properly
+	 * before using the SIMD in kernel.
 	 * fpsimd_context_busy is only set while preemption is disabled,
 	 * and is clear whenever preemption is enabled. Since
 	 * this_cpu_read() is atomic w.r.t. preemption, fpsimd_context_busy
@@ -33,8 +35,10 @@ static __must_check inline bool may_use_simd(void)
 	 * migrated, and if it's clear we cannot be migrated to a CPU
 	 * where it is set.
 	 */
-	return !in_irq() && !irqs_disabled() && !in_nmi() &&
-		!this_cpu_read(fpsimd_context_busy);
+	return system_capabilities_finalized() &&
+	       system_supports_fpsimd() &&
+	       !in_irq() && !irqs_disabled() && !in_nmi() &&
+	       !this_cpu_read(fpsimd_context_busy);
 }
 
 #else /* ! CONFIG_KERNEL_MODE_NEON */
-- 
2.23.0


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

Re: [PATCH v2 2/7] arm64: fpsimd: Make sure SVE setup is complete before SIMD is used

[Catalin Marinas]

On Tue, Dec 17, 2019 at 06:33:57PM +0000, Suzuki K Poulose wrote:
> In-kernel users of NEON rely on may_use_simd() to check if the SIMD
> can be used. However, we must initialize the SVE before SIMD can
> be used. Add a sanity check to make sure that we have completed the
> SVE setup before anyone uses the SIMD.
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
> Discussion here : https://lkml.kernel.org/r/20191014145204.GS27757@arm.com

Re-reading this thread, I think the conclusion was more towards having a
WARN_ON in system_supports_fpsimd() (or may_use_simd()). We don't expect
code to start using neon before the SMP is initialised (other than
early_initcall(), the rest run after the secondary CPUs are brought up).

-- 
Catalin

Re: [PATCH v2 2/7] arm64: fpsimd: Make sure SVE setup is complete before SIMD is used

[Catalin Marinas]

On Tue, Dec 17, 2019 at 06:33:57PM +0000, Suzuki K Poulose wrote:
> In-kernel users of NEON rely on may_use_simd() to check if the SIMD
> can be used. However, we must initialize the SVE before SIMD can
> be used. Add a sanity check to make sure that we have completed the
> SVE setup before anyone uses the SIMD.
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
> Discussion here : https://lkml.kernel.org/r/20191014145204.GS27757@arm.com

Re-reading this thread, I think the conclusion was more towards having a
WARN_ON in system_supports_fpsimd() (or may_use_simd()). We don't expect
code to start using neon before the SMP is initialised (other than
early_initcall(), the rest run after the secondary CPUs are brought up).

-- 
Catalin

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

Re: [PATCH v2 2/7] arm64: fpsimd: Make sure SVE setup is complete before SIMD is used

[Suzuki Kuruppassery Poulose]

On 10/01/2020 11:51, Catalin Marinas wrote:
> On Tue, Dec 17, 2019 at 06:33:57PM +0000, Suzuki K Poulose wrote:
>> In-kernel users of NEON rely on may_use_simd() to check if the SIMD
>> can be used. However, we must initialize the SVE before SIMD can
>> be used. Add a sanity check to make sure that we have completed the
>> SVE setup before anyone uses the SIMD.
>>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: Will Deacon <will.deacon@arm.com>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>> ---
>> Discussion here : https://lkml.kernel.org/r/20191014145204.GS27757@arm.com
> 
> Re-reading this thread, I think the conclusion was more towards having a
> WARN_ON in system_supports_fpsimd() (or may_use_simd()). We don't expect
> code to start using neon before the SMP is initialised (other than
> early_initcall(), the rest run after the secondary CPUs are brought up).

Thanks for pointing out. I missed this from the Dave's last email.
I have added a WARN_ON(!system_capabilities_finalized()) to
may_use_simd() for the next version.

Thanks for the review !

Suzuki

Re: [PATCH v2 2/7] arm64: fpsimd: Make sure SVE setup is complete before SIMD is used

[Suzuki Kuruppassery Poulose]

On 10/01/2020 11:51, Catalin Marinas wrote:
> On Tue, Dec 17, 2019 at 06:33:57PM +0000, Suzuki K Poulose wrote:
>> In-kernel users of NEON rely on may_use_simd() to check if the SIMD
>> can be used. However, we must initialize the SVE before SIMD can
>> be used. Add a sanity check to make sure that we have completed the
>> SVE setup before anyone uses the SIMD.
>>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: Will Deacon <will.deacon@arm.com>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>> ---
>> Discussion here : https://lkml.kernel.org/r/20191014145204.GS27757@arm.com
> 
> Re-reading this thread, I think the conclusion was more towards having a
> WARN_ON in system_supports_fpsimd() (or may_use_simd()). We don't expect
> code to start using neon before the SMP is initialised (other than
> early_initcall(), the rest run after the secondary CPUs are brought up).

Thanks for pointing out. I missed this from the Dave's last email.
I have added a WARN_ON(!system_capabilities_finalized()) to
may_use_simd() for the next version.

Thanks for the review !

Suzuki

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

[PATCH v2 3/7] arm64: cpufeature: Fix the type of no FP/SIMD capability

[Suzuki K Poulose]

The NO_FPSIMD capability is defined with scope SYSTEM, which implies
that the "absence" of FP/SIMD on at least one CPU is detected only
after all the SMP CPUs are brought up. However, we use the status
of this capability for every context switch. So, let us change
the scope to LOCAL_CPU to allow the detection of this capability
as and when the first CPU without FP is brought up.

Also, the current type allows hotplugged CPU to be brought up without
FP/SIMD when all the current CPUs have FP/SIMD and we have the userspace
up. Fix both of these issues by changing the capability to
BOOT_RESTRICTED_LOCAL_CPU_FEATURE.

Fixes: 82e0191a1aa11abf ("arm64: Support systems without FP/ASIMD")
Cc: Will Deacon <will@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 arch/arm64/kernel/cpufeature.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index d25ad65bfac2..2fc9f18e2d2d 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -1369,7 +1369,7 @@ static const struct arm64_cpu_capabilities arm64_features[] = {
 	{
 		/* FP/SIMD is not implemented */
 		.capability = ARM64_HAS_NO_FPSIMD,
-		.type = ARM64_CPUCAP_SYSTEM_FEATURE,
+		.type = ARM64_CPUCAP_BOOT_RESTRICTED_CPU_LOCAL_FEATURE,
 		.min_field_value = 0,
 		.matches = has_no_fpsimd,
 	},
-- 
2.23.0

[PATCH v2 3/7] arm64: cpufeature: Fix the type of no FP/SIMD capability

[Suzuki K Poulose]

The NO_FPSIMD capability is defined with scope SYSTEM, which implies
that the "absence" of FP/SIMD on at least one CPU is detected only
after all the SMP CPUs are brought up. However, we use the status
of this capability for every context switch. So, let us change
the scope to LOCAL_CPU to allow the detection of this capability
as and when the first CPU without FP is brought up.

Also, the current type allows hotplugged CPU to be brought up without
FP/SIMD when all the current CPUs have FP/SIMD and we have the userspace
up. Fix both of these issues by changing the capability to
BOOT_RESTRICTED_LOCAL_CPU_FEATURE.

Fixes: 82e0191a1aa11abf ("arm64: Support systems without FP/ASIMD")
Cc: Will Deacon <will@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 arch/arm64/kernel/cpufeature.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index d25ad65bfac2..2fc9f18e2d2d 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -1369,7 +1369,7 @@ static const struct arm64_cpu_capabilities arm64_features[] = {
 	{
 		/* FP/SIMD is not implemented */
 		.capability = ARM64_HAS_NO_FPSIMD,
-		.type = ARM64_CPUCAP_SYSTEM_FEATURE,
+		.type = ARM64_CPUCAP_BOOT_RESTRICTED_CPU_LOCAL_FEATURE,
 		.min_field_value = 0,
 		.matches = has_no_fpsimd,
 	},
-- 
2.23.0


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

Re: [PATCH v2 3/7] arm64: cpufeature: Fix the type of no FP/SIMD capability

[Catalin Marinas]

On Tue, Dec 17, 2019 at 06:33:58PM +0000, Suzuki K Poulose wrote:
> The NO_FPSIMD capability is defined with scope SYSTEM, which implies
> that the "absence" of FP/SIMD on at least one CPU is detected only
> after all the SMP CPUs are brought up. However, we use the status
> of this capability for every context switch. So, let us change
> the scope to LOCAL_CPU to allow the detection of this capability
> as and when the first CPU without FP is brought up.
> 
> Also, the current type allows hotplugged CPU to be brought up without
> FP/SIMD when all the current CPUs have FP/SIMD and we have the userspace
> up. Fix both of these issues by changing the capability to
> BOOT_RESTRICTED_LOCAL_CPU_FEATURE.
> 
> Fixes: 82e0191a1aa11abf ("arm64: Support systems without FP/ASIMD")
> Cc: Will Deacon <will@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>

Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

Re: [PATCH v2 3/7] arm64: cpufeature: Fix the type of no FP/SIMD capability

[Catalin Marinas]

On Tue, Dec 17, 2019 at 06:33:58PM +0000, Suzuki K Poulose wrote:
> The NO_FPSIMD capability is defined with scope SYSTEM, which implies
> that the "absence" of FP/SIMD on at least one CPU is detected only
> after all the SMP CPUs are brought up. However, we use the status
> of this capability for every context switch. So, let us change
> the scope to LOCAL_CPU to allow the detection of this capability
> as and when the first CPU without FP is brought up.
> 
> Also, the current type allows hotplugged CPU to be brought up without
> FP/SIMD when all the current CPUs have FP/SIMD and we have the userspace
> up. Fix both of these issues by changing the capability to
> BOOT_RESTRICTED_LOCAL_CPU_FEATURE.
> 
> Fixes: 82e0191a1aa11abf ("arm64: Support systems without FP/ASIMD")
> Cc: Will Deacon <will@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>

Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

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

[PATCH v2 4/7] arm64: cpufeature: Set the FP/SIMD compat HWCAP bits properly

[Suzuki K Poulose]

We set the compat_elf_hwcap bits unconditionally on arm64 to
include the VFP and NEON support. However, the FP/SIMD unit
is optional on Arm v8 and thus could be missing. We already
handle this properly in the kernel, but still advertise to
the COMPAT applications that the VFP is available. Fix this
to make sure we only advertise when we really have them.

Fixes: 82e0191a1aa11abf ("arm64: Support systems without FP/ASIMD")
Cc: Will Deacon <will@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
Changes since v1:
  - Switch to using cpuid_feature_extract_unsigned_field() rather than
    hard coding field extraction.
---
 arch/arm64/kernel/cpufeature.c | 37 +++++++++++++++++++++++++++++++---
 1 file changed, 34 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 2fc9f18e2d2d..c008164b0848 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -32,9 +32,7 @@ static unsigned long elf_hwcap __read_mostly;
 #define COMPAT_ELF_HWCAP_DEFAULT	\
 				(COMPAT_HWCAP_HALF|COMPAT_HWCAP_THUMB|\
 				 COMPAT_HWCAP_FAST_MULT|COMPAT_HWCAP_EDSP|\
-				 COMPAT_HWCAP_TLS|COMPAT_HWCAP_VFP|\
-				 COMPAT_HWCAP_VFPv3|COMPAT_HWCAP_VFPv4|\
-				 COMPAT_HWCAP_NEON|COMPAT_HWCAP_IDIV|\
+				 COMPAT_HWCAP_TLS|COMPAT_HWCAP_IDIV|\
 				 COMPAT_HWCAP_LPAE)
 unsigned int compat_elf_hwcap __read_mostly = COMPAT_ELF_HWCAP_DEFAULT;
 unsigned int compat_elf_hwcap2 __read_mostly;
@@ -1597,6 +1595,12 @@ static const struct arm64_cpu_capabilities arm64_features[] = {
 		.match_list = list,						\
 	}
 
+#define HWCAP_CAP_MATCH(match, cap_type, cap)					\
+	{									\
+		__HWCAP_CAP(#cap, cap_type, cap)				\
+		.matches = match,						\
+	}
+
 #ifdef CONFIG_ARM64_PTR_AUTH
 static const struct arm64_cpu_capabilities ptr_auth_hwcap_addr_matches[] = {
 	{
@@ -1670,8 +1674,35 @@ static const struct arm64_cpu_capabilities arm64_elf_hwcaps[] = {
 	{},
 };
 
+#ifdef CONFIG_COMPAT
+static bool compat_has_neon(const struct arm64_cpu_capabilities *cap, int scope)
+{
+	/*
+	 * Check that all of MVFR1_EL1.{SIMDSP, SIMDInt, SIMDLS} are available,
+	 * in line with that of arm32 as in vfp_init(). We make sure that the
+	 * check is future proof, by making sure value is non-zero.
+	 */
+	u32 mvfr1;
+
+	WARN_ON(scope == SCOPE_LOCAL_CPU && preemptible());
+	if (scope == SCOPE_SYSTEM)
+		mvfr1 = read_sanitised_ftr_reg(SYS_MVFR1_EL1);
+	else
+		mvfr1 = read_sysreg_s(SYS_MVFR1_EL1);
+
+	return cpuid_feature_extract_unsigned_field(mvfr1, MVFR1_SIMDSP_SHIFT) &&
+		cpuid_feature_extract_unsigned_field(mvfr1, MVFR1_SIMDINT_SHIFT) &&
+		cpuid_feature_extract_unsigned_field(mvfr1, MVFR1_SIMDLS_SHIFT);
+}
+#endif
+
 static const struct arm64_cpu_capabilities compat_elf_hwcaps[] = {
 #ifdef CONFIG_COMPAT
+	HWCAP_CAP_MATCH(compat_has_neon, CAP_COMPAT_HWCAP, COMPAT_HWCAP_NEON),
+	HWCAP_CAP(SYS_MVFR1_EL1, MVFR1_SIMDFMAC_SHIFT, FTR_UNSIGNED, 1, CAP_COMPAT_HWCAP, COMPAT_HWCAP_VFPv4),
+	/* Arm v8 mandates MVFR0.FPDP == {0, 2}. So, piggy back on this for the presence of VFP support */
+	HWCAP_CAP(SYS_MVFR0_EL1, MVFR0_FPDP_SHIFT, FTR_UNSIGNED, 2, CAP_COMPAT_HWCAP, COMPAT_HWCAP_VFP),
+	HWCAP_CAP(SYS_MVFR0_EL1, MVFR0_FPDP_SHIFT, FTR_UNSIGNED, 2, CAP_COMPAT_HWCAP, COMPAT_HWCAP_VFPv3),
 	HWCAP_CAP(SYS_ID_ISAR5_EL1, ID_ISAR5_AES_SHIFT, FTR_UNSIGNED, 2, CAP_COMPAT_HWCAP2, COMPAT_HWCAP2_PMULL),
 	HWCAP_CAP(SYS_ID_ISAR5_EL1, ID_ISAR5_AES_SHIFT, FTR_UNSIGNED, 1, CAP_COMPAT_HWCAP2, COMPAT_HWCAP2_AES),
 	HWCAP_CAP(SYS_ID_ISAR5_EL1, ID_ISAR5_SHA1_SHIFT, FTR_UNSIGNED, 1, CAP_COMPAT_HWCAP2, COMPAT_HWCAP2_SHA1),
-- 
2.23.0

[PATCH v2 4/7] arm64: cpufeature: Set the FP/SIMD compat HWCAP bits properly

[Suzuki K Poulose]

We set the compat_elf_hwcap bits unconditionally on arm64 to
include the VFP and NEON support. However, the FP/SIMD unit
is optional on Arm v8 and thus could be missing. We already
handle this properly in the kernel, but still advertise to
the COMPAT applications that the VFP is available. Fix this
to make sure we only advertise when we really have them.

Fixes: 82e0191a1aa11abf ("arm64: Support systems without FP/ASIMD")
Cc: Will Deacon <will@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
Changes since v1:
  - Switch to using cpuid_feature_extract_unsigned_field() rather than
    hard coding field extraction.
---
 arch/arm64/kernel/cpufeature.c | 37 +++++++++++++++++++++++++++++++---
 1 file changed, 34 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 2fc9f18e2d2d..c008164b0848 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -32,9 +32,7 @@ static unsigned long elf_hwcap __read_mostly;
 #define COMPAT_ELF_HWCAP_DEFAULT	\
 				(COMPAT_HWCAP_HALF|COMPAT_HWCAP_THUMB|\
 				 COMPAT_HWCAP_FAST_MULT|COMPAT_HWCAP_EDSP|\
-				 COMPAT_HWCAP_TLS|COMPAT_HWCAP_VFP|\
-				 COMPAT_HWCAP_VFPv3|COMPAT_HWCAP_VFPv4|\
-				 COMPAT_HWCAP_NEON|COMPAT_HWCAP_IDIV|\
+				 COMPAT_HWCAP_TLS|COMPAT_HWCAP_IDIV|\
 				 COMPAT_HWCAP_LPAE)
 unsigned int compat_elf_hwcap __read_mostly = COMPAT_ELF_HWCAP_DEFAULT;
 unsigned int compat_elf_hwcap2 __read_mostly;
@@ -1597,6 +1595,12 @@ static const struct arm64_cpu_capabilities arm64_features[] = {
 		.match_list = list,						\
 	}
 
+#define HWCAP_CAP_MATCH(match, cap_type, cap)					\
+	{									\
+		__HWCAP_CAP(#cap, cap_type, cap)				\
+		.matches = match,						\
+	}
+
 #ifdef CONFIG_ARM64_PTR_AUTH
 static const struct arm64_cpu_capabilities ptr_auth_hwcap_addr_matches[] = {
 	{
@@ -1670,8 +1674,35 @@ static const struct arm64_cpu_capabilities arm64_elf_hwcaps[] = {
 	{},
 };
 
+#ifdef CONFIG_COMPAT
+static bool compat_has_neon(const struct arm64_cpu_capabilities *cap, int scope)
+{
+	/*
+	 * Check that all of MVFR1_EL1.{SIMDSP, SIMDInt, SIMDLS} are available,
+	 * in line with that of arm32 as in vfp_init(). We make sure that the
+	 * check is future proof, by making sure value is non-zero.
+	 */
+	u32 mvfr1;
+
+	WARN_ON(scope == SCOPE_LOCAL_CPU && preemptible());
+	if (scope == SCOPE_SYSTEM)
+		mvfr1 = read_sanitised_ftr_reg(SYS_MVFR1_EL1);
+	else
+		mvfr1 = read_sysreg_s(SYS_MVFR1_EL1);
+
+	return cpuid_feature_extract_unsigned_field(mvfr1, MVFR1_SIMDSP_SHIFT) &&
+		cpuid_feature_extract_unsigned_field(mvfr1, MVFR1_SIMDINT_SHIFT) &&
+		cpuid_feature_extract_unsigned_field(mvfr1, MVFR1_SIMDLS_SHIFT);
+}
+#endif
+
 static const struct arm64_cpu_capabilities compat_elf_hwcaps[] = {
 #ifdef CONFIG_COMPAT
+	HWCAP_CAP_MATCH(compat_has_neon, CAP_COMPAT_HWCAP, COMPAT_HWCAP_NEON),
+	HWCAP_CAP(SYS_MVFR1_EL1, MVFR1_SIMDFMAC_SHIFT, FTR_UNSIGNED, 1, CAP_COMPAT_HWCAP, COMPAT_HWCAP_VFPv4),
+	/* Arm v8 mandates MVFR0.FPDP == {0, 2}. So, piggy back on this for the presence of VFP support */
+	HWCAP_CAP(SYS_MVFR0_EL1, MVFR0_FPDP_SHIFT, FTR_UNSIGNED, 2, CAP_COMPAT_HWCAP, COMPAT_HWCAP_VFP),
+	HWCAP_CAP(SYS_MVFR0_EL1, MVFR0_FPDP_SHIFT, FTR_UNSIGNED, 2, CAP_COMPAT_HWCAP, COMPAT_HWCAP_VFPv3),
 	HWCAP_CAP(SYS_ID_ISAR5_EL1, ID_ISAR5_AES_SHIFT, FTR_UNSIGNED, 2, CAP_COMPAT_HWCAP2, COMPAT_HWCAP2_PMULL),
 	HWCAP_CAP(SYS_ID_ISAR5_EL1, ID_ISAR5_AES_SHIFT, FTR_UNSIGNED, 1, CAP_COMPAT_HWCAP2, COMPAT_HWCAP2_AES),
 	HWCAP_CAP(SYS_ID_ISAR5_EL1, ID_ISAR5_SHA1_SHIFT, FTR_UNSIGNED, 1, CAP_COMPAT_HWCAP2, COMPAT_HWCAP2_SHA1),
-- 
2.23.0


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

Re: [PATCH v2 4/7] arm64: cpufeature: Set the FP/SIMD compat HWCAP bits properly

[Catalin Marinas]

On Tue, Dec 17, 2019 at 06:33:59PM +0000, Suzuki K Poulose wrote:
> We set the compat_elf_hwcap bits unconditionally on arm64 to
> include the VFP and NEON support. However, the FP/SIMD unit
> is optional on Arm v8 and thus could be missing. We already
> handle this properly in the kernel, but still advertise to
> the COMPAT applications that the VFP is available. Fix this
> to make sure we only advertise when we really have them.
> 
> Fixes: 82e0191a1aa11abf ("arm64: Support systems without FP/ASIMD")
> Cc: Will Deacon <will@kernel.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>

Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

Re: [PATCH v2 4/7] arm64: cpufeature: Set the FP/SIMD compat HWCAP bits properly

[Catalin Marinas]

On Tue, Dec 17, 2019 at 06:33:59PM +0000, Suzuki K Poulose wrote:
> We set the compat_elf_hwcap bits unconditionally on arm64 to
> include the VFP and NEON support. However, the FP/SIMD unit
> is optional on Arm v8 and thus could be missing. We already
> handle this properly in the kernel, but still advertise to
> the COMPAT applications that the VFP is available. Fix this
> to make sure we only advertise when we really have them.
> 
> Fixes: 82e0191a1aa11abf ("arm64: Support systems without FP/ASIMD")
> Cc: Will Deacon <will@kernel.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>

Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

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

[PATCH v2 5/7] arm64: ptrace: nofpsimd: Fail FP/SIMD regset operations

[Suzuki K Poulose]

When fp/simd is not supported on the system, fail the operations
of FP/SIMD regsets.

Fixes: 82e0191a1aa11abf ("arm64: Support systems without FP/ASIMD")
Cc: Will Deacon <will.deacon@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 arch/arm64/kernel/ptrace.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
index 6771c399d40c..0135b944b8db 100644
--- a/arch/arm64/kernel/ptrace.c
+++ b/arch/arm64/kernel/ptrace.c
@@ -637,6 +637,9 @@ static int fpr_get(struct task_struct *target, const struct user_regset *regset,
 		   unsigned int pos, unsigned int count,
 		   void *kbuf, void __user *ubuf)
 {
+	if (!system_supports_fpsimd())
+		return -EINVAL;
+
 	if (target == current)
 		fpsimd_preserve_current_state();
 
@@ -676,6 +679,9 @@ static int fpr_set(struct task_struct *target, const struct user_regset *regset,
 {
 	int ret;
 
+	if (!system_supports_fpsimd())
+		return -EINVAL;
+
 	ret = __fpr_set(target, regset, pos, count, kbuf, ubuf, 0);
 	if (ret)
 		return ret;
@@ -1348,6 +1354,9 @@ static int compat_vfp_get(struct task_struct *target,
 	compat_ulong_t fpscr;
 	int ret, vregs_end_pos;
 
+	if (!system_supports_fpsimd())
+		return -EINVAL;
+
 	uregs = &target->thread.uw.fpsimd_state;
 
 	if (target == current)
@@ -1381,6 +1390,9 @@ static int compat_vfp_set(struct task_struct *target,
 	compat_ulong_t fpscr;
 	int ret, vregs_end_pos;
 
+	if (!system_supports_fpsimd())
+		return -EINVAL;
+
 	uregs = &target->thread.uw.fpsimd_state;
 
 	vregs_end_pos = VFP_STATE_SIZE - sizeof(compat_ulong_t);
-- 
2.23.0

[PATCH v2 5/7] arm64: ptrace: nofpsimd: Fail FP/SIMD regset operations

[Suzuki K Poulose]

When fp/simd is not supported on the system, fail the operations
of FP/SIMD regsets.

Fixes: 82e0191a1aa11abf ("arm64: Support systems without FP/ASIMD")
Cc: Will Deacon <will.deacon@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 arch/arm64/kernel/ptrace.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
index 6771c399d40c..0135b944b8db 100644
--- a/arch/arm64/kernel/ptrace.c
+++ b/arch/arm64/kernel/ptrace.c
@@ -637,6 +637,9 @@ static int fpr_get(struct task_struct *target, const struct user_regset *regset,
 		   unsigned int pos, unsigned int count,
 		   void *kbuf, void __user *ubuf)
 {
+	if (!system_supports_fpsimd())
+		return -EINVAL;
+
 	if (target == current)
 		fpsimd_preserve_current_state();
 
@@ -676,6 +679,9 @@ static int fpr_set(struct task_struct *target, const struct user_regset *regset,
 {
 	int ret;
 
+	if (!system_supports_fpsimd())
+		return -EINVAL;
+
 	ret = __fpr_set(target, regset, pos, count, kbuf, ubuf, 0);
 	if (ret)
 		return ret;
@@ -1348,6 +1354,9 @@ static int compat_vfp_get(struct task_struct *target,
 	compat_ulong_t fpscr;
 	int ret, vregs_end_pos;
 
+	if (!system_supports_fpsimd())
+		return -EINVAL;
+
 	uregs = &target->thread.uw.fpsimd_state;
 
 	if (target == current)
@@ -1381,6 +1390,9 @@ static int compat_vfp_set(struct task_struct *target,
 	compat_ulong_t fpscr;
 	int ret, vregs_end_pos;
 
+	if (!system_supports_fpsimd())
+		return -EINVAL;
+
 	uregs = &target->thread.uw.fpsimd_state;
 
 	vregs_end_pos = VFP_STATE_SIZE - sizeof(compat_ulong_t);
-- 
2.23.0


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

Re: [PATCH v2 5/7] arm64: ptrace: nofpsimd: Fail FP/SIMD regset operations

[Catalin Marinas]

On Tue, Dec 17, 2019 at 06:34:00PM +0000, Suzuki K Poulose wrote:
> diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
> index 6771c399d40c..0135b944b8db 100644
> --- a/arch/arm64/kernel/ptrace.c
> +++ b/arch/arm64/kernel/ptrace.c
> @@ -637,6 +637,9 @@ static int fpr_get(struct task_struct *target, const struct user_regset *regset,
>  		   unsigned int pos, unsigned int count,
>  		   void *kbuf, void __user *ubuf)
>  {
> +	if (!system_supports_fpsimd())
> +		return -EINVAL;
> +
>  	if (target == current)
>  		fpsimd_preserve_current_state();

I checked the coredump code (fill_thread_core_info) and works correctly
if we return -EINVAL here. But for completeness, we could add an
fpr_active() callback to aarch{32,64}_regsets (x86 does the same).

-- 
Catalin

Re: [PATCH v2 5/7] arm64: ptrace: nofpsimd: Fail FP/SIMD regset operations

[Catalin Marinas]

On Tue, Dec 17, 2019 at 06:34:00PM +0000, Suzuki K Poulose wrote:
> diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
> index 6771c399d40c..0135b944b8db 100644
> --- a/arch/arm64/kernel/ptrace.c
> +++ b/arch/arm64/kernel/ptrace.c
> @@ -637,6 +637,9 @@ static int fpr_get(struct task_struct *target, const struct user_regset *regset,
>  		   unsigned int pos, unsigned int count,
>  		   void *kbuf, void __user *ubuf)
>  {
> +	if (!system_supports_fpsimd())
> +		return -EINVAL;
> +
>  	if (target == current)
>  		fpsimd_preserve_current_state();

I checked the coredump code (fill_thread_core_info) and works correctly
if we return -EINVAL here. But for completeness, we could add an
fpr_active() callback to aarch{32,64}_regsets (x86 does the same).

-- 
Catalin

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

Re: [PATCH v2 5/7] arm64: ptrace: nofpsimd: Fail FP/SIMD regset operations

[Suzuki Kuruppassery Poulose]

On 10/01/2020 15:12, Catalin Marinas wrote:
> On Tue, Dec 17, 2019 at 06:34:00PM +0000, Suzuki K Poulose wrote:
>> diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
>> index 6771c399d40c..0135b944b8db 100644
>> --- a/arch/arm64/kernel/ptrace.c
>> +++ b/arch/arm64/kernel/ptrace.c
>> @@ -637,6 +637,9 @@ static int fpr_get(struct task_struct *target, const struct user_regset *regset,
>>   		   unsigned int pos, unsigned int count,
>>   		   void *kbuf, void __user *ubuf)
>>   {
>> +	if (!system_supports_fpsimd())
>> +		return -EINVAL;
>> +
>>   	if (target == current)
>>   		fpsimd_preserve_current_state();
> 
> I checked the coredump code (fill_thread_core_info) and works correctly
> if we return -EINVAL here. But for completeness, we could add an
> fpr_active() callback to aarch{32,64}_regsets (x86 does the same).
> 

Sure, makes sense. I have now added fpr_active() hook for the FP
regsets.

Suzuki

Re: [PATCH v2 5/7] arm64: ptrace: nofpsimd: Fail FP/SIMD regset operations

[Suzuki Kuruppassery Poulose]

On 10/01/2020 15:12, Catalin Marinas wrote:
> On Tue, Dec 17, 2019 at 06:34:00PM +0000, Suzuki K Poulose wrote:
>> diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
>> index 6771c399d40c..0135b944b8db 100644
>> --- a/arch/arm64/kernel/ptrace.c
>> +++ b/arch/arm64/kernel/ptrace.c
>> @@ -637,6 +637,9 @@ static int fpr_get(struct task_struct *target, const struct user_regset *regset,
>>   		   unsigned int pos, unsigned int count,
>>   		   void *kbuf, void __user *ubuf)
>>   {
>> +	if (!system_supports_fpsimd())
>> +		return -EINVAL;
>> +
>>   	if (target == current)
>>   		fpsimd_preserve_current_state();
> 
> I checked the coredump code (fill_thread_core_info) and works correctly
> if we return -EINVAL here. But for completeness, we could add an
> fpr_active() callback to aarch{32,64}_regsets (x86 does the same).
> 

Sure, makes sense. I have now added fpr_active() hook for the FP
regsets.

Suzuki

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

[PATCH v2 6/7] arm64: signal: nofpsimd: Handle fp/simd context for signal frames

[Suzuki K Poulose]

Make sure we try to save/restore the vfp/fpsimd context for signal
handling only when the fp/simd support is available. Otherwise, skip
the frames.

Cc: Will Deacon <will.deacon@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 arch/arm64/kernel/signal.c   | 17 +++++++++++++++--
 arch/arm64/kernel/signal32.c | 11 +++++++++--
 2 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
index dd2cdc0d5be2..c648f7627035 100644
--- a/arch/arm64/kernel/signal.c
+++ b/arch/arm64/kernel/signal.c
@@ -173,6 +173,10 @@ static int preserve_fpsimd_context(struct fpsimd_context __user *ctx)
 		&current->thread.uw.fpsimd_state;
 	int err;
 
+	/* This must not be called when FP/SIMD support is missing */
+	if (WARN_ON(!system_supports_fpsimd()))
+		return -EINVAL;
+
 	/* copy the FP and status/control registers */
 	err = __copy_to_user(ctx->vregs, fpsimd->vregs, sizeof(fpsimd->vregs));
 	__put_user_error(fpsimd->fpsr, &ctx->fpsr, err);
@@ -191,6 +195,10 @@ static int restore_fpsimd_context(struct fpsimd_context __user *ctx)
 	__u32 magic, size;
 	int err = 0;
 
+	/* This must not be called when FP/SIMD support is missing */
+	if (WARN_ON(!system_supports_fpsimd()))
+		return -EINVAL;
+
 	/* check the magic/size information */
 	__get_user_error(magic, &ctx->head.magic, err);
 	__get_user_error(size, &ctx->head.size, err);
@@ -261,6 +269,9 @@ static int restore_sve_fpsimd_context(struct user_ctxs *user)
 	struct user_fpsimd_state fpsimd;
 	struct sve_context sve;
 
+	if (WARN_ON(!system_supports_fpsimd()))
+		return -EINVAL;
+
 	if (__copy_from_user(&sve, user->sve, sizeof(sve)))
 		return -EFAULT;
 
@@ -371,6 +382,8 @@ static int parse_user_sigframe(struct user_ctxs *user,
 			goto done;
 
 		case FPSIMD_MAGIC:
+			if (!system_supports_fpsimd())
+				goto invalid;
 			if (user->fpsimd)
 				goto invalid;
 
@@ -506,7 +519,7 @@ static int restore_sigframe(struct pt_regs *regs,
 	if (err == 0)
 		err = parse_user_sigframe(&user, sf);
 
-	if (err == 0) {
+	if (err == 0 && system_supports_fpsimd()) {
 		if (!user.fpsimd)
 			return -EINVAL;
 
@@ -623,7 +636,7 @@ static int setup_sigframe(struct rt_sigframe_user_layout *user,
 
 	err |= __copy_to_user(&sf->uc.uc_sigmask, set, sizeof(*set));
 
-	if (err == 0) {
+	if (err == 0 && system_supports_fpsimd()) {
 		struct fpsimd_context __user *fpsimd_ctx =
 			apply_user_offset(user, user->fpsimd_offset);
 		err |= preserve_fpsimd_context(fpsimd_ctx);
diff --git a/arch/arm64/kernel/signal32.c b/arch/arm64/kernel/signal32.c
index 12a585386c2f..97ace6919bc2 100644
--- a/arch/arm64/kernel/signal32.c
+++ b/arch/arm64/kernel/signal32.c
@@ -100,6 +100,9 @@ static int compat_preserve_vfp_context(struct compat_vfp_sigframe __user *frame)
 	compat_ulong_t fpscr, fpexc;
 	int i, err = 0;
 
+	/* This must not be called when the FP/SIMD is missing */
+	if (WARN_ON(!system_supports_fpsimd()))
+		return -EINVAL;
 	/*
 	 * Save the hardware registers to the fpsimd_state structure.
 	 * Note that this also saves V16-31, which aren't visible
@@ -149,6 +152,10 @@ static int compat_restore_vfp_context(struct compat_vfp_sigframe __user *frame)
 	compat_ulong_t fpscr;
 	int i, err = 0;
 
+	/* This must not be called when the FP/SIMD is missing */
+	if (WARN_ON(!system_supports_fpsimd()))
+		return -EINVAL;
+
 	__get_user_error(magic, &frame->magic, err);
 	__get_user_error(size, &frame->size, err);
 
@@ -223,7 +230,7 @@ static int compat_restore_sigframe(struct pt_regs *regs,
 	err |= !valid_user_regs(&regs->user_regs, current);
 
 	aux = (struct compat_aux_sigframe __user *) sf->uc.uc_regspace;
-	if (err == 0)
+	if (err == 0 && system_supports_fpsimd())
 		err |= compat_restore_vfp_context(&aux->vfp);
 
 	return err;
@@ -419,7 +426,7 @@ static int compat_setup_sigframe(struct compat_sigframe __user *sf,
 
 	aux = (struct compat_aux_sigframe __user *) sf->uc.uc_regspace;
 
-	if (err == 0)
+	if (err == 0 && system_supports_fpsimd())
 		err |= compat_preserve_vfp_context(&aux->vfp);
 	__put_user_error(0, &aux->end_magic, err);
 
-- 
2.23.0

[PATCH v2 6/7] arm64: signal: nofpsimd: Handle fp/simd context for signal frames

[Suzuki K Poulose]

Make sure we try to save/restore the vfp/fpsimd context for signal
handling only when the fp/simd support is available. Otherwise, skip
the frames.

Cc: Will Deacon <will.deacon@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 arch/arm64/kernel/signal.c   | 17 +++++++++++++++--
 arch/arm64/kernel/signal32.c | 11 +++++++++--
 2 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
index dd2cdc0d5be2..c648f7627035 100644
--- a/arch/arm64/kernel/signal.c
+++ b/arch/arm64/kernel/signal.c
@@ -173,6 +173,10 @@ static int preserve_fpsimd_context(struct fpsimd_context __user *ctx)
 		&current->thread.uw.fpsimd_state;
 	int err;
 
+	/* This must not be called when FP/SIMD support is missing */
+	if (WARN_ON(!system_supports_fpsimd()))
+		return -EINVAL;
+
 	/* copy the FP and status/control registers */
 	err = __copy_to_user(ctx->vregs, fpsimd->vregs, sizeof(fpsimd->vregs));
 	__put_user_error(fpsimd->fpsr, &ctx->fpsr, err);
@@ -191,6 +195,10 @@ static int restore_fpsimd_context(struct fpsimd_context __user *ctx)
 	__u32 magic, size;
 	int err = 0;
 
+	/* This must not be called when FP/SIMD support is missing */
+	if (WARN_ON(!system_supports_fpsimd()))
+		return -EINVAL;
+
 	/* check the magic/size information */
 	__get_user_error(magic, &ctx->head.magic, err);
 	__get_user_error(size, &ctx->head.size, err);
@@ -261,6 +269,9 @@ static int restore_sve_fpsimd_context(struct user_ctxs *user)
 	struct user_fpsimd_state fpsimd;
 	struct sve_context sve;
 
+	if (WARN_ON(!system_supports_fpsimd()))
+		return -EINVAL;
+
 	if (__copy_from_user(&sve, user->sve, sizeof(sve)))
 		return -EFAULT;
 
@@ -371,6 +382,8 @@ static int parse_user_sigframe(struct user_ctxs *user,
 			goto done;
 
 		case FPSIMD_MAGIC:
+			if (!system_supports_fpsimd())
+				goto invalid;
 			if (user->fpsimd)
 				goto invalid;
 
@@ -506,7 +519,7 @@ static int restore_sigframe(struct pt_regs *regs,
 	if (err == 0)
 		err = parse_user_sigframe(&user, sf);
 
-	if (err == 0) {
+	if (err == 0 && system_supports_fpsimd()) {
 		if (!user.fpsimd)
 			return -EINVAL;
 
@@ -623,7 +636,7 @@ static int setup_sigframe(struct rt_sigframe_user_layout *user,
 
 	err |= __copy_to_user(&sf->uc.uc_sigmask, set, sizeof(*set));
 
-	if (err == 0) {
+	if (err == 0 && system_supports_fpsimd()) {
 		struct fpsimd_context __user *fpsimd_ctx =
 			apply_user_offset(user, user->fpsimd_offset);
 		err |= preserve_fpsimd_context(fpsimd_ctx);
diff --git a/arch/arm64/kernel/signal32.c b/arch/arm64/kernel/signal32.c
index 12a585386c2f..97ace6919bc2 100644
--- a/arch/arm64/kernel/signal32.c
+++ b/arch/arm64/kernel/signal32.c
@@ -100,6 +100,9 @@ static int compat_preserve_vfp_context(struct compat_vfp_sigframe __user *frame)
 	compat_ulong_t fpscr, fpexc;
 	int i, err = 0;
 
+	/* This must not be called when the FP/SIMD is missing */
+	if (WARN_ON(!system_supports_fpsimd()))
+		return -EINVAL;
 	/*
 	 * Save the hardware registers to the fpsimd_state structure.
 	 * Note that this also saves V16-31, which aren't visible
@@ -149,6 +152,10 @@ static int compat_restore_vfp_context(struct compat_vfp_sigframe __user *frame)
 	compat_ulong_t fpscr;
 	int i, err = 0;
 
+	/* This must not be called when the FP/SIMD is missing */
+	if (WARN_ON(!system_supports_fpsimd()))
+		return -EINVAL;
+
 	__get_user_error(magic, &frame->magic, err);
 	__get_user_error(size, &frame->size, err);
 
@@ -223,7 +230,7 @@ static int compat_restore_sigframe(struct pt_regs *regs,
 	err |= !valid_user_regs(&regs->user_regs, current);
 
 	aux = (struct compat_aux_sigframe __user *) sf->uc.uc_regspace;
-	if (err == 0)
+	if (err == 0 && system_supports_fpsimd())
 		err |= compat_restore_vfp_context(&aux->vfp);
 
 	return err;
@@ -419,7 +426,7 @@ static int compat_setup_sigframe(struct compat_sigframe __user *sf,
 
 	aux = (struct compat_aux_sigframe __user *) sf->uc.uc_regspace;
 
-	if (err == 0)
+	if (err == 0 && system_supports_fpsimd())
 		err |= compat_preserve_vfp_context(&aux->vfp);
 	__put_user_error(0, &aux->end_magic, err);
 
-- 
2.23.0


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

Re: [PATCH v2 6/7] arm64: signal: nofpsimd: Handle fp/simd context for signal frames

[Catalin Marinas]

On Tue, Dec 17, 2019 at 06:34:01PM +0000, Suzuki K Poulose wrote:
> diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
> index dd2cdc0d5be2..c648f7627035 100644
> --- a/arch/arm64/kernel/signal.c
> +++ b/arch/arm64/kernel/signal.c
> @@ -173,6 +173,10 @@ static int preserve_fpsimd_context(struct fpsimd_context __user *ctx)
>  		&current->thread.uw.fpsimd_state;
>  	int err;
>  
> +	/* This must not be called when FP/SIMD support is missing */
> +	if (WARN_ON(!system_supports_fpsimd()))
> +		return -EINVAL;

I'd drop this, see below.

> @@ -191,6 +195,10 @@ static int restore_fpsimd_context(struct fpsimd_context __user *ctx)
>  	__u32 magic, size;
>  	int err = 0;
>  
> +	/* This must not be called when FP/SIMD support is missing */
> +	if (WARN_ON(!system_supports_fpsimd()))
> +		return -EINVAL;
> +
>  	/* check the magic/size information */
>  	__get_user_error(magic, &ctx->head.magic, err);
>  	__get_user_error(size, &ctx->head.size, err);
> @@ -261,6 +269,9 @@ static int restore_sve_fpsimd_context(struct user_ctxs *user)
>  	struct user_fpsimd_state fpsimd;
>  	struct sve_context sve;
>  
> +	if (WARN_ON(!system_supports_fpsimd()))
> +		return -EINVAL;
> +
>  	if (__copy_from_user(&sve, user->sve, sizeof(sve)))
>  		return -EFAULT;
>  
> @@ -371,6 +382,8 @@ static int parse_user_sigframe(struct user_ctxs *user,
>  			goto done;
>  
>  		case FPSIMD_MAGIC:
> +			if (!system_supports_fpsimd())
> +				goto invalid;
>  			if (user->fpsimd)
>  				goto invalid;
>  
> @@ -506,7 +519,7 @@ static int restore_sigframe(struct pt_regs *regs,
>  	if (err == 0)
>  		err = parse_user_sigframe(&user, sf);
>  
> -	if (err == 0) {
> +	if (err == 0 && system_supports_fpsimd()) {
>  		if (!user.fpsimd)
>  			return -EINVAL;

I don't think we need to be over paranoid here and add three/four checks
and two warnings in static functions. parse_user_sigframe() already
returns -EINVAL if SVE or FPSIMD is missing (the latter with your check
above). We don't need this additional check in restore_sigframe() and
restore_{sve_,}fpsimd_context(), the call paths are simple enough.

>  
> @@ -623,7 +636,7 @@ static int setup_sigframe(struct rt_sigframe_user_layout *user,
>  
>  	err |= __copy_to_user(&sf->uc.uc_sigmask, set, sizeof(*set));
>  
> -	if (err == 0) {
> +	if (err == 0 && system_supports_fpsimd()) {
>  		struct fpsimd_context __user *fpsimd_ctx =
>  			apply_user_offset(user, user->fpsimd_offset);
>  		err |= preserve_fpsimd_context(fpsimd_ctx);

This check is also sufficient for a static function not to have the
WARN_ON.

> diff --git a/arch/arm64/kernel/signal32.c b/arch/arm64/kernel/signal32.c
> index 12a585386c2f..97ace6919bc2 100644
> --- a/arch/arm64/kernel/signal32.c
> +++ b/arch/arm64/kernel/signal32.c
> @@ -100,6 +100,9 @@ static int compat_preserve_vfp_context(struct compat_vfp_sigframe __user *frame)
>  	compat_ulong_t fpscr, fpexc;
>  	int i, err = 0;
>  
> +	/* This must not be called when the FP/SIMD is missing */
> +	if (WARN_ON(!system_supports_fpsimd()))
> +		return -EINVAL;
>  	/*
>  	 * Save the hardware registers to the fpsimd_state structure.
>  	 * Note that this also saves V16-31, which aren't visible
> @@ -149,6 +152,10 @@ static int compat_restore_vfp_context(struct compat_vfp_sigframe __user *frame)
>  	compat_ulong_t fpscr;
>  	int i, err = 0;
>  
> +	/* This must not be called when the FP/SIMD is missing */
> +	if (WARN_ON(!system_supports_fpsimd()))
> +		return -EINVAL;
> +
>  	__get_user_error(magic, &frame->magic, err);
>  	__get_user_error(size, &frame->size, err);
>  
> @@ -223,7 +230,7 @@ static int compat_restore_sigframe(struct pt_regs *regs,
>  	err |= !valid_user_regs(&regs->user_regs, current);
>  
>  	aux = (struct compat_aux_sigframe __user *) sf->uc.uc_regspace;
> -	if (err == 0)
> +	if (err == 0 && system_supports_fpsimd())
>  		err |= compat_restore_vfp_context(&aux->vfp);
>  
>  	return err;
> @@ -419,7 +426,7 @@ static int compat_setup_sigframe(struct compat_sigframe __user *sf,
>  
>  	aux = (struct compat_aux_sigframe __user *) sf->uc.uc_regspace;
>  
> -	if (err == 0)
> +	if (err == 0 && system_supports_fpsimd())
>  		err |= compat_preserve_vfp_context(&aux->vfp);
>  	__put_user_error(0, &aux->end_magic, err);

Same comments as for the native signals.

-- 
Catalin

Re: [PATCH v2 6/7] arm64: signal: nofpsimd: Handle fp/simd context for signal frames

[Catalin Marinas]

On Tue, Dec 17, 2019 at 06:34:01PM +0000, Suzuki K Poulose wrote:
> diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
> index dd2cdc0d5be2..c648f7627035 100644
> --- a/arch/arm64/kernel/signal.c
> +++ b/arch/arm64/kernel/signal.c
> @@ -173,6 +173,10 @@ static int preserve_fpsimd_context(struct fpsimd_context __user *ctx)
>  		&current->thread.uw.fpsimd_state;
>  	int err;
>  
> +	/* This must not be called when FP/SIMD support is missing */
> +	if (WARN_ON(!system_supports_fpsimd()))
> +		return -EINVAL;

I'd drop this, see below.

> @@ -191,6 +195,10 @@ static int restore_fpsimd_context(struct fpsimd_context __user *ctx)
>  	__u32 magic, size;
>  	int err = 0;
>  
> +	/* This must not be called when FP/SIMD support is missing */
> +	if (WARN_ON(!system_supports_fpsimd()))
> +		return -EINVAL;
> +
>  	/* check the magic/size information */
>  	__get_user_error(magic, &ctx->head.magic, err);
>  	__get_user_error(size, &ctx->head.size, err);
> @@ -261,6 +269,9 @@ static int restore_sve_fpsimd_context(struct user_ctxs *user)
>  	struct user_fpsimd_state fpsimd;
>  	struct sve_context sve;
>  
> +	if (WARN_ON(!system_supports_fpsimd()))
> +		return -EINVAL;
> +
>  	if (__copy_from_user(&sve, user->sve, sizeof(sve)))
>  		return -EFAULT;
>  
> @@ -371,6 +382,8 @@ static int parse_user_sigframe(struct user_ctxs *user,
>  			goto done;
>  
>  		case FPSIMD_MAGIC:
> +			if (!system_supports_fpsimd())
> +				goto invalid;
>  			if (user->fpsimd)
>  				goto invalid;
>  
> @@ -506,7 +519,7 @@ static int restore_sigframe(struct pt_regs *regs,
>  	if (err == 0)
>  		err = parse_user_sigframe(&user, sf);
>  
> -	if (err == 0) {
> +	if (err == 0 && system_supports_fpsimd()) {
>  		if (!user.fpsimd)
>  			return -EINVAL;

I don't think we need to be over paranoid here and add three/four checks
and two warnings in static functions. parse_user_sigframe() already
returns -EINVAL if SVE or FPSIMD is missing (the latter with your check
above). We don't need this additional check in restore_sigframe() and
restore_{sve_,}fpsimd_context(), the call paths are simple enough.

>  
> @@ -623,7 +636,7 @@ static int setup_sigframe(struct rt_sigframe_user_layout *user,
>  
>  	err |= __copy_to_user(&sf->uc.uc_sigmask, set, sizeof(*set));
>  
> -	if (err == 0) {
> +	if (err == 0 && system_supports_fpsimd()) {
>  		struct fpsimd_context __user *fpsimd_ctx =
>  			apply_user_offset(user, user->fpsimd_offset);
>  		err |= preserve_fpsimd_context(fpsimd_ctx);

This check is also sufficient for a static function not to have the
WARN_ON.

> diff --git a/arch/arm64/kernel/signal32.c b/arch/arm64/kernel/signal32.c
> index 12a585386c2f..97ace6919bc2 100644
> --- a/arch/arm64/kernel/signal32.c
> +++ b/arch/arm64/kernel/signal32.c
> @@ -100,6 +100,9 @@ static int compat_preserve_vfp_context(struct compat_vfp_sigframe __user *frame)
>  	compat_ulong_t fpscr, fpexc;
>  	int i, err = 0;
>  
> +	/* This must not be called when the FP/SIMD is missing */
> +	if (WARN_ON(!system_supports_fpsimd()))
> +		return -EINVAL;
>  	/*
>  	 * Save the hardware registers to the fpsimd_state structure.
>  	 * Note that this also saves V16-31, which aren't visible
> @@ -149,6 +152,10 @@ static int compat_restore_vfp_context(struct compat_vfp_sigframe __user *frame)
>  	compat_ulong_t fpscr;
>  	int i, err = 0;
>  
> +	/* This must not be called when the FP/SIMD is missing */
> +	if (WARN_ON(!system_supports_fpsimd()))
> +		return -EINVAL;
> +
>  	__get_user_error(magic, &frame->magic, err);
>  	__get_user_error(size, &frame->size, err);
>  
> @@ -223,7 +230,7 @@ static int compat_restore_sigframe(struct pt_regs *regs,
>  	err |= !valid_user_regs(&regs->user_regs, current);
>  
>  	aux = (struct compat_aux_sigframe __user *) sf->uc.uc_regspace;
> -	if (err == 0)
> +	if (err == 0 && system_supports_fpsimd())
>  		err |= compat_restore_vfp_context(&aux->vfp);
>  
>  	return err;
> @@ -419,7 +426,7 @@ static int compat_setup_sigframe(struct compat_sigframe __user *sf,
>  
>  	aux = (struct compat_aux_sigframe __user *) sf->uc.uc_regspace;
>  
> -	if (err == 0)
> +	if (err == 0 && system_supports_fpsimd())
>  		err |= compat_preserve_vfp_context(&aux->vfp);
>  	__put_user_error(0, &aux->end_magic, err);

Same comments as for the native signals.

-- 
Catalin

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

[PATCH v2 7/7] arm64: nofpsmid: Handle TIF_FOREIGN_FPSTATE flag cleanly

[Suzuki K Poulose]

We detect the absence of FP/SIMD after an incapable CPU is brought up,
and by then we have kernel threads running already with TIF_FOREIGN_FPSTATE set
which could be set for early userspace applications (e.g, modprobe triggered
from initramfs) and init. This could cause the applications to loop forever in
do_nofity_resume() as we never clear the TIF flag, once we now know that
we don't support FP.

Fix this by making sure that we clear the TIF_FOREIGN_FPSTATE flag
for tasks which may have them set, as we would have done in the normal
case, but avoiding touching the hardware state (since we don't support any).

Also to make sure we handle the cases seemlessly we categorise the
helper functions to two :
 1) Helpers for common core code, which calls into take appropriate
    actions without knowing the current FPSIMD state of the CPU/task.

    e.g fpsimd_restore_current_state(), fpsimd_flush_task_state(),
        fpsimd_save_and_flush_cpu_state().

    We bail out early for these functions, taking any appropriate actions
    (e.g, clearing the TIF flag) where necessary to hide the handling
    from core code.

 2) Helpers used when the presence of FP/SIMD is apparent.
    i.e, save/restore the FP/SIMD register state, modify the CPU/task
    FP/SIMD state.
    e.g,

    fpsimd_save(), task_fpsimd_load() - save/restore task FP/SIMD registers

    fpsimd_bind_task_to_cpu()  \
                                - Update the "state" metadata for CPU/task.
    fpsimd_bind_state_to_cpu() /

    fpsimd_update_current_state() - Update the fp/simd state for the current
                                    task from memory.

    These must not be called in the absence of FP/SIMD. Put in a WARNING
    to make sure they are not invoked in the absence of FP/SIMD.

KVM also uses the TIF_FOREIGN_FPSTATE flag to manage the FP/SIMD state
on the CPU. However, without FP/SIMD support we trap all accesses and
inject undefined instruction. Thus we should never "load" guest state.
Add a sanity check to make sure this is valid.

Fixes: 82e0191a1aa11abf ("arm64: Support systems without FP/ASIMD")
Cc: Will Deacon <will@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 arch/arm64/kernel/fpsimd.c  | 31 +++++++++++++++++++++++++++----
 arch/arm64/kvm/hyp/switch.c |  9 +++++++++
 2 files changed, 36 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index 3eb338f14386..240c52b71cda 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -269,7 +269,7 @@ static void sve_free(struct task_struct *task)
  */
 static void task_fpsimd_load(void)
 {
-	WARN_ON(!have_cpu_fpsimd_context());
+	WARN_ON(!system_supports_fpsimd() || !have_cpu_fpsimd_context());
 
 	if (system_supports_sve() && test_thread_flag(TIF_SVE))
 		sve_load_state(sve_pffr(&current->thread),
@@ -289,6 +289,7 @@ static void fpsimd_save(void)
 		this_cpu_ptr(&fpsimd_last_state);
 	/* set by fpsimd_bind_task_to_cpu() or fpsimd_bind_state_to_cpu() */
 
+	WARN_ON(!system_supports_fpsimd());
 	WARN_ON(!have_cpu_fpsimd_context());
 
 	if (!test_thread_flag(TIF_FOREIGN_FPSTATE)) {
@@ -1092,6 +1093,7 @@ void fpsimd_bind_task_to_cpu(void)
 	struct fpsimd_last_state_struct *last =
 		this_cpu_ptr(&fpsimd_last_state);
 
+	WARN_ON(!system_supports_fpsimd());
 	last->st = &current->thread.uw.fpsimd_state;
 	last->sve_state = current->thread.sve_state;
 	last->sve_vl = current->thread.sve_vl;
@@ -1114,6 +1116,7 @@ void fpsimd_bind_state_to_cpu(struct user_fpsimd_state *st, void *sve_state,
 	struct fpsimd_last_state_struct *last =
 		this_cpu_ptr(&fpsimd_last_state);
 
+	WARN_ON(!system_supports_fpsimd());
 	WARN_ON(!in_softirq() && !irqs_disabled());
 
 	last->st = st;
@@ -1128,8 +1131,19 @@ void fpsimd_bind_state_to_cpu(struct user_fpsimd_state *st, void *sve_state,
  */
 void fpsimd_restore_current_state(void)
 {
-	if (!system_supports_fpsimd())
+	/*
+	 * For the tasks that were created before we detected the absence of
+	 * FP/SIMD, the TIF_FOREIGN_FPSTATE could be set via fpsimd_thread_switch(),
+	 * e.g, init. This could be then inherited by the children processes.
+	 * If we later detect that the system doesn't support FP/SIMD,
+	 * we must clear the flag for  all the tasks to indicate that the
+	 * FPSTATE is clean (as we can't have one) to avoid looping for ever in
+	 * do_notify_resume().
+	 */
+	if (!system_supports_fpsimd()) {
+		clear_thread_flag(TIF_FOREIGN_FPSTATE);
 		return;
+	}
 
 	get_cpu_fpsimd_context();
 
@@ -1148,7 +1162,7 @@ void fpsimd_restore_current_state(void)
  */
 void fpsimd_update_current_state(struct user_fpsimd_state const *state)
 {
-	if (!system_supports_fpsimd())
+	if (WARN_ON(!system_supports_fpsimd()))
 		return;
 
 	get_cpu_fpsimd_context();
@@ -1179,7 +1193,13 @@ void fpsimd_update_current_state(struct user_fpsimd_state const *state)
 void fpsimd_flush_task_state(struct task_struct *t)
 {
 	t->thread.fpsimd_cpu = NR_CPUS;
-
+	/*
+	 * If we don't support fpsimd, bail out after we have
+	 * reset the fpsimd_cpu for this task and clear the
+	 * FPSTATE.
+	 */
+	if (!system_supports_fpsimd())
+		return;
 	barrier();
 	set_tsk_thread_flag(t, TIF_FOREIGN_FPSTATE);
 
@@ -1193,6 +1213,7 @@ void fpsimd_flush_task_state(struct task_struct *t)
  */
 static void fpsimd_flush_cpu_state(void)
 {
+	WARN_ON(!system_supports_fpsimd());
 	__this_cpu_write(fpsimd_last_state.st, NULL);
 	set_thread_flag(TIF_FOREIGN_FPSTATE);
 }
@@ -1203,6 +1224,8 @@ static void fpsimd_flush_cpu_state(void)
  */
 void fpsimd_save_and_flush_cpu_state(void)
 {
+	if (!system_supports_fpsimd())
+		return;
 	WARN_ON(preemptible());
 	__get_cpu_fpsimd_context();
 	fpsimd_save();
diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
index 72fbbd86eb5e..9696ebb5c13a 100644
--- a/arch/arm64/kvm/hyp/switch.c
+++ b/arch/arm64/kvm/hyp/switch.c
@@ -28,10 +28,19 @@
 /* Check whether the FP regs were dirtied while in the host-side run loop: */
 static bool __hyp_text update_fp_enabled(struct kvm_vcpu *vcpu)
 {
+	/*
+	 * When the system doesn't support FP/SIMD, we cannot rely on
+	 * the state of _TIF_FOREIGN_FPSTATE. However, we will never
+	 * set the KVM_ARM64_FP_ENABLED, as the FP/SIMD accesses always
+	 * inject an abort into the guest. Thus we always trap the
+	 * accesses.
+	 */
 	if (vcpu->arch.host_thread_info->flags & _TIF_FOREIGN_FPSTATE)
 		vcpu->arch.flags &= ~(KVM_ARM64_FP_ENABLED |
 				      KVM_ARM64_FP_HOST);
 
+	WARN_ON(!system_supports_fpsimd() &&
+		(vcpu->arch.flags & KVM_ARM64_FP_ENABLED));
 	return !!(vcpu->arch.flags & KVM_ARM64_FP_ENABLED);
 }
 
-- 
2.23.0

[PATCH v2 7/7] arm64: nofpsmid: Handle TIF_FOREIGN_FPSTATE flag cleanly

[Suzuki K Poulose]

We detect the absence of FP/SIMD after an incapable CPU is brought up,
and by then we have kernel threads running already with TIF_FOREIGN_FPSTATE set
which could be set for early userspace applications (e.g, modprobe triggered
from initramfs) and init. This could cause the applications to loop forever in
do_nofity_resume() as we never clear the TIF flag, once we now know that
we don't support FP.

Fix this by making sure that we clear the TIF_FOREIGN_FPSTATE flag
for tasks which may have them set, as we would have done in the normal
case, but avoiding touching the hardware state (since we don't support any).

Also to make sure we handle the cases seemlessly we categorise the
helper functions to two :
 1) Helpers for common core code, which calls into take appropriate
    actions without knowing the current FPSIMD state of the CPU/task.

    e.g fpsimd_restore_current_state(), fpsimd_flush_task_state(),
        fpsimd_save_and_flush_cpu_state().

    We bail out early for these functions, taking any appropriate actions
    (e.g, clearing the TIF flag) where necessary to hide the handling
    from core code.

 2) Helpers used when the presence of FP/SIMD is apparent.
    i.e, save/restore the FP/SIMD register state, modify the CPU/task
    FP/SIMD state.
    e.g,

    fpsimd_save(), task_fpsimd_load() - save/restore task FP/SIMD registers

    fpsimd_bind_task_to_cpu()  \
                                - Update the "state" metadata for CPU/task.
    fpsimd_bind_state_to_cpu() /

    fpsimd_update_current_state() - Update the fp/simd state for the current
                                    task from memory.

    These must not be called in the absence of FP/SIMD. Put in a WARNING
    to make sure they are not invoked in the absence of FP/SIMD.

KVM also uses the TIF_FOREIGN_FPSTATE flag to manage the FP/SIMD state
on the CPU. However, without FP/SIMD support we trap all accesses and
inject undefined instruction. Thus we should never "load" guest state.
Add a sanity check to make sure this is valid.

Fixes: 82e0191a1aa11abf ("arm64: Support systems without FP/ASIMD")
Cc: Will Deacon <will@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 arch/arm64/kernel/fpsimd.c  | 31 +++++++++++++++++++++++++++----
 arch/arm64/kvm/hyp/switch.c |  9 +++++++++
 2 files changed, 36 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index 3eb338f14386..240c52b71cda 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -269,7 +269,7 @@ static void sve_free(struct task_struct *task)
  */
 static void task_fpsimd_load(void)
 {
-	WARN_ON(!have_cpu_fpsimd_context());
+	WARN_ON(!system_supports_fpsimd() || !have_cpu_fpsimd_context());
 
 	if (system_supports_sve() && test_thread_flag(TIF_SVE))
 		sve_load_state(sve_pffr(&current->thread),
@@ -289,6 +289,7 @@ static void fpsimd_save(void)
 		this_cpu_ptr(&fpsimd_last_state);
 	/* set by fpsimd_bind_task_to_cpu() or fpsimd_bind_state_to_cpu() */
 
+	WARN_ON(!system_supports_fpsimd());
 	WARN_ON(!have_cpu_fpsimd_context());
 
 	if (!test_thread_flag(TIF_FOREIGN_FPSTATE)) {
@@ -1092,6 +1093,7 @@ void fpsimd_bind_task_to_cpu(void)
 	struct fpsimd_last_state_struct *last =
 		this_cpu_ptr(&fpsimd_last_state);
 
+	WARN_ON(!system_supports_fpsimd());
 	last->st = &current->thread.uw.fpsimd_state;
 	last->sve_state = current->thread.sve_state;
 	last->sve_vl = current->thread.sve_vl;
@@ -1114,6 +1116,7 @@ void fpsimd_bind_state_to_cpu(struct user_fpsimd_state *st, void *sve_state,
 	struct fpsimd_last_state_struct *last =
 		this_cpu_ptr(&fpsimd_last_state);
 
+	WARN_ON(!system_supports_fpsimd());
 	WARN_ON(!in_softirq() && !irqs_disabled());
 
 	last->st = st;
@@ -1128,8 +1131,19 @@ void fpsimd_bind_state_to_cpu(struct user_fpsimd_state *st, void *sve_state,
  */
 void fpsimd_restore_current_state(void)
 {
-	if (!system_supports_fpsimd())
+	/*
+	 * For the tasks that were created before we detected the absence of
+	 * FP/SIMD, the TIF_FOREIGN_FPSTATE could be set via fpsimd_thread_switch(),
+	 * e.g, init. This could be then inherited by the children processes.
+	 * If we later detect that the system doesn't support FP/SIMD,
+	 * we must clear the flag for  all the tasks to indicate that the
+	 * FPSTATE is clean (as we can't have one) to avoid looping for ever in
+	 * do_notify_resume().
+	 */
+	if (!system_supports_fpsimd()) {
+		clear_thread_flag(TIF_FOREIGN_FPSTATE);
 		return;
+	}
 
 	get_cpu_fpsimd_context();
 
@@ -1148,7 +1162,7 @@ void fpsimd_restore_current_state(void)
  */
 void fpsimd_update_current_state(struct user_fpsimd_state const *state)
 {
-	if (!system_supports_fpsimd())
+	if (WARN_ON(!system_supports_fpsimd()))
 		return;
 
 	get_cpu_fpsimd_context();
@@ -1179,7 +1193,13 @@ void fpsimd_update_current_state(struct user_fpsimd_state const *state)
 void fpsimd_flush_task_state(struct task_struct *t)
 {
 	t->thread.fpsimd_cpu = NR_CPUS;
-
+	/*
+	 * If we don't support fpsimd, bail out after we have
+	 * reset the fpsimd_cpu for this task and clear the
+	 * FPSTATE.
+	 */
+	if (!system_supports_fpsimd())
+		return;
 	barrier();
 	set_tsk_thread_flag(t, TIF_FOREIGN_FPSTATE);
 
@@ -1193,6 +1213,7 @@ void fpsimd_flush_task_state(struct task_struct *t)
  */
 static void fpsimd_flush_cpu_state(void)
 {
+	WARN_ON(!system_supports_fpsimd());
 	__this_cpu_write(fpsimd_last_state.st, NULL);
 	set_thread_flag(TIF_FOREIGN_FPSTATE);
 }
@@ -1203,6 +1224,8 @@ static void fpsimd_flush_cpu_state(void)
  */
 void fpsimd_save_and_flush_cpu_state(void)
 {
+	if (!system_supports_fpsimd())
+		return;
 	WARN_ON(preemptible());
 	__get_cpu_fpsimd_context();
 	fpsimd_save();
diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
index 72fbbd86eb5e..9696ebb5c13a 100644
--- a/arch/arm64/kvm/hyp/switch.c
+++ b/arch/arm64/kvm/hyp/switch.c
@@ -28,10 +28,19 @@
 /* Check whether the FP regs were dirtied while in the host-side run loop: */
 static bool __hyp_text update_fp_enabled(struct kvm_vcpu *vcpu)
 {
+	/*
+	 * When the system doesn't support FP/SIMD, we cannot rely on
+	 * the state of _TIF_FOREIGN_FPSTATE. However, we will never
+	 * set the KVM_ARM64_FP_ENABLED, as the FP/SIMD accesses always
+	 * inject an abort into the guest. Thus we always trap the
+	 * accesses.
+	 */
 	if (vcpu->arch.host_thread_info->flags & _TIF_FOREIGN_FPSTATE)
 		vcpu->arch.flags &= ~(KVM_ARM64_FP_ENABLED |
 				      KVM_ARM64_FP_HOST);
 
+	WARN_ON(!system_supports_fpsimd() &&
+		(vcpu->arch.flags & KVM_ARM64_FP_ENABLED));
 	return !!(vcpu->arch.flags & KVM_ARM64_FP_ENABLED);
 }
 
-- 
2.23.0


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

Re: [PATCH v2 7/7] arm64: nofpsmid: Handle TIF_FOREIGN_FPSTATE flag cleanly

[Marc Zyngier]

Hi Suzuki,

On 2019-12-17 18:34, Suzuki K Poulose wrote:
> We detect the absence of FP/SIMD after an incapable CPU is brought 
> up,
> and by then we have kernel threads running already with
> TIF_FOREIGN_FPSTATE set
> which could be set for early userspace applications (e.g, modprobe 
> triggered
> from initramfs) and init. This could cause the applications to loop
> forever in
> do_nofity_resume() as we never clear the TIF flag, once we now know 
> that
> we don't support FP.
>
> Fix this by making sure that we clear the TIF_FOREIGN_FPSTATE flag
> for tasks which may have them set, as we would have done in the 
> normal
> case, but avoiding touching the hardware state (since we don't 
> support any).
>
> Also to make sure we handle the cases seemlessly we categorise the
> helper functions to two :
>  1) Helpers for common core code, which calls into take appropriate
>     actions without knowing the current FPSIMD state of the CPU/task.
>
>     e.g fpsimd_restore_current_state(), fpsimd_flush_task_state(),
>         fpsimd_save_and_flush_cpu_state().
>
>     We bail out early for these functions, taking any appropriate 
> actions
>     (e.g, clearing the TIF flag) where necessary to hide the handling
>     from core code.
>
>  2) Helpers used when the presence of FP/SIMD is apparent.
>     i.e, save/restore the FP/SIMD register state, modify the CPU/task
>     FP/SIMD state.
>     e.g,
>
>     fpsimd_save(), task_fpsimd_load() - save/restore task FP/SIMD 
> registers
>
>     fpsimd_bind_task_to_cpu()  \
>                                 - Update the "state" metadata for 
> CPU/task.
>     fpsimd_bind_state_to_cpu() /
>
>     fpsimd_update_current_state() - Update the fp/simd state for the 
> current
>                                     task from memory.
>
>     These must not be called in the absence of FP/SIMD. Put in a 
> WARNING
>     to make sure they are not invoked in the absence of FP/SIMD.
>
> KVM also uses the TIF_FOREIGN_FPSTATE flag to manage the FP/SIMD 
> state
> on the CPU. However, without FP/SIMD support we trap all accesses and
> inject undefined instruction. Thus we should never "load" guest 
> state.
> Add a sanity check to make sure this is valid.

Yes, but no, see below.

>
> Fixes: 82e0191a1aa11abf ("arm64: Support systems without FP/ASIMD")
> Cc: Will Deacon <will@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Marc Zyngier <marc.zyngier@arm.com>

No idea who that guy is. It's a fake! ;-)

> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
>  arch/arm64/kernel/fpsimd.c  | 31 +++++++++++++++++++++++++++----
>  arch/arm64/kvm/hyp/switch.c |  9 +++++++++
>  2 files changed, 36 insertions(+), 4 deletions(-)
>

[...]

> diff --git a/arch/arm64/kvm/hyp/switch.c 
> b/arch/arm64/kvm/hyp/switch.c
> index 72fbbd86eb5e..9696ebb5c13a 100644
> --- a/arch/arm64/kvm/hyp/switch.c
> +++ b/arch/arm64/kvm/hyp/switch.c
> @@ -28,10 +28,19 @@
>  /* Check whether the FP regs were dirtied while in the host-side run
> loop: */
>  static bool __hyp_text update_fp_enabled(struct kvm_vcpu *vcpu)
>  {
> +	/*
> +	 * When the system doesn't support FP/SIMD, we cannot rely on
> +	 * the state of _TIF_FOREIGN_FPSTATE. However, we will never
> +	 * set the KVM_ARM64_FP_ENABLED, as the FP/SIMD accesses always
> +	 * inject an abort into the guest. Thus we always trap the
> +	 * accesses.
> +	 */
>  	if (vcpu->arch.host_thread_info->flags & _TIF_FOREIGN_FPSTATE)
>  		vcpu->arch.flags &= ~(KVM_ARM64_FP_ENABLED |
>  				      KVM_ARM64_FP_HOST);
>
> +	WARN_ON(!system_supports_fpsimd() &&
> +		(vcpu->arch.flags & KVM_ARM64_FP_ENABLED));

Careful, this will panic the host if it happens on a !VHE host
(calling non-inline stuff from a __hyp_text function is usually
not a good idea).

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v2 7/7] arm64: nofpsmid: Handle TIF_FOREIGN_FPSTATE flag cleanly

[Marc Zyngier]

Hi Suzuki,

On 2019-12-17 18:34, Suzuki K Poulose wrote:
> We detect the absence of FP/SIMD after an incapable CPU is brought 
> up,
> and by then we have kernel threads running already with
> TIF_FOREIGN_FPSTATE set
> which could be set for early userspace applications (e.g, modprobe 
> triggered
> from initramfs) and init. This could cause the applications to loop
> forever in
> do_nofity_resume() as we never clear the TIF flag, once we now know 
> that
> we don't support FP.
>
> Fix this by making sure that we clear the TIF_FOREIGN_FPSTATE flag
> for tasks which may have them set, as we would have done in the 
> normal
> case, but avoiding touching the hardware state (since we don't 
> support any).
>
> Also to make sure we handle the cases seemlessly we categorise the
> helper functions to two :
>  1) Helpers for common core code, which calls into take appropriate
>     actions without knowing the current FPSIMD state of the CPU/task.
>
>     e.g fpsimd_restore_current_state(), fpsimd_flush_task_state(),
>         fpsimd_save_and_flush_cpu_state().
>
>     We bail out early for these functions, taking any appropriate 
> actions
>     (e.g, clearing the TIF flag) where necessary to hide the handling
>     from core code.
>
>  2) Helpers used when the presence of FP/SIMD is apparent.
>     i.e, save/restore the FP/SIMD register state, modify the CPU/task
>     FP/SIMD state.
>     e.g,
>
>     fpsimd_save(), task_fpsimd_load() - save/restore task FP/SIMD 
> registers
>
>     fpsimd_bind_task_to_cpu()  \
>                                 - Update the "state" metadata for 
> CPU/task.
>     fpsimd_bind_state_to_cpu() /
>
>     fpsimd_update_current_state() - Update the fp/simd state for the 
> current
>                                     task from memory.
>
>     These must not be called in the absence of FP/SIMD. Put in a 
> WARNING
>     to make sure they are not invoked in the absence of FP/SIMD.
>
> KVM also uses the TIF_FOREIGN_FPSTATE flag to manage the FP/SIMD 
> state
> on the CPU. However, without FP/SIMD support we trap all accesses and
> inject undefined instruction. Thus we should never "load" guest 
> state.
> Add a sanity check to make sure this is valid.

Yes, but no, see below.

>
> Fixes: 82e0191a1aa11abf ("arm64: Support systems without FP/ASIMD")
> Cc: Will Deacon <will@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Marc Zyngier <marc.zyngier@arm.com>

No idea who that guy is. It's a fake! ;-)

> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
>  arch/arm64/kernel/fpsimd.c  | 31 +++++++++++++++++++++++++++----
>  arch/arm64/kvm/hyp/switch.c |  9 +++++++++
>  2 files changed, 36 insertions(+), 4 deletions(-)
>

[...]

> diff --git a/arch/arm64/kvm/hyp/switch.c 
> b/arch/arm64/kvm/hyp/switch.c
> index 72fbbd86eb5e..9696ebb5c13a 100644
> --- a/arch/arm64/kvm/hyp/switch.c
> +++ b/arch/arm64/kvm/hyp/switch.c
> @@ -28,10 +28,19 @@
>  /* Check whether the FP regs were dirtied while in the host-side run
> loop: */
>  static bool __hyp_text update_fp_enabled(struct kvm_vcpu *vcpu)
>  {
> +	/*
> +	 * When the system doesn't support FP/SIMD, we cannot rely on
> +	 * the state of _TIF_FOREIGN_FPSTATE. However, we will never
> +	 * set the KVM_ARM64_FP_ENABLED, as the FP/SIMD accesses always
> +	 * inject an abort into the guest. Thus we always trap the
> +	 * accesses.
> +	 */
>  	if (vcpu->arch.host_thread_info->flags & _TIF_FOREIGN_FPSTATE)
>  		vcpu->arch.flags &= ~(KVM_ARM64_FP_ENABLED |
>  				      KVM_ARM64_FP_HOST);
>
> +	WARN_ON(!system_supports_fpsimd() &&
> +		(vcpu->arch.flags & KVM_ARM64_FP_ENABLED));

Careful, this will panic the host if it happens on a !VHE host
(calling non-inline stuff from a __hyp_text function is usually
not a good idea).

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

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

Re: [PATCH v2 7/7] arm64: nofpsmid: Handle TIF_FOREIGN_FPSTATE flag cleanly

[Suzuki Kuruppassery Poulose]

Hi Marc,

On 17/12/2019 19:05, Marc Zyngier wrote:
> Hi Suzuki,
> 
> On 2019-12-17 18:34, Suzuki K Poulose wrote:
>> We detect the absence of FP/SIMD after an incapable CPU is brought up,
>> and by then we have kernel threads running already with
>> TIF_FOREIGN_FPSTATE set
>> which could be set for early userspace applications (e.g, modprobe 
>> triggered
>> from initramfs) and init. This could cause the applications to loop
>> forever in
>> do_nofity_resume() as we never clear the TIF flag, once we now know that
>> we don't support FP.
>>
>> Fix this by making sure that we clear the TIF_FOREIGN_FPSTATE flag
>> for tasks which may have them set, as we would have done in the normal
>> case, but avoiding touching the hardware state (since we don't support 
>> any).
>>
>> Also to make sure we handle the cases seemlessly we categorise the
>> helper functions to two :
>>  1) Helpers for common core code, which calls into take appropriate
>>     actions without knowing the current FPSIMD state of the CPU/task.
>>
>>     e.g fpsimd_restore_current_state(), fpsimd_flush_task_state(),
>>         fpsimd_save_and_flush_cpu_state().
>>
>>     We bail out early for these functions, taking any appropriate actions
>>     (e.g, clearing the TIF flag) where necessary to hide the handling
>>     from core code.
>>
>>  2) Helpers used when the presence of FP/SIMD is apparent.
>>     i.e, save/restore the FP/SIMD register state, modify the CPU/task
>>     FP/SIMD state.
>>     e.g,
>>
>>     fpsimd_save(), task_fpsimd_load() - save/restore task FP/SIMD 
>> registers
>>
>>     fpsimd_bind_task_to_cpu()  \
>>                                 - Update the "state" metadata for 
>> CPU/task.
>>     fpsimd_bind_state_to_cpu() /
>>
>>     fpsimd_update_current_state() - Update the fp/simd state for the 
>> current
>>                                     task from memory.
>>
>>     These must not be called in the absence of FP/SIMD. Put in a WARNING
>>     to make sure they are not invoked in the absence of FP/SIMD.
>>
>> KVM also uses the TIF_FOREIGN_FPSTATE flag to manage the FP/SIMD state
>> on the CPU. However, without FP/SIMD support we trap all accesses and
>> inject undefined instruction. Thus we should never "load" guest state.
>> Add a sanity check to make sure this is valid.
> 
> Yes, but no, see below.
> 
>>
>> Fixes: 82e0191a1aa11abf ("arm64: Support systems without FP/ASIMD")
>> Cc: Will Deacon <will@kernel.org>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Marc Zyngier <marc.zyngier@arm.com>
> 
> No idea who that guy is. It's a fake! ;-)

Sorry about that, will fix it.

> 
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>> ---
>>  arch/arm64/kernel/fpsimd.c  | 31 +++++++++++++++++++++++++++----
>>  arch/arm64/kvm/hyp/switch.c |  9 +++++++++
>>  2 files changed, 36 insertions(+), 4 deletions(-)
>>
> 
> [...]
> 
>> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
>> index 72fbbd86eb5e..9696ebb5c13a 100644
>> --- a/arch/arm64/kvm/hyp/switch.c
>> +++ b/arch/arm64/kvm/hyp/switch.c
>> @@ -28,10 +28,19 @@
>>  /* Check whether the FP regs were dirtied while in the host-side run
>> loop: */
>>  static bool __hyp_text update_fp_enabled(struct kvm_vcpu *vcpu)
>>  {
>> +    /*
>> +     * When the system doesn't support FP/SIMD, we cannot rely on
>> +     * the state of _TIF_FOREIGN_FPSTATE. However, we will never
>> +     * set the KVM_ARM64_FP_ENABLED, as the FP/SIMD accesses always
>> +     * inject an abort into the guest. Thus we always trap the
>> +     * accesses.
>> +     */
>>      if (vcpu->arch.host_thread_info->flags & _TIF_FOREIGN_FPSTATE)
>>          vcpu->arch.flags &= ~(KVM_ARM64_FP_ENABLED |
>>                        KVM_ARM64_FP_HOST);
>>
>> +    WARN_ON(!system_supports_fpsimd() &&
>> +        (vcpu->arch.flags & KVM_ARM64_FP_ENABLED));
> 
> Careful, this will panic the host if it happens on a !VHE host
> (calling non-inline stuff from a __hyp_text function is usually
> not a good idea).

Ouch! Sorry about that WARN_ON()! I could drop the warning and
make this :

	if (!system_supports_fpsimd() ||
	    (vcpu->arch.host_thread_info->flags & _TIF_FOREIGN_FPSTATE))
		vcpu->arch.flags &= ~(KVM_ARM64_FP_ENABLED |
				      KVM_ARM64_FP_HOST);

to make sure we never say fp is enabled.

What do you think ?

Cheers
Suzuki

Re: [PATCH v2 7/7] arm64: nofpsmid: Handle TIF_FOREIGN_FPSTATE flag cleanly

[Suzuki Kuruppassery Poulose]

Hi Marc,

On 17/12/2019 19:05, Marc Zyngier wrote:
> Hi Suzuki,
> 
> On 2019-12-17 18:34, Suzuki K Poulose wrote:
>> We detect the absence of FP/SIMD after an incapable CPU is brought up,
>> and by then we have kernel threads running already with
>> TIF_FOREIGN_FPSTATE set
>> which could be set for early userspace applications (e.g, modprobe 
>> triggered
>> from initramfs) and init. This could cause the applications to loop
>> forever in
>> do_nofity_resume() as we never clear the TIF flag, once we now know that
>> we don't support FP.
>>
>> Fix this by making sure that we clear the TIF_FOREIGN_FPSTATE flag
>> for tasks which may have them set, as we would have done in the normal
>> case, but avoiding touching the hardware state (since we don't support 
>> any).
>>
>> Also to make sure we handle the cases seemlessly we categorise the
>> helper functions to two :
>>  1) Helpers for common core code, which calls into take appropriate
>>     actions without knowing the current FPSIMD state of the CPU/task.
>>
>>     e.g fpsimd_restore_current_state(), fpsimd_flush_task_state(),
>>         fpsimd_save_and_flush_cpu_state().
>>
>>     We bail out early for these functions, taking any appropriate actions
>>     (e.g, clearing the TIF flag) where necessary to hide the handling
>>     from core code.
>>
>>  2) Helpers used when the presence of FP/SIMD is apparent.
>>     i.e, save/restore the FP/SIMD register state, modify the CPU/task
>>     FP/SIMD state.
>>     e.g,
>>
>>     fpsimd_save(), task_fpsimd_load() - save/restore task FP/SIMD 
>> registers
>>
>>     fpsimd_bind_task_to_cpu()  \
>>                                 - Update the "state" metadata for 
>> CPU/task.
>>     fpsimd_bind_state_to_cpu() /
>>
>>     fpsimd_update_current_state() - Update the fp/simd state for the 
>> current
>>                                     task from memory.
>>
>>     These must not be called in the absence of FP/SIMD. Put in a WARNING
>>     to make sure they are not invoked in the absence of FP/SIMD.
>>
>> KVM also uses the TIF_FOREIGN_FPSTATE flag to manage the FP/SIMD state
>> on the CPU. However, without FP/SIMD support we trap all accesses and
>> inject undefined instruction. Thus we should never "load" guest state.
>> Add a sanity check to make sure this is valid.
> 
> Yes, but no, see below.
> 
>>
>> Fixes: 82e0191a1aa11abf ("arm64: Support systems without FP/ASIMD")
>> Cc: Will Deacon <will@kernel.org>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Marc Zyngier <marc.zyngier@arm.com>
> 
> No idea who that guy is. It's a fake! ;-)

Sorry about that, will fix it.

> 
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>> ---
>>  arch/arm64/kernel/fpsimd.c  | 31 +++++++++++++++++++++++++++----
>>  arch/arm64/kvm/hyp/switch.c |  9 +++++++++
>>  2 files changed, 36 insertions(+), 4 deletions(-)
>>
> 
> [...]
> 
>> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
>> index 72fbbd86eb5e..9696ebb5c13a 100644
>> --- a/arch/arm64/kvm/hyp/switch.c
>> +++ b/arch/arm64/kvm/hyp/switch.c
>> @@ -28,10 +28,19 @@
>>  /* Check whether the FP regs were dirtied while in the host-side run
>> loop: */
>>  static bool __hyp_text update_fp_enabled(struct kvm_vcpu *vcpu)
>>  {
>> +    /*
>> +     * When the system doesn't support FP/SIMD, we cannot rely on
>> +     * the state of _TIF_FOREIGN_FPSTATE. However, we will never
>> +     * set the KVM_ARM64_FP_ENABLED, as the FP/SIMD accesses always
>> +     * inject an abort into the guest. Thus we always trap the
>> +     * accesses.
>> +     */
>>      if (vcpu->arch.host_thread_info->flags & _TIF_FOREIGN_FPSTATE)
>>          vcpu->arch.flags &= ~(KVM_ARM64_FP_ENABLED |
>>                        KVM_ARM64_FP_HOST);
>>
>> +    WARN_ON(!system_supports_fpsimd() &&
>> +        (vcpu->arch.flags & KVM_ARM64_FP_ENABLED));
> 
> Careful, this will panic the host if it happens on a !VHE host
> (calling non-inline stuff from a __hyp_text function is usually
> not a good idea).

Ouch! Sorry about that WARN_ON()! I could drop the warning and
make this :

	if (!system_supports_fpsimd() ||
	    (vcpu->arch.host_thread_info->flags & _TIF_FOREIGN_FPSTATE))
		vcpu->arch.flags &= ~(KVM_ARM64_FP_ENABLED |
				      KVM_ARM64_FP_HOST);

to make sure we never say fp is enabled.

What do you think ?

Cheers
Suzuki

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

Re: [PATCH v2 7/7] arm64: nofpsmid: Handle TIF_FOREIGN_FPSTATE flag cleanly

[Marc Zyngier]

On 2019-12-18 11:42, Suzuki Kuruppassery Poulose wrote:
> Hi Marc,
>
> On 17/12/2019 19:05, Marc Zyngier wrote:
>> Hi Suzuki,
>> On 2019-12-17 18:34, Suzuki K Poulose wrote:
>>> We detect the absence of FP/SIMD after an incapable CPU is brought 
>>> up,
>>> and by then we have kernel threads running already with
>>> TIF_FOREIGN_FPSTATE set
>>> which could be set for early userspace applications (e.g, modprobe 
>>> triggered
>>> from initramfs) and init. This could cause the applications to loop
>>> forever in
>>> do_nofity_resume() as we never clear the TIF flag, once we now know 
>>> that
>>> we don't support FP.
>>>
>>> Fix this by making sure that we clear the TIF_FOREIGN_FPSTATE flag
>>> for tasks which may have them set, as we would have done in the 
>>> normal
>>> case, but avoiding touching the hardware state (since we don't 
>>> support any).
>>>
>>> Also to make sure we handle the cases seemlessly we categorise the
>>> helper functions to two :
>>>  1) Helpers for common core code, which calls into take appropriate
>>>     actions without knowing the current FPSIMD state of the 
>>> CPU/task.
>>>
>>>     e.g fpsimd_restore_current_state(), fpsimd_flush_task_state(),
>>>         fpsimd_save_and_flush_cpu_state().
>>>
>>>     We bail out early for these functions, taking any appropriate 
>>> actions
>>>     (e.g, clearing the TIF flag) where necessary to hide the 
>>> handling
>>>     from core code.
>>>
>>>  2) Helpers used when the presence of FP/SIMD is apparent.
>>>     i.e, save/restore the FP/SIMD register state, modify the 
>>> CPU/task
>>>     FP/SIMD state.
>>>     e.g,
>>>
>>>     fpsimd_save(), task_fpsimd_load() - save/restore task FP/SIMD 
>>> registers
>>>
>>>     fpsimd_bind_task_to_cpu()  \
>>>                                 - Update the "state" metadata for 
>>> CPU/task.
>>>     fpsimd_bind_state_to_cpu() /
>>>
>>>     fpsimd_update_current_state() - Update the fp/simd state for 
>>> the current
>>>                                     task from memory.
>>>
>>>     These must not be called in the absence of FP/SIMD. Put in a 
>>> WARNING
>>>     to make sure they are not invoked in the absence of FP/SIMD.
>>>
>>> KVM also uses the TIF_FOREIGN_FPSTATE flag to manage the FP/SIMD 
>>> state
>>> on the CPU. However, without FP/SIMD support we trap all accesses 
>>> and
>>> inject undefined instruction. Thus we should never "load" guest 
>>> state.
>>> Add a sanity check to make sure this is valid.
>> Yes, but no, see below.
>>
>>>
>>> Fixes: 82e0191a1aa11abf ("arm64: Support systems without FP/ASIMD")
>>> Cc: Will Deacon <will@kernel.org>
>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>> No idea who that guy is. It's a fake! ;-)
>
> Sorry about that, will fix it.
>
>>
>>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>>> ---
>>>  arch/arm64/kernel/fpsimd.c  | 31 +++++++++++++++++++++++++++----
>>>  arch/arm64/kvm/hyp/switch.c |  9 +++++++++
>>>  2 files changed, 36 insertions(+), 4 deletions(-)
>>>
>> [...]
>>
>>> diff --git a/arch/arm64/kvm/hyp/switch.c 
>>> b/arch/arm64/kvm/hyp/switch.c
>>> index 72fbbd86eb5e..9696ebb5c13a 100644
>>> --- a/arch/arm64/kvm/hyp/switch.c
>>> +++ b/arch/arm64/kvm/hyp/switch.c
>>> @@ -28,10 +28,19 @@
>>>  /* Check whether the FP regs were dirtied while in the host-side 
>>> run
>>> loop: */
>>>  static bool __hyp_text update_fp_enabled(struct kvm_vcpu *vcpu)
>>>  {
>>> +    /*
>>> +     * When the system doesn't support FP/SIMD, we cannot rely on
>>> +     * the state of _TIF_FOREIGN_FPSTATE. However, we will never
>>> +     * set the KVM_ARM64_FP_ENABLED, as the FP/SIMD accesses 
>>> always
>>> +     * inject an abort into the guest. Thus we always trap the
>>> +     * accesses.
>>> +     */
>>>      if (vcpu->arch.host_thread_info->flags & _TIF_FOREIGN_FPSTATE)
>>>          vcpu->arch.flags &= ~(KVM_ARM64_FP_ENABLED |
>>>                        KVM_ARM64_FP_HOST);
>>>
>>> +    WARN_ON(!system_supports_fpsimd() &&
>>> +        (vcpu->arch.flags & KVM_ARM64_FP_ENABLED));
>> Careful, this will panic the host if it happens on a !VHE host
>> (calling non-inline stuff from a __hyp_text function is usually
>> not a good idea).
>
> Ouch! Sorry about that WARN_ON()! I could drop the warning and
> make this :
>
> if (!system_supports_fpsimd() ||
>     (vcpu->arch.host_thread_info->flags & _TIF_FOREIGN_FPSTATE))
> 	vcpu->arch.flags &= ~(KVM_ARM64_FP_ENABLED |
> 			      KVM_ARM64_FP_HOST);
>
> to make sure we never say fp is enabled.
>
> What do you think ?

Sure, that would work. I can't really see how KVM_ARM64_FP_ENABLED
would get set though. But it probably doesn't matter (WTF is going
to run KVM with such broken HW?), and better safe than sorry.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v2 7/7] arm64: nofpsmid: Handle TIF_FOREIGN_FPSTATE flag cleanly

[Marc Zyngier]

On 2019-12-18 11:42, Suzuki Kuruppassery Poulose wrote:
> Hi Marc,
>
> On 17/12/2019 19:05, Marc Zyngier wrote:
>> Hi Suzuki,
>> On 2019-12-17 18:34, Suzuki K Poulose wrote:
>>> We detect the absence of FP/SIMD after an incapable CPU is brought 
>>> up,
>>> and by then we have kernel threads running already with
>>> TIF_FOREIGN_FPSTATE set
>>> which could be set for early userspace applications (e.g, modprobe 
>>> triggered
>>> from initramfs) and init. This could cause the applications to loop
>>> forever in
>>> do_nofity_resume() as we never clear the TIF flag, once we now know 
>>> that
>>> we don't support FP.
>>>
>>> Fix this by making sure that we clear the TIF_FOREIGN_FPSTATE flag
>>> for tasks which may have them set, as we would have done in the 
>>> normal
>>> case, but avoiding touching the hardware state (since we don't 
>>> support any).
>>>
>>> Also to make sure we handle the cases seemlessly we categorise the
>>> helper functions to two :
>>>  1) Helpers for common core code, which calls into take appropriate
>>>     actions without knowing the current FPSIMD state of the 
>>> CPU/task.
>>>
>>>     e.g fpsimd_restore_current_state(), fpsimd_flush_task_state(),
>>>         fpsimd_save_and_flush_cpu_state().
>>>
>>>     We bail out early for these functions, taking any appropriate 
>>> actions
>>>     (e.g, clearing the TIF flag) where necessary to hide the 
>>> handling
>>>     from core code.
>>>
>>>  2) Helpers used when the presence of FP/SIMD is apparent.
>>>     i.e, save/restore the FP/SIMD register state, modify the 
>>> CPU/task
>>>     FP/SIMD state.
>>>     e.g,
>>>
>>>     fpsimd_save(), task_fpsimd_load() - save/restore task FP/SIMD 
>>> registers
>>>
>>>     fpsimd_bind_task_to_cpu()  \
>>>                                 - Update the "state" metadata for 
>>> CPU/task.
>>>     fpsimd_bind_state_to_cpu() /
>>>
>>>     fpsimd_update_current_state() - Update the fp/simd state for 
>>> the current
>>>                                     task from memory.
>>>
>>>     These must not be called in the absence of FP/SIMD. Put in a 
>>> WARNING
>>>     to make sure they are not invoked in the absence of FP/SIMD.
>>>
>>> KVM also uses the TIF_FOREIGN_FPSTATE flag to manage the FP/SIMD 
>>> state
>>> on the CPU. However, without FP/SIMD support we trap all accesses 
>>> and
>>> inject undefined instruction. Thus we should never "load" guest 
>>> state.
>>> Add a sanity check to make sure this is valid.
>> Yes, but no, see below.
>>
>>>
>>> Fixes: 82e0191a1aa11abf ("arm64: Support systems without FP/ASIMD")
>>> Cc: Will Deacon <will@kernel.org>
>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>> No idea who that guy is. It's a fake! ;-)
>
> Sorry about that, will fix it.
>
>>
>>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>>> ---
>>>  arch/arm64/kernel/fpsimd.c  | 31 +++++++++++++++++++++++++++----
>>>  arch/arm64/kvm/hyp/switch.c |  9 +++++++++
>>>  2 files changed, 36 insertions(+), 4 deletions(-)
>>>
>> [...]
>>
>>> diff --git a/arch/arm64/kvm/hyp/switch.c 
>>> b/arch/arm64/kvm/hyp/switch.c
>>> index 72fbbd86eb5e..9696ebb5c13a 100644
>>> --- a/arch/arm64/kvm/hyp/switch.c
>>> +++ b/arch/arm64/kvm/hyp/switch.c
>>> @@ -28,10 +28,19 @@
>>>  /* Check whether the FP regs were dirtied while in the host-side 
>>> run
>>> loop: */
>>>  static bool __hyp_text update_fp_enabled(struct kvm_vcpu *vcpu)
>>>  {
>>> +    /*
>>> +     * When the system doesn't support FP/SIMD, we cannot rely on
>>> +     * the state of _TIF_FOREIGN_FPSTATE. However, we will never
>>> +     * set the KVM_ARM64_FP_ENABLED, as the FP/SIMD accesses 
>>> always
>>> +     * inject an abort into the guest. Thus we always trap the
>>> +     * accesses.
>>> +     */
>>>      if (vcpu->arch.host_thread_info->flags & _TIF_FOREIGN_FPSTATE)
>>>          vcpu->arch.flags &= ~(KVM_ARM64_FP_ENABLED |
>>>                        KVM_ARM64_FP_HOST);
>>>
>>> +    WARN_ON(!system_supports_fpsimd() &&
>>> +        (vcpu->arch.flags & KVM_ARM64_FP_ENABLED));
>> Careful, this will panic the host if it happens on a !VHE host
>> (calling non-inline stuff from a __hyp_text function is usually
>> not a good idea).
>
> Ouch! Sorry about that WARN_ON()! I could drop the warning and
> make this :
>
> if (!system_supports_fpsimd() ||
>     (vcpu->arch.host_thread_info->flags & _TIF_FOREIGN_FPSTATE))
> 	vcpu->arch.flags &= ~(KVM_ARM64_FP_ENABLED |
> 			      KVM_ARM64_FP_HOST);
>
> to make sure we never say fp is enabled.
>
> What do you think ?

Sure, that would work. I can't really see how KVM_ARM64_FP_ENABLED
would get set though. But it probably doesn't matter (WTF is going
to run KVM with such broken HW?), and better safe than sorry.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

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

Re: [PATCH v2 7/7] arm64: nofpsmid: Handle TIF_FOREIGN_FPSTATE flag cleanly

[Suzuki Kuruppassery Poulose]

On 18/12/2019 11:56, Marc Zyngier wrote:
> On 2019-12-18 11:42, Suzuki Kuruppassery Poulose wrote:
>> Hi Marc,
>>
>> On 17/12/2019 19:05, Marc Zyngier wrote:
>>> Hi Suzuki,
>>> On 2019-12-17 18:34, Suzuki K Poulose wrote:
>>>> We detect the absence of FP/SIMD after an incapable CPU is brought up,
>>>> and by then we have kernel threads running already with
>>>> TIF_FOREIGN_FPSTATE set
>>>> which could be set for early userspace applications (e.g, modprobe 
>>>> triggered
>>>> from initramfs) and init. This could cause the applications to loop
>>>> forever in
>>>> do_nofity_resume() as we never clear the TIF flag, once we now know 
>>>> that
>>>> we don't support FP.
>>>>
>>>> Fix this by making sure that we clear the TIF_FOREIGN_FPSTATE flag
>>>> for tasks which may have them set, as we would have done in the normal
>>>> case, but avoiding touching the hardware state (since we don't 
>>>> support any).
>>>>
>>>> Also to make sure we handle the cases seemlessly we categorise the
>>>> helper functions to two :
>>>>  1) Helpers for common core code, which calls into take appropriate
>>>>     actions without knowing the current FPSIMD state of the CPU/task.
>>>>
>>>>     e.g fpsimd_restore_current_state(), fpsimd_flush_task_state(),
>>>>         fpsimd_save_and_flush_cpu_state().
>>>>
>>>>     We bail out early for these functions, taking any appropriate 
>>>> actions
>>>>     (e.g, clearing the TIF flag) where necessary to hide the handling
>>>>     from core code.
>>>>
>>>>  2) Helpers used when the presence of FP/SIMD is apparent.
>>>>     i.e, save/restore the FP/SIMD register state, modify the CPU/task
>>>>     FP/SIMD state.
>>>>     e.g,
>>>>
>>>>     fpsimd_save(), task_fpsimd_load() - save/restore task FP/SIMD 
>>>> registers
>>>>
>>>>     fpsimd_bind_task_to_cpu()  \
>>>>                                 - Update the "state" metadata for 
>>>> CPU/task.
>>>>     fpsimd_bind_state_to_cpu() /
>>>>
>>>>     fpsimd_update_current_state() - Update the fp/simd state for the 
>>>> current
>>>>                                     task from memory.
>>>>
>>>>     These must not be called in the absence of FP/SIMD. Put in a 
>>>> WARNING
>>>>     to make sure they are not invoked in the absence of FP/SIMD.
>>>>
>>>> KVM also uses the TIF_FOREIGN_FPSTATE flag to manage the FP/SIMD state
>>>> on the CPU. However, without FP/SIMD support we trap all accesses and
>>>> inject undefined instruction. Thus we should never "load" guest state.
>>>> Add a sanity check to make sure this is valid.
>>> Yes, but no, see below.
>>>
>>>>
>>>> Fixes: 82e0191a1aa11abf ("arm64: Support systems without FP/ASIMD")
>>>> Cc: Will Deacon <will@kernel.org>
>>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>>>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>>> No idea who that guy is. It's a fake! ;-)
>>
>> Sorry about that, will fix it.
>>
>>>
>>>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>>>> ---
>>>>  arch/arm64/kernel/fpsimd.c  | 31 +++++++++++++++++++++++++++----
>>>>  arch/arm64/kvm/hyp/switch.c |  9 +++++++++
>>>>  2 files changed, 36 insertions(+), 4 deletions(-)
>>>>
>>> [...]
>>>
>>>> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
>>>> index 72fbbd86eb5e..9696ebb5c13a 100644
>>>> --- a/arch/arm64/kvm/hyp/switch.c
>>>> +++ b/arch/arm64/kvm/hyp/switch.c
>>>> @@ -28,10 +28,19 @@
>>>>  /* Check whether the FP regs were dirtied while in the host-side run
>>>> loop: */
>>>>  static bool __hyp_text update_fp_enabled(struct kvm_vcpu *vcpu)
>>>>  {
>>>> +    /*
>>>> +     * When the system doesn't support FP/SIMD, we cannot rely on
>>>> +     * the state of _TIF_FOREIGN_FPSTATE. However, we will never
>>>> +     * set the KVM_ARM64_FP_ENABLED, as the FP/SIMD accesses always
>>>> +     * inject an abort into the guest. Thus we always trap the
>>>> +     * accesses.
>>>> +     */
>>>>      if (vcpu->arch.host_thread_info->flags & _TIF_FOREIGN_FPSTATE)
>>>>          vcpu->arch.flags &= ~(KVM_ARM64_FP_ENABLED |
>>>>                        KVM_ARM64_FP_HOST);
>>>>
>>>> +    WARN_ON(!system_supports_fpsimd() &&
>>>> +        (vcpu->arch.flags & KVM_ARM64_FP_ENABLED));
>>> Careful, this will panic the host if it happens on a !VHE host
>>> (calling non-inline stuff from a __hyp_text function is usually
>>> not a good idea).
>>
>> Ouch! Sorry about that WARN_ON()! I could drop the warning and
>> make this :
>>
>> if (!system_supports_fpsimd() ||
>>     (vcpu->arch.host_thread_info->flags & _TIF_FOREIGN_FPSTATE))
>>     vcpu->arch.flags &= ~(KVM_ARM64_FP_ENABLED |
>>                   KVM_ARM64_FP_HOST);
>>
>> to make sure we never say fp is enabled.
>>
>> What do you think ?
> 
> Sure, that would work. I can't really see how KVM_ARM64_FP_ENABLED

Thanks I have fixed this locally now.

> would get set though. But it probably doesn't matter (WTF is going

Right. That cannot be set to begin with, as the first access to FP/SIMD
injects an abort back to the guest, which is why I added a WARN() to
begin with.

Just wanted to be extra safe.

> to run KVM with such broken HW?), and better safe than sorry.

Right, with no COMPAT KVM support it is really hard to get this far.

Cheers
Suzuki

Re: [PATCH v2 7/7] arm64: nofpsmid: Handle TIF_FOREIGN_FPSTATE flag cleanly

[Suzuki Kuruppassery Poulose]

On 18/12/2019 11:56, Marc Zyngier wrote:
> On 2019-12-18 11:42, Suzuki Kuruppassery Poulose wrote:
>> Hi Marc,
>>
>> On 17/12/2019 19:05, Marc Zyngier wrote:
>>> Hi Suzuki,
>>> On 2019-12-17 18:34, Suzuki K Poulose wrote:
>>>> We detect the absence of FP/SIMD after an incapable CPU is brought up,
>>>> and by then we have kernel threads running already with
>>>> TIF_FOREIGN_FPSTATE set
>>>> which could be set for early userspace applications (e.g, modprobe 
>>>> triggered
>>>> from initramfs) and init. This could cause the applications to loop
>>>> forever in
>>>> do_nofity_resume() as we never clear the TIF flag, once we now know 
>>>> that
>>>> we don't support FP.
>>>>
>>>> Fix this by making sure that we clear the TIF_FOREIGN_FPSTATE flag
>>>> for tasks which may have them set, as we would have done in the normal
>>>> case, but avoiding touching the hardware state (since we don't 
>>>> support any).
>>>>
>>>> Also to make sure we handle the cases seemlessly we categorise the
>>>> helper functions to two :
>>>>  1) Helpers for common core code, which calls into take appropriate
>>>>     actions without knowing the current FPSIMD state of the CPU/task.
>>>>
>>>>     e.g fpsimd_restore_current_state(), fpsimd_flush_task_state(),
>>>>         fpsimd_save_and_flush_cpu_state().
>>>>
>>>>     We bail out early for these functions, taking any appropriate 
>>>> actions
>>>>     (e.g, clearing the TIF flag) where necessary to hide the handling
>>>>     from core code.
>>>>
>>>>  2) Helpers used when the presence of FP/SIMD is apparent.
>>>>     i.e, save/restore the FP/SIMD register state, modify the CPU/task
>>>>     FP/SIMD state.
>>>>     e.g,
>>>>
>>>>     fpsimd_save(), task_fpsimd_load() - save/restore task FP/SIMD 
>>>> registers
>>>>
>>>>     fpsimd_bind_task_to_cpu()  \
>>>>                                 - Update the "state" metadata for 
>>>> CPU/task.
>>>>     fpsimd_bind_state_to_cpu() /
>>>>
>>>>     fpsimd_update_current_state() - Update the fp/simd state for the 
>>>> current
>>>>                                     task from memory.
>>>>
>>>>     These must not be called in the absence of FP/SIMD. Put in a 
>>>> WARNING
>>>>     to make sure they are not invoked in the absence of FP/SIMD.
>>>>
>>>> KVM also uses the TIF_FOREIGN_FPSTATE flag to manage the FP/SIMD state
>>>> on the CPU. However, without FP/SIMD support we trap all accesses and
>>>> inject undefined instruction. Thus we should never "load" guest state.
>>>> Add a sanity check to make sure this is valid.
>>> Yes, but no, see below.
>>>
>>>>
>>>> Fixes: 82e0191a1aa11abf ("arm64: Support systems without FP/ASIMD")
>>>> Cc: Will Deacon <will@kernel.org>
>>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>>>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>>> No idea who that guy is. It's a fake! ;-)
>>
>> Sorry about that, will fix it.
>>
>>>
>>>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>>>> ---
>>>>  arch/arm64/kernel/fpsimd.c  | 31 +++++++++++++++++++++++++++----
>>>>  arch/arm64/kvm/hyp/switch.c |  9 +++++++++
>>>>  2 files changed, 36 insertions(+), 4 deletions(-)
>>>>
>>> [...]
>>>
>>>> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
>>>> index 72fbbd86eb5e..9696ebb5c13a 100644
>>>> --- a/arch/arm64/kvm/hyp/switch.c
>>>> +++ b/arch/arm64/kvm/hyp/switch.c
>>>> @@ -28,10 +28,19 @@
>>>>  /* Check whether the FP regs were dirtied while in the host-side run
>>>> loop: */
>>>>  static bool __hyp_text update_fp_enabled(struct kvm_vcpu *vcpu)
>>>>  {
>>>> +    /*
>>>> +     * When the system doesn't support FP/SIMD, we cannot rely on
>>>> +     * the state of _TIF_FOREIGN_FPSTATE. However, we will never
>>>> +     * set the KVM_ARM64_FP_ENABLED, as the FP/SIMD accesses always
>>>> +     * inject an abort into the guest. Thus we always trap the
>>>> +     * accesses.
>>>> +     */
>>>>      if (vcpu->arch.host_thread_info->flags & _TIF_FOREIGN_FPSTATE)
>>>>          vcpu->arch.flags &= ~(KVM_ARM64_FP_ENABLED |
>>>>                        KVM_ARM64_FP_HOST);
>>>>
>>>> +    WARN_ON(!system_supports_fpsimd() &&
>>>> +        (vcpu->arch.flags & KVM_ARM64_FP_ENABLED));
>>> Careful, this will panic the host if it happens on a !VHE host
>>> (calling non-inline stuff from a __hyp_text function is usually
>>> not a good idea).
>>
>> Ouch! Sorry about that WARN_ON()! I could drop the warning and
>> make this :
>>
>> if (!system_supports_fpsimd() ||
>>     (vcpu->arch.host_thread_info->flags & _TIF_FOREIGN_FPSTATE))
>>     vcpu->arch.flags &= ~(KVM_ARM64_FP_ENABLED |
>>                   KVM_ARM64_FP_HOST);
>>
>> to make sure we never say fp is enabled.
>>
>> What do you think ?
> 
> Sure, that would work. I can't really see how KVM_ARM64_FP_ENABLED

Thanks I have fixed this locally now.

> would get set though. But it probably doesn't matter (WTF is going

Right. That cannot be set to begin with, as the first access to FP/SIMD
injects an abort back to the guest, which is why I added a WARN() to
begin with.

Just wanted to be extra safe.

> to run KVM with such broken HW?), and better safe than sorry.

Right, with no COMPAT KVM support it is really hard to get this far.

Cheers
Suzuki

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

Re: [PATCH v2 7/7] arm64: nofpsmid: Handle TIF_FOREIGN_FPSTATE flag cleanly

[Marc Zyngier]

On 2019-12-18 12:00, Suzuki Kuruppassery Poulose wrote:
> On 18/12/2019 11:56, Marc Zyngier wrote:
>> On 2019-12-18 11:42, Suzuki Kuruppassery Poulose wrote:
>>> Hi Marc,
>>> 
>>> On 17/12/2019 19:05, Marc Zyngier wrote:
>>>> Hi Suzuki,
>>>> On 2019-12-17 18:34, Suzuki K Poulose wrote:
>>>>> We detect the absence of FP/SIMD after an incapable CPU is brought 
>>>>> up,
>>>>> and by then we have kernel threads running already with
>>>>> TIF_FOREIGN_FPSTATE set
>>>>> which could be set for early userspace applications (e.g, modprobe 
>>>>> triggered
>>>>> from initramfs) and init. This could cause the applications to loop
>>>>> forever in
>>>>> do_nofity_resume() as we never clear the TIF flag, once we now know 
>>>>> that
>>>>> we don't support FP.
>>>>> 
>>>>> Fix this by making sure that we clear the TIF_FOREIGN_FPSTATE flag
>>>>> for tasks which may have them set, as we would have done in the 
>>>>> normal
>>>>> case, but avoiding touching the hardware state (since we don't 
>>>>> support any).
>>>>> 
>>>>> Also to make sure we handle the cases seemlessly we categorise the
>>>>> helper functions to two :
>>>>>  1) Helpers for common core code, which calls into take appropriate
>>>>>     actions without knowing the current FPSIMD state of the 
>>>>> CPU/task.
>>>>> 
>>>>>     e.g fpsimd_restore_current_state(), fpsimd_flush_task_state(),
>>>>>         fpsimd_save_and_flush_cpu_state().
>>>>> 
>>>>>     We bail out early for these functions, taking any appropriate 
>>>>> actions
>>>>>     (e.g, clearing the TIF flag) where necessary to hide the 
>>>>> handling
>>>>>     from core code.
>>>>> 
>>>>>  2) Helpers used when the presence of FP/SIMD is apparent.
>>>>>     i.e, save/restore the FP/SIMD register state, modify the 
>>>>> CPU/task
>>>>>     FP/SIMD state.
>>>>>     e.g,
>>>>> 
>>>>>     fpsimd_save(), task_fpsimd_load() - save/restore task FP/SIMD 
>>>>> registers
>>>>> 
>>>>>     fpsimd_bind_task_to_cpu()  \
>>>>>                                 - Update the "state" metadata for 
>>>>> CPU/task.
>>>>>     fpsimd_bind_state_to_cpu() /
>>>>> 
>>>>>     fpsimd_update_current_state() - Update the fp/simd state for 
>>>>> the current
>>>>>                                     task from memory.
>>>>> 
>>>>>     These must not be called in the absence of FP/SIMD. Put in a 
>>>>> WARNING
>>>>>     to make sure they are not invoked in the absence of FP/SIMD.
>>>>> 
>>>>> KVM also uses the TIF_FOREIGN_FPSTATE flag to manage the FP/SIMD 
>>>>> state
>>>>> on the CPU. However, without FP/SIMD support we trap all accesses 
>>>>> and
>>>>> inject undefined instruction. Thus we should never "load" guest 
>>>>> state.
>>>>> Add a sanity check to make sure this is valid.
>>>> Yes, but no, see below.
>>>> 
>>>>> 
>>>>> Fixes: 82e0191a1aa11abf ("arm64: Support systems without FP/ASIMD")
>>>>> Cc: Will Deacon <will@kernel.org>
>>>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>>>>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>>>> No idea who that guy is. It's a fake! ;-)
>>> 
>>> Sorry about that, will fix it.
>>> 
>>>> 
>>>>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>>>>> ---
>>>>>  arch/arm64/kernel/fpsimd.c  | 31 +++++++++++++++++++++++++++----
>>>>>  arch/arm64/kvm/hyp/switch.c |  9 +++++++++
>>>>>  2 files changed, 36 insertions(+), 4 deletions(-)
>>>>> 
>>>> [...]
>>>> 
>>>>> diff --git a/arch/arm64/kvm/hyp/switch.c 
>>>>> b/arch/arm64/kvm/hyp/switch.c
>>>>> index 72fbbd86eb5e..9696ebb5c13a 100644
>>>>> --- a/arch/arm64/kvm/hyp/switch.c
>>>>> +++ b/arch/arm64/kvm/hyp/switch.c
>>>>> @@ -28,10 +28,19 @@
>>>>>  /* Check whether the FP regs were dirtied while in the host-side 
>>>>> run
>>>>> loop: */
>>>>>  static bool __hyp_text update_fp_enabled(struct kvm_vcpu *vcpu)
>>>>>  {
>>>>> +    /*
>>>>> +     * When the system doesn't support FP/SIMD, we cannot rely on
>>>>> +     * the state of _TIF_FOREIGN_FPSTATE. However, we will never
>>>>> +     * set the KVM_ARM64_FP_ENABLED, as the FP/SIMD accesses 
>>>>> always
>>>>> +     * inject an abort into the guest. Thus we always trap the
>>>>> +     * accesses.
>>>>> +     */
>>>>>      if (vcpu->arch.host_thread_info->flags & _TIF_FOREIGN_FPSTATE)
>>>>>          vcpu->arch.flags &= ~(KVM_ARM64_FP_ENABLED |
>>>>>                        KVM_ARM64_FP_HOST);
>>>>> 
>>>>> +    WARN_ON(!system_supports_fpsimd() &&
>>>>> +        (vcpu->arch.flags & KVM_ARM64_FP_ENABLED));
>>>> Careful, this will panic the host if it happens on a !VHE host
>>>> (calling non-inline stuff from a __hyp_text function is usually
>>>> not a good idea).
>>> 
>>> Ouch! Sorry about that WARN_ON()! I could drop the warning and
>>> make this :
>>> 
>>> if (!system_supports_fpsimd() ||
>>>     (vcpu->arch.host_thread_info->flags & _TIF_FOREIGN_FPSTATE))
>>>     vcpu->arch.flags &= ~(KVM_ARM64_FP_ENABLED |
>>>                   KVM_ARM64_FP_HOST);
>>> 
>>> to make sure we never say fp is enabled.
>>> 
>>> What do you think ?
>> 
>> Sure, that would work. I can't really see how KVM_ARM64_FP_ENABLED
> 
> Thanks I have fixed this locally now.
> 
>> would get set though. But it probably doesn't matter (WTF is going
> 
> Right. That cannot be set to begin with, as the first access to FP/SIMD
> injects an abort back to the guest, which is why I added a WARN() to
> begin with.
> 
> Just wanted to be extra safe.
> 
>> to run KVM with such broken HW?), and better safe than sorry.
> 
> Right, with no COMPAT KVM support it is really hard to get this far.

So with the above fix:

Acked-by: Marc Zyngier <maz@kernel.org>

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v2 7/7] arm64: nofpsmid: Handle TIF_FOREIGN_FPSTATE flag cleanly

[Marc Zyngier]

On 2019-12-18 12:00, Suzuki Kuruppassery Poulose wrote:
> On 18/12/2019 11:56, Marc Zyngier wrote:
>> On 2019-12-18 11:42, Suzuki Kuruppassery Poulose wrote:
>>> Hi Marc,
>>> 
>>> On 17/12/2019 19:05, Marc Zyngier wrote:
>>>> Hi Suzuki,
>>>> On 2019-12-17 18:34, Suzuki K Poulose wrote:
>>>>> We detect the absence of FP/SIMD after an incapable CPU is brought 
>>>>> up,
>>>>> and by then we have kernel threads running already with
>>>>> TIF_FOREIGN_FPSTATE set
>>>>> which could be set for early userspace applications (e.g, modprobe 
>>>>> triggered
>>>>> from initramfs) and init. This could cause the applications to loop
>>>>> forever in
>>>>> do_nofity_resume() as we never clear the TIF flag, once we now know 
>>>>> that
>>>>> we don't support FP.
>>>>> 
>>>>> Fix this by making sure that we clear the TIF_FOREIGN_FPSTATE flag
>>>>> for tasks which may have them set, as we would have done in the 
>>>>> normal
>>>>> case, but avoiding touching the hardware state (since we don't 
>>>>> support any).
>>>>> 
>>>>> Also to make sure we handle the cases seemlessly we categorise the
>>>>> helper functions to two :
>>>>>  1) Helpers for common core code, which calls into take appropriate
>>>>>     actions without knowing the current FPSIMD state of the 
>>>>> CPU/task.
>>>>> 
>>>>>     e.g fpsimd_restore_current_state(), fpsimd_flush_task_state(),
>>>>>         fpsimd_save_and_flush_cpu_state().
>>>>> 
>>>>>     We bail out early for these functions, taking any appropriate 
>>>>> actions
>>>>>     (e.g, clearing the TIF flag) where necessary to hide the 
>>>>> handling
>>>>>     from core code.
>>>>> 
>>>>>  2) Helpers used when the presence of FP/SIMD is apparent.
>>>>>     i.e, save/restore the FP/SIMD register state, modify the 
>>>>> CPU/task
>>>>>     FP/SIMD state.
>>>>>     e.g,
>>>>> 
>>>>>     fpsimd_save(), task_fpsimd_load() - save/restore task FP/SIMD 
>>>>> registers
>>>>> 
>>>>>     fpsimd_bind_task_to_cpu()  \
>>>>>                                 - Update the "state" metadata for 
>>>>> CPU/task.
>>>>>     fpsimd_bind_state_to_cpu() /
>>>>> 
>>>>>     fpsimd_update_current_state() - Update the fp/simd state for 
>>>>> the current
>>>>>                                     task from memory.
>>>>> 
>>>>>     These must not be called in the absence of FP/SIMD. Put in a 
>>>>> WARNING
>>>>>     to make sure they are not invoked in the absence of FP/SIMD.
>>>>> 
>>>>> KVM also uses the TIF_FOREIGN_FPSTATE flag to manage the FP/SIMD 
>>>>> state
>>>>> on the CPU. However, without FP/SIMD support we trap all accesses 
>>>>> and
>>>>> inject undefined instruction. Thus we should never "load" guest 
>>>>> state.
>>>>> Add a sanity check to make sure this is valid.
>>>> Yes, but no, see below.
>>>> 
>>>>> 
>>>>> Fixes: 82e0191a1aa11abf ("arm64: Support systems without FP/ASIMD")
>>>>> Cc: Will Deacon <will@kernel.org>
>>>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>>>>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>>>> No idea who that guy is. It's a fake! ;-)
>>> 
>>> Sorry about that, will fix it.
>>> 
>>>> 
>>>>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>>>>> ---
>>>>>  arch/arm64/kernel/fpsimd.c  | 31 +++++++++++++++++++++++++++----
>>>>>  arch/arm64/kvm/hyp/switch.c |  9 +++++++++
>>>>>  2 files changed, 36 insertions(+), 4 deletions(-)
>>>>> 
>>>> [...]
>>>> 
>>>>> diff --git a/arch/arm64/kvm/hyp/switch.c 
>>>>> b/arch/arm64/kvm/hyp/switch.c
>>>>> index 72fbbd86eb5e..9696ebb5c13a 100644
>>>>> --- a/arch/arm64/kvm/hyp/switch.c
>>>>> +++ b/arch/arm64/kvm/hyp/switch.c
>>>>> @@ -28,10 +28,19 @@
>>>>>  /* Check whether the FP regs were dirtied while in the host-side 
>>>>> run
>>>>> loop: */
>>>>>  static bool __hyp_text update_fp_enabled(struct kvm_vcpu *vcpu)
>>>>>  {
>>>>> +    /*
>>>>> +     * When the system doesn't support FP/SIMD, we cannot rely on
>>>>> +     * the state of _TIF_FOREIGN_FPSTATE. However, we will never
>>>>> +     * set the KVM_ARM64_FP_ENABLED, as the FP/SIMD accesses 
>>>>> always
>>>>> +     * inject an abort into the guest. Thus we always trap the
>>>>> +     * accesses.
>>>>> +     */
>>>>>      if (vcpu->arch.host_thread_info->flags & _TIF_FOREIGN_FPSTATE)
>>>>>          vcpu->arch.flags &= ~(KVM_ARM64_FP_ENABLED |
>>>>>                        KVM_ARM64_FP_HOST);
>>>>> 
>>>>> +    WARN_ON(!system_supports_fpsimd() &&
>>>>> +        (vcpu->arch.flags & KVM_ARM64_FP_ENABLED));
>>>> Careful, this will panic the host if it happens on a !VHE host
>>>> (calling non-inline stuff from a __hyp_text function is usually
>>>> not a good idea).
>>> 
>>> Ouch! Sorry about that WARN_ON()! I could drop the warning and
>>> make this :
>>> 
>>> if (!system_supports_fpsimd() ||
>>>     (vcpu->arch.host_thread_info->flags & _TIF_FOREIGN_FPSTATE))
>>>     vcpu->arch.flags &= ~(KVM_ARM64_FP_ENABLED |
>>>                   KVM_ARM64_FP_HOST);
>>> 
>>> to make sure we never say fp is enabled.
>>> 
>>> What do you think ?
>> 
>> Sure, that would work. I can't really see how KVM_ARM64_FP_ENABLED
> 
> Thanks I have fixed this locally now.
> 
>> would get set though. But it probably doesn't matter (WTF is going
> 
> Right. That cannot be set to begin with, as the first access to FP/SIMD
> injects an abort back to the guest, which is why I added a WARN() to
> begin with.
> 
> Just wanted to be extra safe.
> 
>> to run KVM with such broken HW?), and better safe than sorry.
> 
> Right, with no COMPAT KVM support it is really hard to get this far.

So with the above fix:

Acked-by: Marc Zyngier <maz@kernel.org>

         M.
-- 
Jazz is not dead. It just smells funny...

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

Re: [PATCH v2 7/7] arm64: nofpsmid: Handle TIF_FOREIGN_FPSTATE flag cleanly

[Suzuki Kuruppassery Poulose]

On 10/01/2020 15:21, Marc Zyngier wrote:
> On 2019-12-18 12:00, Suzuki Kuruppassery Poulose wrote:
>> On 18/12/2019 11:56, Marc Zyngier wrote:
>>> On 2019-12-18 11:42, Suzuki Kuruppassery Poulose wrote:
>>>> Hi Marc,
>>>>
>>>> On 17/12/2019 19:05, Marc Zyngier wrote:


>>>>>> KVM also uses the TIF_FOREIGN_FPSTATE flag to manage the FP/SIMD 
>>>>>> state
>>>>>> on the CPU. However, without FP/SIMD support we trap all accesses and
>>>>>> inject undefined instruction. Thus we should never "load" guest 
>>>>>> state.
>>>>>> Add a sanity check to make sure this is valid.
>>>>> Yes, but no, see below.
>>>>>
>>>>>>
>>>>>> Fixes: 82e0191a1aa11abf ("arm64: Support systems without FP/ASIMD")
>>>>>> Cc: Will Deacon <will@kernel.org>
>>>>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>>>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>>>>>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>>>>> No idea who that guy is. It's a fake! ;-)
>>>>
>>>> Sorry about that, will fix it.
>>>>
>>>>>
>>>>>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>>>>>> ---
>>>>>>  arch/arm64/kernel/fpsimd.c  | 31 +++++++++++++++++++++++++++----
>>>>>>  arch/arm64/kvm/hyp/switch.c |  9 +++++++++
>>>>>>  2 files changed, 36 insertions(+), 4 deletions(-)
>>>>>>
>>>>> [...]
>>>>>
>>>>>> diff --git a/arch/arm64/kvm/hyp/switch.c 
>>>>>> b/arch/arm64/kvm/hyp/switch.c
>>>>>> index 72fbbd86eb5e..9696ebb5c13a 100644
>>>>>> --- a/arch/arm64/kvm/hyp/switch.c
>>>>>> +++ b/arch/arm64/kvm/hyp/switch.c
>>>>>> @@ -28,10 +28,19 @@
>>>>>>  /* Check whether the FP regs were dirtied while in the host-side run
>>>>>> loop: */
>>>>>>  static bool __hyp_text update_fp_enabled(struct kvm_vcpu *vcpu)
>>>>>>  {
>>>>>> +    /*
>>>>>> +     * When the system doesn't support FP/SIMD, we cannot rely on
>>>>>> +     * the state of _TIF_FOREIGN_FPSTATE. However, we will never
>>>>>> +     * set the KVM_ARM64_FP_ENABLED, as the FP/SIMD accesses always
>>>>>> +     * inject an abort into the guest. Thus we always trap the
>>>>>> +     * accesses.
>>>>>> +     */
>>>>>>      if (vcpu->arch.host_thread_info->flags & _TIF_FOREIGN_FPSTATE)
>>>>>>          vcpu->arch.flags &= ~(KVM_ARM64_FP_ENABLED |
>>>>>>                        KVM_ARM64_FP_HOST);
>>>>>>
>>>>>> +    WARN_ON(!system_supports_fpsimd() &&
>>>>>> +        (vcpu->arch.flags & KVM_ARM64_FP_ENABLED));
>>>>> Careful, this will panic the host if it happens on a !VHE host
>>>>> (calling non-inline stuff from a __hyp_text function is usually
>>>>> not a good idea).
>>>>
>>>> Ouch! Sorry about that WARN_ON()! I could drop the warning and
>>>> make this :
>>>>
>>>> if (!system_supports_fpsimd() ||
>>>>     (vcpu->arch.host_thread_info->flags & _TIF_FOREIGN_FPSTATE))
>>>>     vcpu->arch.flags &= ~(KVM_ARM64_FP_ENABLED |
>>>>                   KVM_ARM64_FP_HOST);
>>>>
>>>> to make sure we never say fp is enabled.
>>>>
>>>> What do you think ?
>>>
>>> Sure, that would work. I can't really see how KVM_ARM64_FP_ENABLED
>>
>> Thanks I have fixed this locally now.
>>
>>> would get set though. But it probably doesn't matter (WTF is going
>>
>> Right. That cannot be set to begin with, as the first access to FP/SIMD
>> injects an abort back to the guest, which is why I added a WARN() to
>> begin with.
>>
>> Just wanted to be extra safe.
>>
>>> to run KVM with such broken HW?), and better safe than sorry.
>>
>> Right, with no COMPAT KVM support it is really hard to get this far.
> 
> So with the above fix:
> 
> Acked-by: Marc Zyngier <maz@kernel.org>
> 
>          M.

Thanks, I have changed the KVM hunk to :


diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
index 72fbbd86eb5e..e5816d885761 100644
--- a/arch/arm64/kvm/hyp/switch.c
+++ b/arch/arm64/kvm/hyp/switch.c
@@ -28,7 +28,15 @@
  /* Check whether the FP regs were dirtied while in the host-side run 
loop: */
  static bool __hyp_text update_fp_enabled(struct kvm_vcpu *vcpu)
  {
-	if (vcpu->arch.host_thread_info->flags & _TIF_FOREIGN_FPSTATE)
+	/*
+	 * When the system doesn't support FP/SIMD, we cannot rely on
+	 * the _TIF_FOREIGN_FPSTATE flag. However, we always inject an
+	 * abort on the very first access to FP and thus we should never
+	 * see KVM_ARM64_FP_ENABLED. For added safety, make sure we always
+	 * trap the accesses.
+	 */
+	if (!system_supports_fpsimd() ||
+	    vcpu->arch.host_thread_info->flags & _TIF_FOREIGN_FPSTATE)
  		vcpu->arch.flags &= ~(KVM_ARM64_FP_ENABLED |
  				      KVM_ARM64_FP_HOST);


Suzuki

Re: [PATCH v2 7/7] arm64: nofpsmid: Handle TIF_FOREIGN_FPSTATE flag cleanly

[Suzuki Kuruppassery Poulose]

On 10/01/2020 15:21, Marc Zyngier wrote:
> On 2019-12-18 12:00, Suzuki Kuruppassery Poulose wrote:
>> On 18/12/2019 11:56, Marc Zyngier wrote:
>>> On 2019-12-18 11:42, Suzuki Kuruppassery Poulose wrote:
>>>> Hi Marc,
>>>>
>>>> On 17/12/2019 19:05, Marc Zyngier wrote:


>>>>>> KVM also uses the TIF_FOREIGN_FPSTATE flag to manage the FP/SIMD 
>>>>>> state
>>>>>> on the CPU. However, without FP/SIMD support we trap all accesses and
>>>>>> inject undefined instruction. Thus we should never "load" guest 
>>>>>> state.
>>>>>> Add a sanity check to make sure this is valid.
>>>>> Yes, but no, see below.
>>>>>
>>>>>>
>>>>>> Fixes: 82e0191a1aa11abf ("arm64: Support systems without FP/ASIMD")
>>>>>> Cc: Will Deacon <will@kernel.org>
>>>>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>>>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>>>>>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>>>>> No idea who that guy is. It's a fake! ;-)
>>>>
>>>> Sorry about that, will fix it.
>>>>
>>>>>
>>>>>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>>>>>> ---
>>>>>>  arch/arm64/kernel/fpsimd.c  | 31 +++++++++++++++++++++++++++----
>>>>>>  arch/arm64/kvm/hyp/switch.c |  9 +++++++++
>>>>>>  2 files changed, 36 insertions(+), 4 deletions(-)
>>>>>>
>>>>> [...]
>>>>>
>>>>>> diff --git a/arch/arm64/kvm/hyp/switch.c 
>>>>>> b/arch/arm64/kvm/hyp/switch.c
>>>>>> index 72fbbd86eb5e..9696ebb5c13a 100644
>>>>>> --- a/arch/arm64/kvm/hyp/switch.c
>>>>>> +++ b/arch/arm64/kvm/hyp/switch.c
>>>>>> @@ -28,10 +28,19 @@
>>>>>>  /* Check whether the FP regs were dirtied while in the host-side run
>>>>>> loop: */
>>>>>>  static bool __hyp_text update_fp_enabled(struct kvm_vcpu *vcpu)
>>>>>>  {
>>>>>> +    /*
>>>>>> +     * When the system doesn't support FP/SIMD, we cannot rely on
>>>>>> +     * the state of _TIF_FOREIGN_FPSTATE. However, we will never
>>>>>> +     * set the KVM_ARM64_FP_ENABLED, as the FP/SIMD accesses always
>>>>>> +     * inject an abort into the guest. Thus we always trap the
>>>>>> +     * accesses.
>>>>>> +     */
>>>>>>      if (vcpu->arch.host_thread_info->flags & _TIF_FOREIGN_FPSTATE)
>>>>>>          vcpu->arch.flags &= ~(KVM_ARM64_FP_ENABLED |
>>>>>>                        KVM_ARM64_FP_HOST);
>>>>>>
>>>>>> +    WARN_ON(!system_supports_fpsimd() &&
>>>>>> +        (vcpu->arch.flags & KVM_ARM64_FP_ENABLED));
>>>>> Careful, this will panic the host if it happens on a !VHE host
>>>>> (calling non-inline stuff from a __hyp_text function is usually
>>>>> not a good idea).
>>>>
>>>> Ouch! Sorry about that WARN_ON()! I could drop the warning and
>>>> make this :
>>>>
>>>> if (!system_supports_fpsimd() ||
>>>>     (vcpu->arch.host_thread_info->flags & _TIF_FOREIGN_FPSTATE))
>>>>     vcpu->arch.flags &= ~(KVM_ARM64_FP_ENABLED |
>>>>                   KVM_ARM64_FP_HOST);
>>>>
>>>> to make sure we never say fp is enabled.
>>>>
>>>> What do you think ?
>>>
>>> Sure, that would work. I can't really see how KVM_ARM64_FP_ENABLED
>>
>> Thanks I have fixed this locally now.
>>
>>> would get set though. But it probably doesn't matter (WTF is going
>>
>> Right. That cannot be set to begin with, as the first access to FP/SIMD
>> injects an abort back to the guest, which is why I added a WARN() to
>> begin with.
>>
>> Just wanted to be extra safe.
>>
>>> to run KVM with such broken HW?), and better safe than sorry.
>>
>> Right, with no COMPAT KVM support it is really hard to get this far.
> 
> So with the above fix:
> 
> Acked-by: Marc Zyngier <maz@kernel.org>
> 
>          M.

Thanks, I have changed the KVM hunk to :


diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
index 72fbbd86eb5e..e5816d885761 100644
--- a/arch/arm64/kvm/hyp/switch.c
+++ b/arch/arm64/kvm/hyp/switch.c
@@ -28,7 +28,15 @@
  /* Check whether the FP regs were dirtied while in the host-side run 
loop: */
  static bool __hyp_text update_fp_enabled(struct kvm_vcpu *vcpu)
  {
-	if (vcpu->arch.host_thread_info->flags & _TIF_FOREIGN_FPSTATE)
+	/*
+	 * When the system doesn't support FP/SIMD, we cannot rely on
+	 * the _TIF_FOREIGN_FPSTATE flag. However, we always inject an
+	 * abort on the very first access to FP and thus we should never
+	 * see KVM_ARM64_FP_ENABLED. For added safety, make sure we always
+	 * trap the accesses.
+	 */
+	if (!system_supports_fpsimd() ||
+	    vcpu->arch.host_thread_info->flags & _TIF_FOREIGN_FPSTATE)
  		vcpu->arch.flags &= ~(KVM_ARM64_FP_ENABLED |
  				      KVM_ARM64_FP_HOST);


Suzuki



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

Re: [PATCH v2 7/7] arm64: nofpsmid: Handle TIF_FOREIGN_FPSTATE flag cleanly

[Catalin Marinas]

On Tue, Dec 17, 2019 at 06:34:02PM +0000, Suzuki K Poulose wrote:
> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
> index 3eb338f14386..240c52b71cda 100644
> --- a/arch/arm64/kernel/fpsimd.c
> +++ b/arch/arm64/kernel/fpsimd.c
> @@ -269,7 +269,7 @@ static void sve_free(struct task_struct *task)
>   */
>  static void task_fpsimd_load(void)
>  {
> -	WARN_ON(!have_cpu_fpsimd_context());
> +	WARN_ON(!system_supports_fpsimd() || !have_cpu_fpsimd_context());

Nitpick: in the other functions you kept to WARN_ONs, apart from this
one where you added a ||.

Other than the KVM bits:

Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

Re: [PATCH v2 7/7] arm64: nofpsmid: Handle TIF_FOREIGN_FPSTATE flag cleanly

[Catalin Marinas]

On Tue, Dec 17, 2019 at 06:34:02PM +0000, Suzuki K Poulose wrote:
> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
> index 3eb338f14386..240c52b71cda 100644
> --- a/arch/arm64/kernel/fpsimd.c
> +++ b/arch/arm64/kernel/fpsimd.c
> @@ -269,7 +269,7 @@ static void sve_free(struct task_struct *task)
>   */
>  static void task_fpsimd_load(void)
>  {
> -	WARN_ON(!have_cpu_fpsimd_context());
> +	WARN_ON(!system_supports_fpsimd() || !have_cpu_fpsimd_context());

Nitpick: in the other functions you kept to WARN_ONs, apart from this
one where you added a ||.

Other than the KVM bits:

Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

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