On 23.05.2013 18:39, Peter Maydell wrote: > On 23 May 2013 09:18, Claudio Fontana <claudio.font...@huawei.com> 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). Will change for the next version. > thanks > -- PMM > Thank you, Claudio