Request for review/comments: 32-bit compat for non-x86 architectures

Post by Nathan Whitehorn
The patch at http://people.freebsd.org/~nwhitehorn/compat_freebsd32.diff
(pre-generated freebsd32 syscalls stuff is included, which will be done
in two steps on commit) provides groundwork for supporting 32-bit
compatibility for 64-bit MIPS and PowerPC systems. It has been tested on
amd64 and powerpc64, and compile-tested on ia64. There are two main
1) COMPAT_IA32 is renamed COMPAT_FREEBSD32, in analogy to
COMPAT_LINUX32, etc. This requires updating kernel configurations, but
is less painful than filling machine-independent bits of the kernel with
#if defined(COMPAT_IA32) || defined(COMPAT_PPC32) ||
defined(COMPAT_MIPS32) || ..., and is no less descriptive than the old name.
2) Modifications to the freebsd32 compat layer to support big-endian
architectures.
I would appreciate any comments, bugs, or test results on ia64.
-Nathan

This fragment
--- sys/kern/imgact_elf.c (revision 204681)
+++ sys/kern/imgact_elf.c (working copy)
@@ -75,7 +75,7 @@
#include <machine/elf.h>
#include <machine/md_var.h>

-#if defined(COMPAT_IA32) && __ELF_WORD_SIZE == 32
+#if (defined(__amd64__) || defined(__ia64__)) && __ELF_WORD_SIZE == 32
#include <machine/fpu.h>
#include <compat/ia32/ia32_reg.h>
#endif
probably should be changed ? How are struct reg32 for !ia32 case is
brought into the imgact_elf ? Can it be unified for ia32 case ?
(Similar fragment is present in sys_process.c at least).

I do not understand how +#if !defined(PAD64_REQUIRED) && defined(__powerpc__)
etc lines are generated.

You may want to change sysent->sv_flag SV_IA32 to SV_FREEBSD32, or add
SV_FREEBSD32. You might want to review SV_IA32 usage, if any.

Nathan Whitehorn

2010-03-10 14:35:09 UTC

Post by Kostik Belousov

For !ia32, they are brought in machine/reg.h. You can see how that works
here:
http://svn.freebsd.org/viewvc/base/projects/ppc64/sys/powerpc/include/reg.h?revision=204915&view=markup

I would be more than happy for them to be brought in the same way for
amd64 and ia64. Since John Baldwin seems to want this too, I might roll
a new version of the diff today or tomorrow that does that.

Post by Kostik Belousov
I do not understand how +#if !defined(PAD64_REQUIRED) && defined(__powerpc__)
etc lines are generated.

These end up in syscalls.master and take advantage of the syscalls
generator propagating preprocessor statements through. 32-bit powerpc
has an ABI quirk where 64-bit arguments are transmitted in "aligned"
registers, so this adds some padding to those syscalls such that this is
preserved. The !defined bit just protects against redefinition when one
of the .c files generated from syscalls.master includes one of the
header files.

Post by Kostik Belousov
You may want to change sysent->sv_flag SV_IA32 to SV_FREEBSD32, or add
SV_FREEBSD32. You might want to review SV_IA32 usage, if any.

We already have SV_ILP32, which is used pretty consistently for this.
The patch includes a fix for the one erroneous use in
sys/kern/kern_jail.c that I could find with grep -R SV_IA32 /sys.
-Nathan

John Baldwin

2010-03-10 13:10:10 UTC

This doesn't look right for non-x86 32-bit ABIs:

Index: sys/kern/imgact_elf.c
===================================================================
--- sys/kern/imgact_elf.c (revision 204681)
+++ sys/kern/imgact_elf.c (working copy)
@@ -1439,7 +1439,7 @@
ehdr->e_ident[EI_ABIVERSION] = 0;
ehdr->e_ident[EI_PAD] = 0;
ehdr->e_type = ET_CORE;
-#if defined(COMPAT_IA32) && __ELF_WORD_SIZE == 32
+#if defined(COMPAT_FREEBSD32) && __ELF_WORD_SIZE == 32
ehdr->e_machine = EM_386;
#else
ehdr->e_machine = ELF_ARCH;

It would be nice to eliminate having <compat/ia32*> includes in MI code by
instead including those headers in appropriate headers in <machine/*>. For
example, we could change <machine/reg.h> on amd64 and ia64 to include these
headers, perhaps under an #ifdef COMPAT_FREEBSD32.

Hmm, actually, I'm quite convinced now that <machine/reg.h> for ia64 and amd64
should include <compat/ia32/ia32_reg.h> in the #ifdef _KERNEL section to avoid
polluting those includes in MI code. I'm not sure what the various
<machine/fpu.h> includes are for, but fixing ia32_reg.h would be a good first
step. It would make your diffs smaller I think.

The rest of the diff looks fine to me.

--
John Baldwin

Nathan Whitehorn

2010-03-10 14:39:15 UTC

Index: sys/kern/imgact_elf.c
===================================================================
--- sys/kern/imgact_elf.c (revision 204681)
+++ sys/kern/imgact_elf.c (working copy)
@@ -1439,7 +1439,7 @@
ehdr->e_ident[EI_ABIVERSION] = 0;
ehdr->e_ident[EI_PAD] = 0;
ehdr->e_type = ET_CORE;
-#if defined(COMPAT_IA32) && __ELF_WORD_SIZE == 32
+#if defined(COMPAT_FREEBSD32) && __ELF_WORD_SIZE == 32
ehdr->e_machine = EM_386;
#else
ehdr->e_machine = ELF_ARCH;

Good catch! Such are the dangers of sed. How about defining an
ELF_ARCH32 in machine/elf.h for this case?

Post by John Baldwin
It would be nice to eliminate having <compat/ia32*> includes in MI code by
instead including those headers in appropriate headers in <machine/*>. For
example, we could change <machine/reg.h> on amd64 and ia64 to include these
headers, perhaps under an #ifdef COMPAT_FREEBSD32.
Hmm, actually, I'm quite convinced now that <machine/reg.h> for ia64 and amd64
should include <compat/ia32/ia32_reg.h> in the #ifdef _KERNEL section to avoid
polluting those includes in MI code. I'm not sure what the various
<machine/fpu.h> includes are for, but fixing ia32_reg.h would be a good first
step. It would make your diffs smaller I think.

This is how it works on powerpc64. I didn't modify amd64 and ia64 in
order to avoid making too many changes, but I think you're right that
this is a good idea. I'll add that to the patch when fixing the EM_386
bit you pointed out above.

Post by John Baldwin
The rest of the diff looks fine to me.

Thanks for the comments!
-Nathan

John Baldwin

2010-03-10 15:43:23 UTC

Good catch! Such are the dangers of sed. How about defining an
ELF_ARCH32 in machine/elf.h for this case?

Yes, that sounds good.

Ok, thanks.

--
John Baldwin

Nathan Whitehorn

2010-03-10 15:55:40 UTC

Good catch! Such are the dangers of sed. How about defining an
ELF_ARCH32 in machine/elf.h for this case?

Yes, that sounds good.

Ok, thanks.

I've updated the patch to incorporate these two changes, at
http://people.freebsd.org/~nwhitehorn/compat_freebsd32_2.diff. Due to
recursive inclusion issues with sys/procfs.h, it also moves prstatus32
and friends to compat/freebsd32/freebsd32.h from ia32_reg.h. They are
MI, and seems like a more appropriate place for them anyway.
-Nathan

John Baldwin

2010-03-10 17:59:07 UTC

Good catch! Such are the dangers of sed. How about defining an
ELF_ARCH32 in machine/elf.h for this case?

Yes, that sounds good.

Ok, thanks.

Looks good to me.

--
John Baldwin

Kostik Belousov

2010-03-10 20:24:58 UTC

Post by Nathan Whitehorn
The patch at
http://people.freebsd.org/~nwhitehorn/compat_freebsd32.diff
(pre-generated freebsd32 syscalls stuff is included, which will be done
in two steps on commit) provides groundwork for supporting 32-bit
compatibility for 64-bit MIPS and PowerPC systems. It has been tested
on amd64 and powerpc64, and compile-tested on ia64. There are two main
1) COMPAT_IA32 is renamed COMPAT_FREEBSD32, in analogy to
COMPAT_LINUX32, etc. This requires updating kernel configurations, but
is less painful than filling machine-independent bits of the kernel
with #if defined(COMPAT_IA32) || defined(COMPAT_PPC32) ||
defined(COMPAT_MIPS32) || ..., and is no less descriptive than the old name.
2) Modifications to the freebsd32 compat layer to support big-endian
architectures.
I would appreciate any comments, bugs, or test results on ia64.

Good catch! Such are the dangers of sed. How about defining an
ELF_ARCH32 in machine/elf.h for this case?

Yes, that sounds good.

Post by John Baldwin
It would be nice to eliminate having <compat/ia32*> includes in MI code
by instead including those headers in appropriate headers in
<machine/*>. For example, we could change <machine/reg.h> on amd64 and
ia64 to include these headers, perhaps under an #ifdef COMPAT_FREEBSD32.
Hmm, actually, I'm quite convinced now that <machine/reg.h> for ia64 and
amd64 should include <compat/ia32/ia32_reg.h> in the #ifdef _KERNEL
section to avoid polluting those includes in MI code. I'm not sure what
the various <machine/fpu.h> includes are for, but fixing ia32_reg.h
would be a good first step. It would make your diffs smaller I think.

Ok, thanks.

First chunk for the sys_generic.c about ibits/obits looks like a bug fix ?
If yes, it probably would make sense to commit it separately to be able
to MFC it.

The same note about chunks that remove #include <compat/ia32...>, if
possible ?

Nathan Whitehorn

2010-03-10 20:29:47 UTC

Post by Kostik Belousov

Post by Nathan Whitehorn
The patch at
http://people.freebsd.org/~nwhitehorn/compat_freebsd32.diff
(pre-generated freebsd32 syscalls stuff is included, which will be done
in two steps on commit) provides groundwork for supporting 32-bit
compatibility for 64-bit MIPS and PowerPC systems. It has been tested
on amd64 and powerpc64, and compile-tested on ia64. There are two main
1) COMPAT_IA32 is renamed COMPAT_FREEBSD32, in analogy to
COMPAT_LINUX32, etc. This requires updating kernel configurations, but
is less painful than filling machine-independent bits of the kernel
with #if defined(COMPAT_IA32) || defined(COMPAT_PPC32) ||
defined(COMPAT_MIPS32) || ..., and is no less descriptive than the old name.
2) Modifications to the freebsd32 compat layer to support big-endian
architectures.
I would appreciate any comments, bugs, or test results on ia64.

Good catch! Such are the dangers of sed. How about defining an
ELF_ARCH32 in machine/elf.h for this case?

Yes, that sounds good.