From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([208.118.235.92]:45462)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <peter.maydell@linaro.org>) id 1UfYZ1-0003mK-8Q
	for qemu-devel@nongnu.org; Thu, 23 May 2013 12:40:32 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <peter.maydell@linaro.org>) id 1UfYYw-0005n3-EQ
	for qemu-devel@nongnu.org; Thu, 23 May 2013 12:40:27 -0400
Received: from mail-la0-x22a.google.com ([2a00:1450:4010:c03::22a]:44933)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <peter.maydell@linaro.org>) id 1UfYYw-0005mr-7Z
	for qemu-devel@nongnu.org; Thu, 23 May 2013 12:40:22 -0400
Received: by mail-la0-f42.google.com with SMTP id fg20so3497004lab.15
	for <qemu-devel@nongnu.org>; Thu, 23 May 2013 09:40:20 -0700 (PDT)
MIME-Version: 1.0
In-Reply-To: <519DD0BF.4090702@huawei.com>
References: <5141F36E.10004@huawei.com> <519DCEC8.8060000@huawei.com>
	<519DD0BF.4090702@huawei.com>
From: Peter Maydell <peter.maydell@linaro.org>
Date: Thu, 23 May 2013 17:39:59 +0100
Message-ID: <CAFEAcA-8DSFA1XPaqFhhyGa0MCHNNUtsxL+zOBsNJ6ZbTdTKjw@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Subject: Re: [Qemu-devel] [PATCH 2/4] tcg/aarch64: implement new TCG target
	for aarch64
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Claudio Fontana <claudio.fontana@huawei.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>, qemu-devel@nongnu.org, Richard Henderson <rth@twiddle.net>

On 23 May 2013 09:18, Claudio Fontana <claudio.fontana@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.)

> +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).

> +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.

> +    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).

thanks
-- PMM