On Thu, 1 Feb 2007, Franck Bui-Huu wrote:
> > Checking for code correctness and validation of the toolchain (Linux is
> > one of the few non-PIC users of (n)64) without having to chase hardware
> > that would support running from XPHYS without serious pain (the firmware
> > being the usual offender).
>
> This use case was unknown by the time we introduced __pa_page_offset().
Well, I am afraid it was known well before. I introduced it first to 2.4
a while ago and I forward-ported the patch immediately to 2.6. Both
changes went in on Oct 20th, 2004. The help text for the option has not
changed since. And even 2.6.18 that I'm still using does not have this
__pa_page_offset() macro! I did build various kernel versions with
BUILD_ELF64 set for the DECstation (which links at 0xffffffff80040000).
> Basically this macro assumes that if BUILD_ELF64 is set the load
> address is in XKPHYS. This allows to simplify __pa_page_offset()
> definition for this case.
Wrong assumption. And nowhere guaranteed either.
> However if BUILD_ELF64 is not set then the macro deals with both
> CKSEG0 and XKPHYS virtual addresses.
Indeed.
> > That said, I have not checked the every single use of __pa_page_offset(),
> > but the sole existence of this condition raises a question about whether
> > we are sure __pa_page_offset() is going to be only used on virtual
> > addresses in the same segment the kernel is linked to.
>
> Well it all depends if we consider the case with BUILD_ELF64 set and a
> load address in CKSEG0 a useful case. If so, then we can remove "&&
> !defined(CONFIG_BUILD_ELF64)"
> from __pa_page_offset(). It shouldn't hurt the case where BUILD_ELF64
> is not set and Atsushi seems to agree.
It hurts performance a little bit, so if you can assure the macro shall
never be used on addresses from CKSEG0 if the load address is in XPHYS,
then you can easily arrange for the load address to be passed to the
preprocessor and use it as a condition here instead, which will be
optimised away as required by the compiler.
> BTW, maybe we can simply remove BUILD_ELF64 at all, since it's only
> used to add '-msym32' switch in the makefile. This switch could be
> automatically be added by the makefile instead thanks the following
> condition:
>
> if CONFIG_64BITS and ${load-y} in CKSEG0
> cflags-y += -msym32
> endif
>
> what do you think ?
I do not see enough of justification for -msym32 to be forced. This will
also raise the minimum version of binutils required to 2.16 for the
affected platforms, which may be a little bit too aggressive.
> Please keep these conversions in the platform specific codes before
> calling back the firmware.
The DECstation uses CPHYSADDR() for these purposes; see e.g.
arch/mips/dec/tc.c (not yet in the linux-mips.org repository -- to be
merged from the -mm tree sometime after 2.6.20). But I recall seeing
suggestions for this macro to be removed. Which I object against if there
is no usable alternative available (and I refuse to implement generic
functionality in platform-specific code -- there has been too much pain
already to merge many such bits scattered around).
Maciej
