[v2] MIPS: Fix a preemption issue with thread's FPU defaults
diff mbox

Message ID
State Accepted
Delegated to: Ralf Baechle
Headers show

Commit Message

Maciej W. Rozycki May 12, 2015, 2:20 p.m. UTC 
Fix "BUG: using smp_processor_id() in preemptible" reported in accesses 
to thread's FPU defaults: the value to initialise FSCR to at program 
startup, the FCSR r/w mask and the contents of FIR in full FPU 
emulation, removing a regression introduced with 9b26616c [MIPS: Respect 
the ISA level in FCSR handling] and f6843626 [MIPS: math-emu: Set FIR 
feature flags for full emulation].

Use `boot_cpu_data' to obtain the data from, following the approach that 
`cpu_has_*' macros take and avoiding the call to `smp_processor_id' made 
in the reference to `current_cpu_data'.  The contents of FSCR have to be 
consistent across processors in an SMP system, the settings there must 
not change as a thread is migrated across processors.  And the contents 
of FIR are guaranteed to be consistent in FPU emulation, by definition.

Signed-off-by: Maciej W. Rozycki <>
---
Changes from v1:

- also fix PTRACE_SETFPREGS (thanks, Paul!).

Paul, Ezequiel --

 Can you verify this change addresses the problems you saw?

[Ralf, this is another 4.1 material, please push it to Linus ASAP, once 
this has been confirmed to DTRT.]

 Thanks,

  Maciej

linux-mips-emu-fcsr-isa-fix.diff

Comments

Ezequiel Garcia May 12, 2015, 5:28 p.m. UTC | #1 
On 05/12/2015 11:20 AM, Maciej W. Rozycki wrote:
> Fix "BUG: using smp_processor_id() in preemptible" reported in accesses 
> to thread's FPU defaults: the value to initialise FSCR to at program 
> startup, the FCSR r/w mask and the contents of FIR in full FPU 
> emulation, removing a regression introduced with 9b26616c [MIPS: Respect 
> the ISA level in FCSR handling] and f6843626 [MIPS: math-emu: Set FIR 
> feature flags for full emulation].
> 
> Use `boot_cpu_data' to obtain the data from, following the approach that 
> `cpu_has_*' macros take and avoiding the call to `smp_processor_id' made 
> in the reference to `current_cpu_data'.  The contents of FSCR have to be 
> consistent across processors in an SMP system, the settings there must 
> not change as a thread is migrated across processors.  And the contents 
> of FIR are guaranteed to be consistent in FPU emulation, by definition.
> 
> Signed-off-by: Maciej W. Rozycki <>
> ---
> Changes from v1:
> 
> - also fix PTRACE_SETFPREGS (thanks, Paul!).
> 
> Paul, Ezequiel --
> 
>  Can you verify this change addresses the problems you saw?
> 

Tested-by: Ezequiel Garcia <>

> [Ralf, this is another 4.1 material, please push it to Linus ASAP, once 
> this has been confirmed to DTRT.]
> 
>  Thanks,
> 
>   Maciej
> 
> linux-mips-emu-fcsr-isa-fix.diff
> Index: linux-org-test/arch/mips/include/asm/elf.h
> ===================================================================
> --- linux-org-test.orig/arch/mips/include/asm/elf.h	:20: +0100
> +++ linux-org-test/arch/mips/include/asm/elf.h	:37: +0100
> @@ -304,7 +304,7 @@ do {									\
>  									\
>  	current->thread.abi = &mips_abi;				\
>  									\
> -	current->thread.fpu.fcr31 = current_cpu_data.fpu_csr31;		\
> +	current->thread.fpu.fcr31 = boot_cpu_data.fpu_csr31;		\
>  } while (0)
>  
>  #endif /* CONFIG_32BIT */
> @@ -366,7 +366,7 @@ do {									\
>  	else								\
>  		current->thread.abi = &mips_abi;			\
>  									\
> -	current->thread.fpu.fcr31 = current_cpu_data.fpu_csr31;		\
> +	current->thread.fpu.fcr31 = boot_cpu_data.fpu_csr31;		\
>  									\
>  	p = personality(current->personality);				\
>  	if (p != PER_LINUX32 && p != PER_LINUX)				\
> Index: linux-org-test/arch/mips/kernel/ptrace.c
> ===================================================================
> --- linux-org-test.orig/arch/mips/kernel/ptrace.c	:52:04. +0100
> +++ linux-org-test/arch/mips/kernel/ptrace.c	:12: +0100
> @@ -176,7 +176,7 @@ int ptrace_setfpregs(struct task_struct 
>  
>  	__get_user(value, data + 64);
>  	fcr31 = child->thread.fpu.fcr31;
> -	mask = current_cpu_data.fpu_msk31;
> +	mask = boot_cpu_data.fpu_msk31;
>  	child->thread.fpu.fcr31 = (value & ~mask) | (fcr31 & mask);
>  
>  	/* FIR may not be written.  */
> Index: linux-org-test/arch/mips/math-emu/cp1emu.c
> ===================================================================
> --- linux-org-test.orig/arch/mips/math-emu/cp1emu.c	:52:04. +0100
> +++ linux-org-test/arch/mips/math-emu/cp1emu.c	:41: +0100
> @@ -889,7 +889,7 @@ static inline void cop1_cfc(struct pt_re
>  		break;
>  
>  	case FPCREG_RID:
> -		value = current_cpu_data.fpu_id;
> +		value = boot_cpu_data.fpu_id;
>  		break;
>  
>  	default:
> @@ -921,7 +921,7 @@ static inline void cop1_ctc(struct pt_re
>  			 (void *)xcp->cp0_epc, MIPSInst_RT(ir), value);
>  
>  		/* Preserve read-only bits.  */
> -		mask = current_cpu_data.fpu_msk31;
> +		mask = boot_cpu_data.fpu_msk31;
>  		fcr31 = (value & ~mask) | (fcr31 & mask);
>  		break;
>  
>
Paul Martin May 12, 2015, 5:34 p.m. UTC | #2 
On Tue, May 12, 2015 at 02:28:23PM -0300, Ezequiel Garcia wrote:
> 
> 
> On 05/12/2015 11:20 AM, Maciej W. Rozycki wrote:
> > Fix "BUG: using smp_processor_id() in preemptible" reported in accesses 
> > to thread's FPU defaults: the value to initialise FSCR to at program 
> > startup, the FCSR r/w mask and the contents of FIR in full FPU 
> > emulation, removing a regression introduced with 9b26616c [MIPS: Respect 
> > the ISA level in FCSR handling] and f6843626 [MIPS: math-emu: Set FIR 
> > feature flags for full emulation].
> > 
> > Use `boot_cpu_data' to obtain the data from, following the approach that 
> > `cpu_has_*' macros take and avoiding the call to `smp_processor_id' made 
> > in the reference to `current_cpu_data'.  The contents of FSCR have to be 
> > consistent across processors in an SMP system, the settings there must 
> > not change as a thread is migrated across processors.  And the contents 
> > of FIR are guaranteed to be consistent in FPU emulation, by definition.
> > 
> > Signed-off-by: Maciej W. Rozycki <>
> > ---
> > Changes from v1:
> > 
> > - also fix PTRACE_SETFPREGS (thanks, Paul!).
> > 
> > Paul, Ezequiel --
> > 
> >  Can you verify this change addresses the problems you saw?
> > 
> 
> Tested-by: Ezequiel Garcia <>

Tested-by: Paul Martin <>

> 
> > [Ralf, this is another 4.1 material, please push it to Linus ASAP, once 
> > this has been confirmed to DTRT.]
> > 
> >  Thanks,
> > 
> >   Maciej
> > 
> > linux-mips-emu-fcsr-isa-fix.diff
> > Index: linux-org-test/arch/mips/include/asm/elf.h
> > ===================================================================
> > --- linux-org-test.orig/arch/mips/include/asm/elf.h	:20: +0100
> > +++ linux-org-test/arch/mips/include/asm/elf.h	:37: +0100
> > @@ -304,7 +304,7 @@ do {									\
> >  									\
> >  	current->thread.abi = &mips_abi;				\
> >  									\
> > -	current->thread.fpu.fcr31 = current_cpu_data.fpu_csr31;		\
> > +	current->thread.fpu.fcr31 = boot_cpu_data.fpu_csr31;		\
> >  } while (0)
> >  
> >  #endif /* CONFIG_32BIT */
> > @@ -366,7 +366,7 @@ do {									\
> >  	else								\
> >  		current->thread.abi = &mips_abi;			\
> >  									\
> > -	current->thread.fpu.fcr31 = current_cpu_data.fpu_csr31;		\
> > +	current->thread.fpu.fcr31 = boot_cpu_data.fpu_csr31;		\
> >  									\
> >  	p = personality(current->personality);				\
> >  	if (p != PER_LINUX32 && p != PER_LINUX)				\
> > Index: linux-org-test/arch/mips/kernel/ptrace.c
> > ===================================================================
> > --- linux-org-test.orig/arch/mips/kernel/ptrace.c	:52:04. +0100
> > +++ linux-org-test/arch/mips/kernel/ptrace.c	:12: +0100
> > @@ -176,7 +176,7 @@ int ptrace_setfpregs(struct task_struct 
> >  
> >  	__get_user(value, data + 64);
> >  	fcr31 = child->thread.fpu.fcr31;
> > -	mask = current_cpu_data.fpu_msk31;
> > +	mask = boot_cpu_data.fpu_msk31;
> >  	child->thread.fpu.fcr31 = (value & ~mask) | (fcr31 & mask);
> >  
> >  	/* FIR may not be written.  */
> > Index: linux-org-test/arch/mips/math-emu/cp1emu.c
> > ===================================================================
> > --- linux-org-test.orig/arch/mips/math-emu/cp1emu.c	:52:04. +0100
> > +++ linux-org-test/arch/mips/math-emu/cp1emu.c	:41: +0100
> > @@ -889,7 +889,7 @@ static inline void cop1_cfc(struct pt_re
> >  		break;
> >  
> >  	case FPCREG_RID:
> > -		value = current_cpu_data.fpu_id;
> > +		value = boot_cpu_data.fpu_id;
> >  		break;
> >  
> >  	default:
> > @@ -921,7 +921,7 @@ static inline void cop1_ctc(struct pt_re
> >  			 (void *)xcp->cp0_epc, MIPSInst_RT(ir), value);
> >  
> >  		/* Preserve read-only bits.  */
> > -		mask = current_cpu_data.fpu_msk31;
> > +		mask = boot_cpu_data.fpu_msk31;
> >  		fcr31 = (value & ~mask) | (fcr31 & mask);
> >  		break;
> >  
> > 
>
Ralf Baechle May 12, 2015, 9:14 p.m. UTC | #3 
On Tue, May 12, 2015 at 03:20:57PM +0100, Maciej W. Rozycki wrote:

On systems with multiple types of FPUs this would also result in a
more consistent behaviour when a process is scheduled between different
CPUs.

  Ralf
Maciej W. Rozycki May 12, 2015, 10:35 p.m. UTC | #4 
On Tue, 12 May 2015, Ralf Baechle wrote:

> On systems with multiple types of FPUs this would also result in a
> more consistent behaviour when a process is scheduled between different
> CPUs.

 Hmm, do we support SMP systems where individual FPUs (or CPUs, for that 
matter) are different significantly enough to matter?  E.g. anyhow 
beyond FIR[15:0], that the relevant machine instructions read directly 
from hardware where implemented anyway (and likewise FCSR).

 I think eventually we should migrate properties that have to be uniform 
across all the CPUs in a SMP system outside the per-CPU `cpu_data' array 
and have a single global copy only.  This will include the ISA level, 
some options like the exception and cache model (not the particular 
topology though), VM size, etc.

 I think this FPU (and also MSA, specifically `msa_id') stuff will 
belong there as well, unless proven otherwise.  That is unless we have a 
mixed system really available in the first place (QEMU does not count, 
sorry) or one is at least is being considered, and then we actually want 
to support it beyond finding the common set of features across all the 
CPUs and limiting userland to using them only (well, if limiting would 
at all be possible, that is, via appropriate knobs requiring privilege 
to control).

  Maciej

Patch
diff mbox 

Index: linux-org-test/arch/mips/include/asm/elf.h
===================================================================
--- linux-org-test.orig/arch/mips/include/asm/elf.h	:20: +0100
+++ linux-org-test/arch/mips/include/asm/elf.h	:37: +0100
@@ -304,7 +304,7 @@  do {									\
 									\
 	current->thread.abi = &mips_abi;				\
 									\
-	current->thread.fpu.fcr31 = current_cpu_data.fpu_csr31;		\
+	current->thread.fpu.fcr31 = boot_cpu_data.fpu_csr31;		\
 } while (0)
 
 #endif /* CONFIG_32BIT */
@@ -366,7 +366,7 @@  do {									\
 	else								\
 		current->thread.abi = &mips_abi;			\
 									\
-	current->thread.fpu.fcr31 = current_cpu_data.fpu_csr31;		\
+	current->thread.fpu.fcr31 = boot_cpu_data.fpu_csr31;		\
 									\
 	p = personality(current->personality);				\
 	if (p != PER_LINUX32 && p != PER_LINUX)				\
Index: linux-org-test/arch/mips/kernel/ptrace.c
===================================================================
--- linux-org-test.orig/arch/mips/kernel/ptrace.c	:52:04. +0100
+++ linux-org-test/arch/mips/kernel/ptrace.c	:12: +0100
@@ -176,7 +176,7 @@  int ptrace_setfpregs(struct task_struct 
 
 	__get_user(value, data + 64);
 	fcr31 = child->thread.fpu.fcr31;
-	mask = current_cpu_data.fpu_msk31;
+	mask = boot_cpu_data.fpu_msk31;
 	child->thread.fpu.fcr31 = (value & ~mask) | (fcr31 & mask);
 
 	/* FIR may not be written.  */
Index: linux-org-test/arch/mips/math-emu/cp1emu.c
===================================================================
--- linux-org-test.orig/arch/mips/math-emu/cp1emu.c	:52:04. +0100
+++ linux-org-test/arch/mips/math-emu/cp1emu.c	:41: +0100
@@ -889,7 +889,7 @@  static inline void cop1_cfc(struct pt_re
 		break;
 
 	case FPCREG_RID:
-		value = current_cpu_data.fpu_id;
+		value = boot_cpu_data.fpu_id;
 		break;
 
 	default:
@@ -921,7 +921,7 @@  static inline void cop1_ctc(struct pt_re
 			 (void *)xcp->cp0_epc, MIPSInst_RT(ir), value);
 
 		/* Preserve read-only bits.  */
-		mask = current_cpu_data.fpu_msk31;
+		mask = boot_cpu_data.fpu_msk31;
 		fcr31 = (value & ~mask) | (fcr31 & mask);
 		break;