build fix for x86_64...

build fix for x86_64...

[Arthur Jones]

[-- Attachment #1: Type: text/plain, Size: 363 bytes --]

hi vasily,

commit b716395e2b8e450e294537de0c91476ded2f0395
breaks the build for x86_64 (and ia64 too, i guess) if
CONFIG_COMPAT is not turned on.  here is a patch, which
i think is more complete than the previous one posted
to LKML by Doug Chapman (which only fixes ia64, i think).
it lets the build continue, but i have no idea if it's
correct or not..

arthur

[-- Attachment #2: t.patch --]
[-- Type: text/plain, Size: 483 bytes --]

diff --git a/fs/quota.c b/fs/quota.c
index e6577ac..5baeede 100644
--- a/fs/quota.c
+++ b/fs/quota.c
@@ -387,7 +387,7 @@ asmlinkage long sys_quotactl(unsigned int cmd, const char __user *special, qid_t
 	return ret;
 }
 
-#if defined(CONFIG_X86_64) || defined(CONFIG_IA64)
+#if defined(CONFIG_COMPAT) && (defined(CONFIG_X86_64) || defined(CONFIG_IA64))
 /*
  * This code works only for 32 bit quota tools over 64 bit OS (x86_64, ia64)
  * and is necessary due to alignment problems.

Re: build fix for x86_64...

[Andi Kleen]

On Friday 20 July 2007 00:24:11 Arthur Jones wrote:
> hi vasily,
> 
> commit b716395e2b8e450e294537de0c91476ded2f0395
> breaks the build for x86_64 (and ia64 too, i guess) if
> CONFIG_COMPAT is not turned on.  here is a patch, which
> i think is more complete than the previous one posted
> to LKML by Doug Chapman (which only fixes ia64, i think).
> it lets the build continue, but i have no idea if it's
> correct or not..

Looks good,

Tony, perhaps it would make sense to define some common CONFIG
for COMPAT_FOR_U64_ALIGNMENT longer term to make the #ifs for this
case a little less ugly.

-Andi

RE: build fix for x86_64...

[Luck, Tony]

> Looks good.

Looks good to me too (I'm to blame for the version posted by Doug
that only fixed ia64 ... I didn't take the time to check whether
there was a CONFIG_COMPAT option for x86_64 ... oops).

> Tony, perhaps it would make sense to define some common CONFIG
> for COMPAT_FOR_U64_ALIGNMENT longer term to make the #ifs for this
> case a little less ugly.

Certainly if there is ever a third architecture that needs this the
#if mess will be beyond bad.  But with only the two of us we'd trade
one ugly #ifdef line here for another half-dozen lines in each of
our Kconfig files to define the new term.  I'm not sure whether that
is a win.

-Tony

Re: build fix for x86_64...

[Andi Kleen]

On Friday 20 July 2007 01:54:51 Luck, Tony wrote:
 
> > Tony, perhaps it would make sense to define some common CONFIG
> > for COMPAT_FOR_U64_ALIGNMENT longer term to make the #ifs for this
> > case a little less ugly.
> 
> Certainly if there is ever a third architecture that needs this the
> #if mess will be beyond bad.  But with only the two of us we'd trade
> one ugly #ifdef line here for another half-dozen lines in each of
> our Kconfig files to define the new term. 

Half dozens? 

config COMPAT_FOR_U64_ALIGNMENT
	bool
	default y

-Andi

Re: build fix for x86_64...

[Randy Dunlap]

On Fri, 20 Jul 2007 10:32:47 +0200 Andi Kleen wrote:

> On Friday 20 July 2007 01:54:51 Luck, Tony wrote:
>  
> > > Tony, perhaps it would make sense to define some common CONFIG
> > > for COMPAT_FOR_U64_ALIGNMENT longer term to make the #ifs for this
> > > case a little less ugly.
> > 
> > Certainly if there is ever a third architecture that needs this the
> > #if mess will be beyond bad.  But with only the two of us we'd trade
> > one ugly #ifdef line here for another half-dozen lines in each of
> > our Kconfig files to define the new term. 
> 
> Half dozens? 
> 
> config COMPAT_FOR_U64_ALIGNMENT
> 	bool
> 	default y

or even
	def_bool y


---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

RE: build fix for x86_64...

[Luck, Tony]

> Half dozens?

I was imagining having to have "help" messages ... which of course we don't.

> config COMPAT_FOR_U64_ALIGNMENT
> 	bool
> 	default y

Plausible.

Randy suggested:
>	def_bool y

Which is better.  But if we unconditionally set this CONFIG variable, then the
code in fs/quota.c will have to read:

#if defined(CONFIG_COMPAT) && defined(CONFIG_COMPAT_FOR_U64_ALIGNMENT)

We can keep it simpler if the Kconfig file does the conditional for us:

config COMPAT_FOR_U64_ALIGNMENT
	def_bool y
	depends on COMPAT

Then the patch looks like this:

diff --git a/arch/ia64/Kconfig b/arch/ia64/Kconfig
index 36c7b96..36c1582 100644
--- a/arch/ia64/Kconfig
+++ b/arch/ia64/Kconfig
@@ -425,6 +425,10 @@ config COMPAT
 	depends on IA32_SUPPORT
 	default y
 
+config COMPAT_FOR_U64_ALIGNMENT
+	def_bool y
+	depends on COMPAT
+
 config IA64_MCA_RECOVERY
 	tristate "MCA recovery from errors other than TLB."
 
diff --git a/arch/x86_64/Kconfig b/arch/x86_64/Kconfig
index 14bf8ce..c77510b 100644
--- a/arch/x86_64/Kconfig
+++ b/arch/x86_64/Kconfig
@@ -753,6 +753,10 @@ config COMPAT
 	depends on IA32_EMULATION
 	default y
 
+config COMPAT_FOR_U64_ALIGNMENT
+	def_bool y
+	depends on COMPAT
+
 config SYSVIPC_COMPAT
 	bool
 	depends on COMPAT && SYSVIPC
diff --git a/fs/quota.c b/fs/quota.c
index e6577ac..99b24b5 100644
--- a/fs/quota.c
+++ b/fs/quota.c
@@ -387,7 +387,7 @@ asmlinkage long sys_quotactl(unsigned int cmd, const char __user *special, qid_t
 	return ret;
 }
 
-#if defined(CONFIG_X86_64) || defined(CONFIG_IA64)
+#if defined(CONFIG_COMPAT_FOR_U64_ALIGNMENT)
 /*
  * This code works only for 32 bit quota tools over 64 bit OS (x86_64, ia64)
  * and is necessary due to alignment problems.

Re: build fix for x86_64...

[Matthew Wilcox]

On Fri, Jul 20, 2007 at 09:20:15AM -0700, Luck, Tony wrote:
> Which is better.  But if we unconditionally set this CONFIG variable, then the
> code in fs/quota.c will have to read:
> 
> #if defined(CONFIG_COMPAT) && defined(CONFIG_COMPAT_FOR_U64_ALIGNMENT)
> 
> We can keep it simpler if the Kconfig file does the conditional for us:
> 
> config COMPAT_FOR_U64_ALIGNMENT
> 	def_bool y
> 	depends on COMPAT

Ah, but you can do better:

config COMPAT_FOR_U64_ALIGNMENT
	def_bool COMPAT

I hand-edited your patch:

diff --git a/arch/ia64/Kconfig b/arch/ia64/Kconfig
index 36c7b96..36c1582 100644
--- a/arch/ia64/Kconfig
+++ b/arch/ia64/Kconfig
@@ -425,6 +425,9 @@ config COMPAT
 	depends on IA32_SUPPORT
 	default y
 
+config COMPAT_FOR_U64_ALIGNMENT
+	def_bool COMPAT
+
 config IA64_MCA_RECOVERY
 	tristate "MCA recovery from errors other than TLB."
 
diff --git a/arch/x86_64/Kconfig b/arch/x86_64/Kconfig
index 14bf8ce..c77510b 100644
--- a/arch/x86_64/Kconfig
+++ b/arch/x86_64/Kconfig
@@ -753,6 +753,9 @@ config COMPAT
 	depends on IA32_EMULATION
 	default y
 
+config COMPAT_FOR_U64_ALIGNMENT
+	def_bool COMPAT
+
 config SYSVIPC_COMPAT
 	bool
 	depends on COMPAT && SYSVIPC
diff --git a/fs/quota.c b/fs/quota.c
index e6577ac..99b24b5 100644
--- a/fs/quota.c
+++ b/fs/quota.c
@@ -387,7 +387,7 @@ asmlinkage long sys_quotactl(unsigned int cmd, const char __user *special, qid_t
 	return ret;
 }
 
-#if defined(CONFIG_X86_64) || defined(CONFIG_IA64)
+#if defined(CONFIG_COMPAT_FOR_U64_ALIGNMENT)
 /*
  * This code works only for 32 bit quota tools over 64 bit OS (x86_64, ia64)
  * and is necessary due to alignment problems.

-- 
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: build fix for x86_64...

[H. Peter Anvin]

Luck, Tony wrote:
> 
> Which is better.  But if we unconditionally set this CONFIG variable, then the
> code in fs/quota.c will have to read:
> 
> #if defined(CONFIG_COMPAT) && defined(CONFIG_COMPAT_FOR_U64_ALIGNMENT)
> 
> We can keep it simpler if the Kconfig file does the conditional for us:
> 
> config COMPAT_FOR_U64_ALIGNMENT
> 	def_bool y
> 	depends on COMPAT
> 

No, that would be bad.  If compat_u64 is used to carry 32-bit ABIs
forward into 64-bit space without needing compatibility hacks, then this
would actually introduce ABI incompatibilities depending on CONFIG_COMPAT!

	-hpa

Re: build fix for x86_64...

[Andi Kleen]

On Friday 20 July 2007 19:03, H. Peter Anvin wrote:
> Luck, Tony wrote:
> > Which is better.  But if we unconditionally set this CONFIG variable,
> > then the code in fs/quota.c will have to read:
> >
> > #if defined(CONFIG_COMPAT) && defined(CONFIG_COMPAT_FOR_U64_ALIGNMENT)
> >
> > We can keep it simpler if the Kconfig file does the conditional for us:
> >
> > config COMPAT_FOR_U64_ALIGNMENT
> > 	def_bool y
> > 	depends on COMPAT
>
> No, that would be bad.  If compat_u64 is used to carry 32-bit ABIs

That doesn't help for any old interfaces, like the one here. For those
still ifdefs are needed. Interfaces that use compat_u64 just use
a normal #ifdef CONFIG_COMPAT.

Besides I have my doubts compat_u64 will be the solution
to these worries. We have hundreds of people adding various interfaces
to Linux and it's unlikely they all heard about it. So likely
these cases will occur again and again.

-Andi

Re: build fix for x86_64...

[H. Peter Anvin]

Andi Kleen wrote:
> 
> That doesn't help for any old interfaces, like the one here. For those
> still ifdefs are needed. Interfaces that use compat_u64 just use
> a normal #ifdef CONFIG_COMPAT.
> 
> Besides I have my doubts compat_u64 will be the solution
> to these worries. We have hundreds of people adding various interfaces
> to Linux and it's unlikely they all heard about it. So likely
> these cases will occur again and again.
> 

Of course they will.  compat_u64 will have no effect on a properly
designed interface where everything is aligned.

To me, the whole point with compat_u64 is that when someone has designed
an interface improperly (so it's alignment-dependent) then we can
replace u64 with compat_u64 and use the same structure on 64 bits even
though the original interface was broken.

	-hpa

Re: build fix for x86_64...

[Andreas Schwab]

"H. Peter Anvin" <hpa@zytor.com> writes:

> No, that would be bad.  If compat_u64 is used to carry 32-bit ABIs
> forward into 64-bit space without needing compatibility hacks, then this
> would actually introduce ABI incompatibilities depending on CONFIG_COMPAT!

But without CONFIG_COMPAT there is no 32-bit ABI, thus no need for
compat_u64 in the first place.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: build fix for x86_64...

[H. Peter Anvin]

Andreas Schwab wrote:
> "H. Peter Anvin" <hpa@zytor.com> writes:
> 
>> No, that would be bad.  If compat_u64 is used to carry 32-bit ABIs
>> forward into 64-bit space without needing compatibility hacks, then this
>> would actually introduce ABI incompatibilities depending on CONFIG_COMPAT!
> 
> But without CONFIG_COMPAT there is no 32-bit ABI, thus no need for
> compat_u64 in the first place.
> 

You're missing the point.

Someone introduces an interface, which uses a structure:

struct foo {
	u32 bar;
	u64 baz;
	u32 quux;
};

Now, we want to port that to 64 bits.  We can either introduce a
thunking function to mangle the argument, or we can redefine the structure:

struct foo {
	u32 bar;
	compat_u64 baz;
	u32 quux;
};

... which is still ABI compatible on 32 bits, but doesn't require thunking.

Obviously, this is not a panacea; if the original "struct foo" has also
been introduced on 64 bits before the bug is caught, then you're screwed.

	-hpa

	

Re: build fix for x86_64...

[Andi Kleen]

> Obviously, this is not a panacea; if the original "struct foo" has also
> been introduced on 64 bits before the bug is caught, 

That's usually the case. There is already an established 64bit ABI

-Andi

Re: build fix for x86_64...

[H. Peter Anvin]

Andi Kleen wrote:
>> Obviously, this is not a panacea; if the original "struct foo" has also
>> been introduced on 64 bits before the bug is caught, 
> 
> That's usually the case. There is already an established 64bit ABI
> 

This is true.

Kind of makes me wonder if the right thing is to introduce an
"interface" 64-bit type, call it __iu64 (__is64) defined as:

typedef __u64 __attribute__((aligned(8))) __iu64;
typedef __s64 __attribute__((aligned(8))) __is64;

... and encourage people to use it when creating new interfaces.

Don't know if that would do any good.

	-hpa

RE: build fix for x86_64...

[Luck, Tony]

At the moment our problem is that there is some code that has
been added to handle the compatability problem caused by u64
objects having different alignment when running on 32-bit and
64-bit systems.  This only affects ia64 and x86-64 because all
the other 32/64 bit capable systems wisely avoided this issue
by making 64-bit objects *always* 8-byte aligned.

It is possible that in the future more such issues will arise
(either because we find some more existing interfaces that
have this problem, or because new interfaces are introduced
that also have this problem).  Such new code will also require
some compatability functions.  These functions will also only
be needed on ia64 and x86-64, and even on these systems the
code will only be needed if CONFIG_COMPAT=y

So I'm failing to see what's wrong with putting such compatabilty
code inside a

#if defined(CONFIG_COMPAT_FOR_U64_ALIGNMENT)
...
#endif

to prevent it wasting object code space in kernels that don't
need it.

-Tony

Re: build fix for x86_64...

[H. Peter Anvin]

Luck, Tony wrote:
> At the moment our problem is that there is some code that has
> been added to handle the compatability problem caused by u64
> objects having different alignment when running on 32-bit and
> 64-bit systems.  This only affects ia64 and x86-64 because all
> the other 32/64 bit capable systems wisely avoided this issue
> by making 64-bit objects *always* 8-byte aligned.

> It is possible that in the future more such issues will arise
> (either because we find some more existing interfaces that
> have this problem, or because new interfaces are introduced
> that also have this problem).  Such new code will also require
> some compatability functions.  These functions will also only
> be needed on ia64 and x86-64, and even on these systems the
> code will only be needed if CONFIG_COMPAT=y

The issue here is I was looking at it from a new interfaces perspective,
and not from a legacy interfaces perspective.  However, for new
interfaces we want the opposite -- properly aligned elements -- so
please disregard previous objection.

However, I'm still thinking it might be worthwhile to consider the
__i[us]64 typedefs previously discussed as a way to avoid alignment
bloopers in new interfaces.

	-hpa

Re: build fix for x86_64...

[Andreas Schwab]

"H. Peter Anvin" <hpa@zytor.com> writes:

> struct foo {
> 	u32 bar;
> 	compat_u64 baz;
> 	u32 quux;
> };

compat_u64 is only for use in CONFIG_COMPAT code.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."