Message ID | 1364985128-23772-7-git-send-email-pbonzini@redhat.com |
---|---|
State | New |
Headers | show |
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
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
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
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
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
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
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
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
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(-)