From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 6156 invoked by alias); 16 Apr 2019 07:14:05 -0000 Mailing-List: contact elfutils-devel-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Post: List-Help: List-Subscribe: Sender: elfutils-devel-owner@sourceware.org Received: (qmail 6139 invoked by uid 89); 16 Apr 2019 07:14:05 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Checked: by ClamAV 0.100.3 on sourceware.org X-Virus-Found: No X-Spam-SWARE-Status: No, score=-20.2 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,UNPARSEABLE_RELAY autolearn=ham version=3.3.1 spammy=HTo:U*mark, H*r:sk:SMTPD_-, H*r:mailfrom, H*r:sk:smtp.al X-Spam-Status: No, score=-20.2 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,UNPARSEABLE_RELAY autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on sourceware.org X-Spam-Level: X-HELO: smtp2200-217.mail.aliyun.com Received: from smtp2200-217.mail.aliyun.com (HELO smtp2200-217.mail.aliyun.com) (121.197.200.217) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 16 Apr 2019 07:14:02 +0000 X-Alimail-AntiSpam:AC=CONTINUE;BC=0.07436282|-1;CH=green;DM=CONTINUE|CONTINUE|true|0.400768-0.0127404-0.586491;FP=0|0|0|0|0|-1|-1|-1;HT=e01l01425;MF=han_mao@c-sky.com;NM=1;PH=DS;RN=3;RT=3;SR=0;TI=SMTPD_---.EM0TZE0_1555398839; Received: from localhost(mailfrom:han_mao@c-sky.com fp:SMTPD_---.EM0TZE0_1555398839) by smtp.aliyun-inc.com(10.147.42.241); Tue, 16 Apr 2019 15:13:59 +0800 Date: Tue, 16 Apr 2019 07:14:00 -0000 From: Mao Han To: Mark Wielaard Cc: guoren@kernel.org, elfutils-devel@sourceware.org Subject: Re: [PATCH V2 2/2] Add backend support for C-SKY Message-ID: <20190416071311.GA1542@vmh-VirtualBox> References: <5fdf33dec186807395a72cc3b6c20eeb63d5a026.1554880646.git.han_mao@c-sky.com> <688502083df410857b86612e05ea675481584f25.camel@klomp.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <688502083df410857b86612e05ea675481584f25.camel@klomp.org> User-Agent: Mutt/1.5.24 (2015-08-30) X-IsSubscribed: yes X-SW-Source: 2019-q2/txt/msg00019.txt.bz2 On Mon, Apr 15, 2019 at 01:08:58PM +0200, Mark Wielaard wrote: > > + * backends/Makefile.am: Add C-SKY. > > + * backends/csky_cfi.c: New file. > > + * backends/csky_corenote.c: Likewise. > > + * backends/csky_init.c: Likewise. > > + * backends/csky_initreg.c: Likewise. > > + * backends/csky_regs.c: Likewise. > > + * backends/csky_reloc.def: Likewise. > > + * backends/csky_symbol.c: Likewise. > > + * libebl/eblopenbackend.c: Add C-SKY. > > + * src/elflint.c: Likewise. > > We have ChangeLog files per directory. > So you don't need to prefix with backends/ > And the last two entries should go into their own (libebl and src) > ChangeLog files. > OK > > diff --git a/backends/csky_corenote.c b/backends/csky_corenote.c > > new file mode 100644 > > index 0000000..c32b386 > > --- /dev/null > > +++ b/backends/csky_corenote.c > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +#define BACKEND csky_ > > +#include "libebl_CPU.h" > > + > > +#define ULONG uint32_t > > +#define PID_T int32_t > > +#define UID_T uint32_t > > +#define GID_T uint32_t > > +#define ALIGN_ULONG 4 > > +#define ALIGN_PID_T 4 > > +#define ALIGN_UID_T 4 > > +#define ALIGN_GID_T 4 > > +#define TYPE_ULONG ELF_T_WORD > > +#define TYPE_PID_T ELF_T_SWORD > > +#define TYPE_UID_T ELF_T_WORD > > +#define TYPE_GID_T ELF_T_WORD > > + > > +static const Ebl_Register_Location prstatus_regs[] = > > + { > > + { .offset = 0, .regno = 0, .count = 38, .bits = 32 } /* r0..r31 */ > > + }; > > +#define PRSTATUS_REGS_SIZE (38 * 4) > > + > > +#include "linux-core-note.c" > > OK, that is a very simple 32bit architecture/register setup. The actual size of pr_reg is 36 word, so changed to static const Ebl_Register_Location prstatus_regs[] = { /* pc, a1, a0, sr, r2 ~ r31, reserved, reserved */ { .offset = 0, .regno = 0, .count = 36, .bits = 32 } }; #define PRSTATUS_REGS_SIZE (36 * 4) We also have 400 bytes fpu registers .reg2 section, will it cause any prblem if it is not handled? > > + /* gcc/config/ #define DWARF_FRAME_REGISTERS. */ > > + eh->frame_nregs = 71; > > + > > + return MODVERSION; > > The 71 matches what can/is being restored through other hooks. But > seems to not match the c-SKY V2 CPU ABI DWARF register mappings. Also > maybe it should be 70, because you are using link as return register > and don't explicitly need to store the pc (see below). As I mentioned in: https://sourceware.org/ml/elfutils-devel/2019-q2/msg00006.html DWARF mappings from the document is mismatch with GCC source code. 71 comes from FIRST_PSEUDO_REGISTER ./gcc/defaults.h:#define DWARF_FRAME_REGISTERS FIRST_PSEUDO_REGISTER The DWARF register mappings should be some thing like(rr stands for reserved): // r0 r1 r2 r3 r4 r5 r6 r7 // 0 1 2 3 4 5 6 7 // r8 r9 r10 r11 r12 r13 sp lr // 8 9 10 11 12 13 14 15 // r16 r17 r18 r19 r20 r21 r22 r23 // 16 17 18 19 20 21 22 23 // r24 r25 r26 r27 r28 r29 r30 r31 // 24 25 26 27 28 29 30 31 // rr rr rr rr hi lo epc // 32 33 34 35 36 37 72 This part is quite similar to native register numbering. from 38 to 68 are fpu registers, each fpu register is consist of two dwarf register. I was intended to support the fpu part, but the fpu register mapping is not so clear at present. So I'll drop the fpu part first, only use 38 DWARF registers. > > diff --git a/backends/csky_regs.c b/backends/csky_regs.c > > new file mode 100644 > > index 0000000..0b6706e > > + > This confuses me too. So there are 71 registers? But you only handle > 0..38? The names don't seem to match the usage in other places, but I > am assuming DWARF register numbers match native register numbering. > Which might not be how things actually work? > Likewise. > > + dwarf_regs[3] = user_regs.a3; > > + for (int i = 4; i < 14; i++) > > + dwarf_regs[i] = user_regs.regs[i - 4]; > > + /* r ~ r13. */ > > + for (int i = 16; i < 31; i++) > > + dwarf_regs[i] = user_regs.exregs[i - 16]; > > + /* tls. */ > > + dwarf_regs[31] = user_regs.tls; > > + /* hi. */ > > + dwarf_regs[34] = user_regs.rhi; > > + /* lo. */ > > + dwarf_regs[35] = user_regs.rlo; > > + /* pc. */ > > + dwarf_regs[70] = user_regs.pc; > > + > > + return setfunc (0, 71, dwarf_regs, arg); > > +#endif > > +} > > I think you should use setfunc (-1, -1, ®s[70], arg) to set the pc > explicitly and then setfunc (0, 36, dwarf_regs, arg) to set the rest. > But you don't seem to actually need the full 71 dwarf_regs array. Again > I am confused about the actual DWARF register number mapping. > OK > > > +bool > > +csky_machine_flag_check (GElf_Word flags) > > +{ > > + switch (flags & EF_CSKY_ABIMASK) > > + { > > + case EF_CSKY_ABIV1: > > + case EF_CSKY_ABIV2: > > + return true; > > + default: > > + return false; > > + } > > +} > > OK. > Does the current backend handle both? ABIV1 is not supported with this patch, so return false for ABIV1? > > +const char * > > +csky_section_type_name (int type, > > + char *buf __attribute__ ((unused)), > > + size_t len __attribute__ ((unused))) > > +{ > > + if (type == SHT_CSKY_ATTRIBUTES) > > + return "CSKY_ATTRIBUTES"; > > + > > + return NULL; > > +} > > OK. > I couldn't find any description of this section. > Is it like SHT_ARM_ATTRIBUTES/SHT_GNU_ATTRIBUTES? > Then you might also want to handle it like that in src/readelf.c > (print_attributes). Yes, it is. static int process_csky_specific (FILE * file) { return process_attributes (file, "csky", SHT_CSKY_ATTRIBUTES, display_csky_attribute, NULL); } binutils-gdb/binutils/readelf.c https://github.com/c-sky/binutils-gdb/blob/binutils-2_27-branch-csky/binutils/readelf.c Thanks, Mao Han