[Qemu-devel] [PATCH] tci: Detect function argument alignment

[Qemu-devel] [PATCH] tci: Detect function argument alignment

[Stefan Weil]

This fixes TCI on ARM hosts (tested with Debian's busybox-static
running in linux-user emulation).

Signed-off-by: Stefan Weil <sw@weilnetz.de>
---
 configure            |   63 ++++++++++++++++++++++++++++++++++++++++++++++++++
 tcg/tci/tcg-target.h |    6 ++++-
 2 files changed, 68 insertions(+), 1 deletion(-)

diff --git a/configure b/configure
index 2b83936..93c3a32 100755
--- a/configure
+++ b/configure
@@ -185,6 +185,7 @@ debug_tcg="no"
 debug="no"
 strip_opt="yes"
 tcg_interpreter="no"
+call_align_args=""
 bigendian="no"
 mingw32="no"
 gcov="no"
@@ -808,6 +809,10 @@ for opt do
   ;;
   --enable-tcg-interpreter) tcg_interpreter="yes"
   ;;
+  --disable-call-align-args) call_align_args="no"
+  ;;
+  --enable-call-align-args) call_align_args="yes"
+  ;;
   --disable-cap-ng)  cap_ng="no"
   ;;
   --enable-cap-ng) cap_ng="yes"
@@ -1461,6 +1466,58 @@ esac
 fi
 
 ##########################################
+# Alignment probe for 64 bit function arguments (only needed for TCG interpreter)
+
+if test "$tcg_interpreter" = "yes" -a -z "$call_align_args"; then
+
+if test -z "$cross_prefix"; then
+
+cat > $TMPC << EOF
+#include <stdbool.h>
+#include <stdio.h>
+#include <stdint.h>
+#include <inttypes.h>
+static bool call_align_args(uint32_t arg1, uint64_t arg2) {
+  if (arg2 == 0x000000030000004ULL || arg2 == 0x0000000400000003ULL) {
+    return true;
+  } else if (arg2 == 2) {
+    /* 64 bit host, 64 bit function arguments are not aligned. */
+  } else if (arg2 == 0x0000000200000003 || arg2 == 0x0000000300000002ULL) {
+    /* 64 bit function arguments are not aligned. */
+  } else {
+    fprintf(stderr, "unexpected 64 bit function argument 0x%016" PRIx64 "\n", arg2);
+  }
+  return false;
+}
+
+typedef bool (*fptr)(uint32_t, uint32_t, uint32_t, uint32_t);
+
+int main(void) {
+  fptr f = (fptr)call_align_args;
+  return f(1, 2, 3, 4) ? 0 : 1;
+}
+EOF
+
+if compile_prog "" ""; then
+  call_align_args="no"
+  $TMPE && call_align_args="yes"
+else
+  error_exit "Function argument alignment test failed"
+fi
+
+else
+
+  # Cross compilation, so we cannot launch a program. Require configure argument.
+  error_exit "Unknown function argument alignment" \
+             "The TCG interpreter must know the alignment of function arguments." \
+             "Configure cannot determine the alignment because this is a cross build," \
+             "so please add --enable-call-align-args or --disable-call-align-args."
+
+fi
+
+fi # "$tcg_interpreter" = "yes"
+
+##########################################
 # pkg-config probe
 
 if ! has "$pkg_config_exe"; then
@@ -3709,6 +3766,9 @@ echo "Install blobs     $blobs"
 echo "KVM support       $kvm"
 echo "RDMA support      $rdma"
 echo "TCG interpreter   $tcg_interpreter"
+if test "$tcg_interpreter" = "yes"; then
+echo "call align args   $call_align_args"
+fi
 echo "fdt support       $fdt"
 echo "preadv support    $preadv"
 echo "fdatasync         $fdatasync"
@@ -4029,6 +4089,9 @@ if test "$signalfd" = "yes" ; then
 fi
 if test "$tcg_interpreter" = "yes" ; then
   echo "CONFIG_TCG_INTERPRETER=y" >> $config_host_mak
+  if test "$call_align_args" = "yes"; then
+    echo "CONFIG_CALL_ALIGN_ARGS=y" >> $config_host_mak
+  fi
 fi
 if test "$fdatasync" = "yes" ; then
   echo "CONFIG_FDATASYNC=y" >> $config_host_mak
diff --git a/tcg/tci/tcg-target.h b/tcg/tci/tcg-target.h
index c2ecfbe..0420e96 100644
--- a/tcg/tci/tcg-target.h
+++ b/tcg/tci/tcg-target.h
@@ -1,7 +1,7 @@
 /*
  * Tiny Code Generator for QEMU
  *
- * Copyright (c) 2009, 2011 Stefan Weil
+ * Copyright (c) 2009, 2013 Stefan Weil
  *
  * Permission is hereby granted, free of charge, to any person obtaining a copy
  * of this software and associated documentation files (the "Software"), to deal
@@ -63,6 +63,10 @@
 #endif
 #endif
 
+#if defined(CONFIG_CALL_ALIGN_ARGS)
+# define TCG_TARGET_CALL_ALIGN_ARGS     1
+#endif
+
 /* Optional instructions. */
 
 #define TCG_TARGET_HAS_bswap16_i32      1
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH] tci: Detect function argument alignment

[Richard Henderson]

On 09/12/2013 12:45 PM, Stefan Weil wrote:
> +cat > $TMPC << EOF
> +#include <stdbool.h>
> +#include <stdio.h>
> +#include <stdint.h>
> +#include <inttypes.h>
> +static bool call_align_args(uint32_t arg1, uint64_t arg2) {
> +  if (arg2 == 0x000000030000004ULL || arg2 == 0x0000000400000003ULL) {
> +    return true;
> +  } else if (arg2 == 2) {
> +    /* 64 bit host, 64 bit function arguments are not aligned. */
> +  } else if (arg2 == 0x0000000200000003 || arg2 == 0x0000000300000002ULL) {
> +    /* 64 bit function arguments are not aligned. */
> +  } else {
> +    fprintf(stderr, "unexpected 64 bit function argument 0x%016" PRIx64 "\n", arg2);
> +  }
> +  return false;
> +}

You will of course be printing that error when run on a 64-bit host.

If you're going to go this way, rather than changing to rely on libffi,
then you'll also need to detect TCG_TARGET_EXTEND_ARGS for 64-bit hosts.

Or perhaps skip that detection and define it always.  It's more likely
to be right than not doing it.  This will break if MIPS64 were added,
but then we'd have to extend tcg_gen_callN for that anyway.


r~

Re: [Qemu-devel] [PATCH] tci: Detect function argument alignment

[Stefan Weil]

Am 12.09.2013 22:29, schrieb Richard Henderson:
> On 09/12/2013 12:45 PM, Stefan Weil wrote:
>> +cat > $TMPC << EOF
>> +#include <stdbool.h>
>> +#include <stdio.h>
>> +#include <stdint.h>
>> +#include <inttypes.h>
>> +static bool call_align_args(uint32_t arg1, uint64_t arg2) {
>> +  if (arg2 == 0x000000030000004ULL || arg2 == 0x0000000400000003ULL) {
>> +    return true;
>> +  } else if (arg2 == 2) {
>> +    /* 64 bit host, 64 bit function arguments are not aligned. */
>> +  } else if (arg2 == 0x0000000200000003 || arg2 == 0x0000000300000002ULL) {
>> +    /* 64 bit function arguments are not aligned. */
>> +  } else {
>> +    fprintf(stderr, "unexpected 64 bit function argument 0x%016" PRIx64 "\n", arg2);
>> +  }
>> +  return false;
>> +}
> You will of course be printing that error when run on a 64-bit host.

No. See the comment in the code where a 64 bit host is handled.

> If you're going to go this way, rather than changing to rely on libffi,
> then you'll also need to detect TCG_TARGET_EXTEND_ARGS for 64-bit hosts.

Ok. I had not noticed that macro TCG_TARGET_EXTEND_ARGS before,
but can try to fix that in a separate patch.

For ARM and similar hosts, the current patch does its job.

> Or perhaps skip that detection and define it always.  It's more likely
> to be right than not doing it.  This will break if MIPS64 were added,
> but then we'd have to extend tcg_gen_callN for that anyway.

Would this break x86_64 hosts? They don't define it as far as I could see.

Re: [Qemu-devel] [PATCH] tci: Detect function argument alignment

[Richard Henderson]

On 09/12/2013 01:40 PM, Stefan Weil wrote:
> Am 12.09.2013 22:29, schrieb Richard Henderson:
>> On 09/12/2013 12:45 PM, Stefan Weil wrote:
>>> +cat > $TMPC << EOF
>>> +#include <stdbool.h>
>>> +#include <stdio.h>
>>> +#include <stdint.h>
>>> +#include <inttypes.h>
>>> +static bool call_align_args(uint32_t arg1, uint64_t arg2) {
>>> +  if (arg2 == 0x000000030000004ULL || arg2 == 0x0000000400000003ULL) {
>>> +    return true;
>>> +  } else if (arg2 == 2) {
>>> +    /* 64 bit host, 64 bit function arguments are not aligned. */
>>> +  } else if (arg2 == 0x0000000200000003 || arg2 == 0x0000000300000002ULL) {
>>> +    /* 64 bit function arguments are not aligned. */
>>> +  } else {
>>> +    fprintf(stderr, "unexpected 64 bit function argument 0x%016" PRIx64 "\n", arg2);
>>> +  }
>>> +  return false;
>>> +}
>> You will of course be printing that error when run on a 64-bit host.
> 
> No. See the comment in the code where a 64 bit host is handled.

Oh, I see that now, sorry.

>> Or perhaps skip that detection and define it always.  It's more likely
>> to be right than not doing it.  This will break if MIPS64 were added,
>> but then we'd have to extend tcg_gen_callN for that anyway.
> 
> Would this break x86_64 hosts? They don't define it as far as I could see.

Any 64-bit host that doesn't define TCG_TARGET_EXTEND_ARGS has an
abi that considers the high bits of a 32-bit parameter to be garbage.

Thus extending the value on x86_64 host is unused work, but certainly
not harmful.


r~

Re: [Qemu-devel] [PATCH] tci: Detect function argument alignment

[Peter Maydell]

On 12 September 2013 20:45, Stefan Weil <sw@weilnetz.de> wrote:
> +else
> +
> +  # Cross compilation, so we cannot launch a program. Require configure argument.
> +  error_exit "Unknown function argument alignment" \
> +             "The TCG interpreter must know the alignment of function arguments." \
> +             "Configure cannot determine the alignment because this is a cross build," \
> +             "so please add --enable-call-align-args or --disable-call-align-args."
> +
> +fi

No new configure tests which can't cope with cross compilation,
please.

> +#if defined(CONFIG_CALL_ALIGN_ARGS)
> +# define TCG_TARGET_CALL_ALIGN_ARGS     1
> +#endif

You're not a native assembly backend, you can't rely on this
to be sufficient. Use libffi or call the target function with
the exact correct prototype.

-- PMM

Re: [Qemu-devel] [PATCH] tci: Detect function argument alignment

[Stefan Weil]

Am 12.09.2013 22:35, schrieb Peter Maydell:
> On 12 September 2013 20:45, Stefan Weil <sw@weilnetz.de> wrote:
>> +else
>> +
>> +  # Cross compilation, so we cannot launch a program. Require configure argument.
>> +  error_exit "Unknown function argument alignment" \
>> +             "The TCG interpreter must know the alignment of function arguments." \
>> +             "Configure cannot determine the alignment because this is a cross build," \
>> +             "so please add --enable-call-align-args or --disable-call-align-args."
>> +
>> +fi
> No new configure tests which can't cope with cross compilation,
> please.

I generally agree (and still hope that the endianness patch will be
committed soon). For the calling convention test I don't see an
alternative, see my remarks below.

>
>> +#if defined(CONFIG_CALL_ALIGN_ARGS)
>> +# define TCG_TARGET_CALL_ALIGN_ARGS     1
>> +#endif
> You're not a native assembly backend, you can't rely on this
> to be sufficient. Use libffi or call the target function with
> the exact correct prototype.
>
> -- PMM

I had a look on libffi now and don't see how it could solve my problem.
As far as I could see, libffi must be ported to new architectures, so
its use would restrict the portability of TCI.

Calling the helper function with the correct prototype seems to be the
solution with the best portability and would also make TCI a little bit
faster.
There is a drawback of that solution: it needs modifications in the TCG
opcode generation which would no longer be identical to all other TCG
targets (or I'd have to search the given address of the helper function
in a lookup table which would cost too much time).

Therefore I'd like to implement that solution as a configure time option:
either TCI calls helpers with the correct prototype, or it uses the macros
TCG_TARGET_CALL_ALIGN_ARGS and TCG_TARGET_EXTEND_ARGS.

This means that we still need some way to determine the call alignment
and whether 32 bit arguments are extended to 64 bit values on 64 bit hosts
for the solution with unmodified TCG opcode generator. Any test for this
requires native compilation. Of course I can add the known values for
the common architectures, so these architectures would not require the
test. Then only cross compilation for exotic architectures would require
an explicit configure option which defines argument alignment and
extension.

Regards
Stefan

Re: [Qemu-devel] [PATCH] tci: Detect function argument alignment

[Peter Maydell]

On 14 September 2013 08:18, Stefan Weil <sw@weilnetz.de> wrote:
> Am 12.09.2013 22:35, schrieb Peter Maydell:
>> You're not a native assembly backend, you can't rely on this
>> to be sufficient. Use libffi or call the target function with
>> the exact correct prototype.

> I had a look on libffi now and don't see how it could solve my problem.
> As far as I could see, libffi must be ported to new architectures, so
> its use would restrict the portability of TCI.

Yes, but it's somebody else's problem to port it, not ours.
I present it mostly as an alternative to doing it the hard way.

> Calling the helper function with the correct prototype seems to be the
> solution with the best portability and would also make TCI a little bit
> faster.
> There is a drawback of that solution: it needs modifications in the TCG
> opcode generation which would no longer be identical to all other TCG
> targets (or I'd have to search the given address of the helper function
> in a lookup table which would cost too much time).

Nobody's running TCI for the performance benefit :-) Use a hash
table, there's one in glib and it won't have much overhead at all for lookups.

> Therefore I'd like to implement that solution as a configure time option:
> either TCI calls helpers with the correct prototype, or it uses the macros
> TCG_TARGET_CALL_ALIGN_ARGS and TCG_TARGET_EXTEND_ARGS.
>
> This means that we still need some way to determine the call alignment
> and whether 32 bit arguments are extended to 64 bit values on 64 bit hosts
> for the solution with unmodified TCG opcode generator. Any test for this
> requires native compilation. Of course I can add the known values for
> the common architectures, so these architectures would not require the
> test.

This is worse -- it means the common cases (where there's probably
an ffi port anyhow) won't take the same code paths as the obscure
architectures which are the cases where you might care about TCI
being portable where QEMU otherwise isn't.

-- PMM

Re: [Qemu-devel] [PATCH] tci: Detect function argument alignment

[Richard Henderson]

Michael Walle and Jia Liu, see below wrt minor lm32 and openrisc mistakes.

On 09/14/2013 02:51 AM, Peter Maydell wrote:
>> I had a look on libffi now and don't see how it could solve my problem.
>> As far as I could see, libffi must be ported to new architectures, so
>> its use would restrict the portability of TCI.
> 
> Yes, but it's somebody else's problem to port it, not ours.
> I present it mostly as an alternative to doing it the hard way.

In particular, libffi is used by gcc within its libjava implementation.
Which tends to mean that every (non-embedded) target to which gcc is
ported also gets a port of libffi.

It is tempting to want to avoid the extra external build dependency, but
we have so many now that working too hard to avoid another seems silly.


>> There is a drawback of that solution: it needs modifications in the TCG
>> opcode generation which would no longer be identical to all other TCG
>> targets (or I'd have to search the given address of the helper function
>> in a lookup table which would cost too much time).
> 
> Nobody's running TCI for the performance benefit :-) Use a hash
> table, there's one in glib and it won't have much overhead at all for lookups.

Indeed.  And one can minimize that lookup cost by doing that when building
the TB, as opposed to when we interpret it.

As far as actually interfacing with libffi, one builds ffi_cif structures
that describe the arguments and return value.  Given the contents of the
target-foo/helper.h file, we ought to be able to statically build data
structures of the ffi_type* inputs, and use those to generate all of the
ffi_cif structures within tcg_target_init.

We probably need to clean up the interface from target-foo to tcg.c a bit
to facilitate this.  In the process, we'd be able to reduce some startup
overhead wrt cpu_translate_init.  We currently use

    /* register helpers */
#define GEN_HELPER 2
#include "helper.h"

which expands to, in the case of i386, 514 calls to tcg_register_helper.
We ought to be able to build a static table within tcg.o instead.

It also means that one wouldn't be able to *forget* to register helpers
on the target side.  The lm32 and openrisc target have forgotten that.

That's not to say that we don't still need changes to tcg.c to allow
ffi_call to actually be used.  In particular, getting the data into the
avalue array is non-trivial.  As is getting the results out of rvalue.
One needs to store the argument data such that one knows where the
argument boundaries are.  I.e., at minimum one still needs to know the
sizes of each argument.  That minimum could be passing along the "sizemask"
value used during tcg_gen_callN.  Currently we discard that value there
in tcg_gen_callN, but that's not to say we couldn't store it in the
INDEX_op_call opcode.  And pass it along to tcg_gen_opc as args[1].

Actually, with just that last change we wouldn't necessarily have to
clean up the target-foo/helper.h interface first.  We'd have just enough
info to build the ffi_cif on demand during translation of the INDEX_op_call.


r~