[PATCH v2 0/7] exec: Improve code for TARGET_PAGE_BITS_VARY

[PATCH v2 0/7] exec: Improve code for TARGET_PAGE_BITS_VARY

[Richard Henderson]

Changes since v1:
  * Replace my patch for TARGET_PAGE_ALIGN with Wei Yang's.
  * Detect __attribute__((alias)) support in configure.
    If unsupported, as for Apple Xcode9, use preprocessor trickery.
    Passes travis build tests for xcode9.

Link for v1:
  https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg04519.html


r~


Richard Henderson (6):
  exec: Split out variable page size support to exec-vary.c
  configure: Detect compiler support for __attribute__((alias))
  exec: Use const alias for TARGET_PAGE_BITS_VARY
  exec: Restrict TARGET_PAGE_BITS_VARY assert to CONFIG_DEBUG_TCG
  exec: Promote TARGET_PAGE_MASK to target_long
  exec: Cache TARGET_PAGE_MASK for TARGET_PAGE_BITS_VARY

Wei Yang (1):
  cpu: use ROUND_UP() to define xxx_PAGE_ALIGN

 Makefile.target        |   2 +-
 include/exec/cpu-all.h |  33 +++++++++----
 include/qemu-common.h  |   6 +++
 exec-vary.c            | 102 +++++++++++++++++++++++++++++++++++++++++
 exec.c                 |  34 --------------
 configure              |  19 ++++++++
 6 files changed, 151 insertions(+), 45 deletions(-)
 create mode 100644 exec-vary.c

-- 
2.17.1

[PATCH v2 1/7] cpu: use ROUND_UP() to define xxx_PAGE_ALIGN

[Richard Henderson]

From: Wei Yang <richardw.yang@linux.intel.com>

Use ROUND_UP() to define, which is a little bit easy to read.

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Juan Quintela <quintela@redhat.com>
Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
Message-Id: <20191013021145.16011-2-richardw.yang@linux.intel.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/exec/cpu-all.h | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
index ad9ab85eb3..255bb186ac 100644
--- a/include/exec/cpu-all.h
+++ b/include/exec/cpu-all.h
@@ -220,7 +220,7 @@ extern int target_page_bits;
 
 #define TARGET_PAGE_SIZE (1 << TARGET_PAGE_BITS)
 #define TARGET_PAGE_MASK ~(TARGET_PAGE_SIZE - 1)
-#define TARGET_PAGE_ALIGN(addr) (((addr) + TARGET_PAGE_SIZE - 1) & TARGET_PAGE_MASK)
+#define TARGET_PAGE_ALIGN(addr) ROUND_UP((addr), TARGET_PAGE_SIZE)
 
 /* Using intptr_t ensures that qemu_*_page_mask is sign-extended even
  * when intptr_t is 32-bit and we are aligning a long long.
@@ -228,9 +228,8 @@ extern int target_page_bits;
 extern uintptr_t qemu_host_page_size;
 extern intptr_t qemu_host_page_mask;
 
-#define HOST_PAGE_ALIGN(addr) (((addr) + qemu_host_page_size - 1) & qemu_host_page_mask)
-#define REAL_HOST_PAGE_ALIGN(addr) (((addr) + qemu_real_host_page_size - 1) & \
-                                    qemu_real_host_page_mask)
+#define HOST_PAGE_ALIGN(addr) ROUND_UP((addr), qemu_host_page_size)
+#define REAL_HOST_PAGE_ALIGN(addr) ROUND_UP((addr), qemu_real_host_page_size)
 
 /* same as PROT_xxx */
 #define PAGE_READ      0x0001
-- 
2.17.1

[PATCH v2 2/7] exec: Split out variable page size support to exec-vary.c

[Richard Henderson]

The next patch will play a trick with "const" that will
confuse the compiler about the uses of target_page_bits
within exec.c.  Moving everything to a new file prevents
this confusion.

No functional change so far.

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 Makefile.target       |  2 +-
 include/qemu-common.h |  6 +++++
 exec-vary.c           | 57 +++++++++++++++++++++++++++++++++++++++++++
 exec.c                | 34 --------------------------
 4 files changed, 64 insertions(+), 35 deletions(-)
 create mode 100644 exec-vary.c

diff --git a/Makefile.target b/Makefile.target
index 5e916230c4..ca3d14efe1 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -107,7 +107,7 @@ obj-y += trace/
 
 #########################################################
 # cpu emulator library
-obj-y += exec.o
+obj-y += exec.o exec-vary.o
 obj-y += accel/
 obj-$(CONFIG_TCG) += tcg/tcg.o tcg/tcg-op.o tcg/tcg-op-vec.o tcg/tcg-op-gvec.o
 obj-$(CONFIG_TCG) += tcg/tcg-common.o tcg/optimize.o
diff --git a/include/qemu-common.h b/include/qemu-common.h
index 8d84db90b0..082da59e85 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -74,6 +74,12 @@ void cpu_exec_step_atomic(CPUState *cpu);
  */
 bool set_preferred_target_page_bits(int bits);
 
+/**
+ * finalize_target_page_bits:
+ * Commit the final value set by set_preferred_target_page_bits.
+ */
+void finalize_target_page_bits(void);
+
 /**
  * Sends a (part of) iovec down a socket, yielding when the socket is full, or
  * Receives data into a (part of) iovec from a socket,
diff --git a/exec-vary.c b/exec-vary.c
new file mode 100644
index 0000000000..48c0ab306c
--- /dev/null
+++ b/exec-vary.c
@@ -0,0 +1,57 @@
+/*
+ * Variable page size handling
+ *
+ *  Copyright (c) 2003 Fabrice Bellard
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu-common.h"
+#include "exec/exec-all.h"
+
+#ifdef TARGET_PAGE_BITS_VARY
+int target_page_bits;
+bool target_page_bits_decided;
+#endif
+
+bool set_preferred_target_page_bits(int bits)
+{
+    /*
+     * The target page size is the lowest common denominator for all
+     * the CPUs in the system, so we can only make it smaller, never
+     * larger. And we can't make it smaller once we've committed to
+     * a particular size.
+     */
+#ifdef TARGET_PAGE_BITS_VARY
+    assert(bits >= TARGET_PAGE_BITS_MIN);
+    if (target_page_bits == 0 || target_page_bits > bits) {
+        if (target_page_bits_decided) {
+            return false;
+        }
+        target_page_bits = bits;
+    }
+#endif
+    return true;
+}
+
+void finalize_target_page_bits(void)
+{
+#ifdef TARGET_PAGE_BITS_VARY
+    if (target_page_bits == 0) {
+        target_page_bits = TARGET_PAGE_BITS_MIN;
+    }
+    target_page_bits_decided = true;
+#endif
+}
diff --git a/exec.c b/exec.c
index fb0943cfed..5bf181d23e 100644
--- a/exec.c
+++ b/exec.c
@@ -91,11 +91,6 @@ AddressSpace address_space_memory;
 static MemoryRegion io_mem_unassigned;
 #endif
 
-#ifdef TARGET_PAGE_BITS_VARY
-int target_page_bits;
-bool target_page_bits_decided;
-#endif
-
 CPUTailQ cpus = QTAILQ_HEAD_INITIALIZER(cpus);
 
 /* current CPU in the current thread. It is only valid inside
@@ -109,37 +104,8 @@ int use_icount;
 uintptr_t qemu_host_page_size;
 intptr_t qemu_host_page_mask;
 
-bool set_preferred_target_page_bits(int bits)
-{
-    /* The target page size is the lowest common denominator for all
-     * the CPUs in the system, so we can only make it smaller, never
-     * larger. And we can't make it smaller once we've committed to
-     * a particular size.
-     */
-#ifdef TARGET_PAGE_BITS_VARY
-    assert(bits >= TARGET_PAGE_BITS_MIN);
-    if (target_page_bits == 0 || target_page_bits > bits) {
-        if (target_page_bits_decided) {
-            return false;
-        }
-        target_page_bits = bits;
-    }
-#endif
-    return true;
-}
-
 #if !defined(CONFIG_USER_ONLY)
 
-static void finalize_target_page_bits(void)
-{
-#ifdef TARGET_PAGE_BITS_VARY
-    if (target_page_bits == 0) {
-        target_page_bits = TARGET_PAGE_BITS_MIN;
-    }
-    target_page_bits_decided = true;
-#endif
-}
-
 typedef struct PhysPageEntry PhysPageEntry;
 
 struct PhysPageEntry {
-- 
2.17.1

[PATCH v2 3/7] configure: Detect compiler support for __attribute__((alias))

[Richard Henderson]

Such support is present almost everywhere, except for Xcode 9.
It is added in Xcode 10, but travis uses xcode9 by default,
so we should support it for a while yet.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 configure | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/configure b/configure
index 145fcabbb3..3a9862fe5e 100755
--- a/configure
+++ b/configure
@@ -5518,6 +5518,21 @@ if compile_prog "" "" ; then
   vector16=yes
 fi
 
+########################################
+# See if __attribute__((alias)) is supported.
+# This false for Xcode 9, but has been remedied for Xcode 10.
+# Unfortunately, travis uses Xcode 9 by default.
+
+attralias=no
+cat > $TMPC << EOF
+int x = 1;
+extern const int y __attribute__((alias("x")));
+int main(void) { return 0; }
+EOF
+if compile_prog "" "" ; then
+    attralias=yes
+fi
+
 ########################################
 # check if getauxval is available.
 
@@ -7083,6 +7098,10 @@ if test "$vector16" = "yes" ; then
   echo "CONFIG_VECTOR16=y" >> $config_host_mak
 fi
 
+if test "$attralias" = "yes" ; then
+  echo "CONFIG_ATTRIBUTE_ALIAS=y" >> $config_host_mak
+fi
+
 if test "$getauxval" = "yes" ; then
   echo "CONFIG_GETAUXVAL=y" >> $config_host_mak
 fi
-- 
2.17.1

[PATCH v2 4/7] exec: Use const alias for TARGET_PAGE_BITS_VARY

[Richard Henderson]

Using a variable that is declared "const" for this tells the
compiler that it may read the value once and assume that it
does not change across function calls.

For target_page_size, this means we have only one assert per
function, and one read of the variable.

This reduces the size of qemu-system-aarch64 by 8k.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
v2: Notice CONFIG_ATTRIBUTE_ALIAS, and work around Xcode 9 lossage.
---
 include/exec/cpu-all.h | 14 +++++++---
 exec-vary.c            | 60 ++++++++++++++++++++++++++++++++++++------
 2 files changed, 62 insertions(+), 12 deletions(-)

diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
index 255bb186ac..76515dc8d9 100644
--- a/include/exec/cpu-all.h
+++ b/include/exec/cpu-all.h
@@ -210,10 +210,16 @@ static inline void stl_phys_notdirty(AddressSpace *as, hwaddr addr, uint32_t val
 /* page related stuff */
 
 #ifdef TARGET_PAGE_BITS_VARY
-extern bool target_page_bits_decided;
-extern int target_page_bits;
-#define TARGET_PAGE_BITS ({ assert(target_page_bits_decided); \
-                            target_page_bits; })
+typedef struct {
+    bool decided;
+    int bits;
+} TargetPageBits;
+# if defined(CONFIG_ATTRIBUTE_ALIAS) || !defined(IN_EXEC_VARY)
+extern const TargetPageBits target_page;
+#else
+extern TargetPageBits target_page;
+# endif
+#define TARGET_PAGE_BITS (assert(target_page.decided), target_page.bits)
 #else
 #define TARGET_PAGE_BITS_MIN TARGET_PAGE_BITS
 #endif
diff --git a/exec-vary.c b/exec-vary.c
index 48c0ab306c..e0befd502a 100644
--- a/exec-vary.c
+++ b/exec-vary.c
@@ -19,11 +19,55 @@
 
 #include "qemu/osdep.h"
 #include "qemu-common.h"
+
+#define IN_EXEC_VARY 1
+
 #include "exec/exec-all.h"
 
 #ifdef TARGET_PAGE_BITS_VARY
-int target_page_bits;
-bool target_page_bits_decided;
+# ifdef CONFIG_ATTRIBUTE_ALIAS
+/*
+ * We want to declare the "target_page" variable as const, which tells
+ * the compiler that it can cache any value that it reads across calls.
+ * This avoids multiple assertions and multiple reads within any one user.
+ *
+ * This works because we initialize the target_page data very early, in a
+ * location far removed from the functions that require the final results.
+ *
+ * This also requires that we have a non-constant symbol by which we can
+ * perform the actual initialization, and which forces the data to be
+ * allocated within writable memory.  Thus "init_target_page", and we use
+ * that symbol exclusively in the two functions that initialize this value.
+ *
+ * The "target_page" symbol is created as an alias of "init_target_page".
+ */
+static TargetPageBits init_target_page;
+
+/*
+ * Note that this is *not* a redundant decl, this is the definition of
+ * the "target_page" symbol.  The syntax for this definition requires
+ * the use of the extern keyword.  This seems to be a GCC bug in
+ * either the syntax for the alias attribute or in -Wredundant-decls.
+ *
+ * See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91765
+ */
+#  pragma GCC diagnostic push
+#  pragma GCC diagnostic ignored "-Wredundant-decls"
+
+extern const TargetPageBits target_page
+    __attribute__((alias("init_target_page")));
+
+#  pragma GCC diagnostic pop
+# else
+/*
+ * When aliases are not supported then we force two different declarations,
+ * by way of suppressing the header declaration with IN_EXEC_VARY.
+ * We assume that on such an old compiler, LTO cannot be used, and so the
+ * compiler cannot not detect the mismatched declarations, and all is well.
+ */
+TargetPageBits target_page;
+#  define init_target_page target_page
+# endif
 #endif
 
 bool set_preferred_target_page_bits(int bits)
@@ -36,11 +80,11 @@ bool set_preferred_target_page_bits(int bits)
      */
 #ifdef TARGET_PAGE_BITS_VARY
     assert(bits >= TARGET_PAGE_BITS_MIN);
-    if (target_page_bits == 0 || target_page_bits > bits) {
-        if (target_page_bits_decided) {
+    if (init_target_page.bits == 0 || init_target_page.bits > bits) {
+        if (init_target_page.decided) {
             return false;
         }
-        target_page_bits = bits;
+        init_target_page.bits = bits;
     }
 #endif
     return true;
@@ -49,9 +93,9 @@ bool set_preferred_target_page_bits(int bits)
 void finalize_target_page_bits(void)
 {
 #ifdef TARGET_PAGE_BITS_VARY
-    if (target_page_bits == 0) {
-        target_page_bits = TARGET_PAGE_BITS_MIN;
+    if (init_target_page.bits == 0) {
+        init_target_page.bits = TARGET_PAGE_BITS_MIN;
     }
-    target_page_bits_decided = true;
+    init_target_page.decided = true;
 #endif
 }
-- 
2.17.1

[PATCH v2 5/7] exec: Restrict TARGET_PAGE_BITS_VARY assert to CONFIG_DEBUG_TCG

[Richard Henderson]

This reduces the size of a release build by about 10k.
Noticably, within the tlb miss helpers.

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/exec/cpu-all.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
index 76515dc8d9..d3e4660d50 100644
--- a/include/exec/cpu-all.h
+++ b/include/exec/cpu-all.h
@@ -219,7 +219,11 @@ extern const TargetPageBits target_page;
 #else
 extern TargetPageBits target_page;
 # endif
-#define TARGET_PAGE_BITS (assert(target_page.decided), target_page.bits)
+# ifdef CONFIG_DEBUG_TCG
+#  define TARGET_PAGE_BITS (assert(target_page.decided), target_page.bits)
+# else
+#  define TARGET_PAGE_BITS target_page.bits
+# endif
 #else
 #define TARGET_PAGE_BITS_MIN TARGET_PAGE_BITS
 #endif
-- 
2.17.1

[PATCH v2 6/7] exec: Promote TARGET_PAGE_MASK to target_long

[Richard Henderson]

There are some uint64_t uses that expect TARGET_PAGE_MASK to
extend for a 32-bit, so this must continue to be a signed type.
Define based on TARGET_PAGE_BITS not TARGET_PAGE_SIZE; this
will make a following patch more clear.

This should not have a functional effect so far.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/exec/cpu-all.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
index d3e4660d50..ba6d3306bf 100644
--- a/include/exec/cpu-all.h
+++ b/include/exec/cpu-all.h
@@ -229,7 +229,7 @@ extern TargetPageBits target_page;
 #endif
 
 #define TARGET_PAGE_SIZE (1 << TARGET_PAGE_BITS)
-#define TARGET_PAGE_MASK ~(TARGET_PAGE_SIZE - 1)
+#define TARGET_PAGE_MASK ((target_long)-1 << TARGET_PAGE_BITS)
 #define TARGET_PAGE_ALIGN(addr) ROUND_UP((addr), TARGET_PAGE_SIZE)
 
 /* Using intptr_t ensures that qemu_*_page_mask is sign-extended even
-- 
2.17.1

[PATCH v2 7/7] exec: Cache TARGET_PAGE_MASK for TARGET_PAGE_BITS_VARY

[Richard Henderson]

This eliminates a set of runtime shifts.  It turns out that we
require TARGET_PAGE_MASK more often than TARGET_PAGE_SIZE, so
redefine TARGET_PAGE_SIZE based on TARGET_PAGE_MASK instead of
the other way around.

Reviewed-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/exec/cpu-all.h | 8 ++++++--
 exec-vary.c            | 1 +
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
index ba6d3306bf..08b3a5ab06 100644
--- a/include/exec/cpu-all.h
+++ b/include/exec/cpu-all.h
@@ -213,6 +213,7 @@ static inline void stl_phys_notdirty(AddressSpace *as, hwaddr addr, uint32_t val
 typedef struct {
     bool decided;
     int bits;
+    target_long mask;
 } TargetPageBits;
 # if defined(CONFIG_ATTRIBUTE_ALIAS) || !defined(IN_EXEC_VARY)
 extern const TargetPageBits target_page;
@@ -221,15 +222,18 @@ extern TargetPageBits target_page;
 # endif
 # ifdef CONFIG_DEBUG_TCG
 #  define TARGET_PAGE_BITS (assert(target_page.decided), target_page.bits)
+#  define TARGET_PAGE_MASK (assert(target_page.decided), target_page.mask)
 # else
 #  define TARGET_PAGE_BITS target_page.bits
+#  define TARGET_PAGE_MASK target_page.mask
 # endif
+# define TARGET_PAGE_SIZE  (-(int)TARGET_PAGE_MASK)
 #else
 #define TARGET_PAGE_BITS_MIN TARGET_PAGE_BITS
+#define TARGET_PAGE_SIZE   (1 << TARGET_PAGE_BITS)
+#define TARGET_PAGE_MASK   ((target_long)-1 << TARGET_PAGE_BITS)
 #endif
 
-#define TARGET_PAGE_SIZE (1 << TARGET_PAGE_BITS)
-#define TARGET_PAGE_MASK ((target_long)-1 << TARGET_PAGE_BITS)
 #define TARGET_PAGE_ALIGN(addr) ROUND_UP((addr), TARGET_PAGE_SIZE)
 
 /* Using intptr_t ensures that qemu_*_page_mask is sign-extended even
diff --git a/exec-vary.c b/exec-vary.c
index e0befd502a..0594f61fef 100644
--- a/exec-vary.c
+++ b/exec-vary.c
@@ -97,5 +97,6 @@ void finalize_target_page_bits(void)
         init_target_page.bits = TARGET_PAGE_BITS_MIN;
     }
     init_target_page.decided = true;
+    init_target_page.mask = (target_long)-1 << init_target_page.bits;
 #endif
 }
-- 
2.17.1

Re: [PATCH v2 0/7] exec: Improve code for TARGET_PAGE_BITS_VARY

[no-reply]

Patchew URL: https://patchew.org/QEMU/20191023154505.30521-1-richard.henderson@linaro.org/



Hi,

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

Subject: [PATCH v2 0/7] exec: Improve code for TARGET_PAGE_BITS_VARY
Type: series
Message-id: 20191023154505.30521-1-richard.henderson@linaro.org

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Switched to a new branch 'test'
19cd753 exec: Cache TARGET_PAGE_MASK for TARGET_PAGE_BITS_VARY
606b070 exec: Promote TARGET_PAGE_MASK to target_long
896d42b exec: Restrict TARGET_PAGE_BITS_VARY assert to CONFIG_DEBUG_TCG
722d22e exec: Use const alias for TARGET_PAGE_BITS_VARY
6ac4efc configure: Detect compiler support for __attribute__((alias))
e28d025 exec: Split out variable page size support to exec-vary.c
1d02251 cpu: use ROUND_UP() to define xxx_PAGE_ALIGN

=== OUTPUT BEGIN ===
1/7 Checking commit 1d0225166fde (cpu: use ROUND_UP() to define xxx_PAGE_ALIGN)
2/7 Checking commit e28d025c7c6c (exec: Split out variable page size support to exec-vary.c)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#33: 
new file mode 100644

total: 0 errors, 1 warnings, 125 lines checked

Patch 2/7 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
3/7 Checking commit 6ac4efc3168c (configure: Detect compiler support for __attribute__((alias)))
4/7 Checking commit 722d22e4af52 (exec: Use const alias for TARGET_PAGE_BITS_VARY)
ERROR: externs should be avoided in .c files
#65: FILE: exec-vary.c:57:
+extern const TargetPageBits target_page

total: 1 errors, 0 warnings, 103 lines checked

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

5/7 Checking commit 896d42b9ac68 (exec: Restrict TARGET_PAGE_BITS_VARY assert to CONFIG_DEBUG_TCG)
6/7 Checking commit 606b070ea74e (exec: Promote TARGET_PAGE_MASK to target_long)
7/7 Checking commit 19cd7537b926 (exec: Cache TARGET_PAGE_MASK for TARGET_PAGE_BITS_VARY)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20191023154505.30521-1-richard.henderson@linaro.org/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH v2 1/7] cpu: use ROUND_UP() to define xxx_PAGE_ALIGN

[Philippe Mathieu-Daudé]

On 10/23/19 5:44 PM, Richard Henderson wrote:
> From: Wei Yang <richardw.yang@linux.intel.com>
> 
> Use ROUND_UP() to define, which is a little bit easy to read.
> 
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

^ one is enough ;)

> Reviewed-by: Juan Quintela <quintela@redhat.com>
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> Message-Id: <20191013021145.16011-2-richardw.yang@linux.intel.com>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   include/exec/cpu-all.h | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
> index ad9ab85eb3..255bb186ac 100644
> --- a/include/exec/cpu-all.h
> +++ b/include/exec/cpu-all.h
> @@ -220,7 +220,7 @@ extern int target_page_bits;
>   
>   #define TARGET_PAGE_SIZE (1 << TARGET_PAGE_BITS)
>   #define TARGET_PAGE_MASK ~(TARGET_PAGE_SIZE - 1)
> -#define TARGET_PAGE_ALIGN(addr) (((addr) + TARGET_PAGE_SIZE - 1) & TARGET_PAGE_MASK)
> +#define TARGET_PAGE_ALIGN(addr) ROUND_UP((addr), TARGET_PAGE_SIZE)
>   
>   /* Using intptr_t ensures that qemu_*_page_mask is sign-extended even
>    * when intptr_t is 32-bit and we are aligning a long long.
> @@ -228,9 +228,8 @@ extern int target_page_bits;
>   extern uintptr_t qemu_host_page_size;
>   extern intptr_t qemu_host_page_mask;
>   
> -#define HOST_PAGE_ALIGN(addr) (((addr) + qemu_host_page_size - 1) & qemu_host_page_mask)
> -#define REAL_HOST_PAGE_ALIGN(addr) (((addr) + qemu_real_host_page_size - 1) & \
> -                                    qemu_real_host_page_mask)
> +#define HOST_PAGE_ALIGN(addr) ROUND_UP((addr), qemu_host_page_size)
> +#define REAL_HOST_PAGE_ALIGN(addr) ROUND_UP((addr), qemu_real_host_page_size)
>   
>   /* same as PROT_xxx */
>   #define PAGE_READ      0x0001
> 

Re: [PATCH v2 1/7] cpu: use ROUND_UP() to define xxx_PAGE_ALIGN

[Paolo Bonzini]

On 24/10/19 13:52, Philippe Mathieu-Daudé wrote:
>> diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
>> index ad9ab85eb3..255bb186ac 100644
>> --- a/include/exec/cpu-all.h
>> +++ b/include/exec/cpu-all.h
>> @@ -220,7 +220,7 @@ extern int target_page_bits;
>>     #define TARGET_PAGE_SIZE (1 << TARGET_PAGE_BITS)
>>   #define TARGET_PAGE_MASK ~(TARGET_PAGE_SIZE - 1)
>> -#define TARGET_PAGE_ALIGN(addr) (((addr) + TARGET_PAGE_SIZE - 1) &
>> TARGET_PAGE_MASK)
>> +#define TARGET_PAGE_ALIGN(addr) ROUND_UP((addr), TARGET_PAGE_SIZE)
>>     /* Using intptr_t ensures that qemu_*_page_mask is sign-extended even
>>    * when intptr_t is 32-bit and we are aligning a long long.
>> @@ -228,9 +228,8 @@ extern int target_page_bits;
>>   extern uintptr_t qemu_host_page_size;
>>   extern intptr_t qemu_host_page_mask;
>>   -#define HOST_PAGE_ALIGN(addr) (((addr) + qemu_host_page_size - 1) &
>> qemu_host_page_mask)
>> -#define REAL_HOST_PAGE_ALIGN(addr) (((addr) +
>> qemu_real_host_page_size - 1) & \
>> -                                    qemu_real_host_page_mask)
>> +#define HOST_PAGE_ALIGN(addr) ROUND_UP((addr), qemu_host_page_size)
>> +#define REAL_HOST_PAGE_ALIGN(addr) ROUND_UP((addr),
>> qemu_real_host_page_size)
>>     /* same as PROT_xxx */
>>   #define PAGE_READ      0x0001
>>

Isn't this the patch where Richard pointed out that the compiler
generates worse code?

Paolo

Re: [PATCH v2 1/7] cpu: use ROUND_UP() to define xxx_PAGE_ALIGN

[Richard Henderson]

On 10/24/19 8:04 AM, Paolo Bonzini wrote:
> On 24/10/19 13:52, Philippe Mathieu-Daudé wrote:
>>> diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
>>> index ad9ab85eb3..255bb186ac 100644
>>> --- a/include/exec/cpu-all.h
>>> +++ b/include/exec/cpu-all.h
>>> @@ -220,7 +220,7 @@ extern int target_page_bits;
>>>     #define TARGET_PAGE_SIZE (1 << TARGET_PAGE_BITS)
>>>   #define TARGET_PAGE_MASK ~(TARGET_PAGE_SIZE - 1)
>>> -#define TARGET_PAGE_ALIGN(addr) (((addr) + TARGET_PAGE_SIZE - 1) &
>>> TARGET_PAGE_MASK)
>>> +#define TARGET_PAGE_ALIGN(addr) ROUND_UP((addr), TARGET_PAGE_SIZE)
>>>     /* Using intptr_t ensures that qemu_*_page_mask is sign-extended even
>>>    * when intptr_t is 32-bit and we are aligning a long long.
>>> @@ -228,9 +228,8 @@ extern int target_page_bits;
>>>   extern uintptr_t qemu_host_page_size;
>>>   extern intptr_t qemu_host_page_mask;
>>>   -#define HOST_PAGE_ALIGN(addr) (((addr) + qemu_host_page_size - 1) &
>>> qemu_host_page_mask)
>>> -#define REAL_HOST_PAGE_ALIGN(addr) (((addr) +
>>> qemu_real_host_page_size - 1) & \
>>> -                                    qemu_real_host_page_mask)
>>> +#define HOST_PAGE_ALIGN(addr) ROUND_UP((addr), qemu_host_page_size)
>>> +#define REAL_HOST_PAGE_ALIGN(addr) ROUND_UP((addr),
>>> qemu_real_host_page_size)
>>>     /* same as PROT_xxx */
>>>   #define PAGE_READ      0x0001
>>>
> 
> Isn't this the patch where Richard pointed out that the compiler
> generates worse code?

Richard confused ROUND_UP with QEMU_ALIGN_UP.


r~

Re: [PATCH v2 1/7] cpu: use ROUND_UP() to define xxx_PAGE_ALIGN

[Paolo Bonzini]

On 24/10/19 16:06, Richard Henderson wrote:
> On 10/24/19 8:04 AM, Paolo Bonzini wrote:
>> On 24/10/19 13:52, Philippe Mathieu-Daudé wrote:
>>>> diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
>>>> index ad9ab85eb3..255bb186ac 100644
>>>> --- a/include/exec/cpu-all.h
>>>> +++ b/include/exec/cpu-all.h
>>>> @@ -220,7 +220,7 @@ extern int target_page_bits;
>>>>     #define TARGET_PAGE_SIZE (1 << TARGET_PAGE_BITS)
>>>>   #define TARGET_PAGE_MASK ~(TARGET_PAGE_SIZE - 1)
>>>> -#define TARGET_PAGE_ALIGN(addr) (((addr) + TARGET_PAGE_SIZE - 1) &
>>>> TARGET_PAGE_MASK)
>>>> +#define TARGET_PAGE_ALIGN(addr) ROUND_UP((addr), TARGET_PAGE_SIZE)
>>>>     /* Using intptr_t ensures that qemu_*_page_mask is sign-extended even
>>>>    * when intptr_t is 32-bit and we are aligning a long long.
>>>> @@ -228,9 +228,8 @@ extern int target_page_bits;
>>>>   extern uintptr_t qemu_host_page_size;
>>>>   extern intptr_t qemu_host_page_mask;
>>>>   -#define HOST_PAGE_ALIGN(addr) (((addr) + qemu_host_page_size - 1) &
>>>> qemu_host_page_mask)
>>>> -#define REAL_HOST_PAGE_ALIGN(addr) (((addr) +
>>>> qemu_real_host_page_size - 1) & \
>>>> -                                    qemu_real_host_page_mask)
>>>> +#define HOST_PAGE_ALIGN(addr) ROUND_UP((addr), qemu_host_page_size)
>>>> +#define REAL_HOST_PAGE_ALIGN(addr) ROUND_UP((addr),
>>>> qemu_real_host_page_size)
>>>>     /* same as PROT_xxx */
>>>>   #define PAGE_READ      0x0001
>>>>
>>
>> Isn't this the patch where Richard pointed out that the compiler
>> generates worse code?
> 
> Richard confused ROUND_UP with QEMU_ALIGN_UP.

Uh, those are both really badly named.  Especially considering that
DIV_ROUND_UP divides the result of QEMU_ALIGN_UP.  Should we consider
swapping them?!?

Paolo

Re: [PATCH v2 1/7] cpu: use ROUND_UP() to define xxx_PAGE_ALIGN

[Richard Henderson]

On 10/24/19 10:14 AM, Paolo Bonzini wrote:
> On 24/10/19 16:06, Richard Henderson wrote:
>> Richard confused ROUND_UP with QEMU_ALIGN_UP.
> 
> Uh, those are both really badly named.  Especially considering that
> DIV_ROUND_UP divides the result of QEMU_ALIGN_UP.  Should we consider
> swapping them?!?

Perhaps not swapping, but perhaps renaming ROUND_UP to POW2_ROUND_UP or
something.  I dunno, naming is hard.  :-P


r~

Re: [PATCH v2 0/7] exec: Improve code for TARGET_PAGE_BITS_VARY

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> Changes since v1:
>   * Replace my patch for TARGET_PAGE_ALIGN with Wei Yang's.
>   * Detect __attribute__((alias)) support in configure.
>     If unsupported, as for Apple Xcode9, use preprocessor trickery.
>     Passes travis build tests for xcode9.
>
> Link for v1:
>   https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg04519.html
>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

FWIW POW2_ROUND_UP is a better name than it currently has.

>
> r~
>
>
> Richard Henderson (6):
>   exec: Split out variable page size support to exec-vary.c
>   configure: Detect compiler support for __attribute__((alias))
>   exec: Use const alias for TARGET_PAGE_BITS_VARY
>   exec: Restrict TARGET_PAGE_BITS_VARY assert to CONFIG_DEBUG_TCG
>   exec: Promote TARGET_PAGE_MASK to target_long
>   exec: Cache TARGET_PAGE_MASK for TARGET_PAGE_BITS_VARY
>
> Wei Yang (1):
>   cpu: use ROUND_UP() to define xxx_PAGE_ALIGN
>
>  Makefile.target        |   2 +-
>  include/exec/cpu-all.h |  33 +++++++++----
>  include/qemu-common.h  |   6 +++
>  exec-vary.c            | 102 +++++++++++++++++++++++++++++++++++++++++
>  exec.c                 |  34 --------------
>  configure              |  19 ++++++++
>  6 files changed, 151 insertions(+), 45 deletions(-)
>  create mode 100644 exec-vary.c


--
Alex Bennée

Re: [PATCH v2 2/7] exec: Split out variable page size support to exec-vary.c

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> The next patch will play a trick with "const" that will
> confuse the compiler about the uses of target_page_bits
> within exec.c.  Moving everything to a new file prevents
> this confusion.
>
> No functional change so far.
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> ---
>  Makefile.target       |  2 +-
>  include/qemu-common.h |  6 +++++
>  exec-vary.c           | 57 +++++++++++++++++++++++++++++++++++++++++++
>  exec.c                | 34 --------------------------
>  4 files changed, 64 insertions(+), 35 deletions(-)
>  create mode 100644 exec-vary.c
>
> diff --git a/Makefile.target b/Makefile.target
> index 5e916230c4..ca3d14efe1 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -107,7 +107,7 @@ obj-y += trace/
>
>  #########################################################
>  # cpu emulator library
> -obj-y += exec.o
> +obj-y += exec.o exec-vary.o
>  obj-y += accel/
>  obj-$(CONFIG_TCG) += tcg/tcg.o tcg/tcg-op.o tcg/tcg-op-vec.o tcg/tcg-op-gvec.o
>  obj-$(CONFIG_TCG) += tcg/tcg-common.o tcg/optimize.o
> diff --git a/include/qemu-common.h b/include/qemu-common.h
> index 8d84db90b0..082da59e85 100644
> --- a/include/qemu-common.h
> +++ b/include/qemu-common.h
> @@ -74,6 +74,12 @@ void cpu_exec_step_atomic(CPUState *cpu);
>   */
>  bool set_preferred_target_page_bits(int bits);
>
> +/**
> + * finalize_target_page_bits:
> + * Commit the final value set by set_preferred_target_page_bits.
> + */
> +void finalize_target_page_bits(void);
> +
>  /**
>   * Sends a (part of) iovec down a socket, yielding when the socket is full, or
>   * Receives data into a (part of) iovec from a socket,
> diff --git a/exec-vary.c b/exec-vary.c
> new file mode 100644
> index 0000000000..48c0ab306c
> --- /dev/null
> +++ b/exec-vary.c
> @@ -0,0 +1,57 @@
> +/*
> + * Variable page size handling
> + *
> + *  Copyright (c) 2003 Fabrice Bellard
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu-common.h"
> +#include "exec/exec-all.h"
> +
> +#ifdef TARGET_PAGE_BITS_VARY
> +int target_page_bits;
> +bool target_page_bits_decided;
> +#endif
> +
> +bool set_preferred_target_page_bits(int bits)
> +{
> +    /*
> +     * The target page size is the lowest common denominator for all
> +     * the CPUs in the system, so we can only make it smaller, never
> +     * larger. And we can't make it smaller once we've committed to
> +     * a particular size.
> +     */
> +#ifdef TARGET_PAGE_BITS_VARY
> +    assert(bits >= TARGET_PAGE_BITS_MIN);
> +    if (target_page_bits == 0 || target_page_bits > bits) {
> +        if (target_page_bits_decided) {
> +            return false;
> +        }
> +        target_page_bits = bits;
> +    }
> +#endif
> +    return true;
> +}
> +
> +void finalize_target_page_bits(void)
> +{
> +#ifdef TARGET_PAGE_BITS_VARY
> +    if (target_page_bits == 0) {
> +        target_page_bits = TARGET_PAGE_BITS_MIN;
> +    }
> +    target_page_bits_decided = true;
> +#endif
> +}
> diff --git a/exec.c b/exec.c
> index fb0943cfed..5bf181d23e 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -91,11 +91,6 @@ AddressSpace address_space_memory;
>  static MemoryRegion io_mem_unassigned;
>  #endif
>
> -#ifdef TARGET_PAGE_BITS_VARY
> -int target_page_bits;
> -bool target_page_bits_decided;
> -#endif
> -
>  CPUTailQ cpus = QTAILQ_HEAD_INITIALIZER(cpus);
>
>  /* current CPU in the current thread. It is only valid inside
> @@ -109,37 +104,8 @@ int use_icount;
>  uintptr_t qemu_host_page_size;
>  intptr_t qemu_host_page_mask;
>
> -bool set_preferred_target_page_bits(int bits)
> -{
> -    /* The target page size is the lowest common denominator for all
> -     * the CPUs in the system, so we can only make it smaller, never
> -     * larger. And we can't make it smaller once we've committed to
> -     * a particular size.
> -     */
> -#ifdef TARGET_PAGE_BITS_VARY
> -    assert(bits >= TARGET_PAGE_BITS_MIN);
> -    if (target_page_bits == 0 || target_page_bits > bits) {
> -        if (target_page_bits_decided) {
> -            return false;
> -        }
> -        target_page_bits = bits;
> -    }
> -#endif
> -    return true;
> -}
> -
>  #if !defined(CONFIG_USER_ONLY)
>
> -static void finalize_target_page_bits(void)
> -{
> -#ifdef TARGET_PAGE_BITS_VARY
> -    if (target_page_bits == 0) {
> -        target_page_bits = TARGET_PAGE_BITS_MIN;
> -    }
> -    target_page_bits_decided = true;
> -#endif
> -}
> -
>  typedef struct PhysPageEntry PhysPageEntry;
>
>  struct PhysPageEntry {


--
Alex Bennée

Re: [PATCH v2 3/7] configure: Detect compiler support for __attribute__((alias))

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> Such support is present almost everywhere, except for Xcode 9.
> It is added in Xcode 10, but travis uses xcode9 by default,
> so we should support it for a while yet.

We really should get someone who has an AppleID to check what the
support period is because if Apple don't care about Xcode9 anymore I
doubt we should.

Anyway:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  configure | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>
> diff --git a/configure b/configure
> index 145fcabbb3..3a9862fe5e 100755
> --- a/configure
> +++ b/configure
> @@ -5518,6 +5518,21 @@ if compile_prog "" "" ; then
>    vector16=yes
>  fi
>
> +########################################
> +# See if __attribute__((alias)) is supported.
> +# This false for Xcode 9, but has been remedied for Xcode 10.
> +# Unfortunately, travis uses Xcode 9 by default.
> +
> +attralias=no
> +cat > $TMPC << EOF
> +int x = 1;
> +extern const int y __attribute__((alias("x")));
> +int main(void) { return 0; }
> +EOF
> +if compile_prog "" "" ; then
> +    attralias=yes
> +fi
> +
>  ########################################
>  # check if getauxval is available.
>
> @@ -7083,6 +7098,10 @@ if test "$vector16" = "yes" ; then
>    echo "CONFIG_VECTOR16=y" >> $config_host_mak
>  fi
>
> +if test "$attralias" = "yes" ; then
> +  echo "CONFIG_ATTRIBUTE_ALIAS=y" >> $config_host_mak
> +fi
> +
>  if test "$getauxval" = "yes" ; then
>    echo "CONFIG_GETAUXVAL=y" >> $config_host_mak
>  fi


--
Alex Bennée

Re: [PATCH v2 4/7] exec: Use const alias for TARGET_PAGE_BITS_VARY

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> Using a variable that is declared "const" for this tells the
> compiler that it may read the value once and assume that it
> does not change across function calls.
>
> For target_page_size, this means we have only one assert per
> function, and one read of the variable.
>
> This reduces the size of qemu-system-aarch64 by 8k.
>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> ---
> v2: Notice CONFIG_ATTRIBUTE_ALIAS, and work around Xcode 9 lossage.
> ---
>  include/exec/cpu-all.h | 14 +++++++---
>  exec-vary.c            | 60 ++++++++++++++++++++++++++++++++++++------
>  2 files changed, 62 insertions(+), 12 deletions(-)
>
> diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
> index 255bb186ac..76515dc8d9 100644
> --- a/include/exec/cpu-all.h
> +++ b/include/exec/cpu-all.h
> @@ -210,10 +210,16 @@ static inline void stl_phys_notdirty(AddressSpace *as, hwaddr addr, uint32_t val
>  /* page related stuff */
>
>  #ifdef TARGET_PAGE_BITS_VARY
> -extern bool target_page_bits_decided;
> -extern int target_page_bits;
> -#define TARGET_PAGE_BITS ({ assert(target_page_bits_decided); \
> -                            target_page_bits; })
> +typedef struct {
> +    bool decided;
> +    int bits;
> +} TargetPageBits;
> +# if defined(CONFIG_ATTRIBUTE_ALIAS) || !defined(IN_EXEC_VARY)
> +extern const TargetPageBits target_page;
> +#else
> +extern TargetPageBits target_page;
> +# endif
> +#define TARGET_PAGE_BITS (assert(target_page.decided), target_page.bits)
>  #else
>  #define TARGET_PAGE_BITS_MIN TARGET_PAGE_BITS
>  #endif
> diff --git a/exec-vary.c b/exec-vary.c
> index 48c0ab306c..e0befd502a 100644
> --- a/exec-vary.c
> +++ b/exec-vary.c
> @@ -19,11 +19,55 @@
>
>  #include "qemu/osdep.h"
>  #include "qemu-common.h"
> +
> +#define IN_EXEC_VARY 1
> +
>  #include "exec/exec-all.h"
>
>  #ifdef TARGET_PAGE_BITS_VARY
> -int target_page_bits;
> -bool target_page_bits_decided;
> +# ifdef CONFIG_ATTRIBUTE_ALIAS
> +/*
> + * We want to declare the "target_page" variable as const, which tells
> + * the compiler that it can cache any value that it reads across calls.
> + * This avoids multiple assertions and multiple reads within any one user.
> + *
> + * This works because we initialize the target_page data very early, in a
> + * location far removed from the functions that require the final results.
> + *
> + * This also requires that we have a non-constant symbol by which we can
> + * perform the actual initialization, and which forces the data to be
> + * allocated within writable memory.  Thus "init_target_page", and we use
> + * that symbol exclusively in the two functions that initialize this value.
> + *
> + * The "target_page" symbol is created as an alias of "init_target_page".
> + */
> +static TargetPageBits init_target_page;
> +
> +/*
> + * Note that this is *not* a redundant decl, this is the definition of
> + * the "target_page" symbol.  The syntax for this definition requires
> + * the use of the extern keyword.  This seems to be a GCC bug in
> + * either the syntax for the alias attribute or in -Wredundant-decls.
> + *
> + * See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91765
> + */
> +#  pragma GCC diagnostic push
> +#  pragma GCC diagnostic ignored "-Wredundant-decls"
> +
> +extern const TargetPageBits target_page
> +    __attribute__((alias("init_target_page")));
> +
> +#  pragma GCC diagnostic pop
> +# else
> +/*
> + * When aliases are not supported then we force two different declarations,
> + * by way of suppressing the header declaration with IN_EXEC_VARY.
> + * We assume that on such an old compiler, LTO cannot be used, and so the
> + * compiler cannot not detect the mismatched declarations, and all is well.
> + */
> +TargetPageBits target_page;
> +#  define init_target_page target_page
> +# endif
>  #endif
>
>  bool set_preferred_target_page_bits(int bits)
> @@ -36,11 +80,11 @@ bool set_preferred_target_page_bits(int bits)
>       */
>  #ifdef TARGET_PAGE_BITS_VARY
>      assert(bits >= TARGET_PAGE_BITS_MIN);
> -    if (target_page_bits == 0 || target_page_bits > bits) {
> -        if (target_page_bits_decided) {
> +    if (init_target_page.bits == 0 || init_target_page.bits > bits) {
> +        if (init_target_page.decided) {
>              return false;
>          }
> -        target_page_bits = bits;
> +        init_target_page.bits = bits;
>      }
>  #endif
>      return true;
> @@ -49,9 +93,9 @@ bool set_preferred_target_page_bits(int bits)
>  void finalize_target_page_bits(void)
>  {
>  #ifdef TARGET_PAGE_BITS_VARY
> -    if (target_page_bits == 0) {
> -        target_page_bits = TARGET_PAGE_BITS_MIN;
> +    if (init_target_page.bits == 0) {
> +        init_target_page.bits = TARGET_PAGE_BITS_MIN;
>      }
> -    target_page_bits_decided = true;
> +    init_target_page.decided = true;
>  #endif
>  }


--
Alex Bennée

Re: [PATCH v2 5/7] exec: Restrict TARGET_PAGE_BITS_VARY assert to CONFIG_DEBUG_TCG

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> This reduces the size of a release build by about 10k.
> Noticably, within the tlb miss helpers.
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> ---
>  include/exec/cpu-all.h | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
> index 76515dc8d9..d3e4660d50 100644
> --- a/include/exec/cpu-all.h
> +++ b/include/exec/cpu-all.h
> @@ -219,7 +219,11 @@ extern const TargetPageBits target_page;
>  #else
>  extern TargetPageBits target_page;
>  # endif
> -#define TARGET_PAGE_BITS (assert(target_page.decided), target_page.bits)
> +# ifdef CONFIG_DEBUG_TCG
> +#  define TARGET_PAGE_BITS (assert(target_page.decided), target_page.bits)
> +# else
> +#  define TARGET_PAGE_BITS target_page.bits
> +# endif
>  #else
>  #define TARGET_PAGE_BITS_MIN TARGET_PAGE_BITS
>  #endif


--
Alex Bennée

Re: [PATCH v2 4/7] exec: Use const alias for TARGET_PAGE_BITS_VARY

[Peter Maydell]

On Wed, 23 Oct 2019 at 18:06, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Using a variable that is declared "const" for this tells the
> compiler that it may read the value once and assume that it
> does not change across function calls.
>
> For target_page_size, this means we have only one assert per
> function, and one read of the variable.
>
> This reduces the size of qemu-system-aarch64 by 8k.
>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>


> + * We want to declare the "target_page" variable as const, which tells
> + * the compiler that it can cache any value that it reads across calls.
> + * This avoids multiple assertions and multiple reads within any one user.
> + *
> + * This works because we initialize the target_page data very early, in a
> + * location far removed from the functions that require the final results.

I have to say that this feels like a worryingly large amount
of magic. Is this actually guaranteed to work by the compiler?

thanks
-- PMM

Re: [PATCH v2 4/7] exec: Use const alias for TARGET_PAGE_BITS_VARY

[Richard Henderson]

On 10/25/19 10:51 AM, Peter Maydell wrote:
>> + * We want to declare the "target_page" variable as const, which tells
>> + * the compiler that it can cache any value that it reads across calls.
>> + * This avoids multiple assertions and multiple reads within any one user.
>> + *
>> + * This works because we initialize the target_page data very early, in a
>> + * location far removed from the functions that require the final results.
> 
> I have to say that this feels like a worryingly large amount
> of magic. Is this actually guaranteed to work by the compiler?

Yes.


r~

Re: [PATCH v2 4/7] exec: Use const alias for TARGET_PAGE_BITS_VARY

[Peter Maydell]

On Fri, 25 Oct 2019 at 21:43, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 10/25/19 10:51 AM, Peter Maydell wrote:
> >> + * We want to declare the "target_page" variable as const, which tells
> >> + * the compiler that it can cache any value that it reads across calls.
> >> + * This avoids multiple assertions and multiple reads within any one user.
> >> + *
> >> + * This works because we initialize the target_page data very early, in a
> >> + * location far removed from the functions that require the final results.
> >
> > I have to say that this feels like a worryingly large amount
> > of magic. Is this actually guaranteed to work by the compiler?
>
> Yes.

I'm curious to know how the compiler engineers define
"very early" and "far removed" -- in my experience they
usually prefer to be more precise than that :-)

thanks
-- PMM

Re: [PATCH v2 4/7] exec: Use const alias for TARGET_PAGE_BITS_VARY

[Richard Henderson]

On 10/25/19 5:01 PM, Peter Maydell wrote:
> On Fri, 25 Oct 2019 at 21:43, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> On 10/25/19 10:51 AM, Peter Maydell wrote:
>>>> + * We want to declare the "target_page" variable as const, which tells
>>>> + * the compiler that it can cache any value that it reads across calls.
>>>> + * This avoids multiple assertions and multiple reads within any one user.
>>>> + *
>>>> + * This works because we initialize the target_page data very early, in a
>>>> + * location far removed from the functions that require the final results.
>>>
>>> I have to say that this feels like a worryingly large amount
>>> of magic. Is this actually guaranteed to work by the compiler?
>>
>> Yes.
> 
> I'm curious to know how the compiler engineers define
> "very early" and "far removed" -- in my experience they
> usually prefer to be more precise than that :-)

I remembered putting more precise language in there, but I don't see it now.
Perhaps I just dreamt it.

The last write to the non-const variable happens before the first time we
access the const variable.  At the first access to the const variable, we
assert that it has been initialized.

There's no specific barrier to avoid that first read of the const variable not
be hoisted by the compiler before the last store of the non-const variable,
except for being in a separate function, in a separate compilation unit, and
thus "far away".

We could, perhaps, put a barrier() at the end of finalize_target_page_bits(),
documenting this fact against some future date when compilation with -flto is
viable.  I will say, though, that I've tried that recently and quite some work
is required before one could enable -flto.  In the meantime, the barrier()
would compile away to nothing.


r~

Re: [PATCH v2 3/7] configure: Detect compiler support for __attribute__((alias))

[Thomas Huth]

On 25/10/2019 16.04, Alex Bennée wrote:
> 
> Richard Henderson <richard.henderson@linaro.org> writes:
> 
>> Such support is present almost everywhere, except for Xcode 9.
>> It is added in Xcode 10, but travis uses xcode9 by default,
>> so we should support it for a while yet.
> 
> We really should get someone who has an AppleID to check what the
> support period is because if Apple don't care about Xcode9 anymore I
> doubt we should.

The page at https://docs.travis-ci.com/user/reference/osx/ has a nice 
list which Xcode versions are used with which macOS versions.
Xcode10 was also available for macOS 10.13 (which is likely the oldest 
macOS version that we care about), so I think we can assume that macOS 
users that are still using 10.13 can upgrade to xcode10.1 if they want 
to compile QEMU.

  Thomas