Re: [PATCH v2 63/93] tcg/tci: Use ffi for calls

Re: [PATCH v2 63/93] tcg/tci: Use ffi for calls

[Philippe Mathieu-Daudé]

On 7/2/21 22:33, Stefan Weil wrote:
> On 07.02.21 21:12, Richard Henderson wrote:
>> On 2/7/21 11:52 AM, Peter Maydell wrote:
>>> On Sun, 7 Feb 2021 at 17:41, Richard Henderson
>>> <richard.henderson@linaro.org> wrote:
>>>>
>>>> On 2/7/21 8:25 AM, Stefan Weil wrote:
>>>>>> +#include "qemu-common.h"
>>>>>> +#include "tcg/tcg.h"           /* MAX_OPC_PARAM_IARGS */
>>>>>> +#include "exec/cpu_ldst.h"
>>>>>> +#include "tcg/tcg-op.h"
>>>>>> +#include "qemu/compiler.h"
>>>>>> +#include <ffi.h>
>>>>>> +
>>>>>
>>>>>
>>>>> ffi.h is not found on macOS with Homebrew.
>>>>>
>>>>> This can be fixed by using pkg-config to find the right compiler (and maybe
>>>>> also linker) flags:
>>>>>
>>>>> % pkg-config --cflags libffi
>>>>> -I/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/ffi
>>>>> % pkg-config --libs libffi
>>>>> -lffi
>>>>
>>>>
>>>> Which is exactly what I do in the previous patch:
>>>>
>>>>
>>>>> +++ b/meson.build
>>>>> @@ -1901,7 +1901,14 @@ specific_ss.add(when: 'CONFIG_TCG', if_true: files(
>>>>>     'tcg/tcg-op.c',
>>>>>     'tcg/tcg.c',
>>>>>   ))
>>>>> -specific_ss.add(when: 'CONFIG_TCG_INTERPRETER', if_true: files('tcg/tci.c'))
>>>>> +
>>>>> +if get_option('tcg_interpreter')
>>>>> +  libffi = dependency('libffi', version: '>=3.0',
>>>>> +                      static: enable_static, method: 'pkg-config',
>>>>> +                      required: true)
>>>>> +  specific_ss.add(libffi)
>>>>> +  specific_ss.add(files('tcg/tci.c'))
>>>>> +endif
>>>>
>>>> Did you need a PKG_CONFIG_LIBDIR set for homebrew?
>>>
>>> Is this the "meson doesn't actually add the cflags everywhere it should"
>>> bug again ?
>>
>> I guess so.  I realized after sending this reply that PKG_CONFIG_LIBDIR can't
>> be the answer, since the original configure should have failed if pkg-config
>> didn't find ffi.
>>
>> Was there a resolution to said meson bug?
>
> Meanwhile I noticed an additional detail:
>
> There exist two different pkg-config configurations for libffi on Homebrew:
>
> % pkg-config --cflags libffi
> -I/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/ffi
> % export PKG_CONFIG_PATH="/opt/homebrew/opt/libffi/lib/pkgconfig"
> % pkg-config --cflags libffi
> -I/opt/homebrew/Cellar/libffi/3.3_2/include
>
> By default it points to a system directory which does not exist at all
> on my Mac, so that will never work.
>
> With the right PKG_CONFIG_PATH a correct include directory is set, and
> the latest rebased tci-next branch now works for me with a compiler warning:
>
> /opt/homebrew/Cellar/libffi/3.3_2/include/ffi.h:441:5: warning:
> 'FFI_GO_CLOSURES' is not defined, evaluates to 0 [-Wundef]

This got introduced in
https://github.com/libffi/libffi/commit/e951d64c0852; should we report there?

Cc'ing Jeremy who sent patches around FFI_GO_CLOSURES:
https://github.com/libffi/libffi/pull/586/commits/8c25da7d2cdf8

Meanwhile, about 5 years have passed, I'll post a patch...

[PATCH v2 00/93] TCI fixes and cleanups

[Richard Henderson]

Almost 7 years ago I detailed 5 major problems in tci[1], of
which three still remain:

  * Unaligned accesses to the bytecode stream, which means
    that we immediately SIGBUS on any host requiring alignment.
  * Non-portable calls to helper functions.
  * Full of useless ifdefs and TODOs.

To my mind, this means the code is unmaintained, despite what it
says in MAINTAINERS.  Thus tci *should* be simply removed.
However, every time removal is suggested, someone comes out of the
woodwork and says we should keep it, because it's useful for $FOO.

Anyway, if we're not going to remove tci, then we have to fix it.

Previously, I rewrote tci all in one lump.  Which was obviously a
mistake, since it meant that the patch was never going to get
reviewed.  This time I've done the rewrite in tiny pieces.

Previously, I invented a moderately complex encoding scheme that
allowed any operand to encode a register or an int32_t immediate.
This time the encoding is quite simple: with only 4 exceptions
all operands encoded into 4-bit slots (registers & conditions).
I rely on the new TEMP_CONST optimization to do a decent job of
loading an immediate value into a register once (via movi), and
reusing that across the TB.

There is a disassembler built into tcg/tci.c, replacing the stub
in disas/tci.c, which reuses the same decoding helpers that are
used by the interpreter.  So finally -d out_asm is useful.

This is good enough to pass make check check-tcg with all of the
docker cross-compilers enabled.  I can boot linux with aarch64,
alpha, and s390x guests.


r~


Based-on: 20210203021550.375058-1-richard.henderson@linaro.org
("[PULL 00/24] tcg patch queue")

[1] https://lists.gnu.org/archive/html/qemu-devel/2014-05/msg02594.html

Richard Henderson (91):
  gdbstub: Fix handle_query_xfer_auxv
  tcg: Split out tcg_raise_tb_overflow
  configure: Fix --enable-tcg-interpreter
  tcg: Manage splitwx in tc_ptr_to_region_tree by hand
  tcg/tci: Make tci_tb_ptr thread-local
  tcg/tci: Inline tci_write_reg32s into the only caller
  tcg/tci: Inline tci_write_reg8 into its callers
  tcg/tci: Inline tci_write_reg16 into the only caller
  tcg/tci: Inline tci_write_reg32 into all callers
  tcg/tci: Inline tci_write_reg64 into 64-bit callers
  tcg/tci: Merge INDEX_op_ld8u_{i32,i64}
  tcg/tci: Merge INDEX_op_ld8s_{i32,i64}
  tcg/tci: Merge INDEX_op_ld16u_{i32,i64}
  tcg/tci: Merge INDEX_op_ld16s_{i32,i64}
  tcg/tci: Merge INDEX_op_{ld_i32,ld32u_i64}
  tcg/tci: Merge INDEX_op_st8_{i32,i64}
  tcg/tci: Merge INDEX_op_st16_{i32,i64}
  tcg/tci: Move stack bounds check to compile-time
  tcg/tci: Merge INDEX_op_{st_i32,st32_i64}
  tcg/tci: Use g_assert_not_reached
  tcg/tci: Remove dead code for TCG_TARGET_HAS_div2_*
  tcg/tci: Implement 64-bit division
  tcg/tci: Remove TODO as unused
  tcg/tci: Restrict TCG_TARGET_NB_REGS to 16
  tcg/tci: Fix TCG_REG_R4 misusage
  tcg/tci: Use bool in tcg_out_ri*
  tcg/tci: Remove TCG_CONST
  tcg/tci: Merge identical cases in generation
  tcg/tci: Remove tci_read_r8
  tcg/tci: Remove tci_read_r8s
  tcg/tci: Remove tci_read_r16
  tcg/tci: Remove tci_read_r16s
  tcg/tci: Remove tci_read_r32s
  tcg/tci: Reduce use of tci_read_r64
  tcg/tci: Merge basic arithmetic operations
  tcg/tci: Merge extension operations
  tcg/tci: Remove ifdefs for TCG_TARGET_HAS_ext32[us]_i64
  tcg/tci: Merge bswap operations
  tcg/tci: Merge mov, not and neg operations
  tcg/tci: Rename tci_read_r to tci_read_rval
  tcg/tci: Split out tci_args_rrs
  tcg/tci: Split out tci_args_rr
  tcg/tci: Split out tci_args_rrr
  tcg/tci: Split out tci_args_rrrc
  tcg/tci: Split out tci_args_l
  tcg/tci: Split out tci_args_rrrrrc
  tcg/tci: Split out tci_args_rrcl and tci_args_rrrrcl
  tcg/tci: Split out tci_args_ri and tci_args_rI
  tcg/tci: Reuse tci_args_l for calls.
  tcg/tci: Reuse tci_args_l for exit_tb
  tcg/tci: Reuse tci_args_l for goto_tb
  tcg/tci: Split out tci_args_rrrrrr
  tcg/tci: Split out tci_args_rrrr
  tcg/tci: Clean up deposit operations
  tcg/tci: Reduce qemu_ld/st TCGMemOpIdx operand to 32-bits
  tcg/tci: Split out tci_args_{rrm,rrrm,rrrrm}
  tcg/tci: Hoist op_size checking into tci_args_*
  tcg/tci: Remove tci_disas
  tcg/tci: Implement the disassembler properly
  tcg: Build ffi data structures for helpers
  tcg/tci: Use ffi for calls
  tcg/tci: Improve tcg_target_call_clobber_regs
  tcg/tci: Move call-return regs to end of tcg_target_reg_alloc_order
  tcg/tci: Push opcode emit into each case
  tcg/tci: Split out tcg_out_op_rrs
  tcg/tci: Split out tcg_out_op_l
  tcg/tci: Split out tcg_out_op_p
  tcg/tci: Split out tcg_out_op_rr
  tcg/tci: Split out tcg_out_op_rrr
  tcg/tci: Split out tcg_out_op_rrrc
  tcg/tci: Split out tcg_out_op_rrrrrc
  tcg/tci: Split out tcg_out_op_rrrbb
  tcg/tci: Split out tcg_out_op_rrcl
  tcg/tci: Split out tcg_out_op_rrrrrr
  tcg/tci: Split out tcg_out_op_rrrr
  tcg/tci: Split out tcg_out_op_rrrrcl
  tcg/tci: Split out tcg_out_op_{rrm,rrrm,rrrrm}
  tcg/tci: Split out tcg_out_op_v
  tcg/tci: Split out tcg_out_op_np
  tcg/tci: Split out tcg_out_op_r[iI]
  tcg/tci: Reserve r13 for a temporary
  tcg/tci: Emit setcond before brcond
  tcg/tci: Remove tci_write_reg
  tcg/tci: Change encoding to uint32_t units
  tcg/tci: Implement goto_ptr
  tcg/tci: Implement movcond
  tcg/tci: Implement andc, orc, eqv, nand, nor
  tcg/tci: Implement extract, sextract
  tcg/tci: Implement clz, ctz, ctpop
  tcg/tci: Implement mulu2, muls2
  tcg/tci: Implement add2, sub2

Stefan Weil (2):
  tcg/tci: Implement INDEX_op_ld16s_i32
  tcg/tci: Implement INDEX_op_ld8s_i64

 configure                              |    5 +-
 meson.build                            |    9 +-
 include/exec/exec-all.h                |    2 +-
 include/exec/helper-ffi.h              |  115 ++
 include/exec/helper-tcg.h              |   24 +-
 include/tcg/tcg-opc.h                  |    6 +-
 include/tcg/tcg.h                      |    1 +
 target/hppa/helper.h                   |    2 +
 target/i386/ops_sse_header.h           |    6 +
 target/m68k/helper.h                   |    1 +
 target/ppc/helper.h                    |    3 +
 tcg/tci/tcg-target-con-set.h           |    8 +-
 tcg/tci/tcg-target.h                   |  118 +-
 disas/tci.c                            |   61 -
 gdbstub.c                              |   17 +-
 tcg/tcg-common.c                       |    4 -
 tcg/tcg.c                              |  117 +-
 tcg/tci.c                              | 1695 +++++++++++++-----------
 tcg/tci/tcg-target.c.inc               |  989 +++++++-------
 tcg/tci/README                         |   20 +-
 tests/docker/dockerfiles/fedora.docker |    1 +
 21 files changed, 1734 insertions(+), 1470 deletions(-)
 create mode 100644 include/exec/helper-ffi.h
 delete mode 100644 disas/tci.c

-- 
2.25.1

[PATCH v2 63/93] tcg/tci: Use ffi for calls

[Richard Henderson]

This requires adjusting where arguments are stored.
Place them on the stack at left-aligned positions.
Adjust the stack frame to be at entirely positive offsets.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/tcg/tcg.h        |   1 +
 tcg/tci/tcg-target.h     |   2 +-
 tcg/tcg.c                |  72 ++++++++++++---------
 tcg/tci.c                | 131 ++++++++++++++++++++++-----------------
 tcg/tci/tcg-target.c.inc |  50 +++++++--------
 5 files changed, 143 insertions(+), 113 deletions(-)

diff --git a/include/tcg/tcg.h b/include/tcg/tcg.h
index 0f0695e90d..e5573a9877 100644
--- a/include/tcg/tcg.h
+++ b/include/tcg/tcg.h
@@ -53,6 +53,7 @@
 #define MAX_OPC_PARAM (4 + (MAX_OPC_PARAM_PER_ARG * MAX_OPC_PARAM_ARGS))
 
 #define CPU_TEMP_BUF_NLONGS 128
+#define TCG_STATIC_FRAME_SIZE  (CPU_TEMP_BUF_NLONGS * sizeof(long))
 
 /* Default target word size to pointer size.  */
 #ifndef TCG_TARGET_REG_BITS
diff --git a/tcg/tci/tcg-target.h b/tcg/tci/tcg-target.h
index 52af6d8bc5..4df10e2e83 100644
--- a/tcg/tci/tcg-target.h
+++ b/tcg/tci/tcg-target.h
@@ -161,7 +161,7 @@ typedef enum {
 
 /* Used for function call generation. */
 #define TCG_TARGET_CALL_STACK_OFFSET    0
-#define TCG_TARGET_STACK_ALIGN          16
+#define TCG_TARGET_STACK_ALIGN          8
 
 #define HAVE_TCG_QEMU_TB_EXEC
 
diff --git a/tcg/tcg.c b/tcg/tcg.c
index 6382112215..92aec0d238 100644
--- a/tcg/tcg.c
+++ b/tcg/tcg.c
@@ -208,6 +208,18 @@ static size_t tree_size;
 static TCGRegSet tcg_target_available_regs[TCG_TYPE_COUNT];
 static TCGRegSet tcg_target_call_clobber_regs;
 
+typedef struct TCGHelperInfo {
+    void *func;
+#ifdef CONFIG_TCG_INTERPRETER
+    ffi_cif *cif;
+#endif
+    const char *name;
+    unsigned flags;
+    unsigned sizemask;
+} TCGHelperInfo;
+
+static GHashTable *helper_table;
+
 #if TCG_TARGET_INSN_UNIT_SIZE == 1
 static __attribute__((unused)) inline void tcg_out8(TCGContext *s, uint8_t v)
 {
@@ -1084,16 +1096,6 @@ void tcg_pool_reset(TCGContext *s)
     s->pool_current = NULL;
 }
 
-typedef struct TCGHelperInfo {
-    void *func;
-#ifdef CONFIG_TCG_INTERPRETER
-    ffi_cif *cif;
-#endif
-    const char *name;
-    unsigned flags;
-    unsigned sizemask;
-} TCGHelperInfo;
-
 #include "exec/helper-proto.h"
 
 #ifdef CONFIG_TCG_INTERPRETER
@@ -1103,7 +1105,6 @@ typedef struct TCGHelperInfo {
 static const TCGHelperInfo all_helpers[] = {
 #include "exec/helper-tcg.h"
 };
-static GHashTable *helper_table;
 
 static int indirect_reg_alloc_order[ARRAY_SIZE(tcg_target_reg_alloc_order)];
 static void process_op_defs(TCGContext *s);
@@ -2081,25 +2082,38 @@ void tcg_gen_callN(void *func, TCGTemp *ret, int nargs, TCGTemp **args)
 
     real_args = 0;
     for (i = 0; i < nargs; i++) {
-        int is_64bit = sizemask & (1 << (i+1)*2);
-        if (TCG_TARGET_REG_BITS < 64 && is_64bit) {
-#ifdef TCG_TARGET_CALL_ALIGN_ARGS
-            /* some targets want aligned 64 bit args */
-            if (real_args & 1) {
-                op->args[pi++] = TCG_CALL_DUMMY_ARG;
-                real_args++;
-            }
+        bool is_64bit = sizemask & (1 << (i+1)*2);
+        bool want_align = false;
+
+#if defined(CONFIG_TCG_INTERPRETER)
+        /*
+         * Align all arguments, so that they land in predictable places
+         * for passing off to ffi_call.
+         */
+        want_align = true;
+#elif defined(TCG_TARGET_CALL_ALIGN_ARGS)
+        /* Some targets want aligned 64 bit args */
+        want_align = is_64bit;
 #endif
-           /* If stack grows up, then we will be placing successive
-              arguments at lower addresses, which means we need to
-              reverse the order compared to how we would normally
-              treat either big or little-endian.  For those arguments
-              that will wind up in registers, this still works for
-              HPPA (the only current STACK_GROWSUP target) since the
-              argument registers are *also* allocated in decreasing
-              order.  If another such target is added, this logic may
-              have to get more complicated to differentiate between
-              stack arguments and register arguments.  */
+
+        if (TCG_TARGET_REG_BITS < 64 && want_align && (real_args & 1)) {
+            op->args[pi++] = TCG_CALL_DUMMY_ARG;
+            real_args++;
+        }
+
+        if (TCG_TARGET_REG_BITS < 64 && is_64bit) {
+           /*
+            * If stack grows up, then we will be placing successive
+            * arguments at lower addresses, which means we need to
+            * reverse the order compared to how we would normally
+            * treat either big or little-endian.  For those arguments
+            * that will wind up in registers, this still works for
+            * HPPA (the only current STACK_GROWSUP target) since the
+            * argument registers are *also* allocated in decreasing
+            * order.  If another such target is added, this logic may
+            * have to get more complicated to differentiate between
+            * stack arguments and register arguments.
+            */
 #if defined(HOST_WORDS_BIGENDIAN) != defined(TCG_TARGET_STACK_GROWSUP)
             op->args[pi++] = temp_arg(args[i] + 1);
             op->args[pi++] = temp_arg(args[i]);
diff --git a/tcg/tci.c b/tcg/tci.c
index 6843e837ae..d27db9f720 100644
--- a/tcg/tci.c
+++ b/tcg/tci.c
@@ -18,6 +18,13 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu-common.h"
+#include "tcg/tcg.h"           /* MAX_OPC_PARAM_IARGS */
+#include "exec/cpu_ldst.h"
+#include "tcg/tcg-op.h"
+#include "qemu/compiler.h"
+#include <ffi.h>
+
 
 /* Enable TCI assertions only when debugging TCG (and without NDEBUG defined).
  * Without assertions, the interpreter runs much faster. */
@@ -27,36 +34,8 @@
 # define tci_assert(cond) ((void)(cond))
 #endif
 
-#include "qemu-common.h"
-#include "tcg/tcg.h"           /* MAX_OPC_PARAM_IARGS */
-#include "exec/cpu_ldst.h"
-#include "tcg/tcg-op.h"
-#include "qemu/compiler.h"
-
-#if MAX_OPC_PARAM_IARGS != 6
-# error Fix needed, number of supported input arguments changed!
-#endif
-#if TCG_TARGET_REG_BITS == 32
-typedef uint64_t (*helper_function)(tcg_target_ulong, tcg_target_ulong,
-                                    tcg_target_ulong, tcg_target_ulong,
-                                    tcg_target_ulong, tcg_target_ulong,
-                                    tcg_target_ulong, tcg_target_ulong,
-                                    tcg_target_ulong, tcg_target_ulong,
-                                    tcg_target_ulong, tcg_target_ulong);
-#else
-typedef uint64_t (*helper_function)(tcg_target_ulong, tcg_target_ulong,
-                                    tcg_target_ulong, tcg_target_ulong,
-                                    tcg_target_ulong, tcg_target_ulong);
-#endif
-
 __thread uintptr_t tci_tb_ptr;
 
-static tcg_target_ulong tci_read_reg(const tcg_target_ulong *regs, TCGReg index)
-{
-    tci_assert(index < TCG_TARGET_NB_REGS);
-    return regs[index];
-}
-
 static void
 tci_write_reg(tcg_target_ulong *regs, TCGReg index, tcg_target_ulong value)
 {
@@ -131,6 +110,7 @@ static tcg_target_ulong tci_read_label(const uint8_t **tb_ptr)
  *   i = immediate (uint32_t)
  *   I = immediate (tcg_target_ulong)
  *   m = immediate (TCGMemOpIdx)
+ *   n = immediate (call return length)
  *   r = register
  *   s = signed ldst offset
  */
@@ -151,6 +131,16 @@ static void tci_args_l(const uint8_t **tb_ptr, void **l0)
     check_size(start, tb_ptr);
 }
 
+static void tci_args_nl(const uint8_t **tb_ptr, uint8_t *n0, void **l1)
+{
+    const uint8_t *start = *tb_ptr;
+
+    *n0 = tci_read_b(tb_ptr);
+    *l1 = (void *)tci_read_label(tb_ptr);
+
+    check_size(start, tb_ptr);
+}
+
 static void tci_args_rr(const uint8_t **tb_ptr,
                         TCGReg *r0, TCGReg *r1)
 {
@@ -491,6 +481,7 @@ static bool tci_compare64(uint64_t u0, uint64_t u1, TCGCond condition)
 # define CASE_64(x)
 #endif
 
+
 /* Interpret pseudo code in tb. */
 /*
  * Disable CFI checks.
@@ -502,11 +493,13 @@ uintptr_t QEMU_DISABLE_CFI tcg_qemu_tb_exec(CPUArchState *env,
 {
     const uint8_t *tb_ptr = v_tb_ptr;
     tcg_target_ulong regs[TCG_TARGET_NB_REGS];
-    long tcg_temps[CPU_TEMP_BUF_NLONGS];
-    uintptr_t sp_value = (uintptr_t)(tcg_temps + CPU_TEMP_BUF_NLONGS);
+    uint64_t stack[(TCG_STATIC_CALL_ARGS_SIZE + TCG_STATIC_FRAME_SIZE)
+                   / sizeof(uint64_t)];
+    void *call_slots[TCG_STATIC_CALL_ARGS_SIZE / sizeof(uint64_t)];
 
     regs[TCG_AREG0] = (tcg_target_ulong)env;
-    regs[TCG_REG_CALL_STACK] = sp_value;
+    regs[TCG_REG_CALL_STACK] = (uintptr_t)stack;
+    call_slots[0] = NULL;
     tci_assert(tb_ptr);
 
     for (;;) {
@@ -531,33 +524,53 @@ uintptr_t QEMU_DISABLE_CFI tcg_qemu_tb_exec(CPUArchState *env,
 
         switch (opc) {
         case INDEX_op_call:
-            tci_args_l(&tb_ptr, &ptr);
+            /*
+             * We are passed a pointer to the TCGHelperInfo, which contains
+             * the function pointer followed by the ffi_cif pointer.
+             */
+            tci_args_nl(&tb_ptr, &len, &ptr);
+
+            /* Helper functions may need to access the "return address" */
             tci_tb_ptr = (uintptr_t)tb_ptr;
-#if TCG_TARGET_REG_BITS == 32
-            tmp64 = ((helper_function)ptr)(tci_read_reg(regs, TCG_REG_R0),
-                                           tci_read_reg(regs, TCG_REG_R1),
-                                           tci_read_reg(regs, TCG_REG_R2),
-                                           tci_read_reg(regs, TCG_REG_R3),
-                                           tci_read_reg(regs, TCG_REG_R4),
-                                           tci_read_reg(regs, TCG_REG_R5),
-                                           tci_read_reg(regs, TCG_REG_R6),
-                                           tci_read_reg(regs, TCG_REG_R7),
-                                           tci_read_reg(regs, TCG_REG_R8),
-                                           tci_read_reg(regs, TCG_REG_R9),
-                                           tci_read_reg(regs, TCG_REG_R10),
-                                           tci_read_reg(regs, TCG_REG_R11));
-            tci_write_reg(regs, TCG_REG_R0, tmp64);
-            tci_write_reg(regs, TCG_REG_R1, tmp64 >> 32);
-#else
-            tmp64 = ((helper_function)ptr)(tci_read_reg(regs, TCG_REG_R0),
-                                           tci_read_reg(regs, TCG_REG_R1),
-                                           tci_read_reg(regs, TCG_REG_R2),
-                                           tci_read_reg(regs, TCG_REG_R3),
-                                           tci_read_reg(regs, TCG_REG_R4),
-                                           tci_read_reg(regs, TCG_REG_R5));
-            tci_write_reg(regs, TCG_REG_R0, tmp64);
-#endif
+
+            /*
+             * Set up the ffi_avalue array once, delayed until now
+             * because many TB's do not make any calls. In tcg_gen_callN,
+             * we arranged for every real argument to be "left-aligned"
+             * in each 64-bit slot.
+             */
+            if (call_slots[0] == NULL) {
+                for (int i = 0; i < ARRAY_SIZE(call_slots); ++i) {
+                    call_slots[i] = &stack[i];
+                }
+            }
+
+            /*
+             * Call the helper function.  Any result winds up
+             * "left-aligned" in the stack[0] slot.
+             */
+            {
+                void **pptr = ptr;
+                ffi_call(pptr[1], pptr[0], stack, call_slots);
+            }
+            switch (len) {
+            case 0: /* void */
+                break;
+            case 1: /* uint32_t */
+                regs[TCG_REG_R0] = *(uint32_t *)stack;
+                break;
+            case 2: /* uint64_t */
+                if (TCG_TARGET_REG_BITS == 32) {
+                    tci_write_reg64(regs, TCG_REG_R1, TCG_REG_R0, stack[0]);
+                } else {
+                    regs[TCG_REG_R0] = stack[0];
+                }
+                break;
+            default:
+                g_assert_not_reached();
+            }
             break;
+
         case INDEX_op_br:
             tci_args_l(&tb_ptr, &ptr);
             tb_ptr = ptr;
@@ -1162,13 +1175,17 @@ int print_insn_tci(bfd_vma addr, disassemble_info *info)
 
     switch (op) {
     case INDEX_op_br:
-    case INDEX_op_call:
     case INDEX_op_exit_tb:
     case INDEX_op_goto_tb:
         tci_args_l(&tb_ptr, &ptr);
         info->fprintf_func(info->stream, "%-12s  %p", op_name, ptr);
         break;
 
+    case INDEX_op_call:
+        tci_args_nl(&tb_ptr, &len, &ptr);
+        info->fprintf_func(info->stream, "%-12s  %d,%p", op_name, len, ptr);
+        break;
+
     case INDEX_op_brcond_i32:
     case INDEX_op_brcond_i64:
         tci_args_rrcl(&tb_ptr, &r0, &r1, &c, &ptr);
diff --git a/tcg/tci/tcg-target.c.inc b/tcg/tci/tcg-target.c.inc
index 7fb3b04eaf..8d75482546 100644
--- a/tcg/tci/tcg-target.c.inc
+++ b/tcg/tci/tcg-target.c.inc
@@ -192,23 +192,8 @@ static const int tcg_target_reg_alloc_order[] = {
 # error Fix needed, number of supported input arguments changed!
 #endif
 
-static const int tcg_target_call_iarg_regs[] = {
-    TCG_REG_R0,
-    TCG_REG_R1,
-    TCG_REG_R2,
-    TCG_REG_R3,
-    TCG_REG_R4,
-    TCG_REG_R5,
-#if TCG_TARGET_REG_BITS == 32
-    /* 32 bit hosts need 2 * MAX_OPC_PARAM_IARGS registers. */
-    TCG_REG_R6,
-    TCG_REG_R7,
-    TCG_REG_R8,
-    TCG_REG_R9,
-    TCG_REG_R10,
-    TCG_REG_R11,
-#endif
-};
+/* No call arguments via registers.  All will be stored on the "stack". */
+static const int tcg_target_call_iarg_regs[] = { };
 
 static const int tcg_target_call_oarg_regs[] = {
     TCG_REG_R0,
@@ -292,8 +277,9 @@ static void tci_out_label(TCGContext *s, TCGLabel *label)
 static void stack_bounds_check(TCGReg base, target_long offset)
 {
     if (base == TCG_REG_CALL_STACK) {
-        tcg_debug_assert(offset < 0);
-        tcg_debug_assert(offset >= -(CPU_TEMP_BUF_NLONGS * sizeof(long)));
+        tcg_debug_assert(offset >= 0);
+        tcg_debug_assert(offset < (TCG_STATIC_CALL_ARGS_SIZE +
+                                   TCG_STATIC_FRAME_SIZE));
     }
 }
 
@@ -360,11 +346,25 @@ static void tcg_out_movi(TCGContext *s, TCGType type,
     old_code_ptr[1] = s->code_ptr - old_code_ptr;
 }
 
-static inline void tcg_out_call(TCGContext *s, const tcg_insn_unit *arg)
+static void tcg_out_call(TCGContext *s, const tcg_insn_unit *arg)
 {
     uint8_t *old_code_ptr = s->code_ptr;
+    const TCGHelperInfo *info;
+    uint8_t which;
+
+    info = g_hash_table_lookup(helper_table, (gpointer)arg);
+    if (info->cif->rtype == &ffi_type_void) {
+        which = 0;
+    } else if (info->cif->rtype->size == 4) {
+        which = 1;
+    } else {
+        tcg_debug_assert(info->cif->rtype->size == 8);
+        which = 2;
+    }
     tcg_out_op_t(s, INDEX_op_call);
-    tcg_out_i(s, (uintptr_t)arg);
+    tcg_out8(s, which);
+    tcg_out_i(s, (uintptr_t)info);
+
     old_code_ptr[1] = s->code_ptr - old_code_ptr;
 }
 
@@ -629,11 +629,9 @@ static void tcg_target_init(TCGContext *s)
     s->reserved_regs = 0;
     tcg_regset_set_reg(s->reserved_regs, TCG_REG_CALL_STACK);
 
-    /* We use negative offsets from "sp" so that we can distinguish
-       stores that might pretend to be call arguments.  */
-    tcg_set_frame(s, TCG_REG_CALL_STACK,
-                  -CPU_TEMP_BUF_NLONGS * sizeof(long),
-                  CPU_TEMP_BUF_NLONGS * sizeof(long));
+    /* The call arguments come first, followed by the temp storage. */
+    tcg_set_frame(s, TCG_REG_CALL_STACK, TCG_STATIC_CALL_ARGS_SIZE,
+                  TCG_STATIC_FRAME_SIZE);
 }
 
 /* Generate global QEMU prologue and epilogue code. */
-- 
2.25.1

Re: [PATCH v2 63/93] tcg/tci: Use ffi for calls

[Stefan Weil]

Am 04.02.21 um 02:44 schrieb Richard Henderson:

> This requires adjusting where arguments are stored.
> Place them on the stack at left-aligned positions.
> Adjust the stack frame to be at entirely positive offsets.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
[...]
> diff --git a/tcg/tci.c b/tcg/tci.c
> index 6843e837ae..d27db9f720 100644
> --- a/tcg/tci.c
> +++ b/tcg/tci.c
> @@ -18,6 +18,13 @@
>    */
>   
>   #include "qemu/osdep.h"
> +#include "qemu-common.h"
> +#include "tcg/tcg.h"           /* MAX_OPC_PARAM_IARGS */
> +#include "exec/cpu_ldst.h"
> +#include "tcg/tcg-op.h"
> +#include "qemu/compiler.h"
> +#include <ffi.h>
> +


ffi.h is not found on macOS with Homebrew.

This can be fixed by using pkg-config to find the right compiler (and 
maybe also linker) flags:

% pkg-config --cflags libffi
-I/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/ffi
% pkg-config --libs libffi
-lffi

Regards,

Stefan

Re: [PATCH v2 63/93] tcg/tci: Use ffi for calls

[Richard Henderson]

On 2/7/21 8:25 AM, Stefan Weil wrote:
>> +#include "qemu-common.h"
>> +#include "tcg/tcg.h"           /* MAX_OPC_PARAM_IARGS */
>> +#include "exec/cpu_ldst.h"
>> +#include "tcg/tcg-op.h"
>> +#include "qemu/compiler.h"
>> +#include <ffi.h>
>> +
> 
> 
> ffi.h is not found on macOS with Homebrew.
> 
> This can be fixed by using pkg-config to find the right compiler (and maybe
> also linker) flags:
> 
> % pkg-config --cflags libffi
> -I/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/ffi
> % pkg-config --libs libffi
> -lffi


Which is exactly what I do in the previous patch:


> +++ b/meson.build
> @@ -1901,7 +1901,14 @@ specific_ss.add(when: 'CONFIG_TCG', if_true: files(
>    'tcg/tcg-op.c',
>    'tcg/tcg.c',
>  ))
> -specific_ss.add(when: 'CONFIG_TCG_INTERPRETER', if_true: files('tcg/tci.c'))
> +
> +if get_option('tcg_interpreter')
> +  libffi = dependency('libffi', version: '>=3.0',
> +                      static: enable_static, method: 'pkg-config',
> +                      required: true)
> +  specific_ss.add(libffi)
> +  specific_ss.add(files('tcg/tci.c'))
> +endif

Did you need a PKG_CONFIG_LIBDIR set for homebrew?


r~

Re: [PATCH v2 63/93] tcg/tci: Use ffi for calls

[Peter Maydell]

On Sun, 7 Feb 2021 at 17:41, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 2/7/21 8:25 AM, Stefan Weil wrote:
> >> +#include "qemu-common.h"
> >> +#include "tcg/tcg.h"           /* MAX_OPC_PARAM_IARGS */
> >> +#include "exec/cpu_ldst.h"
> >> +#include "tcg/tcg-op.h"
> >> +#include "qemu/compiler.h"
> >> +#include <ffi.h>
> >> +
> >
> >
> > ffi.h is not found on macOS with Homebrew.
> >
> > This can be fixed by using pkg-config to find the right compiler (and maybe
> > also linker) flags:
> >
> > % pkg-config --cflags libffi
> > -I/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/ffi
> > % pkg-config --libs libffi
> > -lffi
>
>
> Which is exactly what I do in the previous patch:
>
>
> > +++ b/meson.build
> > @@ -1901,7 +1901,14 @@ specific_ss.add(when: 'CONFIG_TCG', if_true: files(
> >    'tcg/tcg-op.c',
> >    'tcg/tcg.c',
> >  ))
> > -specific_ss.add(when: 'CONFIG_TCG_INTERPRETER', if_true: files('tcg/tci.c'))
> > +
> > +if get_option('tcg_interpreter')
> > +  libffi = dependency('libffi', version: '>=3.0',
> > +                      static: enable_static, method: 'pkg-config',
> > +                      required: true)
> > +  specific_ss.add(libffi)
> > +  specific_ss.add(files('tcg/tci.c'))
> > +endif
>
> Did you need a PKG_CONFIG_LIBDIR set for homebrew?

Is this the "meson doesn't actually add the cflags everywhere it should"
bug again ?

thanks
-- PMM

Re: [PATCH v2 63/93] tcg/tci: Use ffi for calls

[Richard Henderson]

On 2/7/21 11:52 AM, Peter Maydell wrote:
> On Sun, 7 Feb 2021 at 17:41, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> On 2/7/21 8:25 AM, Stefan Weil wrote:
>>>> +#include "qemu-common.h"
>>>> +#include "tcg/tcg.h"           /* MAX_OPC_PARAM_IARGS */
>>>> +#include "exec/cpu_ldst.h"
>>>> +#include "tcg/tcg-op.h"
>>>> +#include "qemu/compiler.h"
>>>> +#include <ffi.h>
>>>> +
>>>
>>>
>>> ffi.h is not found on macOS with Homebrew.
>>>
>>> This can be fixed by using pkg-config to find the right compiler (and maybe
>>> also linker) flags:
>>>
>>> % pkg-config --cflags libffi
>>> -I/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/ffi
>>> % pkg-config --libs libffi
>>> -lffi
>>
>>
>> Which is exactly what I do in the previous patch:
>>
>>
>>> +++ b/meson.build
>>> @@ -1901,7 +1901,14 @@ specific_ss.add(when: 'CONFIG_TCG', if_true: files(
>>>    'tcg/tcg-op.c',
>>>    'tcg/tcg.c',
>>>  ))
>>> -specific_ss.add(when: 'CONFIG_TCG_INTERPRETER', if_true: files('tcg/tci.c'))
>>> +
>>> +if get_option('tcg_interpreter')
>>> +  libffi = dependency('libffi', version: '>=3.0',
>>> +                      static: enable_static, method: 'pkg-config',
>>> +                      required: true)
>>> +  specific_ss.add(libffi)
>>> +  specific_ss.add(files('tcg/tci.c'))
>>> +endif
>>
>> Did you need a PKG_CONFIG_LIBDIR set for homebrew?
> 
> Is this the "meson doesn't actually add the cflags everywhere it should"
> bug again ?

I guess so.  I realized after sending this reply that PKG_CONFIG_LIBDIR can't
be the answer, since the original configure should have failed if pkg-config
didn't find ffi.

Was there a resolution to said meson bug?


r~

Re: [PATCH v2 63/93] tcg/tci: Use ffi for calls

[Stefan Weil]

On 07.02.21 21:12, Richard Henderson wrote:
> On 2/7/21 11:52 AM, Peter Maydell wrote:
>> On Sun, 7 Feb 2021 at 17:41, Richard Henderson
>> <richard.henderson@linaro.org> wrote:
>>>
>>> On 2/7/21 8:25 AM, Stefan Weil wrote:
>>>>> +#include "qemu-common.h"
>>>>> +#include "tcg/tcg.h"           /* MAX_OPC_PARAM_IARGS */
>>>>> +#include "exec/cpu_ldst.h"
>>>>> +#include "tcg/tcg-op.h"
>>>>> +#include "qemu/compiler.h"
>>>>> +#include <ffi.h>
>>>>> +
>>>>
>>>>
>>>> ffi.h is not found on macOS with Homebrew.
>>>>
>>>> This can be fixed by using pkg-config to find the right compiler (and maybe
>>>> also linker) flags:
>>>>
>>>> % pkg-config --cflags libffi
>>>> -I/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/ffi
>>>> % pkg-config --libs libffi
>>>> -lffi
>>>
>>>
>>> Which is exactly what I do in the previous patch:
>>>
>>>
>>>> +++ b/meson.build
>>>> @@ -1901,7 +1901,14 @@ specific_ss.add(when: 'CONFIG_TCG', if_true: files(
>>>>    'tcg/tcg-op.c',
>>>>    'tcg/tcg.c',
>>>>  ))
>>>> -specific_ss.add(when: 'CONFIG_TCG_INTERPRETER', if_true: files('tcg/tci.c'))
>>>> +
>>>> +if get_option('tcg_interpreter')
>>>> +  libffi = dependency('libffi', version: '>=3.0',
>>>> +                      static: enable_static, method: 'pkg-config',
>>>> +                      required: true)
>>>> +  specific_ss.add(libffi)
>>>> +  specific_ss.add(files('tcg/tci.c'))
>>>> +endif
>>>
>>> Did you need a PKG_CONFIG_LIBDIR set for homebrew?
>>
>> Is this the "meson doesn't actually add the cflags everywhere it should"
>> bug again ?
> 
> I guess so.  I realized after sending this reply that PKG_CONFIG_LIBDIR can't
> be the answer, since the original configure should have failed if pkg-config
> didn't find ffi.
> 
> Was there a resolution to said meson bug?

Meanwhile I noticed an additional detail:

There exist two different pkg-config configurations for libffi on Homebrew:

% pkg-config --cflags libffi
-I/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/ffi
% export PKG_CONFIG_PATH="/opt/homebrew/opt/libffi/lib/pkgconfig"
% pkg-config --cflags libffi
-I/opt/homebrew/Cellar/libffi/3.3_2/include

By default it points to a system directory which does not exist at all
on my Mac, so that will never work.

With the right PKG_CONFIG_PATH a correct include directory is set, and
the latest rebased tci-next branch now works for me with a compiler warning:

/opt/homebrew/Cellar/libffi/3.3_2/include/ffi.h:441:5: warning:
'FFI_GO_CLOSURES' is not defined, evaluates to 0 [-Wundef]

Stefan

Re: [PATCH v2 63/93] tcg/tci: Use ffi for calls

[Peter Maydell]

On Sun, 7 Feb 2021 at 20:12, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 2/7/21 11:52 AM, Peter Maydell wrote:
> > On Sun, 7 Feb 2021 at 17:41, Richard Henderson
> > <richard.henderson@linaro.org> wrote:
> >>
> >> On 2/7/21 8:25 AM, Stefan Weil wrote:
> >>>> +#include "qemu-common.h"
> >>>> +#include "tcg/tcg.h"           /* MAX_OPC_PARAM_IARGS */
> >>>> +#include "exec/cpu_ldst.h"
> >>>> +#include "tcg/tcg-op.h"
> >>>> +#include "qemu/compiler.h"
> >>>> +#include <ffi.h>
> >>>> +
> >>>
> >>>
> >>> ffi.h is not found on macOS with Homebrew.
> >>>
> >>> This can be fixed by using pkg-config to find the right compiler (and maybe
> >>> also linker) flags:
> >>>
> >>> % pkg-config --cflags libffi
> >>> -I/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/ffi
> >>> % pkg-config --libs libffi
> >>> -lffi
> >>
> >>
> >> Which is exactly what I do in the previous patch:
> >>
> >>
> >>> +++ b/meson.build
> >>> @@ -1901,7 +1901,14 @@ specific_ss.add(when: 'CONFIG_TCG', if_true: files(
> >>>    'tcg/tcg-op.c',
> >>>    'tcg/tcg.c',
> >>>  ))
> >>> -specific_ss.add(when: 'CONFIG_TCG_INTERPRETER', if_true: files('tcg/tci.c'))
> >>> +
> >>> +if get_option('tcg_interpreter')
> >>> +  libffi = dependency('libffi', version: '>=3.0',
> >>> +                      static: enable_static, method: 'pkg-config',
> >>> +                      required: true)
> >>> +  specific_ss.add(libffi)
> >>> +  specific_ss.add(files('tcg/tci.c'))
> >>> +endif
> >>
> >> Did you need a PKG_CONFIG_LIBDIR set for homebrew?
> >
> > Is this the "meson doesn't actually add the cflags everywhere it should"
> > bug again ?
>
> I guess so.  I realized after sending this reply that PKG_CONFIG_LIBDIR can't
> be the answer, since the original configure should have failed if pkg-config
> didn't find ffi.
>
> Was there a resolution to said meson bug?

There's a workaround involving adding the library to the dependencies
list in the right places -- commit 3eacf70bb5a83e4 did this for gnutls.
Paolo may be able to help further.

thanks
-- PMM

Re: [PATCH v2 63/93] tcg/tci: Use ffi for calls

[Paolo Bonzini]

On 08/02/21 10:20, Peter Maydell wrote:
>>>> +
>>>> +if get_option('tcg_interpreter')
>>>> +  libffi = dependency('libffi', version: '>=3.0',
>>>> +                      static: enable_static, method: 'pkg-config',
>>>> +                      required: true)
>>>> +  specific_ss.add(libffi)
>>>> +  specific_ss.add(files('tcg/tci.c'))
>>>> +endif
>>> Did you need a PKG_CONFIG_LIBDIR set for homebrew?
>> Is this the "meson doesn't actually add the cflags everywhere it should"
>> bug again ?

No, it shouldn't be the same bug.  In this case the dependency is not 
indirect dependency, specific_ss is included directly:

   target_specific = specific_ss.apply(config_target, strict: false)
   arch_srcs += target_specific.sources()
   arch_deps += target_specific.dependencies()

   lib = static_library('qemu-' + target,
                  sources: arch_srcs + genh,
                  dependencies: arch_deps,
                  objects: objects,
                  include_directories: target_inc,
                  c_args: c_args,
                  build_by_default: false,
                  name_suffix: 'fa')

It's more likely to be what Stefan pointed out later:

> Meanwhile I noticed an additional detail:
> 
> There exist two different pkg-config configurations for libffi on Homebrew:
> 
> % pkg-config --cflags libffi
> -I/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/ffi
> % export PKG_CONFIG_PATH="/opt/homebrew/opt/libffi/lib/pkgconfig"
> % pkg-config --cflags libffi
> -I/opt/homebrew/Cellar/libffi/3.3_2/include
>
> By default it points to a system directory which does not exist at all
> on my Mac, so that will never work.

Paolo

Re: [PATCH v2 63/93] tcg/tci: Use ffi for calls

[Stefan Weil]

Am 08.02.21 um 10:35 schrieb Paolo Bonzini:

> On 08/02/21 10:20, Peter Maydell wrote:
>>>>> +
>>>>> +if get_option('tcg_interpreter')
>>>>> +  libffi = dependency('libffi', version: '>=3.0',
>>>>> +                      static: enable_static, method: 'pkg-config',
>>>>> +                      required: true)
>>>>> +  specific_ss.add(libffi)
>>>>> +  specific_ss.add(files('tcg/tci.c'))
>>>>> +endif
>>>> Did you need a PKG_CONFIG_LIBDIR set for homebrew?
>>> Is this the "meson doesn't actually add the cflags everywhere it 
>>> should"
>>> bug again ?
>
> No, it shouldn't be the same bug.  In this case the dependency is not 
> indirect dependency, specific_ss is included directly:
>
>   target_specific = specific_ss.apply(config_target, strict: false)
>   arch_srcs += target_specific.sources()
>   arch_deps += target_specific.dependencies()
>
>   lib = static_library('qemu-' + target,
>                  sources: arch_srcs + genh,
>                  dependencies: arch_deps,
>                  objects: objects,
>                  include_directories: target_inc,
>                  c_args: c_args,
>                  build_by_default: false,
>                  name_suffix: 'fa')
>
> It's more likely to be what Stefan pointed out later:
>
>> Meanwhile I noticed an additional detail:
>>
>> There exist two different pkg-config configurations for libffi on 
>> Homebrew:
>>
>> % pkg-config --cflags libffi
>> -I/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/ffi
>> % export PKG_CONFIG_PATH="/opt/homebrew/opt/libffi/lib/pkgconfig"
>> % pkg-config --cflags libffi
>> -I/opt/homebrew/Cellar/libffi/3.3_2/include
>>
>> By default it points to a system directory which does not exist at all
>> on my Mac, so that will never work.
>
> Paolo


Yes, it looks like setting the right PKG_CONFIG_PATH is sufficient for 
builds with Homebrew on macOS.

Richard, this commit is also the one which breaks qemu-system-i386 on 
sparc64 for me:

sw@gcc102:~/src/gitlab/qemu-project/qemu$ git bisect good
115a01c323e6a01902894ec23ba704bf3dc8215a is the first bad commit
commit 115a01c323e6a01902894ec23ba704bf3dc8215a
Author: Richard Henderson <richard.henderson@linaro.org>
Date:   Sat Jan 30 14:24:25 2021 -0800

     tcg/tci: Use ffi for calls

     This requires adjusting where arguments are stored.
     Place them on the stack at left-aligned positions.
     Adjust the stack frame to be at entirely positive offsets.

     Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

  include/tcg/tcg.h        |   1 +
  tcg/tcg.c                |  72 +++++++++++++++-----------
  tcg/tci.c                | 131 
++++++++++++++++++++++++++---------------------
  tcg/tci/tcg-target.c.inc |  50 +++++++++---------
  tcg/tci/tcg-target.h     |   2 +-
  5 files changed, 143 insertions(+), 113 deletions(-)

Regards,

Stefan

Re: [PATCH v2 63/93] tcg/tci: Use ffi for calls

[Richard Henderson]

On 2/8/21 5:07 AM, Stefan Weil wrote:
> Richard, this commit is also the one which breaks qemu-system-i386 on sparc64
> for me:

You'll have to give me more details than that, because qemu-system-i386 works
for me on a niagara5 w/ debian sid.


r~

Re: [PATCH v2 63/93] tcg/tci: Use ffi for calls

[Stefan Weil]

Am 08.02.21 um 18:39 schrieb Richard Henderson:
> On 2/8/21 5:07 AM, Stefan Weil wrote:
>> Richard, this commit is also the one which breaks qemu-system-i386 on sparc64
>> for me:
> You'll have to give me more details than that, because qemu-system-i386 works
> for me on a niagara5 w/ debian sid.


I am testing on a similar Debian system (debian-ports unstable), but 
with a Niagara3 cpu:

Linux gcc102.fsffrance.org 5.10.0-3-sparc64-smp #1 SMP Debian 5.10.12-1 
(2021-01-30) sparc64 GNU/Linux

gcc (Debian 10.2.1-6) 10.2.1 20210110

$ cat /proc/cpuinfo
cpu        : UltraSparc T3 (Niagara3)
fpu        : UltraSparc T3 integrated FPU
pmu        : niagara3
prom        : OBP 4.34.6.c 2017/03/22 13:55
type        : sun4v
ncpus probed    : 256
ncpus active    : 256
D$ parity tl1    : 0
I$ parity tl1    : 0
cpucaps        : 
flush,stbar,swap,muldiv,v9,blkinit,n2,mul32,div32,v8plus,popc,vis,vis2,ASIBlkInit,fmaf,vis3,hpc
[...]

During "git bisect" I had to apply fixes for unaligned memory access as 
your series starts with TCI code which does not include my patch for 
that. The first bisect step was tested with your tci-next branch, the 
last step was tested with my bisect-tci branch 
(https://gitlab.com/stweil/qemu/-/commits/bisect-tci).

Stefan

Re: [PATCH v2 63/93] tcg/tci: Use ffi for calls

[Richard Henderson]

On 2/8/21 11:04 AM, Stefan Weil wrote:
> 
> Am 08.02.21 um 18:39 schrieb Richard Henderson:
>> On 2/8/21 5:07 AM, Stefan Weil wrote:
>>> Richard, this commit is also the one which breaks qemu-system-i386 on sparc64
>>> for me:
>> You'll have to give me more details than that, because qemu-system-i386 works
>> for me on a niagara5 w/ debian sid.
> 
> 
> I am testing on a similar Debian system (debian-ports unstable), but with a
> Niagara3 cpu:
> 
> Linux gcc102.fsffrance.org 5.10.0-3-sparc64-smp #1 SMP Debian 5.10.12-1
> (2021-01-30) sparc64 GNU/Linux
> 
> gcc (Debian 10.2.1-6) 10.2.1 20210110
> 
> $ cat /proc/cpuinfo
> cpu        : UltraSparc T3 (Niagara3)
> fpu        : UltraSparc T3 integrated FPU
> pmu        : niagara3
> prom        : OBP 4.34.6.c 2017/03/22 13:55
> type        : sun4v
> ncpus probed    : 256
> ncpus active    : 256
> D$ parity tl1    : 0
> I$ parity tl1    : 0
> cpucaps        :
> flush,stbar,swap,muldiv,v9,blkinit,n2,mul32,div32,v8plus,popc,vis,vis2,ASIBlkInit,fmaf,vis3,hpc
> 

Ok, I've reproduced something on a T3 (gcc102.fsffrance.org).
Running the same code side-by-side vs the T5, I get different results.

I'll see if I can track down the difference, since they're both running the
same base os.


r~

Re: [PATCH v2 63/93] tcg/tci: Use ffi for calls

[Richard Henderson]

[-- Attachment #1: Type: text/plain, Size: 353 bytes --]

On 2/8/21 2:55 PM, Richard Henderson wrote:
> Ok, I've reproduced something on a T3 (gcc102.fsffrance.org).
> Running the same code side-by-side vs the T5, I get different results.

Brown paper bag time: the T5 build dir lost the --enable-tcg-interpreter flag,
so was testing tcg native.

Big-endian bug wrt an odd api wart in libffi.  Fixed thus.


r~

[-- Attachment #2: ffi.patch --]
[-- Type: text/x-patch, Size: 893 bytes --]

diff --git a/tcg/tci.c b/tcg/tci.c
index d27db9f720..dd0cca296a 100644
--- a/tcg/tci.c
+++ b/tcg/tci.c
@@ -557,8 +557,15 @@ uintptr_t QEMU_DISABLE_CFI tcg_qemu_tb_exec(CPUArchState *env,
             case 0: /* void */
                 break;
             case 1: /* uint32_t */
-                regs[TCG_REG_R0] = *(uint32_t *)stack;
-                break;
+                /*
+                 * Note that libffi has an odd special case in that it will
+                 * always widen an integral result to ffi_arg.
+                 */
+                if (sizeof(ffi_arg) == 4) {
+                    regs[TCG_REG_R0] = *(uint32_t *)stack;
+                    break;
+                }
+                /* fall through */
             case 2: /* uint64_t */
                 if (TCG_TARGET_REG_BITS == 32) {
                     tci_write_reg64(regs, TCG_REG_R1, TCG_REG_R0, stack[0]);

Re: [PATCH v2 63/93] tcg/tci: Use ffi for calls

[Stefan Weil]

Am 09.02.21 um 21:46 schrieb Richard Henderson:

> On 2/8/21 2:55 PM, Richard Henderson wrote:
>> Ok, I've reproduced something on a T3 (gcc102.fsffrance.org).
>> Running the same code side-by-side vs the T5, I get different results.
> Brown paper bag time: the T5 build dir lost the --enable-tcg-interpreter flag,
> so was testing tcg native.
>
> Big-endian bug wrt an odd api wart in libffi.  Fixed thus.


Thanks for solving this. The patch works for me.

BIOS boot time with qemu-system-i386 is about 41 s (with my code which 
lacks thread support and ffi it was 40 s).

With qemu-system-x86_64 it is twice as fast, so it looks like in my last 
report where I said that the new code had doubled the speed I compared 
different system emulations.

Apropos "slow" TCI: on Apple's M1 it is faster than native TCG on most 
of my Intel / AMD machines. And it works with current git master while 
native TCG still waits for pending patches which fix the memory access.

Stefan

Re: [PATCH v2 63/93] tcg/tci: Use ffi for calls

[Stefan Weil]

Am 09.02.21 um 22:15 schrieb Stefan Weil:

>
> Thanks for solving this. The patch works for me.
>
> BIOS boot time with qemu-system-i386 is about 41 s (with my code which 
> lacks thread support and ffi it was 40 s).
>
> With qemu-system-x86_64 it is twice as fast, so it looks like in my 
> last report where I said that the new code had doubled the speed I 
> compared different system emulations.


Update: with Richard's latest tci-next branch which includes the fixed 
code both qemu-system-x86_64 and qemu-system-i386 require about 20 s 
user time for the BIOS boot.

Stefan