From mboxrd@z Thu Jan 1 00:00:00 1970 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-1.0 required=5.0 tests=MAILING_LIST_MULTI, RCVD_IN_MSPIKE_H2 autolearn=ham autolearn_force=no version=3.4.4 Received: (qmail 19056 invoked from network); 12 Oct 2022 18:20:56 -0000 Received: from second.openwall.net (193.110.157.125) by inbox.vuxu.org with ESMTPUTF8; 12 Oct 2022 18:20:56 -0000 Received: (qmail 32764 invoked by uid 550); 12 Oct 2022 18:20:53 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Reply-To: musl@lists.openwall.com Received: (qmail 32729 invoked from network); 12 Oct 2022 18:20:52 -0000 Date: Wed, 12 Oct 2022 14:20:39 -0400 From: Rich Felker To: =?utf-8?B?546L5rSq5Lqu?= Cc: musl@lists.openwall.com Message-ID: <20221012182038.GI29905@brightrain.aerifal.cx> References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Subject: Re: [musl] add loongarch64 port v5 On Wed, Aug 03, 2022 at 04:15:57PM +0800, 王洪亮 wrote: > Hi, > > I published loongarch64 port v5 in > https://github.com/loongson/musl/tree/loongarch-v2.0. > > fixed the following problems: > 1.Removed arch/loongarch64/bits/hwcap.h,which is not used. > 2.Removed the unused macro definitions in arch/loongarch64/bits/reg.h. > 3.#define SA_RESTORER 0x0,which means invalid in LoongArch. > 4.Modified the position of "loongarch64*) ARCH=loongarch64 ;;" by > Alphabetic order in configure. > 5.Modified the macro definition of VDSO_CGT_VER to "LINUX_5.10" > 6.Modified the macro definition of EF_LARCH_ABI and > EF_LARCH_ABI_LP64D in elf.h > 7.Modified some inappropriate comment. > 8.Some assembly instructions optimization. > > please check again,I'm Looking forward to your reply,thanks. OK, I've finally collected my notes from the past reviews and gone thru the current version to check it against them. As far as I can tell, all the requested changes were made, and I don't see anything major wrong. A few minor details... > diff --git a/include/elf.h b/include/elf.h > index 86e2f0bb..1b0e9e71 100644 > --- a/include/elf.h > +++ b/include/elf.h > @@ -697,6 +697,11 @@ typedef struct { > #define NT_MIPS_FP_MODE 0x801 > #define NT_MIPS_MSA 0x802 > #define NT_VERSION 1 > +#define NT_LOONGARCH_CPUCFG 0xa00 > +#define NT_LOONGARCH_CSR 0xa01 > +#define NT_LOONGARCH_LSX 0xa02 > +#define NT_LOONGARCH_LASX 0xa03 > +#define NT_LOONGARCH_LBT 0xa04 > > > > @@ -3288,6 +3293,66 @@ enum > #define R_RISCV_SET32 56 > #define R_RISCV_32_PCREL 57 > > +/* LoongArch ELF Flags */ > +#define EM_LOONGARCH 258 > + > +#define EF_LARCH_ABI 0x07 > +#define EF_LARCH_ABI_LP64D 0x03 > + > +/* LoongArch specific dynamic relocations. */ > +#define R_LARCH_NONE 0 These are all the relocations, not just dynamic ones like the comment says. But more to the point, elf.h and the public headers in general don't have existing comments like this, and should not add them for a new arch. > diff --git a/src/fenv/loongarch64/fenv.S b/src/fenv/loongarch64/fenv.S > new file mode 100644 > index 00000000..aa012c97 > --- /dev/null > +++ b/src/fenv/loongarch64/fenv.S > @@ -0,0 +1,72 @@ > +#ifndef __loongarch_soft_float > + > +.global feclearexcept > +.type feclearexcept,@function > +feclearexcept: > + li.w $a1, 0x1f0000 > + and $a0, $a0, $a1 > + movfcsr2gr $a1, $r0 > + or $a1, $a1, $a0 > + xor $a1, $a1, $a0 > + movgr2fcsr $r0, $a1 > + li.w $a0, 0 > + jr $ra > + > +.global feraiseexcept > +.type feraiseexcept,@function > +feraiseexcept: > + li.w $a1, 0x1f0000 > + and $a0, $a0, $a1 > + movfcsr2gr $a1, $r0 > + or $a1, $a1, $a0 > + movgr2fcsr $r0, $a1 > + li.w $a0, 0 > + jr $ra > [...] Here especially there is inconsistent indentation using spaces mixed in with tabs in some places. Please fix this to use all tabs for indentation. The general style rule in musl is that tabs are used for indention levels and spaces are used to align text, with the principle (a useful method to check) that nothing should look wrong if the reader is using a non-default tab width. There seem to be related issues throughout the asm source files; see below: > diff --git a/src/thread/loongarch64/__set_thread_area.s b/src/thread/loongarch64/__set_thread_area.s > new file mode 100644 > index 00000000..6fd09a92 > --- /dev/null > +++ b/src/thread/loongarch64/__set_thread_area.s > @@ -0,0 +1,7 @@ > +.global __set_thread_area > +.hidden __set_thread_area > +.type __set_thread_area,@function Here for example there are tabs between the directive and the name on some lines but not others. It's not clear if this was copied from somewhere upstream we have a mess now, or just introduced in the new port. But it should probably be spaces everywhere, either a single space or spaces to align at a common column. > diff --git a/src/thread/loongarch64/clone.s b/src/thread/loongarch64/clone.s > new file mode 100644 > index 00000000..86e69cfa > --- /dev/null > +++ b/src/thread/loongarch64/clone.s > @@ -0,0 +1,47 @@ > +#__clone(func, stack, flags, arg, ptid, tls, ctid) > +# a0, a1, a2, a3, a4, a5, a6 > +# sys_clone3(struct clone_args *cl_args, size_t size) > +# a0 a1 > + > +.global __clone > +.hidden __clone > +.type __clone,@function > +__clone: > + # Save function pointer and argument pointer on new thread stack > + addi.d $a1, $a1, -16 > + st.d $a0, $a1, 0 # save function pointer > + st.d $a3, $a1, 8 # save argument pointer > + > + li.d $t0, ~0x004000ff # mask CSIGNAL and CLONE_DETACHED > + and $t1, $a2, $t0 # cl_args.flags > + li.d $t0, 0x000000ff # CSIGNAL > + and $t2, $a2, $t0 # cl_args.exit_signal > + > + bstrins.d $sp, $zero, 3, 0 # align stack to 16 bytes > + addi.d $sp, $sp, -88 # struct clone_args > + st.d $t1, $sp, 0 # flags > + st.d $a4, $sp, 8 # pidfd > + st.d $a6, $sp, 16 # child_tid > + st.d $a4, $sp, 24 # parent_tid > + st.d $t2, $sp, 32 # exit_signal > + st.d $a1, $sp, 40 # stack > + st.d $zero, $sp, 48 # stack_size > + st.d $a5, $sp, 56 # tls > + st.d $zero, $sp, 64 # set_tid > + st.d $zero, $sp, 72 # set_tid_size > + st.d $zero, $sp, 80 # cgroup > + > + move $a0, $sp > + li.d $a1, 88 > + li.d $a7, 435 # __NR_clone3 > + syscall 0 # call clone3 > + > + beqz $a0, 1f # whether child process > + addi.d $sp, $sp, 88 > + jr $ra # parent process return > +1: > + ld.d $t8, $sp, 0 # function pointer > + ld.d $a0, $sp, 8 # argument pointer > + jirl $ra, $t8, 0 # call the user's function > + li.d $a7, 93 > + syscall 0 # child process exit And here some of the column alignment is done with tabs while other parts are done with spaces. Should be all-spaces for alignment. > diff --git a/src/thread/loongarch64/syscall_cp.s b/src/thread/loongarch64/syscall_cp.s > new file mode 100644 > index 00000000..9f57d254 > --- /dev/null > +++ b/src/thread/loongarch64/syscall_cp.s > @@ -0,0 +1,29 @@ > +.global __cp_begin > +.hidden __cp_begin > +.global __cp_end > +.hidden __cp_end > +.global __cp_cancel > +.hidden __cp_cancel > +.hidden __cancel > +.global __syscall_cp_asm > +.hidden __syscall_cp_asm > +.type __syscall_cp_asm,@function > + > +__syscall_cp_asm: > +__cp_begin: > + ld.w $a0, $a0, 0 > + bnez $a0, __cp_cancel > + move $t8, $a1 # reserve system call number > + move $a0, $a2 > + move $a1, $a3 > + move $a2, $a4 > + move $a3, $a5 > + move $a4, $a6 > + move $a5, $a7 > + move $a7, $t8 > + syscall 0 > +__cp_end: > + jr $ra > +__cp_cancel: > + la.local $t8, __cancel > + jr $t8 > -- > 2.36.0 More here too. I might have missed some other instances of this. I'll try to run it through some regex's to get a more complete list. At this point I think this is all stylistic stuff, no real content changes needed. I don't have a setup to test any of this, so there's an assumption on my part baked in that this is basically working and smoke-tested and whatnot. Can you point us at directions to get a working toolchain and test execution environment so that myself or others can run some basic checks? Rich