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

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(-)

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