[PATCH] x86: Put trampoline_data into readonly section.

[PATCH] x86: Put trampoline_data into readonly section.

[Rakib Mullick]

 x86, trampoline_32: Put trampoline_data into readonly section.

 Put trampoline_data into readonly section when cpu
hotplug is supported. We cannot free up trampoline_data
because its been referenced from tboot_setup_sleep() which
is in non-cpuinit section.

 We were warned by the following warning:

WARNING: arch/x86/kernel/built-in.o(.text+0x13c77): Section mismatch
in reference from the function tboot_setup_sleep() to the variable
.cpuinit.rodata:trampoline_end
The function tboot_setup_sleep() references
the variable __cpuinitconst trampoline_end.
This is often because tboot_setup_sleep lacks a __cpuinitconst
annotation or the annotation of trampoline_end is wrong.

WARNING: arch/x86/kernel/built-in.o(.text+0x13c84): Section mismatch
in reference from the function tboot_setup_sleep() to the variable
.cpuinit.rodata:trampoline_data
The function tboot_setup_sleep() references
the variable __cpuinitconst trampoline_data.
This is often because tboot_setup_sleep lacks a __cpuinitconst
annotation or the annotation of trampoline_data is wrong.

Signed-off-by: Rakib Mullick <rakib.mullick@gmail.com>
---

--- linus/arch/x86/kernel/trampoline_32.S	2009-12-04 17:17:51.000000000 +0600
+++ rakib/arch/x86/kernel/trampoline_32.S	2009-12-04 17:24:44.000000000 +0600
@@ -32,8 +32,13 @@
 #include <asm/segment.h>
 #include <asm/page_types.h>

+#ifdef CONFIG_ACPI_SLEEP
+.section .rodata, "a", @progbits
+#else
 /* We can free up trampoline after bootup if cpu hotplug is not supported. */
 __CPUINITRODATA
+#endif
+
 .code16

 ENTRY(trampoline_data)

Re: [PATCH] x86: Put trampoline_data into readonly section.

[H. Peter Anvin]

On 12/04/2009 08:24 AM, Rakib Mullick wrote:
> WARNING: arch/x86/kernel/built-in.o(.text+0x13c77): Section mismatch
> in reference from the function tboot_setup_sleep() to the variable
> .cpuinit.rodata:trampoline_end
> The function tboot_setup_sleep() references
> the variable __cpuinitconst trampoline_end.
> This is often because tboot_setup_sleep lacks a __cpuinitconst
> annotation or the annotation of trampoline_end is wrong.
> 
> WARNING: arch/x86/kernel/built-in.o(.text+0x13c84): Section mismatch
> in reference from the function tboot_setup_sleep() to the variable
> .cpuinit.rodata:trampoline_data
> The function tboot_setup_sleep() references
> the variable __cpuinitconst trampoline_data.
> This is often because tboot_setup_sleep lacks a __cpuinitconst
> annotation or the annotation of trampoline_data is wrong.
> 
> --- linus/arch/x86/kernel/trampoline_32.S	2009-12-04 17:17:51.000000000 +0600
> +++ rakib/arch/x86/kernel/trampoline_32.S	2009-12-04 17:24:44.000000000 +0600
> @@ -32,8 +32,13 @@
>  #include <asm/segment.h>
>  #include <asm/page_types.h>
> 
> +#ifdef CONFIG_ACPI_SLEEP
> +.section .rodata, "a", @progbits
> +#else
>  /* We can free up trampoline after bootup if cpu hotplug is not supported. */
>  __CPUINITRODATA
> +#endif
> +

This is a false positive.

The reference in question is actually:

	add_mac_region(virt_to_phys(trampoline_base), TRAMPOLINE_SIZE);

... which in turn is defined as ...

#define TRAMPOLINE_SIZE roundup(trampoline_end - trampoline_data, PAGE_SIZE)

This causes "references" of two symbols which aren't actually referenced
at all.

Perhaps the better thing would be to define trampoline_size as an
absolute symbol in arch/x86/kernel/trampoline_*.S, especially since it
probably generates bad code to subtract two global symbols like that.

	-hpa

Re: [PATCH] x86: Put trampoline_data into readonly section.

[Rakib Mullick]

On 12/5/09, H. Peter Anvin <hpa@zytor.com> wrote:
> On 12/04/2009 08:24 AM, Rakib Mullick wrote:
>
>  Perhaps the better thing would be to define trampoline_size as an
>  absolute symbol in arch/x86/kernel/trampoline_*.S, especially since it
>  probably generates bad code to subtract two global symbols like that.
>
But - trampoline_data is placed into - cpu init readonly data section.
And we are
referencing it from non-cpuinit function. ....... so I think the
problem remains.
Isn't it ? Or am I missing anything?

Rakib
>
>         -hpa
>

Re: [PATCH] x86: Put trampoline_data into readonly section.

[H. Peter Anvin]

On 12/04/2009 08:47 PM, Rakib Mullick wrote:
> On 12/5/09, H. Peter Anvin<hpa@zytor.com>  wrote:
>> On 12/04/2009 08:24 AM, Rakib Mullick wrote:
>>
>>   Perhaps the better thing would be to define trampoline_size as an
>>   absolute symbol in arch/x86/kernel/trampoline_*.S, especially since it
>>   probably generates bad code to subtract two global symbols like that.
>>
> But - trampoline_data is placed into - cpu init readonly data section.
> And we are
> referencing it from non-cpuinit function. ....... so I think the
> problem remains.
> Isn't it ? Or am I missing anything?
>

No, we're not actually referencing it at all.  We're just using its 
length, whereas in reality we actually have copied the code elsewhere 
already.

	-hpa