This is a follow-up to Emilio's patch set.

My primary changes to Emilio's patches are to the first patch, in
merging the existing implementations from tcg/ppc/tcg-target.inc.c
into util/cacheinfo.c.

Then I've a few follow-up patches to take advantage of the new TB
placement for arm platforms.  I've had a look at the asm output for
ppc64 and s390x, and don't see anything obvious that can be improved.

Changes since v4:
  * The first patch reorganized a bit for aarch64 and ppc64.
    Re-tested on win32, for which there was a Werror.
    Incorporated feedback from Emilio re MacOS.
  * Fixed the short description for the tcg/arm patches.


r~


Emilio G. Cota (2):
  util: add cacheinfo
  tcg: allocate TB structs before the corresponding translated code

Richard Henderson (5):
  tcg/aarch64: Use ADR in tcg_out_movi
  tcg/arm: Use indirect branch for goto_tb
  tcg/arm: Remove limit on code buffer size
  tcg/arm: Try pc-relative addresses for movi
  tcg/arm: Use ldr (literal) for goto_tb

 include/exec/exec-all.h      |   5 +-
 include/exec/tb-context.h    |   3 +-
 include/qemu/osdep.h         |   3 +
 tcg/aarch64/tcg-target.inc.c |   7 +-
 tcg/arm/tcg-target.inc.c     |  82 +++++++++++--------
 tcg/ppc/tcg-target.inc.c     |  71 +----------------
 tcg/tcg.c                    |  20 +++++
 tcg/tcg.h                    |   2 +-
 translate-all.c              |  41 ++++++----
 util/Makefile.objs           |   1 +
 util/cacheinfo.c             | 185 +++++++++++++++++++++++++++++++++++++++++++
 11 files changed, 293 insertions(+), 127 deletions(-)
 create mode 100644 util/cacheinfo.c

-- 
2.9.4

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Subject: [Qemu-devel] [PATCH v5 0/7] tcg: allocate TB structs preceding translate
Message-id: 20170609053719.26251-1-rth@twiddle.net

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/1496991197-1815-1-git-send-email-thuth@redhat.com -> patchew/1496991197-1815-1-git-send-email-thuth@redhat.com
Switched to a new branch 'test'
0d1e2e8 tcg/arm: Use ldr (literal) for goto_tb
7887e52 tcg/arm: Try pc-relative addresses for movi
e014497 tcg/arm: Remove limit on code buffer size
1c0e563 tcg/arm: Use indirect branch for goto_tb
388468e tcg/aarch64: Use ADR in tcg_out_movi
cf1c30b tcg: allocate TB structs before the corresponding translated code
857d34e util: add cacheinfo

=== OUTPUT BEGIN ===
Checking PATCH 1/7: util: add cacheinfo...
ERROR: do not initialise globals to 0 or NULL
#149: FILE: util/cacheinfo.c:11:
+int qemu_icache_linesize = 0;

ERROR: do not initialise globals to 0 or NULL
#150: FILE: util/cacheinfo.c:12:
+int qemu_dcache_linesize = 0;

ERROR: space prohibited after that '&&' (ctx:ExW)
#191: FILE: util/cacheinfo.c:53:
+            && buf[i].Cache.Level == 1) {
             ^

WARNING: architecture specific defines should be avoided
#214: FILE: util/cacheinfo.c:76:
+# if defined(__APPLE__)

WARNING: architecture specific defines should be avoided
#248: FILE: util/cacheinfo.c:110:
+#if defined(__aarch64__)

total: 3 errors, 2 warnings, 218 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 2/7: tcg: allocate TB structs before the corresponding translated code...
Checking PATCH 3/7: tcg/aarch64: Use ADR in tcg_out_movi...
Checking PATCH 4/7: tcg/arm: Use indirect branch for goto_tb...
Checking PATCH 5/7: tcg/arm: Remove limit on code buffer size...
Checking PATCH 6/7: tcg/arm: Try pc-relative addresses for movi...
ERROR: code indent should never use tabs
#54: FILE: tcg/arm/tcg-target.inc.c:446:
+^I}$

ERROR: code indent should never use tabs
#64: FILE: tcg/arm/tcg-target.inc.c:453:
+^I}$

total: 2 errors, 0 warnings, 54 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 7/7: tcg/arm: Use ldr (literal) for goto_tb...
=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

On Thu, Jun 08, 2017 at 22:37:12 -0700, Richard Henderson wrote:
> This is a follow-up to Emilio's patch set.
> 
> My primary changes to Emilio's patches are to the first patch, in
> merging the existing implementations from tcg/ppc/tcg-target.inc.c
> into util/cacheinfo.c.
> 
> Then I've a few follow-up patches to take advantage of the new TB
> placement for arm platforms.  I've had a look at the asm output for
> ppc64 and s390x, and don't see anything obvious that can be improved.
> 
> Changes since v4:
>   * The first patch reorganized a bit for aarch64 and ppc64.
>     Re-tested on win32, for which there was a Werror.
>     Incorporated feedback from Emilio re MacOS.
>   * Fixed the short description for the tcg/arm patches.

This is shaping up quite nicely. Some minor suggestions:

Can we get these checkpatch warnings fixed ..

> === OUTPUT BEGIN ===
> Checking PATCH 1/7: util: add cacheinfo...
> ERROR: do not initialise globals to 0 or NULL
> #149: FILE: util/cacheinfo.c:11:
> +int qemu_icache_linesize = 0;
> 
> ERROR: do not initialise globals to 0 or NULL
> #150: FILE: util/cacheinfo.c:12:
> +int qemu_dcache_linesize = 0;
> 
> ERROR: space prohibited after that '&&' (ctx:ExW)
> #191: FILE: util/cacheinfo.c:53:
> +            && buf[i].Cache.Level == 1) {
>              ^

.. as well as these?

> Checking PATCH 6/7: tcg/arm: Try pc-relative addresses for movi...
> ERROR: code indent should never use tabs
> #54: FILE: tcg/arm/tcg-target.inc.c:446:
> +^I}$
> 
> ERROR: code indent should never use tabs
> #64: FILE: tcg/arm/tcg-target.inc.c:453:
> +^I}$

While at it, we might want to add the tiny patch I'll send as
a reply to this message.

Thanks,

		Emilio

On 06/09/2017 12:52 PM, Emilio G. Cota wrote:
> On Thu, Jun 08, 2017 at 22:37:12 -0700, Richard Henderson wrote:
>> This is a follow-up to Emilio's patch set.
>>
>> My primary changes to Emilio's patches are to the first patch, in
>> merging the existing implementations from tcg/ppc/tcg-target.inc.c
>> into util/cacheinfo.c.
>>
>> Then I've a few follow-up patches to take advantage of the new TB
>> placement for arm platforms.  I've had a look at the asm output for
>> ppc64 and s390x, and don't see anything obvious that can be improved.
>>
>> Changes since v4:
>>    * The first patch reorganized a bit for aarch64 and ppc64.
>>      Re-tested on win32, for which there was a Werror.
>>      Incorporated feedback from Emilio re MacOS.
>>    * Fixed the short description for the tcg/arm patches.
> 
> This is shaping up quite nicely. Some minor suggestions:
> 
> Can we get these checkpatch warnings fixed ..
> 
>> === OUTPUT BEGIN ===
>> Checking PATCH 1/7: util: add cacheinfo...
>> ERROR: do not initialise globals to 0 or NULL
>> #149: FILE: util/cacheinfo.c:11:
>> +int qemu_icache_linesize = 0;
>>
>> ERROR: do not initialise globals to 0 or NULL
>> #150: FILE: util/cacheinfo.c:12:
>> +int qemu_dcache_linesize = 0;

These are bogus checkpatch warnings.  If we really want this, we should also 
use -fno-common.  But without that, there is a real difference between 
initialized and non-initialized global variables.

>>
>> ERROR: space prohibited after that '&&' (ctx:ExW)
>> #191: FILE: util/cacheinfo.c:53:
>> +            && buf[i].Cache.Level == 1) {
>>               ^

This is also bogus.  I have no idea what it's attempting to detect.

> 
> .. as well as these?
> 
>> Checking PATCH 6/7: tcg/arm: Try pc-relative addresses for movi...
>> ERROR: code indent should never use tabs
>> #54: FILE: tcg/arm/tcg-target.inc.c:446:
>> +^I}$
>>
>> ERROR: code indent should never use tabs
>> #64: FILE: tcg/arm/tcg-target.inc.c:453:
>> +^I}$

Yes, I can fix these.


r~

On Fri, Jun 09, 2017 at 12:58:17 -0700, Richard Henderson wrote:
> >>=== OUTPUT BEGIN ===
> >>Checking PATCH 1/7: util: add cacheinfo...
> >>ERROR: do not initialise globals to 0 or NULL
> >>#149: FILE: util/cacheinfo.c:11:
> >>+int qemu_icache_linesize = 0;
> >>
> >>ERROR: do not initialise globals to 0 or NULL
> >>#150: FILE: util/cacheinfo.c:12:
> >>+int qemu_dcache_linesize = 0;
> 
> These are bogus checkpatch warnings.  If we really want this, we should also
> use -fno-common.  But without that, there is a real difference between
> initialized and non-initialized global variables.
> 
> >>
> >>ERROR: space prohibited after that '&&' (ctx:ExW)
> >>#191: FILE: util/cacheinfo.c:53:
> >>+            && buf[i].Cache.Level == 1) {
> >>              ^
> 
> This is also bogus.  I have no idea what it's attempting to detect.

I think this one is just enforcing a certain coding style convention;
"A &&\nB" will pass but "A\n&&B" won't.

		E.

We are partially initializing tb in tb_alloc. Instead, fully
initialize it in tb_gen_code, which is tb_alloc's only caller.

This saves an unnecessary write to tb->cflags.

Signed-off-by: Emilio G. Cota <cota@braap.org>
---
 translate-all.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/translate-all.c b/translate-all.c
index 966747a..d4f364d 100644
--- a/translate-all.c
+++ b/translate-all.c
@@ -841,9 +841,6 @@ static TranslationBlock *tb_alloc(target_ulong pc)
         ctx->tbs = g_renew(TranslationBlock *, ctx->tbs, ctx->tbs_size);
     }
     ctx->tbs[ctx->nb_tbs++] = tb;
-    tb->pc = pc;
-    tb->cflags = 0;
-    tb->invalid = false;
     return tb;
 }
 
@@ -1287,9 +1284,11 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
 
     gen_code_buf = tcg_ctx.code_gen_ptr;
     tb->tc_ptr = gen_code_buf;
+    tb->pc = pc;
     tb->cs_base = cs_base;
     tb->flags = flags;
     tb->cflags = cflags;
+    tb->invalid = false;
 
 #ifdef CONFIG_PROFILER
     tcg_ctx.tb_count1++; /* includes aborted translations because of
-- 
2.7.4