[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Mon, 24 Apr 2017 09:48:08 -0400
From: Rich Felker <dalias@...c.org>
To: Jaydeep Patil <Jaydeep.Patil@...tec.com>
Cc: Szabolcs Nagy <nsz@...t70.net>,
"musl@...ts.openwall.com" <musl@...ts.openwall.com>,
Andre McCurdy <armccurdy@...il.com>
Subject: Re: microMIPS32R2 O32 port
On Mon, Apr 24, 2017 at 05:30:34AM +0000, Jaydeep Patil wrote:
> >-----Original Message-----
> >From: Rich Felker [mailto:dalias@...ifal.cx] On Behalf Of Rich Felker
> >Sent: 21 April 2017 PM 07:03
> >To: Jaydeep Patil
> >Cc: Szabolcs Nagy; musl@...ts.openwall.com; Andre McCurdy
> >Subject: Re: [musl] [MUSL] microMIPS32R2 O32 port
> >
> >On Thu, Apr 13, 2017 at 10:37:05AM +0000, Jaydeep Patil wrote:
> >> Hi Szabolcs,
> >>
> >> Please find the attached patch.
> >
> >I still don't follow the motivation of all the changes in this patch.
> >See comments below:
> >
> >> [...]
> >> diff --git a/arch/mips/crt_arch.h b/arch/mips/crt_arch.h index
> >> 9fc50d7..78832b0 100644
> >> --- a/arch/mips/crt_arch.h
> >> +++ b/arch/mips/crt_arch.h
> >> @@ -1,6 +1,7 @@
> >> __asm__(
> >> ".set push\n"
> >> ".set noreorder\n"
> >> +".set nomicromips\n"
> >> ".text \n"
> >> ".global _" START "\n"
> >> ".global " START "\n"
> >
> >Is there a need for crt1.o (or other users of this file like
> >dlstart/rcrt1) to be built as plain mips rather than micromips? If so, please
> >explain. Arbitrary changes like this without an explanation of why they're
> >being made (what was broken before, and why this is the correct fix) are not
> >acceptable.
>
> The crt1.o and __cp_begin are built as pure MIPS functions rather
> than microMIPS as they are reading data using $ra register (with ISA
> bit set). We can clear the ISA bit (using INS) and choose to compile
> these functions as microMIPS.
I see. So all the .s files are assembled as plain (32-bit) MIPS, and
these files are just affected because the asm is included in .c files
that might be compiled in microMIPS mode. That makes sense. I think
it's probably just better to make the code microMIPS-compatible though
by clearing the low bits. But syscall_cp.s needs some care because
saved instruction pointer values are compared against these labels. In
micromips mode, do the labels evaluate with the +1 low bit offset?
> >> diff --git a/arch/mips/reloc.h b/arch/mips/reloc.h index
> >> b3d59a4..772b3aa 100644
> >> --- a/arch/mips/reloc.h
> >> +++ b/arch/mips/reloc.h
> >> @@ -36,15 +36,23 @@
> >> #define CRTJMP(pc,sp) __asm__ __volatile__( \
> >> "move $sp,%1 ; jr %0" : : "r"(pc), "r"(sp) : "memory" )
> >>
> >> +/*
> >> + * When compiled for microMIPS, .align makes sure that .gpword
> >> + * is placed at word boundary. $ra must point to first .gpword.
> >> + * ISA bit of $ra must be cleared for microMIPS before using it
> >> + * as a base address. For MIPS, ISA bit is always zero.
> >> +*/
> >> #define GETFUNCSYM(fp, sym, got) __asm__ ( \
> >> ".hidden " #sym "\n" \
> >> ".set push \n" \
> >> ".set noreorder \n" \
> >> + " .align 2 \n" \
> >> " bal 1f \n" \
> >> " nop \n" \
> >> " .gpword . \n" \
> >> " .gpword " #sym " \n" \
> >> - "1: lw %0, ($ra) \n" \
> >> + "1: ins $ra, $0, 0, 1 \n" \
> >> + " lw %0, ($ra) \n" \
> >> " subu %0, $ra, %0 \n" \
> >> " lw $ra, 4($ra) \n" \
> >> " addu %0, %0, $ra \n" \
> >
> >By this, do you mean that .gpword is producing a value that's relative to the
> >actual byte position of the directive rather than the logical (ISA bit set) value
> >of "." at the point of .gpword?
>
> ISA bit is set for all microMIPS symbols (including the dot symbol).
> The GETFUNCSYM macro cannot be compiled as pure MIPS code as it
> might be expanded in a microMIPS function. We need to clear the ISA
> bit of $ra before using it as a base address in load/store
> instructions.
OK. And the same appproach would work for crt_arch.h and other places,
right?
> >> diff --git a/src/thread/mips/syscall_cp.s
> >> b/src/thread/mips/syscall_cp.s index d284626..9c5f55e 100644
> >> --- a/src/thread/mips/syscall_cp.s
> >> +++ b/src/thread/mips/syscall_cp.s
> >> @@ -1,5 +1,5 @@
> >> .set noreorder
> >> -
> >> +.set nomicromips
> >> .global __cp_begin
> >> .hidden __cp_begin
> >> .type __cp_begin,@function
> >
> >I'm also unclear on the motivation of this one. Before (v1) you had a lot of
> >changes to replace .s files with something micromips-compatible (removing
> >branch delay slots); now (v2) those changes are not included. So are .s files
> >even being built as micromips at all? If not, why is the above needed? If so,
> >how do the files with delay slots work?
>
> Branch delay slots are removed (called as compact instructions) in
> the newer MIPS/microMIPS cores (in development).
> The MIPS/microMIPS R2-R6 still support instructions with delay slot.
> Assembler takes care of converting a BRANCH + NOP to its appropriate
> compact instruction (BEQ + NOP to BEQC).
> With the v1 branch I was trying to create generic hand-written
> assembly which can be used for newer cores with the compact
> instructions.
> However I realized that it would appropriate to create a new arch
> instead of creating generic assembly.
> Thus in v2 branch I modified only those functions which would create
> issues when compiled with interlinking on.
Based on the discussions so far, I don't think pure-micromips
qualifies as a new arch. If it would be possible to take a program
compiled as micromips-only, and run it with the libc.so/ldso built for
plain mips on a machine that supports both forms of code, then it's
not a separate arch, and as I understand it this should be possible.
Rich
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
