[PATCH v2] MIPS: Fix type and FCSR mask.

[PATCH v2] MIPS: Fix type and FCSR mask.

[Steven J. Hill]

The FCSR register is always 32-bits regardless if the platform is
32 or 64-bits. Change the type from 'long' to 'int' to reflect this.
The entire upper half-word of the FCSR register orginally set all
the bits to 1. Some platforms like the Octeon III simulator will
actually fault if ones are written to the reserved and/or the FPU
bits. Correct the mask to avoid this.
    
Signed-off-by: Steven J. Hill <Steven.Hill@caviumnetworks.com>
Acked-by: David Daney <ddaney@caviumnetworks.com>
---
v2: Change David to be Acked-by instead of Signed-off-by.


diff --git a/arch/mips/kernel/cpu-probe.c b/arch/mips/kernel/cpu-probe.c
index b725b71..4aa8c76 100644
--- a/arch/mips/kernel/cpu-probe.c
+++ b/arch/mips/kernel/cpu-probe.c
@@ -75,7 +75,7 @@ static inline unsigned long cpu_get_msa_id(void)
  */
 static inline void cpu_set_fpu_fcsr_mask(struct cpuinfo_mips *c)
 {
-	unsigned long sr, mask, fcsr, fcsr0, fcsr1;
+	unsigned int sr, mask, fcsr, fcsr0, fcsr1;
 
 	fcsr = c->fpu_csr31;
 	mask = FPU_CSR_ALL_X | FPU_CSR_ALL_E | FPU_CSR_ALL_S | FPU_CSR_RM;
@@ -87,7 +87,7 @@ static inline void cpu_set_fpu_fcsr_mask(struct cpuinfo_mips *c)
 	write_32bit_cp1_register(CP1_STATUS, fcsr0);
 	fcsr0 = read_32bit_cp1_register(CP1_STATUS);
 
-	fcsr1 = fcsr | ~mask;
+	fcsr1 = fcsr | (FPU_CSR_COND | FPU_CSR_FS | FPU_CSR_CONDX);
 	write_32bit_cp1_register(CP1_STATUS, fcsr1);
 	fcsr1 = read_32bit_cp1_register(CP1_STATUS);
 

Re: [PATCH v2] MIPS: Fix type and FCSR mask.

[Maciej W. Rozycki]

On Tue, 10 May 2016, Steven J. Hill wrote:

> The FCSR register is always 32-bits regardless if the platform is
> 32 or 64-bits. Change the type from 'long' to 'int' to reflect this.
> The entire upper half-word of the FCSR register orginally set all
> the bits to 1. Some platforms like the Octeon III simulator will
> actually fault if ones are written to the reserved and/or the FPU
> bits. Correct the mask to avoid this.

 This change is broken, see the description of commit 9b26616c8d9d ("MIPS: 
Respect the ISA level in FCSR handling") where this code comes from.  The 
very purpose is to probe for the writability of bits 31:18, in particular 
NAN2008 and ABS2008 stuff, but it applies to vendor bits too.  An accurate 
identification of writable bits is required for the correct presentation 
of FCSR via ptrace(2) for programs like GDB.

 You need to fix your simulator instead, the architecture does not permit 
trapping on optional FCSR bits (there are no reserved bits there anymore 
with the current architecture revision) especially as access to this 
register is unprivileged.  I don't think we can support arbitrary 
non-compliant architecture implementations -- if you need to handle an 
erratum, then please do it on a PRId by PRId basis.

 As to changing the data type, I'm fine in principle, but then please do 
so across all our source base where CP1 control registers are handled.  
Here the `long' type is used for consistency with the rest of code, so 
changing just this single place seems gratuitous to me.

 Ralf, please discard this change until it has been corrected.

  Maciej

Re: [PATCH v2] MIPS: Fix type and FCSR mask.

[Ralf Baechle]

On Wed, May 11, 2016 at 12:16:13PM +0100, Maciej W. Rozycki wrote:

> On Tue, 10 May 2016, Steven J. Hill wrote:
> 
> > The FCSR register is always 32-bits regardless if the platform is
> > 32 or 64-bits. Change the type from 'long' to 'int' to reflect this.
> > The entire upper half-word of the FCSR register orginally set all
> > the bits to 1. Some platforms like the Octeon III simulator will
> > actually fault if ones are written to the reserved and/or the FPU
> > bits. Correct the mask to avoid this.
> 
>  This change is broken, see the description of commit 9b26616c8d9d ("MIPS: 
> Respect the ISA level in FCSR handling") where this code comes from.  The 
> very purpose is to probe for the writability of bits 31:18, in particular 
> NAN2008 and ABS2008 stuff, but it applies to vendor bits too.  An accurate 
> identification of writable bits is required for the correct presentation 
> of FCSR via ptrace(2) for programs like GDB.
> 
>  You need to fix your simulator instead, the architecture does not permit 
> trapping on optional FCSR bits (there are no reserved bits there anymore 
> with the current architecture revision) especially as access to this 
> register is unprivileged.  I don't think we can support arbitrary 
> non-compliant architecture implementations -- if you need to handle an 
> erratum, then please do it on a PRId by PRId basis.
> 
>  As to changing the data type, I'm fine in principle, but then please do 
> so across all our source base where CP1 control registers are handled.  
> Here the `long' type is used for consistency with the rest of code, so 
> changing just this single place seems gratuitous to me.
> 
>  Ralf, please discard this change until it has been corrected.

Using a 32 bit variable made sufficient sense to me to apply the patch
to me.  However I agree, that the simulator's behaviour is overzealous.
While in violation of the architecture specification this is probably
similar in spirit as the mode of MIPSsim that was keeping every bit of
the system as a tristate (0, 1 and uninitialized) which indeed cought a
number of issues.

  Ralf

Re: [PATCH v2] MIPS: Fix type and FCSR mask.

[Maciej W. Rozycki]

On Wed, 11 May 2016, Ralf Baechle wrote:

> >  This change is broken, see the description of commit 9b26616c8d9d ("MIPS: 
> > Respect the ISA level in FCSR handling") where this code comes from.  The 
> > very purpose is to probe for the writability of bits 31:18, in particular 
> > NAN2008 and ABS2008 stuff, but it applies to vendor bits too.  An accurate 
> > identification of writable bits is required for the correct presentation 
> > of FCSR via ptrace(2) for programs like GDB.
> > 
> >  You need to fix your simulator instead, the architecture does not permit 
> > trapping on optional FCSR bits (there are no reserved bits there anymore 
> > with the current architecture revision) especially as access to this 
> > register is unprivileged.  I don't think we can support arbitrary 
> > non-compliant architecture implementations -- if you need to handle an 
> > erratum, then please do it on a PRId by PRId basis.
> > 
> >  As to changing the data type, I'm fine in principle, but then please do 
> > so across all our source base where CP1 control registers are handled.  
> > Here the `long' type is used for consistency with the rest of code, so 
> > changing just this single place seems gratuitous to me.
> > 
> >  Ralf, please discard this change until it has been corrected.
> 
> Using a 32 bit variable made sufficient sense to me to apply the patch
> to me.  However I agree, that the simulator's behaviour is overzealous.
> While in violation of the architecture specification this is probably
> similar in spirit as the mode of MIPSsim that was keeping every bit of
> the system as a tristate (0, 1 and uninitialized) which indeed cought a
> number of issues.

 The fundamental problem here is this functional change:

> @@ -87,7 +87,7 @@ static inline void cpu_set_fpu_fcsr_mask(struct cpuinfo_mips
> *c)
>  	write_32bit_cp1_register(CP1_STATUS, fcsr0);
>  	fcsr0 = read_32bit_cp1_register(CP1_STATUS);
>  
> -	fcsr1 = fcsr | ~mask;
> +	fcsr1 = fcsr | (FPU_CSR_COND | FPU_CSR_FS | FPU_CSR_CONDX);
>  	write_32bit_cp1_register(CP1_STATUS, fcsr1);
>  	fcsr1 = read_32bit_cp1_register(CP1_STATUS);
>  

which largely defeats the original commit referred above and certainly 
regresses 2008-NaN support.  And then any type cleanup (which is syntactic 
sugar really anyway, which however I'm absolutely fine with if not 
enthusiastic as long as applied consistently) should be made as a separate 
patch, not along a functional change.

  Maciej