From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:59645) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UgtSw-0002aL-V5 for qemu-devel@nongnu.org; Mon, 27 May 2013 05:11:48 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UgtSm-0001DM-Az for qemu-devel@nongnu.org; Mon, 27 May 2013 05:11:42 -0400 Received: from lhrrgout.huawei.com ([194.213.3.17]:37398) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UgtSm-00019h-2c for qemu-devel@nongnu.org; Mon, 27 May 2013 05:11:32 -0400 Message-ID: <51A32301.8030304@huawei.com> Date: Mon, 27 May 2013 11:10:25 +0200 From: Claudio Fontana MIME-Version: 1.0 References: <5141F36E.10004@huawei.com> <519DCEC8.8060000@huawei.com> <519DD0BF.4090702@huawei.com> <519F2A06.8090200@huawei.com> In-Reply-To: <519F2A06.8090200@huawei.com> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/4] tcg/aarch64: implement new TCG target for aarch64 List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: Jani Kokkonen , qemu-devel@nongnu.org, Richard Henderson (removing Paolo from CC as agreed with him) On 24.05.2013 10:51, Claudio Fontana wrote: > On 23.05.2013 18:39, Peter Maydell wrote: >> On 23 May 2013 09:18, Claudio Fontana wrote: >>> >>> add preliminary support for TCG target aarch64. >> >> Richard's handling the technical bits of the review, so >> just some minor style nits here. >> >> I tested this on the foundation model and was able to boot >> a 32-bit-ARM kernel. >> >>> +static inline void reloc_pc19(void *code_ptr, tcg_target_long target) >>> +{ >>> + tcg_target_long offset; uint32_t insn; >>> + offset = (target - (tcg_target_long)code_ptr) / 4; >>> + offset &= 0x07ffff; >>> + /* read instruction, mask away previous PC_REL19 parameter contents, >>> + set the proper offset, then write back the instruction. */ >>> + insn = *(uint32_t *)code_ptr; >>> + insn = (insn & 0xff00001f) | offset << 5; /* lower 5 bits = condition */ >> >> You can say >> insn = deposit32(insn, 5, 19, offset); >> here rather than doing >> offset &= 0x07ffff; >> insn = (insn & 0xff00001f) | offset << 5; >> >> (might as well also use deposit32 for consistency in the pc26 function.) > > Ok, I'll make use of it. > >>> +static inline enum aarch64_ldst_op_data >>> +aarch64_ldst_get_data(TCGOpcode tcg_op) >>> +{ >>> + switch (tcg_op) { >>> + case INDEX_op_ld8u_i32: case INDEX_op_ld8s_i32: >>> + case INDEX_op_ld8u_i64: case INDEX_op_ld8s_i64: >>> + case INDEX_op_st8_i32: case INDEX_op_st8_i64: >> >> One case per line, please (here and elsewhere). > > Will comply. > >>> +static inline void tcg_out_call(TCGContext *s, tcg_target_long target) >>> +{ >>> + tcg_target_long offset; >>> + >>> + offset = (target - (tcg_target_long)s->code_ptr) / 4; >>> + >>> + if (offset <= -0x02000000 || offset >= 0x02000000) { /* out of 26bit rng */ >>> + tcg_out_movi64(s, TCG_REG_TMP, target); >>> + tcg_out_callr(s, TCG_REG_TMP); >>> + >> >> Stray blank line. > > I should remove this \n I assume. Ok. > >>> + case INDEX_op_mov_i64: ext = 1; >> >> Please don't put code on the same line as a case statement. >> Also fall-through cases should have an explicit /* fall through */ >> comment (except in the case where there is no code at all >> between one case statement and the next). Would it be acceptable to put a comment at the beginning of the function describing ext use, to avoiding a series of /* fall through */ comments? Like this: /* ext will be set in the switch below, which will fall through to the common code. It triggers the use of extended registers where appropriate. */ and then going: case INDEX_op_something_64: ext = 1; case INDEX_op_something_32: the_actual_meat(s, ext, ...); break; > > Will change for the next version. > >> thanks >> -- PMM >> Claudio