From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:47339)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <rth7680@gmail.com>) id 1UpiIn-0000m8-F0
	for qemu-devel@nongnu.org; Thu, 20 Jun 2013 13:05:44 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <rth7680@gmail.com>) id 1UpiIg-0005Uo-UR
	for qemu-devel@nongnu.org; Thu, 20 Jun 2013 13:05:41 -0400
Received: from mail-qc0-x232.google.com ([2607:f8b0:400d:c01::232]:41477)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <rth7680@gmail.com>) id 1UpiIg-0005Ue-Qe
	for qemu-devel@nongnu.org; Thu, 20 Jun 2013 13:05:34 -0400
Received: by mail-qc0-f178.google.com with SMTP id c11so3842160qcv.37
	for <qemu-devel@nongnu.org>; Thu, 20 Jun 2013 10:05:34 -0700 (PDT)
Sender: Richard Henderson <rth7680@gmail.com>
Message-ID: <51C3365A.5000009@twiddle.net>
Date: Thu, 20 Jun 2013 10:05:30 -0700
From: Richard Henderson <rth@twiddle.net>
MIME-Version: 1.0
References: <51C2DDFD.4030702@huawei.com> <51C2DF23.3020202@huawei.com>
In-Reply-To: <51C2DF23.3020202@huawei.com>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH 1/1] tcg/aarch64: Implement tlb lookup fast
	path
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: Jani Kokkonen <Jani.Kokkonen@huawei.com>
Cc: Peter Maydell <peter.maydell@linaro.org>, Claudio Fontana <claudio.fontana@huawei.com>, "qemu-devel@nongnu.org" <qemu-devel@nongnu.org>

On 06/20/2013 03:53 AM, Jani Kokkonen wrote:
>  #ifndef _EXEC_ALL_H_
>  #define _EXEC_ALL_H_
> -
>  #include "qemu-common.h"
> -

Whitespace change?

> +/* Load and compare a TLB entry, emitting the conditional jump to the
> +slow path for the failure case, which will be patched later when finalizing
> +the slow pathClobbers X0,X1,X2,X3 and TMP.  */

Indentation.

> +    tcg_out_ldst(s, TARGET_LONG_BITS == 64 ? LDST_64 : LDST_32,
> +                  LDST_LD, TCG_REG_X0, TCG_REG_X2, tlb_offset & 0xfff);
> +    tcg_out_ldst(s, LDST_64, LDST_LD, TCG_REG_X1, TCG_REG_X2,
> +        (tlb_offset & 0xfff) + (offsetof(CPUTLBEntry, addend) -
> +             (is_read ? offsetof(CPUTLBEntry, addr_read) :
> +                   offsetof(CPUTLBEntry, addr_write))));

I wonder if it wouldn't be clearer to not include the addr_read/write offset in
the passed tlb_offset value.  So more like

  int tlb_offset = offsetof(CPUArchState, tlb_table[mem_index])

  tcg_out_ldst(s, TARGET_LONG_BITS == 64 ? LDST_64 : LDST_32,
               LDST_LD, TCG_REG_X0, TCG_REG_X2,
               (tlb_offset & 0xfff) +
               (is_read ? offsetof(CPUTLBEntry, addr_read)
                : offsetof(CPUTLBEntry, addr_write)));
  tcg_out_ldst(s, LDST_64, LDST_LD, TCG_REG_X1, TCG_REG_X2,
               (tlb_offset & 0xfff) + (offsetof(CPUTLBEntry, addend));

and then in the two callers pass down mem_index instead of tlb_offset.

In addition, the function could use some commentary.


r~