Patchwork [v2,06/10] elfload: only give abi_long/ulong the alignment specified by the target

login
register
mail settings
Submitter Paolo Bonzini
Date April 3, 2013, 10:32 a.m.
Message ID <1364985128-23772-7-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/233398/
State New
Headers show

Comments

Paolo Bonzini - April 3, 2013, 10:32 a.m.
Previously, this was done for target_long/ulong, and propagated to
abi_long/ulong via a typedef.  But target_long/ulong should not
have any specific alignment, it is never used to access guest
memory.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 configure                    |  9 ---------
 include/exec/cpu-defs.h      |  8 ++++----
 include/exec/user/abitypes.h | 19 +++++++++++++------
 target-m68k/cpu.h            |  1 +
 4 files changed, 18 insertions(+), 19 deletions(-)
Peter Maydell - April 4, 2013, 2:09 p.m.
On 3 April 2013 11:32, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Previously, this was done for target_long/ulong, and propagated to
> abi_long/ulong via a typedef.  But target_long/ulong should not
> have any specific alignment, it is never used to access guest
> memory.

Agreed in principle, but you seem to have missed some structs
which use target_ulong currently and which presumably should
use abi_ulong instead, eg all the target_ucontext etc structs
in linux-user/signal.c

Also linux-user/elfload.c:symfind() is casting a pointer to
target_ulong* and dereferencing it, and that might now cause
an alignment fault on some host CPUs if the host CPU alignment
requirements are stricter than the guest's.

thanks
-- PMM
Paolo Bonzini - April 4, 2013, 2:11 p.m.
Il 04/04/2013 16:09, Peter Maydell ha scritto:
> Agreed in principle, but you seem to have missed some structs
> which use target_ulong currently and which presumably should
> use abi_ulong instead, eg all the target_ucontext etc structs
> in linux-user/signal.c

Right.

> Also linux-user/elfload.c:symfind() is casting a pointer to
> target_ulong* and dereferencing it, and that might now cause
> an alignment fault on some host CPUs if the host CPU alignment
> requirements are stricter than the guest's.

I had seen this, but it is only used with bsearch and safe:

static const char *lookup_symbolxx(struct syminfo *s, target_ulong orig_addr)
{
#if ELF_CLASS == ELFCLASS32
    struct elf_sym *syms = s->disas_symtab.elf32;
#else
    struct elf_sym *syms = s->disas_symtab.elf64;
#endif

    // binary search
    struct elf_sym *sym;

    sym = bsearch(&orig_addr, syms, s->disas_num_syms, sizeof(*syms), symfind);
    ...
}

Paolo
Peter Maydell - April 4, 2013, 2:18 p.m.
On 4 April 2013 15:11, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 04/04/2013 16:09, Peter Maydell ha scritto:
>> Also linux-user/elfload.c:symfind() is casting a pointer to
>> target_ulong* and dereferencing it, and that might now cause
>> an alignment fault on some host CPUs if the host CPU alignment
>> requirements are stricter than the guest's.
>
> I had seen this, but it is only used with bsearch and safe

Ah yes, you're right. Sorry.

-- PMM
Paolo Bonzini - April 4, 2013, 2:26 p.m.
Il 04/04/2013 16:18, Peter Maydell ha scritto:
>>> Also linux-user/elfload.c:symfind() is casting a pointer to
>>> >> target_ulong* and dereferencing it, and that might now cause
>>> >> an alignment fault on some host CPUs if the host CPU alignment
>>> >> requirements are stricter than the guest's.
>> >
>> > I had seen this, but it is only used with bsearch and safe
> Ah yes, you're right. Sorry.

Regarding the others, none of them are in target-generic places, and
none of them affect m68k (ARM only uses non-standard alignment for llong):

- linux-user/mips64/syscall.h is correct with target_ulong, and in
general MIPS is best left as it is (it often uses uint32_t/uint64_t or
target_long/ulong explicitly so that n32 is handled correctly).

- linux-user/openrisc/syscall.h could use abi_ulong instead of
target_ulong, and abi_uint instead of uint32_t, but it doesn't change
anything so it is more of a cleanup

- linux-user/syscall_defs.h's use of target_ulong for st_ino would be a
bug, but on these architectures target_ulong==abi_ulong.  In general the
whole struct should be using abi_* types, but again it is more of a cleanup.

So the patch is okay as is, I think.

Paolo
Peter Maydell - April 4, 2013, 2:37 p.m.
On 4 April 2013 15:26, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Regarding the others, none of them are in target-generic places, and
> none of them affect m68k (ARM only uses non-standard alignment for llong):
>
> - linux-user/mips64/syscall.h is correct with target_ulong, and in
> general MIPS is best left as it is (it often uses uint32_t/uint64_t or
> target_long/ulong explicitly so that n32 is handled correctly).

Hmm, is this really right? target_ulong before this patch would
have had an explicit alignment attribute, and it no longer does.
So if you're running a mips64 guest on an m68k host then you'll
now get structs with the natural m68k alignment rather than the
desired mips64 alignment...

(I can entirely believe that we get this wrong in a lot of
places, and that in theory just about anything in a target_
struct needs an alignment specifier.)

I'm running your patchset through an LTP test run for ARM
guest, by the way.

-- PMM
Paolo Bonzini - April 4, 2013, 2:45 p.m.
Il 04/04/2013 16:37, Peter Maydell ha scritto:
>> Regarding the others, none of them are in target-generic places, and
>> > none of them affect m68k (ARM only uses non-standard alignment for llong):
>> >
>> > - linux-user/mips64/syscall.h is correct with target_ulong, and in
>> > general MIPS is best left as it is (it often uses uint32_t/uint64_t or
>> > target_long/ulong explicitly so that n32 is handled correctly).
> Hmm, is this really right? target_ulong before this patch would
> have had an explicit alignment attribute, and it no longer does.
> So if you're running a mips64 guest on an m68k host then you'll
> now get structs with the natural m68k alignment rather than the
> desired mips64 alignment...

Note that target_ulong and abi_ulong would have different sizes.  Better
to get alignment wrong on m68k, than size wrong on all platforms. :)

Also I think this should not be a problem.  These structures have no
holes in them, the padding is always written down explicitly and they
are accessed (or should be) via copy_from/to_user.

> (I can entirely believe that we get this wrong in a lot of
> places, and that in theory just about anything in a target_
> struct needs an alignment specifier.)

Yes, or more simply just use abi_* types.  It doesn't help that
potential problems would only show up on m68k.

Paolo
Peter Maydell - April 4, 2013, 2:50 p.m.
On 4 April 2013 15:45, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 04/04/2013 16:37, Peter Maydell ha scritto:
>> (I can entirely believe that we get this wrong in a lot of
>> places, and that in theory just about anything in a target_
>> struct needs an alignment specifier.)
>
> Yes, or more simply just use abi_* types.  It doesn't help that
> potential problems would only show up on m68k.

Mmm. Maybe we could use an addition to HACKING which at
least defines what the "right thing" is for new target
structs (and when you should use abi_* vs target_*).

PS: LTP test run says no new failures, so that's good.

-- PMM

Patch

diff --git a/configure b/configure
index 8276e79..0763047 100755
--- a/configure
+++ b/configure
@@ -4011,7 +4011,6 @@  bflt="no"
 target_nptl="no"
 interp_prefix1=`echo "$interp_prefix" | sed "s/%M/$target_arch2/g"`
 gdb_xml_files=""
-target_long_alignment=4
 target_llong_alignment=8
 target_libs_softmmu=
 
@@ -4024,10 +4023,8 @@  case "$target_arch2" in
   ;;
   x86_64)
     TARGET_BASE_ARCH=i386
-    target_long_alignment=8
   ;;
   alpha)
-    target_long_alignment=8
     target_nptl="yes"
   ;;
   arm|armeb)
@@ -4046,7 +4043,6 @@  case "$target_arch2" in
   m68k)
     bflt="yes"
     gdb_xml_files="cf-core.xml cf-fp.xml"
-    target_long_alignment=2
     target_llong_alignment=2
   ;;
   microblaze|microblazeel)
@@ -4069,7 +4065,6 @@  case "$target_arch2" in
     TARGET_ARCH=mips64
     TARGET_BASE_ARCH=mips
     echo "TARGET_ABI_MIPSN64=y" >> $config_target_mak
-    target_long_alignment=8
   ;;
   moxie)
   ;;
@@ -4091,7 +4086,6 @@  case "$target_arch2" in
     TARGET_BASE_ARCH=ppc
     TARGET_ABI_DIR=ppc
     gdb_xml_files="power64-core.xml power-fpu.xml power-altivec.xml power-spe.xml"
-    target_long_alignment=8
   ;;
   ppc64abi32)
     TARGET_ARCH=ppc64
@@ -4109,7 +4103,6 @@  case "$target_arch2" in
   ;;
   sparc64)
     TARGET_BASE_ARCH=sparc
-    target_long_alignment=8
   ;;
   sparc32plus)
     TARGET_ARCH=sparc64
@@ -4119,7 +4112,6 @@  case "$target_arch2" in
   ;;
   s390x)
     target_nptl="yes"
-    target_long_alignment=8
   ;;
   unicore32)
   ;;
@@ -4151,7 +4143,6 @@  case "$cpu" in
   ;;
 esac
 
-echo "TARGET_LONG_ALIGNMENT=$target_long_alignment" >> $config_target_mak
 echo "TARGET_LLONG_ALIGNMENT=$target_llong_alignment" >> $config_target_mak
 echo "TARGET_ARCH=$TARGET_ARCH" >> $config_target_mak
 target_arch_name="`upper $TARGET_ARCH`"
diff --git a/include/exec/cpu-defs.h b/include/exec/cpu-defs.h
index 2aa9331..3cf1272 100644
--- a/include/exec/cpu-defs.h
+++ b/include/exec/cpu-defs.h
@@ -40,14 +40,14 @@  typedef int64_t target_llong __attribute__((aligned(TARGET_LLONG_ALIGNMENT)));
 typedef uint64_t target_ullong __attribute__((aligned(TARGET_LLONG_ALIGNMENT)));
 /* target_ulong is the type of a virtual address */
 #if TARGET_LONG_SIZE == 4
-typedef int32_t target_long __attribute__((aligned(TARGET_LONG_ALIGNMENT)));
-typedef uint32_t target_ulong __attribute__((aligned(TARGET_LONG_ALIGNMENT)));
+typedef int32_t target_long;
+typedef uint32_t target_ulong;
 #define TARGET_FMT_lx "%08x"
 #define TARGET_FMT_ld "%d"
 #define TARGET_FMT_lu "%u"
 #elif TARGET_LONG_SIZE == 8
-typedef int64_t target_long __attribute__((aligned(TARGET_LONG_ALIGNMENT)));
-typedef uint64_t target_ulong __attribute__((aligned(TARGET_LONG_ALIGNMENT)));
+typedef int64_t target_long;
+typedef uint64_t target_ulong;
 #define TARGET_FMT_lx "%016" PRIx64
 #define TARGET_FMT_ld "%" PRId64
 #define TARGET_FMT_lu "%" PRIu64
diff --git a/include/exec/user/abitypes.h b/include/exec/user/abitypes.h
index 97a87a6..9bf916a 100644
--- a/include/exec/user/abitypes.h
+++ b/include/exec/user/abitypes.h
@@ -2,12 +2,21 @@ 
 #define QEMU_TYPES_H
 #include "cpu.h"
 
+#ifdef TARGET_ABI32
+#define TARGET_ABI_BITS 32
+#else
+#define TARGET_ABI_BITS TARGET_LONG_BITS
+#endif
+
 #ifndef ABI_SHORT_ALIGNMENT
 #define ABI_SHORT_ALIGNMENT 2
 #endif
 #ifndef ABI_INT_ALIGNMENT
 #define ABI_INT_ALIGNMENT 4
 #endif
+#ifndef ABI_LONG_ALIGNMENT
+#define ABI_LONG_ALIGNMENT (TARGET_ABI_BITS / 8)
+#endif
 
 typedef int16_t abi_short __attribute__ ((aligned(ABI_SHORT_ALIGNMENT)));
 typedef uint16_t abi_ushort __attribute__((aligned(ABI_SHORT_ALIGNMENT)));
@@ -15,12 +24,11 @@  typedef int32_t abi_int __attribute__((aligned(ABI_INT_ALIGNMENT)));
 typedef uint32_t abi_uint __attribute__((aligned(ABI_INT_ALIGNMENT)));
 
 #ifdef TARGET_ABI32
-typedef uint32_t abi_ulong;
-typedef int32_t abi_long;
+typedef uint32_t abi_ulong __attribute__((aligned(ABI_LONG_ALIGNMENT)));
+typedef int32_t abi_long __attribute__((aligned(ABI_LONG_ALIGNMENT)));
 #define TARGET_ABI_FMT_lx "%08x"
 #define TARGET_ABI_FMT_ld "%d"
 #define TARGET_ABI_FMT_lu "%u"
-#define TARGET_ABI_BITS 32
 
 static inline abi_ulong tswapal(abi_ulong v)
 {
@@ -28,12 +36,11 @@  static inline abi_ulong tswapal(abi_ulong v)
 }
 
 #else
-typedef target_ulong abi_ulong;
-typedef target_long abi_long;
+typedef target_ulong abi_ulong __attribute__((aligned(ABI_LONG_ALIGNMENT)));
+typedef target_long abi_long __attribute__((aligned(ABI_LONG_ALIGNMENT)));
 #define TARGET_ABI_FMT_lx TARGET_FMT_lx
 #define TARGET_ABI_FMT_ld TARGET_FMT_ld
 #define TARGET_ABI_FMT_lu TARGET_FMT_lu
-#define TARGET_ABI_BITS TARGET_LONG_BITS
 /* for consistency, define ABI32 too */
 #if TARGET_ABI_BITS == 32
 #define TARGET_ABI32 1
diff --git a/target-m68k/cpu.h b/target-m68k/cpu.h
index 29cc887..6825e7c 100644
--- a/target-m68k/cpu.h
+++ b/target-m68k/cpu.h
@@ -22,6 +22,7 @@ 
 
 #define TARGET_LONG_BITS 32
 #define ABI_INT_ALIGNMENT 2
+#define ABI_LONG_ALIGNMENT 2
 
 #define CPUArchState struct CPUM68KState