[PATCH 0/5] X86/XEN: Merge x86_init.paging.pagetable_setup_start and x86_init.paging.pagetable_setup_done PVOPS and document the semantic

[PATCH 0/5] X86/XEN: Merge x86_init.paging.pagetable_setup_start and x86_init.paging.pagetable_setup_done PVOPS and document the semantic

[Attilio Rao]

Currently the definition of x86_init.paging.pagetable_setup_start and
x86_init.paging.pagetable_setup_done is twisted and not really well
defined (in terms of prototypes desired). More specifically:
pagetable_setup_start:
 * it is a nop on x86_32
 * it is a nop for the XEN case
 * cleans up the boot time page table in the x86_64 case

pagetable_setup_done:
 * it is a nop on x86_32
 * sets up accessor functions for pagetable manipulation, for the
   XEN case
 * it is a nop on x86_64

Most of this logic can be skipped by creating a new PVOPS that can handle
pagetable setup and pre/post operations on it.
The new PVOPS must be called only once, during boot-time setup and
after the direct mapping for physical memory is available.

Attilio Rao (5):
  XEN: Remove the base argument from
    x86_init.paging.pagetable_setup_done PVOPS
  XEN: Remove the base argument from
    x86_init.paging.pagetable_setup_start PVOPS
  X86/XEN: Introduce the x86_init.paging.pagetable_init() PVOPS
  X86/XEN: Retire now unused x86_init.paging.pagetable_setup_start and
    x86_init.paging.pagetable_setup_done PVOPS
  X86/XEN: Add few lines explaining simple semantic for
    x86_init.paging.pagetable_init PVOPS

 arch/x86/include/asm/pgtable_types.h |    6 ++----
 arch/x86/include/asm/x86_init.h      |   11 +++++++----
 arch/x86/kernel/setup.c              |    4 +---
 arch/x86/kernel/x86_init.c           |    4 +---
 arch/x86/mm/init_32.c                |   12 ++++++------
 arch/x86/xen/mmu.c                   |   18 +++++++-----------
 6 files changed, 24 insertions(+), 31 deletions(-)

-- 
1.7.2.5

[PATCH 1/5] XEN: Remove the base argument from x86_init.paging.pagetable_setup_done PVOPS

[Attilio Rao]

x86_init.paging.pagetable_setup_done PVOPS does not really need to know about
the base argument, thus just remove it.

Signed-off-by: Attilio Rao <attilio.rao@citrix.com>
---
 arch/x86/include/asm/pgtable_types.h |    6 +++---
 arch/x86/include/asm/x86_init.h      |    2 +-
 arch/x86/kernel/setup.c              |    2 +-
 arch/x86/kernel/x86_init.c           |    3 ++-
 arch/x86/mm/init_32.c                |    2 +-
 arch/x86/xen/mmu.c                   |    2 +-
 6 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/arch/x86/include/asm/pgtable_types.h b/arch/x86/include/asm/pgtable_types.h
index 013286a..f9e07b0 100644
--- a/arch/x86/include/asm/pgtable_types.h
+++ b/arch/x86/include/asm/pgtable_types.h
@@ -304,10 +304,10 @@ void set_pte_vaddr(unsigned long vaddr, pte_t pte);
 extern void native_pagetable_reserve(u64 start, u64 end);
 #ifdef CONFIG_X86_32
 extern void native_pagetable_setup_start(pgd_t *base);
-extern void native_pagetable_setup_done(pgd_t *base);
+extern void native_pagetable_setup_done(void);
 #else
-#define native_pagetable_setup_start x86_init_pgd_noop
-#define native_pagetable_setup_done  x86_init_pgd_noop
+#define native_pagetable_setup_start x86_init_pgd_start_noop
+#define native_pagetable_setup_done  x86_init_pgd_stop_noop
 #endif
 
 struct seq_file;
diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
index 38155f6..439a4c3 100644
--- a/arch/x86/include/asm/x86_init.h
+++ b/arch/x86/include/asm/x86_init.h
@@ -86,7 +86,7 @@ struct x86_init_mapping {
  */
 struct x86_init_paging {
 	void (*pagetable_setup_start)(pgd_t *base);
-	void (*pagetable_setup_done)(pgd_t *base);
+	void (*pagetable_setup_done)(void);
 };
 
 /**
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index f4b9b80..ed9094d 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -963,7 +963,7 @@ void __init setup_arch(char **cmdline_p)
 
 	x86_init.paging.pagetable_setup_start(swapper_pg_dir);
 	paging_init();
-	x86_init.paging.pagetable_setup_done(swapper_pg_dir);
+	x86_init.paging.pagetable_setup_done();
 
 	if (boot_cpu_data.cpuid_level >= 0) {
 		/* A CPU has %cr4 if and only if it has CPUID */
diff --git a/arch/x86/kernel/x86_init.c b/arch/x86/kernel/x86_init.c
index 9f3167e..b27b30d 100644
--- a/arch/x86/kernel/x86_init.c
+++ b/arch/x86/kernel/x86_init.c
@@ -26,7 +26,8 @@
 
 void __cpuinit x86_init_noop(void) { }
 void __init x86_init_uint_noop(unsigned int unused) { }
-void __init x86_init_pgd_noop(pgd_t *unused) { }
+void __init x86_init_pgd_start_noop(pgd_t *unused) { }
+void __init x86_init_pgd_stop_noop(void) { }
 int __init iommu_init_noop(void) { return 0; }
 void iommu_shutdown_noop(void) { }
 
diff --git a/arch/x86/mm/init_32.c b/arch/x86/mm/init_32.c
index 575d86f..1019156 100644
--- a/arch/x86/mm/init_32.c
+++ b/arch/x86/mm/init_32.c
@@ -477,7 +477,7 @@ void __init native_pagetable_setup_start(pgd_t *base)
 	paravirt_alloc_pmd(&init_mm, __pa(base) >> PAGE_SHIFT);
 }
 
-void __init native_pagetable_setup_done(pgd_t *base)
+void __init native_pagetable_setup_done(void)
 {
 }
 
diff --git a/arch/x86/xen/mmu.c b/arch/x86/xen/mmu.c
index b65a761..d847548 100644
--- a/arch/x86/xen/mmu.c
+++ b/arch/x86/xen/mmu.c
@@ -1194,7 +1194,7 @@ static __init void xen_mapping_pagetable_reserve(u64 start, u64 end)
 
 static void xen_post_allocator_init(void);
 
-static void __init xen_pagetable_setup_done(pgd_t *base)
+static void __init xen_pagetable_setup_done(void)
 {
 	xen_setup_shared_info();
 	xen_post_allocator_init();
-- 
1.7.2.5

[PATCH 2/5] XEN: Remove the base argument from x86_init.paging.pagetable_setup_start PVOPS

[Attilio Rao]

x86_init.paging.pagetable_setup_start for native will however use
swapper_pg_dir in the single place where it is used and for native the
argument is simply unused. Aditionally, the comments already point to
swapper_pg_dir as the sole base touched.
Finally, this will help with further merging of
x86_init.paging.pagetable_setup_start with
x86_init.paging.pagetable_setup_done PVOPS.

Signed-off-by: Attilio Rao <attilio.rao@citrix.com>
---
 arch/x86/include/asm/pgtable_types.h |    6 +++---
 arch/x86/include/asm/x86_init.h      |    2 +-
 arch/x86/kernel/setup.c              |    2 +-
 arch/x86/kernel/x86_init.c           |    3 +--
 arch/x86/mm/init_32.c                |    5 ++++-
 arch/x86/xen/mmu.c                   |    2 +-
 6 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/arch/x86/include/asm/pgtable_types.h b/arch/x86/include/asm/pgtable_types.h
index f9e07b0..9b1c1f7 100644
--- a/arch/x86/include/asm/pgtable_types.h
+++ b/arch/x86/include/asm/pgtable_types.h
@@ -303,11 +303,11 @@ void set_pte_vaddr(unsigned long vaddr, pte_t pte);
 
 extern void native_pagetable_reserve(u64 start, u64 end);
 #ifdef CONFIG_X86_32
-extern void native_pagetable_setup_start(pgd_t *base);
+extern void native_pagetable_setup_start(void);
 extern void native_pagetable_setup_done(void);
 #else
-#define native_pagetable_setup_start x86_init_pgd_start_noop
-#define native_pagetable_setup_done  x86_init_pgd_stop_noop
+#define native_pagetable_setup_start x86_init_pgd_noop
+#define native_pagetable_setup_done  x86_init_pgd_noop
 #endif
 
 struct seq_file;
diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
index 439a4c3..efd0075 100644
--- a/arch/x86/include/asm/x86_init.h
+++ b/arch/x86/include/asm/x86_init.h
@@ -85,7 +85,7 @@ struct x86_init_mapping {
  * @pagetable_setup_done:	platform specific post paging_init() call
  */
 struct x86_init_paging {
-	void (*pagetable_setup_start)(pgd_t *base);
+	void (*pagetable_setup_start)(void);
 	void (*pagetable_setup_done)(void);
 };
 
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index ed9094d..d3d8f00 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -961,7 +961,7 @@ void __init setup_arch(char **cmdline_p)
 	kvmclock_init();
 #endif
 
-	x86_init.paging.pagetable_setup_start(swapper_pg_dir);
+	x86_init.paging.pagetable_setup_start();
 	paging_init();
 	x86_init.paging.pagetable_setup_done();
 
diff --git a/arch/x86/kernel/x86_init.c b/arch/x86/kernel/x86_init.c
index b27b30d..849be14 100644
--- a/arch/x86/kernel/x86_init.c
+++ b/arch/x86/kernel/x86_init.c
@@ -26,8 +26,7 @@
 
 void __cpuinit x86_init_noop(void) { }
 void __init x86_init_uint_noop(unsigned int unused) { }
-void __init x86_init_pgd_start_noop(pgd_t *unused) { }
-void __init x86_init_pgd_stop_noop(void) { }
+void __init x86_init_pgd_noop(void) { }
 int __init iommu_init_noop(void) { return 0; }
 void iommu_shutdown_noop(void) { }
 
diff --git a/arch/x86/mm/init_32.c b/arch/x86/mm/init_32.c
index 1019156..7999cef 100644
--- a/arch/x86/mm/init_32.c
+++ b/arch/x86/mm/init_32.c
@@ -445,14 +445,17 @@ static inline void permanent_kmaps_init(pgd_t *pgd_base)
 }
 #endif /* CONFIG_HIGHMEM */
 
-void __init native_pagetable_setup_start(pgd_t *base)
+void __init native_pagetable_setup_start(void)
 {
 	unsigned long pfn, va;
+	pgd_t *base;
 	pgd_t *pgd;
 	pud_t *pud;
 	pmd_t *pmd;
 	pte_t *pte;
 
+	base = swapper_pg_dir;
+
 	/*
 	 * Remove any mappings which extend past the end of physical
 	 * memory from the boot time page table:
diff --git a/arch/x86/xen/mmu.c b/arch/x86/xen/mmu.c
index d847548..04a0a8f 100644
--- a/arch/x86/xen/mmu.c
+++ b/arch/x86/xen/mmu.c
@@ -1174,7 +1174,7 @@ static void xen_exit_mmap(struct mm_struct *mm)
 	spin_unlock(&mm->page_table_lock);
 }
 
-static void __init xen_pagetable_setup_start(pgd_t *base)
+static void __init xen_pagetable_setup_start(void)
 {
 }
 
-- 
1.7.2.5

Re: [PATCH 2/5] XEN: Remove the base argument from x86_init.paging.pagetable_setup_start PVOPS

[Thomas Gleixner]

On Tue, 21 Aug 2012, Attilio Rao wrote:
> diff --git a/arch/x86/mm/init_32.c b/arch/x86/mm/init_32.c
> index 1019156..7999cef 100644
> --- a/arch/x86/mm/init_32.c
> +++ b/arch/x86/mm/init_32.c
> @@ -445,14 +445,17 @@ static inline void permanent_kmaps_init(pgd_t *pgd_base)
>  }
>  #endif /* CONFIG_HIGHMEM */
>  
> -void __init native_pagetable_setup_start(pgd_t *base)
> +void __init native_pagetable_setup_start(void)
>  {
>  	unsigned long pfn, va;
> +	pgd_t *base;
>  	pgd_t *pgd;

	pgd_t *pgd, *base = swapper_pg_dir;

Please. No need to add 5 lines just for this.

Thanks,

	tglx

Re: [PATCH 2/5] XEN: Remove the base argument from x86_init.paging.pagetable_setup_start PVOPS

[Attilio Rao]

On 21/08/12 16:41, Thomas Gleixner wrote:
> On Tue, 21 Aug 2012, Attilio Rao wrote:
>    
>> diff --git a/arch/x86/mm/init_32.c b/arch/x86/mm/init_32.c
>> index 1019156..7999cef 100644
>> --- a/arch/x86/mm/init_32.c
>> +++ b/arch/x86/mm/init_32.c
>> @@ -445,14 +445,17 @@ static inline void permanent_kmaps_init(pgd_t *pgd_base)
>>   }
>>   #endif /* CONFIG_HIGHMEM */
>>
>> -void __init native_pagetable_setup_start(pgd_t *base)
>> +void __init native_pagetable_setup_start(void)
>>   {
>>   	unsigned long pfn, va;
>> +	pgd_t *base;
>>   	pgd_t *pgd;
>>      
> 	pgd_t *pgd, *base = swapper_pg_dir;
>
> Please. No need to add 5 lines just for this.
>
>    

I honestly thought it was cleaner -- what is exactly your preferred 
choice? Just use swapper_pg_dir directly in the 2 places needing it?

Attilio

Re: [PATCH 2/5] XEN: Remove the base argument from x86_init.paging.pagetable_setup_start PVOPS

[Thomas Gleixner]

On Tue, 21 Aug 2012, Attilio Rao wrote:
> On 21/08/12 16:41, Thomas Gleixner wrote:
> > On Tue, 21 Aug 2012, Attilio Rao wrote:
> >    
> > > diff --git a/arch/x86/mm/init_32.c b/arch/x86/mm/init_32.c
> > > index 1019156..7999cef 100644
> > > --- a/arch/x86/mm/init_32.c
> > > +++ b/arch/x86/mm/init_32.c
> > > @@ -445,14 +445,17 @@ static inline void permanent_kmaps_init(pgd_t
> > > *pgd_base)
> > >   }
> > >   #endif /* CONFIG_HIGHMEM */
> > > 
> > > -void __init native_pagetable_setup_start(pgd_t *base)
> > > +void __init native_pagetable_setup_start(void)
> > >   {
> > >   	unsigned long pfn, va;
> > > +	pgd_t *base;
> > >   	pgd_t *pgd;
> > >      
> > 	pgd_t *pgd, *base = swapper_pg_dir;
> > 
> > Please. No need to add 5 lines just for this.
> > 
> >    
> 
> I honestly thought it was cleaner -- what is exactly your preferred choice?
> Just use swapper_pg_dir directly in the 2 places needing it?

Either that or the line I wrote above.

[PATCH 3/5] X86/XEN: Introduce the x86_init.paging.pagetable_init PVOPS

[Attilio Rao]

This new PVOPS is responsible to setup the kernel pagetables and
replace entirely x86_init.paging.pagetable_setup_start and
x86_init.paging.pagetable_setup_done PVOPS work.

For performance the x86_64 stub is implemented as a macro to paging_init()
rather than an actual function stub.

Signed-off-by: Attilio Rao <attilio.rao@citrix.com>
---
 arch/x86/include/asm/pgtable_types.h |    2 +
 arch/x86/include/asm/x86_init.h      |    2 +
 arch/x86/kernel/x86_init.c           |    1 +
 arch/x86/mm/init_32.c                |   35 ++++++++++++++++++++++++++++++++++
 arch/x86/xen/mmu.c                   |   12 +++++++++-
 5 files changed, 50 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/pgtable_types.h b/arch/x86/include/asm/pgtable_types.h
index 9b1c1f7..55f24b5 100644
--- a/arch/x86/include/asm/pgtable_types.h
+++ b/arch/x86/include/asm/pgtable_types.h
@@ -303,9 +303,11 @@ void set_pte_vaddr(unsigned long vaddr, pte_t pte);
 
 extern void native_pagetable_reserve(u64 start, u64 end);
 #ifdef CONFIG_X86_32
+extern void native_pagetable_init(void);
 extern void native_pagetable_setup_start(void);
 extern void native_pagetable_setup_done(void);
 #else
+#define native_pagetable_init        paging_init
 #define native_pagetable_setup_start x86_init_pgd_noop
 #define native_pagetable_setup_done  x86_init_pgd_noop
 #endif
diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
index efd0075..a74cc19 100644
--- a/arch/x86/include/asm/x86_init.h
+++ b/arch/x86/include/asm/x86_init.h
@@ -81,10 +81,12 @@ struct x86_init_mapping {
 
 /**
  * struct x86_init_paging - platform specific paging functions
+ * @pagetable_init:		platform specific paging initialization call
  * @pagetable_setup_start:	platform specific pre paging_init() call
  * @pagetable_setup_done:	platform specific post paging_init() call
  */
 struct x86_init_paging {
+	void (*pagetable_init)(void);
 	void (*pagetable_setup_start)(void);
 	void (*pagetable_setup_done)(void);
 };
diff --git a/arch/x86/kernel/x86_init.c b/arch/x86/kernel/x86_init.c
index 849be14..c1e910a 100644
--- a/arch/x86/kernel/x86_init.c
+++ b/arch/x86/kernel/x86_init.c
@@ -68,6 +68,7 @@ struct x86_init_ops x86_init __initdata = {
 	},
 
 	.paging = {
+		.pagetable_init		= native_pagetable_init,
 		.pagetable_setup_start	= native_pagetable_setup_start,
 		.pagetable_setup_done	= native_pagetable_setup_done,
 	},
diff --git a/arch/x86/mm/init_32.c b/arch/x86/mm/init_32.c
index 7999cef..2ff4790 100644
--- a/arch/x86/mm/init_32.c
+++ b/arch/x86/mm/init_32.c
@@ -445,6 +445,41 @@ static inline void permanent_kmaps_init(pgd_t *pgd_base)
 }
 #endif /* CONFIG_HIGHMEM */
 
+void __init native_pagetable_init(void)
+{
+	unsigned long pfn, va;
+	pgd_t *base;
+	pgd_t *pgd;
+	pud_t *pud;
+	pmd_t *pmd;
+	pte_t *pte;
+
+	base = swapper_pg_dir;
+
+	/*
+	 * Remove any mappings which extend past the end of physical
+	 * memory from the boot time page table:
+	 */
+	for (pfn = max_low_pfn + 1; pfn < 1<<(32-PAGE_SHIFT); pfn++) {
+		va = PAGE_OFFSET + (pfn<<PAGE_SHIFT);
+		pgd = base + pgd_index(va);
+		if (!pgd_present(*pgd))
+			break;
+
+		pud = pud_offset(pgd, va);
+		pmd = pmd_offset(pud, va);
+		if (!pmd_present(*pmd))
+			break;
+
+		pte = pte_offset_kernel(pmd, va);
+		if (!pte_present(*pte))
+			break;
+
+		pte_clear(NULL, va, pte);
+	}
+	paravirt_alloc_pmd(&init_mm, __pa(base) >> PAGE_SHIFT);
+	paging_init();
+}
 void __init native_pagetable_setup_start(void)
 {
 	unsigned long pfn, va;
diff --git a/arch/x86/xen/mmu.c b/arch/x86/xen/mmu.c
index 04a0a8f..68466ce 100644
--- a/arch/x86/xen/mmu.c
+++ b/arch/x86/xen/mmu.c
@@ -1174,6 +1174,15 @@ static void xen_exit_mmap(struct mm_struct *mm)
 	spin_unlock(&mm->page_table_lock);
 }
 
+static void xen_post_allocator_init(void);
+
+static void __init xen_pagetable_init(void)
+{
+	paging_init();
+	xen_setup_shared_info();
+	xen_post_allocator_init();
+}
+
 static void __init xen_pagetable_setup_start(void)
 {
 }
@@ -1192,8 +1201,6 @@ static __init void xen_mapping_pagetable_reserve(u64 start, u64 end)
 	}
 }
 
-static void xen_post_allocator_init(void);
-
 static void __init xen_pagetable_setup_done(void)
 {
 	xen_setup_shared_info();
@@ -2068,6 +2075,7 @@ static const struct pv_mmu_ops xen_mmu_ops __initconst = {
 void __init xen_init_mmu_ops(void)
 {
 	x86_init.mapping.pagetable_reserve = xen_mapping_pagetable_reserve;
+	x86_init.paging.pagetable_init = xen_pagetable_init;
 	x86_init.paging.pagetable_setup_start = xen_pagetable_setup_start;
 	x86_init.paging.pagetable_setup_done = xen_pagetable_setup_done;
 	pv_mmu_ops = xen_mmu_ops;
-- 
1.7.2.5

Re: [PATCH 3/5] X86/XEN: Introduce the x86_init.paging.pagetable_init PVOPS

[Thomas Gleixner]

On Tue, 21 Aug 2012, Attilio Rao wrote:

> This new PVOPS is responsible to setup the kernel pagetables and
> replace entirely x86_init.paging.pagetable_setup_start and
> x86_init.paging.pagetable_setup_done PVOPS work.
 
> For performance the x86_64 stub is implemented as a macro to paging_init()
> rather than an actual function stub.

Huch, using a macro for an once per boot time call is really a massive
performance improvement.

It's confusing and wrong. You just use a macro because x86_64 does not
need any extra setups aside of paging_init().
 
> diff --git a/arch/x86/kernel/x86_init.c b/arch/x86/kernel/x86_init.c
> index 849be14..c1e910a 100644
> --- a/arch/x86/kernel/x86_init.c
> +++ b/arch/x86/kernel/x86_init.c
> @@ -68,6 +68,7 @@ struct x86_init_ops x86_init __initdata = {
>  	},
>  
>  	.paging = {
> +		.pagetable_init		= native_pagetable_init,

I'd prefer to see these patches implemented differently.

 #1 Remove the base argument from pagetable_setup_start (leave
    pagetable_setup_done() alone).

 #2 Rename pagetable_setup_start to pagetable_init,
    native_pagetable_setup_start to native_pagetable_init and
    xen_pagetable_setup_start to xen_pagetable_init
 
 #3 Instead of copying the whole native_pagetable_setup_start()
    function and deleting it later, move the paging_init() call from
    setup.c to native_pagetable_init() and xen_pagetable_init()
    and define native_pagetable_init as paging_init() for x86_64

 #4 Move the code from xen_pagetable_setup_done() into
    xen_pagetable_init() and remove the now unused
    pagetable_setup_done().

That's less code shuffling and pointless copying which makes the
review way easier.

Thanks,

	tglx

Re: [PATCH 3/5] X86/XEN: Introduce the x86_init.paging.pagetable_init PVOPS

[Attilio Rao]

On 21/08/12 16:44, Thomas Gleixner wrote:
> On Tue, 21 Aug 2012, Attilio Rao wrote:
>
>    
>> This new PVOPS is responsible to setup the kernel pagetables and
>> replace entirely x86_init.paging.pagetable_setup_start and
>> x86_init.paging.pagetable_setup_done PVOPS work.
>>      
>
>    
>> For performance the x86_64 stub is implemented as a macro to paging_init()
>> rather than an actual function stub.
>>      
> Huch, using a macro for an once per boot time call is really a massive
> performance improvement.
>
> It's confusing and wrong. You just use a macro because x86_64 does not
> need any extra setups aside of paging_init().
>
>    
>> diff --git a/arch/x86/kernel/x86_init.c b/arch/x86/kernel/x86_init.c
>> index 849be14..c1e910a 100644
>> --- a/arch/x86/kernel/x86_init.c
>> +++ b/arch/x86/kernel/x86_init.c
>> @@ -68,6 +68,7 @@ struct x86_init_ops x86_init __initdata = {
>>   	},
>>
>>   	.paging = {
>> +		.pagetable_init		= native_pagetable_init,
>>      
> I'd prefer to see these patches implemented differently.
>
>   #1 Remove the base argument from pagetable_setup_start (leave
>      pagetable_setup_done() alone).
>
>   #2 Rename pagetable_setup_start to pagetable_init,
>      native_pagetable_setup_start to native_pagetable_init and
>      xen_pagetable_setup_start to xen_pagetable_init
>
>   #3 Instead of copying the whole native_pagetable_setup_start()
>      function and deleting it later, move the paging_init() call from
>      setup.c to native_pagetable_init() and xen_pagetable_init()
>      and define native_pagetable_init as paging_init() for x86_64
>
>   #4 Move the code from xen_pagetable_setup_done() into
>      xen_pagetable_init() and remove the now unused
>      pagetable_setup_done().
>
> That's less code shuffling and pointless copying which makes the
> review way easier.
>    

I've followed these steps in a new patch series (integrating suggestions 
from Konrad and Stefano too).

Attilio

[PATCH 4/5] X86/XEN: Retire now unused x86_init.paging.pagetable_setup_start and x86_init.paging.pagetable_setup_done PVOPS

[Attilio Rao]

Now that x86_init.paging.pagetable_init PVOPS is implemented just
retire the 2 mentioned above and replace their usage with it.

Signed-off-by: Attilio Rao <attilio.rao@citrix.com>
---
 arch/x86/include/asm/pgtable_types.h |    4 ---
 arch/x86/include/asm/x86_init.h      |    4 ---
 arch/x86/kernel/setup.c              |    4 +--
 arch/x86/kernel/x86_init.c           |    3 --
 arch/x86/mm/init_32.c                |   40 +---------------------------------
 arch/x86/xen/mmu.c                   |   12 ----------
 6 files changed, 2 insertions(+), 65 deletions(-)

diff --git a/arch/x86/include/asm/pgtable_types.h b/arch/x86/include/asm/pgtable_types.h
index 55f24b5..db8fec6 100644
--- a/arch/x86/include/asm/pgtable_types.h
+++ b/arch/x86/include/asm/pgtable_types.h
@@ -304,12 +304,8 @@ void set_pte_vaddr(unsigned long vaddr, pte_t pte);
 extern void native_pagetable_reserve(u64 start, u64 end);
 #ifdef CONFIG_X86_32
 extern void native_pagetable_init(void);
-extern void native_pagetable_setup_start(void);
-extern void native_pagetable_setup_done(void);
 #else
 #define native_pagetable_init        paging_init
-#define native_pagetable_setup_start x86_init_pgd_noop
-#define native_pagetable_setup_done  x86_init_pgd_noop
 #endif
 
 struct seq_file;
diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
index a74cc19..995ea5c 100644
--- a/arch/x86/include/asm/x86_init.h
+++ b/arch/x86/include/asm/x86_init.h
@@ -82,13 +82,9 @@ struct x86_init_mapping {
 /**
  * struct x86_init_paging - platform specific paging functions
  * @pagetable_init:		platform specific paging initialization call
- * @pagetable_setup_start:	platform specific pre paging_init() call
- * @pagetable_setup_done:	platform specific post paging_init() call
  */
 struct x86_init_paging {
 	void (*pagetable_init)(void);
-	void (*pagetable_setup_start)(void);
-	void (*pagetable_setup_done)(void);
 };
 
 /**
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index d3d8f00..4f16547 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -961,9 +961,7 @@ void __init setup_arch(char **cmdline_p)
 	kvmclock_init();
 #endif
 
-	x86_init.paging.pagetable_setup_start();
-	paging_init();
-	x86_init.paging.pagetable_setup_done();
+	x86_init.paging.pagetable_init();
 
 	if (boot_cpu_data.cpuid_level >= 0) {
 		/* A CPU has %cr4 if and only if it has CPUID */
diff --git a/arch/x86/kernel/x86_init.c b/arch/x86/kernel/x86_init.c
index c1e910a..7a3d075 100644
--- a/arch/x86/kernel/x86_init.c
+++ b/arch/x86/kernel/x86_init.c
@@ -26,7 +26,6 @@
 
 void __cpuinit x86_init_noop(void) { }
 void __init x86_init_uint_noop(unsigned int unused) { }
-void __init x86_init_pgd_noop(void) { }
 int __init iommu_init_noop(void) { return 0; }
 void iommu_shutdown_noop(void) { }
 
@@ -69,8 +68,6 @@ struct x86_init_ops x86_init __initdata = {
 
 	.paging = {
 		.pagetable_init		= native_pagetable_init,
-		.pagetable_setup_start	= native_pagetable_setup_start,
-		.pagetable_setup_done	= native_pagetable_setup_done,
 	},
 
 	.timers = {
diff --git a/arch/x86/mm/init_32.c b/arch/x86/mm/init_32.c
index 2ff4790..f8771ad 100644
--- a/arch/x86/mm/init_32.c
+++ b/arch/x86/mm/init_32.c
@@ -480,44 +480,6 @@ void __init native_pagetable_init(void)
 	paravirt_alloc_pmd(&init_mm, __pa(base) >> PAGE_SHIFT);
 	paging_init();
 }
-void __init native_pagetable_setup_start(void)
-{
-	unsigned long pfn, va;
-	pgd_t *base;
-	pgd_t *pgd;
-	pud_t *pud;
-	pmd_t *pmd;
-	pte_t *pte;
-
-	base = swapper_pg_dir;
-
-	/*
-	 * Remove any mappings which extend past the end of physical
-	 * memory from the boot time page table:
-	 */
-	for (pfn = max_low_pfn + 1; pfn < 1<<(32-PAGE_SHIFT); pfn++) {
-		va = PAGE_OFFSET + (pfn<<PAGE_SHIFT);
-		pgd = base + pgd_index(va);
-		if (!pgd_present(*pgd))
-			break;
-
-		pud = pud_offset(pgd, va);
-		pmd = pmd_offset(pud, va);
-		if (!pmd_present(*pmd))
-			break;
-
-		pte = pte_offset_kernel(pmd, va);
-		if (!pte_present(*pte))
-			break;
-
-		pte_clear(NULL, va, pte);
-	}
-	paravirt_alloc_pmd(&init_mm, __pa(base) >> PAGE_SHIFT);
-}
-
-void __init native_pagetable_setup_done(void)
-{
-}
 
 /*
  * Build a proper pagetable for the kernel mappings.  Up until this
@@ -531,7 +493,7 @@ void __init native_pagetable_setup_done(void)
  * If we're booting paravirtualized under a hypervisor, then there are
  * more options: we may already be running PAE, and the pagetable may
  * or may not be based in swapper_pg_dir.  In any case,
- * paravirt_pagetable_setup_start() will set up swapper_pg_dir
+ * paravirt_pagetable_init() will set up swapper_pg_dir
  * appropriately for the rest of the initialization to work.
  *
  * In general, pagetable_init() assumes that the pagetable may already
diff --git a/arch/x86/xen/mmu.c b/arch/x86/xen/mmu.c
index 68466ce..4290d83 100644
--- a/arch/x86/xen/mmu.c
+++ b/arch/x86/xen/mmu.c
@@ -1183,10 +1183,6 @@ static void __init xen_pagetable_init(void)
 	xen_post_allocator_init();
 }
 
-static void __init xen_pagetable_setup_start(void)
-{
-}
-
 static __init void xen_mapping_pagetable_reserve(u64 start, u64 end)
 {
 	/* reserve the range used */
@@ -1201,12 +1197,6 @@ static __init void xen_mapping_pagetable_reserve(u64 start, u64 end)
 	}
 }
 
-static void __init xen_pagetable_setup_done(void)
-{
-	xen_setup_shared_info();
-	xen_post_allocator_init();
-}
-
 static void xen_write_cr2(unsigned long cr2)
 {
 	this_cpu_read(xen_vcpu)->arch.cr2 = cr2;
@@ -2076,8 +2066,6 @@ void __init xen_init_mmu_ops(void)
 {
 	x86_init.mapping.pagetable_reserve = xen_mapping_pagetable_reserve;
 	x86_init.paging.pagetable_init = xen_pagetable_init;
-	x86_init.paging.pagetable_setup_start = xen_pagetable_setup_start;
-	x86_init.paging.pagetable_setup_done = xen_pagetable_setup_done;
 	pv_mmu_ops = xen_mmu_ops;
 
 	memset(dummy_mapping, 0xff, PAGE_SIZE);
-- 
1.7.2.5

[PATCH 5/5] X86/XEN: Add few lines explaining simple semantic for x86_init.paging.pagetable_init PVOPS

[Attilio Rao]

- Explain the purpose of the PVOPS
- Report execution constraints

Signed-off-by: Attilio Rao <attilio.rao@citrix.com>
---
 arch/x86/include/asm/x86_init.h |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
index 995ea5c..7ea4186 100644
--- a/arch/x86/include/asm/x86_init.h
+++ b/arch/x86/include/asm/x86_init.h
@@ -82,6 +82,11 @@ struct x86_init_mapping {
 /**
  * struct x86_init_paging - platform specific paging functions
  * @pagetable_init:		platform specific paging initialization call
+ *
+ * It does setup the kernel pagetables and prepares accessors functions to
+ * manipulate them.
+ * It must be called once, during the boot sequence and after the direct
+ * mapping for phys memory is setup.
  */
 struct x86_init_paging {
 	void (*pagetable_init)(void);
-- 
1.7.2.5

Re: [PATCH 0/5] X86/XEN: Merge x86_init.paging.pagetable_setup_start and x86_init.paging.pagetable_setup_done PVOPS and document the semantic

[Stefano Stabellini]

On Tue, 21 Aug 2012, Attilio Rao wrote:
> Currently the definition of x86_init.paging.pagetable_setup_start and
> x86_init.paging.pagetable_setup_done is twisted and not really well
> defined (in terms of prototypes desired). More specifically:
> pagetable_setup_start:
>  * it is a nop on x86_32
>  * it is a nop for the XEN case
>  * cleans up the boot time page table in the x86_64 case

the description is wrong: you swapped x86_32 and x86_64 here


> pagetable_setup_done:
>  * it is a nop on x86_32
>  * sets up accessor functions for pagetable manipulation, for the
>    XEN case
>  * it is a nop on x86_64
> 
> Most of this logic can be skipped by creating a new PVOPS that can handle
> pagetable setup and pre/post operations on it.
> The new PVOPS must be called only once, during boot-time setup and
> after the direct mapping for physical memory is available.
> 
> Attilio Rao (5):
>   XEN: Remove the base argument from
>     x86_init.paging.pagetable_setup_done PVOPS
>   XEN: Remove the base argument from
>     x86_init.paging.pagetable_setup_start PVOPS
>   X86/XEN: Introduce the x86_init.paging.pagetable_init() PVOPS
>   X86/XEN: Retire now unused x86_init.paging.pagetable_setup_start and
>     x86_init.paging.pagetable_setup_done PVOPS
>   X86/XEN: Add few lines explaining simple semantic for
>     x86_init.paging.pagetable_init PVOPS
> 
>  arch/x86/include/asm/pgtable_types.h |    6 ++----
>  arch/x86/include/asm/x86_init.h      |   11 +++++++----
>  arch/x86/kernel/setup.c              |    4 +---
>  arch/x86/kernel/x86_init.c           |    4 +---
>  arch/x86/mm/init_32.c                |   12 ++++++------
>  arch/x86/xen/mmu.c                   |   18 +++++++-----------
>  6 files changed, 24 insertions(+), 31 deletions(-)
> 
> -- 
> 1.7.2.5
> 

Re: [PATCH 0/5] X86/XEN: Merge x86_init.paging.pagetable_setup_start and x86_init.paging.pagetable_setup_done PVOPS and document the semantic

[Konrad Rzeszutek Wilk]

On Tue, Aug 21, 2012 at 02:14:01AM +0100, Attilio Rao wrote:
> Currently the definition of x86_init.paging.pagetable_setup_start and
> x86_init.paging.pagetable_setup_done is twisted and not really well
> defined (in terms of prototypes desired). More specifically:
> pagetable_setup_start:
>  * it is a nop on x86_32
>  * it is a nop for the XEN case
>  * cleans up the boot time page table in the x86_64 case

Is it safe to call that 'boot time page table' in Xen case?
Since that is what it would be doing? Did you test it with dom0 and
PV guest and with 2GB, 3GB, 4GB, and 8GB layouts? I think those were
the ones that caught earlier mistakes.
> 
> pagetable_setup_done:
>  * it is a nop on x86_32
>  * sets up accessor functions for pagetable manipulation, for the
>    XEN case
>  * it is a nop on x86_64
> 
> Most of this logic can be skipped by creating a new PVOPS that can handle
> pagetable setup and pre/post operations on it.
> The new PVOPS must be called only once, during boot-time setup and
> after the direct mapping for physical memory is available.

Looks like you are missing the other crucial bit of information:
It removes two of the pvops and replaces them with just one.

> 
> Attilio Rao (5):
>   XEN: Remove the base argument from
>     x86_init.paging.pagetable_setup_done PVOPS
>   XEN: Remove the base argument from
>     x86_init.paging.pagetable_setup_start PVOPS
>   X86/XEN: Introduce the x86_init.paging.pagetable_init() PVOPS
>   X86/XEN: Retire now unused x86_init.paging.pagetable_setup_start and
>     x86_init.paging.pagetable_setup_done PVOPS
>   X86/XEN: Add few lines explaining simple semantic for
>     x86_init.paging.pagetable_init PVOPS
> 
>  arch/x86/include/asm/pgtable_types.h |    6 ++----
>  arch/x86/include/asm/x86_init.h      |   11 +++++++----
>  arch/x86/kernel/setup.c              |    4 +---
>  arch/x86/kernel/x86_init.c           |    4 +---
>  arch/x86/mm/init_32.c                |   12 ++++++------
>  arch/x86/xen/mmu.c                   |   18 +++++++-----------
>  6 files changed, 24 insertions(+), 31 deletions(-)
> 
> -- 
> 1.7.2.5

Re: [PATCH 0/5] X86/XEN: Merge x86_init.paging.pagetable_setup_start and x86_init.paging.pagetable_setup_done PVOPS and document the semantic

[Thomas Gleixner]

On Tue, 21 Aug 2012, Attilio Rao wrote:

> Currently the definition of x86_init.paging.pagetable_setup_start and
> x86_init.paging.pagetable_setup_done is twisted and not really well
> defined (in terms of prototypes desired). More specifically:
> pagetable_setup_start:
>  * it is a nop on x86_32
>  * it is a nop for the XEN case
>  * cleans up the boot time page table in the x86_64 case
> 
> pagetable_setup_done:
>  * it is a nop on x86_32
>  * sets up accessor functions for pagetable manipulation, for the
>    XEN case
>  * it is a nop on x86_64
> 
> Most of this logic can be skipped by creating a new PVOPS that can handle
> pagetable setup and pre/post operations on it.
> The new PVOPS must be called only once, during boot-time setup and
> after the direct mapping for physical memory is available.

Can you please refrain from naming that PVOPS? The setup function
pointers have nothing to do with PVOPS.

They are explicitely meant for platforms and XEN is just another
platform as is 32bit and 64bit.

Thanks,

	tglx