Markos Chandras <Markos.Chandras@xxxxxxxxxx> writes:
> (top posting so Matthew can see the entire patch)
>
> +CC Matthew Fortune who has some comments on the patch.
>
> On 10/11/2014 11:09 AM, Manuel Lauss wrote:
> > diff --git a/arch/mips/include/asm/asmmacro-32.h
> b/arch/mips/include/asm/asmmacro-32.h
> > index e38c281..a97ce53 100644
> > --- a/arch/mips/include/asm/asmmacro-32.h
> > +++ b/arch/mips/include/asm/asmmacro-32.h
> > @@ -12,6 +12,9 @@
> > #include <asm/fpregdef.h>
> > #include <asm/mipsregs.h>
> >
> > + .set push
> > + SET_HARDFLOAT
> > +
> > .macro fpu_save_single thread tmp=t0
> > cfc1 \tmp, fcr31
> > swc1 $f0, THREAD_FPR0_LS64(\thread)
> > @@ -86,6 +89,8 @@
> > ctc1 \tmp, fcr31
> > .endm
> >
> > + .set pop
> > +
Any reason for putting the push/pop outside of the macro here but
inside the macros elsewhere?
> > diff --git a/arch/mips/include/asm/mipsregs.h
> b/arch/mips/include/asm/mipsregs.h
> > index cf3b580..889c012 100644
> > --- a/arch/mips/include/asm/mipsregs.h
> > +++ b/arch/mips/include/asm/mipsregs.h
> > @@ -1324,6 +1324,7 @@ do {
> \
> > /*
> > * Macros to access the floating point coprocessor control registers
> > */
> > +#ifdef GAS_HAS_SET_HARDFLOAT
> > #define read_32bit_cp1_register(source) \
> > ({ \
> > int __res; \
> > @@ -1334,11 +1335,29 @@ do {
> \
> > " # gas fails to assemble cfc1 for some archs, \n" \
> > " # like Octeon. \n" \
> > " .set mips1 \n" \
> > + " .set hardfloat \n" \
> > " cfc1 %0,"STR(source)" \n" \
> > " .set pop \n" \
> > : "=r" (__res)); \
> > __res; \
> > })
> > +#else
> > +#define read_32bit_cp1_register(source) \
> > +({ \
> > + int __res; \
> > + \
> > + __asm__ __volatile__( \
> > + " .set push \n" \
> > + " .set reorder \n" \
> > + " # gas fails to assemble cfc1 for some archs, \n" \
> > + " # like Octeon. \n" \
> > + " .set mips1 \n" \
> > + " cfc1 %0,"STR(source)" \n" \
> > + " .set pop \n" \
> > + : "=r" (__res)); \
> > + __res; \
> > +})
> > +#endif
This looks fairly ugly. I believe you can just add the hardfloat using:
> > " # gas fails to assemble cfc1 for some archs, \n" \
> > " # like Octeon. \n" \
> > " .set mips1 \n" \
> > + " "STR(SET_HARDFLOAT)" \n" \
> > " cfc1 %0,"STR(source)" \n" \
> > " .set pop \n" \
> > : "=r" (__res)); \
> > __res; \
The ctc1/cfc1 instructions are quite unusual as they were (before my binutils
patch) floating point instructions but are general purpose after the patch.
While that may indicate that you don't need .set hardfloat to use ctc1/cfc1
that is not true when you consider using a new compiler and/or the
-Wa,-msoft-float CFLAGS patch with an old assembler. It would therefore be
wise to test the kernel patch with an assembler which pre-dates my FPXX
patch to make sure that all instances of ctc1/cfc1 have been caught.
That is the reason why Markos saw issues with using one of the mentor
toolchains with your patch in place.
Markos' secondary issue relating to odd-numbered single-precision registers
is probably something to address on the GCC side but I haven't quite
figured out what the root cause is. I'm trying to avoid the kernel having to
add both .set hardfloat and .set oddspreg as I think it is legitimate for
the kernel to expect that when enabling hardfloat in a softfloat module then
the standard hardfloat ABI should apply to that region and all registers
should be available.
Hope that is helpful,
Matthew
