setup.c: introduce __pa_symbol() and get ride of CPHYSADDR()

PATCH In review Franck Bui-Huu 4 participants 19 messages Oct 6, 2006 – Oct 11, 2006 MIPS

lore

Share & tools

lore

Franck Bui-Huu

author

2006-10-06 13:35 UTC #0 lore raw

PATCH 1/1 setup.c: introduce __pa_symbol() and get ride of CPHYSADDR()

From: Franck Bui-Huu <[email protected]>

This patch introduces __pa_symbol() macro which should be used to calculate the physical address of kernel symbols. We should fix any linker issues in this macro, if any, but more importantly __pa_symbol() uses __pa() to do the real conversion.

One resulting thing is that we can see that most of CPHYSADDR() uses weren't needed.

It also rely on RELOC_HIDE() to avoid any compiler's oddities when doing arithmetics on symbols.

Signed-off-by Franck Bui-Huu <[email protected]>

---
 arch/mips/kernel/setup.c |   17 ++++++++++-------
 include/asm-mips/page.h  |    1 +
 2 files changed, 11 insertions(+), 7 deletions(-)

arch/mips/kernel/setup.c +10 -7

index fdbb508..cccccd5 100644
@@ -204,12 +204,12 @@ static void __init finalize_initrd(void)
 		printk(KERN_INFO "Initrd not found or empty");
 		goto disable;
 	}
-	if (CPHYSADDR(initrd_end) > PFN_PHYS(max_low_pfn)) {
+	if (__pa(initrd_end) > PFN_PHYS(max_low_pfn)) {
 		printk("Initrd extends beyond end of memory");
 		goto disable;
 	}
 
-	reserve_bootmem(CPHYSADDR(initrd_start), size);
+	reserve_bootmem(__pa(initrd_start), size);
 	initrd_below_start_ok = 1;
 
 	printk(KERN_INFO "Initial ramdisk at: 0x%lx (%lu bytes)\n",

@@ -256,7 +256,10 @@ static void __init bootmem_init(void)
 	 * of usable memory.
 	 */
 	reserved_end = init_initrd();
-	reserved_end = PFN_UP(CPHYSADDR(max(reserved_end, (unsigned long)&_end)));
+	if (reserved_end > (unsigned long)&_end)
+		reserved_end = PFN_UP(__pa(reserved_end));
+	else
+		reserved_end = PFN_UP(__pa_symbol(&_end));
 
 	/*
 	 * Find the highest page frame number we have available.

@@ -428,10 +431,10 @@ static void __init resource_init(void)
 	if (UNCAC_BASE != IO_BASE)
 		return;
 
-	code_resource.start = virt_to_phys(&_text);
-	code_resource.end = virt_to_phys(&_etext) - 1;
-	data_resource.start = virt_to_phys(&_etext);
-	data_resource.end = virt_to_phys(&_edata) - 1;
+	code_resource.start = __pa_symbol(&_text);
+	code_resource.end = __pa_symbol(&_etext) - 1;
+	data_resource.start = __pa_symbol(&_etext);
+	data_resource.end = __pa_symbol(&_edata) - 1;
 
 	/*
 	 * Request address space for all standard RAM.

include/asm-mips/page.h +1 -0

index 32e5625..1d5f4a3 100644
@@ -132,6 +132,7 @@ #endif /* !__ASSEMBLY__ */
 #define PAGE_ALIGN(addr)	(((addr) + PAGE_SIZE - 1) & PAGE_MASK)
 
 #define __pa(x)			((unsigned long) (x) - PAGE_OFFSET)
+#define __pa_symbol(x)		__pa(RELOC_HIDE((unsigned long)(x),0))
 #define __va(x)			((void *)((unsigned long) (x) + PAGE_OFFSET))
 
 #define pfn_to_kaddr(pfn)	__va((pfn) << PAGE_SHIFT)

Signature · 1 line

1.4.2.3

Thiemo Seufer

2006-10-06 17:22 UTC #1 lore raw

Quoted 24 lines · Franck Bui-Huu

From: Franck Bui-Huu <[email protected]>
This patch introduces __pa_symbol() macro which should be used to calculate the physical address of kernel symbols. We should fix any linker issues in this macro, if any, but more importantly __pa_symbol() uses __pa() to do the real conversion.
One resulting thing is that we can see that most of CPHYSADDR() uses weren't needed.
It also rely on RELOC_HIDE() to avoid any compiler's oddities when doing arithmetics on symbols.
Signed-off-by Franck Bui-Huu <[email protected]>
---
 arch/mips/kernel/setup.c |   17 ++++++++++-------
 include/asm-mips/page.h  |    1 +
 2 files changed, 11 insertions(+), 7 deletions(-)
arch/mips/kernel/setup.c +0 -0
index fdbb508..cccccd5 100644

arch/mips/kernel/setup.c static void __init finalize_initrd(void) · line 207

Quoted 5 lines

arch/mips/kernel/setup.c +1 -1

@@ -204,12 +204,12 @@ static void __init finalize_initrd(void)
 		printk(KERN_INFO "Initrd not found or empty");
 		goto disable;
 	}
-	if (CPHYSADDR(initrd_end) > PFN_PHYS(max_low_pfn)) {
+	if (__pa(initrd_end) > PFN_PHYS(max_low_pfn)) {

ISTR this failed on O2, where kernel+initrd are loaded into KSEG0 but the PAGE_OFFSET is for XKPHYS.

Thiemo

Franck Bui-Huu

author

2006-10-09 11:57 UTC #2 lore raw

Quoted 8 lines · Thiemo Seufer

arch/mips/kernel/setup.c static void __init finalize_initrd(void) · line 207
Quoted 3 lines · Franck Bui-Huu
arch/mips/kernel/setup.c +1 -1
-	if (CPHYSADDR(initrd_end) > PFN_PHYS(max_low_pfn)) {
+	if (__pa(initrd_end) > PFN_PHYS(max_low_pfn)) {
ISTR this failed on O2, where kernel+initrd are loaded into KSEG0 but the PAGE_OFFSET is for XKPHYS.

I guess that you were meaning somthing like:

LOADADDR = 0xffffffff80004000
PAGE_OFFSET = 0xa800000000000000

is that correct ? If so could you explain the choice of these values because I fail to understand it.

Thanks
Franck

Thiemo Seufer

2006-10-09 13:21 UTC #3 lore raw

Quoted 10 lines · Franck Bui-Huu

Quoted 8 lines · Thiemo Seufer
arch/mips/kernel/setup.c static void __init finalize_initrd(void) · line 207
Quoted 3 lines · Franck Bui-Huu
arch/mips/kernel/setup.c +1 -1
-	if (CPHYSADDR(initrd_end) > PFN_PHYS(max_low_pfn)) {
+	if (__pa(initrd_end) > PFN_PHYS(max_low_pfn)) {
ISTR this failed on O2, where kernel+initrd are loaded into KSEG0 but the PAGE_OFFSET is for XKPHYS.

Quoted 6 lines

I guess that you were meaning somthing like:
LOADADDR = 0xffffffff80004000
PAGE_OFFSET = 0xa800000000000000
is that correct ?

Yes.

Quoted 2 lines

If so could you explain the choice of these values because I fail to understand it.

It allows to load a 64-bit kernel in KSEG0, and use short 2-instruction symbol references there. At the same time, it allows access to more address space for memory and I/O than would fit in KSEG0.

Thiemo

Franck Bui-Huu

author

2006-10-09 14:24 UTC #4 lore raw

Quoted 13 lines · Thiemo Seufer

Quoted 12 lines · Franck Bui-Huu
Quoted 6 lines · Thiemo Seufer
arch/mips/kernel/setup.c static void __init finalize_initrd(void) · line 207
Quoted 2 lines · Franck Bui-Huu
arch/mips/kernel/setup.c +1 -1
-	if (CPHYSADDR(initrd_end) > PFN_PHYS(max_low_pfn)) {
+	if (__pa(initrd_end) > PFN_PHYS(max_low_pfn)) {
ISTR this failed on O2, where kernel+initrd are loaded into KSEG0 but the PAGE_OFFSET is for XKPHYS.
I guess that you were meaning somthing like:
LOADADDR = 0xffffffff80004000
PAGE_OFFSET = 0xa800000000000000

Quoted 8 lines

Quoted 1 line
is that correct ?
Yes.
Quoted 2 lines
If so could you explain the choice of these values because I fail to understand it.
It allows to load a 64-bit kernel in KSEG0,

sorry to be ignorant of 64 bit kernels, but what's the point to load them in KSEG0.

Quoted 1 line

and use short 2-instruction symbol references there.

do you mean "it allows to use only 2 'lui' instructions to load a symbol address into a register" ?

Futhermore I don't see how some part of the kernel convert virtual address into a physical one with such values. For example in setup.c, the function resource_init() does:

	code_resource.start = virt_to_phys(&_text);
	code_resource.end = virt_to_phys(&_etext) - 1;
	data_resource.start = virt_to_phys(&_etext);
	data_resource.end = virt_to_phys(&_edata) - 1;

How does it work in this case ?

Thanks
Franck

Thiemo Seufer

2006-10-09 14:58 UTC #5 lore raw

Franck Bui-Huu wrote:
[snip]

Quoted 7 lines

Quoted 4 lines
Quoted 2 lines
If so could you explain the choice of these values because I fail to understand it.
It allows to load a 64-bit kernel in KSEG0,
sorry to be ignorant of 64 bit kernels, but what's the point to load them in KSEG0.

Smaller code with better performance.

Quoted 4 lines

Quoted 1 line
and use short 2-instruction symbol references there.
do you mean "it allows to use only 2 'lui' instructions to load a symbol address into a register" ?

It allows a 2-instruction "lui ; addiu" sequence instead of a 6-instruction "lui ; lui ; addiu ; addiu ; dsll32 ; addu" sequence.

Quoted 10 lines

Futhermore I don't see how some part of the kernel convert virtual address into a physical one with such values. For example in setup.c, the function resource_init() does:
	code_resource.start = virt_to_phys(&_text);
	code_resource.end = virt_to_phys(&_etext) - 1;
	data_resource.start = virt_to_phys(&_etext);
	data_resource.end = virt_to_phys(&_edata) - 1;
How does it work in this case ?

Those are addresses in 64-bit space, no special handling is needed there.

The same doesn't hold for the initrd addresses supplied by the (32-bit) firmware. The firmware doesn't convert the kernel parameters to 64-bit values because the O2 kernel used to allow a pure 32-bit build, and the firmware can't find out what's actually inside the object file.

Thiemo

Franck Bui-Huu

author

2006-10-09 15:29 UTC #6 lore raw

Quoted 6 lines · Thiemo Seufer

Quoted 2 lines · Franck Bui-Huu
sorry to be ignorant of 64 bit kernels, but what's the point to load them in KSEG0.
Smaller code with better performance.

you mean we get smaller code _only_ by using the short 2 instructions you described below ?

Quoted 7 lines

Quoted 3 lines
Quoted 1 line
and use short 2-instruction symbol references there.
do you mean "it allows to use only 2 'lui' instructions to load a symbol address into a register" ?
It allows a 2-instruction "lui ; addiu" sequence instead of a 6-instruction "lui ; lui ; addiu ; addiu ; dsll32 ; addu" sequence.

[snip]

Quoted 10 lines

Quoted 7 lines
	code_resource.start = virt_to_phys(&_text);
	code_resource.end = virt_to_phys(&_etext) - 1;
	data_resource.start = virt_to_phys(&_etext);
	data_resource.end = virt_to_phys(&_edata) - 1;
How does it work in this case ?
Those are addresses in 64-bit space, no special handling is needed there.

hm I'missing something there. Let's say that '&_text' is in KSEG0 and is equal to 0xffffffff80000000. In this case virt_to_phys() returns 0x57ffffff80000000 (with PAGE_OFFSET = 0xa800000000000000). Is this physical address correct ??

Quoted 6 lines

The same doesn't hold for the initrd addresses supplied by the (32-bit) firmware. The firmware doesn't convert the kernel parameters to 64-bit values because the O2 kernel used to allow a pure 32-bit build, and the firmware can't find out what's actually inside the object file.

This should be already handled by this code taken from setup.c:

static int __init rd_start_early(char *p)

{
        unsigned long start = memparse(p, &p);

#ifdef CONFIG_64BIT
        /* HACK: Guess if the sign extension was forgotten */
        if (start > 0x0000000080000000 && start < 0x00000000ffffffff)
                start |= 0xffffffff00000000UL;
#endif
        initrd_start = start;
        initrd_end += start;

        return 0;
}

Thanks
Franck

Atsushi Nemoto

2006-10-09 15:49 UTC #7 lore raw

In reply to Thiemo Seufer

Quoted 5 lines · Thiemo Seufer

Quoted 2 lines
do you mean "it allows to use only 2 'lui' instructions to load a symbol address into a register" ?
It allows a 2-instruction "lui ; addiu" sequence instead of a 6-instruction "lui ; lui ; addiu ; addiu ; dsll32 ; addu" sequence.

Just for clarification: IIRC this optimization needs somewhat up-to-date binutils/gcc and is not enabled on current lmo kernel, right?

---
Atsushi Nemoto

Thiemo Seufer

2006-10-09 16:59 UTC #8 lore raw

Quoted 10 lines · Atsushi Nemoto

Quoted 5 lines · Thiemo Seufer
Quoted 2 lines
do you mean "it allows to use only 2 'lui' instructions to load a symbol address into a register" ?
It allows a 2-instruction "lui ; addiu" sequence instead of a 6-instruction "lui ; lui ; addiu ; addiu ; dsll32 ; addu" sequence.
Just for clarification: IIRC this optimization needs somewhat up-to-date binutils/gcc and is not enabled on current lmo kernel, right?

For old toolchains there used to be a gruesome hack (which AFAIR broke at some point), for modern toolchains there's -msym32.

Thiemo

Atsushi Nemoto

2006-10-10 08:49 UTC #9 lore raw

Quoted 6 lines · Thiemo Seufer

Quoted 3 lines
Just for clarification: IIRC this optimization needs somewhat up-to-date binutils/gcc and is not enabled on current lmo kernel, right?
For old toolchains there used to be a gruesome hack (which AFAIR broke at some point), for modern toolchains there's -msym32.

Hmm, I found that the -msym32 is enabled if BUILD_ELF64 was not selected, since 2.6.17. But does CONFIG_BUILD_ELF64=n really work for modules? While MAP_BASE is 0xc000000000000000 for most 64-bit platforms, I suppose modules should not be compiled with -msym32.

---
Atsushi Nemoto

Franck Bui-Huu

author

2006-10-10 13:48 UTC #10 lore raw

Quoted 4 lines · Atsushi Nemoto

Quoted 3 lines · Thiemo Seufer
Quoted 3 lines
Just for clarification: IIRC this optimization needs somewhat up-to-date binutils/gcc and is not enabled on current lmo kernel, right?

Quoted 8 lines

Quoted 2 lines
For old toolchains there used to be a gruesome hack (which AFAIR broke at some point), for modern toolchains there's -msym32.
Hmm, I found that the -msym32 is enabled if BUILD_ELF64 was not selected, since 2.6.17. But does CONFIG_BUILD_ELF64=n really work for modules? While MAP_BASE is 0xc000000000000000 for most 64-bit platforms, I suppose modules should not be compiled with -msym32.

heh ? I'm wondering if anybody is using 'CONFIG_BUILD_ELF64=n' config at all...

Atsushi, do you have any idea on how address are translated with 'CONFIG_BUILD_ELF64=n' config ? How such code is supposed to work ?

	code_resource.start = virt_to_phys(&_text);
 	code_resource.end = virt_to_phys(&_etext) - 1;
	data_resource.start = virt_to_phys(&_etext);

data_resource.end = virt_to_phys(&_edata) - 1;

Let's say that '&_text' is in KSEG0 and is equal to 0xffffffff80000000. In this case virt_to_phys() returns 0x57ffffff80000000
(with PAGE_OFFSET = 0xa800000000000000). Is this physical address correct ?

Thanks
Franck

Atsushi Nemoto

2006-10-10 14:17 UTC #11 lore raw

Quoted 2 lines · Franck Bui-Huu

heh ? I'm wondering if anybody is using 'CONFIG_BUILD_ELF64=n' config at all...

arch/mips/configs/bigsur_defconfig:# CONFIG_BUILD_ELF64 is not set arch/mips/configs/ip27_defconfig:# CONFIG_BUILD_ELF64 is not set arch/mips/configs/ip32_defconfig:# CONFIG_BUILD_ELF64 is not set arch/mips/configs/ocelot_c_defconfig:# CONFIG_BUILD_ELF64 is not set arch/mips/configs/ocelot_g_defconfig:# CONFIG_BUILD_ELF64 is not set arch/mips/configs/sb1250-swarm_defconfig:# CONFIG_BUILD_ELF64 is not set

According to arch/mips/configs, nobody is using CONFIG_BUILD_ELF64=y :-)

Also one might use gcc 3.x which ignore -msym32 option ...

Quoted 6 lines

Atsushi, do you have any idea on how address are translated with 'CONFIG_BUILD_ELF64=n' config ? How such code is supposed to work ?
	code_resource.start = virt_to_phys(&_text);
 	code_resource.end = virt_to_phys(&_etext) - 1;
	data_resource.start = virt_to_phys(&_etext);

Quoted 6 lines

data_resource.end = virt_to_phys(&_edata) - 1;
Let's say that '&_text' is in KSEG0 and is equal to 0xffffffff80000000. In this case virt_to_phys() returns 0x57ffffff80000000
(with PAGE_OFFSET = 0xa800000000000000). Is this physical address correct ?

I think this peice of code is just broken, as you said. This is bogus but harmless since we have not checked these resources are successfully registered or not.

---
Atsushi Nemoto

Franck Bui-Huu

author

2006-10-10 15:00 UTC #12 lore raw

Quoted 11 lines · Atsushi Nemoto

Quoted 2 lines · Franck Bui-Huu
heh ? I'm wondering if anybody is using 'CONFIG_BUILD_ELF64=n' config at all...
arch/mips/configs/bigsur_defconfig:# CONFIG_BUILD_ELF64 is not set arch/mips/configs/ip27_defconfig:# CONFIG_BUILD_ELF64 is not set arch/mips/configs/ip32_defconfig:# CONFIG_BUILD_ELF64 is not set arch/mips/configs/ocelot_c_defconfig:# CONFIG_BUILD_ELF64 is not set arch/mips/configs/ocelot_g_defconfig:# CONFIG_BUILD_ELF64 is not set arch/mips/configs/sb1250-swarm_defconfig:# CONFIG_BUILD_ELF64 is not set

Quoted 4 lines

According to arch/mips/configs, nobody is using CONFIG_BUILD_ELF64=y :-)
Also one might use gcc 3.x which ignore -msym32 option ...

yeah, that's probably the reason...

Quoted 8 lines

Quoted 8 lines
Atsushi, do you have any idea on how address are translated with 'CONFIG_BUILD_ELF64=n' config ? How such code is supposed to work ?
	code_resource.start = virt_to_phys(&_text);
 	code_resource.end = virt_to_phys(&_etext) - 1;
	data_resource.start = virt_to_phys(&_etext);
data_resource.end = virt_to_phys(&_edata) - 1;

Quoted 9 lines

Quoted 4 lines
Let's say that '&_text' is in KSEG0 and is equal to 0xffffffff80000000. In this case virt_to_phys() returns 0x57ffffff80000000
(with PAGE_OFFSET = 0xa800000000000000). Is this physical address correct ?
I think this peice of code is just broken, as you said. This is bogus but harmless since we have not checked these resources are successfully registered or not.

what about all other uses of virt_to_phys(x) ? And what the point to set PAGE_OFFSET to 0xa800000000000000 ? I'm really confused...

Franck

Atsushi Nemoto

2006-10-10 15:27 UTC #13 lore raw

Quoted 6 lines · Franck Bui-Huu

Quoted 3 lines
I think this peice of code is just broken, as you said. This is bogus but harmless since we have not checked these resources are successfully registered or not.
what about all other uses of virt_to_phys(x) ? And what the point to set PAGE_OFFSET to 0xa800000000000000 ? I'm really confused...

For now I have not seen any problem on other usages.

We can use large flat mapping space in XKPHYS. No TLB conversion, no highmem trick.

---
Atsushi Nemoto

Franck Bui-Huu

author

2006-10-10 16:03 UTC #14 lore raw

Quoted 9 lines · Atsushi Nemoto

Quoted 5 lines · Franck Bui-Huu
Quoted 3 lines
I think this peice of code is just broken, as you said. This is bogus but harmless since we have not checked these resources are successfully registered or not.
what about all other uses of virt_to_phys(x) ? And what the point to set PAGE_OFFSET to 0xa800000000000000 ? I'm really confused...
For now I have not seen any problem on other usages.

do you know at which point the kernel starts to use XKPHYS addresses ?

Quoted 3 lines

We can use large flat mapping space in XKPHYS. No TLB conversion, no highmem trick.

ok, and does the trick on KSEG0/XKPHYS really worth ? I mean what is the size code gain ?

Thanks
Franck

Franck Bui-Huu

author

2006-10-10 16:15 UTC #15 lore raw

Quoted 5 lines · Franck Bui-Huu

Quoted 4 lines · Atsushi Nemoto
Quoted 3 lines · Franck Bui-Huu
Quoted 3 lines
I think this peice of code is just broken, as you said. This is bogus but harmless since we have not checked these resources are successfully registered or not.

Quoted 7 lines

Quoted 4 lines
Quoted 2 lines
what about all other uses of virt_to_phys(x) ? And what the point to set PAGE_OFFSET to 0xa800000000000000 ? I'm really confused...
For now I have not seen any problem on other usages.
do you know at which point the kernel starts to use XKPHYS addresses ?

Ok, I found the answer to this question...

Franck

Ralf Baechle

2006-10-10 21:53 UTC #16 lore raw

In reply to Franck Bui-Huu

Quoted 2 lines · Franck Bui-Huu

ok, and does the trick on KSEG0/XKPHYS really worth ? I mean what is the size code gain ?

Gcc / gas generate a 6 instruction sequence to load something from a 64-bit address, basically lui, add, dsll16, add, dsll16, add. It's just 2 instructions for 32-bit addresses. This boils down to space savings in the hundred of kilobytes for a kernel.

Of course there are more intelligent way to load an address via global pointer optimization but that's the two choices we got today.

Ralf

Atsushi Nemoto

2006-10-11 03:12 UTC #17 lore raw

Quoted 7 lines · Ralf Baechle

Quoted 2 lines
ok, and does the trick on KSEG0/XKPHYS really worth ? I mean what is the size code gain ?
Gcc / gas generate a 6 instruction sequence to load something from a 64-bit address, basically lui, add, dsll16, add, dsll16, add. It's just 2 instructions for 32-bit addresses. This boils down to space savings in the hundred of kilobytes for a kernel.

Yes, I got ~10% smaller kernel (4.8MB to 4.4MB) with -msym32.

If modules are loaded into CKSEG2, we can use -msym32 for modules as well. Until then this patch can be used for workaroung:

http://www.linux-mips.org/archives/linux-mips/2006-10/msg00111.html

---
Atsushi Nemoto

Franck Bui-Huu

author

2006-10-11 09:37 UTC #18 lore raw

In reply to Ralf Baechle

Quoted 10 lines · Ralf Baechle

Quoted 2 lines · Franck Bui-Huu
ok, and does the trick on KSEG0/XKPHYS really worth ? I mean what is the size code gain ?
Gcc / gas generate a 6 instruction sequence to load something from a 64-bit address, basically lui, add, dsll16, add, dsll16, add. It's just 2 instructions for 32-bit addresses. This boils down to space savings in the hundred of kilobytes for a kernel.

ok, the hack seems to worth it...

Franck