*_pagetable_setup_[start,done] crap ?

*_pagetable_setup_[start,done] crap ?

[Jacek Luczak]

Hi,

while investigating following section mismatch warning:
WARNING: arch/x86/mm/built-in.o(.text+0xf2): Section mismatch in reference from the function paravirt_pagetable_setup_start() to the function
 .init.text:native_pagetable_setup_start()

I have found something with looks quite strange for me. This section mismatch is really easy to fix, as paravirt_pagetable_setup_start() is used by __init pagetable_init().
paravirt_pagetable_setup_start() (body in include/asm-x86/pgtable_32.h) calls, #ifndef CONFIG_PARAVIRT, __init native_pagetable_setup_start(). Here everything is OK and case is clear: to fix that mismatch, we need to annotate paravirt_pagetable_setup_start() with __init.

Same situation applies to paravirt_pagetable_setup_done(), but here it calls __init native_pagetable_setup_done(). On the other hand native_pagetable_setup_done() is quite strange:
void __init native_pagetable_setup_done(pgd_t *base)
{
}

This ,,change'' was introduced by commit: 551889a6e2a24a9c06fd453ea03b57b7746ffdc0 (x86: construct 32-bit boot time page tables in native format.), diff:
 void __init native_pagetable_setup_done(pgd_t *base)
 {
-#ifdef CONFIG_X86_PAE
-       /*
-        * Add low memory identity-mappings - SMP needs it when
-        * starting up on an AP from real-mode. In the non-PAE
-        * case we already have these mappings through head.S.
-        * All user-space mappings are explicitly cleared after
-        * SMP startup.
-        */
-       set_pgd(&base[0], base[USER_PTRS_PER_PGD]);
-#endif
 }

native_pagetable_setup_done() is used in __init pagetable_init() through paravirt_pagetable_setup_done(), nothing really scary.
One can take a look at arch/x86/kernel/paravirt.c:
struct pv_mmu_ops pv_mmu_ops = {
#ifndef CONFIG_X86_64
        .pagetable_setup_start = native_pagetable_setup_start,
        .pagetable_setup_done = native_pagetable_setup_done,
#endif
So when paravirt_pagetable_setup_done() will be called while CONFIG_PARAVIRT set it will be exactly same function as aravirt_pagetable_setup_done() while CONFIG_PARAVIRT not set.

Ohh... and prototypes are wrong:
diff --git a/include/asm-x86/pgtable_32.h b/include/asm-x86/pgtable_32.h
index 6cbc520..2167879 100644
--- a/include/asm-x86/pgtable_32.h
+++ b/include/asm-x86/pgtable_32.h
@@ -204,8 +204,8 @@ do {                                                \
  */
 #define update_mmu_cache(vma, address, pte) do { } while (0)

-void native_pagetable_setup_start(pgd_t *base);
-void native_pagetable_setup_done(pgd_t *base);
+extern void native_pagetable_setup_start(pgd_t *base) __init;
+extern void native_pagetable_setup_done(pgd_t *base) __init;

For me it looks like a complete crap :) Maybe it's OK, can anybody convince me?

-Jacek

Re: *_pagetable_setup_[start,done] crap ?

[Ingo Molnar]

* Jacek Luczak <difrost.kernel@gmail.com> wrote:

> Ohh... and prototypes are wrong:
> diff --git a/include/asm-x86/pgtable_32.h b/include/asm-x86/pgtable_32.h
> index 6cbc520..2167879 100644
> --- a/include/asm-x86/pgtable_32.h
> +++ b/include/asm-x86/pgtable_32.h
> @@ -204,8 +204,8 @@ do {                                                \
>   */
>  #define update_mmu_cache(vma, address, pte) do { } while (0)
> 
> -void native_pagetable_setup_start(pgd_t *base);
> -void native_pagetable_setup_done(pgd_t *base);
> +extern void native_pagetable_setup_start(pgd_t *base) __init;
> +extern void native_pagetable_setup_done(pgd_t *base) __init;

while generally it's nice to add 'extern' to prototypes, it's not an 
outright bug to not have it. Also, __init is a must-have in the 
definition only - it should not be added to the prototype.

	Ingo

Re: *_pagetable_setup_[start,done] crap ?

[Jacek Luczak]

Ingo Molnar pisze:
> * Jacek Luczak <difrost.kernel@gmail.com> wrote:
> 
>> Ohh... and prototypes are wrong:
>> diff --git a/include/asm-x86/pgtable_32.h b/include/asm-x86/pgtable_32.h
>> index 6cbc520..2167879 100644
>> --- a/include/asm-x86/pgtable_32.h
>> +++ b/include/asm-x86/pgtable_32.h
>> @@ -204,8 +204,8 @@ do {                                                \
>>   */
>>  #define update_mmu_cache(vma, address, pte) do { } while (0)
>>
>> -void native_pagetable_setup_start(pgd_t *base);
>> -void native_pagetable_setup_done(pgd_t *base);
>> +extern void native_pagetable_setup_start(pgd_t *base) __init;
>> +extern void native_pagetable_setup_done(pgd_t *base) __init;
> 
> while generally it's nice to add 'extern' to prototypes, it's not an 
> outright bug to not have it. Also, __init is a must-have in the 
> definition only - it should not be added to the prototype.
> 

Sounds reasonably. Sorry for calling something crap. When I slept well it looks fine and clear for me.

-Jacek