MIPS: Fix type and FCSR mask.
PATCH v2 1/1 MIPS: Fix type and FCSR mask.
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.
---
v2: Change David to be Acked-by instead of Signed-off-by.
arch/mips/kernel/cpu-probe.c +2 -2
@@ -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 );
Quoted 6 lines · 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.
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
Quoted 23 lines · Maciej W. Rozycki
Quoted 6 lines · 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.
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.
Quoted 6 lines
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
Quoted 19 lines · Ralf Baechle
Quoted 19 lines
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.
Quoted 8 lines
Quoted 1 line
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:
arch/mips/kernel/cpu-probe.c static inline void cpu_set_fpu_fcsr_mask(struct cpuinfo_mips · line 93
Quoted 10 lines
arch/mips/kernel/cpu-probe.c +1
-1@@ -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