| Message ID | 1379015124-21055-1-git-send-email-sw@weilnetz.de |
|---|---|
| State | Under Review |
| Headers | show |
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~
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
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.
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~
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
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
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~
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
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(-)