Patchwork [v2,02/10] elfload: fix size of registers for N32

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

Comments

Paolo Bonzini - April 3, 2013, 10:32 a.m.
Registers are 64-bit in size for the MIPS n32 ABI.  Define
target_elf_greg_t accordingly, and use the correct function
to do endian swaps.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 linux-user/elfload.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)
Peter Maydell - April 4, 2013, 3:32 p.m.
On 3 April 2013 11:32, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Registers are 64-bit in size for the MIPS n32 ABI.  Define
> target_elf_greg_t accordingly, and use the correct function
> to do endian swaps.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  linux-user/elfload.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index d3589ff..9d5dbb8 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c
> @@ -101,7 +101,14 @@ enum {
>  #define ELF_DATA        ELFDATA2LSB
>  #endif
>
> +#ifdef TARGET_ABI_MIPSN32
>  typedef target_ulong    target_elf_greg_t;
> +#define tswapreg(ptr)   tswapl(ptr)
> +#else
> +typedef abi_ulong       target_elf_greg_t;
> +#define tswapreg(ptr)   tswapal(ptr)
> +#endif

This is kind of ugly but it looks like the kernel is kind
of ugly too (ie elf_greg_t as a type is not defined the
same way necessarily for all targets and ABIs). At some
point this type should probably live in a header file
in linux-user/$arch/ but for now I guess it can pass.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

-- PMM
Paolo Bonzini - April 4, 2013, 3:41 p.m.
Il 04/04/2013 17:32, Peter Maydell ha scritto:
> This is kind of ugly but it looks like the kernel is kind
> of ugly too (ie elf_greg_t as a type is not defined the
> same way necessarily for all targets and ABIs). At some
> point this type should probably live in a header file
> in linux-user/$arch/ but for now I guess it can pass.
> 
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

I'll add abi_reg to abitypes.h and move tswapreg there, so that the
signal handling structures could use it too.

Paolo
Peter Maydell - April 4, 2013, 3:46 p.m.
On 4 April 2013 16:41, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 04/04/2013 17:32, Peter Maydell ha scritto:
>> This is kind of ugly but it looks like the kernel is kind
>> of ugly too (ie elf_greg_t as a type is not defined the
>> same way necessarily for all targets and ABIs). At some
>> point this type should probably live in a header file
>> in linux-user/$arch/ but for now I guess it can pass.
>>
>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>
> I'll add abi_reg to abitypes.h and move tswapreg there, so
> that the signal handling structures could use it too.

I started off thinking that was the right thing, but then
I realised that the signal handling structures vary in what
they use (eg the MIPS ones tend to really use u32 and u64),
so they aren't necessarily the same type as elf_greg_t).

I think the conceptual ideal is that we should have
abi types which match the kernel's user-facing types.
This is a bit messed up here because the kernel doesn't
actually expose elf_greg_t in most archs (and because
MIPS has different types for "what the kernel thinks
long is" vs "what userspace thinks long is", which is
just plain confusing when looking at non-user-facing
struct definitions).

-- PMM
Paolo Bonzini - April 4, 2013, 4 p.m.
Il 04/04/2013 17:46, Peter Maydell ha scritto:
> On 4 April 2013 16:41, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Il 04/04/2013 17:32, Peter Maydell ha scritto:
>>> This is kind of ugly but it looks like the kernel is kind
>>> of ugly too (ie elf_greg_t as a type is not defined the
>>> same way necessarily for all targets and ABIs). At some
>>> point this type should probably live in a header file
>>> in linux-user/$arch/ but for now I guess it can pass.
>>>
>>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>>
>> I'll add abi_reg to abitypes.h and move tswapreg there, so
>> that the signal handling structures could use it too.
> 
> I started off thinking that was the right thing, but then
> I realised that the signal handling structures vary in what
> they use (eg the MIPS ones tend to really use u32 and u64),
> so they aren't necessarily the same type as elf_greg_t).

Right, MIPS o32 uses uint64_t.  So I'll keep target_elf_greg_t as in v2.

> I think the conceptual ideal is that we should have
> abi types which match the kernel's user-facing types.
> This is a bit messed up here because the kernel doesn't
> actually expose elf_greg_t in most archs (and because
> MIPS has different types for "what the kernel thinks
> long is" vs "what userspace thinks long is", which is
> just plain confusing when looking at non-user-facing
> struct definitions).

Paolo

Patch

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index d3589ff..9d5dbb8 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -101,7 +101,14 @@  enum {
 #define ELF_DATA        ELFDATA2LSB
 #endif
 
+#ifdef TARGET_ABI_MIPSN32
 typedef target_ulong    target_elf_greg_t;
+#define tswapreg(ptr)   tswapl(ptr)
+#else
+typedef abi_ulong       target_elf_greg_t;
+#define tswapreg(ptr)   tswapal(ptr)
+#endif
+
 #ifdef USE_UID16
 typedef target_ushort   target_uid_t;
 typedef target_ushort   target_gid_t;
@@ -747,17 +754,17 @@  static void elf_core_copy_regs(target_elf_gregset_t *regs, const CPUMIPSState *e
     (*regs)[TARGET_EF_R0] = 0;
 
     for (i = 1; i < ARRAY_SIZE(env->active_tc.gpr); i++) {
-        (*regs)[TARGET_EF_R0 + i] = tswapl(env->active_tc.gpr[i]);
+        (*regs)[TARGET_EF_R0 + i] = tswapreg(env->active_tc.gpr[i]);
     }
 
     (*regs)[TARGET_EF_R26] = 0;
     (*regs)[TARGET_EF_R27] = 0;
-    (*regs)[TARGET_EF_LO] = tswapl(env->active_tc.LO[0]);
-    (*regs)[TARGET_EF_HI] = tswapl(env->active_tc.HI[0]);
-    (*regs)[TARGET_EF_CP0_EPC] = tswapl(env->active_tc.PC);
-    (*regs)[TARGET_EF_CP0_BADVADDR] = tswapl(env->CP0_BadVAddr);
-    (*regs)[TARGET_EF_CP0_STATUS] = tswapl(env->CP0_Status);
-    (*regs)[TARGET_EF_CP0_CAUSE] = tswapl(env->CP0_Cause);
+    (*regs)[TARGET_EF_LO] = tswapreg(env->active_tc.LO[0]);
+    (*regs)[TARGET_EF_HI] = tswapreg(env->active_tc.HI[0]);
+    (*regs)[TARGET_EF_CP0_EPC] = tswapreg(env->active_tc.PC);
+    (*regs)[TARGET_EF_CP0_BADVADDR] = tswapreg(env->CP0_BadVAddr);
+    (*regs)[TARGET_EF_CP0_STATUS] = tswapreg(env->CP0_Status);
+    (*regs)[TARGET_EF_CP0_CAUSE] = tswapreg(env->CP0_Cause);
 }
 
 #define USE_ELF_CORE_DUMP