On Thu, Oct 23, 2014 at 6:21 PM, Matthew Fortune
<Matthew.Fortune@xxxxxxxxxx> wrote:
> 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?
I think I figured I just enclose all float-using macros with this
in one go. I'll fix it.
>> > 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; \
I didn't know that, thanks!
> 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.
Actually, I did test the patch with 2.24 first, but just with my mips32r1
alchemy target. I'll try to fix everything, retest and resend.
Thank you,
Manuel
