[PATCH v2 45/95] linux-user: Remove target_elf_greg_t

Richard Henderson posted 95 patches 3 months, 2 weeks ago
Maintainers: Riku Voipio <riku.voipio@iki.fi>, Laurent Vivier <laurent@vivier.eu>, Brian Cain <brian.cain@oss.qualcomm.com>, Paolo Bonzini <pbonzini@redhat.com>, "Marc-André Lureau" <marcandre.lureau@redhat.com>, "Daniel P. Berrangé" <berrange@redhat.com>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, "Alex Bennée" <alex.bennee@linaro.org>
There is a newer version of this series
[PATCH v2 45/95] linux-user: Remove target_elf_greg_t
Posted by Richard Henderson 3 months, 2 weeks ago
This typedef is synonymous with target_ulong.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/elfload.c | 29 +++++++++++++----------------
 1 file changed, 13 insertions(+), 16 deletions(-)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index fce4c05674..70a1e402d3 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -131,10 +131,8 @@ int info_is_fdpic(struct image_info *info)
 #endif
 
 #ifdef TARGET_ABI_MIPSN32
-typedef abi_ullong      target_elf_greg_t;
 #define tswapreg(ptr)   tswap64(ptr)
 #else
-typedef abi_ulong       target_elf_greg_t;
 #define tswapreg(ptr)   tswapal(ptr)
 #endif
 
@@ -154,7 +152,7 @@ typedef abi_int         target_pid_t;
 #define ELF_ARCH       EM_X86_64
 
 #define ELF_NREG    27
-typedef target_elf_greg_t  target_elf_gregset_t[ELF_NREG];
+typedef target_ulong target_elf_gregset_t[ELF_NREG];
 
 /*
  * Note that ELF_NREG should be 29 as there should be place for
@@ -231,7 +229,7 @@ static bool init_guest_commpage(void)
 #define EXSTACK_DEFAULT true
 
 #define ELF_NREG    17
-typedef target_elf_greg_t  target_elf_gregset_t[ELF_NREG];
+typedef target_ulong target_elf_gregset_t[ELF_NREG];
 
 /*
  * Note that ELF_NREG should be 19 as there should be place for
@@ -292,7 +290,7 @@ static void elf_core_copy_regs(target_elf_gregset_t *regs, const CPUX86State *en
 #define EXSTACK_DEFAULT true
 
 #define ELF_NREG    18
-typedef target_elf_greg_t  target_elf_gregset_t[ELF_NREG];
+typedef target_ulong target_elf_gregset_t[ELF_NREG];
 
 static void elf_core_copy_regs(target_elf_gregset_t *regs, const CPUARMState *env)
 {
@@ -392,7 +390,7 @@ static const VdsoImageInfo *vdso_image_info(uint32_t elf_flags)
 #define ELF_CLASS       ELFCLASS64
 
 #define ELF_NREG    34
-typedef target_elf_greg_t  target_elf_gregset_t[ELF_NREG];
+typedef target_ulong target_elf_gregset_t[ELF_NREG];
 
 static void elf_core_copy_regs(target_elf_gregset_t *regs,
                                const CPUARMState *env)
@@ -479,7 +477,7 @@ static void elf_core_copy_regs(target_elf_gregset_t *regs,
 
 /* See linux kernel: arch/powerpc/include/asm/elf.h.  */
 #define ELF_NREG 48
-typedef target_elf_greg_t target_elf_gregset_t[ELF_NREG];
+typedef target_ulong target_elf_gregset_t[ELF_NREG];
 
 static void elf_core_copy_regs(target_elf_gregset_t *regs, const CPUPPCState *env)
 {
@@ -525,7 +523,7 @@ static void elf_core_copy_regs(target_elf_gregset_t *regs, const CPUPPCState *en
 
 /* See linux kernel: arch/loongarch/include/asm/elf.h */
 #define ELF_NREG 45
-typedef target_elf_greg_t target_elf_gregset_t[ELF_NREG];
+typedef target_ulong target_elf_gregset_t[ELF_NREG];
 
 enum {
     TARGET_EF_R0 = 0,
@@ -571,7 +569,7 @@ static void elf_core_copy_regs(target_elf_gregset_t *regs,
 
 /* See linux kernel: arch/mips/include/asm/elf.h.  */
 #define ELF_NREG 45
-typedef target_elf_greg_t target_elf_gregset_t[ELF_NREG];
+typedef target_ulong target_elf_gregset_t[ELF_NREG];
 
 /* See linux kernel: arch/mips/include/asm/reg.h.  */
 enum {
@@ -630,7 +628,7 @@ static void elf_core_copy_regs(target_elf_gregset_t *regs, const CPUMIPSState *e
 
 #define USE_ELF_CORE_DUMP
 #define ELF_NREG 38
-typedef target_elf_greg_t target_elf_gregset_t[ELF_NREG];
+typedef target_ulong target_elf_gregset_t[ELF_NREG];
 
 /* See linux kernel: arch/mips/kernel/process.c:elf_dump_regs.  */
 static void elf_core_copy_regs(target_elf_gregset_t *regs, const CPUMBState *env)
@@ -662,7 +660,7 @@ static void elf_core_copy_regs(target_elf_gregset_t *regs, const CPUMBState *env
 
 /* See linux kernel arch/openrisc/include/asm/elf.h.  */
 #define ELF_NREG 34 /* gprs and pc, sr */
-typedef target_elf_greg_t target_elf_gregset_t[ELF_NREG];
+typedef target_ulong target_elf_gregset_t[ELF_NREG];
 
 static void elf_core_copy_regs(target_elf_gregset_t *regs,
                                const CPUOpenRISCState *env)
@@ -685,7 +683,7 @@ static void elf_core_copy_regs(target_elf_gregset_t *regs,
 
 /* See linux kernel: arch/sh/include/asm/elf.h.  */
 #define ELF_NREG 23
-typedef target_elf_greg_t target_elf_gregset_t[ELF_NREG];
+typedef target_ulong target_elf_gregset_t[ELF_NREG];
 
 /* See linux kernel: arch/sh/include/asm/ptrace.h.  */
 enum {
@@ -728,7 +726,7 @@ static inline void elf_core_copy_regs(target_elf_gregset_t *regs,
 
 /* See linux kernel: arch/m68k/include/asm/elf.h.  */
 #define ELF_NREG 20
-typedef target_elf_greg_t target_elf_gregset_t[ELF_NREG];
+typedef target_ulong target_elf_gregset_t[ELF_NREG];
 
 static void elf_core_copy_regs(target_elf_gregset_t *regs, const CPUM68KState *env)
 {
@@ -776,7 +774,7 @@ static void elf_core_copy_regs(target_elf_gregset_t *regs, const CPUM68KState *e
 
 /* See linux kernel: arch/s390/include/uapi/asm/ptrace.h (s390_regs).  */
 #define ELF_NREG 27
-typedef target_elf_greg_t target_elf_gregset_t[ELF_NREG];
+typedef target_ulong target_elf_gregset_t[ELF_NREG];
 
 enum {
     TARGET_REG_PSWM = 0,
@@ -877,7 +875,7 @@ static bool init_guest_commpage(void)
 
 /* See linux kernel: arch/xtensa/include/asm/elf.h.  */
 #define ELF_NREG 128
-typedef target_elf_greg_t target_elf_gregset_t[ELF_NREG];
+typedef target_ulong target_elf_gregset_t[ELF_NREG];
 
 enum {
     TARGET_REG_PC,
@@ -2864,7 +2862,6 @@ int load_elf_binary(struct linux_binprm *bprm, struct image_info *info)
  * Next you define type of register set used for dumping.  ELF specification
  * says that it needs to be array of elf_greg_t that has size of ELF_NREG.
  *
- * typedef <target_regtype> target_elf_greg_t;
  * #define ELF_NREG <number of registers>
  * typedef taret_elf_greg_t target_elf_gregset_t[ELF_NREG];
  *
-- 
2.43.0
Re: [PATCH v2 45/95] linux-user: Remove target_elf_greg_t
Posted by Peter Maydell 3 months, 1 week ago
On Sun, 3 Aug 2025 at 00:20, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> This typedef is synonymous with target_ulong.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  linux-user/elfload.c | 29 +++++++++++++----------------
>  1 file changed, 13 insertions(+), 16 deletions(-)
>
> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index fce4c05674..70a1e402d3 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c
> @@ -131,10 +131,8 @@ int info_is_fdpic(struct image_info *info)
>  #endif
>
>  #ifdef TARGET_ABI_MIPSN32
> -typedef abi_ullong      target_elf_greg_t;
>  #define tswapreg(ptr)   tswap64(ptr)
>  #else
> -typedef abi_ulong       target_elf_greg_t;
>  #define tswapreg(ptr)   tswapal(ptr)
>  #endif

Previously we had target_elf_greg_t:
 * for MIPSN32: abi_ullong, which is 64 bits
 * for other TARGET_ABI32: abi_ulong, which is 32 bits
 * for 64-bit archs: abi_ulong, which is 64 bits
 * for 32-bit archs: abi_ulong, which is 32 bits

Now we have target_ulong, which is:
 * for 64-bit archs: 64 bits
 * for 32-bit archs: 32 bits

So the two TARGET_ABI32 which weren't special cased
(hppa and sparc32plus) will go from a 32-bit type to a 64-bit
type, won't they ?

It wouldn't surprise me if this is a bug in the hppa and
sparc32plus cases, but if so we should say in the commit
message that we're fixing it.

thanks
-- PMM
Re: [PATCH v2 45/95] linux-user: Remove target_elf_greg_t
Posted by Richard Henderson 3 months, 1 week ago
On 8/3/25 20:59, Peter Maydell wrote:
> On Sun, 3 Aug 2025 at 00:20, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> This typedef is synonymous with target_ulong.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>   linux-user/elfload.c | 29 +++++++++++++----------------
>>   1 file changed, 13 insertions(+), 16 deletions(-)
>>
>> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
>> index fce4c05674..70a1e402d3 100644
>> --- a/linux-user/elfload.c
>> +++ b/linux-user/elfload.c
>> @@ -131,10 +131,8 @@ int info_is_fdpic(struct image_info *info)
>>   #endif
>>
>>   #ifdef TARGET_ABI_MIPSN32
>> -typedef abi_ullong      target_elf_greg_t;
>>   #define tswapreg(ptr)   tswap64(ptr)
>>   #else
>> -typedef abi_ulong       target_elf_greg_t;
>>   #define tswapreg(ptr)   tswapal(ptr)
>>   #endif
> 
> Previously we had target_elf_greg_t:
>   * for MIPSN32: abi_ullong, which is 64 bits

MIPSN32 is a mips64 target.

>   * for other TARGET_ABI32: abi_ulong, which is 32 bits
>   * for 64-bit archs: abi_ulong, which is 64 bits
>   * for 32-bit archs: abi_ulong, which is 32 bits
> 
> Now we have target_ulong, which is:
>   * for 64-bit archs: 64 bits
>   * for 32-bit archs: 32 bits
> 
> So the two TARGET_ABI32 which weren't special cased
> (hppa and sparc32plus) will go from a 32-bit type to a 64-bit
> type, won't they ?
> 
> It wouldn't surprise me if this is a bug in the hppa and
> sparc32plus cases, but if so we should say in the commit
> message that we're fixing it.

Neither sparc nor hppa implement core dumps so far,
so we can put off considering them.

Perhaps the commit message can simply be expanded to

   For the set of targets which support core dumps,
   target_elf_greg_t is synonymous with target_ulong.

?

r~
Re: [PATCH v2 45/95] linux-user: Remove target_elf_greg_t
Posted by Peter Maydell 3 months, 1 week ago
On Sun, 3 Aug 2025 at 21:11, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 8/3/25 20:59, Peter Maydell wrote:
> > On Sun, 3 Aug 2025 at 00:20, Richard Henderson
> > <richard.henderson@linaro.org> wrote:
> >>
> >> This typedef is synonymous with target_ulong.
> >>
> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> >> ---
> >>   linux-user/elfload.c | 29 +++++++++++++----------------
> >>   1 file changed, 13 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> >> index fce4c05674..70a1e402d3 100644
> >> --- a/linux-user/elfload.c
> >> +++ b/linux-user/elfload.c
> >> @@ -131,10 +131,8 @@ int info_is_fdpic(struct image_info *info)
> >>   #endif
> >>
> >>   #ifdef TARGET_ABI_MIPSN32
> >> -typedef abi_ullong      target_elf_greg_t;
> >>   #define tswapreg(ptr)   tswap64(ptr)
> >>   #else
> >> -typedef abi_ulong       target_elf_greg_t;
> >>   #define tswapreg(ptr)   tswapal(ptr)
> >>   #endif
> >
> > Previously we had target_elf_greg_t:
> >   * for MIPSN32: abi_ullong, which is 64 bits
>
> MIPSN32 is a mips64 target.
>
> >   * for other TARGET_ABI32: abi_ulong, which is 32 bits
> >   * for 64-bit archs: abi_ulong, which is 64 bits
> >   * for 32-bit archs: abi_ulong, which is 32 bits
> >
> > Now we have target_ulong, which is:
> >   * for 64-bit archs: 64 bits
> >   * for 32-bit archs: 32 bits
> >
> > So the two TARGET_ABI32 which weren't special cased
> > (hppa and sparc32plus) will go from a 32-bit type to a 64-bit
> > type, won't they ?
> >
> > It wouldn't surprise me if this is a bug in the hppa and
> > sparc32plus cases, but if so we should say in the commit
> > message that we're fixing it.
>
> Neither sparc nor hppa implement core dumps so far,
> so we can put off considering them.

I guess so, but if we have to undo the refactoring because
it doesn't fit those architectures that would be annoying.

I had a look at the kernel sources and I think that here
mips N32 really is an outlier --
arch/mips/include/asm/elfcore-compat.h defines the
compat_elf_gregset_t to be the same as the (64-bit)
elf_gregset_t, and has some macro magic to handle the
O32 case which does have 32-bit registers.
On the other hand arch/parisc/include/asm/compat.h and
arch/sparc/include/asm/elf_64.h both define the
compat_elf_gregset_t type as 32-bit.

So I think it would be good to have at least a sketch of
how hppa and sparc32plus would work in the new setup and
why we wouldn't need to reintroduce the target_elf_greg_t
type, before we rip it out.

thanks
-- PMM